[PATCH] leds: expresswire: Fix chip state breakage

[PATCH] leds: expresswire: Fix chip state breakage

[Duje Mihanović]

It is possible to put the hardware state of the KTD2801 chip in an
unknown state by rapidly changing the brightness for a short period of
time (for example, with a brightness slider). When this happens, the
brightness is stuck on max and cannot be changed until the chip is power
cycled.

Fix this by disabling interrupts while the chip is being programmed.
While at it, make expresswire_power_off() use fsleep() and also unexport
some functions meant to be internal.

Signed-off-by: Duje Mihanović <duje@dujemihanovic.xyz>
---
 drivers/leds/leds-expresswire.c  | 24 +++++++++++++++++-------
 include/linux/leds-expresswire.h |  3 ---
 2 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/leds/leds-expresswire.c b/drivers/leds/leds-expresswire.c
index bb69be228a6d3c7a8f2abc7fa75bd473d4c7c79c..25c6b159a6ee93210bf8239388b5cdc434872240 100644
--- a/drivers/leds/leds-expresswire.c
+++ b/drivers/leds/leds-expresswire.c
@@ -9,6 +9,7 @@
 #include <linux/delay.h>
 #include <linux/export.h>
 #include <linux/gpio/consumer.h>
+#include <linux/irqflags.h>
 #include <linux/types.h>
 
 #include <linux/leds-expresswire.h>
@@ -16,37 +17,41 @@
 void expresswire_power_off(struct expresswire_common_props *props)
 {
 	gpiod_set_value_cansleep(props->ctrl_gpio, 0);
-	usleep_range(props->timing.poweroff_us, props->timing.poweroff_us * 2);
+	fsleep(props->timing.poweroff_us);
 }
 EXPORT_SYMBOL_NS_GPL(expresswire_power_off, "EXPRESSWIRE");
 
 void expresswire_enable(struct expresswire_common_props *props)
 {
+	unsigned long flags;
+
+	local_irq_save(flags);
+
 	gpiod_set_value(props->ctrl_gpio, 1);
 	udelay(props->timing.detect_delay_us);
 	gpiod_set_value(props->ctrl_gpio, 0);
 	udelay(props->timing.detect_us);
 	gpiod_set_value(props->ctrl_gpio, 1);
+
+	local_irq_restore(flags);
 }
 EXPORT_SYMBOL_NS_GPL(expresswire_enable, "EXPRESSWIRE");
 
-void expresswire_start(struct expresswire_common_props *props)
+static void expresswire_start(struct expresswire_common_props *props)
 {
 	gpiod_set_value(props->ctrl_gpio, 1);
 	udelay(props->timing.data_start_us);
 }
-EXPORT_SYMBOL_NS_GPL(expresswire_start, "EXPRESSWIRE");
 
-void expresswire_end(struct expresswire_common_props *props)
+static void expresswire_end(struct expresswire_common_props *props)
 {
 	gpiod_set_value(props->ctrl_gpio, 0);
 	udelay(props->timing.end_of_data_low_us);
 	gpiod_set_value(props->ctrl_gpio, 1);
 	udelay(props->timing.end_of_data_high_us);
 }
-EXPORT_SYMBOL_NS_GPL(expresswire_end, "EXPRESSWIRE");
 
-void expresswire_set_bit(struct expresswire_common_props *props, bool bit)
+static void expresswire_set_bit(struct expresswire_common_props *props, bool bit)
 {
 	if (bit) {
 		gpiod_set_value(props->ctrl_gpio, 0);
@@ -60,13 +65,18 @@ void expresswire_set_bit(struct expresswire_common_props *props, bool bit)
 		udelay(props->timing.short_bitset_us);
 	}
 }
-EXPORT_SYMBOL_NS_GPL(expresswire_set_bit, "EXPRESSWIRE");
 
 void expresswire_write_u8(struct expresswire_common_props *props, u8 val)
 {
+	unsigned long flags;
+
+	local_irq_save(flags);
+
 	expresswire_start(props);
 	for (int i = 7; i >= 0; i--)
 		expresswire_set_bit(props, val & BIT(i));
 	expresswire_end(props);
+
+	local_irq_restore(flags);
 }
 EXPORT_SYMBOL_NS_GPL(expresswire_write_u8, "EXPRESSWIRE");
diff --git a/include/linux/leds-expresswire.h b/include/linux/leds-expresswire.h
index a422921f4159f2f51e40f50ad5c9d89d9070511a..7f8c4795f69fa0337bd3b201279652c8e353c6d4 100644
--- a/include/linux/leds-expresswire.h
+++ b/include/linux/leds-expresswire.h
@@ -30,9 +30,6 @@ struct expresswire_common_props {
 
 void expresswire_power_off(struct expresswire_common_props *props);
 void expresswire_enable(struct expresswire_common_props *props);
-void expresswire_start(struct expresswire_common_props *props);
-void expresswire_end(struct expresswire_common_props *props);
-void expresswire_set_bit(struct expresswire_common_props *props, bool bit);
 void expresswire_write_u8(struct expresswire_common_props *props, u8 val);
 
 #endif /* _LEDS_EXPRESSWIRE_H */

---
base-commit: 1b237f190eb3d36f52dffe07a40b5eb210280e00
change-id: 20250829-expresswire-fix-663d1b3c32aa

Best regards,
-- 
Duje Mihanović <duje@dujemihanovic.xyz>

Re: [PATCH] leds: expresswire: Fix chip state breakage

[Karel Balej]

Duje Mihanović, 2025-08-29T23:08:30+02:00:
> It is possible to put the hardware state of the KTD2801 chip in an
> unknown state by rapidly changing the brightness for a short period of
> time (for example, with a brightness slider). When this happens, the
> brightness is stuck on max and cannot be changed until the chip is power
> cycled.
>
> Fix this by disabling interrupts while the chip is being programmed.
> While at it, make expresswire_power_off() use fsleep() and also unexport
> some functions meant to be internal.
>
> Signed-off-by: Duje Mihanović <duje@dujemihanovic.xyz>

You could also add a Fixes trailer and Cc this to stable so that it gets
backported correctly.

> ---
>  drivers/leds/leds-expresswire.c  | 24 +++++++++++++++++-------
>  include/linux/leds-expresswire.h |  3 ---
>  2 files changed, 17 insertions(+), 10 deletions(-)

I have run this on samsung,coreprimevelte over the weekend and haven't
observed any direct issues with this. I will note that I've had the WiFi
fail to load FW two times while I haven't had that happen to me for
quite some time, still given the nature of the WiFi issues, I believe it
was just a coincidence.

Also when I first loaded this, I was still able to break the chip once,
however it's entirely possible that I just copied over the wrong version
of the module and after a fresh full installation, I have not been able
to reproduce that.

The "laggines" this introduces to the backlight slider is noticable but
pretty much only when trying to change the brightness back and forth
very fast which isn't possible without this patch anyway because it
usually breaks the chip.

Tested-by: Karel Balej <balejk@matfyz.cz>