Re: [PATCH v4 09/15] iio: adc: stm32: Use device_for_each_child_node_scoped()

From: Jonathan Cameron <jic23@kernel.org>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: linux-iio@vger.kernel.org,
	"Rafael J . Wysocki" <rafael@kernel.org>,
	"Len Brown" <lenb@kernel.org>,
	linux-acpi@vger.kernel.org,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Daniel Scally" <djrscally@gmail.com>,
	"Heikki Krogerus" <heikki.krogerus@linux.intel.com>,
	"Sakari Ailus" <sakari.ailus@linux.intel.com>,
	"Julia Lawall" <Julia.Lawall@inria.fr>,
	"Nuno Sá" <nuno.sa@analog.com>,
	"Cosmin Tanislav" <cosmin.tanislav@analog.com>,
	"Mihail Chindris" <mihail.chindris@analog.com>,
	"Rasmus Villemoes" <linux@rasmusvillemoes.dk>,
	"Tomislav Denis" <tomislav.denis@avl.com>,
	"Marek Vasut" <marex@denx.de>,
	"Olivier Moysan" <olivier.moysan@foss.st.com>,
	"Fabrice Gasnier" <fabrice.gasnier@foss.st.com>,
	"Lad Prabhakar" <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	"Dmitry Baryshkov" <dmitry.baryshkov@linaro.org>,
	"Marijn Suijten" <marijn.suijten@somainline.org>,
	"Marius Cristea" <marius.cristea@microchip.com>,
	"Ibrahim Tilki" <Ibrahim.Tilki@analog.com>,
	"Peter Zijlstra" <peterz@infradead.org>,
	"Jonathan Cameron" <Jonathan.Cameron@huawei.com>
Subject: Re: [PATCH v4 09/15] iio: adc: stm32: Use device_for_each_child_node_scoped()
Date: Sat, 24 Feb 2024 12:16:51 +0000	[thread overview]
Message-ID: <20240224121651.260404a5@jic23-huawei> (raw)
In-Reply-To: <ZdM_P93vVIrBHkmg@smile.fi.intel.com>

On Mon, 19 Feb 2024 13:45:03 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Sat, Feb 17, 2024 at 04:42:43PM +0000, Jonathan Cameron wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > Switching to the _scoped() version removes the need for manual
> > calling of fwnode_handle_put() in the paths where the code
> > exits the loop early. In this case that's all in error paths.
> > 
> > Note this includes a probable fix as in one path an error message was
> > printed with ret == 0.
> > 
> > Took advantage of dev_err_probe() to futher simplify things given no
> > longer a need for the goto err.  
> 
> ...
> 
> >  	const struct stm32_adc_info *adc_info = adc->cfg->adc_info;  
> 
> I believe with
> 
> 	struct device *dev = &indio_dev->dev;
> 
> you can make the below neater.

Agreed.  Given the users of indio_dev are all the error messages I'm touching
anyway, this is in scope for this patch I think.

> Also see some side notes.
> 
> > -	struct fwnode_handle *child;
> >  	const char *name;
> >  	int val, scan_index = 0, ret;
> >  	bool differential;
> >  	u32 vin[2];  
> 
> ...
> 
> >  		if (!ret) {  
> 
> Not a fan of this pattern, below we have two different patterns for the cases
> like this :-(

Likewise not a fan.

Tidying this up is unrelated enough though that I won't do it in this
series.

> 
> > +			if (strlen(name) >= STM32_ADC_CH_SZ)
> > +				return dev_err_probe(&indio_dev->dev, -EINVAL,
> > +						     "Label %s exceeds %d characters\n",
> > +						     name, STM32_ADC_CH_SZ);
> > +
> >  			strscpy(adc->chan_name[val], name, STM32_ADC_CH_SZ);
> >  			ret = stm32_adc_populate_int_ch(indio_dev, name, val);
> >  			if (ret == -ENOENT)
> >  				continue;
> >  			else if (ret)  
> 
> 
> This 'else' is redundant.
> 
> > +				return ret;
> > +		} else if (ret != -EINVAL)  
>
I shouldn't have dropped the bracket here, so I'll put that back for now.

> This also...
> 
> > +			return dev_err_probe(&indio_dev->dev, ret, "Invalid label\n");  
> 
> ...if you first do like
> 
> 		if (ret && ret != -EINVAL)
> 			return dev_err_probe(...);
> 		if (!ret) {
> 
> Another option
> 
> 		if (ret) {
> 			if (ret != -EINVAL)
> 				return dev_err_probe(...);
> 		} else {
> 
> ...
> 
> >  		differential = false;
> >  		ret = fwnode_property_read_u32_array(child, "diff-channels", vin, 2);  
> 
> ARRAY_SIZE()?
> 
> >  		/* diff-channels is optional */  
> 
> ...
> 
> >  		if (!ret) {
> > +			if (vin[0] != val || vin[1] >= adc_info->max_channels)
> > +				return dev_err_probe(&indio_dev->dev, -EINVAL,
> > +						     "Invalid channel in%d-in%d\n",
> > +						     vin[0], vin[1]);
> >  		} else if (ret != -EINVAL) {
> > -			dev_err(&indio_dev->dev, "Invalid diff-channels property %d\n", ret);
> > -			goto err;
> > +			return dev_err_probe(&indio_dev->dev, ret,
> > +					     "Invalid diff-channels property\n");
> >  		} 
> 
> As per above?
> 
Agree with all comments, but as you said side notes. Stuff to cleanup in a series
doing a wider driver clean up.

Thanks,

Jonathan