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

[PATCH v4 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.

Changes since v1:
various dt binding fixes
add support for default-brightness

Changes since v2:
more dt binding fixes
drop default-brightness and default-intensity

Changes since v3:
1. add more comments
2. rename reg to cascade
3. redo some line breaking
4. move duplicated pointer calculation into ws2812b_set_byte
5. reword error message
6. get ws2812b_priv from led cdev->dev->parent

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

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

-- 
2.38.1

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

[Chuanhong Guo]

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

Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
Change since v1:
reword commit message
Change since v2: none

 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 v4 2/3] dt-bindings: leds: add worldsemi,ws2812b

[Chuanhong Guo]

Add dt binding schema for WorldSemi WS2812B driven using SPI
bus.

Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
---
Changes since v1:
remove linux driver reference from description
remove some obvious descriptions
fix unit address regex in multi-led property
drop various minItems
add maxItems = 1 to reg
fix node names and property orders in binding example
drop -spi from compatible string
add default-brightness

Change since v2:
drop "this patch" from commit message
rename leds to led-controller
drop default-brightness and default-intensity

Change since v3:
reword commit title

 .../bindings/leds/worldsemi,ws2812b.yaml      | 116 ++++++++++++++++++
 1 file changed, 116 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml

diff --git a/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml b/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml
new file mode 100644
index 000000000000..548c05ac3d31
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.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/worldsemi,ws2812b.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 binding describes a chain of WS2812B LEDs connected to the SPI MOSI pin.
+  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
+
+  reg:
+    maxItems: 1
+
+  spi-max-frequency:
+    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#
+    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
+        maxItems: 3
+
+      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>;
+
+        led-controller@0 {
+            compatible = "worldsemi,ws2812b";
+            reg = <0>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+            spi-max-frequency = <2850000>;
+            multi-led@0 {
+                reg = <0>;
+                color-index = <LED_COLOR_ID_RED LED_COLOR_ID_GREEN LED_COLOR_ID_BLUE>;
+                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 v4 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>
---
Changes since v1:
rename the driver to drop -spi suffix
add support for default-brightness
use fwnode apis for properties

Changes since v2:
drop default-brightness and default-intensity

Changes since v3:
1. add more comments
2. rename reg to cascade
3. redo some line breaking
4. move duplicated pointer calculation into ws2812b_set_byte
5. reword error message
6. get ws2812b_priv from led cdev->dev->parent

 drivers/leds/rgb/Kconfig        |  11 ++
 drivers/leds/rgb/Makefile       |   1 +
 drivers/leds/rgb/leds-ws2812b.c | 231 ++++++++++++++++++++++++++++++++
 3 files changed, 243 insertions(+)
 create mode 100644 drivers/leds/rgb/leds-ws2812b.c

diff --git a/drivers/leds/rgb/Kconfig b/drivers/leds/rgb/Kconfig
index 204cf470beae..5c2081852f01 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
+	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..a6f855eaeb14 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)		+= leds-ws2812b.o
diff --git a/drivers/leds/rgb/leds-ws2812b.c b/drivers/leds/rgb/leds-ws2812b.c
new file mode 100644
index 000000000000..ca9c5caabe48
--- /dev/null
+++ b/drivers/leds/rgb/leds-ws2812b.c
@@ -0,0 +1,231 @@
+// 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/property.h>
+#include <linux/spi/spi.h>
+#include <linux/mutex.h>
+
+#define WS2812B_BYTES_PER_COLOR 3
+#define WS2812B_NUM_COLORS 3
+/* A continuous 0 for 50us+ as the 'reset' signal */
+#define WS2812B_RESET_LEN 18
+
+struct ws2812b_led {
+	struct led_classdev_mc mc_cdev;
+	struct mc_subled subled[WS2812B_NUM_COLORS];
+	int cascade;
+};
+
+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[];
+};
+
+/**
+ * ws2812b_set_byte - convert a byte of data to 3-byte SPI data for pulses
+ * @priv: pointer to the private data structure
+ * @offset: offset of the target byte in the data stream
+ * @val: 1-byte data to be set
+ *
+ * WS2812B receives a stream of bytes from DI, takes the first 3 byte as LED
+ * brightness and pases the rest to the next LED through the DO pin.
+ * This function assembles a single byte of data to the LED:
+ * A bit is represented with a pulse of specific length. A long pulse is a 1
+ * and a short pulse is a 0.
+ * SPI transfers data continuously, MSB first. We can send 3'b100 to create a
+ * 0 pulse and 3'b110 for a 1 pulse. In this way, a byte of data takes up 3
+ * bytes in a SPI transfer:
+ *  1x0 1x0 1x0 1x0 1x0 1x0 1x0 1x0
+ * Let's rearrange it in 8 bits:
+ *  1x01x01x 01x01x01 x01x01x0
+ * The higher 3 bits, middle 2 bits and lower 3 bits are represented with the
+ * 1st, 2nd and 3rd byte in the SPI transfer respectively.
+ * There are only 8 combinations for 3 bits and 4 for 2 bits, so we can create
+ * a lookup table for the 3 bytes.
+ * e.g. For 0x6b -> 2'b01101011:
+ *  Bit 7-5: 3'b011 -> 10011011 -> 0x9b
+ *  Bit 4-3: 2'b01  -> 01001101 -> 0x4d
+ *  Bit 2-0: 3'b011 -> 00110110 -> 0x36
+ */
+static void ws2812b_set_byte(struct ws2812b_priv *priv, size_t offset, u8 val)
+{
+	/* The lookup table for Bit 7-5 4-3 2-0 */
+	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 };
+	u8 *p = priv->data_buf + WS2812B_RESET_LEN + (offset * WS2812B_BYTES_PER_COLOR);
+
+	p[0] = h3b[val >> 5]; /* Bit 7-5 */
+	p[1] = m2b[(val >> 3) & 0x3]; /* Bit 4-3 */
+	p[2] = l3b[val & 0x7]; /* Bit 2-0 */
+}
+
+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 = dev_get_drvdata(cdev->dev->parent);
+	int ret;
+	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(priv, led->cascade * WS2812B_NUM_COLORS + i,
+				 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;
+	int cur_led = 0;
+	struct ws2812b_priv *priv;
+	struct fwnode_handle *led_node;
+	int num_leds, i, cnt, ret;
+
+	num_leds = device_get_child_node_count(dev);
+
+	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, i, 0);
+
+	mutex_init(&priv->mutex);
+	priv->num_leds = num_leds;
+	priv->spi = spi;
+
+	device_for_each_child_node(dev, led_node) {
+		struct led_init_data init_data = {
+			.fwnode = 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 cascade;
+
+		ret = fwnode_property_read_u32(led_node, "reg", &cascade);
+		if (ret) {
+			dev_err(dev, "failed to obtain numerical LED index for %s",
+				fwnode_get_name(led_node));
+			goto ERR_UNREG_LEDS;
+		}
+		if (cascade >= num_leds) {
+			dev_err(dev, "LED index of %s is larger than the number of LEDs.",
+				fwnode_get_name(led_node));
+			ret = -EINVAL;
+			goto ERR_UNREG_LEDS;
+		}
+
+		cnt = fwnode_property_count_u32(led_node, "color-index");
+		if (cnt > 0 && cnt <= WS2812B_NUM_COLORS)
+			fwnode_property_read_u32_array(led_node, "color-index",
+						       color_idx, (size_t)cnt);
+
+		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 = 255;
+		}
+
+		priv->leds[cur_led].cascade = cascade;
+
+		ret = led_classdev_multicolor_register_ext(
+			dev, &priv->leds[cur_led].mc_cdev, &init_data);
+		if (ret) {
+			dev_err(dev, "registration of %s failed.",
+				fwnode_get_name(led_node));
+			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" },
+	{},
+};
+MODULE_DEVICE_TABLE(spi, ws2812b_spi_ids);
+
+static const struct of_device_id ws2812b_dt_ids[] = {
+	{ .compatible = "worldsemi,ws2812b" },
+	{},
+};
+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 v4 2/3] dt-bindings: leds: add worldsemi,ws2812b

[Krzysztof Kozlowski]

On 12/12/2022 05:55, Chuanhong Guo wrote:
> Add dt binding schema for WorldSemi WS2812B driven using SPI
> bus.
> 
> Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
> ---
> Changes since v1:
> remove linux driver reference from description
> remove some obvious descriptions
> fix unit address regex in multi-led property
> drop various minItems
> add maxItems = 1 to reg
> fix node names and property orders in binding example
> drop -spi from compatible string
> add default-brightness
> 
> Change since v2:
> drop "this patch" from commit message
> rename leds to led-controller
> drop default-brightness and default-intensity
> 
> Change since v3:
> reword commit title
> 
>  .../bindings/leds/worldsemi,ws2812b.yaml      | 116 ++++++++++++++++++
>  1 file changed, 116 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml
> 
> diff --git a/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml b/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml
> new file mode 100644
> index 000000000000..548c05ac3d31
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.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/worldsemi,ws2812b.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 binding describes a chain of WS2812B LEDs connected to the SPI MOSI pin.
> +  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
> +
> +  reg:
> +    maxItems: 1
> +
> +  spi-max-frequency:
> +    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#
> +    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
> +        maxItems: 3

In general I am fine with it, although there is still question for
adding more multi-color defines in binding headers to replace this
property - GRB/RBG/GBR and even more for RGBW.

Pavel, Lee, any thoughts from your side?

Best regards,
Krzysztof

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

[Lee Jones]

On Mon, 12 Dec 2022, Krzysztof Kozlowski wrote:

> On 12/12/2022 05:55, Chuanhong Guo wrote:
> > Add dt binding schema for WorldSemi WS2812B driven using SPI
> > bus.
> > 
> > Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
> > ---
> > Changes since v1:
> > remove linux driver reference from description
> > remove some obvious descriptions
> > fix unit address regex in multi-led property
> > drop various minItems
> > add maxItems = 1 to reg
> > fix node names and property orders in binding example
> > drop -spi from compatible string
> > add default-brightness
> > 
> > Change since v2:
> > drop "this patch" from commit message
> > rename leds to led-controller
> > drop default-brightness and default-intensity
> > 
> > Change since v3:
> > reword commit title
> > 
> >  .../bindings/leds/worldsemi,ws2812b.yaml      | 116 ++++++++++++++++++
> >  1 file changed, 116 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml b/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml
> > new file mode 100644
> > index 000000000000..548c05ac3d31
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.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/worldsemi,ws2812b.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 binding describes a chain of WS2812B LEDs connected to the SPI MOSI pin.
> > +  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
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  spi-max-frequency:
> > +    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#
> > +    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
> > +        maxItems: 3
> 
> In general I am fine with it, although there is still question for
> adding more multi-color defines in binding headers to replace this
> property - GRB/RBG/GBR and even more for RGBW.
> 
> Pavel, Lee, any thoughts from your side?

Nothing from me yet.

If Pavel doesn't respond soon, I'll get around to doing a full sweep
after the merge-window has closed.  Actually, most likely the new year
now.

-- 
Lee Jones [李琼斯]

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

[Pavel Machek]

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

Hi!

> > Add dt binding schema for WorldSemi WS2812B driven using SPI
> > bus.
> > 
> > Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
> > ---
> > Changes since v1:
> > remove linux driver reference from description
> > remove some obvious descriptions
> > fix unit address regex in multi-led property
> > drop various minItems
> > add maxItems = 1 to reg
> > fix node names and property orders in binding example
> > drop -spi from compatible string
> > add default-brightness
> > 
> > Change since v2:
> > drop "this patch" from commit message
> > rename leds to led-controller
> > drop default-brightness and default-intensity
> > 
> > Change since v3:
> > reword commit title
> > 
> >  .../bindings/leds/worldsemi,ws2812b.yaml      | 116 ++++++++++++++++++
> >  1 file changed, 116 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml b/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml
> > new file mode 100644
> > index 000000000000..548c05ac3d31
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.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/worldsemi,ws2812b.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 binding describes a chain of WS2812B LEDs connected to the SPI MOSI pin.
> > +  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
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  spi-max-frequency:
> > +    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#
> > +    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
> > +        maxItems: 3
> 
> In general I am fine with it, although there is still question for
> adding more multi-color defines in binding headers to replace this
> property - GRB/RBG/GBR and even more for RGBW.
> 
> Pavel, Lee, any thoughts from your side?

This really needs to mention the name this hardware is known as -- I
believe it is NeoPixel.

Best regards,
								Pavel
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

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

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

[Chuanhong Guo]

Hi!

On Sat, Dec 24, 2022 at 1:19 AM Pavel Machek <pavel@ucw.cz> wrote:
> > In general I am fine with it, although there is still question for
> > adding more multi-color defines in binding headers to replace this
> > property - GRB/RBG/GBR and even more for RGBW.
> >
> > Pavel, Lee, any thoughts from your side?
>
> This really needs to mention the name this hardware is known as -- I
> believe it is NeoPixel.

I'll do so in the next version. I'm actually waiting for an answer to the
comment from Krzysztof (whether I should change color-index) before
sending v5 :)

-- 
Regards,
Chuanhong Guo

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

[Krzysztof Kozlowski]

On 23/12/2022 18:19, Pavel Machek wrote:
> Hi!
> 
>>> Add dt binding schema for WorldSemi WS2812B driven using SPI
>>> bus.
>>>
>>> Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
>>> ---
>>> Changes since v1:
>>> remove linux driver reference from description
>>> remove some obvious descriptions
>>> fix unit address regex in multi-led property
>>> drop various minItems
>>> add maxItems = 1 to reg
>>> fix node names and property orders in binding example
>>> drop -spi from compatible string
>>> add default-brightness
>>>
>>> Change since v2:
>>> drop "this patch" from commit message
>>> rename leds to led-controller
>>> drop default-brightness and default-intensity
>>>
>>> Change since v3:
>>> reword commit title
>>>
>>>  .../bindings/leds/worldsemi,ws2812b.yaml      | 116 ++++++++++++++++++
>>>  1 file changed, 116 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml b/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml
>>> new file mode 100644
>>> index 000000000000..548c05ac3d31
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.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/worldsemi,ws2812b.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 binding describes a chain of WS2812B LEDs connected to the SPI MOSI pin.
>>> +  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
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  spi-max-frequency:
>>> +    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#
>>> +    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
>>> +        maxItems: 3
>>
>> In general I am fine with it, although there is still question for
>> adding more multi-color defines in binding headers to replace this
>> property - GRB/RBG/GBR and even more for RGBW.
>>
>> Pavel, Lee, any thoughts from your side?
> 
> This really needs to mention the name this hardware is known as -- I
> believe it is NeoPixel.

We wait here for feedback on colors... The binding is re-implementing
color, just because of combinations GRB/RBG/GBR, which could be achieved
with new color defines.

Best regards,
Krzysztof

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

[Lee Jones]

On Sat, 24 Dec 2022, Krzysztof Kozlowski wrote:

> On 23/12/2022 18:19, Pavel Machek wrote:
> > Hi!
> > 
> >>> Add dt binding schema for WorldSemi WS2812B driven using SPI
> >>> bus.
> >>>
> >>> Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
> >>> ---
> >>> Changes since v1:
> >>> remove linux driver reference from description
> >>> remove some obvious descriptions
> >>> fix unit address regex in multi-led property
> >>> drop various minItems
> >>> add maxItems = 1 to reg
> >>> fix node names and property orders in binding example
> >>> drop -spi from compatible string
> >>> add default-brightness
> >>>
> >>> Change since v2:
> >>> drop "this patch" from commit message
> >>> rename leds to led-controller
> >>> drop default-brightness and default-intensity
> >>>
> >>> Change since v3:
> >>> reword commit title
> >>>
> >>>  .../bindings/leds/worldsemi,ws2812b.yaml      | 116 ++++++++++++++++++
> >>>  1 file changed, 116 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml b/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml
> >>> new file mode 100644
> >>> index 000000000000..548c05ac3d31
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.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/worldsemi,ws2812b.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 binding describes a chain of WS2812B LEDs connected to the SPI MOSI pin.
> >>> +  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
> >>> +
> >>> +  reg:
> >>> +    maxItems: 1
> >>> +
> >>> +  spi-max-frequency:
> >>> +    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#
> >>> +    unevaluatedProperties: false
> >>> +
> >>> +    properties:
> >>> +      color-index:

Why "index"?

Isn't it just an array of colours rather than an index into something?

> >>> +        description: |
> >>> +          A 3-item array specifying color of each components in this LED. It

Why are you forcing this to 3?

Surely there are multi-colour LEDs containing more or less colours?

> >>> +          should be one of the LED_COLOR_ID_* prefixed definitions from the
> >>> +          header include/dt-bindings/leds/common.h. Defaults to

Isn't "include" a Linuxisum?

> >>> +          <LED_COLOR_ID_GREEN LED_COLOR_ID_RED LED_COLOR_ID_BLUE>
> >>> +          if unspecified.
> >>> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> >>> +        maxItems: 3
> >>
> >> In general I am fine with it, although there is still question for
> >> adding more multi-color defines in binding headers to replace this
> >> property - GRB/RBG/GBR and even more for RGBW.
> >>
> >> Pavel, Lee, any thoughts from your side?
> > 
> > This really needs to mention the name this hardware is known as -- I
> > believe it is NeoPixel.
> 
> We wait here for feedback on colors... The binding is re-implementing
> color, just because of combinations GRB/RBG/GBR, which could be achieved
> with new color defines.

Sure, but where does that end?

How many permutations are there likely to be?

An unlimited array has more of a chance of standing the test of time.

-- 
Lee Jones [李琼斯]

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

[Krzysztof Kozlowski]

On 09/01/2023 17:52, Lee Jones wrote:
> On Sat, 24 Dec 2022, Krzysztof Kozlowski wrote:
> 
>> On 23/12/2022 18:19, Pavel Machek wrote:
>>> Hi!
>>>
>>>>> Add dt binding schema for WorldSemi WS2812B driven using SPI
>>>>> bus.
>>>>>
>>>>> Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
>>>>> ---
>>>>> Changes since v1:
>>>>> remove linux driver reference from description
>>>>> remove some obvious descriptions
>>>>> fix unit address regex in multi-led property
>>>>> drop various minItems
>>>>> add maxItems = 1 to reg
>>>>> fix node names and property orders in binding example
>>>>> drop -spi from compatible string
>>>>> add default-brightness
>>>>>
>>>>> Change since v2:
>>>>> drop "this patch" from commit message
>>>>> rename leds to led-controller
>>>>> drop default-brightness and default-intensity
>>>>>
>>>>> Change since v3:
>>>>> reword commit title
>>>>>
>>>>>  .../bindings/leds/worldsemi,ws2812b.yaml      | 116 ++++++++++++++++++
>>>>>  1 file changed, 116 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml b/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..548c05ac3d31
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.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/worldsemi,ws2812b.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 binding describes a chain of WS2812B LEDs connected to the SPI MOSI pin.
>>>>> +  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
>>>>> +
>>>>> +  reg:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  spi-max-frequency:
>>>>> +    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#
>>>>> +    unevaluatedProperties: false
>>>>> +
>>>>> +    properties:
>>>>> +      color-index:
> 
> Why "index"?
> 
> Isn't it just an array of colours rather than an index into something?

Yeah.

> 
>>>>> +        description: |
>>>>> +          A 3-item array specifying color of each components in this LED. It
> 
> Why are you forcing this to 3?
> 
> Surely there are multi-colour LEDs containing more or less colours?

For this device, because it has only tuples of three.

> 
>>>>> +          should be one of the LED_COLOR_ID_* prefixed definitions from the
>>>>> +          header include/dt-bindings/leds/common.h. Defaults to
> 
> Isn't "include" a Linuxisum?

No, better to have full paths, so automated tools can validate them. If
we ever decide to drop it, we can also make a easier search&replace for
the pattern starting with include/.

> 
>>>>> +          <LED_COLOR_ID_GREEN LED_COLOR_ID_RED LED_COLOR_ID_BLUE>
>>>>> +          if unspecified.
>>>>> +        $ref: /schemas/types.yaml#/definitions/uint32-array
>>>>> +        maxItems: 3
>>>>
>>>> In general I am fine with it, although there is still question for
>>>> adding more multi-color defines in binding headers to replace this
>>>> property - GRB/RBG/GBR and even more for RGBW.
>>>>
>>>> Pavel, Lee, any thoughts from your side?
>>>
>>> This really needs to mention the name this hardware is known as -- I
>>> believe it is NeoPixel.
>>
>> We wait here for feedback on colors... The binding is re-implementing
>> color, just because of combinations GRB/RBG/GBR, which could be achieved
>> with new color defines.
> 
> Sure, but where does that end?
> 
> How many permutations are there likely to be?

For light emitting devices, RGB seems to be used for so long, that I
don't expect more permutations (e.g. CMY). On the other hand, someone
might create LED strip with whatever colors, so maybe indeed better to
allow any variations as in array.

> An unlimited array has more of a chance of standing the test of time.


Best regards,
Krzysztof

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

[Lee Jones]

On Tue, 10 Jan 2023, Krzysztof Kozlowski wrote:

> On 09/01/2023 17:52, Lee Jones wrote:
> > On Sat, 24 Dec 2022, Krzysztof Kozlowski wrote:
> > 
> >> On 23/12/2022 18:19, Pavel Machek wrote:
> >>> Hi!
> >>>
> >>>>> Add dt binding schema for WorldSemi WS2812B driven using SPI
> >>>>> bus.
> >>>>>
> >>>>> Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
> >>>>> ---
> >>>>> Changes since v1:
> >>>>> remove linux driver reference from description
> >>>>> remove some obvious descriptions
> >>>>> fix unit address regex in multi-led property
> >>>>> drop various minItems
> >>>>> add maxItems = 1 to reg
> >>>>> fix node names and property orders in binding example
> >>>>> drop -spi from compatible string
> >>>>> add default-brightness
> >>>>>
> >>>>> Change since v2:
> >>>>> drop "this patch" from commit message
> >>>>> rename leds to led-controller
> >>>>> drop default-brightness and default-intensity
> >>>>>
> >>>>> Change since v3:
> >>>>> reword commit title
> >>>>>
> >>>>>  .../bindings/leds/worldsemi,ws2812b.yaml      | 116 ++++++++++++++++++
> >>>>>  1 file changed, 116 insertions(+)
> >>>>>  create mode 100644 Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml b/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml
> >>>>> new file mode 100644
> >>>>> index 000000000000..548c05ac3d31
> >>>>> --- /dev/null
> >>>>> +++ b/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.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/worldsemi,ws2812b.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 binding describes a chain of WS2812B LEDs connected to the SPI MOSI pin.
> >>>>> +  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
> >>>>> +
> >>>>> +  reg:
> >>>>> +    maxItems: 1
> >>>>> +
> >>>>> +  spi-max-frequency:
> >>>>> +    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#
> >>>>> +    unevaluatedProperties: false
> >>>>> +
> >>>>> +    properties:
> >>>>> +      color-index:
> > 
> > Why "index"?
> > 
> > Isn't it just an array of colours rather than an index into something?
> 
> Yeah.
> 
> > 
> >>>>> +        description: |
> >>>>> +          A 3-item array specifying color of each components in this LED. It
> > 
> > Why are you forcing this to 3?
> > 
> > Surely there are multi-colour LEDs containing more or less colours?
> 
> For this device, because it has only tuples of three.

This doesn't looks like a device specific property to me.

If this is not going to be used by any other device, shouldn't it
contain a prefix?

> >>>>> +          should be one of the LED_COLOR_ID_* prefixed definitions from the
> >>>>> +          header include/dt-bindings/leds/common.h. Defaults to
> > 
> > Isn't "include" a Linuxisum?
> 
> No, better to have full paths, so automated tools can validate them. If
> we ever decide to drop it, we can also make a easier search&replace for
> the pattern starting with include/.

Very well.  It's your train set. :)

> >>>>> +          <LED_COLOR_ID_GREEN LED_COLOR_ID_RED LED_COLOR_ID_BLUE>
> >>>>> +          if unspecified.
> >>>>> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> >>>>> +        maxItems: 3
> >>>>
> >>>> In general I am fine with it, although there is still question for
> >>>> adding more multi-color defines in binding headers to replace this
> >>>> property - GRB/RBG/GBR and even more for RGBW.
> >>>>
> >>>> Pavel, Lee, any thoughts from your side?
> >>>
> >>> This really needs to mention the name this hardware is known as -- I
> >>> believe it is NeoPixel.
> >>
> >> We wait here for feedback on colors... The binding is re-implementing
> >> color, just because of combinations GRB/RBG/GBR, which could be achieved
> >> with new color defines.
> > 
> > Sure, but where does that end?
> > 
> > How many permutations are there likely to be?
> 
> For light emitting devices, RGB seems to be used for so long, that I
> don't expect more permutations (e.g. CMY). On the other hand, someone
> might create LED strip with whatever colors, so maybe indeed better to
> allow any variations as in array.

Even you suggested variation: "even more for RGBW".

Caveat: as you are well aware, "I'm new here", so my input is no more
informed or valuable as yours at this point.  I'm just calling it as I
see it.  If you have strong opinions and they differ wildly from mine,
we may have to take intervention from Pavel.  As it stands, the
property, although slightly restricted IMHO, appears fine.

> > An unlimited array has more of a chance of standing the test of time.

-- 
Lee Jones [李琼斯]

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

[Chuanhong Guo]

Hi!

On Tue, Jan 10, 2023 at 6:21 PM Lee Jones <lee@kernel.org> wrote:
>
> On Tue, 10 Jan 2023, Krzysztof Kozlowski wrote:
>
> > On 09/01/2023 17:52, Lee Jones wrote:
> > > On Sat, 24 Dec 2022, Krzysztof Kozlowski wrote:
> > >
> > >> On 23/12/2022 18:19, Pavel Machek wrote:
> > >>> Hi!
> > >>>
> > >>>>> Add dt binding schema for WorldSemi WS2812B driven using SPI
> > >>>>> bus.
> > >>>>>
> > >>>>> Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
> > >>>>> ---
> > >>>>> Changes since v1:
> > >>>>> remove linux driver reference from description
> > >>>>> remove some obvious descriptions
> > >>>>> fix unit address regex in multi-led property
> > >>>>> drop various minItems
> > >>>>> add maxItems = 1 to reg
> > >>>>> fix node names and property orders in binding example
> > >>>>> drop -spi from compatible string
> > >>>>> add default-brightness
> > >>>>>
> > >>>>> Change since v2:
> > >>>>> drop "this patch" from commit message
> > >>>>> rename leds to led-controller
> > >>>>> drop default-brightness and default-intensity
> > >>>>>
> > >>>>> Change since v3:
> > >>>>> reword commit title
> > >>>>>
> > >>>>>  .../bindings/leds/worldsemi,ws2812b.yaml      | 116 ++++++++++++++++++
> > >>>>>  1 file changed, 116 insertions(+)
> > >>>>>  create mode 100644 Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml
> > >>>>>
> > >>>>> diff --git a/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml b/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml
> > >>>>> new file mode 100644
> > >>>>> index 000000000000..548c05ac3d31
> > >>>>> --- /dev/null
> > >>>>> +++ b/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.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/worldsemi,ws2812b.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 binding describes a chain of WS2812B LEDs connected to the SPI MOSI pin.
> > >>>>> +  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
> > >>>>> +
> > >>>>> +  reg:
> > >>>>> +    maxItems: 1
> > >>>>> +
> > >>>>> +  spi-max-frequency:
> > >>>>> +    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#
> > >>>>> +    unevaluatedProperties: false
> > >>>>> +
> > >>>>> +    properties:
> > >>>>> +      color-index:
> > >
> > > Why "index"?
> > >
> > > Isn't it just an array of colours rather than an index into something?
> >
> > Yeah.

The corresponding sysfs interface is called 'multi_index' so I called
it this way.

> >
> > >
> > >>>>> +        description: |
> > >>>>> +          A 3-item array specifying color of each components in this LED. It
> > >
> > > Why are you forcing this to 3?
> > >
> > > Surely there are multi-colour LEDs containing more or less colours?
> >
> > For this device, because it has only tuples of three.

WS2812B has 3 colors per chip. There are chips using a similar protocol
with 4 colors but my current driver is hard-coded to support exactly 3 colors.

> This doesn't looks like a device specific property to me.
>
> If this is not going to be used by any other device, shouldn't it
> contain a prefix?
>
> > >>>>> +          should be one of the LED_COLOR_ID_* prefixed definitions from the
> > >>>>> +          header include/dt-bindings/leds/common.h. Defaults to
> > >
> > > Isn't "include" a Linuxisum?
> >
> > No, better to have full paths, so automated tools can validate them. If
> > we ever decide to drop it, we can also make a easier search&replace for
> > the pattern starting with include/.
>
> Very well.  It's your train set. :)
>
> > >>>>> +          <LED_COLOR_ID_GREEN LED_COLOR_ID_RED LED_COLOR_ID_BLUE>
> > >>>>> +          if unspecified.
> > >>>>> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> > >>>>> +        maxItems: 3
> > >>>>
> > >>>> In general I am fine with it, although there is still question for
> > >>>> adding more multi-color defines in binding headers to replace this
> > >>>> property - GRB/RBG/GBR and even more for RGBW.
> > >>>>
> > >>>> Pavel, Lee, any thoughts from your side?
> > >>>
> > >>> This really needs to mention the name this hardware is known as -- I
> > >>> believe it is NeoPixel.
> > >>
> > >> We wait here for feedback on colors... The binding is re-implementing
> > >> color, just because of combinations GRB/RBG/GBR, which could be achieved
> > >> with new color defines.
> > >
> > > Sure, but where does that end?
> > >
> > > How many permutations are there likely to be?
> >
> > For light emitting devices, RGB seems to be used for so long, that I
> > don't expect more permutations (e.g. CMY). On the other hand, someone
> > might create LED strip with whatever colors, so maybe indeed better to
> > allow any variations as in array.
>
> Even you suggested variation: "even more for RGBW".
>
> Caveat: as you are well aware, "I'm new here", so my input is no more
> informed or valuable as yours at this point.  I'm just calling it as I
> see it.  If you have strong opinions and they differ wildly from mine,
> we may have to take intervention from Pavel.  As it stands, the
> property, although slightly restricted IMHO, appears fine.
>
> > > An unlimited array has more of a chance of standing the test of time.

I have another idea that avoids this whole conversation: Abandon
color-index completely and determine colors with compatible string
and platform data.
My original idea of this property is to support WS2812B and its clones
with different colors under the same compatible string. Technically
genuine WS2812Bs only come with GRB colors and the clones
should have their own chip names (e.g. xiaomi,hm0807a for an RGB
clone and <unknown vendor>,sk6812 for an RGBW one.). It's
reasonable to have one compatible string per chip for colors, a
chip-specific property. Also, adding more compatible devices to a
driver is less invasive than adding more definitions to leds/common.h
What do you think?

-- 
Regards,
Chuanhong Guo

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

[Krzysztof Kozlowski]

On 11/01/2023 19:53, Chuanhong Guo wrote:
> Hi!
> 
> On Tue, Jan 10, 2023 at 6:21 PM Lee Jones <lee@kernel.org> wrote:
>>
>> On Tue, 10 Jan 2023, Krzysztof Kozlowski wrote:
>>
>>> On 09/01/2023 17:52, Lee Jones wrote:
>>>> On Sat, 24 Dec 2022, Krzysztof Kozlowski wrote:
>>>>
>>>>> On 23/12/2022 18:19, Pavel Machek wrote:
>>>>>> Hi!
>>>>>>
>>>>>>>> Add dt binding schema for WorldSemi WS2812B driven using SPI
>>>>>>>> bus.
>>>>>>>>
>>>>>>>> Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
>>>>>>>> ---
>>>>>>>> Changes since v1:
>>>>>>>> remove linux driver reference from description
>>>>>>>> remove some obvious descriptions
>>>>>>>> fix unit address regex in multi-led property
>>>>>>>> drop various minItems
>>>>>>>> add maxItems = 1 to reg
>>>>>>>> fix node names and property orders in binding example
>>>>>>>> drop -spi from compatible string
>>>>>>>> add default-brightness
>>>>>>>>
>>>>>>>> Change since v2:
>>>>>>>> drop "this patch" from commit message
>>>>>>>> rename leds to led-controller
>>>>>>>> drop default-brightness and default-intensity
>>>>>>>>
>>>>>>>> Change since v3:
>>>>>>>> reword commit title
>>>>>>>>
>>>>>>>>  .../bindings/leds/worldsemi,ws2812b.yaml      | 116 ++++++++++++++++++
>>>>>>>>  1 file changed, 116 insertions(+)
>>>>>>>>  create mode 100644 Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml b/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml
>>>>>>>> new file mode 100644
>>>>>>>> index 000000000000..548c05ac3d31
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.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/worldsemi,ws2812b.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 binding describes a chain of WS2812B LEDs connected to the SPI MOSI pin.
>>>>>>>> +  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
>>>>>>>> +
>>>>>>>> +  reg:
>>>>>>>> +    maxItems: 1
>>>>>>>> +
>>>>>>>> +  spi-max-frequency:
>>>>>>>> +    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#
>>>>>>>> +    unevaluatedProperties: false
>>>>>>>> +
>>>>>>>> +    properties:
>>>>>>>> +      color-index:
>>>>
>>>> Why "index"?
>>>>
>>>> Isn't it just an array of colours rather than an index into something?
>>>
>>> Yeah.
> 
> The corresponding sysfs interface is called 'multi_index' so I called
> it this way.
> 
>>>
>>>>
>>>>>>>> +        description: |
>>>>>>>> +          A 3-item array specifying color of each components in this LED. It
>>>>
>>>> Why are you forcing this to 3?
>>>>
>>>> Surely there are multi-colour LEDs containing more or less colours?
>>>
>>> For this device, because it has only tuples of three.
> 
> WS2812B has 3 colors per chip. There are chips using a similar protocol
> with 4 colors but my current driver is hard-coded to support exactly 3 colors.
> 
>> This doesn't looks like a device specific property to me.
>>
>> If this is not going to be used by any other device, shouldn't it
>> contain a prefix?
>>
>>>>>>>> +          should be one of the LED_COLOR_ID_* prefixed definitions from the
>>>>>>>> +          header include/dt-bindings/leds/common.h. Defaults to
>>>>
>>>> Isn't "include" a Linuxisum?
>>>
>>> No, better to have full paths, so automated tools can validate them. If
>>> we ever decide to drop it, we can also make a easier search&replace for
>>> the pattern starting with include/.
>>
>> Very well.  It's your train set. :)
>>
>>>>>>>> +          <LED_COLOR_ID_GREEN LED_COLOR_ID_RED LED_COLOR_ID_BLUE>
>>>>>>>> +          if unspecified.
>>>>>>>> +        $ref: /schemas/types.yaml#/definitions/uint32-array
>>>>>>>> +        maxItems: 3
>>>>>>>
>>>>>>> In general I am fine with it, although there is still question for
>>>>>>> adding more multi-color defines in binding headers to replace this
>>>>>>> property - GRB/RBG/GBR and even more for RGBW.
>>>>>>>
>>>>>>> Pavel, Lee, any thoughts from your side?
>>>>>>
>>>>>> This really needs to mention the name this hardware is known as -- I
>>>>>> believe it is NeoPixel.
>>>>>
>>>>> We wait here for feedback on colors... The binding is re-implementing
>>>>> color, just because of combinations GRB/RBG/GBR, which could be achieved
>>>>> with new color defines.
>>>>
>>>> Sure, but where does that end?
>>>>
>>>> How many permutations are there likely to be?
>>>
>>> For light emitting devices, RGB seems to be used for so long, that I
>>> don't expect more permutations (e.g. CMY). On the other hand, someone
>>> might create LED strip with whatever colors, so maybe indeed better to
>>> allow any variations as in array.
>>
>> Even you suggested variation: "even more for RGBW".
>>
>> Caveat: as you are well aware, "I'm new here", so my input is no more
>> informed or valuable as yours at this point.  I'm just calling it as I
>> see it.  If you have strong opinions and they differ wildly from mine,
>> we may have to take intervention from Pavel.  As it stands, the
>> property, although slightly restricted IMHO, appears fine.
>>
>>>> An unlimited array has more of a chance of standing the test of time.
> 
> I have another idea that avoids this whole conversation: Abandon
> color-index completely and determine colors with compatible string
> and platform data.
> My original idea of this property is to support WS2812B and its clones
> with different colors under the same compatible string. Technically
> genuine WS2812Bs only come with GRB colors and the clones
> should have their own chip names (e.g. xiaomi,hm0807a for an RGB
> clone and <unknown vendor>,sk6812 for an RGBW one.). It's
> reasonable to have one compatible string per chip for colors, a
> chip-specific property. Also, adding more compatible devices to a
> driver is less invasive than adding more definitions to leds/common.h
> What do you think?

This sounds good. However it should not lead to compatibles like
worldsemi,ws2812b-rgb, worldsemi,ws2812b-bgr etc.

Best regards,
Krzysztof

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

[Lee Jones]

On Thu, 12 Jan 2023, Chuanhong Guo wrote:

> Hi!
> 
> On Tue, Jan 10, 2023 at 6:21 PM Lee Jones <lee@kernel.org> wrote:
> >
> > On Tue, 10 Jan 2023, Krzysztof Kozlowski wrote:
> >
> > > On 09/01/2023 17:52, Lee Jones wrote:
> > > > On Sat, 24 Dec 2022, Krzysztof Kozlowski wrote:
> > > >
> > > >> On 23/12/2022 18:19, Pavel Machek wrote:
> > > >>> Hi!
> > > >>>
> > > >>>>> Add dt binding schema for WorldSemi WS2812B driven using SPI
> > > >>>>> bus.
> > > >>>>>
> > > >>>>> Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
> > > >>>>> ---
> > > >>>>> Changes since v1:
> > > >>>>> remove linux driver reference from description
> > > >>>>> remove some obvious descriptions
> > > >>>>> fix unit address regex in multi-led property
> > > >>>>> drop various minItems
> > > >>>>> add maxItems = 1 to reg
> > > >>>>> fix node names and property orders in binding example
> > > >>>>> drop -spi from compatible string
> > > >>>>> add default-brightness
> > > >>>>>
> > > >>>>> Change since v2:
> > > >>>>> drop "this patch" from commit message
> > > >>>>> rename leds to led-controller
> > > >>>>> drop default-brightness and default-intensity
> > > >>>>>
> > > >>>>> Change since v3:
> > > >>>>> reword commit title
> > > >>>>>
> > > >>>>>  .../bindings/leds/worldsemi,ws2812b.yaml      | 116 ++++++++++++++++++
> > > >>>>>  1 file changed, 116 insertions(+)
> > > >>>>>  create mode 100644 Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml
> > > >>>>>
> > > >>>>> diff --git a/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml b/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml
> > > >>>>> new file mode 100644
> > > >>>>> index 000000000000..548c05ac3d31
> > > >>>>> --- /dev/null
> > > >>>>> +++ b/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.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/worldsemi,ws2812b.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 binding describes a chain of WS2812B LEDs connected to the SPI MOSI pin.
> > > >>>>> +  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
> > > >>>>> +
> > > >>>>> +  reg:
> > > >>>>> +    maxItems: 1
> > > >>>>> +
> > > >>>>> +  spi-max-frequency:
> > > >>>>> +    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#
> > > >>>>> +    unevaluatedProperties: false
> > > >>>>> +
> > > >>>>> +    properties:
> > > >>>>> +      color-index:
> > > >
> > > > Why "index"?
> > > >
> > > > Isn't it just an array of colours rather than an index into something?
> > >
> > > Yeah.
> 
> The corresponding sysfs interface is called 'multi_index' so I called
> it this way.

DT nomenclature should not be affected by Linuxisums.

Please err towards data-sheet and H/W nomenclature.

> > > >>>>> +        description: |
> > > >>>>> +          A 3-item array specifying color of each components in this LED. It
> > > >
> > > > Why are you forcing this to 3?
> > > >
> > > > Surely there are multi-colour LEDs containing more or less colours?
> > >
> > > For this device, because it has only tuples of three.
> 
> WS2812B has 3 colors per chip. There are chips using a similar protocol
> with 4 colors but my current driver is hard-coded to support exactly 3 colors.

So the description is for 'this device' rather than any re-use.

And the handling of this property is only contained in your driver?

In which case, my understanding is that you should use a prefix.

> > This doesn't looks like a device specific property to me.
> >
> > If this is not going to be used by any other device, shouldn't it
> > contain a prefix?
> >
> > > >>>>> +          should be one of the LED_COLOR_ID_* prefixed definitions from the
> > > >>>>> +          header include/dt-bindings/leds/common.h. Defaults to
> > > >
> > > > Isn't "include" a Linuxisum?
> > >
> > > No, better to have full paths, so automated tools can validate them. If
> > > we ever decide to drop it, we can also make a easier search&replace for
> > > the pattern starting with include/.
> >
> > Very well.  It's your train set. :)
> >
> > > >>>>> +          <LED_COLOR_ID_GREEN LED_COLOR_ID_RED LED_COLOR_ID_BLUE>
> > > >>>>> +          if unspecified.
> > > >>>>> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> > > >>>>> +        maxItems: 3
> > > >>>>
> > > >>>> In general I am fine with it, although there is still question for
> > > >>>> adding more multi-color defines in binding headers to replace this
> > > >>>> property - GRB/RBG/GBR and even more for RGBW.
> > > >>>>
> > > >>>> Pavel, Lee, any thoughts from your side?
> > > >>>
> > > >>> This really needs to mention the name this hardware is known as -- I
> > > >>> believe it is NeoPixel.
> > > >>
> > > >> We wait here for feedback on colors... The binding is re-implementing
> > > >> color, just because of combinations GRB/RBG/GBR, which could be achieved
> > > >> with new color defines.
> > > >
> > > > Sure, but where does that end?
> > > >
> > > > How many permutations are there likely to be?
> > >
> > > For light emitting devices, RGB seems to be used for so long, that I
> > > don't expect more permutations (e.g. CMY). On the other hand, someone
> > > might create LED strip with whatever colors, so maybe indeed better to
> > > allow any variations as in array.
> >
> > Even you suggested variation: "even more for RGBW".
> >
> > Caveat: as you are well aware, "I'm new here", so my input is no more
> > informed or valuable as yours at this point.  I'm just calling it as I
> > see it.  If you have strong opinions and they differ wildly from mine,
> > we may have to take intervention from Pavel.  As it stands, the
> > property, although slightly restricted IMHO, appears fine.
> >
> > > > An unlimited array has more of a chance of standing the test of time.
> 
> I have another idea that avoids this whole conversation: Abandon
> color-index completely and determine colors with compatible string
> and platform data.
> My original idea of this property is to support WS2812B and its clones
> with different colors under the same compatible string. Technically
> genuine WS2812Bs only come with GRB colors and the clones
> should have their own chip names (e.g. xiaomi,hm0807a for an RGB
> clone and <unknown vendor>,sk6812 for an RGBW one.). It's
> reasonable to have one compatible string per chip for colors, a
> chip-specific property. Also, adding more compatible devices to a
> driver is less invasive than adding more definitions to leds/common.h
> What do you think?

Yes it's reasonable to have a compatible string per device and yes, you
can do matching (in C) based on compatible string, so this sounds
reasonable to me.  It also sounds reasonable to describe the H/W (inc.
colours along with any ordering, if that's important) inside the DT.

Your call I guess.

-- 
Lee Jones [李琼斯]

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

[Chuanhong Guo]

Hi!

On Fri, Jan 13, 2023 at 10:57 PM Lee Jones <lee@kernel.org> wrote:
> [...]
> So the description is for 'this device' rather than any re-use.
>
> And the handling of this property is only contained in your driver?
>
> In which case, my understanding is that you should use a prefix.

(Just out of curiosity. I don't want this property now.)

My understanding is that a vendor prefix means this property
is for devices by this vendor. However color-index is for supporting
clones, which are chips not made by this vendor. Does a vendor
prefix really apply in this case?

--
Regards,
Chuanhong Guo

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

[Lee Jones]

On Sat, 14 Jan 2023, Chuanhong Guo wrote:

> Hi!
> 
> On Fri, Jan 13, 2023 at 10:57 PM Lee Jones <lee@kernel.org> wrote:
> > [...]
> > So the description is for 'this device' rather than any re-use.
> >
> > And the handling of this property is only contained in your driver?
> >
> > In which case, my understanding is that you should use a prefix.
> 
> (Just out of curiosity. I don't want this property now.)
> 
> My understanding is that a vendor prefix means this property
> is for devices by this vendor. However color-index is for supporting
> clones, which are chips not made by this vendor. Does a vendor
> prefix really apply in this case?

No idea.  Sounds like a grey area.

-- 
Lee Jones [李琼斯]