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

[Je Yen Tam <je.yen.tam@ni.com>]

> Subject: [EXTERNAL] [priv] Re: [PATCH 1/2] serial/8250: Add support for NI-Serial
> PXI/PXIe+485 devices.
> 
> On 02.07.19 05:23, jeyentam wrote:
> 
> Hi,
> 
> better writing to you personally, off-list.
> 
> > Add support for NI-Serial PXIe-RS232, PXI-RS485 and PXIe-RS485 devices.
> >
> > Signed-off-by: jeyentam <je.yen.tam@ni.com>
>                   ^^^^^^
> maybe it would be nice to have your real name here.

Ok, will do so.

> 
> > @@ -1,10 +1,10 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >  /*
> > - *  Probe module for 8250/16550-type PCI serial ports.
> > + *	Probe module for 8250/16550-type PCI serial ports.
> >   *
> > - *  Based on drivers/char/serial.c, by Linus Torvalds, Theodore Ts'o.
> > + *	Based on drivers/char/serial.c, by Linus Torvalds, Theodore Ts'o.
> >   *
> > - *  Copyright (C) 2001 Russell King, All Rights Reserved.
> > + *	Copyright (C) 2001 Russell King, All Rights Reserved.
> >   */
> >  #undef DEBUG
> >  #include <linux/module.h>
> 
> Why all these whitespace-only changes ? I doubt that anybody seriously wanna
> review that.
> 
> As I know Greg, he's doesn't like whitespace-only changes at all, unless you give him
> an really good reason for it.
> 
> > @@ -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)
> 
> I'd prefer that name change as a separate (previous) patch.
> 
> > +#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)
> 
> you should use the BIT() macro here.

Will do so.

> 
> > +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");
> 
> don't leave in such hackish debug helpers
> 
> > +	port->serial_out(port, NI16550_PCR_OFFSET, pcr);
> 
> is that indirection really necessary ? (IOW: are there separate implementations
> needed ?)
> 
> > +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;
> 
> maybe size_t for offset ?
> 
> > @@ -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,
> > +	},
> 
> We should have a config sym for that, so only those who really need the device can
> enable it.
> 
> OTOH, it would be even nicer to have this as an extra module.
> 
> 
> Nevertheless its good that you NI folks now start making your products usable on
> mainline kernel, instead of this really ridiculous and broken- by-design "nikal"/daqmx
> crap (which even sometimes introduce 0day- exploitable leaks allowing remote
> machine takeover - I've posted one @full-disclosure several month ago. One of the
> reasons why I was banned from the forums - criticism in general seems to be disliked
> there)
> 
> Over the recent years, I tried to get some specs on the cRIOs, in order to write
> proper drivers and bring them to mainline (currently these boxes might be nice for
> dumb PLC, but nothing more, and the marketing claim "linux support" is just wrong).
> But there was no way of getting anything from NI (not even after several calls with
> product management and development team). And playing around w/ LV was in no
> way any option. So I had to tell my client that cRIOs are completely unusable for him,
> and some >$1M purchases went off the table.
> (I've rarely seen any company so hostile against Linux support like NI)
> 
> 
> good luck,
> 
> --mtx
> 
> --
> Enrico Weigelt, metux IT consult
> Free software and Linux embedded engineering info@metux.net -- +49-151-
> 27565287

Please refer to the other patch, serial/8250: Add support for NI-Serial 
PXI/PXIe+485 devices as the whitespace changes in this patch was not
intended, it was a "mistake".