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

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

On Wed, Nov 19, 2025 at 03:44:04PM +0100, Daniel Lezcano wrote:
> On 11/19/25 10:27, Andy Shevchenko wrote:
> > On Tue, Nov 18, 2025 at 10:34 PM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:

[ ... ]

> > > +#define NXP_SAR_ADC_EOC_CH(c)          BIT((c) % 32)
> > 
> > Do you expect "c" to be bigger than 31? In which circumstances?
> 
> No, it should be always lesser than 32. We can drop the modulo.

Yep, please avoid adding a code that is never needed. It complicates reading,
reviewing, and understanding the flow.

[ ... ]

> > > +       /*
> > > +        * Ensure there are at least three cycles between the
> > > +        * configuration of NCMR and the setting of NSTART.
> > > +        */
> > > +       if (enable)
> > > +               ndelay(div64_u64(NSEC_PER_SEC, clk_get_rate(info->clk) * 3));
> > 
> > I'm wondering how low the clock rate can be? With low enough clock
> > rates this becomes a 100% CPU busyloop and in atomic context (is this
> > the case?) without even the possibility to schedule.
> 
> I believe this question was already addressed in v1:
> 
> https://lore.kernel.org/all/a34efc36-0100-4a7f-b131-566413ab88ae@linaro.org/
> 
> right ?

Right, so the bottom line is that we miss the one-two phrases in the comments
above to summarize that.

[ ... ]

> > > +       /* FIELD_GET() can not be used here because EOC_CH is not constant */
> > > +       if (!(NXP_SAR_ADC_EOC_CH(chan) & ceocfr))
> > > +               return -EIO;
> > 
> > [nxp_sar_adc_]field_get() may be defined and used. There is a series
> > pending to bring field_get() to bitfield.h next release.
> 
> TBH I don't have an infinite bandwidth to write temporary helpers. So if it
> is ok, I would prefer to keep it as is

Sure, perhaps just add a TODO line instead of the comment above:

	/* TODO: Switch to field_get() when it will be available */

[ ... ]

> > > +       nxp_sar_adc_channels_disable(info, *indio_dev->active_scan_mask);
> > 
> > Wondering why this can't take a pointer to a mask.
> nxp_sar_adc_channels_disable() is also called with BIT(x) parameter in other
> places. So in the function is much easier to do val |= mask;

OK!

> > > +       ret = devm_request_irq(dev, irq, nxp_sar_adc_isr, 0,
> > > +                              dev_name(dev), indio_dev);
> > > +       if (ret < 0)
> > > +               return dev_err_probe(dev, ret, "failed requesting irq, irq = %d\n", irq);
> > 
> > No error code duplication in the message, please.
> 
> Given devm_request will print the "request_irq(%u) %ps %ps %s\n" error
> message. Would you suggest to just return ret here ?

Yes!

-- 
With Best Regards,
Andy Shevchenko