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

[Heiner Kallweit <hkallweit1@xxxxxxxxx>]

On 23.02.2022 21:28, Miguel Ojeda wrote:
> 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?
> 
OK

>> + * 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`).
> 

Maybe we should have both names in all places you mentioned.
I'd be fine with being listed as maintainer.

@Andreas: After you wrote the initial code and I rewrote bigger
parts of it: Do you want to be listed as maintainer too?
And any remark on the questions raised by Miguel?

>> +       u8 cmd = TM1628_CMD_DISPLAY_MODE | grid_mode;
> 
> Consider using `const` for some of the variables.
> 
OK

>> +       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.
> 
OK

>> +       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.
> 
OK

>> +       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.
> 
OK, will beautify this.

> 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`?
> 
Typically there's no issue because command line input always ends
with the (non-printable) newline. But I checked and yes, it's possible
to pipe data w/o trailing newline to the attribute.
Hence I have to fix the code.

>> +       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?
> 
I'll change this check the return code also for the next calls.

>> +       if (!IS_REACHABLE(CONFIG_LEDS_CLASS))
>> +               goto no_leds;
> 
> What about putting the code in the `if` body (negating the condition)?
> 
I did it this way to avoid having to indent the following bigger block.

>> +       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.
> 
The variable is re-used for a slightly different purpose. At first it's
used for the number of LED child nodes (what may include invalid configs),
then it's used for the number of the actually configured LED's.
And right, I have to move the assignment before the check.

>> +       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?
> 

Symbol LED's and display digits are independent. Therefore, if there's a problem
with a DT LED definition, I chose to just skip it instead if bailing out completely.

>> +       for (i = 0; i < 7; i++) {
> 
> Maybe a `#define` for several of the `7`s around?
> 
OK

>> +static void tm1628_spi_remove(struct spi_device *spi)
> 
> Doesn't `.remove` return `int`?
> 
This has changed recently with a0386bba7093 ("spi: make remove callback a void function").

> Thanks!
> 
> Cheers,
> Miguel

Thanks for the comprehensive feedback!

Heiner