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

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