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

[Jonathan Cameron]

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

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

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


>   */
>  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!)

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


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

>  	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");