| 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: <20250504185724.4700284f@jic23-huawei>
Date: Sun, 4 May 2025 18:57:24 +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-kernel@...r.kernel.org>, <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>, <marcelo.schmitt1@...il.com>
Subject: Re: [PATCH v2 3/7] iio: adc: ad4170: Add support for buffered data
capture
On Mon, 28 Apr 2025 09:28:20 -0300
Marcelo Schmitt <marcelo.schmitt@...log.com> wrote:
> Extend the AD4170 driver to allow buffered data capture in continuous read
> mode. In continuous read mode, the chip skips the instruction phase and
> outputs just ADC sample data, enabling faster sample rates to be reached.
> The internal channel sequencer always starts sampling from channel 0 and
> channel 0 must be enabled if more than one channel is selected for data
> capture. The scan mask validation callback checks the aforementioned
> condition is met.
>
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@...log.com>
A few minor things spotted inline.
Thanks,
Jonathan
> ---
> changes since v1
> - Using bitmap_weight().
> - rx_buf changed from __be32 to u8 array to better cope with new regmap config.
>
> drivers/iio/adc/Kconfig | 2 +
> drivers/iio/adc/ad4170.c | 199 ++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 199 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index d5d0308da007..9b4787c127fc 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -75,6 +75,8 @@ config AD4170
> tristate "Analog Device AD4170 ADC Driver"
> depends on SPI
> select REGMAP_SPI
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
> help
> Say yes here to build support for Analog Devices AD4170 SPI analog
> to digital converters (ADC).
> diff --git a/drivers/iio/adc/ad4170.c b/drivers/iio/adc/ad4170.c
> index 4d0af15cb48d..5fcf1c023ac2 100644
> --- a/drivers/iio/adc/ad4170.c
> +++ b/drivers/iio/adc/ad4170.c
> @@ -10,8 +10,12 @@
> #include <linux/delay.h>
> #include <linux/device.h>
> #include <linux/err.h>
> +#include <linux/iio/buffer.h>
> #include <linux/iio/iio.h>
> #include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> #include <linux/interrupt.h>
> #include <linux/irq.h>
> #include <linux/kernel.h>
> @@ -54,6 +58,7 @@
> #define AD4170_FILTER_FS_REG(x) (0xC7 + 14 * (x))
> #define AD4170_OFFSET_REG(x) (0xCA + 14 * (x))
> #define AD4170_GAIN_REG(x) (0xCD + 14 * (x))
> +#define AD4170_ADC_CTRL_CONT_READ_EXIT_REG 0x200 /* virtual reg */
>
> #define AD4170_REG_READ_MASK BIT(14)
>
> @@ -221,6 +226,7 @@ static const unsigned int ad4170_reg_size[] = {
> [AD4170_OFFSET_REG(5) ... AD4170_GAIN_REG(5)] = 3,
> [AD4170_OFFSET_REG(6) ... AD4170_GAIN_REG(6)] = 3,
> [AD4170_OFFSET_REG(7) ... AD4170_GAIN_REG(7)] = 3,
> + [AD4170_ADC_CTRL_CONT_READ_EXIT_REG] = 0,
> };
>
> enum ad4170_ref_buf {
> @@ -347,12 +353,16 @@ struct ad4170_state {
> u32 int_pin_sel;
> int sps_tbl[ARRAY_SIZE(ad4170_filt_names)][AD4170_MAX_FS_TBL_SIZE][2];
> struct completion completion;
> + struct iio_trigger *trig;
> + struct spi_transfer xfer;
> + struct spi_message msg;
> + __be32 bounce_buffer[AD4170_MAX_CHANNELS];
> /*
> * DMA (thus cache coherency maintenance) requires the transfer buffers
> * to live in their own cache lines.
> */
> u8 tx_buf[AD4170_SPI_MAX_XFER_LEN] __aligned(IIO_DMA_MINALIGN);
> - u8 rx_buf[AD4170_SPI_MAX_XFER_LEN];
> + u8 rx_buf[4];
Why this change?
> };
> +
> +static int ad4170_buffer_postenable(struct iio_dev *indio_dev)
> +{
> + struct ad4170_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + ret = ad4170_set_mode(st, AD4170_ADC_CTRL_MODE_CONT);
> + if (ret < 0)
> + return ret;
> +
> + /*
> + * Enables continuous data register read.
> + * This enables continuous read of the ADC Data register. The ADC must
First two sentences seem to say the same thing.
> + * be in a continuous conversion mode.
> + */
> + return regmap_update_bits(st->regmap, AD4170_ADC_CTRL_REG,
> + AD4170_ADC_CTRL_CONT_READ_MSK,
> + FIELD_PREP(AD4170_ADC_CTRL_CONT_READ_MSK,
> + AD4170_ADC_CTRL_CONT_READ_ENABLE));
> +}
> +
> +static int ad4170_buffer_predisable(struct iio_dev *indio_dev)
> +{
> + struct ad4170_state *st = iio_priv(indio_dev);
> + int ret, i;
> +
> + /*
> + * Use a high register address (virtual register) to request a write of
> + * 0xA5 to the ADC during the first 8 SCLKs of the ADC data read cycle,
> + * thus exiting continuous read.
> + */
> + ret = regmap_write(st->regmap, AD4170_ADC_CTRL_CONT_READ_EXIT_REG, 0);
> +
> + ret = regmap_update_bits(st->regmap, AD4170_ADC_CTRL_REG,
> + AD4170_ADC_CTRL_CONT_READ_MSK,
> + FIELD_PREP(AD4170_ADC_CTRL_CONT_READ_MSK,
> + AD4170_ADC_CTRL_CONT_READ_DISABLE));
> + if (ret)
> + return ret;
> +
> + ret = ad4170_set_mode(st, AD4170_ADC_CTRL_MODE_IDLE);
> + if (ret)
> + return ret;
> +
> + /*
> + * The ADC sequences through all the enabled channels (see datasheet
> + * page 95). That can lead to incorrect channel being read if a
> + * single-shot read (or buffered read with different active_scan_mask)
> + * is done after buffer disable. Disable all channels so only requested
> + * channels will be read.
> + */
> + for (i = 0; i < indio_dev->num_channels; i++) {
> + ret = ad4170_set_channel_enable(st, i, false);
> + if (ret)
> + return ret;
> + }
> + return ret;
return 0; When we know we are in a good path we should make that clear.
> +}
Powered by blists - more mailing lists