Re: [PATCH net-next 1/4] net: dsa: mv88e6xxx: Create API to read a single stat counter

[Vladimir Oltean <olteanv@gmail.com>]

On Fri, Dec 01, 2023 at 01:58:09PM +0100, Tobias Waldekranz wrote:
> This change contains no functional change. We simply push the hardware
> specific stats logic to a function reading a single counter, rather
> than the whole set.
> 
> This is a preparatory change for the upcoming standard ethtool
> statistics support (i.e. "eth-mac", "eth-ctrl" etc.).
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> ---

I think I like this patch set. Some nitpicks below.

> -static int mv88e6320_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
> -				     uint64_t *data)
> +static int mv88e6xxx_stats_get_stat(struct mv88e6xxx_chip *chip, int port,
> +				    const struct mv88e6xxx_hw_stat *stat,
> +				    uint64_t *data)
>  {
> -	return mv88e6xxx_stats_get_stats(chip, port, data,
> -					 STATS_TYPE_BANK0 | STATS_TYPE_BANK1,
> -					 MV88E6XXX_G1_STATS_OP_BANK_1_BIT_9,
> -					 MV88E6XXX_G1_STATS_OP_HIST_RX_TX);
> +	int ret = 0;
> +
> +	if (chip->info->ops->stats_get_stat) {
> +		mv88e6xxx_reg_lock(chip);
> +		ret = chip->info->ops->stats_get_stat(chip, port, stat, data);
> +		mv88e6xxx_reg_unlock(chip);
> +	}

There is no chip->info->ops which does not provide stats_get_stat().
As a separate change, I suppose you could drop the "if".

> +
> +	return ret;
>  }
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
> index 44383a03ef2f..b0dfbcae0be9 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.h
> +++ b/drivers/net/dsa/mv88e6xxx/chip.h
> @@ -574,8 +585,9 @@ struct mv88e6xxx_ops {
>  	/* Return the number of strings describing statistics */
>  	int (*stats_get_sset_count)(struct mv88e6xxx_chip *chip);
>  	int (*stats_get_strings)(struct mv88e6xxx_chip *chip,  uint8_t *data);
> -	int (*stats_get_stats)(struct mv88e6xxx_chip *chip,  int port,
> -			       uint64_t *data);
> +	int (*stats_get_stat)(struct mv88e6xxx_chip *chip,  int port,
> +			      const struct mv88e6xxx_hw_stat *stat,
> +			      uint64_t *data);

Since you are touching this function prototype anyway, I guess you could
change its return type. Int assumes it can also return a negative error
code, which it doesn't. Maybe size_t to denote that it returns an unsigned
count of statistics?

Also, there is an extraneous space before the "int port" argument which
you could fix up now.

>  	int (*set_cpu_port)(struct mv88e6xxx_chip *chip, int port);
>  	int (*set_egress_port)(struct mv88e6xxx_chip *chip,
>  			       enum mv88e6xxx_egress_direction direction,