Skip to contentSkip to conversation

iio: health: add MAX86150 ECG and PPG biosensor driver

PATCH In review Md Shofiqul Islam 4 participants 6 messages Sep 13 – Sep 24

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

v11 cover iio: health: add MAX86150 ECG and PPG biosensor driver

Add IIO support for the Analog Devices MAX86150 combined ECG and PPG
biosensor: a devicetree binding and a driver that registers a kfifo buffer directly off the device's hardware FIFO (matching max30102.c in this directory), draining it from a threaded IRQ handler.

Tested against i2c-stub with the expected PART_ID and FIFO bytes pre-seeded, covering probe (both a matching and a mismatched part ID), the chip_init() register programming, raw-channel decode of a simulated FIFO burst, and buffer enable/disable. Not yet tested against real MAX86150 hardware.

A kernel test robot build error reported against an intermediate, unsent tree state (incompatible .validate_trigger pointer type on iio_trigger_validate_own_device()) does not apply here: the trigger framework that error was in is gone in this version, replaced by the kfifo buffer described below.

Full history of this series (v1-v10): https://lore.kernel.org/linux-iio/?q=MAX86150

Changes since v10 (2026-07-17):
- Redesigned onto a kfifo buffer instead of the hardware-trigger + triggered-buffer framework, matching max30102.c in this directory (Jonathan Cameron -- this was the big one: a trigger + triggered buffer doesn't fit a device that just has one hardware FIFO and no concept of a triggered "scan").
- get_unaligned_be24() for the three 24-bit FIFO fields instead of manual shift/or (Andy Shevchenko).
- regmap cache added (volatile_reg() + REGCACHE_RBTREE); FIFO_DATA, FIFO_WR_PTR, FIFO_RD_PTR, OVF_COUNTER and INT_STATUS1/2 stay volatile (Andy Shevchenko).
- devm_regulator_bulk_get_enable() for the four supplies instead of four separate calls (Jonathan Cameron).
- sample_period_ns expressed as NSEC_PER_SEC / 100 instead of a bare 10000000 (Jonathan Cameron).
- Added a comment explaining why a part-ID mismatch is fatal here instead of a warn-and-continue (Jonathan Cameron).
- _nA / _Hz suffixes on the ADC-range and sample-rate defines; moved the LED pulse amplitude comment next to the define it documents; dropped two comments that just repeated what the diff/code already showed (Andy Shevchenko).
- FIELD_PREP_CONST() in place of FIELD_PREP() everywhere the operands are compile-time constants (Andy Shevchenko).
- USEC_PER_MSEC-based constants in the PPG_RDY poll timeout instead of raw 1000/25000 (Andy Shevchenko).
- max86150_do_read_raw() split into two functions (the shutdown-wrap outer function and a max86150_read_raw_locked() inner one) instead of a label + goto (Andy Shevchenko).
- All three switch cases in read_raw() now return IIO_VAL_INT; directly instead of break; + one shared return (Andy Shevchenko).
- C99 initialiser for the i2c_device_id table entry (Andy Shevchenko).
- devm_regmap_init_i2c() now runs before the regulators are enabled, so the pure-software setup happens before any HW interaction (Andy Shevchenko).
- Kconfig now selects IIO_KFIFO_BUF instead of the now-unused IIO_TRIGGERED_BUFFER, matching the buffer redesign above.
- Shortened this commit message -- the implementation-detail bullet list duplicated what's now in this cover letter and in the code comments themselves (Andy Shevchenko).

Changes since v9 (2026-07-07):
- Rewrote the buffer implementation around devm_iio_trigger_alloc() + devm_iio_triggered_buffer_setup() instead of the kfifo buffer and manual postenable/predisable pair. The hard-irq handler only read and cleared INT_STATUS1 before calling iio_trigger_poll(); the threaded trigger handler did the FIFO drain. This relied on the trigger core's own attach/detach synchronization instead of an explicit iio_buffer_enabled() guard or synchronize_irq(), which the old design needed to avoid a NULL active_scan_mask race on teardown (Sashiko, both HIGH severity findings).
[NOTE: v11 above reverts this back onto a kfifo buffer per Jonathan Cameron's v10 review -- a triggered buffer was the wrong tool here.]

Earlier versions (v1-v8) are in the full history link above. The notable structural changes along the way:
- v1->v2: split the original single combined patch into three (dt- bindings, driver, MAINTAINERS entry).
- v4->v5: renamed the devicetree compatible string from "maxim,max86150" to "adi,max86150", matching the vendor prefix Analog Devices actually uses upstream (Maxim was acquired by ADI).
- v8->v9: folded the standalone MAINTAINERS patch back into the driver patch, dropping the series from three patches to two.

Md Shofiqul Islam (2):
dt-bindings: iio: health: add adi,max86150
iio: health: add MAX86150 ECG and PPG biosensor driver

 .../bindings/iio/health/adi,max86150.yaml     |  77 ++
 MAINTAINERS                                   |   7 +
 drivers/iio/health/Kconfig                    |  13 +
 drivers/iio/health/Makefile                   |   1 +
 drivers/iio/health/max86150.c                 | 658 ++++++++++++++++++
 5 files changed, 756 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/health/adi,max86150.yaml
 create mode 100644 drivers/iio/health/max86150.c

--
2.55.0

#1 lore raw

PATCH v11 1/2 dt-bindings: iio: health: add adi,max86150

Add the Device Tree binding schema for the Analog Devices MAX86150 integrated biosensor, which combines two photoplethysmography (PPG) channels (Red LED and IR LED) and one electrocardiogram (ECG) channel in a single I2C device.

The device requires four independent supply rails per the datasheet power tree: digital core (vdd), analog core (avdd), ECG reference (vref), and LED anode (leds).

Assisted-by Claude:claude-sonnet-5
---
 .../bindings/iio/health/adi,max86150.yaml     | 77 +++++++++++++++++++
 MAINTAINERS                                   |  6 ++
 2 files changed, 83 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/health/adi,max86150.yaml
Documentation/devicetree/bindings/iio/health/adi,max86150.yaml new file +77 -0
new file mode 100644
index 000000000000..8392d08edd1b
@@ -0,0 +1,77 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/health/adi,max86150.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices MAX86150 ECG and PPG Biosensor
+
+maintainers:
+  - Md Shofiqul Islam <[email protected]>
+
+description: |
+  The MAX86150 is an integrated biosensor SoC that combines:
+    - Two PPG (photoplethysmography) channels: Red LED and IR LED,
+      for heart rate and blood-oxygen saturation (SpO2) measurement.
+    - One ECG (electrocardiogram) channel for biopotential recording.
+
+  The device communicates over I2C at up to 400 kHz.
+
+  Datasheet:
+    https://www.analog.com/media/en/technical-documentation/data-sheets/MAX86150.pdf
+
+properties:
+  compatible:
+    const: adi,max86150
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+    description:
+      Active-low interrupt line, asserted when the 32-entry hardware
+      FIFO reaches its configurable almost-full threshold, or when a
+      new PPG sample is ready.
+
+  vdd-supply:
+    description: Digital core power supply (1.8 V).
+
+  avdd-supply:
+    description: Analog core power supply (1.8 V).
+
+  vref-supply:
+    description: ECG reference voltage supply.
+
+  leds-supply:
+    description: LED anode supply, typically 3.3 V.
+
+required:
+  - compatible
+  - reg
+  - vdd-supply
+  - avdd-supply
+  - vref-supply
+  - leds-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        heart-rate@5e {
+            compatible = "adi,max86150";
+            reg = <0x5e>;
+            interrupt-parent = <&gpio1>;
+            interrupts = <5 IRQ_TYPE_LEVEL_LOW>;
+            vdd-supply = <&vdd_1v8>;
+            avdd-supply = <&vdd_1v8>;
+            vref-supply = <&vdd_1v8>;
+            leds-supply = <&vdd_3v3>;
+        };
+    };
MAINTAINERS +6 -0
index 3115538ce829..189385966de8 100644
@@ -15705,6 +15705,12 @@ S:	Orphan
 F:	Documentation/hwmon/max6650.rst
 F:	drivers/hwmon/max6650.c
 
+MAX86150 ECG AND PPG BIOSENSOR DRIVER
+M:	Md Shofiqul Islam <[email protected]>
+L:	[email protected]
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/health/adi,max86150.yaml
+
 MAX9286 QUAD GMSL DESERIALIZER DRIVER
 M:	Jacopo Mondi <[email protected]>
 M:	Kieran Bingham <[email protected]>
Signature · 1 line
2.55.0
maintainer Reviewed-by on v11 1/2
#2 lore raw

Re: [PATCH v11 1/2] dt-bindings: iio: health: add adi,max86150

Quoted 9 lines · Md Shofiqul Islam

Add the Device Tree binding schema for the Analog Devices MAX86150 integrated biosensor, which combines two photoplethysmography (PPG) channels (Red LED and IR LED) and one electrocardiogram (ECG) channel in a single I2C device.

The device requires four independent supply rails per the datasheet power tree: digital core (vdd), analog core (avdd), ECG reference (vref), and LED anode (leds).

Quoted 8 lines
Assisted-by Claude:claude-sonnet-5
---
 .../bindings/iio/health/adi,max86150.yaml     | 77 +++++++++++++++++++
 MAINTAINERS                                   |  6 ++
 2 files changed, 83 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/health/adi,max86150.yaml
#3 lore raw

PATCH v11 2/2 iio: health: add MAX86150 ECG and PPG biosensor driver

In reply to Md Shofiqul Islam

Add a new IIO driver for the Analog Devices MAX86150 integrated biosensor, which combines two PPG optical channels (Red/IR LED) and one ECG biopotential channel behind a 32-entry hardware FIFO in a single I2C device.

The driver registers a kfifo buffer directly, matching max30102.c in this directory, rather than using the trigger framework: enabling the buffer powers the chip up and arms the FIFO almost-full interrupt, and a single threaded IRQ handler drains the FIFO and pushes samples straight to the buffer. A part-ID check on probe rejects mismatched hardware before chip_init() writes any configuration blind.

Assisted-by Claude:claude-sonnet-5
---
 MAINTAINERS                   |   1 +
 drivers/iio/health/Kconfig    |  13 +
 drivers/iio/health/Makefile   |   1 +
 drivers/iio/health/max86150.c | 658 ++++++++++++++++++++++++++++++++++
 4 files changed, 673 insertions(+)
 create mode 100644 drivers/iio/health/max86150.c
MAINTAINERS +1 -0
index 189385966de8..5e266c03bda6 100644
@@ -15710,6 +15710,7 @@ M:	Md Shofiqul Islam <[email protected]>
 L:	[email protected]
 S:	Maintained
 F:	Documentation/devicetree/bindings/iio/health/adi,max86150.yaml
+F:	drivers/iio/health/max86150.c
 
 MAX9286 QUAD GMSL DESERIALIZER DRIVER
 M:	Jacopo Mondi <[email protected]>
drivers/iio/health/Kconfig +13 -0
index a89f3abf11f4..3b03744d9a5f 100644
@@ -62,4 +62,17 @@ config MAX30102
 
 endmenu
 
+config MAX86150
+	tristate "MAX86150 ECG and PPG biosensor"
+	depends on I2C
+	select IIO_BUFFER
+	select IIO_KFIFO_BUF
+	select REGMAP_I2C
+	help
+	  Say Y here to enable support for the Maxim MAX86150 combined
+	  ECG and photoplethysmography (PPG) biosensor.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called max86150.
+
 endmenu
drivers/iio/health/Makefile +1 -0
index 910817112258..04fc73c58444 100644
@@ -9,3 +9,4 @@ obj-$(CONFIG_AFE4403)		+= afe4403.o
 obj-$(CONFIG_AFE4404)		+= afe4404.o
 obj-$(CONFIG_MAX30100)		+= max30100.o
 obj-$(CONFIG_MAX30102)		+= max30102.o
+obj-$(CONFIG_MAX86150)		+= max86150.o
drivers/iio/health/max86150.c new file +658 -0

658 lines not on this page · raw message

Signature · 1 line
2.55.0
#4 lore raw
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] Silent buffer failure and userspace hang when probed without a valid IRQ. - [High] The driver…

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Silent buffer failure and userspace hang when probed without a valid IRQ.
- [High] The driver disables the hardware interrupt but fails to synchronize with the threaded IRQ handler, leading to a NULL pointer dereference and data races with sysfs reads.
- [Medium] The driver performs I2C reads immediately after enabling regulators, violating power-up timing requirements.
- [Medium] The timestamp calculation for the non-overflow case uses an incorrect formula that generates future timestamps when the IRQ handler is delayed.
- [Low] The driver uses nonexistent macros IIO_DEV_ACQUIRE_DIRECT_MODE and IIO_DEV_ACQUIRE_FAILED to claim direct mode, which will cause a build failure. Additionally, the acquired lock is never released. --

commit 2dc175e42137c8414df10b568a4d8af128ab280d
Author: Md Shofiqul Islam <[email protected]>
Subject: iio: health: add MAX86150 ECG and PPG biosensor driver

This commit adds a new IIO driver for the MAX86150 combined ECG and PPG biosensor. It registers a direct kfifo buffer and uses a threaded IRQ handler to drain the hardware FIFO for sensor monitoring.

Quoted 4 lines
drivers/iio/health/max86150.c new file +0 -0
new file mode 100644

[ ... ]

Quoted 5 lines
drivers/iio/health/max86150.c new file +5 -0
+static int max86150_read_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int *val, int *val2, long mask)
+{
+	struct max86150_data *data = iio_priv(indio_dev);

drivers/iio/health/max86150.c line 306

Quoted 10 lines
drivers/iio/health/max86150.c new file +10 -0
+	u32 ppg_red, ppg_ir;
+	s32 ecg;
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW: {
+		IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
+
+		if (IIO_DEV_ACQUIRE_FAILED(claim))
+			return -EBUSY;

[Severity: Low]
Are these macros defined in the IIO headers? IIO_DEV_ACQUIRE_DIRECT_MODE and IIO_DEV_ACQUIRE_FAILED appear to be missing from the IIO core headers, which will result in a compile failure.

Quoted 11 lines
drivers/iio/health/max86150.c new file +11 -0
+
+		ret = max86150_do_read_raw(data, &ppg_red, &ppg_ir, &ecg);
+		if (ret)
+			return ret;
+
+		switch (chan->scan_index) {
+		case MAX86150_IDX_PPG_RED:
+			*val = ppg_red;
+			return IIO_VAL_INT;
+		case MAX86150_IDX_PPG_IR:
+			*val = ppg_ir;

drivers/iio/health/max86150.c line 327

Quoted 10 lines
drivers/iio/health/max86150.c new file +10 -0
+			return IIO_VAL_INT;
+		case MAX86150_IDX_ECG:
+			*val = ecg;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	}
+	default:
+		return -EINVAL;

[Severity: Low]
If the direct mode locking macros were valid, would this code leave the acquired lock unreleased on these return paths?

Quoted 2 lines
drivers/iio/health/max86150.c new file +2 -0
+	}
+}

[ ... ]

drivers/iio/health/max86150.c line 340

Quoted 6 lines
drivers/iio/health/max86150.c new file +6 -0
+static int max86150_buffer_predisable(struct iio_dev *indio_dev)
+{
+	struct max86150_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = regmap_write(data->regmap, MAX86150_REG_INT_ENABLE1, 0);

[Severity: High]
Does this predisable callback properly synchronize with the threaded IRQ handler? It masks hardware interrupts but does not wait for the Linux IRQ thread to finish, such as by calling disable_irq().

If the user disables the IIO buffer via sysfs, the IIO core could free and clear active_scan_mask while the concurrent IRQ thread is still running and dereferencing it in max86150_irq_handler().

This lack of synchronization could also lead to data races on the shared data->fifo_raw buffer if a concurrent read_raw sysfs access overlaps with the residual IRQ handler.

Quoted 6 lines
drivers/iio/health/max86150.c new file +6 -0
+	if (ret)
+		return ret;
+
+	return regmap_set_bits(data->regmap, MAX86150_REG_SYS_CTRL,
+			       MAX86150_SYS_SHDN);
+}

[ ... ]

drivers/iio/health/max86150.c line 455

Quoted 11 lines
drivers/iio/health/max86150.c new file +11 -0
+	for (unsigned int i = 0; i < n_avail; i++) {
+		unsigned int j;
+		s64 ts;
+
+		if (ovf > 0)
+			ts = irq_time -
+			     (s64)(n_avail - 1 - i) * data->sample_period_ns;
+		else
+			ts = irq_time +
+			     ((s64)i - (MAX86150_FIFO_A_FULL_SAMPLES - 1)) *
+			     data->sample_period_ns;

[Severity: Medium]
Will this timestamp calculation generate future timestamps?

When there is no overflow, but the threaded IRQ handler execution is delayed by scheduling, more than 17 samples can accumulate in the FIFO. If n_avail is greater than MAX86150_FIFO_A_FULL_SAMPLES, this formula projects the timestamps of the newest samples into the future relative to the delayed irq_time.

Quoted 4 lines
drivers/iio/health/max86150.c new file +4 -0
+
+		ret = max86150_read_one_sample(data, &ppg_red, &ppg_ir, &ecg);
+		if (ret)
+			break;

[ ... ]

Quoted 7 lines
drivers/iio/health/max86150.c new file +7 -0
+static int max86150_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct iio_dev *indio_dev;
+	struct max86150_data *data;
+	unsigned int part_id;
+	int ret;

[ ... ]

drivers/iio/health/max86150.c line 587

Quoted 6 lines
drivers/iio/health/max86150.c new file +6 -0
+	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(max86150_supply_names),
+					     max86150_supply_names);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable supplies\n");
+
+	ret = regmap_read(data->regmap, MAX86150_REG_PART_ID, &part_id);

[Severity: Medium]
Is a delay required before communicating with the device?

The driver performs an I2C read to MAX86150_REG_PART_ID immediately after enabling regulators via devm_regulator_bulk_get_enable(). Without an intervening power-up delay, the first I2C transaction might fail against an uninitialized chip.

Quoted 2 lines
drivers/iio/health/max86150.c new file +2 -0
+	if (ret)
+		return dev_err_probe(dev, ret, "Cannot read part ID\n");

[ ... ]

drivers/iio/health/max86150.c line 624

Quoted 8 lines
drivers/iio/health/max86150.c new file +8 -0
+	ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
+					  &max86150_buffer_setup_ops);
+	if (ret)
+		return ret;
+
+	if (client->irq > 0) {
+		ret = devm_request_threaded_irq(dev, client->irq, NULL,
+						max86150_irq_handler,

[Severity: High]
What happens to the IIO buffer if the device is probed without a valid IRQ?

The buffer setup operations will configure the hardware to generate interrupts, but if this IRQ registration is skipped, no handler will ever drain the FIFO. This could cause a userspace process blocking on a device read to hang indefinitely.

Quoted 8 lines
drivers/iio/health/max86150.c new file +8 -0
+						IRQF_ONESHOT,
+						"max86150", indio_dev);
+		if (ret)
+			return ret;
+	}
+
+	return devm_iio_device_register(dev, indio_dev);
+}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
maintainer
#5 lore raw
In reply to Md Shofiqul Islam
Quoted 12 lines

Add a new IIO driver for the Analog Devices MAX86150 integrated biosensor, which combines two PPG optical channels (Red/IR LED) and one ECG biopotential channel behind a 32-entry hardware FIFO in a single I2C device.

The driver registers a kfifo buffer directly, matching max30102.c in this directory, rather than using the trigger framework: enabling the buffer powers the chip up and arms the FIFO almost-full interrupt, and a single threaded IRQ handler drains the FIFO and pushes samples straight to the buffer. A part-ID check on probe rejects mismatched hardware before chip_init() writes any configuration blind.

Quoted 2 lines

These are only appropriate markings in a patch that is doing just what was suggested, don't apply them to a whole driver.

Quoted 2 lines
Assisted-by Claude:claude-sonnet-5

End of day with me, so this isn't the most thorough of reviews, but a few things inline to sort out.

Thanks,

Jonathan

Quoted 14 lines
drivers/iio/health/max86150.c new file +5 -0
new file mode 100644
index 000000000000..9829edaed32b
@@ -0,0 +1,658 @@
 
 
 
+/**
+ * struct max86150_data - driver private state
+ * @regmap:           register map for this device
+ * @sample_period_ns: sample period in nanoseconds (set from configured rate)
+ * @fifo_raw:         scratch buffer for regmap_noinc_read() FIFO bursts; kept

drivers/iio/health/max86150.c line 143

Quoted 10 lines
drivers/iio/health/max86150.c new file +10 -0
+ *                    in struct (heap) rather than on the stack, since stack
+ *                    memory isn't guaranteed DMA-safe (e.g. CONFIG_VMAP_STACK)
+ *                    and some I2C host controllers DMA the read buffer
+ * @scan:             IIO push buffer; channels[] packed per active_scan_mask
+ */
+struct max86150_data {
+	struct regmap		*regmap;
+	u32			 sample_period_ns;
+	u8			 fifo_raw[MAX86150_SAMPLE_BYTES];
+	IIO_DECLARE_DMA_BUFFER_WITH_TS(s32, scan, MAX86150_NUM_SLOTS);

Why does this sneed to be dma safe?

Quoted 21 lines
drivers/iio/health/max86150.c new file +17 -0
+};
 
 
+static bool max86150_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case MAX86150_REG_INT_STATUS1:
+	case MAX86150_REG_INT_STATUS2:
+	case MAX86150_REG_FIFO_WR_PTR:
+	case MAX86150_REG_OVF_COUNTER:
+	case MAX86150_REG_FIFO_RD_PTR:
+	case MAX86150_REG_FIFO_DATA:
+		return true;
+	default:
+		return false;
+	}
 
 
+static int max86150_read_one_sample(struct max86150_data *data,
+				    u32 *ppg_red, u32 *ppg_ir, s32 *ecg)
+{

drivers/iio/health/max86150.c line 223

Quoted 10 lines
drivers/iio/health/max86150.c new file +10 -0
+	int ret;
+
+	ret = regmap_noinc_read(data->regmap, MAX86150_REG_FIFO_DATA,
+				data->fifo_raw, MAX86150_SAMPLE_BYTES);
+	if (ret)
+		return ret;
+
+	*ppg_red = get_unaligned_be24(&data->fifo_raw[0]) & GENMASK(18, 0);
+	*ppg_ir  = get_unaligned_be24(&data->fifo_raw[3]) & GENMASK(18, 0);
+	*ecg = sign_extend32(get_unaligned_be24(&data->fifo_raw[6]) & GENMASK(17, 0), 17);

It helps readability a little to have a blank line before simple sucess returns like this one.

drivers/iio/health/max86150.c line 228

Quoted 5 lines
drivers/iio/health/max86150.c new file +5 -0
+	return 0;
+}
+
+/* Does the actual work for max86150_do_read_raw(); see that function for the shutdown wrapping. */
+static int max86150_read_raw_locked(struct max86150_data *data,

What is locked? Direct mode is claimed and under the hood that includes a lock but that is an implementation detail.
I was expecting to see a local lock in the caller but there isn't one.

drivers/iio/health/max86150.c

Quoted 2 lines
drivers/iio/health/max86150.c new file +2 -0
+				    u32 *ppg_red, u32 *ppg_ir, s32 *ecg)
+{

There are 3 layers of wrapping going on here. That seems excesive.

Quoted 40 lines
drivers/iio/health/max86150.c new file +40 -0
+	unsigned int ppg_rdy_status;
+	int ret;
+
+	ret = regmap_clear_bits(data->regmap, MAX86150_REG_SYS_CTRL,
+				MAX86150_SYS_SHDN);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(data->regmap, MAX86150_REG_FIFO_WR_PTR, 0);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(data->regmap, MAX86150_REG_OVF_COUNTER, 0);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(data->regmap, MAX86150_REG_FIFO_RD_PTR, 0);
+	if (ret)
+		return ret;
+
+	/*
+	 * Clear stale PPG_RDY from a previous session; reading
+	 * INT_STATUS1 de-asserts any pending flags so the poll
+	 * below waits for a genuinely new sample.
+	 */
+	ret = regmap_read(data->regmap, MAX86150_REG_INT_STATUS1,
+			  &ppg_rdy_status);
+	if (ret)
+		return ret;
+
+	/*
+	 * Poll PPG_RDY rather than sleeping a fixed interval — the
+	 * internal oscillator may start slower than nominal.  25 ms
+	 * covers more than two 100 Hz sample periods.
+	 */
+	ret = regmap_read_poll_timeout(data->regmap,
+				       MAX86150_REG_INT_STATUS1,
+				       ppg_rdy_status,
+				       ppg_rdy_status & MAX86150_INT_PPG_RDY,
+				       USEC_PER_MSEC, 25 * USEC_PER_MSEC);

drivers/iio/health/max86150.c line 281

Quoted 10 lines
drivers/iio/health/max86150.c new file +10 -0
+	if (ret)
+		return ret;
+
+	return max86150_read_one_sample(data, ppg_red, ppg_ir, ecg);
+}
+ *
+ * Take the device out of shutdown, reset the FIFO pointers, wait for the
+ * first PPG sample, and read it back.  Always returns the device to
+ * shutdown before returning, whether or not the read succeeded.
+ */

This comment isn't needed as the code is fairly obvious.

drivers/iio/health/max86150.c line 289

Quoted 8 lines

+ static int max86150_do_read_raw(struct max86150_data *data,

drivers/iio/health/max86150.c new file +7 -0
+				u32 *ppg_red, u32 *ppg_ir, s32 *ecg)
+{
+	int ret;
+
+	ret = max86150_read_raw_locked(data, ppg_red, ppg_ir, ecg);
+	regmap_set_bits(data->regmap, MAX86150_REG_SYS_CTRL, MAX86150_SYS_SHDN);
+	return ret;

We've ended up with too many layers of wrappers. Just have this code in the max86150_read_raw() instead of having this one.

Quoted 6 lines
drivers/iio/health/max86150.c new file +4 -0
+}
+
 
 
+
+static int max86150_buffer_predisable(struct iio_dev *indio_dev)

drivers/iio/health/max86150.c line 345

Quoted 10 lines
drivers/iio/health/max86150.c new file +10 -0
+{
+	struct max86150_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = regmap_write(data->regmap, MAX86150_REG_INT_ENABLE1, 0);
+	if (ret)
+		return ret;
+
+	return regmap_set_bits(data->regmap, MAX86150_REG_SYS_CTRL,
+			       MAX86150_SYS_SHDN);

Sashiko calls out that there maybe a race between an ongoing threaded handler and this. Given the threaded hander requires use of active_scan_masks that is a race that should be closed. The suggestion to disable irqs is a bad one. Instead we may need to synchronize irqs after this code has ensured we should see no new ones.
That should close the race I think

drivers/iio/health/max86150.c line 402

Quoted 11 lines
drivers/iio/health/max86150.c new file +8 -0
+}
 
 
 
+/*
+ * Threaded IRQ handler (primary=NULL): clears INT_STATUS1 to de-assert the
+ * line, then drains every sample currently in the FIFO and pushes each one
+ * straight to the buffer.  No trigger indirection -- enabling/disabling the
+ * buffer is what arms/disarms the interrupt, via
+ * max86150_buffer_postenable()/_predisable() above.  Matches the direct
+ * kfifo pattern max30102.c uses in this same directory.

What does this comment print us? Very little that I can see that we can't see from the code. It is all standard stuff for a fifo equiped part.

Quoted 52 lines

+ */

drivers/iio/health/max86150.c new file +51 -0
+static irqreturn_t max86150_irq_handler(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct max86150_data *data;
+	s64 irq_time;
+	unsigned int status, wr_ptr, rd_ptr, ovf, n_avail;
+	u32 ppg_red, ppg_ir;
+	s32 ecg;
+	int ret;
+
+	data = iio_priv(indio_dev);
+	irq_time = iio_get_time_ns(indio_dev);
+
+	ret = regmap_read(data->regmap, MAX86150_REG_INT_STATUS1, &status);
+	if (ret || !(status & MAX86150_INT_A_FULL))
+		return IRQ_NONE;
+
+	ret = regmap_read(data->regmap, MAX86150_REG_FIFO_WR_PTR, &wr_ptr);
+	if (ret)
+		return IRQ_HANDLED;
+	ret = regmap_read(data->regmap, MAX86150_REG_FIFO_RD_PTR, &rd_ptr);
+	if (ret)
+		return IRQ_HANDLED;
+	ret = regmap_read(data->regmap, MAX86150_REG_OVF_COUNTER, &ovf);
+	if (ret)
+		return IRQ_HANDLED;
+
+	if (ovf > 0) {
+		n_avail = MAX86150_FIFO_DEPTH;
+	} else {
+		n_avail = (wr_ptr - rd_ptr) & (MAX86150_FIFO_DEPTH - 1);
+		/*
+		 * wr_ptr == rd_ptr with no overflow means either empty or
+		 * exactly 32 slots filled (pointer wrapped).  Since this
+		 * handler only runs when A_FULL fired, the FIFO must be
+		 * full — treat as 32 available.
+		 */
+		if (n_avail == 0)
+			n_avail = MAX86150_FIFO_DEPTH;
+	}
+
+	for (unsigned int i = 0; i < n_avail; i++) {
+		unsigned int j;
+		s64 ts;
+
+		if (ovf > 0)
+			ts = irq_time -
+			     (s64)(n_avail - 1 - i) * data->sample_period_ns;
+		else
+			ts = irq_time +
+			     ((s64)i - (MAX86150_FIFO_A_FULL_SAMPLES - 1)) *

drivers/iio/health/max86150.c line 464

Quoted 10 lines
drivers/iio/health/max86150.c new file +10 -0
+			     data->sample_period_ns;
+
+		ret = max86150_read_one_sample(data, &ppg_red, &ppg_ir, &ecg);
+		if (ret)
+			break;
+
+		memset(data->scan, 0, sizeof(data->scan));
+		j = 0;
+
+		if (test_bit(MAX86150_IDX_PPG_RED, indio_dev->active_scan_mask))

These accesses to the active_scan_index are the thing that sashiko called out as problematic if the handling is still running as we exit buffered mode.

I'm also not sure they are useful. If you have to read all the channels back from the device anyway, the core support for demuxing data if you set available_scan_mask is probably a better way to handle this.

drivers/iio/health/max86150.c line 471

Quoted 7 lines
drivers/iio/health/max86150.c new file +7 -0
+			data->scan[j++] = ppg_red;
+		if (test_bit(MAX86150_IDX_PPG_IR, indio_dev->active_scan_mask))
+			data->scan[j++] = ppg_ir;
+		if (test_bit(MAX86150_IDX_ECG, indio_dev->active_scan_mask))
+			data->scan[j++] = ecg;
+
+		iio_push_to_buffers_with_timestamp(indio_dev, data->scan, ts);

Use the newer _with_ts() variant. This one is going away once we've finished converting drivers over.

drivers/iio/health/max86150.c line 480

Quoted 9 lines
drivers/iio/health/max86150.c new file +9 -0
+	}
+
+	return IRQ_HANDLED;
+}
+
+/*
+ * This is a devm_add_action_or_reset() callback, so it can't return an
+ * error like the rest of this driver does -- dev_warn() is the only way
+ * left to surface a failed cleanup write here.

No need for the comment. If anyone actually thinks they can add the returns they'll rapidly figure it out!

Quoted 17 lines

+ */

drivers/iio/health/max86150.c new file +16 -0
+static void max86150_powerdown(void *arg)
+{
+	struct max86150_data *data = arg;
+	struct device *dev = regmap_get_device(data->regmap);
+	int ret;
+
+	ret = regmap_write(data->regmap, MAX86150_REG_INT_ENABLE1, 0);
+	if (ret)
+		dev_warn(dev, "Failed to disable interrupts: %d\n", ret);
+
+	ret = regmap_set_bits(data->regmap, MAX86150_REG_SYS_CTRL,
+			      MAX86150_SYS_SHDN);
+	if (ret)
+		dev_warn(dev, "Failed to shut down device: %d\n", ret);
+}
+

drivers/iio/health/max86150.c line 507

Quoted 10 lines
drivers/iio/health/max86150.c new file +10 -0
+static int max86150_chip_init(struct max86150_data *data)
+{
+	int ret;
+
+	ret = regmap_write(data->regmap, MAX86150_REG_SYS_CTRL,
+			   MAX86150_SYS_RESET);
+	if (ret)
+		return ret;
+
+	/* SYS_RESET self-clears within 1 ms (datasheet SYS_CTRL register) */

So why wait 10? We don't generally add margins as we get a bit extra anyway from the surounding calls and datasheets tend to be conservative.

Quoted 76 lines
drivers/iio/health/max86150.c new file +74 -0
+	fsleep(10 * USEC_PER_MSEC);
+
+	/*
+	 * FIFO_A_FULL holds (FIFO depth - samples available), i.e. how many
+	 * free slots remain when the interrupt should fire.
+	 */
+	ret = regmap_write(data->regmap, MAX86150_REG_FIFO_CONFIG,
+			   MAX86150_FIFO_ROLLOVER_EN |
+			   FIELD_PREP_CONST(MAX86150_FIFO_A_FULL,
+					    MAX86150_FIFO_DEPTH -
+					    MAX86150_FIFO_A_FULL_SAMPLES));
+	if (ret)
+		return ret;
+
+	ret = regmap_write(data->regmap, MAX86150_REG_FIFO_DCTRL1,
+			   FIELD_PREP_CONST(MAX86150_FIFO_FD1, MAX86150_FD_LED1) |
+			   FIELD_PREP_CONST(MAX86150_FIFO_FD2, MAX86150_FD_LED2));
+	if (ret)
+		return ret;
+
+	ret = regmap_write(data->regmap, MAX86150_REG_FIFO_DCTRL2,
+			   FIELD_PREP_CONST(MAX86150_FIFO_FD3, MAX86150_FD_ECG) |
+			   FIELD_PREP_CONST(MAX86150_FIFO_FD4, MAX86150_FD_NONE));
+	if (ret)
+		return ret;
+
+	ret = regmap_write(data->regmap, MAX86150_REG_PPG_CONFIG1,
+			   FIELD_PREP_CONST(MAX86150_PPG_ADC_RGE,
+					    MAX86150_PPG_ADC_RGE_16384_NA) |
+			   FIELD_PREP_CONST(MAX86150_PPG_SR,
+					    MAX86150_PPG_SR_SP_100_HZ));
+	if (ret)
+		return ret;
+
+	/* matches MAX86150_PPG_SR_SP_100_HZ above */
+	data->sample_period_ns = NSEC_PER_SEC / 100;
+
+	ret = regmap_write(data->regmap, MAX86150_REG_LED1_PA,
+			   MAX86150_LED_PA_DEFAULT);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(data->regmap, MAX86150_REG_LED2_PA,
+			   MAX86150_LED_PA_DEFAULT);
+	if (ret)
+		return ret;
+
+	return regmap_write(data->regmap, MAX86150_REG_SYS_CTRL,
+			    MAX86150_SYS_SHDN);
+}
 
 
+static int max86150_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct iio_dev *indio_dev;
+	struct max86150_data *data;
+	unsigned int part_id;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(indio_dev);
+
+	data->regmap = devm_regmap_init_i2c(client, &max86150_regmap_config);
+	if (IS_ERR(data->regmap))
+		return dev_err_probe(dev, PTR_ERR(data->regmap),
+				     "Failed to init regmap\n");
+
+	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(max86150_supply_names),
+					     max86150_supply_names);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable supplies\n");
+

drivers/iio/health/max86150.c line 596

Quoted 10 lines
drivers/iio/health/max86150.c new file +10 -0
+	ret = regmap_read(data->regmap, MAX86150_REG_PART_ID, &part_id);
+	if (ret)
+		return dev_err_probe(dev, ret, "Cannot read part ID\n");
+
+	/*
+	 * Deliberately fatal, not a dev_warn()+continue: chip_init() below
+	 * writes FIFO/PPG/LED configuration blind, with no readback. A
+	 * mismatched part ID means either the wrong device is on this
+	 * address or the bus itself is faulty, and letting chip_init()
+	 * write to that is a worse default than refusing to bind.

This doesn't align with the current thinking on device tree fallback compatible handling.

If the firmware is wrong then people get to keep the pieces and so we should at most print an informational message and then carry on anyway. A missmatched ID can also mean a future compatible part that has a different ID.

Thankfully people are very careful with firmwares when there is any chance of the bad form of smoke emerging!

Signature · 1 line
Jonathan Cameron <[email protected]>