On Tue, Apr 29, 2014 at 8:52 AM, Peter Crosthwaite <peter.crosthwa...@xilinx.com> wrote: > On Tue, Apr 29, 2014 at 12:54 AM, Peter Maydell > <peter.mayd...@linaro.org> wrote: >> On 28 April 2014 01:45, Peter Crosthwaite <peter.crosthwa...@xilinx.com> >> 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 >>
