Re: [PATCH 2/2] hw/arm/tosa: Encapsulate misc GPIO handling in a device

[Philippe Mathieu-Daudé]

Hi Peter,

On 6/28/20 10:37 PM, Peter Maydell wrote:
> Currently we have a free-floating set of IRQs and a function
> tosa_out_switch() which handle the GPIO lines on the tosa board which
> connect to LEDs, and another free-floating IRQ and tosa_reset()
> function to handle the GPIO line that resets the system.  Encapsulate
> this behaviour in a simple QOM device.
> 
> This commit fixes Coverity issue CID 1421929 (which pointed out that
> the 'outsignals' in tosa_gpio_setup() were leaked), because it
> removes the use of the qemu_allocate_irqs() API from this code
> entirely.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> This is simpler than the spitz changes because the new device
> doesn't need to refer to any of the other devices on the board.
> ---
>  hw/arm/tosa.c | 88 +++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 64 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/arm/tosa.c b/hw/arm/tosa.c
> index 06ecf1e7824..383b3b22e24 100644
> --- a/hw/arm/tosa.c
> +++ b/hw/arm/tosa.c
> @@ -65,24 +65,39 @@ static void tosa_microdrive_attach(PXA2xxState *cpu)
>      pxa2xx_pcmcia_attach(cpu->pcmcia[0], md);
>  }
>  
> -static void tosa_out_switch(void *opaque, int line, int level)
> +/*
> + * Encapsulation of some GPIO line behaviour for the Tosa board
> + *
> + * QEMU interface:
> + *  + named GPIO inputs "leds[0..3]": assert to light LEDs
> + *  + named GPIO input "reset": when asserted, resets the system
> + */
> +
> +#define TYPE_TOSA_MISC_GPIO "tosa-misc-gpio"
> +#define TOSA_MISC_GPIO(obj) \
> +    OBJECT_CHECK(TosaMiscGPIOState, (obj), TYPE_TOSA_MISC_GPIO)
> +
> +typedef struct TosaMiscGPIOState {
> +    SysBusDevice parent_obj;
> +} TosaMiscGPIOState;

Since we don't really use this type, can we avoid declaring it?

Like:

  #define TOSA_MISC_GPIO(obj) \
      OBJECT_CHECK(SysBusDevice, (obj), TYPE_TOSA_MISC_GPIO)

And in tosa_misc_gpio_info:

    .instance_size = sizeof(SysBusDevice)

> +
> +static void tosa_gpio_leds(void *opaque, int line, int level)
>  {
>      switch (line) {
> -        case 0:
> -            fprintf(stderr, "blue LED %s.\n", level ? "on" : "off");
> -            break;
> -        case 1:
> -            fprintf(stderr, "green LED %s.\n", level ? "on" : "off");
> -            break;
> -        case 2:
> -            fprintf(stderr, "amber LED %s.\n", level ? "on" : "off");
> -            break;
> -        case 3:
> -            fprintf(stderr, "wlan LED %s.\n", level ? "on" : "off");
> -            break;
> -        default:
> -            fprintf(stderr, "Uhandled out event: %d = %d\n", line, level);
> -            break;
> +    case 0:
> +        fprintf(stderr, "blue LED %s.\n", level ? "on" : "off");
> +        break;
> +    case 1:
> +        fprintf(stderr, "green LED %s.\n", level ? "on" : "off");
> +        break;
> +    case 2:
> +        fprintf(stderr, "amber LED %s.\n", level ? "on" : "off");
> +        break;
> +    case 3:
> +        fprintf(stderr, "wlan LED %s.\n", level ? "on" : "off");
> +        break;

Nitpicking, the indentation change might go in the previous patch.

> +    default:
> +        g_assert_not_reached();
>      }
>  }
>  
> @@ -93,13 +108,22 @@ static void tosa_reset(void *opaque, int line, int level)
>      }
>  }
>  
> +static void tosa_misc_gpio_init(Object *obj)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +

Ah, MachineClass does not inherit from DeviceClass, so we can use
it to create GPIOs.

Something is bugging me here, similar with the LEDs series I sent
recently.

GPIOs are not specific to a bus. I see ResettableClass takes Object
arguments.

We should be able to wire GPIO lines to generic Objects like LEDs.
Parents don't have to be qdev.

Actually looking at qdev_init_gpio_in_named_with_opaque(), the
function only accesses the QOM API, not the QDEV one. The only
field stored in the state is the gpio list:

struct DeviceState {
    ...
    QLIST_HEAD(, NamedGPIOList) gpios;

Having to create a container to wire GPIOs or hold a reference
to a MemoryRegion sounds wrong.

If the MachineState can not do that, can we create a generic
BoardState (like PCB to route signals) so all machines can use it?

> +    qdev_init_gpio_in_named(dev, tosa_gpio_leds, "leds", 4);
> +    qdev_init_gpio_in_named(dev, tosa_reset, "reset", 1);
> +}
> +
>  static void tosa_gpio_setup(PXA2xxState *cpu,
>                  DeviceState *scp0,
>                  DeviceState *scp1,
>                  TC6393xbState *tmio)
>  {
> -    qemu_irq *outsignals = qemu_allocate_irqs(tosa_out_switch, cpu, 4);
> -    qemu_irq reset;
> +    DeviceState *misc_gpio;
> +
> +    misc_gpio = sysbus_create_simple(TYPE_TOSA_MISC_GPIO, -1, NULL);
>  
>      /* MMC/SD host */
>      pxa2xx_mmci_handlers(cpu->mmc,
> @@ -107,8 +131,8 @@ static void tosa_gpio_setup(PXA2xxState *cpu,
>                      qemu_irq_invert(qdev_get_gpio_in(cpu->gpio, 
> TOSA_GPIO_nSD_DETECT)));
>  
>      /* Handle reset */
> -    reset = qemu_allocate_irq(tosa_reset, cpu, 0);
> -    qdev_connect_gpio_out(cpu->gpio, TOSA_GPIO_ON_RESET, reset);
> +    qdev_connect_gpio_out(cpu->gpio, TOSA_GPIO_ON_RESET,
> +                          qdev_get_gpio_in_named(misc_gpio, "reset", 0));
>  
>      /* PCMCIA signals: card's IRQ and Card-Detect */
>      pxa2xx_pcmcia_set_irq_cb(cpu->pcmcia[0],
> @@ -119,10 +143,14 @@ static void tosa_gpio_setup(PXA2xxState *cpu,
>                          qdev_get_gpio_in(cpu->gpio, TOSA_GPIO_JC_CF_IRQ),
>                          NULL);
>  
> -    qdev_connect_gpio_out(scp1, TOSA_GPIO_BT_LED, outsignals[0]);
> -    qdev_connect_gpio_out(scp1, TOSA_GPIO_NOTE_LED, outsignals[1]);
> -    qdev_connect_gpio_out(scp1, TOSA_GPIO_CHRG_ERR_LED, outsignals[2]);
> -    qdev_connect_gpio_out(scp1, TOSA_GPIO_WLAN_LED, outsignals[3]);
> +    qdev_connect_gpio_out(scp1, TOSA_GPIO_BT_LED,
> +                          qdev_get_gpio_in_named(misc_gpio, "leds", 0));
> +    qdev_connect_gpio_out(scp1, TOSA_GPIO_NOTE_LED,
> +                          qdev_get_gpio_in_named(misc_gpio, "leds", 1));
> +    qdev_connect_gpio_out(scp1, TOSA_GPIO_CHRG_ERR_LED,
> +                          qdev_get_gpio_in_named(misc_gpio, "leds", 2));
> +    qdev_connect_gpio_out(scp1, TOSA_GPIO_WLAN_LED,
> +                          qdev_get_gpio_in_named(misc_gpio, "leds", 3));
>  
>      qdev_connect_gpio_out(scp1, TOSA_GPIO_TC6393XB_L3V_ON, 
> tc6393xb_l3v_get(tmio));
>  
> @@ -287,10 +315,22 @@ static const TypeInfo tosa_ssp_info = {
>      .class_init    = tosa_ssp_class_init,
>  };
>  
> +static const TypeInfo tosa_misc_gpio_info = {
> +    .name          = "tosa-misc-gpio",
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(TosaMiscGPIOState),
> +    .instance_init = tosa_misc_gpio_init,
> +    /*
> +     * No class init required: device has no internal state so does not
> +     * need to set up reset or vmstate, and has no realize method.
> +     */
> +};
> +
>  static void tosa_register_types(void)
>  {
>      type_register_static(&tosa_dac_info);
>      type_register_static(&tosa_ssp_info);
> +    type_register_static(&tosa_misc_gpio_info);
>  }
>  
>  type_init(tosa_register_types)
>