Re: [PATCH] ns16550: add support for polling mode when device tree is used

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Wed, 2023-08-16 at 15:39 +0200, Jan Beulich wrote:
> On 11.08.2023 17:30, Oleksii Kurochko wrote:
> > @@ -1555,6 +1566,9 @@ static bool __init parse_positional(struct
> > ns16550 *uart, char **str)
> >          }
> >          else
> >  #endif
> > +        if ( strncmp(conf, "poll", 4) == 0 )
> > +            uart->intr_works = polling;
> 
> Don't you need to update "conf" here as well then?
Yes, sure, 'conf' shoud be updated too.

> 
> As said before, please also update parse_namevalue_pairs(). I would
> appreciate (but not insist) if you also added recognition of "msi"
> at this occasion.
I'll add 'msi' too.

> 
> > +        else
> >              uart->irq = simple_strtol(conf, &conf, 10);
> >      }
> >  
> > @@ -1760,6 +1774,9 @@ static int __init ns16550_uart_dt_init(struct
> > dt_device_node *dev,
> >  
> >      ns16550_init_common(uart);
> >  
> > +    if ( strstr(opt_com1, "poll") )
> > +        uart->intr_works = polling;
> 
> Is strstr() really appropriate? Shouldn't it simply be strcmp(), with
> there not being any other sub-options in the non-x86 case?
It would better to use strcmp(). Thanks.

> 
> Plus the question remains of it necessarily being com1: Is there no
> way with DT to have multiple serial ports (e.g. one for the console
> and one for a debugger)? If there indeed isn't, then unconditionally
> using opt_com1[] here is of course okay, but then opt_com2[]
> is effectively a dead variable and recognizing "com2" on the command
> line (rather than spitting out an error) is then also a mistake. IOW
> in that case both would want keeping x86-only (with a new #ifdef, as
> we certainly don't want to have com1 and com2 stuff in separate
> places).
Actually it can be even more serial ports. For example, I have a board
with 3 UARTs ( serial ports ). 
In this case, it looks that I should have 3 variable of opt_com{1-3}[]?

Taking into account that opt_com{1-2} variables are needed only for
configuration of serial ports in X86 ( in DT-based architectures all
configuration info is inside a node of UART ) then we can check only
opt_com1[].

~ Oleksii