On Wed, Feb 23, 2022 at 7:02 PM Heiner Kallweit <hkallweit1@xxxxxxxxx> wrote:
>
> Co-Developed-by: Heiner Kallweit <hkallweit1@xxxxxxxxx>
If you (Heiner) are going to be the "From" author, then this line
should not be here.
> + Say Y to enable support for Titan Micro Electronics TM1628
> + LED controller.
> + It's a 3-wire SPI device controlling a two-dimensional grid of
> + LEDs. Dimming is applied to all outputs through an internal PWM.
Maybe a newline between paragraphs?
> + * Copyright (c) 2019 Andreas Färber
...here: should there be entries for you (Heiner) too? If not, should
Andreas be the "From" author?
This also applies to the `MODULE_AUTHOR`.
Also it may be a good idea to add the emails:
MODULE_AUTHOR("Andreas Färber <afaerber@xxxxxxx>");
MODULE_AUTHOR("Heiner Kallweit <hkallweit1@xxxxxxxxx>");
(You may also want to consider adding an entry on `MAINTAINERS`).
> + u8 cmd = TM1628_CMD_DISPLAY_MODE | grid_mode;
Consider using `const` for some of the variables.
> + for (i = 0; i < s->grid_size; i++) {
> + int pos = s->grid[i] - 1;
> +
> + if (i < msg_len) {
Consider inverting the condition, doing the set to `0` + `continue;`
to avoid the indentation.
> + struct tm1628_led *led = container_of(led_cdev, struct tm1628_led, leddev);
> + struct tm1628 *s = led->ctrl;
> + int offset;
> + __le16 bit;
Style: sometimes the variables are initialized right away using a
value from above, but other times they are done below.
> + if (count > s->grid_size + 1) /* consider trailing newline */
Style: sometimes comments are trailing the line, others are above.
Also, sometimes they start with uppercase, but in other cases they do
not.
Also, about the `+ 1`: is it possible that sysfs gives us a buffer
full of `isprint()`? i.e. is it possible that `grid_size ==
MAX_GRID_SIZE` and `count == MAX_GRID_SIZE + 1` and then we perform an
out-of-bounds store to `MAX_GRID_SIZE + 2` in `text`?
> + ret = tm1628_write_data(spi, 0, MAX_GRID_SIZE);
> + if (ret)
> + return ret;
> + /* Assume that subsequent SPI transfers will be ok if first was ok */
If not, is there a consequence? i.e. why wouldn't one check and fail
similarly in the `tm1628_set_*` calls below?
> + if (!IS_REACHABLE(CONFIG_LEDS_CLASS))
> + goto no_leds;
What about putting the code in the `if` body (negating the condition)?
> + num_leds = 0;
This is reusing the variable for a different purpose, no? i.e. if we
did not get here, we would have no leds, yet we would report the
number above.
> + device_for_each_child_node(&spi->dev, child) {
> + u32 reg[2];
> +
> + ret = fwnode_property_read_u32_array(child, "reg", reg, 2);
> + if (ret) {
> + dev_err(&spi->dev, "Reading %s reg property failed (%d)\n",
> + fwnode_get_name(child), ret);
Is a failure expected? i.e. this `continue;`s, but should it fail or
is it OK to proceed?
> + for (i = 0; i < 7; i++) {
Maybe a `#define` for several of the `7`s around?
> +static void tm1628_spi_remove(struct spi_device *spi)
Doesn't `.remove` return `int`?
Thanks!
Cheers,
Miguel
