On Thu, Jun 9, 2022 at 6:30 PM Jean-Jacques Hiblot <jjhiblot@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> ... > +#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