Re: [PATCH v6 3/5] iio: adc: ad7949: add vref selection support

On Sun, Aug 29, 2021 at 03:35:39PM +0100, Jonathan Cameron wrote:

On Mon, 16 Aug 2021 16:12:58 +0300
Andy Shevchenko [off-list ref] wrote:

quoted

On Mon, Aug 16, 2021 at 4:07 PM Liam Beguin [off-list ref] wrote:

quoted

On Mon Aug 16, 2021 at 8:48 AM EDT, Andy Shevchenko wrote:  

quoted

On Mon, Aug 16, 2021 at 3:39 PM Liam Beguin [off-list ref]
wrote:  

quoted

On Mon Aug 16, 2021 at 4:04 AM EDT, Andy Shevchenko wrote:  

quoted

On Mon, Aug 16, 2021 at 12:35 AM Liam Beguin [off-list ref]
wrote:  

...

quoted

quoted

quoted

quoted

quoted

+       tmp = 4096000;
+       ret = device_property_read_u32(dev, "adi,internal-ref-microvolt", &tmp);  

quoted

+       if (ret < 0 && ret != -EINVAL) {  

Hi Andy,
 

quoted

What does this check (second part) is supposed to mean?
The first part will make it mandatory, is it the goal?
 

device_property_read_u32() will return -EINVAL if the property isn't
found in the devicetree.

This checks for errors when the property is defined while keeping it
optional.  

Don't assign and don't check the error code of the API. As simply as
that.  

I'm not against getting rid of it, but I was asked to check for these
errors in earlier revisions of the patch.  

Okay, I leave it to you, guys, to decide, just note that the usual
pattern for optional stuff
a) either check for (!ret);
b) or ignore the returned value completely.

Hi Jonathan,

Hmm. My thinking (I suspect I asked for it to be checked, but can't remember :)
was that I'd really like to know if a device tree contains a property but that
property is invalid for some reason. The docs give a bunch of reasons beyond
the property not existing (which is unhelpfully described as just 'invalid parameters'). 

I guess that's a bit far fetched.  Let's drop the check as Andy suggests.

Understood, Thanks for making the change.

Liam

Dropped that check and applied to the togreg branch of iio.git, initially pushed out
as testing for 0-day to poke at it.  + we are about to enter merge window so I don't
want linux-next to pick it up just yet!

Jonathan

quoted

quoted

quoted

quoted

quoted

quoted

+               dev_err(dev, "invalid value for adi,internal-ref-microvolt\n");
+               return ret;
+       }