[PATCH v4 0/2] Add Turris Omnia LEDs driver

[PATCH v4 0/2] Add Turris Omnia LEDs driver

[Marek Behún]

Hi,

this version addresses Jacek's and Rob's discussion, see changes.
This applies on top of Pavel's tree after applying Dan's multicolor
framework patches (v30). Also on Linus' master tree the same way.

Marek

changes since v3:
- added a note in yaml scheme to the description of multi-led property
  that no subnodes for the specific channels are needed since this
  controller only supports RGB LEDs. A comment into the device-tree
  example in the yaml scheme is added saying the same thing.

changes since v2:
- using multicolor LED framework now, major rewrite
- added support for global brightness (Omnia has a button which can
  switch between 8 levels of intensity of all the LEDs at once, but
  in reality any value between 0% and 100% can be set, and we want to
  access this setting via software)

Marek Behún (2):
  dt-bindings: leds: add cznic,turris-omnia-leds binding
  leds: initial support for Turris Omnia LEDs

 .../leds/cznic,turris-omnia-leds.yaml         |  88 ++++++
 drivers/leds/Kconfig                          |  11 +
 drivers/leds/Makefile                         |   1 +
 drivers/leds/leds-turris-omnia.c              | 296 ++++++++++++++++++
 4 files changed, 396 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/cznic,turris-omnia-leds.yaml
 create mode 100644 drivers/leds/leds-turris-omnia.c

-- 
2.26.2

[PATCH v4 1/2] dt-bindings: leds: add cznic,turris-omnia-leds binding

[Marek Behún]

Add device-tree bindings documentation for Turris Omnia RGB LEDs.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org
---
 .../leds/cznic,turris-omnia-leds.yaml         | 88 +++++++++++++++++++
 1 file changed, 88 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/cznic,turris-omnia-leds.yaml

diff --git a/Documentation/devicetree/bindings/leds/cznic,turris-omnia-leds.yaml b/Documentation/devicetree/bindings/leds/cznic,turris-omnia-leds.yaml
new file mode 100644
index 000000000000..0b33ebf22e27
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/cznic,turris-omnia-leds.yaml
@@ -0,0 +1,88 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/cznic,turris-omnia-leds.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: CZ.NIC's Turris Omnia LEDs driver
+
+maintainers:
+  - Marek Behún <marek.behun@nic.cz>
+
+description:
+  This module adds support for the RGB LEDs found on the fron panel of the
+  Turris Omnia router. There are 12 RGB LEDs, they are controlled by device's
+  microcontroller with which the system communicates via I2C. Each LED is
+  described as a subnode of this I2C device.
+
+properties:
+  compatible:
+    const: cznic,turris-omnia-leds
+
+  reg:
+    description: I2C slave address of the microcontroller.
+    maxItems: 1
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+patternProperties:
+  "^multi-led[0-9a-f]$":
+    type: object
+    allOf:
+      - $ref: leds-class-multicolor.yaml#
+    description:
+      This node represents one of the RGB LED devices on Turris Omnia.
+      No subnodes need to be added for subchannels since this controller only
+      supports RGB LEDs.
+
+    properties:
+      reg:
+        minimum: 0
+        maximum: 11
+        description:
+          This property identifies one of the LEDs on the front panel of the
+          Turris Omnia router.
+
+    required:
+      - reg
+
+examples:
+  - |
+
+    #include <dt-bindings/leds/common.h>
+
+    i2c0 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        led-controller@2b {
+            compatible = "cznic,turris-omnia-leds";
+            reg = <0x2b>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            multi-led@0 {
+                /*
+                 * No subnodes are needed, this controller only supports RGB
+                 * LEDs.
+                 */
+                reg = <0>;
+                color = <LED_COLOR_ID_MULTI>;
+                function = LED_FUNCTION_POWER;
+                linux,default-trigger = "heartbeat";
+            };
+
+            multi-led@a {
+                reg = <0xa>;
+                color = <LED_COLOR_ID_MULTI>;
+                function = LED_FUNCTION_INDICATOR;
+                function-enumerator = <1>;
+            };
+        };
+    };
+
+...
-- 
2.26.2

[PATCH v4 2/2] leds: initial support for Turris Omnia LEDs

[Marek Behún]

This adds basic support for LEDs on the front side of CZ.NIC's Turris
Omnia router.

There are 12 RGB LEDs. The controller supports HW triggering mode for
the LEDs, but this driver does not support it yet, and sets all the LEDs
defined in device-tree into SW mode upon probe.

This driver uses the multi color LED framework.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/leds/Kconfig             |  11 ++
 drivers/leds/Makefile            |   1 +
 drivers/leds/leds-turris-omnia.c | 296 +++++++++++++++++++++++++++++++
 3 files changed, 308 insertions(+)
 create mode 100644 drivers/leds/leds-turris-omnia.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 68f63d1a7d48..ce365a22445d 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -176,6 +176,17 @@ config LEDS_EL15203000
 	  To compile this driver as a module, choose M here: the module
 	  will be called leds-el15203000.
 
+config LEDS_TURRIS_OMNIA
+	tristate "LED support for CZ.NIC's Turris Omnia"
+	depends on LEDS_CLASS_MULTI_COLOR
+	depends on I2C
+	depends on MACH_ARMADA_38X || COMPILE_TEST
+	depends on OF
+	help
+	  This option enables basic support for the LEDs found on the front
+	  side of CZ.NIC's Turris Omnia router. There are 12 RGB LEDs on the
+	  front panel.
+
 config LEDS_LM3530
 	tristate "LCD Backlight driver for LM3530"
 	depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 68c05faec99e..0dcea0322fd3 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -88,6 +88,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_OMNIA)		+= leds-turris-omnia.o
 obj-$(CONFIG_LEDS_WM831X_STATUS)	+= leds-wm831x-status.o
 obj-$(CONFIG_LEDS_WM8350)		+= leds-wm8350.o
 obj-$(CONFIG_LEDS_WRAP)			+= leds-wrap.o
diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c
new file mode 100644
index 000000000000..0aa79abf0ed4
--- /dev/null
+++ b/drivers/leds/leds-turris-omnia.c
@@ -0,0 +1,296 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * CZ.NIC's Turris Omnia LEDs driver
+ *
+ * 2020 by Marek Behun <marek.behun@nic.cz>
+ */
+
+#include <linux/i2c.h>
+#include <linux/led-class-multicolor.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include "leds.h"
+
+#define OMNIA_BOARD_LEDS		12
+
+#define CMD_LED_MODE			3
+#define CMD_LED_MODE_LED(l)		((l) & 0x0f)
+#define CMD_LED_MODE_USER		0x10
+
+#define CMD_LED_STATE			4
+#define CMD_LED_STATE_LED(l)		((l) & 0x0f)
+#define CMD_LED_STATE_ON		0x10
+
+#define CMD_LED_COLOR			5
+#define CMD_LED_SET_BRIGHTNESS		7
+#define CMD_LED_GET_BRIGHTNESS		8
+
+#define OMNIA_CMD			0
+
+#define OMNIA_CMD_LED_COLOR_LED		1
+#define OMNIA_CMD_LED_COLOR_R		2
+#define OMNIA_CMD_LED_COLOR_G		3
+#define OMNIA_CMD_LED_COLOR_B		4
+#define OMNIA_CMD_LED_COLOR_LEN		5
+
+struct omnia_led {
+	struct led_classdev_mc mc_cdev;
+	struct mc_subled subled_info[3];
+	int reg;
+};
+
+#define to_omnia_led(l)		container_of(l, struct omnia_led, mc_cdev)
+
+struct omnia_leds {
+	struct i2c_client *client;
+	struct mutex lock;
+	int nleds;
+	struct omnia_led leds[0];
+};
+
+static int omnia_led_brightness_set_blocking(struct led_classdev *cdev,
+					     enum led_brightness brightness)
+{
+	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
+	struct omnia_leds *leds = dev_get_drvdata(cdev->dev->parent);
+	struct omnia_led *led = to_omnia_led(mc_cdev);
+	u8 buf[OMNIA_CMD_LED_COLOR_LEN], state;
+	int ret;
+
+	led_mc_calc_color_components(&led->mc_cdev, brightness);
+
+	mutex_lock(&leds->lock);
+
+	buf[OMNIA_CMD] = CMD_LED_COLOR;
+	buf[OMNIA_CMD_LED_COLOR_LED] = led->reg;
+	buf[OMNIA_CMD_LED_COLOR_R] = mc_cdev->subled_info[0].brightness;
+	buf[OMNIA_CMD_LED_COLOR_G] = mc_cdev->subled_info[1].brightness;
+	buf[OMNIA_CMD_LED_COLOR_B] = mc_cdev->subled_info[2].brightness;
+
+	state = CMD_LED_STATE_LED(led->reg);
+	if (buf[OMNIA_CMD_LED_COLOR_R] || buf[OMNIA_CMD_LED_COLOR_G] || buf[OMNIA_CMD_LED_COLOR_B])
+		state |= CMD_LED_STATE_ON;
+
+	ret = i2c_smbus_write_byte_data(leds->client, CMD_LED_STATE, state);
+	if (ret >= 0 && (state & CMD_LED_STATE_ON))
+		ret = i2c_master_send(leds->client, buf, 5);
+
+	mutex_unlock(&leds->lock);
+
+	return ret;
+}
+
+static int omnia_led_register(struct omnia_leds *leds, struct device_node *np)
+{
+	struct i2c_client *client = leds->client;
+	struct led_init_data init_data = {};
+	struct device *dev = &client->dev;
+	struct led_classdev *cdev;
+	struct omnia_led *led;
+	int ret, color;
+
+	led = &leds->leds[leds->nleds];
+
+	ret = of_property_read_u32(np, "reg", &led->reg);
+	if (ret || led->reg >= OMNIA_BOARD_LEDS) {
+		dev_warn(dev,
+			 "Node %pOF: must contain 'reg' property with values between 0 and %i\n",
+			 np, OMNIA_BOARD_LEDS - 1);
+		return 0;
+	}
+
+	ret = of_property_read_u32(np, "color", &color);
+	if (ret || color != LED_COLOR_ID_MULTI) {
+		dev_warn(dev,
+			 "Node %pOF: must contain 'color' property with value LED_COLOR_ID_MULTI\n",
+			 np);
+		return 0;
+	}
+
+	led->subled_info[0].color_index = LED_COLOR_ID_RED;
+	led->subled_info[0].channel = 0;
+	led->subled_info[1].color_index = LED_COLOR_ID_GREEN;
+	led->subled_info[1].channel = 1;
+	led->subled_info[2].color_index = LED_COLOR_ID_BLUE;
+	led->subled_info[2].channel = 2;
+
+	led->mc_cdev.subled_info = led->subled_info;
+	led->mc_cdev.num_colors = 3;
+
+	init_data.fwnode = &np->fwnode;
+
+	cdev = &led->mc_cdev.led_cdev;
+	cdev->max_brightness = 255;
+	cdev->brightness_set_blocking = omnia_led_brightness_set_blocking;
+
+	of_property_read_string(np, "linux,default-trigger", &cdev->default_trigger);
+
+	/* put the LED into software mode */
+	ret = i2c_smbus_write_byte_data(client, CMD_LED_MODE,
+					CMD_LED_MODE_LED(led->reg) |
+					CMD_LED_MODE_USER);
+	if (ret < 0) {
+		dev_err(dev, "Cannot set LED %pOF to software mode: %i\n", np, ret);
+		return ret;
+	}
+
+	/* disable the LED */
+	ret = i2c_smbus_write_byte_data(client, CMD_LED_STATE, CMD_LED_STATE_LED(led->reg));
+	if (ret < 0) {
+		dev_err(dev, "Cannot set LED %pOF brightness: %i\n", np, ret);
+		return ret;
+	}
+
+	ret = devm_led_classdev_multicolor_register_ext(dev, &led->mc_cdev, &init_data);
+	if (ret < 0) {
+		dev_err(dev, "Cannot register LED %pOF: %i\n", np, ret);
+		return ret;
+	}
+
+	++leds->nleds;
+
+	return 0;
+}
+
+/*
+ * On the front panel of the Turris Omnia router 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 microcontroller cycles between 8 levels of this global brightness (from 100% to 0%), but this
+ * setting can have any integer value between 0 and 100.
+ * It is usable to be able to change this value from software, so that it does not start at 100%
+ * after every power on and annoy the user.
+ * We expose this setting via a sysfs attribute file called "brightness". This file lives in the
+ * device directory of the LED controller, not an individual LED, so it should not confuse users.
+ */
+static ssize_t brightness_show(struct device *dev, struct device_attribute *a, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct omnia_leds *leds = i2c_get_clientdata(client);
+	int ret;
+
+	mutex_lock(&leds->lock);
+	ret = i2c_smbus_read_byte_data(client, CMD_LED_GET_BRIGHTNESS);
+	mutex_unlock(&leds->lock);
+
+	if (ret < 0)
+		return ret;
+
+	return sprintf(buf, "%d\n", ret);
+}
+
+static ssize_t brightness_store(struct device *dev, struct device_attribute *a, const char *buf,
+				size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct omnia_leds *leds = i2c_get_clientdata(client);
+	unsigned int brightness;
+	int ret;
+
+	if (sscanf(buf, "%u", &brightness) != 1)
+		return -EINVAL;
+
+	if (brightness > 100)
+		return -EINVAL;
+
+	mutex_lock(&leds->lock);
+	ret = i2c_smbus_write_byte_data(client, CMD_LED_SET_BRIGHTNESS, (u8) brightness);
+	mutex_unlock(&leds->lock);
+
+	if (ret < 0)
+		return ret;
+
+	return count;
+}
+static DEVICE_ATTR_RW(brightness);
+
+static struct attribute *omnia_led_controller_attrs[] = {
+	&dev_attr_brightness.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(omnia_led_controller);
+
+static int omnia_leds_probe(struct i2c_client *client,
+			    const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	struct device_node *np = dev->of_node, *child;
+	struct omnia_leds *leds;
+	int ret, count;
+
+	count = of_get_available_child_count(np);
+	if (!count) {
+		dev_err(dev, "LEDs are not defined in device tree!\n");
+		return -ENODEV;
+	} else if (count > OMNIA_BOARD_LEDS) {
+		dev_err(dev, "Too many LEDs defined in device tree!\n");
+		return -EINVAL;
+	}
+
+	leds = devm_kzalloc(dev, sizeof(*leds) + count * sizeof(leds->leds[0]),
+			    GFP_KERNEL);
+	if (!leds)
+		return -ENOMEM;
+
+	leds->client = client;
+	i2c_set_clientdata(client, leds);
+
+	mutex_init(&leds->lock);
+
+	for_each_available_child_of_node(np, child) {
+		ret = omnia_led_register(leds, child);
+		if (ret < 0)
+			return ret;
+	}
+
+	if (devm_device_add_groups(dev, omnia_led_controller_groups))
+		dev_warn(dev, "Could not add attribute group!\n");
+
+	return 0;
+}
+
+static int omnia_leds_remove(struct i2c_client *client)
+{
+	u8 buf[OMNIA_CMD_LED_COLOR_LEN];
+
+	/* put all LEDs into default (HW triggered) mode */
+	i2c_smbus_write_byte_data(client, CMD_LED_MODE,
+				  CMD_LED_MODE_LED(OMNIA_BOARD_LEDS));
+
+	/* set all LEDs color to [255, 255, 255] */
+	buf[OMNIA_CMD] = CMD_LED_COLOR;
+	buf[OMNIA_CMD_LED_COLOR_LED] = OMNIA_BOARD_LEDS;
+	buf[OMNIA_CMD_LED_COLOR_R] = 255;
+	buf[OMNIA_CMD_LED_COLOR_G] = 255;
+	buf[OMNIA_CMD_LED_COLOR_B] = 255;
+
+	i2c_master_send(client, buf, 5);
+
+	return 0;
+}
+
+static const struct of_device_id of_omnia_leds_match[] = {
+	{ .compatible = "cznic,turris-omnia-leds", },
+	{},
+};
+
+static const struct i2c_device_id omnia_id[] = {
+	{ "omnia", 0 },
+	{ }
+};
+
+static struct i2c_driver omnia_leds_driver = {
+	.probe		= omnia_leds_probe,
+	.remove		= omnia_leds_remove,
+	.id_table	= omnia_id,
+	.driver		= {
+		.name	= "leds-turris-omnia",
+		.of_match_table = of_omnia_leds_match,
+	},
+};
+
+module_i2c_driver(omnia_leds_driver);
+
+MODULE_AUTHOR("Marek Behun <marek.behun@nic.cz>");
+MODULE_DESCRIPTION("CZ.NIC's Turris Omnia LEDs");
+MODULE_LICENSE("GPL v2");
-- 
2.26.2

Re: [PATCH v4 1/2] dt-bindings: leds: add cznic,turris-omnia-leds binding

[Dan Murphy]

Marek

On 7/15/20 7:40 AM, Marek Behún wrote:
> Add device-tree bindings documentation for Turris Omnia RGB LEDs.
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> ---
>   .../leds/cznic,turris-omnia-leds.yaml         | 88 +++++++++++++++++++
>   1 file changed, 88 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/leds/cznic,turris-omnia-leds.yaml
>
> diff --git a/Documentation/devicetree/bindings/leds/cznic,turris-omnia-leds.yaml b/Documentation/devicetree/bindings/leds/cznic,turris-omnia-leds.yaml
> new file mode 100644
> index 000000000000..0b33ebf22e27
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/cznic,turris-omnia-leds.yaml
> @@ -0,0 +1,88 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/cznic,turris-omnia-leds.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: CZ.NIC's Turris Omnia LEDs driver
> +
> +maintainers:
> +  - Marek Behún <marek.behun@nic.cz>
> +
> +description:
> +  This module adds support for the RGB LEDs found on the fron panel of the
s/fron/front
> +  Turris Omnia router. There are 12 RGB LEDs, they are controlled by device's
> +  microcontroller with which the system communicates via I2C. Each LED is

This is a bit confusing and reads very rough can maybe

There are 12 RGB LEDs that are controlled via a micro controller that 
communicates via the I2C bus.

Dan

Re: [PATCH v4 1/2] dt-bindings: leds: add cznic,turris-omnia-leds binding

[Marek Behún]

On Wed, 15 Jul 2020 07:47:28 -0500
Dan Murphy <dmurphy@ti.com> wrote:

> This is a bit confusing and reads very rough can maybe
> 
> There are 12 RGB LEDs that are controlled via a micro controller that 
> communicates via the I2C bus.
> 
> Dan
> 
> 

How about this?

  This module adds support for the RGB LEDs found on the front panel of
  the Turris Omnia router. There are 12 RGB LEDs that are controlled by
  a microcontroller that communicates via the I2C bus. Each LED is
  described as a subnode of this I2C device.

Re: [PATCH v4 2/2] leds: initial support for Turris Omnia LEDs

[Dan Murphy]

Marek

On 7/15/20 7:40 AM, Marek Behún wrote:
> This adds basic support for LEDs on the front side of CZ.NIC's Turris
> Omnia router.
>
> There are 12 RGB LEDs. The controller supports HW triggering mode for
> the LEDs, but this driver does not support it yet, and sets all the LEDs
> defined in device-tree into SW mode upon probe.
>
> This driver uses the multi color LED framework.
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
>   drivers/leds/Kconfig             |  11 ++
>   drivers/leds/Makefile            |   1 +
>   drivers/leds/leds-turris-omnia.c | 296 +++++++++++++++++++++++++++++++
>   3 files changed, 308 insertions(+)
>   create mode 100644 drivers/leds/leds-turris-omnia.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 68f63d1a7d48..ce365a22445d 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -176,6 +176,17 @@ config LEDS_EL15203000
>   	  To compile this driver as a module, choose M here: the module
>   	  will be called leds-el15203000.
>   
> +config LEDS_TURRIS_OMNIA
> +	tristate "LED support for CZ.NIC's Turris Omnia"
> +	depends on LEDS_CLASS_MULTI_COLOR
> +	depends on I2C
> +	depends on MACH_ARMADA_38X || COMPILE_TEST
> +	depends on OF
> +	help
> +	  This option enables basic support for the LEDs found on the front
> +	  side of CZ.NIC's Turris Omnia router. There are 12 RGB LEDs on the
> +	  front panel.
> +
Can this be built as a module?
>   config LEDS_LM3530
>   	tristate "LCD Backlight driver for LM3530"
>   	depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 68c05faec99e..0dcea0322fd3 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -88,6 +88,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_OMNIA)		+= leds-turris-omnia.o
>   obj-$(CONFIG_LEDS_WM831X_STATUS)	+= leds-wm831x-status.o
>   obj-$(CONFIG_LEDS_WM8350)		+= leds-wm8350.o
>   obj-$(CONFIG_LEDS_WRAP)			+= leds-wrap.o
> diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c
> new file mode 100644
> index 000000000000..0aa79abf0ed4
> --- /dev/null
> +++ b/drivers/leds/leds-turris-omnia.c
> @@ -0,0 +1,296 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CZ.NIC's Turris Omnia LEDs driver
> + *
> + * 2020 by Marek Behun <marek.behun@nic.cz>
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/led-class-multicolor.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include "leds.h"
> +
> +#define OMNIA_BOARD_LEDS		12
> +
> +#define CMD_LED_MODE			3
> +#define CMD_LED_MODE_LED(l)		((l) & 0x0f)
> +#define CMD_LED_MODE_USER		0x10
> +
> +#define CMD_LED_STATE			4
> +#define CMD_LED_STATE_LED(l)		((l) & 0x0f)
> +#define CMD_LED_STATE_ON		0x10
> +
> +#define CMD_LED_COLOR			5
> +#define CMD_LED_SET_BRIGHTNESS		7
> +#define CMD_LED_GET_BRIGHTNESS		8
> +
> +#define OMNIA_CMD			0
> +
> +#define OMNIA_CMD_LED_COLOR_LED		1
> +#define OMNIA_CMD_LED_COLOR_R		2
> +#define OMNIA_CMD_LED_COLOR_G		3
> +#define OMNIA_CMD_LED_COLOR_B		4
> +#define OMNIA_CMD_LED_COLOR_LEN		5
> +
> +struct omnia_led {
> +	struct led_classdev_mc mc_cdev;
> +	struct mc_subled subled_info[3];
nit. #define the 3 and use it here and when indicating the num_colors
> +	int reg;
> +};
> +
> +#define to_omnia_led(l)		container_of(l, struct omnia_led, mc_cdev)
Spacing look to be off here.
> +
> +struct omnia_leds {
> +	struct i2c_client *client;
> +	struct mutex lock;
> +	int nleds;
This does  not seem to be used anywhere except for during registering
> +	struct omnia_led leds[0];
Remove the 0 as this should be a flexible array
> +};
> +
> +static int omnia_led_brightness_set_blocking(struct led_classdev *cdev,
> +					     enum led_brightness brightness)
> +{
> +	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
> +	struct omnia_leds *leds = dev_get_drvdata(cdev->dev->parent);
> +	struct omnia_led *led = to_omnia_led(mc_cdev);
> +	u8 buf[OMNIA_CMD_LED_COLOR_LEN], state;
> +	int ret;
> +
> +	led_mc_calc_color_components(&led->mc_cdev, brightness);

Do you need this in the lock as well?


> +
> +	mutex_lock(&leds->lock);
> +
> +	buf[OMNIA_CMD] = CMD_LED_COLOR;
> +	buf[OMNIA_CMD_LED_COLOR_LED] = led->reg;
> +	buf[OMNIA_CMD_LED_COLOR_R] = mc_cdev->subled_info[0].brightness;
> +	buf[OMNIA_CMD_LED_COLOR_G] = mc_cdev->subled_info[1].brightness;
> +	buf[OMNIA_CMD_LED_COLOR_B] = mc_cdev->subled_info[2].brightness;
> +
> +	state = CMD_LED_STATE_LED(led->reg);
> +	if (buf[OMNIA_CMD_LED_COLOR_R] || buf[OMNIA_CMD_LED_COLOR_G] || buf[OMNIA_CMD_LED_COLOR_B])
> +		state |= CMD_LED_STATE_ON;
> +
> +	ret = i2c_smbus_write_byte_data(leds->client, CMD_LED_STATE, state);
> +	if (ret >= 0 && (state & CMD_LED_STATE_ON))
> +		ret = i2c_master_send(leds->client, buf, 5);
> +
> +	mutex_unlock(&leds->lock);
> +
> +	return ret;
> +}
> +
> +static int omnia_led_register(struct omnia_leds *leds, struct device_node *np)
> +{
> +	struct i2c_client *client = leds->client;
> +	struct led_init_data init_data = {};
> +	struct device *dev = &client->dev;
> +	struct led_classdev *cdev;
> +	struct omnia_led *led;
> +	int ret, color;
> +
> +	led = &leds->leds[leds->nleds];
> +
> +	ret = of_property_read_u32(np, "reg", &led->reg);
> +	if (ret || led->reg >= OMNIA_BOARD_LEDS) {
> +		dev_warn(dev,
> +			 "Node %pOF: must contain 'reg' property with values between 0 and %i\n",
> +			 np, OMNIA_BOARD_LEDS - 1);
> +		return 0;
> +	}
> +
> +	ret = of_property_read_u32(np, "color", &color);
> +	if (ret || color != LED_COLOR_ID_MULTI) {
> +		dev_warn(dev,
> +			 "Node %pOF: must contain 'color' property with value LED_COLOR_ID_MULTI\n",
> +			 np);
> +		return 0;
> +	}
> +
> +	led->subled_info[0].color_index = LED_COLOR_ID_RED;
> +	led->subled_info[0].channel = 0;
> +	led->subled_info[1].color_index = LED_COLOR_ID_GREEN;
> +	led->subled_info[1].channel = 1;
> +	led->subled_info[2].color_index = LED_COLOR_ID_BLUE;
> +	led->subled_info[2].channel = 2;

OK this is why you may want to have sub-nodes.  Where reg is the channel 
and color is the color.  Then you can do a for_each_child.

> +
> +	led->mc_cdev.subled_info = led->subled_info;
> +	led->mc_cdev.num_colors = 3;
> +
> +	init_data.fwnode = &np->fwnode;
> +
> +	cdev = &led->mc_cdev.led_cdev;
> +	cdev->max_brightness = 255;
This is not needed.  It is defaulted to LED_FULL in led_class
> +	cdev->brightness_set_blocking = omnia_led_brightness_set_blocking;
> +
> +	of_property_read_string(np, "linux,default-trigger", &cdev->default_trigger);
> +
> +	/* put the LED into software mode */
> +	ret = i2c_smbus_write_byte_data(client, CMD_LED_MODE,
> +					CMD_LED_MODE_LED(led->reg) |
> +					CMD_LED_MODE_USER);
> +	if (ret < 0) {
> +		dev_err(dev, "Cannot set LED %pOF to software mode: %i\n", np, ret);
> +		return ret;
> +	}
> +
> +	/* disable the LED */
> +	ret = i2c_smbus_write_byte_data(client, CMD_LED_STATE, CMD_LED_STATE_LED(led->reg));
> +	if (ret < 0) {
> +		dev_err(dev, "Cannot set LED %pOF brightness: %i\n", np, ret);
> +		return ret;
> +	}
> +
> +	ret = devm_led_classdev_multicolor_register_ext(dev, &led->mc_cdev, &init_data);
> +	if (ret < 0) {
> +		dev_err(dev, "Cannot register LED %pOF: %i\n", np, ret);
> +		return ret;
> +	}
> +
> +	++leds->nleds;
> +
> +	return 0;
> +}
> +
> +/*
> + * On the front panel of the Turris Omnia router 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 microcontroller cycles between 8 levels of this global brightness (from 100% to 0%), but this
> + * setting can have any integer value between 0 and 100.
> + * It is usable to be able to change this value from software, so that it does not start at 100%
This does not make sense.
> + * after every power on and annoy the user.
> + * We expose this setting via a sysfs attribute file called "brightness". This file lives in the
> + * device directory of the LED controller, not an individual LED, so it should not confuse users.
> + */

Sorry if this has been discussed already

This seems a bit wonky.  You are overriding the brightness set by the 
LED class.

Does this button have an interrupt to the processor or does it go to the 
micro controller?

Where is the current power on value stored?  If this is stored in the 
micro maybe reading that value at power up and setting the brightness 
would be better.

> +static ssize_t brightness_show(struct device *dev, struct device_attribute *a, char *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct omnia_leds *leds = i2c_get_clientdata(client);
> +	int ret;
> +
> +	mutex_lock(&leds->lock);
> +	ret = i2c_smbus_read_byte_data(client, CMD_LED_GET_BRIGHTNESS);
> +	mutex_unlock(&leds->lock);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return sprintf(buf, "%d\n", ret);
> +}
> +
> +static ssize_t brightness_store(struct device *dev, struct device_attribute *a, const char *buf,
> +				size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct omnia_leds *leds = i2c_get_clientdata(client);
> +	unsigned int brightness;
> +	int ret;
> +
> +	if (sscanf(buf, "%u", &brightness) != 1)
> +		return -EINVAL;
> +
> +	if (brightness > 100)
> +		return -EINVAL;
> +
> +	mutex_lock(&leds->lock);
> +	ret = i2c_smbus_write_byte_data(client, CMD_LED_SET_BRIGHTNESS, (u8) brightness);
> +	mutex_unlock(&leds->lock);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(brightness);
> +
> +static struct attribute *omnia_led_controller_attrs[] = {
> +	&dev_attr_brightness.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(omnia_led_controller);
> +
> +static int omnia_leds_probe(struct i2c_client *client,
> +			    const struct i2c_device_id *id)
> +{
> +	struct device *dev = &client->dev;
> +	struct device_node *np = dev->of_node, *child;
> +	struct omnia_leds *leds;
> +	int ret, count;
> +
> +	count = of_get_available_child_count(np);
> +	if (!count) {
> +		dev_err(dev, "LEDs are not defined in device tree!\n");
> +		return -ENODEV;
> +	} else if (count > OMNIA_BOARD_LEDS) {
> +		dev_err(dev, "Too many LEDs defined in device tree!\n");
> +		return -EINVAL;
> +	}
> +
> +	leds = devm_kzalloc(dev, sizeof(*leds) + count * sizeof(leds->leds[0]),
> +			    GFP_KERNEL);
Use the macro struct_size(led, leds, count)


Dan

Re: [PATCH v4 2/2] leds: initial support for Turris Omnia LEDs

[Marek Behún]

On Wed, 15 Jul 2020 08:27:51 -0500
Dan Murphy <dmurphy@ti.com> wrote:

> Can this be built as a module?

Yes. But only if MC framework is compiled in. If MC framework is
compiled as module as well, this results in
  FATAL: modpost: GPL-incompatible module led-class-multicolor.ko uses
  GPL-only symbol 'led_set_brightness'

> > +struct omnia_led {
> > +	struct led_classdev_mc mc_cdev;
> > +	struct mc_subled subled_info[3];  
> nit. #define the 3 and use it here and when indicating the num_colors

Meh, ok


> > +#define to_omnia_led(l)		container_of(l, struct
> > omnia_led, mc_cdev)  
> Spacing look to be off here.

thx

> > +
> > +struct omnia_leds {
> > +	struct i2c_client *client;
> > +	struct mutex lock;
> > +	int nleds;  
> This does  not seem to be used anywhere except for during registering

Hmmm, I'll try to rewrite this

> > +	struct omnia_led leds[0];  
> Remove the 0 as this should be a flexible array

Thx

> > +	led_mc_calc_color_components(&led->mc_cdev, brightness);  
> 
> Do you need this in the lock as well?

You are right

> > +	led->subled_info[0].color_index = LED_COLOR_ID_RED;
> > +	led->subled_info[0].channel = 0;
> > +	led->subled_info[1].color_index = LED_COLOR_ID_GREEN;
> > +	led->subled_info[1].channel = 1;
> > +	led->subled_info[2].color_index = LED_COLOR_ID_BLUE;
> > +	led->subled_info[2].channel = 2;  
> 
> OK this is why you may want to have sub-nodes.  Where reg is the
> channel and color is the color.  Then you can do a for_each_child.

Rob says that it doesn't need to be in DT if the controller only
supports RGB LEDs. This controller is only in Turris Omnia which was
never made with another kind of LEDs. So I think it is pointless and
would only grow the DT and complicate the driver.

> > +	cdev->max_brightness = 255;  
> This is not needed.  It is defaulted to LED_FULL in led_class

This was discussed last year and resulted in LED_FULL being
declared obsolete in the header file.

> > +/*
> > + * On the front panel of the Turris Omnia router 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 microcontroller cycles between 8 levels of this global
> > brightness (from 100% to 0%), but this
> > + * setting can have any integer value between 0 and 100.
> > + * It is usable to be able to change this value from software, so
> > that it does not start at 100%  
> This does not make sense.

It does. The user changes the brightness of all 12 LEDs with the button
to his liking and wants to have the same setting after powering
the router on again.

> > + * after every power on and annoy the user.
> > + * We expose this setting via a sysfs attribute file called
> > "brightness". This file lives in the
> > + * device directory of the LED controller, not an individual LED,
> > so it should not confuse users.
> > + */  
> 
> Sorry if this has been discussed already
> 
> This seems a bit wonky.  You are overriding the brightness set by the 
> LED class.

I am not. Pressing the button does not change the brightness read from
the /sys/class/leds/<LED>/brightness file. This is different brightness,
it is above the classic brightnes in the PWM hierarchy in the
microcontroller. I discussed this with Pavel and he said we can call
this file brightness as well (since it is brightness of the whole
panel), and the file does not reside in /sys/class/leds/<LED> directory.

> Does this button have an interrupt to the processor or does it go to
> the micro controller?
> 
> Where is the current power on value stored?  If this is stored in the 
> micro maybe reading that value at power up and setting the brightness 
> would be better.
> 
> > +static ssize_t brightness_show(struct device *dev, struct
> > device_attribute *a, char *buf) +{
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct omnia_leds *leds = i2c_get_clientdata(client);
> > +	int ret;
> > +
> > +	mutex_lock(&leds->lock);
> > +	ret = i2c_smbus_read_byte_data(client,
> > CMD_LED_GET_BRIGHTNESS);
> > +	mutex_unlock(&leds->lock);
> > +
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return sprintf(buf, "%d\n", ret);
> > +}
> > +
> > +static ssize_t brightness_store(struct device *dev, struct
> > device_attribute *a, const char *buf,led = &leds->leds[leds->nleds];
> > +				size_t count)
> > +{
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct omnia_leds *leds = i2c_get_clientdata(client);
> > +	unsigned int brightness;
> > +	int ret;
> > +
> > +	if (sscanf(buf, "%u", &brightness) != 1)
> > +		return -EINVAL;
> > +
> > +	if (brightness > 100)
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&leds->lock);
> > +	ret = i2c_smbus_write_byte_data(client,
> > CMD_LED_SET_BRIGHTNESS, (u8) brightness);
> > +	mutex_unlock(&leds->lock);
> > +
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return count;
> > +}
> > +static DEVICE_ATTR_RW(brightness);
> > +
> > +static struct attribute *omnia_led_controller_attrs[] = {
> > +	&dev_attr_brightness.attr,
> > +	NULL,
> > +};
> > +ATTRIBUTE_GROUPS(omnia_led_controller);
> > +
> > +static int omnia_leds_probe(struct i2c_client *client,
> > +			    const struct i2c_device_id *id)
> > +{
> > +	struct device *dev = &client->dev;
> > +	struct device_node *np = dev->of_node, *child;
> > +	struct omnia_leds *leds;
> > +	int ret, count;
> > +
> > +	count = of_get_available_child_count(np);
> > +	if (!count) {
> > +		dev_err(dev, "LEDs are not defined in device
> > tree!\n");
> > +		return -ENODEV;
> > +	} else if (count > OMNIA_BOARD_LEDS) {
> > +		dev_err(dev, "Too many LEDs defined in device
> > tree!\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	leds = devm_kzalloc(dev, sizeof(*leds) + count *
> > sizeof(leds->leds[0]),
> > +			    GFP_KERNEL);  
> Use the macro struct_size(led, leds, count)

thx

Re: [PATCH v4 2/2] leds: initial support for Turris Omnia LEDs

[Dan Murphy]

Marek

On 7/15/20 2:03 PM, Marek Behún wrote:
> On Wed, 15 Jul 2020 08:27:51 -0500
> Dan Murphy <dmurphy@ti.com> wrote:
>
>> Can this be built as a module?
> Yes. But only if MC framework is compiled in. If MC framework is
> compiled as module as well, this results in
>    FATAL: modpost: GPL-incompatible module led-class-multicolor.ko uses
>    GPL-only symbol 'led_set_brightness'

Hmm.  That is interesting

<snip>

>>> +	led->subled_info[0].color_index = LED_COLOR_ID_RED;
>>> +	led->subled_info[0].channel = 0;
>>> +	led->subled_info[1].color_index = LED_COLOR_ID_GREEN;
>>> +	led->subled_info[1].channel = 1;
>>> +	led->subled_info[2].color_index = LED_COLOR_ID_BLUE;
>>> +	led->subled_info[2].channel = 2;
>> OK this is why you may want to have sub-nodes.  Where reg is the
>> channel and color is the color.  Then you can do a for_each_child.
> Rob says that it doesn't need to be in DT if the controller only
> supports RGB LEDs. This controller is only in Turris Omnia which was
> never made with another kind of LEDs. So I think it is pointless and
> would only grow the DT and complicate the driver.
OK I know there was a discussion on this
>>> +	cdev->max_brightness = 255;
>> This is not needed.  It is defaulted to LED_FULL in led_class
> This was discussed last year and resulted in LED_FULL being
> declared obsolete in the header file.

No I am referring to setting the max_brightness to 255 the LED class 
sets this to 255 if the value is not set.


>>> +/*
>>> + * On the front panel of the Turris Omnia router 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 microcontroller cycles between 8 levels of this global
>>> brightness (from 100% to 0%), but this
>>> + * setting can have any integer value between 0 and 100.
>>> + * It is usable to be able to change this value from software, so
>>> that it does not start at 100%
>> This does not make sense.
> It does. The user changes the brightness of all 12 LEDs with the button
> to his liking and wants to have the same setting after powering
> the router on again.

No the english does not make sense

" It is usable to be able to change this value from software, so
that it does not start at 100%"

"It is usable" is not really clear.

>>> + * after every power on and annoy the user.
>>> + * We expose this setting via a sysfs attribute file called
>>> "brightness". This file lives in the
>>> + * device directory of the LED controller, not an individual LED,
>>> so it should not confuse users.
>>> + */
>> Sorry if this has been discussed already
>>
>> This seems a bit wonky.  You are overriding the brightness set by the
>> LED class.
> I am not. Pressing the button does not change the brightness read from
> the /sys/class/leds/<LED>/brightness file. This is different brightness,
> it is above the classic brightnes in the PWM hierarchy in the
> microcontroller. I discussed this with Pavel and he said we can call
> this file brightness as well (since it is brightness of the whole
> panel), and the file does not reside in /sys/class/leds/<LED> directory.

OK then there needs to be some ABI documentation no?

Dan

Re: [PATCH v4 2/2] leds: initial support for Turris Omnia LEDs

[Marek Behún]

Hi Dan,

On Wed, 15 Jul 2020 14:10:34 -0500
Dan Murphy <dmurphy@ti.com> wrote:

> >>> +	cdev->max_brightness = 255;  
> >> This is not needed.  It is defaulted to LED_FULL in led_class  
> > This was discussed last year and resulted in LED_FULL being
> > declared obsolete in the header file.  
> 
> No I am referring to setting the max_brightness to 255 the LED class 
> sets this to 255 if the value is not set.

I'll rather have this value here, since this is a controller specific
constant. The LED class sets this to LED_FULL, which is obsolete. It
could be changed to 255 there, but but I think that having this value
here says to the reader that it is controller specific.

> >>> +/*
> >>> + * On the front panel of the Turris Omnia router 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 microcontroller cycles between 8 levels of this global
> >>> brightness (from 100% to 0%), but this
> >>> + * setting can have any integer value between 0 and 100.
> >>> + * It is usable to be able to change this value from software, so
> >>> that it does not start at 100%  
> >> This does not make sense.  
> > It does. The user changes the brightness of all 12 LEDs with the
> > button to his liking and wants to have the same setting after
> > powering the router on again.  
> 
> No the english does not make sense
> 
> " It is usable to be able to change this value from software, so
> that it does not start at 100%"
> 
> "It is usable" is not really clear.

OK I'll change it to "It is convenient to be able to change this
setting from software."

> >>> + * after every power on and annoy the user.
> >>> + * We expose this setting via a sysfs attribute file called
> >>> "brightness". This file lives in the
> >>> + * device directory of the LED controller, not an individual LED,
> >>> so it should not confuse users.
> >>> + */  
> >> Sorry if this has been discussed already
> >>
> >> This seems a bit wonky.  You are overriding the brightness set by
> >> the LED class.  
> > I am not. Pressing the button does not change the brightness read
> > from the /sys/class/leds/<LED>/brightness file. This is different
> > brightness, it is above the classic brightnes in the PWM hierarchy
> > in the microcontroller. I discussed this with Pavel and he said we
> > can call this file brightness as well (since it is brightness of
> > the whole panel), and the file does not reside in
> > /sys/class/leds/<LED> directory.  
> 
> OK then there needs to be some ABI documentation no?

You are right, I'll add this for v5.

Thx