On Thu, 14 Dec 2023 16:47:29 +0200 Andy Shevchenko <andy@xxxxxxxxxx> 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@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; > 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 >
