Re: [PATCH 1/6] led-uclass: do not create fallback label for top-level node
On 10/23/23 10:28, Rasmus Villemoes wrote: On 19/10/2023 15.51, Marek Vasut wrote: On 10/19/23 11:58, Rasmus Villemoes wrote: Many existing drivers, and led-uclass itself, rely on uc_plat->label being NULL for the device representing the top node, as opposed to the child nodes representing individual LEDs. This means that the drivers whose .probe methods rely on this were broken by 83c63f0d1185 ("led: Move OF "label" property parsing to core"), and also that the top node wrongly shows up with 'led list'. Some drivers have since been fixed up individually, e.g. e3aa76644c2a "led: gpio: Check device compatible string to determine the top level node" 01074697801b "led: gpio: Use NOP uclass driver for top-level node" 910b01c27c04 "drivers: led: bcm6753: do not use null label to find the top" Binding the same driver to the top node as to the individual child nodes is arguably wrong, and the approach of using a UCLASS_NOP driver for the top node is probably better. Note that 83c63f0d1185 ("led: Move OF "label" property parsing to core") and 01074697801b ("led: gpio: Use NOP uclass driver for top-level node") were applied shortly after each other, so I don't see the point of the aforementioned rant. What rant? I'm merely trying to write down what I found out while trying to figure out why 83c63f0d1185 broke stuff, while acknowledging that the fixes that have been applied to some drivers are probably the approach in general. I sort-of understand what you are trying to do in this patch based on $SUBJECT of this email, but not from this wall of text, so can you abbreviate the commit message ? Sure, I can try and make it shorter. - if (!uc_plat->label) + if (!uc_plat->label && !dev_read_string(dev, "compatible")) uc_plat->label = ofnode_get_name(dev_ofnode(dev)); Is there an existing driver which has a top-level DT node with "label" property ? -ENOPARSE? A driver doesn't have a top-level DT node. I mean, is there a driver which does: / { led { label = "example"; }; }; ? I think that is what the conditional checks here, right ? And regardless, this wouldn't change anything for a top-level DT node with a "label" property, as we're still unconditionally first trying to read a "label" property, this is merely avoiding adding a fallback value for top-level DT nodes (using "having a "compatible" DT property as proxy for that "is a top-level DT node"). So I really don't understand what you are trying to ask.
Re: [PATCH 1/6] led-uclass: do not create fallback label for top-level node
On 19/10/2023 15.51, Marek Vasut wrote: > On 10/19/23 11:58, Rasmus Villemoes wrote: >> Many existing drivers, and led-uclass itself, rely on uc_plat->label >> being NULL for the device representing the top node, as opposed to the >> child nodes representing individual LEDs. This means that the drivers >> whose .probe methods rely on this were broken by 83c63f0d1185 ("led: >> Move OF "label" property parsing to core"), and also that the top node >> wrongly shows up with 'led list'. Some drivers have since been fixed >> up individually, e.g. >> >> e3aa76644c2a "led: gpio: Check device compatible string to determine >> the top level node" >> 01074697801b "led: gpio: Use NOP uclass driver for top-level node" >> 910b01c27c04 "drivers: led: bcm6753: do not use null label to find the >> top" >> >> Binding the same driver to the top node as to the individual child >> nodes is arguably wrong, and the approach of using a UCLASS_NOP driver >> for the top node is probably better. > > Note that > 83c63f0d1185 ("led: Move OF "label" property parsing to core") > and > 01074697801b ("led: gpio: Use NOP uclass driver for top-level node") > were applied shortly after each other, so I don't see the point of the > aforementioned rant. What rant? I'm merely trying to write down what I found out while trying to figure out why 83c63f0d1185 broke stuff, while acknowledging that the fixes that have been applied to some drivers are probably the approach in general. > I sort-of understand what you are trying to do in this patch based on > $SUBJECT of this email, but not from this wall of text, so can you > abbreviate the commit message ? Sure, I can try and make it shorter. >> - if (!uc_plat->label) >> + if (!uc_plat->label && !dev_read_string(dev, "compatible")) >> uc_plat->label = ofnode_get_name(dev_ofnode(dev)); > > Is there an existing driver which has a top-level DT node with "label" > property ? -ENOPARSE? A driver doesn't have a top-level DT node. And regardless, this wouldn't change anything for a top-level DT node with a "label" property, as we're still unconditionally first trying to read a "label" property, this is merely avoiding adding a fallback value for top-level DT nodes (using "having a "compatible" DT property as proxy for that "is a top-level DT node"). So I really don't understand what you are trying to ask. Rasmus
Re: [PATCH 1/6] led-uclass: do not create fallback label for top-level node
On 10/19/23 11:58, Rasmus Villemoes wrote: Many existing drivers, and led-uclass itself, rely on uc_plat->label being NULL for the device representing the top node, as opposed to the child nodes representing individual LEDs. This means that the drivers whose .probe methods rely on this were broken by 83c63f0d1185 ("led: Move OF "label" property parsing to core"), and also that the top node wrongly shows up with 'led list'. Some drivers have since been fixed up individually, e.g. e3aa76644c2a "led: gpio: Check device compatible string to determine the top level node" 01074697801b "led: gpio: Use NOP uclass driver for top-level node" 910b01c27c04 "drivers: led: bcm6753: do not use null label to find the top" Binding the same driver to the top node as to the individual child nodes is arguably wrong, and the approach of using a UCLASS_NOP driver for the top node is probably better. Note that 83c63f0d1185 ("led: Move OF "label" property parsing to core") and 01074697801b ("led: gpio: Use NOP uclass driver for top-level node") were applied shortly after each other, so I don't see the point of the aforementioned rant. I sort-of understand what you are trying to do in this patch based on $SUBJECT of this email, but not from this wall of text, so can you abbreviate the commit message ? But as a temporary work-around, we can use a heuristic that only sets the label to the fallback value derived from the node name if the node does not have a "compatible" property - i.e., if it has been bound to the LED driver explicitly via device_bind_driver_to_node() [similar to what e3aa76644c2a did, but that then vanished with the next commit.] Fixes: 83c63f0d1185 ("led: Move OF "label" property parsing to core") Signed-off-by: Rasmus Villemoes --- drivers/led/led-uclass.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c index 68ca3c2970..5a5d07b9a7 100644 --- a/drivers/led/led-uclass.c +++ b/drivers/led/led-uclass.c @@ -72,7 +72,7 @@ static int led_post_bind(struct udevice *dev) const char *default_state; uc_plat->label = dev_read_string(dev, "label"); - if (!uc_plat->label) + if (!uc_plat->label && !dev_read_string(dev, "compatible")) uc_plat->label = ofnode_get_name(dev_ofnode(dev)); Is there an existing driver which has a top-level DT node with "label" property ? uc_plat->default_state = LEDST_COUNT;
[PATCH 1/6] led-uclass: do not create fallback label for top-level node
Many existing drivers, and led-uclass itself, rely on uc_plat->label being NULL for the device representing the top node, as opposed to the child nodes representing individual LEDs. This means that the drivers whose .probe methods rely on this were broken by 83c63f0d1185 ("led: Move OF "label" property parsing to core"), and also that the top node wrongly shows up with 'led list'. Some drivers have since been fixed up individually, e.g. e3aa76644c2a "led: gpio: Check device compatible string to determine the top level node" 01074697801b "led: gpio: Use NOP uclass driver for top-level node" 910b01c27c04 "drivers: led: bcm6753: do not use null label to find the top" Binding the same driver to the top node as to the individual child nodes is arguably wrong, and the approach of using a UCLASS_NOP driver for the top node is probably better. But as a temporary work-around, we can use a heuristic that only sets the label to the fallback value derived from the node name if the node does not have a "compatible" property - i.e., if it has been bound to the LED driver explicitly via device_bind_driver_to_node() [similar to what e3aa76644c2a did, but that then vanished with the next commit.] Fixes: 83c63f0d1185 ("led: Move OF "label" property parsing to core") Signed-off-by: Rasmus Villemoes --- drivers/led/led-uclass.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c index 68ca3c2970..5a5d07b9a7 100644 --- a/drivers/led/led-uclass.c +++ b/drivers/led/led-uclass.c @@ -72,7 +72,7 @@ static int led_post_bind(struct udevice *dev) const char *default_state; uc_plat->label = dev_read_string(dev, "label"); - if (!uc_plat->label) + if (!uc_plat->label && !dev_read_string(dev, "compatible")) uc_plat->label = ofnode_get_name(dev_ofnode(dev)); uc_plat->default_state = LEDST_COUNT; -- 2.40.1.1.g1c60b9335d