From: Andy Shevchenko <andriy.shevchenko@intel.com>
To: radu.sabau@analog.com
Cc: "Lars-Peter Clausen" <lars@metafoo.de>,
"Michael Hennerich" <Michael.Hennerich@analog.com>,
"Jonathan Cameron" <jic23@kernel.org>,
"David Lechner" <dlechner@baylibre.com>,
"Nuno Sá" <nuno.sa@analog.com>,
"Andy Shevchenko" <andy@kernel.org>,
"Rob Herring" <robh@kernel.org>,
"Krzysztof Kozlowski" <krzk+dt@kernel.org>,
"Conor Dooley" <conor+dt@kernel.org>,
"Uwe Kleine-König" <ukleinek@kernel.org>,
"Liam Girdwood" <lgirdwood@gmail.com>,
"Mark Brown" <broonie@kernel.org>,
"Linus Walleij" <linusw@kernel.org>,
"Bartosz Golaszewski" <brgl@kernel.org>,
"Philipp Zabel" <p.zabel@pengutronix.de>,
linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-pwm@vger.kernel.org,
linux-gpio@vger.kernel.org
Subject: Re: [PATCH v3 3/4] iio: adc: ad4691: add triggered buffer support
Date: Fri, 13 Mar 2026 13:13:11 +0200 [thread overview]
Message-ID: <abPxR0TVa70sMg38@ashevche-desk.local> (raw)
In-Reply-To: <20260313-ad4692-multichannel-sar-adc-driver-v3-3-b4d14d81a181@analog.com>
On Fri, Mar 13, 2026 at 12:07:27PM +0200, Radu Sabau via B4 Relay wrote:
> Add buffered capture support using the IIO triggered buffer framework.
>
> Both operating modes share a single IIO trigger and trigger handler.
> The handler builds a complete scan — one u32 slot per channel at its
> scan_index position, followed by a timestamp — and pushes it to the
> IIO buffer in a single iio_push_to_buffers_with_ts() call.
>
> For CNV Clock Mode the GP0 pin is configured as DATA_READY output. The
> IRQ handler stops conversions and fires the IIO trigger; the trigger
> handler reads accumulated results from the AVG_IN registers via regmap
> and restarts conversions for the next cycle.
>
> For Manual Mode there is no DATA_READY signal; CNV is tied to SPI CS
> so conversions are triggered by CS assertion rather than by a dedicated
> pin. The standard iio-trig-hrtimer module is not used because the timer
> period must be derived from the SPI clock rate and the number of active
> channels: the pipelined protocol requires N+1 SPI transfers per scan
> (the first result is garbage and is discarded), so the minimum period
> depends on both the SPI speed and the live channel count at buffer
> enable time. A driver-private hrtimer whose period is recomputed by
> buffer_postenable is simpler and avoids requiring the user to configure
> an external trigger with the correct hardware-derived period.
>
> Manual mode channels use storagebits=32 (shift=8, realbits=16) so all
> channel slots in the scan buffer are uniformly sized regardless of the
> SPI wire format (24-bit transfer, 16-bit ADC data in bits[23:8]).
...
> #include <linux/device.h>
> #include <linux/err.h>
> #include <linux/reset.h>
This is unordered.
> +#include <linux/hrtimer.h>
> #include <linux/interrupt.h>
> #include <linux/math.h>
> #include <linux/module.h>
...
> +#include <linux/iio/buffer.h>
> #include <linux/iio/iio.h>
>
Unneeded blank line.
> -#define AD4691_ACC_COUNT_VAL 0x3F
> +#define AD4691_ACC_COUNT_VAL 0x01
No ping-pong, and actually this was not used at all. So, make sure you add
constants when they are really started being used.
> #define AD4691_GPIO_MODE1_REG 0x196
> #define AD4691_GPIO_MODE2_REG 0x197
> #define AD4691_GPIO_READ 0x1A0
...
> static const struct iio_chan_spec ad4691_manual_channels[] = {
> - AD4691_CHANNEL(0, 0, 16, 24, 8),
> - AD4691_CHANNEL(1, 1, 16, 24, 8),
> - AD4691_CHANNEL(2, 2, 16, 24, 8),
> - AD4691_CHANNEL(3, 3, 16, 24, 8),
> - AD4691_CHANNEL(4, 4, 16, 24, 8),
> - AD4691_CHANNEL(5, 5, 16, 24, 8),
> - AD4691_CHANNEL(6, 6, 16, 24, 8),
> - AD4691_CHANNEL(7, 7, 16, 24, 8),
> - AD4691_CHANNEL(8, 8, 16, 24, 8),
> - AD4691_CHANNEL(9, 9, 16, 24, 8),
> - AD4691_CHANNEL(10, 10, 16, 24, 8),
> - AD4691_CHANNEL(11, 11, 16, 24, 8),
> - AD4691_CHANNEL(12, 12, 16, 24, 8),
> - AD4691_CHANNEL(13, 13, 16, 24, 8),
> - AD4691_CHANNEL(14, 14, 16, 24, 8),
> - AD4691_CHANNEL(15, 15, 16, 24, 8)
> + AD4691_CHANNEL(0, 0, 16, 32, 8),
> + AD4691_CHANNEL(1, 1, 16, 32, 8),
> + AD4691_CHANNEL(2, 2, 16, 32, 8),
> + AD4691_CHANNEL(3, 3, 16, 32, 8),
> + AD4691_CHANNEL(4, 4, 16, 32, 8),
> + AD4691_CHANNEL(5, 5, 16, 32, 8),
> + AD4691_CHANNEL(6, 6, 16, 32, 8),
> + AD4691_CHANNEL(7, 7, 16, 32, 8),
> + AD4691_CHANNEL(8, 8, 16, 32, 8),
> + AD4691_CHANNEL(9, 9, 16, 32, 8),
> + AD4691_CHANNEL(10, 10, 16, 32, 8),
> + AD4691_CHANNEL(11, 11, 16, 32, 8),
> + AD4691_CHANNEL(12, 12, 16, 32, 8),
> + AD4691_CHANNEL(13, 13, 16, 32, 8),
> + AD4691_CHANNEL(14, 14, 16, 32, 8),
> + AD4691_CHANNEL(15, 15, 16, 32, 8)
> };
>
> static const struct iio_chan_spec ad4693_manual_channels[] = {
> - AD4691_CHANNEL(0, 0, 16, 24, 8),
> - AD4691_CHANNEL(1, 1, 16, 24, 8),
> - AD4691_CHANNEL(2, 2, 16, 24, 8),
> - AD4691_CHANNEL(3, 3, 16, 24, 8),
> - AD4691_CHANNEL(4, 4, 16, 24, 8),
> - AD4691_CHANNEL(5, 5, 16, 24, 8),
> - AD4691_CHANNEL(6, 6, 16, 24, 8),
> - AD4691_CHANNEL(7, 7, 16, 24, 8)
> + AD4691_CHANNEL(0, 0, 16, 32, 8),
> + AD4691_CHANNEL(1, 1, 16, 32, 8),
> + AD4691_CHANNEL(2, 2, 16, 32, 8),
> + AD4691_CHANNEL(3, 3, 16, 32, 8),
> + AD4691_CHANNEL(4, 4, 16, 32, 8),
> + AD4691_CHANNEL(5, 5, 16, 32, 8),
> + AD4691_CHANNEL(6, 6, 16, 32, 8),
> + AD4691_CHANNEL(7, 7, 16, 32, 8)
> };
Hold on, you just introduced them in the first patch. Are those wrong?!
Please, fix this mess and make sure you have no - (minus) lines from
the previous patch(es).
...
> + /*
> + * DMA (thus cache coherency maintenance) may require the
> + * transfer buffers to live in their own cache lines.
> + */
> + unsigned char rx_data[ALIGN(3, sizeof(s64)) + sizeof(s64)] __aligned(IIO_DMA_MINALIGN);
> + unsigned char tx_data[ALIGN(3, sizeof(s64)) + sizeof(s64)];
Don't we have macro for this?
...
> +static int ad4691_transfer(struct ad4691_state *st, int command,
Signed 'command'? Why?
> + unsigned int *val)
...
> +static int ad4691_buffer_postenable(struct iio_dev *indio_dev)
> +{
> + struct ad4691_state *st = iio_priv(indio_dev);
> + struct device *dev = regmap_get_device(st->regmap);
> + struct spi_device *spi = to_spi_device(dev);
> + u16 mask = ~(*indio_dev->active_scan_mask);
> + u32 acc_mask[2] = { mask & 0xFF, mask >> 8 };
Same Q as per previous patch. This seems very wrong.
> + int n_active = hweight_long(*indio_dev->active_scan_mask);
Why signed?
> + unsigned int bit;
> + int ret;
> +
> + ret = ad4691_enter_conversion_mode(st);
> + if (ret)
> + return ret;
> +
> + if (st->adc_mode == AD4691_MANUAL_MODE) {
> + u64 min_period_ns;
> +
> + /* N+1 transfers needed for N channels, with 50% overhead */
> + min_period_ns = div64_u64((u64)(n_active + 1) * AD4691_BITS_PER_XFER *
> + NSEC_PER_SEC * 3,
> + spi->max_speed_hz * 2);
> +
> + if (ktime_to_ns(st->sampling_period) < min_period_ns) {
> + dev_err(dev,
> + "Sampling period %lld ns too short for %d channels. Min: %llu ns\n",
> + ktime_to_ns(st->sampling_period), n_active,
> + min_period_ns);
> + return -EINVAL;
> + }
> +
> + hrtimer_start(&st->sampling_timer, st->sampling_period,
> + HRTIMER_MODE_REL);
> + return 0;
> + }
> +
> + /* CNV_CLOCK_MODE: configure sequencer and start PWM */
> + ret = regmap_write(st->regmap, AD4691_STATE_RESET_REG,
> + AD4691_STATE_RESET_ALL);
> + if (ret)
> + return ret;
> +
> + ret = regmap_bulk_write(st->regmap, AD4691_ACC_MASK1_REG, acc_mask, 2);
sizeof()? ARRAY_SIZE()? See comments per previous patch.
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->regmap, AD4691_STD_SEQ_CONFIG,
> + *indio_dev->active_scan_mask);
> + if (ret)
> + return ret;
> +
> + iio_for_each_active_channel(indio_dev, bit) {
> + ret = regmap_write(st->regmap, AD4691_ACC_COUNT_LIMIT(bit),
> + AD4691_ACC_COUNT_VAL);
> + if (ret)
> + return ret;
> + }
> +
> + return ad4691_sampling_enable(st, true);
> +}
...
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct ad4691_state *st = iio_priv(indio_dev);
> + unsigned int val, i;
> + int ret;
> +
> + guard(mutex)(&st->lock);
> +
> + if (st->adc_mode == AD4691_MANUAL_MODE) {
> + unsigned int prev_val;
> + int prev_chan = -1;
> +
> + /*
> + * MANUAL_MODE with CNV tied to CS: each transfer triggers a
> + * conversion AND returns the previous conversion's result.
> + * First transfer returns garbage, so we do N+1 transfers for
> + * N channels. Collect all results into scan.vals[], then push
> + * the complete scan once.
> + */
> + iio_for_each_active_channel(indio_dev, i) {
> + ret = ad4691_transfer(st, AD4691_ADC_CHAN(i), &val);
> + if (ret)
> + goto done;
> +
> + if (prev_chan >= 0)
> + st->scan.vals[prev_chan] = prev_val;
> + prev_val = val;
> + prev_chan = i;
> + }
> +
> + /* Final NOOP transfer to retrieve last channel's result */
> + ret = ad4691_transfer(st, AD4691_NOOP, &val);
> + if (ret)
> + goto done;
> + st->scan.vals[prev_chan] = val;
How do you guarantee that prev_chan never ever be -1 here?
> + } else {
> + iio_for_each_active_channel(indio_dev, i) {
> + ret = regmap_read(st->regmap, AD4691_AVG_IN(i), &val);
> + if (ret)
> + goto done;
> +
> + st->scan.vals[i] = val;
> + }
> +
> + regmap_write(st->regmap, AD4691_STATE_RESET_REG, AD4691_STATE_RESET_ALL);
> +
> + /* Restart conversions for the next trigger cycle. */
> + ad4691_sampling_enable(st, true);
> + }
> +
> + iio_push_to_buffers_with_ts(indio_dev, &st->scan, sizeof(st->scan),
> + pf->timestamp);
> +
> +done:
> + iio_trigger_notify_done(indio_dev->trig);
> + return IRQ_HANDLED;
> +}
...
> static int ad4691_config(struct ad4691_state *st)
> {
> struct device *dev = regmap_get_device(st->regmap);
> + struct spi_device *spi = to_spi_device(dev);
> enum ad4691_ref_ctrl ref_val;
> unsigned int reg_val;
> int ret;
> st->adc_mode = AD4691_MANUAL_MODE;
> st->sampling_period = ns_to_ktime(DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC,
> AD4691_MANUAL_MODE_STD_FREQ(st->chip->num_channels,
> - to_spi_device(dev)->max_speed_hz)));
> + spi->max_speed_hz)));
> }
Okay, this should be part of the previous patch.
> return regmap_write(st->regmap, AD4691_GPIO_MODE1_REG, AD4691_ADC_BUSY);
> }
...
> +static int ad4691_setup_triggered_buffer(struct iio_dev *indio_dev,
> + struct ad4691_state *st)
> +{
> + struct device *dev = regmap_get_device(st->regmap);
> + struct spi_device *spi = to_spi_device(dev);
> + int irq, ret;
> +
> + st->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
> + indio_dev->name,
> + iio_device_id(indio_dev));
It seems you ignored some of my comments. Please go back and read carefully
what I commented on previous version of the series.
> + if (!st->trig)
> + return -ENOMEM;
> +
> + st->trig->ops = &ad4691_trigger_ops;
> + iio_trigger_set_drvdata(st->trig, st);
> +
> + ret = devm_iio_trigger_register(dev, st->trig);
> + if (ret)
> + return dev_err_probe(dev, ret, "IIO trigger register failed\n");
> +
> + indio_dev->trig = iio_trigger_get(st->trig);
> +
> + if (st->adc_mode == AD4691_MANUAL_MODE) {
> + /*
> + * No DATA_READY signal in MANUAL_MODE; CNV is tied to CS so
> + * conversions start with each SPI transfer. Use an hrtimer to
> + * schedule periodic reads.
> + */
> + hrtimer_setup(&st->sampling_timer, ad4691_sampling_timer_handler,
> + CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> + st->sampling_period = ns_to_ktime(DIV_ROUND_CLOSEST_ULL(
> + NSEC_PER_SEC,
> + AD4691_MANUAL_MODE_STD_FREQ(st->chip->num_channels,
> + spi->max_speed_hz)));
> + } else {
> + /*
> + * DATA_READY asserts at end-of-conversion. The IRQ handler
> + * stops conversions and fires the IIO trigger so the trigger
> + * handler can read and push the sample to the buffer.
> + */
> + irq = fwnode_irq_get(dev_fwnode(dev), 0);
> + if (irq < 0)
> + return dev_err_probe(dev, irq,
> + "failed to get DATA_READY interrupt\n");
> +
> + ret = devm_request_threaded_irq(dev, irq, NULL,
> + &ad4691_irq,
> + IRQF_ONESHOT,
> + indio_dev->name, indio_dev);
> + if (ret)
> + return ret;
> + }
> +
> + return devm_iio_triggered_buffer_setup(dev, indio_dev,
> + &iio_pollfunc_store_time,
> + &ad4691_trigger_handler,
> + &ad4691_buffer_setup_ops);
> +}
--
With Best Regards,
Andy Shevchenko
next prev parent reply other threads:[~2026-03-13 11:13 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-03-13 10:07 [PATCH v3 0/4] iio: adc: ad4691: add driver for AD4691 multichannel SAR ADC family Radu Sabau via B4 Relay
2026-03-13 10:07 ` [PATCH v3 1/4] dt-bindings: iio: adc: add bindings for AD4691 family Radu Sabau via B4 Relay
2026-03-14 9:41 ` Krzysztof Kozlowski
2026-03-16 11:55 ` Sabau, Radu bogdan
2026-03-14 15:29 ` David Lechner
2026-03-14 16:19 ` David Lechner
2026-03-16 12:39 ` Sabau, Radu bogdan
2026-03-16 12:55 ` Sabau, Radu bogdan
2026-03-16 15:14 ` David Lechner
2026-03-16 15:47 ` Sabau, Radu bogdan
2026-03-14 18:18 ` David Lechner
2026-03-13 10:07 ` [PATCH v3 2/4] iio: adc: ad4691: add initial driver " Radu Sabau via B4 Relay
2026-03-13 10:58 ` Andy Shevchenko
2026-03-16 15:29 ` Sabau, Radu bogdan
2026-03-16 15:51 ` David Lechner
2026-03-16 15:57 ` Sabau, Radu bogdan
2026-03-16 16:13 ` Andy Shevchenko
2026-03-14 11:04 ` Nuno Sá
2026-03-16 14:29 ` Sabau, Radu bogdan
2026-03-16 16:00 ` Sabau, Radu bogdan
2026-03-14 16:36 ` David Lechner
2026-03-16 13:01 ` Sabau, Radu bogdan
2026-03-25 15:01 ` kernel test robot
2026-03-13 10:07 ` [PATCH v3 3/4] iio: adc: ad4691: add triggered buffer support Radu Sabau via B4 Relay
2026-03-13 11:13 ` Andy Shevchenko [this message]
2026-03-13 12:09 ` Sabau, Radu bogdan
2026-03-13 14:40 ` Andy Shevchenko
2026-03-14 18:37 ` David Lechner
2026-03-16 13:22 ` Sabau, Radu bogdan
2026-03-16 15:37 ` David Lechner
2026-03-16 15:56 ` Sabau, Radu bogdan
2026-03-16 16:44 ` David Lechner
2026-03-17 9:27 ` Sabau, Radu bogdan
2026-03-13 10:07 ` [PATCH v3 4/4] iio: adc: ad4691: add SPI offload support Radu Sabau via B4 Relay
2026-03-14 19:37 ` David Lechner
2026-03-16 13:31 ` Sabau, Radu bogdan
2026-03-13 11:14 ` [PATCH v3 0/4] iio: adc: ad4691: add driver for AD4691 multichannel SAR ADC family Andy Shevchenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=abPxR0TVa70sMg38@ashevche-desk.local \
--to=andriy.shevchenko@intel.com \
--cc=Michael.Hennerich@analog.com \
--cc=andy@kernel.org \
--cc=brgl@kernel.org \
--cc=broonie@kernel.org \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=dlechner@baylibre.com \
--cc=jic23@kernel.org \
--cc=krzk+dt@kernel.org \
--cc=lars@metafoo.de \
--cc=lgirdwood@gmail.com \
--cc=linusw@kernel.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pwm@vger.kernel.org \
--cc=nuno.sa@analog.com \
--cc=p.zabel@pengutronix.de \
--cc=radu.sabau@analog.com \
--cc=robh@kernel.org \
--cc=ukleinek@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).