Re: [Qemu-devel] [PATCH 5/7] acpi: Add I2c serial bus CRS handling

[Michael S. Tsirkin]

On Wed, May 11, 2016 at 02:46:04PM -0500, address@hidden wrote:
> From: Corey Minyard <address@hidden>
> 
> Signed-off-by: Corey Minyard <address@hidden>
> ---
>  hw/acpi/aml-build.c         | 42 ++++++++++++++++++++++++++++++++++++++++++
>  include/hw/acpi/aml-build.h | 11 +++++++++++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index ab89ca6..7a3874b 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -1563,3 +1563,45 @@ build_rsdt(GArray *table_data, GArray *linker, GArray 
> *table_offsets,
>      build_header(linker, table_data,
>                   (void *)rsdt, "RSDT", rsdt_len, 1, oem_id, oem_table_id);
>  }
> +

Pls add comment with earliest spec revision that includes this.

> +static Aml *aml_serial_bus_device(AmlSerialBusType type, uint8_t flags,
> +                                  uint16_t type_flags,
> +                                  uint8_t revid, uint16_t data_length,
> +                                  uint16_t resource_source_length)
> +{
> +    Aml *var = aml_alloc();
> +    uint16_t length = data_length + resource_source_length + 9;
> +
> +    build_append_byte(var->buf, 0x8e); /* Serial Bus Connection Descriptor */
> +    build_append_byte(var->buf, length & 0xff);
> +    build_append_byte(var->buf, length >> 8);
> +    build_append_byte(var->buf, 1);    /* Revision */
> +    build_append_byte(var->buf, 0);    /* Resource source index */
> +    build_append_byte(var->buf, type); /* Serial bus type */
> +    build_append_byte(var->buf, flags); /* Serial bus type */

Fix comments to match spec verbatim please.

> +    build_append_byte(var->buf, type_flags & 0xff);
> +    build_append_byte(var->buf, type_flags >> 8);
> +    build_append_byte(var->buf, revid);
> +    build_append_byte(var->buf, data_length & 0xff);
> +    build_append_byte(var->buf, data_length >> 8);
> +

Please use build_append_int_noprefix.

> +    return var;
> +}
> +
> +Aml *aml_i2c_serial_bus_device(uint8_t flags, uint32_t con_speed,
> +                               uint16_t address, const char *resource_source)
> +{
> +    unsigned int resource_source_len = strlen(resource_source) + 1;
> +    Aml *var = aml_serial_bus_device(aml_serial_bus_type_i2c, flags, 0, 1,
> +                                     6, resource_source_len);
> +
> +    build_append_byte(var->buf, con_speed & 0xff);
> +    build_append_byte(var->buf, (con_speed >> 8) & 0xff);
> +    build_append_byte(var->buf, (con_speed >> 16) & 0xff);
> +    build_append_byte(var->buf, (con_speed >> 24) & 0xff);

Please use build_append_int_noprefix.

> +    build_append_byte(var->buf, address & 0xff);
> +    build_append_byte(var->buf, address >> 8);

Same.

> +    g_array_append_vals(var->buf, resource_source, resource_source_len);

Is resource_source a name then? Then you should do
    aml_append(var, aml_name("%s", alias_object));


> +
> +    return var;
> +}
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index 2c994b3..1eb3ebd 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -206,6 +206,15 @@ struct AcpiBuildTables {
>      GArray *linker;
>  } AcpiBuildTables;
>  
> +typedef enum {
> +    aml_serial_bus_type_i2c = 1,
> +    aml_serial_bus_type_spi = 2,
> +    aml_serial_bus_type_uart = 3
> +} AmlSerialBusType;
> +
> +#define AML_SERIAL_BUS_FLAG_MASTER_DEVICE       (1 << 0)
> +#define AML_SERIAL_BUS_FLAG_CONSUME_ONLY        (1 << 1)
> +

Please drop enums and add numbers with comments matching
text in ACPI spec in aml-build.c

>  /**
>   * init_aml_allocator:
>   *
> @@ -327,6 +336,8 @@ Aml *aml_qword_memory(AmlDecode dec, AmlMinFixed 
> min_fixed,
>  Aml *aml_dma(AmlDmaType typ, AmlDmaBusMaster bm, AmlTransferSize sz,
>               uint8_t channel);
>  Aml *aml_sleep(uint64_t msec);
> +Aml *aml_i2c_serial_bus_device(uint8_t flags, uint32_t con_speed,

Drop these two parameters until they are actually useful.

> +                               uint16_t address, const char 
> *resource_source);
>  
>  /* Block AML object primitives */
>  Aml *aml_scope(const char *name_format, ...) GCC_FMT_ATTR(1, 2);
> -- 
> 2.7.4