Messages in this thread | | | From | Je Yen Tam <> | Date | Thu, 4 Jul 2019 08:06:49 +0000 | Subject | RE: [priv] Re: [PATCH 1/2] serial/8250: Add support for NI-Serial PXI/PXIe+485 devices. |
| |
> 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".
| |