linux-kernel - Re: [PATCH v15 1/3] dt-bindings: adc: add AD7173

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] [day] [month] [year] [list]

Date: Mon, 26 Feb 2024 14:55:00 +0000
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Jonathan Cameron <jic23@...nel.org>
CC: Dumitru Ceclan <mitrutzceclan@...il.com>, Krzysztof Kozlowski
	<krzysztof.kozlowski+dt@...aro.org>, <linus.walleij@...aro.org>,
	<brgl@...ev.pl>, <andy@...nel.org>, <linux-gpio@...r.kernel.org>, "Lars-Peter
 Clausen" <lars@...afoo.de>, Rob Herring <robh+dt@...nel.org>, Conor Dooley
	<conor+dt@...nel.org>, Michael Walle <michael@...le.cc>, Andy Shevchenko
	<andy.shevchenko@...il.com>, Arnd Bergmann <arnd@...db.de>, ChiaEn Wu
	<chiaen_wu@...htek.com>, Niklas Schnelle <schnelle@...ux.ibm.com>, Leonard
 Göhrs <l.goehrs@...gutronix.de>, Mike Looijmans
	<mike.looijmans@...ic.nl>, Haibo Chen <haibo.chen@....com>, Hugo Villeneuve
	<hvilleneuve@...onoff.com>, David Lechner <dlechner@...libre.com>, "Ceclan
 Dumitru" <dumitru.ceclan@...log.com>, <linux-iio@...r.kernel.org>,
	<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>, Conor Dooley
	<conor.dooley@...rochip.com>
Subject: Re: [PATCH v15 1/3] dt-bindings: adc: add AD7173

On Sat, 24 Feb 2024 17:30:55 +0000
Jonathan Cameron <jic23@...nel.org> wrote:

> On Fri, 23 Feb 2024 15:37:28 +0200
> Dumitru Ceclan <mitrutzceclan@...il.com> wrote:
> 
> > The AD7173 family offer a complete integrated Sigma-Delta ADC solution
> > which can be used in high precision, low noise single channel applications
> > or higher speed multiplexed applications. The Sigma-Delta ADC is intended
> > primarily for measurement of signals close to DC but also delivers
> > outstanding performance with input bandwidths out to ~10kHz.
> > 
> > Reviewed-by: Conor Dooley <conor.dooley@...rochip.com>
> > Signed-off-by: Dumitru Ceclan <mitrutzceclan@...il.com>  
> 
> Ok, in the interests of perfect not being the enemy of good enough.
> I'll leave the supplies for now.  There are lots of existing drivers
> where we don't list them as required (because my understanding of this
> changed in more recent times).
> 
> It's been on my list of jobs for a really boring Friday afternoon
> to bring them all inline with the convention of if it needs power
> on the pin, it's required, so what's one more? :)
> 
> As Nuno pointed out, patch 2 clashed with work already upstream to
> allow firmware to have the final say on interrupt types. I think
> I've resolved that correctly.
> 
> I tidied up the docs ordering issue Andy noted.
> 
> Also, ad_sigma_delta is namespaced. So added
> MODULE_IMPORT_NS(IIO_AD_SIGMA_DELTA).
> 
> Make sure you test your patches with a modular build
> on a more recent tree - that change was early last in 2022!
> 
> A few lines in the driver were too long.
> I don't mind them going over 80 for readability reasons, but
> not over 100.
> 
> Anyhow, with those changes (and please check I didn't mess things up!)
> applied to the togreg branch of iio.git and pushed for now as testing
> for 0-day to get a look in.

Not good news.  There are 2 issues.
>> drivers/iio/adc/ad7173.c:854:3: warning: variable 'chan_arr' is uninitialized when used here [-Wuninitialized]  
     854 |                 chan_arr[chan_index] = ad7173_temp_iio_channel_template;
         |                 ^~~~~~~~
   drivers/iio/adc/ad7173.c:848:32: note: initialize the variable 'chan_arr' to silence this warning
     848 |         struct iio_chan_spec *chan_arr, *chan;
         |                                       ^
         |                                        = NULL
>> drivers/iio/adc/ad7173.c:855:19: warning: variable 'chans_st_arr' is uninitialized when used here [-Wuninitialized]  
     855 |                 chan_st_priv = &chans_st_arr[chan_index];
         |                                 ^~~~~~~~~~~~
   drivers/iio/adc/ad7173.c:845:37: note: initialize the variable 'chans_st_arr' to silence this warning
     845 |         struct ad7173_channel *chans_st_arr, *chan_st_priv;
         |                                            ^
         |                                             = NULL

+ if you build with !CONFIG_GPIOLIB

ad7173_gpio_init() isn't defined.  That needs a stub.

I'll back this driver out for now as fixing the first issue is a little fiddly because
indio_dev->channels is const so the code should allocate and fill the array via a local pointer
before assigning it to indio_dev.

Please send a new version with these resolved + make sure you run some build tests.

Thanks,

Jonathan

> 
> Thanks,
> 
> Jonathan
>