[PATCH net-next v2 1/2] dt-bindings: net: pse-pd: microchip,pd692x0: add port disable GPIO

[PATCH net-next v2 1/2] dt-bindings: net: pse-pd: microchip,pd692x0: add port disable GPIO

[Robert Marko]

From: Robert Marko <robert.marko@sartura.hr>

Microchip PSE controllers have a dedicated port disable input that like it
name suggest, will disable PoE on all ports.

So, lets document that GPIO.

Acked-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Robert Marko <robert.marko@sartura.hr>
---
Changes in v2:
* Pick Acked-by from Rob

 .../devicetree/bindings/net/pse-pd/microchip,pd692x0.yaml     | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/pse-pd/microchip,pd692x0.yaml b/Documentation/devicetree/bindings/net/pse-pd/microchip,pd692x0.yaml
index ca61cc37a790..ca4200afa793 100644
--- a/Documentation/devicetree/bindings/net/pse-pd/microchip,pd692x0.yaml
+++ b/Documentation/devicetree/bindings/net/pse-pd/microchip,pd692x0.yaml
@@ -22,6 +22,10 @@ properties:
   reg:
     maxItems: 1
 
+  disable-ports-gpios:
+    description: GPIO pin to disable PoE on all ports
+    maxItems: 1
+
   vdd-supply:
     description: Regulator that provides 3.3V VDD power supply.
 
-- 
2.54.0

[PATCH net-next v2 2/2] net: pse-pd: pd692x0: support disabling disable ports GPIO

[Robert Marko]

From: Robert Marko <robert.marko@sartura.hr>

Microchip PSE controllers have a dedicated disable ports input that like it
name says disables PoE on all ports.

So lets support parsing that GPIO and using the GPIO flags to set it to
output high by default and enable PoE on all ports during probe.

Signed-off-by: Robert Marko <robert.marko@sartura.hr>
---
Changes in v2:
* Use GPIOD_OUT_LOW by default so that DTS reflects the real active state

 drivers/net/pse-pd/pd692x0.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/pse-pd/pd692x0.c b/drivers/net/pse-pd/pd692x0.c
index 4a3c852780f5..6c5c5a26bbaa 100644
--- a/drivers/net/pse-pd/pd692x0.c
+++ b/drivers/net/pse-pd/pd692x0.c
@@ -7,6 +7,7 @@
 
 #include <linux/delay.h>
 #include <linux/firmware.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/of.h>
@@ -1757,6 +1758,7 @@ static int pd692x0_i2c_probe(struct i2c_client *client)
 	static const char * const regulators[] = { "vdd", "vdda" };
 	struct pd692x0_msg msg, buf = {0}, zero = {0};
 	struct device *dev = &client->dev;
+	struct gpio_desc *disable_ports;
 	struct pd692x0_msg_ver ver;
 	struct pd692x0_priv *priv;
 	struct fw_upload *fwl;
@@ -1780,6 +1782,11 @@ static int pd692x0_i2c_probe(struct i2c_client *client)
 	priv->client = client;
 	i2c_set_clientdata(client, priv);
 
+	disable_ports = devm_gpiod_get_optional(dev, "disable-ports", GPIOD_OUT_LOW);
+	if (IS_ERR(disable_ports))
+		return dev_err_probe(&client->dev, PTR_ERR(disable_ports),
+				     "Failed to get disable ports GPIO\n");
+
 	ret = i2c_master_recv(client, (u8 *)&buf, sizeof(buf));
 	if (ret != sizeof(buf)) {
 		dev_err(dev, "Failed to get device status\n");
-- 
2.54.0

Re: [PATCH net-next v2 2/2] net: pse-pd: pd692x0: support disabling disable ports GPIO

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Contradiction between commit message and code regarding the initial GPIO state.
- [Low] Inconsistent usage of the device pointer in the error handling path.
--

commit e53266c93f6ce835ab8c0c15eb780fa85ffd5a46
Author: Robert Marko <robert.marko@sartura.hr>

net: pse-pd: pd692x0: support disabling disable ports GPIO

This commit adds support for parsing the disable ports GPIO on Microchip
PSE controllers and configuring its default state to enable PoE during probe.

> So lets support parsing that GPIO and using the GPIO flags to set it to
> output high by default and enable PoE on all ports during probe.

Does the commit message contradict the implementation? 

The message mentions setting the GPIO to "output high by default", but the
code below uses GPIOD_OUT_LOW. 

While setting a "disable-ports" pin low to enable PoE makes logical sense,
could the commit message be updated to accurately reflect the code's behavior?

> diff --git a/drivers/net/pse-pd/pd692x0.c b/drivers/net/pse-pd/pd692x0.c
> index 4a3c852780f58..6c5c5a26bbaa6 100644
> --- a/drivers/net/pse-pd/pd692x0.c
> +++ b/drivers/net/pse-pd/pd692x0.c
[ ... ]
> @@ -1780,6 +1782,11 @@ static int pd692x0_i2c_probe(struct i2c_client *client)
>  	priv->client = client;
>  	i2c_set_clientdata(client, priv);
>  
> +	disable_ports = devm_gpiod_get_optional(dev, "disable-ports", GPIOD_OUT_LOW);
> +	if (IS_ERR(disable_ports))
> +		return dev_err_probe(&client->dev, PTR_ERR(disable_ports),
> +				     "Failed to get disable ports GPIO\n");

This isn't a bug, but should this use the local 'dev' variable instead of
dereferencing '&client->dev' again? 

The 'dev' variable is used in the devm_gpiod_get_optional() call just above
this line.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515115050.691119-1-robimarko@gmail.com?part=2