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, 11:09, Andy Shevchenko wrote:
> On Tue, Oct 13, 2026 at 09:53:26AM +0200, Jiri Slaby wrote:
>> On 13. 10. 26, 9:33, Andy Shevchenko wrote:
>>> On Tue, Oct 13, 2026 at 08:48:51AM +0200, Jiri Slaby (SUSE) wrote:
> 
> ...
> 
>>>> -	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;
>>>> +	}
>>
>>> 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?
> 
> Because it's the style.

There is no hard style though. It varies across subsystems.

> Linus also told it many times

Which is neither an argument. Linus is wrong at times ("Link:" use as an 
example).

All in all, I do not insist on this at all. It's easier to read and 
follow the code for me. Not necessarily the case for others...

>> 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/
> 
> (you made me to install something I haven't heard about, it's better to use
>   `git grep` though)

rg is order of magnitude faster (searches in parallel). More importantly 
in this case, supports multiline matches. (And respects .git ignores.)

> Note excluded __free() if I get regexp correctly
> 
> $ rg -U -l '}\n+\s+struct .*\*[^)]*=' drivers/ | less
> drivers/ufs/host/ufs-qcom.c
...
> drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> 
> So, less than dozen of drivers doing wrong is not an argument, try something
> better. But thanks, I will check and address this in some of the above.

Well, it's only an example of a declaration after a previous opening 
brace "{" -- this one turned out to be easy to search for. There is a 
lot of more, of course. kernel/ included.

thanks,
-- 
js
suse labs