Re: [Qemu-devel] [PATCH V1 02/10] qemu-clk: allow to attach a clock to a device
Le 17/10/2016 à 20:13, Peter Maydell a écrit :
On 5 October 2016 at 23:10, wrote:
From: KONRAD Frederic
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
---
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
Re: [Qemu-devel] [PATCH V1 02/10] qemu-clk: allow to attach a clock to a device
On 5 October 2016 at 23:10, wrote:
> From: KONRAD Frederic
>
> 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
> ---
> 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
