linux-kernel - Re: [PATCH v2 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20250912151848.0000470e@huawei.com>
Date: Fri, 12 Sep 2025 15:18:48 +0100
From: Jonathan Cameron <jonathan.cameron@...wei.com>
To: Daniel Lezcano <daniel.lezcano@...aro.org>
CC: Jonathan Cameron <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>
Subject: Re: [PATCH v2 2/2] iio: adc: Add the NXP SAR ADC support for the
 s32g2/3 platforms

On Thu, 11 Sep 2025 14:55:00 +0200
Daniel Lezcano <daniel.lezcano@...aro.org> wrote:

> Hi Jonathan,
> 
> thanks for the review
> 
> On 10/09/2025 19:32, Jonathan Cameron wrote:
> > On Wed, 10 Sep 2025 17:57:56 +0200
> > Daniel Lezcano <daniel.lezcano@...aro.org> wrote:  
> 
> [ ... ]
> 
> >> +/* Main Configuration Register */
> >> +#define REG_ADC_MCR(__base)		((__base) + 0x00)  
> > 
> > I'm not really convinced these macros help over just having
> > readl(info->regs + NXP_SADC_MCR_REG);  
> 
> That is really a matter of taste :)
> 
> I used to create this format in order to stick the macros with the 
> debugfs register code which is not part of these changes. There is a 
> similar format in drivers/clocksource/timer-nxp-stm.c or 
> driver/thermal/mediatek/lvts.c IMHO is less prone to error than base + 
> REG all around the code.
> 
> Do you want me to convert all the macros to info->__base + MACRO ?

I'm not that fussed if there is other code for related devices using this
style.  To me it adds little benefit but it doesn't hurt that much either!

> 
> [ ... ]
> 
> >> +static const struct iio_chan_spec nxp_sar_adc_iio_channels[] = {
> >> +	ADC_CHAN(0, IIO_VOLTAGE),
> >> +	ADC_CHAN(1, IIO_VOLTAGE),
> >> +	ADC_CHAN(2, IIO_VOLTAGE),
> >> +	ADC_CHAN(3, IIO_VOLTAGE),
> >> +	ADC_CHAN(4, IIO_VOLTAGE),
> >> +	ADC_CHAN(5, IIO_VOLTAGE),
> >> +	ADC_CHAN(6, IIO_VOLTAGE),
> >> +	ADC_CHAN(7, IIO_VOLTAGE),
> >> +	IIO_CHAN_SOFT_TIMESTAMP(32),  
> > 
> > Whilst we only insist on monotonic numbering, putting it all the way down
> > at 32 seems excessive. Why not 8?  Perhaps a comment if this is to avoid
> > moving it for some future feature.  
> 
> The ADC has 8 channels for external acquisition however others channels 
> 8->31 are described as reserved. They may evolve in the future to more 
> channels. That is probably the reason why 32 is used here.

Add a comment on that so we don't forget the reasoning.

Thanks,

Jonathan