Re: [Qemu-devel] [PATCH 1/2] ACPI: Add definitions for the SPCR table

[Andrew Jones <drjones@redhat.com>]

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 <drjones@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