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

[Jiri Slaby]

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@xxxxxxxxxx>
> > > Suggested-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> >
> > Heh, I didn't even realize I was suggesting this :-D but it's good
> > nonetheless.
> >
> > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> >
> > > Cc: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> > > ---
> > > [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