[PATCH v2 00/11] leds: ktd202x: Add Etek ET6326, hardware breathing and fixes

[PATCH v2 00/11] leds: ktd202x: Add Etek ET6326, hardware breathing and fixes

[Joseph Telaak via B4 Relay]

The Etek ET6326 drives the status LED of the GL.iNet GL-SFT1200 (Opal)
router. It is register compatible with the Kinetic KTD2026, except for
the flash period: Reg1 = n is (n + 1) * 128 ms on the ET6326 and
256 ms + n * 128 ms on the KTD202x. Patches 6 and 7 add it with the
KTD2026 as fallback, so older kernels drive it with slightly off blinks.

Patches 1 to 5 fix bugs that have been there since the driver was
added, and come first so they can be applied on their own:
- a blink shorter than 256 ms became a 16 s blink on 32 bit
- with fewer LEDs in the DT than channels, as on this board, the driver
  read past the end of its LED array
- a blink with a very short on time was programmed as never on
- delays from the timer trigger that add up to 2^32 (or 2^64) divided
  by zero, and long blinks overflowed the on time on 32 bit
- a blinking multicolor LED also lit its colors at brightness 0

Patch 8 lets the pattern trigger's hw_pattern use the chip's rise and
fall ramps, so a breathing LED needs no I2C traffic. Patch 9 honors
led-max-microamp on single color LEDs: the GL-SFT1200 drives three LEDs
in parallel on each channel at 1.375 mA, and the driver's full
brightness is 24 mA. Patch 10 fails the probe when nothing answers at
the address. Patch 11 stops a second blinking LED from silently
retiming the first: it shares the flash timer only with the same
timing, and otherwise blinks in software. The second PWM timer is no
help, since in the timer slot the driver uses it repeats the first.

ET6326 datasheet:
https://www.etekmicro.com/wp-content/uploads/datasheets/ET6326_datasheet.pdf

Tested on a GL-SFT1200 (SF19A2890, MIPS32) with OpenWrt and Linux
6.18.60, with the two LEDs of the board (blue and white, three LEDs in
parallel on each channel, led-max-microamp = <1375>):
- brightness 1 to 11 (max_brightness 11 from led-max-microamp), and
  waking the chip from sleep
- the timer trigger from 1/1 ms to 500/500 ms; a camera measured a
  0.917 s period for 896 ms, the same 2.2 % slow oscillator as with the
  stock firmware, so the ET6326 period encoding is the one used here
- a 1/200 ms blink, which keeps blinking after a new delay_off, and
  delays that add up to 2^32 ms
- a second LED blinking at another rate falls back to software and
  leaves the first one alone; with the same delays it shares the timer
  in step with the first; a LED that blinks in software takes the timer
  over at its own brightness once it is free
- the timer programmed again after the chip slept
- breathing through hw_pattern, written right after selecting the
  pattern trigger
- invalid patterns and finite repeats rejected
- the same pattern on both LEDs shares the timer, and a pattern that
  cannot get the timer leaves its LED off
- 240 writes to one LED while the other stayed lit, module unload and
  reload, and a reboot while blinking
The bug fixes were also checked with a simulated chip under ASan and
UBSan, and the series builds with W=1 on x86_64, i386, MIPS and arm64.

---
Changes in v2:
- New patch 5: a blinking multicolor LED keeps its colors at brightness
  0 off. The bug is older than the series.
- Patch 6: add Krzysztof's Ack.
- Patch 7: report delay_on rounded to the nearest ms and at least 1 ms.
  On the 128 ms ET6326 period a short on time read back as 0 ms, and the
  timer trigger's next delay_off write then turned the LED off.
- Patch 8:
  - skip the colors at brightness 0, and build the channel list under
    the mutex
  - use GPL-2.0-only, not the deprecated GPL-2.0 SPDX identifier, in the
    new document
- Patch 10: the KTD2026/7 does not ACK the byte that resets it, so v1
  failed the probe on every KTD2026/7. Ignore that write again, and fail
  the probe when the write that puts the chip to sleep fails.
- Patch 11:
  - write the timer registers whenever no other LED uses the timer, so
    they are set again after the chip lost power, and release the timer
    when the write fails
  - hold the chip mutex from the brightness to the timer in blink and
    hw_pattern, and track the timer from the brightness being set, not
    cdev->brightness
  - blink at blink_brightness, not max_brightness
  - check the timer before a pattern touches the LED, and leave the LED
    as it was on -EBUSY
- Link to v1: https://lore.kernel.org/r/20261101-ktd202x-et6326-v1-0-eb51eddad08c@telaak.dev

---
Joseph Telaak (11):
      leds: rgb: ktd202x: Fix the flash period of short blinks on 32 bit
      leds: rgb: ktd202x: Fix an out-of-bounds read with unused channels
      leds: rgb: ktd202x: Never program an on time of zero
      leds: rgb: ktd202x: Fix a division by zero with long blinks
      leds: rgb: ktd202x: Keep the unlit colors of a blinking LED off
      dt-bindings: leds: kinetic,ktd202x: Add Etek ET6326
      leds: rgb: ktd202x: Add Etek ET6326 support
      leds: rgb: ktd202x: Add hardware pattern support
      leds: rgb: ktd202x: Limit the LED current to led-max-microamp
      leds: rgb: ktd202x: Fail the probe when the chip does not answer
      leds: rgb: ktd202x: Don't let a blinking LED retime another one

 .../devicetree/bindings/leds/kinetic,ktd202x.yaml  |  15 +-
 Documentation/leds/index.rst                       |   1 +
 Documentation/leds/leds-ktd202x.rst                |  51 ++
 drivers/leds/rgb/Kconfig                           |   3 +-
 drivers/leds/rgb/leds-ktd202x.c                    | 578 +++++++++++++++++----
 5 files changed, 546 insertions(+), 102 deletions(-)
---
base-commit: 59cf91cf87abb67a49b1bb7b9d3b06486abde8cf
change-id: 20261101-ktd202x-et6326-73ed3046c534

Best regards,
--  
Joseph Telaak <joseph@telaak.dev>

[PATCH v2 01/11] leds: rgb: ktd202x: Fix the flash period of short blinks on 32 bit

[Joseph Telaak via B4 Relay]

From: Joseph Telaak <joseph@telaak.dev>

When delay_on + delay_off is below 256 ms, the unsigned subtraction of
KTD202X_FLASH_PERIOD_MIN_MS wraps. On 64 bit the result truncates to -1
or -2 and gets clamped to the shortest period, but on 32 bit it
truncates to 33554431 or 33554430 and gets clamped to the longest one,
so asking for a fast blink gives a blink every 16 seconds.

Compare before subtracting.

Fixes: 0ebdb7210943 ("leds: Add ktd202x driver")
Cc: stable@vger.kernel.org
Signed-off-by: Joseph Telaak <joseph@telaak.dev>
---
 drivers/leds/rgb/leds-ktd202x.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/leds/rgb/leds-ktd202x.c b/drivers/leds/rgb/leds-ktd202x.c
index 143020945e23..539cf425cdb7 100644
--- a/drivers/leds/rgb/leds-ktd202x.c
+++ b/drivers/leds/rgb/leds-ktd202x.c
@@ -296,9 +296,13 @@ static int ktd202x_blink_set(struct ktd202x_led *led, unsigned long *delay_on,
 	/* Convert into values the HW will understand. */
 
 	/* Integer representation of time of flash period */
-	num_steps = (*delay_on + *delay_off - KTD202X_FLASH_PERIOD_MIN_MS) /
-		    KTD202X_FLASH_PERIOD_STEP_MS;
-	num_steps = clamp(num_steps, 0, KTD202X_FLASH_PERIOD_MAX_STEPS);
+	delay_total_ms = *delay_on + *delay_off;
+	if (delay_total_ms < KTD202X_FLASH_PERIOD_MIN_MS)
+		num_steps = 0;
+	else
+		num_steps = min_t(unsigned long, KTD202X_FLASH_PERIOD_MAX_STEPS,
+				  (delay_total_ms - KTD202X_FLASH_PERIOD_MIN_MS) /
+				  KTD202X_FLASH_PERIOD_STEP_MS);
 
 	/* Integer representation of percentage of LED ON time */
 	on = (*delay_on * KTD202X_FLASH_ON_MAX) / (*delay_on + *delay_off);

-- 
Git-155)

[PATCH v2 02/11] leds: rgb: ktd202x: Fix an out-of-bounds read with unused channels

[Joseph Telaak via B4 Relay]

From: Joseph Telaak <joseph@telaak.dev>

The LED array is allocated for the number of child nodes, but
ktd202x_chip_in_use() walks it up to num_leds, the number of channels of
the chip. With fewer LEDs in the device tree than channels, for example
two LEDs on a KTD2026, it reads past the end of the allocation and the
__counted_by() annotation hides that from the bounds checkers.

Allocate one entry per channel.

Fixes: 0ebdb7210943 ("leds: Add ktd202x driver")
Cc: stable@vger.kernel.org
Signed-off-by: Joseph Telaak <joseph@telaak.dev>
---
 drivers/leds/rgb/leds-ktd202x.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/leds/rgb/leds-ktd202x.c b/drivers/leds/rgb/leds-ktd202x.c
index 539cf425cdb7..482af28e8370 100644
--- a/drivers/leds/rgb/leds-ktd202x.c
+++ b/drivers/leds/rgb/leds-ktd202x.c
@@ -537,6 +537,7 @@ static int ktd202x_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
 	struct ktd202x *chip;
+	unsigned long num_leds;
 	int count;
 	int ret;
 
@@ -544,10 +545,13 @@ static int ktd202x_probe(struct i2c_client *client)
 	if (!count || count > KTD202X_MAX_LEDS)
 		return dev_err_probe(dev, -EINVAL, "Incorrect number of leds (%d)", count);
 
-	chip = devm_kzalloc(dev, struct_size(chip, leds, count), GFP_KERNEL);
+	/* Loops over the LEDs go up to the number of channels, not of child nodes */
+	num_leds = (unsigned long)i2c_get_match_data(client);
+	chip = devm_kzalloc(dev, struct_size(chip, leds, num_leds), GFP_KERNEL);
 	if (!chip)
 		return -ENOMEM;
 
+	chip->num_leds = num_leds;
 	chip->dev = dev;
 	i2c_set_clientdata(client, chip);
 
@@ -562,8 +566,6 @@ static int ktd202x_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
-	chip->num_leds = (unsigned long)i2c_get_match_data(client);
-
 	chip->regulators[0].supply = "vin";
 	chip->regulators[1].supply = "vio";
 	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(chip->regulators), chip->regulators);

-- 
Git-155)

[PATCH v2 03/11] leds: rgb: ktd202x: Never program an on time of zero

[Joseph Telaak via B4 Relay]

From: Joseph Telaak <joseph@telaak.dev>

blink_set() turns delay_on into a fraction of the period in 1/256 steps.
When delay_on is less than 1/256 of the period, for example 1 ms on and
1000 ms off, that fraction is 0: the LED stays dark, while the timer
trigger reports that it is blinking with delay_on = 0.

Use the shortest on time the hardware has, 1/256 of the period, and
report it back.

Fixes: 0ebdb7210943 ("leds: Add ktd202x driver")
Signed-off-by: Joseph Telaak <joseph@telaak.dev>
---
 drivers/leds/rgb/leds-ktd202x.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/rgb/leds-ktd202x.c b/drivers/leds/rgb/leds-ktd202x.c
index 482af28e8370..8c890ea5a754 100644
--- a/drivers/leds/rgb/leds-ktd202x.c
+++ b/drivers/leds/rgb/leds-ktd202x.c
@@ -304,8 +304,12 @@ static int ktd202x_blink_set(struct ktd202x_led *led, unsigned long *delay_on,
 				  (delay_total_ms - KTD202X_FLASH_PERIOD_MIN_MS) /
 				  KTD202X_FLASH_PERIOD_STEP_MS);
 
-	/* Integer representation of percentage of LED ON time */
+	/*
+	 * Integer representation of percentage of LED ON time. A value of 0
+	 * keeps the LED dark, so use the shortest on time instead.
+	 */
 	on = (*delay_on * KTD202X_FLASH_ON_MAX) / (*delay_on + *delay_off);
+	on = max(on, 1);
 
 	/* Actually used delay_{on,off} values */
 	delay_total_ms = num_steps * KTD202X_FLASH_PERIOD_STEP_MS + KTD202X_FLASH_PERIOD_MIN_MS;

-- 
Git-155)

[PATCH v2 04/11] leds: rgb: ktd202x: Fix a division by zero with long blinks

[Joseph Telaak via B4 Relay]

From: Joseph Telaak <joseph@telaak.dev>

blink_set() works out the on time as delay_on * 256 / (delay_on +
delay_off) in unsigned long, with delays straight from the timer
trigger's sysfs files:

- when the two delays add up to exactly 2^32 on 32 bit, or 2^64 on
  64 bit, the sum wraps to 0 and the kernel divides by zero, for
  example with "echo 2147483648" into both delay_on and delay_off.
  That oopses on x86 and gives an arbitrary result on MIPS and arm64.
- on 32 bit the product already overflows once delay_on is above about
  4.6 hours, and a sum that wraps sets the wrong period, so long blinks
  get an arbitrary duty cycle

Saturate the sum, and compute the on time with mul_u64_u64_div_u64().

Fixes: 0ebdb7210943 ("leds: Add ktd202x driver")
Cc: stable@vger.kernel.org
Signed-off-by: Joseph Telaak <joseph@telaak.dev>
---
 drivers/leds/rgb/leds-ktd202x.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/leds/rgb/leds-ktd202x.c b/drivers/leds/rgb/leds-ktd202x.c
index 8c890ea5a754..feea33b7e885 100644
--- a/drivers/leds/rgb/leds-ktd202x.c
+++ b/drivers/leds/rgb/leds-ktd202x.c
@@ -9,10 +9,12 @@
 
 #include <linux/i2c.h>
 #include <linux/led-class-multicolor.h>
+#include <linux/math64.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/overflow.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 
@@ -296,7 +298,8 @@ static int ktd202x_blink_set(struct ktd202x_led *led, unsigned long *delay_on,
 	/* Convert into values the HW will understand. */
 
 	/* Integer representation of time of flash period */
-	delay_total_ms = *delay_on + *delay_off;
+	if (check_add_overflow(*delay_on, *delay_off, &delay_total_ms))
+		delay_total_ms = ULONG_MAX;
 	if (delay_total_ms < KTD202X_FLASH_PERIOD_MIN_MS)
 		num_steps = 0;
 	else
@@ -308,8 +311,8 @@ static int ktd202x_blink_set(struct ktd202x_led *led, unsigned long *delay_on,
 	 * Integer representation of percentage of LED ON time. A value of 0
 	 * keeps the LED dark, so use the shortest on time instead.
 	 */
-	on = (*delay_on * KTD202X_FLASH_ON_MAX) / (*delay_on + *delay_off);
-	on = max(on, 1);
+	on = mul_u64_u64_div_u64(*delay_on, KTD202X_FLASH_ON_MAX, delay_total_ms);
+	on = clamp(on, 1, KTD202X_FLASH_ON_MAX - 1);
 
 	/* Actually used delay_{on,off} values */
 	delay_total_ms = num_steps * KTD202X_FLASH_PERIOD_STEP_MS + KTD202X_FLASH_PERIOD_MIN_MS;

-- 
Git-155)

[PATCH v2 05/11] leds: rgb: ktd202x: Keep the unlit colors of a blinking LED off

[Joseph Telaak via B4 Relay]

From: Joseph Telaak <joseph@telaak.dev>

blink_set() switches every channel of a multicolor LED to the flash
timer, or to always on when delay_off is 0, including the colors whose
brightness is 0. Their current register still holds the last brightness
they had, or the 10 mA default, so for example a red blink set with a
multi_intensity of "255 0 0" also blinks green and blue.

Leave the channels of the colors with no brightness off, as
brightness_set() does. A single color LED now passes its brightness too.

Fixes: 0ebdb7210943 ("leds: Add ktd202x driver")
Cc: stable@vger.kernel.org
Signed-off-by: Joseph Telaak <joseph@telaak.dev>
---
 drivers/leds/rgb/leds-ktd202x.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/leds/rgb/leds-ktd202x.c b/drivers/leds/rgb/leds-ktd202x.c
index feea33b7e885..042de16c28bc 100644
--- a/drivers/leds/rgb/leds-ktd202x.c
+++ b/drivers/leds/rgb/leds-ktd202x.c
@@ -284,6 +284,10 @@ static int ktd202x_blink_set(struct ktd202x_led *led, unsigned long *delay_on,
 		int channel = subleds[i].channel;
 
 		ctrl_mask |= KTD202X_CHANNEL_CTRL_MASK(channel);
+		/* A color with no brightness stays off */
+		if (!subleds[i].brightness)
+			continue;
+
 		ctrl_on |= KTD202X_CHANNEL_CTRL_ON(channel);
 		ctrl_pwm1 |= KTD202X_CHANNEL_CTRL_PWM1(channel);
 	}
@@ -359,6 +363,7 @@ static int ktd202x_blink_single_set(struct led_classdev *cdev,
 	if (!*delay_on)
 		return ktd202x_brightness_single_set(cdev, LED_OFF);
 
+	info.brightness = cdev->brightness;
 	info.channel = led->index;
 
 	return ktd202x_blink_set(led, delay_on, delay_off, &info, 1);

-- 
Git-155)

[PATCH v2 06/11] dt-bindings: leds: kinetic,ktd202x: Add Etek ET6326

[Joseph Telaak via B4 Relay]

From: Joseph Telaak <joseph@telaak.dev>

The Etek ET6326 is a 3 channel LED driver with the register map, the I2C
address and the current steps of the KTD2026. It only differs in the
flash period, which is (Reg1 + 1) * 128 ms instead of 256 ms + Reg1 *
128 ms, so a KTD2026 driver runs it with blink periods that are 128 ms
off. Document it with the KTD2026 as fallback.

The ET6326 drives the status LED of the GL.iNet GL-SFT1200 (Opal).

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Signed-off-by: Joseph Telaak <joseph@telaak.dev>
---
 .../devicetree/bindings/leds/kinetic,ktd202x.yaml         | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/leds/kinetic,ktd202x.yaml b/Documentation/devicetree/bindings/leds/kinetic,ktd202x.yaml
index 832c030a5acf..b13c71721853 100644
--- a/Documentation/devicetree/bindings/leds/kinetic,ktd202x.yaml
+++ b/Documentation/devicetree/bindings/leds/kinetic,ktd202x.yaml
@@ -15,11 +15,20 @@ description: |
   The data sheet can be found at:
     https://www.kinet-ic.com/uploads/KTD2026-7-04h.pdf
 
+  The Etek ET6326 is a 3 channel variant that is register compatible with
+  the KTD2026, but encodes the flash period differently. Its data sheet can
+  be found at:
+    https://www.etekmicro.com/wp-content/uploads/datasheets/ET6326_datasheet.pdf
+
 properties:
   compatible:
-    enum:
-      - kinetic,ktd2026
-      - kinetic,ktd2027
+    oneOf:
+      - enum:
+          - kinetic,ktd2026
+          - kinetic,ktd2027
+      - items:
+          - const: etekmicro,et6326
+          - const: kinetic,ktd2026
 
   reg:
     maxItems: 1

-- 
Git-155)

[PATCH v2 07/11] leds: rgb: ktd202x: Add Etek ET6326 support

[Joseph Telaak via B4 Relay]

From: Joseph Telaak <joseph@telaak.dev>

The Etek ET6326 is a KTD2026 with a different flash period encoding:
Reg1 = n selects (n + 1) * 128 ms, from 128 ms to 16.38 s, where the
KTD202x selects 256 ms + n * 128 ms and uses Reg1 = 127 for one shot
mode.

Move the number of LEDs and the flash period limits into per chip data
and add the ET6326, so the timer trigger gets the delays it asked for.

Report the on time rounded to the nearest millisecond rather than down.
The shortest ET6326 period is 128 ms, and 1/256 of it rounded down is
0 ms, which the timer trigger passes back with the next delay_off
write, where an on time of 0 turns the LED off. Rounded, the delays
read back also give the same on time when they are written again.

Link: https://www.etekmicro.com/wp-content/uploads/datasheets/ET6326_datasheet.pdf
Signed-off-by: Joseph Telaak <joseph@telaak.dev>
---
 drivers/leds/rgb/Kconfig        |  3 +-
 drivers/leds/rgb/leds-ktd202x.c | 72 ++++++++++++++++++++++++++++++++---------
 2 files changed, 58 insertions(+), 17 deletions(-)

diff --git a/drivers/leds/rgb/Kconfig b/drivers/leds/rgb/Kconfig
index 6e9ab5f60714..3a57e5819eeb 100644
--- a/drivers/leds/rgb/Kconfig
+++ b/drivers/leds/rgb/Kconfig
@@ -20,7 +20,8 @@ config LEDS_KTD202X
 	select REGMAP_I2C
 	help
 	  This option enables support for the Kinetic KTD2026/KTD2027
-	  RGB/White LED driver found in different BQ mobile phones.
+	  RGB/White LED driver found in different BQ mobile phones, and
+	  for the compatible Etek ET6326.
 	  It is a 3 or 4 channel LED driver programmed via an I2C interface.
 
 	  To compile this driver as a module, choose M here: the module
diff --git a/drivers/leds/rgb/leds-ktd202x.c b/drivers/leds/rgb/leds-ktd202x.c
index 042de16c28bc..2650573b6c5a 100644
--- a/drivers/leds/rgb/leds-ktd202x.c
+++ b/drivers/leds/rgb/leds-ktd202x.c
@@ -5,6 +5,7 @@
  * Copyright 2023 André Apitzsch <git@apitzsch.eu>
  *
  * Datasheet: https://www.kinet-ic.com/uploads/KTD2026-7-04h.pdf
+ * Datasheet: https://www.etekmicro.com/wp-content/uploads/datasheets/ET6326_datasheet.pdf
  */
 
 #include <linux/i2c.h>
@@ -65,9 +66,7 @@
 /* Register 6-9 */
 #define KTD202X_LED_CURRENT_10_mA		0x4f
 
-#define KTD202X_FLASH_PERIOD_MIN_MS 256
 #define KTD202X_FLASH_PERIOD_STEP_MS 128
-#define KTD202X_FLASH_PERIOD_MAX_STEPS 126
 #define KTD202X_FLASH_ON_MAX 256
 
 #define KTD202X_MAX_BRIGHTNESS 192
@@ -86,6 +85,33 @@ static const struct reg_default ktd202x_reg_defaults[] = {
 	{ KTD202X_REG_LED_IOUT(3), KTD202X_LED_CURRENT_10_mA },
 };
 
+struct ktd202x_info {
+	unsigned long num_leds;
+	/* Flash period encoded by a Reg1 value of 0 */
+	unsigned int flash_period_min_ms;
+	unsigned int flash_period_max_steps;
+};
+
+/* Reg1 = 127 selects one shot mode on the KTD202x */
+static const struct ktd202x_info ktd2026_info = {
+	.num_leds = KTD2026_NUM_LEDS,
+	.flash_period_min_ms = 256,
+	.flash_period_max_steps = 126,
+};
+
+static const struct ktd202x_info ktd2027_info = {
+	.num_leds = KTD2027_NUM_LEDS,
+	.flash_period_min_ms = 256,
+	.flash_period_max_steps = 126,
+};
+
+/* The ET6326 flash period is (Reg1 + 1) * 128 ms, up to 16.38 s */
+static const struct ktd202x_info et6326_info = {
+	.num_leds = KTD2026_NUM_LEDS,
+	.flash_period_min_ms = 128,
+	.flash_period_max_steps = 127,
+};
+
 struct ktd202x_led {
 	struct ktd202x *chip;
 	union {
@@ -100,6 +126,7 @@ struct ktd202x {
 	struct regulator_bulk_data regulators[2];
 	struct device *dev;
 	struct regmap *regmap;
+	const struct ktd202x_info *info;
 	bool enabled;
 	unsigned long num_leds;
 	struct ktd202x_led leds[] __counted_by(num_leds);
@@ -271,6 +298,7 @@ static int ktd202x_blink_set(struct ktd202x_led *led, unsigned long *delay_on,
 			     unsigned long *delay_off, struct mc_subled *subleds,
 			     unsigned int num_channels)
 {
+	const struct ktd202x_info *info = led->chip->info;
 	unsigned long delay_total_ms;
 	int ret, num_steps, on;
 	u8 ctrl_mask = 0;
@@ -304,11 +332,11 @@ static int ktd202x_blink_set(struct ktd202x_led *led, unsigned long *delay_on,
 	/* Integer representation of time of flash period */
 	if (check_add_overflow(*delay_on, *delay_off, &delay_total_ms))
 		delay_total_ms = ULONG_MAX;
-	if (delay_total_ms < KTD202X_FLASH_PERIOD_MIN_MS)
+	if (delay_total_ms < info->flash_period_min_ms)
 		num_steps = 0;
 	else
-		num_steps = min_t(unsigned long, KTD202X_FLASH_PERIOD_MAX_STEPS,
-				  (delay_total_ms - KTD202X_FLASH_PERIOD_MIN_MS) /
+		num_steps = min_t(unsigned long, info->flash_period_max_steps,
+				  (delay_total_ms - info->flash_period_min_ms) /
 				  KTD202X_FLASH_PERIOD_STEP_MS);
 
 	/*
@@ -319,8 +347,14 @@ static int ktd202x_blink_set(struct ktd202x_led *led, unsigned long *delay_on,
 	on = clamp(on, 1, KTD202X_FLASH_ON_MAX - 1);
 
 	/* Actually used delay_{on,off} values */
-	delay_total_ms = num_steps * KTD202X_FLASH_PERIOD_STEP_MS + KTD202X_FLASH_PERIOD_MIN_MS;
-	*delay_on = (delay_total_ms * on) / KTD202X_FLASH_ON_MAX;
+	delay_total_ms = num_steps * KTD202X_FLASH_PERIOD_STEP_MS + info->flash_period_min_ms;
+	/*
+	 * Rounded to the nearest ms, so that the delays give the same on time
+	 * when written back. Rounded down, 1/256 of the 128 ms ET6326 period
+	 * would be 0 ms, which means never on.
+	 */
+	*delay_on = clamp(DIV_ROUND_CLOSEST(delay_total_ms * on, KTD202X_FLASH_ON_MAX),
+			  1UL, delay_total_ms - 1);
 	*delay_off = delay_total_ms - *delay_on;
 
 	/* Set timings */
@@ -548,8 +582,8 @@ static const struct regmap_config ktd202x_regmap_config = {
 static int ktd202x_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
+	const struct ktd202x_info *info;
 	struct ktd202x *chip;
-	unsigned long num_leds;
 	int count;
 	int ret;
 
@@ -557,13 +591,17 @@ static int ktd202x_probe(struct i2c_client *client)
 	if (!count || count > KTD202X_MAX_LEDS)
 		return dev_err_probe(dev, -EINVAL, "Incorrect number of leds (%d)", count);
 
+	info = i2c_get_match_data(client);
+	if (!info)
+		return -ENODEV;
+
 	/* Loops over the LEDs go up to the number of channels, not of child nodes */
-	num_leds = (unsigned long)i2c_get_match_data(client);
-	chip = devm_kzalloc(dev, struct_size(chip, leds, num_leds), GFP_KERNEL);
+	chip = devm_kzalloc(dev, struct_size(chip, leds, info->num_leds), GFP_KERNEL);
 	if (!chip)
 		return -ENOMEM;
 
-	chip->num_leds = num_leds;
+	chip->info = info;
+	chip->num_leds = info->num_leds;
 	chip->dev = dev;
 	i2c_set_clientdata(client, chip);
 
@@ -623,15 +661,17 @@ static void ktd202x_shutdown(struct i2c_client *client)
 }
 
 static const struct i2c_device_id ktd202x_id[] = {
-	{ .name = "ktd2026", .driver_data = KTD2026_NUM_LEDS },
-	{ .name = "ktd2027", .driver_data = KTD2027_NUM_LEDS },
+	{ .name = "et6326", .driver_data = (kernel_ulong_t)&et6326_info },
+	{ .name = "ktd2026", .driver_data = (kernel_ulong_t)&ktd2026_info },
+	{ .name = "ktd2027", .driver_data = (kernel_ulong_t)&ktd2027_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, ktd202x_id);
 
 static const struct of_device_id ktd202x_match_table[] = {
-	{ .compatible = "kinetic,ktd2026", .data = (void *)KTD2026_NUM_LEDS },
-	{ .compatible = "kinetic,ktd2027", .data = (void *)KTD2027_NUM_LEDS },
+	{ .compatible = "etekmicro,et6326", .data = &et6326_info },
+	{ .compatible = "kinetic,ktd2026", .data = &ktd2026_info },
+	{ .compatible = "kinetic,ktd2027", .data = &ktd2027_info },
 	{}
 };
 MODULE_DEVICE_TABLE(of, ktd202x_match_table);
@@ -649,5 +689,5 @@ static struct i2c_driver ktd202x_driver = {
 module_i2c_driver(ktd202x_driver);
 
 MODULE_AUTHOR("André Apitzsch <git@apitzsch.eu>");
-MODULE_DESCRIPTION("Kinetic KTD2026/7 LED driver");
+MODULE_DESCRIPTION("Kinetic KTD2026/7 and Etek ET6326 LED driver");
 MODULE_LICENSE("GPL");

-- 
Git-155)

[PATCH v2 08/11] leds: rgb: ktd202x: Add hardware pattern support

[Joseph Telaak via B4 Relay]

From: Joseph Telaak <joseph@telaak.dev>

The KTD202x and the ET6326 can ramp the LED current up and down on
their own: the flash timer that already does blink_set also has rise
and fall times of up to 1.92 s. Without them, a breathing light needs
the software pattern trigger, which is an I2C write every 50 ms.

Implement pattern_set and pattern_clear for the pattern trigger's
hw_pattern. A pattern is the one breathing cycle the hardware has,
"0 rise brightness high brightness fall 0 low", which the software
pattern trigger plays as the same breath, repeated forever.

blink_set now clears the ramp times, so a blink after a pattern stays
sharp.

Signed-off-by: Joseph Telaak <joseph@telaak.dev>
---
 Documentation/leds/index.rst        |   1 +
 Documentation/leds/leds-ktd202x.rst |  41 ++++++++
 drivers/leds/rgb/leds-ktd202x.c     | 191 ++++++++++++++++++++++++++++++++++--
 3 files changed, 226 insertions(+), 7 deletions(-)

diff --git a/Documentation/leds/index.rst b/Documentation/leds/index.rst
index 23fa9ff7aaf4..54d1c73684ad 100644
--- a/Documentation/leds/index.rst
+++ b/Documentation/leds/index.rst
@@ -19,6 +19,7 @@ LEDs
    leds-blinkm
    leds-cht-wcove
    leds-el15203000
+   leds-ktd202x
    leds-lm3556
    leds-lp3944
    leds-lp5521
diff --git a/Documentation/leds/leds-ktd202x.rst b/Documentation/leds/leds-ktd202x.rst
new file mode 100644
index 000000000000..cb9bc9326241
--- /dev/null
+++ b/Documentation/leds/leds-ktd202x.rst
@@ -0,0 +1,41 @@
+.. SPDX-License-Identifier: GPL-2.0-only
+
+=============================================
+Kinetic KTD2026/7 and Etek ET6326 LED driver
+=============================================
+
+The chips have a hardware timer that blinks or breathes the LEDs without the
+CPU. The timer trigger uses it for blinking, and the pattern trigger uses it
+for breathing through the ``hw_pattern`` file.
+
+The hardware pattern is one breathing cycle of four steps, which repeats
+forever::
+
+    "0 rise brightness high brightness fall 0 low"
+
+rise
+    Ramp-up time in ms, from 0 to 1920 in steps of 128 ms.
+
+high
+    Time spent at the brightness in ms.
+
+fall
+    Ramp-down time in ms, from 0 to 1920 in steps of 128 ms.
+
+low
+    Time spent off in ms.
+
+The first and the last brightness must be 0, and the two in the middle must be
+equal and not 0. ``repeat`` must be -1, its default.
+
+The sum of the four times is the flash period. The driver rounds it up to the
+next step of 128 ms, between 256 ms (128 ms on the ET6326) and 16384 ms.
+
+The flash period, the on time and the ramp times are shared by all LEDs of a
+chip. When several LEDs blink or breathe at once, they all follow the timing
+that was set last.
+
+For example, to breathe with a period of 4 seconds::
+
+    echo pattern > /sys/class/leds/blue:status/trigger
+    echo "0 1000 192 1000 192 1000 0 1000" > /sys/class/leds/blue:status/hw_pattern
diff --git a/drivers/leds/rgb/leds-ktd202x.c b/drivers/leds/rgb/leds-ktd202x.c
index 2650573b6c5a..daa04f8ecefd 100644
--- a/drivers/leds/rgb/leds-ktd202x.c
+++ b/drivers/leds/rgb/leds-ktd202x.c
@@ -62,12 +62,20 @@
 
 /* Register 5 */
 #define KTD202X_RAMP_TIMES_2_MS			0x00
+#define KTD202X_RAMP_TRISE(x)			(x)
+#define KTD202X_RAMP_TFALL(x)			((x) << 4)
 
 /* Register 6-9 */
 #define KTD202X_LED_CURRENT_10_mA		0x4f
 
 #define KTD202X_FLASH_PERIOD_STEP_MS 128
 #define KTD202X_FLASH_ON_MAX 256
+#define KTD202X_RAMP_STEP_MS 128
+#define KTD202X_RAMP_MAX_STEPS 15
+
+/* rise, high, fall and low */
+#define KTD202X_PATTERN_LEN 4
+#define KTD202X_PATTERN_MAX_MS 16384
 
 #define KTD202X_MAX_BRIGHTNESS 192
 
@@ -294,6 +302,23 @@ static int ktd202x_brightness_mc_set(struct led_classdev *cdev,
 	return ret;
 }
 
+static unsigned int ktd202x_flash_period_steps(const struct ktd202x_info *info,
+					       unsigned long period_ms)
+{
+	if (period_ms < info->flash_period_min_ms)
+		return 0;
+
+	return min_t(unsigned long, info->flash_period_max_steps,
+		     (period_ms - info->flash_period_min_ms) /
+		     KTD202X_FLASH_PERIOD_STEP_MS);
+}
+
+static unsigned long ktd202x_flash_period_ms(const struct ktd202x_info *info,
+					     unsigned int num_steps)
+{
+	return num_steps * KTD202X_FLASH_PERIOD_STEP_MS + info->flash_period_min_ms;
+}
+
 static int ktd202x_blink_set(struct ktd202x_led *led, unsigned long *delay_on,
 			     unsigned long *delay_off, struct mc_subled *subleds,
 			     unsigned int num_channels)
@@ -332,12 +357,7 @@ static int ktd202x_blink_set(struct ktd202x_led *led, unsigned long *delay_on,
 	/* Integer representation of time of flash period */
 	if (check_add_overflow(*delay_on, *delay_off, &delay_total_ms))
 		delay_total_ms = ULONG_MAX;
-	if (delay_total_ms < info->flash_period_min_ms)
-		num_steps = 0;
-	else
-		num_steps = min_t(unsigned long, info->flash_period_max_steps,
-				  (delay_total_ms - info->flash_period_min_ms) /
-				  KTD202X_FLASH_PERIOD_STEP_MS);
+	num_steps = ktd202x_flash_period_steps(info, delay_total_ms);
 
 	/*
 	 * Integer representation of percentage of LED ON time. A value of 0
@@ -347,7 +367,7 @@ static int ktd202x_blink_set(struct ktd202x_led *led, unsigned long *delay_on,
 	on = clamp(on, 1, KTD202X_FLASH_ON_MAX - 1);
 
 	/* Actually used delay_{on,off} values */
-	delay_total_ms = num_steps * KTD202X_FLASH_PERIOD_STEP_MS + info->flash_period_min_ms;
+	delay_total_ms = ktd202x_flash_period_ms(info, num_steps);
 	/*
 	 * Rounded to the nearest ms, so that the delays give the same on time
 	 * when written back. Rounded down, 1/256 of the 128 ms ET6326 period
@@ -366,6 +386,12 @@ static int ktd202x_blink_set(struct ktd202x_led *led, unsigned long *delay_on,
 	if (ret)
 		goto out;
 
+	/* Drop the ramps a hardware pattern may have left behind */
+	ret = regmap_write(led->chip->regmap, KTD202X_REG_TRISE_FALL,
+			   KTD202X_RAMP_TIMES_2_MS);
+	if (ret)
+		goto out;
+
 	ret = regmap_update_bits(led->chip->regmap, KTD202X_REG_CHANNEL_CTRL,
 				 ctrl_mask, ctrl_pwm1);
 out:
@@ -431,6 +457,153 @@ static int ktd202x_blink_mc_set(struct led_classdev *cdev,
 				 mc->num_colors);
 }
 
+/*
+ * The hardware pattern is a single breathing cycle that repeats forever:
+ *
+ *   "0 rise brightness high brightness fall 0 low"
+ *
+ * The LED ramps up for the rise time, stays at the brightness for the high
+ * time, ramps down for the fall time and stays off for the low time.
+ */
+static int ktd202x_pattern_check(struct led_classdev *cdev,
+				 struct led_pattern *pattern, u32 len,
+				 int repeat)
+{
+	/*
+	 * Only forever: the pattern trigger passes -1, or 0 when repeat
+	 * has not been written since the trigger was set.
+	 */
+	if (len != KTD202X_PATTERN_LEN || repeat > 0)
+		return -EINVAL;
+
+	if (pattern[0].brightness || pattern[3].brightness ||
+	    pattern[1].brightness != pattern[2].brightness ||
+	    pattern[1].brightness <= 0 ||
+	    pattern[1].brightness > cdev->max_brightness)
+		return -EINVAL;
+
+	return 0;
+}
+
+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 < 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);
+
+	for (i = 0; i < num_channels; i++) {
+		int channel = subleds[i].channel;
+
+		ctrl_mask |= KTD202X_CHANNEL_CTRL_MASK(channel);
+		/* A color with no brightness stays off */
+		if (!subleds[i].brightness)
+			continue;
+
+		ctrl_pwm1 |= KTD202X_CHANNEL_CTRL_PWM1(channel);
+	}
+
+	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.brightness = cdev->brightness;
+	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);
+}
+
+static int ktd202x_pattern_mc_clear(struct led_classdev *cdev)
+{
+	return ktd202x_brightness_mc_set(cdev, LED_OFF);
+}
+
 static int ktd202x_setup_led_rgb(struct ktd202x *chip, struct fwnode_handle *fwnode,
 				 struct ktd202x_led *led, struct led_init_data *init_data)
 {
@@ -482,6 +655,8 @@ static int ktd202x_setup_led_rgb(struct ktd202x *chip, struct fwnode_handle *fwn
 	cdev = &led->mcdev.led_cdev;
 	cdev->brightness_set_blocking = ktd202x_brightness_mc_set;
 	cdev->blink_set = ktd202x_blink_mc_set;
+	cdev->pattern_set = ktd202x_pattern_mc_set;
+	cdev->pattern_clear = ktd202x_pattern_mc_clear;
 
 	return devm_led_classdev_multicolor_register_ext(chip->dev, &led->mcdev, init_data);
 }
@@ -503,6 +678,8 @@ static int ktd202x_setup_led_single(struct ktd202x *chip, struct fwnode_handle *
 	cdev = &led->cdev;
 	cdev->brightness_set_blocking = ktd202x_brightness_single_set;
 	cdev->blink_set = ktd202x_blink_single_set;
+	cdev->pattern_set = ktd202x_pattern_single_set;
+	cdev->pattern_clear = ktd202x_pattern_single_clear;
 
 	return devm_led_classdev_register_ext(chip->dev, &led->cdev, init_data);
 }

-- 
Git-155)

[PATCH v2 09/11] leds: rgb: ktd202x: Limit the LED current to led-max-microamp

[Joseph Telaak via B4 Relay]

From: Joseph Telaak <joseph@telaak.dev>

The full brightness of 192 is 24 mA, which is more than many indicator
LEDs are made for, and a channel may drive several LEDs in parallel. The
GL.iNet GL-SFT1200 firmware, for example, runs each of its two channels,
three LEDs each, at 1.375 mA.

Honor the common led-max-microamp property for single color LEDs by
lowering max_brightness, since each brightness step adds 0.125 mA. Set
max_brightness before the LED is registered, and blink at max_brightness
rather than at the chip maximum.

Multicolor LEDs are left as they are: the LED core already bounds each
color intensity, and the hardware this was tested on has none.

Signed-off-by: Joseph Telaak <joseph@telaak.dev>
---
 drivers/leds/rgb/leds-ktd202x.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/leds/rgb/leds-ktd202x.c b/drivers/leds/rgb/leds-ktd202x.c
index daa04f8ecefd..2a3fda860e68 100644
--- a/drivers/leds/rgb/leds-ktd202x.c
+++ b/drivers/leds/rgb/leds-ktd202x.c
@@ -78,6 +78,7 @@
 #define KTD202X_PATTERN_MAX_MS 16384
 
 #define KTD202X_MAX_BRIGHTNESS 192
+#define KTD202X_LED_CURRENT_STEP_uA 125
 
 static const struct reg_default ktd202x_reg_defaults[] = {
 	{ KTD202X_REG_RESET_CONTROL, KTD202X_TIMER_SLOT_CONTROL_TSLOT1 |
@@ -408,7 +409,7 @@ static int ktd202x_blink_single_set(struct led_classdev *cdev,
 	int ret;
 
 	if (!cdev->brightness) {
-		ret = ktd202x_brightness_single_set(cdev, KTD202X_MAX_BRIGHTNESS);
+		ret = ktd202x_brightness_single_set(cdev, cdev->max_brightness);
 		if (ret)
 			return ret;
 	}
@@ -604,6 +605,18 @@ static int ktd202x_pattern_mc_clear(struct led_classdev *cdev)
 	return ktd202x_brightness_mc_set(cdev, LED_OFF);
 }
 
+/* The current limit of a LED in brightness steps, each of which adds 0.125 mA */
+static unsigned int ktd202x_max_brightness(struct fwnode_handle *fwnode)
+{
+	u32 max_microamp;
+
+	if (fwnode_property_read_u32(fwnode, "led-max-microamp", &max_microamp))
+		return KTD202X_MAX_BRIGHTNESS;
+
+	return clamp_t(u32, max_microamp / KTD202X_LED_CURRENT_STEP_uA, 1,
+		       KTD202X_MAX_BRIGHTNESS);
+}
+
 static int ktd202x_setup_led_rgb(struct ktd202x *chip, struct fwnode_handle *fwnode,
 				 struct ktd202x_led *led, struct led_init_data *init_data)
 {
@@ -653,6 +666,7 @@ static int ktd202x_setup_led_rgb(struct ktd202x *chip, struct fwnode_handle *fwn
 	led->mcdev.num_colors = num_channels;
 
 	cdev = &led->mcdev.led_cdev;
+	cdev->max_brightness = KTD202X_MAX_BRIGHTNESS;
 	cdev->brightness_set_blocking = ktd202x_brightness_mc_set;
 	cdev->blink_set = ktd202x_blink_mc_set;
 	cdev->pattern_set = ktd202x_pattern_mc_set;
@@ -676,6 +690,8 @@ static int ktd202x_setup_led_single(struct ktd202x *chip, struct fwnode_handle *
 	led->index = reg;
 
 	cdev = &led->cdev;
+	cdev->max_brightness = ktd202x_max_brightness(fwnode);
+
 	cdev->brightness_set_blocking = ktd202x_brightness_single_set;
 	cdev->blink_set = ktd202x_blink_single_set;
 	cdev->pattern_set = ktd202x_pattern_single_set;
@@ -715,8 +731,6 @@ static int ktd202x_add_led(struct ktd202x *chip, struct fwnode_handle *fwnode, u
 		return ret;
 	}
 
-	cdev->max_brightness = KTD202X_MAX_BRIGHTNESS;
-
 	return 0;
 }
 

-- 
Git-155)

[PATCH v2 10/11] leds: rgb: ktd202x: Fail the probe when the chip does not answer

[Joseph Telaak via B4 Relay]

From: Joseph Telaak <joseph@telaak.dev>

The driver never reads from the chip: the register cache starts from the
defaults. So with a wrong address, or no chip at all, the LEDs get
registered anyway, and every later write fails.

The reset write can't tell: the KTD202x does not ACK the byte that
resets it, and its data sheet says to ignore that. So once the reset
is done, put the chip to sleep, as ktd202x_chip_disable() does, and
fail the probe when that write fails.

Signed-off-by: Joseph Telaak <joseph@telaak.dev>
---
 drivers/leds/rgb/leds-ktd202x.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/rgb/leds-ktd202x.c b/drivers/leds/rgb/leds-ktd202x.c
index 2a3fda860e68..0cc2ec3f6a7f 100644
--- a/drivers/leds/rgb/leds-ktd202x.c
+++ b/drivers/leds/rgb/leds-ktd202x.c
@@ -738,20 +738,29 @@ static int ktd202x_probe_fw(struct ktd202x *chip)
 {
 	struct device *dev = chip->dev;
 	int count;
+	int ret;
 	int i = 0;
 
 	count = device_get_child_node_count(dev);
 	if (!count || count > chip->num_leds)
 		return -EINVAL;
 
+	/* The KTD202x does not ACK the byte that resets it, so ignore the result */
 	regmap_write(chip->regmap, KTD202X_REG_RESET_CONTROL, KTD202X_RSTR_RESET);
 
 	/* Allow the device to execute the complete reset */
 	usleep_range(200, 300);
 
-	device_for_each_child_node_scoped(dev, child) {
-		int ret = ktd202x_add_led(chip, child, i);
+	/*
+	 * Nothing is read back, so a failed write is the only sign of a missing
+	 * chip. Leave it asleep until a LED is turned on.
+	 */
+	ret = regmap_write(chip->regmap, KTD202X_REG_RESET_CONTROL, KTD202X_ENABLE_CTRL_SLEEP);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to reach the chip\n");
 
+	device_for_each_child_node_scoped(dev, child) {
+		ret = ktd202x_add_led(chip, child, i);
 		if (ret)
 			return ret;
 

-- 
Git-155)

[PATCH v2 11/11] leds: rgb: ktd202x: Don't let a blinking LED retime another one

[Joseph Telaak via B4 Relay]

From: Joseph Telaak <joseph@telaak.dev>

Every LED blinks with the PWM1 flash timer. So when a second LED blinks,
its period and on time silently replace those of the first one, while
the sysfs of the first one still shows its old delays. The same happens
between a blink and a hardware pattern, and the ramps a pattern sets
leak into a later blink.

The PWM2 timer can't take the second LED: in timer slot 1, which the
driver uses, it repeats PWM1 (Reg3 has no effect, measured on an
ET6326), and in the other slots it only runs after PWM1 within the same
period.

Track which LEDs are on the timer instead of reading the channel state
back. A LED may join the timer while no other LED uses it, or when it
asks for the very same period, on time and ramps. Otherwise it stays on
and gets -EBUSY, which makes the LED core blink it in software. Program
the timer only while no other LED uses it, so that a LED joining the
timer does not restart the blink of the others, and leave it again if
a register write fails.

Hold the chip mutex from setting the brightness until the timer is
programmed, so that a LED turned off in between can't end up on the
timer, and tell that a LED is off from the brightness it is given, not
from cdev->brightness, which the LED core writes without the mutex. A
hardware pattern checks the timer first and leaves the LED as it was
on -EBUSY. An unlit LED blinks at the brightness of a software blink
caught in its off phase rather than at full brightness.

The match is on the programmed values. The timer trigger writes delay_on
and delay_off one at a time, and each write is rounded against the other's
rounded value, so two LEDs set to the same delays can get on times a few
steps apart. The second one then blinks in software, at the timing it
asked for. The documentation says how to make them share.

Signed-off-by: Joseph Telaak <joseph@telaak.dev>
---
 Documentation/leds/leds-ktd202x.rst |  16 +-
 drivers/leds/rgb/leds-ktd202x.c     | 396 +++++++++++++++++++++++-------------
 2 files changed, 273 insertions(+), 139 deletions(-)

diff --git a/Documentation/leds/leds-ktd202x.rst b/Documentation/leds/leds-ktd202x.rst
index cb9bc9326241..64cadda20144 100644
--- a/Documentation/leds/leds-ktd202x.rst
+++ b/Documentation/leds/leds-ktd202x.rst
@@ -31,9 +31,19 @@ equal and not 0. ``repeat`` must be -1, its default.
 The sum of the four times is the flash period. The driver rounds it up to the
 next step of 128 ms, between 256 ms (128 ms on the ET6326) and 16384 ms.
 
-The flash period, the on time and the ramp times are shared by all LEDs of a
-chip. When several LEDs blink or breathe at once, they all follow the timing
-that was set last.
+All LEDs of a chip blink and breathe with one flash timer. Several LEDs can
+use it at once only with the same period, on time and ramps. A LED that asks
+for different ones while another LED is on the timer gets -EBUSY: blinking
+then falls back to the software timer, and ``hw_pattern`` returns the error
+and leaves the LED as it was.
+
+The match is on the values programmed into the chip. The timer trigger sets
+``delay_on`` and ``delay_off`` one at a time and the driver rounds each pair,
+so two LEDs given the same delays can end up with slightly different on times,
+and then the second one blinks in software. To share the timer, give the
+second LED the delays that the first one reads back. Only at the 128 ms
+ET6326 period, where a millisecond is two steps of the on time, can an odd
+on time still come out one step off.
 
 For example, to breathe with a period of 4 seconds::
 
diff --git a/drivers/leds/rgb/leds-ktd202x.c b/drivers/leds/rgb/leds-ktd202x.c
index 0cc2ec3f6a7f..e2f389d8888c 100644
--- a/drivers/leds/rgb/leds-ktd202x.c
+++ b/drivers/leds/rgb/leds-ktd202x.c
@@ -128,6 +128,8 @@ struct ktd202x_led {
 		struct led_classdev_mc mcdev;
 	};
 	u32 index;
+	/* Blinking or breathing with the flash timer */
+	bool on_timer;
 };
 
 struct ktd202x {
@@ -137,6 +139,10 @@ struct ktd202x {
 	struct regmap *regmap;
 	const struct ktd202x_info *info;
 	bool enabled;
+	/* Settings of the flash timer, valid while a LED is on it */
+	u8 flash_period;
+	u8 flash_on;
+	u8 ramp_times;
 	unsigned long num_leds;
 	struct ktd202x_led leds[] __counted_by(num_leds);
 };
@@ -196,13 +202,22 @@ static bool ktd202x_chip_in_use(struct ktd202x *chip)
 	return false;
 }
 
+/* The channel mode of a lit LED: on, or blinking with the flash timer */
+static unsigned int ktd202x_channel_mode(struct ktd202x_led *led, int channel)
+{
+	if (led->on_timer)
+		return KTD202X_CHANNEL_CTRL_PWM1(channel);
+
+	return KTD202X_CHANNEL_CTRL_ON(channel);
+}
+
+/* Called with the chip mutex held */
 static int ktd202x_brightness_set(struct ktd202x_led *led,
 				  struct mc_subled *subleds,
 				  unsigned int num_channels)
 {
-	bool mode_blink = false;
+	bool lit = false;
 	int channel;
-	int state;
 	int ret;
 	int i;
 
@@ -212,24 +227,13 @@ static int ktd202x_brightness_set(struct ktd202x_led *led,
 			return ret;
 	}
 
-	ret = regmap_read(led->chip->regmap, KTD202X_REG_CHANNEL_CTRL, &state);
-	if (ret)
-		return ret;
-
-	/*
-	 * In multicolor case, assume blink mode if PWM is set for at least one
-	 * channel because another channel cannot be in state ON at the same time
-	 */
+	/* A LED that is turned off stops blinking and leaves the timer */
 	for (i = 0; i < num_channels; i++) {
-		int channel_state;
-
-		channel = subleds[i].channel;
-		channel_state = (state >> 2 * channel) & KTD202X_CHANNEL_CTRL_MASK(0);
-		if (channel_state == KTD202X_CHANNEL_CTRL_OFF)
-			continue;
-		mode_blink = channel_state == KTD202X_CHANNEL_CTRL_PWM1(0);
-		break;
+		if (subleds[i].brightness)
+			lit = true;
 	}
+	if (!lit)
+		led->on_timer = false;
 
 	for (i = 0; i < num_channels; i++) {
 		enum led_brightness brightness;
@@ -245,10 +249,7 @@ static int ktd202x_brightness_set(struct ktd202x_led *led,
 			if (ret)
 				return ret;
 
-			if (mode_blink)
-				mode = KTD202X_CHANNEL_CTRL_PWM1(channel);
-			else
-				mode = KTD202X_CHANNEL_CTRL_ON(channel);
+			mode = ktd202x_channel_mode(led, channel);
 		} else {
 			mode = KTD202X_CHANNEL_CTRL_OFF;
 		}
@@ -264,26 +265,46 @@ static int ktd202x_brightness_set(struct ktd202x_led *led,
 	return 0;
 }
 
-static int ktd202x_brightness_single_set(struct led_classdev *cdev,
-					 enum led_brightness value)
+/* Called with the chip mutex held */
+static int __ktd202x_brightness_single_set(struct ktd202x_led *led,
+					   enum led_brightness value)
 {
-	struct ktd202x_led *led = container_of(cdev, struct ktd202x_led, cdev);
 	struct mc_subled info;
-	int ret;
 
-	cdev->brightness = value;
-
-	mutex_lock(&led->chip->mutex);
+	led->cdev.brightness = value;
 
 	info.brightness = value;
 	info.channel = led->index;
-	ret = ktd202x_brightness_set(led, &info, 1);
 
+	return ktd202x_brightness_set(led, &info, 1);
+}
+
+static int ktd202x_brightness_single_set(struct led_classdev *cdev,
+					 enum led_brightness value)
+{
+	struct ktd202x_led *led = container_of(cdev, struct ktd202x_led, cdev);
+	int ret;
+
+	mutex_lock(&led->chip->mutex);
+	ret = __ktd202x_brightness_single_set(led, value);
 	mutex_unlock(&led->chip->mutex);
 
 	return ret;
 }
 
+/* Called with the chip mutex held */
+static int __ktd202x_brightness_mc_set(struct ktd202x_led *led,
+				       enum led_brightness value)
+{
+	struct led_classdev_mc *mc = &led->mcdev;
+
+	mc->led_cdev.brightness = value;
+
+	led_mc_calc_color_components(mc, value);
+
+	return ktd202x_brightness_set(led, mc->subled_info, mc->num_colors);
+}
+
 static int ktd202x_brightness_mc_set(struct led_classdev *cdev,
 				     enum led_brightness value)
 {
@@ -291,13 +312,8 @@ static int ktd202x_brightness_mc_set(struct led_classdev *cdev,
 	struct ktd202x_led *led = container_of(mc, struct ktd202x_led, mcdev);
 	int ret;
 
-	cdev->brightness = value;
-
 	mutex_lock(&led->chip->mutex);
-
-	led_mc_calc_color_components(mc, value);
-	ret = ktd202x_brightness_set(led, mc->subled_info, mc->num_colors);
-
+	ret = __ktd202x_brightness_mc_set(led, value);
 	mutex_unlock(&led->chip->mutex);
 
 	return ret;
@@ -320,19 +336,53 @@ static unsigned long ktd202x_flash_period_ms(const struct ktd202x_info *info,
 	return num_steps * KTD202X_FLASH_PERIOD_STEP_MS + info->flash_period_min_ms;
 }
 
-static int ktd202x_blink_set(struct ktd202x_led *led, unsigned long *delay_on,
-			     unsigned long *delay_off, struct mc_subled *subleds,
-			     unsigned int num_channels)
+/*
+ * All LEDs blink with the PWM1 flash timer. PWM2 is no help: in timer slot 1
+ * it repeats PWM1, and in the other slots it only runs after PWM1 within the
+ * same period. So a LED may only join the timer while it is unused, or used
+ * with the very same period, on time and ramps, so that it never retimes
+ * another LED behind its back.
+ *
+ * Returns -EBUSY when another LED uses the timer with other settings, 1 when
+ * one uses it with the same settings, and 0 when the timer is free. Called
+ * with the chip mutex held.
+ */
+static int ktd202x_timer_check(struct ktd202x_led *led, u8 flash_period,
+			       u8 flash_on, u8 ramp_times)
 {
-	const struct ktd202x_info *info = led->chip->info;
-	unsigned long delay_total_ms;
-	int ret, num_steps, on;
+	struct ktd202x *chip = led->chip;
+	int i;
+
+	for (i = 0; i < chip->num_leds; i++) {
+		struct ktd202x_led *other = &chip->leds[i];
+
+		if (other == led || !other->on_timer)
+			continue;
+
+		if (flash_period != chip->flash_period ||
+		    flash_on != chip->flash_on || ramp_times != chip->ramp_times)
+			return -EBUSY;
+
+		return 1;
+	}
+
+	return 0;
+}
+
+/* Called with the chip mutex held */
+static int ktd202x_timer_set(struct ktd202x_led *led, struct mc_subled *subleds,
+			     unsigned int num_channels, u8 flash_period, u8 flash_on,
+			     u8 ramp_times)
+{
+	struct ktd202x *chip = led->chip;
 	u8 ctrl_mask = 0;
-	u8 ctrl_pwm1 = 0;
-	u8 ctrl_on = 0;
+	u8 ctrl_mode = 0;
+	int shared;
+	int ret;
 	int i;
 
-	mutex_lock(&led->chip->mutex);
+	shared = ktd202x_timer_check(led, flash_period, flash_on, ramp_times);
+	led->on_timer = shared >= 0;
 
 	for (i = 0; i < num_channels; i++) {
 		int channel = subleds[i].channel;
@@ -342,15 +392,79 @@ static int ktd202x_blink_set(struct ktd202x_led *led, unsigned long *delay_on,
 		if (!subleds[i].brightness)
 			continue;
 
-		ctrl_on |= KTD202X_CHANNEL_CTRL_ON(channel);
-		ctrl_pwm1 |= KTD202X_CHANNEL_CTRL_PWM1(channel);
+		ctrl_mode |= ktd202x_channel_mode(led, channel);
+	}
+
+	/* Without the timer, leave the LED steadily on */
+	if (shared < 0) {
+		ret = regmap_update_bits(chip->regmap, KTD202X_REG_CHANNEL_CTRL,
+					 ctrl_mask, ctrl_mode);
+		return ret ? ret : -EBUSY;
+	}
+
+	/*
+	 * Program the timer only while no other LED uses it, so that a LED
+	 * joining the timer does not restart the blink of the others. Always
+	 * write it then: the chip may have lost power since the last time.
+	 */
+	if (!shared) {
+		chip->flash_period = flash_period;
+		chip->flash_on = flash_on;
+		chip->ramp_times = ramp_times;
+
+		ret = regmap_write(chip->regmap, KTD202X_REG_FLASH_PERIOD, flash_period);
+		if (ret)
+			goto err;
+
+		ret = regmap_write(chip->regmap, KTD202X_REG_PWM1_TIMER, flash_on);
+		if (ret)
+			goto err;
+
+		ret = regmap_write(chip->regmap, KTD202X_REG_TRISE_FALL, ramp_times);
+		if (ret)
+			goto err;
 	}
 
+	ret = regmap_update_bits(chip->regmap, KTD202X_REG_CHANNEL_CTRL,
+				 ctrl_mask, ctrl_mode);
+	if (ret)
+		goto err;
+
+	return 0;
+
+err:
+	led->on_timer = false;
+	return ret;
+}
+
+/* Called with the chip mutex held */
+static int ktd202x_blink_set(struct ktd202x_led *led, unsigned long *delay_on,
+			     unsigned long *delay_off, struct mc_subled *subleds,
+			     unsigned int num_channels)
+{
+	const struct ktd202x_info *info = led->chip->info;
+	unsigned long delay_total_ms;
+	int ret, num_steps, on;
+	u8 ctrl_mask = 0;
+	u8 ctrl_on = 0;
+	int i;
+
 	/* Never off - brightness is already set, disable blinking */
 	if (!*delay_off) {
-		ret = regmap_update_bits(led->chip->regmap, KTD202X_REG_CHANNEL_CTRL,
-					 ctrl_mask, ctrl_on);
-		goto out;
+		for (i = 0; i < num_channels; i++) {
+			int channel = subleds[i].channel;
+
+			ctrl_mask |= KTD202X_CHANNEL_CTRL_MASK(channel);
+			/* A color with no brightness stays off */
+			if (!subleds[i].brightness)
+				continue;
+
+			ctrl_on |= KTD202X_CHANNEL_CTRL_ON(channel);
+		}
+
+		led->on_timer = false;
+		return regmap_update_bits(led->chip->regmap, KTD202X_REG_CHANNEL_CTRL,
+					  ctrl_mask, ctrl_on);
 	}
 
 	/* Convert into values the HW will understand. */
@@ -367,6 +481,15 @@ static int ktd202x_blink_set(struct ktd202x_led *led, unsigned long *delay_on,
 	on = mul_u64_u64_div_u64(*delay_on, KTD202X_FLASH_ON_MAX, delay_total_ms);
 	on = clamp(on, 1, KTD202X_FLASH_ON_MAX - 1);
 
+	/*
+	 * Without the ramps a hardware pattern may have set. On -EBUSY the
+	 * LED core falls back to software blinking.
+	 */
+	ret = ktd202x_timer_set(led, subleds, num_channels, num_steps, on,
+				KTD202X_RAMP_TIMES_2_MS);
+	if (ret)
+		return ret;
+
 	/* Actually used delay_{on,off} values */
 	delay_total_ms = ktd202x_flash_period_ms(info, num_steps);
 	/*
@@ -378,26 +501,7 @@ static int ktd202x_blink_set(struct ktd202x_led *led, unsigned long *delay_on,
 			  1UL, delay_total_ms - 1);
 	*delay_off = delay_total_ms - *delay_on;
 
-	/* Set timings */
-	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;
-
-	/* Drop the ramps a hardware pattern may have left behind */
-	ret = regmap_write(led->chip->regmap, KTD202X_REG_TRISE_FALL,
-			   KTD202X_RAMP_TIMES_2_MS);
-	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;
+	return 0;
 }
 
 static int ktd202x_blink_single_set(struct led_classdev *cdev,
@@ -408,12 +512,6 @@ static int ktd202x_blink_single_set(struct led_classdev *cdev,
 	struct mc_subled info;
 	int ret;
 
-	if (!cdev->brightness) {
-		ret = ktd202x_brightness_single_set(cdev, cdev->max_brightness);
-		if (ret)
-			return ret;
-	}
-
 	/* If no blink specified, default to 1 Hz. */
 	if (!*delay_off && !*delay_on) {
 		*delay_off = 500;
@@ -424,10 +522,26 @@ static int ktd202x_blink_single_set(struct led_classdev *cdev,
 	if (!*delay_on)
 		return ktd202x_brightness_single_set(cdev, LED_OFF);
 
+	mutex_lock(&led->chip->mutex);
+
+	/*
+	 * An unlit LED blinks at the brightness of a software blink, which may
+	 * be in its off phase, or else at full brightness.
+	 */
+	if (!cdev->brightness) {
+		ret = __ktd202x_brightness_single_set(led, cdev->blink_brightness ?:
+						      cdev->max_brightness);
+		if (ret)
+			goto out;
+	}
+
 	info.brightness = cdev->brightness;
 	info.channel = led->index;
 
-	return ktd202x_blink_set(led, delay_on, delay_off, &info, 1);
+	ret = ktd202x_blink_set(led, delay_on, delay_off, &info, 1);
+out:
+	mutex_unlock(&led->chip->mutex);
+	return ret;
 }
 
 static int ktd202x_blink_mc_set(struct led_classdev *cdev,
@@ -438,12 +552,6 @@ static int ktd202x_blink_mc_set(struct led_classdev *cdev,
 	struct ktd202x_led *led = container_of(mc, struct ktd202x_led, mcdev);
 	int ret;
 
-	if (!cdev->brightness) {
-		ret = ktd202x_brightness_mc_set(cdev, KTD202X_MAX_BRIGHTNESS);
-		if (ret)
-			return ret;
-	}
-
 	/* If no blink specified, default to 1 Hz. */
 	if (!*delay_off && !*delay_on) {
 		*delay_off = 500;
@@ -454,8 +562,24 @@ static int ktd202x_blink_mc_set(struct led_classdev *cdev,
 	if (!*delay_on)
 		return ktd202x_brightness_mc_set(cdev, LED_OFF);
 
-	return ktd202x_blink_set(led, delay_on, delay_off, mc->subled_info,
-				 mc->num_colors);
+	mutex_lock(&led->chip->mutex);
+
+	/*
+	 * An unlit LED blinks at the brightness of a software blink, which may
+	 * be in its off phase, or else at full brightness.
+	 */
+	if (!cdev->brightness) {
+		ret = __ktd202x_brightness_mc_set(led, cdev->blink_brightness ?:
+						  KTD202X_MAX_BRIGHTNESS);
+		if (ret)
+			goto out;
+	}
+
+	ret = ktd202x_blink_set(led, delay_on, delay_off, mc->subled_info,
+				mc->num_colors);
+out:
+	mutex_unlock(&led->chip->mutex);
+	return ret;
 }
 
 /*
@@ -486,18 +610,14 @@ static int ktd202x_pattern_check(struct led_classdev *cdev,
 	return 0;
 }
 
-static int ktd202x_pattern_program(struct ktd202x_led *led,
-				   struct led_pattern *pattern,
-				   struct mc_subled *subleds,
-				   unsigned int num_channels)
+/* The flash timer settings of a hardware pattern */
+static void ktd202x_pattern_timer(const struct ktd202x_info *info,
+				  struct led_pattern *pattern, u8 *flash_period,
+				  u8 *flash_on, u8 *ramp_times)
 {
-	const struct ktd202x_info *info = led->chip->info;
 	unsigned int t[KTD202X_PATTERN_LEN];
-	unsigned int rise, fall, num_steps, on;
+	unsigned int rise, fall, num_steps;
 	unsigned long period_ms, ton_ms;
-	u8 ctrl_mask = 0;
-	u8 ctrl_pwm1 = 0;
-	int ret;
 	int i;
 
 	for (i = 0; i < KTD202X_PATTERN_LEN; i++)
@@ -516,41 +636,12 @@ static int ktd202x_pattern_program(struct ktd202x_led *led,
 
 	/* 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);
-
-	for (i = 0; i < num_channels; i++) {
-		int channel = subleds[i].channel;
 
-		ctrl_mask |= KTD202X_CHANNEL_CTRL_MASK(channel);
-		/* A color with no brightness stays off */
-		if (!subleds[i].brightness)
-			continue;
-
-		ctrl_pwm1 |= KTD202X_CHANNEL_CTRL_PWM1(channel);
-	}
-
-	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;
+	*flash_period = num_steps;
+	*flash_on = clamp_t(unsigned long,
+			    DIV_ROUND_CLOSEST(ton_ms * KTD202X_FLASH_ON_MAX, period_ms),
+			    1, KTD202X_FLASH_ON_MAX - 1);
+	*ramp_times = KTD202X_RAMP_TFALL(fall) | KTD202X_RAMP_TRISE(rise);
 }
 
 static int ktd202x_pattern_single_set(struct led_classdev *cdev,
@@ -558,6 +649,7 @@ static int ktd202x_pattern_single_set(struct led_classdev *cdev,
 				      int repeat)
 {
 	struct ktd202x_led *led = container_of(cdev, struct ktd202x_led, cdev);
+	u8 flash_period, flash_on, ramp_times;
 	struct mc_subled info;
 	int ret;
 
@@ -565,14 +657,30 @@ static int ktd202x_pattern_single_set(struct led_classdev *cdev,
 	if (ret)
 		return ret;
 
-	ret = ktd202x_brightness_single_set(cdev, pattern[1].brightness);
+	ktd202x_pattern_timer(led->chip->info, pattern, &flash_period,
+			      &flash_on, &ramp_times);
+
+	mutex_lock(&led->chip->mutex);
+
+	/* While another LED uses the timer differently, leave this one alone */
+	ret = ktd202x_timer_check(led, flash_period, flash_on, ramp_times);
+	if (ret < 0)
+		goto out;
+
+	ret = __ktd202x_brightness_single_set(led, pattern[1].brightness);
 	if (ret)
-		return ret;
+		goto out;
 
-	info.brightness = cdev->brightness;
+	info.brightness = pattern[1].brightness;
 	info.channel = led->index;
 
-	return ktd202x_pattern_program(led, pattern, &info, 1);
+	ret = ktd202x_timer_set(led, &info, 1, flash_period, flash_on,
+				ramp_times);
+	if (ret)
+		__ktd202x_brightness_single_set(led, LED_OFF);
+out:
+	mutex_unlock(&led->chip->mutex);
+	return ret;
 }
 
 static int ktd202x_pattern_single_clear(struct led_classdev *cdev)
@@ -586,18 +694,34 @@ static int ktd202x_pattern_mc_set(struct led_classdev *cdev,
 {
 	struct led_classdev_mc *mc = lcdev_to_mccdev(cdev);
 	struct ktd202x_led *led = container_of(mc, struct ktd202x_led, mcdev);
+	u8 flash_period, flash_on, ramp_times;
 	int ret;
 
 	ret = ktd202x_pattern_check(cdev, pattern, len, repeat);
 	if (ret)
 		return ret;
 
-	ret = ktd202x_brightness_mc_set(cdev, pattern[1].brightness);
+	ktd202x_pattern_timer(led->chip->info, pattern, &flash_period,
+			      &flash_on, &ramp_times);
+
+	mutex_lock(&led->chip->mutex);
+
+	/* While another LED uses the timer differently, leave this one alone */
+	ret = ktd202x_timer_check(led, flash_period, flash_on, ramp_times);
+	if (ret < 0)
+		goto out;
+
+	ret = __ktd202x_brightness_mc_set(led, pattern[1].brightness);
 	if (ret)
-		return ret;
+		goto out;
 
-	return ktd202x_pattern_program(led, pattern, mc->subled_info,
-				       mc->num_colors);
+	ret = ktd202x_timer_set(led, mc->subled_info, mc->num_colors,
+				flash_period, flash_on, ramp_times);
+	if (ret)
+		__ktd202x_brightness_mc_set(led, LED_OFF);
+out:
+	mutex_unlock(&led->chip->mutex);
+	return ret;
 }
 
 static int ktd202x_pattern_mc_clear(struct led_classdev *cdev)

-- 
Git-155)

Re: [PATCH v2 06/11] dt-bindings: leds: kinetic,ktd202x: Add Etek ET6326

[sashiko-bot]

> From: Joseph Telaak <joseph@telaak.dev>
> 
> The Etek ET6326 is a 3 channel LED driver with the register map, the I2C
> address and the current steps of the KTD2026. It only differs in the
> flash period, which is (Reg1 + 1) * 128 ms instead of 256 ms + Reg1 *
> 128 ms, so a KTD2026 driver runs it with blink periods that are 128 ms
> off. Document it with the KTD2026 as fallback.
> 
> The ET6326 drives the status LED of the GL.iNet GL-SFT1200 (Opal).
> 
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
> Signed-off-by: Joseph Telaak <joseph@telaak.dev>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-ktd202x-et6326-v2-0-56e97c226658@telaak.dev?part=6

Re: [PATCH v2 01/11] leds: rgb: ktd202x: Fix the flash period of short blinks on 32 bit

[sashiko-bot]

> From: Joseph Telaak <joseph@telaak.dev>
> 
> When delay_on + delay_off is below 256 ms, the unsigned subtraction of
> KTD202X_FLASH_PERIOD_MIN_MS wraps. On 64 bit the result truncates to -1
> or -2 and gets clamped to the shortest period, but on 32 bit it
> truncates to 33554431 or 33554430 and gets clamped to the longest one,
> so asking for a fast blink gives a blink every 16 seconds.
> 
> Compare before subtracting.
> 
> Fixes: 0ebdb7210943 ("leds: Add ktd202x driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Joseph Telaak <joseph@telaak.dev>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-ktd202x-et6326-v2-0-56e97c226658@telaak.dev?part=1

Re: [PATCH v2 03/11] leds: rgb: ktd202x: Never program an on time of zero

[sashiko-bot]

> From: Joseph Telaak <joseph@telaak.dev>
> 
> blink_set() turns delay_on into a fraction of the period in 1/256 steps.
> When delay_on is less than 1/256 of the period, for example 1 ms on and
> 1000 ms off, that fraction is 0: the LED stays dark, while the timer
> trigger reports that it is blinking with delay_on = 0.
> 
> Use the shortest on time the hardware has, 1/256 of the period, and
> report it back.
> 
> Fixes: 0ebdb7210943 ("leds: Add ktd202x driver")
> Signed-off-by: Joseph Telaak <joseph@telaak.dev>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-ktd202x-et6326-v2-0-56e97c226658@telaak.dev?part=3

Re: [PATCH v2 07/11] leds: rgb: ktd202x: Add Etek ET6326 support

[sashiko-bot]

> From: Joseph Telaak <joseph@telaak.dev>
> 
> The Etek ET6326 is a KTD2026 with a different flash period encoding:
> Reg1 = n selects (n + 1) * 128 ms, from 128 ms to 16.38 s, where the
> KTD202x selects 256 ms + n * 128 ms and uses Reg1 = 127 for one shot
> mode.
> 
> Move the number of LEDs and the flash period limits into per chip data
> and add the ET6326, so the timer trigger gets the delays it asked for.
> 
> Report the on time rounded to the nearest millisecond rather than down.
> The shortest ET6326 period is 128 ms, and 1/256 of it rounded down is
> 0 ms, which the timer trigger passes back with the next delay_off
> write, where an on time of 0 turns the LED off. Rounded, the delays
> read back also give the same on time when they are written again.
> 
> Link: https://www.etekmicro.com/wp-content/uploads/datasheets/ET6326_datasheet.pdf
> Signed-off-by: Joseph Telaak <joseph@telaak.dev>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-ktd202x-et6326-v2-0-56e97c226658@telaak.dev?part=7

Re: [PATCH v2 04/11] leds: rgb: ktd202x: Fix a division by zero with long blinks

[sashiko-bot]

> From: Joseph Telaak <joseph@telaak.dev>
> 
> blink_set() works out the on time as delay_on * 256 / (delay_on +
> delay_off) in unsigned long, with delays straight from the timer
> trigger's sysfs files:
> 
> - when the two delays add up to exactly 2^32 on 32 bit, or 2^64 on
>   64 bit, the sum wraps to 0 and the kernel divides by zero, for
>   example with "echo 2147483648" into both delay_on and delay_off.
>   That oopses on x86 and gives an arbitrary result on MIPS and arm64.
> - on 32 bit the product already overflows once delay_on is above about
>   4.6 hours, and a sum that wraps sets the wrong period, so long blinks
>   get an arbitrary duty cycle
> 
> Saturate the sum, and compute the on time with mul_u64_u64_div_u64().
> 
> Fixes: 0ebdb7210943 ("leds: Add ktd202x driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Joseph Telaak <joseph@telaak.dev>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-ktd202x-et6326-v2-0-56e97c226658@telaak.dev?part=4

Re: [PATCH v2 05/11] leds: rgb: ktd202x: Keep the unlit colors of a blinking LED off

[sashiko-bot]

> From: Joseph Telaak <joseph@telaak.dev>
> 
> blink_set() switches every channel of a multicolor LED to the flash
> timer, or to always on when delay_off is 0, including the colors whose
> brightness is 0. Their current register still holds the last brightness
> they had, or the 10 mA default, so for example a red blink set with a
> multi_intensity of "255 0 0" also blinks green and blue.
> 
> Leave the channels of the colors with no brightness off, as
> brightness_set() does. A single color LED now passes its brightness too.
> 
> Fixes: 0ebdb7210943 ("leds: Add ktd202x driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Joseph Telaak <joseph@telaak.dev>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-ktd202x-et6326-v2-0-56e97c226658@telaak.dev?part=5

Re: [PATCH v2 02/11] leds: rgb: ktd202x: Fix an out-of-bounds read with unused channels

[sashiko-bot]

> From: Joseph Telaak <joseph@telaak.dev>
> 
> The LED array is allocated for the number of child nodes, but
> ktd202x_chip_in_use() walks it up to num_leds, the number of channels of
> the chip. With fewer LEDs in the device tree than channels, for example
> two LEDs on a KTD2026, it reads past the end of the allocation and the
> __counted_by() annotation hides that from the bounds checkers.
> 
> Allocate one entry per channel.
> 
> Fixes: 0ebdb7210943 ("leds: Add ktd202x driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Joseph Telaak <joseph@telaak.dev>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-ktd202x-et6326-v2-0-56e97c226658@telaak.dev?part=2

Re: [PATCH v2 09/11] leds: rgb: ktd202x: Limit the LED current to led-max-microamp

[sashiko-bot]

> From: Joseph Telaak <joseph@telaak.dev>
> 
> The full brightness of 192 is 24 mA, which is more than many indicator
> LEDs are made for, and a channel may drive several LEDs in parallel. The
> GL.iNet GL-SFT1200 firmware, for example, runs each of its two channels,
> three LEDs each, at 1.375 mA.
> 
> Honor the common led-max-microamp property for single color LEDs by
> lowering max_brightness, since each brightness step adds 0.125 mA. Set
> max_brightness before the LED is registered, and blink at max_brightness
> rather than at the chip maximum.
> 
> Multicolor LEDs are left as they are: the LED core already bounds each
> color intensity, and the hardware this was tested on has none.
> 
> Signed-off-by: Joseph Telaak <joseph@telaak.dev>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-ktd202x-et6326-v2-0-56e97c226658@telaak.dev?part=9

Re: [PATCH v2 10/11] leds: rgb: ktd202x: Fail the probe when the chip does not answer

[sashiko-bot]

> From: Joseph Telaak <joseph@telaak.dev>
> 
> The driver never reads from the chip: the register cache starts from the
> defaults. So with a wrong address, or no chip at all, the LEDs get
> registered anyway, and every later write fails.
> 
> The reset write can't tell: the KTD202x does not ACK the byte that
> resets it, and its data sheet says to ignore that. So once the reset
> is done, put the chip to sleep, as ktd202x_chip_disable() does, and
> fail the probe when that write fails.
> 
> Signed-off-by: Joseph Telaak <joseph@telaak.dev>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-ktd202x-et6326-v2-0-56e97c226658@telaak.dev?part=10

Re: [PATCH v2 08/11] leds: rgb: ktd202x: Add hardware pattern support

[sashiko-bot]

> From: Joseph Telaak <joseph@telaak.dev>
> 
> The KTD202x and the ET6326 can ramp the LED current up and down on
> their own: the flash timer that already does blink_set also has rise
> and fall times of up to 1.92 s. Without them, a breathing light needs
> the software pattern trigger, which is an I2C write every 50 ms.
> 
> Implement pattern_set and pattern_clear for the pattern trigger's
> hw_pattern. A pattern is the one breathing cycle the hardware has,
> "0 rise brightness high brightness fall 0 low", which the software
> pattern trigger plays as the same breath, repeated forever.
> 
> blink_set now clears the ramp times, so a blink after a pattern stays
> sharp.
> 
> Signed-off-by: Joseph Telaak <joseph@telaak.dev>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-ktd202x-et6326-v2-0-56e97c226658@telaak.dev?part=8

Re: [PATCH v2 11/11] leds: rgb: ktd202x: Don't let a blinking LED retime another one

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] leds: rgb: ktd202x: on_timer cleared in
  ktd202x_brightness_set() when dimmed
- [Medium] leds: rgb: ktd202x: unlocked cdev->brightness race in
  ktd202x_blink_*_set()
--

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

leds: rgb: ktd202x: Don't let a blinking LED retime another one

This commit tracks which LEDs use the shared PWM1 flash timer so that
starting a blink or hardware pattern on another LED with different timings
returns -EBUSY instead of retiming the active LEDs.

> diff --git a/drivers/leds/rgb/leds-ktd202x.c b/drivers/leds/rgb/leds-ktd202x.c
> index 0cc2ec3f6a7f6..e2f389d8888c9 100644
> --- a/drivers/leds/rgb/leds-ktd202x.c
> +++ b/drivers/leds/rgb/leds-ktd202x.c
[ ... ]
> @@ -212,24 +227,13 @@ static int ktd202x_brightness_set(struct ktd202x_led *led,
>  			return ret;
>  	}
>  
> -	ret = regmap_read(led->chip->regmap, KTD202X_REG_CHANNEL_CTRL, &state);
> -	if (ret)
> -		return ret;
> -
> -	/*
> -	 * In multicolor case, assume blink mode if PWM is set for at least one
> -	 * channel because another channel cannot be in state ON at the same time
> -	 */
> +	/* A LED that is turned off stops blinking and leaves the timer */
>  	for (i = 0; i < num_channels; i++) {
> -		int channel_state;
> -
> -		channel = subleds[i].channel;
> -		channel_state = (state >> 2 * channel) & KTD202X_CHANNEL_CTRL_MASK(0);
> -		if (channel_state == KTD202X_CHANNEL_CTRL_OFF)
> -			continue;
> -		mode_blink = channel_state == KTD202X_CHANNEL_CTRL_PWM1(0);
> -		break;
> +		if (subleds[i].brightness)
> +			lit = true;
>  	}
> +	if (!lit)
> +		led->on_timer = false;

[Severity: Medium]
Can this clear led->on_timer while a multicolor LED's overall brightness is
still non-zero?

In __ktd202x_brightness_mc_set(), led_mc_calc_color_components() scales
each subled[i].brightness with DIV_ROUND_CLOSEST(value * intensity,
max_brightness).  When a blinking or breathing multicolor LED is dimmed to
a small non-zero brightness where every channel rounds down to 0, or when
multi_intensity is set to 0 0 0, the LED core keeps the trigger active
because brightness is not LED_OFF, yet !lit clears led->on_timer.  A later
brightness or multi_intensity increase then programs
KTD202X_CHANNEL_CTRL_ON instead of KTD202X_CHANNEL_CTRL_PWM1, leaving the
LED steadily on and letting another LED overwrite the shared timer.

Conversely, ktd202x_timer_set() sets led->on_timer = true even when all
subleds[i].brightness are 0.

[ ... ]

> @@ -424,10 +522,26 @@ static int ktd202x_blink_single_set(struct led_classdev *cdev,
>  	if (!*delay_on)
>  		return ktd202x_brightness_single_set(cdev, LED_OFF);
>  
> +	mutex_lock(&led->chip->mutex);
> +
> +	/*
> +	 * An unlit LED blinks at the brightness of a software blink, which may
> +	 * be in its off phase, or else at full brightness.
> +	 */
> +	if (!cdev->brightness) {
> +		ret = __ktd202x_brightness_single_set(led, cdev->blink_brightness ?:
> +						      cdev->max_brightness);
> +		if (ret)
> +			goto out;
> +	}
> +
>  	info.brightness = cdev->brightness;
>  	info.channel = led->index;
>  
> -	return ktd202x_blink_set(led, delay_on, delay_off, &info, 1);
> +	ret = ktd202x_blink_set(led, delay_on, delay_off, &info, 1);
> +out:
> +	mutex_unlock(&led->chip->mutex);
> +	return ret;
>  }
>  
>  static int ktd202x_blink_mc_set(struct led_classdev *cdev,
> @@ -438,12 +552,6 @@ static int ktd202x_blink_mc_set(struct led_classdev *cdev,
>  	struct ktd202x_led *led = container_of(mc, struct ktd202x_led, mcdev);
>  	int ret;
>  
> -	if (!cdev->brightness) {
> -		ret = ktd202x_brightness_mc_set(cdev, KTD202X_MAX_BRIGHTNESS);
> -		if (ret)
> -			return ret;
> -	}
> -
>  	/* If no blink specified, default to 1 Hz. */
>  	if (!*delay_off && !*delay_on) {
>  		*delay_off = 500;
> @@ -454,8 +562,24 @@ static int ktd202x_blink_mc_set(struct led_classdev *cdev,
>  	if (!*delay_on)
>  		return ktd202x_brightness_mc_set(cdev, LED_OFF);
>  
> -	return ktd202x_blink_set(led, delay_on, delay_off, mc->subled_info,
> -				 mc->num_colors);
> +	mutex_lock(&led->chip->mutex);
> +
> +	/*
> +	 * An unlit LED blinks at the brightness of a software blink, which may
> +	 * be in its off phase, or else at full brightness.
> +	 */
> +	if (!cdev->brightness) {
> +		ret = __ktd202x_brightness_mc_set(led, cdev->blink_brightness ?:
> +						  KTD202X_MAX_BRIGHTNESS);
> +		if (ret)
> +			goto out;
> +	}
> +
> +	ret = ktd202x_blink_set(led, delay_on, delay_off, mc->subled_info,
> +				mc->num_colors);
> +out:
> +	mutex_unlock(&led->chip->mutex);
> +	return ret;
>  }

[Severity: Medium]
Can reading cdev->brightness in ktd202x_blink_single_set() and
ktd202x_blink_mc_set() still race with led_set_brightness_nosleep()?

Because this driver uses brightness_set_blocking,
led_set_brightness_nosleep() updates cdev->brightness without holding
chip->mutex and defers the hardware update to set_brightness_work:

CPU0 (<softirq> / sysfs)          CPU1
led_set_brightness_nosleep()
  cdev->brightness = val
  queue_work(set_brightness_work)
                                  ktd202x_blink_mc_set()
                                    mutex_lock(&led->chip->mutex)
                                    if (!cdev->brightness) -> false
                                    ktd202x_blink_set()
                                      ktd202x_timer_set()  <-- chip disabled

When the LED was off in hardware (such as after the off phase of software
blinking), skipping __ktd202x_brightness_mc_set() or
__ktd202x_brightness_single_set() leaves the chip disabled when no other
LED is lit and leaves mc->subled_info[i].brightness at 0, so
ktd202x_timer_set() writes registers to a powered-down chip and leaves the
channels off while setting led->on_timer = true.

In ktd202x_blink_single_set(), cdev->brightness is also read a second time
for info.brightness, so a concurrent led_set_brightness_nosleep(cdev,
LED_OFF) can set cdev->brightness = 0 in between and pass info.brightness =
0 to ktd202x_blink_set().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-ktd202x-et6326-v2-0-56e97c226658@telaak.dev?part=11