Re: [Qemu-devel] [PATCH v2 1/2] qdev: Implement named GPIOs

[Peter Maydell]

On 28 April 2014 01:45, Peter Crosthwaite <address@hidden> wrote:
> Implement named GPIOs on the Device layer. Listifies the existing GPIOs
> stuff using string keys. Legacy un-named GPIOs are preserved by using
> a NULL name string - they are just a single matchable element in the
> name list.

> @@ -252,7 +260,11 @@ void qdev_machine_creation_done(void);
>  bool qdev_machine_modified(void);
>
>  qemu_irq qdev_get_gpio_in(DeviceState *dev, int n);
> +qemu_irq qdev_get_gpio_in_named(DeviceState *dev, int n, const char *name);
> +
>  void qdev_connect_gpio_out(DeviceState *dev, int n, qemu_irq pin);
> +void qdev_connect_gpio_out_named(DeviceState *dev, int n, qemu_irq pin,
> +                                 const char *name);
>
>  BusState *qdev_get_child_bus(DeviceState *dev, const char *name);
>
> @@ -262,6 +274,10 @@ BusState *qdev_get_child_bus(DeviceState *dev, const 
> char *name);
>  /* GPIO inputs also double as IRQ sinks.  */
>  void qdev_init_gpio_in(DeviceState *dev, qemu_irq_handler handler, int n);
>  void qdev_init_gpio_out(DeviceState *dev, qemu_irq *pins, int n);
> +void qdev_init_gpio_in_named(DeviceState *dev, qemu_irq_handler handler, int 
> n,
> +                             const char *name);
> +void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins, int n,
> +                              const char *name);
>
>  BusState *qdev_get_parent_bus(DeviceState *dev);

I like being able to have named GPIO pins. I have two
areas of concern here:

(1) is this going in the wrong direction for some potential
future change to use QOM child properties later?

(2) (related) is the API for boards and devices correct,
so it won't need further global changes if we reimplement
the mechanism later (possibly including using child props)?

If we want a simple "just add named GPIOs" change then this
patch and API seems the obvious one. I would reorder the
arguments so that all the _named functions take "name, n"
in that order where the non-named versions have just "n",
but that's a nitpick.

[This last bit is something of a tangent/distraction:

One possibility that I think has been suggested in the
past is that we make some fields in the device state structs
"public", so that code using them could say
  thisdev->timer_outputs[3]
rather than having to call a function passing it "timer_outputs", 3.
(I guess this would also imply having some kind of QOM
property definitions so as to allow introspection and generally
non-C-code access.)

]

thanks
-- PMM