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

[Miguel Ojeda <miguel.ojeda.sandonis@xxxxxxxxx>]

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