* [PATCH v2 1/3] iio: temperature: mlx90632 Add runtime powermanagement modes
@ 2022-09-03 22:23 cmo
2022-09-04 14:39 ` Jonathan Cameron
0 siblings, 1 reply; 4+ messages in thread
From: cmo @ 2022-09-03 22:23 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-iio, linux-kernel, Andy Shevchenko, Crt Mori
From: Crt Mori <cmo@melexis.com>
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@melexis.com>
---
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)
#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
+ * 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 */
};
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) {
+ 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) {
+ 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, ®);
+ if (ret < 0)
+ return ret;
+
+ return MLX90632_MEAS_MAX_TIME >> (MLX90632_EE_RR(reg) >> 8);
+}
+
+static int mlx90632_calculate_dataset_ready_time(struct mlx90632_data *data)
+{
+ unsigned int refresh_time;
+ int ret;
+
+ 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 += 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 += ret;
+
+ ret = mlx90632_get_measurement_time(data->regmap,
+ MLX90632_EE_EXTENDED_MEAS3);
+ if (ret < 0)
+ return ret;
+
+ 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 +347,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 = regmap_write(regmap, MLX90632_REG_I2C_CMD, MLX90632_RESET_CMD);
+ 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;
+}
+
+
+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);
+
+ 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) {
+ 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)
+{
+ 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);
+ 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);
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);
}
@@ -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);
+ 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);
}
-static UNIVERSAL_DEV_PM_OPS(mlx90632_pm_ops, mlx90632_pm_suspend,
- mlx90632_pm_resume, NULL);
+const struct dev_pm_ops mlx90632_pm_ops = {
+ SYSTEM_SLEEP_PM_OPS(mlx90632_pm_suspend, mlx90632_pm_resume)
+ RUNTIME_PM_OPS(mlx90632_pm_runtime_suspend, NULL, NULL)
+};
static struct i2c_driver mlx90632_driver = {
.driver = {
.name = "mlx90632",
.of_match_table = mlx90632_of_match,
- .pm = &mlx90632_pm_ops,
+ .pm = pm_ptr(&mlx90632_pm_ops),
},
.probe = mlx90632_probe,
.remove = mlx90632_remove,
--
2.34.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2 1/3] iio: temperature: mlx90632 Add runtime powermanagement modes
2022-09-03 22:23 [PATCH v2 1/3] iio: temperature: mlx90632 Add runtime powermanagement modes cmo
@ 2022-09-04 14:39 ` Jonathan Cameron
2022-09-04 21:36 ` Crt Mori
0 siblings, 1 reply; 4+ messages in thread
From: Jonathan Cameron @ 2022-09-04 14:39 UTC (permalink / raw)
To: cmo; +Cc: linux-iio, linux-kernel, Andy Shevchenko
On Sun, 4 Sep 2022 00:23:36 +0200
cmo@melexis.com wrote:
> From: Crt Mori <cmo@melexis.com>
>
> 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@melexis.com>
Hi Crt,
If you can fix the patch threading (patches should be replies to the cover letter)
that would be great.
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, ®);
> + 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.
> +
> + 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?
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.
> 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.
> + 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);
> }
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2 1/3] iio: temperature: mlx90632 Add runtime powermanagement modes
2022-09-04 14:39 ` Jonathan Cameron
@ 2022-09-04 21:36 ` Crt Mori
2022-09-04 21:44 ` Andy Shevchenko
0 siblings, 1 reply; 4+ messages in thread
From: Crt Mori @ 2022-09-04 21:36 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-iio, linux-kernel, Andy Shevchenko
On Sun, 4 Sept 2022 at 17:13, Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sun, 4 Sep 2022 00:23:36 +0200
> cmo@melexis.com wrote:
>
> > From: Crt Mori <cmo@melexis.com>
> >
> > 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@melexis.com>
>
> 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, ®);
> > + 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);
> > }
> >
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2 1/3] iio: temperature: mlx90632 Add runtime powermanagement modes
2022-09-04 21:36 ` Crt Mori
@ 2022-09-04 21:44 ` Andy Shevchenko
0 siblings, 0 replies; 4+ messages in thread
From: Andy Shevchenko @ 2022-09-04 21:44 UTC (permalink / raw)
To: Crt Mori; +Cc: Jonathan Cameron, linux-iio, Linux Kernel Mailing List
On Mon, Sep 5, 2022 at 12:36 AM Crt Mori <cmo@melexis.com> wrote:
> On Sun, 4 Sept 2022 at 17:13, Jonathan Cameron <jic23@kernel.org> wrote:
> > On Sun, 4 Sep 2022 00:23:36 +0200
> > cmo@melexis.com wrote:
First of _all_, please remove unnecessary context when replying!
...
> > 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?
`git format-patch --cover-letter --thread ...`
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-09-04 21:50 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-03 22:23 [PATCH v2 1/3] iio: temperature: mlx90632 Add runtime powermanagement modes cmo
2022-09-04 14:39 ` Jonathan Cameron
2022-09-04 21:36 ` Crt Mori
2022-09-04 21:44 ` Andy Shevchenko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.