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

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

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