On Tue, 7 May 2024 14:02:08 -0500
David Lechner <dlechner@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
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;
> +}
> +
>
