Re: [Qemu-devel] [PATCH V1 02/10] qemu-clk: allow to attach a clock to a device

[KONRAD Frederic]

Le 17/10/2016 à 20:13, Peter Maydell a écrit :
> On 5 October 2016 at 23:10,  <address@hidden> wrote:
> > From: KONRAD Frederic <address@hidden>
> >
> > 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 <address@hidden>
> > ---
> >  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