Re: [PATCH 2/2] iio: temperature: add ADI MAX30210 driver

[Jonathan Cameron]

On Fri, 27 Feb 2026 00:30:41 +0800
John Erasmus Mari Geronimo <johnerasmusmari.geronimo@xxxxxxxxxx> wrote:

> MAX30210 ±0.1°C Accurate Ultra-Small Low-Power Digital Temperature Sensor

For a temperature sensor, this description doesn't give enough detail
on why you are proposing an IIO driver rather than a hwmon one.

Please add more for v2.  Focus on what the part is and features you are supporting
that don't have a path to being supporting in hwmon. E.g. the fifo.

The other thing to add a brief note on is why this support cannot be easily
added to an existing driver.
> 
> Signed-off-by: John Erasmus Mari Geronimo <johnerasmusmari.geronimo@xxxxxxxxxx>
Various additional comments inline from me.

Welcome to IIO!

Jonathan


> diff --git a/drivers/iio/temperature/max30210.c b/drivers/iio/temperature/max30210.c
> new file mode 100644
> index 000000000000..aaa3a26be131
> --- /dev/null
> +++ b/drivers/iio/temperature/max30210.c
> @@ -0,0 +1,758 @@

> +
> +#define MAX30210_STATUS_REG		0x00
> +#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_A_FULL_MASK   BIT(7)

I'm very keen that field names reflect which register they are in.
That makes it much easier to review whether they are being correctly
used.  Usual way to do that is have a prefix that incorporates enough 
of the register name to work out that mapping.

> +#define MAX30210_TEMP_RDY_MASK BIT(6)
> +#define MAX30210_TEMP_DEC_MASK BIT(5)
> +#define MAX30210_TEMP_INC_MASK BIT(4)
> +#define MAX30210_TEMP_LO_MASK  BIT(3)
> +#define MAX30210_TEMP_HI_MASK  BIT(2)
> +#define MAX30210_PWR_RDY_MASK  BIT(0)
> +
> +#define MAX30210_FLUSH_FIFO_MASK BIT(4)
> +
> +#define MAX30210_EXT_CNV_EN_MASK	BIT(7)
> +#define MAX30210_EXT_CVT_ICFG_MASK	BIT(6)
> +#define MAX30210_INT_FCFG_MASK		GENMASK(3, 2)
> +#define MAX30210_INT_OCFG_MASK		GENMASK(1, 0)
> +
> +#define MAX30210_CHG_DET_EN_MASK	BIT(3)
> +#define MAX30210_RATE_CHG_FILTER_MASK	GENMASK(2, 0)
> +
> +#define MAX30210_TEMP_PERIOD_MASK	GENMASK(3, 0)
> +#define MAX30210_ALERT_MODE_MASK	BIT(7)
> +
> +#define MAX30210_AUTO_MASK	BIT(1)
> +#define MAX30210_CONV_T_MASK	BIT(0)
> +
> +#define MAX30210_PART_ID		0x45
> +#define MAX30210_FIFO_SIZE		64
> +#define MAX30210_FIFO_INVAL_DATA	GENMASK(23, 0)
> +#define MAX30210_WATERMARK_DEFAULT	(0x40 - 0x1F)
> +
> +#define MAX30210_INT_EN(state, mask)	((state) ? (mask) : 0x0)
Use regmap_assign_bits() to replace this macro.


> +
> +struct max30210_state {
> +	/*
> +	 * Prevent simultaneous access to the i2c client.
Why does that matter?  I'd imagine you have some read / modify write
sequences or need to not change the mode whilst something else is going
on?

> +	 */
> +	struct mutex lock;
> +	struct regmap *regmap;
> +	struct iio_trigger *trig;
> +	struct gpio_desc *powerdown_gpio;
> +	u8 watermark;
> +	u8 data[3 * MAX30210_FIFO_SIZE]  __aligned(IIO_DMA_MINALIGN);
> +};

> +static int max30210_read_temp(struct regmap *regmap, unsigned int reg,
> +			      int *temp)
> +{
> +	u8 uval[2] __aligned(IIO_DMA_MINALIGN);

As below, forcing alignment on the stack doesn't work for this purpose.
Needs to be on the heap. Put it next to data in the _state structure.

However, this is an i2c driver. I2C doesn't have any such requirements
on buffer alignment because it always bounce buffers data if needed.


> +	int ret;
> +
> +	ret = regmap_bulk_read(regmap, reg, uval, 2);
> +	if (ret)
> +		return ret;
> +
> +	*temp = sign_extend32(get_unaligned_be16(uval), 15);
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static int max30210_write_temp(struct regmap *regmap, unsigned int reg,
> +			       int temp)
> +{
> +	u8 uval[2] __aligned(IIO_DMA_MINALIGN);

You've miss understood the purpose of IIO_DMA_MINALIGN.
Make sure to look into that but the short answer is you can't do it on the stack.

> +
> +	put_unaligned_be16(temp, uval);
> +
> +	return regmap_bulk_write(regmap, reg, uval, 2);
Use a __be16 type for the buffer and then sizeof() for the 2

> +}
> +
> +static void max30210_fifo_read(struct iio_dev *indio_dev)
> +{
> +	struct max30210_state *st = iio_priv(indio_dev);
> +	u32 samp;
> +	int ret, i, j;
> +
> +	ret = regmap_bulk_read(st->regmap, MAX30210_FIFO_DATA_REG,
> +			       st->data, 3 * st->watermark);
> +	if (ret < 0)
> +		return dev_err(&indio_dev->dev, "Failed to read from fifo.\n");
> +
> +	for (i = 0; i < st->watermark; i++) {

		u32 samp = 0;

> +		samp = 0;
> +
> +		for (j = 0; j < 3; j++) {
> +			samp <<= 8;
> +			samp |= st->data[3 * i + j];

Looks like get_unaligned_be24() or similar. Use that instead of opencoding
the endian conversion.  However, you are claiming this is a big endian channel
and this is an endian conversion so something is wrong.  If you keep it as
a big endian channel, this probably wants to be simply memcpy()




> +		}
> +
> +		if (samp == MAX30210_FIFO_INVAL_DATA) {
> +			dev_err(&indio_dev->dev, "Invalid data\n");
> +			continue;
> +		}
> +
> +		iio_push_to_buffers(indio_dev, &samp);
> +	}
> +}
> +
> +static irqreturn_t max30210_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct max30210_state *st = iio_priv(indio_dev);
> +	unsigned int status;
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	ret = regmap_read(st->regmap, MAX30210_STATUS_REG, &status);
> +	if (ret) {
> +		dev_err(&indio_dev->dev, "Status byte read error\n");
> +		goto exit_irq;
> +	}
> +
> +	if (status & MAX30210_PWR_RDY_MASK) {
> +		dev_info(&indio_dev->dev, "power-on\n");

dev_dbg() at most.

> +		st->watermark = MAX30210_WATERMARK_DEFAULT;
> +	}
> +
> +	if (status & MAX30210_A_FULL_MASK)
> +		max30210_fifo_read(indio_dev);
> +
> +	if (status & MAX30210_TEMP_HI_MASK)
This is unusual.  If you have a single interrupt for both trigger
and thresholds then you can't use iio_trigger_generic_data_rdy_poll()

The main interrupt handler needs to work out what has happened then
ultimately use iio_trigger_poll_nested() to fire of the data capture.

However, there is a hardware fifo going on here. So what benefit
is the trigger providing?  Triggers are optional and often not appropriate
when there are hardware fifos present because it is not useful to use
them to capture data from other devices etc.  So just drop the trigger
here and have this as the main irq handler.

> +		iio_push_event(indio_dev,
> +			       IIO_UNMOD_EVENT_CODE(IIO_TEMP, 0,
> +						    IIO_EV_TYPE_THRESH,
> +						    IIO_EV_DIR_RISING),
> +			       iio_get_time_ns(indio_dev));
> +
> +	if (status & MAX30210_TEMP_LO_MASK)
> +		iio_push_event(indio_dev,
> +			       IIO_UNMOD_EVENT_CODE(IIO_TEMP, 0,
> +						    IIO_EV_TYPE_THRESH,
> +						    IIO_EV_DIR_FALLING),
> +			       iio_get_time_ns(indio_dev));
> +
> +exit_irq:

Whilst this is not buggy the advice (see cleanup.h comments) is never
combine gotos and guard() / __free() in one function. There are some evil corner
case and GCC at least doesn't catch them all.  Various ways to refactor
the code to avoid the mix.

> +	iio_trigger_notify_done(indio_dev->trig);
> +	return IRQ_HANDLED;
> +}

> +
> +static int max30210_validate_trigger(struct iio_dev *indio_dev,
> +				     struct iio_trigger *trig)
> +{
> +	struct max30210_state *st = iio_priv(indio_dev);
> +
> +	if (st->trig != trig)
> +		return -EINVAL;
Can you use iio_validate_own_trigger()?
They should both have the same parent.

> +
> +	return 0;

> +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) {
Can flip the logic to reduce indent.
	if (info != IIO_EV_INFO_VALUE)
		return -EINVAL;

	switch()

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

As below (check for other instances of this)

> +		case IIO_EV_DIR_FALLING:
> +			switch (type) {
> +			case IIO_EV_TYPE_THRESH:
> +				return max30210_write_temp(st->regmap,
> +							   MAX30210_TEMP_ALM_LO_REG, val);
> +			default:
> +				return -EINVAL;
> +			}
> +			break;

Can't get here so drop the break.

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

> +
> +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;
> +
> +		return IIO_VAL_INT;
> +	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_TEMP_PERIOD_MASK, uval);
> +
> +		*val = 8;
> +
> +		/**

/* 
See below and run a W=1 build which will probably complain about this.

> +		 * register values 0x9 or above have the same sample
> +		 * rate of 8Hz
> +		 */
> +		*val2 = uval >= 0x9 ? 1 : BIT(0x9 - uval);
> +
> +		return IIO_VAL_FRACTIONAL;
> +	case IIO_CHAN_INFO_RAW:
> +		ret = iio_device_claim_direct_mode(indio_dev);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_write(st->regmap, MAX30210_TEMP_CONV_REG,
> +				   MAX30210_CONV_T_MASK);
> +		if (ret)
> +			goto release_dmode;
> +
> +		fsleep(8000);

Add a spec reference for this.

> +
> +		ret = max30210_read_temp(st->regmap, MAX30210_TEMP_DATA_REG, val);
> +
> +release_dmode:

Labels for gotos inside a switch are not a good thing to do for readability.
We have the new ACQUIRE() stuff that David mentioned, but if that isn't appropriate
I'd suggest factoring out some of the code here so you can avoid the goto.

> +		iio_device_release_direct_mode(indio_dev);
> +		return ret;
> +	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);
> +	u64 data;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		ret = iio_device_claim_direct_mode(indio_dev);

Others pointed out this needs to be based on upstream where that
particular function has gone away.

> +		if (ret)
> +			return ret;
> +
> +		/**
Not kernel-doc style so /*

> +		 * micro_value = val * 1000000 + val2
> +		 * reg_value = ((micro_value * 64) / 1000000) - 1

Except that's not the reg_val, because you then call fls() on it.

> +		 */
> +		data = (val * MICRO + val2) << 6;
> +		do_div(data, MICRO);
> +
> +		data = fls_long(data - 1);

This is unusual enough I'd add a comment on the maths.

> +		data = FIELD_PREP(MAX30210_TEMP_PERIOD_MASK, data);

Use a different variable of the appropriate type to store the result.
That way no need for a cast below.

> +
> +		ret = regmap_update_bits(st->regmap, MAX30210_TEMP_CONF_2_REG,
> +					 MAX30210_TEMP_PERIOD_MASK,
> +					 (unsigned int)data);
> +
> +		iio_device_release_direct_mode(indio_dev);
> +		return ret;
> +	default:
> +		return -EINVAL;
> +	}
> +}


> +
> +static const struct iio_dev_attr *max30210_fifo_attributes[] = {
> +	&iio_dev_attr_hwfifo_watermark_min,
> +	&iio_dev_attr_hwfifo_watermark_max,
> +	&iio_dev_attr_hwfifo_watermark,
> +	NULL,

No comma for a null terminator.  Doesn't add anything useful and
makes it easier to put things after this (which is obviously a bug).

> +};
> +
> +static int max30210_buffer_preenable(struct iio_dev *indio_dev)
> +{
> +	struct max30210_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = regmap_update_bits(st->regmap, MAX30210_INT_EN_REG,
> +				 MAX30210_A_FULL_MASK, MAX30210_A_FULL_MASK);

regmap_set_bits() just avoids repeating the mask.


> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(st->regmap, MAX30210_FIFO_CONF_2_REG,
> +				 MAX30210_FLUSH_FIFO_MASK,
> +				 MAX30210_FLUSH_FIFO_MASK);

set bits is fine here.  I assume it auto-clears?


> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, MAX30210_TEMP_CONV_REG,
> +			   MAX30210_AUTO_MASK | MAX30210_CONV_T_MASK);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

return regmap_write()

> +}
> +
> +static int max30210_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> +	struct max30210_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = regmap_update_bits(st->regmap, MAX30210_INT_EN_REG,
> +				 MAX30210_A_FULL_MASK, 0x0);
Tiny bit simpler as
	ret = regmap_clear_bits(st->regmap, MAX30210_INT_EN_REG,
				MAX30210_A_FULL_MASK);

> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(st->regmap, MAX30210_FIFO_CONF_2_REG,
> +				 MAX30210_FLUSH_FIFO_MASK,
> +				 MAX30210_FLUSH_FIFO_MASK);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, MAX30210_TEMP_CONV_REG, 0x0);

return regmap_write();

I'm a bit surprised by the ordering in here (I haven't looked at the datasheet).
Mostly we'd expect the tear down of settings to be the reverse of setup. Here
that probably means that final write belongs before the flushing of the fifo.

> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static const struct iio_buffer_setup_ops max30210_buffer_ops = {
> +	.preenable = max30210_buffer_preenable,
> +	.postdisable = max30210_buffer_postdisable,
> +};
> +
> +static const struct iio_info max30210_info = {
> +	.read_raw = max30210_read_raw,
> +	.read_avail = max30210_read_avail,
> +	.write_raw = max30210_write_raw,
> +	.write_raw_get_fmt = max30210_write_raw_get_fmt,
> +	.hwfifo_set_watermark = max30210_set_watermark,
> +	.debugfs_reg_access = &max30210_reg_access,

Why & for some function pointers and not others?

> +	.validate_trigger = &max30210_validate_trigger,
> +	.read_event_value = max30210_read_event,
> +	.write_event_value = max30210_write_event,
> +	.write_event_config = max30210_write_event_config,
> +	.read_event_config = max30210_read_event_config,
> +};

> +
> +static const struct iio_chan_spec max30210_channels = {
> +	.type = IIO_TEMP,
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +			      BIT(IIO_CHAN_INFO_SCALE) |
> +			      BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +	.info_mask_separate_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +	.output = 0,

0 is a natural default for a boolean type thing like .output, so
no need to specify it. Let the C spec guarantees around zeroing all
fields deal with it for you.

> +	.scan_index = 0,
> +	.event_spec = max30210_events,
> +	.num_event_specs = ARRAY_SIZE(max30210_events),
> +	.scan_type = {
> +		.sign = 's',
> +		.realbits = 16,
> +		.storagebits = 32,
> +		.shift = 8,
> +		.endianness = IIO_BE,
> +	},
> +};

> +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;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +
> +	mutex_init(&st->lock);

	ret = devm_mutex_init(&st->lock);
	if (ret)
		return ret;

Look at what that does.  We don't care a lot about lock lifetime debugging
but given it's easy to do, why not enable it for the driver.


> +
> +	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");
> +
> +	ret = max30210_setup(st, dev);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = &max30210_channels;
> +	indio_dev->num_channels = 1;
> +	indio_dev->name = "max30210";
> +	indio_dev->info = &max30210_info;
> +
> +	ret = devm_iio_triggered_buffer_setup_ext(dev, indio_dev, NULL,
> +						  max30210_trigger_handler,
> +						  IIO_BUFFER_DIRECTION_IN,
> +						  &max30210_buffer_ops,
> +						  max30210_fifo_attributes);
> +	if (ret < 0)
> +		return ret;

For consistency, if (ret) should be fine here.  I don't think any
IIO core calls return positive integers.

> +
> +	if (client->irq) {
> +		st->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
> +						  indio_dev->name,
> +						  iio_device_id(indio_dev));
> +		if (!st->trig)
> +			return -ENOMEM;
> +
> +		st->trig->ops = &max30210_trigger_ops;
> +		iio_trigger_set_drvdata(st->trig, indio_dev);
> +		ret = devm_iio_trigger_register(dev, st->trig);
> +		if (ret)
> +			return ret;
> +
> +		indio_dev->trig = st->trig;
> +		ret = devm_request_threaded_irq(dev, client->irq,
> +						iio_trigger_generic_data_rdy_poll,
> +						NULL, IRQF_TRIGGER_FALLING,

So we have an unfortunate history of drivers that specify the interrupt
polarity that we can't fix, but for new drivers, that is a job for firmware
not the driver (because there may be an inverter in the path or similar).
Hence most likely flags here should be IRQF_NO_THREAD.

There is no thread, so devm_request_irq() is enough.
However, note that iio_trigger_generic_data_ready_poll() is ultimately kicking
of activity on an interrupt chip (software one) that is buried in the
IIO core. That can't be done correctly from a thread.  Hence the IRQF_NO_THREAD.

> +						indio_dev->name, st->trig);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

	return devm_iio_device_register();

> +}
> +
> +static const struct i2c_device_id max30210_id[] = {
> +	{ "max30210", 0 },
No point in the 0, so  just
	{ "max30210" },

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