Re: [PATCH v3 12/12] serial: 8250_dwlib: Support for 9th bit multipoint addressing

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

On Mon, Apr 11, 2022 at 11:33:21AM +0300, Ilpo Järvinen wrote:
> This change adds 9th bit multipoint addressing mode for DW UART using
> the new ioctls introduced in the previous change. 9th bit addressing
> can be used only when HW RS485 is available.
> 
> Updating RAR (receive address register) is bit tricky because
> busy indication is not be available when DW UART is strictly
> 16550 compatible, which is the case with the hardware I was
> testing with. RAR should not be updated while receive is in
> progress which is now achieved by deasserting RE and waiting
> for one frame (in case rx would be in progress, the driver
> seems to have no way of knowing it w/o busy indication).

...

> +static void dw8250_update_rar(struct uart_port *p, u32 addr)
> +{
> +	u32 re_en = dw8250_readl_ext(p, DW_UART_RE_EN);
> +
> +	/*
> +	 * RAR shouldn't be changed while receiving. Thus, de-assert RE_EN
> +	 * if asserted and wait.
> +	 */
> +	if (re_en)
> +		dw8250_writel_ext(p, DW_UART_RE_EN, 0);
> +	dw8250_wait_re_deassert(p);
> +	dw8250_writel_ext(p, DW_UART_RAR, addr);
> +	if (re_en)
> +		dw8250_writel_ext(p, DW_UART_RE_EN, re_en);

I would write it

	u32 re_en;

	/*
	 * RAR shouldn't be changed while receiving. Thus, de-assert RE_EN
	 * if asserted and wait.
	 */
	re_en = dw8250_readl_ext(p, DW_UART_RE_EN);
	if (re_en) {
		dw8250_writel_ext(p, DW_UART_RE_EN, 0);
		dw8250_wait_re_deassert(p);
		dw8250_writel_ext(p, DW_UART_RAR, addr);
		dw8250_writel_ext(p, DW_UART_RE_EN, re_en);
	} else {
		dw8250_wait_re_deassert(p);
		dw8250_writel_ext(p, DW_UART_RAR, addr);
	}

or something similar with extracting these two lines into another function.

But it's up to you, it's just pure style thingy.

> +}

...

> +		dw8250_writel_ext(p, DW_UART_TAR, addr->addr & 0xff);

GENMASK() ?
Perhaps even a separate definition?

...

> +		addr->addr = dw8250_readl_ext(p, DW_UART_TAR) & 0xff;

Ditto.

...

> +			addr->addr = dw8250_readl_ext(p, DW_UART_RAR) & 0xff;

Ditto.

-- 
With Best Regards,
Andy Shevchenko