netdev - Re: [PATCH net-next 2/2] net: phy: microchip_t1s: add support for Microchip LAN865x Rev.B0 PHYs

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]

Message-ID: <85f657bf-e2c6-0ad3-bac0-b87904697644@microchip.com>
Date:   Thu, 27 Apr 2023 14:19:24 +0000
From:   <Parthiban.Veerasooran@...rochip.com>
To:     <andrew@...n.ch>
CC:     <netdev@...r.kernel.org>, <davem@...emloft.net>,
        <Jan.Huber@...rochip.com>, <Thorsten.Kummermehr@...rochip.com>,
        <ramon.nordin.rodriguez@...roamp.se>
Subject: Re: [PATCH net-next 2/2] net: phy: microchip_t1s: add support for
 Microchip LAN865x Rev.B0 PHYs

On 27/04/23 3:22 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> -#define LAN867X_REG_IRQ_1_CTL 0x001C
>> -#define LAN867X_REG_IRQ_2_CTL 0x001D
>> +#define LAN86XX_REG_IRQ_1_CTL 0x001C
>> +#define LAN86XX_REG_IRQ_2_CTL 0x001D
> 
> This patch is pretty big. Please split to LAN867X to LAN86XX rename
> out, to make the patches smaller and easier to reviewSure, will do it in the next version.
> 
>> +static int lan865x_revb0_plca_set_cfg(struct phy_device *phydev,
>> +                                   const struct phy_plca_cfg *plca_cfg)
>> +{
>> +     int ret;
>> +
>> +     ret = genphy_c45_plca_set_cfg(phydev, plca_cfg);
>> +     if (ret)
>> +             return ret;
>> +
>> +     /* Disable the collision detection when PLCA is enabled and enable
>> +      * collision detection when CSMA/CD mode is enabled.
>> +      */
>> +     if (plca_cfg->enabled)
>> +             return phy_write_mmd(phydev, MDIO_MMD_VEND2, 0x0087, 0x0000);
>> +     else
>> +             return phy_write_mmd(phydev, MDIO_MMD_VEND2, 0x0087, 0x0083);
>> +}
>> +
> 
> This could be in a patch of its own, with a good commit message
> explaining why it is needed.
Sure, as Ramon asked in his review comment this is also needed for 
lan867x rev.b1 as well. So will make it as a separate patch before 
lan865x rev.b0 patch.
> 
> Once you replace the magic numbers with #defines, the comment becomes
> pointless. But what is missing is the answer to the question Why?
As I commented in previous email to Ramon, I am in contact with design 
team for the clarification that why it is not documented. Will get more 
info soon.
> 
>             Andrew
>