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

Andy Shevchenko
2025-10-21T15:20:59Z

On Tue, Oct 21, 2025 at 11:20:30AM +0530, Akhilesh Patil wrote:
> Add driver for Aosong adp810 differential pressure and temperature sensor.
> This sensor provides an I2C interface for reading data.
> Calculate CRC of the data received using standard crc8 library to verify
> data integrity.

Thanks for an update! Looks almost good to me, some comments below.

...

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

Putting it here makes checkpatch unhappy. If someone thinks that is a false
positive of the tool, perhaps one needs to fix that.

> +F: drivers/iio/pressure/adp810.c

...

> # When adding new entries keep the list in alphabetical order
> obj-$(CONFIG_ABP060MG) += abp060mg.o
> +obj-$(CONFIG_ADP810) += adp810.o
> obj-$(CONFIG_ROHM_BM1390) += rohm-bm1390.o
> obj-$(CONFIG_BMP280) += bmp280.o
> bmp280-objs := bmp280-core.o bmp280-regmap.o
> @@ -15,6 +16,7 @@ obj-$(CONFIG_DPS310) += dps310.o
> obj-$(CONFIG_IIO_CROS_EC_BARO) += cros_ec_baro.o
> obj-$(CONFIG_HID_SENSOR_PRESS) += hid-sensor-press.o
> obj-$(CONFIG_HP03) += hp03.o
> +obj-$(CONFIG_HP206C) += hp206c.o
> obj-$(CONFIG_HSC030PA) += hsc030pa.o
> obj-$(CONFIG_HSC030PA_I2C) += hsc030pa_i2c.o
> obj-$(CONFIG_HSC030PA_SPI) += hsc030pa_spi.o
> @@ -34,11 +36,9 @@ obj-$(CONFIG_SDP500) += sdp500.o
> obj-$(CONFIG_IIO_ST_PRESS) += st_pressure.o
> st_pressure-y := st_pressure_core.o
> st_pressure-$(CONFIG_IIO_BUFFER) += st_pressure_buffer.o
> +obj-$(CONFIG_IIO_ST_PRESS_I2C) += st_pressure_i2c.o
> +obj-$(CONFIG_IIO_ST_PRESS_SPI) += st_pressure_spi.o
> obj-$(CONFIG_T5403) += t5403.o
> -obj-$(CONFIG_HP206C) += hp206c.o
> obj-$(CONFIG_ZPA2326) += zpa2326.o
> obj-$(CONFIG_ZPA2326_I2C) += zpa2326_i2c.o
> obj-$(CONFIG_ZPA2326_SPI) += zpa2326_spi.o
> -
> -obj-$(CONFIG_IIO_ST_PRESS_I2C) += st_pressure_i2c.o
> -obj-$(CONFIG_IIO_ST_PRESS_SPI) += st_pressure_spi.o

I would split order fix into a separate change, but if maintainers are okay
with this approach, I would not object.

...

> +#include <linux/cleanup.h>
> +#include <linux/crc8.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/dev_printk.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/mutex.h>
> +#include <linux/unaligned.h>

Something is still missing.

...

> +struct adp810_read_buf {
> + __be16 dp;
> + u8 dp_crc;
> + __be16 tmp;
> + u8 tmp_crc;
> + __be16 sf;
> + u8 sf_crc;

All these types are provided in types.h

> +} __packed;

...

> +static int adp810_measure(struct adp810_data *data, struct adp810_read_buf *buf)
> +{
> + struct i2c_client *client = data->client;
> + struct device *dev = &client->dev;
> + int ret;
> + u16 trig_cmd = ADP810_TRIGGER_COMMAND;

Shouldn't this be __be16 or __le16? Or is that really a full 16-bit command?
I have a gut feeling that this should be u8 x[2] = { ... }; instead.

> + /* Send trigger to the sensor for measurement */
> + ret = i2c_master_send(client, (char *)&trig_cmd, sizeof(trig_cmd));
> + if (ret < 0) {
> + dev_err(dev, "Error sending trigger command\n");
> + return ret;
> + }
> + if (ret != sizeof(trig_cmd))
> + return -EIO;

-EIO is defined down from linux/errno.h.

> + /*
> + * Wait for the sensor to acquire data. As per datasheet section 5.3.1,
> + * wait for at least 10ms before reading measurements from the sensor.
> + */
> + msleep(ADP810_MEASURE_LATENCY_MS);
> +
> + /* Read sensor values */
> + ret = i2c_master_recv(client, (char *)buf, sizeof(*buf));
> + if (ret < 0) {
> + dev_err(dev, "Error reading from sensor\n");
> + return ret;
> + }
> + if (ret != sizeof(*buf))
> + return -EIO;
> +
> + /* CRC checks */
> + crc8_populate_msb(crc_table, ADP810_CRC8_POLYNOMIAL);
> + if (buf->dp_crc != crc8(crc_table, (u8 *)&buf->dp, 0x2, CRC8_INIT_VALUE)) {
> + dev_err(dev, "CRC error for pressure\n");
> + return -EIO;
> + }
> +
> + if (buf->tmp_crc != crc8(crc_table, (u8 *)&buf->tmp, 0x2, CRC8_INIT_VALUE)) {
> + dev_err(dev, "CRC error for temperature\n");
> + return -EIO;
> + }
> +
> + if (buf->sf_crc != crc8(crc_table, (u8 *)&buf->sf, 0x2, CRC8_INIT_VALUE)) {
> + dev_err(dev, "CRC error for scale\n");
> + return -EIO;
> + }
> +
> + return 0;
> +}

...

> + indio_dev->num_channels = ARRAY_SIZE(adp810_channels);

ARRAY_SIZE() is defined in a specific header.

--
With Best Regards,
Andy Shevchenko