[PATCH 0/2] device property: Add fwnode_property_get_reference_optional_args

[PATCH 0/2] device property: Add fwnode_property_get_reference_optional_args

[Sean Anderson]

This series adds a fwnode-equivalent to of_parse_phandle_with_optional_args.


Sean Anderson (2):
  device property: Add optional nargs_prop for get_reference_args
  device property: Add fwnode_property_get_reference_optional_args

 drivers/base/property.c  | 50 ++++++++++++++++++++++++++++++++++++++--
 drivers/base/swnode.c    | 13 +++++++----
 drivers/of/property.c    | 10 +++-----
 include/linux/fwnode.h   |  2 +-
 include/linux/property.h |  4 ++++
 5 files changed, 65 insertions(+), 14 deletions(-)

-- 
2.35.1.1320.gc452695387.dirty

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

[Sean Anderson]

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.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 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,
-- 
2.35.1.1320.gc452695387.dirty

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

[Andy Shevchenko]

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.

And what is the case to support old DTs on most likely outdated hardware?

...

>  	ret = fwnode_call_int_op(fwnode, get_reference_args, prop, nargs_prop,
> -				 nargs, index, args);
> +				 nargs_prop ? -1 : nargs, index, args);

>  	return fwnode_call_int_op(fwnode->secondary, get_reference_args, prop, nargs_prop,
> -				  nargs, index, args);
> +				  nargs_prop ? -1 : nargs, index, args);

I don't understand why it's needed here. The nargs_prop is passed to the callee.

...

> -				 unsigned int nargs, unsigned int index,
> +				 int nargs, unsigned int index,

As per above.

...

>  		error = property_entry_read_int_array(ref->node->properties,
>  						      nargs_prop, sizeof(u32),
>  						      &nargs_prop_val, 1);
> -		if (error)

> +

Stray blank line.

> +		if (error == -EINVAL) {

Why do we need an explicit error code check? This is fragile. Just check the
parameter before calling the above.

> +			if (nargs < 0)
> +				return error;
> +		} else if (error) {
>  			return error;
> -
> -		nargs = nargs_prop_val;
> +		} else {
> +			nargs = nargs_prop_val;
> +		}

...

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

Same comments as per above.

-- 
With Best Regards,
Andy Shevchenko

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

[Rob Herring]

On Tue, Apr 8, 2025 at 3:37 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> 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.
>
> And what is the case to support old DTs on most likely outdated hardware?

People still care when I break 1990s PowerMacs...

It's more that some bindings (like MSI) start out without #foo-cells,
and then we end up adding arg cells later. So we have to support no
#foo-cells means 0.

Rob

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@linux.dev>
> ---
> 
>  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

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

[Rob Herring]

On Tue, Apr 8, 2025 at 4:14 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> 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?

#msi-cells for one is optional because we added it after the initial
bindings and missing means 0.

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

The DT API supports this already.

Rob

[PATCH 2/2] device property: Add fwnode_property_get_reference_optional_args

[Sean Anderson]

Add a fwnode variant of of_parse_phandle_with_optional_args to allow
nargs_prop to be absent from the referenced node. This improves
compatibility for references where the devicetree might not always have
nargs_prop.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 drivers/base/property.c  | 46 ++++++++++++++++++++++++++++++++++++++++
 include/linux/property.h |  4 ++++
 2 files changed, 50 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 049f8a6088a1..ef13ca32079b 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -618,6 +618,52 @@ int fwnode_property_get_reference_args(const struct fwnode_handle *fwnode,
 }
 EXPORT_SYMBOL_GPL(fwnode_property_get_reference_args);
 
+/**
+ * fwnode_property_get_reference_optional_args() - Find a reference with optional arguments
+ * @fwnode:	Firmware node where to look for the reference
+ * @prop:	The name of the property
+ * @nargs_prop:	The name of the property telling the number of
+ *		arguments in the referred node.
+ * @index:	Index of the reference, from zero onwards.
+ * @args:	Result structure with reference and integer arguments.
+ *		May be NULL.
+ *
+ * Obtain a reference based on a named property in an fwnode, with
+ * integer arguments. If @nargs_prop is absent from the referenced node, then
+ * number of arguments is be assumed to be 0.
+ *
+ * The caller is responsible for calling fwnode_handle_put() on the returned
+ * @args->fwnode pointer.
+ *
+ * Return: %0 on success
+ *	    %-ENOENT when the index is out of bounds, the index has an empty
+ *		     reference or the property was not found
+ *	    %-EINVAL on parse error
+ */
+int fwnode_property_get_reference_optional_args(const struct fwnode_handle *fwnode,
+						const char *prop,
+						const char *nargs_prop,
+						unsigned int index,
+						struct fwnode_reference_args *args)
+{
+	int ret;
+
+	if (IS_ERR_OR_NULL(fwnode))
+		return -ENOENT;
+
+	ret = fwnode_call_int_op(fwnode, get_reference_args, prop, nargs_prop,
+				 0, index, args);
+	if (ret == 0)
+		return ret;
+
+	if (IS_ERR_OR_NULL(fwnode->secondary))
+		return ret;
+
+	return fwnode_call_int_op(fwnode->secondary, get_reference_args, prop, nargs_prop,
+				  0, index, args);
+}
+EXPORT_SYMBOL_GPL(fwnode_property_get_reference_optional_args);
+
 /**
  * fwnode_find_reference - Find named reference to a fwnode_handle
  * @fwnode: Firmware node where to look for the reference
diff --git a/include/linux/property.h b/include/linux/property.h
index e214ecd241eb..a1662b36d15f 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -139,6 +139,10 @@ int fwnode_property_get_reference_args(const struct fwnode_handle *fwnode,
 				       const char *prop, const char *nargs_prop,
 				       unsigned int nargs, unsigned int index,
 				       struct fwnode_reference_args *args);
+int fwnode_property_get_reference_optional_args(const struct fwnode_handle *fwnode,
+						const char *prop, const char *nargs_prop,
+						unsigned int index,
+						struct fwnode_reference_args *args);
 
 struct fwnode_handle *fwnode_find_reference(const struct fwnode_handle *fwnode,
 					    const char *name,
-- 
2.35.1.1320.gc452695387.dirty

Re: [PATCH 2/2] device property: Add fwnode_property_get_reference_optional_args

[Andy Shevchenko]

On Mon, Apr 07, 2025 at 06:37:14PM -0400, Sean Anderson wrote:
> Add a fwnode variant of of_parse_phandle_with_optional_args to allow
> nargs_prop to be absent from the referenced node. This improves
> compatibility for references where the devicetree might not always have
> nargs_prop.

...

> +/**
> + * fwnode_property_get_reference_optional_args() - Find a reference with optional arguments
> + * @fwnode:	Firmware node where to look for the reference
> + * @prop:	The name of the property
> + * @nargs_prop:	The name of the property telling the number of

Use space instead of TAB as it's already too long to make it aligned with the
rest.

> + *		arguments in the referred node.
> + * @index:	Index of the reference, from zero onwards.
> + * @args:	Result structure with reference and integer arguments.
> + *		May be NULL.
> + *
> + * Obtain a reference based on a named property in an fwnode, with
> + * integer arguments. If @nargs_prop is absent from the referenced node, then
> + * number of arguments is be assumed to be 0.
> + *
> + * The caller is responsible for calling fwnode_handle_put() on the returned
> + * @args->fwnode pointer.
> + *
> + * Return: %0 on success
> + *	    %-ENOENT when the index is out of bounds, the index has an empty
> + *		     reference or the property was not found
> + *	    %-EINVAL on parse error
> + */
> +int fwnode_property_get_reference_optional_args(const struct fwnode_handle *fwnode,
> +						const char *prop,
> +						const char *nargs_prop,
> +						unsigned int index,
> +						struct fwnode_reference_args *args)
> +{
> +	int ret;

> +	if (IS_ERR_OR_NULL(fwnode))
> +		return -ENOENT;

This is incorrect most likely, see below.

> +	ret = fwnode_call_int_op(fwnode, get_reference_args, prop, nargs_prop,
> +				 0, index, args);
> +	if (ret == 0)
> +		return ret;
> +
> +	if (IS_ERR_OR_NULL(fwnode->secondary))
> +		return ret;

Here no such error code shadowing, and TBH I do not like the shadowing without
real need.

> +	return fwnode_call_int_op(fwnode->secondary, get_reference_args, prop, nargs_prop,
> +				  0, index, args);
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] device property: Add fwnode_property_get_reference_optional_args

[Sean Anderson]

On 4/8/25 04:39, Andy Shevchenko wrote:
> On Mon, Apr 07, 2025 at 06:37:14PM -0400, Sean Anderson wrote:
>> Add a fwnode variant of of_parse_phandle_with_optional_args to allow
>> nargs_prop to be absent from the referenced node. This improves
>> compatibility for references where the devicetree might not always have
>> nargs_prop.
> 
> ...
> 
>> +/**
>> + * fwnode_property_get_reference_optional_args() - Find a reference with optional arguments
>> + * @fwnode:	Firmware node where to look for the reference
>> + * @prop:	The name of the property
>> + * @nargs_prop:	The name of the property telling the number of
> 
> Use space instead of TAB as it's already too long to make it aligned with the
> rest.
> 
>> + *		arguments in the referred node.
>> + * @index:	Index of the reference, from zero onwards.
>> + * @args:	Result structure with reference and integer arguments.
>> + *		May be NULL.
>> + *
>> + * Obtain a reference based on a named property in an fwnode, with
>> + * integer arguments. If @nargs_prop is absent from the referenced node, then
>> + * number of arguments is be assumed to be 0.
>> + *
>> + * The caller is responsible for calling fwnode_handle_put() on the returned
>> + * @args->fwnode pointer.
>> + *
>> + * Return: %0 on success
>> + *	    %-ENOENT when the index is out of bounds, the index has an empty
>> + *		     reference or the property was not found
>> + *	    %-EINVAL on parse error
>> + */
>> +int fwnode_property_get_reference_optional_args(const struct fwnode_handle *fwnode,
>> +						const char *prop,
>> +						const char *nargs_prop,
>> +						unsigned int index,
>> +						struct fwnode_reference_args *args)
>> +{
>> +	int ret;
> 
>> +	if (IS_ERR_OR_NULL(fwnode))
>> +		return -ENOENT;
> 
> This is incorrect most likely, see below.
> 
>> +	ret = fwnode_call_int_op(fwnode, get_reference_args, prop, nargs_prop,
>> +				 0, index, args);
>> +	if (ret == 0)
>> +		return ret;
>> +
>> +	if (IS_ERR_OR_NULL(fwnode->secondary))
>> +		return ret;
> 
> Here no such error code shadowing, and TBH I do not like the shadowing without
> real need.

I don't understand the objection. First, this logic is identical to
fwnode_property_get_reference_args. Second, the process seems clear to
me:

- If we have a primary fwnode, try it otherwise return -ENOENT
- If we have a secondary fwnode and the first failed, try it otherwise
  return the original error code

The purpose of a secondary fwnode is to allow supplying missing
properties absent from the primary fwnode. Which part of the above do
you dislike?

--Sean

Re: [PATCH 2/2] device property: Add fwnode_property_get_reference_optional_args

[Rob Herring]

On Mon, Apr 7, 2025 at 5:37 PM Sean Anderson <sean.anderson@linux.dev> wrote:
>
> Add a fwnode variant of of_parse_phandle_with_optional_args to allow
> nargs_prop to be absent from the referenced node. This improves
> compatibility for references where the devicetree might not always have
> nargs_prop.

Can't we just make fwnode_property_get_reference_args() handle this
case? Or why is it not just a 1 line wrapper function?

Re: [PATCH 2/2] device property: Add fwnode_property_get_reference_optional_args

[Sean Anderson]

On 4/8/25 09:00, Rob Herring wrote:
> On Mon, Apr 7, 2025 at 5:37 PM Sean Anderson <sean.anderson@linux.dev> wrote:
>>
>> Add a fwnode variant of of_parse_phandle_with_optional_args to allow
>> nargs_prop to be absent from the referenced node. This improves
>> compatibility for references where the devicetree might not always have
>> nargs_prop.
> 
> Can't we just make fwnode_property_get_reference_args() handle this
> case? Or why is it not just a 1 line wrapper function?

fwnode_property_get_reference_args ignores nargs when nargs_prop is
non-NULL. So all the existing callers just pass 0 to nargs. Rather than
convert them, I chose to add another function with different defaults.
There are only four callers that pass nargs_prop, so I could just as
easily change the callers instead.

--Sean

Re: [PATCH 2/2] device property: Add fwnode_property_get_reference_optional_args

[Rob Herring]

On Tue, Apr 8, 2025 at 10:12 AM Sean Anderson <sean.anderson@linux.dev> wrote:
>
> On 4/8/25 09:00, Rob Herring wrote:
> > On Mon, Apr 7, 2025 at 5:37 PM Sean Anderson <sean.anderson@linux.dev> wrote:
> >>
> >> Add a fwnode variant of of_parse_phandle_with_optional_args to allow
> >> nargs_prop to be absent from the referenced node. This improves
> >> compatibility for references where the devicetree might not always have
> >> nargs_prop.
> >
> > Can't we just make fwnode_property_get_reference_args() handle this
> > case? Or why is it not just a 1 line wrapper function?
>
> fwnode_property_get_reference_args ignores nargs when nargs_prop is
> non-NULL. So all the existing callers just pass 0 to nargs. Rather than
> convert them, I chose to add another function with different defaults.
> There are only four callers that pass nargs_prop, so I could just as
> easily change the callers instead.

Why do you have to change the callers? nargs value won't matter
because they obviously have nargs_prop present or they would not have
worked in the first place. If behavior changes because there's an
error in their DT, who cares. That's their problem for not validating
the DT.

Rob

Re: [PATCH 2/2] device property: Add fwnode_property_get_reference_optional_args

[Sean Anderson]

On 4/8/25 11:19, Rob Herring wrote:
> On Tue, Apr 8, 2025 at 10:12 AM Sean Anderson <sean.anderson@linux.dev> wrote:
>>
>> On 4/8/25 09:00, Rob Herring wrote:
>> > On Mon, Apr 7, 2025 at 5:37 PM Sean Anderson <sean.anderson@linux.dev> wrote:
>> >>
>> >> Add a fwnode variant of of_parse_phandle_with_optional_args to allow
>> >> nargs_prop to be absent from the referenced node. This improves
>> >> compatibility for references where the devicetree might not always have
>> >> nargs_prop.
>> >
>> > Can't we just make fwnode_property_get_reference_args() handle this
>> > case? Or why is it not just a 1 line wrapper function?
>>
>> fwnode_property_get_reference_args ignores nargs when nargs_prop is
>> non-NULL. So all the existing callers just pass 0 to nargs. Rather than
>> convert them, I chose to add another function with different defaults.
>> There are only four callers that pass nargs_prop, so I could just as
>> easily change the callers instead.
> 
> Why do you have to change the callers? nargs value won't matter
> because they obviously have nargs_prop present or they would not have
> worked in the first place. If behavior changes because there's an
> error in their DT, who cares. That's their problem for not validating
> the DT.

Because the change would be to make nargs matter even when nargs_prop is
present. For the sake of example, consider something like

  foo: foo {
    #my-cells = <1>;
  };
  
  bar: bar {
  };

  baz {
    my-prop = <&bar>, <&foo 5>, ;
    my-prop-names = "bar", "foo";
  };

Before we would have

fwnode_property_get_reference_args(baz, "my-prop", NULL, 0, "bar", args) <bar>
fwnode_property_get_reference_args(baz, "my-prop", NULL, 0, "foo", args) <foo>
fwnode_property_get_reference_args(baz, "my-prop", "#my-cells", -1, "bar", args) ERROR
fwnode_property_get_reference_args(baz, "my-prop", "#my-cells", -1, "foo", args) ERROR
fwnode_property_get_reference_args(baz, "my-prop", "#my-cells", 0, "bar", args) ERROR
fwnode_property_get_reference_args(baz, "my-prop", "#my-cells", 0, "foo", args) ERROR

and after we would have

fwnode_property_get_reference_args(baz, "my-prop", NULL, 0, "bar", args) <bar>
fwnode_property_get_reference_args(baz, "my-prop", NULL, 0, "foo", args) <foo>
fwnode_property_get_reference_args(baz, "my-prop", "#my-cells", -1, "bar", args) ERROR
fwnode_property_get_reference_args(baz, "my-prop", "#my-cells", -1, "foo", args) ERROR
fwnode_property_get_reference_args(baz, "my-prop", "#my-cells", 0, "bar", args) <bar>
fwnode_property_get_reference_args(baz, "my-prop", "#my-cells", 0, "foo", args) <foo 5>

The problem is that all existing callers pass nargs=0 when
nargs_prop="#my-cells" so they will get the new behavior even when they
shouldn't. So if we change the behavior we have to change the callers
too. If we make a new function with new behavior the callers stay the
same.

--Sean

Re: [PATCH 2/2] device property: Add fwnode_property_get_reference_optional_args

[Rob Herring]

On Tue, Apr 8, 2025 at 10:28 AM Sean Anderson <sean.anderson@linux.dev> wrote:
>
> On 4/8/25 11:19, Rob Herring wrote:
> > On Tue, Apr 8, 2025 at 10:12 AM Sean Anderson <sean.anderson@linux.dev> wrote:
> >>
> >> On 4/8/25 09:00, Rob Herring wrote:
> >> > On Mon, Apr 7, 2025 at 5:37 PM Sean Anderson <sean.anderson@linux.dev> wrote:
> >> >>
> >> >> Add a fwnode variant of of_parse_phandle_with_optional_args to allow
> >> >> nargs_prop to be absent from the referenced node. This improves
> >> >> compatibility for references where the devicetree might not always have
> >> >> nargs_prop.
> >> >
> >> > Can't we just make fwnode_property_get_reference_args() handle this
> >> > case? Or why is it not just a 1 line wrapper function?
> >>
> >> fwnode_property_get_reference_args ignores nargs when nargs_prop is
> >> non-NULL. So all the existing callers just pass 0 to nargs. Rather than
> >> convert them, I chose to add another function with different defaults.
> >> There are only four callers that pass nargs_prop, so I could just as
> >> easily change the callers instead.
> >
> > Why do you have to change the callers? nargs value won't matter
> > because they obviously have nargs_prop present or they would not have
> > worked in the first place. If behavior changes because there's an
> > error in their DT, who cares. That's their problem for not validating
> > the DT.
>
> Because the change would be to make nargs matter even when nargs_prop is
> present. For the sake of example, consider something like
>
>   foo: foo {
>     #my-cells = <1>;
>   };
>
>   bar: bar {
>   };
>
>   baz {
>     my-prop = <&bar>, <&foo 5>, ;
>     my-prop-names = "bar", "foo";
>   };
>
> Before we would have
>
> fwnode_property_get_reference_args(baz, "my-prop", NULL, 0, "bar", args) <bar>
> fwnode_property_get_reference_args(baz, "my-prop", NULL, 0, "foo", args) <foo>
> fwnode_property_get_reference_args(baz, "my-prop", "#my-cells", -1, "bar", args) ERROR
> fwnode_property_get_reference_args(baz, "my-prop", "#my-cells", -1, "foo", args) ERROR
> fwnode_property_get_reference_args(baz, "my-prop", "#my-cells", 0, "bar", args) ERROR
> fwnode_property_get_reference_args(baz, "my-prop", "#my-cells", 0, "foo", args) ERROR
>
> and after we would have
>
> fwnode_property_get_reference_args(baz, "my-prop", NULL, 0, "bar", args) <bar>
> fwnode_property_get_reference_args(baz, "my-prop", NULL, 0, "foo", args) <foo>
> fwnode_property_get_reference_args(baz, "my-prop", "#my-cells", -1, "bar", args) ERROR
> fwnode_property_get_reference_args(baz, "my-prop", "#my-cells", -1, "foo", args) ERROR
> fwnode_property_get_reference_args(baz, "my-prop", "#my-cells", 0, "bar", args) <bar>
> fwnode_property_get_reference_args(baz, "my-prop", "#my-cells", 0, "foo", args) <foo 5>
>
> The problem is that all existing callers pass nargs=0 when
> nargs_prop="#my-cells" so they will get the new behavior even when they
> shouldn't. So if we change the behavior we have to change the callers
> too. If we make a new function with new behavior the callers stay the
> same.

It does not matter! It is not the kernel's job to validate the DT. If
it was: it does a terrible job at it and we wouldn't have needed
dtschema. In your example, the change actually makes things work
despite the error! Why would you not want that? No one is relying on
the last 2 cases returning an error other than to debug their DT.

That being said, looking at the DT side, we have
of_parse_phandle_with_args(), of_parse_phandle_with_fixed_args(), and
of_parse_phandle_with_optional_args(). We should mirror that API here
as you have done. So to rephrase, make
fwnode_property_get_reference_args() contents a static function and
then both fwnode_property_get_reference_args() and
fwnode_property_get_reference_optional_args() call that static
function. IOW, similar to the DT API implementation using
__of_parse_phandle_with_args().

Rob

Re: [PATCH 0/2] device property: Add fwnode_property_get_reference_optional_args

[Andy Shevchenko]

On Mon, Apr 07, 2025 at 06:37:12PM -0400, Sean Anderson wrote:
> This series adds a fwnode-equivalent to of_parse_phandle_with_optional_args.

Thanks, but no user. Not acceptable in this form. Also see individual comments.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/2] device property: Add fwnode_property_get_reference_optional_args

[Sean Anderson]

On 4/8/25 04:40, Andy Shevchenko wrote:
> On Mon, Apr 07, 2025 at 06:37:12PM -0400, Sean Anderson wrote:
>> This series adds a fwnode-equivalent to of_parse_phandle_with_optional_args.
> 
> Thanks, but no user. Not acceptable in this form. Also see individual comments.
> 

https://lore.kernel.org/netdev/20250407231746.2316518-1-sean.anderson@linux.dev/

I was hoping to get these patches reviewed/applied separately, since netdev has
a limit on series length and these patches push that series over.

--Sean