On Tue, Aug 12, 2014 at 7:24 PM, Alexander Graf <ag...@suse.de> wrote:
>
> On 04.08.14 03:58, Peter Crosthwaite wrote:
>>
>> Allows a container to take ownership of GPIOs in a contained
>> device and automatically connect them as GPIOs to the container.
>>
>> This prepares for deprecation of the SYSBUS IRQ functionality, which
>> has this feature. We push it up to the device level instead of sysbus
>> level. There's nothing sysbus specific about passing GPIOs to
>> containers so its a legitimate device-level generic feature.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwa...@xilinx.com>
>> ---
>>
>> hw/core/qdev.c | 28 ++++++++++++++++++++++++++++
>> include/hw/qdev-core.h | 3 +++
>> 2 files changed, 31 insertions(+)
>>
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index bf2c227..708363f 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -440,6 +440,34 @@ void qdev_connect_gpio_out(DeviceState * dev, int n,
>> qemu_irq pin)
>> qdev_connect_gpio_out_named(dev, NULL, n, pin);
>> }
>> +void qdev_pass_gpios(DeviceState *dev, DeviceState *container,
>> + const char *name)
>> +{
>> + int i;
>> + NamedGPIOList *ngl = qdev_get_named_gpio_list(dev, name);
>> +
>> + for (i = 0; i < ngl->num_in; i++) {
>> + char *propname = g_strdup_printf("%s[%d]",
>> + ngl->name ? ngl->name :
>> + "unnamed-gpio-in",
>
>
> Really just a minor nit, but I think the code flow would look a lot nicer if
> you did the name check in a separate variable.
>
> const char *name = ngl->name ? ngl->name : "unnamed-gpio-in";
I think I may even go an extra step and get it macroified. How about:
#define QDEV_GPIO_IN_NAME(a) ((a) ? (a) : "unnamed-gpio-io")
and then you can continue to use it inline without extra variables or
nasty "?:"?
> for (i = 0; ...) {
> char *propname = g_strdup_printf("%s[%d]", name, i);
> ....
> }
>
> Also I don't fully grasp what the naming scheme is supposed to be here. Who
> sets the name and why is there only a single global name for all GPIOs?
>
Ideally, the instantiating device sets the names. There's not a global
name for all GPIOs, just an over-used default. The intention is that
qdev_init_gpio_in is phased out in favor of qdev_init_gpio_in_named.
If NULL name is given or qdev_init_gpio_in is used, it defaults to
this single global name here. All sysbus IRQs also share a single
name (seperate from the qdev default) but that sharing is implemented
on the sysbus level and transparent to qdev. The need for a default
name is to appease QOM, which needs a valid string for canonical path.
Regards,
Peter
>
> Alex
>
>
>> + i);
>> + object_property_add_alias(OBJECT(container), propname,
>> + OBJECT(dev), propname,
>> + &error_abort);
>> + }
>> + for (i = 0; i < ngl->num_out; i++) {
>> + char *propname = g_strdup_printf("%s[%d]",
>> + ngl->name ? ngl->name :
>> + "unnamed-gpio-in",
>> + i);
>> + object_property_add_alias(OBJECT(container), propname,
>> + OBJECT(dev), propname,
>> + &error_abort);
>> + }
>> + QLIST_REMOVE(ngl, node);
>> + QLIST_INSERT_HEAD(&container->gpios, ngl, node);
>> +}
>> +
>> BusState *qdev_get_child_bus(DeviceState *dev, const char *name)
>> {
>> BusState *bus;
>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> index d3326b1..08dafda 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -289,6 +289,9 @@ void qdev_init_gpio_in_named(DeviceState *dev,
>> qemu_irq_handler handler,
>> void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
>> const char *name, int n);
>> +void qdev_pass_gpios(DeviceState *dev, DeviceState *container,
>> + const char *name);
>> +
>> BusState *qdev_get_parent_bus(DeviceState *dev);
>> /*** BUS API. ***/
>
>
>
