Re: [RFC][PATCH 1/2] ethtool: Add BroadRReach Master/Slave PHY tunable

[Andrew Lunn <andrew@lunn.ch>]

On Wed, Mar 25, 2020 at 11:17:35AM +0100, Marek Vasut wrote:
> Add a PHY tunable to select BroadRReach PHY Master/Slave mode.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Cc: Jean Delvare <jdelvare@suse.com>
> Cc: netdev@vger.kernel.org
> ---
>  include/uapi/linux/ethtool.h | 1 +
>  net/core/ethtool.c           | 2 ++
>  2 files changed, 3 insertions(+)
> 
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index dc69391d2bba..ebe658804ef1 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -259,6 +259,7 @@ struct ethtool_tunable {
>  enum phy_tunable_id {
>  	ETHTOOL_PHY_ID_UNSPEC,
>  	ETHTOOL_PHY_DOWNSHIFT,
> +	ETHTOOL_PHY_BRR_MODE,
>  	/*
>  	 * Add your fresh new phy tunable attribute above and remember to update
>  	 * phy_tunable_strings[] in net/core/ethtool.c
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index 09d828a6a173..553f3d0e2624 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -133,6 +133,7 @@ static const char
>  phy_tunable_strings[__ETHTOOL_PHY_TUNABLE_COUNT][ETH_GSTRING_LEN] = {
>  	[ETHTOOL_ID_UNSPEC]     = "Unspec",
>  	[ETHTOOL_PHY_DOWNSHIFT]	= "phy-downshift",
> +	[ETHTOOL_PHY_BRR_MODE]	= "phy-broadrreach-mode",
>  };

>  
>  static int ethtool_get_features(struct net_device *dev, void __user *useraddr)
> @@ -2524,6 +2525,7 @@ static int ethtool_phy_tunable_valid(const struct ethtool_tunable *tuna)
>  {
>  	switch (tuna->id) {
>  	case ETHTOOL_PHY_DOWNSHIFT:
> +	case ETHTOOL_PHY_BRR_MODE:
>  		if (tuna->len != sizeof(u8) ||
>  		    tuna->type_id != ETHTOOL_TUNABLE_U8)
>  			return -EINVAL;

Hi Marek

As far as i understand, there are only two settings. Master and
Slave. So you can make the validation here more specific.

I would also add some #defines for this in
include/uapi/linux/ethtool.h so it is clear what value represents
master and what is slave.

       Andrew