| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3be3ca59-157d-4ceb-81bd-4a1acdbccb9c@wanadoo.fr>
Date: Mon, 31 Mar 2025 18:35:26 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Corentin Guillevic <corentin.guillevic@...le.fr>,
Pavel Machek <pavel@....cz>, Lee Jones <lee@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-leds@...r.kernel.org
Subject: Re: [PATCH 1/2] leds: tlc5928: Driver for the TI 16 Channel spi LED
driver
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@...le.fr>
> ---
...
> +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()?
> + 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()?
> + struct tlc5928_led *led;
> + struct led_init_data init_data = {};
> +
> + init_data.fwnode = of_fwnode_handle(child);
> +
> + err = of_property_read_u32(child, "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;
?
> + 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()?
> + 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.
> + }
> +
> + 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.
> + }
> + }
> +
> + return 0;
> +}
> +
> +static const struct spi_device_id tlc5928_id[] = {
> + { "tlc5928" },
> + {},
Unneeded trailing ,
> +};
> +MODULE_DEVICE_TABLE(spi, tlc5928_id);
...
CJ
Powered by blists - more mailing lists