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

[<Parthiban.Veerasooran@microchip.com>]

Hi Simon,

Thank you for reviewing the patch.

On 10/09/23 11:14 pm, Simon Horman wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Fri, Sep 08, 2023 at 07:59:18PM +0530, Parthiban Veerasooran wrote:
>> The LAN8650/1 is designed to conform to the OPEN Alliance 10BASE‑T1x
>> MAC‑PHY Serial Interface specification, Version 1.1. The IEEE Clause 4
>> MAC integration provides the low pin count standard SPI interface to any
>> microcontroller therefore providing Ethernet functionality without
>> requiring MAC integration within the microcontroller. The LAN8650/1
>> operates as an SPI client supporting SCLK clock rates up to a maximum of
>> 25 MHz. This SPI interface supports the transfer of both data (Ethernet
>> frames) and control (register access).
>>
>> By default, the chunk data payload is 64 bytes in size. A smaller payload
>> data size of 32 bytes is also supported and may be configured in the
>> Chunk Payload Size (CPS) field of the Configuration 0 (OA_CONFIG0)
>> register. Changing the chunk payload size requires the LAN8650/1 be reset
>> and shall not be done during normal operation.
>>
>> The Ethernet Media Access Controller (MAC) module implements a 10 Mbps
>> half duplex Ethernet MAC, compatible with the IEEE 802.3 standard.
>> 10BASE-T1S physical layer transceiver integrated into the LAN8650/1. The
>> PHY and MAC are connected via an internal Media Independent Interface
>> (MII).
>>
>> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
> 
> Hi Parthiban,
> 
> thanks for your patches.
> Some minor feedback on this one follows.
> 
> ...
> 
>> diff --git a/drivers/net/ethernet/microchip/lan865x.c b/drivers/net/ethernet/microchip/lan865x.c
> 
> ...
> 
>> +static int lan865x_set_hw_macaddr(struct net_device *netdev)
>> +{
>> +     u32 regval;
>> +     bool ret;
>> +     struct lan865x_priv *priv = netdev_priv(netdev);
>> +     const u8 *mac = netdev->dev_addr;
> 
> Please arrange local variables in Networking code in reverse xmas tree
> order - longest line to shortest.
> 
> This tool can be of assistance here:
> https://github.com/ecree-solarflare/xmastree
Sure will do it. Somehow this area got escaped from my eyes.
> 
> ...
> 
>> +static int lan865x_probe(struct spi_device *spi)
>> +{
>> +     struct net_device *netdev;
>> +     struct lan865x_priv *priv;
>> +     u32 regval;
>> +     int ret;
>> +
>> +     netdev = alloc_etherdev(sizeof(struct lan865x_priv));
>> +     if (!netdev)
>> +             return -ENOMEM;
>> +
>> +     priv = netdev_priv(netdev);
>> +     priv->netdev = netdev;
>> +     priv->spi = spi;
>> +     priv->msg_enable = 0;
>> +     spi_set_drvdata(spi, priv);
>> +     SET_NETDEV_DEV(netdev, &spi->dev);
>> +
>> +     ret = lan865x_get_dt_data(priv);
>> +     if (ret)
>> +             return ret;
>> +
>> +     spi->rt = true;
>> +     spi_setup(spi);
>> +
>> +     priv->tc6 = oa_tc6_init(spi, netdev);
>> +     if (!priv->tc6) {
>> +             ret = -ENOMEM;
>> +             goto error_oa_tc6_init;
>> +     }
>> +
>> +     if (priv->cps == 32) {
>> +             regval = CCS_Q0_TX_CFG_32;
>> +             ret = oa_tc6_write_register(priv->tc6, CCS_Q0_TX_CFG, &regval, 1);
>> +             if (ret)
>> +                     return ret;
>> +
>> +             regval = CCS_Q0_RX_CFG_32;
>> +             ret = oa_tc6_write_register(priv->tc6, CCS_Q0_RX_CFG, &regval, 1);
>> +             if (ret)
>> +                     return ret;
>> +     }
>> +
>> +     if (oa_tc6_configure(priv->tc6, priv->cps, priv->protected, priv->txcte,
>> +                          priv->rxcte))
>> +             goto err_macphy_config;
> 
> Jumping to err_macphy_config will result in this function returning ret.
> However, ret will be 0 at this point. Perhaps it should be set to an
> error value.
Ah yes, thanks for pointing it out. Will correct in the next version.

Best Regards,
Parthiban V
> 
> Flagged by Smatch.
> 
>> +
>> +     ret = lan865x_phy_init(priv);
>> +     if (ret)
>> +             goto error_phy;
>> +
>> +     if (device_get_ethdev_address(&spi->dev, netdev))
>> +             eth_hw_addr_random(netdev);
>> +
>> +     ret = lan865x_set_hw_macaddr(netdev);
>> +     if (ret) {
>> +             if (netif_msg_probe(priv))
>> +                     dev_err(&spi->dev, "Failed to configure MAC");
>> +             goto error_set_mac;
>> +     }
>> +
>> +     netdev->if_port = IF_PORT_10BASET;
>> +     netdev->irq = spi->irq;
>> +     netdev->netdev_ops = &lan865x_netdev_ops;
>> +     netdev->watchdog_timeo = TX_TIMEOUT;
>> +     netdev->ethtool_ops = &lan865x_ethtool_ops;
>> +     ret = register_netdev(netdev);
>> +     if (ret) {
>> +             if (netif_msg_probe(priv))
>> +                     dev_err(&spi->dev, "Register netdev failed (ret = %d)",
>> +                             ret);
>> +             goto error_netdev_register;
>> +     }
>> +
>> +     phy_start(priv->phydev);
>> +     return 0;
>> +
>> +error_netdev_register:
>> +error_set_mac:
>> +     phy_disconnect(priv->phydev);
>> +     mdiobus_unregister(priv->mdiobus);
>> +     mdiobus_free(priv->mdiobus);
>> +error_phy:
>> +err_macphy_config:
>> +     oa_tc6_deinit(priv->tc6);
>> +error_oa_tc6_init:
>> +     free_netdev(priv->netdev);
>> +     return ret;
>> +}
> 
> 
> ...