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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75VfJMpf3GeOryt9cH6-tK48BB8ZcfuGxhXovObBanDcc7w@mail.gmail.com>
Date:   Fri, 2 Sep 2022 21:38:23 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Crt Mori <cmo@...exis.com>
Cc:     Jonathan Cameron <jic23@...nel.org>,
        linux-iio <linux-iio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/3] iio: temperature: mlx90632 Add runtime
 powermanagement modes

On Fri, Sep 2, 2022 at 8:59 PM Crt Mori <cmo@...exis.com> wrote:
> On Fri, 2 Sept 2022 at 17:28, Andy Shevchenko <andy.shevchenko@...il.com> wrote:
> > On Fri, Sep 2, 2022 at 4:13 PM <cmo@...exis.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