[PATCH net-next v2 0/2] net: phy: adin: add support for setting led-, link-status-pin polarity

[PATCH net-next v2 0/2] net: phy: adin: add support for setting led-, link-status-pin polarity

[Josua Mayer]

ADIN1300/1200 support software control over pin polarity for both LED_0
and LINK_ST pins.

LED_0 polarity can be supported in led framework with led_polarity_set
callback in  a future patch-set.
LINK_ST is a fixed-function output not suitable for led framework.

Add bindings for specifying the polarity of LINK_ST pin in device-tree,
and implement in adin phy driver.

Signed-off-by: Josua Mayer <josua@solid-run.com>
---
Changes in v2:
- removed led0 configuration since it can be supported through led
  framework
- Link to v1: https://lore.kernel.org/r/20240419-adin-pin-polarity-v1-0-eaae8708db8d@solid-run.com

---
Josua Mayer (2):
      dt-bindings: net: adin: add property for link-status pin polarity
      net: phy: adin: add support for setting link-status-pin polarity

 .../devicetree/bindings/net/adi,adin.yaml          |  9 +++++++
 drivers/net/phy/adin.c                             | 29 ++++++++++++++++++++++
 2 files changed, 38 insertions(+)
---
base-commit: 4cece764965020c22cff7665b18a012006359095
change-id: 20240419-adin-pin-polarity-30f7f85f6756

Sincerely,
-- 
Josua Mayer <josua@solid-run.com>

[PATCH net-next v2 1/2] dt-bindings: net: adin: add property for link-status pin polarity

[Josua Mayer]

ADIN1200/1300 support software control over LINK_ST pin polarity.
This is a fixed function output reflecting phy link-status that is not
otherwise controllable by software.

Add new property adi,link-st-polarity for specifying polarity as either
active-high (reset-default) or active-low.

Signed-off-by: Josua Mayer <josua@solid-run.com>
---
 Documentation/devicetree/bindings/net/adi,adin.yaml | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/adi,adin.yaml b/Documentation/devicetree/bindings/net/adi,adin.yaml
index 929cf8c0b0fd..cf195e070b26 100644
--- a/Documentation/devicetree/bindings/net/adi,adin.yaml
+++ b/Documentation/devicetree/bindings/net/adi,adin.yaml
@@ -52,6 +52,15 @@ properties:
     description: Enable 25MHz reference clock output on CLK25_REF pin.
     type: boolean
 
+  adi,link-st-polarity:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      LINK_ST pin polarity.
+    enum:
+      - 0 # active high
+      - 1 # active low
+    default: 0
+
 unevaluatedProperties: false
 
 examples:

-- 
2.35.3

Re: [PATCH net-next v2 1/2] dt-bindings: net: adin: add property for link-status pin polarity

[Andrew Lunn]

> +  adi,link-st-polarity:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      LINK_ST pin polarity.
> +    enum:
> +      - 0 # active high
> +      - 1 # active low
> +    default: 0
> +

How does this differ from:

Documentation/devicetree/bindings/leds/common.yaml

+  active-low:
+    type: boolean
+    description:
+      Makes LED active low. To turn the LED ON, line needs to be
+      set to low voltage instead of high.


Why do we need a vendor property when there is a generic property?

	Andrew

Re: [PATCH net-next v2 1/2] dt-bindings: net: adin: add property for link-status pin polarity

[Josua Mayer]

Am 20.04.24 um 18:10 schrieb Andrew Lunn:
>> +  adi,link-st-polarity:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description:
>> +      LINK_ST pin polarity.
>> +    enum:
>> +      - 0 # active high
>> +      - 1 # active low
>> +    default: 0
>> +
> How does this differ from:
>
> Documentation/devicetree/bindings/leds/common.yaml
>
> +  active-low:
> +    type: boolean
> +    description:
> +      Makes LED active low. To turn the LED ON, line needs to be
> +      set to low voltage instead of high.
>
>
> Why do we need a vendor property when there is a generic property?
It differs in tree depth and naming.
To use led binding we need to add a leds node with a led node inside,
and decide on an index for this not-an-led pin.
Sure, could be done but  maybe should be documented somewhere
as it is not intuitive.

Main reason for having a vendor-specific and non-led property
is that this pin is not a led, it is merely a signal.
The PHY can  be configured to provide via this signal either:
- link-status
- collision detection
- carrier sense
- tx packet start
- rx packet start

The purpose of the binding I propose is just polarity of this signal.
A more complete binding would also allow selection from the
above listed functions.
This kind of configuration is much more like pinctrl than led.

Re: [PATCH net-next v2 1/2] dt-bindings: net: adin: add property for link-status pin polarity

[Andrew Lunn]

> Main reason for having a vendor-specific and non-led property
> is that this pin is not a led.

So you are not driving an LED with its? What are you using it for?

> This kind of configuration is much more like pinctrl than led.
 
So what is the pinctrl way of describing this? You should not be
inventing something new if there is an existing mechanism to describe
it. We want consistency, not 42 different ways of doing one thing.

	Andrew

Re: [PATCH net-next v2 1/2] dt-bindings: net: adin: add property for link-status pin polarity

[Josua Mayer]

Am 20.04.24 um 19:09 schrieb Andrew Lunn:
>> Main reason for having a vendor-specific and non-led property
>> is that this pin is not a led.
> So you are not driving an LED with its? What are you using it for?

The unit I am currently working on connects an LED, yes.

Therefore I agree with you that it could be described adequately
by an led node with active-low property.

I merely don't like the idea that this makes no sense for the other
possible pin functions.
Once somebody uses this pin for different use-case, they will need
to solve it again.

>> This kind of configuration is much more like pinctrl than led.
>  
> So what is the pinctrl way of describing this? You should not be
> inventing something new if there is an existing mechanism to describe
> it. We want consistency, not 42 different ways of doing one thing.
I am mostly familiar with the
#define PIN_FUNCTION magic-numbers
pins = <PIN_FUNCTION more-magic-numbers>;

But on Marvell platforms there is:
marvell,pins =  "mpp1";
marvell,function = "gpio";

I also found more generic???:
Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
Documentation/devicetree/bindings/pinctrl/pinmux-node.yaml
which have output-high/output-low, function, pin.

Interestingly LED_0 supports some non-led functions, too:
- collision detection
- carrier sense
- tx/rx start
- tx error
so polarity is also relevant to non-led usage of LED_0 pin.

Might be I am not seeing the big picture how this fits a generic structure.

Re: [PATCH net-next v2 1/2] dt-bindings: net: adin: add property for link-status pin polarity

[Andrew Lunn]

> I merely don't like the idea that this makes no sense for the other
> possible pin functions.
> Once somebody uses this pin for different use-case, they will need
> to solve it again.

There are not too many different uses of this pin. The data sheet
indicates you can connect it to the MAC to indicate link. You might
also be able to use it with an external PTP stamper, using the start
of frame indication.

I don't know of any bindings for such use case, but something will be
needed to describe how the pin is connected to the other device. And
at that point, the active low property could be used.

> >> This kind of configuration is much more like pinctrl than led.
> >  
> > So what is the pinctrl way of describing this? You should not be
> > inventing something new if there is an existing mechanism to describe
> > it. We want consistency, not 42 different ways of doing one thing.
> I am mostly familiar with the
> #define PIN_FUNCTION magic-numbers
> pins = <PIN_FUNCTION more-magic-numbers>;
> 
> But on Marvell platforms there is:
> marvell,pins =  "mpp1";
> marvell,function = "gpio";
> 
> I also found more generic???:
> Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
> Documentation/devicetree/bindings/pinctrl/pinmux-node.yaml
> which have output-high/output-low, function, pin.

So that is probably your alternative if you want to not use the LED
binding.

> 
> Interestingly LED_0 supports some non-led functions, too:
> - collision detection
> - carrier sense
> - tx/rx start
> - tx error
> so polarity is also relevant to non-led usage of LED_0 pin.

Collision detection is an LED usage, you just don't see it very often
since half duplex is pretty unusual this century. Carrier sense is
also similar age, from when Ethernet was CSMA/CD.

Since they are not really used any more we don't have them in the LED
framework, but i think we could implement them if somebody actually
wanted them. My intention was to keep the LED framework KISS, since
vendors tend to implement all sorts of odd LED blink reasons. But if
nobody wants them, nobody has a good end user use case for them, why
support them?

       Andrew

Re: [PATCH net-next v2 1/2] dt-bindings: net: adin: add property for link-status pin polarity

[Josua Mayer]

Am 21.04.24 um 19:54 schrieb Andrew Lunn:
>> I merely don't like the idea that this makes no sense for the other
>> possible pin functions.
>> Once somebody uses this pin for different use-case, they will need
>> to solve it again.
> There are not too many different uses of this pin. The data sheet
> indicates you can connect it to the MAC to indicate link. You might
> also be able to use it with an external PTP stamper, using the start
> of frame indication.
>
> I don't know of any bindings for such use case, but something will be
> needed to describe how the pin is connected to the other device. And
> at that point, the active low property could be used.
>
>>>> This kind of configuration is much more like pinctrl than led.
>>>  
>>> So what is the pinctrl way of describing this? You should not be
>>> inventing something new if there is an existing mechanism to describe
>>> it. We want consistency, not 42 different ways of doing one thing.
>> I am mostly familiar with the
>> #define PIN_FUNCTION magic-numbers
>> pins = <PIN_FUNCTION more-magic-numbers>;
>>
>> But on Marvell platforms there is:
>> marvell,pins =  "mpp1";
>> marvell,function = "gpio";
>>
>> I also found more generic???:
>> Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
>> Documentation/devicetree/bindings/pinctrl/pinmux-node.yaml
>> which have output-high/output-low, function, pin.
> So that is probably your alternative if you want to not use the LED
> binding.
I will consider using pincfg/-mux
>
>> Interestingly LED_0 supports some non-led functions, too:
>> - collision detection
>> - carrier sense
>> - tx/rx start
>> - tx error
>> so polarity is also relevant to non-led usage of LED_0 pin.
> Collision detection is an LED usage, you just don't see it very often
> since half duplex is pretty unusual this century. Carrier sense is
> also similar age, from when Ethernet was CSMA/CD.
>
> Since they are not really used any more we don't have them in the LED
> framework, but i think we could implement them if somebody actually
> wanted them. My intention was to keep the LED framework KISS, since
> vendors tend to implement all sorts of odd LED blink reasons. But if
> nobody wants them, nobody has a good end user use case for them, why
> support them?
I see. So in fact most functions I wanted to enable muxing are LED functions,
leaving only some specifically for pinmux.

I believe pinmux is more correct, but there is overlap with led function.

I will  try to find some time for
1. describing both signals as LEDs, taking care of active-low
2. look into using pinmux (lower priority).
I think this would be more interesting to bigger phys with more muxable signals,
adin1300 is rather small.

Thank you for all the comments!

[PATCH net-next v2 2/2] net: phy: adin: add support for setting link-status-pin polarity

[Josua Mayer]

ADIN1300/1200 support software control over pin polarity for LINK_ST
(link-status) pin.

Configure the polarity during config_init based on device-tree property.

Polarity is only set if specified in device-tree, otherwise the phy
defaults to active-high during reset.

Signed-off-by: Josua Mayer <josua@solid-run.com>
---
 drivers/net/phy/adin.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
index 2e1a46e121d9..a533932afcb8 100644
--- a/drivers/net/phy/adin.c
+++ b/drivers/net/phy/adin.c
@@ -158,6 +158,9 @@
 #define ADIN1300_RMII_20_BITS			0x0004
 #define ADIN1300_RMII_24_BITS			0x0005
 
+#define ADIN1300_GE_LNK_STAT_INV_EN_REG		0xff3c
+#define   ADIN1300_GE_LNK_STAT_INV_EN		BIT(0)
+
 /**
  * struct adin_cfg_reg_map - map a config value to aregister value
  * @cfg:	value in device configuration
@@ -522,6 +525,28 @@ static int adin_config_clk_out(struct phy_device *phydev)
 			      ADIN1300_GE_CLK_CFG_MASK, sel);
 }
 
+static int adin_config_link_status_pin_polarity(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->mdio.dev;
+	int ret;
+	u32 val;
+
+	if (!device_property_present(dev, "adi,link-st-polarity"))
+		return 0;
+
+	ret = device_property_read_u32(dev, "adi,link-st-polarity", &val);
+	if (ret) {
+		return ret;
+	} else if (val > 1) {
+		phydev_err(phydev, "invalid adi,link-st-polarity\n");
+		return -EINVAL;
+	}
+
+	return phy_modify_mmd(phydev, MDIO_MMD_VEND1,
+			     ADIN1300_GE_LNK_STAT_INV_EN_REG,
+			     ADIN1300_GE_LNK_STAT_INV_EN, val);
+}
+
 static int adin_config_init(struct phy_device *phydev)
 {
 	int rc;
@@ -548,6 +573,10 @@ static int adin_config_init(struct phy_device *phydev)
 	if (rc < 0)
 		return rc;
 
+	rc = adin_config_link_status_pin_polarity(phydev);
+	if (rc < 0)
+		return rc;
+
 	phydev_dbg(phydev, "PHY is using mode '%s'\n",
 		   phy_modes(phydev->interface));
 

-- 
2.35.3