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

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

On Fri, Jul 24, 2026 at 08:30:06PM +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.

I tried not to clash with Ilpo's review (all he said is still applicable here).

...

> +#define XINDIV_CLOCK		20000000


20 * HZ_PER_MHZ

(should include units.h)

...

> +static const struct airoha_8250_clk_div_info airoha_clk_div_info[] = {
> +	{ .div = 10, .mask = BIT(2) },
> +	{ .div = 4, .mask = BIT(1) },
> +	{ .div = 2, .mask = BIT(0) },
> +};

The (reversed) array of integers should suffice.

static const unsigned int airoha_clk_divs[] = { 2, 4, 10 };

See below for more.

...

> +static void airoha_set_divisor(struct uart_port *port, unsigned int baud,
> +			       unsigned int quot, unsigned int quot_frac)
> +{
> +	const struct airoha_8250_clk_div_info *clk_div_info;
> +	struct uart_8250_port *up = up_to_u8250p(port);
> +	u32 xindiv_clk;
> +	u64 xyd_x, nom;

> +	int i;

Not used outside of the loop, define it there.

> +	/* Set DLAB to access the baud rate divider registers (BRDH, BRDL) */
> +	serial_port_out(port, UART_LCR, up->lcr | UART_LCR_DLAB);
> +
> +	/* Set baud rate calculation defaults (BRDIV ([BRDH,BRDL]) to 1) */
> +	serial_port_out(port, UART_AIROHA_BRDL, UART_BRDL_20M);
> +	serial_port_out(port, UART_AIROHA_BRDH, UART_BRDH_20M);

The above three calls repeat serial8250_do_set_divisor(), don't they?

	/* Set baud rate calculation defaults (BRDIV ([BRDH,BRDL]) to 1) */
	serial8250_do_set_divisor(port, baud, 1);

> +	/*
> +	 * Calculate XYD_x and XINCLKDR register by searching
> +	 * through a table of crystal_clock divisors.
> +	 */
> +	nom = baud * XYD_Y;

> +	for (i = 0 ; i < ARRAY_SIZE(airoha_clk_div_info) ; i++) {
> +		clk_div_info = &airoha_clk_div_info[i];
> +		xindiv_clk = XINDIV_CLOCK / clk_div_info->div;

	for (unsigned int i = ARRAY_SIZE(airoha_clk_div_info) - 1; i >= 0; i--) {
		xindiv_clk = XINDIV_CLOCK / BIT(i);

Also variant (but may be a little bit confusing)

	for (unsigned int i = ARRAY_SIZE(airoha_clk_div_info); i; i--) {
		xindiv_clk = XINDIV_CLOCK / BIT(i - 1);

> +		xyd_x = div_u64(nom, xindiv_clk) * 16;
> +
> +		/* 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;
> +	}
> +
> +	serial_port_out(port, UART_AIROHA_XINCLKDR, clk_div_info->mask);
> +	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);

Hmm... do you really need this? It doesn't seem required (at least for many
other 8250 compatible devices).

> +}

...

> +static int airoha_8250_probe(struct platform_device *pdev)
> +{
> +	struct uart_8250_port uart = { };
> +	struct device *dev = &pdev->dev;
> +	struct airoha_8250_priv *priv;
> +	struct resource *res;
> +	int ret;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return dev_err_probe(dev, -EINVAL, "invalid address\n");
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	uart.port.dev = dev;
> +	if (device_is_compatible(dev, "airoha,an7581-hsuart"))
> +		uart.port.type = UART_PORT_AIROHA_HS;
> +	else
> +		uart.port.type = UART_PORT_AIROHA;

> +	uart.port.flags = UPF_BOOT_AUTOCONF | UPF_FIXED_PORT |
> +			  UPF_FIXED_TYPE | UPF_IOREMAP;

> +	uart.port.set_divisor = airoha_set_divisor;
> +	uart.port.get_divisor = airoha_get_divisor;
> +	uart.port.mapbase = res->start;
> +	uart.port.mapsize = resource_size(res);

> +	ret = uart_read_and_validate_port_properties(&uart.port);

I'm not sure about 'validate' as only one or two drivers use it. Just double
check that this is indeed what you want (read the kernel-doc for that function
carefully, it's not that trivial, unfortunately).

> +	if (ret)
> +		return ret;
> +
> +	ret = serial8250_register_8250_port(&uart);
> +	if (ret < 0)
> +		return ret;
> +
> +	priv->line = ret;
> +	platform_set_drvdata(pdev, priv);
> +
> +	return 0;
> +}

...

> +static const struct of_device_id airoha_8250_dt_ids[] = {
> +	{ .compatible = "airoha,en7523-uart" },
> +	{ .compatible = "airoha,an7581-hsuart" },
> +	{ },

No comma in the terminator.

> +};

...

> +static struct platform_driver airoha_8250_driver = {
> +	.driver = {
> +		.name = "8250_airoha",
> +		.of_match_table = airoha_8250_dt_ids,
> +	},
> +	.probe = airoha_8250_probe,
> +	.remove = airoha_8250_remove,
> +};

> +

Drop this redundant blank line.

> +module_platform_driver(airoha_8250_driver);

-- 
With Best Regards,
Andy Shevchenko