commit 83c63f0d118 (led: Move OF "label" property parsing to core)
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. 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), 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).
WDYT?
Rasmus
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() .
On 17/10/2023 17.33, Marek Vasut wrote:
On 10/17/23 15:29, Rasmus Villemoes wrote:
static int led_gpio_bind(struct udevice *parent) {
...
ret = device_bind_driver_to_node(parent, "gpio_led", ofnode_get_name(node), node, &dev);
static int bcm6753_led_bind(struct udevice *parent) {
...
ret = device_bind_driver_to_node(parent, "bcm6753-led", ofnode_get_name(node), node, &dev);
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.
Exactly. The only difference between the two examples (apart from the scope of the variables) is the driver name, but I think a generic_led_bind() could just do this inside the loop:
device_bind_driver_to_node(parent, parent->driver->name, ofnode_get_name(node), node, &dev);
and with that we could have most, if not all, existing .bind methods assigned directly to generic_led_bind(), we don't even need one-line wrapper functions in each driver passing that string explicitly.
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.
It's the led command I'm testing with (with my "new" driver for the lp5562), and where I see the problems. Currently, the top node also gets listed in "led list" as led-controller@30 (it's an i2c device), but probe of that device fails because the logic in the .probe method thinks this is a subnode (the pattern is the same as in e.g. cortina_led_probe).
Since I'm simply copying the pattern from existing drivers, I'm pretty sure those other drivers are also currently broken (see also the fixups I mentioned).
I am all for generic_led_bind() .
I'll try to write some real patches.
Rasmus
On 18/10/2023 09.43, Rasmus Villemoes wrote:
On 17/10/2023 17.33, Marek Vasut wrote:
Which string ? The "bcm6753-led" is driver name , so that would have to be parametrized.
Exactly. The only difference between the two examples (apart from the scope of the variables) is the driver name, but I think a generic_led_bind() could just do this inside the loop:
device_bind_driver_to_node(parent, parent->driver->name, ofnode_get_name(node), node, &dev);
Ah, no, that won't work for the drivers that do distinguish between the top-level and child nodes, e.g. the gpio one. Oh well, a one-line wrapper in each driver is still better than what we have currently.
Rasmus
On 10/18/23 11:00, Rasmus Villemoes wrote:
On 18/10/2023 09.43, Rasmus Villemoes wrote:
On 17/10/2023 17.33, Marek Vasut wrote:
Which string ? The "bcm6753-led" is driver name , so that would have to be parametrized.
Exactly. The only difference between the two examples (apart from the scope of the variables) is the driver name, but I think a generic_led_bind() could just do this inside the loop:
device_bind_driver_to_node(parent, parent->driver->name, ofnode_get_name(node), node, &dev);
Ah, no, that won't work for the drivers that do distinguish between the top-level and child nodes, e.g. the gpio one. Oh well, a one-line wrapper in each driver is still better than what we have currently.
Right. I can actually test the gpio-leds one if you need that, since that is what I use anyway.
participants (2)
-
Marek Vasut -
Rasmus Villemoes