Re: [PATCH v3 4/6] leds: turris-omnia: make set_brightness() more efficient

[Lee Jones <lee@kernel.org>]

On Wed, 02 Aug 2023, Marek Behún wrote:

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

How many transactions does all of this extra code actually save?

> 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 only ~1670 color changing commands and ~3200 state changing
> commands per second.

Only?  Seems like quite a lot.

> 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 out cached values are consistent with

Nit: "our"

> 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).
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
>  drivers/leds/leds-turris-omnia.c | 96 ++++++++++++++++++++++++++------
>  1 file changed, 78 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c
> index 9fca0acb2270..636c6f802bcf 100644
> --- a/drivers/leds/leds-turris-omnia.c
> +++ b/drivers/leds/leds-turris-omnia.c
> @@ -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;
>  };
>  
> @@ -75,36 +77,82 @@ static int omnia_cmd_read(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 */

Nit: Please start comments with an upper case char.

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

Why the pre-increment?

It's non-standard and doesn't appear to have any affect.

> +		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(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(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,
> @@ -132,11 +180,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;
> @@ -164,6 +216,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) {
> -- 
> 2.41.0
> 

-- 
Lee Jones [李琼斯]