diff mbox series

[RFC,1/2] dt: bindings: as3645a: Update dt node example with standard

Message ID 20171212215024.30116-1-dmurphy@ti.com
State New
Headers show
Series [RFC,1/2] dt: bindings: as3645a: Update dt node example with standard | expand

Commit Message

Dan Murphy Dec. 12, 2017, 9:50 p.m. UTC 
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.

Signed-off-by: Dan Murphy <dmurphy@ti.com>

---
 .../devicetree/bindings/leds/ams,as3645a.txt       | 36 +++++++++++-----------
 1 file changed, 18 insertions(+), 18 deletions(-)

-- 
2.15.0.124.g7668cbc60

Comments

Laurent Pinchart Dec. 13, 2017, 8:09 a.m. UTC | #1 
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.

> 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.

>  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.

> +		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";

> +	};

> +};


-- 
Regards,

Laurent Pinchart
Dan Murphy Dec. 13, 2017, 12:55 p.m. UTC | #2 
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
Laurent Pinchart Dec. 13, 2017, 4:29 p.m. UTC | #3 
Hi Dan,

On Wednesday, 13 December 2017 14:55:03 EET Dan Murphy wrote:
> On 12/13/2017 02:09 AM, Laurent Pinchart wrote:

> > 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


OK, that makes sense to me.

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>


> >> +		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";

> >> +	};

> >> +};


-- 
Regards,

Laurent Pinchart
diff mbox series

Patch

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.
 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 {
+	compatible = "ams,as3645a";
+	#address-cells = <1>;
+	#size-cells = <0>;
+	reg = <0x30>;
+	led@0 {
+		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";
+	};
+};