Re: [PATCH 30/33] serial: 8250: invert serial8250_register_8250_port() CIR condition

Andy Shevchenko
2025-06-18T05:48:58Z

On Wed, Jun 11, 2025 at 12:03:16PM +0200, Jiri Slaby (SUSE) wrote:
> There is no point in a long 'if' in serial8250_register_8250_port() to
> just return ENOSPC for PORT_8250_CIR ports. Invert the condition and
> return immediately.
>
> 'gpios' variable was moved to its set location.
>
> And return ENODEV instead of ENOSPC. The latter is a leftover from the
> previous find-uart 'if'. The former makes a lot more sense in this case.

...

> + if (uart->port.type == PORT_8250_CIR) {
> + ret = -ENODEV;
> + goto unlock;
> + }

> + if (up->port.flags & UPF_FIXED_TYPE)
> + uart->port.type = up->port.type;

> + if (uart->port.type != PORT_8250_CIR) {

I admit that there tons of mysterious ways of UART initialisation, but can you
elaborate how this is not a always-true conditional?

> + if (uart_console_registered(&uart->port))
> + pm_runtime_get_sync(uart->port.dev);
> +
> + if (serial8250_isa_config != NULL)
> + serial8250_isa_config(0, &uart->port,
> + &uart->capabilities);
> +
> + serial8250_apply_quirks(uart);
> + ret = uart_add_one_port(&serial8250_reg,
> + &uart->port);
> + if (ret)
> + goto err;
> +
> + ret = uart->port.line;
> + } else {
> + dev_info(uart->port.dev,
> + "skipping CIR port at 0x%lx / 0x%llx, IRQ %d\n",
> + uart->port.iobase,
> + (unsigned long long)uart->port.mapbase,
> + uart->port.irq);
> +
> + ret = 0;
> + }

--
With Best Regards,
Andy Shevchenko