Re: [PATCH v3 0/2] hwmon: (tps23861) add class restrictions and semi-auto mode support

[Gregory Fuchedgi <gfuchedgi@gmail.com>]

On Sun, Sep 7, 2025 at 9:51 PM Oleksij Rempel <o.rempel@pengutronix.de> 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@gmail.com>
> >
> > 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?

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.