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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

Hi Julien,

On Thu, 2023-07-13 at 10:43 +0100, Julien Grall wrote:
> Hi Oleksii,
> 
> Title: IMO, Your patch doesn't do any refactor. Instead, it add
> support 
> for polling when using the DT.
Agree. It would be better to rephrase the title.

> 
> On 13/07/2023 10:30, Oleksii Kurochko wrote:
> > In ns16550_init_postirq() there is the following check:
> >      if ( uart->irq > 0 )
> >      {
> >          uart->irqaction.handler = ns16550_interrupt;
> >          uart->irqaction.name    = "ns16550";
> >          uart->irqaction.dev_id  = port;
> >          if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0
> > )
> >              printk("ERROR: Failed to allocate ns16550 IRQ %d\n",
> > uart->irq);
> >      }
> > 
> > Thereby to have ns16550 work in polling mode uart->irq, should be
> > equal to 0.
> > 
> > So it is needed to relax the following check in
> > ns16550_uart_dt_init():
> >      res = platform_get_irq(dev, 0);
> >      if ( ! res )
> >          return -EINVAL;
> >      uart->irq = res;
> > If 'res' equals to -1 then polling mode should be used instead of
> > return
> > -EINVAL.
> 
> This commit message has a bit too much code in it for me taste. I
> don't 
> think it is necessary to quote the code. Instead, you can explain the
> following:
> 
>   * Why you want to support polling
>   * Why this is valid to have a node without interrupts (add a
> reference 
> to the bindings)
>   * That polling is indicated by using 'irq = 0'. I would consider to
> provide a define (e.g NO_IRQ_POLL) to make it more clearer.
Thanks. I'll update the commit message.

> 
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > ---
> >   xen/drivers/char/ns16550.c | 12 ++++++++++--
> >   1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/xen/drivers/char/ns16550.c
> > b/xen/drivers/char/ns16550.c
> > index 2aed6ec707..f30f10d175 100644
> > --- 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 )
> 
> Why do you check explicitely for -1 instead of < 0? Also, the
> behavior 
> is somewhat change now.
I checked it for -1 as I missed that platform_get_irq() returns 'int'
and uart->irq is also 'int'. 'irq' variable inside plaform_get_irq is
declared as 'unsigned int', so I thought that in case of 'interrupt'
property is processed successfully we will have some positive value
otherwise platform_get_irq() returns -1 ( in current implementation ).
So it would be better to check for " res < 0 ".

>  Before, we would return -EINVAL when res equals 
> 0. Can you explain in the commit message why this is done?
This is not clear for me.
It was done during replacing of dt_device_get_irq by platform_get_irq
( 
https://gitlab.com/xen-project/xen/-/commit/554cbe32381fa4482e1a47cd31afb054e97d986d
) and for other similar cases it was changed to "res < 0" except
ns16550 driver.

> 
> > +    {
> > +        printk("ns1650: polling will be used\n");
> > +        /*
> > +         * 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.
> > +         */
> 
> Similar remark to the commit message. You could write:
> 
> "If the node doesn't have any interrupt, then it means the driver
> will 
> want to using polling."
Thanks. I'll take into account.
> 
> > +        res = 0;
> > +    }
> >       uart->irq = res;
> >   
> >       uart->dw_usr_bsy = dt_device_is_compatible(dev, "snps,dw-apb-
> > uart");
> 
> Cheers,
> 

~ Oleksii