diff mbox series

[v6,5/9] qdev-clock: introduce an init array to ease the device construction

Message ID 20190904125531.27545-6-damien.hedde@greensocs.com
State New
Headers show
Series Clock framework API | expand

Commit Message

Damien Hedde Sept. 4, 2019, 12:55 p.m. UTC 
Introduce a function and macro helpers to setup several clocks
in a device from a static array description.

An element of the array describes the clock (name and direction) as
well as the related callback and an optional offset to store the
created object pointer in the device state structure.

The array must be terminated by a special element QDEV_CLOCK_END.

This is based on the original work of Frederic Konrad.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/core/qdev-clock.c    | 26 ++++++++++++++++
 include/hw/qdev-clock.h | 67 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 93 insertions(+)

Comments

Peter Maydell Dec. 2, 2019, 3:13 p.m. UTC | #1 
On Wed, 4 Sep 2019 at 13:56, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> Introduce a function and macro helpers to setup several clocks
> in a device from a static array description.
>
> An element of the array describes the clock (name and direction) as
> well as the related callback and an optional offset to store the
> created object pointer in the device state structure.
>
> The array must be terminated by a special element QDEV_CLOCK_END.
>
> This is based on the original work of Frederic Konrad.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/core/qdev-clock.c    | 26 ++++++++++++++++
>  include/hw/qdev-clock.h | 67 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 93 insertions(+)
>
> diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c
> index bebdd8fa15..32ad45c061 100644
> --- a/hw/core/qdev-clock.c
> +++ b/hw/core/qdev-clock.c
> @@ -153,3 +153,29 @@ void qdev_connect_clock_out(DeviceState *dev, const char *name, ClockIn *clk,
>
>      clock_connect(clk, clkout);
>  }
> +
> +void qdev_init_clocks(DeviceState *dev, const ClockPortInitArray clocks)
> +{
> +    const struct ClockPortInitElem *elem;
> +
> +    assert(dev);
> +    assert(clocks);

More unnecessary asserts, I think.



> +/**
> + * ClockInitElem:
> + * @name: name of the clock (can't be NULL)
> + * @is_output: indicates whether the clock is input or output
> + * @callback: for inputs, optional callback to be called on clock's update
> + * with device as opaque
> + * @offset: optional offset to store the ClockIn or ClockOut pointer in device
> + * state structure (0 means unused)
> + */
> +struct ClockPortInitElem {
> +    const char *name;
> +    bool is_output;
> +    ClockCallback *callback;
> +    size_t offset;
> +};
> +
> +#define clock_offset_value(_type, _devstate, _field) \
> +    (offsetof(_devstate, _field) + \
> +     type_check(_type *, typeof_field(_devstate, _field)))

Avoid leading underscores, please.

> +
> +#define QDEV_CLOCK(_is_output, _type, _devstate, _field, _callback) { \
> +    .name = (stringify(_field)), \
> +    .is_output = _is_output, \
> +    .callback = _callback, \
> +    .offset = clock_offset_value(_type, _devstate, _field), \
> +}
> +
> +/**
> + * QDEV_CLOCK_(IN|OUT):
> + * @_devstate: structure type. @dev argument of qdev_init_clocks below must be
> + * a pointer to that same type.

It's a bit unclear what "below" here is referring to. Maybe
just have this be "@devstate: name of a C struct type"
and then explain below...

> + * @_field: a field in @_devstate (must be ClockIn* or ClockOut*)
> + * @_callback: (for input only) callback (or NULL) to be called with the device
> + * state as argument
> + *

...here, where we can have a paragraph giving the purpose
of the macro:

"Define an entry in a ClockPortInitArray which is intended
to be passed to qdev_init_clocks(), which should be called
with an @dev argument which is a pointer to the @devstate
struct type."

> + * The name of the clock will be derived from @_field

Derived how? Guessing from the stringify(_field) above that it
will be the same as the field name ?

It makes sense to hardcode the opaque pointer for the callback to be
the device pointer.


> + */
> +#define QDEV_CLOCK_IN(_devstate, _field, _callback) \
> +    QDEV_CLOCK(false, ClockIn, _devstate, _field, _callback)
> +
> +#define QDEV_CLOCK_OUT(_devstate, _field) \
> +    QDEV_CLOCK(true, ClockOut, _devstate, _field, NULL)
> +
> +/**
> + * QDEV_CLOCK_IN_NOFIELD:
> + * @_name: name of the clock
> + * @_callback: callback (or NULL) to be called with the device state as argument
> + */
> +#define QDEV_CLOCK_IN_NOFIELD(_name, _callback) { \
> +    .name = _name, \
> +    .is_output = false, \
> +    .callback = _callback, \
> +    .offset = 0, \
> +}

When would we want to use this one ?

> +
> +#define QDEV_CLOCK_END { .name = NULL }
> +
> +typedef struct ClockPortInitElem ClockPortInitArray[];
> +
> +/**
> + * qdev_init_clocks:
> + * @dev: the device to add clocks

"to add clocks to"

> + * @clocks: a QDEV_CLOCK_END-terminated array which contains the
> + * clocks information.
> + */
> +void qdev_init_clocks(DeviceState *dev, const ClockPortInitArray clocks);
> +
>  #endif /* QDEV_CLOCK_H */
> --
> 2.22.0
>

thanks
-- PMM
Damien Hedde Dec. 4, 2019, 11:04 a.m. UTC | #2 
On 12/2/19 4:13 PM, Peter Maydell wrote:
> On Wed, 4 Sep 2019 at 13:56, Damien Hedde <damien.hedde@greensocs.com> wrote:
>>
>> Introduce a function and macro helpers to setup several clocks
>> in a device from a static array description.
>>
>> An element of the array describes the clock (name and direction) as
>> well as the related callback and an optional offset to store the
>> created object pointer in the device state structure.
>>
>> The array must be terminated by a special element QDEV_CLOCK_END.
>>
>> This is based on the original work of Frederic Konrad.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  hw/core/qdev-clock.c    | 26 ++++++++++++++++
>>  include/hw/qdev-clock.h | 67 +++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 93 insertions(+)
>>
>> diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c
>> index bebdd8fa15..32ad45c061 100644
>> --- a/hw/core/qdev-clock.c
>> +++ b/hw/core/qdev-clock.c
>> @@ -153,3 +153,29 @@ void qdev_connect_clock_out(DeviceState *dev, const char *name, ClockIn *clk,
>>
>>      clock_connect(clk, clkout);
>>  }
>> +
>> +void qdev_init_clocks(DeviceState *dev, const ClockPortInitArray clocks)
>> +{
>> +    const struct ClockPortInitElem *elem;
>> +
>> +    assert(dev);
>> +    assert(clocks);
> 
> More unnecessary asserts, I think.
> 
> 
> 
>> +/**
>> + * ClockInitElem:
>> + * @name: name of the clock (can't be NULL)
>> + * @is_output: indicates whether the clock is input or output
>> + * @callback: for inputs, optional callback to be called on clock's update
>> + * with device as opaque
>> + * @offset: optional offset to store the ClockIn or ClockOut pointer in device
>> + * state structure (0 means unused)
>> + */
>> +struct ClockPortInitElem {
>> +    const char *name;
>> +    bool is_output;
>> +    ClockCallback *callback;
>> +    size_t offset;
>> +};
>> +
>> +#define clock_offset_value(_type, _devstate, _field) \
>> +    (offsetof(_devstate, _field) + \
>> +     type_check(_type *, typeof_field(_devstate, _field)))
> 
> Avoid leading underscores, please.
> 
>> +
>> +#define QDEV_CLOCK(_is_output, _type, _devstate, _field, _callback) { \
>> +    .name = (stringify(_field)), \
>> +    .is_output = _is_output, \
>> +    .callback = _callback, \
>> +    .offset = clock_offset_value(_type, _devstate, _field), \
>> +}
>> +
>> +/**
>> + * QDEV_CLOCK_(IN|OUT):
>> + * @_devstate: structure type. @dev argument of qdev_init_clocks below must be
>> + * a pointer to that same type.
> 
> It's a bit unclear what "below" here is referring to. Maybe
> just have this be "@devstate: name of a C struct type"
> and then explain below...
> 
>> + * @_field: a field in @_devstate (must be ClockIn* or ClockOut*)
>> + * @_callback: (for input only) callback (or NULL) to be called with the device
>> + * state as argument
>> + *
> 
> ...here, where we can have a paragraph giving the purpose
> of the macro:
> 
> "Define an entry in a ClockPortInitArray which is intended
> to be passed to qdev_init_clocks(), which should be called
> with an @dev argument which is a pointer to the @devstate
> struct type."

Sounds good.

> 
>> + * The name of the clock will be derived from @_field
> 
> Derived how? Guessing from the stringify(_field) above that it
> will be the same as the field name ?

yes.

> 
> It makes sense to hardcode the opaque pointer for the callback to be
> the device pointer.
> 
> 
>> + */
>> +#define QDEV_CLOCK_IN(_devstate, _field, _callback) \
>> +    QDEV_CLOCK(false, ClockIn, _devstate, _field, _callback)
>> +
>> +#define QDEV_CLOCK_OUT(_devstate, _field) \
>> +    QDEV_CLOCK(true, ClockOut, _devstate, _field, NULL)
>> +
>> +/**
>> + * QDEV_CLOCK_IN_NOFIELD:
>> + * @_name: name of the clock
>> + * @_callback: callback (or NULL) to be called with the device state as argument
>> + */
>> +#define QDEV_CLOCK_IN_NOFIELD(_name, _callback) { \
>> +    .name = _name, \
>> +    .is_output = false, \
>> +    .callback = _callback, \
>> +    .offset = 0, \
>> +}
> 
> When would we want to use this one ?

If the callback interaction is enough, we don't need to access the clock
object directly. So we don't need the field in the device state
structure. I can remove this macro for sake of simplicity.

--
Damien
diff mbox series

Patch

diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c
index bebdd8fa15..32ad45c061 100644
--- a/hw/core/qdev-clock.c
+++ b/hw/core/qdev-clock.c
@@ -153,3 +153,29 @@  void qdev_connect_clock_out(DeviceState *dev, const char *name, ClockIn *clk,
 
     clock_connect(clk, clkout);
 }
+
+void qdev_init_clocks(DeviceState *dev, const ClockPortInitArray clocks)
+{
+    const struct ClockPortInitElem *elem;
+
+    assert(dev);
+    assert(clocks);
+
+    for (elem = &clocks[0]; elem->name != NULL; elem++) {
+        /* offset cannot be inside the DeviceState part */
+        assert(elem->offset == 0 || elem->offset > sizeof(DeviceState));
+        if (elem->is_output) {
+            ClockOut *clk;
+            clk = qdev_init_clock_out(dev, elem->name);
+            if (elem->offset) {
+                *(ClockOut **)(((void *) dev) + elem->offset) = clk;
+            }
+        } else {
+            ClockIn *clk;
+            clk = qdev_init_clock_in(dev, elem->name, elem->callback, dev);
+            if (elem->offset) {
+                *(ClockIn **)(((void *) dev) + elem->offset) = clk;
+            }
+        }
+    }
+}
diff --git a/include/hw/qdev-clock.h b/include/hw/qdev-clock.h
index c4ea912fdc..b6edb9421b 100644
--- a/include/hw/qdev-clock.h
+++ b/include/hw/qdev-clock.h
@@ -64,4 +64,71 @@  void qdev_connect_clock_out(DeviceState *dev, const char *name, ClockIn *clk,
 void qdev_pass_clock(DeviceState *dev, const char *name,
                      DeviceState *container, const char *cont_name);
 
+/**
+ * ClockInitElem:
+ * @name: name of the clock (can't be NULL)
+ * @is_output: indicates whether the clock is input or output
+ * @callback: for inputs, optional callback to be called on clock's update
+ * with device as opaque
+ * @offset: optional offset to store the ClockIn or ClockOut pointer in device
+ * state structure (0 means unused)
+ */
+struct ClockPortInitElem {
+    const char *name;
+    bool is_output;
+    ClockCallback *callback;
+    size_t offset;
+};
+
+#define clock_offset_value(_type, _devstate, _field) \
+    (offsetof(_devstate, _field) + \
+     type_check(_type *, typeof_field(_devstate, _field)))
+
+#define QDEV_CLOCK(_is_output, _type, _devstate, _field, _callback) { \
+    .name = (stringify(_field)), \
+    .is_output = _is_output, \
+    .callback = _callback, \
+    .offset = clock_offset_value(_type, _devstate, _field), \
+}
+
+/**
+ * QDEV_CLOCK_(IN|OUT):
+ * @_devstate: structure type. @dev argument of qdev_init_clocks below must be
+ * a pointer to that same type.
+ * @_field: a field in @_devstate (must be ClockIn* or ClockOut*)
+ * @_callback: (for input only) callback (or NULL) to be called with the device
+ * state as argument
+ *
+ * The name of the clock will be derived from @_field
+ */
+#define QDEV_CLOCK_IN(_devstate, _field, _callback) \
+    QDEV_CLOCK(false, ClockIn, _devstate, _field, _callback)
+
+#define QDEV_CLOCK_OUT(_devstate, _field) \
+    QDEV_CLOCK(true, ClockOut, _devstate, _field, NULL)
+
+/**
+ * QDEV_CLOCK_IN_NOFIELD:
+ * @_name: name of the clock
+ * @_callback: callback (or NULL) to be called with the device state as argument
+ */
+#define QDEV_CLOCK_IN_NOFIELD(_name, _callback) { \
+    .name = _name, \
+    .is_output = false, \
+    .callback = _callback, \
+    .offset = 0, \
+}
+
+#define QDEV_CLOCK_END { .name = NULL }
+
+typedef struct ClockPortInitElem ClockPortInitArray[];
+
+/**
+ * qdev_init_clocks:
+ * @dev: the device to add clocks
+ * @clocks: a QDEV_CLOCK_END-terminated array which contains the
+ * clocks information.
+ */
+void qdev_init_clocks(DeviceState *dev, const ClockPortInitArray clocks);
+
 #endif /* QDEV_CLOCK_H */