| Message ID | b0b1863f-40f4-d78e-7bb7-dc4312449d9e@sealevel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [V3,1/2] serial: exar: Revert "serial: exar: Add support for Sealevel 7xxxC serial cards" | expand |
On Thu, Aug 31, 2023 at 03:48:08PM -0400, Matthew Howell wrote: > From: Matthew Howell <matthew.howell@sealevel.com > > Sealevel XR17V35X based cards utilize DTR to control RS-485 Enable, but > the current implementation in 8250_exar uses RTS for the auto-RS485-Enable > mode of the XR17V35X UARTs. This patch implements sealevel_rs485_config to Please, read Submitting Patches documentation, in particular find there the paragraph that matches to "This patch". With that, amend commit message accordingly. We refer to the functions as func() (note the parentheses). > configure the XR17V35X for DTR control of RS485 Enable and assigns it to Your lines have trailing whitespaces, please get rid of them. > Sealevel cards in pci_sealevel_setup. > Fixed defines and various format issues from previous submissions. What does this mean? If it's a changelog, use the correct place for it (see below). > > Link: > https://lore.kernel.org/linux-serial/b2a721-227-14ef-75eb-36244ba2942@sealevel.com Tags must occupy a single line: a single line per each tag. > Tag block must have no blank lines. Most of these is described in the above mentioned documentation. > Signed-off-by: Matthew Howell <matthew.howell@sealevel.com> > --- Here you add comments and/or changelog. > diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c > index 3886f78ecbbf..6b28f5a3df17 100644 ... > +#define UART_EXAR_DLD 0x02 /* Divisor Fractional */ > +#define UART_EXAR_DLD_485_POLARITY 0x80 /* RS-485 Enable Signal Polarity */ Mixed TABs and spaces in a wrong order, usually we use only TABs in such cases. ... > +static int sealevel_rs485_config(struct uart_port *port, struct ktermios *termios, > + struct serial_rs485 *rs485) > +{ > + u8 __iomem *p = port->membase; > + u8 old_lcr; > + > + generic_rs485_config(port, termios, rs485); > + if (rs485->flags & SER_RS485_ENABLED) { Seems you haven't seen / ignored my comments. Please, read my previous reply. > + /* Set EFR[4]=1 to enable enhanced feature registers */ > + writeb(readb(p + UART_XR_EFR) | UART_EFR_ECB, p + UART_XR_EFR); > + > + /* Set MCR to use DTR as Auto-RS485 Enable signal */ > + writeb(UART_MCR_OUT1, p + UART_MCR); > + > + /* Store original LCR and set LCR[7]=1 to enable access to DLD register */ > + old_lcr = readb(p + UART_LCR); > + writeb(old_lcr | UART_LCR_DLAB, p + UART_LCR); > + > + /* Set DLD[7]=1 for inverted RS485 Enable logic */ > + writeb(readb(p + UART_EXAR_DLD) | UART_EXAR_DLD_485_POLARITY, p + UART_EXAR_DLD); > + > + writeb(old_lcr, p + UART_LCR); > + } > + > + return 0; > + }
On Thu, 31 Aug 2023, Andy Shevchenko wrote: > On Thu, Aug 31, 2023 at 03:48:08PM -0400, Matthew Howell wrote: > > From: Matthew Howell <matthew.howell@sealevel.com > > > > Sealevel XR17V35X based cards utilize DTR to control RS-485 Enable, but > > the current implementation in 8250_exar uses RTS for the auto-RS485-Enable > > mode of the XR17V35X UARTs. This patch implements sealevel_rs485_config to > > Please, read Submitting Patches documentation, in particular find there > the paragraph that matches to "This patch". With that, amend commit message > accordingly. > > We refer to the functions as func() (note the parentheses). I did read it but it was not clear that the parentheses are part of the 'rules'. > > configure the XR17V35X for DTR control of RS485 Enable and assigns it to > > Your lines have trailing whitespaces, please get rid of them. > > > Sealevel cards in pci_sealevel_setup. > > > Fixed defines and various format issues from previous submissions. > > What does this mean? If it's a changelog, use the correct place for it > (see below). Sorry, I did not realize. I found that section of the document unclear on first pass. > > > > Link: > > https://lore.kernel.org/linux-serial/b2a721-227-14ef-75eb-36244ba2942@sealevel.com > > Tags must occupy a single line: a single line per each tag. > > > > > Tag block must have no blank lines. > > Most of these is described in the above mentioned documentation. Sorry, I had missed that tags are exempt from the normal character per line limit. > > Signed-off-by: Matthew Howell <matthew.howell@sealevel.com> > > --- > > Here you add comments and/or changelog. > > > diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c > > index 3886f78ecbbf..6b28f5a3df17 100644 > > ... > > > +#define UART_EXAR_DLD 0x02 /* Divisor Fractional */ > > +#define UART_EXAR_DLD_485_POLARITY 0x80 /* RS-485 Enable Signal Polarity */ > > Mixed TABs and spaces in a wrong order, usually we use only TABs in such cases. > > ... > > > +static int sealevel_rs485_config(struct uart_port *port, struct ktermios *termios, > > + struct serial_rs485 *rs485) > > +{ > > + u8 __iomem *p = port->membase; > > + u8 old_lcr; > > + > > + generic_rs485_config(port, termios, rs485); > > > + if (rs485->flags & SER_RS485_ENABLED) { > > Seems you haven't seen / ignored my comments. Please, read my previous reply. You said !!() is redundant and I have removed !!(). Previous feedback also suggested that is_rs485 is not needed, but I had reverted both changes as I initially thought it was the cause of a breakage. However, further testing found the breakage was unrelated to this patch series. Therefore, I attempted to address both suggestions by removing is_rs485 and !!() in this submission. I did not ignore your comments and I do not appreciate these insenuations. I have made changes based on every one of your comments in the previous submission, I just did not always address the comment in exactly you suggested. Please, clarify how this fails to address your comments and I will be happy to correct it in the next submission. > > + /* Set EFR[4]=1 to enable enhanced feature registers */ > > + writeb(readb(p + UART_XR_EFR) | UART_EFR_ECB, p + UART_XR_EFR); > > + > > + /* Set MCR to use DTR as Auto-RS485 Enable signal */ > > + writeb(UART_MCR_OUT1, p + UART_MCR); > > + > > + /* Store original LCR and set LCR[7]=1 to enable access to DLD register */ > > + old_lcr = readb(p + UART_LCR); > > + writeb(old_lcr | UART_LCR_DLAB, p + UART_LCR); > > + > > + /* Set DLD[7]=1 for inverted RS485 Enable logic */ > > + writeb(readb(p + UART_EXAR_DLD) | UART_EXAR_DLD_485_POLARITY, p + UART_EXAR_DLD); > > + > > + writeb(old_lcr, p + UART_LCR); > > + } > > + > > + return 0; > > + } > > -- > With Best Regards, > Andy Shevchenko > > >
On Fri, Sep 01, 2023 at 10:26:01AM -0400, Matthew Howell wrote: > On Thu, 31 Aug 2023, Andy Shevchenko wrote: > > On Thu, Aug 31, 2023 at 03:48:08PM -0400, Matthew Howell wrote: ... > > > + if (rs485->flags & SER_RS485_ENABLED) { > > > > Seems you haven't seen / ignored my comments. Please, read my previous reply. > > You said !!() is redundant and I have removed !!(). Previous feedback also > suggested that is_rs485 is not needed, but I had reverted both changes as > I initially thought it was the cause of a breakage. However, further testing > found the breakage was unrelated to this patch series. Therefore, I > attempted to address both suggestions by removing is_rs485 and !!() in > this submission. > > I did not ignore your comments and I do not appreciate these insenuations. > I have made changes based on every one of your comments in the previous > submission, I just did not always address the comment in exactly you > suggested. > > Please, clarify how this fails to address your comments and I will be > happy to correct it in the next submission. I believe there is a misunderstanding in what I meant. My previous comment was to change if (is_...) { ... } return 0; to if (!is_...) return 0; ... return 0; which is missing here. But as you said the entire "if" is redundant, so drop it. > > > + }
diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c index 3886f78ecbbf..6b28f5a3df17 100644 --- a/drivers/tty/serial/8250/8250_exar.c +++ b/drivers/tty/serial/8250/8250_exar.c @@ -78,6 +78,9 @@ #define UART_EXAR_RS485_DLY(x) ((x) << 4) +#define UART_EXAR_DLD 0x02 /* Divisor Fractional */ +#define UART_EXAR_DLD_485_POLARITY 0x80 /* RS-485 Enable Signal Polarity */ + /* * IOT2040 MPIO wiring semantics: * @@ -439,6 +442,34 @@ static int generic_rs485_config(struct uart_port *port, struct ktermios *termios return 0; } +static int sealevel_rs485_config(struct uart_port *port, struct ktermios *termios, + struct serial_rs485 *rs485) +{ + u8 __iomem *p = port->membase; + u8 old_lcr; + + generic_rs485_config(port, termios, rs485); + + if (rs485->flags & SER_RS485_ENABLED) { + /* Set EFR[4]=1 to enable enhanced feature registers */ + writeb(readb(p + UART_XR_EFR) | UART_EFR_ECB, p + UART_XR_EFR); + + /* Set MCR to use DTR as Auto-RS485 Enable signal */ + writeb(UART_MCR_OUT1, p + UART_MCR); + + /* Store original LCR and set LCR[7]=1 to enable access to DLD register */ + old_lcr = readb(p + UART_LCR); + writeb(old_lcr | UART_LCR_DLAB, p + UART_LCR); + + /* Set DLD[7]=1 for inverted RS485 Enable logic */ + writeb(readb(p + UART_EXAR_DLD) | UART_EXAR_DLD_485_POLARITY, p + UART_EXAR_DLD); + + writeb(old_lcr, p + UART_LCR); + } + + return 0; + } + static const struct serial_rs485 generic_rs485_supported = { .flags = SER_RS485_ENABLED, }; @@ -744,6 +775,16 @@ static int __maybe_unused exar_resume(struct device *dev) return 0; } +static int pci_sealevel_setup(struct exar8250 *priv, struct pci_dev *pcidev, + struct uart_8250_port *port, int idx) +{ + int ret = pci_xr17v35x_setup(priv, pcidev, port, idx); + + port->port.rs485_config = sealevel_rs485_config; + + return ret; +} + static SIMPLE_DEV_PM_OPS(exar_pci_pm, exar_suspend, exar_resume); static const struct exar8250_board pbn_fastcom335_2 = { @@ -809,6 +850,17 @@ static const struct exar8250_board pbn_exar_XR17V8358 = { .exit = pci_xr17v35x_exit, }; +static const struct exar8250_board pbn_sealevel = { + .setup = pci_sealevel_setup, + .exit = pci_xr17v35x_exit, +}; + +static const struct exar8250_board pbn_sealevel_16 = { + .num_ports = 16, + .setup = pci_sealevel_setup, + .exit = pci_xr17v35x_exit, +}; + #define CONNECT_DEVICE(devid, sdevid, bd) { \ PCI_DEVICE_SUB( \ PCI_VENDOR_ID_EXAR, \ @@ -838,6 +890,15 @@ static const struct exar8250_board pbn_exar_XR17V8358 = { (kernel_ulong_t)&bd \ } +#define SEALEVEL_DEVICE(devid, bd) { \ + PCI_DEVICE_SUB( \ + PCI_VENDOR_ID_EXAR, \ + PCI_DEVICE_ID_EXAR_##devid, \ + PCI_VENDOR_ID_SEALEVEL, \ + PCI_ANY_ID), 0, 0, \ + (kernel_ulong_t)&bd \ + } + static const struct pci_device_id exar_pci_tbl[] = { EXAR_DEVICE(ACCESSIO, COM_2S, pbn_exar_XR17C15x), EXAR_DEVICE(ACCESSIO, COM_4S, pbn_exar_XR17C15x), @@ -860,6 +921,12 @@ static const struct pci_device_id exar_pci_tbl[] = { CONNECT_DEVICE(XR17C154, UART_4_485, pbn_connect), CONNECT_DEVICE(XR17C158, UART_8_485, pbn_connect), + SEALEVEL_DEVICE(XR17V352, pbn_sealevel), + SEALEVEL_DEVICE(XR17V354, pbn_sealevel), + SEALEVEL_DEVICE(XR17V358, pbn_sealevel), + SEALEVEL_DEVICE(XR17V4358, pbn_sealevel_16), + SEALEVEL_DEVICE(XR17V8358, pbn_sealevel_16), + IBM_DEVICE(XR17C152, SATURN_SERIAL_ONE_PORT, pbn_exar_ibm_saturn), /* USRobotics USR298x-OEM PCI Modems */