Re: [PATCH net-next V4] net: lan743x: Add support to SGMII register dump for PCI11010/PCI11414 chips

[Jakub Kicinski]

On Tue, 18 Oct 2022 11:44:25 +0530 Raju Lakkaraju wrote:
> Add support to SGMII register dump

> +++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
> @@ -24,6 +24,9 @@
>  #define LOCK_TIMEOUT_MAX_CNT		    (100) // 1 sec (10 msce * 100)
>  
>  #define LAN743X_CSR_READ_OP(offset)	     lan743x_csr_read(adapter, offset)
> +#define VSPEC1			MDIO_MMD_VEND1
> +#define VSPEC2			MDIO_MMD_VEND2
> +#define SGMII_RD(adp, dev, adr) lan743x_sgmii_dump_read(adp, dev, adr)

These defines help limit the line length?
Please don't obfuscate code like that, see below.

> +static void lan743x_sgmii_regs(struct net_device *dev, void *p)
> +{
> +	struct lan743x_adapter *adp = netdev_priv(dev);
> +	u32 *rb = p;
> +
> +	rb[ETH_SR_VSMMD_DEV_ID1]                = SGMII_RD(adp, VSPEC1, 0x0002);
> +	rb[ETH_SR_VSMMD_DEV_ID2]                = SGMII_RD(adp, VSPEC1, 0x0003);
> +	rb[ETH_SR_VSMMD_PCS_ID1]                = SGMII_RD(adp, VSPEC1, 0x0004);
> +	rb[ETH_SR_VSMMD_PCS_ID2]                = SGMII_RD(adp, VSPEC1, 0x0005);
> +	rb[ETH_SR_VSMMD_STS]                    = SGMII_RD(adp, VSPEC1, 0x0008);
> +	rb[ETH_SR_VSMMD_CTRL]                   = SGMII_RD(adp, VSPEC1, 0x0009);
> +	rb[ETH_SR_MII_CTRL]                     = SGMII_RD(adp, VSPEC2, 0x0000);
> +	rb[ETH_SR_MII_STS]                      = SGMII_RD(adp, VSPEC2, 0x0001);
> +	rb[ETH_SR_MII_DEV_ID1]                  = SGMII_RD(adp, VSPEC2, 0x0002);
> +	rb[ETH_SR_MII_DEV_ID2]                  = SGMII_RD(adp, VSPEC2, 0x0003);
> +	rb[ETH_SR_MII_AN_ADV]                   = SGMII_RD(adp, VSPEC2, 0x0004);
> +	rb[ETH_SR_MII_LP_BABL]                  = SGMII_RD(adp, VSPEC2, 0x0005);
> +	rb[ETH_SR_MII_EXPN]                     = SGMII_RD(adp, VSPEC2, 0x0006);
> +	rb[ETH_SR_MII_EXT_STS]                  = SGMII_RD(adp, VSPEC2, 0x000F);
> +	rb[ETH_SR_MII_TIME_SYNC_ABL]            = SGMII_RD(adp, VSPEC2, 0x0708);
> +	rb[ETH_SR_MII_TIME_SYNC_TX_MAX_DLY_LWR] = SGMII_RD(adp, VSPEC2, 0x0709);

You can declare a structure holding the params and save the info there:

	struct {
		u8 id;
		u8 dev;
		u16 addr;
	} regs[] = {
		{ ETH_SR_MII_TIME_SYNC_TX_MAX_DLY_LWR,	MDIO_MMD_VEND2,	0x0709 },
	};

that should fit on the line.

You can then read the values in a loop. And inside that loop you can
handle errors (perhaps avoiding the need for lan743x_sgmii_dump_read()
which seems rather unnecessary as lan743x_sgmii_read() already prints 
errors).

FWIW I like Andrew's suggestion from v3 to use version as a bitfield, too.