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

Groups > linux.kernel > #1570166 > unrolled thread

[PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

Back to article view | Back to linux.kernel

Contents

  [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2017-01-30 23:40 +0100
    Re: [PATCH v13 1/2] serial: exar: split out the exar code from  8250_pci Jan Kiszka <jan.kiszka@siemens.com> - 2017-02-03 15:10 +0100
      Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-02-03 15:10 +0100
      Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2017-02-03 22:40 +0100
        Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-02-04 16:00 +0100
        Re: [PATCH v13 1/2] serial: exar: split out the exar code from  8250_pci Jan Kiszka <jan.kiszka@siemens.com> - 2017-02-06 14:50 +0100
          Re: [PATCH v13 1/2] serial: exar: split out the exar code from  8250_pci Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-02-06 15:10 +0100
            Re: [PATCH v13 1/2] serial: exar: split out the exar code from  8250_pci Jan Kiszka <jan.kiszka@siemens.com> - 2017-02-06 15:30 +0100
            Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2017-02-06 23:10 +0100
          Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2017-02-06 23:20 +0100
    Re: [PATCH v13 1/2] serial: exar: split out the exar code from  8250_pci Jan Kiszka <jan.kiszka@siemens.com> - 2017-02-06 15:50 +0100
      Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2017-02-06 23:10 +0100
        Re: [PATCH v13 1/2] serial: exar: split out the exar code from  8250_pci Jan Kiszka <jan.kiszka@siemens.com> - 2017-02-07 11:10 +0100
    Re: [PATCH v13 1/2] serial: exar: split out the exar code from  8250_pci Jan Kiszka <jan.kiszka@siemens.com> - 2017-02-06 20:40 +0100
      Re: [PATCH v13 1/2] serial: exar: split out the exar code from  8250_pci Jan Kiszka <jan.kiszka@siemens.com> - 2017-02-06 20:50 +0100

#1570166 — [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

From: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>

Add the serial driver for the Exar chips. And also register the
platform device for the GPIO provided by the Exar chips.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>
---

Andy,
I have added the if (!board) check, but I am not sure how board can be
NULL here. If probe executes that will mean there was a match of the
device id and so in that case board can not be NULL.

 drivers/tty/serial/8250/8250_exar.c | 396 ++++++++++++++++++++++++++++++++++++
 drivers/tty/serial/8250/Kconfig     |   4 +
 drivers/tty/serial/8250/Makefile    |   1 +
 3 files changed, 401 insertions(+)
 create mode 100644 drivers/tty/serial/8250/8250_exar.c

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
new file mode 100644
index 0000000..ba1f359
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -0,0 +1,396 @@
+/*
+ *  Probe module for 8250/16550-type Exar chips PCI serial ports.
+ *
+ *  Based on drivers/tty/serial/8250/8250_pci.c,
+ *
+ *  Copyright (C) 2017 Sudip Mukherjee, All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License.
+ */
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/serial_core.h>
+#include <linux/serial_reg.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/tty.h>
+#include <linux/8250_pci.h>
+
+#include <asm/byteorder.h>
+
+#include "8250.h"
+
+#define PCI_DEVICE_ID_COMMTECH_4224PCIE	0x0020
+#define PCI_DEVICE_ID_COMMTECH_4228PCIE	0x0021
+#define PCI_DEVICE_ID_COMMTECH_4222PCIE	0x0022
+#define PCI_DEVICE_ID_EXAR_XR17V4358	0x4358
+#define PCI_DEVICE_ID_EXAR_XR17V8358	0x8358
+
+#define UART_EXAR_MPIOINT_7_0	0x8f	/* MPIOINT[7:0] */
+#define UART_EXAR_MPIOLVL_7_0	0x90	/* MPIOLVL[7:0] */
+#define UART_EXAR_MPIO3T_7_0	0x91	/* MPIO3T[7:0] */
+#define UART_EXAR_MPIOINV_7_0	0x92	/* MPIOINV[7:0] */
+#define UART_EXAR_MPIOSEL_7_0	0x93	/* MPIOSEL[7:0] */
+#define UART_EXAR_MPIOOD_7_0	0x94	/* MPIOOD[7:0] */
+#define UART_EXAR_MPIOINT_15_8	0x95	/* MPIOINT[15:8] */
+#define UART_EXAR_MPIOLVL_15_8	0x96	/* MPIOLVL[15:8] */
+#define UART_EXAR_MPIO3T_15_8	0x97	/* MPIO3T[15:8] */
+#define UART_EXAR_MPIOINV_15_8	0x98	/* MPIOINV[15:8] */
+#define UART_EXAR_MPIOSEL_15_8	0x99	/* MPIOSEL[15:8] */
+#define UART_EXAR_MPIOOD_15_8	0x9a	/* MPIOOD[15:8] */
+
+struct exar8250;
+
+/**
+ * struct exar8250_board - board information
+ * @num_ports: number of serial ports
+ * @reg_shift: describes UART register mapping in PCI memory
+ */
+struct exar8250_board {
+	unsigned int num_ports;
+	unsigned int reg_shift;
+	bool has_slave;
+	int	(*setup)(struct exar8250 *, struct pci_dev *,
+			 struct uart_8250_port *, int);
+	void	(*exit)(struct pci_dev *pcidev);
+};
+
+struct exar8250 {
+	unsigned int		nr;
+	struct exar8250_board	*board;
+	int			line[0];
+};
+
+static int default_setup(struct exar8250 *priv, struct pci_dev *pcidev,
+			 int idx, unsigned int offset,
+			 struct uart_8250_port *port)
+{
+	const struct exar8250_board *board = priv->board;
+	unsigned int bar = 0;
+
+	port->port.iotype = UPIO_MEM;
+	port->port.mapbase = pci_resource_start(pcidev, bar) + offset;
+	port->port.membase = pcim_iomap_table(pcidev)[bar] + offset;
+	port->port.regshift = board->reg_shift;
+
+	return 0;
+}
+
+static int
+pci_connect_tech_setup(struct exar8250 *priv, struct pci_dev *pcidev,
+		       struct uart_8250_port *port, int idx)
+{
+	unsigned int offset = idx * 0x200;
+	unsigned int baud = 1843200;
+
+	port->port.uartclk = baud * 16;
+	return default_setup(priv, pcidev, idx, offset, port);
+}
+
+static int
+pci_xr17c154_setup(struct exar8250 *priv, struct pci_dev *pcidev,
+		   struct uart_8250_port *port, int idx)
+{
+	unsigned int offset = idx * 0x200;
+	unsigned int baud = 921600;
+
+	port->port.uartclk = baud * 16;
+	return default_setup(priv, pcidev, idx, offset, port);
+}
+
+static void setup_gpio(u8 __iomem *p)
+{
+	writeb(0x00, p + UART_EXAR_MPIOINT_7_0);
+	writeb(0x00, p + UART_EXAR_MPIOLVL_7_0);
+	writeb(0x00, p + UART_EXAR_MPIO3T_7_0);
+	writeb(0x00, p + UART_EXAR_MPIOINV_7_0);
+	writeb(0x00, p + UART_EXAR_MPIOSEL_7_0);
+	writeb(0x00, p + UART_EXAR_MPIOOD_7_0);
+	writeb(0x00, p + UART_EXAR_MPIOINT_15_8);
+	writeb(0x00, p + UART_EXAR_MPIOLVL_15_8);
+	writeb(0x00, p + UART_EXAR_MPIO3T_15_8);
+	writeb(0x00, p + UART_EXAR_MPIOINV_15_8);
+	writeb(0x00, p + UART_EXAR_MPIOSEL_15_8);
+	writeb(0x00, p + UART_EXAR_MPIOOD_15_8);
+}
+
+static void *
+xr17v35x_register_gpio(struct pci_dev *pcidev)
+{
+	struct platform_device *pdev;
+
+	pdev = platform_device_alloc("gpio_exar", PLATFORM_DEVID_AUTO);
+	if (!pdev)
+		return NULL;
+
+	platform_set_drvdata(pdev, pcidev);
+	if (platform_device_add(pdev) < 0) {
+		platform_device_put(pdev);
+		return NULL;
+	}
+
+	return pdev;
+}
+
+static int
+pci_xr17v35x_setup(struct exar8250 *priv, struct pci_dev *pcidev,
+		   struct uart_8250_port *port, int idx)
+{
+	const struct exar8250_board *board = priv->board;
+	unsigned int offset = idx * 0x400;
+	unsigned int baud = 7812500;
+	u8 __iomem *p;
+	int ret;
+
+	port->port.uartclk = baud * 16;
+	/*
+	 * Setup the uart clock for the devices on expansion slot to
+	 * half the clock speed of the main chip (which is 125MHz)
+	 */
+	if (board->has_slave && idx >= 8)
+		port->port.uartclk /= 2;
+
+	p = pci_ioremap_bar(pcidev, 0);
+	if (!p)
+		return -ENOMEM;
+
+	/* Setup Multipurpose Input/Output pins. */
+	if (idx == 0)
+		setup_gpio(p);
+
+	writeb(0x00, p + UART_EXAR_8XMODE);
+	writeb(UART_FCTR_EXAR_TRGD, p + UART_EXAR_FCTR);
+	writeb(128, p + UART_EXAR_TXTRG);
+	writeb(128, p + UART_EXAR_RXTRG);
+	iounmap(p);
+
+	ret = default_setup(priv, pcidev, idx, offset, port);
+	if (ret)
+		return ret;
+
+	if (idx == 0)
+		port->port.private_data =
+			xr17v35x_register_gpio(pcidev);
+
+	return 0;
+}
+
+static void pci_xr17v35x_exit(struct pci_dev *pcidev)
+{
+	struct exar8250 *priv = pci_get_drvdata(pcidev);
+	struct uart_8250_port *port = serial8250_get_port(priv->line[0]);
+	struct platform_device *pdev = port->port.private_data;
+
+	platform_device_unregister(pdev);
+	port->port.private_data = NULL;
+}
+
+static int
+exar_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *ent)
+{
+	unsigned int nr_ports, i, bar = 0, maxnr;
+	struct exar8250_board *board;
+	struct uart_8250_port uart;
+	struct exar8250 *priv;
+	int rc;
+
+	board = (struct exar8250_board *)ent->driver_data;
+	if (!board)
+		return -EINVAL;
+
+	rc = pcim_enable_device(pcidev);
+	if (rc)
+		return rc;
+
+	maxnr = pci_resource_len(pcidev, bar) >> (board->reg_shift + 3);
+
+	nr_ports = board->num_ports ? board->num_ports : pcidev->device & 0x0f;
+
+	priv = devm_kzalloc(&pcidev->dev, sizeof(*priv) +
+			    sizeof(unsigned int) * nr_ports,
+			    GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->board = board;
+
+	memset(&uart, 0, sizeof(uart));
+	uart.port.flags = UPF_SKIP_TEST | UPF_BOOT_AUTOCONF | UPF_SHARE_IRQ
+			  | UPF_EXAR_EFR;
+	uart.port.irq = pcidev->irq;
+	uart.port.dev = &pcidev->dev;
+
+	for (i = 0; i < nr_ports && i < maxnr; i++) {
+		rc = board->setup(priv, pcidev, &uart, i);
+		if (rc) {
+			dev_err(&pcidev->dev, "Failed to setup port %u\n", i);
+			break;
+		}
+
+		dev_dbg(&pcidev->dev, "Setup PCI port: port %lx, irq %d, type %d\n",
+			uart.port.iobase, uart.port.irq, uart.port.iotype);
+
+		priv->line[i] = serial8250_register_8250_port(&uart);
+		if (priv->line[i] < 0) {
+			dev_err(&pcidev->dev,
+				"Couldn't register serial port %lx, irq %d, type %d, error %d\n",
+				uart.port.iobase, uart.port.irq,
+				uart.port.iotype, priv->line[i]);
+			break;
+		}
+	}
+	priv->nr = i;
+	pci_set_drvdata(pcidev, priv);
+	return 0;
+}
+
+static void exar_pci_remove(struct pci_dev *pcidev)
+{
+	struct exar8250 *priv = pci_get_drvdata(pcidev);
+	unsigned int i;
+
+	for (i = 0; i < priv->nr; i++)
+		serial8250_unregister_port(priv->line[i]);
+
+	if (priv->board->exit)
+		priv->board->exit(pcidev);
+}
+
+static int __maybe_unused exar_suspend(struct device *dev)
+{
+	struct pci_dev *pcidev = to_pci_dev(dev);
+	struct exar8250 *priv = pci_get_drvdata(pcidev);
+	unsigned int i;
+
+	for (i = 0; i < priv->nr; i++)
+		if (priv->line[i] >= 0)
+			serial8250_suspend_port(priv->line[i]);
+
+	/* Ensure that every init quirk is properly torn down */
+	if (priv->board->exit)
+		priv->board->exit(pcidev);
+
+	return 0;
+}
+
+static int __maybe_unused exar_resume(struct device *dev)
+{
+	struct pci_dev *pcidev = to_pci_dev(dev);
+	struct exar8250 *priv = pci_get_drvdata(pcidev);
+	unsigned int i;
+
+	for (i = 0; i < priv->nr; i++)
+		if (priv->line[i] >= 0)
+			serial8250_resume_port(priv->line[i]);
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(exar_pci_pm, exar_suspend, exar_resume);
+
+static const struct exar8250_board pbn_connect = {
+	.setup		= pci_connect_tech_setup,
+};
+
+static const struct exar8250_board pbn_exar_ibm_saturn = {
+	.num_ports	= 1,
+	.setup		= pci_xr17c154_setup,
+};
+
+static const struct exar8250_board pbn_exar_XR17C15x = {
+	.setup		= pci_xr17c154_setup,
+};
+
+static const struct exar8250_board pbn_exar_XR17V35x = {
+	.setup		= pci_xr17v35x_setup,
+	.exit		= pci_xr17v35x_exit,
+};
+
+static const struct exar8250_board pbn_exar_XR17V4358 = {
+	.num_ports	= 12,
+	.has_slave	= true,
+	.setup		= pci_xr17v35x_setup,
+	.exit		= pci_xr17v35x_exit,
+};
+
+static const struct exar8250_board pbn_exar_XR17V8358 = {
+	.num_ports	= 16,
+	.has_slave	= true,
+	.setup		= pci_xr17v35x_setup,
+	.exit		= pci_xr17v35x_exit,
+};
+
+#define CONNECT_DEVICE(devid, sdevid, bd) {				\
+	PCI_DEVICE_SUB(							\
+		PCI_VENDOR_ID_EXAR,					\
+		PCI_DEVICE_ID_EXAR_##devid,				\
+		PCI_SUBVENDOR_ID_CONNECT_TECH,				\
+		PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_##sdevid), 0, 0,	\
+		(kernel_ulong_t)&bd					\
+	}
+
+#define EXAR_DEVICE(vend, devid, bd) {					\
+	PCI_VDEVICE(vend, PCI_DEVICE_ID_##devid), (kernel_ulong_t)&bd	\
+	}
+
+#define IBM_DEVICE(devid, sdevid, bd) {			\
+	PCI_DEVICE_SUB(					\
+		PCI_VENDOR_ID_EXAR,			\
+		PCI_DEVICE_ID_EXAR_##devid,		\
+		PCI_VENDOR_ID_IBM,			\
+		PCI_SUBDEVICE_ID_IBM_##sdevid), 0, 0,	\
+		(kernel_ulong_t)&bd			\
+	}
+
+static struct pci_device_id exar_pci_tbl[] = {
+	CONNECT_DEVICE(XR17C152, UART_2_232, pbn_connect),
+	CONNECT_DEVICE(XR17C154, UART_4_232, pbn_connect),
+	CONNECT_DEVICE(XR17C158, UART_8_232, pbn_connect),
+	CONNECT_DEVICE(XR17C152, UART_1_1, pbn_connect),
+	CONNECT_DEVICE(XR17C154, UART_2_2, pbn_connect),
+	CONNECT_DEVICE(XR17C158, UART_4_4, pbn_connect),
+	CONNECT_DEVICE(XR17C152, UART_2, pbn_connect),
+	CONNECT_DEVICE(XR17C154, UART_4, pbn_connect),
+	CONNECT_DEVICE(XR17C158, UART_8, pbn_connect),
+	CONNECT_DEVICE(XR17C152, UART_2_485, pbn_connect),
+	CONNECT_DEVICE(XR17C154, UART_4_485, pbn_connect),
+	CONNECT_DEVICE(XR17C158, UART_8_485, pbn_connect),
+
+	IBM_DEVICE(XR17C152, SATURN_SERIAL_ONE_PORT, pbn_exar_ibm_saturn),
+
+	/* Exar Corp. XR17C15[248] Dual/Quad/Octal UART */
+	EXAR_DEVICE(EXAR, EXAR_XR17C152, pbn_exar_XR17C15x),
+	EXAR_DEVICE(EXAR, EXAR_XR17C154, pbn_exar_XR17C15x),
+	EXAR_DEVICE(EXAR, EXAR_XR17C158, pbn_exar_XR17C15x),
+
+	/* Exar Corp. XR17V[48]35[248] Dual/Quad/Octal/Hexa PCIe UARTs */
+	EXAR_DEVICE(EXAR, EXAR_XR17V352, pbn_exar_XR17V35x),
+	EXAR_DEVICE(EXAR, EXAR_XR17V354, pbn_exar_XR17V35x),
+	EXAR_DEVICE(EXAR, EXAR_XR17V358, pbn_exar_XR17V35x),
+	EXAR_DEVICE(EXAR, EXAR_XR17V4358, pbn_exar_XR17V4358),
+	EXAR_DEVICE(EXAR, EXAR_XR17V8358, pbn_exar_XR17V8358),
+	EXAR_DEVICE(COMMTECH, COMMTECH_4222PCIE, pbn_exar_XR17V35x),
+	EXAR_DEVICE(COMMTECH, COMMTECH_4224PCIE, pbn_exar_XR17V35x),
+	EXAR_DEVICE(COMMTECH, COMMTECH_4228PCIE, pbn_exar_XR17V35x),
+	{ 0, }
+};
+MODULE_DEVICE_TABLE(pci, exar_pci_tbl);
+
+static struct pci_driver exar_pci_driver = {
+	.name		= "exar_serial",
+	.probe		= exar_pci_probe,
+	.remove		= exar_pci_remove,
+	.driver         = {
+		.pm     = &exar_pci_pm,
+	},
+	.id_table	= exar_pci_tbl,
+};
+module_pci_driver(exar_pci_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Exar Serial Dricer");
+MODULE_AUTHOR("Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>");
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index c0bf996..2573ded 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -127,6 +127,10 @@ config SERIAL_8250_PCI
 	  Note that serial ports on NetMos 9835 Multi-I/O cards are handled
 	  by the parport_serial driver, enabled with CONFIG_PARPORT_SERIAL.
 
+config SERIAL_8250_EXAR
+        tristate "8250/16550 PCI device support"
+        depends on SERIAL_8250_PCI
+
 config SERIAL_8250_HP300
 	tristate
 	depends on SERIAL_8250 && HP300
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index 850e721..2f30f9e 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_SERIAL_8250)		+= 8250.o 8250_base.o
 8250_base-$(CONFIG_SERIAL_8250_FINTEK)	+= 8250_fintek.o
 obj-$(CONFIG_SERIAL_8250_GSC)		+= 8250_gsc.o
 obj-$(CONFIG_SERIAL_8250_PCI)		+= 8250_pci.o
+obj-$(CONFIG_SERIAL_8250_EXAR)		+= 8250_exar.o
 obj-$(CONFIG_SERIAL_8250_HP300)		+= 8250_hp300.o
 obj-$(CONFIG_SERIAL_8250_CS)		+= serial_cs.o
 obj-$(CONFIG_SERIAL_8250_ACORN)		+= 8250_acorn.o
-- 
1.9.1

[toc] | [next] | [standalone]

#1573110 — Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

On 2017-01-30 23:28, Sudip Mukherjee wrote:
> From: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>
> 
> Add the serial driver for the Exar chips. And also register the
> platform device for the GPIO provided by the Exar chips.

"Also" means you are doing two things in one patch - was this already
discussed and accepted in previous review rounds? If so, ignore my
comment, but I would have asked for two patches, one that just
translates the existing code and another that adds this new feature.

> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>
> ---
> 
> Andy,
> I have added the if (!board) check, but I am not sure how board can be
> NULL here. If probe executes that will mean there was a match of the
> device id and so in that case board can not be NULL.
> 
>  drivers/tty/serial/8250/8250_exar.c | 396 ++++++++++++++++++++++++++++++++++++
>  drivers/tty/serial/8250/Kconfig     |   4 +
>  drivers/tty/serial/8250/Makefile    |   1 +
>  3 files changed, 401 insertions(+)
>  create mode 100644 drivers/tty/serial/8250/8250_exar.c
> 
> diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> new file mode 100644
> index 0000000..ba1f359
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_exar.c
> @@ -0,0 +1,396 @@
> +/*
> + *  Probe module for 8250/16550-type Exar chips PCI serial ports.
> + *
> + *  Based on drivers/tty/serial/8250/8250_pci.c,
> + *
> + *  Copyright (C) 2017 Sudip Mukherjee, All Rights Reserved.

It's legally cleaner to carry over the copyright notice from the
original file, unless you rewrote everything (unlikely on first glance).
You may still add yours to the list for the significant contributions.

BTW, are you personally the copyright holder or your employer Codethink?
Depends on your contractual situation, but the former is less common.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

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

#1573112

On Fri, Feb 3, 2017 at 4:02 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2017-01-30 23:28, Sudip Mukherjee wrote:
>> From: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>
>>
>> Add the serial driver for the Exar chips. And also register the
>> platform device for the GPIO provided by the Exar chips.
>
> "Also" means you are doing two things in one patch - was this already
> discussed and accepted in previous review rounds? If so, ignore my
> comment, but I would have asked for two patches, one that just
> translates the existing code and another that adds this new feature.

Since it's already in Greg's tty-next, no point to fix anymore this
particular part.
However, you are right that few lines of code might be split to a
separate change.

>> +/*
>> + *  Probe module for 8250/16550-type Exar chips PCI serial ports.
>> + *
>> + *  Based on drivers/tty/serial/8250/8250_pci.c,
>> + *
>> + *  Copyright (C) 2017 Sudip Mukherjee, All Rights Reserved.
>
> It's legally cleaner to carry over the copyright notice from the
> original file, unless you rewrote everything (unlikely on first glance).
> You may still add yours to the list for the significant contributions.
>
> BTW, are you personally the copyright holder or your employer Codethink?
> Depends on your contractual situation, but the former is less common.

This is good comment and I think it needs to be addressed (as a
separate change due to above).

-- 
With Best Regards,
Andy Shevchenko

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

#1573460

On Friday 03 February 2017 02:02 PM, Jan Kiszka wrote:
> On 2017-01-30 23:28, Sudip Mukherjee wrote:
>> From: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>
>>
>> Add the serial driver for the Exar chips. And also register the
>> platform device for the GPIO provided by the Exar chips.
>
> "Also" means you are doing two things in one patch - was this already
> discussed and accepted in previous review rounds? If so, ignore my
> comment, but I would have asked for two patches, one that just
> translates the existing code and another that adds this new feature.
>

Like Andy replied, this is already in tty-next.

>>
>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>> Signed-off-by: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>
>> ---
>>
>> Andy,
>> I have added the if (!board) check, but I am not sure how board can be
>> NULL here. If probe executes that will mean there was a match of the
>> device id and so in that case board can not be NULL.
>>
>>   drivers/tty/serial/8250/8250_exar.c | 396 ++++++++++++++++++++++++++++++++++++
>>   drivers/tty/serial/8250/Kconfig     |   4 +
>>   drivers/tty/serial/8250/Makefile    |   1 +
>>   3 files changed, 401 insertions(+)
>>   create mode 100644 drivers/tty/serial/8250/8250_exar.c
>>
>> diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
>> new file mode 100644
>> index 0000000..ba1f359
>> --- /dev/null
>> +++ b/drivers/tty/serial/8250/8250_exar.c
>> @@ -0,0 +1,396 @@
>> +/*
>> + *  Probe module for 8250/16550-type Exar chips PCI serial ports.
>> + *
>> + *  Based on drivers/tty/serial/8250/8250_pci.c,
>> + *
>> + *  Copyright (C) 2017 Sudip Mukherjee, All Rights Reserved.
>
> It's legally cleaner to carry over the copyright notice from the
> original file, unless you rewrote everything (unlikely on first glance).
> You may still add yours to the list for the significant contributions.

Should i send a separate patch to modify those? Andy?

>
> BTW, are you personally the copyright holder or your employer Codethink?
> Depends on your contractual situation, but the former is less common.

Well, Codethink has nothing to do with this patch. This was a voluntary 
work started before I joined Codethink, but then I joined Codethink and 
found very little time to finish this. So finally now its done.

https://lists.kernelnewbies.org/pipermail/kernelnewbies/2015-November/015372.html


Regards
Sudip

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

#1573677

On Fri, Feb 3, 2017 at 11:31 PM, Sudip Mukherjee
<sudipm.mukherjee@gmail.com> wrote:
> On Friday 03 February 2017 02:02 PM, Jan Kiszka wrote:
>> On 2017-01-30 23:28, Sudip Mukherjee wrote:


>>> @@ -0,0 +1,396 @@
>>> +/*
>>> + *  Probe module for 8250/16550-type Exar chips PCI serial ports.
>>> + *
>>> + *  Based on drivers/tty/serial/8250/8250_pci.c,
>>> + *
>>> + *  Copyright (C) 2017 Sudip Mukherjee, All Rights Reserved.
>>
>>
>> It's legally cleaner to carry over the copyright notice from the
>> original file, unless you rewrote everything (unlikely on first glance).
>> You may still add yours to the list for the significant contributions.
>
> Should i send a separate patch to modify those? Andy?

Please, do.

-- 
With Best Regards,
Andy Shevchenko

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

#1574732 — Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

On 2017-02-03 22:31, Sudip Mukherjee wrote:
> On Friday 03 February 2017 02:02 PM, Jan Kiszka wrote:
>> BTW, are you personally the copyright holder or your employer Codethink?
>> Depends on your contractual situation, but the former is less common.
> 
> Well, Codethink has nothing to do with this patch. This was a voluntary
> work started before I joined Codethink, but then I joined Codethink and
> found very little time to finish this. So finally now its done.
> 
> https://lists.kernelnewbies.org/pipermail/kernelnewbies/2015-November/015372.html
> 

Hmm, why using your corporate email address then? This suggests a
different copyright situation.

Funnily, I just received this question internally: How can you tell
apart if someone sends a personal contribution via his/her employer
account from someone contributing on behalf of a company, thus with that
company holding the rights? I argued that no one would do the former to
prevent wrong accounting, but you just proved a counterexample. :)

Regards,
Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

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

#1574800 — Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

On Mon, Feb 06, 2017 at 02:49:07PM +0100, Jan Kiszka wrote:
> On 2017-02-03 22:31, Sudip Mukherjee wrote:
> > On Friday 03 February 2017 02:02 PM, Jan Kiszka wrote:
> >> BTW, are you personally the copyright holder or your employer Codethink?
> >> Depends on your contractual situation, but the former is less common.
> > 
> > Well, Codethink has nothing to do with this patch. This was a voluntary
> > work started before I joined Codethink, but then I joined Codethink and
> > found very little time to finish this. So finally now its done.
> > 
> > https://lists.kernelnewbies.org/pipermail/kernelnewbies/2015-November/015372.html
> > 
> 
> Hmm, why using your corporate email address then? This suggests a
> different copyright situation.
> 
> Funnily, I just received this question internally: How can you tell
> apart if someone sends a personal contribution via his/her employer
> account from someone contributing on behalf of a company, thus with that
> company holding the rights? I argued that no one would do the former to
> prevent wrong accounting, but you just proved a counterexample. :)

There are numerous companies that do this, some create whole shell
orginizations in order to "hide" their kernel contributions for various
"interesting" reasons.

Fun stuff.  I suggest having your internal people talk to your lawyers,
they should know all about this (and if not, have those lawyers talk to
the LF lawyers...)

But that's not the issue here, we know Sudip :)

thanks,

greg k-h

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

#1574830 — Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

On 2017-02-06 15:06, Greg Kroah-Hartman wrote:
> On Mon, Feb 06, 2017 at 02:49:07PM +0100, Jan Kiszka wrote:
>> On 2017-02-03 22:31, Sudip Mukherjee wrote:
>>> On Friday 03 February 2017 02:02 PM, Jan Kiszka wrote:
>>>> BTW, are you personally the copyright holder or your employer Codethink?
>>>> Depends on your contractual situation, but the former is less common.
>>>
>>> Well, Codethink has nothing to do with this patch. This was a voluntary
>>> work started before I joined Codethink, but then I joined Codethink and
>>> found very little time to finish this. So finally now its done.
>>>
>>> https://lists.kernelnewbies.org/pipermail/kernelnewbies/2015-November/015372.html
>>>
>>
>> Hmm, why using your corporate email address then? This suggests a
>> different copyright situation.
>>
>> Funnily, I just received this question internally: How can you tell
>> apart if someone sends a personal contribution via his/her employer
>> account from someone contributing on behalf of a company, thus with that
>> company holding the rights? I argued that no one would do the former to
>> prevent wrong accounting, but you just proved a counterexample. :)
> 
> There are numerous companies that do this, some create whole shell
> orginizations in order to "hide" their kernel contributions for various
> "interesting" reasons.

I was not talking about companies but individuals: If they use their
company address for something written in their spare time (and their
contract allow to keep ownership of that), they needless suggest their
company holds the copyright that way around. If they stick with a
private address, it remains more clearly in their hand.

What you mentioned is a different story and can indeed be interesting
for the companies when they realize they would like for prove their code
ownership to some legal authority for whatever reason.

Anyway, off-topic now.

Jan

> 
> Fun stuff.  I suggest having your internal people talk to your lawyers,
> they should know all about this (and if not, have those lawyers talk to
> the LF lawyers...)
> 
> But that's not the issue here, we know Sudip :)
> 
> thanks,
> 
> greg k-h
> 

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

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

#1575198

On Monday 06 February 2017 02:06 PM, Greg Kroah-Hartman wrote:
> On Mon, Feb 06, 2017 at 02:49:07PM +0100, Jan Kiszka wrote:
>> On 2017-02-03 22:31, Sudip Mukherjee wrote:
>>> On Friday 03 February 2017 02:02 PM, Jan Kiszka wrote:
>>>> BTW, are you personally the copyright holder or your employer Codethink?
>>>> Depends on your contractual situation, but the former is less common.
>>>
>>> Well, Codethink has nothing to do with this patch. This was a voluntary
>>> work started before I joined Codethink, but then I joined Codethink and
>>> found very little time to finish this. So finally now its done.
>>>
>>> https://lists.kernelnewbies.org/pipermail/kernelnewbies/2015-November/015372.html
>>>
>>
>> Hmm, why using your corporate email address then? This suggests a
>> different copyright situation.
>>
>> Funnily, I just received this question internally: How can you tell
>> apart if someone sends a personal contribution via his/her employer
>> account from someone contributing on behalf of a company, thus with that
>> company holding the rights? I argued that no one would do the former to
>> prevent wrong accounting, but you just proved a counterexample. :)
>
> There are numerous companies that do this, some create whole shell
> orginizations in order to "hide" their kernel contributions for various
> "interesting" reasons.
>
> Fun stuff.  I suggest having your internal people talk to your lawyers,
> they should know all about this (and if not, have those lawyers talk to
> the LF lawyers...)
>
> But that's not the issue here, we know Sudip :)

:)

Regards
Sudip

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

#1575201

On Monday 06 February 2017 01:49 PM, Jan Kiszka wrote:
> On 2017-02-03 22:31, Sudip Mukherjee wrote:
>> On Friday 03 February 2017 02:02 PM, Jan Kiszka wrote:
>>> BTW, are you personally the copyright holder or your employer Codethink?
>>> Depends on your contractual situation, but the former is less common.
>>
>> Well, Codethink has nothing to do with this patch. This was a voluntary
>> work started before I joined Codethink, but then I joined Codethink and
>> found very little time to finish this. So finally now its done.
>>
>> https://lists.kernelnewbies.org/pipermail/kernelnewbies/2015-November/015372.html
>>
>
> Hmm, why using your corporate email address then? This suggests a
> different copyright situation.
>
> Funnily, I just received this question internally: How can you tell
> apart if someone sends a personal contribution via his/her employer
> account from someone contributing on behalf of a company, thus with that
> company holding the rights? I argued that no one would do the former to
> prevent wrong accounting, but you just proved a counterexample. :)

well, I have been doing it this way from the very first day I started 
contributing.


Regards
Sudip

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

#1574856 — Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

On 2017-01-30 23:28, Sudip Mukherjee wrote:
> From: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>
> 
> Add the serial driver for the Exar chips. And also register the
> platform device for the GPIO provided by the Exar chips.
> 

And another question: you left pci_fastcom335_setup and related things
untouched - did that code come later, or is it left in 8250_pci.c for a
reason?

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

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

#1575195

On Monday 06 February 2017 02:45 PM, Jan Kiszka wrote:
> On 2017-01-30 23:28, Sudip Mukherjee wrote:
>> From: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>
>>
>> Add the serial driver for the Exar chips. And also register the
>> platform device for the GPIO provided by the Exar chips.
>>
>
> And another question: you left pci_fastcom335_setup and related things
> untouched - did that code come later, or is it left in 8250_pci.c for a
> reason?

That was discussed.
Those are separate chips from different vendor and this patchset was 
specifically for Exar chips. So i suggested I will do it via separate patch.

Regards
Sudip

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

#1575542 — Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

On 2017-02-06 23:04, Sudip Mukherjee wrote:
> On Monday 06 February 2017 02:45 PM, Jan Kiszka wrote:
>> On 2017-01-30 23:28, Sudip Mukherjee wrote:
>>> From: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>
>>>
>>> Add the serial driver for the Exar chips. And also register the
>>> platform device for the GPIO provided by the Exar chips.
>>>
>>
>> And another question: you left pci_fastcom335_setup and related things
>> untouched - did that code come later, or is it left in 8250_pci.c for a
>> reason?
> 
> That was discussed.
> Those are separate chips from different vendor and this patchset was
> specifically for Exar chips. So i suggested I will do it via separate
> patch.

If they are from different vendors, why are they addressing Exar
registers? Seems more like they are just rebranded and should indeed be
moved as well.

Jan

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

#1575094 — Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

On 2017-01-30 23:28, Sudip Mukherjee wrote:
> From: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>
> 
> Add the serial driver for the Exar chips. And also register the
> platform device for the GPIO provided by the Exar chips.
> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>

...

> +static int default_setup(struct exar8250 *priv, struct pci_dev *pcidev,
> +			 int idx, unsigned int offset,
> +			 struct uart_8250_port *port)
> +{
> +	const struct exar8250_board *board = priv->board;
> +	unsigned int bar = 0;
> +
> +	port->port.iotype = UPIO_MEM;
> +	port->port.mapbase = pci_resource_start(pcidev, bar) + offset;
> +	port->port.membase = pcim_iomap_table(pcidev)[bar] + offset;

This always gives you 0 for membase because you missed to call
pcim_iomap for bar 0.

Sorry to pick on this piece-wise, but I just ran into this bug now while
porting patches over.

> +	port->port.regshift = board->reg_shift;
> +
> +	return 0;
> +}
> +
> +static int
> +pci_connect_tech_setup(struct exar8250 *priv, struct pci_dev *pcidev,
> +		       struct uart_8250_port *port, int idx)
> +{
> +	unsigned int offset = idx * 0x200;
> +	unsigned int baud = 1843200;
> +
> +	port->port.uartclk = baud * 16;
> +	return default_setup(priv, pcidev, idx, offset, port);
> +}
> +
> +static int
> +pci_xr17c154_setup(struct exar8250 *priv, struct pci_dev *pcidev,
> +		   struct uart_8250_port *port, int idx)
> +{
> +	unsigned int offset = idx * 0x200;
> +	unsigned int baud = 921600;
> +
> +	port->port.uartclk = baud * 16;
> +	return default_setup(priv, pcidev, idx, offset, port);
> +}
> +
> +static void setup_gpio(u8 __iomem *p)
> +{
> +	writeb(0x00, p + UART_EXAR_MPIOINT_7_0);
> +	writeb(0x00, p + UART_EXAR_MPIOLVL_7_0);
> +	writeb(0x00, p + UART_EXAR_MPIO3T_7_0);
> +	writeb(0x00, p + UART_EXAR_MPIOINV_7_0);
> +	writeb(0x00, p + UART_EXAR_MPIOSEL_7_0);
> +	writeb(0x00, p + UART_EXAR_MPIOOD_7_0);
> +	writeb(0x00, p + UART_EXAR_MPIOINT_15_8);
> +	writeb(0x00, p + UART_EXAR_MPIOLVL_15_8);
> +	writeb(0x00, p + UART_EXAR_MPIO3T_15_8);
> +	writeb(0x00, p + UART_EXAR_MPIOINV_15_8);
> +	writeb(0x00, p + UART_EXAR_MPIOSEL_15_8);
> +	writeb(0x00, p + UART_EXAR_MPIOOD_15_8);
> +}
> +
> +static void *
> +xr17v35x_register_gpio(struct pci_dev *pcidev)
> +{
> +	struct platform_device *pdev;
> +
> +	pdev = platform_device_alloc("gpio_exar", PLATFORM_DEVID_AUTO);
> +	if (!pdev)
> +		return NULL;
> +
> +	platform_set_drvdata(pdev, pcidev);
> +	if (platform_device_add(pdev) < 0) {
> +		platform_device_put(pdev);
> +		return NULL;
> +	}
> +
> +	return pdev;
> +}
> +
> +static int
> +pci_xr17v35x_setup(struct exar8250 *priv, struct pci_dev *pcidev,
> +		   struct uart_8250_port *port, int idx)
> +{
> +	const struct exar8250_board *board = priv->board;
> +	unsigned int offset = idx * 0x400;
> +	unsigned int baud = 7812500;
> +	u8 __iomem *p;
> +	int ret;
> +
> +	port->port.uartclk = baud * 16;
> +	/*
> +	 * Setup the uart clock for the devices on expansion slot to
> +	 * half the clock speed of the main chip (which is 125MHz)
> +	 */
> +	if (board->has_slave && idx >= 8)
> +		port->port.uartclk /= 2;
> +
> +	p = pci_ioremap_bar(pcidev, 0);

If we move the default_setup before this, we can use pcim_iomap_table()
and avoid this temporary mapping completely.

> +	if (!p)
> +		return -ENOMEM;
> +
> +	/* Setup Multipurpose Input/Output pins. */
> +	if (idx == 0)
> +		setup_gpio(p);
> +
> +	writeb(0x00, p + UART_EXAR_8XMODE);
> +	writeb(UART_FCTR_EXAR_TRGD, p + UART_EXAR_FCTR);
> +	writeb(128, p + UART_EXAR_TXTRG);
> +	writeb(128, p + UART_EXAR_RXTRG);
> +	iounmap(p);
> +
> +	ret = default_setup(priv, pcidev, idx, offset, port);
> +	if (ret)
> +		return ret;
> +
> +	if (idx == 0)
> +		port->port.private_data =
> +			xr17v35x_register_gpio(pcidev);
> +
> +	return 0;
> +}

I suppose I should still send patches on top, right?

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

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

#1575097 — Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

On 2017-02-06 20:37, Jan Kiszka wrote:
> On 2017-01-30 23:28, Sudip Mukherjee wrote:
>> From: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>
>>
>> Add the serial driver for the Exar chips. And also register the
>> platform device for the GPIO provided by the Exar chips.
>>
>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>> Signed-off-by: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>
> 
> ...
> 
>> +static int default_setup(struct exar8250 *priv, struct pci_dev *pcidev,
>> +			 int idx, unsigned int offset,
>> +			 struct uart_8250_port *port)
>> +{
>> +	const struct exar8250_board *board = priv->board;
>> +	unsigned int bar = 0;
>> +
>> +	port->port.iotype = UPIO_MEM;
>> +	port->port.mapbase = pci_resource_start(pcidev, bar) + offset;
>> +	port->port.membase = pcim_iomap_table(pcidev)[bar] + offset;
> 
> This always gives you 0 for membase because you missed to call
> pcim_iomap for bar 0.
> 
> Sorry to pick on this piece-wise, but I just ran into this bug now while
> porting patches over.
> 
>> +	port->port.regshift = board->reg_shift;
>> +
>> +	return 0;
>> +}
>> +
>> +static int
>> +pci_connect_tech_setup(struct exar8250 *priv, struct pci_dev *pcidev,
>> +		       struct uart_8250_port *port, int idx)
>> +{
>> +	unsigned int offset = idx * 0x200;
>> +	unsigned int baud = 1843200;
>> +
>> +	port->port.uartclk = baud * 16;
>> +	return default_setup(priv, pcidev, idx, offset, port);
>> +}
>> +
>> +static int
>> +pci_xr17c154_setup(struct exar8250 *priv, struct pci_dev *pcidev,
>> +		   struct uart_8250_port *port, int idx)
>> +{
>> +	unsigned int offset = idx * 0x200;
>> +	unsigned int baud = 921600;
>> +
>> +	port->port.uartclk = baud * 16;
>> +	return default_setup(priv, pcidev, idx, offset, port);
>> +}
>> +
>> +static void setup_gpio(u8 __iomem *p)
>> +{
>> +	writeb(0x00, p + UART_EXAR_MPIOINT_7_0);
>> +	writeb(0x00, p + UART_EXAR_MPIOLVL_7_0);
>> +	writeb(0x00, p + UART_EXAR_MPIO3T_7_0);
>> +	writeb(0x00, p + UART_EXAR_MPIOINV_7_0);
>> +	writeb(0x00, p + UART_EXAR_MPIOSEL_7_0);
>> +	writeb(0x00, p + UART_EXAR_MPIOOD_7_0);
>> +	writeb(0x00, p + UART_EXAR_MPIOINT_15_8);
>> +	writeb(0x00, p + UART_EXAR_MPIOLVL_15_8);
>> +	writeb(0x00, p + UART_EXAR_MPIO3T_15_8);
>> +	writeb(0x00, p + UART_EXAR_MPIOINV_15_8);
>> +	writeb(0x00, p + UART_EXAR_MPIOSEL_15_8);
>> +	writeb(0x00, p + UART_EXAR_MPIOOD_15_8);
>> +}
>> +
>> +static void *
>> +xr17v35x_register_gpio(struct pci_dev *pcidev)
>> +{
>> +	struct platform_device *pdev;
>> +
>> +	pdev = platform_device_alloc("gpio_exar", PLATFORM_DEVID_AUTO);
>> +	if (!pdev)
>> +		return NULL;
>> +
>> +	platform_set_drvdata(pdev, pcidev);
>> +	if (platform_device_add(pdev) < 0) {
>> +		platform_device_put(pdev);
>> +		return NULL;
>> +	}
>> +
>> +	return pdev;
>> +}
>> +
>> +static int
>> +pci_xr17v35x_setup(struct exar8250 *priv, struct pci_dev *pcidev,
>> +		   struct uart_8250_port *port, int idx)
>> +{
>> +	const struct exar8250_board *board = priv->board;
>> +	unsigned int offset = idx * 0x400;
>> +	unsigned int baud = 7812500;
>> +	u8 __iomem *p;
>> +	int ret;
>> +
>> +	port->port.uartclk = baud * 16;
>> +	/*
>> +	 * Setup the uart clock for the devices on expansion slot to
>> +	 * half the clock speed of the main chip (which is 125MHz)
>> +	 */
>> +	if (board->has_slave && idx >= 8)
>> +		port->port.uartclk /= 2;
>> +
>> +	p = pci_ioremap_bar(pcidev, 0);
> 
> If we move the default_setup before this, we can use pcim_iomap_table()
> and avoid this temporary mapping completely.

Actually, we should use port->port.membase here: The original code that
came from 8250_pci was broken already by always programming port 0,
irrespective of idx. Using membase, we will pick the right address for
the target port.

Jan

> 
>> +	if (!p)
>> +		return -ENOMEM;
>> +
>> +	/* Setup Multipurpose Input/Output pins. */
>> +	if (idx == 0)
>> +		setup_gpio(p);
>> +
>> +	writeb(0x00, p + UART_EXAR_8XMODE);
>> +	writeb(UART_FCTR_EXAR_TRGD, p + UART_EXAR_FCTR);
>> +	writeb(128, p + UART_EXAR_TXTRG);
>> +	writeb(128, p + UART_EXAR_RXTRG);
>> +	iounmap(p);
>> +
>> +	ret = default_setup(priv, pcidev, idx, offset, port);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (idx == 0)
>> +		port->port.private_data =
>> +			xr17v35x_register_gpio(pcidev);
>> +
>> +	return 0;
>> +}
> 
> I suppose I should still send patches on top, right?
> 
> Jan
> 

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web