[PATCH 0/3] leds: add driver for SPI driven WorldSemi WS2812B RGB LEDs

[PATCH 0/3] leds: add driver for SPI driven WorldSemi WS2812B RGB LEDs

[Chuanhong Guo]

This patch adds support for driving a chain of WS2812B LED chips
using SPI bus.

WorldSemi WS2812B is a individually addressable LED chip that
can be chained together and controlled individually using a
single wire. The chip recognize a long pulse as a bit of 1 and
a short pulse as a bit of 0. Host sends a continuous stream
of 24-bits color values, each LED chip takes the first 3 byte
it receives as its color value and passes the leftover bytes to
the next LED on the chain.

This driver simulates this protocol using SPI bus by sending
a long pulse as 3'b110 and a short pulse as 3'b100. The SPI
frequency needs to be 2.105MHz~2.85MHz for the timing to be
correct and the controller needs to transfer all the bytes
continuously.

Chuanhong Guo (3):
  dt-bindings: vendor-prefixes: add an entry for WorldSemi
  dt-bindings: leds: add dt schema for worldsemi,ws2812b-spi
  leds: add driver for SPI driven WorldSemi WS2812B RGB LEDs

 .../bindings/leds/worldsemi,ws2812b-spi.yaml  | 131 +++++++++++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 drivers/leds/rgb/Kconfig                      |  11 +
 drivers/leds/rgb/Makefile                     |   1 +
 drivers/leds/rgb/leds-ws2812b-spi.c           | 222 ++++++++++++++++++
 5 files changed, 367 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/worldsemi,ws2812b-spi.yaml
 create mode 100644 drivers/leds/rgb/leds-ws2812b-spi.c

-- 
2.38.1

[PATCH 1/3] dt-bindings: vendor-prefixes: add an entry for WorldSemi

[Chuanhong Guo]

WorldSemi is well-known for their WS2812B individually-addressable
RGB LEDs on the market.

Add a vendor prefix for it.

Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 10c178d97b02..32274d894664 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -1462,6 +1462,8 @@ patternProperties:
     description: Wondermedia Technologies, Inc.
   "^wobo,.*":
     description: Wobo
+  "^worldsemi,.*":
+    description: WorldSemi Co., Limited
   "^wanchanglong,.*":
     description: Wanchanglong Electronics Technology（SHENZHEN）Co.，Ltd.
   "^x-powers,.*":
-- 
2.38.1

[PATCH 2/3] dt-bindings: leds: add dt schema for worldsemi,ws2812b-spi

[Chuanhong Guo]

This patch adds dt binding schema for WorldSemi WS2812B driven using SPI
bus.

Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
---
 .../bindings/leds/worldsemi,ws2812b-spi.yaml  | 131 ++++++++++++++++++
 1 file changed, 131 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/worldsemi,ws2812b-spi.yaml

diff --git a/Documentation/devicetree/bindings/leds/worldsemi,ws2812b-spi.yaml b/Documentation/devicetree/bindings/leds/worldsemi,ws2812b-spi.yaml
new file mode 100644
index 000000000000..8544bde4f261
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/worldsemi,ws2812b-spi.yaml
@@ -0,0 +1,131 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/worldsemi,ws2812b-spi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: WS2812B LEDs driven using SPI
+
+maintainers:
+  - Chuanhong Guo <gch981213@gmail.com>
+
+description: |
+  WorldSemi WS2812B is a individually addressable LED chip that can be chained
+  together and controlled individually using a single wire.
+  This driver simulates the protocol used by this LED chip with SPI bus.
+  Typical setups includes connecting the data pin of the LED chain to MOSI as
+  the only device or using CS and MOSI with a tri-state voltage-level shifter
+  for the data pin.
+  The SPI frequency needs to be 2.105MHz~2.85MHz for the timing to be correct
+  and the controller needs to send all the bytes continuously.
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+  compatible:
+    const: worldsemi,ws2812b-spi
+
+  reg:
+    description: The chip-select line on the SPI bus
+    maxItems: 1
+
+  spi-max-frequency:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Maximum SPI clocking speed of the device in Hz.
+    minimum: 2105000
+    maximum: 2850000
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+patternProperties:
+  "^multi-led(@[0-9a-f])?$":
+    type: object
+    $ref: leds-class-multicolor.yaml#
+
+    properties:
+      color-index:
+        description: |
+          A 3-item array specifying color of each components in this LED. It
+          should be one of the LED_COLOR_ID_* prefixed definitions from the
+          header include/dt-bindings/leds/common.h. Defaults to
+          <LED_COLOR_ID_GREEN LED_COLOR_ID_RED LED_COLOR_ID_BLUE>
+          if unspecified.
+        $ref: /schemas/types.yaml#/definitions/uint32-array
+        minItems: 3
+        maxItems: 3
+
+      default-intensity:
+        description: |
+          An array of 3 integer specifying the default intensity of each color
+          components in this LED. <255 255 255> if unspecified.
+        $ref: /schemas/types.yaml#/definitions/uint32-array
+        minItems: 3
+        maxItems: 3
+        items:
+          minimum: 0
+          maximum: 255
+
+      reg:
+        description: |
+          Which LED this node represents. The reg of the first LED on the chain
+          is 0.
+
+    required:
+      - reg
+      - color
+      - function
+
+required:
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/leds/common.h>
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ws2812b@0 {
+            #address-cells = <1>;
+            #size-cells = <0>;
+            compatible = "worldsemi,ws2812b-spi";
+            reg = <0>;
+            spi-max-frequency = <2850000>;
+            multi-led@0 {
+                reg = <0>;
+                color-index = <LED_COLOR_ID_RED LED_COLOR_ID_GREEN LED_COLOR_ID_BLUE>;
+                default-intensity = <10 50 90>;
+                color = <LED_COLOR_ID_RGB>;
+                function = LED_FUNCTION_STATUS;
+                function-enumerator = <0>;
+            };
+            multi-led@1 {
+                reg = <1>;
+                color = <LED_COLOR_ID_RGB>;
+                function = LED_FUNCTION_STATUS;
+                function-enumerator = <1>;
+            };
+            multi-led@2 {
+                reg = <2>;
+                color = <LED_COLOR_ID_RGB>;
+                function = LED_FUNCTION_STATUS;
+                function-enumerator = <2>;
+            };
+            multi-led@3 {
+                reg = <3>;
+                color = <LED_COLOR_ID_RGB>;
+                function = LED_FUNCTION_STATUS;
+                function-enumerator = <3>;
+            };
+        };
+    };
+
+...
-- 
2.38.1

[PATCH 3/3] leds: add driver for SPI driven WorldSemi WS2812B RGB LEDs

[Chuanhong Guo]

This patch adds support for driving a chain of WS2812B LED chips
using SPI bus.

WorldSemi WS2812B is a individually addressable LED chip that
can be chained together and controlled individually using a
single wire. The chip recognize a long pulse as a bit of 1 and
a short pulse as a bit of 0. Host sends a continuous stream
of 24-bits color values, each LED chip takes the first 3 byte
it receives as its color value and passes the leftover bytes to
the next LED on the chain.

This driver simulates this protocol using SPI bus by sending
a long pulse as 3'b110 and a short pulse as 3'b100. The SPI
frequency needs to be 2.105MHz~2.85MHz for the timing to be
correct and the controller needs to transfer all the bytes
continuously.

Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
---
 drivers/leds/rgb/Kconfig            |  11 ++
 drivers/leds/rgb/Makefile           |   1 +
 drivers/leds/rgb/leds-ws2812b-spi.c | 222 ++++++++++++++++++++++++++++
 3 files changed, 234 insertions(+)
 create mode 100644 drivers/leds/rgb/leds-ws2812b-spi.c

diff --git a/drivers/leds/rgb/Kconfig b/drivers/leds/rgb/Kconfig
index 204cf470beae..09daa90cd30a 100644
--- a/drivers/leds/rgb/Kconfig
+++ b/drivers/leds/rgb/Kconfig
@@ -26,4 +26,15 @@ config LEDS_QCOM_LPG
 
 	  If compiled as a module, the module will be named leds-qcom-lpg.
 
+config LEDS_WS2812B_SPI
+	tristate "SPI driven WS2812B RGB LED support"
+	depends on OF
+	depends on SPI
+	help
+	  This option enables support for driving daisy-chained WS2812B RGB
+	  LED chips using SPI bus. This driver simulates the single-wire
+	  protocol by sending bits over the SPI MOSI pin. For this to work,
+	  the SPI frequency should be 2.105MHz~2.85MHz and the controller
+	  needs to transfer all the bytes continuously.
+
 endif # LEDS_CLASS_MULTICOLOR
diff --git a/drivers/leds/rgb/Makefile b/drivers/leds/rgb/Makefile
index 0675bc0f6e18..3af2081ea6c3 100644
--- a/drivers/leds/rgb/Makefile
+++ b/drivers/leds/rgb/Makefile
@@ -2,3 +2,4 @@
 
 obj-$(CONFIG_LEDS_PWM_MULTICOLOR)	+= leds-pwm-multicolor.o
 obj-$(CONFIG_LEDS_QCOM_LPG)		+= leds-qcom-lpg.o
+obj-$(CONFIG_LEDS_WS2812B_SPI)		+= leds-ws2812b-spi.o
\ No newline at end of file
diff --git a/drivers/leds/rgb/leds-ws2812b-spi.c b/drivers/leds/rgb/leds-ws2812b-spi.c
new file mode 100644
index 000000000000..1e17bd856474
--- /dev/null
+++ b/drivers/leds/rgb/leds-ws2812b-spi.c
@@ -0,0 +1,222 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * WorldSemi WS2812B individually-addressable LED driver using SPI
+ *
+ * Copyright 2022 Chuanhong Guo <gch981213@gmail.com>
+ *
+ * This driver simulates WS2812B protocol using SPI MOSI pin. A one pulse
+ * is transferred as 3'b110 and a zero pulse is 3'b100. For this driver to
+ * work properly, the SPI frequency should be 2.105MHz~2.85MHz and it needs
+ * to transfer all the bytes continuously.
+ */
+
+#include <linux/led-class-multicolor.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/spi/spi.h>
+#include <linux/mutex.h>
+
+#define WS2812B_BYTES_PER_COLOR 3
+#define WS2812B_NUM_COLORS 3
+#define WS2812B_RESET_LEN 18
+
+struct ws2812b_led {
+	struct led_classdev_mc mc_cdev;
+	struct mc_subled subled[WS2812B_NUM_COLORS];
+	struct ws2812b_priv *priv;
+	int reg;
+};
+
+struct ws2812b_priv {
+	struct led_classdev ldev;
+	struct spi_device *spi;
+	struct mutex mutex;
+	int num_leds;
+	size_t data_len;
+	u8 *data_buf;
+	struct ws2812b_led leds[];
+};
+
+static void ws2812b_set_byte(u8 *p, u8 val)
+{
+	/*
+	 * Every bit of data is represented using 3 bits: 3'b100 for
+	 * 0 and 3'b110 for 1.
+	 * 1 byte of data takes up 3 bytes in a SPI transfer. The higher
+	 * 3 bits, middle 2 bits and lower 3 bits are represented
+	 * with the 1st, 2nd and 3rd byte in the SPI transfer.
+	 * Here's the lookup table for them.
+	 */
+	const u8 h3b[] = { 0x92, 0x93, 0x9a, 0x9b, 0xd2, 0xd3, 0xda, 0xdb };
+	const u8 m2b[] = { 0x49, 0x4d, 0x69, 0x6d };
+	const u8 l3b[] = { 0x24, 0x26, 0x34, 0x36, 0xa4, 0xa6, 0xb4, 0xb6 };
+
+	p[0] = h3b[val >> 5];
+	p[1] = m2b[(val >> 3) & 0x3];
+	p[2] = l3b[val & 0x7];
+}
+
+static int ws2812b_set(struct led_classdev *cdev,
+		       enum led_brightness brightness)
+{
+	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
+	struct ws2812b_led *led =
+		container_of(mc_cdev, struct ws2812b_led, mc_cdev);
+	struct ws2812b_priv *priv = led->priv;
+	u8 *buf = priv->data_buf + WS2812B_RESET_LEN +
+		  led->reg * WS2812B_NUM_COLORS * WS2812B_BYTES_PER_COLOR;
+	int ret = 0;
+	int i;
+
+	led_mc_calc_color_components(mc_cdev, brightness);
+
+	mutex_lock(&priv->mutex);
+	for (i = 0; i < WS2812B_NUM_COLORS; i++)
+		ws2812b_set_byte(buf + i * WS2812B_BYTES_PER_COLOR,
+				 led->subled[i].brightness);
+	ret = spi_write(priv->spi, priv->data_buf, priv->data_len);
+	mutex_unlock(&priv->mutex);
+
+	return ret;
+}
+
+static int ws2812b_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct device_node *np = dev_of_node(dev);
+	int ret = 0, cur_led = 0;
+	struct ws2812b_priv *priv;
+	struct device_node *led_node;
+	int num_leds, i;
+
+	num_leds = of_get_available_child_count(np);
+
+	priv = devm_kzalloc(dev, struct_size(priv, leds, num_leds), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+	priv->data_len =
+		num_leds * WS2812B_BYTES_PER_COLOR * WS2812B_NUM_COLORS +
+		WS2812B_RESET_LEN;
+	priv->data_buf = kzalloc(priv->data_len, GFP_KERNEL);
+	if (!priv->data_buf)
+		return -ENOMEM;
+
+	for (i = 0; i < num_leds * WS2812B_NUM_COLORS; i++)
+		ws2812b_set_byte(priv->data_buf + WS2812B_RESET_LEN +
+					 i * WS2812B_BYTES_PER_COLOR,
+				 0);
+
+	mutex_init(&priv->mutex);
+	priv->num_leds = num_leds;
+	priv->spi = spi;
+
+	for_each_available_child_of_node(np, led_node) {
+		u32 reg = -1;
+		struct led_init_data init_data = {
+			.fwnode = of_fwnode_handle(led_node),
+		};
+		/* WS2812B LEDs usually come with GRB color */
+		u32 color_idx[WS2812B_NUM_COLORS] = {
+			LED_COLOR_ID_GREEN,
+			LED_COLOR_ID_RED,
+			LED_COLOR_ID_BLUE,
+		};
+		u32 default_intensity[WS2812B_NUM_COLORS] = { 255, 255, 255 };
+
+		ret = of_property_read_u32(led_node, "reg", &reg);
+		if (ret) {
+			dev_err(dev, "failed to get reg of the %dth led.",
+				cur_led);
+			goto ERR_UNREG_LEDS;
+		}
+		if (reg >= num_leds) {
+			dev_err(dev, "reg of the %dth led is too big.",
+				cur_led);
+			ret = -EINVAL;
+			goto ERR_UNREG_LEDS;
+		}
+
+		of_property_read_u32_array(led_node, "color-index", color_idx,
+					   WS2812B_NUM_COLORS);
+		of_property_read_u32_array(led_node, "default-intensity",
+					   default_intensity,
+					   WS2812B_NUM_COLORS);
+
+		priv->leds[cur_led].mc_cdev.subled_info =
+			priv->leds[cur_led].subled;
+		priv->leds[cur_led].mc_cdev.num_colors = WS2812B_NUM_COLORS;
+		priv->leds[cur_led].mc_cdev.led_cdev.max_brightness = 255;
+		priv->leds[cur_led].mc_cdev.led_cdev.brightness_set_blocking =
+			ws2812b_set;
+
+		for (i = 0; i < WS2812B_NUM_COLORS; i++) {
+			priv->leds[cur_led].subled[i].color_index =
+				color_idx[i];
+			priv->leds[cur_led].subled[i].intensity =
+				default_intensity[i];
+		}
+
+		priv->leds[cur_led].priv = priv;
+		priv->leds[cur_led].reg = reg;
+
+		ret = led_classdev_multicolor_register_ext(
+			dev, &priv->leds[cur_led].mc_cdev, &init_data);
+		if (ret) {
+			dev_err(dev, "registration of led@%d failed.", reg);
+			goto ERR_UNREG_LEDS;
+		}
+		cur_led++;
+	}
+
+	spi_set_drvdata(spi, priv);
+
+	return 0;
+ERR_UNREG_LEDS:
+	for (; cur_led >= 0; cur_led--)
+		led_classdev_multicolor_unregister(
+			&priv->leds[cur_led].mc_cdev);
+	mutex_destroy(&priv->mutex);
+	kfree(priv->data_buf);
+	return ret;
+}
+
+static void ws2812b_remove(struct spi_device *spi)
+{
+	struct ws2812b_priv *priv = spi_get_drvdata(spi);
+	int cur_led;
+
+	for (cur_led = priv->num_leds - 1; cur_led >= 0; cur_led--)
+		led_classdev_multicolor_unregister(
+			&priv->leds[cur_led].mc_cdev);
+	kfree(priv->data_buf);
+	mutex_destroy(&priv->mutex);
+}
+
+static const struct spi_device_id ws2812b_spi_ids[] = {
+	{ "ws2812b-spi" },
+	{},
+};
+MODULE_DEVICE_TABLE(spi, ws2812b_spi_ids);
+
+static const struct of_device_id ws2812b_dt_ids[] = {
+	{ .compatible = "worldsemi,ws2812b-spi" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, ws2812b_dt_ids);
+
+static struct spi_driver ws2812b_driver = {
+	.probe		= ws2812b_probe,
+	.remove		= ws2812b_remove,
+	.id_table	= ws2812b_spi_ids,
+	.driver = {
+		.name		= KBUILD_MODNAME,
+		.of_match_table	= ws2812b_dt_ids,
+	},
+};
+
+module_spi_driver(ws2812b_driver);
+
+MODULE_AUTHOR("Chuanhong Guo <gch981213@gmail.com>");
+MODULE_DESCRIPTION("WS2812B LED driver using SPI");
+MODULE_LICENSE("GPL");
-- 
2.38.1

Re: [PATCH 1/3] dt-bindings: vendor-prefixes: add an entry for WorldSemi

[Krzysztof Kozlowski]

On 29/11/2022 15:09, Chuanhong Guo wrote:
> WorldSemi is well-known for their WS2812B individually-addressable

Drop marketing. If they were well known, they would be already in vendor
prefixes, right?

Add vendor prefix for WorldSemi that makes WS2812B
individually-addressable RGB LEDs.

> RGB LEDs on the market.
> 
> Add a vendor prefix for it.
> 
> Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
> ---
>  Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> index 10c178d97b02..32274d894664 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> @@ -1462,6 +1462,8 @@ patternProperties:
>      description: Wondermedia Technologies, Inc.
>    "^wobo,.*":
>      description: Wobo
> +  "^worldsemi,.*":
> +    description: WorldSemi Co., Limited
>    "^wanchanglong,.*":
>      description: Wanchanglong Electronics Technology（SHENZHEN）Co.，Ltd.
>    "^x-powers,.*":

Best regards,
Krzysztof

Re: [PATCH 2/3] dt-bindings: leds: add dt schema for worldsemi,ws2812b-spi

[Krzysztof Kozlowski]

On 29/11/2022 15:09, Chuanhong Guo wrote:
> This patch adds dt binding schema for WorldSemi WS2812B driven using SPI
> bus.
> 
> Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
> ---
>  .../bindings/leds/worldsemi,ws2812b-spi.yaml  | 131 ++++++++++++++++++
>  1 file changed, 131 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/worldsemi,ws2812b-spi.yaml
> 
> diff --git a/Documentation/devicetree/bindings/leds/worldsemi,ws2812b-spi.yaml b/Documentation/devicetree/bindings/leds/worldsemi,ws2812b-spi.yaml
> new file mode 100644
> index 000000000000..8544bde4f261
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/worldsemi,ws2812b-spi.yaml
> @@ -0,0 +1,131 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/worldsemi,ws2812b-spi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: WS2812B LEDs driven using SPI
> +
> +maintainers:
> +  - Chuanhong Guo <gch981213@gmail.com>
> +
> +description: |
> +  WorldSemi WS2812B is a individually addressable LED chip that can be chained
> +  together and controlled individually using a single wire.
> +  This driver simulates the protocol used by this LED chip with SPI bus.

Drop references to Linux driver, unless important for the binding.

> +  Typical setups includes connecting the data pin of the LED chain to MOSI as
> +  the only device or using CS and MOSI with a tri-state voltage-level shifter
> +  for the data pin.
> +  The SPI frequency needs to be 2.105MHz~2.85MHz for the timing to be correct
> +  and the controller needs to send all the bytes continuously.
> +
> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> +  compatible:
> +    const: worldsemi,ws2812b-spi

Drop "-spi". Compatibles are not supposed to include bus information.
The same for file name.

> +
> +  reg:
> +    description: The chip-select line on the SPI bus

Drop description, it's obvious.

> +    maxItems: 1
> +
> +  spi-max-frequency:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Maximum SPI clocking speed of the device in Hz.

No need for ref and description. It comes from spi-peripheral-props.

> +    minimum: 2105000
> +    maximum: 2850000
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +patternProperties:
> +  "^multi-led(@[0-9a-f])?$":

Why unit address is optional?

> +    type: object
> +    $ref: leds-class-multicolor.yaml#

    unevaluatedProperties: false

> +
> +    properties:
> +      color-index:
> +        description: |
> +          A 3-item array specifying color of each components in this LED. It
> +          should be one of the LED_COLOR_ID_* prefixed definitions from the
> +          header include/dt-bindings/leds/common.h. Defaults to
> +          <LED_COLOR_ID_GREEN LED_COLOR_ID_RED LED_COLOR_ID_BLUE>
> +          if unspecified.
> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> +        minItems: 3

Drop minItems.... but see comment below:

> +        maxItems: 3

Why this is different than other multi-color LEDs? I would expect here
children with common.yaml.

> +
> +      default-intensity:
> +        description: |
> +          An array of 3 integer specifying the default intensity of each color
> +          components in this LED. <255 255 255> if unspecified.
> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> +        minItems: 3

Drop minItems.... but:

> +        maxItems: 3
> +        items:
> +          minimum: 0
> +          maximum: 255

default: 255

What controls the intensity? Don't you have PWM there?

> +
> +      reg:
> +        description: |
> +          Which LED this node represents. The reg of the first LED on the chain
> +          is 0.

maxItems: 1

> +
> +    required:
> +      - reg
> +      - color
> +      - function
> +
> +required:
> +  - compatible
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/leds/common.h>
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        ws2812b@0 {

Node names should be generic.
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            compatible = "worldsemi,ws2812b-spi";
> +            reg = <0>;

compatible is first property, reg is second.


Best regards,
Krzysztof

Re: [PATCH 2/3] dt-bindings: leds: add dt schema for worldsemi,ws2812b-spi

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1448 bytes --]

Hi,

On Tue, Nov 29, 2022 at 05:54:52PM +0100, Krzysztof Kozlowski wrote:
> On 29/11/2022 15:09, Chuanhong Guo wrote:
> > This patch adds dt binding schema for WorldSemi WS2812B driven using SPI
> > bus.

Nice, I have a hobby project for something similar (SK6812, which is
basically WS2812 with an extra white channel). I will switch to this
work and extend it once it lands :)

[...]

> > +      default-intensity:
> > +        description: |
> > +          An array of 3 integer specifying the default intensity of each color
> > +          components in this LED. <255 255 255> if unspecified.
> > +        $ref: /schemas/types.yaml#/definitions/uint32-array
> > +        minItems: 3
> 
> Drop minItems.... but:
> 
> > +        maxItems: 3
> > +        items:
> > +          minimum: 0
> > +          maximum: 255
> 
> default: 255

I would argue, that the default should be 0 (off) instead of 255
(full power).

> What controls the intensity? Don't you have PWM there?

WS2812 is a RGB led, which contains a small Microcontroller. The µC
takes 24 byte intensity data from a serial input and then passes on
any following bits to the next LED. SPI clk and chip-select are
ignored (chip-select support can be trivially added though).

You can find them everywhere nowadays, since they are quite cheap
(a few cents per LED) and need only one MOSI pin to control hundreds
of LEDs.

[...]

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/3] dt-bindings: vendor-prefixes: add an entry for WorldSemi

[Chuanhong Guo]

Hi!

On Wed, Nov 30, 2022 at 12:46 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 29/11/2022 15:09, Chuanhong Guo wrote:
> > WorldSemi is well-known for their WS2812B individually-addressable
>
> Drop marketing. If they were well known, they would be already in vendor
> prefixes, right?
>
> Add vendor prefix for WorldSemi that makes WS2812B
> individually-addressable RGB LEDs.
>

 OK. I'll do so in v2.

-- 
Regards,
Chuanhong Guo

Re: [PATCH 2/3] dt-bindings: leds: add dt schema for worldsemi,ws2812b-spi

[Krzysztof Kozlowski]

On 30/11/2022 01:14, Sebastian Reichel wrote:
>>> +      default-intensity:
>>> +        description: |
>>> +          An array of 3 integer specifying the default intensity of each color
>>> +          components in this LED. <255 255 255> if unspecified.
>>> +        $ref: /schemas/types.yaml#/definitions/uint32-array
>>> +        minItems: 3
>>
>> Drop minItems.... but:
>>
>>> +        maxItems: 3
>>> +        items:
>>> +          minimum: 0
>>> +          maximum: 255
>>
>> default: 255
> 
> I would argue, that the default should be 0 (off) instead of 255
> (full power).

Yes.

> 
>> What controls the intensity? Don't you have PWM there?
> 
> WS2812 is a RGB led, which contains a small Microcontroller. The µC
> takes 24 byte intensity data from a serial input and then passes on
> any following bits to the next LED. SPI clk and chip-select are
> ignored (chip-select support can be trivially added though).
> 
> You can find them everywhere nowadays, since they are quite cheap
> (a few cents per LED) and need only one MOSI pin to control hundreds
> of LEDs.

OK. This should be anyway existing property, so default-brightness.

Best regards,
Krzysztof

Re: [PATCH 2/3] dt-bindings: leds: add dt schema for worldsemi,ws2812b-spi

[Chuanhong Guo]

Hi!

On Wed, Nov 30, 2022 at 12:54 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
> > +description: |
> > +  WorldSemi WS2812B is a individually addressable LED chip that can be chained
> > +  together and controlled individually using a single wire.
> > +  This driver simulates the protocol used by this LED chip with SPI bus.
>
> Drop references to Linux driver, unless important for the binding.

I think the SPI part is important. (I'll explain it below.) What about:

This binding describes a chain of WS2812B LEDs connected to the SPI MOSI pin.

instead?

> > +  Typical setups includes connecting the data pin of the LED chain to MOSI as
> > +  the only device or using CS and MOSI with a tri-state voltage-level shifter
> > +  for the data pin.
> > +  The SPI frequency needs to be 2.105MHz~2.85MHz for the timing to be correct
> > +  and the controller needs to send all the bytes continuously.
> > +
> > +allOf:
> > +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    const: worldsemi,ws2812b-spi
>
> Drop "-spi". Compatibles are not supposed to include bus information.
> The same for file name.

WS2812B isn't a SPI chip. It's controlled with only a single wire and
can be driven
using anything that can produce a long and a short pulse meeting its timing
requirement.
This driver uses a SPI bus to send the pulses, but it can also be
controlled with
I2S and the PIO pins on a Raspberry Pi Pico.
This spi suffix is to distinguish it from other possible
implementations if someone
else submits a support with a different peripheral.

>
> > +
> > +  reg:
> > +    description: The chip-select line on the SPI bus
>
> Drop description, it's obvious.

OK.

>
> > +    maxItems: 1
> > +
> > +  spi-max-frequency:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description:
> > +      Maximum SPI clocking speed of the device in Hz.
>
> No need for ref and description. It comes from spi-peripheral-props.

OK.

>
> > +    minimum: 2105000
> > +    maximum: 2850000
> > +
> > +  "#address-cells":
> > +    const: 1
> > +
> > +  "#size-cells":
> > +    const: 0
> > +
> > +patternProperties:
> > +  "^multi-led(@[0-9a-f])?$":
>
> Why unit address is optional?

It isn't. I copy-pasted it from led-class-multicolor.yaml and
didn't check the exact regex.
I'll fix it in the next version.

>
> > +    type: object
> > +    $ref: leds-class-multicolor.yaml#
>
>     unevaluatedProperties: false

OK.

> > +
> > +    properties:
> > +      color-index:
> > +        description: |
> > +          A 3-item array specifying color of each components in this LED. It
> > +          should be one of the LED_COLOR_ID_* prefixed definitions from the
> > +          header include/dt-bindings/leds/common.h. Defaults to
> > +          <LED_COLOR_ID_GREEN LED_COLOR_ID_RED LED_COLOR_ID_BLUE>
> > +          if unspecified.
> > +        $ref: /schemas/types.yaml#/definitions/uint32-array
> > +        minItems: 3
>
> Drop minItems.... but see comment below:
>
> > +        maxItems: 3
>
> Why this is different than other multi-color LEDs? I would expect here
> children with common.yaml.

WS2812B is a single LED package with 3 diodes and a microcontroller.
Each LED package has 3 colors. The original chip comes with GRB
color while there are some clones with RGB arrangement instead.
The LED chain can be really long so I'd like to simplify the binding
by using a single property to override the only variable, color, here.

>
> > +
> > +      default-intensity:
> > +        description: |
> > +          An array of 3 integer specifying the default intensity of each color
> > +          components in this LED. <255 255 255> if unspecified.
> > +        $ref: /schemas/types.yaml#/definitions/uint32-array
> > +        minItems: 3
>
> Drop minItems.... but:
>
> > +        maxItems: 3
> > +        items:
> > +          minimum: 0
> > +          maximum: 255
>
> default: 255
>
> What controls the intensity? Don't you have PWM there?

The LED takes 3-byte brightness value of each color. This property is used to
specify the default multi_intensity field for the multi-color LED. The final
brightness value is calculated with led_mc_calc_color_components like this:

mcled_cdev->subled_info[i].brightness = brightness *
mcled_cdev->subled_info[i].intensity / led_cdev->max_brightness;

The LED chip takes exactly 8 bits for the brightness (max_brightness = 255
which can't be changed.), so according to the formula above the maximum
intensity should be 255.

>
> > +
> > +      reg:
> > +        description: |
> > +          Which LED this node represents. The reg of the first LED on the chain
> > +          is 0.
>
> maxItems: 1

OK.

>
> > +
> > +    required:
> > +      - reg
> > +      - color
> > +      - function
> > +
> > +required:
> > +  - compatible
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/leds/common.h>
> > +    spi {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        ws2812b@0 {
>
> Node names should be generic.
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

OK. I'll use leds instead.

>
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +            compatible = "worldsemi,ws2812b-spi";
> > +            reg = <0>;
>
> compatible is first property, reg is second.

Got it.

--
Regards,
Chuanhong Guo

Re: [PATCH 2/3] dt-bindings: leds: add dt schema for worldsemi,ws2812b-spi

[Chuanhong Guo]

Hi!

On Wed, Nov 30, 2022 at 4:33 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
> >> What controls the intensity? Don't you have PWM there?
> >
> > WS2812 is a RGB led, which contains a small Microcontroller. The µC
> > takes 24 byte intensity data from a serial input and then passes on
> > any following bits to the next LED. SPI clk and chip-select are
> > ignored (chip-select support can be trivially added though).
> >
> > You can find them everywhere nowadays, since they are quite cheap
> > (a few cents per LED) and need only one MOSI pin to control hundreds
> > of LEDs.
>
> OK. This should be anyway existing property, so default-brightness.

default-intensity is a different property. Intensity controls only the
color for a multicolor led. The final brightness is calculated with
this intensity value with the single brightness value of the current
LED. (See my previous mail.)

-- 
Regards,
Chuanhong Guo

Re: [PATCH 2/3] dt-bindings: leds: add dt schema for worldsemi,ws2812b-spi

[Krzysztof Kozlowski]

On 30/11/2022 09:36, Chuanhong Guo wrote:
> Hi!
> 
> On Wed, Nov 30, 2022 at 12:54 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>> +description: |
>>> +  WorldSemi WS2812B is a individually addressable LED chip that can be chained
>>> +  together and controlled individually using a single wire.
>>> +  This driver simulates the protocol used by this LED chip with SPI bus.
>>
>> Drop references to Linux driver, unless important for the binding.
> 
> I think the SPI part is important. (I'll explain it below.) What about:
> 
> This binding describes a chain of WS2812B LEDs connected to the SPI MOSI pin.
> 
> instead?

OK

> 
>>> +  Typical setups includes connecting the data pin of the LED chain to MOSI as
>>> +  the only device or using CS and MOSI with a tri-state voltage-level shifter
>>> +  for the data pin.
>>> +  The SPI frequency needs to be 2.105MHz~2.85MHz for the timing to be correct
>>> +  and the controller needs to send all the bytes continuously.
>>> +
>>> +allOf:
>>> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: worldsemi,ws2812b-spi
>>
>> Drop "-spi". Compatibles are not supposed to include bus information.
>> The same for file name.
> 
> WS2812B isn't a SPI chip. It's controlled with only a single wire and
> can be driven
> using anything that can produce a long and a short pulse meeting its timing
> requirement.
> This driver uses a SPI bus to send the pulses, but it can also be
> controlled with
> I2S and the PIO pins on a Raspberry Pi Pico.
> This spi suffix is to distinguish it from other possible
> implementations if someone
> else submits a support with a different peripheral.

And that's exactly what I said - the compatibles should not include bus
information. The bus information comes from... the bus!

> 
>>
>>> +
>>> +  reg:
>>> +    description: The chip-select line on the SPI bus
>>
>> Drop description, it's obvious.
> 
> OK.
> 
>>
>>> +    maxItems: 1
>>> +
>>> +  spi-max-frequency:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    description:
>>> +      Maximum SPI clocking speed of the device in Hz.
>>
>> No need for ref and description. It comes from spi-peripheral-props.
> 
> OK.
> 
>>
>>> +    minimum: 2105000
>>> +    maximum: 2850000
>>> +
>>> +  "#address-cells":
>>> +    const: 1
>>> +
>>> +  "#size-cells":
>>> +    const: 0
>>> +
>>> +patternProperties:
>>> +  "^multi-led(@[0-9a-f])?$":
>>
>> Why unit address is optional?
> 
> It isn't. I copy-pasted it from led-class-multicolor.yaml and
> didn't check the exact regex.
> I'll fix it in the next version.

Make it required and matching your case.

> 
>>
>>> +    type: object
>>> +    $ref: leds-class-multicolor.yaml#
>>
>>     unevaluatedProperties: false
> 
> OK.
> 
>>> +
>>> +    properties:
>>> +      color-index:
>>> +        description: |
>>> +          A 3-item array specifying color of each components in this LED. It
>>> +          should be one of the LED_COLOR_ID_* prefixed definitions from the
>>> +          header include/dt-bindings/leds/common.h. Defaults to
>>> +          <LED_COLOR_ID_GREEN LED_COLOR_ID_RED LED_COLOR_ID_BLUE>
>>> +          if unspecified.
>>> +        $ref: /schemas/types.yaml#/definitions/uint32-array
>>> +        minItems: 3
>>
>> Drop minItems.... but see comment below:
>>
>>> +        maxItems: 3
>>
>> Why this is different than other multi-color LEDs? I would expect here
>> children with common.yaml.
> 
> WS2812B is a single LED package with 3 diodes and a microcontroller.
> Each LED package has 3 colors. The original chip comes with GRB
> color while there are some clones with RGB arrangement instead.
> The LED chain can be really long so I'd like to simplify the binding
> by using a single property to override the only variable, color, here.

OK, that makes sense.

> 
>>
>>> +
>>> +      default-intensity:
>>> +        description: |
>>> +          An array of 3 integer specifying the default intensity of each color
>>> +          components in this LED. <255 255 255> if unspecified.
>>> +        $ref: /schemas/types.yaml#/definitions/uint32-array
>>> +        minItems: 3
>>
>> Drop minItems.... but:
>>
>>> +        maxItems: 3
>>> +        items:
>>> +          minimum: 0
>>> +          maximum: 255
>>
>> default: 255
>>
>> What controls the intensity? Don't you have PWM there?
> 
> The LED takes 3-byte brightness value of each color. This property is used to
> specify the default multi_intensity field for the multi-color LED. The final
> brightness value is calculated with led_mc_calc_color_components like this:
> 
> mcled_cdev->subled_info[i].brightness = brightness *
> mcled_cdev->subled_info[i].intensity / led_cdev->max_brightness;
> 
> The LED chip takes exactly 8 bits for the brightness (max_brightness = 255
> which can't be changed.), so according to the formula above the maximum
> intensity should be 255.

So this is brightness of each color... why insisting on calling it
differently?

Best regards,
Krzysztof

Re: [PATCH 2/3] dt-bindings: leds: add dt schema for worldsemi,ws2812b-spi

[Chuanhong Guo]

Hi!

On Wed, Nov 30, 2022 at 5:08 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
> And that's exactly what I said - the compatibles should not include bus
> information. The bus information comes from... the bus!

Oh. I thought there will be a conflict if there is a SPI driver and
, say, an I2C driver with the same compatible string.

> [...]
> >>
> >> Why unit address is optional?
> >
> > It isn't. I copy-pasted it from led-class-multicolor.yaml and
> > didn't check the exact regex.
> > I'll fix it in the next version.
>
> Make it required and matching your case.

Got it.

> [...]
> >>> +      default-intensity:
> >>> +        description: |
> >>> +          An array of 3 integer specifying the default intensity of each color
> >>> +          components in this LED. <255 255 255> if unspecified.
> >>> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> >>> +        minItems: 3
> [...]
> So this is brightness of each color...

I don't think so.
See the kernel doc for multicolor LED:
https://docs.kernel.org/leds/leds-class-multicolor.html
This property sets the sysfs file multi_intensity while the
actual LED brightness is controlled with another sysfs
file called 'brightness'.
Setting multi_intensity alone doesn't change the LED
brightness at all.

-- 
Regards,
Chuanhong Guo

Re: [PATCH 2/3] dt-bindings: leds: add dt schema for worldsemi,ws2812b-spi

[Krzysztof Kozlowski]

On 30/11/2022 10:25, Chuanhong Guo wrote:
> Hi!
> 
> On Wed, Nov 30, 2022 at 5:08 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>> And that's exactly what I said - the compatibles should not include bus
>> information. The bus information comes from... the bus!
> 
> Oh. I thought there will be a conflict if there is a SPI driver and
> , say, an I2C driver with the same compatible string.

We already have such. For example: adi,adxl312

> 
>> [...]
>>>>
>>>> Why unit address is optional?
>>>
>>> It isn't. I copy-pasted it from led-class-multicolor.yaml and
>>> didn't check the exact regex.
>>> I'll fix it in the next version.
>>
>> Make it required and matching your case.
> 
> Got it.
> 
>> [...]
>>>>> +      default-intensity:
>>>>> +        description: |
>>>>> +          An array of 3 integer specifying the default intensity of each color
>>>>> +          components in this LED. <255 255 255> if unspecified.
>>>>> +        $ref: /schemas/types.yaml#/definitions/uint32-array
>>>>> +        minItems: 3
>> [...]
>> So this is brightness of each color...
> 
> I don't think so.
> See the kernel doc for multicolor LED:
> https://docs.kernel.org/leds/leds-class-multicolor.html
> This property sets the sysfs file multi_intensity while the
> actual LED brightness is controlled with another sysfs
> file called 'brightness'.
> Setting multi_intensity alone doesn't change the LED
> brightness at all.

If you had brightness, that would be correct. But you do not have
brightness, right? Therefore the final brightness is always:

subled[i].brightness = 255 * subled[i].intensity / max_brightness (also
255);

Or your bindings are incomplete...

Best regards,
Krzysztof

Re: [PATCH 2/3] dt-bindings: leds: add dt schema for worldsemi,ws2812b-spi

[Chuanhong Guo]

Hi!

On Wed, Nov 30, 2022 at 7:59 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
> >> [...]
> >> So this is brightness of each color...
> >
> > I don't think so.
> > See the kernel doc for multicolor LED:
> > https://docs.kernel.org/leds/leds-class-multicolor.html
> > This property sets the sysfs file multi_intensity while the
> > actual LED brightness is controlled with another sysfs
> > file called 'brightness'.
> > Setting multi_intensity alone doesn't change the LED
> > brightness at all.
>
> If you had brightness, that would be correct. But you do not have
> brightness, right?

The brightness can be set using sysfs, but I didn't implement a
default brightness in my driver.
My original intent for this property is to set a default color for legacy
led softwares which can only control the brightness and doesn't
support setting the color value yet.

> Therefore the final brightness is always:
>
> subled[i].brightness = 255 * subled[i].intensity / max_brightness (also
> 255);
>
> Or your bindings are incomplete...

I do agree that adding a default-brightness will make this more
useful. I'll add this feature in the next version.

-- 
Regards,
Chuanhong Guo