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

[Peter Crosthwaite]

On Tue, Apr 29, 2014 at 8:52 AM, Peter Crosthwaite
<address@hidden> wrote:
> 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).
>

So amongst my sysbus experiment, I've looked into GPIOs as QOM
objects. It's reasonably orthogonal to this work, and I rather view
this is a stepping stone to a sane API (one that at least involved
names) without a tree-wide. There a few annoying direct accesses
iterators to GPIOs (qtest and qtree) that make the full conversion a
little tricky AFAICS, so this data structure still has a place
alongside QOM linkages.

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

Respinning.

Regards,
Peter

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