Re: [PATCH 2/2] iio: pressure: adp810: Add driver for adp810 sensor
From: Andy Shevchenko
Date: Sat Oct 11 2025 - 10:11:49 EST
On Sat, Oct 11, 2025 at 3:25 PM Akhilesh Patil <akhilesh@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> +L: linux-iio@xxxxxxxxxxxxxxx
> +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