From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45025) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z2NKG-0001ql-7F for qemu-devel@nongnu.org; Tue, 09 Jun 2015 13:28:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z2NKD-0001DM-1z for qemu-devel@nongnu.org; Tue, 09 Jun 2015 13:28:36 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45408) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z2NKC-0001By-Qf for qemu-devel@nongnu.org; Tue, 09 Jun 2015 13:28:32 -0400 Date: Tue, 9 Jun 2015 19:28:26 +0200 From: "Michael S. Tsirkin" Message-ID: <20150609191908-mutt-send-email-mst@redhat.com> References: <1433772050-28113-1-git-send-email-drjones@redhat.com> <1433772050-28113-2-git-send-email-drjones@redhat.com> <20150609163941-mutt-send-email-mst@redhat.com> <20150609170304.GB9626@hawk.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150609170304.GB9626@hawk.localdomain> Subject: Re: [Qemu-devel] [PATCH 1/2] ACPI: Add definitions for the SPCR table List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andrew Jones Cc: imammedo@redhat.com, qemu-devel@nongnu.org, shannon.zhao@linaro.org 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 > > > --- > > > 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