Re: [PATCH v3 4/5] auxdisplay: add support for Titanmec TM1628 7 segment display controller

On Wed, Feb 23, 2022 at 7:02 PM Heiner Kallweit [off-list ref] wrote:

Co-Developed-by: Heiner Kallweit <hkallweit1@gmail.com>

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 [off-list ref]");
    MODULE_AUTHOR("Heiner Kallweit [off-list ref]");

(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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel