Re: [PATCH v2 1/3] iio: temperature: mlx90632 Add runtime powermanagement modes

From: Crt Mori
Date: Sun Sep 04 2022 - 17:37:40 EST

On Sun, 4 Sept 2022 at 17:13, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Sun, 4 Sep 2022 00:23:36 +0200
> cmo@xxxxxxxxxxx wrote:
>
> > From: Crt Mori <cmo@xxxxxxxxxxx>
> >
> > The sensor can operate in lower power modes and even make measurements when
> > 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).
> >
> > Signed-off-by: Crt Mori <cmo@xxxxxxxxxxx>
>
> Hi Crt,
>
> If you can fix the patch threading (patches should be replies to the cover letter)
> that would be great.
Can you help me with a bit more details? I use simple git send-mail
and I added a sendmail.from option in gitconfig as you complained that
patches have no from, but I still send patch per patch. How do I make
it as reply to cover letter?

>
> The approach you've taken here looks good to me. A few
> comments inline on the implementation + requests for future possible
> cleanup of the driver.
>
> Thanks,
>
> Jonathan
>
> > ---
> > drivers/iio/temperature/mlx90632.c | 315 ++++++++++++++++++++++++-----
> > 1 file changed, 269 insertions(+), 46 deletions(-)
> >
> > diff --git a/drivers/iio/temperature/mlx90632.c b/drivers/iio/temperature/mlx90632.c
> > index 549c0ab5c2be..9acd819c76a6 100644
> > --- a/drivers/iio/temperature/mlx90632.c
> > +++ b/drivers/iio/temperature/mlx90632.c
> > @@ -7,10 +7,12 @@
> > * Driver for the Melexis MLX90632 I2C 16-bit IR thermopile sensor
> > */
> > #include <linux/delay.h>
> > +#include <linux/device.h>
> > #include <linux/err.h>
> > #include <linux/gpio/consumer.h>
> > #include <linux/i2c.h>
> > #include <linux/iopoll.h>
> > +#include <linux/jiffies.h>
> > #include <linux/kernel.h>
> > #include <linux/limits.h>
> > #include <linux/mod_devicetable.h>
> > @@ -55,6 +57,12 @@
> > #define MLX90632_EE_Ha 0x2481 /* Ha customer calib value reg 16bit */
> > #define MLX90632_EE_Hb 0x2482 /* Hb customer calib value reg 16bit */
> >
> > +#define MLX90632_EE_MEDICAL_MEAS1 0x24E1 /* Medical measurement 1 16bit */
> > +#define MLX90632_EE_MEDICAL_MEAS2 0x24E2 /* Medical measurement 2 16bit */
> > +#define MLX90632_EE_EXTENDED_MEAS1 0x24F1 /* Extended measurement 1 16bit */
> > +#define MLX90632_EE_EXTENDED_MEAS2 0x24F2 /* Extended measurement 2 16bit */
> > +#define MLX90632_EE_EXTENDED_MEAS3 0x24F3 /* Extended measurement 3 16bit */
> > +
> > /* Register addresses - volatile */
> > #define MLX90632_REG_I2C_ADDR 0x3000 /* Chip I2C address register */
> >
> > @@ -62,13 +70,16 @@
> > #define MLX90632_REG_CONTROL 0x3001 /* Control Register address */
> > #define MLX90632_CFG_PWR_MASK GENMASK(2, 1) /* PowerMode Mask */
> > #define MLX90632_CFG_MTYP_MASK GENMASK(8, 4) /* Meas select Mask */
> > +#define MLX90632_CFG_SOB_MASK BIT(11)
> >
> > /* PowerModes statuses */
> > #define MLX90632_PWR_STATUS(ctrl_val) (ctrl_val << 1)
> As noted below. Using masks and FIELD_GET() / FIELD_PREP() will make the
> driver a little bit easier to ready. Not necessary to make that part of this
> series though unless you want to.
>
> > #define MLX90632_PWR_STATUS_HALT MLX90632_PWR_STATUS(0) /* hold */
> > -#define MLX90632_PWR_STATUS_SLEEP_STEP MLX90632_PWR_STATUS(1) /* sleep step*/
> > +#define MLX90632_PWR_STATUS_SLEEP_STEP MLX90632_PWR_STATUS(1) /* sleep step */
> > #define MLX90632_PWR_STATUS_STEP MLX90632_PWR_STATUS(2) /* step */
> > -#define MLX90632_PWR_STATUS_CONTINUOUS MLX90632_PWR_STATUS(3) /* continuous*/
> > +#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 */
> >
> > /* Measurement types */
> > #define MLX90632_MTYP_MEDICAL 0
> > @@ -116,8 +127,9 @@
> > #define MLX90632_REF_12 12LL /* ResCtrlRef value of Ch 1 or Ch 2 */
> > #define MLX90632_REF_3 12LL /* ResCtrlRef value of Channel 3 */
> > #define MLX90632_MAX_MEAS_NUM 31 /* Maximum measurements in list */
> > -#define MLX90632_SLEEP_DELAY_MS 3000 /* Autosleep delay */
> > +#define MLX90632_SLEEP_DELAY_MS 6000 /* Autosleep delay */
> > #define MLX90632_EXTENDED_LIMIT 27000 /* Extended mode raw value limit */
> > +#define MLX90632_MEAS_MAX_TIME 2000 /* Max measurement time in ms for the lowest refresh rate */
> >
> > /**
> > * struct mlx90632_data - private data for the MLX90632 device
> > @@ -130,6 +142,9 @@
> > * @object_ambient_temperature: Ambient temperature at object (might differ of
> > * the ambient temperature of sensor.
> > * @regulator: Regulator of the device
> > + * @powerstatus: Current POWER status of the device
> > + * @interraction_ts: Timestamp of the last temperature read that is used
>
> interaction_ts (one r)
>
> > + * for power management
> > */
> > struct mlx90632_data {
> > struct i2c_client *client;
> > @@ -139,6 +154,8 @@ struct mlx90632_data {
> > u8 mtyp;
> > u32 object_ambient_temperature;
> > struct regulator *regulator;
> > + int powerstatus;
> > + unsigned long interraction_ts; /* in jiffies */
>
> I'd just add the 'in jiffies' note to the kernel doc above. Might well
> get missed down here.
>
> > };
> >
> > static const struct regmap_range mlx90632_volatile_reg_range[] = {
> > @@ -158,6 +175,8 @@ static const struct regmap_range mlx90632_read_reg_range[] = {
> > regmap_reg_range(MLX90632_EE_VERSION, MLX90632_EE_Ka),
> > regmap_reg_range(MLX90632_EE_CTRL, MLX90632_EE_I2C_ADDR),
> > regmap_reg_range(MLX90632_EE_Ha, MLX90632_EE_Hb),
> > + regmap_reg_range(MLX90632_EE_MEDICAL_MEAS1, MLX90632_EE_MEDICAL_MEAS2),
> > + regmap_reg_range(MLX90632_EE_EXTENDED_MEAS1, MLX90632_EE_EXTENDED_MEAS3),
> > regmap_reg_range(MLX90632_REG_I2C_ADDR, MLX90632_REG_CONTROL),
> > regmap_reg_range(MLX90632_REG_I2C_CMD, MLX90632_REG_I2C_CMD),
> > regmap_reg_range(MLX90632_REG_STATUS, MLX90632_REG_STATUS),
> > @@ -198,16 +217,38 @@ static const struct regmap_config mlx90632_regmap = {
> >
> > static s32 mlx90632_pwr_set_sleep_step(struct regmap *regmap)
> > {
> > - return regmap_update_bits(regmap, MLX90632_REG_CONTROL,
> > - MLX90632_CFG_PWR_MASK,
> > - MLX90632_PWR_STATUS_SLEEP_STEP);
> > + struct mlx90632_data *data =
> > + iio_priv(dev_get_drvdata(regmap_get_device(regmap)));
> > + s32 ret;
> > +
> > + if (data->powerstatus != MLX90632_PWR_STATUS_SLEEP_STEP) {
>
> As below, flip the condition to help readability a little.
>
> > + 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 0;
> > }
> >
> > static s32 mlx90632_pwr_continuous(struct regmap *regmap)
> > {
> > - return regmap_update_bits(regmap, MLX90632_REG_CONTROL,
> > - MLX90632_CFG_PWR_MASK,
> > - MLX90632_PWR_STATUS_CONTINUOUS);
> > + struct mlx90632_data *data =
> > + iio_priv(dev_get_drvdata(regmap_get_device(regmap)));
> > + s32 ret;
> > +
> > + if (data->powerstatus != MLX90632_PWR_STATUS_CONTINUOUS) {
> I would prefer you flip this to reduce indent.
>
> if (data->powerstatus == MLX90632_PWR_STATUS_CONTINUOUS)
> return 0;
>
> ret = regmap....
>
> a line more as you need to duplicate the return, but more readable code.
>
> Same for other similar cases.
>
> > + ret = regmap_write_bits(regmap, MLX90632_REG_CONTROL,
> > + MLX90632_CFG_PWR_MASK,
> > + MLX90632_PWR_STATUS_CONTINUOUS);
> > + if (ret < 0)
> > + return ret;
> > +
> > + data->powerstatus = MLX90632_PWR_STATUS_CONTINUOUS;
> > + }
> > + return 0;
> > }
> >
> > /**
> > @@ -219,6 +260,63 @@ static void mlx90632_reset_delay(void)
> > usleep_range(150, 200);
> > }
> >
> > +static int mlx90632_get_measurement_time(struct regmap *regmap, u16 meas)
> > +{
> > + unsigned int reg;
> > + int ret;
> > +
> > + ret = regmap_read(regmap, meas, &reg);
> > + if (ret < 0)
> > + return ret;
> > +
> > + return MLX90632_MEAS_MAX_TIME >> (MLX90632_EE_RR(reg) >> 8);
>
> Prefer FIELD_GET() and mask definition for extracting the field.
> Obviously driver isn't doing that for any other fields though so I
> guess that could be a nice cleanup follow up patch
> (though only a couple of relevant places form a quick grep for >>)
>
> > +}
> > +
>
>
> > +static int mlx90632_set_meas_type(struct mlx90632_data *data, u8 type)
> > +{
> > + int current_powerstatus;
> > + int ret;
> > +
> > + if (data->mtyp == type)
> > + return 0;
> > +
> > + current_powerstatus = data->powerstatus;
> > + ret = mlx90632_pwr_continuous(data->regmap);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = regmap_write(data->regmap, MLX90632_REG_I2C_CMD, MLX90632_RESET_CMD);
> > if (ret < 0)
> > return ret;
> >
> > mlx90632_reset_delay();
> >
> > - ret = regmap_write_bits(regmap, MLX90632_REG_CONTROL,
> > + ret = regmap_update_bits(data->regmap, MLX90632_REG_CONTROL,
> > (MLX90632_CFG_MTYP_MASK | MLX90632_CFG_PWR_MASK),
> > (MLX90632_MTYP_STATUS(type) | MLX90632_PWR_STATUS_HALT));
> > if (ret < 0)
> > return ret;
> >
> > - return mlx90632_pwr_continuous(regmap);
> > + data->mtyp = type;
> > + data->powerstatus = MLX90632_PWR_STATUS_HALT;
> > +
> > + if (current_powerstatus == MLX90632_PWR_STATUS_SLEEP_STEP)
> > + return mlx90632_pwr_set_sleep_step(data->regmap);
> I'd prefer this as a switch statement to make it clear there are only
> two valid options and we want to match one of them.
>
> Makes for a clearer relationship to the state machine you have
> for this stuff. At the cost of a few more lines of code.
>
> At the moment you vary between explicitly checking each option and
> only checking one of them. If there are two options, a switch with
> default error path makes that clear.

In reality, it is 1 option or a continuous mode. So it would be a
really plain switch as the default option would be the continuous
below.

> > +
> > + return mlx90632_pwr_continuous(data->regmap);
> > }
> >
> > static int mlx90632_channel_new_select(int perform_ret, uint8_t *channel_new,
> > @@ -355,11 +503,24 @@ static int mlx90632_read_all_channel(struct mlx90632_data *data,
> > s32 ret, measurement;
> >
> > mutex_lock(&data->lock);
> > - measurement = mlx90632_perform_measurement(data);
> > - if (measurement < 0) {
> > - ret = measurement;
> > + ret = mlx90632_set_meas_type(data, MLX90632_MTYP_MEDICAL);
> > + if (ret < 0)
> > goto read_unlock;
> > +
> > + if (data->powerstatus == MLX90632_PWR_STATUS_CONTINUOUS) {
> > + measurement = mlx90632_perform_measurement(data);
> > + if (measurement < 0) {
> > + ret = measurement;
> > + goto read_unlock;
> > + }
> > + } else if (data->powerstatus == MLX90632_PWR_STATUS_SLEEP_STEP) {
> Can power status be anything else?
It can be (there is halt mode and step mode), but not with current
implementation of the driver. But I see where this is going, so
instead of reporting and error in default case we will just go through
with bad data. I agree to use switch here.

>
> Might be better to go with a switch statement so we have a default
> that returns an error if nothing else is valid. If there is another
> option then at least a comment to say what it is would be useful.
>
> > + measurement = mlx90632_perform_measurement_burst(data);
> > + if (measurement < 0) {
> > + ret = measurement;
> > + goto read_unlock;
> > + }
> > }
> > +
> > ret = mlx90632_read_ambient_raw(data->regmap, ambient_new_raw,
> > ambient_old_raw);
> > if (ret < 0)
> > @@ -441,14 +602,20 @@ static int mlx90632_read_all_channel_extended(struct mlx90632_data *data, s16 *o
> > s32 ret, meas;
> >
> > mutex_lock(&data->lock);
> > - ret = mlx90632_set_meas_type(data->regmap, MLX90632_MTYP_EXTENDED);
> > + ret = mlx90632_set_meas_type(data, MLX90632_MTYP_EXTENDED);
> > if (ret < 0)
> > goto read_unlock;
> >
> > - ret = read_poll_timeout(mlx90632_perform_measurement, meas, meas == 19,
> > - 50000, 800000, false, data);
> > - if (ret != 0)
> > - goto read_unlock;
> > + if (data->powerstatus == MLX90632_PWR_STATUS_CONTINUOUS) {
> > + ret = read_poll_timeout(mlx90632_perform_measurement, meas, meas == 19,
> > + 50000, 800000, false, data);
> > + if (ret)
> > + goto read_unlock;
> > + } else if (data->powerstatus == MLX90632_PWR_STATUS_SLEEP_STEP) {
> > + ret = mlx90632_perform_measurement_burst(data);
> > + if (ret < 0)
> > + goto read_unlock;
> > + }
> >
> > ret = mlx90632_read_object_raw_extended(data->regmap, object_new_raw);
> > if (ret < 0)
> > @@ -457,8 +624,6 @@ static int mlx90632_read_all_channel_extended(struct mlx90632_data *data, s16 *o
> > ret = mlx90632_read_ambient_raw_extended(data->regmap, ambient_new_raw, ambient_old_raw);
> >
> > read_unlock:
> > - (void) mlx90632_set_meas_type(data->regmap, MLX90632_MTYP_MEDICAL);
> > -
> > mutex_unlock(&data->lock);
> > return ret;
> > }
> > @@ -743,12 +908,39 @@ static int mlx90632_calc_ambient_dsp105(struct mlx90632_data *data, int *val)
> > return ret;
> > }
> >
> > +static int mlx90632_pm_interraction_wakeup(struct mlx90632_data *data)
>
> I'd suggest a comment for this function to briefly lay out the logic
> under which it decides to switch the mode to continuous.
>
> > +{
> > + unsigned long now;
> > + int ret;
> > +
> > + now = jiffies;
> > + if (time_in_range(now, data->interraction_ts,
> > + data->interraction_ts +
> > + msecs_to_jiffies(MLX90632_MEAS_MAX_TIME + 100))) {
> > + if (data->powerstatus == MLX90632_PWR_STATUS_SLEEP_STEP) {
> > + ret = mlx90632_pwr_continuous(data->regmap);
> > + if (ret < 0)
> > + return ret;
> > + }
> > + }
> > +
> > + data->interraction_ts = now;
> > +
> > + return 0;
> > +}
> > +
> > static int mlx90632_read_raw(struct iio_dev *indio_dev,
> > struct iio_chan_spec const *channel, int *val,
> > int *val2, long mask)
> > {
> > struct mlx90632_data *data = iio_priv(indio_dev);
> > int ret;
> > + int cr;
> > +
> > + pm_runtime_get_sync(&data->client->dev);
> > + ret = mlx90632_pm_interraction_wakeup(data);
> > + if (ret < 0)
> > + goto mlx90632_read_raw_pm;
> >
> > switch (mask) {
> > case IIO_CHAN_INFO_PROCESSED:
> > @@ -756,16 +948,22 @@ static int mlx90632_read_raw(struct iio_dev *indio_dev,
> > case IIO_MOD_TEMP_AMBIENT:
> > ret = mlx90632_calc_ambient_dsp105(data, val);
> > if (ret < 0)
> > - return ret;
> > - return IIO_VAL_INT;
> > + goto mlx90632_read_raw_pm;
> > +
> > + ret = IIO_VAL_INT;
> > + break;
> > case IIO_MOD_TEMP_OBJECT:
> > ret = mlx90632_calc_object_dsp105(data, val);
> > if (ret < 0)
> > - return ret;
> > - return IIO_VAL_INT;
> > + goto mlx90632_read_raw_pm;
> > +
> > + ret = IIO_VAL_INT;
> > + break;
> > default:
> > - return -EINVAL;
> > + ret = -EINVAL;
> > + break;
> > }
> > + break;
> > case IIO_CHAN_INFO_CALIBEMISSIVITY:
> > if (data->emissivity == 1000) {
> > *val = 1;
> > @@ -774,13 +972,22 @@ static int mlx90632_read_raw(struct iio_dev *indio_dev,
> > *val = 0;
> > *val2 = data->emissivity * 1000;
> > }
> > - return IIO_VAL_INT_PLUS_MICRO;
> > + ret = IIO_VAL_INT_PLUS_MICRO;
> > + break;
> > case IIO_CHAN_INFO_CALIBAMBIENT:
> > *val = data->object_ambient_temperature;
> > - return IIO_VAL_INT;
> > + ret = IIO_VAL_INT;
> > + break;
> > default:
> > - return -EINVAL;
> > + ret = -EINVAL;
> > + break;
> > }
> > +
> > +mlx90632_read_raw_pm:
> > + mutex_unlock(&data->lock);
>
> Where did the locking here come from? I can't see why you'd
> hold the lock at this point as we don't need it for the
> runtime pm calls.
>
> > + pm_runtime_mark_last_busy(&data->client->dev);
> > + pm_runtime_put_autosuspend(&data->client->dev);
> > + return ret;
> > }
> >
> > static int mlx90632_write_raw(struct iio_dev *indio_dev,
> > @@ -902,6 +1109,7 @@ static int mlx90632_probe(struct i2c_client *client,
> > mlx90632->client = client;
> > mlx90632->regmap = regmap;
> > mlx90632->mtyp = MLX90632_MTYP_MEDICAL;
> > + mlx90632->powerstatus = MLX90632_PWR_STATUS_HALT;
> >
> > mutex_init(&mlx90632->lock);
> > indio_dev->name = id->name;
> > @@ -961,16 +1169,18 @@ static int mlx90632_probe(struct i2c_client *client,
> >
> > mlx90632->emissivity = 1000;
> > mlx90632->object_ambient_temperature = 25000; /* 25 degrees milliCelsius */
> > + mlx90632->interraction_ts = jiffies; /* Set initial value */
> >
> > - pm_runtime_disable(&client->dev);
> > + pm_runtime_get_noresume(&client->dev);
> > ret = pm_runtime_set_active(&client->dev);
> > if (ret < 0) {
> > mlx90632_sleep(mlx90632);
> > return ret;
> > }
> > - pm_runtime_enable(&client->dev);
> > + devm_pm_runtime_enable(&client->dev);
>
> This automates disabling of runtime pm, but I'm not seeing changes to the remove()
> function. Hence I think you are turning it off twice.
>
> There is an ordering issue though in that if you remove the tear down in remove()
> the sleep() call in there and the iio_device_unregister in there
> will happen before runtime pm is disabled, thus leaving us with some potential
> race conditions. I would suggest registering a callback with
> devm_add_action_or_reset() that puts the device to sleep (register that just
> after wherever you turn it on). Then the iio_device_register to the devm
> form and drop the remove function entirely. Ideally that conversion to devm
> would be a separate patch, but it is a bit interwoven with this one so
> I'm fine with you just calling it clearly in the patch description.
>
I had simple enable and add action to start with, but then I read some
kernel docs that devm_pm_runtime_enable should be used in that case. I
can revert to all the things other drivers have.

> > pm_runtime_set_autosuspend_delay(&client->dev, MLX90632_SLEEP_DELAY_MS);
> > pm_runtime_use_autosuspend(&client->dev);
> > + pm_runtime_put(&client->dev);
> Hmm. I'd feel slightly happier if this were a pm_runtime_put_autosuspend() so
> all paths use the same infrastructure. Doubt it matters much though.
>
> >
> > return iio_device_register(indio_dev);
> > }
> > @@ -1003,30 +1213,43 @@ static const struct of_device_id mlx90632_of_match[] = {
> > };
> > MODULE_DEVICE_TABLE(of, mlx90632_of_match);
> >
> > -static int __maybe_unused mlx90632_pm_suspend(struct device *dev)
> > +static int mlx90632_pm_suspend(struct device *dev)
> > {
> > - struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> > - struct mlx90632_data *data = iio_priv(indio_dev);
> > + struct mlx90632_data *data = iio_priv(dev_get_drvdata(dev));
> > + int ret;
> > +
> > + ret = regulator_disable(data->regulator);
>
> As the regulator may not be controllable, can we at least ensure we
> do the same as in runtime suspend? Obviously that will be irrelevant
> if we do just turn the power off. That would probably mean calling
> pm_runtime_force_suspend()/resume() in these handlers.
Well since resume is nothing and suspend is setting to sleep step
mode, I can just add a function call to sleep step mode.
>
> > + if (ret < 0)
> > + dev_err(regmap_get_device(data->regmap),
> > + "Failed to disable power regulator: %d\n", ret);
> >
> > - return mlx90632_sleep(data);
> > + return ret;
> > }
> >
> > -static int __maybe_unused mlx90632_pm_resume(struct device *dev)
> > +static int mlx90632_pm_resume(struct device *dev)
> > {
> > - struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> > - struct mlx90632_data *data = iio_priv(indio_dev);
> > + struct mlx90632_data *data = iio_priv(dev_get_drvdata(dev));
> > +
> > + return mlx90632_enable_regulator(data);
> > +}
> >
> > - return mlx90632_wakeup(data);
> > +static int mlx90632_pm_runtime_suspend(struct device *dev)
> > +{
> > + struct mlx90632_data *data = iio_priv(dev_get_drvdata(dev));
> > +
> > + return mlx90632_pwr_set_sleep_step(data->regmap);
> > }
> >
>