On Mon, Sep 8, 2025 at 11:02 AM Guenter Roeck <linux@xxxxxxxxxxxx> wrote: > > 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. Alright, then. I'll try to find some time in the next few months to port this over to a separate driver in pse-pd. And then remove the in*_enable from hwmon one. Even if it's there by mistake, probably shouldn't fix it until there's an alternative in place. > > 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.
