[PATCH v3 0/3] Add support for the TLC5925

[PATCH v3 0/3] Add support for the TLC5925

[Jean-Jacques Hiblot]

This series add the support for the TLC5925 LED controller.
This LED controller is driven though SPI. There is little internal logic
and it can be thought of as a deserializer + latches.
The TLC5925 itself drives up to 16 LEDs, but multiple TLC5925s can be
chained to drive more.

The first patch describes the dt bindings.
The second patch implements most of the driver and supports only
synchronous brightness setting (brightness_set_blocking).
The last patch implements the non-blocking version (brightness_set).

changes v2->v3:
 * fixed the yaml description of the bindings (now passes dt_binding_check)
 * renamed shit-register-length into ti,shift-register-length and specify its
   type

changes v1->v2:
 * renamed property shift_register_length into shift-register-length
 * add a SPI MODULE_DEVICE_TABLE structure
 * fixed the yaml description of the bindings (now passes dt_binding_check)

Jean-Jacques Hiblot (3):
  dt-bindings: leds: Add bindings for the TLC5925 controller
  leds: Add driver for the TLC5925 LED controller
  leds: tlc5925: Add support for non blocking operations

 .../devicetree/bindings/leds/ti,tlc5925.yaml  | 107 +++++++++
 drivers/leds/Kconfig                          |   6 +
 drivers/leds/Makefile                         |   1 +
 drivers/leds/leds-tlc5925.c                   | 224 ++++++++++++++++++
 4 files changed, 338 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/ti,tlc5925.yaml
 create mode 100644 drivers/leds/leds-tlc5925.c

-- 
2.25.1

[PATCH v3 1/3] dt-bindings: leds: Add bindings for the TLC5925 controller

[Jean-Jacques Hiblot]

Add bindings documentation for the TLC5925 LED controller.

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

diff --git a/Documentation/devicetree/bindings/leds/ti,tlc5925.yaml b/Documentation/devicetree/bindings/leds/ti,tlc5925.yaml
new file mode 100644
index 000000000000..12a71e48f854
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/ti,tlc5925.yaml
@@ -0,0 +1,107 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/ti,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.
+
+$ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+  compatible:
+    const: ti,tlc5925
+
+  ti,shift-register-length:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 8
+    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:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description: |
+      Optional GPIO pins to enable/disable the parallel output. They describe
+      the GPIOs connected to the OE/ pin of the TLC5925s.
+
+patternProperties:
+  "@[0-9a-f]+$":
+    type: object
+    $ref: common.yaml#
+
+    properties:
+      reg:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        description: |
+          LED pin number (must be lower than ti,shift-register-length).
+          The furthest LED down the chain has the pin number 0.
+
+    required:
+      - reg
+
+required:
+  - "#address-cells"
+  - "#size-cells"
+  - ti,shift-register-length
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/leds/common.h>
+
+    spi0 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        leds@2 {
+            compatible = "ti,tlc5925";
+            reg = <0x02>;
+            spi-max-frequency = <30000000>;
+            ti,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@0 {
+                reg = <0>;
+                function = LED_FUNCTION_STATUS;
+                color = <LED_COLOR_ID_GREEN>;
+            };
+
+            led@4 {
+                reg = <4>;
+                function = LED_FUNCTION_STATUS;
+                color = <LED_COLOR_ID_RED>;
+            };
+
+            led@1f {
+                reg = <31>;
+                function = LED_FUNCTION_PANIC;
+                color = <LED_COLOR_ID_RED>;
+            };
+        };
+
+    };
-- 
2.25.1

[PATCH v3 2/3] leds: Add driver for the TLC5925 LED controller

[Jean-Jacques Hiblot]

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 | 164 ++++++++++++++++++++++++++++++++++++
 3 files changed, 171 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..2c50ba10bdbf
--- /dev/null
+++ b/drivers/leds/leds-tlc5925.c
@@ -0,0 +1,164 @@
+// 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, "ti,shift-register-length",
+					     &max_num_leds, 1);
+	if (ret) {
+		dev_err(dev, "'ti,shift-register-length' property is required.\n");
+		return -EINVAL;
+	}
+
+	if (max_num_leds % 8) {
+		dev_err(dev, "'ti,shift-register-length' must be a multiple of 8\n");
+		return -EINVAL;
+	}
+
+	if (max_num_leds == 0) {
+		dev_err(dev, "'ti,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 const struct spi_device_id tlc5925_id[] = {
+	{"tlc5925", 0},
+	{}
+};
+MODULE_DEVICE_TABLE(spi, tlc5925_id);
+
+static struct spi_driver tlc5925_driver = {
+	.driver = {
+		.name		= KBUILD_MODNAME,
+		.of_match_table	= tlc5925_dt_ids,
+	},
+	.id_table = tlc5925_id,
+	.probe = tlc5925_probe,
+};
+
+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

[PATCH v3 3/3] leds: tlc5925: Add support for non blocking operations

[Jean-Jacques Hiblot]

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 2c50ba10bdbf..943514d2c459 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

Re: [PATCH v3 3/3] leds: tlc5925: Add support for non blocking operations

[Andy Shevchenko]

On Thu, Jun 9, 2022 at 6:29 PM Jean-Jacques Hiblot
<jjhiblot@traphandler.com> wrote:
>
> 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()).

i.e.

> +#define BITS_PER_ATOMIC (sizeof(atomic_t) * 8)

We have BITS_PER_TYPE(). Use it directly in the code, no need for a
whole new macro.

...

> +static int xmit(struct tlc5925_leds_priv *priv)
> +{
> +       int i;
> +
> +       spin_lock(&priv->lock);

This can't be called during IRQ?

> +       for (i = 0; i < priv->max_state / (sizeof(atomic_t) * 8) ; i++)

BITS_PER_TYPE() ?

> +               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);

One line?

Missed blank line here.

> +       xmit(priv);
> +};

...

>         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]);

The whole bunch looks like reinventing the bitmap / bitops.
Use unsigned long (or DECLARE_BITMAP() if it can be higher than 32)
for state and set_bit() / clear_bit() / assign_bit() that are atomic.

...

> +       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]);

assign_bit()

...

> +       // 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;

devm_bitmap_zalloc() ?

...

> +       // Allocate a second buffer for the communication on the SPI bus
> +       priv->spi_buffer = devm_kzalloc(dev,
> +                                  priv->max_state / 8,
> +                                  GFP_KERNEL);

Not sure I understand the output, but perhaps here the BITS_TO_BYTES()
should be used.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/3] leds: Add driver for the TLC5925 LED controller

[Andy Shevchenko]

On Thu, Jun 9, 2022 at 6:30 PM Jean-Jacques Hiblot
<jjhiblot@traphandler.com> wrote:
>
> 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.

Can you add Datasheet: tag here with the corresponding URL? Rationale
is to get a link to the datasheet by just browsing Git log without
browsing the source code, which will benefit via Web UIs.
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>

...

> +#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>

Keep it sorted?

...

> +struct single_led_priv {
> +       int idx;
> +       struct led_classdev cdev;

For pointer arithmetics it's better to swap these two members.

> +};
> +
> +struct tlc5925_leds_priv {
> +       int max_num_leds;
> +       u8 *state;

unsigned long? DECLARE_BITMAP() ?

> +       spinlock_t lock;
> +       struct single_led_priv leds[];
> +};

...

> +       if (brightness)
> +               priv->state[index / 8] |= (1 << (index % 8));
> +       else
> +               priv->state[index / 8] &= ~(1 << (index % 8));

assign_bit()

...

> +       return spi_write(spi, priv->state, priv->max_num_leds / 8);

BITS_TO_BYTES() ?

...

> +       count = device_get_child_node_count(dev);
> +       if (!count) {
> +               dev_err(dev, "no led defined.\n");
> +               return -ENODEV;

  return dev_err_probe(...);

here and everywhere in ->probe() and Co.

> +       }

...

> +       ret = device_property_read_u32_array(dev, "ti,shift-register-length",
> +                                            &max_num_leds, 1);

Always an array of 1 element? call device_property_read_u32().

...

> +       if (max_num_leds % 8) {
> +               dev_err(dev, "'ti,shift-register-length' must be a multiple of 8\n");
> +               return -EINVAL;
> +       }

Is this really fatal? I would rather issue a warning and go on if it
has at least 8 there. So the idea is to use a minimum that holds
multiple of 8.

...

> +       /* 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);
> +       }

You have to use dev_err_probe() here, otherwise it will spam logs a
lot in case of deferred probe.

...

> +       priv->state = devm_kzalloc(dev, DIV_ROUND_UP(max_num_leds, 8), GFP_KERNEL);

devm_bitmap_zalloc()

...

> +       device_for_each_child_node(dev, child) {
> +               struct led_init_data init_data = {.fwnode = child};

Missed spaces.

> +               struct led_classdev *cdev;
> +               u32 idx;
> +
> +               ret = fwnode_property_read_u32_array(child, "reg", &idx, 1);

fwnode_property_read_u32()

> +               if (ret || idx >= max_num_leds) {
> +                       dev_err(dev, "%s: invalid reg value. Ignoring.\n",
> +                               fwnode_get_name(child));
> +                       fwnode_handle_put(child);
> +                       continue;

Either dev_warn + continue, or dev_err + return dev_err_probe().

> +               }
> +
> +               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) {

Ditto.

> +                       dev_err(dev, "%s: cannot create LED device.\n",
> +                               fwnode_get_name(child));
> +                       fwnode_handle_put(child);
> +                       continue;
> +               }
> +       }

...

> +static const struct of_device_id tlc5925_dt_ids[] = {
> +       { .compatible = "ti,tlc5925", },
> +       {},

No comma for terminator entry.

> +};

Where is the MODULE_DEVICE_TABLE() for this one?

...

> +

No  need for this blank line.

> +module_spi_driver(tlc5925_driver);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/3] leds: Add driver for the TLC5925 LED controller

[Jean-Jacques Hiblot]

On 09/06/2022 18:57, Andy Shevchenko wrote:
> On Thu, Jun 9, 2022 at 6:30 PM Jean-Jacques Hiblot
> <jjhiblot@traphandler.com> wrote:
>> 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.
> Can you add Datasheet: tag here with the corresponding URL? Rationale
> is to get a link to the datasheet by just browsing Git log without
> browsing the source code, which will benefit via Web UIs.
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
> ...
>
>> +#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>
> Keep it sorted?
>
> ...
>
>> +struct single_led_priv {
>> +       int idx;
>> +       struct led_classdev cdev;
> For pointer arithmetics it's better to swap these two members.
>
>> +};
>> +
>> +struct tlc5925_leds_priv {
>> +       int max_num_leds;
>> +       u8 *state;
> unsigned long? DECLARE_BITMAP() ?
>
>> +       spinlock_t lock;
>> +       struct single_led_priv leds[];
>> +};
> ...
>
>> +       if (brightness)
>> +               priv->state[index / 8] |= (1 << (index % 8));
>> +       else
>> +               priv->state[index / 8] &= ~(1 << (index % 8));
> assign_bit()
>
> ...
>
>> +       return spi_write(spi, priv->state, priv->max_num_leds / 8);
> BITS_TO_BYTES() ?
>
> ...
>
>> +       count = device_get_child_node_count(dev);
>> +       if (!count) {
>> +               dev_err(dev, "no led defined.\n");
>> +               return -ENODEV;
>    return dev_err_probe(...);
>
> here and everywhere in ->probe() and Co.
>
>> +       }
> ...
>
>> +       ret = device_property_read_u32_array(dev, "ti,shift-register-length",
>> +                                            &max_num_leds, 1);
> Always an array of 1 element? call device_property_read_u32().
>
> ...
>
>> +       if (max_num_leds % 8) {
>> +               dev_err(dev, "'ti,shift-register-length' must be a multiple of 8\n");
>> +               return -EINVAL;
>> +       }
> Is this really fatal? I would rather issue a warning and go on if it
> has at least 8 there. So the idea is to use a minimum that holds
> multiple of 8.
It is more than likely that it will always be a multiple of 8 here.

We could in theory use a non-multiple of 8 (some LEDs of the first or 
last chips of the chain are not populated).

I didn't think it would add a significant benefit in memory usage. In 
terms of SPI usage it wouldn't change anything.


Thanks for your feedback.

> ...
>
>> +       /* 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);
>> +       }
> You have to use dev_err_probe() here, otherwise it will spam logs a
> lot in case of deferred probe.
>
> ...
>
>> +       priv->state = devm_kzalloc(dev, DIV_ROUND_UP(max_num_leds, 8), GFP_KERNEL);
> devm_bitmap_zalloc()
>
> ...
>
>> +       device_for_each_child_node(dev, child) {
>> +               struct led_init_data init_data = {.fwnode = child};
> Missed spaces.
>
>> +               struct led_classdev *cdev;
>> +               u32 idx;
>> +
>> +               ret = fwnode_property_read_u32_array(child, "reg", &idx, 1);
> fwnode_property_read_u32()
>
>> +               if (ret || idx >= max_num_leds) {
>> +                       dev_err(dev, "%s: invalid reg value. Ignoring.\n",
>> +                               fwnode_get_name(child));
>> +                       fwnode_handle_put(child);
>> +                       continue;
> Either dev_warn + continue, or dev_err + return dev_err_probe().
>
>> +               }
>> +
>> +               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) {
> Ditto.
>
>> +                       dev_err(dev, "%s: cannot create LED device.\n",
>> +                               fwnode_get_name(child));
>> +                       fwnode_handle_put(child);
>> +                       continue;
>> +               }
>> +       }
> ...
>
>> +static const struct of_device_id tlc5925_dt_ids[] = {
>> +       { .compatible = "ti,tlc5925", },
>> +       {},
> No comma for terminator entry.
>
>> +};
> Where is the MODULE_DEVICE_TABLE() for this one?
>
> ...
>
>> +
> No  need for this blank line.
>
>> +module_spi_driver(tlc5925_driver);

Re: [PATCH v3 3/3] leds: tlc5925: Add support for non blocking operations

[Jean-Jacques Hiblot]

On 09/06/2022 18:43, Andy Shevchenko wrote:
> On Thu, Jun 9, 2022 at 6:29 PM Jean-Jacques Hiblot
> <jjhiblot@traphandler.com> wrote:
>> 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()).
> i.e.
>
>> +#define BITS_PER_ATOMIC (sizeof(atomic_t) * 8)
> We have BITS_PER_TYPE(). Use it directly in the code, no need for a
> whole new macro.
>
> ...
>
>> +static int xmit(struct tlc5925_leds_priv *priv)
>> +{
>> +       int i;
>> +
>> +       spin_lock(&priv->lock);
> This can't be called during IRQ?
>
>> +       for (i = 0; i < priv->max_state / (sizeof(atomic_t) * 8) ; i++)
> BITS_PER_TYPE() ?
>
>> +               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);
> One line?
>
> Missed blank line here.
>
>> +       xmit(priv);
>> +};
> ...
>
>>          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]);
> The whole bunch looks like reinventing the bitmap / bitops.
> Use unsigned long (or DECLARE_BITMAP() if it can be higher than 32)
> for state and set_bit() / clear_bit() / assign_bit() that are atomic.

Thanks for pointing this out.

It will drastically simplify the code.

>
> ...
>
>> +       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]);
> assign_bit()
>
> ...
>
>> +       // 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;
> devm_bitmap_zalloc() ?
>
> ...
>
>> +       // Allocate a second buffer for the communication on the SPI bus
>> +       priv->spi_buffer = devm_kzalloc(dev,
>> +                                  priv->max_state / 8,
>> +                                  GFP_KERNEL);
> Not sure I understand the output, but perhaps here the BITS_TO_BYTES()
> should be used.
>

Re: [PATCH v3 2/3] leds: Add driver for the TLC5925 LED controller

[Pavel Machek]

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

On Thu 2022-06-09 18:57:24, Andy Shevchenko wrote:
> On Thu, Jun 9, 2022 at 6:30 PM Jean-Jacques Hiblot
> <jjhiblot@traphandler.com> wrote:
> >
> > 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.
> 
> Can you add Datasheet: tag here with the corresponding URL? Rationale
> is to get a link to the datasheet by just browsing Git log without
> browsing the source code, which will benefit via Web UIs.

If you want to add datasheet url, add it as a comment to the source,
not to the git log.

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

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

Re: [PATCH v3 2/3] leds: Add driver for the TLC5925 LED controller

[Andy Shevchenko]

On Mon, Jun 27, 2022 at 10:49 AM Pavel Machek <pavel@ucw.cz> wrote:
>
> On Thu 2022-06-09 18:57:24, Andy Shevchenko wrote:
> > On Thu, Jun 9, 2022 at 6:30 PM Jean-Jacques Hiblot
> > <jjhiblot@traphandler.com> wrote:
> > >
> > > 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.
> >
> > Can you add Datasheet: tag here with the corresponding URL? Rationale
> > is to get a link to the datasheet by just browsing Git log without
> > browsing the source code, which will benefit via Web UIs.
>
> If you want to add datasheet url, add it as a comment to the source,
> not to the git log.

I don't see anything wrong with having it in the Git log. Do you?
(Note, I'm not objecting to have it in the code at the same time)

P.S. Can you review the three patches of the series [1] that have been
submitted day 1 after closing the merge window? It's already a few
weeks passed, or even months if you take into account that the top of
that series has been sent before separately.

[1]: https://lore.kernel.org/linux-leds/20220606164138.66535-1-andriy.shevchenko@linux.intel.com/

-- 
With Best Regards,
Andy Shevchenko