| 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: <e260e47064955085cb18124e9d0f18af8ac04b00.camel@gmail.com>
Date: Thu, 10 Apr 2025 10:32:24 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: Marcelo Schmitt <marcelo.schmitt@...log.com>, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Cc: jic23@...nel.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 v1 3/7] iio: adc: ad4170: Add support for buffered data
capture
On Wed, 2025-04-09 at 09:25 -0300, Marcelo Schmitt 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>
> ---
> drivers/iio/adc/ad4170.c | 170 ++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 169 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/adc/ad4170.c b/drivers/iio/adc/ad4170.c
> index 0d24286ac2ab..5ffcdedf3e7f 100644
> --- a/drivers/iio/adc/ad4170.c
> +++ b/drivers/iio/adc/ad4170.c
> @@ -10,10 +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>
> @@ -323,6 +325,16 @@ struct ad4170_state {
> 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.
> + */
> + __be32 rx_buf __aligned(IIO_DMA_MINALIGN);
> + u8 tx_buf[2];
> };
>
> static void ad4170_fill_sps_tbl(struct ad4170_state *st)
> @@ -882,6 +894,7 @@ static const struct iio_chan_spec ad4170_channel_template
> = {
> .scan_type = {
> .realbits = 24,
> .storagebits = 32,
> + .shift = 8,
> .endianness = IIO_BE,
> },
> };
> @@ -1480,11 +1493,29 @@ static int ad4170_write_raw_get_fmt(struct iio_dev
> *indio_dev,
> }
> }
>
> +static int ad4170_update_scan_mode(struct iio_dev *indio_dev,
> + const unsigned long *active_scan_mask)
> +{
> + struct ad4170_state *st = iio_priv(indio_dev);
> + unsigned int chan_index;
> + int ret;
> +
> + guard(mutex)(&st->lock);
> +
Do we need this? In theory we should be fine since you do
iio_device_claim_direct() in ad4170_read_sample().
Otherwise I would expect the same locking rules in postenable() and
predisable().
> + iio_for_each_active_channel(indio_dev, chan_index) {
> + ret = ad4170_set_channel_enable(st, chan_index, true);
> + if (ret)
> + return ret;
> + }
> + return 0;
> +}
> +
> static const struct iio_info ad4170_info = {
> .read_raw = ad4170_read_raw,
> .read_avail = ad4170_read_avail,
> .write_raw = ad4170_write_raw,
> .write_raw_get_fmt = ad4170_write_raw_get_fmt,
> + .update_scan_mode = ad4170_update_scan_mode,
> .debugfs_reg_access = ad4170_debugfs_reg_access,
> };
>
> @@ -1759,6 +1790,130 @@ static int ad4170_initial_config(struct iio_dev
> *indio_dev)
> AD4170_ADC_CTRL_MULTI_DATA_REG_SEL_MSK);
> }
>
> +static int ad4170_prepare_spi_message(struct ad4170_state *st)
> +{
> + /*
> + * Continuous data register read is enabled on buffer postenable so
> + * no instruction phase is needed meaning we don't need to send the
> + * register address to read data. Transfer only needs the read
> buffer.
> + */
> + st->xfer.rx_buf = &st->rx_buf;
> + st->xfer.len =
> BITS_TO_BYTES(ad4170_channel_template.scan_type.realbits);
> +
> + spi_message_init_with_transfers(&st->msg, &st->xfer, 1);
> +
> + return devm_spi_optimize_message(&st->spi->dev, st->spi, &st->msg);
> +}
> +
> +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
> + * be in a continuous conversion mode.
> + */
> + return regmap_update_bits(st->regmap16, 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;
> +
> + /*
> + * To exit continuous read, write 0xA5 to the ADC during the first 8
> + * SCLKs of the ADC data read.
> + */
> + st->tx_buf[0] = AD4170_ADC_CTRL_CONT_READ_EXIT;
> + st->tx_buf[1] = 0;
> + ret = spi_write(st->spi, st->tx_buf, 2);
> + if (ret)
> + return ret;
Couldn't we still use regmap? Like
regmap_write(st->regmap8, AD4170_ADC_CTRL_CONT_READ_EXIT, 0)?
Likely fails on the register ranges and not sure it's any better... Still not a
fan of mixing "raw" bus read/writes and regmap().
> +
> + ret = regmap_update_bits(st->regmap16, 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;
> +}
> +
> +static bool ad4170_validate_scan_mask(struct iio_dev *indio_dev,
> + const unsigned long *scan_mask)
> +{
> + unsigned int masklength = iio_get_masklength(indio_dev);
> + unsigned long first, next;
> +
> + /*
> + * The channel sequencer cycles through the enabled channels in
> + * sequential order, from channel 0 to channel 15, bypassing disabled
> + * channels. When more than one channel is enabled, channel 0 must
> + * always be enabled. See datasheet channel_en register description
> at
> + * page 95.
> + */
> + first = find_next_bit(scan_mask, masklength, 0);
> + next = find_next_bit(scan_mask, masklength, first + 1);
> + if (next < masklength)
> + return test_bit(0, scan_mask);
Hmm, maybe this is simpler?
if (bitmap_weight(scan_mask, masklength) > 1)
return test_bit(0, scan_mask);
return true;
- Nuno Sá
Powered by blists - more mailing lists