From: Petko Manolov <[email protected]>
Moving constant and structure definitions out of rtl8150.c;
Signed-off-by: Petko Manolov <[email protected]>
---
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 <[email protected]>"
#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";
/*
Petko Manolov <[email protected]> :
> From: Petko Manolov <[email protected]>
>
> 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
On Sat, 18 May 2013, Francois Romieu wrote:
> Petko Manolov <[email protected]> :
> > From: Petko Manolov <[email protected]>
> >
> > 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.
Petko Manolov <[email protected]> :
> On Sat, 18 May 2013, Francois Romieu wrote:
> > > From: Petko Manolov <[email protected]>
[...]
> > > 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
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
From: Petko Manolov <[email protected]>
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.
On Mon, 20 May 2013, David Miller wrote:
> From: Petko Manolov <[email protected]>
> 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?
From: Petko Manolov <[email protected]>
Date: Mon, 20 May 2013 10:09:00 +0300 (EEST)
> What about the .c/.h split?
I have no strong opinion either way.
On Mon, 20 May 2013, David Miller wrote:
> From: Petko Manolov <[email protected]>
> 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?
From: Petko Manolov <[email protected]>
Date: Mon, 20 May 2013 10:18:17 +0300 (EEST)
> On Mon, 20 May 2013, David Miller wrote:
>
>> From: Petko Manolov <[email protected]>
>> 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'.
On Mon, 20 May 2013, David Miller wrote:
> From: Petko Manolov <[email protected]>
> Date: Mon, 20 May 2013 10:18:17 +0300 (EEST)
>
> > On Mon, 20 May 2013, David Miller wrote:
> >
> >> From: Petko Manolov <[email protected]>
> >> 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.