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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aR3s-gFf76mXN_uZ@smile.fi.intel.com>
Date: Wed, 19 Nov 2025 18:14:50 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Daniel Lezcano <daniel.lezcano@...aro.org>
Cc: Andy Shevchenko <andy.shevchenko@...il.com>, jic23@...nel.org,
	dlechner@...libre.com, nuno.sa@...log.com, andy@...nel.org,
	robh@...nel.org, conor+dt@...nel.org, krzk+dt@...nel.org,
	linux-iio@...r.kernel.org, s32@....com,
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
	chester62515@...il.com, mbrugger@...e.com,
	ghennadi.procopciuc@....nxp.com, vkoul@...nel.org
Subject: Re: [PATCH v6 2/2] iio: adc: Add the NXP SAR ADC support for the
 s32g2/3 platforms

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@...aro.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