[U-Boot] [PATCH v2 1/6] net: phy: mv88e61xx: rework to enable detection of 88E6071 devices

[Anatolij Gustschin]

On Tue, 23 Jul 2019 04:08:23 +0000
Joe Hershberger joe.hershberger at ni.com wrote:
...
> > -#define DEVADDR_PORT(p)                        (0x10 + (p))
> > +#define DEVADDR_PORT(p)                        (priv->port_reg_base + (p))  
> 
> This is an obtuse macro. It should not reference a local variable
> internally. If you want to use priv, pass it as a parameter.

Okay.

> >  struct mv88e61xx_phy_priv {
> >         struct mii_dev *mdio_bus;
> >         int smi_addr;
> >         int id;
> > +       int port_count;
> > +       int port_reg_base;
> > +       u8 global1;
> > +       u8 global2;  
> 
> I think this could stand some commenting. global what? Why 2?

I'll add some comments here.

...
> >  static int mv88e61xx_phy_wait(struct phy_device *phydev)
> >  {
> > +       struct mv88e61xx_phy_priv *priv = phydev->priv;
> >         int val;
> >         u32 timeout = 100;
> >
> >         do {
> > -               val = mv88e61xx_reg_read(phydev, DEVADDR_GLOBAL_2,
> > +               val = mv88e61xx_reg_read(phydev, priv->global2,  
> 
> Probably just use phydev->priv->global2 instead of the local variable.

Done in v3.

...
> >  static int mv88e61xx_port_read(struct phy_device *phydev, u8 port, u8 reg)
> >  {
> > +       struct mv88e61xx_phy_priv *priv = phydev->priv;  
> 
> Huh? When casually read, this seems useless.

Okay, I'll remove DEVADDR_PORT() macro.
 
...
> >  static int mv88e61xx_port_write(struct phy_device *phydev, u8 port, u8 reg,
> >                                                                 u16 val)
> >  {
> > +       struct mv88e61xx_phy_priv *priv = phydev->priv;  
> 
> Huh? Ditto.

Will fix it.

...
> > +static void mv88e61xx_priv_reg_offs_pre_init(struct mv88e61xx_phy_priv *priv)
> > +{
> > +       /*
> > +        * Initial 'port_reg_base' value must be in the port register,
> > +        * map and the global_N register offsets must be correct,  
> 
> They are globalN, not global_N. What is the map that this referenced?
> Please reword.

Okay, done in v3.

> > +        * otherwise detection of switch ID won't work!  
> 
> That seems likely... but this looks very magic. Please document it a
> little better and if possible reference any public datasheets or
> reference manuals.

Will add more comments here. The datasheets or manuals are all under
NDA, I can't add references here.

> > +        */
> > +#ifndef CONFIG_MV88E61XX_88E6020_FAMILY
> > +       priv->global1 = 0x1B;
> > +       priv->global2 = 0x1C;
> > +       priv->port_reg_base = 0x10;
> > +#else
> > +       priv->global1 = 0x0F;
> > +       priv->global2 = 0x07;
> > +       priv->port_reg_base = 0x08;
> > +#endif  
> 
> I think it would be much cleaner if the contents of this function and
> the following switch case in the next function were implemented as
> driver data selections in the udevice_id structure and then used from
> there.

The problem is that this offset pre-initialisation is also required
before the actual driver probing (i.e. calls of get_phy_id() from PHY
framework).

...
> > @@ -1068,6 +1123,8 @@ int get_phy_id(struct mii_dev *bus, int smi_addr, int devad, u32 *phy_id)
> >         temp_phy.priv = &temp_priv;
> >         temp_mii.priv = &temp_phy;
> >
> > +       mv88e61xx_priv_reg_offs_pre_init(&temp_priv);  
> 
> Ick.

will add a comment why it is required here.

--
Anatolij