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

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

On Mon, Oct 13, 2025 at 08:16:14PM +0530, Akhilesh Patil wrote:
> On Sat, Oct 11, 2025 at 05:10:58PM +0300, Andy Shevchenko wrote:
> > On Sat, Oct 11, 2025 at 3:25 PM Akhilesh Patil <akhilesh@ee.iitb.ac.in> wrote:

...

> > > +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.

Right, but in this form packed only affects the last member size (due to
alignment), in any case since it's HW mandated requirement, perhaps add a
comment. (Since we also going to use __be16 types, the __packed is required
for that to be properly placed in the memory.)


...

> > > +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.

Ah, makes sense.

-- 
With Best Regards,
Andy Shevchenko