* Re: [PATCH v2 2/2] iio: temperature: add support for Analog Devices MAX30210
2026-03-04 12:25 ` [PATCH v2 2/2] iio: temperature: add support for Analog Devices MAX30210 John Erasmus Mari Geronimo
@ 2026-03-05 0:34 ` kernel test robot
2026-03-05 0:45 ` kernel test robot
` (2 subsequent siblings)
3 siblings, 0 replies; 11+ messages in thread
From: kernel test robot @ 2026-03-05 0:34 UTC (permalink / raw)
To: John Erasmus Mari Geronimo, linux-iio, jic23
Cc: llvm, oe-kbuild-all, dlechner, nuno.sa, andy, linux-kernel
Hi John,
kernel test robot noticed the following build errors:
[auto build test ERROR on 70a9ae59c5b1f2f5501e78e2d85bfeefd153f854]
url: https://github.com/intel-lab-lkp/linux/commits/John-Erasmus-Mari-Geronimo/dt-bindings-iio-temperature-add-ADI-MAX30210/20260304-202920
base: 70a9ae59c5b1f2f5501e78e2d85bfeefd153f854
patch link: https://lore.kernel.org/r/20260304122509.67931-3-johnerasmusmari.geronimo%40analog.com
patch subject: [PATCH v2 2/2] iio: temperature: add support for Analog Devices MAX30210
config: sparc64-allmodconfig (https://download.01.org/0day-ci/archive/20260305/202603050819.rN8MJLQm-lkp@intel.com/config)
compiler: clang version 23.0.0git (https://github.com/llvm/llvm-project 9a109fbb6e184ec9bcce10615949f598f4c974a9)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260305/202603050819.rN8MJLQm-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202603050819.rN8MJLQm-lkp@intel.com/
All errors (new ones prefixed by >>):
>> drivers/iio/temperature/max30210.c:136:13: error: call to undeclared function 'get_unaligned_be24'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
136 | u32 raw = get_unaligned_be24(&st->fifo_buf[3 * i]);
| ^
>> drivers/iio/temperature/max30210.c:272:11: error: call to undeclared function 'FIELD_GET'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
272 | return FIELD_GET(MAX30210_STATUS_TEMP_HI_MASK, val);
| ^
drivers/iio/temperature/max30210.c:279:11: error: call to undeclared function 'FIELD_GET'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
279 | return FIELD_GET(MAX30210_STATUS_TEMP_LO_MASK, val);
| ^
drivers/iio/temperature/max30210.c:336:10: error: call to undeclared function 'FIELD_GET'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
336 | uval = FIELD_GET(MAX30210_TEMPCONF2_TEMP_PERIOD_MASK, uval);
| ^
>> drivers/iio/temperature/max30210.c:418:7: error: call to undeclared function 'FIELD_PREP'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
418 | FIELD_PREP(MAX30210_TEMPCONF2_TEMP_PERIOD_MASK, i));
| ^
5 errors generated.
vim +/get_unaligned_be24 +136 drivers/iio/temperature/max30210.c
122
123 static void max30210_fifo_read(struct iio_dev *indio_dev)
124 {
125 struct max30210_state *st = iio_priv(indio_dev);
126 int ret;
127
128 ret = regmap_bulk_read(st->regmap, MAX30210_FIFO_DATA_REG,
129 st->fifo_buf, 3 * st->watermark);
130 if (ret) {
131 dev_err(&indio_dev->dev, "Failed to read from fifo.\n");
132 return;
133 }
134
135 for (unsigned int i = 0; i < st->watermark; i++) {
> 136 u32 raw = get_unaligned_be24(&st->fifo_buf[3 * i]);
137
138 if (raw == MAX30210_FIFO_INVAL_DATA) {
139 dev_err_ratelimited(&indio_dev->dev, "Invalid data\n");
140 continue;
141 }
142
143 s16 temp = (s16)(raw & 0xFFFF);
144
145 iio_push_to_buffers(indio_dev, &temp);
146 }
147 }
148
149 static irqreturn_t max30210_irq_handler(int irq, void *dev_id)
150 {
151 struct iio_dev *indio_dev = dev_id;
152 struct max30210_state *st = iio_priv(indio_dev);
153 unsigned int status;
154 int ret;
155
156 ret = regmap_read(st->regmap, MAX30210_STATUS_REG, &status);
157 if (ret) {
158 dev_err(&indio_dev->dev, "Status read failed\n");
159 return IRQ_NONE;
160 }
161
162 if (status & MAX30210_STATUS_A_FULL_MASK)
163 max30210_fifo_read(indio_dev);
164
165 if (status & MAX30210_STATUS_TEMP_HI_MASK)
166 iio_push_event(indio_dev,
167 IIO_UNMOD_EVENT_CODE(IIO_TEMP, 0,
168 IIO_EV_TYPE_THRESH,
169 IIO_EV_DIR_RISING),
170 iio_get_time_ns(indio_dev));
171
172 if (status & MAX30210_STATUS_TEMP_LO_MASK)
173 iio_push_event(indio_dev,
174 IIO_UNMOD_EVENT_CODE(IIO_TEMP, 0,
175 IIO_EV_TYPE_THRESH,
176 IIO_EV_DIR_FALLING),
177 iio_get_time_ns(indio_dev));
178
179 return IRQ_HANDLED;
180 }
181
182 static int max30210_reg_access(struct iio_dev *indio_dev, unsigned int reg,
183 unsigned int writeval, unsigned int *readval)
184 {
185 struct max30210_state *st = iio_priv(indio_dev);
186
187 if (!readval)
188 return regmap_write(st->regmap, reg, writeval);
189
190 return regmap_read(st->regmap, reg, readval);
191 }
192
193 static int max30210_read_event(struct iio_dev *indio_dev,
194 const struct iio_chan_spec *chan,
195 enum iio_event_type type,
196 enum iio_event_direction dir,
197 enum iio_event_info info, int *val, int *val2)
198 {
199 struct max30210_state *st = iio_priv(indio_dev);
200
201 if (info != IIO_EV_INFO_VALUE)
202 return -EINVAL;
203
204 switch (dir) {
205 case IIO_EV_DIR_RISING:
206 switch (type) {
207 case IIO_EV_TYPE_THRESH:
208 return max30210_read_temp(st->regmap, MAX30210_TEMP_ALM_HI_REG, val);
209 default:
210 return -EINVAL;
211 }
212 case IIO_EV_DIR_FALLING:
213 switch (type) {
214 case IIO_EV_TYPE_THRESH:
215 return max30210_read_temp(st->regmap, MAX30210_TEMP_ALM_LO_REG, val);
216 default:
217 return -EINVAL;
218 }
219 default:
220 return -EINVAL;
221 }
222 }
223
224 static int max30210_write_event(struct iio_dev *indio_dev,
225 const struct iio_chan_spec *chan,
226 enum iio_event_type type,
227 enum iio_event_direction dir,
228 enum iio_event_info info, int val, int val2)
229 {
230 struct max30210_state *st = iio_priv(indio_dev);
231
232 if (info != IIO_EV_INFO_VALUE)
233 return -EINVAL;
234
235 switch (dir) {
236 case IIO_EV_DIR_RISING:
237 switch (type) {
238 case IIO_EV_TYPE_THRESH:
239 return max30210_write_temp(st->regmap, MAX30210_TEMP_ALM_HI_REG, val);
240 default:
241 return -EINVAL;
242 }
243 case IIO_EV_DIR_FALLING:
244 switch (type) {
245 case IIO_EV_TYPE_THRESH:
246 return max30210_write_temp(st->regmap, MAX30210_TEMP_ALM_LO_REG, val);
247 default:
248 return -EINVAL;
249 }
250 default:
251 return -EINVAL;
252 }
253 }
254
255 static int max30210_read_event_config(struct iio_dev *indio_dev,
256 const struct iio_chan_spec *chan,
257 enum iio_event_type type,
258 enum iio_event_direction dir)
259 {
260 struct max30210_state *st = iio_priv(indio_dev);
261 unsigned int val;
262 int ret;
263
264 ret = regmap_read(st->regmap, MAX30210_INT_EN_REG, &val);
265 if (ret)
266 return ret;
267
268 switch (dir) {
269 case IIO_EV_DIR_RISING:
270 switch (type) {
271 case IIO_EV_TYPE_THRESH:
> 272 return FIELD_GET(MAX30210_STATUS_TEMP_HI_MASK, val);
273 default:
274 return -EINVAL;
275 }
276 case IIO_EV_DIR_FALLING:
277 switch (type) {
278 case IIO_EV_TYPE_THRESH:
279 return FIELD_GET(MAX30210_STATUS_TEMP_LO_MASK, val);
280 default:
281 return -EINVAL;
282 }
283 default:
284 return -EINVAL;
285 }
286 }
287
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2 2/2] iio: temperature: add support for Analog Devices MAX30210
2026-03-04 12:25 ` [PATCH v2 2/2] iio: temperature: add support for Analog Devices MAX30210 John Erasmus Mari Geronimo
2026-03-05 0:34 ` kernel test robot
@ 2026-03-05 0:45 ` kernel test robot
2026-03-05 0:56 ` David Lechner
2026-03-07 12:38 ` Jonathan Cameron
3 siblings, 0 replies; 11+ messages in thread
From: kernel test robot @ 2026-03-05 0:45 UTC (permalink / raw)
To: John Erasmus Mari Geronimo, linux-iio, jic23
Cc: oe-kbuild-all, dlechner, nuno.sa, andy, linux-kernel
Hi John,
kernel test robot noticed the following build errors:
[auto build test ERROR on 70a9ae59c5b1f2f5501e78e2d85bfeefd153f854]
url: https://github.com/intel-lab-lkp/linux/commits/John-Erasmus-Mari-Geronimo/dt-bindings-iio-temperature-add-ADI-MAX30210/20260304-202920
base: 70a9ae59c5b1f2f5501e78e2d85bfeefd153f854
patch link: https://lore.kernel.org/r/20260304122509.67931-3-johnerasmusmari.geronimo%40analog.com
patch subject: [PATCH v2 2/2] iio: temperature: add support for Analog Devices MAX30210
config: i386-randconfig-r071-20260305 (https://download.01.org/0day-ci/archive/20260305/202603050822.weVzrjhU-lkp@intel.com/config)
compiler: gcc-14 (Debian 14.2.0-19) 14.2.0
smatch: v0.5.0-9004-gb810ac53
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260305/202603050822.weVzrjhU-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202603050822.weVzrjhU-lkp@intel.com/
All errors (new ones prefixed by >>):
drivers/iio/temperature/max30210.c: In function 'max30210_fifo_read':
>> drivers/iio/temperature/max30210.c:136:27: error: implicit declaration of function 'get_unaligned_be24' [-Wimplicit-function-declaration]
136 | u32 raw = get_unaligned_be24(&st->fifo_buf[3 * i]);
| ^~~~~~~~~~~~~~~~~~
drivers/iio/temperature/max30210.c: In function 'max30210_read_event_config':
>> drivers/iio/temperature/max30210.c:272:32: error: implicit declaration of function 'FIELD_GET' [-Wimplicit-function-declaration]
272 | return FIELD_GET(MAX30210_STATUS_TEMP_HI_MASK, val);
| ^~~~~~~~~
drivers/iio/temperature/max30210.c: In function 'max30210_write_raw':
>> drivers/iio/temperature/max30210.c:418:49: error: implicit declaration of function 'FIELD_PREP' [-Wimplicit-function-declaration]
418 | FIELD_PREP(MAX30210_TEMPCONF2_TEMP_PERIOD_MASK, i));
| ^~~~~~~~~~
vim +/get_unaligned_be24 +136 drivers/iio/temperature/max30210.c
122
123 static void max30210_fifo_read(struct iio_dev *indio_dev)
124 {
125 struct max30210_state *st = iio_priv(indio_dev);
126 int ret;
127
128 ret = regmap_bulk_read(st->regmap, MAX30210_FIFO_DATA_REG,
129 st->fifo_buf, 3 * st->watermark);
130 if (ret) {
131 dev_err(&indio_dev->dev, "Failed to read from fifo.\n");
132 return;
133 }
134
135 for (unsigned int i = 0; i < st->watermark; i++) {
> 136 u32 raw = get_unaligned_be24(&st->fifo_buf[3 * i]);
137
138 if (raw == MAX30210_FIFO_INVAL_DATA) {
139 dev_err_ratelimited(&indio_dev->dev, "Invalid data\n");
140 continue;
141 }
142
143 s16 temp = (s16)(raw & 0xFFFF);
144
145 iio_push_to_buffers(indio_dev, &temp);
146 }
147 }
148
149 static irqreturn_t max30210_irq_handler(int irq, void *dev_id)
150 {
151 struct iio_dev *indio_dev = dev_id;
152 struct max30210_state *st = iio_priv(indio_dev);
153 unsigned int status;
154 int ret;
155
156 ret = regmap_read(st->regmap, MAX30210_STATUS_REG, &status);
157 if (ret) {
158 dev_err(&indio_dev->dev, "Status read failed\n");
159 return IRQ_NONE;
160 }
161
162 if (status & MAX30210_STATUS_A_FULL_MASK)
163 max30210_fifo_read(indio_dev);
164
165 if (status & MAX30210_STATUS_TEMP_HI_MASK)
166 iio_push_event(indio_dev,
167 IIO_UNMOD_EVENT_CODE(IIO_TEMP, 0,
168 IIO_EV_TYPE_THRESH,
169 IIO_EV_DIR_RISING),
170 iio_get_time_ns(indio_dev));
171
172 if (status & MAX30210_STATUS_TEMP_LO_MASK)
173 iio_push_event(indio_dev,
174 IIO_UNMOD_EVENT_CODE(IIO_TEMP, 0,
175 IIO_EV_TYPE_THRESH,
176 IIO_EV_DIR_FALLING),
177 iio_get_time_ns(indio_dev));
178
179 return IRQ_HANDLED;
180 }
181
182 static int max30210_reg_access(struct iio_dev *indio_dev, unsigned int reg,
183 unsigned int writeval, unsigned int *readval)
184 {
185 struct max30210_state *st = iio_priv(indio_dev);
186
187 if (!readval)
188 return regmap_write(st->regmap, reg, writeval);
189
190 return regmap_read(st->regmap, reg, readval);
191 }
192
193 static int max30210_read_event(struct iio_dev *indio_dev,
194 const struct iio_chan_spec *chan,
195 enum iio_event_type type,
196 enum iio_event_direction dir,
197 enum iio_event_info info, int *val, int *val2)
198 {
199 struct max30210_state *st = iio_priv(indio_dev);
200
201 if (info != IIO_EV_INFO_VALUE)
202 return -EINVAL;
203
204 switch (dir) {
205 case IIO_EV_DIR_RISING:
206 switch (type) {
207 case IIO_EV_TYPE_THRESH:
208 return max30210_read_temp(st->regmap, MAX30210_TEMP_ALM_HI_REG, val);
209 default:
210 return -EINVAL;
211 }
212 case IIO_EV_DIR_FALLING:
213 switch (type) {
214 case IIO_EV_TYPE_THRESH:
215 return max30210_read_temp(st->regmap, MAX30210_TEMP_ALM_LO_REG, val);
216 default:
217 return -EINVAL;
218 }
219 default:
220 return -EINVAL;
221 }
222 }
223
224 static int max30210_write_event(struct iio_dev *indio_dev,
225 const struct iio_chan_spec *chan,
226 enum iio_event_type type,
227 enum iio_event_direction dir,
228 enum iio_event_info info, int val, int val2)
229 {
230 struct max30210_state *st = iio_priv(indio_dev);
231
232 if (info != IIO_EV_INFO_VALUE)
233 return -EINVAL;
234
235 switch (dir) {
236 case IIO_EV_DIR_RISING:
237 switch (type) {
238 case IIO_EV_TYPE_THRESH:
239 return max30210_write_temp(st->regmap, MAX30210_TEMP_ALM_HI_REG, val);
240 default:
241 return -EINVAL;
242 }
243 case IIO_EV_DIR_FALLING:
244 switch (type) {
245 case IIO_EV_TYPE_THRESH:
246 return max30210_write_temp(st->regmap, MAX30210_TEMP_ALM_LO_REG, val);
247 default:
248 return -EINVAL;
249 }
250 default:
251 return -EINVAL;
252 }
253 }
254
255 static int max30210_read_event_config(struct iio_dev *indio_dev,
256 const struct iio_chan_spec *chan,
257 enum iio_event_type type,
258 enum iio_event_direction dir)
259 {
260 struct max30210_state *st = iio_priv(indio_dev);
261 unsigned int val;
262 int ret;
263
264 ret = regmap_read(st->regmap, MAX30210_INT_EN_REG, &val);
265 if (ret)
266 return ret;
267
268 switch (dir) {
269 case IIO_EV_DIR_RISING:
270 switch (type) {
271 case IIO_EV_TYPE_THRESH:
> 272 return FIELD_GET(MAX30210_STATUS_TEMP_HI_MASK, val);
273 default:
274 return -EINVAL;
275 }
276 case IIO_EV_DIR_FALLING:
277 switch (type) {
278 case IIO_EV_TYPE_THRESH:
279 return FIELD_GET(MAX30210_STATUS_TEMP_LO_MASK, val);
280 default:
281 return -EINVAL;
282 }
283 default:
284 return -EINVAL;
285 }
286 }
287
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2 2/2] iio: temperature: add support for Analog Devices MAX30210
2026-03-04 12:25 ` [PATCH v2 2/2] iio: temperature: add support for Analog Devices MAX30210 John Erasmus Mari Geronimo
2026-03-05 0:34 ` kernel test robot
2026-03-05 0:45 ` kernel test robot
@ 2026-03-05 0:56 ` David Lechner
2026-03-07 12:38 ` Jonathan Cameron
3 siblings, 0 replies; 11+ messages in thread
From: David Lechner @ 2026-03-05 0:56 UTC (permalink / raw)
To: John Erasmus Mari Geronimo, linux-iio, jic23; +Cc: nuno.sa, andy, linux-kernel
On 3/4/26 6:25 AM, John Erasmus Mari Geronimo wrote:
> Add support for the Analog Devices MAX30210 I2C temperature
> sensor.
>
> The driver uses regmap for register access and integrates with
> the IIO framework. It supports:
>
> - Direct mode temperature conversion
> - Configurable sampling frequency
> - Threshold events
> - FIFO operation with IIO kfifo buffer support
> - Optional interrupt-driven data ready signaling
>
> The device provides 16-bit signed temperature data and a
> 64-sample FIFO.
>
> Signed-off-by: John Erasmus Mari Geronimo <johnerasmusmari.geronimo@analog.com>
> ---
> MAINTAINERS | 1 +
> drivers/iio/temperature/Kconfig | 10 +
> drivers/iio/temperature/Makefile | 1 +
> drivers/iio/temperature/max30210.c | 664 +++++++++++++++++++++++++++++
> 4 files changed, 676 insertions(+)
> create mode 100644 drivers/iio/temperature/max30210.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 09345b9f32ed..2abdbcf3a8e5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1644,6 +1644,7 @@ L: linux-iio@vger.kernel.org
> S: Supported
> W: https://ez.analog.com/linux-software-drivers
> F: Documentation/devicetree/bindings/iio/temperature/adi,max30210.yaml
> +F: drivers/iio/temperature/max30210.c
>
> ANALOG DEVICES INC ADA4250 DRIVER
> M: Antoniu Miclaus <antoniu.miclaus@analog.com>
> diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig
> index 9328b2250ace..b396757eb761 100644
> --- a/drivers/iio/temperature/Kconfig
> +++ b/drivers/iio/temperature/Kconfig
> @@ -184,4 +184,14 @@ config MCP9600
> This driver can also be built as a module. If so, the module
> will be called mcp9600.
>
Let's keep these in order too. MAX30... goes before MAX31...
> +config MAX30210
> + tristate "Analog Devices MAX30210 temperature sensor"
> + depends on I2C
> + help
> + If you say yes here you get support for MAX30210 low-power digital
> + temperature sensor chip connected via I2C.
> +
> + This driver can also be build as a module. If so, the module
> + will be called max30210.
> +
> endmenu
> diff --git a/drivers/iio/temperature/Makefile b/drivers/iio/temperature/Makefile
> index 07d6e65709f7..e5aad14dc09b 100644
> --- a/drivers/iio/temperature/Makefile
> +++ b/drivers/iio/temperature/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_LTC2983) += ltc2983.o
> obj-$(CONFIG_HID_SENSOR_TEMP) += hid-sensor-temperature.o
> obj-$(CONFIG_MAXIM_THERMOCOUPLE) += maxim_thermocouple.o
> obj-$(CONFIG_MAX30208) += max30208.o
> +obj-$(CONFIG_MAX30210) += max30210.o
> obj-$(CONFIG_MAX31856) += max31856.o
> obj-$(CONFIG_MAX31865) += max31865.o
> obj-$(CONFIG_MCP9600) += mcp9600.o
> diff --git a/drivers/iio/temperature/max30210.c b/drivers/iio/temperature/max30210.c
> new file mode 100644
> index 000000000000..839ed9830957
> --- /dev/null
> +++ b/drivers/iio/temperature/max30210.c
> @@ -0,0 +1,664 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Analog Devices MAX30210 I2C Temperature Sensor driver
> + *
> + * Copyright 2026 Analog Devices Inc.
> + */
> +
Looks like we are missing some headers here. We try to include
what is actually used in the file rather than relying on includes
from other includes.
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/kfifo_buf.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/regmap.h>
> +#include <linux/units.h>
> +
> +#define MAX30210_STATUS_REG 0x00
> +#define MAX30210_INT_EN_REG 0x02
> +#define MAX30210_FIFO_DATA_REG 0x08
> +#define MAX30210_FIFO_CONF_1_REG 0x09
> +#define MAX30210_FIFO_CONF_2_REG 0x0A
> +#define MAX30210_SYS_CONF_REG 0x11
> +#define MAX30210_PIN_CONF_REG 0x12
> +#define MAX30210_TEMP_ALM_HI_REG 0x22
> +#define MAX30210_TEMP_ALM_LO_REG 0x24
> +#define MAX30210_TEMP_INC_THRESH_REG 0x26
> +#define MAX30210_TEMP_DEC_THRESH_REG 0x27
> +#define MAX30210_TEMP_CONF_1_REG 0x28
> +#define MAX30210_TEMP_CONF_2_REG 0x29
> +#define MAX30210_TEMP_CONV_REG 0x2A
> +#define MAX30210_TEMP_DATA_REG 0x2B
> +#define MAX30210_TEMP_SLOPE_REG 0x2D
> +#define MAX30210_UNIQUE_ID_REG 0x30
> +#define MAX30210_PART_ID_REG 0xFF
> +
> +#define MAX30210_STATUS_A_FULL_MASK BIT(7)
> +#define MAX30210_STATUS_TEMP_RDY_MASK BIT(6)
> +#define MAX30210_STATUS_TEMP_DEC_MASK BIT(5)
> +#define MAX30210_STATUS_TEMP_INC_MASK BIT(4)
> +#define MAX30210_STATUS_TEMP_LO_MASK BIT(3)
> +#define MAX30210_STATUS_TEMP_HI_MASK BIT(2)
> +#define MAX30210_STATUS_PWR_RDY_MASK BIT(0)
> +
> +#define MAX30210_FIFOCONF1_FLUSH_FIFO_MASK BIT(4)
> +
> +#define MAX30210_SYSCONF_RESET_MASK BIT(0)
> +
> +#define MAX30210_PINCONF_EXT_CNV_EN_MASK BIT(7)
> +#define MAX30210_PINCONF_EXT_CVT_ICFG_MASK BIT(6)
> +#define MAX30210_PINCONF_INT_FCFG_MASK GENMASK(3, 2)
> +#define MAX30210_PINCONF_INT_OCFG_MASK GENMASK(1, 0)
> +
> +#define MAX30210_TEMPCONF1_CHG_DET_EN_MASK BIT(3)
> +#define MAX30210_TEMPCONF1_RATE_CHG_FILTER_MASK GENMASK(2, 0)
> +
> +#define MAX30210_TEMPCONF2_TEMP_PERIOD_MASK GENMASK(3, 0)
> +#define MAX30210_TEMPCONF2_ALERT_MODE_MASK BIT(7)
> +
> +#define MAX30210_TEMPCONV_AUTO_MASK BIT(1)
> +#define MAX30210_TEMPCONV_CONV_T_MASK BIT(0)
> +
> +#define MAX30210_PART_ID 0x45
> +#define MAX30210_FIFO_SIZE 64
> +#define MAX30210_FIFO_INVAL_DATA GENMASK(23, 0)
> +#define MAX30210_WATERMARK_DEFAULT (0x40 - 0x1F)
> +
> +#define MAX30210_UNIQUE_ID_LEN 6
> +#define MAX30210_EXT_CVT_FREQ_MIN 1
> +#define MAX30210_EXT_CVT_FREQ_MAX 20
> +
> +struct max30210_state {
> + struct regmap *regmap;
> + u8 watermark;
> + /* Raw FIFO byte buffer (3 bytes per sample) */
This 3 is used a few other places in the code, so might be nice to have
a macro for that.
> + u8 fifo_buf[3 * MAX30210_FIFO_SIZE];
> +};
> +
> +static const int max30210_samp_freq_avail[][2] = {
> + { 0, 15625 },
> + { 0, 31250 },
> + { 0, 62500 },
> + { 0, 125000 },
> + { 0, 250000 },
> + { 0, 500000 },
> + { 1, 0 },
> + { 2, 0 },
> + { 4, 0 },
> + { 8, 0 },
> +};
> +
> +static const struct regmap_config max30210_regmap = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = MAX30210_PART_ID_REG,
> +};
> +
> +static int max30210_read_temp(struct regmap *regmap, unsigned int reg,
> + int *temp)
> +{
> + __be16 val;
> + int ret;
> +
> + ret = regmap_bulk_read(regmap, reg, &val, sizeof(val));
> + if (ret)
> + return ret;
> +
> + *temp = sign_extend32(be16_to_cpu(val), 15);
> +
> + return IIO_VAL_INT;
> +}
> +
> +static int max30210_write_temp(struct regmap *regmap, unsigned int reg,
> + int temp)
> +{
> + __be16 val;
> +
> + val = cpu_to_be16(temp);
> +
> + return regmap_bulk_write(regmap, reg, &val, sizeof(val));
> +}
> +
> +static void max30210_fifo_read(struct iio_dev *indio_dev)
> +{
> + struct max30210_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + ret = regmap_bulk_read(st->regmap, MAX30210_FIFO_DATA_REG,
> + st->fifo_buf, 3 * st->watermark);
> + if (ret) {
> + dev_err(&indio_dev->dev, "Failed to read from fifo.\n");
> + return;
> + }
> +
> + for (unsigned int i = 0; i < st->watermark; i++) {
> + u32 raw = get_unaligned_be24(&st->fifo_buf[3 * i]);
> +
> + if (raw == MAX30210_FIFO_INVAL_DATA) {
> + dev_err_ratelimited(&indio_dev->dev, "Invalid data\n");
> + continue;
> + }
> +
> + s16 temp = (s16)(raw & 0xFFFF);
Can use FIELD_GET().
> +
> + iio_push_to_buffers(indio_dev, &temp);
> + }
> +}
> +
> +static irqreturn_t max30210_irq_handler(int irq, void *dev_id)
> +{
> + struct iio_dev *indio_dev = dev_id;
> + struct max30210_state *st = iio_priv(indio_dev);
> + unsigned int status;
> + int ret;
> +
> + ret = regmap_read(st->regmap, MAX30210_STATUS_REG, &status);
> + if (ret) {
> + dev_err(&indio_dev->dev, "Status read failed\n");
> + return IRQ_NONE;
> + }
> +
> + if (status & MAX30210_STATUS_A_FULL_MASK)
> + max30210_fifo_read(indio_dev);
> +
> + if (status & MAX30210_STATUS_TEMP_HI_MASK)
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_TEMP, 0,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_RISING),
> + iio_get_time_ns(indio_dev));
> +
> + if (status & MAX30210_STATUS_TEMP_LO_MASK)
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_TEMP, 0,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_FALLING),
> + iio_get_time_ns(indio_dev));
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int max30210_reg_access(struct iio_dev *indio_dev, unsigned int reg,
> + unsigned int writeval, unsigned int *readval)
> +{
> + struct max30210_state *st = iio_priv(indio_dev);
> +
> + if (!readval)
Usual way is to swap these to avoid the !.
> + return regmap_write(st->regmap, reg, writeval);
> +
> + return regmap_read(st->regmap, reg, readval);
> +}
> +
> +static int max30210_read_event(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info, int *val, int *val2)
> +{
> + struct max30210_state *st = iio_priv(indio_dev);
> +
> + if (info != IIO_EV_INFO_VALUE)
> + return -EINVAL;
> +
> + switch (dir) {
> + case IIO_EV_DIR_RISING:
> + switch (type) {
> + case IIO_EV_TYPE_THRESH:
> + return max30210_read_temp(st->regmap, MAX30210_TEMP_ALM_HI_REG, val);
> + default:
> + return -EINVAL;
> + }
> + case IIO_EV_DIR_FALLING:
> + switch (type) {
> + case IIO_EV_TYPE_THRESH:
> + return max30210_read_temp(st->regmap, MAX30210_TEMP_ALM_LO_REG, val);
> + default:
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int max30210_write_event(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info, int val, int val2)
> +{
> + struct max30210_state *st = iio_priv(indio_dev);
> +
> + if (info != IIO_EV_INFO_VALUE)
> + return -EINVAL;
No range checking on val?
> +
> + switch (dir) {
> + case IIO_EV_DIR_RISING:
> + switch (type) {
> + case IIO_EV_TYPE_THRESH:
> + return max30210_write_temp(st->regmap, MAX30210_TEMP_ALM_HI_REG, val);
> + default:
> + return -EINVAL;
> + }
> + case IIO_EV_DIR_FALLING:
> + switch (type) {
> + case IIO_EV_TYPE_THRESH:
> + return max30210_write_temp(st->regmap, MAX30210_TEMP_ALM_LO_REG, val);
> + default:
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int max30210_read_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir)
> +{
> + struct max30210_state *st = iio_priv(indio_dev);
> + unsigned int val;
> + int ret;
> +
> + ret = regmap_read(st->regmap, MAX30210_INT_EN_REG, &val);
> + if (ret)
> + return ret;
> +
> + switch (dir) {
> + case IIO_EV_DIR_RISING:
> + switch (type) {
> + case IIO_EV_TYPE_THRESH:
> + return FIELD_GET(MAX30210_STATUS_TEMP_HI_MASK, val);
> + default:
> + return -EINVAL;
> + }
> + case IIO_EV_DIR_FALLING:
> + switch (type) {
> + case IIO_EV_TYPE_THRESH:
> + return FIELD_GET(MAX30210_STATUS_TEMP_LO_MASK, val);
> + default:
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int max30210_write_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir, bool state)
> +{
> + struct max30210_state *st = iio_priv(indio_dev);
> +
> + switch (dir) {
> + case IIO_EV_DIR_RISING:
> + switch (type) {
> + case IIO_EV_TYPE_THRESH:
> + return regmap_assign_bits(st->regmap, MAX30210_INT_EN_REG,
> + MAX30210_STATUS_TEMP_HI_MASK, state);
> + default:
> + return -EINVAL;
> + }
> + case IIO_EV_DIR_FALLING:
> + switch (type) {
> + case IIO_EV_TYPE_THRESH:
> + return regmap_assign_bits(st->regmap, MAX30210_INT_EN_REG,
> + MAX30210_STATUS_TEMP_LO_MASK, state);
> + default:
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int max30210_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int *val,
> + int *val2, long mask)
> +{
> + struct max30210_state *st = iio_priv(indio_dev);
> + unsigned int uval;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + *val = 5;
> + *val2 = 1000;
> +
> + return IIO_VAL_FRACTIONAL;
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + ret = regmap_read(st->regmap, MAX30210_TEMP_CONF_2_REG, &uval);
> + if (ret)
> + return ret;
> +
> + uval = FIELD_GET(MAX30210_TEMPCONF2_TEMP_PERIOD_MASK, uval);
> +
Could use a comment here to explain code below. It makes sense when you
read the datasheet, but looks odd without that context.
> + if (uval >= ARRAY_SIZE(max30210_samp_freq_avail))
> + uval = ARRAY_SIZE(max30210_samp_freq_avail) - 1;
> +
> + *val = max30210_samp_freq_avail[uval][0];
> + *val2 = max30210_samp_freq_avail[uval][1];
> +
> + return IIO_VAL_FRACTIONAL;
> + case IIO_CHAN_INFO_RAW: {
Techicnally, there is a race condition here. We could be exiting buffer mode
here. So this looks like one of those rare cases where we should use
IIO_DEV_GUARD_CURRENT_MODE().
Typically we just don't allow reading raw during buffered read though
unless there is a real-world use case for it.
> + if (iio_buffer_enabled(indio_dev))
> + return max30210_read_temp(st->regmap, MAX30210_TEMP_DATA_REG, val);
> +
> + IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
> + if (IIO_DEV_ACQUIRE_FAILED(claim))
> + return -EBUSY;
> +
> + ret = regmap_write(st->regmap, MAX30210_TEMP_CONV_REG,
> + MAX30210_TEMPCONV_CONV_T_MASK);
> + if (ret)
> + return ret;
> +
> + /*
> + * Wait until CONVERT_T auto-clears.
> + * Datasheet:
> + * tBIAS_WU = 260 µs
> + * tINT = 8 ms
> + *
> + * Worst-case conversion ≈ 8.26 ms.
> + * Use 10 ms timeout for margin.
> + */
> + ret = regmap_read_poll_timeout(st->regmap, MAX30210_TEMP_CONV_REG, uval,
> + !(uval & MAX30210_TEMPCONV_CONV_T_MASK),
> + 500, /* poll every 500 µs */
> + 10000); /* 10 ms timeout */
> + if (ret)
> + return ret;
> +
> + return max30210_read_temp(st->regmap, MAX30210_TEMP_DATA_REG, val);
> + }
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int max30210_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 = &max30210_samp_freq_avail[0][0];
> + *type = IIO_VAL_INT_PLUS_MICRO;
> + *length = ARRAY_SIZE(max30210_samp_freq_avail) * 2;
> +
> + return IIO_AVAIL_LIST;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int max30210_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int val,
> + int val2, long mask)
> +{
> + struct max30210_state *st = iio_priv(indio_dev);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ: {
> + IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
> + if (IIO_DEV_ACQUIRE_FAILED(claim))
> + return -EBUSY;
> +
> + if (val < 0 || val2 < 0)
> + return -EINVAL;
> +
> + for (unsigned int i = 0; i < ARRAY_SIZE(max30210_samp_freq_avail); i++) {
> + if (val == max30210_samp_freq_avail[i][0] &&
> + val2 == max30210_samp_freq_avail[i][1]) {
> + return regmap_update_bits(st->regmap, MAX30210_TEMP_CONF_2_REG,
> + MAX30210_TEMPCONF2_TEMP_PERIOD_MASK,
> + FIELD_PREP(MAX30210_TEMPCONF2_TEMP_PERIOD_MASK, i));
> + }
> + }
> +
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int max30210_set_watermark(struct iio_dev *indio_dev, unsigned int val)
> +{
> + struct max30210_state *st = iio_priv(indio_dev);
> + unsigned int reg;
> + int ret;
> +
> + if (val < 1 || val > MAX30210_FIFO_SIZE)
> + return -EINVAL;
> +
> + reg = MAX30210_FIFO_SIZE - val;
This looks a bit odd and could use some explanation.
And even if it technically doesn't need it, would still expect a FIELD_PREP()
here since there are some reserved bits.
> +
> + ret = regmap_write(st->regmap, MAX30210_FIFO_CONF_1_REG, reg);
> + if (ret)
> + return ret;
> +
> + st->watermark = val;
> +
> + return 0;
> +}
> +
> +static ssize_t hwfifo_watermark_show(struct device *dev,
> + struct device_attribute *devattr,
> + char *buf)
> +{
> + struct max30210_state *st = iio_priv(dev_to_iio_dev(dev));
> +
> + return sysfs_emit(buf, "%d\n", st->watermark);
> +}
> +
> +IIO_STATIC_CONST_DEVICE_ATTR(hwfifo_watermark_min, "1");
> +IIO_STATIC_CONST_DEVICE_ATTR(hwfifo_watermark_max,
> + __stringify(MAX30210_FIFO_SIZE));
> +static IIO_DEVICE_ATTR_RO(hwfifo_watermark, 0);
> +
> +static const struct iio_dev_attr *max30210_fifo_attributes[] = {
> + &iio_dev_attr_hwfifo_watermark_min,
> + &iio_dev_attr_hwfifo_watermark_max,
> + &iio_dev_attr_hwfifo_watermark,
> + NULL
> +};
> +
> +static int max30210_buffer_preenable(struct iio_dev *indio_dev)
> +{
> + struct max30210_state *st = iio_priv(indio_dev);
> + int ret;
The register names are a bit cryptic. I wouldn't mind a high-level comment
explaining the intention here.
> +
> + ret = regmap_set_bits(st->regmap, MAX30210_INT_EN_REG, MAX30210_STATUS_A_FULL_MASK);
> + if (ret)
> + return ret;
> +
> + ret = regmap_set_bits(st->regmap, MAX30210_FIFO_CONF_2_REG,
> + MAX30210_FIFOCONF1_FLUSH_FIFO_MASK);
> + if (ret)
> + return ret;
> +
> + return regmap_write(st->regmap, MAX30210_TEMP_CONV_REG,
> + MAX30210_TEMPCONV_AUTO_MASK | MAX30210_TEMPCONV_CONV_T_MASK);
> +}
> +
> +static int max30210_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> + struct max30210_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + ret = regmap_write(st->regmap, MAX30210_TEMP_CONV_REG, 0x0);
> + if (ret)
> + return ret;
> +
> + ret = regmap_set_bits(st->regmap, MAX30210_FIFO_CONF_2_REG,
> + MAX30210_FIFOCONF1_FLUSH_FIFO_MASK);
> + if (ret)
> + return ret;
> +
> + return regmap_clear_bits(st->regmap, MAX30210_INT_EN_REG,
> + MAX30210_STATUS_A_FULL_MASK);
> +}
> +
> +static const struct iio_buffer_setup_ops max30210_buffer_ops = {
> + .preenable = max30210_buffer_preenable,
> + .postdisable = max30210_buffer_postdisable,
> +};
> +
> +static const struct iio_info max30210_info = {
> + .read_raw = max30210_read_raw,
> + .read_avail = max30210_read_avail,
> + .write_raw = max30210_write_raw,
> + .hwfifo_set_watermark = max30210_set_watermark,
> + .debugfs_reg_access = max30210_reg_access,
> + .read_event_value = max30210_read_event,
> + .write_event_value = max30210_write_event,
> + .write_event_config = max30210_write_event_config,
> + .read_event_config = max30210_read_event_config,
> +};
> +
> +static const struct iio_event_spec max30210_events[] = {
> + {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_RISING,
> + .mask_separate = BIT(IIO_EV_INFO_VALUE) | BIT(IIO_EV_INFO_ENABLE),
> + }, {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_FALLING,
> + .mask_separate = BIT(IIO_EV_INFO_VALUE) | BIT(IIO_EV_INFO_ENABLE),
> + },
> +};
> +
> +static const struct iio_chan_spec max30210_channels = {
> + .type = IIO_TEMP,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE) |
> + BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + .info_mask_separate_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + .scan_index = 0,
> + .event_spec = max30210_events,
> + .num_event_specs = ARRAY_SIZE(max30210_events),
> + .scan_type = {
> + .sign = 's',
> + .realbits = 16,
> + .storagebits = 16,
> + .shift = 0,
> + .endianness = IIO_CPU,
> + },
> +};
> +
> +static int max30210_setup(struct max30210_state *st, struct device *dev)
> +{
> + struct gpio_desc *powerdown_gpio;
> + unsigned int val;
> + int ret;
> +
> + /* Optional hardware reset via powerdown GPIO */
> + powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> + GPIOD_OUT_HIGH);
> + if (IS_ERR(powerdown_gpio))
> + return dev_err_probe(dev, PTR_ERR(powerdown_gpio),
> + "failed to request powerdown GPIO\n");
> +
> + if (powerdown_gpio) {
> + /* Deassert powerdown to power up device */
> + gpiod_set_value(powerdown_gpio, 0);
> + } else {
> + /* Software reset fallback */
> + ret = regmap_update_bits(st->regmap, MAX30210_SYS_CONF_REG,
regmap_set_bits()
> + MAX30210_SYSCONF_RESET_MASK,
> + MAX30210_SYSCONF_RESET_MASK);
> + if (ret)
> + return ret;
> + }
> +
Use IIO style multi-line comments:
/*
* Datasheet Figure 6:
> + /* Datasheet Figure 6:
> + * tPU max = 700 µs after power-up or reset before device is ready.
> + */
> + fsleep(700);
> +
> + /* Clear status byte */
> + return regmap_read(st->regmap, MAX30210_STATUS_REG, &val);
> +}
> +
> +static int max30210_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct iio_dev *indio_dev;
> + struct max30210_state *st;
> + int ret;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> + return -EOPNOTSUPP;
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> +
> + ret = devm_regulator_get_enable(dev, "vdd");
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Failed to enable vdd regulator.\n");
> +
> + st->regmap = devm_regmap_init_i2c(client, &max30210_regmap);
> + if (IS_ERR(st->regmap))
> + return dev_err_probe(dev, PTR_ERR(st->regmap),
> + "Failed to allocate regmap.\n");
> +
> + st->watermark = MAX30210_WATERMARK_DEFAULT;
> +
> + ret = max30210_setup(st, dev);
> + if (ret)
> + return ret;
> +
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->channels = &max30210_channels;
> + indio_dev->num_channels = 1;
> + indio_dev->name = "max30210";
> + indio_dev->info = &max30210_info;
> +
> + ret = devm_iio_kfifo_buffer_setup_ext(dev, indio_dev, &max30210_buffer_ops,
> + max30210_fifo_attributes);
> + if (ret)
> + return ret;
> +
> + if (client->irq) {
The way the driver is written currently, the interrup is required.
Either we need to reutrn error on !client->irq or we need to have a second
max30210_info that excludes buffer and events.
> + ret = devm_request_irq(dev, client->irq, max30210_irq_handler, IRQF_NO_THREAD,
> + indio_dev->name, indio_dev);
> + if (ret)
> + return ret;
> + }
> +
> + return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +static const struct i2c_device_id max30210_id[] = {
> + { "max30210" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, max30210_id);
> +
> +static const struct of_device_id max30210_of_match[] = {
> + { .compatible = "adi,max30210" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, max30210_of_match);
> +
> +static struct i2c_driver max30210_driver = {
> + .driver = {
> + .name = "max30210",
> + .of_match_table = max30210_of_match,
> + },
> + .probe = max30210_probe,
> + .id_table = max30210_id,
> +};
> +module_i2c_driver(max30210_driver);
> +
> +MODULE_AUTHOR("John Erasmus Mari Geronimo <johnerasmusmari.geronimo@analog.com");
> +MODULE_DESCRIPTION("MAX30210 low-power digital temperature sensor");
> +MODULE_LICENSE("GPL");
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2 2/2] iio: temperature: add support for Analog Devices MAX30210
2026-03-04 12:25 ` [PATCH v2 2/2] iio: temperature: add support for Analog Devices MAX30210 John Erasmus Mari Geronimo
` (2 preceding siblings ...)
2026-03-05 0:56 ` David Lechner
@ 2026-03-07 12:38 ` Jonathan Cameron
3 siblings, 0 replies; 11+ messages in thread
From: Jonathan Cameron @ 2026-03-07 12:38 UTC (permalink / raw)
To: John Erasmus Mari Geronimo
Cc: linux-iio, dlechner, nuno.sa, andy, linux-kernel
On Wed, 4 Mar 2026 20:25:09 +0800
John Erasmus Mari Geronimo <johnerasmusmari.geronimo@analog.com> wrote:
> Add support for the Analog Devices MAX30210 I2C temperature
> sensor.
>
> The driver uses regmap for register access and integrates with
> the IIO framework. It supports:
>
> - Direct mode temperature conversion
> - Configurable sampling frequency
> - Threshold events
> - FIFO operation with IIO kfifo buffer support
> - Optional interrupt-driven data ready signaling
>
> The device provides 16-bit signed temperature data and a
> 64-sample FIFO.
>
> Signed-off-by: John Erasmus Mari Geronimo <johnerasmusmari.geronimo@analog.com>
Hi John
A few additional comment from me, but overall this is coming together nicely.
Thanks,
Jonathan
> diff --git a/drivers/iio/temperature/max30210.c b/drivers/iio/temperature/max30210.c
> new file mode 100644
> index 000000000000..839ed9830957
> --- /dev/null
> +++ b/drivers/iio/temperature/max30210.c
> +static void max30210_fifo_read(struct iio_dev *indio_dev)
> +{
> + struct max30210_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + ret = regmap_bulk_read(st->regmap, MAX30210_FIFO_DATA_REG,
> + st->fifo_buf, 3 * st->watermark);
> + if (ret) {
> + dev_err(&indio_dev->dev, "Failed to read from fifo.\n");
> + return;
> + }
> +
> + for (unsigned int i = 0; i < st->watermark; i++) {
> + u32 raw = get_unaligned_be24(&st->fifo_buf[3 * i]);
> +
> + if (raw == MAX30210_FIFO_INVAL_DATA) {
Whilst this aligns with how the datasheet describes it, the
only bit that is definitely different for invalid data is bit 7 of the
tag. So could just check that. The code to get the temperature value then
just becomes memcpy() with the channel described as bit endian.
I don't mind if you prefer it this way though, just thought I'd raise
the possibility.
> + dev_err_ratelimited(&indio_dev->dev, "Invalid data\n");
> + continue;
> + }
> +
> + s16 temp = (s16)(raw & 0xFFFF);
> +
> + iio_push_to_buffers(indio_dev, &temp);
> + }
> +}
> +static int max30210_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int *val,
> + int *val2, long mask)
> +{
> + struct max30210_state *st = iio_priv(indio_dev);
> + unsigned int uval;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + *val = 5;
> + *val2 = 1000;
> +
> + return IIO_VAL_FRACTIONAL;
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + ret = regmap_read(st->regmap, MAX30210_TEMP_CONF_2_REG, &uval);
> + if (ret)
> + return ret;
> +
> + uval = FIELD_GET(MAX30210_TEMPCONF2_TEMP_PERIOD_MASK, uval);
> +
> + if (uval >= ARRAY_SIZE(max30210_samp_freq_avail))
> + uval = ARRAY_SIZE(max30210_samp_freq_avail) - 1;
uval = min(uval, ARRAY_SIZE(max30210_samp_freq_avail) - 1);
perhaps?
> +
> + *val = max30210_samp_freq_avail[uval][0];
> + *val2 = max30210_samp_freq_avail[uval][1];
> +
> + return IIO_VAL_FRACTIONAL;
> +static int max30210_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int val,
> + int val2, long mask)
> +{
> + struct max30210_state *st = iio_priv(indio_dev);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ: {
> + IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
> + if (IIO_DEV_ACQUIRE_FAILED(claim))
> + return -EBUSY;
> +
> + if (val < 0 || val2 < 0)
> + return -EINVAL;
> +
> + for (unsigned int i = 0; i < ARRAY_SIZE(max30210_samp_freq_avail); i++) {
> + if (val == max30210_samp_freq_avail[i][0] &&
> + val2 == max30210_samp_freq_avail[i][1]) {
Flip the logic to reduce indent.
if (val != max...[0] ||
val != max...[1])
continue;
return....
> + return regmap_update_bits(st->regmap, MAX30210_TEMP_CONF_2_REG,
> + MAX30210_TEMPCONF2_TEMP_PERIOD_MASK,
> + FIELD_PREP(MAX30210_TEMPCONF2_TEMP_PERIOD_MASK, i));
> + }
> + }
> +
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
> +}
> +static int max30210_buffer_preenable(struct iio_dev *indio_dev)
> +{
> + struct max30210_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + ret = regmap_set_bits(st->regmap, MAX30210_INT_EN_REG, MAX30210_STATUS_A_FULL_MASK);
I would wrap this one. Won't hurt readability and keeps it inline with the wrapping
on the clear in postdisable()
> + if (ret)
> + return ret;
> +
> + ret = regmap_set_bits(st->regmap, MAX30210_FIFO_CONF_2_REG,
> + MAX30210_FIFOCONF1_FLUSH_FIFO_MASK);
> + if (ret)
> + return ret;
> +
> + return regmap_write(st->regmap, MAX30210_TEMP_CONV_REG,
> + MAX30210_TEMPCONV_AUTO_MASK | MAX30210_TEMPCONV_CONV_T_MASK);
> +}
> +
> +static int max30210_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> + struct max30210_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + ret = regmap_write(st->regmap, MAX30210_TEMP_CONV_REG, 0x0);
> + if (ret)
> + return ret;
> +
> + ret = regmap_set_bits(st->regmap, MAX30210_FIFO_CONF_2_REG,
> + MAX30210_FIFOCONF1_FLUSH_FIFO_MASK);
> + if (ret)
> + return ret;
> +
> + return regmap_clear_bits(st->regmap, MAX30210_INT_EN_REG,
> + MAX30210_STATUS_A_FULL_MASK);
> +}
> +
> +static const struct iio_chan_spec max30210_channels = {
only one. So max30210_channel
> + .type = IIO_TEMP,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE) |
> + BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + .info_mask_separate_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + .scan_index = 0,
> + .event_spec = max30210_events,
> + .num_event_specs = ARRAY_SIZE(max30210_events),
> + .scan_type = {
> + .sign = 's',
> + .realbits = 16,
> + .storagebits = 16,
> + .shift = 0,
A zero shift is kind of considered the obvious default, so we normally don't
bother setting it explicitly.
> + .endianness = IIO_CPU,
> + },
> +};
> +
> +static int max30210_setup(struct max30210_state *st, struct device *dev)
> +{
> + struct gpio_desc *powerdown_gpio;
> + unsigned int val;
> + int ret;
> +
> + /* Optional hardware reset via powerdown GPIO */
> + powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> + GPIOD_OUT_HIGH);
> + if (IS_ERR(powerdown_gpio))
> + return dev_err_probe(dev, PTR_ERR(powerdown_gpio),
> + "failed to request powerdown GPIO\n");
> +
> + if (powerdown_gpio) {
> + /* Deassert powerdown to power up device */
> + gpiod_set_value(powerdown_gpio, 0);
> + } else {
> + /* Software reset fallback */
> + ret = regmap_update_bits(st->regmap, MAX30210_SYS_CONF_REG,
> + MAX30210_SYSCONF_RESET_MASK,
> + MAX30210_SYSCONF_RESET_MASK);
> + if (ret)
> + return ret;
> + }
> +
> + /* Datasheet Figure 6:
/*
* Datasheet..
> + * tPU max = 700 µs after power-up or reset before device is ready.
> + */
> + fsleep(700);
> +
> + /* Clear status byte */
> + return regmap_read(st->regmap, MAX30210_STATUS_REG, &val);
> +}
> +
> +static int max30210_probe(struct i2c_client *client)
> +{
...
> + ret = devm_iio_kfifo_buffer_setup_ext(dev, indio_dev, &max30210_buffer_ops,
> + max30210_fifo_attributes);
> + if (ret)
> + return ret;
> +
> + if (client->irq) {
> + ret = devm_request_irq(dev, client->irq, max30210_irq_handler, IRQF_NO_THREAD,
You are doing a bunch of bus accesses in your handler and I can't see anything int here
that wouldn't work in a thread. So I think this is backwards. All the work should be
done in an interrupt thread.
> + indio_dev->name, indio_dev);
> + if (ret)
> + return ret;
> + }
> +
> + return devm_iio_device_register(dev, indio_dev);
> +}
^ permalink raw reply [flat|nested] 11+ messages in thread