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
