From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57902) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XH9kA-0005Tf-5V for qemu-devel@nongnu.org; Tue, 12 Aug 2014 06:55:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XH9k4-00025j-5R for qemu-devel@nongnu.org; Tue, 12 Aug 2014 06:55:53 -0400 Received: from cantor2.suse.de ([195.135.220.15]:41971 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XH9k3-00024u-QI for qemu-devel@nongnu.org; Tue, 12 Aug 2014 06:55:48 -0400 Message-ID: <53E9F2B2.5020501@suse.de> Date: Tue, 12 Aug 2014 12:55:46 +0200 From: Alexander Graf MIME-Version: 1.0 References: <430adeb742bf80def5cc117a766fa79171e8ecc8.1407116648.git.peter.crosthwaite@xilinx.com> <53E9DD5D.3040806@suse.de> In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v1 13/16] qdev: gpio: Define qdev_pass_gpios() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Crosthwaite Cc: Peter Maydell , Paolo Bonzini , "qemu-devel@nongnu.org Developers" , =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= On 12.08.14 12:48, Peter Crosthwaite wrote: > On Tue, Aug 12, 2014 at 7:24 PM, Alexander Graf 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 >>> --- >>> >>> 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 "?:"? The variable will get optimized out, so for the sake of readability I would still vote to have it around. Whether you declare it via a macro or with an a ? a : b macro I don't really care much about :). In fact, it might make more sense to literally have the typical (a ? a : b) flow macroified in a generic place somewhere. > >> 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. Ah, I missed the fact that qdev_get_named_gpio_list() returns a list of elements with exactly the name you searched for in the first place. Alex