On Wed, Aug 30, 2023 at 11:08:28AM -0400, Matthew Howell wrote:
> From: Matthew Howell <matthew.howell@xxxxxxxxxxxx>
>
> 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 applies a new sealevel_rs485_config
s/This patch applies/Apply/
> function to configure the XR17V35X of Sealevel cards for DTR control of
> RS485 Enable.
>
> Based on feedback from the first submission I replaced the hex values with
> defines and fixed up various format issues. I couldn't find an existing
> define for the DLD register or its RS485 Polarity bit so I created a new
> define. I tried to follow the format of the other defines in serial_reg.h.
...
> +static int sealevel_rs485_config(struct uart_port *port, struct ktermios *termios,
> + struct serial_rs485 *rs485)
> +{
> + bool is_rs485 = !!(rs485->flags & SER_RS485_ENABLED);
!!() is redundant.
> + u8 __iomem *p = port->membase;
> + u8 old_lcr;
> +
> + generic_rs485_config(port, termios, rs485);
> + if (is_rs485) {
if (!is_rs485)
return 0;
...
> + // Set EFR[4]=1 to enable enhanced feature registers
> + // Set MCR to use DTR as Auto-RS485 Enable signal
> + // Store original LCR and set LCR[7]=1 to enable access to DLD register
> + // Set DLD[7]=1 for inverted RS485 Enable logic
I believe the comment style in this file is /* */.
> + }
> +
> + return 0;
> + }
...
> +static int pci_sealevel_setup(struct exar8250 *priv, struct pci_dev *pcidev,
> + struct uart_8250_port *port, int idx)
Wrong indentation.
> +{
> + int ret = pci_xr17v35x_setup(priv, pcidev, port, idx);
> +
> + if (ret)
> + return ret;
Use more robust style
int ret;
ret = pci_xr17v35x_setup(priv, pcidev, port, idx);
if (ret)
return ret;
> + port->port.rs485_config = sealevel_rs485_config;
> +
> + return ret;
return 0;
> +}
...
> +static const struct exar8250_board pbn_sealevel_16 = {
> + .num_ports = 16,
> + .setup = pci_sealevel_setup,
TABs vs. spaces, please fix.
> + .exit = pci_xr17v35x_exit,
> +};
...
> @@ -885,6 +953,7 @@ static const struct pci_device_id exar_pci_tbl[] = {
> EXAR_DEVICE(COMMTECH, 4224PCI335, pbn_fastcom335_4),
> EXAR_DEVICE(COMMTECH, 2324PCI335, pbn_fastcom335_4),
> EXAR_DEVICE(COMMTECH, 2328PCI335, pbn_fastcom335_8),
> +
> { 0, }
> };
> MODULE_DEVICE_TABLE(pci, exar_pci_tbl);
Stray change.
...
> --- a/include/uapi/linux/serial_reg.h
> +++ b/include/uapi/linux/serial_reg.h
Really?
Please, localize this to the driver.
It may be the same reason to do so as in 7e12357ed64a ("serial: exar:
Move register defines from uapi header to consumer site")
> #define UART_DLL 0 /* Out: Divisor Latch Low */
> #define UART_DLM 1 /* Out: Divisor Latch High */
> +#define UART_DLD 2 /* Divisor Fractional */
> +#define UART_DLD_485_POLARITY 0x80 /* RS-485 Enable Signal Polarity */
> #define UART_DIV_MAX 0xFFFF /* Max divisor value */
--
With Best Regards,
Andy Shevchenko
