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

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

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

-- 
With Best Regards,
Andy Shevchenko