| Message ID | 1475705464-27130-3-git-send-email-fred.konrad@greensocs.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 5 October 2016 at 23:10, <fred.konrad@greensocs.com> wrote: > From: KONRAD Frederic <fred.konrad@greensocs.com> > > This allows to attach a clock to a DeviceState. > Contrary to gpios, the clock pins are not contained in the DeviceState but > with the child property so they can appears in the qom-tree. Is this API patterned along the same lines as some other API we already have ? If so, it would be helpful to mention it in the commit message. (Also this series would probably benefit from somebody who actually knows how the QOM APIs are supposed to work having a look at it.) > Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com> > --- > include/qemu/qemu-clock.h | 24 +++++++++++++++++++++++- > qemu-clock.c | 23 +++++++++++++++++++++++ > 2 files changed, 46 insertions(+), 1 deletion(-) > > diff --git a/include/qemu/qemu-clock.h b/include/qemu/qemu-clock.h > index e7acd68..1d56a2e 100644 > --- a/include/qemu/qemu-clock.h > +++ b/include/qemu/qemu-clock.h > @@ -33,8 +33,30 @@ > typedef struct qemu_clk { > /*< private >*/ > Object parent_obj; > + char *name; /* name of this clock in the device. */ > } *qemu_clk; > > -#endif /* QEMU_CLOCK_H */ > +/** > + * qemu_clk_attach_to_device: > + * @dev: the device on which the clock need to be attached. > + * @clk: the clock which need to be attached. > + * @name: the name of the clock can't be NULL. > + * > + * Attach @clk named @name to the device @dev. I'm not quite sure what this function does, but it looks like what it actually does is attach @clk to device @dev as a clock named @name. > + * > + */ > +void qemu_clk_attach_to_device(DeviceState *dev, qemu_clk clk, > + const char *name); > > +/** > + * qemu_clk_get_pin: > + * @dev: the device which contain the clock. > + * @name: the name of the clock. > + * > + * Get the clock named @name located in the device @dev, or NULL if not found. > + * > + * Returns the clock named @name contained in @dev. > + */ > +qemu_clk qemu_clk_get_pin(DeviceState *dev, const char *name); I don't understand the name of this function. It says "get_pin" but it returns a clock, not a pin ? > +#endif /* QEMU_CLOCK_H */ > diff --git a/qemu-clock.c b/qemu-clock.c > index ceea98d..0ba6caf 100644 > --- a/qemu-clock.c > +++ b/qemu-clock.c > @@ -25,6 +25,7 @@ > #include "qemu/qemu-clock.h" > #include "hw/hw.h" > #include "qemu/log.h" > +#include "qapi/error.h" > > #ifndef DEBUG_QEMU_CLOCK > #define DEBUG_QEMU_CLOCK 0 > @@ -36,6 +37,28 @@ > } \ > } while (0); > > +void qemu_clk_attach_to_device(DeviceState *dev, qemu_clk clk, > + const char *name) > +{ > + assert(name); > + assert(!clk->name); This is really checking that the clock hasn't already been attached to something else, right? > + object_property_add_child(OBJECT(dev), name, OBJECT(clk), &error_abort); > + clk->name = g_strdup(name); When does this string get freed? > +} > + > +qemu_clk qemu_clk_get_pin(DeviceState *dev, const char *name) > +{ > + gchar *path = NULL; > + Object *clk; > + bool ambiguous; > + > + path = g_strdup_printf("%s/%s", object_get_canonical_path(OBJECT(dev)), > + name); > + clk = object_resolve_path(path, &ambiguous); Needs a QOM API expert, but I'm about 90% certain that you don't need to go via the canonical path to get hold of an object that's a child of some other object you already have. (Also object_resolve_path() accepts NULL if you don't care about the return value in 'ambiguous', but I think you'll end up not needing to call it at all.) Possibly you want object_resolve_path_component(OBJECT(dev), name) ? > + g_free(path); > + return QEMU_CLOCK(clk); > +} > + > static const TypeInfo qemu_clk_info = { > .name = TYPE_CLOCK, > .parent = TYPE_OBJECT, > -- > 2.5.5 > thanks -- PMM
Le 17/10/2016 à 20:13, Peter Maydell a écrit : > On 5 October 2016 at 23:10, <fred.konrad@greensocs.com> wrote: >> From: KONRAD Frederic <fred.konrad@greensocs.com> >> >> This allows to attach a clock to a DeviceState. >> Contrary to gpios, the clock pins are not contained in the DeviceState but >> with the child property so they can appears in the qom-tree. > > Is this API patterned along the same lines as some other > API we already have ? If so, it would be helpful to mention > it in the commit message. > > (Also this series would probably benefit from somebody who > actually knows how the QOM APIs are supposed to work having > a look at it.) I CC'ed Andreas maybe he can help. > >> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com> >> --- >> include/qemu/qemu-clock.h | 24 +++++++++++++++++++++++- >> qemu-clock.c | 23 +++++++++++++++++++++++ >> 2 files changed, 46 insertions(+), 1 deletion(-) >> >> diff --git a/include/qemu/qemu-clock.h b/include/qemu/qemu-clock.h >> index e7acd68..1d56a2e 100644 >> --- a/include/qemu/qemu-clock.h >> +++ b/include/qemu/qemu-clock.h >> @@ -33,8 +33,30 @@ >> typedef struct qemu_clk { >> /*< private >*/ >> Object parent_obj; >> + char *name; /* name of this clock in the device. */ >> } *qemu_clk; >> >> -#endif /* QEMU_CLOCK_H */ >> +/** >> + * qemu_clk_attach_to_device: >> + * @dev: the device on which the clock need to be attached. >> + * @clk: the clock which need to be attached. >> + * @name: the name of the clock can't be NULL. >> + * >> + * Attach @clk named @name to the device @dev. > > I'm not quite sure what this function does, but it looks like > what it actually does is attach @clk to device @dev as a > clock named @name. > >> + * >> + */ >> +void qemu_clk_attach_to_device(DeviceState *dev, qemu_clk clk, >> + const char *name); >> >> +/** >> + * qemu_clk_get_pin: >> + * @dev: the device which contain the clock. >> + * @name: the name of the clock. >> + * >> + * Get the clock named @name located in the device @dev, or NULL if not found. >> + * >> + * Returns the clock named @name contained in @dev. >> + */ >> +qemu_clk qemu_clk_get_pin(DeviceState *dev, const char *name); > > I don't understand the name of this function. It says "get_pin" > but it returns a clock, not a pin ? Yes actually you're right. Only we only use this function outside the model. In the machine when you want to connects the clock model together. That's why I called it like that. But I can change if you want. > >> +#endif /* QEMU_CLOCK_H */ >> diff --git a/qemu-clock.c b/qemu-clock.c >> index ceea98d..0ba6caf 100644 >> --- a/qemu-clock.c >> +++ b/qemu-clock.c >> @@ -25,6 +25,7 @@ >> #include "qemu/qemu-clock.h" >> #include "hw/hw.h" >> #include "qemu/log.h" >> +#include "qapi/error.h" >> >> #ifndef DEBUG_QEMU_CLOCK >> #define DEBUG_QEMU_CLOCK 0 >> @@ -36,6 +37,28 @@ >> } \ >> } while (0); >> >> +void qemu_clk_attach_to_device(DeviceState *dev, qemu_clk clk, >> + const char *name) >> +{ >> + assert(name); >> + assert(!clk->name); > > This is really checking that the clock hasn't already been attached > to something else, right? Yes. > >> + object_property_add_child(OBJECT(dev), name, OBJECT(clk), &error_abort); >> + clk->name = g_strdup(name); > > When does this string get freed? Nowhere actually! I'll fix that. > >> +} >> + >> +qemu_clk qemu_clk_get_pin(DeviceState *dev, const char *name) >> +{ >> + gchar *path = NULL; >> + Object *clk; >> + bool ambiguous; >> + >> + path = g_strdup_printf("%s/%s", object_get_canonical_path(OBJECT(dev)), >> + name); >> + clk = object_resolve_path(path, &ambiguous); > > Needs a QOM API expert, but I'm about 90% certain that you > don't need to go via the canonical path to get hold of > an object that's a child of some other object you already > have. (Also object_resolve_path() accepts NULL if you > don't care about the return value in 'ambiguous', but I > think you'll end up not needing to call it at all.) > Possibly you want object_resolve_path_component(OBJECT(dev), name) ? > >> + g_free(path); >> + return QEMU_CLOCK(clk); >> +} >> + >> static const TypeInfo qemu_clk_info = { >> .name = TYPE_CLOCK, >> .parent = TYPE_OBJECT, >> -- >> 2.5.5 >> > > thanks > -- PMM >
diff --git a/include/qemu/qemu-clock.h b/include/qemu/qemu-clock.h index e7acd68..1d56a2e 100644 --- a/include/qemu/qemu-clock.h +++ b/include/qemu/qemu-clock.h @@ -33,8 +33,30 @@ typedef struct qemu_clk { /*< private >*/ Object parent_obj; + char *name; /* name of this clock in the device. */ } *qemu_clk; -#endif /* QEMU_CLOCK_H */ +/** + * qemu_clk_attach_to_device: + * @dev: the device on which the clock need to be attached. + * @clk: the clock which need to be attached. + * @name: the name of the clock can't be NULL. + * + * Attach @clk named @name to the device @dev. + * + */ +void qemu_clk_attach_to_device(DeviceState *dev, qemu_clk clk, + const char *name); +/** + * qemu_clk_get_pin: + * @dev: the device which contain the clock. + * @name: the name of the clock. + * + * Get the clock named @name located in the device @dev, or NULL if not found. + * + * Returns the clock named @name contained in @dev. + */ +qemu_clk qemu_clk_get_pin(DeviceState *dev, const char *name); +#endif /* QEMU_CLOCK_H */ diff --git a/qemu-clock.c b/qemu-clock.c index ceea98d..0ba6caf 100644 --- a/qemu-clock.c +++ b/qemu-clock.c @@ -25,6 +25,7 @@ #include "qemu/qemu-clock.h" #include "hw/hw.h" #include "qemu/log.h" +#include "qapi/error.h" #ifndef DEBUG_QEMU_CLOCK #define DEBUG_QEMU_CLOCK 0 @@ -36,6 +37,28 @@ } \ } while (0); +void qemu_clk_attach_to_device(DeviceState *dev, qemu_clk clk, + const char *name) +{ + assert(name); + assert(!clk->name); + object_property_add_child(OBJECT(dev), name, OBJECT(clk), &error_abort); + clk->name = g_strdup(name); +} + +qemu_clk qemu_clk_get_pin(DeviceState *dev, const char *name) +{ + gchar *path = NULL; + Object *clk; + bool ambiguous; + + path = g_strdup_printf("%s/%s", object_get_canonical_path(OBJECT(dev)), + name); + clk = object_resolve_path(path, &ambiguous); + g_free(path); + return QEMU_CLOCK(clk); +} + static const TypeInfo qemu_clk_info = { .name = TYPE_CLOCK, .parent = TYPE_OBJECT,