Re: [PATCH] smsc95xx: support ethtool get_regs

["Émeric Vigier" <emeric.vigier@savoirfairelinux.com>]

----- Mail original -----
> On Fri, 2012-07-06 at 14:15 -0400, Émeric Vigier wrote:
> > From: Emeric Vigier <emeric.vigier@savoirfairelinux.com>
> > 
> > Inspired by implementation in smsc911x.c and smsc9420.c
> > Tested on ARM/pandaboard rev A3
> > 
> > Signed-off-by: Emeric Vigier <emeric.vigier@savoirfairelinux.com>
> > ---
> >  drivers/net/usb/smsc95xx.c |   37
> >  +++++++++++++++++++++++++++++++++++++
> >  1 files changed, 37 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/net/usb/smsc95xx.c
> > b/drivers/net/usb/smsc95xx.c
> > index b1112e7..bce14f6 100644
> > --- a/drivers/net/usb/smsc95xx.c
> > +++ b/drivers/net/usb/smsc95xx.c
> > @@ -578,6 +578,41 @@ static int smsc95xx_ethtool_set_eeprom(struct
> > net_device *netdev,
> >  	return smsc95xx_write_eeprom(dev, ee->offset, ee->len, data);
> >  }
> >  
> > +
> > +static int smsc95xx_ethtool_getregslen(struct net_device *dev)
> > +{
> > +	/* all smsc95xx registers plus all phy registers */
> > +	return COE_CR - ID_REV + 1 + 32 * sizeof(u32);
> > +}
> > +
> > +static void
> > +smsc95xx_ethtool_getregs(struct net_device *netdev, struct
> > ethtool_regs *regs,
> > +			 void *buf)
> > +{
> > +	struct usbnet *dev = netdev_priv(netdev);
> > +	unsigned int i, j = 0, retval;
> > +	u32 *data = buf;
> > +
> > +	netif_dbg(dev, hw, dev->net, "ethtool_getregs\n");
> > +
> > +	retval = smsc95xx_read_reg(dev, ID_REV, &regs->version);
> > +	if (retval < 0) {
> > +		netdev_warn(dev->net, "REGS: cannot read ID_REV\n");
> > +		return;
> > +	}
> > +
> > +	for (i = 0; i <= COE_CR; i += (sizeof(u32))) {
> > +		retval = smsc95xx_read_reg(dev, i, &data[j++]);
> > +		if (retval < 0) {
> > +			netdev_warn(dev->net, "REGS: cannot read reg[%x]\n", i);
> > +			return;
> > +		}
> > +	}
> 
> Why does this start with i = 0 whereas the calculation of the length
> uses ID_REV as the starting point?  Maybe ID_REV == 0, but you should
> be
> consistent in whether you use the name or literal number.

You are right. I will broadcast ID_REV usage.

> 
> > +	for (i = 0; i <= PHY_SPECIAL; i++)
> > +		data[j++] = smsc95xx_mdio_read(netdev, dev->mii.phy_id, i);
> > +}
> 
> Again, why use PHY_SPECIAL (+ 1) here as opposed to 32 in the
> calculation of the length?

32 was ok, but I hesitated between defining a SMSC95XX_PHY_END or using the last defined register.
Are 32 register-PHY generic to most devices? I mean could 32 be use widely?

> 
> Ben.
> 
> >  static const struct ethtool_ops smsc95xx_ethtool_ops = {
> >  	.get_link	= usbnet_get_link,
> >  	.nway_reset	= usbnet_nway_reset,
> > @@ -589,6 +624,8 @@ static const struct ethtool_ops
> > smsc95xx_ethtool_ops = {
> >  	.get_eeprom_len	= smsc95xx_ethtool_get_eeprom_len,
> >  	.get_eeprom	= smsc95xx_ethtool_get_eeprom,
> >  	.set_eeprom	= smsc95xx_ethtool_set_eeprom,
> > +	.get_regs_len	= smsc95xx_ethtool_getregslen,
> > +	.get_regs	= smsc95xx_ethtool_getregs,
> >  };
> >  
> >  static int smsc95xx_ioctl(struct net_device *netdev, struct ifreq
> >  *rq, int cmd)
> 
> --
> Ben Hutchings, Staff Engineer, Solarflare
> Not speaking for my employer; that's the marketing department's job.
> They asked us to note that Solarflare product names are trademarked.
> 
> 

-- 
Emeric