[PATCH v6 0/2] Add support for Analog Devices MAX30210

[PATCH v6 0/2] Add support for Analog Devices MAX30210

[John Erasmus Mari Geronimo]

This series adds support for the Analog Devices MAX30210 I2C
temperature sensor.

Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/max30210.pdf

The device provides a 16-bit temperature output and includes
features such as a hardware FIFO, programmable sampling rate,
threshold alarms, and interrupt support. The FIFO is exposed as an
IIO buffered capture (kfifo buffer, fed from the FIFO watermark
interrupt) and the threshold alarms as IIO events. As far as I can
tell hwmon has no equivalent for streaming samples out of a hardware
FIFO, and the related MAX30208 is an IIO driver as well.

Although the MAX30210 belongs to the same device family as the
MAX30208, it is implemented as a separate driver. The MAX30210 has
a significantly different feature set, including a 64-sample hardware
FIFO with configurable watermark interrupts, programmable sampling
rates, and event support. These features require buffered operation
and IRQ-driven data handling which are not present in the MAX30208
driver (which talks to the device directly via i2c_smbus_*() calls
with no regmap, and has no IIO buffer or event interface at all).

In addition, the MAX30210 uses a different register map and a more
flexible I2C address selection scheme. Supporting both devices in a
single driver would introduce substantial conditional logic, so a
separate driver keeps the implementation simpler and easier to
maintain.

Patch 1 adds the devicetree binding.
Patch 2 adds the IIO driver implementation.

Changes since v5:
https://lore.kernel.org/all/20261023080623.1830-1-johnerasmusmari.geronimo@analog.com/
- Fix the temperature scale: one LSB is 5 milli degrees Celsius, so
  report 5 (IIO_VAL_INT) instead of 5/1000.
- Mask the interrupt status with the enabled interrupts, so threshold
  events and the FIFO that are not enabled are not serviced.
- Start the FIFO interrupt and the autonomous conversion in
  postenable(), after the watermark has been set, and stop them in
  predisable() followed by synchronize_irq(). This also removes the
  goto from preenable(), which mixed a guard with a goto.
- Read STATUS before enabling the FIFO interrupt so a stale A_FULL
  flag does not fire on the freshly flushed FIFO.
- Clamp the hardware FIFO watermark to the FIFO size instead of
  failing.
- Mark SYS_CONF_REG volatile (its reset bit is self-clearing).
- Use gpiod_set_value_cansleep() for the powerdown GPIO.
- Keep the powerdown GPIO asserted for 10 us before deasserting it,
  the datasheet lists a CVT/PDB input low-pulse width of 5 us.
- Do not expose the threshold events when there is no interrupt.
- Review cleanups: check the event type first, use a local regmap
  variable, regmap_test_bits(), simpler set/clear handling in
  write_event_config(), a logical split of the poll call, and rename
  the sampling frequency index to freq_idx.
- Not changed: the interrupt is still requested after
  devm_iio_device_register(), because the event interface is freed
  in iio_device_unregister_eventset(), and autoconv_put() still drops
  its reference when the register write fails (a later direct read
  rewrites TEMP_CONV_REG without AUTO, which stops it).
- Cover letter: removed the incorrect statement about hwmon and
  interrupts, thanks to Guenter for pointing it out.

Tested on real MAX30210 hardware (Raspberry Pi 4) with a 6.12 based
vendor kernel, which has no KASAN or lockdep; this version is only
build-tested against mainline. What was run: the sysfs scale against
the raw reading, FIFO streaming with a fresh first sample, the
watermark clamp, threshold events with the buffer off, disabled
events staying silent while the FIFO interrupt runs (checked against
v5, which leaked them), 50 buffer enable/disable cycles, 80
rmmod/insmod cycles (20 with armed events and 60 timed to land within
about 250 ms of live interrupt traffic), the no-interrupt and the
no-powerdown-GPIO configurations, and the INT_EN/TEMP_CONV state the
chip is left in after removal.

Changes since v4:
https://lore.kernel.org/all/20261015012159.44-1-johnerasmusmari.geronimo@analog.com/
- Patch 1: restore Krzysztof's Reviewed-by, which was accidentally
  left out of v4.
- Add Datasheet: tags and a datasheet link in the driver header.
- Disable interrupt sources and autonomous conversion on removal,
  independently of whether a powerdown GPIO is present.
- Only take/release an autonomous-conversion reference when an
  interrupt enable bit really changes; repeated writes of the same
  value no longer leak a reference.
- buffer_preenable() now unwinds the A_FULL interrupt enable on
  failure and buffer_postdisable() always clears it.
- Return IRQ_NONE from the interrupt handler when none of the
  handled status bits is set.
- Wait tPU before the software reset when no powerdown GPIO is used.
- Minor review cleanups: error paths out of line in
  max30210_autoconv_get(), min() for the period clamp,
  devm_mutex_init(), designated initializer for i2c_device_id,
  header grouping, one-line val/val2/mask argument lists.

Changes since v3:
https://lore.kernel.org/all/cover.1789032019.git.johnerasmusmari.geronimo@analog.com/
- regmap_noinc_read() for FIFO reads instead of regmap_bulk_read(),
  which (combined with regmap caching) could silently return stale
  cached values of unrelated registers instead of real FIFO data.
- Mark both bytes of TEMP_DATA_REG volatile for the same reason.
- Decouple threshold events from the buffer via a refcounted
  autoconv_get()/autoconv_put() pair, so either can be used
  independently or concurrently.
- Don't let a single-shot direct-mode read race with/clobber
  autonomous conversion mode if a threshold event or the buffer is
  already using it -- return -EBUSY instead.
- Don't hard-fail probe when there is no interrupt; fall back to
  direct-mode-only operation.
- Fix devm teardown ordering so an in-flight interrupt can't use a
  freed event interface on removal.
- Re-assert the powerdown GPIO on removal so the device reaches
  standby instead of staying powered indefinitely.
- Style/API fixes from review (register/field grouping, _MASK
  naming, FIELD_PREP/regmap_assign_bits usage, C99 initializers,
  dropped redundant i2c_check_functionality() check, comment
  cleanup).

Changes since v2:
https://lore.kernel.org/all/20260304122509.67931-1-johnerasmusmari.geronimo@analog.com/
- Fix FIFO byte-count handling and sample decoding.
- Fix IIO debug register read/write access.
- Validate event threshold values and sampling-frequency register values.
- Return the correct sampling-frequency format and reject direct reads
  while buffered capture is enabled.
- Clean up the Kconfig entry and device-tree supply declaration.

John Erasmus Mari Geronimo (2):
  dt-bindings: iio: temperature: add ADI MAX30210
  iio: temperature: add support for Analog Devices MAX30210

 .../iio/temperature/adi,max30210.yaml         |  61 ++
 MAINTAINERS                                   |   8 +
 drivers/iio/temperature/Kconfig               |  13 +
 drivers/iio/temperature/Makefile              |   1 +
 drivers/iio/temperature/max30210.c            | 853 ++++++++++++++++++
 5 files changed, 936 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/temperature/adi,max30210.yaml
 create mode 100644 drivers/iio/temperature/max30210.c

-- 
2.34.1

[PATCH v6 1/2] dt-bindings: iio: temperature: add ADI MAX30210

[John Erasmus Mari Geronimo]

Add device tree binding documentation for the Analog Devices
MAX30210 temperature sensor.

Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/max30210.pdf
Signed-off-by: John Erasmus Mari Geronimo <johnerasmusmari.geronimo@analog.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
---
 .../iio/temperature/adi,max30210.yaml         | 61 +++++++++++++++++++
 MAINTAINERS                                   |  7 +++
 2 files changed, 68 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/temperature/adi,max30210.yaml

diff --git a/Documentation/devicetree/bindings/iio/temperature/adi,max30210.yaml b/Documentation/devicetree/bindings/iio/temperature/adi,max30210.yaml
new file mode 100644
index 0000000000000..8bfd154bbc438
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/temperature/adi,max30210.yaml
@@ -0,0 +1,61 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright 2026 Analog Devices Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/temperature/adi,max30210.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices MAX30210 Temperature Sensor
+
+maintainers:
+  - John Erasmus Mari Geronimo <johnerasmusmari.geronimo@analog.com>
+
+description: |
+  The MAX30210 is a temperature sensor with an I2C interface.
+  https://www.analog.com/media/en/technical-documentation/data-sheets/max30210.pdf
+
+properties:
+  compatible:
+    enum:
+      - adi,max30210
+
+  reg:
+    maxItems: 1
+
+  vdd-supply: true
+
+  powerdown-gpios:
+    description: GPIO connected to the CVT/PDB pin (active low).
+    maxItems: 1
+
+  interrupts:
+    description: Connected to INT pin. Interrupt triggered on both rising and falling edges.
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - vdd-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        temperature-sensor@40 {
+            compatible = "adi,max30210";
+            reg = <0x40>;
+            vdd-supply = <&vdd>;
+            powerdown-gpios = <&gpio 12 GPIO_ACTIVE_LOW>;
+
+            interrupt-parent = <&gpio>;
+            interrupts = <17 IRQ_TYPE_EDGE_BOTH>;
+        };
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 0017755f593bc..87e455b592461 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1786,6 +1786,13 @@ W:	https://ez.analog.com/linux-software-drivers
 F:	Documentation/devicetree/bindings/iio/dac/adi,max22007.yaml
 F:	drivers/iio/dac/max22007.c
 
+ANALOG DEVICES INC MAX30210 DRIVER
+M:	John Erasmus Mari Geronimo <johnerasmusmari.geronimo@analog.com>
+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
+
 ANALOG DEVICES INC ADA4250 DRIVER
 M:	Antoniu Miclaus <antoniu.miclaus@analog.com>
 L:	linux-iio@vger.kernel.org
-- 
2.34.1

[PATCH v6 2/2] iio: temperature: add support for Analog Devices MAX30210

[John Erasmus Mari Geronimo]

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.

Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/max30210.pdf
Signed-off-by: John Erasmus Mari Geronimo <johnerasmusmari.geronimo@analog.com>
---
 MAINTAINERS                        |   1 +
 drivers/iio/temperature/Kconfig    |  13 +
 drivers/iio/temperature/Makefile   |   1 +
 drivers/iio/temperature/max30210.c | 853 +++++++++++++++++++++++++++++
 4 files changed, 868 insertions(+)
 create mode 100644 drivers/iio/temperature/max30210.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 87e455b592461..47be5975c2b41 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1792,6 +1792,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 9328b2250aced..05b3af6872b31 100644
--- a/drivers/iio/temperature/Kconfig
+++ b/drivers/iio/temperature/Kconfig
@@ -152,6 +152,19 @@ config MAX30208
 	  This driver can also be built as a module. If so, the module
 	  will be called max30208.
 
+config MAX30210
+	tristate "Analog Devices MAX30210 temperature sensor"
+	depends on I2C
+	select REGMAP_I2C
+	select IIO_BUFFER
+	select IIO_KFIFO_BUF
+	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 built as a module. If so, the module
+	  will be called max30210.
+
 config MAX31856
 	tristate "MAX31856 thermocouple sensor"
 	depends on SPI
diff --git a/drivers/iio/temperature/Makefile b/drivers/iio/temperature/Makefile
index 0850bf6918204..bb9fdccc104e1 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 0000000000000..8866a476eb37c
--- /dev/null
+++ b/drivers/iio/temperature/max30210.c
@@ -0,0 +1,853 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Analog Devices MAX30210 I2C Temperature Sensor driver
+ *
+ * Copyright 2026 Analog Devices Inc.
+ *
+ * Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/max30210.pdf
+ */
+
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/cleanup.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/lockdep.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/sysfs.h>
+#include <linux/time.h>
+#include <linux/types.h>
+#include <linux/unaligned.h>
+
+#include <asm/byteorder.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>
+
+#define MAX30210_STATUS_REG			0x00
+#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		BIT(0)
+#define MAX30210_INT_EN_REG			0x02
+#define MAX30210_FIFO_DATA_REG			0x08
+#define MAX30210_FIFO_CONF_1_REG		0x09
+#define   MAX30210_FIFOCONF1_A_FULL_MASK	GENMASK(5, 0)
+#define MAX30210_FIFO_CONF_2_REG		0x0A
+#define   MAX30210_FIFOCONF2_FLUSH_FIFO		BIT(4)
+#define MAX30210_SYS_CONF_REG			0x11
+#define   MAX30210_SYSCONF_RESET		BIT(0)
+#define MAX30210_PIN_CONF_REG			0x12
+#define   MAX30210_PINCONF_EXT_CNV_EN		BIT(7)
+#define   MAX30210_PINCONF_EXT_CVT_ICFG		BIT(6)
+#define   MAX30210_PINCONF_INT_FCFG_MASK	GENMASK(3, 2)
+#define   MAX30210_PINCONF_INT_OCFG_MASK	GENMASK(1, 0)
+#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_TEMPCONF1_CHG_DET_EN		BIT(3)
+#define   MAX30210_TEMPCONF1_RATE_CHG_FILTER_MASK	GENMASK(2, 0)
+#define MAX30210_TEMP_CONF_2_REG		0x29
+#define   MAX30210_TEMPCONF2_TEMP_PERIOD_MASK	GENMASK(3, 0)
+#define   MAX30210_TEMPCONF2_ALERT_MODE		BIT(7)
+#define MAX30210_TEMP_CONV_REG			0x2A
+#define   MAX30210_TEMPCONV_CONV_T		BIT(0)
+#define   MAX30210_TEMPCONV_AUTO		BIT(1)
+#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_PART_ID			0x45
+#define MAX30210_FIFO_SIZE			64
+#define MAX30210_FIFO_BYTES_PER_SAMPLE		3
+#define MAX30210_FIFO_TEMP_MASK			GENMASK(15, 0)
+#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 iio_chan_spec channel;
+	struct regmap *regmap;
+	/* Raw FIFO byte buffer */
+	u8 fifo_buf[MAX30210_FIFO_BYTES_PER_SAMPLE * MAX30210_FIFO_SIZE];
+	/* Protects autoconv_users and the autonomous-conversion enable bit */
+	struct mutex lock;
+	unsigned int autoconv_users;
+	int irq;
+	u8 watermark;
+};
+
+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 bool max30210_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case MAX30210_STATUS_REG:
+	case MAX30210_FIFO_DATA_REG:
+	case MAX30210_FIFO_CONF_2_REG:
+	case MAX30210_TEMP_CONV_REG:
+	case MAX30210_SYS_CONF_REG:
+	/*
+	 * TEMP_DATA_REG is a 16-bit value spanning two byte registers;
+	 * both must be volatile or regmap_bulk_read()'s range check falls
+	 * back to caching the second byte forever (see max30210_read_temp()).
+	 */
+	case MAX30210_TEMP_DATA_REG:
+	case MAX30210_TEMP_DATA_REG + 1:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static const struct regmap_config max30210_regmap = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = MAX30210_PART_ID_REG,
+	.volatile_reg = max30210_volatile_reg,
+	.cache_type = REGCACHE_MAPLE,
+};
+
+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));
+}
+
+/* Keeps the device sampling autonomously while buffer and/or events need it. */
+static int max30210_autoconv_get(struct max30210_state *st)
+{
+	int ret;
+
+	lockdep_assert_held(&st->lock);
+
+	if (st->autoconv_users == 0) {
+		ret = regmap_write(st->regmap, MAX30210_TEMP_CONV_REG,
+				   MAX30210_TEMPCONV_AUTO | MAX30210_TEMPCONV_CONV_T);
+		if (ret)
+			return ret;
+	}
+
+	st->autoconv_users++;
+
+	return 0;
+}
+
+static int max30210_autoconv_put(struct max30210_state *st)
+{
+	int ret = 0;
+
+	lockdep_assert_held(&st->lock);
+
+	/* Drop the reference even if the write fails; a later direct read clears AUTO anyway. */
+	if (st->autoconv_users == 1)
+		ret = regmap_write(st->regmap, MAX30210_TEMP_CONV_REG, 0);
+	if (st->autoconv_users)
+		st->autoconv_users--;
+
+	return ret;
+}
+
+static void max30210_fifo_read(struct iio_dev *indio_dev)
+{
+	struct max30210_state *st = iio_priv(indio_dev);
+	int ret;
+
+	/*
+	 * FIFO_DATA_REG auto-increments in hardware; bulk_read would treat
+	 * each subsequent byte as a separate cacheable register.
+	 */
+	ret = regmap_noinc_read(st->regmap, MAX30210_FIFO_DATA_REG,
+				st->fifo_buf,
+				MAX30210_FIFO_BYTES_PER_SAMPLE * 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[MAX30210_FIFO_BYTES_PER_SAMPLE * i]);
+		s16 temp;
+
+		if (raw == MAX30210_FIFO_INVAL_DATA) {
+			dev_err_ratelimited(&indio_dev->dev, "Invalid data\n");
+			continue;
+		}
+
+		temp = sign_extend32(FIELD_GET(MAX30210_FIFO_TEMP_MASK, raw), 15);
+
+		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, enabled;
+	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;
+	}
+
+	/* STATUS flags are set whether or not they are enabled in INT_EN. */
+	ret = regmap_read(st->regmap, MAX30210_INT_EN_REG, &enabled);
+	if (ret) {
+		dev_err(&indio_dev->dev, "Interrupt enable read failed\n");
+		return IRQ_NONE;
+	}
+
+	status &= enabled;
+	if (!(status & (MAX30210_STATUS_A_FULL_MASK |
+			MAX30210_STATUS_TEMP_HI_MASK |
+			MAX30210_STATUS_TEMP_LO_MASK)))
+		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)
+		return regmap_read(st->regmap, reg, readval);
+
+	return regmap_write(st->regmap, reg, writeval);
+}
+
+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);
+	struct regmap *map = st->regmap;
+
+	if (type != IIO_EV_TYPE_THRESH || info != IIO_EV_INFO_VALUE)
+		return -EINVAL;
+
+	switch (dir) {
+	case IIO_EV_DIR_RISING:
+		return max30210_read_temp(map, MAX30210_TEMP_ALM_HI_REG, val);
+	case IIO_EV_DIR_FALLING:
+		return max30210_read_temp(map, MAX30210_TEMP_ALM_LO_REG, val);
+	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);
+	struct regmap *map = st->regmap;
+
+	if (type != IIO_EV_TYPE_THRESH || info != IIO_EV_INFO_VALUE)
+		return -EINVAL;
+
+	if (val < S16_MIN || val > S16_MAX)
+		return -EINVAL;
+
+	switch (dir) {
+	case IIO_EV_DIR_RISING:
+		return max30210_write_temp(map, MAX30210_TEMP_ALM_HI_REG, val);
+	case IIO_EV_DIR_FALLING:
+		return max30210_write_temp(map, MAX30210_TEMP_ALM_LO_REG, val);
+	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;
+
+	if (type != IIO_EV_TYPE_THRESH)
+		return -EINVAL;
+
+	ret = regmap_read(st->regmap, MAX30210_INT_EN_REG, &val);
+	if (ret)
+		return ret;
+
+	switch (dir) {
+	case IIO_EV_DIR_RISING:
+		return FIELD_GET(MAX30210_STATUS_TEMP_HI_MASK, val);
+	case IIO_EV_DIR_FALLING:
+		return FIELD_GET(MAX30210_STATUS_TEMP_LO_MASK, val);
+	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);
+	struct regmap *map = st->regmap;
+	unsigned int mask;
+	int ret;
+
+	if (type != IIO_EV_TYPE_THRESH)
+		return -EINVAL;
+
+	switch (dir) {
+	case IIO_EV_DIR_RISING:
+		mask = MAX30210_STATUS_TEMP_HI_MASK;
+		break;
+	case IIO_EV_DIR_FALLING:
+		mask = MAX30210_STATUS_TEMP_LO_MASK;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	guard(mutex)(&st->lock);
+
+	/* The core does not dedupe; only take/drop a reference on a real change. */
+	ret = regmap_test_bits(map, MAX30210_INT_EN_REG, mask);
+	if (ret < 0)
+		return ret;
+
+	if (!!ret == state)
+		return 0;
+
+	/*
+	 * Threshold events need the device sampling autonomously, independent
+	 * of whether the FIFO buffer is also enabled.
+	 */
+	if (state) {
+		ret = max30210_autoconv_get(st);
+		if (ret)
+			return ret;
+
+		ret = regmap_set_bits(map, MAX30210_INT_EN_REG, mask);
+		if (ret)
+			max30210_autoconv_put(st);
+
+		return ret;
+	}
+
+	ret = regmap_clear_bits(map, MAX30210_INT_EN_REG, mask);
+	if (ret)
+		return ret;
+
+	return max30210_autoconv_put(st);
+}
+
+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);
+	struct regmap *map = st->regmap;
+	unsigned int uval;
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		*val = 5;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SAMP_FREQ: {
+		unsigned int freq_idx;
+
+		ret = regmap_read(map, MAX30210_TEMP_CONF_2_REG, &uval);
+		if (ret)
+			return ret;
+
+		freq_idx = FIELD_GET(MAX30210_TEMPCONF2_TEMP_PERIOD_MASK, uval);
+
+		/*
+		 * TEMP_PERIOD is an index into the sampling frequency lookup
+		 * table. Clamp in case the register holds a reserved value.
+		 */
+		freq_idx = min(freq_idx, ARRAY_SIZE(max30210_samp_freq_avail) - 1);
+
+		*val = max30210_samp_freq_avail[freq_idx][0];
+		*val2 = max30210_samp_freq_avail[freq_idx][1];
+
+		return IIO_VAL_INT_PLUS_MICRO;
+	}
+	case IIO_CHAN_INFO_RAW: {
+		IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
+		if (IIO_DEV_ACQUIRE_FAILED(claim))
+			return -EBUSY;
+
+		guard(mutex)(&st->lock);
+
+		/*
+		 * TEMP_DATA_REG is only refreshed by a single-shot
+		 * conversion; the buffer/FIFO path is the only thing that
+		 * gets updated during AUTO mode. If AUTO is already running
+		 * for the buffer and/or events, triggering a one-off
+		 * conversion here would stomp the AUTO bit out from under
+		 * whoever enabled it, and there is no way to get a fresh
+		 * single-shot value without doing so -- refuse instead of
+		 * returning stale data.
+		 */
+		if (st->autoconv_users > 0)
+			return -EBUSY;
+
+		ret = regmap_write(map, MAX30210_TEMP_CONV_REG,
+				   MAX30210_TEMPCONV_CONV_T);
+		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(map, MAX30210_TEMP_CONV_REG,
+					       uval, !(uval & MAX30210_TEMPCONV_CONV_T),
+					       500, 10 * USEC_PER_MSEC);
+		if (ret)
+			return ret;
+
+		return max30210_read_temp(map, 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: {
+		if (val < 0 || val2 < 0)
+			return -EINVAL;
+
+		IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
+		if (IIO_DEV_ACQUIRE_FAILED(claim))
+			return -EBUSY;
+
+		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])
+				continue;
+
+			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);
+	int ret;
+
+	if (!val)
+		return -EINVAL;
+
+	val = min(val, MAX30210_FIFO_SIZE);
+
+	/*
+	 * FIFO_A_FULL sets the number of empty FIFO slots remaining when
+	 * the A_FULL interrupt fires. For a watermark of N samples, set
+	 * FIFO_A_FULL = FIFO_SIZE - N.
+	 */
+	ret = regmap_write(st->regmap, MAX30210_FIFO_CONF_1_REG,
+			   FIELD_PREP(MAX30210_FIFOCONF1_A_FULL_MASK,
+				      MAX30210_FIFO_SIZE - val));
+	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);
+}
+
+static IIO_DEVICE_ATTR_RO(hwfifo_watermark, 0);
+
+IIO_STATIC_CONST_DEVICE_ATTR(hwfifo_watermark_min, "1");
+IIO_STATIC_CONST_DEVICE_ATTR(hwfifo_watermark_max, __stringify(MAX30210_FIFO_SIZE));
+
+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);
+
+	/* Start from an empty FIFO so the first sample is fresh. */
+	return regmap_set_bits(st->regmap, MAX30210_FIFO_CONF_2_REG,
+			       MAX30210_FIFOCONF2_FLUSH_FIFO);
+}
+
+/* The core calls this after hwfifo_set_watermark(), so the watermark is stable here. */
+static int max30210_buffer_postenable(struct iio_dev *indio_dev)
+{
+	struct max30210_state *st = iio_priv(indio_dev);
+	unsigned int status;
+	int ret;
+
+	guard(mutex)(&st->lock);
+
+	ret = max30210_autoconv_get(st);
+	if (ret)
+		return ret;
+
+	/* Read STATUS to clear a stale A_FULL so it cannot fire on the flushed FIFO. */
+	ret = regmap_read(st->regmap, MAX30210_STATUS_REG, &status);
+	if (!ret)
+		ret = regmap_set_bits(st->regmap, MAX30210_INT_EN_REG,
+				      MAX30210_STATUS_A_FULL_MASK);
+	if (ret)
+		max30210_autoconv_put(st);
+
+	return ret;
+}
+
+static int max30210_buffer_predisable(struct iio_dev *indio_dev)
+{
+	struct max30210_state *st = iio_priv(indio_dev);
+	int ret, err;
+
+	/* Stop the FIFO interrupt and wait for a running handler before the buffer goes away. */
+	ret = regmap_clear_bits(st->regmap, MAX30210_INT_EN_REG,
+				MAX30210_STATUS_A_FULL_MASK);
+	synchronize_irq(st->irq);
+
+	scoped_guard(mutex, &st->lock)
+		err = max30210_autoconv_put(st);
+
+	return ret ?: err;
+}
+
+static int max30210_buffer_postdisable(struct iio_dev *indio_dev)
+{
+	struct max30210_state *st = iio_priv(indio_dev);
+
+	return regmap_set_bits(st->regmap, MAX30210_FIFO_CONF_2_REG,
+			       MAX30210_FIFOCONF2_FLUSH_FIFO);
+}
+
+static const struct iio_buffer_setup_ops max30210_buffer_ops = {
+	.preenable = max30210_buffer_preenable,
+	.postenable = max30210_buffer_postenable,
+	.predisable = max30210_buffer_predisable,
+	.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_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,
+		.endianness = IIO_CPU,
+	},
+};
+
+static void max30210_powerdown(void *data)
+{
+	struct gpio_desc *powerdown_gpio = data;
+
+	gpiod_set_value_cansleep(powerdown_gpio, 1);
+}
+
+static void max30210_quiesce(void *data)
+{
+	struct max30210_state *st = data;
+
+	/* Stop interrupt sources and autonomous conversions; errors are not actionable here */
+	regmap_write(st->regmap, MAX30210_INT_EN_REG, 0);
+	regmap_write(st->regmap, MAX30210_TEMP_CONV_REG, 0);
+}
+
+static int max30210_setup(struct max30210_state *st, struct device *dev)
+{
+	struct gpio_desc *powerdown_gpio;
+	unsigned int val;
+	int ret;
+
+	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) {
+		/* CVT/PDB needs a 5 us low pulse; hold it asserted for twice that. */
+		fsleep(10);
+
+		/* Deassert powerdown to power up device */
+		gpiod_set_value_cansleep(powerdown_gpio, 0);
+
+		ret = devm_add_action_or_reset(dev, max30210_powerdown, powerdown_gpio);
+		if (ret)
+			return ret;
+	} else {
+		/* The supply was just enabled; the device needs tPU before it accepts I2C. */
+		fsleep(700);
+
+		ret = regmap_set_bits(st->regmap, MAX30210_SYS_CONF_REG,
+				      MAX30210_SYSCONF_RESET);
+		if (ret)
+			return ret;
+	}
+
+	/*
+	 * 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;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+
+	ret = devm_mutex_init(dev, &st->lock);
+	if (ret)
+		return ret;
+
+	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;
+	st->irq = client->irq;
+
+	ret = max30210_setup(st, dev);
+	if (ret)
+		return ret;
+
+	/* Without an interrupt no event can be delivered, so do not expose any. */
+	st->channel = max30210_channel;
+	if (!client->irq) {
+		st->channel.event_spec = NULL;
+		st->channel.num_event_specs = 0;
+	}
+
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = &st->channel;
+	indio_dev->num_channels = 1;
+	indio_dev->name = "max30210";
+	indio_dev->info = &max30210_info;
+
+	/* Currently, without an interrupt there is no way to drain the FIFO into the buffer */
+	if (client->irq) {
+		ret = devm_iio_kfifo_buffer_setup_ext(dev, indio_dev, &max30210_buffer_ops,
+						      max30210_fifo_attributes);
+		if (ret)
+			return ret;
+
+		/* Runs after unregister, so nothing can re-arm the device; before powerdown. */
+		ret = devm_add_action_or_reset(dev, max30210_quiesce, st);
+		if (ret)
+			return ret;
+	}
+
+	ret = devm_iio_device_register(dev, indio_dev);
+	if (ret)
+		return ret;
+
+	if (!client->irq)
+		return 0;
+
+	/*
+	 * Request the IRQ after registering the IIO device so devm tears it
+	 * down first on removal, before the event interface is freed.
+	 */
+	return devm_request_threaded_irq(dev, client->irq, NULL,
+					 max30210_irq_handler, IRQF_ONESHOT,
+					 indio_dev->name, indio_dev);
+}
+
+static const struct i2c_device_id max30210_id[] = {
+	{ .name = "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");
-- 
2.34.1

Re: [PATCH v6 2/2] iio: temperature: add support for Analog Devices MAX30210

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] iio: temperature: max30210: IRQ requested after
  devm_iio_device_register()
- [Medium] iio: max30210: status bits discarded in
  max30210_buffer_postenable()
--

commit 421e9f8d7f502a8b5bdb83438a4e54f4019fbac4
Author: John Erasmus Mari Geronimo <johnerasmusmari.geronimo@analog.com>

iio: temperature: add support for Analog Devices MAX30210

This commit adds an IIO driver for the Analog Devices MAX30210 I2C
temperature sensor with support for direct reads, threshold events, and a
hardware FIFO buffer.

> diff --git a/drivers/iio/temperature/max30210.c b/drivers/iio/temperature/max30210.c
> new file mode 100644
> index 0000000000000..8866a476eb37c
> --- /dev/null
> +++ b/drivers/iio/temperature/max30210.c

[ ... ]

> +/* The core calls this after hwfifo_set_watermark(), so the watermark is stable here. */
> +static int max30210_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct max30210_state *st = iio_priv(indio_dev);
> +	unsigned int status;
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	ret = max30210_autoconv_get(st);
> +	if (ret)
> +		return ret;
> +
> +	/* Read STATUS to clear a stale A_FULL so it cannot fire on the flushed FIFO. */
> +	ret = regmap_read(st->regmap, MAX30210_STATUS_REG, &status);
> +	if (!ret)
> +		ret = regmap_set_bits(st->regmap, MAX30210_INT_EN_REG,
> +				      MAX30210_STATUS_A_FULL_MASK);
> +	if (ret)
> +		max30210_autoconv_put(st);
> +
> +	return ret;
> +}

[Severity: Medium]
Can reading and discarding MAX30210_STATUS_REG here drop pending
temperature threshold events?

MAX30210_STATUS_REG is clear-on-read and shared with
MAX30210_STATUS_TEMP_HI_MASK and MAX30210_STATUS_TEMP_LO_MASK.  When
threshold events are already enabled (st->autoconv_users > 0), reading
MAX30210_STATUS_REG without reporting those bits clears any pending alarm
in hardware and causes a concurrent max30210_irq_handler() to read 0 and
return IRQ_NONE.

In addition, when st->autoconv_users == 0, calling max30210_autoconv_get()
before reading MAX30210_STATUS_REG starts autonomous conversions before
clearing the stale status byte.

[ ... ]

> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	if (!client->irq)
> +		return 0;
> +
> +	/*
> +	 * Request the IRQ after registering the IIO device so devm tears it
> +	 * down first on removal, before the event interface is freed.
> +	 */
> +	return devm_request_threaded_irq(dev, client->irq, NULL,
> +					 max30210_irq_handler, IRQF_ONESHOT,
> +					 indio_dev->name, indio_dev);
> +}

[Severity: Medium]
As reported by Jonathan Cameron on v4 [1], does registering the IIO device
before requesting client->irq expose the buffer and event interfaces
before the interrupt handler is installed, and invert the teardown order
on removal?

During probe, userspace can enable the buffer or threshold events (or
disable the buffer and hit synchronize_irq(st->irq) in
max30210_buffer_predisable()) before devm_request_threaded_irq() runs.  On
driver unbind, devres frees client->irq before devm_iio_device_unreg()
runs iio_device_unregister() -> iio_disable_all_buffers() ->
max30210_buffer_predisable(), which then calls synchronize_irq(st->irq) on
the already-freed IRQ, and before max30210_quiesce() disables hardware
interrupts.

The event interface is freed in iio_dev_release() (bound to
devm_iio_device_alloc()) rather than iio_device_unregister(), so
requesting the IRQ and registering max30210_quiesce() before
devm_iio_device_register() still tears down the IRQ before the event
interface is freed.

[1] https://lore.kernel.org/r/20261017230705.678a791c@jic23-hlaptop

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110021442.410-1-johnerasmusmari.geronimo@analog.com?part=2

Re: [PATCH v6 2/2] iio: temperature: add support for Analog Devices MAX30210

[Andy Shevchenko]

On Tue, Nov 10, 2026 at 10:14:42AM +0800, 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.

...

> +	/* Drop the reference even if the write fails; a later direct read clears AUTO anyway. */

It's a bit too long. Make it multi-line.

...

> +static void max30210_fifo_read(struct iio_dev *indio_dev)
> +{
> +	struct max30210_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	/*
> +	 * FIFO_DATA_REG auto-increments in hardware; bulk_read would treat
> +	 * each subsequent byte as a separate cacheable register.
> +	 */
> +	ret = regmap_noinc_read(st->regmap, MAX30210_FIFO_DATA_REG,
> +				st->fifo_buf,
> +				MAX30210_FIFO_BYTES_PER_SAMPLE * 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[MAX30210_FIFO_BYTES_PER_SAMPLE * i]);

		unsigned int idx = MAX30210_FIFO_BYTES_PER_SAMPLE * i;
		u32 raw = get_unaligned_be24(&st->fifo_buf[idx]);

> +		s16 temp;
> +
> +		if (raw == MAX30210_FIFO_INVAL_DATA) {
> +			dev_err_ratelimited(&indio_dev->dev, "Invalid data\n");
> +			continue;
> +		}
> +
> +		temp = sign_extend32(FIELD_GET(MAX30210_FIFO_TEMP_MASK, raw), 15);
> +
> +		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, enabled;
> +	int ret;
> +
> +	ret = regmap_read(st->regmap, MAX30210_STATUS_REG, &status);
> +	if (ret) {
> +		dev_err(&indio_dev->dev, "Status read failed\n");

It's always recommended to have the messages to be ratelimited in IRQ handler,
that's in case of intermittent problems with reading this may flood the kernel
log.

> +		return IRQ_NONE;
> +	}
> +
> +	/* STATUS flags are set whether or not they are enabled in INT_EN. */
> +	ret = regmap_read(st->regmap, MAX30210_INT_EN_REG, &enabled);
> +	if (ret) {
> +		dev_err(&indio_dev->dev, "Interrupt enable read failed\n");
> +		return IRQ_NONE;
> +	}
> +
> +	status &= enabled;
> +	if (!(status & (MAX30210_STATUS_A_FULL_MASK |
> +			MAX30210_STATUS_TEMP_HI_MASK |
> +			MAX30210_STATUS_TEMP_LO_MASK)))
> +		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_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: {

		struct regmap *map = st->regmap;

> +		if (val < 0 || val2 < 0)
> +			return -EINVAL;
> +
> +		IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
> +		if (IIO_DEV_ACQUIRE_FAILED(claim))
> +			return -EBUSY;
> +
> +		for (unsigned int i = 0; i < ARRAY_SIZE(max30210_samp_freq_avail); i++) {

			unsigned int mask = MAX30210_TEMPCONF2_TEMP_PERIOD_MASK;
			unsigned int value = field_prep(mask, i);

> +			if (val != max30210_samp_freq_avail[i][0] ||
> +			    val2 != max30210_samp_freq_avail[i][1])
> +				continue;
> +
> +			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 regmap_update_bits(map, MAX30210_TEMP_CONF_2_REG,
						  mask, value);

> +		}
> +
> +		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);
> +	int ret;
> +
> +	if (!val)
> +		return -EINVAL;
> +
> +	val = min(val, MAX30210_FIFO_SIZE);

I would put check here to make it look more natural.

	val = min(val, MAX30210_FIFO_SIZE);
	if (!val)
		return -EINVAL;

> +	/*
> +	 * FIFO_A_FULL sets the number of empty FIFO slots remaining when
> +	 * the A_FULL interrupt fires. For a watermark of N samples, set
> +	 * FIFO_A_FULL = FIFO_SIZE - N.
> +	 */
> +	ret = regmap_write(st->regmap, MAX30210_FIFO_CONF_1_REG,
> +			   FIELD_PREP(MAX30210_FIFOCONF1_A_FULL_MASK,
> +				      MAX30210_FIFO_SIZE - val));
> +	if (ret)
> +		return ret;
> +
> +	st->watermark = val;
> +
> +	return 0;
> +}

...

> +static int max30210_setup(struct max30210_state *st, struct device *dev)
> +{
> +	struct gpio_desc *powerdown_gpio;
> +	unsigned int val;
> +	int ret;
> +
> +	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) {
> +		/* CVT/PDB needs a 5 us low pulse; hold it asserted for twice that. */

If I am not mistaken somewhere else you used Greek MU, why not here? Please,
check the comments for the consistency.

> +		fsleep(10);
> +
> +		/* Deassert powerdown to power up device */
> +		gpiod_set_value_cansleep(powerdown_gpio, 0);
> +
> +		ret = devm_add_action_or_reset(dev, max30210_powerdown, powerdown_gpio);
> +		if (ret)
> +			return ret;
> +	} else {
> +		/* The supply was just enabled; the device needs tPU before it accepts I2C. */
> +		fsleep(700);
> +
> +		ret = regmap_set_bits(st->regmap, MAX30210_SYS_CONF_REG,
> +				      MAX30210_SYSCONF_RESET);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/*
> +	 * 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);
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 2/2] iio: temperature: add support for Analog Devices MAX30210

[Andy Shevchenko]

On Tue, Nov 10, 2026 at 10:53:23PM +0200, Andy Shevchenko wrote:
> On Tue, Nov 10, 2026 at 10:14:42AM +0800, John Erasmus Mari Geronimo wrote:

...

> > +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: {
> 
> 		struct regmap *map = st->regmap;
> 
> > +		if (val < 0 || val2 < 0)
> > +			return -EINVAL;
> > +
> > +		IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
> > +		if (IIO_DEV_ACQUIRE_FAILED(claim))
> > +			return -EBUSY;
> > +
> > +		for (unsigned int i = 0; i < ARRAY_SIZE(max30210_samp_freq_avail); i++) {
> 
> 			unsigned int mask = MAX30210_TEMPCONF2_TEMP_PERIOD_MASK;
> 			unsigned int value = field_prep(mask, i);

Ah, there is a conflict with above 'mask'. Something like 'm' and 'v'
would fit?

> > +			if (val != max30210_samp_freq_avail[i][0] ||
> > +			    val2 != max30210_samp_freq_avail[i][1])
> > +				continue;
> > +
> > +			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 regmap_update_bits(map, MAX30210_TEMP_CONF_2_REG,
> 						  mask, value);
> 
> > +		}
> > +
> > +		return -EINVAL;
> > +	}
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}

-- 
With Best Regards,
Andy Shevchenko