Re: [PATCH v3 3/4] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver

["Jean-François Lessard" <jefflessard3@gmail.com>]

Le 22 août 2025 02 h 08 min 42 s HAE, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit :
>On Fri, Aug 22, 2025 at 5:20 AM Jean-François Lessard
><jefflessard3@gmail.com> wrote:
>> Le 21 août 2025 16 h 19 min 23 s HAE, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit :
>> >On Thu, Aug 21, 2025 at 10:04 PM Jean-François Lessard
>> ><jefflessard3@gmail.com> wrote:
>> >> Le 21 août 2025 04 h 08 min 51 s HAE, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit :
>> >> >On Wed, Aug 20, 2025 at 7:32 PM Jean-François Lessard
>> >> ><jefflessard3@gmail.com> wrote:
>
>...
>
>> >> >> +#define TM16XX_DRIVER_NAME "tm16xx"
>
>> >> The TM16XX_DRIVER_NAME macro is standard practice for consistent string usage
>> >> in registration and module macros.
>> >> If helpful, I can add a leading /* module name */ header comment.
>> >
>> >Instead of an unneeded comment it seems better to use explicit string
>> >literal in all cases (two?).
>>
>> I'm surprised by this preference since driver name macros are very common
>> practice, but I'll use explicit string literals to align on this preference.
>
>Usually we put a macro to something which (theoretically) might
>change. The driver name is part of an ABI and I prefer it to be
>explicit as we do not break an ABI. Once introduced it can't be
>modified or removed. Also it's better to see clearly exactly at the
>place in use in the code the name as it's easier to (git) grep for
>something similar. With a macro I would need to grep at least twice to
>see the users.
>

Well received. I'll use explicit string literals.

>...
>
>> >> >> +       keypad->state = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL);
>> >> >> +       keypad->last_state = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL);
>> >> >> +       keypad->changes = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL);
>> >> >> +       if (!keypad->state || !keypad->last_state || !keypad->changes) {
>> >> >> +               ret = -ENOMEM;
>> >
>> >> >> +               goto free_keypad;
>> >
>> >(Hit send too early that time...) This goto is bad. It means
>> >misunderstanding of the devm concept. See below.
>>
>> I can assure I understand the devm paradigm. The keypad probe is optional,
>> failure doesn't fail the main driver probe but only generates a warning. The
>> cleanup code prevents memory from staying allocated until device removal
>> in this specific optional failure case. However, if you insist, I'll remove the
>> cleanup and let devm handle it normally.
>
>Assume you have a separate feature, let's say keypad driver for some
>complex HW, like this one, and you have even a separate (library)
>driver for it. Then you want to introduce some kind of library
>functions to probe and remove the only keypad part, here are two
>options:
>- follow the library pattern with plain (non-devm) k*alloc() in probe
>and kfree in remove
>- use driver pattern with devm
>
>If you choose the second one, it will be weird to call devm_kfree().
>The rule of thumb the devm_$FREE_MY_RESOURSE() *must* be *well*
>justified. Because it's exceptional. Losing 1kb memory or so is not
>enough to justify.
>

Understood. I'll remove the cleanup gotos for this scenario given that
devm is generally preferred and that losing 1kb memory or so is not
enough to justify.

>> >> >> +       }
>> >> >> +
>> >> >> +       input = devm_input_allocate_device(display->dev);
>
>> >> >> +free_bitmaps:
>> >> >> +       devm_kfree(display->dev, keypad->state);
>> >> >> +       devm_kfree(display->dev, keypad->last_state);
>> >> >> +       devm_kfree(display->dev, keypad->changes);
>> >> >> +free_keypad:
>> >> >> +       devm_kfree(display->dev, keypad);
>> >> >> +       return ret;
>> >
>> >No way. We don't do that, If required it signals about exceptional
>> >case (0.01% probability) or misunderstanding of devm:
>> >- managed resources are managed by core, no need to call for free
>> >- using managed resources in the contexts when object lifetime is
>> >short is incorrect, needs to be switched to the plain alloc (nowadays
>> >with __free() from cleanup.h to have RAII enabled)
>> >
>> >Choose one of these and fix the code accordingly.
>>
>> Same as above.
>

...

>> >I stopped here, I believe it's enough for now (and I would wait for
>> >the smaller changes per patch, perhaps 2 DT bindings patch + common
>> >part (basic functionality) + spi driver + i2c driver + keyboard,
>> >something like 6+ patches).

I know it's a lot to review ~1800 lines at once and that the split into multiple
files will require a review anyway, but feel free to share additional feedback
on any other obvious issues you may observe, so I can fix these for v4.

Thanks again for your time and thorough feedback,
Jean-François Lessard