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

[Jonathan Cameron <jic23@kernel.org>]

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