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

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

[Dan Murphy]

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

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";
+	};
+};
-- 
2.15.0.124.g7668cbc60

[RFC PATCH 2/2] leds: as3645a: Update LED label generation

[Dan Murphy]

Generate the LED label based off either the
DT label node or off the I2C ID in the
i2c device id struct.

If the label is used then this should denote
the LED function.  As an example it would
be as3645a:<function>

 Otherwise if the label is not
used the LED label will be as3645a:flash and
as3645a:indicator.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 drivers/leds/leds-as3645a.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/leds/leds-as3645a.c b/drivers/leds/leds-as3645a.c
index f883616d9e60..197acd69ddcc 100644
--- a/drivers/leds/leds-as3645a.c
+++ b/drivers/leds/leds-as3645a.c
@@ -526,10 +526,11 @@ static int as3645a_parse_node(struct as3645a *flash,
 
 	rval = of_property_read_string(flash->flash_node, "label", &name);
 	if (!rval)
-		strlcpy(names->flash, name, sizeof(names->flash));
+		snprintf(names->flash, sizeof(names->flash), "%s:%s",
+			 id->name, name);
 	else
 		snprintf(names->flash, sizeof(names->flash),
-			 "%s:flash", node->name);
+			 "%s:flash", id->name);
 
 	rval = of_property_read_u32(flash->flash_node, "flash-timeout-us",
 				    &cfg->flash_timeout_us);
@@ -570,10 +571,11 @@ static int as3645a_parse_node(struct as3645a *flash,
 
 	rval = of_property_read_string(flash->indicator_node, "label", &name);
 	if (!rval)
-		strlcpy(names->indicator, name, sizeof(names->indicator));
+		snprintf(names->indicator, sizeof(names->indicator), "%s:%s",
+			 id->name, name);
 	else
 		snprintf(names->indicator, sizeof(names->indicator),
-			 "%s:indicator", node->name);
+			 "%s:indicator", id->name);
 
 	rval = of_property_read_u32(flash->indicator_node, "led-max-microamp",
 				    &cfg->indicator_max_ua);
-- 
2.15.0.124.g7668cbc60

Re: [RFC PATCH 2/2] leds: as3645a: Update LED label generation

[Sakari Ailus]

Hi Dan,

Thanks for the patchset.

On Tue, Dec 12, 2017 at 03:50:24PM -0600, Dan Murphy wrote:
> Generate the LED label based off either the
> DT label node or off the I2C ID in the
> i2c device id struct.
> 
> If the label is used then this should denote
> the LED function.  As an example it would
> be as3645a:<function>
> 
>  Otherwise if the label is not
> used the LED label will be as3645a:flash and
> as3645a:indicator.

Which tree the patch is for? I see "id" is a u32 in what I have, and
also not defined in this scope.

> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
>  drivers/leds/leds-as3645a.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/leds/leds-as3645a.c b/drivers/leds/leds-as3645a.c
> index f883616d9e60..197acd69ddcc 100644
> --- a/drivers/leds/leds-as3645a.c
> +++ b/drivers/leds/leds-as3645a.c
> @@ -526,10 +526,11 @@ static int as3645a_parse_node(struct as3645a *flash,
>  
>  	rval = of_property_read_string(flash->flash_node, "label", &name);
>  	if (!rval)
> -		strlcpy(names->flash, name, sizeof(names->flash));
> +		snprintf(names->flash, sizeof(names->flash), "%s:%s",
> +			 id->name, name);
>  	else
>  		snprintf(names->flash, sizeof(names->flash),
> -			 "%s:flash", node->name);
> +			 "%s:flash", id->name);
>  
>  	rval = of_property_read_u32(flash->flash_node, "flash-timeout-us",
>  				    &cfg->flash_timeout_us);
> @@ -570,10 +571,11 @@ static int as3645a_parse_node(struct as3645a *flash,
>  
>  	rval = of_property_read_string(flash->indicator_node, "label", &name);
>  	if (!rval)
> -		strlcpy(names->indicator, name, sizeof(names->indicator));
> +		snprintf(names->indicator, sizeof(names->indicator), "%s:%s",
> +			 id->name, name);
>  	else
>  		snprintf(names->indicator, sizeof(names->indicator),
> -			 "%s:indicator", node->name);
> +			 "%s:indicator", id->name);
>  
>  	rval = of_property_read_u32(flash->indicator_node, "led-max-microamp",
>  				    &cfg->indicator_max_ua);



-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [RFC PATCH 2/2] leds: as3645a: Update LED label generation

[Dan Murphy]

Sakari

On 12/14/2017 12:03 PM, Sakari Ailus wrote:
> Hi Dan,
> 
> Thanks for the patchset.
> 
> On Tue, Dec 12, 2017 at 03:50:24PM -0600, Dan Murphy wrote:
>> Generate the LED label based off either the
>> DT label node or off the I2C ID in the
>> i2c device id struct.
>>
>> If the label is used then this should denote
>> the LED function.  As an example it would
>> be as3645a:<function>
>>
>>  Otherwise if the label is not
>> used the LED label will be as3645a:flash and
>> as3645a:indicator.
> 
> Which tree the patch is for? I see "id" is a u32 in what I have, and
> also not defined in this scope.

Ah.  I did not notice that the driver used the probe_new call back.

I will be re-sending anyway since this is a RFC

Dan

> 
>>
>> Signed-off-by: Dan Murphy <dmurphy-l0cyMroinI0@public.gmane.org>
>> ---
>>  drivers/leds/leds-as3645a.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/leds/leds-as3645a.c b/drivers/leds/leds-as3645a.c
>> index f883616d9e60..197acd69ddcc 100644
>> --- a/drivers/leds/leds-as3645a.c
>> +++ b/drivers/leds/leds-as3645a.c
>> @@ -526,10 +526,11 @@ static int as3645a_parse_node(struct as3645a *flash,
>>  
>>  	rval = of_property_read_string(flash->flash_node, "label", &name);
>>  	if (!rval)
>> -		strlcpy(names->flash, name, sizeof(names->flash));
>> +		snprintf(names->flash, sizeof(names->flash), "%s:%s",
>> +			 id->name, name);
>>  	else
>>  		snprintf(names->flash, sizeof(names->flash),
>> -			 "%s:flash", node->name);
>> +			 "%s:flash", id->name);
>>  
>>  	rval = of_property_read_u32(flash->flash_node, "flash-timeout-us",
>>  				    &cfg->flash_timeout_us);
>> @@ -570,10 +571,11 @@ static int as3645a_parse_node(struct as3645a *flash,
>>  
>>  	rval = of_property_read_string(flash->indicator_node, "label", &name);
>>  	if (!rval)
>> -		strlcpy(names->indicator, name, sizeof(names->indicator));
>> +		snprintf(names->indicator, sizeof(names->indicator), "%s:%s",
>> +			 id->name, name);
>>  	else
>>  		snprintf(names->indicator, sizeof(names->indicator),
>> -			 "%s:indicator", node->name);
>> +			 "%s:indicator", id->name);
>>  
>>  	rval = of_property_read_u32(flash->indicator_node, "led-max-microamp",
>>  				    &cfg->indicator_max_ua);
> 
> 
> 


-- 
------------------
Dan Murphy
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Laurent Pinchart]

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

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

[Dan Murphy]

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

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

[Laurent Pinchart]

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

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

[Rob Herring]

On Tue, Dec 12, 2017 at 03:50:23PM -0600, 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.

The subject seems incomplete "... with standard ?"

> 
> Also removed the indentation on the example.
> 
> Signed-off-by: Dan Murphy <dmurphy-l0cyMroinI0@public.gmane.org>
> ---
>  .../devicetree/bindings/leds/ams,as3645a.txt       | 36 +++++++++++-----------
>  1 file changed, 18 insertions(+), 18 deletions(-)

Otherwise,

Reviewed-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html