Re: [Xen-devel] [PATCH] ns16550: Add ACPI support

[Wei Xu <xuwei5@hisilicon.com>]

Hi Jan, Julien,

On 2020/1/20 16:38, Jan Beulich wrote:
> On 18.01.2020 13:32, Julien Grall wrote:
>> On 17/01/2020 08:33, Jan Beulich wrote:
>>> On 17.01.2020 04:40, Wei Xu wrote:
>>>> --- a/xen/drivers/char/ns16550.c
>>>> +++ b/xen/drivers/char/ns16550.c
>>>> @@ -1620,6 +1620,61 @@ DT_DEVICE_START(ns16550, "NS16550 UART", DEVICE_SERIAL)
>>>>    DT_DEVICE_END
>>>>    
>>>>    #endif /* HAS_DEVICE_TREE */
>>>> +
>>>> +#ifdef CONFIG_ACPI
>>>> +#include <xen/acpi.h>
>>>> +
>>>> +static int __init ns16550_acpi_uart_init(const void *data)
>>>> +{
>>>> +    struct acpi_table_spcr *spcr = NULL;
>>>> +    acpi_status status;
>>>> +    struct ns16550 *uart;
>>>> +
>>>> +    status = acpi_get_table(ACPI_SIG_SPCR, 0,
>>>> +                            (struct acpi_table_header **)&spcr);
>>>> +
>>>> +    if ( ACPI_FAILURE(status) )
>>>> +    {
>>>> +        printk("ns16550: Failed to get SPCR table\n");
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    uart = &ns16550_com[0];
>>> You want to justify the choice of what (on x86 at least= we'd call
>>> com1 in the patch description. Also this could be the initializer
>>> of the variable.
>> This is the same choice as we made for the DT binding (see
>> ns16550_uart()). We only support one UART on Arm which happen to be
>> ns16550_com[0] (but named diferrently).
>>
>> The code below is actually quite similar to the DT parsing, so maybe we
>> want to provide a common helper here.
> That's all fine, but doesn't eliminate the need to say so in the
> description.
>
>>>> +    /*  Register with generic serial driver. */
>>>> +    serial_register_uart(uart - ns16550_com, &ns16550_driver, uart);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +ACPI_DEVICE_START(ans16550, "NS16550 UART", DEVICE_SERIAL)
>>>> +    .class_type = ACPI_DBG2_16550_COMPATIBLE,
>>>> +    .init = ns16550_acpi_uart_init,
>>>> +ACPI_DEVICE_END
>>> I don't expect this to build on x86.
>> This is only meant to target Arm. So maybe we want to protect the whole
>> code with defined(CONFIG_ACPI) && defined(CONFIG_ARM).
> Indeed, that's what the remark was aiming at.
>
> Jan
>
> .
>

Thanks!
I will address the comments in V2.

Best Regards,
Wei



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel