Re: [LinuxBIOS] [PATCH] Add support for the Asus A8V-E SE motherboard

[Stefan Reinauer]

* Uwe Hermann  [071030 15:18]:
> > +void acpi_create_fadt(acpi_fadt_t * fadt, acpi_facs_t * facs, void *dsdt)
> > +{
> > acpi_header_t *header = &(fadt->header);
> > memset((void *) fadt, 0, sizeof(acpi_fadt_t));
> > memcpy(header->signature, "FACP", 4);
> > header->length = 244;
> > header->revision = 1;
> > memcpy(header->oem_id, "LXBIOS", 6);
>
> "LinuxBIOS" as string? How long can the ID become?
> > memcpy(header->oem_table_id, "LXBACPI ", 8);
> > memcpy(header->asl_compiler_id, "LXB", 8);
> Same here. How long?

How about a little RTFS in the reviews? ;-) SCNR

from src/arch/i386/include/arch/acpi.h:

typedef struct acpi_table_header         /* ACPI common table header */
{
        char signature [4];          /* ACPI signature (4 ASCII characters) */\
        u32  length;                 /* Length of table, in bytes, including header */\
        u8   revision;               /* ACPI Specification minor version # */\
        u8   checksum;               /* To make sum of entire table == 0 */\
        char oem_id [6];             /* OEM identification */\
        char oem_table_id [8];       /* OEM table identification */\
        u32  oem_revision;           /* OEM revision number */\
        char asl_compiler_id [4];    /* ASL compiler vendor ID */\
        u32  asl_compiler_revision;  /* ASL compiler revision number */
} __attribute__ ((packed)) acpi_header_t;
> > fadt->firmware_ctrl = facs;
> > fadt->dsdt = dsdt;
> > fadt->preferred_pm_profile = 0;
> > fadt->sci_int = 9;
> > fadt->smi_cmd = 0;
> > fadt->acpi_enable = 0;
> > fadt->acpi_disable = 0;
> > fadt->s4bios_req = 0x0;
> > fadt->pstate_cnt = 0x0;

[..]
> > fadt->x_gpe1_blk.space_id = 1;
> > fadt->x_gpe1_blk.bit_width = 0;
> > fadt->x_gpe1_blk.bit_offset = 0;
> > fadt->x_gpe1_blk.resv = 0;
> > fadt->x_gpe1_blk.addrl = 0x0;
> > fadt->x_gpe1_blk.addrh = 0x0;
> > header->checksum =
> >    acpi_checksum((void *) fadt, sizeof(acpi_fadt_t));
>
> Let's commit this, but can it be changed later?
> This all looks very similar, maybe a 'static const struct ...' table
> with all the numbers, and a small loop to set the entries would
> be possible?

This table would be struct acpi_fadt? And copying it to ram ("the loop")
would work with memcpy?
> > Index: src/mainboard/asus/a8v-e_se/acpi_tables.c
> >
> > --- src/mainboard/asus/a8v-e_se/acpi_tables.c	(revision 0)
> > +++ src/mainboard/asus/a8v-e_se/acpi_tables.c	(revision 0)
> > @@ -0,0 +1,170 @@
> > +/*
> > This file is part of the LinuxBIOS project.
> > written by Stefan Reinauer
> > ACPI FADT, FACS, and DSDT table support added by
> > Nick Barker , and those portions
> > Copyright (C) 2007 Rudolf Marek
>
> Hm, please add
>
> Copyright (C) xxxx Stefan Reinauer 
> Copyright (C) xxxx Nick Barker
>
> then. I think this is not done in the original file either, and should
> be fixed there, too. Listing all copyright owners explicitly is important.

Let's fix this issue in another patch. It is pretty irrelevant for this review.

Stefan