[PATCH v9 2/2] iio: health: add MAX86150 ECG and PPG biosensor driver

[PATCH v9 2/2] iio: health: add MAX86150 ECG and PPG biosensor driver

[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 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
- synchronize_irq() in predisable ensures threaded handler completes
  before the IIO core clears active_scan_mask
- INT_STATUS1 cleared before polling PPG_RDY to avoid stale flag
- 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

Signed-off-by: Md Shofiqul Islam <shofiqtest@gmail.com>
---
 MAINTAINERS                   |   1 +
 drivers/iio/health/Kconfig    |  17 ++
 drivers/iio/health/Makefile   |   1 +
 drivers/iio/health/max86150.c | 560 ++++++++++++++++++++++++++++++++++
 4 files changed, 579 insertions(+)
 create mode 100644 drivers/iio/health/max86150.c

diff --git a/MAINTAINERS b/MAINTAINERS
index e9b9fd619bd86..361a7c8b99ead 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15868,6 +15868,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
 
 MAXIM MUIC CHARGER DRIVERS FOR EXYNOS BASED BOARDS
 M:	Krzysztof Kozlowski <krzk@kernel.org>
diff --git a/drivers/iio/health/Kconfig b/drivers/iio/health/Kconfig
index a89f3abf11f4a..c18d41d5044fa 100644
--- a/drivers/iio/health/Kconfig
+++ b/drivers/iio/health/Kconfig
@@ -62,4 +62,21 @@ 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
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..e8394031f3acb
--- /dev/null
+++ b/drivers/iio/health/max86150.c
@@ -0,0 +1,560 @@
+// 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/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/timekeeping.h>
+#include <linux/unaligned.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/kfifo_buf.h>
+
+/* Register addresses */
+#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
+
+/* INT_STATUS1 / INT_ENABLE1 */
+#define MAX86150_INT_A_FULL		BIT(7)
+#define MAX86150_INT_PPG_RDY		BIT(6)
+
+/* SYS_CTRL */
+#define MAX86150_SYS_CTRL_SHDN		BIT(1)
+#define MAX86150_SYS_CTRL_RESET		BIT(0)
+
+/* FIFO_CONFIG */
+#define MAX86150_FIFO_CONFIG_SMP_AVE_MASK	GENMASK(7, 5)
+#define MAX86150_FIFO_CONFIG_ROLLOVER_EN	BIT(4)
+#define MAX86150_FIFO_CONFIG_A_FULL_MASK	GENMASK(3, 0)
+
+/* FIFO slot data-type codes */
+#define MAX86150_FD_NONE		0x0
+#define MAX86150_FD_LED1		0x1
+#define MAX86150_FD_LED2		0x2
+#define MAX86150_FD_ECG			0x9
+
+/* FIFO_DCTRL1 / FIFO_DCTRL2 */
+#define MAX86150_FIFO_DCTRL_FD_LO_MASK	GENMASK(3, 0)
+#define MAX86150_FIFO_DCTRL_FD_HI_MASK	GENMASK(7, 4)
+
+/* PPG_CONFIG1 */
+#define MAX86150_PPG_CONFIG1_ADC_RGE_MASK	GENMASK(7, 6)
+#define MAX86150_PPG_CONFIG1_SR_MASK		GENMASK(5, 1)
+
+#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)
+
+/* Fire A_FULL when 17 slots are available (32 - 15 = 17) */
+#define MAX86150_FIFO_A_FULL_VAL	15
+
+#define MAX86150_LED_PA_50MA		0x3F
+#define MAX86150_PPG_SR_100HZ		4
+#define MAX86150_PPG_ADC_RGE_16384	2
+
+enum max86150_scan_idx {
+	MAX86150_IDX_PPG_RED,
+	MAX86150_IDX_PPG_IR,
+	MAX86150_IDX_ECG,
+	MAX86150_IDX_TS,
+};
+
+struct max86150_data {
+	struct regmap *regmap;
+	int irq;
+	u32 sample_period_ns;
+	u8 fifo_raw[ALIGN(MAX86150_SAMPLE_BYTES, IIO_DMA_MINALIGN)]
+		__aligned(IIO_DMA_MINALIGN);
+	IIO_DECLARE_BUFFER_WITH_TS(s32, buf, 3);
+};
+
+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 = 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);
+
+	return 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;
+
+		/*
+		 * 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.
+		 */
+		regmap_read(data->regmap, MAX86150_REG_INT_STATUS1,
+			    &ppg_rdy_status);
+
+		/*
+		 * 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);
+		if (ret)
+			goto out_shutdown;
+
+		ret = max86150_read_one_sample(data, &ppg_red, &ppg_ir, &ecg);
+
+out_shutdown:
+		regmap_set_bits(data->regmap, MAX86150_REG_SYS_CTRL,
+				MAX86150_SYS_CTRL_SHDN);
+		iio_device_release_direct(indio_dev);
+
+		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,
+};
+
+static int max86150_buffer_postenable(struct iio_dev *indio_dev)
+{
+	struct max86150_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = regmap_clear_bits(data->regmap, MAX86150_REG_SYS_CTRL,
+				MAX86150_SYS_CTRL_SHDN);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(data->regmap, MAX86150_REG_FIFO_WR_PTR, 0);
+	if (ret)
+		goto err_shutdown;
+	ret = regmap_write(data->regmap, MAX86150_REG_OVF_COUNTER, 0);
+	if (ret)
+		goto err_shutdown;
+	ret = regmap_write(data->regmap, MAX86150_REG_FIFO_RD_PTR, 0);
+	if (ret)
+		goto err_shutdown;
+	ret = regmap_write(data->regmap, MAX86150_REG_INT_ENABLE1,
+			   MAX86150_INT_A_FULL);
+	if (ret)
+		goto err_shutdown;
+	return 0;
+
+err_shutdown:
+	regmap_set_bits(data->regmap, MAX86150_REG_SYS_CTRL,
+			MAX86150_SYS_CTRL_SHDN);
+	return ret;
+}
+
+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);
+	regmap_set_bits(data->regmap, MAX86150_REG_SYS_CTRL,
+			MAX86150_SYS_CTRL_SHDN);
+	/*
+	 * Mask the hardware interrupt first, then synchronize to ensure any
+	 * threaded handler already in flight completes before the IIO core
+	 * clears active_scan_mask; without this a delayed handler would
+	 * dereference a NULL active_scan_mask.
+	 */
+	synchronize_irq(data->irq);
+	return 0;
+}
+
+static const struct iio_buffer_setup_ops max86150_buffer_setup_ops = {
+	.postenable = max86150_buffer_postenable,
+	.predisable = max86150_buffer_predisable,
+};
+
+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, n_avail;
+	u32 ppg_red, ppg_ir;
+	s32 ecg;
+	s64 ts;
+	unsigned int i, j;
+	int ret;
+
+	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;
+	}
+
+	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);
+	/*
+	 * When the FIFO holds exactly MAX86150_FIFO_DEPTH samples the
+	 * write pointer wraps around to equal the read pointer even though
+	 * OVF_COUNTER is still zero.  The A_FULL status bit disambiguates
+	 * this wrap-around from a genuinely empty FIFO.
+	 */
+	if (!n_avail && (status & MAX86150_INT_A_FULL))
+		n_avail = MAX86150_FIFO_DEPTH;
+	if (!n_avail)
+		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();
+
+	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))
+			data->buf[j++] = ppg_red;
+		if (test_bit(MAX86150_IDX_PPG_IR, indio_dev->active_scan_mask))
+			data->buf[j++] = ppg_ir;
+		if (test_bit(MAX86150_IDX_ECG, indio_dev->active_scan_mask))
+			data->buf[j++] = ecg;
+
+		iio_push_to_buffers_with_ts(indio_dev, data->buf,
+					    sizeof(data->buf), sample_ts);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static void max86150_powerdown(void *arg)
+{
+	struct max86150_data *data = arg;
+
+	regmap_write(data->regmap, MAX86150_REG_INT_ENABLE1, 0);
+	regmap_set_bits(data->regmap, MAX86150_REG_SYS_CTRL,
+			MAX86150_SYS_CTRL_SHDN);
+}
+
+static int max86150_chip_init(struct max86150_data *data)
+{
+	int ret;
+
+	/* Software reset; the bit self-clears within 1 ms */
+	ret = regmap_write(data->regmap, MAX86150_REG_SYS_CTRL,
+			   MAX86150_SYS_CTRL_RESET);
+	if (ret)
+		return ret;
+	fsleep(1000);
+
+	ret = regmap_write(data->regmap, MAX86150_REG_FIFO_CONFIG,
+			   MAX86150_FIFO_CONFIG_ROLLOVER_EN |
+			   FIELD_PREP(MAX86150_FIFO_CONFIG_A_FULL_MASK,
+				      MAX86150_FIFO_A_FULL_VAL));
+	if (ret)
+		return ret;
+
+	/* Slot 1 = PPG Red (LED1), Slot 2 = PPG IR (LED2) */
+	ret = regmap_write(data->regmap, MAX86150_REG_FIFO_DCTRL1,
+			   FIELD_PREP(MAX86150_FIFO_DCTRL_FD_LO_MASK,
+				      MAX86150_FD_LED1) |
+			   FIELD_PREP(MAX86150_FIFO_DCTRL_FD_HI_MASK,
+				      MAX86150_FD_LED2));
+	if (ret)
+		return ret;
+
+	/* Slot 3 = ECG, Slot 4 = disabled */
+	ret = regmap_write(data->regmap, MAX86150_REG_FIFO_DCTRL2,
+			   FIELD_PREP(MAX86150_FIFO_DCTRL_FD_LO_MASK,
+				      MAX86150_FD_ECG) |
+			   FIELD_PREP(MAX86150_FIFO_DCTRL_FD_HI_MASK,
+				      MAX86150_FD_NONE));
+	if (ret)
+		return ret;
+
+	/* PPG: 100 Hz sample rate, 16384 nA ADC full-scale range */
+	ret = regmap_write(data->regmap, MAX86150_REG_PPG_CONFIG1,
+			   FIELD_PREP(MAX86150_PPG_CONFIG1_ADC_RGE_MASK,
+				      MAX86150_PPG_ADC_RGE_16384) |
+			   FIELD_PREP(MAX86150_PPG_CONFIG1_SR_MASK,
+				      MAX86150_PPG_SR_100HZ));
+	if (ret)
+		return ret;
+
+	ret = regmap_write(data->regmap, MAX86150_REG_LED1_PA, MAX86150_LED_PA_50MA);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(data->regmap, MAX86150_REG_LED2_PA, MAX86150_LED_PA_50MA);
+	if (ret)
+		return ret;
+
+	data->sample_period_ns = 10 * NSEC_PER_MSEC;
+
+	return regmap_write(data->regmap, MAX86150_REG_SYS_CTRL,
+			    MAX86150_SYS_CTRL_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->irq = client->irq;
+
+	ret = devm_regulator_get_enable(dev, "vdd");
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to get/enable vdd supply\n");
+
+	ret = devm_regulator_get_enable(dev, "vled");
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to get/enable vled 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 initialise 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)
+		dev_warn(dev, "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) {
+		unsigned long irq_trig = irq_get_trigger_type(client->irq);
+
+		ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
+						  &max86150_buffer_setup_ops);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Cannot setup kfifo buffer\n");
+
+		ret = devm_request_threaded_irq(dev, client->irq,
+						NULL,
+						max86150_interrupt_handler,
+						irq_trig | IRQF_ONESHOT,
+						"max86150", indio_dev);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Cannot request IRQ %d\n",
+					     client->irq);
+	}
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static const struct i2c_device_id max86150_id[] = {
+	{ .name = "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

Re: [PATCH v9 2/2] iio: health: add MAX86150 ECG and PPG biosensor driver

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Stale A_FULL interrupt status bit is not cleared in buffer_postenable, leading to immediate spurious interrupts and data corruption.
- [High] The threaded interrupt handler can execute while the IIO buffer is disabled, leading to a fatal NULL pointer dereference on active_scan_mask.
- [Medium] The driver bypasses the IIO core's clock selection mechanism by using ktime_get_ns() instead of iio_get_time_ns() for soft timestamps.
- [Low] Stale sensor data is leaked to user-space via untouched padding elements in the local data->buf array.
--

--- Patch [2]: [PATCH v9 2/2] iio: health: add MAX86150 ECG and PPG biosensor driver ---
commit 9f8edea36d720ecd87ad044648a96413f3156ea3
Author: Md Shofiqul Islam <shofiqtest@gmail.com>

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.

> diff --git a/drivers/iio/health/max86150.c b/drivers/iio/health/max86150.c
> new file mode 100644
> index 0000000000000..e8394031f3acb
> --- /dev/null
> +++ b/drivers/iio/health/max86150.c

[ ... ]

> +static int max86150_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct max86150_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = regmap_clear_bits(data->regmap, MAX86150_REG_SYS_CTRL,
> +				MAX86150_SYS_CTRL_SHDN);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(data->regmap, MAX86150_REG_FIFO_WR_PTR, 0);
> +	if (ret)
> +		goto err_shutdown;
> +	ret = regmap_write(data->regmap, MAX86150_REG_OVF_COUNTER, 0);
> +	if (ret)
> +		goto err_shutdown;
> +	ret = regmap_write(data->regmap, MAX86150_REG_FIFO_RD_PTR, 0);
> +	if (ret)
> +		goto err_shutdown;
> +	ret = regmap_write(data->regmap, MAX86150_REG_INT_ENABLE1,
> +			   MAX86150_INT_A_FULL);

[Severity: High]
Does this unmask the interrupt without first clearing the stale A_FULL
status in MAX86150_REG_INT_STATUS1?

If the FIFO was full when the buffer was previously stopped, the A_FULL
bit remains latched. When the interrupt is unmasked here with
zero-initialized FIFO pointers, couldn't the latched bit immediately
trigger the handler, causing it to read and push 32 samples of garbage?

> +	if (ret)
> +		goto err_shutdown;
> +	return 0;
> +
> +err_shutdown:
> +	regmap_set_bits(data->regmap, MAX86150_REG_SYS_CTRL,
> +			MAX86150_SYS_CTRL_SHDN);
> +	return ret;
> +}
> +
> +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);
> +	regmap_set_bits(data->regmap, MAX86150_REG_SYS_CTRL,
> +			MAX86150_SYS_CTRL_SHDN);
> +	/*
> +	 * Mask the hardware interrupt first, then synchronize to ensure any
> +	 * threaded handler already in flight completes before the IIO core
> +	 * clears active_scan_mask; without this a delayed handler would
> +	 * dereference a NULL active_scan_mask.
> +	 */
> +	synchronize_irq(data->irq);

[Severity: High]
Is synchronize_irq() sufficient to prevent the handler from running after
the buffer is disabled?

If a spurious interrupt fires while the buffer is disabled, the OS-level
IRQ is still active since disable_irq() wasn't called. Wouldn't the
threaded handler run and unconditionally dereference
indio_dev->active_scan_mask (which the IIO core clears to NULL after
predisable), causing a fatal crash?

> +	return 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, n_avail;

[ ... ]

> +	/*
> +	 * 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();

[Severity: Medium]
Should this use iio_get_time_ns(indio_dev) instead of ktime_get_ns()?

Using ktime_get_ns() unconditionally generates timestamps from
CLOCK_MONOTONIC, which ignores the user's selected IIO clock source
(such as REALTIME or BOOTTIME) configured via sysfs.

> +
> +	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))
> +			data->buf[j++] = ppg_red;
> +		if (test_bit(MAX86150_IDX_PPG_IR, indio_dev->active_scan_mask))
> +			data->buf[j++] = ppg_ir;
> +		if (test_bit(MAX86150_IDX_ECG, indio_dev->active_scan_mask))
> +			data->buf[j++] = ecg;
> +
> +		iio_push_to_buffers_with_ts(indio_dev, data->buf,
> +					    sizeof(data->buf), sample_ts);

[Severity: Low]
Does pushing sizeof(data->buf) leak stale sensor data in untouched padding
elements?

If a user enables 3 channels, reads data, then disables the buffer and
restarts it with only 1 channel enabled (e.g., ECG), data->buf is reused.
The IIO core pushes the ECG sample and uses the untouched data->buf[1]
element as padding to align the timestamp, leaking the stale PPG data
from the previous 3-channel scan.

> +	}
> +
> +	return IRQ_HANDLED;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707112714.2261727-1-shofiqtest@gmail.com?part=2

Re: [PATCH v9 2/2] iio: health: add MAX86150 ECG and PPG biosensor driver

[Joshua Crofts]

On Tue,  7 Jul 2026 14:27:14 +0300
Md Shofiqul Islam <shofiqtest@gmail.com> wrote:

> 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.  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
> - synchronize_irq() in predisable ensures threaded handler completes
>   before the IIO core clears active_scan_mask
> - INT_STATUS1 cleared before polling PPG_RDY to avoid stale flag
> - 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
> 
> Signed-off-by: Md Shofiqul Islam <shofiqtest@gmail.com>

Hi, I took a quick look at this driver - surely I've missed something
and there are a few Analog folk in IIO which will probably know more.

Additionally, Sashiko had some remarks, please check them out.

https://sashiko.dev/#/patchset/20260707104234.1957104-1-shofiqtest%40gmail.com

Several comments inline.

> ---
>  MAINTAINERS                   |   1 +
>  drivers/iio/health/Kconfig    |  17 ++
>  drivers/iio/health/Makefile   |   1 +
>  drivers/iio/health/max86150.c | 560 ++++++++++++++++++++++++++++++++++
>  4 files changed, 579 insertions(+)
>  create mode 100644 drivers/iio/health/max86150.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e9b9fd619bd86..361a7c8b99ead 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15868,6 +15868,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
>  
>  MAXIM MUIC CHARGER DRIVERS FOR EXYNOS BASED BOARDS
>  M:	Krzysztof Kozlowski <krzk@kernel.org>
> diff --git a/drivers/iio/health/Kconfig b/drivers/iio/health/Kconfig
> index a89f3abf11f4a..c18d41d5044fa 100644
> --- a/drivers/iio/health/Kconfig
> +++ b/drivers/iio/health/Kconfig
> @@ -62,4 +62,21 @@ 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.

Delete the above paragraph. You don't describe the code in the Kconfig.

> +
> +	  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..e8394031f3acb
> --- /dev/null
> +++ b/drivers/iio/health/max86150.c
> @@ -0,0 +1,560 @@
> +// 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
> + */
> +

You're missing array_size.h, err.h, types.h

> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/timekeeping.h>
> +#include <linux/unaligned.h>

Blank line here.

> +#include <linux/iio/buffer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/kfifo_buf.h>
> +
> +/* Register addresses */
> +#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
> +
> +/* INT_STATUS1 / INT_ENABLE1 */
> +#define MAX86150_INT_A_FULL		BIT(7)
> +#define MAX86150_INT_PPG_RDY		BIT(6)
> +
> +/* SYS_CTRL */
> +#define MAX86150_SYS_CTRL_SHDN		BIT(1)
> +#define MAX86150_SYS_CTRL_RESET		BIT(0)
> +
> +/* FIFO_CONFIG */
> +#define MAX86150_FIFO_CONFIG_SMP_AVE_MASK	GENMASK(7, 5)
> +#define MAX86150_FIFO_CONFIG_ROLLOVER_EN	BIT(4)
> +#define MAX86150_FIFO_CONFIG_A_FULL_MASK	GENMASK(3, 0)
> +
> +/* FIFO slot data-type codes */
> +#define MAX86150_FD_NONE		0x0
> +#define MAX86150_FD_LED1		0x1
> +#define MAX86150_FD_LED2		0x2
> +#define MAX86150_FD_ECG			0x9
> +
> +/* FIFO_DCTRL1 / FIFO_DCTRL2 */
> +#define MAX86150_FIFO_DCTRL_FD_LO_MASK	GENMASK(3, 0)
> +#define MAX86150_FIFO_DCTRL_FD_HI_MASK	GENMASK(7, 4)
> +
> +/* PPG_CONFIG1 */
> +#define MAX86150_PPG_CONFIG1_ADC_RGE_MASK	GENMASK(7, 6)
> +#define MAX86150_PPG_CONFIG1_SR_MASK		GENMASK(5, 1)
> +
> +#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)
> +
> +/* Fire A_FULL when 17 slots are available (32 - 15 = 17) */
> +#define MAX86150_FIFO_A_FULL_VAL	15
> +
> +#define MAX86150_LED_PA_50MA		0x3F
> +#define MAX86150_PPG_SR_100HZ		4
> +#define MAX86150_PPG_ADC_RGE_16384	2
> +
> +enum max86150_scan_idx {
> +	MAX86150_IDX_PPG_RED,
> +	MAX86150_IDX_PPG_IR,
> +	MAX86150_IDX_ECG,
> +	MAX86150_IDX_TS,
> +};
> +
> +struct max86150_data {
> +	struct regmap *regmap;
> +	int irq;
> +	u32 sample_period_ns;
> +	u8 fifo_raw[ALIGN(MAX86150_SAMPLE_BYTES, IIO_DMA_MINALIGN)]
> +		__aligned(IIO_DMA_MINALIGN);
> +	IIO_DECLARE_BUFFER_WITH_TS(s32, buf, 3);
> +};

Have you checked this struct layout with pahole?

> +
> +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,
> +		},
> +	},

...

> +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;

Blank line.

> +		ret = regmap_write(data->regmap, MAX86150_REG_OVF_COUNTER, 0);
> +		if (ret)
> +			goto out_shutdown;

Blank line.

> +		ret = regmap_write(data->regmap, MAX86150_REG_FIFO_RD_PTR, 0);
> +		if (ret)
> +			goto out_shutdown;
> +
> +		/*
> +		 * 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.
> +		 */
> +		regmap_read(data->regmap, MAX86150_REG_INT_STATUS1,
> +			    &ppg_rdy_status);

You're not checking the return value of regmap_read.

> +
> +		/*
> +		 * 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);
> +		if (ret)
> +			goto out_shutdown;
> +
> +		ret = max86150_read_one_sample(data, &ppg_red, &ppg_ir, &ecg);
> +
> +out_shutdown:
> +		regmap_set_bits(data->regmap, MAX86150_REG_SYS_CTRL,
> +				MAX86150_SYS_CTRL_SHDN);
> +		iio_device_release_direct(indio_dev);
> +
> +		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,
> +};
> +
> +static int max86150_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct max86150_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = regmap_clear_bits(data->regmap, MAX86150_REG_SYS_CTRL,
> +				MAX86150_SYS_CTRL_SHDN);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(data->regmap, MAX86150_REG_FIFO_WR_PTR, 0);
> +	if (ret)
> +		goto err_shutdown;

Blank line.

> +	ret = regmap_write(data->regmap, MAX86150_REG_OVF_COUNTER, 0);
> +	if (ret)
> +		goto err_shutdown;

Blank line.

> +	ret = regmap_write(data->regmap, MAX86150_REG_FIFO_RD_PTR, 0);
> +	if (ret)
> +		goto err_shutdown;

Blank line.

> +	ret = regmap_write(data->regmap, MAX86150_REG_INT_ENABLE1,
> +			   MAX86150_INT_A_FULL);
> +	if (ret)
> +		goto err_shutdown;
> +	return 0;
> +
> +err_shutdown:
> +	regmap_set_bits(data->regmap, MAX86150_REG_SYS_CTRL,
> +			MAX86150_SYS_CTRL_SHDN);

Not checking the return value of regmap_set_bits either.

> +	return ret;
> +}
> +
> +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);
> +	regmap_set_bits(data->regmap, MAX86150_REG_SYS_CTRL,
> +			MAX86150_SYS_CTRL_SHDN);
> +	/*
> +	 * Mask the hardware interrupt first, then synchronize to ensure any
> +	 * threaded handler already in flight completes before the IIO core
> +	 * clears active_scan_mask; without this a delayed handler would
> +	 * dereference a NULL active_scan_mask.
> +	 */
> +	synchronize_irq(data->irq);
> +	return 0;
> +}
> +
> +static const struct iio_buffer_setup_ops max86150_buffer_setup_ops = {
> +	.postenable = max86150_buffer_postenable,
> +	.predisable = max86150_buffer_predisable,
> +};
> +
> +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, n_avail;
> +	u32 ppg_red, ppg_ir;
> +	s32 ecg;
> +	s64 ts;
> +	unsigned int i, j;

You can remove this line, see below.

> +	int ret;
> +
> +	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);

Please check the return value of all regmap functions you call.

> +		regmap_write(data->regmap, MAX86150_REG_OVF_COUNTER, 0);
> +		regmap_write(data->regmap, MAX86150_REG_FIFO_RD_PTR, 0);
> +		return IRQ_HANDLED;
> +	}
> +
> +	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);
> +	/*
> +	 * When the FIFO holds exactly MAX86150_FIFO_DEPTH samples the
> +	 * write pointer wraps around to equal the read pointer even though
> +	 * OVF_COUNTER is still zero.  The A_FULL status bit disambiguates
> +	 * this wrap-around from a genuinely empty FIFO.
> +	 */
> +	if (!n_avail && (status & MAX86150_INT_A_FULL))
> +		n_avail = MAX86150_FIFO_DEPTH;
> +	if (!n_avail)
> +		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();
> +
> +	for (i = 0; i < n_avail; i++) {

for (unsigned int i = 0; ...)

> +		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;

What is the point of having j if we always know the buffer size is 3?
Can't you just do buf[0], buf[1] and buf[2] instead of incrementing j
after a write? Seems awfully redundant.

> +		if (test_bit(MAX86150_IDX_PPG_RED, indio_dev->active_scan_mask))
> +			data->buf[j++] = ppg_red;
> +		if (test_bit(MAX86150_IDX_PPG_IR, indio_dev->active_scan_mask))
> +			data->buf[j++] = ppg_ir;
> +		if (test_bit(MAX86150_IDX_ECG, indio_dev->active_scan_mask))
> +			data->buf[j++] = ecg;
> +
> +		iio_push_to_buffers_with_ts(indio_dev, data->buf,
> +					    sizeof(data->buf), sample_ts);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void max86150_powerdown(void *arg)
> +{
> +	struct max86150_data *data = arg;
> +
> +	regmap_write(data->regmap, MAX86150_REG_INT_ENABLE1, 0);
> +	regmap_set_bits(data->regmap, MAX86150_REG_SYS_CTRL,
> +			MAX86150_SYS_CTRL_SHDN);

Also not checking return values here.

> +}
> +
> +static int max86150_chip_init(struct max86150_data *data)
> +{
> +	int ret;
> +
> +	/* Software reset; the bit self-clears within 1 ms */
> +	ret = regmap_write(data->regmap, MAX86150_REG_SYS_CTRL,
> +			   MAX86150_SYS_CTRL_RESET);
> +	if (ret)
> +		return ret;

Blank line.

> +	fsleep(1000);

Why are we sleeping for 1000? Add a comment why this value exactly.
(Datasheet reference etc.)

> +
> +	ret = regmap_write(data->regmap, MAX86150_REG_FIFO_CONFIG,
> +			   MAX86150_FIFO_CONFIG_ROLLOVER_EN |
> +			   FIELD_PREP(MAX86150_FIFO_CONFIG_A_FULL_MASK,
> +				      MAX86150_FIFO_A_FULL_VAL));
> +	if (ret)
> +		return ret;
> +
> +	/* Slot 1 = PPG Red (LED1), Slot 2 = PPG IR (LED2) */
> +	ret = regmap_write(data->regmap, MAX86150_REG_FIFO_DCTRL1,
> +			   FIELD_PREP(MAX86150_FIFO_DCTRL_FD_LO_MASK,
> +				      MAX86150_FD_LED1) |
> +			   FIELD_PREP(MAX86150_FIFO_DCTRL_FD_HI_MASK,
> +				      MAX86150_FD_LED2));
> +	if (ret)
> +		return ret;
> +
> +	/* Slot 3 = ECG, Slot 4 = disabled */
> +	ret = regmap_write(data->regmap, MAX86150_REG_FIFO_DCTRL2,
> +			   FIELD_PREP(MAX86150_FIFO_DCTRL_FD_LO_MASK,
> +				      MAX86150_FD_ECG) |
> +			   FIELD_PREP(MAX86150_FIFO_DCTRL_FD_HI_MASK,
> +				      MAX86150_FD_NONE));
> +	if (ret)
> +		return ret;
> +
> +	/* PPG: 100 Hz sample rate, 16384 nA ADC full-scale range */
> +	ret = regmap_write(data->regmap, MAX86150_REG_PPG_CONFIG1,
> +			   FIELD_PREP(MAX86150_PPG_CONFIG1_ADC_RGE_MASK,
> +				      MAX86150_PPG_ADC_RGE_16384) |
> +			   FIELD_PREP(MAX86150_PPG_CONFIG1_SR_MASK,
> +				      MAX86150_PPG_SR_100HZ));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(data->regmap, MAX86150_REG_LED1_PA, MAX86150_LED_PA_50MA);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(data->regmap, MAX86150_REG_LED2_PA, MAX86150_LED_PA_50MA);
> +	if (ret)
> +		return ret;
> +
> +	data->sample_period_ns = 10 * NSEC_PER_MSEC;
> +
> +	return regmap_write(data->regmap, MAX86150_REG_SYS_CTRL,
> +			    MAX86150_SYS_CTRL_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->irq = client->irq;
> +
> +	ret = devm_regulator_get_enable(dev, "vdd");
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to get/enable vdd supply\n");

Just keep enable.

> +
> +	ret = devm_regulator_get_enable(dev, "vled");
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to get/enable vled supply\n");

Same as above.

> +
> +	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 initialise 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)
> +		dev_warn(dev, "Unexpected part ID 0x%02x (expected 0x%02x)\n",
> +			 part_id, MAX86150_PART_ID_VAL);

dev_info() instead of dev_warn() is better.

> +
> +	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) {
> +		unsigned long irq_trig = irq_get_trigger_type(client->irq);
> +
> +		ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
> +						  &max86150_buffer_setup_ops);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Cannot setup kfifo buffer\n");
> +
> +		ret = devm_request_threaded_irq(dev, client->irq,
> +						NULL,
> +						max86150_interrupt_handler,
> +						irq_trig | IRQF_ONESHOT,
> +						"max86150", indio_dev);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Cannot request IRQ %d\n",
> +					     client->irq);

Just return ret instead of dev_err_probe() - it's already called in
devm_request_threaded_irq() on failure.

> +	}
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +static const struct i2c_device_id max86150_id[] = {
> +	{ .name = "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");


-- 
Kind regards

CJD

Re: [PATCH v9 2/2] iio: health: add MAX86150 ECG and PPG biosensor driver

[Andy Shevchenko]

On Tue, Jul 07, 2026 at 02:05:45PM +0200, Joshua Crofts wrote:
> On Tue,  7 Jul 2026 14:27:14 +0300
> Md Shofiqul Islam <shofiqtest@gmail.com> wrote:

...

> You're missing array_size.h, err.h, types.h

My comment against types.h was ignored. I think that this contribution is
heavily assisted by AI (which has to be mentioned), otherwise I can not
explain such an ignorance and mistakes from a human.

> > +#include <linux/bitfield.h>
> > +#include <linux/bitops.h>
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/timekeeping.h>
> > +#include <linux/unaligned.h>
> 
> Blank line here.
> 
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/kfifo_buf.h>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v9 2/2] iio: health: add MAX86150 ECG and PPG biosensor driver

[Md Shofiqul Islam]

Hi Andy,

I want to address your concern directly and honestly.

I did use AI tools during this development, primarily to understand
kernel conventions and check my code against subsystem patterns. The
implementation decisions and the responsibility for what I submitted
are mine. I should have disclosed this from the start, and I did not.
I apologize for that.

Regarding types.h: Your feedback was not ignored. As a new contributor
managing feedback from multiple reviewers across nine versions in
overlapping threads, I lost track of your comment from v4. When
Sashiko flagged HIGH severity issues, I panicked and sent new versions
too quickly without properly reading all outstanding comments. That
was a mistake in my process and entirely my responsibility.

I am committed to contributing properly to this community. All
outstanding comments, including types.h, array_size.h, and err.h, are
addressed in v10. I will not send another version until I've read and
addressed every open comment from every reviewer.

Thank you for the direct feedback. I understand why you raised it.

Regards,
Md Shofiqul Islam


On Tue, Jul 7, 2026 at 6:15 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Tue, Jul 07, 2026 at 02:05:45PM +0200, Joshua Crofts wrote:
> > On Tue,  7 Jul 2026 14:27:14 +0300
> > Md Shofiqul Islam <shofiqtest@gmail.com> wrote:
>
> ...
>
> > You're missing array_size.h, err.h, types.h
>
> My comment against types.h was ignored. I think that this contribution is
> heavily assisted by AI (which has to be mentioned), otherwise I can not
> explain such an ignorance and mistakes from a human.
>
> > > +#include <linux/bitfield.h>
> > > +#include <linux/bitops.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/irq.h>
> > > +#include <linux/module.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/regulator/consumer.h>
> > > +#include <linux/timekeeping.h>
> > > +#include <linux/unaligned.h>
> >
> > Blank line here.
> >
> > > +#include <linux/iio/buffer.h>
> > > +#include <linux/iio/iio.h>
> > > +#include <linux/iio/kfifo_buf.h>
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v9 2/2] iio: health: add MAX86150 ECG and PPG biosensor driver

[Andy Shevchenko]

On Wed, Jul 08, 2026 at 03:54:19PM +0300, Md Shofiqul Islam wrote:
> Hi Andy,
> 
> I want to address your concern directly and honestly.
> 
> I did use AI tools during this development, primarily to understand
> kernel conventions and check my code against subsystem patterns. The
> implementation decisions and the responsibility for what I submitted
> are mine. I should have disclosed this from the start, and I did not.
> I apologize for that.

Yes, we have a tag for that Assisted-by.

> Regarding types.h: Your feedback was not ignored. As a new contributor
> managing feedback from multiple reviewers across nine versions in
> overlapping threads, I lost track of your comment from v4. When
> Sashiko flagged HIGH severity issues, I panicked and sent new versions
> too quickly without properly reading all outstanding comments. That
> was a mistake in my process and entirely my responsibility.
> 
> I am committed to contributing properly to this community. All
> outstanding comments, including types.h, array_size.h, and err.h, are
> addressed in v10. I will not send another version until I've read and
> addressed every open comment from every reviewer.
> 
> Thank you for the direct feedback. I understand why you raised it.

Thank you for following the suggestions!

P.S. Please, do not top-post.

> On Tue, Jul 7, 2026 at 6:15 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Tue, Jul 07, 2026 at 02:05:45PM +0200, Joshua Crofts wrote:
> > > On Tue,  7 Jul 2026 14:27:14 +0300
> > > Md Shofiqul Islam <shofiqtest@gmail.com> wrote:

...

> > > You're missing array_size.h, err.h, types.h
> >
> > My comment against types.h was ignored. I think that this contribution is
> > heavily assisted by AI (which has to be mentioned), otherwise I can not
> > explain such an ignorance and mistakes from a human.

-- 
With Best Regards,
Andy Shevchenko