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@xxxxxxxxxxxxx> > > 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, ®val, 1); >> + if (ret) >> + return ret; >> + >> + regval = CCS_Q0_RX_CFG_32; >> + ret = oa_tc6_write_register(priv->tc6, CCS_Q0_RX_CFG, ®val, 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; >> +} > > > ...