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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The driver uses hallucinated locking macros causing a build failure, and omits the mandatory lock release.
- [High] The I2C read buffer `fifo_raw` is not cacheline aligned, violating DMA API requirements.
- [High] The threaded IRQ handler incorrectly calls a hardirq-only IIO trigger poll function.
- [Low] The i2c_device_id array is missing named initializers.
--

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

iio: health: add MAX86150 ECG and PPG biosensor driver

This commit adds a new IIO driver for the Analog Devices MAX86150
combined ECG and PPG biosensor. It supports hardware FIFO captures via
a threaded trigger handler.

> 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
[ ... ]
> +struct max86150_data {
> +	struct regmap		*regmap;
> +	struct iio_trigger	*trig;
> +	u32			 sample_period_ns;
> +	u8			 fifo_raw[MAX86150_SAMPLE_BYTES];

[Severity: High]
Since this array is the target of regmap_noinc_read() and some I2C host
controllers use DMA for bulk reads, should this buffer be cacheline aligned?
Sharing a cacheline with sample_period_ns and trig can lead to silent
corruption if the CPU accesses them concurrently during a DMA transfer.

> +	IIO_DECLARE_DMA_BUFFER_WITH_TS(s32, scan, MAX86150_NUM_SLOTS);
> +};
[ ... ]
> +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: {
> +		IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
> +
> +		if (IIO_DEV_ACQUIRE_FAILED(claim))
> +			return -EBUSY;

[Severity: High]
Do the IIO_DEV_ACQUIRE_DIRECT_MODE and IIO_DEV_ACQUIRE_FAILED macros
exist in the IIO framework? If these are replaced with the standard
iio_device_claim_direct() function, does this function also need
corresponding iio_device_release_direct() calls on its return paths to
prevent deadlocking the IIO device state?

> +
> +		ret = max86150_do_read_raw(data, &ppg_red, &ppg_ir, &ecg);
[ ... ]
> +static irqreturn_t max86150_irq_handler(int irq, void *private)
> +{
> +	struct iio_trigger *trig = private;
> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> +	struct max86150_data *data = iio_priv(indio_dev);
> +	unsigned int status;
> +	int ret;
> +
> +	ret = regmap_read(data->regmap, MAX86150_REG_INT_STATUS1, &status);
> +	if (ret || !(status & MAX86150_INT_A_FULL))
> +		return IRQ_NONE;
> +
> +	iio_trigger_poll(trig);

[Severity: High]
Since this handler is registered as the thread_fn in devm_request_threaded_irq
with a NULL hardirq handler, does calling iio_trigger_poll() here violate
locking rules? iio_trigger_poll() expects to run in hardirq context. Should
this use iio_trigger_poll_nested() instead?

> +	return IRQ_HANDLED;
> +}
[ ... ]
> +static const struct i2c_device_id max86150_id[] = {
> +	{ "max86150" },

[Severity: Low]
Does the I2C subsystem API require initialized arrays of type
struct i2c_device_id to use named initializers? Using named initializers
such as { .name = "max86150" } ensures compliance with subsystem guidelines.

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, max86150_id);

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