Re: [RFC PATCH v3 net-next 09/10] net: dsa: ocelot: felix: add support for VSC75XX control over SPI
From: Vladimir Oltean <olteanv@gmail.com>
Date: 2021-08-17 09:41:17
Also in:
lkml, netdev
On Sun, Aug 15, 2021 at 01:41:49PM -0700, Colin Foster wrote:
I also came across some curious code in Seville where it is callocing a struct phy_device * array instead of struct lynx_pcs *. I'm not sure if that's technically a bug or if the thought is "a pointer array is a pointer array."
git blame will show you that it is a harmless leftover of commit
588d05504d2d ("net: dsa: ocelot: use the Lynx PCS helpers in Felix and
Seville"). Before that patch, the pcs was a struct phy_device.
quoted hunk ↗ jump to hunk
@@ -1062,12 +1062,12 @@ static void vsc9953_mdio_bus_free(struct ocelot *ocelot) int port; for (port = 0; port < ocelot->num_phys_ports; port++) { - struct lynx_pcs *pcs = felix->pcs[port]; + struct phylink_pcs *pcs = felix->pcs[port]; if (!pcs) continue; - mdio_device_free(pcs->mdio); + mdio_device_free(lynx_pcs_get_mdio(pcs));
Don't really have a better suggestion than lynx_pcs_get_mdio.
quoted hunk ↗ jump to hunk
lynx_pcs_destroy(pcs); } felix_mdio_bus_free(ocelot);diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c index ccaf7e35abeb..484f0d4efefe 100644 --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c@@ -270,10 +270,11 @@ static int dpaa2_pcs_create(struct dpaa2_mac *mac, static void dpaa2_pcs_destroy(struct dpaa2_mac *mac) { - struct lynx_pcs *pcs = mac->pcs; + struct phylink_pcs *pcs = mac->pcs; if (pcs) { - struct device *dev = &pcs->mdio->dev; + struct mdio_device *mdio = lynx_get_mdio_device(pcs); + struct device *dev = &mdio->dev; lynx_pcs_destroy(pcs); put_device(dev);
Ideally dpaa2 would call mdio_device_free too, just like the others.
quoted hunk ↗ jump to hunk
mac->pcs = NULL;@@ -336,7 +337,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac) mac->phylink = phylink; if (mac->pcs) - phylink_set_pcs(mac->phylink, &mac->pcs->pcs); + phylink_set_pcs(mac->phylink, mac->pcs); err = phylink_of_phy_connect(mac->phylink, dpmac_node, 0); if (err) {diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c index 31274325159a..cc2ca51ac984 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c@@ -823,7 +823,7 @@ static int enetc_imdio_create(struct enetc_pf *pf) { struct device *dev = &pf->si->pdev->dev; struct enetc_mdio_priv *mdio_priv; - struct lynx_pcs *pcs_lynx; + struct phylink_pcs *pcs_phylink; struct mdio_device *pcs;
Agree with Russell's suggestion to replace "pcs" with "mdiodev" wherever it refers to a struct mdio_device. Likely as a separate patch.
quoted hunk ↗ jump to hunk
struct mii_bus *bus; int err;@@ -341,13 +355,13 @@ struct lynx_pcs *lynx_pcs_create(struct mdio_device *mdio) lynx_pcs->pcs.ops = &lynx_pcs_phylink_ops; lynx_pcs->pcs.poll = true; - return lynx_pcs; + return lynx_to_phylink_pcs(lynx_pcs);
I would probably write another patch to convert all occurrences of "struct lynx_pcs" variables to the same naming scheme. Currently we have "lynx", "pcs", "lynx_pcs" only within the pcs-lynx.c file itself. "lynx" seems to be the predominant name so all others could be replaced with that too.
}
EXPORT_SYMBOL(lynx_pcs_create);
-void lynx_pcs_destroy(struct lynx_pcs *pcs)
+void lynx_pcs_destroy(struct phylink_pcs *pcs)
{
- kfree(pcs);
+ kfree(phylink_pcs_to_lynx(pcs));I would perhaps do this in two stages struct lynx_pcs *lynx = phylink_pcs_to_lynx(pcs); kfree(lynx);
quoted hunk ↗ jump to hunk
} EXPORT_SYMBOL(lynx_pcs_destroy);diff --git a/include/linux/pcs-lynx.h b/include/linux/pcs-lynx.h index a6440d6ebe95..5712cc2ce775 100644 --- a/include/linux/pcs-lynx.h +++ b/include/linux/pcs-lynx.h@@ -9,13 +9,10 @@ #include <linux/mdio.h> #include <linux/phylink.h> -struct lynx_pcs { - struct phylink_pcs pcs; - struct mdio_device *mdio; -};
Good that this structure is no longer exposed.
+struct mdio_device *lynx_get_mdio_device(struct phylink_pcs *pcs); -struct lynx_pcs *lynx_pcs_create(struct mdio_device *mdio); +struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio); -void lynx_pcs_destroy(struct lynx_pcs *pcs); +void lynx_pcs_destroy(struct phylink_pcs *pcs);
We don't want the few phylink_pcs drivers going in different directions, so we should modify pcs-xpcs.c too such that it no longer exposes struct dw_xpcs to the outside world. I think I hid most of that away already, and grepping for "xpcs->" in drivers/net/dsa and drivers/net/ethernet, I only see xpcs->mdiodev and xpcs->pcs being accessed, so converting khat should be a walk in the park. Anyway, I would focus for now on getting the ocelot hardware to work and writing the phylink_pcs driver for that. That is one part where I can't help a lot with.