* Re: [PATCH v2 1/1] device property: Align return codes of acpi_fwnode_get_reference_with_args
2017-09-19 21:16 ` [PATCH v2 1/1] device property: Align return codes of acpi_fwnode_get_reference_with_args Rafael J. Wysocki
@ 2017-09-19 21:38 ` Sakari Ailus
0 siblings, 0 replies; 2+ messages in thread
From: Sakari Ailus @ 2017-09-19 21:38 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: linux-pm, mika.westerberg, Linux ACPI
Hi Rafael,
Rafael J. Wysocki wrote:
> On Tuesday, September 12, 2017 4:15:12 PM CEST Sakari Ailus wrote:
>> acpi_fwnode_get_reference_with_args(), the function implementing ACPI
>> support for fwnode_property_get_reference_with_args(), returns directly
>> error codes from __acpi_node_get_property_reference(). The latter uses
>> different error codes than the OF implementation. In particular, the OF
>> implementation uses -ENOENT to indicate there are no more entries whereas
>> the former uses -ENODATA for the purpose.
>>
>> To make matters more complicated, the ACPI implementation uses -ENOENT but
>> for a different purpose --- when an entry exists but contains no
>> reference. This isn't recognised by OF.
>>
>> Document and align the error codes for property for
>> fwnode_property_get_reference_with_args() so that they match with
>> of_parse_phandle_with_args(), with the difference that -ENODATA is
>> returned whenever an entry exists but contains no reference.
>>
>> Also return -ENOENT if the property does not exist, which is what the DT
>> equivalent does.
>>
>> Fixes: 3e3119d3088f ("device property: Introduce fwnode_property_get_reference_args")
>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>> ---
>> since v1:
>>
>> - Return -ENOENT if the property is not found.
>>
>> drivers/acpi/property.c | 7 +++++++
>> drivers/base/property.c | 5 +++++
>> 2 files changed, 12 insertions(+)
>>
>> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
>> index a65c09cc223f..8970dd288d9d 100644
>> --- a/drivers/acpi/property.c
>> +++ b/drivers/acpi/property.c
>> @@ -1205,8 +1205,15 @@ acpi_fwnode_get_reference_args(const struct fwnode_handle *fwnode,
>> unsigned int i;
>> int ret;
>>
>> + if (acpi_node_prop_get(fwnode, prop, NULL))
>> + return -ENOENT;
>> +
>> ret = __acpi_node_get_property_reference(fwnode, prop, index,
>> args_count, &acpi_args);
>> + if (ret == -ENODATA)
>> + return -ENOENT;
>> + if (ret == -ENOENT)
>> + return -ENODATA;
>
> I agree with Mika that it would be good to have a comment describing what is
> going on here.
>
> Or maybe better, why don't you change __acpi_node_get_property_reference()
> itself?
I had a chat with Mika after sending the patch and ended up proposing
changing the OF behaviour slightly.
The subject is "[RFC 0/5] Align and document return values of phandle
and reference parsing for OF and ACPI". Rob commented on that already.
One option could be that the fwnode variant is simply made to mirror the
OF behaviour. The fwnode API will need to be amended with function to
tell the number of references available.
>
>> if (ret < 0)
>> return ret;
>> if (!args)
>> diff --git a/drivers/base/property.c b/drivers/base/property.c
>> index d0b65bbe7e15..79b07ed83304 100644
>> --- a/drivers/base/property.c
>> +++ b/drivers/base/property.c
>> @@ -682,6 +682,11 @@ EXPORT_SYMBOL_GPL(fwnode_property_match_string);
>> * Caller is responsible to call fwnode_handle_put() on the returned
>> * args->fwnode pointer.
>> *
>> + * Returns: %0 on success
>> + * %-ENOENT when the index is out of bounds, or the property was not
>> + * found
>> + * %-EINVAL on parse error
>> + * %-ENODATA when an entry exists but contains no reference
>> */
>> int fwnode_property_get_reference_args(const struct fwnode_handle *fwnode,
>> const char *prop, const char *nargs_prop,
>>
>
> Thanks,
> Rafael
>
--
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 2+ messages in thread