Re: [PATCH] ns1650: refactor interrupt handling in ns16550_uart_dt_init()

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Thu, 2023-07-13 at 11:13 +0100, Julien Grall wrote:
> Hi Jan,
> 
> On 13/07/2023 11:08, Jan Beulich wrote:
> > On 13.07.2023 11:30, Oleksii Kurochko wrote:
> > > --- a/xen/drivers/char/ns16550.c
> > > +++ b/xen/drivers/char/ns16550.c
> > > @@ -1791,8 +1791,16 @@ static int __init
> > > ns16550_uart_dt_init(struct dt_device_node *dev,
> > >       }
> > >   
> > >       res = platform_get_irq(dev, 0);
> > > -    if ( ! res )
> > > -        return -EINVAL;
> > > +    if ( res == -1 )
> > > +    {
> > > +        printk("ns1650: polling will be used\n");
> > 
> > Nit: Please don't omit one of the two 5-s here.
> > 
> > > +        /*
> > > +         * There is the check 'if ( uart->irq > 0 )' in
> > > ns16550_init_postirq().
> > > +         * If the check is true then interrupt mode will be used
> > > otherwise
> > > +         * ( when irq = 0 )polling.
> > > +         */
> > 
> > I wonder in how far that's actually correct outside of x86. On x86
> > IRQ0 is
> > always the timer interrupt, but I'm not convinced something similar
> > can be
> > used as kind of a heuristic on Arm, RISC-V, or basically any other
> > architecture.
> 
> I wondered the same. On Arm we are fine because the UART will be an
> SPI 
> which starts at 32.
> 
> That's part why I was suggesting to use a define. Because we don't
> have 
> to hardcode the poll value everywhere.
Probably then it would be better to introduce 'bool is_polling_mode'
inside struct ns16550?

The same thing ( with uart->irq = 0 ) is used for detecting if polling
mode should be used in case of x86 and PCI:
https://gitlab.com/xen-project/xen/-/blame/staging/xen/drivers/char/ns16550.c?page=2#L1332

~ Oleksii