Re: [PATCH 2/2] iio: pressure: adp810: Add driver for adp810 sensor

Andy Shevchenko
2025-10-11T14:11:37Z

On Sat, Oct 11, 2025 at 3:25 PM Akhilesh Patil <akhilesh@ee.iitb.ac.in> wrote:
>
> Add driver for Aosong adp810 differential pressure and
> temperature sensor. This sensor provides I2C interface for

an I²C

> reading data. Calculate CRC of the data received using standard
> crc8 library to verify data integrity.
>
> Tested on TI am62x sk board with sensor connected at i2c-2

Missing period at the end.

...

> +AOSONG ADP810 DIFFERENTIAL PRESSURE SENSOR DRIVER
> +M: Akhilesh Patil <akhilesh@ee.iitb.ac.in>
> +L: linux-iio@vger.kernel.org
> +S: Maintained
> +F: Documentation/devicetree/bindings/iio/pressure/aosong,adp810.yaml
> +F: drivers/iio/pressure/adp810.c

Some tools will report an orphaned yaml file if you apply patch 1
without patch 2.

...

> +config ADP810
> + tristate "Aosong adp810 differential pressure and temperature sensor"
> + depends on I2C
> + select CRC8
> + help
> + Say yes here to build adp810 differential pressure and temperature
> + sensor driver. ADP810 can measure pressure range up to 500Pa.
> + It supports I2C interface for data communication.

Same as in the commit message.

> + To compile this driver as a module, choose M here: the module will
> + be called adp810

...

> obj-$(CONFIG_IIO_ST_PRESS_I2C) += st_pressure_i2c.o
> obj-$(CONFIG_IIO_ST_PRESS_SPI) += st_pressure_spi.o
> +obj-$(CONFIG_ADP810) += adp810.o

Is Makefile ordered in terms of files and/or configuration options?

> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/crc8.h>

Please,
1) keep it alphabetically ordered;
2) follow the IWYU (Include What You Use) principle.

...

> +/* Time taken in ms by sensor to do measurements after triggering.

/*
* Wrong multi-line comment format. You
* may use this as a reference.
*/

> + * As per datahseet, 10ms is sufficient but we define 30 for better margin

datasheet

Please, respect grammar and punctuation, i.e. again as in the commit
message you made a mistake.

...

> +#define ADP810_MEASURE_LATENCY 30

What's the unit of this value?

...

This needs a comment to explain the choice of this. E.g., reference to
the Datasheet section / chapter.

> +#define ADP810_CRC8_POLYNOMIAL 0x31

...

> +struct adp810_read_buf {
> + u8 dp_msb;
> + u8 dp_lsb;
> + u8 dp_crc;
> + u8 tmp_msb;
> + u8 tmp_lsb;
> + u8 tmp_crc;
> + u8 sf_msb;
> + u8 sf_lsb;
> + u8 sf_crc;
> +} __packed;

Why __packed?

...

> +struct adp810_data {
> + struct i2c_client *client;
> + /* Use lock to synchronize access to device during read sequence */
> + struct mutex lock;
> +};

Is there a deliberate choice to not use regmap I²C APIs?

...

> + /* Wait for sensor to aquire data */

Spell-check. Also the comment is semi-useless, add the reference to
the datasheet or even cite a part of it to explain this.

> + msleep(ADP810_MEASURE_LATENCY);

...

> + mutex_lock(&data->lock);
> + ret = adp810_measure(data, &buf);
> + mutex_unlock(&data->lock);
> +
> + if (ret) {
> + dev_err(&indio_dev->dev, "Failed to read from device\n");
> + return ret;
> + }

Instead, include cleanup,h and use scoped_guard() (and possible
guard()() in some other places, but first answer why not regmap).

...

> + case IIO_CHAN_INFO_RAW:
> + switch (chan->type) {
> + case IIO_PRESSURE:
> + *val = buf.dp_msb << 8 | buf.dp_lsb;

Those have to be properly defined to begin with, i.e. __be16. With
that done, use existing conversion helpers from asm/byteorder.h.

> + return IIO_VAL_INT;
> + case IIO_TEMP:
> + *val = buf.tmp_msb << 8 | buf.tmp_lsb;

Ditto and so on...

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

...

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

Why is dead code required?

...

> + mutex_init(&data->lock);

devm

--
With Best Regards,
Andy Shevchenko