* [PATCH 1/5] drivers: net: usb: rtl8150: bug fixing and cleanup
@ 2013-05-18 16:23 Petko Manolov
2013-05-18 21:09 ` Francois Romieu
0 siblings, 1 reply; 11+ messages in thread
From: Petko Manolov @ 2013-05-18 16:23 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-kernel
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(-)
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";
/*
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/5] drivers: net: usb: rtl8150: bug fixing and cleanup
2013-05-18 16:23 [PATCH 1/5] drivers: net: usb: rtl8150: bug fixing and cleanup Petko Manolov
@ 2013-05-18 21:09 ` Francois Romieu
2013-05-19 8:42 ` Petko Manolov
0 siblings, 1 reply; 11+ messages in thread
From: Francois Romieu @ 2013-05-18 21:09 UTC (permalink / raw)
To: Petko Manolov; +Cc: David Miller, netdev, linux-kernel
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.
--
Ueimor
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/5] drivers: net: usb: rtl8150: bug fixing and cleanup
2013-05-18 21:09 ` Francois Romieu
@ 2013-05-19 8:42 ` Petko Manolov
2013-05-19 19:01 ` Francois Romieu
0 siblings, 1 reply; 11+ messages in thread
From: Petko Manolov @ 2013-05-19 8:42 UTC (permalink / raw)
To: Francois Romieu; +Cc: David Miller, netdev, linux-kernel
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/5] drivers: net: usb: rtl8150: bug fixing and cleanup
2013-05-19 8:42 ` Petko Manolov
@ 2013-05-19 19:01 ` Francois Romieu
2013-05-20 6:58 ` Petko Manolov
0 siblings, 1 reply; 11+ messages in thread
From: Francois Romieu @ 2013-05-19 19:01 UTC (permalink / raw)
To: Petko Manolov; +Cc: David Miller, netdev, linux-kernel
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...
--
Ueimor
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/5] drivers: net: usb: rtl8150: bug fixing and cleanup
2013-05-19 19:01 ` Francois Romieu
@ 2013-05-20 6:58 ` Petko Manolov
2013-05-20 7:00 ` David Miller
0 siblings, 1 reply; 11+ messages in thread
From: Petko Manolov @ 2013-05-20 6:58 UTC (permalink / raw)
To: Francois Romieu; +Cc: David Miller, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/5] drivers: net: usb: rtl8150: bug fixing and cleanup
2013-05-20 6:58 ` Petko Manolov
@ 2013-05-20 7:00 ` David Miller
2013-05-20 7:09 ` Petko Manolov
0 siblings, 1 reply; 11+ messages in thread
From: David Miller @ 2013-05-20 7:00 UTC (permalink / raw)
To: petkan; +Cc: romieu, netdev, linux-kernel
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/5] drivers: net: usb: rtl8150: bug fixing and cleanup
2013-05-20 7:00 ` David Miller
@ 2013-05-20 7:09 ` Petko Manolov
2013-05-20 7:12 ` David Miller
0 siblings, 1 reply; 11+ messages in thread
From: Petko Manolov @ 2013-05-20 7:09 UTC (permalink / raw)
To: David Miller; +Cc: romieu, netdev, linux-kernel
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?
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/5] drivers: net: usb: rtl8150: bug fixing and cleanup
2013-05-20 7:09 ` Petko Manolov
@ 2013-05-20 7:12 ` David Miller
2013-05-20 7:18 ` Petko Manolov
0 siblings, 1 reply; 11+ messages in thread
From: David Miller @ 2013-05-20 7:12 UTC (permalink / raw)
To: petkan; +Cc: romieu, netdev, linux-kernel
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/5] drivers: net: usb: rtl8150: bug fixing and cleanup
2013-05-20 7:12 ` David Miller
@ 2013-05-20 7:18 ` Petko Manolov
2013-05-20 7:43 ` David Miller
0 siblings, 1 reply; 11+ messages in thread
From: Petko Manolov @ 2013-05-20 7:18 UTC (permalink / raw)
To: David Miller; +Cc: romieu, netdev, linux-kernel
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?
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/5] drivers: net: usb: rtl8150: bug fixing and cleanup
2013-05-20 7:18 ` Petko Manolov
@ 2013-05-20 7:43 ` David Miller
2013-05-20 7:49 ` Petko Manolov
0 siblings, 1 reply; 11+ messages in thread
From: David Miller @ 2013-05-20 7:43 UTC (permalink / raw)
To: petkan; +Cc: romieu, netdev, linux-kernel
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'.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/5] drivers: net: usb: rtl8150: bug fixing and cleanup
2013-05-20 7:43 ` David Miller
@ 2013-05-20 7:49 ` Petko Manolov
0 siblings, 0 replies; 11+ messages in thread
From: Petko Manolov @ 2013-05-20 7:49 UTC (permalink / raw)
To: David Miller; +Cc: romieu, netdev, linux-kernel
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2013-05-20 7:50 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-05-18 16:23 [PATCH 1/5] drivers: net: usb: rtl8150: bug fixing and cleanup Petko Manolov
2013-05-18 21:09 ` Francois Romieu
2013-05-19 8:42 ` Petko Manolov
2013-05-19 19:01 ` Francois Romieu
2013-05-20 6:58 ` Petko Manolov
2013-05-20 7:00 ` David Miller
2013-05-20 7:09 ` Petko Manolov
2013-05-20 7:12 ` David Miller
2013-05-20 7:18 ` Petko Manolov
2013-05-20 7:43 ` David Miller
2013-05-20 7:49 ` Petko Manolov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®