[PATCH 1/2] leds: tlc5928: Driver for the TI 16 Channel spi LED driver

[PATCH 1/2] leds: tlc5928: Driver for the TI 16 Channel spi LED driver

[Corentin Guillevic]

The TLC59928 is an SPI-connected bus controlled 16-channel LED driver.
A single 16-bit register handles the whole LEDs. Following a write, a
latch GPIO applies the new LED configuration. An "enable" GPIO (blank
in the TLC59928 datasheet) turns off the whole LEDs when active/high.

This driver is able to handle a daisy-chain case, so when several
TLC59928 controllers are connected in serie.

Signed-off-by: Corentin Guillevic <corentin.guillevic@smile.fr>
---
 drivers/leds/Kconfig        |   9 ++
 drivers/leds/Makefile       |   1 +
 drivers/leds/leds-tlc5928.c | 248 ++++++++++++++++++++++++++++++++++++
 3 files changed, 258 insertions(+)
 create mode 100644 drivers/leds/leds-tlc5928.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index b784bb74a837..f429214dd9c2 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -747,6 +747,15 @@ config LEDS_TLC591XX
 	  This option enables support for Texas Instruments TLC59108
 	  and TLC59116 LED controllers.
 
+config LEDS_TLC5928
+	tristate "LED driver for TLC5928 controller"
+	depends on LEDS_CLASS && SPI
+	depends on GPIOLIB
+	select REGMAP_SPI
+	help
+	  This option enables support for Texas Instruments TLC5928
+	  LED controller.
+
 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 18afbb5a23ee..085d4917232a 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -87,6 +87,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_TLC5928)		+= leds-tlc5928.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-tlc5928.c b/drivers/leds/leds-tlc5928.c
new file mode 100644
index 000000000000..5d6663d5d3f3
--- /dev/null
+++ b/drivers/leds/leds-tlc5928.c
@@ -0,0 +1,248 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2025 Corentin Guillevic <corentin.guillevic@smile.fr>
+ */
+
+#include <linux/gpio/consumer.h>
+#include <linux/delay.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/slab.h>
+#include <linux/spi/spi.h>
+
+#define TLC5928_MAX_LEDS	16
+
+#define ldev_to_led(c)		container_of(c, struct tlc5928_led, ldev)
+
+struct tlc5928_led {
+	bool active;
+	unsigned int led_no;
+	struct led_classdev ldev;
+	struct tlc5928_chip *chip;
+};
+
+struct tlc5928_chip {
+	struct gpio_desc *enable_gpio;
+	struct tlc5928_led leds[TLC5928_MAX_LEDS];
+	struct list_head list;
+	struct tlc5928_priv *priv;
+	u16 leds_state;
+};
+
+struct tlc5928_priv {
+	struct spi_device *spi;
+	struct gpio_desc *latch_gpio;
+	struct list_head chips_list;
+	struct mutex lock;
+};
+
+static int
+tlc5928_set_ledout(struct tlc5928_led *led, bool val)
+{
+	struct tlc5928_chip *chip;
+	struct tlc5928_chip *chip_owner = led->chip;
+	struct tlc5928_priv *priv = chip_owner->priv;
+	int ret;
+
+	mutex_lock(&priv->lock);
+
+	if (val)
+		chip_owner->leds_state |= (1 << led->led_no);
+	else
+		chip_owner->leds_state &= ~(1 << led->led_no);
+
+	list_for_each_entry_reverse(chip, &priv->chips_list, list) {
+		u16 leds_state = cpu_to_be16(chip->leds_state);
+
+		ret = spi_write(priv->spi, &(leds_state), sizeof(leds_state));
+
+		if (ret)
+			return ret;
+	}
+
+	gpiod_set_value(priv->latch_gpio, 0);
+	udelay(1);
+	gpiod_set_value(priv->latch_gpio, 1);
+
+	mutex_unlock(&priv->lock);
+
+	return 0;
+}
+
+static int
+tlc5928_brightness_set(struct led_classdev *led_cdev,
+			enum led_brightness brightness)
+{
+	struct tlc5928_led *led = ldev_to_led(led_cdev);
+
+	/* TLC5928 only allows on/off, no brightness */
+	return tlc5928_set_ledout(led, !!brightness);
+}
+
+static const struct of_device_id of_tlc5928_leds_match[] __maybe_unused = {
+	{ .compatible = "ti,tlc5928" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, of_tlc5928_leds_match);
+
+static int tlc5928_probe_chip_dt(struct device *dev, struct device_node *node,
+		struct tlc5928_chip *chip)
+{
+	struct device_node *child;
+	int count, err, reg;
+
+	count = of_get_available_child_count(node);
+	if (!count)
+		return -EINVAL;
+
+	chip->leds_state = 0;
+
+	for_each_available_child_of_node(node, child) {
+		struct tlc5928_led *led;
+		struct led_init_data init_data = {};
+
+		init_data.fwnode = of_fwnode_handle(child);
+
+		err = of_property_read_u32(child, "reg", &reg);
+		if (err) {
+			dev_err(dev, "%pOF: failed to read reg\n", child);
+			of_node_put(child);
+			return err;
+		}
+
+		if (reg < 0 || reg >= TLC5928_MAX_LEDS ||
+				chip->leds[reg].active) {
+			of_node_put(child);
+			return -EINVAL;
+		}
+
+		led = &chip->leds[reg];
+
+		led->active = true;
+		led->chip = chip;
+		led->led_no = reg;
+		led->ldev.brightness_set_blocking = tlc5928_brightness_set;
+		err = devm_led_classdev_register_ext(dev, &led->ldev,
+							 &init_data);
+		if (err < 0) {
+			of_node_put(child);
+			dev_err(dev, "Failed to register LED for node %pfw\n",
+				init_data.fwnode);
+			return err;
+		}
+	}
+
+	return 0;
+}
+
+static int tlc5928_probe(struct spi_device *spi)
+{
+	struct device_node *node, *child;
+	struct device *dev = &spi->dev;
+	struct list_head *pos;
+	struct tlc5928_chip *chip;
+	struct tlc5928_priv *priv;
+	int count, err, i;
+
+	node = dev_of_node(dev);
+	if (!node)
+		return -ENODEV;
+
+	count = of_get_available_child_count(node);
+	if (!count)
+		return -EINVAL;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->spi = spi;
+	priv->latch_gpio = devm_gpiod_get(dev, "latch", GPIOD_OUT_HIGH);
+	if (IS_ERR(priv->latch_gpio))
+		return dev_err_probe(dev, PTR_ERR(priv->latch_gpio),
+				     "Failed to get latch GPIO\n");
+
+	mutex_init(&priv->lock);
+	INIT_LIST_HEAD(&priv->chips_list);
+
+	i = 0;
+	for_each_available_child_of_node(node, child) {
+		chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
+		if (!chip)
+			return -ENOMEM;
+
+		list_add_tail(&chip->list, &priv->chips_list);
+		chip->priv = priv;
+		chip->enable_gpio = devm_gpiod_get_index_optional(dev, "enable", i,
+				GPIOD_OUT_HIGH);
+		if (IS_ERR(chip->enable_gpio)) {
+			dev_err(dev, "Error getting enable GPIO %i property: %ld\n", i,
+					PTR_ERR(chip->enable_gpio));
+			return PTR_ERR(chip->enable_gpio);
+		}
+
+		err = tlc5928_probe_chip_dt(dev, child, chip);
+		if (err)
+			return err;
+
+		i++;
+	}
+
+	list_for_each(pos, &priv->chips_list) {
+		chip = container_of(pos, struct tlc5928_chip, list);
+		if (chip->enable_gpio)
+			gpiod_set_value(chip->enable_gpio, 0);
+	}
+
+	spi_set_drvdata(spi, priv);
+
+	return 0;
+}
+
+static int tlc5928_remove(struct spi_device *spi)
+{
+	struct list_head *pos;
+	struct tlc5928_priv *priv = spi_get_drvdata(spi);
+	int i;
+
+	list_for_each(pos, &priv->chips_list) {
+		struct tlc5928_chip *chip = container_of(pos, struct tlc5928_chip,
+				list);
+
+		for (i = 0; i < TLC5928_MAX_LEDS; i++) {
+			if (chip->leds[i].active)
+				devm_led_classdev_unregister(&spi->dev,
+					     &chip->leds[i].ldev);
+		}
+
+		if (chip->enable_gpio) {
+			gpiod_set_value(chip->enable_gpio, 1);
+			gpiod_put(chip->enable_gpio);
+		}
+	}
+
+	return 0;
+}
+
+static const struct spi_device_id tlc5928_id[] = {
+	{ "tlc5928" },
+	{},
+};
+MODULE_DEVICE_TABLE(spi, tlc5928_id);
+
+static struct spi_driver tlc5928_driver = {
+	.driver = {
+		.name = "tlc5928",
+		.of_match_table = of_match_ptr(of_tlc5928_leds_match),
+	},
+	.probe = tlc5928_probe,
+	.remove = tlc5928_remove,
+	.id_table = tlc5928_id,
+};
+
+module_spi_driver(tlc5928_driver);
+
+MODULE_AUTHOR("Corentin Guillevic <corentin.guillevic@smile.fr>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("TLC5928 LED driver");
-- 
2.45.2

[PATCH 2/2] dt-bindings: leds: Add TI TLC5928 LED

[Corentin Guillevic]

Document Texas Instruments TLC5928 LED driver devicetree bindings.

Signed-off-by: Corentin Guillevic <corentin.guillevic@smile.fr>
---
 .../bindings/leds/leds-tlc5928.yaml           | 212 ++++++++++++++++++
 1 file changed, 212 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-tlc5928.yaml

diff --git a/Documentation/devicetree/bindings/leds/leds-tlc5928.yaml b/Documentation/devicetree/bindings/leds/leds-tlc5928.yaml
new file mode 100644
index 000000000000..0d857c9b1feb
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-tlc5928.yaml
@@ -0,0 +1,212 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/leds-tlc5928.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: LED driver for TLC5928 from Texas Instruments.
+
+maintainers:
+  - Corentin Guillevic <corentin.guillevic@smile.fr>
+
+description: |
+  The TLC5928 is a LED controller handling up to 16 LEDs. It can
+  control LED on/off using a SPI-compatible interface, and has an
+  on/off control data shift register (blank) and data latch.
+
+  This driver also supports the daisy-chaining of several TLC5928
+  chips, as illustrated by the diagram below (with two controllers):
+
+  +--------------+           +--------------+
+  |          SCLK|-----+---->|SCLK     BLANK|--
+  |              |     |     |              |
+  |  SPI     MOSI|-----|---->|MOSI  TLC5928 |
+  | Master       |     |     |        (1)   |
+  |          MISO|<--+ |  +--|MISO          |
+  |              |   | |  |  |              |
+  |      CS/LATCH|-+-|-|--|->|LATCH         |
+  +--------------+ | | |  |  +--------------+
+                   | | |  |  +--------------+
+                   | | +--|->|SCLK     BLANK|--
+                   | |    |  |              |
+                   | |    +->|MOSI  TLC5928 |
+                   | |       |        (2)   |
+                   | +-------|MISO          |
+                   |         |              |
+                   +-------->|LATCH         |
+                             +--------------+
+
+  For more product information please see the link below:
+  https://www.ti.com/product/TLC5928/part-details/TLC5928PWPR
+
+properties:
+  compatible:
+    const: ti,tlc5928
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+  reg:
+    maxItems: 1
+    description:
+      SPI slave address
+
+  enable-gpios:
+    description: |
+      Array of GPIO specifiers, referring to the GPIO pins to enable/disable
+      each device (active high to disable). In the daisy chain case, each
+      GPIO has to be in the same sequence than the devices.
+
+  latch-gpio:
+    maxItems: 1
+    description: Latch GPIO (SPI chip select)
+
+patternProperties:
+  "^spi-chip@[0-9]$":
+    type: object
+    unevaluatedProperties: false
+    description: Properties for a TLC5928 controller.
+
+    properties:
+      '#address-cells':
+        const: 1
+
+      '#size-cells':
+        const: 0
+
+    patternProperties:
+
+      "^led@[0-9a-f]+$":
+        type: object
+        $ref: common.yaml#
+        unevaluatedProperties: false
+        description:
+          Properties for a single LED.
+
+        properties:
+          reg:
+            description: Index of the LED.
+            minimum: 0
+            maximum: 15
+
+        required:
+          - reg
+
+    required:
+      - "#address-cells"
+      - "#size-cells"
+
+required:
+  - compatible
+  - reg
+  - latch-gpio
+
+unevaluatedProperties: false
+
+examples:
+  # Single controller
+  - |
+	#include <dt-bindings/leds/common.h>
+
+	tlc5928@0 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "ti,tlc5928";
+		reg = <0x0>;
+
+		enable-gpios = <&gpiof 10 GPIO_ACTIVE_HIGH>;
+		latch-gpio = <&gpiof 3 GPIO_ACTIVE_HIGH>;
+
+		spi-chip@0 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			led@0 {
+				label = "tlc5928:led0";
+				led = <LED_COLOR_ID_WHITE>;
+				reg = <0x0>;
+			};
+
+			led@1 {
+				label = "tlc5928:led1";
+				led = <LED_COLOR_ID_RED>;
+				reg = <0x1>;
+			};
+
+			led@f {
+				label = "tlc5928:led15";
+				led = <LED_COLOR_ID_GREEN>;
+				reg = <0xf>;
+			};
+		};
+	};
+
+  # Two controllers, in daisy chain
+  - |
+	#include <dt-bindings/leds/common.h>
+
+	tlc5928@0 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "ti,tlc5928";
+		reg = <0x0>;
+
+		enable-gpios = <&gpiof 10 GPIO_ACTIVE_HIGH>, /* OUT_EN_1 */
+			<&gpiof 9 GPIO_ACTIVE_HIGH>; /* OUT_EN_2 */
+		latch-gpio = <&gpiof 3 GPIO_ACTIVE_HIGH>;
+
+		spi-chip@0 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			led@0 {
+				label = "tlc5928:0:led0";
+				led = <LED_COLOR_ID_WHITE>;
+				reg = <0x0>;
+			};
+
+			led@1 {
+				label = "tlc5928:0:led1";
+				led = <LED_COLOR_ID_RED>;
+				reg = <0x1>;
+			};
+
+			led@f {
+				label = "tlc5928:0:led15";
+				led = <LED_COLOR_ID_GREEN>;
+				reg = <0xf>;
+			};
+		};
+
+		spi-chip@1 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			led@0 {
+				label = "tlc5928:1:led0";
+				led = <LED_COLOR_ID_BLUE>;
+				reg = <0x0>;
+			};
+
+			led@1 {
+				label = "tlc5928:1:led1";
+				led = <LED_COLOR_ID_AMBER>;
+				reg = <0x1>;
+			};
+
+			led@2 {
+				label = "tlc5928:1:led2";
+				led = <LED_COLOR_ID_VIOLET>;
+				reg = <0x2>;
+			};
+
+			led@f {
+				label = "tlc5928:1:led15";
+				led = <LED_COLOR_ID_YELLOW>;
+				reg = <0xf>;
+			};
+		};
+	};
-- 
2.45.2

Re: [PATCH 2/2] dt-bindings: leds: Add TI TLC5928 LED

[Rob Herring]

On Wed, Mar 26, 2025 at 04:35:33PM +0100, Corentin Guillevic wrote:
> Document Texas Instruments TLC5928 LED driver devicetree bindings.
> 
> Signed-off-by: Corentin Guillevic <corentin.guillevic@smile.fr>
> ---
>  .../bindings/leds/leds-tlc5928.yaml           | 212 ++++++++++++++++++
>  1 file changed, 212 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-tlc5928.yaml
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-tlc5928.yaml b/Documentation/devicetree/bindings/leds/leds-tlc5928.yaml
> new file mode 100644
> index 000000000000..0d857c9b1feb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-tlc5928.yaml
> @@ -0,0 +1,212 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/leds-tlc5928.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: LED driver for TLC5928 from Texas Instruments.
> +
> +maintainers:
> +  - Corentin Guillevic <corentin.guillevic@smile.fr>
> +
> +description: |
> +  The TLC5928 is a LED controller handling up to 16 LEDs. It can
> +  control LED on/off using a SPI-compatible interface, and has an
> +  on/off control data shift register (blank) and data latch.
> +
> +  This driver also supports the daisy-chaining of several TLC5928
> +  chips, as illustrated by the diagram below (with two controllers):
> +
> +  +--------------+           +--------------+
> +  |          SCLK|-----+---->|SCLK     BLANK|--
> +  |              |     |     |              |
> +  |  SPI     MOSI|-----|---->|MOSI  TLC5928 |
> +  | Master       |     |     |        (1)   |
> +  |          MISO|<--+ |  +--|MISO          |
> +  |              |   | |  |  |              |
> +  |      CS/LATCH|-+-|-|--|->|LATCH         |
> +  +--------------+ | | |  |  +--------------+
> +                   | | |  |  +--------------+
> +                   | | +--|->|SCLK     BLANK|--
> +                   | |    |  |              |
> +                   | |    +->|MOSI  TLC5928 |
> +                   | |       |        (2)   |
> +                   | +-------|MISO          |
> +                   |         |              |
> +                   +-------->|LATCH         |
> +                             +--------------+
> +
> +  For more product information please see the link below:
> +  https://www.ti.com/product/TLC5928/part-details/TLC5928PWPR
> +
> +properties:
> +  compatible:
> +    const: ti,tlc5928
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +  reg:
> +    maxItems: 1
> +    description:
> +      SPI slave address
> +
> +  enable-gpios:
> +    description: |

Don't need '|' if no formatting.

> +      Array of GPIO specifiers, referring to the GPIO pins to enable/disable
> +      each device (active high to disable). In the daisy chain case, each
> +      GPIO has to be in the same sequence than the devices.
> +
> +  latch-gpio:

latch-gpios

-gpio is deprecated.

> +    maxItems: 1
> +    description: Latch GPIO (SPI chip select)
> +
> +patternProperties:
> +  "^spi-chip@[0-9]$":
> +    type: object
> +    unevaluatedProperties: false
> +    description: Properties for a TLC5928 controller.

I don't think this level is needed. Just encode the chip instance into 
the LED address. Either 1 cell splitting the bits or 2 cells with chip 
instance in 1st cell and LED# in the 2nd cell.

> +
> +    properties:
> +      '#address-cells':
> +        const: 1
> +
> +      '#size-cells':
> +        const: 0
> +
> +    patternProperties:
> +
> +      "^led@[0-9a-f]+$":

Don't need the '+' as the max is 'f'. That will change with the above 
though. You'll want something like '^led@[0-9a-f]+,[0-9a-f]$' with the 
chip and led numbers split.

> +        type: object
> +        $ref: common.yaml#
> +        unevaluatedProperties: false
> +        description:
> +          Properties for a single LED.

Re: [PATCH 1/2] leds: tlc5928: Driver for the TI 16 Channel spi LED driver

[Christophe JAILLET]

Le 26/03/2025 à 16:35, Corentin Guillevic a écrit :
> The TLC59928 is an SPI-connected bus controlled 16-channel LED driver.
> A single 16-bit register handles the whole LEDs. Following a write, a
> latch GPIO applies the new LED configuration. An "enable" GPIO (blank
> in the TLC59928 datasheet) turns off the whole LEDs when active/high.
> 
> This driver is able to handle a daisy-chain case, so when several
> TLC59928 controllers are connected in serie.
> 
> Signed-off-by: Corentin Guillevic <corentin.guillevic@smile.fr>
> ---

...

> +static int
> +tlc5928_set_ledout(struct tlc5928_led *led, bool val)
> +{
> +	struct tlc5928_chip *chip;
> +	struct tlc5928_chip *chip_owner = led->chip;
> +	struct tlc5928_priv *priv = chip_owner->priv;
> +	int ret;
> +
> +	mutex_lock(&priv->lock);
> +
> +	if (val)
> +		chip_owner->leds_state |= (1 << led->led_no);
> +	else
> +		chip_owner->leds_state &= ~(1 << led->led_no);
> +
> +	list_for_each_entry_reverse(chip, &priv->chips_list, list) {
> +		u16 leds_state = cpu_to_be16(chip->leds_state);
> +
> +		ret = spi_write(priv->spi, &(leds_state), sizeof(leds_state));
> +
> +		if (ret)

Missing unlock.
Or use guard()?

> +			return ret;
> +	}
> +
> +	gpiod_set_value(priv->latch_gpio, 0);
> +	udelay(1);
> +	gpiod_set_value(priv->latch_gpio, 1);
> +
> +	mutex_unlock(&priv->lock);
> +
> +	return 0;
> +}
> +
> +static int
> +tlc5928_brightness_set(struct led_classdev *led_cdev,
> +			enum led_brightness brightness)
> +{
> +	struct tlc5928_led *led = ldev_to_led(led_cdev);
> +
> +	/* TLC5928 only allows on/off, no brightness */
> +	return tlc5928_set_ledout(led, !!brightness);
> +}
> +
> +static const struct of_device_id of_tlc5928_leds_match[] __maybe_unused = {
> +	{ .compatible = "ti,tlc5928" },
> +	{},

Unneeded trailing ,

> +};
> +MODULE_DEVICE_TABLE(of, of_tlc5928_leds_match);
> +
> +static int tlc5928_probe_chip_dt(struct device *dev, struct device_node *node,
> +		struct tlc5928_chip *chip)
> +{
> +	struct device_node *child;
> +	int count, err, reg;
> +
> +	count = of_get_available_child_count(node);
> +	if (!count)
> +		return -EINVAL;
> +
> +	chip->leds_state = 0;
> +
> +	for_each_available_child_of_node(node, child) {

for_each_available_child_of_node_scoped()?

> +		struct tlc5928_led *led;
> +		struct led_init_data init_data = {};
> +
> +		init_data.fwnode = of_fwnode_handle(child);
> +
> +		err = of_property_read_u32(child, "reg", &reg);
> +		if (err) {
> +			dev_err(dev, "%pOF: failed to read reg\n", child);
> +			of_node_put(child);
> +			return err;
> +		}
> +
> +		if (reg < 0 || reg >= TLC5928_MAX_LEDS ||
> +				chip->leds[reg].active) {
> +			of_node_put(child);
> +			return -EINVAL;
> +		}
> +
> +		led = &chip->leds[reg];
> +
> +		led->active = true;
> +		led->chip = chip;
> +		led->led_no = reg;
> +		led->ldev.brightness_set_blocking = tlc5928_brightness_set;
> +		err = devm_led_classdev_register_ext(dev, &led->ldev,
> +							 &init_data);
> +		if (err < 0) {
> +			of_node_put(child);
> +			dev_err(dev, "Failed to register LED for node %pfw\n",
> +				init_data.fwnode);
> +			return err;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int tlc5928_probe(struct spi_device *spi)
> +{
> +	struct device_node *node, *child;
> +	struct device *dev = &spi->dev;
> +	struct list_head *pos;
> +	struct tlc5928_chip *chip;
> +	struct tlc5928_priv *priv;
> +	int count, err, i;
> +
> +	node = dev_of_node(dev);
> +	if (!node)
> +		return -ENODEV;
> +
> +	count = of_get_available_child_count(node);
> +	if (!count)
> +		return -EINVAL;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->spi = spi;
> +	priv->latch_gpio = devm_gpiod_get(dev, "latch", GPIOD_OUT_HIGH);
> +	if (IS_ERR(priv->latch_gpio))
> +		return dev_err_probe(dev, PTR_ERR(priv->latch_gpio),
> +				     "Failed to get latch GPIO\n");
> +
> +	mutex_init(&priv->lock);

Maybe:
err = devm_mutex_init(...);
if (err)
	return err;

?

> +	INIT_LIST_HEAD(&priv->chips_list);
> +
> +	i = 0;
> +	for_each_available_child_of_node(node, child) {
> +		chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
> +		if (!chip)
> +			return -ENOMEM;
> +
> +		list_add_tail(&chip->list, &priv->chips_list);
> +		chip->priv = priv;
> +		chip->enable_gpio = devm_gpiod_get_index_optional(dev, "enable", i,
> +				GPIOD_OUT_HIGH);
> +		if (IS_ERR(chip->enable_gpio)) {
> +			dev_err(dev, "Error getting enable GPIO %i property: %ld\n", i,
> +					PTR_ERR(chip->enable_gpio));
> +			return PTR_ERR(chip->enable_gpio);
> +		}
> +
> +		err = tlc5928_probe_chip_dt(dev, child, chip);
> +		if (err)
> +			return err;
> +
> +		i++;
> +	}
> +
> +	list_for_each(pos, &priv->chips_list) {

list_for_each_entry()?

> +		chip = container_of(pos, struct tlc5928_chip, list);
> +		if (chip->enable_gpio)
> +			gpiod_set_value(chip->enable_gpio, 0);
> +	}
> +
> +	spi_set_drvdata(spi, priv);
> +
> +	return 0;
> +}
> +
> +static int tlc5928_remove(struct spi_device *spi)
> +{
> +	struct list_head *pos;
> +	struct tlc5928_priv *priv = spi_get_drvdata(spi);
> +	int i;
> +
> +	list_for_each(pos, &priv->chips_list) {

list_for_each_entry()?

> +		struct tlc5928_chip *chip = container_of(pos, struct tlc5928_chip,
> +				list);
> +
> +		for (i = 0; i < TLC5928_MAX_LEDS; i++) {
> +			if (chip->leds[i].active)
> +				devm_led_classdev_unregister(&spi->dev,
> +					     &chip->leds[i].ldev);

Why is it needed?
devm_led_classdev_register_ext() was used.

> +		}
> +
> +		if (chip->enable_gpio) {
> +			gpiod_set_value(chip->enable_gpio, 1);
> +			gpiod_put(chip->enable_gpio);

Why is it needed?
devm_gpiod_get_index_optional() was used.

> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct spi_device_id tlc5928_id[] = {
> +	{ "tlc5928" },
> +	{},

Unneeded trailing ,

> +};
> +MODULE_DEVICE_TABLE(spi, tlc5928_id);

...

CJ

Re: [PATCH 1/2] leds: tlc5928: Driver for the TI 16 Channel spi LED driver

[Corentin GUILLEVIC]

Hi,

Thank you for your review! I've fixed everything (new patch is
coming), but I have issue for some of them: I can't use the suggested
functions (guard(), for_each_available_child_of_node_scoped() and
devm_mutex_init()) because the kernel version used on my device is too
old (v5.15). No way to test with a newer version...

Should I let the "old" functions because of my kernel version?

>
> Le 26/03/2025 à 16:35, Corentin Guillevic a écrit :
> > The TLC59928 is an SPI-connected bus controlled 16-channel LED driver.
> > A single 16-bit register handles the whole LEDs. Following a write, a
> > latch GPIO applies the new LED configuration. An "enable" GPIO (blank
> > in the TLC59928 datasheet) turns off the whole LEDs when active/high.
> >
> > This driver is able to handle a daisy-chain case, so when several
> > TLC59928 controllers are connected in serie.
> >
> > Signed-off-by: Corentin Guillevic <corentin.guillevic@smile.fr>
> > ---
>
> ...
>
> > +static int
> > +tlc5928_set_ledout(struct tlc5928_led *led, bool val)
> > +{
> > +     struct tlc5928_chip *chip;
> > +     struct tlc5928_chip *chip_owner = led->chip;
> > +     struct tlc5928_priv *priv = chip_owner->priv;
> > +     int ret;
> > +
> > +     mutex_lock(&priv->lock);
> > +
> > +     if (val)
> > +             chip_owner->leds_state |= (1 << led->led_no);
> > +     else
> > +             chip_owner->leds_state &= ~(1 << led->led_no);
> > +
> > +     list_for_each_entry_reverse(chip, &priv->chips_list, list) {
> > +             u16 leds_state = cpu_to_be16(chip->leds_state);
> > +
> > +             ret = spi_write(priv->spi, &(leds_state), sizeof(leds_state));
> > +
> > +             if (ret)
>
> Missing unlock.
> Or use guard()?
>

Fixed! But guard() is unavailable on my kernel.

> > +                     return ret;
> > +     }
> > +
> > +     gpiod_set_value(priv->latch_gpio, 0);
> > +     udelay(1);
> > +     gpiod_set_value(priv->latch_gpio, 1);
> > +
> > +     mutex_unlock(&priv->lock);
> > +
> > +     return 0;
> > +}
> > +
> > +static int
> > +tlc5928_brightness_set(struct led_classdev *led_cdev,
> > +                     enum led_brightness brightness)
> > +{
> > +     struct tlc5928_led *led = ldev_to_led(led_cdev);
> > +
> > +     /* TLC5928 only allows on/off, no brightness */
> > +     return tlc5928_set_ledout(led, !!brightness);
> > +}
> > +
> > +static const struct of_device_id of_tlc5928_leds_match[] __maybe_unused = {
> > +     { .compatible = "ti,tlc5928" },
> > +     {},
>
> Unneeded trailing ,
>
> > +};
> > +MODULE_DEVICE_TABLE(of, of_tlc5928_leds_match);
> > +
> > +static int tlc5928_probe_chip_dt(struct device *dev, struct device_node *node,
> > +             struct tlc5928_chip *chip)
> > +{
> > +     struct device_node *child;
> > +     int count, err, reg;
> > +
> > +     count = of_get_available_child_count(node);
> > +     if (!count)
> > +             return -EINVAL;
> > +
> > +     chip->leds_state = 0;
> > +
> > +     for_each_available_child_of_node(node, child) {
>
> for_each_available_child_of_node_scoped()?
>

Same, not defined because my kernel is too old.

> > +             struct tlc5928_led *led;
> > +             struct led_init_data init_data = {};
> > +
> > +             init_data.fwnode = of_fwnode_handle(child);
> > +
> > +             err = of_property_read_u32(child, "reg", &reg);
> > +             if (err) {
> > +                     dev_err(dev, "%pOF: failed to read reg\n", child);
> > +                     of_node_put(child);
> > +                     return err;
> > +             }
> > +
> > +             if (reg < 0 || reg >= TLC5928_MAX_LEDS ||
> > +                             chip->leds[reg].active) {
> > +                     of_node_put(child);
> > +                     return -EINVAL;
> > +             }
> > +
> > +             led = &chip->leds[reg];
> > +
> > +             led->active = true;
> > +             led->chip = chip;
> > +             led->led_no = reg;
> > +             led->ldev.brightness_set_blocking = tlc5928_brightness_set;
> > +             err = devm_led_classdev_register_ext(dev, &led->ldev,
> > +                                                      &init_data);
> > +             if (err < 0) {
> > +                     of_node_put(child);
> > +                     dev_err(dev, "Failed to register LED for node %pfw\n",
> > +                             init_data.fwnode);
> > +                     return err;
> > +             }
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int tlc5928_probe(struct spi_device *spi)
> > +{
> > +     struct device_node *node, *child;
> > +     struct device *dev = &spi->dev;
> > +     struct list_head *pos;
> > +     struct tlc5928_chip *chip;
> > +     struct tlc5928_priv *priv;
> > +     int count, err, i;
> > +
> > +     node = dev_of_node(dev);
> > +     if (!node)
> > +             return -ENODEV;
> > +
> > +     count = of_get_available_child_count(node);
> > +     if (!count)
> > +             return -EINVAL;
> > +
> > +     priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +     if (!priv)
> > +             return -ENOMEM;
> > +
> > +     priv->spi = spi;
> > +     priv->latch_gpio = devm_gpiod_get(dev, "latch", GPIOD_OUT_HIGH);
> > +     if (IS_ERR(priv->latch_gpio))
> > +             return dev_err_probe(dev, PTR_ERR(priv->latch_gpio),
> > +                                  "Failed to get latch GPIO\n");
> > +
> > +     mutex_init(&priv->lock);
>
> Maybe:
> err = devm_mutex_init(...);
> if (err)
>         return err;
>
> ?

Same.

>
> > +     INIT_LIST_HEAD(&priv->chips_list);
> > +
> > +     i = 0;
> > +     for_each_available_child_of_node(node, child) {
> > +             chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
> > +             if (!chip)
> > +                     return -ENOMEM;
> > +
> > +             list_add_tail(&chip->list, &priv->chips_list);
> > +             chip->priv = priv;
> > +             chip->enable_gpio = devm_gpiod_get_index_optional(dev, "enable", i,
> > +                             GPIOD_OUT_HIGH);
> > +             if (IS_ERR(chip->enable_gpio)) {
> > +                     dev_err(dev, "Error getting enable GPIO %i property: %ld\n", i,
> > +                                     PTR_ERR(chip->enable_gpio));
> > +                     return PTR_ERR(chip->enable_gpio);
> > +             }
> > +
> > +             err = tlc5928_probe_chip_dt(dev, child, chip);
> > +             if (err)
> > +                     return err;
> > +
> > +             i++;
> > +     }
> > +
> > +     list_for_each(pos, &priv->chips_list) {
>
> list_for_each_entry()?
>
> > +             chip = container_of(pos, struct tlc5928_chip, list);
> > +             if (chip->enable_gpio)
> > +                     gpiod_set_value(chip->enable_gpio, 0);
> > +     }
> > +
> > +     spi_set_drvdata(spi, priv);
> > +
> > +     return 0;
> > +}
> > +
> > +static int tlc5928_remove(struct spi_device *spi)
> > +{
> > +     struct list_head *pos;
> > +     struct tlc5928_priv *priv = spi_get_drvdata(spi);
> > +     int i;
> > +
> > +     list_for_each(pos, &priv->chips_list) {
>
> list_for_each_entry()?
>

Fixed!

> > +             struct tlc5928_chip *chip = container_of(pos, struct tlc5928_chip,
> > +                             list);
> > +
> > +             for (i = 0; i < TLC5928_MAX_LEDS; i++) {
> > +                     if (chip->leds[i].active)
> > +                             devm_led_classdev_unregister(&spi->dev,
> > +                                          &chip->leds[i].ldev);
>
> Why is it needed?
> devm_led_classdev_register_ext() was used.
>

Yes, because the latch GPIO is set each time a LED is set. But during
the module removing process, devm releases before the GPIO and then
each LED (turning them off). So the kernel gets a NULL pointer after
deference.

An explicit unregister allows to free the LEDs before the GPIO.

> > +             }
> > +
> > +             if (chip->enable_gpio) {
> > +                     gpiod_set_value(chip->enable_gpio, 1);
> > +                     gpiod_put(chip->enable_gpio);
>
> Why is it needed?
> devm_gpiod_get_index_optional() was used.
>

Fixed!

> > +             }
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct spi_device_id tlc5928_id[] = {
> > +     { "tlc5928" },
> > +     {},
>
> Unneeded trailing ,
>
> > +};
> > +MODULE_DEVICE_TABLE(spi, tlc5928_id);
>
> ...
>
> CJ
>

Corentin Guillevic

Re: [PATCH 1/2] leds: tlc5928: Driver for the TI 16 Channel spi LED driver

[Christophe JAILLET]

Le 04/04/2025 à 15:54, Corentin GUILLEVIC a écrit :
> Hi,
> 
> Thank you for your review! I've fixed everything (new patch is
> coming), but I have issue for some of them: I can't use the suggested
> functions (guard(), for_each_available_child_of_node_scoped() and
> devm_mutex_init()) because the kernel version used on my device is too
> old (v5.15). No way to test with a newer version...
> 
> Should I let the "old" functions because of my kernel version?

No strong opinion on my side on this.

The proposed changes were just to have a slightly less verbose code. I 
guess that having a tested code worth much more than using some 
convenience functions to save a few lines of code.

Let see the position of maintainers on it.

CJ