Laurent On 12/13/2017 02:09 AM, Laurent Pinchart wrote: > Hi Dan, > > Thank you for the patch. > > On Tuesday, 12 December 2017 23:50:23 EET Dan Murphy wrote: >> Update the DT binding to remove the device name from >> the DT parent node as well as removing the device >> name from the label. The LED label will be generated >> based off the id name stored in the local driver so >> the LED function can be indicated in the label DT >> entry. >> >> Also removed the indentation on the example. > > This makes the patch a bit harder to review and seems to be a matter of style. >
I debated whether to remove the extra tabs. The changes below came from comments from a recent LED driver I submitted. >> Signed-off-by: Dan Murphy <[email protected]> >> --- >> .../devicetree/bindings/leds/ams,as3645a.txt | 36 ++++++++++--------- >> 1 file changed, 18 insertions(+), 18 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/leds/ams,as3645a.txt >> b/Documentation/devicetree/bindings/leds/ams,as3645a.txt index >> fc7f5f9f234c..122aa7165cf3 100644 >> --- a/Documentation/devicetree/bindings/leds/ams,as3645a.txt >> +++ b/Documentation/devicetree/bindings/leds/ams,as3645a.txt >> @@ -58,22 +58,22 @@ label : The label of the indicator LED. > > I believe you should expand the documentation of the label property to detail > how it should be formed. It's nice to update the example, but the bindings > should be understandable without it. OK. I will add a reference to Documentation/devicetree/bindings/leds/common.txt label formation will be undergoing some changes. I wanted to make sure there were some good examples in the LED tree for other developers to reference. > >> Example >> ======= >> >> - as3645a@30 { >> - compatible = "ams,as3645a"; >> - #address-cells = <1>; >> - #size-cells = <0>; >> - reg = <0x30>; >> - flash@0 { >> - reg = <0x0>; >> - flash-timeout-us = <150000>; >> - flash-max-microamp = <320000>; >> - led-max-microamp = <60000>; >> - ams,input-max-microamp = <1750000>; >> - label = "as3645a:flash"; >> - }; >> - indicator@1 { >> - reg = <0x1>; >> - led-max-microamp = <10000>; >> - label = "as3645a:indicator"; >> - }; >> +led-controller@30 { > > This change looks fine to me. > >> + compatible = "ams,as3645a"; >> + #address-cells = <1>; >> + #size-cells = <0>; >> + reg = <0x30>; >> + led@0 { > > What's the rationale for changing the node name here ? It should be explained > in the commit message, and in the DT bindings documentation. In my patch to the DT maintainers Rob H indicated "Actually, it should be led-controller and led or leds be used for the LED child nodes (and gpio-led or pwd-led bindings)" Here is the patch that the node naming conventions took place https://patchwork.kernel.org/patch/10093757 > >> + reg = <0x0>; >> + flash-timeout-us = <150000>; >> + flash-max-microamp = <320000>; >> + led-max-microamp = <60000>; >> + ams,input-max-microamp = <1750000>; >> + label = "flash"; >> }; >> + led@1 { >> + reg = <0x1>; >> + led-max-microamp = <10000>; >> + label = "indicator"; >> + }; >> +}; > -- ------------------ Dan Murphy
