| [Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
 Re: [Xen-devel] [PATCH] ns16550: Add ACPI support
 
To: Jan Beulich <jbeulich@xxxxxxxx>, Julien Grall <julien@xxxxxxx>From: Wei Xu <xuwei5@xxxxxxxxxxxxx>Date: Tue, 21 Jan 2020 11:28:49 +0800Cc: sstabellini@xxxxxxxxxx, Wei Liu <wl@xxxxxxx>, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>, George Dunlap <George.Dunlap@xxxxxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Ian Jackson <ian.jackson@xxxxxxxxxxxxx>, linuxarm@xxxxxxxxxx, shameerali.kolothum.thodi@xxxxxxxxxx, prime.zeng@xxxxxxxxxxxxx, xen-devel@xxxxxxxxxxxxxxxxxxxxDelivery-date: Tue, 21 Jan 2020 03:29:09 +0000List-id: Xen developer discussion <xen-devel.lists.xenproject.org> 
 
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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel 
 |