Re: Re: [PATCH-SR9700] Merge USB 1.1 Ethernet Adapter SR9700 DeviceDriver into the Linux Kernel

2013-08-22 Thread Simon Horman 
On Wed, Aug 21, 2013 at 06:02:45PM +0800, liujunliang_ljl wrote:
 Dear Ben :
 
   1,  please give me email address of David Miller, and Thanks a 
 lot.

You can find him in the MAINTAINERS file.
He should also show up if you use scripts/get_maintainer.pl
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Re: [PATCH-SR9700] Merge USB 1.1 Ethernet Adapter SR9700 DeviceDriver into the Linux Kernel

2013-08-21 Thread liujunliang_ljl 
TXC_USBS_TXC1   (1  1)
+#defineTXC_USBS_TXC2   (1  2)
+#defineTXC_USBS_EP1RDY (1  5)
+#defineTXC_USBS_SUSFLAG(1  6)
+#defineTXC_USBS_RXFAULT(1  7)
+/* USB Control register */
+#defineUSBC0xF4
+#defineUSBC_EP3NAK (1  4)
+#defineUSBC_EP3ACK (1  5)
+
+/* Register access commands and flags */
+#defineSR_RD_REGS  0x00
+#defineSR_WR_REGS  0x01
+#defineSR_WR_REG   0x03
+#defineSR_REQ_RD_REG   (USB_DIR_IN | USB_TYPE_VENDOR | 
USB_RECIP_DEVICE)
+#defineSR_REQ_WR_REG   (USB_DIR_OUT | USB_TYPE_VENDOR | 
USB_RECIP_DEVICE)
+
+/* parameters */
+#defineSR_SHARE_TIMEOUT1000
+#defineSR_EEPROM_LEN   256
+#defineSR_MCAST_SIZE   8
+#defineSR_MCAST_ADDR_FLAG  0x80
+#defineSR_MCAST_MAX64
+#defineSR_TX_OVERHEAD  2   /* 2bytes header */
+#defineSR_RX_OVERHEAD  7   /* 3bytes header + 4crc tail */
+
+#endif /* _SR9700_H */




2013-08-21 



liujunliang_ljl 



发件人： Ben Hutchings 
发送时间： 2013-08-20  21:36:50 
收件人： liujunliang_ljl 
抄送： gregkh; linux-usb; netdev; linux-kernel; sunhecheng 
主题： Re: [PATCH-SR9700] Merge USB 1.1 Ethernet Adapter SR9700 DeviceDriver into 
the Linux Kernel 
 
On Tue, 2013-08-20 at 18:50 +0800, liujunliang_ljl wrote:
 Dear Gregkh  all :
 
  I am the software engineer Liu Junliang from ShenZhen CoreChips high 
 technology company, on the market of SR9700 chip is designed and owned by us. 
 SR9700 is a type of USB to Ethernet Converter and is compatible with 
 USB 1.1 protocol, We want to merge SR9700 device driver into the Linux 
 Kernel. The following is the Linux 3.10.7 version patch for SR9700, Please 
 give us the assessment and support.
 Thanks a lot.
As this is a net driver, the relevant maintainer is David Miller and not
Greg.
There are some style errors here which can be found using
scripts/checkpatch.pl.  You'll need to fix those and re-submit.  I'll
point out some more problems inline.
 --- /dev/null
 +++ b/drivers/net/usb/sr9700.c
[...]
 +static int sr9700_get_eeprom(struct net_device *net, struct ethtool_eeprom 
 *eeprom, u8 * data)
 +{
 + struct usbnet *dev = netdev_priv(net);
 + __le16 *ebuf = (__le16 *) data;
 + int i;
 +
 + /* access is 16bit */
 + if ((eeprom-offset % 2) || (eeprom-len % 2))
 + return -EINVAL;
You're really supposed to handle these cases by shifting as necessary.
 + for (i = 0; i  eeprom-len / 2; i++) {
 + if (sr_read_eeprom_word(dev, eeprom-offset / 2 + i, ebuf[i])  0)
 + return -EINVAL;
You should pass up the error code, not substitute -EINVAL.
[...]
 +static void sr9700_get_drvinfo(struct net_device *net, struct 
 ethtool_drvinfo *info)
 +{
 + /* Inherit standard device info */
 + usbnet_get_drvinfo(net, info);
 + info-eedump_len = SR_EEPROM_LEN;
You don't need to set eedump_len; the ethtool core will set it after
calling sr9700_get_eeprom_len().  So you don't need your own
implementation of get_drvinfo at all.
[...]
 +static int sr9700_bind(struct usbnet *dev, struct usb_interface *intf)
 +{
[...]
 + /* read MAC */
 + if (sr_read(dev, PAR, ETH_ALEN, dev-net-dev_addr)  0) {
 + printk(KERN_ERR Error reading MAC address\n);
 + ret = -ENODEV;
 + goto out;
 + }
[...]
I think this should read the MAC address from EEPROM and copy it to both
dev_addr to perm_addr.  MAC address changes should not persist if the
driver is reloaded.
Ben.
-- 
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.
N�r��yb�X��ǧv�^�)޺{.n�+{��^n�r���z���h����G���h�(�階�ݢj���m��z�ޖ���f���h���~�m�

Re: Re: [PATCH-SR9700] Merge USB 1.1 Ethernet Adapter SR9700 DeviceDriver into the Linux Kernel

2013-08-21 Thread liujunliang_ljl 
 (1  4)
+#defineUSBC_EP3ACK (1  5)
+
+/* Register access commands and flags */
+#defineSR_RD_REGS  0x00
+#defineSR_WR_REGS  0x01
+#defineSR_WR_REG   0x03
+#defineSR_REQ_RD_REG   (USB_DIR_IN | USB_TYPE_VENDOR | 
USB_RECIP_DEVICE)
+#defineSR_REQ_WR_REG   (USB_DIR_OUT | USB_TYPE_VENDOR | 
USB_RECIP_DEVICE)
+
+/* parameters */
+#defineSR_SHARE_TIMEOUT1000
+#defineSR_EEPROM_LEN   256
+#defineSR_MCAST_SIZE   8
+#defineSR_MCAST_ADDR_FLAG  0x80
+#defineSR_MCAST_MAX64
+#defineSR_TX_OVERHEAD  2   /* 2bytes header */
+#defineSR_RX_OVERHEAD  7   /* 3bytes header + 4crc tail */
+
+#endif /* _SR9700_H */



2013-08-21 



liujunliang_ljl 



发件人： Francois Romieu 
发送时间： 2013-08-21  04:46:12 
收件人： liujunliang_ljl 
抄送： gregkh; sunhecheng; linux-usb; netdev; linux-kernel 
主题： Re: [PATCH-SR9700] Merge USB 1.1 Ethernet Adapter SR9700 DeviceDriver into 
the Linux Kernel 
 
liujunliang_ljl liujunliang_...@163.com :
[...]
 We want to merge SR9700 device driver into the Linux Kernel. The following
 is the Linux 3.10.7 version patch for SR9700, Please give us the assessment
 and support.
Welcome. Go ahead.
[...]
 diff --git a/drivers/net/usb/sr9700.c b/drivers/net/usb/sr9700.c
 new file mode 100644
 index 000..6a6429a
 --- /dev/null
 +++ b/drivers/net/usb/sr9700.c
[...]
 +static int sr_read(struct usbnet *dev, u8 reg, u16 length, void *data)
 +{
 + int err;
 +
 + err = usbnet_read_cmd(dev, SR_RD_REGS, SR_REQ_RD_REG,
 + 0, reg, data, length);
err = usbnet_read_cmd(dev, SR_RD_REGS, SR_REQ_RD_REG, 0, reg, data,
  length);
 + if(err != length  err = 0)
 ^^ missing space
 + err = -EINVAL;
 + return err;
 +}
 +
 +static int sr_write(struct usbnet *dev, u8 reg, u16 length, void *data)
 +{
 + int err;
 +
 + err = usbnet_write_cmd(dev, SR_WR_REGS, SR_REQ_WR_REG,
 + 0, reg, data, length);
See above.
 + if (err = 0  err  length)
 + err = -EINVAL;
 + 
 + return err;
 +}
 +
 +static int sr_read_reg(struct usbnet *dev, u8 reg, u8 *value)
 +{
 + return sr_read(dev, reg, 1, value);
 +}
 +
 +static int sr_write_reg(struct usbnet *dev, u8 reg, u8 value)
 +{
 + return usbnet_write_cmd(dev, SR_WR_REGS, SR_REQ_WR_REG,
 +value, reg, NULL, 0);
Sic.
 +}
 +
 +static void sr_write_async(struct usbnet *dev, u8 reg, u16 length, void 
 *data)
 +{
 + usbnet_write_cmd_async(dev, SR_WR_REGS, SR_REQ_WR_REG,
 + 0, reg, data, length);
Sic.
 +}
 +
 +static void sr_write_reg_async(struct usbnet *dev, u8 reg, u8 value)
 +{
 + usbnet_write_cmd_async(dev, SR_WR_REGS, SR_REQ_WR_REG,
 + value, reg, NULL, 0);
Sic.
 +}
 +
 +static int sr_share_read_word(struct usbnet *dev, int phy, u8 reg, __le16 
 *value)
 +{
 + int ret, i;
 +
 + mutex_lock(dev-phy_mutex);
 +
 + sr_write_reg(dev, EPAR, phy ? (reg | 0x40) : reg);
 + sr_write_reg(dev, EPCR, phy ? 0xc : 0x4);
 +
 + for (i = 0; i  SR_SHARE_TIMEOUT; i++) {
 + u8 tmp = 0;
 +
 + udelay(1);
 + ret = sr_read_reg(dev, EPCR, tmp);
 + if (ret  0)
 + goto out;
goto out_unlock;
 +
 + /* ready */
 + if ((tmp  1) == 0)
if ((tmp  EPCR_ERRE) == 0)
 + break;
 + }
 +
 + if (i = SR_SHARE_TIMEOUT) {
 + netdev_err(dev-net, %s read timed out!, phy ? phy : eeprom);
 + ret = -EIO;
 + goto out;
 + }
 +
 + sr_write_reg(dev, EPCR, 0x0);
 + ret = sr_read(dev, EPDR, 2, value);
 +
 + netdev_dbg(dev-net, read shared %d 0x%02x returned 0x%04x, %d,
 +phy, reg, *value, ret);
 +
 + out:
   ^ please remove space.
 + mutex_unlock(dev-phy_mutex);
 + return ret;
 +}
 +
 +static int sr_share_write_word(struct usbnet *dev, int phy, u8 reg, __le16 
 value)
 +{
 + int ret, i;
 +
 + mutex_lock(dev-phy_mutex);
 +
 + ret = sr_write(dev, EPDR, 2, value);
 + if (ret  0)
 + goto out;
 +
 + sr_write_reg(dev, EPAR, phy ? (reg | 0x40) : reg);
 + sr_write_reg(dev, EPCR, phy ? 0x1a : 0x12);
 +
 + for (i = 0; i  SR_SHARE_TIMEOUT; i++) {
 + u8 tmp = 0;
 +
 + udelay(1);
 + ret = sr_read_reg(dev, EPCR, tmp);
 + if (ret  0)
 + goto out;
 +
 + /* ready */
 + if ((tmp  1) == 0)
 + break;
 + }
The 11 lines above are identical in sr_share_read_word. Please refactor.
[...]
 +static int sr_mdio_read(struct net_device *netdev, int phy_id, int loc)
 +{
 + struct usbnet *dev = netdev_priv(netdev);
 +
 + __le16 res;
Excess empty line.
 + int rc = 0;
 +
 + if (phy_id) {
 + netdev_dbg(dev-net, Only internal phy supported);
 + return 0;
 + }
 +
 + /* Access NSR_LINKST bit for link status instead of MII_BMSR */
 + if(loc == MII_BMSR){
 ^^   ^^
Missing spaces.
 + u8 value;
Excess tabs and missing empty line.
 + sr_read_reg(dev, NSR, value);
 + if(value  NSR_LINKST) {
Excess tabs, missing spaces, useless {.
 + rc = 1;
 + }
 + }
 + sr_share_read_word(dev, 1, loc, res);
 + if(rc == 1)
 + return (le16_to_cpu(res) | BMSR_LSTATUS);
 + else
 + return (le16_to_cpu(res)  ~BMSR_LSTATUS);
Excess ( (aka return is not a function

Re: Re: [PATCH-SR9700] Merge USB 1.1 Ethernet Adapter SR9700 DeviceDriver into the Linux Kernel

2013-08-21 Thread liujunliang_ljl 
 and flags */
+#defineSR_RD_REGS  0x00
+#defineSR_WR_REGS  0x01
+#defineSR_WR_REG   0x03
+#defineSR_REQ_RD_REG   (USB_DIR_IN | USB_TYPE_VENDOR | 
USB_RECIP_DEVICE)
+#defineSR_REQ_WR_REG   (USB_DIR_OUT | USB_TYPE_VENDOR | 
USB_RECIP_DEVICE)
+
+/* parameters */
+#defineSR_SHARE_TIMEOUT1000
+#defineSR_EEPROM_LEN   256
+#defineSR_MCAST_SIZE   8
+#defineSR_MCAST_ADDR_FLAG  0x80
+#defineSR_MCAST_MAX64
+#defineSR_TX_OVERHEAD  2   /* 2bytes header */
+#defineSR_RX_OVERHEAD  7   /* 3bytes header + 4crc tail */
+
+#endif /* _SR9700_H */





2013-08-21 



liujunliang_ljl 



发件人： Joe Perches 
发送时间： 2013-08-21  04:58:27 
收件人： Francois Romieu 
抄送： liujunliang_ljl; gregkh; sunhecheng; linux-usb; netdev; linux-kernel 
主题： Re: [PATCH-SR9700] Merge USB 1.1 Ethernet Adapter SR9700 DeviceDriver into 
the Linux Kernel 
 
On Tue, 2013-08-20 at 22:46 +0200, Francois Romieu wrote:
 liujunliang_ljl liujunliang_...@163.com :
  + if (i = SR_SHARE_TIMEOUT) {
  + netdev_err(dev-net, %s read timed out!, phy ? phy : eeprom);
netdev_level, like almost all other printk
messages needs a terminating \n newline to
avoid any possible message interleaving by other
printks.
  + if (!is_valid_ether_addr(addr-sa_data)) {
  + dev_err(net-dev, not setting invalid mac address %pM\n,
  + addr-sa_data);
 
  dev_err(net-dev, not setting invalid mac address %pM\n,
  addr-sa_data);
prefer netdev_level to dev_level where possible.
N�r��yb�X��ǧv�^�)޺{.n�+{��^n�r���z���h����G���h�(�階�ݢj���m��z�ޖ���f���h���~�m�

Re: Re: [PATCH-SR9700] Merge USB 1.1 Ethernet Adapter SR9700 DeviceDriver into the Linux Kernel

2013-08-21 Thread Joe Perches 
On Wed, 2013-08-21 at 18:07 +0800, liujunliang_ljl wrote:
   Thanks a lot and I have been fixed all the problems mentioned 
 above. please check the following patch and thanks again. 

Just trivial comments below:

 diff --git a/drivers/net/usb/sr9700.c b/drivers/net/usb/sr9700.c
[]
 +static void sr_write_reg_async(struct usbnet *dev, u8 reg, u8 value)
 +{
 + usbnet_write_cmd_async(dev, SR_WR_REGS, SR_REQ_WR_REG,
 + value, reg, NULL, 0);
 +}
 +
 +static int sr_share_read_word(struct usbnet *dev, int phy, u8 reg, __le16 
 *value)
 +{
[]
 + netdev_dbg(dev-net, read shared %d 0x%02x returned 0x%04x, %d\n,
 +phy, reg, *value, ret);

You have a lot of code that uses inconsistent
indentation.  Code in drivers/net and drivers/usb/net
generally prefers to use alignment to parenthesis for
multi-line statements

The first could use

usbnet_write_cmd_async(dev, SR_WR_REGS, SR_REQ_WR_REG,
   value, reg, NULL, 0);

and the second

netdev_dbg(dev-net, read shared %d 0x%02x returned 0x%04x, %d\n,
   phy, reg, *value, ret);

Maximal use of 8 space indentation tabs followed by
minimal spaces.

There are many of these above.

 +static int sr9700_get_eeprom(struct net_device *net, struct ethtool_eeprom 
 *eeprom, u8 *data)
[]
 + for (i = 0; i  eeprom-len / 2; i++)
 + ret = sr_read_eeprom_word(dev, eeprom-offset / 2 + i, 
 ebuf[i]);

One too many tabs for the second line, a few of these...

[]

 +static int sr_mdio_read(struct net_device *netdev, int phy_id, int loc)
[]
 + if (rc == 1)
 + return le16_to_cpu(res) | BMSR_LSTATUS;
 + else
 + return le16_to_cpu(res)  ~BMSR_LSTATUS;

The code below the returns here is unreachable.

 +
 + netdev_dbg(dev-net,
 +sr_mdio_read() phy_id=0x%02x, loc=0x%02x, returns=0x%04x\n,
 +phy_id, loc, le16_to_cpu(res));
 +
 + return le16_to_cpu(res);
 +}

You might try to use scripts/checkpatch.pl --strict if you
care about these.  It should flag most of these coding
style inconsistencies.

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Re: [PATCH-SR9700] Merge USB 1.1 Ethernet Adapter SR9700 DeviceDriver into the Linux Kernel

2013-08-21 Thread Greg KH 
On Wed, Aug 21, 2013 at 06:06:02PM +0800, liujunliang_ljl wrote:
 Dear Francois Romieu :
 
   1, all the format problems have been fixed
   
   2, sr9700.h registers definition is re-written
 
   3, Thanks for your detail checking and I have beed 
 scripts/checkpatch.pl the patch and please check it.
 
 [PATCH] :

You still aren't sending the patch in a format in which we can apply it
in.  Please read the file, Documentation/SubmittingPatches, specifically
the part about the Signed-off-by: line, and provding a proper body of
the patch for the changelog.

thanks,

greg k-h
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html