Messages in this thread | | | Date | Tue, 14 Jun 2022 14:52:49 +0200 | Subject | Re: [PATCH v3 2/3] leds: Add driver for the TLC5925 LED controller | From | Jean-Jacques Hiblot <> |
| |
On 09/06/2022 18:57, Andy Shevchenko wrote: > 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. It is more than likely that it will always be a multiple of 8 here.
We could in theory use a non-multiple of 8 (some LEDs of the first or last chips of the chain are not populated).
I didn't think it would add a significant benefit in memory usage. In terms of SPI usage it wouldn't change anything.
Thanks for your feedback.
> ... > >> + /* 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);
| |