[PATCH net-next 1/4] net: phy: mscc: factorize code for LEDs mode

LEDs modes are set the same way, except they are offset by 4 times the
index of the LED.

Let's factorize all the code so that it's easier to add support for the
4 LEDs of the VSC8584 PHY.

Signed-off-by: Quentin Schulz <redacted>
---
 drivers/net/phy/mscc.c | 66 ++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 34 deletions(-)

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index 84ca9ff40ae0..02954cc0d2ad 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c

@@ -54,9 +54,9 @@ enum rgmii_rx_clock_delay {
 #define HP_AUTO_MDIX_X_OVER_IND_MASK	  0x2000
 
 #define MSCC_PHY_LED_MODE_SEL		  29
-#define LED_1_MODE_SEL_MASK		  0x00F0
-#define LED_0_MODE_SEL_MASK		  0x000F
-#define LED_1_MODE_SEL_POS		  4
+#define LED_MODE_SEL_POS(x)		  ((x) * 4)
+#define LED_MODE_SEL_MASK(x)		  (GENMASK(3, 0) << LED_MODE_SEL_POS(x))
+#define LED_MODE_SEL(x, mode)		  (((mode) << LED_MODE_SEL_POS(x)) & LED_MODE_SEL_MASK(x))
 
 #define MSCC_EXT_PAGE_ACCESS		  31
 #define MSCC_PHY_PAGE_STANDARD		  0x0000 /* Standard registers */

@@ -103,10 +103,11 @@ enum rgmii_rx_clock_delay {
 
 #define DOWNSHIFT_COUNT_MAX		  5
 
+#define MAX_LEDS			  4
 struct vsc8531_private {
 	int rate_magic;
-	u8 led_0_mode;
-	u8 led_1_mode;
+	u8 leds_mode[MAX_LEDS];
+	u8 nleds;
 };
 
 #ifdef CONFIG_OF_MDIO

@@ -140,14 +141,8 @@ static int vsc85xx_led_cntl_set(struct phy_device *phydev,
 
 	mutex_lock(&phydev->lock);
 	reg_val = phy_read(phydev, MSCC_PHY_LED_MODE_SEL);
-	if (led_num) {
-		reg_val &= ~LED_1_MODE_SEL_MASK;
-		reg_val |= (((u16)mode << LED_1_MODE_SEL_POS) &
-			    LED_1_MODE_SEL_MASK);
-	} else {
-		reg_val &= ~LED_0_MODE_SEL_MASK;
-		reg_val |= ((u16)mode & LED_0_MODE_SEL_MASK);
-	}
+	reg_val &= ~LED_MODE_SEL_MASK(led_num);
+	reg_val |= LED_MODE_SEL(led_num, (u16)mode);
 	rc = phy_write(phydev, MSCC_PHY_LED_MODE_SEL, reg_val);
 	mutex_unlock(&phydev->lock);
 

@@ -545,7 +540,7 @@ static int vsc85xx_set_tunable(struct phy_device *phydev,
 
 static int vsc85xx_config_init(struct phy_device *phydev)
 {
-	int rc;
+	int rc, i;
 	struct vsc8531_private *vsc8531 = phydev->priv;
 
 	rc = vsc85xx_default_config(phydev);

@@ -560,13 +555,12 @@ static int vsc85xx_config_init(struct phy_device *phydev)
 	if (rc)
 		return rc;
 
-	rc = vsc85xx_led_cntl_set(phydev, 1, vsc8531->led_1_mode);
-	if (rc)
-		return rc;
-
-	rc = vsc85xx_led_cntl_set(phydev, 0, vsc8531->led_0_mode);
-	if (rc)
-		return rc;
+	/* Support for only 2 LEDs */
+	for (i = 0; i < vsc8531->nleds; i++) {
+		rc = vsc85xx_led_cntl_set(phydev, i, vsc8531->leds_mode[i]);
+		if (rc)
+			return rc;
+	}
 
 	rc = genphy_config_init(phydev);
 

@@ -627,6 +621,10 @@ static int vsc85xx_probe(struct phy_device *phydev)
 	struct vsc8531_private *vsc8531;
 	int rate_magic;
 	int led_mode;
+	int i;
+	char led_dt_prop[19];
+	u8 default_mode[2] = {VSC8531_LINK_1000_ACTIVITY,
+	   VSC8531_LINK_100_ACTIVITY};
 
 	rate_magic = vsc85xx_edge_rate_magic_get(phydev);
 	if (rate_magic < 0)

@@ -639,19 +637,19 @@ static int vsc85xx_probe(struct phy_device *phydev)
 	phydev->priv = vsc8531;
 
 	vsc8531->rate_magic = rate_magic;
-
-	/* LED[0] and LED[1] mode */
-	led_mode = vsc85xx_dt_led_mode_get(phydev, "vsc8531,led-0-mode",
-					   VSC8531_LINK_1000_ACTIVITY);
-	if (led_mode < 0)
-		return led_mode;
-	vsc8531->led_0_mode = led_mode;
-
-	led_mode = vsc85xx_dt_led_mode_get(phydev, "vsc8531,led-1-mode",
-					   VSC8531_LINK_100_ACTIVITY);
-	if (led_mode < 0)
-		return led_mode;
-	vsc8531->led_1_mode = led_mode;
+	vsc8531->nleds = 2;
+
+	for (i = 0; i < vsc8531->nleds; i++) {
+		led_mode = sprintf(led_dt_prop, "vsc8531,led-%d-mode", i);
+		if (led_mode < 0)
+			return led_mode;
+
+		led_mode = vsc85xx_dt_led_mode_get(phydev, led_dt_prop,
+						   default_mode[i]);
+		if (led_mode < 0)
+			return led_mode;
+		vsc8531->leds_mode[i] = led_mode;
+	}
 
 	return 0;
 }

-- 
2.17.1

VSC8584 supports 4 LEDs while VSC8531 only supports 2. Let's factorize
the documentation for LED mode properties and give the 4 default values
(the first two being shared between VSC8531 and VSC8584).

Signed-off-by: Quentin Schulz <redacted>
---
 .../devicetree/bindings/net/mscc-phy-vsc8531.txt | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
index 4c7d1d384df0..4d3f8b07a286 100644
--- a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
+++ b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt

@@ -22,14 +22,16 @@ Optional properties:
 			  'vddmac'.
 			  Default value is 0%.
 			  Ref: Table:1 - Edge rate change (below).
-- vsc8531,led-0-mode	: LED mode. Specify how the LED[0] should behave.
-			  Allowed values are define in
+- vsc8531,led-[N]-mode	: LED mode. Specify how the LED[N] should behave.
+			  N depends on the number of LEDs supported by a
+			  PHY.
+			  Allowed values are defined in
 			  "include/dt-bindings/net/mscc-phy-vsc8531.h".
-			  Default value is VSC8531_LINK_1000_ACTIVITY (1).
-- vsc8531,led-1-mode	: LED mode. Specify how the LED[1] should behave.
-			  Allowed values are define in
-			  "include/dt-bindings/net/mscc-phy-vsc8531.h".
-			  Default value is VSC8531_LINK_100_ACTIVITY (2).
+			  Default values are VSC8531_LINK_1000_ACTIVITY (1),
+			  VSC8531_LINK_100_ACTIVITY (2),
+			  VSC8531_LINK_ACTIVITY (0) and
+			  VSC8531_DUPLEX_COLLISION (8).
+
 
 Table: 1 - Edge rate change
 ----------------------------------------------------------------|

-- 
2.17.1

Compatible isn't a required property for PHYs so let's remove it from
the binding DT of the VSC8531 PHYs.

Signed-off-by: Quentin Schulz <redacted>
---
 Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
index 0eedabe22cc3..664d9d0543fc 100644
--- a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
+++ b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt

@@ -1,10 +1,5 @@
 * Microsemi - vsc8531 Giga bit ethernet phy
 
-Required properties:
-- compatible	: Should contain phy id as "ethernet-phy-idAAAA.BBBB"
-		  The PHY device uses the binding described in
-		  Documentation/devicetree/bindings/net/phy.txt
-
 Optional properties:
 - vsc8531,vddmac	: The vddmac in mV. Allowed values is listed
 			  in the first row of Table 1 (below).

-- 
2.17.1

The "vsc8531,led-N-mode" property is read as a u8 in the driver and
there aren't a lot of modes anyway.

Without the "/bits/ 8" in front of the value of the property, the
value is stored as an u32 resulting in of_read_property_u8 to always
return 0.

Fix the example so that people using the property can actually use it.

Signed-off-by: Quentin Schulz <redacted>
---
 Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
index 664d9d0543fc..4c7d1d384df0 100644
--- a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
+++ b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt

@@ -63,6 +63,6 @@ Example:
                 compatible = "ethernet-phy-id0007.0570";
                 vsc8531,vddmac		= <3300>;
                 vsc8531,edge-slowdown	= <7>;
-                vsc8531,led-0-mode	= <LINK_1000_ACTIVITY>;
-                vsc8531,led-1-mode	= <LINK_100_ACTIVITY>;
+                vsc8531,led-0-mode	= /bits/ 8 <LINK_1000_ACTIVITY>;
+                vsc8531,led-1-mode	= /bits/ 8 <LINK_100_ACTIVITY>;
         };

-- 
2.17.1

On Mon, Jul 30, 2018 at 03:02:34PM +0200, Quentin Schulz wrote:

Compatible isn't a required property for PHYs so let's remove it from
the binding DT of the VSC8531 PHYs.

Signed-off-by: Quentin Schulz <redacted>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

On Mon, Jul 30, 2018 at 03:02:35PM +0200, Quentin Schulz wrote:

The "vsc8531,led-N-mode" property is read as a u8 in the driver and
there aren't a lot of modes anyway.

Without the "/bits/ 8" in front of the value of the property, the
value is stored as an u32 resulting in of_read_property_u8 to always
return 0.

Hi Quentin

on big endian systems. I'm expect this worked on little endian ARM. I
think the development work was done on a hacked RPi, if i remember
correctly.

I don't know the device tree language well enough...

Would this work?

vsc8531,led-1-mode	= < /bits/ 8  LINK_100_ACTIVITY>;

If so, you can make it part of the #define.

   Andrew

Hi Andrew,

On Mon, Jul 30, 2018 at 03:58:13PM +0200, Andrew Lunn wrote:

On Mon, Jul 30, 2018 at 03:02:35PM +0200, Quentin Schulz wrote:

quoted

The "vsc8531,led-N-mode" property is read as a u8 in the driver and
there aren't a lot of modes anyway.

Without the "/bits/ 8" in front of the value of the property, the
value is stored as an u32 resulting in of_read_property_u8 to always
return 0.

Hi Quentin

on big endian systems. I'm expect this worked on little endian ARM. I
think the development work was done on a hacked RPi, if i remember
correctly.

quoted

Fix the example so that people using the property can actually use it.

Signed-off-by: Quentin Schulz <redacted>
---
 Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
index 664d9d0543fc..4c7d1d384df0 100644
--- a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
+++ b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt

@@ -63,6 +63,6 @@ Example:
                 compatible = "ethernet-phy-id0007.0570";
                 vsc8531,vddmac		= <3300>;
                 vsc8531,edge-slowdown	= <7>;
-                vsc8531,led-0-mode	= <LINK_1000_ACTIVITY>;
-                vsc8531,led-1-mode	= <LINK_100_ACTIVITY>;
+                vsc8531,led-0-mode	= /bits/ 8 <LINK_1000_ACTIVITY>;
+                vsc8531,led-1-mode	= /bits/ 8 <LINK_100_ACTIVITY>;

I don't know the device tree language well enough...

Would this work?

vsc8531,led-1-mode	= < /bits/ 8  LINK_100_ACTIVITY>;

If so, you can make it part of the #define.

It does not compile unfortunately. According to the documentation of
DTC[1], /bits/ 8 should prefix the value/array so I guess we're out of
luck for using the same define in the kernel and DTS if we want to
abstract this prefix.

Quentin

[1] https://git.kernel.org/pub/scm/utils/dtc/dtc.git/tree/Documentation/dts-format.txt#n46

On Mon, Jul 30, 2018 at 03:02:34PM +0200, Quentin Schulz wrote:

Compatible isn't a required property for PHYs so let's remove it from
the binding DT of the VSC8531 PHYs.

Signed-off-by: Quentin Schulz <redacted>
---
 Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt | 5 -----
 1 file changed, 5 deletions(-)

Acked-by: Rob Herring <robh@kernel.org>

On Mon, Jul 30, 2018 at 03:02:35PM +0200, Quentin Schulz wrote:

The "vsc8531,led-N-mode" property is read as a u8 in the driver and
there aren't a lot of modes anyway.

Without the "/bits/ 8" in front of the value of the property, the
value is stored as an u32 resulting in of_read_property_u8 to always
return 0.

Humm, I thought this would return an error if the size was wrong, but 
there must have been some reason otherwise.

Fix the example so that people using the property can actually use it.

Signed-off-by: Quentin Schulz <redacted>
---
 Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Really, the driver should be changed to use u32 if that's what's already 
in use.

Either way,

Reviewed-by: Rob Herring <robh@kernel.org>

On Mon, Jul 30, 2018 at 03:02:36PM +0200, Quentin Schulz wrote:

VSC8584 supports 4 LEDs while VSC8531 only supports 2. Let's factorize
the documentation for LED mode properties and give the 4 default values
(the first two being shared between VSC8531 and VSC8584).

Signed-off-by: Quentin Schulz <redacted>
---
 .../devicetree/bindings/net/mscc-phy-vsc8531.txt | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

Reviewed-by: Rob Herring <robh@kernel.org>