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

Jonathan Cameron
2023-12-17T13:31:32Z

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
>