iio: temperature: mlx90635 Driver for MLX90635 IR temperature sensor
Subsystems (3)
Crt Mori
maintainerv2 cover iio: temperature: mlx90635 Driver for MLX90635 IR temperature sensor
Hi everybody,
There is a new contactless sensor in Melexis portfolio. MLX90635 is just 1.8x1.8mm in size, but with factory calibration offers instant usage in every project. It offers wide refresh rate range that is configurable between 100ms and 4s.
Driver currently provides temperature calculations, power management and changes to the refresh rate. Since sensor is aimed towards the consumer market there is really low number of EEPROM write cycles available, so driver changes refresh rate only in run time registers to avoid writing to EEPROM. Reading EEPROM is not available in Sleep Step mode, so I am using caching at the driver initialization to ensure that measurements can still be taken in Sleep Step mode.
Changes v1 -> v2:
- dt-bindings are now enum according to the comments from Krzysztof Kozlowski
- expand the path recipient list for the patches using get_maintainers.pl
- driver cache and probe (and some style) adjusted according to comments from Jonathan Cameron
- improve DSP version detection (make it clearer)
Crt Mori (2):
iio: temperature: mlx90635 MLX90635 IR Temperature sensor
dt-bindings: iio: temperature: add MLX90635 device
.../iio/temperature/melexis,mlx90632.yaml | 19 +- MAINTAINERS | 7 + drivers/iio/temperature/Kconfig | 12 + drivers/iio/temperature/Makefile | 1 + drivers/iio/temperature/mlx90635.c | 1071 +++++++++++++++++ 5 files changed, 1108 insertions(+), 2 deletions(-) create mode 100644 drivers/iio/temperature/mlx90635.c
Signature · 1 line
2.40.1
Crt Mori
maintainerPATCH v2 1/2 iio: temperature: mlx90635 MLX90635 IR Temperature sensor
MLX90635 is an Infra Red contactless temperature sensor most suitable for consumer applications where measured object temperature is in range between -20 to 100 degrees Celsius. It has improved accuracy for measurements within temperature range of human body and can operate in ambient temperature range between -20 to 85 degrees Celsius.
Driver provides simple power management possibility as it returns to lowest possible power mode (Step sleep mode) in which temperature measurements can still be performed, yet for continuous measuring it switches to Continuous power mode where measurements constantly change without triggering.
--- MAINTAINERS | 7 + drivers/iio/temperature/Kconfig | 12 + drivers/iio/temperature/Makefile | 1 + drivers/iio/temperature/mlx90635.c | 1071 ++++++++++++++++++++++++++++ 4 files changed, 1091 insertions(+) create mode 100644 drivers/iio/temperature/mlx90635.c
MAINTAINERS +7 -0
@@ -13725,6 +13725,13 @@ S: Supported W: http://www.melexis.com F: drivers/iio/temperature/mlx90632.c +MELEXIS MLX90635 DRIVER +M: Crt Mori <[email protected]> +L: [email protected] +S: Supported +W: http://www.melexis.com +F: drivers/iio/temperature/mlx90635.c + MELFAS MIP4 TOUCHSCREEN DRIVER M: Sangwon Jee <[email protected]> S: Supported
drivers/iio/temperature/Kconfig +12 -0
@@ -76,6 +76,18 @@ config MLX90632 This driver can also be built as a module. If so, the module will be called mlx90632. +config MLX90635 + tristate "MLX90635 contact-less infrared sensor with medical accuracy" + depends on I2C + select REGMAP_I2C + help + If you say yes here you get support for the Melexis + MLX90635 contact-less infrared sensor with medical accuracy + connected with I2C. + + This driver can also be built as a module. If so, the module will + be called mlx90635. + config TMP006 tristate "TMP006 infrared thermopile sensor" depends on I2C
drivers/iio/temperature/Makefile +1 -0
@@ -13,6 +13,7 @@ obj-$(CONFIG_MAX31865) += max31865.o obj-$(CONFIG_MCP9600) += mcp9600.o obj-$(CONFIG_MLX90614) += mlx90614.o obj-$(CONFIG_MLX90632) += mlx90632.o +obj-$(CONFIG_MLX90632) += mlx90635.o obj-$(CONFIG_TMP006) += tmp006.o obj-$(CONFIG_TMP007) += tmp007.o obj-$(CONFIG_TMP117) += tmp117.o
drivers/iio/temperature/mlx90635.c new file +1071 -0
1,071 lines not on this page · raw message
Signature · 1 line
2.40.1
Jonathan Cameron
maintainerRe: [PATCH v2 1/2] iio: temperature: mlx90635 MLX90635 IR Temperature sensor
Quoted 6 lines · On Tue, 28 Nov 2023 12:02:22 +0100 Crt Mori <[email protected]> wrote:
MLX90635 is an Infra Red contactless temperature sensor most suitable for consumer applications where measured object temperature is in range between -20 to 100 degrees Celsius. It has improved accuracy for measurements within temperature range of human body and can operate in ambient temperature range between -20 to 85 degrees Celsius.
Quoted 7 lines
Driver provides simple power management possibility as it returns to lowest possible power mode (Step sleep mode) in which temperature measurements can still be performed, yet for continuous measuring it switches to Continuous power mode where measurements constantly change without triggering.
Signed-off-by Crt Mori<[email protected]>
Hi Crt,
I don't understand some of the regcache_cache_only() manipulation in here. If I understand the aim correctly it is to allow us to write settings whilst powered down (in sleep_step) that will then by synced to the device when it enters continuous mode?
If so, I'd expect to only see manipulation of whether the caching is or or not at places where we transition state. You currently have them in various other place. In some cases I scan see it's to allow a temporary change of state, but it's not obvious. So perhaps a comment ever time you manually tweak whether writes hit the device or just stick in the regacache. That comment can explain why each of them is needed.
A few other comments inline,
Thanks,
Jonathan
Quoted 45 lines
drivers/iio/temperature/Makefile +1
-0@@ -13,6 +13,7 @@ obj-$(CONFIG_MAX31865) += max31865.o obj-$(CONFIG_MCP9600) += mcp9600.o obj-$(CONFIG_MLX90614) += mlx90614.o obj-$(CONFIG_MLX90632) += mlx90632.o +obj-$(CONFIG_MLX90632) += mlx90635.o obj-$(CONFIG_TMP006) += tmp006.o obj-$(CONFIG_TMP007) += tmp007.o obj-$(CONFIG_TMP117) += tmp117.odrivers/iio/temperature/mlx90635.c new file +27
-0@@ -0,0 +1,1071 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* mlx90635.c - Melexis MLX90635 contactless IR temperature sensor +* +* Copyright (c) 2023 Melexis <[email protected]> +* +* Driver for the Melexis MLX90635 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> +#include <linux/module.h> +#include <linux/math64.h> +#include <linux/pm_runtime.h> +#include <linux/regmap.h> +#include <linux/regulator/consumer.h> + +#include <linux/iio/iio.h> +
...
drivers/iio/temperature/mlx90635.c line 80
Quoted 3 lines
drivers/iio/temperature/mlx90635.c new file +3
-0+/* Control register2 address - volatile */ +#define MLX90635_REG_CTRL2 0x0016/* Control Register2 address */ +#define MLX90635_CTRL2_BURST_CNT_SHIFT 6/* Burst count */
It very rarely makes sense to have a _SHIFT macro now we have GENMASK() that will encode it anyway.
#define MLX90635_CTRL2_BURST_CND_MASK GENMASK(10, 6)
is something we can easily compare with the datasheet. Same applies to all the other _SHIFT defines in here.
drivers/iio/temperature/mlx90635.c line 90
Quoted 10 lines
drivers/iio/temperature/mlx90635.c new file +10
-0+#define MLX90635_CTRL2_BURST_CNT_MASK GENMASK(MLX90635_CTRL2_BURST_CNT_SHIFT + 4, MLX90635_CTRL2_BURST_CNT_SHIFT) +#define MLX90635_CTRL2_MODE_SHIFT 11/* Power mode */ +#define MLX90635_CTRL2_MODE_MASK GENMASK(MLX90635_CTRL2_MODE_SHIFT + 1, MLX90635_CTRL2_MODE_SHIFT) +#define MLX90635_CTRL2_SOB_SHIFT 15/* Start burst measurement in step mode */ +#define MLX90635_CTRL2_SOB_MASK BIT(MLX90635_CTRL2_SOB_SHIFT) + +/* PowerModes statuses */ +#define MLX90635_PWR_STATUS_HALT 0/* Pwrmode hold */ +#define MLX90635_PWR_STATUS_SLEEP_STEP 1/* Pwrmode sleep step*/ +#define MLX90635_PWR_STATUS_STEP 2/* Pwrmode step */
The comments here add little given the define names are covering that information already I think. Keep them for non obvious cases.
Quoted 24 lines
drivers/iio/temperature/mlx90635.c new file +24
-0+#define MLX90635_PWR_STATUS_CONTINUOUS 3/* Pwrmode continuous*/ + +/* Measurement data addresses */ +#define MLX90635_RESULT_1 0x0002 +#define MLX90635_RESULT_2 0x0004 +#define MLX90635_RESULT_3 0x0006 +#define MLX90635_RESULT_4 0x0008 +#define MLX90635_RESULT_5 0x000A + +/* Timings (ms) */ +#define MLX90635_TIMING_RST_MIN 200/* Minimum time after addressed reset command */ +#define MLX90635_TIMING_RST_MAX 250/* Maximum time after addressed reset command */ +#define MLX90635_TIMING_POLLING 10000/* Time between bit polling*/ +#define MLX90635_TIMING_EE_ACTIVE_MIN 100/* Minimum time after activating the EEPROM for read */ +#define MLX90635_TIMING_EE_ACTIVE_MAX 150/* Maximum time after activating the EEPROM for read */ + +/* Magic constants */ +#define MLX90635_ID_DSPv1 0x01/* EEPROM DSP version */ +#define MLX90635_RESET_CMD 0x0006/* Reset sensor (address or global) */ +#define MLX90635_MAX_MEAS_NUM 31/* Maximum number of measurements in list */ +#define MLX90635_PTAT_DIV 12/* Used to divide the PTAT value in pre-processing */ +#define MLX90635_IR_DIV 24/* Used to divide the IR value in pre-processing */ +#define MLX90635_SLEEP_DELAY_MS 6000/* Autosleep delay */ +#define MLX90635_MEAS_MAX_TIME 2000/* Max measurement time in ms for the lowest refresh rate */
drivers/iio/temperature/mlx90635.c line 124
Quoted 10 lines
drivers/iio/temperature/mlx90635.c new file +10
-0+#define MLX90635_READ_RETRIES 100/* Number of read retries before quitting with timeout error */ +#define MLX90635_VERSION_MASK (GENMASK(15, 12) | GENMASK(7, 4)) +#define MLX90635_DSP_VERSION (reg) (((reg & GENMASK(14,12)) >> 9) | ((reg & GENMASK(6, 4)) >> 4)) +#define MLX90635_DSP_FIXED BIT(15) + + +/** +* struct mlx90635_data - private data for the MLX90635 device +* @client: I2C client of the device +* @lock: Internal mutex for multiple reads for single measurement
Multiple reads shouldn't be a problem, unless someone else can do something destructive in between. Perhaps a little more detail on why multiple reads matter?
Quoted 16 lines
+ * @regmap: Regmap of the device
+ * @emissivity: Object emissivity from 0 to 1000 where 1000 = 1.
+ * @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
+ */drivers/iio/temperature/mlx90635.c new file +9
-0+struct mlx90635_data { +struct i2c_client *client; +struct mutex lock; +struct regmap *regmap; +u16 emissivity; +struct regulator *regulator; +int powerstatus; +unsigned long interaction_ts; +};
...
Quoted 40 lines
drivers/iio/temperature/mlx90635.c new file +38
-0+ +static int mlx90635_pwr_sleep_step (struct mlx90635_data *data) +{ +int ret; + +if (data->powerstatus ==MLX90635_PWR_STATUS_SLEEP_STEP ) +return 0 ; + + ret =regmap_write_bits (data->regmap,MLX90635_REG_CTRL2 ,MLX90635_CTRL2_MODE_MASK , +FIELD_PREP (MLX90635_CTRL2_MODE_MASK ,MLX90635_PWR_STATUS_SLEEP_STEP )); +if (ret <0 ) +return ret; + +regcache_cache_only (data->regmap, true); + + data->powerstatus =MLX90635_PWR_STATUS_SLEEP_STEP ; +return 0 ; +} + +static int mlx90635_pwr_continuous (struct mlx90635_data *data) +{ +int ret; + +if (data->powerstatus ==MLX90635_PWR_STATUS_CONTINUOUS ) +return 0 ; + +regcache_cache_only (data->regmap, false); + + ret =regmap_write_bits (data->regmap,MLX90635_REG_CTRL2 ,MLX90635_CTRL2_MODE_MASK , +FIELD_PREP (MLX90635_CTRL2_MODE_MASK ,MLX90635_PWR_STATUS_CONTINUOUS )); +if (ret <0 ) +return ret; + + data->powerstatus =MLX90635_PWR_STATUS_CONTINUOUS ; +return 0 ; +} + +
drivers/iio/temperature/mlx90635.c
Quoted 10 lines
drivers/iio/temperature/mlx90635.c new file +10
-0+static int mlx90635_read_all_channel (struct mlx90635_data *data, +s16 *ambient_new_raw,s16 *ambient_old_raw, +s16 *object_raw) +{ +int ret; + +mutex_lock (&data->lock); +if (data->powerstatus ==MLX90635_PWR_STATUS_SLEEP_STEP ) { +regcache_cache_only (data->regmap, false); + ret =mlx90635_perform_measurement_burst (data);
Why is a burst needed here? Perhaps a comment?
Quoted 17 lines
drivers/iio/temperature/mlx90635.c new file +17
-0+if (ret <0 ) + goto read_unlock; + } + + ret =mlx90635_read_ambient_raw (data->regmap, ambient_new_raw, + ambient_old_raw); +if (ret <0 ) + goto read_unlock; + + ret =mlx90635_read_object_raw (data->regmap, object_raw); +read_unlock: +if (data->powerstatus ==MLX90635_PWR_STATUS_SLEEP_STEP ) +regcache_cache_only (data->regmap, true); + +mutex_unlock (&data->lock); +return ret; +}
drivers/iio/temperature/mlx90635.c
Quoted 10 lines
drivers/iio/temperature/mlx90635.c new file +9
-0+ +static int mlx90635_get_refresh_rate (struct mlx90635_data *data, +unsigned int *refresh_rate) +{ +unsigned int reg; +int ret; + +if (data->powerstatus ==MLX90635_PWR_STATUS_SLEEP_STEP ) +regcache_cache_only (data->regmap, false);
Definitely needs a comment on why this is needed in this case.
Quoted 8 lines
drivers/iio/temperature/mlx90635.c new file +8
-0+ + ret =regmap_read (data->regmap,MLX90635_REG_CTRL1 , ®); +if (ret <0 ) +return ret; + +if (data->powerstatus ==MLX90635_PWR_STATUS_SLEEP_STEP ) +regcache_cache_only (data->regmap, true); +
drivers/iio/temperature/mlx90635.c line 644
Quoted 10 lines
drivers/iio/temperature/mlx90635.c new file +10
-0+ *refresh_rate =FIELD_GET (MLX90635_CTRL1_REFRESH_RATE_MASK , reg); + +return 0 ; +} + +static const struct { +int val; +int val2; +} mlx90635_freqs[] = { + {0 ,200000 },
Prefer spaces after { and before }
Quoted 27 lines
drivers/iio/temperature/mlx90635.c new file +26
-0+ {0, 500000}, + {0, 900000}, + {1, 700000}, + {3, 0}, + {4, 800000}, + {6, 900000}, + {8, 900000} +}; + +static int mlx90635_write_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *channel,int val, +int val2,long mask) +{ +struct mlx90635_data *data =iio_priv (indio_dev); +int ret; +int i; + +switch (mask) { +case IIO_CHAN_INFO_CALIBEMISSIVITY : +/* Confirm we are within 0 and 1.0 */ +if (val <0 || val2 <0 || val >1 || + (val ==1 && val2 !=0 )) +return -EINVAL ; + data->emissivity = val *1000 + val2 /1000 ; +return 0 ; +case IIO_CHAN_INFO_SAMP_FREQ :
drivers/iio/temperature/mlx90635.c line 772
Quoted 10 lines
drivers/iio/temperature/mlx90635.c new file +10
-0+for (i =0 ; i <ARRAY_SIZE (mlx90635_freqs); i++) { +if (val == mlx90635_freqs[i].val && + val2 == mlx90635_freqs[i].val2) +break ; + } +if (i ==ARRAY_SIZE (mlx90635_freqs)) +return -EINVAL ; + +if (data->powerstatus ==MLX90635_PWR_STATUS_SLEEP_STEP ) +regcache_cache_only (data->regmap, false);
So here you want the rate to get through even though we otherwise have the device powered down? Is that because some registers are safe for writes and not others? If so you may need some locking to stop a race where you turn on writes here and someone else writes.
Quoted 11 lines
drivers/iio/temperature/mlx90635.c new file +11
-0+ + ret =regmap_write_bits (data->regmap,MLX90635_REG_CTRL1 , +MLX90635_CTRL1_REFRESH_RATE_MASK , i); + +if (data->powerstatus ==MLX90635_PWR_STATUS_SLEEP_STEP ) +regcache_cache_only (data->regmap, true); +return ret; +default : +return -EINVAL ; + } +}
Crt Mori
maintainerRe: [PATCH v2 1/2] iio: temperature: mlx90635 MLX90635 IR Temperature sensor
Quoted 1 line · Jonathan Cameron
...
Quoted 19 lines
Quoted 4 lines
switches to Continuous power mode where measurements constantly change without triggering.
Signed-off-by Crt Mori<[email protected]>Hi Crt,
I don't understand some of the
regcache_cache_only()manipulation in here. If I understand the aim correctly it is to allow us to write settings whilst powered down (insleep_step) that will then by synced to the device when it enters continuous mode?If so, I'd expect to only see manipulation of whether the caching is or or not at places where we transition state. You currently have them in various other place. In some cases I scan see it's to allow a temporary change of state, but it's not obvious. So perhaps a comment ever time you manually tweak whether writes hit the device or just stick in the regacache. That comment can explain why each of them is needed.
Quoted 6 lines
A few other comments inline,
Thanks,
Jonathan
While in Sleep Step mode, the EEPROM is powered down, but the cache buffers those values. Still when you try to write or read a volatile register (which should not be prevented by cache enabled as per my opinion, but code says differently) in that mode, it returns -EBUSY (as we discovered by code), so this kind of manipulation is needed to enable write and read operations from volatile registers. And you need to trigger the measurement (burst mode) in that state, but since you cannot read EEPROM, yet still need its values to calculate the final temperature, the cache is used for this case. There is nothing to re-cache when we get back as all registers I read/write to are marked as volatile, so they would not be cached anyway.
Thanks for the review - I still have some questions below (and explanation, but not sure where to put those).
Quoted 1 line
Quoted 1 line
drivers/iio/temperature/Makefile +0
-0
...
Quoted 5 lines
Quoted 1 line
+ * @lock: Internal mutex for multiple reads for single measurement
Multiple reads shouldn't be a problem, unless someone else can do something destructive in between. Perhaps a little more detail on why multiple reads matter?
You trigger device to perform measurement in Sleep Step mode, so to ensure both object and ambient temperature reads are from the same triggered measurement, the mutex needs to be held. If for example in between you would retrigger the measurement, then you would operate on "invalid" data (shouldn't differ much, but I wanted to prevent that as it might be 0).
Quoted 7 lines
Quoted 7 lines
+ * @regmap: Regmap of the device
+ * @emissivity: Object emissivity from 0 to 1000 where 1000 = 1.
+ * @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
+ */
...
Quoted 7 lines
drivers/iio/temperature/Makefile
Quoted 4 lines
drivers/iio/temperature/Makefile +4
-0+ mutex_lock(&data->lock); + if (data->powerstatus == MLX90635_PWR_STATUS_SLEEP_STEP) { + regcache_cache_only(data->regmap, false); + ret = mlx90635_perform_measurement_burst(data);Why is a burst needed here? Perhaps a comment?
Burst is from 90632 terminology (and our chip register map), but maybe more general would be "trigger_measurement"?
Quoted 11 lines
drivers/iio/temperature/Makefile
Quoted 8 lines
drivers/iio/temperature/Makefile +8
-0+static int mlx90635_get_refresh_rate(struct mlx90635_data *data, + unsigned int *refresh_rate) +{ + unsigned int reg; + int ret; + + if (data->powerstatus == MLX90635_PWR_STATUS_SLEEP_STEP) + regcache_cache_only(data->regmap, false);Definitely needs a comment on why this is needed in this case.
Here and below (where we turn it back to true?), but then I assume in all other instances as well? Maybe a more general comment in the sleep_step mode function?
Quoted 11 lines
Quoted 11 lines
drivers/iio/temperature/Makefile +11
-0+ + ret = regmap_read(data->regmap, MLX90635_REG_CTRL1, ®); + if (ret < 0) + return ret; + + if (data->powerstatus == MLX90635_PWR_STATUS_SLEEP_STEP) + regcache_cache_only(data->regmap, true); + + *refresh_rate = FIELD_GET(MLX90635_CTRL1_REFRESH_RATE_MASK, reg); + + return 0;
Quoted 8 lines
drivers/iio/temperature/Makefile
Quoted 7 lines
drivers/iio/temperature/Makefile +7
-0+} + +static const struct { + int val; + int val2; +} mlx90635_freqs[] = { + {0, 200000},Prefer spaces after { and before }
ok.
Quoted 8 lines
Quoted 8 lines
drivers/iio/temperature/Makefile +8
-0+ {0, 500000}, + {0, 900000}, + {1, 700000}, + {3, 0}, + {4, 800000}, + {6, 900000}, + {8, 900000} +};
...
Quoted 11 lines
drivers/iio/temperature/Makefile
Quoted 5 lines
drivers/iio/temperature/Makefile +5
-0+ if (i == ARRAY_SIZE(mlx90635_freqs)) + return -EINVAL; + + if (data->powerstatus == MLX90635_PWR_STATUS_SLEEP_STEP) + regcache_cache_only(data->regmap, false);So here you want the rate to get through even though we otherwise have the device powered down? Is that because some registers are safe for writes and not others? If so you may need some locking to stop a race where you turn on writes here and someone else writes.
Yes, exactly the case. Read/Write into registers (REG_) is possible in all modes, but read of EEPROM is not (to save power the EEPROM is turned off). I do not see how write race would get us into trouble here since it is only 1, and as long as chip powerstatus is not changed we should end up in correct state. I can wrap a mutex around though.
Quoted 12 lines
Quoted 11 lines
drivers/iio/temperature/Makefile +11
-0+ + ret = regmap_write_bits(data->regmap, MLX90635_REG_CTRL1, + MLX90635_CTRL1_REFRESH_RATE_MASK, i); + + if (data->powerstatus == MLX90635_PWR_STATUS_SLEEP_STEP) + regcache_cache_only(data->regmap, true); + return ret; + default: + return -EINVAL; + } +}
Jonathan Cameron
maintainerRe: [PATCH v2 1/2] iio: temperature: mlx90635 MLX90635 IR Temperature sensor
Quoted 29 lines · On Mon, 4 Dec 2023 16:34:30 +0100 Crt Mori <[email protected]> wrote:
Quoted 1 line · Jonathan Cameron
...
Quoted 19 lines
Quoted 4 lines
switches to Continuous power mode where measurements constantly change without triggering.
Signed-off-by Crt Mori<[email protected]>Hi Crt,
I don't understand some of the
regcache_cache_only()manipulation in here. If I understand the aim correctly it is to allow us to write settings whilst powered down (insleep_step) that will then by synced to the device when it enters continuous mode?If so, I'd expect to only see manipulation of whether the caching is or or not at places where we transition state. You currently have them in various other place. In some cases I scan see it's to allow a temporary change of state, but it's not obvious. So perhaps a comment ever time you manually tweak whether writes hit the device or just stick in the regacache. That comment can explain why each of them is needed.
Quoted 6 lines
A few other comments inline,
Thanks,
Jonathan
Quoted 6 lines
While in Sleep Step mode, the EEPROM is powered down, but the cache buffers those values. Still when you try to write or read a volatile register (which should not be prevented by cache enabled as per my opinion, but code says differently) in that mode, it returns
-EBUSY(as we discovered by code), so this kind of manipulation is needed to enable write and read operations from volatile registers.
So the cache trick is just meant for the eeprom? Can you use two regmaps. (I've seen similar done for devices with different ways of reading which this 'kind of' corresponds to).
One to cover the eeprom and the other the registers that always work. That should let you separately control if they are in caching state or not.
Or just read the eeprom into a manually created cache on boot?
Quoted 18 lines
And you need
to trigger the measurement (burst mode) in that state, but since you cannot read EEPROM, yet still need its values to calculate the final temperature, the cache is used for this case. There is nothing to re-cache when we get back as all registers I read/write to are marked as volatile, so they would not be cached anyway..Thanks for the review - I still have some questions below (and explanation, but not sure where to put those).
Quoted 1 line
Quoted 1 line
drivers/iio/temperature/Makefile +0
-0...
Quoted 5 lines
Quoted 1 line
+ * @lock: Internal mutex for multiple reads for single measurement
Multiple reads shouldn't be a problem, unless someone else can do something destructive in between. Perhaps a little more detail on why multiple reads matter?
Quoted 6 lines
You trigger device to perform measurement in Sleep Step mode, so to ensure both object and ambient temperature reads are from the same triggered measurement, the mutex needs to be held. If for example in between you would retrigger the measurement, then you would operate on "invalid" data (shouldn't differ much, but I wanted to prevent that as it might be 0).
ok. Just give a little bit more of that detail. I'd not understood intent is to ensure one trigger -> one measurement.
Quoted 14 lines
Quoted 8 lines
Quoted 8 lines
+ * @regmap: Regmap of the device
+ * @emissivity: Object emissivity from 0 to 1000 where 1000 = 1.
+ * @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
+ */...
Quoted 5 lines
Quoted 4 lines
drivers/iio/temperature/Makefile +4
-0+ mutex_lock(&data->lock); + if (data->powerstatus == MLX90635_PWR_STATUS_SLEEP_STEP) { + regcache_cache_only(data->regmap, false); + ret = mlx90635_perform_measurement_burst(data);
Quoted 5 lines
Quoted 2 lines
Why is a burst needed here? Perhaps a comment?
Burst is from 90632 terminology (and our chip register map), but maybe more general would be "
trigger_measurement"?
ok. But why only if in SLEEP_STEP?
Quoted 13 lines
Quoted 12 lines
drivers/iio/temperature/Makefile
Quoted 9 lines
drivers/iio/temperature/Makefile +8
-0+static int mlx90635_get_refresh_rate(struct mlx90635_data *data, + unsigned int *refresh_rate) +{ + unsigned int reg; + int ret; + + if (data->powerstatus == MLX90635_PWR_STATUS_SLEEP_STEP) + regcache_cache_only(data->regmap, false);Definitely needs a comment on why this is needed in this case.
Quoted 3 lines
Here and below (where we turn it back to true?), but then I assume in all other instances as well? Maybe a more general comment in the
sleep_stepmode function?
If we keep this, then yes I think we need comments on these - even if it's as simple as 'not accessing an eeprom register so we want to talk to the device'.
Quoted 44 lines
Quoted 12 lines
Quoted 12 lines
drivers/iio/temperature/Makefile +11
-0+ + ret = regmap_read(data->regmap, MLX90635_REG_CTRL1, ®); + if (ret < 0) + return ret; + + if (data->powerstatus == MLX90635_PWR_STATUS_SLEEP_STEP) + regcache_cache_only(data->regmap, true); + + *refresh_rate = FIELD_GET(MLX90635_CTRL1_REFRESH_RATE_MASK, reg); + + return 0;Quoted 8 lines
drivers/iio/temperature/Makefile
Quoted 7 lines
drivers/iio/temperature/Makefile +7
-0+} + +static const struct { + int val; + int val2; +} mlx90635_freqs[] = { + {0, 200000},Prefer spaces after { and before }
ok.
Quoted 8 lines
Quoted 8 lines
drivers/iio/temperature/Makefile +8
-0+ {0, 500000}, + {0, 900000}, + {1, 700000}, + {3, 0}, + {4, 800000}, + {6, 900000}, + {8, 900000} +};...
Quoted 11 lines
drivers/iio/temperature/Makefile
Quoted 5 lines
drivers/iio/temperature/Makefile +5
-0+ if (i == ARRAY_SIZE(mlx90635_freqs)) + return -EINVAL; + + if (data->powerstatus == MLX90635_PWR_STATUS_SLEEP_STEP) + regcache_cache_only(data->regmap, false);So here you want the rate to get through even though we otherwise have the device powered down? Is that because some registers are safe for writes and not others? If so you may need some locking to stop a race where you turn on writes here and someone else writes.
Quoted 6 lines
Yes, exactly the case. Read/Write into registers (
REG_) is possible in all modes, but read of EEPROM is not (to save power the EEPROM is turned off). I do not see how write race would get us into trouble here since it is only 1, and as long as chip powerstatus is not changed we should end up in correct state. I can wrap a mutex around though.
Assuming regcache_cache_only() isn't refcounted, you could end up with a second copy of this racing through and accessing the data after the first one turned the cache back on so the -EBUSY your mentioned.
Quoted 15 lines
Quoted 15 lines
Quoted 14 lines
drivers/iio/temperature/Makefile +11
-0+ + ret = regmap_write_bits(data->regmap, MLX90635_REG_CTRL1, + MLX90635_CTRL1_REFRESH_RATE_MASK, i); + + if (data->powerstatus == MLX90635_PWR_STATUS_SLEEP_STEP) + regcache_cache_only(data->regmap, true); + return ret; + default: + return -EINVAL; + } +}
Crt Mori
maintainerRe: [PATCH v2 1/2] iio: temperature: mlx90635 MLX90635 IR Temperature sensor
Quoted 6 lines · Jonathan Cameron
Quoted 2 lines · On Mon, 4 Dec 2023 16:34:30 +0100 Crt Mori <[email protected]> wrote:
Quoted 1 line · Jonathan Cameron
...
Quoted 7 lines
Quoted 6 lines
While in Sleep Step mode, the EEPROM is powered down, but the cache buffers those values. Still when you try to write or read a volatile register (which should not be prevented by cache enabled as per my opinion, but code says differently) in that mode, it returns
-EBUSY(as we discovered by code), so this kind of manipulation is needed to enable write and read operations from volatile registers.
Quoted 8 lines
So the cache trick is just meant for the eeprom? Can you use two regmaps. (I've seen similar done for devices with different ways of reading which this 'kind of' corresponds to).
One to cover the eeprom and the other the registers that always work. That should let you separately control if they are in caching state or not.
Or just read the eeprom into a manually created cache on boot?
It did not seem correct to create a manual cache, since regcache does this job. I tried two separated regmaps, but when I tried to initialize them I got into kernel panic/crash, so I could not get it working on same device. Do you have any device in mind I could template this against?
...
Quoted 5 lines
Quoted 2 lines
"invalid" data (shouldn't differ much, but I wanted to prevent that as it might be 0).
ok. Just give a little bit more of that detail. I'd not understood intent is to ensure one trigger -> one measurement.
OK.
Quoted 1 line
Quoted 1 line
...
Quoted 6 lines
Quoted 3 lines
Burst is from 90632 terminology (and our chip register map), but maybe more general would be "
trigger_measurement"?ok. But why only if in
SLEEP_STEP?
Because in continuous mode (other mode used here) the measurement table is constantly updated, so trigger is not useful and would only slow down the reading. And I did not want to block the data retrieval when person wants to read the data fast.
Quoted 13 lines
Quoted 13 lines
Quoted 12 lines
drivers/iio/temperature/mlx90635.c line 626
Quoted 9 lines
drivers/iio/temperature/mlx90635.c new file +8
-0+static int mlx90635_get_refresh_rate (struct mlx90635_data *data, +unsigned int *refresh_rate) +{ +unsigned int reg; +int ret; + +if (data->powerstatus ==MLX90635_PWR_STATUS_SLEEP_STEP ) +regcache_cache_only (data->regmap, false);Definitely needs a comment on why this is needed in this case.
Quoted 7 lines
Quoted 3 lines
Here and below (where we turn it back to true?), but then I assume in all other instances as well? Maybe a more general comment in the
sleep_stepmode function?If we keep this, then yes I think we need comments on these - even if it's as simple as 'not accessing an eeprom register so we want to talk to the device'.
OK, then this is an option if I cannot make two regmaps work.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
+
...
Quoted 7 lines
Quoted 2 lines
changed we should end up in correct state. I can wrap a mutex around though.
Assuming
regcache_cache_only()isn't refcounted, you could end up with a second copy of this racing through and accessing the data after the first one turned the cache back on so the-EBUSYyour mentioned.
True. I will use mutex then for this action.
Jonathan Cameron
maintainerRe: [PATCH v2 1/2] iio: temperature: mlx90635 MLX90635 IR Temperature sensor
Quoted 24 lines · On Mon, 4 Dec 2023 20:56:39 +0100 Crt Mori <[email protected]> wrote:
Quoted 6 lines · Jonathan Cameron
Quoted 2 lines · On Mon, 4 Dec 2023 16:34:30 +0100 Crt Mori <[email protected]> wrote:
Quoted 1 line · Jonathan Cameron
...
Quoted 7 lines
Quoted 6 lines
While in Sleep Step mode, the EEPROM is powered down, but the cache buffers those values. Still when you try to write or read a volatile register (which should not be prevented by cache enabled as per my opinion, but code says differently) in that mode, it returns
-EBUSY(as we discovered by code), so this kind of manipulation is needed to enable write and read operations from volatile registers.Quoted 8 lines
So the cache trick is just meant for the eeprom? Can you use two regmaps. (I've seen similar done for devices with different ways of reading which this 'kind of' corresponds to).
One to cover the eeprom and the other the registers that always work. That should let you separately control if they are in caching state or not.
Or just read the eeprom into a manually created cache on boot?
Quoted 5 lines
It did not seem correct to create a manual cache, since regcache does this job. I tried two separated regmaps, but when I tried to initialize them I got into kernel panic/crash, so I could not get it working on same device. Do you have any device in mind I could template this against?
I'm not sure which device I was thinking of, but grepping and looking for likely targets got me
https://elixir.bootlin.com/linux/latest/source/drivers/mfd/madera-spi.c#L90
which registers one regmap for 32bit registers and one for 16 bit registers as the devices have two non overlapping ranges.
Not sure why it would crash (as opposed to one trampling on the other) but maybe there is something tied more tightly to the device than I think.
Quoted 15 lines
...
Quoted 5 lines
Quoted 2 lines
"invalid" data (shouldn't differ much, but I wanted to prevent that as it might be 0).
ok. Just give a little bit more of that detail. I'd not understood intent is to ensure one trigger -> one measurement.
OK.
Quoted 1 line
Quoted 1 line
...
Quoted 4 lines
Quoted 3 lines
Burst is from 90632 terminology (and our chip register map), but maybe more general would be "
trigger_measurement"?
Quoted 7 lines
Quoted 2 lines
ok. But why only if in
SLEEP_STEP?Because in continuous mode (other mode used here) the measurement table is constantly updated, so trigger is not useful and would only slow down the reading. And I did not want to block the data retrieval when person wants to read the data fast.
Fair enough - add a comment so reader can easily follow that.
Quoted 36 lines
Quoted 14 lines
Quoted 14 lines
Quoted 13 lines
drivers/iio/temperature/mlx90635.c line 626
Quoted 10 lines
drivers/iio/temperature/mlx90635.c new file +8
-0+static int mlx90635_get_refresh_rate (struct mlx90635_data *data, +unsigned int *refresh_rate) +{ +unsigned int reg; +int ret; + +if (data->powerstatus ==MLX90635_PWR_STATUS_SLEEP_STEP ) +regcache_cache_only (data->regmap, false);Definitely needs a comment on why this is needed in this case.
Quoted 7 lines
Quoted 3 lines
Here and below (where we turn it back to true?), but then I assume in all other instances as well? Maybe a more general comment in the
sleep_stepmode function?If we keep this, then yes I think we need comments on these - even if it's as simple as 'not accessing an eeprom register so we want to talk to the device'.
OK, then this is an option if I cannot make two regmaps work.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
+
...
Quoted 7 lines
Quoted 2 lines
changed we should end up in correct state. I can wrap a mutex around though.
Assuming
regcache_cache_only()isn't refcounted, you could end up with a second copy of this racing through and accessing the data after the first one turned the cache back on so the-EBUSYyour mentioned.True. I will use mutex then for this action.
Crt Mori
maintainerPATCH v2 2/2 dt-bindings: iio: temperature: add MLX90635 device
In reply to Crt MoriAdd device tree bindings for MLX90635 Infra Red contactless temperature sensor.
--- .../iio/temperature/melexis,mlx90632.yaml | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)
Documentation/devicetree/bindings/iio/temperature/melexis,mlx90632.yaml +17 -2
@@ -4,7 +4,7 @@ $id: http://devicetree.org/schemas/iio/temperature/melexis,mlx90632.yaml# $schema: http://devicetree.org/meta-schemas/core.yaml# -title: Melexis MLX90632 contactless Infra Red temperature sensor +title: Melexis MLX90632 and MLX90635 contactless Infra Red temperature sensor maintainers: - Crt Mori <[email protected]>
@@ -27,9 +27,24 @@ description: | Since measured object emissivity effects Infra Red energy emitted, emissivity should be set before requesting the object temperature. + https://www.melexis.com/en/documents/documentation/datasheets/datasheet-mlx90635 + + MLX90635 is most suitable for consumer applications where + measured object temperature is in range between -20 to 100 degrees + Celsius with relative error of measurement 2 degree Celsius in + object temperature range for industrial applications, while just 0.2 + degree Celsius for human body measurement applications. Since it can + operate and measure ambient temperature in range of -20 to 85 degrees + Celsius it is suitable also for outdoor use. + + Since measured object emissivity effects Infra Red energy emitted, + emissivity should be set before requesting the object temperature. + properties: compatible: - const: melexis,mlx90632 + enum: + - melexis,mlx90632 + - melexis,mlx90635 reg: maxItems: 1
Signature · 1 line
2.40.1
Re: [PATCH v2 2/2] dt-bindings: iio: temperature: add MLX90635 device
Quoted 8 lines · Crt Mori
Add device tree bindings for MLX90635 Infra Red contactless temperature sensor.
Signed-off-by Crt Mori <[email protected]>--- .../iio/temperature/melexis,mlx90632.yaml | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)