Re: [PATCH v6] serial: support for 16550A serial ports on LP-8x4x
From: Andy Shevchenko <hidden>
Date: 2016-02-29 10:29:10
Also in:
linux-devicetree, lkml
On Sat, 2016-02-27 at 19:14 +0300, Sergei Ianovich wrote:
The patch adds support for 3 additional LP-8x4x built-in serial ports. The device can also host up to 8 extension cards with 4 serial ports on each card for a total of 35 ports. However, I don't have the hardware to test extension cards, so they are not supported, yet.
My comments below. After addressing them: Reviewed-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
quoted hunk ↗ jump to hunk
+++ b/drivers/tty/serial/8250/8250_lp8841.c@@ -0,0 +1,166 @@ +/* linux/drivers/tty/serial/8250/8250_lp8841.c + * + * Support for 16550A serial ports on ICP DAS LP-8841 + * + * Copyright (C) 2013 Sergei Ianovich <ynvich-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> + * + * This program is free software; you can redistribute it and/ormodify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ +#include <linux/init.h> +#include <linux/io.h> +#include <linux/irq.h> +#include <linux/module.h> +#include <linux/serial_8250.h> +#include <linux/of.h> +#include <linux/platform_device.h> + +struct lp8841_serial_data { + int line; + void *ios_mem;
__iomem
+};
+ if (termios->c_cflag & CSTOPB) + len++; + if (termios->c_cflag & PARENB) + len++; + if (!(termios->c_cflag & PARODD)) + len++; +#ifdef CMSPAR + if (termios->c_cflag & CMSPAR) + len++; +#endif + + len -= 9;
If you have 5 bit mode, no parity, 1/1.5 stop bits, you may end up with negative value here. Am I right? If so, is it expected?
+ len &= 3; + len <<= 3;
+ writeb(len, data->ios_mem); + +}
+static int lp8841_serial_probe(struct platform_device *pdev)
+{
+ struct uart_8250_port uart = {};
+ struct lp8841_serial_data *data;
+ struct resource *mmres, *mires;
+ int ret;
+
+ mmres = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ mires = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ if (!mmres || !mires)
+ return -ENODEV;No need to check mires here, devm_ioremap_resource() will take care about.
+ + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); + if (!data) + return -ENOMEM; + + data->ios_mem = devm_ioremap_resource(&pdev->dev, mires); + if (!data->ios_mem) + return -EFAULT;
You have to propagate the actual error code from devm_ioremap_resource().
+ + uart.port.iotype = UPIO_MEM;
+ uart.port.mapbase = mmres->start; + uart.port.iobase = mmres->start;
I'm not sure about this. If you ask for UPIO_MEM why do you need to fill iobase? And I suppose iobase can't hold (at the end inb/outb calls) big port numbers anyway (16 bit on x86, for example).
+ uart.port.regshift = 1;
+ uart.port.irq = platform_get_irq(pdev, 0);
+ uart.port.flags = UPF_IOREMAP;
+ uart.port.dev = &pdev->dev;
+ uart.port.uartclk = 14745600;
+ uart.port.set_termios = lp8841_serial_set_termios;
+ uart.port.private_data = data;
+
+ ret = serial8250_register_8250_port(&uart);
+ if (ret < 0)
+ return ret;
+
+ data->line = ret;
+
+ platform_set_drvdata(pdev, data);
+
+ return 0;
+}
+
+static int lp8841_serial_remove(struct platform_device *pdev)
+{
+ struct lp8841_serial_data *data =
platform_get_drvdata(pdev);
+
+ serial8250_unregister_port(data->line);
+
+ return 0;
+}
+
+static struct platform_driver lp8841_serial_driver = {
+ .probe = lp8841_serial_probe,
+ .remove = lp8841_serial_remove,
+
+ .driver = {
+ .name = "uart-lp8841",
+ .of_match_table = lp8841_serial_dt_ids,
+ },
+};-- Andy Shevchenko [off-list ref] Intel Finland Oy -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html