All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/3] dt-bindings: leds: Add bindings for the TLC5925 controller
       [not found] <20220523084958.2723943-1-jjhiblot@traphandler.com>
@ 2022-05-23  8:49 ` Jean-Jacques Hiblot
  2022-05-23 10:14   ` Krzysztof Kozlowski
                     ` (2 more replies)
  2022-05-23  8:49 ` [PATCH 2/3] leds: Add driver for the TLC5925 LED controller Jean-Jacques Hiblot
  2022-05-23  8:49 ` [PATCH 3/3] leds: tlc5925: Add support for non blocking operations Jean-Jacques Hiblot
  2 siblings, 3 replies; 11+ messages in thread
From: Jean-Jacques Hiblot @ 2022-05-23  8:49 UTC (permalink / raw)
  To: Pavel Machek, Rob Herring, Krzysztof Kozlowski, Jean-Jacques Hiblot
  Cc: linux-leds, devicetree, linux-kernel

Add bindings documentation for the TLC5925 LED controller.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
---
devicetree@vger.kernel.org
 .../bindings/leds/leds-tlc5925.yaml           | 100 ++++++++++++++++++
 1 file changed, 100 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-tlc5925.yaml

diff --git a/Documentation/devicetree/bindings/leds/leds-tlc5925.yaml b/Documentation/devicetree/bindings/leds/leds-tlc5925.yaml
new file mode 100644
index 000000000000..156db599d5a1
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-tlc5925.yaml
@@ -0,0 +1,100 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/leds-tlc5925.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: LEDs connected to TI TLC5925 controller
+
+maintainers:
+  - Jean-Jacques Hiblot <jjhiblot@traphandler.com>
+
+description: |
+  The TLC5925 is a low-power 16-channel constant-current LED sink driver.
+  It is controlled through a SPI interface.
+  It is built around a shift register and latches which convert serial
+  input data into a parallel output. Several TLC5925 can be chained to
+  control more than 16 LEDs with a single chip-select.
+  The brightness level cannot be controlled, each LED is either on or off.
+
+  Each LED is represented as a sub-node of the ti,tlc5925 device.
+
+properties:
+  compatible:
+    const: ti,tlc5925
+
+  shift_register_length:
+    maxItems: 1
+    description: |
+      The length of the shift register. If several TLC5925 are chained,
+      shift_register_length should be set to 16 times the number of TLC5925.
+      The value must be a multiple of 8.
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+  output-enable-b-gpios:
+    description: |
+      GPIO pins to enable/disable the parallel output. They describe the GPIOs
+      connected to the OE/ pin of the TLC5925s.
+
+patternProperties:
+  "@[a-f0-9]+$":
+    type: object
+
+    $ref: common.yaml#
+
+    properties:
+      reg:
+        items:
+        description: |
+          LED pin number (must be lower than shift_register_length).
+          The furthest LED down the chain has the pin number 0.
+
+    required:
+      - reg
+
+required:
+  - reg
+  - "#address-cells"
+  - "#size-cells"
+  - shift_register_length
+
+examples:
+  - |
+    &spi0 {
+        leds@2 {
+                compatible = "ti,tlc5925";
+                reg = <0x02>;
+                spi-max-frequency = <30000000>;
+                shift_register_length = <32>;
+                output-enable-b-gpios = <&gpio0b 9 GPIO_ACTIVE_HIGH>, <&gpio0b 7 GPIO_ACTIVE_HIGH>;
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                led-satus@0 {
+                        reg = <0>;
+                        function = LED_FUNCTION_STATUS;
+                        color = <LED_COLOR_ID_GREEN>;
+                };
+
+                led-satus@4 {
+                        reg = <4>;
+                        function = LED_FUNCTION_STATUS;
+                        color = <LED_COLOR_ID_RED>;
+                };
+
+                led-alive@24 {
+                        reg = <24>;
+                        label = "green:alive"
+                };
+
+                led-panic@31 {
+                        reg = <31>;
+                        label = "red:panic"
+                };
+        };
+    };
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/3] leds: Add driver for the TLC5925 LED controller
       [not found] <20220523084958.2723943-1-jjhiblot@traphandler.com>
  2022-05-23  8:49 ` [PATCH 1/3] dt-bindings: leds: Add bindings for the TLC5925 controller Jean-Jacques Hiblot
@ 2022-05-23  8:49 ` Jean-Jacques Hiblot
  2022-05-23  8:49 ` [PATCH 3/3] leds: tlc5925: Add support for non blocking operations Jean-Jacques Hiblot
  2 siblings, 0 replies; 11+ messages in thread
From: Jean-Jacques Hiblot @ 2022-05-23  8:49 UTC (permalink / raw)
  To: Pavel Machek; +Cc: Jean-Jacques Hiblot, linux-kernel, linux-leds

The TLC5925 is a 16-channels constant-current LED sink driver.
It is controlled via SPI but doesn't offer a register-based interface.
Instead it contains a shift register and latches that convert the
serial input into a parallel output.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
---
 drivers/leds/Kconfig        |   6 ++
 drivers/leds/Makefile       |   1 +
 drivers/leds/leds-tlc5925.c | 157 ++++++++++++++++++++++++++++++++++++
 3 files changed, 164 insertions(+)
 create mode 100644 drivers/leds/leds-tlc5925.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index a49979f41eee..b17eb01210ba 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -658,6 +658,12 @@ config LEDS_TLC591XX
 	  This option enables support for Texas Instruments TLC59108
 	  and TLC59116 LED controllers.
 
+config LEDS_TLC5925
+	tristate "LED driver for TLC5925 controller"
+	depends on LEDS_CLASS && SPI
+	help
+	  This option enables support for Texas Instruments TLC5925.
+
 config LEDS_MAX77650
 	tristate "LED support for Maxim MAX77650 PMIC"
 	depends on LEDS_CLASS && MFD_MAX77650
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 4fd2f92cd198..9d15b88d482f 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -81,6 +81,7 @@ obj-$(CONFIG_LEDS_SYSCON)		+= leds-syscon.o
 obj-$(CONFIG_LEDS_TCA6507)		+= leds-tca6507.o
 obj-$(CONFIG_LEDS_TI_LMU_COMMON)	+= leds-ti-lmu-common.o
 obj-$(CONFIG_LEDS_TLC591XX)		+= leds-tlc591xx.o
+obj-$(CONFIG_LEDS_TLC5925)		+= leds-tlc5925.o
 obj-$(CONFIG_LEDS_TPS6105X)		+= leds-tps6105x.o
 obj-$(CONFIG_LEDS_TURRIS_OMNIA)		+= leds-turris-omnia.o
 obj-$(CONFIG_LEDS_WM831X_STATUS)	+= leds-wm831x-status.o
diff --git a/drivers/leds/leds-tlc5925.c b/drivers/leds/leds-tlc5925.c
new file mode 100644
index 000000000000..f33eff8b7333
--- /dev/null
+++ b/drivers/leds/leds-tlc5925.c
@@ -0,0 +1,157 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * The driver supports controllers with a very simple SPI protocol:
+ * - the data is deserialized in a shift-register when CS is asserted
+ * - the data is latched when CS is de-asserted
+ * - the LED are either on or off (no control of the brightness)
+ *
+ * Supported devices:
+ * - "ti,tlc5925":  Low-Power 16-Channel Constant-Current LED Sink Driver
+ *                  https://www.ti.com/lit/ds/symlink/tlc5925.pdf
+ */
+
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/leds.h>
+#include <linux/err.h>
+#include <linux/spi/spi.h>
+#include <linux/property.h>
+#include <linux/workqueue.h>
+
+struct single_led_priv {
+	int idx;
+	struct led_classdev cdev;
+};
+
+struct tlc5925_leds_priv {
+	int max_num_leds;
+	u8 *state;
+	spinlock_t lock;
+	struct single_led_priv leds[];
+};
+
+static int tlc5925_brightness_set_blocking(struct led_classdev *cdev,
+					    enum led_brightness brightness)
+{
+	struct spi_device *spi = to_spi_device(cdev->dev->parent);
+	struct tlc5925_leds_priv *priv = spi_get_drvdata(spi);
+	struct single_led_priv *led = container_of(cdev,
+						   struct single_led_priv,
+						   cdev);
+	int index = led->idx;
+
+	spin_lock(&priv->lock);
+	if (brightness)
+		priv->state[index / 8] |= (1 << (index % 8));
+	else
+		priv->state[index / 8] &= ~(1 << (index % 8));
+	spin_unlock(&priv->lock);
+
+	return spi_write(spi, priv->state, priv->max_num_leds / 8);
+}
+
+
+static int tlc5925_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct fwnode_handle *child;
+	struct tlc5925_leds_priv *priv;
+	int ret;
+	int max_num_leds, count;
+	struct gpio_descs *gpios;
+
+	count = device_get_child_node_count(dev);
+	if (!count) {
+		dev_err(dev, "no led defined.\n");
+		return -ENODEV;
+	}
+
+	ret = device_property_read_u32_array(dev, "shift_register_length",
+					     &max_num_leds, 1);
+	if (ret) {
+		dev_err(dev, "'shift_register_length' property is required.\n");
+		return -EINVAL;
+	}
+
+	if (max_num_leds % 8) {
+		dev_err(dev, "'shift_register_length' must be a multiple of 8\n");
+		return -EINVAL;
+	}
+
+	if (max_num_leds == 0) {
+		dev_err(dev, "'shift_register_length' must be greater than 0\n");
+		return -EINVAL;
+	}
+
+	/* Assert all the OE/ lines */
+	gpios = devm_gpiod_get_array(dev, "output-enable-b", GPIOD_OUT_LOW);
+	if (IS_ERR(gpios)) {
+		dev_err(dev, "Unable to get the 'output-enable-b' gpios\n");
+		return PTR_ERR(gpios);
+	}
+
+	priv = devm_kzalloc(dev, struct_size(priv, leds, count), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	spin_lock_init(&priv->lock);
+
+	priv->state = devm_kzalloc(dev, DIV_ROUND_UP(max_num_leds, 8), GFP_KERNEL);
+	if (!priv->state)
+		return -ENOMEM;
+
+	priv->max_num_leds = max_num_leds;
+
+	device_for_each_child_node(dev, child) {
+		struct led_init_data init_data = {.fwnode = child};
+		struct led_classdev *cdev;
+		u32 idx;
+
+		ret = fwnode_property_read_u32_array(child, "reg", &idx, 1);
+		if (ret || idx >= max_num_leds) {
+			dev_err(dev, "%s: invalid reg value. Ignoring.\n",
+				fwnode_get_name(child));
+			fwnode_handle_put(child);
+			continue;
+		}
+
+		count--;
+		priv->leds[count].idx = idx;
+		cdev = &(priv->leds[count].cdev);
+		cdev->brightness = LED_OFF;
+		cdev->max_brightness = 1;
+		cdev->brightness_set_blocking = tlc5925_brightness_set_blocking;
+
+		ret = devm_led_classdev_register_ext(dev, cdev, &init_data);
+		if (ret) {
+			dev_err(dev, "%s: cannot create LED device.\n",
+				fwnode_get_name(child));
+			fwnode_handle_put(child);
+			continue;
+		}
+	}
+
+	spi_set_drvdata(spi, priv);
+
+	return 0;
+}
+
+static const struct of_device_id tlc5925_dt_ids[] = {
+	{ .compatible = "ti,tlc5925", },
+	{},
+};
+
+static struct spi_driver tlc5925_driver = {
+	.probe = tlc5925_probe,
+	.driver = {
+		.name		= KBUILD_MODNAME,
+		.of_match_table	= tlc5925_dt_ids,
+	},
+};
+
+module_spi_driver(tlc5925_driver);
+
+MODULE_AUTHOR("Jean-Jacques Hiblot <jjhiblot@traphandler.com>");
+MODULE_DESCRIPTION("TLC5925 LED driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("spi:tlc5925");
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 3/3] leds: tlc5925: Add support for non blocking operations
       [not found] <20220523084958.2723943-1-jjhiblot@traphandler.com>
  2022-05-23  8:49 ` [PATCH 1/3] dt-bindings: leds: Add bindings for the TLC5925 controller Jean-Jacques Hiblot
  2022-05-23  8:49 ` [PATCH 2/3] leds: Add driver for the TLC5925 LED controller Jean-Jacques Hiblot
@ 2022-05-23  8:49 ` Jean-Jacques Hiblot
  2 siblings, 0 replies; 11+ messages in thread
From: Jean-Jacques Hiblot @ 2022-05-23  8:49 UTC (permalink / raw)
  To: Pavel Machek; +Cc: Jean-Jacques Hiblot, linux-leds, linux-kernel

Settings multiple LEDs in a row can be a slow operation because of the
time required to acquire the bus and prepare the transfer.
And, in most cases, it is not required that the operation is synchronous.

Implementing the non-blocking brightness_set() for such cases.
A work queue is used to perform the actual SPI transfer.

The blocking method is still available in case someone needs to perform
this operation synchronously (ie by calling led_set_brightness_sync()).


Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
---
 drivers/leds/leds-tlc5925.c | 76 +++++++++++++++++++++++++++++++++----
 1 file changed, 68 insertions(+), 8 deletions(-)

diff --git a/drivers/leds/leds-tlc5925.c b/drivers/leds/leds-tlc5925.c
index f33eff8b7333..d7dd572eca69 100644
--- a/drivers/leds/leds-tlc5925.c
+++ b/drivers/leds/leds-tlc5925.c
@@ -18,6 +18,8 @@
 #include <linux/property.h>
 #include <linux/workqueue.h>
 
+#define BITS_PER_ATOMIC (sizeof(atomic_t) * 8)
+
 struct single_led_priv {
 	int idx;
 	struct led_classdev cdev;
@@ -25,12 +27,35 @@ struct single_led_priv {
 
 struct tlc5925_leds_priv {
 	int max_num_leds;
-	u8 *state;
+	int max_state;
+	atomic_t *state;
+	int *spi_buffer;
 	spinlock_t lock;
+	struct spi_device *spi;
+	struct work_struct xmit_work;
 	struct single_led_priv leds[];
 };
 
-static int tlc5925_brightness_set_blocking(struct led_classdev *cdev,
+static int xmit(struct tlc5925_leds_priv *priv)
+{
+	int i;
+
+	spin_lock(&priv->lock);
+	for (i = 0; i < priv->max_state / (sizeof(atomic_t) * 8) ; i++)
+		priv->spi_buffer[i] = atomic_read(&priv->state[i]);
+	spin_unlock(&priv->lock);
+
+	return spi_write(priv->spi, priv->spi_buffer, priv->max_num_leds / 8);
+}
+
+static void xmit_work(struct work_struct *ws)
+{
+	struct tlc5925_leds_priv *priv =
+		container_of(ws, struct tlc5925_leds_priv, xmit_work);
+	xmit(priv);
+};
+
+static void tlc5925_brightness_set(struct led_classdev *cdev,
 					    enum led_brightness brightness)
 {
 	struct spi_device *spi = to_spi_device(cdev->dev->parent);
@@ -40,16 +65,36 @@ static int tlc5925_brightness_set_blocking(struct led_classdev *cdev,
 						   cdev);
 	int index = led->idx;
 
-	spin_lock(&priv->lock);
 	if (brightness)
-		priv->state[index / 8] |= (1 << (index % 8));
+		atomic_or(1 << (index % BITS_PER_ATOMIC),
+			  &priv->state[index / BITS_PER_ATOMIC]);
 	else
-		priv->state[index / 8] &= ~(1 << (index % 8));
-	spin_unlock(&priv->lock);
+		atomic_and(~(1 << (index % BITS_PER_ATOMIC)),
+			   &priv->state[index / BITS_PER_ATOMIC]);
 
-	return spi_write(spi, priv->state, priv->max_num_leds / 8);
+	schedule_work(&priv->xmit_work);
 }
 
+static int tlc5925_brightness_set_blocking(struct led_classdev *cdev,
+					    enum led_brightness brightness)
+{
+	struct spi_device *spi = to_spi_device(cdev->dev->parent);
+	struct tlc5925_leds_priv *priv = spi_get_drvdata(spi);
+	struct single_led_priv *led = container_of(cdev,
+						   struct single_led_priv,
+						   cdev);
+	int index = led->idx;
+
+	if (brightness)
+		atomic_or(1 << (index % BITS_PER_ATOMIC),
+			  &priv->state[index / BITS_PER_ATOMIC]);
+	else
+		atomic_and(~(1 << (index % BITS_PER_ATOMIC)),
+			   &priv->state[index / BITS_PER_ATOMIC]);
+
+	cancel_work_sync(&priv->xmit_work);
+	return xmit(priv);
+}
 
 static int tlc5925_probe(struct spi_device *spi)
 {
@@ -96,10 +141,24 @@ static int tlc5925_probe(struct spi_device *spi)
 
 	spin_lock_init(&priv->lock);
 
-	priv->state = devm_kzalloc(dev, DIV_ROUND_UP(max_num_leds, 8), GFP_KERNEL);
+	priv->spi = spi;
+	INIT_WORK(&priv->xmit_work, xmit_work);
+
+	// Allocate the buffer used to hold the state of each LED
+	priv->max_state = round_up(max_num_leds, BITS_PER_ATOMIC);
+	priv->state = devm_kzalloc(dev,
+				   priv->max_state / 8,
+				   GFP_KERNEL);
 	if (!priv->state)
 		return -ENOMEM;
 
+	// Allocate a second buffer for the communication on the SPI bus
+	priv->spi_buffer = devm_kzalloc(dev,
+				   priv->max_state / 8,
+				   GFP_KERNEL);
+	if (!priv->spi_buffer)
+		return -ENOMEM;
+
 	priv->max_num_leds = max_num_leds;
 
 	device_for_each_child_node(dev, child) {
@@ -120,6 +179,7 @@ static int tlc5925_probe(struct spi_device *spi)
 		cdev = &(priv->leds[count].cdev);
 		cdev->brightness = LED_OFF;
 		cdev->max_brightness = 1;
+		cdev->brightness_set = tlc5925_brightness_set;
 		cdev->brightness_set_blocking = tlc5925_brightness_set_blocking;
 
 		ret = devm_led_classdev_register_ext(dev, cdev, &init_data);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] dt-bindings: leds: Add bindings for the TLC5925 controller
  2022-05-23  8:49 ` [PATCH 1/3] dt-bindings: leds: Add bindings for the TLC5925 controller Jean-Jacques Hiblot
@ 2022-05-23 10:14   ` Krzysztof Kozlowski
  2022-05-23 13:07     ` Jean-Jacques Hiblot
  2022-05-23 15:16     ` Jean-Jacques Hiblot
  2022-05-23 12:33   ` Rob Herring
  2022-05-23 20:05   ` Pavel Machek
  2 siblings, 2 replies; 11+ messages in thread
From: Krzysztof Kozlowski @ 2022-05-23 10:14 UTC (permalink / raw)
  To: Jean-Jacques Hiblot, Pavel Machek, Rob Herring, Krzysztof Kozlowski
  Cc: linux-leds, devicetree, linux-kernel

On 23/05/2022 10:49, Jean-Jacques Hiblot wrote:
> Add bindings documentation for the TLC5925 LED controller.
> 
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>

Thank you for your patch. There is something to discuss/improve.

> ---
> devicetree@vger.kernel.org
>  .../bindings/leds/leds-tlc5925.yaml           | 100 ++++++++++++++++++
>  1 file changed, 100 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-tlc5925.yaml
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-tlc5925.yaml b/Documentation/devicetree/bindings/leds/leds-tlc5925.yaml
> new file mode 100644
> index 000000000000..156db599d5a1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-tlc5925.yaml

Filename: vendor,device
so "ti,tlc5925-leds.yaml" for example.



> @@ -0,0 +1,100 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/leds-tlc5925.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: LEDs connected to TI TLC5925 controller
> +
> +maintainers:
> +  - Jean-Jacques Hiblot <jjhiblot@traphandler.com>
> +
> +description: |
> +  The TLC5925 is a low-power 16-channel constant-current LED sink driver.
> +  It is controlled through a SPI interface.
> +  It is built around a shift register and latches which convert serial
> +  input data into a parallel output. Several TLC5925 can be chained to
> +  control more than 16 LEDs with a single chip-select.
> +  The brightness level cannot be controlled, each LED is either on or off.
> +
> +  Each LED is represented as a sub-node of the ti,tlc5925 device.
> +
> +properties:
> +  compatible:
> +    const: ti,tlc5925
> +
> +  shift_register_length:
> +    maxItems: 1

No...
1. Did you test your bindings with dt_binding_check? This fails
obviously... please, do not send untested bindings.

2. vendor prefix, no underscores, proper type, maxItems look wrong here

> +    description: |
> +      The length of the shift register. If several TLC5925 are chained,
> +      shift_register_length should be set to 16 times the number of TLC5925.
> +      The value must be a multiple of 8.
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +  output-enable-b-gpios:
> +    description: |
> +      GPIO pins to enable/disable the parallel output. They describe the GPIOs
> +      connected to the OE/ pin of the TLC5925s.

maxItems


> +
> +patternProperties:
> +  "@[a-f0-9]+$":

How many LEDs you can have here? Usually it is limited, so the pattern
should be narrowed.

> +    type: object
> +
> +    $ref: common.yaml#
> +
> +    properties:
> +      reg:
> +        items:

Not correct syntax... I will stop reviewing. There is no point to use
reviewers time to do the job of a tool.


> +examples:
> +  - |
> +    &spi0 {
> +        leds@2 {
> +                compatible = "ti,tlc5925";

Messed up indentation. 4 spaces for DTS example.

> +                reg = <0x02>;
> +                spi-max-frequency = <30000000>;
> +                shift_register_length = <32>;
> +                output-enable-b-gpios = <&gpio0b 9 GPIO_ACTIVE_HIGH>, <&gpio0b 7 GPIO_ACTIVE_HIGH>;
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +
> +                led-satus@0 {
> +                        reg = <0>;
> +                        function = LED_FUNCTION_STATUS;
> +                        color = <LED_COLOR_ID_GREEN>;
> +                };
> +
> +                led-satus@4 {
> +                        reg = <4>;
> +                        function = LED_FUNCTION_STATUS;
> +                        color = <LED_COLOR_ID_RED>;
> +                };
> +
> +                led-alive@24 {
> +                        reg = <24>;
> +                        label = "green:alive"
> +                };
> +
> +                led-panic@31 {
> +                        reg = <31>;
> +                        label = "red:panic"
> +                };
> +        };
> +    };


Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] dt-bindings: leds: Add bindings for the TLC5925 controller
  2022-05-23  8:49 ` [PATCH 1/3] dt-bindings: leds: Add bindings for the TLC5925 controller Jean-Jacques Hiblot
  2022-05-23 10:14   ` Krzysztof Kozlowski
@ 2022-05-23 12:33   ` Rob Herring
  2022-05-23 20:05   ` Pavel Machek
  2 siblings, 0 replies; 11+ messages in thread
From: Rob Herring @ 2022-05-23 12:33 UTC (permalink / raw)
  To: Jean-Jacques Hiblot
  Cc: Krzysztof Kozlowski, linux-leds, Rob Herring, devicetree,
	Pavel Machek, linux-kernel

On Mon, 23 May 2022 10:49:55 +0200, Jean-Jacques Hiblot wrote:
> Add bindings documentation for the TLC5925 LED controller.
> 
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
> ---
> devicetree@vger.kernel.org
>  .../bindings/leds/leds-tlc5925.yaml           | 100 ++++++++++++++++++
>  1 file changed, 100 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-tlc5925.yaml
> 

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

yamllint warnings/errors:
./Documentation/devicetree/bindings/leds/leds-tlc5925.yaml:52:15: [error] empty value in block mapping (empty-values)

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/leds/leds-tlc5925.yaml: patternProperties:@[a-f0-9]+$:properties:reg:items: 'anyOf' conditional failed, one must be fixed:
	None is not of type 'object', 'boolean'
	None is not of type 'array'
	from schema $id: http://json-schema.org/draft-07/schema#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/leds/leds-tlc5925.yaml: patternProperties:@[a-f0-9]+$:properties:reg:items: 'oneOf' conditional failed, one must be fixed:
	None is not of type 'object'
	None is not of type 'array'
	from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/leds/leds-tlc5925.yaml: 'oneOf' conditional failed, one must be fixed:
	'unevaluatedProperties' is a required property
	'additionalProperties' is a required property
	hint: Either unevaluatedProperties or additionalProperties must be present
	from schema $id: http://devicetree.org/meta-schemas/core.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/leds/leds-tlc5925.yaml: patternProperties:@[a-f0-9]+$:properties:reg: 'anyOf' conditional failed, one must be fixed:
	'maxItems' is a required property
		hint: Only "maxItems" is required for a single entry if there are no constraints defined for the values.
	'items' is not one of ['maxItems', 'description', 'deprecated']
		hint: Only "maxItems" is required for a single entry if there are no constraints defined for the values.
	'items' is not one of ['description', 'deprecated', 'const', 'enum', 'minimum', 'maximum', 'multipleOf', 'default', '$ref', 'oneOf']
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/leds/leds-tlc5925.yaml: patternProperties:@[a-f0-9]+$:properties:reg:items: 'oneOf' conditional failed, one must be fixed:
		None is not of type 'object'
		None is not of type 'array'
		hint: "items" can be a list defining each entry or a schema applying to all items. A list has an implicit size. A schema requires minItems/maxItems to define the size.
		from schema $id: http://devicetree.org/meta-schemas/core.yaml#
	hint: cell array properties must define how many entries and what the entries are when there is more than one entry.
	from schema $id: http://devicetree.org/meta-schemas/core.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/leds/leds-tlc5925.yaml: ignoring, error in schema: patternProperties: @[a-f0-9]+$: properties: reg: items
Error: Documentation/devicetree/bindings/leds/leds-tlc5925.example.dts:18.9-14 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:364: Documentation/devicetree/bindings/leds/leds-tlc5925.example.dtb] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1401: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

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

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

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

pip3 install dtschema --upgrade

Please check and re-submit.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] dt-bindings: leds: Add bindings for the TLC5925 controller
  2022-05-23 10:14   ` Krzysztof Kozlowski
@ 2022-05-23 13:07     ` Jean-Jacques Hiblot
  2022-05-23 15:16     ` Jean-Jacques Hiblot
  1 sibling, 0 replies; 11+ messages in thread
From: Jean-Jacques Hiblot @ 2022-05-23 13:07 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Pavel Machek, Rob Herring, Krzysztof Kozlowski
  Cc: linux-leds, devicetree, linux-kernel


On 23/05/2022 12:14, Krzysztof Kozlowski wrote:
> On 23/05/2022 10:49, Jean-Jacques Hiblot wrote:
>> Add bindings documentation for the TLC5925 LED controller.
>>
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
> Thank you for your patch. There is something to discuss/improve.

Thanks for the review. I didn't know about dt_binding_check.

I'll make sure it passes next time.

JJ

>> ---
>> devicetree@vger.kernel.org
>>   .../bindings/leds/leds-tlc5925.yaml           | 100 ++++++++++++++++++
>>   1 file changed, 100 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/leds/leds-tlc5925.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/leds/leds-tlc5925.yaml b/Documentation/devicetree/bindings/leds/leds-tlc5925.yaml
>> new file mode 100644
>> index 000000000000..156db599d5a1
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/leds-tlc5925.yaml
> Filename: vendor,device
> so "ti,tlc5925-leds.yaml" for example.
>
>
>
>> @@ -0,0 +1,100 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/leds/leds-tlc5925.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: LEDs connected to TI TLC5925 controller
>> +
>> +maintainers:
>> +  - Jean-Jacques Hiblot <jjhiblot@traphandler.com>
>> +
>> +description: |
>> +  The TLC5925 is a low-power 16-channel constant-current LED sink driver.
>> +  It is controlled through a SPI interface.
>> +  It is built around a shift register and latches which convert serial
>> +  input data into a parallel output. Several TLC5925 can be chained to
>> +  control more than 16 LEDs with a single chip-select.
>> +  The brightness level cannot be controlled, each LED is either on or off.
>> +
>> +  Each LED is represented as a sub-node of the ti,tlc5925 device.
>> +
>> +properties:
>> +  compatible:
>> +    const: ti,tlc5925
>> +
>> +  shift_register_length:
>> +    maxItems: 1
> No...
> 1. Did you test your bindings with dt_binding_check? This fails
> obviously... please, do not send untested bindings.
>
> 2. vendor prefix, no underscores, proper type, maxItems look wrong here
>
>> +    description: |
>> +      The length of the shift register. If several TLC5925 are chained,
>> +      shift_register_length should be set to 16 times the number of TLC5925.
>> +      The value must be a multiple of 8.
>> +
>> +  "#address-cells":
>> +    const: 1
>> +
>> +  "#size-cells":
>> +    const: 0
>> +
>> +  output-enable-b-gpios:
>> +    description: |
>> +      GPIO pins to enable/disable the parallel output. They describe the GPIOs
>> +      connected to the OE/ pin of the TLC5925s.
> maxItems
>
>
>> +
>> +patternProperties:
>> +  "@[a-f0-9]+$":
> How many LEDs you can have here? Usually it is limited, so the pattern
> should be narrowed.
>
>> +    type: object
>> +
>> +    $ref: common.yaml#
>> +
>> +    properties:
>> +      reg:
>> +        items:
> Not correct syntax... I will stop reviewing. There is no point to use
> reviewers time to do the job of a tool.
>
>
>> +examples:
>> +  - |
>> +    &spi0 {
>> +        leds@2 {
>> +                compatible = "ti,tlc5925";
> Messed up indentation. 4 spaces for DTS example.
>
>> +                reg = <0x02>;
>> +                spi-max-frequency = <30000000>;
>> +                shift_register_length = <32>;
>> +                output-enable-b-gpios = <&gpio0b 9 GPIO_ACTIVE_HIGH>, <&gpio0b 7 GPIO_ACTIVE_HIGH>;
>> +                #address-cells = <1>;
>> +                #size-cells = <0>;
>> +
>> +                led-satus@0 {
>> +                        reg = <0>;
>> +                        function = LED_FUNCTION_STATUS;
>> +                        color = <LED_COLOR_ID_GREEN>;
>> +                };
>> +
>> +                led-satus@4 {
>> +                        reg = <4>;
>> +                        function = LED_FUNCTION_STATUS;
>> +                        color = <LED_COLOR_ID_RED>;
>> +                };
>> +
>> +                led-alive@24 {
>> +                        reg = <24>;
>> +                        label = "green:alive"
>> +                };
>> +
>> +                led-panic@31 {
>> +                        reg = <31>;
>> +                        label = "red:panic"
>> +                };
>> +        };
>> +    };
>
> Best regards,
> Krzysztof

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] dt-bindings: leds: Add bindings for the TLC5925 controller
  2022-05-23 10:14   ` Krzysztof Kozlowski
  2022-05-23 13:07     ` Jean-Jacques Hiblot
@ 2022-05-23 15:16     ` Jean-Jacques Hiblot
  2022-05-23 15:30       ` Krzysztof Kozlowski
  1 sibling, 1 reply; 11+ messages in thread
From: Jean-Jacques Hiblot @ 2022-05-23 15:16 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Pavel Machek, Rob Herring, Krzysztof Kozlowski
  Cc: linux-leds, devicetree, linux-kernel


On 23/05/2022 12:14, Krzysztof Kozlowski wrote:
> On 23/05/2022 10:49, Jean-Jacques Hiblot wrote:
>> Add bindings documentation for the TLC5925 LED controller.
>>
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
> Thank you for your patch. There is something to discuss/improve.
>
>> ---
>> devicetree@vger.kernel.org
>>   .../bindings/leds/leds-tlc5925.yaml           | 100 ++++++++++++++++++
>>   1 file changed, 100 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/leds/leds-tlc5925.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/leds/leds-tlc5925.yaml b/Documentation/devicetree/bindings/leds/leds-tlc5925.yaml
>> new file mode 100644
>> index 000000000000..156db599d5a1
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/leds-tlc5925.yaml
> Filename: vendor,device
> so "ti,tlc5925-leds.yaml" for example.
>
>
>
>> @@ -0,0 +1,100 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/leds/leds-tlc5925.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: LEDs connected to TI TLC5925 controller
>> +
>> +maintainers:
>> +  - Jean-Jacques Hiblot <jjhiblot@traphandler.com>
>> +
>> +description: |
>> +  The TLC5925 is a low-power 16-channel constant-current LED sink driver.
>> +  It is controlled through a SPI interface.
>> +  It is built around a shift register and latches which convert serial
>> +  input data into a parallel output. Several TLC5925 can be chained to
>> +  control more than 16 LEDs with a single chip-select.
>> +  The brightness level cannot be controlled, each LED is either on or off.
>> +
>> +  Each LED is represented as a sub-node of the ti,tlc5925 device.
>> +
>> +properties:
>> +  compatible:
>> +    const: ti,tlc5925
>> +
>> +  shift_register_length:
>> +    maxItems: 1
> No...
> 1. Did you test your bindings with dt_binding_check? This fails
> obviously... please, do not send untested bindings.
>
> 2. vendor prefix, no underscores, proper type, maxItems look wrong here
>
>> +    description: |
>> +      The length of the shift register. If several TLC5925 are chained,
>> +      shift_register_length should be set to 16 times the number of TLC5925.
>> +      The value must be a multiple of 8.
>> +
>> +  "#address-cells":
>> +    const: 1
>> +
>> +  "#size-cells":
>> +    const: 0
>> +
>> +  output-enable-b-gpios:
>> +    description: |
>> +      GPIO pins to enable/disable the parallel output. They describe the GPIOs
>> +      connected to the OE/ pin of the TLC5925s.
> maxItems

There is no limitation in the driver itself. The actual number of items 
here really depends on the number of chips and how they are wired.

>
>
>> +
>> +patternProperties:
>> +  "@[a-f0-9]+$":
> How many LEDs you can have here? Usually it is limited, so the pattern
> should be narrowed.

There is no limitation here either. The chips can be chained to augment 
the number of LEDs.

The max number of LED is equal to the length of the shift-register.


Jean-Jacques

>
>> +    type: object
>> +
>> +    $ref: common.yaml#
>> +
>> +    properties:
>> +      reg:
>> +        items:
> Not correct syntax... I will stop reviewing. There is no point to use
> reviewers time to do the job of a tool.
>
>
>> +examples:
>> +  - |
>> +    &spi0 {
>> +        leds@2 {
>> +                compatible = "ti,tlc5925";
> Messed up indentation. 4 spaces for DTS example.
>
>> +                reg = <0x02>;
>> +                spi-max-frequency = <30000000>;
>> +                shift_register_length = <32>;
>> +                output-enable-b-gpios = <&gpio0b 9 GPIO_ACTIVE_HIGH>, <&gpio0b 7 GPIO_ACTIVE_HIGH>;
>> +                #address-cells = <1>;
>> +                #size-cells = <0>;
>> +
>> +                led-satus@0 {
>> +                        reg = <0>;
>> +                        function = LED_FUNCTION_STATUS;
>> +                        color = <LED_COLOR_ID_GREEN>;
>> +                };
>> +
>> +                led-satus@4 {
>> +                        reg = <4>;
>> +                        function = LED_FUNCTION_STATUS;
>> +                        color = <LED_COLOR_ID_RED>;
>> +                };
>> +
>> +                led-alive@24 {
>> +                        reg = <24>;
>> +                        label = "green:alive"
>> +                };
>> +
>> +                led-panic@31 {
>> +                        reg = <31>;
>> +                        label = "red:panic"
>> +                };
>> +        };
>> +    };
>
> Best regards,
> Krzysztof

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] dt-bindings: leds: Add bindings for the TLC5925 controller
  2022-05-23 15:16     ` Jean-Jacques Hiblot
@ 2022-05-23 15:30       ` Krzysztof Kozlowski
  2022-05-23 15:37         ` Jean-Jacques Hiblot
  0 siblings, 1 reply; 11+ messages in thread
From: Krzysztof Kozlowski @ 2022-05-23 15:30 UTC (permalink / raw)
  To: Jean-Jacques Hiblot, Pavel Machek, Rob Herring, Krzysztof Kozlowski
  Cc: linux-leds, devicetree, linux-kernel

On 23/05/2022 17:16, Jean-Jacques Hiblot wrote:
> 
> On 23/05/2022 12:14, Krzysztof Kozlowski wrote:
>> On 23/05/2022 10:49, Jean-Jacques Hiblot wrote:
>>> Add bindings documentation for the TLC5925 LED controller.
>>>
>>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
>> Thank you for your patch. There is something to discuss/improve.
>>
>>> ---
>>> devicetree@vger.kernel.org
>>>   .../bindings/leds/leds-tlc5925.yaml           | 100 ++++++++++++++++++
>>>   1 file changed, 100 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/leds/leds-tlc5925.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/leds/leds-tlc5925.yaml b/Documentation/devicetree/bindings/leds/leds-tlc5925.yaml
>>> new file mode 100644
>>> index 000000000000..156db599d5a1
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/leds/leds-tlc5925.yaml
>> Filename: vendor,device
>> so "ti,tlc5925-leds.yaml" for example.
>>
>>
>>
>>> @@ -0,0 +1,100 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/leds/leds-tlc5925.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: LEDs connected to TI TLC5925 controller
>>> +
>>> +maintainers:
>>> +  - Jean-Jacques Hiblot <jjhiblot@traphandler.com>
>>> +
>>> +description: |
>>> +  The TLC5925 is a low-power 16-channel constant-current LED sink driver.
>>> +  It is controlled through a SPI interface.
>>> +  It is built around a shift register and latches which convert serial
>>> +  input data into a parallel output. Several TLC5925 can be chained to
>>> +  control more than 16 LEDs with a single chip-select.
>>> +  The brightness level cannot be controlled, each LED is either on or off.
>>> +
>>> +  Each LED is represented as a sub-node of the ti,tlc5925 device.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: ti,tlc5925
>>> +
>>> +  shift_register_length:
>>> +    maxItems: 1
>> No...
>> 1. Did you test your bindings with dt_binding_check? This fails
>> obviously... please, do not send untested bindings.
>>
>> 2. vendor prefix, no underscores, proper type, maxItems look wrong here
>>
>>> +    description: |
>>> +      The length of the shift register. If several TLC5925 are chained,
>>> +      shift_register_length should be set to 16 times the number of TLC5925.
>>> +      The value must be a multiple of 8.
>>> +
>>> +  "#address-cells":
>>> +    const: 1
>>> +
>>> +  "#size-cells":
>>> +    const: 0
>>> +
>>> +  output-enable-b-gpios:
>>> +    description: |
>>> +      GPIO pins to enable/disable the parallel output. They describe the GPIOs
>>> +      connected to the OE/ pin of the TLC5925s.
>> maxItems
> 
> There is no limitation in the driver itself. The actual number of items 
> here really depends on the number of chips and how they are wired.

So you could daisy chain 4 billion of devices? Because by not using any
limit you claim that 4 billion is doable?

> 
>>
>>
>>> +
>>> +patternProperties:
>>> +  "@[a-f0-9]+$":
>> How many LEDs you can have here? Usually it is limited, so the pattern
>> should be narrowed.
> 
> There is no limitation here either. The chips can be chained to augment 
> the number of LEDs.
> 
> The max number of LED is equal to the length of the shift-register.



Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] dt-bindings: leds: Add bindings for the TLC5925 controller
  2022-05-23 15:30       ` Krzysztof Kozlowski
@ 2022-05-23 15:37         ` Jean-Jacques Hiblot
  2022-05-23 15:39           ` Krzysztof Kozlowski
  0 siblings, 1 reply; 11+ messages in thread
From: Jean-Jacques Hiblot @ 2022-05-23 15:37 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Pavel Machek, Rob Herring, Krzysztof Kozlowski
  Cc: linux-leds, devicetree, linux-kernel


On 23/05/2022 17:30, Krzysztof Kozlowski wrote:
> On 23/05/2022 17:16, Jean-Jacques Hiblot wrote:
>> On 23/05/2022 12:14, Krzysztof Kozlowski wrote:
>>> On 23/05/2022 10:49, Jean-Jacques Hiblot wrote:
>>>> Add bindings documentation for the TLC5925 LED controller.
>>>>
>>>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
>>> Thank you for your patch. There is something to discuss/improve.
>>>
>>>> ---
>>>> devicetree@vger.kernel.org
>>>>    .../bindings/leds/leds-tlc5925.yaml           | 100 ++++++++++++++++++
>>>>    1 file changed, 100 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/leds/leds-tlc5925.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/leds/leds-tlc5925.yaml b/Documentation/devicetree/bindings/leds/leds-tlc5925.yaml
>>>> new file mode 100644
>>>> index 000000000000..156db599d5a1
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/leds/leds-tlc5925.yaml
>>> Filename: vendor,device
>>> so "ti,tlc5925-leds.yaml" for example.
>>>
>>>
>>>
>>>> @@ -0,0 +1,100 @@
>>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/leds/leds-tlc5925.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: LEDs connected to TI TLC5925 controller
>>>> +
>>>> +maintainers:
>>>> +  - Jean-Jacques Hiblot <jjhiblot@traphandler.com>
>>>> +
>>>> +description: |
>>>> +  The TLC5925 is a low-power 16-channel constant-current LED sink driver.
>>>> +  It is controlled through a SPI interface.
>>>> +  It is built around a shift register and latches which convert serial
>>>> +  input data into a parallel output. Several TLC5925 can be chained to
>>>> +  control more than 16 LEDs with a single chip-select.
>>>> +  The brightness level cannot be controlled, each LED is either on or off.
>>>> +
>>>> +  Each LED is represented as a sub-node of the ti,tlc5925 device.
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: ti,tlc5925
>>>> +
>>>> +  shift_register_length:
>>>> +    maxItems: 1
>>> No...
>>> 1. Did you test your bindings with dt_binding_check? This fails
>>> obviously... please, do not send untested bindings.
>>>
>>> 2. vendor prefix, no underscores, proper type, maxItems look wrong here
>>>
>>>> +    description: |
>>>> +      The length of the shift register. If several TLC5925 are chained,
>>>> +      shift_register_length should be set to 16 times the number of TLC5925.
>>>> +      The value must be a multiple of 8.
>>>> +
>>>> +  "#address-cells":
>>>> +    const: 1
>>>> +
>>>> +  "#size-cells":
>>>> +    const: 0
>>>> +
>>>> +  output-enable-b-gpios:
>>>> +    description: |
>>>> +      GPIO pins to enable/disable the parallel output. They describe the GPIOs
>>>> +      connected to the OE/ pin of the TLC5925s.
>>> maxItems
>> There is no limitation in the driver itself. The actual number of items
>> here really depends on the number of chips and how they are wired.
> So you could daisy chain 4 billion of devices? Because by not using any
> limit you claim that 4 billion is doable?

You could chain 1000 devices or more and have 16000 leds. It would be a 
bit tedious to describe them all in the DTS though.

We can impose a limit but it will be arbitrary. Is this how it is 
usually treated ?

>>>
>>>> +
>>>> +patternProperties:
>>>> +  "@[a-f0-9]+$":
>>> How many LEDs you can have here? Usually it is limited, so the pattern
>>> should be narrowed.
>> There is no limitation here either. The chips can be chained to augment
>> the number of LEDs.
>>
>> The max number of LED is equal to the length of the shift-register.
>
>
> Best regards,
> Krzysztof

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] dt-bindings: leds: Add bindings for the TLC5925 controller
  2022-05-23 15:37         ` Jean-Jacques Hiblot
@ 2022-05-23 15:39           ` Krzysztof Kozlowski
  0 siblings, 0 replies; 11+ messages in thread
From: Krzysztof Kozlowski @ 2022-05-23 15:39 UTC (permalink / raw)
  To: Jean-Jacques Hiblot, Pavel Machek, Rob Herring, Krzysztof Kozlowski
  Cc: linux-leds, devicetree, linux-kernel

On 23/05/2022 17:37, Jean-Jacques Hiblot wrote:
> 
> On 23/05/2022 17:30, Krzysztof Kozlowski wrote:
>> On 23/05/2022 17:16, Jean-Jacques Hiblot wrote:
>>> On 23/05/2022 12:14, Krzysztof Kozlowski wrote:
>>>> On 23/05/2022 10:49, Jean-Jacques Hiblot wrote:
>>>>> Add bindings documentation for the TLC5925 LED controller.
>>>>>
>>>>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
>>>> Thank you for your patch. There is something to discuss/improve.
>>>>
>>>>> ---
>>>>> devicetree@vger.kernel.org
>>>>>    .../bindings/leds/leds-tlc5925.yaml           | 100 ++++++++++++++++++
>>>>>    1 file changed, 100 insertions(+)
>>>>>    create mode 100644 Documentation/devicetree/bindings/leds/leds-tlc5925.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/leds/leds-tlc5925.yaml b/Documentation/devicetree/bindings/leds/leds-tlc5925.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..156db599d5a1
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/leds/leds-tlc5925.yaml
>>>> Filename: vendor,device
>>>> so "ti,tlc5925-leds.yaml" for example.
>>>>
>>>>
>>>>
>>>>> @@ -0,0 +1,100 @@
>>>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/leds/leds-tlc5925.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: LEDs connected to TI TLC5925 controller
>>>>> +
>>>>> +maintainers:
>>>>> +  - Jean-Jacques Hiblot <jjhiblot@traphandler.com>
>>>>> +
>>>>> +description: |
>>>>> +  The TLC5925 is a low-power 16-channel constant-current LED sink driver.
>>>>> +  It is controlled through a SPI interface.
>>>>> +  It is built around a shift register and latches which convert serial
>>>>> +  input data into a parallel output. Several TLC5925 can be chained to
>>>>> +  control more than 16 LEDs with a single chip-select.
>>>>> +  The brightness level cannot be controlled, each LED is either on or off.
>>>>> +
>>>>> +  Each LED is represented as a sub-node of the ti,tlc5925 device.
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: ti,tlc5925
>>>>> +
>>>>> +  shift_register_length:
>>>>> +    maxItems: 1
>>>> No...
>>>> 1. Did you test your bindings with dt_binding_check? This fails
>>>> obviously... please, do not send untested bindings.
>>>>
>>>> 2. vendor prefix, no underscores, proper type, maxItems look wrong here
>>>>
>>>>> +    description: |
>>>>> +      The length of the shift register. If several TLC5925 are chained,
>>>>> +      shift_register_length should be set to 16 times the number of TLC5925.
>>>>> +      The value must be a multiple of 8.
>>>>> +
>>>>> +  "#address-cells":
>>>>> +    const: 1
>>>>> +
>>>>> +  "#size-cells":
>>>>> +    const: 0
>>>>> +
>>>>> +  output-enable-b-gpios:
>>>>> +    description: |
>>>>> +      GPIO pins to enable/disable the parallel output. They describe the GPIOs
>>>>> +      connected to the OE/ pin of the TLC5925s.
>>>> maxItems
>>> There is no limitation in the driver itself. The actual number of items
>>> here really depends on the number of chips and how they are wired.
>> So you could daisy chain 4 billion of devices? Because by not using any
>> limit you claim that 4 billion is doable?
> 
> You could chain 1000 devices or more and have 16000 leds. It would be a 
> bit tedious to describe them all in the DTS though.
> 
> We can impose a limit but it will be arbitrary. Is this how it is 
> usually treated ?

1000 is still less than billion, although above that number it probably
does not make sense to have any limit.

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] dt-bindings: leds: Add bindings for the TLC5925 controller
  2022-05-23  8:49 ` [PATCH 1/3] dt-bindings: leds: Add bindings for the TLC5925 controller Jean-Jacques Hiblot
  2022-05-23 10:14   ` Krzysztof Kozlowski
  2022-05-23 12:33   ` Rob Herring
@ 2022-05-23 20:05   ` Pavel Machek
  2 siblings, 0 replies; 11+ messages in thread
From: Pavel Machek @ 2022-05-23 20:05 UTC (permalink / raw)
  To: Jean-Jacques Hiblot
  Cc: Rob Herring, Krzysztof Kozlowski, linux-leds, devicetree, linux-kernel

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

Hi!

> +                led-satus@0 {

led-status.

							
> +                led-alive@24 {
> +                        reg = <24>;
> +                        label = "green:alive"
> +                };
> +
> +                led-panic@31 {
> +                        reg = <31>;
> +                        label = "red:panic"
> +                };

Please check documentation, if live and panic leds are common, you may
want to add it there. But better make it green:power or something.


BR,
							Pavel

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

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2022-05-23 20:06 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20220523084958.2723943-1-jjhiblot@traphandler.com>
2022-05-23  8:49 ` [PATCH 1/3] dt-bindings: leds: Add bindings for the TLC5925 controller Jean-Jacques Hiblot
2022-05-23 10:14   ` Krzysztof Kozlowski
2022-05-23 13:07     ` Jean-Jacques Hiblot
2022-05-23 15:16     ` Jean-Jacques Hiblot
2022-05-23 15:30       ` Krzysztof Kozlowski
2022-05-23 15:37         ` Jean-Jacques Hiblot
2022-05-23 15:39           ` Krzysztof Kozlowski
2022-05-23 12:33   ` Rob Herring
2022-05-23 20:05   ` Pavel Machek
2022-05-23  8:49 ` [PATCH 2/3] leds: Add driver for the TLC5925 LED controller Jean-Jacques Hiblot
2022-05-23  8:49 ` [PATCH 3/3] leds: tlc5925: Add support for non blocking operations Jean-Jacques Hiblot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.