Resend LED patches

Allow to define inverted logic (0 - enable LED, 1 - disable LED) via active-low property.

Signed-off-by Pali Rohár <[email protected]>

Reviewed-by Linus Walleij <[email protected]>

---
 Documentation/devicetree/bindings/leds/register-bit-led.yaml | 5 +++++
 1 file changed, 5 insertions(+)

Documentation/devicetree/bindings/leds/register-bit-led.yaml +5 -0

index ed26ec19ecbd..418130b29caa 100644
@@ -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

Signature · 1 line

2.20.1

Quoted 3 lines · Pali Rohár

Allow to define inverted logic (0 - enable LED, 1 - disable LED) via active-low property.

Quoted 5 lines

Signed-off-by Pali Rohár <[email protected]>

Reviewed-by Linus Walleij <[email protected]>

---
 Documentation/devicetree/bindings/leds/register-bit-led.yaml | 5 +++++
 1 file changed, 5 insertions(+)

Needs a DT Ack (now Cc:ed)

Quoted 19 lines

Documentation/devicetree/bindings/leds/register-bit-led.yaml +5 -0

index ed26ec19ecbd..418130b29caa 100644
@@ -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

Signature · 1 line

2.20.1

Signature · 1 line

Lee Jones [李琼斯]

Quoted 11 lines · Lee Jones

Quoted 8 lines · Pali Rohár

Allow to define inverted logic (0 - enable LED, 1 - disable LED) via active-low property.

Signed-off-by Pali Rohár <[email protected]>

Reviewed-by Linus Walleij <[email protected]>

---
 Documentation/devicetree/bindings/leds/register-bit-led.yaml | 5 +++++
 1 file changed, 5 insertions(+)

Quoted 1 line

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.

Quoted 22 lines

Quoted 19 lines

Documentation/devicetree/bindings/leds/register-bit-led.yaml +5 -0

index ed26ec19ecbd..418130b29caa 100644
@@ -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

Signature · 1 line

2.20.1

Signature · 1 line

Lee Jones [李琼斯]

Signature · 1 line

Lee Jones [李琼斯]

Quoted 17 lines · On Thursday 23 February 2023 14:22:52 Lee Jones wrote:

Quoted 5 lines · Lee Jones

Quoted 3 lines · Pali Rohár

Allow to define inverted logic (0 - enable LED, 1 - disable LED) via active-low property.

Quoted 7 lines

Quoted 5 lines

Signed-off-by Pali Rohár <[email protected]>

Reviewed-by Linus Walleij <[email protected]>

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

Quoted 3 lines

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.

Quoted 22 lines · Pali Rohár

Quoted 20 lines · On Thursday 23 February 2023 14:22:52 Lee Jones wrote:

Quoted 5 lines · Lee Jones

Quoted 3 lines · Pali Rohár

Allow to define inverted logic (0 - enable LED, 1 - disable LED) via active-low property.

Quoted 7 lines

Quoted 5 lines

Signed-off-by Pali Rohár <[email protected]>

Reviewed-by Linus Walleij <[email protected]>

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

Quoted 3 lines

(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 :/

Quoted 3 lines

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.

Quoted 6 lines

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

This new active-low property specify that LED has inverted logic (0 - enable LED, 1 - disable LED).

Signed-off-by Pali Rohár <[email protected]>

Acked-by Linus Walleij <[email protected]>

---
 drivers/leds/leds-syscon.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

drivers/leds/leds-syscon.c +10 -4

index 7eddb8ecb44e..5e605d8438e9 100644
@@ -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;
 		}

Signature · 1 line

2.20.1

Add device-tree bindings documentation for Turris 1.x RGB LEDs.

Signed-off-by Pali Rohár <[email protected]>

---
 .../bindings/leds/cznic,turris1x-leds.yaml    | 118 ++++++++++++++++++
 1 file changed, 118 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/cznic,turris1x-leds.yaml

Documentation/devicetree/bindings/leds/cznic,turris1x-leds.yaml new file +118 -0

new file mode 100644
index 000000000000..bcaab5b03128
@@ -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 <[email protected]>
+
+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;
+            };
+        };
+    };
+
+...

Signature · 1 line

2.20.1

Quoted 7 lines · Pali Rohár

Add device-tree bindings documentation for Turris 1.x RGB LEDs.

Signed-off-by Pali Rohár <[email protected]>

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

Quoted 11 lines · Lee Jones

Quoted 7 lines · Pali Rohár

Add device-tree bindings documentation for Turris 1.x RGB LEDs.

Signed-off-by Pali Rohár <[email protected]>

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

Quoted 16 lines · Krzysztof Kozlowski

Quoted 11 lines · Lee Jones

Quoted 7 lines · Pali Rohár

Add device-tree bindings documentation for Turris 1.x RGB LEDs.

Signed-off-by Pali Rohár <[email protected]>

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

Quoted 4 lines

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.

Quoted 18 lines · On Friday 24 February 2023 09:38:37 Lee Jones wrote:

Quoted 16 lines · Krzysztof Kozlowski

Quoted 11 lines · Lee Jones

Quoted 7 lines · Pali Rohár

Add device-tree bindings documentation for Turris 1.x RGB LEDs.

Signed-off-by Pali Rohár <[email protected]>

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

Quoted 6 lines

Quoted 4 lines

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.

Add device-tree bindings documentation for Turris 1.x RGB LEDs.

Signed-off-by Pali Rohár <[email protected]>

---
 .../bindings/leds/cznic,turris1x-leds.yaml    | 118 ++++++++++++++++++
 1 file changed, 118 insertions(+)

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 <[email protected]>

Reviewed-by Marek Behún <[email protected]>

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

Documentation/ABI/testing/sysfs-class-led-driver-turris1x new file +31 -0

new file mode 100644
index 000000000000..272695ae400b
@@ -0,0 +1,31 @@
+What:		/sys/class/leds/<led>/device/brightness
+Date:		July 2022
+KernelVersion:	6.3
+Contact:	Pali Rohár <[email protected]>
+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 <[email protected]>
+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 <[email protected]>
+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

drivers/leds/Kconfig +9 -0

index 499d0f215a8b..6f78764e20aa 100644
@@ -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

drivers/leds/Makefile +1 -0

index 4fd2f92cd198..de08083dbbca 100644
@@ -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

drivers/leds/leds-turris-1x.c new file +474 -0

new file mode 100644
index 000000000000..cf7567b64306
@@ -0,0 +1,474 @@
+// SPDX-License-Identifier: GPL-2.0
+// (C) 2022 Pali Rohár <[email protected]>
+//
+// 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", &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 <[email protected]>");
+MODULE_DESCRIPTION("CZ.NIC's Turris 1.x LEDs");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:turris1x_leds");

Signature · 1 line

2.20.1

Pavel, are you happy with the user-space interface?

Quoted 62 lines · Pali Rohár

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 <[email protected]>

Reviewed-by Marek Behún <[email protected]>

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

Documentation/ABI/testing/sysfs-class-led-driver-turris1x new file +31 -0

new file mode 100644
index 000000000000..272695ae400b
@@ -0,0 +1,31 @@
+What:		/sys/class/leds/<led>/device/brightness
+Date:		July 2022
+KernelVersion:	6.3
+Contact:	Pali Rohár <[email protected]>
+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 <[email protected]>
+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 <[email protected]>
+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

Pavel, are you happy with the user-space interface?

Quoted 2 lines · Lee Jones

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/

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 <[email protected]>

Reviewed-by Marek Behún <[email protected]>

---

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 <[email protected]>

Reviewed-by Marek Behún <[email protected]>

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

Documentation/ABI/testing/sysfs-class-led-driver-turris1x new file +0 -0

new file mode 100644
index 000000000000..272695ae400b

Documentation/ABI/testing/sysfs-class-led-driver-turris1x new file +5 -0

+		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

drivers/leds/Kconfig +7 -0

+	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

drivers/leds/Makefile +1 -0

index 4fd2f92cd198..de08083dbbca 100644
@@ -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

drivers/leds/leds-turris-1x.c new file +6 -0

new file mode 100644
index 000000000000..cf7567b64306
@@ -0,0 +1,474 @@
+// SPDX-License-Identifier: GPL-2.0
+// (C) 2022 Pali Rohár <[email protected]>
+//
+// 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
+

drivers/leds/leds-turris-1x.c new file +23 -0

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

drivers/leds/leds-turris-1x.c new file +5 -0

+}
+
+static void turris1x_led_brightness_set(struct led_classdev *cdev,
+					enum led_brightness brightness)
+{

drivers/leds/leds-turris-1x.c new file +12 -0

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

drivers/leds/leds-turris-1x.c new file +4 -0

+	init_data.fwnode = &np->fwnode;
+
+	cdev = &led->mc_cdev.led_cdev;
+	cdev->max_brightness = 1;

drivers/leds/leds-turris-1x.c new file +154 -0

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

drivers/leds/leds-turris-1x.c new file +35 -0

+	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;
+}
+

drivers/leds/leds-turris-1x.c new file +37 -0

+	/* 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 <[email protected]>");
+MODULE_DESCRIPTION("CZ.NIC's Turris 1.x LEDs");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:turris1x_leds");

Signature · 1 line

2.20.1

Quoted 31 lines · Pali Rohár

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 <[email protected]>

Reviewed-by Marek Behún <[email protected]>

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

Documentation/ABI/testing/sysfs-class-led-driver-turris1x new file +0 -0

new file mode 100644
index 000000000000..272695ae400b

Documentation/ABI/testing/sysfs-class-led-driver-turris1x line 3

Quoted 3 lines

Documentation/ABI/testing/sysfs-class-led-driver-turris1x new file +3 -0

@@ -0,0 +1,31 @@
+What:		/sys/class/leds/<led>/device/brightness
+Date:		July 2022
+KernelVersion:	6.3

Date doesn't match the kernel version.

Documentation/ABI/testing/sysfs-class-led-driver-turris1x line 5

Quoted 2 lines

Documentation/ABI/testing/sysfs-class-led-driver-turris1x new file +2 -0

+Contact:	Pali Rohár <[email protected]>
+Description:	(RW) On the back size of the Turris 1.x routers there is also

"side"

Drop "also"

Documentation/ABI/testing/sysfs-class-led-driver-turris1x line 7

Quoted 2 lines

Documentation/ABI/testing/sysfs-class-led-driver-turris1x new file +2 -0

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

Documentation/ABI/testing/sysfs-class-led-driver-turris1x line 9

Quoted 1 line

Documentation/ABI/testing/sysfs-class-led-driver-turris1x new file +1 -0

+		The CPLD firmware cycles between 8 levels of this global

Drop "this"

Quoted 5 lines

Documentation/ABI/testing/sysfs-class-led-driver-turris1x new file +5 -0

+		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

Documentation/ABI/testing/sysfs-class-led-driver-turris1x line 24

Quoted 10 lines

Documentation/ABI/testing/sysfs-class-led-driver-turris1x new file +10 -0

+
+What:		/sys/class/leds/<led>/device/brightness_level
+Date:		July 2022
+KernelVersion:	6.3
+Contact:	Pali Rohár <[email protected]>
+Description:	(RW) Current brightness level value (0-7).
+
+		Format: %u
+
+What:		/sys/class/leds/<led>/device/brightness_values

brightness_level_values?

Documentation/ABI/testing/sysfs-class-led-driver-turris1x line 31

Quoted 7 lines

Documentation/ABI/testing/sysfs-class-led-driver-turris1x new file +7 -0

+Date:		July 2022
+KernelVersion:	6.3
+Contact:	Pali Rohár <[email protected]>
+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.

drivers/leds/Kconfig config LEDS_EL15203000 · line 161

Quoted 10 lines

drivers/leds/Kconfig +2 -0

index 499d0f215a8b..6f78764e20aa 100644
@@ -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?

Quoted 34 lines

drivers/leds/Kconfig +7 -0

+	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

drivers/leds/Makefile +1 -0

index 4fd2f92cd198..de08083dbbca 100644
@@ -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

drivers/leds/leds-turris-1x.c new file +6 -0

new file mode 100644
index 000000000000..cf7567b64306
@@ -0,0 +1,474 @@
+// SPDX-License-Identifier: GPL-2.0
+// (C) 2022 Pali Rohár <[email protected]>
+//
+// 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
+

drivers/leds/leds-turris-1x.c line 16

Quoted 10 lines

drivers/leds/leds-turris-1x.c new file +10 -0

+#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?

Quoted 4 lines

drivers/leds/leds-turris-1x.c new file +4 -0

+/* 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 : \

drivers/leds/leds-turris-1x.c line 26

Quoted 5 lines

drivers/leds/leds-turris-1x.c new file +5 -0

+						   (_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.

drivers/leds/leds-turris-1x.c line 35

Quoted 9 lines

drivers/leds/leds-turris-1x.c new file +9 -0

+#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().

drivers/leds/leds-turris-1x.c line 40

Quoted 5 lines

drivers/leds/leds-turris-1x.c new file +5 -0

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

drivers/leds/leds-turris-1x.c line 45

Quoted 4 lines

drivers/leds/leds-turris-1x.c new file +4 -0

+struct turris1x_leds {
+	void __iomem *regs;
+	struct mutex lock;
+	struct turris1x_led leds[8];

As above s/3/8/.

drivers/leds/leds-turris-1x.c line 48

Quoted 3 lines

drivers/leds/leds-turris-1x.c new file +3 -0

+};
+
+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?

drivers/leds/leds-turris-1x.c line 56

Quoted 7 lines

drivers/leds/leds-turris-1x.c new file +7 -0

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

Quoted 23 lines

drivers/leds/leds-turris-1x.c new file +23 -0

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

drivers/leds/leds-turris-1x.c line 89

Quoted 10 lines

drivers/leds/leds-turris-1x.c new file +10 -0

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

drivers/leds/leds-turris-1x.c line 94

Quoted 5 lines

drivers/leds/leds-turris-1x.c new file +5 -0

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

drivers/leds/leds-turris-1x.c line 98

Quoted 4 lines

drivers/leds/leds-turris-1x.c new file +4 -0

+	else if (!(readb(leds->regs + TURRIS1X_LED_SW_DISABLE_REG) & BIT(led->reg)))
+		return 1;
+	else
+		return 0;

"LED_OFF"

Quoted 5 lines

drivers/leds/leds-turris-1x.c new file +5 -0

+}
+
+static void turris1x_led_brightness_set(struct led_classdev *cdev,
+					enum led_brightness brightness)
+{

drivers/leds/leds-turris-1x.c line 113

Quoted 10 lines

drivers/leds/leds-turris-1x.c new file +10 -0

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

drivers/leds/leds-turris-1x.c line 114

Quoted 1 line

drivers/leds/leds-turris-1x.c new file +1 -0

+		led_mc_calc_color_components(mc_cdev, brightness);

'\n'

drivers/leds/leds-turris-1x.c line 117

Quoted 3 lines

drivers/leds/leds-turris-1x.c new file +3 -0

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

drivers/leds/leds-turris-1x.c line 124

Quoted 7 lines

drivers/leds/leds-turris-1x.c new file +7 -0

+
+		/*
+		 * 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.

drivers/leds/leds-turris-1x.c line 130

Quoted 6 lines

drivers/leds/leds-turris-1x.c new file +6 -0

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

drivers/leds/leds-turris-1x.c line 133

Quoted 3 lines

drivers/leds/leds-turris-1x.c new file +3 -0

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

drivers/leds/leds-turris-1x.c line 142

Quoted 9 lines

drivers/leds/leds-turris-1x.c new file +9 -0

+			}
+		}
+	}
+
+	/* 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.

drivers/leds/leds-turris-1x.c line 145

Quoted 3 lines

drivers/leds/leds-turris-1x.c new file +3 -0

+	else if (!brightness && !(val & BIT(led->reg)))
+		writeb(val | BIT(led->reg),
+		       leds->regs + TURRIS1X_LED_SW_DISABLE_REG);

Single line.

Quoted 12 lines

drivers/leds/leds-turris-1x.c new file +12 -0

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

drivers/leds/leds-turris-1x.c line 168

Quoted 10 lines

drivers/leds/leds-turris-1x.c new file +10 -0

+	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", &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.

drivers/leds/leds-turris-1x.c line 176

Quoted 8 lines

drivers/leds/leds-turris-1x.c new file +8 -0

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

drivers/leds/leds-turris-1x.c line 185

Quoted 9 lines

drivers/leds/leds-turris-1x.c new file +9 -0

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

drivers/leds/leds-turris-1x.c line 192

Quoted 7 lines

drivers/leds/leds-turris-1x.c new file +7 -0

+		return -EINVAL;
+	}
+
+	led->registered = true;
+	led->reg = reg;
+
+	/* Set initial colors to what are currently in use */

"to those currently"

drivers/leds/leds-turris-1x.c line 197

Quoted 5 lines

drivers/leds/leds-turris-1x.c new file +5 -0

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

drivers/leds/leds-turris-1x.c line 200

Quoted 3 lines

drivers/leds/leds-turris-1x.c new file +3 -0

+	}
+
+	led->mc_cdev.subled_info = led->subled_info;

Why do we need 2 copies of the same info?

drivers/leds/leds-turris-1x.c line 201

Quoted 1 line

drivers/leds/leds-turris-1x.c new file +1 -0

+	led->mc_cdev.num_colors = ARRAY_SIZE(led->subled_info);

This is always 3 - use a define instead.

Quoted 4 lines

drivers/leds/leds-turris-1x.c new file +4 -0

+	init_data.fwnode = &np->fwnode;
+
+	cdev = &led->mc_cdev.led_cdev;
+	cdev->max_brightness = 1;

drivers/leds/leds-turris-1x.c line 216

Quoted 10 lines

drivers/leds/leds-turris-1x.c new file +10 -0

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

drivers/leds/leds-turris-1x.c line 219

Quoted 3 lines

drivers/leds/leds-turris-1x.c new file +3 -0

+			val_sw_disable |= BIT(6);
+			writeb(val_sw_disable,
+			       leds->regs + TURRIS1X_LED_SW_DISABLE_REG);

Wrap at 100 everywhere please.

drivers/leds/leds-turris-1x.c line 224

Quoted 5 lines

drivers/leds/leds-turris-1x.c new file +5 -0

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

Quoted 154 lines

drivers/leds/leds-turris-1x.c new file +154 -0

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

drivers/leds/leds-turris-1x.c line 389

Quoted 10 lines

drivers/leds/leds-turris-1x.c new file +10 -0

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

drivers/leds/leds-turris-1x.c line 391

Quoted 1 line

drivers/leds/leds-turris-1x.c new file +1 -0

+	leds = devm_kzalloc(dev, sizeof(*leds), GFP_KERNEL);

Call this ddata everywhere.

leds->leds is not overly forthcoming.

Quoted 40 lines

drivers/leds/leds-turris-1x.c new file +40 -0

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

drivers/leds/leds-turris-1x.c line 436

Quoted 5 lines

drivers/leds/leds-turris-1x.c new file +5 -0

+
+	/*
+	 * 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?

Quoted 40 lines

drivers/leds/leds-turris-1x.c new file +37 -0

+	/* 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 <[email protected]>");
+MODULE_DESCRIPTION("CZ.NIC's Turris 1.x LEDs");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:turris1x_leds");

Signature · 1 line

2.20.1

Signature · 1 line

Lee Jones [李琼斯]

From: Marek Behún <[email protected]>

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 <[email protected]>

Fixes 40624346b7ae ("ARM: dts: turris-omnia: enable LED controller node")

Reviewed-by Pali Rohár <[email protected]>

---
 drivers/leds/Kconfig             |  1 +
 drivers/leds/leds-turris-omnia.c | 41 ++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

drivers/leds/Kconfig +1 -0

index 6f78764e20aa..f957b86411de 100644
@@ -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

drivers/leds/leds-turris-omnia.c +0 -0

index c7c9851c894a..bb7af9e59ad6 100644

drivers/leds/leds-turris-omnia.c +22 -0

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

Signature · 1 line

2.20.1

From: Marek Behún <[email protected]>

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 <[email protected]>

Fixes 089381b27abe ("leds: initial support for Turris Omnia LEDs")

Reviewed-by Pali Rohár <[email protected]>

---
 drivers/leds/leds-turris-omnia.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/leds/leds-turris-omnia.c +3 -0

index bb7af9e59ad6..71340dec492a 100644
@@ -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;

Signature · 1 line

2.20.1

From: Marek Behún <[email protected]>

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 <[email protected]>

Fixes 089381b27abe ("leds: initial support for Turris Omnia LEDs")

Reviewed-by Pali Rohár <[email protected]>

---
 drivers/leds/leds-turris-omnia.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/leds/leds-turris-omnia.c +0 -0

index 71340dec492a..04f01ae46c27 100644