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 <[email protected]>
---
.../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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> .../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
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 <[email protected]>
>> ---
>> .../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
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 <[email protected]>
> >> ---
> >>
> >> .../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 <[email protected]>
> >> + 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
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 <[email protected]>
> ---
> 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: [email protected]
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
> ---
> .../devicetree/bindings/leds/ams,as3645a.txt | 36 +++++++++++-----------
> 1 file changed, 18 insertions(+), 18 deletions(-)
Otherwise,
Reviewed-by: Rob Herring <[email protected]>