[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

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

[Jonathan Cameron]

On Tue, 7 Jul 2026 14:05:45 +0200
Joshua Crofts <joshua.crofts1@gmail.com> wrote:

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

I note that ADI had this as 'last time buy'.  I don't mind supporting a
device if it is in use but I thought I'd just call that out.

> 
> 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.

You caught a lot :)

> 
> Additionally, Sashiko had some remarks, please check them out.
> 
> https://sashiko.dev/#/patchset/20260707104234.1957104-1-shofiqtest%40gmail.com
> 
> Several comments inline.

I added a few things on top. Reduces duplication seeing as Joshua had enough
comments scattered through that they didn't crop out anything.
> 
> > ---
> >  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 */

Given well named defines (which they are) the comment doesn't add
value. So drop it. Just keep ones that add something less obvious.

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

Put that value inline.  It isn't a hardware provided choice but rather the one
you've decided to use, so I'd rather see the value + comment at the the place
where the field is written.

> > +
> > +#define MAX86150_LED_PA_50MA		0x3F
As below.
> > +#define MAX86150_PPG_SR_100HZ		4

Even though only this value is used, I'd be tempted to list all of them.  That would
also make the naming include that this is the single pulse per second version of
100 Hz. There is also a double pulse value.  Similar applies to other field values.
Having just one defined makes them look very random whereas, with the full set we 
can see that a value out the middle has been chosen.

> > +#define MAX86150_PPG_ADC_RGE_16384	2
> > +
Similar for this one.

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

I2C device, why do you need an aligned buffer?  Note that Sashiko
regularly gives wrong advice on this. For I2C we have to opt in
if we don't want to bounce buffer. Given I2C is slow we rarely bother.

> > +	IIO_DECLARE_BUFFER_WITH_TS(s32, buf, 3);
> > +};  
> 
> Have you checked this struct layout with pahole?


> 
> > +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;
Can use 
	case IIO_CHAN_INFO_RAW: {
		IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
		if (IIO_DEV_ACQUIRE_DIRECT_MODE(claim))
			return -EBUSY;

		ret = regmap_clear_bits(data->regmap, MAX86150_REG_SYS_CTRL,
					MAX86150_SYS_CTRL_SHDN);
		if (ret)
			return ret;

		ret = do_max86150_read_raw(data); // see below for a similar helper suggestion
		if (ret) {
			regmap_set_bits(data->regmap, MAX86150_REG_SYS_CTRL,
					MAX86150_SYS_CTRL_SHDN);
			return ret;
		}

		ret = regmap_set_bits(data->regmap, MAX86150_REG_SYS_CTRL,
				      MAX86150_SYS_CTRL_SHDN);
		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;
	} //claim on direct mode will be released here, but not a problem to hold it over setting val.

> > +		ret = regmap_clear_bits(data->regmap, MAX86150_REG_SYS_CTRL,
> > +					MAX86150_SYS_CTRL_SHDN);
> > +		if (ret)
> > +			goto out_shutdown;

Why?  you failed to turn it on.

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

For these stacks where there is only one thing to do, it may
make more sense to use a helper.  Typically called something like:
do_max86150_buffer_postenable() 
Then you get this function as something like:

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

In an error path I'd not bother - best you can do is say
oh look a second read failed. (I agree with this feedback
in lots of other places!)

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 = do_max86150_buffer_postenable(data);
	if (ret) {
		regmap_set_bits(data->regmap, MAX86150_REG_SYS_CTRL,
				MAX86150_SYS_CTRL_SHDN);
		return ret;
	}
	
	return 0;
}

> 
> > +	return ret;
> > +}


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

FWIW there isn't a lot of point in a devm callback. Mostly you can
do is print an error.  Everything is going down anyway so we tend
not to care that much about regmap errors.

> 
> > +}
> > +
> > +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));
See above. Bring the 15 down here.  Maybe it's worth a macro to convert
from what people actually want - which is how many entries are in the
fifo when the interrupt fires, to the value that goes in the field.
If so that would be something like
			   FIELD_PREP(MAX86150_FIFO_CONFIG_A_FULL_MASK,
				      max86150_fifo_thresh_to_fieldval(17));
> > +	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;

Which register write is this associated with?  It's nothing to do with the ones
immediate above or below this line so seems odd to have it here.

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

No need to go through this dance.  The IRQ core will take whatever is
coming from firmware and combine it (if appropriate) with flags you
add.  

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

Hence IRQF_ONESHOT should be enough here.

We only extract the type for cases where the interrupt polarity or type
can also be configured at the device end.

> > +						"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);
> > +}