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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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