Message-ID: <CA+GgBR9v-FGYk-6fhC7cCcQKcQqoKac+oRcUvhU8qKG5T0GHvg@mail.gmail.com>
Date: Tue, 27 Aug 2024 16:53:03 +0300
From: Alexandru Ardelean <aardelean@...libre.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, 
	devicetree@...r.kernel.org, krzk+dt@...nel.org, robh@...nel.org, 
	lars@...afoo.de, michael.hennerich@...log.com, gstols@...libre.com
Subject: Re: [PATCH 7/7] iio: adc: ad7606: add support for AD7606C-{16,18} parts

On Fri, Aug 23, 2024 at 10:19 PM Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Mon, 19 Aug 2024 09:47:17 +0300
> Alexandru Ardelean <aardelean@...libre.com> wrote:
>
> > The AD7606C-16 and AD7606C-18 are pretty similar with the AD7606B.
> > The main difference between AD7606C-16 & AD7606C-18 is the precision in
> > bits (16 vs 18).
> > Because of that, some scales need to be defined for the 18-bit variants, as
> > they need to be computed against 2**18 (vs 2**16 for the 16 bit-variants).
> >
> > Because the AD7606C-16,18 also supports bipolar & differential channels,
> > for SW-mode, the default range of 10 V or ±10V should be set at probe.
> > On reset, the default range (in the registers) is set to value 0x3 which
> > corresponds to '±10 V single-ended range', regardless of bipolar or
> > differential configuration.
> >
> > Aside from the scale/ranges, the AD7606C-16 is similar to the AD7606B.
> >
> > And the AD7606C-18 variant offers 18-bit precision. The unfortunate effect
> > of this 18-bit sample size, is that there is no simple/neat way to get the
> > samples into a 32-bit array without having to do a home-brewed bit-buffer.
> > The ADC must read all samples (from all 8 channels) in order to get the
> > N-th sample (this could be reworked to do up-to-N-th sample for scan-direct).
> > There doesn't seem to be any quick-trick to be usable to pad the samples
> > up to at least 24 bits.
> > Even the optional status-header is 8-bits, which would mean 26-bits of data
> > per sample.
> > That means that when using a simple SPI controller (which can usually read
> > 8 bit multiples) a simple bit-buffer trick is required.
> >
> > Datasheet links:
> >   https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-16.pdf
> >   https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-18.pdf
> >
> > Signed-off-by: Alexandru Ardelean <aardelean@...libre.com>
>
> A few minor things. If we can just start with 18 bit word spi controllers only
> maybe that's worth doing to make things simpler.

Will go for 18 bit word SPI controllers-only for now.

>
> > +static int ad7606c_sw_mode_setup_channels(struct iio_dev *indio_dev,
> > +                                       ad7606c_chan_setup_cb_t chan_setup_cb)
> > +{
> > +     unsigned int num_channels = indio_dev->num_channels - 1;
> > +     struct ad7606_state *st = iio_priv(indio_dev);
> > +     bool chan_configured[AD760X_MAX_CHANNELS];
> = {};
> and drop the memset.

ack

>
> > +     struct device *dev = st->dev;
> > +     int ret;
> > +     u32 ch;
> > +
> > +     /* We need to hook this first */
> > +     ret = st->bops->sw_mode_config(indio_dev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     indio_dev->info = &ad7606c_info_sw_mode;
> > +
> > +     memset(chan_configured, 0, sizeof(chan_configured));
> > +
> > +     device_for_each_child_node_scoped(dev, child) {
> > +             bool bipolar, differential;
> > +
> > +             ret = fwnode_property_read_u32(child, "reg", &ch);
> > +             if (ret)
> > +                     continue;
> > +
> > +             if (ch >= num_channels) {
> > +                     dev_warn(st->dev,
> > +                              "Invalid channel number (ignoring): %d\n", ch);
> > +                     continue;
> > +             }
> > +
> > +             bipolar = fwnode_property_present(child, "bipolar");
> > +             differential = fwnode_property_present(child, "diff-channel");
> > +
> > +             chan_setup_cb(st, ch, bipolar, differential);
> > +             chan_configured[ch] = true;
> > +     }
> > +
> > +     /* Apply default configuration to unconfigured (via DT) channels */
> > +     for (ch = 0; ch < num_channels; ch++) {
> > +             struct ad7606_chan_scale *cs;
> > +             unsigned int *scale_avail_show;
> > +             int i;
> > +
> > +             if (!chan_configured[ch])
> > +                     chan_setup_cb(st, ch, false, false);
> > +
> > +             /* AD7606C supports different scales per channel */
> > +             cs = &st->chan_scales[ch];
> > +
> > +             scale_avail_show = devm_kcalloc(st->dev, cs->num_scales * 2,
> > +                                             sizeof(*scale_avail_show),
> > +                                             GFP_KERNEL);
>
> Maybe just make it big enough for worst case and stick it in st always?
> How big can it get?

So, that would be 16 channels x 8 bytes-per-scale x 5 = 640 bytes.
Not too bad.

>
>
>
> > +             if (!scale_avail_show)
> > +                     return -ENOMEM;
> > +
> > +             /* Generate a scale_avail list for showing to userspace */
> > +             for (i = 0; i < cs->num_scales; i++) {
> > +                     scale_avail_show[i * 2] = 0;
> > +                     scale_avail_show[i * 2 + 1] = cs->scale_avail[i];
> > +             }
> > +
> > +             cs->scale_avail_show = scale_avail_show;
> > +     }
> > +
> > +     return 0;
> > +}
> >
>
> > diff --git a/drivers/iio/adc/ad7606_spi.c b/drivers/iio/adc/ad7606_spi.c
> > index dd0075c97c24..73a7b0007bf8 100644
> > --- a/drivers/iio/adc/ad7606_spi.c
> > +++ b/drivers/iio/adc/ad7606_spi.c
> > @@ -45,6 +45,8 @@
>
> >
> > +static int ad7606_spi_read_block18to32(struct device *dev,
> > +                                    int count, void *buf)
> > +{
> > +     struct spi_device *spi = to_spi_device(dev);
> > +     u32 i, bit_buffer, buf_size, bit_buf_size;
> > +     u32 *data = buf;
> > +     u8 *bdata = buf;
> > +     int j, ret;
> > +
> > +     /**
> Not kernel doc.  /*
> > +      * With the 18 bit ADC variants (here) is that we can't assume that all
> > +      * SPI controllers will pad 18-bit sequences into 32-bit arrays,
> > +      * so we need to do a bit of buffer magic here.
> > +      * Alternatively, we can have a variant of this function that works
> > +      * for SPI controllers that can pad 18-bit samples into 32-bit arrays.
> > +      */
> > +
> > +     /* Write 'count' bytes to the right, to not overwrite samples */
> > +     bdata += count;
> > +
> > +     /* Read 24 bits only, as we'll only get samples of 18 bits each */
> > +     buf_size = count * 3;
> > +     ret = spi_read(spi, bdata, buf_size);
> > +     if (ret < 0) {
> > +             dev_err(&spi->dev, "SPI read error\n");
> > +             return ret;
> > +     }
> > +
> > +     bit_buffer = 0;
> > +     bit_buf_size = 0;
> > +     for (j = 0, i = 0; i < buf_size; i++) {
> > +             u32 sample;
> > +
> > +             bit_buffer = (bit_buffer << 8) | bdata[i];
> > +             bit_buf_size += 8;
> > +
> > +             if (bit_buf_size < 18)
> > +                     continue;
> > +
> > +             bit_buf_size -= 18;
> > +             sample = (bit_buffer >> bit_buf_size) & AD7606C_18_SAMPLE_MASK;
> > +             data[j++] = sign_extend32(sample, 17);
> > +
> > +             if (j == count)
> > +                     break;
> > +     }
> > +
> > +     return 0;
> > +}
>

Powered by blists - more mailing lists