| 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: <20250622150845.1558da7e@jic23-huawei>
Date: Sun, 22 Jun 2025 15:08:45 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Marcelo Schmitt <marcelo.schmitt@...log.com>
Cc: <linux-iio@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-gpio@...r.kernel.org>, <linux-kernel@...r.kernel.org>, Ana-Maria
Cusco <ana-maria.cusco@...log.com>, <lars@...afoo.de>,
<Michael.Hennerich@...log.com>, <dlechner@...libre.com>,
<nuno.sa@...log.com>, <andy@...nel.org>, <robh@...nel.org>,
<krzk+dt@...nel.org>, <conor+dt@...nel.org>, <linus.walleij@...aro.org>,
<brgl@...ev.pl>, <broonie@...nel.org>, <lgirdwood@...il.com>,
<marcelo.schmitt1@...il.com>
Subject: Re: [PATCH v6 02/12] iio: adc: Add basic support for AD4170
On Wed, 18 Jun 2025 14:35:38 -0300
Marcelo Schmitt <marcelo.schmitt@...log.com> wrote:
> From: Ana-Maria Cusco <ana-maria.cusco@...log.com>
>
> The AD4170 is a multichannel, low noise, 24-bit precision sigma-delta
> analog to digital converter. The AD4170 design offers a flexible data
> acquisition solution with crosspoint multiplexed analog inputs,
> configurable ADC voltage reference inputs, ultra-low noise integrated PGA,
> digital filtering, wide range of configurable output data rates, internal
> oscillator and temperature sensor, four GPIOs, and integrated features for
> interfacing with load cell weigh scales, RTD, and thermocouple sensors.
>
> Add basic support for the AD4170 ADC with the following features:
> - Single-shot read.
> - Analog front end PGA configuration.
> - Differential and pseudo-differential input configuration.
>
> Signed-off-by: Ana-Maria Cusco <ana-maria.cusco@...log.com>
> Co-developed-by: Marcelo Schmitt <marcelo.schmitt@...log.com>
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@...log.com>
One comment directing you back to some stuff that came in after you'd
posted this but was in the v5 thread. The other is a random musing on
whether this should just use spi_write_then_read() to simplify things
at the cost of a few extra data copies. For that I'm not that fussed
so it is entirely up to you.
Jonathan
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 6516ccb4d63b..d99c35ff9a1c 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_AD4000) += ad4000.o
> obj-$(CONFIG_AD4030) += ad4030.o
> obj-$(CONFIG_AD4080) += ad4080.o
> obj-$(CONFIG_AD4130) += ad4130.o
> +obj-$(CONFIG_AD4170) += ad4170.o
> obj-$(CONFIG_AD4695) += ad4695.o
> obj-$(CONFIG_AD4851) += ad4851.o
> obj-$(CONFIG_AD7091R) += ad7091r-base.o
> diff --git a/drivers/iio/adc/ad4170.c b/drivers/iio/adc/ad4170.c
> new file mode 100644
> index 000000000000..58716ad6e7fc
> --- /dev/null
> +++ b/drivers/iio/adc/ad4170.c
> +static int ad4170_reg_write(void *context, unsigned int reg, unsigned int val)
> +{
> + struct ad4170_state *st = context;
> + unsigned int size;
> + int ret;
> +
> + ret = ad4170_get_reg_size(st, reg, &size);
> + if (ret)
> + return ret;
> +
> + put_unaligned_be16(reg, st->tx_buf);
> + switch (size) {
> + case 3:
> + put_unaligned_be24(val, &st->tx_buf[AD4170_SPI_INST_PHASE_LEN]);
> + break;
> + case 2:
> + put_unaligned_be16(val, &st->tx_buf[AD4170_SPI_INST_PHASE_LEN]);
> + break;
> + case 1:
> + st->tx_buf[AD4170_SPI_INST_PHASE_LEN] = val;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return spi_write(st->spi, st->tx_buf, AD4170_SPI_INST_PHASE_LEN + size);
If we did use the below suggestion and switch the read path to spi_write_then_read()
we could do the same here (with zero size write).
Perhaps neither is a change worth doing though. Up to you.
> +}
> +
> +static int ad4170_reg_read(void *context, unsigned int reg, unsigned int *val)
> +{
> + struct ad4170_state *st = context;
> + struct spi_transfer t[] = {
> + {
> + .tx_buf = st->tx_buf,
> + .len = AD4170_SPI_INST_PHASE_LEN,
> + },
> + {
> + .rx_buf = st->rx_buf,
> + },
I wonder.... Would we better off just using spi_write_then_read() here
given they are all fairly small register sizes. The big advantage
is we can skip messing around with dma safe buffers as spi_write_then_read
always bounces the data through some buffers the SPI core creates for this
purpose.
> + };
> + unsigned int size;
> + int ret;
> +
> + ret = ad4170_get_reg_size(st, reg, &size);
> + if (ret)
> + return ret;
> +
> + put_unaligned_be16(AD4170_REG_READ_MASK | reg, st->tx_buf);
> + t[1].len = size;
> +
> + ret = spi_sync_transfer(st->spi, t, ARRAY_SIZE(t));
> + if (ret)
> + return ret;
> +
> + switch (size) {
> + case 3:
> + *val = get_unaligned_be24(st->rx_buf);
> + return 0;
> + case 2:
> + *val = get_unaligned_be16(st->rx_buf);
> + return 0;
> + case 1:
> + *val = st->rx_buf[0];
> + return 0;
> + default:
> + return -EINVAL;
> + }
> +}
> +static int ad4170_parse_adc_channel_type(struct device *dev,
> + struct fwnode_handle *child,
> + struct iio_chan_spec *chan)
> +{
> + const char *propname, *propname2;
> + int ret, ret2;
> + u32 pins[2];
> +
> + /* Parse pseudo-differential channel configuration */
> + propname = "single-channel";
> + propname2 = "common-mode-channel";
> + ret = fwnode_property_read_u32(child, propname, &pins[0]);
> + ret2 = fwnode_property_read_u32(child, propname2, &pins[1]);
> + if (!ret && ret2)
> + return dev_err_probe(dev, ret,
ret isn't appropriate to use here as it's 0 so you'll report success.
> + "When %s is defined, %s must be defined too\n",
> + propname, propname2);
> +
> + if (!ret && !ret2) {
See feedback following from Dan's email in v5 thread that came in just after you'd
posted this.
Andy was keen that we do this differently and I think his suggestion makes sense.
> + chan->differential = false;
> + chan->channel = pins[0];
> + chan->channel2 = pins[1];
> + return 0;
> + }
> + /* Failed to parse pseudo-diff chan props so try diff chan */
> +
> + /* Parse differential channel configuration */
> + propname2 = "diff-channels";
> + ret = fwnode_property_read_u32_array(child, propname2, pins,
> + ARRAY_SIZE(pins));
> + if (!ret) {
> + chan->differential = true;
> + chan->channel = pins[0];
> + chan->channel2 = pins[1];
> + return 0;
> + }
> + return dev_err_probe(dev, ret, "Channel must define one of %s or %s.\n",
> + propname, propname2);
> +}
Powered by blists - more mailing lists