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.kuroc...@gmail.com>
> > ---
> > 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
