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

[Andy Shevchenko]

On Sat, 2016-02-20 at 12:00 +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
> 
> This patch is based on information extracted from
> vendor mxupcie driver available on MOXA website.
> 
> I was able to test it on a CP-168EL-A on PC.

Looks much better!
Though few comments below.

> +static int moxa8250_probe(struct pci_dev *pdev, const struct
> pci_device_id *id)
> +{
> +	struct uart_8250_port uart;
> +	struct moxa8250_board *brd;
> +	void __iomem *ioaddr;
> +	int nr_ports, ret, i, offset;

i looks like unsigned and maybe split per logical lines? I don't think
nr_ports correlates to ret somehow, same for the rest.

unsigned int i, nr_ports;
... offset;
int ret;

?

> +
> +	brd = &moxa8250_boards[id->driver_data];
> +	nr_ports = brd->num_ports;
> +
> +	ret = pcim_enable_device(pdev);
> +	if (ret)
> +		return ret;
> +
> +	brd = devm_kzalloc(&pdev->dev, sizeof(struct moxa8250_board)
> +
> +			ÂÂÂsizeof(unsigned int) * nr_ports,
> GFP_KERNEL);
> +	if (!brd)
> +		return -ENOMEM;
> +
> +	memset(&uart, 0, sizeof(struct uart_8250_port));
> +
> +	uart.port.dev = &pdev->dev;
> +	uart.port.irq = pdev->irq;
> +	uart.port.private_data = brd;
> +	uart.port.uartclk = MOXA_BASE_BAUD * 16;
> +	uart.port.flags = UPF_SKIP_TEST | UPF_BOOT_AUTOCONF |
> UPF_SHARE_IRQ;
> +
> +	ioaddr = pcim_iomap(pdev, 1, 0);

And if we got NULL here?

1 is magic bar number, perhaps define it.

> +
> +	for (i = 0; i < nr_ports; i++) {
> +
> +		/*
> +		Â* MOXA Smartio MUE boards with 4 ports have
> +		Â* a different offset for port #3
> +		Â*/
> +		if (nr_ports == 4 && i == 3)
> +			offset = 7 * MOXA_UART_OFFSET;
> +		else
> +			offset = i * MOXA_UART_OFFSET;
> +
> +		uart.port.iotype = UPIO_MEM;
> +		uart.port.iobase = 0;
> +		uart.port.mapbase = pci_resource_start(pdev, 1) +
> offset;
> +		uart.port.membase = ioaddr + offset;
> +		uart.port.regshift = 0;
> +
> +		dev_dbg(&pdev->dev, "Setup PCI port: port %lx, irq
> %d, type %d\n",
> +			uart.port.iobase, uart.port.irq,
> uart.port.iotype);
> +
> +		brd->line[i] = serial8250_register_8250_port(&uart);
> +		if (brd->line[i] < 0) {
> +			dev_err(&pdev->dev,
> +				"Couldn't register serial port %lx,
> irq %d, type %d, error %d\n",
> +				uart.port.iobase, uart.port.irq,
> +				uart.port.iotype, brd->line[i]);
> +			break;
> +		}
> +	}
> +
> +	pci_set_drvdata(pdev, brd);
> +	return 0;
> +}
> +
> +static void moxa8250_remove(struct pci_dev *pdev)
> +{
> +	struct moxa8250_board *brd = pci_get_drvdata(pdev);
> +	int i;

unsigned

> +
> +	for (i = 0; i < brd->num_ports; i++)
> +		serial8250_unregister_port(brd->line[i]);
> +}
> +
> +static const struct pci_device_id pci_ids[] = {
> +	{PCI_DEVICE(PCI_VENDOR_ID_MOXA, PCI_DEVICE_ID_MOXA_CP102E),

...and PCI_VDEVICE will look even shorter!

> 
> +	Â.driver_data = moxa8250_2p},

Maybe a macro which will also cast to kernel_ulong_t ?

Something like

#define MOXA_DEVICE(id,data) { PCI_VDEVICE(MOXA, id), .driver_data =
(kernel_ulong_t)(data) }

static const struct pci_device_id pci_ids[] = {
Â
MOXA_DEVICE(PCI_DEVICE_ID_MOXA_CP102E, moxa8250_2p),
 ...
};


> +	{PCI_DEVICE(PCI_VENDOR_ID_MOXA, PCI_DEVICE_ID_MOXA_CP102EL),
> +	Â.driver_data = moxa8250_2p},
> +	{PCI_DEVICE(PCI_VENDOR_ID_MOXA,
> PCI_DEVICE_ID_MOXA_CP104EL_A),
> +	Â.driver_data = moxa8250_4p},
> +	{PCI_DEVICE(PCI_VENDOR_ID_MOXA, PCI_DEVICE_ID_MOXA_CP114EL),
> +	Â.driver_data = moxa8250_4p},
> +	{PCI_DEVICE(PCI_VENDOR_ID_MOXA,
> PCI_DEVICE_ID_MOXA_CP116E_A_A),
> +	Â.driver_data = moxa8250_8p},
> +	{PCI_DEVICE(PCI_VENDOR_ID_MOXA,
> PCI_DEVICE_ID_MOXA_CP116E_A_B),
> +	Â.driver_data = moxa8250_8p},
> +	{PCI_DEVICE(PCI_VENDOR_ID_MOXA,
> PCI_DEVICE_ID_MOXA_CP118EL_A),
> +	Â.driver_data = moxa8250_8p},
> +	{PCI_DEVICE(PCI_VENDOR_ID_MOXA,
> PCI_DEVICE_ID_MOXA_CP118E_A_I),
> +	Â.driver_data = moxa8250_8p},
> +	{PCI_DEVICE(PCI_VENDOR_ID_MOXA, PCI_DEVICE_ID_MOXA_CP132EL),
> +	Â.driver_data = moxa8250_2p},
> +	{PCI_DEVICE(PCI_VENDOR_ID_MOXA,
> PCI_DEVICE_ID_MOXA_CP134EL_A),
> +	Â.driver_data = moxa8250_4p},
> +	{PCI_DEVICE(PCI_VENDOR_ID_MOXA,
> PCI_DEVICE_ID_MOXA_CP138E_A),
> +	Â.driver_data = moxa8250_8p},
> +	{PCI_DEVICE(PCI_VENDOR_ID_MOXA,
> PCI_DEVICE_ID_MOXA_CP168EL_A),
> +	Â.driver_data = moxa8250_8p},

I think your device provides a PCI serial class, thus you have to
blacklist them in 8250_pci.c.

> +	{0}
> +};

> +};

Redundant empty line.

> +MODULE_DEVICE_TABLE(pci, pci_ids);

-- 
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Intel Finland Oy