From: David Lechner <dlechner@baylibre.com>
To: "Sabau, Radu bogdan" <Radu.Sabau@analog.com>,
"Lars-Peter Clausen" <lars@metafoo.de>,
"Hennerich, Michael" <Michael.Hennerich@analog.com>,
"Jonathan Cameron" <jic23@kernel.org>,
"Sa, Nuno" <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>,
"Jonathan Corbet" <corbet@lwn.net>,
"Shuah Khan" <skhan@linuxfoundation.org>
Cc: "linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-pwm@vger.kernel.org" <linux-pwm@vger.kernel.org>,
"linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>
Subject: Re: [PATCH v6 4/4] iio: adc: ad4691: add SPI offload support
Date: Mon, 6 Apr 2026 08:53:42 -0500 [thread overview]
Message-ID: <0192c4d2-5adf-434e-9b58-4843f5ffa68a@baylibre.com> (raw)
In-Reply-To: <LV9PR03MB8414CB6B07EA81FB5A42436AF75DA@LV9PR03MB8414.namprd03.prod.outlook.com>
On 4/6/26 5:39 AM, Sabau, Radu bogdan wrote:
>> -----Original Message-----
>> From: David Lechner <dlechner@baylibre.com>
>> Sent: Saturday, April 4, 2026 6:57 PM
>
> ...
>
>>> +
>>> #define AD4691_CHANNEL(ch)
>> \
>>> { \
>>> .type = IIO_VOLTAGE, \
>>> @@ -122,11 +155,9 @@ struct ad4691_chip_info {
>>> .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE),
>> \
>>> .channel = ch, \
>>> .scan_index = ch, \
>>> - .scan_type = { \
>>> - .sign = 'u', \
>>> - .realbits = 16, \
>>> - .storagebits = 16, \
>>> - }, \
>>> + .has_ext_scan_type = 1,
>> \
>>> + .ext_scan_type = ad4691_scan_types, \
>>> + .num_ext_scan_type = ARRAY_SIZE(ad4691_scan_types),
>> \
>>
>> Usually, we just make two separte ad4691_chip_info structs for offload
>> vs. not offload.
>>
>> ext_scan_type is generally only used when the scan type can change
>> dynamically after probe.
>>
>
> So, just to be clear, you are saying I should have different chip_info structs
> and change the triggered-buffer for offload ones if offload is present?
> I am asking since offload has different scan types as well, and this would
> mean 3 different chip_info structs for each chip -> total of 12 chip_info structs,
> each with a different channel array, or perhaps there is a more compact way
> to have this implemented.
> I could make the channel arrays use the same macro and have the scan_type
> reversed to storage and shift done as parameters.
>
> Please let me know your thoughts on this.
If it gets too complex, we can dynamically create the chip info
struct during probe. But in general we prefer to statically define
them even if it gets a little verbose. Macros usually help here.
>>> }
>>>
>>> @@ -883,6 +1184,20 @@ static ssize_t sampling_frequency_store(struct
>> device *dev,
>>> if (iio_buffer_enabled(indio_dev))
>>> return -EBUSY;
>>>
>>> + if (st->manual_mode && st->offload) {
>>> + struct spi_offload_trigger_config config = {
>>> + .type = SPI_OFFLOAD_TRIGGER_PERIODIC,
>>> + .periodic = { .frequency_hz = freq },
>>> + };
>>
>> Same comment as other patches. This needs to account for oversampling
>> ratio.
>>
>
> I am thinking that since we would have different chip_info structs, manual
> mode channels could omit the oversampling attribute, since it is not supported
> by the chip on this mode.
Yes, this would be ideal.
>> SPI_OFFLOAD_TRIGGER_PERIODIC);
>>> + if (IS_ERR(offload->trigger))
>>> + return dev_err_probe(dev, PTR_ERR(offload->trigger),
>>> + "Failed to get periodic offload
>> trigger\n");
>>> +
>>> + offload->trigger_hz = st->info->max_rate;
>>
>> I think I mentioned this elsewhere, but can we really get max_rate in manual
>> mode
>> due to the extra SPI overhead? Probably safer to start with a lower rate.
>
> You are right a slower rate would be nicer, from my tests 311kHz worked perfect
> with a 10MHz SPI frequency, but perhaps these numbers are a bit "odd".
>
> How do you feel about 100kHz for a starting sample rate?
Sounds reasonable.
>> IIO_BUFFER_DIRECTION_IN);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + indio_dev->buffer->attrs = ad4691_buffer_attrs;
>>
>> Should including ad4691_buffer_attrs depend on st->manual_mode?
>>
>> I thought it was only used when PWM is connected to CNV.
>>
>
> For offload manual mode, I thought buffer sampling frequency should also be available,
> since the offload trigger's frequency is accessible.
Ah right. Not sure what I was thinking when I wrote that.
>
>>> +
>>> + return 0;
>>> +}
>>> +
>>> static int ad4691_probe(struct spi_device *spi)
>>> {
>>> struct device *dev = &spi->dev;
>>> + struct spi_offload *spi_offload;
>>> struct iio_dev *indio_dev;
>>> struct ad4691_state *st;
>>> int ret;
>>> @@ -1232,6 +1626,13 @@ static int ad4691_probe(struct spi_device *spi)
>>> if (ret)
>>> return ret;
>>>
>>> + spi_offload = devm_spi_offload_get(dev, spi,
>> &ad4691_offload_config);
>>> + ret = PTR_ERR_OR_ZERO(spi_offload);
>>> + if (ret == -ENODEV)
>>> + spi_offload = NULL;
>>> + else if (ret)
>>> + return dev_err_probe(dev, ret, "Failed to get SPI offload\n");
>>> +
>>> indio_dev->name = st->info->name;
>>> indio_dev->info = &ad4691_info;
>>> indio_dev->modes = INDIO_DIRECT_MODE;
>>> @@ -1239,7 +1640,10 @@ static int ad4691_probe(struct spi_device *spi)
>>> indio_dev->channels = st->info->channels;
>>> indio_dev->num_channels = st->info->num_channels;
>>
>> As mentioned earlier, we generally want separate channel structs
>> for SPI offload. These will also have different num_channels because
>> there is no timestamp channel in SPI offload.
>
> If different chip_info structs will be used, wouldn't they already have specific
> channels attached to them?
>
Yes.
prev parent reply other threads:[~2026-04-06 13:53 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-04-03 11:03 [PATCH v6 0/4] iio: adc: ad4691: add driver for AD4691 multichannel SAR ADC family Radu Sabau via B4 Relay
2026-04-03 11:03 ` [PATCH v6 1/4] dt-bindings: iio: adc: add AD4691 family Radu Sabau via B4 Relay
2026-04-03 11:03 ` [PATCH v6 2/4] iio: adc: ad4691: add initial driver for " Radu Sabau via B4 Relay
2026-04-04 14:25 ` David Lechner
2026-04-03 11:03 ` [PATCH v6 3/4] iio: adc: ad4691: add triggered buffer support Radu Sabau via B4 Relay
2026-04-04 15:12 ` David Lechner
2026-04-05 8:57 ` Andy Shevchenko
2026-04-06 9:22 ` Sabau, Radu bogdan
2026-04-06 13:39 ` David Lechner
2026-04-03 11:03 ` [PATCH v6 4/4] iio: adc: ad4691: add SPI offload support Radu Sabau via B4 Relay
2026-04-04 15:34 ` David Lechner
2026-04-06 9:34 ` Sabau, Radu bogdan
2026-04-06 13:44 ` David Lechner
2026-04-06 14:16 ` Sabau, Radu bogdan
2026-04-06 15:02 ` David Lechner
2026-04-07 12:38 ` Sabau, Radu bogdan
2026-04-04 15:57 ` David Lechner
2026-04-06 10:39 ` Sabau, Radu bogdan
2026-04-06 11:08 ` Sabau, Radu bogdan
2026-04-06 13:30 ` Sabau, Radu bogdan
2026-04-06 13:56 ` David Lechner
2026-04-06 14:20 ` Sabau, Radu bogdan
2026-04-06 13:53 ` David Lechner [this message]
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=0192c4d2-5adf-434e-9b58-4843f5ffa68a@baylibre.com \
--to=dlechner@baylibre.com \
--cc=Michael.Hennerich@analog.com \
--cc=Nuno.Sa@analog.com \
--cc=Radu.Sabau@analog.com \
--cc=andy@kernel.org \
--cc=brgl@kernel.org \
--cc=broonie@kernel.org \
--cc=conor+dt@kernel.org \
--cc=corbet@lwn.net \
--cc=devicetree@vger.kernel.org \
--cc=jic23@kernel.org \
--cc=krzk+dt@kernel.org \
--cc=lars@metafoo.de \
--cc=lgirdwood@gmail.com \
--cc=linusw@kernel.org \
--cc=linux-doc@vger.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=p.zabel@pengutronix.de \
--cc=robh@kernel.org \
--cc=skhan@linuxfoundation.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).