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

[Kory Maincent]

Le Mon, 30 Jun 2025 14:57:09 +0000,
Piotr Kubik <piotr.kubik@xxxxxxxxxx> a écrit :

> From: Piotr Kubik <piotr.kubik@xxxxxxxxxx>
> 
> Add a driver for the Skyworks Si3474 I2C Power Sourcing Equipment
> controller.
> 
> 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.
> 
> Signed-off-by: Piotr Kubik <piotr.kubik@xxxxxxxxxx>
> ---

...

> +
> +static int si3474_pi_get_admin_state(struct pse_controller_dev *pcdev, int
> id,
> +				     struct pse_admin_state *admin_state)
> +{
> +	struct si3474_priv *priv = to_si3474_priv(pcdev);
> +	struct i2c_client *client;
> +	bool is_enabled = false;
> +	u8 chan0, chan1;
> +	s32 ret;
> +
> +	if (id >= SI3474_MAX_CHANS)
> +		return -ERANGE;
> +
> +	si3474_get_channels(priv, id, &chan0, &chan1);
> +	client = si3474_get_chan_client(priv, chan0);
> +
> +	ret = i2c_smbus_read_byte_data(client, PORT_MODE_REG);
> +	if (ret < 0) {
> +		admin_state->c33_admin_state =
> +			ETHTOOL_C33_PSE_ADMIN_STATE_UNKNOWN;
> +		return ret;
> +	}
> +
> +	is_enabled = ((ret & CHAN_MASK(chan0)) |
> +		      (ret & CHAN_MASK(chan1))) != 0;

I don't think this "!= 0" check is needed here.
A bool is an int so it will be set even without this and the next condition
will work.

> +	if (is_enabled)
> +		admin_state->c33_admin_state =
> +			ETHTOOL_C33_PSE_ADMIN_STATE_ENABLED;
> +	else
> +		admin_state->c33_admin_state =
> +			ETHTOOL_C33_PSE_ADMIN_STATE_DISABLED;
> +
> +	return 0;
> +}
> +

...

> +
> +static int si3474_pi_enable(struct pse_controller_dev *pcdev, int id)
> +{
> +	struct si3474_priv *priv = to_si3474_priv(pcdev);
> +	struct i2c_client *client;
> +	u8 chan0, chan1;
> +	u8 val = 0;
> +	s32 ret;
> +
> +	if (id >= SI3474_MAX_CHANS)
> +		return -ERANGE;
> +
> +	si3474_get_channels(priv, id, &chan0, &chan1);
> +	client = si3474_get_chan_client(priv, chan0);
> +
> +	/* Release PI from shutdown */
> +	ret = i2c_smbus_read_byte_data(client, PORT_MODE_REG);
> +	if (ret < 0)
> +		return ret;
> +
> +	val = (u8)ret;
> +	val |= CHAN_MASK(chan0);
> +	val |= CHAN_MASK(chan1);
> +
> +	ret = i2c_smbus_write_byte_data(client, PORT_MODE_REG, val);
> +	if (ret)
> +		return ret;
> +
> +	/* DETECT_CLASS_ENABLE must be set when using AUTO mode,
> +	 * otherwise PI does not power up - datasheet section 2.10.2
> +	 */

What happen in a PD disconnection case? According to the datasheet it simply
raise a disconnection interrupt and disconnect the power with a
DISCONNECT_PCUT_FAULT fault. But it is not clear if it goes back to the
detection + classification process. If it is not the case you will face the
same issue I did and will need to deal with the interrupt and the disconnection
management.

Could you try to enable a port, plug a PD then disconnect it and plug another PD
which belong to another power class. Finally read the class detected to verify that the
class detected have changed.

> +	val = (CHAN_BIT(chan0) | CHAN_UPPER_BIT(chan0) |
> +	       CHAN_BIT(chan1) | CHAN_UPPER_BIT(chan1));
> +	ret = i2c_smbus_write_byte_data(client, DETECT_CLASS_ENABLE_REG,
> val);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int si3474_pi_disable(struct pse_controller_dev *pcdev, int id)
> +{
> +	struct si3474_priv *priv = to_si3474_priv(pcdev);
> +	struct i2c_client *client;
> +	u8 chan0, chan1;
> +	u8 val = 0;
> +	s32 ret;
> +
> +	if (id >= SI3474_MAX_CHANS)
> +		return -ERANGE;
> +
> +	si3474_get_channels(priv, id, &chan0, &chan1);
> +	client = si3474_get_chan_client(priv, chan0);
> +
> +	/* Set PI in shutdown mode */
> +	ret = i2c_smbus_read_byte_data(client, PORT_MODE_REG);
> +	if (ret < 0)
> +		return ret;
> +
> +	val = (u8)ret;
> +	val &= ~(CHAN_MASK(chan0));
> +	val &= ~(CHAN_MASK(chan1));
> +
> +	ret = i2c_smbus_write_byte_data(client, PORT_MODE_REG, val);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int si3474_pi_get_chan_current(struct si3474_priv *priv, u8 chan)
> +{
> +	struct i2c_client *client;
> +	s32 ret;
> +	u8 reg;
> +	u64 tmp_64;

Reverse christmas tree please.

> +
> +	client = si3474_get_chan_client(priv, chan);
> +
> +	/* Registers 0x30 to 0x3d */
> +	reg = CHAN_REG(PORT1_CURRENT_LSB_REG, chan);
> +
> +	ret = i2c_smbus_read_word_data(client, reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	tmp_64 = ret * SI3474_NA_STEP;
> +
> +	/* uA = nA / 1000 */
> +	tmp_64 = DIV_ROUND_CLOSEST_ULL(tmp_64, 1000);
> +	return (int)tmp_64;
> +}
> +
> +static int si3474_pi_get_chan_voltage(struct si3474_priv *priv, u8 chan)
> +{
> +	struct i2c_client *client;
> +	s32 ret;
> +	u8 reg;
> +	u32 val;

Same.

> +
> +	client = si3474_get_chan_client(priv, chan);
> +
> +	/* Registers 0x32 to 0x3f */
> +	reg = CHAN_REG(PORT1_VOLTAGE_LSB_REG, chan);
> +
> +	ret = i2c_smbus_read_word_data(client, reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	val = ret * SI3474_UV_STEP;
> +
> +	return (int)val;
> +}
> +
> +static int si3474_pi_get_voltage(struct pse_controller_dev *pcdev, int id)
> +{
> +	struct si3474_priv *priv = to_si3474_priv(pcdev);
> +	struct i2c_client *client;
> +	u8 chan0, chan1;
> +	s32 ret;
> +
> +	si3474_get_channels(priv, id, &chan0, &chan1);
> +	client = si3474_get_chan_client(priv, chan0);
> +
> +	/* Check which channels are enabled*/
> +	ret = i2c_smbus_read_byte_data(client, POWER_STATUS_REG);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Take voltage from the first enabled channel */
> +	if (ret & CHAN_BIT(chan0))
> +		ret = si3474_pi_get_chan_voltage(priv, chan0);
> +	else if (ret & CHAN_BIT(chan1))
> +		ret = si3474_pi_get_chan_voltage(priv, chan1);
> +	else
> +		/* 'should' be no voltage in this case */
> +		return 0;
> +
> +	return ret;
> +}
> +
> +static int si3474_pi_get_actual_pw(struct pse_controller_dev *pcdev, int id)
> +{
> +	struct si3474_priv *priv = to_si3474_priv(pcdev);
> +	s32 ret;
> +	u32 uV, uA;
> +	u64 tmp_64;
> +	u8 chan0, chan1;

Same


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