| Message ID | 20210119105312.2636-1-schuchmann@schleissheimer.de |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] leds: lp50xx: add setting of default intensity from DT | expand |
Hi, sorry to ask but was someone able to look at this? Any thoughts? Best Regards, Sven > -----Ursprüngliche Nachricht----- > Von: Sven Schuchmann <schuchmann@schleissheimer.de> > Gesendet: Dienstag, 19. Januar 2021 11:53 > An: Sven Schuchmann <schuchmann@schleissheimer.de> > Cc: Pavel Machek <pavel@ucw.cz>; Dan Murphy <dmurphy@ti.com>; Rob Herring <robh+dt@kernel.org>; linux- > leds@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org > Betreff: [PATCH v1] leds: lp50xx: add setting of default intensity from DT > > In order to use a multicolor-led together with a trigger > from DT the led needs to have an intensity set to see something. > The trigger changes the brightness of the led but if there > is no intensity we actually see nothing. > > This patch adds the ability to set the default intensity > of each led so that it is turned on from DT. > > Signed-off-by: Sven Schuchmann <schuchmann@schleissheimer.de> > --- > Documentation/devicetree/bindings/leds/leds-lp50xx.yaml | 8 +++++++- > drivers/leds/leds-lp50xx.c | 4 ++++ > 2 files changed, 11 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml b/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml > index c192b5feadc7..5ad2a0c3c052 100644 > --- a/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml > +++ b/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml > @@ -69,7 +69,12 @@ patternProperties: > patternProperties: > "(^led-[0-9a-f]$|led)": > type: object > - $ref: common.yaml# > + allOf: > + - $ref: common.yaml# > + properties: > + default-intensity: > + maxItems: 1 > + description: The intensity the LED gets initialised with. > > required: > - compatible > @@ -102,6 +107,7 @@ examples: > > led-0 { > color = <LED_COLOR_ID_RED>; > + default-intensity = <100>; > }; > > led-1 { > diff --git a/drivers/leds/leds-lp50xx.c b/drivers/leds/leds-lp50xx.c > index f13117eed976..ba760fa33bdc 100644 > --- a/drivers/leds/leds-lp50xx.c > +++ b/drivers/leds/leds-lp50xx.c > @@ -501,6 +501,10 @@ static int lp50xx_probe_dt(struct lp50xx *priv) > } > > mc_led_info[num_colors].color_index = color_id; > + > + fwnode_property_read_u32(led_node, "default-intensity", > + &mc_led_info[num_colors].intensity); > + > num_colors++; > } > > -- > 2.17.1
Hi Sven, On 1/19/21 11:53 AM, Sven Schuchmann wrote: > In order to use a multicolor-led together with a trigger > from DT the led needs to have an intensity set to see something. > The trigger changes the brightness of the led but if there > is no intensity we actually see nothing. > > This patch adds the ability to set the default intensity > of each led so that it is turned on from DT. > > Signed-off-by: Sven Schuchmann <schuchmann@schleissheimer.de> > --- > Documentation/devicetree/bindings/leds/leds-lp50xx.yaml | 8 +++++++- > drivers/leds/leds-lp50xx.c | 4 ++++ > 2 files changed, 11 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml b/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml > index c192b5feadc7..5ad2a0c3c052 100644 > --- a/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml > +++ b/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml > @@ -69,7 +69,12 @@ patternProperties: > patternProperties: > "(^led-[0-9a-f]$|led)": > type: object > - $ref: common.yaml# > + allOf: > + - $ref: common.yaml# > + properties: > + default-intensity: > + maxItems: 1 > + description: The intensity the LED gets initialised with. > > required: > - compatible > @@ -102,6 +107,7 @@ examples: > > led-0 { > color = <LED_COLOR_ID_RED>; > + default-intensity = <100>; > }; > > led-1 { Please split this into a separate patch, preceding this one in the series. > diff --git a/drivers/leds/leds-lp50xx.c b/drivers/leds/leds-lp50xx.c > index f13117eed976..ba760fa33bdc 100644 > --- a/drivers/leds/leds-lp50xx.c > +++ b/drivers/leds/leds-lp50xx.c > @@ -501,6 +501,10 @@ static int lp50xx_probe_dt(struct lp50xx *priv) > } > > mc_led_info[num_colors].color_index = color_id; > + > + fwnode_property_read_u32(led_node, "default-intensity", > + &mc_led_info[num_colors].intensity); > + > num_colors++; > } > > For this part: Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com> -- Best regards, Jacek Anaszewski
Hi! > In order to use a multicolor-led together with a trigger > from DT the led needs to have an intensity set to see something. > The trigger changes the brightness of the led but if there > is no intensity we actually see nothing. > > This patch adds the ability to set the default intensity > of each led so that it is turned on from DT. Do we need this to be configurable from device tree? Can we just set it to max or something? Aha, this basically sets the initial color for LEDs the monochromatic triggers, right? Pavel -- http://www.livejournal.com/~pavelmachek
On Wed 2021-02-03 15:39:59, Sven Schuchmann wrote: > Hello Pavel, > > > > In order to use a multicolor-led together with a trigger > > > from DT the led needs to have an intensity set to see something. > > > The trigger changes the brightness of the led but if there > > > is no intensity we actually see nothing. > > > > > > This patch adds the ability to set the default intensity > > > of each led so that it is turned on from DT. > > > > Do we need this to be configurable from device tree? Can we just set > > it to max or something? > > > > Aha, this basically sets the initial color for LEDs the monochromatic > > triggers, right? > > Let me try to explain in other words: I have one RGB-LED > which consists of 3 Colors. Each of the three colors (Red, Green, Blue) you have > to define in the DT. For example this is my setup for one RGB-LED which I wanted > to show the heartbeat in Red (half intensity): > > multi-led@3 { > #address-cells = <1>; > #size-cells = <0>; > reg = <0x3>; > color = <LED_COLOR_ID_RGB>; > > linux,default-trigger = "heartbeat"; > function = LED_FUNCTION_HEARTBEAT; > > led-9 { > color = <LED_COLOR_ID_RED>; > default-intensity = <100>; > }; > > led-10 { > color = <LED_COLOR_ID_GREEN>; > }; > > led-11 { > color = <LED_COLOR_ID_BLUE>; > }; > }; > > If I would not have the default-intensity I would actually see nothing, > since the intensity (which goes from 0-255) of each led is initialized with 0. > > I hope I could clarify this a little more? Yes, sounds reasonable. Could we get default intensity of 100% on all channels if nothing else is specified? Or maybe simply "if intensity is not specified, start with 100%, and use explicit =0 if other color is expected". Best regards, Pavel
On Wed, 3 Feb 2021 17:35:55 +0100 Pavel Machek <pavel@ucw.cz> wrote: > On Wed 2021-02-03 15:39:59, Sven Schuchmann wrote: > > Hello Pavel, > > > > > > In order to use a multicolor-led together with a trigger > > > > from DT the led needs to have an intensity set to see something. > > > > The trigger changes the brightness of the led but if there > > > > is no intensity we actually see nothing. > > > > > > > > This patch adds the ability to set the default intensity > > > > of each led so that it is turned on from DT. > > > > > > Do we need this to be configurable from device tree? Can we just set > > > it to max or something? > > > > > > Aha, this basically sets the initial color for LEDs the monochromatic > > > triggers, right? > > > > Let me try to explain in other words: I have one RGB-LED > > which consists of 3 Colors. Each of the three colors (Red, Green, Blue) you have > > to define in the DT. For example this is my setup for one RGB-LED which I wanted > > to show the heartbeat in Red (half intensity): > > > > multi-led@3 { > > #address-cells = <1>; > > #size-cells = <0>; > > reg = <0x3>; > > color = <LED_COLOR_ID_RGB>; > > > > linux,default-trigger = "heartbeat"; > > function = LED_FUNCTION_HEARTBEAT; > > > > led-9 { > > color = <LED_COLOR_ID_RED>; > > default-intensity = <100>; > > }; > > > > led-10 { > > color = <LED_COLOR_ID_GREEN>; > > }; > > > > led-11 { > > color = <LED_COLOR_ID_BLUE>; > > }; > > }; > > > > If I would not have the default-intensity I would actually see nothing, > > since the intensity (which goes from 0-255) of each led is initialized with 0. > > > > I hope I could clarify this a little more? > > Yes, sounds reasonable. Could we get default intensity of 100% on all > channels if nothing else is specified? > > Or maybe simply "if intensity is not specified, start with 100%, and > use explicit =0 if other color is expected". > > Best regards, > Pavel Is the property default-intensity documented in DT bindings? Wouldn't it be better if the property was used in the multi-led node instead of the channel node? I.e. multi-led@3 { color = <LED_COLOR_ID_RGB>; default-intensity = <100 0 0>; };
Hi! > > Yes, sounds reasonable. Could we get default intensity of 100% on all > > channels if nothing else is specified? > > > > Or maybe simply "if intensity is not specified, start with 100%, and > > use explicit =0 if other color is expected". > > > Mh, if someone is already using the led driver and updates to a newer kernel > we would then turn on all leds per default to the maximum intensity during boot > until they are set the way they should be from userspace. I don't know if this > is what we want? If yes, sure, we could set them to maximum per > default. Not really. If they don't have trigger configured, nothing will happen. > Also if we want to use Percentage Values (%) for setting the intensity > I think this should also be done for the userspace interfaces and > not only from DT. We don't want to use percentages in the API (but let me still use percentages in discussion). Best regards, Pavel
Hello Pavel, hello Marek > > Is the property default-intensity documented in DT bindings? I updated the documentation in leds-lp50xx.yaml. Is it this what you mean? > > Wouldn't it be better if the property was used in the multi-led node > > instead of the channel node? I.e. > > multi-led@3 { > > color = <LED_COLOR_ID_RGB>; > > default-intensity = <100 0 0>; > > }; > > Yes, this would be better. Sound good, I will see what I can do here. Regards, Sven
Helo Pavel, > > > Yes, sounds reasonable. Could we get default intensity of 100% on all > > > channels if nothing else is specified? > > > > > > Or maybe simply "if intensity is not specified, start with 100%, and > > > use explicit =0 if other color is expected". > > > > > Mh, if someone is already using the led driver and updates to a newer kernel > > we would then turn on all leds per default to the maximum intensity during boot > > until they are set the way they should be from userspace. I don't know if this > > is what we want? If yes, sure, we could set them to maximum per > > default. > > Not really. If they don't have trigger configured, nothing will happen. Oops, you are right, my fault. > > > Also if we want to use Percentage Values (%) for setting the intensity > > I think this should also be done for the userspace interfaces and > > not only from DT. > > We don't want to use percentages in the API (but let me still use > percentages in discussion). No Problem. Best Regards, Sven
diff --git a/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml b/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml index c192b5feadc7..5ad2a0c3c052 100644 --- a/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml +++ b/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml @@ -69,7 +69,12 @@ patternProperties: patternProperties: "(^led-[0-9a-f]$|led)": type: object - $ref: common.yaml# + allOf: + - $ref: common.yaml# + properties: + default-intensity: + maxItems: 1 + description: The intensity the LED gets initialised with. required: - compatible @@ -102,6 +107,7 @@ examples: led-0 { color = <LED_COLOR_ID_RED>; + default-intensity = <100>; }; led-1 { diff --git a/drivers/leds/leds-lp50xx.c b/drivers/leds/leds-lp50xx.c index f13117eed976..ba760fa33bdc 100644 --- a/drivers/leds/leds-lp50xx.c +++ b/drivers/leds/leds-lp50xx.c @@ -501,6 +501,10 @@ static int lp50xx_probe_dt(struct lp50xx *priv) } mc_led_info[num_colors].color_index = color_id; + + fwnode_property_read_u32(led_node, "default-intensity", + &mc_led_info[num_colors].intensity); + num_colors++; }
In order to use a multicolor-led together with a trigger from DT the led needs to have an intensity set to see something. The trigger changes the brightness of the led but if there is no intensity we actually see nothing. This patch adds the ability to set the default intensity of each led so that it is turned on from DT. Signed-off-by: Sven Schuchmann <schuchmann@schleissheimer.de> --- Documentation/devicetree/bindings/leds/leds-lp50xx.yaml | 8 +++++++- drivers/leds/leds-lp50xx.c | 4 ++++ 2 files changed, 11 insertions(+), 1 deletion(-)