[PATCH 0/9] net: phy: micrel: refactoring and KSZ8081/KSZ8091 features

[PATCH 0/9] net: phy: micrel: refactoring and KSZ8081/KSZ8091 features

[Johan Hovold]

This series cleans up and refactors parts of the micrel PHY driver, and
adds support for broadcast-address-disable and led-mode configuration
for KSZ8081 and KSZ8091 PHYs.

Specifically, this enables dual KSZ8081 setups (which are limited to
using address 0 and 3).

A follow up series will add device-type abstraction which will allow for
further refactoring and shared initialisation code.

Johan


Johan Hovold (9):
  dt/bindings: fix documentation of ethernet-phy compatible property
  net: phy: micrel: fix config_intr error handling
  net: phy: micrel: use BIT macro
  net: phy: micrel: refactor broadcast disable
  net: phy: micrel: disable broadcast for KSZ8081/KSZ8091
  net: phy: micrel: add led-mode sanity check
  net: phy: micrel: refactor led-mode error handling
  net: phy: micrel: clean up led-mode setup
  net: phy: micrel: enable led-mode for KSZ8081/KSZ8091

 Documentation/devicetree/bindings/net/micrel.txt |   2 +
 Documentation/devicetree/bindings/net/phy.txt    |   3 +-
 drivers/net/phy/micrel.c                         | 125 ++++++++++++++++-------
 3 files changed, 93 insertions(+), 37 deletions(-)

-- 
2.0.4

[PATCH 1/9] dt/bindings: fix documentation of ethernet-phy compatible property

[Johan Hovold]

A recent commit extended the documentation of the ethernet-phy
compatible property, but placed the new paragraph under the max-speed
property.

Fixes: f00e756ed12d ("dt: Document a compatible entry for MDIO ethernet
Phys")
Cc: devicetree@vger.kernel.org
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 Documentation/devicetree/bindings/net/phy.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt
index 5b8c58903077..40831fbaff72 100644
--- a/Documentation/devicetree/bindings/net/phy.txt
+++ b/Documentation/devicetree/bindings/net/phy.txt
@@ -19,7 +19,6 @@ Optional Properties:
   specifications. If neither of these are specified, the default is to
   assume clause 22. The compatible list may also contain other
   elements.
-- max-speed: Maximum PHY supported speed (10, 100, 1000...)
 
   If the phy's identifier is known then the list may contain an entry
   of the form: "ethernet-phy-idAAAA.BBBB" where
@@ -29,6 +28,8 @@ Optional Properties:
             4 hex digits. This is the chip vendor OUI bits 19:24,
             followed by 10 bits of a vendor specific ID.
 
+- max-speed: Maximum PHY supported speed (10, 100, 1000...)
+
 Example:
 
 ethernet-phy@0 {
-- 
2.0.4

[PATCH 2/9] net: phy: micrel: fix config_intr error handling

[Johan Hovold]

Make sure never to update the control register with random data (an
error code) by checking the return value after reading it.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/net/phy/micrel.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index bcc6c0ea75fa..62ca9613a514 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -122,6 +122,8 @@ static int kszphy_config_intr(struct phy_device *phydev)
 
 	/* set the interrupt pin active low */
 	temp = phy_read(phydev, MII_KSZPHY_CTRL);
+	if (temp < 0)
+		return temp;
 	temp &= ~KSZPHY_CTRL_INT_ACTIVE_HIGH;
 	phy_write(phydev, MII_KSZPHY_CTRL, temp);
 	rc = kszphy_set_interrupt(phydev);
@@ -134,6 +136,8 @@ static int ksz9021_config_intr(struct phy_device *phydev)
 
 	/* set the interrupt pin active low */
 	temp = phy_read(phydev, MII_KSZPHY_CTRL);
+	if (temp < 0)
+		return temp;
 	temp &= ~KSZ9021_CTRL_INT_ACTIVE_HIGH;
 	phy_write(phydev, MII_KSZPHY_CTRL, temp);
 	rc = kszphy_set_interrupt(phydev);
@@ -146,6 +150,8 @@ static int ks8737_config_intr(struct phy_device *phydev)
 
 	/* set the interrupt pin active low */
 	temp = phy_read(phydev, MII_KSZPHY_CTRL);
+	if (temp < 0)
+		return temp;
 	temp &= ~KS8737_CTRL_INT_ACTIVE_HIGH;
 	phy_write(phydev, MII_KSZPHY_CTRL, temp);
 	rc = kszphy_set_interrupt(phydev);
-- 
2.0.4

[PATCH 3/9] net: phy: micrel: use BIT macro

[Johan Hovold]

Use BIT macro for bitmask definitions.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/net/phy/micrel.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 62ca9613a514..d962a2866bba 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -30,30 +30,30 @@
 
 /* Operation Mode Strap Override */
 #define MII_KSZPHY_OMSO				0x16
-#define KSZPHY_OMSO_B_CAST_OFF			(1 << 9)
-#define KSZPHY_OMSO_RMII_OVERRIDE		(1 << 1)
-#define KSZPHY_OMSO_MII_OVERRIDE		(1 << 0)
+#define KSZPHY_OMSO_B_CAST_OFF			BIT(9)
+#define KSZPHY_OMSO_RMII_OVERRIDE		BIT(1)
+#define KSZPHY_OMSO_MII_OVERRIDE		BIT(0)
 
 /* general Interrupt control/status reg in vendor specific block. */
 #define MII_KSZPHY_INTCS			0x1B
-#define	KSZPHY_INTCS_JABBER			(1 << 15)
-#define	KSZPHY_INTCS_RECEIVE_ERR		(1 << 14)
-#define	KSZPHY_INTCS_PAGE_RECEIVE		(1 << 13)
-#define	KSZPHY_INTCS_PARELLEL			(1 << 12)
-#define	KSZPHY_INTCS_LINK_PARTNER_ACK		(1 << 11)
-#define	KSZPHY_INTCS_LINK_DOWN			(1 << 10)
-#define	KSZPHY_INTCS_REMOTE_FAULT		(1 << 9)
-#define	KSZPHY_INTCS_LINK_UP			(1 << 8)
+#define	KSZPHY_INTCS_JABBER			BIT(15)
+#define	KSZPHY_INTCS_RECEIVE_ERR		BIT(14)
+#define	KSZPHY_INTCS_PAGE_RECEIVE		BIT(13)
+#define	KSZPHY_INTCS_PARELLEL			BIT(12)
+#define	KSZPHY_INTCS_LINK_PARTNER_ACK		BIT(11)
+#define	KSZPHY_INTCS_LINK_DOWN			BIT(10)
+#define	KSZPHY_INTCS_REMOTE_FAULT		BIT(9)
+#define	KSZPHY_INTCS_LINK_UP			BIT(8)
 #define	KSZPHY_INTCS_ALL			(KSZPHY_INTCS_LINK_UP |\
 						KSZPHY_INTCS_LINK_DOWN)
 
 /* general PHY control reg in vendor specific block. */
 #define	MII_KSZPHY_CTRL			0x1F
 /* bitmap of PHY register to set interrupt mode */
-#define KSZPHY_CTRL_INT_ACTIVE_HIGH		(1 << 9)
-#define KSZ9021_CTRL_INT_ACTIVE_HIGH		(1 << 14)
-#define KS8737_CTRL_INT_ACTIVE_HIGH		(1 << 14)
-#define KSZ8051_RMII_50MHZ_CLK			(1 << 7)
+#define KSZPHY_CTRL_INT_ACTIVE_HIGH		BIT(9)
+#define KSZ9021_CTRL_INT_ACTIVE_HIGH		BIT(14)
+#define KS8737_CTRL_INT_ACTIVE_HIGH		BIT(14)
+#define KSZ8051_RMII_50MHZ_CLK			BIT(7)
 
 /* Write/read to/from extended registers */
 #define MII_KSZPHY_EXTREG                       0x0b
@@ -400,8 +400,8 @@ static int ksz9031_config_init(struct phy_device *phydev)
 }
 
 #define KSZ8873MLL_GLOBAL_CONTROL_4	0x06
-#define KSZ8873MLL_GLOBAL_CONTROL_4_DUPLEX	(1 << 6)
-#define KSZ8873MLL_GLOBAL_CONTROL_4_SPEED	(1 << 4)
+#define KSZ8873MLL_GLOBAL_CONTROL_4_DUPLEX	BIT(6)
+#define KSZ8873MLL_GLOBAL_CONTROL_4_SPEED	BIT(4)
 static int ksz8873mll_read_status(struct phy_device *phydev)
 {
 	int regval;
-- 
2.0.4

[PATCH 4/9] net: phy: micrel: refactor broadcast disable

[Johan Hovold]

Refactor and clean up broadcast disable.

Some Micrel PHYs have a broadcast-off bit in the Operation Mode Strap
Override register which can be used to disable PHY address 0 as the
broadcast address, so that it can be used as a unique (non-broadcast)
address on a shared bus.

Note that the KSZPHY_OMSO_RMII_OVERRIDE bit is set by default on
KSZ8021/8031.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/net/phy/micrel.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index d962a2866bba..21abe5ade3df 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -184,6 +184,25 @@ static int kszphy_setup_led(struct phy_device *phydev,
 	return rc < 0 ? rc : 0;
 }
 
+/* Disable PHY address 0 as the broadcast address, so that it can be used as a
+ * unique (non-broadcast) address on a shared bus.
+ */
+static int kszphy_broadcast_disable(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = phy_read(phydev, MII_KSZPHY_OMSO);
+	if (ret < 0)
+		goto out;
+
+	ret = phy_write(phydev, MII_KSZPHY_OMSO, ret | KSZPHY_OMSO_B_CAST_OFF);
+out:
+	if (ret)
+		dev_err(&phydev->dev, "failed to disable broadcast address\n");
+
+	return ret;
+}
+
 static int kszphy_config_init(struct phy_device *phydev)
 {
 	return 0;
@@ -197,7 +216,6 @@ static int kszphy_config_init_led8041(struct phy_device *phydev)
 
 static int ksz8021_config_init(struct phy_device *phydev)
 {
-	const u16 val = KSZPHY_OMSO_B_CAST_OFF | KSZPHY_OMSO_RMII_OVERRIDE;
 	int rc;
 
 	rc = kszphy_setup_led(phydev, 0x1f, 4);
@@ -207,7 +225,9 @@ static int ksz8021_config_init(struct phy_device *phydev)
 	rc = ksz_config_flags(phydev);
 	if (rc < 0)
 		return rc;
-	rc = phy_write(phydev, MII_KSZPHY_OMSO, val);
+
+	rc = kszphy_broadcast_disable(phydev);
+
 	return rc < 0 ? rc : 0;
 }
 
-- 
2.0.4

[PATCH 5/9] net: phy: micrel: disable broadcast for KSZ8081/KSZ8091

[Johan Hovold]

Disable PHY address 0 as the broadcast address, so that it can be used
as a unique (non-broadcast) address on a shared bus.

Note that this can also be configured using the B-CAST_OFF pin on
KSZ9091, but that KSZ8081 lacks this pin and is also limited to
addresses 0 and 3.

Specifically, this allows for dual KSZ8081 setups.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/net/phy/micrel.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 21abe5ade3df..16135ac18bfe 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -243,6 +243,13 @@ static int ks8051_config_init(struct phy_device *phydev)
 	return rc < 0 ? rc : 0;
 }
 
+static int ksz8081_config_init(struct phy_device *phydev)
+{
+	kszphy_broadcast_disable(phydev);
+
+	return 0;
+}
+
 static int ksz9021_load_values_from_of(struct phy_device *phydev,
 				       struct device_node *of_node, u16 reg,
 				       char *field1, char *field2,
@@ -605,7 +612,7 @@ static struct phy_driver ksphy_driver[] = {
 	.phy_id_mask	= 0x00fffff0,
 	.features	= (PHY_BASIC_FEATURES | SUPPORTED_Pause),
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
-	.config_init	= kszphy_config_init,
+	.config_init	= ksz8081_config_init,
 	.config_aneg	= genphy_config_aneg,
 	.read_status	= genphy_read_status,
 	.ack_interrupt	= kszphy_ack_interrupt,
-- 
2.0.4

[PATCH 6/9] net: phy: micrel: add led-mode sanity check

[Johan Hovold]

Make sure never to update more than two bits when setting the led mode,
something which could for example change the reference-clock setting.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/net/phy/micrel.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 16135ac18bfe..1b3985cdc64c 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -173,6 +173,11 @@ static int kszphy_setup_led(struct phy_device *phydev,
 	if (of_property_read_u32(of_node, "micrel,led-mode", &val))
 		return 0;
 
+	if (val > 3) {
+		dev_err(&phydev->dev, "invalid led mode: 0x%02x\n", val);
+		return -EINVAL;
+	}
+
 	temp = phy_read(phydev, reg);
 	if (temp < 0)
 		return temp;
-- 
2.0.4

[PATCH 7/9] net: phy: micrel: refactor led-mode error handling

[Johan Hovold]

Refactor led-mode error handling.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/net/phy/micrel.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 1b3985cdc64c..ec9ce35e934b 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -179,14 +179,19 @@ static int kszphy_setup_led(struct phy_device *phydev,
 	}
 
 	temp = phy_read(phydev, reg);
-	if (temp < 0)
-		return temp;
+	if (temp < 0) {
+		rc = temp;
+		goto out;
+	}
 
 	temp &= ~(3 << shift);
 	temp |= val << shift;
 	rc = phy_write(phydev, reg, temp);
+out:
+	if (rc < 0)
+		dev_err(&phydev->dev, "failed to set led mode\n");
 
-	return rc < 0 ? rc : 0;
+	return rc;
 }
 
 /* Disable PHY address 0 as the broadcast address, so that it can be used as a
@@ -223,9 +228,7 @@ static int ksz8021_config_init(struct phy_device *phydev)
 {
 	int rc;
 
-	rc = kszphy_setup_led(phydev, 0x1f, 4);
-	if (rc)
-		dev_err(&phydev->dev, "failed to set led mode\n");
+	kszphy_setup_led(phydev, 0x1f, 4);
 
 	rc = ksz_config_flags(phydev);
 	if (rc < 0)
@@ -240,9 +243,7 @@ static int ks8051_config_init(struct phy_device *phydev)
 {
 	int rc;
 
-	rc = kszphy_setup_led(phydev, 0x1f, 4);
-	if (rc)
-		dev_err(&phydev->dev, "failed to set led mode\n");
+	kszphy_setup_led(phydev, 0x1f, 4);
 
 	rc = ksz_config_flags(phydev);
 	return rc < 0 ? rc : 0;
-- 
2.0.4

[PATCH 8/9] net: phy: micrel: clean up led-mode setup

[Johan Hovold]

Clean up led-mode setup by introducing proper defines for PHY Control
registers 1 and 2 and only passing the register to the setup function.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/net/phy/micrel.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index ec9ce35e934b..12e18f7273ce 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -47,8 +47,12 @@
 #define	KSZPHY_INTCS_ALL			(KSZPHY_INTCS_LINK_UP |\
 						KSZPHY_INTCS_LINK_DOWN)
 
-/* general PHY control reg in vendor specific block. */
-#define	MII_KSZPHY_CTRL			0x1F
+/* PHY Control 1 */
+#define	MII_KSZPHY_CTRL_1			0x1e
+
+/* PHY Control 2 / PHY Control (if no PHY Control 1) */
+#define	MII_KSZPHY_CTRL_2			0x1f
+#define	MII_KSZPHY_CTRL				MII_KSZPHY_CTRL_2
 /* bitmap of PHY register to set interrupt mode */
 #define KSZPHY_CTRL_INT_ACTIVE_HIGH		BIT(9)
 #define KSZ9021_CTRL_INT_ACTIVE_HIGH		BIT(14)
@@ -158,13 +162,12 @@ static int ks8737_config_intr(struct phy_device *phydev)
 	return rc < 0 ? rc : 0;
 }
 
-static int kszphy_setup_led(struct phy_device *phydev,
-			    unsigned int reg, unsigned int shift)
+static int kszphy_setup_led(struct phy_device *phydev, u32 reg)
 {
 
 	struct device *dev = &phydev->dev;
 	struct device_node *of_node = dev->of_node;
-	int rc, temp;
+	int rc, temp, shift;
 	u32 val;
 
 	if (!of_node && dev->parent->of_node)
@@ -178,6 +181,17 @@ static int kszphy_setup_led(struct phy_device *phydev,
 		return -EINVAL;
 	}
 
+	switch (reg) {
+	case MII_KSZPHY_CTRL_1:
+		shift = 14;
+		break;
+	case MII_KSZPHY_CTRL_2:
+		shift = 4;
+		break;
+	default:
+		return -EINVAL;
+	}
+
 	temp = phy_read(phydev, reg);
 	if (temp < 0) {
 		rc = temp;
@@ -220,15 +234,14 @@ static int kszphy_config_init(struct phy_device *phydev)
 
 static int kszphy_config_init_led8041(struct phy_device *phydev)
 {
-	/* single led control, register 0x1e bits 15..14 */
-	return kszphy_setup_led(phydev, 0x1e, 14);
+	return kszphy_setup_led(phydev, MII_KSZPHY_CTRL_1);
 }
 
 static int ksz8021_config_init(struct phy_device *phydev)
 {
 	int rc;
 
-	kszphy_setup_led(phydev, 0x1f, 4);
+	kszphy_setup_led(phydev, MII_KSZPHY_CTRL_2);
 
 	rc = ksz_config_flags(phydev);
 	if (rc < 0)
@@ -243,7 +256,7 @@ static int ks8051_config_init(struct phy_device *phydev)
 {
 	int rc;
 
-	kszphy_setup_led(phydev, 0x1f, 4);
+	kszphy_setup_led(phydev, MII_KSZPHY_CTRL_2);
 
 	rc = ksz_config_flags(phydev);
 	return rc < 0 ? rc : 0;
-- 
2.0.4

[PATCH 9/9] net: phy: micrel: enable led-mode for KSZ8081/KSZ8091

[Johan Hovold]

Enable led-mode configuration for KSZ8081 and KSZ8091.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 Documentation/devicetree/bindings/net/micrel.txt | 2 ++
 drivers/net/phy/micrel.c                         | 1 +
 2 files changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/micrel.txt b/Documentation/devicetree/bindings/net/micrel.txt
index e1d99b95c4ec..30062fae5623 100644
--- a/Documentation/devicetree/bindings/net/micrel.txt
+++ b/Documentation/devicetree/bindings/net/micrel.txt
@@ -14,6 +14,8 @@ Optional properties:
 	      KSZ8021: register 0x1f, bits 5..4
 	      KSZ8031: register 0x1f, bits 5..4
 	      KSZ8051: register 0x1f, bits 5..4
+	      KSZ8081: register 0x1f, bits 5..4
+	      KSZ8091: register 0x1f, bits 5..4
 
               See the respective PHY datasheet for the mode values.
 
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 12e18f7273ce..30e894d6ffbd 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -265,6 +265,7 @@ static int ks8051_config_init(struct phy_device *phydev)
 static int ksz8081_config_init(struct phy_device *phydev)
 {
 	kszphy_broadcast_disable(phydev);
+	kszphy_setup_led(phydev, MII_KSZPHY_CTRL_2);
 
 	return 0;
 }
-- 
2.0.4

Re: [PATCH 8/9] net: phy: micrel: clean up led-mode setup

[Sergei Shtylyov]

Hello.

On 11/11/2014 10:00 PM, Johan Hovold wrote:

> Clean up led-mode setup by introducing proper defines for PHY Control
> registers 1 and 2 and only passing the register to the setup function.

    Not sure that's really better that it was before (modulo naming).

> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>   drivers/net/phy/micrel.c | 31 ++++++++++++++++++++++---------
>   1 file changed, 22 insertions(+), 9 deletions(-)

> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index ec9ce35e934b..12e18f7273ce 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -47,8 +47,12 @@
>   #define	KSZPHY_INTCS_ALL			(KSZPHY_INTCS_LINK_UP |\
>   						KSZPHY_INTCS_LINK_DOWN)
>
> -/* general PHY control reg in vendor specific block. */
> -#define	MII_KSZPHY_CTRL			0x1F
> +/* PHY Control 1 */
> +#define	MII_KSZPHY_CTRL_1			0x1e
> +
> +/* PHY Control 2 / PHY Control (if no PHY Control 1) */
> +#define	MII_KSZPHY_CTRL_2			0x1f
> +#define	MII_KSZPHY_CTRL				MII_KSZPHY_CTRL_2
>   /* bitmap of PHY register to set interrupt mode */
>   #define KSZPHY_CTRL_INT_ACTIVE_HIGH		BIT(9)
>   #define KSZ9021_CTRL_INT_ACTIVE_HIGH		BIT(14)
> @@ -158,13 +162,12 @@ static int ks8737_config_intr(struct phy_device *phydev)
>   	return rc < 0 ? rc : 0;
>   }
>
> -static int kszphy_setup_led(struct phy_device *phydev,
> -			    unsigned int reg, unsigned int shift)
> +static int kszphy_setup_led(struct phy_device *phydev, u32 reg)
>   {
>
>   	struct device *dev = &phydev->dev;
>   	struct device_node *of_node = dev->of_node;
> -	int rc, temp;
> +	int rc, temp, shift;
>   	u32 val;
>
>   	if (!of_node && dev->parent->of_node)
> @@ -178,6 +181,17 @@ static int kszphy_setup_led(struct phy_device *phydev,
>   		return -EINVAL;
>   	}
>
> +	switch (reg) {
> +	case MII_KSZPHY_CTRL_1:
> +		shift = 14;
> +		break;
> +	case MII_KSZPHY_CTRL_2:
> +		shift = 4;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
>   	temp = phy_read(phydev, reg);
>   	if (temp < 0) {
>   		rc = temp;
> @@ -220,15 +234,14 @@ static int kszphy_config_init(struct phy_device *phydev)
>
>   static int kszphy_config_init_led8041(struct phy_device *phydev)
>   {
> -	/* single led control, register 0x1e bits 15..14 */
> -	return kszphy_setup_led(phydev, 0x1e, 14);
> +	return kszphy_setup_led(phydev, MII_KSZPHY_CTRL_1);
>   }
>
>   static int ksz8021_config_init(struct phy_device *phydev)
>   {
>   	int rc;
>
> -	kszphy_setup_led(phydev, 0x1f, 4);
> +	kszphy_setup_led(phydev, MII_KSZPHY_CTRL_2);
>
>   	rc = ksz_config_flags(phydev);
>   	if (rc < 0)
> @@ -243,7 +256,7 @@ static int ks8051_config_init(struct phy_device *phydev)
>   {
>   	int rc;
>
> -	kszphy_setup_led(phydev, 0x1f, 4);
> +	kszphy_setup_led(phydev, MII_KSZPHY_CTRL_2);
>
>   	rc = ksz_config_flags(phydev);
>   	return rc < 0 ? rc : 0;

WBR, Sergei

Re: [PATCH 8/9] net: phy: micrel: clean up led-mode setup

[Florian Fainelli]

On 11/11/2014 01:41 PM, Sergei Shtylyov wrote:
> Hello.
> 
> On 11/11/2014 10:00 PM, Johan Hovold wrote:
> 
>> Clean up led-mode setup by introducing proper defines for PHY Control
>> registers 1 and 2 and only passing the register to the setup function.
> 
>    Not sure that's really better that it was before (modulo naming).

We do have proper error handling in kszphy_setup_led() which is already
an improvement.

> 
>> Signed-off-by: Johan Hovold <johan@kernel.org>
>> ---
>>   drivers/net/phy/micrel.c | 31 ++++++++++++++++++++++---------
>>   1 file changed, 22 insertions(+), 9 deletions(-)
> 
>> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
>> index ec9ce35e934b..12e18f7273ce 100644
>> --- a/drivers/net/phy/micrel.c
>> +++ b/drivers/net/phy/micrel.c
>> @@ -47,8 +47,12 @@
>>   #define    KSZPHY_INTCS_ALL            (KSZPHY_INTCS_LINK_UP |\
>>                           KSZPHY_INTCS_LINK_DOWN)
>>
>> -/* general PHY control reg in vendor specific block. */
>> -#define    MII_KSZPHY_CTRL            0x1F
>> +/* PHY Control 1 */
>> +#define    MII_KSZPHY_CTRL_1            0x1e
>> +
>> +/* PHY Control 2 / PHY Control (if no PHY Control 1) */
>> +#define    MII_KSZPHY_CTRL_2            0x1f
>> +#define    MII_KSZPHY_CTRL                MII_KSZPHY_CTRL_2
>>   /* bitmap of PHY register to set interrupt mode */
>>   #define KSZPHY_CTRL_INT_ACTIVE_HIGH        BIT(9)
>>   #define KSZ9021_CTRL_INT_ACTIVE_HIGH        BIT(14)
>> @@ -158,13 +162,12 @@ static int ks8737_config_intr(struct phy_device
>> *phydev)
>>       return rc < 0 ? rc : 0;
>>   }
>>
>> -static int kszphy_setup_led(struct phy_device *phydev,
>> -                unsigned int reg, unsigned int shift)
>> +static int kszphy_setup_led(struct phy_device *phydev, u32 reg)
>>   {
>>
>>       struct device *dev = &phydev->dev;
>>       struct device_node *of_node = dev->of_node;
>> -    int rc, temp;
>> +    int rc, temp, shift;
>>       u32 val;
>>
>>       if (!of_node && dev->parent->of_node)
>> @@ -178,6 +181,17 @@ static int kszphy_setup_led(struct phy_device
>> *phydev,
>>           return -EINVAL;
>>       }
>>
>> +    switch (reg) {
>> +    case MII_KSZPHY_CTRL_1:
>> +        shift = 14;
>> +        break;
>> +    case MII_KSZPHY_CTRL_2:
>> +        shift = 4;
>> +        break;
>> +    default:
>> +        return -EINVAL;
>> +    }
>> +
>>       temp = phy_read(phydev, reg);
>>       if (temp < 0) {
>>           rc = temp;
>> @@ -220,15 +234,14 @@ static int kszphy_config_init(struct phy_device
>> *phydev)
>>
>>   static int kszphy_config_init_led8041(struct phy_device *phydev)
>>   {
>> -    /* single led control, register 0x1e bits 15..14 */
>> -    return kszphy_setup_led(phydev, 0x1e, 14);
>> +    return kszphy_setup_led(phydev, MII_KSZPHY_CTRL_1);
>>   }
>>
>>   static int ksz8021_config_init(struct phy_device *phydev)
>>   {
>>       int rc;
>>
>> -    kszphy_setup_led(phydev, 0x1f, 4);
>> +    kszphy_setup_led(phydev, MII_KSZPHY_CTRL_2);
>>
>>       rc = ksz_config_flags(phydev);
>>       if (rc < 0)
>> @@ -243,7 +256,7 @@ static int ks8051_config_init(struct phy_device
>> *phydev)
>>   {
>>       int rc;
>>
>> -    kszphy_setup_led(phydev, 0x1f, 4);
>> +    kszphy_setup_led(phydev, MII_KSZPHY_CTRL_2);
>>
>>       rc = ksz_config_flags(phydev);
>>       return rc < 0 ? rc : 0;
> 
> WBR, Sergei
> 

Re: [PATCH 8/9] net: phy: micrel: clean up led-mode setup

[Johan Hovold]

On Tue, Nov 11, 2014 at 01:43:06PM -0800, Florian Fainelli wrote:
> On 11/11/2014 01:41 PM, Sergei Shtylyov wrote:
> > Hello.
> > 
> > On 11/11/2014 10:00 PM, Johan Hovold wrote:
> > 
> >> Clean up led-mode setup by introducing proper defines for PHY Control
> >> registers 1 and 2 and only passing the register to the setup function.
> > 
> >    Not sure that's really better that it was before (modulo naming).
> 
> We do have proper error handling in kszphy_setup_led() which is already
> an improvement.

This also means handling the led mode in one place rather than spreading
it all over the driver, with multiple config functions providing one of
the same two combinations of register and shift.

It's usefulness will perhaps become more apparent if you look at the
follow up patches, which does further refactoring and store the led-mode
register in the type data.

Johan

Re: [PATCH 0/9] net: phy: micrel: refactoring and KSZ8081/KSZ8091 features

[David Miller]

From: Johan Hovold <johan@kernel.org>
Date: Tue, 11 Nov 2014 20:00:06 +0100

> This series cleans up and refactors parts of the micrel PHY driver, and
> adds support for broadcast-address-disable and led-mode configuration
> for KSZ8081 and KSZ8091 PHYs.
> 
> Specifically, this enables dual KSZ8081 setups (which are limited to
> using address 0 and 3).
> 
> A follow up series will add device-type abstraction which will allow for
> further refactoring and shared initialisation code.

Series applied, thanks Johan.