Re: [RESEND PATCH v2 4/6] dt: bindings: as3645a: Improve label documentation, DT example

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

On 09/22/2017 11:07 PM, Jacek Anaszewski wrote:
> On 09/20/2017 10:53 PM, Rob Herring wrote:
>> On Tue, Sep 19, 2017 at 11:01:02PM +0200, Jacek Anaszewski wrote:
>>> Hi Pavel,
>>>
>>> On 09/18/2017 10:54 PM, Pavel Machek wrote:
>>>> On Mon 2017-09-18 17:49:23, Sakari Ailus wrote:
>>>>> Hi Pavel,
>>>>>
>>>>> On Mon, Sep 18, 2017 at 12:56:55PM +0200, Pavel Machek wrote:
>>>>>> Hi!
>>>>>>
>>>>>>> Specify the exact label used if the label property is omitted in DT, as
>>>>>>> well as use label in the example that conforms to LED device naming.
>>>>>>>
>>>>>>> @@ -69,11 +73,11 @@ Example
>>>>>>>  			flash-max-microamp = <320000>;
>>>>>>>  			led-max-microamp = <60000>;
>>>>>>>  			ams,input-max-microamp = <1750000>;
>>>>>>> -			label = "as3645a:flash";
>>>>>>> +			label = "as3645a:white:flash";
>>>>>>>  		};
>>>>>>>  		indicator@1 {
>>>>>>>  			reg = <0x1>;
>>>>>>>  			led-max-microamp = <10000>;
>>>>>>> -			label = "as3645a:indicator";
>>>>>>> +			label = "as3645a:red:indicator";
>>>>>>>  		};
>>>>>>>  	};
>>>>>>
>>>>>> Ok, but userspace still has no chance to determine if this is flash
>>>>>> from main camera or flash for front camera; todays smartphones have
>>>>>> flashes on both cameras.
>>>>>>
>>>>>> So.. Can I suggset as3645a:white:main_camera_flash or main_flash or
>>>>>> ....?
>>>>>
>>>>> If there's just a single one in the device, could you use that?
>>>>>
>>>>> Even if we name this so for N9 (and N900), the application still would only
>>>>> work with the two devices.
>>>>
>>>> Well, I'd plan to name it on other devices, too.
>>>>
>>>>> My suggestion would be to look for a flash LED, and perhaps the maximum
>>>>> current as well. That should generally work better than assumptions on the
>>>>> label.
>>>>
>>>> If you just look for flash LED, you don't know if it is front one or
>>>> back one. Its true that if you have just one flash it is usually on
>>>> the back camera, but you can't know if maybe driver is not available
>>>> for the main flash.
>>>>
>>>> Lets get this right, please "main_camera_flash" is 12 bytes more than
>>>> "flash", and it saves application logic.. more than 12 bytes, I'm sure. 
>>>
>>> What you are trying to introduce is yet another level of LED class
>>> device naming standard, one level below devicename:colour:function.
>>> It seems you want also to come up with the set of standarized LED
>>> function names. This would certainly have to be covered for consistency.
>>
>> 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.
> 
> Regarding devicename - there is indeed inconsistency in the way how LED
> DT bindings use label, as some of them use it for defining full LED
> class device name, and the rest fill only colour and function, leaving
> addition of a devicename to the driver.
> 
> The problem is also in current definition of label in LED common
> bindings documentation, which says:
> 
> "It has to uniquely identify a device, i.e. no other LED class device
> can be assigned the same label."
> 
> In view of your above words this is not true, and we probably should
> remove this sentence (it doesn't have DT maintainer ack btw).
> 
>> I do want to see standard names though. On 96boards for example, there 
>> are defined LEDs and locations. The function on some are defined (e.g. 
>> WiFi/BT) and somewhat undefined on others (user{1-4}). I'd like to see 
>> the same label across all boards.
> 
> Currently we have following LED functions (obtained with
> grep label Documentation/devicetree/bindings/leds/* | sed
> s'/^.*label/label/g' | awk -F"=" '{print $2}' | sed '/^$/d' | sed
> s'/.*:\(.*\)";/\1/' | sed '/^\s\{1,\}/d' | sort -u)
> 
> 0
> 1
> 2
> 2g
> 3
> 4
> 5
> 6
> 7
> adsl
> alarm
> alive
> aux
> broadband
> chrg
> dsl
> flash
> green
> indicator
> inet
> keypad
> phone
> power
> red
> sata
> sata0
> sata1
> tel
> tv
> upgrading
> usb
> usr0
> usr1
> usr35
> wan
> white
> wireless
> wps
> yellow
> 
> By extracting numerical pattern names and replacing numbers with N
> we're getting something like this:
> 
> N
> Ng
> colour
> adsl
> alarm
> alive
> aux
> broadband
> chrg
> dsl
> flash
> indicator
> inet
> keypad
> phone
> power
> sataN
> tel
> tv
> upgrading
> usb
> usrN
> wan
> wireless
> wps
> 
> Is this list something you'd like to see as a base of standard LED
> functions? It seems that this list would have to be continuously
> supplemented with new positions.
> 

Even better option is to grep through all *.dts* files in the arch
directory. A slightly modified command chain. which removes numerical
postfixes (there are some not covered corner cases though)

find arch -name "*.dts*" | xargs grep label | sed s'/^.*label/label/g' |
awk -F"=" '{print $2}' | sed s'/.*:\(.*\)";/\1/' | sed '/^\s\{1,\}/d' |
sed s'/\([^0-9]*\)\([0-9]*\)/\1/' | sed '/^$/d' | sort -u

gives following 135 positions (~5 colours percolated due
to some non-covered corner cases), and some of them don't belong
to LED DT nodes unfortunately, as e.g. gpio-keys use also ":" as
a delimiter in their labels, but the whole set gives reasonable
overview I think:

active
activity_led
adsl
alarm
alive
all
amber
app
aux
backup
backup_led
bl
blue
bluetooth
boot
bottom
brick-status
bt
CEL
chrg
COM
copy
core_module
cpu
D
debug
DIA
disk
disk_led
down
dsl
enocean
enter
err
error
esata
ethernet-status
fail
fault
front
func
function
g
ghz
ghz-1
ghz-2
gpio
green
gsm
HD
hdd
hdderr
health
health_led
heart
heartbeat
home
inet
info
internet
keypad
L
lan
led
ledb
left
l_hdd
live
logo
microSD
misc
mmc
nand
network
on
orange
os
panel
pmu_stat
power
power_led
programming
proximitysensor
pulse
pwr
qss
rebuild_led
red
r_hdd
right
router
rs
rx
sata
sata-l
sata-r
sd
SD
sleep
standby
stat
state
status
Status
sw
sys
system
system-status
tel
top
tv
tx
up
usb
USB
usb_1
usb_2
usb_copy
usb-port1
usb-port2
user
USER
usr
wan
white
wifi
wifi_ap
wifi-status
wireless
wlan
wlan_g
wmode
wps
WPS
yellow

-- 
Best regards,
Jacek Anaszewski