On 11/11/21 1:04 AM, Wells Lu wrote:

Add driver for Sunplus SP7021.

Signed-off-by: Wells Lu <redacted>
---

[snip]

+u32 mdio_read(struct sp_mac *mac, u32 phy_id, u16 regnum)
+{
+	int ret;
+
+	ret = hal_mdio_access(mac, MDIO_READ_CMD, phy_id, regnum, 0);
+	if (ret < 0)
+		return -EOPNOTSUPP;
+
+	return ret;
+}
+
+u32 mdio_write(struct sp_mac *mac, u32 phy_id, u32 regnum, u16 val)
+{
+	int ret;
+
+	ret = hal_mdio_access(mac, MDIO_WRITE_CMD, phy_id, regnum, val);
+	if (ret < 0)
+		return -EOPNOTSUPP;
+
+	return 0;
+}

You should not be exposing these functions, if you do, that means
another part of your code performs MDIO bus read/write operations
without using the appropriate layer, so no.

+
+static int mii_read(struct mii_bus *bus, int phy_id, int regnum)
+{
+	struct sp_mac *mac = bus->priv;
+
+	return mdio_read(mac, phy_id, regnum);
+}
+
+static int mii_write(struct mii_bus *bus, int phy_id, int regnum, u16 val)
+{
+	struct sp_mac *mac = bus->priv;
+
+	return mdio_write(mac, phy_id, regnum, val);
+}
+
+u32 mdio_init(struct platform_device *pdev, struct net_device *ndev)

Those function names need to be prefixed with sp_ to denote the driver
local scope, this applies for your entire patch set.

[snip]

Please scope your functions better, and name them sp_mdio_read, etc.
because mdio_read() is way too generic. Also, can you please follow the
same prototype as what include/linux/mdio.h has for the mdiobus->read
and ->write calls, that is phy_id is int, regnum is u32, etc.

Does your MAC fully auto-configure based on the PHY's link parameters,
if so, how does it do it? You most certainly need to act on duplex
changes, or speed changes no?

+
+int sp_phy_probe(struct net_device *ndev)
+{
+	struct sp_mac *mac = netdev_priv(ndev);
+	struct phy_device *phydev;
+	int i;
+
+	phydev = of_phy_connect(ndev, mac->phy_node, mii_linkchange,
+				0, mac->phy_mode);
+	if (!phydev) {
+		netdev_err(ndev, "\"%s\" failed to connect to phy!\n", ndev->name);
+		return -ENODEV;
+	}
+
+	for (i = 0; i < sizeof(phydev->supported) / sizeof(long); i++)
+		phydev->advertising[i] = phydev->supported[i];
+
+	phydev->irq = PHY_MAC_INTERRUPT;
+	mac->phy_dev = phydev;
+
+	// Bug workaround:
+	// Flow-control of phy should be enabled. MAC flow-control will refer
+	// to the bit to decide to enable or disable flow-control.
+	mdio_write(mac, mac->phy_addr, 4, mdio_read(mac, mac->phy_addr, 4) | (1 << 10));

This is a layering violation, and you should not be doing those things
here, if you need to advertise flow control, then please set
ADVERTISE_PAUSE_CAP and/or ADVERTISE_PAUSE_ASYM accordingly, see whether
phy_set_asym_pause() can do what you need it to.

+
+	return 0;
+}
+
+void sp_phy_start(struct net_device *ndev)
+{
+	struct sp_mac *mac = netdev_priv(ndev);
+
+	if (mac->phy_dev)
+		phy_start(mac->phy_dev);
+}
+
+void sp_phy_stop(struct net_device *ndev)
+{
+	struct sp_mac *mac = netdev_priv(ndev);
+
+	if (mac->phy_dev)
+		phy_stop(mac->phy_dev);
+}
+
+void sp_phy_remove(struct net_device *ndev)
+{
+	struct sp_mac *mac = netdev_priv(ndev);
+
+	if (mac->phy_dev) {
+		phy_disconnect(mac->phy_dev);
+		mac->phy_dev = NULL;
+	}

The net_device structure already contains a phy_device pointer, you
don't need to have one in your sp_mac structure, too.
-- 
Florian