[PATCH 1/2] dt-bindings: leds: Add cznic,turris1x-leds.yaml binding

[PATCH 1/2] dt-bindings: leds: Add cznic,turris1x-leds.yaml binding

[Pali Rohár]

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    | 116 ++++++++++++++++++
 1 file changed, 116 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..fd09613c8d2d
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/cznic,turris1x-leds.yaml
@@ -0,0 +1,116 @@
+# 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:
+    maxItems: 2
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+patternProperties:
+  "^multi-led@[0-7]$":
+    type: object
+    $ref: leds-class-multicolor.yaml#
+
+    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>;
+
+        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

[PATCH 2/2] leds: Add support for Turris 1.x LEDs

[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 <pali@kernel.org>
---
 .../testing/sysfs-class-led-driver-turris1x   |  23 +
 drivers/leds/Kconfig                          |   9 +
 drivers/leds/Makefile                         |   1 +
 drivers/leds/leds-turris-1x.c                 | 412 ++++++++++++++++++
 4 files changed, 445 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..02be9197554d
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-driver-turris1x
@@ -0,0 +1,23 @@
+What:		/sys/class/leds/<led>/device/brightness
+Date:		July 2022
+KernelVersion:	5.20
+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 (from 100% to 0%), 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_values
+Date:		July 2022
+KernelVersion:	5.20
+Contact:	Pali Rohár <pali@kernel.org>
+Description:	(RW) Values of all 8 levels between which CPLD firmware cycles
+		when brightness buffer is pressed.
+
+		Format: %u %u %u %u %u %u %u %u
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index a49979f41eee..71caf45c8ac3 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..bd566f5210c7
--- /dev/null
+++ b/drivers/leds/leds-turris-1x.c
@@ -0,0 +1,412 @@
+// 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 LED_ON;
+	else if (!(readb(leds->regs + TURRIS1X_LED_SW_DISABLE_REG) & BIT(led->reg)))
+		return LED_ON;
+	else
+		return LED_OFF;
+}
+
+static int turris1x_led_brightness_set_blocking(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_ON) {
+		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 == LED_ON && (val & BIT(led->reg)))
+		writeb(val & ~BIT(led->reg),
+		       leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
+	else if (brightness == LED_OFF && !(val & BIT(led->reg)))
+		writeb(val | BIT(led->reg),
+		       leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
+
+	mutex_unlock(&leds->lock);
+
+	return 0;
+}
+
+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;
+
+	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;
+
+	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;
+	led->mc_cdev.num_colors = ARRAY_SIZE(led->subled_info);
+
+	init_data.fwnode = &np->fwnode;
+
+	cdev = &led->mc_cdev.led_cdev;
+	cdev->max_brightness = LED_ON;
+	cdev->brightness_get = turris1x_led_brightness_get;
+	cdev->brightness_set_blocking = turris1x_led_brightness_set_blocking;
+
+	/* 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 = LED_ON;
+
+	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_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_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 const struct of_device_id of_turris1x_leds_match[] = {
+	{ .compatible = "cznic,turris1x-leds" },
+	{},
+};
+
+static struct platform_driver turris1x_leds_driver = {
+	.probe = turris1x_leds_probe,
+	.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

Re: [PATCH 2/2] leds: Add support for Turris 1.x LEDs

[Marek Behún]

On Tue,  5 Jul 2022 02:04:48 +0200
Pali Rohár <pali@kernel.org> 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>
> ---
>  .../testing/sysfs-class-led-driver-turris1x   |  23 +
>  drivers/leds/Kconfig                          |   9 +
>  drivers/leds/Makefile                         |   1 +
>  drivers/leds/leds-turris-1x.c                 | 412 ++++++++++++++++++
>  4 files changed, 445 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..02be9197554d
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-led-driver-turris1x
> @@ -0,0 +1,23 @@
> +What:		/sys/class/leds/<led>/device/brightness
> +Date:		July 2022
> +KernelVersion:	5.20
> +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 (from 100% to 0%), 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_values
> +Date:		July 2022
> +KernelVersion:	5.20
> +Contact:	Pali Rohár <pali@kernel.org>
> +Description:	(RW) Values of all 8 levels between which CPLD firmware cycles
> +		when brightness buffer is pressed.

*button

> +		Format: %u %u %u %u %u %u %u %u
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index a49979f41eee..71caf45c8ac3 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..bd566f5210c7
> --- /dev/null
> +++ b/drivers/leds/leds-turris-1x.c
> @@ -0,0 +1,412 @@
> +// 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 LED_ON;
> +	else if (!(readb(leds->regs + TURRIS1X_LED_SW_DISABLE_REG) & BIT(led->reg)))
> +		return LED_ON;
> +	else
> +		return LED_OFF;
> +}

LED_ON and LED_OFF are deprecated, as says in include/linux/leds.h.
Just return 0 or 1.

> +
> +static int turris1x_led_brightness_set_blocking(struct led_classdev *cdev,
> +						enum led_brightness brightness)
> +{

The register write should be immediate, we don't need to use the
_blocking variant here. On Omnia we need to, cause I2C is slow.

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

	if (brightness)

So this will also make sure that I can change LED color by writing
new color to multi_intensity and 1 to brightness, even if the LED is in
HW control mode, am I correct?

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

I don't consider this a problem, but suppose that we are rewriting the
other LEDs colors, and at the same time someone is reading from sysfs
the multi_intensity file of a LED that is being rewritten. This can
result in wrong value being read, right? But probably only on multi-core
systems...

> +		}
> +	}
> +
> +	/* Enable / disable LED for software control */
> +	val = readb(leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
> +	if (brightness == LED_ON && (val & BIT(led->reg)))
> +		writeb(val & ~BIT(led->reg),
> +		       leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
> +	else if (brightness == LED_OFF && !(val & BIT(led->reg)))
> +		writeb(val | BIT(led->reg),
> +		       leds->regs + TURRIS1X_LED_SW_DISABLE_REG);

Pls just use brightness or !brightness. Or maybe just write the value?
Is the register write expensive on the CPLD or why are you trying to
avoid it if unnecessary?

> +	mutex_unlock(&leds->lock);
> +
> +	return 0;
> +}
> +
> +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;
> +
> +	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);

Weird that checkpatch doesn't warn about spaces around the minus
operator in ARRAY_SIZE()-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;
> +
> +	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;
> +	led->mc_cdev.num_colors = ARRAY_SIZE(led->subled_info);
> +
> +	init_data.fwnode = &np->fwnode;
> +
> +	cdev = &led->mc_cdev.led_cdev;
> +	cdev->max_brightness = LED_ON;

s/LED_ON/1/

> +	cdev->brightness_get = turris1x_led_brightness_get;
> +	cdev->brightness_set_blocking = turris1x_led_brightness_set_blocking;

See above about _blocking.

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

Again, is the register write that expensive that you try to avoid it if
the corresponding bit has the value you want?

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

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

Hmm. Wouldn't it make more sense to simply have the global brightness
accept values from 0 to 7, instead of mapping it to 256 values? And
call it something like selected_brightness_index?

> +	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_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_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 const struct of_device_id of_turris1x_leds_match[] = {
> +	{ .compatible = "cznic,turris1x-leds" },
> +	{},
> +};
> +
> +static struct platform_driver turris1x_leds_driver = {
> +	.probe = turris1x_leds_probe,
> +	.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");

Re: [PATCH 2/2] leds: Add support for Turris 1.x LEDs

[Pali Rohár]

On Tuesday 05 July 2022 12:37:05 Marek Behún wrote:
> On Tue,  5 Jul 2022 02:04:48 +0200
> Pali Rohár <pali@kernel.org> 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>
> > ---
> >  .../testing/sysfs-class-led-driver-turris1x   |  23 +
> >  drivers/leds/Kconfig                          |   9 +
> >  drivers/leds/Makefile                         |   1 +
> >  drivers/leds/leds-turris-1x.c                 | 412 ++++++++++++++++++
> >  4 files changed, 445 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..02be9197554d
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-class-led-driver-turris1x
> > @@ -0,0 +1,23 @@
> > +What:		/sys/class/leds/<led>/device/brightness
> > +Date:		July 2022
> > +KernelVersion:	5.20
> > +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 (from 100% to 0%), 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_values
> > +Date:		July 2022
> > +KernelVersion:	5.20
> > +Contact:	Pali Rohár <pali@kernel.org>
> > +Description:	(RW) Values of all 8 levels between which CPLD firmware cycles
> > +		when brightness buffer is pressed.
> 
> *button

OK.

> > +		Format: %u %u %u %u %u %u %u %u
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > index a49979f41eee..71caf45c8ac3 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..bd566f5210c7
> > --- /dev/null
> > +++ b/drivers/leds/leds-turris-1x.c
> > @@ -0,0 +1,412 @@
> > +// 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 LED_ON;
> > +	else if (!(readb(leds->regs + TURRIS1X_LED_SW_DISABLE_REG) & BIT(led->reg)))
> > +		return LED_ON;
> > +	else
> > +		return LED_OFF;
> > +}
> 
> LED_ON and LED_OFF are deprecated, as says in include/linux/leds.h.
> Just return 0 or 1.

Ok!

> > +
> > +static int turris1x_led_brightness_set_blocking(struct led_classdev *cdev,
> > +						enum led_brightness brightness)
> > +{
> 
> The register write should be immediate, we don't need to use the
> _blocking variant here. On Omnia we need to, cause I2C is slow.

Ok! (I thought that usage of mutex is also blocking variant)

> > +	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_ON) {
> 
> 	if (brightness)
> 
> So this will also make sure that I can change LED color by writing
> new color to multi_intensity and 1 to brightness, even if the LED is in
> HW control mode, am I correct?

This is for ability to disable LAN1 and enable LAN2, both in SW mode
which shares TURRIS1X_LED_BRIGHTNESS_REG register. Disabling LED cause
that mc_cdev->subled_info[0..3].brightness is 0.

> > +		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;
> > +			}
> 
> I don't consider this a problem

I think it is a problem, to ensure that 'cat multi_intensity' for every
LAN led reports correct (= current) value. If you change multi_intensity
of LAN1 then color is changed for all LAN* but sysfs reports new value
only for LAN1 and old values for LAN2..5.

> but suppose that we are rewriting the
> other LEDs colors, and at the same time someone is reading from sysfs
> the multi_intensity file of a LED that is being rewritten. This can
> result in wrong value being read, right? But probably only on multi-core
> systems...

This is eventual inconsistency and I do not consider this as issue.
Moreover achieving eventual consistency here is probably not possible as
there is still some latency between "writeb" call and real reflection of
LED color on the hardware.

> > +		}
> > +	}
> > +
> > +	/* Enable / disable LED for software control */
> > +	val = readb(leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
> > +	if (brightness == LED_ON && (val & BIT(led->reg)))
> > +		writeb(val & ~BIT(led->reg),
> > +		       leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
> > +	else if (brightness == LED_OFF && !(val & BIT(led->reg)))
> > +		writeb(val | BIT(led->reg),
> > +		       leds->regs + TURRIS1X_LED_SW_DISABLE_REG);
> 
> Pls just use brightness or !brightness.

Ok!

> Or maybe just write the value?
> Is the register write expensive on the CPLD or why are you trying to
> avoid it if unnecessary?

I just do not see any reason to do unnecessary writes.

> > +	mutex_unlock(&leds->lock);
> > +
> > +	return 0;
> > +}
> > +
> > +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;
> > +
> > +	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);
> 
> Weird that checkpatch doesn't warn about spaces around the minus
> operator in ARRAY_SIZE()-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;
> > +
> > +	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;
> > +	led->mc_cdev.num_colors = ARRAY_SIZE(led->subled_info);
> > +
> > +	init_data.fwnode = &np->fwnode;
> > +
> > +	cdev = &led->mc_cdev.led_cdev;
> > +	cdev->max_brightness = LED_ON;
> 
> s/LED_ON/1/
> 
> > +	cdev->brightness_get = turris1x_led_brightness_get;
> > +	cdev->brightness_set_blocking = turris1x_led_brightness_set_blocking;
> 
> See above about _blocking.
> 
> > +
> > +	/* 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);
> 
> Again, is the register write that expensive that you try to avoid it if
> the corresponding bit has the value you want?
> 
> > +	}
> > +
> > +	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 = LED_ON;
> 
> s/LED_ON/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.
> > +	 */
> 
> Hmm. Wouldn't it make more sense to simply have the global brightness
> accept values from 0 to 7, instead of mapping it to 256 values? And
> call it something like selected_brightness_index?

All other drivers have brightness entry which operates on monotone
brightness property.

Brightness levels do not have to be monotone and by default are
decreasing: 0 = brightness with higher intensity; 7 = no intensity (off)

I cannot image who would like or prefer usage of such API.

Just stick with existing APIs. "brightness" entry takes intensity value
which is monotone, 0 the lowest, MAX (=255) the highest.

For setting individual level values I introduced a new entry
"brightness_values".

> > +	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_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_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 const struct of_device_id of_turris1x_leds_match[] = {
> > +	{ .compatible = "cznic,turris1x-leds" },
> > +	{},
> > +};
> > +
> > +static struct platform_driver turris1x_leds_driver = {
> > +	.probe = turris1x_leds_probe,
> > +	.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");
> 

Re: [PATCH 2/2] leds: Add support for Turris 1.x LEDs

[Marek Behún]

On Tue, 5 Jul 2022 12:56:09 +0200
Pali Rohár <pali@kernel.org> wrote:

> > 
> > I don't consider this a problem  
> 
> I think it is a problem, to ensure that 'cat multi_intensity' for every

Misunderstanding. I meant that I don't consider the eventual
inconsistency a problem, i.e. I agree with your code.

> > Or maybe just write the value?
> > Is the register write expensive on the CPLD or why are you trying to
> > avoid it if unnecessary?  
> 
> I just do not see any reason to do unnecessary writes.

But now you do an unnecessary check. Unless the writeb() is slower than
that check. Since this isn't i2c, I am wondering how fast that writeb()
is... But this is just me wondering, we can keep it the way you wrote
it...

> > 
> > Hmm. Wouldn't it make more sense to simply have the global brightness
> > accept values from 0 to 7, instead of mapping it to 256 values? And
> > call it something like selected_brightness_index?  
> 
> All other drivers have brightness entry which operates on monotone
> brightness property.
> Brightness levels do not have to be monotone and by default are
> decreasing: 0 = brightness with higher intensity; 7 = no intensity (off)

What do you mean all other drivers? AFAIK only one driver does this
global brightness thing, and that is Omnia. The global brightness is
something different from LED cdev brightness property, the same names
are just coincidental (in fact it caused confusion when Pavel was
first reviewing Turris Omnia driver). Maybe it should have been called
global_intensity, to avoid the confusion...

> I cannot image who would like or prefer usage of such API.

One file that represents the index of the selected global intensity (as
is stored internally in the CPLD) and another file that represents the
configured intensities between which the button switches makes sense,
IMO.

> Just stick with existing APIs. "brightness" entry takes intensity value
> which is monotone, 0 the lowest, MAX (=255) the highest.

Again, the name "brightness" does not imply that it is the same thing
as "brightness" of a LED cdev. And since it even doesn't live in
/sys/class/<led>/ directory, we are proposing new API and can use
whatever makes sense.

I am not saying that the way you did it doesn't make sense. I am just
wondering if it wouldn't make more sense to be able to read the index
of what the user selected by button pressing.

Marek

Re: [PATCH 2/2] leds: Add support for Turris 1.x LEDs

[Pali Rohár]

On Tuesday 05 July 2022 13:52:27 Marek Behún wrote:
> On Tue, 5 Jul 2022 12:56:09 +0200
> Pali Rohár <pali@kernel.org> wrote:
> 
> > > 
> > > I don't consider this a problem  
> > 
> > I think it is a problem, to ensure that 'cat multi_intensity' for every
> 
> Misunderstanding. I meant that I don't consider the eventual
> inconsistency a problem, i.e. I agree with your code.
> 
> > > Or maybe just write the value?
> > > Is the register write expensive on the CPLD or why are you trying to
> > > avoid it if unnecessary?  
> > 
> > I just do not see any reason to do unnecessary writes.
> 
> But now you do an unnecessary check.

I think that testing if some bit is set in 32-bit general purpose
processor register is something which really does not play role here.

Note that readb() is always needed to do because it is required to
modify just one bit and this cannot be done without read-modify-write
operations.

> Unless the writeb() is slower than
> that check. Since this isn't i2c, I am wondering how fast that writeb()
> is... But this is just me wondering, we can keep it the way you wrote
> it...
> 
> > > 
> > > Hmm. Wouldn't it make more sense to simply have the global brightness
> > > accept values from 0 to 7, instead of mapping it to 256 values? And
> > > call it something like selected_brightness_index?  
> > 
> > All other drivers have brightness entry which operates on monotone
> > brightness property.
> > Brightness levels do not have to be monotone and by default are
> > decreasing: 0 = brightness with higher intensity; 7 = no intensity (off)
> 
> What do you mean all other drivers? AFAIK only one driver does this
> global brightness thing, and that is Omnia. The global brightness is
> something different from LED cdev brightness property, the same names
> are just coincidental (in fact it caused confusion when Pavel was
> first reviewing Turris Omnia driver). Maybe it should have been called
> global_intensity, to avoid the confusion...

Ok. I thought "brightness" == "brightness" too.

Anyway, as Omnia has this API it makes sense to use same API for other
devices, this allows userspace software to be compatible with more
devices.

> > I cannot image who would like or prefer usage of such API.
> 
> One file that represents the index of the selected global intensity (as
> is stored internally in the CPLD) and another file that represents the
> configured intensities between which the button switches makes sense,
> IMO.

And this is the issue. If you want to get current brightness, you need
to read two files and then do non-trivial logic to derive current
brightness.

> > Just stick with existing APIs. "brightness" entry takes intensity value
> > which is monotone, 0 the lowest, MAX (=255) the highest.
> 
> Again, the name "brightness" does not imply that it is the same thing
> as "brightness" of a LED cdev. And since it even doesn't live in
> /sys/class/<led>/ directory, we are proposing new API and can use
> whatever makes sense.
> 
> I am not saying that the way you did it doesn't make sense. I am just
> wondering if it wouldn't make more sense to be able to read the index
> of what the user selected by button pressing.
> 
> Marek

So what about exporting another sysfs file which controls current level (0-7)?

Re: [PATCH 2/2] leds: Add support for Turris 1.x LEDs

[Marek Behún]

On Tue, 5 Jul 2022 14:22:38 +0200
Pali Rohár <pali@kernel.org> wrote:

> On Tuesday 05 July 2022 13:52:27 Marek Behún wrote:
> > On Tue, 5 Jul 2022 12:56:09 +0200
> > Pali Rohár <pali@kernel.org> wrote:
> >   
> > > > 
> > > > I don't consider this a problem    
> > > 
> > > I think it is a problem, to ensure that 'cat multi_intensity' for every  
> > 
> > Misunderstanding. I meant that I don't consider the eventual
> > inconsistency a problem, i.e. I agree with your code.
> >   
> > > > Or maybe just write the value?
> > > > Is the register write expensive on the CPLD or why are you trying to
> > > > avoid it if unnecessary?    
> > > 
> > > I just do not see any reason to do unnecessary writes.  
> > 
> > But now you do an unnecessary check.  
> 
> I think that testing if some bit is set in 32-bit general purpose
> processor register is something which really does not play role here.
> 
> Note that readb() is always needed to do because it is required to
> modify just one bit and this cannot be done without read-modify-write
> operations.
> 
> > Unless the writeb() is slower than
> > that check. Since this isn't i2c, I am wondering how fast that writeb()
> > is... But this is just me wondering, we can keep it the way you wrote
> > it...
> >   
> > > > 
> > > > Hmm. Wouldn't it make more sense to simply have the global brightness
> > > > accept values from 0 to 7, instead of mapping it to 256 values? And
> > > > call it something like selected_brightness_index?    
> > > 
> > > All other drivers have brightness entry which operates on monotone
> > > brightness property.
> > > Brightness levels do not have to be monotone and by default are
> > > decreasing: 0 = brightness with higher intensity; 7 = no intensity (off)  
> > 
> > What do you mean all other drivers? AFAIK only one driver does this
> > global brightness thing, and that is Omnia. The global brightness is
> > something different from LED cdev brightness property, the same names
> > are just coincidental (in fact it caused confusion when Pavel was
> > first reviewing Turris Omnia driver). Maybe it should have been called
> > global_intensity, to avoid the confusion...  
> 
> Ok. I thought "brightness" == "brightness" too.
> 
> Anyway, as Omnia has this API it makes sense to use same API for other
> devices, this allows userspace software to be compatible with more
> devices.
> 
> > > I cannot image who would like or prefer usage of such API.  
> > 
> > One file that represents the index of the selected global intensity (as
> > is stored internally in the CPLD) and another file that represents the
> > configured intensities between which the button switches makes sense,
> > IMO.  
> 
> And this is the issue. If you want to get current brightness, you need
> to read two files and then do non-trivial logic to derive current
> brightness.
> 
> > > Just stick with existing APIs. "brightness" entry takes intensity value
> > > which is monotone, 0 the lowest, MAX (=255) the highest.  
> > 
> > Again, the name "brightness" does not imply that it is the same thing
> > as "brightness" of a LED cdev. And since it even doesn't live in
> > /sys/class/<led>/ directory, we are proposing new API and can use
> > whatever makes sense.
> > 
> > I am not saying that the way you did it doesn't make sense. I am just
> > wondering if it wouldn't make more sense to be able to read the index
> > of what the user selected by button pressing.
> > 
> > Marek  
> 
> So what about exporting another sysfs file which controls current level (0-7)?

OK, that would be satisfactory. Something like
"selected_brightness_index".

Marek

Re: [PATCH 2/2] leds: Add support for Turris 1.x LEDs

[Pali Rohár]

On Tuesday 05 July 2022 14:30:01 Marek Behún wrote:
> On Tue, 5 Jul 2022 14:22:38 +0200
> Pali Rohár <pali@kernel.org> wrote:
> > So what about exporting another sysfs file which controls current level (0-7)?
> 
> OK, that would be satisfactory. Something like
> "selected_brightness_index".

Too long? What about just "brightness_level" and make it R/W?
We already call it level (not index).

Re: [PATCH 2/2] leds: Add support for Turris 1.x LEDs

[Marek Behún]

On Tue, 5 Jul 2022 14:32:06 +0200
Pali Rohár <pali@kernel.org> wrote:

> On Tuesday 05 July 2022 14:30:01 Marek Behún wrote:
> > On Tue, 5 Jul 2022 14:22:38 +0200
> > Pali Rohár <pali@kernel.org> wrote:  
> > > So what about exporting another sysfs file which controls current level (0-7)?  
> > 
> > OK, that would be satisfactory. Something like
> > "selected_brightness_index".  
> 
> Too long? What about just "brightness_level" and make it R/W?
> We already call it level (not index).

ok

Re: [PATCH 1/2] dt-bindings: leds: Add cznic,turris1x-leds.yaml binding

[Krzysztof Kozlowski]

On 05/07/2022 02:04, 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    | 116 ++++++++++++++++++
>  1 file changed, 116 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..fd09613c8d2d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/cznic,turris1x-leds.yaml
> @@ -0,0 +1,116 @@
> +# 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:
> +    maxItems: 2

You need to describe the items, if it is really two items. However your
example has only one item, so this was not tested and won't work.

You'll get warning from Rob's robot soon... but you should test the
bindings instead.

> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +patternProperties:
> +  "^multi-led@[0-7]$":
> +    type: object
> +    $ref: leds-class-multicolor.yaml#

This looks incorrect, unless you rebased on my patchset?

> +
> +    properties:
> +      reg:
> +        minimum: 0
> +        maximum: 7
> +
> +    required:
> +      - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +

No blank line.

> +    #include <dt-bindings/leds/common.h>
> +
> +    cpld@3,0 {

Generic node name.

> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +
> +        led-controller@13 {
> +            compatible = "cznic,turris1x-leds";
> +            reg = <0x13 0x1d>;
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +


Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: leds: Add cznic,turris1x-leds.yaml binding

[Pali Rohár]

On Tuesday 05 July 2022 13:36:54 Krzysztof Kozlowski wrote:
> On 05/07/2022 02:04, 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    | 116 ++++++++++++++++++
> >  1 file changed, 116 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..fd09613c8d2d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/leds/cznic,turris1x-leds.yaml
> > @@ -0,0 +1,116 @@
> > +# 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:
> > +    maxItems: 2
> 
> You need to describe the items, if it is really two items. However your
> example has only one item, so this was not tested and won't work.

Ehm? Example has two items in the reg.

> You'll get warning from Rob's robot soon... but you should test the
> bindings instead.

I have tested bindings on the real hardware and it is working fine
together with the driver from patch 2/2.

> > +
> > +  "#address-cells":
> > +    const: 1
> > +
> > +  "#size-cells":
> > +    const: 0
> > +
> > +patternProperties:
> > +  "^multi-led@[0-7]$":
> > +    type: object
> > +    $ref: leds-class-multicolor.yaml#
> 
> This looks incorrect, unless you rebased on my patchset?

So what is the correct? (I used inspiration from
cznic,turris-omnia-leds.yaml file)

> > +
> > +    properties:
> > +      reg:
> > +        minimum: 0
> > +        maximum: 7
> > +
> > +    required:
> > +      - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +
> 
> No blank line.

Ok.

> > +    #include <dt-bindings/leds/common.h>
> > +
> > +    cpld@3,0 {
> 
> Generic node name.

Is not cpld name generic enough?

> > +        #address-cells = <1>;
> > +        #size-cells = <1>;
> > +
> > +        led-controller@13 {
> > +            compatible = "cznic,turris1x-leds";
> > +            reg = <0x13 0x1d>;
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH 1/2] dt-bindings: leds: Add cznic,turris1x-leds.yaml binding

[Krzysztof Kozlowski]

On 05/07/2022 13:42, Pali Rohár wrote:
> On Tuesday 05 July 2022 13:36:54 Krzysztof Kozlowski wrote:
>> On 05/07/2022 02:04, 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    | 116 ++++++++++++++++++
>>>  1 file changed, 116 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..fd09613c8d2d
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/leds/cznic,turris1x-leds.yaml
>>> @@ -0,0 +1,116 @@
>>> +# 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:
>>> +    maxItems: 2
>>
>> You need to describe the items, if it is really two items. However your
>> example has only one item, so this was not tested and won't work.
> 
> Ehm? Example has two items in the reg.

No, you have exactly one item.
<0x13 0x1d>

Two items are for example:
<0x13 0x1d>, <0x23 0x1d>

> 
>> You'll get warning from Rob's robot soon... but you should test the
>> bindings instead.
> 
> I have tested bindings on the real hardware and it is working fine
> together with the driver from patch 2/2.

Bindings cannot be tested on real hardware. Bindings are tested with
dt_binding_check, as explained in writing-schema.rst

> 
>>> +
>>> +  "#address-cells":
>>> +    const: 1
>>> +
>>> +  "#size-cells":
>>> +    const: 0
>>> +
>>> +patternProperties:
>>> +  "^multi-led@[0-7]$":
>>> +    type: object
>>> +    $ref: leds-class-multicolor.yaml#
>>
>> This looks incorrect, unless you rebased on my patchset?
> 
> So what is the correct? (I used inspiration from
> cznic,turris-omnia-leds.yaml file)

Which according to current multicolor bindings is not correct. Correct
is pwm-multicolor. However if you rebase on [1], it looks fine, except
missing unevaluatedProperties.

[1]
https://lore.kernel.org/all/20220624112106.111351-1-krzysztof.kozlowski@linaro.org/

> 
>>> +
>>> +    properties:
>>> +      reg:
>>> +        minimum: 0
>>> +        maximum: 7
>>> +
>>> +    required:
>>> +      - reg
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +
>>
>> No blank line.
> 
> Ok.
> 
>>> +    #include <dt-bindings/leds/common.h>
>>> +
>>> +    cpld@3,0 {
>>
>> Generic node name.
> 
> Is not cpld name generic enough?

No, it means nothing to me. Just like "a", "ashjd" or "wrls".

"The name of a node should be somewhat generic, reflecting the function
of the device and not its precise programming
 model. If appropriate, the name should be one of the following choices:"

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: leds: Add cznic,turris1x-leds.yaml binding

[Pali Rohár]

On Tuesday 05 July 2022 13:51:12 Krzysztof Kozlowski wrote:
> On 05/07/2022 13:42, Pali Rohár wrote:
> > On Tuesday 05 July 2022 13:36:54 Krzysztof Kozlowski wrote:
> >> On 05/07/2022 02:04, 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    | 116 ++++++++++++++++++
> >>>  1 file changed, 116 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..fd09613c8d2d
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/leds/cznic,turris1x-leds.yaml
> >>> @@ -0,0 +1,116 @@
> >>> +# 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:
> >>> +    maxItems: 2
> >>
> >> You need to describe the items, if it is really two items. However your
> >> example has only one item, so this was not tested and won't work.
> > 
> > Ehm? Example has two items in the reg.
> 
> No, you have exactly one item.
> <0x13 0x1d>
> 
> Two items are for example:
> <0x13 0x1d>, <0x23 0x1d>

Ok. So I should change maxItems to 1 in this case?

And how you want to describe those items?

> > 
> >> You'll get warning from Rob's robot soon... but you should test the
> >> bindings instead.
> > 
> > I have tested bindings on the real hardware and it is working fine
> > together with the driver from patch 2/2.
> 
> Bindings cannot be tested on real hardware. Bindings are tested with
> dt_binding_check, as explained in writing-schema.rst

Ou... this is something which I was not able to run, it just does not
work, throws lot of python dependency hell errors and it spend more than
hour with it. So sorry, I really cannot run it. Maybe it would be a wise
to provide web service for these checks for those who cannot run them
locally?

> > 
> >>> +
> >>> +  "#address-cells":
> >>> +    const: 1
> >>> +
> >>> +  "#size-cells":
> >>> +    const: 0
> >>> +
> >>> +patternProperties:
> >>> +  "^multi-led@[0-7]$":
> >>> +    type: object
> >>> +    $ref: leds-class-multicolor.yaml#
> >>
> >> This looks incorrect, unless you rebased on my patchset?
> > 
> > So what is the correct? (I used inspiration from
> > cznic,turris-omnia-leds.yaml file)
> 
> Which according to current multicolor bindings is not correct. Correct
> is pwm-multicolor. However if you rebase on [1], it looks fine, except
> missing unevaluatedProperties.

Ok. So does it mean that I should just add
"unevaluatedProperties: false"?

> [1]
> https://lore.kernel.org/all/20220624112106.111351-1-krzysztof.kozlowski@linaro.org/
> 
> > 
> >>> +
> >>> +    properties:
> >>> +      reg:
> >>> +        minimum: 0
> >>> +        maximum: 7
> >>> +
> >>> +    required:
> >>> +      - reg
> >>> +
> >>> +additionalProperties: false
> >>> +
> >>> +examples:
> >>> +  - |
> >>> +
> >>
> >> No blank line.
> > 
> > Ok.
> > 
> >>> +    #include <dt-bindings/leds/common.h>
> >>> +
> >>> +    cpld@3,0 {
> >>
> >> Generic node name.
> > 
> > Is not cpld name generic enough?
> 
> No, it means nothing to me. Just like "a", "ashjd" or "wrls".

If you never heard about it, I would suggest to read something about
Programmable logic devices. It is interesting category of hardware with
which you can play. CPLD and FPGA are very often used in lot of products
and FPGA is very easy for playing and programming custom logic.

For example on wikipedia is list of different technologies of
programmable logic devices:
https://en.wikipedia.org/wiki/Programmable_logic_device

So if you want more generic name, just name it "pld"? But as it is CPLD
device I would suggest to name it really as "cpld". It does not matter
from which manufactor you have CPLD, just like it does not matter from
which manufactor you have NAND.

From bus point of view, cpld is like nand or nor nodes in DTS. All of
them refers to specific memory map of chip selects on the local bus.

> "The name of a node should be somewhat generic, reflecting the function
> of the device and not its precise programming
>  model. If appropriate, the name should be one of the following choices:"

Hm... You forgot to send what are those "choices:"?

> Best regards,
> Krzysztof

Re: [PATCH 1/2] dt-bindings: leds: Add cznic,turris1x-leds.yaml binding

[Krzysztof Kozlowski]

On 05/07/2022 14:15, Pali Rohár wrote:
>>>> You need to describe the items, if it is really two items. However your
>>>> example has only one item, so this was not tested and won't work.
>>>
>>> Ehm? Example has two items in the reg.
>>
>> No, you have exactly one item.
>> <0x13 0x1d>
>>
>> Two items are for example:
>> <0x13 0x1d>, <0x23 0x1d>
> 
> Ok. So I should change maxItems to 1 in this case?

Yes

> 
> And how you want to describe those items?

In that case, no need to describe.

> 
>>>
>>>> You'll get warning from Rob's robot soon... but you should test the
>>>> bindings instead.
>>>
>>> I have tested bindings on the real hardware and it is working fine
>>> together with the driver from patch 2/2.
>>
>> Bindings cannot be tested on real hardware. Bindings are tested with
>> dt_binding_check, as explained in writing-schema.rst
> 
> Ou... this is something which I was not able to run, it just does not
> work, throws lot of python dependency hell errors and it spend more than
> hour with it. So sorry, I really cannot run it. Maybe it would be a wise
> to provide web service for these checks for those who cannot run them
> locally?

It's one pip command to install and one make command to run... I would
say easy to start using, unless of course you use some unusual distro
without Python 3 (cannot believe nowadays...) or without pip.

Rob's bot will test it for you.

Anyway, in such case please mark your bindings always as RFT, so we will
not waste time on reviewing obvious stuff which is found by automated
tools. I think we both agree that reviewers time should not be used for
trivial stuff already pointed out by compiler/linter/automation.

> 
>>>
>>>>> +
>>>>> +  "#address-cells":
>>>>> +    const: 1
>>>>> +
>>>>> +  "#size-cells":
>>>>> +    const: 0
>>>>> +
>>>>> +patternProperties:
>>>>> +  "^multi-led@[0-7]$":
>>>>> +    type: object
>>>>> +    $ref: leds-class-multicolor.yaml#
>>>>
>>>> This looks incorrect, unless you rebased on my patchset?
>>>
>>> So what is the correct? (I used inspiration from
>>> cznic,turris-omnia-leds.yaml file)
>>
>> Which according to current multicolor bindings is not correct. Correct
>> is pwm-multicolor. However if you rebase on [1], it looks fine, except
>> missing unevaluatedProperties.
> 
> Ok. So does it mean that I should just add
> "unevaluatedProperties: false"?

Yes, on that level of indentation, so:
    $ref: leds-class-multicolor.yaml#
    unevaluatedProperties: false


> 
>> [1]
>> https://lore.kernel.org/all/20220624112106.111351-1-krzysztof.kozlowski@linaro.org/
>>
>>>
>>>>> +
>>>>> +    properties:
>>>>> +      reg:
>>>>> +        minimum: 0
>>>>> +        maximum: 7
>>>>> +
>>>>> +    required:
>>>>> +      - reg
>>>>> +
>>>>> +additionalProperties: false
>>>>> +
>>>>> +examples:
>>>>> +  - |
>>>>> +
>>>>
>>>> No blank line.
>>>
>>> Ok.
>>>
>>>>> +    #include <dt-bindings/leds/common.h>
>>>>> +
>>>>> +    cpld@3,0 {
>>>>
>>>> Generic node name.
>>>
>>> Is not cpld name generic enough?
>>
>> No, it means nothing to me. Just like "a", "ashjd" or "wrls".
> 
> If you never heard about it, I would suggest to read something about
> Programmable logic devices. It is interesting category of hardware with
> which you can play. CPLD and FPGA are very often used in lot of products
> and FPGA is very easy for playing and programming custom logic.

The are many different acronyms in the language so without context might
be tricky to connect the dots.

> 
> For example on wikipedia is list of different technologies of
> programmable logic devices:
> https://en.wikipedia.org/wiki/Programmable_logic_device
> 
> So if you want more generic name, just name it "pld"? 

That one would be fine.

> But as it is CPLD
> device I would suggest to name it really as "cpld". It does not matter
> from which manufactor you have CPLD, just like it does not matter from
> which manufactor you have NAND.

Then cpld is fine as well.

> 
> From bus point of view, cpld is like nand or nor nodes in DTS. All of
> them refers to specific memory map of chip selects on the local bus.
> 
>> "The name of a node should be somewhat generic, reflecting the function
>> of the device and not its precise programming
>>  model. If appropriate, the name should be one of the following choices:"
> 
> Hm... You forgot to send what are those "choices:"?

I didn't, I just assumed you will Google it (or use other web-search
engine of your choice) to get the spec. As this is a quote, Google
results should be very accurate. No need to duplicate entire pages of
publicly available specification.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: leds: Add cznic,turris1x-leds.yaml binding

[Pali Rohár]

On Tuesday 05 July 2022 14:55:10 Krzysztof Kozlowski wrote:
> On 05/07/2022 14:15, Pali Rohár wrote:
> >>>> You need to describe the items, if it is really two items. However your
> >>>> example has only one item, so this was not tested and won't work.
> >>>
> >>> Ehm? Example has two items in the reg.
> >>
> >> No, you have exactly one item.
> >> <0x13 0x1d>
> >>
> >> Two items are for example:
> >> <0x13 0x1d>, <0x23 0x1d>
> > 
> > Ok. So I should change maxItems to 1 in this case?
> 
> Yes
> 
> > 
> > And how you want to describe those items?
> 
> In that case, no need to describe.
> 
> > 
> >>>
> >>>> You'll get warning from Rob's robot soon... but you should test the
> >>>> bindings instead.
> >>>
> >>> I have tested bindings on the real hardware and it is working fine
> >>> together with the driver from patch 2/2.
> >>
> >> Bindings cannot be tested on real hardware. Bindings are tested with
> >> dt_binding_check, as explained in writing-schema.rst
> > 
> > Ou... this is something which I was not able to run, it just does not
> > work, throws lot of python dependency hell errors and it spend more than
> > hour with it. So sorry, I really cannot run it. Maybe it would be a wise
> > to provide web service for these checks for those who cannot run them
> > locally?
> 
> It's one pip command to install and one make command to run... I would
> say easy to start using, unless of course you use some unusual distro
> without Python 3 (cannot believe nowadays...) or without pip.
> 
> Rob's bot will test it for you.

Ok, so lets wait for the robot. After that I will try to fix found
issues and send a new patch version.

> Anyway, in such case please mark your bindings always as RFT, so we will
> not waste time on reviewing obvious stuff which is found by automated
> tools. I think we both agree that reviewers time should not be used for
> trivial stuff already pointed out by compiler/linter/automation.

Yes!

> > 
> >>>
> >>>>> +
> >>>>> +  "#address-cells":
> >>>>> +    const: 1
> >>>>> +
> >>>>> +  "#size-cells":
> >>>>> +    const: 0
> >>>>> +
> >>>>> +patternProperties:
> >>>>> +  "^multi-led@[0-7]$":
> >>>>> +    type: object
> >>>>> +    $ref: leds-class-multicolor.yaml#
> >>>>
> >>>> This looks incorrect, unless you rebased on my patchset?
> >>>
> >>> So what is the correct? (I used inspiration from
> >>> cznic,turris-omnia-leds.yaml file)
> >>
> >> Which according to current multicolor bindings is not correct. Correct
> >> is pwm-multicolor. However if you rebase on [1], it looks fine, except
> >> missing unevaluatedProperties.
> > 
> > Ok. So does it mean that I should just add
> > "unevaluatedProperties: false"?
> 
> Yes, on that level of indentation, so:
>     $ref: leds-class-multicolor.yaml#
>     unevaluatedProperties: false

Ok.

> > 
> >> [1]
> >> https://lore.kernel.org/all/20220624112106.111351-1-krzysztof.kozlowski@linaro.org/
> >>
> >>>
> >>>>> +
> >>>>> +    properties:
> >>>>> +      reg:
> >>>>> +        minimum: 0
> >>>>> +        maximum: 7
> >>>>> +
> >>>>> +    required:
> >>>>> +      - reg
> >>>>> +
> >>>>> +additionalProperties: false
> >>>>> +
> >>>>> +examples:
> >>>>> +  - |
> >>>>> +
> >>>>
> >>>> No blank line.
> >>>
> >>> Ok.
> >>>
> >>>>> +    #include <dt-bindings/leds/common.h>
> >>>>> +
> >>>>> +    cpld@3,0 {
> >>>>
> >>>> Generic node name.
> >>>
> >>> Is not cpld name generic enough?
> >>
> >> No, it means nothing to me. Just like "a", "ashjd" or "wrls".
> > 
> > If you never heard about it, I would suggest to read something about
> > Programmable logic devices. It is interesting category of hardware with
> > which you can play. CPLD and FPGA are very often used in lot of products
> > and FPGA is very easy for playing and programming custom logic.
> 
> The are many different acronyms in the language so without context might
> be tricky to connect the dots.

Anyway, playing with FPGA is really a fun!

> > 
> > For example on wikipedia is list of different technologies of
> > programmable logic devices:
> > https://en.wikipedia.org/wiki/Programmable_logic_device
> > 
> > So if you want more generic name, just name it "pld"? 
> 
> That one would be fine.
> 
> > But as it is CPLD
> > device I would suggest to name it really as "cpld". It does not matter
> > from which manufactor you have CPLD, just like it does not matter from
> > which manufactor you have NAND.
> 
> Then cpld is fine as well.

Ok, so stick with cpld.

> > 
> > From bus point of view, cpld is like nand or nor nodes in DTS. All of
> > them refers to specific memory map of chip selects on the local bus.
> > 
> >> "The name of a node should be somewhat generic, reflecting the function
> >> of the device and not its precise programming
> >>  model. If appropriate, the name should be one of the following choices:"
> > 
> > Hm... You forgot to send what are those "choices:"?
> 
> I didn't, I just assumed you will Google it (or use other web-search
> engine of your choice) to get the spec. As this is a quote, Google
> results should be very accurate. No need to duplicate entire pages of
> publicly available specification.

This was the first thing which I did when I read email. No usable
result. So the next thing was that I started git grep on the linux tree.
Again no result. So at the end I come to the conclusion that you forgot
to copy+paste whole quote or something like that.

Now I started searching a bit more and found it in following documentation:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Original link to the quote would be useful (but now I have it).

> Best regards,
> Krzysztof

Re: [PATCH 1/2] dt-bindings: leds: Add cznic,turris1x-leds.yaml binding

[Krzysztof Kozlowski]

On 05/07/2022 15:05, Pali Rohár wrote:
> 
> This was the first thing which I did when I read email. No usable
> result. So the next thing was that I started git grep on the linux tree.
> Again no result. So at the end I come to the conclusion that you forgot
> to copy+paste whole quote or something like that.
> 
> Now I started searching a bit more and found it in following documentation:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> 
> Original link to the quote would be useful (but now I have it).

Good point, thanks for the link. I forgot that devicetree spec is also
available as webpage.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: leds: Add cznic,turris1x-leds.yaml binding

[Marek Behún]

On Tue, 5 Jul 2022 13:42:38 +0200
Pali Rohár <pali@kernel.org> wrote:

> I have tested bindings on the real hardware and it is working fine
> together with the driver from patch 2/2.

I think Krzysztof meant testing with dtschema.

Marek

Re: [PATCH 1/2] dt-bindings: leds: Add cznic,turris1x-leds.yaml binding

[Rob Herring]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1422 bytes --]

On Tue, 05 Jul 2022 02:04:47 +0200, 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    | 116 ++++++++++++++++++
>  1 file changed, 116 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/cznic,turris1x-leds.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/leds/cznic,turris1x-leds.example.dts:21.18-84.11: Warning (unit_address_vs_reg): /example-0/cpld@3,0: node has a unit name, but no reg or ranges property
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/leds/cznic,turris1x-leds.example.dtb: led-controller@13: reg: [[19, 29]] is too short
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/leds/cznic,turris1x-leds.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

[PATCH v2 1/2] [RFT] dt-bindings: leds: Add cznic,turris1x-leds.yaml binding

[Pali Rohár]

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

Signed-off-by: Pali Rohár <pali@kernel.org>

---
Changes in v2:
* Fix schema errors
---
 .../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

[PATCH v2 2/2] leds: Add support for Turris 1.x LEDs

[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 <pali@kernel.org>

---
Changes in v2:
* Use 0/1 instead of LED_OFF/LED_ON
* Add brightness_level device attribute
* Implement callback brightness_set instead of brightness_set_blocking
* Reset LEDs to default state before kernel reboots
---
 .../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..bb8b82b43165
--- /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:	5.20
+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 (from 100% to 0%), 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:	5.20
+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:	5.20
+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 a49979f41eee..71caf45c8ac3 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", &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

Re: [PATCH v2 2/2] leds: Add support for Turris 1.x LEDs

[Marek Behún]

On Tue,  5 Jul 2022 17:59:29 +0200
Pali Rohár <pali@kernel.org> 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>

Re: [PATCH v2 2/2] leds: Add support for Turris 1.x LEDs

[Andy Shevchenko]

On Tue, Jul 5, 2022 at 6:11 PM Pali Rohár <pali@kernel.org> 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.

Pardon me, but this driver seems like 3 years old by the APIs it's
using... I have to say this, because I was surprised a lot to see some
calls.

...

> +config LEDS_TURRIS_1X
> +       tristate "LED support for CZ.NIC's Turris 1.x"
> +       depends on LEDS_CLASS_MULTICOLOR

> +       depends on OF

Why?

If it's a functional (not compile time) dependency, make it

  depends on OF || COMPILE_TEST

> +       select LEDS_TRIGGERS
> +       help
> +         This option enables support for LEDs found on the front side of
> +         CZ.NIC's Turris 1.x routers.

...

> +#include <linux/i2c.h>
> +#include <linux/led-class-multicolor.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>

> +#include <linux/of.h>

Rather property.h. See below how.

...

> +/* LEDs 1-5 share common register for setting brightness */
> +#define TURRIS1X_LED_BRIGHTNESS_OFF(idx)       ({ const u8 _idx = (idx) & 7; \

Can you start with the GCC expression on a new line? I may give a much
shorter next line and increase readability.

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

Ditto.

...

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

Redundant 'else' in both cases.

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

chaining

> +                * reflect color change for all of them.
> +                */

> +               if (led->reg >= 1 && led->reg <= 5) {

Same is used in the macro above. Maybe you can provide a shortcut for
this instead of duplicating?

> +                       for (j = 0; j < ARRAY_SIZE(leds->leds); j++) {

> +                               if (leds->leds[j].reg < 1 ||
> +                                   leds->leds[j].reg > 5 ||

Ditto.

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

...

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

> +       init_data.fwnode = &np->fwnode;

Oh, no. We do not dereference fwnode, we have helpers for that.
Moreover, why not use fwnode to begin with?

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

sysfs_emit()

> +}

...

> +       if (kstrtoul(buf, 10, &brightness))
> +               return -EINVAL;

Why shadow the error code from kstrtoul()? Note it might return
something different.

Do you really need unsigned long? Can't you use u8 and kstrtou8() respectively?

> +       if (brightness > 255)
> +               return -EINVAL;

Yeah, read above about u8.

...

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

a level
the nearest

> +        */

...

> +               error = abs(value - (int)brightness);

Why casting?!

...

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

sysfs_emit()

> +}

...

> +       if (kstrtoul(buf, 10, &level))
> +               return -EINVAL;

As per above.

...

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

sysfs_emit()
Wouldn't it be better to have CSV instead? I think for such cases we
usually have this kind of format.

> +}

...

> +static struct attribute *turris1x_leds_controller_attrs[] = {
> +       &dev_attr_brightness.attr,
> +       &dev_attr_brightness_level.attr,
> +       &dev_attr_brightness_values.attr,

> +       NULL,

No comma for terminator.

> +};

...

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

Why not use fwnode to begin with?

> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

> +       if (!res)
> +               return -ENODEV;

Besides dup code, which actually does not print a message...

> +       regs = devm_ioremap_resource(dev, res);

...we have devm_platform_ioremap_resource() to combine two above into one.

> +       if (IS_ERR(regs))
> +               return PTR_ERR(regs);

...

> +       ret = devm_led_trigger_register(dev, &turris1x_hw_trigger);
> +       if (ret) {
> +               dev_err(dev, "Cannot register private LED trigger: %d\n", ret);
> +               return ret;

return dev_err_probe(...);

> +       }

...

> +       for_each_available_child_of_node(np, child) {

device_for_each_child_node()

> +               ret = turris1x_led_register(dev, leds, child,
> +                                           val_sw_override, val_sw_disable);
> +               if (ret) {

> +                       of_node_put(child);

fwnode_handle_put()

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

> +       }
> +
> +       return 0;
> +}

...

> +       /*
> +        * LED registers are persisent across board resets.

persistent

> +        * So reset LEDs to default state before kernel reboots.
> +        */

...

> +                       writeb(0xff,

GENMASK() ?

> +                              leds->regs + TURRIS1X_LED_BRIGHTNESS_REG(i, j));
> +       }
> +}

...

> +static const struct of_device_id of_turris1x_leds_match[] = {
> +       { .compatible = "cznic,turris1x-leds" },
> +       {},

No comma for terminator.

> +};

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] leds: Add support for Turris 1.x LEDs

[Pali Rohár]

On Tuesday 05 July 2022 20:40:26 Andy Shevchenko wrote:
> On Tue, Jul 5, 2022 at 6:11 PM Pali Rohár <pali@kernel.org> 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.
> 
> Pardon me, but this driver seems like 3 years old by the APIs it's
> using... I have to say this, because I was surprised a lot to see some
> calls.

I wrote it just recently according to other omnia multicolor driver.

Re: [PATCH v2 2/2] leds: Add support for Turris 1.x LEDs

[Andy Shevchenko]

On Tue, Jul 5, 2022 at 8:47 PM Pali Rohár <pali@kernel.org> wrote:
>
> On Tuesday 05 July 2022 20:40:26 Andy Shevchenko wrote:
> > On Tue, Jul 5, 2022 at 6:11 PM Pali Rohár <pali@kernel.org> 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.
> >
> > Pardon me, but this driver seems like 3 years old by the APIs it's
> > using... I have to say this, because I was surprised a lot to see some
> > calls.
>
> I wrote it just recently according to other omnia multicolor driver.

I see. I hope you will find my review useful then.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] leds: Add support for Turris 1.x LEDs

[Pali Rohár]

Hello! What with this driver? Its DTS part was already merged and ready to serve.

On Tuesday 05 July 2022 17:59:29 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>
> 
> ---
> Changes in v2:
> * Use 0/1 instead of LED_OFF/LED_ON
> * Add brightness_level device attribute
> * Implement callback brightness_set instead of brightness_set_blocking
> * Reset LEDs to default state before kernel reboots
> ---
>  .../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..bb8b82b43165
> --- /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:	5.20
> +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 (from 100% to 0%), 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:	5.20
> +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:	5.20
> +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 a49979f41eee..71caf45c8ac3 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", &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
> 

Re: [PATCH v2 1/2] [RFT] dt-bindings: leds: Add cznic,turris1x-leds.yaml binding

[Krzysztof Kozlowski]

On 05/07/2022 17:59, Pali Rohár wrote:
> Add device-tree bindings documentation for Turris 1.x RGB LEDs.

The patchset is difficult to find because you attached it to some older
thread. Don't.

> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> 
> ---
> Changes in v2:
> * Fix schema errors
> ---
>  .../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>;
You have some weird indentation here. In other places of DTS example it
is correct four spaces, so here should be four spaces as well.

Best regards,
Krzysztof

Re: [PATCH v2 1/2] [RFT] dt-bindings: leds: Add cznic,turris1x-leds.yaml binding

[Rob Herring]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1270 bytes --]

On Tue, 05 Jul 2022 17:59:28 +0200, Pali Rohár wrote:
> Add device-tree bindings documentation for Turris 1.x RGB LEDs.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> 
> ---
> Changes in v2:
> * Fix schema errors
> ---
>  .../bindings/leds/cznic,turris1x-leds.yaml    | 118 ++++++++++++++++++
>  1 file changed, 118 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/cznic,turris1x-leds.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/leds/cznic,turris1x-leds.example.dts:23.13-47: Warning (ranges_format): /example-0/cpld@3,0:ranges: "ranges" property has invalid length (16 bytes) (parent #address-cells == 1, child #address-cells == 1, #size-cells == 1)

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH v2 1/2] [RFT] dt-bindings: leds: Add cznic,turris1x-leds.yaml binding

[Marek Behún]

On Tue,  5 Jul 2022 17:59:28 +0200
Pali Rohár <pali@kernel.org> wrote:

> +examples:
> +  - |
> +    #include <dt-bindings/leds/common.h>
> +
> +    cpld@3,0 {

The generic node name should be just "bus". That it is a CPLD
implementation should come from compatible string.

This is similar to how we have generic names like "led-controller",
"switch", "ethernet-phy", ...

Pali, I can take over this and get it merged if you are getting
frustrated here.

Marek

Re: [PATCH v2 1/2] [RFT] dt-bindings: leds: Add cznic,turris1x-leds.yaml binding

[Pali Rohár]

On Wednesday 06 July 2022 13:15:07 Marek Behún wrote:
> On Tue,  5 Jul 2022 17:59:28 +0200
> Pali Rohár <pali@kernel.org> wrote:
> 
> > +examples:
> > +  - |
> > +    #include <dt-bindings/leds/common.h>
> > +
> > +    cpld@3,0 {
> 
> The generic node name should be just "bus". That it is a CPLD
> implementation should come from compatible string.

Sorry, I do not understand why "bus". Why other memory chips are named
e.g. "nand" or "nor" and not "bus" too? By this logic should not be
_every_ node called just "bus"? Hm... and are names needed at all then?

Re: [PATCH v2 1/2] [RFT] dt-bindings: leds: Add cznic,turris1x-leds.yaml binding

[Marek Behún]

On Wed, 6 Jul 2022 13:19:12 +0200
Pali Rohár <pali@kernel.org> wrote:

> On Wednesday 06 July 2022 13:15:07 Marek Behún wrote:
> > On Tue,  5 Jul 2022 17:59:28 +0200
> > Pali Rohár <pali@kernel.org> wrote:
> >   
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/leds/common.h>
> > > +
> > > +    cpld@3,0 {  
> > 
> > The generic node name should be just "bus". That it is a CPLD
> > implementation should come from compatible string.  
> 
> Sorry, I do not understand why "bus". Why other memory chips are named
> e.g. "nand" or "nor" and not "bus" too?

As far as I understand this is because that is the preferred name for
busses and this is a bus, since there is also the simple-bus compatible.

> By this logic should not be _every_ node called just "bus"? Hm... and 
> are names needed at all then?

:-)

The schema
  https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/simple-bus.yaml
allows for different names (soc|axi|ahb|*-bus) to avoid warnings on
existing old dts files.

The preferred way is to not have the implementation in nodename,
similar to how we use 'switch' instead of 'mv88e6xxx', or
'ethernet-phy' instead of 'mv88e151x', or 'led-controller', ...

I wasn't there when people started requesting for this to be that way,
but I guess it makes some sense to make it more readable and less
redundant (the generic name in nodename and the implementation in
compatible string...).

Marek

Re: [PATCH v2 1/2] [RFT] dt-bindings: leds: Add cznic,turris1x-leds.yaml binding

[Krzysztof Kozlowski]

On 06/07/2022 17:27, Marek Behún wrote:
> On Wed, 6 Jul 2022 13:19:12 +0200
> Pali Rohár <pali@kernel.org> wrote:
> 
>> On Wednesday 06 July 2022 13:15:07 Marek Behún wrote:
>>> On Tue,  5 Jul 2022 17:59:28 +0200
>>> Pali Rohár <pali@kernel.org> wrote:
>>>   
>>>> +examples:
>>>> +  - |
>>>> +    #include <dt-bindings/leds/common.h>
>>>> +
>>>> +    cpld@3,0 {  
>>>
>>> The generic node name should be just "bus". That it is a CPLD
>>> implementation should come from compatible string.  
>>
>> Sorry, I do not understand why "bus". Why other memory chips are named
>> e.g. "nand" or "nor" and not "bus" too?
> 
> As far as I understand this is because that is the preferred name for
> busses and this is a bus, since there is also the simple-bus compatible.
> 
>> By this logic should not be _every_ node called just "bus"? Hm... and 
>> are names needed at all then?
> 
> :-)
> 
> The schema
>   https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/simple-bus.yaml
> allows for different names (soc|axi|ahb|*-bus) to avoid warnings on
> existing old dts files.
> 
> The preferred way is to not have the implementation in nodename,
> similar to how we use 'switch' instead of 'mv88e6xxx', or
> 'ethernet-phy' instead of 'mv88e151x', or 'led-controller', ...

Thanks Marek for detailed explanation.
The cases above rather trigger my comments and this one here, after
Pali's explanation, do not fit them. pld is a generic class of a device,
so it is okay here. cpld probably as well (although one could argue that
it is a subset of pld, so the generic name is pld, but then one would
say fpga also should be called pld). For me it does not have to be bus,
just don't want mv88e6xxx or any other vendor/model names. Therefore
cpld is fine.

> 
> I wasn't there when people started requesting for this to be that way,
> but I guess it makes some sense to make it more readable and less
> redundant (the generic name in nodename and the implementation in
> compatible string...).




Best regards,
Krzysztof

Re: [PATCH v2 1/2] [RFT] dt-bindings: leds: Add cznic,turris1x-leds.yaml binding

[Marek Behún]

On Wed, 6 Jul 2022 17:36:43 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 06/07/2022 17:27, Marek Behún wrote:
> > On Wed, 6 Jul 2022 13:19:12 +0200
> > Pali Rohár <pali@kernel.org> wrote:
> >   
> >> On Wednesday 06 July 2022 13:15:07 Marek Behún wrote:  
> >>> On Tue,  5 Jul 2022 17:59:28 +0200
> >>> Pali Rohár <pali@kernel.org> wrote:
> >>>     
> >>>> +examples:
> >>>> +  - |
> >>>> +    #include <dt-bindings/leds/common.h>
> >>>> +
> >>>> +    cpld@3,0 {    
> >>>
> >>> The generic node name should be just "bus". That it is a CPLD
> >>> implementation should come from compatible string.    
> >>
> >> Sorry, I do not understand why "bus". Why other memory chips are named
> >> e.g. "nand" or "nor" and not "bus" too?  
> > 
> > As far as I understand this is because that is the preferred name for
> > busses and this is a bus, since there is also the simple-bus compatible.
> >   
> >> By this logic should not be _every_ node called just "bus"? Hm... and 
> >> are names needed at all then?  
> > 
> > :-)
> > 
> > The schema
> >   https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/simple-bus.yaml
> > allows for different names (soc|axi|ahb|*-bus) to avoid warnings on
> > existing old dts files.
> > 
> > The preferred way is to not have the implementation in nodename,
> > similar to how we use 'switch' instead of 'mv88e6xxx', or
> > 'ethernet-phy' instead of 'mv88e151x', or 'led-controller', ...  
> 
> Thanks Marek for detailed explanation.
> The cases above rather trigger my comments and this one here, after
> Pali's explanation, do not fit them. pld is a generic class of a device,
> so it is okay here. cpld probably as well (although one could argue that
> it is a subset of pld, so the generic name is pld, but then one would
> say fpga also should be called pld). For me it does not have to be bus,
> just don't want mv88e6xxx or any other vendor/model names. Therefore
> cpld is fine.

What about cpld-bus? It is used as a bus (simple-bus compatible) and
would work with the *-bus pattern in dt-schema.

Marek

Re: [PATCH v2 1/2] [RFT] dt-bindings: leds: Add cznic,turris1x-leds.yaml binding

[Krzysztof Kozlowski]

On 06/07/2022 18:43, Marek Behún wrote:
> On Wed, 6 Jul 2022 17:36:43 +0200
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>> On 06/07/2022 17:27, Marek Behún wrote:
>>> On Wed, 6 Jul 2022 13:19:12 +0200
>>> Pali Rohár <pali@kernel.org> wrote:
>>>   
>>>> On Wednesday 06 July 2022 13:15:07 Marek Behún wrote:  
>>>>> On Tue,  5 Jul 2022 17:59:28 +0200
>>>>> Pali Rohár <pali@kernel.org> wrote:
>>>>>     
>>>>>> +examples:
>>>>>> +  - |
>>>>>> +    #include <dt-bindings/leds/common.h>
>>>>>> +
>>>>>> +    cpld@3,0 {    
>>>>>
>>>>> The generic node name should be just "bus". That it is a CPLD
>>>>> implementation should come from compatible string.    
>>>>
>>>> Sorry, I do not understand why "bus". Why other memory chips are named
>>>> e.g. "nand" or "nor" and not "bus" too?  
>>>
>>> As far as I understand this is because that is the preferred name for
>>> busses and this is a bus, since there is also the simple-bus compatible.
>>>   
>>>> By this logic should not be _every_ node called just "bus"? Hm... and 
>>>> are names needed at all then?  
>>>
>>> :-)
>>>
>>> The schema
>>>   https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/simple-bus.yaml
>>> allows for different names (soc|axi|ahb|*-bus) to avoid warnings on
>>> existing old dts files.
>>>
>>> The preferred way is to not have the implementation in nodename,
>>> similar to how we use 'switch' instead of 'mv88e6xxx', or
>>> 'ethernet-phy' instead of 'mv88e151x', or 'led-controller', ...  
>>
>> Thanks Marek for detailed explanation.
>> The cases above rather trigger my comments and this one here, after
>> Pali's explanation, do not fit them. pld is a generic class of a device,
>> so it is okay here. cpld probably as well (although one could argue that
>> it is a subset of pld, so the generic name is pld, but then one would
>> say fpga also should be called pld). For me it does not have to be bus,
>> just don't want mv88e6xxx or any other vendor/model names. Therefore
>> cpld is fine.
> 
> What about cpld-bus? It is used as a bus (simple-bus compatible) and
> would work with the *-bus pattern in dt-schema.

If we talk about the example - it does not use any compatible, so we are
focusing on unimportant piece. Anyway using a simple-bus compatible does
not necessarily mean it is a bus. "soc" nodes also use it, but these are
not buses.


Best regards,
Krzysztof

Re: [PATCH v2 1/2] [RFT] dt-bindings: leds: Add cznic,turris1x-leds.yaml binding

[Pali Rohár]

On Wednesday 06 July 2022 17:36:43 Krzysztof Kozlowski wrote:
> On 06/07/2022 17:27, Marek Behún wrote:
> > On Wed, 6 Jul 2022 13:19:12 +0200
> > Pali Rohár <pali@kernel.org> wrote:
> > 
> >> On Wednesday 06 July 2022 13:15:07 Marek Behún wrote:
> >>> On Tue,  5 Jul 2022 17:59:28 +0200
> >>> Pali Rohár <pali@kernel.org> wrote:
> >>>   
> >>>> +examples:
> >>>> +  - |
> >>>> +    #include <dt-bindings/leds/common.h>
> >>>> +
> >>>> +    cpld@3,0 {  
> >>>
> >>> The generic node name should be just "bus". That it is a CPLD
> >>> implementation should come from compatible string.  
> >>
> >> Sorry, I do not understand why "bus". Why other memory chips are named
> >> e.g. "nand" or "nor" and not "bus" too?
> > 
> > As far as I understand this is because that is the preferred name for
> > busses and this is a bus, since there is also the simple-bus compatible.
> > 
> >> By this logic should not be _every_ node called just "bus"? Hm... and 
> >> are names needed at all then?
> > 
> > :-)
> > 
> > The schema
> >   https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/simple-bus.yaml
> > allows for different names (soc|axi|ahb|*-bus) to avoid warnings on
> > existing old dts files.
> > 
> > The preferred way is to not have the implementation in nodename,
> > similar to how we use 'switch' instead of 'mv88e6xxx', or
> > 'ethernet-phy' instead of 'mv88e151x', or 'led-controller', ...
> 
> Thanks Marek for detailed explanation.
> The cases above rather trigger my comments and this one here, after
> Pali's explanation, do not fit them. pld is a generic class of a device,
> so it is okay here. cpld probably as well (although one could argue that
> it is a subset of pld, so the generic name is pld, but then one would
> say fpga also should be called pld). For me it does not have to be bus,
> just don't want mv88e6xxx or any other vendor/model names. Therefore
> cpld is fine.

Exactly. cpld, fpga, nor, nand, soc... all of them are not real buses.

simple-bus here is just name invented by device tree and without which
existing kernel drivers refuse to work.

> > 
> > I wasn't there when people started requesting for this to be that way,
> > but I guess it makes some sense to make it more readable and less
> > redundant (the generic name in nodename and the implementation in
> > compatible string...).
> 
> 
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH v2 1/2] [RFT] dt-bindings: leds: Add cznic,turris1x-leds.yaml binding

[Marek Behún]

On Fri, 8 Jul 2022 18:05:28 +0200
Pali Rohár <pali@kernel.org> wrote:

> On Wednesday 06 July 2022 17:36:43 Krzysztof Kozlowski wrote:
> > On 06/07/2022 17:27, Marek Behún wrote:  
> > > On Wed, 6 Jul 2022 13:19:12 +0200
> > > Pali Rohár <pali@kernel.org> wrote:
> > >   
> > >> On Wednesday 06 July 2022 13:15:07 Marek Behún wrote:  
> > >>> On Tue,  5 Jul 2022 17:59:28 +0200
> > >>> Pali Rohár <pali@kernel.org> wrote:
> > >>>     
> > >>>> +examples:
> > >>>> +  - |
> > >>>> +    #include <dt-bindings/leds/common.h>
> > >>>> +
> > >>>> +    cpld@3,0 {    
> > >>>
> > >>> The generic node name should be just "bus". That it is a CPLD
> > >>> implementation should come from compatible string.    
> > >>
> > >> Sorry, I do not understand why "bus". Why other memory chips are named
> > >> e.g. "nand" or "nor" and not "bus" too?  
> > > 
> > > As far as I understand this is because that is the preferred name for
> > > busses and this is a bus, since there is also the simple-bus compatible.
> > >   
> > >> By this logic should not be _every_ node called just "bus"? Hm... and 
> > >> are names needed at all then?  
> > > 
> > > :-)
> > > 
> > > The schema
> > >   https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/simple-bus.yaml
> > > allows for different names (soc|axi|ahb|*-bus) to avoid warnings on
> > > existing old dts files.
> > > 
> > > The preferred way is to not have the implementation in nodename,
> > > similar to how we use 'switch' instead of 'mv88e6xxx', or
> > > 'ethernet-phy' instead of 'mv88e151x', or 'led-controller', ...  
> > 
> > Thanks Marek for detailed explanation.
> > The cases above rather trigger my comments and this one here, after
> > Pali's explanation, do not fit them. pld is a generic class of a device,
> > so it is okay here. cpld probably as well (although one could argue that
> > it is a subset of pld, so the generic name is pld, but then one would
> > say fpga also should be called pld). For me it does not have to be bus,
> > just don't want mv88e6xxx or any other vendor/model names. Therefore
> > cpld is fine.  
> 
> Exactly. cpld, fpga, nor, nand, soc... all of them are not real buses.
> 
> simple-bus here is just name invented by device tree and without which
> existing kernel drivers refuse to work.

OK, then cpld seems correct. I thought it was considered a bus in a way,
since "simple-bus" is used in compatible.

Marek

Re: [PATCH v2 1/2] [RFT] dt-bindings: leds: Add cznic,turris1x-leds.yaml binding

[Rob Herring]

On Fri, Jul 08, 2022 at 06:05:28PM +0200, Pali Rohár wrote:
> On Wednesday 06 July 2022 17:36:43 Krzysztof Kozlowski wrote:
> > On 06/07/2022 17:27, Marek Behún wrote:
> > > On Wed, 6 Jul 2022 13:19:12 +0200
> > > Pali Rohár <pali@kernel.org> wrote:
> > > 
> > >> On Wednesday 06 July 2022 13:15:07 Marek Behún wrote:
> > >>> On Tue,  5 Jul 2022 17:59:28 +0200
> > >>> Pali Rohár <pali@kernel.org> wrote:
> > >>>   
> > >>>> +examples:
> > >>>> +  - |
> > >>>> +    #include <dt-bindings/leds/common.h>
> > >>>> +
> > >>>> +    cpld@3,0 {  
> > >>>
> > >>> The generic node name should be just "bus". That it is a CPLD
> > >>> implementation should come from compatible string.  
> > >>
> > >> Sorry, I do not understand why "bus". Why other memory chips are named
> > >> e.g. "nand" or "nor" and not "bus" too?
> > > 
> > > As far as I understand this is because that is the preferred name for
> > > busses and this is a bus, since there is also the simple-bus compatible.
> > > 
> > >> By this logic should not be _every_ node called just "bus"? Hm... and 
> > >> are names needed at all then?
> > > 
> > > :-)
> > > 
> > > The schema
> > >   https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/simple-bus.yaml
> > > allows for different names (soc|axi|ahb|*-bus) to avoid warnings on
> > > existing old dts files.
> > > 
> > > The preferred way is to not have the implementation in nodename,
> > > similar to how we use 'switch' instead of 'mv88e6xxx', or
> > > 'ethernet-phy' instead of 'mv88e151x', or 'led-controller', ...
> > 
> > Thanks Marek for detailed explanation.
> > The cases above rather trigger my comments and this one here, after
> > Pali's explanation, do not fit them. pld is a generic class of a device,
> > so it is okay here. cpld probably as well (although one could argue that
> > it is a subset of pld, so the generic name is pld, but then one would
> > say fpga also should be called pld). For me it does not have to be bus,
> > just don't want mv88e6xxx or any other vendor/model names. Therefore
> > cpld is fine.
> 
> Exactly. cpld, fpga, nor, nand, soc... all of them are not real buses.

I guess you could (and we do) have 'cpld' and 'fpga' as devices where 
the OS can reprogram them and such. But if the implementation is fixed 
with no implementation specific controls, I don't think naming how it's 
implemented adds too much. If there's nothing else to identify the 
device though, having 'cpld' in there does have some value I guess.

Same could be argued for soc too perhaps, but that's in the list largely 
to avoid a bunch of s/soc/bus/ on dts files.
 
> simple-bus here is just name invented by device tree and without which
> existing kernel drivers refuse to work.

Everything is just names invented by DT.

simple-bus means 'a bus containing MMIO devices without any bus 
configuration'. You want to add a clock to it?, then no longer a 
simple-bus. Based on that definition, the kernel can enumerate child 
devices without platform specific help. 

Just use 'cpld-bus' and lets move on with our lives.

Rob