Re: [PATCH V2 2/2] serial: exar: Add RS-485 support for Sealevel XR17V35X based cards

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Wed, Aug 30, 2023 at 11:08:28AM -0400, Matthew Howell wrote:
> From: Matthew Howell <matthew.howell@sealevel.com>
> 
> 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