[PATCH net-next v3 0/2] Add Si3474 PSE controller driver

[PATCH net-next v3 0/2] Add Si3474 PSE controller driver

[Piotr Kubik]

From: Piotr Kubik <piotr.kubik@adtran.com>

These patch series provide support for Skyworks Si3474 I2C Power
Sourcing Equipment controller.

Based on the TPS23881 driver code.

Supported features of Si3474:
- get port status,
- get port power,
- get port voltage,
- enable/disable port power

Signed-off-by: Piotr Kubik <piotr.kubik@adtran.com>
---

Changes in v3:
  - Use _scoped version of for_each_child_of_node().
  - Remove redundant return value assignments in si3474_get_of_channels().
  - Change dev_info() to dev_dbg() on successful probe.
  - Rename all instances of "slave" to "secondary".
  - Register devm cleanup action for ancillary i2c, simplifying cleanup logic in si3474_i2c_probe().
  - Add explicit return 0 on successful probe.
  - Drop unnecessary .remove callback.
  - Update channel node description in device tree binding documentation.
  - Reorder reg and reg-names properties in device tree binding documentation.
  - Rename all "slave" references to "secondary" in device tree bindings documentation.
  - Link to v2: https://lore.kernel.org/netdev/bf9e5c77-512d-4efb-ad1d-f14120c4e06b@adtran.com

Changes in v2:
  - Handle both IC quads via single driver instance
  - Add architecture & terminology description comment
  - Change pi_enable, pi_disable, pi_get_admin_state to use PORT_MODE register
  - Rename power ports to 'pi'
  - Use i2c_smbus_write_byte_data() for single byte registers
  - Coding style improvements
  - Link to v1: https://lore.kernel.org/netdev/a92be603-7ad4-4dd3-b083-548658a4448a@adtran.com

---
Piotr Kubik (2):
  dt-bindings: net: pse-pd: Add bindings for Si3474 PSE controller
  net: pse-pd: Add Si3474 PSE controller driver

 .../bindings/net/pse-pd/skyworks,si3474.yaml  | 144 ++++
 drivers/net/pse-pd/Kconfig                    |  10 +
 drivers/net/pse-pd/Makefile                   |   1 +
 drivers/net/pse-pd/si3474.c                   | 649 ++++++++++++++++++
 4 files changed, 804 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/pse-pd/skyworks,si3474.yaml
 create mode 100644 drivers/net/pse-pd/si3474.c

--
2.43.0

Piotr Kubik

piotr.kubik@adtran.com
www.adtran.com

[PATCH net-next v3 1/2] dt-bindings: net: pse-pd: Add bindings for Si3474 PSE controller

[Piotr Kubik]

From: Piotr Kubik <piotr.kubik@adtran.com>

Add the Si3474 I2C Power Sourcing Equipment controller device tree
bindings documentation.

Signed-off-by: Piotr Kubik <piotr.kubik@adtran.com>
---
 .../bindings/net/pse-pd/skyworks,si3474.yaml  | 144 ++++++++++++++++++
 1 file changed, 144 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/pse-pd/skyworks,si3474.yaml

diff --git a/Documentation/devicetree/bindings/net/pse-pd/skyworks,si3474.yaml b/Documentation/devicetree/bindings/net/pse-pd/skyworks,si3474.yaml
new file mode 100644
index 000000000000..edd36a43a387
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/pse-pd/skyworks,si3474.yaml
@@ -0,0 +1,144 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/pse-pd/skyworks,si3474.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Skyworks Si3474 Power Sourcing Equipment controller
+
+maintainers:
+  - Piotr Kubik <piotr.kubik@adtran.com>
+
+allOf:
+  - $ref: pse-controller.yaml#
+
+properties:
+  compatible:
+    enum:
+      - skyworks,si3474
+
+  reg:
+    maxItems: 2
+
+  reg-names:
+    items:
+      - const: main
+      - const: secondary
+
+  channels:
+    description: The Si3474 is a single-chip PoE PSE controller managing
+      8 physical power delivery channels. Internally, it's structured
+      into two logical "Quads".
+      Quad 0 Manages physical channels ('ports' in datasheet) 0, 1, 2, 3
+      Quad 1 Manages physical channels ('ports' in datasheet) 4, 5, 6, 7.
+
+    type: object
+    additionalProperties: false
+
+    properties:
+      "#address-cells":
+        const: 1
+
+      "#size-cells":
+        const: 0
+
+    patternProperties:
+      '^channel@[0-7]$':
+        type: object
+        additionalProperties: false
+
+        properties:
+          reg:
+            maxItems: 1
+
+        required:
+          - reg
+
+    required:
+      - "#address-cells"
+      - "#size-cells"
+
+required:
+  - compatible
+  - reg
+  - pse-pis
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      ethernet-pse@26 {
+        compatible = "skyworks,si3474";
+        reg-names = "main", "secondary";
+        reg = <0x26>, <0x27>;
+
+        channels {
+          #address-cells = <1>;
+          #size-cells = <0>;
+          phys0_0: channel@0 {
+            reg = <0>;
+          };
+          phys0_1: channel@1 {
+            reg = <1>;
+          };
+          phys0_2: channel@2 {
+            reg = <2>;
+          };
+          phys0_3: channel@3 {
+            reg = <3>;
+          };
+          phys0_4: channel@4 {
+            reg = <4>;
+          };
+          phys0_5: channel@5 {
+            reg = <5>;
+          };
+          phys0_6: channel@6 {
+            reg = <6>;
+          };
+          phys0_7: channel@7 {
+            reg = <7>;
+          };
+        };
+        pse-pis {
+          #address-cells = <1>;
+          #size-cells = <0>;
+          pse_pi0: pse-pi@0 {
+            reg = <0>;
+            #pse-cells = <0>;
+            pairset-names = "alternative-a", "alternative-b";
+            pairsets = <&phys0_0>, <&phys0_1>;
+            polarity-supported = "MDI-X", "S";
+            vpwr-supply = <&reg_pse>;
+          };
+          pse_pi1: pse-pi@1 {
+            reg = <1>;
+            #pse-cells = <0>;
+            pairset-names = "alternative-a", "alternative-b";
+            pairsets = <&phys0_2>, <&phys0_3>;
+            polarity-supported = "MDI-X", "S";
+            vpwr-supply = <&reg_pse>;
+          };
+          pse_pi2: pse-pi@2 {
+            reg = <2>;
+            #pse-cells = <0>;
+            pairset-names = "alternative-a", "alternative-b";
+            pairsets = <&phys0_4>, <&phys0_5>;
+            polarity-supported = "MDI-X", "S";
+            vpwr-supply = <&reg_pse>;
+          };
+          pse_pi3: pse-pi@3 {
+            reg = <3>;
+            #pse-cells = <0>;
+            pairset-names = "alternative-a", "alternative-b";
+            pairsets = <&phys0_6>, <&phys0_7>;
+            polarity-supported = "MDI-X", "S";
+            vpwr-supply = <&reg_pse>;
+          };
+        };
+      };
+    };
-- 
2.43.0

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

[Piotr Kubik]

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.

Signed-off-by: Piotr Kubik <piotr.kubik@adtran.com>
---
 drivers/net/pse-pd/Kconfig  |  10 +
 drivers/net/pse-pd/Makefile |   1 +
 drivers/net/pse-pd/si3474.c | 649 ++++++++++++++++++++++++++++++++++++
 3 files changed, 660 insertions(+)
 create mode 100644 drivers/net/pse-pd/si3474.c

diff --git a/drivers/net/pse-pd/Kconfig b/drivers/net/pse-pd/Kconfig
index 7fab916a7f46..d1b100eb8c52 100644
--- a/drivers/net/pse-pd/Kconfig
+++ b/drivers/net/pse-pd/Kconfig
@@ -32,6 +32,16 @@ config PSE_PD692X0
 	  To compile this driver as a module, choose M here: the
 	  module will be called pd692x0.
 
+config PSE_SI3474
+	tristate "Si3474 PSE controller"
+	depends on I2C
+	help
+	  This module provides support for Si3474 regulator based Ethernet
+	  Power Sourcing Equipment.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called si3474.
+
 config PSE_TPS23881
 	tristate "TPS23881 PSE controller"
 	depends on I2C
diff --git a/drivers/net/pse-pd/Makefile b/drivers/net/pse-pd/Makefile
index 9d2898b36737..cc78f7ea7f5f 100644
--- a/drivers/net/pse-pd/Makefile
+++ b/drivers/net/pse-pd/Makefile
@@ -5,4 +5,5 @@ obj-$(CONFIG_PSE_CONTROLLER) += pse_core.o
 
 obj-$(CONFIG_PSE_REGULATOR) += pse_regulator.o
 obj-$(CONFIG_PSE_PD692X0) += pd692x0.o
+obj-$(CONFIG_PSE_SI3474) += si3474.o
 obj-$(CONFIG_PSE_TPS23881) += tps23881.o
diff --git a/drivers/net/pse-pd/si3474.c b/drivers/net/pse-pd/si3474.c
new file mode 100644
index 000000000000..7c21b475ca1a
--- /dev/null
+++ b/drivers/net/pse-pd/si3474.c
@@ -0,0 +1,649 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Driver for the Skyworks Si3474 PoE PSE Controller
+ *
+ * Chip Architecture & Terminology:
+ *
+ * The Si3474 is a single-chip PoE PSE controller managing 8 physical power
+ * delivery channels. Internally, it's structured into two logical "Quads".
+ *
+ * Quad 0: Manages physical channels ('ports' in datasheet) 0, 1, 2, 3
+ * Quad 1: Manages physical channels ('ports' in datasheet) 4, 5, 6, 7
+ *
+ * Each Quad is accessed via a separate I2C address. The base address range is
+ * set by hardware pins A1-A4, and the specific address selects Quad 0 (usually
+ * the lower/even address) or Quad 1 (usually the higher/odd address).
+ * See datasheet Table 2.2 for the address mapping.
+ *
+ * While the Quads manage channel-specific operations, the Si3474 package has
+ * several resources shared across the entire chip:
+ * - Single RESETb input pin.
+ * - Single INTb output pin (signals interrupts from *either* Quad).
+ * - Single OSS input pin (Emergency Shutdown).
+ * - Global I2C Address (0x7F) used for firmware updates.
+ * - Global status monitoring (Temperature, VDD/VPWR Undervoltage Lockout).
+ *
+ * Driver Architecture:
+ *
+ * To handle the mix of per-Quad access and shared resources correctly, this
+ * driver treats the entire Si3474 package as one logical device. The driver
+ * instance associated with the primary I2C address (Quad 0) takes ownership.
+ * It discovers and manages the I2C client for the secondary address (Quad 1).
+ * This primary instance handles shared resources like IRQ management and
+ * registers a single PSE controller device representing all logical PIs.
+ * Internal functions route I2C commands to the appropriate Quad's i2c_client
+ * based on the target channel or PI.
+ *
+ * Terminology Mapping:
+ *
+ * - "PI" (Power Interface): Refers to the logical PSE port as defined by
+ * IEEE 802.3 (typically corresponds to an RJ45 connector). This is the
+ * `id` (0-7) used in the pse_controller_ops.
+ * - "Channel": Refers to one of the 8 physical power control paths within
+ * the Si3474 chip itself (hardware channels 0-7). This terminology is
+ * used internally within the driver to avoid confusion with 'ports'.
+ * - "Quad": One of the two internal 4-channel management units within the
+ * Si3474, each accessed via its own I2C address.
+ *
+ * Relationship:
+ * - A 2-Pair PoE PI uses 1 Channel.
+ * - A 4-Pair PoE PI uses 2 Channels.
+ *
+ * ASCII Schematic:
+ *
+ * +-----------------------------------------------------+
+ * |                    Si3474 Chip                      |
+ * |                                                     |
+ * | +---------------------+     +---------------------+ |
+ * | |      Quad 0         |     |      Quad 1         | |
+ * | | Channels 0, 1, 2, 3 |     | Channels 4, 5, 6, 7 | |
+ * | +----------^----------+     +-------^-------------+ |
+ * | I2C Addr 0 |                        | I2C Addr 1    |
+ * |            +------------------------+               |
+ * | (Primary Driver Instance) (Managed by Primary)      |
+ * |                                                     |
+ * | Shared Resources (affect whole chip):               |
+ * |  - Single INTb Output -> Handled by Primary         |
+ * |  - Single RESETb Input                              |
+ * |  - Single OSS Input   -> Handled by Primary         |
+ * |  - Global I2C Addr (0x7F) for Firmware Update       |
+ * |  - Global Status (Temp, VDD/VPWR UVLO)              |
+ * +-----------------------------------------------------+
+ *        |   |   |   |        |   |   |   |
+ *        Ch0 Ch1 Ch2 Ch3      Ch4 Ch5 Ch6 Ch7  (Physical Channels)
+ *
+ * Example Mapping (Logical PI to Physical Channel(s)):
+ * * 2-Pair Mode (8 PIs):
+ * PI 0 -> Ch 0
+ * PI 1 -> Ch 1
+ * ...
+ * PI 7 -> Ch 7
+ * * 4-Pair Mode (4 PIs):
+ * PI 0 -> Ch 0 + Ch 1  (Managed via Quad 0 Addr)
+ * PI 1 -> Ch 2 + Ch 3  (Managed via Quad 0 Addr)
+ * PI 2 -> Ch 4 + Ch 5  (Managed via Quad 1 Addr)
+ * PI 3 -> Ch 6 + Ch 7  (Managed via Quad 1 Addr)
+ * (Note: Actual mapping depends on Device Tree and PORT_REMAP config)
+ */
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pse-pd/pse.h>
+
+#define SI3474_MAX_CHANS 8
+
+#define MANUFACTURER_ID 0x08
+#define IC_ID 0x05
+#define SI3474_DEVICE_ID (MANUFACTURER_ID << 3 | IC_ID)
+
+/* Misc registers */
+#define VENDOR_IC_ID_REG 0x1B
+#define TEMPERATURE_REG 0x2C
+#define FIRMWARE_REVISION_REG 0x41
+#define CHIP_REVISION_REG 0x43
+
+/* Main status registers */
+#define POWER_STATUS_REG 0x10
+#define PORT_MODE_REG 0x12
+#define PB_POWER_ENABLE_REG 0x19
+
+/* PORTn Current */
+#define PORT1_CURRENT_LSB_REG 0x30
+
+/* PORTn Current [mA], return in [nA] */
+/* 1000 * ((PORTn_CURRENT_MSB << 8) + PORTn_CURRENT_LSB) / 16384 */
+#define SI3474_NA_STEP (1000 * 1000 * 1000 / 16384)
+
+/* VPWR Voltage */
+#define VPWR_LSB_REG 0x2E
+#define VPWR_MSB_REG 0x2F
+
+/* PORTn Voltage */
+#define PORT1_VOLTAGE_LSB_REG 0x32
+
+/* VPWR Voltage [V], return in [uV] */
+/* 60 * (( VPWR_MSB << 8) + VPWR_LSB) / 16384 */
+#define SI3474_UV_STEP (1000 * 1000 * 60 / 16384)
+
+struct si3474_pi_desc {
+	u8 chan[2];
+	bool is_4p;
+};
+
+struct si3474_priv {
+	struct i2c_client *client[2];
+	struct pse_controller_dev pcdev;
+	struct device_node *np;
+	struct si3474_pi_desc pi[SI3474_MAX_CHANS];
+};
+
+static struct si3474_priv *to_si3474_priv(struct pse_controller_dev *pcdev)
+{
+	return container_of(pcdev, struct si3474_priv, pcdev);
+}
+
+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;
+
+	chan0 = priv->pi[id].chan[0];
+	chan1 = priv->pi[id].chan[1];
+
+	if (chan0 < 4)
+		client = priv->client[0];
+	else
+		client = priv->client[1];
+
+	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 & (0x03 << (2 * (chan0 % 4)))) |
+		      (ret & (0x03 << (2 * (chan1 % 4))))) != 0;
+
+	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_get_pw_status(struct pse_controller_dev *pcdev, int id,
+				   struct pse_pw_status *pw_status)
+{
+	struct si3474_priv *priv = to_si3474_priv(pcdev);
+	struct i2c_client *client;
+	bool delivering = false;
+	u8 chan0, chan1;
+	s32 ret;
+
+	if (id >= SI3474_MAX_CHANS)
+		return -ERANGE;
+
+	chan0 = priv->pi[id].chan[0];
+	chan1 = priv->pi[id].chan[1];
+
+	if (chan0 < 4)
+		client = priv->client[0];
+	else
+		client = priv->client[1];
+
+	ret = i2c_smbus_read_byte_data(client, POWER_STATUS_REG);
+	if (ret < 0) {
+		pw_status->c33_pw_status = ETHTOOL_C33_PSE_PW_D_STATUS_UNKNOWN;
+		return ret;
+	}
+
+	delivering = (ret & (BIT((chan0 % 4) + 4) | BIT((chan1 % 4) + 4))) != 0;
+
+	if (delivering)
+		pw_status->c33_pw_status =
+			ETHTOOL_C33_PSE_PW_D_STATUS_DELIVERING;
+	else
+		pw_status->c33_pw_status = ETHTOOL_C33_PSE_PW_D_STATUS_DISABLED;
+
+	return 0;
+}
+
+/* Parse pse-pis subnode into chan array of si3474_priv */
+static int si3474_get_of_channels(struct si3474_priv *priv)
+{
+	struct device_node *pse_node;
+	struct pse_pi *pi;
+	u32 pi_no, chan_id;
+	s8 pairset_cnt;
+	s32 ret = 0;
+
+	pse_node = of_get_child_by_name(priv->np, "pse-pis");
+	if (!pse_node) {
+		dev_warn(&priv->client[0]->dev,
+			 "Unable to parse DT PSE power interface matrix, no pse-pis node\n");
+		return -EINVAL;
+	}
+
+	for_each_child_of_node_scoped(pse_node, node) {
+		if (!of_node_name_eq(node, "pse-pi"))
+			continue;
+
+		ret = of_property_read_u32(node, "reg", &pi_no);
+		if (ret) {
+			dev_err(&priv->client[0]->dev,
+				"Failed to read pse-pi reg property\n");
+			goto out;
+		}
+		if (pi_no >= SI3474_MAX_CHANS) {
+			dev_err(&priv->client[0]->dev,
+				"Invalid power interface number %u\n", pi_no);
+			ret = -EINVAL;
+			goto out;
+		}
+
+		pairset_cnt = of_property_count_elems_of_size(node, "pairsets",
+							      sizeof(u32));
+		if (!pairset_cnt) {
+			dev_err(&priv->client[0]->dev,
+				"Failed to get pairsets property\n");
+			goto out;
+		}
+
+		pi = &priv->pcdev.pi[pi_no];
+		if (!pi->pairset[0].np) {
+			dev_err(&priv->client[0]->dev,
+				"Missing pairset reference, power interface: %u\n",
+				pi_no);
+			ret = -EINVAL;
+			goto out;
+		}
+
+		ret = of_property_read_u32(pi->pairset[0].np, "reg", &chan_id);
+		if (ret) {
+			dev_err(&priv->client[0]->dev,
+				"Failed to read channel reg property, ret:%d\n",
+				ret);
+			goto out;
+		}
+		priv->pi[pi_no].chan[0] = chan_id;
+		priv->pi[pi_no].is_4p = FALSE;
+
+		if (pairset_cnt == 2) {
+			if (!pi->pairset[1].np) {
+				dev_err(&priv->client[0]->dev,
+					"Missing pairset reference, power interface: %u\n",
+					pi_no);
+				ret = -EINVAL;
+				goto out;
+			}
+
+			ret = of_property_read_u32(pi->pairset[1].np, "reg",
+						   &chan_id);
+			if (ret) {
+				dev_err(&priv->client[0]->dev,
+					"Failed to read channel reg property\n");
+				goto out;
+			}
+			priv->pi[pi_no].chan[1] = chan_id;
+			priv->pi[pi_no].is_4p = TRUE;
+		} else {
+			dev_err(&priv->client[0]->dev,
+				"Number of pairsets incorrect - only 4p configurations supported\n");
+			ret = -EINVAL;
+			goto out;
+		}
+	}
+
+out:
+	of_node_put(pse_node);
+	return ret;
+}
+
+static int si3474_setup_pi_matrix(struct pse_controller_dev *pcdev)
+{
+	struct si3474_priv *priv = to_si3474_priv(pcdev);
+	s32 ret;
+
+	ret = si3474_get_of_channels(priv);
+	if (ret < 0) {
+		dev_warn(&priv->client[0]->dev,
+			 "Unable to parse DT PSE power interface matrix\n");
+	}
+	return ret;
+}
+
+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;
+
+	chan0 = priv->pi[id].chan[0];
+	chan1 = priv->pi[id].chan[1];
+
+	if (chan0 < 4)
+		client = priv->client[0];
+	else
+		client = priv->client[1];
+
+	/* Release pi from shutdown */
+	ret = i2c_smbus_read_byte_data(client, PORT_MODE_REG);
+	if (ret < 0)
+		return ret;
+
+	val = (u8)ret;
+	val |= (0x03 << (2 * (chan0 % 4)));
+	val |= (0x03 << (2 * (chan1 % 4)));
+
+	ret = i2c_smbus_write_byte_data(client, PORT_MODE_REG, val);
+	if (ret)
+		return ret;
+
+	/* Give time for transition to complete */
+	ssleep(1);
+
+	/* Trigger pi to power up */
+	val = (BIT(chan0 % 4) | BIT(chan1 % 4));
+	ret = i2c_smbus_write_byte_data(client, PB_POWER_ENABLE_REG, val);
+
+	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;
+
+	chan0 = priv->pi[id].chan[0];
+	chan1 = priv->pi[id].chan[1];
+
+	if (chan0 < 4)
+		client = priv->client[0];
+	else
+		client = priv->client[1];
+
+	/* Trigger pi to power down */
+	val = (BIT((chan0 % 4) + 4) | BIT((chan1 % 4) + 4));
+	ret = i2c_smbus_write_byte_data(client, PB_POWER_ENABLE_REG, val);
+
+	/* Shutdown pi */
+	ret = i2c_smbus_read_byte_data(client, PORT_MODE_REG);
+	if (ret < 0)
+		return ret;
+
+	val = (u8)ret;
+	val &= ~(0x03 << (2 * (chan0 % 4)));
+	val &= ~(0x03 << (2 * (chan1 % 4)));
+
+	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;
+
+	if (chan < 4)
+		client = priv->client[0];
+	else
+		client = priv->client[1];
+
+	/* Registers 0x30 to 0x3d */
+	reg = PORT1_CURRENT_LSB_REG + (chan % 4) * 4;
+
+	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;
+
+	if (chan < 4)
+		client = priv->client[0];
+	else
+		client = priv->client[1];
+
+	/* Registers 0x32 to 0x3f */
+	reg = PORT1_VOLTAGE_LSB_REG + (chan % 4) * 4;
+
+	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;
+
+	chan0 = priv->pi[id].chan[0];
+	chan1 = priv->pi[id].chan[1];
+
+	if (chan0 < 4)
+		client = priv->client[0];
+	else
+		client = priv->client[1];
+
+	/* 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 & BIT(chan0 % 4))
+		ret = si3474_pi_get_chan_voltage(priv, chan0);
+	else if (ret & 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;
+
+	if (id >= SI3474_MAX_CHANS)
+		return -ERANGE;
+
+	ret = si3474_pi_get_voltage(&priv->pcdev, id);
+	if (ret < 0)
+		return ret;
+	uV = ret;
+
+	chan0 = priv->pi[id].chan[0];
+	chan1 = priv->pi[id].chan[1];
+
+	ret = si3474_pi_get_chan_current(priv, chan0);
+	if (ret < 0)
+		return ret;
+	uA = ret;
+
+	ret = si3474_pi_get_chan_current(priv, chan1);
+	if (ret < 0)
+		return ret;
+	uA += ret;
+
+	tmp_64 = uV;
+	tmp_64 *= uA;
+	/* mW = uV * uA / 1000000000 */
+	return DIV_ROUND_CLOSEST_ULL(tmp_64, 1000000000);
+}
+
+static const struct pse_controller_ops si3474_ops = {
+	.setup_pi_matrix = si3474_setup_pi_matrix,
+	.pi_enable = si3474_pi_enable,
+	.pi_disable = si3474_pi_disable,
+	.pi_get_actual_pw = si3474_pi_get_actual_pw,
+	.pi_get_voltage = si3474_pi_get_voltage,
+	.pi_get_admin_state = si3474_pi_get_admin_state,
+	.pi_get_pw_status = si3474_pi_get_pw_status,
+};
+
+static void si3474_ancillary_i2c_remove(void *data)
+{
+	struct i2c_client *client = data;
+
+	i2c_unregister_device(client);
+}
+
+static int si3474_i2c_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct si3474_priv *priv;
+	s32 ret;
+	u8 fw_version;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+		dev_err(dev, "i2c check functionality failed\n");
+		return -ENXIO;
+	}
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	ret = i2c_smbus_read_byte_data(client, VENDOR_IC_ID_REG);
+	if (ret < 0)
+		return ret;
+
+	if (ret != SI3474_DEVICE_ID) {
+		dev_err(dev, "Wrong device ID: 0x%x\n", ret);
+		return -ENXIO;
+	}
+
+	ret = i2c_smbus_read_byte_data(client, FIRMWARE_REVISION_REG);
+	if (ret < 0)
+		return ret;
+	fw_version = ret;
+
+	ret = i2c_smbus_read_byte_data(client, CHIP_REVISION_REG);
+	if (ret < 0)
+		return ret;
+
+	dev_dbg(dev, "Chip revision: 0x%x, firmware version: 0x%x\n",
+		ret, fw_version);
+
+	priv->client[0] = client;
+	i2c_set_clientdata(client, priv);
+
+	priv->client[1] = i2c_new_ancillary_device(priv->client[0], "secondary",
+						   priv->client[0]->addr + 1);
+	if (IS_ERR(priv->client[1]))
+		return PTR_ERR(priv->client[1]);
+
+	ret = devm_add_action_or_reset(dev, si3474_ancillary_i2c_remove, priv->client[1]);
+	if (ret < 0) {
+		dev_err(&priv->client[1]->dev, "Cannot register remove callback\n");
+		return ret;
+	}
+
+	ret = i2c_smbus_read_byte_data(priv->client[1], VENDOR_IC_ID_REG);
+	if (ret < 0) {
+		dev_err(&priv->client[1]->dev, "Cannot access secondary PSE controller\n");
+		return ret;
+	}
+
+	if (ret != SI3474_DEVICE_ID) {
+		dev_err(&priv->client[1]->dev,
+			"Wrong device ID for secondary PSE controller: 0x%x\n", ret);
+		return -ENXIO;
+	}
+
+	priv->np = dev->of_node;
+	priv->pcdev.owner = THIS_MODULE;
+	priv->pcdev.ops = &si3474_ops;
+	priv->pcdev.dev = dev;
+	priv->pcdev.types = ETHTOOL_PSE_C33;
+	priv->pcdev.nr_lines = SI3474_MAX_CHANS;
+
+	ret = devm_pse_controller_register(dev, &priv->pcdev);
+	if (ret) {
+		dev_err(dev, "Failed to register PSE controller: 0x%x\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct i2c_device_id si3474_id[] = {
+	{ "si3474" },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, si3474_id);
+
+static const struct of_device_id si3474_of_match[] = {
+	{
+		.compatible = "skyworks,si3474",
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, si3474_of_match);
+
+static struct i2c_driver si3474_driver = {
+	.probe = si3474_i2c_probe,
+	.id_table = si3474_id,
+	.driver = {
+		.name = "si3474",
+		.of_match_table = si3474_of_match,
+	},
+};
+module_i2c_driver(si3474_driver);
+
+MODULE_AUTHOR("Piotr Kubik <piotr.kubik@adtran.com>");
+MODULE_DESCRIPTION("Skyworks Si3474 PoE PSE Controller driver");
+MODULE_LICENSE("GPL");
-- 
2.43.0

Re: [PATCH net-next v3 1/2] dt-bindings: net: pse-pd: Add bindings for Si3474 PSE controller

[Krzysztof Kozlowski]

On 16/05/2025 15:07, Piotr Kubik wrote:
> From: Piotr Kubik <piotr.kubik@adtran.com>
> 
> Add the Si3474 I2C Power Sourcing Equipment controller device tree
> bindings documentation.
> 
> Signed-off-by: Piotr Kubik <piotr.kubik@adtran.com>
> ---

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

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

[ALOK TIWARI]

On 16-05-2025 18:37, Piotr Kubik 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.
> 
> Signed-off-by: Piotr Kubik <piotr.kubik@adtran.com>
> ---
>   drivers/net/pse-pd/Kconfig  |  10 +
>   drivers/net/pse-pd/Makefile |   1 +
>   drivers/net/pse-pd/si3474.c | 649 ++++++++++++++++++++++++++++++++++++
>   3 files changed, 660 insertions(+)
>   create mode 100644 drivers/net/pse-pd/si3474.c
> 
> diff --git a/drivers/net/pse-pd/Kconfig b/drivers/net/pse-pd/Kconfig
> index 7fab916a7f46..d1b100eb8c52 100644
> --- a/drivers/net/pse-pd/Kconfig
> +++ b/drivers/net/pse-pd/Kconfig
> @@ -32,6 +32,16 @@ config PSE_PD692X0
>   	  To compile this driver as a module, choose M here: the
>   	  module will be called pd692x0.
>   
> +config PSE_SI3474
> +	tristate "Si3474 PSE controller"
> +	depends on I2C
> +	help
> +	  This module provides support for Si3474 regulator based Ethernet
> +	  Power Sourcing Equipment.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called si3474.
> +
>   config PSE_TPS23881
>   	tristate "TPS23881 PSE controller"
>   	depends on I2C
> diff --git a/drivers/net/pse-pd/Makefile b/drivers/net/pse-pd/Makefile
> index 9d2898b36737..cc78f7ea7f5f 100644
> --- a/drivers/net/pse-pd/Makefile
> +++ b/drivers/net/pse-pd/Makefile
> @@ -5,4 +5,5 @@ obj-$(CONFIG_PSE_CONTROLLER) += pse_core.o
>   
>   obj-$(CONFIG_PSE_REGULATOR) += pse_regulator.o
>   obj-$(CONFIG_PSE_PD692X0) += pd692x0.o
> +obj-$(CONFIG_PSE_SI3474) += si3474.o
>   obj-$(CONFIG_PSE_TPS23881) += tps23881.o
> diff --git a/drivers/net/pse-pd/si3474.c b/drivers/net/pse-pd/si3474.c
> new file mode 100644
> index 000000000000..7c21b475ca1a
> --- /dev/null
> +++ b/drivers/net/pse-pd/si3474.c
> @@ -0,0 +1,649 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Driver for the Skyworks Si3474 PoE PSE Controller
> + *
> + * Chip Architecture & Terminology:
> + *
> + * The Si3474 is a single-chip PoE PSE controller managing 8 physical power
> + * delivery channels. Internally, it's structured into two logical "Quads".
> + *
> + * Quad 0: Manages physical channels ('ports' in datasheet) 0, 1, 2, 3
> + * Quad 1: Manages physical channels ('ports' in datasheet) 4, 5, 6, 7
> + *
> + * Each Quad is accessed via a separate I2C address. The base address range is
> + * set by hardware pins A1-A4, and the specific address selects Quad 0 (usually
> + * the lower/even address) or Quad 1 (usually the higher/odd address).
> + * See datasheet Table 2.2 for the address mapping.
> + *
> + * While the Quads manage channel-specific operations, the Si3474 package has
> + * several resources shared across the entire chip:
> + * - Single RESETb input pin.
> + * - Single INTb output pin (signals interrupts from *either* Quad).
> + * - Single OSS input pin (Emergency Shutdown).
> + * - Global I2C Address (0x7F) used for firmware updates.
> + * - Global status monitoring (Temperature, VDD/VPWR Undervoltage Lockout).
> + *
> + * Driver Architecture:
> + *
> + * To handle the mix of per-Quad access and shared resources correctly, this
> + * driver treats the entire Si3474 package as one logical device. The driver
> + * instance associated with the primary I2C address (Quad 0) takes ownership.
> + * It discovers and manages the I2C client for the secondary address (Quad 1).
> + * This primary instance handles shared resources like IRQ management and
> + * registers a single PSE controller device representing all logical PIs.
> + * Internal functions route I2C commands to the appropriate Quad's i2c_client
> + * based on the target channel or PI.
> + *
> + * Terminology Mapping:
> + *
> + * - "PI" (Power Interface): Refers to the logical PSE port as defined by
> + * IEEE 802.3 (typically corresponds to an RJ45 connector). This is the
> + * `id` (0-7) used in the pse_controller_ops.
> + * - "Channel": Refers to one of the 8 physical power control paths within
> + * the Si3474 chip itself (hardware channels 0-7). This terminology is
> + * used internally within the driver to avoid confusion with 'ports'.
> + * - "Quad": One of the two internal 4-channel management units within the
> + * Si3474, each accessed via its own I2C address.
> + *
> + * Relationship:
> + * - A 2-Pair PoE PI uses 1 Channel.
> + * - A 4-Pair PoE PI uses 2 Channels.
> + *
> + * ASCII Schematic:
> + *
> + * +-----------------------------------------------------+
> + * |                    Si3474 Chip                      |
> + * |                                                     |
> + * | +---------------------+     +---------------------+ |
> + * | |      Quad 0         |     |      Quad 1         | |
> + * | | Channels 0, 1, 2, 3 |     | Channels 4, 5, 6, 7 | |
> + * | +----------^----------+     +-------^-------------+ |
> + * | I2C Addr 0 |                        | I2C Addr 1    |
> + * |            +------------------------+               |
> + * | (Primary Driver Instance) (Managed by Primary)      |
> + * |                                                     |
> + * | Shared Resources (affect whole chip):               |
> + * |  - Single INTb Output -> Handled by Primary         |
> + * |  - Single RESETb Input                              |
> + * |  - Single OSS Input   -> Handled by Primary         |
> + * |  - Global I2C Addr (0x7F) for Firmware Update       |
> + * |  - Global Status (Temp, VDD/VPWR UVLO)              |
> + * +-----------------------------------------------------+
> + *        |   |   |   |        |   |   |   |
> + *        Ch0 Ch1 Ch2 Ch3      Ch4 Ch5 Ch6 Ch7  (Physical Channels)
> + *
> + * Example Mapping (Logical PI to Physical Channel(s)):
> + * * 2-Pair Mode (8 PIs):
> + * PI 0 -> Ch 0
> + * PI 1 -> Ch 1
> + * ...
> + * PI 7 -> Ch 7
> + * * 4-Pair Mode (4 PIs):
> + * PI 0 -> Ch 0 + Ch 1  (Managed via Quad 0 Addr)
> + * PI 1 -> Ch 2 + Ch 3  (Managed via Quad 0 Addr)
> + * PI 2 -> Ch 4 + Ch 5  (Managed via Quad 1 Addr)
> + * PI 3 -> Ch 6 + Ch 7  (Managed via Quad 1 Addr)
> + * (Note: Actual mapping depends on Device Tree and PORT_REMAP config)
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pse-pd/pse.h>
> +
> +#define SI3474_MAX_CHANS 8

SI3474_MAX_CHANS, this could be sound like changes or chance

> +
> +#define MANUFACTURER_ID 0x08
> +#define IC_ID 0x05
> +#define SI3474_DEVICE_ID (MANUFACTURER_ID << 3 | IC_ID)
> +
> +/* Misc registers */
> +#define VENDOR_IC_ID_REG 0x1B
> +#define TEMPERATURE_REG 0x2C
> +#define FIRMWARE_REVISION_REG 0x41
> +#define CHIP_REVISION_REG 0x43
> +
> +/* Main status registers */
> +#define POWER_STATUS_REG 0x10
> +#define PORT_MODE_REG 0x12
> +#define PB_POWER_ENABLE_REG 0x19
> +
> +/* PORTn Current */
> +#define PORT1_CURRENT_LSB_REG 0x30
> +
> +/* PORTn Current [mA], return in [nA] */
> +/* 1000 * ((PORTn_CURRENT_MSB << 8) + PORTn_CURRENT_LSB) / 16384 */
> +#define SI3474_NA_STEP (1000 * 1000 * 1000 / 16384)
> +
> +/* VPWR Voltage */
> +#define VPWR_LSB_REG 0x2E
> +#define VPWR_MSB_REG 0x2F
> +
> +/* PORTn Voltage */
> +#define PORT1_VOLTAGE_LSB_REG 0x32
> +
> +/* VPWR Voltage [V], return in [uV] */
> +/* 60 * (( VPWR_MSB << 8) + VPWR_LSB) / 16384 */
> +#define SI3474_UV_STEP (1000 * 1000 * 60 / 16384)
> +
> +struct si3474_pi_desc {
> +	u8 chan[2];

here chan does not sound smooth -> channels[] ?
especially for an array or list

> +	bool is_4p;
> +};
> +
> +struct si3474_priv {
> +	struct i2c_client *client[2];
> +	struct pse_controller_dev pcdev;
> +	struct device_node *np;
> +	struct si3474_pi_desc pi[SI3474_MAX_CHANS];
> +};

Thanks,
Alok

Re: [PATCH net-next v3 1/2] dt-bindings: net: pse-pd: Add bindings for Si3474 PSE controller

[Kory Maincent]

On Fri, 16 May 2025 13:07:05 +0000
Piotr Kubik <piotr.kubik@adtran.com> wrote:

> From: Piotr Kubik <piotr.kubik@adtran.com>
> 
> Add the Si3474 I2C Power Sourcing Equipment controller device tree
> bindings documentation.
> 
> Signed-off-by: Piotr Kubik <piotr.kubik@adtran.com>

Reviewed-by: Kory Maincent <kory.maincent@bootlin.com>

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

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

[Kory Maincent]

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.

By curiosity, I suppose your hardware have only PoE4 PIs. Could you support and
test 2p configuration by only configuring 2 of the 4 pairs on one PI?

Maybe it could be done on a second stage if you prefer.

> 
> Signed-off-by: Piotr Kubik <piotr.kubik@adtran.com>

...

> +	is_enabled = ((ret & (0x03 << (2 * (chan0 % 4)))) |
> +		      (ret & (0x03 << (2 * (chan1 % 4))))) != 0;

There are precedence in the operators. I don't think you need that much of
parenthesis.
This should do the work:
is_enabled = (ret & 0x03 << chan0 % 4 * 2) |
             (ret & 0x03 << chan1 % 4 * 2) != 0;

Also I saw that this kind of calculation is made several times. Maybe you could
add a helper with documentation like I did:
https://elixir.bootlin.com/linux/v6.14.7/source/drivers/net/pse-pd/tps23881.c#L79 

> +/* Parse pse-pis subnode into chan array of si3474_priv */
> +static int si3474_get_of_channels(struct si3474_priv *priv)
> +{
> +	struct device_node *pse_node;
> +	struct pse_pi *pi;
> +	u32 pi_no, chan_id;
> +	s8 pairset_cnt;
> +	s32 ret = 0;
> +
> +	pse_node = of_get_child_by_name(priv->np, "pse-pis");
> +	if (!pse_node) {
> +		dev_warn(&priv->client[0]->dev,
> +			 "Unable to parse DT PSE power interface matrix, no
> pse-pis node\n");
> +		return -EINVAL;
> +	}

You should not parse the pse-pis node and subnodes, it is already done before
the setup_pi_matrix ops call in the core framework.
https://elixir.bootlin.com/linux/v6.14.7/source/drivers/net/pse-pd/pse_core.c#L131

You should rather use directly the pcdev->pi[x] table. You have to match the
the phandle save in pcdev->pi[x].pairset[0/1].np to your channel device node
and set up the hardware matrix accordingly.

That's good to have another development on PSE, this shows me that documentation
are missing or not enough in PSE core like on this ops. I will update it when I
have time.

> +
> +	for_each_child_of_node_scoped(pse_node, node) {
> +		if (!of_node_name_eq(node, "pse-pi"))
> +			continue;
> +
> +		ret = of_property_read_u32(node, "reg", &pi_no);
> +		if (ret) {
> +			dev_err(&priv->client[0]->dev,
> +				"Failed to read pse-pi reg property\n");
> +			goto out;
> +		}
> +		if (pi_no >= SI3474_MAX_CHANS) {
> +			dev_err(&priv->client[0]->dev,
> +				"Invalid power interface number %u\n",
> pi_no);
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +

...

> +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;
> +
> +	chan0 = priv->pi[id].chan[0];
> +	chan1 = priv->pi[id].chan[1];
> +
> +	if (chan0 < 4)
> +		client = priv->client[0];
> +	else
> +		client = priv->client[1];
> +
> +	/* Release pi from shutdown */
> +	ret = i2c_smbus_read_byte_data(client, PORT_MODE_REG);
> +	if (ret < 0)
> +		return ret;
> +
> +	val = (u8)ret;
> +	val |= (0x03 << (2 * (chan0 % 4)));
> +	val |= (0x03 << (2 * (chan1 % 4)));

Same calculation as before, use a helper as said before.

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

> +
> +	/* Trigger pi to power up */
> +	val = (BIT(chan0 % 4) | BIT(chan1 % 4));
> +	ret = i2c_smbus_write_byte_data(client, PB_POWER_ENABLE_REG, val);
> +
> +	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;
> +
> +	chan0 = priv->pi[id].chan[0];
> +	chan1 = priv->pi[id].chan[1];
> +
> +	if (chan0 < 4)
> +		client = priv->client[0];
> +	else
> +		client = priv->client[1];
> +
> +	/* Trigger pi to power down */
> +	val = (BIT((chan0 % 4) + 4) | BIT((chan1 % 4) + 4));

This calculation is also used several times. Please use a helper with
documentation.

> +	ret = i2c_smbus_write_byte_data(client, PB_POWER_ENABLE_REG, val);
> +
> +	/* Shutdown pi */
> +	ret = i2c_smbus_read_byte_data(client, PORT_MODE_REG);
> +	if (ret < 0)
> +		return ret;
> +
> +	val = (u8)ret;
> +	val &= ~(0x03 << (2 * (chan0 % 4)));
> +	val &= ~(0x03 << (2 * (chan1 % 4)));

Use a helper.

> +
> +	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;
> +
> +	if (chan < 4)
> +		client = priv->client[0];
> +	else
> +		client = priv->client[1];
> +
> +	/* Registers 0x30 to 0x3d */
> +	reg = PORT1_CURRENT_LSB_REG + (chan % 4) * 4;

Idem

> +
> +	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;
> +
> +	if (chan < 4)
> +		client = priv->client[0];
> +	else
> +		client = priv->client[1];
> +
> +	/* Registers 0x32 to 0x3f */
> +	reg = PORT1_VOLTAGE_LSB_REG + (chan % 4) * 4;

Idem

> +
> +	ret = i2c_smbus_read_word_data(client, reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	val = ret * SI3474_UV_STEP;
> +
> +	return (int)val;
> +}

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

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

[Piotr Kubik]

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.
> 
> By curiosity, I suppose your hardware have only PoE4 PIs. Could you support and
> test 2p configuration by only configuring 2 of the 4 pairs on one PI?
> 
> Maybe it could be done on a second stage if you prefer.
> 

Yes, exactly. My PSE is hardware 4p type. Some tweaks needs to be done to test/support 2p PSE.
And, yes, I'd prefer to do it in a second stage as a feature.

>>
>> Signed-off-by: Piotr Kubik <piotr.kubik@adtran.com>
> 
> ...
> 
>> +	is_enabled = ((ret & (0x03 << (2 * (chan0 % 4)))) |
>> +		      (ret & (0x03 << (2 * (chan1 % 4))))) != 0;
> 
> There are precedence in the operators. I don't think you need that much of
> parenthesis.
> This should do the work:
> is_enabled = (ret & 0x03 << chan0 % 4 * 2) |
>              (ret & 0x03 << chan1 % 4 * 2) != 0;

Indeed, I left if for readability of what is going on, but your version looks also readable. 
But I'd keep the most surrounding parenthesis as '!=' has precedence over '|' 

is_enabled = ((ret & 0x03 << chan0 % 4 * 2) |
              (ret & 0x03 << chan1 % 4 * 2)) != 0;
> 
> Also I saw that this kind of calculation is made several times. Maybe you could
> add a helper with documentation like I did:
> https://elixir.bootlin.com/linux/v6.14.7/source/drivers/net/pse-pd/tps23881.c#L79 

Sure, I will. 

>> +/* Parse pse-pis subnode into chan array of si3474_priv */
>> +static int si3474_get_of_channels(struct si3474_priv *priv)
>> +{
>> +	struct device_node *pse_node;
>> +	struct pse_pi *pi;
>> +	u32 pi_no, chan_id;
>> +	s8 pairset_cnt;
>> +	s32 ret = 0;
>> +
>> +	pse_node = of_get_child_by_name(priv->np, "pse-pis");
>> +	if (!pse_node) {
>> +		dev_warn(&priv->client[0]->dev,
>> +			 "Unable to parse DT PSE power interface matrix, no
>> pse-pis node\n");
>> +		return -EINVAL;
>> +	}
> 
> You should not parse the pse-pis node and subnodes, it is already done before
> the setup_pi_matrix ops call in the core framework.
> https://elixir.bootlin.com/linux/v6.14.7/source/drivers/net/pse-pd/pse_core.c#L131
> 
> You should rather use directly the pcdev->pi[x] table. You have to match the
> the phandle save in pcdev->pi[x].pairset[0/1].np to your channel device node
> and set up the hardware matrix accordingly.

OK, I didn't notice that, will try. 

> 
> That's good to have another development on PSE, this shows me that documentation
> are missing or not enough in PSE core like on this ops. I will update it when I
> have time.
> 
>> +
>> +	for_each_child_of_node_scoped(pse_node, node) {
>> +		if (!of_node_name_eq(node, "pse-pi"))
>> +			continue;
>> +
>> +		ret = of_property_read_u32(node, "reg", &pi_no);
>> +		if (ret) {
>> +			dev_err(&priv->client[0]->dev,
>> +				"Failed to read pse-pi reg property\n");
>> +			goto out;
>> +		}
>> +		if (pi_no >= SI3474_MAX_CHANS) {
>> +			dev_err(&priv->client[0]->dev,
>> +				"Invalid power interface number %u\n",
>> pi_no);
>> +			ret = -EINVAL;
>> +			goto out;
>> +		}
>> +
> 
> ...
> 
>> +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;
>> +
>> +	chan0 = priv->pi[id].chan[0];
>> +	chan1 = priv->pi[id].chan[1];
>> +
>> +	if (chan0 < 4)
>> +		client = priv->client[0];
>> +	else
>> +		client = priv->client[1];
>> +
>> +	/* Release pi from shutdown */
>> +	ret = i2c_smbus_read_byte_data(client, PORT_MODE_REG);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	val = (u8)ret;
>> +	val |= (0x03 << (2 * (chan0 % 4)));
>> +	val |= (0x03 << (2 * (chan1 % 4)));
> 
> Same calculation as before, use a helper as said before.
> 
>> +
>> +	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.

>> +
>> +	/* Trigger pi to power up */
>> +	val = (BIT(chan0 % 4) | BIT(chan1 % 4));
>> +	ret = i2c_smbus_write_byte_data(client, PB_POWER_ENABLE_REG, val);
>> +
>> +	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;
>> +
>> +	chan0 = priv->pi[id].chan[0];
>> +	chan1 = priv->pi[id].chan[1];
>> +
>> +	if (chan0 < 4)
>> +		client = priv->client[0];
>> +	else
>> +		client = priv->client[1];
>> +
>> +	/* Trigger pi to power down */
>> +	val = (BIT((chan0 % 4) + 4) | BIT((chan1 % 4) + 4));
> 
> This calculation is also used several times. Please use a helper with
> documentation.
> 
>> +	ret = i2c_smbus_write_byte_data(client, PB_POWER_ENABLE_REG, val);
>> +
>> +	/* Shutdown pi */
>> +	ret = i2c_smbus_read_byte_data(client, PORT_MODE_REG);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	val = (u8)ret;
>> +	val &= ~(0x03 << (2 * (chan0 % 4)));
>> +	val &= ~(0x03 << (2 * (chan1 % 4)));
> 
> Use a helper.
> 
>> +
>> +	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;
>> +
>> +	if (chan < 4)
>> +		client = priv->client[0];
>> +	else
>> +		client = priv->client[1];
>> +
>> +	/* Registers 0x30 to 0x3d */
>> +	reg = PORT1_CURRENT_LSB_REG + (chan % 4) * 4;
> 
> Idem
> 
>> +
>> +	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;
>> +
>> +	if (chan < 4)
>> +		client = priv->client[0];
>> +	else
>> +		client = priv->client[1];
>> +
>> +	/* Registers 0x32 to 0x3f */
>> +	reg = PORT1_VOLTAGE_LSB_REG + (chan % 4) * 4;
> 
> Idem
> 
>> +
>> +	ret = i2c_smbus_read_word_data(client, reg);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	val = ret * SI3474_UV_STEP;
>> +
>> +	return (int)val;
>> +}
> 
> Regards,

Thanks Kory!
/Piotr

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

[Piotr Kubik]

On 5/16/25 23:38, ALOK TIWARI wrote:
> 
> On 16-05-2025 18:37, Piotr Kubik 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.
>>
>> Signed-off-by: Piotr Kubik <piotr.kubik@adtran.com>
>> ---
>>   drivers/net/pse-pd/Kconfig  |  10 +
>>   drivers/net/pse-pd/Makefile |   1 +
>>   drivers/net/pse-pd/si3474.c | 649 ++++++++++++++++++++++++++++++++++++
>>   3 files changed, 660 insertions(+)
>>   create mode 100644 drivers/net/pse-pd/si3474.c
>>
>> diff --git a/drivers/net/pse-pd/Kconfig b/drivers/net/pse-pd/Kconfig
>> index 7fab916a7f46..d1b100eb8c52 100644
>> --- a/drivers/net/pse-pd/Kconfig
>> +++ b/drivers/net/pse-pd/Kconfig
>> @@ -32,6 +32,16 @@ config PSE_PD692X0
>>         To compile this driver as a module, choose M here: the
>>         module will be called pd692x0.
>>
>> +config PSE_SI3474
>> +     tristate "Si3474 PSE controller"
>> +     depends on I2C
>> +     help
>> +       This module provides support for Si3474 regulator based Ethernet
>> +       Power Sourcing Equipment.
>> +
>> +       To compile this driver as a module, choose M here: the
>> +       module will be called si3474.
>> +
>>   config PSE_TPS23881
>>       tristate "TPS23881 PSE controller"
>>       depends on I2C
>> diff --git a/drivers/net/pse-pd/Makefile b/drivers/net/pse-pd/Makefile
>> index 9d2898b36737..cc78f7ea7f5f 100644
>> --- a/drivers/net/pse-pd/Makefile
>> +++ b/drivers/net/pse-pd/Makefile
>> @@ -5,4 +5,5 @@ obj-$(CONFIG_PSE_CONTROLLER) += pse_core.o
>>
>>   obj-$(CONFIG_PSE_REGULATOR) += pse_regulator.o
>>   obj-$(CONFIG_PSE_PD692X0) += pd692x0.o
>> +obj-$(CONFIG_PSE_SI3474) += si3474.o
>>   obj-$(CONFIG_PSE_TPS23881) += tps23881.o
>> diff --git a/drivers/net/pse-pd/si3474.c b/drivers/net/pse-pd/si3474.c
>> new file mode 100644
>> index 000000000000..7c21b475ca1a
>> --- /dev/null
>> +++ b/drivers/net/pse-pd/si3474.c
>> @@ -0,0 +1,649 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Driver for the Skyworks Si3474 PoE PSE Controller
>> + *
>> + * Chip Architecture & Terminology:
>> + *
>> + * The Si3474 is a single-chip PoE PSE controller managing 8 physical power
>> + * delivery channels. Internally, it's structured into two logical "Quads".
>> + *
>> + * Quad 0: Manages physical channels ('ports' in datasheet) 0, 1, 2, 3
>> + * Quad 1: Manages physical channels ('ports' in datasheet) 4, 5, 6, 7
>> + *
>> + * Each Quad is accessed via a separate I2C address. The base address range is
>> + * set by hardware pins A1-A4, and the specific address selects Quad 0 (usually
>> + * the lower/even address) or Quad 1 (usually the higher/odd address).
>> + * See datasheet Table 2.2 for the address mapping.
>> + *
>> + * While the Quads manage channel-specific operations, the Si3474 package has
>> + * several resources shared across the entire chip:
>> + * - Single RESETb input pin.
>> + * - Single INTb output pin (signals interrupts from *either* Quad).
>> + * - Single OSS input pin (Emergency Shutdown).
>> + * - Global I2C Address (0x7F) used for firmware updates.
>> + * - Global status monitoring (Temperature, VDD/VPWR Undervoltage Lockout).
>> + *
>> + * Driver Architecture:
>> + *
>> + * To handle the mix of per-Quad access and shared resources correctly, this
>> + * driver treats the entire Si3474 package as one logical device. The driver
>> + * instance associated with the primary I2C address (Quad 0) takes ownership.
>> + * It discovers and manages the I2C client for the secondary address (Quad 1).
>> + * This primary instance handles shared resources like IRQ management and
>> + * registers a single PSE controller device representing all logical PIs.
>> + * Internal functions route I2C commands to the appropriate Quad's i2c_client
>> + * based on the target channel or PI.
>> + *
>> + * Terminology Mapping:
>> + *
>> + * - "PI" (Power Interface): Refers to the logical PSE port as defined by
>> + * IEEE 802.3 (typically corresponds to an RJ45 connector). This is the
>> + * `id` (0-7) used in the pse_controller_ops.
>> + * - "Channel": Refers to one of the 8 physical power control paths within
>> + * the Si3474 chip itself (hardware channels 0-7). This terminology is
>> + * used internally within the driver to avoid confusion with 'ports'.
>> + * - "Quad": One of the two internal 4-channel management units within the
>> + * Si3474, each accessed via its own I2C address.
>> + *
>> + * Relationship:
>> + * - A 2-Pair PoE PI uses 1 Channel.
>> + * - A 4-Pair PoE PI uses 2 Channels.
>> + *
>> + * ASCII Schematic:
>> + *
>> + * +-----------------------------------------------------+
>> + * |                    Si3474 Chip                      |
>> + * |                                                     |
>> + * | +---------------------+     +---------------------+ |
>> + * | |      Quad 0         |     |      Quad 1         | |
>> + * | | Channels 0, 1, 2, 3 |     | Channels 4, 5, 6, 7 | |
>> + * | +----------^----------+     +-------^-------------+ |
>> + * | I2C Addr 0 |                        | I2C Addr 1    |
>> + * |            +------------------------+               |
>> + * | (Primary Driver Instance) (Managed by Primary)      |
>> + * |                                                     |
>> + * | Shared Resources (affect whole chip):               |
>> + * |  - Single INTb Output -> Handled by Primary         |
>> + * |  - Single RESETb Input                              |
>> + * |  - Single OSS Input   -> Handled by Primary         |
>> + * |  - Global I2C Addr (0x7F) for Firmware Update       |
>> + * |  - Global Status (Temp, VDD/VPWR UVLO)              |
>> + * +-----------------------------------------------------+
>> + *        |   |   |   |        |   |   |   |
>> + *        Ch0 Ch1 Ch2 Ch3      Ch4 Ch5 Ch6 Ch7  (Physical Channels)
>> + *
>> + * Example Mapping (Logical PI to Physical Channel(s)):
>> + * * 2-Pair Mode (8 PIs):
>> + * PI 0 -> Ch 0
>> + * PI 1 -> Ch 1
>> + * ...
>> + * PI 7 -> Ch 7
>> + * * 4-Pair Mode (4 PIs):
>> + * PI 0 -> Ch 0 + Ch 1  (Managed via Quad 0 Addr)
>> + * PI 1 -> Ch 2 + Ch 3  (Managed via Quad 0 Addr)
>> + * PI 2 -> Ch 4 + Ch 5  (Managed via Quad 1 Addr)
>> + * PI 3 -> Ch 6 + Ch 7  (Managed via Quad 1 Addr)
>> + * (Note: Actual mapping depends on Device Tree and PORT_REMAP config)
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/i2c.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pse-pd/pse.h>
>> +
>> +#define SI3474_MAX_CHANS 8
> 
> SI3474_MAX_CHANS, this could be sound like changes or chance
> 

Well, for me it sounds good. 
Moreover, this shortcut is widely used in kernel code for many different types of channels.

>> +
>> +#define MANUFACTURER_ID 0x08
>> +#define IC_ID 0x05
>> +#define SI3474_DEVICE_ID (MANUFACTURER_ID << 3 | IC_ID)
>> +
>> +/* Misc registers */
>> +#define VENDOR_IC_ID_REG 0x1B
>> +#define TEMPERATURE_REG 0x2C
>> +#define FIRMWARE_REVISION_REG 0x41
>> +#define CHIP_REVISION_REG 0x43
>> +
>> +/* Main status registers */
>> +#define POWER_STATUS_REG 0x10
>> +#define PORT_MODE_REG 0x12
>> +#define PB_POWER_ENABLE_REG 0x19
>> +
>> +/* PORTn Current */
>> +#define PORT1_CURRENT_LSB_REG 0x30
>> +
>> +/* PORTn Current [mA], return in [nA] */
>> +/* 1000 * ((PORTn_CURRENT_MSB << 8) + PORTn_CURRENT_LSB) / 16384 */
>> +#define SI3474_NA_STEP (1000 * 1000 * 1000 / 16384)
>> +
>> +/* VPWR Voltage */
>> +#define VPWR_LSB_REG 0x2E
>> +#define VPWR_MSB_REG 0x2F
>> +
>> +/* PORTn Voltage */
>> +#define PORT1_VOLTAGE_LSB_REG 0x32
>> +
>> +/* VPWR Voltage [V], return in [uV] */
>> +/* 60 * (( VPWR_MSB << 8) + VPWR_LSB) / 16384 */
>> +#define SI3474_UV_STEP (1000 * 1000 * 60 / 16384)
>> +
>> +struct si3474_pi_desc {
>> +     u8 chan[2];
> 
> here chan does not sound smooth -> channels[] ?
> especially for an array or list
> 

Same as above. 

>> +     bool is_4p;
>> +};
>> +
>> +struct si3474_priv {
>> +     struct i2c_client *client[2];
>> +     struct pse_controller_dev pcdev;
>> +     struct device_node *np;
>> +     struct si3474_pi_desc pi[SI3474_MAX_CHANS];
>> +};
> 
> Thanks,
> Alok

Thanks
/Piotr

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

[Kory Maincent]

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

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

[Oleksij Rempel]

Hi Piotr,

here are some comments.

On Fri, May 16, 2025 at 01:07:18PM +0000, Piotr Kubik 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.
> 
> Signed-off-by: Piotr Kubik <piotr.kubik@adtran.com>
> ---
>  drivers/net/pse-pd/Kconfig  |  10 +
>  drivers/net/pse-pd/Makefile |   1 +
>  drivers/net/pse-pd/si3474.c | 649 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 660 insertions(+)
>  create mode 100644 drivers/net/pse-pd/si3474.c
> 
> diff --git a/drivers/net/pse-pd/Kconfig b/drivers/net/pse-pd/Kconfig
> index 7fab916a7f46..d1b100eb8c52 100644
> --- a/drivers/net/pse-pd/Kconfig
> +++ b/drivers/net/pse-pd/Kconfig
> @@ -32,6 +32,16 @@ config PSE_PD692X0
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called pd692x0.
>  
> +config PSE_SI3474
> +	tristate "Si3474 PSE controller"
> +	depends on I2C
> +	help
> +	  This module provides support for Si3474 regulator based Ethernet
> +	  Power Sourcing Equipment.

Will be good to add here current limitation, that is supports only
4-pair mode.

> +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;
> +
> +	chan0 = priv->pi[id].chan[0];
> +	chan1 = priv->pi[id].chan[1];
> +
> +	if (chan0 < 4)
> +		client = priv->client[0];
> +	else
> +		client = priv->client[1];
> +
> +	ret = i2c_smbus_read_byte_data(client, PORT_MODE_REG);

There are repeating patterns in client and channel calculation.
Some of them can be uput in a separate function.

> +	if (ret < 0) {
> +		admin_state->c33_admin_state =
> +			ETHTOOL_C33_PSE_ADMIN_STATE_UNKNOWN;
> +		return ret;
> +	}
> +
> +	is_enabled = ((ret & (0x03 << (2 * (chan0 % 4)))) |
> +		      (ret & (0x03 << (2 * (chan1 % 4))))) != 0;

Please replace magic numbers with defines.

> +
> +	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_get_pw_status(struct pse_controller_dev *pcdev, int id,
> +				   struct pse_pw_status *pw_status)
> +{
> +	struct si3474_priv *priv = to_si3474_priv(pcdev);
> +	struct i2c_client *client;
> +	bool delivering = false;
> +	u8 chan0, chan1;
> +	s32 ret;
> +
> +	if (id >= SI3474_MAX_CHANS)
> +		return -ERANGE;
> +
> +	chan0 = priv->pi[id].chan[0];
> +	chan1 = priv->pi[id].chan[1];
> +
> +	if (chan0 < 4)
> +		client = priv->client[0];
> +	else
> +		client = priv->client[1];
> +
> +	ret = i2c_smbus_read_byte_data(client, POWER_STATUS_REG);
> +	if (ret < 0) {
> +		pw_status->c33_pw_status = ETHTOOL_C33_PSE_PW_D_STATUS_UNKNOWN;
> +		return ret;
> +	}
> +
> +	delivering = (ret & (BIT((chan0 % 4) + 4) | BIT((chan1 % 4) + 4))) != 0;
> +
> +	if (delivering)
> +		pw_status->c33_pw_status =
> +			ETHTOOL_C33_PSE_PW_D_STATUS_DELIVERING;
> +	else
> +		pw_status->c33_pw_status = ETHTOOL_C33_PSE_PW_D_STATUS_DISABLED;
> +
> +	return 0;
> +}
> +
> +/* Parse pse-pis subnode into chan array of si3474_priv */
> +static int si3474_get_of_channels(struct si3474_priv *priv)
> +{
> +	struct device_node *pse_node;
> +	struct pse_pi *pi;
> +	u32 pi_no, chan_id;
> +	s8 pairset_cnt;
> +	s32 ret = 0;
> +
> +	pse_node = of_get_child_by_name(priv->np, "pse-pis");
> +	if (!pse_node) {
> +		dev_warn(&priv->client[0]->dev,
> +			 "Unable to parse DT PSE power interface matrix, no pse-pis node\n");
> +		return -EINVAL;
> +	}
> +
> +	for_each_child_of_node_scoped(pse_node, node) {
> +		if (!of_node_name_eq(node, "pse-pi"))
> +			continue;
> +
> +		ret = of_property_read_u32(node, "reg", &pi_no);
> +		if (ret) {
> +			dev_err(&priv->client[0]->dev,
> +				"Failed to read pse-pi reg property\n");
> +			goto out;
> +		}
> +		if (pi_no >= SI3474_MAX_CHANS) {
> +			dev_err(&priv->client[0]->dev,
> +				"Invalid power interface number %u\n", pi_no);
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
> +		pairset_cnt = of_property_count_elems_of_size(node, "pairsets",
> +							      sizeof(u32));
> +		if (!pairset_cnt) {
> +			dev_err(&priv->client[0]->dev,
> +				"Failed to get pairsets property\n");
> +			goto out;
> +		}
> +
> +		pi = &priv->pcdev.pi[pi_no];
> +		if (!pi->pairset[0].np) {
> +			dev_err(&priv->client[0]->dev,
> +				"Missing pairset reference, power interface: %u\n",
> +				pi_no);
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
> +		ret = of_property_read_u32(pi->pairset[0].np, "reg", &chan_id);
> +		if (ret) {
> +			dev_err(&priv->client[0]->dev,
> +				"Failed to read channel reg property, ret:%d\n",
> +				ret);
> +			goto out;
> +		}
> +		priv->pi[pi_no].chan[0] = chan_id;

should we validated chan_id?


> +		priv->pi[pi_no].is_4p = FALSE;

Please use lower case variant (false/true).

> +
> +		if (pairset_cnt == 2) {
> +			if (!pi->pairset[1].np) {
> +				dev_err(&priv->client[0]->dev,
> +					"Missing pairset reference, power interface: %u\n",
> +					pi_no);
> +				ret = -EINVAL;
> +				goto out;
> +			}
> +
> +			ret = of_property_read_u32(pi->pairset[1].np, "reg",
> +						   &chan_id);
> +			if (ret) {
> +				dev_err(&priv->client[0]->dev,
> +					"Failed to read channel reg property\n");
> +				goto out;
> +			}
> +			priv->pi[pi_no].chan[1] = chan_id;

same here

> +			priv->pi[pi_no].is_4p = TRUE;
> +		} else {
> +			dev_err(&priv->client[0]->dev,
> +				"Number of pairsets incorrect - only 4p configurations supported\n");
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +	}
> +
> +out:
> +	of_node_put(pse_node);
> +	return ret;
> +}
> +
...

> +static int si3474_pi_get_chan_current(struct si3474_priv *priv, u8 chan)
> +{
> +	struct i2c_client *client;
> +	s32 ret;
> +	u8 reg;
> +	u64 tmp_64;
> +
> +	if (chan < 4)
> +		client = priv->client[0];
> +	else
> +		client = priv->client[1];
> +
> +	/* Registers 0x30 to 0x3d */
> +	reg = PORT1_CURRENT_LSB_REG + (chan % 4) * 4;

Do this values are valid channels is not enabled and/or not delivering? 

> +	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;
> +
> +	if (chan < 4)
> +		client = priv->client[0];
> +	else
> +		client = priv->client[1];
> +
> +	/* Registers 0x32 to 0x3f */
> +	reg = PORT1_VOLTAGE_LSB_REG + (chan % 4) * 4;
> +
> +	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;
> +
> +	chan0 = priv->pi[id].chan[0];
> +	chan1 = priv->pi[id].chan[1];
> +
> +	if (chan0 < 4)
> +		client = priv->client[0];
> +	else
> +		client = priv->client[1];
> +
> +	/* Check which channels are enabled*/
> +	ret = i2c_smbus_read_byte_data(client, POWER_STATUS_REG);
> +	if (ret < 0)
> +		return ret;

Do voltage values are valide if channel is enabled by not delivering?

> +	/* Take voltage from the first enabled channel */
> +	if (ret & BIT(chan0 % 4))
> +		ret = si3474_pi_get_chan_voltage(priv, chan0);
> +	else if (ret & BIT(chan1))

should it be (chan1 % 4) ?

> +		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;
> +
> +	if (id >= SI3474_MAX_CHANS)
> +		return -ERANGE;
> +
> +	ret = si3474_pi_get_voltage(&priv->pcdev, id);
> +	if (ret < 0)
> +		return ret;
> +	uV = ret;
> +
> +	chan0 = priv->pi[id].chan[0];
> +	chan1 = priv->pi[id].chan[1];
> +
> +	ret = si3474_pi_get_chan_current(priv, chan0);
> +	if (ret < 0)
> +		return ret;
> +	uA = ret;
> +
> +	ret = si3474_pi_get_chan_current(priv, chan1);
> +	if (ret < 0)
> +		return ret;
> +	uA += ret;
> +
> +	tmp_64 = uV;
> +	tmp_64 *= uA;
> +	/* mW = uV * uA / 1000000000 */
> +	return DIV_ROUND_CLOSEST_ULL(tmp_64, 1000000000);
> +}
> +
> +static const struct pse_controller_ops si3474_ops = {
> +	.setup_pi_matrix = si3474_setup_pi_matrix,
> +	.pi_enable = si3474_pi_enable,
> +	.pi_disable = si3474_pi_disable,
> +	.pi_get_actual_pw = si3474_pi_get_actual_pw,
> +	.pi_get_voltage = si3474_pi_get_voltage,
> +	.pi_get_admin_state = si3474_pi_get_admin_state,
> +	.pi_get_pw_status = si3474_pi_get_pw_status,
> +};
> +
> +static void si3474_ancillary_i2c_remove(void *data)
> +{
> +	struct i2c_client *client = data;
> +
> +	i2c_unregister_device(client);
> +}
> +
> +static int si3474_i2c_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct si3474_priv *priv;
> +	s32 ret;
> +	u8 fw_version;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> +		dev_err(dev, "i2c check functionality failed\n");
> +		return -ENXIO;
> +	}
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	ret = i2c_smbus_read_byte_data(client, VENDOR_IC_ID_REG);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ret != SI3474_DEVICE_ID) {
> +		dev_err(dev, "Wrong device ID: 0x%x\n", ret);
> +		return -ENXIO;
> +	}
> +
> +	ret = i2c_smbus_read_byte_data(client, FIRMWARE_REVISION_REG);
> +	if (ret < 0)
> +		return ret;
> +	fw_version = ret;
> +
> +	ret = i2c_smbus_read_byte_data(client, CHIP_REVISION_REG);
> +	if (ret < 0)
> +		return ret;
> +
> +	dev_dbg(dev, "Chip revision: 0x%x, firmware version: 0x%x\n",
> +		ret, fw_version);
> +
> +	priv->client[0] = client;
> +	i2c_set_clientdata(client, priv);
> +
> +	priv->client[1] = i2c_new_ancillary_device(priv->client[0], "secondary",
> +						   priv->client[0]->addr + 1);
> +	if (IS_ERR(priv->client[1]))
> +		return PTR_ERR(priv->client[1]);
> +
> +	ret = devm_add_action_or_reset(dev, si3474_ancillary_i2c_remove, priv->client[1]);
> +	if (ret < 0) {
> +		dev_err(&priv->client[1]->dev, "Cannot register remove callback\n");
> +		return ret;
> +	}
> +
> +	ret = i2c_smbus_read_byte_data(priv->client[1], VENDOR_IC_ID_REG);
> +	if (ret < 0) {
> +		dev_err(&priv->client[1]->dev, "Cannot access secondary PSE controller\n");
> +		return ret;
> +	}
> +
> +	if (ret != SI3474_DEVICE_ID) {
> +		dev_err(&priv->client[1]->dev,
> +			"Wrong device ID for secondary PSE controller: 0x%x\n", ret);
> +		return -ENXIO;
> +	}
> +
> +	priv->np = dev->of_node;
> +	priv->pcdev.owner = THIS_MODULE;
> +	priv->pcdev.ops = &si3474_ops;
> +	priv->pcdev.dev = dev;
> +	priv->pcdev.types = ETHTOOL_PSE_C33;
> +	priv->pcdev.nr_lines = SI3474_MAX_CHANS;

Do we actually have SI3474_MAX_CHANS (8 channels) in 4p mode? I guess it
will be 4.

> +
> +	ret = devm_pse_controller_register(dev, &priv->pcdev);
> +	if (ret) {
> +		dev_err(dev, "Failed to register PSE controller: 0x%x\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id si3474_id[] = {
> +	{ "si3474" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, si3474_id);
> +
> +static const struct of_device_id si3474_of_match[] = {
> +	{
> +		.compatible = "skyworks,si3474",
> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, si3474_of_match);
> +
> +static struct i2c_driver si3474_driver = {
> +	.probe = si3474_i2c_probe,
> +	.id_table = si3474_id,
> +	.driver = {
> +		.name = "si3474",
> +		.of_match_table = si3474_of_match,
> +	},
> +};
> +module_i2c_driver(si3474_driver);
> +
> +MODULE_AUTHOR("Piotr Kubik <piotr.kubik@adtran.com>");
> +MODULE_DESCRIPTION("Skyworks Si3474 PoE PSE Controller driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.43.0
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Piotr Kubik]

Hi Oleksij, 

Thanks for  a review.  

On 5/22/25 11:29, Oleksij Rempel wrote:
> Hi Piotr,
> 
> here are some comments.
> 
> On Fri, May 16, 2025 at 01:07:18PM +0000, Piotr Kubik 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.
>>
>> Signed-off-by: Piotr Kubik <piotr.kubik@adtran.com>
>> ---
>>  drivers/net/pse-pd/Kconfig  |  10 +
>>  drivers/net/pse-pd/Makefile |   1 +
>>  drivers/net/pse-pd/si3474.c | 649 ++++++++++++++++++++++++++++++++++++
>>  3 files changed, 660 insertions(+)
>>  create mode 100644 drivers/net/pse-pd/si3474.c
>>
>> diff --git a/drivers/net/pse-pd/Kconfig b/drivers/net/pse-pd/Kconfig
>> index 7fab916a7f46..d1b100eb8c52 100644
>> --- a/drivers/net/pse-pd/Kconfig
>> +++ b/drivers/net/pse-pd/Kconfig
>> @@ -32,6 +32,16 @@ config PSE_PD692X0
>>  	  To compile this driver as a module, choose M here: the
>>  	  module will be called pd692x0.
>>  
>> +config PSE_SI3474
>> +	tristate "Si3474 PSE controller"
>> +	depends on I2C
>> +	help
>> +	  This module provides support for Si3474 regulator based Ethernet
>> +	  Power Sourcing Equipment.
> 
> Will be good to add here current limitation, that is supports only
> 4-pair mode.
> 

Well, at the moment it is only in commitmsg, so yes, I think it is a good place to
notify potential user.

>> +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;
>> +
>> +	chan0 = priv->pi[id].chan[0];
>> +	chan1 = priv->pi[id].chan[1];
>> +
>> +	if (chan0 < 4)
>> +		client = priv->client[0];
>> +	else
>> +		client = priv->client[1];
>> +
>> +	ret = i2c_smbus_read_byte_data(client, PORT_MODE_REG);
> 
> There are repeating patterns in client and channel calculation.
> Some of them can be uput in a separate function.
> 
>> +	if (ret < 0) {
>> +		admin_state->c33_admin_state =
>> +			ETHTOOL_C33_PSE_ADMIN_STATE_UNKNOWN;
>> +		return ret;
>> +	}
>> +
>> +	is_enabled = ((ret & (0x03 << (2 * (chan0 % 4)))) |
>> +		      (ret & (0x03 << (2 * (chan1 % 4))))) != 0;
> 
> Please replace magic numbers with defines.
> 
>> +
>> +	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_get_pw_status(struct pse_controller_dev *pcdev, int id,
>> +				   struct pse_pw_status *pw_status)
>> +{
>> +	struct si3474_priv *priv = to_si3474_priv(pcdev);
>> +	struct i2c_client *client;
>> +	bool delivering = false;
>> +	u8 chan0, chan1;
>> +	s32 ret;
>> +
>> +	if (id >= SI3474_MAX_CHANS)
>> +		return -ERANGE;
>> +
>> +	chan0 = priv->pi[id].chan[0];
>> +	chan1 = priv->pi[id].chan[1];
>> +
>> +	if (chan0 < 4)
>> +		client = priv->client[0];
>> +	else
>> +		client = priv->client[1];
>> +
>> +	ret = i2c_smbus_read_byte_data(client, POWER_STATUS_REG);
>> +	if (ret < 0) {
>> +		pw_status->c33_pw_status = ETHTOOL_C33_PSE_PW_D_STATUS_UNKNOWN;
>> +		return ret;
>> +	}
>> +
>> +	delivering = (ret & (BIT((chan0 % 4) + 4) | BIT((chan1 % 4) + 4))) != 0;
>> +
>> +	if (delivering)
>> +		pw_status->c33_pw_status =
>> +			ETHTOOL_C33_PSE_PW_D_STATUS_DELIVERING;
>> +	else
>> +		pw_status->c33_pw_status = ETHTOOL_C33_PSE_PW_D_STATUS_DISABLED;
>> +
>> +	return 0;
>> +}
>> +
>> +/* Parse pse-pis subnode into chan array of si3474_priv */
>> +static int si3474_get_of_channels(struct si3474_priv *priv)
>> +{
>> +	struct device_node *pse_node;
>> +	struct pse_pi *pi;
>> +	u32 pi_no, chan_id;
>> +	s8 pairset_cnt;
>> +	s32 ret = 0;
>> +
>> +	pse_node = of_get_child_by_name(priv->np, "pse-pis");
>> +	if (!pse_node) {
>> +		dev_warn(&priv->client[0]->dev,
>> +			 "Unable to parse DT PSE power interface matrix, no pse-pis node\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	for_each_child_of_node_scoped(pse_node, node) {
>> +		if (!of_node_name_eq(node, "pse-pi"))
>> +			continue;
>> +
>> +		ret = of_property_read_u32(node, "reg", &pi_no);
>> +		if (ret) {
>> +			dev_err(&priv->client[0]->dev,
>> +				"Failed to read pse-pi reg property\n");
>> +			goto out;
>> +		}
>> +		if (pi_no >= SI3474_MAX_CHANS) {
>> +			dev_err(&priv->client[0]->dev,
>> +				"Invalid power interface number %u\n", pi_no);
>> +			ret = -EINVAL;
>> +			goto out;
>> +		}
>> +
>> +		pairset_cnt = of_property_count_elems_of_size(node, "pairsets",
>> +							      sizeof(u32));
>> +		if (!pairset_cnt) {
>> +			dev_err(&priv->client[0]->dev,
>> +				"Failed to get pairsets property\n");
>> +			goto out;
>> +		}
>> +
>> +		pi = &priv->pcdev.pi[pi_no];
>> +		if (!pi->pairset[0].np) {
>> +			dev_err(&priv->client[0]->dev,
>> +				"Missing pairset reference, power interface: %u\n",
>> +				pi_no);
>> +			ret = -EINVAL;
>> +			goto out;
>> +		}
>> +
>> +		ret = of_property_read_u32(pi->pairset[0].np, "reg", &chan_id);
>> +		if (ret) {
>> +			dev_err(&priv->client[0]->dev,
>> +				"Failed to read channel reg property, ret:%d\n",
>> +				ret);
>> +			goto out;
>> +		}
>> +		priv->pi[pi_no].chan[0] = chan_id;
> 
> should we validated chan_id?
> 
> 
OK, I will.
>> +		priv->pi[pi_no].is_4p = FALSE;
> 
> Please use lower case variant (false/true).
> 
>> +
>> +		if (pairset_cnt == 2) {
>> +			if (!pi->pairset[1].np) {
>> +				dev_err(&priv->client[0]->dev,
>> +					"Missing pairset reference, power interface: %u\n",
>> +					pi_no);
>> +				ret = -EINVAL;
>> +				goto out;
>> +			}
>> +
>> +			ret = of_property_read_u32(pi->pairset[1].np, "reg",
>> +						   &chan_id);
>> +			if (ret) {
>> +				dev_err(&priv->client[0]->dev,
>> +					"Failed to read channel reg property\n");
>> +				goto out;
>> +			}
>> +			priv->pi[pi_no].chan[1] = chan_id;
> 
> same here
> 
>> +			priv->pi[pi_no].is_4p = TRUE;
>> +		} else {
>> +			dev_err(&priv->client[0]->dev,
>> +				"Number of pairsets incorrect - only 4p configurations supported\n");
>> +			ret = -EINVAL;
>> +			goto out;
>> +		}
>> +	}
>> +
>> +out:
>> +	of_node_put(pse_node);
>> +	return ret;
>> +}
>> +
> ...
> 
>> +static int si3474_pi_get_chan_current(struct si3474_priv *priv, u8 chan)
>> +{
>> +	struct i2c_client *client;
>> +	s32 ret;
>> +	u8 reg;
>> +	u64 tmp_64;
>> +
>> +	if (chan < 4)
>> +		client = priv->client[0];
>> +	else
>> +		client = priv->client[1];
>> +
>> +	/* Registers 0x30 to 0x3d */
>> +	reg = PORT1_CURRENT_LSB_REG + (chan % 4) * 4;
> 
> Do this values are valid channels is not enabled and/or not delivering? 
> 

get_chan_current() is used in get_actual_pw only()
and there multiplied with actual channel voltage, that is zero if channel is disabled. 
I will optimize si3474_pi_get_actual_pw() to not read currents if voltage is zero.


>> +	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;
>> +
>> +	if (chan < 4)
>> +		client = priv->client[0];
>> +	else
>> +		client = priv->client[1];
>> +
>> +	/* Registers 0x32 to 0x3f */
>> +	reg = PORT1_VOLTAGE_LSB_REG + (chan % 4) * 4;
>> +
>> +	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;
>> +
>> +	chan0 = priv->pi[id].chan[0];
>> +	chan1 = priv->pi[id].chan[1];
>> +
>> +	if (chan0 < 4)
>> +		client = priv->client[0];
>> +	else
>> +		client = priv->client[1];
>> +
>> +	/* Check which channels are enabled*/
>> +	ret = i2c_smbus_read_byte_data(client, POWER_STATUS_REG);
>> +	if (ret < 0)
>> +		return ret;
> 
> Do voltage values are valide if channel is enabled by not delivering?
> 

If I understand your question correctly, then - yes.  
POWER_STATUS_REG returns enabled status if power is actually being delivered, 
so voltages must be valid in this case.

>> +	/* Take voltage from the first enabled channel */
>> +	if (ret & BIT(chan0 % 4))
>> +		ret = si3474_pi_get_chan_voltage(priv, chan0);
>> +	else if (ret & BIT(chan1))
> 
> should it be (chan1 % 4) ?
> 

yea, good catch. 

>> +		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;
>> +
>> +	if (id >= SI3474_MAX_CHANS)
>> +		return -ERANGE;
>> +
>> +	ret = si3474_pi_get_voltage(&priv->pcdev, id);
>> +	if (ret < 0)
>> +		return ret;
>> +	uV = ret;
>> +
>> +	chan0 = priv->pi[id].chan[0];
>> +	chan1 = priv->pi[id].chan[1];
>> +
>> +	ret = si3474_pi_get_chan_current(priv, chan0);
>> +	if (ret < 0)
>> +		return ret;
>> +	uA = ret;
>> +
>> +	ret = si3474_pi_get_chan_current(priv, chan1);
>> +	if (ret < 0)
>> +		return ret;
>> +	uA += ret;
>> +
>> +	tmp_64 = uV;
>> +	tmp_64 *= uA;
>> +	/* mW = uV * uA / 1000000000 */
>> +	return DIV_ROUND_CLOSEST_ULL(tmp_64, 1000000000);
>> +}
>> +
>> +static const struct pse_controller_ops si3474_ops = {
>> +	.setup_pi_matrix = si3474_setup_pi_matrix,
>> +	.pi_enable = si3474_pi_enable,
>> +	.pi_disable = si3474_pi_disable,
>> +	.pi_get_actual_pw = si3474_pi_get_actual_pw,
>> +	.pi_get_voltage = si3474_pi_get_voltage,
>> +	.pi_get_admin_state = si3474_pi_get_admin_state,
>> +	.pi_get_pw_status = si3474_pi_get_pw_status,
>> +};
>> +
>> +static void si3474_ancillary_i2c_remove(void *data)
>> +{
>> +	struct i2c_client *client = data;
>> +
>> +	i2c_unregister_device(client);
>> +}
>> +
>> +static int si3474_i2c_probe(struct i2c_client *client)
>> +{
>> +	struct device *dev = &client->dev;
>> +	struct si3474_priv *priv;
>> +	s32 ret;
>> +	u8 fw_version;
>> +
>> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
>> +		dev_err(dev, "i2c check functionality failed\n");
>> +		return -ENXIO;
>> +	}
>> +
>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	ret = i2c_smbus_read_byte_data(client, VENDOR_IC_ID_REG);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (ret != SI3474_DEVICE_ID) {
>> +		dev_err(dev, "Wrong device ID: 0x%x\n", ret);
>> +		return -ENXIO;
>> +	}
>> +
>> +	ret = i2c_smbus_read_byte_data(client, FIRMWARE_REVISION_REG);
>> +	if (ret < 0)
>> +		return ret;
>> +	fw_version = ret;
>> +
>> +	ret = i2c_smbus_read_byte_data(client, CHIP_REVISION_REG);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	dev_dbg(dev, "Chip revision: 0x%x, firmware version: 0x%x\n",
>> +		ret, fw_version);
>> +
>> +	priv->client[0] = client;
>> +	i2c_set_clientdata(client, priv);
>> +
>> +	priv->client[1] = i2c_new_ancillary_device(priv->client[0], "secondary",
>> +						   priv->client[0]->addr + 1);
>> +	if (IS_ERR(priv->client[1]))
>> +		return PTR_ERR(priv->client[1]);
>> +
>> +	ret = devm_add_action_or_reset(dev, si3474_ancillary_i2c_remove, priv->client[1]);
>> +	if (ret < 0) {
>> +		dev_err(&priv->client[1]->dev, "Cannot register remove callback\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = i2c_smbus_read_byte_data(priv->client[1], VENDOR_IC_ID_REG);
>> +	if (ret < 0) {
>> +		dev_err(&priv->client[1]->dev, "Cannot access secondary PSE controller\n");
>> +		return ret;
>> +	}
>> +
>> +	if (ret != SI3474_DEVICE_ID) {
>> +		dev_err(&priv->client[1]->dev,
>> +			"Wrong device ID for secondary PSE controller: 0x%x\n", ret);
>> +		return -ENXIO;
>> +	}
>> +
>> +	priv->np = dev->of_node;
>> +	priv->pcdev.owner = THIS_MODULE;
>> +	priv->pcdev.ops = &si3474_ops;
>> +	priv->pcdev.dev = dev;
>> +	priv->pcdev.types = ETHTOOL_PSE_C33;
>> +	priv->pcdev.nr_lines = SI3474_MAX_CHANS;
> 
> Do we actually have SI3474_MAX_CHANS (8 channels) in 4p mode? I guess it
> will be 4.
> 

yes, in places where SI3474_MAX_CHANS is used we refer to physical channels which is 8 (4 PIs), 
but need to check what pcdev.nr_lines refers to.

>> +
>> +	ret = devm_pse_controller_register(dev, &priv->pcdev);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to register PSE controller: 0x%x\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct i2c_device_id si3474_id[] = {
>> +	{ "si3474" },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(i2c, si3474_id);
>> +
>> +static const struct of_device_id si3474_of_match[] = {
>> +	{
>> +		.compatible = "skyworks,si3474",
>> +	},
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, si3474_of_match);
>> +
>> +static struct i2c_driver si3474_driver = {
>> +	.probe = si3474_i2c_probe,
>> +	.id_table = si3474_id,
>> +	.driver = {
>> +		.name = "si3474",
>> +		.of_match_table = si3474_of_match,
>> +	},
>> +};
>> +module_i2c_driver(si3474_driver);
>> +
>> +MODULE_AUTHOR("Piotr Kubik <piotr.kubik@adtran.com>");
>> +MODULE_DESCRIPTION("Skyworks Si3474 PoE PSE Controller driver");
>> +MODULE_LICENSE("GPL");
>> -- 
>> 2.43.0
>>
> 

Thanks
/Piotr