Re: [Intel-wired-lan] [PATCH iwl-next] ixgbe: Add 1000BASE-BX support

[Tony Nguyen <anthony.l.nguyen@intel.com>]

+ Piotr

On 1/27/2024 9:48 AM, Ernesto Castellotti wrote:
> Added support for 1000BASE-BX, i.e. Gigabit Ethernet over single strand
> of single-mode fiber).
> The initialization of a 1000BASE-BX SFP is the same as 1000BASE-SX/LX
> with the only difference that the Bit Rate Nominal Value must be
> checked to make sure it is a Gigabit Ethernet transceiver, as described
> by the SFF-8472 specification.
> 

Some nits on the patch...

> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c
> @@ -1539,6 +1539,7 @@ s32 ixgbe_identify_sfp_module_generic(struct ixgbe_hw *hw)
>   	u8 oui_bytes[3] = {0, 0, 0};
>   	u8 cable_tech = 0;
>   	u8 cable_spec = 0;
> +	u8 bitrate_nominal = 0;

This should be reverse xmas tree (longest line to shortest) so this 
should go under oui_bytes.

>   	u16 enforce_sfp = 0;
>   
>   	if (hw->mac.ops.get_media_type(hw) != ixgbe_media_type_fiber) {
> @@ -1577,6 +1578,13 @@ s32 ixgbe_identify_sfp_module_generic(struct ixgbe_hw *hw)
>   					     IXGBE_SFF_CABLE_TECHNOLOGY,
>   					     &cable_tech);
>   
> +	if (status)
> +		goto err_read_i2c_eeprom;

No newline between function call and error check please.

> +
> +	status = hw->phy.ops.read_i2c_eeprom(hw,
> +					     IXGBE_SFF_BITRATE_NOMINAL,
> +					     &bitrate_nominal);
> +
>   	if (status)
>   		goto err_read_i2c_eeprom;

Same here.

>   
> @@ -1659,6 +1667,17 @@ s32 ixgbe_identify_sfp_module_generic(struct ixgbe_hw *hw)
>   			else
>   				hw->phy.sfp_type =
>   					ixgbe_sfp_type_1g_lx_core1;
> +		// Support only 1000BASE-BX10, checking the Bit Rate Nominal Value as per SFF-8472
> +		// by convention 1.25 Gb/s should be rounded up to 0Dh (13 in units of 100 MBd)
> +		// for Ethernet 1000BASE-X

This isn't the correct commenting style for kernel or netdev [1]. Also, 
please wrap to 80 chars.

> +		} else if ((comp_codes_1g & IXGBE_SFF_BASEBX10_CAPABLE) &&
> +			   (bitrate_nominal == 0xD)) {
> +			if (hw->bus.lan_id == 0)
> +				hw->phy.sfp_type =
> +					ixgbe_sfp_type_1g_bx_core0;
> +			else
> +				hw->phy.sfp_type =
> +					ixgbe_sfp_type_1g_bx_core1;
>   		} else {
>   			hw->phy.sfp_type = ixgbe_sfp_type_unknown;
>   		}

Thanks,
Tony

[1] 
https://docs.kernel.org/process/maintainer-netdev.html#multi-line-comments