Re: [PATCH 1/2] leds: tlc5928: Driver for the TI 16 Channel spi LED driver

[Corentin GUILLEVIC]

Hi,

Thank you for your review! I've fixed everything (new patch is
coming), but I have issue for some of them: I can't use the suggested
functions (guard(), for_each_available_child_of_node_scoped() and
devm_mutex_init()) because the kernel version used on my device is too
old (v5.15). No way to test with a newer version...

Should I let the "old" functions because of my kernel version?

>
> Le 26/03/2025 à 16:35, Corentin Guillevic a écrit :
> > The TLC59928 is an SPI-connected bus controlled 16-channel LED driver.
> > A single 16-bit register handles the whole LEDs. Following a write, a
> > latch GPIO applies the new LED configuration. An "enable" GPIO (blank
> > in the TLC59928 datasheet) turns off the whole LEDs when active/high.
> >
> > This driver is able to handle a daisy-chain case, so when several
> > TLC59928 controllers are connected in serie.
> >
> > Signed-off-by: Corentin Guillevic <corentin.guillevic@xxxxxxxx>
> > ---
>
> ...
>
> > +static int
> > +tlc5928_set_ledout(struct tlc5928_led *led, bool val)
> > +{
> > +     struct tlc5928_chip *chip;
> > +     struct tlc5928_chip *chip_owner = led->chip;
> > +     struct tlc5928_priv *priv = chip_owner->priv;
> > +     int ret;
> > +
> > +     mutex_lock(&priv->lock);
> > +
> > +     if (val)
> > +             chip_owner->leds_state |= (1 << led->led_no);
> > +     else
> > +             chip_owner->leds_state &= ~(1 << led->led_no);
> > +
> > +     list_for_each_entry_reverse(chip, &priv->chips_list, list) {
> > +             u16 leds_state = cpu_to_be16(chip->leds_state);
> > +
> > +             ret = spi_write(priv->spi, &(leds_state), sizeof(leds_state));
> > +
> > +             if (ret)
>
> Missing unlock.
> Or use guard()?
>

Fixed! But guard() is unavailable on my kernel.

> > +                     return ret;
> > +     }
> > +
> > +     gpiod_set_value(priv->latch_gpio, 0);
> > +     udelay(1);
> > +     gpiod_set_value(priv->latch_gpio, 1);
> > +
> > +     mutex_unlock(&priv->lock);
> > +
> > +     return 0;
> > +}
> > +
> > +static int
> > +tlc5928_brightness_set(struct led_classdev *led_cdev,
> > +                     enum led_brightness brightness)
> > +{
> > +     struct tlc5928_led *led = ldev_to_led(led_cdev);
> > +
> > +     /* TLC5928 only allows on/off, no brightness */
> > +     return tlc5928_set_ledout(led, !!brightness);
> > +}
> > +
> > +static const struct of_device_id of_tlc5928_leds_match[] __maybe_unused = {
> > +     { .compatible = "ti,tlc5928" },
> > +     {},
>
> Unneeded trailing ,
>
> > +};
> > +MODULE_DEVICE_TABLE(of, of_tlc5928_leds_match);
> > +
> > +static int tlc5928_probe_chip_dt(struct device *dev, struct device_node *node,
> > +             struct tlc5928_chip *chip)
> > +{
> > +     struct device_node *child;
> > +     int count, err, reg;
> > +
> > +     count = of_get_available_child_count(node);
> > +     if (!count)
> > +             return -EINVAL;
> > +
> > +     chip->leds_state = 0;
> > +
> > +     for_each_available_child_of_node(node, child) {
>
> for_each_available_child_of_node_scoped()?
>

Same, not defined because my kernel is too old.

> > +             struct tlc5928_led *led;
> > +             struct led_init_data init_data = {};
> > +
> > +             init_data.fwnode = of_fwnode_handle(child);
> > +
> > +             err = of_property_read_u32(child, "reg", &reg);
> > +             if (err) {
> > +                     dev_err(dev, "%pOF: failed to read reg\n", child);
> > +                     of_node_put(child);
> > +                     return err;
> > +             }
> > +
> > +             if (reg < 0 || reg >= TLC5928_MAX_LEDS ||
> > +                             chip->leds[reg].active) {
> > +                     of_node_put(child);
> > +                     return -EINVAL;
> > +             }
> > +
> > +             led = &chip->leds[reg];
> > +
> > +             led->active = true;
> > +             led->chip = chip;
> > +             led->led_no = reg;
> > +             led->ldev.brightness_set_blocking = tlc5928_brightness_set;
> > +             err = devm_led_classdev_register_ext(dev, &led->ldev,
> > +                                                      &init_data);
> > +             if (err < 0) {
> > +                     of_node_put(child);
> > +                     dev_err(dev, "Failed to register LED for node %pfw\n",
> > +                             init_data.fwnode);
> > +                     return err;
> > +             }
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int tlc5928_probe(struct spi_device *spi)
> > +{
> > +     struct device_node *node, *child;
> > +     struct device *dev = &spi->dev;
> > +     struct list_head *pos;
> > +     struct tlc5928_chip *chip;
> > +     struct tlc5928_priv *priv;
> > +     int count, err, i;
> > +
> > +     node = dev_of_node(dev);
> > +     if (!node)
> > +             return -ENODEV;
> > +
> > +     count = of_get_available_child_count(node);
> > +     if (!count)
> > +             return -EINVAL;
> > +
> > +     priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +     if (!priv)
> > +             return -ENOMEM;
> > +
> > +     priv->spi = spi;
> > +     priv->latch_gpio = devm_gpiod_get(dev, "latch", GPIOD_OUT_HIGH);
> > +     if (IS_ERR(priv->latch_gpio))
> > +             return dev_err_probe(dev, PTR_ERR(priv->latch_gpio),
> > +                                  "Failed to get latch GPIO\n");
> > +
> > +     mutex_init(&priv->lock);
>
> Maybe:
> err = devm_mutex_init(...);
> if (err)
>         return err;
>
> ?

Same.

>
> > +     INIT_LIST_HEAD(&priv->chips_list);
> > +
> > +     i = 0;
> > +     for_each_available_child_of_node(node, child) {
> > +             chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
> > +             if (!chip)
> > +                     return -ENOMEM;
> > +
> > +             list_add_tail(&chip->list, &priv->chips_list);
> > +             chip->priv = priv;
> > +             chip->enable_gpio = devm_gpiod_get_index_optional(dev, "enable", i,
> > +                             GPIOD_OUT_HIGH);
> > +             if (IS_ERR(chip->enable_gpio)) {
> > +                     dev_err(dev, "Error getting enable GPIO %i property: %ld\n", i,
> > +                                     PTR_ERR(chip->enable_gpio));
> > +                     return PTR_ERR(chip->enable_gpio);
> > +             }
> > +
> > +             err = tlc5928_probe_chip_dt(dev, child, chip);
> > +             if (err)
> > +                     return err;
> > +
> > +             i++;
> > +     }
> > +
> > +     list_for_each(pos, &priv->chips_list) {
>
> list_for_each_entry()?
>
> > +             chip = container_of(pos, struct tlc5928_chip, list);
> > +             if (chip->enable_gpio)
> > +                     gpiod_set_value(chip->enable_gpio, 0);
> > +     }
> > +
> > +     spi_set_drvdata(spi, priv);
> > +
> > +     return 0;
> > +}
> > +
> > +static int tlc5928_remove(struct spi_device *spi)
> > +{
> > +     struct list_head *pos;
> > +     struct tlc5928_priv *priv = spi_get_drvdata(spi);
> > +     int i;
> > +
> > +     list_for_each(pos, &priv->chips_list) {
>
> list_for_each_entry()?
>

Fixed!

> > +             struct tlc5928_chip *chip = container_of(pos, struct tlc5928_chip,
> > +                             list);
> > +
> > +             for (i = 0; i < TLC5928_MAX_LEDS; i++) {
> > +                     if (chip->leds[i].active)
> > +                             devm_led_classdev_unregister(&spi->dev,
> > +                                          &chip->leds[i].ldev);
>
> Why is it needed?
> devm_led_classdev_register_ext() was used.
>

Yes, because the latch GPIO is set each time a LED is set. But during
the module removing process, devm releases before the GPIO and then
each LED (turning them off). So the kernel gets a NULL pointer after
deference.

An explicit unregister allows to free the LEDs before the GPIO.

> > +             }
> > +
> > +             if (chip->enable_gpio) {
> > +                     gpiod_set_value(chip->enable_gpio, 1);
> > +                     gpiod_put(chip->enable_gpio);
>
> Why is it needed?
> devm_gpiod_get_index_optional() was used.
>

Fixed!

> > +             }
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct spi_device_id tlc5928_id[] = {
> > +     { "tlc5928" },
> > +     {},
>
> Unneeded trailing ,
>
> > +};
> > +MODULE_DEVICE_TABLE(spi, tlc5928_id);
>
> ...
>
> CJ
>

Corentin Guillevic