From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Dan Murphy <dmurphy@ti.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 18:29:29 +0200 [thread overview]
Message-ID: <2178454.bfcIRroSfr@avalon> (raw)
In-Reply-To: <90a1c908-3170-6956-8983-f9d88234cc07@ti.com>
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
next prev parent reply other threads:[~2017-12-13 16:29 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
2017-12-13 16:29 ` Laurent Pinchart [this message]
[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=2178454.bfcIRroSfr@avalon \
--to=laurent.pinchart@ideasonboard.com \
--cc=devicetree@vger.kernel.org \
--cc=dmurphy@ti.com \
--cc=jacek.anaszewski@gmail.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).