Re: [PATCH v1 1/1] hwmon: (isl28022) Don't check for specific errors when parsing properties

Re: [PATCH v1 1/1] hwmon: (isl28022) Don't check for specific errors when parsing properties

[Geert Uytterhoeven]

Hi Andy,

CC devicetree

On Thu, 19 Feb 2026 at 15:06, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Instead of checking for the specific error codes (that can be considered
> a layering violation to some extent) check for the property existence first
> and then either parse it, or apply a default value.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks for your patch!

> --- a/drivers/hwmon/isl28022.c
> +++ b/drivers/hwmon/isl28022.c
> @@ -337,21 +337,28 @@ DEFINE_SHOW_ATTRIBUTE(shunt_voltage);
>   */
>  static int isl28022_read_properties(struct device *dev, struct isl28022_data *data)
>  {
> +       const char *propname;
>         u32 val;
>         int err;
>
> -       err = device_property_read_u32(dev, "shunt-resistor-micro-ohms", &val);
> -       if (err == -EINVAL)
> +       propname = "shunt-resistor-micro-ohms";
> +       if (device_property_present(dev, propname)) {
> +               err = device_property_read_u32(dev, propname, &val);
> +               if (err)
> +                       return err;
> +       } else {
>                 val = 10000;
> -       else if (err < 0)
> -               return err;
> +       }
>         data->shunt = val;

IIRC, we have removed superfluous presence checks all over the tree
during the past few years? E.g. of_property_read_*() is documented to
return -EINVAL if a property does not exist.
So this patch looks like a step back to me...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v1 1/1] hwmon: (isl28022) Don't check for specific errors when parsing properties

[Andy Shevchenko]

On Thu, Feb 19, 2026 at 03:21:29PM +0100, Geert Uytterhoeven wrote:
> On Thu, 19 Feb 2026 at 15:06, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > Instead of checking for the specific error codes (that can be considered
> > a layering violation to some extent) check for the property existence first
> > and then either parse it, or apply a default value.

> IIRC, we have removed superfluous presence checks all over the tree
> during the past few years? E.g. of_property_read_*() is documented to
> return -EINVAL if a property does not exist.

Even though, it's still fragile. When we have a check for explicit device
presence, we wouldn't care of the error code we get in case of unsuccessful
parsing.

> So this patch looks like a step back to me...

Obviously I have a disagreement here, this is step forward to weaken
the dependency on the certain error code in the cases when we can avoid
that. Motivation is mentioned in the commit message.

Also note, -EINVAL can sneak in tons of mysterious ways as it's one of
the most overloaded error code in the kernel, its semantic is basically
equals to "an error happened".

Having the code above, we make it robust against some subtle nuances which
may not be discovered in time.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] hwmon: (isl28022) Don't check for specific errors when parsing properties

[Guenter Roeck]

On 2/19/26 06:30, Andy Shevchenko wrote:
> On Thu, Feb 19, 2026 at 03:21:29PM +0100, Geert Uytterhoeven wrote:
>> On Thu, 19 Feb 2026 at 15:06, Andy Shevchenko
>> <andriy.shevchenko@linux.intel.com> wrote:
>>> Instead of checking for the specific error codes (that can be considered
>>> a layering violation to some extent) check for the property existence first
>>> and then either parse it, or apply a default value.
> 
>> IIRC, we have removed superfluous presence checks all over the tree
>> during the past few years? E.g. of_property_read_*() is documented to
>> return -EINVAL if a property does not exist.
> 
> Even though, it's still fragile. When we have a check for explicit device
> presence, we wouldn't care of the error code we get in case of unsuccessful
> parsing.
> 
>> So this patch looks like a step back to me...
> 
> Obviously I have a disagreement here, this is step forward to weaken
> the dependency on the certain error code in the cases when we can avoid
> that. Motivation is mentioned in the commit message.
> 
> Also note, -EINVAL can sneak in tons of mysterious ways as it's one of
> the most overloaded error code in the kernel, its semantic is basically
> equals to "an error happened".
> 
> Having the code above, we make it robust against some subtle nuances which
> may not be discovered in time.
> 

Is that documented somewhere ? I have been asking people to use the current
approach to reduce code size. device_property_present() isn't even mentioned
as API in Documentation/. If "do not rely on error codes from device property
API functions to determine if a default should be applied" or similar is a new
rule or guidance, it should be clearly documented somewhere.

Thanks,
Guenter

Re: [PATCH v1 1/1] hwmon: (isl28022) Don't check for specific errors when parsing properties

[Andy Shevchenko]

On Fri, Feb 20, 2026 at 12:49:25PM -0800, Guenter Roeck wrote:
> On 2/19/26 06:30, Andy Shevchenko wrote:
> > On Thu, Feb 19, 2026 at 03:21:29PM +0100, Geert Uytterhoeven wrote:
> > > On Thu, 19 Feb 2026 at 15:06, Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > Instead of checking for the specific error codes (that can be considered
> > > > a layering violation to some extent) check for the property existence first
> > > > and then either parse it, or apply a default value.
> > 
> > > IIRC, we have removed superfluous presence checks all over the tree
> > > during the past few years? E.g. of_property_read_*() is documented to
> > > return -EINVAL if a property does not exist.
> > 
> > Even though, it's still fragile. When we have a check for explicit device
> > presence, we wouldn't care of the error code we get in case of unsuccessful
> > parsing.
> > 
> > > So this patch looks like a step back to me...
> > 
> > Obviously I have a disagreement here, this is step forward to weaken
> > the dependency on the certain error code in the cases when we can avoid
> > that. Motivation is mentioned in the commit message.
> > 
> > Also note, -EINVAL can sneak in tons of mysterious ways as it's one of
> > the most overloaded error code in the kernel, its semantic is basically
> > equals to "an error happened".
> > 
> > Having the code above, we make it robust against some subtle nuances which
> > may not be discovered in time.
> > 
> 
> Is that documented somewhere ? I have been asking people to use the current
> approach to reduce code size. device_property_present() isn't even mentioned
> as API in Documentation/. If "do not rely on error codes from device property
> API functions to determine if a default should be applied" or similar is a new
> rule or guidance, it should be clearly documented somewhere.

Fair enough.

Currently if one reads kernel-doc for fwnode_property_*() / device_property_*()
APIs, there is no clear specification that -EINVAL is equal to "property not
present". It's documented as

 *         %-EINVAL if given arguments are not valid,

which may be the case, if the parameter itself is wrong, for example fwnode is
invalid to begin with. Checking against -EINVAL for 'property not present' is
layering violation and relying on deep implementation detail.

I will craft a change to put something like this to the
fwnode_property_present() and device_property_present() kernel-docs.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] hwmon: (isl28022) Don't check for specific errors when parsing properties

[Andy Shevchenko]

On Thu, Feb 19, 2026 at 03:21:29PM +0100, Geert Uytterhoeven wrote:
> On Thu, 19 Feb 2026 at 15:06, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > Instead of checking for the specific error codes (that can be considered
> > a layering violation to some extent) check for the property existence first
> > and then either parse it, or apply a default value.

> IIRC, we have removed superfluous presence checks all over the tree
> during the past few years? E.g. of_property_read_*() is documented to
> return -EINVAL if a property does not exist.

Implementation detail and actually not accurate.

> So this patch looks like a step back to me...

Not to me, just sent
20260317210828.2117631-1-andriy.shevchenko@linux.intel.com
to clarify in the documentation what's this about.

-- 
With Best Regards,
Andy Shevchenko