Re: [PATCH net-next v3 2/2] net: pse-pd: Add Si3474 PSE controller driver

[Kory Maincent <kory.maincent@bootlin.com>]

On Wed, 21 May 2025 08:04:23 +0000
Piotr Kubik <piotr.kubik@adtran.com> wrote:

> On 5/19/25 11:54, Kory Maincent wrote:
> > On Fri, 16 May 2025 13:07:18 +0000
> > Piotr Kubik <piotr.kubik@adtran.com> wrote:
> >   
> >> From: Piotr Kubik <piotr.kubik@adtran.com>
> >>
> >> Add a driver for the Skyworks Si3474 I2C Power Sourcing Equipment
> >> controller.
> >>
> >> Based on the TPS23881 driver code.
> >>
> >> Driver supports basic features of Si3474 IC:
> >> - get port status,
> >> - get port power,
> >> - get port voltage,
> >> - enable/disable port power.
> >>
> >> Only 4p configurations are supported at this moment.  

...

> >> +
> >> +	ret = i2c_smbus_write_byte_data(client, PORT_MODE_REG, val);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	/* Give time for transition to complete */
> >> +	ssleep(1);  
> > 
> > 1s sleep?! It is a lot. Why do you need this? Does it comes from the
> > datasheet? 
> 
> This comes from my experience. I didn't find it in a datasheet.
> I agree this seems a lot, but for 500ms sometimes ports were not powered up. 
> I think I'll give a try to another register and instead PB_POWER_ENABLE 
> I will try to use PB_RESET in combination with PORT_MODE as this seems
> promising.
> 
> btw. Regarding power enable/disable, I think you may have same issue in
> tps23881 as I had here as tps looks very similar to si3474.
> For Si3474 POWER_STATUS register cannot be used as an admin state register as
> it holds actual power interface status (powered/not powered) instead of its
> administrative state (enabled/disabled). 
> Ethtool in this approach was showing for both Admin state and Detection
> status always the same state - actual status.
> PB_POWER_ENABLE register cannot be used for this purpose as well as it is a
> write-only register. That's why I used PORT_MODE register, it acts like an
> admin state holder in my implementation.

Indeed I figured that the power status of the tps23881 can not be really
considered as an admin_state as described in the standard. For example, it
doesn't automatically power off in case of PD unplugged.
That's why I fixed it in the current budget evaluation strategy patch series.
The admin_state is now managed by software and the the PSE core will power on
the port if it catches a classification interrupt event or if a PD was already
plugged and classify.
https://lore.kernel.org/netdev/20250520-feature_poe_port_prio-v11-12-bbaf447e1b28@bootlin.com/

If the Si3474 behaves the same maybe you should rebase your patch on my series.

But waiting this long won't be ok, as we have rtnl lock acquired here.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com