[PATCH v4 12/21] serial: support for 16550A serial ports on LP-8x4x

[PATCH v4 12/21] serial: support for 16550A serial ports on LP-8x4x

[Sergei Ianovich]

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.

Signed-off-by: Sergei Ianovich <ynvich@gmail.com>
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
   v3..v4
   * move DTS bindings to a different patch (8/21) as suggested by
     Heikki Krogerus

   v2..v3
   * no changes (except number 10/16 -> 12/21)

   v0..v2
   * register platform driver instead of platform device
   * use device tree
   * use devm helpers where possible

 .../devicetree/bindings/serial/lp8x4x-serial.txt   |  35 +++++
 arch/arm/configs/lp8x4x_defconfig                  |   1 +
 drivers/tty/serial/8250/8250_lp8x4x.c              | 169 +++++++++++++++++++++
 drivers/tty/serial/8250/Kconfig                    |  12 ++
 drivers/tty/serial/8250/Makefile                   |   1 +
 5 files changed, 218 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/lp8x4x-serial.txt
 create mode 100644 drivers/tty/serial/8250/8250_lp8x4x.c

diff --git a/Documentation/devicetree/bindings/serial/lp8x4x-serial.txt b/Documentation/devicetree/bindings/serial/lp8x4x-serial.txt
new file mode 100644
index 0000000..5f9a4c1
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/lp8x4x-serial.txt
@@ -0,0 +1,35 @@
+UART ports on ICP DAS LP-8x4x
+
+ICP DAS LP-8x4x contains three additional serial ports interfaced via
+Analog Devices ADM213EA chips in addition to 3 serial ports on PXA CPU.
+
+Required properties:
+- compatible : should be "icpdas,uart-lp8x4x"
+
+- reg : should provide 16 byte man IO memory region and 1 byte region for
+	termios
+
+- interrupts : should provide interrupt
+
+- interrupt-parent : should provide a link to interrupt controller either
+		     explicitly or implicitly from a parent node
+
+Examples (from pxa27x-lp8x4x.dts):
+
+		uart@9050 {
+			compatible = "icpdas,uart-lp8x4x";
+			reg = <0x9050 0x10
+			       0x9030 0x02>;
+			interrupt-parent = <&fpgairg>;
+			interrupts = <13>;
+			status = "okay";
+		};
+
+		uart@9060 {
+			compatible = "icpdas,uart-lp8x4x";
+			reg = <0x9060 0x10
+			       0x9032 0x02>;
+			interrupt-parent = <&fpgairg>;
+			interrupts = <14>;
+			status = "okay";
+		};
diff --git a/arch/arm/configs/lp8x4x_defconfig b/arch/arm/configs/lp8x4x_defconfig
index 17a4e6f..9116ce1 100644
--- a/arch/arm/configs/lp8x4x_defconfig
+++ b/arch/arm/configs/lp8x4x_defconfig
@@ -112,6 +112,7 @@ CONFIG_SERIAL_8250_EXTENDED=y
 CONFIG_SERIAL_8250_MANY_PORTS=y
 CONFIG_SERIAL_8250_SHARE_IRQ=y
 CONFIG_SERIAL_PXA=y
+CONFIG_SERIAL_8250_LP8X4X=m
 CONFIG_HW_RANDOM=y
 CONFIG_I2C=m
 # CONFIG_I2C_COMPAT is not set
diff --git a/drivers/tty/serial/8250/8250_lp8x4x.c b/drivers/tty/serial/8250/8250_lp8x4x.c
new file mode 100644
index 0000000..93e0f59
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_lp8x4x.c
@@ -0,0 +1,169 @@
+/*  linux/drivers/tty/serial/8250/8250_lp8x4x.c
+ *
+ *  Support for 16550A serial ports on ICP DAS LP-8x4x
+ *
+ *  Copyright (C) 2013 Sergei Ianovich <ynvich@gmail.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  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 lp8x4x_serial_data {
+	int			line;
+	void			*ios_mem;
+};
+
+static void lp8x4x_serial_set_termios(struct uart_port *port,
+		struct ktermios *termios, struct ktermios *old)
+{
+	unsigned int len;
+	unsigned int baud;
+	struct lp8x4x_serial_data *data = port->private_data;
+
+	switch (termios->c_cflag & CSIZE) {
+	case CS5:
+		len = 7;
+		break;
+	case CS6:
+		len = 8;
+		break;
+	case CS7:
+		len = 9;
+		break;
+	default:
+	case CS8:
+		len = 10;
+		break;
+	}
+
+	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;
+	len &= 3;
+	len <<= 3;
+	/*
+	 * Ask the core to calculate the divisor for us.
+	 */
+	baud = uart_get_baud_rate(port, termios, old,
+				  port->uartclk / 16 / 0xffff,
+				  port->uartclk / 16);
+	switch (baud) {
+	case 2400:
+		len |= 1;
+		break;
+	case 4800:
+		len |= 2;
+		break;
+	case 19200:
+		len |= 4;
+		break;
+	case 38400:
+		len |= 5;
+		break;
+	case 57600:
+		len |= 6;
+		break;
+	case 115200:
+		len |= 7;
+		break;
+	case 9600:
+	default:
+		len |= 3;
+		break;
+	};
+	iowrite8(len, data->ios_mem);
+
+	serial8250_do_set_termios(port, termios, old);
+}
+
+static struct of_device_id lp8x4x_serial_dt_ids[] = {
+	{ .compatible = "icpdas,uart-lp8x4x", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, lp8x4x_serial_dt_ids);
+
+static int lp8x4x_serial_probe(struct platform_device *pdev)
+{
+	struct uart_8250_port uart = {};
+	struct lp8x4x_serial_data *data;
+	struct resource *mmres, *mires, *irqres;
+	int ret;
+
+	mmres = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	mires = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	irqres = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (!mmres || !mires || !irqres)
+		return -ENODEV;
+
+	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;
+
+	uart.port.iotype = UPIO_MEM;
+	uart.port.mapbase = mmres->start;
+	uart.port.iobase = mmres->start;
+	uart.port.regshift = 1;
+	uart.port.irq = irqres->start;
+	uart.port.flags = UPF_IOREMAP;
+	uart.port.dev = &pdev->dev;
+	uart.port.uartclk = 14745600;
+	uart.port.set_termios = lp8x4x_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 lp8x4x_serial_remove(struct platform_device *pdev)
+{
+	struct lp8x4x_serial_data *data = platform_get_drvdata(pdev);
+
+	serial8250_unregister_port(data->line);
+
+	return 0;
+}
+
+static struct platform_driver lp8x4x_serial_driver = {
+	.probe          = lp8x4x_serial_probe,
+	.remove         = lp8x4x_serial_remove,
+
+	.driver		= {
+		.name	= "uart-lp8x4x",
+		.owner	= THIS_MODULE,
+		.of_match_table = lp8x4x_serial_dt_ids,
+	},
+};
+
+module_platform_driver(lp8x4x_serial_driver);
+
+MODULE_AUTHOR("Sergei Ianovich");
+MODULE_DESCRIPTION("8250 serial port module for LP-8x4x");
+MODULE_LICENSE("GPL");
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 81bd7c9..9fb0fbb 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -311,3 +311,15 @@ config SERIAL_PXA
 	  can enable its onboard serial ports by enabling this option.
 
 	  If you choose M here, the module name will be 8250_pxa.
+
+config SERIAL_8250_LP8X4X
+	tristate "Support 16550A ports on ICP DAS LP-8x4x"
+	depends on OF && SERIAL_8250 && SERIAL_8250_MANY_PORTS && ARCH_PXA
+	select LP8X4X_IRQ
+	help
+	  In addition to serial ports on PXA270 SoC, LP-8x4x has 1 dual
+	  RS232/RS485 port, 1 RS485 port and 1 RS232 port.
+
+	  Say N here, unless you plan to run this kernel on a LP-8x4x system.
+
+	  If you choose M here, the module name will be 8250_lp8x4x.
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index b7d1b61..7370bfb 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_SERIAL_8250_ACCENT)	+= 8250_accent.o
 obj-$(CONFIG_SERIAL_8250_BOCA)		+= 8250_boca.o
 obj-$(CONFIG_SERIAL_8250_EXAR_ST16C554)	+= 8250_exar_st16c554.o
 obj-$(CONFIG_SERIAL_8250_HUB6)		+= 8250_hub6.o
+obj-$(CONFIG_SERIAL_8250_LP8X4X)	+= 8250_lp8x4x.o
 obj-$(CONFIG_SERIAL_8250_FSL)		+= 8250_fsl.o
 obj-$(CONFIG_SERIAL_8250_DW)		+= 8250_dw.o
 obj-$(CONFIG_SERIAL_8250_EM)		+= 8250_em.o
-- 
1.8.4.2

Re: [PATCH v4 12/21] serial: support for 16550A serial ports on LP-8x4x

[One Thousand Gnomes]

> +	baud = uart_get_baud_rate(port, termios, old,
> +				  port->uartclk / 16 / 0xffff,
> +				  port->uartclk / 16);
> +	switch (baud) {
> +	case 2400:
> +		len |= 1;
> +		break;
> +	case 4800:
> +		len |= 2;
> +		break;
> +	case 19200:
> +		len |= 4;
> +		break;
> +	case 38400:
> +		len |= 5;
> +		break;
> +	case 57600:
> +		len |= 6;
> +		break;
> +	case 115200:
> +		len |= 7;
> +		break;
> +	case 9600:
> +	default:
> +		len |= 3;
> +		break;
> +	};

Some explanation of this would be useful - eg why is it set to 7 for
115200 baud and 3 for 115201 baud ?

Re: [PATCH v4 12/21] serial: support for 16550A serial ports on LP-8x4x

[Sergei Ianovich]

One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> wrote:
>> +	baud = uart_get_baud_rate(port, termios, old,
>> +				  port->uartclk / 16 / 0xffff,
>> +				  port->uartclk / 16);
>> +	switch (baud) {
>> +	case 2400:
>> +		len |= 1;
>> +		break;
>> +	case 4800:
>> +		len |= 2;
>> +		break;
>> +	case 19200:
>> +		len |= 4;
>> +		break;
>> +	case 38400:
>> +		len |= 5;
>> +		break;
>> +	case 57600:
>> +		len |= 6;
>> +		break;
>> +	case 115200:
>> +		len |= 7;
>> +		break;
>> +	case 9600:
>> +	default:
>> +		len |= 3;
>> +		break;
>> +	};
>
>Some explanation of this would be useful - eg why is it set to 7 for
>115200 baud and 3 for 115201 baud ?

I am not related to the device vendor in any way, so please take my answers for what they are worth.

It seems that there is not enough pins to properly connect the chips to the memory bus and just you the standard 8250 UART driver. Instead, clock divisor is set using this register.

So, if you know you're asking for (115200) you get it. If you don't or guess (115201), you get the default 9600.

This is a policy, it may not be the right way to write a driver, but it is cheap and it works.

Re: [PATCH v4 12/21] serial: support for 16550A serial ports on LP-8x4x

[One Thousand Gnomes]

On Wed, 16 Apr 2014 23:01:47 +0400
Sergei Ianovich <ynvich@gmail.com> wrote:

> One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> wrote:
> >> +	baud = uart_get_baud_rate(port, termios, old,
> >> +				  port->uartclk / 16 / 0xffff,
> >> +				  port->uartclk / 16);
> >> +	switch (baud) {
> >> +	case 2400:
> >> +		len |= 1;
> >> +		break;
> >> +	case 4800:
> >> +		len |= 2;
> >> +		break;
> >> +	case 19200:
> >> +		len |= 4;
> >> +		break;
> >> +	case 38400:
> >> +		len |= 5;
> >> +		break;
> >> +	case 57600:
> >> +		len |= 6;
> >> +		break;
> >> +	case 115200:
> >> +		len |= 7;
> >> +		break;
> >> +	case 9600:
> >> +	default:
> >> +		len |= 3;
> >> +		break;
> >> +	};
> >
> >Some explanation of this would be useful - eg why is it set to 7 for
> >115200 baud and 3 for 115201 baud ?
> 
> I am not related to the device vendor in any way, so please take my answers for what they are worth.
> 
> It seems that there is not enough pins to properly connect the chips to the memory bus and just you the standard 8250 UART driver. Instead, clock divisor is set using this register.
> 
> So, if you know you're asking for (115200) you get it. If you don't or guess (115201), you get the default 9600.

Thats not quite what the code actually implements though.

> This is a policy, it may not be the right way to write a driver, but it is cheap and it works.

If thats how the hardware works and only supports a few fixed rates then
fine, but for default: you need to actually update he baudrate in the
passed termios struct so that the userspace knows its request was not
matched.

Take a look how the standard 8250 termios does it.

Also looking at this and some of the other serial bits I see no
dependencies on the problematic DMA driver, so does that mean you've got
a set of pieces that can be submitted anyway while the DMA driver
discussion continues ?

Alan

Re: [PATCH v4 12/21] serial: support for 16550A serial ports on LP-8x4x

[Sergei Ianovich]

One Thousand Gnomes <gnomes-qBU/x9rampVanCEyBjwyrvXRex20P6io@public.gmane.org> wrote:
>On Wed, 16 Apr 2014 23:01:47 +0400
>Sergei Ianovich <ynvich-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> One Thousand Gnomes <gnomes-qBU/x9rampVanCEyBjwyrvXRex20P6io@public.gmane.org> wrote:
>> >> +	baud = uart_get_baud_rate(port, termios, old,
>> >> +				  port->uartclk / 16 / 0xffff,
>> >> +				  port->uartclk / 16);
>> >> +	switch (baud) {
>> >> +	case 2400:
>> >> +		len |= 1;
>> >> +		break;
>> >> +	case 4800:
>> >> +		len |= 2;
>> >> +		break;
>> >> +	case 19200:
>> >> +		len |= 4;
>> >> +		break;
>> >> +	case 38400:
>> >> +		len |= 5;
>> >> +		break;
>> >> +	case 57600:
>> >> +		len |= 6;
>> >> +		break;
>> >> +	case 115200:
>> >> +		len |= 7;
>> >> +		break;
>> >> +	case 9600:
>> >> +	default:
>> >> +		len |= 3;
>> >> +		break;
>> >> +	};
>> >
>> >Some explanation of this would be useful - eg why is it set to 7 for
>> >115200 baud and 3 for 115201 baud ?
>> 
>> I am not related to the device vendor in any way, so please take my
>answers for what they are worth.
>> 
>> It seems that there is not enough pins to properly connect the chips
>to the memory bus and just you the standard 8250 UART driver. Instead,
>clock divisor is set using this register.
>> 
>> So, if you know you're asking for (115200) you get it. If you don't
>or guess (115201), you get the default 9600.
>
>Thats not quite what the code actually implements though.

>> This is a policy, it may not be the right way to write a driver, but
>it is cheap and it works.

>If thats how the hardware works and only supports a few fixed rates
>then
>fine, but for default: you need to actually update he baudrate in the
>passed termios struct so that the userspace knows its request was not
>matched.
>
>Take a look how the standard 8250 termios does it.

I will. I've tested the driver by setting termios from userspace. There was no chance to ask for arbitrary rate, so I didn't think much about this case.

>Also looking at this and some of the other serial bits I see no
>dependencies on the problematic DMA driver, so does that mean you've
>got
>a set of pieces that can be submitted anyway while the DMA driver
>discussion continues ?

Yes, getting each peace in will reduce the support burden.

The degree of interdependence varies from feature to feature. rtc, mtd, irq and serial could be taken independently.

The main patch is #8. It requires #1-7. The is problem with DMA is one of policy, not technical one. It is explained in #6-7 in details.

Everything in the misc depends on bus. But the bus is very simple. There is even no interrupts.

--
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

[PATCH v5] serial: support for 16550A serial ports on LP-8x4x

[Sergei Ianovich]

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.

Signed-off-by: Sergei Ianovich <ynvich-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Reviewed-by: Heikki Krogerus <heikki.krogerus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
CC: Alan Cox <gnomes-qBU/x9rampVanCEyBjwyrvXRex20P6io@public.gmane.org>
---
   v4..v5
   * constify struct of_device_id
   * drop .owner from struct platform_driver
   * rewrite set_termios() baud rate hadnling as suggested by Alan Cox

   v3..v4
   * move DTS bindings to a different patch (8/21) as suggested by
     Heikki Krogerus

   v2..v3
   * no changes (except number 10/16 -> 12/21)

   v0..v2
   * register platform driver instead of platform device
   * use device tree
   * use devm helpers where possible

 .../devicetree/bindings/serial/lp8x4x-serial.txt   |  35 +++++
 drivers/tty/serial/8250/8250_lp8x4x.c              | 168 +++++++++++++++++++++
 drivers/tty/serial/8250/Kconfig                    |  12 ++
 drivers/tty/serial/8250/Makefile                   |   1 +
 4 files changed, 216 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/lp8x4x-serial.txt
 create mode 100644 drivers/tty/serial/8250/8250_lp8x4x.c

diff --git a/Documentation/devicetree/bindings/serial/lp8x4x-serial.txt b/Documentation/devicetree/bindings/serial/lp8x4x-serial.txt
new file mode 100644
index 0000000..5f9a4c1
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/lp8x4x-serial.txt
@@ -0,0 +1,35 @@
+UART ports on ICP DAS LP-8x4x
+
+ICP DAS LP-8x4x contains three additional serial ports interfaced via
+Analog Devices ADM213EA chips in addition to 3 serial ports on PXA CPU.
+
+Required properties:
+- compatible : should be "icpdas,uart-lp8x4x"
+
+- reg : should provide 16 byte man IO memory region and 1 byte region for
+	termios
+
+- interrupts : should provide interrupt
+
+- interrupt-parent : should provide a link to interrupt controller either
+		     explicitly or implicitly from a parent node
+
+Examples (from pxa27x-lp8x4x.dts):
+
+		uart@9050 {
+			compatible = "icpdas,uart-lp8x4x";
+			reg = <0x9050 0x10
+			       0x9030 0x02>;
+			interrupt-parent = <&fpgairg>;
+			interrupts = <13>;
+			status = "okay";
+		};
+
+		uart@9060 {
+			compatible = "icpdas,uart-lp8x4x";
+			reg = <0x9060 0x10
+			       0x9032 0x02>;
+			interrupt-parent = <&fpgairg>;
+			interrupts = <14>;
+			status = "okay";
+		};
diff --git a/drivers/tty/serial/8250/8250_lp8x4x.c b/drivers/tty/serial/8250/8250_lp8x4x.c
new file mode 100644
index 0000000..0e07220
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_lp8x4x.c
@@ -0,0 +1,168 @@
+/*  linux/drivers/tty/serial/8250/8250_lp8x4x.c
+ *
+ *  Support for 16550A serial ports on ICP DAS LP-8x4x
+ *
+ *  Copyright (C) 2013 Sergei Ianovich <ynvich-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  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 lp8x4x_serial_data {
+	int			line;
+	void			*ios_mem;
+};
+
+static void lp8x4x_serial_set_termios(struct uart_port *port,
+		struct ktermios *termios, struct ktermios *old)
+{
+	unsigned int len;
+	unsigned int baud;
+	struct lp8x4x_serial_data *data = port->private_data;
+
+	serial8250_do_set_termios(port, termios, old);
+
+	switch (termios->c_cflag & CSIZE) {
+	case CS5:
+		len = 7;
+		break;
+	case CS6:
+		len = 8;
+		break;
+	case CS7:
+		len = 9;
+		break;
+	default:
+	case CS8:
+		len = 10;
+		break;
+	}
+
+	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;
+	len &= 3;
+	len <<= 3;
+	/*
+	 * Ask the core to calculate the divisor for us.
+	 */
+	baud = tty_termios_baud_rate(termios);
+
+	switch (baud) {
+	case 115200:
+		len |= 7;
+		break;
+	case 57600:
+		len |= 6;
+		break;
+	case 38400:
+		len |= 5;
+		break;
+	case 19200:
+		len |= 4;
+		break;
+	case 9600:
+		len |= 3;
+		break;
+	case 4800:
+		len |= 2;
+		break;
+	case 2400:
+	default:
+		len |= 1;
+		break;
+	};
+	iowrite8(len, data->ios_mem);
+
+}
+
+static const struct of_device_id lp8x4x_serial_dt_ids[] = {
+	{ .compatible = "icpdas,uart-lp8x4x", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, lp8x4x_serial_dt_ids);
+
+static int lp8x4x_serial_probe(struct platform_device *pdev)
+{
+	struct uart_8250_port uart = {};
+	struct lp8x4x_serial_data *data;
+	struct resource *mmres, *mires, *irqres;
+	int ret;
+
+	mmres = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	mires = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	irqres = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (!mmres || !mires || !irqres)
+		return -ENODEV;
+
+	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;
+
+	uart.port.iotype = UPIO_MEM;
+	uart.port.mapbase = mmres->start;
+	uart.port.iobase = mmres->start;
+	uart.port.regshift = 1;
+	uart.port.irq = irqres->start;
+	uart.port.flags = UPF_IOREMAP;
+	uart.port.dev = &pdev->dev;
+	uart.port.uartclk = 14745600;
+	uart.port.set_termios = lp8x4x_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 lp8x4x_serial_remove(struct platform_device *pdev)
+{
+	struct lp8x4x_serial_data *data = platform_get_drvdata(pdev);
+
+	serial8250_unregister_port(data->line);
+
+	return 0;
+}
+
+static struct platform_driver lp8x4x_serial_driver = {
+	.probe          = lp8x4x_serial_probe,
+	.remove         = lp8x4x_serial_remove,
+
+	.driver		= {
+		.name	= "uart-lp8x4x",
+		.of_match_table = lp8x4x_serial_dt_ids,
+	},
+};
+
+module_platform_driver(lp8x4x_serial_driver);
+
+MODULE_AUTHOR("Sergei Ianovich");
+MODULE_DESCRIPTION("8250 serial port module for LP-8x4x");
+MODULE_LICENSE("GPL");
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 48b6253..00eb6b0 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -387,3 +387,15 @@ config SERIAL_8250_PXA
 	  can enable its onboard serial ports by enabling this option.
 
 	  If you choose M here, the module name will be 8250_pxa.
+
+config SERIAL_8250_LP8X4X
+	tristate "Support 16550A ports on ICP DAS LP-8x4x"
+	depends on OF && SERIAL_8250 && SERIAL_8250_MANY_PORTS && ARCH_PXA
+	select LP8X4X_IRQ
+	help
+	  In addition to serial ports on PXA270 SoC, LP-8x4x has 1 dual
+	  RS232/RS485 port, 1 RS485 port and 1 RS232 port.
+
+	  Say N here, unless you plan to run this kernel on a LP-8x4x system.
+
+	  If you choose M here, the module name will be 8250_lp8x4x.
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index 7e54413..8bdbf40 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_SERIAL_8250_ACCENT)	+= 8250_accent.o
 obj-$(CONFIG_SERIAL_8250_BOCA)		+= 8250_boca.o
 obj-$(CONFIG_SERIAL_8250_EXAR_ST16C554)	+= 8250_exar_st16c554.o
 obj-$(CONFIG_SERIAL_8250_HUB6)		+= 8250_hub6.o
+obj-$(CONFIG_SERIAL_8250_LP8X4X)	+= 8250_lp8x4x.o
 obj-$(CONFIG_SERIAL_8250_FSL)		+= 8250_fsl.o
 obj-$(CONFIG_SERIAL_8250_DW)		+= 8250_dw.o
 obj-$(CONFIG_SERIAL_8250_EM)		+= 8250_em.o
-- 
2.6.2

--
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

Re: [PATCH v5] serial: support for 16550A serial ports on LP-8x4x

[Arnd Bergmann]

On Wednesday 16 December 2015 00:04:45 Sergei Ianovich wrote:
> index 0000000..5f9a4c1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serial/lp8x4x-serial.txt
> @@ -0,0 +1,35 @@
> +UART ports on ICP DAS LP-8x4x
> +
> +ICP DAS LP-8x4x contains three additional serial ports interfaced via
> +Analog Devices ADM213EA chips in addition to 3 serial ports on PXA CPU.
> +
> +Required properties:
> +- compatible : should be "icpdas,uart-lp8x4x"

Compatible strings should not include a 'x' wildcard like this, better use
the specific chip name.

Also, it sounds like you named them after the board vendor, which sounds
wrong as the vendor part of the compatible string should be the whoever
made that part (analog?)

> +- reg : should provide 16 byte man IO memory region and 1 byte region for
> +       termios
> +
> +- interrupts : should provide interrupt
> +
> +- interrupt-parent : should provide a link to interrupt controller either
> +                    explicitly or implicitly from a parent node

interrupt-parent should be an optional property, or you can leave it out,
as this is a standard property that can always be there when there is 
interrupts.

> +Examples (from pxa27x-lp8x4x.dts):
> +
> +               uart@9050 {

By convention, the name should be 'serial', not 'uart'.

	Arnd.

Re: [PATCH v5] serial: support for 16550A serial ports on LP-8x4x

[Sergei Ianovich]

On Tue, 2015-12-15 at 22:51 +0100, Arnd Bergmann wrote:
> On Wednesday 16 December 2015 00:04:45 Sergei Ianovich wrote:
> > index 0000000..5f9a4c1
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/serial/lp8x4x-serial.txt
> > @@ -0,0 +1,35 @@
> > +UART ports on ICP DAS LP-8x4x
> > +
> > +ICP DAS LP-8x4x contains three additional serial ports interfaced
> > via
> > +Analog Devices ADM213EA chips in addition to 3 serial ports on PXA
> > CPU.
> > +
> > +Required properties:
> > +- compatible : should be "icpdas,uart-lp8x4x"
> 
> Compatible strings should not include a 'x' wildcard like this, better
> use
> the specific chip name.
> 
> Also, it sounds like you named them after the board vendor, which
> sounds
> wrong as the vendor part of the compatible string should be the
> whoever
> made that part (analog?)

The chips themselves are standard, they would work with 8250_core if
properly connected. However, they are not connected normally. Al least
some of their config pins are wired to a different address region. So
the driver is board-specific.

'x' wildcards in the name of the board seem important. There are devices
made by the same vendor without 8 or 4 in their name. Those devices
either are not shipped with linux or are base on a x86 platform.

Does this justify the choice of the compatible string?

> > +- reg : should provide 16 byte man IO memory region and 1 byte
> > region for
> > +       termios
> > +
> > +- interrupts : should provide interrupt
> > +
> > +- interrupt-parent : should provide a link to interrupt controller
> > either
> > +                    explicitly or implicitly from a parent node
> 
> interrupt-parent should be an optional property, or you can leave it
> out,
> as this is a standard property that can always be there when there is 
> interrupts.

ok

> > +Examples (from pxa27x-lp8x4x.dts):
> > +
> > +               uart@9050 {
> 
> By convention, the name should be 'serial', not 'uart'.

ok
--
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

Re: [PATCH v5] serial: support for 16550A serial ports on LP-8x4x

[Arnd Bergmann]

On Wednesday 16 December 2015 11:04:57 Sergei Ianovich wrote:
> On Tue, 2015-12-15 at 22:51 +0100, Arnd Bergmann wrote:
> > On Wednesday 16 December 2015 00:04:45 Sergei Ianovich wrote:
> > > index 0000000..5f9a4c1
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/serial/lp8x4x-serial.txt
> > > @@ -0,0 +1,35 @@
> > > +UART ports on ICP DAS LP-8x4x
> > > +
> > > +ICP DAS LP-8x4x contains three additional serial ports interfaced
> > > via
> > > +Analog Devices ADM213EA chips in addition to 3 serial ports on PXA
> > > CPU.
> > > +
> > > +Required properties:
> > > +- compatible : should be "icpdas,uart-lp8x4x"
> > 
> > Compatible strings should not include a 'x' wildcard like this, better
> > use
> > the specific chip name.
> > 
> > Also, it sounds like you named them after the board vendor, which
> > sounds
> > wrong as the vendor part of the compatible string should be the
> > whoever
> > made that part (analog?)
> 
> The chips themselves are standard, they would work with 8250_core if
> properly connected. However, they are not connected normally. Al least
> some of their config pins are wired to a different address region. So
> the driver is board-specific.

Ok, I see.

> 'x' wildcards in the name of the board seem important. There are devices
> made by the same vendor without 8 or 4 in their name. Those devices
> either are not shipped with linux or are base on a x86 platform.
> 
> Does this justify the choice of the compatible string?

What I meant was that you should use the specific numbers of one machine,
precisely for the reason you list above.

If there is e.g. a LP-8040 and a LP-8141 today, and you use lp8x4x in
the compatible string to cover both, this will no longer work when the
vendor comes out with a LP8047 that is completely different.

Instead, what you should do is to use the compatible string to identify
one particular board (e.g. the first one that used this setup), and
then list the other ones as compatible with this. You can also add the
other board names in addition, e.g.

compatible = "icpdas,uart-lp8041", "icpdas,uart-lp8040";

for a lp8041 that is compatible with the lp8040. If it turns out later
that they are not entirely compatible, we can work around this in the
driver by checking for the lp8041 string that will be matched first, while
the lp8040 can be used by the driver to match the entire family of
compatible machines (no need to list every one in the driver).

	Arnd

Re: [PATCH v5] serial: support for 16550A serial ports on LP-8x4x

[Andy Shevchenko]

On Wed, 2015-12-16 at 00:04 +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.
> 

Few nitpicks, though everything looks okay.

> Signed-off-by: Sergei Ianovich <ynvich@gmail.com>
> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> CC: Alan Cox <gnomes@lxorguk.ukuu.org.uk>
> ---
>    v4..v5
>    * constify struct of_device_id
>    * drop .owner from struct platform_driver
>    * rewrite set_termios() baud rate hadnling as suggested by Alan
> Cox
> 
>    v3..v4
>    * move DTS bindings to a different patch (8/21) as suggested by
>      Heikki Krogerus
> 
>    v2..v3
>    * no changes (except number 10/16 -> 12/21)
> 
>    v0..v2
>    * register platform driver instead of platform device
>    * use device tree
>    * use devm helpers where possible
> 
>  .../devicetree/bindings/serial/lp8x4x-serial.txt   |  35 +++++
>  drivers/tty/serial/8250/8250_lp8x4x.c              | 168
> +++++++++++++++++++++
>  drivers/tty/serial/8250/Kconfig                    |  12 ++
>  drivers/tty/serial/8250/Makefile                   |   1 +
>  4 files changed, 216 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/serial/lp8x4x-
> serial.txt
>  create mode 100644 drivers/tty/serial/8250/8250_lp8x4x.c
> 
> diff --git a/Documentation/devicetree/bindings/serial/lp8x4x-
> serial.txt b/Documentation/devicetree/bindings/serial/lp8x4x-
> serial.txt
> new file mode 100644
> index 0000000..5f9a4c1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serial/lp8x4x-serial.txt
> @@ -0,0 +1,35 @@
> +UART ports on ICP DAS LP-8x4x
> +
> +ICP DAS LP-8x4x contains three additional serial ports interfaced
> via
> +Analog Devices ADM213EA chips in addition to 3 serial ports on PXA
> CPU.
> +
> +Required properties:
> +- compatible : should be "icpdas,uart-lp8x4x"
> +
> +- reg : should provide 16 byte man IO memory region and 1 byte
> region for
> +	termios
> +
> +- interrupts : should provide interrupt
> +
> +- interrupt-parent : should provide a link to interrupt controller
> either
> +		     explicitly or implicitly from a parent node
> +
> +Examples (from pxa27x-lp8x4x.dts):
> +
> +		uart@9050 {
> +			compatible = "icpdas,uart-lp8x4x";
> +			reg = <0x9050 0x10
> +			       0x9030 0x02>;
> +			interrupt-parent = <&fpgairg>;
> +			interrupts = <13>;
> +			status = "okay";
> +		};
> +
> +		uart@9060 {
> +			compatible = "icpdas,uart-lp8x4x";
> +			reg = <0x9060 0x10
> +			       0x9032 0x02>;
> +			interrupt-parent = <&fpgairg>;
> +			interrupts = <14>;
> +			status = "okay";
> +		};
> diff --git a/drivers/tty/serial/8250/8250_lp8x4x.c
> b/drivers/tty/serial/8250/8250_lp8x4x.c
> new file mode 100644
> index 0000000..0e07220
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_lp8x4x.c
> @@ -0,0 +1,168 @@
> +/*  linux/drivers/tty/serial/8250/8250_lp8x4x.c
> + *
> + *  Support for 16550A serial ports on ICP DAS LP-8x4x
> + *
> + *  Copyright (C) 2013 Sergei Ianovich <ynvich@gmail.com>
> + *
> + *  This program is free software; you can redistribute it and/or
> modify
> + *  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 lp8x4x_serial_data {
> +	int			line;
> +	void			*ios_mem;
> +};
> +
> +static void lp8x4x_serial_set_termios(struct uart_port *port,
> +		struct ktermios *termios, struct ktermios *old)
> +{
> +	unsigned int len;
> +	unsigned int baud;
> +	struct lp8x4x_serial_data *data = port->private_data;
> +
> +	serial8250_do_set_termios(port, termios, old);
> +
> +	switch (termios->c_cflag & CSIZE) {
> +	case CS5:
> +		len = 7;
> +		break;
> +	case CS6:
> +		len = 8;
> +		break;
> +	case CS7:
> +		len = 9;
> +		break;

> +	default:
> +	case CS8:

I would suggest to exchange those lines.

> +		len = 10;
> +		break;
> +	}
> +
> +	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;
> +	len &= 3;
> +	len <<= 3;

> +	/*
> +	 * Ask the core to calculate the divisor for us.
> +	 */

Can it be one line?

> +	baud = tty_termios_baud_rate(termios);
> +
> +	switch (baud) {
> +	case 115200:
> +		len |= 7;
> +		break;
> +	case 57600:
> +		len |= 6;
> +		break;
> +	case 38400:
> +		len |= 5;
> +		break;
> +	case 19200:
> +		len |= 4;
> +		break;
> +	case 9600:
> +		len |= 3;
> +		break;
> +	case 4800:
> +		len |= 2;
> +		break;
> +	case 2400:
> +	default:
> +		len |= 1;
> +		break;
> +	};
> +	iowrite8(len, data->ios_mem);

writeb() ?

> +
> +}
> +
> +static const struct of_device_id lp8x4x_serial_dt_ids[] = {
> +	{ .compatible = "icpdas,uart-lp8x4x", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, lp8x4x_serial_dt_ids);
> +
> +static int lp8x4x_serial_probe(struct platform_device *pdev)
> +{
> +	struct uart_8250_port uart = {};
> +	struct lp8x4x_serial_data *data;
> +	struct resource *mmres, *mires, *irqres;
> +	int ret;
> +
> +	mmres = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	mires = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	irqres = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +	if (!mmres || !mires || !irqres)
> +		return -ENODEV;
> +
> +	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;
> +
> +	uart.port.iotype = UPIO_MEM;
> +	uart.port.mapbase = mmres->start;
> +	uart.port.iobase = mmres->start;
> +	uart.port.regshift = 1;
> +	uart.port.irq = irqres->start;
> +	uart.port.flags = UPF_IOREMAP;
> +	uart.port.dev = &pdev->dev;
> +	uart.port.uartclk = 14745600;
> +	uart.port.set_termios = lp8x4x_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 lp8x4x_serial_remove(struct platform_device *pdev)
> +{
> +	struct lp8x4x_serial_data *data =
> platform_get_drvdata(pdev);
> +
> +	serial8250_unregister_port(data->line);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver lp8x4x_serial_driver = {
> +	.probe          = lp8x4x_serial_probe,
> +	.remove         = lp8x4x_serial_remove,
> +
> +	.driver		= {
> +		.name	= "uart-lp8x4x",
> +		.of_match_table = lp8x4x_serial_dt_ids,
> +	},
> +};
> +
> +module_platform_driver(lp8x4x_serial_driver);
> +
> +MODULE_AUTHOR("Sergei Ianovich");
> +MODULE_DESCRIPTION("8250 serial port module for LP-8x4x");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/tty/serial/8250/Kconfig
> b/drivers/tty/serial/8250/Kconfig
> index 48b6253..00eb6b0 100644
> --- a/drivers/tty/serial/8250/Kconfig
> +++ b/drivers/tty/serial/8250/Kconfig
> @@ -387,3 +387,15 @@ config SERIAL_8250_PXA
>  	  can enable its onboard serial ports by enabling this
> option.
>  
>  	  If you choose M here, the module name will be 8250_pxa.
> +
> +config SERIAL_8250_LP8X4X
> +	tristate "Support 16550A ports on ICP DAS LP-8x4x"
> +	depends on OF && SERIAL_8250 && SERIAL_8250_MANY_PORTS &&
> ARCH_PXA
> +	select LP8X4X_IRQ
> +	help
> +	  In addition to serial ports on PXA270 SoC, LP-8x4x has 1
> dual
> +	  RS232/RS485 port, 1 RS485 port and 1 RS232 port.
> +
> +	  Say N here, unless you plan to run this kernel on a LP-
> 8x4x system.
> +
> +	  If you choose M here, the module name will be 8250_lp8x4x.
> diff --git a/drivers/tty/serial/8250/Makefile
> b/drivers/tty/serial/8250/Makefile
> index 7e54413..8bdbf40 100644
> --- a/drivers/tty/serial/8250/Makefile
> +++ b/drivers/tty/serial/8250/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_SERIAL_8250_ACCENT)	+=
> 8250_accent.o
>  obj-$(CONFIG_SERIAL_8250_BOCA)		+= 8250_boca.o
>  obj-$(CONFIG_SERIAL_8250_EXAR_ST16C554)	+=
> 8250_exar_st16c554.o
>  obj-$(CONFIG_SERIAL_8250_HUB6)		+= 8250_hub6.o
> +obj-$(CONFIG_SERIAL_8250_LP8X4X)	+= 8250_lp8x4x.o
>  obj-$(CONFIG_SERIAL_8250_FSL)		+= 8250_fsl.o
>  obj-$(CONFIG_SERIAL_8250_DW)		+= 8250_dw.o
>  obj-$(CONFIG_SERIAL_8250_EM)		+= 8250_em.o

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

Re: [PATCH v5] serial: support for 16550A serial ports on LP-8x4x

[Sergei Ianovich]

On Wed, 2015-12-16 at 11:26 +0100, Arnd Bergmann wrote:
> On Wednesday 16 December 2015 11:04:57 Sergei Ianovich wrote:
> > On Tue, 2015-12-15 at 22:51 +0100, Arnd Bergmann wrote:
> > > 'x' wildcards in the name of the board seem important. There are
> > devices
> > made by the same vendor without 8 or 4 in their name. Those devices
> > either are not shipped with linux or are base on a x86 platform.
> > 
> > Does this justify the choice of the compatible string?
> 
> What I meant was that you should use the specific numbers of one
> machine,
> precisely for the reason you list above.
> 
> If there is e.g. a LP-8040 and a LP-8141 today, and you use lp8x4x in
> the compatible string to cover both, this will no longer work when the
> vendor comes out with a LP8047 that is completely different.
> 
> Instead, what you should do is to use the compatible string to
> identify
> one particular board (e.g. the first one that used this setup), and
> then list the other ones as compatible with this. You can also add the
> other board names in addition, e.g.
> 
> compatible = "icpdas,uart-lp8041", "icpdas,uart-lp8040";
> 
> for a lp8041 that is compatible with the lp8040. If it turns out later
> that they are not entirely compatible, we can work around this in the
> driver by checking for the lp8041 string that will be matched first,
> while
> the lp8040 can be used by the driver to match the entire family of
> compatible machines (no need to list every one in the driver).

I'll try to be more specific. This driver will support ports on LP-8081, 
LP-8141, LP-8441, LP-8841. Last time I checked the vendor was announcing
a series with 3 as the last digit. They use lp8x4x name, eg. in
documentation like `LP-8x4x_ChangeLog.txt`. They ship their proprietary
SDK in `lp8x4x_sdk_for_linux.tar`. All of this implies that it is a
single board.

Re: [PATCH v5] serial: support for 16550A serial ports on LP-8x4x

[Sergei Ianovich]

On Tue, 2015-12-15 at 22:51 +0100, Arnd Bergmann wrote:
> On Wednesday 16 December 2015 00:04:45 Sergei Ianovich wrote:
> > +Examples (from pxa27x-lp8x4x.dts):
> > +
> > +               uart@9050 {
> 
> By convention, the name should be 'serial', not 'uart'.
> 

arch/arm/boot/dts/pxa2xx.dtsi uses 'uart'. Should I change the names in
the patch with dts file for LP-8xx?

[PATCH v6] serial: support for 16550A serial ports on LP-8x4x

[Sergei Ianovich]

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.

Signed-off-by: Sergei Ianovich <ynvich-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Reviewed-by: Heikki Krogerus <heikki.krogerus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
CC: Alan Cox <gnomes-qBU/x9rampVanCEyBjwyrvXRex20P6io@public.gmane.org>
CC: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
CC: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
---
   v5..v6
   fix review comments by Arnd Bergmann
   * remove wildcards from compatible
   * update doc file
   * drop interrupt parent from doc file
   * replace uart w/ serial in device names in doc file

   fix review comments by Andy Shevchenko
   * exchange labels in switch block
   * replace iowrite8() with writeb()
   * compact comment to one line

   v4..v5
   * constify struct of_device_id
   * drop .owner from struct platform_driver
   * rewrite set_termios() baud rate hadnling as suggested by Alan Cox

   v3..v4
   * move DTS bindings to a different patch (8/21) as suggested by
     Heikki Krogerus

   v2..v3
   * no changes (except number 10/16 -> 12/21)

   v0..v2
   * register platform driver instead of platform device
   * use device tree
   * use devm helpers where possible

 .../bindings/serial/icpdas-lp8841-uart.txt         |  41 +++++
 drivers/tty/serial/8250/8250_lp8841.c              | 166 +++++++++++++++++++++
 drivers/tty/serial/8250/Kconfig                    |  14 ++
 drivers/tty/serial/8250/Makefile                   |   2 +
 4 files changed, 223 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/icpdas-lp8841-uart.txt
 create mode 100644 drivers/tty/serial/8250/8250_lp8841.c

diff --git a/Documentation/devicetree/bindings/serial/icpdas-lp8841-uart.txt b/Documentation/devicetree/bindings/serial/icpdas-lp8841-uart.txt
new file mode 100644
index 0000000..d6acd22
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/icpdas-lp8841-uart.txt
@@ -0,0 +1,41 @@
+* UART ports on ICP DAS LP-8841
+
+LP-8441, LP-8141 and LP-8041 are fully compatible.
+
+ICP DAS LP-8841 contains three additional serial ports interfaced via
+Analog Devices ADM213EA chips in addition to 3 serial ports on PXA CPU.
+
+The chips themselves are standard, they would work with 8250_core if
+properly connected. However, they are not connected normally. Al least
+some of their config pins are wired to a different address region. So
+the driver is board-specific.
+
+Required properties:
+- compatible : should be "icpdas,uart-lp8841"
+
+- reg : should provide 16 byte man IO memory region and 1 byte region for
+	termios
+
+- interrupts : should provide interrupt
+
+Optional property:
+- interrupt-parent : should provide a link to interrupt controller either
+		     explicitly or implicitly from a parent node
+
+Examples (from pxa27x-lp8x4x.dts):
+
+		serial@9050 {
+			compatible = "icpdas,uart-lp8841";
+			reg = <0x9050 0x10
+			       0x9030 0x02>;
+			interrupts = <13>;
+			status = "okay";
+		};
+
+		serial@9060 {
+			compatible = "icpdas,uart-lp8841";
+			reg = <0x9060 0x10
+			       0x9032 0x02>;
+			interrupts = <14>;
+			status = "okay";
+		};
diff --git a/drivers/tty/serial/8250/8250_lp8841.c b/drivers/tty/serial/8250/8250_lp8841.c
new file mode 100644
index 0000000..6fef37f
--- /dev/null
+++ 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/or modify
+ *  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;
+};
+
+static void lp8841_serial_set_termios(struct uart_port *port,
+		struct ktermios *termios, struct ktermios *old)
+{
+	unsigned int len;
+	unsigned int baud;
+	struct lp8841_serial_data *data = port->private_data;
+
+	serial8250_do_set_termios(port, termios, old);
+
+	switch (termios->c_cflag & CSIZE) {
+	case CS5:
+		len = 7;
+		break;
+	case CS6:
+		len = 8;
+		break;
+	case CS7:
+		len = 9;
+		break;
+	case CS8:
+	default:
+		len = 10;
+		break;
+	}
+
+	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;
+	len &= 3;
+	len <<= 3;
+
+	/* Ask the core to calculate the divisor for us. */
+	baud = tty_termios_baud_rate(termios);
+
+	switch (baud) {
+	case 115200:
+		len |= 7;
+		break;
+	case 57600:
+		len |= 6;
+		break;
+	case 38400:
+		len |= 5;
+		break;
+	case 19200:
+		len |= 4;
+		break;
+	case 9600:
+		len |= 3;
+		break;
+	case 4800:
+		len |= 2;
+		break;
+	case 2400:
+	default:
+		len |= 1;
+		break;
+	};
+	writeb(len, data->ios_mem);
+
+}
+
+static const struct of_device_id lp8841_serial_dt_ids[] = {
+	{ .compatible = "icpdas,lp8841-uart", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, lp8841_serial_dt_ids);
+
+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;
+
+	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;
+
+	uart.port.iotype = UPIO_MEM;
+	uart.port.mapbase = mmres->start;
+	uart.port.iobase = mmres->start;
+	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,
+	},
+};
+
+module_platform_driver(lp8841_serial_driver);
+
+MODULE_AUTHOR("Sergei Ianovich");
+MODULE_DESCRIPTION("8250 serial port module for LP-8841");
+MODULE_LICENSE("GPL");
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 3b5cf9c..68640c1 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -394,3 +394,17 @@ config SERIAL_8250_PXA
 	help
 	  If you have a machine based on an Intel XScale PXA2xx CPU you
 	  can enable its onboard serial ports by enabling this option.
+
+	  If you choose M here, the module name will be 8250_pxa.
+
+config SERIAL_8250_LP8841
+	tristate "Support 16550A ports on ICP DAS LP-8841"
+	depends on SERIAL_8250 && MACH_PXA27X_DT
+	select LP8841_IRQ
+	help
+	  In addition to serial ports on PXA270 SoC, LP-8841 has 1 dual
+	  RS232/RS485 port, 1 RS485 port and 1 RS232 port.
+
+	  Say N here, unless you plan to run this kernel on a LP-8841 system.
+
+	  If you choose M here, the module name will be 8250_lp8841.
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index d1e2f2d..10b4bf0 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_SERIAL_8250_ACCENT)	+= 8250_accent.o
 obj-$(CONFIG_SERIAL_8250_BOCA)		+= 8250_boca.o
 obj-$(CONFIG_SERIAL_8250_EXAR_ST16C554)	+= 8250_exar_st16c554.o
 obj-$(CONFIG_SERIAL_8250_HUB6)		+= 8250_hub6.o
+obj-$(CONFIG_SERIAL_8250_LP8X4X)	+= 8250_lp8x4x.o
 obj-$(CONFIG_SERIAL_8250_FSL)		+= 8250_fsl.o
 obj-$(CONFIG_SERIAL_8250_DW)		+= 8250_dw.o
 obj-$(CONFIG_SERIAL_8250_EM)		+= 8250_em.o
@@ -30,5 +31,6 @@ obj-$(CONFIG_SERIAL_8250_INGENIC)	+= 8250_ingenic.o
 obj-$(CONFIG_SERIAL_8250_MID)		+= 8250_mid.o
 obj-$(CONFIG_SERIAL_OF_PLATFORM)	+= 8250_of.o
 obj-$(CONFIG_SERIAL_8250_PXA)		+= 8250_pxa.o
+obj-$(CONFIG_SERIAL_8250_LP8841)	+= 8250_lp8841.o
 
 CFLAGS_8250_ingenic.o += -I$(srctree)/scripts/dtc/libfdt
-- 
2.7.0

--
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

Re: [PATCH v6] serial: support for 16550A serial ports on LP-8x4x

[Andy Shevchenko]

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>

> +++ 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/or
> modify
> + *  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 <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
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

Re: [PATCH v6] serial: support for 16550A serial ports on LP-8x4x

[Sergei Ianovich]

On Mon, 2016-02-29 at 12:29 +0200, Andy Shevchenko wrote:
> On Sat, 2016-02-27 at 19:14 +0300, Sergei Ianovich wrote:
> > +struct lp8841_serial_data {
> > +	int			line;
> > +	void			*ios_mem;
> 
> __iomem

OK

> > +};
> 
> > +	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;

I haven't tested this mode. I am pretty sure it will fail. There is
also no support for speeds higher than 115200.

CS7 and CS8 at speeds up to 115200 work well.

However, there is no way to report errors from set_termios(). Should
anything be done about those limitations?

> +	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.

OK

> > +	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().

OK

> > +
> > +	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).

There is no need for iobase. I'll remove that line.

Re: [PATCH v6] serial: support for 16550A serial ports on LP-8x4x

[One Thousand Gnomes]

> I haven't tested this mode. I am pretty sure it will fail. There is
> also no support for speeds higher than 115200.
> 
> CS7 and CS8 at speeds up to 115200 work well.
> 
> However, there is no way to report errors from set_termios(). Should
> anything be done about those limitations?

You report back by setting the termios fields to the values actually
selected. Eg if you can't do CS5/CS6 you'd pick CS7 or CS8 and write that
back into tty->termios so the caller knows what they actually got. Ditto
for speed (see the 16550A core driver for the speed write backs).

Alan
--
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

[PATCH v7] serial: support for 16550A serial ports on LP-8x4x

[Sergei Ianovich]

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.

Signed-off-by: Sergei Ianovich <ynvich@gmail.com>
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
CC: Alan Cox <gnomes@lxorguk.ukuu.org.uk>
CC: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
CC: Arnd Bergmann <arnd@arndb.de>

   v6..v7
   fix review comments by Andy Shevchenko
   not applying Acked-by as the 1st change is big
   * handle unsupported tty modes correctly as suggested by Alan Cox
   * remove extra check of platform_get_resource() result
   * propagate error code from devm_ioremap_resource()
   * drop uart.port.iobase for UPIO_MEM device

   v5..v6
   fix review comments by Arnd Bergmann
   * remove wildcards from compatible
   * update doc file
   * drop interrupt parent from doc file
   * replace uart w/ serial in device names in doc file

   fix review comments by Andy Shevchenko
   * exchange labels in switch block
   * replace iowrite8() with writeb()
   * compact comment to one line

   v4..v5
   * constify struct of_device_id
   * drop .owner from struct platform_driver
   * rewrite set_termios() baud rate hadnling as suggested by Alan Cox

   v3..v4
   * move DTS bindings to a different patch (8/21) as suggested by
     Heikki Krogerus

   v2..v3
   * no changes (except number 10/16 -> 12/21)

   v0..v2
   * register platform driver instead of platform device
   * use device tree
   * use devm helpers where possible

 .../bindings/serial/icpdas-lp8841-uart.txt         |  41 +++++
 drivers/tty/serial/8250/8250_lp8841.c              | 193 +++++++++++++++++++++
 drivers/tty/serial/8250/Kconfig                    |  14 ++
 drivers/tty/serial/8250/Makefile                   |   2 +
 4 files changed, 250 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/icpdas-lp8841-uart.txt
 create mode 100644 drivers/tty/serial/8250/8250_lp8841.c

diff --git a/Documentation/devicetree/bindings/serial/icpdas-lp8841-uart.txt b/Documentation/devicetree/bindings/serial/icpdas-lp8841-uart.txt
new file mode 100644
index 0000000..d6acd22
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/icpdas-lp8841-uart.txt
@@ -0,0 +1,41 @@
+* UART ports on ICP DAS LP-8841
+
+LP-8441, LP-8141 and LP-8041 are fully compatible.
+
+ICP DAS LP-8841 contains three additional serial ports interfaced via
+Analog Devices ADM213EA chips in addition to 3 serial ports on PXA CPU.
+
+The chips themselves are standard, they would work with 8250_core if
+properly connected. However, they are not connected normally. Al least
+some of their config pins are wired to a different address region. So
+the driver is board-specific.
+
+Required properties:
+- compatible : should be "icpdas,uart-lp8841"
+
+- reg : should provide 16 byte man IO memory region and 1 byte region for
+	termios
+
+- interrupts : should provide interrupt
+
+Optional property:
+- interrupt-parent : should provide a link to interrupt controller either
+		     explicitly or implicitly from a parent node
+
+Examples (from pxa27x-lp8x4x.dts):
+
+		serial@9050 {
+			compatible = "icpdas,uart-lp8841";
+			reg = <0x9050 0x10
+			       0x9030 0x02>;
+			interrupts = <13>;
+			status = "okay";
+		};
+
+		serial@9060 {
+			compatible = "icpdas,uart-lp8841";
+			reg = <0x9060 0x10
+			       0x9032 0x02>;
+			interrupts = <14>;
+			status = "okay";
+		};
diff --git a/drivers/tty/serial/8250/8250_lp8841.c b/drivers/tty/serial/8250/8250_lp8841.c
new file mode 100644
index 0000000..e92c01c
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_lp8841.c
@@ -0,0 +1,193 @@
+/*  linux/drivers/tty/serial/8250/8250_lp8841.c
+ *
+ *  Support for 16550A serial ports on ICP DAS LP-8841
+ *
+ *  Copyright (C) 2013 Sergei Ianovich <ynvich@gmail.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  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 __iomem		*ios_mem;
+};
+
+static void lp8841_serial_set_termios(struct uart_port *port,
+		struct ktermios *termios, struct ktermios *old)
+{
+#ifdef BOTHER
+	unsigned int cbaud;
+#endif
+	unsigned int baud;
+	unsigned int len;
+	unsigned int old_csize = old ? old->c_cflag & CSIZE : CS8;
+	struct lp8841_serial_data *data = port->private_data;
+
+	/* We only support CS7 and CS8 */
+	while ((termios->c_cflag & CSIZE) != CS7 &&
+	       (termios->c_cflag & CSIZE) != CS8) {
+		termios->c_cflag &= ~CSIZE;
+		termios->c_cflag |= old_csize;
+		old_csize = CS8;
+	}
+
+	serial8250_do_set_termios(port, termios, old);
+
+	if ((termios->c_cflag & CSIZE) == CS7)
+		len = 9;
+	else
+		len = 10;
+
+	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;
+	len &= 3;
+	len <<= 3;
+
+	baud = tty_termios_baud_rate(termios);
+
+#ifdef BOTHER
+	/* We only support fixed rates */
+	cbaud = termios->c_cflag & CBAUD;
+
+	if (cbaud == BOTHER) {
+		termios->c_cflag &= ~BOTHER;
+
+		/* Don't rewrite B0 */
+		if (baud) {
+			tty_termios_encode_baud_rate(termios, baud, baud);
+			baud = tty_termios_baud_rate(termios);
+
+			/* Set sane default speed if we get 0 */
+			if (!baud) {
+				baud = 9600;
+				tty_termios_encode_baud_rate(termios,
+						baud, baud);
+			}
+		}
+	}
+#endif
+
+	/* We only support up to 115200 */
+	if (baud > 115200) {
+		baud = 115200;
+		tty_termios_encode_baud_rate(termios, baud, baud);
+	}
+
+	switch (baud) {
+	case 115200:
+		len |= 7;
+		break;
+	case 57600:
+		len |= 6;
+		break;
+	case 38400:
+		len |= 5;
+		break;
+	case 19200:
+		len |= 4;
+		break;
+	case 9600:
+		len |= 3;
+		break;
+	case 4800:
+		len |= 2;
+		break;
+	case 2400:
+	default:
+		len |= 1;
+		break;
+	};
+	writeb(len, data->ios_mem);
+
+}
+
+static const struct of_device_id lp8841_serial_dt_ids[] = {
+	{ .compatible = "icpdas,lp8841-uart", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, lp8841_serial_dt_ids);
+
+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)
+		return -ENODEV;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->ios_mem = devm_ioremap_resource(&pdev->dev, mires);
+	if (IS_ERR(data->ios_mem))
+		return PTR_ERR(data->ios_mem);
+
+	uart.port.iotype = UPIO_MEM;
+	uart.port.mapbase = mmres->start;
+	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,
+	},
+};
+
+module_platform_driver(lp8841_serial_driver);
+
+MODULE_AUTHOR("Sergei Ianovich");
+MODULE_DESCRIPTION("8250 serial port module for LP-8841");
+MODULE_LICENSE("GPL");
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 3b5cf9c..68640c1 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -394,3 +394,17 @@ config SERIAL_8250_PXA
 	help
 	  If you have a machine based on an Intel XScale PXA2xx CPU you
 	  can enable its onboard serial ports by enabling this option.
+
+	  If you choose M here, the module name will be 8250_pxa.
+
+config SERIAL_8250_LP8841
+	tristate "Support 16550A ports on ICP DAS LP-8841"
+	depends on SERIAL_8250 && MACH_PXA27X_DT
+	select LP8841_IRQ
+	help
+	  In addition to serial ports on PXA270 SoC, LP-8841 has 1 dual
+	  RS232/RS485 port, 1 RS485 port and 1 RS232 port.
+
+	  Say N here, unless you plan to run this kernel on a LP-8841 system.
+
+	  If you choose M here, the module name will be 8250_lp8841.
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index d1e2f2d..10b4bf0 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_SERIAL_8250_ACCENT)	+= 8250_accent.o
 obj-$(CONFIG_SERIAL_8250_BOCA)		+= 8250_boca.o
 obj-$(CONFIG_SERIAL_8250_EXAR_ST16C554)	+= 8250_exar_st16c554.o
 obj-$(CONFIG_SERIAL_8250_HUB6)		+= 8250_hub6.o
+obj-$(CONFIG_SERIAL_8250_LP8X4X)	+= 8250_lp8x4x.o
 obj-$(CONFIG_SERIAL_8250_FSL)		+= 8250_fsl.o
 obj-$(CONFIG_SERIAL_8250_DW)		+= 8250_dw.o
 obj-$(CONFIG_SERIAL_8250_EM)		+= 8250_em.o
@@ -30,5 +31,6 @@ obj-$(CONFIG_SERIAL_8250_INGENIC)	+= 8250_ingenic.o
 obj-$(CONFIG_SERIAL_8250_MID)		+= 8250_mid.o
 obj-$(CONFIG_SERIAL_OF_PLATFORM)	+= 8250_of.o
 obj-$(CONFIG_SERIAL_8250_PXA)		+= 8250_pxa.o
+obj-$(CONFIG_SERIAL_8250_LP8841)	+= 8250_lp8841.o
 
 CFLAGS_8250_ingenic.o += -I$(srctree)/scripts/dtc/libfdt
-- 
2.7.0

Re: [PATCH v7] serial: support for 16550A serial ports on LP-8x4x

[Andy Shevchenko]

On Tue, 2016-03-01 at 00:26 +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.
> 
> Signed-off-by: Sergei Ianovich <ynvich-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Reviewed-by: Heikki Krogerus <heikki.krogerus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

Few more comments, mostly about style.

> +static void lp8841_serial_set_termios(struct uart_port *port,
> +		struct ktermios *termios, struct ktermios *old)
> +{
> +#ifdef BOTHER
> +	unsigned int cbaud;
> +#endif
> +	unsigned int baud;
> +	unsigned int len;

Since you are writing this to the register at the end maybe 
 - unsigned int -> u8 (write*b* — exactly one byte)
 - len -> value (it's not only about data length)

> +	unsigned int old_csize = old ? old->c_cflag & CSIZE : CS8;
> +	struct lp8841_serial_data *data = port->private_data;
> +
> +	/* We only support CS7 and CS8 */
> +	while ((termios->c_cflag & CSIZE) != CS7 &&
> +	       (termios->c_cflag & CSIZE) != CS8) {
> +		termios->c_cflag &= ~CSIZE;
> +		termios->c_cflag |= old_csize;
> +		old_csize = CS8;
> +	}
> +
> +	serial8250_do_set_termios(port, termios, old);
> +
> +	if ((termios->c_cflag & CSIZE) == CS7)
> +		len = 9;
> +	else
> +		len = 10;
> +
> +	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

I don't know if someone likes it or not (up to you), but for me looks
better to have ternary operators here:

value += (termios->c_cflag & CSTOPB) ? 1 : 0;
value += (termios->c_cflag & PARENB) ? 1 : 0;
value += (termios->c_cflag & PARODD) ? 0 : 1;

#ifdef CMSPAR
value += (termios->c_cflag & CMSPAR) ? 1 : 0;
#endif

> +	len -= 9;

This one could be part of previous evaluation:
 value = (termios->c_cflag & CSIZE) == CS7 ? 0 : 1;

> +	len &= 3;
> +	len <<= 3;

Perhaps to define magic number (e.g. LP8841_DATA_LEN_SHIFT).

> +
> +	baud = tty_termios_baud_rate(termios);
> +
> +#ifdef BOTHER
> +	/* We only support fixed rates */
> +	cbaud = termios->c_cflag & CBAUD;
> +
> +	if (cbaud == BOTHER) {
> +		termios->c_cflag &= ~BOTHER;
> +
> +		/* Don't rewrite B0 */
> +		if (baud) {
> +			tty_termios_encode_baud_rate(termios, baud,
> baud);
> +			baud = tty_termios_baud_rate(termios);
> +
> +			/* Set sane default speed if we get 0 */
> +			if (!baud) {
> +				baud = 9600;

> +				tty_termios_encode_baud_rate(termios
> ,
> +						baud, baud);

I think you can call this unconditionally together with case > 115200.

> +			}
> +		}
> +	}
> +#endif
> +
> +	/* We only support up to 115200 */
> +	if (baud > 115200) {
> +		baud = 115200;
> +		tty_termios_encode_baud_rate(termios, baud, baud);
> +	}

Btw, can we use uart_get_baud_rate() here?


> +	writeb(len, data->ios_mem);
> +
> +}


> +static int lp8841_serial_probe(struct platform_device *pdev)
> +{
> +	struct uart_8250_port uart = {};

{0}

> +	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);

Perhaps move it down to be closer to devm_ioremap_resource() call.

> +	if (!mmres)
> +		return -ENODEV;
> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +

+	mires = platform_get_resource(pdev, IORESOURCE_MEM, 1);

> +	data->ios_mem = devm_ioremap_resource(&pdev->dev, mires);
> +	if (IS_ERR(data->ios_mem))
> +		return PTR_ERR(data->ios_mem);

-- 
Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
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

Re: [PATCH v7] serial: support for 16550A serial ports on LP-8x4x

[Sergei Ianovich]

On Tue, 2016-03-01 at 13:06 +0200, Andy Shevchenko wrote:
> On Tue, 2016-03-01 at 00:26 +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.
> > 
> > Signed-off-by: Sergei Ianovich <ynvich-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Reviewed-by: Heikki Krogerus <heikki.krogerus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> 
> Few more comments, mostly about style.
> 
> > +static void lp8841_serial_set_termios(struct uart_port *port,
> > +		struct ktermios *termios, struct ktermios *old)
> > +{
> > +#ifdef BOTHER
> > +	unsigned int cbaud;
> > +#endif
> > +	unsigned int baud;
> > +	unsigned int len;
> 
> Since you are writing this to the register at the end maybe 
>  - unsigned int -> u8 (write*b* — exactly one byte)
>  - len -> value (it's not only about data length)
> 
> > +	unsigned int old_csize = old ? old->c_cflag & CSIZE : CS8;
> > +	struct lp8841_serial_data *data = port->private_data;
> > +
> > +	/* We only support CS7 and CS8 */
> > +	while ((termios->c_cflag & CSIZE) != CS7 &&
> > +	       (termios->c_cflag & CSIZE) != CS8) {
> > +		termios->c_cflag &= ~CSIZE;
> > +		termios->c_cflag |= old_csize;
> > +		old_csize = CS8;
> > +	}
> > +
> > +	serial8250_do_set_termios(port, termios, old);
> > +
> > +	if ((termios->c_cflag & CSIZE) == CS7)
> > +		len = 9;
> > +	else
> > +		len = 10;
> > +
> > +	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
> 
> I don't know if someone likes it or not (up to you), but for me looks
> better to have ternary operators here:
> 
> value += (termios->c_cflag & CSTOPB) ? 1 : 0;
> value += (termios->c_cflag & PARENB) ? 1 : 0;
> value += (termios->c_cflag & PARODD) ? 0 : 1;
> 
> #ifdef CMSPAR
> value += (termios->c_cflag & CMSPAR) ? 1 : 0;
> #endif
> 
> > +	len -= 9;
> 
> This one could be part of previous evaluation:
>  value = (termios->c_cflag & CSIZE) == CS7 ? 0 : 1;

Great point.

> > +	len &= 3;
> > +	len <<= 3;
> 
> Perhaps to define magic number (e.g. LP8841_DATA_LEN_SHIFT).

OK

> > +
> > +	baud = tty_termios_baud_rate(termios);
> > +
> > +#ifdef BOTHER
> > +	/* We only support fixed rates */
> > +	cbaud = termios->c_cflag & CBAUD;
> > +
> > +	if (cbaud == BOTHER) {
> > +		termios->c_cflag &= ~BOTHER;
> > +
> > +		/* Don't rewrite B0 */
> > +		if (baud) {
> > +			tty_termios_encode_baud_rate(termios,
> > baud,
> > baud);
> > +			baud = tty_termios_baud_rate(termios);
> > +
> > +			/* Set sane default speed if we get 0 */
> > +			if (!baud) {
> > +				baud = 9600;
> 
> > +				tty_termios_encode_baud_rate(termi
> > os
> > ,
> > +						baud, baud);
> 
> I think you can call this unconditionally together with case >
> 115200.

The calls are orthogonal. This one deals with the case when BOTHER is
defined and set, and we have non-zero rate with BOTHER, but we have
zero rate after BOTHER is cleared. So we set 9600 as a sane default
speed.

> > +			}
> > +		}
> > +	}
> > +#endif
> > +
> > +	/* We only support up to 115200 */
> > +	if (baud > 115200) {
> > +		baud = 115200;
> > +		tty_termios_encode_baud_rate(termios, baud, baud);
> > +	}

This one deals with the case when the rate is over 115200. If the
previous case has been triggered, this one won't be.

> Btw, can we use uart_get_baud_rate() here?

uart_get_baud_rate() has already been called
in serial8250_do_set_termios(). uart_get_baud_rate()
calls tty_termios_encode_baud_rate(). uart_get_baud_rate() won't help
us if BOTHER is set. Once BOTHER is cleared, we don't need any special
processing of uart_get_baud_rate().

> > +static int lp8841_serial_probe(struct platform_device *pdev)
> > +{
> > +     struct uart_8250_port uart = {};
> 
> {0}

---
drivers/tty/serial/8250/8250_lp8841.c: In function 'lp8841_serial_probe':
drivers/tty/serial/8250/8250_lp8841.c:124:32: warning: excess elements in struct initializer
  struct uart_8250_port uart = {0};
                                ^
drivers/tty/serial/8250/8250_lp8841.c:124:32: note: (near initialization for 'uart.port.lock.<anonymous>.rlock.raw_lock')
---

Zero triggers a warning. I'll use memset().

> > +     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);
> 
> Perhaps move it down to be closer to devm_ioremap_resource() call.

OK

Thanks for lightning fast reviews. I'll resubmit v8 if there is no
objections to the points above.
--
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

Re: [PATCH v7] serial: support for 16550A serial ports on LP-8x4x

[Andy Shevchenko]

On Tue, 2016-03-01 at 19:25 +0300, Sergei Ianovich wrote:
> On Tue, 2016-03-01 at 13:06 +0200, Andy Shevchenko wrote:
> > On Tue, 2016-03-01 at 00:26 +0300, Sergei Ianovich wrote:

> > > +	len &= 3;

Mask as well to be defined.

> > > +	len <<= 3;
> > 
> > Perhaps to define magic number (e.g. LP8841_DATA_LEN_SHIFT).
> 
> OK


> +	baud = tty_termios_baud_rate(termios);
> > > +
> > > +#ifdef BOTHER
> > > +	/* We only support fixed rates */

So, but if you support only fixed rates, why do you care about BOTHER
at all?

> > > 
> > I think you can call this unconditionally together with case >
> > 115200.
> 
> The calls are orthogonal. This one deals with the case when BOTHER is
> defined and set, and we have non-zero rate with BOTHER, but we have
> zero rate after BOTHER is cleared. So we set 9600 as a sane default
> speed.

> +
> > > +	/* We only support up to 115200 */
> > > +	if (baud > 115200) {
> > > +		baud = 115200;
> > > +		tty_termios_encode_baud_rate(termios, baud,
> > > baud);
> > > +	}
> 
> This one deals with the case when the rate is over 115200. If the
> previous case has been triggered, this one won't be.

Yeah, but I meant to unconditionally call it just once here every time.
 tty_termios_encode_baud_rate(termios, baud, baud);

> +static int lp8841_serial_probe(struct platform_device *pdev)
> > > +{
> > > +     struct uart_8250_port uart = {};
> > 
> > {0}
> 
> ---
> drivers/tty/serial/8250/8250_lp8841.c: In function
> 'lp8841_serial_probe':
> drivers/tty/serial/8250/8250_lp8841.c:124:32: warning: excess
> elements in struct initializer
>   struct uart_8250_port uart = {0};
>                                 ^
> drivers/tty/serial/8250/8250_lp8841.c:124:32: note: (near
> initialization for 'uart.port.lock.<anonymous>.rlock.raw_lock')

Do you have any warning verbosity enabled? I see a lot of stuff like
this in the code

$ git grep -n 'struct .* = {0};' | wc -l
338

$ git grep -n 'struct .* = { \?0 \?};' | wc -l
550

( '… = { 0 };' included)

> ---
> 
> Zero triggers a warning. I'll use memset().

Either will work.

> Thanks for lightning fast reviews. I'll resubmit v8 if there is no
> objections to the points above.

See above.

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

Re: [PATCH v7] serial: support for 16550A serial ports on LP-8x4x

[Sergei Ianovich]

On Tue, 2016-03-01 at 18:46 +0200, Andy Shevchenko wrote:
> On Tue, 2016-03-01 at 19:25 +0300, Sergei Ianovich wrote:
> > On Tue, 2016-03-01 at 13:06 +0200, Andy Shevchenko wrote:
> > > On Tue, 2016-03-01 at 00:26 +0300, Sergei Ianovich wrote:
> 
> > > > +	len &= 3;
> 
> Mask as well to be defined.

Sure.

> So, but if you support only fixed rates, why do you care about BOTHER
> at all?

If BOTHER is defined, tty_termios_baud_rate()
and tty_termios_encode_baud_rate() allow non-standard baud rates. I
should clear it from c_cflag to indicate I don't support it.

> > > >  
> > > I think you can call this unconditionally together with case >
> > > 115200.
> > 
> > The calls are orthogonal. This one deals with the case when BOTHER
> > is
> > defined and set, and we have non-zero rate with BOTHER, but we have
> > zero rate after BOTHER is cleared. So we set 9600 as a sane default
> > speed.

> > 
> > This one deals with the case when the rate is over 115200. If the
> > previous case has been triggered, this one won't be.
> 
> Yeah, but I meant to unconditionally call it just once here every
> time.

I see. It saves a few lines.

> > ---
> > drivers/tty/serial/8250/8250_lp8841.c: In function
> > 'lp8841_serial_probe':
> > drivers/tty/serial/8250/8250_lp8841.c:124:32: warning: excess
> > elements in struct initializer
> >   struct uart_8250_port uart = {0};
> >                                 ^
> > drivers/tty/serial/8250/8250_lp8841.c:124:32: note: (near
> > initialization for 'uart.port.lock.<anonymous>.rlock.raw_lock')
> 
> Do you have any warning verbosity enabled? I see a lot of stuff like
> this in the code

Plain `make`.

The warning seems to be the result of initializing a spinlock with
zero. Spinlocks are intentionally obfuscated, but I didn't investigate
further.

> $ git grep -n 'struct .* = {0};' | wc -l
> 338
> 
> $ git grep -n 'struct .* = { \?0 \?};' | wc -l
> 550
> 
> ( '… = { 0 };' included)

The first structure member is most likely not a spinlock in those
cases.

> > ---
> > 
> > Zero triggers a warning. I'll use memset().
> 
> Either will work.

OK

The only remaining open point is BOTHER handling.
--
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

Re: [PATCH v7] serial: support for 16550A serial ports on LP-8x4x

[Andy Shevchenko]

On Tue, 2016-03-01 at 20:14 +0300, Sergei Ianovich wrote:
> On Tue, 2016-03-01 at 18:46 +0200, Andy Shevchenko wrote:
> > On Tue, 2016-03-01 at 19:25 +0300, Sergei Ianovich wrote:
> > > On Tue, 2016-03-01 at 13:06 +0200, Andy Shevchenko wrote:
> > > > On Tue, 2016-03-01 at 00:26 +0300, Sergei Ianovich wrote:

> > So, but if you support only fixed rates, why do you care about
> > BOTHER
> > at all?
> 
> If BOTHER is defined, tty_termios_baud_rate()
> and tty_termios_encode_baud_rate() allow non-standard baud rates. I
> should clear it from c_cflag to indicate I don't support it.
> 
> > > > >  
> > > > I think you can call this unconditionally together with case >
> > > > 115200.
> > > 
> > > The calls are orthogonal. This one deals with the case when
> > > BOTHER
> > > is
> > > defined and set, and we have non-zero rate with BOTHER, but we
> > > have
> > > zero rate after BOTHER is cleared. So we set 9600 as a sane
> > > default
> > > speed.

Maybe you just set a baud rate nearest to the one from the table in
case of BOTHER?

In that case perhaps you have to supply +-1 to the range. That's why I
asked about uart_get_baud_rate(). 

Maybe this flow will work for you

if (BOTHER)
 clear BOTHER
 call uart_get_baud_rate()

?

> The warning seems to be the result of initializing a spinlock with
> zero. Spinlocks are intentionally obfuscated, but I didn't
> investigate
> further.
> 
> > $ git grep -n 'struct .* = {0};' | wc -l
> > 338
> > 
> > $ git grep -n 'struct .* = { \?0 \?};' | wc -l
> > 550
> > 
> > ( '… = { 0 };' included)
> 
> The first structure member is most likely not a spinlock in those
> cases.

Hmm... Interesting. On one hand the poison is reasonable, on the other
we often do a memset() or {0} on structures, i.o.w. assign 0 as initial
value until spinlock_init().

Arnd, what do you think about this (and similar) case(s)?

-- 
Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
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

Re: [PATCH v7] serial: support for 16550A serial ports on LP-8x4x

[One Thousand Gnomes]

> Maybe you just set a baud rate nearest to the one from the table in
> case of BOTHER?

This is broken. BOTHER can be set with a perfectly valid baud rate that
could equally be represented by B9600 say.

If you are stuck with limited ranges then

	switch(baud) {
	case 9600:
	case 4800:

etc

and don't worry about BOTHER, it's entirely transparent to you. The core
kernel code will provide you with a baud rate number, the re-encoder will
always do the right thing.

A driver should never care about BOTHER or any of the baud bits in the
termios structure directly.

Alan

Re: [PATCH v7] serial: support for 16550A serial ports on LP-8x4x

[Sergei Ianovich]

On Tue, 2016-03-01 at 19:48 +0200, Andy Shevchenko wrote:
> On Tue, 2016-03-01 at 20:14 +0300, Sergei Ianovich wrote:
> > On Tue, 2016-03-01 at 18:46 +0200, Andy Shevchenko wrote:
> > > On Tue, 2016-03-01 at 19:25 +0300, Sergei Ianovich wrote:
> > > > On Tue, 2016-03-01 at 13:06 +0200, Andy Shevchenko wrote:
> > > > > On Tue, 2016-03-01 at 00:26 +0300, Sergei Ianovich wrote:
> 
> > > So, but if you support only fixed rates, why do you care about
> > > BOTHER
> > > at all?
> > 
> > If BOTHER is defined, tty_termios_baud_rate()
> > and tty_termios_encode_baud_rate() allow non-standard baud rates. I
> > should clear it from c_cflag to indicate I don't support it.
> > 
> > > > > >  
> > > > > I think you can call this unconditionally together with case
> > > > > >
> > > > > 115200.
> > > > 
> > > > The calls are orthogonal. This one deals with the case when
> > > > BOTHER
> > > > is
> > > > defined and set, and we have non-zero rate with BOTHER, but we
> > > > have
> > > > zero rate after BOTHER is cleared. So we set 9600 as a sane
> > > > default
> > > > speed.
> 
> Maybe you just set a baud rate nearest to the one from the table in
> case of BOTHER?
> 
> In that case perhaps you have to supply +-1 to the range. That's why
> I
> asked about uart_get_baud_rate(). 
> 
> Maybe this flow will work for you
> 
> if (BOTHER)
>  clear BOTHER
>  call uart_get_baud_rate()
> 
> ?

It works well for standard rates, let it be so. If there ever is a
problem, we can fix it.
--
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

Re: [PATCH v7] serial: support for 16550A serial ports on LP-8x4x

[One Thousand Gnomes]

> > Maybe this flow will work for you
> > 
> > if (BOTHER)
> >  clear BOTHER
> >  call uart_get_baud_rate()
> > 
> > ?  
> 
> It works well for standard rates, let it be so. If there ever is a
> problem, we can fix it.

I'm NAKking the v7 PATCH because we spent ages getting all the drivers to
use tty_termios_get_baud_rate() cleanly.

Get rid of everything in the ifdef BOTHER
Remove the if baud > 115200 stuff

For the default: entry in the case add

             tty_termios_encode_baud_rate(termios, 2400, 2400);

and all will be good. Anything not a standard rate will get 2400 baud and
reported back to the user properly as that rate.

You could do matches for "within 10%" but really I don't think it matters
and other drivers don't bother either when they have such fixed clocks.

Alan
--
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

[PATCH v8] serial: support for 16550A serial ports on LP-8x4x

[Sergei Ianovich]

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.

Signed-off-by: Sergei Ianovich <ynvich@gmail.com>
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
CC: Alan Cox <gnomes@lxorguk.ukuu.org.uk>
CC: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
CC: Arnd Bergmann <arnd@arndb.de>

   v7..v8
   * call serial8250_do_set_termios() after speed check, not before.
     This way clock divisor is properly inited for the new baud rate,
     if any
   fix review comments by Andy Shevchenko
   * change board variable name and type
   * use ternary operators
   * use #defines instead of magic numbers
   * simplify speed check and use uart_get_baud_rate()
   * zero-init uart structure
   * re-organized probing calls

   v6..v7
   fix review comments by Andy Shevchenko
   not applying Acked-by as the 1st change is big
   * handle unsupported tty modes correctly
   * remove extra check of platform_get_resource() result
   * propagate error code from devm_ioremap_resource()
   * drop uart.port.iobase for UPIO_MEM device

   v5..v6
   fix review comments by Arnd Bergmann
   * remove wildcards from compatible
   * update doc file
   * drop interrupt parent from doc file
   * replace uart w/ serial in device names in doc file

   fix review comments by Andy Shevchenko
   * exchange labels in switch block
   * replace iowrite8() with writeb()
   * compact comment to one line

   v4..v5
   * constify struct of_device_id
   * drop .owner from struct platform_driver
   * rewrite set_termios() baud rate hadnling as suggested by Alan Cox

   v3..v4
   * move DTS bindings to a different patch (8/21) as suggested by
     Heikki Krogerus

   v2..v3
   * no changes (except number 10/16 -> 12/21)

   v0..v2
   * register platform driver instead of platform device
   * use device tree
   * use devm helpers where possible

 .../bindings/serial/icpdas-lp8841-uart.txt         |  41 +++++
 drivers/tty/serial/8250/8250_lp8841.c              | 173 +++++++++++++++++++++
 drivers/tty/serial/8250/Kconfig                    |  14 ++
 drivers/tty/serial/8250/Makefile                   |   2 +
 4 files changed, 230 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/icpdas-lp8841-uart.txt
 create mode 100644 drivers/tty/serial/8250/8250_lp8841.c

diff --git a/Documentation/devicetree/bindings/serial/icpdas-lp8841-uart.txt b/Documentation/devicetree/bindings/serial/icpdas-lp8841-uart.txt
new file mode 100644
index 0000000..d6acd22
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/icpdas-lp8841-uart.txt
@@ -0,0 +1,41 @@
+* UART ports on ICP DAS LP-8841
+
+LP-8441, LP-8141 and LP-8041 are fully compatible.
+
+ICP DAS LP-8841 contains three additional serial ports interfaced via
+Analog Devices ADM213EA chips in addition to 3 serial ports on PXA CPU.
+
+The chips themselves are standard, they would work with 8250_core if
+properly connected. However, they are not connected normally. Al least
+some of their config pins are wired to a different address region. So
+the driver is board-specific.
+
+Required properties:
+- compatible : should be "icpdas,uart-lp8841"
+
+- reg : should provide 16 byte man IO memory region and 1 byte region for
+	termios
+
+- interrupts : should provide interrupt
+
+Optional property:
+- interrupt-parent : should provide a link to interrupt controller either
+		     explicitly or implicitly from a parent node
+
+Examples (from pxa27x-lp8x4x.dts):
+
+		serial@9050 {
+			compatible = "icpdas,uart-lp8841";
+			reg = <0x9050 0x10
+			       0x9030 0x02>;
+			interrupts = <13>;
+			status = "okay";
+		};
+
+		serial@9060 {
+			compatible = "icpdas,uart-lp8841";
+			reg = <0x9060 0x10
+			       0x9032 0x02>;
+			interrupts = <14>;
+			status = "okay";
+		};
diff --git a/drivers/tty/serial/8250/8250_lp8841.c b/drivers/tty/serial/8250/8250_lp8841.c
new file mode 100644
index 0000000..d80e218
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_lp8841.c
@@ -0,0 +1,173 @@
+/*  linux/drivers/tty/serial/8250/8250_lp8841.c
+ *
+ *  Support for 16550A serial ports on ICP DAS LP-8841
+ *
+ *  Copyright (C) 2013 Sergei Ianovich <ynvich@gmail.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  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 __iomem		*ios_mem;
+};
+
+#define LP8841_DATA_LEN_MASK		0x3
+#define LP8841_DATA_LEN_SHIFT_OFFSET	3
+
+static void lp8841_serial_set_termios(struct uart_port *port,
+		struct ktermios *termios, struct ktermios *old)
+{
+#ifdef BOTHER
+	unsigned int cbaud;
+#endif
+	unsigned int baud;
+	u8 value;
+	unsigned int old_csize = old ? old->c_cflag & CSIZE : CS8;
+	struct lp8841_serial_data *data = port->private_data;
+
+	/* We only support CS7 and CS8 */
+	while ((termios->c_cflag & CSIZE) != CS7 &&
+	       (termios->c_cflag & CSIZE) != CS8) {
+		termios->c_cflag &= ~CSIZE;
+		termios->c_cflag |= old_csize;
+		old_csize = CS8;
+	}
+
+	value =  (termios->c_cflag & CSIZE) == CS7 ? 0 : 1;
+	value += (termios->c_cflag & CSTOPB) ? 1 : 0;
+	value += (termios->c_cflag & PARENB) ? 1 : 0;
+	value += (termios->c_cflag & PARODD) ? 0 : 1;
+#ifdef CMSPAR
+	value += (termios->c_cflag & CMSPAR) ? 1 : 0;
+#endif
+
+	value &=  LP8841_DATA_LEN_MASK;
+	value <<= LP8841_DATA_LEN_SHIFT_OFFSET;
+
+	baud = tty_termios_baud_rate(termios);
+
+#ifdef BOTHER
+	/* We only support fixed rates */
+	cbaud = termios->c_cflag & CBAUD;
+
+	if (cbaud == BOTHER)
+		termios->c_cflag &= ~BOTHER;
+#endif
+
+	/* We only support up to 115200 */
+	if (baud > 115200)
+		baud = 115200;
+
+	baud = uart_get_baud_rate(port, termios, old, baud, baud);
+
+	serial8250_do_set_termios(port, termios, old);
+
+	switch (baud) {
+	case 115200:
+		value |= 7;
+		break;
+	case 57600:
+		value |= 6;
+		break;
+	case 38400:
+		value |= 5;
+		break;
+	case 19200:
+		value |= 4;
+		break;
+	case 9600:
+		value |= 3;
+		break;
+	case 4800:
+		value |= 2;
+		break;
+	case 2400:
+	default:
+		value |= 1;
+		break;
+	};
+	writeb(value, data->ios_mem);
+}
+
+static const struct of_device_id lp8841_serial_dt_ids[] = {
+	{ .compatible = "icpdas,lp8841-uart", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, lp8841_serial_dt_ids);
+
+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;
+
+	memset(&uart, 0, sizeof(uart));
+
+	mmres = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!mmres)
+		return -ENODEV;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	mires = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	data->ios_mem = devm_ioremap_resource(&pdev->dev, mires);
+	if (IS_ERR(data->ios_mem))
+		return PTR_ERR(data->ios_mem);
+
+	uart.port.iotype = UPIO_MEM;
+	uart.port.mapbase = mmres->start;
+	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,
+	},
+};
+
+module_platform_driver(lp8841_serial_driver);
+
+MODULE_AUTHOR("Sergei Ianovich");
+MODULE_DESCRIPTION("8250 serial port module for LP-8841");
+MODULE_LICENSE("GPL");
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 3b5cf9c..68640c1 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -394,3 +394,17 @@ config SERIAL_8250_PXA
 	help
 	  If you have a machine based on an Intel XScale PXA2xx CPU you
 	  can enable its onboard serial ports by enabling this option.
+
+	  If you choose M here, the module name will be 8250_pxa.
+
+config SERIAL_8250_LP8841
+	tristate "Support 16550A ports on ICP DAS LP-8841"
+	depends on SERIAL_8250 && MACH_PXA27X_DT
+	select LP8841_IRQ
+	help
+	  In addition to serial ports on PXA270 SoC, LP-8841 has 1 dual
+	  RS232/RS485 port, 1 RS485 port and 1 RS232 port.
+
+	  Say N here, unless you plan to run this kernel on a LP-8841 system.
+
+	  If you choose M here, the module name will be 8250_lp8841.
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index d1e2f2d..10b4bf0 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_SERIAL_8250_ACCENT)	+= 8250_accent.o
 obj-$(CONFIG_SERIAL_8250_BOCA)		+= 8250_boca.o
 obj-$(CONFIG_SERIAL_8250_EXAR_ST16C554)	+= 8250_exar_st16c554.o
 obj-$(CONFIG_SERIAL_8250_HUB6)		+= 8250_hub6.o
+obj-$(CONFIG_SERIAL_8250_LP8X4X)	+= 8250_lp8x4x.o
 obj-$(CONFIG_SERIAL_8250_FSL)		+= 8250_fsl.o
 obj-$(CONFIG_SERIAL_8250_DW)		+= 8250_dw.o
 obj-$(CONFIG_SERIAL_8250_EM)		+= 8250_em.o
@@ -30,5 +31,6 @@ obj-$(CONFIG_SERIAL_8250_INGENIC)	+= 8250_ingenic.o
 obj-$(CONFIG_SERIAL_8250_MID)		+= 8250_mid.o
 obj-$(CONFIG_SERIAL_OF_PLATFORM)	+= 8250_of.o
 obj-$(CONFIG_SERIAL_8250_PXA)		+= 8250_pxa.o
+obj-$(CONFIG_SERIAL_8250_LP8841)	+= 8250_lp8841.o
 
 CFLAGS_8250_ingenic.o += -I$(srctree)/scripts/dtc/libfdt
-- 
2.7.0

[PATCH v9] serial: support for 16550A serial ports on LP-8x4x

[Sergei Ianovich]

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.

Signed-off-by: Sergei Ianovich <ynvich-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Reviewed-by: Heikki Krogerus <heikki.krogerus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
CC: Alan Cox <gnomes-qBU/x9rampVanCEyBjwyrvXRex20P6io@public.gmane.org>
CC: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
CC: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>

   v8..v9
   fix review comments by Alan Cox
   * further simplify speed check

   v7..v8
   * call serial8250_do_set_termios() after speed check, not before.
     This way clock divisor is properly inited for the new baud rate,
     if any
   fix review comments by Andy Shevchenko
   * change board variable name and type
   * use ternary operators
   * use #defines instead of magic numbers
   * simplify speed check and use uart_get_baud_rate()
   * zero-init uart structure
   * re-organized probing calls

   v6..v7
   fix review comments by Andy Shevchenko
   not applying Acked-by as the 1st change is big
   * handle unsupported tty modes correctly
   * remove extra check of platform_get_resource() result
   * propagate error code from devm_ioremap_resource()
   * drop uart.port.iobase for UPIO_MEM device

   v5..v6
   fix review comments by Arnd Bergmann
   * remove wildcards from compatible
   * update doc file
   * drop interrupt parent from doc file
   * replace uart w/ serial in device names in doc file

   fix review comments by Andy Shevchenko
   * exchange labels in switch block
   * replace iowrite8() with writeb()
   * compact comment to one line

   v4..v5
   * constify struct of_device_id
   * drop .owner from struct platform_driver
   * rewrite set_termios() baud rate hadnling as suggested by Alan Cox

   v3..v4
   * move DTS bindings to a different patch (8/21) as suggested by
     Heikki Krogerus

   v2..v3
   * no changes (except number 10/16 -> 12/21)

   v0..v2
   * register platform driver instead of platform device
   * use device tree
   * use devm helpers where possible

 .../bindings/serial/icpdas-lp8841-uart.txt         |  41 ++++++
 drivers/tty/serial/8250/8250_lp8841.c              | 159 +++++++++++++++++++++
 drivers/tty/serial/8250/Kconfig                    |  14 ++
 drivers/tty/serial/8250/Makefile                   |   2 +
 4 files changed, 216 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/icpdas-lp8841-uart.txt
 create mode 100644 drivers/tty/serial/8250/8250_lp8841.c

diff --git a/Documentation/devicetree/bindings/serial/icpdas-lp8841-uart.txt b/Documentation/devicetree/bindings/serial/icpdas-lp8841-uart.txt
new file mode 100644
index 0000000..d6acd22
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/icpdas-lp8841-uart.txt
@@ -0,0 +1,41 @@
+* UART ports on ICP DAS LP-8841
+
+LP-8441, LP-8141 and LP-8041 are fully compatible.
+
+ICP DAS LP-8841 contains three additional serial ports interfaced via
+Analog Devices ADM213EA chips in addition to 3 serial ports on PXA CPU.
+
+The chips themselves are standard, they would work with 8250_core if
+properly connected. However, they are not connected normally. Al least
+some of their config pins are wired to a different address region. So
+the driver is board-specific.
+
+Required properties:
+- compatible : should be "icpdas,uart-lp8841"
+
+- reg : should provide 16 byte man IO memory region and 1 byte region for
+	termios
+
+- interrupts : should provide interrupt
+
+Optional property:
+- interrupt-parent : should provide a link to interrupt controller either
+		     explicitly or implicitly from a parent node
+
+Examples (from pxa27x-lp8x4x.dts):
+
+		serial@9050 {
+			compatible = "icpdas,uart-lp8841";
+			reg = <0x9050 0x10
+			       0x9030 0x02>;
+			interrupts = <13>;
+			status = "okay";
+		};
+
+		serial@9060 {
+			compatible = "icpdas,uart-lp8841";
+			reg = <0x9060 0x10
+			       0x9032 0x02>;
+			interrupts = <14>;
+			status = "okay";
+		};
diff --git a/drivers/tty/serial/8250/8250_lp8841.c b/drivers/tty/serial/8250/8250_lp8841.c
new file mode 100644
index 0000000..548f382
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_lp8841.c
@@ -0,0 +1,159 @@
+/*  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/or modify
+ *  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 __iomem		*ios_mem;
+};
+
+#define LP8841_DATA_LEN_MASK		0x3
+#define LP8841_DATA_LEN_SHIFT_OFFSET	3
+
+static void lp8841_serial_set_termios(struct uart_port *port,
+		struct ktermios *termios, struct ktermios *old)
+{
+	unsigned int baud;
+	u8 value;
+	unsigned int old_csize = old ? old->c_cflag & CSIZE : CS8;
+	struct lp8841_serial_data *data = port->private_data;
+
+	/* We only support CS7 and CS8 */
+	while ((termios->c_cflag & CSIZE) != CS7 &&
+	       (termios->c_cflag & CSIZE) != CS8) {
+		termios->c_cflag &= ~CSIZE;
+		termios->c_cflag |= old_csize;
+		old_csize = CS8;
+	}
+
+	value =  (termios->c_cflag & CSIZE) == CS7 ? 0 : 1;
+	value += (termios->c_cflag & CSTOPB) ? 1 : 0;
+	value += (termios->c_cflag & PARENB) ? 1 : 0;
+	value += (termios->c_cflag & PARODD) ? 0 : 1;
+#ifdef CMSPAR
+	value += (termios->c_cflag & CMSPAR) ? 1 : 0;
+#endif
+
+	value &=  LP8841_DATA_LEN_MASK;
+	value <<= LP8841_DATA_LEN_SHIFT_OFFSET;
+
+	baud = tty_termios_baud_rate(termios);
+
+	switch (baud) {
+	case 115200:
+		value |= 7;
+		break;
+	case 57600:
+		value |= 6;
+		break;
+	case 38400:
+		value |= 5;
+		break;
+	case 19200:
+		value |= 4;
+		break;
+	case 9600:
+		value |= 3;
+		break;
+	case 4800:
+		value |= 2;
+		break;
+	case 2400:
+		value |= 1;
+		break;
+	default:
+		value |= 1;
+		tty_termios_encode_baud_rate(termios, 2400, 2400);
+		break;
+	};
+	writeb(value, data->ios_mem);
+
+	serial8250_do_set_termios(port, termios, old);
+}
+
+static const struct of_device_id lp8841_serial_dt_ids[] = {
+	{ .compatible = "icpdas,lp8841-uart", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, lp8841_serial_dt_ids);
+
+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;
+
+	memset(&uart, 0, sizeof(uart));
+
+	mmres = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!mmres)
+		return -ENODEV;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	mires = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	data->ios_mem = devm_ioremap_resource(&pdev->dev, mires);
+	if (IS_ERR(data->ios_mem))
+		return PTR_ERR(data->ios_mem);
+
+	uart.port.iotype = UPIO_MEM;
+	uart.port.mapbase = mmres->start;
+	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,
+	},
+};
+
+module_platform_driver(lp8841_serial_driver);
+
+MODULE_AUTHOR("Sergei Ianovich");
+MODULE_DESCRIPTION("8250 serial port module for LP-8841");
+MODULE_LICENSE("GPL");
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 3b5cf9c..68640c1 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -394,3 +394,17 @@ config SERIAL_8250_PXA
 	help
 	  If you have a machine based on an Intel XScale PXA2xx CPU you
 	  can enable its onboard serial ports by enabling this option.
+
+	  If you choose M here, the module name will be 8250_pxa.
+
+config SERIAL_8250_LP8841
+	tristate "Support 16550A ports on ICP DAS LP-8841"
+	depends on SERIAL_8250 && MACH_PXA27X_DT
+	select LP8841_IRQ
+	help
+	  In addition to serial ports on PXA270 SoC, LP-8841 has 1 dual
+	  RS232/RS485 port, 1 RS485 port and 1 RS232 port.
+
+	  Say N here, unless you plan to run this kernel on a LP-8841 system.
+
+	  If you choose M here, the module name will be 8250_lp8841.
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index d1e2f2d..10b4bf0 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_SERIAL_8250_ACCENT)	+= 8250_accent.o
 obj-$(CONFIG_SERIAL_8250_BOCA)		+= 8250_boca.o
 obj-$(CONFIG_SERIAL_8250_EXAR_ST16C554)	+= 8250_exar_st16c554.o
 obj-$(CONFIG_SERIAL_8250_HUB6)		+= 8250_hub6.o
+obj-$(CONFIG_SERIAL_8250_LP8X4X)	+= 8250_lp8x4x.o
 obj-$(CONFIG_SERIAL_8250_FSL)		+= 8250_fsl.o
 obj-$(CONFIG_SERIAL_8250_DW)		+= 8250_dw.o
 obj-$(CONFIG_SERIAL_8250_EM)		+= 8250_em.o
@@ -30,5 +31,6 @@ obj-$(CONFIG_SERIAL_8250_INGENIC)	+= 8250_ingenic.o
 obj-$(CONFIG_SERIAL_8250_MID)		+= 8250_mid.o
 obj-$(CONFIG_SERIAL_OF_PLATFORM)	+= 8250_of.o
 obj-$(CONFIG_SERIAL_8250_PXA)		+= 8250_pxa.o
+obj-$(CONFIG_SERIAL_8250_LP8841)	+= 8250_lp8841.o
 
 CFLAGS_8250_ingenic.o += -I$(srctree)/scripts/dtc/libfdt
-- 
2.7.0

--
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

Re: [PATCH v9] serial: support for 16550A serial ports on LP-8x4x

[Andy Shevchenko]

On Tue, Mar 1, 2016 at 10:08 PM, Sergei Ianovich <ynvich-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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.
>
> Signed-off-by: Sergei Ianovich <ynvich-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Reviewed-by: Heikki Krogerus <heikki.krogerus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

Sorry, but still few nitpicks and then
Reviewed-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

> +#define LP8841_DATA_LEN_MASK           0x3
> +#define LP8841_DATA_LEN_SHIFT_OFFSET   3

No need to have _OFFSET suffix.

> +
> +static void lp8841_serial_set_termios(struct uart_port *port,
> +               struct ktermios *termios, struct ktermios *old)
> +{
> +       unsigned int baud;
> +       u8 value;

> +       unsigned int old_csize = old ? old->c_cflag & CSIZE : CS8;
> +       struct lp8841_serial_data *data = port->private_data;

I would rearrange to have assignments first in the definition block.

Code duplication (no idea if it worth to fix):

+       case 2400:
+               value |= 1;
+               break;
+       default:
+               value |= 1;
+               tty_termios_encode_baud_rate(termios, 2400, 2400);
+               break;

> +static int lp8841_serial_probe(struct platform_device *pdev)
> +{
> +       struct uart_8250_port uart = {};

No need {} since memset().

> +       struct lp8841_serial_data *data;
> +       struct resource *mmres, *mires;
> +       int ret;
> +
> +       memset(&uart, 0, sizeof(uart));

Move this closer to the first assignment of a field.

> +
> +       mmres = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       if (!mmres)
> +               return -ENODEV;
> +
> +       data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       mires = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +       data->ios_mem = devm_ioremap_resource(&pdev->dev, mires);
> +       if (IS_ERR(data->ios_mem))
> +               return PTR_ERR(data->ios_mem);
> +

+       memset(&uart, 0, sizeof(uart));

> +       uart.port.iotype = UPIO_MEM;
> +       uart.port.mapbase = mmres->start;
> +       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;

-- 
With Best Regards,
Andy Shevchenko
--
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

[PATCH v10] serial: support for 16550A serial ports on LP-8x4x

[Sergei Ianovich]

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.

Signed-off-by: Sergei Ianovich <ynvich@gmail.com>
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
CC: Alan Cox <gnomes@lxorguk.ukuu.org.uk>
CC: Arnd Bergmann <arnd@arndb.de>

   v9..v10
   fix review comments by Andy Shevchenko
   * fix code styling

   v8..v9
   fix review comments by Alan Cox
   * further simplify speed check

   v7..v8
   * call serial8250_do_set_termios() after speed check, not before.
     This way clock divisor is properly inited for the new baud rate,
     if any
   fix review comments by Andy Shevchenko
   * change board variable name and type
   * use ternary operators
   * use #defines instead of magic numbers
   * simplify speed check and use uart_get_baud_rate()
   * zero-init uart structure
   * re-organized probing calls

   v6..v7
   fix review comments by Andy Shevchenko
   not applying Acked-by as the 1st change is big
   * handle unsupported tty modes correctly
   * remove extra check of platform_get_resource() result
   * propagate error code from devm_ioremap_resource()
   * drop uart.port.iobase for UPIO_MEM device

   v5..v6
   fix review comments by Arnd Bergmann
   * remove wildcards from compatible
   * update doc file
   * drop interrupt parent from doc file
   * replace uart w/ serial in device names in doc file

   fix review comments by Andy Shevchenko
   * exchange labels in switch block
   * replace iowrite8() with writeb()
   * compact comment to one line

   v4..v5
   * constify struct of_device_id
   * drop .owner from struct platform_driver
   * rewrite set_termios() baud rate hadnling as suggested by Alan Cox

   v3..v4
   * move DTS bindings to a different patch (8/21) as suggested by
     Heikki Krogerus

   v2..v3
   * no changes (except number 10/16 -> 12/21)

   v0..v2
   * register platform driver instead of platform device
   * use device tree
   * use devm helpers where possible

 .../bindings/serial/icpdas-lp8841-uart.txt         |  41 ++++++
 drivers/tty/serial/8250/8250_lp8841.c              | 156 +++++++++++++++++++++
 drivers/tty/serial/8250/Kconfig                    |  14 ++
 drivers/tty/serial/8250/Makefile                   |   2 +
 4 files changed, 213 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/icpdas-lp8841-uart.txt
 create mode 100644 drivers/tty/serial/8250/8250_lp8841.c

diff --git a/Documentation/devicetree/bindings/serial/icpdas-lp8841-uart.txt b/Documentation/devicetree/bindings/serial/icpdas-lp8841-uart.txt
new file mode 100644
index 0000000..d6acd22
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/icpdas-lp8841-uart.txt
@@ -0,0 +1,41 @@
+* UART ports on ICP DAS LP-8841
+
+LP-8441, LP-8141 and LP-8041 are fully compatible.
+
+ICP DAS LP-8841 contains three additional serial ports interfaced via
+Analog Devices ADM213EA chips in addition to 3 serial ports on PXA CPU.
+
+The chips themselves are standard, they would work with 8250_core if
+properly connected. However, they are not connected normally. Al least
+some of their config pins are wired to a different address region. So
+the driver is board-specific.
+
+Required properties:
+- compatible : should be "icpdas,uart-lp8841"
+
+- reg : should provide 16 byte man IO memory region and 1 byte region for
+	termios
+
+- interrupts : should provide interrupt
+
+Optional property:
+- interrupt-parent : should provide a link to interrupt controller either
+		     explicitly or implicitly from a parent node
+
+Examples (from pxa27x-lp8x4x.dts):
+
+		serial@9050 {
+			compatible = "icpdas,uart-lp8841";
+			reg = <0x9050 0x10
+			       0x9030 0x02>;
+			interrupts = <13>;
+			status = "okay";
+		};
+
+		serial@9060 {
+			compatible = "icpdas,uart-lp8841";
+			reg = <0x9060 0x10
+			       0x9032 0x02>;
+			interrupts = <14>;
+			status = "okay";
+		};
diff --git a/drivers/tty/serial/8250/8250_lp8841.c b/drivers/tty/serial/8250/8250_lp8841.c
new file mode 100644
index 0000000..d3a72da
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_lp8841.c
@@ -0,0 +1,156 @@
+/*  linux/drivers/tty/serial/8250/8250_lp8841.c
+ *
+ *  Support for 16550A serial ports on ICP DAS LP-8841
+ *
+ *  Copyright (C) 2013 Sergei Ianovich <ynvich@gmail.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  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 __iomem		*ios_mem;
+};
+
+#define LP8841_DATA_LEN_MASK		0x3
+#define LP8841_DATA_LEN_SHIFT		3
+
+static void lp8841_serial_set_termios(struct uart_port *port,
+		struct ktermios *termios, struct ktermios *old)
+{
+	struct lp8841_serial_data *data = port->private_data;
+	unsigned int old_csize = old ? old->c_cflag & CSIZE : CS8;
+	unsigned int baud;
+	u8 value;
+
+	/* We only support CS7 and CS8 */
+	while ((termios->c_cflag & CSIZE) != CS7 &&
+	       (termios->c_cflag & CSIZE) != CS8) {
+		termios->c_cflag &= ~CSIZE;
+		termios->c_cflag |= old_csize;
+		old_csize = CS8;
+	}
+
+	value =  (termios->c_cflag & CSIZE) == CS7 ? 0 : 1;
+	value += (termios->c_cflag & CSTOPB) ? 1 : 0;
+	value += (termios->c_cflag & PARENB) ? 1 : 0;
+	value += (termios->c_cflag & PARODD) ? 0 : 1;
+#ifdef CMSPAR
+	value += (termios->c_cflag & CMSPAR) ? 1 : 0;
+#endif
+
+	value &=  LP8841_DATA_LEN_MASK;
+	value <<= LP8841_DATA_LEN_SHIFT;
+
+	baud = tty_termios_baud_rate(termios);
+
+	switch (baud) {
+	case 115200:
+		value |= 7;
+		break;
+	case 57600:
+		value |= 6;
+		break;
+	case 38400:
+		value |= 5;
+		break;
+	case 19200:
+		value |= 4;
+		break;
+	case 9600:
+		value |= 3;
+		break;
+	case 4800:
+		value |= 2;
+		break;
+	default:
+		tty_termios_encode_baud_rate(termios, 2400, 2400);
+	case 2400:
+		value |= 1;
+		break;
+	};
+	writeb(value, data->ios_mem);
+
+	serial8250_do_set_termios(port, termios, old);
+}
+
+static const struct of_device_id lp8841_serial_dt_ids[] = {
+	{ .compatible = "icpdas,lp8841-uart", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, lp8841_serial_dt_ids);
+
+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);
+	if (!mmres)
+		return -ENODEV;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	mires = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	data->ios_mem = devm_ioremap_resource(&pdev->dev, mires);
+	if (IS_ERR(data->ios_mem))
+		return PTR_ERR(data->ios_mem);
+
+	memset(&uart, 0, sizeof(uart));
+	uart.port.iotype = UPIO_MEM;
+	uart.port.mapbase = mmres->start;
+	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,
+	},
+};
+
+module_platform_driver(lp8841_serial_driver);
+
+MODULE_AUTHOR("Sergei Ianovich");
+MODULE_DESCRIPTION("8250 serial port module for LP-8841");
+MODULE_LICENSE("GPL");
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 3b5cf9c..68640c1 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -394,3 +394,17 @@ config SERIAL_8250_PXA
 	help
 	  If you have a machine based on an Intel XScale PXA2xx CPU you
 	  can enable its onboard serial ports by enabling this option.
+
+	  If you choose M here, the module name will be 8250_pxa.
+
+config SERIAL_8250_LP8841
+	tristate "Support 16550A ports on ICP DAS LP-8841"
+	depends on SERIAL_8250 && MACH_PXA27X_DT
+	select LP8841_IRQ
+	help
+	  In addition to serial ports on PXA270 SoC, LP-8841 has 1 dual
+	  RS232/RS485 port, 1 RS485 port and 1 RS232 port.
+
+	  Say N here, unless you plan to run this kernel on a LP-8841 system.
+
+	  If you choose M here, the module name will be 8250_lp8841.
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index d1e2f2d..10b4bf0 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_SERIAL_8250_ACCENT)	+= 8250_accent.o
 obj-$(CONFIG_SERIAL_8250_BOCA)		+= 8250_boca.o
 obj-$(CONFIG_SERIAL_8250_EXAR_ST16C554)	+= 8250_exar_st16c554.o
 obj-$(CONFIG_SERIAL_8250_HUB6)		+= 8250_hub6.o
+obj-$(CONFIG_SERIAL_8250_LP8X4X)	+= 8250_lp8x4x.o
 obj-$(CONFIG_SERIAL_8250_FSL)		+= 8250_fsl.o
 obj-$(CONFIG_SERIAL_8250_DW)		+= 8250_dw.o
 obj-$(CONFIG_SERIAL_8250_EM)		+= 8250_em.o
@@ -30,5 +31,6 @@ obj-$(CONFIG_SERIAL_8250_INGENIC)	+= 8250_ingenic.o
 obj-$(CONFIG_SERIAL_8250_MID)		+= 8250_mid.o
 obj-$(CONFIG_SERIAL_OF_PLATFORM)	+= 8250_of.o
 obj-$(CONFIG_SERIAL_8250_PXA)		+= 8250_pxa.o
+obj-$(CONFIG_SERIAL_8250_LP8841)	+= 8250_lp8841.o
 
 CFLAGS_8250_ingenic.o += -I$(srctree)/scripts/dtc/libfdt
-- 
2.7.0

Re: [PATCH v10] serial: support for 16550A serial ports on LP-8x4x

[Rob Herring]

On Wed, Mar 02, 2016 at 12:25:35AM +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.

That's a lot of serial ports...

[...]

> diff --git a/Documentation/devicetree/bindings/serial/icpdas-lp8841-uart.txt b/Documentation/devicetree/bindings/serial/icpdas-lp8841-uart.txt
> new file mode 100644
> index 0000000..d6acd22
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serial/icpdas-lp8841-uart.txt
> @@ -0,0 +1,41 @@
> +* UART ports on ICP DAS LP-8841
> +
> +LP-8441, LP-8141 and LP-8041 are fully compatible.
> +
> +ICP DAS LP-8841 contains three additional serial ports interfaced via
> +Analog Devices ADM213EA chips in addition to 3 serial ports on PXA CPU.
> +
> +The chips themselves are standard, they would work with 8250_core if

Describe in h/w terms how they are different, not what Linux driver 
won't work.

> +properly connected. However, they are not connected normally. Al least

s/Al/At/

> +some of their config pins are wired to a different address region. So
> +the driver is board-specific.
> +
> +Required properties:
> +- compatible : should be "icpdas,uart-lp8841"
> +
> +- reg : should provide 16 byte man IO memory region and 1 byte region for

What is "man IO"?

> +	termios

termios is a Linux term.

> +
> +- interrupts : should provide interrupt

Perhaps you should include other properties standard for 8250 such as 
access size or shift. Possibly if the non-standard bits are already 
configured, the UART could be used for earlycon?

> +
> +Optional property:
> +- interrupt-parent : should provide a link to interrupt controller either
> +		     explicitly or implicitly from a parent node
> +
> +Examples (from pxa27x-lp8x4x.dts):
> +
> +		serial@9050 {
> +			compatible = "icpdas,uart-lp8841";
> +			reg = <0x9050 0x10
> +			       0x9030 0x02>;
> +			interrupts = <13>;
> +			status = "okay";
> +		};
> +
> +		serial@9060 {
> +			compatible = "icpdas,uart-lp8841";
> +			reg = <0x9060 0x10
> +			       0x9032 0x02>;
> +			interrupts = <14>;
> +			status = "okay";
> +		};

[...]

> diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> index 3b5cf9c..68640c1 100644
> --- a/drivers/tty/serial/8250/Kconfig
> +++ b/drivers/tty/serial/8250/Kconfig
> @@ -394,3 +394,17 @@ config SERIAL_8250_PXA
>  	help
>  	  If you have a machine based on an Intel XScale PXA2xx CPU you
>  	  can enable its onboard serial ports by enabling this option.
> +
> +	  If you choose M here, the module name will be 8250_pxa.
> +
> +config SERIAL_8250_LP8841
> +	tristate "Support 16550A ports on ICP DAS LP-8841"
> +	depends on SERIAL_8250 && MACH_PXA27X_DT
> +	select LP8841_IRQ

Generally, drivers don't select their interrupt controller.

> +	help
> +	  In addition to serial ports on PXA270 SoC, LP-8841 has 1 dual
> +	  RS232/RS485 port, 1 RS485 port and 1 RS232 port.
> +
> +	  Say N here, unless you plan to run this kernel on a LP-8841 system.
> +
> +	  If you choose M here, the module name will be 8250_lp8841.

> diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
> index d1e2f2d..10b4bf0 100644
> --- a/drivers/tty/serial/8250/Makefile
> +++ b/drivers/tty/serial/8250/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_SERIAL_8250_ACCENT)	+= 8250_accent.o

>  obj-$(CONFIG_SERIAL_8250_BOCA)		+= 8250_boca.o
>  obj-$(CONFIG_SERIAL_8250_EXAR_ST16C554)	+= 8250_exar_st16c554.o
>  obj-$(CONFIG_SERIAL_8250_HUB6)		+= 8250_hub6.o
> +obj-$(CONFIG_SERIAL_8250_LP8X4X)	+= 8250_lp8x4x.o

This should be dropped.

>  obj-$(CONFIG_SERIAL_8250_FSL)		+= 8250_fsl.o
>  obj-$(CONFIG_SERIAL_8250_DW)		+= 8250_dw.o
>  obj-$(CONFIG_SERIAL_8250_EM)		+= 8250_em.o
> @@ -30,5 +31,6 @@ obj-$(CONFIG_SERIAL_8250_INGENIC)	+= 8250_ingenic.o
>  obj-$(CONFIG_SERIAL_8250_MID)		+= 8250_mid.o
>  obj-$(CONFIG_SERIAL_OF_PLATFORM)	+= 8250_of.o
>  obj-$(CONFIG_SERIAL_8250_PXA)		+= 8250_pxa.o
> +obj-$(CONFIG_SERIAL_8250_LP8841)	+= 8250_lp8841.o

This should be in alphabetical order. OF_PLATFORM is not for legacy 
reasons I think.

>  
>  CFLAGS_8250_ingenic.o += -I$(srctree)/scripts/dtc/libfdt
> -- 
> 2.7.0
>