LinuxLists.cc - [PATCH v2 0/3] leds: add driver for SPI driven WorldSemi WS2812B RGB LEDs

2022-12-02 03:51:59

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

2022-12-02 04:14:54

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

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 <[email protected]>
---
Changes since v1:
rename the driver to drop -spi suffix
add support for default-brightness
use fwnode apis for properties

drivers/leds/rgb/Kconfig | 11 ++
drivers/leds/rgb/Makefile | 1 +
drivers/leds/rgb/leds-ws2812b.c | 235 ++++++++++++++++++++++++++++++++
3 files changed, 247 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..ca2ae83de573
--- /dev/null
+++ b/drivers/leds/rgb/leds-ws2812b.c
@@ -0,0 +1,235 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * WorldSemi WS2812B individually-addressable LED driver using SPI
+ *
+ * Copyright 2022 Chuanhong Guo <[email protected]>
+ *
+ * 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
+#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;
+ int ret = 0, cur_led = 0;
+ struct ws2812b_priv *priv;
+ struct fwnode_handle *led_node;
+ int num_leds, i, cnt;
+
+ 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->data_buf + WS2812B_RESET_LEN +
+ i * WS2812B_BYTES_PER_COLOR,
+ 0);
+
+ mutex_init(&priv->mutex);
+ priv->num_leds = num_leds;
+ priv->spi = spi;
+
+ device_for_each_child_node(dev, led_node) {
+ u32 reg = -1;
+ 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 default_intensity[WS2812B_NUM_COLORS] = { 255, 255, 255 };
+ u32 default_brightness = 0;
+
+ ret = fwnode_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;
+ }
+
+ 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);
+ cnt = fwnode_property_count_u32(led_node, "default-intensity");
+ if (cnt > 0 && cnt <= WS2812B_NUM_COLORS)
+ fwnode_property_read_u32_array(led_node,
+ "default-intensity",
+ default_intensity,
+ (size_t)cnt);
+ fwnode_property_read_u32(led_node, "default-brightness",
+ &default_brightness);
+
+ 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.brightness =
+ default_brightness;
+ 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;
+
+ ws2812b_set(&priv->leds[cur_led].mc_cdev.led_cdev,
+ default_brightness);
+
+ 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" },
+ {},
+};
+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 <[email protected]>");
+MODULE_DESCRIPTION("WS2812B LED driver using SPI");
+MODULE_LICENSE("GPL");
--
2.38.1

2022-12-02 04:37:13

by Chuanhong Guo

[permalink] [raw]

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

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

Signed-off-by: Chuanhong Guo <[email protected]>
--
Change since v1:
reword commit message

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

2022-12-02 04:49:45

by Chuanhong Guo

[permalink] [raw]

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

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

Signed-off-by: Chuanhong Guo <[email protected]>
---
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

.../bindings/leds/worldsemi,ws2812b.yaml | 138 ++++++++++++++++++
1 file changed, 138 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..f91908d0acef
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml
@@ -0,0 +1,138 @@
+# 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 <[email protected]>
+
+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
+
+ default-brightness:
+ description:
+ The default brightness that should be applied to the LED by the operating
+ system on start-up. The brightness should not exceed the brightness the
+ LED can provide.
+ $ref: /schemas/types.yaml#/definitions/uint32
+ minimum: 0
+ maximum: 255
+ default: 0
+
+ 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
+ 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.
+ maxItems: 1
+
+ required:
+ - reg
+ - color
+ - function
+
+required:
+ - compatible
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/leds/common.h>
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ leds@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>;
+ default-brightness = <30>;
+ 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

2022-12-02 11:15:08

by Krzysztof Kozlowski

[permalink] [raw]

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

On 02/12/2022 04:42, Chuanhong Guo wrote:
> Add vendor prefix for WorldSemi that makes WS2812B
> individually-addressable RGB LEDs.
>
> Signed-off-by: Chuanhong Guo <[email protected]>
> --
> Change since v1:
> reword commit message

Acked-by: Krzysztof Kozlowski <[email protected]>

Best regards,
Krzysztof

2022-12-02 11:50:49

by Krzysztof Kozlowski

[permalink] [raw]

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

On 02/12/2022 04:42, Chuanhong Guo wrote:
> This patch adds dt binding schema for WorldSemi WS2812B driven using SPI
> bus.

Do not use "This commit/patch".
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

>
> Signed-off-by: Chuanhong Guo <[email protected]>
> ---
> 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
>
> .../bindings/leds/worldsemi,ws2812b.yaml | 138 ++++++++++++++++++
> 1 file changed, 138 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..f91908d0acef
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/worldsemi,ws2812b.yaml
> @@ -0,0 +1,138 @@
> +# 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 <[email protected]>
> +
> +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
> +
> + default-brightness:
> + description:
> + The default brightness that should be applied to the LED by the operating
> + system on start-up. The brightness should not exceed the brightness the
> + LED can provide.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + minimum: 0
> + maximum: 255
> + default: 0
> +
> + 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

I am still not convinced these two properties are correct. Why this LED
is special and defines default brightness and intensity and other LEDs
do not? You explained you are doing it for user-space which is usually
not a valid reason for changes specific to one binding. Either all
bindings should support it or none.

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

git grep leds@ -- Documentation/devicetree/ | wc -l
1
git grep led@ -- Documentation/devicetree/ | wc -l
165

so rather not the first one ("leds").

There is also:
git grep led-controller@ -- Documentation/devicetree/ | wc -l
30

> + compatible = "worldsemi,ws2812b";
> + reg = <0>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + spi-max-frequency = <2850000>;
> + multi-led@0 {
> + reg = <0>;

Best regards,
Krzysztof

2022-12-02 13:23:22

by Chuanhong Guo

[permalink] [raw]

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

Hi!

On Fri, Dec 2, 2022 at 7:14 PM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 02/12/2022 04:42, Chuanhong Guo wrote:
> > This patch adds dt binding schema for WorldSemi WS2812B driven using SPI
> > bus.
>
> Do not use "This commit/patch".
> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

OK.

> [...]
> > +
> > + default-brightness:
> > + description:
> > + The default brightness that should be applied to the LED by the operating
> > + system on start-up. The brightness should not exceed the brightness the
> > + LED can provide.
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + minimum: 0
> > + maximum: 255
> > + default: 0
> > +
> > + 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
>
> I am still not convinced these two properties are correct. Why this LED
> is special and defines default brightness and intensity and other LEDs
> do not? You explained you are doing it for user-space which is usually
> not a valid reason for changes specific to one binding. Either all
> bindings should support it or none.

There's already a default-state for simple LEDs without brightness
control so I think it makes sense to add default-brightness for LEDs
with brightness control and default-intensity for colored LEDs.
The default-state seems to be implemented in various LED drivers,
so I implemented these two properties in my LED driver.
There's nothing device-specific about these two properties.

>
> > + 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.
> > + maxItems: 1
> > +
> > + required:
> > + - reg
> > + - color
> > + - function
> > +
> > +required:
> > + - compatible
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/leds/common.h>
> > + spi {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + leds@0 {
>
> git grep leds@ -- Documentation/devicetree/ | wc -l
> 1
> git grep led@ -- Documentation/devicetree/ | wc -l
> 165
>
> so rather not the first one ("leds").

As you can see, this node describes a chain of LEDs, not
a single LED, so the plural form is more appropriate than
the singular form.

>
> There is also:
> git grep led-controller@ -- Documentation/devicetree/ | wc -l
> 30

This also isn't appropriate. WS2812B is a single LED package
of 3 diodes and a microcontroller. If we treat every package
as a LED, the SPI MOSI is connected directly to the LED
packages themselves with no controller in between.
If we treat the microcontroller as a led-controller, every
LED contains its own controller, instead of one controller
controlling all LEDs, and the parent node still shouldn't
be called a led-controller.

Here's a picture of the WS2812B LED package:
https://cdn-shop.adafruit.com/970x728/1655-00.jpg
and a chain of them:
https://cdn-shop.adafruit.com/970x728/1463-00.jpg

--
Regards,
Chuanhong Guo

2022-12-03 11:23:14

by Krzysztof Kozlowski

[permalink] [raw]

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

On 02/12/2022 13:55, Chuanhong Guo wrote:
>>> +
>>> + default-brightness:
>>> + description:
>>> + The default brightness that should be applied to the LED by the operating
>>> + system on start-up. The brightness should not exceed the brightness the
>>> + LED can provide.
>>> + $ref: /schemas/types.yaml#/definitions/uint32
>>> + minimum: 0
>>> + maximum: 255
>>> + default: 0
>>> +
>>> + 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
>>
>> I am still not convinced these two properties are correct. Why this LED
>> is special and defines default brightness and intensity and other LEDs
>> do not? You explained you are doing it for user-space which is usually
>> not a valid reason for changes specific to one binding. Either all
>> bindings should support it or none.
>
> There's already a default-state for simple LEDs without brightness
> control so I think it makes sense to add default-brightness for LEDs
> with brightness control and default-intensity for colored LEDs.
> The default-state seems to be implemented in various LED drivers,
> so I implemented these two properties in my LED driver.
> There's nothing device-specific about these two properties.

default-state has a bit different purpose - to prevent any
glitches/changes when probing driver. Other values should not be
controller by default, at least not for this specific LED. If you want
to change common LED bindings and LED maintainers agree, sure, that's
different case.

Your device does not require these default brightness/intensity
properties... or at least you did not provide rationale why your device
(not driver... you keep referring to driver for some reason) needs them.

>>
>>> + 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.
>>> + maxItems: 1
>>> +
>>> + required:
>>> + - reg
>>> + - color
>>> + - function
>>> +
>>> +required:
>>> + - compatible
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> + - |
>>> + #include <dt-bindings/leds/common.h>
>>> + spi {
>>> + #address-cells = <1>;
>>> + #size-cells = <0>;
>>> +
>>> + leds@0 {
>>
>> git grep leds@ -- Documentation/devicetree/ | wc -l
>> 1
>> git grep led@ -- Documentation/devicetree/ | wc -l
>> 165
>>
>> so rather not the first one ("leds").
>
> As you can see, this node describes a chain of LEDs, not
> a single LED, so the plural form is more appropriate than
> the singular form.
>
>>
>> There is also:
>> git grep led-controller@ -- Documentation/devicetree/ | wc -l
>> 30
>
> This also isn't appropriate. WS2812B is a single LED package
> of 3 diodes and a microcontroller. If we treat every package
> as a LED, the SPI MOSI is connected directly to the LED
> packages themselves with no controller in between.
> If we treat the microcontroller as a led-controller, every
> LED contains its own controller, instead of one controller
> controlling all LEDs, and the parent node still shouldn't
> be called a led-controller.
>
> Here's a picture of the WS2812B LED package:
> https://cdn-shop.adafruit.com/970x728/1655-00.jpg
> and a chain of them:
> https://cdn-shop.adafruit.com/970x728/1463-00.jpg

Then your bindings and DTS do not represent the hardware.

Best regards,
Krzysztof

2022-12-03 12:39:39

by Chuanhong Guo

[permalink] [raw]

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

Hi!

On Sat, Dec 3, 2022 at 6:52 PM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 02/12/2022 13:55, Chuanhong Guo wrote:
> >>> +
> >>> + default-brightness:
> >>> + description:
> >>> + The default brightness that should be applied to the LED by the operating
> >>> + system on start-up. The brightness should not exceed the brightness the
> >>> + LED can provide.
> >>> + $ref: /schemas/types.yaml#/definitions/uint32
> >>> + minimum: 0
> >>> + maximum: 255
> >>> + default: 0
> >>> +
> >>> + 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
> >>
> >> I am still not convinced these two properties are correct. Why this LED
> >> is special and defines default brightness and intensity and other LEDs
> >> do not? You explained you are doing it for user-space which is usually
> >> not a valid reason for changes specific to one binding. Either all
> >> bindings should support it or none.
> >
> > There's already a default-state for simple LEDs without brightness
> > control so I think it makes sense to add default-brightness for LEDs
> > with brightness control and default-intensity for colored LEDs.
> > The default-state seems to be implemented in various LED drivers,
> > so I implemented these two properties in my LED driver.
> > There's nothing device-specific about these two properties.
>
> default-state has a bit different purpose - to prevent any
> glitches/changes when probing driver.

OK. I didn't know that property is used in this way.
I can live without them. I'll drop it in the next version.

>
> >>
> >>> + 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.
> >>> + maxItems: 1
> >>> +
> >>> + required:
> >>> + - reg
> >>> + - color
> >>> + - function
> >>> +
> >>> +required:
> >>> + - compatible
> >>> +
> >>> +additionalProperties: false
> >>> +
> >>> +examples:
> >>> + - |
> >>> + #include <dt-bindings/leds/common.h>
> >>> + spi {
> >>> + #address-cells = <1>;
> >>> + #size-cells = <0>;
> >>> +
> >>> + leds@0 {
> >>
> >> git grep leds@ -- Documentation/devicetree/ | wc -l
> >> 1
> >> git grep led@ -- Documentation/devicetree/ | wc -l
> >> 165
> >>
> >> so rather not the first one ("leds").
> >
> > As you can see, this node describes a chain of LEDs, not
> > a single LED, so the plural form is more appropriate than
> > the singular form.
> >
> >>
> >> There is also:
> >> git grep led-controller@ -- Documentation/devicetree/ | wc -l
> >> 30
> >
> > This also isn't appropriate. WS2812B is a single LED package
> > of 3 diodes and a microcontroller. If we treat every package
> > as a LED, the SPI MOSI is connected directly to the LED
> > packages themselves with no controller in between.
> > If we treat the microcontroller as a led-controller, every
> > LED contains its own controller, instead of one controller
> > controlling all LEDs, and the parent node still shouldn't
> > be called a led-controller.
> >
> > Here's a picture of the WS2812B LED package:
> > https://cdn-shop.adafruit.com/970x728/1655-00.jpg
> > and a chain of them:
> > https://cdn-shop.adafruit.com/970x728/1463-00.jpg
>
> Then your bindings and DTS do not represent the hardware.

How should this hardware be represented, then?

The connection can be:

SPI-MOSI---LED1---LED2---LED3---...---LEDN

or

SPI-MOSI---Tri-state signal gate---LED1---LED2---LED3---...---LEDN
SPI-CS-----|

--
Regards,
Chuanhong Guo

2022-12-03 13:34:46

by Krzysztof Kozlowski

[permalink] [raw]

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

On 03/12/2022 13:11, Chuanhong Guo wrote:
> Hi!
>
> On Sat, Dec 3, 2022 at 6:52 PM Krzysztof Kozlowski
> <[email protected]> wrote:
>>
>> On 02/12/2022 13:55, Chuanhong Guo wrote:
>>>>> +
>>>>> + default-brightness:
>>>>> + description:
>>>>> + The default brightness that should be applied to the LED by the operating
>>>>> + system on start-up. The brightness should not exceed the brightness the
>>>>> + LED can provide.
>>>>> + $ref: /schemas/types.yaml#/definitions/uint32
>>>>> + minimum: 0
>>>>> + maximum: 255
>>>>> + default: 0
>>>>> +
>>>>> + 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
>>>>
>>>> I am still not convinced these two properties are correct. Why this LED
>>>> is special and defines default brightness and intensity and other LEDs
>>>> do not? You explained you are doing it for user-space which is usually
>>>> not a valid reason for changes specific to one binding. Either all
>>>> bindings should support it or none.
>>>
>>> There's already a default-state for simple LEDs without brightness
>>> control so I think it makes sense to add default-brightness for LEDs
>>> with brightness control and default-intensity for colored LEDs.
>>> The default-state seems to be implemented in various LED drivers,
>>> so I implemented these two properties in my LED driver.
>>> There's nothing device-specific about these two properties.
>>
>> default-state has a bit different purpose - to prevent any
>> glitches/changes when probing driver.
>
> OK. I didn't know that property is used in this way.
> I can live without them. I'll drop it in the next version.
>
>>
>>>>
>>>>> + 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.
>>>>> + maxItems: 1
>>>>> +
>>>>> + required:
>>>>> + - reg
>>>>> + - color
>>>>> + - function
>>>>> +
>>>>> +required:
>>>>> + - compatible
>>>>> +
>>>>> +additionalProperties: false
>>>>> +
>>>>> +examples:
>>>>> + - |
>>>>> + #include <dt-bindings/leds/common.h>
>>>>> + spi {
>>>>> + #address-cells = <1>;
>>>>> + #size-cells = <0>;
>>>>> +
>>>>> + leds@0 {
>>>>
>>>> git grep leds@ -- Documentation/devicetree/ | wc -l
>>>> 1
>>>> git grep led@ -- Documentation/devicetree/ | wc -l
>>>> 165
>>>>
>>>> so rather not the first one ("leds").
>>>
>>> As you can see, this node describes a chain of LEDs, not
>>> a single LED, so the plural form is more appropriate than
>>> the singular form.
>>>
>>>>
>>>> There is also:
>>>> git grep led-controller@ -- Documentation/devicetree/ | wc -l
>>>> 30
>>>
>>> This also isn't appropriate. WS2812B is a single LED package
>>> of 3 diodes and a microcontroller. If we treat every package
>>> as a LED, the SPI MOSI is connected directly to the LED
>>> packages themselves with no controller in between.
>>> If we treat the microcontroller as a led-controller, every
>>> LED contains its own controller, instead of one controller
>>> controlling all LEDs, and the parent node still shouldn't
>>> be called a led-controller.
>>>
>>> Here's a picture of the WS2812B LED package:
>>> https://cdn-shop.adafruit.com/970x728/1655-00.jpg
>>> and a chain of them:
>>> https://cdn-shop.adafruit.com/970x728/1463-00.jpg
>>
>> Then your bindings and DTS do not represent the hardware.
>
> How should this hardware be represented, then?
>
> The connection can be:
>
> SPI-MOSI---LED1---LED2---LED3---...---LEDN
>
> or
>
> SPI-MOSI---Tri-state signal gate---LED1---LED2---LED3---...---LEDN
> SPI-CS-----|

I would look at it this as either of:
1. serially linked separate LED controllers (so multiple device nodes)

2. one LED controller handling the entire LED system of many
sub-controllers.

For the (2) I proposed the name led-controller. If you think this is not
(2), then looks like (1) and you should have many devices... which
probably is not doable on itself. And how to call this entire set of
separate LED controllers? Heh, can be also some controller, like
led-controller.

Best regards,
Krzysztof

2022-12-03 14:06:48

by Chuanhong Guo

[permalink] [raw]

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

On Sat, Dec 3, 2022 at 8:25 PM Krzysztof Kozlowski
<[email protected]> wrote:
> >>>>> + 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.
> >>>>> + maxItems: 1
> >>>>> +
> >>>>> + required:
> >>>>> + - reg
> >>>>> + - color
> >>>>> + - function
> >>>>> +
> >>>>> +required:
> >>>>> + - compatible
> >>>>> +
> >>>>> +additionalProperties: false
> >>>>> +
> >>>>> +examples:
> >>>>> + - |
> >>>>> + #include <dt-bindings/leds/common.h>
> >>>>> + spi {
> >>>>> + #address-cells = <1>;
> >>>>> + #size-cells = <0>;
> >>>>> +
> >>>>> + leds@0 {
> >>>>
> >>>> git grep leds@ -- Documentation/devicetree/ | wc -l
> >>>> 1
> >>>> git grep led@ -- Documentation/devicetree/ | wc -l
> >>>> 165
> >>>>
> >>>> so rather not the first one ("leds").
> >>>
> >>> As you can see, this node describes a chain of LEDs, not
> >>> a single LED, so the plural form is more appropriate than
> >>> the singular form.
> >>>
> >>>>
> >>>> There is also:
> >>>> git grep led-controller@ -- Documentation/devicetree/ | wc -l
> >>>> 30
> >>>
> >>> This also isn't appropriate. WS2812B is a single LED package
> >>> of 3 diodes and a microcontroller. If we treat every package
> >>> as a LED, the SPI MOSI is connected directly to the LED
> >>> packages themselves with no controller in between.
> >>> If we treat the microcontroller as a led-controller, every
> >>> LED contains its own controller, instead of one controller
> >>> controlling all LEDs, and the parent node still shouldn't
> >>> be called a led-controller.
> >>>
> >>> Here's a picture of the WS2812B LED package:
> >>> https://cdn-shop.adafruit.com/970x728/1655-00.jpg
> >>> and a chain of them:
> >>> https://cdn-shop.adafruit.com/970x728/1463-00.jpg
> >>
> >> Then your bindings and DTS do not represent the hardware.
> >
> > How should this hardware be represented, then?
> >
> > The connection can be:
> >
> > SPI-MOSI---LED1---LED2---LED3---...---LEDN
> >
> > or
> >
> > SPI-MOSI---Tri-state signal gate---LED1---LED2---LED3---...---LEDN
> > SPI-CS-----|
>
> I would look at it this as either of:
> 1. serially linked separate LED controllers (so multiple device nodes)
>
> 2. one LED controller handling the entire LED system of many
> sub-controllers.
>
> For the (2) I proposed the name led-controller. If you think this is not
> (2), then looks like (1) and you should have many devices... which
> probably is not doable on itself. And how to call this entire set of
> separate LED controllers? Heh, can be also some controller, like
> led-controller.

OK. I'll name it led-controller in the next version.

--
Regards,
Chuanhong Guo