On Sun, Jul 19, 2020 at 2:16 PM Michal Kubecek <mkube...@suse.cz> wrote:
>
> On Thu, Jul 16, 2020 at 10:55:26AM -0700, Chris Healy wrote:
> > From: Andrew Lunn <and...@lunn.ch>
> >
> > In addition to the port registers, the device can provide the
> > SERDES/PCS registers. Dump these, and for a few of the important
> > SGMII/1000Base-X registers decode the bits.
> >
> > Signed-off-by: Andrew Lunn <and...@lunn.ch>
> > Signed-off-by: Chris Healy <cphe...@gmail.com>
> > ---
> [...]
> > + case 32 + 0:
> > + REG(reg - 32, "Fiber Control", val);
>
> Could you give these "32" (and similar below) a name?
Yep, will do in v2.
>
> [...]
> > @@ -667,6 +850,17 @@ static int dsa_mv88e6xxx_dump_regs(struct ethtool_regs
> > *regs)
> > else
> > REG(i, "", data[i]);
> >
> > + /* Dump the SERDES registers, if provided */
> > + if (regs->len > 32 * 2) {
>
> sizeof(u16) would be easier to read, IMHO
Agreed, will do in v2.
>
> > + printf("\n%s Switch Port SERDES Registers\n", sw->name);
> > + printf("-------------------------------------\n");
> > + for (i = 32; i < regs->len / 2; i++)
> > + if (sw->dump)
> > + sw->dump(i, data[i]);
> > + else
> > + REG(i, "", data[i]);
>
> In the dump handler above you subtract 32 (offset of SERDES registers,
> IIUC) from register number but in the generic branch you don't, this
> seems inconsistent.
You are correct. I tested this code path and it was reporting
incorrectly. Fix coming in v2.
>
> Michal
>
> > + }
> > +
> > return 0;
> > }
> >
> > --
> > 2.21.3
> >
