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

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

On 2/8/24 00:15, 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.
> 

Thank you for your patch!, and sorry for long break in review process.

[...]

> 
> Signed-off-by: Ernesto Castellotti <ernesto@castellotti.net>
> ---
> v2: Fixed indentation/style issues as per review by Tony Nguyen
> v1: https://lore.kernel.org/intel-wired-lan/20240127174803.20793-1-ernesto@castellotti.net/
> ---
>   .../net/ethernet/intel/ixgbe/ixgbe_82599.c    |  4 ++-
>   .../net/ethernet/intel/ixgbe/ixgbe_ethtool.c  |  2 ++
>   drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c  | 32 ++++++++++++++++---
>   drivers/net/ethernet/intel/ixgbe/ixgbe_phy.h  |  2 ++
>   drivers/net/ethernet/intel/ixgbe/ixgbe_type.h |  3 ++
>   5 files changed, 38 insertions(+), 5 deletions(-)
> 

[...]

> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.h
> index ef72729d7c93..b5bc60916402 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.h
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.h
> @@ -17,6 +17,7 @@
>   #define IXGBE_SFF_1GBE_COMP_CODES	0x6
>   #define IXGBE_SFF_10GBE_COMP_CODES	0x3
>   #define IXGBE_SFF_CABLE_TECHNOLOGY	0x8
> +#define IXGBE_SFF_BITRATE_NOMINAL	0xC
>   #define IXGBE_SFF_CABLE_SPEC_COMP	0x3C
>   #define IXGBE_SFF_SFF_8472_SWAP		0x5C
>   #define IXGBE_SFF_SFF_8472_COMP		0x5E
> @@ -39,6 +40,7 @@
>   #define IXGBE_SFF_1GBASESX_CAPABLE		0x1
>   #define IXGBE_SFF_1GBASELX_CAPABLE		0x2
>   #define IXGBE_SFF_1GBASET_CAPABLE		0x8
> +#define IXGBE_SFF_BASEBX10_CAPABLE		0x64

this define looks off - perhaps it should be named
IXGBE_SFF_1GBASEBX_CAPABLE?

>   #define IXGBE_SFF_10GBASESR_CAPABLE		0x10
>   #define IXGBE_SFF_10GBASELR_CAPABLE		0x20
>   #define IXGBE_SFF_SOFT_RS_SELECT_MASK		0x8

[...]

code looks fine (as much as our ixgbe will go ;))

just one nitpick, so:
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>