[PATCH 0/2] Add Si3474 PSE controller driver

[PATCH 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>
---
Piotr Kubik (2):
  net: pse-pd: Add Si3474 PSE controller driver
  dt-bindings: net: pse-pd: Add bindings for Si3474 PSE controller

 .../bindings/net/pse-pd/skyworks,si3474.yaml  | 154 ++++++
 drivers/net/pse-pd/Kconfig                    |  10 +
 drivers/net/pse-pd/Makefile                   |   1 +
 drivers/net/pse-pd/si3474.c                   | 477 ++++++++++++++++++
 4 files changed, 642 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

General Business

[PATCH 1/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 | 477 ++++++++++++++++++++++++++++++++++++
 3 files changed, 488 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..6d2fef6c2602 100644
--- a/drivers/net/pse-pd/Kconfig
+++ b/drivers/net/pse-pd/Kconfig
@@ -41,4 +41,14 @@ config PSE_TPS23881

          To compile this driver as a module, choose M here: the
          module will be called tps23881.
+
+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.
 endif
diff --git a/drivers/net/pse-pd/Makefile b/drivers/net/pse-pd/Makefile
index 9d2898b36737..b33b4d905cd5 100644
--- a/drivers/net/pse-pd/Makefile
+++ b/drivers/net/pse-pd/Makefile
@@ -6,3 +6,4 @@ obj-$(CONFIG_PSE_CONTROLLER) += pse_core.o
 obj-$(CONFIG_PSE_REGULATOR) += pse_regulator.o
 obj-$(CONFIG_PSE_PD692X0) += pd692x0.o
 obj-$(CONFIG_PSE_TPS23881) += tps23881.o
+obj-$(CONFIG_PSE_SI3474) += si3474.o
\ No newline at end of file
diff --git a/drivers/net/pse-pd/si3474.c b/drivers/net/pse-pd/si3474.c
new file mode 100644
index 000000000000..a2b4b8bff393
--- /dev/null
+++ b/drivers/net/pse-pd/si3474.c
@@ -0,0 +1,477 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Driver for the Skyworks Si3474 PoE PSE Controller
+ *
+ */
+
+#include <linux/bitfield.h>
+#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 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_port_desc {
+       u8 chan[2];
+       bool is_4p;
+};
+
+struct si3474_priv {
+       struct i2c_client *client;
+       struct pse_controller_dev pcdev;
+       struct device_node *np;
+       struct si3474_port_desc port[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 = priv->client;
+       bool enabled = FALSE;
+       u8 chan0, chan1;
+       s32 ret;
+
+       ret = i2c_smbus_read_byte_data(client, POWER_STATUS_REG);
+       if (ret < 0) {
+               admin_state->c33_admin_state =
+                   ETHTOOL_C33_PSE_ADMIN_STATE_UNKNOWN;
+               return ret;
+       }
+
+       chan0 = priv->port[id].chan[0];
+       chan1 = priv->port[id].chan[1];
+
+       if (chan0 < 4 && chan1 < 4)
+               enabled = (ret & (BIT(chan0) | BIT(chan1))) != 0;
+
+       if (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 = priv->client;
+       bool delivering = FALSE;
+       u8 chan0, chan1;
+       s32 ret;
+
+       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;
+       }
+
+       chan0 = priv->port[id].chan[0];
+       chan1 = priv->port[id].chan[1];
+
+       if (chan0 < 4 && chan1 < 4)
+               delivering = (ret & (BIT(chan0 + 4) | BIT(chan1 + 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;
+       us32 port_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->dev,
+                        "Unable to parse DT PSE port-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", &port_no);
+               if (ret) {
+                       dev_err(&priv->client->dev,
+                               "Failed to read pse-pi reg property\n");
+                       ret = -EINVAL;
+                       goto out;
+               }
+               if (port_no >= SI3474_MAX_CHANS) {
+                       dev_err(&priv->client->dev, "Invalid port number %u\n",
+                               port_no);
+                       ret = -EINVAL;
+                       goto out;
+               }
+
+               pairset_cnt = of_property_count_elems_of_size(node, "pairsets",
+                                                             sizeof(u32));
+               if (!pairset_cnt) {
+                       dev_err(&priv->client->dev,
+                               "Failed to get pairsets property\n");
+                       ret = -EINVAL;
+                       goto out;
+               }
+
+               pi = &priv->pcdev.pi[port_no];
+               if (!pi->pairset[0].np) {
+                       dev_err(&priv->client->dev,
+                               "Missing pairset reference, port: %u\n",
+                               port_no);
+                       ret = -EINVAL;
+                       goto out;
+               }
+
+               ret = of_property_read_u32(pi->pairset[0].np, "reg", &chan_id);
+               if (ret) {
+                       dev_err(&priv->client->dev,
+                               "Failed to read channel reg property, ret:%d\n",
+                               ret);
+                       ret = -EINVAL;
+                       goto out;
+               }
+               priv->port[port_no].chan[0] = chan_id;
+               priv->port[port_no].is_4p = FALSE;
+
+               if (pairset_cnt == 2) {
+                       if (!pi->pairset[1].np) {
+                               dev_err(&priv->client->dev,
+                                       "Missing pairset reference, port: %u\n",
+                                       port_no);
+                               ret = -EINVAL;
+                               goto out;
+                       }
+
+                       ret = of_property_read_u32(pi->pairset[1].np, "reg",
+                                                  &chan_id);
+                       if (ret) {
+                               dev_err(&priv->client->dev,
+                                       "Failed to read channel reg property\n");
+                               ret = -EINVAL;
+                               goto out;
+                       }
+                       priv->port[port_no].chan[1] = chan_id;
+                       priv->port[port_no].is_4p = TRUE;
+               } else {
+                       dev_err(&priv->client->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->dev,
+                        "Unable to parse DT PSE port-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 = priv->client;
+       u8 chan0, chan1;
+       u16 val = 0;
+       s32 ret;
+
+       if (id >= SI3474_MAX_CHANS)
+               return -ERANGE;
+
+       chan0 = priv->port[id].chan[0];
+       chan1 = priv->port[id].chan[1];
+
+       if (chan0 >= 4 || chan1 >= 4)
+               return -ERANGE;
+
+       val = (BIT(chan0) | BIT(chan1));
+       ret = i2c_smbus_write_word_data(client, PB_POWER_ENABLE_REG, val);
+
+       if (ret)
+               return ret;
+
+       return 0;
+}
+
+static int si3474_pi_disable(struct pse_controller_dev *pcdev, int id)
+{
+       struct si3474_priv *priv = to_si3474_priv(pcdev);
+       struct i2c_client *client = priv->client;
+       u8 chan0, chan1;
+       u16 val = 0;
+       s32 ret;
+
+       if (id >= SI3474_MAX_CHANS)
+               return -ERANGE;
+
+       chan0 = priv->port[id].chan[0];
+       chan1 = priv->port[id].chan[1];
+
+       if (chan0 >= 4 || chan1 >= 4)
+               return -ERANGE;
+
+       val = (BIT(chan0 + 4) | BIT(chan1 + 4));
+       ret = i2c_smbus_write_word_data(client, PB_POWER_ENABLE_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 = priv->client;
+       s32 ret;
+       u8 reg;
+       u64 tmp_64;
+
+       /* 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 = priv->client;
+       s32 ret;
+       u8 reg;
+       us32 val;
+
+       /* 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 = priv->client;
+
+       u8 chan0, chan1;
+       s32 ret;
+
+       chan0 = priv->port[id].chan[0];
+       chan1 = priv->port[id].chan[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))
+               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;
+       us32 uV, uA;
+       u64 tmp_64;
+       u8 chan0, chan1;
+
+       ret = si3474_pi_get_voltage(&priv->pcdev, id);
+       if (ret < 0)
+               return ret;
+       uV = ret;
+
+       chan0 = priv->port[id].chan[0];
+       chan1 = priv->port[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(&client->dev, "Chip revision: 0x%x, firmware version: 0x%x\n",
+                ret, fw_version);
+
+       priv->client = client;
+       i2c_set_clientdata(client, priv);
+       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;
+}
+
+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


General Business

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

[Krzysztof Kozlowski]

On 16/04/2025 12:47, 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 | 477 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 488 insertions(+)
>  create mode 100644 drivers/net/pse-pd/si3474.c

Please put bindings before their user (see DT submitting patches)

> 
> diff --git a/drivers/net/pse-pd/Kconfig b/drivers/net/pse-pd/Kconfig
> index 7fab916a7f46..6d2fef6c2602 100644
> --- a/drivers/net/pse-pd/Kconfig
> +++ b/drivers/net/pse-pd/Kconfig
> @@ -41,4 +41,14 @@ config PSE_TPS23881
> 
>           To compile this driver as a module, choose M here: the
>           module will be called tps23881.
> +
> +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.
>  endif
> diff --git a/drivers/net/pse-pd/Makefile b/drivers/net/pse-pd/Makefile
> index 9d2898b36737..b33b4d905cd5 100644
> --- a/drivers/net/pse-pd/Makefile
> +++ b/drivers/net/pse-pd/Makefile
> @@ -6,3 +6,4 @@ obj-$(CONFIG_PSE_CONTROLLER) += pse_core.o
>  obj-$(CONFIG_PSE_REGULATOR) += pse_regulator.o
>  obj-$(CONFIG_PSE_PD692X0) += pd692x0.o
>  obj-$(CONFIG_PSE_TPS23881) += tps23881.o
> +obj-$(CONFIG_PSE_SI3474) += si3474.o
> \ No newline at end of file

1. Warnin ghere
2. Don't add your entries to the end but in more-or-less alphabetically
sorted place.

> diff --git a/drivers/net/pse-pd/si3474.c b/drivers/net/pse-pd/si3474.c
> new file mode 100644
> index 000000000000..a2b4b8bff393
> --- /dev/null
> +++ b/drivers/net/pse-pd/si3474.c
> @@ -0,0 +1,477 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Driver for the Skyworks Si3474 PoE PSE Controller
> + *
> + */
> +
> +#include <linux/bitfield.h>
> +#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 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_port_desc {
> +       u8 chan[2];
> +       bool is_4p;
> +};
> +
> +struct si3474_priv {
> +       struct i2c_client *client;
> +       struct pse_controller_dev pcdev;
> +       struct device_node *np;
> +       struct si3474_port_desc port[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,

Your patchset is corrupted

> +                                    struct pse_admin_state *admin_state)
> +{
> +       struct si3474_priv *priv = to_si3474_priv(pcdev);
> +       struct i2c_client *client = priv->client;
> +       bool enabled = FALSE;

I believe it is "false", not FALSE.


...

> +
> +       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(&client->dev, "Chip revision: 0x%x, firmware version: 0x%x\n",
> +                ret, fw_version);

dev_dbg, don't pollute dmesg on success.

> +
> +       priv->client = client;
> +       i2c_set_clientdata(client, priv);
> +       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;
> +}
> +
> +static const struct i2c_device_id si3474_id[] = {{"si3474"}, {}};

Use existing kernel style for such arrays.



Best regards,
Krzysztof

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

[Piotr Kubik]

Thanks Krzysztof for the review, I agree with most of the issues stated, will fix. 


On 4/16/25 12:54, Krzysztof Kozlowski wrote:
> [Nie otrzymujesz często wiadomości e-mail z krzk@kernel.org. Dowiedz się, dlaczego jest to ważne, na stronie https://aka.ms/LearnAboutSenderIdentification ]
> 
> On 16/04/2025 12:47, 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 | 477 ++++++++++++++++++++++++++++++++++++
>>  3 files changed, 488 insertions(+)
>>  create mode 100644 drivers/net/pse-pd/si3474.c
> 
> Please put bindings before their user (see DT submitting patches)
> 
>>
>> diff --git a/drivers/net/pse-pd/Kconfig b/drivers/net/pse-pd/Kconfig
>> index 7fab916a7f46..6d2fef6c2602 100644
>> --- a/drivers/net/pse-pd/Kconfig
>> +++ b/drivers/net/pse-pd/Kconfig
>> @@ -41,4 +41,14 @@ config PSE_TPS23881
>>
>>           To compile this driver as a module, choose M here: the
>>           module will be called tps23881.
>> +
>> +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.
>>  endif
>> diff --git a/drivers/net/pse-pd/Makefile b/drivers/net/pse-pd/Makefile
>> index 9d2898b36737..b33b4d905cd5 100644
>> --- a/drivers/net/pse-pd/Makefile
>> +++ b/drivers/net/pse-pd/Makefile
>> @@ -6,3 +6,4 @@ obj-$(CONFIG_PSE_CONTROLLER) += pse_core.o
>>  obj-$(CONFIG_PSE_REGULATOR) += pse_regulator.o
>>  obj-$(CONFIG_PSE_PD692X0) += pd692x0.o
>>  obj-$(CONFIG_PSE_TPS23881) += tps23881.o
>> +obj-$(CONFIG_PSE_SI3474) += si3474.o
>> \ No newline at end of file
> 
> 1. Warnin ghere
> 2. Don't add your entries to the end but in more-or-less alphabetically
> sorted place.
> 
>> diff --git a/drivers/net/pse-pd/si3474.c b/drivers/net/pse-pd/si3474.c
>> new file mode 100644
>> index 000000000000..a2b4b8bff393
>> --- /dev/null
>> +++ b/drivers/net/pse-pd/si3474.c
>> @@ -0,0 +1,477 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Driver for the Skyworks Si3474 PoE PSE Controller
>> + *
>> + */
>> +
>> +#include <linux/bitfield.h>
>> +#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 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_port_desc {
>> +       u8 chan[2];
>> +       bool is_4p;
>> +};
>> +
>> +struct si3474_priv {
>> +       struct i2c_client *client;
>> +       struct pse_controller_dev pcdev;
>> +       struct device_node *np;
>> +       struct si3474_port_desc port[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,
> 
> Your patchset is corrupted

yeah, my mail client silently folded long lines. 

> 
>> +                                    struct pse_admin_state *admin_state)
>> +{
>> +       struct si3474_priv *priv = to_si3474_priv(pcdev);
>> +       struct i2c_client *client = priv->client;
>> +       bool enabled = FALSE;
> 
> I believe it is "false", not FALSE.
> 
> 
> ...
> 
>> +
>> +       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(&client->dev, "Chip revision: 0x%x, firmware version: 0x%x\n",
>> +                ret, fw_version);
> 
> dev_dbg, don't pollute dmesg on success.

I disagree here, as 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.

> 
>> +
>> +       priv->client = client;
>> +       i2c_set_clientdata(client, priv);
>> +       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;
>> +}
>> +
>> +static const struct i2c_device_id si3474_id[] = {{"si3474"}, {}};
> 
> Use existing kernel style for such arrays.
> 
> 
> 
> Best regards,
> Krzysztof

Regards,
Piotr

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

[Oleksij Rempel]

Hi Piotr,

Thanks again for the patch! Looking a bit deeper into the Si3474
architecture based on the datasheet and thinking about future
extensions, there's a challenge with how the chip mixes shared resources
with its two-quad/two-address I2C setup that we should probably tackle
architecturally.

The current approach, common for multi-address devices, is to treat each
I2C address (and thus each quad) as a separate i2c_client handled by
potentially independent driver instances. This works for the basic port
access implemented here, but might get complicated quickly for the
Si3474.

The Si3474 has several key resources that are inherently shared across
the whole chip package, not just per-quad:

- Single RESETb pin
- Single INTb pin
- Firmware Update
- Global Status (Temperature, VDD/VPWR UVLO)
- OSS Pin
- ... and likely others.

Trying to manage these shared aspects across two potentially independent
driver instances would a bit challenging :)

Proposed Architectural Change:

It seems much more robust to treat the entire Si3474 package as one
logical device within the driver. A possible approach could be:

1. The driver instance probed for the primary address (Quad 0)
takes ownership.
2. It finds/acquires the i2c_client for the secondary address (Quad
1).
3. The primary instance handles all shared resources (IRQ, global
state, etc.).
4. PSE controller registration (devm_pse_controller_register) happens
only once for all 8 logical PIs.
5. Internal functions use the "correct" i2c_client based on the target
channel/PI.

Search for i2c_new_ancillary_device()

Naming Conventions:

- Regarding naming, the goal is to align with IEEE 802.3 terminology where
  possible. Exzeption are register and bit names.

Regarding naming: Could you please rename `priv->port` (and similar variables
representing the logical PSE port/`id`) to `priv->pi`? This aligns better with
the IEEE 802.3 term 'PI' (Power Interface) for the logical port, avoiding the
datasheet's overloaded use of 'port'. We can stick with 'channel' internally
for the physical Si3474 control paths (0-7) ('ports'). Adding the introductory
comment explaining this would still be great too.

Regarding the current patch:
- The `PB_POWER_ENABLE_REG` seems to be 8-bit register, but the driver
  is using i2c_smbus_write_word_data(). Please use i2c_smbus_write_byte_data()
  or add a comment explaining why 'word' version is used.

A comment like this on the top of this driver would be helpful:

/*
 * 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)
 */

Best Regards,
Oleksij
-- 
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 1/2] net: pse-pd: Add Si3474 PSE controller driver

[Piotr Kubik]

On 4/17/25 11:32, Oleksij Rempel wrote:
> Hi Piotr,
> 
> Thanks again for the patch! Looking a bit deeper into the Si3474
> architecture based on the datasheet and thinking about future
> extensions, there's a challenge with how the chip mixes shared resources
> with its two-quad/two-address I2C setup that we should probably tackle
> architecturally.
> 
> The current approach, common for multi-address devices, is to treat each
> I2C address (and thus each quad) as a separate i2c_client handled by
> potentially independent driver instances. This works for the basic port
> access implemented here, but might get complicated quickly for the
> Si3474.
> 
> The Si3474 has several key resources that are inherently shared across
> the whole chip package, not just per-quad:
> 
> - Single RESETb pin
> - Single INTb pin
> - Firmware Update
> - Global Status (Temperature, VDD/VPWR UVLO)
> - OSS Pin
> - ... and likely others.
> 
> Trying to manage these shared aspects across two potentially independent
> driver instances would a bit challenging :)
> 
> Proposed Architectural Change:
> 
> It seems much more robust to treat the entire Si3474 package as one
> logical device within the driver. A possible approach could be:
> 
> 1. The driver instance probed for the primary address (Quad 0)
> takes ownership.
> 2. It finds/acquires the i2c_client for the secondary address (Quad
> 1).
> 3. The primary instance handles all shared resources (IRQ, global
> state, etc.).
> 4. PSE controller registration (devm_pse_controller_register) happens
> only once for all 8 logical PIs.
> 5. Internal functions use the "correct" i2c_client based on the target
> channel/PI.
> 
> Search for i2c_new_ancillary_device()
> 
> Naming Conventions:
> 
> - Regarding naming, the goal is to align with IEEE 802.3 terminology where
>   possible. Exzeption are register and bit names.
> 
> Regarding naming: Could you please rename `priv->port` (and similar variables
> representing the logical PSE port/`id`) to `priv->pi`? This aligns better with
> the IEEE 802.3 term 'PI' (Power Interface) for the logical port, avoiding the
> datasheet's overloaded use of 'port'. We can stick with 'channel' internally
> for the physical Si3474 control paths (0-7) ('ports'). Adding the introductory
> comment explaining this would still be great too.
> 
> Regarding the current patch:
> - The `PB_POWER_ENABLE_REG` seems to be 8-bit register, but the driver
>   is using i2c_smbus_write_word_data(). Please use i2c_smbus_write_byte_data()
>   or add a comment explaining why 'word' version is used.
> 
> A comment like this on the top of this driver would be helpful:
> 
> /*
>  * 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)
>  */
> 
> Best Regards,
> Oleksij

Hi Oleksij, 

Huge thanks for the really deep analysis and reply! You tackled the right point.
Writing this driver I had in mind that using separate i2c instances for one IC has some cons, 
but this way was the quickest approach to have it working, especially when no shared resources 
were used in my implementation.

I agree with your architectural idea and will try to update the driver accordingly. 

Regards,
Piotr

[PATCH 2/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  | 154 ++++++++++++++++++
 1 file changed, 154 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..fd48eeb2f79b
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/pse-pd/skyworks,si3474.yaml
@@ -0,0 +1,154 @@
+# 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:
+  - Kory Maincent <kory.maincent@bootlin.com>
+
+allOf:
+  - $ref: pse-controller.yaml#
+
+properties:
+  compatible:
+    enum:
+      - skyworks,si347
+
+  reg:
+    maxItems: 1
+
+  '#pse-cells':
+    const: 1
+
+  channels:
+    description: Each Si3474 is divided into two quad PoE controllers
+      accessible on different i2c addresses. Each set of quad ports can be
+      assigned to two physical channels (currently 4p support only).
+      This parameter describes the configuration of the ports conversion
+      matrix that establishes relationship between the logical ports and
+      the physical channels.
+    type: object
+    additionalProperties: false
+
+    properties:
+      "#address-cells":
+        const: 1
+
+      "#size-cells":
+        const: 0
+
+    patternProperties:
+      '^channel@[0-3]$':
+        type: object
+        additionalProperties: false
+
+        properties:
+          reg:
+            maxItems: 1
+
+        required:
+          - reg
+
+    required:
+      - "#address-cells"
+      - "#size-cells"
+
+unevaluatedProperties: false
+
+required:
+  - compatible
+  - reg
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      ethernet-pse@26 {
+        compatible = "skyworks,si3474";
+        reg = <0x26>;
+
+        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>;
+          };
+        };
+        pse-pis {
+          #address-cells = <1>;
+          #size-cells = <0>;
+          pse_pi2: pse-pi@2 {
+            reg = <2>;
+            #pse-cells = <0>;
+            pairset-names = "alternative-a", "alternative-b";
+            pairsets = <&phys0_0>, <&phys0_1>;
+            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_2>, <&phys0_3>;
+            polarity-supported = "MDI-X", "S";
+            vpwr-supply = <&reg_pse>;
+          };
+        };
+      };
+
+      ethernet-pse@27 {
+        compatible = "skyworks,si3474";
+        reg = <0x27>;
+
+        channels {
+          #address-cells = <1>;
+          #size-cells = <0>;
+          phys0_4: channel@0 {
+            reg = <0>;
+          };
+          phys0_5: channel@1 {
+            reg = <1>;
+          };
+          phys0_6: channel@2 {
+            reg = <2>;
+          };
+          phys0_7: channel@3 {
+            reg = <3>;
+          };
+        };
+        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_4>, <&phys0_5>;
+            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_6>, <&phys0_7>;
+            polarity-supported = "MDI-X", "S";
+            vpwr-supply = <&reg_pse>;
+          };
+        };
+      };
+    };
--
2.43.0


General Business

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

[Krzysztof Kozlowski]

On 16/04/2025 12:47, 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>
> ---
>  .../bindings/net/pse-pd/skyworks,si3474.yaml  | 154 ++++++++++++++++++
>  1 file changed, 154 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/net/pse-pd/skyworks,si3474.yaml

Also looks like corrupted patch.

> 
> 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..fd48eeb2f79b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/pse-pd/skyworks,si3474.yaml
> @@ -0,0 +1,154 @@
> +# 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:
> +  - Kory Maincent <kory.maincent@bootlin.com>

This should be someone interested in this hardware, not subsystem
maintainer.

> +
> +allOf:
> +  - $ref: pse-controller.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - skyworks,si347
> +
> +  reg:
> +    maxItems: 1
> +
> +  '#pse-cells':
> +    const: 1
> +
> +  channels:
> +    description: Each Si3474 is divided into two quad PoE controllers
> +      accessible on different i2c addresses. Each set of quad ports can be
> +      assigned to two physical channels (currently 4p support only).

What this "currently" means? Limitation of hardware or Linux? If the
latter, then drop.

> +      This parameter describes the configuration of the ports conversion
> +      matrix that establishes relationship between the logical ports and
> +      the physical channels.
> +    type: object
> +    additionalProperties: false
> +
> +    properties:
> +      "#address-cells":
> +        const: 1
> +
> +      "#size-cells":
> +        const: 0
> +
> +    patternProperties:
> +      '^channel@[0-3]$':
> +        type: object
> +        additionalProperties: false
> +
> +        properties:
> +          reg:
> +            maxItems: 1
> +
> +        required:
> +          - reg
> +
> +    required:
> +      - "#address-cells"
> +      - "#size-cells"
> +
> +unevaluatedProperties: false

This goes after required: block.

> +
> +required:
> +  - compatible
> +  - reg
> +
> +examples:
> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      ethernet-pse@26 {
> +        compatible = "skyworks,si3474";
> +        reg = <0x26>;
> +
> +        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>;
> +          };
> +        };
> +        pse-pis {
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +          pse_pi2: pse-pi@2 {
> +            reg = <2>;
> +            #pse-cells = <0>;
> +            pairset-names = "alternative-a", "alternative-b";
> +            pairsets = <&phys0_0>, <&phys0_1>;
> +            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_2>, <&phys0_3>;
> +            polarity-supported = "MDI-X", "S";
> +            vpwr-supply = <&reg_pse>;
> +          };
> +        };
> +      };
> +
> +      ethernet-pse@27 {
> +        compatible = "skyworks,si3474";


This is the same as other example, so drop and keep only one.


Best regards,
Krzysztof

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

[Piotr Kubik]

On 4/16/25 12:58, Krzysztof Kozlowski wrote:
> [Nie otrzymujesz często wiadomości e-mail z krzk@kernel.org. Dowiedz się, dlaczego jest to ważne, na stronie https://aka.ms/LearnAboutSenderIdentification ]
>
> On 16/04/2025 12:47, 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>
>> ---
>>  .../bindings/net/pse-pd/skyworks,si3474.yaml  | 154 ++++++++++++++++++
>>  1 file changed, 154 insertions(+)
>>  create mode 100644
>> Documentation/devicetree/bindings/net/pse-pd/skyworks,si3474.yaml
>
> Also looks like corrupted patch.
>
>>
>> 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..fd48eeb2f79b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/pse-pd/skyworks,si3474.yaml
>> @@ -0,0 +1,154 @@
>> +# 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:
>> +  - Kory Maincent <kory.maincent@bootlin.com>
>
> This should be someone interested in this hardware, not subsystem
> maintainer.
>
>> +
>> +allOf:
>> +  - $ref: pse-controller.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - skyworks,si347
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  '#pse-cells':
>> +    const: 1
>> +
>> +  channels:
>> +    description: Each Si3474 is divided into two quad PoE controllers
>> +      accessible on different i2c addresses. Each set of quad ports can be
>> +      assigned to two physical channels (currently 4p support only).
>
> What this "currently" means? Limitation of hardware or Linux? If the
> latter, then drop.
>
>> +      This parameter describes the configuration of the ports conversion
>> +      matrix that establishes relationship between the logical ports and
>> +      the physical channels.
>> +    type: object
>> +    additionalProperties: false
>> +
>> +    properties:
>> +      "#address-cells":
>> +        const: 1
>> +
>> +      "#size-cells":
>> +        const: 0
>> +
>> +    patternProperties:
>> +      '^channel@[0-3]$':
>> +        type: object
>> +        additionalProperties: false
>> +
>> +        properties:
>> +          reg:
>> +            maxItems: 1
>> +
>> +        required:
>> +          - reg
>> +
>> +    required:
>> +      - "#address-cells"
>> +      - "#size-cells"
>> +
>> +unevaluatedProperties: false
>
> This goes after required: block.
>
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +examples:
>> +  - |
>> +    i2c {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +
>> +      ethernet-pse@26 {
>> +        compatible = "skyworks,si3474";
>> +        reg = <0x26>;
>> +
>> +        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>;
>> +          };
>> +        };
>> +        pse-pis {
>> +          #address-cells = <1>;
>> +          #size-cells = <0>;
>> +          pse_pi2: pse-pi@2 {
>> +            reg = <2>;
>> +            #pse-cells = <0>;
>> +            pairset-names = "alternative-a", "alternative-b";
>> +            pairsets = <&phys0_0>, <&phys0_1>;
>> +            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_2>, <&phys0_3>;
>> +            polarity-supported = "MDI-X", "S";
>> +            vpwr-supply = <&reg_pse>;
>> +          };
>> +        };
>> +      };
>> +
>> +      ethernet-pse@27 {
>> +        compatible = "skyworks,si3474";
>
>
> This is the same as other example, so drop and keep only one.

Right, but Si3474 is specific, like it has two i2c addresses, one for each quad port.
That's why I kept both here to show how the full config for the IC looks like.
I agree it's almost the same and one will easily figure out how to configure the second one.
Anyway, if I update the driver according to Oleksij's comment regarding one driver instance
for both quads, this issue will be gone.

Regards,
Piotr

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

[Kory Maincent]

On Wed, 16 Apr 2025 10:47:12 +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 power,
> - get port voltage,
> - enable/disable port power

Nice to see a new PSE driver!

In net subsystem we add the "net-next" prefix to the subject when it is not a
fix. Please see
https://elixir.bootlin.com/linux/v6.14-rc6/source/Documentation/process/maintainer-netdev.rst#L61

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

Re: [EXTERNAL]Re: [PATCH 0/2] Add Si3474 PSE controller driver

[Piotr Kubik]

On 4/16/25 14:32, Kory Maincent wrote:
> On Wed, 16 Apr 2025 10:47:12 +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 power,
>> - get port voltage,
>> - enable/disable port power
> 
> Nice to see a new PSE driver!
> 
> In net subsystem we add the "net-next" prefix to the subject when it is not a
> fix. Please see
> https://elixir.bootlin.com/linux/v6.14-rc6/source/Documentation/process/maintainer-netdev.rst#L61 
> Regards,

Hi Kory, 
Thanks to your previous good work in this area making writing these drivers possible. 
I'll update the prefix in the next patch.

Regards
Piotr