On Thu, Jun 9, 2022 at 6:30 PM Jean-Jacques HiblotIt is more than likely that it will always be a multiple of 8 here.
<jjhiblot@xxxxxxxxxxxxxxx> wrote:
The TLC5925 is a 16-channels constant-current LED sink driver.Can you add Datasheet: tag here with the corresponding URL? Rationale
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.
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>Keep it sorted?
+#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>
...
+struct single_led_priv {For pointer arithmetics it's better to swap these two members.
+ int idx;
+ struct led_classdev cdev;
+};unsigned long? DECLARE_BITMAP() ?
+
+struct tlc5925_leds_priv {
+ int max_num_leds;
+ u8 *state;
+ spinlock_t lock;...
+ struct single_led_priv leds[];
+};
+ if (brightness)assign_bit()
+ priv->state[index / 8] |= (1 << (index % 8));
+ else
+ priv->state[index / 8] &= ~(1 << (index % 8));
...
+ return spi_write(spi, priv->state, priv->max_num_leds / 8);BITS_TO_BYTES() ?
...
+ count = device_get_child_node_count(dev);return dev_err_probe(...);
+ if (!count) {
+ dev_err(dev, "no led defined.\n");
+ return -ENODEV;
here and everywhere in ->probe() and Co.
+ }...
+ ret = device_property_read_u32_array(dev, "ti,shift-register-length",Always an array of 1 element? call device_property_read_u32().
+ &max_num_leds, 1);
...
+ if (max_num_leds % 8) {Is this really fatal? I would rather issue a warning and go on if it
+ dev_err(dev, "'ti,shift-register-length' must be a multiple of 8\n");
+ return -EINVAL;
+ }
has at least 8 there. So the idea is to use a minimum that holds
multiple of 8.
...
+ /* Assert all the OE/ lines */You have to use dev_err_probe() here, otherwise it will spam logs a
+ 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);
+ }
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) {Missed spaces.
+ struct led_init_data init_data = {.fwnode = child};
+ struct led_classdev *cdev;fwnode_property_read_u32()
+ u32 idx;
+
+ ret = fwnode_property_read_u32_array(child, "reg", &idx, 1);
+ if (ret || idx >= max_num_leds) {Either dev_warn + continue, or dev_err + return dev_err_probe().
+ dev_err(dev, "%s: invalid reg value. Ignoring.\n",
+ fwnode_get_name(child));
+ fwnode_handle_put(child);
+ continue;
+ }Ditto.
+
+ 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) {
+ 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[] = {No comma for terminator entry.
+ { .compatible = "ti,tlc5925", },
+ {},
+};Where is the MODULE_DEVICE_TABLE() for this one?
...
+No need for this blank line.
+module_spi_driver(tlc5925_driver);