diff mbox series

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

Message ID 20220722081146.47262-3-jjhiblot@traphandler.com
State New
Headers show
Series Add support for the TLC5925 | expand

Commit Message

Jean-Jacques Hiblot July 22, 2022, 8:11 a.m. UTC
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

Comments

Pavel Machek July 30, 2022, 9:27 p.m. UTC | #1
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
Andy Shevchenko July 31, 2022, 7:28 p.m. UTC | #2
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?
Jean-Jacques Hiblot Aug. 4, 2022, 8:23 p.m. UTC | #3
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
Andy Shevchenko Aug. 4, 2022, 8:40 p.m. UTC | #4
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!
Pavel Machek Aug. 4, 2022, 9:04 p.m. UTC | #5
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
Jean-Jacques Hiblot Aug. 24, 2022, 8:39 a.m. UTC | #6
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
>
Andy Shevchenko Aug. 24, 2022, 8:55 a.m. UTC | #7
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.
Jean-Jacques Hiblot Aug. 24, 2022, 9:58 a.m. UTC | #8
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.
>
Andy Shevchenko Aug. 24, 2022, 10:18 a.m. UTC | #9
+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.
Linus Walleij Aug. 26, 2022, 9:11 a.m. UTC | #10
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
diff mbox series

Patch

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