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

[Daniel Lezcano <daniel.lezcano@linaro.org>]

Hi Andy,

On 10/31/25 13:45, Andy Shevchenko wrote:
> On Fri, Oct 31, 2025 at 12:32:03PM +0100, Daniel Lezcano wrote:
>> On 10/30/25 10:28, Andy Shevchenko wrote:
>>> On Thu, Oct 30, 2025 at 09:27:21AM +0100, Daniel Lezcano wrote:
>>>> On 10/18/25 22:12, Andy Shevchenko wrote:
>>>>> On Fri, Oct 17, 2025 at 06:42:38PM +0200, Daniel Lezcano wrote:
> 
> [ ... ]
> 
>>>>>> +	dma_samples = (u32 *)dma_buf->buf;
>>>>>
>>>>> Is it aligned properly for this type of casting?
>>>>
>>>> TBH, I don't know the answer :/
>>>>
>>>> How can I check that ?
>>>
>>> Is buf defined as a pointer to u32 / int or bigger? or is it just byte buffer?
>>> If the latter, how does the address of it being formed? Does it come from a heap
>>> (memory allocator)? If yes, we are fine, as this is usually the case for all
>>> (k)malloc'ed memory.
>>
>> buf is a byte buffer allocated with dmam_alloc_coherent(..., GFP_KERNEL)
> 
> We are fine :-)
> 
> ...
> 
>>>>>> +	dmaengine_tx_status(info->dma_chan, info->cookie, &state);
>>>>>
>>>>> No return value check?
>>>>
>>>> The return value is not necessary here because the caller of the callback
>>>> will check with dma_submit_error() in case of error which covers the
>>>> DMA_ERROR case and the other cases are not useful because the residue is
>>>> taken into account right after.
>>>
>>> In some cases it might return DMA_PAUSE (and actually this is the correct way
>>> to get residue, one needs to pause the channel to read it, otherwise it will
>>> give outdated / incorrect information).
>>
>> But if the residue is checked in the callback routine without checking
>> DMA_PAUSED, the result is the same no ?
> 
> DMA in some corner cases might have already be charged for the next transfer.
> Do you have a synchronisation between DMA start and residue check?
> 
> I.o.w. this may work for your case, but in general it's not guaranteed. The proper
> read of residue is to: pause DMA --> read residue --> resume DMA.

I discussed with Vinod about this change and he suggested to use the 
callback_result() to get the residue as a parameter so the 
dmaengine_txstatus() call won't be needed anymore.

Unfortunately, it does not work. I had a look in the DMA driver and the 
internals but my knowledge is limited in this area so I was unable to 
find out what is going on. Moreover there are no so many driver using 
this API I can use as an example. The best I was able to do was 
propagating the residue to the result in the vchan_complete() but it 
does not work.

Then I stepped back by not using the callback_result() and used 
dmaengine_pause(), read the residue, dmaengine_resume() but there are no 
result after these calls. I don't know why.

The issue you are mentioning above should be handled in other drivers 
doing the same kind of acquisition but the routine is similar to the one 
proposed here (eg. stm32).

The NXP SAR acquisition routine is running since several years in 
production AFAICT.

I investigated the different solutions without success, while I can run 
the acquisition routine without problem here with my hardware. A signal 
generator captured by the ADC, plotted and compared with the 
oscilloscope display.

The circ buffer is working well here and no bug was spotted with the 
current routine. I think I did my best to make the driver better from 
its initial submission. The best is the enemy of the good, and I would 
like to make some progress here in the driver acceptance. Changing the 
entire driver for the sake of replacing the circ_buffer by the kfifo and 
change the code for a scenario which is not happening is not really 
worth. Especially that the DMA engine is being modified to take into the 
cyclic DMA in its API, thus the circ_buffer and the routine will go away 
once the driver is changed to take into account this new API.

IOW, can we keep this routine as it is for now as it works fine and go 
forward for a v6 ?





-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog