Hi Romain On Wed, Apr 24, 2024 at 11:06:20AM +0200, Romain Gantois wrote: > From: "Russell King (Oracle)" <rmk+kernel@xxxxxxxxxxxxxxx> > > Introduce a mechanism whereby platforms can create their PCS instances > prior to the network device being published to userspace, but after > some of the core stmmac initialisation has been completed. This means > that the data structures that platforms need will be available. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@xxxxxxxxxxxxxxx> > Reviewed-by: Maxime Chevallier <maxime.chevallier@xxxxxxxxxxx> > [rgantois: removed second parameters of new callbacks] > Signed-off-by: Romain Gantois <romain.gantois@xxxxxxxxxxx> > --- > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 14 ++++++++++++++ > include/linux/stmmac.h | 2 ++ > 2 files changed, 16 insertions(+) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index 59bf83904b62d..bee9c9ab31a88 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -7200,6 +7200,12 @@ static int stmmac_hw_init(struct stmmac_priv *priv) > if (ret) > return ret; > > + if (priv->plat->pcs_init) { > + ret = priv->plat->pcs_init(priv); > + if (ret) > + return ret; > + } > + Once again. There is a ready-to-use stmmac_xpcs_setup() method. Which is currently intended for the XPCS setups. Let's collect all the PCS-related stuff in a single place there. That will make code cleaner and easier to read. This was discussed on v3: https://lore.kernel.org/netdev/42chuecdt7dpgm6fcrtt2crifvv5hflmtnmdrw5fvk3r7pwjgu@hlcv56dbeosf/ You agreed to do that, but just ignored in result. I'll repeat what I said in v3: On Tue, 16 Apr 2024 16:41:33 +0300, Serge Semin wrote: > I am currently working on my Memory-mapped DW XPCS patchset cooking: > https://lore.kernel.org/netdev/20231205103559.9605-1-fancer.lancer@xxxxxxxxx/ > The changes in this series seems to intersect to what is/will be > introduced in my patchset. In particular as before I am going to > use the "pcs-handle" property for getting the XPCS node. If so what > about collecting PCS-related things in a single place. Like this: > > int stmmac_xpcs_setup(struct net_device *ndev) > { > ... > > if (priv->plat->pcs_init) { > return priv->plat->pcs_init(priv); /* Romain' part */ > } else if (fwnode_property_present(priv->plat->port_node, "pcs-handle")) { > /* My DW XPCS part */ > } else if (priv->plat->mdio_bus_data && priv->plat->mdio_bus_data->has_xpcs) { > /* Currently implemented procedure */ > } > > ... > } > > void stmmac_xpcs_clean(struct net_device *ndev) > { > ... > > if (priv->plat->pcs_exit) { > priv->plat->pcs_exit(priv); > return; > > } > > xpcs_destroy(priv->hw->xpcs); > priv->hw->xpcs = NULL; > } > > Please see the last two patches in my series: > https://lore.kernel.org/netdev/20231205103559.9605-16-fancer.lancer@xxxxxxxxx/ > https://lore.kernel.org/netdev/20231205103559.9605-17-fancer.lancer@xxxxxxxxx/ > as a reference of how the changes could be provided. You replied it was a good idea, but the function names should be renamed. That's not a problem. Just create a pre-requisite patch which does that. So the patch in the subject could be replaced with four subsequent patches: 1. Move the conditional XPCS-setup execution into the stmmac_xpcs_setup() method. This change is partly implemented here https://lore.kernel.org/netdev/20231205103559.9605-17-fancer.lancer@xxxxxxxxx/ 2. Rename stmmac_xpcs_setup() method to just stmmac_pcs_setup() as a preparation before adding the platform-specific PCS init()/exit() callbacks. 3. Introduce the PCS-cleanup method. You can pick it up from here, but use the stmmac_pcs_clean() name: https://lore.kernel.org/netdev/20231205103559.9605-16-fancer.lancer@xxxxxxxxx/ 4. Add pcc_init()/pcs_exit() callbacks as it's done in this patch but call them in the stmmac_pcs_setup()/stmmac_pcs_clean() methods instead of open-coding in the more generic stmmac_hw_init()/stmmac_hw_exit() functions. It doesn't look as that much hard thing to do, but will cause having a better readable code by providing a single coherent function for all PCS'es. -Serge(y) > /* Get the HW capability (new GMAC newer than 3.50a) */ > priv->hw_cap_support = stmmac_get_hw_features(priv); > if (priv->hw_cap_support) { > @@ -7282,6 +7288,12 @@ static int stmmac_hw_init(struct stmmac_priv *priv) > return 0; > } > > +static void stmmac_hw_exit(struct stmmac_priv *priv) > +{ > + if (priv->plat->pcs_exit) > + priv->plat->pcs_exit(priv); > +} > + > > [...]