* [PATCH v3 1/3] iio: temperature: mlx90632 Add runtime powermanagement modes
2022-09-06 9:06 [PATCH v3 0/3] iio: temperature: mlx90632: Add powermanagement cmo
@ 2022-09-06 9:04 ` cmo
2022-09-06 10:20 ` Andy Shevchenko
2022-09-15 14:07 ` Jonathan Cameron
2022-09-06 9:04 ` [PATCH v3 2/3] iio: temperature: mlx90632 Read sampling frequency cmo
2022-09-06 9:04 ` [PATCH v3 3/3] iio: temperature: mlx90632 Change return value of sensor measurement channel cmo
2 siblings, 2 replies; 16+ messages in thread
From: cmo @ 2022-09-06 9:04 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 | 347 +++++++++++++++++++++++++----
1 file changed, 302 insertions(+), 45 deletions(-)
diff --git a/drivers/iio/temperature/mlx90632.c b/drivers/iio/temperature/mlx90632.c
index 549c0ab5c2be..e41a18edbc65 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, ®);
+ 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,76 @@ static int mlx90632_perform_measurement(struct mlx90632_data *data)
return (reg_status & MLX90632_STAT_CYCLE_POS) >> 2;
}
-static int mlx90632_set_meas_type(struct regmap *regmap, u8 type)
+/**
+ * mlx90632_perform_measurement_burst() - Trigger and retrieve current measurement
+ * cycle in step sleep mode
+ * @data: pointer to mlx90632_data object containing regmap information
+ *
+ * Perform a measurement and return 2 as measurement cycle position reported
+ * by sensor. This is a blocking function for amount dependent on the sensor
+ * refresh rate.
+ */
+static int mlx90632_perform_measurement_burst(struct mlx90632_data *data)
{
+ unsigned int reg_status;
int ret;
- if ((type != MLX90632_MTYP_MEDICAL) && (type != MLX90632_MTYP_EXTENDED))
- return -EINVAL;
+ ret = regmap_write_bits(data->regmap, MLX90632_REG_CONTROL,
+ MLX90632_CFG_SOB_MASK, MLX90632_CFG_SOB_MASK);
+ if (ret < 0)
+ return ret;
+
+ ret = mlx90632_calculate_dataset_ready_time(data);
+ if (ret < 0)
+ return ret;
+
+ msleep(ret); /* Wait minimum time for dataset to be ready */
+
+ ret = regmap_read_poll_timeout(data->regmap, MLX90632_REG_STATUS,
+ reg_status,
+ (reg_status & MLX90632_STAT_BUSY) == 0,
+ 10000, 100 * 10000);
+ if (ret < 0) {
+ dev_err(&data->client->dev, "data not ready");
+ return -ETIMEDOUT;
+ }
+
+ return 2;
+}
+
+
+static int mlx90632_set_meas_type(struct mlx90632_data *data, u8 type)
+{
+ int current_powerstatus;
+ int ret;
+
+ if (data->mtyp == type)
+ return 0;
- ret = regmap_write(regmap, MLX90632_REG_I2C_CMD, MLX90632_RESET_CMD);
+ 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 +504,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 = -ENOTSUPP;
goto read_unlock;
}
+
ret = mlx90632_read_ambient_raw(data->regmap, ambient_new_raw,
ambient_old_raw);
if (ret < 0)
@@ -441,14 +609,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 +631,6 @@ static int mlx90632_read_all_channel_extended(struct mlx90632_data *data, s16 *o
ret = mlx90632_read_ambient_raw_extended(data->regmap, ambient_new_raw, ambient_old_raw);
read_unlock:
- (void) mlx90632_set_meas_type(data->regmap, MLX90632_MTYP_MEDICAL);
-
mutex_unlock(&data->lock);
return ret;
}
@@ -743,12 +915,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 +963,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 +987,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,
@@ -875,6 +1096,15 @@ static int mlx90632_enable_regulator(struct mlx90632_data *data)
return ret;
}
+static void mlx90632_pm_disable(void *data)
+{
+ struct device *dev = data;
+
+ pm_runtime_get_sync(dev);
+ pm_runtime_put_noidle(dev);
+ pm_runtime_disable(dev);
+}
+
static int mlx90632_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
@@ -902,6 +1132,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 +1192,25 @@ 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);
+ 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);
pm_runtime_set_autosuspend_delay(&client->dev, MLX90632_SLEEP_DELAY_MS);
pm_runtime_use_autosuspend(&client->dev);
+ pm_runtime_put_autosuspend(&client->dev);
+
+ ret = devm_add_action_or_reset(&client->dev, mlx90632_pm_disable, &client->dev);
+ if (ret) {
+ mlx90632_sleep(mlx90632);
+ return ret;
+ }
return iio_device_register(indio_dev);
}
@@ -1003,30 +1243,47 @@ 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_pwr_set_sleep_step(data->regmap);
+ 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));
- return mlx90632_wakeup(data);
+ return mlx90632_enable_regulator(data);
}
-static UNIVERSAL_DEV_PM_OPS(mlx90632_pm_ops, mlx90632_pm_suspend,
- mlx90632_pm_resume, NULL);
+static int 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,
--
2.34.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v3 2/3] iio: temperature: mlx90632 Read sampling frequency
2022-09-06 9:06 [PATCH v3 0/3] iio: temperature: mlx90632: Add powermanagement cmo
2022-09-06 9:04 ` [PATCH v3 1/3] iio: temperature: mlx90632 Add runtime powermanagement modes cmo
@ 2022-09-06 9:04 ` cmo
2022-09-06 10:21 ` Andy Shevchenko
2022-09-06 9:04 ` [PATCH v3 3/3] iio: temperature: mlx90632 Change return value of sensor measurement channel cmo
2 siblings, 1 reply; 16+ messages in thread
From: cmo @ 2022-09-06 9:04 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-iio, linux-kernel, Andy Shevchenko, Crt Mori
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 | 51 ++++++++++++++++++++++++++++++
1 file changed, 51 insertions(+)
diff --git a/drivers/iio/temperature/mlx90632.c b/drivers/iio/temperature/mlx90632.c
index e41a18edbc65..6b33284eb0b6 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
@@ -915,6 +918,24 @@ 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
@@ -993,6 +1014,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;
@@ -1026,12 +1056,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,
@@ -1039,12 +1087,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 int mlx90632_sleep(struct mlx90632_data *data)
--
2.34.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v3 3/3] iio: temperature: mlx90632 Change return value of sensor measurement channel
2022-09-06 9:06 [PATCH v3 0/3] iio: temperature: mlx90632: Add powermanagement cmo
2022-09-06 9:04 ` [PATCH v3 1/3] iio: temperature: mlx90632 Add runtime powermanagement modes cmo
2022-09-06 9:04 ` [PATCH v3 2/3] iio: temperature: mlx90632 Read sampling frequency cmo
@ 2022-09-06 9:04 ` cmo
2022-09-06 10:23 ` Andy Shevchenko
2 siblings, 1 reply; 16+ messages in thread
From: cmo @ 2022-09-06 9:04 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-iio, linux-kernel, Andy Shevchenko, Crt Mori
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 6b33284eb0b6..aee065a38772 100644
--- a/drivers/iio/temperature/mlx90632.c
+++ b/drivers/iio/temperature/mlx90632.c
@@ -436,7 +436,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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v3 0/3] iio: temperature: mlx90632: Add powermanagement
@ 2022-09-06 9:06 cmo
2022-09-06 9:04 ` [PATCH v3 1/3] iio: temperature: mlx90632 Add runtime powermanagement modes cmo
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: cmo @ 2022-09-06 9:06 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-iio, linux-kernel, Andy Shevchenko, Crt Mori
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 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 | 400 +++++++++++++++++++++++++----
1 file changed, 354 insertions(+), 46 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/3] iio: temperature: mlx90632 Add runtime powermanagement modes
2022-09-06 9:04 ` [PATCH v3 1/3] iio: temperature: mlx90632 Add runtime powermanagement modes cmo
@ 2022-09-06 10:20 ` Andy Shevchenko
2022-09-06 10:51 ` Crt Mori
2022-09-15 14:07 ` Jonathan Cameron
1 sibling, 1 reply; 16+ messages in thread
From: Andy Shevchenko @ 2022-09-06 10:20 UTC (permalink / raw)
To: Crt Mori; +Cc: Jonathan Cameron, linux-iio, Linux Kernel Mailing List
On Tue, Sep 6, 2022 at 12:04 PM <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).
Very good and documented code, thanks!
I believe you better to use DEFINE_.*_PM_OPS instead of legacy ones
(due to pm_ptr() usage).
Otherwise, with some nitpicks that wouldn't prevent a green light,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Crt Mori <cmo@melexis.com>
> ---
> drivers/iio/temperature/mlx90632.c | 347 +++++++++++++++++++++++++----
> 1 file changed, 302 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/iio/temperature/mlx90632.c b/drivers/iio/temperature/mlx90632.c
> index 549c0ab5c2be..e41a18edbc65 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, ®);
> + 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,76 @@ static int mlx90632_perform_measurement(struct mlx90632_data *data)
> return (reg_status & MLX90632_STAT_CYCLE_POS) >> 2;
> }
>
> -static int mlx90632_set_meas_type(struct regmap *regmap, u8 type)
> +/**
> + * mlx90632_perform_measurement_burst() - Trigger and retrieve current measurement
> + * cycle in step sleep mode
> + * @data: pointer to mlx90632_data object containing regmap information
> + *
> + * Perform a measurement and return 2 as measurement cycle position reported
> + * by sensor. This is a blocking function for amount dependent on the sensor
> + * refresh rate.
> + */
> +static int mlx90632_perform_measurement_burst(struct mlx90632_data *data)
> {
> + unsigned int reg_status;
> int ret;
>
> - if ((type != MLX90632_MTYP_MEDICAL) && (type != MLX90632_MTYP_EXTENDED))
> - return -EINVAL;
> + ret = regmap_write_bits(data->regmap, MLX90632_REG_CONTROL,
> + MLX90632_CFG_SOB_MASK, MLX90632_CFG_SOB_MASK);
> + if (ret < 0)
> + return ret;
> +
> + ret = mlx90632_calculate_dataset_ready_time(data);
> + if (ret < 0)
> + return ret;
> +
> + msleep(ret); /* Wait minimum time for dataset to be ready */
> +
> + ret = regmap_read_poll_timeout(data->regmap, MLX90632_REG_STATUS,
> + reg_status,
> + (reg_status & MLX90632_STAT_BUSY) == 0,
> + 10000, 100 * 10000);
> + if (ret < 0) {
> + dev_err(&data->client->dev, "data not ready");
> + return -ETIMEDOUT;
> + }
> +
> + return 2;
> +}
> +
> +
> +static int mlx90632_set_meas_type(struct mlx90632_data *data, u8 type)
> +{
> + int current_powerstatus;
> + int ret;
> +
> + if (data->mtyp == type)
> + return 0;
>
> - ret = regmap_write(regmap, MLX90632_REG_I2C_CMD, MLX90632_RESET_CMD);
> + 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 +504,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 = -ENOTSUPP;
> goto read_unlock;
> }
> +
> ret = mlx90632_read_ambient_raw(data->regmap, ambient_new_raw,
> ambient_old_raw);
> if (ret < 0)
> @@ -441,14 +609,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 +631,6 @@ static int mlx90632_read_all_channel_extended(struct mlx90632_data *data, s16 *o
> ret = mlx90632_read_ambient_raw_extended(data->regmap, ambient_new_raw, ambient_old_raw);
>
> read_unlock:
> - (void) mlx90632_set_meas_type(data->regmap, MLX90632_MTYP_MEDICAL);
> -
> mutex_unlock(&data->lock);
> return ret;
> }
> @@ -743,12 +915,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 +963,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 +987,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,
> @@ -875,6 +1096,15 @@ static int mlx90632_enable_regulator(struct mlx90632_data *data)
> return ret;
> }
>
> +static void mlx90632_pm_disable(void *data)
> +{
> + struct device *dev = data;
> +
> + pm_runtime_get_sync(dev);
> + pm_runtime_put_noidle(dev);
> + pm_runtime_disable(dev);
> +}
> +
> static int mlx90632_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
> {
> @@ -902,6 +1132,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 +1192,25 @@ 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);
> + 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);
> pm_runtime_set_autosuspend_delay(&client->dev, MLX90632_SLEEP_DELAY_MS);
> pm_runtime_use_autosuspend(&client->dev);
> + pm_runtime_put_autosuspend(&client->dev);
> +
> + ret = devm_add_action_or_reset(&client->dev, mlx90632_pm_disable, &client->dev);
> + if (ret) {
> + mlx90632_sleep(mlx90632);
> + return ret;
> + }
>
> return iio_device_register(indio_dev);
> }
> @@ -1003,30 +1243,47 @@ 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_pwr_set_sleep_step(data->regmap);
> + 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));
>
> - return mlx90632_wakeup(data);
> + return mlx90632_enable_regulator(data);
> }
>
> -static UNIVERSAL_DEV_PM_OPS(mlx90632_pm_ops, mlx90632_pm_suspend,
> - mlx90632_pm_resume, NULL);
> +static int 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,
> --
> 2.34.1
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 2/3] iio: temperature: mlx90632 Read sampling frequency
2022-09-06 9:04 ` [PATCH v3 2/3] iio: temperature: mlx90632 Read sampling frequency cmo
@ 2022-09-06 10:21 ` Andy Shevchenko
2022-09-15 14:09 ` Jonathan Cameron
0 siblings, 1 reply; 16+ messages in thread
From: Andy Shevchenko @ 2022-09-06 10:21 UTC (permalink / raw)
To: Crt Mori; +Cc: Jonathan Cameron, linux-iio, Linux Kernel Mailing List
On Tue, Sep 6, 2022 at 12:04 PM <cmo@melexis.com> wrote:
>
> 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 | 51 ++++++++++++++++++++++++++++++
> 1 file changed, 51 insertions(+)
>
> diff --git a/drivers/iio/temperature/mlx90632.c b/drivers/iio/temperature/mlx90632.c
> index e41a18edbc65..6b33284eb0b6 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
> @@ -915,6 +918,24 @@ 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} };
I would indent this as
_freqs ... = {
...pairs...
};
Either way,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> +
> /**
> * mlx90632_pm_interraction_wakeup() - Measure time between user interactions to change powermode
> * @data: pointer to mlx90632_data object containing interaction_ts information
> @@ -993,6 +1014,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;
> @@ -1026,12 +1056,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,
> @@ -1039,12 +1087,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 int mlx90632_sleep(struct mlx90632_data *data)
> --
> 2.34.1
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 3/3] iio: temperature: mlx90632 Change return value of sensor measurement channel
2022-09-06 9:04 ` [PATCH v3 3/3] iio: temperature: mlx90632 Change return value of sensor measurement channel cmo
@ 2022-09-06 10:23 ` Andy Shevchenko
0 siblings, 0 replies; 16+ messages in thread
From: Andy Shevchenko @ 2022-09-06 10:23 UTC (permalink / raw)
To: Crt Mori; +Cc: Jonathan Cameron, linux-iio, Linux Kernel Mailing List
On Tue, Sep 6, 2022 at 12:05 PM <cmo@melexis.com> wrote:
>
> From: Crt Mori <cmo@melexis.com>
>
> The current EINVAL value is more applicable to embedded library, where
an embedded
...or...
libraries
> 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.
As I said earlier I'm in favour of this change to at least make good
use of this error code since it is confirmed to help debugging.
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 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 6b33284eb0b6..aee065a38772 100644
> --- a/drivers/iio/temperature/mlx90632.c
> +++ b/drivers/iio/temperature/mlx90632.c
> @@ -436,7 +436,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
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/3] iio: temperature: mlx90632 Add runtime powermanagement modes
2022-09-06 10:20 ` Andy Shevchenko
@ 2022-09-06 10:51 ` Crt Mori
2022-09-06 12:36 ` Andy Shevchenko
0 siblings, 1 reply; 16+ messages in thread
From: Crt Mori @ 2022-09-06 10:51 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Jonathan Cameron, linux-iio, Linux Kernel Mailing List
On Tue, 6 Sept 2022 at 12:21, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> Very good and documented code, thanks!
> I believe you better to use DEFINE_.*_PM_OPS instead of legacy ones
> (due to pm_ptr() usage).
> Otherwise, with some nitpicks that wouldn't prevent a green light,
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>
I checked DEFINE_.*_PM_OPS usage around drivers and you either have
SIMPLE (where you define suspend/resume) or you have RUNTIME (for
runtime suspend/resume), but never are those two together. So I am a
bit puzzled how to get this working.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/3] iio: temperature: mlx90632 Add runtime powermanagement modes
2022-09-06 10:51 ` Crt Mori
@ 2022-09-06 12:36 ` Andy Shevchenko
2022-09-06 13:04 ` Crt Mori
0 siblings, 1 reply; 16+ messages in thread
From: Andy Shevchenko @ 2022-09-06 12:36 UTC (permalink / raw)
To: Crt Mori; +Cc: Jonathan Cameron, linux-iio, Linux Kernel Mailing List
On Tue, Sep 6, 2022 at 1:52 PM Crt Mori <cmo@melexis.com> wrote:
> On Tue, 6 Sept 2022 at 12:21, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> >
> > Very good and documented code, thanks!
> > I believe you better to use DEFINE_.*_PM_OPS instead of legacy ones
> > (due to pm_ptr() usage).
> > Otherwise, with some nitpicks that wouldn't prevent a green light,
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> >
> I checked DEFINE_.*_PM_OPS usage around drivers and you either have
> SIMPLE (where you define suspend/resume) or you have RUNTIME (for
> runtime suspend/resume), but never are those two together. So I am a
> bit puzzled how to get this working.
The one which suits here is called _DEFINE_DEV_PM_OPS(). But it's
basically the same what you put here with the possible unused case.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/3] iio: temperature: mlx90632 Add runtime powermanagement modes
2022-09-06 12:36 ` Andy Shevchenko
@ 2022-09-06 13:04 ` Crt Mori
2022-09-06 13:16 ` Andy Shevchenko
0 siblings, 1 reply; 16+ messages in thread
From: Crt Mori @ 2022-09-06 13:04 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Jonathan Cameron, linux-iio, Linux Kernel Mailing List
On Tue, 6 Sept 2022 at 14:37, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> On Tue, Sep 6, 2022 at 1:52 PM Crt Mori <cmo@melexis.com> wrote:
> > On Tue, 6 Sept 2022 at 12:21, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > >
> > > Very good and documented code, thanks!
> > > I believe you better to use DEFINE_.*_PM_OPS instead of legacy ones
> > > (due to pm_ptr() usage).
> > > Otherwise, with some nitpicks that wouldn't prevent a green light,
> > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > >
> > I checked DEFINE_.*_PM_OPS usage around drivers and you either have
> > SIMPLE (where you define suspend/resume) or you have RUNTIME (for
> > runtime suspend/resume), but never are those two together. So I am a
> > bit puzzled how to get this working.
>
> The one which suits here is called _DEFINE_DEV_PM_OPS(). But it's
> basically the same what you put here with the possible unused case.
>
I thought underscore prefixed macros are the ones not to be used
directly by drivers. I also found no occurrence in current drivers, so
it was not something that was done so far?
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/3] iio: temperature: mlx90632 Add runtime powermanagement modes
2022-09-06 13:04 ` Crt Mori
@ 2022-09-06 13:16 ` Andy Shevchenko
2022-09-08 12:43 ` Jonathan Cameron
0 siblings, 1 reply; 16+ messages in thread
From: Andy Shevchenko @ 2022-09-06 13:16 UTC (permalink / raw)
To: Crt Mori; +Cc: Jonathan Cameron, linux-iio, Linux Kernel Mailing List
On Tue, Sep 6, 2022 at 4:04 PM Crt Mori <cmo@melexis.com> wrote:
> On Tue, 6 Sept 2022 at 14:37, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Tue, Sep 6, 2022 at 1:52 PM Crt Mori <cmo@melexis.com> wrote:
> > > On Tue, 6 Sept 2022 at 12:21, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > >
> > > > Very good and documented code, thanks!
> > > > I believe you better to use DEFINE_.*_PM_OPS instead of legacy ones
> > > > (due to pm_ptr() usage).
> > > > Otherwise, with some nitpicks that wouldn't prevent a green light,
> > > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > >
> > > I checked DEFINE_.*_PM_OPS usage around drivers and you either have
> > > SIMPLE (where you define suspend/resume) or you have RUNTIME (for
> > > runtime suspend/resume), but never are those two together. So I am a
> > > bit puzzled how to get this working.
> >
> > The one which suits here is called _DEFINE_DEV_PM_OPS(). But it's
> > basically the same what you put here with the possible unused case.
> >
> I thought underscore prefixed macros are the ones not to be used
> directly by drivers. I also found no occurrence in current drivers, so
> it was not something that was done so far?
Looks like... Okay, then let's leave it to the maintainer to decide.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/3] iio: temperature: mlx90632 Add runtime powermanagement modes
2022-09-06 13:16 ` Andy Shevchenko
@ 2022-09-08 12:43 ` Jonathan Cameron
0 siblings, 0 replies; 16+ messages in thread
From: Jonathan Cameron @ 2022-09-08 12:43 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Crt Mori, Jonathan Cameron, linux-iio, Linux Kernel Mailing List
On Tue, 6 Sep 2022 16:16:07 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Tue, Sep 6, 2022 at 4:04 PM Crt Mori <cmo@melexis.com> wrote:
> > On Tue, 6 Sept 2022 at 14:37, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > On Tue, Sep 6, 2022 at 1:52 PM Crt Mori <cmo@melexis.com> wrote:
> > > > On Tue, 6 Sept 2022 at 12:21, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > >
> > > > > Very good and documented code, thanks!
> > > > > I believe you better to use DEFINE_.*_PM_OPS instead of legacy ones
> > > > > (due to pm_ptr() usage).
> > > > > Otherwise, with some nitpicks that wouldn't prevent a green light,
> > > > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > > >
> > > > I checked DEFINE_.*_PM_OPS usage around drivers and you either have
> > > > SIMPLE (where you define suspend/resume) or you have RUNTIME (for
> > > > runtime suspend/resume), but never are those two together. So I am a
> > > > bit puzzled how to get this working.
> > >
> > > The one which suits here is called _DEFINE_DEV_PM_OPS(). But it's
> > > basically the same what you put here with the possible unused case.
> > >
> > I thought underscore prefixed macros are the ones not to be used
> > directly by drivers. I also found no occurrence in current drivers, so
> > it was not something that was done so far?
>
> Looks like... Okay, then let's leave it to the maintainer to decide.
>
Indeed. Discussion ended up at right place. Don't use _DEFINE_DEV_PM_OPS()
in a driver. The solution here is correct as is.
As a side note, I think _DEFINE_DEV_PM_OPS() is getting removed or redefined
as part of tidying up the missing EXPORT_* cases. (I'm too lazy to find the
patch set but it was for mfd drivers).
Jonathan
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/3] iio: temperature: mlx90632 Add runtime powermanagement modes
2022-09-06 9:04 ` [PATCH v3 1/3] iio: temperature: mlx90632 Add runtime powermanagement modes cmo
2022-09-06 10:20 ` Andy Shevchenko
@ 2022-09-15 14:07 ` Jonathan Cameron
2022-09-15 14:35 ` Crt Mori
1 sibling, 1 reply; 16+ messages in thread
From: Jonathan Cameron @ 2022-09-15 14:07 UTC (permalink / raw)
To: cmo; +Cc: linux-iio, linux-kernel, Andy Shevchenko
On Tue, 6 Sep 2022 11:04:30 +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>
This is missing necessary disabling of autosuspend (see inline).
Also, I just realised you have the original pm disables in remove after
this patch - so effectively you disable runtime pm twice.
There is a sleep in that remove function which can also be done with
a devm_add_action_or_reset(). Do that as well and you can move
the device register to the devm form and drop the remove() function entirely.
> ---
> drivers/iio/temperature/mlx90632.c | 347 +++++++++++++++++++++++++----
> 1 file changed, 302 insertions(+), 45 deletions(-)
> + return 2;
> +}
> +
Nitpick, but single line is plenty.
> +
> +static int mlx90632_set_meas_type(struct mlx90632_data *data, u8 type)
> +{
>
> static int mlx90632_write_raw(struct iio_dev *indio_dev,
> @@ -875,6 +1096,15 @@ static int mlx90632_enable_regulator(struct mlx90632_data *data)
> return ret;
> }
>
> +static void mlx90632_pm_disable(void *data)
> +{
> + struct device *dev = data;
> +
> + pm_runtime_get_sync(dev);
So, this isn't quite enough.
Take a look at what devm_pm_runtime_enable()
does as the documentation for
pm_runtime_use_autosuspend()
I'd suggest using devm_pm_runtime_enable() and
an additional callback to turn the device on that
is registered after devm_pm_runtime_enable()
(so will maintain the ordering you have here).
> + pm_runtime_put_noidle(dev);
> + pm_runtime_disable(dev);
> +}
> +
> static int mlx90632_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
> {
> @@ -902,6 +1132,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 +1192,25 @@ 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);
> + 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);
> pm_runtime_set_autosuspend_delay(&client->dev, MLX90632_SLEEP_DELAY_MS);
> pm_runtime_use_autosuspend(&client->dev);
> + pm_runtime_put_autosuspend(&client->dev);
> +
> + ret = devm_add_action_or_reset(&client->dev, mlx90632_pm_disable, &client->dev);
Having moved those over to devm you need to also have dropped the calls in remove()
(I only noticed this whilst trying to fix the autosuspend issue above.)
> + if (ret) {
> + mlx90632_sleep(mlx90632);
> + return ret;
> + }
>
> return iio_device_register(indio_dev);
> }
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 2/3] iio: temperature: mlx90632 Read sampling frequency
2022-09-06 10:21 ` Andy Shevchenko
@ 2022-09-15 14:09 ` Jonathan Cameron
0 siblings, 0 replies; 16+ messages in thread
From: Jonathan Cameron @ 2022-09-15 14:09 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Crt Mori, linux-iio, Linux Kernel Mailing List
On Tue, 6 Sep 2022 13:21:36 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Tue, Sep 6, 2022 at 12:04 PM <cmo@melexis.com> wrote:
> >
> > 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 | 51 ++++++++++++++++++++++++++++++
> > 1 file changed, 51 insertions(+)
> >
> > diff --git a/drivers/iio/temperature/mlx90632.c b/drivers/iio/temperature/mlx90632.c
> > index e41a18edbc65..6b33284eb0b6 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
> > @@ -915,6 +918,24 @@ 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} };
>
> I would indent this as
> _freqs ... = {
> ...pairs...
> };
>
As you are going around again, I agree with Andy on this.
Burn a few lines for better readability!
> Either way,
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>
> > +
> > /**
> > * mlx90632_pm_interraction_wakeup() - Measure time between user interactions to change powermode
> > * @data: pointer to mlx90632_data object containing interaction_ts information
> > @@ -993,6 +1014,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;
> > @@ -1026,12 +1056,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,
> > @@ -1039,12 +1087,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 int mlx90632_sleep(struct mlx90632_data *data)
> > --
> > 2.34.1
> >
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/3] iio: temperature: mlx90632 Add runtime powermanagement modes
2022-09-15 14:07 ` Jonathan Cameron
@ 2022-09-15 14:35 ` Crt Mori
2022-09-16 15:22 ` Jonathan Cameron
0 siblings, 1 reply; 16+ messages in thread
From: Crt Mori @ 2022-09-15 14:35 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-iio, linux-kernel, Andy Shevchenko
> > + pm_runtime_get_sync(dev);
> So, this isn't quite enough.
>
> Take a look at what devm_pm_runtime_enable()
> does as the documentation for
> pm_runtime_use_autosuspend()
>
> I'd suggest using devm_pm_runtime_enable() and
> an additional callback to turn the device on that
> is registered after devm_pm_runtime_enable()
> (so will maintain the ordering you have here).
>
>
>
I copied this from pressure/bmp280-core driver. I had devm_pm
originally, but was asked to replace it.
> > + pm_runtime_put_noidle(dev);
> > + pm_runtime_disable(dev);
> > +}
> > +
> > static int mlx90632_probe(struct i2c_client *client,
> > const struct i2c_device_id *id)
> > {
> > @@ -902,6 +1132,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 +1192,25 @@ 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);
> > + 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);
> > pm_runtime_set_autosuspend_delay(&client->dev, MLX90632_SLEEP_DELAY_MS);
> > pm_runtime_use_autosuspend(&client->dev);
> > + pm_runtime_put_autosuspend(&client->dev);
> > +
> > + ret = devm_add_action_or_reset(&client->dev, mlx90632_pm_disable, &client->dev);
>
> Having moved those over to devm you need to also have dropped the calls in remove()
> (I only noticed this whilst trying to fix the autosuspend issue above.)
So in remove, there should be no pm calls, because mlx90632_pm_disable
function handle all of it? I still keep the sleep call, so that it
also puts the sensor in lowest state, or I rather keep it only in
regulator_disable (which should also be called at removal)?
> > + if (ret) {
> > + mlx90632_sleep(mlx90632);
> > + return ret;
> > + }
> >
> > return iio_device_register(indio_dev);
> > }
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/3] iio: temperature: mlx90632 Add runtime powermanagement modes
2022-09-15 14:35 ` Crt Mori
@ 2022-09-16 15:22 ` Jonathan Cameron
0 siblings, 0 replies; 16+ messages in thread
From: Jonathan Cameron @ 2022-09-16 15:22 UTC (permalink / raw)
To: Crt Mori; +Cc: Jonathan Cameron, linux-iio, linux-kernel, Andy Shevchenko
On Thu, 15 Sep 2022 16:35:33 +0200
Crt Mori <cmo@melexis.com> wrote:
> > > + pm_runtime_get_sync(dev);
> > So, this isn't quite enough.
> >
> > Take a look at what devm_pm_runtime_enable()
> > does as the documentation for
> > pm_runtime_use_autosuspend()
> >
> > I'd suggest using devm_pm_runtime_enable() and
> > an additional callback to turn the device on that
> > is registered after devm_pm_runtime_enable()
> > (so will maintain the ordering you have here).
> >
> >
Oops. I should have looked for a reply before responding to your v4.
> >
> I copied this from pressure/bmp280-core driver. I had devm_pm
> originally, but was asked to replace it.
It is a little odd to mix and match, but I think it makes sense in
this case. Can see why people might disagree (maybe it was me!)
>
> > > + pm_runtime_put_noidle(dev);
> > > + pm_runtime_disable(dev);
> > > +}
> > > +
> > > static int mlx90632_probe(struct i2c_client *client,
> > > const struct i2c_device_id *id)
> > > {
> > > @@ -902,6 +1132,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 +1192,25 @@ 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);
> > > + 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);
> > > pm_runtime_set_autosuspend_delay(&client->dev, MLX90632_SLEEP_DELAY_MS);
> > > pm_runtime_use_autosuspend(&client->dev);
> > > + pm_runtime_put_autosuspend(&client->dev);
> > > +
> > > + ret = devm_add_action_or_reset(&client->dev, mlx90632_pm_disable, &client->dev);
> >
> > Having moved those over to devm you need to also have dropped the calls in remove()
> > (I only noticed this whilst trying to fix the autosuspend issue above.)
>
> So in remove, there should be no pm calls, because mlx90632_pm_disable
> function handle all of it? I still keep the sleep call, so that it
> also puts the sensor in lowest state, or I rather keep it only in
> regulator_disable (which should also be called at removal)?
No, you still need some calls, because the devm_pm_runtime_enable()
registers a callback that only does part of what you have - it leaves
the device in an unknown state. If you want to power it up again so
that you can in turn switch it off cleanly (and hence handle the
non runtime pm case nicely) then you still need to do that.
The sleep is still useful because we may not have a controllable regulator.
In that case we want to go to a low power state anyway.
If we do have a controllable regulator, then sleeping followed by hitting
the power button does no harm.
Jonathan
>
> > > + if (ret) {
> > > + mlx90632_sleep(mlx90632);
> > > + return ret;
> > > + }
> > >
> > > return iio_device_register(indio_dev);
> > > }
> >
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2022-09-16 15:22 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-06 9:06 [PATCH v3 0/3] iio: temperature: mlx90632: Add powermanagement cmo
2022-09-06 9:04 ` [PATCH v3 1/3] iio: temperature: mlx90632 Add runtime powermanagement modes cmo
2022-09-06 10:20 ` Andy Shevchenko
2022-09-06 10:51 ` Crt Mori
2022-09-06 12:36 ` Andy Shevchenko
2022-09-06 13:04 ` Crt Mori
2022-09-06 13:16 ` Andy Shevchenko
2022-09-08 12:43 ` Jonathan Cameron
2022-09-15 14:07 ` Jonathan Cameron
2022-09-15 14:35 ` Crt Mori
2022-09-16 15:22 ` Jonathan Cameron
2022-09-06 9:04 ` [PATCH v3 2/3] iio: temperature: mlx90632 Read sampling frequency cmo
2022-09-06 10:21 ` Andy Shevchenko
2022-09-15 14:09 ` Jonathan Cameron
2022-09-06 9:04 ` [PATCH v3 3/3] iio: temperature: mlx90632 Change return value of sensor measurement channel cmo
2022-09-06 10:23 ` 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.