Received-SPF: pass (google.com: domain of linux-kernel+bounces-81744-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Mon, 26 Feb 2024 14:55:00 +0000
From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Jonathan Cameron <jic23@kernel.org>
CC: Dumitru Ceclan <mitrutzceclan@gmail.com>, Krzysztof Kozlowski
	<krzysztof.kozlowski+dt@linaro.org>, <linus.walleij@linaro.org>,
	<brgl@bgdev.pl>, <andy@kernel.org>, <linux-gpio@vger.kernel.org>, "Lars-Peter
 Clausen" <lars@metafoo.de>, Rob Herring <robh+dt@kernel.org>, Conor Dooley
	<conor+dt@kernel.org>, Michael Walle <michael@walle.cc>, Andy Shevchenko
	<andy.shevchenko@gmail.com>, Arnd Bergmann <arnd@arndb.de>, ChiaEn Wu
	<chiaen_wu@richtek.com>, Niklas Schnelle <schnelle@linux.ibm.com>, Leonard
 =?ISO-8859-1?Q?G=F6hrs?= <l.goehrs@pengutronix.de>, Mike Looijmans
	<mike.looijmans@topic.nl>, Haibo Chen <haibo.chen@nxp.com>, Hugo Villeneuve
	<hvilleneuve@dimonoff.com>, David Lechner <dlechner@baylibre.com>, "Ceclan
 Dumitru" <dumitru.ceclan@analog.com>, <linux-iio@vger.kernel.org>,
	<devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>, Conor Dooley
	<conor.dooley@microchip.com>
Subject: Re: [PATCH v15 1/3] dt-bindings: adc: add AD7173
Message-ID: <20240226145500.00007783@Huawei.com>
In-Reply-To: <20240224173055.2b2e067c@jic23-huawei>
References: <20240223133758.9787-1-mitrutzceclan@gmail.com>
	<20240224173055.2b2e067c@jic23-huawei>
Organization: Huawei Technologies Research and Development (UK) Ltd.
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="US-ASCII"
Content-Transfer-Encoding: 7bit

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

> On Fri, 23 Feb 2024 15:37:28 +0200
> Dumitru Ceclan <mitrutzceclan@gmail.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@microchip.com>
> > Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.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
>