| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20240907162032.3cca0a17@jic23-huawei>
Date: Sat, 7 Sep 2024 16:20:32 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Alexandru Ardelean <aardelean@...libre.com>
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 v5 9/9] iio: adc: ad7606: add support for
AD7606C-{16,18} parts
On Sat, 7 Sep 2024 09:50:42 +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.
>
> The AD7606C-18 variant offers 18-bit precision. Because of this, the
> requirement to use this chip is that the SPI controller supports padding
> of 18-bit sequences to 32-bit arrays.
>
> 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
>
Keep automation happy and make these official tags.
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-16.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-18.pdf
> Signed-off-by: Alexandru Ardelean <aardelean@...libre.com>
A few other things inline.
Jonathan
> ---
> drivers/iio/adc/ad7606.c | 237 ++++++++++++++++++++++++++++++++---
> drivers/iio/adc/ad7606.h | 13 +-
> drivers/iio/adc/ad7606_spi.c | 55 ++++++++
> 3 files changed, 284 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
> index 364f16fb96bf..67bac6c97fff 100644
> --- a/drivers/iio/adc/ad7606.c
> +++ b/drivers/iio/adc/ad7606.c
> +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] = {};
Maybe use a bitmap as then...
> + struct device *dev = st->dev;
> + int ret;
> + u32 ch;
> +
> + /* We call this first, so that the proper SW scales get assigned */
> + ret = st->bops->sw_mode_config(indio_dev);
> + if (ret)
> + return ret;
> +
> + device_for_each_child_node_scoped(dev, child) {
> + bool bipolar, differential;
> + u32 pins[2];
> +
> + ret = fwnode_property_read_u32(child, "reg", &ch);
> + if (ret)
> + continue;
> +
> + /* channel number (here) is from 1 to num_channels */
> + if (ch == 0 || ch > num_channels) {
> + dev_warn(st->dev,
> + "Invalid channel number (ignoring): %d\n", ch);
> + continue;
> + }
> +
> + bipolar = fwnode_property_read_bool(child, "bipolar");
> +
> + ret = fwnode_property_read_u32_array(child, "diff-channels",
> + pins, ARRAY_SIZE(pins));
> + /* Channel is differential, if pins are the same as 'reg' */
> + if (ret == 0 && (pins[0] != ch || pins[1] != ch)) {
> + dev_err(st->dev,
> + "Differential pins must be the same as 'reg'");
> + return -EINVAL;
> + }
> +
> + differential = (ret == 0);
> +
> + ch--;
> +
> + 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++) {
this can be for_each_clear_bit()
> + if (!chan_configured[ch])
> + chan_setup_cb(st, ch, false, false);
> + }
> +
> + return 0;
> +}
> +
> +static int ad7606_sw_mode_setup(struct iio_dev *indio_dev, unsigned int id)
> {
> unsigned int num_channels = indio_dev->num_channels - 1;
> struct ad7606_state *st = iio_priv(indio_dev);
> @@ -572,17 +751,30 @@ static int ad7606_sw_mode_setup(struct iio_dev *indio_dev)
>
> indio_dev->info = &ad7606_info_sw_mode;
>
> - /* Scale of 0.076293 is only available in sw mode */
> - /* After reset, in software mode, ±10 V is set by default */
> - for (ch = 0; ch < num_channels; ch++) {
> - struct ad7606_chan_scale *cs = &st->chan_scales[ch];
> + switch (id) {
> + case ID_AD7606C_18:
> + ret = ad7606c_sw_mode_setup_channels(indio_dev,
> + ad7606c_18_chan_setup);
As below, a callback in chip info that can be called directly
would be preferable here.
> + break;
> + case ID_AD7606C_16:
> + ret = ad7606c_sw_mode_setup_channels(indio_dev,
> + ad7606c_16_chan_setup);
> + break;
> + default:
> + /* Scale of 0.076293 is only available in sw mode */
> + /* After reset, in software mode, ±10 V is set by default */
> + for (ch = 0; ch < num_channels; ch++) {
> + struct ad7606_chan_scale *cs = &st->chan_scales[ch];
> +
> + cs->scale_avail = ad7616_sw_scale_avail;
> + cs->num_scales = ARRAY_SIZE(ad7616_sw_scale_avail);
> + cs->range = 2;
> + }
>
> - cs->scale_avail = ad7616_sw_scale_avail;
> - cs->num_scales = ARRAY_SIZE(ad7616_sw_scale_avail);
> - cs->range = 2;
> + ret = st->bops->sw_mode_config(indio_dev);
> + break;
> }
>
> - ret = st->bops->sw_mode_config(indio_dev);
> if (ret)
> return ret;
>
> @@ -631,9 +823,16 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
> st->oversampling = 1;
>
> cs = &st->chan_scales[0];
> - cs->range = 0;
> - cs->scale_avail = ad7606_scale_avail;
> - cs->num_scales = ARRAY_SIZE(ad7606_scale_avail);
> + switch (id) {
> + case ID_AD7606C_18:
> + cs->scale_avail = ad7606_18bit_hw_scale_avail;
Can we push this into the chip_info structure?
Ideally we'd get away form any ID based code selection but if there is
some already in the driver that can be for another day.
Let's not make it worse though.
> + cs->num_scales = ARRAY_SIZE(ad7606_18bit_hw_scale_avail);
> + break;
> + default:
> + cs->scale_avail = ad7606_16bit_hw_scale_avail;
> + cs->num_scales = ARRAY_SIZE(ad7606_16bit_hw_scale_avail);
> + break;
> + }
> diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
> index fa175cff256c..c2cb536ecef1 100644
> --- a/drivers/iio/adc/ad7606.h
> +++ b/drivers/iio/adc/ad7606.h
> /**
> @@ -151,9 +154,13 @@ struct ad7606_state {
> /*
> * DMA (thus cache coherency maintenance) may require the
> * transfer buffers to live in their own cache lines.
> - * 16 * 16-bit samples + 64-bit timestamp
> + * 16 * 16-bit samples + 64-bit timestamp - for AD7616
> + * 8 * 32-bit samples + 64-bit timestamp - for AD7616C-18 (and similar)
> */
> - unsigned short data[20] __aligned(IIO_DMA_MINALIGN);
> + union {
> + unsigned short d16[20];
> + unsigned int d32[10];
Can we use fixed sizes for these?
u16 and u32 for instance?
> + } data __aligned(IIO_DMA_MINALIGN);
> __be16 d16[2];
That's odd. You have a d16 inside the union and another one right next to it.
Maybe rename the new one? The other is a bounce buffer used in the spi
paths I think.
> };
Powered by blists - more mailing lists