Re: [PATCH v3 5/6] leds: turris-omnia: support HW controlled mode via private trigger

[Lee Jones <lee@kernel.org>]

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@kernel.org>
> ---
>  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 [李琼斯]