On Thu, 11 Jul 2024 at 12:54, Michael S. Tsirkin <m...@redhat.com> wrote:
>
> On Thu, Jul 11, 2024 at 11:52:03AM +0200, Mauro Carvalho Chehab wrote:
> > From: Jonathan Cameron <jonathan.came...@huawei.com>
> >
> > Creates a GED - Generic Event Device and set a GPIO to
> > be used or error injection.
> >
> > Signed-off-by: Jonathan Cameron <jonathan.came...@huawei.com>
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+hua...@kernel.org>
> > ---
> > hw/arm/virt-acpi-build.c | 29 +++++++++++++++++++++++++----
> > hw/arm/virt.c | 12 +++++++++++-
> > include/hw/boards.h | 1 +
> > 3 files changed, 37 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index e10cad86dd73..b6f2e55014a2 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -63,6 +63,7 @@
> >
> > #define ARM_SPI_BASE 32
> >
> > +#define ACPI_GENERIC_EVENT_DEVICE "GEDD"
> > #define ACPI_BUILD_TABLE_SIZE 0x20000
> >
> > static void acpi_dsdt_add_cpus(Aml *scope, VirtMachineState *vms)
> > @@ -155,9 +156,14 @@ static void acpi_dsdt_add_gpio(Aml *scope, const
> > MemMapEntry *gpio_memmap,
> >
> > Aml *aei = aml_resource_template();
> > /* Pin 3 for power button */
> > - const uint32_t pin_list[1] = {3};
> > + uint32_t pin = 3;
> > aml_append(aei, aml_gpio_int(AML_CONSUMER, AML_EDGE, AML_ACTIVE_HIGH,
> > - AML_EXCLUSIVE, AML_PULL_UP, 0, pin_list,
> > 1,
> > + AML_EXCLUSIVE, AML_PULL_UP, 0, &pin, 1,
> > + "GPO0", NULL, 0));
> > + pin = 6;
> > + /* Pin 8 for generic error */
>
> For real? Code says 6, comment says 8.
>
> Comments must come before the code they comment, not after it,
> then this kind of thing won't happen.
It might also be nice to have a symbolic constant for the
pin number somewhere, so we don't rely on the magic number here and...
> > @@ -1014,6 +1021,8 @@ static void create_gpio_keys(char *fdt, DeviceState
> > *pl061_dev,
> > {
> > gpio_key_dev = sysbus_create_simple("gpio-key", -1,
> > qdev_get_gpio_in(pl061_dev, 3));
> > + gpio_error_dev = sysbus_create_simple("gpio-key", -1,
> > + qdev_get_gpio_in(pl061_dev, 6));
...here being the same.
Then if the code says "pin = VIRT_GPIO_ERROR_PIN" or something
similar the comment isn't required because the code is clear
without it.
(This is already a problem for the power-button pin 3, so we could
do an initial cleanup patch to sort out pin 3 first).
thanks
-- PMM
