leds: turris-omnia: updates

PATCH Superseded Marek Behún 3 participants 10 messages Jul 14, 2023 – Aug 1, 2023

Subsystems (2)

LED Arm/cz.nic turris support

lore

Share & tools

lore

Marek Behún

author maintainer

2023-07-14 08:53 UTC #0 lore raw

v2 cover leds: turris-omnia: updates

Hi Pavel, Lee,

:-( I realized it was a year ago when I sent v1 of this series, so I am resending this with a small change (I reversed the order of the patches so that the most complicated patch comes last.).

The summary can be read in the original cover letter [1].

Marek

[1] https://lore.kernel.org/all/20221102002311.twso5wb4tzba5h2k@pali/t/

Marek Behún (3):
leds: turris-omnia: change max brightness from 255 to 1
leds: turris-omnia: initialize multi-intensity to full
leds: turris-omnia: support HW controlled mode via private trigger

 drivers/leds/Kconfig             |  1 +
 drivers/leds/leds-turris-omnia.c | 46 +++++++++++++++++++++++++++++++-
 2 files changed, 46 insertions(+), 1 deletion(-)

Signature · 1 line

2.41.0

Marek Behún

author maintainer

2023-07-14 08:53 UTC #1 lore raw

PATCH v2 1/3 leds: turris-omnia: change max brightness from 255 to 1

Using binary brightness makes more sense for this controller, because internally in the MCU it works that way: the LED has a color, and a state whether it is ON or OFF.

The resulting brightness computation with led_mc_calc_color_components() will now always result in either (0, 0, 0) or the multi_intensity value.

Signed-off-by Marek Behún <[email protected]>

---
 drivers/leds/leds-turris-omnia.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/leds/leds-turris-omnia.c +1 -1

index 64b2d7b6d3f3..c063b6b710f9 100644
@@ -110,7 +110,7 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
 	init_data.fwnode = &np->fwnode;
 
 	cdev = &led->mc_cdev.led_cdev;
-	cdev->max_brightness = 255;
+	cdev->max_brightness = 1;
 	cdev->brightness_set_blocking = omnia_led_brightness_set_blocking;
 
 	/* put the LED into software mode */

Signature · 1 line

2.41.0

Pavel Machek

2023-07-16 09:19 UTC #2 lore raw

Re: [PATCH v2 1/3] leds: turris-omnia: change max brightness from 255 to 1

Hi!

Quoted 3 lines

Using binary brightness makes more sense for this controller, because internally in the MCU it works that way: the LED has a color, and a state whether it is ON or OFF.

So, controller can do (1, 3, 5) but not (3, 3, 3)?

Quoted 2 lines

The resulting brightness computation with led_mc_calc_color_components() will now always result in either (0, 0, 0) or the multi_intensity value.

Won't that limit you to 8 colors total?

I guess I`m confused how this hw works...

Best regards,
Pavel

Quoted 8 lines

drivers/leds/leds-turris-omnia.c +1 -1

 	init_data.fwnode = &np->fwnode;
 
 	cdev = &led->mc_cdev.led_cdev;
-	cdev->max_brightness = 255;
+	cdev->max_brightness = 1;
 	cdev->brightness_set_blocking = omnia_led_brightness_set_blocking;
 
 	/* put the LED into software mode */

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Marek Behún

author maintainer

2023-07-16 15:01 UTC #3 lore raw

Re: [PATCH v2 1/3] leds: turris-omnia: change max brightness from 255 to 1

Quoted 6 lines · On Sun, 16 Jul 2023 11:19:30 +0200 Pavel Machek <[email protected]> wrote:

Hi!
Quoted 3 lines
Using binary brightness makes more sense for this controller, because internally in the MCU it works that way: the LED has a color, and a state whether it is ON or OFF.

Quoted 8 lines

So, controller can do (1, 3, 5) but not (3, 3, 3)?
Quoted 2 lines
The resulting brightness computation with led_mc_calc_color_components() will now always result in either (0, 0, 0) or the multi_intensity value.
Won't that limit you to 8 colors total?
I guess I`m confused how this hw works...

Hi Pavel.

No no no. That's not how it is.

The HW exposes color control for three channels (RGB), each channel with range 0-255 (so 16M colors). The driver exposes this via the multi_intensity sysfs file. This is communicated to the HW via LED_SET_COLOR I2C command.

HW also exposes setting the LED ON and OFF, via the LED_SET_STATE I2C command.

We currently have the following sysfs files via which we set LED state and color:
brightness
multi_intensity

Because currently the driver sets max_brightness to 255, the actual color that is sent to HW is recalculated via led_mc_calc_color_components(). For example with

  $ echo 255 255 100 >multi_intensity
  $ echo 150 >brightness

the led_mc_calc_color_components() function recalculates the channel intensities with formula
brightness * channel_intensity / max_brightness
and so the (255, 255, 100) tuple is converted to (150, 150, 58) before sending to HW.

What I think would make more sense is to make the two sysfs files
brightness
multi_intensity
correspond 1-to-1 with I2C commands LED_SET_STATE and LED_SET_COLOR.

This can be simply done by setting max_brightness to 1. The brightness sysfs file then can simply control whether the LED is ON or OFF. The multi_intensity file control the color.

I realize now that in the patch I should also make away with the call to led_mc_calc_color_components()...

Marek

Lee Jones

maintainer

2023-07-28 09:56 UTC #4 lore raw

Re: [PATCH v2 1/3] leds: turris-omnia: change max brightness from 255 to 1

Quoted 53 lines · Marek Behún

Quoted 6 lines · On Sun, 16 Jul 2023 11:19:30 +0200 Pavel Machek <[email protected]> wrote:
Hi!
Quoted 3 lines
Using binary brightness makes more sense for this controller, because internally in the MCU it works that way: the LED has a color, and a state whether it is ON or OFF.
Quoted 8 lines
So, controller can do (1, 3, 5) but not (3, 3, 3)?
Quoted 2 lines
The resulting brightness computation with led_mc_calc_color_components() will now always result in either (0, 0, 0) or the multi_intensity value.
Won't that limit you to 8 colors total?
I guess I`m confused how this hw works...
Hi Pavel.
No no no. That's not how it is.
The HW exposes color control for three channels (RGB), each channel with range 0-255 (so 16M colors). The driver exposes this via the multi_intensity sysfs file. This is communicated to the HW via LED_SET_COLOR I2C command.
HW also exposes setting the LED ON and OFF, via the LED_SET_STATE I2C command.
We currently have the following sysfs files via which we set LED state and color:
brightness
multi_intensity
Because currently the driver sets max_brightness to 255, the actual color that is sent to HW is recalculated via led_mc_calc_color_components(). For example with
  $ echo 255 255 100 >multi_intensity
  $ echo 150 >brightness
the led_mc_calc_color_components() function recalculates the channel intensities with formula
brightness * channel_intensity / max_brightness
and so the (255, 255, 100) tuple is converted to (150, 150, 58) before sending to HW.
What I think would make more sense is to make the two sysfs files
brightness
multi_intensity
correspond 1-to-1 with I2C commands LED_SET_STATE and LED_SET_COLOR.

Quoted 6 lines

This can be simply done by setting max_brightness to 1. The brightness sysfs file then can simply control whether the LED is ON or OFF. The multi_intensity file control the color.
I realize now that in the patch I should also make away with the call to led_mc_calc_color_components()...

FYI, due to the revelations above, I'm dropping this from my queue.

Signature · 1 line

Lee Jones [李琼斯]

Pavel Machek

2023-07-28 10:10 UTC #5 lore raw

Re: [PATCH v2 1/3] leds: turris-omnia: change max brightness from 255 to 1

In reply to Marek Behún

Hi!

Quoted 17 lines

The HW exposes color control for three channels (RGB), each channel with range 0-255 (so 16M colors). The driver exposes this via the multi_intensity sysfs file. This is communicated to the HW via LED_SET_COLOR I2C command.
HW also exposes setting the LED ON and OFF, via the LED_SET_STATE I2C command.
We currently have the following sysfs files via which we set LED state and color:
brightness
multi_intensity
Because currently the driver sets max_brightness to 255, the actual color that is sent to HW is recalculated via led_mc_calc_color_components(). For example with

Quoted 8 lines

  $ echo 255 255 100 >multi_intensity
  $ echo 150 >brightness
the led_mc_calc_color_components() function recalculates the channel intensities with formula
brightness * channel_intensity / max_brightness
and so the (255, 255, 100) tuple is converted to (150, 150, 58) before sending to HW.

And this seems ok.

Quoted 4 lines

What I think would make more sense is to make the two sysfs files
brightness
multi_intensity
correspond 1-to-1 with I2C commands LED_SET_STATE and LED_SET_COLOR.

We want your driver to be have same API as other drivers, 1-to-1 correspondence with I2c commands is not important.

NAK-ed-by: Pavel

Best regards,
Pavel

Signature · 1 line

People of Russia, stop Putin before his war on Ukraine escalates.

Marek Behún

author maintainer

2023-08-01 09:12 UTC #6 lore raw

Re: [PATCH v2 1/3] leds: turris-omnia: change max brightness from 255 to 1

Quoted 35 lines · On Fri, 28 Jul 2023 12:10:44 +0200 Pavel Machek <[email protected]> wrote:

Hi!
Quoted 17 lines
The HW exposes color control for three channels (RGB), each channel with range 0-255 (so 16M colors). The driver exposes this via the multi_intensity sysfs file. This is communicated to the HW via LED_SET_COLOR I2C command.
HW also exposes setting the LED ON and OFF, via the LED_SET_STATE I2C command.
We currently have the following sysfs files via which we set LED state and color:
brightness
multi_intensity
Because currently the driver sets max_brightness to 255, the actual color that is sent to HW is recalculated via led_mc_calc_color_components(). For example with
Quoted 8 lines
  $ echo 255 255 100 >multi_intensity
  $ echo 150 >brightness
the led_mc_calc_color_components() function recalculates the channel intensities with formula
brightness * channel_intensity / max_brightness
and so the (255, 255, 100) tuple is converted to (150, 150, 58) before sending to HW.
And this seems ok.
Quoted 4 lines
What I think would make more sense is to make the two sysfs files
brightness
multi_intensity
correspond 1-to-1 with I2C commands LED_SET_STATE and LED_SET_COLOR.

Quoted 7 lines

We want your driver to be have same API as other drivers, 1-to-1 correspondence with I2c commands is not important.
NAK-ed-by: Pavel
Best regards,
Pavel

Hmm, thinking more about it I guess you are right. And I thought of what I think is better change anyway. I shall send patch for review, let's see what you think about that one :)

Marek

Marek Behún

author maintainer

2023-07-14 08:53 UTC #7 lore raw

PATCH v2 2/3 leds: turris-omnia: initialize multi-intensity to full

In reply to Marek Behún

The default color of each LED before driver probe is (255, 255, 255). Initialize multi_intensity to this value, so that it corresponds to the reality.

Signed-off-by Marek Behún <[email protected]>

---
 drivers/leds/leds-turris-omnia.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/leds/leds-turris-omnia.c +3 -0

index c063b6b710f9..7977c0372667 100644
@@ -98,10 +98,13 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
 	}
 
 	led->subled_info[0].color_index = LED_COLOR_ID_RED;
+	led->subled_info[0].intensity = 255;
 	led->subled_info[0].channel = 0;
 	led->subled_info[1].color_index = LED_COLOR_ID_GREEN;
+	led->subled_info[1].intensity = 255;
 	led->subled_info[1].channel = 1;
 	led->subled_info[2].color_index = LED_COLOR_ID_BLUE;
+	led->subled_info[2].intensity = 255;
 	led->subled_info[2].channel = 2;
 
 	led->mc_cdev.subled_info = led->subled_info;

Signature · 1 line

2.41.0

Pavel Machek

Reviewed-by on v2 2/3

2023-07-16 09:19 UTC #8 lore raw

Re: [PATCH v2 2/3] leds: turris-omnia: initialize multi-intensity to full

Quoted 5 lines · Marek Beh??n

The default color of each LED before driver probe is (255, 255, 255). Initialize multi_intensity to this value, so that it corresponds to the reality.
Signed-off-by Marek Beh??n <[email protected]>

Reviewed-by Pavel Machek <[email protected]>

Quoted 17 lines

+++ b/drivers/leds/leds-turris-omnia.c

drivers/leds/leds-turris-omnia.c +3 -0

@@ -98,10 +98,13 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
 	}
 
 	led->subled_info[0].color_index = LED_COLOR_ID_RED;
+	led->subled_info[0].intensity = 255;
 	led->subled_info[0].channel = 0;
 	led->subled_info[1].color_index = LED_COLOR_ID_GREEN;
+	led->subled_info[1].intensity = 255;
 	led->subled_info[1].channel = 1;
 	led->subled_info[2].color_index = LED_COLOR_ID_BLUE;
+	led->subled_info[2].intensity = 255;
 	led->subled_info[2].channel = 2;
 
 	led->mc_cdev.subled_info = led->subled_info;

Signature · 1 line

2.41.0

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Marek Behún

author maintainer

2023-07-14 08:53 UTC #9 lore raw

PATCH v2 3/3 leds: turris-omnia: support HW controlled mode via private trigger

In reply to Marek Behún

Add support for enabling MCU controlled mode of the Turris Omnia LEDs via a LED private trigger called "omnia-mcu".

When in MCU controlled mode, the user can still set LED color, but the blinking is done by MCU, which does different things for various LEDs:
- WAN LED is blinked according to the LED[0] pin of the WAN PHY
- LAN LEDs are blinked according to the LED[0] output of corresponding port of the LAN switch
- PCIe LEDs are blinked according to the logical OR of the MiniPCIe port LED pins

For a long time I wanted to actually do this differently: I wanted to make the netdev trigger to transparently offload the blinking to the HW if user set compatible settings for the netdev trigger. There was some work on this, and hopefully we will be able to complete it sometime, but since there are various complications, it will probably not be soon.

In the meantime let's support HW controlled mode via this private LED trigger. If, in the future, we manage to complete the netdev trigger offloading, we can still keep this private trigger for backwards compatibility, if needed.

We also set "omnia-mcu" to cdev->default_trigger, so that the MCU keeps control until the user first wants to take over it. If a different default trigger is specified in device-tree via the linux,default-trigger property, LED class will overwrite cdev->default_trigger, and so the DT property will be respected.

Signed-off-by Marek Behún <[email protected]>

---
 drivers/leds/Kconfig             |  1 +
 drivers/leds/leds-turris-omnia.c | 41 ++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

drivers/leds/Kconfig +1 -0

index 6046dfeca16f..ebb3b84d7a4f 100644
@@ -187,6 +187,7 @@ config LEDS_TURRIS_OMNIA
 	depends on I2C
 	depends on MACH_ARMADA_38X || COMPILE_TEST
 	depends on OF
+	select LEDS_TRIGGERS
 	help
 	  This option enables basic support for the LEDs found on the front
 	  side of CZ.NIC's Turris Omnia router. There are 12 RGB LEDs on the

drivers/leds/leds-turris-omnia.c +41 -0

59 lines not on this page · raw message

Signature · 1 line

2.41.0