Message-ID: <CAHp75VdaPVcKhLSCOiUw+0cqNq6pkxZqVpd2Bk-q-9dNV=+kqA@mail.gmail.com>
Date: Sat, 1 Nov 2025 19:06:55 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Jean-François Lessard <jefflessard3@...il.com>
Cc: Andy Shevchenko <andriy.shevchenko@...el.com>, Andy Shevchenko <andy@...nel.org>, 
	Geert Uytterhoeven <geert@...ux-m68k.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, linux-kernel@...r.kernel.org, 
	linux-leds@...r.kernel.org, devicetree@...r.kernel.org, 
	Paolo Sabatino <paolo.sabatino@...il.com>, Christian Hewitt <christianshewitt@...il.com>, 
	Martin Blumenstingl <martin.blumenstingl@...glemail.com>
Subject: Re: [PATCH v5 4/7] auxdisplay: Add TM16xx 7-segment LED matrix
 display controllers driver

On Fri, Oct 31, 2025 at 7:17 PM Jean-François Lessard
<jefflessard3@...il.com> wrote:
> Le 31 octobre 2025 05 h 41 min 44 s HAE, Andy Shevchenko <andriy.shevchenko@...el.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.

> >> +}

...

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

...

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

...

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

...

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

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

...

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

...

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

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists