On 9/8/25 09:39, Gregory Fuchedgi wrote:
On Sun, Sep 7, 2025 at 9:51 PM Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx> wrote:
On Sun, Sep 07, 2025 at 09:06:25AM -0700, Guenter Roeck wrote:
+Cc: pse-pd maintainers and netdev mailing list
On 9/4/25 10:33, Gregory Fuchedgi via B4 Relay wrote:
This patch series introduces per-port device tree configuration with poe
class restrictions. Also adds optional reset/shutdown gpios.
Tested with hw poe tester:
- Auto mode tested with no per-port DT settings as well as explicit port
DT ti,class=4. Tested that no IRQ is required in this case.
- Semi-Auto mode with class restricted to 0, 1, 2 or 3. IRQ required.
- Tested current cut-offs in Semi-Auto mode.
- On/off by default setting tested for both Auto and Semi-Auto modes.
- Tested fully disabling the ports in DT.
- Tested with both reset and ti,ports-shutdown gpios defined, as well as
with reset only, as well as with neither reset nor shutdown.
Signed-off-by: Gregory Fuchedgi <gfuchedgi@xxxxxxxxx>
This entire series makes me more and more unhappy. It is not the responsibility
of the hardware monitoring subsystem to control power. The hardware monitoring
subsystem is for monitoring, not for control.
Please consider adding a driver for this chip to the pse-pd subsystem
(drivers/net/pse-pd). As it turns out, that subsystem already supports
tps23881. This is a similar chip which even has a similar register set.
This driver could then be modified to be an auxiliary driver of that driver.
Alternatively, we could drop this driver entirely since the pse-pd subsystem
registers the chips it supports as regulator which has its own means to handle
telemetry.
Yes, Guenter is right. This driver belongs to the pse-pd framework.
No disagreement here in principle. However, the current hwmon driver
already implements power control and exposes it via in*_enable sysfs
files. I found this a bit odd, but I don't write drivers often.
My understanding of Guenter's suggestion is that it would require breaking
this userspace API?
If the enable attributes enable power to the ports, that code and functionality
is simply wrong. It should only enable (or have enabled) power _monitoring_.
As such, changing that would from my perspective be a bug fix.
And, yes, that slipped my attention when reviewing the original code.
Sorry to have to say that, but I am not perfect.
From a quick look at the tps23881 datasheet I can see that it is
similar, however, it is quite different in the context of this patch.
tps23881 (unlike tps23861) has Port Power Allocation register that can
limit poe power class. This register can be set prior to
detection/classification. So the extra complexity of an interrupt
handler that decides whether to enable the power may not be required.
Perhaps it still makes sense to merge these drivers, but I don't have
time or hardware to do it at the moment.
I didn't suggest to merge the tps23881 and tps23861 drivers; I just pointed out
that they have a similar register set.
The point here is that a hardware monitoring driver should limit itself
to hardware monitoring. Actual control should, for example, be implemented
through the regulator or thermal subsystems.
Guenter
