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

[Sean Anderson <sean.anderson@linux.dev>]

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