lkml.org 
[lkml]   [2022]   [Jun]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v3 2/3] leds: Add driver for the TLC5925 LED controller
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.

...

> + /* 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);

--
With Best Regards,
Andy Shevchenko

\
 
 \ /
  Last update: 2022-06-09 18:59    [W:0.063 / U:3.632 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site