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

[Sakari Ailus]

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?

> 
> > 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.

> 
> > > > 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.

-- 
Regards,

Sakari Ailus