Re: [PATCH net-next v2 1/5] net: stmmac: add select_pcs() platform method

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Fri, Jun 14, 2024 at 07:38:40PM +0300, Serge Semin wrote:
> Hi Russell
> 
> On Thu, Jun 13, 2024 at 11:36:06AM +0100, Russell King (Oracle) wrote:
> > Allow platform drivers to provide their logic to select an appropriate
> > PCS.
> > 
> > Tested-by: Romain Gantois <romain.gantois@bootlin.com>
> > Reviewed-by: Romain Gantois <romain.gantois@bootlin.com>
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 7 +++++++
> >  include/linux/stmmac.h                            | 4 +++-
> >  2 files changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index bbedf2a8c60f..302aa4080de3 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -949,6 +949,13 @@ static struct phylink_pcs *stmmac_mac_select_pcs(struct phylink_config *config,
> >  						 phy_interface_t interface)
> >  {
> >  	struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev));
> > +	struct phylink_pcs *pcs;
> > +
> > +	if (priv->plat->select_pcs) {
> > +		pcs = priv->plat->select_pcs(priv, interface);
> > +		if (!IS_ERR(pcs))
> > +			return pcs;
> > +	}
> >  
> >  	if (priv->hw->xpcs)
> >  		return &priv->hw->xpcs->pcs;
> > diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
> > index 8f0f156d50d3..9c54f82901a1 100644
> > --- a/include/linux/stmmac.h
> > +++ b/include/linux/stmmac.h
> > @@ -13,7 +13,7 @@
> >  #define __STMMAC_PLATFORM_DATA
> >  
> >  #include <linux/platform_device.h>
> > -#include <linux/phy.h>
> > +#include <linux/phylink.h>
> >  
> >  #define MTL_MAX_RX_QUEUES	8
> >  #define MTL_MAX_TX_QUEUES	8
> > @@ -271,6 +271,8 @@ struct plat_stmmacenet_data {
> >  	void (*dump_debug_regs)(void *priv);
> 
> >  	int (*pcs_init)(struct stmmac_priv *priv);
> >  	void (*pcs_exit)(struct stmmac_priv *priv);
> > +	struct phylink_pcs *(*select_pcs)(struct stmmac_priv *priv,
> > +					  phy_interface_t interface);
> 
> Just a small note/nitpick. We've got pcs_init() and pcs_exit()
> callbacks. Both of them have the pcs_ prefix followed by the action
> verb. What about using the same notation for the PCS-select method,
> using the plat_stmmacenet_data::pcs_select() callback-name instead?

From phylink's perspective, it's not part of the PCS, it's something
that the MAC does.

The interface passed in to mac_select_pcs() so so the MAC code can
decide which PCS (if it has many to choose from) will be used for
the specified interface mode to either a PHY or other device
connected to the netdev. It isn't a PCS operation, it's an
operation that returns an appropriate PCS.

So, I want to keep "select_pcs" as at least a suffix, because it
is selecting a PCS. Eventually, I would like to see the stmmac
implementations check the "interface" passed to it before deciding
whether to return a PCS or not - thus how it's intended to be
implemented.

"pcs_select" seems to make it sound like it's part of a PCS
implementation, which as I've explained above, it isn't. It also
doesn't convey that it's selecting a PCS based on its arguments.

I'd also like to keep the ability to grep for "select_pcs" to
find implementations and not have complex grep expressions to
find whatever the driver has called it! To that end, I much prefer
that drivers that name sub-implementations the same way that
phylink names them to make grepping easier.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!