[PATCH v6 0/3] iio: temperature: mlx90632: Add powermanagement

[PATCH v6 0/3] iio: temperature: mlx90632: Add powermanagement

[cmo]

From: Crt Mori <cmo@melexis.com>

As discussed previously on the group under the
"Controlling device power management from terminal" thread the mlx90632
sensor provides measurement capabilities under sleep_step mode. This
series runtime suspends the unused chip to sleep step mode to save power
but in case of continuous sequential reading it switches to continuous
mode for faster readouts. This value is hardcoded to
MLX90632_MEAS_MAX_TIME (with some buffer) and not user configurable.

The sensor runtime suspension is set to MLX90632_SLEEP_DELAY_MS which is
hardcoded to 3 times as much as MEAS_MAX_TIME.

Changes in v6:

 - Revert changes to the suspend to prevent power mode regression

Changes in v5 (per review comments from Jonathan Cameron):

 - Migrate to devm also for driver removal, along with putting it to low
   power mode

Changes in v4 (per review comments from Jonathan Cameron):

 - Migrate back to devm_pm_runtime_enable and remove the pm_disable function
 - Remove pm stuff from remove and also sleep, since when iio device is
   not registered also sleep makes no sense.
 - Replace use EOPNOTSUPP as per checkpatch suggestion although some drivers
   still use ENOTSUPP.
 - Change the style of read frequency

Changes in v3 (per review comments from Jonathan Cameron):

 - Change the "available" attribute presentation to more recent way
   suggested
 - Replace devm_pm_runtime_enable with enable and devm_add_action_or_reset
 - When suspending device also put it to lower power mode in case there is
   dummy regulator
 - Use more switch cases instead of if/else

Changes in v2:

 - apply review comments from Andy Shevchenko

Crt Mori (3):
  iio: temperature: mlx90632 Add runtime powermanagement modes
  iio: temperature: mlx90632 Read sampling frequency
  iio: temperature: mlx90632 Change return value of sensor measurement
    channel

 drivers/iio/temperature/mlx90632.c | 440 ++++++++++++++++++++++++-----
 1 file changed, 369 insertions(+), 71 deletions(-)

-- 
2.34.1

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

[cmo]

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 | 379 +++++++++++++++++++++++------
 1 file changed, 309 insertions(+), 70 deletions(-)

diff --git a/drivers/iio/temperature/mlx90632.c b/drivers/iio/temperature/mlx90632.c
index 549c0ab5c2be..71130d237a69 100644
--- a/drivers/iio/temperature/mlx90632.c
+++ b/drivers/iio/temperature/mlx90632.c
@@ -6,11 +6,14 @@
  *
  * Driver for the Melexis MLX90632 I2C 16-bit IR thermopile sensor
  */
+#include <linux/bitfield.h>
 #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 +58,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 +71,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 GENMASK(10, 8) /* Only Refresh Rate bits */
 
 /* Measurement types */
 #define MLX90632_MTYP_MEDICAL 0
@@ -116,8 +128,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 +143,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
+ * @interaction_ts: Timestamp of the last temperature read that is used
+ *		    for power management in jiffies
  */
 struct mlx90632_data {
 	struct i2c_client *client;
@@ -139,6 +155,8 @@ struct mlx90632_data {
 	u8 mtyp;
 	u32 object_ambient_temperature;
 	struct regulator *regulator;
+	int powerstatus;
+	unsigned long interaction_ts;
 };
 
 static const struct regmap_range mlx90632_volatile_reg_range[] = {
@@ -158,6 +176,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 +218,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)
+		return 0;
+
+	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 mlx90632_data *data =
+		iio_priv(dev_get_drvdata(regmap_get_device(regmap)));
+	s32 ret;
+
+	if (data->powerstatus == MLX90632_PWR_STATUS_CONTINUOUS)
+		return 0;
+
+	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 +261,63 @@ static void mlx90632_reset_delay(void)
 	usleep_range(150, 200);
 }
 
+static int mlx90632_get_measurement_time(struct regmap *regmap, u16 meas)
+{
+	unsigned int reg;
+	int ret;
+
+	ret = regmap_read(regmap, meas, &reg);
+	if (ret < 0)
+		return ret;
+
+	return MLX90632_MEAS_MAX_TIME >> FIELD_GET(MLX90632_EE_RR, reg);
+}
+
+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 +348,75 @@ 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,30 @@ 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;
+
+	switch (data->powerstatus) {
+	case MLX90632_PWR_STATUS_CONTINUOUS:
+		measurement = mlx90632_perform_measurement(data);
+		if (measurement < 0) {
+			ret = measurement;
+			goto read_unlock;
+		}
+		break;
+	case MLX90632_PWR_STATUS_SLEEP_STEP:
+		measurement = mlx90632_perform_measurement_burst(data);
+		if (measurement < 0) {
+			ret = measurement;
+			goto read_unlock;
+		}
+		break;
+	default:
+		ret = -EOPNOTSUPP;
 		goto read_unlock;
 	}
+
 	ret = mlx90632_read_ambient_raw(data->regmap, ambient_new_raw,
 					ambient_old_raw);
 	if (ret < 0)
@@ -441,14 +608,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 +630,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 +914,47 @@ static int mlx90632_calc_ambient_dsp105(struct mlx90632_data *data, int *val)
 	return ret;
 }
 
+/**
+ * mlx90632_pm_interraction_wakeup() - Measure time between user interactions to change powermode
+ * @data: pointer to mlx90632_data object containing interaction_ts information
+ *
+ * Switch to continuous mode when interaction is faster than MLX90632_MEAS_MAX_TIME. Update the
+ * interaction_ts for each function call with the jiffies to enable measurement between function
+ * calls. Initial value of the interaction_ts needs to be set before this function call.
+ */
+static int mlx90632_pm_interraction_wakeup(struct mlx90632_data *data)
+{
+	unsigned long now;
+	int ret;
+
+	now = jiffies;
+	if (time_in_range(now, data->interaction_ts,
+			  data->interaction_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->interaction_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 +962,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 +986,21 @@ 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:
+	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,
@@ -826,11 +1046,18 @@ static const struct iio_info mlx90632_info = {
 	.write_raw = mlx90632_write_raw,
 };
 
-static int mlx90632_sleep(struct mlx90632_data *data)
+static void mlx90632_sleep(void *_data)
+{
+	struct mlx90632_data *data = _data;
+
+	mlx90632_pwr_set_sleep_step(data->regmap);
+}
+
+static int mlx90632_suspend(struct mlx90632_data *data)
 {
 	regcache_mark_dirty(data->regmap);
 
-	dev_dbg(&data->client->dev, "Requesting sleep");
+	dev_dbg(&data->client->dev, "Requesting suspend");
 	return mlx90632_pwr_set_sleep_step(data->regmap);
 }
 
@@ -902,6 +1129,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;
@@ -933,6 +1161,13 @@ static int mlx90632_probe(struct i2c_client *client,
 		return ret;
 	}
 
+	ret = devm_add_action_or_reset(&client->dev, mlx90632_sleep, mlx90632);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to setup low power cleanup action %d\n",
+			ret);
+		return ret;
+	}
+
 	ret = regmap_read(mlx90632->regmap, MLX90632_EE_VERSION, &read);
 	if (ret < 0) {
 		dev_err(&client->dev, "read of version failed: %d\n", ret);
@@ -961,34 +1196,17 @@ static int mlx90632_probe(struct i2c_client *client,
 
 	mlx90632->emissivity = 1000;
 	mlx90632->object_ambient_temperature = 25000; /* 25 degrees milliCelsius */
+	mlx90632->interaction_ts = jiffies; /* Set initial value */
 
-	pm_runtime_disable(&client->dev);
-	ret = pm_runtime_set_active(&client->dev);
-	if (ret < 0) {
-		mlx90632_sleep(mlx90632);
-		return ret;
-	}
-	pm_runtime_enable(&client->dev);
+	pm_runtime_get_noresume(&client->dev);
+	pm_runtime_set_active(&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_autosuspend(&client->dev);
 
-	return iio_device_register(indio_dev);
-}
-
-static int mlx90632_remove(struct i2c_client *client)
-{
-	struct iio_dev *indio_dev = i2c_get_clientdata(client);
-	struct mlx90632_data *data = iio_priv(indio_dev);
-
-	iio_device_unregister(indio_dev);
-
-	pm_runtime_disable(&client->dev);
-	pm_runtime_set_suspended(&client->dev);
-	pm_runtime_put_noidle(&client->dev);
-
-	mlx90632_sleep(data);
-
-	return 0;
+	return devm_iio_device_register(&client->dev, indio_dev);
 }
 
 static const struct i2c_device_id mlx90632_id[] = {
@@ -1003,33 +1221,54 @@ 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 = mlx90632_suspend(data);
+	if (ret < 0)
+		return 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)
+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));
+	int ret;
+
+	ret = mlx90632_enable_regulator(data);
+	if (ret < 0)
+		return ret;
 
 	return mlx90632_wakeup(data);
 }
 
-static UNIVERSAL_DEV_PM_OPS(mlx90632_pm_ops, mlx90632_pm_suspend,
-			    mlx90632_pm_resume, NULL);
+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);
+}
+
+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,
 	.id_table = mlx90632_id,
 };
 module_i2c_driver(mlx90632_driver);
-- 
2.34.1

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

[Christophe JAILLET]

Le 22/09/2022 à 10:13, cmo-fc6wVz46lShBDgjK7y7TUQ@public.gmane.org a écrit :
> From: Crt Mori <cmo-fc6wVz46lShBDgjK7y7TUQ@public.gmane.org>
> 
> 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).

Hi,

should there be a v7, a few nitpick below.

> 
> Signed-off-by: Crt Mori <cmo-fc6wVz46lShBDgjK7y7TUQ@public.gmane.org>
> ---
>   drivers/iio/temperature/mlx90632.c | 379 +++++++++++++++++++++++------
>   1 file changed, 309 insertions(+), 70 deletions(-)
> 
> diff --git a/drivers/iio/temperature/mlx90632.c b/drivers/iio/temperature/mlx90632.c
> index 549c0ab5c2be..71130d237a69 100644
> --- a/drivers/iio/temperature/mlx90632.c
> +++ b/drivers/iio/temperature/mlx90632.c
> @@ -6,11 +6,14 @@
>    *
>    * Driver for the Melexis MLX90632 I2C 16-bit IR thermopile sensor
>    */
> +#include <linux/bitfield.h>
>   #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 +58,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 +71,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 GENMASK(10, 8) /* Only Refresh Rate bits */
>   
>   /* Measurement types */
>   #define MLX90632_MTYP_MEDICAL 0
> @@ -116,8 +128,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 +143,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
> + * @interaction_ts: Timestamp of the last temperature read that is used
> + *		    for power management in jiffies
>    */
>   struct mlx90632_data {
>   	struct i2c_client *client;
> @@ -139,6 +155,8 @@ struct mlx90632_data {
>   	u8 mtyp;
>   	u32 object_ambient_temperature;
>   	struct regulator *regulator;
> +	int powerstatus;
> +	unsigned long interaction_ts;
>   };
>   
>   static const struct regmap_range mlx90632_volatile_reg_range[] = {
> @@ -158,6 +176,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 +218,38 @@ static const struct regmap_config mlx90632_regmap = {
>   
>   static s32 mlx90632_pwr_set_sleep_step(struct regmap *regmap)

Not related to this patch, but why s32 and not int?
It would be more in line with places where this function is used.

>   {
> -	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)
> +		return 0;
> +
> +	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;

return 0; ?

>   }
>   
>   static s32 mlx90632_pwr_continuous(struct regmap *regmap)

Not related to this patch, but why s32 and not int?
It would be more in line with places where this function is used.

>   {
> -	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)
> +		return 0;
> +
> +	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;

return 0; ?

>   }
>   
>   /**
> @@ -219,6 +261,63 @@ static void mlx90632_reset_delay(void)
>   	usleep_range(150, 200);
>   }
>   
> +static int mlx90632_get_measurement_time(struct regmap *regmap, u16 meas)
> +{
> +	unsigned int reg;
> +	int ret;
> +
> +	ret = regmap_read(regmap, meas, &reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	return MLX90632_MEAS_MAX_TIME >> FIELD_GET(MLX90632_EE_RR, reg);
> +}
> +
> +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 +348,75 @@ 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;

Why not "return ret;"?

> +	}
> +
> +	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,30 @@ 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;
> +
> +	switch (data->powerstatus) {
> +	case MLX90632_PWR_STATUS_CONTINUOUS:
> +		measurement = mlx90632_perform_measurement(data);

   ret = mlx90632_perform_measurement(data);
and
   measurement = ret;
on success would be less verbose (no need for {}, and save 1 LoC) and 
more in line with mlx90632_calculate_dataset_ready_time() above.

> +		if (measurement < 0) {
> +			ret = measurement;
> +			goto read_unlock;
> +		}
> +		break;
> +	case MLX90632_PWR_STATUS_SLEEP_STEP:
> +		measurement = mlx90632_perform_measurement_burst(data);

Same.

> +		if (measurement < 0) {
> +			ret = measurement;
> +			goto read_unlock;
> +		}
> +		break;
> +	default:
> +		ret = -EOPNOTSUPP;
>   		goto read_unlock;
>   	}
> +
>   	ret = mlx90632_read_ambient_raw(data->regmap, ambient_new_raw,
>   					ambient_old_raw);
>   	if (ret < 0)
> @@ -441,14 +608,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 +630,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 +914,47 @@ static int mlx90632_calc_ambient_dsp105(struct mlx90632_data *data, int *val)
>   	return ret;
>   }
>   
> +/**
> + * mlx90632_pm_interraction_wakeup() - Measure time between user interactions to change powermode
> + * @data: pointer to mlx90632_data object containing interaction_ts information
> + *
> + * Switch to continuous mode when interaction is faster than MLX90632_MEAS_MAX_TIME. Update the
> + * interaction_ts for each function call with the jiffies to enable measurement between function
> + * calls. Initial value of the interaction_ts needs to be set before this function call.
> + */
> +static int mlx90632_pm_interraction_wakeup(struct mlx90632_data *data)
> +{
> +	unsigned long now;
> +	int ret;
> +
> +	now = jiffies;
> +	if (time_in_range(now, data->interaction_ts,
> +			  data->interaction_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->interaction_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;

This 'cr' seems to be unused.

> +
> +	pm_runtime_get_sync(&data->client->dev);
> +	ret = mlx90632_pm_interraction_wakeup(data);
> +	if (ret < 0)
> +		goto mlx90632_read_raw_pm;

[...]

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

[Christophe JAILLET]

Le 02/10/2022 à 18:09, Christophe JAILLET a écrit :
>>   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;
> 
> This 'cr' seems to be unused.

Ok, used in patch 2/3.
Sorry for the noise.

CJ

> 
>> +
>> +    pm_runtime_get_sync(&data->client->dev);
>> +    ret = mlx90632_pm_interraction_wakeup(data);
>> +    if (ret < 0)
>> +        goto mlx90632_read_raw_pm;
> 
> [...]
> 

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

[Andy Shevchenko]

On Sun, Oct 2, 2022 at 7:11 PM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
> Le 02/10/2022 à 18:09, Christophe JAILLET a écrit :

...

> >>   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;
> >
> > This 'cr' seems to be unused.
>
> Ok, used in patch 2/3.
> Sorry for the noise.

But it should be used in this patch if it was introduced by it.

-- 
With Best Regards,
Andy Shevchenko

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

[Crt Mori]

On Sun, 2 Oct 2022 at 18:09, Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
>
> Le 22/09/2022 à 10:13, cmo-fc6wVz46lShBDgjK7y7TUQ@public.gmane.org a écrit :
> > From: Crt Mori <cmo-fc6wVz46lShBDgjK7y7TUQ@public.gmane.org>
> > measurements in lower power mode (SLEEP_STEP), with the lowest refresh
> > rate (2 seconds).
>
> Hi,
>
> should there be a v7, a few nitpick below.
>
It was already applied, but I can spin a new patch for the suggested
changes (the s32 is mostly there because before this patch it was
returning value for further bit manipulation).

> >
> > +     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;
>
> Why not "return ret;"?
>
If you came to this point there were already several i2c reads, so I
think it is more important to convert those to timeout.

> >       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;
> > +
> > +     switch (data->powerstatus) {
> > +     case MLX90632_PWR_STATUS_CONTINUOUS:
> > +             measurement = mlx90632_perform_measurement(data);
>
>    ret = mlx90632_perform_measurement(data);
> and
>    measurement = ret;
> on success would be less verbose (no need for {}, and save 1 LoC) and
> more in line with mlx90632_calculate_dataset_ready_time() above.
>
I wanted to change as few lines as possible to avoid clogging the
patch with unrelated changes. Also in most cases, we will be
in-success here, so limiting the number of variable copies in the
success path should be the priority.

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

[Jonathan Cameron]

On Mon, 3 Oct 2022 10:20:18 +0200
Crt Mori <cmo@melexis.com> wrote:

> On Sun, 2 Oct 2022 at 18:09, Christophe JAILLET
> <christophe.jaillet@wanadoo.fr> wrote:
> >
> > Le 22/09/2022 à 10:13, cmo-fc6wVz46lShBDgjK7y7TUQ@public.gmane.org a écrit :  
> > > From: Crt Mori <cmo-fc6wVz46lShBDgjK7y7TUQ@public.gmane.org>
> > > measurements in lower power mode (SLEEP_STEP), with the lowest refresh
> > > rate (2 seconds).  
> >
> > Hi,
> >
> > should there be a v7, a few nitpick below.
> >  
> It was already applied, but I can spin a new patch for the suggested
> changes (the s32 is mostly there because before this patch it was
> returning value for further bit manipulation).

Follow on patch welcome!

Thanks,

Jonathan

> 
> > >
> > > +     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;  
> >
> > Why not "return ret;"?
> >  
> If you came to this point there were already several i2c reads, so I
> think it is more important to convert those to timeout.
> 
> > >       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;
> > > +
> > > +     switch (data->powerstatus) {
> > > +     case MLX90632_PWR_STATUS_CONTINUOUS:
> > > +             measurement = mlx90632_perform_measurement(data);  
> >
> >    ret = mlx90632_perform_measurement(data);
> > and
> >    measurement = ret;
> > on success would be less verbose (no need for {}, and save 1 LoC) and
> > more in line with mlx90632_calculate_dataset_ready_time() above.
> >  
> I wanted to change as few lines as possible to avoid clogging the
> patch with unrelated changes. Also in most cases, we will be
> in-success here, so limiting the number of variable copies in the
> success path should be the priority.

[PATCH v6 2/3] iio: temperature: mlx90632 Read sampling frequency

[cmo]

From: Crt Mori <cmo@melexis.com>

Allow users to read sensor sampling frequency to better plan the
application measurement requests.

Signed-off-by: Crt Mori <cmo@melexis.com>
---
 drivers/iio/temperature/mlx90632.c | 59 ++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/drivers/iio/temperature/mlx90632.c b/drivers/iio/temperature/mlx90632.c
index 71130d237a69..081803940261 100644
--- a/drivers/iio/temperature/mlx90632.c
+++ b/drivers/iio/temperature/mlx90632.c
@@ -81,6 +81,9 @@
 #define MLX90632_PWR_STATUS_CONTINUOUS MLX90632_PWR_STATUS(3) /* continuous */
 
 #define MLX90632_EE_RR GENMASK(10, 8) /* Only Refresh Rate bits */
+#define MLX90632_REFRESH_RATE(ee_val) FIELD_GET(MLX90632_EE_RR, ee_val)
+					/* Extract Refresh Rate from ee register */
+#define MLX90632_REFRESH_RATE_STATUS(refresh_rate) (refresh_rate << 8)
 
 /* Measurement types */
 #define MLX90632_MTYP_MEDICAL 0
@@ -914,6 +917,32 @@ static int mlx90632_calc_ambient_dsp105(struct mlx90632_data *data, int *val)
 	return ret;
 }
 
+static int mlx90632_get_refresh_rate(struct mlx90632_data *data,
+				     int *refresh_rate)
+{
+	unsigned int meas1;
+	int ret;
+
+	ret = regmap_read(data->regmap, MLX90632_EE_MEDICAL_MEAS1, &meas1);
+	if (ret < 0)
+		return ret;
+
+	*refresh_rate = MLX90632_REFRESH_RATE(meas1);
+
+	return ret;
+}
+
+static const int mlx90632_freqs[][2] = {
+	{0, 500000},
+	{1, 0},
+	{2, 0},
+	{4, 0},
+	{8, 0},
+	{16, 0},
+	{32, 0},
+	{64, 0}
+};
+
 /**
  * mlx90632_pm_interraction_wakeup() - Measure time between user interactions to change powermode
  * @data: pointer to mlx90632_data object containing interaction_ts information
@@ -992,6 +1021,15 @@ static int mlx90632_read_raw(struct iio_dev *indio_dev,
 		*val = data->object_ambient_temperature;
 		ret = IIO_VAL_INT;
 		break;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		ret = mlx90632_get_refresh_rate(data, &cr);
+		if (ret < 0)
+			goto mlx90632_read_raw_pm;
+
+		*val = mlx90632_freqs[cr][0];
+		*val2 = mlx90632_freqs[cr][1];
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
 	default:
 		ret = -EINVAL;
 		break;
@@ -1025,12 +1063,30 @@ static int mlx90632_write_raw(struct iio_dev *indio_dev,
 	}
 }
 
+static int mlx90632_read_avail(struct iio_dev *indio_dev,
+			       struct iio_chan_spec const *chan,
+			       const int **vals, int *type, int *length,
+			       long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*vals = (int *)mlx90632_freqs;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		*length = 2 * ARRAY_SIZE(mlx90632_freqs);
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
 static const struct iio_chan_spec mlx90632_channels[] = {
 	{
 		.type = IIO_TEMP,
 		.modified = 1,
 		.channel2 = IIO_MOD_TEMP_AMBIENT,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),
 	},
 	{
 		.type = IIO_TEMP,
@@ -1038,12 +1094,15 @@ static const struct iio_chan_spec mlx90632_channels[] = {
 		.channel2 = IIO_MOD_TEMP_OBJECT,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
 			BIT(IIO_CHAN_INFO_CALIBEMISSIVITY) | BIT(IIO_CHAN_INFO_CALIBAMBIENT),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),
 	},
 };
 
 static const struct iio_info mlx90632_info = {
 	.read_raw = mlx90632_read_raw,
 	.write_raw = mlx90632_write_raw,
+	.read_avail = mlx90632_read_avail,
 };
 
 static void mlx90632_sleep(void *_data)
-- 
2.34.1

[PATCH v6 3/3] iio: temperature: mlx90632 Change return value of sensor measurement channel

[cmo]

From: Crt Mori <cmo@melexis.com>

The current EINVAL value is more applicable to embedded library, where
user can actually put the fixed value to the sensor. In case of the
driver if the value of the channel is invalid it is better in inform
userspace that Channel was out of range as that implies more to internal
driver error than invalid input. It also makes for easier debugging of
where the error comes from during the development.

Signed-off-by: Crt Mori <cmo@melexis.com>
---
 drivers/iio/temperature/mlx90632.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/temperature/mlx90632.c b/drivers/iio/temperature/mlx90632.c
index 081803940261..224db7513baa 100644
--- a/drivers/iio/temperature/mlx90632.c
+++ b/drivers/iio/temperature/mlx90632.c
@@ -435,7 +435,7 @@ static int mlx90632_channel_new_select(int perform_ret, uint8_t *channel_new,
 		*channel_old = 1;
 		break;
 	default:
-		return -EINVAL;
+		return -ECHRNG;
 	}
 
 	return 0;
-- 
2.34.1

Re: [PATCH v6 0/3] iio: temperature: mlx90632: Add powermanagement

[Jonathan Cameron]

On Thu, 22 Sep 2022 10:13:21 +0200
cmo@melexis.com wrote:

> From: Crt Mori <cmo@melexis.com>
> 
> As discussed previously on the group under the
> "Controlling device power management from terminal" thread the mlx90632
> sensor provides measurement capabilities under sleep_step mode. This
> series runtime suspends the unused chip to sleep step mode to save power
> but in case of continuous sequential reading it switches to continuous
> mode for faster readouts. This value is hardcoded to
> MLX90632_MEAS_MAX_TIME (with some buffer) and not user configurable.
> 
> The sensor runtime suspension is set to MLX90632_SLEEP_DELAY_MS which is
> hardcoded to 3 times as much as MEAS_MAX_TIME.
> 
Hi Crt,

Applied. However, we are cutting it very tight for the coming merge window
so I'm not sure I'll get a 3rd pull request out (this just missed the 2nd
one as I only queued up material that was in a final state last weekend)
So for now pushed out as testing and we'll see if Linus hints at an rc8
when he releases rc7 tomorrow.  If not this will be 6.2 material now.

Thanks,

Jonathan


> Changes in v6:
> 
>  - Revert changes to the suspend to prevent power mode regression
> 
> Changes in v5 (per review comments from Jonathan Cameron):
> 
>  - Migrate to devm also for driver removal, along with putting it to low
>    power mode
> 
> Changes in v4 (per review comments from Jonathan Cameron):
> 
>  - Migrate back to devm_pm_runtime_enable and remove the pm_disable function
>  - Remove pm stuff from remove and also sleep, since when iio device is
>    not registered also sleep makes no sense.
>  - Replace use EOPNOTSUPP as per checkpatch suggestion although some drivers
>    still use ENOTSUPP.
>  - Change the style of read frequency
> 
> Changes in v3 (per review comments from Jonathan Cameron):
> 
>  - Change the "available" attribute presentation to more recent way
>    suggested
>  - Replace devm_pm_runtime_enable with enable and devm_add_action_or_reset
>  - When suspending device also put it to lower power mode in case there is
>    dummy regulator
>  - Use more switch cases instead of if/else
> 
> Changes in v2:
> 
>  - apply review comments from Andy Shevchenko
> 
> Crt Mori (3):
>   iio: temperature: mlx90632 Add runtime powermanagement modes
>   iio: temperature: mlx90632 Read sampling frequency
>   iio: temperature: mlx90632 Change return value of sensor measurement
>     channel
> 
>  drivers/iio/temperature/mlx90632.c | 440 ++++++++++++++++++++++++-----
>  1 file changed, 369 insertions(+), 71 deletions(-)
> 

Re: [PATCH v6 0/3] iio: temperature: mlx90632: Add powermanagement

[Crt Mori]

On Sat, 24 Sept 2022 at 18:32, Jonathan Cameron <jic23@kernel.org> wrote:
>
> > The sensor runtime suspension is set to MLX90632_SLEEP_DELAY_MS which is
> > hardcoded to 3 times as much as MEAS_MAX_TIME.
> >
> Hi Crt,
>
> Applied. However, we are cutting it very tight for the coming merge window
> so I'm not sure I'll get a 3rd pull request out (this just missed the 2nd
> one as I only queued up material that was in a final state last weekend)
> So for now pushed out as testing and we'll see if Linus hints at an rc8
> when he releases rc7 tomorrow.  If not this will be 6.2 material now.
>
I sure hope you mean 6.1 material... It would be great to get into 6.0
as i think that is a most likely candidate also for Android kernel
baseline (which is what I am also targeting).

> Thanks,
>
> Jonathan
>
>

Re: [PATCH v6 0/3] iio: temperature: mlx90632: Add powermanagement

[Jonathan Cameron]

On Mon, 26 Sep 2022 15:20:16 +0200
Crt Mori <cmo@melexis.com> wrote:

> On Sat, 24 Sept 2022 at 18:32, Jonathan Cameron <jic23@kernel.org> wrote:
> >  
> > > The sensor runtime suspension is set to MLX90632_SLEEP_DELAY_MS which is
> > > hardcoded to 3 times as much as MEAS_MAX_TIME.
> > >  
> > Hi Crt,
> >
> > Applied. However, we are cutting it very tight for the coming merge window
> > so I'm not sure I'll get a 3rd pull request out (this just missed the 2nd
> > one as I only queued up material that was in a final state last weekend)
> > So for now pushed out as testing and we'll see if Linus hints at an rc8
> > when he releases rc7 tomorrow.  If not this will be 6.2 material now.
> >  
> I sure hope you mean 6.1 material... It would be great to get into 6.0
> as i think that is a most likely candidate also for Android kernel
> baseline (which is what I am also targeting).

Sorry, I do mean 6.2.

The merge window for 6.0 was months ago and we have to line 6.1 material
up a week or so before the 6.1 merge window (which probably starts today).
That allows us to get build bots and similar results before the merge window
and with time to fix any issues.

So unfortunately this will only be in a released kernel in about 6 months time:
3 months for the 6.1 cycle that this just missed and then it'll go into Linus'
mainline tree, but the release will still be 3 months after that.

In the meantime it'll be in linux-next from just after 6.1-rc1, and in
Linus' tree for 6.2-rc1.

Kernel cycles are short for a bit project, but they still have about
3 to 6 month delay for new code reaching a release.  This just happened
to hit the maximum.

Jonathan


> 
> > Thanks,
> >
> > Jonathan
> >
> >  

Re: [PATCH v6 0/3] iio: temperature: mlx90632: Add powermanagement

[Crt Mori]

>> I sure hope you mean 6.1 material... It would be great to get into 6.0
>> as i think that is a most likely candidate also for Android kernel
>> baseline (which is what I am also targeting).
>
> Sorry, I do mean 6.2.
>
> The merge window for 6.0 was months ago and we have to line 6.1 material
> up a week or so before the 6.1 merge window (which probably starts today).
> That allows us to get build bots and similar results before the merge window
> and with time to fix any issues.
>
> So unfortunately this will only be in a released kernel in about 6 months time:
> 3 months for the 6.1 cycle that this just missed and then it'll go into Linus'
> mainline tree, but the release will still be 3 months after that.
>
> In the meantime it'll be in linux-next from just after 6.1-rc1, and in
> Linus' tree for 6.2-rc1.
>
> Kernel cycles are short for a bit project, but they still have about
> 3 to 6 month delay for new code reaching a release.  This just happened
> to hit the maximum.
>
> Jonathan

OK, then fingers crossed we get rc8, as then I imagine you could put
this series into the 6.1 right? Would be really grateful if it would
not take 6 months to get this into mainline.

Crt

On Sun, 2 Oct 2022 at 13:25, Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon, 26 Sep 2022 15:20:16 +0200
> Crt Mori <cmo@melexis.com> wrote:
>
> > On Sat, 24 Sept 2022 at 18:32, Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > > The sensor runtime suspension is set to MLX90632_SLEEP_DELAY_MS which is
> > > > hardcoded to 3 times as much as MEAS_MAX_TIME.
> > > >
> > > Hi Crt,
> > >
> > > Applied. However, we are cutting it very tight for the coming merge window
> > > so I'm not sure I'll get a 3rd pull request out (this just missed the 2nd
> > > one as I only queued up material that was in a final state last weekend)
> > > So for now pushed out as testing and we'll see if Linus hints at an rc8
> > > when he releases rc7 tomorrow.  If not this will be 6.2 material now.
> > >
> > I sure hope you mean 6.1 material... It would be great to get into 6.0
> > as i think that is a most likely candidate also for Android kernel
> > baseline (which is what I am also targeting).
>
> Sorry, I do mean 6.2.
>
> The merge window for 6.0 was months ago and we have to line 6.1 material
> up a week or so before the 6.1 merge window (which probably starts today).
> That allows us to get build bots and similar results before the merge window
> and with time to fix any issues.
>
> So unfortunately this will only be in a released kernel in about 6 months time:
> 3 months for the 6.1 cycle that this just missed and then it'll go into Linus'
> mainline tree, but the release will still be 3 months after that.
>
> In the meantime it'll be in linux-next from just after 6.1-rc1, and in
> Linus' tree for 6.2-rc1.
>
> Kernel cycles are short for a bit project, but they still have about
> 3 to 6 month delay for new code reaching a release.  This just happened
> to hit the maximum.
>
> Jonathan
>
>
> >
> > > Thanks,
> > >
> > > Jonathan
> > >
> > >
>

Re: [PATCH v6 0/3] iio: temperature: mlx90632: Add powermanagement

[Jonathan Cameron]

On Sun, 2 Oct 2022 16:36:39 +0200
Crt Mori <cmo@melexis.com> wrote:

> >> I sure hope you mean 6.1 material... It would be great to get into 6.0
> >> as i think that is a most likely candidate also for Android kernel
> >> baseline (which is what I am also targeting).  
> >
> > Sorry, I do mean 6.2.
> >
> > The merge window for 6.0 was months ago and we have to line 6.1 material
> > up a week or so before the 6.1 merge window (which probably starts today).
> > That allows us to get build bots and similar results before the merge window
> > and with time to fix any issues.
> >
> > So unfortunately this will only be in a released kernel in about 6 months time:
> > 3 months for the 6.1 cycle that this just missed and then it'll go into Linus'
> > mainline tree, but the release will still be 3 months after that.
> >
> > In the meantime it'll be in linux-next from just after 6.1-rc1, and in
> > Linus' tree for 6.2-rc1.
> >
> > Kernel cycles are short for a bit project, but they still have about
> > 3 to 6 month delay for new code reaching a release.  This just happened
> > to hit the maximum.
> >
> > Jonathan  
> 
> OK, then fingers crossed we get rc8, as then I imagine you could put
> this series into the 6.1 right? Would be really grateful if it would
> not take 6 months to get this into mainline.

Pretty unlikely as last Sunday Linus indicated that he was expecting to
release new kernel today :(

Note it'll be in mainline in 3 months, just not in a release because those
take just under 3 months.

Jonathan


> 
> Crt
> 
> On Sun, 2 Oct 2022 at 13:25, Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Mon, 26 Sep 2022 15:20:16 +0200
> > Crt Mori <cmo@melexis.com> wrote:
> >  
> > > On Sat, 24 Sept 2022 at 18:32, Jonathan Cameron <jic23@kernel.org> wrote:  
> > > >  
> > > > > The sensor runtime suspension is set to MLX90632_SLEEP_DELAY_MS which is
> > > > > hardcoded to 3 times as much as MEAS_MAX_TIME.
> > > > >  
> > > > Hi Crt,
> > > >
> > > > Applied. However, we are cutting it very tight for the coming merge window
> > > > so I'm not sure I'll get a 3rd pull request out (this just missed the 2nd
> > > > one as I only queued up material that was in a final state last weekend)
> > > > So for now pushed out as testing and we'll see if Linus hints at an rc8
> > > > when he releases rc7 tomorrow.  If not this will be 6.2 material now.
> > > >  
> > > I sure hope you mean 6.1 material... It would be great to get into 6.0
> > > as i think that is a most likely candidate also for Android kernel
> > > baseline (which is what I am also targeting).  
> >
> > Sorry, I do mean 6.2.
> >
> > The merge window for 6.0 was months ago and we have to line 6.1 material
> > up a week or so before the 6.1 merge window (which probably starts today).
> > That allows us to get build bots and similar results before the merge window
> > and with time to fix any issues.
> >
> > So unfortunately this will only be in a released kernel in about 6 months time:
> > 3 months for the 6.1 cycle that this just missed and then it'll go into Linus'
> > mainline tree, but the release will still be 3 months after that.
> >
> > In the meantime it'll be in linux-next from just after 6.1-rc1, and in
> > Linus' tree for 6.2-rc1.
> >
> > Kernel cycles are short for a bit project, but they still have about
> > 3 to 6 month delay for new code reaching a release.  This just happened
> > to hit the maximum.
> >
> > Jonathan
> >
> >  
> > >  
> > > > Thanks,
> > > >
> > > > Jonathan
> > > >
> > > >  
> >