Skip to contentSkip to conversation

leds: turris-omnia: updates

PATCH Applied Marek Behún 2 participants 6 messages Sep 18, 2023 – Sep 21, 2023

Subsystems (2)
lore
Share & tools
lore

Marek Behún

author maintainer
#0 lore raw

v4 cover leds: turris-omnia: updates

Hello Lee,

I am sending version 4 of Turris Omnia's LED controller updates. I tried to address the issues you mentioned:
- I made the omnia_cmd_read_raw() function to return the number of read bytes on success
- I have renamed omnia_cmd_read() to omnia_cmd_read_u8() since it always reads one byte. The function returns the read byte on success and a negative errno on failure
- accordingly, I have renamed omnia_cmd_write() to omnia_cmd_write_u8(), and made it so that it returns the number of written bytes on success
- the newly added comments now start with a capital letter

Marek

Marek Behún (4):
leds: turris-omnia: do not use SMBUS calls
leds: turris-omnia: make set_brightness() more efficient
leds: turris-omnia: support HW controlled mode via private trigger
leds: turris-omnia: add support for enabling/disabling HW gamma
correction

 .../sysfs-class-led-driver-turris-omnia       |  14 +
 drivers/leds/Kconfig                          |   1 +
 drivers/leds/leds-turris-omnia.c              | 352 ++++++++++++++++--
 3 files changed, 329 insertions(+), 38 deletions(-)
Signature · 1 line
2.41.0
author maintainer
#1 lore raw

PATCH v4 1/4 leds: turris-omnia: do not use SMBUS calls

The leds-turris-omnia driver uses three function for I2C access:
- i2c_smbus_write_byte_data() and i2c_smbus_read_byte_data(), which cause an emulated SMBUS transfer,
- i2c_master_send(), which causes an ordinary I2C transfer.

The Turris Omnia MCU LED controller is not semantically SMBUS, it operates as a simple I2C bus. It does not implement any of the SMBUS specific features, like PEC, or procedure calls, or anything. Moreover the I2C controller driver also does not implement SMBUS, and so the emulated SMBUS procedure from drivers/i2c/i2c-core-smbus.c is used for the SMBUS calls, which gives an unnecessary overhead.

When I first wrote the driver, I was unaware of these facts, and I simply used the first function that worked.

Drop the I2C SMBUS calls and instead use simple I2C transfers.

Fixes 089381b27abe ("leds: initial support for Turris Omnia LEDs")
---
 drivers/leds/leds-turris-omnia.c | 55 +++++++++++++++++++++++++-------
 1 file changed, 43 insertions(+), 12 deletions(-)
drivers/leds/leds-turris-omnia.c +43 -12
index b8a95a917cfa..6ef44de773fe 100644
@@ -2,7 +2,7 @@
 /*
  * CZ.NIC's Turris Omnia LEDs driver
  *
- * 2020 by Marek Behún <[email protected]>
+ * 2020, 2023 by Marek Behún <[email protected]>
  */
 
 #include <linux/i2c.h>
@@ -41,6 +41,38 @@ struct omnia_leds {
 	struct omnia_led leds[];
 };
 
+static int omnia_cmd_write_u8(const struct i2c_client *client, u8 cmd, u8 val)
+{
+	u8 buf[2] = { cmd, val };
+	int ret;
+
+	return i2c_master_send(client, buf, sizeof(buf));
+}
+
+static int omnia_cmd_read_u8(const struct i2c_client *client, u8 cmd)
+{
+	struct i2c_msg msgs[2];
+	u8 reply;
+	int ret;
+
+	msgs[0].addr = client->addr;
+	msgs[0].flags = 0;
+	msgs[0].len = 1;
+	msgs[0].buf = &cmd;
+	msgs[1].addr = client->addr;
+	msgs[1].flags = I2C_M_RD;
+	msgs[1].len = 1;
+	msgs[1].buf = &reply;
+
+	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
+	if (likely(ret == ARRAY_SIZE(msgs)))
+		return reply;
+	else if (ret < 0)
+		return ret;
+	else
+		return -EIO;
+}
+
 static int omnia_led_brightness_set_blocking(struct led_classdev *cdev,
 					     enum led_brightness brightness)
 {
@@ -64,7 +96,7 @@ static int omnia_led_brightness_set_blocking(struct led_classdev *cdev,
 	if (buf[2] || buf[3] || buf[4])
 		state |= CMD_LED_STATE_ON;
 
-	ret = i2c_smbus_write_byte_data(leds->client, CMD_LED_STATE, state);
+	ret = omnia_cmd_write_u8(leds->client, CMD_LED_STATE, state);
 	if (ret >= 0 && (state & CMD_LED_STATE_ON))
 		ret = i2c_master_send(leds->client, buf, 5);
 
@@ -114,9 +146,9 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
 	cdev->brightness_set_blocking = omnia_led_brightness_set_blocking;
 
 	/* put the LED into software mode */
-	ret = i2c_smbus_write_byte_data(client, CMD_LED_MODE,
-					CMD_LED_MODE_LED(led->reg) |
-					CMD_LED_MODE_USER);
+	ret = omnia_cmd_write_u8(client, CMD_LED_MODE,
+				 CMD_LED_MODE_LED(led->reg) |
+				 CMD_LED_MODE_USER);
 	if (ret < 0) {
 		dev_err(dev, "Cannot set LED %pOF to software mode: %i\n", np,
 			ret);
@@ -124,8 +156,8 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
 	}
 
 	/* disable the LED */
-	ret = i2c_smbus_write_byte_data(client, CMD_LED_STATE,
-					CMD_LED_STATE_LED(led->reg));
+	ret = omnia_cmd_write_u8(client, CMD_LED_STATE,
+				 CMD_LED_STATE_LED(led->reg));
 	if (ret < 0) {
 		dev_err(dev, "Cannot set LED %pOF brightness: %i\n", np, ret);
 		return ret;
@@ -158,7 +190,7 @@ static ssize_t brightness_show(struct device *dev, struct device_attribute *a,
 	struct i2c_client *client = to_i2c_client(dev);
 	int ret;
 
-	ret = i2c_smbus_read_byte_data(client, CMD_LED_GET_BRIGHTNESS);
+	ret = omnia_cmd_read_u8(client, CMD_LED_GET_BRIGHTNESS);
 
 	if (ret < 0)
 		return ret;
@@ -179,8 +211,7 @@ static ssize_t brightness_store(struct device *dev, struct device_attribute *a,
 	if (brightness > 100)
 		return -EINVAL;
 
-	ret = i2c_smbus_write_byte_data(client, CMD_LED_SET_BRIGHTNESS,
-					(u8)brightness);
+	ret = omnia_cmd_write_u8(client, CMD_LED_SET_BRIGHTNESS, brightness);
 
 	return ret < 0 ? ret : count;
 }
@@ -237,8 +268,8 @@ static void omnia_leds_remove(struct i2c_client *client)
 	u8 buf[5];
 
 	/* put all LEDs into default (HW triggered) mode */
-	i2c_smbus_write_byte_data(client, CMD_LED_MODE,
-				  CMD_LED_MODE_LED(OMNIA_BOARD_LEDS));
+	omnia_cmd_write_u8(client, CMD_LED_MODE,
+			   CMD_LED_MODE_LED(OMNIA_BOARD_LEDS));
 
 	/* set all LEDs color to [255, 255, 255] */
 	buf[0] = CMD_LED_COLOR;
Signature · 1 line
2.41.0
author maintainer
#2 lore raw

PATCH v4 2/4 leds: turris-omnia: make set_brightness() more efficient

In reply to Marek Behún

Implement caching of the LED color and state values that are sent to MCU in order to make the set_brightness() operation more efficient by avoiding I2C transactions which are not needed.

On Turris Omnia's MCU, which acts as the RGB LED controller, each LED has a RGB color, and a ON/OFF state, which are configurable via I2C commands CMD_LED_COLOR and CMD_LED_STATE.

The CMD_LED_COLOR command sends 5 bytes and the CMD_LED_STATE command 2 bytes over the I2C bus, which operates at 100 kHz. With I2C overhead this allows ~1670 color changing commands and ~3200 state changing commands per second (or around 1000 color + state changes per second). This may seem more than enough, but the issue is that the I2C bus is shared with another peripheral, the MCU. The MCU exposes an interrupt interface, and it can trigger hundreds of interrupts per second. Each time, we need to read the interrupt state register over this I2C bus. Whenever we are sending a LED color/state changing command, the interrupt reading is waiting.

Currently, every time LED brightness or LED multi intensity is changed, we send a CMD_LED_STATE command, and if the computed color (brightness adjusted multi_intensity) is non-zero, we also send a CMD_LED_COLOR command.

Consider for example the situation when we have a netdev trigger enabled for a LED. The netdev trigger does not change the LED color, only the brightness (either to 0 or to currently configured brightness), and so there is no need to send the CMD_LED_COLOR command. But each change of brightness to 0 sends one CMD_LED_STATE command, and each change of brightness to max_brightness sends one CMD_LED_STATE command and one CMD_LED_COLOR command:

    set_brightness(0)   ->  CMD_LED_STATE
    set_brightness(255) ->  CMD_LED_STATE + CMD_LED_COLOR
                                            (unnecessary)

We can avoid the unnecessary I2C transactions if we cache the values of state and color that are sent to the controller. If the color does not change from the one previously sent, there is no need to do the CMD_LED_COLOR I2C transaction, and if the state does not change, there is no need to do the CMD_LED_STATE transaction.

Because we need to make sure that our cached values are consistent with the controller state, add explicit setting of the LED color to white at probe time (this is the default setting when MCU resets, but does not necessarily need to be the case, for example if U-Boot played with the LED colors).

---
 drivers/leds/leds-turris-omnia.c | 96 ++++++++++++++++++++++++++------
 1 file changed, 78 insertions(+), 18 deletions(-)
drivers/leds/leds-turris-omnia.c +78 -18
index 6ef44de773fe..ec5c47d39e85 100644
@@ -30,6 +30,8 @@
 struct omnia_led {
 	struct led_classdev_mc mc_cdev;
 	struct mc_subled subled_info[OMNIA_LED_NUM_CHANNELS];
+	u8 cached_channels[OMNIA_LED_NUM_CHANNELS];
+	bool on;
 	int reg;
 };
 
@@ -73,36 +75,82 @@ static int omnia_cmd_read_u8(const struct i2c_client *client, u8 cmd)
 		return -EIO;
 }
 
+static int omnia_led_send_color_cmd(const struct i2c_client *client,
+				    struct omnia_led *led)
+{
+	char cmd[5];
+	int ret;
+
+	cmd[0] = CMD_LED_COLOR;
+	cmd[1] = led->reg;
+	cmd[2] = led->subled_info[0].brightness;
+	cmd[3] = led->subled_info[1].brightness;
+	cmd[4] = led->subled_info[2].brightness;
+
+	/* Send the color change command */
+	ret = i2c_master_send(client, cmd, 5);
+	if (ret < 0)
+		return ret;
+
+	/* Cache the RGB channel brightnesses */
+	for (int i = 0; i < OMNIA_LED_NUM_CHANNELS; ++i)
+		led->cached_channels[i] = led->subled_info[i].brightness;
+
+	return 0;
+}
+
+/* Determine if the computed RGB channels are different from the cached ones */
+static bool omnia_led_channels_changed(struct omnia_led *led)
+{
+	for (int i = 0; i < OMNIA_LED_NUM_CHANNELS; ++i)
+		if (led->subled_info[i].brightness != led->cached_channels[i])
+			return true;
+
+	return false;
+}
+
 static int omnia_led_brightness_set_blocking(struct led_classdev *cdev,
 					     enum led_brightness brightness)
 {
 	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
 	struct omnia_leds *leds = dev_get_drvdata(cdev->dev->parent);
 	struct omnia_led *led = to_omnia_led(mc_cdev);
-	u8 buf[5], state;
-	int ret;
+	int err = 0;
 
 	mutex_lock(&leds->lock);
 
-	led_mc_calc_color_components(&led->mc_cdev, brightness);
+	/*
+	 * Only recalculate RGB brightnesses from intensities if brightness is
+	 * non-zero. Otherwise we won't be using them and we can save ourselves
+	 * some software divisions (Omnia's CPU does not implement the division
+	 * instruction).
+	 */
+	if (brightness) {
+		led_mc_calc_color_components(mc_cdev, brightness);
+
+		/*
+		 * Send color command only if brightness is non-zero and the RGB
+		 * channel brightnesses changed.
+		 */
+		if (omnia_led_channels_changed(led))
+			err = omnia_led_send_color_cmd(leds->client, led);
+	}
 
-	buf[0] = CMD_LED_COLOR;
-	buf[1] = led->reg;
-	buf[2] = mc_cdev->subled_info[0].brightness;
-	buf[3] = mc_cdev->subled_info[1].brightness;
-	buf[4] = mc_cdev->subled_info[2].brightness;
+	/* Send on/off state change only if (bool)brightness changed */
+	if (!err && !brightness != !led->on) {
+		u8 state = CMD_LED_STATE_LED(led->reg);
 
-	state = CMD_LED_STATE_LED(led->reg);
-	if (buf[2] || buf[3] || buf[4])
-		state |= CMD_LED_STATE_ON;
+		if (brightness)
+			state |= CMD_LED_STATE_ON;
 
-	ret = omnia_cmd_write_u8(leds->client, CMD_LED_STATE, state);
-	if (ret >= 0 && (state & CMD_LED_STATE_ON))
-		ret = i2c_master_send(leds->client, buf, 5);
+		err = omnia_cmd_write_u8(leds->client, CMD_LED_STATE, state);
+		if (!err)
+			led->on = !!brightness;
+	}
 
 	mutex_unlock(&leds->lock);
 
-	return ret;
+	return err;
 }
 
 static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
@@ -130,11 +178,15 @@ 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].channel = 0;
 	led->subled_info[1].color_index = LED_COLOR_ID_GREEN;
-	led->subled_info[1].channel = 1;
 	led->subled_info[2].color_index = LED_COLOR_ID_BLUE;
-	led->subled_info[2].channel = 2;
+
+	/* Initial color is white */
+	for (int i = 0; i < OMNIA_LED_NUM_CHANNELS; ++i) {
+		led->subled_info[i].intensity = 255;
+		led->subled_info[i].brightness = 255;
+		led->subled_info[i].channel = i;
+	}
 
 	led->mc_cdev.subled_info = led->subled_info;
 	led->mc_cdev.num_colors = OMNIA_LED_NUM_CHANNELS;
@@ -163,6 +215,14 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
 		return ret;
 	}
 
+	/* Set initial color and cache it */
+	ret = omnia_led_send_color_cmd(client, led);
+	if (ret < 0) {
+		dev_err(dev, "Cannot set LED %pOF initial color: %i\n", np,
+			ret);
+		return ret;
+	}
+
 	ret = devm_led_classdev_multicolor_register_ext(dev, &led->mc_cdev,
 							&init_data);
 	if (ret < 0) {
Signature · 1 line
2.41.0
author maintainer
#3 lore raw

PATCH v4 3/4 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". Recall that private LED triggers will only be listed in the sysfs trigger file for LEDs that support them (currently there is no user of this mechanism).

When in MCU controlled mode, the user can still set LED color, but the blinking is done by MCU, which does different things for different 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 the corresponding port of the LAN switch
- PCIe LEDs are blinked according to the logical OR of the MiniPCIe port LED pins

In the future I want to make the netdev trigger to transparently offload the blinking to the HW if user sets compatible settings for the netdev trigger (for LEDs associated with network devices).
There was some work on this already, and hopefully we will be able to complete it sometime, but for now there are still multiple blockers for this, and even if there weren't, we still would not be able to configure HW controlled mode for the LEDs associated with MiniPCIe ports.

In the meantime let's support HW controlled mode via the private LED trigger mechanism. 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.

---
 drivers/leds/Kconfig             |  1 +
 drivers/leds/leds-turris-omnia.c | 98 +++++++++++++++++++++++++++++---
 2 files changed, 91 insertions(+), 8 deletions(-)
drivers/leds/Kconfig +1 -0
index b92208eccdea..6292fddcc55c 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 +90 -8

135 lines not on this page · raw message

Signature · 1 line
2.41.0
author maintainer
#4 lore raw

PATCH v4 4/4 leds: turris-omnia: add support for enabling/disabling HW gamma correction

In reply to Marek Behún

If the MCU on Turris Omnia is running newer firmware versions, the LED controller supports RGB gamma correction (and enables it by default for newer boards).

Determine whether the gamma correction setting feature is supported and add the ability to set it via sysfs attribute file.

---
 .../sysfs-class-led-driver-turris-omnia       |  14 ++
 drivers/leds/leds-turris-omnia.c              | 137 +++++++++++++++---
 2 files changed, 134 insertions(+), 17 deletions(-)
Documentation/ABI/testing/sysfs-class-led-driver-turris-omnia +14 -0
index c4d46970c1cf..369b4ae8be5f 100644
@@ -12,3 +12,17 @@ Description:	(RW) On the front panel of the Turris Omnia router there is also
 		able to change this setting from software.
 
 		Format: %i
+
+What:		/sys/class/leds/<led>/device/gamma_correction
+Date:		August 2023
+KernelVersion:	6.6
+Contact:	Marek Behún <[email protected]>
+Description:	(RW) Newer versions of the microcontroller firmware of the
+		Turris Omnia router support gamma correction for the RGB LEDs.
+		This feature can be enabled/disabled by writing to this file.
+
+		If the feature is not supported because the MCU firmware is too
+		old, the file always reads as 0, and writing to the file results
+		in the EOPNOTSUPP error.
+
+		Format: %i
drivers/leds/leds-turris-omnia.c +120 -17

191 lines not on this page · raw message

Signature · 1 line
2.41.0
maintainer Applied
#5 lore raw
In reply to Marek Behún
Quoted 11 lines · Marek Behún

I am sending version 4 of Turris Omnia's LED controller updates. I tried to address the issues you mentioned:
- I made the omnia_cmd_read_raw() function to return the number of read bytes on success
- I have renamed omnia_cmd_read() to omnia_cmd_read_u8() since it always reads one byte. The function returns the read byte on success and a negative errno on failure
- accordingly, I have renamed omnia_cmd_write() to omnia_cmd_write_u8(), and made it so that it returns the number of written bytes on success
- the newly added comments now start with a capital letter

Quoted 1 line

[...]

Applied, thanks!

[1/4] leds: turris-omnia: do not use SMBUS calls

      commit: 2b423ec3d05b5b9f6e663e99d40314c011a23a41
[2/4] leds: turris-omnia: make set_brightness() more efficient
      commit: aa5799d8cf1958e1359023aa13842952c6fde267
[3/4] leds: turris-omnia: support HW controlled mode via private trigger
      commit: 276f5ece909ee04676c92d4a942354a75059d832
[4/4] leds: turris-omnia: add support for enabling/disabling HW gamma correction
      commit: d82d0db48329785d31e7fed3ead8429383c94127

--
Lee Jones [李琼斯]