devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Dan Murphy <dmurphy@ti.com>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: robh+dt@kernel.org, mark.rutland@arm.com, rpurdie@rpsys.net,
	jacek.anaszewski@gmail.com, pavel@ucw.cz, sakari.ailus@iki.fi,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-leds@vger.kernel.org
Subject: Re: [RFC PATCH 1/2] dt: bindings: as3645a: Update dt node example with standard
Date: Wed, 13 Dec 2017 06:55:03 -0600	[thread overview]
Message-ID: <90a1c908-3170-6956-8983-f9d88234cc07@ti.com> (raw)
In-Reply-To: <4192002.yKJgEXTVKE@avalon>

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

  reply	other threads:[~2017-12-13 12:55 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-12-12 21:50 [RFC PATCH 1/2] dt: bindings: as3645a: Update dt node example with standard Dan Murphy
2017-12-12 21:50 ` [RFC PATCH 2/2] leds: as3645a: Update LED label generation Dan Murphy
2017-12-14 18:03   ` Sakari Ailus
     [not found]     ` <20171214180335.j6qk2rrge77gpbba-S+BSfZ9RZZmRSg0ZkenSGLdO1Tsj/99ntUK59QYPAWc@public.gmane.org>
2017-12-14 19:13       ` Dan Murphy
2017-12-13  8:09 ` [RFC PATCH 1/2] dt: bindings: as3645a: Update dt node example with standard Laurent Pinchart
2017-12-13 12:55   ` Dan Murphy [this message]
2017-12-13 16:29     ` Laurent Pinchart
     [not found] ` <20171212215024.30116-1-dmurphy-l0cyMroinI0@public.gmane.org>
2017-12-15 22:54   ` Rob Herring

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=90a1c908-3170-6956-8983-f9d88234cc07@ti.com \
    --to=dmurphy@ti.com \
    --cc=devicetree@vger.kernel.org \
    --cc=jacek.anaszewski@gmail.com \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-leds@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=pavel@ucw.cz \
    --cc=robh+dt@kernel.org \
    --cc=rpurdie@rpsys.net \
    --cc=sakari.ailus@iki.fi \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).