[V2,02/10] qemu-clk: allow to add a clock to a device

From: KONRAD Frederic <fred.konrad@greensocs.com>

This allows to add 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.

Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>

V1 -> V2:
  * Rename the function use 'add' instead of 'attach'
---
 include/qemu/qemu-clock.h | 24 +++++++++++++++++++++++-
 qemu-clock.c              | 23 +++++++++++++++++++++++
 2 files changed, 46 insertions(+), 1 deletion(-)

On 01/26/2017 10:47 AM, fred.konrad@greensocs.com wrote:
> From: KONRAD Frederic <fred.konrad@greensocs.com>
> 
> This allows to add 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.
> 
> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
> 
> V1 -> V2:
>   * Rename the function use 'add' instead of 'attach'
> ---
>  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..3e692d3 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_device_add_clock:
> + * @dev: the device on which the clock needs to be added.
> + * @clk: the clock which needs to be added.
> + * @name: the name of the clock can't be NULL.
> + *
> + * Add @clk to device @dev as a clock named @name.
> + *
> + */
> +void qemu_clk_device_add_clock(DeviceState *dev, qemu_clk clk,
> +                               const char *name);
>  
> +/**
> + * qemu_clk_device_get_clock:
> + * @dev: the device which contains the clock.
> + * @name: the name of the clock.
> + *
> + * Get the clock named @name contained in the device @dev, or NULL if not found.
> + *
> + * Returns the clock named @name contained in @dev.
> + */
> +qemu_clk qemu_clk_device_get_clock(DeviceState *dev, const char *name);
>  
> +#endif /* QEMU_CLOCK_H */
> diff --git a/qemu-clock.c b/qemu-clock.c
> index ceea98d..803deb3 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_device_add_clock(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_device_get_clock(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);
> +}


I see how these routines are used in patch 10/10. But if we were
open coding device CRF_APB, I don't think we would need them at
all and it would make the code a little simple IMHO.

Thanks,

C.  

>  static const TypeInfo qemu_clk_info = {
>      .name          = TYPE_CLOCK,
>      .parent        = TYPE_OBJECT,
>

On 02/06/2017 03:20 PM, Cédric Le Goater wrote:
> On 01/26/2017 10:47 AM, fred.konrad@greensocs.com wrote:
>> From: KONRAD Frederic <fred.konrad@greensocs.com>
>>
>> This allows to add 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.
>>
>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>>
>> V1 -> V2:
>>   * Rename the function use 'add' instead of 'attach'
>> ---
>>  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..3e692d3 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_device_add_clock:
>> + * @dev: the device on which the clock needs to be added.
>> + * @clk: the clock which needs to be added.
>> + * @name: the name of the clock can't be NULL.
>> + *
>> + * Add @clk to device @dev as a clock named @name.
>> + *
>> + */
>> +void qemu_clk_device_add_clock(DeviceState *dev, qemu_clk clk,
>> +                               const char *name);
>>  
>> +/**
>> + * qemu_clk_device_get_clock:
>> + * @dev: the device which contains the clock.
>> + * @name: the name of the clock.
>> + *
>> + * Get the clock named @name contained in the device @dev, or NULL if not found.
>> + *
>> + * Returns the clock named @name contained in @dev.
>> + */
>> +qemu_clk qemu_clk_device_get_clock(DeviceState *dev, const char *name);
>>  
>> +#endif /* QEMU_CLOCK_H */
>> diff --git a/qemu-clock.c b/qemu-clock.c
>> index ceea98d..803deb3 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_device_add_clock(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_device_get_clock(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);
>> +}
> 
> 
> I see how these routines are used in patch 10/10. But if we were
> open coding device CRF_APB, I don't think we would need them at
> all and it would make the code a little simple IMHO.

What do you mean by open coding?

Fred
> 
> Thanks,
> 
> C.  
> 
>>  static const TypeInfo qemu_clk_info = {
>>      .name          = TYPE_CLOCK,
>>      .parent        = TYPE_OBJECT,
>>
> 
>

On 02/07/2017 10:22 AM, Frederic Konrad wrote:
>> I see how these routines are used in patch 10/10. But if we were
>> open coding device CRF_APB, I don't think we would need them at
>> all and it would make the code a little simple IMHO.
> What do you mean by open coding?

I mean to externalize the definition of the objects in a 
header file. 

Then you can replace the type 'Object *' of the attribute 
holding a reference to an instance by a non pointer type.

Anyhow, I think we need to replace object_new() by 
object_initialize() for QOM, and so we need to expose a 
little more the object internals. 

Thanks,

C.

On 02/07/2017 10:31 AM, Cédric Le Goater wrote:
> On 02/07/2017 10:22 AM, Frederic Konrad wrote:
>>> I see how these routines are used in patch 10/10. But if we were
>>> open coding device CRF_APB, I don't think we would need them at
>>> all and it would make the code a little simple IMHO.
>> What do you mean by open coding?
> 
> I mean to externalize the definition of the objects in a 
> header file. 
> 
> Then you can replace the type 'Object *' of the attribute 
> holding a reference to an instance by a non pointer type.
> 
> Anyhow, I think we need to replace object_new() by 
> object_initialize() for QOM, and so we need to expose a 
> little more the object internals. 

Ok got you.

I'll take a look.

Thanks,
Fred

> 
> Thanks,
> 
> C.
> 
>