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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 9 Sep 2017 09:03:05 +0530
From:   Jassi Brar <jaswinder.singh@...aro.org>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     Kunihiko Hayashi <hayashi.kunihiko@...ionext.com>,
        netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        Andrew Lunn <andrew@...n.ch>, Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        lkml <linux-kernel@...r.kernel.org>,
        Devicetree List <devicetree@...r.kernel.org>,
        Masahiro Yamada <yamada.masahiro@...ionext.com>,
        Masami Hiramatsu <masami.hiramatsu@...aro.org>,
        Jongsung Kim <neidhard.kim@....com>
Subject: Re: [PATCH net-next 3/3] net: phy: realtek: add RTL8201F phy-id and functions

On 9 September 2017 at 00:21, Florian Fainelli <f.fainelli@...il.com> wrote:
> On 09/08/2017 06:02 AM, Kunihiko Hayashi wrote:
>> From: Jassi Brar <jaswinder.singh@...aro.org>
>>
>> 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 <neidhard.kim@....com>
>> Signed-off-by: Jassi Brar <jaswinder.singh@...aro.org>
>> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@...ionext.com>
>> ---
>>  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.

>> +#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.

>> +     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 <f.fainelli@...il.com>
>
Thank you.