Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753222AbdLMM5F (ORCPT ); Wed, 13 Dec 2017 07:57:05 -0500 Received: from fllnx209.ext.ti.com ([198.47.19.16]:29117 "EHLO fllnx209.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753057AbdLMM5D (ORCPT ); Wed, 13 Dec 2017 07:57:03 -0500 Subject: Re: [RFC PATCH 1/2] dt: bindings: as3645a: Update dt node example with standard To: Laurent Pinchart CC: , , , , , , , , References: <20171212215024.30116-1-dmurphy@ti.com> <4192002.yKJgEXTVKE@avalon> From: Dan Murphy Message-ID: <90a1c908-3170-6956-8983-f9d88234cc07@ti.com> Date: Wed, 13 Dec 2017 06:55:03 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: <4192002.yKJgEXTVKE@avalon> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit X-EXCLAIMER-MD-CONFIG: e1e8a2fd-e40a-4ac6-ac9b-f7e9cc9ee180 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3162 Lines: 109 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 >> --- >> .../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