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;
>> +}
>
>
> ...
