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

[Guenter Roeck <linux@roeck-us.net>]

On 9/8/25 09:39, Gregory Fuchedgi wrote:
> 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?
> 

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