Re: [PATCH] wan: new driver retina
Not sure what's going on, but your patch looks scrambled, as scripts look at it: From: Matti Linnanvuori [EMAIL PROTECTED]=0A=0AAdding Retina G.7= 03 and G.SHDSL driver.=0A=0ASigned-off-by: Matti Linnanvuori mattilinnanvu= [EMAIL PROTECTED]=0A---=0A=0AFixing bugs and style according to netdev mailing= list comments.=0A=0Adiff -Napur linux-2.6.23/drivers/net/wan/Kconfig linux= -2.6.24/drivers/net/wan/Kconfig=0A--- linux-2.6.23/drivers/net/wan/Kconfig= =092007-10-09 23:31:38.0 +0300=0A+++ linux-2.6.24/drivers/net/wan/K= config=092007-10-25 09:23:19.933170522 +0300=0A@@ -494,4 +494,15 @@ config = SBNI_MULTILINE=0A =0A =09 If unsure, say N.=0A =0A+config RETINA=0A+=09tri= state Retina support=0A+=09depends on PCI=0A+=09help=0A+=09 Driver for R= Its been mangled into Content-Transfer-Encoding: quoted-printable by your mailer. Also, when you revise a patch, please note somehow that this is an updated version. One convention is changing the subject line: [PATCH v3] wan: new driver retina Otherwise I have an mbox full of seemingly-similar patches, all with the subject wan: new driver retina Jeff - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] wan: new driver retina
On Mon, 22 Oct 2007 04:14:57 -0700 (PDT) Matti Linnanvuori wrote: From: Matti Linnanvuori [EMAIL PROTECTED] Retina G.703 and G.SHDSL driver. Signed-off-by: Matti Linnanvuori [EMAIL PROTECTED] --- Hi, Just a few basic questions... I'm not reviewing any of the code parts. Fixed Kconfig and the body of the explanation. diff -Napur linux-2.6.23/drivers/net/wan/Kconfig linux-2.6.24/drivers/net/wan/Kconfig --- linux-2.6.23/drivers/net/wan/Kconfig 2007-10-09 23:31:38.0 +0300 +++ linux-2.6.24/drivers/net/wan/Kconfig 2007-10-22 13:54:55.383644167 +0300 @@ -494,4 +494,15 @@ config SBNI_MULTILINE If unsure, say N. +config RETINA + tristate Retina support + depends on m PCI + help + Driver for Retina C5400 and E2200 network cards, which + support G.703, G.SHDSL, Ethernet encapsulation and PCI. + + The driver will be compiled as a module: the + module will be called retina. Why is the driver restricted/limited to module only? + endif # WAN diff -Napur linux-2.6.23/drivers/net/wan/retina.c linux-2.6.24/drivers/net/wan/retina.c --- linux-2.6.23/drivers/net/wan/retina.c 1970-01-01 02:00:00.0 +0200 +++ linux-2.6.24/drivers/net/wan/retina.c 2007-10-22 12:01:44.856033562 +0300 @@ -0,0 +1,4503 @@ +/* V1.2.4 */ + +/* retina.c: */ + + +#define DRV_NAME retina +#define DRV_VERSION 1.2.5 +#define DRV_RELDATE November 14, 2003 huh? +#if !defined(__OPTIMIZE__) +#warning You must compile this file with correct options. +#warning See the last lines of the source file. +#error You have to compile this driver with -O. +#endif What defines __OPTIMIZE__ in the kernel build environment? diff -Napur linux-2.6.23/drivers/net/wan/retina.h linux-2.6.24/drivers/net/wan/retina.h --- linux-2.6.23/drivers/net/wan/retina.h 1970-01-01 02:00:00.0 +0200 +++ linux-2.6.24/drivers/net/wan/retina.h 2007-10-22 12:01:44.856033562 +0300 @@ -0,0 +1,164 @@ diff -Napur linux-2.6.23/MAINTAINERS linux-2.6.24/MAINTAINERS --- linux-2.6.23/MAINTAINERS 2007-10-09 23:31:38.0 +0300 +++ linux-2.6.24/MAINTAINERS 2007-10-22 11:50:13.190983316 +0300 @@ -3149,6 +3149,11 @@ L: [EMAIL PROTECTED] W: http://www.namesys.com S: Supported +RETINA DRIVER +P: Matti Linnanvuori +M: [EMAIL PROTECTED] and what mailing list? +S: Supported + ROCKETPORT DRIVER P: Comtrol Corp. W: http://www.comtrol.com --- ~Randy - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] wan: new driver retina
A quick look only:
Matti Linnanvuori [EMAIL PROTECTED] writes:
+++ linux-2.6.24/drivers/net/wan/retina.c
+ CHANGES
+ ---
+
+ v1.0.0 (JK) - May 27, 2003:
+ * Original driver.
+
+ v1.1.0 (JK) - June, 2003:
+* final Flexibilis driver
+
+ v1.2.0: NO_ARP option back again
+
+v1.2.1: (JT) - Aug 21, 2003:
+ * Added support for Retina C5400 card including PROC stuff.
+
+ v1.2.2: (Petri Ahonen) - Sep 19, 2003:
And so on - I'm not sure such logs belong here.
+#define DRV_NAME retina
+#define DRV_VERSION 1.2.5
+#define DRV_RELDATE November 14, 2003
Hmm...
+/* obsolete
+ see retina_noarp_with_ptp
+define FEPCI_NO_ARP */
If it's obsolete (or rather unused), just drop it.
+#undef inb
+#undef inw
+#undef inl
+#undef outb
+#undef outw
+#undef outl
+#define inb nonexistent /* force using only 32bit access */
+#define inw nonexistent /* force using only 32bit access */
+#define inl(x) le32_to_cpu(readl(x))
+#define outb nonexistent /* force using only 32bit access */
+#define outw nonexistent /* force using only 32bit access */
+#define outl(value, address) writel(cpu_to_le32(value), address)
Any code like that is write-only, why don't just use readl()/
writel() in the actual code?
Are you sure about this cpu_to_le32? readl()/writel() already
preserve the value.
+#define VMA_OFFSET(vma) ((vma)-vm_pgoff PAGE_SHIFT)
Not sure about such things in a driver.
+enum pci_id_flags_bits {
+ /* Set PCI command register bits before calling probe */
+ PCI_USES_IO = 1, PCI_USES_MEM = 2, PCI_USES_MASTER = 4,
+ /* Read and map the single following PCI BAR */
+ PCI_ADDR0 = 0 4, PCI_ADDR1 = 1 4, PCI_ADDR2 =
+ 2 4, PCI_ADDR3 = 3 4,
+ PCI_ADDR_64BITS = 0x100, PCI_NO_ACPI_WAKE =
+ 0x200, PCI_NO_MIN_LATENCY = 0x400,
+ PCI_UNUSED_IRQ = 0x800,
+};
We already have such things in PCI headers, don't we?
+/* Linux 2.4 appears to drop POINTOPOINT,BROADCAST and NOARP flags
Linux 2.4?
+/* proc filesystem functions introduced: */
I'm not sure we're adding new /proc files.
Perhaps you should investigate sysfs and friends?
+ case FEPCI_IOCTL_R_SHARED_MEM:
+ DBG_PRINT( %s: ioctl read shared mem commanded.\n,
+ fepci_NAME);
+ fepci_copy_to_user(arg, ioaddr + FEPCI_SHARED_MEM_OFFSETT,
+_IOC_SIZE(cmd), 0);
+ break;
+ case FEPCI_IOCTL_W_SHARED_MEM:
+ DBG_PRINT( %s: ioctl write shared mem commanded.\n,
+ fepci_NAME);
+ fepci_copy_from_user(ioaddr + FEPCI_SHARED_MEM_OFFSETT,
+ arg, _IOC_SIZE(cmd), 0);
+ break;
+ case FEPCI_IOCTL_G_IDENTIFICATION:
+ DBG_PRINT( %s: IOCTL_G_IDENTIFICATION commanded.\n,
+ fepci_NAME);
+ fepci_copy_to_user(arg,
+ioaddr + FEPCI_IDENTIFICATION_OFFSETT,
+_IOC_SIZE(cmd), 1);
+ break;
+ case FEPCI_IOCTL_G_FEATURES:
+ DBG_PRINT( %s: IOCTL_G_FEATURES commanded.\n, fepci_NAME);
+ fepci_copy_to_user(arg, ioaddr + FEPCI_FEATURES_OFFSETT,
+_IOC_SIZE(cmd), 1);
+ break;
Are you sure these ioctls are a good idea? Perhaps sysfs attributes
would be much better?
+ if (length == 0) {
+ fp-rx_packets_of_size_0_stream++;
+ } else if (length == 1) {
+ fp-rx_packets_of_size_1_stream++;
+ } else if (length == 2) {
+ fp-rx_packets_of_size_2_stream++;
+ } else if (length == 3) {
+ fp-rx_packets_of_size_3_stream++;
+ } else if (length 8) {
+ fp-rx_packets_of_size_4_7_stream++;
+ } else if (length 16) {
...
I think style details are really a personal thing but this would
look much better without the braces.
+ }
+ temp_tx = (temp_tx + 1) (TX_RING_SIZE - 1);
+ temp_tx_unit = (temp_tx_unit + 1);
+ temp_tx_unit *= temp_tx_unit fp-units;
+ }
+
+ return IRQ_HANDLED;
No unhandled IRQ protection anymore?
+#ifdef FEPCI_POINT_TO_POINT
+static int is_ptp_interface(struct net_device *dev)
+{
+ char **p_ptp_if_name = retina_ptp_interfaces;
+ unsigned int i = interfaces;
+ while (i 0 *p_ptp_if_name != NULL) {
+ if (!strncmp(dev-name, *p_ptp_if_name, sizeof(dev-name))) {
+ return 1;
+ } else {
+ }
+ p_ptp_if_name++;
+ i--;
+ }
+ return 0;
+}
A bit weird, isn't it?
+static int __devinit fepci_init_one(struct pci_dev *pdev,
