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

[Raju Lakkaraju <Raju.Lakkaraju@microchip.com>]

Hi Jakub,

Thank you for review comments.

The 10/19/2022 16:43, Jakub Kicinski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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.
> 

Accepted. I will remove the above code.

> > +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.

O.K. I will change.

> 
> 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.

I will implement Andrew's suggestion in my next regdump function patch.
Is it OK ?

--------
Thanks,
Raju