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

The TLC5925 is a 16-channels constant-current LED sink driver.
It is controlled via SPI but doesn't offer a register-based interface.
Instead it contains a shift register and latches that convert the
serial input into a parallel output.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
---
 drivers/leds/Kconfig        |   6 ++
 drivers/leds/Makefile       |   1 +
 drivers/leds/leds-tlc5925.c | 164 ++++++++++++++++++++++++++++++++++++
 3 files changed, 171 insertions(+)
 create mode 100644 drivers/leds/leds-tlc5925.c

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

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

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


Thanks for your feedback.

> ...
>
>> +       /* Assert all the OE/ lines */
>> +       gpios = devm_gpiod_get_array(dev, "output-enable-b", GPIOD_OUT_LOW);
>> +       if (IS_ERR(gpios)) {
>> +               dev_err(dev, "Unable to get the 'output-enable-b' gpios\n");
>> +               return PTR_ERR(gpios);
>> +       }
> You have to use dev_err_probe() here, otherwise it will spam logs a
> lot in case of deferred probe.
>
> ...
>
>> +       priv->state = devm_kzalloc(dev, DIV_ROUND_UP(max_num_leds, 8), GFP_KERNEL);
> devm_bitmap_zalloc()
>
> ...
>
>> +       device_for_each_child_node(dev, child) {
>> +               struct led_init_data init_data = {.fwnode = child};
> Missed spaces.
>
>> +               struct led_classdev *cdev;
>> +               u32 idx;
>> +
>> +               ret = fwnode_property_read_u32_array(child, "reg", &idx, 1);
> fwnode_property_read_u32()
>
>> +               if (ret || idx >= max_num_leds) {
>> +                       dev_err(dev, "%s: invalid reg value. Ignoring.\n",
>> +                               fwnode_get_name(child));
>> +                       fwnode_handle_put(child);
>> +                       continue;
> Either dev_warn + continue, or dev_err + return dev_err_probe().
>
>> +               }
>> +
>> +               count--;
>> +               priv->leds[count].idx = idx;
>> +               cdev = &(priv->leds[count].cdev);
>> +               cdev->brightness = LED_OFF;
>> +               cdev->max_brightness = 1;
>> +               cdev->brightness_set_blocking = tlc5925_brightness_set_blocking;
>> +
>> +               ret = devm_led_classdev_register_ext(dev, cdev, &init_data);
>> +               if (ret) {
> Ditto.
>
>> +                       dev_err(dev, "%s: cannot create LED device.\n",
>> +                               fwnode_get_name(child));
>> +                       fwnode_handle_put(child);
>> +                       continue;
>> +               }
>> +       }
> ...
>
>> +static const struct of_device_id tlc5925_dt_ids[] = {
>> +       { .compatible = "ti,tlc5925", },
>> +       {},
> No comma for terminator entry.
>
>> +};
> Where is the MODULE_DEVICE_TABLE() for this one?
>
> ...
>
>> +
> No  need for this blank line.
>
>> +module_spi_driver(tlc5925_driver);

On Thu 2022-06-09 18:57:24, Andy Shevchenko wrote:
> On Thu, Jun 9, 2022 at 6:30 PM Jean-Jacques Hiblot
> <jjhiblot@traphandler.com> wrote:
> >
> > The TLC5925 is a 16-channels constant-current LED sink driver.
> > It is controlled via SPI but doesn't offer a register-based interface.
> > Instead it contains a shift register and latches that convert the
> > serial input into a parallel output.
> 
> Can you add Datasheet: tag here with the corresponding URL? Rationale
> is to get a link to the datasheet by just browsing Git log without
> browsing the source code, which will benefit via Web UIs.

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

Thanks,
							Pavel

On Mon, Jun 27, 2022 at 10:49 AM Pavel Machek <pavel@ucw.cz> wrote:
>
> On Thu 2022-06-09 18:57:24, Andy Shevchenko wrote:
> > On Thu, Jun 9, 2022 at 6:30 PM Jean-Jacques Hiblot
> > <jjhiblot@traphandler.com> wrote:
> > >
> > > The TLC5925 is a 16-channels constant-current LED sink driver.
> > > It is controlled via SPI but doesn't offer a register-based interface.
> > > Instead it contains a shift register and latches that convert the
> > > serial input into a parallel output.
> >
> > Can you add Datasheet: tag here with the corresponding URL? Rationale
> > is to get a link to the datasheet by just browsing Git log without
> > browsing the source code, which will benefit via Web UIs.
>
> If you want to add datasheet url, add it as a comment to the source,
> not to the git log.

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

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

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