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

[Peter Crosthwaite]

On Tue, Apr 29, 2014 at 12:54 AM, Peter Maydell
<address@hidden> wrote:
> 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?
>

I don't think so. Shouldn't be obstructionist. But do you have a link
for the implementation proposal you are referring to here? I worry
about ending up in an already-had conversation here. Ultimately all I
care about is named GPIOs and don't really care how I do it. If there
is a better under the hood implementation that ok for me. Ill just
layer the qemu_foo_gpio APIs on top of it to avoid doing the tree wide
today (then we can just document another coding transition).

> (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.
>

Can we give it a couple of days for further objections then proceed
with the trivial changes and merge? The fact that this series is done
without a tree wide should indicate that we are going from a less
robust to more robust API so on that alone, it's going to simply be
closer to any ideal solution that solves the named GPIO solution. Ill
investigate property driven GPIOs in the meantime. All mailing list
links are welcome.

> [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]

It relys on GPIOs being in the struct and of fixed length. Some are
malloced so it helps to have the qdev wrapping API there to do bounds
checking.

Regards,
Peter

> 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
>