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

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 23 Oct 2026 16:06:23 +0800
John Erasmus Mari Geronimo <johnerasmusmari.geronimo@analog.com> wrote:

> Add support for the Analog Devices MAX30210 I2C temperature
> sensor.
> 
> The driver uses regmap for register access and integrates with
> the IIO framework. It supports:
> 
> - Direct mode temperature conversion
> - Configurable sampling frequency
> - Threshold events
> - FIFO operation with IIO kfifo buffer support
> - Optional interrupt-driven data ready signaling
> 
> The device provides 16-bit signed temperature data and a
> 64-sample FIFO.
> 
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/max30210.pdf
> Signed-off-by: John Erasmus Mari Geronimo <johnerasmusmari.geronimo@analog.com>
A few things from sashiko
https://sashiko.dev/#/patchset/20261023080623.1830-1-johnerasmusmari.geronimo%40analog.com

Feel free to just reply to the thread saying why they are wrong if they are.

In particularly it reminded me of the reply I made to patch 2
about what we might want to do to fix the event interface lifetime
issue.  I haven't had a chance to get back to that since.


> diff --git a/drivers/iio/temperature/max30210.c b/drivers/iio/temperature/max30210.c
> new file mode 100644
> index 0000000000000..01a1eff4da663
> --- /dev/null
> +++ b/drivers/iio/temperature/max30210.c

> +
> +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_FIFOCONF2_FLUSH_FIFO);
> +	if (ret)
> +		goto err_disable_irq;
> +
> +	scoped_guard(mutex, &st->lock)

See the stuff in cleanup.h  Don't combine guards and gotos in
same function. They may be fine in some cases but it is considered
bad practice.  Here I'd just not use the guard.

Make sure to check for other instances of this.

> +		ret = max30210_autoconv_get(st);
> +	if (ret)
> +		goto err_disable_irq;
> +
> +	return 0;
> +
> +err_disable_irq:
> +	/* The core won't call postdisable() when preenable() fails. */
> +	regmap_clear_bits(st->regmap, MAX30210_INT_EN_REG,
> +			  MAX30210_STATUS_A_FULL_MASK);
> +
> +	return ret;
> +}


> +static int max30210_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct iio_dev *indio_dev;
> +	struct max30210_state *st;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +
> +	ret = devm_mutex_init(dev, &st->lock);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_regulator_get_enable(dev, "vdd");
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to enable vdd regulator.\n");
> +
> +	st->regmap = devm_regmap_init_i2c(client, &max30210_regmap);
> +	if (IS_ERR(st->regmap))
> +		return dev_err_probe(dev, PTR_ERR(st->regmap),
> +				     "Failed to allocate regmap.\n");
> +
> +	st->watermark = MAX30210_WATERMARK_DEFAULT;
> +
> +	ret = max30210_setup(st, dev);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = &max30210_channel;
> +	indio_dev->num_channels = 1;
> +	indio_dev->name = "max30210";
> +	indio_dev->info = &max30210_info;
> +
> +	/* Currently, without an interrupt there is no way to drain the FIFO into the buffer */
> +	if (client->irq) {
> +		ret = devm_iio_kfifo_buffer_setup_ext(dev, indio_dev, &max30210_buffer_ops,
> +						      max30210_fifo_attributes);
> +		if (ret)
> +			return ret;
> +
> +		/* Runs after unregister, so nothing can re-arm the device; before powerdown. */
> +		ret = devm_add_action_or_reset(dev, max30210_quiesce, st);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	if (!client->irq)
> +		return 0;
> +
> +	/*
> +	 * Request the IRQ after registering the IIO device so devm tears it
> +	 * down first on removal, before the event interface is freed.
> +	 */
> +	return devm_request_threaded_irq(dev, client->irq, NULL,
> +					 max30210_irq_handler, IRQF_ONESHOT,
> +					 indio_dev->name, indio_dev);

Please take a look at what I said on v4 about this. I agree
there is an issue in the core code but don't want to fix it by
introducing a race here.  It would be helpful to know if the
code I put in that thread works or not. If you are seeing reasons
not to make a change along those lines reply to that thread
to continue the discussion.

Jonathan


> +}