Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220902131258.3316367-1-cmo@melexis.com>
In-Reply-To: <20220902131258.3316367-1-cmo@melexis.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Fri, 2 Sep 2022 18:27:46 +0300
Message-ID: <CAHp75VdOHDHUVhVXj4L-6ZV25mTWTeO3s3EJQVgLxknXHKRUMg@mail.gmail.com>
Subject: Re: [PATCH 1/3] iio: temperature: mlx90632 Add runtime
 powermanagement modes
To:     Crt Mori <cmo@melexis.com>
Cc:     Jonathan Cameron <jic23@kernel.org>,
        linux-iio <linux-iio@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Sep 2, 2022 at 4:13 PM <cmo@melexis.com> wrote:
>
> From: Crt Mori <cmo@melexis.com>
>
> Sensor can operate in lower power modes and even make measurements when

The sensor
...or..
Sensors

> in those lower powered modes. The decision was taken that if measurement
> is not requested within 2 seconds the sensor will remain in SLEEP_STEP
> power mode, where measurements are triggered on request with setting the
> start of measurement bit (SOB). In this mode the measurements are taking
> a bit longer because we need to start it and complete it. Currently, in
> continuous mode we read ready data and this mode is activated if sensor
> measurement is requested within 2 seconds. The suspend timeout is
> increased to 6 seconds (instead of 3 before), because that enables more
> measurements in lower power mode (SLEEP_STEP), with the lowest refresh
> rate (2 seconds).

...

>  #define MLX90632_PWR_STATUS_CONTINUOUS MLX90632_PWR_STATUS(3) /* continuous*/
>
> +#define MLX90632_EE_RR(ee_val) (ee_val & GENMASK(10, 8)) /* Only Refresh Rate bits*/

Missed space. Seems like a copy'n'paste from previous comments that
lacks the space as well.

...

> +       unsigned long interraction_timestamp; /* in jiffies */

_ts for timestamp is a fine abbreviation. Also move comment to the kernel doc.

...

> +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)

>  static s32 mlx90632_pwr_set_sleep_step(struct regmap *regmap)
>  {

> +       struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(regmap_get_device(regmap)));

Why ping-ponging here and not using dev_get_drvdata()? Ditto for similar cases.

> +       struct mlx90632_data *data = iio_priv(indio_dev);
> +       s32 ret = 0;

Assignment is not needed, use 'return 0;' directly. Ditto for all
cases like this.

> +       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;
>  }

...

> +       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?

> +       return MLX90632_MEAS_MAX_TIME >> reg;

...

> +               refresh_time = refresh_time + ret;

+= ?

...

> +               refresh_time = refresh_time + ret;

+=

...

> +               refresh_time = refresh_time + ret;

Ditto.

...

> +       unsigned int reg_status;
>         int ret;

Keep the reversed xmas tree order (like here!) elsewhere for the sake
of consistency.

...

> +       ret = regmap_read_poll_timeout(data->regmap, MLX90632_REG_STATUS,
> +                                      reg_status,
> +                                      ((reg_status & MLX90632_STAT_BUSY) == 0),

Too many parentheses

> +                                      10000, 100 * 10000);
> +       if (ret < 0) {
> +               dev_err(&data->client->dev, "data not ready");
> +               return -ETIMEDOUT;
> +       }

...

> +       int current_powerstatus = data->powerstatus;

Please, split the assignment and move it closer to the first user.

...

> +       data->powerstatus = MLX90632_PWR_STATUS_HALT;
> +
> +       if (current_powerstatus == MLX90632_PWR_STATUS_SLEEP_STEP)
> +               return mlx90632_pwr_set_sleep_step(data->regmap);

> +       else

Redundant.

> +               return mlx90632_pwr_continuous(data->regmap);

...

> +               ret = read_poll_timeout(mlx90632_perform_measurement, meas, meas == 19,
> +                                       50000, 800000, false, data);
> +               if (ret != 0)

Drop this ' != 0' part. It's useless.

> +                       goto read_unlock;

> +

Seems redundant blank line.

...

> +       }
> +
>

Ditto.

...

> +       int ret = 0;

Redundant assignment. Use return 0; directly.

...

> +       if (time_in_range(now, data->interraction_timestamp,
> +                         data->interraction_timestamp +

> +                         msecs_to_jiffies(MLX90632_MEAS_MAX_TIME + 100))) {

With a local variable you will have better to read code.

> +       }


...

>         struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));

Maybe a separate patch to drop these here-there dereferences...

...

> +static int __maybe_unused mlx90632_pm_runtime_suspend(struct device *dev)

No __maybe_unused, use pm_ptr() / pm_sleep_ptr() below.

> +{
> +       struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +       struct mlx90632_data *data = iio_priv(indio_dev);
> +
> +       return mlx90632_pwr_set_sleep_step(data->regmap);
> +}
> +
> +const struct dev_pm_ops mlx90632_pm_ops = {
> +       SET_SYSTEM_SLEEP_PM_OPS(mlx90632_pm_suspend, mlx90632_pm_resume)
> +       SET_RUNTIME_PM_OPS(mlx90632_pm_runtime_suspend,
> +                          NULL, NULL)

Please, use new macros from pm.h / runtime_pm.h

> +};
> +EXPORT_SYMBOL_GPL(mlx90632_pm_ops);

Can we use special EXPORT macro from pm.h

-- 
With Best Regards,
Andy Shevchenko