Re: [PATCH] net: pse-pd: add LED trigger support

Re: [PATCH] net: pse-pd: add LED trigger support

[Carlo Szelinsky]

Hey Kory, Hey Oleksij,

Thanks again for taking the time to give detailed feedback. I am not really experienced with working on the kernel, so I took some time to process and get a clear picture. I will try to implement and test it asap... My action points would be the following:
                                                                                                                              
1. Replace the LED specific polling with some generic devm_pse_poll_helper() that is based on the existing pse_isr() logic but in a timer instead of an IRQ - pushing events through ntf_fifo / pse_send_ntf_worker() like other IRQ-based controllers already do.
2. Fire LED triggers from the notification path, not from a separate poll loop: LEDs react to state changes e.g. they don't drive their own polling.
3. Fix pse_pw_d_is_sw_pw_control() - it currently requires pcdev->irq to be set in the PSE_BUDGET_EVAL_STRAT_DISABLED path, so poll-only controllers like hs104 would never enter software power control. Needs to also check for an active poll worker.
4. Add #define for the default poll interval (e.g. 500ms) with a comment explainin why.

Did I understand you correctly to not waste any time?

Unclear is for me still:
* Poll helper design: new devm_pse_poll_helper() vs extending devm_pse_irq_helper() with IRQ=0 fallback? I suggest a separate devm_pse_poll_helper() - it keeps the IRQ and poll paths clean and symmetric, and avoids overloading devm_pse_irq_helper() with conditional logic.
* Who decides to poll? The driver explicitly calls the poll helper, or the core auto-detects missing IRQ? I suggest the driver decides explicitly - the driver knows its hardware best, and an explicit call is easier to review and reason about than auto-detection magic.
* DT property: rename led-poll-interval-ms to poll-interval-ms since polling is now generic? I suggest yes - the polling is no longer LED-specific, so the property name should reflect that.
* Kory mentioned two distinct cases: (a) controller has no IRQ support at all (like the hs104), (b) controller supports IRQ but it's not wired on the board. Should both cases be handled by the same poll helper, or does (b) need different treatment? I suggest the same devm_pse_poll_helper() handles both - from the core's perspective the situation is identical: no IRQ available, need to poll. The driver just calls the poll helper instead of the IRQ helper in either case.

Have a nice weekend,
cheers.

Re: [PATCH] net: pse-pd: add LED trigger support

[Kory Maincent]

Hello Carlo,

On Sat, 21 Mar 2026 18:55:46 +0100
Carlo Szelinsky <github@szelinsky.de> wrote:

> Hey Kory, Hey Oleksij,
> 
> Thanks again for taking the time to give detailed feedback. I am not really
> experienced with working on the kernel, so I took some time to process and
> get a clear picture. I will try to implement and test it asap... My action
> points would be the following: 1. Replace the LED specific polling with some
> generic devm_pse_poll_helper() that is based on the existing pse_isr() logic
> but in a timer instead of an IRQ - pushing events through ntf_fifo /
> pse_send_ntf_worker() like other IRQ-based controllers already do. 

Please add a patch to introduce the poll path before any LED support.
I will test this new path with my boards with and without the IRQ configured. 

> 2. Fire
> LED triggers from the notification path, not from a separate poll loop: LEDs
> react to state changes e.g. they don't drive their own polling. 3. Fix
> pse_pw_d_is_sw_pw_control() - it currently requires pcdev->irq to be set in
> the PSE_BUDGET_EVAL_STRAT_DISABLED path, so poll-only controllers like hs104
> would never enter software power control. Needs to also check for an active
> poll worker. 4. Add #define for the default poll interval (e.g. 500ms) with a
> comment explainin why.
> 
> Did I understand you correctly to not waste any time?

Yes that's it.

> Unclear is for me still:
> * Poll helper design: new devm_pse_poll_helper() vs extending
> devm_pse_irq_helper() with IRQ=0 fallback? I suggest a separate
> devm_pse_poll_helper() - it keeps the IRQ and poll paths clean and symmetric,
> and avoids overloading devm_pse_irq_helper() with conditional logic.
> * Who decides to poll? The driver explicitly calls the poll helper, or the
> core auto-detects missing IRQ? I suggest the driver decides explicitly - the
> driver knows its hardware best, and an explicit call is easier to review and
> reason about than auto-detection magic.
> * DT property: rename led-poll-interval-ms to poll-interval-ms since polling
> is now generic? I suggest yes - the polling is no longer LED-specific, so the
> property name should reflect that.
> * Kory mentioned two distinct cases: (a) controller has no IRQ support at all
> (like the hs104), (b) controller supports IRQ but it's not wired on the
> board. Should both cases be handled by the same poll helper, or does (b) need
> different treatment? I suggest the same devm_pse_poll_helper() handles both -
> from the core's perspective the situation is identical: no IRQ available,
> need to poll. The driver just calls the poll helper instead of the IRQ helper
> in either case.

I agreed with all your points.
Thank you!

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH] net: pse-pd: add LED trigger support

[Carlo Szelinsky]

Thank you for the support Kory, Oleksij and Andrew.