Message ID | alpine.DEB.2.02.1305181902080.7518@fry.nucleusys.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
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.
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
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...
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
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
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
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
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
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
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 --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"; /*