Re: [PATCH v4 2/3] iio: health: add MAX86150 ECG and PPG biosensor driver
From: Andy Shevchenko
Date: Tue Jun 23 2026 - 16:55:28 EST
On Tue, Jun 23, 2026 at 08:45:59PM +0300, Md Shofiqul Islam wrote:
> The MAX86150 (Maxim/Analog Devices) combines two PPG optical channels
> (Red/IR LED) and one ECG biopotential channel in a single I2C device with
> a 32-entry hardware FIFO.
>
> Driver features:
> - Three IIO channels: in_intensityred_raw, in_intensityir_raw,
> in_voltage0_raw
> - Triggered buffer path driven by the FIFO almost-full interrupt;
> set_trigger_state enables/disables the interrupt only while the buffer
> is active and FIFO is flushed before capture starts
> - validate_trigger = iio_trigger_validate_own_device prevents
> incompatible external triggers from being attached
> - IRQ trigger type taken from irq_get_trigger_type() to honour DT
> configuration; falls back to falling-edge if unspecified
> - fifo_raw burst-read buffer is heap-allocated in struct max86150_data
> and aligned to ARCH_DMA_MINALIGN to satisfy DMA mapping requirements
> of I2C host controllers that use DMA for burst reads
> - SYS_SHDN asserted at end of chip_init so LED drivers draw no current
> when capture is inactive; set_trigger_state() and read_raw() wake and
> sleep the device on demand
> - Per-sample timestamps anchored to the A_FULL IRQ capture time: the
> sample at index (A_FULL_SAMPLES - 1) maps to pf->timestamp, samples
> accumulated after the IRQ receive future timestamps, eliminating
> load-dependent jitter
> - FIFO empty vs exactly-full disambiguation: when wr_ptr == rd_ptr with
> OVF_COUNTER == 0, the A_FULL status bit distinguishes a pointer
> wrap-around (full) from a genuinely empty FIFO
> - devm_regulator_get_enable_optional() for the two optional supplies;
> -ENODEV is treated as success (supply absent, not an error)
> - Powerdown devm action disables interrupts and asserts SYS_SHDN
...
> endmenu
>
> +
Single blank line is enough.
> +config MAX86150
...
IWYU, please! (The list of missing headers below may not be comprehensive.)
+ array_size.h
> +#include <linux/bitfield.h>
+ bitops.h // GENMASK(), sign_extend32()
> +#include <linux/delay.h>
+ dev_printk.h // dev_err_probe()
+ device/devres.h // devm_add_action_or_reset()
+ err.h // IS_ERR(), -ENOMEM, ...
> +#include <linux/i2c.h>
> +#include <linux/irq.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
Can we move this group out...
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
+ types.h // uXX, sXX
> +
...to be here?
...
> +/* Scan element indices */
> +enum max86150_scan_idx {
> + MAX86150_IDX_PPG_RED = 0,
> + MAX86150_IDX_PPG_IR = 1,
> + MAX86150_IDX_ECG = 2,
> + MAX86150_IDX_TS,
Why no value for TS? Or why values for the rest? I assume it's HW/SW cases?
> +};
> +/**
> + * struct max86150_data - driver private state
> + * @regmap: register map for this device
> + * @dev: parent device (for dev_err logging)
> + * @trig: IIO hardware trigger backed by the device interrupt line
> + * @sample_period_ns: sample period in nanoseconds (set from configured rate)
> + * @fifo_raw: DMA-safe buffer for regmap_noinc_read() FIFO bursts;
> + * must be in struct (heap) not on the stack to satisfy
> + * DMA mapping requirements of some I2C host controllers
> + * @buf: IIO push buffer sized for worst-case (all 3 channels
> + * active): 3 x s32 (12 bytes) + 4-byte pad + s64
> + * timestamp = 24 bytes. __aligned(8) satisfies
> + * iio_push_to_buffers_with_timestamp().
> + */
> +struct max86150_data {
> + struct regmap *regmap;
> + struct device *dev;
Is regmap device and dev is the same? If so, drop one and derive the other.
> + struct iio_trigger *trig;
> + u32 sample_period_ns;
> + u8 fifo_raw[MAX86150_SAMPLE_BYTES] __aligned(ARCH_DMA_MINALIGN);
> + s32 buf[6] __aligned(8);
We have a macro for these. (For alignment.)
> +};
...
> +static int max86150_read_one_sample(struct max86150_data *data,
> + u32 *ppg_red, u32 *ppg_ir, s32 *ecg)
> +{
> + int ret;
> +
> + /*
> + * Use data->fifo_raw (heap memory) not a local array so the buffer is
> + * DMA-mappable for I2C host controllers that use DMA for burst reads.
> + */
> + ret = regmap_noinc_read(data->regmap, MAX86150_REG_FIFO_DATA,
> + data->fifo_raw, sizeof(data->fifo_raw));
> + if (ret)
> + return ret;
> +
> + /* Bytes [0..2]: PPG Red - 19-bit value in bits [18:0] */
> + *ppg_red = (u32)(data->fifo_raw[0] & 0x07) << 16 |
> + (u32)data->fifo_raw[1] << 8 | data->fifo_raw[2];
Casting to u32 makes a little sense. Why?
> + /* Bytes [3..5]: PPG IR - same format */
> + *ppg_ir = (u32)(data->fifo_raw[3] & 0x07) << 16 |
> + (u32)data->fifo_raw[4] << 8 | data->fifo_raw[5];
> +
> + /* Bytes [6..8]: ECG - 18-bit signed, sign-extend to s32 */
> + *ecg = sign_extend32((u32)(data->fifo_raw[6] & 0x03) << 16 |
> + (u32)data->fifo_raw[7] << 8 | data->fifo_raw[8], 17);
Ditto for all these...
> + 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);
> + u32 ppg_red, ppg_ir;
> + s32 ecg;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + /*
> + * Claim direct mode to prevent concurrent sysfs reads from
> + * corrupting the FIFO pointers while a triggered buffer
> + * capture is active.
> + */
> + if (!iio_device_claim_direct(indio_dev))
> + return -EBUSY;
> +
> + /*
> + * Single-shot path: wake the device, flush stale FIFO data,
> + * wait one sample period, read, then return to shutdown so
> + * the LEDs are not drawing current when idle.
> + */
> + ret = regmap_update_bits(data->regmap, MAX86150_REG_SYS_CTRL,
> + MAX86150_SYS_SHDN, 0);
> + if (!ret)
> + ret = regmap_write(data->regmap,
> + MAX86150_REG_FIFO_WR_PTR, 0);
> + if (!ret)
> + ret = regmap_write(data->regmap,
> + MAX86150_REG_OVF_COUNTER, 0);
> + if (!ret)
> + ret = regmap_write(data->regmap,
> + MAX86150_REG_FIFO_RD_PTR, 0);
> + if (ret) {
> + regmap_update_bits(data->regmap, MAX86150_REG_SYS_CTRL,
> + MAX86150_SYS_SHDN, MAX86150_SYS_SHDN);
> + iio_device_release_direct(indio_dev);
> + return ret;
No, use regular pattern, id est
if (ret)
return ret;
Ditto for other cases like this.
> + }
> +
> + /* Wait for one complete sample period at 100 Hz (<= 10 ms) */
> + usleep_range(11000, 13000);
> +
> + ret = max86150_read_one_sample(data, &ppg_red, &ppg_ir, &ecg);
> + regmap_update_bits(data->regmap, MAX86150_REG_SYS_CTRL,
> + MAX86150_SYS_SHDN, MAX86150_SYS_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;
> + }
> +}
...
> +/*
> + * Control device power and the FIFO almost-full interrupt when the IIO
> + * triggered buffer is started (state=true) or stopped (state=false).
> + *
> + * On start: wake from shutdown, flush stale FIFO data so the first
> + * samples pushed to userspace are from after buffer enable, then
> + * unmask the A_FULL interrupt.
> + *
> + * On stop: mask the interrupt, then return to shutdown so the LED
> + * drivers do not draw current while capture is inactive.
> + */
> +static int max86150_set_trigger_state(struct iio_trigger *trig, bool state)
> +{
> + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> + struct max86150_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + if (!state) {
> + ret = regmap_write(data->regmap, MAX86150_REG_INT_ENABLE1, 0);
> + regmap_update_bits(data->regmap, MAX86150_REG_SYS_CTRL,
> + MAX86150_SYS_SHDN, MAX86150_SYS_SHDN);
_set_bits(), but why no check?
> + return ret;
> + }
> +
> + ret = regmap_update_bits(data->regmap, MAX86150_REG_SYS_CTRL,
> + MAX86150_SYS_SHDN, 0);
> + if (!ret)
> + ret = regmap_write(data->regmap, MAX86150_REG_FIFO_WR_PTR, 0);
> + if (!ret)
> + ret = regmap_write(data->regmap, MAX86150_REG_OVF_COUNTER, 0);
> + if (!ret)
> + ret = regmap_write(data->regmap, MAX86150_REG_FIFO_RD_PTR, 0);
> + if (!ret)
> + ret = regmap_write(data->regmap, MAX86150_REG_INT_ENABLE1,
> + MAX86150_INT_A_FULL);
> + if (ret)
> + regmap_update_bits(data->regmap, MAX86150_REG_SYS_CTRL,
> + MAX86150_SYS_SHDN, MAX86150_SYS_SHDN);
> + return ret;
Ditto.
...
Also why not to split to trigger_enable and trigger_disable?
_trigger_enable(max86150_data *data)
_trigger_disable(...)
_set_trigger_state()
if (enable)
return _trigger_enable()
return trigger_disable();
> +}
...
> +/**
> + * max86150_trigger_handler - threaded IRQ handler for FIFO almost-full
> + *
> + * Called by the IIO buffer infrastructure when the hardware trigger fires.
> + * Reads INT_STATUS1 to de-assert the interrupt, then drains all available
> + * FIFO samples into the IIO push buffer, packing only the channels that
> + * are currently enabled in active_scan_mask.
> + */
> +static irqreturn_t max86150_trigger_handler(int irq, void *p)
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *idev = pf->indio_dev;
> + struct max86150_data *data = iio_priv(idev);
> + unsigned int status, wr_ptr, rd_ptr, ovf;
> + u32 ppg_red, ppg_ir;
> + s32 ecg;
> + int ret, n_avail, i, j;
Usually it's not a good idea to mix ret with other variables that not
semantically related. Also, why those are signed?
> + /*
> + * Reading INT_STATUS1 clears the interrupt. Do this before touching
> + * the FIFO so the pin is de-asserted while we drain samples.
> + */
> + ret = regmap_read(data->regmap, MAX86150_REG_INT_STATUS1, &status);
> + if (ret)
> + goto done;
> +
> + ret = regmap_read(data->regmap, MAX86150_REG_FIFO_WR_PTR, &wr_ptr);
> + if (ret)
> + goto done;
> + ret = regmap_read(data->regmap, MAX86150_REG_FIFO_RD_PTR, &rd_ptr);
> + if (ret)
> + goto done;
> +
> + /*
> + * OVF_COUNTER: if non-zero the FIFO overflowed; drain all 32 slots.
> + * When wr_ptr == rd_ptr with no overflow the FIFO could be empty OR
> + * hold exactly MAX86150_FIFO_DEPTH entries (pointer wrap-around).
> + * Use the A_FULL status bit to disambiguate: if the IRQ fired for
> + * A_FULL but the computed count is zero, the FIFO wrapped to full.
> + */
> + ret = regmap_read(data->regmap, MAX86150_REG_OVF_COUNTER, &ovf);
> + if (ret)
> + goto done;
> +
> + if (ovf > 0) {
> + n_avail = MAX86150_FIFO_DEPTH;
> + } else {
> + n_avail = (wr_ptr - rd_ptr) & (MAX86150_FIFO_DEPTH - 1);
> + if (n_avail == 0 && (status & MAX86150_INT_A_FULL))
> + n_avail = MAX86150_FIFO_DEPTH;
> + }
> +
> + /*
> + * Anchor timestamps to the A_FULL IRQ capture time: sample index
> + * (MAX86150_A_FULL_SAMPLES - 1) corresponds to pf->timestamp.
> + * Samples that accumulated between the IRQ and handler execution
> + * receive timestamps in the future relative to the IRQ, eliminating
> + * load-dependent jitter in multi-sample drains.
> + */
> + for (i = 0; i < n_avail; i++) {
for (unsigned int i = 0; i < n_avail; i++) {
> + s64 ts = pf->timestamp +
> + (s64)(i - (MAX86150_A_FULL_SAMPLES - 1)) *
> + data->sample_period_ns;
This is unmaintainable. Split definition and assignment. Also, do you really
need a casting? I.o.w. isn't pf->timestamp already an s64?
> + ret = max86150_read_one_sample(data, &ppg_red, &ppg_ir, &ecg);
> + if (ret)
> + break;
> +
> + /*
> + * Zero the entire buffer before packing so padding bytes
> + * between enabled channels do not leak previous sample data
> + * to userspace when fewer than 3 channels are active.
> + */
> + memset(data->buf, 0, sizeof(data->buf));
> +
> + /*
> + * Pack only active channels at consecutive positions [0..j-1].
> + * iio_push_to_buffers_with_timestamp() uses scan_bytes (which
> + * accounts for the active channel count) to place the timestamp,
> + * so static indexing would misplace it when fewer than 3
> + * channels are enabled.
> + */
> + j = 0;
> + if (test_bit(MAX86150_IDX_PPG_RED, idev->active_scan_mask))
> + data->buf[j++] = ppg_red;
> + if (test_bit(MAX86150_IDX_PPG_IR, idev->active_scan_mask))
> + data->buf[j++] = ppg_ir;
> + if (test_bit(MAX86150_IDX_ECG, idev->active_scan_mask))
> + data->buf[j++] = ecg;
> +
> + iio_push_to_buffers_with_timestamp(idev, data->buf, ts);
> + }
> +
> +done:
> + iio_trigger_notify_done(idev->trig);
> + return IRQ_HANDLED;
> +}
> +
> +/* Chip initialisation / teardown */
> +
> +static void max86150_powerdown(void *arg)
> +{
> + struct max86150_data *data = arg;
> +
> + regmap_write(data->regmap, MAX86150_REG_INT_ENABLE1, 0);
> + regmap_write(data->regmap, MAX86150_REG_SYS_CTRL, MAX86150_SYS_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_RESET);
> + if (ret)
> + return ret;
> + usleep_range(1000, 2000);
fsleep(). Also need a reference to datasheet section, table, et cetera.
> + /*
> + * FIFO: no sample averaging, rollover enabled, assert A_FULL when
> + * 17 samples are in the FIFO (32 - 15 = 17 available to read).
> + */
> + ret = regmap_write(data->regmap, MAX86150_REG_FIFO_CONFIG,
> + MAX86150_FIFO_ROLLOVER_EN |
> + FIELD_PREP(MAX86150_FIFO_A_FULL,
> + 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_FD1, MAX86150_FD_LED1) |
> + FIELD_PREP(MAX86150_FIFO_FD2, 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_FD3, MAX86150_FD_ECG) |
> + FIELD_PREP(MAX86150_FIFO_FD4, 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_ADC_RGE,
> + MAX86150_PPG_ADC_RGE_16384) |
> + FIELD_PREP(MAX86150_PPG_SR,
> + MAX86150_PPG_SR_100HZ));
> + if (ret)
> + return ret;
> +
> + /* LED pulse amplitudes (~50 mA) */
> + ret = regmap_write(data->regmap, MAX86150_REG_LED1_PA,
> + MAX86150_LED_PA_DEFAULT);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(data->regmap, MAX86150_REG_LED2_PA,
> + MAX86150_LED_PA_DEFAULT);
> + if (ret)
> + return ret;
> +
> + /*
> + * Record sample period for timestamp reconstruction in the trigger
> + * handler. The PPG_SR field is fixed to 100 Hz in this driver.
> + */
> + data->sample_period_ns = 10000000; /* 100 Hz = 10 ms */
10 * NSEC_PER_MSEC
from time.h.
> + /*
> + * Assert SYS_SHDN so the LED drivers do not draw current while
> + * the driver is bound but no capture is active.
> + * set_trigger_state() clears SHDN when the IIO buffer is enabled
> + * and re-asserts it when disabled. read_raw() wakes and sleeps
> + * the device around each single-shot read.
> + */
> + return regmap_write(data->regmap, MAX86150_REG_SYS_CTRL,
> + MAX86150_SYS_SHDN);
> +}
...
> +/* Probe */
These comments bring no value, please, drop all of a such.
...
> +static int max86150_probe(struct i2c_client *client)
> +{
> + struct iio_dev *indio_dev;
> + struct max86150_data *data;
> + unsigned int part_id;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + data = iio_priv(indio_dev);
> + data->dev = &client->dev;
A duplication. You have regmap with the same device.
> + /*
> + * Enable power supplies before any I2C access. Both supplies are
> + * optional in the device tree; use _optional variant so probing
> + * succeeds on boards that power the device from fixed rails with no
> + * DT regulator node.
> + */
> + ret = devm_regulator_get_enable_optional(&client->dev, "vdd");
> + if (ret && ret != -ENODEV)
> + return dev_err_probe(&client->dev, ret,
Add
struct device *dev = &client->dev;
to the top of the function and use everywhere.
> + "Failed to get/enable vdd supply\n");
> +
> + ret = devm_regulator_get_enable_optional(&client->dev, "leds");
> + if (ret && ret != -ENODEV)
> + return dev_err_probe(&client->dev, ret,
> + "Failed to get/enable leds supply\n");
> +
> + data->regmap = devm_regmap_init_i2c(client, &max86150_regmap_config);
> + if (IS_ERR(data->regmap))
> + return dev_err_probe(&client->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(&client->dev, ret,
> + "Cannot read part ID\n");
> +
> + if (part_id != MAX86150_PART_ID_VAL)
> + return dev_err_probe(&client->dev, -ENODEV,
> + "Unexpected part ID 0x%02x (expected 0x%02x)\n",
> + part_id, MAX86150_PART_ID_VAL);
> +
> + ret = max86150_chip_init(data);
> + if (ret)
> + return dev_err_probe(&client->dev, ret,
> + "Chip initialisation failed\n");
> +
> + ret = devm_add_action_or_reset(&client->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 the device tree provides an interrupt, set up a hardware
> + * trigger so userspace can use the FIFO almost-full signal to
> + * drive capture without polling.
> + */
> + if (client->irq > 0) {
> + unsigned long irq_trig;
> +
> + data->trig = devm_iio_trigger_alloc(&client->dev,
> + "%s-dev%d",
> + indio_dev->name,
> + iio_device_id(indio_dev));
> + if (!data->trig)
> + return -ENOMEM;
> +
> + data->trig->ops = &max86150_trigger_ops;
> + iio_trigger_set_drvdata(data->trig, indio_dev);
> +
> + /*
> + * Honour the interrupt trigger type from the device tree.
> + * Fall back to falling-edge if the DT does not specify one.
Why? Do we need to support broken DT?
> + */
> + irq_trig = irq_get_trigger_type(client->irq);
> + if (!irq_trig)
> + irq_trig = IRQF_TRIGGER_FALLING;
Simply drop this.
> + ret = devm_request_irq(&client->dev, client->irq,
> + iio_trigger_generic_data_rdy_poll,
> + irq_trig,
> + "max86150", data->trig);
> + if (ret)
> + return dev_err_probe(&client->dev, ret,
> + "Cannot request IRQ %d\n",
> + client->irq);
No dup messages.
> +
> + ret = devm_iio_trigger_register(&client->dev, data->trig);
> + if (ret)
> + return dev_err_probe(&client->dev, ret,
> + "Cannot register trigger\n");
> + }
> +
> + ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev,
> + iio_pollfunc_store_time,
> + max86150_trigger_handler,
> + NULL);
> + if (ret)
> + return dev_err_probe(&client->dev, ret,
> + "Cannot setup triggered buffer\n");
So, it seems this message won't ever be printed. Drop it.
> + return devm_iio_device_register(&client->dev, indio_dev);
> +}
--
With Best Regards,
Andy Shevchenko