| Message ID | 20171212215024.30116-1-dmurphy@ti.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [RFC,1/2] dt: bindings: as3645a: Update dt node example with standard | expand |
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"; > + }; > +};
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"; >> + }; >> +}; >
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"; > >> + }; > >> +};
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 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@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); > > >
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@ti.com> > --- > .../devicetree/bindings/leds/ams,as3645a.txt | 36 +++++++++++----------- > 1 file changed, 18 insertions(+), 18 deletions(-) Otherwise, Reviewed-by: Rob Herring <robh@kernel.org> -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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"; + }; +};
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(-)