Messages in this thread | | | From | Andy Shevchenko <> | Date | Fri, 2 Sep 2022 21:38:23 +0300 | Subject | Re: [PATCH 1/3] iio: temperature: mlx90632 Add runtime powermanagement modes |
| |
On Fri, Sep 2, 2022 at 8:59 PM Crt Mori <cmo@melexis.com> wrote: > On Fri, 2 Sept 2022 at 17:28, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Sep 2, 2022 at 4:13 PM <cmo@melexis.com> 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
| |