Hi Simon,
-----"Simon Glass" <s...@chromium.org> schrieb: -----
> Betreff: [PATCH v2 20/35] acpi: Support writing a GPIO
>
> Allowing writing out a reference to a GPIO within the ACPI output. This
> can be used by ACPI code to access a GPIO at runtime.
>
> Signed-off-by: Simon Glass <s...@chromium.org>
> ---
>
> Changes in v2: None
> Changes in v1: None
>
> include/acpi/acpi_dp.h | 18 ++++++++++++++++++
> lib/acpi/acpi_dp.c | 21 +++++++++++++++++++++
> test/dm/acpi_dp.c | 39 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 78 insertions(+)
>
> diff --git a/include/acpi/acpi_dp.h b/include/acpi/acpi_dp.h
> index 840be855c5..f42602405a 100644
> --- a/include/acpi/acpi_dp.h
> +++ b/include/acpi/acpi_dp.h
> @@ -202,6 +202,24 @@ struct acpi_dp *acpi_dp_add_integer_array(struct acpi_dp
> *dp, const char *name,
> struct acpi_dp *acpi_dp_add_child(struct acpi_dp *dp, const char *name,
> struct acpi_dp *child);
>
> +/**
> + * acpi_dp_add_gpio() - Add a GPIO to a list of Device Properties
> + *
> + * A new node is added to the end of the property list of @dp, with the
> + * GPIO properties added to the the new node
> + *
> + * @dp: Table to add this property to
> + * @name: Name of property
> + * @ref: Reference to device with a _CRS containing GpioIO or GpioInt
> + * @index: Index of the GPIO resource in _CRS starting from zero
> + * @pin: Pin in the GPIO resource, typically zero
> + * @active_low: true if pin is active low
> + * @return pointer to new node, or NULL if out of memory
> + */
> +struct acpi_dp *acpi_dp_add_gpio(struct acpi_dp *dp, const char *name,
> + const char *ref, int index, int pin,
> + bool active_low);
> +
> /**
> * acpi_dp_write() - Write Device Property hierarchy and clean up resources
> *
> diff --git a/lib/acpi/acpi_dp.c b/lib/acpi/acpi_dp.c
> index 479cb6743c..4ba5d555a0 100644
> --- a/lib/acpi/acpi_dp.c
> +++ b/lib/acpi/acpi_dp.c
> @@ -322,3 +322,24 @@ struct acpi_dp *acpi_dp_add_integer_array(struct acpi_dp
> *dp, const char *name,
>
> return dp_array;
> }
> +
> +struct acpi_dp *acpi_dp_add_gpio(struct acpi_dp *dp, const char *name,
> + const char *ref, int index, int pin,
> + bool active_low)
Boolean parameters can be hard to read.
Could we instead of a boolean parameter use the type enum acpi_irq_polarity
which is defined in acpi_device.h?
> +{
> + struct acpi_dp *gpio;
> +
> + assert(dp);
> + gpio = acpi_dp_new_table(name);
> + if (!gpio)
> + return NULL;
> +
> + acpi_dp_add_reference(gpio, NULL, ref);
> + acpi_dp_add_integer(gpio, NULL, index);
> + acpi_dp_add_integer(gpio, NULL, pin);
> + acpi_dp_add_integer(gpio, NULL, active_low);
Is it allowed to pass NULL for the parameter 'name' of these functions?
The descriptions of acpi_dp_add_reference() and acpi_dp_add_integer()
don't mention that name may be NULL, and both of these functions pass
it on internally to acpi_dp_new() which has an explicit check for
assert(name) ... ?
Also, acpi_dp_add_reference() and acpi_dp_add_integer() could return an error
(NULL), which is not checked for here.
> +
> + acpi_dp_add_array(dp, gpio);
> +
> + return gpio;
> +}
> diff --git a/test/dm/acpi_dp.c b/test/dm/acpi_dp.c
> index c11394786f..d6a054a98b 100644
> --- a/test/dm/acpi_dp.c
> +++ b/test/dm/acpi_dp.c
> @@ -403,3 +403,42 @@ static int dm_test_acpi_dp_child(struct unit_test_state
> *uts)
> return 0;
> }
> DM_TEST(dm_test_acpi_dp_child, 0);
> +
> +/* Test emitting a GPIO */
> +static int dm_test_acpi_dp_gpio(struct unit_test_state *uts)
> +{
> + struct acpi_ctx *ctx;
> + struct acpi_dp *dp;
> + u8 *ptr, *pptr;
> +
> + ut_assertok(alloc_context(&ctx));
> +
> + dp = acpi_dp_new_table("FRED");
> + ut_assertnonnull(dp);
> +
> + /* Try a few different parameters */
> + ut_assertnonnull(acpi_dp_add_gpio(dp, "reset", TEST_REF, 0x23, 0x24,
> + false));
> + ut_assertnonnull(acpi_dp_add_gpio(dp, "allow", TEST_REF, 0, 0, true));
> +
> + ptr = acpigen_get_current(ctx);
> + ut_assertok(acpi_dp_write(ctx, dp));
> + ut_asserteq(0x6e, acpigen_get_current(ctx) - ptr);
> +
> + pptr = ptr + 0x2c; //0x3a;
> + ut_asserteq_str("reset", (char *)pptr);
> + ut_asserteq_strn(EXPECT_REF, (char *)pptr + 0xe);
> + ut_asserteq(0x23, pptr[0x1b]);
> + ut_asserteq(0x24, pptr[0x1d]);
> + ut_asserteq(ZERO_OP, pptr[0x1e]);
> +
> + pptr = ptr + 0x51;
> + ut_asserteq_str("allow", (char *)pptr);
> + ut_asserteq_strn(EXPECT_REF, (char *)pptr + 0xe);
> + ut_asserteq(ZERO_OP, pptr[0x1a]);
> + ut_asserteq(ZERO_OP, pptr[0x1b]);
> + ut_asserteq(ONE_OP, pptr[0x1c]);
> +
> + return 0;
> +}
> +DM_TEST(dm_test_acpi_dp_gpio, 0);
> --
> 2.26.2.645.ge9eca65c58-goog
>
regards, Wolfgang
