Re: [RFC PATCH net-next 5/6] microchip: lan865x: add driver support for Microchip's LAN865X MACPHY

[Parthiban.Veerasooran]

Hi Andrew,

On 19/09/23 6:20 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> Sure, I can move this part to oa_tc6 lib. If I understand you correctly
>> you are talking about the Standard Capabilities Register (0x0002)
>> defined in the OPEN Alliance 10BASE-T1x MAC-PHY Serial Interface spec
>> right? If so, the 9th bit of this register tells about Indirect PHY
>> Register access Capability. Did you mean this bit? If so, this bit
>> describes the below,
>>
>> IPRAC - Indirect PHY Register Access Capability. Indicates if PHY
>> registers are indirectly accessible through the MDIO/MDC registers MDIOACCn.
> 
> Yes. If the core relies on any functionality which is optional in the
> standard, it should check if the capability bit is set, and do a
> dev_erro() and return -ENODEV if a device does not actually have
> it. That makes it clear the core needs extending to support a device.
Ok, will do that.
> 
> If you are only using mandatory parts of the spec, then no test is
> needed.
Ok.
> 
>>> I would expect to see a call to phy_ethtool_ksettings_set()
>>> here. phylib should be able to do some of the validation.
>> Ah ok, doing the below will make the life easier.
>> .set_link_ksettings   = phy_ethtool_set_link_ksettings,
> 
> Please do some testing and check that phy_ethtool_set_link_ksettings
> doe actually reject all invalid setting. I cannot guarantee it does,
> and if it does not, it might actually be a PHY driver bug.
Yes sure.
> 
>>>> +static int lan865x_net_open(struct net_device *netdev)
>>>> +{
>>>> +     struct lan865x_priv *priv = netdev_priv(netdev);
>>>> +     int ret;
>>>> +
>>>> +     if (!is_valid_ether_addr(netdev->dev_addr)) {
>>>> +             if (netif_msg_ifup(priv))
>>>> +                     netdev_err(netdev, "Invalid MAC address %pm", netdev->dev_addr);
>>>> +             return -EADDRNOTAVAIL;
>>>
>>> Using a random MAC address is the normal workaround for not having a
>>> valid MAC address via OTP flash etc.
>> Ah ok, you mean to use eth_hw_addr_random(netdev) instead of returning
>> error.
> 
> Yes. And this is generally done earlier than open, as part of
> probe. You want to avoid surprising userspace when the MAC address
> suddenly changes at open time.
Ah ok ok, will move that to probe.

Best Regards,
Parthiban V
> 
>           Andrew