Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1345040 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v6] serial: support for 16550A serial ports on LP-8x4x Sergei Ianovich <ynvich@gmail.com> - 2016-02-27 17:20 +0100
    Re: [PATCH v6] serial: support for 16550A serial ports on LP-8x4x Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2016-02-29 11:30 +0100
      Re: [PATCH v6] serial: support for 16550A serial ports on LP-8x4x Sergei Ianovich <ynvich@gmail.com> - 2016-02-29 14:10 +0100
        Re: [PATCH v6] serial: support for 16550A serial ports on LP-8x4x One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> - 2016-02-29 15:50 +0100
    [PATCH v7] serial: support for 16550A serial ports on LP-8x4x Sergei Ianovich <ynvich@gmail.com> - 2016-02-29 22:30 +0100
      Re: [PATCH v7] serial: support for 16550A serial ports on LP-8x4x Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2016-03-01 12:10 +0100
        Re: [PATCH v7] serial: support for 16550A serial ports on LP-8x4x Sergei Ianovich <ynvich@gmail.com> - 2016-03-01 17:30 +0100
          Re: [PATCH v7] serial: support for 16550A serial ports on LP-8x4x Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2016-03-01 17:50 +0100
            Re: [PATCH v7] serial: support for 16550A serial ports on LP-8x4x Sergei Ianovich <ynvich@gmail.com> - 2016-03-01 18:20 +0100
              Re: [PATCH v7] serial: support for 16550A serial ports on LP-8x4x Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2016-03-01 18:50 +0100
                Re: [PATCH v7] serial: support for 16550A serial ports on LP-8x4x One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> - 2016-03-01 19:50 +0100
                Re: [PATCH v7] serial: support for 16550A serial ports on LP-8x4x Sergei Ianovich <ynvich@gmail.com> - 2016-03-01 20:30 +0100
                  Re: [PATCH v7] serial: support for 16550A serial ports on LP-8x4x One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> - 2016-03-01 21:00 +0100
      [PATCH v8] serial: support for 16550A serial ports on LP-8x4x Sergei Ianovich <ynvich@gmail.com> - 2016-03-01 21:00 +0100
        [PATCH v9] serial: support for 16550A serial ports on LP-8x4x Sergei Ianovich <ynvich@gmail.com> - 2016-03-01 21:10 +0100
          Re: [PATCH v9] serial: support for 16550A serial ports on LP-8x4x Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-03-01 21:30 +0100
          [PATCH v10] serial: support for 16550A serial ports on LP-8x4x Sergei Ianovich <ynvich@gmail.com> - 2016-03-01 22:30 +0100

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

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

[toc] | [next] | [standalone]

#1345627

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@linux.intel.com>

> +++ 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@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			*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@linux.intel.com>
Intel Finland Oy

[toc] | [prev] | [next] | [standalone]

#1345785

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.

[toc] | [prev] | [next] | [standalone]

#1345872

> 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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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@gmail.com>
> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

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@linux.intel.com>
Intel Finland Oy

[toc] | [prev] | [next] | [standalone]

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

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@gmail.com>
> > Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
> 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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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@linux.intel.com>
Intel Finland Oy

[toc] | [prev] | [next] | [standalone]

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

> 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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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>

   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@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)
+{
+	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

[toc] | [prev] | [next] | [standalone]

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

On Tue, Mar 1, 2016 at 10:08 PM, Sergei Ianovich <ynvich@gmail.com> 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@gmail.com>
> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

Sorry, but still few nitpicks and then
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web