Re: [PATCH v7 2/3] serial: exar: split out the exar code from 8250_pci

[Sudip Mukherjee]

On Sunday 08 January 2017 01:02 AM, Andy Shevchenko wrote:
> On Sun, Jan 8, 2017 at 1:57 AM, Sudip Mukherjee
> <sudipm.mukherjee@xxxxxxxxx> wrote:
> > Add the serial driver for the exar chips. And also register the
> > platform device for the exar gpio.
>
> Did you ignore some comments?
>
> IIRC I recommended to use proper vendor name like Exar (or how is it spelled?).

oops, sorry. I missed that.

> > Headers, if arranged in alphabetical order, will give a build warning.
>
> I think I know how to make it better.
>
> > And thanks for revewing that v6. I think those were the worst patch I
> > have ever posted.
>
> You may do even more better. See below.
>
> > +#undef DEBUG
>
> > +#include <asm/byteorder.h>
>
> (1)
>
> > +#include <linux/pci.h>
>
> Squeez this to the rest
>
> > +#include <linux/8250_pci.h>
>
> (2)
>
> > +#include <linux/bitops.h>
> > +#include <linux/delay.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/serial_core.h>
> > +#include <linux/serial_reg.h>
> > +#include <linux/slab.h>
> > +#include <linux/string.h>
> > +#include <linux/tty.h>
>
> You perhaps need something like this here:
>
> + empty line
> (1) +#include <asm/byteorder.h>
>
> > +
>
> (2) +#include <linux/8250_pci.h>
>
> > +#include "8250.h"

not sure if I have understood this header thing properly. But let me 
play with it and see,

> > +
> > +#define PCI_NUM_BAR_RESOURCES  6
<snip>
> > +static struct pci_device_id exar_pci_tbl[] = {
> > +       {       PCI_DEVICE_SUB(PCI_VENDOR_ID_EXAR,
> > +               PCI_DEVICE_ID_EXAR_XR17C152,
> > +               PCI_SUBVENDOR_ID_CONNECT_TECH,
> > +               PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_232), 0, 0,
> > +               (kernel_ulong_t)&pbn_b0_2_1843200_200 },
>
> You ignored my comment regarding to make a macro(s).

I used PCI_DEVICE_SUB() and PCI_VDEVICE(), but yes, custom macro might 
be better here. I was trying to have one custom macro, but with two 
different macros it should be ok.

> Moreover, some of data, like number of ports, can be easily calculated
> from device ID.

yes, but since the baudrate is different i will need to have different 
board id for it.
Like: 'PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_232' has a device id 
'PCI_DEVICE_ID_EXAR_XR17C154' is having a baudrate of 1843200 but the 
other devices with the same deviceid will have a baudrate of 921600.

unless, in the probe I compare the subvendor with 
PCI_SUBVENDOR_ID_CONNECT_TECH and modify the baud. Let me try.

Thanks for reviewing.

Regards
Sudip