iio: health: add MAX86150 ECG and PPG biosensor driver
Subsystems (3)
v7 cover iio: health: add MAX86150 ECG and PPG biosensor driver
Changes in v7 (Jonathan Cameron's review of v5 driver):
- Switch from triggered buffer to kfifo buffer (like MAX30100/MAX30102) and remove the IIO trigger framework from this driver entirely
- Remove spurious 'select IIO_TRIGGER' added to AFE4403, AFE4404, MAX30100, MAX30102 in the Kconfig patch
- Rename all register field macros to include the parent register name and a _MASK suffix (e.g. MAX86150_PPG_CONFIG1_ADC_RGE_MASK)
- Use IIO_DMA_MINALIGN instead of ARCH_DMA_MINALIGN
- Use IIO_DECLARE_BUFFER_WITH_TS() macro for the push buffer
- Fix regmap_noinc_read() length: use MAX86150_SAMPLE_BYTES (9) not sizeof(fifo_raw) (64) -- the padded size is for DMA alignment only; using it would over-read the FIFO by 7 extra samples per call
- Use get_unaligned_be24() with a single mask for 24-bit FIFO reads
- Use regmap_set_bits() / regmap_clear_bits() instead of regmap_update_bits
- Use iio_push_to_buffers_with_ts() (new name)
- Use fsleep() instead of usleep_range() after software reset
- Use 10 * NSEC_PER_MSEC instead of bare 10000000
- Replace if (!ret) chains with goto-based error handling
- Remove all code-section comments (/* Trigger */, /* Probe */, etc.)
- Remove forced variable declaration alignment
- Print a warning (not -ENODEV) for unexpected part ID
- Remove IRQ trigger type fallback; honour DT specification only
- Remove memset of push buffer before packing
- Drop overflow timestamp reconstruction; flush FIFO and discard samples
- Remove struct device *dev field (unused after removing trigger)
- Rename LED_PA_DEFAULT to LED_PA_50MA to make the value self-describing
- Remove vref regulator (not a supply pin per the datasheet); keep vdd and vled; change to non-optional devm_regulator_get_enable()
Changes in v6 (Jonathan Cameron's review of v5 DT binding):
- Remove "SoC" from description; spell out "photoplethysmography (PPG)" and "electrocardiogram (ECG)" in full
- Remove the I2C fast-mode (400 kHz) line
- Simplify interrupt description to a single sentence
Changes in v5:
- Rebase on iio/testing
- Adjust FIFO slot descriptor defines
- Fix scan_type for ECG (signed 18-bit)
v4 and earlier: initial submission and review iterations
Md Shofiqul Islam (3):
dt-bindings: iio: health: add adi,max86150
iio: health: add MAX86150 ECG and PPG biosensor driver
MAINTAINERS: add entry for MAX86150 IIO health driver
.../bindings/iio/health/adi,max86150.yaml | 65 +++ MAINTAINERS | 7 + drivers/iio/health/Kconfig | 18 + drivers/iio/health/Makefile | 1 + drivers/iio/health/max86150.c | 533 ++++++++++++++++++ 5 files changed, 624 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/health/adi,max86150.yaml create mode 100644 drivers/iio/health/max86150.c
--
2.51.1
PATCH v7 1/3 dt-bindings: iio: health: add adi,max86150
Add Device Tree binding schema for the Analog Devices MAX86150 integrated ECG and PPG biosensor.
The device exposes two PPG channels (Red LED and IR LED) and one ECG channel over I2C, with a 32-entry hardware FIFO and an active-low interrupt.
--- .../bindings/iio/health/adi,max86150.yaml | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/health/adi,max86150.yaml
Documentation/devicetree/bindings/iio/health/adi,max86150.yaml new file +65 -0
@@ -0,0 +1,65 @@ +# 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 that combines two + photoplethysmography (PPG) channels (Red LED and IR LED) and one + electrocardiogram (ECG) channel for biopotential recording. Samples + are buffered in a 32-entry hardware FIFO with a configurable + almost-full interrupt. + + 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: + Interrupt line, asserted when the FIFO almost-full threshold is + reached. + + vdd-supply: + description: VDD power supply (1.8 V). + + vled-supply: + description: VLED LED anode supply (3.3 V to 5 V). + +required: + - compatible + - reg + - vdd-supply + - vled-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>; + vled-supply = <&vdd_3v3>; + }; + };
Signature · 1 line
2.51.1
PATCH v7 2/3 iio: health: add MAX86150 ECG and PPG biosensor driver
In reply to Md Shofiqul IslamAdd 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. Because all samples arrive via the FIFO, the driver uses a kfifo buffer (like the sibling MAX30100 and MAX30102 drivers) rather than the triggered-buffer framework. The interrupt handler drains the FIFO on each A_FULL event and timestamps samples back-calculated from the interrupt arrival time by one sample_period_ns per step.
Key implementation details:
- FIFO draining via iio_buffer_setup_ops postenable/predisable
- DMA-safe FIFO read buffer aligned to IIO_DMA_MINALIGN
- IIO_DECLARE_BUFFER_WITH_TS for the push buffer
- 24-bit FIFO words decoded via get_unaligned_be24()
- regmap_set_bits() / regmap_clear_bits() for single-direction writes
- Overflow drops all samples; timestamps are unreliable after overflow
- Device remains in shutdown between captures to suppress LED current
- vdd and vled regulators required per datasheet; vref is not a supply
--- drivers/iio/health/Kconfig | 18 ++ drivers/iio/health/Makefile | 1 + drivers/iio/health/max86150.c | 533 ++++++++++++++++++++++++++++++++++ 3 files changed, 552 insertions(+) create mode 100644 drivers/iio/health/max86150.c
drivers/iio/health/Kconfig +18 -0
@@ -62,4 +62,22 @@ 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. + + The driver exposes three IIO channels: two PPG optical channels + (Red and IR LED) for heart rate and SpO2 monitoring, and one + ECG channel for biopotential recording. + + This driver can also be built as a module. If so, the module + will be called max86150. + endmenu
drivers/iio/health/Makefile +1 -0
@@ -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 +533 -0
533 lines not on this page · raw message
Signature · 1 line
2.51.1
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] Unsynchronized threaded IRQ handler causes a data race on `data->fifo_raw` with sysfs raw reads.…
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Unsynchronized threaded IRQ handler causes a data race on data->fifo_raw with sysfs raw reads.
- [High] The threaded interrupt handler can dereference a NULL indio_dev->active_scan_mask during buffer teardown.
- [Medium] Stale MAX86150_INT_PPG_RDY interrupt status causes read_raw to incorrectly read an empty FIFO.
- [Medium] The FIFO available sample calculation evaluates to 0 when the FIFO is perfectly full, dropping valid data. --
--- Patch [2]: [PATCH v7 v7 2/3] iio: health: add MAX86150 ECG and PPG biosensor driver --- commit fed360c474511b667d4da2e81a88fe147d4dc428
Author: Md Shofiqul Islam <[email protected]>
iio: health: add MAX86150 ECG and PPG biosensor driver
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.
Quoted 5 lines
drivers/iio/health/max86150.c new file +0
-0
[ ... ]
drivers/iio/health/max86150.c line 160
Quoted 7 lines
drivers/iio/health/max86150.c new file +7
-0+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 );
[Severity: High]
Can this result in a data race on data->fifo_raw?
When the IIO buffer is disabled, max86150_buffer_predisable() masks the interrupt in hardware but doesn't synchronize the threaded IRQ handler (e.g., via disable_irq()).
If a delayed IRQ handler runs concurrently with a sysfs raw read triggered via max86150_read_raw(), the raw read will successfully claim direct mode (since the core considers the buffer disabled) and both threads will execute max86150_read_one_sample() concurrently.
Quoted 2 lines
drivers/iio/health/max86150.c new file +2
-0+if (ret) +return ret;
[ ... ]
Quoted 30 lines
drivers/iio/health/max86150.c new file +30
-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); +unsigned int ppg_rdy_status; +u32 ppg_red, ppg_ir; +s32 ecg; +int ret; + +switch (mask) { +case IIO_CHAN_INFO_RAW : +if (!iio_device_claim_direct (indio_dev)) +return -EBUSY ; + + ret =regmap_clear_bits (data->regmap,MAX86150_REG_SYS_CTRL , +MAX86150_SYS_CTRL_SHDN ); +if (ret) + goto out_shutdown; + + ret =regmap_write (data->regmap,MAX86150_REG_FIFO_WR_PTR ,0 ); +if (ret) + goto out_shutdown; + ret =regmap_write (data->regmap,MAX86150_REG_OVF_COUNTER ,0 ); +if (ret) + goto out_shutdown; + ret =regmap_write (data->regmap,MAX86150_REG_FIFO_RD_PTR ,0 ); +if (ret) + goto out_shutdown; +
drivers/iio/health/max86150.c line 211
Quoted 10 lines
drivers/iio/health/max86150.c new file +10
-0+/* +* Poll PPG_RDY rather than sleeping a fixed interval -- the +* internal oscillator may start slower than nominal, leaving +* the FIFO empty if we read too early. +*/ + ret =regmap_read_poll_timeout (data->regmap, +MAX86150_REG_INT_STATUS1 , + ppg_rdy_status, + ppg_rdy_status &MAX86150_INT_PPG_RDY , +1000 ,25000 );
[Severity: Medium]
Will this incorrectly succeed immediately if there is a stale interrupt flag?
regmap_read_poll_timeout() performs an immediate read before sleeping. If MAX86150_INT_PPG_RDY is already set from previous activity, it will falsely succeed, bypassing the wait for a new sample.
Because the FIFO pointers were just reset to 0, reading from FIFO_DATA at this point will read an empty FIFO, resulting in corrupted output. Does INT_STATUS1 need to be cleared before polling?
Quoted 2 lines
drivers/iio/health/max86150.c new file +2
-0+if (ret) + goto out_shutdown;
[ ... ]
drivers/iio/health/max86150.c line 284
Quoted 5 lines
drivers/iio/health/max86150.c new file +5
-0+static int max86150_buffer_predisable (struct iio_dev *indio_dev) +{ +struct max86150_data *data =iio_priv (indio_dev); + +regmap_write (data->regmap,MAX86150_REG_INT_ENABLE1 ,0 );
[Severity: High]
Does this leave the threaded interrupt handler unsynchronized during teardown?
Since this only disables the interrupt in hardware without calling disable_irq(), a delayed threaded interrupt handler can still execute after the IIO core has disabled the buffer and freed indio_dev->active_scan_mask.
Quoted 4 lines
drivers/iio/health/max86150.c new file +4
-0+regmap_set_bits (data->regmap,MAX86150_REG_SYS_CTRL , +MAX86150_SYS_CTRL_SHDN ); +return 0 ; +}
[ ... ]
Quoted 28 lines
drivers/iio/health/max86150.c new file +28
-0+static irqreturn_t max86150_interrupt_handler (int irq,void *private) +{ +struct iio_dev *indio_dev = private; +struct max86150_data *data =iio_priv (indio_dev); +unsigned int status, wr_ptr, rd_ptr, ovf; +u32 ppg_red, ppg_ir; +s32 ecg; +s64 ts; +int ret, n_avail, i, j; + + ret =regmap_read (data->regmap,MAX86150_REG_INT_STATUS1 , &status); +if (ret) +return IRQ_HANDLED ; + +if (!(status &MAX86150_INT_A_FULL )) +return IRQ_NONE ; + + ret =regmap_read (data->regmap,MAX86150_REG_OVF_COUNTER , &ovf); +if (ret) +return IRQ_HANDLED ; + +if (ovf >0 ) { +/* FIFO overflowed; timestamps are unreliable - flush and discard */ +regmap_write (data->regmap,MAX86150_REG_FIFO_WR_PTR ,0 ); +regmap_write (data->regmap,MAX86150_REG_OVF_COUNTER ,0 ); +regmap_write (data->regmap,MAX86150_REG_FIFO_RD_PTR ,0 ); +return IRQ_HANDLED ; + }
drivers/iio/health/max86150.c line 332
Quoted 10 lines
drivers/iio/health/max86150.c new file +10
-0+ + 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 ; + + n_avail = (wr_ptr - rd_ptr) & (MAX86150_FIFO_DEPTH -1 ); +if (!n_avail)
[Severity: Medium]
Does this calculation incorrectly evaluate to 0 when the FIFO is perfectly full?
If exactly 32 samples are written by the hardware before the interrupt handler runs, the 5-bit wr_ptr wraps around and equals rd_ptr (but OVF_COUNTER is still 0).
The modulo arithmetic (wr_ptr - rd_ptr) & (MAX86150_FIFO_DEPTH - 1) will evaluate to 0, causing the driver to unconditionally drop all 32 valid samples instead of reading them.
Quoted 9 lines
drivers/iio/health/max86150.c new file +9
-0+return IRQ_HANDLED ; + +/* +* Anchor timestamps to the interrupt time: sample (n_avail - 1) is +* the newest and corresponds to ts; earlier samples are back-calculated +* by one sample_period_ns per step. +*/ + ts =ktime_get_ns (); +
drivers/iio/health/max86150.c line 351
Quoted 10 lines
drivers/iio/health/max86150.c new file +10
-0+for (i =0 ; i < n_avail; i++) { +s64 sample_ts = ts - + (s64)(n_avail -1 - i) * data->sample_period_ns; + + ret =max86150_read_one_sample (data, &ppg_red, &ppg_ir, &ecg); +if (ret) +break ; + + j =0 ; +if (test_bit (MAX86150_IDX_PPG_RED , indio_dev->active_scan_mask))
[Severity: High]
Can this dereference a NULL indio_dev->active_scan_mask?
Because max86150_buffer_predisable() doesn't call disable_irq() to synchronize with the threaded IRQ handler, this handler could execute after the IIO core has already disabled the buffer and freed indio_dev->active_scan_mask, leading to a NULL pointer dereference.
Quoted 3 lines
drivers/iio/health/max86150.c new file +3
-0+ data->buf[j++] = ppg_red; +if (test_bit (MAX86150_IDX_PPG_IR , indio_dev->active_scan_mask)) + data->buf[j++] = ppg_ir;
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
PATCH v7 3/3 MAINTAINERS: add entry for MAX86150 IIO health driver
In reply to Md Shofiqul Islam--- MAINTAINERS | 7 +++++++ 1 file changed, 7 insertions(+)
MAINTAINERS +7 -0
@@ -15863,6 +15863,13 @@ S: Supported F: Documentation/devicetree/bindings/power/supply/maxim,max77976.yaml F: drivers/power/supply/max77976_charger.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 +F: drivers/iio/health/max86150.c + MAXIM MUIC CHARGER DRIVERS FOR EXYNOS BASED BOARDS M: Krzysztof Kozlowski <[email protected]> L: [email protected]
Signature · 1 line
2.51.1
Re: [PATCH v7 v7 3/3] MAINTAINERS: add entry for MAX86150 IIO health driver
Quoted 13 lines · CDT Md Shofiqul Islam
Signed-off-by Md Shofiqul Islam <[email protected]>--- MAINTAINERS | 7 +++++++ 1 file changed, 7 insertions(+)MAINTAINERS +0
-0@@ -15863,6 +15863,13 @@ S: Supported F: Documentation/devicetree/bindings/power/supply/maxim,max77976.yaml F: drivers/power/supply/max77976_charger.c
MAINTAINERS S: Supported · line 15875
Quoted 10 lines
MAINTAINERS +7
-0+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 +F: drivers/iio/health/max86150.c + MAXIM MUIC CHARGER DRIVERS FOR EXYNOS BASED BOARDS M: Krzysztof Kozlowski <[email protected]> L: [email protected]
This should probably be squashed into either the dt-bindings patch or the driver file patch (or both, with just the yaml being put on the entry in the dt-bindings patch and adding the .c file to the entry in the second patch).
Signature · 2 lines
best regards, max