Re: [PATCH net-next 3/3] net: phy: realtek: add RTL8201F phy-id and functions

On 9 September 2017 at 00:21, Florian Fainelli [off-list ref] wrote:

On 09/08/2017 06:02 AM, Kunihiko Hayashi wrote:

quoted

From: Jassi Brar <redacted>

Add RTL8201F phy-id and the related functions to the driver.

The original patch is as follows:
https://patchwork.kernel.org/patch/2538341/

Signed-off-by: Jongsung Kim <redacted>
Signed-off-by: Jassi Brar <redacted>
Signed-off-by: Kunihiko Hayashi <redacted>
---
 drivers/net/phy/realtek.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 9cbe645..d9974ce 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c

@@ -29,10 +29,23 @@
 #define RTL8211F_PAGE_SELECT 0x1f
 #define RTL8211F_TX_DELAY    0x100

+#define RTL8201F_ISR         0x1e
+#define RTL8201F_PAGE_SELECT 0x1f

We have a page select register define for the RTL8211F right above, so
surely we can make that a common definition?

That is just for the sake of consistency.
I mean RTL8211 wouldn't look neat among everything else RTL8201.

Also the page-select offsets just _happen_ to be same value...
RTL8211E_INER_LINK_STATUS and RTL8211F_INER_LINK_STATUS are very
different.

quoted

+#define RTL8201F_IER         0x13
+
 MODULE_DESCRIPTION("Realtek PHY driver");
 MODULE_AUTHOR("Johnson Leung");
 MODULE_LICENSE("GPL");

+static int rtl8201_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;

@@ -54,6 +67,25 @@ static int rtl8211f_ack_interrupt(struct phy_device *phydev)
      return (err < 0) ? err : 0;
 }

+static int rtl8201_config_intr(struct phy_device *phydev)
+{
+     int err;
+
+     /* switch to page 7 */
+     phy_write(phydev, RTL8201F_PAGE_SELECT, 0x7);
+
+     if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+             err = phy_write(phydev, RTL8201F_IER,
+                             BIT(13) | BIT(12) | BIT(11));

Can you detail what bits 11, 12 and 13 do? Do they correspond to link,
duplex and pause changes by any chance?

Sorry no idea. The datasheet would say, and other functions too use
such magic values.

quoted

+     else
+             err = phy_write(phydev, RTL8201F_IER, 0);
+
+     /* restore to default page 0 */
+     phy_write(phydev, RTL8201F_PAGE_SELECT, 0x0);
+
+     return err;
+}
+

Other than that, LGTM:

Reviewed-by: Florian Fainelli <redacted>

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