[RFC 0/5] Align and document return values of phandle and reference parsing for OF and ACPI

[RFC 0/5] Align and document return values of phandle and reference parsing for OF and ACPI

[Sakari Ailus]

Hi everyone,

I recently came across a difference in behaviour of OF phandle parsing and
ACPI reference parsing, both of which can soon be accessed using
fwnode_property_get_reference_args.

The main change in this proposal touches OF, and specifically the change
is about using -ENODATA to tell that the phandle reference list entry that
was accessed does not exist. -ENOENT was used previously, but the same
error code was also used to tell that a phandle was empty, making it
impossible for the caller to figure out which of the two was the case.

I'm sending the set as RFC. In my limited testing I have found no ill
effects.

These patches are on top of linux-next.

Comments on the approach and the changes themselves would be most welcome.

For what it's worth, fwnode_property_get_reference_args is in Rafael's
tree (as well as in linux-next) and hasn't reached mainline yet. It will
be used in the near future for parsing references to external devices in
the V4L2 framework.

These patches can be found here:

<URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=fwnode-parse>

Regards,

- Sakari

Sakari Ailus (5):
  clk: Increment assigned-clocks index for empty phandles
  of: Return -ENODATA on accessing out of bounds indices in phandle
    parsing
  of: Return total number of entries in of_count_phandle_with_args
  ACPI: align acpi_fwnode_get_reference_args with OF counterpart
  device property: Document fwnode_property_get_reference_args better

 drivers/acpi/property.c              |  3 +++
 drivers/base/power/domain.c          |  2 +-
 drivers/base/property.c              |  5 +++++
 drivers/clk/clk-conf.c               |  8 +++++---
 drivers/clk/clkdev.c                 |  2 +-
 drivers/gpio/gpiolib-of.c            |  2 +-
 drivers/gpio/gpiolib.c               |  2 +-
 drivers/gpu/host1x/mipi.c            |  2 +-
 drivers/hwspinlock/hwspinlock_core.c |  2 +-
 drivers/iio/inkern.c                 |  2 +-
 drivers/of/base.c                    | 37 ++++++++++++++++++++++++------------
 drivers/pwm/core.c                   |  2 +-
 drivers/regulator/gpio-regulator.c   |  2 +-
 drivers/reset/core.c                 |  3 ++-
 drivers/spi/spi.c                    |  2 +-
 15 files changed, 50 insertions(+), 26 deletions(-)

-- 
2.11.0

[RFC 2/5] of: Return -ENODATA on accessing out of bounds indices in phandle parsing

[Sakari Ailus]

On functions iterating over lists of phandles using
of_phandle_iterator_init() and of_phandle_iterator_next()
(of_count_phandle_with_args, of_parse_phandle_with_args() and
of_parse_phandle_with_fixed_args()), -ENOENT is used to be signal three
distinct conditions:

1) Iterating over an entry that contains an empty phandle. An empty
phandle may be situated in the middle of a phandle list but there may be
valid phandles after it.

2) Accessing an index in a property containing phandles (with possible
integer arguments) which does not exist in a property.

3) Parsing a property that does not exist.

Because of 1 and 2 above,, when parsing phandles and arguments one by one
without knowing the total number, it is not possible to distinguish
whether one has reached the end of the list or whether an individual entry
is empty.

What this patch does is that it changes the functions using
of_phandle_iterator_next() to return -ENODATA in the case of accessing an
index that is not present in an array, instead of -ENOENT. This way the
two conditions can be told apart.

This is in line with the behaviour of existing ACPI framework function
__acpi_node_get_property_reference().

-ENODATA will be also returned in case the property does not exist.
__acpi_node_get_property_reference() returns -EINVAL in that case but also
on parse error whereas for the caller a non-existent property is
essentially the same as having a property but it's got no entries, hence
-ENODATA makes sense.

-EINVAL is returned by both the OF and ACPI variants on parse error.

Document the different error codes for of_parse_phandle_with_args() and
of_parse_phandle_with_fixed_args().

As part of the change, distribute the handling of the new error code to
the callers that make use of -ENOENT to skip entries. In order to retain
the original functionality, return -ENOENT to the original caller in case
of -ENODATA.

Not call callers are changed, a majority if the callers are actually
driver probe functions or machine setup code where the precise error code
does not really matter.

For consistency, of_count_phandle_with_args() has been changed to reflect
this as well; consequently of_gpio_named_count() and of_gpio_count() are
affected, too.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/base/power/domain.c          |  2 +-
 drivers/clk/clk-conf.c               |  4 ++--
 drivers/clk/clkdev.c                 |  2 +-
 drivers/gpio/gpiolib-of.c            |  2 +-
 drivers/gpio/gpiolib.c               |  2 +-
 drivers/gpu/host1x/mipi.c            |  2 +-
 drivers/hwspinlock/hwspinlock_core.c |  2 +-
 drivers/iio/inkern.c                 |  2 +-
 drivers/of/base.c                    | 23 ++++++++++++++++++-----
 drivers/pwm/core.c                   |  2 +-
 drivers/regulator/gpio-regulator.c   |  2 +-
 drivers/reset/core.c                 |  3 ++-
 drivers/spi/spi.c                    |  2 +-
 13 files changed, 32 insertions(+), 18 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index e8ca5e2cf1e5..a346001678ff 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -2112,7 +2112,7 @@ int genpd_dev_pm_attach(struct device *dev)
 	ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
 					"#power-domain-cells", 0, &pd_args);
 	if (ret < 0) {
-		if (ret != -ENOENT)
+		if (ret != -ENOENT && ret != -ENODATA)
 			return ret;
 
 		/*
diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c
index 7b7113d86875..8535f6a613cb 100644
--- a/drivers/clk/clk-conf.c
+++ b/drivers/clk/clk-conf.c
@@ -31,7 +31,7 @@ static int __set_clk_parents(struct device_node *node, bool clk_supplier)
 					"#clock-cells",	index, &clkspec);
 		if (rc < 0) {
 			/* skip empty (null) phandles */
-			if (rc == -ENOENT)
+			if (rc == -ENOENT || rc == -ENODATA)
 				continue;
 			else
 				return rc;
@@ -91,7 +91,7 @@ static int __set_clk_rates(struct device_node *node, bool clk_supplier)
 					"#clock-cells",	index, &clkspec);
 			if (rc < 0) {
 				/* skip empty (null) phandles */
-				if (rc == -ENOENT) {
+				if (rc == -ENOENT || rc == -ENODATA) {
 					index++;
 					continue;
 				} else {
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 6b2f29df3f70..b6946241a193 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -41,7 +41,7 @@ static struct clk *__of_clk_get(struct device_node *np, int index,
 	rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index,
 					&clkspec);
 	if (rc)
-		return ERR_PTR(rc);
+		return ERR_PTR(rc == -ENODATA ? -ENOENT : rc);
 
 	clk = __of_clk_get_from_provider(&clkspec, dev_id, con_id);
 	of_node_put(clkspec.np);
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index bfcd20699ec8..928f9ac732cc 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -80,7 +80,7 @@ struct gpio_desc *of_get_named_gpiod_flags(struct device_node *np,
 	if (ret) {
 		pr_debug("%s: can't parse '%s' property of node '%pOF[%d]'\n",
 			__func__, propname, np, index);
-		return ERR_PTR(ret);
+		return ERR_PTR(ret == -ENODATA ? -ENOENT : ret);
 	}
 
 	chip = of_find_gpiochip_by_xlate(&gpiospec);
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index eb80dac4e26a..5ef114a77493 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -3163,7 +3163,7 @@ static int dt_gpio_count(struct device *dev, const char *con_id)
 		if (ret > 0)
 			break;
 	}
-	return ret ? ret : -ENOENT;
+	return ret ? ret : -ENODATA;
 }
 
 static int platform_gpio_count(struct device *dev, const char *con_id)
diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
index e00809d996a2..ed08ff54e3ef 100644
--- a/drivers/gpu/host1x/mipi.c
+++ b/drivers/gpu/host1x/mipi.c
@@ -217,7 +217,7 @@ struct tegra_mipi_device *tegra_mipi_request(struct device *device)
 					 "#nvidia,mipi-calibrate-cells", 0,
 					 &args);
 	if (err < 0)
-		return ERR_PTR(err);
+		return ERR_PTR(err == -ENODATA ? -ENOENT : err);
 
 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
 	if (!dev) {
diff --git a/drivers/hwspinlock/hwspinlock_core.c b/drivers/hwspinlock/hwspinlock_core.c
index 4074441444fe..efac9beb72c6 100644
--- a/drivers/hwspinlock/hwspinlock_core.c
+++ b/drivers/hwspinlock/hwspinlock_core.c
@@ -304,7 +304,7 @@ int of_hwspin_lock_get_id(struct device_node *np, int index)
 	ret = of_parse_phandle_with_args(np, "hwlocks", "#hwlock-cells", index,
 					 &args);
 	if (ret)
-		return ret;
+		return ret == -ENODATA ? -ENOENT : ret;
 
 	/* Find the hwspinlock device: we need its base_id */
 	ret = -EPROBE_DEFER;
diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index 069defcc6d9b..fb850b040228 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -135,7 +135,7 @@ static int __of_iio_channel_get(struct iio_channel *channel,
 					 "#io-channel-cells",
 					 index, &iiospec);
 	if (err)
-		return err;
+		return err == -ENODATA ? -ENOENT : err;
 
 	idev = bus_find_device(&iio_bus_type, NULL, iiospec.np,
 			       iio_dev_node_match);
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 260d33c0f26c..fb2fd90f525c 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1142,7 +1142,7 @@ int of_phandle_iterator_init(struct of_phandle_iterator *it,
 
 	list = of_get_property(np, list_name, &size);
 	if (!list)
-		return -ENOENT;
+		return -ENODATA;
 
 	it->cells_name = cells_name;
 	it->cell_count = cell_count;
@@ -1165,7 +1165,7 @@ int of_phandle_iterator_next(struct of_phandle_iterator *it)
 	}
 
 	if (!it->cur || it->phandle_end >= it->list_end)
-		return -ENOENT;
+		return -ENODATA;
 
 	it->cur = it->phandle_end;
 
@@ -1351,6 +1351,10 @@ EXPORT_SYMBOL(of_parse_phandle);
  *
  * To get a device_node of the `node2' node you may call this:
  * of_parse_phandle_with_args(node3, "list", "#list-cells", 1, &args);
+ *
+ * Returns %0 on success, %-ENOENT on an empty phandle, %-ENODATA on
+ * out-of-bounds index or if the property does not exist and %-EINVAL on
+ * parse error or if index is negative.
  */
 int of_parse_phandle_with_args(const struct device_node *np, const char *list_name,
 				const char *cells_name, int index,
@@ -1392,6 +1396,10 @@ EXPORT_SYMBOL(of_parse_phandle_with_args);
  *
  * To get a device_node of the `node2' node you may call this:
  * of_parse_phandle_with_fixed_args(node3, "list", 2, 1, &args);
+ *
+ * Returns %0 on success, %-ENOENT on an empty phandle, %-ENODATA on
+ * out-of-bounds index or if the property does not exist and %-EINVAL on
+ * parse error or if index is negative.
  */
 int of_parse_phandle_with_fixed_args(const struct device_node *np,
 				const char *list_name, int cell_count,
@@ -1418,6 +1426,10 @@ EXPORT_SYMBOL(of_parse_phandle_with_fixed_args);
  * phandle and 1 or more arguments. The number of arguments are
  * determined by the #gpio-cells property in the node pointed to by the
  * phandle.
+ *
+ * Returns the number of phandles + arguments tuples within a property
+ * on success, %-ENODATA if the property isn't found and %-EINVAL on
+ * parse error.
  */
 int of_count_phandle_with_args(const struct device_node *np, const char *list_name,
 				const char *cells_name)
@@ -1429,11 +1441,12 @@ int of_count_phandle_with_args(const struct device_node *np, const char *list_na
 	if (rc)
 		return rc;
 
-	while ((rc = of_phandle_iterator_next(&it)) == 0)
+	for (rc = of_phandle_iterator_next(&it); !rc;
+	     rc = of_phandle_iterator_next(&it))
 		cur_index += 1;
 
-	if (rc != -ENOENT)
-		return rc;
+	if (rc != -ENODATA && rc != -ENOENT)
+		return rc == -ENOENT ? -ENODATA : rc;
 
 	return cur_index;
 }
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 1581f6ab1b1f..f54f4f58994c 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -673,7 +673,7 @@ struct pwm_device *of_pwm_get(struct device_node *np, const char *con_id)
 					 &args);
 	if (err) {
 		pr_err("%s(): can't parse \"pwms\" property\n", __func__);
-		return ERR_PTR(err);
+		return ERR_PTR(err == -ENODATA ? -ENOENT : err);
 	}
 
 	pc = of_node_to_pwmchip(args.np);
diff --git a/drivers/regulator/gpio-regulator.c b/drivers/regulator/gpio-regulator.c
index 0fce06acfaec..8adcd25ec843 100644
--- a/drivers/regulator/gpio-regulator.c
+++ b/drivers/regulator/gpio-regulator.c
@@ -167,7 +167,7 @@ of_get_gpio_regulator_config(struct device *dev, struct device_node *np,
 
 	/* Fetch GPIOs. - optional property*/
 	ret = of_gpio_count(np);
-	if ((ret < 0) && (ret != -ENOENT))
+	if ((ret < 0) && (ret != -ENODATA))
 		return ERR_PTR(ret);
 
 	if (ret > 0) {
diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index 1d21c6f7d56c..81aab379042f 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -456,7 +456,8 @@ struct reset_control *__of_reset_control_get(struct device_node *node,
 	if (ret == -EINVAL)
 		return ERR_PTR(ret);
 	if (ret)
-		return optional ? NULL : ERR_PTR(ret);
+		return optional ? NULL :
+				  ERR_PTR(ret == -ENODATA ? -ENOENT : ret);
 
 	mutex_lock(&reset_list_mutex);
 	rcdev = NULL;
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 6e65524cbfd9..6d35154d84ff 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2031,7 +2031,7 @@ static int of_spi_register_master(struct spi_controller *ctlr)
 	ctlr->num_chipselect = max_t(int, nb, ctlr->num_chipselect);
 
 	/* Return error only for an incorrectly formed cs-gpios property */
-	if (nb == 0 || nb == -ENOENT)
+	if (nb == 0 || nb == -ENODATA)
 		return 0;
 	else if (nb < 0)
 		return nb;
-- 
2.11.0

[RFC 3/5] of: Return total number of entries in of_count_phandle_with_args

[Sakari Ailus]

of_count_phandle_with_args used to count the phandles until the end of the
list or when the first empty phandle was encountered. There may be valid
entries after the empty phandle as well, include all entries to the count.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/of/base.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index fb2fd90f525c..4c8b93cfd06c 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1418,14 +1418,14 @@ EXPORT_SYMBOL(of_parse_phandle_with_fixed_args);
  * @list_name:	property name that contains a list
  * @cells_name:	property name that specifies phandles' arguments count
  *
- * Returns the number of phandle + argument tuples within a property. It
- * is a typical pattern to encode a list of phandle and variable
- * arguments into a single property. The number of arguments is encoded
- * by a property in the phandle-target node. For example, a gpios
- * property would contain a list of GPIO specifies consisting of a
- * phandle and 1 or more arguments. The number of arguments are
- * determined by the #gpio-cells property in the node pointed to by the
- * phandle.
+ * Returns the number of phandle + argument tuples within a property
+ * including empty phandles. It is a typical pattern to encode a list
+ * of phandle and variable arguments into a single property. The
+ * number of arguments is encoded by a property in the phandle-target
+ * node. For example, a gpios property would contain a list of GPIO
+ * specifies consisting of a phandle and 1 or more arguments. The
+ * number of arguments are determined by the #gpio-cells property in
+ * the node pointed to by the phandle.
  *
  * Returns the number of phandles + arguments tuples within a property
  * on success, %-ENODATA if the property isn't found and %-EINVAL on
@@ -1441,12 +1441,12 @@ int of_count_phandle_with_args(const struct device_node *np, const char *list_na
 	if (rc)
 		return rc;
 
-	for (rc = of_phandle_iterator_next(&it); !rc;
+	for (rc = of_phandle_iterator_next(&it); !rc || rc == -ENOENT;
 	     rc = of_phandle_iterator_next(&it))
 		cur_index += 1;
 
-	if (rc != -ENODATA && rc != -ENOENT)
-		return rc == -ENOENT ? -ENODATA : rc;
+	if (rc != -ENODATA)
+		return rc;
 
 	return cur_index;
 }
-- 
2.11.0

[RFC 1/5] clk: Increment assigned-clocks index for empty phandles

[Sakari Ailus]

If the assigned-clocks property contains an empty phandle, skip the index
rather than trying it over on the next iteration (with an related clock
rate related to another clock).

Signed-off-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/clk/clk-conf.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c
index 49819b546134..7b7113d86875 100644
--- a/drivers/clk/clk-conf.c
+++ b/drivers/clk/clk-conf.c
@@ -91,10 +91,12 @@ static int __set_clk_rates(struct device_node *node, bool clk_supplier)
 					"#clock-cells",	index, &clkspec);
 			if (rc < 0) {
 				/* skip empty (null) phandles */
-				if (rc == -ENOENT)
+				if (rc == -ENOENT) {
+					index++;
 					continue;
-				else
+				} else {
 					return rc;
+				}
 			}
 			if (clkspec.np == node && !clk_supplier)
 				return 0;
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC 4/5] ACPI: align acpi_fwnode_get_reference_args with OF counterpart

[Sakari Ailus]

acpi_fwnode_get_reference_args currently returns -EINVAL if a property
isn't found. -EINVAL is returned on parse error as well, and a
non-existent property is, for a list of references with arguments, usually
effectively the same as a property without entries.

Thus use -ENODATA to signal this as well. -EINVAL is still used to tell
about parse errors.

Signed-off-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/acpi/property.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index c1c216163de3..c6c777341023 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -1211,6 +1211,9 @@ acpi_fwnode_get_reference_args(const struct fwnode_handle *fwnode,
 	unsigned int i;
 	int ret;
 
+	if (acpi_node_prop_get(fwnode, prop, NULL))
+		return -ENODATA;
+
 	ret = __acpi_node_get_property_reference(fwnode, prop, index,
 						 args_count, &acpi_args);
 	if (ret < 0)
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC 5/5] device property: Document fwnode_property_get_reference_args better

[Sakari Ailus]

Define and document return values for fwnode_property_get_reference_args
which now is aligned with OF and ACPI.

Signed-off-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/base/property.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index d0b65bbe7e15..2f88b0594770 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 accessing an entry without a reference
+ *	    %-EINVAL on parse error and
+ *	    %-ENODATA when accessing an entry (at @index) that does not exist or
+ *	    the property @prop cannot be found.
  */
 int fwnode_property_get_reference_args(const struct fwnode_handle *fwnode,
 				       const char *prop, const char *nargs_prop,
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 0/5] Align and document return values of phandle and reference parsing for OF and ACPI

[Rob Herring]

On Thu, Sep 14, 2017 at 7:29 AM, Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
> Hi everyone,
>
> I recently came across a difference in behaviour of OF phandle parsing and
> ACPI reference parsing, both of which can soon be accessed using
> fwnode_property_get_reference_args.
>
> The main change in this proposal touches OF, and specifically the change
> is about using -ENODATA to tell that the phandle reference list entry that
> was accessed does not exist. -ENOENT was used previously, but the same
> error code was also used to tell that a phandle was empty, making it
> impossible for the caller to figure out which of the two was the case.
>
> I'm sending the set as RFC. In my limited testing I have found no ill
> effects.
>
> These patches are on top of linux-next.
>
> Comments on the approach and the changes themselves would be most welcome.

It's not really valid to both have a variable count (and hence need to
retrieve it) and use blank phandle entries. The whole point of blank
entries is to have a fixed length and know what each index corresponds
too. Do you have an example where we hit this?

Rob

Re: [RFC 0/5] Align and document return values of phandle and reference parsing for OF and ACPI

[Sakari Ailus]

Hi Rob,

Thanks for the review.

On Fri, Sep 15, 2017 at 02:57:35PM -0500, Rob Herring wrote:
> On Thu, Sep 14, 2017 at 7:29 AM, Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> > Hi everyone,
> >
> > I recently came across a difference in behaviour of OF phandle parsing and
> > ACPI reference parsing, both of which can soon be accessed using
> > fwnode_property_get_reference_args.
> >
> > The main change in this proposal touches OF, and specifically the change
> > is about using -ENODATA to tell that the phandle reference list entry that
> > was accessed does not exist. -ENOENT was used previously, but the same
> > error code was also used to tell that a phandle was empty, making it
> > impossible for the caller to figure out which of the two was the case.
> >
> > I'm sending the set as RFC. In my limited testing I have found no ill
> > effects.
> >
> > These patches are on top of linux-next.
> >
> > Comments on the approach and the changes themselves would be most welcome.
> 
> It's not really valid to both have a variable count (and hence need to
> retrieve it) and use blank phandle entries. The whole point of blank
> entries is to have a fixed length and know what each index corresponds
> too. Do you have an example where we hit this?

Well, the above is true. I think I'd still use different error codes to
tell about different situations.

The alternative is that the fwnode counterpart error codes are changed to
to -ENOENT.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [RFC 0/5] Align and document return values of phandle and reference parsing for OF and ACPI

[Sakari Ailus]

On Wed, Sep 20, 2017 at 02:01:25AM +0300, Sakari Ailus wrote:
> Hi Rob,
> 
> Thanks for the review.
> 
> On Fri, Sep 15, 2017 at 02:57:35PM -0500, Rob Herring wrote:
> > On Thu, Sep 14, 2017 at 7:29 AM, Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > > Hi everyone,
> > >
> > > I recently came across a difference in behaviour of OF phandle parsing and
> > > ACPI reference parsing, both of which can soon be accessed using
> > > fwnode_property_get_reference_args.
> > >
> > > The main change in this proposal touches OF, and specifically the change
> > > is about using -ENODATA to tell that the phandle reference list entry that
> > > was accessed does not exist. -ENOENT was used previously, but the same
> > > error code was also used to tell that a phandle was empty, making it
> > > impossible for the caller to figure out which of the two was the case.
> > >
> > > I'm sending the set as RFC. In my limited testing I have found no ill
> > > effects.
> > >
> > > These patches are on top of linux-next.
> > >
> > > Comments on the approach and the changes themselves would be most welcome.
> > 
> > It's not really valid to both have a variable count (and hence need to
> > retrieve it) and use blank phandle entries. The whole point of blank
> > entries is to have a fixed length and know what each index corresponds
> > too. Do you have an example where we hit this?
> 
> Well, the above is true. I think I'd still use different error codes to
> tell about different situations.
> 
> The alternative is that the fwnode counterpart error codes are changed to
> to -ENOENT.

I'll post a patch implemeting switching the fwnode variant to use -ENOENT,
cc'ing you. I think the first patch of the set is still relevant, it's
rather a bugfix.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi