lkml.org 
[lkml]   [2017]   [Dec]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC PATCH 1/2] dt: bindings: as3645a: Update dt node example with standard
From
Date
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 <dmurphy@ti.com>
>> ---
>> .../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

\
 
 \ /
  Last update: 2017-12-13 13:57    [W:0.123 / U:0.140 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site