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

Akhilesh Patil
2025-10-25T05:28:43Z

On Tue, Oct 21, 2025 at 06:20:52PM +0300, Andy Shevchenko wrote:
> 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.

Thanks for the below comments and suggestions.
Addressed them in v4.

Regards,
Akhilesh

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

This is being followed up in v1 thread.
https://lore.kernel.org/lkml/aPs6raLIcM3QbQXJ@smile.fi.intel.com/

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

Okay. Adding only adp810.o here. Will revert all other entries.
This will make this change logically connected to this series.

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

Added more include files as pointed out further below.

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

ACK. Added linux/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.

Ohh yes. Nice catch !
This is 16 bit trigger command. We need to send 0x37 then 0x2d.
I defined trig_cmd = 0x2d37 which will work fine on little endian
systems, but will fail on big endian systems I think.
I use ARM v8-A configured in little endian, so did not catch this.
Let me use u8 trig_cmd[2] = {0x37, 0x2d} to make this architecture
agnostic.

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

ACK. Added 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;
> > +
> > +
> > + 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.

ACK. Added linux/array_size.h

>
> --
> With Best Regards,
> Andy Shevchenko
>
>