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

From: <[email-masked]>
Date: Thu, 27 Apr 2023 14:19:24 +0000

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