Patchwork net: phy: realtek: add rtl8201f driver

login
register
mail settings
Submitter Jongsung Kim
Date May 8, 2013, 10:10 a.m.
Message ID <201305081910.27203.neidhard.kim@lge.com>
Download mbox | patch
Permalink /patch/242540/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Jongsung Kim - May 8, 2013, 10:10 a.m.
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(-)

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
fran├žois romieu - May 8, 2013, 10:41 a.m.
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}
Sergei Shtylyov - May 8, 2013, 3:43 p.m.
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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jongsung Kim - May 9, 2013, 2:26 a.m.
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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jongsung Kim - May 9, 2013, 2:35 a.m.
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..


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov - May 9, 2013, 4:58 p.m.
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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jongsung Kim - May 10, 2013, 3:44 a.m.
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.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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 },
 	{ }