commit 83c63f0d118 (led: Move OF "label" property parsing to core)

[Marek Vasut]

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