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

[Julien Grall <julien@xxxxxxx>]

Hi Oleksii,

Title: IMO, Your patch doesn't do any refactor. Instead, it add support 
for polling when using the DT.
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.
> 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. Before, we would return -EINVAL when res equals 
0. Can you explain in the commit message why this is done?
> +    {
> +        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."
> +        res = 0;
> +    }
>       uart->irq = res;
>   
>       uart->dw_usr_bsy = dt_device_is_compatible(dev, "snps,dw-apb-uart");
Cheers,

--
Julien Grall