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() .