Re: [PATCH] serial: 8250_pci: add MOXA Smartio MUE boards support

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Fri, 2016-02-12 at 19:28 +0100, Mathieu OTHACEHE wrote:
> Add support for :
> 
> - CP-102E: 2 ports RS232 PCIE card
> - CP-102EL: 2 ports RS232 PCIE card
> - CP-132EL: 2 ports RS422/485 PCIE card
> - CP-114EL: 4 ports RS232/422/485 PCIE card
> - CP-104EL-A: 4 ports RS232 PCIE card
> - CP-168EL-A: 8 ports RS232 PCIE card
> - CP-118EL-A: 8 ports RS232/422/485 PCIE card
> - CP-118E-A: 8 ports RS422/485 PCIE card
> - CP-138E-A: 8 ports RS422/485 PCIE card
> - CP-134EL-A: 4 ports RS422/485 PCIE card
> - CP-116E-A (A): 8 ports RS232/422/485 PCIE card
> - CP-116E-A (B): 8 ports RS232/422/485 PCIE card
> 
> Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
> ---
> 
> Hi,
> 
> This patch add support for MOXA Smartio MUE boards to 8250 driver.
> I already submitted an independant driver based on the vendor one :
> 
> https://lkml.org/lkml/2016/2/1/720
> 
> But as Andy pointed out, it seems to be a better idea to add this
> support
> directly in 8250 driver.

Yes, something like this. It would be even better to have something
like 8250_moxa.c at some point. I don't know if it worth to do right
now.

See my comments below.

> I was able to test this patch on a CP-168EL-A on PC.
> 
> Mathieu
> 
>  drivers/tty/serial/8250/8250_pci.c | 193
> +++++++++++++++++++++++++++++++++++++
>  1 file changed, 193 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_pci.c
> b/drivers/tty/serial/8250/8250_pci.c
> index 8f8d5c5..67c1028 100644
> --- a/drivers/tty/serial/8250/8250_pci.c
> +++ b/drivers/tty/serial/8250/8250_pci.c
> @@ -1862,6 +1862,25 @@ pci_wch_ch38x_setup(struct serial_private
> *priv,
>  	return pci_default_setup(priv, board, port, idx);
>  }
>  
> +static int pci_mxupcie_setup(struct serial_private *priv,
> +			     const struct pciserial_board *board,
> +			     struct uart_8250_port *port, int idx)
> +{
> +	unsigned int bar, offset;
> +
> +	/*
> +	 * MOXA Smartio MUE boards with 4 ports have
> +	 * a different offset for port #3
> +	 */
> +	if (idx == 3) {
> +		bar = FL_GET_BASE(board->flags);
> +		offset = 7 * board->uart_offset;
> +		return setup_port(priv, port, bar, offset, board-
> >reg_shift);
> +	} else {
> +		return pci_default_setup(priv, board, port, idx);
> +	}
> +}

So, looks like it would be pretty simple to have it in a separate file.

> +
>  #define PCI_VENDOR_ID_SBSMODULARIO	0x124B
>  #define PCI_SUBVENDOR_ID_SBSMODULARIO	0x124B
>  #define PCI_DEVICE_ID_OCTPRO		0x0001
> @@ -1927,6 +1946,19 @@ pci_wch_ch38x_setup(struct serial_private
> *priv,
>  #define PCI_DEVICE_ID_PERICOM_PI7C9X7954	0x7954
>  #define PCI_DEVICE_ID_PERICOM_PI7C9X7958	0x7958
>  
> +#define	PCI_DEVICE_ID_MOXA_CP102E	0x1024
> +#define	PCI_DEVICE_ID_MOXA_CP102EL	0x1025
> +#define	PCI_DEVICE_ID_MOXA_CP132EL	0x1322
> +#define	PCI_DEVICE_ID_MOXA_CP114EL	0x1144
> +#define	PCI_DEVICE_ID_MOXA_CP104EL_A	0x1045
> +#define	PCI_DEVICE_ID_MOXA_CP168EL_A	0x1683
> +#define	PCI_DEVICE_ID_MOXA_CP118EL_A	0x1182
> +#define	PCI_DEVICE_ID_MOXA_CP118E_A_I	0x1183
> +#define	PCI_DEVICE_ID_MOXA_CP138E_A	0x1381
> +#define	PCI_DEVICE_ID_MOXA_CP134EL_A	0x1342
> +#define	PCI_DEVICE_ID_MOXA_CP116E_A_A	0x1160
> +#define	PCI_DEVICE_ID_MOXA_CP116E_A_B	0x1161

But you can keep this sorted by IDs, right?


> @@ -2938,6 +2994,19 @@ enum pci_board_num_t {
>  	pbn_pericom_PI7C9X7952,
>  	pbn_pericom_PI7C9X7954,
>  	pbn_pericom_PI7C9X7958,
> 

> +	pbn_moxa_CP102E,
> +	pbn_moxa_CP102EL,
> +	pbn_moxa_CP132EL,
> +	pbn_moxa_CP114EL,
> +	pbn_moxa_CP104EL_A,
> +	pbn_moxa_CP168EL_A,
> +	pbn_moxa_CP118EL_A,
> +	pbn_moxa_CP118E_A_I,
> +	pbn_moxa_CP138E_A,
> +	pbn_moxa_CP134EL_A,
> +	pbn_moxa_CP116E_A_A,
> +	pbn_moxa_CP116E_A_B

This part seems too verbose. You have similarities in the bunch of
boards.

+
>  };
>  
>  /*
> @@ -3794,6 +3863,81 @@ static struct pciserial_board pci_boards[] = {
>  		.base_baud      = 921600,
>  		.uart_offset	= 0x8,
>  	},
> +	/*
> +	 * MOXA Smartio MUE boards
> +	 */
> +	[pbn_moxa_CP102E] = {
> +		.flags          = FL_BASE1,
> +		.num_ports      = 2,
> +		.base_baud      = 921600,
> +		.uart_offset	= 0x200,
> +	},
> +	[pbn_moxa_CP102EL] = {
> +		.flags          = FL_BASE1,
> +		.num_ports      = 2,
> +		.base_baud      = 921600,
> +		.uart_offset	= 0x200,
> +	},
> +	[pbn_moxa_CP132EL] = {
> +		.flags          = FL_BASE1,
> +		.num_ports      = 2,
> +		.base_baud      = 921600,
> +		.uart_offset	= 0x200,
> +	},
> +	[pbn_moxa_CP114EL] = {
> +		.flags          = FL_BASE1,
> +		.num_ports      = 4,
> +		.base_baud      = 921600,
> +		.uart_offset	= 0x200,
> +	},
> +	[pbn_moxa_CP104EL_A] = {
> +		.flags          = FL_BASE1,
> +		.num_ports      = 4,
> +		.base_baud      = 921600,
> +		.uart_offset	= 0x200,
> +	},
> +	[pbn_moxa_CP168EL_A] = {
> +		.flags          = FL_BASE1,
> +		.num_ports      = 8,
> +		.base_baud      = 921600,
> +		.uart_offset	= 0x200,
> +	},
> +	[pbn_moxa_CP118EL_A] = {
> +		.flags          = FL_BASE1,
> +		.num_ports      = 8,
> +		.base_baud      = 921600,
> +		.uart_offset	= 0x200,
> +	},
> +	[pbn_moxa_CP118E_A_I] = {
> +		.flags          = FL_BASE1,
> +		.num_ports      = 8,
> +		.base_baud      = 921600,
> +		.uart_offset	= 0x200,
> +	},
> +	[pbn_moxa_CP138E_A] = {
> +		.flags          = FL_BASE1,
> +		.num_ports      = 8,
> +		.base_baud      = 921600,
> +		.uart_offset	= 0x200,
> +	},
> +	[pbn_moxa_CP134EL_A] = {
> +		.flags          = FL_BASE1,
> +		.num_ports      = 4,
> +		.base_baud      = 921600,
> +		.uart_offset	= 0x200,
> +	},
> +	[pbn_moxa_CP116E_A_A] = {
> +		.flags          = FL_BASE1,
> +		.num_ports      = 8,
> +		.base_baud      = 921600,
> +		.uart_offset	= 0x200,
> +	},
> +	[pbn_moxa_CP116E_A_B] = {
> +		.flags          = FL_BASE1,
> +		.num_ports      = 8,
> +		.base_baud      = 921600,
> +		.uart_offset	= 0x200,

So, I see only 3. Only difference is a number of ports.

So, if you go with 8250_moxa.c you may use your private structure to
keep only what is needed.


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy