[RESEND PATCH v6 0/3] Add support for the TLC5925

[RESEND PATCH v6 0/3] Add support for the TLC5925

[Jean-Jacques Hiblot]

Resending the v6 of the series.

Pavel, do you have any comment/suggestion on this series ?

Thanks,

JJ


Original cover-letter:

This series adds the support for the TLC5925 LED controller.
This LED controller is driven through 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 v5->v6:
 * Make the 'ti,shif-register-length' property optional. The default
   value is 16: the length of the shift register of a single TLC5925.
 * fix minor issues in the binding description

 changes v4->v5:
 * add the headers that the code is a direct user of
 * replace dev_err() with dev_err_probe() in ->probe()
 * comestic changes (spaces, alignment and blank lines)

changes v3->v4:
 * add missing MODULE_DEVICE_TABLE(of, ...) 
 * use dev_err_probe() to avoid spamming the log in case of deferred probe
 * use bitmap ops instead of direct memory access + lock
 * sort headers and a few other cosmetic changes.

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  | 105 ++++++++++
 drivers/leds/Kconfig                          |   6 +
 drivers/leds/Makefile                         |   1 +
 drivers/leds/leds-tlc5925.c                   | 182 ++++++++++++++++++
 4 files changed, 294 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/ti,tlc5925.yaml
 create mode 100644 drivers/leds/leds-tlc5925.c

-- 
2.25.1

[RESEND PATCH v6 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>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/leds/ti,tlc5925.yaml  | 105 ++++++++++++++++++
 1 file changed, 105 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..ce376917b423
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/ti,tlc5925.yaml
@@ -0,0 +1,105 @@
+# 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
+    multipleOf: 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:
+    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#
+    unevaluatedProperties: false
+    properties:
+      reg:
+        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"
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/leds/common.h>
+
+    spi {
+        #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

[RESEND PATCH v6 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.

Datasheet: https://www.ti.com/lit/ds/symlink/tlc5925.pdf
Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/leds/Kconfig        |   6 ++
 drivers/leds/Makefile       |   1 +
 drivers/leds/leds-tlc5925.c | 148 ++++++++++++++++++++++++++++++++++++
 3 files changed, 155 insertions(+)
 create mode 100644 drivers/leds/leds-tlc5925.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index eaba0a8347fa..197d2e4b6c72 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..797836354c74
--- /dev/null
+++ b/drivers/leds/leds-tlc5925.c
@@ -0,0 +1,148 @@
+// 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/container_of.h>
+#include <linux/err.h>
+#include <linux/gpio/consumer.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/spi/spi.h>
+#include <linux/types.h>
+
+#define TLC5925_SHIFT_REGISTER_LENGTH 16
+
+struct single_led_priv {
+	struct led_classdev cdev;
+	int idx;
+};
+
+struct tlc5925_leds_priv {
+	int max_num_leds;
+	unsigned long *state;
+	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;
+
+	assign_bit(index, priv->state, !!brightness);
+
+	return spi_write(spi, priv->state, BITS_TO_BYTES(priv->max_num_leds));
+}
+
+static int tlc5925_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct fwnode_handle *child;
+	struct tlc5925_leds_priv *priv;
+	int count;
+	int max_num_leds = TLC5925_SHIFT_REGISTER_LENGTH;
+	struct gpio_descs *gpios;
+
+	/* Assert all the OE/ lines */
+	gpios = devm_gpiod_get_array(dev, "output-enable-b", GPIOD_OUT_LOW);
+	if (IS_ERR(gpios))
+		return dev_err_probe(dev, PTR_ERR(gpios),
+			      "Unable to get the 'output-enable-b' gpios\n");
+
+	count = device_get_child_node_count(dev);
+	if (!count)
+		return dev_err_probe(dev, -ENODEV, "no led defined.\n");
+
+	device_property_read_u32(dev, "ti,shift-register-length",
+				 &max_num_leds);
+
+	if (max_num_leds % 8)
+		return dev_err_probe(dev, -EINVAL,
+				     "'ti,shift-register-length' must be a multiple of 8\n");
+	if (max_num_leds == 0)
+		return dev_err_probe(dev, -EINVAL,
+				     "'ti,shift-register-length' must be greater than 0\n");
+
+	priv = devm_kzalloc(dev, struct_size(priv, leds, count), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->state = devm_bitmap_zalloc(dev, max_num_leds, 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;
+		int ret;
+
+		ret = fwnode_property_read_u32(child, "reg", &idx);
+		if (ret || idx >= max_num_leds) {
+			dev_warn(dev, "%pfwP: invalid reg value. Ignoring.\n",
+				 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_warn(dev, "%pfwP: cannot create LED device.\n",
+				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", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, tlc5925_dt_ids);
+
+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

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

[Pavel Machek]

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

Hi!

> 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.
> 
> Datasheet: https://www.ti.com/lit/ds/symlink/tlc5925.pdf
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> ---
>  drivers/leds/Kconfig        |   6 ++
>  drivers/leds/Makefile       |   1 +
>  drivers/leds/leds-tlc5925.c | 148 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 155 insertions(+)

Lets make this go to drivers/leds/simple/ ?


> +	gpios = devm_gpiod_get_array(dev, "output-enable-b", GPIOD_OUT_LOW);
> +	if (IS_ERR(gpios))
> +		return dev_err_probe(dev, PTR_ERR(gpios),
> +			      "Unable to get the 'output-enable-b' gpios\n");
> +
> +	count = device_get_child_node_count(dev);
> +	if (!count)
> +		return dev_err_probe(dev, -ENODEV, "no led defined.\n");

"No LED..."

> +	device_property_read_u32(dev, "ti,shift-register-length",
> +				 &max_num_leds);
> +
> +	if (max_num_leds % 8)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "'ti,shift-register-length' must be a multiple of 8\n");
> +	if (max_num_leds == 0)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "'ti,shift-register-length' must be greater than 0\n");
> +

I thought you had #define for leds number before?

Otherwise looks good, thank you.

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

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

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

[Andy Shevchenko]

On Fri, Jul 22, 2022 at 10:14 AM 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.
>
> Datasheet: https://www.ti.com/lit/ds/symlink/tlc5925.pdf
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Sorry for my slowpokeness, but I just realized that this driver may
not be needed. What is the difference to existing gpio-74x164?

-- 
With Best Regards,
Andy Shevchenko

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

[Jean-Jacques Hiblot]

On 31/07/2022 21:28, Andy Shevchenko wrote:
> On Fri, Jul 22, 2022 at 10:14 AM 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.
>>
>> Datasheet: https://www.ti.com/lit/ds/symlink/tlc5925.pdf
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sorry for my slowpokeness, but I just realized that this driver may
> not be needed. What is the difference to existing gpio-74x164?

It might work. However it might not be as practical and efficient as the 
dedicated LED driver.

I'll give a try.

JJ

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

[Andy Shevchenko]

On Thu, Aug 4, 2022 at 10:23 PM Jean-Jacques Hiblot
<jjhiblot@traphandler.com> wrote:
> On 31/07/2022 21:28, Andy Shevchenko wrote:
> > On Fri, Jul 22, 2022 at 10:14 AM 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.
> >>
> >> Datasheet: https://www.ti.com/lit/ds/symlink/tlc5925.pdf
> >> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
> >> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Sorry for my slowpokeness, but I just realized that this driver may
> > not be needed. What is the difference to existing gpio-74x164?
>
> It might work. However it might not be as practical and efficient as the
> dedicated LED driver.

I'm not sure what efficiency you are talking about? Using LED GPIO +
GPIO? But the chip, even if marketed as an LED driver, might be used
not for LEDs, right? Technically speaking it's a GPIO with powerful
(by current sink) outputs. With LED + GPIO you get flexibility to
configure and describe your system exactly how it is designed on the
PCB level.

Note, we have already examples of other chips (like PWM) being used as
GPIO. In different cases in Linux we have different approaches on how
to solve that. In general, Linux kernel pin control misses some
special functions of the pins that may be assigned to them, while
being a simple pin control (or even GPIO expander).

> I'll give a try.

Thanks!

-- 
With Best Regards,
Andy Shevchenko

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

[Pavel Machek]

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

On Thu 2022-08-04 22:23:00, Jean-Jacques Hiblot wrote:
> On 31/07/2022 21:28, Andy Shevchenko wrote:
> > On Fri, Jul 22, 2022 at 10:14 AM 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.
> > > 
> > > Datasheet: https://www.ti.com/lit/ds/symlink/tlc5925.pdf
> > > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
> > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Sorry for my slowpokeness, but I just realized that this driver may
> > not be needed. What is the difference to existing gpio-74x164?
> 
> It might work. However it might not be as practical and efficient as the
> dedicated LED driver.
> 
> I'll give a try.

It is certainly preffered solution. If you decide to re-submit the
driver anyway, please mention that we already have GPIO driver for
compatible chip, and explain why this is superior.

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: [RESEND PATCH v6 2/3] leds: Add driver for the TLC5925 LED controller

[Jean-Jacques Hiblot]

On 04/08/2022 23:04, Pavel Machek wrote:
> On Thu 2022-08-04 22:23:00, Jean-Jacques Hiblot wrote:
>> On 31/07/2022 21:28, Andy Shevchenko wrote:
>>> On Fri, Jul 22, 2022 at 10:14 AM 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.
>>>>
>>>> Datasheet: https://www.ti.com/lit/ds/symlink/tlc5925.pdf
>>>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
>>>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>>> Sorry for my slowpokeness, but I just realized that this driver may
>>> not be needed. What is the difference to existing gpio-74x164?
>> It might work. However it might not be as practical and efficient as the
>> dedicated LED driver.
>>
>> I'll give a try.
> It is certainly preffered solution. If you decide to re-submit the
> driver anyway, please mention that we already have GPIO driver for
> compatible chip, and explain why this is superior.

Hi all,

sorry for the delay. I tried with the  74x164 gpio driver and it works 
as expected.

The only drawbacks are:

- as-is the 74x164 gpio driver supports only one output-enable gpio. 
However in practice I don't think multiple OE GPIOs will ever be used.

- with this approach, every time a LED status is changed the whole 
register has to be sent on the SPI bus. In other words, changes cannot 
be coalesced.


I don't know if this is enough to make a dedicated TLC5925 driver 
desirable in the kernel.

JJ

>
> Thanks,
> 								Pavel
>

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

[Andy Shevchenko]

On Wed, Aug 24, 2022 at 11:39 AM Jean-Jacques Hiblot
<jjhiblot@traphandler.com> wrote:
> On 04/08/2022 23:04, Pavel Machek wrote:
> > On Thu 2022-08-04 22:23:00, Jean-Jacques Hiblot wrote:
> >> On 31/07/2022 21:28, Andy Shevchenko wrote:
> >>> On Fri, Jul 22, 2022 at 10:14 AM Jean-Jacques Hiblot
> >>> <jjhiblot@traphandler.com> wrote:

...

> >>> Sorry for my slowpokeness, but I just realized that this driver may
> >>> not be needed. What is the difference to existing gpio-74x164?
> >> It might work. However it might not be as practical and efficient as the
> >> dedicated LED driver.
> >>
> >> I'll give a try.
> > It is certainly preffered solution. If you decide to re-submit the
> > driver anyway, please mention that we already have GPIO driver for
> > compatible chip, and explain why this is superior.

> sorry for the delay. I tried with the  74x164 gpio driver and it works
> as expected.
>
> The only drawbacks are:
>
> - as-is the 74x164 gpio driver supports only one output-enable gpio.
> However in practice I don't think multiple OE GPIOs will ever be used.

Let's leave it to the case when it will be needed. So, we can skip this point.

> - with this approach, every time a LED status is changed the whole
> register has to be sent on the SPI bus. In other words, changes cannot
> be coalesced.

But isn't it the same as what you do in your driver? To me it looks
like you send the entire range of the values each time you change one
LED's brightness. I don't see any differences with the GPIO driver.

> I don't know if this is enough to make a dedicated TLC5925 driver
> desirable in the kernel.

I don't think you have enough justification for a new driver.

-- 
With Best Regards,
Andy Shevchenko

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

[Jean-Jacques Hiblot]

On 24/08/2022 10:55, Andy Shevchenko wrote:
> On Wed, Aug 24, 2022 at 11:39 AM Jean-Jacques Hiblot
> <jjhiblot@traphandler.com> wrote:
>> On 04/08/2022 23:04, Pavel Machek wrote:
>>> On Thu 2022-08-04 22:23:00, Jean-Jacques Hiblot wrote:
>>>> On 31/07/2022 21:28, Andy Shevchenko wrote:
>>>>> On Fri, Jul 22, 2022 at 10:14 AM Jean-Jacques Hiblot
>>>>> <jjhiblot@traphandler.com> wrote:
> ...
>
>>>>> Sorry for my slowpokeness, but I just realized that this driver may
>>>>> not be needed. What is the difference to existing gpio-74x164?
>>>> It might work. However it might not be as practical and efficient as the
>>>> dedicated LED driver.
>>>>
>>>> I'll give a try.
>>> It is certainly preffered solution. If you decide to re-submit the
>>> driver anyway, please mention that we already have GPIO driver for
>>> compatible chip, and explain why this is superior.
>> sorry for the delay. I tried with the  74x164 gpio driver and it works
>> as expected.
>>
>> The only drawbacks are:
>>
>> - as-is the 74x164 gpio driver supports only one output-enable gpio.
>> However in practice I don't think multiple OE GPIOs will ever be used.
> Let's leave it to the case when it will be needed. So, we can skip this point.
>
>> - with this approach, every time a LED status is changed the whole
>> register has to be sent on the SPI bus. In other words, changes cannot
>> be coalesced.
> But isn't it the same as what you do in your driver? To me it looks
> like you send the entire range of the values each time you change one
> LED's brightness. I don't see any differences with the GPIO driver.
No. The TLC5925 driver updates the register asynchronously: the cached 
value of the register is updated synchronously and then it is 
transferred over SPI using a workqueue. This way if multiple LED are set 
in a short time, the changes are coalesced into a single SPI transfer. 
This is however probably not a must-have feature.
>
>> I don't know if this is enough to make a dedicated TLC5925 driver
>> desirable in the kernel.
> I don't think you have enough justification for a new driver.
>

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

[Andy Shevchenko]

+Cc: GPIO maintainers

On Wed, Aug 24, 2022 at 12:58 PM Jean-Jacques Hiblot
<jjhiblot@traphandler.com> wrote:
> On 24/08/2022 10:55, Andy Shevchenko wrote:
> > On Wed, Aug 24, 2022 at 11:39 AM Jean-Jacques Hiblot
> > <jjhiblot@traphandler.com> wrote:

...

> >> - with this approach, every time a LED status is changed the whole
> >> register has to be sent on the SPI bus. In other words, changes cannot
> >> be coalesced.
> > But isn't it the same as what you do in your driver? To me it looks
> > like you send the entire range of the values each time you change one
> > LED's brightness. I don't see any differences with the GPIO driver.
> No. The TLC5925 driver updates the register asynchronously: the cached
> value of the register is updated synchronously and then it is
> transferred over SPI using a workqueue. This way if multiple LED are set
> in a short time, the changes are coalesced into a single SPI transfer.
> This is however probably not a must-have feature.

Ah, thanks for elaboration. But GPIO supports this type of ops.

And the more I think about this feature I find it more harmful than
useful. The problem is that delayed operation may take an
unpredictable amount of time and on the heavily loaded machine the
event might be lost (imagine the blinking LED informing about some
critical issue and it blinks only once and then, for example, machine
reboots or so). I believe we both understand that for the GPIO is a
no-go feature for sure, because sequence of the GPIO signals is highly
important (imagine bit-banging of any of the protocols).

> >> I don't know if this is enough to make a dedicated TLC5925 driver
> >> desirable in the kernel.
> > I don't think you have enough justification for a new driver.

After this message I first must withdraw my Rb tag, and turn my voice
against this driver because of the above. On the contrary we might ask
the GPIO library for a specific API to have what you do with the
user's consent of side effects. Linus, Bart, I'm talking of the
delayed (async) version of gpio_set_multiple(). But personally I think
it's not so easy to implement in a bugless manner (because we need to
synchronize it forcibly at any time we call another GPIO API against
the same chip).

Summarize this:
 - you may add a compatible string to the GPIO driver and DT schema,
and we are done.

-- 
With Best Regards,
Andy Shevchenko

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

[Linus Walleij]

On Wed, Aug 24, 2022 at 12:19 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:

> > >> I don't know if this is enough to make a dedicated TLC5925 driver
> > >> desirable in the kernel.
> > > I don't think you have enough justification for a new driver.

One thing to keep in mind is that LEDs are MMI (man-machine-interface)
and designed as such, so small glitches etc are fine as long as they are
not noticeable by human perception...

> After this message I first must withdraw my Rb tag, and turn my voice
> against this driver because of the above. On the contrary we might ask
> the GPIO library for a specific API to have what you do with the
> user's consent of side effects. Linus, Bart, I'm talking of the
> delayed (async) version of gpio_set_multiple(). But personally I think
> it's not so easy to implement in a bugless manner (because we need to
> synchronize it forcibly at any time we call another GPIO API against
> the same chip).

I suppose this can just be a gpio-led using the GPIO driver
underneath?

If the usecase for TLC5925 is such that it is often (as defined by
experienced developers having seen it on boards in the wild) used
as a GPIO expander rather than a pure LED driver, then it is better
to have this in the GPIO subsystem in some or other form.

If it is always just used for LEDs then my first comment about
this being MMI applies I suppose. Or rather, ask the question
from an operator point of view rather than a logic level point of
view. (I think that was Andy's point though.)

I agree that we probably need some generic library to properly handle
the jungle of funny TTL-type constructs that is popping up left and
right for GPIO. Someone should ideally sit down and think about
what is common among these.

Yours,
Linus Walleij

[RESEND PATCH v6 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>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/leds/leds-tlc5925.c | 38 +++++++++++++++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/leds-tlc5925.c b/drivers/leds/leds-tlc5925.c
index 797836354c74..0423e3592bd7 100644
--- a/drivers/leds/leds-tlc5925.c
+++ b/drivers/leds/leds-tlc5925.c
@@ -18,6 +18,7 @@
 #include <linux/property.h>
 #include <linux/spi/spi.h>
 #include <linux/types.h>
+#include <linux/workqueue.h>
 
 #define TLC5925_SHIFT_REGISTER_LENGTH 16
 
@@ -29,10 +30,25 @@ struct single_led_priv {
 struct tlc5925_leds_priv {
 	int max_num_leds;
 	unsigned long *state;
+	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)
+{
+	return spi_write(priv->spi, priv->state, BITS_TO_BYTES(priv->max_num_leds));
+}
+
+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);
@@ -43,9 +59,23 @@ static int tlc5925_brightness_set_blocking(struct led_classdev *cdev,
 
 	assign_bit(index, priv->state, !!brightness);
 
-	return spi_write(spi, priv->state, BITS_TO_BYTES(priv->max_num_leds));
+	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;
+
+	assign_bit(index, priv->state, !!brightness);
+
+	cancel_work_sync(&priv->xmit_work);
+	return xmit(priv);
+}
 static int tlc5925_probe(struct spi_device *spi)
 {
 	struct device *dev = &spi->dev;
@@ -83,6 +113,9 @@ static int tlc5925_probe(struct spi_device *spi)
 	if (!priv->state)
 		return -ENOMEM;
 
+	priv->spi = spi;
+	INIT_WORK(&priv->xmit_work, xmit_work);
+
 	priv->max_num_leds = max_num_leds;
 
 	device_for_each_child_node(dev, child) {
@@ -104,6 +137,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: [RESEND PATCH v6 3/3] leds: tlc5925: Add support for non blocking operations

[Pavel Machek]

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

On Fri 2022-07-22 10:11:46, Jean-Jacques Hiblot wrote:
> Settings multiple LEDs in a row can be a slow operation because of the

"Setting"

> 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()).

Why do this? We have other LEDs that are slow, and core already has
workqueues (etc) to deal with that...

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

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