RE: [PATCH v1 net-next] net: stmmac: enable clause 45 mdio support

[Voon, Weifeng]

> I think there is too much passing variables around by reference than by
> value, to make this code easy to understand.
> 
> Maybe a better structure would be
> 
> static int stmmac_mdion_c45_read(struct stmmac_priv *priv, int phyaddr,
> int phyreg) {
> 
> 	unsigned int reg_shift = priv->hw->mii.reg_shift;
> 	unsigned int reg_mask = priv->hw->mii.reg_mask;
> 	u32 mii_addr_val, mii_data_val;
> 
> 	mii_addr_val = MII_GMAC4_C45E |
>                        ((phyreg >> MII_DEVADDR_C45_SHIFT) << reg_shift)
> & reg_mask;
>         mii_data_val = (phyreg & MII_REGADDR_C45_MASK) <<
> MII_GMAC4_REG_ADDR_SHIFT;
> 
> 	writel(mii_data_val, priv->ioaddr + priv->hw->mii_data);
> 	writel(mii_addr_val, priv->ioaddr + priv->hw->mii_addrress);
> 
> 	return (int)readl(priv->ioaddr + mii_data) & MII_DATA_MASK;
> }
> 
> static int stmmac_mdio_read(struct mii_bus *bus, int phyaddr, int phyreg)
> {
> 
> ...
> 	if (readl_poll_timeout(priv->ioaddr + mii_address, v, !(v &
> MII_BUSY),
>  	   		      100, 10000))
>  		return -EBUSY;
> 
>       if (priv->plat->has_gmac4 && phyreg & MII_ADDR_C45)
>       	return stmmac_mdio_c45_read(priv, phyaddr, phyreg);
> 
> 	Andrew

Both c45 read/write needs to set c45 enable bit(MII_ADDR_C45) in mii_adrress
and set the register address in mii_data. Besides this, the whole programming
flow will be the same as c22. With the current design, user can easily know
that the different between c22 and c45 is just in stmmac_mdio_c45_setup(). 

If the community prefers readability, I will suggest to do the c45 setup in
both stmmac_mdio_read() and stmmac_mdio_write() 's if(C45) condition rather
than splitting into 2 new c45_read() and c45_write() functions.     

static int stmmac_mdio_read(struct mii_bus *bus, int phyaddr, int phyreg)
{

...
	if (phyreg & MII_ADDR_C45)
       *val |= MII_GMAC4_C45E;
       *val &= ~reg_mask;
       *val |= ((phyreg >> MII_DEVADDR_C45_SHIFT) << reg_shift) & reg_mask;

       *data |= (phyreg & MII_REGADDR_C45_MASK) << MII_GMAC4_REG_ADDR_SHIFT;

Weifeng