Re: [PATCH net-next 3/5] net: dsa: mv88e6xxx: global2: Expose ATU stats register

[Andrew Lunn <andrew@lunn.ch>]

> > +int mv88e6xxx_g2_atu_stats_get(struct mv88e6xxx_chip *chip)
> > +{
> > +	int err;
> > +	u16 val;
> > +
> > +	err = mv88e6xxx_g2_read(chip, MV88E6XXX_G2_ATU_STATS, &val);
> > +	if (err)
> > +		return err;
> > +
> > +	return val & MV88E6XXX_G2_ATU_STATS_MASK;
> > +}
> 
> I would use the logic commonly used in the mv88e6xxx driver for
> functions that may fail, returning only an error code and taking a
> pointer to the correct type as argument, so that we do as usual:
> 
>     err = mv88e6xxx_g2_atu_stats_get(chip, &val);
>     if (err)
>         return err;

Well, actually. Take a look at the next patch. This function gets
changed there. After you reviewed the first devlink patchset adding
control of the ATU algorithm, i went through this patchset and applied
the same comments to it. So i change the implementation to how you
actually wanted it. But i messed up my git commands and that changed
ended up in the next patch :-(

> >  /* Offset 0x0E: ATU Stats Register */
> > -#define MV88E6XXX_G2_ATU_STATS		0x0e
> > +#define MV88E6XXX_G2_ATU_STATS				0x0e
> > +#define MV88E6XXX_G2_ATU_STATS_BIN_0			(0x0 << 14)
> > +#define MV88E6XXX_G2_ATU_STATS_BIN_1			(0x1 << 14)
> > +#define MV88E6XXX_G2_ATU_STATS_BIN_2			(0x2 << 14)
> > +#define MV88E6XXX_G2_ATU_STATS_BIN_3			(0x3 << 14)
> > +#define MV88E6XXX_G2_ATU_STATS_MODE_ALL			(0x0 << 12)
> > +#define MV88E6XXX_G2_ATU_STATS_MODE_ALL_DYNAMIC		(0x1 << 12)
> > +#define MV88E6XXX_G2_ATU_STATS_MODE_FID_ALL		(0x2 << 12)
> > +#define MV88E6XXX_G2_ATU_STATS_MODE_FID_ALL_DYNAMIC	(0x3 << 12)
> > +#define MV88E6XXX_G2_ATU_STATS_MASK			0x0fff
> 
> Please use the 0x1234 format for these 16-bit registers.

Ug, no. That is a lot less readable. The datasheet describes there
fields in terms of bit offsets in the registers. Bit 14 and 15 for the
bin, bits 12 and 13 for the mode. You can clearly see that when using
value and shift representation. 0x0200 is much harder to read, and
much more error prone.

      Andrew