Re: [Qemu-devel] [PATCH v2 1/2] qdev: Implement named GPIOs
On Tue, Apr 29, 2014 at 8:52 AM, Peter Crosthwaite wrote: > On Tue, Apr 29, 2014 at 12:54 AM, Peter Maydell > wrote: >> On 28 April 2014 01:45, Peter Crosthwaite >> 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 >>
Re: [Qemu-devel] [PATCH v2 1/2] qdev: Implement named GPIOs
On Tue, Apr 29, 2014 at 12:54 AM, Peter Maydell wrote: > On 28 April 2014 01:45, Peter Crosthwaite > 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 >
Re: [Qemu-devel] [PATCH v2 1/2] qdev: Implement named GPIOs
On 28 April 2014 01:45, Peter Crosthwaite 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
