Le Mon, 30 Jun 2025 14:57:09 +0000,
Piotr Kubik [off-list ref] a écrit :

From: Piotr Kubik <redacted>

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 <redacted>
---

...

+
+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