Re: [Intel-wired-lan] [PATCH net-next v2] ice: Allow 100M speeds for some devices

[Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>]

> Am 28.07.22 um 21:23 schrieb Mikael Barsehyan:
>> From: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
>>
>> For certain devices, 100M speeds are supported. Do not mask off
>> 100M speed for these devices.
> 
> Please list the devices in the commit message.

So you're asking that device IDs be listed in the commit message? Seems 
a bit redundant seeing as they can be inferred from the code quite 
easily. Let me know if you feel otherwise.

> 
> Please also describe the implementation in the commit message.

My understanding is that commit messages should list the what and the 
why, not the how.

> 
> How did you test this?

For ethtool <ethX>, 100M will not be listed as a link mode for devices 
that don't support it.

> 
>> Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
>> Co-developed-by: Chinh T Cao <chinh.t.cao@intel.com>
>> Signed-off-by: Chinh T Cao <chinh.t.cao@intel.com>
>> Signed-off-by: Mikael Barsehyan <mikael.barsehyan@intel.com>
>> ---
>>   drivers/net/ethernet/intel/ice/ice_common.c  | 20 ++++++++++++++++++++
>>   drivers/net/ethernet/intel/ice/ice_common.h  |  1 +
>>   drivers/net/ethernet/intel/ice/ice_ethtool.c | 11 +++++++----
>>   3 files changed, 28 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_common.c 
>> b/drivers/net/ethernet/intel/ice/ice_common.c
>> index 05a4acfbdd1d..010385e67665 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_common.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
>> @@ -2775,6 +2775,26 @@ ice_aq_set_port_params(struct ice_port_info 
>> *pi, bool double_vlan,
>>       return ice_aq_send_cmd(hw, &desc, NULL, 0, cd);
>>   }
>> +/**
>> + * ice_is_100m_speed_supported
>> + * @hw: pointer to the HW struct
>> + *
>> + * returns true if 100M speeds are supported by the device,
>> + * false otherwise.
>> + */
>> +bool ice_is_100m_speed_supported(struct ice_hw *hw)
>> +{
>> +    switch (hw->device_id) {
>> +    case ICE_DEV_ID_E822C_SGMII:
>> +    case ICE_DEV_ID_E822L_SGMII:
>> +    case ICE_DEV_ID_E823L_1GBE:
>> +    case ICE_DEV_ID_E823C_SGMII:
>> +        return true;
>> +    default:
>> +        return false;
>> +    }
> 
> Is there no way to determine this during run-time without maintaining a 
> list?

No.

> 
>> +}
>> +
>>   /**
>>    * ice_get_link_speed_based_on_phy_type - returns link speed
>>    * @phy_type_low: lower part of phy_type
>> diff --git a/drivers/net/ethernet/intel/ice/ice_common.h 
>> b/drivers/net/ethernet/intel/ice/ice_common.h
>> index a74df1d3a002..2734296bdd3b 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_common.h
>> +++ b/drivers/net/ethernet/intel/ice/ice_common.h
>> @@ -205,6 +205,7 @@ ice_aq_set_gpio(struct ice_hw *hw, u16 
>> gpio_ctrl_handle, u8 pin_idx, bool value,
>>   int
>>   ice_aq_get_gpio(struct ice_hw *hw, u16 gpio_ctrl_handle, u8 pin_idx,
>>           bool *value, struct ice_sq_cd *cd);
>> +bool ice_is_100m_speed_supported(struct ice_hw *hw);
> 
> I’d name it `is_100mbits_supported`.

Naming is a bit subjective I suppose. As long as the function name is 
sensible and readable, it's fine.

To each their own I suppose. :-)

Ani
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan