On Tue, Jun 14, 2022 at 4:27 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. > > Datasheet: https://www.ti.com/lit/ds/symlink/tlc5925.pdf > No blank lines are allowed in the tag block. > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@xxxxxxxxxxxxxxx> ... > +#include <linux/err.h> > +#include <linux/leds.h> > +#include <linux/module.h> > +#include <linux/property.h> > +#include <linux/spi/spi.h> This misses a few headers that this code is direct user of: container_of.h gpio/consumer.h types.h ... > + // assign_bit() is atomic, no need for lock Comment is useless, since it's a pattern that is used in the kernel: __op is non-atomic, op is atomic. ... > + > + One blank line is enough ... > + > + Ditto. ... > + gpios = devm_gpiod_get_array(dev, "output-enable-b", GPIOD_OUT_LOW); > + if (IS_ERR(gpios)) { > + return dev_err_probe(dev, PTR_ERR(gpios), > + "Unable to get the 'output-enable-b' gpios\n"); > + } {} are not needed, and you may put the return on one line. ... > + count = device_get_child_node_count(dev); > + if (!count) { > + dev_err(dev, "no led defined.\n"); > + return -ENODEV; > + } It's fine to use return dev_err_probe() in such cases like above, it's written in the documentation. ... > + ret = fwnode_property_read_u32(child, "reg", &idx); > + if (ret || idx >= max_num_leds) { > + dev_warn(dev, "%s: invalid reg value. Ignoring.\n", > + fwnode_get_name(child)); %pfw / %pfwP ? > + fwnode_handle_put(child); > + continue; > + } ... > + ret = devm_led_classdev_register_ext(dev, cdev, &init_data); > + if (ret) { > + dev_warn(dev, "%s: cannot create LED device.\n", > + fwnode_get_name(child)); Ditto. > + fwnode_handle_put(child); > + continue; > + } -- With Best Regards, Andy Shevchenko
