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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Sun, 17 Dec 2023 13:31:12 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Andy Shevchenko <andy@...nel.org>
Cc: Ceclan Dumitru <mitrutzceclan@...il.com>, Jonathan Cameron
 <Jonathan.Cameron@...wei.com>, linus.walleij@...aro.org, brgl@...ev.pl,
 linux-gpio@...r.kernel.org, Lars-Peter Clausen <lars@...afoo.de>, Rob
 Herring <robh+dt@...nel.org>, Krzysztof Kozlowski
 <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>,
 Michael Walle <michael@...le.cc>, Arnd Bergmann <arnd@...db.de>, ChiaEn Wu
 <chiaen_wu@...htek.com>, Niklas Schnelle <schnelle@...ux.ibm.com>, Leonard
 Göhrs <l.goehrs@...gutronix.de>, Mike Looijmans
 <mike.looijmans@...ic.nl>, Haibo Chen <haibo.chen@....com>, Hugo Villeneuve
 <hvilleneuve@...onoff.com>, Ceclan Dumitru <dumitru.ceclan@...log.com>,
 linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8 2/2] iio: adc: ad7173: add AD7173 driver

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