Message ID | 20220722081146.47262-3-jjhiblot@traphandler.com |
---|---|
State | New |
Headers | show |
Series | Add support for the TLC5925 | expand |
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
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?
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
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!
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
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 >
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.
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. >
+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.
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 --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");