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

[Jiri Slaby <jirislaby@kernel.org>]

On 13. 10. 26, 9:33, Andy Shevchenko wrote:
> On Tue, Oct 13, 2026 at 08:48:51AM +0200, Jiri Slaby (SUSE) wrote:
>> 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>
> 
>> 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.

>> static int aspeed_vuart_probe(struct platform_device *pdev)
> 
>>   	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