On 10/17/23 15:29, Rasmus Villemoes wrote:
Hi,
Hi,
I'm trying to resurrect an old submission of a driver for ti,lp5562, so
had occasion to dig into drivers/led/. And I think commit 83c63f0d118 is
buggy or at least incomplete.
Many of the drivers that were subsequently modified to not do that
"label" parsing rely, in their .probe method, on the top node being
distinguished by not having a ->label. And led-uclass itself does that
/* Ignore the top-level LED node */
if (uc_plat->label && !strcmp(label, uc_plat->label))
return uclass_get_device_tail(dev, 0, devp);
The drivers used to only do that label-parsing for subnodes of the top
node, so that worked, but the new code assigns some ->label for anything
bound to a LED uclass driver. There have been at least two patches to
individual drivers to fix this up since (the current top two patches
touching drivers/led/), but I think that's the wrong way to handle this.
At the same time, I actually think 83c63f0d118 didn't go far enough in
deduplicating, since all drivers are left with essentially the same loop:
static int led_gpio_bind(struct udevice *parent)
{
struct udevice *dev;
ofnode node;
int ret;
dev_for_each_subnode(node, parent) {
ret = device_bind_driver_to_node(parent, "gpio_led",
ofnode_get_name(node),
node, &dev);
if (ret)
return ret;
}
return 0;
}
static int bcm6753_led_bind(struct udevice *parent)
{
ofnode node;
dev_for_each_subnode(node, parent) {
struct udevice *dev;
int ret;
ret = device_bind_driver_to_node(parent, "bcm6753-led",
ofnode_get_name(node),
node, &dev);
if (ret)
return ret;
}
return 0;
}
and that string can, if I'm not mistaken, be gotten from
parent->driver->name.
Which string ? The "bcm6753-led" is driver name , so that would have to
be parametrized.
So we really should be able to create a
generic_led_bind() that does exactly this loop + the "label" parsing,
remove the label parsing from post_bind (so it doesn't apply to the top
nodes)
Make sure you test the 'led' command. The LEDs might be bound, but not
probed (so label parsing in LED probe would be too late), and if I
recall it right, the label parsing has to be in post-bind for the labels
to correctly show in the 'led' command listing.
, and switch all drivers over to this generic_led_bind().
Alternatively, we can still create a generic_led_bind() that just does
the loop as above, but then somehow prevent the top nodes from gaining
->label, say by not adding the nodename fallback if the node has a
"compatible" property (though that seems like a hack, maybe there's a
cleaner way).
I am all for generic_led_bind() .
