Re: [PATCH 07/10] leds: rgb: ktd202x: Add hardware pattern support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] leds: ktd202x: ktd202x_pattern_program() enables PWM1 on 0-brightness channels
--

commit cb2ed015f23bfa814b09df784ba07474c986d4de
Author: Joseph Telaak <joseph@telaak.dev>

leds: rgb: ktd202x: Add hardware pattern support

This commit adds hardware pattern support to the ktd202x LED driver by
implementing the pattern_set and pattern_clear callbacks using the chip's
hardware flash timer and rise/fall ramp registers.

> diff --git a/drivers/leds/rgb/leds-ktd202x.c b/drivers/leds/rgb/leds-ktd202x.c
> index 67e65c1fcb5fb..ed0e8e74c9eab 100644
> --- a/drivers/leds/rgb/leds-ktd202x.c
> +++ b/drivers/leds/rgb/leds-ktd202x.c
[ ... ]
> @@ -420,6 +446,148 @@ static int ktd202x_blink_mc_set(struct led_classdev *cdev,
[ ... ]
> +static int ktd202x_pattern_program(struct ktd202x_led *led,
> +				   struct led_pattern *pattern,
> +				   struct mc_subled *subleds,
> +				   unsigned int num_channels)
> +{
> +	const struct ktd202x_info *info = led->chip->info;
> +	unsigned int t[KTD202X_PATTERN_LEN];
> +	unsigned int rise, fall, num_steps, on;
> +	unsigned long period_ms, ton_ms;
> +	u8 ctrl_mask = 0;
> +	u8 ctrl_pwm1 = 0;
> +	int ret;
> +	int i;
> +
> +	for (i = 0; i < num_channels; i++) {
> +		int channel = subleds[i].channel;
> +
> +		ctrl_mask |= KTD202X_CHANNEL_CTRL_MASK(channel);
> +		ctrl_pwm1 |= KTD202X_CHANNEL_CTRL_PWM1(channel);
> +	}
> +
> +	for (i = 0; i < KTD202X_PATTERN_LEN; i++)
> +		t[i] = min_t(u32, pattern[i].delta_t, KTD202X_PATTERN_MAX_MS);
> +
> +	rise = min_t(unsigned int, KTD202X_RAMP_MAX_STEPS,
> +		     DIV_ROUND_CLOSEST(t[0], KTD202X_RAMP_STEP_MS));
> +	fall = min_t(unsigned int, KTD202X_RAMP_MAX_STEPS,
> +		     DIV_ROUND_CLOSEST(t[2], KTD202X_RAMP_STEP_MS));
> +
> +	/* Round the period up so that the fall ramp is not cut short */
> +	period_ms = t[0] + t[1] + t[2] + t[3];
> +	num_steps = ktd202x_flash_period_steps(info, period_ms +
> +					       KTD202X_FLASH_PERIOD_STEP_MS - 1);
> +	period_ms = ktd202x_flash_period_ms(info, num_steps);
> +
> +	/* The on time includes the rise time */
> +	ton_ms = t[0] + t[1];
> +	on = clamp_t(unsigned long,
> +		     DIV_ROUND_CLOSEST(ton_ms * KTD202X_FLASH_ON_MAX, period_ms),
> +		     1, KTD202X_FLASH_ON_MAX - 1);
> +
> +	mutex_lock(&led->chip->mutex);
> +
> +	ret = regmap_write(led->chip->regmap, KTD202X_REG_FLASH_PERIOD, num_steps);
> +	if (ret)
> +		goto out;
> +
> +	ret = regmap_write(led->chip->regmap, KTD202X_REG_PWM1_TIMER, on);
> +	if (ret)
> +		goto out;
> +
> +	ret = regmap_write(led->chip->regmap, KTD202X_REG_TRISE_FALL,
> +			   KTD202X_RAMP_TFALL(fall) | KTD202X_RAMP_TRISE(rise));
> +	if (ret)
> +		goto out;
> +
> +	ret = regmap_update_bits(led->chip->regmap, KTD202X_REG_CHANNEL_CTRL,
> +				 ctrl_mask, ctrl_pwm1);
> +out:
> +	mutex_unlock(&led->chip->mutex);
> +	return ret;
> +}
> +
> +static int ktd202x_pattern_single_set(struct led_classdev *cdev,
> +				      struct led_pattern *pattern, u32 len,
> +				      int repeat)
> +{
> +	struct ktd202x_led *led = container_of(cdev, struct ktd202x_led, cdev);
> +	struct mc_subled info;
> +	int ret;
> +
> +	ret = ktd202x_pattern_check(cdev, pattern, len, repeat);
> +	if (ret)
> +		return ret;
> +
> +	ret = ktd202x_brightness_single_set(cdev, pattern[1].brightness);
> +	if (ret)
> +		return ret;
> +
> +	info.channel = led->index;
> +
> +	return ktd202x_pattern_program(led, pattern, &info, 1);
> +}
> +
> +static int ktd202x_pattern_single_clear(struct led_classdev *cdev)
> +{
> +	return ktd202x_brightness_single_set(cdev, LED_OFF);
> +}
> +
> +static int ktd202x_pattern_mc_set(struct led_classdev *cdev,
> +				  struct led_pattern *pattern, u32 len,
> +				  int repeat)
> +{
> +	struct led_classdev_mc *mc = lcdev_to_mccdev(cdev);
> +	struct ktd202x_led *led = container_of(mc, struct ktd202x_led, mcdev);
> +	int ret;
> +
> +	ret = ktd202x_pattern_check(cdev, pattern, len, repeat);
> +	if (ret)
> +		return ret;
> +
> +	ret = ktd202x_brightness_mc_set(cdev, pattern[1].brightness);
> +	if (ret)
> +		return ret;
> +
> +	return ktd202x_pattern_program(led, pattern, mc->subled_info,
> +				       mc->num_colors);
> +}

[Severity: Medium]
When a hardware pattern is set on a multicolor LED where one or more color
channels have a computed brightness of 0 (for example, via multi_intensity),
won't ktd202x_pattern_program() turn those 0-brightness channels on at their
previous or reset-default (10 mA) current?

In ktd202x_pattern_mc_set(), the call to ktd202x_brightness_mc_set() computes
per-channel brightness into mc->subled_info[i].brightness and calls
ktd202x_brightness_set():

drivers/leds/rgb/leds-ktd202x.c:ktd202x_brightness_set() {
	...
	for (i = 0; i < num_channels; i++) {
		enum led_brightness brightness;
		int mode;

		brightness = subleds[i].brightness;
		channel = subleds[i].channel;

		if (brightness) {
			/* Register expects brightness between 0 and MAX_BRIGHTNESS - 1 */
			ret = regmap_write(led->chip->regmap, KTD202X_REG_LED_IOUT(channel),
					   brightness - 1);
			...
		} else {
			mode = KTD202X_CHANNEL_CTRL_OFF;
		}
		ret = regmap_update_bits(led->chip->regmap, KTD202X_REG_CHANNEL_CTRL,
					 KTD202X_CHANNEL_CTRL_MASK(channel), mode);
		...
	}
}

Because KTD202X_REG_LED_IOUT(channel) encodes currents starting at 0.125 mA
(brightness - 1) and cannot encode 0 mA, channels with brightness == 0 do not
update KTD202X_REG_LED_IOUT(channel) and instead rely on
KTD202X_CHANNEL_CTRL_OFF in KTD202X_REG_CHANNEL_CTRL to remain off.

However, ktd202x_pattern_program() then loops over all num_channels and
unconditionally sets KTD202X_CHANNEL_CTRL_PWM1(channel) in ctrl_pwm1 for every
channel regardless of subleds[i].brightness, overwriting
KTD202X_REG_CHANNEL_CTRL so that the 0-brightness channels also breathe on
PWM1.

Also note that ktd202x_pattern_single_set() leaves info.brightness
uninitialized on the stack before passing &info to ktd202x_pattern_program(),
and ktd202x_pattern_program() builds ctrl_pwm1 before acquiring
led->chip->mutex (after ktd202x_brightness_mc_set() has already dropped it).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261101-ktd202x-et6326-v1-0-eb51eddad08c@telaak.dev?part=7