RE: [PATCH 1/3] iio: adc: ltc2309: introduce chip_info structure

[Jones, Carlos jr]

> On Fri, 20 Mar 2026 22:08:17 +0800
> Carlos Jones Jr <carlosjr.jones@xxxxxxxxxx> wrote:
> 
> > This is a preparatory patch that introduces a chip_info structure to
> > the LTC2309 driver to facilitate adding support for additional chip
> > variants with different channel configurations and timing
> > requirements.
> >
> > The chip_info structure contains chip-specific data including the
> > channel specifications, number of channels, and read delay timing.
> > This change does not modify the existing LTC2309 functionality.
> >
> > Signed-off-by: Carlos Jones Jr <carlosjr.jones@xxxxxxxxxx>
> Hi Carlos,
> 
> Firstly welcome to IIO!
> 
> A few comments inline. Some overlap with Andy's review.
> 
> Jonathan
> 

Thank you for the warm welcome, Jonathan. 

> > ---
> >  drivers/iio/adc/ltc2309.c | 24 ++++++++++++++++++++++--
> >  1 file changed, 22 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iio/adc/ltc2309.c b/drivers/iio/adc/ltc2309.c
> > index 5f0d947d0615..4ea25873398c 100644
> > --- a/drivers/iio/adc/ltc2309.c
> > +++ b/drivers/iio/adc/ltc2309.c
> > @@ -8,6 +8,7 @@
> >   * Copyright (c) 2023, Liam Beguin <liambeguin@xxxxxxxxx>
> >   */
> >  #include <linux/bitfield.h>
> > +#include <linux/delay.h>
> >  #include <linux/i2c.h>
> >  #include <linux/iio/iio.h>
> >  #include <linux/kernel.h>
> > @@ -26,18 +27,26 @@
> >  #define LTC2309_DIN_UNI		BIT(3)
> >  #define LTC2309_DIN_SLEEP	BIT(2)
> >
> > +struct ltc2309_chip_info {
> > +	const struct iio_chan_spec *channels;
> 
> We now have __counted_by_ptr so you can use that marking to make it
> explicit that num_channels is telling us how many elements channels has.
> 

Thanks for the instruction, I will apply it to the code.

> 
> >   */
> >  struct ltc2309 {
> >  	struct device		*dev;
> >  	struct i2c_client	*client;
> >  	struct mutex		lock; /* serialize data access */
> >  	int			vref_mv;
> > +	const struct ltc2309_chip_info *chip_info;
> >  };
> >
> >  /* Order matches expected channel address, See datasheet Table 1. */
> > @@ -117,6 +126,10 @@ static int ltc2309_read_raw_channel(struct
> ltc2309 *ltc2309,
> >  		return ret;
> >  	}
> >
> > +	if (ltc2309->chip_info->read_delay_us)
> > +		usleep_range(ltc2309->chip_info->read_delay_us,
> > +			     ltc2309->chip_info->read_delay_us * 2);
> 
> Andy covered this. fsleep() provides standard tolerance on usleeps if we don't
> care about precise timing (and given it's a sleep we never get precise timing
> anyway!)
> 

Noted and thanks.

> > +	unsigned int num_channels;
> > +	unsigned int read_delay_us;
> > +};
> > +
> >  /**
> >   * struct ltc2309 - internal device data structure
> >   * @dev:	Device reference
> >   * @client:	I2C reference
> >   * @lock:	Lock to serialize data access
> >   * @vref_mv:	Internal voltage reference
> > + * @chip_info:	Chip-specific configuration data
> See below. Maybe more appropriate to copy the read_delay rather than
> keeping pointer to full structure around.
>
> > +
> >  	ret = i2c_master_recv(ltc2309->client, (char *)&buf, 2);
> >  	if (ret < 0) {
> >  		dev_err(ltc2309->dev, "i2c read failed: %pe\n", ERR_PTR(ret));
> @@
> > -156,6 +169,12 @@ static const struct iio_info ltc2309_info = {
> >  	.read_raw = ltc2309_read_raw,
> >  };
> >
> > +static const struct ltc2309_chip_info ltc2309_chip_info = {
> > +	.channels = ltc2309_channels,
> > +	.num_channels = ARRAY_SIZE(ltc2309_channels),
> > +	.read_delay_us = 0,
> > +};
> > +
> >  static int ltc2309_probe(struct i2c_client *client)  {
> >  	struct iio_dev *indio_dev;
> > @@ -169,11 +188,12 @@ static int ltc2309_probe(struct i2c_client *client)
> >  	ltc2309 = iio_priv(indio_dev);
> >  	ltc2309->dev = &indio_dev->dev;
> >  	ltc2309->client = client;
> > +	ltc2309->chip_info = &ltc2309_chip_info;
> 
> Given only the read_delay_us is used after probe, I'd add a variable for that
> and copy just that value over.  If you have other changes that are coming in
> the near future that will add more fields to the structure that are needed after
> probe, then fine to leave it as you have it (but add a mention in the commit
> message).
> 

As, I have no visibility into the other similar devices that may require more
fields to the structure, let me revert the structure changes and use a local
variable to deal with the read_delay_us instead. Thanks.

> >
> >  	indio_dev->name = "ltc2309";
> 
> Given we try to present the actual device name in sysfs, I'd expect to see the
> name coming from the chip_info structure as well.
> 

Will do, thank you.

> >  	indio_dev->modes = INDIO_DIRECT_MODE;
> > -	indio_dev->channels = ltc2309_channels;
> > -	indio_dev->num_channels = ARRAY_SIZE(ltc2309_channels);
> > +	indio_dev->channels = ltc2309->chip_info->channels;
> > +	indio_dev->num_channels = ltc2309->chip_info->num_channels;
> >  	indio_dev->info = &ltc2309_info;
> >
> >  	ret = devm_regulator_get_enable_read_voltage(&client->dev, "vref");