"leds: gpio: Use generic support for composing LED names" breaks gpio debugfs

"leds: gpio: Use generic support for composing LED names" breaks gpio debugfs

[Russell King - ARM Linux admin]

Hi,

Patch d7235f5feaa0 ("leds: gpio: Use generic support for composing LED
names") changed the way labels are used.

devm_fwnode_get_gpiod_from_child() is passed led.name, which used to be
set to the label or node name, resulting in /sys/kernel/debug/gpio
containing useful information.  For example:

 gpio-487 (                    |udpu:green:power    ) out lo
 gpio-488 (                    |udpu:red:power      ) out hi
 gpio-489 (                    |udpu:green:network  ) out hi
 gpio-490 (                    |udpu:red:network    ) out hi
 gpio-491 (                    |udpu:green:alarm    ) out hi
 gpio-492 (                    |udpu:red:alarm      ) out hi

After this commit, it now contains a rather less useful:

 gpio-487 (                    |?                   ) out hi ACTIVE LOW
 gpio-488 (                    |?                   ) out hi ACTIVE LOW
 gpio-489 (                    |?                   ) out hi ACTIVE LOW
 gpio-490 (                    |?                   ) out hi ACTIVE LOW
 gpio-491 (                    |?                   ) out hi ACTIVE LOW
 gpio-492 (                    |?                   ) out hi ACTIVE LOW

This is because led.name is now NULL prior to the call to
devm_fwnode_get_gpiod_from_child().

While this is not an API, it is useful for debugging, and given the
dynamic allocation of GPIOs to physical GPIOs, having the labels
present is an advantage.

Can this be fixed / old behaviour restored?

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: "leds: gpio: Use generic support for composing LED names" breaks gpio debugfs

[Jacek Anaszewski]

Hi Russel,

On 12/4/19 12:02 PM, Russell King - ARM Linux admin wrote:
> Hi,
> 
> Patch d7235f5feaa0 ("leds: gpio: Use generic support for composing LED
> names") changed the way labels are used.
> 
> devm_fwnode_get_gpiod_from_child() is passed led.name, which used to be
> set to the label or node name, resulting in /sys/kernel/debug/gpio
> containing useful information.  For example:
> 
>  gpio-487 (                    |udpu:green:power    ) out lo
>  gpio-488 (                    |udpu:red:power      ) out hi
>  gpio-489 (                    |udpu:green:network  ) out hi
>  gpio-490 (                    |udpu:red:network    ) out hi
>  gpio-491 (                    |udpu:green:alarm    ) out hi
>  gpio-492 (                    |udpu:red:alarm      ) out hi
> 
> After this commit, it now contains a rather less useful:
> 
>  gpio-487 (                    |?                   ) out hi ACTIVE LOW
>  gpio-488 (                    |?                   ) out hi ACTIVE LOW
>  gpio-489 (                    |?                   ) out hi ACTIVE LOW
>  gpio-490 (                    |?                   ) out hi ACTIVE LOW
>  gpio-491 (                    |?                   ) out hi ACTIVE LOW
>  gpio-492 (                    |?                   ) out hi ACTIVE LOW
> 
> This is because led.name is now NULL prior to the call to
> devm_fwnode_get_gpiod_from_child().

Ugh, my bad. But it actually has been wrong since the patch
a96aa64cb5723 ("leds/led-class: Handle LEDs with the same name"),
which enabled LED core to amend the LED name just beforehand
the registration of a LED class device.

It seem that the only proper solution would be introducing a new
pre_register_cdev(const char *name) op to the LED core, that would allow
drivers to come up with their implementation thereof. In this particular
case leds-gpio driver would need to put there gpiod initialization.

The pre_register_device op would need to be called (when available)
from the led_classdev_register_ext() after calling
led_classdev_next_name() and before device_create_with_groups().
The op would have to be passed the final_name variable then.

> While this is not an API, it is useful for debugging, and given the
> dynamic allocation of GPIOs to physical GPIOs, having the labels
> present is an advantage.
> 
> Can this be fixed / old behaviour restored?

If there are no objections I can come up with a fix, but no
sooner than at the weekend.

-- 
Best regards,
Jacek Anaszewski

Re: "leds: gpio: Use generic support for composing LED names" breaks gpio debugfs

[Linus Walleij]

On Wed, Dec 4, 2019 at 9:52 PM Jacek Anaszewski
<jacek.anaszewski@gmail.com> wrote:

> It seem that the only proper solution would be introducing a new
> pre_register_cdev(const char *name) op to the LED core, that would allow
> drivers to come up with their implementation thereof. In this particular
> case leds-gpio driver would need to put there gpiod initialization.

I don't know the particulars but if it helps you can at any point
call:
int gpiod_set_consumer_name(struct gpio_desc *desc, const char *name);

For any successfully obtained descriptor, to set the consumer name,
i.e. the text appearing in debugfs and the GPIO character device.

Yours,
Linus Walleij

Re: "leds: gpio: Use generic support for composing LED names" breaks gpio debugfs

[Jacek Anaszewski]

On 12/5/19 12:45 AM, Linus Walleij wrote:
> On Wed, Dec 4, 2019 at 9:52 PM Jacek Anaszewski
> <jacek.anaszewski@gmail.com> wrote:
> 
>> It seem that the only proper solution would be introducing a new
>> pre_register_cdev(const char *name) op to the LED core, that would allow
>> drivers to come up with their implementation thereof. In this particular
>> case leds-gpio driver would need to put there gpiod initialization.
> 
> I don't know the particulars but if it helps you can at any point
> call:
> int gpiod_set_consumer_name(struct gpio_desc *desc, const char *name);
> 
> For any successfully obtained descriptor, to set the consumer name,
> i.e. the text appearing in debugfs and the GPIO character device.

Thanks for the pointer. I've just used that to fix the issue.

-- 
Best regards,
Jacek Anaszewski