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

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

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