Re: [PATCH v1 2/2] iio: adc: Add the NXP SAR ADC support for the s32g2/3 platforms

[David Lechner <dlechner@baylibre.com>]

On 9/5/25 4:44 AM, Daniel Lezcano wrote:
> On 04/09/2025 19:49, David Lechner wrote:
>> On 9/4/25 12:40 PM, Daniel Lezcano wrote:
>>>
>>> Hi Nuno,
>>>
>>> On 03/09/2025 13:20, Nuno Sá wrote:
>>>> On Wed, 2025-09-03 at 12:27 +0200, Daniel Lezcano wrote:
>>>>> From: Stefan-Gabriel Mirea <stefan-gabriel.mirea@nxp.com>
>>>>>
>>>>> The NXP S32G2 and S32G3 platforms integrate a successive approximation
>>>>> register (SAR) ADC. Two instances are available, each providing 8
>>>>> multiplexed input channels with 12-bit resolution. The conversion rate
>>>>> is up to 1 Msps depending on the configuration and sampling window.
>>>>>
>>>>> The SAR ADC supports raw, buffer, and trigger modes. It can operate
>>>>> in both single-shot and continuous conversion modes, with optional
>>>>> hardware triggering through the cross-trigger unit (CTU) or external
>>>>> events. An internal prescaler allows adjusting the sampling clock,
>>>>> while per-channel programmable sampling times provide fine-grained
>>>>> trade-offs between accuracy and latency. Automatic calibration is
>>>>> performed at probe time to minimize offset and gain errors.
>>>>>
>>>>> The driver is derived from the BSP implementation and has been partly
>>>>> rewritten to comply with upstream requirements. For this reason, all
>>>>> contributors are listed as co-developers, while the author refers to
>>>>> the initial BSP driver file creator.
>>>>>
>>>>> All modes have been validated on the S32G274-RDB2 platform using an
>>>>> externally generated square wave captured by the ADC. Tests covered
>>>>> buffered streaming via IIO, trigger synchronization, and accuracy
>>>>> verification against a precision laboratory signal source.
>>>>>
>>>>> Co-developed-by: Alexandru-Catalin Ionita <alexandru-catalin.ionita@nxp.com>
>>>>> Signed-off-by: Alexandru-Catalin Ionita <alexandru-catalin.ionita@nxp.com>
>>>>> Co-developed-by: Ciprian Costea <ciprianmarian.costea@nxp.com>
>>>>> Signed-off-by: Ciprian Costea <ciprianmarian.costea@nxp.com>
>>>>> Co-developed-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@oss.nxp.com>
>>>>> Signed-off-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@oss.nxp.com>
>>>>> Signed-off-by: Stefan-Gabriel Mirea <stefan-gabriel.mirea@nxp.com>
>>>>> Co-developed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>>> ---
>>>
>>> [ ... ]
>>>
>>>> The above needs some discussion at the very least. Have you considered the IIO
>>>> DMA buffer interface? It should be extendable to accommodate any particularity
>>>> of your usecase (or we should at least discuss it).
>>>>
>>>> With it, you also gain a userspace interface where you can actually share DMA
>>>> buffers in a zero copy fashion. You can also share these buffers with USB
>>>> gadgets. For instance, with libiio, you would be able to fetch samples from your
>>>> host machine (through USB) in a very fast way (zero copy between IIO and USB).
>>>>
>>>> Setting up DMA to then "having" to push it to a SW buffer and needing a syscall
>>>> to retrieve the data seems counter-productive.
>>>
>>> I've read a bit about the DMA engine. It is unclear how to use it and there are very few examples in the different drivers to refer to.
>>>
>>> This proposed driver supports the RAW, BUFFER and TRIGGERED.
>>>
>>> Shall I create an IIO device with the modes:
>>>
>>> indio_dev->modes =
>>>      INDIO_DIRECT_MODE |
>>
>> Only INDIO_DIRECT_MODE needs to be set here.
>>
>>>      INDIO_BUFFER_HARDWARE |
>>>      INDIO_BUFFER_TRIGGERED
>>>
>>> And then use:
>>>
>>> devm_iio_triggered_buffer_setup()
>>
>> Yes, use this and it will add INDIO_BUFFER_TRIGGERED to the flags.
>>
>>>
>>> and
>>>
>>> devm_iio_dmaengine_buffer_setup_with_handle
>>
>> Likewise, this will add INDIO_BUFFER_HARDWARE.
>>
>> And you likely only need to call devm_iio_dmaengine_buffer_setup() which will
>> save some boilerplate code.
> 
> What is still unclear for me is the trigger and the dma modes.
> 
> If the dma engine is supported, I should use devm_iio_dmaengine_buffer_setup_with_handle(), but will the trigger mode be supported also automatically (I don't see a clear answer in the documentation neither in the drivers) ?
> 
> If not, shall I call devm_iio_triggered_buffer_setup() and devm_iio_dmaengine_buffer_setup_with_handle() ?
> 

Taking a step back, what sort of real-world uses cases do you need to support?
Or are you just trying to implement everything that the ADC can do? The latter
can be a bit risky because you might end making something where you can't do
a buffered read and a single channel read at the same time, but later find out
you have a real-world application that needs to do this.

It looks like it would be possible to implement buffered reads in lots of ways.
IIO devices can have more than one buffer per device so we can add more in the
future if we need to. So I would just drop the DMA part of the implementation
for now and implement the basic triggered buffer using MCR[NSTART] and ECH
(End of Chain) interrupt request and just reading data from the ICDR registers.

I would wait to have a real-world application that requires DMA to decide the
best way to implement that. There are lots of possibilities, like does it need
an external trigger or is continuous mode good enough? Does it need to be cyclic
(something the IIO subsystem doesn't really support yet) or not. Is exact sample
timing important or do we just need a big buffer? These questions we can't
really answer without a specific application to use it.