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

[Jassi Brar]

On 9 September 2017 at 00:21, Florian Fainelli <f.fainelli@xxxxxxxxx> wrote:
> On 09/08/2017 06:02 AM, Kunihiko Hayashi wrote:
>> From: Jassi Brar <jaswinder.singh@xxxxxxxxxx>
>>
>> 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@xxxxxxx>
>> Signed-off-by: Jassi Brar <jaswinder.singh@xxxxxxxxxx>
>> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@xxxxxxxxxxxxx>
>> ---
>>  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@xxxxxxxxx>
>
Thank you.