diff mbox

[1/5] drivers: net: usb: rtl8150: bug fixing and cleanup

Message ID alpine.DEB.2.02.1305181902080.7518@fry.nucleusys.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Petko Manolov May 18, 2013, 4:23 p.m. UTC 
From: Petko Manolov <petkan@nucleusys.com>

Moving constant and structure definitions out of rtl8150.c;

Signed-off-by: Petko Manolov <petkan@nucleusys.com>
---
 drivers/net/usb/rtl8150.c |  121 +----------------------------------
 1 file changed, 2 insertions(+), 119 deletions(-)

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

Comments

Francois Romieu May 18, 2013, 9:09 p.m. UTC | #1 
Petko Manolov <petkan@nucleusys.com> :
> From: Petko Manolov <petkan@nucleusys.com>
> 
> Moving constant and structure definitions out of rtl8150.c;

What's the point ?

[...]
> ---
>  drivers/net/usb/rtl8150.c |  121 +----------------------------------
>  1 file changed, 2 insertions(+), 119 deletions(-)
> 
> diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> index a491d3a..7d1897b 100644
> --- a/drivers/net/usb/rtl8150.c
> +++ b/drivers/net/usb/rtl8150.c
> @@ -17,132 +17,15 @@
>  #include <linux/usb.h>
>  #include <asm/uaccess.h>
>  
> +#include "rtl8150.h"

It won't compile. You shouldn't do that.
Petko Manolov May 19, 2013, 8:42 a.m. UTC | #2 
On Sat, 18 May 2013, Francois Romieu wrote:

> Petko Manolov <petkan@nucleusys.com> :
> > From: Petko Manolov <petkan@nucleusys.com>
> > 
> > Moving constant and structure definitions out of rtl8150.c;
> 
> What's the point ?

The general logic of having .h files applies.
 
> [...]
> > ---
> >  drivers/net/usb/rtl8150.c |  121 +----------------------------------
> >  1 file changed, 2 insertions(+), 119 deletions(-)
> > 
> > diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> > index a491d3a..7d1897b 100644
> > --- a/drivers/net/usb/rtl8150.c
> > +++ b/drivers/net/usb/rtl8150.c
> > @@ -17,132 +17,15 @@
> >  #include <linux/usb.h>
> >  #include <asm/uaccess.h>
> >  
> > +#include "rtl8150.h"
> 
> It won't compile. You shouldn't do that.

It does compile.  Both inside and outside of the tree.

If the proper place for rtl8150.h is somewhere in include/linux/... then 
it is different matter.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Francois Romieu May 19, 2013, 7:01 p.m. UTC | #3 
Petko Manolov <petkan@nucleusys.com> :
> On Sat, 18 May 2013, Francois Romieu wrote:
> > > From: Petko Manolov <petkan@nucleusys.com>
[...]
> > > Moving constant and structure definitions out of rtl8150.c;
> > 
> > What's the point ?
> 
> The general logic of having .h files applies.

Which one ?
- share it through the kernel or with userspace
- personal choice .c split

I don't mind the later even if it does not exactly apply to a
WIP driver. I'd still avoid future copycat followers though.

[...]
> > >  drivers/net/usb/rtl8150.c |  121 +----------------------------------
> > >  1 file changed, 2 insertions(+), 119 deletions(-)
> > > 
> > > diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> > > index a491d3a..7d1897b 100644
> > > --- a/drivers/net/usb/rtl8150.c
> > > +++ b/drivers/net/usb/rtl8150.c
> > > @@ -17,132 +17,15 @@
> > >  #include <linux/usb.h>
> > >  #include <asm/uaccess.h>
> > >  
> > > +#include "rtl8150.h"
> > 
> > It won't compile. You shouldn't do that.
> 
> It does compile.  Both inside and outside of the tree.

The rtl8150.h file is created in patch #2. This is patch #1.

So...
Petko Manolov May 20, 2013, 6:58 a.m. UTC | #4 
On Sun, 19 May 2013, Francois Romieu wrote:

> Which one ?
> - share it through the kernel or with userspace
> - personal choice .c split

It is obviously not the former.  I think that in general constant and 
other definitions (in their majority) should be in a header file.  I 
definitely like this way better.

Perhaps in this particular case my patch is a bit of an overkill as the 
code lines aren't that many.  If the consensus is in this direction i'll 
revert this part of the series.

> I don't mind the later even if it does not exactly apply to a
> WIP driver. I'd still avoid future copycat followers though.

This isn't WIP anymore as the W(ork) part is missing.  After so many quiet 
years i assume the experimental tag should be removed.

> The rtl8150.h file is created in patch #2. This is patch #1.
> 
> So...

So first apply patch #1 and then patch #2.

However, if it is required for the driver to be in build-able form after 
applying any single patch i'll coalesce #1 and #2 before next submission.


David?


		Petko
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller May 20, 2013, 7 a.m. UTC | #5 
From: Petko Manolov <petkan@nucleusys.com>
Date: Mon, 20 May 2013 09:58:39 +0300 (EEST)

> So first apply patch #1 and then patch #2.

Then nobody can properly GIT bisect through your patch series.

The tree must work and build properly at each and every step
of your patch series.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Petko Manolov May 20, 2013, 7:09 a.m. UTC | #6 
On Mon, 20 May 2013, David Miller wrote:

> From: Petko Manolov <petkan@nucleusys.com>
> Date: Mon, 20 May 2013 09:58:39 +0300 (EEST)
> 
> > So first apply patch #1 and then patch #2.
> 
> Then nobody can properly GIT bisect through your patch series.
> 
> The tree must work and build properly at each and every step
> of your patch series.

Got it.  What about the .c/.h split?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller May 20, 2013, 7:12 a.m. UTC | #7 
From: Petko Manolov <petkan@nucleusys.com>
Date: Mon, 20 May 2013 10:09:00 +0300 (EEST)

> What about the .c/.h split?

I have no strong opinion either way.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Petko Manolov May 20, 2013, 7:18 a.m. UTC | #8 
On Mon, 20 May 2013, David Miller wrote:

> From: Petko Manolov <petkan@nucleusys.com>
> Date: Mon, 20 May 2013 10:09:00 +0300 (EEST)
> 
> > What about the .c/.h split?
> 
> I have no strong opinion either way.

OK, so i'll prepare new patch series that coalesce my original patch #1 
and #2, and apply the Francois suggestion about using the generic 
netdev_alloc_skb_ip_align() in the interrupt path.

Which tree would you want me to diff against?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller May 20, 2013, 7:43 a.m. UTC | #9 
From: Petko Manolov <petkan@nucleusys.com>
Date: Mon, 20 May 2013 10:18:17 +0300 (EEST)

> On Mon, 20 May 2013, David Miller wrote:
> 
>> From: Petko Manolov <petkan@nucleusys.com>
>> Date: Mon, 20 May 2013 10:09:00 +0300 (EEST)
>> 
>> > What about the .c/.h split?
>> 
>> I have no strong opinion either way.
> 
> OK, so i'll prepare new patch series that coalesce my original patch #1 
> and #2, and apply the Francois suggestion about using the generic 
> netdev_alloc_skb_ip_align() in the interrupt path.
> 
> Which tree would you want me to diff against?

As has been explained to you already, cleanups belong in 'net-next',
bug fixes belong in 'net'.

If you series has both, you have to submit them separately.  Submit
the bug fixes to 'net', then the next time I merge 'net' into 'net-next'
you can submit the cleanups on top against 'net-next'.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Petko Manolov May 20, 2013, 7:49 a.m. UTC | #10 
On Mon, 20 May 2013, David Miller wrote:

> From: Petko Manolov <petkan@nucleusys.com>
> Date: Mon, 20 May 2013 10:18:17 +0300 (EEST)
> 
> > On Mon, 20 May 2013, David Miller wrote:
> > 
> >> From: Petko Manolov <petkan@nucleusys.com>
> >> Date: Mon, 20 May 2013 10:09:00 +0300 (EEST)
> >> 
> >> > What about the .c/.h split?
> >> 
> >> I have no strong opinion either way.
> > 
> > OK, so i'll prepare new patch series that coalesce my original patch #1 
> > and #2, and apply the Francois suggestion about using the generic 
> > netdev_alloc_skb_ip_align() in the interrupt path.
> > 
> > Which tree would you want me to diff against?
> 
> As has been explained to you already, cleanups belong in 'net-next',
> bug fixes belong in 'net'.
> 
> If you series has both, you have to submit them separately.  Submit
> the bug fixes to 'net', then the next time I merge 'net' into 'net-next'
> you can submit the cleanups on top against 'net-next'.

Got it.  Thanks.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
index a491d3a..7d1897b 100644
--- a/drivers/net/usb/rtl8150.c
+++ b/drivers/net/usb/rtl8150.c
@@ -17,132 +17,15 @@ 
 #include <linux/usb.h>
 #include <asm/uaccess.h>
 
+#include "rtl8150.h"
+
 /* Version Information */
 #define DRIVER_VERSION "v0.6.2 (2004/08/27)"
 #define DRIVER_AUTHOR "Petko Manolov <petkan@users.sourceforge.net>"
 #define DRIVER_DESC "rtl8150 based usb-ethernet driver"
 
-#define	IDR			0x0120
-#define	MAR			0x0126
-#define	CR			0x012e
-#define	TCR			0x012f
-#define	RCR			0x0130
-#define	TSR			0x0132
-#define	RSR			0x0133
-#define	CON0			0x0135
-#define	CON1			0x0136
-#define	MSR			0x0137
-#define	PHYADD			0x0138
-#define	PHYDAT			0x0139
-#define	PHYCNT			0x013b
-#define	GPPC			0x013d
-#define	BMCR			0x0140
-#define	BMSR			0x0142
-#define	ANAR			0x0144
-#define	ANLP			0x0146
-#define	AER			0x0148
-#define CSCR			0x014C  /* This one has the link status */
-#define CSCR_LINK_STATUS	(1 << 3)
-
-#define	IDR_EEPROM		0x1202
-
-#define	PHY_READ		0
-#define	PHY_WRITE		0x20
-#define	PHY_GO			0x40
-
-#define	MII_TIMEOUT		10
-#define	INTBUFSIZE		8
-
-#define	RTL8150_REQT_READ	0xc0
-#define	RTL8150_REQT_WRITE	0x40
-#define	RTL8150_REQ_GET_REGS	0x05
-#define	RTL8150_REQ_SET_REGS	0x05
-
-
-/* Transmit status register errors */
-#define TSR_ECOL		(1<<5)
-#define TSR_LCOL		(1<<4)
-#define TSR_LOSS_CRS		(1<<3)
-#define TSR_JBR			(1<<2)
-#define TSR_ERRORS		(TSR_ECOL | TSR_LCOL | TSR_LOSS_CRS | TSR_JBR)
-/* Receive status register errors */
-#define RSR_CRC			(1<<2)
-#define RSR_FAE			(1<<1)
-#define RSR_ERRORS		(RSR_CRC | RSR_FAE)
-
-/* Media status register definitions */
-#define MSR_DUPLEX		(1<<4)
-#define MSR_SPEED		(1<<3)
-#define MSR_LINK		(1<<2)
-
-/* Interrupt pipe data */
-#define INT_TSR			0x00
-#define INT_RSR			0x01
-#define INT_MSR			0x02
-#define INT_WAKSR		0x03
-#define INT_TXOK_CNT		0x04
-#define INT_RXLOST_CNT		0x05
-#define INT_CRERR_CNT		0x06
-#define INT_COL_CNT		0x07
-
-
-#define	RTL8150_MTU		1540
-#define	RTL8150_TX_TIMEOUT	(HZ)
-#define	RX_SKB_POOL_SIZE	4
-
-/* rtl8150 flags */
-#define	RTL8150_HW_CRC		0
-#define	RX_REG_SET		1
-#define	RTL8150_UNPLUG		2
-#define	RX_URB_FAIL		3
-
-/* Define these values to match your device */
-#define	VENDOR_ID_REALTEK		0x0bda
-#define	VENDOR_ID_MELCO			0x0411
-#define	VENDOR_ID_MICRONET		0x3980
-#define	VENDOR_ID_LONGSHINE		0x07b8
-#define	VENDOR_ID_OQO			0x1557
-#define	VENDOR_ID_ZYXEL			0x0586
-
-#define PRODUCT_ID_RTL8150		0x8150
-#define	PRODUCT_ID_LUAKTX		0x0012
-#define	PRODUCT_ID_LCS8138TX		0x401a
-#define PRODUCT_ID_SP128AR		0x0003
-#define	PRODUCT_ID_PRESTIGE		0x401a
-
-#undef	EEPROM_WRITE
-
-/* table of devices that work with this driver */
-static struct usb_device_id rtl8150_table[] = {
-	{USB_DEVICE(VENDOR_ID_REALTEK, PRODUCT_ID_RTL8150)},
-	{USB_DEVICE(VENDOR_ID_MELCO, PRODUCT_ID_LUAKTX)},
-	{USB_DEVICE(VENDOR_ID_MICRONET, PRODUCT_ID_SP128AR)},
-	{USB_DEVICE(VENDOR_ID_LONGSHINE, PRODUCT_ID_LCS8138TX)},
-	{USB_DEVICE(VENDOR_ID_OQO, PRODUCT_ID_RTL8150)},
-	{USB_DEVICE(VENDOR_ID_ZYXEL, PRODUCT_ID_PRESTIGE)},
-	{}
-};
-
 MODULE_DEVICE_TABLE(usb, rtl8150_table);
 
-struct rtl8150 {
-	unsigned long flags;
-	struct usb_device *udev;
-	struct tasklet_struct tl;
-	struct net_device *netdev;
-	struct urb *rx_urb, *tx_urb, *intr_urb, *ctrl_urb;
-	struct sk_buff *tx_skb, *rx_skb;
-	struct sk_buff *rx_skb_pool[RX_SKB_POOL_SIZE];
-	spinlock_t rx_pool_lock;
-	struct usb_ctrlrequest dr;
-	int intr_interval;
-	__le16 rx_creg;
-	u8 *intr_buff;
-	u8 phy;
-};
-
-typedef struct rtl8150 rtl8150_t;
-
 static const char driver_name [] = "rtl8150";
 
 /*