On Wed, 02 Aug 2023, Marek Behún wrote:
> 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.
>
> Signed-off-by: Marek Behún <kabel@xxxxxxxxxx>
> ---
> drivers/leds/Kconfig | 1 +
> drivers/leds/leds-turris-omnia.c | 97 +++++++++++++++++++++++++++++---
> 2 files changed, 90 insertions(+), 8 deletions(-)
>
> 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 636c6f802bcf..180b0cbeb92e 100644
> --- a/drivers/leds/leds-turris-omnia.c
> +++ b/drivers/leds/leds-turris-omnia.c
> @@ -31,7 +31,7 @@ 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;
> + bool on, hwtrig;
> int reg;
> };
>
> @@ -123,12 +123,14 @@ static int omnia_led_brightness_set_blocking(struct led_classdev *cdev,
>
> /*
> * 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).
> + * non-zero (if it is zero and the LED is in HW blinking mode, we use
> + * max_brightness as brightness). 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);
> + if (brightness || led->hwtrig) {
> + led_mc_calc_color_components(mc_cdev, brightness ?:
> + cdev->max_brightness);
>
> /*
> * Send color command only if brightness is non-zero and the RGB
> @@ -138,8 +140,11 @@ static int omnia_led_brightness_set_blocking(struct led_classdev *cdev,
> err = omnia_led_send_color_cmd(leds->client, led);
> }
>
> - /* send on/off state change only if (bool)brightness changed */
> - if (!err && !brightness != !led->on) {
> + /*
> + * Send on/off state change only if (bool)brightness changed and the LED
> + * is not being blinked by HW.
> + */
> + if (!err && !led->hwtrig && !brightness != !led->on) {
> u8 state = CMD_LED_STATE_LED(led->reg);
>
> if (brightness)
> @@ -155,6 +160,70 @@ static int omnia_led_brightness_set_blocking(struct led_classdev *cdev,
> return err;
> }
>
> +static struct led_hw_trigger_type omnia_hw_trigger_type;
> +
> +static int omnia_hwtrig_activate(struct led_classdev *cdev)
> +{
> + 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);
> + int err = 0;
> +
> + mutex_lock(&leds->lock);
> +
> + if (!led->on) {
> + /*
> + * If the LED is off (brightness was set to 0), the last
> + * configured color was not necessarily sent to the MCU.
> + * Recompute with max_brightness and send if needed.
> + */
> + led_mc_calc_color_components(mc_cdev, cdev->max_brightness);
> +
> + if (omnia_led_channels_changed(led))
> + err = omnia_led_send_color_cmd(leds->client, led);
> + }
> +
> + if (!err) {
> + /* put the LED into MCU controlled mode */
Nit: You improved the comment above to be more grammatically correct by
starting with an uppercase character. Please continue with this
improvement for all comments there in.
> + err = omnia_cmd_write(leds->client, CMD_LED_MODE,
> + CMD_LED_MODE_LED(led->reg));
> + if (!err)
> + led->hwtrig = true;
> + }
> +
> + mutex_unlock(&leds->lock);
> +
> + return err;
> +}
> +
> +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 err;
> +
> + mutex_lock(&leds->lock);
> +
> + led->hwtrig = false;
> +
> + /* put the LED into software mode */
> + err = omnia_cmd_write(leds->client, CMD_LED_MODE,
> + CMD_LED_MODE_LED(led->reg) | CMD_LED_MODE_USER);
> +
> + mutex_unlock(&leds->lock);
> +
> + if (err < 0)
> + dev_err(cdev->dev, "Cannot put LED to software mode: %i\n",
> + err);
> +}
> +
> +static struct led_trigger omnia_hw_trigger = {
> + .name = "omnia-mcu",
> + .activate = omnia_hwtrig_activate,
> + .deactivate = omnia_hwtrig_deactivate,
> + .trigger_type = &omnia_hw_trigger_type,
Not sure I understand this interface.
Why not just set a bool instead of carrying around an empty struct?
> +};
> +
> static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
> struct device_node *np)
> {
> @@ -198,6 +267,12 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
> cdev = &led->mc_cdev.led_cdev;
> cdev->max_brightness = 255;
> cdev->brightness_set_blocking = omnia_led_brightness_set_blocking;
> + cdev->trigger_type = &omnia_hw_trigger_type;
> + /*
> + * Use the omnia-mcu trigger as the default trigger. It may be rewritten
> + * by LED class from the linux,default-trigger property.
> + */
> + cdev->default_trigger = omnia_hw_trigger.name;
>
> /* put the LED into software mode */
> ret = omnia_cmd_write(client, CMD_LED_MODE, CMD_LED_MODE_LED(led->reg) |
> @@ -310,6 +385,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
>
--
Lee Jones [李琼斯]
