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

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 11 Sep 2026 06:12:17 +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.
> 
> Signed-off-by: John Erasmus Mari Geronimo <johnerasmusmari.geronimo@analog.com>
Hi John

Various comments inline

Jonathan

> diff --git a/drivers/iio/temperature/max30210.c b/drivers/iio/temperature/max30210.c
> new file mode 100644
> index 0000000000000..07ff85d6d4848
> --- /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/bitfield.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/kfifo_buf.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/regmap.h>
> +#include <linux/unaligned.h>
> +#include <linux/units.h>
> +
> +#define MAX30210_STATUS_REG		0x00
Up to you but I'm very much a fan of register address and fields together
in the code, using a bit of extra whitespace to make it obvious what
is going on. E.g. 
#define MAX30210_STATUS_REG		0x00
#define   MAX30210_STATUS_A_FULL_MASK   BIT(7)
#define   MAX30210_STATUS_TEMP_RDY_MASK BIT(6)
#define   MAX30210_STATUS_TEMP_DEC_MASK BIT(5)
#define   MAX30210_STATUS_TEMP_INC_MASK BIT(4)
#define   MAX30210_STATUS_TEMP_LO_MASK  BIT(3)
#define   MAX30210_STATUS_TEMP_HI_MASK  BIT(2)
#define   MAX30210_STATUS_PWR_RDY_MASK  BIT(0)
#define MAX30210_INT_EN_REG		0x02
...

That means everything related to the register is one place
which saves me jumping around when reviewing.

> +#define MAX30210_INT_EN_REG		0x02
> +#define MAX30210_FIFO_DATA_REG		0x08
> +#define MAX30210_FIFO_CONF_1_REG	0x09
> +#define MAX30210_FIFO_CONF_2_REG	0x0A
> +#define MAX30210_SYS_CONF_REG		0x11
> +#define MAX30210_PIN_CONF_REG		0x12
> +#define MAX30210_TEMP_ALM_HI_REG	0x22
> +#define MAX30210_TEMP_ALM_LO_REG	0x24
> +#define MAX30210_TEMP_INC_THRESH_REG	0x26
> +#define MAX30210_TEMP_DEC_THRESH_REG	0x27
> +#define MAX30210_TEMP_CONF_1_REG	0x28
> +#define MAX30210_TEMP_CONF_2_REG	0x29
> +#define MAX30210_TEMP_CONV_REG		0x2A
> +#define MAX30210_TEMP_DATA_REG		0x2B
> +#define MAX30210_TEMP_SLOPE_REG		0x2D
> +#define MAX30210_UNIQUE_ID_REG		0x30
> +#define MAX30210_PART_ID_REG		0xFF
> +
> +#define MAX30210_STATUS_A_FULL_MASK   BIT(7)
> +#define MAX30210_STATUS_TEMP_RDY_MASK BIT(6)
> +#define MAX30210_STATUS_TEMP_DEC_MASK BIT(5)
> +#define MAX30210_STATUS_TEMP_INC_MASK BIT(4)
> +#define MAX30210_STATUS_TEMP_LO_MASK  BIT(3)
> +#define MAX30210_STATUS_TEMP_HI_MASK  BIT(2)
> +#define MAX30210_STATUS_PWR_RDY_MASK  BIT(0)
> +
> +#define MAX30210_FIFOCONF1_A_FULL_MASK		GENMASK(5, 0)
> +#define MAX30210_FIFOCONF1_FLUSH_FIFO_MASK	BIT(4)
> +
> +#define MAX30210_SYSCONF_RESET_MASK		BIT(0)

We often drop the MASK postfix for single bit flags where
the meaning is obvious like this one.
#define MAX30210_SYSCONF_RESET			BIT(0)
Is fine and feels more normal when you use it in the code.

Be careful though when doing this.  Sometimes a field is simply
one that takes only values 0 and 1 but is not representing a boolean.
Those ones need the _MASK postfix and use of FIELD_GET / FIELD_PREP.
A recent example of that was a register bank selector in another driver.
There were only two banks so it was one bit but the code was easier
to read with it being via FIELD_PREP(_BANK_MASK, 0) etc

> +
> +#define MAX30210_PINCONF_EXT_CNV_EN_MASK	BIT(7)
> +#define MAX30210_PINCONF_EXT_CVT_ICFG_MASK	BIT(6)
> +#define MAX30210_PINCONF_INT_FCFG_MASK		GENMASK(3, 2)
> +#define MAX30210_PINCONF_INT_OCFG_MASK		GENMASK(1, 0)

> +
> +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 = 1000;
> +
> +		return IIO_VAL_FRACTIONAL;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		ret = regmap_read(st->regmap, MAX30210_TEMP_CONF_2_REG, &uval);
> +		if (ret)
> +			return ret;
> +
> +		uval = 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.
> +		 */
> +		uval = min_t(unsigned int, uval, ARRAY_SIZE(max30210_samp_freq_avail) - 1);

min() without very good reasons.

> +
> +		*val = max30210_samp_freq_avail[uval][0];
> +		*val2 = max30210_samp_freq_avail[uval][1];
> +
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	case IIO_CHAN_INFO_RAW: {
> +		if (iio_buffer_enabled(indio_dev))
> +			return -EBUSY;
> +
> +		IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
> +		if (IIO_DEV_ACQUIRE_FAILED(claim))
> +			return -EBUSY;
> +
> +		ret = regmap_write(st->regmap, MAX30210_TEMP_CONV_REG,
> +				   MAX30210_TEMPCONV_CONV_T_MASK);
> +		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_MASK),
> +					       500,          /* poll every 500 µs */
> +					       10000);       /* 10 ms timeout */
> +		if (ret)
> +			return ret;
> +
> +		return max30210_read_temp(st->regmap, MAX30210_TEMP_DATA_REG, val);
> +	}
> +	default:
> +		return -EINVAL;
> +	}
> +}

> +
> +static int max30210_write_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);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ: {
> +		IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
> +		if (IIO_DEV_ACQUIRE_FAILED(claim))
> +			return -EBUSY;
> +
> +		if (val < 0 || val2 < 0)
> +			return -EINVAL;

Not important but I'd move this check before the DIRECT_MODE claim
as it has nothing to do with device state.

> +
> +		for (unsigned int i = 0; i < ARRAY_SIZE(max30210_samp_freq_avail); i++) {
> +			if (val != max30210_samp_freq_avail[i][0] ||
> +			    val2 != max30210_samp_freq_avail[i][1])
> +				continue;
> +
> +			return regmap_update_bits(st->regmap, MAX30210_TEMP_CONF_2_REG,
> +					MAX30210_TEMPCONF2_TEMP_PERIOD_MASK,
> +					FIELD_PREP(MAX30210_TEMPCONF2_TEMP_PERIOD_MASK, i));
> +		}
> +
> +		return -EINVAL;
> +	}
> +	default:
> +		return -EINVAL;
> +	}
> +}


> +
> +static int max30210_buffer_preenable(struct iio_dev *indio_dev)
> +{
> +	struct max30210_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	/* Enable FIFO-full interrupt */

More comments to think about whether they add anything useful over
the code.  If they do maybe the defines need their names to be
tweaked!  E.g. ALMOST_FULL would be clearer.  You don't need
to exactly match datasheets where space demands etc may have
lead to things getting too abbreviated to be useful!

> +	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);
> +	if (ret)
> +		return ret;
> +
> +	/* Start autonomous temperature conversion */
> +	return regmap_write(st->regmap, MAX30210_TEMP_CONV_REG,
> +			    MAX30210_TEMPCONV_AUTO_MASK | MAX30210_TEMPCONV_CONV_T_MASK);

If you do use MASK prefix, then I'd expect to always have FIELD_PREP()
involved.  So another reason to drop _MASK from these things
that have obvious meaning without.  Even then I'd be temped
to use FIELD_PREP simply to align with the off case I mention below.

> +}
> +
> +static int max30210_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> +	struct max30210_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	/* Stop autonomous conversion */

Not obvious why you'd write the whole register. Perhaps
specify that 0 as

FIELD_PREP(MAX30210_TEMP_CONV_AUTO, 0) |
FIELD_PREP(MAX30210_TEMP_CONV_T, 0),

Compiler can flatten that to 0.


> +	ret = regmap_write(st->regmap, MAX30210_TEMP_CONV_REG, 0x0);
> +	if (ret)
> +		return ret;
> +
> +	/* Flush FIFO */
> +	ret = regmap_set_bits(st->regmap, MAX30210_FIFO_CONF_2_REG,
> +			      MAX30210_FIFOCONF1_FLUSH_FIFO_MASK);
> +	if (ret)
> +		return ret;
> +
> +	/* Disable FIFO-full interrupt */
> +	return regmap_clear_bits(st->regmap, MAX30210_INT_EN_REG,
> +				 MAX30210_STATUS_A_FULL_MASK);

Similar to below, the use of _MASK postfix for a flag like this to
me makes the code harder to read because I feel a need to go check
that it is just one bit.  Anyhow, that applies in lots of places.

> +}


> +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 */

That is kind of obvious from the code so I'd drop the comment.

> +	powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> +						 GPIOD_OUT_HIGH);
It is fine to go a bit long on lines if it helps readability.
Here the gain is minor but it's only a few chars long.

> +	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 {
> +		/* Software reset fallback */
Also obvious.  Check all your comments for ones that are saying things
that are naturally true.

> +		ret = regmap_set_bits(st->regmap, MAX30210_SYS_CONF_REG,
> +				      MAX30210_SYSCONF_RESET_MASK);

Comment at the top about use of _MASK postfix was from seeing this line.

> +		if (ret)
> +			return ret;
> +	}
> +
> +	/*
> +	 * Datasheet Figure 6:
> +	 * tPU max = 700 µs after power-up or reset before device is ready.
> +	 */
> +	fsleep(700);
> +
> +	/* Clear status byte */

This is a good comment so keep it. Not obvious to anyone seeing the code
that the register is read to clear.

> +	return regmap_read(st->regmap, MAX30210_STATUS_REG, &val);
> +}
> +
> +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 (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> +		return -EOPNOTSUPP;
When I see one of these I immediate search for uses of smbus
functions to check it's right.  Obviously here they are all hidden
by regmap which got me wondering why it wasn't regmap doing this
check...  So 
https://elixir.bootlin.com/linux/v7.2.5/source/drivers/base/regmap/regmap-i2c.c#L350
for val_bits = 8 and reg_bits = 8 there are two possible paths.
	else if (config->val_bits == 8 && config->reg_bits == 8 &&
		 i2c_check_functionality(i2c->adapter,
					 I2C_FUNC_SMBUS_I2C_BLOCK))
		bus = &regmap_i2c_smbus_i2c_block;
	}
or
	else if (config->val_bits == 8 && config->reg_bits == 8 &&
		 i2c_check_functionality(i2c->adapter,
					 I2C_FUNC_SMBUS_BYTE_DATA))
		bus = &regmap_smbus_byte;
	}

The first one relies on auto increment behaviour for multi reg reads.
The second doesn't.  I guess this works because a device that doesn't
do autoincrement reads won't have a driver that issues them.

Anyhow upshot is it is already checked, so you don't need another check
in the driver.  At somepoint I'll do a sweep for these and remove
any that might get cut and paste into new drivers.

> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;