[PATCH net-next 0/2] net: pse-pd: pd692x0: Add permanent configuration management support

[PATCH net-next 0/2] net: pse-pd: pd692x0: Add permanent configuration management support

[Kory Maincent]

From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>

This patch series adds support for saving and resetting the PD692x0
device's permanent configuration through new sysfs attributes:
save_conf and reset_conf.

The permanent configuration allows settings to persist across device
resets and power cycles, providing better control over PSE behavior
in production environments.

Signed-off-by: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
---
Kory Maincent (2):
      net: pse-pd: pd692x0: Separate configuration parsing from hardware setup
      net: pse-pd: pd692x0: Add sysfs interface for configuration save/reset

 drivers/net/pse-pd/pd692x0.c | 262 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 221 insertions(+), 41 deletions(-)
---
base-commit: 02ccf77ea7d93268e52b4ea31f5538874bca5ac1
change-id: 20250813-feature_poe_permanent_conf-ec640dace1f2

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

[PATCH net-next 1/2] net: pse-pd: pd692x0: Separate configuration parsing from hardware setup

[Kory Maincent]

From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>

Cache the port matrix configuration in driver private data to enable
PSE controller reconfiguration. This refactoring separates device tree
parsing from hardware configuration application, allowing settings to be
reapplied without reparsing the device tree.

This prepares for upcoming permanent configuration support where cached
settings can be restored after device resets or firmware updates.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 drivers/net/pse-pd/pd692x0.c | 113 ++++++++++++++++++++++++++++---------------
 1 file changed, 74 insertions(+), 39 deletions(-)

diff --git a/drivers/net/pse-pd/pd692x0.c b/drivers/net/pse-pd/pd692x0.c
index f4e91ba64a66..8b94967bb4fb 100644
--- a/drivers/net/pse-pd/pd692x0.c
+++ b/drivers/net/pse-pd/pd692x0.c
@@ -85,6 +85,11 @@ enum {
 	PD692X0_MSG_CNT
 };
 
+struct pd692x0_matrix {
+	u8 hw_port_a;
+	u8 hw_port_b;
+};
+
 struct pd692x0_priv {
 	struct i2c_client *client;
 	struct pse_controller_dev pcdev;
@@ -101,6 +106,8 @@ struct pd692x0_priv {
 	enum ethtool_c33_pse_admin_state admin_state[PD692X0_MAX_PIS];
 	struct regulator_dev *manager_reg[PD692X0_MAX_MANAGERS];
 	int manager_pw_budget[PD692X0_MAX_MANAGERS];
+	int nmanagers;
+	struct pd692x0_matrix *port_matrix;
 };
 
 /* Template list of communication messages. The non-null bytes defined here
@@ -809,11 +816,6 @@ struct pd692x0_manager {
 	int nports;
 };
 
-struct pd692x0_matrix {
-	u8 hw_port_a;
-	u8 hw_port_b;
-};
-
 static int
 pd692x0_of_get_ports_manager(struct pd692x0_priv *priv,
 			     struct pd692x0_manager *manager,
@@ -903,7 +905,8 @@ pd692x0_of_get_managers(struct pd692x0_priv *priv,
 	}
 
 	of_node_put(managers_node);
-	return nmanagers;
+	priv->nmanagers = nmanagers;
+	return 0;
 
 out:
 	for (i = 0; i < nmanagers; i++) {
@@ -963,8 +966,7 @@ pd692x0_register_manager_regulator(struct device *dev, char *reg_name,
 
 static int
 pd692x0_register_managers_regulator(struct pd692x0_priv *priv,
-				    const struct pd692x0_manager *manager,
-				    int nmanagers)
+				    const struct pd692x0_manager *manager)
 {
 	struct device *dev = &priv->client->dev;
 	size_t reg_name_len;
@@ -975,7 +977,7 @@ pd692x0_register_managers_regulator(struct pd692x0_priv *priv,
 	 */
 	reg_name_len = strlen(dev_name(dev)) + 23;
 
-	for (i = 0; i < nmanagers; i++) {
+	for (i = 0; i < priv->nmanagers; i++) {
 		static const char * const regulators[] = { "vaux5", "vaux3p3" };
 		struct regulator_dev *rdev;
 		char *reg_name;
@@ -1008,10 +1010,14 @@ pd692x0_register_managers_regulator(struct pd692x0_priv *priv,
 }
 
 static int
-pd692x0_conf_manager_power_budget(struct pd692x0_priv *priv, int id, int pw)
+pd692x0_conf_manager_power_budget(struct pd692x0_priv *priv, int id)
 {
 	struct pd692x0_msg msg, buf;
-	int ret, pw_mW = pw / 1000;
+	int ret, pw_mW;
+
+	pw_mW = priv->manager_pw_budget[id] / 1000;
+	if (!pw_mW)
+		return 0;
 
 	msg = pd692x0_msg_template_list[PD692X0_MSG_GET_POWER_BANK];
 	msg.data[0] = id;
@@ -1032,11 +1038,11 @@ pd692x0_conf_manager_power_budget(struct pd692x0_priv *priv, int id, int pw)
 }
 
 static int
-pd692x0_configure_managers(struct pd692x0_priv *priv, int nmanagers)
+pd692x0_req_managers_pw_budget(struct pd692x0_priv *priv)
 {
 	int i, ret;
 
-	for (i = 0; i < nmanagers; i++) {
+	for (i = 0; i < priv->nmanagers; i++) {
 		struct regulator *supply = priv->manager_reg[i]->supply;
 		int pw_budget;
 
@@ -1053,7 +1059,18 @@ pd692x0_configure_managers(struct pd692x0_priv *priv, int nmanagers)
 			return ret;
 
 		priv->manager_pw_budget[i] = pw_budget;
-		ret = pd692x0_conf_manager_power_budget(priv, i, pw_budget);
+	}
+
+	return 0;
+}
+
+static int
+pd692x0_configure_managers(struct pd692x0_priv *priv)
+{
+	int i, ret;
+
+	for (i = 0; i < priv->nmanagers; i++) {
+		ret = pd692x0_conf_manager_power_budget(priv, i);
 		if (ret < 0)
 			return ret;
 	}
@@ -1101,10 +1118,9 @@ pd692x0_set_port_matrix(const struct pse_pi_pairset *pairset,
 
 static int
 pd692x0_set_ports_matrix(struct pd692x0_priv *priv,
-			 const struct pd692x0_manager *manager,
-			 int nmanagers,
-			 struct pd692x0_matrix port_matrix[PD692X0_MAX_PIS])
+			 const struct pd692x0_manager *manager)
 {
+	struct pd692x0_matrix *port_matrix = priv->port_matrix;
 	struct pse_controller_dev *pcdev = &priv->pcdev;
 	int i, ret;
 
@@ -1117,7 +1133,7 @@ pd692x0_set_ports_matrix(struct pd692x0_priv *priv,
 	/* Update with values for every PSE PIs */
 	for (i = 0; i < pcdev->nr_lines; i++) {
 		ret = pd692x0_set_port_matrix(&pcdev->pi[i].pairset[0],
-					      manager, nmanagers,
+					      manager, priv->nmanagers,
 					      &port_matrix[i]);
 		if (ret) {
 			dev_err(&priv->client->dev,
@@ -1126,7 +1142,7 @@ pd692x0_set_ports_matrix(struct pd692x0_priv *priv,
 		}
 
 		ret = pd692x0_set_port_matrix(&pcdev->pi[i].pairset[1],
-					      manager, nmanagers,
+					      manager, priv->nmanagers,
 					      &port_matrix[i]);
 		if (ret) {
 			dev_err(&priv->client->dev,
@@ -1139,9 +1155,9 @@ pd692x0_set_ports_matrix(struct pd692x0_priv *priv,
 }
 
 static int
-pd692x0_write_ports_matrix(struct pd692x0_priv *priv,
-			   const struct pd692x0_matrix port_matrix[PD692X0_MAX_PIS])
+pd692x0_write_ports_matrix(struct pd692x0_priv *priv)
 {
+	struct pd692x0_matrix *port_matrix = priv->port_matrix;
 	struct pd692x0_msg msg, buf;
 	int ret, i;
 
@@ -1166,13 +1182,32 @@ pd692x0_write_ports_matrix(struct pd692x0_priv *priv,
 	return 0;
 }
 
+static int pd692x0_hw_conf_init(struct pd692x0_priv *priv)
+{
+	int ret;
+
+	/* Is PD692x0 ready to be configured? */
+	if (priv->fw_state != PD692X0_FW_OK &&
+	    priv->fw_state != PD692X0_FW_COMPLETE)
+		return 0;
+
+	ret = pd692x0_configure_managers(priv);
+	if (ret)
+		return ret;
+
+	ret = pd692x0_write_ports_matrix(priv);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static void pd692x0_of_put_managers(struct pd692x0_priv *priv,
-				    struct pd692x0_manager *manager,
-				    int nmanagers)
+				    struct pd692x0_manager *manager)
 {
 	int i, j;
 
-	for (i = 0; i < nmanagers; i++) {
+	for (i = 0; i < priv->nmanagers; i++) {
 		for (j = 0; j < manager[i].nports; j++)
 			of_node_put(manager[i].port_node[j]);
 		of_node_put(manager[i].node);
@@ -1202,46 +1237,46 @@ static int pd692x0_setup_pi_matrix(struct pse_controller_dev *pcdev)
 {
 	struct pd692x0_manager *manager __free(kfree) = NULL;
 	struct pd692x0_priv *priv = to_pd692x0_priv(pcdev);
-	struct pd692x0_matrix port_matrix[PD692X0_MAX_PIS];
-	int ret, nmanagers;
-
-	/* Should we flash the port matrix */
-	if (priv->fw_state != PD692X0_FW_OK &&
-	    priv->fw_state != PD692X0_FW_COMPLETE)
-		return 0;
+	struct pd692x0_matrix *port_matrix;
+	int ret;
 
 	manager = kcalloc(PD692X0_MAX_MANAGERS, sizeof(*manager), GFP_KERNEL);
 	if (!manager)
 		return -ENOMEM;
 
+	port_matrix = devm_kcalloc(&priv->client->dev, PD692X0_MAX_PIS,
+				   sizeof(*port_matrix), GFP_KERNEL);
+	if (!port_matrix)
+		return -ENOMEM;
+	priv->port_matrix = port_matrix;
+
 	ret = pd692x0_of_get_managers(priv, manager);
 	if (ret < 0)
 		return ret;
 
-	nmanagers = ret;
-	ret = pd692x0_register_managers_regulator(priv, manager, nmanagers);
+	ret = pd692x0_register_managers_regulator(priv, manager);
 	if (ret)
 		goto err_of_managers;
 
-	ret = pd692x0_configure_managers(priv, nmanagers);
+	ret = pd692x0_req_managers_pw_budget(priv);
 	if (ret)
 		goto err_of_managers;
 
-	ret = pd692x0_set_ports_matrix(priv, manager, nmanagers, port_matrix);
+	ret = pd692x0_set_ports_matrix(priv, manager);
 	if (ret)
 		goto err_managers_req_pw;
 
-	ret = pd692x0_write_ports_matrix(priv, port_matrix);
+	ret = pd692x0_hw_conf_init(priv);
 	if (ret)
 		goto err_managers_req_pw;
 
-	pd692x0_of_put_managers(priv, manager, nmanagers);
+	pd692x0_of_put_managers(priv, manager);
 	return 0;
 
 err_managers_req_pw:
 	pd692x0_managers_free_pw_budget(priv);
 err_of_managers:
-	pd692x0_of_put_managers(priv, manager, nmanagers);
+	pd692x0_of_put_managers(priv, manager);
 	return ret;
 }
 
@@ -1644,7 +1679,7 @@ static enum fw_upload_err pd692x0_fw_poll_complete(struct fw_upload *fwl)
 		return FW_UPLOAD_ERR_FW_INVALID;
 	}
 
-	ret = pd692x0_setup_pi_matrix(&priv->pcdev);
+	ret = pd692x0_hw_conf_init(priv);
 	if (ret < 0) {
 		dev_err(&client->dev, "Error configuring ports matrix (%pe)\n",
 			ERR_PTR(ret));

-- 
2.43.0

Re: [PATCH net-next 1/2] net: pse-pd: pd692x0: Separate configuration parsing from hardware setup

[Jakub Kicinski]

On Fri, 22 Aug 2025 17:37:01 +0200 Kory Maincent wrote:
>  	manager = kcalloc(PD692X0_MAX_MANAGERS, sizeof(*manager), GFP_KERNEL);
>  	if (!manager)
>  		return -ENOMEM;
>  
> +	port_matrix = devm_kcalloc(&priv->client->dev, PD692X0_MAX_PIS,
> +				   sizeof(*port_matrix), GFP_KERNEL);
> +	if (!port_matrix)
> +		return -ENOMEM;

Leaking manager..

Re: [PATCH net-next 1/2] net: pse-pd: pd692x0: Separate configuration parsing from hardware setup

[Kory Maincent]

Le Mon, 25 Aug 2025 15:10:01 -0700,
Jakub Kicinski <kuba@kernel.org> a écrit :

> On Fri, 22 Aug 2025 17:37:01 +0200 Kory Maincent wrote:
> >  	manager = kcalloc(PD692X0_MAX_MANAGERS, sizeof(*manager),
> > GFP_KERNEL); if (!manager)
> >  		return -ENOMEM;
> >  
> > +	port_matrix = devm_kcalloc(&priv->client->dev, PD692X0_MAX_PIS,
> > +				   sizeof(*port_matrix), GFP_KERNEL);
> > +	if (!port_matrix)
> > +		return -ENOMEM;  
> 
> Leaking manager..

I don't think so, as manager is declared like the following it should not.
struct pd692x0_manager *manager __free(kfree) = NULL;

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

Re: [PATCH net-next 1/2] net: pse-pd: pd692x0: Separate configuration parsing from hardware setup

[Jakub Kicinski]

On Thu, 28 Aug 2025 10:46:12 +0200 Kory Maincent wrote:
> > On Fri, 22 Aug 2025 17:37:01 +0200 Kory Maincent wrote:  
> > >  	manager = kcalloc(PD692X0_MAX_MANAGERS, sizeof(*manager),
> > > GFP_KERNEL); if (!manager)
> > >  		return -ENOMEM;
> > >  
> > > +	port_matrix = devm_kcalloc(&priv->client->dev, PD692X0_MAX_PIS,
> > > +				   sizeof(*port_matrix), GFP_KERNEL);
> > > +	if (!port_matrix)
> > > +		return -ENOMEM;    
> > 
> > Leaking manager..  
> 
> I don't think so, as manager is declared like the following it should not.
> struct pd692x0_manager *manager __free(kfree) = NULL;

Please consult documentation on the user of __free() within networking.

Re: [PATCH net-next 1/2] net: pse-pd: pd692x0: Separate configuration parsing from hardware setup

[Kory Maincent]

On Thu, 28 Aug 2025 15:12:18 -0700
Jakub Kicinski <kuba@kernel.org> wrote:

> On Thu, 28 Aug 2025 10:46:12 +0200 Kory Maincent wrote:
> > > On Fri, 22 Aug 2025 17:37:01 +0200 Kory Maincent wrote:    
>  [...]  
> > > 
> > > Leaking manager..    
> > 
> > I don't think so, as manager is declared like the following it should not.
> > struct pd692x0_manager *manager __free(kfree) = NULL;  
> 
> Please consult documentation on the user of __free() within networking.

Oh, I didn't know about this net policy.
I didn't follow the maintainer-netdev doc changes. Maybe I should.
Ok, I will add a patch to the series to remove the __free macro. 

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

Re: [PATCH net-next 1/2] net: pse-pd: pd692x0: Separate configuration parsing from hardware setup

[Jakub Kicinski]

On Fri, 29 Aug 2025 10:26:24 +0200 Kory Maincent wrote:
> > > I don't think so, as manager is declared like the following it should not.
> > > struct pd692x0_manager *manager __free(kfree) = NULL;    
> > 
> > Please consult documentation on the user of __free() within networking.  
> 
> Oh, I didn't know about this net policy.
> I didn't follow the maintainer-netdev doc changes. Maybe I should.
> Ok, I will add a patch to the series to remove the __free macro. 

Up to you on existing code, but maybe avoid using it in the future?

[PATCH net-next 2/2] net: pse-pd: pd692x0: Add sysfs interface for configuration save/reset

[Kory Maincent]

From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>

Add sysfs attributes save_conf and reset_conf to enable userspace
management of the PSE's permanent configuration stored in EEPROM.

The save_conf attribute allows saving the current configuration to
EEPROM by writing '1'. The reset_conf attribute restores factory
defaults and reinitializes the port matrix configuration.

Skip hardware configuration initialization on probe when a saved
configuration is already present in EEPROM (detected via user byte 42).

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 drivers/net/pse-pd/pd692x0.c | 151 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 148 insertions(+), 3 deletions(-)

diff --git a/drivers/net/pse-pd/pd692x0.c b/drivers/net/pse-pd/pd692x0.c
index 8b94967bb4fb..202e91ec9b9a 100644
--- a/drivers/net/pse-pd/pd692x0.c
+++ b/drivers/net/pse-pd/pd692x0.c
@@ -30,6 +30,8 @@
 #define PD692X0_FW_MIN_VER	5
 #define PD692X0_FW_PATCH_VER	5
 
+#define PD692X0_USER_BYTE	42
+
 enum pd692x0_fw_state {
 	PD692X0_FW_UNKNOWN,
 	PD692X0_FW_OK,
@@ -80,6 +82,9 @@ enum {
 	PD692X0_MSG_GET_PORT_PARAM,
 	PD692X0_MSG_GET_POWER_BANK,
 	PD692X0_MSG_SET_POWER_BANK,
+	PD692X0_MSG_SET_USER_BYTE,
+	PD692X0_MSG_SAVE_SYS_SETTINGS,
+	PD692X0_MSG_RESTORE_FACTORY,
 
 	/* add new message above here */
 	PD692X0_MSG_CNT
@@ -103,6 +108,7 @@ struct pd692x0_priv {
 	bool last_cmd_key;
 	unsigned long last_cmd_key_time;
 
+	bool cfg_saved;
 	enum ethtool_c33_pse_admin_state admin_state[PD692X0_MAX_PIS];
 	struct regulator_dev *manager_reg[PD692X0_MAX_MANAGERS];
 	int manager_pw_budget[PD692X0_MAX_MANAGERS];
@@ -193,6 +199,24 @@ static const struct pd692x0_msg pd692x0_msg_template_list[PD692X0_MSG_CNT] = {
 		.key = PD692X0_KEY_CMD,
 		.sub = {0x07, 0x0b, 0x57},
 	},
+	[PD692X0_MSG_SET_USER_BYTE] = {
+		.key = PD692X0_KEY_PRG,
+		.sub = {0x41, PD692X0_USER_BYTE},
+		.data = {0x4e, 0x4e, 0x4e, 0x4e,
+			 0x4e, 0x4e, 0x4e, 0x4e},
+	},
+	[PD692X0_MSG_SAVE_SYS_SETTINGS] = {
+		.key = PD692X0_KEY_PRG,
+		.sub = {0x06, 0x0f},
+		.data = {0x4e, 0x4e, 0x4e, 0x4e,
+			 0x4e, 0x4e, 0x4e, 0x4e},
+	},
+	[PD692X0_MSG_RESTORE_FACTORY] = {
+		.key = PD692X0_KEY_PRG,
+		.sub = {0x2d, 0x4e},
+		.data = {0x4e, 0x4e, 0x4e, 0x4e,
+			 0x4e, 0x4e, 0x4e, 0x4e},
+	},
 };
 
 static u8 pd692x0_build_msg(struct pd692x0_msg *msg, u8 echo)
@@ -1266,9 +1290,12 @@ static int pd692x0_setup_pi_matrix(struct pse_controller_dev *pcdev)
 	if (ret)
 		goto err_managers_req_pw;
 
-	ret = pd692x0_hw_conf_init(priv);
-	if (ret)
-		goto err_managers_req_pw;
+	/* Do not init the conf if it is already saved */
+	if (!priv->cfg_saved) {
+		ret = pd692x0_hw_conf_init(priv);
+		if (ret)
+			goto err_managers_req_pw;
+	}
 
 	pd692x0_of_put_managers(priv, manager);
 	return 0;
@@ -1722,6 +1749,104 @@ static const struct fw_upload_ops pd692x0_fw_ops = {
 	.cleanup = pd692x0_fw_cleanup,
 };
 
+static ssize_t save_conf_store(struct device *dev,
+			       struct device_attribute *attr,
+			       const char *save_buf, size_t count)
+{
+	struct pd692x0_priv *priv = dev_get_drvdata(dev);
+	struct pd692x0_msg msg, buf = {0};
+	bool save;
+	int ret;
+
+	if (kstrtobool(save_buf, &save) || !save)
+		return -EINVAL;
+
+	mutex_lock(&priv->pcdev.lock);
+	ret = pd692x0_fw_unavailable(priv);
+	if (ret)
+		goto out;
+
+	msg = pd692x0_msg_template_list[PD692X0_MSG_SET_USER_BYTE];
+	ret = pd692x0_sendrecv_msg(priv, &msg, &buf);
+	if (ret)
+		goto out;
+
+	msg = pd692x0_msg_template_list[PD692X0_MSG_SAVE_SYS_SETTINGS];
+	ret = pd692x0_send_msg(priv, &msg);
+	if (ret) {
+		dev_err(&priv->client->dev,
+			"Failed to save the configuration (%pe)\n",
+			ERR_PTR(ret));
+		goto out;
+	}
+
+	msleep(50); /* Sleep 50ms as described in the datasheet */
+
+	ret = i2c_master_recv(priv->client, (u8 *)&buf, sizeof(buf));
+	if (ret != sizeof(buf)) {
+		if (ret >= 0)
+			ret = -EIO;
+		goto out;
+	}
+
+	ret = count;
+out:
+	mutex_unlock(&priv->pcdev.lock);
+
+	return ret;
+}
+static DEVICE_ATTR_WO(save_conf);
+
+static ssize_t reset_conf_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *reset_buf, size_t count)
+{
+	struct pd692x0_priv *priv = dev_get_drvdata(dev);
+	struct pd692x0_msg msg, buf = {0};
+	bool reset;
+	int ret;
+
+	if (kstrtobool(reset_buf, &reset) || !reset)
+		return -EINVAL;
+
+	mutex_lock(&priv->pcdev.lock);
+	ret = pd692x0_fw_unavailable(priv);
+	if (ret)
+		goto out;
+
+	msg = pd692x0_msg_template_list[PD692X0_MSG_RESTORE_FACTORY];
+	ret = pd692x0_send_msg(priv, &msg);
+	if (ret) {
+		dev_err(&priv->client->dev,
+			"Failed to reset the configuration (%pe)\n",
+			ERR_PTR(ret));
+		goto out;
+	}
+
+	msleep(100); /* Sleep 100ms as described in the datasheet */
+
+	ret = i2c_master_recv(priv->client, (u8 *)&buf, sizeof(buf));
+	if (ret != sizeof(buf)) {
+		if (ret >= 0)
+			ret = -EIO;
+		goto out;
+	}
+
+	ret = pd692x0_hw_conf_init(priv);
+	if (ret < 0) {
+		dev_err(&priv->client->dev,
+			"Error configuring ports matrix (%pe)\n",
+			ERR_PTR(ret));
+	}
+
+	ret = count;
+out:
+	mutex_unlock(&priv->pcdev.lock);
+
+	return ret;
+}
+static DEVICE_ATTR_WO(reset_conf);
+
 static int pd692x0_i2c_probe(struct i2c_client *client)
 {
 	static const char * const regulators[] = { "vdd", "vdda" };
@@ -1788,6 +1913,9 @@ static int pd692x0_i2c_probe(struct i2c_client *client)
 		}
 	}
 
+	if (buf.data[2] == PD692X0_USER_BYTE)
+		priv->cfg_saved = true;
+
 	priv->np = dev->of_node;
 	priv->pcdev.nr_lines = PD692X0_MAX_PIS;
 	priv->pcdev.owner = THIS_MODULE;
@@ -1808,7 +1936,22 @@ static int pd692x0_i2c_probe(struct i2c_client *client)
 				     "failed to register to the Firmware Upload API\n");
 	priv->fwl = fwl;
 
+	ret = sysfs_create_file(&dev->kobj, &dev_attr_save_conf.attr);
+	if (ret)
+		goto err_sysfs;
+
+	ret = sysfs_create_file(&dev->kobj, &dev_attr_reset_conf.attr);
+	if (ret)
+		goto err_sysfs_reset_conf;
+
 	return 0;
+
+err_sysfs_reset_conf:
+	sysfs_remove_file(&dev->kobj, &dev_attr_save_conf.attr);
+err_sysfs:
+	firmware_upload_unregister(priv->fwl);
+
+	return ret;
 }
 
 static void pd692x0_i2c_remove(struct i2c_client *client)
@@ -1816,6 +1959,8 @@ static void pd692x0_i2c_remove(struct i2c_client *client)
 	struct pd692x0_priv *priv = i2c_get_clientdata(client);
 
 	pd692x0_managers_free_pw_budget(priv);
+	sysfs_remove_file(&client->dev.kobj, &dev_attr_reset_conf.attr);
+	sysfs_remove_file(&client->dev.kobj, &dev_attr_save_conf.attr);
 	firmware_upload_unregister(priv->fwl);
 }
 

-- 
2.43.0

Re: [PATCH net-next 2/2] net: pse-pd: pd692x0: Add sysfs interface for configuration save/reset

[Andrew Lunn]

On Fri, Aug 22, 2025 at 05:37:02PM +0200, Kory Maincent wrote:
> From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
> 
> Add sysfs attributes save_conf and reset_conf to enable userspace
> management of the PSE's permanent configuration stored in EEPROM.
> 
> The save_conf attribute allows saving the current configuration to
> EEPROM by writing '1'. The reset_conf attribute restores factory
> defaults and reinitializes the port matrix configuration.

I'm not sure sysfs is the correct interface for this.

Lets take a step back.

I assume ethtool will report the correct state after a reboot when the
EEPROM has content? The driver does not hold configuration state which
cannot be represented in the EEPROM?

Is the EEPROM mandatory, or optional? Is it built into the controller?

How fast is it to store the settings?

I'm wondering if rather than having this sysfs parameter, you just
store every configuration change? That could be more intuitive.

I've not looked at the sysfs documentation. Are there other examples
of such a property?

      Andrew

Re: [PATCH net-next 2/2] net: pse-pd: pd692x0: Add sysfs interface for configuration save/reset

[Kory Maincent]

Hello Andrew,

Le Fri, 22 Aug 2025 19:17:55 +0200,
Andrew Lunn <andrew@lunn.ch> a écrit :

> On Fri, Aug 22, 2025 at 05:37:02PM +0200, Kory Maincent wrote:
> > From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
> > 
> > Add sysfs attributes save_conf and reset_conf to enable userspace
> > management of the PSE's permanent configuration stored in EEPROM.
> > 
> > The save_conf attribute allows saving the current configuration to
> > EEPROM by writing '1'. The reset_conf attribute restores factory
> > defaults and reinitializes the port matrix configuration.  
> 
> I'm not sure sysfs is the correct interface for this.
> 
> Lets take a step back.
> 
> I assume ethtool will report the correct state after a reboot when the
> EEPROM has content? The driver does not hold configuration state which
> cannot be represented in the EEPROM?

In fact I assumed it is an EEPROM but it is described as non volatile memory
so I don't know which type it is.

Yes ethtool report the current configuration which match the saved one if it has
been saved before. No the driver doesn't hold any state that can not be
represented in the non-volatile memory.

> Is the EEPROM mandatory, or optional? Is it built into the controller?

It is built into the controller. It seem there are version of this
controller that does not support it : "This command is not supported by
PD69200M."

> How fast is it to store the settings?

2 i2c messages and a 50 ms wait as described in the datasheet.
 
> I'm wondering if rather than having this sysfs parameter, you just
> store every configuration change? That could be more intuitive.

I have not thought of it. I don't know if it is a good idea. We may need
feedback from people that actually use PSE on field. Kyle any idea on this?
In any case we still need a way to reset the configuration through sysfs or
whatever other way.

> I've not looked at the sysfs documentation. Are there other examples
> of such a property?

Not sure for that particular save/reset configuration case.
Have you another implementation idea in mind?

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

Re: [PATCH net-next 2/2] net: pse-pd: pd692x0: Add sysfs interface for configuration save/reset

[Oleksij Rempel]

Hello Kory,

On Mon, Aug 25, 2025 at 10:47:21AM +0200, Kory Maincent wrote:
> > I've not looked at the sysfs documentation. Are there other examples
> > of such a property?
> 
> Not sure for that particular save/reset configuration case.
> Have you another implementation idea in mind?

My personal preference would be to use devlink (netlink based)
interface. We will have more controller/domain specific functions which can't
be represented per port.

PS: if you are on the OSS Amsterdam, we can talk in person about it.

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: [PATCH net-next 2/2] net: pse-pd: pd692x0: Add sysfs interface for configuration save/reset

[Andrew Lunn]

On Mon, Aug 25, 2025 at 11:14:03AM +0200, Oleksij Rempel wrote:
> Hello Kory,
> 
> On Mon, Aug 25, 2025 at 10:47:21AM +0200, Kory Maincent wrote:
> > > I've not looked at the sysfs documentation. Are there other examples
> > > of such a property?
> > 
> > Not sure for that particular save/reset configuration case.
> > Have you another implementation idea in mind?
> 
> My personal preference would be to use devlink (netlink based)
> interface.

Yes, devlink also crossed my mind, probably devlink params. Although
saving the current configuration to non-volatile memory is more a meta
parameter.

	Andrew

Re: [PATCH net-next 2/2] net: pse-pd: pd692x0: Add sysfs interface for configuration save/reset

[Jakub Kicinski]

On Mon, 25 Aug 2025 14:18:25 +0200 Andrew Lunn wrote:
> On Mon, Aug 25, 2025 at 11:14:03AM +0200, Oleksij Rempel wrote:
> > On Mon, Aug 25, 2025 at 10:47:21AM +0200, Kory Maincent wrote:  
> > > > I've not looked at the sysfs documentation. Are there other examples
> > > > of such a property?  
> > > 
> > > Not sure for that particular save/reset configuration case.
> > > Have you another implementation idea in mind?  
> > 
> > My personal preference would be to use devlink (netlink based)
> > interface.  
> 
> Yes, devlink also crossed my mind, probably devlink params. Although
> saving the current configuration to non-volatile memory is more a meta
> parameter.

This is a bit of a perennial topic. Most datacenter NIC vendors have 
a way to save link settings and alike to flash. None bothered with
adding upstream APIs tho. If the configs are fully within ethtool
I think we should be able to add an ethtool header flag that says 
"this config request is to be written to flash". And vice versa 
(get should read from flash)?

Resetting would work either via devlink reload, or ethtool --reset,
don't think we even need any API addition there.

Re: [PATCH net-next 2/2] net: pse-pd: pd692x0: Add sysfs interface for configuration save/reset

[Kory Maincent]

Le Mon, 25 Aug 2025 15:14:22 -0700,
Jakub Kicinski <kuba@kernel.org> a écrit :

> On Mon, 25 Aug 2025 14:18:25 +0200 Andrew Lunn wrote:
> > On Mon, Aug 25, 2025 at 11:14:03AM +0200, Oleksij Rempel wrote:  
> > > On Mon, Aug 25, 2025 at 10:47:21AM +0200, Kory Maincent wrote:    
>  [...]  
>  [...]  
> > > 
> > > My personal preference would be to use devlink (netlink based)
> > > interface.    
> > 
> > Yes, devlink also crossed my mind, probably devlink params. Although
> > saving the current configuration to non-volatile memory is more a meta
> > parameter.  
> 
> This is a bit of a perennial topic. Most datacenter NIC vendors have 
> a way to save link settings and alike to flash. None bothered with
> adding upstream APIs tho. If the configs are fully within ethtool
> I think we should be able to add an ethtool header flag that says 
> "this config request is to be written to flash". And vice versa 
> (get should read from flash)?

In fact there is the managers power budget parameter taken from the devicetree
which is not in ethtool config. It could be reconfigured after each reboot or
conf reset but it is an example of non ethtool configuration and more could
appear in the future. Talking about perennial, ethtool is then maybe not a good
idea because we still will need a way to save these new global configurations
saved to the non-volatile mem.
I am not really familiar with devlink but indeed after a quick look devlink
seems more suitable for the PSE global configurations.
I don't really know if we should use devlink param and devlink reload or only
devlink param or a new devlink conf.

Or we could save the configuration on every change but it will bring 70ms (I2C
read/write + store waiting time) latency for every command.

> Resetting would work either via devlink reload, or ethtool --reset,
> don't think we even need any API addition there.

Is there no way for NIC to reset their configuration except through ethtool
reload?

PS: got a client mail issue so you might have receive two mails. Sorry for that.
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next 2/2] net: pse-pd: pd692x0: Add sysfs interface for configuration save/reset

[Jakub Kicinski]

On Thu, 28 Aug 2025 13:29:01 +0200 Kory Maincent wrote:
> > Resetting would work either via devlink reload, or ethtool --reset,
> > don't think we even need any API addition there.  
> 
> Is there no way for NIC to reset their configuration except through ethtool
> reload?

Hm, on second thought I'm actually no longer sure if even ethtool or
devlink are right. I think the expectation may be that ethtool resets
the underlying HW but doesn't actually lose the configuration.

Re: [PATCH net-next 2/2] net: pse-pd: pd692x0: Add sysfs interface for configuration save/reset

[Kory Maincent]

Hello Oleksij,

Le Mon, 25 Aug 2025 11:14:03 +0200,
Oleksij Rempel <o.rempel@pengutronix.de> a écrit :

> Hello Kory,
> 
> On Mon, Aug 25, 2025 at 10:47:21AM +0200, Kory Maincent wrote:
> > > I've not looked at the sysfs documentation. Are there other examples
> > > of such a property?  
> > 
> > Not sure for that particular save/reset configuration case.
> > Have you another implementation idea in mind?  
> 
> My personal preference would be to use devlink (netlink based)
> interface. We will have more controller/domain specific functions which can't
> be represented per port.

Ok, I never played with devlink but I will check. 

> PS: if you are on the OSS Amsterdam, we can talk in person about it.

Yes sure. Let's meet at the social event around a beer!

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