Re: [PATCH v2 6/6] serial: 8250: make port operations work on struct uart_8250_port

On 13. 10. 26, 9:33, Andy Shevchenko wrote:

On Tue, Oct 13, 2026 at 08:48:51AM +0200, Jiri Slaby (SUSE) wrote:

quoted

serial8250_register_8250_port() returns a line number and the other
port operations (serial8250_unregister_port(), serial8250_suspend_port(),
serial8250_resume_port()) accept it. Many drivers then convert the line
back to the port using serial8250_get_port() anyway.

Switch the interface to struct uart_8250_port:
* serial8250_register_8250_port() now returns the registered port, or
an ERR_PTR() on failure.
* serial8250_unregister_port(), serial8250_suspend_port() and
serial8250_resume_port() accept the port.

Convert all users accordingly. Drivers store the port pointer instead
of the line number. Several drivers kept a private struct only to hold
the line (airoha, dfl, em, fsl, ioc3); these are dropped and the port
is stored as drvdata directly.

This also fixes two latent issues:
* For a CIR port, serial8250_register_8250_port() used to return 0.
Callers stored this as the line, so a later unregister/suspend/resume
operated on ttyS0. Now the CIR port itself is returned.
* 8250_acorn checked ports[i] > 0 on removal, so a port registered as
line 0 was never unregistered.

There are no more users of serial8250_get_port() outside of the 8250
core, so drop its EXPORT.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>

quoted

Cc: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>

Why not having Ilpo's Cc in the below list?

He commented on the v1. Now you will be listed here as well.

quoted

static int aspeed_vuart_probe(struct platform_device *pdev)

quoted

  	if (of_property_read_bool(np, "auto-flow-control"))
  		port.capabilities |= UART_CAP_AFE;
  
-	rc = serial8250_register_8250_port(&port);
-	if (rc < 0)
+	struct uart_8250_port *uport = serial8250_register_8250_port(&port);
+	if (IS_ERR(uport)) {
+		rc = PTR_ERR(uport);
  		goto err_sysfs_remove;
+	}

You can get rid of these {} by using PTR_ERR_OR_ZERO().

Also do not define
variables in the middle of the code (it's only allowed for auto cleanup
variables and for-loop iterators).

Why, actually? First, it is used in the kernel quite a few times already [1]. Second, while it is sort of c++ism (and I like to use modern c++), it makes the life of the variable a lot more obvious.

[1] eg. see output of: rg -U -C 5 '}\n+\s+struct .*\*.*=' drivers/

thanks,
-- 
js
suse labs