Message-ID: <20251025-52830-939994@bhairav-test.ee.iitb.ac.in>
Date: Sat, 25 Oct 2025 10:58:30 +0530
From: Akhilesh Patil <akhilesh@...iitb.ac.in>
To: Andy Shevchenko <andriy.shevchenko@...el.com>
Cc: jic23@...nel.org, dlechner@...libre.com, robh@...nel.org,
	krzk+dt@...nel.org, conor+dt@...nel.org, nuno.sa@...log.com,
	andy@...nel.org, marcelo.schmitt1@...il.com, vassilisamir@...il.com,
	salah.triki@...il.com, skhan@...uxfoundation.org,
	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
	devicetree@...r.kernel.org, akhileshpatilvnit@...il.com
Subject: Re: [PATCH v3 2/2] iio: pressure: adp810: Add driver for adp810
 sensor

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@...iitb.ac.in>
> > +L:	linux-iio@...r.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
> 
> 

Powered by blists - more mailing lists