From: Crt Mori <[email protected]>
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 <[email protected]>
---
drivers/iio/temperature/mlx90632.c | 309 +++++++++++++++++++++++++----
1 file changed, 272 insertions(+), 37 deletions(-)
diff --git a/drivers/iio/temperature/mlx90632.c b/drivers/iio/temperature/mlx90632.c
index 549c0ab5c2be..45a1ab51fd0c 100644
--- a/drivers/iio/temperature/mlx90632.c
+++ b/drivers/iio/temperature/mlx90632.c
@@ -7,10 +7,12 @@
* Driver for the Melexis MLX90632 I2C 16-bit IR thermopile sensor
*/
#include <linux/delay.h>
+#include <linux/device.h>
#include <linux/err.h>
#include <linux/gpio/consumer.h>
#include <linux/i2c.h>
#include <linux/iopoll.h>
+#include <linux/jiffies.h>
#include <linux/kernel.h>
#include <linux/limits.h>
#include <linux/mod_devicetable.h>
@@ -55,6 +57,12 @@
#define MLX90632_EE_Ha 0x2481 /* Ha customer calib value reg 16bit */
#define MLX90632_EE_Hb 0x2482 /* Hb customer calib value reg 16bit */
+#define MLX90632_EE_MEDICAL_MEAS1 0x24E1 /* Medical measurement 1 16bit */
+#define MLX90632_EE_MEDICAL_MEAS2 0x24E2 /* Medical measurement 2 16bit */
+#define MLX90632_EE_EXTENDED_MEAS1 0x24F1 /* Extended measurement 1 16bit */
+#define MLX90632_EE_EXTENDED_MEAS2 0x24F2 /* Extended measurement 2 16bit */
+#define MLX90632_EE_EXTENDED_MEAS3 0x24F3 /* Extended measurement 3 16bit */
+
/* Register addresses - volatile */
#define MLX90632_REG_I2C_ADDR 0x3000 /* Chip I2C address register */
@@ -62,6 +70,7 @@
#define MLX90632_REG_CONTROL 0x3001 /* Control Register address */
#define MLX90632_CFG_PWR_MASK GENMASK(2, 1) /* PowerMode Mask */
#define MLX90632_CFG_MTYP_MASK GENMASK(8, 4) /* Meas select Mask */
+#define MLX90632_CFG_SOB_MASK BIT(11)
/* PowerModes statuses */
#define MLX90632_PWR_STATUS(ctrl_val) (ctrl_val << 1)
@@ -70,6 +79,8 @@
#define MLX90632_PWR_STATUS_STEP MLX90632_PWR_STATUS(2) /* step */
#define MLX90632_PWR_STATUS_CONTINUOUS MLX90632_PWR_STATUS(3) /* continuous*/
+#define MLX90632_EE_RR(ee_val) (ee_val & GENMASK(10, 8)) /* Only Refresh Rate bits*/
+
/* Measurement types */
#define MLX90632_MTYP_MEDICAL 0
#define MLX90632_MTYP_EXTENDED 17
@@ -116,8 +127,9 @@
#define MLX90632_REF_12 12LL /* ResCtrlRef value of Ch 1 or Ch 2 */
#define MLX90632_REF_3 12LL /* ResCtrlRef value of Channel 3 */
#define MLX90632_MAX_MEAS_NUM 31 /* Maximum measurements in list */
-#define MLX90632_SLEEP_DELAY_MS 3000 /* Autosleep delay */
+#define MLX90632_SLEEP_DELAY_MS 6000 /* Autosleep delay */
#define MLX90632_EXTENDED_LIMIT 27000 /* Extended mode raw value limit */
+#define MLX90632_MEAS_MAX_TIME 2000 /* Maximum measurement time in ms for the lowest possible refresh rate */
/**
* struct mlx90632_data - private data for the MLX90632 device
@@ -130,6 +142,9 @@
* @object_ambient_temperature: Ambient temperature at object (might differ of
* the ambient temperature of sensor.
* @regulator: Regulator of the device
+ * @powerstatus: Current POWER status of the device
+ * @interraction_timestamp: Timestamp of the last temperature read that is used
+ * for power management
*/
struct mlx90632_data {
struct i2c_client *client;
@@ -139,6 +154,8 @@ struct mlx90632_data {
u8 mtyp;
u32 object_ambient_temperature;
struct regulator *regulator;
+ int powerstatus;
+ unsigned long interraction_timestamp; /* in jiffies */
};
static const struct regmap_range mlx90632_volatile_reg_range[] = {
@@ -158,6 +175,8 @@ static const struct regmap_range mlx90632_read_reg_range[] = {
regmap_reg_range(MLX90632_EE_VERSION, MLX90632_EE_Ka),
regmap_reg_range(MLX90632_EE_CTRL, MLX90632_EE_I2C_ADDR),
regmap_reg_range(MLX90632_EE_Ha, MLX90632_EE_Hb),
+ regmap_reg_range(MLX90632_EE_MEDICAL_MEAS1, MLX90632_EE_MEDICAL_MEAS2),
+ regmap_reg_range(MLX90632_EE_EXTENDED_MEAS1, MLX90632_EE_EXTENDED_MEAS3),
regmap_reg_range(MLX90632_REG_I2C_ADDR, MLX90632_REG_CONTROL),
regmap_reg_range(MLX90632_REG_I2C_CMD, MLX90632_REG_I2C_CMD),
regmap_reg_range(MLX90632_REG_STATUS, MLX90632_REG_STATUS),
@@ -196,18 +215,42 @@ static const struct regmap_config mlx90632_regmap = {
.cache_type = REGCACHE_RBTREE,
};
+static int mlx90632_wakeup(struct mlx90632_data *data);
+
static s32 mlx90632_pwr_set_sleep_step(struct regmap *regmap)
{
- return regmap_update_bits(regmap, MLX90632_REG_CONTROL,
- MLX90632_CFG_PWR_MASK,
- MLX90632_PWR_STATUS_SLEEP_STEP);
+ struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(regmap_get_device(regmap)));
+ struct mlx90632_data *data = iio_priv(indio_dev);
+ s32 ret = 0;
+
+ if (data->powerstatus != MLX90632_PWR_STATUS_SLEEP_STEP) {
+ ret = regmap_write_bits(regmap, MLX90632_REG_CONTROL,
+ MLX90632_CFG_PWR_MASK,
+ MLX90632_PWR_STATUS_SLEEP_STEP);
+ if (ret < 0)
+ return ret;
+
+ data->powerstatus = MLX90632_PWR_STATUS_SLEEP_STEP;
+ }
+ return ret;
}
static s32 mlx90632_pwr_continuous(struct regmap *regmap)
{
- return regmap_update_bits(regmap, MLX90632_REG_CONTROL,
- MLX90632_CFG_PWR_MASK,
- MLX90632_PWR_STATUS_CONTINUOUS);
+ struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(regmap_get_device(regmap)));
+ struct mlx90632_data *data = iio_priv(indio_dev);
+ s32 ret = 0;
+
+ if (data->powerstatus != MLX90632_PWR_STATUS_CONTINUOUS) {
+ ret = regmap_write_bits(regmap, MLX90632_REG_CONTROL,
+ MLX90632_CFG_PWR_MASK,
+ MLX90632_PWR_STATUS_CONTINUOUS);
+ if (ret < 0)
+ return ret;
+
+ data->powerstatus = MLX90632_PWR_STATUS_CONTINUOUS;
+ }
+ return ret;
}
/**
@@ -219,6 +262,65 @@ static void mlx90632_reset_delay(void)
usleep_range(150, 200);
}
+static int mlx90632_get_measurement_time(struct regmap *regmap, u16 meas)
+{
+ int ret;
+ unsigned int reg;
+
+ ret = regmap_read(regmap, meas, ®);
+ if (ret < 0)
+ return ret;
+
+ reg = MLX90632_EE_RR(reg) >> 8;
+
+ return MLX90632_MEAS_MAX_TIME >> reg;
+}
+
+static int mlx90632_calculate_dataset_ready_time(struct mlx90632_data *data)
+{
+ int ret;
+ unsigned int refresh_time;
+
+ if (data->mtyp == MLX90632_MTYP_MEDICAL) {
+ ret = mlx90632_get_measurement_time(data->regmap,
+ MLX90632_EE_MEDICAL_MEAS1);
+ if (ret < 0)
+ return ret;
+
+ refresh_time = ret;
+
+ ret = mlx90632_get_measurement_time(data->regmap,
+ MLX90632_EE_MEDICAL_MEAS2);
+ if (ret < 0)
+ return ret;
+
+ refresh_time = refresh_time + ret;
+ } else {
+ ret = mlx90632_get_measurement_time(data->regmap,
+ MLX90632_EE_EXTENDED_MEAS1);
+ if (ret < 0)
+ return ret;
+
+ refresh_time = ret;
+
+ ret = mlx90632_get_measurement_time(data->regmap,
+ MLX90632_EE_EXTENDED_MEAS2);
+ if (ret < 0)
+ return ret;
+
+ refresh_time = refresh_time + ret;
+
+ ret = mlx90632_get_measurement_time(data->regmap,
+ MLX90632_EE_EXTENDED_MEAS3);
+ if (ret < 0)
+ return ret;
+
+ refresh_time = refresh_time + ret;
+ }
+
+ return refresh_time;
+}
+
/**
* mlx90632_perform_measurement() - Trigger and retrieve current measurement cycle
* @data: pointer to mlx90632_data object containing regmap information
@@ -249,26 +351,76 @@ static int mlx90632_perform_measurement(struct mlx90632_data *data)
return (reg_status & MLX90632_STAT_CYCLE_POS) >> 2;
}
-static int mlx90632_set_meas_type(struct regmap *regmap, u8 type)
+/**
+ * mlx90632_perform_measurement_burst() - Trigger and retrieve current measurement
+ * cycle in step sleep mode
+ * @data: pointer to mlx90632_data object containing regmap information
+ *
+ * Perform a measurement and return 2 as measurement cycle position reported
+ * by sensor. This is a blocking function for amount dependent on the sensor
+ * refresh rate.
+ */
+static int mlx90632_perform_measurement_burst(struct mlx90632_data *data)
{
+ unsigned int reg_status;
int ret;
- if ((type != MLX90632_MTYP_MEDICAL) && (type != MLX90632_MTYP_EXTENDED))
- return -EINVAL;
+ ret = regmap_write_bits(data->regmap, MLX90632_REG_CONTROL,
+ MLX90632_CFG_SOB_MASK, MLX90632_CFG_SOB_MASK);
+ if (ret < 0)
+ return ret;
+
+ ret = mlx90632_calculate_dataset_ready_time(data);
+ if (ret < 0)
+ return ret;
+
+ msleep(ret); /* Wait minimum time for dataset to be ready */
+
+ ret = regmap_read_poll_timeout(data->regmap, MLX90632_REG_STATUS,
+ reg_status,
+ ((reg_status & MLX90632_STAT_BUSY) == 0),
+ 10000, 100 * 10000);
+ if (ret < 0) {
+ dev_err(&data->client->dev, "data not ready");
+ return -ETIMEDOUT;
+ }
+
+ return 2;
+}
+
- ret = regmap_write(regmap, MLX90632_REG_I2C_CMD, MLX90632_RESET_CMD);
+static int mlx90632_set_meas_type(struct mlx90632_data *data, u8 type)
+{
+ int ret;
+ int current_powerstatus = data->powerstatus;
+
+ if (data->mtyp == type) {
+ return 0;
+ }
+
+ ret = mlx90632_pwr_continuous(data->regmap);
+ if (ret < 0)
+ return ret;
+
+ ret = regmap_write(data->regmap, MLX90632_REG_I2C_CMD, MLX90632_RESET_CMD);
if (ret < 0)
return ret;
mlx90632_reset_delay();
- ret = regmap_write_bits(regmap, MLX90632_REG_CONTROL,
+ ret = regmap_update_bits(data->regmap, MLX90632_REG_CONTROL,
(MLX90632_CFG_MTYP_MASK | MLX90632_CFG_PWR_MASK),
(MLX90632_MTYP_STATUS(type) | MLX90632_PWR_STATUS_HALT));
if (ret < 0)
return ret;
- return mlx90632_pwr_continuous(regmap);
+ data->mtyp = type;
+ data->powerstatus = MLX90632_PWR_STATUS_HALT;
+
+ if (current_powerstatus == MLX90632_PWR_STATUS_SLEEP_STEP)
+ return mlx90632_pwr_set_sleep_step(data->regmap);
+ else
+ return mlx90632_pwr_continuous(data->regmap);
}
static int mlx90632_channel_new_select(int perform_ret, uint8_t *channel_new,
@@ -355,11 +507,24 @@ static int mlx90632_read_all_channel(struct mlx90632_data *data,
s32 ret, measurement;
mutex_lock(&data->lock);
- measurement = mlx90632_perform_measurement(data);
- if (measurement < 0) {
- ret = measurement;
+ ret = mlx90632_set_meas_type(data, MLX90632_MTYP_MEDICAL);
+ if (ret < 0)
goto read_unlock;
+
+ if (data->powerstatus == MLX90632_PWR_STATUS_CONTINUOUS) {
+ measurement = mlx90632_perform_measurement(data);
+ if (measurement < 0) {
+ ret = measurement;
+ goto read_unlock;
+ }
+ } else if (data->powerstatus == MLX90632_PWR_STATUS_SLEEP_STEP) {
+ measurement = mlx90632_perform_measurement_burst(data);
+ if (measurement < 0) {
+ ret = measurement;
+ goto read_unlock;
+ }
}
+
ret = mlx90632_read_ambient_raw(data->regmap, ambient_new_raw,
ambient_old_raw);
if (ret < 0)
@@ -441,14 +606,23 @@ static int mlx90632_read_all_channel_extended(struct mlx90632_data *data, s16 *o
s32 ret, meas;
mutex_lock(&data->lock);
- ret = mlx90632_set_meas_type(data->regmap, MLX90632_MTYP_EXTENDED);
+ ret = mlx90632_set_meas_type(data, MLX90632_MTYP_EXTENDED);
if (ret < 0)
goto read_unlock;
- ret = read_poll_timeout(mlx90632_perform_measurement, meas, meas == 19,
- 50000, 800000, false, data);
- if (ret != 0)
- goto read_unlock;
+ if (data->powerstatus == MLX90632_PWR_STATUS_CONTINUOUS) {
+ ret = read_poll_timeout(mlx90632_perform_measurement, meas, meas == 19,
+ 50000, 800000, false, data);
+ if (ret != 0)
+ goto read_unlock;
+
+ } else if (data->powerstatus == MLX90632_PWR_STATUS_SLEEP_STEP) {
+ ret = mlx90632_perform_measurement_burst(data);
+ if (ret < 0) {
+ goto read_unlock;
+ }
+ }
+
ret = mlx90632_read_object_raw_extended(data->regmap, object_new_raw);
if (ret < 0)
@@ -457,8 +631,6 @@ static int mlx90632_read_all_channel_extended(struct mlx90632_data *data, s16 *o
ret = mlx90632_read_ambient_raw_extended(data->regmap, ambient_new_raw, ambient_old_raw);
read_unlock:
- (void) mlx90632_set_meas_type(data->regmap, MLX90632_MTYP_MEDICAL);
-
mutex_unlock(&data->lock);
return ret;
}
@@ -743,12 +915,39 @@ static int mlx90632_calc_ambient_dsp105(struct mlx90632_data *data, int *val)
return ret;
}
+static int mlx90632_pm_interraction_wakeup(struct mlx90632_data *data)
+{
+ unsigned long now;
+ int ret = 0;
+
+ now = jiffies;
+ if (time_in_range(now, data->interraction_timestamp,
+ data->interraction_timestamp +
+ msecs_to_jiffies(MLX90632_MEAS_MAX_TIME + 100))) {
+ if (data->powerstatus == MLX90632_PWR_STATUS_SLEEP_STEP) {
+ ret = mlx90632_pwr_continuous(data->regmap);
+ if (ret < 0)
+ return ret;
+ }
+ }
+
+ data->interraction_timestamp = now;
+
+ return ret;
+}
+
static int mlx90632_read_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *channel, int *val,
int *val2, long mask)
{
struct mlx90632_data *data = iio_priv(indio_dev);
int ret;
+ int cr;
+
+ pm_runtime_get_sync(&data->client->dev);
+ ret = mlx90632_pm_interraction_wakeup(data);
+ if (ret < 0)
+ goto mlx90632_read_raw_pm;
switch (mask) {
case IIO_CHAN_INFO_PROCESSED:
@@ -756,16 +955,22 @@ static int mlx90632_read_raw(struct iio_dev *indio_dev,
case IIO_MOD_TEMP_AMBIENT:
ret = mlx90632_calc_ambient_dsp105(data, val);
if (ret < 0)
- return ret;
- return IIO_VAL_INT;
+ goto mlx90632_read_raw_pm;
+
+ ret = IIO_VAL_INT;
+ break;
case IIO_MOD_TEMP_OBJECT:
ret = mlx90632_calc_object_dsp105(data, val);
if (ret < 0)
- return ret;
- return IIO_VAL_INT;
+ goto mlx90632_read_raw_pm;
+
+ ret = IIO_VAL_INT;
+ break;
default:
- return -EINVAL;
+ ret = -EINVAL;
+ break;
}
+ break;
case IIO_CHAN_INFO_CALIBEMISSIVITY:
if (data->emissivity == 1000) {
*val = 1;
@@ -774,13 +979,22 @@ static int mlx90632_read_raw(struct iio_dev *indio_dev,
*val = 0;
*val2 = data->emissivity * 1000;
}
- return IIO_VAL_INT_PLUS_MICRO;
+ ret = IIO_VAL_INT_PLUS_MICRO;
+ break;
case IIO_CHAN_INFO_CALIBAMBIENT:
*val = data->object_ambient_temperature;
- return IIO_VAL_INT;
+ ret = IIO_VAL_INT;
+ break;
default:
- return -EINVAL;
+ ret = -EINVAL;
+ break;
}
+
+mlx90632_read_raw_pm:
+ mutex_unlock(&data->lock);
+ pm_runtime_mark_last_busy(&data->client->dev);
+ pm_runtime_put_autosuspend(&data->client->dev);
+ return ret;
}
static int mlx90632_write_raw(struct iio_dev *indio_dev,
@@ -902,6 +1116,7 @@ static int mlx90632_probe(struct i2c_client *client,
mlx90632->client = client;
mlx90632->regmap = regmap;
mlx90632->mtyp = MLX90632_MTYP_MEDICAL;
+ mlx90632->powerstatus = MLX90632_PWR_STATUS_HALT;
mutex_init(&mlx90632->lock);
indio_dev->name = id->name;
@@ -961,16 +1176,18 @@ static int mlx90632_probe(struct i2c_client *client,
mlx90632->emissivity = 1000;
mlx90632->object_ambient_temperature = 25000; /* 25 degrees milliCelsius */
+ mlx90632->interraction_timestamp = jiffies; /* Set initial value */
- pm_runtime_disable(&client->dev);
+ pm_runtime_get_noresume(&client->dev);
ret = pm_runtime_set_active(&client->dev);
if (ret < 0) {
mlx90632_sleep(mlx90632);
return ret;
}
- pm_runtime_enable(&client->dev);
+ devm_pm_runtime_enable(&client->dev);
pm_runtime_set_autosuspend_delay(&client->dev, MLX90632_SLEEP_DELAY_MS);
pm_runtime_use_autosuspend(&client->dev);
+ pm_runtime_put(&client->dev);
return iio_device_register(indio_dev);
}
@@ -1007,8 +1224,14 @@ static int __maybe_unused mlx90632_pm_suspend(struct device *dev)
{
struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
struct mlx90632_data *data = iio_priv(indio_dev);
+ int ret;
- return mlx90632_sleep(data);
+ ret = regulator_disable(data->regulator);
+ if (ret < 0)
+ dev_err(regmap_get_device(data->regmap),
+ "Failed to disable power regulator: %d\n", ret);
+
+ return ret;
}
static int __maybe_unused mlx90632_pm_resume(struct device *dev)
@@ -1016,11 +1239,23 @@ static int __maybe_unused mlx90632_pm_resume(struct device *dev)
struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
struct mlx90632_data *data = iio_priv(indio_dev);
- return mlx90632_wakeup(data);
+ return mlx90632_enable_regulator(data);
}
-static UNIVERSAL_DEV_PM_OPS(mlx90632_pm_ops, mlx90632_pm_suspend,
- mlx90632_pm_resume, NULL);
+static int __maybe_unused mlx90632_pm_runtime_suspend(struct device *dev)
+{
+ struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+ struct mlx90632_data *data = iio_priv(indio_dev);
+
+ return mlx90632_pwr_set_sleep_step(data->regmap);
+}
+
+const struct dev_pm_ops mlx90632_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(mlx90632_pm_suspend, mlx90632_pm_resume)
+ SET_RUNTIME_PM_OPS(mlx90632_pm_runtime_suspend,
+ NULL, NULL)
+};
+EXPORT_SYMBOL_GPL(mlx90632_pm_ops);
static struct i2c_driver mlx90632_driver = {
.driver = {
--
2.34.1
On Fri, Sep 2, 2022 at 4:13 PM <[email protected]> wrote:
>
> From: Crt Mori <[email protected]>
>
> Sensor can operate in lower power modes and even make measurements when
The sensor
...or..
Sensors
> in those lower powered modes. The decision was taken that if measurement
> is not requested within 2 seconds the sensor will remain in SLEEP_STEP
> power mode, where measurements are triggered on request with setting the
> start of measurement bit (SOB). In this mode the measurements are taking
> a bit longer because we need to start it and complete it. Currently, in
> continuous mode we read ready data and this mode is activated if sensor
> measurement is requested within 2 seconds. The suspend timeout is
> increased to 6 seconds (instead of 3 before), because that enables more
> measurements in lower power mode (SLEEP_STEP), with the lowest refresh
> rate (2 seconds).
...
> #define MLX90632_PWR_STATUS_CONTINUOUS MLX90632_PWR_STATUS(3) /* continuous*/
>
> +#define MLX90632_EE_RR(ee_val) (ee_val & GENMASK(10, 8)) /* Only Refresh Rate bits*/
Missed space. Seems like a copy'n'paste from previous comments that
lacks the space as well.
...
> + unsigned long interraction_timestamp; /* in jiffies */
_ts for timestamp is a fine abbreviation. Also move comment to the kernel doc.
...
> +static int mlx90632_wakeup(struct mlx90632_data *data);
Can we avoid forward declaration? (I don't even see how it is used
among dozens of lines of below code in the patch)
> static s32 mlx90632_pwr_set_sleep_step(struct regmap *regmap)
> {
> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(regmap_get_device(regmap)));
Why ping-ponging here and not using dev_get_drvdata()? Ditto for similar cases.
> + struct mlx90632_data *data = iio_priv(indio_dev);
> + s32 ret = 0;
Assignment is not needed, use 'return 0;' directly. Ditto for all
cases like this.
> + if (data->powerstatus != MLX90632_PWR_STATUS_SLEEP_STEP) {
> + ret = regmap_write_bits(regmap, MLX90632_REG_CONTROL,
> + MLX90632_CFG_PWR_MASK,
> + MLX90632_PWR_STATUS_SLEEP_STEP);
> + if (ret < 0)
> + return ret;
> +
> + data->powerstatus = MLX90632_PWR_STATUS_SLEEP_STEP;
> + }
> + return ret;
> }
...
> + reg = MLX90632_EE_RR(reg) >> 8;
This makes it harder to understand the semantics of reg, can we simply
unite this line with the below?
> + return MLX90632_MEAS_MAX_TIME >> reg;
...
> + refresh_time = refresh_time + ret;
+= ?
...
> + refresh_time = refresh_time + ret;
+=
...
> + refresh_time = refresh_time + ret;
Ditto.
...
> + unsigned int reg_status;
> int ret;
Keep the reversed xmas tree order (like here!) elsewhere for the sake
of consistency.
...
> + ret = regmap_read_poll_timeout(data->regmap, MLX90632_REG_STATUS,
> + reg_status,
> + ((reg_status & MLX90632_STAT_BUSY) == 0),
Too many parentheses
> + 10000, 100 * 10000);
> + if (ret < 0) {
> + dev_err(&data->client->dev, "data not ready");
> + return -ETIMEDOUT;
> + }
...
> + int current_powerstatus = data->powerstatus;
Please, split the assignment and move it closer to the first user.
...
> + data->powerstatus = MLX90632_PWR_STATUS_HALT;
> +
> + if (current_powerstatus == MLX90632_PWR_STATUS_SLEEP_STEP)
> + return mlx90632_pwr_set_sleep_step(data->regmap);
> + else
Redundant.
> + return mlx90632_pwr_continuous(data->regmap);
...
> + ret = read_poll_timeout(mlx90632_perform_measurement, meas, meas == 19,
> + 50000, 800000, false, data);
> + if (ret != 0)
Drop this ' != 0' part. It's useless.
> + goto read_unlock;
> +
Seems redundant blank line.
...
> + }
> +
>
Ditto.
...
> + int ret = 0;
Redundant assignment. Use return 0; directly.
...
> + if (time_in_range(now, data->interraction_timestamp,
> + data->interraction_timestamp +
> + msecs_to_jiffies(MLX90632_MEAS_MAX_TIME + 100))) {
With a local variable you will have better to read code.
> + }
...
> struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
Maybe a separate patch to drop these here-there dereferences...
...
> +static int __maybe_unused mlx90632_pm_runtime_suspend(struct device *dev)
No __maybe_unused, use pm_ptr() / pm_sleep_ptr() below.
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> + struct mlx90632_data *data = iio_priv(indio_dev);
> +
> + return mlx90632_pwr_set_sleep_step(data->regmap);
> +}
> +
> +const struct dev_pm_ops mlx90632_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(mlx90632_pm_suspend, mlx90632_pm_resume)
> + SET_RUNTIME_PM_OPS(mlx90632_pm_runtime_suspend,
> + NULL, NULL)
Please, use new macros from pm.h / runtime_pm.h
> +};
> +EXPORT_SYMBOL_GPL(mlx90632_pm_ops);
Can we use special EXPORT macro from pm.h
--
With Best Regards,
Andy Shevchenko
Hi Andy,
Thanks for the review. I have few questions about your remarks below.
On Fri, 2 Sept 2022 at 17:28, Andy Shevchenko <[email protected]> wrote:
>
> On Fri, Sep 2, 2022 at 4:13 PM <[email protected]> wrote:
> >
> > From: Crt Mori <[email protected]>
> >
> > Sensor can operate in lower power modes and even make measurements when
>
> The sensor
> ...or..
> Sensors
>
> > in those lower powered modes. The decision was taken that if measurement
> > is not requested within 2 seconds the sensor will remain in SLEEP_STEP
> > power mode, where measurements are triggered on request with setting the
> > start of measurement bit (SOB). In this mode the measurements are taking
> > a bit longer because we need to start it and complete it. Currently, in
> > continuous mode we read ready data and this mode is activated if sensor
> > measurement is requested within 2 seconds. The suspend timeout is
> > increased to 6 seconds (instead of 3 before), because that enables more
> > measurements in lower power mode (SLEEP_STEP), with the lowest refresh
> > rate (2 seconds).
>
> ...
>
> > #define MLX90632_PWR_STATUS_CONTINUOUS MLX90632_PWR_STATUS(3) /* continuous*/
> >
> > +#define MLX90632_EE_RR(ee_val) (ee_val & GENMASK(10, 8)) /* Only Refresh Rate bits*/
>
> Missed space. Seems like a copy'n'paste from previous comments that
> lacks the space as well.
>
> ...
>
> > + unsigned long interraction_timestamp; /* in jiffies */
>
> _ts for timestamp is a fine abbreviation. Also move comment to the kernel doc.
>
> ...
>
> > +static int mlx90632_wakeup(struct mlx90632_data *data);
>
> Can we avoid forward declaration? (I don't even see how it is used
> among dozens of lines of below code in the patch)
>
This is existing function that I did not want to move upwards. Should
I have just moved it rather?
> > static s32 mlx90632_pwr_set_sleep_step(struct regmap *regmap)
> > {
>
> > + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(regmap_get_device(regmap)));
>
> Why ping-ponging here and not using dev_get_drvdata()? Ditto for similar cases.
>
> > + struct mlx90632_data *data = iio_priv(indio_dev);
> > + s32 ret = 0;
>
> Assignment is not needed, use 'return 0;' directly. Ditto for all
> cases like this.
>
This is used, because when powerstatus is not equal to sleep_step it
returns, otherwise the ret is changed in the function.
> > + if (data->powerstatus != MLX90632_PWR_STATUS_SLEEP_STEP) {
> > + ret = regmap_write_bits(regmap, MLX90632_REG_CONTROL,
> > + MLX90632_CFG_PWR_MASK,
> > + MLX90632_PWR_STATUS_SLEEP_STEP);
> > + if (ret < 0)
> > + return ret;
> > +
> > + data->powerstatus = MLX90632_PWR_STATUS_SLEEP_STEP;
> > + }
> > + return ret;
> > }
>
> ...
>
> > + reg = MLX90632_EE_RR(reg) >> 8;
>
> This makes it harder to understand the semantics of reg, can we simply
> unite this line with the below?
>
I find it easier to have it split but I can make one long statement.
> > + return MLX90632_MEAS_MAX_TIME >> reg;
>
> ...
>
> > + refresh_time = refresh_time + ret;
>
> += ?
>
> ...
>
> > + refresh_time = refresh_time + ret;
>
> +=
>
> ...
>
> > + refresh_time = refresh_time + ret;
>
> Ditto.
>
> ...
>
> > + unsigned int reg_status;
> > int ret;
>
> Keep the reversed xmas tree order (like here!) elsewhere for the sake
> of consistency.
>
> ...
>
> > + ret = regmap_read_poll_timeout(data->regmap, MLX90632_REG_STATUS,
> > + reg_status,
> > + ((reg_status & MLX90632_STAT_BUSY) == 0),
>
> Too many parentheses
>
I like the outer parentheses it shows that it is a break condition. I
have same in another function in this file.
> > + 10000, 100 * 10000);
> > + if (ret < 0) {
> > + dev_err(&data->client->dev, "data not ready");
> > + return -ETIMEDOUT;
> > + }
>
> ...
>
> > + int current_powerstatus = data->powerstatus;
>
> Please, split the assignment and move it closer to the first user.
>
> ...
>
> > + data->powerstatus = MLX90632_PWR_STATUS_HALT;
> > +
> > + if (current_powerstatus == MLX90632_PWR_STATUS_SLEEP_STEP)
> > + return mlx90632_pwr_set_sleep_step(data->regmap);
>
> > + else
>
> Redundant.
>
No, the powermode changes among the type.
> > + return mlx90632_pwr_continuous(data->regmap);
>
> ...
>
> > + ret = read_poll_timeout(mlx90632_perform_measurement, meas, meas == 19,
> > + 50000, 800000, false, data);
> > + if (ret != 0)
>
> Drop this ' != 0' part. It's useless.
>
> > + goto read_unlock;
>
> > +
>
> Seems redundant blank line.
>
> ...
>
> > + }
> > +
> >
>
> Ditto.
>
> ...
>
> > + int ret = 0;
>
> Redundant assignment. Use return 0; directly.
>
> ...
>
> > + if (time_in_range(now, data->interraction_timestamp,
> > + data->interraction_timestamp +
>
> > + msecs_to_jiffies(MLX90632_MEAS_MAX_TIME + 100))) {
>
> With a local variable you will have better to read code.
>
> > + }
>
>
> ...
>
> > struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>
> Maybe a separate patch to drop these here-there dereferences...
>
> ...
>
> > +static int __maybe_unused mlx90632_pm_runtime_suspend(struct device *dev)
>
> No __maybe_unused, use pm_ptr() / pm_sleep_ptr() below.
>
Care to explain a bit more about this? I just followed what other
drivers have...
> > +{
> > + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> > + struct mlx90632_data *data = iio_priv(indio_dev);
> > +
> > + return mlx90632_pwr_set_sleep_step(data->regmap);
> > +}
> > +
> > +const struct dev_pm_ops mlx90632_pm_ops = {
> > + SET_SYSTEM_SLEEP_PM_OPS(mlx90632_pm_suspend, mlx90632_pm_resume)
> > + SET_RUNTIME_PM_OPS(mlx90632_pm_runtime_suspend,
> > + NULL, NULL)
>
> Please, use new macros from pm.h / runtime_pm.h
>
> > +};
> > +EXPORT_SYMBOL_GPL(mlx90632_pm_ops);
>
> Can we use special EXPORT macro from pm.h
>
> --
> With Best Regards,
> Andy Shevchenko
On Fri, Sep 2, 2022 at 8:59 PM Crt Mori <[email protected]> wrote:
> On Fri, 2 Sept 2022 at 17:28, Andy Shevchenko <[email protected]> wrote:
> > On Fri, Sep 2, 2022 at 4:13 PM <[email protected]> wrote:
For future replies, please remove unnecessary context when replying
(I'm wasting my time for that)!
...
> > > +static int mlx90632_wakeup(struct mlx90632_data *data);
> >
> > Can we avoid forward declaration? (I don't even see how it is used
> > among dozens of lines of below code in the patch)
> >
> This is existing function that I did not want to move upwards. Should
> I have just moved it rather?
Yes, move it in the preparatory (separate) patch.
...
> > > + s32 ret = 0;
> >
> > Assignment is not needed, use 'return 0;' directly. Ditto for all
> > cases like this.
> >
> This is used, because when powerstatus is not equal to sleep_step it
> returns, otherwise the ret is changed in the function.
Please, read what I have suggested.
> > > + if (data->powerstatus != MLX90632_PWR_STATUS_SLEEP_STEP) {
> > > + ret = regmap_write_bits(regmap, MLX90632_REG_CONTROL,
> > > + MLX90632_CFG_PWR_MASK,
> > > + MLX90632_PWR_STATUS_SLEEP_STEP);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + data->powerstatus = MLX90632_PWR_STATUS_SLEEP_STEP;
> > > + }
> > > + return ret;
For your convenience...
return 0;
...here.
Or do you state that ret can be a positive number? In such cases,
elaboration is required.
...
> > > + reg = MLX90632_EE_RR(reg) >> 8;
> >
> > This makes it harder to understand the semantics of reg, can we simply
> > unite this line with the below?
> >
> I find it easier to have it split but I can make one long statement.
> > > + return MLX90632_MEAS_MAX_TIME >> reg;
If so, you need another variable with better naming to show what is in it.
...
> > > + ret = regmap_read_poll_timeout(data->regmap, MLX90632_REG_STATUS,
> > > + reg_status,
> > > + ((reg_status & MLX90632_STAT_BUSY) == 0),
> >
> > Too many parentheses
> >
> I like the outer parentheses it shows that it is a break condition. I
> have same in another function in this file.
It's not a Lisp, we don't need to pollute code with unneeded obstacles.
> > > + 10000, 100 * 10000);
...
> > > + if (current_powerstatus == MLX90632_PWR_STATUS_SLEEP_STEP)
> > > + return mlx90632_pwr_set_sleep_step(data->regmap);
> >
> > > + else
> >
> > Redundant.
> >
> No, the powermode changes among the type.
Yes. 'else' keyword is always redundant in the
if (...)
return / break / continue / goto
else
cases.
> > > + return mlx90632_pwr_continuous(data->regmap);
...
> > > +static int __maybe_unused mlx90632_pm_runtime_suspend(struct device *dev)
> >
> > No __maybe_unused, use pm_ptr() / pm_sleep_ptr() below.
> >
> Care to explain a bit more about this? I just followed what other
> drivers have...
And other drivers have what I said, but it's a new feature.
If you run `git log --no-merges --grep 'pm_ptr' -- drivers/iio
include/linux/` and read the history it will explain the case.
--
With Best Regards,
Andy Shevchenko
On Fri, 2 Sept 2022 at 20:39, Andy Shevchenko <[email protected]> wrote:
>
> On Fri, Sep 2, 2022 at 8:59 PM Crt Mori <[email protected]> wrote:
> > On Fri, 2 Sept 2022 at 17:28, Andy Shevchenko <[email protected]> wrote:
> > > On Fri, Sep 2, 2022 at 4:13 PM <[email protected]> wrote:
> > > > + if (current_powerstatus == MLX90632_PWR_STATUS_SLEEP_STEP)
> > > > + return mlx90632_pwr_set_sleep_step(data->regmap);
> > >
> > > > + else
> > >
> > > Redundant.
> > >
> > No, the powermode changes among the type.
>
> Yes. 'else' keyword is always redundant in the
>
> if (...)
> return / break / continue / goto
> else
>
> cases.
>
In this case current power mode of the sensor is halt, so the else is
needed to set it to continuous mode, which means I can't just remove
the else here because this statement restores the power mode before
this function was entered (and changed powermode for the setup).
> > > > + return mlx90632_pwr_continuous(data->regmap);
>
> ...
>
> > > > +static int __maybe_unused mlx90632_pm_runtime_suspend(struct device *dev)
> > >
> > > No __maybe_unused, use pm_ptr() / pm_sleep_ptr() below.
> > >
> > Care to explain a bit more about this? I just followed what other
> > drivers have...
>
> And other drivers have what I said, but it's a new feature.
> If you run `git log --no-merges --grep 'pm_ptr' -- drivers/iio
> include/linux/` and read the history it will explain the case.
>
Thanks for the hint.
On Sat, Sep 3, 2022 at 3:06 PM Crt Mori <[email protected]> wrote:
> On Fri, 2 Sept 2022 at 20:39, Andy Shevchenko <[email protected]> wrote:
> > On Fri, Sep 2, 2022 at 8:59 PM Crt Mori <[email protected]> wrote:
> > > On Fri, 2 Sept 2022 at 17:28, Andy Shevchenko <[email protected]> wrote:
> > > > On Fri, Sep 2, 2022 at 4:13 PM <[email protected]> wrote:
...
> > > > > + if (current_powerstatus == MLX90632_PWR_STATUS_SLEEP_STEP)
> > > > > + return mlx90632_pwr_set_sleep_step(data->regmap);
> > > >
> > > > > + else
> > > >
> > > > Redundant.
> > > >
> > > No, the powermode changes among the type.
> >
> > Yes. 'else' keyword is always redundant in the
> >
> > if (...)
> > return / break / continue / goto
> > else
> > cases.
> >
> In this case current power mode of the sensor is halt, so the else is
> needed to set it to continuous mode, which means I can't just remove
> the else here because this statement restores the power mode before
> this function was entered (and changed powermode for the setup).
What do you mean by all this? Read your code again and see what I'm
talking about...
> > > > > + return mlx90632_pwr_continuous(data->regmap);
--
With Best Regards,
Andy Shevchenko
On Sat, 3 Sep 2022 14:05:38 +0200
Crt Mori <[email protected]> wrote:
> On Fri, 2 Sept 2022 at 20:39, Andy Shevchenko <[email protected]> wrote:
> >
> > On Fri, Sep 2, 2022 at 8:59 PM Crt Mori <[email protected]> wrote:
> > > On Fri, 2 Sept 2022 at 17:28, Andy Shevchenko <[email protected]> wrote:
> > > > On Fri, Sep 2, 2022 at 4:13 PM <[email protected]> wrote:
> > > > > + if (current_powerstatus == MLX90632_PWR_STATUS_SLEEP_STEP)
> > > > > + return mlx90632_pwr_set_sleep_step(data->regmap);
> > > >
> > > > > + else
> > > >
> > > > Redundant.
> > > >
> > > No, the powermode changes among the type.
> >
> > Yes. 'else' keyword is always redundant in the
> >
> > if (...)
> > return / break / continue / goto
> > else
> >
> > cases.
> >
> In this case current power mode of the sensor is halt, so the else is
> needed to set it to continuous mode, which means I can't just remove
> the else here because this statement restores the power mode before
> this function was entered (and changed powermode for the setup).
>
> > > > > + return mlx90632_pwr_continuous(data->regmap);
> >
> > ...
> >
> > > > > +static int __maybe_unused mlx90632_pm_runtime_suspend(struct device *dev)
> > > >
> > > > No __maybe_unused, use pm_ptr() / pm_sleep_ptr() below.
> > > >
> > > Care to explain a bit more about this? I just followed what other
> > > drivers have...
> >
> > And other drivers have what I said, but it's a new feature.
> > If you run `git log --no-merges --grep 'pm_ptr' -- drivers/iio
> > include/linux/` and read the history it will explain the case.
> >
> Thanks for the hint.
The relevant EXPORT_ for this particular case isn't upstream yet
We had a proposal on IIO list, but there was a better one as part of
cleaning this up for MFD. I haven't checked if there is a suitable
immutable branch for that patch yet...
Jonathan
On Sun, Sep 4, 2022 at 5:37 PM Jonathan Cameron <[email protected]> wrote:
> On Sat, 3 Sep 2022 14:05:38 +0200
> Crt Mori <[email protected]> wrote:
> > On Fri, 2 Sept 2022 at 20:39, Andy Shevchenko <[email protected]> wrote:
> > > On Fri, Sep 2, 2022 at 8:59 PM Crt Mori <[email protected]> wrote:
> > > > On Fri, 2 Sept 2022 at 17:28, Andy Shevchenko <[email protected]> wrote:
> > > > > On Fri, Sep 2, 2022 at 4:13 PM <[email protected]> wrote:
...
> > > > > > +static int __maybe_unused mlx90632_pm_runtime_suspend(struct device *dev)
> > > > >
> > > > > No __maybe_unused, use pm_ptr() / pm_sleep_ptr() below.
> > > > >
> > > > Care to explain a bit more about this? I just followed what other
> > > > drivers have...
> > >
> > > And other drivers have what I said, but it's a new feature.
> > > If you run `git log --no-merges --grep 'pm_ptr' -- drivers/iio
> > > include/linux/` and read the history it will explain the case.
> > >
> > Thanks for the hint.
>
> The relevant EXPORT_ for this particular case isn't upstream yet
> We had a proposal on IIO list, but there was a better one as part of
> cleaning this up for MFD. I haven't checked if there is a suitable
> immutable branch for that patch yet...
Oh, well to know. But in any case pm_ptr() / DEFINE_*PM_OPS can still be used.
--
With Best Regards,
Andy Shevchenko