Re: [PATCH v4 2/5] iio: adc: ti-adc128s052: Use shift and realbits

[Jonathan Cameron]

On Sat, 14 Jun 2025 02:15:01 -0700
Sukrut Bellary <sbellary@xxxxxxxxxxxx> wrote:

> This adcxx communicates with a host processor via an SPI/Microwire Bus
> interface. The device family responds with 12-bit data, of which the LSB bits
> are transmitted by the lower resolution devices as 0. The unavailable bits are
> 0 in LSB. Shift is calculated per resolution and used in scaling and raw data
> read.
> 
> Create a separate structure for each device type instead of an array.
> These changes help to reuse the driver to support the family of devices with
> name ADC<bb><c>S<sss>, where
> * bb is the resolution in number of bits (8, 10, 12)
> * c is the number of channels (1, 2, 4, 8)
> * sss is the maximum conversion speed (021 for 200 kSPS, 051 for 500 kSPS
> and 101 for 1 MSPS)
> 
> Complete datasheets are available at TI's website here:
> https://www.ti.com/lit/ds/symlink/adc<bb><c>s<sss>.pdf
> 
> Co-developed-by: Nishanth Menon <nm@xxxxxx>
> Signed-off-by: Nishanth Menon <nm@xxxxxx>
> Signed-off-by: Sukrut Bellary <sbellary@xxxxxxxxxxxx>
> ---
>  drivers/iio/adc/ti-adc128s052.c | 115 ++++++++++++++++++--------------
>  1 file changed, 66 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
> index 1b46a8155803..2b206745e53d 100644
> --- a/drivers/iio/adc/ti-adc128s052.c
> +++ b/drivers/iio/adc/ti-adc128s052.c
> @@ -41,13 +41,14 @@ struct adc128 {
>  	} __aligned(IIO_DMA_MINALIGN);
>  };
>  
> -static int adc128_adc_conversion(struct adc128 *adc, u8 channel)
> +static int adc128_adc_conversion(struct adc128 *adc,
> +				 struct iio_chan_spec const *channel)
>  {
>  	int ret;
>  
>  	guard(mutex)(&adc->lock);
>  
> -	adc->buffer[0] = channel << 3;
> +	adc->buffer[0] = channel->channel << 3;
>  	adc->buffer[1] = 0;
>  
>  	ret = spi_write(adc->spi, &adc->buffer, sizeof(adc->buffer));
> @@ -58,7 +59,10 @@ static int adc128_adc_conversion(struct adc128 *adc, u8 channel)
>  	if (ret < 0)
>  		return ret;
>  
> -	return be16_to_cpu(adc->buffer16) & 0xFFF;
> +	ret = (be16_to_cpu(adc->buffer16) >> channel->scan_type.shift) &
> +	       GENMASK(channel->scan_type.realbits - 1, 0);
> +
Even though it is a bit long I'd go with

	return (be16_to_cpu(adc->buffer16) >> channel->scan_type.shift) &
		GENMASK();

> +	return ret;
>  }
>  
>  static int adc128_read_raw(struct iio_dev *indio_dev,
> @@ -71,7 +75,7 @@ static int adc128_read_raw(struct iio_dev *indio_dev,
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
>  
> -		ret = adc128_adc_conversion(adc, channel->channel);
> +		ret = adc128_adc_conversion(adc, channel);
>  		if (ret < 0)
>  			return ret;
>  
> @@ -81,7 +85,7 @@ static int adc128_read_raw(struct iio_dev *indio_dev,
>  	case IIO_CHAN_INFO_SCALE:
>  
>  		*val = adc->vref_mv;
> -		*val2 = 12;
> +		*val2 = channel->scan_type.realbits;
>  		return IIO_VAL_FRACTIONAL_LOG2;
>  
>  	default:
> @@ -90,15 +94,24 @@ static int adc128_read_raw(struct iio_dev *indio_dev,
>  
>  }
>  
> -#define ADC128_VOLTAGE_CHANNEL(num)	\
> -	{ \
> -		.type = IIO_VOLTAGE, \
> -		.indexed = 1, \
> -		.channel = (num), \
> -		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> -		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) \
> +#define _ADC128_VOLTAGE_CHANNEL(num, real_bits)				\
> +	{								\

I would minimise the churn and stick to existing style of one space then \
I don't think we have any specific style guidance around this.

> +		.type = IIO_VOLTAGE,					\
> +		.indexed = 1,						\
> +		.channel = (num),					\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> +		.scan_index = (num),					\
> +		.scan_type = {						\
> +			.sign = 'u',					\
> +			.realbits = (real_bits),			\
> +			.storagebits = 16,				\
> +			.shift = (12 - real_bits),			\
> +		},							\
>  	}
>  
> +#define ADC128_VOLTAGE_CHANNEL(num) _ADC128_VOLTAGE_CHANNEL(num, 12)

I wonder if it would be clearer to just have the 12 explicit in each entry
and skip this two levels of macro thing?

> +
>  static const struct iio_chan_spec adc128s052_channels[] = {
>  	ADC128_VOLTAGE_CHANNEL(0),
>  	ADC128_VOLTAGE_CHANNEL(1),
> @@ -124,26 +137,30 @@ static const struct iio_chan_spec adc124s021_channels[] = {
>  
>  static const char * const bd79104_regulators[] = { "iovdd" };
>  
> -static const struct adc128_configuration adc128_config[] = {
> -	{
> -		.channels = adc128s052_channels,
> -		.num_channels = ARRAY_SIZE(adc128s052_channels),
> -		.refname = "vref",
> -	}, {
> -		.channels = adc122s021_channels,
> -		.num_channels = ARRAY_SIZE(adc122s021_channels),
> -		.refname = "vref",
> -	}, {
> -		.channels = adc124s021_channels,
> -		.num_channels = ARRAY_SIZE(adc124s021_channels),
> -		.refname = "vref",
> -	}, {
> -		.channels = adc128s052_channels,
> -		.num_channels = ARRAY_SIZE(adc128s052_channels),
> -		.refname = "vdd",
> -		.other_regulators = &bd79104_regulators,
> -		.num_other_regulators = 1,
> -	},
> +static const struct adc128_configuration adc122s021_config = {
> +	.channels = adc122s021_channels,
> +	.num_channels = ARRAY_SIZE(adc122s021_channels),
> +	.refname = "vref",
> +};

Ideal would be to have this as a precursor patch rather than adding complexity
to this one which is focused on the bits related stuff.

It's a good change to have but does make it harder to spot the main
content in here.


> +
> +static const struct adc128_configuration adc124s021_config = {
> +	.channels = adc124s021_channels,
> +	.num_channels = ARRAY_SIZE(adc124s021_channels),
> +	.refname = "vref",
> +};