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

Akhilesh Patil
2025-10-13T16:06:54Z

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@ee.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@ee.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