Re: [PATCH 1/6] led-uclass: do not create fallback label for top-level node

[Marek Vasut <marex@denx.de>]

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.