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

[PATCH net-next 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 admin state,
- get port power,
- get port voltage,
- enable/disable port power

Signed-off-by: Piotr Kubik <piotr.kubik@adtran.com>
---
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
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  | 146 ++++
 drivers/net/pse-pd/Kconfig                    |  10 +
 drivers/net/pse-pd/Makefile                   |   1 +
 drivers/net/pse-pd/si3474.c                   | 654 ++++++++++++++++++
 4 files changed, 811 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 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  | 146 ++++++++++++++++++
 1 file changed, 146 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..646924a3cfb0
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/pse-pd/skyworks,si3474.yaml
@@ -0,0 +1,146 @@
+# 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-names:
+    items:
+      - const: main
+      - const: slave
+
+  reg:
+    maxItems: 2
+
+  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.
+      This parameter describes the relationship between the logical and
+      the physical power channels.
+
+    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", "slave";
+        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 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 | 654 ++++++++++++++++++++++++++++++++++++
 3 files changed, 665 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..67f2070f21bb
--- /dev/null
+++ b/drivers/net/pse-pd/si3474.c
@@ -0,0 +1,654 @@
+// 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, *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(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");
+			ret = -EINVAL;
+			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");
+			ret = -EINVAL;
+			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);
+			ret = -EINVAL;
+			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");
+				ret = -EINVAL;
+				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);
+	of_node_put(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 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_info(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], "slave",
+						   priv->client[0]->addr + 1);
+	if (IS_ERR(priv->client[1]))
+		return PTR_ERR(priv->client[1]);
+
+	ret = i2c_smbus_read_byte_data(priv->client[1], VENDOR_IC_ID_REG);
+	if (ret < 0) {
+		dev_err(&priv->client[1]->dev, "Cannot access slave PSE controller\n");
+		goto out_err_slave;
+	}
+
+	if (ret != SI3474_DEVICE_ID) {
+		dev_err(&priv->client[1]->dev,
+			"Wrong device ID for slave PSE controller: 0x%x\n", ret);
+		ret = -ENXIO;
+		goto out_err_slave;
+	}
+
+	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) {
+		return dev_err_probe(dev, ret,
+				     "Failed to register PSE controller\n");
+	}
+
+	return ret;
+
+out_err_slave:
+	i2c_unregister_device(priv->client[1]);
+	return ret;
+}
+
+static void si3474_i2c_remove(struct i2c_client *client)
+{
+	struct si3474_priv *priv = i2c_get_clientdata(client);
+
+	i2c_unregister_device(priv->client[1]);
+}
+
+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,
+	.remove = si3474_i2c_remove,
+	.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 2/2] net: pse-pd: Add Si3474 PSE controller driver

[Krzysztof Kozlowski]

On 13/05/2025 00:06, Piotr Kubik wrote:
> +/* 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, *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(pse_node, node) {

Use scoped variant. One cleanup less.


> +		if (!of_node_name_eq(node, "pse-pi"))
> +			continue;

...

> +
> +	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_info(dev, "Chip revision: 0x%x, firmware version: 0x%x\n",

dev_dbg or just drop. Drivers should be silent on success.

> +		 ret, fw_version);
> +
> +	priv->client[0] = client;
> +	i2c_set_clientdata(client, priv);
> +
> +	priv->client[1] = i2c_new_ancillary_device(priv->client[0], "slave",
> +						   priv->client[0]->addr + 1);
> +	if (IS_ERR(priv->client[1]))
> +		return PTR_ERR(priv->client[1]);
> +
> +	ret = i2c_smbus_read_byte_data(priv->client[1], VENDOR_IC_ID_REG);
> +	if (ret < 0) {
> +		dev_err(&priv->client[1]->dev, "Cannot access slave PSE controller\n");
> +		goto out_err_slave;
> +	}
> +
> +	if (ret != SI3474_DEVICE_ID) {
> +		dev_err(&priv->client[1]->dev,
> +			"Wrong device ID for slave PSE controller: 0x%x\n", ret);
> +		ret = -ENXIO;
> +		goto out_err_slave;
> +	}
> +
> +	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) {

No need for {}

> +		return dev_err_probe(dev, ret,
> +				     "Failed to register PSE controller\n");

No cleanup here? That's odd.

> +	}
> +
> +	return ret;

return 0

> +
> +out_err_slave:
> +	i2c_unregister_device(priv->client[1]);
> +	return ret;
> +}
> +
> +static void si3474_i2c_remove(struct i2c_client *client)
> +{
> +	struct si3474_priv *priv = i2c_get_clientdata(client);
> +
> +	i2c_unregister_device(priv->client[1]);

So you first unregister i2c device and then unregister pse controller.
Feels like possible issues, difficult to debug.... Use devm reset
wrapper for that.


Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 13/05/2025 00:05, Piotr Kubik wrote:
> +
> +maintainers:
> +  - Piotr Kubik <piotr.kubik@adtran.com>
> +
> +allOf:
> +  - $ref: pse-controller.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - skyworks,si3474
> +
> +  reg-names:
> +    items:
> +      - const: main
> +      - const: slave

s/slave/secondary/ (or whatever is there in recommended names in coding
style)

> +
> +  reg:

First reg, then reg-names. Please follow other bindings/examples.

> +    maxItems: 2
> +
> +  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.
> +      This parameter describes the relationship between the logical and
> +      the physical power channels.

How exactly this maps here logical and physical channels? You just
listed channels one after another...

> +
> +    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
> +
Best regards,
Krzysztof

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

[Kory Maincent]

On Mon, 12 May 2025 22:02:52 +0000
Piotr Kubik <piotr.kubik@adtran.com> wrote:

> 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 admin state,
> - get port power,
> - get port voltage,
> - enable/disable port power

You forgot the series version in the subject like this: [PATCH net-next v2 0/2]

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

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

[Piotr Kubik]

On 5/13/25 10:24, Krzysztof Kozlowski wrote:
> On 13/05/2025 00:05, Piotr Kubik wrote:
>> +
>> +maintainers:
>> +  - Piotr Kubik <piotr.kubik@adtran.com>
>> +
>> +allOf:
>> +  - $ref: pse-controller.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - skyworks,si3474
>> +
>> +  reg-names:
>> +    items:
>> +      - const: main
>> +      - const: slave
> 
> s/slave/secondary/ (or whatever is there in recommended names in coding
> style)
> 

Well I was thinking about it and decided to use 'slave' for at least two reasons:
- si3474 datasheet calls the second part of IC (we configure it here) this way
- description of i2c_new_ancillary_device() calls this device explicitly slave multiple times

>> +
>> +  reg:
> 
> First reg, then reg-names. Please follow other bindings/examples.
> 
>> +    maxItems: 2
>> +
>> +  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.
>> +      This parameter describes the relationship between the logical and
>> +      the physical power channels.
> 
> How exactly this maps here logical and physical channels? You just
> listed channels one after another...

yes, here in this example it is 1 to 1 simple mapping, but in a real world,
depending on hw connections, there is a possibility that 
e.g. "pse_pi0" will use "<&phys0_4>, <&phys0_5>" pairset for lan port 3.

> 
>> +
>> +    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
>> +
> Best regards,
> Krzysztof

Thanks
/Piotr

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

[Piotr Kubik]

Thanks Krzysztof for your review,

> On 13/05/2025 00:06, Piotr Kubik wrote:
>> +/* 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, *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(pse_node, node) {
> 
> Use scoped variant. One cleanup less.

good point

> 
> 
>> +		if (!of_node_name_eq(node, "pse-pi"))
>> +			continue;
> 
> ...
> 
>> +
>> +	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_info(dev, "Chip revision: 0x%x, firmware version: 0x%x\n",
> 
> dev_dbg or just drop. Drivers should be silent on success.

Is there any rule for this I'm not aware of? 
I'd like to know that device is present and what versions it runs just by looking into dmesg.
This approach is similar to other drivers, all current PSE drivers log it this way.

> 
>> +		 ret, fw_version);
>> +
>> +	priv->client[0] = client;
>> +	i2c_set_clientdata(client, priv);
>> +
>> +	priv->client[1] = i2c_new_ancillary_device(priv->client[0], "slave",
>> +						   priv->client[0]->addr + 1);
>> +	if (IS_ERR(priv->client[1]))
>> +		return PTR_ERR(priv->client[1]);
>> +
>> +	ret = i2c_smbus_read_byte_data(priv->client[1], VENDOR_IC_ID_REG);
>> +	if (ret < 0) {
>> +		dev_err(&priv->client[1]->dev, "Cannot access slave PSE controller\n");
>> +		goto out_err_slave;
>> +	}
>> +
>> +	if (ret != SI3474_DEVICE_ID) {
>> +		dev_err(&priv->client[1]->dev,
>> +			"Wrong device ID for slave PSE controller: 0x%x\n", ret);
>> +		ret = -ENXIO;
>> +		goto out_err_slave;
>> +	}
>> +
>> +	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) {
> 
> No need for {}
> 
>> +		return dev_err_probe(dev, ret,
>> +				     "Failed to register PSE controller\n");
> 
> No cleanup here? That's odd.
> 

Indeed, will fix

>> +	}
>> +
>> +	return ret;
> 
> return 0
> 

This is actually not needed. return above will return

>> +
>> +out_err_slave:
>> +	i2c_unregister_device(priv->client[1]);
>> +	return ret;
>> +}
>> +
>> +static void si3474_i2c_remove(struct i2c_client *client)
>> +{
>> +	struct si3474_priv *priv = i2c_get_clientdata(client);
>> +
>> +	i2c_unregister_device(priv->client[1]);
> 
> So you first unregister i2c device and then unregister pse controller.
> Feels like possible issues, difficult to debug.... Use devm reset
> wrapper for that.
> 

ok, right
since there is no devm_ version of i2c_new_ancillary_device() 
I'll register a remove callback

> 
> Best regards,
> Krzysztof

Thanks
/Piotr

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

[Krzysztof Kozlowski]

On 15/05/2025 17:20, Piotr Kubik wrote:
> Thanks Krzysztof for your review,
> 
>> On 13/05/2025 00:06, Piotr Kubik wrote:
>>> +/* 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, *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(pse_node, node) {
>>
>> Use scoped variant. One cleanup less.
> 
> good point
> 
>>
>>
>>> +		if (!of_node_name_eq(node, "pse-pi"))
>>> +			continue;
>>
>> ...
>>
>>> +
>>> +	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_info(dev, "Chip revision: 0x%x, firmware version: 0x%x\n",
>>
>> dev_dbg or just drop. Drivers should be silent on success.
> 
> Is there any rule for this I'm not aware of? 
> I'd like to know that device is present and what versions it runs just by looking into dmesg.

sysfs is the interface for this.

> This approach is similar to other drivers, all current PSE drivers log it this way.

And this approach was dropped for many, many more drivers. One poor
choice should not be reason to do the same.


> 
>>
>>> +		 ret, fw_version);
>>> +
>>> +	priv->client[0] = client;
>>> +	i2c_set_clientdata(client, priv);
>>> +
>>> +	priv->client[1] = i2c_new_ancillary_device(priv->client[0], "slave",
>>> +						   priv->client[0]->addr + 1);
>>> +	if (IS_ERR(priv->client[1]))
>>> +		return PTR_ERR(priv->client[1]);
>>> +
>>> +	ret = i2c_smbus_read_byte_data(priv->client[1], VENDOR_IC_ID_REG);
>>> +	if (ret < 0) {
>>> +		dev_err(&priv->client[1]->dev, "Cannot access slave PSE controller\n");
>>> +		goto out_err_slave;
>>> +	}
>>> +
>>> +	if (ret != SI3474_DEVICE_ID) {
>>> +		dev_err(&priv->client[1]->dev,
>>> +			"Wrong device ID for slave PSE controller: 0x%x\n", ret);
>>> +		ret = -ENXIO;
>>> +		goto out_err_slave;
>>> +	}
>>> +
>>> +	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) {
>>
>> No need for {}
>>
>>> +		return dev_err_probe(dev, ret,
>>> +				     "Failed to register PSE controller\n");
>>
>> No cleanup here? That's odd.
>>
> 
> Indeed, will fix
> 
>>> +	}
>>> +
>>> +	return ret;
>>
>> return 0
>>
> 
> This is actually not needed. return above will return

Huh? I know it will, but this should be explicit 0. Don't make code more
complicated than it should be.



Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 15/05/2025 17:32, Krzysztof Kozlowski wrote:
> On 15/05/2025 17:20, Piotr Kubik wrote:
>> Thanks Krzysztof for your review,
>>
>>> On 13/05/2025 00:06, Piotr Kubik wrote:
>>>> +/* 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, *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(pse_node, node) {
>>>
>>> Use scoped variant. One cleanup less.
>>
>> good point
>>
>>>
>>>
>>>> +		if (!of_node_name_eq(node, "pse-pi"))
>>>> +			continue;
>>>
>>> ...
>>>
>>>> +
>>>> +	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_info(dev, "Chip revision: 0x%x, firmware version: 0x%x\n",
>>>
>>> dev_dbg or just drop. Drivers should be silent on success.
>>
>> Is there any rule for this I'm not aware of? 
>> I'd like to know that device is present and what versions it runs just by looking into dmesg.
> 
> sysfs is the interface for this.
> 
>> This approach is similar to other drivers, all current PSE drivers log it this way.
> 
> And this approach was dropped for many, many more drivers. One poor
> choice should not be reason to do the same.
... and I missed reference:

> In almost all cases the
> debug statements shouldn't be upstreamed, as a working driver is
> supposed to be


dev_info should not be upstreamed even more. Really, really there is no
need to tell the user that every time device was probed. That's obvious
thing. That's I2C, not really pluggable interface, unless you claim this
is on some mikrobus or other connector?

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 15/05/2025 17:20, Piotr Kubik wrote:
> Thanks Krzysztof for your review,
> 
>> On 13/05/2025 00:06, Piotr Kubik wrote:
>>> +/* 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, *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(pse_node, node) {
>>
>> Use scoped variant. One cleanup less.
> 
> good point
> 
>>
>>
>>> +		if (!of_node_name_eq(node, "pse-pi"))
>>> +			continue;
>>
>> ...
>>
>>> +
>>> +	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_info(dev, "Chip revision: 0x%x, firmware version: 0x%x\n",
>>
>> dev_dbg or just drop. Drivers should be silent on success.
> 
> Is there any rule for this I'm not aware of? 
> I'd like to know that device is present and what versions it runs just by looking into dmesg.
> This approach is similar to other drivers, all current PSE drivers log it this way.
> 
And now I noticed that you already sent it, you got review:
https://lore.kernel.org/all/6ee047d4-f3de-4c25-aaae-721221dc3003@kernel.org/

and you ignored it completely sending the same again.

Sending the same over and over and asking us to do the same review over
and over is really waste of our time.

Go back to v1, implement entire review. Then start versioning your patches.

Best regards,
Krzysztof

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

[Piotr Kubik]

On 5/15/25 17:40, Krzysztof Kozlowski wrote:
> On 15/05/2025 17:20, Piotr Kubik wrote:
>> Thanks Krzysztof for your review,
>>
>>> On 13/05/2025 00:06, Piotr Kubik wrote:
>>>> +/* 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, *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(pse_node, node) {
>>>
>>> Use scoped variant. One cleanup less.
>>
>> good point
>>
>>>
>>>
>>>> +          if (!of_node_name_eq(node, "pse-pi"))
>>>> +                  continue;
>>>
>>> ...
>>>
>>>> +
>>>> +  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_info(dev, "Chip revision: 0x%x, firmware version: 0x%x\n",
>>>
>>> dev_dbg or just drop. Drivers should be silent on success.
>>
>> Is there any rule for this I'm not aware of?
>> I'd like to know that device is present and what versions it runs just by looking into dmesg.
>> This approach is similar to other drivers, all current PSE drivers log it this way.
>>
> And now I noticed that you already sent it, you got review:
> https://lore.kernel.org/all/6ee047d4-f3de-4c25-aaae-721221dc3003@kernel.org/
>
> and you ignored it completely sending the same again.
>
> Sending the same over and over and asking us to do the same review over
> and over is really waste of our time.
>
> Go back to v1, implement entire review. Then start versioning your patches.
>
> Best regards,
> Krzysztof


I didn't ignore, I replied to your comment, since there was no answer I assumed you agree.
https://lore.kernel.org/all/38b02e2d-7935-4a23-b351-d23941e781b0@adtran.com/

Thanks for a reference and explanation, I'll change it.

/Piotr

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

[Krzysztof Kozlowski]

On 15/05/2025 17:58, Piotr Kubik wrote:
> On 5/15/25 17:40, Krzysztof Kozlowski wrote:
>> On 15/05/2025 17:20, Piotr Kubik wrote:
>>> Thanks Krzysztof for your review,
>>>
>>>> On 13/05/2025 00:06, Piotr Kubik wrote:
>>>>> +/* 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, *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(pse_node, node) {
>>>>
>>>> Use scoped variant. One cleanup less.
>>>
>>> good point
>>>
>>>>
>>>>
>>>>> +          if (!of_node_name_eq(node, "pse-pi"))
>>>>> +                  continue;
>>>>
>>>> ...
>>>>
>>>>> +
>>>>> +  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_info(dev, "Chip revision: 0x%x, firmware version: 0x%x\n",
>>>>
>>>> dev_dbg or just drop. Drivers should be silent on success.
>>>
>>> Is there any rule for this I'm not aware of?
>>> I'd like to know that device is present and what versions it runs just by looking into dmesg.
>>> This approach is similar to other drivers, all current PSE drivers log it this way.
>>>
>> And now I noticed that you already sent it, you got review:
>> https://lore.kernel.org/all/6ee047d4-f3de-4c25-aaae-721221dc3003@kernel.org/
>>
>> and you ignored it completely sending the same again.
>>
>> Sending the same over and over and asking us to do the same review over
>> and over is really waste of our time.
>>
>> Go back to v1, implement entire review. Then start versioning your patches.
>>
>> Best regards,
>> Krzysztof
> 
> 
> I didn't ignore, I replied to your comment, since there was no answer I assumed you agree.
> https://lore.kernel.org/all/38b02e2d-7935-4a23-b351-d23941e781b0@adtran.com/
> 
> Thanks for a reference and explanation, I'll change it.
Coding style has it pretty explicit as well.

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 15/05/2025 17:20, Piotr Kubik wrote:
> On 5/13/25 10:24, Krzysztof Kozlowski wrote:
>> On 13/05/2025 00:05, Piotr Kubik wrote:
>>> +
>>> +maintainers:
>>> +  - Piotr Kubik <piotr.kubik@adtran.com>
>>> +
>>> +allOf:
>>> +  - $ref: pse-controller.yaml#
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - skyworks,si3474
>>> +
>>> +  reg-names:
>>> +    items:
>>> +      - const: main
>>> +      - const: slave
>>
>> s/slave/secondary/ (or whatever is there in recommended names in coding
>> style)
>>
> 
> Well I was thinking about it and decided to use 'slave' for at least two reasons:
> - si3474 datasheet calls the second part of IC (we configure it here) this way


This could be a reason, but specs are changing over time (see I2C, I3C)
to include different namings. If this annoys certain government sending
their executive directives, then even better.


> - description of i2c_new_ancillary_device() calls this device explicitly slave multiple times

Old driver code should not be an argument. If code changes, which it can
anytime, are you going to change binding? No, because such change in the
binding would not be allowed.

> 
>>> +
>>> +  reg:
>>
>> First reg, then reg-names. Please follow other bindings/examples.
>>
>>> +    maxItems: 2
>>> +
>>> +  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.
>>> +      This parameter describes the relationship between the logical and
>>> +      the physical power channels.
>>
>> How exactly this maps here logical and physical channels? You just
>> listed channels one after another...
> 
> yes, here in this example it is 1 to 1 simple mapping, but in a real world,
> depending on hw connections, there is a possibility that 
> e.g. "pse_pi0" will use "<&phys0_4>, <&phys0_5>" pairset for lan port 3.
> 

Ack, I see that's actually common for pse-pd. It's fine.


Best regards,
Krzysztof

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

[Piotr Kubik]

On 5/16/25 15:37, Krzysztof Kozlowski wrote:
> On 15/05/2025 17:20, Piotr Kubik wrote:
>> On 5/13/25 10:24, Krzysztof Kozlowski wrote:
>>> On 13/05/2025 00:05, Piotr Kubik wrote:
>>>> +
>>>> +maintainers:
>>>> +  - Piotr Kubik <piotr.kubik@adtran.com>
>>>> +
>>>> +allOf:
>>>> +  - $ref: pse-controller.yaml#
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    enum:
>>>> +      - skyworks,si3474
>>>> +
>>>> +  reg-names:
>>>> +    items:
>>>> +      - const: main
>>>> +      - const: slave
>>>
>>> s/slave/secondary/ (or whatever is there in recommended names in coding
>>> style)
>>>
>>
>> Well I was thinking about it and decided to use 'slave' for at least two reasons:
>> - si3474 datasheet calls the second part of IC (we configure it here) this way
> 
> 
> This could be a reason, but specs are changing over time (see I2C, I3C)
> to include different namings. If this annoys certain government sending
> their executive directives, then even better.
> 

OK, I've read some discussions regarding this issue and decided to rename here and in si3474 code.

> 
>> - description of i2c_new_ancillary_device() calls this device explicitly slave multiple times
> 
> Old driver code should not be an argument. If code changes, which it can
> anytime, are you going to change binding? No, because such change in the
> binding would not be allowed.
> 



>>
>>>> +
>>>> +  reg:
>>>
>>> First reg, then reg-names. Please follow other bindings/examples.
>>>
>>>> +    maxItems: 2
>>>> +
>>>> +  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.
>>>> +      This parameter describes the relationship between the logical and
>>>> +      the physical power channels.
>>>
>>> How exactly this maps here logical and physical channels? You just
>>> listed channels one after another...
>>
>> yes, here in this example it is 1 to 1 simple mapping, but in a real world,
>> depending on hw connections, there is a possibility that 
>> e.g. "pse_pi0" will use "<&phys0_4>, <&phys0_5>" pairset for lan port 3.
>>
> 
> Ack, I see that's actually common for pse-pd. It's fine.
> 
Actually, after some consideration I agreed with you and decided to remove this part 
in v3 as this 'channels' node does not really  describe HW mapping, the pse-pis part does.
v3: https://lore.kernel.org/netdev/ebe9a9f5-db9c-4b82-a5e9-3b108a0c6338@adtran.com/

> 
> Best regards,
> Krzysztof

Thanks
/Piotr

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

[Kory Maincent]

On Thu, 15 May 2025 15:20:40 +0000
Piotr Kubik <piotr.kubik@adtran.com> wrote:

> On 5/13/25 10:24, Krzysztof Kozlowski wrote:
> > On 13/05/2025 00:05, Piotr Kubik wrote:  
> >> +
> >> +maintainers:
> >> +  - Piotr Kubik <piotr.kubik@adtran.com>
> >> +
> >> +allOf:
> >> +  - $ref: pse-controller.yaml#
> >> +
> >> +properties:
> >> +  compatible:
> >> +    enum:
> >> +      - skyworks,si3474
> >> +
> >> +  reg-names:
> >> +    items:
> >> +      - const: main
> >> +      - const: slave  
> > 
> > s/slave/secondary/ (or whatever is there in recommended names in coding
> > style)
> >   
> 
> Well I was thinking about it and decided to use 'slave' for at least two
> reasons:
> - si3474 datasheet calls the second part of IC (we configure it here) this way
> - description of i2c_new_ancillary_device() calls this device explicitly
> slave multiple times

It is better to avoid the usage of such word in new code. Secondary suits well
for replacement.

> >> +
> >> +  reg:  
> > 
> > First reg, then reg-names. Please follow other bindings/examples.
> >   
> >> +    maxItems: 2
> >> +
> >> +  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.
> >> +      This parameter describes the relationship between the logical and
> >> +      the physical power channels.  
> > 
> > How exactly this maps here logical and physical channels? You just
> > listed channels one after another...  
> 
> yes, here in this example it is 1 to 1 simple mapping, but in a real world,
> depending on hw connections, there is a possibility that 
> e.g. "pse_pi0" will use "<&phys0_4>, <&phys0_5>" pairset for lan port 3.

But here you should describe the channels of the controller and the channel has
no link to the relationship between logical and physical power channels. This
relationship rather is described in the "pairsets" parameter of PSE PI.

Maybe something like that:

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.
This parameter defines the 8 physical delivery channels on the controller that
can be referenced by PSE PIs through their "pairsets" property. The actual port
matrix mapping is created when PSE PIs reference these channels in their
pairsets. For 4-pair operation, two channels from the same group (0-3 or 4-7)
must be referenced by a single PSE PI.

Similarly the description I used on the tps23881 is also not correct. I have to
change it.
 
I didn't look into the datasheet, could we have parameters specific to a
quad? If that the case we maybe should have something like that:
          quad0: quad@0 {                                                 
            reg = <0>;                                                          
            #address-cells = <1>;                                               
            #size-cells = <0>;                                                                                       
                                                                                
            phys0: port@0 {                                                     
              reg = <0>;                                                        
            };                                                                  
                                                                                
            phys1: port@1 {                                                     
              reg = <1>;                                                        
            };                                                                  
                                                                                
            phys2: port@2 {                                                     
              reg = <2>;                                                        
            };                                                                  
                                                                                
            phys3: port@3 {                                                     
              reg = <3>;                                                        
            };                                                                  
          };                                                                    
                                                                                
          quad@1 {                                                           
            reg = <1>;                                                          
            #address-cells = <1>;                                               
            #size-cells = <0>;                                                  
                                                                                
            phys4: port@0 {                                                     
              reg = <0>;                                                        
            };                                                                  
                                                                                
            phys5: port@1 {                                                     
              reg = <1>;                                                        
            };                                                                  
                                                                                
            phys6: port@2 {                                                     
              reg = <2>;                                                        
            };                                                                  
                                                                                
            phys7: port@3 {                                                     
              reg = <3>;                                                        
            };                                                                  
          };                                                                    
        };

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

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

[Piotr Kubik]

On 5/17/25 00:35, Kory Maincent wrote:
> On Thu, 15 May 2025 15:20:40 +0000
> Piotr Kubik <piotr.kubik@adtran.com> wrote:
> 
>> On 5/13/25 10:24, Krzysztof Kozlowski wrote:
>>> On 13/05/2025 00:05, Piotr Kubik wrote:  
>>>> +
>>>> +maintainers:
>>>> +  - Piotr Kubik <piotr.kubik@adtran.com>
>>>> +
>>>> +allOf:
>>>> +  - $ref: pse-controller.yaml#
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    enum:
>>>> +      - skyworks,si3474
>>>> +
>>>> +  reg-names:
>>>> +    items:
>>>> +      - const: main
>>>> +      - const: slave  
>>>
>>> s/slave/secondary/ (or whatever is there in recommended names in coding
>>> style)
>>>   
>>
>> Well I was thinking about it and decided to use 'slave' for at least two
>> reasons:
>> - si3474 datasheet calls the second part of IC (we configure it here) this way
>> - description of i2c_new_ancillary_device() calls this device explicitly
>> slave multiple times
> 
> It is better to avoid the usage of such word in new code. Secondary suits well
> for replacement.
> 
OK, I replaced it already in v3:
https://lore.kernel.org/netdev/ebe9a9f5-db9c-4b82-a5e9-3b108a0c6338@adtran.com

>>>> +
>>>> +  reg:  
>>>
>>> First reg, then reg-names. Please follow other bindings/examples.
>>>   
>>>> +    maxItems: 2
>>>> +
>>>> +  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.
>>>> +      This parameter describes the relationship between the logical and
>>>> +      the physical power channels.  
>>>
>>> How exactly this maps here logical and physical channels? You just
>>> listed channels one after another...  
>>
>> yes, here in this example it is 1 to 1 simple mapping, but in a real world,
>> depending on hw connections, there is a possibility that 
>> e.g. "pse_pi0" will use "<&phys0_4>, <&phys0_5>" pairset for lan port 3.
> 
> But here you should describe the channels of the controller and the channel has
> no link to the relationship between logical and physical power channels. This
> relationship rather is described in the "pairsets" parameter of PSE PI.
> 

Exactly, I got to the same conclusion, that it is the pse-pis node responsibility 
and removed that part in v3.
Commented it in v2:
https://lore.kernel.org/netdev/0b7afab0-0283-4a52-82bc-0d492f752034@adtran.com/

> Maybe something like that:
> 
> 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.
> This parameter defines the 8 physical delivery channels on the controller that
> can be referenced by PSE PIs through their "pairsets" property. The actual port
> matrix mapping is created when PSE PIs reference these channels in their
> pairsets. For 4-pair operation, two channels from the same group (0-3 or 4-7)
> must be referenced by a single PSE PI.
> 
> Similarly the description I used on the tps23881 is also not correct. I have to
> change it.

I like it. I will update.
>  
> I didn't look into the datasheet, could we have parameters specific to a
> quad? If that the case we maybe should have something like that:
>           quad0: quad@0 {                                                 
>             reg = <0>;                                                          
>             #address-cells = <1>;                                               
>             #size-cells = <0>;                                                                                       
>                                                                                 
>             phys0: port@0 {                                                     
>               reg = <0>;                                                        
>             };                                                                  
>                                                                                 
>             phys1: port@1 {                                                     
>               reg = <1>;                                                        
>             };                                                                  
>                                                                                 
>             phys2: port@2 {                                                     
>               reg = <2>;                                                        
>             };                                                                  
>                                                                                 
>             phys3: port@3 {                                                     
>               reg = <3>;                                                        
>             };                                                                  
>           };                                                                    
>                                                                                 
>           quad@1 {                                                           
>             reg = <1>;                                                          
>             #address-cells = <1>;                                               
>             #size-cells = <0>;                                                  
>                                                                                 
>             phys4: port@0 {                                                     
>               reg = <0>;                                                        
>             };                                                                  
>                                                                                 
>             phys5: port@1 {                                                     
>               reg = <1>;                                                        
>             };                                                                  
>                                                                                 
>             phys6: port@2 {                                                     
>               reg = <2>;                                                        
>             };                                                                  
>                                                                                 
>             phys7: port@3 {                                                     
>               reg = <3>;                                                        
>             };                                                                  
>           };                                                                    
>         };
> 
No, I don't see any quad specific parameters, except i2c address we define the level above. 
So I think this would be over-engineered.

> Regards,

Thanks
/Piotr