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

[Vivien Didelot <vivien.didelot@gmail.com>]

On Tue,  5 Nov 2019 01:12:59 +0100, Andrew Lunn <andrew@lunn.ch> wrote:
> Add helpers to set/get the ATU statistics register.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/net/dsa/mv88e6xxx/global2.c | 20 ++++++++++++++++++++
>  drivers/net/dsa/mv88e6xxx/global2.h | 24 +++++++++++++++++++++++-
>  2 files changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/global2.c b/drivers/net/dsa/mv88e6xxx/global2.c
> index bdbb72fc20ed..14954d92c564 100644
> --- a/drivers/net/dsa/mv88e6xxx/global2.c
> +++ b/drivers/net/dsa/mv88e6xxx/global2.c
> @@ -280,6 +280,26 @@ int mv88e6xxx_g2_set_switch_mac(struct mv88e6xxx_chip *chip, u8 *addr)
>  	return err;
>  }
>  
> +/* Offset 0x0E: ATU Statistics */
> +
> +int mv88e6xxx_g2_atu_stats_set(struct mv88e6xxx_chip *chip, u16 kind, u16 bin)
> +{
> +	return mv88e6xxx_g2_write(chip, MV88E6XXX_G2_ATU_STATS,
> +				  kind | bin);
> +}
> +
> +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;

> +
>  /* Offset 0x0F: Priority Override Table */
>  
>  static int mv88e6xxx_g2_pot_write(struct mv88e6xxx_chip *chip, int pointer,
> diff --git a/drivers/net/dsa/mv88e6xxx/global2.h b/drivers/net/dsa/mv88e6xxx/global2.h
> index 42da4bca73e8..a308ca7a7da6 100644
> --- a/drivers/net/dsa/mv88e6xxx/global2.h
> +++ b/drivers/net/dsa/mv88e6xxx/global2.h
> @@ -113,7 +113,16 @@
>  #define MV88E6XXX_G2_SWITCH_MAC_DATA_MASK	0x00ff
>  
>  /* 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.

>  
>  /* Offset 0x0F: Priority Override Table */
>  #define MV88E6XXX_G2_PRIO_OVERRIDE		0x0f
> @@ -353,6 +362,8 @@ extern const struct mv88e6xxx_gpio_ops mv88e6352_gpio_ops;
>  
>  int mv88e6xxx_g2_scratch_gpio_set_smi(struct mv88e6xxx_chip *chip,
>  				      bool external);
> +int mv88e6xxx_g2_atu_stats_set(struct mv88e6xxx_chip *chip, u16 kind, u16 bin);
> +int mv88e6xxx_g2_atu_stats_get(struct mv88e6xxx_chip *chip);
>  
>  #else /* !CONFIG_NET_DSA_MV88E6XXX_GLOBAL2 */
>  
> @@ -515,6 +526,17 @@ static inline int mv88e6xxx_g2_device_mapping_write(struct mv88e6xxx_chip *chip,
>  	return -EOPNOTSUPP;
>  }
>  
> +static inline int mv88e6xxx_g2_atu_stats_set(struct mv88e6xxx_chip *chip,
> +					     u16 kind, u16 bin)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static inline int mv88e6xxx_g2_atu_stats_get(struct mv88e6xxx_chip *chip)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
>  #endif /* CONFIG_NET_DSA_MV88E6XXX_GLOBAL2 */
>  
>  #endif /* _MV88E6XXX_GLOBAL2_H */
> -- 
> 2.23.0
> 

Otherwise this looks good.


Thanks,

	Vivien