Re: [PATCH v1 1/1] device property: Document how to check for the property presence

[Andy Shevchenko]

On Wed, Mar 18, 2026 at 01:26:28PM +0200, Sakari Ailus wrote:
> On Wed, Mar 18, 2026 at 11:41:57AM +0200, Andy Shevchenko wrote:
> > On Wed, Mar 18, 2026 at 11:10:49AM +0200, Sakari Ailus wrote:
> > > On Wed, Mar 18, 2026 at 10:03:27AM +0200, Andy Shevchenko wrote:
> > > > On Wed, Mar 18, 2026 at 12:27:24AM +0200, Sakari Ailus wrote:
> > > > > On Tue, Mar 17, 2026 at 10:08:28PM +0100, Andy Shevchenko wrote:

...

> > > > > > + * In order to check for the property presence, use device_property_present().
> > > > > 
> > > > > Do you really think we should add this clause for each of these functions?
> > > > 
> > > > Yes, as Guenter pointed out that this has to be documented clearly.
> > > > 
> > > > > I don't think it belongs here.
> > > > 
> > > > And? What should we do then (taking into account my below comments)?
> > > > 
> > > > > The error code list doesn't document what is returned if a property doesn't
> > > > > exist (-EINVAL) and it'd be helpful to add this.
> > > > 
> > > > No, this change is exactly against this. Because using an error code that may
> > > > cover not only that case is at bare minimum fragile and layering violation.
> > > > APIs that require to know the implementation details are not good APIs.
> > > 
> > > I have to say I disagree with that,
> > 
> > And I definitely disagree with tribal knowledge based (implementation details
> > as it's not properly documented and can't, because -EINVAL is overloaded) and
> > confusing approach that is in use and may be amended.
> 
> In general it's a good idea to document API functions' return values. It
> certainly won't add to confusion, will it?

No, but it's already documented. -EINVAL is for "invalid parameters". Of course
one may describe that it's not only that, but also property not present and
possibly other cases hidden deeper in the stack of the backend functions. This
will just show my point, that -EINVAL is not guaranteed to be unique for the
purpose.

> > > there's nothing wrong with checking
> > > error codes if you need to.
> > 
> > If that error code defines the case. Here we have not a such.
> > 
> > The wrong parameter to the function will lead to the same error code
> > which is simply wrong.
> > 
> > > Either way, I checked the original patch. If you really think you need to
> > > check for property presence and use default in the case the property isn't
> > > found and error out on other errors, add helper functions for the purpose
> > > instead of open-coding it all.
> > 
> > You mean adding 20+ helpers (at least 5 for arrays, 5 for single element,
> > and doubled for device_/fwnode_) ?! This sounds like way over verbose
> > approach.
> 
> It's still better than open coding this where differentiating behaviour is
> desired. You'd probably need a few macros to generate the functions for
> each type needed.

There are not so many cases, and actually in some of them open coded variant
suits very well. I do not see enough justification in adding those helpers.

> > > > > It would have been best to have a separate error code for this albeit
> > > > > changing this now might not be that troublesome either: very, very few
> > > > > callers depend on receiving such an error code but there are still many
> > > > > callers.
> > > > 
> > > > I'm against this because we have already a dedicated API to check for property
> > > > presence, why do we need to have another (confusing!) way of doing the same?
> > > > 
> > > > Having a dedicated code may help to debug, but shouldn't be used as a main
> > > > feature in my opinion.
> > 
> > I will send a next version of this patch touching only the present/bool functions.
> 
> Sounds good.
> 
> I'd also drop this from the patch
> 
> + * This function is the correct way to check that given property is present
> + * in the firmware node description.
> 
> as redundant.

Then it will be no sense in the patch, will it be? The whole point is to clarify
how one should use API when the desire is to detect if property is present or not.

So, I send my v2 as I see it useful. Then you can NAK it at your wish.

-- 
With Best Regards,
Andy Shevchenko