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

[Andy Shevchenko <andy@xxxxxxxxxx>]

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@xxxxxxxxx> 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;

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.

-- 
With Best Regards,
Andy Shevchenko