[PATCH/RFC] DT: leds: Fix 'label' property description and add 'colour' property

[PATCH/RFC] DT: leds: Fix 'label' property description and add 'colour' property

[Jacek Anaszewski]

Label property was imposed a uniqueness requirement, which was erroneous,
since ePAPR defines it to "a human readable string describing a device".

Also the binding description misleadingly suggested direct usage of label
for LED class device name, whereas it should only define a LED function.

Therefore an additional 'colour' property is being introduced, which together
with the parent DT node name used for devicename shall be used for naming LED
class device according to the patterh <devicename>:<colour>:<function>.

Fixes: 116b8e164116 ("DT: leds: Add uniqueness requirement for 'label' property")
Signed-off-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
---
 Documentation/devicetree/bindings/leds/common.txt | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
index 1d4afe9..16973ee 100644
--- a/Documentation/devicetree/bindings/leds/common.txt
+++ b/Documentation/devicetree/bindings/leds/common.txt
@@ -14,10 +14,9 @@ Optional properties for child nodes:
 - led-sources : List of device current outputs the LED is connected to. The
 		outputs are identified by the numbers that must be defined
 		in the LED device binding documentation.
-- label : The label for this LED. If omitted, the label is taken from the node
-	  name (excluding the unit address). It has to uniquely identify
-	  a device, i.e. no other LED class device can be assigned the same
-	  label.
+- label : The label for this LED. It should describe its function. If omitted,
+          the label is taken from the node name (excluding the unit address).
+- colour : Colour of the LED.
 
 - default-state : The initial state of the LED. Valid values are "on", "off",
   and "keep". If the LED is already on or off and the default-state property is
-- 
2.1.4

Re: [PATCH/RFC] DT: leds: Fix 'label' property description and add 'colour' property

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2204 bytes --]

Hi!

> Label property was imposed a uniqueness requirement, which was erroneous,
> since ePAPR defines it to "a human readable string describing a device".
> 
> Also the binding description misleadingly suggested direct usage of label
> for LED class device name, whereas it should only define a LED function.
> 
> Therefore an additional 'colour' property is being introduced, which together
> with the parent DT node name used for devicename shall be used for naming LED
> class device according to the patterh
> <devicename>:<colour>:<function>.


> -- label : The label for this LED. If omitted, the label is taken from the node
> -	  name (excluding the unit address). It has to uniquely identify
> -	  a device, i.e. no other LED class device can be assigned the same
> -	  label.
> +- label : The label for this LED. It should describe its function. If omitted,
> +          the label is taken from the node name (excluding the unit address).

So the label contains "as1235:green:capslock"? I guess it might be
nice to mention that. Or just the "capslock" part?

Also.. it would be good to start pushing for more consistency in the
labels: I have these on the thinkpad:

input5::scrolllock/   tpacpi::dock_status2/ tpacpi::unknown_led/
mmc0::/               tpacpi:green:batt/    tpacpi::unknown_led2/
phy0-led/             tpacpi:orange:batt/   tpacpi::unknown_led3/
tpacpi::bay_active/   tpacpi::power/

On embedded system, I'd like to see <devicename> to corespond
to.. device the led belongs to, as opposed to name of the chip that
drives the led. Maybe we should do 'main_camera:white:flash' instead of
'as4132:white:flash' because userspace already has information on what
chip it is (sysfs paths), but can not easily figure out to which
device the flash belongs.

> +- colour : Colour of the LED.
>  
>  - default-state : The initial state of the LED. Valid values are "on", "off",
>    and "keep". If the LED is already on or off and the default-state
> -	  property is

Best regards,
								Pavel


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH/RFC] DT: leds: Fix 'label' property description and add 'colour' property

[Jacek Anaszewski]

Hi Pavel,

On 12/04/2017 11:43 AM, Pavel Machek wrote:
> Hi!
> 
>> Label property was imposed a uniqueness requirement, which was erroneous,
>> since ePAPR defines it to "a human readable string describing a device".
>>
>> Also the binding description misleadingly suggested direct usage of label
>> for LED class device name, whereas it should only define a LED function.
>>
>> Therefore an additional 'colour' property is being introduced, which together
>> with the parent DT node name used for devicename shall be used for naming LED
>> class device according to the patterh
>> <devicename>:<colour>:<function>.
> 
> 
>> -- label : The label for this LED. If omitted, the label is taken from the node
>> -	  name (excluding the unit address). It has to uniquely identify
>> -	  a device, i.e. no other LED class device can be assigned the same
>> -	  label.
>> +- label : The label for this LED. It should describe its function. If omitted,
>> +          the label is taken from the node name (excluding the unit address).
> 
> So the label contains "as1235:green:capslock"? I guess it might be
> nice to mention that. Or just the "capslock" part?

Linguistic fix:

s/It should describe its function/It should describe LED's function/

So in your example it would be "capslock" part alone. Existing examples
already show that properly IMO.

> 
> Also.. it would be good to start pushing for more consistency in the
> labels: I have these on the thinkpad:
> 
> input5::scrolllock/   tpacpi::dock_status2/ tpacpi::unknown_led/
> mmc0::/               tpacpi:green:batt/    tpacpi::unknown_led2/
> phy0-led/             tpacpi:orange:batt/   tpacpi::unknown_led3/
> tpacpi::bay_active/   tpacpi::power/
> 
> On embedded system, I'd like to see <devicename> to corespond
> to.. device the led belongs to, as opposed to name of the chip that
> drives the led. Maybe we should do 'main_camera:white:flash' instead of
> 'as4132:white:flash' because userspace already has information on what
> chip it is (sysfs paths), but can not easily figure out to which
> device the flash belongs.

It would be inconsistent with current design of V4L2 media device,
which presents every media entity with chip name.
Also, it would be hard to prove that LED controller is part of the
main camera hardware when it is a standalone chip.

>> +- colour : Colour of the LED.
>>  
>>  - default-state : The initial state of the LED. Valid values are "on", "off",
>>    and "keep". If the LED is already on or off and the default-state
>> -	  property is
> 
> Best regards,
> 								Pavel
> 
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH/RFC] DT: leds: Fix 'label' property description and add 'colour' property

[Dan Murphy]

Jacek

On 12/02/2017 03:50 PM, Jacek Anaszewski wrote:
> Label property was imposed a uniqueness requirement, which was erroneous,
> since ePAPR defines it to "a human readable string describing a device".
> 
> Also the binding description misleadingly suggested direct usage of label
> for LED class device name, whereas it should only define a LED function.
> 
> Therefore an additional 'colour' property is being introduced, which together
> with the parent DT node name used for devicename shall be used for naming LED
> class device according to the patterh <devicename>:<colour>:<function>.
> 

In submitting updated DT bindings the maintainers are requesting all device parent
LED nodes be 

leds@xx {

Now this will make every LED class devicename

leds:<colour>:<function>

So either we cannot use the devicename or we need need to use the compatible string instead.

So an example would be

ti,lp8860:white:backlight

You should be on these patches.

Dan

> Fixes: 116b8e164116 ("DT: leds: Add uniqueness requirement for 'label' property")
> Signed-off-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> ---
>  Documentation/devicetree/bindings/leds/common.txt | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
> index 1d4afe9..16973ee 100644
> --- a/Documentation/devicetree/bindings/leds/common.txt
> +++ b/Documentation/devicetree/bindings/leds/common.txt
> @@ -14,10 +14,9 @@ Optional properties for child nodes:
>  - led-sources : List of device current outputs the LED is connected to. The
>  		outputs are identified by the numbers that must be defined
>  		in the LED device binding documentation.
> -- label : The label for this LED. If omitted, the label is taken from the node
> -	  name (excluding the unit address). It has to uniquely identify
> -	  a device, i.e. no other LED class device can be assigned the same
> -	  label.
> +- label : The label for this LED. It should describe its function. If omitted,
> +          the label is taken from the node name (excluding the unit address).
> +- colour : Colour of the LED.
>  
>  - default-state : The initial state of the LED. Valid values are "on", "off",
>    and "keep". If the LED is already on or off and the default-state property is
> 


-- 
------------------
Dan Murphy

Re: [PATCH/RFC] DT: leds: Fix 'label' property description and add 'colour' property

[Rob Herring]

On Mon, Dec 4, 2017 at 4:43 AM, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
>> Label property was imposed a uniqueness requirement, which was erroneous,
>> since ePAPR defines it to "a human readable string describing a device".

But it still needs to be unique to be useful. It's just unique from a
different perspective. Ideally, if you had a block diagram level
drawing of a device or board showing LEDs (and displays with
backlights), you would simply take the labels from that drawing.

>> Also the binding description misleadingly suggested direct usage of label
>> for LED class device name, whereas it should only define a LED function.
>>
>> Therefore an additional 'colour' property is being introduced, which together
>> with the parent DT node name used for devicename shall be used for naming LED
>> class device according to the patterh
>> <devicename>:<colour>:<function>.
>
>
>> -- label : The label for this LED. If omitted, the label is taken from the node
>> -       name (excluding the unit address). It has to uniquely identify
>> -       a device, i.e. no other LED class device can be assigned the same
>> -       label.
>> +- label : The label for this LED. It should describe its function. If omitted,
>> +          the label is taken from the node name (excluding the unit address).
>
> So the label contains "as1235:green:capslock"? I guess it might be
> nice to mention that. Or just the "capslock" part?
>
> Also.. it would be good to start pushing for more consistency in the
> labels: I have these on the thinkpad:
>
> input5::scrolllock/   tpacpi::dock_status2/ tpacpi::unknown_led/
> mmc0::/               tpacpi:green:batt/    tpacpi::unknown_led2/
> phy0-led/             tpacpi:orange:batt/   tpacpi::unknown_led3/
> tpacpi::bay_active/   tpacpi::power/
>
> On embedded system, I'd like to see <devicename> to corespond
> to.. device the led belongs to, as opposed to name of the chip that
> drives the led. Maybe we should do 'main_camera:white:flash' instead of
> 'as4132:white:flash' because userspace already has information on what
> chip it is (sysfs paths), but can not easily figure out to which
> device the flash belongs.

A couple of points:

I already mentioned DT node naming policies in the lm3692x thread, so
I won't repeat here.

Using the node name is not going to guarantee uniqueness in the names.
Even if you added the unit address it would still not. I can easily
have 2 or more LED driver chips at the same I2C address on different
buses. The only guaranteed unique name is the full DT path. Once
you've added some OS specific numbering to make device names unique,
then I'm not sure there's a lot of value in naming things after what
drives them. You can walk the sysfs hierarchy to determine that
anyway.

A case I care about is I have a family of boards that all have a
common defined set of 4 LEDs. They could be driven by anything, but I
want the same interface presented to userspace across boards.

Rob

Re: [PATCH/RFC] DT: leds: Fix 'label' property description and add 'colour' property

[Jacek Anaszewski]

On 12/11/2017 05:27 PM, Rob Herring wrote:
> On Mon, Dec 4, 2017 at 4:43 AM, Pavel Machek <pavel@ucw.cz> wrote:
>> Hi!
>>
>>> Label property was imposed a uniqueness requirement, which was erroneous,
>>> since ePAPR defines it to "a human readable string describing a device".
> 
> But it still needs to be unique to be useful. It's just unique from a

This patch is a follow-up of your following statement from [0]:

>>>>>>>
I really dislike how this naming convention is used for label. label is
supposed to be the phyically identifiable name. Having the devicename
defeats that. Perhaps color, too. We'd be better off with a color
property. It seems we're overloading the naming with too many things.
Now we're adding device association.
<<<<<<<


In effect, I intended to remove the uniqueness requirement for the
label since the label alone doesn't make a LED class device name
in case of many current LED class drivers.

Instead I inferred from your message that label should contain only
LED function. Therefore I'm splitting colour (maybe color would fit
better here, but adopted the spelling from
Documentation/leds/leds-class.txt, to be decided which one fits better.)

But see below.

> different perspective. Ideally, if you had a block diagram level
> drawing of a device or board showing LEDs (and displays with
> backlights), you would simply take the labels from that drawing.
> 
>>> Also the binding description misleadingly suggested direct usage of label
>>> for LED class device name, whereas it should only define a LED function.
>>>
>>> Therefore an additional 'colour' property is being introduced, which together
>>> with the parent DT node name used for devicename shall be used for naming LED
>>> class device according to the patterh
>>> <devicename>:<colour>:<function>.
>>
>>
>>> -- label : The label for this LED. If omitted, the label is taken from the node
>>> -       name (excluding the unit address). It has to uniquely identify
>>> -       a device, i.e. no other LED class device can be assigned the same
>>> -       label.
>>> +- label : The label for this LED. It should describe its function. If omitted,
>>> +          the label is taken from the node name (excluding the unit address).
>>
>> So the label contains "as1235:green:capslock"? I guess it might be
>> nice to mention that. Or just the "capslock" part?
>>
>> Also.. it would be good to start pushing for more consistency in the
>> labels: I have these on the thinkpad:
>>
>> input5::scrolllock/   tpacpi::dock_status2/ tpacpi::unknown_led/
>> mmc0::/               tpacpi:green:batt/    tpacpi::unknown_led2/
>> phy0-led/             tpacpi:orange:batt/   tpacpi::unknown_led3/
>> tpacpi::bay_active/   tpacpi::power/
>>
>> On embedded system, I'd like to see <devicename> to corespond
>> to.. device the led belongs to, as opposed to name of the chip that
>> drives the led. Maybe we should do 'main_camera:white:flash' instead of
>> 'as4132:white:flash' because userspace already has information on what
>> chip it is (sysfs paths), but can not easily figure out to which
>> device the flash belongs.
> 
> A couple of points:
> 
> I already mentioned DT node naming policies in the lm3692x thread, so
> I won't repeat here.
> 
> Using the node name is not going to guarantee uniqueness in the names.
> Even if you added the unit address it would still not. I can easily
> have 2 or more LED driver chips at the same I2C address on different
> buses. The only guaranteed unique name is the full DT path. Once
> you've added some OS specific numbering to make device names unique,
> then I'm not sure there's a lot of value in naming things after what
> drives them. You can walk the sysfs hierarchy to determine that
> anyway.
> 
> A case I care about is I have a family of boards that all have a
> common defined set of 4 LEDs. They could be driven by anything, but I
> want the same interface presented to userspace across boards.

In effect it looks like we should drop devicename section from
the LED class devicename pattern, label should describe only
LED function and additional color property could be introduced,
to be concatenated with LED function as a final LED class device
name.


[0] https://www.mail-archive.com/linux-media@vger.kernel.org/msg119473.html

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH/RFC] DT: leds: Fix 'label' property description and add 'colour' property

[Jacek Anaszewski]

cc devicetree@vger.kernel.org

On 12/14/2017 10:35 PM, Jacek Anaszewski wrote:
> On 12/11/2017 05:27 PM, Rob Herring wrote:
>> On Mon, Dec 4, 2017 at 4:43 AM, Pavel Machek <pavel@ucw.cz> wrote:
>>> Hi!
>>>
>>>> Label property was imposed a uniqueness requirement, which was erroneous,
>>>> since ePAPR defines it to "a human readable string describing a device".
>>
>> But it still needs to be unique to be useful. It's just unique from a
> 
> This patch is a follow-up of your following statement from [0]:
> 
>>>>>>>>
> I really dislike how this naming convention is used for label. label is
> supposed to be the phyically identifiable name. Having the devicename
> defeats that. Perhaps color, too. We'd be better off with a color
> property. It seems we're overloading the naming with too many things.
> Now we're adding device association.
> <<<<<<<
> 
> 
> In effect, I intended to remove the uniqueness requirement for the
> label since the label alone doesn't make a LED class device name
> in case of many current LED class drivers.
> 
> Instead I inferred from your message that label should contain only
> LED function. Therefore I'm splitting colour (maybe color would fit
> better here, but adopted the spelling from
> Documentation/leds/leds-class.txt, to be decided which one fits better.)
> 
> But see below.
> 
>> different perspective. Ideally, if you had a block diagram level
>> drawing of a device or board showing LEDs (and displays with
>> backlights), you would simply take the labels from that drawing.
>>
>>>> Also the binding description misleadingly suggested direct usage of label
>>>> for LED class device name, whereas it should only define a LED function.
>>>>
>>>> Therefore an additional 'colour' property is being introduced, which together
>>>> with the parent DT node name used for devicename shall be used for naming LED
>>>> class device according to the patterh
>>>> <devicename>:<colour>:<function>.
>>>
>>>
>>>> -- label : The label for this LED. If omitted, the label is taken from the node
>>>> -       name (excluding the unit address). It has to uniquely identify
>>>> -       a device, i.e. no other LED class device can be assigned the same
>>>> -       label.
>>>> +- label : The label for this LED. It should describe its function. If omitted,
>>>> +          the label is taken from the node name (excluding the unit address).
>>>
>>> So the label contains "as1235:green:capslock"? I guess it might be
>>> nice to mention that. Or just the "capslock" part?
>>>
>>> Also.. it would be good to start pushing for more consistency in the
>>> labels: I have these on the thinkpad:
>>>
>>> input5::scrolllock/   tpacpi::dock_status2/ tpacpi::unknown_led/
>>> mmc0::/               tpacpi:green:batt/    tpacpi::unknown_led2/
>>> phy0-led/             tpacpi:orange:batt/   tpacpi::unknown_led3/
>>> tpacpi::bay_active/   tpacpi::power/
>>>
>>> On embedded system, I'd like to see <devicename> to corespond
>>> to.. device the led belongs to, as opposed to name of the chip that
>>> drives the led. Maybe we should do 'main_camera:white:flash' instead of
>>> 'as4132:white:flash' because userspace already has information on what
>>> chip it is (sysfs paths), but can not easily figure out to which
>>> device the flash belongs.
>>
>> A couple of points:
>>
>> I already mentioned DT node naming policies in the lm3692x thread, so
>> I won't repeat here.
>>
>> Using the node name is not going to guarantee uniqueness in the names.
>> Even if you added the unit address it would still not. I can easily
>> have 2 or more LED driver chips at the same I2C address on different
>> buses. The only guaranteed unique name is the full DT path. Once
>> you've added some OS specific numbering to make device names unique,
>> then I'm not sure there's a lot of value in naming things after what
>> drives them. You can walk the sysfs hierarchy to determine that
>> anyway.
>>
>> A case I care about is I have a family of boards that all have a
>> common defined set of 4 LEDs. They could be driven by anything, but I
>> want the same interface presented to userspace across boards.
> 
> In effect it looks like we should drop devicename section from
> the LED class devicename pattern, label should describe only
> LED function and additional color property could be introduced,
> to be concatenated with LED function as a final LED class device
> name.
> 
> 
> [0] https://www.mail-archive.com/linux-media@vger.kernel.org/msg119473.html
> 

Re: [PATCH/RFC] DT: leds: Fix 'label' property description and add 'colour' property

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1366 bytes --]

Hi!

> >> Also.. it would be good to start pushing for more consistency in the
> >> labels: I have these on the thinkpad:
> >>
> >> input5::scrolllock/   tpacpi::dock_status2/ tpacpi::unknown_led/
> >> mmc0::/               tpacpi:green:batt/    tpacpi::unknown_led2/
> >> phy0-led/             tpacpi:orange:batt/   tpacpi::unknown_led3/
> >> tpacpi::bay_active/   tpacpi::power/
> 
> In effect it looks like we should drop devicename section from
> the LED class devicename pattern, label should describe only
> LED function and additional color property could be introduced,
> to be concatenated with LED function as a final LED class device
> name.

Well, the device name makes sense if the LED is not on the "main"
device.

I do have "scrollock" led on internal keyboard, and scollock led on
USB keyboard... it is good to be able to tell  which is which.

But yes, IMO it makes sense to move from

"as4134::backlight" to "screen::backlight" or something, and have it
standard across the machines.

tpacpi:: is not useful prefix for a led name. mmc0:: and input5:: are
quite useful.

Similary, as5124::flash is not useful, but main_camera::flash or
video5::flash might be.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]