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

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

On Fri, Feb 27, 2026 at 12:30:41AM +0800, John Erasmus Mari Geronimo wrote:
> MAX30210 ±0.1°C Accurate Ultra-Small Low-Power Digital Temperature Sensor

Not enough for the commit message.

...

> +#include <asm/div64.h>

linux/math64.h

> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>

> +#include <linux/debugfs.h>

Used?

> +#include <linux/delay.h>

> +#include <linux/errno.h>

You missed err.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/sysfs.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>

Wow! All of them are in use?

> +#include <linux/interrupt.h>
> +#include <linux/log2.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/property.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +#include <linux/stat.h>
> +#include <linux/string.h>
> +#include <linux/unaligned.h>
> +#include <linux/units.h>

...

> +struct max30210_state {
> +	/*
> +	 * Prevent simultaneous access to the i2c client.
> +	 */
> +	struct mutex lock;

> +	struct regmap *regmap;


And if you swap them, won't the binary size be less?

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

Hmm... Don't we have a macro for this nowadays?

> +};

...

> +static const int samp_freq_avail[] = {

Why not 2D array?

> +	0, 15625,
> +	0, 31250,
> +	0, 62500,
> +	0, 125000,
> +	0, 250000,
> +	0, 500000,
> +	1, 0,
> +	2, 0,
> +	4, 0,
> +	8, 0

Leave trailing comma, it's not a terminator.

> +};

...

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

No way. This is variable on stack, not all CPUs / architectures allow this.
And actually why this alignment to begin with? Wouldn't

	__be16 val;

suffice?

> +	int ret;
> +
> +	ret = regmap_bulk_read(regmap, reg, uval, 2);

sizeof()

> +	if (ret)
> +		return ret;
> +
> +	*temp = sign_extend32(get_unaligned_be16(uval), 15);
> +
> +	return IIO_VAL_INT;
> +}

...

> +static void max30210_fifo_read(struct iio_dev *indio_dev)
> +{
> +	struct max30210_state *st = iio_priv(indio_dev);
> +	u32 samp;
> +	int ret, i, j;

Why are 'i' and 'j' signed?


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

'i' is not used outside for-loop, hence

	for (unsigned int i = 0; i < st->watermark; i++) {

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

Reinventing get_unaligned_be32() if I'm not mistaken.

> +		if (samp == MAX30210_FIFO_INVAL_DATA) {
> +			dev_err(&indio_dev->dev, "Invalid data\n");
> +			continue;
> +		}
> +
> +		iio_push_to_buffers(indio_dev, &samp);
> +	}
> +}

...

> +static int max30210_setup(struct max30210_state *st, struct device *dev)
> +{
> +	unsigned int val;
> +
> +	/* Power down to reset device */
> +	st->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> +						     GPIOD_OUT_HIGH);
> +	if (IS_ERR(st->powerdown_gpio))
> +		return dev_err_probe(dev, PTR_ERR(st->powerdown_gpio),
> +				     "Failed to request powerdown GPIO.\n");
> +
> +	/* Power up device */
> +	gpiod_set_value(st->powerdown_gpio, 0);

All delays must be documented. Add a comment with the datasheet reference to
explain the value and need of the sleep.

> +	fsleep(700);

> +	/* Clear status byte */
> +	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;
> +
> +	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(...);

> +	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;
> +
> +	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,
> +						indio_dev->name, st->trig);
> +		if (ret)
> +			return ret;
> +	}

> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

Wouldn't

	return devm_iio_device_register(dev, indio_dev);

suffice?

> +}

...

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

No ', 0' part.

> +	{ }
> +};

...

Can somebody at Analog start a common internal Wiki or other resources
and collect there typical requirements for the code in IIO? It will prevent
reviewers and maintainers from doing the same replies again and again.

-- 
With Best Regards,
Andy Shevchenko