| Message ID | 1351928793-14375-2-git-send-email-mika.westerberg@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Sat, Nov 3, 2012 at 8:46 AM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > +/** > + * acpi_get_gpio() - Translate ACPI GPIO pin to GPIO number usable with GPIO API > + * @path: ACPI GPIO controller full path name, (e.g. "\\_SB.GPO1") > + * @pin: ACPI GPIO pin number (0-based, controller-relative) > + * > + * Returns GPIO number to use with Linux generic GPIO API, or errno error value > + */ So by just looking at that we can see that this is yet another instance of papering over the fact that the Linux GPIO numbers are global to the kernel and not per-chip, as would be preferred. Can you please contribute to the parallel discussion on how to get rid of the global GPIO numberspace, thread named: "How about a gpio_get(device *, char *) function?" Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/05/2012 01:53 PM, Linus Walleij wrote: > On Sat, Nov 3, 2012 at 8:46 AM, Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > >> +/** >> + * acpi_get_gpio() - Translate ACPI GPIO pin to GPIO number usable with GPIO API >> + * @path: ACPI GPIO controller full path name, (e.g. "\\_SB.GPO1") >> + * @pin: ACPI GPIO pin number (0-based, controller-relative) >> + * >> + * Returns GPIO number to use with Linux generic GPIO API, or errno error value >> + */ > > So by just looking at that we can see that this is yet another > instance of papering > over the fact that the Linux GPIO numbers are global to the kernel and not > per-chip, as would be preferred. Yes, it is. ACPI5 GPIO resources are numbered per-chip. This is just a way for device drivers to find the corresponding Linux GPIO. per-chip based numbering sounds saner, but this deals with what we currently have. > > Can you please contribute to the parallel discussion on how to get rid of > the global GPIO numberspace, thread named: > "How about a gpio_get(device *, char *) function?" I'll take a look at it, (and see if I got anything of value to add) -Mathias -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday, November 05, 2012 02:14:21 PM Mathias Nyman wrote: > On 11/05/2012 01:53 PM, Linus Walleij wrote: > > On Sat, Nov 3, 2012 at 8:46 AM, Mika Westerberg > > <mika.westerberg@linux.intel.com> wrote: > > > >> +/** > >> + * acpi_get_gpio() - Translate ACPI GPIO pin to GPIO number usable with GPIO API > >> + * @path: ACPI GPIO controller full path name, (e.g. "\\_SB.GPO1") > >> + * @pin: ACPI GPIO pin number (0-based, controller-relative) > >> + * > >> + * Returns GPIO number to use with Linux generic GPIO API, or errno error value > >> + */ > > > > So by just looking at that we can see that this is yet another > > instance of papering > > over the fact that the Linux GPIO numbers are global to the kernel and not > > per-chip, as would be preferred. > > Yes, it is. ACPI5 GPIO resources are numbered per-chip. This is just a > way for device drivers to find the corresponding Linux GPIO. > > per-chip based numbering sounds saner, but this deals with what we > currently have. And we need something to hook up drivers to right now. Thanks, Rafael
On Mon, Nov 5, 2012 at 1:46 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > On Monday, November 05, 2012 02:14:21 PM Mathias Nyman wrote: >> per-chip based numbering sounds saner, but this deals with what we >> currently have. > > And we need something to hook up drivers to right now. So speaking of it I have these drivers consuming pinctrl that I need to hook up right now and the subsystem maintainers are NACKing the patches because they want a centralized solution instead of per-driver hooks and I get the red light... Do you think they will change their mind and give me green light if I tell them I just need to do it right now? ;-) I suggest to take it easy and prolong the deadline, but none of my business primarily, you will have to go through Grant who is probably way more reasonable than me. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday, November 05, 2012 02:11:03 PM Linus Walleij wrote: > On Mon, Nov 5, 2012 at 1:46 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > On Monday, November 05, 2012 02:14:21 PM Mathias Nyman wrote: > > >> per-chip based numbering sounds saner, but this deals with what we > >> currently have. > > > > And we need something to hook up drivers to right now. > > So speaking of it I have these drivers consuming pinctrl that I need > to hook up right now and the subsystem maintainers are NACKing > the patches because they want a centralized solution instead of > per-driver hooks and I get the red light... > > Do you think they will change their mind and give me green light if > I tell them I just need to do it right now? ;-) Well, it's just a matter of fairness to me, actually. If you allowed somebody to do something in the past, it's simply unfair to forbid someone else to do a similar thing later, isn't it? Rafael
On Mon, Nov 5, 2012 at 2:19 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > On Monday, November 05, 2012 02:11:03 PM Linus Walleij wrote: >> Do you think they will change their mind and give me green light if >> I tell them I just need to do it right now? ;-) > > Well, it's just a matter of fairness to me, actually. > > If you allowed somebody to do something in the past, it's simply unfair > to forbid someone else to do a similar thing later, isn't it? So if some subsystem has previously merged clk_get() for the silicon clock pertaining to a driver the maintainer should be fair to the pinctrl people and merge pinctrl_get() as well. Well, they don't. But it wasn't any of your subsystems so I don't blame you. I have been stand-in-maintaining the GPIO subsystem for the last two merge windows and I am worrying about the long term viability of the subsystem if we keep doing this without facing the real problem of the global GPIO numberspace. But since Grant merged the gpiolib-of.c thing and is still the main maintainer I can atleast chicken out by referring to him this time ... hit me back if he doesn't respond and I will have to refactor the world or something. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday, November 05, 2012 02:28:45 PM Linus Walleij wrote: > On Mon, Nov 5, 2012 at 2:19 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > On Monday, November 05, 2012 02:11:03 PM Linus Walleij wrote: > >> Do you think they will change their mind and give me green light if > >> I tell them I just need to do it right now? ;-) > > > > Well, it's just a matter of fairness to me, actually. > > > > If you allowed somebody to do something in the past, it's simply unfair > > to forbid someone else to do a similar thing later, isn't it? > > So if some subsystem has previously merged clk_get() for > the silicon clock pertaining to a driver the maintainer should > be fair to the pinctrl people and merge pinctrl_get() as well. Well, let's just say if I were the maintainer of the subsystem in question, I would propose a deal like this: I'm going to merge your patches now, provided that you'll work on the centralized solution going forward. And it's quite easy to call them on the promise like this in the future, when they ask you to merge something again. > Well, they don't. But it wasn't any of your subsystems so > I don't blame you. > > I have been stand-in-maintaining the GPIO subsystem > for the last two merge windows and I am worrying about the > long term viability of the subsystem if we keep doing this > without facing the real problem of the global GPIO > numberspace. > > But since Grant merged the gpiolib-of.c thing and is still the > main maintainer I can atleast chicken out by referring to him > this time ... hit me back if he doesn't respond and I will > have to refactor the world or something. No need to be grumpy. :-) I forgot to mention that we want to hook up _existing_ drivers to those things, and they already use the global GPIO numbers, don't they? Rafael
On Mon, Nov 5, 2012 at 2:50 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > On Monday, November 05, 2012 02:28:45 PM Linus Walleij wrote: >> But since Grant merged the gpiolib-of.c thing and is still the >> main maintainer I can atleast chicken out by referring to him >> this time ... hit me back if he doesn't respond and I will >> have to refactor the world or something. > > No need to be grumpy. :-) Just a little bit ... sorry. > I forgot to mention that we want to hook up _existing_ drivers to those things, > and they already use the global GPIO numbers, don't they? Yes they do, usually this is either passed from the platform using platform data or handled by device tree lookups to individual drivers. So you will have to modify each such existing driver to do ACPI probe akin to the DT codepath and call acpi_get_gpio() on every pin they need going forward. But that is the plan I guess. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 05, 2012 at 03:40:14PM +0100, Linus Walleij wrote: > > I forgot to mention that we want to hook up _existing_ drivers to those things, > > and they already use the global GPIO numbers, don't they? > > Yes they do, usually this is either passed from the platform using platform > data or handled by device tree lookups to individual drivers. > > So you will have to modify each such existing driver to do ACPI > probe akin to the DT codepath and call acpi_get_gpio() on every pin they > need going forward. But that is the plan I guess. Yes, that's the plan. Do you think it is OK to go with this implementation (acpi_get_gpio()) for now? We will try to make sure that the gpio_get() (or whatever it will be called that time) supports ACPI as well. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 6, 2012 at 10:39 AM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > On Mon, Nov 05, 2012 at 03:40:14PM +0100, Linus Walleij wrote: >> > I forgot to mention that we want to hook up _existing_ drivers to those things, >> > and they already use the global GPIO numbers, don't they? >> >> Yes they do, usually this is either passed from the platform using platform >> data or handled by device tree lookups to individual drivers. >> >> So you will have to modify each such existing driver to do ACPI >> probe akin to the DT codepath and call acpi_get_gpio() on every pin they >> need going forward. But that is the plan I guess. > > Yes, that's the plan. > > Do you think it is OK to go with this implementation (acpi_get_gpio()) for > now? We will try to make sure that the gpio_get() (or whatever it will be > called that time) supports ACPI as well. Yes I'll be OK with it but I don't dare to merge it unless Grant ACKs it. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 06, 2012 at 11:15:21AM +0100, Linus Walleij wrote: > On Tue, Nov 6, 2012 at 10:39 AM, Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > On Mon, Nov 05, 2012 at 03:40:14PM +0100, Linus Walleij wrote: > >> > I forgot to mention that we want to hook up _existing_ drivers to those things, > >> > and they already use the global GPIO numbers, don't they? > >> > >> Yes they do, usually this is either passed from the platform using platform > >> data or handled by device tree lookups to individual drivers. > >> > >> So you will have to modify each such existing driver to do ACPI > >> probe akin to the DT codepath and call acpi_get_gpio() on every pin they > >> need going forward. But that is the plan I guess. > > > > Yes, that's the plan. > > > > Do you think it is OK to go with this implementation (acpi_get_gpio()) for > > now? We will try to make sure that the gpio_get() (or whatever it will be > > called that time) supports ACPI as well. > > Yes I'll be OK with it but I don't dare to merge it unless Grant > ACKs it. Is that an Ack from you? ;-) Since there is a dependency to linux-pm tree (we use the new dev->acpi_handle member) I would like this series merged via that tree. Grant, are you OK with this? -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mika, On Sat, Nov 3, 2012 at 7:46 AM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > From: Mathias Nyman <mathias.nyman@linux.intel.com> > > Add support for translating ACPI GPIO pin numbers to Linux GPIO API pins. > Needs a gpio controller driver with the acpi handler hook set. > > Drivers can use acpi_get_gpio() to translate ACPI5 GpioIO and GpioInt > resources to Linux GPIO's. How does the mapping work? Is the ACPI gpio number space kept completely separate from the Linux GPIO numbers, or is there any attempt to line up ACPI and Linux GPIO numbering? From my reading, it /looks/ like the ACPI GPIO numbering is controller-local (no single large global space) because both a full path and a pin number are specified, but I'd like to know for sure. > Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/gpio/Kconfig | 4 +++ > drivers/gpio/Makefile | 1 + > drivers/gpio/gpiolib-acpi.c | 60 +++++++++++++++++++++++++++++++++++++++++++ > include/linux/acpi_gpio.h | 19 ++++++++++++++ > 4 files changed, 84 insertions(+) > create mode 100644 drivers/gpio/gpiolib-acpi.c > create mode 100644 include/linux/acpi_gpio.h > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > index d055cee..2f1905b 100644 > --- a/drivers/gpio/Kconfig > +++ b/drivers/gpio/Kconfig > @@ -49,6 +49,10 @@ config OF_GPIO > def_bool y > depends on OF && !SPARC > > +config ACPI_GPIO Nit: Can you flip this around to GPIO_ACPI? I know OF_GPIO is the other way around, but it should be changed. > diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c > new file mode 100644 > index 0000000..ef56ea4 > --- /dev/null > +++ b/drivers/gpio/gpiolib-acpi.c > @@ -0,0 +1,60 @@ > +/* > + * ACPI helpers for GPIO API > + * > + * Copyright (C) 2012, Intel Corporation > + * Author: Mathias Nyman <mathias.nyman@linux.intel.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#include <linux/errno.h> > +#include <linux/gpio.h> > +#include <linux/module.h> > +#include <linux/acpi_gpio.h> > +#include <linux/acpi.h> > + > +static int acpi_gpiochip_find(struct gpio_chip *gc, void *data) > +{ > + acpi_handle handle = data; > + acpi_handle gc_handle; > + > + if (!gc->dev) > + return false; > + > + gc_handle = gc->dev->acpi_handle; > + if (!gc_handle) > + return false; This test is redundant with the next one... unless 'handle' is also NULL :-) Is it at all possible for multiple gpiochips to be used for a single ACPI gpio controller node? Such as if the gpio controller has multiple banks that should be controlled separately? If so then this won't be sufficient. I've got the same issue with DT support where the find function needs to also check if the pin is provided by that specific gpiochip. Overall the patch looks good, but I need to see how it is used. It would be really nice if device drivers could use basically the same interface to obtain Linux gpio numbers regardless of if the backing data was ACPI or DT. This API is one level below that. > + > + return gc_handle == handle; > +} > + > +/** > + * acpi_get_gpio() - Translate ACPI GPIO pin to GPIO number usable with GPIO API > + * @path: ACPI GPIO controller full path name, (e.g. "\\_SB.GPO1") > + * @pin: ACPI GPIO pin number (0-based, controller-relative) > + * > + * Returns GPIO number to use with Linux generic GPIO API, or errno error value > + */ > + > +int acpi_get_gpio(char *path, int pin) > +{ > + struct gpio_chip *chip; > + acpi_handle handle; > + acpi_status status; > + > + status = acpi_get_handle(NULL, path, &handle); > + if (ACPI_FAILURE(status)) > + return -ENODEV; > + > + chip = gpiochip_find(handle, acpi_gpiochip_find); > + if (!chip) > + return -ENODEV; > + > + if (!gpio_is_valid(chip->base + pin)) > + return -EINVAL; > + > + return chip->base + pin; > +} > +EXPORT_SYMBOL(acpi_get_gpio); > diff --git a/include/linux/acpi_gpio.h b/include/linux/acpi_gpio.h > new file mode 100644 > index 0000000..e025664 > --- /dev/null > +++ b/include/linux/acpi_gpio.h > @@ -0,0 +1,19 @@ > +#ifndef _LINUX_ACPI_GPIO_H_ > +#define _LINUX_ACPI_GPIO_H_ > + > +#include <linux/errno.h> > + > +#ifdef CONFIG_ACPI_GPIO > + > +int acpi_get_gpio(char *path, int pin); > + > +#else /* CONFIG_ACPI_GPIO */ > + > +static inline int acpi_get_gpio(char *path, int pin) > +{ > + return -ENODEV; > +} > + > +#endif /* CONFIG_ACPI_GPIO */ > + > +#endif /* _LINUX_ACPI_GPIO_H_ */ > -- > 1.7.10.4 >
On Thu, Nov 08, 2012 at 03:55:18PM +0000, Grant Likely wrote: > Hi Mika, > > On Sat, Nov 3, 2012 at 7:46 AM, Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > From: Mathias Nyman <mathias.nyman@linux.intel.com> > > > > Add support for translating ACPI GPIO pin numbers to Linux GPIO API pins. > > Needs a gpio controller driver with the acpi handler hook set. > > > > Drivers can use acpi_get_gpio() to translate ACPI5 GpioIO and GpioInt > > resources to Linux GPIO's. > > How does the mapping work? Is the ACPI gpio number space kept > completely separate from the Linux GPIO numbers, or is there any > attempt to line up ACPI and Linux GPIO numbering? From my reading, it > /looks/ like the ACPI GPIO numbering is controller-local (no single > large global space) because both a full path and a pin number are > specified, but I'd like to know for sure. Yes, the ACPI GPIO number from GpioIO/GpioInt resources are controller relative and we use the path from the resource to find the actual controller. > > > Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com> > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > drivers/gpio/Kconfig | 4 +++ > > drivers/gpio/Makefile | 1 + > > drivers/gpio/gpiolib-acpi.c | 60 +++++++++++++++++++++++++++++++++++++++++++ > > include/linux/acpi_gpio.h | 19 ++++++++++++++ > > 4 files changed, 84 insertions(+) > > create mode 100644 drivers/gpio/gpiolib-acpi.c > > create mode 100644 include/linux/acpi_gpio.h > > > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > > index d055cee..2f1905b 100644 > > --- a/drivers/gpio/Kconfig > > +++ b/drivers/gpio/Kconfig > > @@ -49,6 +49,10 @@ config OF_GPIO > > def_bool y > > depends on OF && !SPARC > > > > +config ACPI_GPIO > > Nit: Can you flip this around to GPIO_ACPI? I know OF_GPIO is the > other way around, but it should be changed. Sure. > > > diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c > > new file mode 100644 > > index 0000000..ef56ea4 > > --- /dev/null > > +++ b/drivers/gpio/gpiolib-acpi.c > > @@ -0,0 +1,60 @@ > > +/* > > + * ACPI helpers for GPIO API > > + * > > + * Copyright (C) 2012, Intel Corporation > > + * Author: Mathias Nyman <mathias.nyman@linux.intel.com> > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 as > > + * published by the Free Software Foundation. > > + */ > > + > > +#include <linux/errno.h> > > +#include <linux/gpio.h> > > +#include <linux/module.h> > > +#include <linux/acpi_gpio.h> > > +#include <linux/acpi.h> > > + > > +static int acpi_gpiochip_find(struct gpio_chip *gc, void *data) > > +{ > > + acpi_handle handle = data; > > + acpi_handle gc_handle; > > + > > + if (!gc->dev) > > + return false; > > + > > + gc_handle = gc->dev->acpi_handle; > > + if (!gc_handle) > > + return false; > > This test is redundant with the next one... unless 'handle' is also NULL :-) > > Is it at all possible for multiple gpiochips to be used for a single > ACPI gpio controller node? Such as if the gpio controller has multiple > banks that should be controlled separately? If so then this won't be > sufficient. I've got the same issue with DT support where the find > function needs to also check if the pin is provided by that specific > gpiochip. AFAIK no but I'll let Mathias to answer that as he knows this better. > Overall the patch looks good, but I need to see how it is used. It > would be really nice if device drivers could use basically the same > interface to obtain Linux gpio numbers regardless of if the backing > data was ACPI or DT. This API is one level below that. Yeah, this patch just mimics the DT version but in general it would be better if there was only one API to get the GPIO. There has been discussion about adding gpio_get() or something similar which could perhaps be used to abstract away DT or ACPI. We use this in a driver so that we walk through the ACPI resources for a given device (if we have the ACPI handle) and parse the GpioIO/GpioInt resources like: struct acpi_resource_gpio *acpi_gpio; struct acpi_device *adev; acpi_resource *res; int gpio; /* obtain the ACPI device from handle */ ... /* walk through the resources attached to adev */ ... switch (res->type) { case ACPI_RESOURCE_TYPE_GPIO: acpi_gpio = &res->data.gpio; gpio = acpi_get_gpio(acpi_gpio->resource_source.string_ptr, acpi_gpio->pin_table[0]); ... } -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/08/2012 09:38 PM, Mika Westerberg wrote: ... >>> +#include<linux/errno.h> >>> +#include<linux/gpio.h> >>> +#include<linux/module.h> >>> +#include<linux/acpi_gpio.h> >>> +#include<linux/acpi.h> >>> + >>> +static int acpi_gpiochip_find(struct gpio_chip *gc, void *data) >>> +{ >>> + acpi_handle handle = data; >>> + acpi_handle gc_handle; >>> + >>> + if (!gc->dev) >>> + return false; >>> + >>> + gc_handle = gc->dev->acpi_handle; >>> + if (!gc_handle) >>> + return false; >> >> This test is redundant with the next one... unless 'handle' is also NULL :-) >> >> Is it at all possible for multiple gpiochips to be used for a single >> ACPI gpio controller node? Such as if the gpio controller has multiple >> banks that should be controlled separately? If so then this won't be >> sufficient. I've got the same issue with DT support where the find >> function needs to also check if the pin is provided by that specific >> gpiochip. > > AFAIK no but I'll let Mathias to answer that as he knows this better. I'm interpreting it the same way as Mika, max one actual controller per ACPI device node The path (called ResourceSource in ACPI5 specs) in GpioIO/GpioInt resources is a "string which uniquely identifies the GPIO controller referred to by this descriptor." The pin number is zero based controller relative. The ACPI device controller node includes all other resources needed by the controller driver (ioport/mem base, range, interrupt, and Hardware ID used to pair with a driver) Checked a board with two identical gpio controllers on it and it had two separate ACPI device node entries. (with only different io address base and interrupt resources) -Mathias > >> Overall the patch looks good, but I need to see how it is used. It >> would be really nice if device drivers could use basically the same >> interface to obtain Linux gpio numbers regardless of if the backing >> data was ACPI or DT. This API is one level below that. > > Yeah, this patch just mimics the DT version but in general it would be > better if there was only one API to get the GPIO. There has been discussion > about adding gpio_get() or something similar which could perhaps be used to > abstract away DT or ACPI. > > We use this in a driver so that we walk through the ACPI resources for a > given device (if we have the ACPI handle) and parse the GpioIO/GpioInt > resources like: > > struct acpi_resource_gpio *acpi_gpio; > struct acpi_device *adev; > acpi_resource *res; > int gpio; > > /* obtain the ACPI device from handle */ > ... > > /* walk through the resources attached to adev */ > ... > switch (res->type) { > case ACPI_RESOURCE_TYPE_GPIO: > acpi_gpio =&res->data.gpio; > > gpio = acpi_get_gpio(acpi_gpio->resource_source.string_ptr, > acpi_gpio->pin_table[0]); > ... > } -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 9, 2012 at 2:11 PM, Mathias Nyman <mathias.nyman@linux.intel.com> wrote: > On 11/08/2012 09:38 PM, Mika Westerberg wrote: > ... > >>>> +#include<linux/errno.h> >>>> +#include<linux/gpio.h> >>>> +#include<linux/module.h> >>>> +#include<linux/acpi_gpio.h> >>>> +#include<linux/acpi.h> >>>> + >>>> +static int acpi_gpiochip_find(struct gpio_chip *gc, void *data) >>>> +{ >>>> + acpi_handle handle = data; >>>> + acpi_handle gc_handle; >>>> + >>>> + if (!gc->dev) >>>> + return false; >>>> + >>>> + gc_handle = gc->dev->acpi_handle; >>>> + if (!gc_handle) >>>> + return false; >>> >>> >>> This test is redundant with the next one... unless 'handle' is also NULL >>> :-) >>> >>> Is it at all possible for multiple gpiochips to be used for a single >>> ACPI gpio controller node? Such as if the gpio controller has multiple >>> banks that should be controlled separately? If so then this won't be >>> sufficient. I've got the same issue with DT support where the find >>> function needs to also check if the pin is provided by that specific >>> gpiochip. >> >> >> AFAIK no but I'll let Mathias to answer that as he knows this better. > > > I'm interpreting it the same way as Mika, max one actual controller per ACPI > device node > > The path (called ResourceSource in ACPI5 specs) in GpioIO/GpioInt resources > is a "string which uniquely identifies the GPIO controller referred to by > this descriptor." The pin number is zero based controller relative. > > The ACPI device controller node includes all other resources needed by the > controller driver (ioport/mem base, range, interrupt, and Hardware ID used > to pair with a driver) > > Checked a board with two identical gpio controllers on it and it had two > separate ACPI device node entries. (with only different io address base and > interrupt resources) That's not really the situation that I'm thinking about. What I mean is for a gpio controller that is more convenient for Linux to support using multiple gpiochips (Linux internal detail), even though there it is described with a single ACPI node. g. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/09/2012 04:18 PM, Grant Likely wrote: > On Fri, Nov 9, 2012 at 2:11 PM, Mathias Nyman > <mathias.nyman@linux.intel.com> wrote: >> On 11/08/2012 09:38 PM, Mika Westerberg wrote: >> ... >> >>>>> +#include<linux/errno.h> >>>>> +#include<linux/gpio.h> >>>>> +#include<linux/module.h> >>>>> +#include<linux/acpi_gpio.h> >>>>> +#include<linux/acpi.h> >>>>> + >>>>> +static int acpi_gpiochip_find(struct gpio_chip *gc, void *data) >>>>> +{ >>>>> + acpi_handle handle = data; >>>>> + acpi_handle gc_handle; >>>>> + >>>>> + if (!gc->dev) >>>>> + return false; >>>>> + >>>>> + gc_handle = gc->dev->acpi_handle; >>>>> + if (!gc_handle) >>>>> + return false; >>>> >>>> >>>> This test is redundant with the next one... unless 'handle' is also NULL >>>> :-) >>>> >>>> Is it at all possible for multiple gpiochips to be used for a single >>>> ACPI gpio controller node? Such as if the gpio controller has multiple >>>> banks that should be controlled separately? If so then this won't be >>>> sufficient. I've got the same issue with DT support where the find >>>> function needs to also check if the pin is provided by that specific >>>> gpiochip. >>> >>> >>> AFAIK no but I'll let Mathias to answer that as he knows this better. >> >> >> I'm interpreting it the same way as Mika, max one actual controller per ACPI >> device node >> >> The path (called ResourceSource in ACPI5 specs) in GpioIO/GpioInt resources >> is a "string which uniquely identifies the GPIO controller referred to by >> this descriptor." The pin number is zero based controller relative. >> >> The ACPI device controller node includes all other resources needed by the >> controller driver (ioport/mem base, range, interrupt, and Hardware ID used >> to pair with a driver) >> >> Checked a board with two identical gpio controllers on it and it had two >> separate ACPI device node entries. (with only different io address base and >> interrupt resources) > > That's not really the situation that I'm thinking about. What I mean > is for a gpio controller that is more convenient for Linux to support > using multiple gpiochips (Linux internal detail), even though there it > is described with a single ACPI node. > Ok, now I get it. Yes, in case a driver uses several gpiochips internally for different banks of a controller then all would have the same acpi_handle. acpi_get_gpio() would use the gpiobase of the first gpiochip that matches the handle, even if it's the wrong one. I guess It's possible to write a driver like that. The only acpi enumerated driver with the acpi_handle set (soon coming to upstream) is not done like that. Do you think this is a case that should be solved now? or just expect acpi gpio device driver to not use several gpiochips in one driver? -Mathias -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 9, 2012 at 3:05 PM, Mathias Nyman <mathias.nyman@linux.intel.com> wrote: > On 11/09/2012 04:18 PM, Grant Likely wrote: >> >> On Fri, Nov 9, 2012 at 2:11 PM, Mathias Nyman >> <mathias.nyman@linux.intel.com> wrote: >>> >>> On 11/08/2012 09:38 PM, Mika Westerberg wrote: >>> ... >>> >>>>>> +#include<linux/errno.h> >>>>>> +#include<linux/gpio.h> >>>>>> +#include<linux/module.h> >>>>>> +#include<linux/acpi_gpio.h> >>>>>> +#include<linux/acpi.h> >>>>>> + >>>>>> +static int acpi_gpiochip_find(struct gpio_chip *gc, void *data) >>>>>> +{ >>>>>> + acpi_handle handle = data; >>>>>> + acpi_handle gc_handle; >>>>>> + >>>>>> + if (!gc->dev) >>>>>> + return false; >>>>>> + >>>>>> + gc_handle = gc->dev->acpi_handle; >>>>>> + if (!gc_handle) >>>>>> + return false; >>>>> >>>>> >>>>> >>>>> This test is redundant with the next one... unless 'handle' is also >>>>> NULL >>>>> :-) >>>>> >>>>> Is it at all possible for multiple gpiochips to be used for a single >>>>> ACPI gpio controller node? Such as if the gpio controller has multiple >>>>> banks that should be controlled separately? If so then this won't be >>>>> sufficient. I've got the same issue with DT support where the find >>>>> function needs to also check if the pin is provided by that specific >>>>> gpiochip. >>>> >>>> >>>> >>>> AFAIK no but I'll let Mathias to answer that as he knows this better. >>> >>> >>> >>> I'm interpreting it the same way as Mika, max one actual controller per >>> ACPI >>> device node >>> >>> The path (called ResourceSource in ACPI5 specs) in GpioIO/GpioInt >>> resources >>> is a "string which uniquely identifies the GPIO controller referred to by >>> this descriptor." The pin number is zero based controller relative. >>> >>> The ACPI device controller node includes all other resources needed by >>> the >>> controller driver (ioport/mem base, range, interrupt, and Hardware ID >>> used >>> to pair with a driver) >>> >>> Checked a board with two identical gpio controllers on it and it had two >>> separate ACPI device node entries. (with only different io address base >>> and >>> interrupt resources) >> >> >> That's not really the situation that I'm thinking about. What I mean >> is for a gpio controller that is more convenient for Linux to support >> using multiple gpiochips (Linux internal detail), even though there it >> is described with a single ACPI node. >> > > Ok, now I get it. > > Yes, in case a driver uses several gpiochips internally for different banks > of a controller then all would have the same acpi_handle. > acpi_get_gpio() would use the gpiobase of the first gpiochip that matches > the handle, even if it's the wrong one. > > I guess It's possible to write a driver like that. > The only acpi enumerated driver with the acpi_handle set (soon coming to > upstream) is not done like that. > > Do you think this is a case that should be solved now? or just expect acpi > gpio device driver to not use several gpiochips in one driver? Look at what the DT code does. It is actually pretty easy to solve. I would do it now, but I won't block the changes if you do not. g. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 09, 2012 at 03:46:58PM +0000, Grant Likely wrote: > > I guess It's possible to write a driver like that. > > The only acpi enumerated driver with the acpi_handle set (soon coming to > > upstream) is not done like that. > > > > Do you think this is a case that should be solved now? or just expect acpi > > gpio device driver to not use several gpiochips in one driver? > > Look at what the DT code does. It is actually pretty easy to solve. I > would do it now, but I won't block the changes if you do not. I think for now we just implement the single gpiochip solution and once (if ever) there emerges need for multiple gpiochips in a single driver, we add the support then. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index d055cee..2f1905b 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -49,6 +49,10 @@ config OF_GPIO def_bool y depends on OF && !SPARC +config ACPI_GPIO + def_bool y + depends on ACPI + config DEBUG_GPIO bool "Debug GPIO calls" depends on DEBUG_KERNEL diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index 9aeed67..5254b6d 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -4,6 +4,7 @@ ccflags-$(CONFIG_DEBUG_GPIO) += -DDEBUG obj-$(CONFIG_GPIOLIB) += gpiolib.o devres.o obj-$(CONFIG_OF_GPIO) += gpiolib-of.o +obj-$(CONFIG_ACPI_GPIO) += gpiolib-acpi.o # Device drivers. Generally keep list sorted alphabetically obj-$(CONFIG_GPIO_GENERIC) += gpio-generic.o diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c new file mode 100644 index 0000000..ef56ea4 --- /dev/null +++ b/drivers/gpio/gpiolib-acpi.c @@ -0,0 +1,60 @@ +/* + * ACPI helpers for GPIO API + * + * Copyright (C) 2012, Intel Corporation + * Author: Mathias Nyman <mathias.nyman@linux.intel.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include <linux/errno.h> +#include <linux/gpio.h> +#include <linux/module.h> +#include <linux/acpi_gpio.h> +#include <linux/acpi.h> + +static int acpi_gpiochip_find(struct gpio_chip *gc, void *data) +{ + acpi_handle handle = data; + acpi_handle gc_handle; + + if (!gc->dev) + return false; + + gc_handle = gc->dev->acpi_handle; + if (!gc_handle) + return false; + + return gc_handle == handle; +} + +/** + * acpi_get_gpio() - Translate ACPI GPIO pin to GPIO number usable with GPIO API + * @path: ACPI GPIO controller full path name, (e.g. "\\_SB.GPO1") + * @pin: ACPI GPIO pin number (0-based, controller-relative) + * + * Returns GPIO number to use with Linux generic GPIO API, or errno error value + */ + +int acpi_get_gpio(char *path, int pin) +{ + struct gpio_chip *chip; + acpi_handle handle; + acpi_status status; + + status = acpi_get_handle(NULL, path, &handle); + if (ACPI_FAILURE(status)) + return -ENODEV; + + chip = gpiochip_find(handle, acpi_gpiochip_find); + if (!chip) + return -ENODEV; + + if (!gpio_is_valid(chip->base + pin)) + return -EINVAL; + + return chip->base + pin; +} +EXPORT_SYMBOL(acpi_get_gpio); diff --git a/include/linux/acpi_gpio.h b/include/linux/acpi_gpio.h new file mode 100644 index 0000000..e025664 --- /dev/null +++ b/include/linux/acpi_gpio.h @@ -0,0 +1,19 @@ +#ifndef _LINUX_ACPI_GPIO_H_ +#define _LINUX_ACPI_GPIO_H_ + +#include <linux/errno.h> + +#ifdef CONFIG_ACPI_GPIO + +int acpi_get_gpio(char *path, int pin); + +#else /* CONFIG_ACPI_GPIO */ + +static inline int acpi_get_gpio(char *path, int pin) +{ + return -ENODEV; +} + +#endif /* CONFIG_ACPI_GPIO */ + +#endif /* _LINUX_ACPI_GPIO_H_ */