Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
On Tue, Nov 13, 2018 at 8:55 PM Jacek Anaszewski wrote: > LED core has had a protection > against name clash since the commit: > > commit a96aa64cb5723d941de879a9cd1fea025d6acb1b > Author: Ricardo Ribalda Delgado > Date: Mon Mar 30 10:45:59 2015 -0700 > > leds/led-class: Handle LEDs with the same name Oh neat! I guess the sysfs clashes I have seen can have been other stuff then, but it was a while back, I don't think it was so far back as this patch. Yours, Linus Walleij
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
On Tue, Nov 13, 2018 at 8:55 PM Jacek Anaszewski wrote: > LED core has had a protection > against name clash since the commit: > > commit a96aa64cb5723d941de879a9cd1fea025d6acb1b > Author: Ricardo Ribalda Delgado > Date: Mon Mar 30 10:45:59 2015 -0700 > > leds/led-class: Handle LEDs with the same name Oh neat! I guess the sysfs clashes I have seen can have been other stuff then, but it was a while back, I don't think it was so far back as this patch. Yours, Linus Walleij
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
On Tue 2018-11-13 22:38:49, Geert Uytterhoeven wrote: > Hi Pavel, > > On Mon, Nov 12, 2018 at 11:07 PM Pavel Machek wrote: > > Not really, I'm afraid. Hard drives have no red LEDs on them (and the > > LED would not be visible, anyway) so the "device" symlink in such case > > would point to some kind of i2c LED controller. > > I've seen hard drives with LEDs on the PCB. Me too. > External USB enclosures with hard drives typically have LEDs, too. Yes, but typically neither are OS controlled. I'm thinking home NAS system; it may have per-disk LEDs and they'll probably be on some kind of dedicated LED controller. Anyway, my point is that "device that controls this LED" is often different from "device this LED is about". Regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
On Tue 2018-11-13 22:38:49, Geert Uytterhoeven wrote: > Hi Pavel, > > On Mon, Nov 12, 2018 at 11:07 PM Pavel Machek wrote: > > Not really, I'm afraid. Hard drives have no red LEDs on them (and the > > LED would not be visible, anyway) so the "device" symlink in such case > > would point to some kind of i2c LED controller. > > I've seen hard drives with LEDs on the PCB. Me too. > External USB enclosures with hard drives typically have LEDs, too. Yes, but typically neither are OS controlled. I'm thinking home NAS system; it may have per-disk LEDs and they'll probably be on some kind of dedicated LED controller. Anyway, my point is that "device that controls this LED" is often different from "device this LED is about". Regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
Hi Pavel,
On Mon, Nov 12, 2018 at 11:07 PM Pavel Machek wrote:
> Not really, I'm afraid. Hard drives have no red LEDs on them (and the
> LED would not be visible, anyway) so the "device" symlink in such case
> would point to some kind of i2c LED controller.
I've seen hard drives with LEDs on the PCB.
External USB enclosures with hard drives typically have LEDs, too.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
Hi Pavel,
On Mon, Nov 12, 2018 at 11:07 PM Pavel Machek wrote:
> Not really, I'm afraid. Hard drives have no red LEDs on them (and the
> LED would not be visible, anyway) so the "device" symlink in such case
> would point to some kind of i2c LED controller.
I've seen hard drives with LEDs on the PCB.
External USB enclosures with hard drives typically have LEDs, too.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
On 11/12/2018 11:06 PM, Pavel Machek wrote: > On Mon 2018-11-12 21:11:32, Jacek Anaszewski wrote: >> On 11/12/2018 08:05 PM, Pavel Machek wrote: >>> Hi! >>> >>> My system looks like this: >>> >>> input16::capslocktpacpi::bay_activetpacpi::standby >>> input16::numlock tpacpi::dock_active tpacpi::thinklight >>> input16::scrolllock tpacpi::dock_batttpacpi::thinkvantage >>> input5::capslock tpacpi::dock_status1 tpacpi::unknown_led >>> input5::numlock tpacpi::dock_status2 tpacpi::unknown_led2 >>> input5::scrolllock tpacpi:green:batttpacpi::unknown_led3 >>> > But it is not just for backwards compatibility. See my examples above, > it is needed to tell which device the LED is asociated with, and it is > absolutely required for USB devices (for example). For USB devices there is already ledtrig-usbport available, which provides sysfs interface for defining and reading the usb ports, the status of which the LED indicates. Since the USB devices can be attached/removed dynamically, it would be impossible to reflect the associations in the LED class device name. >>> >>> I'm not talking USB activity. I'm talking USB devices with LEDs on >>> them, like for example keyboards. >>> >>> Please take a look at example above. input16::numlock ; >>> input5::numlock . You absolutely need device part. >> >> It would be redundant since there is "device" symbolic link >> created in given LED class device sysfs directory, pointing to the >> corresponding input device directory, like in case of my USB >> keyboard: > > You are right I forgot about the device symlink, and it partly helps > with the USB keyboard case... > > But you still need the device part. Sysfs will not like two > directories named "::numlock". LED core has a protection against that. See my reply to Linus. > >> #/sys/class/leds/input5::scrolllock$ ls -l >> total 0 >> -rw-r--r-- 1 root root 4096 Nov 12 20:22 brightness >> lrwxrwxrwx 1 root root0 Nov 12 20:22 device -> ../../input5 >> -r--r--r-- 1 root root 4096 Nov 12 20:22 max_brightness >> drwxr-xr-x 2 root root0 Nov 12 20:22 power >> lrwxrwxrwx 1 root root0 Nov 12 20:04 subsystem -> >> ../../../../../../../../../../../class/leds >> -rw-r--r-- 1 root root 4096 Nov 12 20:22 trigger > >>> Because userspace needs that information? >>> >>> Say you have raid array, with "error" leds for each drive (your list >>> already contains "hdderr"). Now userland detects problem with hdparm >>> on /dev/sdi. It would like to turn on corresponding LED. >>> >>> How do you propose we do that? >> >> Similarly as in case of USB keyboard. > > Not really, I'm afraid. Hard drives have no red LEDs on them (and the > LED would not be visible, anyway) so the "device" symlink in such case > would point to some kind of i2c LED controller. > > Eventually we'll need to have two devices for each LED. "Controller > this is on" -- in device symlink and "device this talks about". After thinking it over I agree - we will still need devicename part Please refer to my reply to Rob. -- Best regards, Jacek Anaszewski
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
On 11/12/2018 11:06 PM, Pavel Machek wrote: > On Mon 2018-11-12 21:11:32, Jacek Anaszewski wrote: >> On 11/12/2018 08:05 PM, Pavel Machek wrote: >>> Hi! >>> >>> My system looks like this: >>> >>> input16::capslocktpacpi::bay_activetpacpi::standby >>> input16::numlock tpacpi::dock_active tpacpi::thinklight >>> input16::scrolllock tpacpi::dock_batttpacpi::thinkvantage >>> input5::capslock tpacpi::dock_status1 tpacpi::unknown_led >>> input5::numlock tpacpi::dock_status2 tpacpi::unknown_led2 >>> input5::scrolllock tpacpi:green:batttpacpi::unknown_led3 >>> > But it is not just for backwards compatibility. See my examples above, > it is needed to tell which device the LED is asociated with, and it is > absolutely required for USB devices (for example). For USB devices there is already ledtrig-usbport available, which provides sysfs interface for defining and reading the usb ports, the status of which the LED indicates. Since the USB devices can be attached/removed dynamically, it would be impossible to reflect the associations in the LED class device name. >>> >>> I'm not talking USB activity. I'm talking USB devices with LEDs on >>> them, like for example keyboards. >>> >>> Please take a look at example above. input16::numlock ; >>> input5::numlock . You absolutely need device part. >> >> It would be redundant since there is "device" symbolic link >> created in given LED class device sysfs directory, pointing to the >> corresponding input device directory, like in case of my USB >> keyboard: > > You are right I forgot about the device symlink, and it partly helps > with the USB keyboard case... > > But you still need the device part. Sysfs will not like two > directories named "::numlock". LED core has a protection against that. See my reply to Linus. > >> #/sys/class/leds/input5::scrolllock$ ls -l >> total 0 >> -rw-r--r-- 1 root root 4096 Nov 12 20:22 brightness >> lrwxrwxrwx 1 root root0 Nov 12 20:22 device -> ../../input5 >> -r--r--r-- 1 root root 4096 Nov 12 20:22 max_brightness >> drwxr-xr-x 2 root root0 Nov 12 20:22 power >> lrwxrwxrwx 1 root root0 Nov 12 20:04 subsystem -> >> ../../../../../../../../../../../class/leds >> -rw-r--r-- 1 root root 4096 Nov 12 20:22 trigger > >>> Because userspace needs that information? >>> >>> Say you have raid array, with "error" leds for each drive (your list >>> already contains "hdderr"). Now userland detects problem with hdparm >>> on /dev/sdi. It would like to turn on corresponding LED. >>> >>> How do you propose we do that? >> >> Similarly as in case of USB keyboard. > > Not really, I'm afraid. Hard drives have no red LEDs on them (and the > LED would not be visible, anyway) so the "device" symlink in such case > would point to some kind of i2c LED controller. > > Eventually we'll need to have two devices for each LED. "Controller > this is on" -- in device symlink and "device this talks about". After thinking it over I agree - we will still need devicename part Please refer to my reply to Rob. -- Best regards, Jacek Anaszewski
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
On 11/12/2018 10:23 PM, Linus Walleij wrote: > On Sun, Nov 11, 2018 at 1:03 PM Pavel Machek wrote: > >>> -"devicename:colour:function" >>> +"colour:function" >> >> I don't think we want to do it in all cases. >> >> So, on my cellphone seeing lp5523:green:led is indeed not useful. >> >> But on notebook with usb keyboard attached, you need to keep the >> devicename to be able to distinguish capslock on internal keyboard and >> capslock on first USB keyboard and capslock on second USB keyboard. > > I agree with Pavel. > > I ran into this when connecting two identical keyboards to the > same machine. The driver worked fine up until it tries > to create two sysfs entries with the same name for capslock. > BOOM! What kernel version did you use? LED core has had a protection against name clash since the commit: commit a96aa64cb5723d941de879a9cd1fea025d6acb1b Author: Ricardo Ribalda Delgado Date: Mon Mar 30 10:45:59 2015 -0700 leds/led-class: Handle LEDs with the same name The current code expected that every LED had an unique name. This is a legit expectation when the device tree can no be modified or extended. But with device tree overlays this requirement can be easily broken. This patch finds out if the name is already in use and adds the suffix _1, _2... if not. Signed-off-by: Ricardo Ribalda Delgado Reported-by: Geert Uytterhoeven Signed-off-by: Bryan Wu > In this case I suggest "serialnumber:color:capslock" and > similar. > > The important point is the string must be unique for each > plugged device. -- Best regards, Jacek Anaszewski
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
On 11/12/2018 10:23 PM, Linus Walleij wrote: > On Sun, Nov 11, 2018 at 1:03 PM Pavel Machek wrote: > >>> -"devicename:colour:function" >>> +"colour:function" >> >> I don't think we want to do it in all cases. >> >> So, on my cellphone seeing lp5523:green:led is indeed not useful. >> >> But on notebook with usb keyboard attached, you need to keep the >> devicename to be able to distinguish capslock on internal keyboard and >> capslock on first USB keyboard and capslock on second USB keyboard. > > I agree with Pavel. > > I ran into this when connecting two identical keyboards to the > same machine. The driver worked fine up until it tries > to create two sysfs entries with the same name for capslock. > BOOM! What kernel version did you use? LED core has had a protection against name clash since the commit: commit a96aa64cb5723d941de879a9cd1fea025d6acb1b Author: Ricardo Ribalda Delgado Date: Mon Mar 30 10:45:59 2015 -0700 leds/led-class: Handle LEDs with the same name The current code expected that every LED had an unique name. This is a legit expectation when the device tree can no be modified or extended. But with device tree overlays this requirement can be easily broken. This patch finds out if the name is already in use and adds the suffix _1, _2... if not. Signed-off-by: Ricardo Ribalda Delgado Reported-by: Geert Uytterhoeven Signed-off-by: Bryan Wu > In this case I suggest "serialnumber:color:capslock" and > similar. > > The important point is the string must be unique for each > plugged device. -- Best regards, Jacek Anaszewski
LEDs on USB keyboards was Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
Hi! > > Couldn't we have here multiple variants that would then be chosen based > > on device tree definition? > > It needs to be subsystem specific or something. > What you say make sense for things based on device > tree. > > The problem hit me on an Intel laptop with several > USB keyboards. > > OK maybe I am stupid for plugging in two USB keyboards, > but we should definately support it. Input/HID supports it > well. Plugging two USB keyboards should be supported. Can we get some details... kernel version, architecture, maybe .config, where/how it fails? On my systems, keyboard leds are prefixed with device name, so I see no chance for duplicities... input5::capslock/ input5::numlock/ input5::scrolllock/ Thanks, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
LEDs on USB keyboards was Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
Hi! > > Couldn't we have here multiple variants that would then be chosen based > > on device tree definition? > > It needs to be subsystem specific or something. > What you say make sense for things based on device > tree. > > The problem hit me on an Intel laptop with several > USB keyboards. > > OK maybe I am stupid for plugging in two USB keyboards, > but we should definately support it. Input/HID supports it > well. Plugging two USB keyboards should be supported. Can we get some details... kernel version, architecture, maybe .config, where/how it fails? On my systems, keyboard leds are prefixed with device name, so I see no chance for duplicities... input5::capslock/ input5::numlock/ input5::scrolllock/ Thanks, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
On Mon 2018-11-12 21:11:32, Jacek Anaszewski wrote: > On 11/12/2018 08:05 PM, Pavel Machek wrote: > > Hi! > > > > My system looks like this: > > > > input16::capslocktpacpi::bay_activetpacpi::standby > > input16::numlock tpacpi::dock_active tpacpi::thinklight > > input16::scrolllock tpacpi::dock_batttpacpi::thinkvantage > > input5::capslock tpacpi::dock_status1 tpacpi::unknown_led > > input5::numlock tpacpi::dock_status2 tpacpi::unknown_led2 > > input5::scrolllock tpacpi:green:batttpacpi::unknown_led3 > > > >>> But it is not just for backwards compatibility. See my examples above, > >>> it is needed to tell which device the LED is asociated with, and it is > >>> absolutely required for USB devices (for example). > >> > >> For USB devices there is already ledtrig-usbport available, which > >> provides sysfs interface for defining and reading the usb ports, > >> the status of which the LED indicates. Since the USB devices can be > >> attached/removed dynamically, it would be impossible to reflect > >> the associations in the LED class device name. > > > > I'm not talking USB activity. I'm talking USB devices with LEDs on > > them, like for example keyboards. > > > > Please take a look at example above. input16::numlock ; > > input5::numlock . You absolutely need device part. > > It would be redundant since there is "device" symbolic link > created in given LED class device sysfs directory, pointing to the > corresponding input device directory, like in case of my USB > keyboard: You are right I forgot about the device symlink, and it partly helps with the USB keyboard case... But you still need the device part. Sysfs will not like two directories named "::numlock". > #/sys/class/leds/input5::scrolllock$ ls -l > total 0 > -rw-r--r-- 1 root root 4096 Nov 12 20:22 brightness > lrwxrwxrwx 1 root root0 Nov 12 20:22 device -> ../../input5 > -r--r--r-- 1 root root 4096 Nov 12 20:22 max_brightness > drwxr-xr-x 2 root root0 Nov 12 20:22 power > lrwxrwxrwx 1 root root0 Nov 12 20:04 subsystem -> > ../../../../../../../../../../../class/leds > -rw-r--r-- 1 root root 4096 Nov 12 20:22 trigger > > Because userspace needs that information? > > > > Say you have raid array, with "error" leds for each drive (your list > > already contains "hdderr"). Now userland detects problem with hdparm > > on /dev/sdi. It would like to turn on corresponding LED. > > > > How do you propose we do that? > > Similarly as in case of USB keyboard. Not really, I'm afraid. Hard drives have no red LEDs on them (and the LED would not be visible, anyway) so the "device" symlink in such case would point to some kind of i2c LED controller. Eventually we'll need to have two devices for each LED. "Controller this is on" -- in device symlink and "device this talks about". Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
On Mon 2018-11-12 21:11:32, Jacek Anaszewski wrote: > On 11/12/2018 08:05 PM, Pavel Machek wrote: > > Hi! > > > > My system looks like this: > > > > input16::capslocktpacpi::bay_activetpacpi::standby > > input16::numlock tpacpi::dock_active tpacpi::thinklight > > input16::scrolllock tpacpi::dock_batttpacpi::thinkvantage > > input5::capslock tpacpi::dock_status1 tpacpi::unknown_led > > input5::numlock tpacpi::dock_status2 tpacpi::unknown_led2 > > input5::scrolllock tpacpi:green:batttpacpi::unknown_led3 > > > >>> But it is not just for backwards compatibility. See my examples above, > >>> it is needed to tell which device the LED is asociated with, and it is > >>> absolutely required for USB devices (for example). > >> > >> For USB devices there is already ledtrig-usbport available, which > >> provides sysfs interface for defining and reading the usb ports, > >> the status of which the LED indicates. Since the USB devices can be > >> attached/removed dynamically, it would be impossible to reflect > >> the associations in the LED class device name. > > > > I'm not talking USB activity. I'm talking USB devices with LEDs on > > them, like for example keyboards. > > > > Please take a look at example above. input16::numlock ; > > input5::numlock . You absolutely need device part. > > It would be redundant since there is "device" symbolic link > created in given LED class device sysfs directory, pointing to the > corresponding input device directory, like in case of my USB > keyboard: You are right I forgot about the device symlink, and it partly helps with the USB keyboard case... But you still need the device part. Sysfs will not like two directories named "::numlock". > #/sys/class/leds/input5::scrolllock$ ls -l > total 0 > -rw-r--r-- 1 root root 4096 Nov 12 20:22 brightness > lrwxrwxrwx 1 root root0 Nov 12 20:22 device -> ../../input5 > -r--r--r-- 1 root root 4096 Nov 12 20:22 max_brightness > drwxr-xr-x 2 root root0 Nov 12 20:22 power > lrwxrwxrwx 1 root root0 Nov 12 20:04 subsystem -> > ../../../../../../../../../../../class/leds > -rw-r--r-- 1 root root 4096 Nov 12 20:22 trigger > > Because userspace needs that information? > > > > Say you have raid array, with "error" leds for each drive (your list > > already contains "hdderr"). Now userland detects problem with hdparm > > on /dev/sdi. It would like to turn on corresponding LED. > > > > How do you propose we do that? > > Similarly as in case of USB keyboard. Not really, I'm afraid. Hard drives have no red LEDs on them (and the LED would not be visible, anyway) so the "device" symlink in such case would point to some kind of i2c LED controller. Eventually we'll need to have two devices for each LED. "Controller this is on" -- in device symlink and "device this talks about". Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
On Mon, Nov 12, 2018 at 1:02 AM Vesa Jääskeläinen wrote: > Couldn't we have here multiple variants that would then be chosen based > on device tree definition? It needs to be subsystem specific or something. What you say make sense for things based on device tree. The problem hit me on an Intel laptop with several USB keyboards. OK maybe I am stupid for plugging in two USB keyboards, but we should definately support it. Input/HID supports it well. Yours, Linus Walleij
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
On Mon, Nov 12, 2018 at 1:02 AM Vesa Jääskeläinen wrote: > Couldn't we have here multiple variants that would then be chosen based > on device tree definition? It needs to be subsystem specific or something. What you say make sense for things based on device tree. The problem hit me on an Intel laptop with several USB keyboards. OK maybe I am stupid for plugging in two USB keyboards, but we should definately support it. Input/HID supports it well. Yours, Linus Walleij
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
On Sun, Nov 11, 2018 at 1:03 PM Pavel Machek wrote: > > -"devicename:colour:function" > > +"colour:function" > > I don't think we want to do it in all cases. > > So, on my cellphone seeing lp5523:green:led is indeed not useful. > > But on notebook with usb keyboard attached, you need to keep the > devicename to be able to distinguish capslock on internal keyboard and > capslock on first USB keyboard and capslock on second USB keyboard. I agree with Pavel. I ran into this when connecting two identical keyboards to the same machine. The driver worked fine up until it tries to create two sysfs entries with the same name for capslock. BOOM! In this case I suggest "serialnumber:color:capslock" and similar. The important point is the string must be unique for each plugged device. Yours, Linus Walleij
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
On Sun, Nov 11, 2018 at 1:03 PM Pavel Machek wrote: > > -"devicename:colour:function" > > +"colour:function" > > I don't think we want to do it in all cases. > > So, on my cellphone seeing lp5523:green:led is indeed not useful. > > But on notebook with usb keyboard attached, you need to keep the > devicename to be able to distinguish capslock on internal keyboard and > capslock on first USB keyboard and capslock on second USB keyboard. I agree with Pavel. I ran into this when connecting two identical keyboards to the same machine. The driver worked fine up until it tries to create two sysfs entries with the same name for capslock. BOOM! In this case I suggest "serialnumber:color:capslock" and similar. The important point is the string must be unique for each plugged device. Yours, Linus Walleij
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
On 11/12/2018 08:05 PM, Pavel Machek wrote: > Hi! > > My system looks like this: > > input16::capslocktpacpi::bay_activetpacpi::standby > input16::numlock tpacpi::dock_active tpacpi::thinklight > input16::scrolllock tpacpi::dock_batt tpacpi::thinkvantage > input5::capslock tpacpi::dock_status1 tpacpi::unknown_led > input5::numlock tpacpi::dock_status2 tpacpi::unknown_led2 > input5::scrolllock tpacpi:green:batt tpacpi::unknown_led3 > >>> But it is not just for backwards compatibility. See my examples above, >>> it is needed to tell which device the LED is asociated with, and it is >>> absolutely required for USB devices (for example). >> >> For USB devices there is already ledtrig-usbport available, which >> provides sysfs interface for defining and reading the usb ports, >> the status of which the LED indicates. Since the USB devices can be >> attached/removed dynamically, it would be impossible to reflect >> the associations in the LED class device name. > > I'm not talking USB activity. I'm talking USB devices with LEDs on > them, like for example keyboards. > > Please take a look at example above. input16::numlock ; > input5::numlock . You absolutely need device part. It would be redundant since there is "device" symbolic link created in given LED class device sysfs directory, pointing to the corresponding input device directory, like in case of my USB keyboard: #/sys/class/leds/input5::scrolllock$ ls -l total 0 -rw-r--r-- 1 root root 4096 Nov 12 20:22 brightness lrwxrwxrwx 1 root root0 Nov 12 20:22 device -> ../../input5 -r--r--r-- 1 root root 4096 Nov 12 20:22 max_brightness drwxr-xr-x 2 root root0 Nov 12 20:22 power lrwxrwxrwx 1 root root0 Nov 12 20:04 subsystem -> ../../../../../../../../../../../class/leds -rw-r--r-- 1 root root 4096 Nov 12 20:22 trigger >>> And even for "embedded" stuff like routers, we want eth0:green:link, >>> eth0:yellow:activity and not some kind of hack. >> >> eth0 is not something you can be certain of at the stage of defining DT >> node. > > In the common case DT is used for, yes, you can, because whole system > is in one box. Only when all kernel modules are built-in. > (And really DT does not matter. We are talking kernel <-> user > interface here). OK, I was fixed on the devicename understood as LED controller name. >>> Ideally, colors would come from fixed list, functions would come from >>> fixed list, and device part would come from name used elsewhere in the >>> kernel. >>> >>> (And yes, it probably means we should have something in device tree to >>> link LED to its device. device = "name" would be good start...) >> >> Why would you need such link? > > Because userspace needs that information? > > Say you have raid array, with "error" leds for each drive (your list > already contains "hdderr"). Now userland detects problem with hdparm > on /dev/sdi. It would like to turn on corresponding LED. > > How do you propose we do that? Similarly as in case of USB keyboard. -- Best regards, Jacek Anaszewski
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
On 11/12/2018 08:05 PM, Pavel Machek wrote: > Hi! > > My system looks like this: > > input16::capslocktpacpi::bay_activetpacpi::standby > input16::numlock tpacpi::dock_active tpacpi::thinklight > input16::scrolllock tpacpi::dock_batt tpacpi::thinkvantage > input5::capslock tpacpi::dock_status1 tpacpi::unknown_led > input5::numlock tpacpi::dock_status2 tpacpi::unknown_led2 > input5::scrolllock tpacpi:green:batt tpacpi::unknown_led3 > >>> But it is not just for backwards compatibility. See my examples above, >>> it is needed to tell which device the LED is asociated with, and it is >>> absolutely required for USB devices (for example). >> >> For USB devices there is already ledtrig-usbport available, which >> provides sysfs interface for defining and reading the usb ports, >> the status of which the LED indicates. Since the USB devices can be >> attached/removed dynamically, it would be impossible to reflect >> the associations in the LED class device name. > > I'm not talking USB activity. I'm talking USB devices with LEDs on > them, like for example keyboards. > > Please take a look at example above. input16::numlock ; > input5::numlock . You absolutely need device part. It would be redundant since there is "device" symbolic link created in given LED class device sysfs directory, pointing to the corresponding input device directory, like in case of my USB keyboard: #/sys/class/leds/input5::scrolllock$ ls -l total 0 -rw-r--r-- 1 root root 4096 Nov 12 20:22 brightness lrwxrwxrwx 1 root root0 Nov 12 20:22 device -> ../../input5 -r--r--r-- 1 root root 4096 Nov 12 20:22 max_brightness drwxr-xr-x 2 root root0 Nov 12 20:22 power lrwxrwxrwx 1 root root0 Nov 12 20:04 subsystem -> ../../../../../../../../../../../class/leds -rw-r--r-- 1 root root 4096 Nov 12 20:22 trigger >>> And even for "embedded" stuff like routers, we want eth0:green:link, >>> eth0:yellow:activity and not some kind of hack. >> >> eth0 is not something you can be certain of at the stage of defining DT >> node. > > In the common case DT is used for, yes, you can, because whole system > is in one box. Only when all kernel modules are built-in. > (And really DT does not matter. We are talking kernel <-> user > interface here). OK, I was fixed on the devicename understood as LED controller name. >>> Ideally, colors would come from fixed list, functions would come from >>> fixed list, and device part would come from name used elsewhere in the >>> kernel. >>> >>> (And yes, it probably means we should have something in device tree to >>> link LED to its device. device = "name" would be good start...) >> >> Why would you need such link? > > Because userspace needs that information? > > Say you have raid array, with "error" leds for each drive (your list > already contains "hdderr"). Now userland detects problem with hdparm > on /dev/sdi. It would like to turn on corresponding LED. > > How do you propose we do that? Similarly as in case of USB keyboard. -- Best regards, Jacek Anaszewski
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
Hi! > >>> My system looks like this: > >>> > >>> input16::capslocktpacpi::bay_activetpacpi::standby > >>> input16::numlock tpacpi::dock_active tpacpi::thinklight > >>> input16::scrolllock tpacpi::dock_batt tpacpi::thinkvantage > >>> input5::capslock tpacpi::dock_status1 tpacpi::unknown_led > >>> input5::numlock tpacpi::dock_status2 tpacpi::unknown_led2 > >>> input5::scrolllock tpacpi:green:batt tpacpi::unknown_led3 > > But it is not just for backwards compatibility. See my examples above, > > it is needed to tell which device the LED is asociated with, and it is > > absolutely required for USB devices (for example). > > For USB devices there is already ledtrig-usbport available, which > provides sysfs interface for defining and reading the usb ports, > the status of which the LED indicates. Since the USB devices can be > attached/removed dynamically, it would be impossible to reflect > the associations in the LED class device name. I'm not talking USB activity. I'm talking USB devices with LEDs on them, like for example keyboards. Please take a look at example above. input16::numlock ; input5::numlock . You absolutely need device part. > > And even for "embedded" stuff like routers, we want eth0:green:link, > > eth0:yellow:activity and not some kind of hack. > > eth0 is not something you can be certain of at the stage of defining DT > node. In the common case DT is used for, yes, you can, because whole system is in one box. (And really DT does not matter. We are talking kernel <-> user interface here). > > Ideally, colors would come from fixed list, functions would come from > > fixed list, and device part would come from name used elsewhere in the > > kernel. > > > > (And yes, it probably means we should have something in device tree to > > link LED to its device. device = "name" would be good start...) > > Why would you need such link? Because userspace needs that information? Say you have raid array, with "error" leds for each drive (your list already contains "hdderr"). Now userland detects problem with hdparm on /dev/sdi. It would like to turn on corresponding LED. How do you propose we do that? Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
Hi! > >>> My system looks like this: > >>> > >>> input16::capslocktpacpi::bay_activetpacpi::standby > >>> input16::numlock tpacpi::dock_active tpacpi::thinklight > >>> input16::scrolllock tpacpi::dock_batt tpacpi::thinkvantage > >>> input5::capslock tpacpi::dock_status1 tpacpi::unknown_led > >>> input5::numlock tpacpi::dock_status2 tpacpi::unknown_led2 > >>> input5::scrolllock tpacpi:green:batt tpacpi::unknown_led3 > > But it is not just for backwards compatibility. See my examples above, > > it is needed to tell which device the LED is asociated with, and it is > > absolutely required for USB devices (for example). > > For USB devices there is already ledtrig-usbport available, which > provides sysfs interface for defining and reading the usb ports, > the status of which the LED indicates. Since the USB devices can be > attached/removed dynamically, it would be impossible to reflect > the associations in the LED class device name. I'm not talking USB activity. I'm talking USB devices with LEDs on them, like for example keyboards. Please take a look at example above. input16::numlock ; input5::numlock . You absolutely need device part. > > And even for "embedded" stuff like routers, we want eth0:green:link, > > eth0:yellow:activity and not some kind of hack. > > eth0 is not something you can be certain of at the stage of defining DT > node. In the common case DT is used for, yes, you can, because whole system is in one box. (And really DT does not matter. We are talking kernel <-> user interface here). > > Ideally, colors would come from fixed list, functions would come from > > fixed list, and device part would come from name used elsewhere in the > > kernel. > > > > (And yes, it probably means we should have something in device tree to > > link LED to its device. device = "name" would be good start...) > > Why would you need such link? Because userspace needs that information? Say you have raid array, with "error" leds for each drive (your list already contains "hdderr"). Now userland detects problem with hdparm on /dev/sdi. It would like to turn on corresponding LED. How do you propose we do that? Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
Hi, On 11/12/2018 11:35 AM, Pavel Machek wrote: > Hi! > It's overcomplicating the naming again. In every case you can tweak the function name to eth0_link, eth1_link etc. >>> >>> Well, but in such case it would be good to keep existing scheme. >>> >>> My system looks like this: >>> >>> input16::capslocktpacpi::bay_activetpacpi::standby >>> input16::numlock tpacpi::dock_active tpacpi::thinklight >>> input16::scrolllock tpacpi::dock_batttpacpi::thinkvantage >>> input5::capslock tpacpi::dock_status1 tpacpi::unknown_led >>> input5::numlock tpacpi::dock_status2 tpacpi::unknown_led2 >>> input5::scrolllock tpacpi:green:batttpacpi::unknown_led3 >>> >>> I agree that we should get rid of "tpacpi:" part in some cases. But >>> it does not make sense to get rid of "input16:" part -- it tells you >>> if the LED is on USB or on built-in keyboard. >>> >>> Ideally, tpacpi::thinklight would be input5::frontlight (as it is >>> frontlight for the keyboard). >>> >>> Yes we should simplify, but it still needs to work in all cases. >> >> Well, label is not being removed. You still can use it an the old >> fashion, even when using new led_compose_name(). >> >> Maybe removing the description of the old LED naming from >> Documentation/leds/leds-class.txt was too drastic move. >> I'll keep it next to the new one, and only add a note that >> it is kept only for backwards compatibility. > > I agree that removing it is "just too drastic". > > But it is not just for backwards compatibility. See my examples above, > it is needed to tell which device the LED is asociated with, and it is > absolutely required for USB devices (for example). For USB devices there is already ledtrig-usbport available, which provides sysfs interface for defining and reading the usb ports, the status of which the LED indicates. Since the USB devices can be attached/removed dynamically, it would be impossible to reflect the associations in the LED class device name. > And even for "embedded" stuff like routers, we want eth0:green:link, > eth0:yellow:activity and not some kind of hack. eth0 is not something you can be certain of at the stage of defining DT node. > Ideally, colors would come from fixed list, functions would come from > fixed list, and device part would come from name used elsewhere in the > kernel. > > (And yes, it probably means we should have something in device tree to > link LED to its device. device = "name" would be good start...) Why would you need such link? -- Best regards, Jacek Anaszewski
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
Hi, On 11/12/2018 11:35 AM, Pavel Machek wrote: > Hi! > It's overcomplicating the naming again. In every case you can tweak the function name to eth0_link, eth1_link etc. >>> >>> Well, but in such case it would be good to keep existing scheme. >>> >>> My system looks like this: >>> >>> input16::capslocktpacpi::bay_activetpacpi::standby >>> input16::numlock tpacpi::dock_active tpacpi::thinklight >>> input16::scrolllock tpacpi::dock_batttpacpi::thinkvantage >>> input5::capslock tpacpi::dock_status1 tpacpi::unknown_led >>> input5::numlock tpacpi::dock_status2 tpacpi::unknown_led2 >>> input5::scrolllock tpacpi:green:batttpacpi::unknown_led3 >>> >>> I agree that we should get rid of "tpacpi:" part in some cases. But >>> it does not make sense to get rid of "input16:" part -- it tells you >>> if the LED is on USB or on built-in keyboard. >>> >>> Ideally, tpacpi::thinklight would be input5::frontlight (as it is >>> frontlight for the keyboard). >>> >>> Yes we should simplify, but it still needs to work in all cases. >> >> Well, label is not being removed. You still can use it an the old >> fashion, even when using new led_compose_name(). >> >> Maybe removing the description of the old LED naming from >> Documentation/leds/leds-class.txt was too drastic move. >> I'll keep it next to the new one, and only add a note that >> it is kept only for backwards compatibility. > > I agree that removing it is "just too drastic". > > But it is not just for backwards compatibility. See my examples above, > it is needed to tell which device the LED is asociated with, and it is > absolutely required for USB devices (for example). For USB devices there is already ledtrig-usbport available, which provides sysfs interface for defining and reading the usb ports, the status of which the LED indicates. Since the USB devices can be attached/removed dynamically, it would be impossible to reflect the associations in the LED class device name. > And even for "embedded" stuff like routers, we want eth0:green:link, > eth0:yellow:activity and not some kind of hack. eth0 is not something you can be certain of at the stage of defining DT node. > Ideally, colors would come from fixed list, functions would come from > fixed list, and device part would come from name used elsewhere in the > kernel. > > (And yes, it probably means we should have something in device tree to > link LED to its device. device = "name" would be good start...) Why would you need such link? -- Best regards, Jacek Anaszewski
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
On 11/12/2018 01:01 AM, Vesa Jääskeläinen wrote: > Hi, > > On 11/11/2018 23.14, Jacek Anaszewski wrote: >> On 11/11/2018 09:16 PM, Pavel Machek wrote: >>> Hi! >>> >> diff --git a/Documentation/leds/leds-class.txt >> b/Documentation/leds/leds-class.txt >> index 836cb16..e9009c4 100644 >> --- a/Documentation/leds/leds-class.txt >> +++ b/Documentation/leds/leds-class.txt >> @@ -43,7 +43,7 @@ LED Device Naming >> Is currently of the form: >> -"devicename:colour:function" >> +"colour:function" > > Couldn't we have here multiple variants that would then be chosen based > on device tree definition? > > "devicename:colour:function" > "devicename:function" > "devicename:label" > "colour:function" > "function" > "label" Documentation/leds/leds-class.txt in a description of "devicename:colour:function" naming scheme states the following: "If sections of the name don't apply, just leave that section blank" Currently not many Device Tree nodes follow that recommendation but with the led_compose_name() I'm going to enforce it (only for the devicename-less naming scheme), which is needed for reliable parsing of LED color and function, which with naming schemes you proposed it wouldn't be possible. But, even with led_compose_name(), it will be possible to get the names listed by you: If you provide the 'label' property in the DT, then with the new led_compose_name() you will get the LED class device name in one of the two below forms, depending on the parameters passed to the function: - If led_hw_name argument is not NULL: led_hw_name:label - If led_hw_name argument is NULL then the label is taken as-is for for LED class device name (it may be appended a numerical suffix if the name is already taken) This flexibility is for backwards compatibility with existing drivers, where some of them add devicename section to the parsed DT label, and others adopts the DT label as-is for LED class device name, relying on DT to provide the devicename. > If "label" would be specified then just use that as a led name, giving > name: > - label Please get acquainted with the entire patch set, the commit messages, and the documentation of led_compose_name(). You'll find out that this is exactly how led_compose_name() is designed. > If "function" would be defined then go to special formatting with > optional "color", giving names: > - color:function > - function Color must not be omitted as explained above. Of course in case 'label' is provided, it won't be validated in any way, thus allowing for any combination. With the 'color' and/or 'function' DT properties you'll get one of the following: - color:function - :function - color: Having considered all the above, I'd even propose to change the delimiter for the new naming convention from ":" to "-", to make it possible to distinguish between old and new naming convention. > I suppose 'devicename' would then be automatically filled based on > driver instance unless one defines 'no-devicename' or something like > that for led definition. devicename section is problematic in the LED class device name, since it doesn't allow for having uniform userspace interface across different platforms. Think of Android - I've seen they have their own LED naming scheme which doesn't contain devicename section. Also, please refer to the DT maintainer's opinion in this regard [0], which actually drew my attention to the problem. Please keep in mind that devicename has been so far used for board vendor name or LED controller name. > Personally I do not see the need for "color" in any led name. In my > opinion the only thing needed here would be location prefix (where > needed -- and it should be possible to disable that) and then logical > name for the led. You mean we don't need the color information at all? And could you please explain what do you mean by "location prefix"? > I don't think we want to do it in all cases. > > So, on my cellphone seeing lp5523:green:led is indeed not useful. > > But on notebook with usb keyboard attached, you need to keep the > devicename to be able to distinguish capslock on internal keyboard and > capslock on first USB keyboard and capslock on second USB keyboard. > > Taking look at the list of functions, here's stuff like "hdd" and > "hdderr". I assume the first means hdd activity... If we can do it, it > would be nicest to have "sda:green:activity" and maybe > "sda:red:error". For a raid array with 8 drives... > > For example I have a router running linux. It has 4 lan ports, with > correspondings LED, and an wan led. > > Having "green:lan1" to "green:lan4" and "green:wan" plus > "red:wanerror" would work, but I'd really preffer > "eth0:green:link"... "adsl0:green:link", "adsl0:red:error". > > There are now phones with flashes on both main and selfie > cameras. Again, knowing which device is which is important.
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
On 11/12/2018 01:01 AM, Vesa Jääskeläinen wrote: > Hi, > > On 11/11/2018 23.14, Jacek Anaszewski wrote: >> On 11/11/2018 09:16 PM, Pavel Machek wrote: >>> Hi! >>> >> diff --git a/Documentation/leds/leds-class.txt >> b/Documentation/leds/leds-class.txt >> index 836cb16..e9009c4 100644 >> --- a/Documentation/leds/leds-class.txt >> +++ b/Documentation/leds/leds-class.txt >> @@ -43,7 +43,7 @@ LED Device Naming >> Is currently of the form: >> -"devicename:colour:function" >> +"colour:function" > > Couldn't we have here multiple variants that would then be chosen based > on device tree definition? > > "devicename:colour:function" > "devicename:function" > "devicename:label" > "colour:function" > "function" > "label" Documentation/leds/leds-class.txt in a description of "devicename:colour:function" naming scheme states the following: "If sections of the name don't apply, just leave that section blank" Currently not many Device Tree nodes follow that recommendation but with the led_compose_name() I'm going to enforce it (only for the devicename-less naming scheme), which is needed for reliable parsing of LED color and function, which with naming schemes you proposed it wouldn't be possible. But, even with led_compose_name(), it will be possible to get the names listed by you: If you provide the 'label' property in the DT, then with the new led_compose_name() you will get the LED class device name in one of the two below forms, depending on the parameters passed to the function: - If led_hw_name argument is not NULL: led_hw_name:label - If led_hw_name argument is NULL then the label is taken as-is for for LED class device name (it may be appended a numerical suffix if the name is already taken) This flexibility is for backwards compatibility with existing drivers, where some of them add devicename section to the parsed DT label, and others adopts the DT label as-is for LED class device name, relying on DT to provide the devicename. > If "label" would be specified then just use that as a led name, giving > name: > - label Please get acquainted with the entire patch set, the commit messages, and the documentation of led_compose_name(). You'll find out that this is exactly how led_compose_name() is designed. > If "function" would be defined then go to special formatting with > optional "color", giving names: > - color:function > - function Color must not be omitted as explained above. Of course in case 'label' is provided, it won't be validated in any way, thus allowing for any combination. With the 'color' and/or 'function' DT properties you'll get one of the following: - color:function - :function - color: Having considered all the above, I'd even propose to change the delimiter for the new naming convention from ":" to "-", to make it possible to distinguish between old and new naming convention. > I suppose 'devicename' would then be automatically filled based on > driver instance unless one defines 'no-devicename' or something like > that for led definition. devicename section is problematic in the LED class device name, since it doesn't allow for having uniform userspace interface across different platforms. Think of Android - I've seen they have their own LED naming scheme which doesn't contain devicename section. Also, please refer to the DT maintainer's opinion in this regard [0], which actually drew my attention to the problem. Please keep in mind that devicename has been so far used for board vendor name or LED controller name. > Personally I do not see the need for "color" in any led name. In my > opinion the only thing needed here would be location prefix (where > needed -- and it should be possible to disable that) and then logical > name for the led. You mean we don't need the color information at all? And could you please explain what do you mean by "location prefix"? > I don't think we want to do it in all cases. > > So, on my cellphone seeing lp5523:green:led is indeed not useful. > > But on notebook with usb keyboard attached, you need to keep the > devicename to be able to distinguish capslock on internal keyboard and > capslock on first USB keyboard and capslock on second USB keyboard. > > Taking look at the list of functions, here's stuff like "hdd" and > "hdderr". I assume the first means hdd activity... If we can do it, it > would be nicest to have "sda:green:activity" and maybe > "sda:red:error". For a raid array with 8 drives... > > For example I have a router running linux. It has 4 lan ports, with > correspondings LED, and an wan led. > > Having "green:lan1" to "green:lan4" and "green:wan" plus > "red:wanerror" would work, but I'd really preffer > "eth0:green:link"... "adsl0:green:link", "adsl0:red:error". > > There are now phones with flashes on both main and selfie > cameras. Again, knowing which device is which is important.
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
Hi! > >> It's overcomplicating the naming again. In every case you can tweak > >> the function name to eth0_link, eth1_link etc. > > > > Well, but in such case it would be good to keep existing scheme. > > > > My system looks like this: > > > > input16::capslocktpacpi::bay_activetpacpi::standby > > input16::numlock tpacpi::dock_active tpacpi::thinklight > > input16::scrolllock tpacpi::dock_batttpacpi::thinkvantage > > input5::capslock tpacpi::dock_status1 tpacpi::unknown_led > > input5::numlock tpacpi::dock_status2 tpacpi::unknown_led2 > > input5::scrolllock tpacpi:green:batttpacpi::unknown_led3 > > > > I agree that we should get rid of "tpacpi:" part in some cases. But > > it does not make sense to get rid of "input16:" part -- it tells you > > if the LED is on USB or on built-in keyboard. > > > > Ideally, tpacpi::thinklight would be input5::frontlight (as it is > > frontlight for the keyboard). > > > > Yes we should simplify, but it still needs to work in all cases. > > Well, label is not being removed. You still can use it an the old > fashion, even when using new led_compose_name(). > > Maybe removing the description of the old LED naming from > Documentation/leds/leds-class.txt was too drastic move. > I'll keep it next to the new one, and only add a note that > it is kept only for backwards compatibility. I agree that removing it is "just too drastic". But it is not just for backwards compatibility. See my examples above, it is needed to tell which device the LED is asociated with, and it is absolutely required for USB devices (for example). And even for "embedded" stuff like routers, we want eth0:green:link, eth0:yellow:activity and not some kind of hack. Ideally, colors would come from fixed list, functions would come from fixed list, and device part would come from name used elsewhere in the kernel. (And yes, it probably means we should have something in device tree to link LED to its device. device = "name" would be good start...) Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
Hi! > >> It's overcomplicating the naming again. In every case you can tweak > >> the function name to eth0_link, eth1_link etc. > > > > Well, but in such case it would be good to keep existing scheme. > > > > My system looks like this: > > > > input16::capslocktpacpi::bay_activetpacpi::standby > > input16::numlock tpacpi::dock_active tpacpi::thinklight > > input16::scrolllock tpacpi::dock_batttpacpi::thinkvantage > > input5::capslock tpacpi::dock_status1 tpacpi::unknown_led > > input5::numlock tpacpi::dock_status2 tpacpi::unknown_led2 > > input5::scrolllock tpacpi:green:batttpacpi::unknown_led3 > > > > I agree that we should get rid of "tpacpi:" part in some cases. But > > it does not make sense to get rid of "input16:" part -- it tells you > > if the LED is on USB or on built-in keyboard. > > > > Ideally, tpacpi::thinklight would be input5::frontlight (as it is > > frontlight for the keyboard). > > > > Yes we should simplify, but it still needs to work in all cases. > > Well, label is not being removed. You still can use it an the old > fashion, even when using new led_compose_name(). > > Maybe removing the description of the old LED naming from > Documentation/leds/leds-class.txt was too drastic move. > I'll keep it next to the new one, and only add a note that > it is kept only for backwards compatibility. I agree that removing it is "just too drastic". But it is not just for backwards compatibility. See my examples above, it is needed to tell which device the LED is asociated with, and it is absolutely required for USB devices (for example). And even for "embedded" stuff like routers, we want eth0:green:link, eth0:yellow:activity and not some kind of hack. Ideally, colors would come from fixed list, functions would come from fixed list, and device part would come from name used elsewhere in the kernel. (And yes, it probably means we should have something in device tree to link LED to its device. device = "name" would be good start...) Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
Hi, On 11/11/2018 23.14, Jacek Anaszewski wrote: On 11/11/2018 09:16 PM, Pavel Machek wrote: Hi! diff --git a/Documentation/leds/leds-class.txt b/Documentation/leds/leds-class.txt index 836cb16..e9009c4 100644 --- a/Documentation/leds/leds-class.txt +++ b/Documentation/leds/leds-class.txt @@ -43,7 +43,7 @@ LED Device Naming Is currently of the form: -"devicename:colour:function" +"colour:function" Couldn't we have here multiple variants that would then be chosen based on device tree definition? "devicename:colour:function" "devicename:function" "devicename:label" "colour:function" "function" "label" If "label" would be specified then just use that as a led name, giving name: - label If "function" would be defined then go to special formatting with optional "color", giving names: - color:function - function I suppose 'devicename' would then be automatically filled based on driver instance unless one defines 'no-devicename' or something like that for led definition. Personally I do not see the need for "color" in any led name. In my opinion the only thing needed here would be location prefix (where needed -- and it should be possible to disable that) and then logical name for the led. I don't think we want to do it in all cases. So, on my cellphone seeing lp5523:green:led is indeed not useful. But on notebook with usb keyboard attached, you need to keep the devicename to be able to distinguish capslock on internal keyboard and capslock on first USB keyboard and capslock on second USB keyboard. Taking look at the list of functions, here's stuff like "hdd" and "hdderr". I assume the first means hdd activity... If we can do it, it would be nicest to have "sda:green:activity" and maybe "sda:red:error". For a raid array with 8 drives... For example I have a router running linux. It has 4 lan ports, with correspondings LED, and an wan led. Having "green:lan1" to "green:lan4" and "green:wan" plus "red:wanerror" would work, but I'd really preffer "eth0:green:link"... "adsl0:green:link", "adsl0:red:error". There are now phones with flashes on both main and selfie cameras. Again, knowing which device is which is important. As is knowing which display is controlled by particular backlight. It's overcomplicating the naming again. In every case you can tweak the function name to eth0_link, eth1_link etc. Well, but in such case it would be good to keep existing scheme. My system looks like this: input16::capslocktpacpi::bay_activetpacpi::standby input16::numlock tpacpi::dock_active tpacpi::thinklight input16::scrolllock tpacpi::dock_batttpacpi::thinkvantage input5::capslock tpacpi::dock_status1 tpacpi::unknown_led input5::numlock tpacpi::dock_status2 tpacpi::unknown_led2 input5::scrolllock tpacpi:green:batttpacpi::unknown_led3 I agree that we should get rid of "tpacpi:" part in some cases. But it does not make sense to get rid of "input16:" part -- it tells you if the LED is on USB or on built-in keyboard. Ideally, tpacpi::thinklight would be input5::frontlight (as it is frontlight for the keyboard). Yes we should simplify, but it still needs to work in all cases. Well, label is not being removed. You still can use it an the old fashion, even when using new led_compose_name(). Maybe removing the description of the old LED naming from Documentation/leds/leds-class.txt was too drastic move. I'll keep it next to the new one, and only add a note that it is kept only for backwards compatibility. With above proposal for naming it should match more or less everyone's needs? Simple naming for embedded device makers and more advanced for server system makers. No need to say that something is legacy or backwards compatibility feature. Thanks, Vesa Jääskeläinen
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
Hi, On 11/11/2018 23.14, Jacek Anaszewski wrote: On 11/11/2018 09:16 PM, Pavel Machek wrote: Hi! diff --git a/Documentation/leds/leds-class.txt b/Documentation/leds/leds-class.txt index 836cb16..e9009c4 100644 --- a/Documentation/leds/leds-class.txt +++ b/Documentation/leds/leds-class.txt @@ -43,7 +43,7 @@ LED Device Naming Is currently of the form: -"devicename:colour:function" +"colour:function" Couldn't we have here multiple variants that would then be chosen based on device tree definition? "devicename:colour:function" "devicename:function" "devicename:label" "colour:function" "function" "label" If "label" would be specified then just use that as a led name, giving name: - label If "function" would be defined then go to special formatting with optional "color", giving names: - color:function - function I suppose 'devicename' would then be automatically filled based on driver instance unless one defines 'no-devicename' or something like that for led definition. Personally I do not see the need for "color" in any led name. In my opinion the only thing needed here would be location prefix (where needed -- and it should be possible to disable that) and then logical name for the led. I don't think we want to do it in all cases. So, on my cellphone seeing lp5523:green:led is indeed not useful. But on notebook with usb keyboard attached, you need to keep the devicename to be able to distinguish capslock on internal keyboard and capslock on first USB keyboard and capslock on second USB keyboard. Taking look at the list of functions, here's stuff like "hdd" and "hdderr". I assume the first means hdd activity... If we can do it, it would be nicest to have "sda:green:activity" and maybe "sda:red:error". For a raid array with 8 drives... For example I have a router running linux. It has 4 lan ports, with correspondings LED, and an wan led. Having "green:lan1" to "green:lan4" and "green:wan" plus "red:wanerror" would work, but I'd really preffer "eth0:green:link"... "adsl0:green:link", "adsl0:red:error". There are now phones with flashes on both main and selfie cameras. Again, knowing which device is which is important. As is knowing which display is controlled by particular backlight. It's overcomplicating the naming again. In every case you can tweak the function name to eth0_link, eth1_link etc. Well, but in such case it would be good to keep existing scheme. My system looks like this: input16::capslocktpacpi::bay_activetpacpi::standby input16::numlock tpacpi::dock_active tpacpi::thinklight input16::scrolllock tpacpi::dock_batttpacpi::thinkvantage input5::capslock tpacpi::dock_status1 tpacpi::unknown_led input5::numlock tpacpi::dock_status2 tpacpi::unknown_led2 input5::scrolllock tpacpi:green:batttpacpi::unknown_led3 I agree that we should get rid of "tpacpi:" part in some cases. But it does not make sense to get rid of "input16:" part -- it tells you if the LED is on USB or on built-in keyboard. Ideally, tpacpi::thinklight would be input5::frontlight (as it is frontlight for the keyboard). Yes we should simplify, but it still needs to work in all cases. Well, label is not being removed. You still can use it an the old fashion, even when using new led_compose_name(). Maybe removing the description of the old LED naming from Documentation/leds/leds-class.txt was too drastic move. I'll keep it next to the new one, and only add a note that it is kept only for backwards compatibility. With above proposal for naming it should match more or less everyone's needs? Simple naming for embedded device makers and more advanced for server system makers. No need to say that something is legacy or backwards compatibility feature. Thanks, Vesa Jääskeläinen
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
On 11/11/2018 09:16 PM, Pavel Machek wrote: > Hi! > diff --git a/Documentation/leds/leds-class.txt b/Documentation/leds/leds-class.txt index 836cb16..e9009c4 100644 --- a/Documentation/leds/leds-class.txt +++ b/Documentation/leds/leds-class.txt @@ -43,7 +43,7 @@ LED Device Naming Is currently of the form: -"devicename:colour:function" +"colour:function" >>> >>> I don't think we want to do it in all cases. >>> >>> So, on my cellphone seeing lp5523:green:led is indeed not useful. >>> >>> But on notebook with usb keyboard attached, you need to keep the >>> devicename to be able to distinguish capslock on internal keyboard and >>> capslock on first USB keyboard and capslock on second USB keyboard. >>> >>> Taking look at the list of functions, here's stuff like "hdd" and >>> "hdderr". I assume the first means hdd activity... If we can do it, it >>> would be nicest to have "sda:green:activity" and maybe >>> "sda:red:error". For a raid array with 8 drives... >>> >>> For example I have a router running linux. It has 4 lan ports, with >>> correspondings LED, and an wan led. >>> >>> Having "green:lan1" to "green:lan4" and "green:wan" plus >>> "red:wanerror" would work, but I'd really preffer >>> "eth0:green:link"... "adsl0:green:link", "adsl0:red:error". >>> >>> There are now phones with flashes on both main and selfie >>> cameras. Again, knowing which device is which is important. As is >>> knowing which display is controlled by particular backlight. >> >> It's overcomplicating the naming again. In every case you can tweak >> the function name to eth0_link, eth1_link etc. > > Well, but in such case it would be good to keep existing scheme. > > My system looks like this: > > input16::capslocktpacpi::bay_activetpacpi::standby > input16::numlock tpacpi::dock_active tpacpi::thinklight > input16::scrolllock tpacpi::dock_batt tpacpi::thinkvantage > input5::capslock tpacpi::dock_status1 tpacpi::unknown_led > input5::numlock tpacpi::dock_status2 tpacpi::unknown_led2 > input5::scrolllock tpacpi:green:batt tpacpi::unknown_led3 > > I agree that we should get rid of "tpacpi:" part in some cases. But > it does not make sense to get rid of "input16:" part -- it tells you > if the LED is on USB or on built-in keyboard. > > Ideally, tpacpi::thinklight would be input5::frontlight (as it is > frontlight for the keyboard). > > Yes we should simplify, but it still needs to work in all cases. Well, label is not being removed. You still can use it an the old fashion, even when using new led_compose_name(). Maybe removing the description of the old LED naming from Documentation/leds/leds-class.txt was too drastic move. I'll keep it next to the new one, and only add a note that it is kept only for backwards compatibility. -- Best regards, Jacek Anaszewski
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
On 11/11/2018 09:16 PM, Pavel Machek wrote: > Hi! > diff --git a/Documentation/leds/leds-class.txt b/Documentation/leds/leds-class.txt index 836cb16..e9009c4 100644 --- a/Documentation/leds/leds-class.txt +++ b/Documentation/leds/leds-class.txt @@ -43,7 +43,7 @@ LED Device Naming Is currently of the form: -"devicename:colour:function" +"colour:function" >>> >>> I don't think we want to do it in all cases. >>> >>> So, on my cellphone seeing lp5523:green:led is indeed not useful. >>> >>> But on notebook with usb keyboard attached, you need to keep the >>> devicename to be able to distinguish capslock on internal keyboard and >>> capslock on first USB keyboard and capslock on second USB keyboard. >>> >>> Taking look at the list of functions, here's stuff like "hdd" and >>> "hdderr". I assume the first means hdd activity... If we can do it, it >>> would be nicest to have "sda:green:activity" and maybe >>> "sda:red:error". For a raid array with 8 drives... >>> >>> For example I have a router running linux. It has 4 lan ports, with >>> correspondings LED, and an wan led. >>> >>> Having "green:lan1" to "green:lan4" and "green:wan" plus >>> "red:wanerror" would work, but I'd really preffer >>> "eth0:green:link"... "adsl0:green:link", "adsl0:red:error". >>> >>> There are now phones with flashes on both main and selfie >>> cameras. Again, knowing which device is which is important. As is >>> knowing which display is controlled by particular backlight. >> >> It's overcomplicating the naming again. In every case you can tweak >> the function name to eth0_link, eth1_link etc. > > Well, but in such case it would be good to keep existing scheme. > > My system looks like this: > > input16::capslocktpacpi::bay_activetpacpi::standby > input16::numlock tpacpi::dock_active tpacpi::thinklight > input16::scrolllock tpacpi::dock_batt tpacpi::thinkvantage > input5::capslock tpacpi::dock_status1 tpacpi::unknown_led > input5::numlock tpacpi::dock_status2 tpacpi::unknown_led2 > input5::scrolllock tpacpi:green:batt tpacpi::unknown_led3 > > I agree that we should get rid of "tpacpi:" part in some cases. But > it does not make sense to get rid of "input16:" part -- it tells you > if the LED is on USB or on built-in keyboard. > > Ideally, tpacpi::thinklight would be input5::frontlight (as it is > frontlight for the keyboard). > > Yes we should simplify, but it still needs to work in all cases. Well, label is not being removed. You still can use it an the old fashion, even when using new led_compose_name(). Maybe removing the description of the old LED naming from Documentation/leds/leds-class.txt was too drastic move. I'll keep it next to the new one, and only add a note that it is kept only for backwards compatibility. -- Best regards, Jacek Anaszewski
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
Hi! > >> diff --git a/Documentation/leds/leds-class.txt > >> b/Documentation/leds/leds-class.txt > >> index 836cb16..e9009c4 100644 > >> --- a/Documentation/leds/leds-class.txt > >> +++ b/Documentation/leds/leds-class.txt > >> @@ -43,7 +43,7 @@ LED Device Naming > >> > >> Is currently of the form: > >> > >> -"devicename:colour:function" > >> +"colour:function" > >> > > > > I don't think we want to do it in all cases. > > > > So, on my cellphone seeing lp5523:green:led is indeed not useful. > > > > But on notebook with usb keyboard attached, you need to keep the > > devicename to be able to distinguish capslock on internal keyboard and > > capslock on first USB keyboard and capslock on second USB keyboard. > > > > Taking look at the list of functions, here's stuff like "hdd" and > > "hdderr". I assume the first means hdd activity... If we can do it, it > > would be nicest to have "sda:green:activity" and maybe > > "sda:red:error". For a raid array with 8 drives... > > > > For example I have a router running linux. It has 4 lan ports, with > > correspondings LED, and an wan led. > > > > Having "green:lan1" to "green:lan4" and "green:wan" plus > > "red:wanerror" would work, but I'd really preffer > > "eth0:green:link"... "adsl0:green:link", "adsl0:red:error". > > > > There are now phones with flashes on both main and selfie > > cameras. Again, knowing which device is which is important. As is > > knowing which display is controlled by particular backlight. > > It's overcomplicating the naming again. In every case you can tweak > the function name to eth0_link, eth1_link etc. Well, but in such case it would be good to keep existing scheme. My system looks like this: input16::capslocktpacpi::bay_activetpacpi::standby input16::numlock tpacpi::dock_active tpacpi::thinklight input16::scrolllock tpacpi::dock_batttpacpi::thinkvantage input5::capslock tpacpi::dock_status1 tpacpi::unknown_led input5::numlock tpacpi::dock_status2 tpacpi::unknown_led2 input5::scrolllock tpacpi:green:batttpacpi::unknown_led3 I agree that we should get rid of "tpacpi:" part in some cases. But it does not make sense to get rid of "input16:" part -- it tells you if the LED is on USB or on built-in keyboard. Ideally, tpacpi::thinklight would be input5::frontlight (as it is frontlight for the keyboard). Yes we should simplify, but it still needs to work in all cases. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
Hi! > >> diff --git a/Documentation/leds/leds-class.txt > >> b/Documentation/leds/leds-class.txt > >> index 836cb16..e9009c4 100644 > >> --- a/Documentation/leds/leds-class.txt > >> +++ b/Documentation/leds/leds-class.txt > >> @@ -43,7 +43,7 @@ LED Device Naming > >> > >> Is currently of the form: > >> > >> -"devicename:colour:function" > >> +"colour:function" > >> > > > > I don't think we want to do it in all cases. > > > > So, on my cellphone seeing lp5523:green:led is indeed not useful. > > > > But on notebook with usb keyboard attached, you need to keep the > > devicename to be able to distinguish capslock on internal keyboard and > > capslock on first USB keyboard and capslock on second USB keyboard. > > > > Taking look at the list of functions, here's stuff like "hdd" and > > "hdderr". I assume the first means hdd activity... If we can do it, it > > would be nicest to have "sda:green:activity" and maybe > > "sda:red:error". For a raid array with 8 drives... > > > > For example I have a router running linux. It has 4 lan ports, with > > correspondings LED, and an wan led. > > > > Having "green:lan1" to "green:lan4" and "green:wan" plus > > "red:wanerror" would work, but I'd really preffer > > "eth0:green:link"... "adsl0:green:link", "adsl0:red:error". > > > > There are now phones with flashes on both main and selfie > > cameras. Again, knowing which device is which is important. As is > > knowing which display is controlled by particular backlight. > > It's overcomplicating the naming again. In every case you can tweak > the function name to eth0_link, eth1_link etc. Well, but in such case it would be good to keep existing scheme. My system looks like this: input16::capslocktpacpi::bay_activetpacpi::standby input16::numlock tpacpi::dock_active tpacpi::thinklight input16::scrolllock tpacpi::dock_batttpacpi::thinkvantage input5::capslock tpacpi::dock_status1 tpacpi::unknown_led input5::numlock tpacpi::dock_status2 tpacpi::unknown_led2 input5::scrolllock tpacpi:green:batttpacpi::unknown_led3 I agree that we should get rid of "tpacpi:" part in some cases. But it does not make sense to get rid of "input16:" part -- it tells you if the LED is on USB or on built-in keyboard. Ideally, tpacpi::thinklight would be input5::frontlight (as it is frontlight for the keyboard). Yes we should simplify, but it still needs to work in all cases. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
Hi Pavel, On 11/11/2018 01:02 PM, Pavel Machek wrote: > Hi! > >> Add public led_compose_name() API for composing LED class device >> name basing on fwnode_handle data. The function composes device name >> according to either a new pattern or the legacy >> pattern. The decision on using the >> particular pattern is made basing on whether fwnode contains new >> "function" and "color" properties, or the legacy "label" proeprty. >> >> Backwards compatibility with in-driver hard-coded LED class device >> names is assured thanks to the default_desc argument. >> >> In case none of the aformentioned properties was found, then, for OF >> nodes, the node name is adopted for LED class device name. >> > >> diff --git a/Documentation/leds/leds-class.txt >> b/Documentation/leds/leds-class.txt >> index 836cb16..e9009c4 100644 >> --- a/Documentation/leds/leds-class.txt >> +++ b/Documentation/leds/leds-class.txt >> @@ -43,7 +43,7 @@ LED Device Naming >> >> Is currently of the form: >> >> -"devicename:colour:function" >> +"colour:function" >> > > I don't think we want to do it in all cases. > > So, on my cellphone seeing lp5523:green:led is indeed not useful. > > But on notebook with usb keyboard attached, you need to keep the > devicename to be able to distinguish capslock on internal keyboard and > capslock on first USB keyboard and capslock on second USB keyboard. > > Taking look at the list of functions, here's stuff like "hdd" and > "hdderr". I assume the first means hdd activity... If we can do it, it > would be nicest to have "sda:green:activity" and maybe > "sda:red:error". For a raid array with 8 drives... > > For example I have a router running linux. It has 4 lan ports, with > correspondings LED, and an wan led. > > Having "green:lan1" to "green:lan4" and "green:wan" plus > "red:wanerror" would work, but I'd really preffer > "eth0:green:link"... "adsl0:green:link", "adsl0:red:error". > > There are now phones with flashes on both main and selfie > cameras. Again, knowing which device is which is important. As is > knowing which display is controlled by particular backlight. It's overcomplicating the naming again. In every case you can tweak the function name to eth0_link, eth1_link etc. -- Best regards, Jacek Anaszewski
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
Hi Pavel, On 11/11/2018 01:02 PM, Pavel Machek wrote: > Hi! > >> Add public led_compose_name() API for composing LED class device >> name basing on fwnode_handle data. The function composes device name >> according to either a new pattern or the legacy >> pattern. The decision on using the >> particular pattern is made basing on whether fwnode contains new >> "function" and "color" properties, or the legacy "label" proeprty. >> >> Backwards compatibility with in-driver hard-coded LED class device >> names is assured thanks to the default_desc argument. >> >> In case none of the aformentioned properties was found, then, for OF >> nodes, the node name is adopted for LED class device name. >> > >> diff --git a/Documentation/leds/leds-class.txt >> b/Documentation/leds/leds-class.txt >> index 836cb16..e9009c4 100644 >> --- a/Documentation/leds/leds-class.txt >> +++ b/Documentation/leds/leds-class.txt >> @@ -43,7 +43,7 @@ LED Device Naming >> >> Is currently of the form: >> >> -"devicename:colour:function" >> +"colour:function" >> > > I don't think we want to do it in all cases. > > So, on my cellphone seeing lp5523:green:led is indeed not useful. > > But on notebook with usb keyboard attached, you need to keep the > devicename to be able to distinguish capslock on internal keyboard and > capslock on first USB keyboard and capslock on second USB keyboard. > > Taking look at the list of functions, here's stuff like "hdd" and > "hdderr". I assume the first means hdd activity... If we can do it, it > would be nicest to have "sda:green:activity" and maybe > "sda:red:error". For a raid array with 8 drives... > > For example I have a router running linux. It has 4 lan ports, with > correspondings LED, and an wan led. > > Having "green:lan1" to "green:lan4" and "green:wan" plus > "red:wanerror" would work, but I'd really preffer > "eth0:green:link"... "adsl0:green:link", "adsl0:red:error". > > There are now phones with flashes on both main and selfie > cameras. Again, knowing which device is which is important. As is > knowing which display is controlled by particular backlight. It's overcomplicating the naming again. In every case you can tweak the function name to eth0_link, eth1_link etc. -- Best regards, Jacek Anaszewski
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
Hi! > Add public led_compose_name() API for composing LED class device > name basing on fwnode_handle data. The function composes device name > according to either a new pattern or the legacy > pattern. The decision on using the > particular pattern is made basing on whether fwnode contains new > "function" and "color" properties, or the legacy "label" proeprty. > > Backwards compatibility with in-driver hard-coded LED class device > names is assured thanks to the default_desc argument. > > In case none of the aformentioned properties was found, then, for OF > nodes, the node name is adopted for LED class device name. > > diff --git a/Documentation/leds/leds-class.txt > b/Documentation/leds/leds-class.txt > index 836cb16..e9009c4 100644 > --- a/Documentation/leds/leds-class.txt > +++ b/Documentation/leds/leds-class.txt > @@ -43,7 +43,7 @@ LED Device Naming > > Is currently of the form: > > -"devicename:colour:function" > +"colour:function" > I don't think we want to do it in all cases. So, on my cellphone seeing lp5523:green:led is indeed not useful. But on notebook with usb keyboard attached, you need to keep the devicename to be able to distinguish capslock on internal keyboard and capslock on first USB keyboard and capslock on second USB keyboard. Taking look at the list of functions, here's stuff like "hdd" and "hdderr". I assume the first means hdd activity... If we can do it, it would be nicest to have "sda:green:activity" and maybe "sda:red:error". For a raid array with 8 drives... For example I have a router running linux. It has 4 lan ports, with correspondings LED, and an wan led. Having "green:lan1" to "green:lan4" and "green:wan" plus "red:wanerror" would work, but I'd really preffer "eth0:green:link"... "adsl0:green:link", "adsl0:red:error". There are now phones with flashes on both main and selfie cameras. Again, knowing which device is which is important. As is knowing which display is controlled by particular backlight. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
Hi! > Add public led_compose_name() API for composing LED class device > name basing on fwnode_handle data. The function composes device name > according to either a new pattern or the legacy > pattern. The decision on using the > particular pattern is made basing on whether fwnode contains new > "function" and "color" properties, or the legacy "label" proeprty. > > Backwards compatibility with in-driver hard-coded LED class device > names is assured thanks to the default_desc argument. > > In case none of the aformentioned properties was found, then, for OF > nodes, the node name is adopted for LED class device name. > > diff --git a/Documentation/leds/leds-class.txt > b/Documentation/leds/leds-class.txt > index 836cb16..e9009c4 100644 > --- a/Documentation/leds/leds-class.txt > +++ b/Documentation/leds/leds-class.txt > @@ -43,7 +43,7 @@ LED Device Naming > > Is currently of the form: > > -"devicename:colour:function" > +"colour:function" > I don't think we want to do it in all cases. So, on my cellphone seeing lp5523:green:led is indeed not useful. But on notebook with usb keyboard attached, you need to keep the devicename to be able to distinguish capslock on internal keyboard and capslock on first USB keyboard and capslock on second USB keyboard. Taking look at the list of functions, here's stuff like "hdd" and "hdderr". I assume the first means hdd activity... If we can do it, it would be nicest to have "sda:green:activity" and maybe "sda:red:error". For a raid array with 8 drives... For example I have a router running linux. It has 4 lan ports, with correspondings LED, and an wan led. Having "green:lan1" to "green:lan4" and "green:wan" plus "red:wanerror" would work, but I'd really preffer "eth0:green:link"... "adsl0:green:link", "adsl0:red:error". There are now phones with flashes on both main and selfie cameras. Again, knowing which device is which is important. As is knowing which display is controlled by particular backlight. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
Hi Jacek,
On 9 November 2018 at 04:47, Jacek Anaszewski
wrote:
> Hi Baolin,
>
> Thanks for the review.
>
> On 11/07/2018 08:20 AM, Baolin Wang wrote:
>> Hi Jacek,
>>
>> On 7 November 2018 at 06:07, Jacek Anaszewski
>> wrote:
>>> Add public led_compose_name() API for composing LED class device
>>> name basing on fwnode_handle data. The function composes device name
>>> according to either a new pattern or the legacy
>>> pattern. The decision on using the
>>> particular pattern is made basing on whether fwnode contains new
>>> "function" and "color" properties, or the legacy "label" proeprty.
>>>
>>> Backwards compatibility with in-driver hard-coded LED class device
>>> names is assured thanks to the default_desc argument.
>>>
>>> In case none of the aformentioned properties was found, then, for OF
>>> nodes, the node name is adopted for LED class device name.
>>>
>>> Signed-off-by: Jacek Anaszewski
>>> Cc: Baolin Wang
>>> Cc: Daniel Mack
>>> Cc: Dan Murphy
>>> Cc: Linus Walleij
>>> Cc: Oleh Kravchenko
>>> Cc: Sakari Ailus
>>> Cc: Simon Shields
>>> Cc: Xiaotong Lu
>>> ---
>>> Documentation/leds/leds-class.txt | 2 +-
>>> drivers/leds/led-core.c | 71
>>> +++
>>> include/linux/leds.h | 32 ++
>>> 3 files changed, 104 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/leds/leds-class.txt
>>> b/Documentation/leds/leds-class.txt
>>> index 836cb16..e9009c4 100644
>>> --- a/Documentation/leds/leds-class.txt
>>> +++ b/Documentation/leds/leds-class.txt
>>> @@ -43,7 +43,7 @@ LED Device Naming
>>>
>>> Is currently of the form:
>>>
>>> -"devicename:colour:function"
>>> +"colour:function"
>>>
>>> There have been calls for LED properties such as colour to be exported as
>>> individual led class attributes. As a solution which doesn't incur as much
>>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
>>> index ede4fa0..f7371fc 100644
>>> --- a/drivers/leds/led-core.c
>>> +++ b/drivers/leds/led-core.c
>>> @@ -16,6 +16,8 @@
>>> #include
>>> #include
>>> #include
>>> +#include
>>> +#include
>>> #include
>>> #include "leds.h"
>>>
>>> @@ -327,3 +329,72 @@ void led_sysfs_enable(struct led_classdev *led_cdev)
>>> led_cdev->flags &= ~LED_SYSFS_DISABLE;
>>> }
>>> EXPORT_SYMBOL_GPL(led_sysfs_enable);
>>> +
>>> +static void led_parse_properties(struct fwnode_handle *fwnode,
>>> +struct led_properties *props)
>>> +{
>>> + int ret;
>>> +
>>> + if (!fwnode)
>>> + return;
>>> +
>>> + ret = fwnode_property_read_string(fwnode, "label", >label);
>>> + if (!ret)
>>> + return;
>>> +
>>> + ret = fwnode_property_read_string(fwnode, "function",
>>> >function);
>>> + if (ret)
>>> + pr_info("Failed to parse function property\n");
>>> +
>>> + ret = fwnode_property_read_string(fwnode, "color", >color);
>>> + if (ret)
>>> + pr_info("Failed to parse color property\n");
>>
>> Now the color and function properties can be optional, so we should
>> deal with different return errors.
>> -EINVAL means we have not set color or function property, which means
>> we should not give failure message for users (maybe "not supply color
>> property\n") and return 0 as success.
>
> The lack of any of these properties is not critical, therefore this
> function does not return the error code, but only prints the status.
> Please note that I'm not using pr_error(), but pr_info().
> Related to the message text - I agree, I will change it to e.g.
> "color property not found"
Yes, that's reasonable.
>
>> For other errors, we should not ignore them, instead we should give
>> failure messages and return errors.
>
> I've skimmed through other kernel drivers and vast majority of
> them don't check exact failure code in case of non-zero return,
> but assume that property is missing, and eventually report
> error code. I'd do the same here.
Fair enough. You can still add my tested tag. Thanks.
>> Tested-by: Baolin Wang
>>
>>> +}
>>> +
>>> +int led_compose_name(struct fwnode_handle *fwnode, const char *led_hw_name,
>>> +const char *default_desc, char *led_classdev_name)
>>> +{
>>> + struct led_properties props = {};
>>> +
>>> + if (!led_classdev_name)
>>> + return -EINVAL;
>>> +
>>> + led_parse_properties(fwnode, );
>>> +
>>> + if (props.label) {
>>> + /*
>>> +* Presence of 'label' DT property implies legacy LED name,
>>> +* formatted as , with possible
>>> +* section omission if doesn't apply to given device.
>>> +*
>>> +* If no led_hw_name has been passed, then it indicates that
>>> +* DT label should be used as-is for LED class device name.
>>> +* Otherwise the label is prepended with led_hw_name to
>>> compose
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
Hi Jacek,
On 9 November 2018 at 04:47, Jacek Anaszewski
wrote:
> Hi Baolin,
>
> Thanks for the review.
>
> On 11/07/2018 08:20 AM, Baolin Wang wrote:
>> Hi Jacek,
>>
>> On 7 November 2018 at 06:07, Jacek Anaszewski
>> wrote:
>>> Add public led_compose_name() API for composing LED class device
>>> name basing on fwnode_handle data. The function composes device name
>>> according to either a new pattern or the legacy
>>> pattern. The decision on using the
>>> particular pattern is made basing on whether fwnode contains new
>>> "function" and "color" properties, or the legacy "label" proeprty.
>>>
>>> Backwards compatibility with in-driver hard-coded LED class device
>>> names is assured thanks to the default_desc argument.
>>>
>>> In case none of the aformentioned properties was found, then, for OF
>>> nodes, the node name is adopted for LED class device name.
>>>
>>> Signed-off-by: Jacek Anaszewski
>>> Cc: Baolin Wang
>>> Cc: Daniel Mack
>>> Cc: Dan Murphy
>>> Cc: Linus Walleij
>>> Cc: Oleh Kravchenko
>>> Cc: Sakari Ailus
>>> Cc: Simon Shields
>>> Cc: Xiaotong Lu
>>> ---
>>> Documentation/leds/leds-class.txt | 2 +-
>>> drivers/leds/led-core.c | 71
>>> +++
>>> include/linux/leds.h | 32 ++
>>> 3 files changed, 104 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/leds/leds-class.txt
>>> b/Documentation/leds/leds-class.txt
>>> index 836cb16..e9009c4 100644
>>> --- a/Documentation/leds/leds-class.txt
>>> +++ b/Documentation/leds/leds-class.txt
>>> @@ -43,7 +43,7 @@ LED Device Naming
>>>
>>> Is currently of the form:
>>>
>>> -"devicename:colour:function"
>>> +"colour:function"
>>>
>>> There have been calls for LED properties such as colour to be exported as
>>> individual led class attributes. As a solution which doesn't incur as much
>>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
>>> index ede4fa0..f7371fc 100644
>>> --- a/drivers/leds/led-core.c
>>> +++ b/drivers/leds/led-core.c
>>> @@ -16,6 +16,8 @@
>>> #include
>>> #include
>>> #include
>>> +#include
>>> +#include
>>> #include
>>> #include "leds.h"
>>>
>>> @@ -327,3 +329,72 @@ void led_sysfs_enable(struct led_classdev *led_cdev)
>>> led_cdev->flags &= ~LED_SYSFS_DISABLE;
>>> }
>>> EXPORT_SYMBOL_GPL(led_sysfs_enable);
>>> +
>>> +static void led_parse_properties(struct fwnode_handle *fwnode,
>>> +struct led_properties *props)
>>> +{
>>> + int ret;
>>> +
>>> + if (!fwnode)
>>> + return;
>>> +
>>> + ret = fwnode_property_read_string(fwnode, "label", >label);
>>> + if (!ret)
>>> + return;
>>> +
>>> + ret = fwnode_property_read_string(fwnode, "function",
>>> >function);
>>> + if (ret)
>>> + pr_info("Failed to parse function property\n");
>>> +
>>> + ret = fwnode_property_read_string(fwnode, "color", >color);
>>> + if (ret)
>>> + pr_info("Failed to parse color property\n");
>>
>> Now the color and function properties can be optional, so we should
>> deal with different return errors.
>> -EINVAL means we have not set color or function property, which means
>> we should not give failure message for users (maybe "not supply color
>> property\n") and return 0 as success.
>
> The lack of any of these properties is not critical, therefore this
> function does not return the error code, but only prints the status.
> Please note that I'm not using pr_error(), but pr_info().
> Related to the message text - I agree, I will change it to e.g.
> "color property not found"
Yes, that's reasonable.
>
>> For other errors, we should not ignore them, instead we should give
>> failure messages and return errors.
>
> I've skimmed through other kernel drivers and vast majority of
> them don't check exact failure code in case of non-zero return,
> but assume that property is missing, and eventually report
> error code. I'd do the same here.
Fair enough. You can still add my tested tag. Thanks.
>> Tested-by: Baolin Wang
>>
>>> +}
>>> +
>>> +int led_compose_name(struct fwnode_handle *fwnode, const char *led_hw_name,
>>> +const char *default_desc, char *led_classdev_name)
>>> +{
>>> + struct led_properties props = {};
>>> +
>>> + if (!led_classdev_name)
>>> + return -EINVAL;
>>> +
>>> + led_parse_properties(fwnode, );
>>> +
>>> + if (props.label) {
>>> + /*
>>> +* Presence of 'label' DT property implies legacy LED name,
>>> +* formatted as , with possible
>>> +* section omission if doesn't apply to given device.
>>> +*
>>> +* If no led_hw_name has been passed, then it indicates that
>>> +* DT label should be used as-is for LED class device name.
>>> +* Otherwise the label is prepended with led_hw_name to
>>> compose
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
Dan,
On 11/08/2018 07:06 PM, Dan Murphy wrote:
> On 11/06/2018 04:07 PM, Jacek Anaszewski wrote:
>> Add public led_compose_name() API for composing LED class device
>> name basing on fwnode_handle data. The function composes device name
>> according to either a new pattern or the legacy
>> pattern. The decision on using the
>> particular pattern is made basing on whether fwnode contains new
>> "function" and "color" properties, or the legacy "label" proeprty.
>>
>> Backwards compatibility with in-driver hard-coded LED class device
>> names is assured thanks to the default_desc argument.
>>
>> In case none of the aformentioned properties was found, then, for OF
>> nodes, the node name is adopted for LED class device name.
>>
>> Signed-off-by: Jacek Anaszewski
>> Cc: Baolin Wang
>> Cc: Daniel Mack
>> Cc: Dan Murphy
>> Cc: Linus Walleij
>> Cc: Oleh Kravchenko
>> Cc: Sakari Ailus
>> Cc: Simon Shields
>> Cc: Xiaotong Lu
>> ---
>> Documentation/leds/leds-class.txt | 2 +-
>> drivers/leds/led-core.c | 71
>> +++
>> include/linux/leds.h | 32 ++
>> 3 files changed, 104 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/leds/leds-class.txt
>> b/Documentation/leds/leds-class.txt
>> index 836cb16..e9009c4 100644
>> --- a/Documentation/leds/leds-class.txt
>> +++ b/Documentation/leds/leds-class.txt
>> @@ -43,7 +43,7 @@ LED Device Naming
>>
>> Is currently of the form:
>>
>> -"devicename:colour:function"
>> +"colour:function"
>>
>> There have been calls for LED properties such as colour to be exported as
>> individual led class attributes. As a solution which doesn't incur as much
>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
>> index ede4fa0..f7371fc 100644
>> --- a/drivers/leds/led-core.c
>> +++ b/drivers/leds/led-core.c
>> @@ -16,6 +16,8 @@
>> #include
>> #include
>> #include
>> +#include
>> +#include
>> #include
>> #include "leds.h"
>>
>> @@ -327,3 +329,72 @@ void led_sysfs_enable(struct led_classdev *led_cdev)
>> led_cdev->flags &= ~LED_SYSFS_DISABLE;
>> }
>> EXPORT_SYMBOL_GPL(led_sysfs_enable);
>> +
>> +static void led_parse_properties(struct fwnode_handle *fwnode,
>> + struct led_properties *props)
>> +{
>> +int ret;
>> +
>> +if (!fwnode)
>> +return;
>> +
>> +ret = fwnode_property_read_string(fwnode, "label", >label);
>
> If we have the label do we need to continue to look for the function and
> color nodes?
>
> It can be checked and diverted using fwnode_property_present for the "label"
> property
Good catch. I will respin it as proposed.
>> +if (!ret)
>> +return;
>> +
>> +ret = fwnode_property_read_string(fwnode, "function", >function);
>> +if (ret)
>> +pr_info("Failed to parse function property\n");
>> +
>> +ret = fwnode_property_read_string(fwnode, "color", >color);
>> +if (ret)
>> +pr_info("Failed to parse color property\n");
>> +}
>> +
>> +int led_compose_name(struct fwnode_handle *fwnode, const char *led_hw_name,
>> + const char *default_desc, char *led_classdev_name)
>> +{
>> +struct led_properties props = {};
>> +
>> +if (!led_classdev_name)
>> +return -EINVAL;
>> +
>> +led_parse_properties(fwnode, );
>> +
>> +if (props.label) {
>> +/*
>> + * Presence of 'label' DT property implies legacy LED name,
>> + * formatted as , with possible
>> + * section omission if doesn't apply to given device.
>> + *
>> + * If no led_hw_name has been passed, then it indicates that
>> + * DT label should be used as-is for LED class device name.
>> + * Otherwise the label is prepended with led_hw_name to compose
>> + * the final LED class device name.
>> + */
>> +if (!led_hw_name) {
>> +strncpy(led_classdev_name, props.label,
>> +LED_MAX_NAME_SIZE);
>> +} else {
>> +snprintf(led_classdev_name, LED_MAX_NAME_SIZE, "%s:%s",
>> + led_hw_name, props.label);
>> +}
>> +} else if (props.function || props.color) {
>> +snprintf(led_classdev_name, LED_MAX_NAME_SIZE, "%s:%s",
>> + props.color ?: "", props.function ?: "");
>> +} else if (default_desc) {
>> +if (!led_hw_name) {
>> +pr_err("Legacy LED naming requires devicename segment");
>> +return -EINVAL;
>> +}
>> +snprintf(led_classdev_name, LED_MAX_NAME_SIZE, "%s:%s",
>> + led_hw_name, default_desc);
>> +} else if (is_of_node(fwnode)) {
>> +strncpy(led_classdev_name, to_of_node(fwnode)->name,
>> +LED_MAX_NAME_SIZE);
>> +} else
>> +return -EINVAL;
>>
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
Dan,
On 11/08/2018 07:06 PM, Dan Murphy wrote:
> On 11/06/2018 04:07 PM, Jacek Anaszewski wrote:
>> Add public led_compose_name() API for composing LED class device
>> name basing on fwnode_handle data. The function composes device name
>> according to either a new pattern or the legacy
>> pattern. The decision on using the
>> particular pattern is made basing on whether fwnode contains new
>> "function" and "color" properties, or the legacy "label" proeprty.
>>
>> Backwards compatibility with in-driver hard-coded LED class device
>> names is assured thanks to the default_desc argument.
>>
>> In case none of the aformentioned properties was found, then, for OF
>> nodes, the node name is adopted for LED class device name.
>>
>> Signed-off-by: Jacek Anaszewski
>> Cc: Baolin Wang
>> Cc: Daniel Mack
>> Cc: Dan Murphy
>> Cc: Linus Walleij
>> Cc: Oleh Kravchenko
>> Cc: Sakari Ailus
>> Cc: Simon Shields
>> Cc: Xiaotong Lu
>> ---
>> Documentation/leds/leds-class.txt | 2 +-
>> drivers/leds/led-core.c | 71
>> +++
>> include/linux/leds.h | 32 ++
>> 3 files changed, 104 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/leds/leds-class.txt
>> b/Documentation/leds/leds-class.txt
>> index 836cb16..e9009c4 100644
>> --- a/Documentation/leds/leds-class.txt
>> +++ b/Documentation/leds/leds-class.txt
>> @@ -43,7 +43,7 @@ LED Device Naming
>>
>> Is currently of the form:
>>
>> -"devicename:colour:function"
>> +"colour:function"
>>
>> There have been calls for LED properties such as colour to be exported as
>> individual led class attributes. As a solution which doesn't incur as much
>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
>> index ede4fa0..f7371fc 100644
>> --- a/drivers/leds/led-core.c
>> +++ b/drivers/leds/led-core.c
>> @@ -16,6 +16,8 @@
>> #include
>> #include
>> #include
>> +#include
>> +#include
>> #include
>> #include "leds.h"
>>
>> @@ -327,3 +329,72 @@ void led_sysfs_enable(struct led_classdev *led_cdev)
>> led_cdev->flags &= ~LED_SYSFS_DISABLE;
>> }
>> EXPORT_SYMBOL_GPL(led_sysfs_enable);
>> +
>> +static void led_parse_properties(struct fwnode_handle *fwnode,
>> + struct led_properties *props)
>> +{
>> +int ret;
>> +
>> +if (!fwnode)
>> +return;
>> +
>> +ret = fwnode_property_read_string(fwnode, "label", >label);
>
> If we have the label do we need to continue to look for the function and
> color nodes?
>
> It can be checked and diverted using fwnode_property_present for the "label"
> property
Good catch. I will respin it as proposed.
>> +if (!ret)
>> +return;
>> +
>> +ret = fwnode_property_read_string(fwnode, "function", >function);
>> +if (ret)
>> +pr_info("Failed to parse function property\n");
>> +
>> +ret = fwnode_property_read_string(fwnode, "color", >color);
>> +if (ret)
>> +pr_info("Failed to parse color property\n");
>> +}
>> +
>> +int led_compose_name(struct fwnode_handle *fwnode, const char *led_hw_name,
>> + const char *default_desc, char *led_classdev_name)
>> +{
>> +struct led_properties props = {};
>> +
>> +if (!led_classdev_name)
>> +return -EINVAL;
>> +
>> +led_parse_properties(fwnode, );
>> +
>> +if (props.label) {
>> +/*
>> + * Presence of 'label' DT property implies legacy LED name,
>> + * formatted as , with possible
>> + * section omission if doesn't apply to given device.
>> + *
>> + * If no led_hw_name has been passed, then it indicates that
>> + * DT label should be used as-is for LED class device name.
>> + * Otherwise the label is prepended with led_hw_name to compose
>> + * the final LED class device name.
>> + */
>> +if (!led_hw_name) {
>> +strncpy(led_classdev_name, props.label,
>> +LED_MAX_NAME_SIZE);
>> +} else {
>> +snprintf(led_classdev_name, LED_MAX_NAME_SIZE, "%s:%s",
>> + led_hw_name, props.label);
>> +}
>> +} else if (props.function || props.color) {
>> +snprintf(led_classdev_name, LED_MAX_NAME_SIZE, "%s:%s",
>> + props.color ?: "", props.function ?: "");
>> +} else if (default_desc) {
>> +if (!led_hw_name) {
>> +pr_err("Legacy LED naming requires devicename segment");
>> +return -EINVAL;
>> +}
>> +snprintf(led_classdev_name, LED_MAX_NAME_SIZE, "%s:%s",
>> + led_hw_name, default_desc);
>> +} else if (is_of_node(fwnode)) {
>> +strncpy(led_classdev_name, to_of_node(fwnode)->name,
>> +LED_MAX_NAME_SIZE);
>> +} else
>> +return -EINVAL;
>>
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
Hi Baolin,
Thanks for the review.
On 11/07/2018 08:20 AM, Baolin Wang wrote:
> Hi Jacek,
>
> On 7 November 2018 at 06:07, Jacek Anaszewski
> wrote:
>> Add public led_compose_name() API for composing LED class device
>> name basing on fwnode_handle data. The function composes device name
>> according to either a new pattern or the legacy
>> pattern. The decision on using the
>> particular pattern is made basing on whether fwnode contains new
>> "function" and "color" properties, or the legacy "label" proeprty.
>>
>> Backwards compatibility with in-driver hard-coded LED class device
>> names is assured thanks to the default_desc argument.
>>
>> In case none of the aformentioned properties was found, then, for OF
>> nodes, the node name is adopted for LED class device name.
>>
>> Signed-off-by: Jacek Anaszewski
>> Cc: Baolin Wang
>> Cc: Daniel Mack
>> Cc: Dan Murphy
>> Cc: Linus Walleij
>> Cc: Oleh Kravchenko
>> Cc: Sakari Ailus
>> Cc: Simon Shields
>> Cc: Xiaotong Lu
>> ---
>> Documentation/leds/leds-class.txt | 2 +-
>> drivers/leds/led-core.c | 71
>> +++
>> include/linux/leds.h | 32 ++
>> 3 files changed, 104 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/leds/leds-class.txt
>> b/Documentation/leds/leds-class.txt
>> index 836cb16..e9009c4 100644
>> --- a/Documentation/leds/leds-class.txt
>> +++ b/Documentation/leds/leds-class.txt
>> @@ -43,7 +43,7 @@ LED Device Naming
>>
>> Is currently of the form:
>>
>> -"devicename:colour:function"
>> +"colour:function"
>>
>> There have been calls for LED properties such as colour to be exported as
>> individual led class attributes. As a solution which doesn't incur as much
>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
>> index ede4fa0..f7371fc 100644
>> --- a/drivers/leds/led-core.c
>> +++ b/drivers/leds/led-core.c
>> @@ -16,6 +16,8 @@
>> #include
>> #include
>> #include
>> +#include
>> +#include
>> #include
>> #include "leds.h"
>>
>> @@ -327,3 +329,72 @@ void led_sysfs_enable(struct led_classdev *led_cdev)
>> led_cdev->flags &= ~LED_SYSFS_DISABLE;
>> }
>> EXPORT_SYMBOL_GPL(led_sysfs_enable);
>> +
>> +static void led_parse_properties(struct fwnode_handle *fwnode,
>> +struct led_properties *props)
>> +{
>> + int ret;
>> +
>> + if (!fwnode)
>> + return;
>> +
>> + ret = fwnode_property_read_string(fwnode, "label", >label);
>> + if (!ret)
>> + return;
>> +
>> + ret = fwnode_property_read_string(fwnode, "function",
>> >function);
>> + if (ret)
>> + pr_info("Failed to parse function property\n");
>> +
>> + ret = fwnode_property_read_string(fwnode, "color", >color);
>> + if (ret)
>> + pr_info("Failed to parse color property\n");
>
> Now the color and function properties can be optional, so we should
> deal with different return errors.
> -EINVAL means we have not set color or function property, which means
> we should not give failure message for users (maybe "not supply color
> property\n") and return 0 as success.
The lack of any of these properties is not critical, therefore this
function does not return the error code, but only prints the status.
Please note that I'm not using pr_error(), but pr_info().
Related to the message text - I agree, I will change it to e.g.
"color property not found"
> For other errors, we should not ignore them, instead we should give
> failure messages and return errors.
I've skimmed through other kernel drivers and vast majority of
them don't check exact failure code in case of non-zero return,
but assume that property is missing, and eventually report
error code. I'd do the same here.
> Tested-by: Baolin Wang
>
>> +}
>> +
>> +int led_compose_name(struct fwnode_handle *fwnode, const char *led_hw_name,
>> +const char *default_desc, char *led_classdev_name)
>> +{
>> + struct led_properties props = {};
>> +
>> + if (!led_classdev_name)
>> + return -EINVAL;
>> +
>> + led_parse_properties(fwnode, );
>> +
>> + if (props.label) {
>> + /*
>> +* Presence of 'label' DT property implies legacy LED name,
>> +* formatted as , with possible
>> +* section omission if doesn't apply to given device.
>> +*
>> +* If no led_hw_name has been passed, then it indicates that
>> +* DT label should be used as-is for LED class device name.
>> +* Otherwise the label is prepended with led_hw_name to
>> compose
>> +* the final LED class device name.
>> +*/
>> + if (!led_hw_name) {
>> + strncpy(led_classdev_name, props.label,
>> + LED_MAX_NAME_SIZE);
>> + } else {
>> +
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
Hi Baolin,
Thanks for the review.
On 11/07/2018 08:20 AM, Baolin Wang wrote:
> Hi Jacek,
>
> On 7 November 2018 at 06:07, Jacek Anaszewski
> wrote:
>> Add public led_compose_name() API for composing LED class device
>> name basing on fwnode_handle data. The function composes device name
>> according to either a new pattern or the legacy
>> pattern. The decision on using the
>> particular pattern is made basing on whether fwnode contains new
>> "function" and "color" properties, or the legacy "label" proeprty.
>>
>> Backwards compatibility with in-driver hard-coded LED class device
>> names is assured thanks to the default_desc argument.
>>
>> In case none of the aformentioned properties was found, then, for OF
>> nodes, the node name is adopted for LED class device name.
>>
>> Signed-off-by: Jacek Anaszewski
>> Cc: Baolin Wang
>> Cc: Daniel Mack
>> Cc: Dan Murphy
>> Cc: Linus Walleij
>> Cc: Oleh Kravchenko
>> Cc: Sakari Ailus
>> Cc: Simon Shields
>> Cc: Xiaotong Lu
>> ---
>> Documentation/leds/leds-class.txt | 2 +-
>> drivers/leds/led-core.c | 71
>> +++
>> include/linux/leds.h | 32 ++
>> 3 files changed, 104 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/leds/leds-class.txt
>> b/Documentation/leds/leds-class.txt
>> index 836cb16..e9009c4 100644
>> --- a/Documentation/leds/leds-class.txt
>> +++ b/Documentation/leds/leds-class.txt
>> @@ -43,7 +43,7 @@ LED Device Naming
>>
>> Is currently of the form:
>>
>> -"devicename:colour:function"
>> +"colour:function"
>>
>> There have been calls for LED properties such as colour to be exported as
>> individual led class attributes. As a solution which doesn't incur as much
>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
>> index ede4fa0..f7371fc 100644
>> --- a/drivers/leds/led-core.c
>> +++ b/drivers/leds/led-core.c
>> @@ -16,6 +16,8 @@
>> #include
>> #include
>> #include
>> +#include
>> +#include
>> #include
>> #include "leds.h"
>>
>> @@ -327,3 +329,72 @@ void led_sysfs_enable(struct led_classdev *led_cdev)
>> led_cdev->flags &= ~LED_SYSFS_DISABLE;
>> }
>> EXPORT_SYMBOL_GPL(led_sysfs_enable);
>> +
>> +static void led_parse_properties(struct fwnode_handle *fwnode,
>> +struct led_properties *props)
>> +{
>> + int ret;
>> +
>> + if (!fwnode)
>> + return;
>> +
>> + ret = fwnode_property_read_string(fwnode, "label", >label);
>> + if (!ret)
>> + return;
>> +
>> + ret = fwnode_property_read_string(fwnode, "function",
>> >function);
>> + if (ret)
>> + pr_info("Failed to parse function property\n");
>> +
>> + ret = fwnode_property_read_string(fwnode, "color", >color);
>> + if (ret)
>> + pr_info("Failed to parse color property\n");
>
> Now the color and function properties can be optional, so we should
> deal with different return errors.
> -EINVAL means we have not set color or function property, which means
> we should not give failure message for users (maybe "not supply color
> property\n") and return 0 as success.
The lack of any of these properties is not critical, therefore this
function does not return the error code, but only prints the status.
Please note that I'm not using pr_error(), but pr_info().
Related to the message text - I agree, I will change it to e.g.
"color property not found"
> For other errors, we should not ignore them, instead we should give
> failure messages and return errors.
I've skimmed through other kernel drivers and vast majority of
them don't check exact failure code in case of non-zero return,
but assume that property is missing, and eventually report
error code. I'd do the same here.
> Tested-by: Baolin Wang
>
>> +}
>> +
>> +int led_compose_name(struct fwnode_handle *fwnode, const char *led_hw_name,
>> +const char *default_desc, char *led_classdev_name)
>> +{
>> + struct led_properties props = {};
>> +
>> + if (!led_classdev_name)
>> + return -EINVAL;
>> +
>> + led_parse_properties(fwnode, );
>> +
>> + if (props.label) {
>> + /*
>> +* Presence of 'label' DT property implies legacy LED name,
>> +* formatted as , with possible
>> +* section omission if doesn't apply to given device.
>> +*
>> +* If no led_hw_name has been passed, then it indicates that
>> +* DT label should be used as-is for LED class device name.
>> +* Otherwise the label is prepended with led_hw_name to
>> compose
>> +* the final LED class device name.
>> +*/
>> + if (!led_hw_name) {
>> + strncpy(led_classdev_name, props.label,
>> + LED_MAX_NAME_SIZE);
>> + } else {
>> +
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
On 11/06/2018 04:07 PM, Jacek Anaszewski wrote:
> Add public led_compose_name() API for composing LED class device
> name basing on fwnode_handle data. The function composes device name
> according to either a new pattern or the legacy
> pattern. The decision on using the
> particular pattern is made basing on whether fwnode contains new
> "function" and "color" properties, or the legacy "label" proeprty.
>
> Backwards compatibility with in-driver hard-coded LED class device
> names is assured thanks to the default_desc argument.
>
> In case none of the aformentioned properties was found, then, for OF
> nodes, the node name is adopted for LED class device name.
>
> Signed-off-by: Jacek Anaszewski
> Cc: Baolin Wang
> Cc: Daniel Mack
> Cc: Dan Murphy
> Cc: Linus Walleij
> Cc: Oleh Kravchenko
> Cc: Sakari Ailus
> Cc: Simon Shields
> Cc: Xiaotong Lu
> ---
> Documentation/leds/leds-class.txt | 2 +-
> drivers/leds/led-core.c | 71
> +++
> include/linux/leds.h | 32 ++
> 3 files changed, 104 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/leds/leds-class.txt
> b/Documentation/leds/leds-class.txt
> index 836cb16..e9009c4 100644
> --- a/Documentation/leds/leds-class.txt
> +++ b/Documentation/leds/leds-class.txt
> @@ -43,7 +43,7 @@ LED Device Naming
>
> Is currently of the form:
>
> -"devicename:colour:function"
> +"colour:function"
>
> There have been calls for LED properties such as colour to be exported as
> individual led class attributes. As a solution which doesn't incur as much
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index ede4fa0..f7371fc 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -16,6 +16,8 @@
> #include
> #include
> #include
> +#include
> +#include
> #include
> #include "leds.h"
>
> @@ -327,3 +329,72 @@ void led_sysfs_enable(struct led_classdev *led_cdev)
> led_cdev->flags &= ~LED_SYSFS_DISABLE;
> }
> EXPORT_SYMBOL_GPL(led_sysfs_enable);
> +
> +static void led_parse_properties(struct fwnode_handle *fwnode,
> + struct led_properties *props)
> +{
> + int ret;
> +
> + if (!fwnode)
> + return;
> +
> + ret = fwnode_property_read_string(fwnode, "label", >label);
If we have the label do we need to continue to look for the function and color
nodes?
It can be checked and diverted using fwnode_property_present for the "label"
property
> + if (!ret)
> + return;
> +
> + ret = fwnode_property_read_string(fwnode, "function", >function);
> + if (ret)
> + pr_info("Failed to parse function property\n");
> +
> + ret = fwnode_property_read_string(fwnode, "color", >color);
> + if (ret)
> + pr_info("Failed to parse color property\n");
> +}
> +
> +int led_compose_name(struct fwnode_handle *fwnode, const char *led_hw_name,
> + const char *default_desc, char *led_classdev_name)
> +{
> + struct led_properties props = {};
> +
> + if (!led_classdev_name)
> + return -EINVAL;
> +
> + led_parse_properties(fwnode, );
> +
> + if (props.label) {
> + /*
> + * Presence of 'label' DT property implies legacy LED name,
> + * formatted as , with possible
> + * section omission if doesn't apply to given device.
> + *
> + * If no led_hw_name has been passed, then it indicates that
> + * DT label should be used as-is for LED class device name.
> + * Otherwise the label is prepended with led_hw_name to compose
> + * the final LED class device name.
> + */
> + if (!led_hw_name) {
> + strncpy(led_classdev_name, props.label,
> + LED_MAX_NAME_SIZE);
> + } else {
> + snprintf(led_classdev_name, LED_MAX_NAME_SIZE, "%s:%s",
> + led_hw_name, props.label);
> + }
> + } else if (props.function || props.color) {
> + snprintf(led_classdev_name, LED_MAX_NAME_SIZE, "%s:%s",
> + props.color ?: "", props.function ?: "");
> + } else if (default_desc) {
> + if (!led_hw_name) {
> + pr_err("Legacy LED naming requires devicename segment");
> + return -EINVAL;
> + }
> + snprintf(led_classdev_name, LED_MAX_NAME_SIZE, "%s:%s",
> + led_hw_name, default_desc);
> + } else if (is_of_node(fwnode)) {
> + strncpy(led_classdev_name, to_of_node(fwnode)->name,
> + LED_MAX_NAME_SIZE);
> + } else
> + return -EINVAL;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(led_compose_name);
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 646c49a..ddb4001 100644
> ---
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
On 11/06/2018 04:07 PM, Jacek Anaszewski wrote:
> Add public led_compose_name() API for composing LED class device
> name basing on fwnode_handle data. The function composes device name
> according to either a new pattern or the legacy
> pattern. The decision on using the
> particular pattern is made basing on whether fwnode contains new
> "function" and "color" properties, or the legacy "label" proeprty.
>
> Backwards compatibility with in-driver hard-coded LED class device
> names is assured thanks to the default_desc argument.
>
> In case none of the aformentioned properties was found, then, for OF
> nodes, the node name is adopted for LED class device name.
>
> Signed-off-by: Jacek Anaszewski
> Cc: Baolin Wang
> Cc: Daniel Mack
> Cc: Dan Murphy
> Cc: Linus Walleij
> Cc: Oleh Kravchenko
> Cc: Sakari Ailus
> Cc: Simon Shields
> Cc: Xiaotong Lu
> ---
> Documentation/leds/leds-class.txt | 2 +-
> drivers/leds/led-core.c | 71
> +++
> include/linux/leds.h | 32 ++
> 3 files changed, 104 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/leds/leds-class.txt
> b/Documentation/leds/leds-class.txt
> index 836cb16..e9009c4 100644
> --- a/Documentation/leds/leds-class.txt
> +++ b/Documentation/leds/leds-class.txt
> @@ -43,7 +43,7 @@ LED Device Naming
>
> Is currently of the form:
>
> -"devicename:colour:function"
> +"colour:function"
>
> There have been calls for LED properties such as colour to be exported as
> individual led class attributes. As a solution which doesn't incur as much
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index ede4fa0..f7371fc 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -16,6 +16,8 @@
> #include
> #include
> #include
> +#include
> +#include
> #include
> #include "leds.h"
>
> @@ -327,3 +329,72 @@ void led_sysfs_enable(struct led_classdev *led_cdev)
> led_cdev->flags &= ~LED_SYSFS_DISABLE;
> }
> EXPORT_SYMBOL_GPL(led_sysfs_enable);
> +
> +static void led_parse_properties(struct fwnode_handle *fwnode,
> + struct led_properties *props)
> +{
> + int ret;
> +
> + if (!fwnode)
> + return;
> +
> + ret = fwnode_property_read_string(fwnode, "label", >label);
If we have the label do we need to continue to look for the function and color
nodes?
It can be checked and diverted using fwnode_property_present for the "label"
property
> + if (!ret)
> + return;
> +
> + ret = fwnode_property_read_string(fwnode, "function", >function);
> + if (ret)
> + pr_info("Failed to parse function property\n");
> +
> + ret = fwnode_property_read_string(fwnode, "color", >color);
> + if (ret)
> + pr_info("Failed to parse color property\n");
> +}
> +
> +int led_compose_name(struct fwnode_handle *fwnode, const char *led_hw_name,
> + const char *default_desc, char *led_classdev_name)
> +{
> + struct led_properties props = {};
> +
> + if (!led_classdev_name)
> + return -EINVAL;
> +
> + led_parse_properties(fwnode, );
> +
> + if (props.label) {
> + /*
> + * Presence of 'label' DT property implies legacy LED name,
> + * formatted as , with possible
> + * section omission if doesn't apply to given device.
> + *
> + * If no led_hw_name has been passed, then it indicates that
> + * DT label should be used as-is for LED class device name.
> + * Otherwise the label is prepended with led_hw_name to compose
> + * the final LED class device name.
> + */
> + if (!led_hw_name) {
> + strncpy(led_classdev_name, props.label,
> + LED_MAX_NAME_SIZE);
> + } else {
> + snprintf(led_classdev_name, LED_MAX_NAME_SIZE, "%s:%s",
> + led_hw_name, props.label);
> + }
> + } else if (props.function || props.color) {
> + snprintf(led_classdev_name, LED_MAX_NAME_SIZE, "%s:%s",
> + props.color ?: "", props.function ?: "");
> + } else if (default_desc) {
> + if (!led_hw_name) {
> + pr_err("Legacy LED naming requires devicename segment");
> + return -EINVAL;
> + }
> + snprintf(led_classdev_name, LED_MAX_NAME_SIZE, "%s:%s",
> + led_hw_name, default_desc);
> + } else if (is_of_node(fwnode)) {
> + strncpy(led_classdev_name, to_of_node(fwnode)->name,
> + LED_MAX_NAME_SIZE);
> + } else
> + return -EINVAL;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(led_compose_name);
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 646c49a..ddb4001 100644
> ---
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
Hi Jacek,
On 7 November 2018 at 06:07, Jacek Anaszewski
wrote:
> Add public led_compose_name() API for composing LED class device
> name basing on fwnode_handle data. The function composes device name
> according to either a new pattern or the legacy
> pattern. The decision on using the
> particular pattern is made basing on whether fwnode contains new
> "function" and "color" properties, or the legacy "label" proeprty.
>
> Backwards compatibility with in-driver hard-coded LED class device
> names is assured thanks to the default_desc argument.
>
> In case none of the aformentioned properties was found, then, for OF
> nodes, the node name is adopted for LED class device name.
>
> Signed-off-by: Jacek Anaszewski
> Cc: Baolin Wang
> Cc: Daniel Mack
> Cc: Dan Murphy
> Cc: Linus Walleij
> Cc: Oleh Kravchenko
> Cc: Sakari Ailus
> Cc: Simon Shields
> Cc: Xiaotong Lu
> ---
> Documentation/leds/leds-class.txt | 2 +-
> drivers/leds/led-core.c | 71
> +++
> include/linux/leds.h | 32 ++
> 3 files changed, 104 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/leds/leds-class.txt
> b/Documentation/leds/leds-class.txt
> index 836cb16..e9009c4 100644
> --- a/Documentation/leds/leds-class.txt
> +++ b/Documentation/leds/leds-class.txt
> @@ -43,7 +43,7 @@ LED Device Naming
>
> Is currently of the form:
>
> -"devicename:colour:function"
> +"colour:function"
>
> There have been calls for LED properties such as colour to be exported as
> individual led class attributes. As a solution which doesn't incur as much
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index ede4fa0..f7371fc 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -16,6 +16,8 @@
> #include
> #include
> #include
> +#include
> +#include
> #include
> #include "leds.h"
>
> @@ -327,3 +329,72 @@ void led_sysfs_enable(struct led_classdev *led_cdev)
> led_cdev->flags &= ~LED_SYSFS_DISABLE;
> }
> EXPORT_SYMBOL_GPL(led_sysfs_enable);
> +
> +static void led_parse_properties(struct fwnode_handle *fwnode,
> +struct led_properties *props)
> +{
> + int ret;
> +
> + if (!fwnode)
> + return;
> +
> + ret = fwnode_property_read_string(fwnode, "label", >label);
> + if (!ret)
> + return;
> +
> + ret = fwnode_property_read_string(fwnode, "function",
> >function);
> + if (ret)
> + pr_info("Failed to parse function property\n");
> +
> + ret = fwnode_property_read_string(fwnode, "color", >color);
> + if (ret)
> + pr_info("Failed to parse color property\n");
Now the color and function properties can be optional, so we should
deal with different return errors.
-EINVAL means we have not set color or function property, which means
we should not give failure message for users (maybe "not supply color
property\n") and return 0 as success.
For other errors, we should not ignore them, instead we should give
failure messages and return errors.
Tested-by: Baolin Wang
> +}
> +
> +int led_compose_name(struct fwnode_handle *fwnode, const char *led_hw_name,
> +const char *default_desc, char *led_classdev_name)
> +{
> + struct led_properties props = {};
> +
> + if (!led_classdev_name)
> + return -EINVAL;
> +
> + led_parse_properties(fwnode, );
> +
> + if (props.label) {
> + /*
> +* Presence of 'label' DT property implies legacy LED name,
> +* formatted as , with possible
> +* section omission if doesn't apply to given device.
> +*
> +* If no led_hw_name has been passed, then it indicates that
> +* DT label should be used as-is for LED class device name.
> +* Otherwise the label is prepended with led_hw_name to
> compose
> +* the final LED class device name.
> +*/
> + if (!led_hw_name) {
> + strncpy(led_classdev_name, props.label,
> + LED_MAX_NAME_SIZE);
> + } else {
> + snprintf(led_classdev_name, LED_MAX_NAME_SIZE,
> "%s:%s",
> +led_hw_name, props.label);
> + }
> + } else if (props.function || props.color) {
> + snprintf(led_classdev_name, LED_MAX_NAME_SIZE, "%s:%s",
> +props.color ?: "", props.function ?: "");
> + } else if (default_desc) {
> + if (!led_hw_name) {
> + pr_err("Legacy LED naming requires devicename
> segment");
> + return -EINVAL;
> + }
> + snprintf(led_classdev_name, LED_MAX_NAME_SIZE, "%s:%s",
> +led_hw_name, default_desc);
> + } else if
Re: [PATCH 02/24] leds: core: Add support for composing LED class device names
Hi Jacek,
On 7 November 2018 at 06:07, Jacek Anaszewski
wrote:
> Add public led_compose_name() API for composing LED class device
> name basing on fwnode_handle data. The function composes device name
> according to either a new pattern or the legacy
> pattern. The decision on using the
> particular pattern is made basing on whether fwnode contains new
> "function" and "color" properties, or the legacy "label" proeprty.
>
> Backwards compatibility with in-driver hard-coded LED class device
> names is assured thanks to the default_desc argument.
>
> In case none of the aformentioned properties was found, then, for OF
> nodes, the node name is adopted for LED class device name.
>
> Signed-off-by: Jacek Anaszewski
> Cc: Baolin Wang
> Cc: Daniel Mack
> Cc: Dan Murphy
> Cc: Linus Walleij
> Cc: Oleh Kravchenko
> Cc: Sakari Ailus
> Cc: Simon Shields
> Cc: Xiaotong Lu
> ---
> Documentation/leds/leds-class.txt | 2 +-
> drivers/leds/led-core.c | 71
> +++
> include/linux/leds.h | 32 ++
> 3 files changed, 104 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/leds/leds-class.txt
> b/Documentation/leds/leds-class.txt
> index 836cb16..e9009c4 100644
> --- a/Documentation/leds/leds-class.txt
> +++ b/Documentation/leds/leds-class.txt
> @@ -43,7 +43,7 @@ LED Device Naming
>
> Is currently of the form:
>
> -"devicename:colour:function"
> +"colour:function"
>
> There have been calls for LED properties such as colour to be exported as
> individual led class attributes. As a solution which doesn't incur as much
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index ede4fa0..f7371fc 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -16,6 +16,8 @@
> #include
> #include
> #include
> +#include
> +#include
> #include
> #include "leds.h"
>
> @@ -327,3 +329,72 @@ void led_sysfs_enable(struct led_classdev *led_cdev)
> led_cdev->flags &= ~LED_SYSFS_DISABLE;
> }
> EXPORT_SYMBOL_GPL(led_sysfs_enable);
> +
> +static void led_parse_properties(struct fwnode_handle *fwnode,
> +struct led_properties *props)
> +{
> + int ret;
> +
> + if (!fwnode)
> + return;
> +
> + ret = fwnode_property_read_string(fwnode, "label", >label);
> + if (!ret)
> + return;
> +
> + ret = fwnode_property_read_string(fwnode, "function",
> >function);
> + if (ret)
> + pr_info("Failed to parse function property\n");
> +
> + ret = fwnode_property_read_string(fwnode, "color", >color);
> + if (ret)
> + pr_info("Failed to parse color property\n");
Now the color and function properties can be optional, so we should
deal with different return errors.
-EINVAL means we have not set color or function property, which means
we should not give failure message for users (maybe "not supply color
property\n") and return 0 as success.
For other errors, we should not ignore them, instead we should give
failure messages and return errors.
Tested-by: Baolin Wang
> +}
> +
> +int led_compose_name(struct fwnode_handle *fwnode, const char *led_hw_name,
> +const char *default_desc, char *led_classdev_name)
> +{
> + struct led_properties props = {};
> +
> + if (!led_classdev_name)
> + return -EINVAL;
> +
> + led_parse_properties(fwnode, );
> +
> + if (props.label) {
> + /*
> +* Presence of 'label' DT property implies legacy LED name,
> +* formatted as , with possible
> +* section omission if doesn't apply to given device.
> +*
> +* If no led_hw_name has been passed, then it indicates that
> +* DT label should be used as-is for LED class device name.
> +* Otherwise the label is prepended with led_hw_name to
> compose
> +* the final LED class device name.
> +*/
> + if (!led_hw_name) {
> + strncpy(led_classdev_name, props.label,
> + LED_MAX_NAME_SIZE);
> + } else {
> + snprintf(led_classdev_name, LED_MAX_NAME_SIZE,
> "%s:%s",
> +led_hw_name, props.label);
> + }
> + } else if (props.function || props.color) {
> + snprintf(led_classdev_name, LED_MAX_NAME_SIZE, "%s:%s",
> +props.color ?: "", props.function ?: "");
> + } else if (default_desc) {
> + if (!led_hw_name) {
> + pr_err("Legacy LED naming requires devicename
> segment");
> + return -EINVAL;
> + }
> + snprintf(led_classdev_name, LED_MAX_NAME_SIZE, "%s:%s",
> +led_hw_name, default_desc);
> + } else if
