drivers/serial/8250_pci.c (add support for '8-port RS-232 MIC-3620 from advantech'

PATCH NAK Michael Bramer 6 participants 17 messages Jan 18, 2009 – Jan 22, 2009 TTY layer and serial drivers

lore

Share & tools

lore

Michael Bramer

author

2009-01-18 14:04 UTC #0 lore raw

PATCH 1/1 drivers/serial/8250_pci.c (add support for '8-port RS-232 MIC-3620 from advantech'

Hello

This is my first mail to this list... sorry for all my mistakes

We use a 8-port RS-232 MIC-3620 from advantech (see
http://www.advantech.com/products/8-port-RS-232-Communication-CPCI-Card/mod_1-2MLG80.aspx)
and need linux support for this card.

This patch add is:
-------------------------------------

drivers/serial/8250_pci.c +15 -0

@@ -768,6 +769,8 @@
 #define PCI_SUBDEVICE_ID_OCTPRO422      0x0208
 #define PCI_SUBDEVICE_ID_POCTAL232      0x0308
 #define PCI_SUBDEVICE_ID_POCTAL422      0x0408
+#define PCI_VENDOR_ID_ADVANTECH         0x13fe
+#define PCI_DEVICE_ID_ADVANTECH_PCI3620 0x3620
 
 /*
  * Master list of serial port init/setup/exit quirks.

@@ -789,6 +792,16 @@
                .setup          = addidata_apci7800_setup,
        },
        /*
+        * ADVANTECH
+        */
+       {
+               .vendor         = PCI_VENDOR_ID_ADVANTECH,
+               .device         = PCI_DEVICE_ID_ADVANTECH_PCI3620,
+               .subvendor      = PCI_DEVICE_ID_ADVANTECH_PCI3620,
+               .subdevice      = PCI_ANY_ID,
+               .setup          = pci_default_setup,
+       },
+       /*
         * AFAVLAB cards - these may be called via parport_serial
         *  It is not clear whether this applies to all products.
         */

@@ -2041,6 +2054,9 @@
 #endif
 
 static struct pci_device_id serial_pci_tbl[] = {
+       {       PCI_VENDOR_ID_ADVANTECH, PCI_DEVICE_ID_ADVANTECH_PCI3620,
+               PCI_DEVICE_ID_ADVANTECH_PCI3620, PCI_ANY_ID, 0, 0,
+               pbn_b2_8_921600 },
        {       PCI_VENDOR_ID_V3, PCI_DEVICE_ID_V3_V960,
                PCI_SUBVENDOR_ID_CONNECT_TECH,
                PCI_SUBDEVICE_ID_CONNECT_TECH_BH8_232, 0, 0,

-------------------------------------

can you add this in the next linux kernel?

Thanks for linux!

Gruss
Grisu

Signature · 8 lines

Michael Bramer  -- http://www.feuerwehr.kreuzau.de/wiki/
PGP: finger [email protected]  -- Linux Sysadmin   -- Use Debian Linux
"Wenn ich die Folgen geahnt hätte, wäre ich Uhrmacher geworden!"
                                                 --- Albert Einstein
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Alan Cox

2009-01-18 14:13 UTC #1 lore raw

drivers/serial/8250_pci.c line 801

Quoted 4 lines

drivers/serial/8250_pci.c +4 -0

+               .vendor         = PCI_VENDOR_ID_ADVANTECH,
+               .device         = PCI_DEVICE_ID_ADVANTECH_PCI3620,
+               .subvendor      = PCI_DEVICE_ID_ADVANTECH_PCI3620,
+               .subdevice      = PCI_ANY_ID,

This looks odd - the subvendor ought to be a vendor id, and there should be a device id as well.

Quoted 9 lines

drivers/serial/8250_pci.c +3 -1

+       {       PCI_VENDOR_ID_ADVANTECH, PCI_DEVICE_ID_ADVANTECH_PCI3620,
+               PCI_DEVICE_ID_ADVANTECH_PCI3620, PCI_ANY_ID, 0, 0,
+               pbn_b2_8_921600 },
        {       PCI_VENDOR_ID_V3, PCI_DEVICE_ID_V3_V960,
                PCI_SUBVENDOR_ID_CONNECT_TECH,
                PCI_SUBDEVICE_ID_CONNECT_TECH_BH8_232, 0, 0,
-------------------------------------

can you add this in the next linux kernel?

Can you firstly send me an lspci -vvxxx of that device. Also see Documentation/SubmittingPatches about Signed-off-by lines and we'll sort out merging it.

Alan

Michael Bramer

author

2009-01-19 11:45 UTC #2 lore raw

Quoted 7 lines · Alan Cox

Review
Quoted 4 lines
(quoted diff) +4 -0
+               .vendor         = PCI_VENDOR_ID_ADVANTECH,
+               .device         = PCI_DEVICE_ID_ADVANTECH_PCI3620,
+               .subvendor      = PCI_DEVICE_ID_ADVANTECH_PCI3620,
+               .subdevice      = PCI_ANY_ID,
This looks odd - the subvendor ought to be a vendor id, and there should be a device id as well.

the number ist the same... i use now numbers in the patch...

Quoted 5 lines

Quoted 1 line
can you add this in the next linux kernel?
Can you firstly send me an lspci -vvxxx of that device. Also see Documentation/SubmittingPatches about Signed-off-by lines and we'll sort out merging it.

done:

eps:~# lspci -vvxxx -d 13fe:3620
04:0d.0 Serial controller: Advantech Co. Ltd Device 3620 (rev 01) (prog-if 00 [8250])
        Subsystem: Device 3620:0001
        Control: I/O+ Mem+ BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
        Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
        Interrupt: pin A routed to IRQ 11
        Region 0: Memory at fddff000 (32-bit, non-prefetchable) [size=128]
        Region 2: I/O ports at df00 [size=128]
        Capabilities: [40] Power Management version 1
                Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold-)
                Status: D0 PME-Enable- DSel=0 DScale=0 PME-
        Capabilities: [48] CompactPCI hot-swap <?>
        Capabilities: [4c] Vital Product Data <?>
        Kernel modules: 8250_pci-, 8250_pci
00: fe 13 20 36 03 00 90 02 01 00 00 07 10 00 00 00
10: 00 f0 df fd 00 00 00 00 01 df 00 00 00 00 00 00
20: 00 00 00 00 00 00 00 00 00 00 00 00 20 36 01 00
30: 00 00 00 00 40 00 00 00 00 00 00 00 0b 01 00 00
40: 01 48 01 48 00 00 00 00 06 4c 00 00 03 00 00 00
50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

and now the patch:
--------------
This Patch add the device information for the MIC-3620 8-port RS-232 cPCI card from Advantech Co. Ltd.

Signed-off-by Michael Bramer <[email protected]>

drivers/serial/8250_pci.c +16 -0

@@ -31,6 +31,7 @@
 #include "8250.h"
 
 #undef SERIAL_DEBUG_PCI
+#define SERIAL_DEBUG_PCI
 
 /*
  * init function returns:

@@ -768,6 +769,8 @@
 #define PCI_SUBDEVICE_ID_OCTPRO422     0x0208
 #define PCI_SUBDEVICE_ID_POCTAL232     0x0308
 #define PCI_SUBDEVICE_ID_POCTAL422     0x0408
+#define PCI_VENDOR_ID_ADVANTECH         0x13fe
+#define PCI_DEVICE_ID_ADVANTECH_PCI3620 0x3620
 
 /*
  * Master list of serial port init/setup/exit quirks.

@@ -789,6 +792,16 @@
                .setup          = addidata_apci7800_setup,
        },
        /*
+        * ADVANTECH
+        */
+       {
+               .vendor         = PCI_VENDOR_ID_ADVANTECH,
+               .device         = PCI_DEVICE_ID_ADVANTECH_PCI3620,
+               .subvendor      = 0x3620,
+               .subdevice      = PCI_ANY_ID,
+               .setup          = pci_default_setup,
+       },
+       /*
         * AFAVLAB cards - these may be called via parport_serial
         *  It is not clear whether this applies to all products.
         */

@@ -2041,6 +2054,9 @@
 #endif
 
 static struct pci_device_id serial_pci_tbl[] = {
+       {       PCI_VENDOR_ID_ADVANTECH, PCI_DEVICE_ID_ADVANTECH_PCI3620,
+               PCI_DEVICE_ID_ADVANTECH_PCI3620, PCI_ANY_ID, 0, 0,
+               pbn_b2_8_921600 },
        {       PCI_VENDOR_ID_V3, PCI_DEVICE_ID_V3_V960,
                PCI_SUBVENDOR_ID_CONNECT_TECH,
                PCI_SUBDEVICE_ID_CONNECT_TECH_BH8_232, 0, 0,

--------------

Thanks for your help

Gruss
Grisu

Signature · 8 lines

Michael Bramer  -- http://www.feuerwehr.kreuzau.de/wiki/
PGP: finger [email protected]  -- Linux Sysadmin   -- Use Debian Linux
"Wenn ich die Folgen geahnt hätte, wäre ich Uhrmacher geworden!"
                                                 --- Albert Einstein
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Andrey Panin

2009-01-19 14:51 UTC #3 lore raw

In reply to Michael Bramer

Quoted 28 lines · Michael Bramer

Hello
This is my first mail to this list... sorry for all my mistakes
We use a 8-port RS-232 MIC-3620 from advantech (see
http://www.advantech.com/products/8-port-RS-232-Communication-CPCI-Card/mod_1-2MLG80.aspx)
and need linux support for this card.
This patch add is:
-------------------------------------
drivers/serial/8250_pci.c +5 -0
@@ -768,6 +769,8 @@
 #define PCI_SUBDEVICE_ID_OCTPRO422      0x0208
 #define PCI_SUBDEVICE_ID_POCTAL232      0x0308
 #define PCI_SUBDEVICE_ID_POCTAL422      0x0408
+#define PCI_VENDOR_ID_ADVANTECH         0x13fe
+#define PCI_DEVICE_ID_ADVANTECH_PCI3620 0x3620
 
 /*
  * Master list of serial port init/setup/exit quirks.
@@ -789,6 +792,16 @@
                .setup          = addidata_apci7800_setup,
        },
        /*
+        * ADVANTECH
+        */
+       {

drivers/serial/8250_pci.c line 807

Quoted 10 lines

drivers/serial/8250_pci.c +7 -0

+               .vendor         = PCI_VENDOR_ID_ADVANTECH,
+               .device         = PCI_DEVICE_ID_ADVANTECH_PCI3620,
+               .subvendor      = PCI_DEVICE_ID_ADVANTECH_PCI3620,
+               .subdevice      = PCI_ANY_ID,
+               .setup          = pci_default_setup,
+       },
+       /*
         * AFAVLAB cards - these may be called via parport_serial
         *  It is not clear whether this applies to all products.
         */

This part is not needed at all, pci_default_setup() will be used by default.

Quoted 29 lines

drivers/serial/8250_pci.c +3 -1

@@ -2041,6 +2054,9 @@
 #endif
 
 static struct pci_device_id serial_pci_tbl[] = {
+       {       PCI_VENDOR_ID_ADVANTECH, PCI_DEVICE_ID_ADVANTECH_PCI3620,
+               PCI_DEVICE_ID_ADVANTECH_PCI3620, PCI_ANY_ID, 0, 0,
+               pbn_b2_8_921600 },
        {       PCI_VENDOR_ID_V3, PCI_DEVICE_ID_V3_V960,
                PCI_SUBVENDOR_ID_CONNECT_TECH,
                PCI_SUBDEVICE_ID_CONNECT_TECH_BH8_232, 0, 0,
-------------------------------------

can you add this in the next linux kernel?

Thanks for linux!

Gruss
Grisu

Signature · 9 lines

Michael Bramer  -- http://www.feuerwehr.kreuzau.de/wiki/
PGP: finger [email protected]  -- Linux Sysadmin   -- Use Debian Linux
"Wenn ich die Folgen geahnt h?tte, w?re ich Uhrmacher geworden!"
                                                 --- Albert Einstein
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Michael Bramer

author

2009-01-19 23:51 UTC #4 lore raw

Quoted 33 lines · Andrey Panin

Quoted 32 lines · Michael Bramer

Hello

This is my first mail to this list... sorry for all my mistakes

We use a 8-port RS-232 MIC-3620 from advantech (see
http://www.advantech.com/products/8-port-RS-232-Communication-CPCI-Card/mod_1-2MLG80.aspx)
and need linux support for this card.

This patch add is:
-------------------------------------

drivers/serial/8250_pci.c +9 -0

@@ -768,6 +769,8 @@
 #define PCI_SUBDEVICE_ID_OCTPRO422      0x0208
 #define PCI_SUBDEVICE_ID_POCTAL232      0x0308
 #define PCI_SUBDEVICE_ID_POCTAL422      0x0408
+#define PCI_VENDOR_ID_ADVANTECH         0x13fe
+#define PCI_DEVICE_ID_ADVANTECH_PCI3620 0x3620
 
 /*
  * Master list of serial port init/setup/exit quirks.

@@ -789,6 +792,16 @@
                .setup          = addidata_apci7800_setup,
        },
        /*
+        * ADVANTECH
+        */
+       {
+               .vendor         = PCI_VENDOR_ID_ADVANTECH,
+               .device         = PCI_DEVICE_ID_ADVANTECH_PCI3620,
+               .subvendor      = PCI_DEVICE_ID_ADVANTECH_PCI3620,
+               .subdevice      = PCI_ANY_ID,

Quoted 8 lines

drivers/serial/8250_pci.c line 807
Quoted 6 lines
drivers/serial/8250_pci.c +3 -0
+               .setup          = pci_default_setup,
+       },
+       /*
         * AFAVLAB cards - these may be called via parport_serial
         *  It is not clear whether this applies to all products.
         */
This part is not needed at all, pci_default_setup() will be used by default.

ok. I don't test this. But you are right. The last entry in pci_serial_quirks should catch it.

This make the patch to a 'add only 5 lines for support of 8 Ports'-Patch. :-)

Should I post a new patch, without this 10 lines?

BTW: there are one other card's with a part like this:
- PCI_DEVICE_ID_PLX_9030

Gruss
Grisu

Signature · 8 lines

Michael Bramer  -- http://www.feuerwehr.kreuzau.de/wiki/
PGP: finger [email protected]  -- Linux Sysadmin   -- Use Debian Linux
"Wenn ich die Folgen geahnt hätte, wäre ich Uhrmacher geworden!"
                                                 --- Albert Einstein
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Andrey Panin

2009-01-20 15:12 UTC #5 lore raw

Quoted 43 lines · Michael Bramer

Quoted 33 lines · Andrey Panin
Quoted 32 lines · Michael Bramer
Hello
This is my first mail to this list... sorry for all my mistakes
We use a 8-port RS-232 MIC-3620 from advantech (see
http://www.advantech.com/products/8-port-RS-232-Communication-CPCI-Card/mod_1-2MLG80.aspx)
and need linux support for this card.
This patch add is:
-------------------------------------
drivers/serial/8250_pci.c +9 -0
@@ -768,6 +769,8 @@
 #define PCI_SUBDEVICE_ID_OCTPRO422      0x0208
 #define PCI_SUBDEVICE_ID_POCTAL232      0x0308
 #define PCI_SUBDEVICE_ID_POCTAL422      0x0408
+#define PCI_VENDOR_ID_ADVANTECH         0x13fe
+#define PCI_DEVICE_ID_ADVANTECH_PCI3620 0x3620
 
 /*
  * Master list of serial port init/setup/exit quirks.
@@ -789,6 +792,16 @@
                .setup          = addidata_apci7800_setup,
        },
        /*
+        * ADVANTECH
+        */
+       {
+               .vendor         = PCI_VENDOR_ID_ADVANTECH,
+               .device         = PCI_DEVICE_ID_ADVANTECH_PCI3620,
+               .subvendor      = PCI_DEVICE_ID_ADVANTECH_PCI3620,
+               .subdevice      = PCI_ANY_ID,
Quoted 8 lines
drivers/serial/8250_pci.c line 807
Quoted 6 lines
drivers/serial/8250_pci.c +3 -0
+               .setup          = pci_default_setup,
+       },
+       /*
         * AFAVLAB cards - these may be called via parport_serial
         *  It is not clear whether this applies to all products.
         */
This part is not needed at all, pci_default_setup() will be used by default.

Quoted 7 lines

ok. I don't test this. But you are right. The last entry in pci_serial_quirks should catch it.
This make the patch to a 'add only 5 lines for support of 8 Ports'-Patch. :-)
Should I post a new patch, without this 10 lines?

Yes and add Signed-off-by: line please.

Quoted 2 lines

BTW: there are one other card's with a part like this:
- PCI_DEVICE_ID_PLX_9030

We can safely remove it.

Quoted 7 lines

Gruss
Grisu

Signature · 4 lines

Michael Bramer  -- http://www.feuerwehr.kreuzau.de/wiki/
PGP: finger [email protected]  -- Linux Sysadmin   -- Use Debian Linux
"Wenn ich die Folgen geahnt h?tte, w?re ich Uhrmacher geworden!"
                                                 --- Albert Einstein

Michael Bramer

author

2009-01-21 07:52 UTC #6 lore raw

Quoted 17 lines · Andrey Panin

Quoted 16 lines · Michael Bramer

Quoted 15 lines · Andrey Panin

Quoted 13 lines · Michael Bramer

(quoted diff) +10 -0

+        * ADVANTECH
+        */
+       {
+               .vendor         = PCI_VENDOR_ID_ADVANTECH,
+               .device         = PCI_DEVICE_ID_ADVANTECH_PCI3620,
+               .subvendor      = PCI_DEVICE_ID_ADVANTECH_PCI3620,
+               .subdevice      = PCI_ANY_ID,
+               .setup          = pci_default_setup,
+       },
+       /*
         * AFAVLAB cards - these may be called via parport_serial
         *  It is not clear whether this applies to all products.
         */

Quoted 8 lines

Quoted 6 lines
Quoted 1 line
This part is not needed at all, pci_default_setup() will be used by default.
ok. I don't test this. But you are right. The last entry in pci_serial_quirks should catch it.
Should I post a new patch, without this 10 lines?
Yes and add Signed-off-by: line please.

the new patch:
--------------
This Patch add the device information for the MIC-3620 8-port RS-232 cPCI card from Advantech Co. Ltd.

Signed-off-by Michael Bramer <[email protected]>

drivers/serial/8250_pci.c +6 -0

@@ -31,6 +31,7 @@
 #include "8250.h"
 
 #undef SERIAL_DEBUG_PCI
+#define SERIAL_DEBUG_PCI
 
 /*
  * init function returns:

@@ -768,6 +769,8 @@
 #define PCI_SUBDEVICE_ID_OCTPRO422     0x0208
 #define PCI_SUBDEVICE_ID_POCTAL232     0x0308
 #define PCI_SUBDEVICE_ID_POCTAL422     0x0408
+#define PCI_VENDOR_ID_ADVANTECH         0x13fe
+#define PCI_DEVICE_ID_ADVANTECH_PCI3620 0x3620
 
 /*
  * Master list of serial port init/setup/exit quirks.

@@ -2041,6 +2054,9 @@
 #endif
 
 static struct pci_device_id serial_pci_tbl[] = {
+       {       PCI_VENDOR_ID_ADVANTECH, PCI_DEVICE_ID_ADVANTECH_PCI3620,
+               0x3620, PCI_ANY_ID, 0, 0,
+               pbn_b2_8_921600 },
        {       PCI_VENDOR_ID_V3, PCI_DEVICE_ID_V3_V960,
                PCI_SUBVENDOR_ID_CONNECT_TECH,
                PCI_SUBDEVICE_ID_CONNECT_TECH_BH8_232, 0, 0,

--------------

Gruss
Grisu

Signature · 8 lines

Michael Bramer  -- http://www.feuerwehr.kreuzau.de/wiki/
PGP: finger [email protected]  -- Linux Sysadmin   -- Use Debian Linux
"Wenn ich die Folgen geahnt hätte, wäre ich Uhrmacher geworden!"
                                                 --- Albert Einstein
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Niels de Vos

2009-01-21 08:40 UTC #8 lore raw

Quoted 1 line · Paulius Zaleckas

Michael Bramer wrote:

...

Quoted 14 lines

drivers/serial/8250_pci.c line 34
Quoted 11 lines
This Patch add the device information for the MIC-3620 8-port RS-232 cPCI card from Advantech Co. Ltd.
Signed-off-by Michael Bramer <[email protected]>
drivers/serial/8250_pci.c +1 -0
@@ -31,6 +31,7 @@
 #include "8250.h"
 
 #undef SERIAL_DEBUG_PCI
+#define SERIAL_DEBUG_PCI
NAK.

Quoted 8 lines

drivers/serial/8250_pci.c line 773

Quoted 8 lines

drivers/serial/8250_pci.c +2 -0

 /*
  * init function returns:

@@ -768,6 +769,8 @@
 #define PCI_SUBDEVICE_ID_OCTPRO422     0x0208
 #define PCI_SUBDEVICE_ID_POCTAL232     0x0308
 #define PCI_SUBDEVICE_ID_POCTAL422     0x0408
+#define PCI_VENDOR_ID_ADVANTECH         0x13fe
+#define PCI_DEVICE_ID_ADVANTECH_PCI3620 0x3620

Should this not be in <include/linux/pci_ids.h>?

Quoted 8 lines

drivers/serial/8250_pci.c line 2058

Quoted 8 lines

drivers/serial/8250_pci.c +2 -0

 /*
  * Master list of serial port init/setup/exit quirks.

@@ -2041,6 +2054,9 @@
 #endif
 
 static struct pci_device_id serial_pci_tbl[] = {
+       {       PCI_VENDOR_ID_ADVANTECH, PCI_DEVICE_ID_ADVANTECH_PCI3620,
+               0x3620, PCI_ANY_ID, 0, 0,

Why not use PCI_VENDOR_ID_ADVANTECH as PCI_SUBVENDOR_ID too?

Cheers,
Niels

Alan Cox

2009-01-21 12:32 UTC #9 lore raw

Quoted 5 lines

Quoted 3 lines
drivers/serial/8250_pci.c line 2058
Quoted 3 lines
static struct pci_device_id serial_pci_tbl[] = {
drivers/serial/8250_pci.c +2 -0
+       {       PCI_VENDOR_ID_ADVANTECH, PCI_DEVICE_ID_ADVANTECH_PCI3620,
+               0x3620, PCI_ANY_ID, 0, 0,
Why not use PCI_VENDOR_ID_ADVANTECH as PCI_SUBVENDOR_ID too?

The Advantech vendor id is not 0x3620. This confused me as well which is why I asked for an lspci. Advantech has stuck the device id in the subvendor bits and '1' in the subdevice (so it should be 1 not PCI_ANY_ID).

Alan

Michael Bramer

author

2009-01-21 23:42 UTC #10 lore raw

Quoted 10 lines · Alan Cox

Quoted 5 lines
Quoted 3 lines
drivers/serial/8250_pci.c line 2058
Quoted 3 lines
static struct pci_device_id serial_pci_tbl[] = {
drivers/serial/8250_pci.c +2 -0
+       {       PCI_VENDOR_ID_ADVANTECH, PCI_DEVICE_ID_ADVANTECH_PCI3620,
+               0x3620, PCI_ANY_ID, 0, 0,
Why not use PCI_VENDOR_ID_ADVANTECH as PCI_SUBVENDOR_ID too?
The Advantech vendor id is not 0x3620. This confused me as well which is why I asked for an lspci. Advantech has stuck the device id in the subvendor bits and '1' in the subdevice (so it should be 1 not PCI_ANY_ID).

is this better?

+ { PCI_VENDOR_ID_ADVANTECH, PCI_DEVICE_ID_ADVANTECH_PCI3620,
+ PCI_DEVICE_ID_ADVANTECH_PCI3620, 1, 0, 0,

Gruss
Grisu

Signature · 8 lines

Michael Bramer  -- http://www.feuerwehr.kreuzau.de/wiki/
PGP: finger [email protected]  -- Linux Sysadmin   -- Use Debian Linux
"Wenn ich die Folgen geahnt hätte, wäre ich Uhrmacher geworden!"
                                                 --- Albert Einstein
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tosoni

2009-01-22 08:43 UTC #11 lore raw

Quoted 16 lines

[mailto:[email protected]]On Behalf Of Michael Bramer
Quoted 2 lines · Alan Cox
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
static struct pci_device_id serial_pci_tbl[] = {
+ { PCI_VENDOR_ID_ADVANTECH,
PCI_DEVICE_ID_ADVANTECH_PCI3620,
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Quoted 1 line
+ 0x3620, PCI_ANY_ID, 0, 0,
Why not use PCI_VENDOR_ID_ADVANTECH as PCI_SUBVENDOR_ID too?
The Advantech vendor id is not 0x3620. This confused me as
well which is
Quoted 3 lines
why I asked for an lspci. Advantech has stuck the device id in the subvendor bits and '1' in the subdevice (so it should be 1 not PCI_ANY_ID).

Quoted 5 lines

is this better?
+ { PCI_VENDOR_ID_ADVANTECH,
PCI_DEVICE_ID_ADVANTECH_PCI3620,
+ PCI_DEVICE_ID_ADVANTECH_PCI3620, 1, 0, 0,

Since the name describes a device id where it should be a (sub)vendor id, I would suggest that you add a line of comment to explain the case. So that no one will be tempted to change it back to PCI_VENDOR_ID_ADVANTECH in the future.

Regards

Niels de Vos

2009-01-22 09:07 UTC #12 lore raw

Quoted 13 lines · Tosoni

Quoted 13 lines · Alan Cox
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
static struct pci_device_id serial_pci_tbl[] = {
+ { PCI_VENDOR_ID_ADVANTECH,
PCI_DEVICE_ID_ADVANTECH_PCI3620,
Quoted 3 lines
Quoted 2 lines
Quoted 1 line
Quoted 1 line
+ 0x3620, PCI_ANY_ID, 0, 0,
Why not use PCI_VENDOR_ID_ADVANTECH as PCI_SUBVENDOR_ID too?
The Advantech vendor id is not 0x3620. This confused me as
well which is
Quoted 3 lines
why I asked for an lspci. Advantech has stuck the device id in the subvendor bits and '1' in the subdevice (so it should be 1 not PCI_ANY_ID).
is this better?

Quoted 8 lines

Quoted 3 lines
+ { PCI_VENDOR_ID_ADVANTECH,
PCI_DEVICE_ID_ADVANTECH_PCI3620,
+ PCI_DEVICE_ID_ADVANTECH_PCI3620, 1, 0, 0,
Since the name describes a device id where it should be a (sub)vendor id, I would suggest that you add a line of comment to explain the case. So that no one will be tempted to change it back to PCI_VENDOR_ID_ADVANTECH in the future.

Definitely!

Niels

Michael Bramer

author

2009-01-22 11:00 UTC #13 lore raw

Quoted 18 lines · Niels de Vos

Quoted 17 lines · Tosoni
Quoted 16 lines · Alan Cox
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
static struct pci_device_id serial_pci_tbl[] = {
+ { PCI_VENDOR_ID_ADVANTECH,
PCI_DEVICE_ID_ADVANTECH_PCI3620,
Quoted 3 lines
Quoted 2 lines
Quoted 1 line
Quoted 1 line
+ 0x3620, PCI_ANY_ID, 0, 0,
Why not use PCI_VENDOR_ID_ADVANTECH as PCI_SUBVENDOR_ID too?
The Advantech vendor id is not 0x3620. This confused me as
well which is
Quoted 3 lines
why I asked for an lspci. Advantech has stuck the device id in the subvendor bits and '1' in the subdevice (so it should be 1 not PCI_ANY_ID).
is this better?
+ { PCI_VENDOR_ID_ADVANTECH,
PCI_DEVICE_ID_ADVANTECH_PCI3620,
+ PCI_DEVICE_ID_ADVANTECH_PCI3620, 1, 0, 0,

Quoted 6 lines

Quoted 4 lines
Since the name describes a device id where it should be a (sub)vendor id, I would suggest that you add a line of comment to explain the case. So that no one will be tempted to change it back to PCI_VENDOR_ID_ADVANTECH in the future.
Definitely!

Is the patch now ok?

----
This Patch add the device information for the MIC-3620 8-port RS-232 cPCI card from Advantech Co. Ltd.

Signed-off-by Michael Bramer <[email protected]>

drivers/serial/8250_pci.c +6 -0

@@ -769,6 +768,8 @@
 #define PCI_SUBDEVICE_ID_OCTPRO422	0x0208
 #define PCI_SUBDEVICE_ID_POCTAL232	0x0308
 #define PCI_SUBDEVICE_ID_POCTAL422	0x0408
+#define PCI_VENDOR_ID_ADVANTECH         0x13fe
+#define PCI_DEVICE_ID_ADVANTECH_PCI3620 0x3620
 
 /*
  * Master list of serial port init/setup/exit quirks.

@@ -2054,6 +2041,10 @@
 #endif
 
 static struct pci_device_id serial_pci_tbl[] = {
+	// Advantech use PCI_DEVICE_ID_ADVANTECH_PCI3620 (0x3620) as 'PCI_SUBVENDOR_ID'
+	{	PCI_VENDOR_ID_ADVANTECH, PCI_DEVICE_ID_ADVANTECH_PCI3620,
+		PCI_DEVICE_ID_ADVANTECH_PCI3620, 1, 0, 0,
+		pbn_b2_8_921600 },
 	{	PCI_VENDOR_ID_V3, PCI_DEVICE_ID_V3_V960,
 		PCI_SUBVENDOR_ID_CONNECT_TECH,
 		PCI_SUBDEVICE_ID_CONNECT_TECH_BH8_232, 0, 0,

----

Gruss
Grisu

Signature · 8 lines

Michael Bramer  -- http://www.feuerwehr.kreuzau.de/wiki/
PGP: finger [email protected]  -- Linux Sysadmin   -- Use Debian Linux
"Wenn ich die Folgen geahnt hätte, wäre ich Uhrmacher geworden!"
                                                 --- Albert Einstein
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Paulius Zaleckas

2009-01-22 11:47 UTC #14 lore raw

Quoted 18 lines · Michael Bramer

Quoted 17 lines · Niels de Vos
Quoted 16 lines · Tosoni
Quoted 16 lines · Alan Cox
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
static struct pci_device_id serial_pci_tbl[] = {
+ { PCI_VENDOR_ID_ADVANTECH,
PCI_DEVICE_ID_ADVANTECH_PCI3620,
Quoted 3 lines
Quoted 2 lines
Quoted 1 line
Quoted 1 line
+ 0x3620, PCI_ANY_ID, 0, 0,
Why not use PCI_VENDOR_ID_ADVANTECH as PCI_SUBVENDOR_ID too?
The Advantech vendor id is not 0x3620. This confused me as
well which is
Quoted 3 lines
why I asked for an lspci. Advantech has stuck the device id in the subvendor bits and '1' in the subdevice (so it should be 1 not PCI_ANY_ID).
is this better?
+ { PCI_VENDOR_ID_ADVANTECH,
PCI_DEVICE_ID_ADVANTECH_PCI3620,
+ PCI_DEVICE_ID_ADVANTECH_PCI3620, 1, 0, 0,

Quoted 7 lines

Quoted 5 lines
Quoted 4 lines
Since the name describes a device id where it should be a (sub)vendor id, I would suggest that you add a line of comment to explain the case. So that no one will be tempted to change it back to PCI_VENDOR_ID_ADVANTECH in the future.
Definitely!
Is the patch now ok?

No :)

drivers/serial/8250_pci.c line 772

Quoted 13 lines

----
This Patch add the device information for the MIC-3620 8-port RS-232 cPCI card from Advantech Co. Ltd.
Signed-off-by Michael Bramer <[email protected]>
drivers/serial/8250_pci.c +2 -0
@@ -769,6 +768,8 @@
 #define PCI_SUBDEVICE_ID_OCTPRO422	0x0208
 #define PCI_SUBDEVICE_ID_POCTAL232	0x0308
 #define PCI_SUBDEVICE_ID_POCTAL422	0x0408
+#define PCI_VENDOR_ID_ADVANTECH         0x13fe
+#define PCI_DEVICE_ID_ADVANTECH_PCI3620 0x3620

Use TABs for indentation here.

Quoted 18 lines

drivers/serial/8250_pci.c +4 -1

 /*
  * Master list of serial port init/setup/exit quirks.

@@ -2054,6 +2041,10 @@
 #endif
 
 static struct pci_device_id serial_pci_tbl[] = {
+	// Advantech use PCI_DEVICE_ID_ADVANTECH_PCI3620 (0x3620) as 'PCI_SUBVENDOR_ID'
+	{	PCI_VENDOR_ID_ADVANTECH, PCI_DEVICE_ID_ADVANTECH_PCI3620,
+		PCI_DEVICE_ID_ADVANTECH_PCI3620, 1, 0, 0,
+		pbn_b2_8_921600 },
 	{	PCI_VENDOR_ID_V3, PCI_DEVICE_ID_V3_V960,
 		PCI_SUBVENDOR_ID_CONNECT_TECH,
 		PCI_SUBDEVICE_ID_CONNECT_TECH_BH8_232, 0, 0,
----

Gruss
Grisu

Michael Bramer

author

2009-01-22 16:44 UTC #15 lore raw

Quoted 8 lines · Paulius Zaleckas

Quoted 1 line
Is the patch now ok?
No :)
Quoted 4 lines
----
This Patch add the device information for the MIC-3620 8-port RS-232 cPCI card from Advantech Co. Ltd.

Quoted 11 lines

drivers/serial/8250_pci.c line 772
Quoted 9 lines
Signed-off-by Michael Bramer <[email protected]>
drivers/serial/8250_pci.c +2 -0
@@ -769,6 +768,8 @@
 #define PCI_SUBDEVICE_ID_OCTPRO422	0x0208
 #define PCI_SUBDEVICE_ID_POCTAL232	0x0308
 #define PCI_SUBDEVICE_ID_POCTAL422	0x0408
+#define PCI_VENDOR_ID_ADVANTECH         0x13fe
+#define PCI_DEVICE_ID_ADVANTECH_PCI3620 0x3620
Use TABs for indentation here.

ok:

-----
This Patch add the device information for the MIC-3620 8-port RS-232 cPCI card from Advantech Co. Ltd.

Signed-off-by Michael Bramer <[email protected]>

drivers/serial/8250_pci.c +6 -0

@@ -769,6 +768,8 @@
 #define PCI_SUBDEVICE_ID_OCTPRO422	0x0208
 #define PCI_SUBDEVICE_ID_POCTAL232	0x0308
 #define PCI_SUBDEVICE_ID_POCTAL422	0x0408
+#define PCI_VENDOR_ID_ADVANTECH	0x13fe
+#define PCI_DEVICE_ID_ADVANTECH_PCI3620	0x3620
 
 /*
  * Master list of serial port init/setup/exit quirks.

@@ -2054,6 +2041,10 @@
 #endif
 
 static struct pci_device_id serial_pci_tbl[] = {
+	// Advantech use PCI_DEVICE_ID_ADVANTECH_PCI3620 (0x3620) as 'PCI_SUBVENDOR_ID'
+	{	PCI_VENDOR_ID_ADVANTECH, PCI_DEVICE_ID_ADVANTECH_PCI3620,
+		PCI_DEVICE_ID_ADVANTECH_PCI3620, 1, 0, 0,
+		pbn_b2_8_921600 },
 	{	PCI_VENDOR_ID_V3, PCI_DEVICE_ID_V3_V960,
 		PCI_SUBVENDOR_ID_CONNECT_TECH,
 		PCI_SUBDEVICE_ID_CONNECT_TECH_BH8_232, 0, 0,

-----

Gruss
Grisu

Signature · 4 lines

Michael Bramer  -- http://www.feuerwehr.kreuzau.de/wiki/
PGP: finger [email protected]  -- Linux Sysadmin   -- Use Debian Linux
"Wenn ich die Folgen geahnt hätte, wäre ich Uhrmacher geworden!"
                                                 --- Albert Einstein

Michael Bramer

author

2009-01-21 23:40 UTC #16 lore raw

In reply to Paulius Zaleckas

Quoted 12 lines · Paulius Zaleckas

Quoted 11 lines · Michael Bramer
the new patch:
--------------
This Patch add the device information for the MIC-3620 8-port RS-232 cPCI card from Advantech Co. Ltd.
Signed-off-by Michael Bramer <[email protected]>
drivers/serial/8250_pci.c +0 -0
@@ -31,6 +31,7 @@
 #include "8250.h"
 

Quoted 4 lines

drivers/serial/8250_pci.c line 34
Quoted 2 lines
drivers/serial/8250_pci.c +1 -0
 #undef SERIAL_DEBUG_PCI
+#define SERIAL_DEBUG_PCI
NAK.

sorry, this was a bug in the patch.

Gruss
Grisu

Signature · 8 lines

Michael Bramer  -- http://www.feuerwehr.kreuzau.de/wiki/
PGP: finger [email protected]  -- Linux Sysadmin   -- Use Debian Linux
"Wenn ich die Folgen geahnt hätte, wäre ich Uhrmacher geworden!"
                                                 --- Albert Einstein
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html