* [PATCH v10 0/2] iio: health: add MAX86150 ECG and PPG biosensor driver
@ 2026-07-17 20:11 Md Shofiqul Islam
2026-07-17 20:11 ` [PATCH v10 1/2] dt-bindings: iio: health: add adi,max86150 Md Shofiqul Islam
2026-07-17 20:11 ` [PATCH v10 2/2] iio: health: add MAX86150 ECG and PPG biosensor driver Md Shofiqul Islam
0 siblings, 2 replies; 5+ messages in thread
From: Md Shofiqul Islam @ 2026-07-17 20:11 UTC (permalink / raw)
To: linux-iio
Cc: Md Shofiqul Islam, jic23, devicetree, robh, krzk+dt, conor+dt,
andriy.shevchenko, u.kleine-koenig, joshua.crofts1, nuno.sa,
Michael.Hennerich, dlechner, linux, linux-kernel
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 7047 bytes --]
Sorry for the long gap since v9. Between then and now the driver's
buffer implementation was rewritten from the kfifo-based design onto
the standard IIO hardware-trigger and triggered-buffer framework, and
I read through every open comment from every reviewer before sending
this rather than sending again too quickly.
I used AI assistance (Claude, Sonnet 5) while working through this, for
understanding kernel conventions and cross-checking against subsystem
patterns; that's disclosed via Assisted-by: Claude:claude-sonnet-5 tags
on both patches per Documentation/process/coding-assistants.rst, and
the design decisions and responsibility for what's here are mine.
Both patches now build and link cleanly (make M=drivers/iio/health),
checkpatch.pl reports 0 errors/0 warnings, and pahole confirms the
driver's private struct is 56 bytes/1 cacheline with only the expected
padding for timestamp alignment.
Testing: booted a kernel with this driver under virtme-ng/QEMU against
an i2c-stub device. Confirmed: probe correctly aborts with -ENODEV on
a part-ID mismatch (stub registers default to 0) and correctly binds
once the stub's PART_ID register is preset to 0x1E; the resulting
IIO device exposes the expected in_intensity_red_raw/in_intensity_ir_raw/
in_voltage_raw channels; direct reads on those channels time out
cleanly through the IIO_DEV_ACQUIRE_DIRECT_MODE()/regmap_read_poll_timeout()
path (i2c-stub never asserts PPG_RDY, so a ~25ms ETIMEDOUT is expected,
not a hang or crash); and module removal runs devm cleanup correctly
(SYS_CTRL read back with SHDN set after unbind, clean rmmod).
Not tested: i2c-stub has no interrupt line, so the actual
interrupt-driven path (max86150_trigger_enable(), the threaded
max86150_trigger_handler() FIFO drain, and buffer enable/disable) was
not exercised. That's the part of this series with the most new logic
and I don't have real hardware on hand to test it against; flagging
this explicitly rather than implying full coverage.
Changes since v9:
- 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 now only
reads and clears INT_STATUS1 before calling iio_trigger_poll();
the threaded trigger handler does the FIFO drain. This relies 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)
- Clear the stale A_FULL status bit before arming INT_ENABLE1, and
clear stale PPG_RDY before polling in the direct-read path; both
prevent a previously-latched flag from firing against garbage state
(Sashiko HIGH; Joshua)
- Use iio_get_time_ns(indio_dev) instead of ktime_get_ns() so
timestamps respect the configured IIO clock source (Sashiko MEDIUM)
- Zero the scan buffer before each push so disabled channels don't
leak stale data (Sashiko LOW)
- Added array_size.h, err.h, types.h; added a blank line before the
iio/ include group (Andy Shevchenko, Joshua)
- Dropped the free-standing FIFO_A_FULL_VAL/PPG_SR_100HZ/ADC_RGE_16384
single-value defines; inlined the FIFO threshold at its point of use
with an explanatory comment, and listed the full set of PPG_SR
(naming the single-/double-pulse variants) and PPG_ADC_RGE register
values instead of just the one in use (Jonathan Cameron)
- Removed the unneeded __aligned(IIO_DMA_MINALIGN) on the plain I2C
FIFO read buffer -- that alignment is for the IIO DMA buffer path,
not a bounce-buffer requirement I2C opts into (Jonathan Cameron)
- Switched read_raw() to IIO_DEV_ACQUIRE_DIRECT_MODE()/
IIO_DEV_ACQUIRE_FAILED(), with the read logic pulled into a
max86150_do_read_raw() helper (Jonathan Cameron)
- Dropped irq_get_trigger_type(): the device has no register to
reconfigure interrupt polarity or type, so IRQF_ONESHOT alone is
sufficient (Jonathan Cameron)
- Checked max86150_powerdown()'s regmap calls and log failures with
dev_warn(); fixed it to use regmap_set_bits() instead of a plain
regmap_write() for the shutdown bit so it no longer clobbers the
rest of SYS_CTRL (Joshua)
- Added blank lines between each regmap write + error-check block for
readability (Joshua)
- Declared the FIFO-drain loop variable at point of use (Joshua)
- Added a datasheet-referenced comment and blank line around the
SYS_RESET self-clear delay (Joshua)
- Moved the sample_period_ns assignment next to the PPG_CONFIG1 write
it's derived from, instead of sitting among unrelated LED_PA writes
(Jonathan Cameron)
- Trimmed the redundant "get/" from regulator failure messages, since
devm_regulator_get_enable() is a single combined call (Joshua)
- Fixed the MAINTAINERS entry to sort alphabetically among the other
MAX-prefixed entries (Sashiko LOW)
- Removed the Kconfig paragraph describing the driver's channels
instead of the hardware (Joshua)
- Added the avdd-supply and vref-supply properties and renamed
vled-supply to leds-supply, to cover all four independent supply
rails in the datasheet power tree (found while re-verifying the
binding against the driver)
- Fixed the DT binding example's interrupt flag from
IRQ_TYPE_EDGE_FALLING to IRQ_TYPE_LEVEL_LOW to match the driver's
actual level-triggered interrupt handling
- Fixed .validate_trigger in iio_info to iio_validate_own_trigger();
the previous iio_trigger_validate_own_device() has its arguments in
the opposite order and belongs on iio_trigger_ops.validate_device,
where it's used correctly. Caught by actually building the module,
not just reading the diff.
- Threaded this series properly with git send-email --thread this
time (Andy Shevchenko, Krzysztof Kozlowski)
- Added Nuno Sá, Michael Hennerich and David Lechner to Cc, per
scripts/get_maintainer.pl -- this is an Analog Devices part and I'd
missed CCing ADI's own IIO maintainers on every version so far
- Part-ID mismatch now aborts probe with dev_err_probe(dev, -ENODEV,
...) instead of logging and continuing. This goes beyond the
dev_info() vs dev_warn() log-level question raised in review: a
mismatched part ID means we're either talking to the wrong device or
something on the bus is wrong, and running chip_init() writes
against an unverified chip seemed like the riskier default (Joshua)
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 | 78 ++
MAINTAINERS | 7 +
drivers/iio/health/Kconfig | 13 +
drivers/iio/health/Makefile | 1 +
drivers/iio/health/max86150.c | 691 ++++++++++++++++++
5 files changed, 790 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/health/adi,max86150.yaml
create mode 100644 drivers/iio/health/max86150.c
--
2.51.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v10 1/2] dt-bindings: iio: health: add adi,max86150
2026-07-17 20:11 [PATCH v10 0/2] iio: health: add MAX86150 ECG and PPG biosensor driver Md Shofiqul Islam
@ 2026-07-17 20:11 ` Md Shofiqul Islam
2026-07-17 20:20 ` sashiko-bot
2026-07-17 20:11 ` [PATCH v10 2/2] iio: health: add MAX86150 ECG and PPG biosensor driver Md Shofiqul Islam
1 sibling, 1 reply; 5+ messages in thread
From: Md Shofiqul Islam @ 2026-07-17 20:11 UTC (permalink / raw)
To: linux-iio
Cc: Md Shofiqul Islam, jic23, devicetree, robh, krzk+dt, conor+dt,
andriy.shevchenko, u.kleine-koenig, joshua.crofts1, nuno.sa,
Michael.Hennerich, dlechner, linux, linux-kernel
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. Samples are buffered in a 32-entry hardware
FIFO with a configurable almost-full interrupt.
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
Signed-off-by: Md Shofiqul Islam <shofiqtest@gmail.com>
---
.../bindings/iio/health/adi,max86150.yaml | 78 +++++++++++++++++++
MAINTAINERS | 6 ++
2 files changed, 84 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/health/adi,max86150.yaml
diff --git a/Documentation/devicetree/bindings/iio/health/adi,max86150.yaml b/Documentation/devicetree/bindings/iio/health/adi,max86150.yaml
new file mode 100644
index 0000000000000..da2ff9b0ada2b
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/health/adi,max86150.yaml
@@ -0,0 +1,78 @@
+# 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 <shofiqtest@gmail.com>
+
+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 and raises an
+ active-low interrupt when the 32-entry hardware FIFO reaches its
+ configurable almost-full threshold.
+
+ 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 FIFO almost-full
+ threshold is reached 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>;
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 3115538ce8291..189385966de82 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -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 <shofiqtest@gmail.com>
+L: linux-iio@vger.kernel.org
+S: Maintained
+F: Documentation/devicetree/bindings/iio/health/adi,max86150.yaml
+
MAX9286 QUAD GMSL DESERIALIZER DRIVER
M: Jacopo Mondi <jacopo+renesas@jmondi.org>
M: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
--
2.51.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v10 2/2] iio: health: add MAX86150 ECG and PPG biosensor driver
2026-07-17 20:11 [PATCH v10 0/2] iio: health: add MAX86150 ECG and PPG biosensor driver Md Shofiqul Islam
2026-07-17 20:11 ` [PATCH v10 1/2] dt-bindings: iio: health: add adi,max86150 Md Shofiqul Islam
@ 2026-07-17 20:11 ` Md Shofiqul Islam
2026-07-17 20:30 ` sashiko-bot
1 sibling, 1 reply; 5+ messages in thread
From: Md Shofiqul Islam @ 2026-07-17 20:11 UTC (permalink / raw)
To: linux-iio
Cc: Md Shofiqul Islam, jic23, devicetree, robh, krzk+dt, conor+dt,
andriy.shevchenko, u.kleine-koenig, joshua.crofts1, nuno.sa,
Michael.Hennerich, dlechner, linux, linux-kernel
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 24309 bytes --]
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 in a single I2C device.
The device has a 32-entry hardware FIFO with a configurable almost-full
interrupt. The driver uses the standard IIO hardware-trigger and
triggered-buffer framework: a hard-irq handler reads and clears
INT_STATUS1 to de-assert the line before calling iio_trigger_poll(),
and the threaded trigger handler drains the FIFO and pushes samples,
with timestamps back-calculated from the interrupt arrival time by one
sample_period_ns per step. Relying on the trigger core's own
attach/detach synchronization avoids the need for an explicit
iio_buffer_enabled() guard or synchronize_irq() in the interrupt path.
Key implementation details:
- Part ID register (0xFF) verified against 0x1E on probe; mismatched
devices are rejected with -ENODEV so the driver cannot bind to the
wrong hardware
- Stale A_FULL and PPG_RDY status bits are cleared before arming the
interrupt or polling for a sample, so a previously-latched flag
cannot fire the handler against garbage state
- 24-bit FIFO words decoded directly from the raw byte buffer
- regmap_set_bits() / regmap_clear_bits() for single-direction writes
- Device remains in shutdown between captures to suppress LED current
- vdd, avdd, vref and leds regulators required per the datasheet power
tree
- iio_get_time_ns() used for timestamps so they respect the IIO
device's configured clock source
- A_FULL status bit used to detect FIFO exactly full (wr_ptr == rd_ptr
with OVF_COUNTER == 0) so valid samples are not silently dropped
- No IRQ trigger-type override: the device has no register to
reconfigure interrupt polarity or type, so IRQF_ONESHOT with
whatever type firmware provides is sufficient
Suggested-by: Jonathan Cameron <jic23@kernel.org>
Suggested-by: Joshua Crofts <joshua.crofts1@gmail.com>
Assisted-by: Claude:claude-sonnet-5
Signed-off-by: Md Shofiqul Islam <shofiqtest@gmail.com>
---
MAINTAINERS | 1 +
drivers/iio/health/Kconfig | 13 +
drivers/iio/health/Makefile | 1 +
drivers/iio/health/max86150.c | 691 ++++++++++++++++++++++++++++++++++
4 files changed, 706 insertions(+)
create mode 100644 drivers/iio/health/max86150.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 189385966de82..5e266c03bda6f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15710,6 +15710,7 @@ M: Md Shofiqul Islam <shofiqtest@gmail.com>
L: linux-iio@vger.kernel.org
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 <jacopo+renesas@jmondi.org>
diff --git a/drivers/iio/health/Kconfig b/drivers/iio/health/Kconfig
index a89f3abf11f4a..7438cdfe4238f 100644
--- a/drivers/iio/health/Kconfig
+++ b/drivers/iio/health/Kconfig
@@ -62,4 +62,17 @@ config MAX30102
endmenu
+config MAX86150
+ tristate "MAX86150 ECG and PPG biosensor"
+ depends on I2C
+ select IIO_BUFFER
+ select IIO_TRIGGERED_BUFFER
+ 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
diff --git a/drivers/iio/health/Makefile b/drivers/iio/health/Makefile
index 9108171122588..04fc73c584449 100644
--- a/drivers/iio/health/Makefile
+++ b/drivers/iio/health/Makefile
@@ -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
diff --git a/drivers/iio/health/max86150.c b/drivers/iio/health/max86150.c
new file mode 100644
index 0000000000000..47d96570a95fc
--- /dev/null
+++ b/drivers/iio/health/max86150.c
@@ -0,0 +1,691 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * MAX86150 combined ECG and PPG biosensor driver
+ *
+ * Copyright (C) 2026 Md Shofiqul Islam <shofiqtest@gmail.com>
+ *
+ * The MAX86150 integrates two PPG optical channels (Red/IR LED) and one
+ * ECG biopotential channel in a single I2C device. Data is captured
+ * through a 32-entry hardware FIFO with a configurable almost-full
+ * interrupt, making it well-suited for continuous monitoring with a
+ * low-power host.
+ *
+ * Datasheet:
+ * https://www.analog.com/media/en/technical-documentation/data-sheets/MAX86150.pdf
+ */
+
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/delay.h>
+#include <linux/dev_printk.h>
+#include <linux/device/devres.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/time.h>
+#include <linux/types.h>
+
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+#define MAX86150_REG_INT_STATUS1 0x00
+#define MAX86150_REG_INT_STATUS2 0x01
+#define MAX86150_REG_INT_ENABLE1 0x02
+#define MAX86150_REG_INT_ENABLE2 0x03
+#define MAX86150_REG_FIFO_WR_PTR 0x04
+#define MAX86150_REG_OVF_COUNTER 0x05
+#define MAX86150_REG_FIFO_RD_PTR 0x06
+#define MAX86150_REG_FIFO_DATA 0x07
+#define MAX86150_REG_FIFO_CONFIG 0x08
+#define MAX86150_REG_FIFO_DCTRL1 0x09
+#define MAX86150_REG_FIFO_DCTRL2 0x0A
+#define MAX86150_REG_SYS_CTRL 0x0D
+#define MAX86150_REG_PPG_CONFIG1 0x10
+#define MAX86150_REG_PPG_CONFIG2 0x11
+#define MAX86150_REG_LED1_PA 0x14
+#define MAX86150_REG_LED2_PA 0x15
+#define MAX86150_REG_ECG_CONFIG1 0x3C
+#define MAX86150_REG_ECG_CONFIG3 0x3E
+#define MAX86150_REG_PART_ID 0xFF
+
+#define MAX86150_PART_ID_VAL 0x1E
+
+#define MAX86150_INT_A_FULL BIT(7)
+#define MAX86150_INT_PPG_RDY BIT(6)
+
+#define MAX86150_SYS_SHDN BIT(1)
+#define MAX86150_SYS_RESET BIT(0)
+
+#define MAX86150_FIFO_SMP_AVE GENMASK(7, 5)
+#define MAX86150_FIFO_ROLLOVER_EN BIT(4)
+#define MAX86150_FIFO_A_FULL GENMASK(3, 0)
+
+#define MAX86150_FD_NONE 0x0
+#define MAX86150_FD_LED1 0x1
+#define MAX86150_FD_LED2 0x2
+#define MAX86150_FD_ECG 0x9
+#define MAX86150_FIFO_FD1 GENMASK(3, 0)
+#define MAX86150_FIFO_FD2 GENMASK(7, 4)
+#define MAX86150_FIFO_FD3 GENMASK(3, 0)
+#define MAX86150_FIFO_FD4 GENMASK(7, 4)
+
+#define MAX86150_PPG_ADC_RGE GENMASK(7, 6)
+#define MAX86150_PPG_SR GENMASK(5, 1)
+
+/* PPG ADC full-scale range (ADC_RGE field of PPG_CONFIG1) */
+#define MAX86150_PPG_ADC_RGE_4096 0 /* 4096 nA */
+#define MAX86150_PPG_ADC_RGE_8192 1 /* 8192 nA */
+#define MAX86150_PPG_ADC_RGE_16384 2 /* 16384 nA */
+#define MAX86150_PPG_ADC_RGE_32768 3 /* 32768 nA */
+
+/* PPG sample rate (SR field of PPG_CONFIG1) - single-pulse variants */
+#define MAX86150_PPG_SR_SP_10HZ 0
+#define MAX86150_PPG_SR_SP_20HZ 1
+#define MAX86150_PPG_SR_SP_50HZ 2
+#define MAX86150_PPG_SR_SP_84HZ 3
+#define MAX86150_PPG_SR_SP_100HZ 4
+#define MAX86150_PPG_SR_SP_200HZ 5
+#define MAX86150_PPG_SR_SP_400HZ 6
+#define MAX86150_PPG_SR_SP_800HZ 7
+#define MAX86150_PPG_SR_SP_1000HZ 8
+#define MAX86150_PPG_SR_SP_1600HZ 9
+#define MAX86150_PPG_SR_SP_3200HZ 10
+/* Double-pulse variants (two LED pulses averaged per sample) */
+#define MAX86150_PPG_SR_DP_10HZ 11
+#define MAX86150_PPG_SR_DP_20HZ 12
+#define MAX86150_PPG_SR_DP_50HZ 13
+#define MAX86150_PPG_SR_DP_84HZ 14
+#define MAX86150_PPG_SR_DP_100HZ 15
+#define MAX86150_PPG_SR_DP_200HZ 16
+#define MAX86150_PPG_SR_DP_400HZ 17
+#define MAX86150_PPG_SR_DP_800HZ 18
+#define MAX86150_PPG_SR_DP_1000HZ 19
+#define MAX86150_PPG_SR_DP_1600HZ 20
+#define MAX86150_PPG_SR_DP_3200HZ 21
+
+/* LED pulse amplitude: 0x00 = 0 mA, step ~0.8 mA, 0x3F ~= 50 mA, 0xFF ~= 200 mA */
+
+#define MAX86150_FIFO_DEPTH 32
+#define MAX86150_BYTES_PER_SLOT 3
+#define MAX86150_NUM_SLOTS 3
+#define MAX86150_SAMPLE_BYTES (MAX86150_NUM_SLOTS * MAX86150_BYTES_PER_SLOT)
+
+/* Samples available in the FIFO when the A_FULL interrupt fires */
+#define MAX86150_FIFO_A_FULL_SAMPLES 17
+
+#define MAX86150_LED_PA_DEFAULT 0x3F
+
+enum max86150_scan_idx {
+ MAX86150_IDX_PPG_RED,
+ MAX86150_IDX_PPG_IR,
+ MAX86150_IDX_ECG,
+ MAX86150_IDX_TS,
+};
+
+/**
+ * struct max86150_data - driver private state
+ * @regmap: register map for this device
+ * @trig: IIO hardware trigger backed by the device interrupt line
+ * @sample_period_ns: sample period in nanoseconds (set from configured rate)
+ * @fifo_raw: scratch buffer for regmap_noinc_read() FIFO bursts; kept
+ * 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,
+ * with a trailing aligned_s64 slot for the timestamp
+ */
+struct max86150_data {
+ struct regmap *regmap;
+ struct iio_trigger *trig;
+ u32 sample_period_ns;
+ u8 fifo_raw[MAX86150_SAMPLE_BYTES];
+ IIO_DECLARE_DMA_BUFFER_WITH_TS(s32, scan, MAX86150_NUM_SLOTS);
+};
+
+static const struct iio_chan_spec max86150_channels[] = {
+ {
+ .type = IIO_INTENSITY,
+ .modified = 1,
+ .channel2 = IIO_MOD_LIGHT_RED,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+ .scan_index = MAX86150_IDX_PPG_RED,
+ .scan_type = {
+ .sign = 'u',
+ .realbits = 19,
+ .storagebits = 32,
+ .endianness = IIO_CPU,
+ },
+ },
+ {
+ .type = IIO_INTENSITY,
+ .modified = 1,
+ .channel2 = IIO_MOD_LIGHT_IR,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+ .scan_index = MAX86150_IDX_PPG_IR,
+ .scan_type = {
+ .sign = 'u',
+ .realbits = 19,
+ .storagebits = 32,
+ .endianness = IIO_CPU,
+ },
+ },
+ {
+ .type = IIO_VOLTAGE,
+ .channel = 0,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+ .scan_index = MAX86150_IDX_ECG,
+ .scan_type = {
+ .sign = 's',
+ .realbits = 18,
+ .storagebits = 32,
+ .endianness = IIO_CPU,
+ },
+ },
+ IIO_CHAN_SOFT_TIMESTAMP(MAX86150_IDX_TS),
+};
+
+static const struct regmap_config max86150_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = MAX86150_REG_PART_ID,
+};
+
+static int max86150_read_one_sample(struct max86150_data *data,
+ u32 *ppg_red, u32 *ppg_ir, s32 *ecg)
+{
+ int ret;
+
+ ret = regmap_noinc_read(data->regmap, MAX86150_REG_FIFO_DATA,
+ data->fifo_raw, MAX86150_SAMPLE_BYTES);
+ if (ret)
+ return ret;
+
+ *ppg_red = (data->fifo_raw[0] & 0x07) << 16 |
+ data->fifo_raw[1] << 8 | data->fifo_raw[2];
+ *ppg_ir = (data->fifo_raw[3] & 0x07) << 16 |
+ data->fifo_raw[4] << 8 | data->fifo_raw[5];
+ *ecg = sign_extend32((data->fifo_raw[6] & 0x03) << 16 |
+ data->fifo_raw[7] << 8 | data->fifo_raw[8], 17);
+ return 0;
+}
+
+/*
+ * 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.
+ */
+static int max86150_do_read_raw(struct max86150_data *data,
+ u32 *ppg_red, u32 *ppg_ir, s32 *ecg)
+{
+ 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)
+ goto out_shdn;
+
+ ret = regmap_write(data->regmap, MAX86150_REG_OVF_COUNTER, 0);
+ if (ret)
+ goto out_shdn;
+
+ ret = regmap_write(data->regmap, MAX86150_REG_FIFO_RD_PTR, 0);
+ if (ret)
+ goto out_shdn;
+
+ /*
+ * 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)
+ goto out_shdn;
+
+ /*
+ * 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,
+ 1000, 25000);
+ if (ret)
+ goto out_shdn;
+
+ ret = max86150_read_one_sample(data, ppg_red, ppg_ir, ecg);
+
+out_shdn:
+ regmap_set_bits(data->regmap, MAX86150_REG_SYS_CTRL, MAX86150_SYS_SHDN);
+ return ret;
+}
+
+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);
+ 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;
+
+ 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;
+ break;
+ case MAX86150_IDX_PPG_IR:
+ *val = ppg_ir;
+ break;
+ case MAX86150_IDX_ECG:
+ *val = ecg;
+ break;
+ default:
+ return -EINVAL;
+ }
+ return IIO_VAL_INT;
+ }
+ default:
+ return -EINVAL;
+ }
+}
+
+static const struct iio_info max86150_iio_info = {
+ .read_raw = max86150_read_raw,
+ .validate_trigger = iio_validate_own_trigger,
+};
+
+static int max86150_trigger_disable(struct max86150_data *data)
+{
+ 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);
+}
+
+static int max86150_trigger_enable(struct max86150_data *data)
+{
+ unsigned int dummy;
+ 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)
+ goto err_shdn;
+
+ ret = regmap_write(data->regmap, MAX86150_REG_OVF_COUNTER, 0);
+ if (ret)
+ goto err_shdn;
+
+ ret = regmap_write(data->regmap, MAX86150_REG_FIFO_RD_PTR, 0);
+ if (ret)
+ goto err_shdn;
+
+ /*
+ * Clear a stale A_FULL latched from before this trigger was enabled;
+ * otherwise arming INT_ENABLE1 below fires the handler immediately
+ * against a FIFO state that was never actually seen as full.
+ */
+ ret = regmap_read(data->regmap, MAX86150_REG_INT_STATUS1, &dummy);
+ if (ret)
+ goto err_shdn;
+
+ ret = regmap_write(data->regmap, MAX86150_REG_INT_ENABLE1,
+ MAX86150_INT_A_FULL);
+ if (ret)
+ goto err_shdn;
+ return 0;
+
+err_shdn:
+ regmap_set_bits(data->regmap, MAX86150_REG_SYS_CTRL, MAX86150_SYS_SHDN);
+ return ret;
+}
+
+static int max86150_set_trigger_state(struct iio_trigger *trig, bool state)
+{
+ struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+ struct max86150_data *data = iio_priv(indio_dev);
+
+ if (state)
+ return max86150_trigger_enable(data);
+ return max86150_trigger_disable(data);
+}
+
+static const struct iio_trigger_ops max86150_trigger_ops = {
+ .set_trigger_state = max86150_set_trigger_state,
+ .validate_device = iio_trigger_validate_own_device,
+};
+
+/*
+ * Threaded IRQ handler: reads and clears INT_STATUS1 to de-assert the
+ * hardware interrupt line BEFORE iio_trigger_poll() re-enables it. This
+ * prevents an IRQ storm on level-triggered lines where the line would
+ * remain asserted until max86150_trigger_handler() ran later in its own
+ * kthread — after IRQF_ONESHOT had already unmasked the IRQ.
+ */
+static irqreturn_t max86150_irq_handler(int irq, void *private)
+{
+ struct iio_trigger *trig = private;
+ struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+ struct max86150_data *data = iio_priv(indio_dev);
+ unsigned int status;
+ int ret;
+
+ ret = regmap_read(data->regmap, MAX86150_REG_INT_STATUS1, &status);
+ if (ret || !(status & MAX86150_INT_A_FULL))
+ return IRQ_NONE;
+
+ iio_trigger_poll(trig);
+ return IRQ_HANDLED;
+}
+
+static irqreturn_t max86150_trigger_handler(int irq, void *p)
+{
+ struct iio_poll_func *pf = p;
+ struct iio_dev *idev = pf->indio_dev;
+ struct max86150_data *data = iio_priv(idev);
+ unsigned int wr_ptr, rd_ptr, ovf, n_avail;
+ u32 ppg_red, ppg_ir;
+ s32 ecg;
+ s64 t_drain = 0;
+ int ret;
+
+ /*
+ * INT_STATUS1 was already read (and the interrupt de-asserted) by
+ * max86150_irq_handler(). Read only the FIFO pointers here.
+ */
+ ret = regmap_read(data->regmap, MAX86150_REG_FIFO_WR_PTR, &wr_ptr);
+ if (ret)
+ goto done;
+ ret = regmap_read(data->regmap, MAX86150_REG_FIFO_RD_PTR, &rd_ptr);
+ if (ret)
+ goto done;
+ ret = regmap_read(data->regmap, MAX86150_REG_OVF_COUNTER, &ovf);
+ if (ret)
+ goto done;
+
+ if (ovf > 0) {
+ n_avail = MAX86150_FIFO_DEPTH;
+ t_drain = iio_get_time_ns(idev);
+ } 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 is only called 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 = 0;
+ s64 ts;
+
+ if (ovf > 0)
+ ts = t_drain -
+ (s64)(n_avail - 1 - i) * data->sample_period_ns;
+ else
+ ts = pf->timestamp +
+ ((s64)i - (MAX86150_FIFO_A_FULL_SAMPLES - 1)) *
+ 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));
+
+ if (test_bit(MAX86150_IDX_PPG_RED, idev->active_scan_mask))
+ data->scan[j++] = ppg_red;
+ if (test_bit(MAX86150_IDX_PPG_IR, idev->active_scan_mask))
+ data->scan[j++] = ppg_ir;
+ if (test_bit(MAX86150_IDX_ECG, idev->active_scan_mask))
+ data->scan[j++] = ecg;
+
+ iio_push_to_buffers_with_timestamp(idev, data->scan, ts);
+ }
+
+done:
+ iio_trigger_notify_done(idev->trig);
+ return IRQ_HANDLED;
+}
+
+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);
+}
+
+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) */
+ 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(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(MAX86150_FIFO_FD1, MAX86150_FD_LED1) |
+ FIELD_PREP(MAX86150_FIFO_FD2, MAX86150_FD_LED2));
+ if (ret)
+ return ret;
+
+ ret = regmap_write(data->regmap, MAX86150_REG_FIFO_DCTRL2,
+ FIELD_PREP(MAX86150_FIFO_FD3, MAX86150_FD_ECG) |
+ FIELD_PREP(MAX86150_FIFO_FD4, MAX86150_FD_NONE));
+ if (ret)
+ return ret;
+
+ ret = regmap_write(data->regmap, MAX86150_REG_PPG_CONFIG1,
+ FIELD_PREP(MAX86150_PPG_ADC_RGE,
+ MAX86150_PPG_ADC_RGE_16384) |
+ FIELD_PREP(MAX86150_PPG_SR,
+ MAX86150_PPG_SR_SP_100HZ));
+ if (ret)
+ return ret;
+
+ data->sample_period_ns = 10000000; /* matches MAX86150_PPG_SR_SP_100HZ above */
+
+ 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);
+
+ ret = devm_regulator_get_enable(dev, "vdd");
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to enable vdd supply\n");
+
+ ret = devm_regulator_get_enable(dev, "avdd");
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to enable avdd supply\n");
+
+ ret = devm_regulator_get_enable(dev, "vref");
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to enable vref supply\n");
+
+ ret = devm_regulator_get_enable(dev, "leds");
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to enable leds supply\n");
+
+ 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 = regmap_read(data->regmap, MAX86150_REG_PART_ID, &part_id);
+ if (ret)
+ return dev_err_probe(dev, ret, "Cannot read part ID\n");
+
+ if (part_id != MAX86150_PART_ID_VAL)
+ return dev_err_probe(dev, -ENODEV,
+ "Unexpected part ID 0x%02x (expected 0x%02x)\n",
+ part_id, MAX86150_PART_ID_VAL);
+
+ ret = max86150_chip_init(data);
+ if (ret)
+ return dev_err_probe(dev, ret, "Chip initialisation failed\n");
+
+ ret = devm_add_action_or_reset(dev, max86150_powerdown, data);
+ if (ret)
+ return ret;
+
+ indio_dev->name = "max86150";
+ indio_dev->channels = max86150_channels;
+ indio_dev->num_channels = ARRAY_SIZE(max86150_channels);
+ indio_dev->info = &max86150_iio_info;
+ indio_dev->modes = INDIO_DIRECT_MODE;
+
+ if (client->irq > 0) {
+ data->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
+ indio_dev->name,
+ iio_device_id(indio_dev));
+ if (!data->trig)
+ return -ENOMEM;
+
+ data->trig->ops = &max86150_trigger_ops;
+ iio_trigger_set_drvdata(data->trig, indio_dev);
+
+ /*
+ * The device only ever drives an active-low interrupt line;
+ * there is no register to reconfigure its polarity or type,
+ * so the trigger type from firmware needs no help here.
+ */
+ ret = devm_request_threaded_irq(dev, client->irq,
+ NULL,
+ max86150_irq_handler,
+ IRQF_ONESHOT,
+ "max86150", data->trig);
+ if (ret)
+ return ret;
+
+ ret = devm_iio_trigger_register(dev, data->trig);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Failed to register trigger\n");
+ }
+
+ ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
+ iio_pollfunc_store_time,
+ max86150_trigger_handler,
+ NULL);
+ if (ret)
+ return ret;
+
+ /*
+ * Set the default trigger AFTER buffer setup succeeds. Setting it
+ * before would leak the iio_trigger_get() reference if buffer setup
+ * failed: INDIO_BUFFER_TRIGGERED is not set on that path so
+ * iio_device_release() skips iio_trigger_put().
+ */
+ if (data->trig)
+ indio_dev->trig = iio_trigger_get(data->trig);
+
+ return devm_iio_device_register(dev, indio_dev);
+}
+
+static const struct i2c_device_id max86150_id[] = {
+ { "max86150" },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, max86150_id);
+
+static const struct of_device_id max86150_of_match[] = {
+ { .compatible = "adi,max86150" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, max86150_of_match);
+
+static struct i2c_driver max86150_driver = {
+ .driver = {
+ .name = "max86150",
+ .of_match_table = max86150_of_match,
+ },
+ .probe = max86150_probe,
+ .id_table = max86150_id,
+};
+module_i2c_driver(max86150_driver);
+
+MODULE_AUTHOR("Md Shofiqul Islam <shofiqtest@gmail.com>");
+MODULE_DESCRIPTION("MAX86150 ECG and PPG biosensor driver");
+MODULE_LICENSE("GPL");
--
2.51.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v10 1/2] dt-bindings: iio: health: add adi,max86150
2026-07-17 20:11 ` [PATCH v10 1/2] dt-bindings: iio: health: add adi,max86150 Md Shofiqul Islam
@ 2026-07-17 20:20 ` sashiko-bot
0 siblings, 0 replies; 5+ messages in thread
From: sashiko-bot @ 2026-07-17 20:20 UTC (permalink / raw)
To: Md Shofiqul Islam; +Cc: conor+dt, devicetree, robh
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The `interrupts` property is missing from the `required` list.
--
--- Patch [1]: [PATCH v10 1/2] dt-bindings: iio: health: add adi,max86150 ---
commit 6596564838aa000e36be829746bacd2e720d88d8
Author: Md Shofiqul Islam <shofiqtest@gmail.com>
dt-bindings: iio: health: add adi,max86150
This commit adds the Device Tree binding schema for the Analog Devices
MAX86150 integrated biosensor. It outlines the configuration for the
I2C device, including supply rails and a configurable interrupt for
the hardware FIFO.
> diff --git a/Documentation/devicetree/bindings/iio/health/adi,max86150.yaml b/Documentation/devicetree/bindings/iio/health/adi,max86150.yaml
> new file mode 100644
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/health/adi,max86150.yaml
[ ... ]
> +required:
> + - compatible
> + - reg
> + - vdd-supply
> + - avdd-supply
> + - vref-supply
> + - leds-supply
[Severity: Medium]
Should the interrupts property be included in this required list?
The commit message notes that the device raises an interrupt when the
hardware FIFO reaches its almost-full threshold. If the interrupts
property is omitted from the required list, device trees without it
will incorrectly pass schema validation, potentially leading to
runtime failures where the driver cannot receive interrupts to drain
the buffer.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260717201138.1078019-1-shofiqtest@gmail.com?part=1
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v10 2/2] iio: health: add MAX86150 ECG and PPG biosensor driver
2026-07-17 20:11 ` [PATCH v10 2/2] iio: health: add MAX86150 ECG and PPG biosensor driver Md Shofiqul Islam
@ 2026-07-17 20:30 ` sashiko-bot
0 siblings, 0 replies; 5+ messages in thread
From: sashiko-bot @ 2026-07-17 20:30 UTC (permalink / raw)
To: Md Shofiqul Islam; +Cc: devicetree, conor+dt, robh
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The driver uses hallucinated locking macros causing a build failure, and omits the mandatory lock release.
- [High] The I2C read buffer `fifo_raw` is not cacheline aligned, violating DMA API requirements.
- [High] The threaded IRQ handler incorrectly calls a hardirq-only IIO trigger poll function.
- [Low] The i2c_device_id array is missing named initializers.
--
--- Patch [2]: [PATCH v10 2/2] iio: health: add MAX86150 ECG and PPG biosensor driver ---
commit b6c51e5d5e6b5f625bd2546556d36647cd05b62e
Author: Md Shofiqul Islam <shofiqtest@gmail.com>
iio: health: add MAX86150 ECG and PPG biosensor driver
This commit adds a new IIO driver for the Analog Devices MAX86150
combined ECG and PPG biosensor. It supports hardware FIFO captures via
a threaded trigger handler.
> diff --git a/drivers/iio/health/max86150.c b/drivers/iio/health/max86150.c
> new file mode 100644
> index 0000000000000..47d96570a95fc
> --- /dev/null
> +++ b/drivers/iio/health/max86150.c
[ ... ]
> +struct max86150_data {
> + struct regmap *regmap;
> + struct iio_trigger *trig;
> + u32 sample_period_ns;
> + u8 fifo_raw[MAX86150_SAMPLE_BYTES];
[Severity: High]
Since this array is the target of regmap_noinc_read() and some I2C host
controllers use DMA for bulk reads, should this buffer be cacheline aligned?
Sharing a cacheline with sample_period_ns and trig can lead to silent
corruption if the CPU accesses them concurrently during a DMA transfer.
> + IIO_DECLARE_DMA_BUFFER_WITH_TS(s32, scan, MAX86150_NUM_SLOTS);
> +};
[ ... ]
> +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);
> + 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: High]
Do the IIO_DEV_ACQUIRE_DIRECT_MODE and IIO_DEV_ACQUIRE_FAILED macros
exist in the IIO framework? If these are replaced with the standard
iio_device_claim_direct() function, does this function also need
corresponding iio_device_release_direct() calls on its return paths to
prevent deadlocking the IIO device state?
> +
> + ret = max86150_do_read_raw(data, &ppg_red, &ppg_ir, &ecg);
[ ... ]
> +static irqreturn_t max86150_irq_handler(int irq, void *private)
> +{
> + struct iio_trigger *trig = private;
> + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> + struct max86150_data *data = iio_priv(indio_dev);
> + unsigned int status;
> + int ret;
> +
> + ret = regmap_read(data->regmap, MAX86150_REG_INT_STATUS1, &status);
> + if (ret || !(status & MAX86150_INT_A_FULL))
> + return IRQ_NONE;
> +
> + iio_trigger_poll(trig);
[Severity: High]
Since this handler is registered as the thread_fn in devm_request_threaded_irq
with a NULL hardirq handler, does calling iio_trigger_poll() here violate
locking rules? iio_trigger_poll() expects to run in hardirq context. Should
this use iio_trigger_poll_nested() instead?
> + return IRQ_HANDLED;
> +}
[ ... ]
> +static const struct i2c_device_id max86150_id[] = {
> + { "max86150" },
[Severity: Low]
Does the I2C subsystem API require initialized arrays of type
struct i2c_device_id to use named initializers? Using named initializers
such as { .name = "max86150" } ensures compliance with subsystem guidelines.
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, max86150_id);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260717201138.1078019-1-shofiqtest@gmail.com?part=2
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-07-17 20:30 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-17 20:11 [PATCH v10 0/2] iio: health: add MAX86150 ECG and PPG biosensor driver Md Shofiqul Islam
2026-07-17 20:11 ` [PATCH v10 1/2] dt-bindings: iio: health: add adi,max86150 Md Shofiqul Islam
2026-07-17 20:20 ` sashiko-bot
2026-07-17 20:11 ` [PATCH v10 2/2] iio: health: add MAX86150 ECG and PPG biosensor driver Md Shofiqul Islam
2026-07-17 20:30 ` sashiko-bot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox