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

[Carlo Szelinsky <github@szelinsky.de>]

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.