Re: [PATCH 1/2] device property: Add optional nargs_prop for get_reference_args

[Sakari Ailus]

Hi Sean,

On Mon, Apr 07, 2025 at 06:37:13PM -0400, Sean Anderson wrote:
> get_reference_args does not permit falling back to nargs when nargs_prop
> is missing. This makes it difficult to support older devicetrees where
> nargs_prop may not be present. Add support for this by converting nargs
> to a signed value. Where before nargs was ignored if nargs_prop was
> passed, now nargs is only ignored if it is strictly negative. When it is
> positive, nargs represents the fallback cells to use if nargs_prop is
> absent.

If you don't know either the argument count or have a property that tells
it, there's no way to differentiate phandles from arguments. I'd say such
DTS are broken. Where do they exist?

At the very least this needs to be documented as a workaround and moved to
the OF framework. I wouldn't add such a workaround to swnodes either, the
bugs should be fixed instead.

> 
> Signed-off-by: Sean Anderson <sean.anderson@xxxxxxxxx>
> ---
> 
>  drivers/base/property.c |  4 ++--
>  drivers/base/swnode.c   | 13 +++++++++----
>  drivers/of/property.c   | 10 +++-------
>  include/linux/fwnode.h  |  2 +-
>  4 files changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index c1392743df9c..049f8a6088a1 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -606,7 +606,7 @@ int fwnode_property_get_reference_args(const struct fwnode_handle *fwnode,
>  		return -ENOENT;
>  
>  	ret = fwnode_call_int_op(fwnode, get_reference_args, prop, nargs_prop,
> -				 nargs, index, args);
> +				 nargs_prop ? -1 : nargs, index, args);
>  	if (ret == 0)
>  		return ret;
>  
> @@ -614,7 +614,7 @@ int fwnode_property_get_reference_args(const struct fwnode_handle *fwnode,
>  		return ret;
>  
>  	return fwnode_call_int_op(fwnode->secondary, get_reference_args, prop, nargs_prop,
> -				  nargs, index, args);
> +				  nargs_prop ? -1 : nargs, index, args);
>  }
>  EXPORT_SYMBOL_GPL(fwnode_property_get_reference_args);
>  
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index b1726a3515f6..11af2001478f 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -503,7 +503,7 @@ software_node_get_named_child_node(const struct fwnode_handle *fwnode,
>  static int
>  software_node_get_reference_args(const struct fwnode_handle *fwnode,
>  				 const char *propname, const char *nargs_prop,
> -				 unsigned int nargs, unsigned int index,
> +				 int nargs, unsigned int index,
>  				 struct fwnode_reference_args *args)
>  {
>  	struct swnode *swnode = to_swnode(fwnode);
> @@ -543,10 +543,15 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
>  		error = property_entry_read_int_array(ref->node->properties,
>  						      nargs_prop, sizeof(u32),
>  						      &nargs_prop_val, 1);
> -		if (error)
> +
> +		if (error == -EINVAL) {
> +			if (nargs < 0)
> +				return error;
> +		} else if (error) {
>  			return error;
> -
> -		nargs = nargs_prop_val;
> +		} else {
> +			nargs = nargs_prop_val;
> +		}
>  	}
>  
>  	if (nargs > NR_FWNODE_REFERENCE_ARGS)
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index c1feb631e383..c41190e47111 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1116,19 +1116,15 @@ of_fwnode_get_named_child_node(const struct fwnode_handle *fwnode,
>  static int
>  of_fwnode_get_reference_args(const struct fwnode_handle *fwnode,
>  			     const char *prop, const char *nargs_prop,
> -			     unsigned int nargs, unsigned int index,
> +			     int nargs, unsigned int index,
>  			     struct fwnode_reference_args *args)
>  {
>  	struct of_phandle_args of_args;
>  	unsigned int i;
>  	int ret;
>  
> -	if (nargs_prop)
> -		ret = of_parse_phandle_with_args(to_of_node(fwnode), prop,
> -						 nargs_prop, index, &of_args);
> -	else
> -		ret = of_parse_phandle_with_fixed_args(to_of_node(fwnode), prop,
> -						       nargs, index, &of_args);
> +	ret = __of_parse_phandle_with_args(to_of_node(fwnode), prop, nargs_prop,
> +					   nargs, index, &of_args);
>  	if (ret < 0)
>  		return ret;
>  	if (!args) {
> diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> index 6fa0a268d538..69fe44c68f8c 100644
> --- a/include/linux/fwnode.h
> +++ b/include/linux/fwnode.h
> @@ -163,7 +163,7 @@ struct fwnode_operations {
>  				const char *name);
>  	int (*get_reference_args)(const struct fwnode_handle *fwnode,
>  				  const char *prop, const char *nargs_prop,
> -				  unsigned int nargs, unsigned int index,
> +				  int nargs, unsigned int index,
>  				  struct fwnode_reference_args *args);
>  	struct fwnode_handle *
>  	(*graph_get_next_endpoint)(const struct fwnode_handle *fwnode,

-- 
Regards,

Sakari Ailus