Hi Krzysztof,
On 12/09/2024 9:38, Krzysztof Olędzki wrote:
> Use SFF8024 constants defined in linux/sfp.h instead of private ones.
>
> Make mlx4_en_get_module_info() and mlx5e_get_module_info() to look
> as close as possible to each other.
Please split mlx4 and mlx5 changes to two patches.
>
> Simplify the logic for selecting SFF_8436 vs SFF_8636.
>
> Signed-off-by: Krzysztof Piotr Oledzki <ole@xxxxxx>
> ---
> .../net/ethernet/mellanox/mlx4/en_ethtool.c | 33 ++++++++++---------
> drivers/net/ethernet/mellanox/mlx4/port.c | 9 ++---
> .../ethernet/mellanox/mlx5/core/en_ethtool.c | 28 +++++++++-------
> .../net/ethernet/mellanox/mlx5/core/port.c | 9 ++---
> include/linux/mlx4/device.h | 7 ----
> include/linux/mlx5/port.h | 8 -----
> 6 files changed, 44 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
> index cd17a3f4faf8..4c985d62af12 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
> @@ -40,6 +40,7 @@
> #include <net/ip.h>
> #include <linux/bitmap.h>
> #include <linux/mii.h>
> +#include <linux/sfp.h>
>
> #include "mlx4_en.h"
> #include "en_port.h"
> @@ -2029,33 +2030,35 @@ static int mlx4_en_get_module_info(struct net_device *dev,
>
> /* Read first 2 bytes to get Module & REV ID */
> ret = mlx4_get_module_info(mdev->dev, priv->port,
> - 0/*offset*/, 2/*size*/, data);
> + 0 /*offset*/, 2 /*size*/, data);
Why?
> if (ret < 2)
> return -EIO;
>
> - switch (data[0] /* identifier */) {
> - case MLX4_MODULE_ID_QSFP:
> - modinfo->type = ETH_MODULE_SFF_8436;
> + /* data[0] = identifier byte */
> + switch (data[0]) {
> + case SFF8024_ID_QSFP_8438:
> + modinfo->type = ETH_MODULE_SFF_8436;
> modinfo->eeprom_len = ETH_MODULE_SFF_8436_MAX_LEN;
> break;
> - case MLX4_MODULE_ID_QSFP_PLUS:
> - if (data[1] >= 0x3) { /* revision id */
> - modinfo->type = ETH_MODULE_SFF_8636;
> - modinfo->eeprom_len = ETH_MODULE_SFF_8636_MAX_LEN;
> - } else {
> - modinfo->type = ETH_MODULE_SFF_8436;
> + case SFF8024_ID_QSFP_8436_8636:
> + /* data[1] = revision id */
> + if (data[1] < 0x3) {
> + modinfo->type = ETH_MODULE_SFF_8436;
> modinfo->eeprom_len = ETH_MODULE_SFF_8436_MAX_LEN;
> + break;
> }
> - break;
> - case MLX4_MODULE_ID_QSFP28:
> - modinfo->type = ETH_MODULE_SFF_8636;
> + fallthrough;
> + case SFF8024_ID_QSFP28_8636:
> + modinfo->type = ETH_MODULE_SFF_8636;
> modinfo->eeprom_len = ETH_MODULE_SFF_8636_MAX_LEN;
> break;
> - case MLX4_MODULE_ID_SFP:
> - modinfo->type = ETH_MODULE_SFF_8472;
> + case SFF8024_ID_SFP:
> + modinfo->type = ETH_MODULE_SFF_8472;
> modinfo->eeprom_len = ETH_MODULE_SFF_8472_LEN;
> break;
> default:
> + netdev_err(dev, "%s: cable type not recognized: 0x%x\n",
> + __func__, data[0]);
0x%x -> %#x.
> return -EINVAL;
> }
>
> diff --git a/drivers/net/ethernet/mellanox/mlx4/port.c b/drivers/net/ethernet/mellanox/mlx4/port.c
> index 4e43f4a7d246..6dbd505e7f30 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/port.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/port.c
> @@ -34,6 +34,7 @@
> #include <linux/if_ether.h>
> #include <linux/if_vlan.h>
> #include <linux/export.h>
> +#include <linux/sfp.h>
>
> #include <linux/mlx4/cmd.h>
>
> @@ -2139,12 +2140,12 @@ int mlx4_get_module_info(struct mlx4_dev *dev, u8 port,
> return ret;
>
> switch (module_id) {
> - case MLX4_MODULE_ID_SFP:
> + case SFF8024_ID_SFP:
> mlx4_sfp_eeprom_params_set(&i2c_addr, &page_num, &offset);
> break;
> - case MLX4_MODULE_ID_QSFP:
> - case MLX4_MODULE_ID_QSFP_PLUS:
> - case MLX4_MODULE_ID_QSFP28:
> + case SFF8024_ID_QSFP_8438:
> + case SFF8024_ID_QSFP_8436_8636:
> + case SFF8024_ID_QSFP28_8636:
> mlx4_qsfp_eeprom_params_set(&i2c_addr, &page_num, &offset);
> break;
> default:
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> index 4d123dae912c..12a22e5c60ae 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> @@ -32,6 +32,7 @@
>
> #include <linux/dim.h>
> #include <linux/ethtool_netlink.h>
> +#include <linux/sfp.h>
>
> #include "en.h"
> #include "en/channels.h"
> @@ -1899,36 +1900,39 @@ static int mlx5e_get_module_info(struct net_device *netdev,
> {
> struct mlx5e_priv *priv = netdev_priv(netdev);
> struct mlx5_core_dev *dev = priv->mdev;
> - int size_read = 0;
> + int ret;
Why did you rename this variable?
> u8 data[4] = {0};
>
> - size_read = mlx5_query_module_eeprom(dev, 0, 2, data);
> - if (size_read < 2)
> + /* Read first 2 bytes to get Module & REV ID */
> + ret = mlx5_query_module_eeprom(dev,
> + 0 /*offset*/, 2 /*size*/, data);
> + if (ret < 2)
This whole hunk is not needed.
> return -EIO;
>
> /* data[0] = identifier byte */
> switch (data[0]) {
> - case MLX5_MODULE_ID_QSFP:
> + case SFF8024_ID_QSFP_8438:
> modinfo->type = ETH_MODULE_SFF_8436;
> modinfo->eeprom_len = ETH_MODULE_SFF_8436_MAX_LEN;
> break;
> - case MLX5_MODULE_ID_QSFP_PLUS:
> - case MLX5_MODULE_ID_QSFP28:
> + case SFF8024_ID_QSFP_8436_8636:
> /* data[1] = revision id */
> - if (data[0] == MLX5_MODULE_ID_QSFP28 || data[1] >= 0x3) {
> - modinfo->type = ETH_MODULE_SFF_8636;
> - modinfo->eeprom_len = ETH_MODULE_SFF_8636_MAX_LEN;
> - } else {
> + if (data[1] < 0x3) {
> modinfo->type = ETH_MODULE_SFF_8436;
> modinfo->eeprom_len = ETH_MODULE_SFF_8436_MAX_LEN;
> + break;
> }
> + fallthrough;
> + case SFF8024_ID_QSFP28_8636:
> + modinfo->type = ETH_MODULE_SFF_8636;
> + modinfo->eeprom_len = ETH_MODULE_SFF_8636_MAX_LEN;
> break;
> - case MLX5_MODULE_ID_SFP:
> + case SFF8024_ID_SFP:
> modinfo->type = ETH_MODULE_SFF_8472;
> modinfo->eeprom_len = ETH_MODULE_SFF_8472_LEN;
> break;
> default:
> - netdev_err(priv->netdev, "%s: cable type not recognized:0x%x\n",
> + netdev_err(priv->netdev, "%s: cable type not recognized: 0x%x\n",
Unrelated to this patch, but OK.
