Re: [PATCH 3/3] iio: adc: ti-ads112c14: add support for buffered read

[Jonathan Cameron]

On Tue, 14 Jul 2026 19:19:32 -0500
"David Lechner (TI)" <dlechner@xxxxxxxxxxxx> wrote:

> Add support for buffered reads using a triggered buffer.
> 
> The device has a continuous conversion mode, but that can only be used
> with one channel at a time since there is nothing like a sequencer to
> support that in hardware. Instead, we use single-shot reads like we do
> for direct reads to be able to read multiple channels.
> 
> Support for continuous conversion mode could be added in the future if
> needed via a 2nd buffer.

How about enabling that if only one channel is requested?  I vaguely recall
us doing that for another driver (though I might be dreaming :)  I did
see your comment in the cover letter about it affecting timing and that
making life complex.  Fine to leave considering this for another day
but maybe don't suggest a particular solution here.

Talk a little in here about why you parse the crc on to userspace.
I think that makes sense but good to have a record of your reasoning.
We aren't telling userspace it is there afterall, it's just in some
left over space!

> 
> Signed-off-by: David Lechner (TI) <dlechner@xxxxxxxxxxxx>
> ---
>  drivers/iio/adc/ti-ads112c14.c | 103 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 99 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads112c14.c b/drivers/iio/adc/ti-ads112c14.c
> index 99ccacd6d56f..1404ca31324b 100644
> --- a/drivers/iio/adc/ti-ads112c14.c
> +++ b/drivers/iio/adc/ti-ads112c14.c
> @@ -15,7 +15,10 @@
>  #include <linux/dev_printk.h>
>  #include <linux/device/devres.h>
>  #include <linux/i2c.h>
> +#include <linux/iio/buffer.h>
>  #include <linux/iio/iio.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
>  #include <linux/math64.h>
>  #include <linux/minmax.h>
>  #include <linux/module.h>
> @@ -29,6 +32,9 @@
>  #include <linux/unaligned.h>
>  #include <linux/units.h>
>  
> +/* Arbitrary limit since channels are dynamic. */
> +#define ADS112C14_MAX_MEASUREMENT_CHANNELS 16
> +
>  /* Datasheet t_d(RST) - time to wait after reset before next I2C use. */
>  #define ADS112C14_DELAY_RESET_US 500
>  
> @@ -255,6 +261,8 @@ struct ads112c14_data {
>  	u32 num_measurements;
>  	u8 sys_mon_chan_short_gain_val;
>  	int sys_mon_chan_short_scale_available[ARRAY_SIZE(ads112c14_pga_gains_x10)][2];
> +	IIO_DECLARE_BUFFER_WITH_TS(__be32, scan, ADS112C14_MAX_MEASUREMENT_CHANNELS +
> +						 ARRAY_SIZE(ads112c14_sys_mon_channels));
>  };
>  
>  static bool ads112c14_writeable_reg(struct device *dev, unsigned int reg)
> @@ -575,7 +583,7 @@ static int ads112c14_prepare_sys_mon_channel(struct ads112c14_data *data,
>  
>  static int ads112c14_single_conversion(struct ads112c14_data *data,
>  				       const struct iio_chan_spec *chan,
> -				       u8 *buf)
> +				       u8 *buf, bool for_scan)
>  {
>  	struct i2c_client *client = to_i2c_client(regmap_get_device(data->regmap));
>  	u32 reg_val;
> @@ -605,6 +613,22 @@ static int ads112c14_single_conversion(struct ads112c14_data *data,
>  	if (ret)
>  		return ret;
>  
> +	/*
> +	 * When doing buffered read, we don't check the CRC, but rather pass it
> +	 * along with the raw data.

Perhaps say why.

> +	 */
> +	if (for_scan) {
> +		u8 len = BITS_TO_BYTES(data->chip_info->resolution_bits) +
> +			 (data->i2c_crc_enabled ? 1 : 0);
> +
> +		ret = i2c_smbus_read_i2c_block_data(client, ADS112C14_CMD_RDATA,
> +						    len, buf);
> +		if (ret < 0)
> +			return ret;
> +
> +		return 0;
> +	}
> +


> @@ -908,7 +967,7 @@ static int ads112c14_parse_channels(struct iio_dev *indio_dev,
>  		return -ENOMEM;
>  
>  	channels = devm_kcalloc(dev, num_child_nodes +
> -				ARRAY_SIZE(ads112c14_sys_mon_channels),
> +				ARRAY_SIZE(ads112c14_sys_mon_channels) + 1,
>  				sizeof(*channels), GFP_KERNEL);
>  	if (!channels)
>  		return -ENOMEM;
> @@ -1069,14 +1128,44 @@ static int ads112c14_parse_channels(struct iio_dev *indio_dev,
>  		if (spec->type == IIO_RESISTANCE)
>  			spec->differential = 0;
>  
> +		spec->scan_type = (struct iio_scan_type){
> +			.format = measurement->bipolar ?
> +				  IIO_SCAN_FORMAT_SIGNED_INT :
> +				  IIO_SCAN_FORMAT_UNSIGNED_INT,
> +			.realbits = data->chip_info->resolution_bits,
> +			.storagebits = 32,
> +			.shift = 32 - data->chip_info->resolution_bits,
> +			.endianness = IIO_BE,
> +		};
> +
>  		i++;
>  	}
>  
>  	data->num_measurements = i;
> +	if (data->num_measurements >= ADS112C14_MAX_MEASUREMENT_CHANNELS)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "too many measurement channels defined\n");
> +
>  	memcpy(channels + i, ads112c14_sys_mon_channels, sizeof(ads112c14_sys_mon_channels));
>  
> +	for (u32 j = 0; j < ARRAY_SIZE(ads112c14_sys_mon_channels); j++) {
> +		struct iio_chan_spec *spec = &channels[i + j];
> +

Add a comment to say this is updating elements of the template.

> +		spec->scan_index = i + j;
> +		spec->scan_type = (struct iio_scan_type){
> +			.format = IIO_SCAN_FORMAT_SIGNED_INT,
> +			.realbits = data->chip_info->resolution_bits,
> +			.storagebits = 32,
> +			.shift = 32 - data->chip_info->resolution_bits,
> +			.endianness = IIO_BE,
> +		};
> +	}
> +
>  	indio_dev->channels = channels;
> -	indio_dev->num_channels = i + ARRAY_SIZE(ads112c14_sys_mon_channels);
> +	indio_dev->num_channels = i + ARRAY_SIZE(ads112c14_sys_mon_channels) + 1;
> +
> +	i = indio_dev->num_channels - 1;

If this is different from doing

i += ARRAY_SIZE(ads112c14_sys_mon_channels); and using that above and here
(with a increment after to account for this) then add a comment on why.

Otherwise I'd prefer that form as it puts it in the same scheme as the channel
increments above.


> +	channels[i] = IIO_CHAN_SOFT_TIMESTAMP(i);
>  
>  	return 0;
>  }