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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Fri, Oct 23, 2026 at 04:06:23PM +0800, John Erasmus Mari Geronimo 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.

...

> +static int max30210_read_event(struct iio_dev *indio_dev,
> +			       const struct iio_chan_spec *chan,
> +			       enum iio_event_type type,
> +			       enum iio_event_direction dir,
> +			       enum iio_event_info info, int *val, int *val2)
> +{
> +	struct max30210_state *st = iio_priv(indio_dev);
> +
> +	if (info != IIO_EV_INFO_VALUE)
> +		return -EINVAL;

Seems in the same way you can test for IIO_EV_TYPE_THRESH.

> +	switch (dir) {
> +	case IIO_EV_DIR_RISING:
> +		switch (type) {
> +		case IIO_EV_TYPE_THRESH:
> +			return max30210_read_temp(st->regmap, MAX30210_TEMP_ALM_HI_REG, val);
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_EV_DIR_FALLING:
> +		switch (type) {
> +		case IIO_EV_TYPE_THRESH:
> +			return max30210_read_temp(st->regmap, MAX30210_TEMP_ALM_LO_REG, val);
> +		default:
> +			return -EINVAL;
> +		}
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int max30210_write_event(struct iio_dev *indio_dev,
> +				const struct iio_chan_spec *chan,
> +				enum iio_event_type type,
> +				enum iio_event_direction dir,
> +				enum iio_event_info info, int val, int val2)
> +{
> +	struct max30210_state *st = iio_priv(indio_dev);
> +
> +	if (info != IIO_EV_INFO_VALUE)
> +		return -EINVAL;

Ditto.

> +	if (val < S16_MIN || val > S16_MAX)
> +		return -EINVAL;
> +
> +	switch (dir) {
> +	case IIO_EV_DIR_RISING:
> +		switch (type) {
> +		case IIO_EV_TYPE_THRESH:
> +			return max30210_write_temp(st->regmap, MAX30210_TEMP_ALM_HI_REG, val);
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_EV_DIR_FALLING:
> +		switch (type) {
> +		case IIO_EV_TYPE_THRESH:
> +			return max30210_write_temp(st->regmap, MAX30210_TEMP_ALM_LO_REG, val);

You can reduce the lines at least by 7 characters with

	struct regmap *map = st->regmap;

at the top of the function. Use same trick elsewhere.

> +		default:
> +			return -EINVAL;
> +		}
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int max30210_read_event_config(struct iio_dev *indio_dev,
> +				      const struct iio_chan_spec *chan,
> +				      enum iio_event_type type,
> +				      enum iio_event_direction dir)
> +{
> +	struct max30210_state *st = iio_priv(indio_dev);
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read(st->regmap, MAX30210_INT_EN_REG, &val);
> +	if (ret)
> +		return ret;

Do you need to do this for other than IIO_EV_TYPE_THRESH?
Otherwise check for that first and bail out earlier.

> +	switch (dir) {
> +	case IIO_EV_DIR_RISING:
> +		switch (type) {
> +		case IIO_EV_TYPE_THRESH:
> +			return FIELD_GET(MAX30210_STATUS_TEMP_HI_MASK, val);
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_EV_DIR_FALLING:
> +		switch (type) {
> +		case IIO_EV_TYPE_THRESH:
> +			return FIELD_GET(MAX30210_STATUS_TEMP_LO_MASK, val);
> +		default:
> +			return -EINVAL;
> +		}
> +	default:
> +		return -EINVAL;
> +	}
> +}

...

> +	guard(mutex)(&st->lock);
> +
> +	/* The core does not dedupe; only take/drop a reference on a real change. */
> +	ret = regmap_read(st->regmap, MAX30210_INT_EN_REG, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (!!(val & mask) == state)
> +		return 0;

regmap_test_bits() ?

> +	/*
> +	 * Threshold events need the device sampling autonomously, independent
> +	 * of whether the FIFO buffer is also enabled.
> +	 */
> +	if (state) {
> +		ret = max30210_autoconv_get(st);
> +		if (ret)
> +			return ret;
> +	}

> +	ret = regmap_assign_bits(st->regmap, MAX30210_INT_EN_REG, mask, state);
> +	if (ret) {
> +		if (state)
> +			max30210_autoconv_put(st);
> +		return ret;
> +	}
> +
> +	return state ? 0 : max30210_autoconv_put(st);

Why not simply

	if (state) {
		ret = regmap_set_bits(map, MAX30210_INT_EN_REG, mask);
		if (ret)
			max30210_autoconv_put(st);

		return ret;
	} else {
		ret = regmap_clear_bits(map, MAX30210_INT_EN_REG, mask);
		if (ret)
			return ret;

		return max30210_autoconv_put(st);
	}

(Yeah, if I suggested _assign_bits() earlier, I was clearly wrong, this
 is more complex logic here.)

> +}

...

> +static int max30210_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long mask)
> +{
> +	struct max30210_state *st = iio_priv(indio_dev);
> +	unsigned int uval;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = 5;
> +		*val2 = MILLI;
> +
> +		return IIO_VAL_FRACTIONAL;
> +	case IIO_CHAN_INFO_SAMP_FREQ: {
> +		unsigned int period;

Units? period_Hz?

> +		ret = regmap_read(st->regmap, MAX30210_TEMP_CONF_2_REG, &uval);
> +		if (ret)
> +			return ret;
> +
> +		period = FIELD_GET(MAX30210_TEMPCONF2_TEMP_PERIOD_MASK, uval);
> +
> +		/*
> +		 * TEMP_PERIOD is an index into the sampling frequency lookup
> +		 * table. Clamp in case the register holds a reserved value.
> +		 */
> +		period = min(period, ARRAY_SIZE(max30210_samp_freq_avail) - 1);
> +
> +		*val = max30210_samp_freq_avail[period][0];
> +		*val2 = max30210_samp_freq_avail[period][1];
> +
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	}
> +	case IIO_CHAN_INFO_RAW: {
> +		IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
> +		if (IIO_DEV_ACQUIRE_FAILED(claim))
> +			return -EBUSY;
> +
> +		guard(mutex)(&st->lock);
> +
> +		/*
> +		 * TEMP_DATA_REG is only refreshed by a single-shot
> +		 * conversion; the buffer/FIFO path is the only thing that
> +		 * gets updated during AUTO mode. If AUTO is already running
> +		 * for the buffer and/or events, triggering a one-off
> +		 * conversion here would stomp the AUTO bit out from under
> +		 * whoever enabled it, and there is no way to get a fresh
> +		 * single-shot value without doing so -- refuse instead of
> +		 * returning stale data.
> +		 */
> +		if (st->autoconv_users > 0)
> +			return -EBUSY;
> +
> +		ret = regmap_write(st->regmap, MAX30210_TEMP_CONV_REG,
> +				   MAX30210_TEMPCONV_CONV_T);
> +		if (ret)
> +			return ret;
> +
> +		/*
> +		 * Wait until CONVERT_T auto-clears.
> +		 * Datasheet:
> +		 *   tBIAS_WU = 260 µs
> +		 *   tINT     = 8 ms
> +		 *
> +		 * Worst-case conversion ≈ 8.26 ms.
> +		 * Use 10 ms timeout for margin.
> +		 */

> +		ret = regmap_read_poll_timeout(st->regmap, MAX30210_TEMP_CONV_REG, uval,
> +					       !(uval & MAX30210_TEMPCONV_CONV_T),
> +					       500, 10 * USEC_PER_MSEC);

Logical split

		ret = regmap_read_poll_timeout(map, MAX30210_TEMP_CONV_REG,
					       uval, !(uval & MAX30210_TEMPCONV_CONV_T),
					       500, 10 * USEC_PER_MSEC);

> +		if (ret)
> +			return ret;
> +
> +		return max30210_read_temp(st->regmap, MAX30210_TEMP_DATA_REG, val);

		return max30210_read_temp(map, MAX30210_TEMP_DATA_REG, val);

will fit into 80 characters limit.

> +	}
> +	default:
> +		return -EINVAL;
> +	}
> +}

...

> +static int max30210_set_watermark(struct iio_dev *indio_dev, unsigned int val)
> +{
> +	struct max30210_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (val < 1 || val > MAX30210_FIFO_SIZE)
> +		return -EINVAL;
> +
> +	/*
> +	 * FIFO_A_FULL sets the number of empty FIFO slots remaining when the
> +	 * A_FULL interrupt fires. For a watermark of N samples, set
> +	 * FIFO_A_FULL = FIFO_SIZE - N.
> +	 */

The rule of thumb is to make lines in the comments balanced (by length):

	/*
	 * FIFO_A_FULL sets the number of empty FIFO slots remaining when
	 * the A_FULL interrupt fires. For a watermark of N samples, set
	 * FIFO_A_FULL = FIFO_SIZE - N.
	 */

> +	ret = regmap_write(st->regmap, MAX30210_FIFO_CONF_1_REG,
> +			   FIELD_PREP(MAX30210_FIFOCONF1_A_FULL_MASK,
> +				      MAX30210_FIFO_SIZE - val));
> +	if (ret)
> +		return ret;
> +
> +	st->watermark = val;
> +
> +	return 0;
> +}

...

> +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)
> +		ret = max30210_autoconv_get(st);
> +	if (ret)
> +		goto err_disable_irq;

Oh, does it compile this way with scoped_guard()?

> +	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_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> +	struct max30210_state *st = iio_priv(indio_dev);
> +	int ret, err;
> +
> +	scoped_guard(mutex, &st->lock)
> +		err = max30210_autoconv_put(st);
> +
> +	/* Keep going on error so the interrupt is always disabled below. */
> +	ret = regmap_set_bits(st->regmap, MAX30210_FIFO_CONF_2_REG,
> +			      MAX30210_FIFOCONF2_FLUSH_FIFO);
> +	if (!err)
> +		err = ret;
> +
> +	/* Disable FIFO-full interrupt */
> +	ret = regmap_clear_bits(st->regmap, MAX30210_INT_EN_REG,
> +				MAX30210_STATUS_A_FULL_MASK);
> +
> +	return err ?: ret;
> +}

TBH, I don't like how these two callbacks are implemented in terms of goto:s
and additional error code variable, but I have nothing to suggest, so we would
live with the above... :-(

...

> +static void max30210_powerdown(void *data)
> +{
> +	struct gpio_desc *powerdown_gpio = data;
> +
> +	gpiod_set_value(powerdown_gpio, 1);
> +}

static void max30210_powerdown(void *powerdown_gpio)
{
	gpiod_set_value(powerdown_gpio, 1);
}

...

> +static void max30210_quiesce(void *data)
> +{
> +	struct max30210_state *st = data;
> +
> +	/* Stop interrupt sources and autonomous conversions; errors are not actionable here */

Make it multi-line to fit 80 characters limit.

> +	regmap_write(st->regmap, MAX30210_INT_EN_REG, 0);
> +	regmap_write(st->regmap, MAX30210_TEMP_CONV_REG, 0);
> +}

...

> +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");

Here it is fine to have a single line

		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);

		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);
> +}

...

Module description:
> +MODULE_DESCRIPTION("MAX30210 low-power digital temperature sensor");

Kconfig:
> +	tristate "Analog Devices MAX30210 temperature sensor"

Top comment:
>  * Analog Devices MAX30210 I2C Temperature Sensor driver

Make all three texts to be the same.

-- 
With Best Regards,
Andy Shevchenko