Re: [PATCH RFC 4/4] iio: adc: ad7380: add support for multiple scan type

[Jonathan Cameron <jic23@kernel.org>]

On Tue,  7 May 2024 14:02:08 -0500
David Lechner <dlechner@baylibre.com> wrote:

> The AD783x chips have a resolution boost feature that allows for 2
> extra bits of resolution. Previously, we had to choose a scan type to
> fit the largest resolution and manipulate the raw data to fit when the
> resolution was lower. This patch adds support for multiple scan types
> for the voltage input channels so that we can support both resolutions
> without having to manipulate the raw data.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
A few comments inline.

> ---
>  drivers/iio/adc/ad7380.c | 185 ++++++++++++++++++++++-------------------------
>  1 file changed, 86 insertions(+), 99 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
> index e240098708e9..ca317e3a72d9 100644
> --- a/drivers/iio/adc/ad7380.c
> +++ b/drivers/iio/adc/ad7380.c
> @@ -89,14 +89,22 @@ struct ad7380_chip_info {
>  	const struct ad7380_timing_specs *timing_specs;
>  };
>  
> -/*
> - * realbits/storagebits cannot be dynamically changed, so in order to
> - * support the resolution boost (additional 2  bits of resolution)
> - * we need to set realbits/storagebits to the maximum value i.e :
> - *   - realbits = 16 + 2 = 18, and storagebits = 32 for 16-bits chips
> - *   - realbits = 14 + 2 = 16, and storagebits = 16 for 14-bits chips
> - * We need to adjust the scale depending on resolution boost status
> - */
> +/** scan type for 14-bit chips with resolution boost enabled. */
> +static const struct iio_scan_type ad7380_scan_type_14_boost = {
> +	.sign = 's',
> +	.realbits = 16,
> +	.storagebits = 16,
> +	.endianness = IIO_CPU,
> +};
> +
> +/** scan type for 16-bit chips with resolution boost enabled. */
Not kernel-doc. Fix all these.

> +static const struct iio_scan_type ad7380_scan_type_16_boost = {
> +	.sign = 's',
> +	.realbits = 18,
> +	.storagebits = 32,
> +	.endianness = IIO_CPU,
> +};
> +
>  #define AD7380_CHANNEL(index, bits, diff) {			\
>  	.type = IIO_VOLTAGE,					\
>  	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
> @@ -113,10 +121,12 @@ struct ad7380_chip_info {
>  	.scan_index = (index),					\
>  	.scan_type = {						\
>  		.sign = 's',					\
> -		.realbits = (bits) + 2,				\
> -		.storagebits = ((bits) + 2 > 16) ? 32 : 16,	\
> +		.realbits = (bits),				\
> +		.storagebits = ((bits) > 16) ? 32 : 16,		\
>  		.endianness = IIO_CPU,				\
>  	},							\
> +	.ext_scan_type = &ad7380_scan_type_##bits##_boost,	\
> +	.num_ext_scan_type = 1,					\
>  }
>  
>  #define DEFINE_AD7380_2_CHANNEL(name, bits, diff)	\
> @@ -376,67 +386,62 @@ static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
>  	unreachable();
>  }
>  
> -static int ad7380_prepare_spi_xfer(struct ad7380_state *st, struct spi_transfer *xfer)
> +/**
This isn't kernel-doc, so /* only

> + * Reads one set of samples from the device. This is a simultaneous sampling
> + * chip, so all channels are always read at the same time.
> + *
> + * On successful return, the raw data is stored in st->scan_data.raw.
> + */
> +static int ad7380_read_one_sample(struct ad7380_state *st,
> +				  const struct iio_scan_type *scan_type)

>  
>  static irqreturn_t ad7380_trigger_handler(int irq, void *p)
>  {
>  	struct iio_poll_func *pf = p;
>  	struct iio_dev *indio_dev = pf->indio_dev;
> +	const struct iio_chan_spec *chan = &indio_dev->channels[0];
> +	const struct iio_scan_type *scan_type = iio_get_current_scan_type(
> +								indio_dev, chan);

As below, pull iio_get_current_scan_type( down to the line below.


> @@ -496,18 +475,14 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
>  			   struct iio_chan_spec const *chan,
>  			   int *val, int *val2, long info)
>  {
> +	const struct iio_scan_type *scan_type = iio_get_current_scan_type(
> +								indio_dev, chan);

Pull the iio_get_current_scan_type( down to the next line and use one tab.

>  	struct ad7380_state *st = iio_priv(indio_dev);
> -	int realbits;
> -
> -	if (st->resolution_boost_enable == RESOLUTION_BOOST_ENABLE)
> -		realbits = chan->scan_type.realbits;
> -	else
> -		realbits = chan->scan_type.realbits - 2;
>  
>  	switch (info) {
>  	case IIO_CHAN_INFO_RAW:
>  		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> -			return ad7380_read_direct(st, chan, val);
> +			return ad7380_read_direct(st, chan, scan_type, val);
>  		}
>  		unreachable();
>  	case IIO_CHAN_INFO_SCALE:
> @@ -520,7 +495,7 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
>  		 * According to IIO ABI, offset is applied before scale,
>  		 * so offset is: vcm_mv / scale
>  		 */
> -		*val = st->vcm_mv[chan->channel] * (1 << realbits)
> +		*val = st->vcm_mv[chan->channel] * (1 << scan_type->realbits)
>  			/ st->vref_mv;
>  
>  		return IIO_VAL_INT;
> @@ -700,6 +675,17 @@ static int ad7380_write_raw(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +static const struct iio_scan_type *ad7380_get_current_scan_type(
> +		const struct iio_dev *indio_dev, struct iio_chan_spec const *chan)
> +{
> +	struct ad7380_state *st = iio_priv(indio_dev);
> +
> +	if (st->resolution_boost_enable && chan->num_ext_scan_type)

I'd put all the scan types in ext_scan_type, then pick rather than falling back
to the main scan_type.

> +		return chan->ext_scan_type;
> +
> +	return &chan->scan_type;
> +}
> +

>