On Tue, Jun 09, 2015 at 07:03:04PM +0200, Andrew Jones wrote:
> 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.
The idea is to document the code, that has both values
and struct names together.
> 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
In some cases, we've already given up on structs too, using
things like build_append_int_noprefix instead.
> (and struct documentation?).
Move the documentation to build_spcr, yes.
> 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?
Yes.
> In both cases, I guess the actual value is
> what would be compared on lookup.
It's very convenient to locate things quickly using
full text search.
Try doing a full text search for 0x0 in a spec :)
> > 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)
> */
Isn't it actually
Microsoft Serial Port Console Redirection Table
and documented in a couple of other specs?
ACPI spec just lists the signatures.
> assuming I understood your comment correctly.
>
> Thanks for the review,
> drew
