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

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

On Fri, Sep 11, 2026 at 06:12:17AM +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.

...

Shouldn't Kconfig select REGMAP_I2C?

...

The header block doesn't fully follow IWYU.

+ array_size.h // ARRAY_SIZE()
+ bitops.h // BIT(), sign_extend32(), et cetera

> +#include <linux/bitfield.h>

+ delay.h // fsleep()

> +#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>

It's better to group them out...

+ minmax.h // min*(), et cetera

> +#include <linux/regmap.h>

+ types.h // uXX, __le16, et cetera

> +#include <linux/unaligned.h>
> +#include <linux/units.h>
> +

...somewhere here.

...

> +struct max30210_state {
> +	struct regmap *regmap;

> +	u8 watermark;

I would swap to make it easier to read the array (which will be aligned).

> +	/* Raw FIFO byte buffer */
> +	u8 fifo_buf[MAX30210_FIFO_BYTES_PER_SAMPLE * MAX30210_FIFO_SIZE];
> +};

...

> +static const struct regmap_config max30210_regmap = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = MAX30210_PART_ID_REG,

No cache? Why?

> +};

...

> +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);
> +	if (ret) {
> +		dev_err(&indio_dev->dev, "Failed to read from fifo.\n");
> +		return;
> +	}
> +
> +	for (unsigned int i = 0; i < st->watermark; i++) {
> +		u32 raw = get_unaligned_be24(&st->fifo_buf[MAX30210_FIFO_BYTES_PER_SAMPLE * i]);
> +
> +		if (raw == MAX30210_FIFO_INVAL_DATA) {
> +			dev_err_ratelimited(&indio_dev->dev, "Invalid data\n");
> +			continue;
> +		}
> +
> +		s16 temp = (s16)FIELD_GET(MAX30210_FIFO_TEMP_MASK, raw);

Defined variable at the proper space, also better to use sign_extend() instead
of casting.

> +
> +		iio_push_to_buffers(indio_dev, &temp);
> +	}
> +}

...

> +static irqreturn_t max30210_irq_handler(int irq, void *dev_id)
> +{
> +	struct iio_dev *indio_dev = dev_id;
> +	struct max30210_state *st = iio_priv(indio_dev);
> +	unsigned int status;
> +	int ret;
> +
> +	ret = regmap_read(st->regmap, MAX30210_STATUS_REG, &status);
> +	if (ret) {
> +		dev_err(&indio_dev->dev, "Status read failed\n");

Imagine if this is an intermittent issue, this will flood the logs very quickly.

> +		return IRQ_NONE;
> +	}
> +
> +	if (status & MAX30210_STATUS_A_FULL_MASK)
> +		max30210_fifo_read(indio_dev);
> +
> +	if (status & MAX30210_STATUS_TEMP_HI_MASK)
> +		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_STATUS_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));
> +
> +	return IRQ_HANDLED;
> +}

...

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

KILO? MILLI?

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

No min_t(). Also add a new temporary variable for the value from the register,
so semantically uval wouldn't be overloaded.

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

10 * USEC_PER_MSEC (will require time.h to be included).

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

Split logically...

			      struct iio_chan_spec const *chan,
			      int val, int val2, long mask)

(I am surprised that this is v3 and too many mistakes that linux-iio@ mailing
 list is full of. You may take your time and read the mailing list archives to
 understand the most common style and API mistakes in IIO contributions.)

> +{
> +	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;
> +
> +		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 ssize_t hwfifo_watermark_show(struct device *dev,
> +				     struct device_attribute *devattr,
> +				     char *buf)
> +{
> +	struct max30210_state *st = iio_priv(dev_to_iio_dev(dev));
> +
> +	return sysfs_emit(buf, "%d\n", st->watermark);
> +}

^^^ (here, see below)

> +
> +IIO_STATIC_CONST_DEVICE_ATTR(hwfifo_watermark_min, "1");
> +IIO_STATIC_CONST_DEVICE_ATTR(hwfifo_watermark_max,
> +			     __stringify(MAX30210_FIFO_SIZE));

> +static IIO_DEVICE_ATTR_RO(hwfifo_watermark, 0);

Move this closer to the callback.

...

> +	ret = devm_regulator_get_enable(dev, "vdd");
> +	if (ret)

> +		return dev_err_probe(dev, ret,
> +				     "Failed to enable vdd regulator.\n");

It's fine to have it on a single line.

...

> +static const struct i2c_device_id max30210_id[] = {
> +	{ "max30210" },

C99 initialisers.

> +	{ }
> +};

-- 
With Best Regards,
Andy Shevchenko