net: phy: realtek: add rtl8201f driver

net: phy: realtek: add rtl8201f driver

[Jongsung Kim]

This patch adds the minimal driver to manage the
Realtek RTL8201F 10/100Mbps Transceivers.

Signed-off-by: Jongsung Kim <neidhard.kim@lge.com>
---
 drivers/net/phy/realtek.c |   60 +++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 53 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 8e7af83..27847ca 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -16,9 +16,13 @@
 #include <linux/phy.h>
 #include <linux/module.h>
 
-#define RTL821x_PHYSR		0x11
-#define RTL821x_PHYSR_DUPLEX	0x2000
-#define RTL821x_PHYSR_SPEED	0xc000
+/* page 0 register 30 - interrupt indicators and SNR display register */
+#define RTL8201F_ISR		0x1e
+/* page 0 register 31 - page select register */
+#define RTL8201F_PSR		0x1f
+/* page 7 register 19 - interrupt, WOL enable, and LEDs function register */
+#define RTL8201F_IER		0x13
+
 #define RTL821x_INER		0x12
 #define RTL821x_INER_INIT	0x6400
 #define RTL821x_INSR		0x13
@@ -29,6 +33,15 @@ MODULE_DESCRIPTION("Realtek PHY driver");
 MODULE_AUTHOR("Johnson Leung");
 MODULE_LICENSE("GPL");
 
+static int rtl8201f_ack_interrupt(struct phy_device *phydev)
+{
+	int err;
+
+	err = phy_read(phydev, RTL8201F_ISR);
+
+	return (err < 0) ? err : 0;
+}
+
 static int rtl821x_ack_interrupt(struct phy_device *phydev)
 {
 	int err;
@@ -38,6 +51,24 @@ static int rtl821x_ack_interrupt(struct phy_device *phydev)
 	return (err < 0) ? err : 0;
 }
 
+static int rtl8201f_config_intr(struct phy_device *phydev)
+{
+	int err;
+
+	phy_write(phydev, RTL8201F_PSR, 0x0007);	/* select page 7 */
+
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+		err = phy_write(phydev, RTL8201F_IER, 0x3800 |
+		                phy_read(phydev, RTL8201F_IER));
+	else
+		err = phy_write(phydev, RTL8201F_IER, ~0x3800 &
+		                phy_read(phydev, RTL8201F_IER));
+
+	phy_write(phydev, RTL8201F_PSR, 0x0000);	/* back to page 0 */
+
+	return err;
+}
+
 static int rtl8211b_config_intr(struct phy_device *phydev)
 {
 	int err;
@@ -64,6 +95,20 @@ static int rtl8211e_config_intr(struct phy_device *phydev)
 	return err;
 }
 
+/* RTL8201F */
+static struct phy_driver rtl8201f_driver = {
+	.phy_id		= 0x001cc816,
+	.name		= "RTL8201F 10/100Mbps Ethernet",
+	.phy_id_mask	= 0x001fffff,
+	.features	= PHY_BASIC_FEATURES,
+	.flags		= PHY_HAS_INTERRUPT,
+	.config_aneg	= &genphy_config_aneg,
+	.read_status	= &genphy_read_status,
+	.ack_interrupt	= &rtl8201f_ack_interrupt,
+	.config_intr	= &rtl8201f_config_intr,
+	.driver		= { .owner = THIS_MODULE,},
+};
+
 /* RTL8211B */
 static struct phy_driver rtl8211b_driver = {
 	.phy_id		= 0x001cc912,
@@ -96,16 +141,16 @@ static struct phy_driver rtl8211e_driver = {
 
 static int __init realtek_init(void)
 {
-	int ret;
-
-	ret = phy_driver_register(&rtl8211b_driver);
-	if (ret < 0)
+	if(phy_driver_register(&rtl8201f_driver) < 0)
+		return -ENODEV;
+	if(phy_driver_register(&rtl8211b_driver) < 0)
 		return -ENODEV;
 	return phy_driver_register(&rtl8211e_driver);
 }
 
 static void __exit realtek_exit(void)
 {
+	phy_driver_unregister(&rtl8201f_driver);
 	phy_driver_unregister(&rtl8211b_driver);
 	phy_driver_unregister(&rtl8211e_driver);
 }
@@ -114,6 +159,7 @@ module_init(realtek_init);
 module_exit(realtek_exit);
 
 static struct mdio_device_id __maybe_unused realtek_tbl[] = {
+	{ 0x001cc816, 0x001fffff },
 	{ 0x001cc912, 0x001fffff },
 	{ 0x001cc915, 0x001fffff },
 	{ }

Re: net: phy: realtek: add rtl8201f driver

[Francois Romieu]

Jongsung Kim <neidhard.kim@lge.com> :
> This patch adds the minimal driver to manage the
> Realtek RTL8201F 10/100Mbps Transceivers.

Your patch contains both "remove unused #define" and "support new hardware"
parts. I am not sure that the former is adequate for submission until
net-next opens.

> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
[...]
> +static int rtl8201f_config_intr(struct phy_device *phydev)
> +{
> +	int err;
> +
> +	phy_write(phydev, RTL8201F_PSR, 0x0007);	/* select page 7 */

static void rtl8201f_page_select(struct phy_device *phydev, int page) ?

[...]
> @@ -96,16 +141,16 @@ static struct phy_driver rtl8211e_driver = {
>  
>  static int __init realtek_init(void)
>  {
> -	int ret;
> -
> -	ret = phy_driver_register(&rtl8211b_driver);
> -	if (ret < 0)
> +	if(phy_driver_register(&rtl8201f_driver) < 0)
         ^^ -> missing space.

> +		return -ENODEV;

Please propagate phy_driver_register error code.

> +	if(phy_driver_register(&rtl8211b_driver) < 0)
>  		return -ENODEV;
>  	return phy_driver_register(&rtl8211e_driver);

Unbalanced error paths.

>  }
>  
>  static void __exit realtek_exit(void)
>  {
> +	phy_driver_unregister(&rtl8201f_driver);
>  	phy_driver_unregister(&rtl8211b_driver);
>  	phy_driver_unregister(&rtl8211e_driver);

Code duplication.

You may use an array of phy_driver for realtek_{init/exit}

-- 
Ueimor

RE: net: phy: realtek: add rtl8201f driver

[Jongsung Kim]

Francois Romieu <romieu@fr.zoreil.com> :
> Your patch contains both "remove unused #define" and "support new
hardware"
> parts. I am not sure that the former is adequate for submission until
net-next opens.

I see. Sorry for trying touching them even without comment. I won't touch
them.

> static void rtl8201f_page_select(struct phy_device *phydev, int page) ?

Okay. Looks better..

>> +	if(phy_driver_register(&rtl8201f_driver) < 0)
>         ^^ -> missing space.

What a shame!

> You may use an array of phy_driver for realtek_{init/exit}

Agreed.

--
Ueimor

Re: net: phy: realtek: add rtl8201f driver

[Sergei Shtylyov]

Hello.

On 08-05-2013 14:10, Jongsung Kim wrote:

> This patch adds the minimal driver to manage the
> Realtek RTL8201F 10/100Mbps Transceivers.

> Signed-off-by: Jongsung Kim <neidhard.kim@lge.com>
> ---
>   drivers/net/phy/realtek.c |   60 +++++++++++++++++++++++++++++++++++++++-----
>   1 files changed, 53 insertions(+), 7 deletions(-)

> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index 8e7af83..27847ca 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -16,9 +16,13 @@
>   #include <linux/phy.h>
>   #include <linux/module.h>
>
> -#define RTL821x_PHYSR		0x11
> -#define RTL821x_PHYSR_DUPLEX	0x2000
> -#define RTL821x_PHYSR_SPEED	0xc000

     Removal of unused #define's is a matter of a separate cleanup patch...

> +/* page 0 register 30 - interrupt indicators and SNR display register */
> +#define RTL8201F_ISR		0x1e
> +/* page 0 register 31 - page select register */
> +#define RTL8201F_PSR		0x1f
> +/* page 7 register 19 - interrupt, WOL enable, and LEDs function register */
> +#define RTL8201F_IER		0x13
> +
>   #define RTL821x_INER		0x12
>   #define RTL821x_INER_INIT	0x6400
>   #define RTL821x_INSR		0x13
> @@ -29,6 +33,15 @@ MODULE_DESCRIPTION("Realtek PHY driver");
>   MODULE_AUTHOR("Johnson Leung");
>   MODULE_LICENSE("GPL");
>
> +static int rtl8201f_ack_interrupt(struct phy_device *phydev)
> +{
> +	int err;
> +
> +	err = phy_read(phydev, RTL8201F_ISR);

    This could be an initializer and so make the function shorter.

> +
> +	return (err < 0) ? err : 0;
> +}
> +
>   static int rtl821x_ack_interrupt(struct phy_device *phydev)
>   {
>   	int err;
[...]
> @@ -96,16 +141,16 @@ static struct phy_driver rtl8211e_driver = {
>
>   static int __init realtek_init(void)
>   {
> -	int ret;
> -
> -	ret = phy_driver_register(&rtl8211b_driver);
> -	if (ret < 0)
> +	if(phy_driver_register(&rtl8201f_driver) < 0)
> +		return -ENODEV;
> +	if(phy_driver_register(&rtl8211b_driver) < 0)

     You haven't run this patch thru scripts/checkpatch.pl -- there 
should be a space between *if* and (.

WBR, Sergei

RE: net: phy: realtek: add rtl8201f driver

[Jongsung Kim]

Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> :
>     Removal of unused #define's is a matter of a separate cleanup patch...

Sorry. I won't touch them.

>> +static int rtl8201f_ack_interrupt(struct phy_device *phydev) {
>> +	int err;
>> +
>> +	err = phy_read(phydev, RTL8201F_ISR);
>
>    This could be an initializer and so make the function shorter.
>

Agreed. I just thought it's better to make it similar to the
rtl821x_ack_interrupt. Then, may I make shorter the rtl821x_ack_interrupt as
well as rtl8201f_ack_interrupt?

>
>     You haven't run this patch thru scripts/checkpatch.pl -- there should
be a space between *if* and (.
>

Sorry.. what a mistake..

Re: net: phy: realtek: add rtl8201f driver

[Sergei Shtylyov]

Hello.

On 09-05-2013 6:35, Jongsung Kim wrote:

>>      Removal of unused #define's is a matter of a separate cleanup patch...

> Sorry. I won't touch them.

>>> +static int rtl8201f_ack_interrupt(struct phy_device *phydev) {
>>> +	int err;
>>> +
>>> +	err = phy_read(phydev, RTL8201F_ISR);

>>     This could be an initializer and so make the function shorter.

> Agreed. I just thought it's better to make it similar to the
> rtl821x_ack_interrupt.

    Ah, then you may leave this code as is.

> Then, may I make shorter the rtl821x_ack_interrupt as
> well as rtl8201f_ack_interrupt?

    In a separate patch, if you wish.

WBR, Sergei

RE: net: phy: realtek: add rtl8201f driver

[Jongsung Kim]

Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> :
>> Agreed. I just thought it's better to make it similar to the 
>> rtl821x_ack_interrupt.
>
>    Ah, then you may leave this code as is.
>
>> Then, may I make shorter the rtl821x_ack_interrupt as well as 
>> rtl8201f_ack_interrupt?
>
>    In a separate patch, if you wish.
>

Thank you for your comment.

[PATCH v2 1/4] net: phy: realtek: simplify multiple drivers registration

[Jongsung Kim]

Use the phy_drivers_register and phy_drivers_unregister to
simplify registration and error handling. The two existing
phy_driver structures are converted to an array of the
structures to do this.

Signed-off-by: Jongsung Kim <neidhard.kim@lge.com>
---
 drivers/net/phy/realtek.c |   65 ++++++++++++++++++++-------------------------
 1 files changed, 29 insertions(+), 36 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 8e7af83..f902107 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -64,50 +64,43 @@ static int rtl8211e_config_intr(struct phy_device *phydev)
 	return err;
 }
 
-/* RTL8211B */
-static struct phy_driver rtl8211b_driver = {
-	.phy_id		= 0x001cc912,
-	.name		= "RTL8211B Gigabit Ethernet",
-	.phy_id_mask	= 0x001fffff,
-	.features	= PHY_GBIT_FEATURES,
-	.flags		= PHY_HAS_INTERRUPT,
-	.config_aneg	= &genphy_config_aneg,
-	.read_status	= &genphy_read_status,
-	.ack_interrupt	= &rtl821x_ack_interrupt,
-	.config_intr	= &rtl8211b_config_intr,
-	.driver		= { .owner = THIS_MODULE,},
-};
-
-/* RTL8211E */
-static struct phy_driver rtl8211e_driver = {
-	.phy_id		= 0x001cc915,
-	.name		= "RTL8211E Gigabit Ethernet",
-	.phy_id_mask	= 0x001fffff,
-	.features	= PHY_GBIT_FEATURES,
-	.flags		= PHY_HAS_INTERRUPT,
-	.config_aneg	= &genphy_config_aneg,
-	.read_status	= &genphy_read_status,
-	.ack_interrupt	= &rtl821x_ack_interrupt,
-	.config_intr	= &rtl8211e_config_intr,
-	.suspend	= genphy_suspend,
-	.resume		= genphy_resume,
-	.driver		= { .owner = THIS_MODULE,},
+static struct phy_driver realtek_drv[] = {
+	{	/* RTL8211B */
+		.phy_id		= 0x001cc912,
+		.name		= "RTL8211B Gigabit Ethernet",
+		.phy_id_mask	= 0x001fffff,
+		.features	= PHY_GBIT_FEATURES,
+		.flags		= PHY_HAS_INTERRUPT,
+		.config_aneg	= &genphy_config_aneg,
+		.read_status	= &genphy_read_status,
+		.ack_interrupt	= &rtl821x_ack_interrupt,
+		.config_intr	= &rtl8211b_config_intr,
+		.driver		= { .owner = THIS_MODULE,},
+	},
+	{	/* RTL8211E */
+		.phy_id		= 0x001cc915,
+		.name		= "RTL8211E Gigabit Ethernet",
+		.phy_id_mask	= 0x001fffff,
+		.features	= PHY_GBIT_FEATURES,
+		.flags		= PHY_HAS_INTERRUPT,
+		.config_aneg	= &genphy_config_aneg,
+		.read_status	= &genphy_read_status,
+		.ack_interrupt	= &rtl821x_ack_interrupt,
+		.config_intr	= &rtl8211e_config_intr,
+		.suspend	= genphy_suspend,
+		.resume		= genphy_resume,
+		.driver		= { .owner = THIS_MODULE,},
+	},
 };
 
 static int __init realtek_init(void)
 {
-	int ret;
-
-	ret = phy_driver_register(&rtl8211b_driver);
-	if (ret < 0)
-		return -ENODEV;
-	return phy_driver_register(&rtl8211e_driver);
+	return phy_drivers_register(realtek_drv, ARRAY_SIZE(realtek_drv));
 }
 
 static void __exit realtek_exit(void)
 {
-	phy_driver_unregister(&rtl8211b_driver);
-	phy_driver_unregister(&rtl8211e_driver);
+	phy_drivers_unregister(realtek_drv, ARRAY_SIZE(realtek_drv));
 }
 
 module_init(realtek_init);
-- 
1.7.1

[PATCH v2 2/4] net: phy: realtek: add support for the RTL8201F

[Jongsung Kim]

Add the minimal driver to support the Realtek RTL8201F 10/100Mbps
Ethernet Transceivers.

The help message for the config REALTEK_PHY is also modified to
describe the current status of the module correctly.

Signed-off-by: Jongsung Kim <neidhard.kim@lge.com>
---
 drivers/net/phy/Kconfig   |    2 +-
 drivers/net/phy/realtek.c |   52 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 1 deletions(-)

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 1e11f2b..000425e 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -84,7 +84,7 @@ config ICPLUS_PHY
 config REALTEK_PHY
 	tristate "Drivers for Realtek PHYs"
 	---help---
-	  Supports the Realtek 821x PHY.
+	  Currently supports the RTL8201F, RTL8211B and RTL8211E PHYs.
 
 config NATIONAL_PHY
 	tristate "Drivers for National Semiconductor PHYs"
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index f902107..8e95e38 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -16,6 +16,11 @@
 #include <linux/phy.h>
 #include <linux/module.h>
 
+#define RTL8201F_INSR		0x1e
+#define RTL8201F_PGSR		0x1f
+#define RTL8201F_INER		0x13
+#define RTL8201F_INER_MASK	0x3800
+
 #define RTL821x_PHYSR		0x11
 #define RTL821x_PHYSR_DUPLEX	0x2000
 #define RTL821x_PHYSR_SPEED	0xc000
@@ -29,6 +34,15 @@ MODULE_DESCRIPTION("Realtek PHY driver");
 MODULE_AUTHOR("Johnson Leung");
 MODULE_LICENSE("GPL");
 
+static int rtl8201f_ack_interrupt(struct phy_device *phydev)
+{
+	int err;
+
+	err = phy_read(phydev, RTL8201F_INSR);
+
+	return (err < 0) ? err : 0;
+}
+
 static int rtl821x_ack_interrupt(struct phy_device *phydev)
 {
 	int err;
@@ -38,6 +52,29 @@ static int rtl821x_ack_interrupt(struct phy_device *phydev)
 	return (err < 0) ? err : 0;
 }
 
+static void rtl8201f_select_page(struct phy_device *phydev, int page)
+{
+	phy_write(phydev, RTL8201F_PGSR, page);
+}
+
+static int rtl8201f_config_intr(struct phy_device *phydev)
+{
+	int err;
+
+	rtl8201f_select_page(phydev, 7);
+
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+		err = phy_write(phydev, RTL8201F_INER, RTL8201F_INER_MASK |
+				phy_read(phydev, RTL8201F_INER));
+	else
+		err = phy_write(phydev, RTL8201F_INER, ~RTL8201F_INER_MASK &
+				phy_read(phydev, RTL8201F_INER));
+
+	rtl8201f_select_page(phydev, 0);
+
+	return err;
+}
+
 static int rtl8211b_config_intr(struct phy_device *phydev)
 {
 	int err;
@@ -65,6 +102,20 @@ static int rtl8211e_config_intr(struct phy_device *phydev)
 }
 
 static struct phy_driver realtek_drv[] = {
+	{	/* RTL8201F */
+		.phy_id		= 0x001cc816,
+		.name		= "RTL8201F 10/100Mbps Ethernet",
+		.phy_id_mask	= 0x001fffff,
+		.features	= PHY_BASIC_FEATURES,
+		.flags		= PHY_HAS_INTERRUPT,
+		.config_aneg	= &genphy_config_aneg,
+		.read_status	= &genphy_read_status,
+		.ack_interrupt	= &rtl8201f_ack_interrupt,
+		.config_intr	= &rtl8201f_config_intr,
+		.suspend	= genphy_suspend,
+		.resume		= genphy_resume,
+		.driver		= { .owner = THIS_MODULE,},
+	},
 	{	/* RTL8211B */
 		.phy_id		= 0x001cc912,
 		.name		= "RTL8211B Gigabit Ethernet",
@@ -107,6 +158,7 @@ module_init(realtek_init);
 module_exit(realtek_exit);
 
 static struct mdio_device_id __maybe_unused realtek_tbl[] = {
+	{ 0x001cc816, 0x001fffff },
 	{ 0x001cc912, 0x001fffff },
 	{ 0x001cc915, 0x001fffff },
 	{ }
-- 
1.7.1

[PATCH v2 3/4] net: phy: realtek: add suspend/resume handlers for the RTL8211B

[Jongsung Kim]

Add compatible but missing generic PHY suspend/resume handlers for
the RTL8211B driver.

Signed-off-by: Jongsung Kim <neidhard.kim@lge.com>
---
 drivers/net/phy/realtek.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 8e95e38..6f0726a 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -126,6 +126,8 @@ static struct phy_driver realtek_drv[] = {
 		.read_status	= &genphy_read_status,
 		.ack_interrupt	= &rtl821x_ack_interrupt,
 		.config_intr	= &rtl8211b_config_intr,
+		.suspend	= genphy_suspend,
+		.resume		= genphy_resume,
 		.driver		= { .owner = THIS_MODULE,},
 	},
 	{	/* RTL8211E */
-- 
1.7.1

[PATCH v2 4/4] net: phy: realtek: cleanup code

[Jongsung Kim]

This patch cleans up the drivers code by:

- using a consistent way to reference functions
- removing unused macro-definitions
- removing unnecessary new-lines
- making ack_interrupt functions shorter.

Signed-off-by: Jongsung Kim <neidhard.kim@lge.com>
---
 drivers/net/phy/realtek.c |   29 ++++++++++-------------------
 1 files changed, 10 insertions(+), 19 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 6f0726a..fd09844 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -21,9 +21,6 @@
 #define RTL8201F_INER		0x13
 #define RTL8201F_INER_MASK	0x3800
 
-#define RTL821x_PHYSR		0x11
-#define RTL821x_PHYSR_DUPLEX	0x2000
-#define RTL821x_PHYSR_SPEED	0xc000
 #define RTL821x_INER		0x12
 #define RTL821x_INER_INIT	0x6400
 #define RTL821x_INSR		0x13
@@ -36,18 +33,14 @@ MODULE_LICENSE("GPL");
 
 static int rtl8201f_ack_interrupt(struct phy_device *phydev)
 {
-	int err;
-
-	err = phy_read(phydev, RTL8201F_INSR);
+	int err = phy_read(phydev, RTL8201F_INSR);
 
 	return (err < 0) ? err : 0;
 }
 
 static int rtl821x_ack_interrupt(struct phy_device *phydev)
 {
-	int err;
-
-	err = phy_read(phydev, RTL821x_INSR);
+	int err = phy_read(phydev, RTL821x_INSR);
 
 	return (err < 0) ? err : 0;
 }
@@ -80,8 +73,7 @@ static int rtl8211b_config_intr(struct phy_device *phydev)
 	int err;
 
 	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
-		err = phy_write(phydev, RTL821x_INER,
-				RTL821x_INER_INIT);
+		err = phy_write(phydev, RTL821x_INER, RTL821x_INER_INIT);
 	else
 		err = phy_write(phydev, RTL821x_INER, 0);
 
@@ -93,8 +85,7 @@ static int rtl8211e_config_intr(struct phy_device *phydev)
 	int err;
 
 	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
-		err = phy_write(phydev, RTL821x_INER,
-				RTL8211E_INER_LINK_STAT);
+		err = phy_write(phydev, RTL821x_INER, RTL8211E_INER_LINK_STAT);
 	else
 		err = phy_write(phydev, RTL821x_INER, 0);
 
@@ -112,8 +103,8 @@ static struct phy_driver realtek_drv[] = {
 		.read_status	= &genphy_read_status,
 		.ack_interrupt	= &rtl8201f_ack_interrupt,
 		.config_intr	= &rtl8201f_config_intr,
-		.suspend	= genphy_suspend,
-		.resume		= genphy_resume,
+		.suspend	= &genphy_suspend,
+		.resume		= &genphy_resume,
 		.driver		= { .owner = THIS_MODULE,},
 	},
 	{	/* RTL8211B */
@@ -126,8 +117,8 @@ static struct phy_driver realtek_drv[] = {
 		.read_status	= &genphy_read_status,
 		.ack_interrupt	= &rtl821x_ack_interrupt,
 		.config_intr	= &rtl8211b_config_intr,
-		.suspend	= genphy_suspend,
-		.resume		= genphy_resume,
+		.suspend	= &genphy_suspend,
+		.resume		= &genphy_resume,
 		.driver		= { .owner = THIS_MODULE,},
 	},
 	{	/* RTL8211E */
@@ -140,8 +131,8 @@ static struct phy_driver realtek_drv[] = {
 		.read_status	= &genphy_read_status,
 		.ack_interrupt	= &rtl821x_ack_interrupt,
 		.config_intr	= &rtl8211e_config_intr,
-		.suspend	= genphy_suspend,
-		.resume		= genphy_resume,
+		.suspend	= &genphy_suspend,
+		.resume		= &genphy_resume,
 		.driver		= { .owner = THIS_MODULE,},
 	},
 };
-- 
1.7.1

Re: [PATCH v2 4/4] net: phy: realtek: cleanup code

[Sergei Shtylyov]

Hello.

On 10-05-2013 11:29, Jongsung Kim wrote:

> This patch cleans up the drivers code by:

> - using a consistent way to reference functions
> - removing unused macro-definitions
> - removing unnecessary new-lines
> - making ack_interrupt functions shorter.

> Signed-off-by: Jongsung Kim <neidhard.kim@lge.com>
> ---
>   drivers/net/phy/realtek.c |   29 ++++++++++-------------------
>   1 files changed, 10 insertions(+), 19 deletions(-)

> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index 6f0726a..fd09844 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
[...]
> @@ -112,8 +103,8 @@ static struct phy_driver realtek_drv[] = {
>   		.read_status	= &genphy_read_status,
>   		.ack_interrupt	= &rtl8201f_ack_interrupt,
>   		.config_intr	= &rtl8201f_config_intr,
> -		.suspend	= genphy_suspend,
> -		.resume		= genphy_resume,
> +		.suspend	= &genphy_suspend,
> +		.resume		= &genphy_resume,

    Contrariwise, you should have dropped & from the other functions. 
It's completely superfluous.

WBR, Sergei

RE: [PATCH v2 4/4] net: phy: realtek: cleanup code

[Jongsung Kim]

Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> :
>> -		.suspend	= genphy_suspend,
>> -		.resume		= genphy_resume,
>> +		.suspend	= &genphy_suspend,
>> +		.resume		= &genphy_resume,
>
>    Contrariwise, you should have dropped & from the other functions. 
>It's completely superfluous.
>

Agreed. Thank you  for your comment, Sergei.

[PATCH v3 1/4] net: phy: realtek: simplify multiple drivers registration

[Jongsung Kim]

Use the phy_drivers_register and phy_drivers_unregister to
simplify registration and error handling. The two existing
phy_driver structures are converted to an array of the
structures to do this.

v2: use proper way to register multiple drivers.

Signed-off-by: Jongsung Kim <neidhard.kim@lge.com>
Cc: Francois Romieu <romieu@fr.zoreil.com>
---
 drivers/net/phy/realtek.c |   65 ++++++++++++++++++++-------------------------
 1 files changed, 29 insertions(+), 36 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 8e7af83..f902107 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -64,50 +64,43 @@ static int rtl8211e_config_intr(struct phy_device *phydev)
 	return err;
 }
 
-/* RTL8211B */
-static struct phy_driver rtl8211b_driver = {
-	.phy_id		= 0x001cc912,
-	.name		= "RTL8211B Gigabit Ethernet",
-	.phy_id_mask	= 0x001fffff,
-	.features	= PHY_GBIT_FEATURES,
-	.flags		= PHY_HAS_INTERRUPT,
-	.config_aneg	= &genphy_config_aneg,
-	.read_status	= &genphy_read_status,
-	.ack_interrupt	= &rtl821x_ack_interrupt,
-	.config_intr	= &rtl8211b_config_intr,
-	.driver		= { .owner = THIS_MODULE,},
-};
-
-/* RTL8211E */
-static struct phy_driver rtl8211e_driver = {
-	.phy_id		= 0x001cc915,
-	.name		= "RTL8211E Gigabit Ethernet",
-	.phy_id_mask	= 0x001fffff,
-	.features	= PHY_GBIT_FEATURES,
-	.flags		= PHY_HAS_INTERRUPT,
-	.config_aneg	= &genphy_config_aneg,
-	.read_status	= &genphy_read_status,
-	.ack_interrupt	= &rtl821x_ack_interrupt,
-	.config_intr	= &rtl8211e_config_intr,
-	.suspend	= genphy_suspend,
-	.resume		= genphy_resume,
-	.driver		= { .owner = THIS_MODULE,},
+static struct phy_driver realtek_drv[] = {
+	{	/* RTL8211B */
+		.phy_id		= 0x001cc912,
+		.name		= "RTL8211B Gigabit Ethernet",
+		.phy_id_mask	= 0x001fffff,
+		.features	= PHY_GBIT_FEATURES,
+		.flags		= PHY_HAS_INTERRUPT,
+		.config_aneg	= &genphy_config_aneg,
+		.read_status	= &genphy_read_status,
+		.ack_interrupt	= &rtl821x_ack_interrupt,
+		.config_intr	= &rtl8211b_config_intr,
+		.driver		= { .owner = THIS_MODULE,},
+	},
+	{	/* RTL8211E */
+		.phy_id		= 0x001cc915,
+		.name		= "RTL8211E Gigabit Ethernet",
+		.phy_id_mask	= 0x001fffff,
+		.features	= PHY_GBIT_FEATURES,
+		.flags		= PHY_HAS_INTERRUPT,
+		.config_aneg	= &genphy_config_aneg,
+		.read_status	= &genphy_read_status,
+		.ack_interrupt	= &rtl821x_ack_interrupt,
+		.config_intr	= &rtl8211e_config_intr,
+		.suspend	= genphy_suspend,
+		.resume		= genphy_resume,
+		.driver		= { .owner = THIS_MODULE,},
+	},
 };
 
 static int __init realtek_init(void)
 {
-	int ret;
-
-	ret = phy_driver_register(&rtl8211b_driver);
-	if (ret < 0)
-		return -ENODEV;
-	return phy_driver_register(&rtl8211e_driver);
+	return phy_drivers_register(realtek_drv, ARRAY_SIZE(realtek_drv));
 }
 
 static void __exit realtek_exit(void)
 {
-	phy_driver_unregister(&rtl8211b_driver);
-	phy_driver_unregister(&rtl8211e_driver);
+	phy_drivers_unregister(realtek_drv, ARRAY_SIZE(realtek_drv));
 }
 
 module_init(realtek_init);
-- 
1.7.1

[PATCH v3 2/4] net: phy: realtek: add support for the RTL8201F

[Jongsung Kim]

Add the minimal driver to support the Realtek RTL8201F 10/100Mbps
Ethernet Transceivers.

The help message for the config REALTEK_PHY is also modified to
describe the current status of the module correctly.

Changes in v2:
* separated patch (pointed by Francois Romieu and Sergei Shtylyov)
* create register page-selecting function (pointed by Francois Romieu)

Signed-off-by: Jongsung Kim <neidhard.kim@lge.com>
Cc: Francois Romieu <romieu@fr.zoreil.com>
Cc: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
---
 drivers/net/phy/Kconfig   |    2 +-
 drivers/net/phy/realtek.c |   52 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 1 deletions(-)

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 1e11f2b..000425e 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -84,7 +84,7 @@ config ICPLUS_PHY
 config REALTEK_PHY
 	tristate "Drivers for Realtek PHYs"
 	---help---
-	  Supports the Realtek 821x PHY.
+	  Currently supports the RTL8201F, RTL8211B and RTL8211E PHYs.
 
 config NATIONAL_PHY
 	tristate "Drivers for National Semiconductor PHYs"
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index f902107..8e95e38 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -16,6 +16,11 @@
 #include <linux/phy.h>
 #include <linux/module.h>
 
+#define RTL8201F_INSR		0x1e
+#define RTL8201F_PGSR		0x1f
+#define RTL8201F_INER		0x13
+#define RTL8201F_INER_MASK	0x3800
+
 #define RTL821x_PHYSR		0x11
 #define RTL821x_PHYSR_DUPLEX	0x2000
 #define RTL821x_PHYSR_SPEED	0xc000
@@ -29,6 +34,15 @@ MODULE_DESCRIPTION("Realtek PHY driver");
 MODULE_AUTHOR("Johnson Leung");
 MODULE_LICENSE("GPL");
 
+static int rtl8201f_ack_interrupt(struct phy_device *phydev)
+{
+	int err;
+
+	err = phy_read(phydev, RTL8201F_INSR);
+
+	return (err < 0) ? err : 0;
+}
+
 static int rtl821x_ack_interrupt(struct phy_device *phydev)
 {
 	int err;
@@ -38,6 +52,29 @@ static int rtl821x_ack_interrupt(struct phy_device *phydev)
 	return (err < 0) ? err : 0;
 }
 
+static void rtl8201f_select_page(struct phy_device *phydev, int page)
+{
+	phy_write(phydev, RTL8201F_PGSR, page);
+}
+
+static int rtl8201f_config_intr(struct phy_device *phydev)
+{
+	int err;
+
+	rtl8201f_select_page(phydev, 7);
+
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+		err = phy_write(phydev, RTL8201F_INER, RTL8201F_INER_MASK |
+				phy_read(phydev, RTL8201F_INER));
+	else
+		err = phy_write(phydev, RTL8201F_INER, ~RTL8201F_INER_MASK &
+				phy_read(phydev, RTL8201F_INER));
+
+	rtl8201f_select_page(phydev, 0);
+
+	return err;
+}
+
 static int rtl8211b_config_intr(struct phy_device *phydev)
 {
 	int err;
@@ -65,6 +102,20 @@ static int rtl8211e_config_intr(struct phy_device *phydev)
 }
 
 static struct phy_driver realtek_drv[] = {
+	{	/* RTL8201F */
+		.phy_id		= 0x001cc816,
+		.name		= "RTL8201F 10/100Mbps Ethernet",
+		.phy_id_mask	= 0x001fffff,
+		.features	= PHY_BASIC_FEATURES,
+		.flags		= PHY_HAS_INTERRUPT,
+		.config_aneg	= &genphy_config_aneg,
+		.read_status	= &genphy_read_status,
+		.ack_interrupt	= &rtl8201f_ack_interrupt,
+		.config_intr	= &rtl8201f_config_intr,
+		.suspend	= genphy_suspend,
+		.resume		= genphy_resume,
+		.driver		= { .owner = THIS_MODULE,},
+	},
 	{	/* RTL8211B */
 		.phy_id		= 0x001cc912,
 		.name		= "RTL8211B Gigabit Ethernet",
@@ -107,6 +158,7 @@ module_init(realtek_init);
 module_exit(realtek_exit);
 
 static struct mdio_device_id __maybe_unused realtek_tbl[] = {
+	{ 0x001cc816, 0x001fffff },
 	{ 0x001cc912, 0x001fffff },
 	{ 0x001cc915, 0x001fffff },
 	{ }
-- 
1.7.1

[PATCH v3 3/4] net: phy: realtek: add suspend/resume handlers for the RTL8211B

[Jongsung Kim]

Add compatible but missing generic PHY suspend/resume handlers for
the RTL8211B driver.

Signed-off-by: Jongsung Kim <neidhard.kim@lge.com>
---
 drivers/net/phy/realtek.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 8e95e38..6f0726a 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -126,6 +126,8 @@ static struct phy_driver realtek_drv[] = {
 		.read_status	= &genphy_read_status,
 		.ack_interrupt	= &rtl821x_ack_interrupt,
 		.config_intr	= &rtl8211b_config_intr,
+		.suspend	= genphy_suspend,
+		.resume		= genphy_resume,
 		.driver		= { .owner = THIS_MODULE,},
 	},
 	{	/* RTL8211E */
-- 
1.7.1

[PATCH v3 4/4] net: phy: realtek: cleanup code

[Jongsung Kim]

This patch cleans up the drivers code by:

- using a consistent way to reference functions
- removing unused macro-definitions
- removing unnecessary new-lines
- making ack_interrupt functions shorter.

Changes in v3:
* don't use the '&' to reference functions (pointed by Sergei Shtylyov)

Changes in v2:
* separated patch (pointed by Francois Romieu and Sergei Shtylyov)
* make functions shorter (pointed by Sergei Shtylyov)

Signed-off-by: Jongsung Kim <neidhard.kim@lge.com>
Cc: Francois Romieu <romieu@fr.zoreil.com>
Cc: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
---
 drivers/net/phy/realtek.c |   41 ++++++++++++++++-------------------------
 1 files changed, 16 insertions(+), 25 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 6f0726a..b55aea2 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -21,9 +21,6 @@
 #define RTL8201F_INER		0x13
 #define RTL8201F_INER_MASK	0x3800
 
-#define RTL821x_PHYSR		0x11
-#define RTL821x_PHYSR_DUPLEX	0x2000
-#define RTL821x_PHYSR_SPEED	0xc000
 #define RTL821x_INER		0x12
 #define RTL821x_INER_INIT	0x6400
 #define RTL821x_INSR		0x13
@@ -36,18 +33,14 @@ MODULE_LICENSE("GPL");
 
 static int rtl8201f_ack_interrupt(struct phy_device *phydev)
 {
-	int err;
-
-	err = phy_read(phydev, RTL8201F_INSR);
+	int err = phy_read(phydev, RTL8201F_INSR);
 
 	return (err < 0) ? err : 0;
 }
 
 static int rtl821x_ack_interrupt(struct phy_device *phydev)
 {
-	int err;
-
-	err = phy_read(phydev, RTL821x_INSR);
+	int err = phy_read(phydev, RTL821x_INSR);
 
 	return (err < 0) ? err : 0;
 }
@@ -80,8 +73,7 @@ static int rtl8211b_config_intr(struct phy_device *phydev)
 	int err;
 
 	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
-		err = phy_write(phydev, RTL821x_INER,
-				RTL821x_INER_INIT);
+		err = phy_write(phydev, RTL821x_INER, RTL821x_INER_INIT);
 	else
 		err = phy_write(phydev, RTL821x_INER, 0);
 
@@ -93,8 +85,7 @@ static int rtl8211e_config_intr(struct phy_device *phydev)
 	int err;
 
 	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
-		err = phy_write(phydev, RTL821x_INER,
-				RTL8211E_INER_LINK_STAT);
+		err = phy_write(phydev, RTL821x_INER, RTL8211E_INER_LINK_STAT);
 	else
 		err = phy_write(phydev, RTL821x_INER, 0);
 
@@ -108,10 +99,10 @@ static struct phy_driver realtek_drv[] = {
 		.phy_id_mask	= 0x001fffff,
 		.features	= PHY_BASIC_FEATURES,
 		.flags		= PHY_HAS_INTERRUPT,
-		.config_aneg	= &genphy_config_aneg,
-		.read_status	= &genphy_read_status,
-		.ack_interrupt	= &rtl8201f_ack_interrupt,
-		.config_intr	= &rtl8201f_config_intr,
+		.config_aneg	= genphy_config_aneg,
+		.read_status	= genphy_read_status,
+		.ack_interrupt	= rtl8201f_ack_interrupt,
+		.config_intr	= rtl8201f_config_intr,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
 		.driver		= { .owner = THIS_MODULE,},
@@ -122,10 +113,10 @@ static struct phy_driver realtek_drv[] = {
 		.phy_id_mask	= 0x001fffff,
 		.features	= PHY_GBIT_FEATURES,
 		.flags		= PHY_HAS_INTERRUPT,
-		.config_aneg	= &genphy_config_aneg,
-		.read_status	= &genphy_read_status,
-		.ack_interrupt	= &rtl821x_ack_interrupt,
-		.config_intr	= &rtl8211b_config_intr,
+		.config_aneg	= genphy_config_aneg,
+		.read_status	= genphy_read_status,
+		.ack_interrupt	= rtl821x_ack_interrupt,
+		.config_intr	= rtl8211b_config_intr,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
 		.driver		= { .owner = THIS_MODULE,},
@@ -136,10 +127,10 @@ static struct phy_driver realtek_drv[] = {
 		.phy_id_mask	= 0x001fffff,
 		.features	= PHY_GBIT_FEATURES,
 		.flags		= PHY_HAS_INTERRUPT,
-		.config_aneg	= &genphy_config_aneg,
-		.read_status	= &genphy_read_status,
-		.ack_interrupt	= &rtl821x_ack_interrupt,
-		.config_intr	= &rtl8211e_config_intr,
+		.config_aneg	= genphy_config_aneg,
+		.read_status	= genphy_read_status,
+		.ack_interrupt	= rtl821x_ack_interrupt,
+		.config_intr	= rtl8211e_config_intr,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
 		.driver		= { .owner = THIS_MODULE,},
-- 
1.7.1