Messages in this thread | | | From | Andy Shevchenko <> | Date | Fri, 2 Sep 2022 18:27:46 +0300 | Subject | Re: [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
| |