Re: [PATCH v8 2/2] iio: adc: ad7173: add AD7173 driver

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 14 Dec 2023 16:47:29 +0200
Andy Shevchenko <andy@kernel.org> wrote:

> On Thu, Dec 14, 2023 at 02:57:35PM +0200, Ceclan Dumitru wrote:
> > On 12/14/23 14:30, Jonathan Cameron wrote:  
> > > On Tue, 12 Dec 2023 12:44:36 +0200
> > > Dumitru Ceclan <mitrutzceclan@gmail.com> wrote:  
> 
> ...
> 
> > >> +		ret = fwnode_property_match_property_string(child,
> > >> +							    "adi,reference-select",
> > >> +							    ad7173_ref_sel_str,
> > >> +							    ARRAY_SIZE(ad7173_ref_sel_str));  
> 
> > >> +  
> 
> Redundant blank line.
> 
> > >> +		if (ret < 0)
> > >> +			ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
> > >> +		else
> > >> +			ref_sel = ret;  
> > > Simpler pattern for properties with a default is not to check the error code.
> > > 
> > > 		ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
> > > 
> > > 		fwnode_property_match_property_String(child, ...
> > > 
> > > so only if it succeeds is the value overridden.  
> > 
> > Where exactly would the value be overridden, the function does not have an
> > argument passed for the found index. The function is written to return either
> > the found index or a negative error.
> > 
> > The proposed pattern would just ignore the returned index and would always
> > leave ref_sel to default. Am I missing something?
> > 
> > I can see in the thread where it was introduced that you proposed:
> > "Looking at the usecases I wonder if it would be better to pass in
> > an unsigned int *ret which is only updated on a match?"
> > 
> > But on the iio togreg branch that was suggested I could the function on, it
> > does not have that parameter.  
> 
> Yeah, with the current API we can have one check (no 'else' branch):
> 
> 		ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
> 		ret = ...
> 		if (ret >= 0)
> 			ref_sel = ret;
> 
Yeah. I was clearly lacking in coffee or just being an idiot that day!

> But your approach is good to me.
> 
> ...
> 
> It's always possible to change prototype, and now of course is the best time
> as all the users are provided in the single tree. That said, patches are
> welcome if this is what we want. (My proposal was to return index in case of
> no error, but at the same time leave it in the returned code, so it will be
> aligned with other match functions of fwnode.
> 
> But this in either way will complicate the implementation. And I don't find
> critical to have if-else in each caller as some of them may do something
> different on the error case, when option is mandatory. In such cases we
> usually don't provide output if we know that an error condition occurs.

I'm fine with it being as it is.  Was just having a slow brain day.

J
>