On Tue, Jun 09, 2015 at 04:52:39PM +0200, Michael S. Tsirkin wrote: > On Mon, Jun 08, 2015 at 10:00:49AM -0400, Andrew Jones wrote: > > SPCR is the Serial Port Console Redirection table. See the document > > linked from http://uefi.org/acpi. For serial port types, "Interface > > Type", see the documentation for the Debug Port Table 2 (DBG2). > > > > Signed-off-by: Andrew Jones <drjo...@redhat.com> > > --- > > include/hw/acpi/acpi-defs.h | 72 > > +++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 72 insertions(+) > > > > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h > > index 59cf277434b37..e579d4c509fc8 100644 > > --- a/include/hw/acpi/acpi-defs.h > > +++ b/include/hw/acpi/acpi-defs.h > > @@ -197,6 +197,78 @@ enum { > > }; > > > > /* > > + * ACPI Serial Port Console Redirection Table > > + */ > > +enum { > > + ACPI_SERIAL_16550_COMPAT = 0, > > + ACPI_SERIAL_16550_SUBSET_COMPAT = 1, > > + ACPI_SERIAL_ARM_PL011_UART = 3, > > +}; > > + > > +enum { > > + ACPI_SERIAL_ITYPE_PC = 1, > > + ACPI_SERIAL_ITYPE_APIC = 2, > > + ACPI_SERIAL_ITYPE_SAPIC = 4, > > + ACPI_SERIAL_ITYPE_ARMH_GIC = 8, > > +}; > > + > > +enum { > > + ACPI_SERIAL_BAUD_9600 = 3, > > + ACPI_SERIAL_BAUD_19200 = 4, > > + ACPI_SERIAL_BAUD_57600 = 6, > > + ACPI_SERIAL_BAUD_115200 = 7, > > +}; > > + > > +enum { > > + ACPI_SERIAL_FLOW_DCD_REQUIRED = 1, > > + ACPI_SERIAL_FLOW_HW = 2, > > + ACPI_SERIAL_FLOW_SW = 4, > > +}; > > + > > +enum { > > + ACPI_SERIAL_TERM_VT100 = 0, > > + ACPI_SERIAL_TERM_VT100_PLUS = 1, > > + ACPI_SERIAL_TERM_VT_UTF8 = 2, > > + ACPI_SERIAL_TERM_ANSI = 3, > > +}; > > + > > Please don't do these single-use enums, this more than doubles the > amount of code required and makes it hard to look up things in spec.
They also serve to document the structure though. Without them we either need to add several lines to the comments for the struct members, or to simply not document the structure at all. But, if the general preference is less lines of code, at the expense of always needing the spec open, then I won't resist removing the enums (and struct documentation?). I don't understand how they make looking things up in the spec more difficult. Is it because the naming doesn't exactly match the spec verbiage? In both cases, I guess the actual value is what would be compared on lookup. > Do this instead > sprc->interface_type = 0x0; /* full 16550 interface */ > > you should also list earliest spec version which has the data > since spec text changes with time. I'll change the header to /* * ACPI 2.0 Serial Port Console Redirection Table (SPCR) */ assuming I understood your comment correctly. Thanks for the review, drew