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

[Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>]

On Fri, 17 Jul 2026 23:11:37 +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. The driver uses the standard IIO hardware-trigger and
> triggered-buffer framework: a hard-irq handler reads and clears
> INT_STATUS1 to de-assert the line before calling iio_trigger_poll(),
> and the threaded trigger handler drains the FIFO and pushes samples,
> with timestamps back-calculated from the interrupt arrival time by one
> sample_period_ns per step. Relying on the trigger core's own
> attach/detach synchronization avoids the need for an explicit
> iio_buffer_enabled() guard or synchronize_irq() in the interrupt path.

Why is it using a triggered buffer + trigger?  We normally don't do
that when a hardware fifo is involved. That stuff is all about when
a trigger indicate a scan of channels (one set of samples of all channels).

Look at the drivers that register a kfifo directly.  Sorry I didn't
raise this earlier.  I think I got too focused on the narrow details
rather than the big picture.


> 
> Key implementation details:
> - Part ID register (0xFF) verified against 0x1E on probe; mismatched
>   devices are rejected with -ENODEV so the driver cannot bind to the
>   wrong hardware
> - Stale A_FULL and PPG_RDY status bits are cleared before arming the
>   interrupt or polling for a sample, so a previously-latched flag
>   cannot fire the handler against garbage state
> - 24-bit FIFO words decoded directly from the raw byte buffer
> - regmap_set_bits() / regmap_clear_bits() for single-direction writes
> - Device remains in shutdown between captures to suppress LED current
> - vdd, avdd, vref and leds regulators required per the datasheet power
>   tree
> - iio_get_time_ns() used for timestamps so they respect the IIO
>   device's configured clock source
> - A_FULL status bit used to detect FIFO exactly full (wr_ptr == rd_ptr
>   with OVF_COUNTER == 0) so valid samples are not silently dropped
> - No IRQ trigger-type override: the device has no register to
>   reconfigure interrupt polarity or type, so IRQF_ONESHOT with
>   whatever type firmware provides is sufficient
> 
> Suggested-by: Jonathan Cameron <jic23@kernel.org>
> Suggested-by: Joshua Crofts <joshua.crofts1@gmail.com>
> Assisted-by: Claude:claude-sonnet-5
> Signed-off-by: Md Shofiqul Islam <shofiqtest@gmail.com>


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



> +static int max86150_chip_init(struct max86150_data *data)
> +{
...
> +	if (ret)
> +		return ret;
> +
> +	data->sample_period_ns = 10000000; /* matches MAX86150_PPG_SR_SP_100HZ above */

Probably express as NANO / 100;


> +
> +static int max86150_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct iio_dev *indio_dev;
> +	struct max86150_data *data;
> +	unsigned int part_id;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +
> +	ret = devm_regulator_get_enable(dev, "vdd");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable vdd supply\n");
> +
> +	ret = devm_regulator_get_enable(dev, "avdd");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable avdd supply\n");
> +
> +	ret = devm_regulator_get_enable(dev, "vref");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable vref supply\n");
> +
> +	ret = devm_regulator_get_enable(dev, "leds");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable leds supply\n");

Given there are 4 of these devm_regulator_bulk_get_enable() probably makes sense.

> +
> +	data->regmap = devm_regmap_init_i2c(client, &max86150_regmap_config);
> +	if (IS_ERR(data->regmap))
> +		return dev_err_probe(dev, PTR_ERR(data->regmap),
> +				     "Failed to init regmap\n");
> +
> +	ret = regmap_read(data->regmap, MAX86150_REG_PART_ID, &part_id);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Cannot read part ID\n");
> +
> +	if (part_id != MAX86150_PART_ID_VAL)
> +		return dev_err_probe(dev, -ENODEV,
> +				     "Unexpected part ID 0x%02x (expected 0x%02x)\n",
> +				     part_id, MAX86150_PART_ID_VAL);

This breaks fallback dt compatibles. Normally if we get a missmatch we just print
a message and carry on anyway.  If we need to not do that for some reason here
add a comment to that affect.

> +
> +	ret = max86150_chip_init(data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Chip initialisation failed\n");
> +
> +	ret = devm_add_action_or_reset(dev, max86150_powerdown, data);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->name         = "max86150";
> +	indio_dev->channels     = max86150_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(max86150_channels);
> +	indio_dev->info         = &max86150_iio_info;
> +	indio_dev->modes        = INDIO_DIRECT_MODE;
> +
> +	if (client->irq > 0) {
> +		data->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
> +						    indio_dev->name,
> +						    iio_device_id(indio_dev));
> +		if (!data->trig)
> +			return -ENOMEM;
> +
> +		data->trig->ops = &max86150_trigger_ops;
> +		iio_trigger_set_drvdata(data->trig, indio_dev);
> +
> +		/*
> +		 * The device only ever drives an active-low interrupt line;
> +		 * there is no register to reconfigure its polarity or type,
> +		 * so the trigger type from firmware needs no help here.

Not need to say this.  It is most common situation.

> +		 */
> +		ret = devm_request_threaded_irq(dev, client->irq,
> +						NULL,
> +						max86150_irq_handler,
> +						IRQF_ONESHOT,
> +						"max86150", data->trig);

Using a trigger for a fifo full interrupt is always a little bit overly
complex.  In this case you have both validation function set so
you can't use the buffer without this trigger being available.
If it is meaningful to trigger this from another source then
maybe you use a triggered_buffer.  In some cases where both types
of operation are things people want (maybe from a sysfs trigger) we don't
necessarily register a trigger for the fifo interrupt. The lack of having
set a trigger can mean that path should be used.

Normal thing to do in this case is no trigger + directly register
a kfifo to fill from the hardware fifo.  Simply turning that buffer
on is the signal to begin capture.

> +		if (ret)
> +			return ret;
> +
> +		ret = devm_iio_trigger_register(dev, data->trig);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to register trigger\n");
> +	}
> +
> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> +					      iio_pollfunc_store_time,
> +					      max86150_trigger_handler,
> +					      NULL);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Set the default trigger AFTER buffer setup succeeds.  Setting it
> +	 * before would leak the iio_trigger_get() reference if buffer setup
> +	 * failed: INDIO_BUFFER_TRIGGERED is not set on that path so
> +	 * iio_device_release() skips iio_trigger_put().

I don't believe there is anything stopping you just registering
the triggered_buffer before the trigger.  That would get rid of
this complexity.

> +	 */
> +	if (data->trig)
> +		indio_dev->trig = iio_trigger_get(data->trig);
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}