Re: [PATCH v2 1/5] serial: 8250: extract serial8250_init_mctrl()

[Jiri Slaby <jirislaby@kernel.org>]

On 24. 06. 25, 13:15, Ilpo Järvinen wrote:
> On Tue, 24 Jun 2025, Ilpo Järvinen wrote:
> 
>> On Tue, 24 Jun 2025, Jiri Slaby (SUSE) wrote:
>>
>>> After commit 795158691cc0 ("serial: 8250: extract
>>> serial8250_initialize()"), split serial8250_initialize() even more --
>>> the mctrl part of this code can be separated into
>>> serial8250_init_mctrl() -- done now.
>>>
>>> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
>>> Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>>
>> Heh, I didn't even realize I was suggesting this :-D but it's good
>> nonetheless.
>>
>> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>>
>>> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
>>> ---
>>> [v2]
>>> * use port-> directly.
>>> * do not remove curly braces.
>>> Both rebase errors -- noticed by Andy.
>>> ---
>>>   drivers/tty/serial/8250/8250_port.c | 21 +++++++++++++--------
>>>   1 file changed, 13 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
>>> index 48c30e158cb8..0f85a2f292fc 100644
>>> --- a/drivers/tty/serial/8250/8250_port.c
>>> +++ b/drivers/tty/serial/8250/8250_port.c
>>> @@ -2216,15 +2216,8 @@ static void serial8250_THRE_test(struct uart_port *port)
>>>   		up->bugs |= UART_BUG_THRE;
>>>   }
>>>   
>>> -static void serial8250_initialize(struct uart_port *port)
>>> +static void serial8250_init_mctrl(struct uart_port *port)
>>>   {
>>> -	struct uart_8250_port *up = up_to_u8250p(port);
>>> -	unsigned long flags;
>>> -	bool lsr_TEMT, iir_NOINT;
>>> -
>>> -	serial_port_out(port, UART_LCR, UART_LCR_WLEN8);
>>> -
>>> -	uart_port_lock_irqsave(port, &flags);
>>>   	if (port->flags & UPF_FOURPORT) {
> 
> I should have also added what I meant with my earlier suggestion. AFAICT,
> this UPF_FOURPORT thing can only occur if SERIAL_8250_FOURPORT is enabled.
> 
> The challenge obviously are the if/else constructs but there are a few
> places that do port->flags & UPF_FOURPORT specific thing and something
> else otherwise. That hw-specific code could be placed into the hw-specific
> 8250_fourport.c file if the hw-specific function is made to return true
> if it did match, and the generic code runs otherwise.

So you can brew a patch for this, right :)? Every time I read some code 
the same you describe, I end up with 10+ patches. I don't want for now :P.

> I also have no idea why serial/sunsu.c checks UPF_FOURPORT, perhaps that's
> copy-paste in action. :-)

Yes, AFAI looked, it was during the switch of sunsu to serial driver. If 
you feel confident (it really appears apparent), drop that too ;).

thanks,
-- 
js
suse labs