On Wed, 6 Sep 2023, Andy Shevchenko wrote: > On Tue, Sep 05, 2023 at 12:06:20PM -0400, Matthew Howell wrote: > > From: Matthew Howell <matthew.howell@xxxxxxxxxxxx > > Okay, I started reviewing it and it has all the same issues that I pointed > out previously. Are you sure you sent a _new_ version? I did send a new version. > For your convenience I left all comments here. > > > > > Sealevel XR1735X based cards utilize DTR to control RS-485 Enable, but the > > current implementation of 8250_exar uses RTS for the auto-RS485-Enable > > mode of the XR17V35X UARTs. This patch implements DTR Auto-RS485 on > > All four above and two below (Link: and supposed to be a blank line) > have a trailing space. Which editor do you use? You probably need to configure > it to avoid this. I use the the default editor in alpine and I had configured alpine per the email client configuration reccomendations in the documentation. I believe this happened because alpine automatically adds a new line to line wrap at the character limit, but since links are exempt from this rule I have to manually undo this and I made a mistake in that process. I will try composing in a different editor and then file-read it into alpine instead of using the built in editor to see if that helps avoid the issue. > > Sealevel cards. > > > Link: > > https://lore.kernel.org/all/b0b1863f-40f4-d78e-7bb7-dc4312449d9e@xxxxxxxxxxxx/ > > Should be a single line. > > > > > No blank lines in the tag block. > > ... > > > + generic_rs485_config(port, termios, rs485); > > > + if (rs485->flags & SER_RS485_ENABLED) { > > What I meant is to have > > if (!)rs485->flags & SER_RS485_ENABLED)) > return 0; > > here, which allows you to reduce indentation level in the below block. > > > + /* 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; I see where you are coming from now, but I find that slightly less clear than having the 'main action' within the conditional statement. And since the code is not heavily indented I don't see much benefit of removing the indent. > ... > > > +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; > > This is incorrect in a way that we do not assign return values in case it is > an error path. This will be prone to mistakes on the caller side. > > int ret; > > ret = ... > if (ret) > return ret; > > ... > > return 0; > > > + return ret; > > +} > ... Ok, will change for next submission. > > +static const struct exar8250_board pbn_sealevel_16 = { > > + .num_ports = 16, > > > + .setup = pci_sealevel_setup, > > Here is the indentation issue. Use leading TABs. > > > + .exit = pci_xr17v35x_exit, > > +}; Sorry, I had missed that one. > ... > > > +#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, \ > > Seems trailing \ is indented wrongly, missing TAB? > > > + (kernel_ulong_t)&bd \ > > + } > Ah, I see. In my editor none of the trailing \ lined up so I thought that there was some sotr of space/tab rule with trailing slashes I wasn't aware of, so I just used the same amount of spaces and tabs as the surrounding ..._DEVICE() macros. However, I now see that I should have had it set to tab size 8 instead of 4. Will fix in the next submission. > -- > With Best Regards, > Andy Shevchenko > > >
