[PATCH V2 1/2] serial/8250: Add support for NI-Serial PXI/PXIe+485 devices

[PATCH V2 1/2] serial/8250: Add support for NI-Serial PXI/PXIe+485 devices

[jeyentam]

Add support for NI-Serial PXIe-RS232, PXI-RS485 and PXIe-RS485 devices.

Signed-off-by: jeyentam <je.yen.tam@ni.com>
---
 drivers/tty/serial/8250/8250_pci.c | 293 ++++++++++++++++++++++++++++-
 1 file changed, 289 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
index df41397de478..ac8138adea9c 100644
--- a/drivers/tty/serial/8250/8250_pci.c
+++ b/drivers/tty/serial/8250/8250_pci.c
@@ -730,8 +730,16 @@ static int pci_ni8430_init(struct pci_dev *dev)
 }
 
 /* UART Port Control Register */
-#define NI8430_PORTCON	0x0f
-#define NI8430_PORTCON_TXVR_ENABLE	(1 << 3)
+#define NI16550_PCR_OFFSET	0x0f
+#define NI16550_PCR_RS422	0x00
+#define NI16550_PCR_ECHO_RS485	0x01
+#define NI16550_PCR_DTR_RS485	0x02
+#define NI16550_PCR_AUTO_RS485	0x03
+#define NI16550_PCR_WIRE_MODE_MASK	0x03
+#define NI16550_PCR_TXVR_ENABLE_BIT	(1 << 3)
+#define NI16550_PCR_RS485_TERMINATION_BIT	(1 << 6)
+#define NI16550_ACR_DTR_AUTO_DTR	(0x2 << 3)
+#define NI16550_ACR_DTR_MANUAL_DTR	(0x0 << 3)
 
 static int
 pci_ni8430_setup(struct serial_private *priv,
@@ -753,14 +761,127 @@ pci_ni8430_setup(struct serial_private *priv,
 		return -ENOMEM;
 
 	/* enable the transceiver */
-	writeb(readb(p + offset + NI8430_PORTCON) | NI8430_PORTCON_TXVR_ENABLE,
-	       p + offset + NI8430_PORTCON);
+	writeb(readb(p + offset + NI16550_PCR_OFFSET) |
+			NI16550_PCR_TXVR_ENABLE_BIT,
+			p + offset + NI16550_PCR_OFFSET);
 
 	iounmap(p);
 
 	return setup_port(priv, port, bar, offset, board->reg_shift);
 }
 
+static int pci_ni8431_config_rs485(struct uart_port *port,
+	struct serial_rs485 *rs485)
+{
+	u8 pcr, acr;
+
+	struct uart_8250_port *up;
+
+	up = container_of(port, struct uart_8250_port, port);
+
+	acr = up->acr;
+
+	dev_dbg(port->dev, "ni16550_config_rs485\n");
+
+	/* "rs485" should be given to us non-NULL. */
+	WARN_ON(rs485 == NULL);
+
+	pcr = port->serial_in(port, NI16550_PCR_OFFSET);
+	pcr &= ~NI16550_PCR_WIRE_MODE_MASK;
+
+	if (rs485->flags & SER_RS485_ENABLED) {
+		/* RS-485 */
+		if ((rs485->flags & SER_RS485_RX_DURING_TX) &&
+			(rs485->flags & SER_RS485_RTS_ON_SEND)) {
+			dev_dbg(port->dev, "Invalid 2-wire mode\n");
+			return -EINVAL;
+		}
+
+		if (rs485->flags & SER_RS485_RX_DURING_TX) {
+			/* Echo */
+			dev_vdbg(port->dev, "2-wire DTR with echo\n");
+			pcr |= NI16550_PCR_ECHO_RS485;
+			acr |= NI16550_ACR_DTR_MANUAL_DTR;
+		} else {
+			/* Auto or DTR */
+			if (rs485->flags & SER_RS485_RTS_ON_SEND) {
+				/* Auto */
+				dev_vdbg(port->dev, "2-wire Auto\n");
+				pcr |= NI16550_PCR_AUTO_RS485;
+				acr |= NI16550_ACR_DTR_AUTO_DTR;
+			} else {
+				/* DTR-controlled */
+				/* No Echo */
+				dev_vdbg(port->dev, "2-wire DTR no echo\n");
+				pcr |= NI16550_PCR_DTR_RS485;
+				acr |= NI16550_ACR_DTR_MANUAL_DTR;
+			}
+		}
+	} else {
+		/* RS-422 */
+		dev_vdbg(port->dev, "4-wire\n");
+		pcr |= NI16550_PCR_RS422;
+		acr |= NI16550_ACR_DTR_MANUAL_DTR;
+	}
+
+	dev_dbg(port->dev, "write pcr: 0x%08x\n", pcr);
+	port->serial_out(port, NI16550_PCR_OFFSET, pcr);
+
+	up->acr = acr;
+	port->serial_out(port, UART_SCR, UART_ACR);
+	port->serial_out(port, UART_ICR, up->acr);
+
+	/* Update the cache. */
+	port->rs485 = *rs485;
+
+	dev_dbg(port->dev, "ni16550_config_rs485\n");
+	return 0;
+}
+
+static int pci_ni8431_setup(struct serial_private *priv,
+		 const struct pciserial_board *board,
+		 struct uart_8250_port *uart, int idx)
+{
+	u8 pcr, acr;
+	struct pci_dev *dev = priv->dev;
+	void __iomem *addr;
+	unsigned int bar, offset = board->first_offset;
+
+	if (idx >= board->num_ports)
+		return 1;
+
+	bar = FL_GET_BASE(board->flags);
+	offset += idx * board->uart_offset;
+
+	addr = pci_ioremap_bar(dev, bar);
+	if (!addr)
+		return -ENOMEM;
+
+	/* enable the transceiver */
+	writeb(readb(addr + NI16550_PCR_OFFSET) | NI16550_PCR_TXVR_ENABLE_BIT,
+		addr + NI16550_PCR_OFFSET);
+
+	pcr = readb(addr + NI16550_PCR_OFFSET);
+	pcr &= ~NI16550_PCR_WIRE_MODE_MASK;
+
+	/* set wire mode to default RS-422 */
+	pcr |= NI16550_PCR_RS422;
+	acr = NI16550_ACR_DTR_MANUAL_DTR;
+
+	/* write port configuration to register */
+	writeb(pcr, addr + NI16550_PCR_OFFSET);
+
+	/* access and write to UART acr register */
+	writeb(UART_ACR, addr + UART_SCR);
+	writeb(acr, addr + UART_ICR);
+
+	uart->port.rs485_config = &pci_ni8431_config_rs485;
+
+	iounmap(addr);
+
+	return setup_port(priv, uart, bar, offset, board->reg_shift);
+}
+
 static int pci_netmos_9900_setup(struct serial_private *priv,
 				const struct pciserial_board *board,
 				struct uart_8250_port *port, int idx)
@@ -1956,6 +2077,87 @@ static struct pci_serial_quirk pci_serial_quirks[] __refdata = {
 		.setup		= pci_ni8430_setup,
 		.exit		= pci_ni8430_exit,
 	},
+	{
+		.vendor		= PCI_VENDOR_ID_NI,
+		.device		= PCIE_DEVICE_ID_NI_PXIE8430_2328,
+		.subvendor	= PCI_ANY_ID,
+		.subdevice	= PCI_ANY_ID,
+		.init		= pci_ni8430_init,
+		.setup		= pci_ni8430_setup,
+		.exit		= pci_ni8430_exit,
+	},
+	{
+		.vendor		= PCI_VENDOR_ID_NI,
+		.device		= PCIE_DEVICE_ID_NI_PXIE8430_23216,
+		.subvendor	= PCI_ANY_ID,
+		.subdevice	= PCI_ANY_ID,
+		.init		= pci_ni8430_init,
+		.setup		= pci_ni8430_setup,
+		.exit		= pci_ni8430_exit,
+	},
+	{
+		.vendor		= PCI_VENDOR_ID_NI,
+		.device		= PCI_DEVICE_ID_NI_PXI8431_4852,
+		.subvendor	= PCI_ANY_ID,
+		.subdevice	= PCI_ANY_ID,
+		.init		= pci_ni8430_init,
+		.setup		= pci_ni8431_setup,
+		.exit		= pci_ni8430_exit,
+	},
+	{
+		.vendor		= PCI_VENDOR_ID_NI,
+		.device		= PCI_DEVICE_ID_NI_PXI8431_4854,
+		.subvendor	= PCI_ANY_ID,
+		.subdevice	= PCI_ANY_ID,
+		.init		= pci_ni8430_init,
+		.setup		= pci_ni8431_setup,
+		.exit		= pci_ni8430_exit,
+	},
+	{
+		.vendor		= PCI_VENDOR_ID_NI,
+		.device		= PCI_DEVICE_ID_NI_PXI8431_4858,
+		.subvendor	= PCI_ANY_ID,
+		.subdevice	= PCI_ANY_ID,
+		.init		= pci_ni8430_init,
+		.setup		= pci_ni8431_setup,
+		.exit		= pci_ni8430_exit,
+	},
+	{
+		.vendor		= PCI_VENDOR_ID_NI,
+		.device		= PCI_DEVICE_ID_NI_PXI8433_4852,
+		.subvendor	= PCI_ANY_ID,
+		.subdevice	= PCI_ANY_ID,
+		.init		= pci_ni8430_init,
+		.setup		= pci_ni8431_setup,
+		.exit		= pci_ni8430_exit,
+	},
+	{
+		.vendor		= PCI_VENDOR_ID_NI,
+		.device		= PCI_DEVICE_ID_NI_PXI8433_4854,
+		.subvendor	= PCI_ANY_ID,
+		.subdevice	= PCI_ANY_ID,
+		.init		= pci_ni8430_init,
+		.setup		= pci_ni8431_setup,
+		.exit		= pci_ni8430_exit,
+	},
+	{
+		.vendor		= PCI_VENDOR_ID_NI,
+		.device		= PCIE_DEVICE_ID_NI_PXIE8431_4858,
+		.subvendor	= PCI_ANY_ID,
+		.subdevice	= PCI_ANY_ID,
+		.init		= pci_ni8430_init,
+		.setup		= pci_ni8431_setup,
+		.exit		= pci_ni8430_exit,
+	},
+	{
+		.vendor		= PCI_VENDOR_ID_NI,
+		.device		= PCIE_DEVICE_ID_NI_PXIE8431_48516,
+		.subvendor	= PCI_ANY_ID,
+		.subdevice	= PCI_ANY_ID,
+		.init		= pci_ni8430_init,
+		.setup		= pci_ni8431_setup,
+		.exit		= pci_ni8430_exit,
+	},
 	/* Quatech */
 	{
 		.vendor		= PCI_VENDOR_ID_QUATECH,
@@ -2679,6 +2881,13 @@ enum pci_board_num_t {
 	pbn_ni8430_4,
 	pbn_ni8430_8,
 	pbn_ni8430_16,
+	pbn_ni8430_pxie_8,
+	pbn_ni8430_pxie_16,
+	pbn_ni8431_2,
+	pbn_ni8431_4,
+	pbn_ni8431_8,
+	pbn_ni8431_pxie_8,
+	pbn_ni8431_pxie_16,
 	pbn_ADDIDATA_PCIe_1_3906250,
 	pbn_ADDIDATA_PCIe_2_3906250,
 	pbn_ADDIDATA_PCIe_4_3906250,
@@ -3320,6 +3529,55 @@ static struct pciserial_board pci_boards[] = {
 		.uart_offset	= 0x10,
 		.first_offset	= 0x800,
 	},
+	[pbn_ni8430_pxie_16] = {
+		.flags		= FL_BASE0,
+		.num_ports	= 16,
+		.base_baud	= 3125000,
+		.uart_offset	= 0x10,
+		.first_offset	= 0x800,
+	},
+	[pbn_ni8430_pxie_8] = {
+		.flags		= FL_BASE0,
+		.num_ports	= 8,
+		.base_baud	= 3125000,
+		.uart_offset	= 0x10,
+		.first_offset	= 0x800,
+	},
+	[pbn_ni8431_8] = {
+		.flags		= FL_BASE0,
+		.num_ports	= 8,
+		.base_baud	= 3686400,
+		.uart_offset	= 0x10,
+		.first_offset	= 0x800,
+	},
+	[pbn_ni8431_4] = {
+		.flags		= FL_BASE0,
+		.num_ports	= 4,
+		.base_baud	= 3686400,
+		.uart_offset	= 0x10,
+		.first_offset	= 0x800,
+	},
+	[pbn_ni8431_2] = {
+		.flags		= FL_BASE0,
+		.num_ports	= 2,
+		.base_baud	= 3686400,
+		.uart_offset	= 0x10,
+		.first_offset	= 0x800,
+	},
+	[pbn_ni8431_pxie_16] = {
+		.flags		= FL_BASE0,
+		.num_ports	= 16,
+		.base_baud	= 3125000,
+		.uart_offset	= 0x10,
+		.first_offset	= 0x800,
+	},
+	[pbn_ni8431_pxie_8] = {
+		.flags		= FL_BASE0,
+		.num_ports	= 8,
+		.base_baud	= 3125000,
+		.uart_offset	= 0x10,
+		.first_offset	= 0x800,
+	},
 	/*
 	 * ADDI-DATA GmbH PCI-Express communication cards <info@addi-data.com>
 	 */
@@ -5003,6 +5261,33 @@ static const struct pci_device_id serial_pci_tbl[] = {
 	{	PCI_VENDOR_ID_NI, PCI_DEVICE_ID_NI_PCI8432_2324,
 		PCI_ANY_ID, PCI_ANY_ID, 0, 0,
 		pbn_ni8430_4 },
+	{	PCI_VENDOR_ID_NI, PCIE_DEVICE_ID_NI_PXIE8430_2328,
+		PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+		pbn_ni8430_pxie_8 },
+	{	PCI_VENDOR_ID_NI, PCIE_DEVICE_ID_NI_PXIE8430_23216,
+		PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+		pbn_ni8430_pxie_16 },
+	{	PCI_VENDOR_ID_NI, PCI_DEVICE_ID_NI_PXI8431_4852,
+		PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+		pbn_ni8431_2 },
+	{	PCI_VENDOR_ID_NI, PCI_DEVICE_ID_NI_PXI8431_4854,
+		PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+		pbn_ni8431_4 },
+	{	PCI_VENDOR_ID_NI, PCI_DEVICE_ID_NI_PXI8431_4858,
+		PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+		pbn_ni8431_8 },
+	{	PCI_VENDOR_ID_NI, PCIE_DEVICE_ID_NI_PXIE8431_4858,
+		PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+		pbn_ni8431_pxie_8 },
+	{	PCI_VENDOR_ID_NI, PCIE_DEVICE_ID_NI_PXIE8431_48516,
+		PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+		pbn_ni8431_pxie_16 },
+	{	PCI_VENDOR_ID_NI, PCI_DEVICE_ID_NI_PXI8433_4852,
+		PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+		pbn_ni8431_2 },
+	{	PCI_VENDOR_ID_NI, PCI_DEVICE_ID_NI_PXI8433_4854,
+		PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+		pbn_ni8431_4 },
 
 	/*
 	* ADDI-DATA GmbH communication cards <info@addi-data.com>
-- 
2.17.1

Re: [PATCH V2 1/2] serial/8250: Add support for NI-Serial PXI/PXIe+485 devices

[Greg KH]

On Wed, Jul 03, 2019 at 12:24:35AM -0700, jeyentam wrote:
> Add support for NI-Serial PXIe-RS232, PXI-RS485 and PXIe-RS485 devices.
> 
> Signed-off-by: jeyentam <je.yen.tam@ni.com>

I need a "full" name here please.

Also, this patch breaks the build, which is not allowed, and makes me
wonder how you tested it :(

Please fix up properly and resend.

Also, some comments below:

> ---
>  drivers/tty/serial/8250/8250_pci.c | 293 ++++++++++++++++++++++++++++-
>  1 file changed, 289 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
> index df41397de478..ac8138adea9c 100644
> --- a/drivers/tty/serial/8250/8250_pci.c
> +++ b/drivers/tty/serial/8250/8250_pci.c
> @@ -730,8 +730,16 @@ static int pci_ni8430_init(struct pci_dev *dev)
>  }
>  
>  /* UART Port Control Register */
> -#define NI8430_PORTCON	0x0f
> -#define NI8430_PORTCON_TXVR_ENABLE	(1 << 3)

Why are you renaming these #defines?

> +#define NI16550_PCR_OFFSET	0x0f
> +#define NI16550_PCR_RS422	0x00
> +#define NI16550_PCR_ECHO_RS485	0x01
> +#define NI16550_PCR_DTR_RS485	0x02
> +#define NI16550_PCR_AUTO_RS485	0x03
> +#define NI16550_PCR_WIRE_MODE_MASK	0x03
> +#define NI16550_PCR_TXVR_ENABLE_BIT	(1 << 3)

BIT(3)?

> +#define NI16550_PCR_RS485_TERMINATION_BIT	(1 << 6)

BIT(6)?

> +#define NI16550_ACR_DTR_AUTO_DTR	(0x2 << 3)
> +#define NI16550_ACR_DTR_MANUAL_DTR	(0x0 << 3)
>  
>  static int
>  pci_ni8430_setup(struct serial_private *priv,
> @@ -753,14 +761,127 @@ pci_ni8430_setup(struct serial_private *priv,
>  		return -ENOMEM;
>  
>  	/* enable the transceiver */
> -	writeb(readb(p + offset + NI8430_PORTCON) | NI8430_PORTCON_TXVR_ENABLE,
> -	       p + offset + NI8430_PORTCON);
> +	writeb(readb(p + offset + NI16550_PCR_OFFSET) |
> +			NI16550_PCR_TXVR_ENABLE_BIT,
> +			p + offset + NI16550_PCR_OFFSET);

Why indent like this?  Please follow the indentation that used to be
there.

>  
>  	iounmap(p);
>  
>  	return setup_port(priv, port, bar, offset, board->reg_shift);
>  }
>  
> +static int pci_ni8431_config_rs485(struct uart_port *port,
> +	struct serial_rs485 *rs485)
> +{
> +	u8 pcr, acr;
> +
> +	struct uart_8250_port *up;
> +
> +	up = container_of(port, struct uart_8250_port, port);
> +
> +	acr = up->acr;
> +
> +	dev_dbg(port->dev, "ni16550_config_rs485\n");

No need for debugging lines like this, use ftrace please.

> +
> +	/* "rs485" should be given to us non-NULL. */
> +	WARN_ON(rs485 == NULL);

Don't crash people's machines, if this can never happen, then no need
for this line.  If it can happen, test and properly return an error.

> +
> +	pcr = port->serial_in(port, NI16550_PCR_OFFSET);
> +	pcr &= ~NI16550_PCR_WIRE_MODE_MASK;
> +
> +	if (rs485->flags & SER_RS485_ENABLED) {
> +		/* RS-485 */
> +		if ((rs485->flags & SER_RS485_RX_DURING_TX) &&
> +			(rs485->flags & SER_RS485_RTS_ON_SEND)) {
> +			dev_dbg(port->dev, "Invalid 2-wire mode\n");
> +			return -EINVAL;
> +		}
> +
> +		if (rs485->flags & SER_RS485_RX_DURING_TX) {
> +			/* Echo */
> +			dev_vdbg(port->dev, "2-wire DTR with echo\n");
> +			pcr |= NI16550_PCR_ECHO_RS485;
> +			acr |= NI16550_ACR_DTR_MANUAL_DTR;
> +		} else {
> +			/* Auto or DTR */
> +			if (rs485->flags & SER_RS485_RTS_ON_SEND) {
> +				/* Auto */
> +				dev_vdbg(port->dev, "2-wire Auto\n");
> +				pcr |= NI16550_PCR_AUTO_RS485;
> +				acr |= NI16550_ACR_DTR_AUTO_DTR;
> +			} else {
> +				/* DTR-controlled */
> +				/* No Echo */
> +				dev_vdbg(port->dev, "2-wire DTR no echo\n");
> +				pcr |= NI16550_PCR_DTR_RS485;
> +				acr |= NI16550_ACR_DTR_MANUAL_DTR;
> +			}
> +		}
> +	} else {
> +		/* RS-422 */
> +		dev_vdbg(port->dev, "4-wire\n");
> +		pcr |= NI16550_PCR_RS422;
> +		acr |= NI16550_ACR_DTR_MANUAL_DTR;
> +	}
> +
> +	dev_dbg(port->dev, "write pcr: 0x%08x\n", pcr);
> +	port->serial_out(port, NI16550_PCR_OFFSET, pcr);
> +
> +	up->acr = acr;
> +	port->serial_out(port, UART_SCR, UART_ACR);
> +	port->serial_out(port, UART_ICR, up->acr);
> +
> +	/* Update the cache. */
> +	port->rs485 = *rs485;
> +
> +	dev_dbg(port->dev, "ni16550_config_rs485\n");

Again, use ftrace, not dev_dbg() for stuff like this.

thanks,

greg k-h

Re: [PATCH V2 1/2] serial/8250: Add support for NI-Serial PXI/PXIe+485 devices

[Enrico Weigelt, metux IT consult]

On 03.07.19 09:24, jeyentam wrote:

Hi,

besides what Greg already said:

>  /* UART Port Control Register */> -#define NI8430_PORTCON	0x0f> -#define NI8430_PORTCON_TXVR_ENABLE	(1 << 3)
Can we have that renaming as a separate patch, to ease review ?

And what about introducing a config sym for the new device specific
stuff ? These devices seem to be pretty rare - never seen them in
embeded world, where we do need to care of kernel size.


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [PATCH V2 1/2] serial/8250: Add support for NI-Serial PXI/PXIe+485 devices

[Greg KH]

On Wed, Jul 03, 2019 at 03:27:11PM +0200, Enrico Weigelt, metux IT consult wrote:
> >  /* UART Port Control Register */> -#define NI8430_PORTCON	0x0f> -#define NI8430_PORTCON_TXVR_ENABLE	(1 << 3)
> Can we have that renaming as a separate patch, to ease review ?
> 
> And what about introducing a config sym for the new device specific
> stuff ? These devices seem to be pretty rare - never seen them in
> embeded world, where we do need to care of kernel size.

No, that's not the way this driver works, sorry.

greg k-h

[PATCH V2 1/2] serial/8250: Add support for NI-Serial PXI/PXIe+485 devices

[Enrico Weigelt, metux IT consult]

On 03.07.19 16:47, Greg KH wrote:
> On Wed, Jul 03, 2019 at 03:27:11PM +0200, Enrico Weigelt, metux IT consult wrote:
>>>  /* UART Port Control Register */> -#define NI8430_PORTCON	0x0f> -#define NI8430_PORTCON_TXVR_ENABLE	(1 << 3)
>> Can we have that renaming as a separate patch, to ease review ?
>>
>> And what about introducing a config sym for the new device specific
>> stuff ? These devices seem to be pretty rare - never seen them in
>> embeded world, where we do need to care of kernel size.
> 
> No, that's not the way this driver works, sorry.

That's sad, because in embedded world we often have to care about
code size, so making those devices optional would be of great help.

OTOH, the 8250 driver already has several such options. What's your
concern about adding another one for a new device ? Config variant
inflation ? Would putting it behind CONFIG_EXPERT or CONFIG_EMBEDDED
an option ?


thx.
--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [PATCH V2 1/2] serial/8250: Add support for NI-Serial PXI/PXIe+485 devices

[Greg KH]

On Wed, Jul 03, 2019 at 05:41:25PM +0200, Enrico Weigelt, metux IT consult wrote:
> On 03.07.19 16:47, Greg KH wrote:
> > On Wed, Jul 03, 2019 at 03:27:11PM +0200, Enrico Weigelt, metux IT consult wrote:
> >>>  /* UART Port Control Register */> -#define NI8430_PORTCON	0x0f> -#define NI8430_PORTCON_TXVR_ENABLE	(1 << 3)
> >> Can we have that renaming as a separate patch, to ease review ?
> >>
> >> And what about introducing a config sym for the new device specific
> >> stuff ? These devices seem to be pretty rare - never seen them in
> >> embeded world, where we do need to care of kernel size.
> > 
> > No, that's not the way this driver works, sorry.
> 
> That's sad, because in embedded world we often have to care about
> code size, so making those devices optional would be of great help.

Really?  are you sure?  Try it and see what you really end up saving.

greg k-h

RE: Re: [PATCH V2 1/2] serial/8250: Add support for NI-Serial PXI/PXIe+485 devices

[Je Yen Tam]

> On Wed, Jul 03, 2019 at 12:24:35AM -0700, jeyentam wrote:
> > Add support for NI-Serial PXIe-RS232, PXI-RS485 and PXIe-RS485 devices.
> >
> > Signed-off-by: jeyentam <je.yen.tam@ni.com>
> 
> I need a "full" name here please.

Ok, will do so.

> 
> Also, this patch breaks the build, which is not allowed, and makes me wonder how
> you tested it :(

I've tested my changes with the steps as follows, it did not break.

1. Clone the Linux repo, check out the source and work on my project.
2. Compile the kernel source with "make j4".
3. Build the kernel modules with "make modules_install".
4. Install the kernel with "make install".
5. Reboot into the newly built kernel.
6. The kernel worked well and able to fit my needs.

Noted that my source is checked out from the latest Linux kernel 5.2.0-rc6.

Also, I checked my patch using checkpatch.pl, it did not show any errors and 
warnings too.

May I know what do you mean by breaking the build?

> 
> Please fix up properly and resend.
> 
> Also, some comments below:
> 
> > ---
> >  drivers/tty/serial/8250/8250_pci.c | 293
> > ++++++++++++++++++++++++++++-
> >  1 file changed, 289 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/tty/serial/8250/8250_pci.c
> > b/drivers/tty/serial/8250/8250_pci.c
> > index df41397de478..ac8138adea9c 100644
> > --- a/drivers/tty/serial/8250/8250_pci.c
> > +++ b/drivers/tty/serial/8250/8250_pci.c
> > @@ -730,8 +730,16 @@ static int pci_ni8430_init(struct pci_dev *dev)
> > }
> >
> >  /* UART Port Control Register */
> > -#define NI8430_PORTCON	0x0f
> > -#define NI8430_PORTCON_TXVR_ENABLE	(1 << 3)
> 
> Why are you renaming these #defines?

NI8430 device is using standard 16550 UART chip so I standardize all the 16550 
UART related #defines to UART chip specific name rather than device specific.

> 
> > +#define NI16550_PCR_OFFSET	0x0f
> > +#define NI16550_PCR_RS422	0x00
> > +#define NI16550_PCR_ECHO_RS485	0x01
> > +#define NI16550_PCR_DTR_RS485	0x02
> > +#define NI16550_PCR_AUTO_RS485	0x03
> > +#define NI16550_PCR_WIRE_MODE_MASK	0x03
> > +#define NI16550_PCR_TXVR_ENABLE_BIT	(1 << 3)
> 
> BIT(3)?

Ok, will change to this style.

> 
> > +#define NI16550_PCR_RS485_TERMINATION_BIT	(1 << 6)
> 
> BIT(6)?

Also, will change this.

> 
> > +#define NI16550_ACR_DTR_AUTO_DTR	(0x2 << 3)
> > +#define NI16550_ACR_DTR_MANUAL_DTR	(0x0 << 3)
> >
> >  static int
> >  pci_ni8430_setup(struct serial_private *priv, @@ -753,14 +761,127 @@
> > pci_ni8430_setup(struct serial_private *priv,
> >  		return -ENOMEM;
> >
> >  	/* enable the transceiver */
> > -	writeb(readb(p + offset + NI8430_PORTCON) |
> NI8430_PORTCON_TXVR_ENABLE,
> > -	       p + offset + NI8430_PORTCON);
> > +	writeb(readb(p + offset + NI16550_PCR_OFFSET) |
> > +			NI16550_PCR_TXVR_ENABLE_BIT,
> > +			p + offset + NI16550_PCR_OFFSET);
> 
> Why indent like this?  Please follow the indentation that used to be there.

I moved the indentation because it complained this line exceeded 80 characters 
when I check the patch. I will change back to original indentation.

> 
> >
> >  	iounmap(p);
> >
> >  	return setup_port(priv, port, bar, offset, board->reg_shift);  }
> >
> > +static int pci_ni8431_config_rs485(struct uart_port *port,
> > +	struct serial_rs485 *rs485)
> > +{
> > +	u8 pcr, acr;
> > +
> > +	struct uart_8250_port *up;
> > +
> > +	up = container_of(port, struct uart_8250_port, port);
> > +
> > +	acr = up->acr;
> > +
> > +	dev_dbg(port->dev, "ni16550_config_rs485\n");
> 
> No need for debugging lines like this, use ftrace please.

Ok, will update this.

> 
> > +
> > +	/* "rs485" should be given to us non-NULL. */
> > +	WARN_ON(rs485 == NULL);
> 
> Don't crash people's machines, if this can never happen, then no need for this line.  If
> it can happen, test and properly return an error.

This is more like a fail safe approach, it should never happen, will remove this line.

> 
> > +
> > +	pcr = port->serial_in(port, NI16550_PCR_OFFSET);
> > +	pcr &= ~NI16550_PCR_WIRE_MODE_MASK;
> > +
> > +	if (rs485->flags & SER_RS485_ENABLED) {
> > +		/* RS-485 */
> > +		if ((rs485->flags & SER_RS485_RX_DURING_TX) &&
> > +			(rs485->flags & SER_RS485_RTS_ON_SEND)) {
> > +			dev_dbg(port->dev, "Invalid 2-wire mode\n");
> > +			return -EINVAL;
> > +		}
> > +
> > +		if (rs485->flags & SER_RS485_RX_DURING_TX) {
> > +			/* Echo */
> > +			dev_vdbg(port->dev, "2-wire DTR with echo\n");
> > +			pcr |= NI16550_PCR_ECHO_RS485;
> > +			acr |= NI16550_ACR_DTR_MANUAL_DTR;
> > +		} else {
> > +			/* Auto or DTR */
> > +			if (rs485->flags & SER_RS485_RTS_ON_SEND) {
> > +				/* Auto */
> > +				dev_vdbg(port->dev, "2-wire Auto\n");
> > +				pcr |= NI16550_PCR_AUTO_RS485;
> > +				acr |= NI16550_ACR_DTR_AUTO_DTR;
> > +			} else {
> > +				/* DTR-controlled */
> > +				/* No Echo */
> > +				dev_vdbg(port->dev, "2-wire DTR no echo\n");
> > +				pcr |= NI16550_PCR_DTR_RS485;
> > +				acr |= NI16550_ACR_DTR_MANUAL_DTR;
> > +			}
> > +		}
> > +	} else {
> > +		/* RS-422 */
> > +		dev_vdbg(port->dev, "4-wire\n");
> > +		pcr |= NI16550_PCR_RS422;
> > +		acr |= NI16550_ACR_DTR_MANUAL_DTR;
> > +	}
> > +
> > +	dev_dbg(port->dev, "write pcr: 0x%08x\n", pcr);
> > +	port->serial_out(port, NI16550_PCR_OFFSET, pcr);
> > +
> > +	up->acr = acr;
> > +	port->serial_out(port, UART_SCR, UART_ACR);
> > +	port->serial_out(port, UART_ICR, up->acr);
> > +
> > +	/* Update the cache. */
> > +	port->rs485 = *rs485;
> > +
> > +	dev_dbg(port->dev, "ni16550_config_rs485\n");
> 
> Again, use ftrace, not dev_dbg() for stuff like this.

Will do so.

> 
> thanks,
> 
> greg k-h


Thank you.
Regards,
Je Yen Tam
Staff Software Engineer
National Instruments 
o   604-3776397
e   je.yen.tam@ni.com

Re: Re: [PATCH V2 1/2] serial/8250: Add support for NI-Serial PXI/PXIe+485 devices

[Greg KH]

On Thu, Jul 04, 2019 at 07:55:33AM +0000, Je Yen Tam wrote:
> > On Wed, Jul 03, 2019 at 12:24:35AM -0700, jeyentam wrote:
> > > Add support for NI-Serial PXIe-RS232, PXI-RS485 and PXIe-RS485 devices.
> > >
> > > Signed-off-by: jeyentam <je.yen.tam@ni.com>
> > 
> > I need a "full" name here please.
> 
> Ok, will do so.
> 
> > 
> > Also, this patch breaks the build, which is not allowed, and makes me wonder how
> > you tested it :(
> 
> I've tested my changes with the steps as follows, it did not break.
> 
> 1. Clone the Linux repo, check out the source and work on my project.
> 2. Compile the kernel source with "make j4".
> 3. Build the kernel modules with "make modules_install".
> 4. Install the kernel with "make install".
> 5. Reboot into the newly built kernel.
> 6. The kernel worked well and able to fit my needs.
> 
> Noted that my source is checked out from the latest Linux kernel 5.2.0-rc6.
> 
> Also, I checked my patch using checkpatch.pl, it did not show any errors and 
> warnings too.
> 
> May I know what do you mean by breaking the build?

If you only apply patch 1/2, the build breaks as you add new device ids
in the second patch.  At every individual patch, you can not break the
build.

thanks,

greg k-h

RE: Re: Re: [PATCH V2 1/2] serial/8250: Add support for NI-Serial PXI/PXIe+485 devices

[Je Yen Tam]

> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Thursday, July 4, 2019 4:15 PM
> To: Je Yen Tam <je.yen.tam@ni.com>
> Cc: linux-serial@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [EXTERNAL] Re: Re: [PATCH V2 1/2] serial/8250: Add support for NI-Serial
> PXI/PXIe+485 devices
> 
> On Thu, Jul 04, 2019 at 07:55:33AM +0000, Je Yen Tam wrote:
> > > On Wed, Jul 03, 2019 at 12:24:35AM -0700, jeyentam wrote:
> > > > Add support for NI-Serial PXIe-RS232, PXI-RS485 and PXIe-RS485 devices.
> > > >
> > > > Signed-off-by: jeyentam <je.yen.tam@ni.com>
> > >
> > > I need a "full" name here please.
> >
> > Ok, will do so.
> >
> > >
> > > Also, this patch breaks the build, which is not allowed, and makes me wonder
> how
> > > you tested it :(
> >
> > I've tested my changes with the steps as follows, it did not break.
> >
> > 1. Clone the Linux repo, check out the source and work on my project.
> > 2. Compile the kernel source with "make j4".
> > 3. Build the kernel modules with "make modules_install".
> > 4. Install the kernel with "make install".
> > 5. Reboot into the newly built kernel.
> > 6. The kernel worked well and able to fit my needs.
> >
> > Noted that my source is checked out from the latest Linux kernel 5.2.0-rc6.
> >
> > Also, I checked my patch using checkpatch.pl, it did not show any errors and
> > warnings too.
> >
> > May I know what do you mean by breaking the build?
> 
> If you only apply patch 1/2, the build breaks as you add new device ids
> in the second patch.  At every individual patch, you can not break the
> build.

Understood, will move the #defines into this file which needed it.

> 
> thanks,
> 
> greg k-h

Re: [PATCH V2 1/2] serial/8250: Add support for NI-Serial PXI/PXIe+485 devices

[Enrico Weigelt, metux IT consult]

On 03.07.19 17:59, Greg KH wrote:

>> That's sad, because in embedded world we often have to care about
>> code size, so making those devices optional would be of great help.
> 
> Really?  are you sure?  Try it and see what you really end up saving.

Just in 8250_pci.c alone, I could (depending on actual target) drop
about 80% LoC. Haven't tested how much the difference in image size
will really be.

But if you really like to know and would consider taking some patches
for making those things optional, I'll take some time and do the
dirty work.

By the way: would you still be opposed when these options are masked
behind CONFIG_EXPERT or CONFIG_EMBEDDED ? I can imagine this could be a
good compromise.


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287