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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.07.2023 13:55, Oleksii wrote:
> 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?

Perhaps. If I was to make such a change, I'd probably convert intr_works
to a tristate. But a boolean will be okay; if I may ask, name it just
"polling" though.

Jan