[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20251013-144614-1551316@bhairav-test.ee.iitb.ac.in>
Date: Mon, 13 Oct 2025 20:16:14 +0530
From: Akhilesh Patil <akhilesh@...iitb.ac.in>
To: Andy Shevchenko <andy.shevchenko@...il.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 2/2] iio: pressure: adp810: Add driver for adp810 sensor
On Sat, Oct 11, 2025 at 05:10:58PM +0300, Andy Shevchenko wrote:
Hi Andy,
Thank you for your time and valuable feedback.
Addressed all of them with best of my understanding.
Kindly check my comments on "regmap usage" and "__packed" usage
decisions below.
Sharing v2 of the series with these improvements and fixes.
Regards,
Akhilesh
> On Sat, Oct 11, 2025 at 3:25 PM Akhilesh Patil <akhilesh@...iitb.ac.in> wrote:
> >
> > Add driver for Aosong adp810 differential pressure and
> > temperature sensor. This sensor provides I2C interface for
>
> an I²C
Fixed.
>
> > reading data. Calculate CRC of the data received using standard
> > crc8 library to verify data integrity.
> >
> > Tested on TI am62x sk board with sensor connected at i2c-2
>
> Missing period at the end.
Fixed.
>
> ...
>
> > +AOSONG ADP810 DIFFERENTIAL PRESSURE SENSOR DRIVER
> > +M: Akhilesh Patil <akhilesh@...iitb.ac.in>
> > +L: linux-iio@...r.kernel.org
> > +S: Maintained
> > +F: Documentation/devicetree/bindings/iio/pressure/aosong,adp810.yaml
> > +F: drivers/iio/pressure/adp810.c
>
> Some tools will report an orphaned yaml file if you apply patch 1
> without patch 2.
ACK. checkpatch.pl did show me the warning.
>
> ...
>
> > +config ADP810
> > + tristate "Aosong adp810 differential pressure and temperature sensor"
> > + depends on I2C
> > + select CRC8
> > + help
> > + Say yes here to build adp810 differential pressure and temperature
> > + sensor driver. ADP810 can measure pressure range up to 500Pa.
> > + It supports I2C interface for data communication.
>
> Same as in the commit message.
Fixed grammer and missing article.
>
> > + To compile this driver as a module, choose M here: the module will
> > + be called adp810
>
> ...
>
> > obj-$(CONFIG_IIO_ST_PRESS_I2C) += st_pressure_i2c.o
> > obj-$(CONFIG_IIO_ST_PRESS_SPI) += st_pressure_spi.o
> > +obj-$(CONFIG_ADP810) += adp810.o
>
> Is Makefile ordered in terms of files and/or configuration options?
Sure. Added adp810 at alphabetically correct place.
While at it, I also corrected order of few other enteries.
>
>
> > +#include <linux/module.h>
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/crc8.h>
>
> Please,
> 1) keep it alphabetically ordered;
> 2) follow the IWYU (Include What You Use) principle.
Sure. Fixed it.
>
> ...
>
> > +/* Time taken in ms by sensor to do measurements after triggering.
>
> /*
> * Wrong multi-line comment format. You
> * may use this as a reference.
> */
>
Fixed multiline comment format.
> > + * As per datahseet, 10ms is sufficient but we define 30 for better margin
>
> datasheet
>
> Please, respect grammar and punctuation, i.e. again as in the commit
> message you made a mistake.
Fixed.
>
> ...
>
> > +#define ADP810_MEASURE_LATENCY 30
>
> What's the unit of this value?
It is milliseconds.
Redefined this macro as ADP810_MEASURE_LATENCY_MS for better clarity.
>
> ...
>
> This needs a comment to explain the choice of this. E.g., reference to
> the Datasheet section / chapter.
>
> > +#define ADP810_CRC8_POLYNOMIAL 0x31
Added reference to the section from datasheet with small explanation.
>
> ...
>
> > +struct adp810_read_buf {
> > + u8 dp_msb;
> > + u8 dp_lsb;
> > + u8 dp_crc;
> > + u8 tmp_msb;
> > + u8 tmp_lsb;
> > + u8 tmp_crc;
> > + u8 sf_msb;
> > + u8 sf_lsb;
> > + u8 sf_crc;
> > +} __packed;
>
> Why __packed?
Yes. This is the structure used as a buffer to store sensor values read.
Each entry in this structure should be contiguous in the memory because
reference of this structure will be passed to i2c_master_recv() to
receive and fill the data.
__packed will avoid any compiler generated paddings in the structure to
force alignments on certain architectures. We do not want these paddings
and want our struct members to be sequentially ordered as shown, with
no padding and size of the struct should also be 9 bytes as only 9 bytes of
data should be read from the sensor as per the specification.
I could have used array here. But I preferred strcture for better
readability of the code as one can easily see what values are expected
from sensor while reading and in which order.
>
> ...
>
> > +struct adp810_data {
> > + struct i2c_client *client;
> > + /* Use lock to synchronize access to device during read sequence */
> > + struct mutex lock;
> > +};
>
> Is there a deliberate choice to not use regmap I²C APIs?
Yes. I explored that possibility. However this sensor follows simple I2C
client protocol. It does not expose the concept of I2C registers. It
does not follow smbus. Specifically, while reading the measurement from
the sensor, we need to only send the device address with read bit on the bus,
and start reading 9 bytes following that. That is, no register address
should be sent. I am not sure if regmap API has some hack to achive
similar because these APIs expect register addresses to read/write which
this sensor does not follow. Hence using raw i2c functions. I also
thought regmap abstraction is not needed here as this sensor has very
limited commands to send and not many command/configurations.
>
> ...
>
> > + /* Wait for sensor to aquire data */
>
> Spell-check. Also the comment is semi-useless, add the reference to
> the datasheet or even cite a part of it to explain this.
Sure.
>
> > + msleep(ADP810_MEASURE_LATENCY);
>
> ...
>
> > + 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");
> > + return ret;
> > + }
>
> Instead, include cleanup,h and use scoped_guard() (and possible
> guard()() in some other places, but first answer why not regmap).
ACK. Used scoped_guard() to cleanly handle resource locks here.
>
> ...
>
> > + case IIO_CHAN_INFO_RAW:
> > + switch (chan->type) {
> > + case IIO_PRESSURE:
> > + *val = buf.dp_msb << 8 | buf.dp_lsb;
>
> Those have to be properly defined to begin with, i.e. __be16. With
> that done, use existing conversion helpers from asm/byteorder.h.
>
> > + return IIO_VAL_INT;
> > + case IIO_TEMP:
> > + *val = buf.tmp_msb << 8 | buf.tmp_lsb;
>
> Ditto and so on...
>
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
Agree. We should use standard helpers.
Used be16 along with get_unalinged_be16() to improve this part.
>
> ...
>
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + return -EINVAL;
>
> Why is dead code required?
Removed dead code.
>
> ...
>
> > + mutex_init(&data->lock);
>
> devm
Nice catch.
Fixed this resource leak which can happen upon module unload.
As I do not have .remove callback with distroy mutex in it,
yes this is needed.
>
> --
> With Best Regards,
> Andy Shevchenko
Regards,
Akhilesh
Powered by blists - more mailing lists