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

[Marek Vasut <marex@denx.de>]

On 3/25/20 2:43 PM, Andrew Lunn wrote:
> 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.

I'll let Oleksij take this series over, since he's working with the
TJA1102 . In the meantime, I'll continue on the KS8851.

Thanks for the feedback !