lkml.org 
[lkml]   [2019]   [Jul]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRE: [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".
\
 
 \ /
  Last update: 2019-07-04 10:07    [W:0.056 / U:7.668 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site