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.
