On Mon, 16 Nov 2015 21:23:05 +0800 shannon.z...@linaro.org wrote: > From: Shannon Zhao <shannon.z...@linaro.org>
Subj can be shortened to: acpi: Add GPIO Connection Descriptor > > Signed-off-by: Shannon Zhao <zhaoshengl...@huawei.com> > Signed-off-by: Shannon Zhao <shannon.z...@linaro.org> > Tested-by: Wei Huang <w...@redhat.com> > --- > hw/acpi/aml-build.c | 61 > +++++++++++++++++++++++++++++++++++++++++++++ > include/hw/acpi/aml-build.h | 20 +++++++++++++++ > 2 files changed, 81 insertions(+) > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c > index a00a0ab..60d4703 100644 > --- a/hw/acpi/aml-build.c > +++ b/hw/acpi/aml-build.c > @@ -565,6 +565,67 @@ Aml *aml_call4(const char *method, Aml *arg1, Aml *arg2, > Aml *arg3, Aml *arg4) > } > > /* > + * ACPI 5.0: 6.4.3.8.1 GPIO Connection Descriptor > + * Type 1, Large Item Name 0xC > + */ > + > +static Aml *aml_gpio_connection(AmlGpioConnectionType type, > + AmlConsumerAndProducer con_and_pro, > + uint8_t flags, AmlPinConfig pin_cfg, > + int16_t output_drive, int16_t > debounce_timeout, > + int32_t pin_num[], int32_t pin_count, Probably intFOO_t should be uintFOO_t. s/pin_num/pin_list/ I've used a bit more complicated to make list of FOO integers, like this: https://github.com/imammedo/qemu/commit/f6925e53aa2e0266a07dbb39ae17efbf13dba388 + Aml *irqs = aml_interrupt_list(); + aml_append_interrupt2list(irqs, uart_irq); aml_append(crs, aml_interrupt(AML_CONSUMER, AML_LEVEL, AML_ACTIVE_HIGH, - AML_EXCLUSIVE, uart_irq)); + AML_EXCLUSIVE, irqs)); but I like simpler array way you're using here, Michael do you have any objection to passing IRQ/PIN lists as arrays like this patch does? > + const char *name, const char *vendor_data) s/name/resource_src_name/ s/const char *vendor_data/const uint8_t *vendor_data/ > +{ > + Aml *var = aml_alloc(); > + uint16_t name_len = name ? (strlen(name) + 1) : 0; name doesn't seem to optional so case 'name == NULL' should be invalid, add assert(name) and drop condition > + uint16_t vendor_data_len = vendor_data ? (strlen(vendor_data) + 1) : 0; vendor_data is binary blob so you can't use strlen() on it. > + uint16_t length = 0x16 + name_len + vendor_data_len; s/0x16/const min_desc_len = 0x16/ > + uint16_t name_offset = 0x17 + pin_count * 2; and then use it here and below for calculating pin_table_offset > + uint16_t vendor_data_offset = name_offset + name_len; > + int i; > + > + build_append_byte(var->buf, 0x8C); /* GpioInt Resource Descriptor */ CpioInt is wrong, it should be "GPIO Connection Descriptor" > + build_append_int_noprefix(var->buf, length, 2); /* Length */ > + build_append_byte(var->buf, 1); /* Revision ID */ > + build_append_byte(var->buf, type); /* GPIO Connection Type */ > + /* General Flags (2 bytes) */ > + build_append_int_noprefix(var->buf, con_and_pro, 2); > + /* Interrupt and IO Flags (2 bytes) */ > + build_append_int_noprefix(var->buf, flags, 2); > + /* Pin Configuration 0 = Default 1 = Pull-up 2 = Pull-down 3 = No Pull */ > + build_append_byte(var->buf, pin_cfg); > + /* Output Drive Strength (2 bytes) */ > + build_append_int_noprefix(var->buf, output_drive, 2); > + /* Debounce Timeout (2 bytes) */ > + build_append_int_noprefix(var->buf, debounce_timeout, 2); > + /* Pin Table Offset (2 bytes) */ > + build_append_int_noprefix(var->buf, 0x17, 2); > + build_append_byte(var->buf, 0); /* Resource Source Index */ > + /* Resource Source Name Offset (2 bytes) */ > + build_append_int_noprefix(var->buf, name_offset, 2); > + /* Vendor Data Offset (2 bytes) */ > + build_append_int_noprefix(var->buf, vendor_data_offset, 2); > + /* Vendor Data Length (2 bytes) */ > + build_append_int_noprefix(var->buf, vendor_data_len, 2); > + /* Pin Number (2n bytes)*/ > + for (i = 0; i < pin_count; i++) { > + build_append_int_noprefix(var->buf, pin_num[i], 2); > + } > + /* Resource Source Name */ > + if (name != NULL) { name shouldn't be NULL ever, so drop it > + build_append_namestring(var->buf, "%s", name); > + build_append_byte(var->buf, '\0'); > + } > + /* Vendor-defined Data */ > + if (vendor_data != NULL) { > + build_append_namestring(var->buf, "%s", vendor_data); > + build_append_byte(var->buf, '\0'); > + } that's wrong, vendor_data is RawDataBuffer and not NameString following would do the right thing: g_array_append_vals(var->buf, vendor_data, vendor_data_len); > + > + return var; > +} > + > +/* > * ACPI 1.0b: 6.4.3.4 32-Bit Fixed Location Memory Range Descriptor > * (Type 1, Large Item Name 0x6) > */ > diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h > index 1b632dc..4e88882 100644 > --- a/include/hw/acpi/aml-build.h > +++ b/include/hw/acpi/aml-build.h > @@ -148,6 +148,26 @@ typedef enum { > AML_SHARED_AND_WAKE = 3, > } AmlShared; > > +/* > + * ACPI 5.0: Table 6-189 GPIO Connection Descriptor Definition > + * GPIO Connection Type > + */ > +typedef enum { > + AML_INTERRUPT_CONNECTION = 0, > + AML_IO_CONNECTION = 1, > +} AmlGpioConnectionType; > + > +/* > + * ACPI 5.0: Table 6-189 GPIO Connection Descriptor Definition > + * _PPI field definition > + */ > +typedef enum { > + AML_DEFAULT_CONFIG = 0, I'd suggest to use AML_PULL_DEFAULT as it's mentioned in spec (see GpioInt/GpioIO) > + AML_PULL_UP = 1, > + AML_PULL_DOWN = 2, > + AML_NO_PULL = 3, Likewise AML_PULL_NONE > +} AmlPinConfig; > + > typedef > struct AcpiBuildTables { > GArray *table_data;