| Message ID | 20220722081146.47262-1-jjhiblot@traphandler.com |
|---|---|
| Headers | show |
| Series | Add support for the TLC5925 | expand |
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
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