Re: [PATCH v3 04/10] serial: uniformize serial port I/O infos display

[Geert Uytterhoeven]

Hi Hugo,

On Thu, 21 May 2026 at 20:23, Hugo Villeneuve <hugo@xxxxxxxxxxx> wrote:
> From: Hugo Villeneuve <hvilleneuve@xxxxxxxxxxxx>
>
> Uniformize serial port I/O infos display from three different functions
> that display mostly the same information, but with some variations, by
> adding a common function.
>
> This make use of new functions uart_iotype_mmio() and
> uart_iotype_legacy_io() to simplify and improve code readability.
> This will prevent displaying irrelevant information for future IO types
> (ex: UPIO_BUS), while also addressing the (eventually) invalid check for
> "iotype >= UPIO_MEM".
>
> This also allows us to remove the confusing cast to (unsigned long long)
> for iobase which is defined as an unsigned long, and use %pa to display the
> mapbase pointer, as it is done in earlycon_print_info().
>
> Replace snprintf with more robust scnprintf so we could perhaps one day get
> rid of snprintf() entirely.
>
> Signed-off-by: Hugo Villeneuve <hvilleneuve@xxxxxxxxxxxx>

Thanks for your patch, which is now commit 86305190f307c05b ("serial:
uniformize serial port I/O infos display") in tty/tty-next.

> Maybe the width information could be dropped entirely?

Perhaps...

> --- a/drivers/tty/serial/earlycon.c
> +++ b/drivers/tty/serial/earlycon.c
> @@ -75,19 +75,12 @@ static void __init earlycon_print_info(struct earlycon_device *device)
>  {
>         struct console *earlycon = device->con;
>         struct uart_port *port = &device->port;
> +       char ioinfos[64];
>
> -       if (port->iotype == UPIO_MEM || port->iotype == UPIO_MEM16 ||
> -           port->iotype == UPIO_MEM32 || port->iotype == UPIO_MEM32BE)
> -               pr_info("%s%d at MMIO%s %pa (options '%s')\n",
> -                       earlycon->name, earlycon->index,
> -                       (port->iotype == UPIO_MEM) ? "" :
> -                       (port->iotype == UPIO_MEM16) ? "16" :
> -                       (port->iotype == UPIO_MEM32) ? "32" : "32be",

This handled 32 vs 32be...

> -                       &port->mapbase, device->options);
> -       else
> -               pr_info("%s%d at I/O port 0x%lx (options '%s')\n",
> -                       earlycon->name, earlycon->index,
> -                       port->iobase, device->options);
> +       uart_get_ioinfos(port, ioinfos, sizeof(ioinfos));
> +
> +       pr_info("%s%d%s (options '%s')\n", earlycon->name, earlycon->index,
> +               ioinfos, device->options);
>  }
>
>  static int __init parse_options(struct earlycon_device *device, char *options)

> @@ -2476,38 +2473,47 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport)
>  }
>  EXPORT_SYMBOL(uart_resume_port);
>
> +static const char *uart_get_mmio_width(struct uart_port *port)
> +{
> +       switch (port->iotype) {
> +       case UPIO_MEM16:
> +               return "16";
> +       case UPIO_MEM32:

Missing

    return "32";

> +       case UPIO_MEM32BE:
> +               return "32be";
> +       case UPIO_AU:
> +       case UPIO_MEM:
> +       default:
> +               return "";
> +       }
> +}

>  static inline void
>  uart_report_port(struct uart_driver *drv, struct uart_port *port)
>  {
> -       char address[64];
> +       char ioinfos[64];
>
> -       switch (port->iotype) {
> -       case UPIO_PORT:
> -               snprintf(address, sizeof(address), "I/O 0x%lx", port->iobase);
> -               break;
> -       case UPIO_HUB6:
> -               snprintf(address, sizeof(address),
> -                        "I/O 0x%lx offset 0x%x", port->iobase, port->hub6);
> -               break;
> -       case UPIO_MEM:
> -       case UPIO_MEM16:
> -       case UPIO_MEM32:
> -       case UPIO_MEM32BE:
> -       case UPIO_AU:
> -       case UPIO_TSI:
> -               snprintf(address, sizeof(address),
> -                        "MMIO 0x%llx", (unsigned long long)port->mapbase);
> -               break;
> -       default:
> -               strscpy(address, "*unknown*", sizeof(address));
> -               break;
> -       }
> +       uart_get_ioinfos(port, ioinfos, sizeof(ioinfos));
>
> -       pr_info("%s%s%s at %s (irq = %u, base_baud = %u) is a %s\n",
> -              port->dev ? dev_name(port->dev) : "",
> -              port->dev ? ": " : "",
> -              port->name,
> -              address, port->irq, port->uartclk / 16, uart_type(port));
> +       pr_info("%s%s%s%s (irq = %u, base_baud = %u) is a %s\n",
> +               port->dev ? dev_name(port->dev) : "",
> +               port->dev ? ": " : "",
> +               port->name, ioinfos, port->irq, port->uartclk / 16, uart_type(port));
>
>         /* The magic multiplier feature is a bit obscure, so report it too.  */
>         if (port->flags & UPF_MAGIC_MULTIPLIER)

I am also wondering if the ", base_baud = %u" should be dropped if
port->uartclk is zero.  But that is a pre-existing issue.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds