Re: [PATCH v3 2/2] serial: 8250: Add Airoha SoC UART and HSUART support

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

On Sat, Aug 08, 2026 at 12:20:40AM +0200, Christian Marangi wrote:
> Add support for Airoha AN7523 UART and AN7581 HSUART.
> 
> These implement a standard 16550 UART with only some custom logic
> for baud rate handling.

Some cosmetic issues below.
With that being addressed,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

...


> +++ b/drivers/tty/serial/8250/8250_airoha.c


> + * Author: Benjamin Larsson <benjamin.larsson@genexis.eu>
> + *	   Christian Marangi <ansuelsmth@gmail.com>

Authors:

...

> +#include <linux/array_size.h>
> +#include <linux/bits.h>
> +#include <linux/bitfield.h>

+ errno.h

> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/platform_device.h>
> +#include <linux/serial_8250.h>

> +#include <linux/serial_core.h>
> +#include <linux/serial_reg.h>

These two are kinda guaranteed by serial_8250.h. At least IIRC some other 8250
drivers rely on this and it sounds logical.

+ types.h // uXX

> +#include <linux/units.h>

...

> +#define UART_BRD_20M		0x1

Perhaps 0x0001 to show that this is 16-bit value?

...

> +static unsigned int airoha_get_divisor(struct uart_port *port,
> +				       unsigned int baud,
> +				       unsigned int *frac)
> +{
> +	/* Hardware always uses BRDIV = 1. */
> +	*frac = 0;
> +
> +	return 1;

Shouldn't this be the same as UART_BRD_20M? I mean return a defined constant.

> +}

...

> +static void airoha_set_divisor(struct uart_port *port, unsigned int baud,
> +			       unsigned int quot, unsigned int quot_frac)
> +{
> +	struct uart_8250_port *up = up_to_u8250p(port);
> +	u32 xindiv_clk;
> +	u64 xyd_x, nom;
> +	int div_bit;
> +
> +	/* Set baud rate calculation defaults (BRDIV ([BRDH,BRDL]) to 1) */

Inner parentheses are not needed.

> +	serial8250_do_set_divisor(port, baud, UART_BRD_20M);
> +
> +	/*
> +	 * Calculate XYD_x and XINCLKDR register by searching
> +	 * through a table of crystal_clock divisors.
> +	 */
> +	nom = (u64)baud * XYD_Y;

> +	for (div_bit = ARRAY_SIZE(airoha_clk_divs) - 1;
> +	     div_bit >= 0; div_bit--) {

It's okay to place these on a single line.

> +		unsigned int div = airoha_clk_divs[div_bit];
> +
> +		xindiv_clk = XINDIV_CLOCK / div;
> +		xyd_x = div_u64(nom * 16, xindiv_clk);
> +
> +		/* For the HSUART xyd_x needs to be scaled by a factor of 2 */
> +		if (port->type == UART_PORT_AIROHA_HS)
> +			xyd_x /= 2;
> +
> +		if (xyd_x < XYD_Y)
> +			break;
> +	}
> +
> +	/* Couldn't find a valid xyd_x */
> +	if (div_bit < 0) {
> +		dev_err(port->dev, "failed to find suitable clock divisor for baud %u\n",
> +			baud);
> +		return;
> +	}
> +
> +	serial_port_out(port, UART_AIROHA_XINCLKDR, BIT(div_bit));
> +	serial_port_out(port, UART_AIROHA_XYD,
> +			FIELD_PREP(UART_AIROHA_XYD_X, xyd_x) |
> +			FIELD_PREP(UART_AIROHA_XYD_Y, XYD_Y));
> +
> +	/* Restore normal register access. */
> +	serial_port_out(port, UART_LCR, up->lcr);
> +}

-- 
With Best Regards,
Andy Shevchenko