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
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
