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

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

[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:
-------------------------------------
--- a/drivers/serial/8250_pci.c
+++ b/drivers/serial/8250_pci.c
@@ -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
-- 
Michael Bramer  -- http://www.feuerwehr.kreuzau.de/wiki/
PGP: finger grisu@db.debian.org  -- Linux Sysadmin   -- Use Debian Linux
"Wenn ich die Folgen geahnt hätte, wäre ich Uhrmacher geworden!"
                                                 --- Albert Einstein

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

[Alan Cox]

> +               .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.

> +       {       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

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

[Michael Bramer]

On Sun, Jan 18, 2009 at 02:14:09PM +0000, Alan Cox wrote:
> > +               .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...

> > 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 <grisu@deb-support.de>
--- a/drivers/serial/8250_pci.c
+++ b/drivers/serial/8250_pci.c
@@ -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
-- 
Michael Bramer  -- http://www.feuerwehr.kreuzau.de/wiki/
PGP: finger grisu@db.debian.org  -- Linux Sysadmin   -- Use Debian Linux
"Wenn ich die Folgen geahnt hätte, wäre ich Uhrmacher geworden!"
                                                 --- Albert Einstein

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

[Andrey Panin]

On 018, 01 18, 2009 at 01:53:59PM +0000, Michael Bramer wrote:
> 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:
> -------------------------------------
> --- a/drivers/serial/8250_pci.c
> +++ b/drivers/serial/8250_pci.c
> @@ -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.
>          */

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

> @@ -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
> -- 
> Michael Bramer  -- http://www.feuerwehr.kreuzau.de/wiki/
> PGP: finger grisu@db.debian.org  -- 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

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

[Michael Bramer]

On Mon, Jan 19, 2009 at 05:51:01PM +0300, Andrey Panin wrote:
> On 018, 01 18, 2009 at 01:53:59PM +0000, Michael Bramer wrote:
> > 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:
> > -------------------------------------
> > --- a/drivers/serial/8250_pci.c
> > +++ b/drivers/serial/8250_pci.c
> > @@ -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.
> >          */
> 
> 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
-- 
Michael Bramer  -- http://www.feuerwehr.kreuzau.de/wiki/
PGP: finger grisu@db.debian.org  -- Linux Sysadmin   -- Use Debian Linux
"Wenn ich die Folgen geahnt hätte, wäre ich Uhrmacher geworden!"
                                                 --- Albert Einstein

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

[Andrey Panin]

On 019, 01 19, 2009 at 11:51:22PM +0000, Michael Bramer wrote:
> On Mon, Jan 19, 2009 at 05:51:01PM +0300, Andrey Panin wrote:
> > On 018, 01 18, 2009 at 01:53:59PM +0000, Michael Bramer wrote:
> > > 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:
> > > -------------------------------------
> > > --- a/drivers/serial/8250_pci.c
> > > +++ b/drivers/serial/8250_pci.c
> > > @@ -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.
> > >          */
> > 
> > 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?

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

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

We can safely remove it.

> Gruss
> Grisu
> -- 
> Michael Bramer  -- http://www.feuerwehr.kreuzau.de/wiki/
> PGP: finger grisu@db.debian.org  -- Linux Sysadmin   -- Use Debian Linux
> "Wenn ich die Folgen geahnt h?tte, w?re ich Uhrmacher geworden!"
>                                                  --- Albert Einstein

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

[Michael Bramer]

On Tue, Jan 20, 2009 at 06:12:16PM +0300, Andrey Panin wrote:
> On 019, 01 19, 2009 at 11:51:22PM +0000, Michael Bramer wrote:
> > On Mon, Jan 19, 2009 at 05:51:01PM +0300, Andrey Panin wrote:
> > > On 018, 01 18, 2009 at 01:53:59PM +0000, Michael Bramer wrote:
> > > > +        * 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.
> > > >          */
> > > 
> > > 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 <grisu@deb-support.de>
--- a/drivers/serial/8250_pci.c
+++ b/drivers/serial/8250_pci.c
@@ -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
-- 
Michael Bramer  -- http://www.feuerwehr.kreuzau.de/wiki/
PGP: finger grisu@db.debian.org  -- Linux Sysadmin   -- Use Debian Linux
"Wenn ich die Folgen geahnt hätte, wäre ich Uhrmacher geworden!"
                                                 --- Albert Einstein

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

[Paulius Zaleckas]

Michael Bramer wrote:
> On Tue, Jan 20, 2009 at 06:12:16PM +0300, Andrey Panin wrote:
>> On 019, 01 19, 2009 at 11:51:22PM +0000, Michael Bramer wrote:
>>> On Mon, Jan 19, 2009 at 05:51:01PM +0300, Andrey Panin wrote:
>>>> On 018, 01 18, 2009 at 01:53:59PM +0000, Michael Bramer wrote:
>>>>> +        * 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.
>>>>>          */
>>>> 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 <grisu@deb-support.de>
> --- a/drivers/serial/8250_pci.c
> +++ b/drivers/serial/8250_pci.c
> @@ -31,6 +31,7 @@
>  #include "8250.h"
> 
>  #undef SERIAL_DEBUG_PCI
> +#define SERIAL_DEBUG_PCI

NAK.

>  /*
>   * 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

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

[Niels de Vos]

[-- Attachment #1: Type: text/plain, Size: 1171 bytes --]

Paulius Zaleckas wrote:
> Michael Bramer wrote:
...
>> 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 <grisu@deb-support.de>
>> --- a/drivers/serial/8250_pci.c
>> +++ b/drivers/serial/8250_pci.c
>> @@ -31,6 +31,7 @@
>>  #include "8250.h"
>>
>>  #undef SERIAL_DEBUG_PCI
>> +#define SERIAL_DEBUG_PCI
> 
> NAK.
> 
>>  /*
>>   * 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>?


>>  /*
>>   * 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

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

[Alan Cox]

> >>  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?

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

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

[Michael Bramer]

On Wed, Jan 21, 2009 at 10:18:05AM +0200, Paulius Zaleckas wrote:
> Michael Bramer wrote:
> > 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 <grisu@deb-support.de>
> > --- a/drivers/serial/8250_pci.c
> > +++ b/drivers/serial/8250_pci.c
> > @@ -31,6 +31,7 @@
> >  #include "8250.h"
> > 
> >  #undef SERIAL_DEBUG_PCI
> > +#define SERIAL_DEBUG_PCI
> 
> NAK.

sorry, this was a bug in the patch.

Gruss
Grisu
-- 
Michael Bramer  -- http://www.feuerwehr.kreuzau.de/wiki/
PGP: finger grisu@db.debian.org  -- Linux Sysadmin   -- Use Debian Linux
"Wenn ich die Folgen geahnt hätte, wäre ich Uhrmacher geworden!"
                                                 --- Albert Einstein

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

[Michael Bramer]

On Wed, Jan 21, 2009 at 12:32:15PM +0000, Alan Cox wrote:
> > >>  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?
> 
> 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
-- 
Michael Bramer  -- http://www.feuerwehr.kreuzau.de/wiki/
PGP: finger grisu@db.debian.org  -- Linux Sysadmin   -- Use Debian Linux
"Wenn ich die Folgen geahnt hätte, wäre ich Uhrmacher geworden!"
                                                 --- Albert Einstein

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

[Tosoni]

> [mailto:linux-serial-owner@vger.kernel.org]On Behalf Of Michael Bramer
>
> On Wed, Jan 21, 2009 at 12:32:15PM +0000, Alan Cox wrote:
> > > >>  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?
> >
> > 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,

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

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

[Niels de Vos]

[-- Attachment #1: Type: text/plain, Size: 1021 bytes --]

Tosoni wrote:
>> On Wed, Jan 21, 2009 at 12:32:15PM +0000, Alan Cox wrote:
>>>>>>  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?
>>> 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,
> 
> 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

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

[Michael Bramer]

On Thu, Jan 22, 2009 at 10:03:53AM +0100, Niels de Vos wrote:
> Tosoni wrote:
> >> On Wed, Jan 21, 2009 at 12:32:15PM +0000, Alan Cox wrote:
> >>>>>>  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?
> >>> 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,
> > 
> > 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 <grisu@deb-support.de>
--- a/drivers/serial/8250_pci.c	2008-07-13 23:51:29.000000000 +0200
+++ b/drivers/serial/8250_pci.c	2009-01-17 21:37:37.000000000 +0100
@@ -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
-- 
Michael Bramer  -- http://www.feuerwehr.kreuzau.de/wiki/
PGP: finger grisu@db.debian.org  -- Linux Sysadmin   -- Use Debian Linux
"Wenn ich die Folgen geahnt hätte, wäre ich Uhrmacher geworden!"
                                                 --- Albert Einstein

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

[Paulius Zaleckas]

Michael Bramer wrote:
> On Thu, Jan 22, 2009 at 10:03:53AM +0100, Niels de Vos wrote:
>> Tosoni wrote:
>>>> On Wed, Jan 21, 2009 at 12:32:15PM +0000, Alan Cox wrote:
>>>>>>>>  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?
>>>>> 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,
>>> 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 :)

> ----
> 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 <grisu@deb-support.de>
> --- a/drivers/serial/8250_pci.c	2008-07-13 23:51:29.000000000 +0200
> +++ b/drivers/serial/8250_pci.c	2009-01-17 21:37:37.000000000 +0100
> @@ -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.

>  /*
>   * 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

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

[Michael Bramer]

On Thu, Jan 22, 2009 at 01:47:35PM +0200, Paulius Zaleckas wrote:
> > Is the patch now ok?
> 
> No :)
> 
> > ----
> > 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 <grisu@deb-support.de>
> > --- a/drivers/serial/8250_pci.c	2008-07-13 23:51:29.000000000 +0200
> > +++ b/drivers/serial/8250_pci.c	2009-01-17 21:37:37.000000000 +0100
> > @@ -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 <grisu@deb-support.de>
--- a/drivers/serial/8250_pci.c	2008-07-13 23:51:29.000000000 +0200
+++ b/drivers/serial/8250_pci.c	2009-01-17 21:37:37.000000000 +0100
@@ -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
-- 
Michael Bramer  -- http://www.feuerwehr.kreuzau.de/wiki/
PGP: finger grisu@db.debian.org  -- Linux Sysadmin   -- Use Debian Linux
"Wenn ich die Folgen geahnt hätte, wäre ich Uhrmacher geworden!"
                                                 --- Albert Einstein