| Message ID | 1457609067-3674-1-git-send-email-josright123@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
The subject line on this is very vague; it should say which ethtool operation you're implementing. On Thu, 2016-03-10 at 19:24 +0800, Joseph CHANG wrote: > Add function dm9601_set_eeprom which tested good with ethtool > utility, include the eeprom words dump and the eeprom byte write. > > Signed-off-by: Joseph CHANG <josright123@gmail.com> > --- > drivers/net/usb/dm9601.c | 39 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/drivers/net/usb/dm9601.c b/drivers/net/usb/dm9601.c > index 50095df..a6904f4 100644 > --- a/drivers/net/usb/dm9601.c > +++ b/drivers/net/usb/dm9601.c > @@ -61,6 +61,7 @@ > #define DM_RX_OVERHEAD 7 /* 3 byte header + 4 byte crc tail */ > #define DM_TIMEOUT 1000 > #define DM_EP3I_VAL 0x07 > +#define MD96XX_EEPROM_MAGIC 0x9620 The get_eeprom operation needs to be changed, to set eeprom->magic to this value. > static int dm_read(struct usbnet *dev, u8 reg, u16 length, void *data) > { > @@ -289,6 +290,43 @@ static int dm9601_get_eeprom(struct net_device *net, > return 0; > } > > +static int dm9601_set_eeprom(struct net_device *net, > + struct ethtool_eeprom *eeprom, u8 *data) > +{ > + struct usbnet *dev = netdev_priv(net); > + int offset = eeprom->offset; > + int len = eeprom->len; > + int done; > + > + if (eeprom->magic != MD96XX_EEPROM_MAGIC) { > + netdev_dbg(dev->net, "EEPROM: magic value mismatch, magic = 0x%x", > + eeprom->magic); > + return -EINVAL; > + } > + > + while (len > 0) { > + if (len & 1 || offset & 1) { Given that the get_eeprom operation only handles word-aligned reads, is it really important to support misaligned writes in set_eeprom? Also, this test should be 'if (len == 1 || offset & 1)'. Consider a write with offset = 2, len = 3. You want to write a word on the first iteration, then a byte on the second iteration. > + int which = offset & 1; > + u8 tmp[2]; > + > + dm_read_eeprom_word(dev, offset / 2, tmp); > + tmp[which] = *data; > + dm_write_eeprom_word(dev, offset / 2, > + tmp[0] | tmp[1] << 8); > + mdelay(10); Why is a delay required here, but not in the other case? > + done = 1; > + } else { > + dm_write_eeprom_word(dev, offset / 2, > + data[0] | data[1] << 8); > + done = 2; > + } > + data += done; > + offset += done; > + len -= done; > + } > + return 0; > +} [...] Ben.
I did verify to dump EEPROM data and also write EEPROM by per byte. 1.Plug dm9601/dm9621 adapter and has get dm9601.ko be 'insmod' to have 'eht0', 2.Run ethtool v3.7 (as attached executable file and it's help display.) 3. Commands: ./ethtool -e eth0 (dump EEPROM data for all the .get_eeprom_len ) ./ethtool -E eth0 magic 0x9620 offset 0 value 0xf1 (write 0xf1 to eeprom byte0) ./ethtool -E eth0 magic 0x9620 offset 1 value 0xf2 (write 0xf2 to eeprom byte1) ./ethtool -E eth0 magic 0x9620 offset 2 value 0xf3 (write 0xf3 to eeprom byte2) *I am not sure if it can cover all the huge purpose functions of ethtool, or some leakage in such implementation. Thanks and need further advice~ Best Regards, Joseph CHANG System Application Engineering Division Davicom Semiconductor, Inc. No. 6 Li-Hsin 6th Rd., Science-Based Park, Hsin-Chu, Taiwan. Tel: 886-3-5798797 Ex 8534 Fax: 886-3-5646929 Web: http://www.davicom.com.tw -----Original Message----- From: Ben Hutchings [mailto:ben@decadent.org.uk] Sent: Thursday, March 10, 2016 7:49 PM To: Joseph CHANG; Peter Korsgaard; netdev@vger.kernel.org; linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org Cc: Joseph Chang Subject: Re: [PATCH 3/3] dm9601: add support ethtool style utility The subject line on this is very vague; it should say which ethtool operation you're implementing. On Thu, 2016-03-10 at 19:24 +0800, Joseph CHANG wrote: > Add function dm9601_set_eeprom which tested good with ethtool > utility, include the eeprom words dump and the eeprom byte write. > > Signed-off-by: Joseph CHANG <josright123@gmail.com> > --- > drivers/net/usb/dm9601.c | 39 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/drivers/net/usb/dm9601.c b/drivers/net/usb/dm9601.c > index 50095df..a6904f4 100644 > --- a/drivers/net/usb/dm9601.c > +++ b/drivers/net/usb/dm9601.c > @@ -61,6 +61,7 @@ > #define DM_RX_OVERHEAD 7 /* 3 byte header + 4 byte crc tail */ > #define DM_TIMEOUT 1000 > #define DM_EP3I_VAL 0x07 > +#define MD96XX_EEPROM_MAGIC 0x9620 The get_eeprom operation needs to be changed, to set eeprom->magic to this value. > static int dm_read(struct usbnet *dev, u8 reg, u16 length, void *data) > { > @@ -289,6 +290,43 @@ static int dm9601_get_eeprom(struct net_device *net, > return 0; > } > > +static int dm9601_set_eeprom(struct net_device *net, > + struct ethtool_eeprom *eeprom, u8 *data) > +{ > + struct usbnet *dev = netdev_priv(net); > + int offset = eeprom->offset; > + int len = eeprom->len; > + int done; > + > + if (eeprom->magic != MD96XX_EEPROM_MAGIC) { > + netdev_dbg(dev->net, "EEPROM: magic value mismatch, magic = 0x%x", > + eeprom->magic); > + return -EINVAL; > + } > + > + while (len > 0) { > + if (len & 1 || offset & 1) { Given that the get_eeprom operation only handles word-aligned reads, is it really important to support misaligned writes in set_eeprom? Also, this test should be 'if (len == 1 || offset & 1)'. Consider a write with offset = 2, len = 3. You want to write a word on the first iteration, then a byte on the second iteration. > + int which = offset & 1; > + u8 tmp[2]; > + > + dm_read_eeprom_word(dev, offset / 2, tmp); > + tmp[which] = *data; > + dm_write_eeprom_word(dev, offset / 2, > + tmp[0] | tmp[1] << 8); > + mdelay(10); Why is a delay required here, but not in the other case? > + done = 1; > + } else { > + dm_write_eeprom_word(dev, offset / 2, > + data[0] | data[1] << 8); > + done = 2; > + } > + data += done; > + offset += done; > + len -= done; > + } > + return 0; > +} [...] Ben.
diff --git a/drivers/net/usb/dm9601.c b/drivers/net/usb/dm9601.c index 50095df..a6904f4 100644 --- a/drivers/net/usb/dm9601.c +++ b/drivers/net/usb/dm9601.c @@ -61,6 +61,7 @@ #define DM_RX_OVERHEAD 7 /* 3 byte header + 4 byte crc tail */ #define DM_TIMEOUT 1000 #define DM_EP3I_VAL 0x07 +#define MD96XX_EEPROM_MAGIC 0x9620 static int dm_read(struct usbnet *dev, u8 reg, u16 length, void *data) { @@ -289,6 +290,43 @@ static int dm9601_get_eeprom(struct net_device *net, return 0; } +static int dm9601_set_eeprom(struct net_device *net, + struct ethtool_eeprom *eeprom, u8 *data) +{ + struct usbnet *dev = netdev_priv(net); + int offset = eeprom->offset; + int len = eeprom->len; + int done; + + if (eeprom->magic != MD96XX_EEPROM_MAGIC) { + netdev_dbg(dev->net, "EEPROM: magic value mismatch, magic = 0x%x", + eeprom->magic); + return -EINVAL; + } + + while (len > 0) { + if (len & 1 || offset & 1) { + int which = offset & 1; + u8 tmp[2]; + + dm_read_eeprom_word(dev, offset / 2, tmp); + tmp[which] = *data; + dm_write_eeprom_word(dev, offset / 2, + tmp[0] | tmp[1] << 8); + mdelay(10); + done = 1; + } else { + dm_write_eeprom_word(dev, offset / 2, + data[0] | data[1] << 8); + done = 2; + } + data += done; + offset += done; + len -= done; + } + return 0; +} + static int dm9601_mdio_read(struct net_device *netdev, int phy_id, int loc) { struct usbnet *dev = netdev_priv(netdev); @@ -354,6 +392,7 @@ static const struct ethtool_ops dm9601_ethtool_ops = { .set_msglevel = usbnet_set_msglevel, .get_eeprom_len = dm9601_get_eeprom_len, .get_eeprom = dm9601_get_eeprom, + .set_eeprom = dm9601_set_eeprom, .get_settings = usbnet_get_settings, .set_settings = usbnet_set_settings, .nway_reset = usbnet_nway_reset,
Add function dm9601_set_eeprom which tested good with ethtool utility, include the eeprom words dump and the eeprom byte write. Signed-off-by: Joseph CHANG <josright123@gmail.com> --- drivers/net/usb/dm9601.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)