Re: [PATCH 1/3] iio: temperature: mlx90632 Add runtime powermanagement modes

[Andy Shevchenko]

On Fri, Sep 2, 2022 at 8:59 PM Crt Mori <cmo@xxxxxxxxxxx> wrote:
> On Fri, 2 Sept 2022 at 17:28, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> > On Fri, Sep 2, 2022 at 4:13 PM <cmo@xxxxxxxxxxx> wrote:

For future replies, please remove unnecessary context when replying
(I'm wasting my time for that)!

...

> > > +static int mlx90632_wakeup(struct mlx90632_data *data);
> >
> > Can we avoid forward declaration? (I don't even see how it is used
> > among dozens of lines of below code in the patch)
> >
> This is existing function that I did not want to move upwards. Should
> I have just moved it rather?

Yes, move it in the preparatory (separate) patch.

...

> > > +       s32 ret = 0;
> >
> > Assignment is not needed, use 'return 0;' directly. Ditto for all
> > cases like this.
> >
> This is used, because when powerstatus is not equal to sleep_step it
> returns, otherwise the ret is changed in the function.

Please, read what I have suggested.

> > > +       if (data->powerstatus != MLX90632_PWR_STATUS_SLEEP_STEP) {
> > > +               ret = regmap_write_bits(regmap, MLX90632_REG_CONTROL,
> > > +                                       MLX90632_CFG_PWR_MASK,
> > > +                                       MLX90632_PWR_STATUS_SLEEP_STEP);
> > > +               if (ret < 0)
> > > +                       return ret;
> > > +
> > > +               data->powerstatus = MLX90632_PWR_STATUS_SLEEP_STEP;
> > > +       }
> > > +       return ret;

For your convenience...

    return 0;

...here.

Or do you state that ret can be a positive number? In such cases,
elaboration is required.

...

> > > +       reg = MLX90632_EE_RR(reg) >> 8;
> >
> > This makes it harder to understand the semantics of reg, can we simply
> > unite this line with the below?
> >
> I find it easier to have it split but I can make one long statement.
> > > +       return MLX90632_MEAS_MAX_TIME >> reg;

If so, you need another variable with better naming to show what is in it.

...

> > > +       ret = regmap_read_poll_timeout(data->regmap, MLX90632_REG_STATUS,
> > > +                                      reg_status,
> > > +                                      ((reg_status & MLX90632_STAT_BUSY) == 0),
> >
> > Too many parentheses
> >
> I like the outer parentheses it shows that it is a break condition. I
> have same in another function in this file.

It's not a Lisp, we don't need to pollute code with unneeded obstacles.

> > > +                                      10000, 100 * 10000);

...

> > > +       if (current_powerstatus == MLX90632_PWR_STATUS_SLEEP_STEP)
> > > +               return mlx90632_pwr_set_sleep_step(data->regmap);
> >
> > > +       else
> >
> > Redundant.
> >
> No, the powermode changes among the type.

Yes. 'else' keyword is always redundant in the

  if (...)
    return / break / continue / goto
  else

cases.

> > > +               return mlx90632_pwr_continuous(data->regmap);

...

> > > +static int __maybe_unused mlx90632_pm_runtime_suspend(struct device *dev)
> >
> > No __maybe_unused, use pm_ptr() / pm_sleep_ptr() below.
> >
> Care to explain a bit more about this? I just followed what other
> drivers have...

And other drivers have what I said, but it's a new feature.
If you run `git log --no-merges --grep 'pm_ptr' -- drivers/iio
include/linux/` and read the history it will explain the case.

-- 
With Best Regards,
Andy Shevchenko