[PATCH v2 0/3] leds: turris-omnia: updates

[PATCH v2 0/3] leds: turris-omnia: updates

[Marek Behún]

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(-)

-- 
2.41.0

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

[Marek Behún]

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 <kabel@kernel.org>
---
 drivers/leds/leds-turris-omnia.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c
index 64b2d7b6d3f3..c063b6b710f9 100644
--- a/drivers/leds/leds-turris-omnia.c
+++ b/drivers/leds/leds-turris-omnia.c
@@ -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 */
-- 
2.41.0

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

[Pavel Machek]

Hi!

> 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)?

> 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

>  	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 */
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[Marek Behún]

On Sun, 16 Jul 2023 11:19:30 +0200
Pavel Machek <pavel@ucw.cz> wrote:

> Hi!
> 
> > 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)?
> 
> > 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

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

[Lee Jones]

On Sun, 16 Jul 2023, Marek Behún wrote:

> On Sun, 16 Jul 2023 11:19:30 +0200
> Pavel Machek <pavel@ucw.cz> wrote:
> 
> > Hi!
> > 
> > > 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)?
> > 
> > > 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()...

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

-- 
Lee Jones [李琼斯]

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1404 bytes --]

Hi!

> 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.

And this seems ok.

> 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
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[Marek Behún]

On Fri, 28 Jul 2023 12:10:44 +0200
Pavel Machek <pavel@ucw.cz> wrote:

> Hi!
> 
> > 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.  
> 
> And this seems ok.
> 
> > 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

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

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

[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 <kabel@kernel.org>
---
 drivers/leds/leds-turris-omnia.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c
index c063b6b710f9..7977c0372667 100644
--- a/drivers/leds/leds-turris-omnia.c
+++ b/drivers/leds/leds-turris-omnia.c
@@ -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;
-- 
2.41.0

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

[Pavel Machek]

On Fri 2023-07-14 10:52:52, Marek Beh??n wrote:
> 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 <kabel@kernel.org>

Reviewed-by: Pavel Machek <pavel@ucw.cz>

> +++ b/drivers/leds/leds-turris-omnia.c
> @@ -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;
> -- 
> 2.41.0

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

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

[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 <kabel@kernel.org>
---
 drivers/leds/Kconfig             |  1 +
 drivers/leds/leds-turris-omnia.c | 41 ++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 6046dfeca16f..ebb3b84d7a4f 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -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
diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c
index 7977c0372667..6ce66e79096c 100644
--- a/drivers/leds/leds-turris-omnia.c
+++ b/drivers/leds/leds-turris-omnia.c
@@ -41,6 +41,39 @@ struct omnia_leds {
 	struct omnia_led leds[];
 };
 
+static struct led_hw_trigger_type omnia_hw_trigger_type;
+
+static int omnia_hwtrig_activate(struct led_classdev *cdev)
+{
+	struct omnia_leds *leds = dev_get_drvdata(cdev->dev->parent);
+	struct omnia_led *led = to_omnia_led(lcdev_to_mccdev(cdev));
+
+	/* put the LED into MCU controlled mode */
+	return i2c_smbus_write_byte_data(leds->client, CMD_LED_MODE,
+					 CMD_LED_MODE_LED(led->reg));
+}
+
+static void omnia_hwtrig_deactivate(struct led_classdev *cdev)
+{
+	struct omnia_leds *leds = dev_get_drvdata(cdev->dev->parent);
+	struct omnia_led *led = to_omnia_led(lcdev_to_mccdev(cdev));
+	int ret;
+
+	/* put the LED into software mode */
+	ret = i2c_smbus_write_byte_data(leds->client, CMD_LED_MODE,
+					CMD_LED_MODE_LED(led->reg) |
+					CMD_LED_MODE_USER);
+	if (ret < 0)
+		dev_err(cdev->dev, "Cannot put to software mode: %i\n", ret);
+}
+
+static struct led_trigger omnia_hw_trigger = {
+	.name		= "omnia-mcu",
+	.activate	= omnia_hwtrig_activate,
+	.deactivate	= omnia_hwtrig_deactivate,
+	.trigger_type	= &omnia_hw_trigger_type,
+};
+
 static int omnia_led_brightness_set_blocking(struct led_classdev *cdev,
 					     enum led_brightness brightness)
 {
@@ -115,6 +148,8 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
 	cdev = &led->mc_cdev.led_cdev;
 	cdev->max_brightness = 1;
 	cdev->brightness_set_blocking = omnia_led_brightness_set_blocking;
+	cdev->trigger_type = &omnia_hw_trigger_type;
+	cdev->default_trigger = omnia_hw_trigger.name;
 
 	/* put the LED into software mode */
 	ret = i2c_smbus_write_byte_data(client, CMD_LED_MODE,
@@ -230,6 +265,12 @@ static int omnia_leds_probe(struct i2c_client *client)
 
 	mutex_init(&leds->lock);
 
+	ret = devm_led_trigger_register(dev, &omnia_hw_trigger);
+	if (ret < 0) {
+		dev_err(dev, "Cannot register private LED trigger: %d\n", ret);
+		return ret;
+	}
+
 	led = &leds->leds[0];
 	for_each_available_child_of_node(np, child) {
 		ret = omnia_led_register(client, led, child);
-- 
2.41.0