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: <b4d2ad54-54d5-4c26-be49-b6ac671683d2@linaro.org>
Date: Thu, 11 Sep 2025 14:55:00 +0200
From: Daniel Lezcano <daniel.lezcano@...aro.org>
To: Jonathan Cameron <jic23@...nel.org>
Cc: 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


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 ?

[ ... ]

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

[ ... ]

>> +	indio_dev->name = dev_name(dev);
> 
> This should be the 'part number'.  That is a little ill defined
> for a SoC integrated ADC, but generally not what we get from dev_name()
> on the platform_device.

Sorry, I don't get the comment. If I refer to the different drivers 
there is not consistency with the iio_dev->name.

rtq6056.c:      indio_dev->name = "rtq6056";
rzg2l_adc.c:    indio_dev->name = DRIVER_NAME;
sc27xx_adc.c:   indio_dev->name = dev_name(dev);
mt6359-auxadc.c:  indio_dev->name = adc_dev->chip_info->model_name;
mcp3911.c:      indio_dev->name = spi_get_device_id(spi)->name;

Are you suggesting to use the compatible part number ?

	indio->name = "s32g2-sar-adc";




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