Re: [PATCH v2] net: atlantic: support reading SFP module info

[Paolo Abeni <pabeni@redhat.com>]

On 10/10/24 21:06, Lorenz Brun wrote:
> Add support for reading SFP module info and digital diagnostic
> monitoring data if supported by the module. The only Aquantia
> controller without an integrated PHY is the AQC100 which belongs to
> the B0 revision, that's why it's only implemented there.
> 
> The register information was extracted from a diagnostic tool made
> publicly available by Dell, but all code was written from scratch by me.
> 
> This has been tested to work with a variety of both optical and direct
> attach modules I had lying around and seems to work fine with all of
> them, including the diagnostics if supported by an optical module.
> All tests have been done with an AQC100 on an TL-NT521F card on firmware
> version 3.1.121 (current at the time of this patch).
> 
> Signed-off-by: Lorenz Brun <lorenz@brun.one>

This does not apply to net-next, you need to rebase.

> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c b/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c
> index 440ff4616fec..ee809f96e9a4 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c
> @@ -15,6 +15,7 @@
>   #include "aq_macsec.h"
>   #include "aq_main.h"
>   
> +#include <linux/ethtool.h>
>   #include <linux/linkmode.h>
>   #include <linux/ptp_clock_kernel.h>
>   
> @@ -977,6 +978,78 @@ static int aq_ethtool_set_phy_tunable(struct net_device *ndev,
>   	return err;
>   }
>   
> +static int aq_ethtool_get_module_info(struct net_device *ndev,
> +				      struct ethtool_modinfo *modinfo)
> +{
> +	struct aq_nic_s *aq_nic = netdev_priv(ndev);
> +	u8 compliance_val, dom_type;
> +	int err;
> +
> +	/* Module EEPROM is only supported for controllers with external PHY */
> +	if (aq_nic->aq_nic_cfg.aq_hw_caps->media_type != AQ_HW_MEDIA_TYPE_FIBRE)
> +		return -EOPNOTSUPP;
> +
> +	if (!aq_nic->aq_hw_ops->hw_read_module_eeprom)
> +		return -EOPNOTSUPP;
> +
> +	err = aq_nic->aq_hw_ops->hw_read_module_eeprom(aq_nic->aq_hw,
> +		SFF_8472_ID_ADDR, SFF_8472_COMP_ADDR, 1, &compliance_val);
> +	if (err)
> +		return err;
> +
> +	err = aq_nic->aq_hw_ops->hw_read_module_eeprom(aq_nic->aq_hw,
> +		SFF_8472_ID_ADDR, SFF_8472_DOM_TYPE_ADDR, 1, &dom_type);
> +	if (err)
> +		return err;
> +
> +	if (dom_type & SFF_8472_ADDRESS_CHANGE_REQ_MASK || compliance_val == 0x00) {
> +		modinfo->type = ETH_MODULE_SFF_8079;
> +		modinfo->eeprom_len = ETH_MODULE_SFF_8079_LEN;
> +	} else {
> +		modinfo->type = ETH_MODULE_SFF_8472;
> +		modinfo->eeprom_len = ETH_MODULE_SFF_8472_LEN;
> +	}
> +	return 0;
> +}
> +
> +static int aq_ethtool_get_module_eeprom(struct net_device *ndev,
> +					struct ethtool_eeprom *ee, unsigned char *data)
> +{
> +	int err;
> +	unsigned int first, last, len;
> +	struct aq_nic_s *aq_nic = netdev_priv(ndev);

Please respect the reverse x-mas tree order in variable declaration.

> +
> +	if (!aq_nic->aq_hw_ops->hw_read_module_eeprom)
> +		return -EOPNOTSUPP;
> +
> +	first = ee->offset;
> +	last = ee->offset + ee->len;
> +
> +	if (first < ETH_MODULE_SFF_8079_LEN) {
> +		len = min(last, ETH_MODULE_SFF_8079_LEN);
> +		len -= first;

The above 2 statements can be collapsed in a single one. A similar thing 
below

[...]
> +// Starts an I2C/SMBUS write to a given address. addr is in 7-bit format,
> +// the read/write bit is not part of it.

Please avoid C99-style comments, use /* */ instead.

Many more cases below.

Thanks,

Paolo