* [PATCH RESEND 0/8] Resend LED patches
@ 2022-12-26 12:36 Pali Rohár
2022-12-26 12:36 ` [PATCH RESEND 1/8] dt-bindings: leds: register-bit-led: Add active-low property Pali Rohár
` (8 more replies)
0 siblings, 9 replies; 42+ messages in thread
From: Pali Rohár @ 2022-12-26 12:36 UTC (permalink / raw)
To: Arnd Bergmann, Linus Walleij; +Cc: soc, linux-kernel
Linus Walleij suggested me to send these patches to SoC tree [1]
instead. So I'm doing it.
This patch series contains LED patches which are on the linux-leds
mailing list for a long time without any future movement. Could you
please handle them here via SoC tree? Thanks.
[1] - https://lore.kernel.org/linux-leds/CACRpkdad6WDo7rGfa4MW8zz0mLXmcPHo+SEC-yLQnRz_kdrryA@mail.gmail.com/
Marek Behún (3):
leds: turris-omnia: support HW controlled mode via private trigger
leds: turris-omnia: initialize multi-intensity to full
leds: turris-omnia: change max brightness from 255 to 1
Pali Rohár (5):
dt-bindings: leds: register-bit-led: Add active-low property
leds: syscon: Implement support for active-low property
powerpc/85xx: DTS: Add CPLD definitions for P1021RDB Combo Board CPL
Design
dt-bindings: leds: Add cznic,turris1x-leds.yaml binding
leds: Add support for Turris 1.x LEDs
.../testing/sysfs-class-led-driver-turris1x | 31 ++
.../bindings/leds/cznic,turris1x-leds.yaml | 118 +++++
.../bindings/leds/register-bit-led.yaml | 5 +
arch/powerpc/boot/dts/fsl/p1020mbg-pc.dtsi | 92 ++++
arch/powerpc/boot/dts/fsl/p1020mbg-pc_32b.dts | 6 +-
arch/powerpc/boot/dts/fsl/p1020mbg-pc_36b.dts | 6 +-
arch/powerpc/boot/dts/fsl/p1020rdb-pd.dts | 44 +-
arch/powerpc/boot/dts/fsl/p1020utm-pc.dtsi | 37 ++
arch/powerpc/boot/dts/fsl/p1020utm-pc_32b.dts | 4 +-
arch/powerpc/boot/dts/fsl/p1020utm-pc_36b.dts | 4 +-
arch/powerpc/boot/dts/fsl/p1021rdb-pc.dtsi | 37 ++
arch/powerpc/boot/dts/fsl/p1021rdb-pc_32b.dts | 5 +-
arch/powerpc/boot/dts/fsl/p1021rdb-pc_36b.dts | 5 +-
arch/powerpc/boot/dts/fsl/p2020rdb-pc.dtsi | 33 +-
drivers/leds/Kconfig | 10 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-syscon.c | 14 +-
drivers/leds/leds-turris-1x.c | 474 ++++++++++++++++++
drivers/leds/leds-turris-omnia.c | 46 +-
19 files changed, 945 insertions(+), 27 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-class-led-driver-turris1x
create mode 100644 Documentation/devicetree/bindings/leds/cznic,turris1x-leds.yaml
create mode 100644 drivers/leds/leds-turris-1x.c
--
2.20.1
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH RESEND 1/8] dt-bindings: leds: register-bit-led: Add active-low property
2022-12-26 12:36 [PATCH RESEND 0/8] Resend LED patches Pali Rohár
@ 2022-12-26 12:36 ` Pali Rohár
2023-01-27 11:16 ` Lee Jones
2022-12-26 12:36 ` [PATCH RESEND 2/8] leds: syscon: Implement support for " Pali Rohár
` (7 subsequent siblings)
8 siblings, 1 reply; 42+ messages in thread
From: Pali Rohár @ 2022-12-26 12:36 UTC (permalink / raw)
To: Arnd Bergmann, Linus Walleij; +Cc: soc, linux-kernel
Allow to define inverted logic (0 - enable LED, 1 - disable LED) via
active-low property.
Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
Documentation/devicetree/bindings/leds/register-bit-led.yaml | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/leds/register-bit-led.yaml b/Documentation/devicetree/bindings/leds/register-bit-led.yaml
index ed26ec19ecbd..418130b29caa 100644
--- a/Documentation/devicetree/bindings/leds/register-bit-led.yaml
+++ b/Documentation/devicetree/bindings/leds/register-bit-led.yaml
@@ -43,6 +43,11 @@ properties:
0x100000, 0x200000, 0x400000, 0x800000, 0x1000000, 0x2000000, 0x4000000,
0x8000000, 0x10000000, 0x20000000, 0x40000000, 0x80000000 ]
+ active-low:
+ $ref: /schemas/types.yaml#/definitions/flag
+ description:
+ LED is ON when bit in register is not set
+
offset:
description:
register offset to the register controlling this LED
--
2.20.1
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH RESEND 2/8] leds: syscon: Implement support for active-low property
2022-12-26 12:36 [PATCH RESEND 0/8] Resend LED patches Pali Rohár
2022-12-26 12:36 ` [PATCH RESEND 1/8] dt-bindings: leds: register-bit-led: Add active-low property Pali Rohár
@ 2022-12-26 12:36 ` Pali Rohár
2023-02-23 14:25 ` Lee Jones
2022-12-26 12:36 ` [PATCH RESEND 3/8] powerpc/85xx: DTS: Add CPLD definitions for P1021RDB Combo Board CPL Design Pali Rohár
` (6 subsequent siblings)
8 siblings, 1 reply; 42+ messages in thread
From: Pali Rohár @ 2022-12-26 12:36 UTC (permalink / raw)
To: Arnd Bergmann, Linus Walleij; +Cc: soc, linux-kernel
This new active-low property specify that LED has inverted logic
(0 - enable LED, 1 - disable LED).
Signed-off-by: Pali Rohár <pali@kernel.org>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
drivers/leds/leds-syscon.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/leds/leds-syscon.c b/drivers/leds/leds-syscon.c
index 7eddb8ecb44e..5e605d8438e9 100644
--- a/drivers/leds/leds-syscon.c
+++ b/drivers/leds/leds-syscon.c
@@ -29,6 +29,7 @@ struct syscon_led {
struct regmap *map;
u32 offset;
u32 mask;
+ bool active_low;
bool state;
};
@@ -41,10 +42,10 @@ static void syscon_led_set(struct led_classdev *led_cdev,
int ret;
if (value == LED_OFF) {
- val = 0;
+ val = sled->active_low ? sled->mask : 0;
sled->state = false;
} else {
- val = sled->mask;
+ val = sled->active_low ? 0 : sled->mask;
sled->state = true;
}
@@ -85,6 +86,8 @@ static int syscon_led_probe(struct platform_device *pdev)
return -EINVAL;
if (of_property_read_u32(np, "mask", &sled->mask))
return -EINVAL;
+ if (of_find_property(np, "active-low", NULL))
+ sled->active_low = true;
state = of_get_property(np, "default-state", NULL);
if (state) {
@@ -95,17 +98,20 @@ static int syscon_led_probe(struct platform_device *pdev)
if (ret < 0)
return ret;
sled->state = !!(val & sled->mask);
+ if (sled->active_low)
+ sled->state = !sled->state;
} else if (!strcmp(state, "on")) {
sled->state = true;
ret = regmap_update_bits(map, sled->offset,
sled->mask,
- sled->mask);
+ sled->active_low ? 0 : sled->mask);
if (ret < 0)
return ret;
} else {
sled->state = false;
ret = regmap_update_bits(map, sled->offset,
- sled->mask, 0);
+ sled->mask,
+ sled->active_low ? sled->mask : 0);
if (ret < 0)
return ret;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH RESEND 3/8] powerpc/85xx: DTS: Add CPLD definitions for P1021RDB Combo Board CPL Design
2022-12-26 12:36 [PATCH RESEND 0/8] Resend LED patches Pali Rohár
2022-12-26 12:36 ` [PATCH RESEND 1/8] dt-bindings: leds: register-bit-led: Add active-low property Pali Rohár
2022-12-26 12:36 ` [PATCH RESEND 2/8] leds: syscon: Implement support for " Pali Rohár
@ 2022-12-26 12:36 ` Pali Rohár
2022-12-26 12:36 ` [PATCH RESEND 4/8] dt-bindings: leds: Add cznic,turris1x-leds.yaml binding Pali Rohár
` (5 subsequent siblings)
8 siblings, 0 replies; 42+ messages in thread
From: Pali Rohár @ 2022-12-26 12:36 UTC (permalink / raw)
To: Arnd Bergmann, Linus Walleij; +Cc: soc, linux-kernel
P1021RDB Combo Board CPLD Design is used on following Freescale boards:
P1021RDB-PC, P1020RDB-PD, P1020MBG-PC, P1020UTM-PC and P2020RDB-PCA.
Add CPLD definitions for all these boards for which already exist DTS file.
CPLD has bank size 128kB, it is connected via CS3 on LBC and mapped to
memory range 0xFFA00000~0xFFA1FFFF.
As CPLD firmware is common on all these boards, use just one compatible
string "fsl,p1021rdb-pc-cpld".
In some DTS files is CPLD already defined, but definition is either
incomplete or wrong. So fix it.
All these boards have via CPLD connected max6370 watchdog at offset 0x2
with GPIO 11, status led at offset 0x8 and reset controller at offset 0xd.
Additionally P1020MBG-PC and P1020RDB-PD boards have FXO led at offset 0x9
and FXS leds at offset 0xa.
Signed-off-by: Pali Rohár <pali@kernel.org>
---
arch/powerpc/boot/dts/fsl/p1020mbg-pc.dtsi | 92 +++++++++++++++++++
arch/powerpc/boot/dts/fsl/p1020mbg-pc_32b.dts | 6 +-
arch/powerpc/boot/dts/fsl/p1020mbg-pc_36b.dts | 6 +-
arch/powerpc/boot/dts/fsl/p1020rdb-pd.dts | 44 +++++++--
arch/powerpc/boot/dts/fsl/p1020utm-pc.dtsi | 37 ++++++++
arch/powerpc/boot/dts/fsl/p1020utm-pc_32b.dts | 4 +-
arch/powerpc/boot/dts/fsl/p1020utm-pc_36b.dts | 4 +-
arch/powerpc/boot/dts/fsl/p1021rdb-pc.dtsi | 37 ++++++++
arch/powerpc/boot/dts/fsl/p1021rdb-pc_32b.dts | 5 +-
arch/powerpc/boot/dts/fsl/p1021rdb-pc_36b.dts | 5 +-
arch/powerpc/boot/dts/fsl/p2020rdb-pc.dtsi | 33 ++++++-
11 files changed, 251 insertions(+), 22 deletions(-)
diff --git a/arch/powerpc/boot/dts/fsl/p1020mbg-pc.dtsi b/arch/powerpc/boot/dts/fsl/p1020mbg-pc.dtsi
index a24699cfea9c..cad670c41572 100644
--- a/arch/powerpc/boot/dts/fsl/p1020mbg-pc.dtsi
+++ b/arch/powerpc/boot/dts/fsl/p1020mbg-pc.dtsi
@@ -83,6 +83,95 @@
compatible = "vitesse-7385";
reg = <0x2 0x0 0x20000>;
};
+
+ cpld@3,0 {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ compatible = "fsl,p1021rdb-pc-cpld", "simple-bus", "syscon";
+ reg = <0x3 0x0 0x20000>;
+ ranges = <0x0 0x3 0x0 0x20000>;
+
+ watchdog@2 {
+ compatible = "maxim,max6370";
+ reg = <0x2 0x1>;
+ gpios = <&gpio 11 1>;
+ };
+
+ led@8,0 {
+ compatible = "register-bit-led";
+ reg = <0x8 0x1>;
+ offset = <0x8>;
+ mask = <0x1>;
+ active-low;
+ default-state = "keep";
+ label = "status";
+ function = "status";
+ color = <6>; /* LED_COLOR_ID_YELLOW */
+ };
+
+ led@9,0 {
+ compatible = "register-bit-led";
+ reg = <0x9 0x1>;
+ offset = <0x9>;
+ mask = <0x1>;
+ active-low;
+ default-state = "keep";
+ label = "fxo";
+ color = <2>; /* LED_COLOR_ID_GREEN */
+ };
+
+ led@a,0 {
+ compatible = "register-bit-led";
+ reg = <0xa 0x1>;
+ offset = <0xa>;
+ mask = <0x1>;
+ active-low;
+ default-state = "keep";
+ label = "fxs0";
+ color = <2>; /* LED_COLOR_ID_GREEN */
+ };
+
+ led@a,1 {
+ compatible = "register-bit-led";
+ reg = <0xa 0x1>;
+ offset = <0xa>;
+ mask = <0x2>;
+ active-low;
+ default-state = "keep";
+ label = "fxs1";
+ color = <2>; /* LED_COLOR_ID_GREEN */
+ };
+
+ led@a,2 {
+ compatible = "register-bit-led";
+ reg = <0xa 0x1>;
+ offset = <0xa>;
+ mask = <0x4>;
+ active-low;
+ default-state = "keep";
+ label = "fxs2";
+ color = <2>; /* LED_COLOR_ID_GREEN */
+ };
+
+ led@a,3 {
+ compatible = "register-bit-led";
+ reg = <0xa 0x1>;
+ offset = <0xa>;
+ mask = <0x8>;
+ active-low;
+ default-state = "keep";
+ label = "fxs3";
+ color = <2>; /* LED_COLOR_ID_GREEN */
+ };
+
+ reboot@d {
+ compatible = "syscon-reboot";
+ reg = <0xd 0x1>;
+ offset = <0xd>;
+ mask = <0x1>;
+ value = <0x1>;
+ };
+ };
};
&soc {
@@ -93,6 +182,9 @@
};
};
+ gpio: gpio-controller@fc00 {
+ };
+
mdio@24000 {
phy0: ethernet-phy@0 {
interrupts = <3 1 0 0>;
diff --git a/arch/powerpc/boot/dts/fsl/p1020mbg-pc_32b.dts b/arch/powerpc/boot/dts/fsl/p1020mbg-pc_32b.dts
index b29d1fcb5e6b..29847d395f1f 100644
--- a/arch/powerpc/boot/dts/fsl/p1020mbg-pc_32b.dts
+++ b/arch/powerpc/boot/dts/fsl/p1020mbg-pc_32b.dts
@@ -44,10 +44,10 @@
lbc: localbus@ffe05000 {
reg = <0x0 0xffe05000 0x0 0x1000>;
- /* NOR and L2 switch */
+ /* NOR, L2 switch and CPLD */
ranges = <0x0 0x0 0x0 0xec000000 0x04000000
- 0x1 0x0 0x0 0xffa00000 0x00040000
- 0x2 0x0 0x0 0xffb00000 0x00020000>;
+ 0x2 0x0 0x0 0xffb00000 0x00020000
+ 0x3 0x0 0x0 0xffa00000 0x00040000>;
};
soc: soc@ffe00000 {
diff --git a/arch/powerpc/boot/dts/fsl/p1020mbg-pc_36b.dts b/arch/powerpc/boot/dts/fsl/p1020mbg-pc_36b.dts
index 678d0eec24e2..b0ce561c38ff 100644
--- a/arch/powerpc/boot/dts/fsl/p1020mbg-pc_36b.dts
+++ b/arch/powerpc/boot/dts/fsl/p1020mbg-pc_36b.dts
@@ -44,10 +44,10 @@
lbc: localbus@fffe05000 {
reg = <0xf 0xffe05000 0x0 0x1000>;
- /* NOR and L2 switch */
+ /* NOR, L2 switch and CPLD */
ranges = <0x0 0x0 0xf 0xec000000 0x04000000
- 0x1 0x0 0xf 0xffa00000 0x00040000
- 0x2 0x0 0xf 0xffb00000 0x00020000>;
+ 0x2 0x0 0xf 0xffb00000 0x00020000
+ 0x3 0x0 0xf 0xffa00000 0x00040000>;
};
soc: soc@fffe00000 {
diff --git a/arch/powerpc/boot/dts/fsl/p1020rdb-pd.dts b/arch/powerpc/boot/dts/fsl/p1020rdb-pd.dts
index f2dc6c09be52..c318a3cc124b 100644
--- a/arch/powerpc/boot/dts/fsl/p1020rdb-pd.dts
+++ b/arch/powerpc/boot/dts/fsl/p1020rdb-pd.dts
@@ -47,8 +47,8 @@
/* NOR, NAND flash, L2 switch and CPLD */
ranges = <0x0 0x0 0x0 0xec000000 0x04000000
0x1 0x0 0x0 0xff800000 0x00040000
- 0x2 0x0 0x0 0xffa00000 0x00020000
- 0x3 0x0 0x0 0xffb00000 0x00020000>;
+ 0x2 0x0 0x0 0xffb00000 0x00020000
+ 0x3 0x0 0x0 0xffa00000 0x00020000>;
nor@0,0 {
#address-cells = <1>;
@@ -128,16 +128,45 @@
};
};
- cpld@2,0 {
- compatible = "fsl,p1020rdb-pd-cpld";
+ L2switch@2,0 {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ compatible = "vitesse-7385";
reg = <0x2 0x0 0x20000>;
};
- L2switch@3,0 {
+ cpld@3,0 {
#address-cells = <1>;
#size-cells = <1>;
- compatible = "vitesse-7385";
+ compatible = "fsl,p1021rdb-pc-cpld", "simple-bus", "syscon";
reg = <0x3 0x0 0x20000>;
+ ranges = <0x0 0x3 0x0 0x20000>;
+
+ watchdog@2 {
+ compatible = "maxim,max6370";
+ reg = <0x2 0x1>;
+ gpios = <&gpio 11 1>;
+ };
+
+ led@8,0 {
+ compatible = "register-bit-led";
+ reg = <0x8 0x1>;
+ offset = <0x8>;
+ mask = <0x1>;
+ active-low;
+ default-state = "keep";
+ label = "status";
+ function = "status";
+ color = <6>; /* LED_COLOR_ID_YELLOW */
+ };
+
+ reboot@d {
+ compatible = "syscon-reboot";
+ reg = <0xd 0x1>;
+ offset = <0xd>;
+ mask = <0x1>;
+ value = <0x1>;
+ };
};
};
@@ -199,6 +228,9 @@
};
};
+ gpio: gpio-controller@fc00 {
+ };
+
mdio@24000 {
phy0: ethernet-phy@0 {
interrupts = <3 1 0 0>;
diff --git a/arch/powerpc/boot/dts/fsl/p1020utm-pc.dtsi b/arch/powerpc/boot/dts/fsl/p1020utm-pc.dtsi
index 7ea85eabcc5c..8a52548b83c0 100644
--- a/arch/powerpc/boot/dts/fsl/p1020utm-pc.dtsi
+++ b/arch/powerpc/boot/dts/fsl/p1020utm-pc.dtsi
@@ -68,6 +68,40 @@
read-only;
};
};
+
+ cpld@3,0 {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ compatible = "fsl,p1021rdb-pc-cpld", "simple-bus", "syscon";
+ reg = <0x3 0x0 0x20000>;
+ ranges = <0x0 0x3 0x0 0x20000>;
+
+ watchdog@2 {
+ compatible = "maxim,max6370";
+ reg = <0x2 0x1>;
+ gpios = <&gpio 11 1>;
+ };
+
+ led@8,0 {
+ compatible = "register-bit-led";
+ reg = <0x8 0x1>;
+ offset = <0x8>;
+ mask = <0x1>;
+ active-low;
+ default-state = "keep";
+ label = "status";
+ function = "status";
+ color = <6>; /* LED_COLOR_ID_YELLOW */
+ };
+
+ reboot@d {
+ compatible = "syscon-reboot";
+ reg = <0xd 0x1>;
+ offset = <0xd>;
+ mask = <0x1>;
+ value = <0x1>;
+ };
+ };
};
&soc {
@@ -78,6 +112,9 @@
};
};
+ gpio: gpio-controller@fc00 {
+ };
+
mdio@24000 {
phy0: ethernet-phy@0 {
interrupts = <3 1 0 0>;
diff --git a/arch/powerpc/boot/dts/fsl/p1020utm-pc_32b.dts b/arch/powerpc/boot/dts/fsl/p1020utm-pc_32b.dts
index bc03ef611f98..9cc8f6726ca7 100644
--- a/arch/powerpc/boot/dts/fsl/p1020utm-pc_32b.dts
+++ b/arch/powerpc/boot/dts/fsl/p1020utm-pc_32b.dts
@@ -46,8 +46,8 @@
/* NOR */
ranges = <0x0 0x0 0x0 0xec000000 0x02000000
- 0x1 0x0 0x0 0xffa00000 0x00040000
- 0x2 0x0 0x0 0xffb00000 0x00020000>;
+ 0x2 0x0 0x0 0xffb00000 0x00020000
+ 0x3 0x0 0x0 0xffa00000 0x00040000>;
};
soc: soc@ffe00000 {
diff --git a/arch/powerpc/boot/dts/fsl/p1020utm-pc_36b.dts b/arch/powerpc/boot/dts/fsl/p1020utm-pc_36b.dts
index 32766f6a475e..d60fb898399f 100644
--- a/arch/powerpc/boot/dts/fsl/p1020utm-pc_36b.dts
+++ b/arch/powerpc/boot/dts/fsl/p1020utm-pc_36b.dts
@@ -46,8 +46,8 @@
/* NOR */
ranges = <0x0 0x0 0xf 0xec000000 0x02000000
- 0x1 0x0 0xf 0xffa00000 0x00040000
- 0x2 0x0 0xf 0xffb00000 0x00020000>;
+ 0x2 0x0 0xf 0xffb00000 0x00020000
+ 0x3 0x0 0xf 0xffa00000 0x00040000>;
};
soc: soc@fffe00000 {
diff --git a/arch/powerpc/boot/dts/fsl/p1021rdb-pc.dtsi b/arch/powerpc/boot/dts/fsl/p1021rdb-pc.dtsi
index 18f9b31602d0..878624ebe392 100644
--- a/arch/powerpc/boot/dts/fsl/p1021rdb-pc.dtsi
+++ b/arch/powerpc/boot/dts/fsl/p1021rdb-pc.dtsi
@@ -136,6 +136,40 @@
compatible = "vitesse-7385";
reg = <0x2 0x0 0x20000>;
};
+
+ cpld@3,0 {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ compatible = "fsl,p1021rdb-pc-cpld", "simple-bus", "syscon";
+ reg = <0x3 0x0 0x20000>;
+ ranges = <0x0 0x3 0x0 0x20000>;
+
+ watchdog@2 {
+ compatible = "maxim,max6370";
+ reg = <0x2 0x1>;
+ gpios = <&gpio 11 1>;
+ };
+
+ led@8,0 {
+ compatible = "register-bit-led";
+ reg = <0x8 0x1>;
+ offset = <0x8>;
+ mask = <0x1>;
+ active-low;
+ default-state = "keep";
+ label = "status";
+ function = "status";
+ color = <6>; /* LED_COLOR_ID_YELLOW */
+ };
+
+ reboot@d {
+ compatible = "syscon-reboot";
+ reg = <0xd 0x1>;
+ offset = <0xd>;
+ mask = <0x1>;
+ value = <0x1>;
+ };
+ };
};
&soc {
@@ -187,6 +221,9 @@
};
};
+ gpio: gpio-controller@fc00 {
+ };
+
usb@22000 {
phy_type = "ulpi";
};
diff --git a/arch/powerpc/boot/dts/fsl/p1021rdb-pc_32b.dts b/arch/powerpc/boot/dts/fsl/p1021rdb-pc_32b.dts
index d2b4710357ac..9721e5ecc86b 100644
--- a/arch/powerpc/boot/dts/fsl/p1021rdb-pc_32b.dts
+++ b/arch/powerpc/boot/dts/fsl/p1021rdb-pc_32b.dts
@@ -44,10 +44,11 @@
lbc: localbus@ffe05000 {
reg = <0 0xffe05000 0 0x1000>;
- /* NOR, NAND Flashes and Vitesse 5 port L2 switch */
+ /* NOR, NAND Flashes, Vitesse 5 port L2 switch and CPLD */
ranges = <0x0 0x0 0x0 0xef000000 0x01000000
0x1 0x0 0x0 0xff800000 0x00040000
- 0x2 0x0 0x0 0xffb00000 0x00020000>;
+ 0x2 0x0 0x0 0xffb00000 0x00020000
+ 0x3 0x0 0x0 0xffa00000 0x00020000>;
};
soc: soc@ffe00000 {
diff --git a/arch/powerpc/boot/dts/fsl/p1021rdb-pc_36b.dts b/arch/powerpc/boot/dts/fsl/p1021rdb-pc_36b.dts
index e298c29e5606..edea14e5d806 100644
--- a/arch/powerpc/boot/dts/fsl/p1021rdb-pc_36b.dts
+++ b/arch/powerpc/boot/dts/fsl/p1021rdb-pc_36b.dts
@@ -44,10 +44,11 @@
lbc: localbus@fffe05000 {
reg = <0xf 0xffe05000 0 0x1000>;
- /* NOR, NAND Flashes and Vitesse 5 port L2 switch */
+ /* NOR, NAND Flashes, Vitesse 5 port L2 switch and CPLD */
ranges = <0x0 0x0 0xf 0xef000000 0x01000000
0x1 0x0 0xf 0xff800000 0x00040000
- 0x2 0x0 0xf 0xffb00000 0x00020000>;
+ 0x2 0x0 0xf 0xffb00000 0x00020000
+ 0x3 0x0 0xf 0xffa00000 0x00020000>;
};
soc: soc@fffe00000 {
diff --git a/arch/powerpc/boot/dts/fsl/p2020rdb-pc.dtsi b/arch/powerpc/boot/dts/fsl/p2020rdb-pc.dtsi
index 03c9afc82436..1d3e8a6639a0 100644
--- a/arch/powerpc/boot/dts/fsl/p2020rdb-pc.dtsi
+++ b/arch/powerpc/boot/dts/fsl/p2020rdb-pc.dtsi
@@ -133,9 +133,35 @@
cpld@3,0 {
#address-cells = <1>;
#size-cells = <1>;
- compatible = "cpld";
+ compatible = "fsl,p1021rdb-pc-cpld", "simple-bus", "syscon";
reg = <0x3 0x0 0x20000>;
- read-only;
+ ranges = <0x0 0x3 0x0 0x20000>;
+
+ watchdog@2 {
+ compatible = "maxim,max6370";
+ reg = <0x2 0x1>;
+ gpios = <&gpio 11 1>;
+ };
+
+ led@8,0 {
+ compatible = "register-bit-led";
+ reg = <0x8 0x1>;
+ offset = <0x8>;
+ mask = <0x1>;
+ active-low;
+ default-state = "keep";
+ label = "status";
+ function = "status";
+ color = <6>; /* LED_COLOR_ID_YELLOW */
+ };
+
+ reboot@d {
+ compatible = "syscon-reboot";
+ reg = <0xd 0x1>;
+ offset = <0xd>;
+ mask = <0x1>;
+ value = <0x1>;
+ };
};
};
@@ -188,6 +214,9 @@
};
};
+ gpio: gpio-controller@fc00 {
+ };
+
usb@22000 {
phy_type = "ulpi";
};
--
2.20.1
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH RESEND 4/8] dt-bindings: leds: Add cznic,turris1x-leds.yaml binding
2022-12-26 12:36 [PATCH RESEND 0/8] Resend LED patches Pali Rohár
` (2 preceding siblings ...)
2022-12-26 12:36 ` [PATCH RESEND 3/8] powerpc/85xx: DTS: Add CPLD definitions for P1021RDB Combo Board CPL Design Pali Rohár
@ 2022-12-26 12:36 ` Pali Rohár
2023-01-27 11:16 ` Lee Jones
2023-02-24 9:13 ` Krzysztof Kozlowski
2022-12-26 12:36 ` [PATCH RESEND 5/8] leds: Add support for Turris 1.x LEDs Pali Rohár
` (4 subsequent siblings)
8 siblings, 2 replies; 42+ messages in thread
From: Pali Rohár @ 2022-12-26 12:36 UTC (permalink / raw)
To: Arnd Bergmann, Linus Walleij; +Cc: soc, linux-kernel
Add device-tree bindings documentation for Turris 1.x RGB LEDs.
Signed-off-by: Pali Rohár <pali@kernel.org>
---
.../bindings/leds/cznic,turris1x-leds.yaml | 118 ++++++++++++++++++
1 file changed, 118 insertions(+)
create mode 100644 Documentation/devicetree/bindings/leds/cznic,turris1x-leds.yaml
diff --git a/Documentation/devicetree/bindings/leds/cznic,turris1x-leds.yaml b/Documentation/devicetree/bindings/leds/cznic,turris1x-leds.yaml
new file mode 100644
index 000000000000..bcaab5b03128
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/cznic,turris1x-leds.yaml
@@ -0,0 +1,118 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/cznic,turris1x-leds.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: CZ.NIC's Turris 1.x LEDs driver
+
+maintainers:
+ - Pali Rohár <pali@kernel.org>
+
+description:
+ This module adds support for the RGB LEDs found on the front panel of the
+ Turris 1.x routers. There are 8 RGB LEDs that are controlled by CZ.NIC CPLD
+ firmware running on Lattice FPGA. Firmware is open source and available at
+ https://gitlab.nic.cz/turris/hw/turris_cpld/-/blob/master/CZ_NIC_Router_CPLD.v
+
+properties:
+ compatible:
+ const: cznic,turris1x-leds
+
+ reg:
+ description: CPLD address range where LED registers are mapped
+ maxItems: 1
+
+ "#address-cells":
+ const: 1
+
+ "#size-cells":
+ const: 0
+
+patternProperties:
+ "^multi-led@[0-7]$":
+ type: object
+ $ref: leds-class-multicolor.yaml#
+ unevaluatedProperties: false
+
+ properties:
+ reg:
+ minimum: 0
+ maximum: 7
+
+ required:
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/leds/common.h>
+
+ cpld@3,0 {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0x0 0x3 0x0 0x00020000>;
+
+ led-controller@13 {
+ compatible = "cznic,turris1x-leds";
+ reg = <0x13 0x1d>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ multi-led@0 {
+ reg = <0x0>;
+ color = <LED_COLOR_ID_RGB>;
+ function = LED_FUNCTION_WAN;
+ };
+
+ multi-led@1 {
+ reg = <0x1>;
+ color = <LED_COLOR_ID_RGB>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <5>;
+ };
+
+ multi-led@2 {
+ reg = <0x2>;
+ color = <LED_COLOR_ID_RGB>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <4>;
+ };
+
+ multi-led@3 {
+ reg = <0x3>;
+ color = <LED_COLOR_ID_RGB>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <3>;
+ };
+
+ multi-led@4 {
+ reg = <0x4>;
+ color = <LED_COLOR_ID_RGB>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <2>;
+ };
+
+ multi-led@5 {
+ reg = <0x5>;
+ color = <LED_COLOR_ID_RGB>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <1>;
+ };
+
+ multi-led@6 {
+ reg = <0x6>;
+ color = <LED_COLOR_ID_RGB>;
+ function = LED_FUNCTION_WLAN;
+ };
+
+ multi-led@7 {
+ reg = <0x7>;
+ color = <LED_COLOR_ID_RGB>;
+ function = LED_FUNCTION_POWER;
+ };
+ };
+ };
+
+...
--
2.20.1
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH RESEND 5/8] leds: Add support for Turris 1.x LEDs
2022-12-26 12:36 [PATCH RESEND 0/8] Resend LED patches Pali Rohár
` (3 preceding siblings ...)
2022-12-26 12:36 ` [PATCH RESEND 4/8] dt-bindings: leds: Add cznic,turris1x-leds.yaml binding Pali Rohár
@ 2022-12-26 12:36 ` Pali Rohár
2023-01-27 11:20 ` Lee Jones
` (2 more replies)
2022-12-26 12:36 ` [PATCH RESEND 6/8] leds: turris-omnia: support HW controlled mode via private trigger Pali Rohár
` (3 subsequent siblings)
8 siblings, 3 replies; 42+ messages in thread
From: Pali Rohár @ 2022-12-26 12:36 UTC (permalink / raw)
To: Arnd Bergmann, Linus Walleij; +Cc: soc, linux-kernel
This adds support for the RGB LEDs found on the front panel of the
Turris 1.x routers. There are 8 RGB LEDs that are controlled by
CZ.NIC CPLD firmware running on Lattice FPGA.
CPLD firmware provides HW triggering mode for all LEDs except WiFi LED
which is automatically enabled after power on reset. LAN LEDs share HW
registers for RGB colors settings, so it is not possible to set different
colors for individual LAN LEDs.
CZ.NIC CPLD firmware is open source and available at:
https://gitlab.nic.cz/turris/hw/turris_cpld/-/blob/master/CZ_NIC_Router_CPLD.v
This driver uses the multicolor LED framework and HW led triggers.
Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <kabel@kernel.org>
---
.../testing/sysfs-class-led-driver-turris1x | 31 ++
drivers/leds/Kconfig | 9 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-turris-1x.c | 474 ++++++++++++++++++
4 files changed, 515 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-class-led-driver-turris1x
create mode 100644 drivers/leds/leds-turris-1x.c
diff --git a/Documentation/ABI/testing/sysfs-class-led-driver-turris1x b/Documentation/ABI/testing/sysfs-class-led-driver-turris1x
new file mode 100644
index 000000000000..272695ae400b
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-driver-turris1x
@@ -0,0 +1,31 @@
+What: /sys/class/leds/<led>/device/brightness
+Date: July 2022
+KernelVersion: 6.3
+Contact: Pali Rohár <pali@kernel.org>
+Description: (RW) On the back size of the Turris 1.x routers there is also
+ a button which can be used to control the intensity of all the
+ LEDs at once, so that if they are too bright, user can dim them.
+
+ The CPLD firmware cycles between 8 levels of this global
+ brightness, but this setting can have any integer value between
+ 0 and 255. It is therefore convenient to be able to change this
+ setting from software.
+
+ Format: %u
+
+What: /sys/class/leds/<led>/device/brightness_level
+Date: July 2022
+KernelVersion: 6.3
+Contact: Pali Rohár <pali@kernel.org>
+Description: (RW) Current brightness level value (0-7).
+
+ Format: %u
+
+What: /sys/class/leds/<led>/device/brightness_values
+Date: July 2022
+KernelVersion: 6.3
+Contact: Pali Rohár <pali@kernel.org>
+Description: (RW) Values of all 8 levels between which CPLD firmware cycles
+ when brightness button is pressed.
+
+ Format: %u %u %u %u %u %u %u %u
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 499d0f215a8b..6f78764e20aa 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -157,6 +157,15 @@ config LEDS_EL15203000
To compile this driver as a module, choose M here: the module
will be called leds-el15203000.
+config LEDS_TURRIS_1X
+ tristate "LED support for CZ.NIC's Turris 1.x"
+ depends on LEDS_CLASS_MULTICOLOR
+ depends on OF
+ select LEDS_TRIGGERS
+ help
+ This option enables support for LEDs found on the front side of
+ CZ.NIC's Turris 1.x routers.
+
config LEDS_TURRIS_OMNIA
tristate "LED support for CZ.NIC's Turris Omnia"
depends on LEDS_CLASS_MULTICOLOR
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 4fd2f92cd198..de08083dbbca 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -82,6 +82,7 @@ obj-$(CONFIG_LEDS_TCA6507) += leds-tca6507.o
obj-$(CONFIG_LEDS_TI_LMU_COMMON) += leds-ti-lmu-common.o
obj-$(CONFIG_LEDS_TLC591XX) += leds-tlc591xx.o
obj-$(CONFIG_LEDS_TPS6105X) += leds-tps6105x.o
+obj-$(CONFIG_LEDS_TURRIS_1X) += leds-turris-1x.o
obj-$(CONFIG_LEDS_TURRIS_OMNIA) += leds-turris-omnia.o
obj-$(CONFIG_LEDS_WM831X_STATUS) += leds-wm831x-status.o
obj-$(CONFIG_LEDS_WM8350) += leds-wm8350.o
diff --git a/drivers/leds/leds-turris-1x.c b/drivers/leds/leds-turris-1x.c
new file mode 100644
index 000000000000..cf7567b64306
--- /dev/null
+++ b/drivers/leds/leds-turris-1x.c
@@ -0,0 +1,474 @@
+// SPDX-License-Identifier: GPL-2.0
+// (C) 2022 Pali Rohár <pali@kernel.org>
+//
+// CZ.NIC's Turris 1.x LEDs driver, controlled by CPLD firmware:
+// https://gitlab.nic.cz/turris/hw/turris_cpld/-/blob/master/CZ_NIC_Router_CPLD.v
+
+#include <linux/i2c.h>
+#include <linux/led-class-multicolor.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include "leds.h"
+
+/* LED registers starts at byte 0x13 in CPLD memory map */
+#define TURRIS1X_LED_REG_OFF(reg) ((reg)-0x13)
+
+/* LEDs 1-5 share common register for setting brightness */
+#define TURRIS1X_LED_BRIGHTNESS_OFF(idx) ({ const u8 _idx = (idx) & 7; \
+ (_idx == 0) ? 0 : \
+ (_idx <= 5) ? 1 : \
+ (_idx - 4); })
+
+#define TURRIS1X_LED_BRIGHTNESS_REG(idx, color) TURRIS1X_LED_REG_OFF(0x13 + \
+ 3 * TURRIS1X_LED_BRIGHTNESS_OFF(idx) + \
+ ((color) & 3))
+#define TURRIS1X_LED_GLOBAL_LEVEL_REG TURRIS1X_LED_REG_OFF(0x20)
+#define TURRIS1X_LED_GET_GLOBAL_BRIGHTNESS_REG TURRIS1X_LED_REG_OFF(0x21)
+#define TURRIS1X_LED_SW_OVERRIDE_REG TURRIS1X_LED_REG_OFF(0x22)
+#define TURRIS1X_LED_SW_DISABLE_REG TURRIS1X_LED_REG_OFF(0x23)
+#define TURRIS1X_LED_GLOBAL_BRIGHTNESS_REG(lvl) TURRIS1X_LED_REG_OFF(0x28 + ((lvl) & 7))
+
+struct turris1x_led {
+ struct led_classdev_mc mc_cdev;
+ struct mc_subled subled_info[3];
+ u32 reg;
+ bool registered;
+};
+
+#define to_turris1x_led(l) container_of(l, struct turris1x_led, mc_cdev)
+
+struct turris1x_leds {
+ void __iomem *regs;
+ struct mutex lock;
+ struct turris1x_led leds[8];
+};
+
+static struct led_hw_trigger_type turris1x_hw_trigger_type;
+
+static int turris1x_hwtrig_activate(struct led_classdev *cdev)
+{
+ struct turris1x_leds *leds = dev_get_drvdata(cdev->dev->parent);
+ struct turris1x_led *led = to_turris1x_led(lcdev_to_mccdev(cdev));
+ u8 val;
+
+ /* Disable software control of LED */
+ mutex_lock(&leds->lock);
+ val = readb(leds->regs + TURRIS1X_LED_SW_OVERRIDE_REG);
+ val &= ~BIT(led->reg);
+ writeb(val, leds->regs + TURRIS1X_LED_SW_OVERRIDE_REG);
+ mutex_unlock(&leds->lock);
+
+ return 0;
+}
+
+static void turris1x_hwtrig_deactivate(struct led_classdev *cdev)
+{
+ struct turris1x_leds *leds = dev_get_drvdata(cdev->dev->parent);
+ struct turris1x_led *led = to_turris1x_led(lcdev_to_mccdev(cdev));
+ u8 val;
+
+ /* Enable software control of LED */
+ mutex_lock(&leds->lock);
+ val = readb(leds->regs + TURRIS1X_LED_SW_OVERRIDE_REG);
+ val |= BIT(led->reg);
+ writeb(val, leds->regs + TURRIS1X_LED_SW_OVERRIDE_REG);
+ mutex_unlock(&leds->lock);
+}
+
+static struct led_trigger turris1x_hw_trigger = {
+ .name = "turris1x-cpld",
+ .activate = turris1x_hwtrig_activate,
+ .deactivate = turris1x_hwtrig_deactivate,
+ .trigger_type = &turris1x_hw_trigger_type,
+};
+
+static enum led_brightness turris1x_led_brightness_get(struct led_classdev *cdev)
+{
+ struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
+ struct turris1x_leds *leds = dev_get_drvdata(cdev->dev->parent);
+ struct turris1x_led *led = to_turris1x_led(mc_cdev);
+
+ if (!(readb(leds->regs + TURRIS1X_LED_SW_OVERRIDE_REG) & BIT(led->reg)))
+ return 1;
+ else if (!(readb(leds->regs + TURRIS1X_LED_SW_DISABLE_REG) & BIT(led->reg)))
+ return 1;
+ else
+ return 0;
+}
+
+static void turris1x_led_brightness_set(struct led_classdev *cdev,
+ enum led_brightness brightness)
+{
+ struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
+ struct turris1x_leds *leds = dev_get_drvdata(cdev->dev->parent);
+ struct turris1x_led *led = to_turris1x_led(mc_cdev);
+ int i, j;
+ u8 val;
+
+ mutex_lock(&leds->lock);
+
+ /* Set new brightness value for each color when LED is enabled */
+ if (brightness) {
+ led_mc_calc_color_components(mc_cdev, brightness);
+ for (i = 0; i < ARRAY_SIZE(led->subled_info); i++)
+ writeb(mc_cdev->subled_info[i].brightness,
+ leds->regs + TURRIS1X_LED_BRIGHTNESS_REG(led->reg, i));
+
+ /*
+ * LEDs 1-5 (LAN) share common color settings in same sets
+ * of HW registers and therefore it is not possible to set
+ * different colors. So when chaning color of one LED then
+ * reflect color change for all of them.
+ */
+ if (led->reg >= 1 && led->reg <= 5) {
+ for (j = 0; j < ARRAY_SIZE(leds->leds); j++) {
+ if (leds->leds[j].reg < 1 ||
+ leds->leds[j].reg > 5 ||
+ leds->leds[j].reg == led->reg)
+ continue;
+ for (i = 0; i < ARRAY_SIZE(led->subled_info); i++)
+ leds->leds[j].mc_cdev.subled_info[i].intensity =
+ mc_cdev->subled_info[i].intensity;
+ }
+ }
+ }
+
+ /* Enable / disable LED for software control */
+ val = readb(leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
+ if (brightness && (val & BIT(led->reg)))
+ writeb(val & ~BIT(led->reg),
+ leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
+ else if (!brightness && !(val & BIT(led->reg)))
+ writeb(val | BIT(led->reg),
+ leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
+
+ mutex_unlock(&leds->lock);
+}
+
+static int turris1x_led_register(struct device *dev, struct turris1x_leds *leds,
+ struct device_node *np, u8 val_sw_override,
+ u8 val_sw_disable)
+{
+ struct led_init_data init_data = {};
+ struct led_classdev *cdev;
+ struct turris1x_led *led;
+ int ret, color;
+ u32 reg;
+ int i;
+
+ const unsigned int colors[ARRAY_SIZE(led->subled_info)] = {
+ LED_COLOR_ID_RED, LED_COLOR_ID_GREEN, LED_COLOR_ID_BLUE
+ };
+
+ ret = of_property_read_u32(np, "reg", ®);
+ if (ret || reg >= ARRAY_SIZE(leds->leds)) {
+ dev_err(dev,
+ "Node %pOF: must contain 'reg' property with values between 0 and %u\n",
+ np, (unsigned int)ARRAY_SIZE(leds->leds) - 1);
+ return -EINVAL;
+ }
+
+ ret = of_property_read_u32(np, "color", &color);
+ if (ret || color != LED_COLOR_ID_RGB) {
+ dev_err(dev,
+ "Node %pOF: must contain 'color' property with value LED_COLOR_ID_RGB\n",
+ np);
+ return -EINVAL;
+ }
+
+ led = &leds->leds[reg];
+
+ if (led->registered) {
+ dev_err(dev, "Node %pOF: duplicate 'reg' property %u\n",
+ np, reg);
+ return -EINVAL;
+ }
+
+ led->registered = true;
+ led->reg = reg;
+
+ /* Set initial colors to what are currently in use */
+ for (i = 0; i < ARRAY_SIZE(led->subled_info); i++) {
+ led->subled_info[i].intensity =
+ readb(leds->regs + TURRIS1X_LED_BRIGHTNESS_REG(reg, i));
+ led->subled_info[i].color_index = colors[i];
+ led->subled_info[i].channel = i;
+ }
+
+ led->mc_cdev.subled_info = led->subled_info;
+ led->mc_cdev.num_colors = ARRAY_SIZE(led->subled_info);
+
+ init_data.fwnode = &np->fwnode;
+
+ cdev = &led->mc_cdev.led_cdev;
+ cdev->max_brightness = 1;
+ cdev->brightness_get = turris1x_led_brightness_get;
+ cdev->brightness_set = turris1x_led_brightness_set;
+
+ /* All LEDs except LED 6 (WiFi) can be controller by hardware trigger */
+ if (reg != 6)
+ cdev->trigger_type = &turris1x_hw_trigger_type;
+
+ /* Disable hardware trigger for LED 6 (WiFi) - allow software control */
+ if (reg == 6 && !(val_sw_override & BIT(6))) {
+ if (!(val_sw_disable & BIT(6))) {
+ val_sw_disable |= BIT(6);
+ writeb(val_sw_disable,
+ leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
+ }
+ val_sw_override |= BIT(6);
+ writeb(val_sw_override,
+ leds->regs + TURRIS1X_LED_SW_OVERRIDE_REG);
+ }
+
+ if (!(val_sw_override & BIT(reg)))
+ cdev->default_trigger = turris1x_hw_trigger.name;
+
+ if (!(val_sw_override & BIT(reg)) || !(val_sw_disable & BIT(reg)))
+ cdev->brightness = 1;
+
+ ret = devm_led_classdev_multicolor_register_ext(dev, &led->mc_cdev,
+ &init_data);
+ if (ret) {
+ dev_err(dev, "Cannot register LED %pOF: %i\n", np, ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static ssize_t brightness_show(struct device *dev, struct device_attribute *a,
+ char *buf)
+{
+ struct turris1x_leds *leds = dev_get_drvdata(dev);
+ unsigned int brightness;
+
+ /*
+ * Current brightness value is available in read-only register
+ * TURRIS1X_LED_GET_GLOBAL_BRIGHTNESS_REG. Equivalent code is:
+ * level = readb(leds->regs + TURRIS1X_LED_GLOBAL_LEVEL_REG) & 7;
+ * brightness = readb(leds->regs + TURRIS1X_LED_GLOBAL_BRIGHTNESS_REG(level));
+ */
+ brightness = readb(leds->regs + TURRIS1X_LED_GET_GLOBAL_BRIGHTNESS_REG);
+
+ return sprintf(buf, "%u\n", brightness);
+}
+
+static ssize_t brightness_store(struct device *dev, struct device_attribute *a,
+ const char *buf, size_t count)
+{
+ struct turris1x_leds *leds = dev_get_drvdata(dev);
+ int best_error, error, level, value;
+ unsigned long brightness;
+ u8 best_level;
+
+ if (kstrtoul(buf, 10, &brightness))
+ return -EINVAL;
+
+ if (brightness > 255)
+ return -EINVAL;
+
+ /*
+ * Brightness can be set only to one of 8 predefined value levels
+ * available in TURRIS1X_LED_GLOBAL_BRIGHTNESS_REG(level) registers.
+ * Choose level which has nearest value to the specified brightness.
+ */
+ best_level = 0;
+ best_error = INT_MAX;
+ for (level = 0; level < 8; level++) {
+ value = readb(leds->regs +
+ TURRIS1X_LED_GLOBAL_BRIGHTNESS_REG(level));
+ error = abs(value - (int)brightness);
+ if (best_error > error) {
+ best_error = error;
+ best_level = level;
+ }
+ }
+
+ writeb(best_level, leds->regs + TURRIS1X_LED_GLOBAL_LEVEL_REG);
+
+ return count;
+}
+static DEVICE_ATTR_RW(brightness);
+
+static ssize_t brightness_level_show(struct device *dev,
+ struct device_attribute *a, char *buf)
+{
+ struct turris1x_leds *leds = dev_get_drvdata(dev);
+ unsigned int level;
+
+ level = readb(leds->regs + TURRIS1X_LED_GLOBAL_LEVEL_REG) & 7;
+
+ return sprintf(buf, "%u\n", level);
+}
+
+static ssize_t brightness_level_store(struct device *dev,
+ struct device_attribute *a,
+ const char *buf, size_t count)
+{
+ struct turris1x_leds *leds = dev_get_drvdata(dev);
+ unsigned long level;
+
+ if (kstrtoul(buf, 10, &level))
+ return -EINVAL;
+
+ if (level > 7)
+ return -EINVAL;
+
+ writeb(level, leds->regs + TURRIS1X_LED_GLOBAL_LEVEL_REG);
+
+ return count;
+}
+static DEVICE_ATTR_RW(brightness_level);
+
+static ssize_t brightness_values_show(struct device *dev,
+ struct device_attribute *a, char *buf)
+{
+ struct turris1x_leds *leds = dev_get_drvdata(dev);
+ unsigned int vals[8];
+ int i;
+
+ for (i = 0; i < 8; i++)
+ vals[i] = readb(leds->regs +
+ TURRIS1X_LED_GLOBAL_BRIGHTNESS_REG(i));
+
+ return sprintf(buf, "%u %u %u %u %u %u %u %u\n", vals[0], vals[1],
+ vals[2], vals[3], vals[4], vals[5], vals[6], vals[7]);
+}
+
+static ssize_t brightness_values_store(struct device *dev,
+ struct device_attribute *a,
+ const char *buf, size_t count)
+{
+ struct turris1x_leds *leds = dev_get_drvdata(dev);
+ unsigned int vals[8];
+ int nchars;
+ int i;
+
+ if (sscanf(buf, "%u %u %u %u %u %u %u %u%n", &vals[0], &vals[1],
+ &vals[2], &vals[3], &vals[4], &vals[5], &vals[6], &vals[7],
+ &nchars) != 8 || nchars + 1 < count)
+ return -EINVAL;
+
+ for (i = 0; i < 8; i++)
+ writeb(vals[i],
+ leds->regs + TURRIS1X_LED_GLOBAL_BRIGHTNESS_REG(i));
+
+ return count;
+}
+static DEVICE_ATTR_RW(brightness_values);
+
+static struct attribute *turris1x_leds_controller_attrs[] = {
+ &dev_attr_brightness.attr,
+ &dev_attr_brightness_level.attr,
+ &dev_attr_brightness_values.attr,
+ NULL,
+};
+ATTRIBUTE_GROUPS(turris1x_leds_controller);
+
+static int turris1x_leds_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct device_node *np = dev_of_node(dev);
+ struct device_node *child;
+ struct turris1x_leds *leds;
+ struct resource *res;
+ void __iomem *regs;
+ u8 val_sw_override;
+ u8 val_sw_disable;
+ int ret;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res)
+ return -ENODEV;
+
+ regs = devm_ioremap_resource(dev, res);
+ if (IS_ERR(regs))
+ return PTR_ERR(regs);
+
+ leds = devm_kzalloc(dev, sizeof(*leds), GFP_KERNEL);
+ if (!leds)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, leds);
+
+ leds->regs = regs;
+ mutex_init(&leds->lock);
+
+ ret = devm_led_trigger_register(dev, &turris1x_hw_trigger);
+ if (ret) {
+ dev_err(dev, "Cannot register private LED trigger: %d\n", ret);
+ return ret;
+ }
+
+ val_sw_override = readb(leds->regs + TURRIS1X_LED_SW_OVERRIDE_REG);
+ val_sw_disable = readb(leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
+
+ for_each_available_child_of_node(np, child) {
+ ret = turris1x_led_register(dev, leds, child,
+ val_sw_override, val_sw_disable);
+ if (ret) {
+ of_node_put(child);
+ return ret;
+ }
+ }
+
+ ret = devm_device_add_groups(dev, turris1x_leds_controller_groups);
+ if (ret) {
+ dev_err(dev, "Could not add attribute group!\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+static void turris1x_leds_shutdown(struct platform_device *pdev)
+{
+ struct turris1x_leds *leds = platform_get_drvdata(pdev);
+ int i, j;
+ u8 val;
+
+ /*
+ * LED registers are persisent across board resets.
+ * So reset LEDs to default state before kernel reboots.
+ */
+
+ /* Disable software control of all LEDs except LED 6 (WiFi) */
+ writeb(BIT(6), leds->regs + TURRIS1X_LED_SW_OVERRIDE_REG);
+
+ /* Turn off LED 6 (WiFi) as there is no hardware trigger for it */
+ val = readb(leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
+ writeb(val | BIT(6), leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
+
+ /* Reset colors of all LEDs to default values */
+ for (i = 0; i < ARRAY_SIZE(leds->leds); i++) {
+ /* Skip LAN2-LAN5 LEDs which share color register with LAN1 */
+ if (i >= 2 && i <= 5)
+ continue;
+ for (j = 0; j < ARRAY_SIZE(leds->leds[i].subled_info); j++)
+ writeb(0xff,
+ leds->regs + TURRIS1X_LED_BRIGHTNESS_REG(i, j));
+ }
+}
+
+static const struct of_device_id of_turris1x_leds_match[] = {
+ { .compatible = "cznic,turris1x-leds" },
+ {},
+};
+
+static struct platform_driver turris1x_leds_driver = {
+ .probe = turris1x_leds_probe,
+ .shutdown = turris1x_leds_shutdown,
+ .driver = {
+ .name = "turris1x_leds",
+ .of_match_table = of_turris1x_leds_match,
+ },
+};
+module_platform_driver(turris1x_leds_driver);
+
+MODULE_AUTHOR("Pali Rohár <pali@kernel.org>");
+MODULE_DESCRIPTION("CZ.NIC's Turris 1.x LEDs");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:turris1x_leds");
--
2.20.1
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH RESEND 6/8] leds: turris-omnia: support HW controlled mode via private trigger
2022-12-26 12:36 [PATCH RESEND 0/8] Resend LED patches Pali Rohár
` (4 preceding siblings ...)
2022-12-26 12:36 ` [PATCH RESEND 5/8] leds: Add support for Turris 1.x LEDs Pali Rohár
@ 2022-12-26 12:36 ` Pali Rohár
2023-02-24 9:32 ` Lee Jones
2022-12-26 12:36 ` [PATCH RESEND 7/8] leds: turris-omnia: initialize multi-intensity to full Pali Rohár
` (2 subsequent siblings)
8 siblings, 1 reply; 42+ messages in thread
From: Pali Rohár @ 2022-12-26 12:36 UTC (permalink / raw)
To: Arnd Bergmann, Linus Walleij; +Cc: soc, linux-kernel
From: Marek Behún <kabel@kernel.org>
Add support for enabling MCU controlled mode of the Turris Omnia LEDs
via a LED private trigger called "omnia-mcu".
When in MCU controlled mode, the user can still set LED color, but the
blinking is done by MCU, which does different things for various LEDs:
- WAN LED is blinked according to the LED[0] pin of the WAN PHY
- LAN LEDs are blinked according to the LED[0] output of corresponding
port of the LAN switch
- PCIe LEDs are blinked according to the logical OR of the MiniPCIe port
LED pins
For a long time I wanted to actually do this differently: I wanted to
make the netdev trigger to transparently offload the blinking to the HW
if user set compatible settings for the netdev trigger.
There was some work on this, and hopefully we will be able to complete
it sometime, but since there are various complications, it will probably
not be soon.
In the meantime let's support HW controlled mode via this private LED
trigger. If, in the future, we manage to complete the netdev trigger
offloading, we can still keep this private trigger for backwards
compatibility, if needed.
We also set "omnia-mcu" to cdev->default_trigger, so that the MCU keeps
control until the user first wants to take over it. If a different
default trigger is specified in device-tree via the
`linux,default-trigger` property, LED class will overwrite
cdev->default_trigger, and so the DT property will be respected.
Signed-off-by: Marek Behún <kabel@kernel.org>
Fixes: 40624346b7ae ("ARM: dts: turris-omnia: enable LED controller node")
Reviewed-by: Pali Rohár <pali@kernel.org>
---
drivers/leds/Kconfig | 1 +
drivers/leds/leds-turris-omnia.c | 41 ++++++++++++++++++++++++++++++++
2 files changed, 42 insertions(+)
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 6f78764e20aa..f957b86411de 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -172,6 +172,7 @@ config LEDS_TURRIS_OMNIA
depends on I2C
depends on MACH_ARMADA_38X || COMPILE_TEST
depends on OF
+ select LEDS_TRIGGERS
help
This option enables basic support for the LEDs found on the front
side of CZ.NIC's Turris Omnia router. There are 12 RGB LEDs on the
diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c
index c7c9851c894a..bb7af9e59ad6 100644
--- a/drivers/leds/leds-turris-omnia.c
+++ b/drivers/leds/leds-turris-omnia.c
@@ -41,6 +41,39 @@ struct omnia_leds {
struct omnia_led leds[];
};
+static struct led_hw_trigger_type omnia_hw_trigger_type;
+
+static int omnia_hwtrig_activate(struct led_classdev *cdev)
+{
+ struct omnia_leds *leds = dev_get_drvdata(cdev->dev->parent);
+ struct omnia_led *led = to_omnia_led(lcdev_to_mccdev(cdev));
+
+ /* put the LED into MCU controlled mode */
+ return i2c_smbus_write_byte_data(leds->client, CMD_LED_MODE,
+ CMD_LED_MODE_LED(led->reg));
+}
+
+static void omnia_hwtrig_deactivate(struct led_classdev *cdev)
+{
+ struct omnia_leds *leds = dev_get_drvdata(cdev->dev->parent);
+ struct omnia_led *led = to_omnia_led(lcdev_to_mccdev(cdev));
+ int ret;
+
+ /* put the LED into software mode */
+ ret = i2c_smbus_write_byte_data(leds->client, CMD_LED_MODE,
+ CMD_LED_MODE_LED(led->reg) |
+ CMD_LED_MODE_USER);
+ if (ret < 0)
+ dev_err(cdev->dev, "Cannot put to software mode: %i\n", ret);
+}
+
+static struct led_trigger omnia_hw_trigger = {
+ .name = "omnia-mcu",
+ .activate = omnia_hwtrig_activate,
+ .deactivate = omnia_hwtrig_deactivate,
+ .trigger_type = &omnia_hw_trigger_type,
+};
+
static int omnia_led_brightness_set_blocking(struct led_classdev *cdev,
enum led_brightness brightness)
{
@@ -112,6 +145,8 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
cdev = &led->mc_cdev.led_cdev;
cdev->max_brightness = 255;
cdev->brightness_set_blocking = omnia_led_brightness_set_blocking;
+ cdev->trigger_type = &omnia_hw_trigger_type;
+ cdev->default_trigger = omnia_hw_trigger.name;
/* put the LED into software mode */
ret = i2c_smbus_write_byte_data(client, CMD_LED_MODE,
@@ -228,6 +263,12 @@ static int omnia_leds_probe(struct i2c_client *client,
mutex_init(&leds->lock);
+ ret = devm_led_trigger_register(dev, &omnia_hw_trigger);
+ if (ret < 0) {
+ dev_err(dev, "Cannot register private LED trigger: %d\n", ret);
+ return ret;
+ }
+
led = &leds->leds[0];
for_each_available_child_of_node(np, child) {
ret = omnia_led_register(client, led, child);
--
2.20.1
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH RESEND 7/8] leds: turris-omnia: initialize multi-intensity to full
2022-12-26 12:36 [PATCH RESEND 0/8] Resend LED patches Pali Rohár
` (5 preceding siblings ...)
2022-12-26 12:36 ` [PATCH RESEND 6/8] leds: turris-omnia: support HW controlled mode via private trigger Pali Rohár
@ 2022-12-26 12:36 ` Pali Rohár
2023-02-24 9:33 ` Lee Jones
2022-12-26 12:36 ` [PATCH RESEND 8/8] leds: turris-omnia: change max brightness from 255 to 1 Pali Rohár
2023-01-20 16:41 ` Arnd Bergmann
8 siblings, 1 reply; 42+ messages in thread
From: Pali Rohár @ 2022-12-26 12:36 UTC (permalink / raw)
To: Arnd Bergmann, Linus Walleij; +Cc: soc, linux-kernel
From: Marek Behún <kabel@kernel.org>
The default color of each LED before driver probe (255, 255, 255).
Initialize multi_intensity to this value, so that it corresponds to the
reality.
Signed-off-by: Marek Behún <kabel@kernel.org>
Fixes: 089381b27abe ("leds: initial support for Turris Omnia LEDs")
Reviewed-by: Pali Rohár <pali@kernel.org>
---
drivers/leds/leds-turris-omnia.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c
index bb7af9e59ad6..71340dec492a 100644
--- a/drivers/leds/leds-turris-omnia.c
+++ b/drivers/leds/leds-turris-omnia.c
@@ -131,10 +131,13 @@ 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].intensity = 255;
led->subled_info[0].channel = 0;
led->subled_info[1].color_index = LED_COLOR_ID_GREEN;
+ led->subled_info[1].intensity = 255;
led->subled_info[1].channel = 1;
led->subled_info[2].color_index = LED_COLOR_ID_BLUE;
+ led->subled_info[2].intensity = 255;
led->subled_info[2].channel = 2;
led->mc_cdev.subled_info = led->subled_info;
--
2.20.1
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH RESEND 8/8] leds: turris-omnia: change max brightness from 255 to 1
2022-12-26 12:36 [PATCH RESEND 0/8] Resend LED patches Pali Rohár
` (6 preceding siblings ...)
2022-12-26 12:36 ` [PATCH RESEND 7/8] leds: turris-omnia: initialize multi-intensity to full Pali Rohár
@ 2022-12-26 12:36 ` Pali Rohár
2023-02-24 9:34 ` Lee Jones
2023-01-20 16:41 ` Arnd Bergmann
8 siblings, 1 reply; 42+ messages in thread
From: Pali Rohár @ 2022-12-26 12:36 UTC (permalink / raw)
To: Arnd Bergmann, Linus Walleij; +Cc: soc, linux-kernel
From: Marek Behún <kabel@kernel.org>
Using binary brightness makes more sense for this controller, because
internally in the MCU it works that way: the LED has a color, and a
state whether it is ON or OFF.
The resulting brightness computation with led_mc_calc_color_components()
will now always result in either (0, 0, 0) or the multi_intensity value.
Signed-off-by: Marek Behún <kabel@kernel.org>
Fixes: 089381b27abe ("leds: initial support for Turris Omnia LEDs")
Reviewed-by: Pali Rohár <pali@kernel.org>
---
drivers/leds/leds-turris-omnia.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c
index 71340dec492a..04f01ae46c27 100644
--- a/drivers/leds/leds-turris-omnia.c
+++ b/drivers/leds/leds-turris-omnia.c
@@ -146,7 +146,7 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
init_data.fwnode = &np->fwnode;
cdev = &led->mc_cdev.led_cdev;
- cdev->max_brightness = 255;
+ cdev->max_brightness = 1;
cdev->brightness_set_blocking = omnia_led_brightness_set_blocking;
cdev->trigger_type = &omnia_hw_trigger_type;
cdev->default_trigger = omnia_hw_trigger.name;
--
2.20.1
^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH RESEND 0/8] Resend LED patches
2022-12-26 12:36 [PATCH RESEND 0/8] Resend LED patches Pali Rohár
@ 2023-01-20 16:41 ` Arnd Bergmann
2022-12-26 12:36 ` [PATCH RESEND 2/8] leds: syscon: Implement support for " Pali Rohár
` (7 subsequent siblings)
8 siblings, 0 replies; 42+ messages in thread
From: Arnd Bergmann @ 2023-01-20 16:41 UTC (permalink / raw)
To: Pali Rohár, Linus Walleij
Cc: soc, linux-kernel, Pavel Machek, Lee Jones, linux-leds,
Michael Ellerman, Nicholas Piggin, Christophe Leroy,
linuxppc-dev
On Mon, Dec 26, 2022, at 13:36, Pali Rohár wrote:
> Linus Walleij suggested me to send these patches to SoC tree [1]
> instead. So I'm doing it.
>
> This patch series contains LED patches which are on the linux-leds
> mailing list for a long time without any future movement. Could you
> please handle them here via SoC tree? Thanks.
>
> [1] -
> https://lore.kernel.org/linux-leds/CACRpkdad6WDo7rGfa4MW8zz0mLXmcPHo+SEC-yLQnRz_kdrryA@mail.gmail.com/
I'm going through the backlog of patches sent to soc@kernel.org
and came across this series. While I don't mind taking these
patches through the soc tree in principle, it is important
that this is only done as an exception, and with all the
relevant parties on Cc.
In particular, the original series that you got no
feedback for did not include the arch/powerpc/ changes,
and I would assume those should go through the powerpc
tree anyway. We have recently decided to take
risc-v and loongarch dts changes through the soc
tree, and I don't mind doing it for powerpc as well
if the powerpc maintainers prefer that, but this is
not something we have even discussed so far.
I've added everyone to Cc on this mail, but please
resend the series once more so everyone has the patches,
and then we can decide who will pick up what.
Arnd
>
> Marek Behún (3):
> leds: turris-omnia: support HW controlled mode via private trigger
> leds: turris-omnia: initialize multi-intensity to full
> leds: turris-omnia: change max brightness from 255 to 1
>
> Pali Rohár (5):
> dt-bindings: leds: register-bit-led: Add active-low property
> leds: syscon: Implement support for active-low property
> powerpc/85xx: DTS: Add CPLD definitions for P1021RDB Combo Board CPL
> Design
> dt-bindings: leds: Add cznic,turris1x-leds.yaml binding
> leds: Add support for Turris 1.x LEDs
>
> .../testing/sysfs-class-led-driver-turris1x | 31 ++
> .../bindings/leds/cznic,turris1x-leds.yaml | 118 +++++
> .../bindings/leds/register-bit-led.yaml | 5 +
> arch/powerpc/boot/dts/fsl/p1020mbg-pc.dtsi | 92 ++++
> arch/powerpc/boot/dts/fsl/p1020mbg-pc_32b.dts | 6 +-
> arch/powerpc/boot/dts/fsl/p1020mbg-pc_36b.dts | 6 +-
> arch/powerpc/boot/dts/fsl/p1020rdb-pd.dts | 44 +-
> arch/powerpc/boot/dts/fsl/p1020utm-pc.dtsi | 37 ++
> arch/powerpc/boot/dts/fsl/p1020utm-pc_32b.dts | 4 +-
> arch/powerpc/boot/dts/fsl/p1020utm-pc_36b.dts | 4 +-
> arch/powerpc/boot/dts/fsl/p1021rdb-pc.dtsi | 37 ++
> arch/powerpc/boot/dts/fsl/p1021rdb-pc_32b.dts | 5 +-
> arch/powerpc/boot/dts/fsl/p1021rdb-pc_36b.dts | 5 +-
> arch/powerpc/boot/dts/fsl/p2020rdb-pc.dtsi | 33 +-
> drivers/leds/Kconfig | 10 +
> drivers/leds/Makefile | 1 +
> drivers/leds/leds-syscon.c | 14 +-
> drivers/leds/leds-turris-1x.c | 474 ++++++++++++++++++
> drivers/leds/leds-turris-omnia.c | 46 +-
> 19 files changed, 945 insertions(+), 27 deletions(-)
> create mode 100644
> Documentation/ABI/testing/sysfs-class-led-driver-turris1x
> create mode 100644
> Documentation/devicetree/bindings/leds/cznic,turris1x-leds.yaml
> create mode 100644 drivers/leds/leds-turris-1x.c
>
> --
> 2.20.1
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH RESEND 0/8] Resend LED patches
@ 2023-01-20 16:41 ` Arnd Bergmann
0 siblings, 0 replies; 42+ messages in thread
From: Arnd Bergmann @ 2023-01-20 16:41 UTC (permalink / raw)
To: Pali Rohár, Linus Walleij
Cc: Lee Jones, linux-kernel, Nicholas Piggin, soc, Pavel Machek,
linuxppc-dev, linux-leds
On Mon, Dec 26, 2022, at 13:36, Pali Rohár wrote:
> Linus Walleij suggested me to send these patches to SoC tree [1]
> instead. So I'm doing it.
>
> This patch series contains LED patches which are on the linux-leds
> mailing list for a long time without any future movement. Could you
> please handle them here via SoC tree? Thanks.
>
> [1] -
> https://lore.kernel.org/linux-leds/CACRpkdad6WDo7rGfa4MW8zz0mLXmcPHo+SEC-yLQnRz_kdrryA@mail.gmail.com/
I'm going through the backlog of patches sent to soc@kernel.org
and came across this series. While I don't mind taking these
patches through the soc tree in principle, it is important
that this is only done as an exception, and with all the
relevant parties on Cc.
In particular, the original series that you got no
feedback for did not include the arch/powerpc/ changes,
and I would assume those should go through the powerpc
tree anyway. We have recently decided to take
risc-v and loongarch dts changes through the soc
tree, and I don't mind doing it for powerpc as well
if the powerpc maintainers prefer that, but this is
not something we have even discussed so far.
I've added everyone to Cc on this mail, but please
resend the series once more so everyone has the patches,
and then we can decide who will pick up what.
Arnd
>
> Marek Behún (3):
> leds: turris-omnia: support HW controlled mode via private trigger
> leds: turris-omnia: initialize multi-intensity to full
> leds: turris-omnia: change max brightness from 255 to 1
>
> Pali Rohár (5):
> dt-bindings: leds: register-bit-led: Add active-low property
> leds: syscon: Implement support for active-low property
> powerpc/85xx: DTS: Add CPLD definitions for P1021RDB Combo Board CPL
> Design
> dt-bindings: leds: Add cznic,turris1x-leds.yaml binding
> leds: Add support for Turris 1.x LEDs
>
> .../testing/sysfs-class-led-driver-turris1x | 31 ++
> .../bindings/leds/cznic,turris1x-leds.yaml | 118 +++++
> .../bindings/leds/register-bit-led.yaml | 5 +
> arch/powerpc/boot/dts/fsl/p1020mbg-pc.dtsi | 92 ++++
> arch/powerpc/boot/dts/fsl/p1020mbg-pc_32b.dts | 6 +-
> arch/powerpc/boot/dts/fsl/p1020mbg-pc_36b.dts | 6 +-
> arch/powerpc/boot/dts/fsl/p1020rdb-pd.dts | 44 +-
> arch/powerpc/boot/dts/fsl/p1020utm-pc.dtsi | 37 ++
> arch/powerpc/boot/dts/fsl/p1020utm-pc_32b.dts | 4 +-
> arch/powerpc/boot/dts/fsl/p1020utm-pc_36b.dts | 4 +-
> arch/powerpc/boot/dts/fsl/p1021rdb-pc.dtsi | 37 ++
> arch/powerpc/boot/dts/fsl/p1021rdb-pc_32b.dts | 5 +-
> arch/powerpc/boot/dts/fsl/p1021rdb-pc_36b.dts | 5 +-
> arch/powerpc/boot/dts/fsl/p2020rdb-pc.dtsi | 33 +-
> drivers/leds/Kconfig | 10 +
> drivers/leds/Makefile | 1 +
> drivers/leds/leds-syscon.c | 14 +-
> drivers/leds/leds-turris-1x.c | 474 ++++++++++++++++++
> drivers/leds/leds-turris-omnia.c | 46 +-
> 19 files changed, 945 insertions(+), 27 deletions(-)
> create mode 100644
> Documentation/ABI/testing/sysfs-class-led-driver-turris1x
> create mode 100644
> Documentation/devicetree/bindings/leds/cznic,turris1x-leds.yaml
> create mode 100644 drivers/leds/leds-turris-1x.c
>
> --
> 2.20.1
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH RESEND 0/8] Resend LED patches
2023-01-20 16:41 ` Arnd Bergmann
@ 2023-01-20 17:15 ` Lee Jones
-1 siblings, 0 replies; 42+ messages in thread
From: Lee Jones @ 2023-01-20 17:15 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Pali Rohár, Linus Walleij, soc, linux-kernel, Pavel Machek,
linux-leds, Michael Ellerman, Nicholas Piggin, Christophe Leroy,
linuxppc-dev
On Fri, 20 Jan 2023, Arnd Bergmann wrote:
> On Mon, Dec 26, 2022, at 13:36, Pali Rohár wrote:
> > Linus Walleij suggested me to send these patches to SoC tree [1]
> > instead. So I'm doing it.
> >
> > This patch series contains LED patches which are on the linux-leds
> > mailing list for a long time without any future movement. Could you
> > please handle them here via SoC tree? Thanks.
> >
> > [1] -
> > https://lore.kernel.org/linux-leds/CACRpkdad6WDo7rGfa4MW8zz0mLXmcPHo+SEC-yLQnRz_kdrryA@mail.gmail.com/
>
> I'm going through the backlog of patches sent to soc@kernel.org
> and came across this series. While I don't mind taking these
> patches through the soc tree in principle, it is important
> that this is only done as an exception, and with all the
> relevant parties on Cc.
>
> In particular, the original series that you got no
> feedback for did not include the arch/powerpc/ changes,
> and I would assume those should go through the powerpc
> tree anyway. We have recently decided to take
> risc-v and loongarch dts changes through the soc
> tree, and I don't mind doing it for powerpc as well
> if the powerpc maintainers prefer that, but this is
> not something we have even discussed so far.
>
> I've added everyone to Cc on this mail, but please
> resend the series once more so everyone has the patches,
> and then we can decide who will pick up what.
Thanks Arnd (PSB).
> > Marek Behún (3):
> > leds: turris-omnia: support HW controlled mode via private trigger
> > leds: turris-omnia: initialize multi-intensity to full
> > leds: turris-omnia: change max brightness from 255 to 1
> >
> > Pali Rohár (5):
> > dt-bindings: leds: register-bit-led: Add active-low property
> > leds: syscon: Implement support for active-low property
> > powerpc/85xx: DTS: Add CPLD definitions for P1021RDB Combo Board CPL
> > Design
> > dt-bindings: leds: Add cznic,turris1x-leds.yaml binding
> > leds: Add support for Turris 1.x LEDs
> >
> > .../testing/sysfs-class-led-driver-turris1x | 31 ++
> > .../bindings/leds/cznic,turris1x-leds.yaml | 118 +++++
> > .../bindings/leds/register-bit-led.yaml | 5 +
> > arch/powerpc/boot/dts/fsl/p1020mbg-pc.dtsi | 92 ++++
> > arch/powerpc/boot/dts/fsl/p1020mbg-pc_32b.dts | 6 +-
> > arch/powerpc/boot/dts/fsl/p1020mbg-pc_36b.dts | 6 +-
> > arch/powerpc/boot/dts/fsl/p1020rdb-pd.dts | 44 +-
> > arch/powerpc/boot/dts/fsl/p1020utm-pc.dtsi | 37 ++
> > arch/powerpc/boot/dts/fsl/p1020utm-pc_32b.dts | 4 +-
> > arch/powerpc/boot/dts/fsl/p1020utm-pc_36b.dts | 4 +-
> > arch/powerpc/boot/dts/fsl/p1021rdb-pc.dtsi | 37 ++
> > arch/powerpc/boot/dts/fsl/p1021rdb-pc_32b.dts | 5 +-
> > arch/powerpc/boot/dts/fsl/p1021rdb-pc_36b.dts | 5 +-
> > arch/powerpc/boot/dts/fsl/p2020rdb-pc.dtsi | 33 +-
> > drivers/leds/Kconfig | 10 +
> > drivers/leds/Makefile | 1 +
> > drivers/leds/leds-syscon.c | 14 +-
> > drivers/leds/leds-turris-1x.c | 474 ++++++++++++++++++
> > drivers/leds/leds-turris-omnia.c | 46 +-
If everyone is convinced that applying these drivers is the correct
thing to do, I'd be happy to (rather) take them via LEDs.
--
Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH RESEND 0/8] Resend LED patches
@ 2023-01-20 17:15 ` Lee Jones
0 siblings, 0 replies; 42+ messages in thread
From: Lee Jones @ 2023-01-20 17:15 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Linus Walleij, linuxppc-dev, linux-kernel, Nicholas Piggin, soc,
Pavel Machek, Pali Rohár, linux-leds
On Fri, 20 Jan 2023, Arnd Bergmann wrote:
> On Mon, Dec 26, 2022, at 13:36, Pali Rohár wrote:
> > Linus Walleij suggested me to send these patches to SoC tree [1]
> > instead. So I'm doing it.
> >
> > This patch series contains LED patches which are on the linux-leds
> > mailing list for a long time without any future movement. Could you
> > please handle them here via SoC tree? Thanks.
> >
> > [1] -
> > https://lore.kernel.org/linux-leds/CACRpkdad6WDo7rGfa4MW8zz0mLXmcPHo+SEC-yLQnRz_kdrryA@mail.gmail.com/
>
> I'm going through the backlog of patches sent to soc@kernel.org
> and came across this series. While I don't mind taking these
> patches through the soc tree in principle, it is important
> that this is only done as an exception, and with all the
> relevant parties on Cc.
>
> In particular, the original series that you got no
> feedback for did not include the arch/powerpc/ changes,
> and I would assume those should go through the powerpc
> tree anyway. We have recently decided to take
> risc-v and loongarch dts changes through the soc
> tree, and I don't mind doing it for powerpc as well
> if the powerpc maintainers prefer that, but this is
> not something we have even discussed so far.
>
> I've added everyone to Cc on this mail, but please
> resend the series once more so everyone has the patches,
> and then we can decide who will pick up what.
Thanks Arnd (PSB).
> > Marek Behún (3):
> > leds: turris-omnia: support HW controlled mode via private trigger
> > leds: turris-omnia: initialize multi-intensity to full
> > leds: turris-omnia: change max brightness from 255 to 1
> >
> > Pali Rohár (5):
> > dt-bindings: leds: register-bit-led: Add active-low property
> > leds: syscon: Implement support for active-low property
> > powerpc/85xx: DTS: Add CPLD definitions for P1021RDB Combo Board CPL
> > Design
> > dt-bindings: leds: Add cznic,turris1x-leds.yaml binding
> > leds: Add support for Turris 1.x LEDs
> >
> > .../testing/sysfs-class-led-driver-turris1x | 31 ++
> > .../bindings/leds/cznic,turris1x-leds.yaml | 118 +++++
> > .../bindings/leds/register-bit-led.yaml | 5 +
> > arch/powerpc/boot/dts/fsl/p1020mbg-pc.dtsi | 92 ++++
> > arch/powerpc/boot/dts/fsl/p1020mbg-pc_32b.dts | 6 +-
> > arch/powerpc/boot/dts/fsl/p1020mbg-pc_36b.dts | 6 +-
> > arch/powerpc/boot/dts/fsl/p1020rdb-pd.dts | 44 +-
> > arch/powerpc/boot/dts/fsl/p1020utm-pc.dtsi | 37 ++
> > arch/powerpc/boot/dts/fsl/p1020utm-pc_32b.dts | 4 +-
> > arch/powerpc/boot/dts/fsl/p1020utm-pc_36b.dts | 4 +-
> > arch/powerpc/boot/dts/fsl/p1021rdb-pc.dtsi | 37 ++
> > arch/powerpc/boot/dts/fsl/p1021rdb-pc_32b.dts | 5 +-
> > arch/powerpc/boot/dts/fsl/p1021rdb-pc_36b.dts | 5 +-
> > arch/powerpc/boot/dts/fsl/p2020rdb-pc.dtsi | 33 +-
> > drivers/leds/Kconfig | 10 +
> > drivers/leds/Makefile | 1 +
> > drivers/leds/leds-syscon.c | 14 +-
> > drivers/leds/leds-turris-1x.c | 474 ++++++++++++++++++
> > drivers/leds/leds-turris-omnia.c | 46 +-
If everyone is convinced that applying these drivers is the correct
thing to do, I'd be happy to (rather) take them via LEDs.
--
Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH RESEND 0/8] Resend LED patches
2023-01-20 17:15 ` Lee Jones
@ 2023-01-20 17:47 ` Arnd Bergmann
-1 siblings, 0 replies; 42+ messages in thread
From: Arnd Bergmann @ 2023-01-20 17:47 UTC (permalink / raw)
To: Lee Jones
Cc: Pali Rohár, Linus Walleij, soc, linux-kernel, Pavel Machek,
linux-leds, Michael Ellerman, Nicholas Piggin, Christophe Leroy,
linuxppc-dev
On Fri, Jan 20, 2023, at 18:15, Lee Jones wrote:
> On Fri, 20 Jan 2023, Arnd Bergmann wrote:
>> > Marek Behún (3):
>> > leds: turris-omnia: support HW controlled mode via private trigger
>> > leds: turris-omnia: initialize multi-intensity to full
>> > leds: turris-omnia: change max brightness from 255 to 1
>> >
>> > Pali Rohár (5):
>> > dt-bindings: leds: register-bit-led: Add active-low property
>> > leds: syscon: Implement support for active-low property
>> > powerpc/85xx: DTS: Add CPLD definitions for P1021RDB Combo Board CPL
>> > Design
>> > dt-bindings: leds: Add cznic,turris1x-leds.yaml binding
>> > leds: Add support for Turris 1.x LEDs
>> >
>> > .../testing/sysfs-class-led-driver-turris1x | 31 ++
>> > .../bindings/leds/cznic,turris1x-leds.yaml | 118 +++++
>> > .../bindings/leds/register-bit-led.yaml | 5 +
>> > arch/powerpc/boot/dts/fsl/p1020mbg-pc.dtsi | 92 ++++
>> > arch/powerpc/boot/dts/fsl/p1020mbg-pc_32b.dts | 6 +-
>> > arch/powerpc/boot/dts/fsl/p1020mbg-pc_36b.dts | 6 +-
>> > arch/powerpc/boot/dts/fsl/p1020rdb-pd.dts | 44 +-
>> > arch/powerpc/boot/dts/fsl/p1020utm-pc.dtsi | 37 ++
>> > arch/powerpc/boot/dts/fsl/p1020utm-pc_32b.dts | 4 +-
>> > arch/powerpc/boot/dts/fsl/p1020utm-pc_36b.dts | 4 +-
>> > arch/powerpc/boot/dts/fsl/p1021rdb-pc.dtsi | 37 ++
>> > arch/powerpc/boot/dts/fsl/p1021rdb-pc_32b.dts | 5 +-
>> > arch/powerpc/boot/dts/fsl/p1021rdb-pc_36b.dts | 5 +-
>> > arch/powerpc/boot/dts/fsl/p2020rdb-pc.dtsi | 33 +-
>
>> > drivers/leds/Kconfig | 10 +
>> > drivers/leds/Makefile | 1 +
>> > drivers/leds/leds-syscon.c | 14 +-
>> > drivers/leds/leds-turris-1x.c | 474 ++++++++++++++++++
>> > drivers/leds/leds-turris-omnia.c | 46 +-
>
> If everyone is convinced that applying these drivers is the correct
> thing to do, I'd be happy to (rather) take them via LEDs.
Ok, thanks. I had not actually looked at the patches until today.
They were in the soc tree backlog but appeared to be misplaced
there until I read the 0/10 message text.
Looking at it now, I see:
- patches 1 and 2 seem obvious and have been reviewed by
others already
- patch 3 is for arch/powerpc and should get merged through
there if there are no objections to the binding in patch 4.
- patch 5 is the big driver patch, with a Reviewed-by tag
from Marek Behún, who is the author of the last three patches.
An earlier version of this patch was sent in June and got
a few Acks and detailed feedback from Andy [1], but he's also
not on Cc, and I don't know if his comments are all resolved
in this version.
- Patches 6, 7 and 8 all seem simple LED subsystem patches,
they just need review from you in order to get applied.
These are also missing a Signed-off-by from the submitter
in addition to the author in order to be applied.
Arnd
[1] https://lore.kernel.org/all/CAHp75Vcr6o2rm+T6Tr8sS4VXCLVHtmLPWy-njOKAvO4AcZoW=A@mail.gmail.com/
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH RESEND 0/8] Resend LED patches
@ 2023-01-20 17:47 ` Arnd Bergmann
0 siblings, 0 replies; 42+ messages in thread
From: Arnd Bergmann @ 2023-01-20 17:47 UTC (permalink / raw)
To: Lee Jones
Cc: Linus Walleij, linuxppc-dev, linux-kernel, Nicholas Piggin, soc,
Pavel Machek, Pali Rohár, linux-leds
On Fri, Jan 20, 2023, at 18:15, Lee Jones wrote:
> On Fri, 20 Jan 2023, Arnd Bergmann wrote:
>> > Marek Behún (3):
>> > leds: turris-omnia: support HW controlled mode via private trigger
>> > leds: turris-omnia: initialize multi-intensity to full
>> > leds: turris-omnia: change max brightness from 255 to 1
>> >
>> > Pali Rohár (5):
>> > dt-bindings: leds: register-bit-led: Add active-low property
>> > leds: syscon: Implement support for active-low property
>> > powerpc/85xx: DTS: Add CPLD definitions for P1021RDB Combo Board CPL
>> > Design
>> > dt-bindings: leds: Add cznic,turris1x-leds.yaml binding
>> > leds: Add support for Turris 1.x LEDs
>> >
>> > .../testing/sysfs-class-led-driver-turris1x | 31 ++
>> > .../bindings/leds/cznic,turris1x-leds.yaml | 118 +++++
>> > .../bindings/leds/register-bit-led.yaml | 5 +
>> > arch/powerpc/boot/dts/fsl/p1020mbg-pc.dtsi | 92 ++++
>> > arch/powerpc/boot/dts/fsl/p1020mbg-pc_32b.dts | 6 +-
>> > arch/powerpc/boot/dts/fsl/p1020mbg-pc_36b.dts | 6 +-
>> > arch/powerpc/boot/dts/fsl/p1020rdb-pd.dts | 44 +-
>> > arch/powerpc/boot/dts/fsl/p1020utm-pc.dtsi | 37 ++
>> > arch/powerpc/boot/dts/fsl/p1020utm-pc_32b.dts | 4 +-
>> > arch/powerpc/boot/dts/fsl/p1020utm-pc_36b.dts | 4 +-
>> > arch/powerpc/boot/dts/fsl/p1021rdb-pc.dtsi | 37 ++
>> > arch/powerpc/boot/dts/fsl/p1021rdb-pc_32b.dts | 5 +-
>> > arch/powerpc/boot/dts/fsl/p1021rdb-pc_36b.dts | 5 +-
>> > arch/powerpc/boot/dts/fsl/p2020rdb-pc.dtsi | 33 +-
>
>> > drivers/leds/Kconfig | 10 +
>> > drivers/leds/Makefile | 1 +
>> > drivers/leds/leds-syscon.c | 14 +-
>> > drivers/leds/leds-turris-1x.c | 474 ++++++++++++++++++
>> > drivers/leds/leds-turris-omnia.c | 46 +-
>
> If everyone is convinced that applying these drivers is the correct
> thing to do, I'd be happy to (rather) take them via LEDs.
Ok, thanks. I had not actually looked at the patches until today.
They were in the soc tree backlog but appeared to be misplaced
there until I read the 0/10 message text.
Looking at it now, I see:
- patches 1 and 2 seem obvious and have been reviewed by
others already
- patch 3 is for arch/powerpc and should get merged through
there if there are no objections to the binding in patch 4.
- patch 5 is the big driver patch, with a Reviewed-by tag
from Marek Behún, who is the author of the last three patches.
An earlier version of this patch was sent in June and got
a few Acks and detailed feedback from Andy [1], but he's also
not on Cc, and I don't know if his comments are all resolved
in this version.
- Patches 6, 7 and 8 all seem simple LED subsystem patches,
they just need review from you in order to get applied.
These are also missing a Signed-off-by from the submitter
in addition to the author in order to be applied.
Arnd
[1] https://lore.kernel.org/all/CAHp75Vcr6o2rm+T6Tr8sS4VXCLVHtmLPWy-njOKAvO4AcZoW=A@mail.gmail.com/
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH RESEND 0/8] Resend LED patches
2023-01-20 17:47 ` Arnd Bergmann
@ 2023-01-20 20:02 ` Lee Jones
-1 siblings, 0 replies; 42+ messages in thread
From: Lee Jones @ 2023-01-20 20:02 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Pali Rohár, Linus Walleij, soc, linux-kernel, Pavel Machek,
linux-leds, Michael Ellerman, Nicholas Piggin, Christophe Leroy,
linuxppc-dev
On Fri, 20 Jan 2023, Arnd Bergmann wrote:
> On Fri, Jan 20, 2023, at 18:15, Lee Jones wrote:
> > On Fri, 20 Jan 2023, Arnd Bergmann wrote:
>
> >> > Marek Behún (3):
> >> > leds: turris-omnia: support HW controlled mode via private trigger
> >> > leds: turris-omnia: initialize multi-intensity to full
> >> > leds: turris-omnia: change max brightness from 255 to 1
> >> >
> >> > Pali Rohár (5):
> >> > dt-bindings: leds: register-bit-led: Add active-low property
> >> > leds: syscon: Implement support for active-low property
> >> > powerpc/85xx: DTS: Add CPLD definitions for P1021RDB Combo Board CPL
> >> > Design
> >> > dt-bindings: leds: Add cznic,turris1x-leds.yaml binding
> >> > leds: Add support for Turris 1.x LEDs
> >> >
> >> > .../testing/sysfs-class-led-driver-turris1x | 31 ++
> >> > .../bindings/leds/cznic,turris1x-leds.yaml | 118 +++++
> >> > .../bindings/leds/register-bit-led.yaml | 5 +
> >> > arch/powerpc/boot/dts/fsl/p1020mbg-pc.dtsi | 92 ++++
> >> > arch/powerpc/boot/dts/fsl/p1020mbg-pc_32b.dts | 6 +-
> >> > arch/powerpc/boot/dts/fsl/p1020mbg-pc_36b.dts | 6 +-
> >> > arch/powerpc/boot/dts/fsl/p1020rdb-pd.dts | 44 +-
> >> > arch/powerpc/boot/dts/fsl/p1020utm-pc.dtsi | 37 ++
> >> > arch/powerpc/boot/dts/fsl/p1020utm-pc_32b.dts | 4 +-
> >> > arch/powerpc/boot/dts/fsl/p1020utm-pc_36b.dts | 4 +-
> >> > arch/powerpc/boot/dts/fsl/p1021rdb-pc.dtsi | 37 ++
> >> > arch/powerpc/boot/dts/fsl/p1021rdb-pc_32b.dts | 5 +-
> >> > arch/powerpc/boot/dts/fsl/p1021rdb-pc_36b.dts | 5 +-
> >> > arch/powerpc/boot/dts/fsl/p2020rdb-pc.dtsi | 33 +-
> >
> >> > drivers/leds/Kconfig | 10 +
> >> > drivers/leds/Makefile | 1 +
> >> > drivers/leds/leds-syscon.c | 14 +-
> >> > drivers/leds/leds-turris-1x.c | 474 ++++++++++++++++++
> >> > drivers/leds/leds-turris-omnia.c | 46 +-
> >
> > If everyone is convinced that applying these drivers is the correct
> > thing to do, I'd be happy to (rather) take them via LEDs.
>
> Ok, thanks. I had not actually looked at the patches until today.
> They were in the soc tree backlog but appeared to be misplaced
> there until I read the 0/10 message text.
>
> Looking at it now, I see:
>
> - patches 1 and 2 seem obvious and have been reviewed by
> others already
>
> - patch 3 is for arch/powerpc and should get merged through
> there if there are no objections to the binding in patch 4.
>
> - patch 5 is the big driver patch, with a Reviewed-by tag
> from Marek Behún, who is the author of the last three patches.
> An earlier version of this patch was sent in June and got
> a few Acks and detailed feedback from Andy [1], but he's also
> not on Cc, and I don't know if his comments are all resolved
> in this version.
>
> - Patches 6, 7 and 8 all seem simple LED subsystem patches,
> they just need review from you in order to get applied.
> These are also missing a Signed-off-by from the submitter
> in addition to the author in order to be applied.
Very well. Let's have them resent then please (with past reviewers on
Cc:) and we'll go from there.
--
Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH RESEND 0/8] Resend LED patches
@ 2023-01-20 20:02 ` Lee Jones
0 siblings, 0 replies; 42+ messages in thread
From: Lee Jones @ 2023-01-20 20:02 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Linus Walleij, linuxppc-dev, linux-kernel, Nicholas Piggin, soc,
Pavel Machek, Pali Rohár, linux-leds
On Fri, 20 Jan 2023, Arnd Bergmann wrote:
> On Fri, Jan 20, 2023, at 18:15, Lee Jones wrote:
> > On Fri, 20 Jan 2023, Arnd Bergmann wrote:
>
> >> > Marek Behún (3):
> >> > leds: turris-omnia: support HW controlled mode via private trigger
> >> > leds: turris-omnia: initialize multi-intensity to full
> >> > leds: turris-omnia: change max brightness from 255 to 1
> >> >
> >> > Pali Rohár (5):
> >> > dt-bindings: leds: register-bit-led: Add active-low property
> >> > leds: syscon: Implement support for active-low property
> >> > powerpc/85xx: DTS: Add CPLD definitions for P1021RDB Combo Board CPL
> >> > Design
> >> > dt-bindings: leds: Add cznic,turris1x-leds.yaml binding
> >> > leds: Add support for Turris 1.x LEDs
> >> >
> >> > .../testing/sysfs-class-led-driver-turris1x | 31 ++
> >> > .../bindings/leds/cznic,turris1x-leds.yaml | 118 +++++
> >> > .../bindings/leds/register-bit-led.yaml | 5 +
> >> > arch/powerpc/boot/dts/fsl/p1020mbg-pc.dtsi | 92 ++++
> >> > arch/powerpc/boot/dts/fsl/p1020mbg-pc_32b.dts | 6 +-
> >> > arch/powerpc/boot/dts/fsl/p1020mbg-pc_36b.dts | 6 +-
> >> > arch/powerpc/boot/dts/fsl/p1020rdb-pd.dts | 44 +-
> >> > arch/powerpc/boot/dts/fsl/p1020utm-pc.dtsi | 37 ++
> >> > arch/powerpc/boot/dts/fsl/p1020utm-pc_32b.dts | 4 +-
> >> > arch/powerpc/boot/dts/fsl/p1020utm-pc_36b.dts | 4 +-
> >> > arch/powerpc/boot/dts/fsl/p1021rdb-pc.dtsi | 37 ++
> >> > arch/powerpc/boot/dts/fsl/p1021rdb-pc_32b.dts | 5 +-
> >> > arch/powerpc/boot/dts/fsl/p1021rdb-pc_36b.dts | 5 +-
> >> > arch/powerpc/boot/dts/fsl/p2020rdb-pc.dtsi | 33 +-
> >
> >> > drivers/leds/Kconfig | 10 +
> >> > drivers/leds/Makefile | 1 +
> >> > drivers/leds/leds-syscon.c | 14 +-
> >> > drivers/leds/leds-turris-1x.c | 474 ++++++++++++++++++
> >> > drivers/leds/leds-turris-omnia.c | 46 +-
> >
> > If everyone is convinced that applying these drivers is the correct
> > thing to do, I'd be happy to (rather) take them via LEDs.
>
> Ok, thanks. I had not actually looked at the patches until today.
> They were in the soc tree backlog but appeared to be misplaced
> there until I read the 0/10 message text.
>
> Looking at it now, I see:
>
> - patches 1 and 2 seem obvious and have been reviewed by
> others already
>
> - patch 3 is for arch/powerpc and should get merged through
> there if there are no objections to the binding in patch 4.
>
> - patch 5 is the big driver patch, with a Reviewed-by tag
> from Marek Behún, who is the author of the last three patches.
> An earlier version of this patch was sent in June and got
> a few Acks and detailed feedback from Andy [1], but he's also
> not on Cc, and I don't know if his comments are all resolved
> in this version.
>
> - Patches 6, 7 and 8 all seem simple LED subsystem patches,
> they just need review from you in order to get applied.
> These are also missing a Signed-off-by from the submitter
> in addition to the author in order to be applied.
Very well. Let's have them resent then please (with past reviewers on
Cc:) and we'll go from there.
--
Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH RESEND 0/8] Resend LED patches
2023-01-20 17:15 ` Lee Jones
@ 2023-01-26 20:07 ` Linus Walleij
-1 siblings, 0 replies; 42+ messages in thread
From: Linus Walleij @ 2023-01-26 20:07 UTC (permalink / raw)
To: Lee Jones
Cc: Arnd Bergmann, Pali Rohár, soc, linux-kernel, Pavel Machek,
linux-leds, Michael Ellerman, Nicholas Piggin, Christophe Leroy,
linuxppc-dev
On Fri, Jan 20, 2023 at 6:15 PM Lee Jones <lee@kernel.org> wrote:
> If everyone is convinced that applying these drivers is the correct
> thing to do, I'd be happy to (rather) take them via LEDs.
Oh you are co-maintainer of the LED subsystem since a month!
Sadly this series stalled way before that, so that's why we didn't notice.
By all means, pick it up!
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH RESEND 0/8] Resend LED patches
@ 2023-01-26 20:07 ` Linus Walleij
0 siblings, 0 replies; 42+ messages in thread
From: Linus Walleij @ 2023-01-26 20:07 UTC (permalink / raw)
To: Lee Jones
Cc: Arnd Bergmann, linuxppc-dev, linux-kernel, Nicholas Piggin, soc,
Pavel Machek, Pali Rohár, linux-leds
On Fri, Jan 20, 2023 at 6:15 PM Lee Jones <lee@kernel.org> wrote:
> If everyone is convinced that applying these drivers is the correct
> thing to do, I'd be happy to (rather) take them via LEDs.
Oh you are co-maintainer of the LED subsystem since a month!
Sadly this series stalled way before that, so that's why we didn't notice.
By all means, pick it up!
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH RESEND 1/8] dt-bindings: leds: register-bit-led: Add active-low property
2022-12-26 12:36 ` [PATCH RESEND 1/8] dt-bindings: leds: register-bit-led: Add active-low property Pali Rohár
@ 2023-01-27 11:16 ` Lee Jones
2023-02-23 14:22 ` Lee Jones
0 siblings, 1 reply; 42+ messages in thread
From: Lee Jones @ 2023-01-27 11:16 UTC (permalink / raw)
To: Pali Rohár
Cc: Arnd Bergmann, Linus Walleij, soc, linux-kernel, devicetree
On Mon, 26 Dec 2022, Pali Rohár wrote:
> Allow to define inverted logic (0 - enable LED, 1 - disable LED) via
> active-low property.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Documentation/devicetree/bindings/leds/register-bit-led.yaml | 5 +++++
> 1 file changed, 5 insertions(+)
Needs a DT Ack (now Cc:ed)
> diff --git a/Documentation/devicetree/bindings/leds/register-bit-led.yaml b/Documentation/devicetree/bindings/leds/register-bit-led.yaml
> index ed26ec19ecbd..418130b29caa 100644
> --- a/Documentation/devicetree/bindings/leds/register-bit-led.yaml
> +++ b/Documentation/devicetree/bindings/leds/register-bit-led.yaml
> @@ -43,6 +43,11 @@ properties:
> 0x100000, 0x200000, 0x400000, 0x800000, 0x1000000, 0x2000000, 0x4000000,
> 0x8000000, 0x10000000, 0x20000000, 0x40000000, 0x80000000 ]
>
> + active-low:
> + $ref: /schemas/types.yaml#/definitions/flag
> + description:
> + LED is ON when bit in register is not set
> +
> offset:
> description:
> register offset to the register controlling this LED
> --
> 2.20.1
>
--
Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH RESEND 4/8] dt-bindings: leds: Add cznic,turris1x-leds.yaml binding
2022-12-26 12:36 ` [PATCH RESEND 4/8] dt-bindings: leds: Add cznic,turris1x-leds.yaml binding Pali Rohár
@ 2023-01-27 11:16 ` Lee Jones
2023-02-24 9:15 ` Krzysztof Kozlowski
2023-02-24 9:13 ` Krzysztof Kozlowski
1 sibling, 1 reply; 42+ messages in thread
From: Lee Jones @ 2023-01-27 11:16 UTC (permalink / raw)
To: Pali Rohár
Cc: Arnd Bergmann, Linus Walleij, soc, linux-kernel, devicetree
On Mon, 26 Dec 2022, Pali Rohár wrote:
> Add device-tree bindings documentation for Turris 1.x RGB LEDs.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
> .../bindings/leds/cznic,turris1x-leds.yaml | 118 ++++++++++++++++++
> 1 file changed, 118 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/leds/cznic,turris1x-leds.yaml
Needs a DT Ack (now Cc:ed)
> diff --git a/Documentation/devicetree/bindings/leds/cznic,turris1x-leds.yaml b/Documentation/devicetree/bindings/leds/cznic,turris1x-leds.yaml
> new file mode 100644
> index 000000000000..bcaab5b03128
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/cznic,turris1x-leds.yaml
> @@ -0,0 +1,118 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/cznic,turris1x-leds.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: CZ.NIC's Turris 1.x LEDs driver
> +
> +maintainers:
> + - Pali Rohár <pali@kernel.org>
> +
> +description:
> + This module adds support for the RGB LEDs found on the front panel of the
> + Turris 1.x routers. There are 8 RGB LEDs that are controlled by CZ.NIC CPLD
> + firmware running on Lattice FPGA. Firmware is open source and available at
> + https://gitlab.nic.cz/turris/hw/turris_cpld/-/blob/master/CZ_NIC_Router_CPLD.v
> +
> +properties:
> + compatible:
> + const: cznic,turris1x-leds
> +
> + reg:
> + description: CPLD address range where LED registers are mapped
> + maxItems: 1
> +
> + "#address-cells":
> + const: 1
> +
> + "#size-cells":
> + const: 0
> +
> +patternProperties:
> + "^multi-led@[0-7]$":
> + type: object
> + $ref: leds-class-multicolor.yaml#
> + unevaluatedProperties: false
> +
> + properties:
> + reg:
> + minimum: 0
> + maximum: 7
> +
> + required:
> + - reg
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/leds/common.h>
> +
> + cpld@3,0 {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges = <0x0 0x3 0x0 0x00020000>;
> +
> + led-controller@13 {
> + compatible = "cznic,turris1x-leds";
> + reg = <0x13 0x1d>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + multi-led@0 {
> + reg = <0x0>;
> + color = <LED_COLOR_ID_RGB>;
> + function = LED_FUNCTION_WAN;
> + };
> +
> + multi-led@1 {
> + reg = <0x1>;
> + color = <LED_COLOR_ID_RGB>;
> + function = LED_FUNCTION_LAN;
> + function-enumerator = <5>;
> + };
> +
> + multi-led@2 {
> + reg = <0x2>;
> + color = <LED_COLOR_ID_RGB>;
> + function = LED_FUNCTION_LAN;
> + function-enumerator = <4>;
> + };
> +
> + multi-led@3 {
> + reg = <0x3>;
> + color = <LED_COLOR_ID_RGB>;
> + function = LED_FUNCTION_LAN;
> + function-enumerator = <3>;
> + };
> +
> + multi-led@4 {
> + reg = <0x4>;
> + color = <LED_COLOR_ID_RGB>;
> + function = LED_FUNCTION_LAN;
> + function-enumerator = <2>;
> + };
> +
> + multi-led@5 {
> + reg = <0x5>;
> + color = <LED_COLOR_ID_RGB>;
> + function = LED_FUNCTION_LAN;
> + function-enumerator = <1>;
> + };
> +
> + multi-led@6 {
> + reg = <0x6>;
> + color = <LED_COLOR_ID_RGB>;
> + function = LED_FUNCTION_WLAN;
> + };
> +
> + multi-led@7 {
> + reg = <0x7>;
> + color = <LED_COLOR_ID_RGB>;
> + function = LED_FUNCTION_POWER;
> + };
> + };
> + };
> +
> +...
> --
> 2.20.1
>
--
Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH RESEND 5/8] leds: Add support for Turris 1.x LEDs
2022-12-26 12:36 ` [PATCH RESEND 5/8] leds: Add support for Turris 1.x LEDs Pali Rohár
@ 2023-01-27 11:20 ` Lee Jones
2023-02-02 23:46 ` Pali Rohár
2023-02-24 9:25 ` Krzysztof Kozlowski
2023-02-24 9:22 ` Krzysztof Kozlowski
2023-02-24 9:28 ` Lee Jones
2 siblings, 2 replies; 42+ messages in thread
From: Lee Jones @ 2023-01-27 11:20 UTC (permalink / raw)
To: Pali Rohár, pavel; +Cc: Arnd Bergmann, Linus Walleij, soc, linux-kernel
Pavel, are you happy with the user-space interface?
On Mon, 26 Dec 2022, Pali Rohár wrote:
> This adds support for the RGB LEDs found on the front panel of the
> Turris 1.x routers. There are 8 RGB LEDs that are controlled by
> CZ.NIC CPLD firmware running on Lattice FPGA.
>
> CPLD firmware provides HW triggering mode for all LEDs except WiFi LED
> which is automatically enabled after power on reset. LAN LEDs share HW
> registers for RGB colors settings, so it is not possible to set different
> colors for individual LAN LEDs.
>
> CZ.NIC CPLD firmware is open source and available at:
> https://gitlab.nic.cz/turris/hw/turris_cpld/-/blob/master/CZ_NIC_Router_CPLD.v
>
> This driver uses the multicolor LED framework and HW led triggers.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <kabel@kernel.org>
> ---
> .../testing/sysfs-class-led-driver-turris1x | 31 ++
> drivers/leds/Kconfig | 9 +
> drivers/leds/Makefile | 1 +
> drivers/leds/leds-turris-1x.c | 474 ++++++++++++++++++
> 4 files changed, 515 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-class-led-driver-turris1x
> create mode 100644 drivers/leds/leds-turris-1x.c
>
> diff --git a/Documentation/ABI/testing/sysfs-class-led-driver-turris1x b/Documentation/ABI/testing/sysfs-class-led-driver-turris1x
> new file mode 100644
> index 000000000000..272695ae400b
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-led-driver-turris1x
> @@ -0,0 +1,31 @@
> +What: /sys/class/leds/<led>/device/brightness
> +Date: July 2022
> +KernelVersion: 6.3
> +Contact: Pali Rohár <pali@kernel.org>
> +Description: (RW) On the back size of the Turris 1.x routers there is also
> + a button which can be used to control the intensity of all the
> + LEDs at once, so that if they are too bright, user can dim them.
> +
> + The CPLD firmware cycles between 8 levels of this global
> + brightness, but this setting can have any integer value between
> + 0 and 255. It is therefore convenient to be able to change this
> + setting from software.
> +
> + Format: %u
> +
> +What: /sys/class/leds/<led>/device/brightness_level
> +Date: July 2022
> +KernelVersion: 6.3
> +Contact: Pali Rohár <pali@kernel.org>
> +Description: (RW) Current brightness level value (0-7).
> +
> + Format: %u
> +
> +What: /sys/class/leds/<led>/device/brightness_values
> +Date: July 2022
> +KernelVersion: 6.3
> +Contact: Pali Rohár <pali@kernel.org>
> +Description: (RW) Values of all 8 levels between which CPLD firmware cycles
> + when brightness button is pressed.
> +
> + Format: %u %u %u %u %u %u %u %u
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 499d0f215a8b..6f78764e20aa 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -157,6 +157,15 @@ config LEDS_EL15203000
> To compile this driver as a module, choose M here: the module
> will be called leds-el15203000.
>
> +config LEDS_TURRIS_1X
> + tristate "LED support for CZ.NIC's Turris 1.x"
> + depends on LEDS_CLASS_MULTICOLOR
> + depends on OF
> + select LEDS_TRIGGERS
> + help
> + This option enables support for LEDs found on the front side of
> + CZ.NIC's Turris 1.x routers.
> +
> config LEDS_TURRIS_OMNIA
> tristate "LED support for CZ.NIC's Turris Omnia"
> depends on LEDS_CLASS_MULTICOLOR
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 4fd2f92cd198..de08083dbbca 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -82,6 +82,7 @@ obj-$(CONFIG_LEDS_TCA6507) += leds-tca6507.o
> obj-$(CONFIG_LEDS_TI_LMU_COMMON) += leds-ti-lmu-common.o
> obj-$(CONFIG_LEDS_TLC591XX) += leds-tlc591xx.o
> obj-$(CONFIG_LEDS_TPS6105X) += leds-tps6105x.o
> +obj-$(CONFIG_LEDS_TURRIS_1X) += leds-turris-1x.o
> obj-$(CONFIG_LEDS_TURRIS_OMNIA) += leds-turris-omnia.o
> obj-$(CONFIG_LEDS_WM831X_STATUS) += leds-wm831x-status.o
> obj-$(CONFIG_LEDS_WM8350) += leds-wm8350.o
> diff --git a/drivers/leds/leds-turris-1x.c b/drivers/leds/leds-turris-1x.c
> new file mode 100644
> index 000000000000..cf7567b64306
> --- /dev/null
> +++ b/drivers/leds/leds-turris-1x.c
> @@ -0,0 +1,474 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// (C) 2022 Pali Rohár <pali@kernel.org>
> +//
> +// CZ.NIC's Turris 1.x LEDs driver, controlled by CPLD firmware:
> +// https://gitlab.nic.cz/turris/hw/turris_cpld/-/blob/master/CZ_NIC_Router_CPLD.v
> +
> +#include <linux/i2c.h>
> +#include <linux/led-class-multicolor.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include "leds.h"
> +
> +/* LED registers starts at byte 0x13 in CPLD memory map */
> +#define TURRIS1X_LED_REG_OFF(reg) ((reg)-0x13)
> +
> +/* LEDs 1-5 share common register for setting brightness */
> +#define TURRIS1X_LED_BRIGHTNESS_OFF(idx) ({ const u8 _idx = (idx) & 7; \
> + (_idx == 0) ? 0 : \
> + (_idx <= 5) ? 1 : \
> + (_idx - 4); })
> +
> +#define TURRIS1X_LED_BRIGHTNESS_REG(idx, color) TURRIS1X_LED_REG_OFF(0x13 + \
> + 3 * TURRIS1X_LED_BRIGHTNESS_OFF(idx) + \
> + ((color) & 3))
> +#define TURRIS1X_LED_GLOBAL_LEVEL_REG TURRIS1X_LED_REG_OFF(0x20)
> +#define TURRIS1X_LED_GET_GLOBAL_BRIGHTNESS_REG TURRIS1X_LED_REG_OFF(0x21)
> +#define TURRIS1X_LED_SW_OVERRIDE_REG TURRIS1X_LED_REG_OFF(0x22)
> +#define TURRIS1X_LED_SW_DISABLE_REG TURRIS1X_LED_REG_OFF(0x23)
> +#define TURRIS1X_LED_GLOBAL_BRIGHTNESS_REG(lvl) TURRIS1X_LED_REG_OFF(0x28 + ((lvl) & 7))
> +
> +struct turris1x_led {
> + struct led_classdev_mc mc_cdev;
> + struct mc_subled subled_info[3];
> + u32 reg;
> + bool registered;
> +};
> +
> +#define to_turris1x_led(l) container_of(l, struct turris1x_led, mc_cdev)
> +
> +struct turris1x_leds {
> + void __iomem *regs;
> + struct mutex lock;
> + struct turris1x_led leds[8];
> +};
> +
> +static struct led_hw_trigger_type turris1x_hw_trigger_type;
> +
> +static int turris1x_hwtrig_activate(struct led_classdev *cdev)
> +{
> + struct turris1x_leds *leds = dev_get_drvdata(cdev->dev->parent);
> + struct turris1x_led *led = to_turris1x_led(lcdev_to_mccdev(cdev));
> + u8 val;
> +
> + /* Disable software control of LED */
> + mutex_lock(&leds->lock);
> + val = readb(leds->regs + TURRIS1X_LED_SW_OVERRIDE_REG);
> + val &= ~BIT(led->reg);
> + writeb(val, leds->regs + TURRIS1X_LED_SW_OVERRIDE_REG);
> + mutex_unlock(&leds->lock);
> +
> + return 0;
> +}
> +
> +static void turris1x_hwtrig_deactivate(struct led_classdev *cdev)
> +{
> + struct turris1x_leds *leds = dev_get_drvdata(cdev->dev->parent);
> + struct turris1x_led *led = to_turris1x_led(lcdev_to_mccdev(cdev));
> + u8 val;
> +
> + /* Enable software control of LED */
> + mutex_lock(&leds->lock);
> + val = readb(leds->regs + TURRIS1X_LED_SW_OVERRIDE_REG);
> + val |= BIT(led->reg);
> + writeb(val, leds->regs + TURRIS1X_LED_SW_OVERRIDE_REG);
> + mutex_unlock(&leds->lock);
> +}
> +
> +static struct led_trigger turris1x_hw_trigger = {
> + .name = "turris1x-cpld",
> + .activate = turris1x_hwtrig_activate,
> + .deactivate = turris1x_hwtrig_deactivate,
> + .trigger_type = &turris1x_hw_trigger_type,
> +};
> +
> +static enum led_brightness turris1x_led_brightness_get(struct led_classdev *cdev)
> +{
> + struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
> + struct turris1x_leds *leds = dev_get_drvdata(cdev->dev->parent);
> + struct turris1x_led *led = to_turris1x_led(mc_cdev);
> +
> + if (!(readb(leds->regs + TURRIS1X_LED_SW_OVERRIDE_REG) & BIT(led->reg)))
> + return 1;
> + else if (!(readb(leds->regs + TURRIS1X_LED_SW_DISABLE_REG) & BIT(led->reg)))
> + return 1;
> + else
> + return 0;
> +}
> +
> +static void turris1x_led_brightness_set(struct led_classdev *cdev,
> + enum led_brightness brightness)
> +{
> + struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
> + struct turris1x_leds *leds = dev_get_drvdata(cdev->dev->parent);
> + struct turris1x_led *led = to_turris1x_led(mc_cdev);
> + int i, j;
> + u8 val;
> +
> + mutex_lock(&leds->lock);
> +
> + /* Set new brightness value for each color when LED is enabled */
> + if (brightness) {
> + led_mc_calc_color_components(mc_cdev, brightness);
> + for (i = 0; i < ARRAY_SIZE(led->subled_info); i++)
> + writeb(mc_cdev->subled_info[i].brightness,
> + leds->regs + TURRIS1X_LED_BRIGHTNESS_REG(led->reg, i));
> +
> + /*
> + * LEDs 1-5 (LAN) share common color settings in same sets
> + * of HW registers and therefore it is not possible to set
> + * different colors. So when chaning color of one LED then
> + * reflect color change for all of them.
> + */
> + if (led->reg >= 1 && led->reg <= 5) {
> + for (j = 0; j < ARRAY_SIZE(leds->leds); j++) {
> + if (leds->leds[j].reg < 1 ||
> + leds->leds[j].reg > 5 ||
> + leds->leds[j].reg == led->reg)
> + continue;
> + for (i = 0; i < ARRAY_SIZE(led->subled_info); i++)
> + leds->leds[j].mc_cdev.subled_info[i].intensity =
> + mc_cdev->subled_info[i].intensity;
> + }
> + }
> + }
> +
> + /* Enable / disable LED for software control */
> + val = readb(leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
> + if (brightness && (val & BIT(led->reg)))
> + writeb(val & ~BIT(led->reg),
> + leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
> + else if (!brightness && !(val & BIT(led->reg)))
> + writeb(val | BIT(led->reg),
> + leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
> +
> + mutex_unlock(&leds->lock);
> +}
> +
> +static int turris1x_led_register(struct device *dev, struct turris1x_leds *leds,
> + struct device_node *np, u8 val_sw_override,
> + u8 val_sw_disable)
> +{
> + struct led_init_data init_data = {};
> + struct led_classdev *cdev;
> + struct turris1x_led *led;
> + int ret, color;
> + u32 reg;
> + int i;
> +
> + const unsigned int colors[ARRAY_SIZE(led->subled_info)] = {
> + LED_COLOR_ID_RED, LED_COLOR_ID_GREEN, LED_COLOR_ID_BLUE
> + };
> +
> + ret = of_property_read_u32(np, "reg", ®);
> + if (ret || reg >= ARRAY_SIZE(leds->leds)) {
> + dev_err(dev,
> + "Node %pOF: must contain 'reg' property with values between 0 and %u\n",
> + np, (unsigned int)ARRAY_SIZE(leds->leds) - 1);
> + return -EINVAL;
> + }
> +
> + ret = of_property_read_u32(np, "color", &color);
> + if (ret || color != LED_COLOR_ID_RGB) {
> + dev_err(dev,
> + "Node %pOF: must contain 'color' property with value LED_COLOR_ID_RGB\n",
> + np);
> + return -EINVAL;
> + }
> +
> + led = &leds->leds[reg];
> +
> + if (led->registered) {
> + dev_err(dev, "Node %pOF: duplicate 'reg' property %u\n",
> + np, reg);
> + return -EINVAL;
> + }
> +
> + led->registered = true;
> + led->reg = reg;
> +
> + /* Set initial colors to what are currently in use */
> + for (i = 0; i < ARRAY_SIZE(led->subled_info); i++) {
> + led->subled_info[i].intensity =
> + readb(leds->regs + TURRIS1X_LED_BRIGHTNESS_REG(reg, i));
> + led->subled_info[i].color_index = colors[i];
> + led->subled_info[i].channel = i;
> + }
> +
> + led->mc_cdev.subled_info = led->subled_info;
> + led->mc_cdev.num_colors = ARRAY_SIZE(led->subled_info);
> +
> + init_data.fwnode = &np->fwnode;
> +
> + cdev = &led->mc_cdev.led_cdev;
> + cdev->max_brightness = 1;
> + cdev->brightness_get = turris1x_led_brightness_get;
> + cdev->brightness_set = turris1x_led_brightness_set;
> +
> + /* All LEDs except LED 6 (WiFi) can be controller by hardware trigger */
> + if (reg != 6)
> + cdev->trigger_type = &turris1x_hw_trigger_type;
> +
> + /* Disable hardware trigger for LED 6 (WiFi) - allow software control */
> + if (reg == 6 && !(val_sw_override & BIT(6))) {
> + if (!(val_sw_disable & BIT(6))) {
> + val_sw_disable |= BIT(6);
> + writeb(val_sw_disable,
> + leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
> + }
> + val_sw_override |= BIT(6);
> + writeb(val_sw_override,
> + leds->regs + TURRIS1X_LED_SW_OVERRIDE_REG);
> + }
> +
> + if (!(val_sw_override & BIT(reg)))
> + cdev->default_trigger = turris1x_hw_trigger.name;
> +
> + if (!(val_sw_override & BIT(reg)) || !(val_sw_disable & BIT(reg)))
> + cdev->brightness = 1;
> +
> + ret = devm_led_classdev_multicolor_register_ext(dev, &led->mc_cdev,
> + &init_data);
> + if (ret) {
> + dev_err(dev, "Cannot register LED %pOF: %i\n", np, ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static ssize_t brightness_show(struct device *dev, struct device_attribute *a,
> + char *buf)
> +{
> + struct turris1x_leds *leds = dev_get_drvdata(dev);
> + unsigned int brightness;
> +
> + /*
> + * Current brightness value is available in read-only register
> + * TURRIS1X_LED_GET_GLOBAL_BRIGHTNESS_REG. Equivalent code is:
> + * level = readb(leds->regs + TURRIS1X_LED_GLOBAL_LEVEL_REG) & 7;
> + * brightness = readb(leds->regs + TURRIS1X_LED_GLOBAL_BRIGHTNESS_REG(level));
> + */
> + brightness = readb(leds->regs + TURRIS1X_LED_GET_GLOBAL_BRIGHTNESS_REG);
> +
> + return sprintf(buf, "%u\n", brightness);
> +}
> +
> +static ssize_t brightness_store(struct device *dev, struct device_attribute *a,
> + const char *buf, size_t count)
> +{
> + struct turris1x_leds *leds = dev_get_drvdata(dev);
> + int best_error, error, level, value;
> + unsigned long brightness;
> + u8 best_level;
> +
> + if (kstrtoul(buf, 10, &brightness))
> + return -EINVAL;
> +
> + if (brightness > 255)
> + return -EINVAL;
> +
> + /*
> + * Brightness can be set only to one of 8 predefined value levels
> + * available in TURRIS1X_LED_GLOBAL_BRIGHTNESS_REG(level) registers.
> + * Choose level which has nearest value to the specified brightness.
> + */
> + best_level = 0;
> + best_error = INT_MAX;
> + for (level = 0; level < 8; level++) {
> + value = readb(leds->regs +
> + TURRIS1X_LED_GLOBAL_BRIGHTNESS_REG(level));
> + error = abs(value - (int)brightness);
> + if (best_error > error) {
> + best_error = error;
> + best_level = level;
> + }
> + }
> +
> + writeb(best_level, leds->regs + TURRIS1X_LED_GLOBAL_LEVEL_REG);
> +
> + return count;
> +}
> +static DEVICE_ATTR_RW(brightness);
> +
> +static ssize_t brightness_level_show(struct device *dev,
> + struct device_attribute *a, char *buf)
> +{
> + struct turris1x_leds *leds = dev_get_drvdata(dev);
> + unsigned int level;
> +
> + level = readb(leds->regs + TURRIS1X_LED_GLOBAL_LEVEL_REG) & 7;
> +
> + return sprintf(buf, "%u\n", level);
> +}
> +
> +static ssize_t brightness_level_store(struct device *dev,
> + struct device_attribute *a,
> + const char *buf, size_t count)
> +{
> + struct turris1x_leds *leds = dev_get_drvdata(dev);
> + unsigned long level;
> +
> + if (kstrtoul(buf, 10, &level))
> + return -EINVAL;
> +
> + if (level > 7)
> + return -EINVAL;
> +
> + writeb(level, leds->regs + TURRIS1X_LED_GLOBAL_LEVEL_REG);
> +
> + return count;
> +}
> +static DEVICE_ATTR_RW(brightness_level);
> +
> +static ssize_t brightness_values_show(struct device *dev,
> + struct device_attribute *a, char *buf)
> +{
> + struct turris1x_leds *leds = dev_get_drvdata(dev);
> + unsigned int vals[8];
> + int i;
> +
> + for (i = 0; i < 8; i++)
> + vals[i] = readb(leds->regs +
> + TURRIS1X_LED_GLOBAL_BRIGHTNESS_REG(i));
> +
> + return sprintf(buf, "%u %u %u %u %u %u %u %u\n", vals[0], vals[1],
> + vals[2], vals[3], vals[4], vals[5], vals[6], vals[7]);
> +}
> +
> +static ssize_t brightness_values_store(struct device *dev,
> + struct device_attribute *a,
> + const char *buf, size_t count)
> +{
> + struct turris1x_leds *leds = dev_get_drvdata(dev);
> + unsigned int vals[8];
> + int nchars;
> + int i;
> +
> + if (sscanf(buf, "%u %u %u %u %u %u %u %u%n", &vals[0], &vals[1],
> + &vals[2], &vals[3], &vals[4], &vals[5], &vals[6], &vals[7],
> + &nchars) != 8 || nchars + 1 < count)
> + return -EINVAL;
> +
> + for (i = 0; i < 8; i++)
> + writeb(vals[i],
> + leds->regs + TURRIS1X_LED_GLOBAL_BRIGHTNESS_REG(i));
> +
> + return count;
> +}
> +static DEVICE_ATTR_RW(brightness_values);
> +
> +static struct attribute *turris1x_leds_controller_attrs[] = {
> + &dev_attr_brightness.attr,
> + &dev_attr_brightness_level.attr,
> + &dev_attr_brightness_values.attr,
> + NULL,
> +};
> +ATTRIBUTE_GROUPS(turris1x_leds_controller);
> +
> +static int turris1x_leds_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *np = dev_of_node(dev);
> + struct device_node *child;
> + struct turris1x_leds *leds;
> + struct resource *res;
> + void __iomem *regs;
> + u8 val_sw_override;
> + u8 val_sw_disable;
> + int ret;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -ENODEV;
> +
> + regs = devm_ioremap_resource(dev, res);
> + if (IS_ERR(regs))
> + return PTR_ERR(regs);
> +
> + leds = devm_kzalloc(dev, sizeof(*leds), GFP_KERNEL);
> + if (!leds)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, leds);
> +
> + leds->regs = regs;
> + mutex_init(&leds->lock);
> +
> + ret = devm_led_trigger_register(dev, &turris1x_hw_trigger);
> + if (ret) {
> + dev_err(dev, "Cannot register private LED trigger: %d\n", ret);
> + return ret;
> + }
> +
> + val_sw_override = readb(leds->regs + TURRIS1X_LED_SW_OVERRIDE_REG);
> + val_sw_disable = readb(leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
> +
> + for_each_available_child_of_node(np, child) {
> + ret = turris1x_led_register(dev, leds, child,
> + val_sw_override, val_sw_disable);
> + if (ret) {
> + of_node_put(child);
> + return ret;
> + }
> + }
> +
> + ret = devm_device_add_groups(dev, turris1x_leds_controller_groups);
> + if (ret) {
> + dev_err(dev, "Could not add attribute group!\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static void turris1x_leds_shutdown(struct platform_device *pdev)
> +{
> + struct turris1x_leds *leds = platform_get_drvdata(pdev);
> + int i, j;
> + u8 val;
> +
> + /*
> + * LED registers are persisent across board resets.
> + * So reset LEDs to default state before kernel reboots.
> + */
> +
> + /* Disable software control of all LEDs except LED 6 (WiFi) */
> + writeb(BIT(6), leds->regs + TURRIS1X_LED_SW_OVERRIDE_REG);
> +
> + /* Turn off LED 6 (WiFi) as there is no hardware trigger for it */
> + val = readb(leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
> + writeb(val | BIT(6), leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
> +
> + /* Reset colors of all LEDs to default values */
> + for (i = 0; i < ARRAY_SIZE(leds->leds); i++) {
> + /* Skip LAN2-LAN5 LEDs which share color register with LAN1 */
> + if (i >= 2 && i <= 5)
> + continue;
> + for (j = 0; j < ARRAY_SIZE(leds->leds[i].subled_info); j++)
> + writeb(0xff,
> + leds->regs + TURRIS1X_LED_BRIGHTNESS_REG(i, j));
> + }
> +}
> +
> +static const struct of_device_id of_turris1x_leds_match[] = {
> + { .compatible = "cznic,turris1x-leds" },
> + {},
> +};
> +
> +static struct platform_driver turris1x_leds_driver = {
> + .probe = turris1x_leds_probe,
> + .shutdown = turris1x_leds_shutdown,
> + .driver = {
> + .name = "turris1x_leds",
> + .of_match_table = of_turris1x_leds_match,
> + },
> +};
> +module_platform_driver(turris1x_leds_driver);
> +
> +MODULE_AUTHOR("Pali Rohár <pali@kernel.org>");
> +MODULE_DESCRIPTION("CZ.NIC's Turris 1.x LEDs");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:turris1x_leds");
> --
> 2.20.1
>
--
Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH RESEND 5/8] leds: Add support for Turris 1.x LEDs
2023-01-27 11:20 ` Lee Jones
@ 2023-02-02 23:46 ` Pali Rohár
2023-02-24 9:25 ` Krzysztof Kozlowski
1 sibling, 0 replies; 42+ messages in thread
From: Pali Rohár @ 2023-02-02 23:46 UTC (permalink / raw)
To: Lee Jones; +Cc: pavel, Arnd Bergmann, Linus Walleij, soc, linux-kernel
This interface was designed from existing turris-omnia interface:
https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-class-led-driver-turris-omnia
On Friday 27 January 2023 11:20:35 Lee Jones wrote:
> Pavel, are you happy with the user-space interface?
>
> On Mon, 26 Dec 2022, Pali Rohár wrote:
>
> > This adds support for the RGB LEDs found on the front panel of the
> > Turris 1.x routers. There are 8 RGB LEDs that are controlled by
> > CZ.NIC CPLD firmware running on Lattice FPGA.
> >
> > CPLD firmware provides HW triggering mode for all LEDs except WiFi LED
> > which is automatically enabled after power on reset. LAN LEDs share HW
> > registers for RGB colors settings, so it is not possible to set different
> > colors for individual LAN LEDs.
> >
> > CZ.NIC CPLD firmware is open source and available at:
> > https://gitlab.nic.cz/turris/hw/turris_cpld/-/blob/master/CZ_NIC_Router_CPLD.v
> >
> > This driver uses the multicolor LED framework and HW led triggers.
> >
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Reviewed-by: Marek Behún <kabel@kernel.org>
> > ---
> > .../testing/sysfs-class-led-driver-turris1x | 31 ++
> > drivers/leds/Kconfig | 9 +
> > drivers/leds/Makefile | 1 +
> > drivers/leds/leds-turris-1x.c | 474 ++++++++++++++++++
> > 4 files changed, 515 insertions(+)
> > create mode 100644 Documentation/ABI/testing/sysfs-class-led-driver-turris1x
> > create mode 100644 drivers/leds/leds-turris-1x.c
> >
> > diff --git a/Documentation/ABI/testing/sysfs-class-led-driver-turris1x b/Documentation/ABI/testing/sysfs-class-led-driver-turris1x
> > new file mode 100644
> > index 000000000000..272695ae400b
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-class-led-driver-turris1x
> > @@ -0,0 +1,31 @@
> > +What: /sys/class/leds/<led>/device/brightness
> > +Date: July 2022
> > +KernelVersion: 6.3
> > +Contact: Pali Rohár <pali@kernel.org>
> > +Description: (RW) On the back size of the Turris 1.x routers there is also
> > + a button which can be used to control the intensity of all the
> > + LEDs at once, so that if they are too bright, user can dim them.
> > +
> > + The CPLD firmware cycles between 8 levels of this global
> > + brightness, but this setting can have any integer value between
> > + 0 and 255. It is therefore convenient to be able to change this
> > + setting from software.
> > +
> > + Format: %u
> > +
> > +What: /sys/class/leds/<led>/device/brightness_level
> > +Date: July 2022
> > +KernelVersion: 6.3
> > +Contact: Pali Rohár <pali@kernel.org>
> > +Description: (RW) Current brightness level value (0-7).
> > +
> > + Format: %u
> > +
> > +What: /sys/class/leds/<led>/device/brightness_values
> > +Date: July 2022
> > +KernelVersion: 6.3
> > +Contact: Pali Rohár <pali@kernel.org>
> > +Description: (RW) Values of all 8 levels between which CPLD firmware cycles
> > + when brightness button is pressed.
> > +
> > + Format: %u %u %u %u %u %u %u %u
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH RESEND 1/8] dt-bindings: leds: register-bit-led: Add active-low property
2023-01-27 11:16 ` Lee Jones
@ 2023-02-23 14:22 ` Lee Jones
2023-02-23 16:48 ` Pali Rohár
0 siblings, 1 reply; 42+ messages in thread
From: Lee Jones @ 2023-02-23 14:22 UTC (permalink / raw)
To: Pali Rohár
Cc: Arnd Bergmann, Linus Walleij, soc, linux-kernel, devicetree
On Fri, 27 Jan 2023, Lee Jones wrote:
> On Mon, 26 Dec 2022, Pali Rohár wrote:
>
> > Allow to define inverted logic (0 - enable LED, 1 - disable LED) via
> > active-low property.
> >
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > ---
> > Documentation/devicetree/bindings/leds/register-bit-led.yaml | 5 +++++
> > 1 file changed, 5 insertions(+)
>
> Needs a DT Ack (now Cc:ed)
I can't do anything with this set until we have a DT Ack.
If you don't receive one soon, I'd suggest resending the set again with
all of the DT people on Cc that it should have been sent to in the first
place.
> > diff --git a/Documentation/devicetree/bindings/leds/register-bit-led.yaml b/Documentation/devicetree/bindings/leds/register-bit-led.yaml
> > index ed26ec19ecbd..418130b29caa 100644
> > --- a/Documentation/devicetree/bindings/leds/register-bit-led.yaml
> > +++ b/Documentation/devicetree/bindings/leds/register-bit-led.yaml
> > @@ -43,6 +43,11 @@ properties:
> > 0x100000, 0x200000, 0x400000, 0x800000, 0x1000000, 0x2000000, 0x4000000,
> > 0x8000000, 0x10000000, 0x20000000, 0x40000000, 0x80000000 ]
> >
> > + active-low:
> > + $ref: /schemas/types.yaml#/definitions/flag
> > + description:
> > + LED is ON when bit in register is not set
> > +
> > offset:
> > description:
> > register offset to the register controlling this LED
> > --
> > 2.20.1
> >
>
> --
> Lee Jones [李琼斯]
--
Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH RESEND 2/8] leds: syscon: Implement support for active-low property
2022-12-26 12:36 ` [PATCH RESEND 2/8] leds: syscon: Implement support for " Pali Rohár
@ 2023-02-23 14:25 ` Lee Jones
0 siblings, 0 replies; 42+ messages in thread
From: Lee Jones @ 2023-02-23 14:25 UTC (permalink / raw)
To: Pali Rohár; +Cc: Arnd Bergmann, Linus Walleij, soc, linux-kernel
On Mon, 26 Dec 2022, Pali Rohár wrote:
> This new active-low property specify that LED has inverted logic
> (0 - enable LED, 1 - disable LED).
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> drivers/leds/leds-syscon.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
Reviewed-by: Lee Jones <lee@kernel.org>
> diff --git a/drivers/leds/leds-syscon.c b/drivers/leds/leds-syscon.c
> index 7eddb8ecb44e..5e605d8438e9 100644
> --- a/drivers/leds/leds-syscon.c
> +++ b/drivers/leds/leds-syscon.c
> @@ -29,6 +29,7 @@ struct syscon_led {
> struct regmap *map;
> u32 offset;
> u32 mask;
> + bool active_low;
> bool state;
> };
>
> @@ -41,10 +42,10 @@ static void syscon_led_set(struct led_classdev *led_cdev,
> int ret;
>
> if (value == LED_OFF) {
> - val = 0;
> + val = sled->active_low ? sled->mask : 0;
> sled->state = false;
> } else {
> - val = sled->mask;
> + val = sled->active_low ? 0 : sled->mask;
> sled->state = true;
> }
>
> @@ -85,6 +86,8 @@ static int syscon_led_probe(struct platform_device *pdev)
> return -EINVAL;
> if (of_property_read_u32(np, "mask", &sled->mask))
> return -EINVAL;
> + if (of_find_property(np, "active-low", NULL))
> + sled->active_low = true;
>
> state = of_get_property(np, "default-state", NULL);
> if (state) {
> @@ -95,17 +98,20 @@ static int syscon_led_probe(struct platform_device *pdev)
> if (ret < 0)
> return ret;
> sled->state = !!(val & sled->mask);
> + if (sled->active_low)
> + sled->state = !sled->state;
> } else if (!strcmp(state, "on")) {
> sled->state = true;
> ret = regmap_update_bits(map, sled->offset,
> sled->mask,
> - sled->mask);
> + sled->active_low ? 0 : sled->mask);
> if (ret < 0)
> return ret;
> } else {
> sled->state = false;
> ret = regmap_update_bits(map, sled->offset,
> - sled->mask, 0);
> + sled->mask,
> + sled->active_low ? sled->mask : 0);
> if (ret < 0)
> return ret;
> }
> --
> 2.20.1
>
--
Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH RESEND 1/8] dt-bindings: leds: register-bit-led: Add active-low property
2023-02-23 14:22 ` Lee Jones
@ 2023-02-23 16:48 ` Pali Rohár
2023-02-23 20:59 ` Linus Walleij
0 siblings, 1 reply; 42+ messages in thread
From: Pali Rohár @ 2023-02-23 16:48 UTC (permalink / raw)
To: Lee Jones; +Cc: Arnd Bergmann, Linus Walleij, soc, linux-kernel, devicetree
On Thursday 23 February 2023 14:22:52 Lee Jones wrote:
> On Fri, 27 Jan 2023, Lee Jones wrote:
>
> > On Mon, 26 Dec 2022, Pali Rohár wrote:
> >
> > > Allow to define inverted logic (0 - enable LED, 1 - disable LED) via
> > > active-low property.
> > >
> > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > > ---
> > > Documentation/devicetree/bindings/leds/register-bit-led.yaml | 5 +++++
> > > 1 file changed, 5 insertions(+)
> >
> > Needs a DT Ack (now Cc:ed)
>
> I can't do anything with this set until we have a DT Ack.
>
> If you don't receive one soon, I'd suggest resending the set again with
> all of the DT people on Cc that it should have been sent to in the first
> place.
(Re)Sending one email multiple times is against email etiquette,
moreover it is spam technique and reason for marking sender on the
blacklist.
Moreover I have already sent it more than one time. DT people are known
to not respond to patches and pull requests and I have no motivation to
send reminder emails for them for more than half of year.
So I would suggest to not send emails to people who just do not want to
receive or read emails. It is logical reaction.
This patch is here for more than 6 months, so I do not see reason why to
wait for Godot. Rather move forward than stepping at the same position.
> > > diff --git a/Documentation/devicetree/bindings/leds/register-bit-led.yaml b/Documentation/devicetree/bindings/leds/register-bit-led.yaml
> > > index ed26ec19ecbd..418130b29caa 100644
> > > --- a/Documentation/devicetree/bindings/leds/register-bit-led.yaml
> > > +++ b/Documentation/devicetree/bindings/leds/register-bit-led.yaml
> > > @@ -43,6 +43,11 @@ properties:
> > > 0x100000, 0x200000, 0x400000, 0x800000, 0x1000000, 0x2000000, 0x4000000,
> > > 0x8000000, 0x10000000, 0x20000000, 0x40000000, 0x80000000 ]
> > >
> > > + active-low:
> > > + $ref: /schemas/types.yaml#/definitions/flag
> > > + description:
> > > + LED is ON when bit in register is not set
> > > +
> > > offset:
> > > description:
> > > register offset to the register controlling this LED
> > > --
> > > 2.20.1
> > >
> >
> > --
> > Lee Jones [李琼斯]
>
> --
> Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH RESEND 1/8] dt-bindings: leds: register-bit-led: Add active-low property
2023-02-23 16:48 ` Pali Rohár
@ 2023-02-23 20:59 ` Linus Walleij
2023-02-24 8:42 ` Krzysztof Kozlowski
0 siblings, 1 reply; 42+ messages in thread
From: Linus Walleij @ 2023-02-23 20:59 UTC (permalink / raw)
To: Pali Rohár, Krzysztof Kozlowski, Rob Herring
Cc: Lee Jones, Arnd Bergmann, soc, linux-kernel, devicetree
On Thu, Feb 23, 2023 at 5:48 PM Pali Rohár <pali@kernel.org> wrote:
> On Thursday 23 February 2023 14:22:52 Lee Jones wrote:
> > On Fri, 27 Jan 2023, Lee Jones wrote:
> >
> > > On Mon, 26 Dec 2022, Pali Rohár wrote:
> > >
> > > > Allow to define inverted logic (0 - enable LED, 1 - disable LED) via
> > > > active-low property.
> > > >
> > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > > > ---
> > > > Documentation/devicetree/bindings/leds/register-bit-led.yaml | 5 +++++
> > > > 1 file changed, 5 insertions(+)
> > >
> > > Needs a DT Ack (now Cc:ed)
> >
> > I can't do anything with this set until we have a DT Ack.
> >
> > If you don't receive one soon, I'd suggest resending the set again with
> > all of the DT people on Cc that it should have been sent to in the first
> > place.
>
> (Re)Sending one email multiple times is against email etiquette,
> moreover it is spam technique and reason for marking sender on the
> blacklist.
No problem on the kernel mailing lists actually, we love to mail
bomb each other here. Yeah maybe we are a bit weird :/
> Moreover I have already sent it more than one time. DT people are known
> to not respond to patches and pull requests and I have no motivation to
> send reminder emails for them for more than half of year.
>
> So I would suggest to not send emails to people who just do not want to
> receive or read emails. It is logical reaction.
>
> This patch is here for more than 6 months, so I do not see reason why to
> wait for Godot. Rather move forward than stepping at the same position.
I understand that it is annoying.
In my experience Krzysztof and Rob (now added on To) are usually
quite responsive and helpful, so something must have made them
miss it I think.
As subsystem maintainer, if the DT reviewers haven't said anything
in ~2 weeks I tend to sanity check the binding as best I can and then
merge it. The bigger and more complex it is the more hesitant I get to
do this...
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH RESEND 1/8] dt-bindings: leds: register-bit-led: Add active-low property
2023-02-23 20:59 ` Linus Walleij
@ 2023-02-24 8:42 ` Krzysztof Kozlowski
0 siblings, 0 replies; 42+ messages in thread
From: Krzysztof Kozlowski @ 2023-02-24 8:42 UTC (permalink / raw)
To: Linus Walleij, Pali Rohár, Krzysztof Kozlowski, Rob Herring
Cc: Lee Jones, Arnd Bergmann, soc, linux-kernel, devicetree
On 23/02/2023 21:59, Linus Walleij wrote:
> On Thu, Feb 23, 2023 at 5:48 PM Pali Rohár <pali@kernel.org> wrote:
>> On Thursday 23 February 2023 14:22:52 Lee Jones wrote:
>>> On Fri, 27 Jan 2023, Lee Jones wrote:
>>>
>>>> On Mon, 26 Dec 2022, Pali Rohár wrote:
>>>>
>>>>> Allow to define inverted logic (0 - enable LED, 1 - disable LED) via
>>>>> active-low property.
>>>>>
>>>>> Signed-off-by: Pali Rohár <pali@kernel.org>
>>>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>>>> ---
>>>>> Documentation/devicetree/bindings/leds/register-bit-led.yaml | 5 +++++
>>>>> 1 file changed, 5 insertions(+)
>>>>
>>>> Needs a DT Ack (now Cc:ed)
>>>
>>> I can't do anything with this set until we have a DT Ack.
>>>
>>> If you don't receive one soon, I'd suggest resending the set again with
>>> all of the DT people on Cc that it should have been sent to in the first
>>> place.
>>
>> (Re)Sending one email multiple times is against email etiquette,
>> moreover it is spam technique and reason for marking sender on the
>> blacklist.
>
> No problem on the kernel mailing lists actually, we love to mail
> bomb each other here. Yeah maybe we are a bit weird :/
>
>> Moreover I have already sent it more than one time. DT people are known
>> to not respond to patches and pull requests and I have no motivation to
>> send reminder emails for them for more than half of year.
Is this a joke? You got here response within one day!
Sent: 18th of August
Reviewed-by: 19th of August
https://lore.kernel.org/all/f635d5a7-6817-cd62-e395-63e346775716@linaro.org/
You ignored the tag and then ignored the process and not Cc'ed us. Yet
you complain that someone did not respond to your emails. Really?
>>
>> So I would suggest to not send emails to people who just do not want to
>> receive or read emails. It is logical reaction.
>>
>> This patch is here for more than 6 months, so I do not see reason why to
>> wait for Godot. Rather move forward than stepping at the same position.
>
> I understand that it is annoying.
>
> In my experience Krzysztof and Rob (now added on To) are usually
> quite responsive and helpful, so something must have made them
> miss it I think.
>
> As subsystem maintainer, if the DT reviewers haven't said anything
> in ~2 weeks I tend to sanity check the binding as best I can and then
> merge it. The bigger and more complex it is the more hesitant I get to
> do this...
Yeah... this patch was never sent to us, thus regardless how hard we
work, it would be quite difficult to answer emails which we never received.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH RESEND 4/8] dt-bindings: leds: Add cznic,turris1x-leds.yaml binding
2022-12-26 12:36 ` [PATCH RESEND 4/8] dt-bindings: leds: Add cznic,turris1x-leds.yaml binding Pali Rohár
2023-01-27 11:16 ` Lee Jones
@ 2023-02-24 9:13 ` Krzysztof Kozlowski
1 sibling, 0 replies; 42+ messages in thread
From: Krzysztof Kozlowski @ 2023-02-24 9:13 UTC (permalink / raw)
To: Pali Rohár, Arnd Bergmann, Linus Walleij; +Cc: soc, linux-kernel
On 26/12/2022 13:36, Pali Rohár wrote:
> Add device-tree bindings documentation for Turris 1.x RGB LEDs.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
> .../bindings/leds/cznic,turris1x-leds.yaml | 118 ++++++++++++++++++
> 1 file changed, 118 insertions(+)
NAK, because:
1. You received comments for this to fix which you ignored:
https://lore.kernel.org/all/ebf5029e-83fd-e50d-b7cb-eae1b64f7145@linaro.org/
Implement the feedback you got.
2. You sent it now - without changes, ignoring feedback - not cc'ing DT
maintainers.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH RESEND 4/8] dt-bindings: leds: Add cznic,turris1x-leds.yaml binding
2023-01-27 11:16 ` Lee Jones
@ 2023-02-24 9:15 ` Krzysztof Kozlowski
2023-02-24 9:38 ` Lee Jones
0 siblings, 1 reply; 42+ messages in thread
From: Krzysztof Kozlowski @ 2023-02-24 9:15 UTC (permalink / raw)
To: Lee Jones, Pali Rohár
Cc: Arnd Bergmann, Linus Walleij, soc, linux-kernel, devicetree
On 27/01/2023 12:16, Lee Jones wrote:
> On Mon, 26 Dec 2022, Pali Rohár wrote:
>
>> Add device-tree bindings documentation for Turris 1.x RGB LEDs.
>>
>> Signed-off-by: Pali Rohár <pali@kernel.org>
>> ---
>> .../bindings/leds/cznic,turris1x-leds.yaml | 118 ++++++++++++++++++
>> 1 file changed, 118 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/leds/cznic,turris1x-leds.yaml
>
> Needs a DT Ack (now Cc:ed)
Within the same day of posting v2 of this - just after 4 hours, Pali
received review and I pointed issue to address.
The issue was not addressed, just ignored.
Therefore this patch shall not be taken, until the issues are resolved
or discussion is finished if there is disagreement about my comments..
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH RESEND 5/8] leds: Add support for Turris 1.x LEDs
2022-12-26 12:36 ` [PATCH RESEND 5/8] leds: Add support for Turris 1.x LEDs Pali Rohár
2023-01-27 11:20 ` Lee Jones
@ 2023-02-24 9:22 ` Krzysztof Kozlowski
2023-02-24 9:28 ` Lee Jones
2 siblings, 0 replies; 42+ messages in thread
From: Krzysztof Kozlowski @ 2023-02-24 9:22 UTC (permalink / raw)
To: Pali Rohár, Arnd Bergmann, Linus Walleij
Cc: soc, linux-kernel, Andy Shevchenko, Lee Jones, Pavel Machek
On 26/12/2022 13:36, Pali Rohár wrote:
> This adds support for the RGB LEDs found on the front panel of the
> Turris 1.x routers. There are 8 RGB LEDs that are controlled by
> CZ.NIC CPLD firmware running on Lattice FPGA.
>
> CPLD firmware provides HW triggering mode for all LEDs except WiFi LED
> which is automatically enabled after power on reset. LAN LEDs share HW
> registers for RGB colors settings, so it is not possible to set different
> colors for individual LAN LEDs.
>
> CZ.NIC CPLD firmware is open source and available at:
> https://gitlab.nic.cz/turris/hw/turris_cpld/-/blob/master/CZ_NIC_Router_CPLD.v
>
> This driver uses the multicolor LED framework and HW led triggers.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <kabel@kernel.org>
> ---
+Cc Andy, Lee, Pavel,
It looks like entire review of Andy was ignored and you just resent the
same version without fixing anything.
https://lore.kernel.org/all/CAHp75Vcr6o2rm+T6Tr8sS4VXCLVHtmLPWy-njOKAvO4AcZoW=A@mail.gmail.com/
Your cover letter statement:
" list for a long time without any future movement."
is so not true.
You got reviews from me for other bits, you got reviews from Andy. All
these you ignored and did not send a corrected version.
This is not how the process looks like. If you receive the feedback,
either you implement it or you keep discussion going (if you do not
agree, for example).
Ignoring the feedback and sending the same version bypassing reviewers
is unacceptable.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH RESEND 5/8] leds: Add support for Turris 1.x LEDs
2023-01-27 11:20 ` Lee Jones
2023-02-02 23:46 ` Pali Rohár
@ 2023-02-24 9:25 ` Krzysztof Kozlowski
2023-02-24 9:37 ` Lee Jones
1 sibling, 1 reply; 42+ messages in thread
From: Krzysztof Kozlowski @ 2023-02-24 9:25 UTC (permalink / raw)
To: Lee Jones, Pali Rohár, pavel
Cc: Arnd Bergmann, Linus Walleij, soc, linux-kernel
On 27/01/2023 12:20, Lee Jones wrote:
> Pavel, are you happy with the user-space interface?
>
Although not about user-space but about code - entire review was ignored
by Pali and not implemented:
https://lore.kernel.org/all/CAHp75Vcr6o2rm+T6Tr8sS4VXCLVHtmLPWy-njOKAvO4AcZoW=A@mail.gmail.com/
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH RESEND 5/8] leds: Add support for Turris 1.x LEDs
2022-12-26 12:36 ` [PATCH RESEND 5/8] leds: Add support for Turris 1.x LEDs Pali Rohár
2023-01-27 11:20 ` Lee Jones
2023-02-24 9:22 ` Krzysztof Kozlowski
@ 2023-02-24 9:28 ` Lee Jones
2023-03-09 20:35 ` Pali Rohár
2 siblings, 1 reply; 42+ messages in thread
From: Lee Jones @ 2023-02-24 9:28 UTC (permalink / raw)
To: Pali Rohár; +Cc: Arnd Bergmann, Linus Walleij, soc, linux-kernel
On Mon, 26 Dec 2022, Pali Rohár wrote:
> This adds support for the RGB LEDs found on the front panel of the
> Turris 1.x routers. There are 8 RGB LEDs that are controlled by
> CZ.NIC CPLD firmware running on Lattice FPGA.
>
> CPLD firmware provides HW triggering mode for all LEDs except WiFi LED
> which is automatically enabled after power on reset. LAN LEDs share HW
> registers for RGB colors settings, so it is not possible to set different
> colors for individual LAN LEDs.
>
> CZ.NIC CPLD firmware is open source and available at:
> https://gitlab.nic.cz/turris/hw/turris_cpld/-/blob/master/CZ_NIC_Router_CPLD.v
>
> This driver uses the multicolor LED framework and HW led triggers.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <kabel@kernel.org>
> ---
> .../testing/sysfs-class-led-driver-turris1x | 31 ++
> drivers/leds/Kconfig | 9 +
> drivers/leds/Makefile | 1 +
> drivers/leds/leds-turris-1x.c | 474 ++++++++++++++++++
> 4 files changed, 515 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-class-led-driver-turris1x
> create mode 100644 drivers/leds/leds-turris-1x.c
>
> diff --git a/Documentation/ABI/testing/sysfs-class-led-driver-turris1x b/Documentation/ABI/testing/sysfs-class-led-driver-turris1x
> new file mode 100644
> index 000000000000..272695ae400b
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-led-driver-turris1x
> @@ -0,0 +1,31 @@
> +What: /sys/class/leds/<led>/device/brightness
> +Date: July 2022
> +KernelVersion: 6.3
Date doesn't match the kernel version.
> +Contact: Pali Rohár <pali@kernel.org>
> +Description: (RW) On the back size of the Turris 1.x routers there is also
"side"
Drop "also"
> + a button which can be used to control the intensity of all the
> + LEDs at once, so that if they are too bright, user can dim them.
"the user"
> + The CPLD firmware cycles between 8 levels of this global
Drop "this"
> + brightness, but this setting can have any integer value between
> + 0 and 255. It is therefore convenient to be able to change this
> + setting from software.
> +
> + Format: %u
> +
> +What: /sys/class/leds/<led>/device/brightness_level
> +Date: July 2022
> +KernelVersion: 6.3
> +Contact: Pali Rohár <pali@kernel.org>
> +Description: (RW) Current brightness level value (0-7).
> +
> + Format: %u
> +
> +What: /sys/class/leds/<led>/device/brightness_values
brightness_level_values?
> +Date: July 2022
> +KernelVersion: 6.3
> +Contact: Pali Rohár <pali@kernel.org>
> +Description: (RW) Values of all 8 levels between which CPLD firmware cycles
> + when brightness button is pressed.
> +
> + Format: %u %u %u %u %u %u %u %u
One value per file please.
As Greg says, userspace should not have to 'parse' sysfs file output.
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 499d0f215a8b..6f78764e20aa 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -157,6 +157,15 @@ config LEDS_EL15203000
> To compile this driver as a module, choose M here: the module
> will be called leds-el15203000.
>
> +config LEDS_TURRIS_1X
> + tristate "LED support for CZ.NIC's Turris 1.x"
Worth specifying that this is a router here?
> + depends on LEDS_CLASS_MULTICOLOR
> + depends on OF
> + select LEDS_TRIGGERS
> + help
> + This option enables support for LEDs found on the front side of
> + CZ.NIC's Turris 1.x routers.
> +
> config LEDS_TURRIS_OMNIA
> tristate "LED support for CZ.NIC's Turris Omnia"
> depends on LEDS_CLASS_MULTICOLOR
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 4fd2f92cd198..de08083dbbca 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -82,6 +82,7 @@ obj-$(CONFIG_LEDS_TCA6507) += leds-tca6507.o
> obj-$(CONFIG_LEDS_TI_LMU_COMMON) += leds-ti-lmu-common.o
> obj-$(CONFIG_LEDS_TLC591XX) += leds-tlc591xx.o
> obj-$(CONFIG_LEDS_TPS6105X) += leds-tps6105x.o
> +obj-$(CONFIG_LEDS_TURRIS_1X) += leds-turris-1x.o
> obj-$(CONFIG_LEDS_TURRIS_OMNIA) += leds-turris-omnia.o
> obj-$(CONFIG_LEDS_WM831X_STATUS) += leds-wm831x-status.o
> obj-$(CONFIG_LEDS_WM8350) += leds-wm8350.o
> diff --git a/drivers/leds/leds-turris-1x.c b/drivers/leds/leds-turris-1x.c
> new file mode 100644
> index 000000000000..cf7567b64306
> --- /dev/null
> +++ b/drivers/leds/leds-turris-1x.c
> @@ -0,0 +1,474 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// (C) 2022 Pali Rohár <pali@kernel.org>
> +//
> +// CZ.NIC's Turris 1.x LEDs driver, controlled by CPLD firmware:
> +// https://gitlab.nic.cz/turris/hw/turris_cpld/-/blob/master/CZ_NIC_Router_CPLD.v
> +
> +#include <linux/i2c.h>
> +#include <linux/led-class-multicolor.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include "leds.h"
> +
> +/* LED registers starts at byte 0x13 in CPLD memory map */
> +#define TURRIS1X_LED_REG_OFF(reg) ((reg)-0x13)
Spaces around the "-" please.
Why not just give the registers their correct value?
Or are they already adjusted in the datasheet?
> +/* LEDs 1-5 share common register for setting brightness */
> +#define TURRIS1X_LED_BRIGHTNESS_OFF(idx) ({ const u8 _idx = (idx) & 7; \
> + (_idx == 0) ? 0 : \
> + (_idx <= 5) ? 1 : \
> + (_idx - 4); })
> +
> +#define TURRIS1X_LED_BRIGHTNESS_REG(idx, color) TURRIS1X_LED_REG_OFF(0x13 + \
> + 3 * TURRIS1X_LED_BRIGHTNESS_OFF(idx) + \
> + ((color) & 3))
These 2 MACROs are making my head hurt. Please find a cleaner way to
represent this logic, even at the expense of a few extra lines / defines.
> +#define TURRIS1X_LED_GLOBAL_LEVEL_REG TURRIS1X_LED_REG_OFF(0x20)
> +#define TURRIS1X_LED_GET_GLOBAL_BRIGHTNESS_REG TURRIS1X_LED_REG_OFF(0x21)
> +#define TURRIS1X_LED_SW_OVERRIDE_REG TURRIS1X_LED_REG_OFF(0x22)
> +#define TURRIS1X_LED_SW_DISABLE_REG TURRIS1X_LED_REG_OFF(0x23)
> +#define TURRIS1X_LED_GLOBAL_BRIGHTNESS_REG(lvl) TURRIS1X_LED_REG_OFF(0x28 + ((lvl) & 7))
> +
> +struct turris1x_led {
> + struct led_classdev_mc mc_cdev;
> + struct mc_subled subled_info[3];
Please define the '3' then use that definition throughout instead of
ARRAY_SIZE().
> + u32 reg;
> + bool registered;
> +};
> +
> +#define to_turris1x_led(l) container_of(l, struct turris1x_led, mc_cdev)
Since cdev is always available at the call-site, you can save yourself a
few lines and some in-function complexity with:
#define to_turris1x_led(cdev) container_of(lcdev_to_mccdev(cdev), struct turris1x_led, mc_cdev)
> +struct turris1x_leds {
> + void __iomem *regs;
> + struct mutex lock;
> + struct turris1x_led leds[8];
As above s/3/8/.
> +};
> +
> +static struct led_hw_trigger_type turris1x_hw_trigger_type;
Why do you need a global copy of this? What's preventing you from
allocating and freeing memory for it like we usually do?
> +static int turris1x_hwtrig_activate(struct led_classdev *cdev)
> +{
> + struct turris1x_leds *leds = dev_get_drvdata(cdev->dev->parent);
> + struct turris1x_led *led = to_turris1x_led(lcdev_to_mccdev(cdev));
> + u8 val;
> +
> + /* Disable software control of LED */
"Disable LED software control"
or
"of the LED"
Same in *_deactivate()
> + mutex_lock(&leds->lock);
> + val = readb(leds->regs + TURRIS1X_LED_SW_OVERRIDE_REG);
> + val &= ~BIT(led->reg);
> + writeb(val, leds->regs + TURRIS1X_LED_SW_OVERRIDE_REG);
> + mutex_unlock(&leds->lock);
> +
> + return 0;
> +}
> +
> +static void turris1x_hwtrig_deactivate(struct led_classdev *cdev)
> +{
> + struct turris1x_leds *leds = dev_get_drvdata(cdev->dev->parent);
> + struct turris1x_led *led = to_turris1x_led(lcdev_to_mccdev(cdev));
> + u8 val;
> +
> + /* Enable software control of LED */
> + mutex_lock(&leds->lock);
> + val = readb(leds->regs + TURRIS1X_LED_SW_OVERRIDE_REG);
> + val |= BIT(led->reg);
> + writeb(val, leds->regs + TURRIS1X_LED_SW_OVERRIDE_REG);
> + mutex_unlock(&leds->lock);
> +}
> +
> +static struct led_trigger turris1x_hw_trigger = {
> + .name = "turris1x-cpld",
> + .activate = turris1x_hwtrig_activate,
> + .deactivate = turris1x_hwtrig_deactivate,
> + .trigger_type = &turris1x_hw_trigger_type,
> +};
> +
> +static enum led_brightness turris1x_led_brightness_get(struct led_classdev *cdev)
> +{
> + struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
With the suggested change above, you can rid this line.
> + struct turris1x_leds *leds = dev_get_drvdata(cdev->dev->parent);
> + struct turris1x_led *led = to_turris1x_led(mc_cdev);
> +
> + if (!(readb(leds->regs + TURRIS1X_LED_SW_OVERRIDE_REG) & BIT(led->reg)))
> + return 1;
That's not an enum. That's an int.
"LED_ON"
> + else if (!(readb(leds->regs + TURRIS1X_LED_SW_DISABLE_REG) & BIT(led->reg)))
> + return 1;
> + else
> + return 0;
"LED_OFF"
> +}
> +
> +static void turris1x_led_brightness_set(struct led_classdev *cdev,
> + enum led_brightness brightness)
> +{
> + struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
> + struct turris1x_leds *leds = dev_get_drvdata(cdev->dev->parent);
> + struct turris1x_led *led = to_turris1x_led(mc_cdev);
> + int i, j;
> + u8 val;
> +
> + mutex_lock(&leds->lock);
> +
> + /* Set new brightness value for each color when LED is enabled */
> + if (brightness) {
if (!brightness)
goto skip_brightness;
> + led_mc_calc_color_components(mc_cdev, brightness);
'\n'
> + for (i = 0; i < ARRAY_SIZE(led->subled_info); i++)
> + writeb(mc_cdev->subled_info[i].brightness,
> + leds->regs + TURRIS1X_LED_BRIGHTNESS_REG(led->reg, i));
Seeing as you always add leds->regs to this, why not incorporate it into
the macro and save yourself these long lines that need to be wrapped.
Also consider using holding variables to achieve the same goal.
Think about the readability and maintainability of your code.
> +
> + /*
> + * LEDs 1-5 (LAN) share common color settings in same sets
> + * of HW registers and therefore it is not possible to set
> + * different colors. So when chaning color of one LED then
> + * reflect color change for all of them.
> + */
Spell check.
> + if (led->reg >= 1 && led->reg <= 5) {
> + for (j = 0; j < ARRAY_SIZE(leds->leds); j++) {
> + if (leds->leds[j].reg < 1 ||
> + leds->leds[j].reg > 5 ||
> + leds->leds[j].reg == led->reg)
> + continue;
'\n'
> + for (i = 0; i < ARRAY_SIZE(led->subled_info); i++)
> + leds->leds[j].mc_cdev.subled_info[i].intensity =
> + mc_cdev->subled_info[i].intensity;
What's the difference between the two 'mc_cdev's here?
> + }
> + }
> + }
> +
> + /* Enable / disable LED for software control */
> + val = readb(leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
> + if (brightness && (val & BIT(led->reg)))
> + writeb(val & ~BIT(led->reg),
> + leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
Single line.
> + else if (!brightness && !(val & BIT(led->reg)))
> + writeb(val | BIT(led->reg),
> + leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
Single line.
> + mutex_unlock(&leds->lock);
> +}
> +
> +static int turris1x_led_register(struct device *dev, struct turris1x_leds *leds,
> + struct device_node *np, u8 val_sw_override,
> + u8 val_sw_disable)
> +{
> + struct led_init_data init_data = {};
> + struct led_classdev *cdev;
> + struct turris1x_led *led;
> + int ret, color;
> + u32 reg;
> + int i;
> +
> + const unsigned int colors[ARRAY_SIZE(led->subled_info)] = {
> + LED_COLOR_ID_RED, LED_COLOR_ID_GREEN, LED_COLOR_ID_BLUE
> + };
> +
> + ret = of_property_read_u32(np, "reg", ®);
> + if (ret || reg >= ARRAY_SIZE(leds->leds)) {
> + dev_err(dev,
> + "Node %pOF: must contain 'reg' property with values between 0 and %u\n",
IMHO this is too verbose. This is a message for an engineer who will
know how to use grep and perform basic debugging.
"Invalid or missing 'reg' property" should clean this line up.
> + np, (unsigned int)ARRAY_SIZE(leds->leds) - 1);
> + return -EINVAL;
> + }
> +
> + ret = of_property_read_u32(np, "color", &color);
> + if (ret || color != LED_COLOR_ID_RGB) {
> + dev_err(dev,
> + "Node %pOF: must contain 'color' property with value LED_COLOR_ID_RGB\n",
As above. Keep it simple and brief.
> + np);
> + return -EINVAL;
> + }
> +
> + led = &leds->leds[reg];
> +
> + if (led->registered) {
> + dev_err(dev, "Node %pOF: duplicate 'reg' property %u\n",
> + np, reg);
"LED %d already registered", reg
Line wrap at 100 chars please.
> + return -EINVAL;
> + }
> +
> + led->registered = true;
> + led->reg = reg;
> +
> + /* Set initial colors to what are currently in use */
"to those currently"
> + for (i = 0; i < ARRAY_SIZE(led->subled_info); i++) {
> + led->subled_info[i].intensity =
> + readb(leds->regs + TURRIS1X_LED_BRIGHTNESS_REG(reg, i));
> + led->subled_info[i].color_index = colors[i];
> + led->subled_info[i].channel = i;
For my own understanding, what's the difference between reg and channel?
> + }
> +
> + led->mc_cdev.subled_info = led->subled_info;
Why do we need 2 copies of the same info?
> + led->mc_cdev.num_colors = ARRAY_SIZE(led->subled_info);
This is always 3 - use a define instead.
> + init_data.fwnode = &np->fwnode;
> +
> + cdev = &led->mc_cdev.led_cdev;
> + cdev->max_brightness = 1;
> + cdev->brightness_get = turris1x_led_brightness_get;
> + cdev->brightness_set = turris1x_led_brightness_set;
> +
> + /* All LEDs except LED 6 (WiFi) can be controller by hardware trigger */
> + if (reg != 6)
> + cdev->trigger_type = &turris1x_hw_trigger_type;
> +
> + /* Disable hardware trigger for LED 6 (WiFi) - allow software control */
> + if (reg == 6 && !(val_sw_override & BIT(6))) {
> + if (!(val_sw_disable & BIT(6))) {
Wouldn't it make things much easier if we just did it anyway (see below)?
> + val_sw_disable |= BIT(6);
> + writeb(val_sw_disable,
> + leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
Wrap at 100 everywhere please.
> + }
> + val_sw_override |= BIT(6);
> + writeb(val_sw_override,
> + leds->regs + TURRIS1X_LED_SW_OVERRIDE_REG);
> + }
if (reg == 6 && !(val_sw_override & BIT(6))) {
writeb(val_sw_disable, leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
writeb(val_sw_override, leds->regs + TURRIS1X_LED_SW_OVERRIDE_REG);
val_sw_disable |= BIT(6);
val_sw_override |= BIT(6);
}
> + if (!(val_sw_override & BIT(reg)))
> + cdev->default_trigger = turris1x_hw_trigger.name;
> +
> + if (!(val_sw_override & BIT(reg)) || !(val_sw_disable & BIT(reg)))
> + cdev->brightness = 1;
> +
> + ret = devm_led_classdev_multicolor_register_ext(dev, &led->mc_cdev,
> + &init_data);
> + if (ret) {
> + dev_err(dev, "Cannot register LED %pOF: %i\n", np, ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static ssize_t brightness_show(struct device *dev, struct device_attribute *a,
> + char *buf)
> +{
> + struct turris1x_leds *leds = dev_get_drvdata(dev);
> + unsigned int brightness;
> +
> + /*
> + * Current brightness value is available in read-only register
> + * TURRIS1X_LED_GET_GLOBAL_BRIGHTNESS_REG. Equivalent code is:
> + * level = readb(leds->regs + TURRIS1X_LED_GLOBAL_LEVEL_REG) & 7;
> + * brightness = readb(leds->regs + TURRIS1X_LED_GLOBAL_BRIGHTNESS_REG(level));
> + */
> + brightness = readb(leds->regs + TURRIS1X_LED_GET_GLOBAL_BRIGHTNESS_REG);
> +
> + return sprintf(buf, "%u\n", brightness);
> +}
> +
> +static ssize_t brightness_store(struct device *dev, struct device_attribute *a,
> + const char *buf, size_t count)
> +{
> + struct turris1x_leds *leds = dev_get_drvdata(dev);
> + int best_error, error, level, value;
> + unsigned long brightness;
> + u8 best_level;
> +
> + if (kstrtoul(buf, 10, &brightness))
> + return -EINVAL;
> +
> + if (brightness > 255)
> + return -EINVAL;
> +
> + /*
> + * Brightness can be set only to one of 8 predefined value levels
> + * available in TURRIS1X_LED_GLOBAL_BRIGHTNESS_REG(level) registers.
> + * Choose level which has nearest value to the specified brightness.
> + */
> + best_level = 0;
> + best_error = INT_MAX;
> + for (level = 0; level < 8; level++) {
> + value = readb(leds->regs +
> + TURRIS1X_LED_GLOBAL_BRIGHTNESS_REG(level));
> + error = abs(value - (int)brightness);
> + if (best_error > error) {
> + best_error = error;
> + best_level = level;
> + }
> + }
> +
> + writeb(best_level, leds->regs + TURRIS1X_LED_GLOBAL_LEVEL_REG);
> +
> + return count;
> +}
> +static DEVICE_ATTR_RW(brightness);
> +
> +static ssize_t brightness_level_show(struct device *dev,
> + struct device_attribute *a, char *buf)
> +{
> + struct turris1x_leds *leds = dev_get_drvdata(dev);
> + unsigned int level;
> +
> + level = readb(leds->regs + TURRIS1X_LED_GLOBAL_LEVEL_REG) & 7;
> +
> + return sprintf(buf, "%u\n", level);
> +}
> +
> +static ssize_t brightness_level_store(struct device *dev,
> + struct device_attribute *a,
> + const char *buf, size_t count)
> +{
> + struct turris1x_leds *leds = dev_get_drvdata(dev);
> + unsigned long level;
> +
> + if (kstrtoul(buf, 10, &level))
> + return -EINVAL;
> +
> + if (level > 7)
> + return -EINVAL;
> +
> + writeb(level, leds->regs + TURRIS1X_LED_GLOBAL_LEVEL_REG);
> +
> + return count;
> +}
> +static DEVICE_ATTR_RW(brightness_level);
> +
> +static ssize_t brightness_values_show(struct device *dev,
> + struct device_attribute *a, char *buf)
> +{
> + struct turris1x_leds *leds = dev_get_drvdata(dev);
> + unsigned int vals[8];
> + int i;
> +
> + for (i = 0; i < 8; i++)
> + vals[i] = readb(leds->regs +
> + TURRIS1X_LED_GLOBAL_BRIGHTNESS_REG(i));
> +
> + return sprintf(buf, "%u %u %u %u %u %u %u %u\n", vals[0], vals[1],
> + vals[2], vals[3], vals[4], vals[5], vals[6], vals[7]);
> +}
> +
> +static ssize_t brightness_values_store(struct device *dev,
> + struct device_attribute *a,
> + const char *buf, size_t count)
> +{
> + struct turris1x_leds *leds = dev_get_drvdata(dev);
> + unsigned int vals[8];
> + int nchars;
> + int i;
> +
> + if (sscanf(buf, "%u %u %u %u %u %u %u %u%n", &vals[0], &vals[1],
> + &vals[2], &vals[3], &vals[4], &vals[5], &vals[6], &vals[7],
> + &nchars) != 8 || nchars + 1 < count)
> + return -EINVAL;
> +
> + for (i = 0; i < 8; i++)
> + writeb(vals[i],
> + leds->regs + TURRIS1X_LED_GLOBAL_BRIGHTNESS_REG(i));
> +
> + return count;
> +}
> +static DEVICE_ATTR_RW(brightness_values);
> +
> +static struct attribute *turris1x_leds_controller_attrs[] = {
> + &dev_attr_brightness.attr,
> + &dev_attr_brightness_level.attr,
> + &dev_attr_brightness_values.attr,
> + NULL,
> +};
> +ATTRIBUTE_GROUPS(turris1x_leds_controller);
> +
> +static int turris1x_leds_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *np = dev_of_node(dev);
> + struct device_node *child;
> + struct turris1x_leds *leds;
> + struct resource *res;
> + void __iomem *regs;
> + u8 val_sw_override;
> + u8 val_sw_disable;
> + int ret;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -ENODEV;
> +
> + regs = devm_ioremap_resource(dev, res);
> + if (IS_ERR(regs))
> + return PTR_ERR(regs);
devm_platform_get_and_ioremap_resource()
> + leds = devm_kzalloc(dev, sizeof(*leds), GFP_KERNEL);
Call this ddata everywhere.
leds->leds is not overly forthcoming.
> + if (!leds)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, leds);
> +
> + leds->regs = regs;
> + mutex_init(&leds->lock);
> +
> + ret = devm_led_trigger_register(dev, &turris1x_hw_trigger);
> + if (ret) {
> + dev_err(dev, "Cannot register private LED trigger: %d\n", ret);
> + return ret;
> + }
> +
> + val_sw_override = readb(leds->regs + TURRIS1X_LED_SW_OVERRIDE_REG);
> + val_sw_disable = readb(leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
> +
> + for_each_available_child_of_node(np, child) {
> + ret = turris1x_led_register(dev, leds, child,
> + val_sw_override, val_sw_disable);
> + if (ret) {
> + of_node_put(child);
> + return ret;
> + }
> + }
> +
> + ret = devm_device_add_groups(dev, turris1x_leds_controller_groups);
> + if (ret) {
> + dev_err(dev, "Could not add attribute group!\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static void turris1x_leds_shutdown(struct platform_device *pdev)
> +{
> + struct turris1x_leds *leds = platform_get_drvdata(pdev);
> + int i, j;
> + u8 val;
> +
> + /*
> + * LED registers are persisent across board resets.
> + * So reset LEDs to default state before kernel reboots.
> + */
Do you really want to rely on the process before us to have done the
same? Wouldn't it be better to reset on boot-up rather than shutdown?
> + /* Disable software control of all LEDs except LED 6 (WiFi) */
> + writeb(BIT(6), leds->regs + TURRIS1X_LED_SW_OVERRIDE_REG);
> +
> + /* Turn off LED 6 (WiFi) as there is no hardware trigger for it */
> + val = readb(leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
> + writeb(val | BIT(6), leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
> +
> + /* Reset colors of all LEDs to default values */
> + for (i = 0; i < ARRAY_SIZE(leds->leds); i++) {
> + /* Skip LAN2-LAN5 LEDs which share color register with LAN1 */
> + if (i >= 2 && i <= 5)
> + continue;
> + for (j = 0; j < ARRAY_SIZE(leds->leds[i].subled_info); j++)
> + writeb(0xff,
> + leds->regs + TURRIS1X_LED_BRIGHTNESS_REG(i, j));
> + }
> +}
> +
> +static const struct of_device_id of_turris1x_leds_match[] = {
> + { .compatible = "cznic,turris1x-leds" },
> + {},
> +};
> +
> +static struct platform_driver turris1x_leds_driver = {
> + .probe = turris1x_leds_probe,
> + .shutdown = turris1x_leds_shutdown,
> + .driver = {
> + .name = "turris1x_leds",
> + .of_match_table = of_turris1x_leds_match,
> + },
> +};
> +module_platform_driver(turris1x_leds_driver);
> +
> +MODULE_AUTHOR("Pali Rohár <pali@kernel.org>");
> +MODULE_DESCRIPTION("CZ.NIC's Turris 1.x LEDs");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:turris1x_leds");
> --
> 2.20.1
>
--
Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH RESEND 6/8] leds: turris-omnia: support HW controlled mode via private trigger
2022-12-26 12:36 ` [PATCH RESEND 6/8] leds: turris-omnia: support HW controlled mode via private trigger Pali Rohár
@ 2023-02-24 9:32 ` Lee Jones
0 siblings, 0 replies; 42+ messages in thread
From: Lee Jones @ 2023-02-24 9:32 UTC (permalink / raw)
To: Pali Rohár; +Cc: Arnd Bergmann, Linus Walleij, soc, linux-kernel
On Mon, 26 Dec 2022, Pali Rohár wrote:
> From: Marek Behún <kabel@kernel.org>
>
> Add support for enabling MCU controlled mode of the Turris Omnia LEDs
> via a LED private trigger called "omnia-mcu".
>
> When in MCU controlled mode, the user can still set LED color, but the
> blinking is done by MCU, which does different things for various LEDs:
> - WAN LED is blinked according to the LED[0] pin of the WAN PHY
> - LAN LEDs are blinked according to the LED[0] output of corresponding
> port of the LAN switch
> - PCIe LEDs are blinked according to the logical OR of the MiniPCIe port
> LED pins
>
> For a long time I wanted to actually do this differently: I wanted to
> make the netdev trigger to transparently offload the blinking to the HW
> if user set compatible settings for the netdev trigger.
> There was some work on this, and hopefully we will be able to complete
> it sometime, but since there are various complications, it will probably
> not be soon.
>
> In the meantime let's support HW controlled mode via this private LED
> trigger. If, in the future, we manage to complete the netdev trigger
> offloading, we can still keep this private trigger for backwards
> compatibility, if needed.
>
> We also set "omnia-mcu" to cdev->default_trigger, so that the MCU keeps
> control until the user first wants to take over it. If a different
> default trigger is specified in device-tree via the
> `linux,default-trigger` property, LED class will overwrite
> cdev->default_trigger, and so the DT property will be respected.
>
> Signed-off-by: Marek Behún <kabel@kernel.org>
> Fixes: 40624346b7ae ("ARM: dts: turris-omnia: enable LED controller node")
> Reviewed-by: Pali Rohár <pali@kernel.org>
> ---
> drivers/leds/Kconfig | 1 +
> drivers/leds/leds-turris-omnia.c | 41 ++++++++++++++++++++++++++++++++
> 2 files changed, 42 insertions(+)
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 6f78764e20aa..f957b86411de 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -172,6 +172,7 @@ config LEDS_TURRIS_OMNIA
> depends on I2C
> depends on MACH_ARMADA_38X || COMPILE_TEST
> depends on OF
> + select LEDS_TRIGGERS
> help
> This option enables basic support for the LEDs found on the front
> side of CZ.NIC's Turris Omnia router. There are 12 RGB LEDs on the
> diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c
> index c7c9851c894a..bb7af9e59ad6 100644
> --- a/drivers/leds/leds-turris-omnia.c
> +++ b/drivers/leds/leds-turris-omnia.c
> @@ -41,6 +41,39 @@ struct omnia_leds {
> struct omnia_led leds[];
> };
>
> +static struct led_hw_trigger_type omnia_hw_trigger_type;
Really not a fan of anything global if it can be worked around.
> +static int omnia_hwtrig_activate(struct led_classdev *cdev)
> +{
> + struct omnia_leds *leds = dev_get_drvdata(cdev->dev->parent);
Out of interest, who is the parent?
> + struct omnia_led *led = to_omnia_led(lcdev_to_mccdev(cdev));
> +
> + /* put the LED into MCU controlled mode */
> + return i2c_smbus_write_byte_data(leds->client, CMD_LED_MODE,
> + CMD_LED_MODE_LED(led->reg));
Wrap at 100 chars everywhere.
> +}
> +
> +static void omnia_hwtrig_deactivate(struct led_classdev *cdev)
> +{
> + struct omnia_leds *leds = dev_get_drvdata(cdev->dev->parent);
> + struct omnia_led *led = to_omnia_led(lcdev_to_mccdev(cdev));
Place lcdev_to_mccdev() in to_omnia_led().
> + int ret;
> +
> + /* put the LED into software mode */
"Put"
> + ret = i2c_smbus_write_byte_data(leds->client, CMD_LED_MODE,
> + CMD_LED_MODE_LED(led->reg) |
> + CMD_LED_MODE_USER);
> + if (ret < 0)
> + dev_err(cdev->dev, "Cannot put to software mode: %i\n", ret);
> +}
> +
> +static struct led_trigger omnia_hw_trigger = {
> + .name = "omnia-mcu",
> + .activate = omnia_hwtrig_activate,
> + .deactivate = omnia_hwtrig_deactivate,
> + .trigger_type = &omnia_hw_trigger_type,
> +};
> +
> static int omnia_led_brightness_set_blocking(struct led_classdev *cdev,
> enum led_brightness brightness)
> {
> @@ -112,6 +145,8 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
> cdev = &led->mc_cdev.led_cdev;
> cdev->max_brightness = 255;
> cdev->brightness_set_blocking = omnia_led_brightness_set_blocking;
> + cdev->trigger_type = &omnia_hw_trigger_type;
> + cdev->default_trigger = omnia_hw_trigger.name;
>
> /* put the LED into software mode */
> ret = i2c_smbus_write_byte_data(client, CMD_LED_MODE,
> @@ -228,6 +263,12 @@ static int omnia_leds_probe(struct i2c_client *client,
>
> mutex_init(&leds->lock);
>
> + ret = devm_led_trigger_register(dev, &omnia_hw_trigger);
> + if (ret < 0) {
> + dev_err(dev, "Cannot register private LED trigger: %d\n", ret);
> + return ret;
> + }
> +
> led = &leds->leds[0];
> for_each_available_child_of_node(np, child) {
> ret = omnia_led_register(client, led, child);
> --
> 2.20.1
>
--
Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH RESEND 7/8] leds: turris-omnia: initialize multi-intensity to full
2022-12-26 12:36 ` [PATCH RESEND 7/8] leds: turris-omnia: initialize multi-intensity to full Pali Rohár
@ 2023-02-24 9:33 ` Lee Jones
0 siblings, 0 replies; 42+ messages in thread
From: Lee Jones @ 2023-02-24 9:33 UTC (permalink / raw)
To: Pali Rohár; +Cc: Arnd Bergmann, Linus Walleij, soc, linux-kernel
On Mon, 26 Dec 2022, Pali Rohár wrote:
> From: Marek Behún <kabel@kernel.org>
>
> The default color of each LED before driver probe (255, 255, 255).
> Initialize multi_intensity to this value, so that it corresponds to the
> reality.
>
> Signed-off-by: Marek Behún <kabel@kernel.org>
> Fixes: 089381b27abe ("leds: initial support for Turris Omnia LEDs")
> Reviewed-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Lee Jones <lee@kernel.org>
> ---
> drivers/leds/leds-turris-omnia.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c
> index bb7af9e59ad6..71340dec492a 100644
> --- a/drivers/leds/leds-turris-omnia.c
> +++ b/drivers/leds/leds-turris-omnia.c
> @@ -131,10 +131,13 @@ 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].intensity = 255;
> led->subled_info[0].channel = 0;
> led->subled_info[1].color_index = LED_COLOR_ID_GREEN;
> + led->subled_info[1].intensity = 255;
> led->subled_info[1].channel = 1;
> led->subled_info[2].color_index = LED_COLOR_ID_BLUE;
> + led->subled_info[2].intensity = 255;
> led->subled_info[2].channel = 2;
>
> led->mc_cdev.subled_info = led->subled_info;
> --
> 2.20.1
>
--
Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH RESEND 8/8] leds: turris-omnia: change max brightness from 255 to 1
2022-12-26 12:36 ` [PATCH RESEND 8/8] leds: turris-omnia: change max brightness from 255 to 1 Pali Rohár
@ 2023-02-24 9:34 ` Lee Jones
2023-03-09 20:07 ` Pali Rohár
0 siblings, 1 reply; 42+ messages in thread
From: Lee Jones @ 2023-02-24 9:34 UTC (permalink / raw)
To: Pali Rohár; +Cc: Arnd Bergmann, Linus Walleij, soc, linux-kernel
On Mon, 26 Dec 2022, Pali Rohár wrote:
> From: Marek Behún <kabel@kernel.org>
>
> Using binary brightness makes more sense for this controller, because
> internally in the MCU it works that way: the LED has a color, and a
> state whether it is ON or OFF.
>
> The resulting brightness computation with led_mc_calc_color_components()
> will now always result in either (0, 0, 0) or the multi_intensity value.
>
> Signed-off-by: Marek Behún <kabel@kernel.org>
> Fixes: 089381b27abe ("leds: initial support for Turris Omnia LEDs")
> Reviewed-by: Pali Rohár <pali@kernel.org>
> ---
> drivers/leds/leds-turris-omnia.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c
> index 71340dec492a..04f01ae46c27 100644
> --- a/drivers/leds/leds-turris-omnia.c
> +++ b/drivers/leds/leds-turris-omnia.c
> @@ -146,7 +146,7 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
> init_data.fwnode = &np->fwnode;
>
> cdev = &led->mc_cdev.led_cdev;
> - cdev->max_brightness = 255;
> + cdev->max_brightness = 1;
Should this now be LED_ON?
> cdev->brightness_set_blocking = omnia_led_brightness_set_blocking;
> cdev->trigger_type = &omnia_hw_trigger_type;
> cdev->default_trigger = omnia_hw_trigger.name;
> --
> 2.20.1
>
--
Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH RESEND 5/8] leds: Add support for Turris 1.x LEDs
2023-02-24 9:25 ` Krzysztof Kozlowski
@ 2023-02-24 9:37 ` Lee Jones
0 siblings, 0 replies; 42+ messages in thread
From: Lee Jones @ 2023-02-24 9:37 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Pali Rohár, pavel, Arnd Bergmann, Linus Walleij, soc, linux-kernel
On Fri, 24 Feb 2023, Krzysztof Kozlowski wrote:
> On 27/01/2023 12:20, Lee Jones wrote:
> > Pavel, are you happy with the user-space interface?
> >
>
> Although not about user-space but about code - entire review was ignored
> by Pali and not implemented:
> https://lore.kernel.org/all/CAHp75Vcr6o2rm+T6Tr8sS4VXCLVHtmLPWy-njOKAvO4AcZoW=A@mail.gmail.com/
I just submitted a review too. Lots to work on.
--
Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH RESEND 4/8] dt-bindings: leds: Add cznic,turris1x-leds.yaml binding
2023-02-24 9:15 ` Krzysztof Kozlowski
@ 2023-02-24 9:38 ` Lee Jones
2023-03-09 20:42 ` Pali Rohár
0 siblings, 1 reply; 42+ messages in thread
From: Lee Jones @ 2023-02-24 9:38 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Pali Rohár, Arnd Bergmann, Linus Walleij, soc, linux-kernel,
devicetree
On Fri, 24 Feb 2023, Krzysztof Kozlowski wrote:
> On 27/01/2023 12:16, Lee Jones wrote:
> > On Mon, 26 Dec 2022, Pali Rohár wrote:
> >
> >> Add device-tree bindings documentation for Turris 1.x RGB LEDs.
> >>
> >> Signed-off-by: Pali Rohár <pali@kernel.org>
> >> ---
> >> .../bindings/leds/cznic,turris1x-leds.yaml | 118 ++++++++++++++++++
> >> 1 file changed, 118 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/leds/cznic,turris1x-leds.yaml
> >
> > Needs a DT Ack (now Cc:ed)
>
> Within the same day of posting v2 of this - just after 4 hours, Pali
> received review and I pointed issue to address.
>
> The issue was not addressed, just ignored.
>
> Therefore this patch shall not be taken, until the issues are resolved
> or discussion is finished if there is disagreement about my comments..
Understood. Thanks for the clarification.
--
Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH RESEND 8/8] leds: turris-omnia: change max brightness from 255 to 1
2023-02-24 9:34 ` Lee Jones
@ 2023-03-09 20:07 ` Pali Rohár
0 siblings, 0 replies; 42+ messages in thread
From: Pali Rohár @ 2023-03-09 20:07 UTC (permalink / raw)
To: Lee Jones; +Cc: Arnd Bergmann, Linus Walleij, soc, linux-kernel
On Friday 24 February 2023 09:34:11 Lee Jones wrote:
> On Mon, 26 Dec 2022, Pali Rohár wrote:
>
> > From: Marek Behún <kabel@kernel.org>
> >
> > Using binary brightness makes more sense for this controller, because
> > internally in the MCU it works that way: the LED has a color, and a
> > state whether it is ON or OFF.
> >
> > The resulting brightness computation with led_mc_calc_color_components()
> > will now always result in either (0, 0, 0) or the multi_intensity value.
> >
> > Signed-off-by: Marek Behún <kabel@kernel.org>
> > Fixes: 089381b27abe ("leds: initial support for Turris Omnia LEDs")
> > Reviewed-by: Pali Rohár <pali@kernel.org>
> > ---
> > drivers/leds/leds-turris-omnia.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c
> > index 71340dec492a..04f01ae46c27 100644
> > --- a/drivers/leds/leds-turris-omnia.c
> > +++ b/drivers/leds/leds-turris-omnia.c
> > @@ -146,7 +146,7 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
> > init_data.fwnode = &np->fwnode;
> >
> > cdev = &led->mc_cdev.led_cdev;
> > - cdev->max_brightness = 255;
> > + cdev->max_brightness = 1;
>
> Should this now be LED_ON?
Somebody said that LED_ON is deprecated.
> > cdev->brightness_set_blocking = omnia_led_brightness_set_blocking;
> > cdev->trigger_type = &omnia_hw_trigger_type;
> > cdev->default_trigger = omnia_hw_trigger.name;
> > --
> > 2.20.1
> >
>
> --
> Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH RESEND 5/8] leds: Add support for Turris 1.x LEDs
2023-02-24 9:28 ` Lee Jones
@ 2023-03-09 20:35 ` Pali Rohár
0 siblings, 0 replies; 42+ messages in thread
From: Pali Rohár @ 2023-03-09 20:35 UTC (permalink / raw)
To: Lee Jones; +Cc: Arnd Bergmann, Linus Walleij, soc, linux-kernel
On Friday 24 February 2023 09:28:54 Lee Jones wrote:
> On Mon, 26 Dec 2022, Pali Rohár wrote:
>
> > This adds support for the RGB LEDs found on the front panel of the
> > Turris 1.x routers. There are 8 RGB LEDs that are controlled by
> > CZ.NIC CPLD firmware running on Lattice FPGA.
> >
> > CPLD firmware provides HW triggering mode for all LEDs except WiFi LED
> > which is automatically enabled after power on reset. LAN LEDs share HW
> > registers for RGB colors settings, so it is not possible to set different
> > colors for individual LAN LEDs.
> >
> > CZ.NIC CPLD firmware is open source and available at:
> > https://gitlab.nic.cz/turris/hw/turris_cpld/-/blob/master/CZ_NIC_Router_CPLD.v
> >
> > This driver uses the multicolor LED framework and HW led triggers.
> >
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Reviewed-by: Marek Behún <kabel@kernel.org>
> > ---
> > .../testing/sysfs-class-led-driver-turris1x | 31 ++
> > drivers/leds/Kconfig | 9 +
> > drivers/leds/Makefile | 1 +
> > drivers/leds/leds-turris-1x.c | 474 ++++++++++++++++++
> > 4 files changed, 515 insertions(+)
> > create mode 100644 Documentation/ABI/testing/sysfs-class-led-driver-turris1x
> > create mode 100644 drivers/leds/leds-turris-1x.c
> >
> > diff --git a/Documentation/ABI/testing/sysfs-class-led-driver-turris1x b/Documentation/ABI/testing/sysfs-class-led-driver-turris1x
> > new file mode 100644
> > index 000000000000..272695ae400b
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-class-led-driver-turris1x
> > @@ -0,0 +1,31 @@
> > +What: /sys/class/leds/<led>/device/brightness
> > +Date: July 2022
> > +KernelVersion: 6.3
>
> Date doesn't match the kernel version.
>
> > +Contact: Pali Rohár <pali@kernel.org>
> > +Description: (RW) On the back size of the Turris 1.x routers there is also
>
> "side"
>
> Drop "also"
>
> > + a button which can be used to control the intensity of all the
> > + LEDs at once, so that if they are too bright, user can dim them.
>
> "the user"
>
> > + The CPLD firmware cycles between 8 levels of this global
>
> Drop "this"
>
> > + brightness, but this setting can have any integer value between
> > + 0 and 255. It is therefore convenient to be able to change this
> > + setting from software.
> > +
> > + Format: %u
> > +
> > +What: /sys/class/leds/<led>/device/brightness_level
> > +Date: July 2022
> > +KernelVersion: 6.3
> > +Contact: Pali Rohár <pali@kernel.org>
> > +Description: (RW) Current brightness level value (0-7).
> > +
> > + Format: %u
> > +
> > +What: /sys/class/leds/<led>/device/brightness_values
>
> brightness_level_values?
>
> > +Date: July 2022
> > +KernelVersion: 6.3
> > +Contact: Pali Rohár <pali@kernel.org>
> > +Description: (RW) Values of all 8 levels between which CPLD firmware cycles
> > + when brightness button is pressed.
> > +
> > + Format: %u %u %u %u %u %u %u %u
>
> One value per file please.
>
> As Greg says, userspace should not have to 'parse' sysfs file output.
>
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > index 499d0f215a8b..6f78764e20aa 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -157,6 +157,15 @@ config LEDS_EL15203000
> > To compile this driver as a module, choose M here: the module
> > will be called leds-el15203000.
> >
> > +config LEDS_TURRIS_1X
> > + tristate "LED support for CZ.NIC's Turris 1.x"
>
> Worth specifying that this is a router here?
>
> > + depends on LEDS_CLASS_MULTICOLOR
> > + depends on OF
> > + select LEDS_TRIGGERS
> > + help
> > + This option enables support for LEDs found on the front side of
> > + CZ.NIC's Turris 1.x routers.
> > +
> > config LEDS_TURRIS_OMNIA
> > tristate "LED support for CZ.NIC's Turris Omnia"
> > depends on LEDS_CLASS_MULTICOLOR
> > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> > index 4fd2f92cd198..de08083dbbca 100644
> > --- a/drivers/leds/Makefile
> > +++ b/drivers/leds/Makefile
> > @@ -82,6 +82,7 @@ obj-$(CONFIG_LEDS_TCA6507) += leds-tca6507.o
> > obj-$(CONFIG_LEDS_TI_LMU_COMMON) += leds-ti-lmu-common.o
> > obj-$(CONFIG_LEDS_TLC591XX) += leds-tlc591xx.o
> > obj-$(CONFIG_LEDS_TPS6105X) += leds-tps6105x.o
> > +obj-$(CONFIG_LEDS_TURRIS_1X) += leds-turris-1x.o
> > obj-$(CONFIG_LEDS_TURRIS_OMNIA) += leds-turris-omnia.o
> > obj-$(CONFIG_LEDS_WM831X_STATUS) += leds-wm831x-status.o
> > obj-$(CONFIG_LEDS_WM8350) += leds-wm8350.o
> > diff --git a/drivers/leds/leds-turris-1x.c b/drivers/leds/leds-turris-1x.c
> > new file mode 100644
> > index 000000000000..cf7567b64306
> > --- /dev/null
> > +++ b/drivers/leds/leds-turris-1x.c
> > @@ -0,0 +1,474 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// (C) 2022 Pali Rohár <pali@kernel.org>
> > +//
> > +// CZ.NIC's Turris 1.x LEDs driver, controlled by CPLD firmware:
> > +// https://gitlab.nic.cz/turris/hw/turris_cpld/-/blob/master/CZ_NIC_Router_CPLD.v
> > +
> > +#include <linux/i2c.h>
> > +#include <linux/led-class-multicolor.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include "leds.h"
> > +
> > +/* LED registers starts at byte 0x13 in CPLD memory map */
> > +#define TURRIS1X_LED_REG_OFF(reg) ((reg)-0x13)
>
> Spaces around the "-" please.
>
> Why not just give the registers their correct value?
>
> Or are they already adjusted in the datasheet?
Yes, they are adjusted to firmware source code (and also to available
documentation written from the source code).
> > +/* LEDs 1-5 share common register for setting brightness */
> > +#define TURRIS1X_LED_BRIGHTNESS_OFF(idx) ({ const u8 _idx = (idx) & 7; \
> > + (_idx == 0) ? 0 : \
> > + (_idx <= 5) ? 1 : \
> > + (_idx - 4); })
> > +
> > +#define TURRIS1X_LED_BRIGHTNESS_REG(idx, color) TURRIS1X_LED_REG_OFF(0x13 + \
> > + 3 * TURRIS1X_LED_BRIGHTNESS_OFF(idx) + \
> > + ((color) & 3))
>
> These 2 MACROs are making my head hurt. Please find a cleaner way to
> represent this logic, even at the expense of a few extra lines / defines.
I'm not sure what is the cleaner way. For me the cleaner way is to put
it on one line without breaking it.
> > +#define TURRIS1X_LED_GLOBAL_LEVEL_REG TURRIS1X_LED_REG_OFF(0x20)
> > +#define TURRIS1X_LED_GET_GLOBAL_BRIGHTNESS_REG TURRIS1X_LED_REG_OFF(0x21)
> > +#define TURRIS1X_LED_SW_OVERRIDE_REG TURRIS1X_LED_REG_OFF(0x22)
> > +#define TURRIS1X_LED_SW_DISABLE_REG TURRIS1X_LED_REG_OFF(0x23)
> > +#define TURRIS1X_LED_GLOBAL_BRIGHTNESS_REG(lvl) TURRIS1X_LED_REG_OFF(0x28 + ((lvl) & 7))
> > +
> > +struct turris1x_led {
> > + struct led_classdev_mc mc_cdev;
> > + struct mc_subled subled_info[3];
>
> Please define the '3' then use that definition throughout instead of
> ARRAY_SIZE().
>
> > + u32 reg;
> > + bool registered;
> > +};
> > +
> > +#define to_turris1x_led(l) container_of(l, struct turris1x_led, mc_cdev)
>
> Since cdev is always available at the call-site, you can save yourself a
> few lines and some in-function complexity with:
>
> #define to_turris1x_led(cdev) container_of(lcdev_to_mccdev(cdev), struct turris1x_led, mc_cdev)
>
> > +struct turris1x_leds {
> > + void __iomem *regs;
> > + struct mutex lock;
> > + struct turris1x_led leds[8];
>
> As above s/3/8/.
>
> > +};
> > +
> > +static struct led_hw_trigger_type turris1x_hw_trigger_type;
>
> Why do you need a global copy of this? What's preventing you from
> allocating and freeing memory for it like we usually do?
This is how other drivers implemented triggers.
> > +static int turris1x_hwtrig_activate(struct led_classdev *cdev)
> > +{
> > + struct turris1x_leds *leds = dev_get_drvdata(cdev->dev->parent);
> > + struct turris1x_led *led = to_turris1x_led(lcdev_to_mccdev(cdev));
> > + u8 val;
> > +
> > + /* Disable software control of LED */
>
> "Disable LED software control"
>
> or
>
> "of the LED"
>
> Same in *_deactivate()
>
> > + mutex_lock(&leds->lock);
> > + val = readb(leds->regs + TURRIS1X_LED_SW_OVERRIDE_REG);
> > + val &= ~BIT(led->reg);
> > + writeb(val, leds->regs + TURRIS1X_LED_SW_OVERRIDE_REG);
> > + mutex_unlock(&leds->lock);
> > +
> > + return 0;
> > +}
> > +
> > +static void turris1x_hwtrig_deactivate(struct led_classdev *cdev)
> > +{
> > + struct turris1x_leds *leds = dev_get_drvdata(cdev->dev->parent);
> > + struct turris1x_led *led = to_turris1x_led(lcdev_to_mccdev(cdev));
> > + u8 val;
> > +
> > + /* Enable software control of LED */
> > + mutex_lock(&leds->lock);
> > + val = readb(leds->regs + TURRIS1X_LED_SW_OVERRIDE_REG);
> > + val |= BIT(led->reg);
> > + writeb(val, leds->regs + TURRIS1X_LED_SW_OVERRIDE_REG);
> > + mutex_unlock(&leds->lock);
> > +}
> > +
> > +static struct led_trigger turris1x_hw_trigger = {
> > + .name = "turris1x-cpld",
> > + .activate = turris1x_hwtrig_activate,
> > + .deactivate = turris1x_hwtrig_deactivate,
> > + .trigger_type = &turris1x_hw_trigger_type,
> > +};
> > +
> > +static enum led_brightness turris1x_led_brightness_get(struct led_classdev *cdev)
> > +{
> > + struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
>
> With the suggested change above, you can rid this line.
>
> > + struct turris1x_leds *leds = dev_get_drvdata(cdev->dev->parent);
> > + struct turris1x_led *led = to_turris1x_led(mc_cdev);
> > +
> > + if (!(readb(leds->regs + TURRIS1X_LED_SW_OVERRIDE_REG) & BIT(led->reg)))
> > + return 1;
>
> That's not an enum. That's an int.
>
> "LED_ON"
In was there before but during review I was told to change LED_ON to 1.
So it should be changed back to LED_ON?
> > + else if (!(readb(leds->regs + TURRIS1X_LED_SW_DISABLE_REG) & BIT(led->reg)))
> > + return 1;
> > + else
> > + return 0;
>
> "LED_OFF"
>
> > +}
> > +
> > +static void turris1x_led_brightness_set(struct led_classdev *cdev,
> > + enum led_brightness brightness)
> > +{
> > + struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
> > + struct turris1x_leds *leds = dev_get_drvdata(cdev->dev->parent);
> > + struct turris1x_led *led = to_turris1x_led(mc_cdev);
> > + int i, j;
> > + u8 val;
> > +
> > + mutex_lock(&leds->lock);
> > +
> > + /* Set new brightness value for each color when LED is enabled */
> > + if (brightness) {
>
> if (!brightness)
> goto skip_brightness;
>
> > + led_mc_calc_color_components(mc_cdev, brightness);
>
> '\n'
>
> > + for (i = 0; i < ARRAY_SIZE(led->subled_info); i++)
> > + writeb(mc_cdev->subled_info[i].brightness,
> > + leds->regs + TURRIS1X_LED_BRIGHTNESS_REG(led->reg, i));
>
> Seeing as you always add leds->regs to this, why not incorporate it into
> the macro and save yourself these long lines that need to be wrapped.
>
> Also consider using holding variables to achieve the same goal.
>
> Think about the readability and maintainability of your code.
>
> > +
> > + /*
> > + * LEDs 1-5 (LAN) share common color settings in same sets
> > + * of HW registers and therefore it is not possible to set
> > + * different colors. So when chaning color of one LED then
> > + * reflect color change for all of them.
> > + */
>
> Spell check.
>
> > + if (led->reg >= 1 && led->reg <= 5) {
> > + for (j = 0; j < ARRAY_SIZE(leds->leds); j++) {
> > + if (leds->leds[j].reg < 1 ||
> > + leds->leds[j].reg > 5 ||
> > + leds->leds[j].reg == led->reg)
> > + continue;
>
> '\n'
>
> > + for (i = 0; i < ARRAY_SIZE(led->subled_info); i++)
> > + leds->leds[j].mc_cdev.subled_info[i].intensity =
> > + mc_cdev->subled_info[i].intensity;
>
> What's the difference between the two 'mc_cdev's here?
>
> > + }
> > + }
> > + }
> > +
> > + /* Enable / disable LED for software control */
> > + val = readb(leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
> > + if (brightness && (val & BIT(led->reg)))
> > + writeb(val & ~BIT(led->reg),
> > + leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
>
> Single line.
>
> > + else if (!brightness && !(val & BIT(led->reg)))
> > + writeb(val | BIT(led->reg),
> > + leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
>
> Single line.
>
> > + mutex_unlock(&leds->lock);
> > +}
> > +
> > +static int turris1x_led_register(struct device *dev, struct turris1x_leds *leds,
> > + struct device_node *np, u8 val_sw_override,
> > + u8 val_sw_disable)
> > +{
> > + struct led_init_data init_data = {};
> > + struct led_classdev *cdev;
> > + struct turris1x_led *led;
> > + int ret, color;
> > + u32 reg;
> > + int i;
> > +
> > + const unsigned int colors[ARRAY_SIZE(led->subled_info)] = {
> > + LED_COLOR_ID_RED, LED_COLOR_ID_GREEN, LED_COLOR_ID_BLUE
> > + };
> > +
> > + ret = of_property_read_u32(np, "reg", ®);
> > + if (ret || reg >= ARRAY_SIZE(leds->leds)) {
> > + dev_err(dev,
> > + "Node %pOF: must contain 'reg' property with values between 0 and %u\n",
>
> IMHO this is too verbose. This is a message for an engineer who will
> know how to use grep and perform basic debugging.
>
> "Invalid or missing 'reg' property" should clean this line up.
>
> > + np, (unsigned int)ARRAY_SIZE(leds->leds) - 1);
> > + return -EINVAL;
> > + }
> > +
> > + ret = of_property_read_u32(np, "color", &color);
> > + if (ret || color != LED_COLOR_ID_RGB) {
> > + dev_err(dev,
> > + "Node %pOF: must contain 'color' property with value LED_COLOR_ID_RGB\n",
>
> As above. Keep it simple and brief.
>
> > + np);
> > + return -EINVAL;
> > + }
> > +
> > + led = &leds->leds[reg];
> > +
> > + if (led->registered) {
> > + dev_err(dev, "Node %pOF: duplicate 'reg' property %u\n",
> > + np, reg);
>
> "LED %d already registered", reg
>
> Line wrap at 100 chars please.
>
> > + return -EINVAL;
> > + }
> > +
> > + led->registered = true;
> > + led->reg = reg;
> > +
> > + /* Set initial colors to what are currently in use */
>
> "to those currently"
>
> > + for (i = 0; i < ARRAY_SIZE(led->subled_info); i++) {
> > + led->subled_info[i].intensity =
> > + readb(leds->regs + TURRIS1X_LED_BRIGHTNESS_REG(reg, i));
> > + led->subled_info[i].color_index = colors[i];
> > + led->subled_info[i].channel = i;
>
> For my own understanding, what's the difference between reg and channel?
reg is the register which represents particular "LED" as seen by the
user on the box and channel is color of that LED. Every "LED" has
3 colors which can be configured separately.
> > + }
> > +
> > + led->mc_cdev.subled_info = led->subled_info;
>
> Why do we need 2 copies of the same info?
>
> > + led->mc_cdev.num_colors = ARRAY_SIZE(led->subled_info);
>
> This is always 3 - use a define instead.
>
> > + init_data.fwnode = &np->fwnode;
> > +
> > + cdev = &led->mc_cdev.led_cdev;
> > + cdev->max_brightness = 1;
> > + cdev->brightness_get = turris1x_led_brightness_get;
> > + cdev->brightness_set = turris1x_led_brightness_set;
> > +
> > + /* All LEDs except LED 6 (WiFi) can be controller by hardware trigger */
> > + if (reg != 6)
> > + cdev->trigger_type = &turris1x_hw_trigger_type;
> > +
> > + /* Disable hardware trigger for LED 6 (WiFi) - allow software control */
> > + if (reg == 6 && !(val_sw_override & BIT(6))) {
> > + if (!(val_sw_disable & BIT(6))) {
>
> Wouldn't it make things much easier if we just did it anyway (see below)?
I just wanted to avoid accessing registers of the external memory
device when not needed.
> > + val_sw_disable |= BIT(6);
> > + writeb(val_sw_disable,
> > + leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
>
> Wrap at 100 everywhere please.
>
> > + }
> > + val_sw_override |= BIT(6);
> > + writeb(val_sw_override,
> > + leds->regs + TURRIS1X_LED_SW_OVERRIDE_REG);
> > + }
>
> if (reg == 6 && !(val_sw_override & BIT(6))) {
> writeb(val_sw_disable, leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
> writeb(val_sw_override, leds->regs + TURRIS1X_LED_SW_OVERRIDE_REG);
> val_sw_disable |= BIT(6);
> val_sw_override |= BIT(6);
> }
>
> > + if (!(val_sw_override & BIT(reg)))
> > + cdev->default_trigger = turris1x_hw_trigger.name;
> > +
> > + if (!(val_sw_override & BIT(reg)) || !(val_sw_disable & BIT(reg)))
> > + cdev->brightness = 1;
> > +
> > + ret = devm_led_classdev_multicolor_register_ext(dev, &led->mc_cdev,
> > + &init_data);
> > + if (ret) {
> > + dev_err(dev, "Cannot register LED %pOF: %i\n", np, ret);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static ssize_t brightness_show(struct device *dev, struct device_attribute *a,
> > + char *buf)
> > +{
> > + struct turris1x_leds *leds = dev_get_drvdata(dev);
> > + unsigned int brightness;
> > +
> > + /*
> > + * Current brightness value is available in read-only register
> > + * TURRIS1X_LED_GET_GLOBAL_BRIGHTNESS_REG. Equivalent code is:
> > + * level = readb(leds->regs + TURRIS1X_LED_GLOBAL_LEVEL_REG) & 7;
> > + * brightness = readb(leds->regs + TURRIS1X_LED_GLOBAL_BRIGHTNESS_REG(level));
> > + */
> > + brightness = readb(leds->regs + TURRIS1X_LED_GET_GLOBAL_BRIGHTNESS_REG);
> > +
> > + return sprintf(buf, "%u\n", brightness);
> > +}
> > +
> > +static ssize_t brightness_store(struct device *dev, struct device_attribute *a,
> > + const char *buf, size_t count)
> > +{
> > + struct turris1x_leds *leds = dev_get_drvdata(dev);
> > + int best_error, error, level, value;
> > + unsigned long brightness;
> > + u8 best_level;
> > +
> > + if (kstrtoul(buf, 10, &brightness))
> > + return -EINVAL;
> > +
> > + if (brightness > 255)
> > + return -EINVAL;
> > +
> > + /*
> > + * Brightness can be set only to one of 8 predefined value levels
> > + * available in TURRIS1X_LED_GLOBAL_BRIGHTNESS_REG(level) registers.
> > + * Choose level which has nearest value to the specified brightness.
> > + */
> > + best_level = 0;
> > + best_error = INT_MAX;
> > + for (level = 0; level < 8; level++) {
> > + value = readb(leds->regs +
> > + TURRIS1X_LED_GLOBAL_BRIGHTNESS_REG(level));
> > + error = abs(value - (int)brightness);
> > + if (best_error > error) {
> > + best_error = error;
> > + best_level = level;
> > + }
> > + }
> > +
> > + writeb(best_level, leds->regs + TURRIS1X_LED_GLOBAL_LEVEL_REG);
> > +
> > + return count;
> > +}
> > +static DEVICE_ATTR_RW(brightness);
> > +
> > +static ssize_t brightness_level_show(struct device *dev,
> > + struct device_attribute *a, char *buf)
> > +{
> > + struct turris1x_leds *leds = dev_get_drvdata(dev);
> > + unsigned int level;
> > +
> > + level = readb(leds->regs + TURRIS1X_LED_GLOBAL_LEVEL_REG) & 7;
> > +
> > + return sprintf(buf, "%u\n", level);
> > +}
> > +
> > +static ssize_t brightness_level_store(struct device *dev,
> > + struct device_attribute *a,
> > + const char *buf, size_t count)
> > +{
> > + struct turris1x_leds *leds = dev_get_drvdata(dev);
> > + unsigned long level;
> > +
> > + if (kstrtoul(buf, 10, &level))
> > + return -EINVAL;
> > +
> > + if (level > 7)
> > + return -EINVAL;
> > +
> > + writeb(level, leds->regs + TURRIS1X_LED_GLOBAL_LEVEL_REG);
> > +
> > + return count;
> > +}
> > +static DEVICE_ATTR_RW(brightness_level);
> > +
> > +static ssize_t brightness_values_show(struct device *dev,
> > + struct device_attribute *a, char *buf)
> > +{
> > + struct turris1x_leds *leds = dev_get_drvdata(dev);
> > + unsigned int vals[8];
> > + int i;
> > +
> > + for (i = 0; i < 8; i++)
> > + vals[i] = readb(leds->regs +
> > + TURRIS1X_LED_GLOBAL_BRIGHTNESS_REG(i));
> > +
> > + return sprintf(buf, "%u %u %u %u %u %u %u %u\n", vals[0], vals[1],
> > + vals[2], vals[3], vals[4], vals[5], vals[6], vals[7]);
> > +}
> > +
> > +static ssize_t brightness_values_store(struct device *dev,
> > + struct device_attribute *a,
> > + const char *buf, size_t count)
> > +{
> > + struct turris1x_leds *leds = dev_get_drvdata(dev);
> > + unsigned int vals[8];
> > + int nchars;
> > + int i;
> > +
> > + if (sscanf(buf, "%u %u %u %u %u %u %u %u%n", &vals[0], &vals[1],
> > + &vals[2], &vals[3], &vals[4], &vals[5], &vals[6], &vals[7],
> > + &nchars) != 8 || nchars + 1 < count)
> > + return -EINVAL;
> > +
> > + for (i = 0; i < 8; i++)
> > + writeb(vals[i],
> > + leds->regs + TURRIS1X_LED_GLOBAL_BRIGHTNESS_REG(i));
> > +
> > + return count;
> > +}
> > +static DEVICE_ATTR_RW(brightness_values);
> > +
> > +static struct attribute *turris1x_leds_controller_attrs[] = {
> > + &dev_attr_brightness.attr,
> > + &dev_attr_brightness_level.attr,
> > + &dev_attr_brightness_values.attr,
> > + NULL,
> > +};
> > +ATTRIBUTE_GROUPS(turris1x_leds_controller);
> > +
> > +static int turris1x_leds_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct device_node *np = dev_of_node(dev);
> > + struct device_node *child;
> > + struct turris1x_leds *leds;
> > + struct resource *res;
> > + void __iomem *regs;
> > + u8 val_sw_override;
> > + u8 val_sw_disable;
> > + int ret;
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!res)
> > + return -ENODEV;
> > +
> > + regs = devm_ioremap_resource(dev, res);
> > + if (IS_ERR(regs))
> > + return PTR_ERR(regs);
>
> devm_platform_get_and_ioremap_resource()
>
> > + leds = devm_kzalloc(dev, sizeof(*leds), GFP_KERNEL);
>
> Call this ddata everywhere.
>
> leds->leds is not overly forthcoming.
>
> > + if (!leds)
> > + return -ENOMEM;
> > +
> > + platform_set_drvdata(pdev, leds);
> > +
> > + leds->regs = regs;
> > + mutex_init(&leds->lock);
> > +
> > + ret = devm_led_trigger_register(dev, &turris1x_hw_trigger);
> > + if (ret) {
> > + dev_err(dev, "Cannot register private LED trigger: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + val_sw_override = readb(leds->regs + TURRIS1X_LED_SW_OVERRIDE_REG);
> > + val_sw_disable = readb(leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
> > +
> > + for_each_available_child_of_node(np, child) {
> > + ret = turris1x_led_register(dev, leds, child,
> > + val_sw_override, val_sw_disable);
> > + if (ret) {
> > + of_node_put(child);
> > + return ret;
> > + }
> > + }
> > +
> > + ret = devm_device_add_groups(dev, turris1x_leds_controller_groups);
> > + if (ret) {
> > + dev_err(dev, "Could not add attribute group!\n");
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void turris1x_leds_shutdown(struct platform_device *pdev)
> > +{
> > + struct turris1x_leds *leds = platform_get_drvdata(pdev);
> > + int i, j;
> > + u8 val;
> > +
> > + /*
> > + * LED registers are persisent across board resets.
> > + * So reset LEDs to default state before kernel reboots.
> > + */
>
> Do you really want to rely on the process before us to have done the
> same?
Yes.
> Wouldn't it be better to reset on boot-up rather than shutdown?
No.
During board reset, firmware shows specific LED pattern which show to
user that board is being reset. As stated above LED registers are
persistent across board resets, so to see visualize this pattern
correctly, it is needed to reset LED registers to default state before
reboot.
U-Boot bootloader resets registers to defaults but obviously it is too
late (as board reset at this stage finished). Also bootloader sets
specific LED pattern if booting into the "recovery" / "reflashing" mode
and we want to preserve this settings. This is already in the probe
phase implemented.
> > + /* Disable software control of all LEDs except LED 6 (WiFi) */
> > + writeb(BIT(6), leds->regs + TURRIS1X_LED_SW_OVERRIDE_REG);
> > +
> > + /* Turn off LED 6 (WiFi) as there is no hardware trigger for it */
> > + val = readb(leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
> > + writeb(val | BIT(6), leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
> > +
> > + /* Reset colors of all LEDs to default values */
> > + for (i = 0; i < ARRAY_SIZE(leds->leds); i++) {
> > + /* Skip LAN2-LAN5 LEDs which share color register with LAN1 */
> > + if (i >= 2 && i <= 5)
> > + continue;
> > + for (j = 0; j < ARRAY_SIZE(leds->leds[i].subled_info); j++)
> > + writeb(0xff,
> > + leds->regs + TURRIS1X_LED_BRIGHTNESS_REG(i, j));
> > + }
> > +}
> > +
> > +static const struct of_device_id of_turris1x_leds_match[] = {
> > + { .compatible = "cznic,turris1x-leds" },
> > + {},
> > +};
> > +
> > +static struct platform_driver turris1x_leds_driver = {
> > + .probe = turris1x_leds_probe,
> > + .shutdown = turris1x_leds_shutdown,
> > + .driver = {
> > + .name = "turris1x_leds",
> > + .of_match_table = of_turris1x_leds_match,
> > + },
> > +};
> > +module_platform_driver(turris1x_leds_driver);
> > +
> > +MODULE_AUTHOR("Pali Rohár <pali@kernel.org>");
> > +MODULE_DESCRIPTION("CZ.NIC's Turris 1.x LEDs");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("platform:turris1x_leds");
> > --
> > 2.20.1
> >
>
> --
> Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH RESEND 4/8] dt-bindings: leds: Add cznic,turris1x-leds.yaml binding
2023-02-24 9:38 ` Lee Jones
@ 2023-03-09 20:42 ` Pali Rohár
2023-03-11 11:47 ` Krzysztof Kozlowski
0 siblings, 1 reply; 42+ messages in thread
From: Pali Rohár @ 2023-03-09 20:42 UTC (permalink / raw)
To: Lee Jones; +Cc: soc, linux-kernel, devicetree
On Friday 24 February 2023 09:38:37 Lee Jones wrote:
> On Fri, 24 Feb 2023, Krzysztof Kozlowski wrote:
>
> > On 27/01/2023 12:16, Lee Jones wrote:
> > > On Mon, 26 Dec 2022, Pali Rohár wrote:
> > >
> > >> Add device-tree bindings documentation for Turris 1.x RGB LEDs.
> > >>
> > >> Signed-off-by: Pali Rohár <pali@kernel.org>
> > >> ---
> > >> .../bindings/leds/cznic,turris1x-leds.yaml | 118 ++++++++++++++++++
> > >> 1 file changed, 118 insertions(+)
> > >> create mode 100644 Documentation/devicetree/bindings/leds/cznic,turris1x-leds.yaml
> > >
> > > Needs a DT Ack (now Cc:ed)
> >
> > Within the same day of posting v2 of this - just after 4 hours, Pali
> > received review and I pointed issue to address.
> >
> > The issue was not addressed, just ignored.
> >
> > Therefore this patch shall not be taken, until the issues are resolved
> > or discussion is finished if there is disagreement about my comments..
>
> Understood. Thanks for the clarification.
Sorry, but this discussion was ignored by the reviewer, not by me.
I have replied to all points. In other cases I have sent other emails
and result was same.
I'm not going to write and send emails or pull requests or other code to
people who do not want to read them or think that it is me who is
ignoring even it is false information.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH RESEND 4/8] dt-bindings: leds: Add cznic,turris1x-leds.yaml binding
2023-03-09 20:42 ` Pali Rohár
@ 2023-03-11 11:47 ` Krzysztof Kozlowski
0 siblings, 0 replies; 42+ messages in thread
From: Krzysztof Kozlowski @ 2023-03-11 11:47 UTC (permalink / raw)
To: Pali Rohár, Lee Jones
Cc: soc, linux-kernel, devicetree, Arnd Bergmann, Linus Walleij
On 09/03/2023 21:42, Pali Rohár wrote:
> On Friday 24 February 2023 09:38:37 Lee Jones wrote:
>> On Fri, 24 Feb 2023, Krzysztof Kozlowski wrote:
>>
>>> On 27/01/2023 12:16, Lee Jones wrote:
>>>> On Mon, 26 Dec 2022, Pali Rohár wrote:
>>>>
>>>>> Add device-tree bindings documentation for Turris 1.x RGB LEDs.
>>>>>
>>>>> Signed-off-by: Pali Rohár <pali@kernel.org>
>>>>> ---
>>>>> .../bindings/leds/cznic,turris1x-leds.yaml | 118 ++++++++++++++++++
>>>>> 1 file changed, 118 insertions(+)
>>>>> create mode 100644 Documentation/devicetree/bindings/leds/cznic,turris1x-leds.yaml
>>>>
>>>> Needs a DT Ack (now Cc:ed)
>>>
>>> Within the same day of posting v2 of this - just after 4 hours, Pali
>>> received review and I pointed issue to address.
>>>
>>> The issue was not addressed, just ignored.
>>>
>>> Therefore this patch shall not be taken, until the issues are resolved
>>> or discussion is finished if there is disagreement about my comments..
>>
>> Understood. Thanks for the clarification.
>
> Sorry, but this discussion was ignored by the reviewer, not by me.
> I have replied to all points. In other cases I have sent other emails
> and result was same.
>
> I'm not going to write and send emails or pull requests or other code to
> people who do not want to read them or think that it is me who is
> ignoring even it is false information.
Why did you trim the reply list from few folks and from me, even though
discussion is about review between you and me? You had to do it
deliberately, so I wonder why?
OK, you claim this is false information.
This is the last feedback from me:
https://lore.kernel.org/all/ebf5029e-83fd-e50d-b7cb-eae1b64f7145@linaro.org/
Where did you respond to this feedback? Point me to the message which
responds to my review comment about messed indentation?
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 42+ messages in thread
end of thread, other threads:[~2023-03-11 11:47 UTC | newest]
Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-26 12:36 [PATCH RESEND 0/8] Resend LED patches Pali Rohár
2022-12-26 12:36 ` [PATCH RESEND 1/8] dt-bindings: leds: register-bit-led: Add active-low property Pali Rohár
2023-01-27 11:16 ` Lee Jones
2023-02-23 14:22 ` Lee Jones
2023-02-23 16:48 ` Pali Rohár
2023-02-23 20:59 ` Linus Walleij
2023-02-24 8:42 ` Krzysztof Kozlowski
2022-12-26 12:36 ` [PATCH RESEND 2/8] leds: syscon: Implement support for " Pali Rohár
2023-02-23 14:25 ` Lee Jones
2022-12-26 12:36 ` [PATCH RESEND 3/8] powerpc/85xx: DTS: Add CPLD definitions for P1021RDB Combo Board CPL Design Pali Rohár
2022-12-26 12:36 ` [PATCH RESEND 4/8] dt-bindings: leds: Add cznic,turris1x-leds.yaml binding Pali Rohár
2023-01-27 11:16 ` Lee Jones
2023-02-24 9:15 ` Krzysztof Kozlowski
2023-02-24 9:38 ` Lee Jones
2023-03-09 20:42 ` Pali Rohár
2023-03-11 11:47 ` Krzysztof Kozlowski
2023-02-24 9:13 ` Krzysztof Kozlowski
2022-12-26 12:36 ` [PATCH RESEND 5/8] leds: Add support for Turris 1.x LEDs Pali Rohár
2023-01-27 11:20 ` Lee Jones
2023-02-02 23:46 ` Pali Rohár
2023-02-24 9:25 ` Krzysztof Kozlowski
2023-02-24 9:37 ` Lee Jones
2023-02-24 9:22 ` Krzysztof Kozlowski
2023-02-24 9:28 ` Lee Jones
2023-03-09 20:35 ` Pali Rohár
2022-12-26 12:36 ` [PATCH RESEND 6/8] leds: turris-omnia: support HW controlled mode via private trigger Pali Rohár
2023-02-24 9:32 ` Lee Jones
2022-12-26 12:36 ` [PATCH RESEND 7/8] leds: turris-omnia: initialize multi-intensity to full Pali Rohár
2023-02-24 9:33 ` Lee Jones
2022-12-26 12:36 ` [PATCH RESEND 8/8] leds: turris-omnia: change max brightness from 255 to 1 Pali Rohár
2023-02-24 9:34 ` Lee Jones
2023-03-09 20:07 ` Pali Rohár
2023-01-20 16:41 ` [PATCH RESEND 0/8] Resend LED patches Arnd Bergmann
2023-01-20 16:41 ` Arnd Bergmann
2023-01-20 17:15 ` Lee Jones
2023-01-20 17:15 ` Lee Jones
2023-01-20 17:47 ` Arnd Bergmann
2023-01-20 17:47 ` Arnd Bergmann
2023-01-20 20:02 ` Lee Jones
2023-01-20 20:02 ` Lee Jones
2023-01-26 20:07 ` Linus Walleij
2023-01-26 20:07 ` Linus Walleij
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.