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

[Guenter Roeck]

On Wed, Mar 18, 2026 at 12:27:24AM +0200, Sakari Ailus wrote:
> Hi Andy,
> 
> Thanks for the patch.
> 
> On Tue, Mar 17, 2026 at 10:08:28PM +0100, Andy Shevchenko wrote:
> > Currently it's unclear if one may or may not rely on the error codes
> > returned from the property getters to check for the property presence.
> > Clarify this by mass updating kernel-doc for fwnode_property_*() and
> > device_property_*() where it's applicable.
> > 
> > Reported-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> > Closes: 4b24f1f4-b395-467a-81b7-1334a2d48845@xxxxxxxxxxxx
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > ---
> >  drivers/base/property.c | 38 +++++++++++++++++++++++++++++++++++---
> >  1 file changed, 35 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/base/property.c b/drivers/base/property.c
> > index 8d9a34be57fb..bffa0070ab13 100644
> > --- a/drivers/base/property.c
> > +++ b/drivers/base/property.c
> > @@ -38,6 +38,8 @@ EXPORT_SYMBOL_GPL(__dev_fwnode_const);
> >   * @propname: Name of the property
> >   *
> >   * Check if property @propname is present in the device firmware description.
> > + * This function is the correct way to check that given property is present
> > + * in the device firmware description.
> >   *
> >   * Return: true if property @propname is present. Otherwise, returns false.
> >   */
> > @@ -52,6 +54,10 @@ EXPORT_SYMBOL_GPL(device_property_present);
> >   * @fwnode: Firmware node whose property to check
> >   * @propname: Name of the property
> >   *
> > + * Check if property @propname is present in the firmware node description.
> > + * This function is the correct way to check that given property is present
> > + * in the firmware node description.
> > + *
> >   * Return: true if property @propname is present. Otherwise, returns false.
> >   */
> >  bool fwnode_property_present(const struct fwnode_handle *fwnode,
> > @@ -75,9 +81,9 @@ EXPORT_SYMBOL_GPL(fwnode_property_present);
> >   * @dev: Device whose property is being checked
> >   * @propname: Name of the property
> >   *
> > - * Return if property @propname is true or false in the device firmware description.
> > + * Use device_property_present() to check for the property presence.
> >   *
> > - * Return: true if property @propname is present. Otherwise, returns false.
> > + * Return: if property @propname is true or false in the device firmware description.
> >   */
> >  bool device_property_read_bool(const struct device *dev, const char *propname)
> >  {
> > @@ -90,7 +96,9 @@ EXPORT_SYMBOL_GPL(device_property_read_bool);
> >   * @fwnode: Firmware node whose property to check
> >   * @propname: Name of the property
> >   *
> > - * Return if property @propname is true or false in the firmware description.
> > + * Use fwnode_property_present() to check for the property presence.
> > + *
> > + * Return: if property @propname is true or false in the firmware node description.
> >   */
> >  bool fwnode_property_read_bool(const struct fwnode_handle *fwnode,
> >  			     const char *propname)
> > @@ -121,6 +129,8 @@ EXPORT_SYMBOL_GPL(fwnode_property_read_bool);
> >   * It's recommended to call device_property_count_u8() instead of calling
> >   * this function with @val equals %NULL and @nval equals 0.
> >   *
> > + * 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?
> I don't think it belongs here.
> 
> 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. 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 also find it a bit confusing since the _bool functions are defined to return
false if the property is not present, and true if it is.

Guenter

> > + *
> >   * Return: number of values if @val was %NULL,
> >   *         %0 if the property was found (success),
> >   *	   %-EINVAL if given arguments are not valid,
> > @@ -149,6 +159,8 @@ EXPORT_SYMBOL_GPL(device_property_read_u8_array);
> >   * It's recommended to call device_property_count_u16() instead of calling
> >   * this function with @val equals %NULL and @nval equals 0.
> >   *
> > + * In order to check for the property presence, use device_property_present().
> > + *
> >   * Return: number of values if @val was %NULL,
> >   *         %0 if the property was found (success),
> >   *	   %-EINVAL if given arguments are not valid,
> > @@ -177,6 +189,8 @@ EXPORT_SYMBOL_GPL(device_property_read_u16_array);
> >   * It's recommended to call device_property_count_u32() instead of calling
> >   * this function with @val equals %NULL and @nval equals 0.
> >   *
> > + * In order to check for the property presence, use device_property_present().
> > + *
> >   * Return: number of values if @val was %NULL,
> >   *         %0 if the property was found (success),
> >   *	   %-EINVAL if given arguments are not valid,
> > @@ -205,6 +219,8 @@ EXPORT_SYMBOL_GPL(device_property_read_u32_array);
> >   * It's recommended to call device_property_count_u64() instead of calling
> >   * this function with @val equals %NULL and @nval equals 0.
> >   *
> > + * In order to check for the property presence, use device_property_present().
> > + *
> >   * Return: number of values if @val was %NULL,
> >   *         %0 if the property was found (success),
> >   *	   %-EINVAL if given arguments are not valid,
> > @@ -233,6 +249,8 @@ EXPORT_SYMBOL_GPL(device_property_read_u64_array);
> >   * It's recommended to call device_property_string_array_count() instead of calling
> >   * this function with @val equals %NULL and @nval equals 0.
> >   *
> > + * In order to check for the property presence, use device_property_present().
> > + *
> >   * Return: number of values read on success if @val is non-NULL,
> >   *	   number of values available on success if @val is NULL,
> >   *	   %-EINVAL if given arguments are not valid,
> > @@ -257,6 +275,8 @@ EXPORT_SYMBOL_GPL(device_property_read_string_array);
> >   * Function reads property @propname from the device firmware description and
> >   * stores the value into @val if found. The value is checked to be a string.
> >   *
> > + * In order to check for the property presence, use device_property_present().
> > + *
> >   * Return: %0 if the property was found (success),
> >   *	   %-EINVAL if given arguments are not valid,
> >   *	   %-ENODATA if the property does not have a value,
> > @@ -324,6 +344,8 @@ static int fwnode_property_read_int_array(const struct fwnode_handle *fwnode,
> >   * It's recommended to call fwnode_property_count_u8() instead of calling
> >   * this function with @val equals %NULL and @nval equals 0.
> >   *
> > + * In order to check for the property presence, use fwnode_property_present().
> > + *
> >   * Return: number of values if @val was %NULL,
> >   *         %0 if the property was found (success),
> >   *	   %-EINVAL if given arguments are not valid,
> > @@ -353,6 +375,8 @@ EXPORT_SYMBOL_GPL(fwnode_property_read_u8_array);
> >   * It's recommended to call fwnode_property_count_u16() instead of calling
> >   * this function with @val equals %NULL and @nval equals 0.
> >   *
> > + * In order to check for the property presence, use fwnode_property_present().
> > + *
> >   * Return: number of values if @val was %NULL,
> >   *         %0 if the property was found (success),
> >   *	   %-EINVAL if given arguments are not valid,
> > @@ -382,6 +406,8 @@ EXPORT_SYMBOL_GPL(fwnode_property_read_u16_array);
> >   * It's recommended to call fwnode_property_count_u32() instead of calling
> >   * this function with @val equals %NULL and @nval equals 0.
> >   *
> > + * In order to check for the property presence, use fwnode_property_present().
> > + *
> >   * Return: number of values if @val was %NULL,
> >   *         %0 if the property was found (success),
> >   *	   %-EINVAL if given arguments are not valid,
> > @@ -411,6 +437,8 @@ EXPORT_SYMBOL_GPL(fwnode_property_read_u32_array);
> >   * It's recommended to call fwnode_property_count_u64() instead of calling
> >   * this function with @val equals %NULL and @nval equals 0.
> >   *
> > + * In order to check for the property presence, use fwnode_property_present().
> > + *
> >   * Return: number of values if @val was %NULL,
> >   *         %0 if the property was found (success),
> >   *	   %-EINVAL if given arguments are not valid,
> > @@ -440,6 +468,8 @@ EXPORT_SYMBOL_GPL(fwnode_property_read_u64_array);
> >   * It's recommended to call fwnode_property_string_array_count() instead of calling
> >   * this function with @val equals %NULL and @nval equals 0.
> >   *
> > + * In order to check for the property presence, use fwnode_property_present().
> > + *
> >   * Return: number of values read on success if @val is non-NULL,
> >   *	   number of values available on success if @val is NULL,
> >   *	   %-EINVAL if given arguments are not valid,
> > @@ -476,6 +506,8 @@ EXPORT_SYMBOL_GPL(fwnode_property_read_string_array);
> >   * Read property @propname from the given firmware node and store the value into
> >   * @val if found.  The value is checked to be a string.
> >   *
> > + * In order to check for the property presence, use fwnode_property_present().
> > + *
> >   * Return: %0 if the property was found (success),
> >   *	   %-EINVAL if given arguments are not valid,
> >   *	   %-ENODATA if the property does not have a value,
> 
> -- 
> Kind regards,
> 
> Sakari Ailus