[PATCH v2 0/8] led: update LED boot/activity to new property implementation

[PATCH v2 0/8] led: update LED boot/activity to new property implementation

[Christian Marangi]

This series is split in 2 part.

While adapting the LED boot and activity code to the new property
accepted by Rob in dt-schema repository, a big BUG was discovered.

The reason wasn't clear at start and took me some days to figure it
out.

This was triggered by adding a new phandle in the test.dts to
introduce test for the new OPs.

This single addition caused the sandbox CI test to fail in the
dm_test_ofnode_phandle_ot test.

This doesn't make sense as reverting the change made the CI test
to correctly finish. Also moving the uboot node down
after the first phandle (in test.dts the gpio one) also made
the CI test to correctly finish.

A little bit of searching and debugging made me realize the
parse phandle OPs didn't support other.dts at all and they
were still referencing phandle index from test.dts.
(more info in the related commit)

In short the test was broken all along and was working by
pure luck. The first 4 patch address and fix the problem for good.

The other 4 patch expand and address the property change for
LED boot/activity.

Posting in a single series as changes are trivial and just
to speedup review process. (and also because the second
part depends on the first)

All CI tested with azure pipeline.

Changes v2:
- Fix handling of flat tree for phandle
- Fix test and other.dts changes

Christian Marangi (8):
  dm: core: implement oftree variant of parse_phandle OPs
  test: dm: fix broken dm_test_ofnode_phandle_ot and get_by_phandle_ot
  dm: core: implement ofnode/tree_parse_phandle() helper
  test: dm: Expand dm_test_ofnode_phandle(_ot) with new
    ofnode/tree_parse_phandle
  dm: core: implement phandle ofnode_options helper
  test: dm: Add test for ofnode options phandle helper
  led: update LED boot/activity to new property implementation
  test: dm: Update test for LED activity and boot

 arch/sandbox/dts/other.dts |  31 ++++++++-
 arch/sandbox/dts/test.dts  |  16 +++--
 drivers/core/of_access.c   |  61 ++++++++++++-----
 drivers/core/ofnode.c      | 124 ++++++++++++++++++++++++++++++++-
 drivers/led/led-uclass.c   |  30 +++++---
 include/dm/of_access.h     |  86 +++++++++++++++++++++++
 include/dm/ofnode.h        | 107 +++++++++++++++++++++++++++++
 test/dm/led.c              |  18 +++--
 test/dm/ofnode.c           | 136 ++++++++++++++++++++++++++++++++-----
 9 files changed, 551 insertions(+), 58 deletions(-)

-- 
2.45.2

[PATCH v2 1/8] dm: core: implement oftree variant of parse_phandle OPs

[Christian Marangi]

Implement oftree variant of parse_phandle OPs.

There is currently a very hidden and laten BUG with parse_phandle OPs
that doesn't permit the support of multiple DTS in a system. One usage
example if sandbox with the usage of other.dts

The BUG is only present on live scenario where of_... OPs are used and
it's not present when fdt... OPs are used.

This is caused by an assumption made in __of_parse_phandle_with_args,
with the of_find_node_by_phandle call that pass the first arg as NULL.

This makes of_find_node_by_phandle use the default root node of the
system and doesn't permit the usage of alternative tree. This is correct
for normal system and also for the linux kernel where it's assumed a
single device tree.

It's problematic if other device tree needs to be used.

To fix this, introduce __of_root_parse_phandle_with_args to define a
root device tree for of_find_node_by_phandle.

Introduce all the variant OPs for this and in ofnode, the oftree OPs
following how it's done for other OPs with similar task.

For FDT scenario, ofnode_from_fdtdec_phandle_args is reworked to accept
a new variable, node and noffset_to_ofnode is used instead of
offset_to_ofnode. This is required to support multiple FDB blob to
calculate the correct of_offset of the ofnode.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/core/of_access.c | 61 ++++++++++++++++++++--------
 drivers/core/ofnode.c    | 51 ++++++++++++++++++++++--
 include/dm/of_access.h   | 86 ++++++++++++++++++++++++++++++++++++++++
 include/dm/ofnode.h      | 66 ++++++++++++++++++++++++++++++
 4 files changed, 244 insertions(+), 20 deletions(-)

diff --git a/drivers/core/of_access.c b/drivers/core/of_access.c
index 77acd766262..b11e36202c1 100644
--- a/drivers/core/of_access.c
+++ b/drivers/core/of_access.c
@@ -666,11 +666,12 @@ int of_property_read_string_helper(const struct device_node *np,
 	return i <= 0 ? -ENODATA : i;
 }
 
-static int __of_parse_phandle_with_args(const struct device_node *np,
-					const char *list_name,
-					const char *cells_name,
-					int cell_count, int index,
-					struct of_phandle_args *out_args)
+static int __of_root_parse_phandle_with_args(struct device_node *root,
+					     const struct device_node *np,
+					     const char *list_name,
+					     const char *cells_name,
+					     int cell_count, int index,
+					     struct of_phandle_args *out_args)
 {
 	const __be32 *list, *list_end;
 	int rc = 0, cur_index = 0;
@@ -706,7 +707,7 @@ static int __of_parse_phandle_with_args(const struct device_node *np,
 			 * below.
 			 */
 			if (cells_name || cur_index == index) {
-				node = of_find_node_by_phandle(NULL, phandle);
+				node = of_find_node_by_phandle(root, phandle);
 				if (!node) {
 					dm_warn("%s: could not find phandle\n",
 						np->full_name);
@@ -783,39 +784,65 @@ static int __of_parse_phandle_with_args(const struct device_node *np,
 	return rc;
 }
 
-struct device_node *of_parse_phandle(const struct device_node *np,
-				     const char *phandle_name, int index)
+struct device_node *of_root_parse_phandle(struct device_node *root,
+					  const struct device_node *np,
+					  const char *phandle_name, int index)
 {
 	struct of_phandle_args args;
 
 	if (index < 0)
 		return NULL;
 
-	if (__of_parse_phandle_with_args(np, phandle_name, NULL, 0, index,
-					 &args))
+	if (__of_root_parse_phandle_with_args(root, np, phandle_name, NULL, 0,
+					      index, &args))
 		return NULL;
 
 	return args.np;
 }
 
+int of_root_parse_phandle_with_args(struct device_node *root,
+				    const struct device_node *np,
+				    const char *list_name, const char *cells_name,
+				    int cell_count, int index,
+				    struct of_phandle_args *out_args)
+{
+	if (index < 0)
+		return -EINVAL;
+
+	return __of_root_parse_phandle_with_args(root, np, list_name, cells_name,
+						 cell_count, index, out_args);
+}
+
+int of_root_count_phandle_with_args(struct device_node *root,
+				    const struct device_node *np,
+				    const char *list_name, const char *cells_name,
+				    int cell_count)
+{
+	return __of_root_parse_phandle_with_args(root, np, list_name, cells_name,
+						 cell_count, -1, NULL);
+}
+
+struct device_node *of_parse_phandle(const struct device_node *np,
+				     const char *phandle_name, int index)
+{
+	return of_root_parse_phandle(NULL, np, phandle_name, index);
+}
+
 int of_parse_phandle_with_args(const struct device_node *np,
 			       const char *list_name, const char *cells_name,
 			       int cell_count, int index,
 			       struct of_phandle_args *out_args)
 {
-	if (index < 0)
-		return -EINVAL;
-
-	return __of_parse_phandle_with_args(np, list_name, cells_name,
-					    cell_count, index, out_args);
+	return of_root_parse_phandle_with_args(NULL, np, list_name, cells_name,
+					       cell_count, index, out_args);
 }
 
 int of_count_phandle_with_args(const struct device_node *np,
 			       const char *list_name, const char *cells_name,
 			       int cell_count)
 {
-	return __of_parse_phandle_with_args(np, list_name, cells_name,
-					    cell_count, -1, NULL);
+	return of_root_count_phandle_with_args(NULL, np, list_name, cells_name,
+					       cell_count);
 }
 
 static void of_alias_add(struct alias_prop *ap, struct device_node *np,
diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
index 950895e72a9..b9cc9419407 100644
--- a/drivers/core/ofnode.c
+++ b/drivers/core/ofnode.c
@@ -879,11 +879,11 @@ int ofnode_read_string_list(ofnode node, const char *property,
 	return count;
 }
 
-static void ofnode_from_fdtdec_phandle_args(struct fdtdec_phandle_args *in,
+static void ofnode_from_fdtdec_phandle_args(ofnode node, struct fdtdec_phandle_args *in,
 					    struct ofnode_phandle_args *out)
 {
 	assert(OF_MAX_PHANDLE_ARGS == MAX_PHANDLE_ARGS);
-	out->node = offset_to_ofnode(in->node);
+	out->node = noffset_to_ofnode(node, in->node);
 	out->args_count = in->args_count;
 	memcpy(out->args, in->args, sizeof(out->args));
 }
@@ -923,7 +923,40 @@ int ofnode_parse_phandle_with_args(ofnode node, const char *list_name,
 						     cell_count, index, &args);
 		if (ret)
 			return ret;
-		ofnode_from_fdtdec_phandle_args(&args, out_args);
+		ofnode_from_fdtdec_phandle_args(node, &args, out_args);
+	}
+
+	return 0;
+}
+
+int oftree_parse_phandle_with_args(oftree tree, ofnode node, const char *list_name,
+				   const char *cells_name, int cell_count,
+				   int index,
+				   struct ofnode_phandle_args *out_args)
+{
+	if (ofnode_is_np(node)) {
+		struct of_phandle_args args;
+		int ret;
+
+		ret = of_root_parse_phandle_with_args(tree.np,
+						      ofnode_to_np(node),
+						      list_name, cells_name,
+						      cell_count, index,
+						      &args);
+		if (ret)
+			return ret;
+		ofnode_from_of_phandle_args(&args, out_args);
+	} else {
+		struct fdtdec_phandle_args args;
+		int ret;
+
+		ret = fdtdec_parse_phandle_with_args(tree.fdt,
+						     ofnode_to_offset(node),
+						     list_name, cells_name,
+						     cell_count, index, &args);
+		if (ret)
+			return ret;
+		ofnode_from_fdtdec_phandle_args(node, &args, out_args);
 	}
 
 	return 0;
@@ -941,6 +974,18 @@ int ofnode_count_phandle_with_args(ofnode node, const char *list_name,
 				cell_count, -1, NULL);
 }
 
+int oftree_count_phandle_with_args(oftree tree, ofnode node, const char *list_name,
+				   const char *cells_name, int cell_count)
+{
+	if (ofnode_is_np(node))
+		return of_root_count_phandle_with_args(tree.np, ofnode_to_np(node),
+				list_name, cells_name, cell_count);
+	else
+		return fdtdec_parse_phandle_with_args(tree.fdt,
+				ofnode_to_offset(node), list_name, cells_name,
+				cell_count, -1, NULL);
+}
+
 ofnode ofnode_path(const char *path)
 {
 	if (of_live_active())
diff --git a/include/dm/of_access.h b/include/dm/of_access.h
index de740d44674..44143a5a391 100644
--- a/include/dm/of_access.h
+++ b/include/dm/of_access.h
@@ -453,6 +453,92 @@ static inline int of_property_count_strings(const struct device_node *np,
 	return of_property_read_string_helper(np, propname, NULL, 0, 0);
 }
 
+/**
+ * of_root_parse_phandle - Resolve a phandle property to a device_node pointer
+ *			   from a root node
+ * @root: Pointer to root device tree node (default root node if NULL)
+ * @np: Pointer to device node holding phandle property
+ * @phandle_name: Name of property holding a phandle value
+ * @index: For properties holding a table of phandles, this is the index into
+ *         the table
+ *
+ * Return:
+ *   the device_node pointer with refcount incremented.  Use
+ *   of_node_put() on it when done.
+ */
+struct device_node *of_root_parse_phandle(struct device_node *root,
+					  const struct device_node *np,
+					  const char *phandle_name, int index);
+
+/**
+ * of_root_parse_phandle_with_args() - Find a node pointed by phandle in a list
+ *				       from a root node
+ *
+ * @root:	pointer to root device tree node (default root node if NULL)
+ * @np:		pointer to a device tree node containing a list
+ * @list_name:	property name that contains a list
+ * @cells_name:	property name that specifies phandles' arguments count
+ * @cells_count: Cell count to use if @cells_name is NULL
+ * @index:	index of a phandle to parse out
+ * @out_args:	optional pointer to output arguments structure (will be filled)
+ * Return:
+ *   0 on success (with @out_args filled out if not NULL), -ENOENT if
+ *   @list_name does not exist, -EINVAL if a phandle was not found,
+ *   @cells_name could not be found, the arguments were truncated or there
+ *   were too many arguments.
+ *
+ * This function is useful to parse lists of phandles and their arguments.
+ * Returns 0 on success and fills out_args, on error returns appropriate
+ * errno value.
+ *
+ * Caller is responsible to call of_node_put() on the returned out_args->np
+ * pointer.
+ *
+ * Example:
+ *
+ * .. code-block::
+ *
+ *   phandle1: node1 {
+ *       #list-cells = <2>;
+ *   };
+ *   phandle2: node2 {
+ *       #list-cells = <1>;
+ *   };
+ *   node3 {
+ *       list = <&phandle1 1 2 &phandle2 3>;
+ *   };
+ *
+ * To get a device_node of the `node2' node you may call this:
+ * of_root_parse_phandle_with_args(node3, "list", "#list-cells", 1, &args);
+ */
+int of_root_parse_phandle_with_args(struct device_node *root,
+				    const struct device_node *np,
+				    const char *list_name, const char *cells_name,
+				    int cells_count, int index,
+				    struct of_phandle_args *out_args);
+
+/**
+ * of_root_count_phandle_with_args() - Count the number of phandle in a list
+ *				       from a root node
+ *
+ * @root:	pointer to root device tree node (default root node if NULL)
+ * @np:		pointer to a device tree node containing a list
+ * @list_name:	property name that contains a list
+ * @cells_name:	property name that specifies phandles' arguments count
+ * @cells_count: Cell count to use if @cells_name is NULL
+ * Return:
+ *   number of phandle found, -ENOENT if @list_name does not exist,
+ *   -EINVAL if a phandle was not found, @cells_name could not be found,
+ *   the arguments were truncated or there were too many arguments.
+ *
+ * Returns number of phandle found on success, on error returns appropriate
+ * errno value.
+ */
+int of_root_count_phandle_with_args(struct device_node *root,
+				    const struct device_node *np,
+				    const char *list_name, const char *cells_name,
+				    int cells_count);
+
 /**
  * of_parse_phandle - Resolve a phandle property to a device_node pointer
  * @np: Pointer to device node holding phandle property
diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h
index 0787758926f..7ece68874ae 100644
--- a/include/dm/ofnode.h
+++ b/include/dm/ofnode.h
@@ -909,6 +909,72 @@ int ofnode_parse_phandle_with_args(ofnode node, const char *list_name,
 int ofnode_count_phandle_with_args(ofnode node, const char *list_name,
 				   const char *cells_name, int cell_count);
 
+/**
+ * oftree_parse_phandle_with_args() - Find a node pointed by phandle in a list
+ *				      from a root node
+ *
+ * This function is useful to parse lists of phandles and their arguments.
+ * Returns 0 on success and fills out_args, on error returns appropriate
+ * errno value.
+ *
+ * Caller is responsible to call of_node_put() on the returned out_args->np
+ * pointer.
+ *
+ * Example:
+ *
+ * .. code-block::
+ *
+ *   phandle1: node1 {
+ *       #list-cells = <2>;
+ *   };
+ *   phandle2: node2 {
+ *       #list-cells = <1>;
+ *   };
+ *   node3 {
+ *       list = <&phandle1 1 2 &phandle2 3>;
+ *   };
+ *
+ * To get a device_node of the `node2' node you may call this:
+ * oftree_parse_phandle_with_args(node3, "list", "#list-cells", 0, 1, &args);
+ *
+ * @tree:	device tree to use
+ * @node:	device tree node containing a list
+ * @list_name:	property name that contains a list
+ * @cells_name:	property name that specifies phandles' arguments count
+ * @cell_count: Cell count to use if @cells_name is NULL
+ * @index:	index of a phandle to parse out
+ * @out_args:	optional pointer to output arguments structure (will be filled)
+ * Return:
+ *   0 on success (with @out_args filled out if not NULL), -ENOENT if
+ *   @list_name does not exist, -EINVAL if a phandle was not found,
+ *   @cells_name could not be found, the arguments were truncated or there
+ *   were too many arguments.
+ */
+int oftree_parse_phandle_with_args(oftree tree, ofnode node, const char *list_name,
+				   const char *cells_name, int cell_count,
+				   int index,
+				   struct ofnode_phandle_args *out_args);
+
+/**
+ * oftree_count_phandle_with_args() - Count number of phandle in a list
+ *				      from a root node
+ *
+ * This function is useful to count phandles into a list.
+ * Returns number of phandle on success, on error returns appropriate
+ * errno value.
+ *
+ * @tree:	device tree to use
+ * @node:	device tree node containing a list
+ * @list_name:	property name that contains a list
+ * @cells_name:	property name that specifies phandles' arguments count
+ * @cell_count: Cell count to use if @cells_name is NULL
+ * Return:
+ *   number of phandle on success, -ENOENT if @list_name does not exist,
+ *   -EINVAL if a phandle was not found, @cells_name could not be found.
+ */
+int oftree_count_phandle_with_args(oftree tree, ofnode node, const char *list_name,
+				   const char *cells_name, int cell_count);
+
 /**
  * ofnode_path() - find a node by full path
  *
-- 
2.45.2

Re: [PATCH v2 1/8] dm: core: implement oftree variant of parse_phandle OPs

[Simon Glass]

On Sun, 10 Nov 2024 at 04:51, Christian Marangi <ansuelsmth@gmail.com> wrote:
>
> Implement oftree variant of parse_phandle OPs.
>
> There is currently a very hidden and laten BUG with parse_phandle OPs
> that doesn't permit the support of multiple DTS in a system. One usage
> example if sandbox with the usage of other.dts
>
> The BUG is only present on live scenario where of_... OPs are used and
> it's not present when fdt... OPs are used.
>
> This is caused by an assumption made in __of_parse_phandle_with_args,
> with the of_find_node_by_phandle call that pass the first arg as NULL.
>
> This makes of_find_node_by_phandle use the default root node of the
> system and doesn't permit the usage of alternative tree. This is correct
> for normal system and also for the linux kernel where it's assumed a
> single device tree.
>
> It's problematic if other device tree needs to be used.
>
> To fix this, introduce __of_root_parse_phandle_with_args to define a
> root device tree for of_find_node_by_phandle.
>
> Introduce all the variant OPs for this and in ofnode, the oftree OPs
> following how it's done for other OPs with similar task.
>
> For FDT scenario, ofnode_from_fdtdec_phandle_args is reworked to accept
> a new variable, node and noffset_to_ofnode is used instead of
> offset_to_ofnode. This is required to support multiple FDB blob to
> calculate the correct of_offset of the ofnode.
>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/core/of_access.c | 61 ++++++++++++++++++++--------
>  drivers/core/ofnode.c    | 51 ++++++++++++++++++++++--
>  include/dm/of_access.h   | 86 ++++++++++++++++++++++++++++++++++++++++
>  include/dm/ofnode.h      | 66 ++++++++++++++++++++++++++++++
>  4 files changed, 244 insertions(+), 20 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v2 2/8] test: dm: fix broken dm_test_ofnode_phandle_ot and get_by_phandle_ot

[Christian Marangi]

Fix broken dm_test_ofnode_phandle_ot test. They never actually worked
and were passing test by pure luck by having the same phandle index of
test.dts that coincicentally had #gpio-cells in the same index node.

It was sufficient to add a phandle to test.dts to make the test fail.

To correctly test these feature, make use oif the new OPs oftree to
parse phandle.

For consistency with the dm_test_ofnode_phandle, rework the test and
other.dts to use the same property with the other- prefix to every
node.

Also fix dm_test_ofnode_get_by_phandle_ot by making it more robust and
renaming the phandle property to other-phandle.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 arch/sandbox/dts/other.dts | 24 ++++++++++-
 test/dm/ofnode.c           | 83 +++++++++++++++++++++++++++++++-------
 2 files changed, 91 insertions(+), 16 deletions(-)

diff --git a/arch/sandbox/dts/other.dts b/arch/sandbox/dts/other.dts
index 395a7923228..b32158c6569 100644
--- a/arch/sandbox/dts/other.dts
+++ b/arch/sandbox/dts/other.dts
@@ -8,13 +8,15 @@
 
 /dts-v1/;
 
+#include <dt-bindings/gpio/gpio.h>
+
 / {
 	compatible = "sandbox-other";
 	#address-cells = <1>;
 	#size-cells = <1>;
 
 	node {
-		target = <&target 3 4>;
+		other-phandle = <&target>;
 
 		subnode {
 			compatible = "sandbox-other2";
@@ -25,9 +27,27 @@
 		};
 	};
 
+	other-a-test {
+		other-test-gpios = <&other_gpio_a 1>, <&other_gpio_a 4>,
+			<&other_gpio_b 5 GPIO_ACTIVE_HIGH 3 2 1>,
+			<0>, <&other_gpio_a 12>;
+		other-phandle-value = <&other_gpio_c 10>, <0xFFFFFFFF 20>, <&other_gpio_a 30>;
+	};
+
+	other_gpio_a: other-gpio-a {
+		#gpio-cells = <1>;
+	};
+
+	other_gpio_b: other-gpio-b {
+		#gpio-cells = <5>;
+	};
+
+	other_gpio_c: other-gpio-c {
+		#gpio-cells = <2>;
+	};
+
 	target: target {
 		compatible = "sandbox-other2";
-		#gpio-cells = <2>;
 		str-prop = "other";
 		reg = <0x8000 0x100>;
 		status = "disabled";
diff --git a/test/dm/ofnode.c b/test/dm/ofnode.c
index ce996567c3c..24b67bbe2b9 100644
--- a/test/dm/ofnode.c
+++ b/test/dm/ofnode.c
@@ -141,9 +141,16 @@ static int dm_test_ofnode_get_by_phandle_ot(struct unit_test_state *uts)
 {
 	oftree otree = get_other_oftree(uts);
 	ofnode node;
+	u32 idx;
+	int ret;
 
-	ut_assert(ofnode_valid(oftree_get_by_phandle(oftree_default(), 1)));
-	node = oftree_get_by_phandle(otree, 1);
+	node = oftree_path(otree, "/node");
+	ut_assert(ofnode_valid(node));
+
+	ret = ofnode_read_u32(node, "other-phandle", &idx);
+	ut_assertok(ret);
+
+	node = oftree_get_by_phandle(otree, idx);
 	ut_assert(ofnode_valid(node));
 	ut_asserteq_str("target", ofnode_get_name(node));
 
@@ -349,30 +356,78 @@ static int dm_test_ofnode_phandle(struct unit_test_state *uts)
 }
 DM_TEST(dm_test_ofnode_phandle, UTF_SCAN_PDATA | UTF_SCAN_FDT);
 
-/* test ofnode_count_/parse_phandle_with_args() with 'other' tree */
+/* test oftree_count_/parse_phandle_with_args() with 'other' tree */
 static int dm_test_ofnode_phandle_ot(struct unit_test_state *uts)
 {
 	oftree otree = get_other_oftree(uts);
 	struct ofnode_phandle_args args;
 	ofnode node;
 	int ret;
+	const char prop[] = "other-test-gpios";
+	const char cell[] = "#gpio-cells";
+	const char prop2[] = "other-phandle-value";
 
-	node = oftree_path(otree, "/node");
+	node = oftree_path(otree, "/other-a-test");
+	ut_assert(ofnode_valid(node));
 
-	/* Test ofnode_count_phandle_with_args with cell name */
-	ret = ofnode_count_phandle_with_args(node, "missing", "#gpio-cells", 0);
+	/* Test oftree_count_phandle_with_args with cell name */
+	ret = oftree_count_phandle_with_args(otree, node, "missing", cell, 0);
 	ut_asserteq(-ENOENT, ret);
-	ret = ofnode_count_phandle_with_args(node, "target", "#invalid", 0);
+	ret = oftree_count_phandle_with_args(otree, node, prop, "#invalid", 0);
 	ut_asserteq(-EINVAL, ret);
-	ret = ofnode_count_phandle_with_args(node, "target", "#gpio-cells", 0);
-	ut_asserteq(1, ret);
+	ret = oftree_count_phandle_with_args(otree, node, prop, cell, 0);
+	ut_asserteq(5, ret);
+
+	/* Test oftree_parse_phandle_with_args with cell name */
+	ret = oftree_parse_phandle_with_args(otree, node, "missing", cell, 0, 0,
+					     &args);
+	ut_asserteq(-ENOENT, ret);
+	ret = oftree_parse_phandle_with_args(otree, node, prop, "#invalid", 0, 0,
+					     &args);
+	ut_asserteq(-EINVAL, ret);
+	ret = oftree_parse_phandle_with_args(otree, node, prop, cell, 0, 0, &args);
+	ut_assertok(ret);
+	ut_asserteq(1, args.args_count);
+	ut_asserteq(1, args.args[0]);
+	ret = oftree_parse_phandle_with_args(otree, node, prop, cell, 0, 1, &args);
+	ut_assertok(ret);
+	ut_asserteq(1, args.args_count);
+	ut_asserteq(4, args.args[0]);
+	ret = oftree_parse_phandle_with_args(otree, node, prop, cell, 0, 2, &args);
+	ut_assertok(ret);
+	ut_asserteq(5, args.args_count);
+	ut_asserteq(5, args.args[0]);
+	ut_asserteq(1, args.args[4]);
+	ret = oftree_parse_phandle_with_args(otree, node, prop, cell, 0, 3, &args);
+	ut_asserteq(-ENOENT, ret);
+	ret = oftree_parse_phandle_with_args(otree, node, prop, cell, 0, 4, &args);
+	ut_assertok(ret);
+	ut_asserteq(1, args.args_count);
+	ut_asserteq(12, args.args[0]);
+	ret = oftree_parse_phandle_with_args(otree, node, prop, cell, 0, 5, &args);
+	ut_asserteq(-ENOENT, ret);
+
+	/* Test oftree_count_phandle_with_args with cell count */
+	ret = oftree_count_phandle_with_args(otree, node, "missing", NULL, 2);
+	ut_asserteq(-ENOENT, ret);
+	ret = oftree_count_phandle_with_args(otree, node, prop2, NULL, 1);
+	ut_asserteq(3, ret);
 
-	ret = ofnode_parse_phandle_with_args(node, "target", "#gpio-cells", 0,
-					     0, &args);
+	/* Test oftree_parse_phandle_with_args with cell count */
+	ret = oftree_parse_phandle_with_args(otree, node, prop2, NULL, 1, 0, &args);
 	ut_assertok(ret);
-	ut_asserteq(2, args.args_count);
-	ut_asserteq(3, args.args[0]);
-	ut_asserteq(4, args.args[1]);
+	ut_asserteq(1, ofnode_valid(args.node));
+	ut_asserteq(1, args.args_count);
+	ut_asserteq(10, args.args[0]);
+	ret = oftree_parse_phandle_with_args(otree, node, prop2, NULL, 1, 1, &args);
+	ut_asserteq(-EINVAL, ret);
+	ret = oftree_parse_phandle_with_args(otree, node, prop2, NULL, 1, 2, &args);
+	ut_assertok(ret);
+	ut_asserteq(1, ofnode_valid(args.node));
+	ut_asserteq(1, args.args_count);
+	ut_asserteq(30, args.args[0]);
+	ret = oftree_parse_phandle_with_args(otree, node, prop2, NULL, 1, 3, &args);
+	ut_asserteq(-ENOENT, ret);
 
 	return 0;
 }
-- 
2.45.2

Re: [PATCH v2 2/8] test: dm: fix broken dm_test_ofnode_phandle_ot and get_by_phandle_ot

[Simon Glass]

On Sun, 10 Nov 2024 at 04:51, Christian Marangi <ansuelsmth@gmail.com> wrote:
>
> Fix broken dm_test_ofnode_phandle_ot test. They never actually worked
> and were passing test by pure luck by having the same phandle index of
> test.dts that coincicentally had #gpio-cells in the same index node.
>
> It was sufficient to add a phandle to test.dts to make the test fail.
>
> To correctly test these feature, make use oif the new OPs oftree to
> parse phandle.
>
> For consistency with the dm_test_ofnode_phandle, rework the test and
> other.dts to use the same property with the other- prefix to every
> node.
>
> Also fix dm_test_ofnode_get_by_phandle_ot by making it more robust and
> renaming the phandle property to other-phandle.
>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  arch/sandbox/dts/other.dts | 24 ++++++++++-
>  test/dm/ofnode.c           | 83 +++++++++++++++++++++++++++++++-------
>  2 files changed, 91 insertions(+), 16 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v2 3/8] dm: core: implement ofnode/tree_parse_phandle() helper

[Christian Marangi]

Implement ofnode/tree_parse_phandle() helper as an equivalent of
of_parse_phandle to handle simple single value phandle.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/core/ofnode.c | 58 +++++++++++++++++++++++++++++++++++++++++++
 include/dm/ofnode.h   | 26 +++++++++++++++++++
 2 files changed, 84 insertions(+)

diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
index b9cc9419407..dc6ac069311 100644
--- a/drivers/core/ofnode.c
+++ b/drivers/core/ofnode.c
@@ -879,6 +879,64 @@ int ofnode_read_string_list(ofnode node, const char *property,
 	return count;
 }
 
+ofnode ofnode_parse_phandle(ofnode node, const char *phandle_name,
+			    int index)
+{
+	ofnode phandle;
+
+	if (ofnode_is_np(node)) {
+		struct device_node *np;
+
+		np = of_parse_phandle(ofnode_to_np(node), phandle_name,
+				      index);
+		if (!np)
+			return ofnode_null();
+
+		phandle = np_to_ofnode(np);
+	} else {
+		struct fdtdec_phandle_args args;
+
+		if (fdtdec_parse_phandle_with_args(ofnode_to_fdt(node),
+						   ofnode_to_offset(node),
+						   phandle_name, NULL,
+						   0, index, &args))
+			return ofnode_null();
+
+		phandle = offset_to_ofnode(args.node);
+	}
+
+	return phandle;
+}
+
+ofnode oftree_parse_phandle(oftree tree, ofnode node, const char *phandle_name,
+			    int index)
+{
+	ofnode phandle;
+
+	if (ofnode_is_np(node)) {
+		struct device_node *np;
+
+		np = of_root_parse_phandle(tree.np, ofnode_to_np(node),
+					   phandle_name, index);
+		if (!np)
+			return ofnode_null();
+
+		phandle = np_to_ofnode(np);
+	} else {
+		struct fdtdec_phandle_args args;
+
+		if (fdtdec_parse_phandle_with_args(tree.fdt,
+						   ofnode_to_offset(node),
+						   phandle_name, NULL,
+						   0, index, &args))
+			return ofnode_null();
+
+		phandle = noffset_to_ofnode(node, args.node);
+	}
+
+	return phandle;
+}
+
 static void ofnode_from_fdtdec_phandle_args(ofnode node, struct fdtdec_phandle_args *in,
 					    struct ofnode_phandle_args *out)
 {
diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h
index 7ece68874ae..eea6b13217c 100644
--- a/include/dm/ofnode.h
+++ b/include/dm/ofnode.h
@@ -847,6 +847,18 @@ int ofnode_read_string_count(ofnode node, const char *property);
 int ofnode_read_string_list(ofnode node, const char *property,
 			    const char ***listp);
 
+/**
+ * ofnode_parse_phandle() - Resolve a phandle property to an ofnode
+ *
+ * @node: node to check
+ * @phandle_name: Name of property holding a phandle value
+ * @index: For properties holding a table of phandles, this is the index into
+ *         the table
+ * Return: ofnode that the phandle points to or ofnode_null() on error.
+ */
+ofnode ofnode_parse_phandle(ofnode node, const char *phandle_name,
+			    int index);
+
 /**
  * ofnode_parse_phandle_with_args() - Find a node pointed by phandle in a list
  *
@@ -909,6 +921,20 @@ int ofnode_parse_phandle_with_args(ofnode node, const char *list_name,
 int ofnode_count_phandle_with_args(ofnode node, const char *list_name,
 				   const char *cells_name, int cell_count);
 
+/**
+ * oftree_parse_phandle() - Resolve a phandle property to an ofnode
+ *			    from a root node
+ *
+ * @tree: device tree to use
+ * @node: node to check
+ * @phandle_name: Name of property holding a phandle value
+ * @index: For properties holding a table of phandles, this is the index into
+ *         the table
+ * Return: ofnode that the phandle points to or ofnode_null() on error.
+ */
+ofnode oftree_parse_phandle(oftree tree, ofnode node, const char *phandle_name,
+			    int index);
+
 /**
  * oftree_parse_phandle_with_args() - Find a node pointed by phandle in a list
  *				      from a root node
-- 
2.45.2

Re: [PATCH v2 3/8] dm: core: implement ofnode/tree_parse_phandle() helper

[Simon Glass]

On Sun, 10 Nov 2024 at 04:51, Christian Marangi <ansuelsmth@gmail.com> wrote:
>
> Implement ofnode/tree_parse_phandle() helper as an equivalent of
> of_parse_phandle to handle simple single value phandle.
>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/core/ofnode.c | 58 +++++++++++++++++++++++++++++++++++++++++++
>  include/dm/ofnode.h   | 26 +++++++++++++++++++
>  2 files changed, 84 insertions(+)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v2 4/8] test: dm: Expand dm_test_ofnode_phandle(_ot) with new ofnode/tree_parse_phandle

[Christian Marangi]

Expand dm_test_ofnode_phandle(_ot) with new ofnode/tree_parse_phandle() op.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 arch/sandbox/dts/other.dts |  7 ++++++
 arch/sandbox/dts/test.dts  |  7 ++++++
 test/dm/ofnode.c           | 44 ++++++++++++++++++++++++++++++++++----
 3 files changed, 54 insertions(+), 4 deletions(-)

diff --git a/arch/sandbox/dts/other.dts b/arch/sandbox/dts/other.dts
index b32158c6569..515d6348b3f 100644
--- a/arch/sandbox/dts/other.dts
+++ b/arch/sandbox/dts/other.dts
@@ -32,6 +32,7 @@
 			<&other_gpio_b 5 GPIO_ACTIVE_HIGH 3 2 1>,
 			<0>, <&other_gpio_a 12>;
 		other-phandle-value = <&other_gpio_c 10>, <0xFFFFFFFF 20>, <&other_gpio_a 30>;
+		other-phandle-nodes = <&other_phandle_node_1>, <&other_phandle_node_2>;
 	};
 
 	other_gpio_a: other-gpio-a {
@@ -46,6 +47,12 @@
 		#gpio-cells = <2>;
 	};
 
+	other_phandle_node_1: other-phandle-node-1 {
+	};
+
+	other_phandle_node_2: other-phandle-node-2 {
+	};
+
 	target: target {
 		compatible = "sandbox-other2";
 		str-prop = "other";
diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index 3017b33d67b..b8a46463158 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -296,6 +296,12 @@
 		compatible = "sandbox,dsi-host";
 	};
 
+	phandle_node_1: phandle-node-1 {
+	};
+
+	phandle_node_2: phandle-node-2 {
+	};
+
 	a-test {
 		reg = <0 1>;
 		compatible = "denx,u-boot-fdt-test";
@@ -334,6 +340,7 @@
 		interrupts-extended = <&irq 3 0>;
 		acpi,name = "GHIJ";
 		phandle-value = <&gpio_c 10>, <0xFFFFFFFF 20>, <&gpio_a 30>;
+		phandle-nodes = <&phandle_node_1>, <&phandle_node_2>;
 
 		mux-controls = <&muxcontroller0 0>, <&muxcontroller0 1>,
 			       <&muxcontroller0 2>, <&muxcontroller0 3>,
diff --git a/test/dm/ofnode.c b/test/dm/ofnode.c
index 24b67bbe2b9..cf10e698d9e 100644
--- a/test/dm/ofnode.c
+++ b/test/dm/ofnode.c
@@ -280,15 +280,16 @@ static int dm_test_ofnode_read_ot(struct unit_test_state *uts)
 }
 DM_TEST(dm_test_ofnode_read_ot, UTF_SCAN_FDT | UTF_OTHER_FDT);
 
-/* test ofnode_count_/parse_phandle_with_args() */
+/* test ofnode_count_/parse/_phandle_with_args() */
 static int dm_test_ofnode_phandle(struct unit_test_state *uts)
 {
 	struct ofnode_phandle_args args;
-	ofnode node;
+	ofnode node, phandle, target;
 	int ret;
 	const char prop[] = "test-gpios";
 	const char cell[] = "#gpio-cells";
 	const char prop2[] = "phandle-value";
+	const char prop3[] = "phandle-nodes";
 
 	node = ofnode_path("/a-test");
 	ut_assert(ofnode_valid(node));
@@ -352,20 +353,38 @@ static int dm_test_ofnode_phandle(struct unit_test_state *uts)
 	ret = ofnode_parse_phandle_with_args(node, prop2, NULL, 1, 3, &args);
 	ut_asserteq(-ENOENT, ret);
 
+	/* Test ofnode_parse_phandle */
+	phandle = ofnode_parse_phandle(node, "missing", 0);
+	ut_assert(ofnode_equal(ofnode_null(), phandle));
+
+	target = ofnode_path("/phandle-node-1");
+	ut_assert(ofnode_valid(target));
+	phandle = ofnode_parse_phandle(node, prop3, 0);
+	ut_assert(ofnode_equal(target, phandle));
+
+	target = ofnode_path("/phandle-node-2");
+	ut_assert(ofnode_valid(target));
+	phandle = ofnode_parse_phandle(node, prop3, 1);
+	ut_assert(ofnode_equal(target, phandle));
+
+	phandle = ofnode_parse_phandle(node, prop3, 3);
+	ut_assert(ofnode_equal(ofnode_null(), phandle));
+
 	return 0;
 }
 DM_TEST(dm_test_ofnode_phandle, UTF_SCAN_PDATA | UTF_SCAN_FDT);
 
-/* test oftree_count_/parse_phandle_with_args() with 'other' tree */
+/* test oftree_count_/parse/_phandle_with_args() with 'other' tree */
 static int dm_test_ofnode_phandle_ot(struct unit_test_state *uts)
 {
 	oftree otree = get_other_oftree(uts);
 	struct ofnode_phandle_args args;
-	ofnode node;
+	ofnode node, phandle, target;
 	int ret;
 	const char prop[] = "other-test-gpios";
 	const char cell[] = "#gpio-cells";
 	const char prop2[] = "other-phandle-value";
+	const char prop3[] = "other-phandle-nodes";
 
 	node = oftree_path(otree, "/other-a-test");
 	ut_assert(ofnode_valid(node));
@@ -429,6 +448,23 @@ static int dm_test_ofnode_phandle_ot(struct unit_test_state *uts)
 	ret = oftree_parse_phandle_with_args(otree, node, prop2, NULL, 1, 3, &args);
 	ut_asserteq(-ENOENT, ret);
 
+	/* Test oftree_parse_phandle */
+	phandle = oftree_parse_phandle(otree, node, "missing", 0);
+	ut_assert(ofnode_equal(ofnode_null(), phandle));
+
+	target = oftree_path(otree, "/other-phandle-node-1");
+	ut_assert(ofnode_valid(target));
+	phandle = oftree_parse_phandle(otree, node, prop3, 0);
+	ut_assert(ofnode_equal(target, phandle));
+
+	target = oftree_path(otree, "/other-phandle-node-2");
+	ut_assert(ofnode_valid(target));
+	phandle = oftree_parse_phandle(otree, node, prop3, 1);
+	ut_assert(ofnode_equal(target, phandle));
+
+	phandle = oftree_parse_phandle(otree, node, prop3, 3);
+	ut_assert(ofnode_equal(ofnode_null(), phandle));
+
 	return 0;
 }
 DM_TEST(dm_test_ofnode_phandle_ot, UTF_OTHER_FDT);
-- 
2.45.2

Re: [PATCH v2 4/8] test: dm: Expand dm_test_ofnode_phandle(_ot) with new ofnode/tree_parse_phandle

[Simon Glass]

On Sun, 10 Nov 2024 at 04:51, Christian Marangi <ansuelsmth@gmail.com> wrote:
>
> Expand dm_test_ofnode_phandle(_ot) with new ofnode/tree_parse_phandle() op.
>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  arch/sandbox/dts/other.dts |  7 ++++++
>  arch/sandbox/dts/test.dts  |  7 ++++++
>  test/dm/ofnode.c           | 44 ++++++++++++++++++++++++++++++++++----
>  3 files changed, 54 insertions(+), 4 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v2 5/8] dm: core: implement phandle ofnode_options helper

[Christian Marangi]

Implement ofnode_options phandle helper to get an ofnode from a phandle
option in /options/u-boot.

This helper can be useful since new DT yaml usually require to link a
phandle of a node instead of referencing it by name or other indirect
way.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/core/ofnode.c | 15 +++++++++++++++
 include/dm/ofnode.h   | 15 +++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
index dc6ac069311..c8161827d1c 100644
--- a/drivers/core/ofnode.c
+++ b/drivers/core/ofnode.c
@@ -1871,6 +1871,21 @@ const char *ofnode_options_read_str(const char *prop_name)
 	return ofnode_read_string(uboot, prop_name);
 }
 
+int ofnode_options_get_by_phandle(const char *prop_name, ofnode *nodep)
+{
+	ofnode uboot;
+
+	uboot = ofnode_path("/options/u-boot");
+	if (!ofnode_valid(uboot))
+		return -EINVAL;
+
+	*nodep = ofnode_parse_phandle(uboot, prop_name, 0);
+	if (!ofnode_valid(*nodep))
+		return -EINVAL;
+
+	return 0;
+}
+
 int ofnode_read_bootscript_address(u64 *bootscr_address, u64 *bootscr_offset)
 {
 	int ret;
diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h
index eea6b13217c..890f0e6cf40 100644
--- a/include/dm/ofnode.h
+++ b/include/dm/ofnode.h
@@ -1720,6 +1720,21 @@ int ofnode_options_read_int(const char *prop_name, int default_val);
  */
 const char *ofnode_options_read_str(const char *prop_name);
 
+/**
+ * ofnode_options_get_by_phandle() - Get a ofnode from phandle from the U-Boot options
+ *
+ * This reads a property from the /options/u-boot/ node of the devicetree.
+ *
+ * This only works with the control FDT.
+ *
+ * See dtschema/schemas/options/u-boot.yaml in dt-schema project for bindings
+ *
+ * @prop_name: property name to look up
+ * @nodep: pointer to ofnode where node is stored
+ * Return: 0, if found, or negative error if not
+ */
+int ofnode_options_get_by_phandle(const char *prop_name, ofnode *nodep);
+
 /**
  * ofnode_read_bootscript_address() - Read bootscr-address or bootscr-ram-offset
  *
-- 
2.45.2

Re: [PATCH v2 5/8] dm: core: implement phandle ofnode_options helper

[Simon Glass]

On Sun, 10 Nov 2024 at 04:51, Christian Marangi <ansuelsmth@gmail.com> wrote:
>
> Implement ofnode_options phandle helper to get an ofnode from a phandle
> option in /options/u-boot.
>
> This helper can be useful since new DT yaml usually require to link a
> phandle of a node instead of referencing it by name or other indirect
> way.
>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/core/ofnode.c | 15 +++++++++++++++
>  include/dm/ofnode.h   | 15 +++++++++++++++
>  2 files changed, 30 insertions(+)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v2 6/8] test: dm: Add test for ofnode options phandle helper

[Christian Marangi]

Add test for ofnode options phandle helper and add new property in the
sandbox test dts.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 arch/sandbox/dts/test.dts |  1 +
 test/dm/ofnode.c          | 11 +++++++++++
 2 files changed, 12 insertions(+)

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index b8a46463158..1ffa64a43e2 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -106,6 +106,7 @@
 			testing-bool;
 			testing-int = <123>;
 			testing-str = "testing";
+			testing-phandle = <&phandle_node_1>;
 		};
 	};
 
diff --git a/test/dm/ofnode.c b/test/dm/ofnode.c
index cf10e698d9e..f16b643fa3f 100644
--- a/test/dm/ofnode.c
+++ b/test/dm/ofnode.c
@@ -704,6 +704,10 @@ static int dm_test_ofnode_options(struct unit_test_state *uts)
 {
 	u64 bootscr_address, bootscr_offset;
 	u64 bootscr_flash_offset, bootscr_flash_size;
+	ofnode node, phandle_node, target;
+
+	node = ofnode_path("/options/u-boot");
+	ut_assert(ofnode_valid(node));
 
 	ut_assert(!ofnode_options_read_bool("missing"));
 	ut_assert(ofnode_options_read_bool("testing-bool"));
@@ -714,6 +718,13 @@ static int dm_test_ofnode_options(struct unit_test_state *uts)
 	ut_assertnull(ofnode_options_read_str("missing"));
 	ut_asserteq_str("testing", ofnode_options_read_str("testing-str"));
 
+	ut_asserteq(-EINVAL, ofnode_options_get_by_phandle("missing", &phandle_node));
+
+	target = ofnode_path("/phandle-node-1");
+	ut_assert(ofnode_valid(target));
+	ut_assertok(ofnode_options_get_by_phandle("testing-phandle", &phandle_node));
+	ut_assert(ofnode_equal(target, phandle_node));
+
 	ut_assertok(ofnode_read_bootscript_address(&bootscr_address,
 						   &bootscr_offset));
 	ut_asserteq_64(0, bootscr_address);
-- 
2.45.2

Re: [PATCH v2 6/8] test: dm: Add test for ofnode options phandle helper

[Simon Glass]

On Sun, 10 Nov 2024 at 04:51, Christian Marangi <ansuelsmth@gmail.com> wrote:
>
> Add test for ofnode options phandle helper and add new property in the
> sandbox test dts.
>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  arch/sandbox/dts/test.dts |  1 +
>  test/dm/ofnode.c          | 11 +++++++++++
>  2 files changed, 12 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v2 7/8] led: update LED boot/activity to new property implementation

[Christian Marangi]

Update LED boot/activity to reference by phandle instead of label and
add to period property the "-ms" suffix.
This is a followup request by dt-schema maintainers that required LED
node to be referenced by a phandle to the node instead of indirectly by
the LED label and for timevalue to have a suffix.

While at it generalize the LED node label parsing since the logic is
common for generic LED bind and LED activity/boot.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/led/led-uclass.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
index 05e09909b7d..760750568c0 100644
--- a/drivers/led/led-uclass.c
+++ b/drivers/led/led-uclass.c
@@ -232,16 +232,24 @@ int led_activity_blink(void)
 #endif
 #endif
 
+static const char *led_get_label(ofnode node)
+{
+	const char *label;
+
+	label = ofnode_read_string(node, "label");
+	if (!label && !ofnode_read_string(node, "compatible"))
+		label = ofnode_get_name(node);
+
+	return label;
+}
+
 static int led_post_bind(struct udevice *dev)
 {
 	struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
 	const char *default_state;
 
 	if (!uc_plat->label)
-		uc_plat->label = dev_read_string(dev, "label");
-
-	if (!uc_plat->label && !dev_read_string(dev, "compatible"))
-		uc_plat->label = ofnode_get_name(dev_ofnode(dev));
+		uc_plat->label = led_get_label(dev_ofnode(dev));
 
 	uc_plat->default_state = LEDST_COUNT;
 
@@ -300,15 +308,21 @@ static int led_post_probe(struct udevice *dev)
 static int led_init(struct uclass *uc)
 {
 	struct led_uc_priv *priv = uclass_get_priv(uc);
+	ofnode led_node;
+	int ret;
 
 #ifdef CONFIG_LED_BOOT
-	priv->boot_led_label = ofnode_options_read_str("boot-led");
-	priv->boot_led_period = ofnode_options_read_int("boot-led-period", 250);
+	ret = ofnode_options_get_by_phandle("boot-led", &led_node);
+	if (!ret)
+		priv->boot_led_label = led_get_label(led_node);
+	priv->boot_led_period = ofnode_options_read_int("boot-led-period-ms", 250);
 #endif
 
 #ifdef CONFIG_LED_ACTIVITY
-	priv->activity_led_label = ofnode_options_read_str("activity-led");
-	priv->activity_led_period = ofnode_options_read_int("activity-led-period",
+	ret = ofnode_options_get_by_phandle("activity-led", &led_node);
+	if (!ret)
+		priv->activity_led_label = led_get_label(led_node);
+	priv->activity_led_period = ofnode_options_read_int("activity-led-period-ms",
 							    250);
 #endif
 
-- 
2.45.2

[PATCH v2 8/8] test: dm: Update test for LED activity and boot

[Christian Marangi]

Update test for LED activity and boot to follow new implementation with
property set to the LED node phandle.

Also update a copy-paste error in the function name for the activity
tests and actually enable the test with the DM_TEST macro.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 arch/sandbox/dts/test.dts |  8 ++++----
 test/dm/led.c             | 18 +++++++++++-------
 2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index 1ffa64a43e2..e9b3b151e10 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -101,8 +101,8 @@
 			bootscr-ram-offset = /bits/ 64 <0x12345678>;
 			bootscr-flash-offset = /bits/ 64 <0>;
 			bootscr-flash-size = /bits/ 64 <0x2000>;
-			boot-led = "sandbox:green";
-			activity-led = "sandbox:red";
+			boot-led = <&sandbox_led_green>;
+			activity-led = <&sandbox_led_red>;
 			testing-bool;
 			testing-int = <123>;
 			testing-str = "testing";
@@ -988,12 +988,12 @@
 	leds {
 		compatible = "gpio-leds";
 
-		iracibble {
+		sandbox_led_red: iracibble {
 			gpios = <&gpio_a 1 0>;
 			label = "sandbox:red";
 		};
 
-		martinet {
+		sandbox_led_green: martinet {
 			gpios = <&gpio_a 2 0>;
 			label = "sandbox:green";
 		};
diff --git a/test/dm/led.c b/test/dm/led.c
index 884f6410b70..e5b86326c3a 100644
--- a/test/dm/led.c
+++ b/test/dm/led.c
@@ -144,7 +144,7 @@ static int dm_test_led_boot(struct unit_test_state *uts)
 {
 	struct udevice *dev
 
-	/* options/u-boot/boot-led is set to "sandbox:green" */
+	/* options/u-boot/boot-led is set to phandle to "sandbox:green" */
 	ut_assertok(led_get_by_label("sandbox:green", &dev));
 	ut_asserteq(LEDST_OFF, led_get_state(dev));
 	ut_assertok(led_boot_on());
@@ -154,14 +154,15 @@ static int dm_test_led_boot(struct unit_test_state *uts)
 
 	return 0;
 }
+DM_TEST(dm_test_led_boot, UTF_SCAN_PDATA | UTF_SCAN_FDT);
 
 /* Test LED boot blink fallback */
 #ifndef CONFIG_LED_BLINK
-static int dm_test_led_boot(struct unit_test_state *uts)
+static int dm_test_led_boot_blink(struct unit_test_state *uts)
 {
 	struct udevice *dev
 
-	/* options/u-boot/boot-led is set to "sandbox:green" */
+	/* options/u-boot/boot-led is set to phandle to "sandbox:green" */
 	ut_assertok(led_get_by_label("sandbox:green", &dev));
 	ut_asserteq(LEDST_OFF, led_get_state(dev));
 	ut_assertok(led_boot_blink());
@@ -171,16 +172,17 @@ static int dm_test_led_boot(struct unit_test_state *uts)
 
 	return 0;
 }
+DM_TEST(dm_test_led_boot_blink, UTF_SCAN_PDATA | UTF_SCAN_FDT);
 #endif
 #endif
 
 /* Test LED activity */
 #ifdef CONFIG_LED_ACTIVITY
-static int dm_test_led_boot(struct unit_test_state *uts)
+static int dm_test_led_activity(struct unit_test_state *uts)
 {
 	struct udevice *dev
 
-	/* options/u-boot/activity-led is set to "sandbox:red" */
+	/* options/u-boot/activity-led is set to phandle to "sandbox:red" */
 	ut_assertok(led_get_by_label("sandbox:red", &dev));
 	ut_asserteq(LEDST_OFF, led_get_state(dev));
 	ut_assertok(led_activity_on());
@@ -190,14 +192,15 @@ static int dm_test_led_boot(struct unit_test_state *uts)
 
 	return 0;
 }
+DM_TEST(dm_test_led_activity, UTF_SCAN_PDATA | UTF_SCAN_FDT);
 
 /* Test LED activity blink fallback */
 #ifndef CONFIG_LED_BLINK
-static int dm_test_led_boot(struct unit_test_state *uts)
+static int dm_test_led_activityt_blink(struct unit_test_state *uts)
 {
 	struct udevice *dev
 
-	/* options/u-boot/activity-led is set to "sandbox:red" */
+	/* options/u-boot/activity-led is set to phandle to "sandbox:red" */
 	ut_assertok(led_get_by_label("sandbox:red", &dev));
 	ut_asserteq(LEDST_OFF, led_get_state(dev));
 	ut_assertok(led_activity_blink());
@@ -207,5 +210,6 @@ static int dm_test_led_boot(struct unit_test_state *uts)
 
 	return 0;
 }
+DM_TEST(dm_test_led_activityt_blink, UTF_SCAN_PDATA | UTF_SCAN_FDT);
 #endif
 #endif
-- 
2.45.2

Re: [PATCH v2 8/8] test: dm: Update test for LED activity and boot

[Simon Glass]

On Sun, 10 Nov 2024 at 04:51, Christian Marangi <ansuelsmth@gmail.com> wrote:
>
> Update test for LED activity and boot to follow new implementation with
> property set to the LED node phandle.
>
> Also update a copy-paste error in the function name for the activity
> tests and actually enable the test with the DM_TEST macro.
>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  arch/sandbox/dts/test.dts |  8 ++++----
>  test/dm/led.c             | 18 +++++++++++-------
>  2 files changed, 15 insertions(+), 11 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v2 0/8] led: update LED boot/activity to new property implementation

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2916 bytes --]

On Sun, Nov 10, 2024 at 12:50:19PM +0100, Christian Marangi wrote:

> This series is split in 2 part.
> 
> While adapting the LED boot and activity code to the new property
> accepted by Rob in dt-schema repository, a big BUG was discovered.
> 
> The reason wasn't clear at start and took me some days to figure it
> out.
> 
> This was triggered by adding a new phandle in the test.dts to
> introduce test for the new OPs.
> 
> This single addition caused the sandbox CI test to fail in the
> dm_test_ofnode_phandle_ot test.
> 
> This doesn't make sense as reverting the change made the CI test
> to correctly finish. Also moving the uboot node down
> after the first phandle (in test.dts the gpio one) also made
> the CI test to correctly finish.
> 
> A little bit of searching and debugging made me realize the
> parse phandle OPs didn't support other.dts at all and they
> were still referencing phandle index from test.dts.
> (more info in the related commit)
> 
> In short the test was broken all along and was working by
> pure luck. The first 4 patch address and fix the problem for good.
> 
> The other 4 patch expand and address the property change for
> LED boot/activity.
> 
> Posting in a single series as changes are trivial and just
> to speedup review process. (and also because the second
> part depends on the first)
> 
> All CI tested with azure pipeline.
> 
> Changes v2:
> - Fix handling of flat tree for phandle
> - Fix test and other.dts changes
> 
> Christian Marangi (8):
>   dm: core: implement oftree variant of parse_phandle OPs
>   test: dm: fix broken dm_test_ofnode_phandle_ot and get_by_phandle_ot
>   dm: core: implement ofnode/tree_parse_phandle() helper
>   test: dm: Expand dm_test_ofnode_phandle(_ot) with new
>     ofnode/tree_parse_phandle
>   dm: core: implement phandle ofnode_options helper
>   test: dm: Add test for ofnode options phandle helper
>   led: update LED boot/activity to new property implementation
>   test: dm: Update test for LED activity and boot
> 
>  arch/sandbox/dts/other.dts |  31 ++++++++-
>  arch/sandbox/dts/test.dts  |  16 +++--
>  drivers/core/of_access.c   |  61 ++++++++++++-----
>  drivers/core/ofnode.c      | 124 ++++++++++++++++++++++++++++++++-
>  drivers/led/led-uclass.c   |  30 +++++---
>  include/dm/of_access.h     |  86 +++++++++++++++++++++++
>  include/dm/ofnode.h        | 107 +++++++++++++++++++++++++++++
>  test/dm/led.c              |  18 +++--
>  test/dm/ofnode.c           | 136 ++++++++++++++++++++++++++++++++-----
>  9 files changed, 551 insertions(+), 58 deletions(-)

My main issue with the series is a lack of documentation updates, as the
biggest challenge thus far has been that for example Peter couldn't
figure out how to make use of this on PinePhone. We generate
documentation today based on include/led.h for this API, yes? Thanks.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 0/8] led: update LED boot/activity to new property implementation

[Christian Marangi]

On Wed, Nov 13, 2024 at 12:00:59PM -0600, Tom Rini wrote:
> On Sun, Nov 10, 2024 at 12:50:19PM +0100, Christian Marangi wrote:
> 
> > This series is split in 2 part.
> > 
> > While adapting the LED boot and activity code to the new property
> > accepted by Rob in dt-schema repository, a big BUG was discovered.
> > 
> > The reason wasn't clear at start and took me some days to figure it
> > out.
> > 
> > This was triggered by adding a new phandle in the test.dts to
> > introduce test for the new OPs.
> > 
> > This single addition caused the sandbox CI test to fail in the
> > dm_test_ofnode_phandle_ot test.
> > 
> > This doesn't make sense as reverting the change made the CI test
> > to correctly finish. Also moving the uboot node down
> > after the first phandle (in test.dts the gpio one) also made
> > the CI test to correctly finish.
> > 
> > A little bit of searching and debugging made me realize the
> > parse phandle OPs didn't support other.dts at all and they
> > were still referencing phandle index from test.dts.
> > (more info in the related commit)
> > 
> > In short the test was broken all along and was working by
> > pure luck. The first 4 patch address and fix the problem for good.
> > 
> > The other 4 patch expand and address the property change for
> > LED boot/activity.
> > 
> > Posting in a single series as changes are trivial and just
> > to speedup review process. (and also because the second
> > part depends on the first)
> > 
> > All CI tested with azure pipeline.
> > 
> > Changes v2:
> > - Fix handling of flat tree for phandle
> > - Fix test and other.dts changes
> > 
> > Christian Marangi (8):
> >   dm: core: implement oftree variant of parse_phandle OPs
> >   test: dm: fix broken dm_test_ofnode_phandle_ot and get_by_phandle_ot
> >   dm: core: implement ofnode/tree_parse_phandle() helper
> >   test: dm: Expand dm_test_ofnode_phandle(_ot) with new
> >     ofnode/tree_parse_phandle
> >   dm: core: implement phandle ofnode_options helper
> >   test: dm: Add test for ofnode options phandle helper
> >   led: update LED boot/activity to new property implementation
> >   test: dm: Update test for LED activity and boot
> > 
> >  arch/sandbox/dts/other.dts |  31 ++++++++-
> >  arch/sandbox/dts/test.dts  |  16 +++--
> >  drivers/core/of_access.c   |  61 ++++++++++++-----
> >  drivers/core/ofnode.c      | 124 ++++++++++++++++++++++++++++++++-
> >  drivers/led/led-uclass.c   |  30 +++++---
> >  include/dm/of_access.h     |  86 +++++++++++++++++++++++
> >  include/dm/ofnode.h        | 107 +++++++++++++++++++++++++++++
> >  test/dm/led.c              |  18 +++--
> >  test/dm/ofnode.c           | 136 ++++++++++++++++++++++++++++++++-----
> >  9 files changed, 551 insertions(+), 58 deletions(-)
> 
> My main issue with the series is a lack of documentation updates, as the
> biggest challenge thus far has been that for example Peter couldn't
> figure out how to make use of this on PinePhone. We generate
> documentation today based on include/led.h for this API, yes? Thanks.
> 

Hi Tom,

actually quite the oppisite, led.h describe how these works and was
instructed to first have the options property merged in dt-schema. [1]

Here we have all the option documented under a yaml.

I notice there is a series from Simon that is also pushing a .yaml
here locally in U-Boot and I was waiting for that to be merged to also
include the additional entry there.

Also led.h description and API have info on where to look about the
handling of /options/u-boot/

I feel the main problem with documentation is currently the fact that we
are migrating to a more robust schema and people are used to using Doc
directory? Anyway happy to get any hint on how to improve this.

[1] https://github.com/devicetree-org/dt-schema

-- 
	Ansuel

Re: [PATCH v2 0/8] led: update LED boot/activity to new property implementation

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4271 bytes --]

On Wed, Nov 13, 2024 at 09:24:59PM +0100, Christian Marangi wrote:
> On Wed, Nov 13, 2024 at 12:00:59PM -0600, Tom Rini wrote:
> > On Sun, Nov 10, 2024 at 12:50:19PM +0100, Christian Marangi wrote:
> > 
> > > This series is split in 2 part.
> > > 
> > > While adapting the LED boot and activity code to the new property
> > > accepted by Rob in dt-schema repository, a big BUG was discovered.
> > > 
> > > The reason wasn't clear at start and took me some days to figure it
> > > out.
> > > 
> > > This was triggered by adding a new phandle in the test.dts to
> > > introduce test for the new OPs.
> > > 
> > > This single addition caused the sandbox CI test to fail in the
> > > dm_test_ofnode_phandle_ot test.
> > > 
> > > This doesn't make sense as reverting the change made the CI test
> > > to correctly finish. Also moving the uboot node down
> > > after the first phandle (in test.dts the gpio one) also made
> > > the CI test to correctly finish.
> > > 
> > > A little bit of searching and debugging made me realize the
> > > parse phandle OPs didn't support other.dts at all and they
> > > were still referencing phandle index from test.dts.
> > > (more info in the related commit)
> > > 
> > > In short the test was broken all along and was working by
> > > pure luck. The first 4 patch address and fix the problem for good.
> > > 
> > > The other 4 patch expand and address the property change for
> > > LED boot/activity.
> > > 
> > > Posting in a single series as changes are trivial and just
> > > to speedup review process. (and also because the second
> > > part depends on the first)
> > > 
> > > All CI tested with azure pipeline.
> > > 
> > > Changes v2:
> > > - Fix handling of flat tree for phandle
> > > - Fix test and other.dts changes
> > > 
> > > Christian Marangi (8):
> > >   dm: core: implement oftree variant of parse_phandle OPs
> > >   test: dm: fix broken dm_test_ofnode_phandle_ot and get_by_phandle_ot
> > >   dm: core: implement ofnode/tree_parse_phandle() helper
> > >   test: dm: Expand dm_test_ofnode_phandle(_ot) with new
> > >     ofnode/tree_parse_phandle
> > >   dm: core: implement phandle ofnode_options helper
> > >   test: dm: Add test for ofnode options phandle helper
> > >   led: update LED boot/activity to new property implementation
> > >   test: dm: Update test for LED activity and boot
> > > 
> > >  arch/sandbox/dts/other.dts |  31 ++++++++-
> > >  arch/sandbox/dts/test.dts  |  16 +++--
> > >  drivers/core/of_access.c   |  61 ++++++++++++-----
> > >  drivers/core/ofnode.c      | 124 ++++++++++++++++++++++++++++++++-
> > >  drivers/led/led-uclass.c   |  30 +++++---
> > >  include/dm/of_access.h     |  86 +++++++++++++++++++++++
> > >  include/dm/ofnode.h        | 107 +++++++++++++++++++++++++++++
> > >  test/dm/led.c              |  18 +++--
> > >  test/dm/ofnode.c           | 136 ++++++++++++++++++++++++++++++++-----
> > >  9 files changed, 551 insertions(+), 58 deletions(-)
> > 
> > My main issue with the series is a lack of documentation updates, as the
> > biggest challenge thus far has been that for example Peter couldn't
> > figure out how to make use of this on PinePhone. We generate
> > documentation today based on include/led.h for this API, yes? Thanks.
> > 
> 
> Hi Tom,
> 
> actually quite the oppisite, led.h describe how these works and was
> instructed to first have the options property merged in dt-schema. [1]
> 
> Here we have all the option documented under a yaml.
> 
> I notice there is a series from Simon that is also pushing a .yaml
> here locally in U-Boot and I was waiting for that to be merged to also
> include the additional entry there.
> 
> Also led.h description and API have info on where to look about the
> handling of /options/u-boot/
> 
> I feel the main problem with documentation is currently the fact that we
> are migrating to a more robust schema and people are used to using Doc
> directory? Anyway happy to get any hint on how to improve this.
> 
> [1] https://github.com/devicetree-org/dt-schema

So Peter, does this bring you what you were asking for in terms of
details, or can you explain what's still missing from your point of
view? Thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 0/8] led: update LED boot/activity to new property implementation

[Christian Marangi]

On Fri, Nov 22, 2024 at 08:41:13PM -0600, Tom Rini wrote:
> On Wed, Nov 13, 2024 at 09:24:59PM +0100, Christian Marangi wrote:
> > On Wed, Nov 13, 2024 at 12:00:59PM -0600, Tom Rini wrote:
> > > On Sun, Nov 10, 2024 at 12:50:19PM +0100, Christian Marangi wrote:
> > > 
> > > > This series is split in 2 part.
> > > > 
> > > > While adapting the LED boot and activity code to the new property
> > > > accepted by Rob in dt-schema repository, a big BUG was discovered.
> > > > 
> > > > The reason wasn't clear at start and took me some days to figure it
> > > > out.
> > > > 
> > > > This was triggered by adding a new phandle in the test.dts to
> > > > introduce test for the new OPs.
> > > > 
> > > > This single addition caused the sandbox CI test to fail in the
> > > > dm_test_ofnode_phandle_ot test.
> > > > 
> > > > This doesn't make sense as reverting the change made the CI test
> > > > to correctly finish. Also moving the uboot node down
> > > > after the first phandle (in test.dts the gpio one) also made
> > > > the CI test to correctly finish.
> > > > 
> > > > A little bit of searching and debugging made me realize the
> > > > parse phandle OPs didn't support other.dts at all and they
> > > > were still referencing phandle index from test.dts.
> > > > (more info in the related commit)
> > > > 
> > > > In short the test was broken all along and was working by
> > > > pure luck. The first 4 patch address and fix the problem for good.
> > > > 
> > > > The other 4 patch expand and address the property change for
> > > > LED boot/activity.
> > > > 
> > > > Posting in a single series as changes are trivial and just
> > > > to speedup review process. (and also because the second
> > > > part depends on the first)
> > > > 
> > > > All CI tested with azure pipeline.
> > > > 
> > > > Changes v2:
> > > > - Fix handling of flat tree for phandle
> > > > - Fix test and other.dts changes
> > > > 
> > > > Christian Marangi (8):
> > > >   dm: core: implement oftree variant of parse_phandle OPs
> > > >   test: dm: fix broken dm_test_ofnode_phandle_ot and get_by_phandle_ot
> > > >   dm: core: implement ofnode/tree_parse_phandle() helper
> > > >   test: dm: Expand dm_test_ofnode_phandle(_ot) with new
> > > >     ofnode/tree_parse_phandle
> > > >   dm: core: implement phandle ofnode_options helper
> > > >   test: dm: Add test for ofnode options phandle helper
> > > >   led: update LED boot/activity to new property implementation
> > > >   test: dm: Update test for LED activity and boot
> > > > 
> > > >  arch/sandbox/dts/other.dts |  31 ++++++++-
> > > >  arch/sandbox/dts/test.dts  |  16 +++--
> > > >  drivers/core/of_access.c   |  61 ++++++++++++-----
> > > >  drivers/core/ofnode.c      | 124 ++++++++++++++++++++++++++++++++-
> > > >  drivers/led/led-uclass.c   |  30 +++++---
> > > >  include/dm/of_access.h     |  86 +++++++++++++++++++++++
> > > >  include/dm/ofnode.h        | 107 +++++++++++++++++++++++++++++
> > > >  test/dm/led.c              |  18 +++--
> > > >  test/dm/ofnode.c           | 136 ++++++++++++++++++++++++++++++++-----
> > > >  9 files changed, 551 insertions(+), 58 deletions(-)
> > > 
> > > My main issue with the series is a lack of documentation updates, as the
> > > biggest challenge thus far has been that for example Peter couldn't
> > > figure out how to make use of this on PinePhone. We generate
> > > documentation today based on include/led.h for this API, yes? Thanks.
> > > 
> > 
> > Hi Tom,
> > 
> > actually quite the oppisite, led.h describe how these works and was
> > instructed to first have the options property merged in dt-schema. [1]
> > 
> > Here we have all the option documented under a yaml.
> > 
> > I notice there is a series from Simon that is also pushing a .yaml
> > here locally in U-Boot and I was waiting for that to be merged to also
> > include the additional entry there.
> > 
> > Also led.h description and API have info on where to look about the
> > handling of /options/u-boot/
> > 
> > I feel the main problem with documentation is currently the fact that we
> > are migrating to a more robust schema and people are used to using Doc
> > directory? Anyway happy to get any hint on how to improve this.
> > 
> > [1] https://github.com/devicetree-org/dt-schema
> 
> So Peter, does this bring you what you were asking for in terms of
> details, or can you explain what's still missing from your point of
> view? Thanks!
> 

Hi Tom,

any idea how to improve this and get it trought?

-- 
	Ansuel

Re: [PATCH v2 0/8] led: update LED boot/activity to new property implementation

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4902 bytes --]

On Fri, Dec 06, 2024 at 06:30:00PM +0100, Christian Marangi wrote:
> On Fri, Nov 22, 2024 at 08:41:13PM -0600, Tom Rini wrote:
> > On Wed, Nov 13, 2024 at 09:24:59PM +0100, Christian Marangi wrote:
> > > On Wed, Nov 13, 2024 at 12:00:59PM -0600, Tom Rini wrote:
> > > > On Sun, Nov 10, 2024 at 12:50:19PM +0100, Christian Marangi wrote:
> > > > 
> > > > > This series is split in 2 part.
> > > > > 
> > > > > While adapting the LED boot and activity code to the new property
> > > > > accepted by Rob in dt-schema repository, a big BUG was discovered.
> > > > > 
> > > > > The reason wasn't clear at start and took me some days to figure it
> > > > > out.
> > > > > 
> > > > > This was triggered by adding a new phandle in the test.dts to
> > > > > introduce test for the new OPs.
> > > > > 
> > > > > This single addition caused the sandbox CI test to fail in the
> > > > > dm_test_ofnode_phandle_ot test.
> > > > > 
> > > > > This doesn't make sense as reverting the change made the CI test
> > > > > to correctly finish. Also moving the uboot node down
> > > > > after the first phandle (in test.dts the gpio one) also made
> > > > > the CI test to correctly finish.
> > > > > 
> > > > > A little bit of searching and debugging made me realize the
> > > > > parse phandle OPs didn't support other.dts at all and they
> > > > > were still referencing phandle index from test.dts.
> > > > > (more info in the related commit)
> > > > > 
> > > > > In short the test was broken all along and was working by
> > > > > pure luck. The first 4 patch address and fix the problem for good.
> > > > > 
> > > > > The other 4 patch expand and address the property change for
> > > > > LED boot/activity.
> > > > > 
> > > > > Posting in a single series as changes are trivial and just
> > > > > to speedup review process. (and also because the second
> > > > > part depends on the first)
> > > > > 
> > > > > All CI tested with azure pipeline.
> > > > > 
> > > > > Changes v2:
> > > > > - Fix handling of flat tree for phandle
> > > > > - Fix test and other.dts changes
> > > > > 
> > > > > Christian Marangi (8):
> > > > >   dm: core: implement oftree variant of parse_phandle OPs
> > > > >   test: dm: fix broken dm_test_ofnode_phandle_ot and get_by_phandle_ot
> > > > >   dm: core: implement ofnode/tree_parse_phandle() helper
> > > > >   test: dm: Expand dm_test_ofnode_phandle(_ot) with new
> > > > >     ofnode/tree_parse_phandle
> > > > >   dm: core: implement phandle ofnode_options helper
> > > > >   test: dm: Add test for ofnode options phandle helper
> > > > >   led: update LED boot/activity to new property implementation
> > > > >   test: dm: Update test for LED activity and boot
> > > > > 
> > > > >  arch/sandbox/dts/other.dts |  31 ++++++++-
> > > > >  arch/sandbox/dts/test.dts  |  16 +++--
> > > > >  drivers/core/of_access.c   |  61 ++++++++++++-----
> > > > >  drivers/core/ofnode.c      | 124 ++++++++++++++++++++++++++++++++-
> > > > >  drivers/led/led-uclass.c   |  30 +++++---
> > > > >  include/dm/of_access.h     |  86 +++++++++++++++++++++++
> > > > >  include/dm/ofnode.h        | 107 +++++++++++++++++++++++++++++
> > > > >  test/dm/led.c              |  18 +++--
> > > > >  test/dm/ofnode.c           | 136 ++++++++++++++++++++++++++++++++-----
> > > > >  9 files changed, 551 insertions(+), 58 deletions(-)
> > > > 
> > > > My main issue with the series is a lack of documentation updates, as the
> > > > biggest challenge thus far has been that for example Peter couldn't
> > > > figure out how to make use of this on PinePhone. We generate
> > > > documentation today based on include/led.h for this API, yes? Thanks.
> > > > 
> > > 
> > > Hi Tom,
> > > 
> > > actually quite the oppisite, led.h describe how these works and was
> > > instructed to first have the options property merged in dt-schema. [1]
> > > 
> > > Here we have all the option documented under a yaml.
> > > 
> > > I notice there is a series from Simon that is also pushing a .yaml
> > > here locally in U-Boot and I was waiting for that to be merged to also
> > > include the additional entry there.
> > > 
> > > Also led.h description and API have info on where to look about the
> > > handling of /options/u-boot/
> > > 
> > > I feel the main problem with documentation is currently the fact that we
> > > are migrating to a more robust schema and people are used to using Doc
> > > directory? Anyway happy to get any hint on how to improve this.
> > > 
> > > [1] https://github.com/devicetree-org/dt-schema
> > 
> > So Peter, does this bring you what you were asking for in terms of
> > details, or can you explain what's still missing from your point of
> > view? Thanks!
> > 
> 
> Hi Tom,
> 
> any idea how to improve this and get it trought?

I'll pick this up for -next soon, thanks.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 0/8] led: update LED boot/activity to new property implementation

[Tom Rini]

On Sun, 10 Nov 2024 12:50:19 +0100, Christian Marangi wrote:

> This series is split in 2 part.
> 
> While adapting the LED boot and activity code to the new property
> accepted by Rob in dt-schema repository, a big BUG was discovered.
> 
> The reason wasn't clear at start and took me some days to figure it
> out.
> 
> [...]

Applied to u-boot/next, thanks!

-- 
Tom