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 <[email protected]>
---
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,
sorry to ask but was someone able to look at this? Any thoughts?
Best Regards,
Sven
> -----Urspr?ngliche Nachricht-----
> Von: Sven Schuchmann <[email protected]>
> Gesendet: Dienstag, 19. Januar 2021 11:53
> An: Sven Schuchmann <[email protected]>
> Cc: Pavel Machek <[email protected]>; Dan Murphy <[email protected]>; Rob Herring <[email protected]>; linux-
> [email protected]; [email protected]; [email protected]
> 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 <[email protected]>
> ---
> 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 <[email protected]>
> ---
> 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 <[email protected]>
--
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
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?
Best Regards,
Sven
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
--
http://www.livejournal.com/~pavelmachek
Hello 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.
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.
So at this time setting the RGB-LED to white is done like this:
echo "255 255 255" > /sys/class/leds/rgb\:led/multi_intensity
would then get
echo "100 100 100" > /sys/class/leds/rgb\:led/multi_intensity
Again over here: If we would change it would break earlier Applications
from userspace. Just my thoughts. What do you think?
Best Regards,
Sven
On Wed, 3 Feb 2021 17:35:55 +0100
Pavel Machek <[email protected]> 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!
> > > 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>;
> };
Yes, this would be better.
--
http://www.livejournal.com/~pavelmachek
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
--
http://www.livejournal.com/~pavelmachek
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