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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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:

...

> > > +#include <linux/circ_buf.h>
> > 
> > Why not kfifo?
> 
> I'm sorry but I don't get your comment.
> 
> Do you mean why not use kfifo.h or use kfifo API and change all the code
> using the circ_buf by the kfifo ?

Yes, I mean why use circ_buf API and not kfifo API.

...

> > > +#define NXP_SAR_ADC_IIO_BUFF_SZ		(NXP_SAR_ADC_NR_CHANNELS + (sizeof(u64) / sizeof(u16)))
> > 
> > Hmm... Don't we have some macros so we can avoid this kind of hard coding?
> 
> I don't find such a macro, do you have a pointer ?

If I got the use case correctly, I was thinking of IIO_DECLARE_BUFFER_WITH_TS().

...

> > > +	ndelay(div64_u64(NSEC_PER_SEC, clk_get_rate(info->clk)) * 80U);
> > 
> > Do you need those 'U':s? clk_get_rate() already returns unsigned value of the
> > same or higher rank than int. No?
> 
> May be not needed, but harmless. I can remove them if you want

I think using them in cases that have no side-effects are not needed.

...

> > > +	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.

...

> > > +	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).

> It could be written differently with the DMA_COMPLETE check but the result
> will be the same. I prefer to keep the current implementation which has been
> tested.

-- 
With Best Regards,
Andy Shevchenko