Message-ID: <20251013-16642-1563944@bhairav-test.ee.iitb.ac.in>
Date: Mon, 13 Oct 2025 21:36:42 +0530
From: Akhilesh Patil <akhilesh@...iitb.ac.in>
To: Jonathan Cameron <jic23@...nel.org>
Cc: 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 2/2] iio: pressure: adp810: Add driver for adp810 sensor

On Sun, Oct 12, 2025 at 08:36:58PM +0100, Jonathan Cameron wrote:
> On Sat, 11 Oct 2025 17:55:28 +0530
> Akhilesh Patil <akhilesh@...iitb.ac.in> wrote:
> 
> Hi Akhilesh, 
> 
> Thanks for sending this and a late welcome to IIO.

Hi Jonathan, Thanks for the review.
Addressing the comments and suggestions in v2.

> 
> > Add driver for Aosong adp810 differential pressure and
> > temperature sensor. This sensor provides I2C interface for
> > reading data. Calculate CRC of the data received using standard
> > crc8 library to verify data integrity.
> > 
> Wrap commit messages to 75 chars.

I think it is already wrapped to 75. 
Still, I will recheck and fix if required.

> 
> > Tested on TI am62x sk board with sensor connected at i2c-2
> > 
> > Signed-off-by: Akhilesh Patil <akhilesh@...iitb.ac.in>
> 
> Where I've remembered Andy commenting on something I've not duplicated
> (assuming I even noticed the same thing!)
> 
> A few things may contradict or provide alternative suggestions though!
> 
> Jonathan

Okay.

> 
> 
> > diff --git a/drivers/iio/pressure/adp810.c b/drivers/iio/pressure/adp810.c
> > new file mode 100644
> > index 000000000000..ff73330b34fc
> > --- /dev/null
> > +++ b/drivers/iio/pressure/adp810.c
> > @@ -0,0 +1,205 @@
> > +/* Trigger command to send to start measurement by the sensor */
> > +#define ADP810_TRIGGER_COMMAND		0x2d37
> > +#define ADP810_CRC8_POLYNOMIAL		0x31
> > +
> > +DECLARE_CRC8_TABLE(crc_table);
> > +
> > +struct adp810_read_buf {
> > +	u8 dp_msb;
> > +	u8 dp_lsb;
> 
> __be16 dp;
> or u8 dp[2]; 
> 
> > +	u8 dp_crc;
> > +	u8 tmp_msb;
> > +	u8 tmp_lsb;
> 
> __be16_tmp;
> 
> > +	u8 tmp_crc;
> > +	u8 sf_msb;
> > +	u8 sf_lsb;
> 
> __be16 sf;
> 
> > +	u8 sf_crc;
> > +} __packed;
> With packed (which you didn't need previously).
> (more below)

Yes. Used __be16 to indicate big endian.

> 
> > +
> > +struct adp810_data {
> > +	struct i2c_client *client;
> > +	/* Use lock to synchronize access to device during read sequence */
> > +	struct mutex lock;
> > +};
> > +
> > +static int adp810_measure(struct adp810_data *data, struct adp810_read_buf *buf)
> > +{
> > +	struct i2c_client *client = data->client;
> > +	int ret;
> Not sure what ordering you are using for declarations but this looks a bit
> odd.  If nothing else makes more sense go with reverse xmas tree.

okay. I will use "reverse xmas tree" wherever applicable.

> > +	if (buf->tmp_crc != crc8(crc_table, &buf->tmp_msb, 0x2, CRC8_INIT_VALUE)) {
> > +		dev_err(&client->dev, "CRC error for temperature\n");
> > +		return -EIO;
> > +	}
> > +
> > +	if (buf->sf_crc != crc8(crc_table, &buf->sf_msb, 0x2, CRC8_INIT_VALUE)) {
> > +		dev_err(&client->dev, "CRC error for scale\n");
> > +		return -EIO;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int adp810_read_raw(struct iio_dev *indio_dev,
> > +			   struct iio_chan_spec const *chan,
> > +			   int *val, int *val2, long mask)
> > +{
> > +	struct adp810_data *data = iio_priv(indio_dev);
> > +	struct adp810_read_buf buf = {0};
> > +	int ret;
> > +
> > +	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");
> It's normally more informative to use the parent dev for error messages.
> data->client->dev here.
> Probably add a local variable struct device *dev, to avoid making the dev_err() lines
> even longer.

Agree. Will use parent dev in all dev_err() calls.

> 
> > +		return ret;
> > +	}
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		switch (chan->type) {
> > +		case IIO_PRESSURE:
> > +			*val = buf.dp_msb << 8 | buf.dp_lsb;
> 
> They are next to each other so either treating them as a small array or
> I think you can even make the be16 you can use
> 			*val = get_unaligned_be16(buf.dp);
> for all 3 similar cases.  1st and 3rd are actually aligned but
> lets not rely on that.

ACK. Used __be16 along with helpers get_unalinged_be16() to handle
endianess in clean and portable way.

> 
> > +			return IIO_VAL_INT;
> > +		case IIO_TEMP:
> > +			*val = buf.tmp_msb << 8 | buf.tmp_lsb;
> > +			return IIO_VAL_INT;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	case IIO_CHAN_INFO_SCALE:
> > +		switch (chan->type) {
> > +		case IIO_PRESSURE:
> > +			*val = buf.sf_msb << 8 | buf.sf_lsb;
> > +			return IIO_VAL_INT;
> > +		case IIO_TEMP:
> 
> > +static int adp810_probe(struct i2c_client *client)
> > +{
> > +	const struct i2c_device_id *dev_id = i2c_client_get_device_id(client);
> > +	struct device *dev = &client->dev;
> > +	struct iio_dev *indio_dev;
> > +	struct adp810_data *data;
> > +	int ret;
> > +
> > +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	data = iio_priv(indio_dev);
> > +	data->client = client;
> > +	mutex_init(&data->lock);
> Andy mentioned this.  I used not to care about mutex debugging in IIO drivers
> as in most cases lifetimes of the containing structure are so closely aligned
> to the mutex it wasn't worth the extra debugging provided by mutex_destroy().
> 
> Now we can do
> 	ret = devm_mutex_init(&data->lock);
> 	if (ret)
> 		return ret;
> 
> It's so easy I would like to see it used in all new code even when the
> gain is very small.

Okay. Using resource managed mutex_init : devm_mutex_init()

> 
> > +
> > +	indio_dev->name = dev_id->name;
> 
> Just set it to "adp810" here.  We can add more complex handling when the driver
> supports additional parts.  The advantage of an explicit string for now is
> we don't have to think about what it can be.
> 

Sure. Directly using string : indio_dev->name = "adp810";

> > +	indio_dev->channels = adp810_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(adp810_channels);
> > +	indio_dev->info = &adp810_info;
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > +	ret = devm_iio_device_register(dev, indio_dev);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to register IIO device\n");
> > +
> > +	return ret;
> return 0; 
> 
> As that clearly indicates to the reader that you can't get here in an error case.

Yes. Fixed.

Regards,
Akhilesh

Powered by blists - more mailing lists