Message-ID: <20131128093224.GA26634@ulmo.nvidia.com>
Date:	Thu, 28 Nov 2013 10:32:41 +0100
From:	Thierry Reding <thierry.reding@...il.com>
To:	Marc Dietrich <Marc.Dietrich@...physik.uni-giessen.de>
Cc:	Alexandre Courbot <gnurou@...il.com>,
	Rhyland Klein <rklein@...dia.com>,
	Mika Westerberg <mika.westerberg@...ux.intel.com>,
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Linus Walleij <linus.walleij@...aro.org>,
	Chris Ball <cjb@...top.org>,
	Johannes Berg <johannes@...solutions.net>,
	Adrian Hunter <adrian.hunter@...el.com>,
	Alex Courbot <acourbot@...dia.com>,
	Mathias Nyman <mathias.nyman@...ux.intel.com>,
	Rob Landley <rob@...dley.net>,
	Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
	Stephen Warren <swarren@...dotorg.org>,
	"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 1/6] ARM: tegra: add gpiod_lookup table for paz00

On Thu, Nov 28, 2013 at 10:09:14AM +0100, Marc Dietrich wrote:
> Hi,
> 
> Am Donnerstag, 28. November 2013, 11:47:34 schrieb Alexandre Courbot:
> > On Thu, Nov 28, 2013 at 1:47 AM, Rhyland Klein <rklein@...dia.com> wrote:
> > > On 11/26/2013 5:05 AM, Mika Westerberg wrote:
> > >> From: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > >> 
> > >> This makes it possible to request the gpio descriptors in
> > >> rfkill_gpio driver regardless of the platform.
> > >> 
> > >> Signed-off-by: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > >> Signed-off-by: Mika Westerberg <mika.westerberg@...ux.intel.com>
> > >> Tested-by: Stephen Warren <swarren@...dia.com>
> > >> ---
> > >> 
> > >>  arch/arm/mach-tegra/board-paz00.c | 7 +++++++
> > >>  1 file changed, 7 insertions(+)
> > >> 
> > >> diff --git a/arch/arm/mach-tegra/board-paz00.c
> > >> b/arch/arm/mach-tegra/board-paz00.c index 06f024070dab..a309795da665
> > >> 100644
> > >> --- a/arch/arm/mach-tegra/board-paz00.c
> > >> +++ b/arch/arm/mach-tegra/board-paz00.c
> > >> @@ -18,6 +18,7 @@
> > >> 
> > >>   */
> > >>  
> > >>  #include <linux/platform_device.h>
> > >> 
> > >> +#include <linux/gpio/driver.h>
> > >> 
> > >>  #include <linux/rfkill-gpio.h>
> > >>  #include "board.h"
> > >> 
> > >> @@ -36,7 +37,13 @@ static struct platform_device wifi_rfkill_device = {
> > >> 
> > >>       },
> > >>  
> > >>  };
> > >> 
> > >> +static struct gpiod_lookup wifi_gpio_lookup[] = {
> > >> +     GPIO_LOOKUP_IDX("tegra-gpio", 25, "rfkill_gpio", NULL, 0, 0),
> > >> +     GPIO_LOOKUP_IDX("tegra-gpio", 85, "rfkill_gpio", NULL, 1, 0),
> > >> +};
> > > 
> > > I wouldn't think this table would match for the gpios as the driver
> > > currently is. From what I see, the driver calls into gpiod_get_index,
> > > which will try 1 of 3 ways of getting the gpios:
> > > 
> > > of-enabled: of_find_gpio
> > > 
> > >         - which I believe wouldn't work for paz00, since rfkill
> > >         
> > >           doesn't support dt?
> > > 
> > > acpi: acpi_find_gpio
> > > 
> > >         - I assume this does work, but I didn't dive into it
> > > 
> > > gpiod lookup table: gpiod_find
> > > 
> > >         - I think this is the path we expect to be taken, given the
> > >         addition of
> > > 
> > > the lookup table here, but I don't think it would actually match.
> > > Looking at the code for gpiod_find, it seems like it would try to match
> > > the con_id, but would fail. Patch 2/6 is passing the reset_name and
> > > shutdown_name for the con_ids, which isn't what is registered in this
> > > table.
> > > 
> > > Shouldn't it look more like this?
> > > 
> > > +static struct gpiod_lookup wifi_gpio_lookup[] = {
> > > +       GPIO_LOOKUP_IDX("tegra-gpio", 25, "rfkill_gpio_reset", NULL, 0,
> > > 0),
> > > +       GPIO_LOOKUP_IDX("tegra-gpio", 85, "rfkill_gpio_shutdown", NULL, 1,
> > > 0), +};
> > 
> > The original GPIO lookup table specifies the device name
> > ("rfkill_gpio"), no con_id, and an index. gpiod_find() will ensure
> > that the device names match, skip the con_id (as it is null in the
> > table), and again require that the indexes are the same. So provided
> > the instanciated device's dev_name() is "rfkill_gpio" (which it seems
> > to be according to the driver - not sure if it could become
> > "rfkill_gpio.0"), then I'd say the GPIOs will match. The con_id passed
> > to gpiod_get() will only be used as a label for debugfs. I am not sure
> > where the "rfkill_gpio_reset" you mention comes from?
> > 
> > One could wonder why the names of the GPIO are not hardcoded in the
> > driver instead of being defined as platform data, but that point could
> > be improved in a future patch.
> > 
> > It is true however that the platform GPIO lookup mechanism is
> > confusing at best, on top of being inefficient (one big linked list).
> > I have a patch in the pipe that should improve both points by making
> > GPIO lookup tables defined per-device - don't hesitate to merge this
> > series first if it works though.
> 
> I'll try to apply the patches and check on the actual hw. rfkill userspace 
> command should be able to enable/disable the gpio. No idea how it finds the 
> required gpio names.
> 
> The real problem with the rfkill driver is that it does not support DT. A 
> naive attempt to convert it was made some year ago, but got rejected because 
> rfkill wasn't seen as isolated device which can be represented in the device-
> tree. Also it can not be added under some existing device node (e.g. the wifi 
> driver) because those devices sit normally on an "enumeratable" bus (e.g. usb, 
> pci), which is not listed in the device tree at all. This is why it still 
> requires a board file and platform_data. I wish we could find a solution for 
> this.

There is a solution at least for PCI. You can list PCI devices within
the device tree, which is really handy (required even) if for instance
one of the PCI devices is an SPI or I2C controller, each providing a bus
that cannot be probed. What you usually do is describe the PCI hierarchy
at least up to the controller and then list slaves as child nodes.

I'm not aware of anything similar for USB, but it should certainly be
possible to come up with a standard binding for the USB bus. It has a
topology that's similar enough to that of PCI so that the same general
rules could be applied.

If that's really the only thing that keeps rfkill from gaining DT
support then it's something worth tackling in my opinion.

Thierry

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists