Re: [PATCH 1/6] arm/virt: Wire up GPIO error source for ACPI / GHES

[Peter Maydell]

On Thu, 11 Jul 2024 at 12:54, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Jul 11, 2024 at 11:52:03AM +0200, Mauro Carvalho Chehab wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > Creates a GED - Generic Event Device and set a GPIO to
> > be used or error injection.
> >
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@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