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

[Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>]

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

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