Re: [PATCH v3 2/2] serial: 8250: Add Airoha SoC UART and HSUART support
From: Andy Shevchenko
Date: Sat Aug 08 2026 - 14:10:42 EST
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@xxxxxxxxxxxxxxx>
...
> +++ b/drivers/tty/serial/8250/8250_airoha.c
> + * Author: Benjamin Larsson <benjamin.larsson@xxxxxxxxxx>
> + * Christian Marangi <ansuelsmth@xxxxxxxxx>
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