Re: [PATCH v3 2/2] iio: temperature: add support for Analog Devices MAX30210

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] The driver uses `regmap_bulk_read` to read from the FIFO, which increments the register address on fallback, corrupting the read.
- [High] Threshold events will never trigger unless the FIFO buffer is also enabled, because autonomous conversion is not managed correctly.
- [High] Use-after-free on IIO `event_interface` during device removal due to incorrect devm registration order.
- [Medium] The driver strictly requires an interrupt, contradicting the commit message claim that interrupt signaling is optional.
- [Low] Missing Kconfig dependencies for REGMAP_I2C, IIO_BUFFER, and IIO_KFIFO_BUF.
- [Low] Device is not powered down or disabled on driver removal, leading to power leaks.
- [Low] Missing `#include <linux/regulator/consumer.h>` causes implicit function declaration.
- [Low] Macro naming typo suggests writing to the wrong register.
--

commit cfe908c78eef329997beda55cb489b59b1cf8d93
Author: John Erasmus Mari Geronimo <johnerasmusmari.geronimo@analog.com>

iio: temperature: add support for Analog Devices MAX30210

This adds support for the Analog Devices MAX30210 I2C temperature sensor.
The driver integrates with the IIO framework, providing direct mode
temperature conversion, threshold events, and FIFO operation.

> --- a/drivers/iio/temperature/Kconfig
> +++ b/drivers/iio/temperature/Kconfig
> @@ -152,6 +152,16 @@ config MAX30208
>  	  This driver can also be built as a module. If so, the module
>  	  will be called max30208.
>  
> +config MAX30210
> +	tristate "Analog Devices MAX30210 temperature sensor"
> +	depends on I2C

[Severity: Low]
Should this select REGMAP_I2C, IIO_BUFFER, and IIO_KFIFO_BUF? The driver 
code calls devm_regmap_init_i2c() and devm_iio_kfifo_buffer_setup_ext(). 
Without these selections, could a build with CONFIG_MAX30210 enabled fail 
at compile or link time if the user doesn't manually enable them?

> --- /dev/null
> +++ b/drivers/iio/temperature/max30210.c
> @@ -0,0 +1,689 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Analog Devices MAX30210 I2C Temperature Sensor driver
> + *
> + * Copyright 2026 Analog Devices Inc.
> + */
[ ... ]
> +#include <linux/regmap.h>
> +#include <linux/unaligned.h>
> +#include <linux/units.h>

[Severity: Low]
Should <linux/regulator/consumer.h> be included here? The probe function 
uses devm_regulator_get_enable(), which might cause an implicit function 
declaration error on some configurations without this header.

> +static void max30210_fifo_read(struct iio_dev *indio_dev)
> +{
> +	struct max30210_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = regmap_bulk_read(st->regmap, MAX30210_FIFO_DATA_REG,
> +			       st->fifo_buf,
> +			       MAX30210_FIFO_BYTES_PER_SAMPLE * st->watermark);

[Severity: High]
Could this corrupt data on SMBus-only adapters? The driver uses 
regmap_bulk_read() to read the FIFO data register but permits probing on 
adapters that only support I2C_FUNC_SMBUS_BYTE_DATA. 

On such adapters, regmap falls back to single-byte reads and automatically 
increments the register address for each byte. Instead of repeatedly 
reading the FIFO register, this would read sequential configuration 
registers and push their contents as temperature samples.

> +static int max30210_write_event_config(struct iio_dev *indio_dev,
> +				       const struct iio_chan_spec *chan,
> +				       enum iio_event_type type,
> +				       enum iio_event_direction dir, bool state)
> +{
> +	struct max30210_state *st = iio_priv(indio_dev);
> +
> +	switch (dir) {
> +	case IIO_EV_DIR_RISING:
> +		switch (type) {
> +		case IIO_EV_TYPE_THRESH:
> +			return regmap_assign_bits(st->regmap, MAX30210_INT_EN_REG,
> +						  MAX30210_STATUS_TEMP_HI_MASK, state);

[Severity: High]
Will threshold events trigger if the IIO buffer is not enabled? 

The autonomous conversion bit (MAX30210_TEMPCONV_AUTO_MASK) is only set in 
max30210_buffer_preenable(). If a user configures and enables a threshold 
event via sysfs without enabling the buffer, does the device remain in 
standby and never sample the temperature?

> +static int max30210_buffer_preenable(struct iio_dev *indio_dev)
> +{
> +	struct max30210_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	/* Enable FIFO-full interrupt */
> +	ret = regmap_set_bits(st->regmap, MAX30210_INT_EN_REG,
> +			      MAX30210_STATUS_A_FULL_MASK);
> +	if (ret)
> +		return ret;
> +
> +	/* Flush FIFO before starting autonomous conversions */
> +	ret = regmap_set_bits(st->regmap, MAX30210_FIFO_CONF_2_REG,
> +			      MAX30210_FIFOCONF1_FLUSH_FIFO_MASK);

[Severity: Low]
Is MAX30210_FIFOCONF1_FLUSH_FIFO_MASK correct here? It is being applied to 
MAX30210_FIFO_CONF_2_REG. Should this use a FIFOCONF2_ prefixed macro 
instead to avoid confusion?

> +static int max30210_setup(struct max30210_state *st, struct device *dev)
> +{
> +	struct gpio_desc *powerdown_gpio;
> +	unsigned int val;
> +	int ret;
> +
> +	/* Optional hardware reset via powerdown GPIO */
> +	powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> +						 GPIOD_OUT_HIGH);
> +	if (IS_ERR(powerdown_gpio))
> +		return dev_err_probe(dev, PTR_ERR(powerdown_gpio),
> +				     "failed to request powerdown GPIO\n");
> +
> +	if (powerdown_gpio) {
> +		/* Deassert powerdown to power up device */
> +		gpiod_set_value(powerdown_gpio, 0);
> +	} else {

[Severity: Low]
Does this leave the hardware drawing power indefinitely on driver unbind? 
The device is powered up by deasserting the powerdown GPIO, but there 
doesn't seem to be a corresponding devm action or .remove callback to 
re-assert it and put the device back into standby when the module is removed.

> +static int max30210_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct iio_dev *indio_dev;
> +	struct max30210_state *st;
> +	int ret;
[ ... ]
> +	if (!client->irq)
> +		return dev_err_probe(dev, -ENXIO, "Missing interrupt.\n");

[Severity: Medium]
Does this unconditionally require an interrupt? The commit message mentions 
"Optional interrupt-driven data ready signaling", but this check causes the 
probe to fail if no IRQ is provided. Should it fall back gracefully to 
direct polling mode instead?

> +	ret = devm_request_threaded_irq(dev, client->irq, NULL,
> +					max30210_irq_handler, IRQF_ONESHOT,
> +					indio_dev->name, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

[Severity: High]
Can this cause a use-after-free on the IIO event_interface during device 
removal?

Because devm resources are cleaned up in LIFO order, 
devm_iio_device_register() will unregister first (which frees the 
event_interface), followed later by the IRQ handler shutdown. 

If an interrupt fires between these two steps, max30210_irq_handler() could 
call iio_push_event() and dereference the already freed event_interface.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1789032019.git.johnerasmusmari.geronimo@analog.com?part=2