lkml.org 
[lkml]   [2022]   [Sep]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [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

    \
     
     \ /
      Last update: 2022-09-02 20:41    [W:2.805 / U:0.816 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site