| Message ID | 20220902131258.3316367-1-cmo@melexis.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/3] iio: temperature: mlx90632 Add runtime powermanagement modes | expand |
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
Hi Andy, Thanks for the review. I have few questions about your remarks below. 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: > > > > 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) > This is existing function that I did not want to move upwards. Should I have just moved it rather? > > 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. > This is used, because when powerstatus is not equal to sleep_step it returns, otherwise the ret is changed in the function. > > + 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? > I find it easier to have it split but I can make one long statement. > > + 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 > I like the outer parentheses it shows that it is a break condition. I have same in another function in this file. > > + 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. > No, the powermode changes among the type. > > + 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. > Care to explain a bit more about this? I just followed what other drivers have... > > +{ > > + 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
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.
On Fri, 2 Sept 2022 at 20:39, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > 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: > > > > + 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. > In this case current power mode of the sensor is halt, so the else is needed to set it to continuous mode, which means I can't just remove the else here because this statement restores the power mode before this function was entered (and changed powermode for the setup). > > > > + 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. > Thanks for the hint.
On Sat, Sep 3, 2022 at 3:06 PM Crt Mori <cmo@melexis.com> wrote: > On Fri, 2 Sept 2022 at 20:39, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > 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: ... > > > > > + 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. > > > In this case current power mode of the sensor is halt, so the else is > needed to set it to continuous mode, which means I can't just remove > the else here because this statement restores the power mode before > this function was entered (and changed powermode for the setup). What do you mean by all this? Read your code again and see what I'm talking about... > > > > > + return mlx90632_pwr_continuous(data->regmap);
On Sat, 3 Sep 2022 14:05:38 +0200 Crt Mori <cmo@melexis.com> wrote: > On Fri, 2 Sept 2022 at 20:39, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > 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: > > > > > + 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. > > > In this case current power mode of the sensor is halt, so the else is > needed to set it to continuous mode, which means I can't just remove > the else here because this statement restores the power mode before > this function was entered (and changed powermode for the setup). > > > > > > + 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. > > > Thanks for the hint. The relevant EXPORT_ for this particular case isn't upstream yet We had a proposal on IIO list, but there was a better one as part of cleaning this up for MFD. I haven't checked if there is a suitable immutable branch for that patch yet... Jonathan
On Sun, Sep 4, 2022 at 5:37 PM Jonathan Cameron <jic23@kernel.org> wrote: > On Sat, 3 Sep 2022 14:05:38 +0200 > Crt Mori <cmo@melexis.com> wrote: > > On Fri, 2 Sept 2022 at 20:39, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > 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: ... > > > > > > +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. > > > > > Thanks for the hint. > > The relevant EXPORT_ for this particular case isn't upstream yet > We had a proposal on IIO list, but there was a better one as part of > cleaning this up for MFD. I haven't checked if there is a suitable > immutable branch for that patch yet... Oh, well to know. But in any case pm_ptr() / DEFINE_*PM_OPS can still be used.
diff --git a/drivers/iio/temperature/mlx90632.c b/drivers/iio/temperature/mlx90632.c index 549c0ab5c2be..45a1ab51fd0c 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,6 +70,7 @@ #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) @@ -70,6 +79,8 @@ #define MLX90632_PWR_STATUS_STEP MLX90632_PWR_STATUS(2) /* step */ #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 #define MLX90632_MTYP_EXTENDED 17 @@ -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 /* Maximum measurement time in ms for the lowest possible 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_timestamp: Timestamp of the last temperature read that is used + * 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_timestamp; /* in jiffies */ }; 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), @@ -196,18 +215,42 @@ static const struct regmap_config mlx90632_regmap = { .cache_type = REGCACHE_RBTREE, }; +static int mlx90632_wakeup(struct mlx90632_data *data); + 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 iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(regmap_get_device(regmap))); + struct mlx90632_data *data = iio_priv(indio_dev); + s32 ret = 0; + + 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; } 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 iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(regmap_get_device(regmap))); + struct mlx90632_data *data = iio_priv(indio_dev); + s32 ret = 0; + + if (data->powerstatus != MLX90632_PWR_STATUS_CONTINUOUS) { + 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 ret; } /** @@ -219,6 +262,65 @@ static void mlx90632_reset_delay(void) usleep_range(150, 200); } +static int mlx90632_get_measurement_time(struct regmap *regmap, u16 meas) +{ + int ret; + unsigned int reg; + + ret = regmap_read(regmap, meas, ®); + if (ret < 0) + return ret; + + reg = MLX90632_EE_RR(reg) >> 8; + + return MLX90632_MEAS_MAX_TIME >> reg; +} + +static int mlx90632_calculate_dataset_ready_time(struct mlx90632_data *data) +{ + int ret; + unsigned int refresh_time; + + if (data->mtyp == MLX90632_MTYP_MEDICAL) { + ret = mlx90632_get_measurement_time(data->regmap, + MLX90632_EE_MEDICAL_MEAS1); + if (ret < 0) + return ret; + + refresh_time = ret; + + ret = mlx90632_get_measurement_time(data->regmap, + MLX90632_EE_MEDICAL_MEAS2); + if (ret < 0) + return ret; + + refresh_time = refresh_time + ret; + } else { + ret = mlx90632_get_measurement_time(data->regmap, + MLX90632_EE_EXTENDED_MEAS1); + if (ret < 0) + return ret; + + refresh_time = ret; + + ret = mlx90632_get_measurement_time(data->regmap, + MLX90632_EE_EXTENDED_MEAS2); + if (ret < 0) + return ret; + + refresh_time = refresh_time + ret; + + ret = mlx90632_get_measurement_time(data->regmap, + MLX90632_EE_EXTENDED_MEAS3); + if (ret < 0) + return ret; + + refresh_time = refresh_time + ret; + } + + return refresh_time; +} + /** * mlx90632_perform_measurement() - Trigger and retrieve current measurement cycle * @data: pointer to mlx90632_data object containing regmap information @@ -249,26 +351,76 @@ static int mlx90632_perform_measurement(struct mlx90632_data *data) return (reg_status & MLX90632_STAT_CYCLE_POS) >> 2; } -static int mlx90632_set_meas_type(struct regmap *regmap, u8 type) +/** + * mlx90632_perform_measurement_burst() - Trigger and retrieve current measurement + * cycle in step sleep mode + * @data: pointer to mlx90632_data object containing regmap information + * + * Perform a measurement and return 2 as measurement cycle position reported + * by sensor. This is a blocking function for amount dependent on the sensor + * refresh rate. + */ +static int mlx90632_perform_measurement_burst(struct mlx90632_data *data) { + unsigned int reg_status; int ret; - if ((type != MLX90632_MTYP_MEDICAL) && (type != MLX90632_MTYP_EXTENDED)) - return -EINVAL; + ret = regmap_write_bits(data->regmap, MLX90632_REG_CONTROL, + MLX90632_CFG_SOB_MASK, MLX90632_CFG_SOB_MASK); + if (ret < 0) + return ret; + + ret = mlx90632_calculate_dataset_ready_time(data); + if (ret < 0) + return ret; + + msleep(ret); /* Wait minimum time for dataset to be ready */ + + ret = regmap_read_poll_timeout(data->regmap, MLX90632_REG_STATUS, + reg_status, + ((reg_status & MLX90632_STAT_BUSY) == 0), + 10000, 100 * 10000); + if (ret < 0) { + dev_err(&data->client->dev, "data not ready"); + return -ETIMEDOUT; + } + + return 2; +} + - ret = regmap_write(regmap, MLX90632_REG_I2C_CMD, MLX90632_RESET_CMD); +static int mlx90632_set_meas_type(struct mlx90632_data *data, u8 type) +{ + int ret; + int current_powerstatus = data->powerstatus; + + if (data->mtyp == type) { + return 0; + } + + 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); + else + return mlx90632_pwr_continuous(data->regmap); } static int mlx90632_channel_new_select(int perform_ret, uint8_t *channel_new, @@ -355,11 +507,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) { + 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 +606,23 @@ 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 != 0) + 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 +631,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 +915,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) +{ + unsigned long now; + int ret = 0; + + now = jiffies; + if (time_in_range(now, data->interraction_timestamp, + data->interraction_timestamp + + 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_timestamp = now; + + return ret; +} + 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 +955,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 +979,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); + 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 +1116,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 +1176,18 @@ static int mlx90632_probe(struct i2c_client *client, mlx90632->emissivity = 1000; mlx90632->object_ambient_temperature = 25000; /* 25 degrees milliCelsius */ + mlx90632->interraction_timestamp = 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); pm_runtime_set_autosuspend_delay(&client->dev, MLX90632_SLEEP_DELAY_MS); pm_runtime_use_autosuspend(&client->dev); + pm_runtime_put(&client->dev); return iio_device_register(indio_dev); } @@ -1007,8 +1224,14 @@ static int __maybe_unused 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); + int ret; - return mlx90632_sleep(data); + ret = regulator_disable(data->regulator); + if (ret < 0) + dev_err(regmap_get_device(data->regmap), + "Failed to disable power regulator: %d\n", ret); + + return ret; } static int __maybe_unused mlx90632_pm_resume(struct device *dev) @@ -1016,11 +1239,23 @@ static int __maybe_unused 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); - return mlx90632_wakeup(data); + return mlx90632_enable_regulator(data); } -static UNIVERSAL_DEV_PM_OPS(mlx90632_pm_ops, mlx90632_pm_suspend, - mlx90632_pm_resume, NULL); +static int __maybe_unused mlx90632_pm_runtime_suspend(struct device *dev) +{ + 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) +}; +EXPORT_SYMBOL_GPL(mlx90632_pm_ops); static struct i2c_driver mlx90632_driver = { .driver = {