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

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

On Tue, Sep 05, 2023 at 12:06:20PM -0400, Matthew Howell wrote:
> From: Matthew Howell <matthew.howell@sealevel.com

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?

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.

> Sealevel cards.

> Link: 
> https://lore.kernel.org/all/b0b1863f-40f4-d78e-7bb7-dc4312449d9e@sealevel.com/

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;

...

> +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;
> +}

...

> +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,
> +};

...

> +#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			\
> +	}

-- 
With Best Regards,
Andy Shevchenko