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

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

Le 1 novembre 2025 13 h 06 min 55 s HAE, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit :
>On Fri, Oct 31, 2025 at 7:17 PM Jean-François Lessard
><jefflessard3@gmail.com> wrote:
>> Le 31 octobre 2025 05 h 41 min 44 s HAE, Andy Shevchenko <andriy.shevchenko@intel.com> a écrit :
>> >On Fri, Sep 26, 2025 at 10:19:05AM -0400, Jean-François Lessard wrote:
>
>...
>
>> >> +static inline void tm16xx_set_seg(const struct tm16xx_display *display,
>> >> +                              const u8 hwgrid, const u8 hwseg, const bool on)
>> >> +{
>> >> +    assign_bit(hwgrid * display->num_hwseg + hwseg, display->state, on);
>> >
>> >Do you need an atomic call here? Perhaps __assign_bit() would suffice,
>>
>> Keeping assign_bit(), it's required here. Two distinct concurrency scenarios
>> exist:
>> - Bitmap: Multiple LED triggers (network, timer) + userspace write to
>>   display->state concurrently -> need atomic ops
>> - Hardware: Mutex serializes different hardware operations (flush_init,
>>   flush_display, keypad polling) that can race
>> The mutex doesn't eliminate bitmap concurrency needs, they're orthogonal
>> concerns.
>
>Okay, but the below bitmap_read() is non-atomic. And in general the
>bitmap API is not atomic.
>

The atomic assign_bit() protects read-modify-write operations from concurrent
modifications. Reads are non-critical since every write schedules a flush work,
providing eventual consistency. This is a valid optimization pattern and
doesn't 
require atomic reads.

>> >> +}
>
>...
>
>> >> +                            ret = fwnode_property_read_u32_array(child,
>> >> +                                                                 "segments", segments,
>> >> +                                                                 TM16XX_DIGIT_SEGMENTS * 2);
>> >
>> >> +                            if (ret < 0)
>> >> +                                    return ret;
>> >
>> >Why '< 0'? Here it's definitely not a counting call, so it should never return
>> >positive in this case.
>>
>> Keeping if (ret < 0). While usage with non-NULL buffer won't return positive
>> values, fwnode_property_read_u32_array() documentation explicitly states it can
>> return count when buffer is NULL. Using < 0 is the defensive, API-compliant
>> pattern that matches the function signature.
>
>Okay, it's fine to keep this way.
>

Acknowledged.

>...
>
>> >> +                            ret = fwnode_property_read_u32_array(child, "reg", reg, 2);
>> >> +                            if (ret < 0)
>> >
>> >Ditto,.
>> >
>>
>> As per above.
>>
>> >> +                                    return ret;
>
>...
>
>> >> +    INIT_WORK(&display->flush_init, tm16xx_display_flush_init);
>> >> +    INIT_WORK(&display->flush_display, tm16xx_display_flush_data);
>> >
>> >devm-helpers.h have something for this case, I believe.
>>
>> Cannot use devm_work_autocancel(). The shutdown sequence requires specific
>> ordering: (1) unregister LEDs to stop triggers, (2) clear display state, (3)
>> flush pending work, (4) turn off display. This sequence prevents hardware
>> access races when triggers attempt to update the display during removal. Manual
>> INIT_WORK with explicit flush/cancel in remove() provides this control.
>
>Do you mean that the removal order is not symmetrical to the probe one?
>At bare minimum this needs a comment in the code (as summary above) to
>explain why devm_*() are not being used.
>

Acknowledged. I'll add a code comment summarizing the non-devm rationale for
both work queues and LED registration: explicit sequencing prevents LED
triggers from accessing hardware post-removal.

>...
>
>> >> +    main->max_brightness = display->controller->max_brightness;
>> >> +    device_property_read_u32(dev, "max-brightness", &main->max_brightness);
>> >> +    main->max_brightness = umin(main->max_brightness,
>> >> +                                display->controller->max_brightness);
>> >
>> >Hmm... Why 'u' variant of macro?
>> >
>> >> +    main->brightness = main->max_brightness;
>> >> +    device_property_read_u32(dev, "default-brightness", &main->brightness);
>> >> +    main->brightness = umin(main->brightness, main->max_brightness);
>> >
>> >Ditto.
>>
>> Correct for unsigned brightness values. umin() is the appropriate macro for
>> unsigned types to avoid type conversion warnings.
>
>But are you in control of all the variable types? If so, why not align
>the types?
>

brightness and max_brightness are u32 (unsigned). Using umin() is the
appropriate macro for unsigned comparisons. If type definitions change in the
future, the compiler will flag any incompatibilities.

I think the overall design is now pretty sound. I would like to get sure we are
not gold-plating and that we stay focused on actual issues.

>...
>
>> >Given a comment about propagating fwnode, why do we need all this? Doesn't led
>> >core take care of these properties as well?
>>
>> Manual handling is necessary because:
>> 1. default-brightness: Not implemented in LED core
>
>Oh, indeed, I mixed this with default-state. But the side question
>here is what prevents us from implementing it? I suspect there were
>discussions in the past, but I haven;t dug the lore archive to see if
>any indeed happened.
>

default-brightness could be added to LED core as a separate enhancement. For
this TM16xx driver, manual handling is necessary given the same considerations
as max-brightness. Any future LED core changes can be addressed independently.

>> 2. max-brightness defaulting: If DT property is absent, default to
>>    controller->max_brightness
>> 3. Ceiling enforcement: When DT property IS present, clamp to not exceed
>>    hardware limits (controller->max_brightness)
>>
>> LED core only reads max-brightness optionally, it doesn't handle defaulting or
>> hardware ceiling enforcement.
>
>Yep, thanks for elaborating.
>

I'll add a code comment about defaulting and hardware ceiling enforcement.

>...
>
>> >> +            ret = led_classdev_register_ext(dev, &led->cdev, &led_init);
>> >
>> >Why not devm_led_*()?
>>
>> Intentional non-devm design documented in commit notes. Explicit unregistration
>> before removal immediately stops LED triggers, preventing them from accessing
>> hardware post-removal. devm_led_*() would require complex brightness callback
>> state tracking to handle trigger activity during remove(). Explicit unregister
>> is cleaner and eliminates this race.
>
>Right, so I think the summary of this needs to be commented on in the
>code (as well).
>

I'll add code comment explaining the non-devm rationale for LED registration.

>...
>
>> >> +    ret = linedisp_attach(&display->linedisp, display->main_led.dev,
>> >> +                          display->num_digits, &tm16xx_linedisp_ops);
>
>> >If we haven't yet devm for this, it can be
>> >1) introduced, OR
>> >2) wrapped to become a such (see devm_add_action_or_reset() usage).
>> >
>>
>> While devm_add_action_or_reset() could wrap linedisp_detach(), the overall
>> shutdown still requires explicit ordering across multiple subsystems (linedisp,
>> LEDs, workqueues, hardware). Using devm for just one component while manually
>> managing others adds complexity without benefit. The current explicit approach
>> keeps all cleanup logic together in remove() for clarity.
>
>Okay, I need to have a look at this again when you send a new version,
>but I want to finish reviewing this one. Sorry it takes time.
>

Thanks for the thorough review of the core driver patch. Before I finalize v6
with these changes and added code comments, would you be able to review the
remaining patches in the series (keypad/I2C/SPI bus support)? Comprehensive
feedback now will help ensure v6 addresses all architectural concerns
systematically, rather than discovering issues after resubmission.

Thanks,
Jean-François