[PATCH v2 0/5] net: phy: dp83848: Add support for TI TLK10x Ethernet PHYs

[PATCH v2 0/5] net: phy: dp83848: Add support for TI TLK10x Ethernet PHYs

[Andrew F. Davis]

Hello all,

This series is [0] split into its logical components.

Thanks,
Andrew

[0] http://www.spinics.net/lists/netdev/msg363106.html

Andrew F. Davis (5):
  net: phy: dp83848: Add macro for dp83848 compatible devices
  net: phy: dp83848: Add PHY ID for TI version of DP83848C
  net: phy: dp83848: Reorganize code for readability and safety
  net: phy: dp83848: Add support for TI TLK10x Ethernet PHYs
  net: phy: dp83848: Add comments for register definitions

 drivers/net/phy/dp83848.c | 88 ++++++++++++++++++++++++++++-------------------
 1 file changed, 52 insertions(+), 36 deletions(-)

-- 
2.7.0

[PATCH v2 1/5] net: phy: dp83848: Add macro for dp83848 compatible devices

[Andrew F. Davis]

Add a helper macro for defining dp83848 compatible phy devices.
Update copyright info.

Signed-off-by: Andrew F. Davis <afd@ti.com>
---
 drivers/net/phy/dp83848.c | 41 ++++++++++++++++++++++-------------------
 1 file changed, 22 insertions(+), 19 deletions(-)

diff --git a/drivers/net/phy/dp83848.c b/drivers/net/phy/dp83848.c
index 5e14e62..4e78f54 100644
--- a/drivers/net/phy/dp83848.c
+++ b/drivers/net/phy/dp83848.c
@@ -1,7 +1,7 @@
 /*
  * Driver for the Texas Instruments DP83848 PHY
  *
- * Copyright (C) 2015 Texas Instruments Inc.
+ * Copyright (C) 2015-2016 Texas Instruments Incorporated - http://www.ti.com/
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -70,25 +70,28 @@ static struct mdio_device_id __maybe_unused dp83848_tbl[] = {
 };
 MODULE_DEVICE_TABLE(mdio, dp83848_tbl);
 
-static struct phy_driver dp83848_driver[] = {
-	{
-		.phy_id		= DP83848_PHY_ID,
-		.phy_id_mask	= 0xfffffff0,
-		.name		= "TI DP83848",
-		.features	= PHY_BASIC_FEATURES,
-		.flags		= PHY_HAS_INTERRUPT,
-
-		.soft_reset	= genphy_soft_reset,
-		.config_init	= genphy_config_init,
-		.suspend	= genphy_suspend,
-		.resume		= genphy_resume,
-		.config_aneg	= genphy_config_aneg,
-		.read_status	= genphy_read_status,
+#define DP83848_PHY_DRIVER(_id, _name)				\
+	{							\
+		.phy_id		= _id,				\
+		.phy_id_mask	= 0xfffffff0,			\
+		.name		= _name,			\
+		.features	= PHY_BASIC_FEATURES,		\
+		.flags		= PHY_HAS_INTERRUPT,		\
+								\
+		.soft_reset	= genphy_soft_reset,		\
+		.config_init	= genphy_config_init,		\
+		.suspend	= genphy_suspend,		\
+		.resume		= genphy_resume,		\
+		.config_aneg	= genphy_config_aneg,		\
+		.read_status	= genphy_read_status,		\
+								\
+		/* IRQ related */				\
+		.ack_interrupt	= dp83848_ack_interrupt,	\
+		.config_intr	= dp83848_config_intr,		\
+	}
 
-		/* IRQ related */
-		.ack_interrupt	= dp83848_ack_interrupt,
-		.config_intr	= dp83848_config_intr,
-	},
+static struct phy_driver dp83848_driver[] = {
+	DP83848_PHY_DRIVER(DP83848_PHY_ID, "TI DP83848 10/100 Mbps PHY"),
 };
 module_phy_driver(dp83848_driver);
 
-- 
2.7.0

[PATCH v2 2/5] net: phy: dp83848: Add PHY ID for TI version of DP83848C

[Andrew F. Davis]

After acquiring National Semiconductor, TI appears to have
changed the Vendor Model Number for the DP83848C PHYs,
add this new ID to supported IDs.

Signed-off-by: Andrew F. Davis <afd@ti.com>
---
 drivers/net/phy/dp83848.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/dp83848.c b/drivers/net/phy/dp83848.c
index 4e78f54..d4686d5f 100644
--- a/drivers/net/phy/dp83848.c
+++ b/drivers/net/phy/dp83848.c
@@ -16,7 +16,8 @@
 #include <linux/module.h>
 #include <linux/phy.h>
 
-#define DP83848_PHY_ID			0x20005c90
+#define TI_DP83848C_PHY_ID		0x20005ca0
+#define NS_DP83848C_PHY_ID		0x20005c90
 
 /* Registers */
 #define DP83848_MICR			0x11
@@ -65,7 +66,8 @@ static int dp83848_config_intr(struct phy_device *phydev)
 }
 
 static struct mdio_device_id __maybe_unused dp83848_tbl[] = {
-	{ DP83848_PHY_ID, 0xfffffff0 },
+	{ TI_DP83848C_PHY_ID, 0xfffffff0 },
+	{ NS_DP83848C_PHY_ID, 0xfffffff0 },
 	{ }
 };
 MODULE_DEVICE_TABLE(mdio, dp83848_tbl);
@@ -91,7 +93,8 @@ MODULE_DEVICE_TABLE(mdio, dp83848_tbl);
 	}
 
 static struct phy_driver dp83848_driver[] = {
-	DP83848_PHY_DRIVER(DP83848_PHY_ID, "TI DP83848 10/100 Mbps PHY"),
+	DP83848_PHY_DRIVER(TI_DP83848C_PHY_ID, "TI DP83848C 10/100 Mbps PHY"),
+	DP83848_PHY_DRIVER(NS_DP83848C_PHY_ID, "TI DP83848C 10/100 Mbps PHY"),
 };
 module_phy_driver(dp83848_driver);
 
-- 
2.7.0

[PATCH v2 3/5] net: phy: dp83848: Reorganize code for readability and safety

[Andrew F. Davis]

Reorganize code by moving the desired interrupt mask definition
out of function. Also rearrange the enable/disable interrupt function
to prevent accidental over-writing of values in registers.

Signed-off-by: Andrew F. Davis <afd@ti.com>
---
 drivers/net/phy/dp83848.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/net/phy/dp83848.c b/drivers/net/phy/dp83848.c
index d4686d5f..20d3b9d 100644
--- a/drivers/net/phy/dp83848.c
+++ b/drivers/net/phy/dp83848.c
@@ -37,6 +37,12 @@
 #define DP83848_MISR_ED_INT_EN		BIT(6) /* Energy detect */
 #define DP83848_MISR_LQM_INT_EN		BIT(7) /* Link Quality Monitor */
 
+#define DP83848_INT_EN_MASK		\
+	(DP83848_MISR_ANC_INT_EN |	\
+	 DP83848_MISR_DUP_INT_EN |	\
+	 DP83848_MISR_SPD_INT_EN |	\
+	 DP83848_MISR_LINK_INT_EN)
+
 static int dp83848_ack_interrupt(struct phy_device *phydev)
 {
 	int err = phy_read(phydev, DP83848_MISR);
@@ -46,23 +52,24 @@ static int dp83848_ack_interrupt(struct phy_device *phydev)
 
 static int dp83848_config_intr(struct phy_device *phydev)
 {
-	int err;
+	int control, ret;
+
+	control = phy_read(phydev, DP83848_MICR);
+	if (control < 0)
+		return control;
 
 	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
-		err = phy_write(phydev, DP83848_MICR,
-				DP83848_MICR_INT_OE |
-				DP83848_MICR_INTEN);
-		if (err < 0)
-			return err;
-
-		return phy_write(phydev, DP83848_MISR,
-				 DP83848_MISR_ANC_INT_EN |
-				 DP83848_MISR_DUP_INT_EN |
-				 DP83848_MISR_SPD_INT_EN |
-				 DP83848_MISR_LINK_INT_EN);
+		control |= DP83848_MICR_INT_OE;
+		control |= DP83848_MICR_INTEN;
+
+		ret = phy_write(phydev, DP83848_MISR, DP83848_INT_EN_MASK);
+		if (ret < 0)
+			return ret;
+	} else {
+		control &= ~DP83848_MICR_INTEN;
 	}
 
-	return phy_write(phydev, DP83848_MICR, 0x0);
+	return phy_write(phydev, DP83848_MICR, control);
 }
 
 static struct mdio_device_id __maybe_unused dp83848_tbl[] = {
-- 
2.7.0

[PATCH v2 4/5] net: phy: dp83848: Add support for TI TLK10x Ethernet PHYs

[Andrew F. Davis]

The TI TLK10x Ethernet PHYs are similar in the interrupt relevant
registers and so are compatible with the DP83848x devices already
supported.

Signed-off-by: Andrew F. Davis <afd@ti.com>
---
 drivers/net/phy/dp83848.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/phy/dp83848.c b/drivers/net/phy/dp83848.c
index 20d3b9d..f897989 100644
--- a/drivers/net/phy/dp83848.c
+++ b/drivers/net/phy/dp83848.c
@@ -18,6 +18,7 @@
 
 #define TI_DP83848C_PHY_ID		0x20005ca0
 #define NS_DP83848C_PHY_ID		0x20005c90
+#define TLK10X_PHY_ID			0x2000a210
 
 /* Registers */
 #define DP83848_MICR			0x11
@@ -75,6 +76,7 @@ static int dp83848_config_intr(struct phy_device *phydev)
 static struct mdio_device_id __maybe_unused dp83848_tbl[] = {
 	{ TI_DP83848C_PHY_ID, 0xfffffff0 },
 	{ NS_DP83848C_PHY_ID, 0xfffffff0 },
+	{ TLK10X_PHY_ID, 0xfffffff0 },
 	{ }
 };
 MODULE_DEVICE_TABLE(mdio, dp83848_tbl);
@@ -102,6 +104,7 @@ MODULE_DEVICE_TABLE(mdio, dp83848_tbl);
 static struct phy_driver dp83848_driver[] = {
 	DP83848_PHY_DRIVER(TI_DP83848C_PHY_ID, "TI DP83848C 10/100 Mbps PHY"),
 	DP83848_PHY_DRIVER(NS_DP83848C_PHY_ID, "TI DP83848C 10/100 Mbps PHY"),
+	DP83848_PHY_DRIVER(TLK10X_PHY_ID, "TI TLK10X 10/100 Mbps PHY"),
 };
 module_phy_driver(dp83848_driver);
 
-- 
2.7.0

[PATCH v2 5/5] net: phy: dp83848: Add comments for register definitions

[Andrew F. Davis]

Signed-off-by: Andrew F. Davis <afd@ti.com>
---
 drivers/net/phy/dp83848.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/dp83848.c b/drivers/net/phy/dp83848.c
index f897989..556904f 100644
--- a/drivers/net/phy/dp83848.c
+++ b/drivers/net/phy/dp83848.c
@@ -21,8 +21,8 @@
 #define TLK10X_PHY_ID			0x2000a210
 
 /* Registers */
-#define DP83848_MICR			0x11
-#define DP83848_MISR			0x12
+#define DP83848_MICR			0x11 /* MII Interrupt Control Register */
+#define DP83848_MISR			0x12 /* MII Interrupt Status Register */
 
 /* MICR Register Fields */
 #define DP83848_MICR_INT_OE		BIT(0) /* Interrupt Output Enable */
-- 
2.7.0

Re: [PATCH v2 3/5] net: phy: dp83848: Reorganize code for readability and safety

[Sergei Shtylyov]

Hello.

On 02/07/2016 08:47 PM, Andrew F. Davis wrote:

> Reorganize code by moving the desired interrupt mask definition
> out of function. Also rearrange the enable/disable interrupt function
> to prevent accidental over-writing of values in registers.
>
> Signed-off-by: Andrew F. Davis <afd@ti.com>
> ---
>   drivers/net/phy/dp83848.c | 33 ++++++++++++++++++++-------------
>   1 file changed, 20 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/phy/dp83848.c b/drivers/net/phy/dp83848.c
> index d4686d5f..20d3b9d 100644
> --- a/drivers/net/phy/dp83848.c
> +++ b/drivers/net/phy/dp83848.c
[...]
> @@ -46,23 +52,24 @@ static int dp83848_ack_interrupt(struct phy_device *phydev)
>
>   static int dp83848_config_intr(struct phy_device *phydev)
>   {
> -	int err;
> +	int control, ret;
> +
> +	control = phy_read(phydev, DP83848_MICR);
> +	if (control < 0)
> +		return control;
>
>   	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> -		err = phy_write(phydev, DP83848_MICR,
> -				DP83848_MICR_INT_OE |
> -				DP83848_MICR_INTEN);
> -		if (err < 0)
> -			return err;
> -
> -		return phy_write(phydev, DP83848_MISR,
> -				 DP83848_MISR_ANC_INT_EN |
> -				 DP83848_MISR_DUP_INT_EN |
> -				 DP83848_MISR_SPD_INT_EN |
> -				 DP83848_MISR_LINK_INT_EN);
> +		control |= DP83848_MICR_INT_OE;
> +		control |= DP83848_MICR_INTEN;

    I'd have written this on a single line.

[...]

MBR, Sergei

Re: [PATCH v2 0/5] net: phy: dp83848: Add support for TI TLK10x Ethernet PHYs

[Florian Fainelli]

On 07/02/2016 09:47, Andrew F. Davis wrote:
> Hello all,
> 
> This series is [0] split into its logical components.
> 
> Thanks,
> Andrew
> 
> [0] http://www.spinics.net/lists/netdev/msg363106.html

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

Thanks for splitting things up!
--
Florian

Re: [PATCH v2 0/5] net: phy: dp83848: Add support for TI TLK10x Ethernet PHYs

[David Miller]

From: "Andrew F. Davis" <afd@ti.com>
Date: Sun, 7 Feb 2016 11:47:16 -0600

> This series is [0] split into its logical components.

Series applied, thanks.

Re: [PATCH v2 2/5] net: phy: dp83848: Add PHY ID for TI version of DP83848C

[Andrew F. Davis]

On 02/07/2016 11:47 AM, Andrew F. Davis wrote:
> After acquiring National Semiconductor, TI appears to have
> changed the Vendor Model Number for the DP83848C PHYs,
> add this new ID to supported IDs.
>
> Signed-off-by: Andrew F. Davis <afd@ti.com>
> ---
>   drivers/net/phy/dp83848.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/phy/dp83848.c b/drivers/net/phy/dp83848.c
> index 4e78f54..d4686d5f 100644
> --- a/drivers/net/phy/dp83848.c
> +++ b/drivers/net/phy/dp83848.c
> @@ -16,7 +16,8 @@
>   #include <linux/module.h>
>   #include <linux/phy.h>
>
> -#define DP83848_PHY_ID			0x20005c90
> +#define TI_DP83848C_PHY_ID		0x20005ca0
> +#define NS_DP83848C_PHY_ID		0x20005c90
>
>   /* Registers */
>   #define DP83848_MICR			0x11
> @@ -65,7 +66,8 @@ static int dp83848_config_intr(struct phy_device *phydev)
>   }
>
>   static struct mdio_device_id __maybe_unused dp83848_tbl[] = {
> -	{ DP83848_PHY_ID, 0xfffffff0 },
> +	{ TI_DP83848C_PHY_ID, 0xfffffff0 },
> +	{ NS_DP83848C_PHY_ID, 0xfffffff0 },
>   	{ }
>   };
>   MODULE_DEVICE_TABLE(mdio, dp83848_tbl);
> @@ -91,7 +93,8 @@ MODULE_DEVICE_TABLE(mdio, dp83848_tbl);
>   	}
>
>   static struct phy_driver dp83848_driver[] = {
> -	DP83848_PHY_DRIVER(DP83848_PHY_ID, "TI DP83848 10/100 Mbps PHY"),
> +	DP83848_PHY_DRIVER(TI_DP83848C_PHY_ID, "TI DP83848C 10/100 Mbps PHY"),
> +	DP83848_PHY_DRIVER(NS_DP83848C_PHY_ID, "TI DP83848C 10/100 Mbps PHY"),

This seems to be causing a warning about duplicate file names (driver name in
sysfs), so the bottom one can probably s/TI/NS, can this be changed in-tree
before the merge or should I submit a patch?

Andrew

>   };
>   module_phy_driver(dp83848_driver);
>
>

Re: [PATCH v2 2/5] net: phy: dp83848: Add PHY ID for TI version of DP83848C

[Florian Fainelli]

On 17/02/2016 15:37, Andrew F. Davis wrote:
> On 02/07/2016 11:47 AM, Andrew F. Davis wrote:
>> After acquiring National Semiconductor, TI appears to have
>> changed the Vendor Model Number for the DP83848C PHYs,
>> add this new ID to supported IDs.
>>
>> Signed-off-by: Andrew F. Davis <afd@ti.com>
>> ---
>>   drivers/net/phy/dp83848.c | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/phy/dp83848.c b/drivers/net/phy/dp83848.c
>> index 4e78f54..d4686d5f 100644
>> --- a/drivers/net/phy/dp83848.c
>> +++ b/drivers/net/phy/dp83848.c
>> @@ -16,7 +16,8 @@
>>   #include <linux/module.h>
>>   #include <linux/phy.h>
>>
>> -#define DP83848_PHY_ID            0x20005c90
>> +#define TI_DP83848C_PHY_ID        0x20005ca0
>> +#define NS_DP83848C_PHY_ID        0x20005c90
>>
>>   /* Registers */
>>   #define DP83848_MICR            0x11
>> @@ -65,7 +66,8 @@ static int dp83848_config_intr(struct phy_device
>> *phydev)
>>   }
>>
>>   static struct mdio_device_id __maybe_unused dp83848_tbl[] = {
>> -    { DP83848_PHY_ID, 0xfffffff0 },
>> +    { TI_DP83848C_PHY_ID, 0xfffffff0 },
>> +    { NS_DP83848C_PHY_ID, 0xfffffff0 },
>>       { }
>>   };
>>   MODULE_DEVICE_TABLE(mdio, dp83848_tbl);
>> @@ -91,7 +93,8 @@ MODULE_DEVICE_TABLE(mdio, dp83848_tbl);
>>       }
>>
>>   static struct phy_driver dp83848_driver[] = {
>> -    DP83848_PHY_DRIVER(DP83848_PHY_ID, "TI DP83848 10/100 Mbps PHY"),
>> +    DP83848_PHY_DRIVER(TI_DP83848C_PHY_ID, "TI DP83848C 10/100 Mbps
>> PHY"),
>> +    DP83848_PHY_DRIVER(NS_DP83848C_PHY_ID, "TI DP83848C 10/100 Mbps
>> PHY"),
> 
> This seems to be causing a warning about duplicate file names (driver
> name in
> sysfs), so the bottom one can probably s/TI/NS, can this be changed in-tree
> before the merge or should I submit a patch?

Once the patches are merged by David in his tree, you will need to
provide an incremental patch to fix the problem. I had not noticed the
duplicate name either here, but it sounds like you should indeed fix it.

Thanks!
--
Florian