Re: [RFC PATCH 0/9] Add ACPI _DSD and unified device properties support

Re: [RFC PATCH 0/9] Add ACPI _DSD and unified device properties support

[Darren Hart]

On 8/15/14, 23:53, "Mika Westerberg" <mika.westerberg@linux.intel.com>
wrote:

>The recent publication of the ACPI 5.1 specification [1] adds a reserved
>name
>for Device Specific Data (_DSD, Section 6.2.5). This mechanism allows for
>passing arbitrary hardware description data to the OS. The exact format
>of the
>_DSD data is specific to the UUID paired with it [2].
>
>An ACPI Device Properties UUID has been defined [3] to provide a format
>compatible with existing device tree schemas. The purpose for this was to
>allow
>for the reuse of the existing schemas and encourage the development of
>firmware
>agnostic device drivers.
>
>This series accomplishes the following (as well as some other
>dependencies):
>
> * Add _DSD support to the ACPI core
>   This simply reads the UUID and the accompanying Package
>
> * Add ACPI Device Properties _DSD format support
>   This understands the hierarchical key:value pair structure
>   defined by the Device Properties UUID
>
> * Add a unified device properties API with ACPI and OF backends
>   This provides for the firmware agnostic device properties
>   Interface to be used by drivers
>
> * Provides 2 example drivers that were previously Device Tree aware that
>   can now be used with either Device Tree or ACPI Device Properties. The
>   both drivers use an arbitrary _HID.
>
>This has been tested on Minnowboard with relevant parts of the modified
>DSDT at the end of this email.


This eliminates the need for the board files that were the subject of my
"How not to write x86 platform drivers" talk at ELC-E last year. With
These ACPI core changes and the small changes to the two example drivers,
the Minnowboard can now use the GPIO buttons and LEDs through these
drivers by adding the ASL fragment below to the DSDT.

--
Darren

>
>------ DSDT For Minnowboard ------
>
>    Scope (\_SB.PCI0.LPC)
>    {
>        Device (LEDS)
>        {
>            Name (_HID, "MNW0001")
>
>            Name (_CRS, ResourceTemplate ()
>            {
>                GpioIo (Exclusive, PullDown, 0, 0,
>IoRestrictionOutputOnly,
>                        "\\_SB.PCI0.LPC", 0, ResourceConsumer) {10}
>                GpioIo (Exclusive, PullDown, 0, 0,
>IoRestrictionOutputOnly,
>                        "\\_SB.PCI0.LPC", 0, ResourceConsumer) {11}
>            })
>
>            Device (LEDH)
>            {
>                Name (_HID, "PRP0000")
>                Name (_DSD, Package ()
>                {
>                    ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>                    Package ()
>                    {
>                        Package () {"label", "Heartbeat"},
>                        Package () {"gpios", Package () {^^LEDS, 0, 0,
>0}},
>                        Package () {"linux,default-trigger", "heartbeat"},
>                        Package () {"linux,default-state", "off"},
>                        Package () {"linux,retain-state-suspended", 1},
>                    }
>                })
>            }
>
>            Device (LEDM)
>            {
>                Name (_HID, "PRP0000")
>                Name (_DSD, Package ()
>                {
>                    ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>                    Package ()
>                    {
>                        Package () {"label", "MMC0 Activity"},
>                        Package () {"gpios", Package () {^^LEDS, 1, 0,
>0}},
>                        Package () {"linux,default-trigger", "mmc0"},
>                        Package () {"linux,default-state", "on"},
>                        Package () {"linux,retain-state-suspended", 1},
>                    }
>                })
>            }
>        }
>        
>        Device (BTNS)
>        {
>            Name (_HID, "MNW0002")
>
>            Name (_CRS, ResourceTemplate ()
>            {
>                GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly,
>                        "\\_SB.PCI0.LPC", 0, ResourceConsumer) {0}
>                GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly,
>                        "\\_SB.PCI0.LPC", 0, ResourceConsumer) {1}
>                GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly,
>                        "\\_SB.PCI0.LPC", 0, ResourceConsumer) {2}
>                GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly,
>                        "\\_SB.PCI0.LPC", 0, ResourceConsumer) {3}
>            })
>
>            Name (_DSD, Package ()
>            {
>                ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>                Package ()
>                {
>                    Package () {"poll-interval", 100},
>                    Package () {"autorepeat", 1}
>                }
>            })
>
>            Device (BTN0)
>            {
>                Name (_HID, "PRP0000")
>                Name (_DSD, Package ()
>                {
>                    ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>                    Package ()
>                    {
>                        Package () {"linux,code", 105},
>                        Package () {"linux,input-type", 1},
>                        Package () {"gpios", Package () {^^BTNS, 0, 0,
>1}},
>                    }
>                })
>            }
>
>            Device (BTN1)
>            {
>                Name (_HID, "PRP0000")
>                Name (_DSD, Package ()
>                {
>                    ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>                    Package ()
>                    {
>                        Package () {"linux,code", 108},
>                        Package () {"linux,input-type", 1},
>                        Package () {"gpios", Package (4) {^^BTNS, 1, 0,
>1}},
>                    }
>                })
>            }
>            
>            Device (BTN2)
>            {
>                Name (_HID, "PRP0000")
>                Name (_DSD, Package ()
>                {
>                    ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>                    Package ()
>                    {
>                        Package () {"linux,code", 103},
>                        Package () {"linux,input-type", 1},
>                        Package () {"gpios", Package () {^^BTNS, 2, 0,
>1}},
>                    }
>                })
>            }
>            
>            Device (BTN3)
>            {
>                Name (_HID, "PRP0000")
>                Name (_DSD, Package ()
>                {
>                    ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>                    Package ()
>                    {
>                        Package () {"linux,code", 106},
>                        Package () {"linux,input-type", 1},
>                        Package () {"gpios", Package (4) {^^BTNS, 3, 0,
>1}},
>                    }
>                })
>            }
>
>        }
>
>    }
>
>-- 
>2.1.0.rc1
>
>


-- 
Darren Hart					Open Source Technology Center
darren.hart@intel.com				            Intel Corporation

Re: [RFC PATCH 0/9] Add ACPI _DSD and unified device properties support

[Dmitry Torokhov]

On Sat, Aug 16, 2014 at 09:06:05AM -0700, Darren Hart wrote:
> On 8/15/14, 23:53, "Mika Westerberg" <mika.westerberg@linux.intel.com>
> wrote:
> 
> >The recent publication of the ACPI 5.1 specification [1] adds a reserved
> >name
> >for Device Specific Data (_DSD, Section 6.2.5). This mechanism allows for
> >passing arbitrary hardware description data to the OS. The exact format
> >of the
> >_DSD data is specific to the UUID paired with it [2].
> >
> >An ACPI Device Properties UUID has been defined [3] to provide a format
> >compatible with existing device tree schemas. The purpose for this was to
> >allow
> >for the reuse of the existing schemas and encourage the development of
> >firmware
> >agnostic device drivers.
> >
> >This series accomplishes the following (as well as some other
> >dependencies):
> >
> > * Add _DSD support to the ACPI core
> >   This simply reads the UUID and the accompanying Package
> >
> > * Add ACPI Device Properties _DSD format support
> >   This understands the hierarchical key:value pair structure
> >   defined by the Device Properties UUID
> >
> > * Add a unified device properties API with ACPI and OF backends
> >   This provides for the firmware agnostic device properties
> >   Interface to be used by drivers
> >
> > * Provides 2 example drivers that were previously Device Tree aware that
> >   can now be used with either Device Tree or ACPI Device Properties. The
> >   both drivers use an arbitrary _HID.
> >
> >This has been tested on Minnowboard with relevant parts of the modified
> >DSDT at the end of this email.
> 
> 
> This eliminates the need for the board files that were the subject of my
> "How not to write x86 platform drivers" talk at ELC-E last year. With
> These ACPI core changes and the small changes to the two example drivers,
> the Minnowboard can now use the GPIO buttons and LEDs through these
> drivers by adding the ASL fragment below to the DSDT.

>From the drivers perspective I am less than impressed with the need to
reshuffle all the drivers to support ACPI with the new API. I thought the plan
was to try and keep OF API and try to translate as much as possible to it?

The same goes for bringing arbitrary HIDs into the drivers. Can we have HID->OF
naming hidden in ACPI (define a new property like "dt-name",  "of_compat" or
whatever) and have ACPI core map one to another.

Thanks.

-- 
Dmitry

Re: [RFC PATCH 0/9] Add ACPI _DSD and unified device properties support

[Josh Triplett]

On Sat, Aug 16, 2014 at 09:53:50AM +0300, Mika Westerberg wrote:
> The recent publication of the ACPI 5.1 specification [1] adds a reserved name
> for Device Specific Data (_DSD, Section 6.2.5). This mechanism allows for
> passing arbitrary hardware description data to the OS. The exact format of the
> _DSD data is specific to the UUID paired with it [2].   
> 
> An ACPI Device Properties UUID has been defined [3] to provide a format
> compatible with existing device tree schemas. The purpose for this was to allow
> for the reuse of the existing schemas and encourage the development of firmware
> agnostic device drivers.
> 
> This series accomplishes the following (as well as some other dependencies):
> 
>  * Add _DSD support to the ACPI core
>    This simply reads the UUID and the accompanying Package
> 
>  * Add ACPI Device Properties _DSD format support
>    This understands the hierarchical key:value pair structure
>    defined by the Device Properties UUID
> 
>  * Add a unified device properties API with ACPI and OF backends
>    This provides for the firmware agnostic device properties
>    Interface to be used by drivers
> 
>  * Provides 2 example drivers that were previously Device Tree aware that
>    can now be used with either Device Tree or ACPI Device Properties. The
>    both drivers use an arbitrary _HID.
> 
> This has been tested on Minnowboard with relevant parts of the modified
> DSDT at the end of this email.
> 
> This series does not provide for a means to append to a system DSDT. That
> will ultimately be required to make the most effective use of the _DSD
> mechanism. Work is underway on that as a separate effort.
> 
> [1] http://www.uefi.org/sites/default/files/resources/ACPI_5_1release.pdf
> [2] http://www.uefi.org/sites/default/files/resources/_DSD-implementation-guide-toplevel.htm
> [3] http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf
> 
> Aaron Lu (3):
>   of: Add property_ops callback for devices with of_node
>   gpiolib: add API to get gpio desc and flags
>   Input: gpio_keys_polled - Make use of device property API
> 
> Max Eliaser (1):
>   leds: leds-gpio: Make use of device property API
> 
> Mika Westerberg (4):
>   ACPI: Add support for device specific properties
>   ACPI: Document ACPI device specific properties
>   mfd: Add ACPI support
>   gpio: sch: Consolidate core and resume banks
> 
> Rafael J. Wysocki (1):
>   Driver core: Unified device properties interface for platform firmware

One issue I noticed with the series: the new read functions, when given
an integer type, don't appear to do any range-checking to make sure the
returned value falls within the range of that type.  They should do so,
and return -EOVERFLOW if so.  And the documentation of the new functions
should note that explicitly as an error case.

With that fixed:
Reviewed-by: Josh Triplett <josh@joshtriplett.org>

Re: [RFC PATCH 0/9] Add ACPI _DSD and unified device properties support

[Mika Westerberg]

On Sat, Aug 16, 2014 at 09:53:50AM +0300, Mika Westerberg wrote:
> The recent publication of the ACPI 5.1 specification [1] adds a reserved name
> for Device Specific Data (_DSD, Section 6.2.5). This mechanism allows for
> passing arbitrary hardware description data to the OS. The exact format of the
> _DSD data is specific to the UUID paired with it [2].   
> 
> An ACPI Device Properties UUID has been defined [3] to provide a format
> compatible with existing device tree schemas. The purpose for this was to allow
> for the reuse of the existing schemas and encourage the development of firmware
> agnostic device drivers.
> 
> This series accomplishes the following (as well as some other dependencies):
> 
>  * Add _DSD support to the ACPI core
>    This simply reads the UUID and the accompanying Package
> 
>  * Add ACPI Device Properties _DSD format support
>    This understands the hierarchical key:value pair structure
>    defined by the Device Properties UUID
> 
>  * Add a unified device properties API with ACPI and OF backends
>    This provides for the firmware agnostic device properties
>    Interface to be used by drivers
> 
>  * Provides 2 example drivers that were previously Device Tree aware that
>    can now be used with either Device Tree or ACPI Device Properties. The
>    both drivers use an arbitrary _HID.
> 
> This has been tested on Minnowboard with relevant parts of the modified
> DSDT at the end of this email.
> 
> This series does not provide for a means to append to a system DSDT. That
> will ultimately be required to make the most effective use of the _DSD
> mechanism. Work is underway on that as a separate effort.
> 
> [1] http://www.uefi.org/sites/default/files/resources/ACPI_5_1release.pdf
> [2] http://www.uefi.org/sites/default/files/resources/_DSD-implementation-guide-toplevel.htm
> [3] http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf

For some reason this series never reached the mailing lists intented, so
I re-sent the series with shorter CC list to LKML, linux-acpi and
devicetree lists. 

LKML thread is here:

https://lkml.org/lkml/2014/8/17/10

Sorry for the inconvenience.

Re: [RFC PATCH 8/9] Input: gpio_keys_polled - Make use of device property API

[Jacob Pan]

On Sat, 16 Aug 2014 09:53:58 +0300
Mika Westerberg <mika.westerberg@linux.intel.com> wrote:

> From: Aaron Lu <aaron.lu@intel.com>
> 
> Make use of device property API in this driver so that both OF based
> system and ACPI based system can use this driver.
> 
Do we always assume OF and ACPI _DSD will have the same property name
strings? i.e. in this patch "gpios"
> +	if (device_property_get(dev, "gpios", NULL)) {
> -		if (!of_find_property(pp, "gpios", NULL)) {

Maybe i missed something, but I don't think we can make that assumption
in BIOS. If not, what is the point of having unified interface?

> The driver isn't converted to descriptor based gpio API due to there
> are a lot of existing users of struct gpio_keys_button that expects
> the gpio integer field. Though this can be solved by adding a new
> field of type struct gpio_desc but then there is another problem: the
> devm_gpiod_get needs to operate on the button device instead of its
> parent device that has the driver binded, so when the driver is
> unloaded, the resources for the gpio will not get freed
> automatically. Alternatively, we can introduce a new API, something
> like gpiod_find_index that does almost the same thing as
> gpiod_get_index but doesn't do the gpiod_request call and use it
> during finding button resources in gpio_keys_polled_get_button
> function. But then this may seem too complicated and not desirable.
> So in the end, a simple dev_get_gpiod_flags which is introduced in
> the last patch gets used and the gpio field of the struct
> gpio_keys_button can be got from the desc_to_gpio.
> 
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> Signed-off-by: Max Eliaser <max@meliaserlow.dyndns.tv>
> Reviewed-by: Darren Hart <dvhart@linux.intel.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/input/keyboard/gpio_keys_polled.c | 139
> ++++++++++++++++++------------ 1 file changed, 84 insertions(+), 55
> deletions(-)
> 
> diff --git a/drivers/input/keyboard/gpio_keys_polled.c
> b/drivers/input/keyboard/gpio_keys_polled.c index
> 432d36395f35..49895d75aaff 100644 ---
> a/drivers/input/keyboard/gpio_keys_polled.c +++
> b/drivers/input/keyboard/gpio_keys_polled.c @@ -27,6 +27,7 @@
>  #include <linux/of.h>
>  #include <linux/of_platform.h>
>  #include <linux/of_gpio.h>
> +#include <linux/property.h>
>  
>  #define DRV_NAME	"gpio-keys-polled"
>  
> @@ -102,78 +103,95 @@ static void gpio_keys_polled_close(struct
> input_polled_dev *dev) pdata->disable(bdev->dev);
>  }
>  
> -#ifdef CONFIG_OF
> -static struct gpio_keys_platform_data
> *gpio_keys_polled_get_devtree_pdata(struct device *dev) -{
> -	struct device_node *node, *pp;
> +#if defined(CONFIG_OF) || defined(CONFIG_ACPI)
> +
> +struct button_proc_context {
>  	struct gpio_keys_platform_data *pdata;
> -	struct gpio_keys_button *button;
> -	int error;
> -	int nbuttons;
>  	int i;
> +};
>  
> -	node = dev->of_node;
> -	if (!node)
> -		return NULL;
> +static int gpio_keys_polled_get_button(struct device *dev, void
> *data) +{
> +	struct button_proc_context *c = data;
> +	struct gpio_keys_platform_data *pdata = c->pdata;
> +	struct gpio_keys_button *button = &pdata->buttons[c->i];
> +	struct gpio_desc *desc;
> +	enum gpio_lookup_flags flags;
> +	void *val;
> +
> +	if (device_property_get(dev, "gpios", NULL)) {
> +		pdata->nbuttons--;
> +		dev_warn(dev, "Found button without gpios\n");
> +		return 0;
> +	}
>  
> -	nbuttons = of_get_child_count(node);
> -	if (nbuttons == 0)
> -		return NULL;
> +	desc = dev_get_gpiod_flags(dev, 0, &flags);
> +	if (IS_ERR(desc)) {
> +		int error = PTR_ERR(desc);
>  
> -	pdata = devm_kzalloc(dev, sizeof(*pdata) + nbuttons *
> sizeof(*button),
> -			     GFP_KERNEL);
> -	if (!pdata)
> -		return ERR_PTR(-ENOMEM);
> +		if (error != -EPROBE_DEFER)
> +			dev_err(dev, "Failed to get gpio flags,
> error: %d\n",
> +				error);
> +		return error;
> +	}
>  
> -	pdata->buttons = (struct gpio_keys_button *)(pdata + 1);
> -	pdata->nbuttons = nbuttons;
> +	button->gpio = desc_to_gpio(desc);
> +	button->active_low = flags & GPIO_ACTIVE_LOW;
>  
> -	pdata->rep = !!of_get_property(node, "autorepeat", NULL);
> -	of_property_read_u32(node, "poll-interval",
> &pdata->poll_interval);
> +	if (device_property_read(dev, "linux,code", DEV_PROP_U32,
> +				 &button->code)) {
> +		dev_err(dev, "Button without keycode: 0x%x\n",
> +				button->gpio);
> +		return -EINVAL;
> +	}
>  
> -	i = 0;
> -	for_each_child_of_node(node, pp) {
> -		int gpio;
> -		enum of_gpio_flags flags;
> +	if (!device_property_get(dev, "label", &val))
> +		button->desc = val;
>  
> -		if (!of_find_property(pp, "gpios", NULL)) {
> -			pdata->nbuttons--;
> -			dev_warn(dev, "Found button without
> gpios\n");
> -			continue;
> -		}
> +	if (device_property_read(dev, "linux,input-type",
> DEV_PROP_U32,
> +				 &button->type))
> +		button->type = EV_KEY;
>  
> -		gpio = of_get_gpio_flags(pp, 0, &flags);
> -		if (gpio < 0) {
> -			error = gpio;
> -			if (error != -EPROBE_DEFER)
> -				dev_err(dev,
> -					"Failed to get gpio flags,
> error: %d\n",
> -					error);
> -			return ERR_PTR(error);
> -		}
> +	button->wakeup = !device_property_get(dev,
> "gpio-key,wakeup", NULL); 
> -		button = &pdata->buttons[i++];
> +	if (device_property_read(dev, "debounce-interval",
> DEV_PROP_U32,
> +				 &button->debounce_interval))
> +		button->debounce_interval = 5;
>  
> -		button->gpio = gpio;
> -		button->active_low = flags & OF_GPIO_ACTIVE_LOW;
> +	c->i++;
> +	return 0;
> +}
>  
> -		if (of_property_read_u32(pp, "linux,code",
> &button->code)) {
> -			dev_err(dev, "Button without keycode:
> 0x%x\n",
> -				button->gpio);
> -			return ERR_PTR(-EINVAL);
> -		}
> +static struct gpio_keys_platform_data *
> +gpio_keys_polled_get_devtree_pdata(struct device *dev)
> +{
> +	struct gpio_keys_platform_data *pdata;
> +	int size;
> +	struct button_proc_context c;
> +	int error;
> +	int nbuttons;
>  
> -		button->desc = of_get_property(pp, "label", NULL);
> +	nbuttons = device_property_child_count(dev);
> +	if (nbuttons <= 0)
> +		return NULL;
>  
> -		if (of_property_read_u32(pp, "linux,input-type",
> &button->type))
> -			button->type = EV_KEY;
> +	size = sizeof(*pdata) + nbuttons * sizeof(struct
> gpio_keys_button);
> +	pdata = devm_kzalloc(dev, size, GFP_KERNEL);
> +	if (!pdata)
> +		return ERR_PTR(-ENOMEM);
>  
> -		button->wakeup = !!of_get_property(pp,
> "gpio-key,wakeup", NULL);
> +	pdata->buttons = (struct gpio_keys_button *)(pdata + 1);
> +	pdata->nbuttons = nbuttons;
>  
> -		if (of_property_read_u32(pp, "debounce-interval",
> -					 &button->debounce_interval))
> -			button->debounce_interval = 5;
> -	}
> +	pdata->rep = !device_property_get(dev, "autorepeat", NULL);
> +	device_property_read(dev, "poll-interval", DEV_PROP_U32,
> +			     &pdata->poll_interval);
> +
> +	c.pdata = pdata;
> +	c.i = 0;
> +	error = device_for_each_child(dev, &c,
> gpio_keys_polled_get_button);
> +	if (error)
> +		return ERR_PTR(error);
>  
>  	if (pdata->nbuttons == 0)
>  		return ERR_PTR(-EINVAL);
> @@ -181,11 +199,21 @@ static struct gpio_keys_platform_data
> *gpio_keys_polled_get_devtree_pdata(struct return pdata;
>  }
>  
> +#ifdef CONFIG_OF
>  static const struct of_device_id gpio_keys_polled_of_match[] = {
>  	{ .compatible = "gpio-keys-polled", },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, gpio_keys_polled_of_match);
> +#endif
> +
> +#ifdef CONFIG_ACPI
> +static struct acpi_device_id gpio_keys_polled_acpi_match[] = {
> +	{ "MNW0002" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(acpi, gpio_keys_polled_acpi_match);
> +#endif
>  
>  #else
>  
> @@ -309,6 +337,7 @@ static struct platform_driver
> gpio_keys_polled_driver = { .name	= DRV_NAME,
>  		.owner	= THIS_MODULE,
>  		.of_match_table =
> of_match_ptr(gpio_keys_polled_of_match),
> +		.acpi_match_table =
> ACPI_PTR(gpio_keys_polled_acpi_match), },
>  };
>  module_platform_driver(gpio_keys_polled_driver);

[Jacob Pan]

Re: [RFC PATCH 8/9] Input: gpio_keys_polled - Make use of device property API

[Mika Westerberg]

On Mon, Aug 18, 2014 at 10:55:12AM -0700, Jacob Pan wrote:
> On Sat, 16 Aug 2014 09:53:58 +0300
> Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
> 
> > From: Aaron Lu <aaron.lu@intel.com>
> > 
> > Make use of device property API in this driver so that both OF based
> > system and ACPI based system can use this driver.
> > 
> Do we always assume OF and ACPI _DSD will have the same property name
> strings? i.e. in this patch "gpios"
> > +	if (device_property_get(dev, "gpios", NULL)) {
> > -		if (!of_find_property(pp, "gpios", NULL)) {
> 
> Maybe i missed something, but I don't think we can make that assumption
> in BIOS. If not, what is the point of having unified interface?

We recommend that when it makes sense, the property names in _DSD follow
the corresponding DT names.

Re: [RFC PATCH 8/9] Input: gpio_keys_polled - Make use of device property API

[Darren Hart]

On 8/19/14, 4:27, "Mika Westerberg" <mika.westerberg@linux.intel.com>
wrote:

>On Mon, Aug 18, 2014 at 10:55:12AM -0700, Jacob Pan wrote:
>> On Sat, 16 Aug 2014 09:53:58 +0300
>> Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
>> 
>> > From: Aaron Lu <aaron.lu@intel.com>
>> > 
>> > Make use of device property API in this driver so that both OF based
>> > system and ACPI based system can use this driver.
>> > 
>> Do we always assume OF and ACPI _DSD will have the same property name
>> strings? i.e. in this patch "gpios"
>> > +	if (device_property_get(dev, "gpios", NULL)) {
>> > -		if (!of_find_property(pp, "gpios", NULL)) {
>> 
>> Maybe i missed something, but I don't think we can make that assumption
>> in BIOS. If not, what is the point of having unified interface?
>
>We recommend that when it makes sense, the property names in _DSD follow
>the corresponding DT names.
>

This is especially try for platform drivers such as this.

We will be creating subsystem types, "gpios", and defining them in the
_DSD Implementors Guide (per our discussion this morning at Kernel Summit).
-- 
Darren Hart					Open Source Technology Center
darren.hart@intel.com				            Intel Corporation

Re: [RFC PATCH 6/9] gpiolib: add API to get gpio desc and flags

[Mika Westerberg]

On Mon, Aug 18, 2014 at 09:24:48AM -0700, Alexandre Courbot wrote:
> On Fri, Aug 15, 2014 at 11:53 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index 2ebc9071e354..e6c2413a6fbf 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -2644,6 +2644,24 @@ static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
> >         return desc;
> >  }
> >
> > +struct gpio_desc *dev_get_gpiod_flags(struct device *dev, unsigned int idx,
> > +                                     enum gpio_lookup_flags *flags)
> > +{
> > +       struct gpio_desc *desc = ERR_PTR(-ENOENT);
> > +
> > +       if (!dev || !flags)
> > +               return ERR_PTR(-EINVAL);
> > +
> > +       /* Using device tree? */
> > +       if (IS_ENABLED(CONFIG_OF) && dev->of_node)
> > +               desc = of_find_gpio(dev, NULL, idx, flags);
> > +       else if (IS_ENABLED(CONFIG_ACPI) && ACPI_COMPANION(dev))
> > +               desc = acpi_get_gpiod_flags(dev, idx, flags);
> > +
> > +       return desc;
> > +}
> > +EXPORT_SYMBOL(dev_get_gpiod_flags);
> 
> Putting aside the fact that this function is clearly ACPI-centric (no
> con_id parameter and no handling of the platform interface), I have
> two big problems with it and it ending up in the consumer interface:
> 
> 1) The returned descriptor is not requested by gpiolib, which means no
> check is made about whether the GPIO has already been requested by
> someone else, and another driver can very well request the same GPIO
> later and obtain it. Any descriptor returned by a function in
> consumer.h *must* be properly requested. Furthermore the 1:1 mapping
> between GPIO descriptors and GPIO numbers is not something we can take
> for granted (since it will likely change soon), so this practice is
> definitely to ban.

My bad, somehow I missed the part that it never requested the GPIO.
Thanks for pointing it out.

> 2) It exposes the GPIO flags, while they are supposed to be opaque to consumers.

And this, of course we should be using gpiod_is_active_low() and similar
functions that work with descriptors.

> These two points would somehow be acceptable if this function was
> gpiolib-private, but here it is clearly not the case and this allows
> pretty nasty thing to happen. Basically you are using it to take
> advantage of the gpiod lookup mechanism and then quickly fall back to
> the legacy integer interface. That's really not something to encourage
> - these drivers should be converted to use gpiod internally (while
> preserving integer-based lookup for compatiblity, if needed).
> 
> In patch 8 you say:
> 
> "this can be solved by adding a new field of type
> struct gpio_desc but then there is another problem: the devm_gpiod_get
> needs to operate on the button device instead of its parent device that
> has the driver binded, so when the driver is unloaded, the resources for
> the gpio will not get freed automatically."
> 
> I'd very much prefer that you use the non-devm variant of gpiod_get()
> and free the resources manually when the driver is unloaded than this
> workaround that introduces an loophole in the gpiod consumer lookup
> functions.

I agree and we are going to rework this and the consumer patches to do
exactly what you say.

Re: [RFC PATCH 6/9] gpiolib: add API to get gpio desc and flags

[Aaron Lu]

On 08/19/2014 04:56 PM, Mika Westerberg wrote:
> On Mon, Aug 18, 2014 at 09:24:48AM -0700, Alexandre Courbot wrote:
>> On Fri, Aug 15, 2014 at 11:53 PM, Mika Westerberg
>> <mika.westerberg@linux.intel.com> wrote:
>>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>>> index 2ebc9071e354..e6c2413a6fbf 100644
>>> --- a/drivers/gpio/gpiolib.c
>>> +++ b/drivers/gpio/gpiolib.c
>>> @@ -2644,6 +2644,24 @@ static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
>>>         return desc;
>>>  }
>>>
>>> +struct gpio_desc *dev_get_gpiod_flags(struct device *dev, unsigned int idx,
>>> +                                     enum gpio_lookup_flags *flags)
>>> +{
>>> +       struct gpio_desc *desc = ERR_PTR(-ENOENT);
>>> +
>>> +       if (!dev || !flags)
>>> +               return ERR_PTR(-EINVAL);
>>> +
>>> +       /* Using device tree? */
>>> +       if (IS_ENABLED(CONFIG_OF) && dev->of_node)
>>> +               desc = of_find_gpio(dev, NULL, idx, flags);
>>> +       else if (IS_ENABLED(CONFIG_ACPI) && ACPI_COMPANION(dev))
>>> +               desc = acpi_get_gpiod_flags(dev, idx, flags);
>>> +
>>> +       return desc;
>>> +}
>>> +EXPORT_SYMBOL(dev_get_gpiod_flags);
>>
>> Putting aside the fact that this function is clearly ACPI-centric (no
>> con_id parameter and no handling of the platform interface), I have
>> two big problems with it and it ending up in the consumer interface:
>>
>> 1) The returned descriptor is not requested by gpiolib, which means no
>> check is made about whether the GPIO has already been requested by
>> someone else, and another driver can very well request the same GPIO
>> later and obtain it. Any descriptor returned by a function in
>> consumer.h *must* be properly requested. Furthermore the 1:1 mapping
>> between GPIO descriptors and GPIO numbers is not something we can take
>> for granted (since it will likely change soon), so this practice is
>> definitely to ban.
> 
> My bad, somehow I missed the part that it never requested the GPIO.
> Thanks for pointing it out.
> 
>> 2) It exposes the GPIO flags, while they are supposed to be opaque to consumers.
> 
> And this, of course we should be using gpiod_is_active_low() and similar
> functions that work with descriptors.
> 
>> These two points would somehow be acceptable if this function was
>> gpiolib-private, but here it is clearly not the case and this allows
>> pretty nasty thing to happen. Basically you are using it to take
>> advantage of the gpiod lookup mechanism and then quickly fall back to
>> the legacy integer interface. That's really not something to encourage
>> - these drivers should be converted to use gpiod internally (while
>> preserving integer-based lookup for compatiblity, if needed).
>>
>> In patch 8 you say:
>>
>> "this can be solved by adding a new field of type
>> struct gpio_desc but then there is another problem: the devm_gpiod_get
>> needs to operate on the button device instead of its parent device that
>> has the driver binded, so when the driver is unloaded, the resources for
>> the gpio will not get freed automatically."
>>
>> I'd very much prefer that you use the non-devm variant of gpiod_get()
>> and free the resources manually when the driver is unloaded than this
>> workaround that introduces an loophole in the gpiod consumer lookup
>> functions.
> 
> I agree and we are going to rework this and the consumer patches to do
> exactly what you say.
 
I agree, and thanks for the suggestions Alexandre.
Will work on this and send an update when it's ready.

Regards,
Aaron

Re: [RFC PATCH 6/9] gpiolib: add API to get gpio desc and flags

[Alexandre Courbot]

On Tue, Aug 19, 2014 at 1:56 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Mon, Aug 18, 2014 at 09:24:48AM -0700, Alexandre Courbot wrote:
>> On Fri, Aug 15, 2014 at 11:53 PM, Mika Westerberg
>> <mika.westerberg@linux.intel.com> wrote:
>> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> > index 2ebc9071e354..e6c2413a6fbf 100644
>> > --- a/drivers/gpio/gpiolib.c
>> > +++ b/drivers/gpio/gpiolib.c
>> > @@ -2644,6 +2644,24 @@ static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
>> >         return desc;
>> >  }
>> >
>> > +struct gpio_desc *dev_get_gpiod_flags(struct device *dev, unsigned int idx,
>> > +                                     enum gpio_lookup_flags *flags)
>> > +{
>> > +       struct gpio_desc *desc = ERR_PTR(-ENOENT);
>> > +
>> > +       if (!dev || !flags)
>> > +               return ERR_PTR(-EINVAL);
>> > +
>> > +       /* Using device tree? */
>> > +       if (IS_ENABLED(CONFIG_OF) && dev->of_node)
>> > +               desc = of_find_gpio(dev, NULL, idx, flags);
>> > +       else if (IS_ENABLED(CONFIG_ACPI) && ACPI_COMPANION(dev))
>> > +               desc = acpi_get_gpiod_flags(dev, idx, flags);
>> > +
>> > +       return desc;
>> > +}
>> > +EXPORT_SYMBOL(dev_get_gpiod_flags);
>>
>> Putting aside the fact that this function is clearly ACPI-centric (no
>> con_id parameter and no handling of the platform interface), I have
>> two big problems with it and it ending up in the consumer interface:
>>
>> 1) The returned descriptor is not requested by gpiolib, which means no
>> check is made about whether the GPIO has already been requested by
>> someone else, and another driver can very well request the same GPIO
>> later and obtain it. Any descriptor returned by a function in
>> consumer.h *must* be properly requested. Furthermore the 1:1 mapping
>> between GPIO descriptors and GPIO numbers is not something we can take
>> for granted (since it will likely change soon), so this practice is
>> definitely to ban.
>
> My bad, somehow I missed the part that it never requested the GPIO.
> Thanks for pointing it out.
>
>> 2) It exposes the GPIO flags, while they are supposed to be opaque to consumers.
>
> And this, of course we should be using gpiod_is_active_low() and similar
> functions that work with descriptors.

Yes, although if you convert the driver to use descriptors you should
not even have to worry about active_low status.

For drivers that still need to handle GPIO numbers for compatibility
reasons, it might be nice if gpiolib provided a gpio_to_desc() variant
that accepts an ACTIVE_LOW flag, so you don't have to worry about the
active low status once you have converted your GPIO number to a
descriptor. Actually for these cases we may be better with a function
that does what gpio_to_desc() does, but also requests the GPIO and
allows some flags to be specified so the integer-handling part of
drivers can be completely dropped afterwards. That's another problem
though. :)

>
>> These two points would somehow be acceptable if this function was
>> gpiolib-private, but here it is clearly not the case and this allows
>> pretty nasty thing to happen. Basically you are using it to take
>> advantage of the gpiod lookup mechanism and then quickly fall back to
>> the legacy integer interface. That's really not something to encourage
>> - these drivers should be converted to use gpiod internally (while
>> preserving integer-based lookup for compatiblity, if needed).
>>
>> In patch 8 you say:
>>
>> "this can be solved by adding a new field of type
>> struct gpio_desc but then there is another problem: the devm_gpiod_get
>> needs to operate on the button device instead of its parent device that
>> has the driver binded, so when the driver is unloaded, the resources for
>> the gpio will not get freed automatically."
>>
>> I'd very much prefer that you use the non-devm variant of gpiod_get()
>> and free the resources manually when the driver is unloaded than this
>> workaround that introduces an loophole in the gpiod consumer lookup
>> functions.
>
> I agree and we are going to rework this and the consumer patches to do
> exactly what you say.

Great, thanks to you and Aaron for your understanding!

Alex.

Re: [RFC PATCH 5/9] mfd: Add ACPI support

[Lee Jones]

On Sat, 16 Aug 2014, Mika Westerberg wrote:
> If an MFD device is backed by ACPI namespace, we should allow subdevice
> drivers to access their corresponding ACPI companion devices through normal
> means (e.g using ACPI_COMPANION()).
> 
> This patch adds such support to the MFD core. If the MFD parent device
> doesn't specify any ACPI _HID/_CID for the child device, the child device
> will share the parent ACPI companion device. Otherwise the child device
> will be assigned with the corresponding ACPI companion, if found in the
> namespace below the parent.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Reviewed-by: Darren Hart <dvhart@linux.intel.com>
> ---
>  Documentation/acpi/enumeration.txt | 27 +++++++++++++++++++++++++
>  drivers/mfd/mfd-core.c             | 41 ++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/core.h           |  3 +++
>  3 files changed, 71 insertions(+)
> 
> diff --git a/Documentation/acpi/enumeration.txt b/Documentation/acpi/enumeration.txt
> index e182be5e3c83..74e35c54febf 100644
> --- a/Documentation/acpi/enumeration.txt
> +++ b/Documentation/acpi/enumeration.txt
> @@ -312,3 +312,30 @@ a code like this:
>  
>  There are also devm_* versions of these functions which release the
>  descriptors once the device is released.
> +
> +MFD devices
> +~~~~~~~~~~~
> +The MFD devices create platform devices from their children. For the

What does this mean?  MFD drivers register their children _as_
platform devices.

> +child devices there needs to be an ACPI handle that they can use to
> +reference parts of the ACPI namespace that relate to them. In the Linux
> +MFD subsystem we provide two ways:
> +
> +	o The children share the parent ACPI handle.
> +	o The MFD cell can specify the ACPI id of the device.
> +
> +For the first case, the MFD drivers do not need to do anything. The
> +resulting child platform device will have its ACPI_COMPANION() set to point
> +to the parent device.
> +
> +If the ACPI namespace has a device that we can match using an ACPI id,
> +the id should be set like:
> +
> +	static struct mfd_cell my_subdevice_cell = {
> +		.name = "my_subdevice",
> +		/* set the resources relative to the parent */
> +		.acpi_pnpid = "XYZ0001",
> +	};
> +
> +The ACPI id "XYZ0001" is then used to lookup an ACPI device directly under
> +the MFD device and if found, that ACPI companion device is bound to the
> +resulting child platform device.
> diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
> index 892d343193ad..bb466b28b3b6 100644
> --- a/drivers/mfd/mfd-core.c
> +++ b/drivers/mfd/mfd-core.c
> @@ -78,6 +78,45 @@ static int mfd_platform_add_cell(struct platform_device *pdev,
>  	return 0;
>  }
>  
> +#if IS_ENABLED(CONFIG_ACPI)
> +static void mfd_acpi_add_device(const struct mfd_cell *cell,
> +				struct platform_device *pdev)
> +{
> +	struct acpi_device *parent_adev;
> +	struct acpi_device *adev = NULL;
> +
> +	parent_adev = ACPI_COMPANION(pdev->dev.parent);
> +	if (!parent_adev)
> +		return;
> +
> +	/*
> +	 * MFD child device gets its ACPI handle either from the ACPI
> +	 * device directly under the parent that matches the acpi_pnpid or
> +	 * it will use the parent handle if is no acpi_pnpid is given.
> +	 */
> +	if (cell->acpi_pnpid) {
> +		struct acpi_device_id ids[2] = {};
> +		struct acpi_device *child_adev;
> +
> +		strlcpy(ids[0].id, cell->acpi_pnpid, sizeof(ids[0].id));
> +		list_for_each_entry(child_adev, &parent_adev->children, node)
> +			if (acpi_match_device_ids(child_adev, ids)) {
> +				adev = child_adev;
> +				break;
> +			}
> +	} else {
> +		adev = parent_adev;
> +	}
> +
> +	ACPI_COMPANION_SET(&pdev->dev, adev);
> +}
> +#else
> +static inline void mfd_acpi_add_device(const struct mfd_cell *cell,
> +				       struct platform_device *pdev)
> +{
> +}
> +#endif
> +

I'm not keen on polluting the MFD core driver with #differy.  Can't you
think of another way to do it?

>  static int mfd_add_device(struct device *parent, int id,
>  			  const struct mfd_cell *cell, atomic_t *usage_count,
>  			  struct resource *mem_base,
> @@ -118,6 +157,8 @@ static int mfd_add_device(struct device *parent, int id,
>  		}
>  	}
>  
> +	mfd_acpi_add_device(cell, pdev);
> +
>  	if (cell->pdata_size) {
>  		ret = platform_device_add_data(pdev,
>  					cell->platform_data, cell->pdata_size);
> diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
> index f543de91ce19..73e1709d4c09 100644
> --- a/include/linux/mfd/core.h
> +++ b/include/linux/mfd/core.h
> @@ -44,6 +44,9 @@ struct mfd_cell {
>  	 */
>  	const char		*of_compatible;
>  
> +	/* Matches ACPI PNP id, either _HID or _CID */
> +	const char		*acpi_pnpid;
> +
>  	/*
>  	 * These resources can be specified relative to the parent device.
>  	 * For accessing hardware you should use resources from the platform dev

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [RFC PATCH 5/9] mfd: Add ACPI support

[Mika Westerberg]

On Wed, Aug 20, 2014 at 04:54:59PM +0100, Lee Jones wrote:
> On Sat, 16 Aug 2014, Mika Westerberg wrote:
> > If an MFD device is backed by ACPI namespace, we should allow subdevice
> > drivers to access their corresponding ACPI companion devices through normal
> > means (e.g using ACPI_COMPANION()).
> > 
> > This patch adds such support to the MFD core. If the MFD parent device
> > doesn't specify any ACPI _HID/_CID for the child device, the child device
> > will share the parent ACPI companion device. Otherwise the child device
> > will be assigned with the corresponding ACPI companion, if found in the
> > namespace below the parent.
> > 
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Reviewed-by: Darren Hart <dvhart@linux.intel.com>
> > ---
> >  Documentation/acpi/enumeration.txt | 27 +++++++++++++++++++++++++
> >  drivers/mfd/mfd-core.c             | 41 ++++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/core.h           |  3 +++
> >  3 files changed, 71 insertions(+)
> > 
> > diff --git a/Documentation/acpi/enumeration.txt b/Documentation/acpi/enumeration.txt
> > index e182be5e3c83..74e35c54febf 100644
> > --- a/Documentation/acpi/enumeration.txt
> > +++ b/Documentation/acpi/enumeration.txt
> > @@ -312,3 +312,30 @@ a code like this:
> >  
> >  There are also devm_* versions of these functions which release the
> >  descriptors once the device is released.
> > +
> > +MFD devices
> > +~~~~~~~~~~~
> > +The MFD devices create platform devices from their children. For the
> 
> What does this mean?  MFD drivers register their children _as_
> platform devices.

Right, will fix.

> > +child devices there needs to be an ACPI handle that they can use to
> > +reference parts of the ACPI namespace that relate to them. In the Linux
> > +MFD subsystem we provide two ways:
> > +
> > +	o The children share the parent ACPI handle.
> > +	o The MFD cell can specify the ACPI id of the device.
> > +
> > +For the first case, the MFD drivers do not need to do anything. The
> > +resulting child platform device will have its ACPI_COMPANION() set to point
> > +to the parent device.
> > +
> > +If the ACPI namespace has a device that we can match using an ACPI id,
> > +the id should be set like:
> > +
> > +	static struct mfd_cell my_subdevice_cell = {
> > +		.name = "my_subdevice",
> > +		/* set the resources relative to the parent */
> > +		.acpi_pnpid = "XYZ0001",
> > +	};
> > +
> > +The ACPI id "XYZ0001" is then used to lookup an ACPI device directly under
> > +the MFD device and if found, that ACPI companion device is bound to the
> > +resulting child platform device.
> > diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
> > index 892d343193ad..bb466b28b3b6 100644
> > --- a/drivers/mfd/mfd-core.c
> > +++ b/drivers/mfd/mfd-core.c
> > @@ -78,6 +78,45 @@ static int mfd_platform_add_cell(struct platform_device *pdev,
> >  	return 0;
> >  }
> >  
> > +#if IS_ENABLED(CONFIG_ACPI)
> > +static void mfd_acpi_add_device(const struct mfd_cell *cell,
> > +				struct platform_device *pdev)
> > +{
> > +	struct acpi_device *parent_adev;
> > +	struct acpi_device *adev = NULL;
> > +
> > +	parent_adev = ACPI_COMPANION(pdev->dev.parent);
> > +	if (!parent_adev)
> > +		return;
> > +
> > +	/*
> > +	 * MFD child device gets its ACPI handle either from the ACPI
> > +	 * device directly under the parent that matches the acpi_pnpid or
> > +	 * it will use the parent handle if is no acpi_pnpid is given.
> > +	 */
> > +	if (cell->acpi_pnpid) {
> > +		struct acpi_device_id ids[2] = {};
> > +		struct acpi_device *child_adev;
> > +
> > +		strlcpy(ids[0].id, cell->acpi_pnpid, sizeof(ids[0].id));
> > +		list_for_each_entry(child_adev, &parent_adev->children, node)
> > +			if (acpi_match_device_ids(child_adev, ids)) {
> > +				adev = child_adev;
> > +				break;
> > +			}
> > +	} else {
> > +		adev = parent_adev;
> > +	}
> > +
> > +	ACPI_COMPANION_SET(&pdev->dev, adev);
> > +}
> > +#else
> > +static inline void mfd_acpi_add_device(const struct mfd_cell *cell,
> > +				       struct platform_device *pdev)
> > +{
> > +}
> > +#endif
> > +
> 
> I'm not keen on polluting the MFD core driver with #differy.  Can't you
> think of another way to do it?

It may be possible to do that since we only use few ACPI functions that
have dummy stubs already.

> 
> >  static int mfd_add_device(struct device *parent, int id,
> >  			  const struct mfd_cell *cell, atomic_t *usage_count,
> >  			  struct resource *mem_base,
> > @@ -118,6 +157,8 @@ static int mfd_add_device(struct device *parent, int id,
> >  		}
> >  	}
> >  
> > +	mfd_acpi_add_device(cell, pdev);
> > +
> >  	if (cell->pdata_size) {
> >  		ret = platform_device_add_data(pdev,
> >  					cell->platform_data, cell->pdata_size);
> > diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
> > index f543de91ce19..73e1709d4c09 100644
> > --- a/include/linux/mfd/core.h
> > +++ b/include/linux/mfd/core.h
> > @@ -44,6 +44,9 @@ struct mfd_cell {
> >  	 */
> >  	const char		*of_compatible;
> >  
> > +	/* Matches ACPI PNP id, either _HID or _CID */
> > +	const char		*acpi_pnpid;
> > +
> >  	/*
> >  	 * These resources can be specified relative to the parent device.
> >  	 * For accessing hardware you should use resources from the platform dev
> 
> -- 
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [RFC PATCH 7/9] gpio: sch: Consolidate core and resume banks

[Linus Walleij]

On Sat, Aug 16, 2014 at 8:53 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:

> This is actually a single device with two sets of identical registers,
> which just happen to start from a different offset. Instead of having
> separate GPIO chips created we consolidate them to be single GPIO chip.
>
> In addition having a single GPIO chip allows us to handle ACPI GPIO
> translation in the core in a more generic way, since the two GPIO chips
> share the same parent ACPI device.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Tested-by: Max Eliaser <max.eliaser@intel.com>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

I guess this needs to be merged with the rest of the stuff in this series
so for the GPIO sch part go ahead.

Yours,
Linus Walleij

[RFC PATCH 0/9] Add ACPI _DSD and unified device properties support

[Mika Westerberg]

[ Resending with fewer people in CC list. For some reason the series never
  reached attented mailing lists. ]

The recent publication of the ACPI 5.1 specification [1] adds a reserved name
for Device Specific Data (_DSD, Section 6.2.5). This mechanism allows for
passing arbitrary hardware description data to the OS. The exact format of the
_DSD data is specific to the UUID paired with it [2].   

An ACPI Device Properties UUID has been defined [3] to provide a format
compatible with existing device tree schemas. The purpose for this was to allow
for the reuse of the existing schemas and encourage the development of firmware
agnostic device drivers.

This series accomplishes the following (as well as some other dependencies):

 * Add _DSD support to the ACPI core
   This simply reads the UUID and the accompanying Package

 * Add ACPI Device Properties _DSD format support
   This understands the hierarchical key:value pair structure
   defined by the Device Properties UUID

 * Add a unified device properties API with ACPI and OF backends
   This provides for the firmware agnostic device properties
   Interface to be used by drivers

 * Provides 2 example drivers that were previously Device Tree aware that
   can now be used with either Device Tree or ACPI Device Properties. The
   both drivers use an arbitrary _HID.

This has been tested on Minnowboard with relevant parts of the modified
DSDT at the end of this email.

This series does not provide for a means to append to a system DSDT. That
will ultimately be required to make the most effective use of the _DSD
mechanism. Work is underway on that as a separate effort.

[1] http://www.uefi.org/sites/default/files/resources/ACPI_5_1release.pdf
[2] http://www.uefi.org/sites/default/files/resources/_DSD-implementation-guide-toplevel.htm
[3] http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf

Aaron Lu (3):
  of: Add property_ops callback for devices with of_node
  gpiolib: add API to get gpio desc and flags
  Input: gpio_keys_polled - Make use of device property API

Max Eliaser (1):
  leds: leds-gpio: Make use of device property API

Mika Westerberg (4):
  ACPI: Add support for device specific properties
  ACPI: Document ACPI device specific properties
  mfd: Add ACPI support
  gpio: sch: Consolidate core and resume banks

Rafael J. Wysocki (1):
  Driver core: Unified device properties interface for platform firmware

 Documentation/acpi/enumeration.txt        |  27 ++
 Documentation/acpi/properties.txt         | 359 ++++++++++++++++++++
 drivers/acpi/Makefile                     |   1 +
 drivers/acpi/glue.c                       |   4 +-
 drivers/acpi/internal.h                   |   6 +
 drivers/acpi/property.c                   | 542 ++++++++++++++++++++++++++++++
 drivers/acpi/scan.c                       |  14 +-
 drivers/base/Makefile                     |   2 +-
 drivers/base/property.c                   |  48 +++
 drivers/gpio/gpio-sch.c                   | 303 ++++++-----------
 drivers/gpio/gpiolib-acpi.c               |  81 +++--
 drivers/gpio/gpiolib.c                    |  18 +
 drivers/gpio/gpiolib.h                    |   8 +
 drivers/input/keyboard/gpio_keys_polled.c | 139 +++++---
 drivers/leds/leds-gpio.c                  | 130 ++++---
 drivers/mfd/mfd-core.c                    |  41 +++
 drivers/of/base.c                         | 194 ++++++++++-
 drivers/of/platform.c                     |   4 +-
 include/acpi/acpi_bus.h                   |   8 +
 include/linux/acpi.h                      |  40 +++
 include/linux/device.h                    |   3 +
 include/linux/gpio/consumer.h             |  11 +
 include/linux/mfd/core.h                  |   3 +
 include/linux/property.h                  |  54 +++
 24 files changed, 1716 insertions(+), 324 deletions(-)
 create mode 100644 Documentation/acpi/properties.txt
 create mode 100644 drivers/acpi/property.c
 create mode 100644 drivers/base/property.c
 create mode 100644 include/linux/property.h

------ DSDT For Minnowboard ------

    Scope (\_SB.PCI0.LPC)
    {
        Device (LEDS)
        {
            Name (_HID, "MNW0001")

            Name (_CRS, ResourceTemplate ()
            {
                GpioIo (Exclusive, PullDown, 0, 0, IoRestrictionOutputOnly,
                        "\\_SB.PCI0.LPC", 0, ResourceConsumer) {10}
                GpioIo (Exclusive, PullDown, 0, 0, IoRestrictionOutputOnly,
                        "\\_SB.PCI0.LPC", 0, ResourceConsumer) {11}
            })

            Device (LEDH)
            {
                Name (_HID, "PRP0000")
                Name (_DSD, Package ()
                {
                    ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
                    Package ()
                    {
                        Package () {"label", "Heartbeat"},
                        Package () {"gpios", Package () {^^LEDS, 0, 0, 0}},
                        Package () {"linux,default-trigger", "heartbeat"},
                        Package () {"linux,default-state", "off"},
                        Package () {"linux,retain-state-suspended", 1},
                    }
                })
            }

            Device (LEDM)
            {
                Name (_HID, "PRP0000")
                Name (_DSD, Package ()
                {
                    ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
                    Package ()
                    {
                        Package () {"label", "MMC0 Activity"},
                        Package () {"gpios", Package () {^^LEDS, 1, 0, 0}},
                        Package () {"linux,default-trigger", "mmc0"},
                        Package () {"linux,default-state", "on"},
                        Package () {"linux,retain-state-suspended", 1},
                    }
                })
            }
        }
        
        Device (BTNS)
        {
            Name (_HID, "MNW0002")

            Name (_CRS, ResourceTemplate ()
            {
                GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly,
                        "\\_SB.PCI0.LPC", 0, ResourceConsumer) {0}
                GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly,
                        "\\_SB.PCI0.LPC", 0, ResourceConsumer) {1}
                GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly,
                        "\\_SB.PCI0.LPC", 0, ResourceConsumer) {2}
                GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly,
                        "\\_SB.PCI0.LPC", 0, ResourceConsumer) {3}
            })

            Name (_DSD, Package ()
            {
                ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
                Package ()
                {
                    Package () {"poll-interval", 100},
                    Package () {"autorepeat", 1}
                }
            })

            Device (BTN0)
            {
                Name (_HID, "PRP0000")
                Name (_DSD, Package ()
                {
                    ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
                    Package ()
                    {
                        Package () {"linux,code", 105},
                        Package () {"linux,input-type", 1},
                        Package () {"gpios", Package () {^^BTNS, 0, 0, 1}},
                    }
                })
            }

            Device (BTN1)
            {
                Name (_HID, "PRP0000")
                Name (_DSD, Package ()
                {
                    ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
                    Package ()
                    {
                        Package () {"linux,code", 108},
                        Package () {"linux,input-type", 1},
                        Package () {"gpios", Package (4) {^^BTNS, 1, 0, 1}},
                    }
                })
            }
            
            Device (BTN2)
            {
                Name (_HID, "PRP0000")
                Name (_DSD, Package ()
                {
                    ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
                    Package ()
                    {
                        Package () {"linux,code", 103},
                        Package () {"linux,input-type", 1},
                        Package () {"gpios", Package () {^^BTNS, 2, 0, 1}},
                    }
                })
            }
            
            Device (BTN3)
            {
                Name (_HID, "PRP0000")
                Name (_DSD, Package ()
                {
                    ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
                    Package ()
                    {
                        Package () {"linux,code", 106},
                        Package () {"linux,input-type", 1},
                        Package () {"gpios", Package (4) {^^BTNS, 3, 0, 1}},
                    }
                })
            }

        }

    }

-- 
2.1.0.rc1

[RFC PATCH 6/9] gpiolib: add API to get gpio desc and flags

[Mika Westerberg]

From: Aaron Lu <aaron.lu@intel.com>

Add a new API to get the GPIO's description pointer and its flags for
both OF based system and ACPI based system. This is useful in drivers
that do not need to care about the underlying firmware interface.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
Signed-off-by: Max Eliaser <max.eliaser@intel.com>
Signed-off-by: Darren Hart <dvhart@linux.intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/gpio/gpiolib-acpi.c   | 81 ++++++++++++++++++++++++++++++++-----------
 drivers/gpio/gpiolib.c        | 18 ++++++++++
 drivers/gpio/gpiolib.h        |  8 +++++
 include/linux/gpio/consumer.h | 11 ++++++
 4 files changed, 97 insertions(+), 21 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 4a987917c186..f35e88d29a47 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -293,11 +293,38 @@ static int acpi_find_gpio(struct acpi_resource *ares, void *data)
 			agpio->connection_type == ACPI_RESOURCE_GPIO_TYPE_INT;
 		lookup->info.active_low =
 			agpio->polarity == ACPI_ACTIVE_LOW;
+		lookup->info.pin_config = agpio->pin_config;
 	}
 
 	return 1;
 }
 
+static struct gpio_desc *
+__acpi_get_gpiod_by_index(struct acpi_device *adev, int index,
+			  struct acpi_gpio_info *info)
+{
+	struct acpi_gpio_lookup lookup;
+	struct list_head resource_list;
+	int ret;
+
+	memset(&lookup, 0, sizeof(lookup));
+	lookup.index = index;
+
+	INIT_LIST_HEAD(&resource_list);
+	ret = acpi_dev_get_resources(adev, &resource_list, acpi_find_gpio,
+				     &lookup);
+	if (ret < 0)
+		return ERR_PTR(ret);
+
+	acpi_dev_free_resource_list(&resource_list);
+
+	if (lookup.desc && info)
+		*info = lookup.info;
+
+	return lookup.desc ? lookup.desc : ERR_PTR(-ENOENT);
+}
+
+
 /**
  * acpi_get_gpiod_by_index() - get a GPIO descriptor from device resources
  * @dev: pointer to a device to get GPIO from
@@ -318,34 +345,46 @@ static int acpi_find_gpio(struct acpi_resource *ares, void *data)
 struct gpio_desc *acpi_get_gpiod_by_index(struct device *dev, int index,
 					  struct acpi_gpio_info *info)
 {
-	struct acpi_gpio_lookup lookup;
-	struct list_head resource_list;
-	struct acpi_device *adev;
-	acpi_handle handle;
-	int ret;
-
-	if (!dev)
+	if (!dev || !ACPI_COMPANION(dev))
 		return ERR_PTR(-EINVAL);
+	return __acpi_get_gpiod_by_index(ACPI_COMPANION(dev), index, info);
+}
 
-	handle = ACPI_HANDLE(dev);
-	if (!handle || acpi_bus_get_device(handle, &adev))
-		return ERR_PTR(-ENODEV);
-
-	memset(&lookup, 0, sizeof(lookup));
-	lookup.index = index;
+struct gpio_desc *acpi_get_gpiod_flags(struct device *dev, int idx,
+				       enum gpio_lookup_flags *flags)
+{
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+	struct acpi_reference_args args;
+	struct acpi_gpio_info info;
+	struct gpio_desc *desc;
+	bool active_low;
+	int ret;
 
-	INIT_LIST_HEAD(&resource_list);
-	ret = acpi_dev_get_resources(adev, &resource_list, acpi_find_gpio,
-				     &lookup);
-	if (ret < 0)
+	ret = acpi_dev_get_property_reference(adev, "gpios", NULL, idx, &args);
+	if (ret)
 		return ERR_PTR(ret);
 
-	acpi_dev_free_resource_list(&resource_list);
+	desc = __acpi_get_gpiod_by_index(args.adev, args.args[0], &info);
+	if (IS_ERR(desc))
+		return desc;
 
-	if (lookup.desc && info)
-		*info = lookup.info;
+	/*
+	 * The 3rd argument optionally specifies the pin polarity. We
+	 * use that if it exists, otherwise we resort to the pin config.
+	 * (Note that the first element of the "gpios" package goes into
+	 * arg.adev, not args.args.)
+	 */
+	if (args.nargs >= 3)
+		active_low = !!args.args[2];
+	else if (info.gpioint)
+		active_low = !!info.active_low;
+	else
+		active_low = !!(info.pin_config & ACPI_PIN_CONFIG_PULLUP);
 
-	return lookup.desc ? lookup.desc : ERR_PTR(-ENOENT);
+	if (active_low)
+		*flags |= GPIO_ACTIVE_LOW;
+
+	return desc;
 }
 
 static acpi_status
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 2ebc9071e354..e6c2413a6fbf 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2644,6 +2644,24 @@ static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
 	return desc;
 }
 
+struct gpio_desc *dev_get_gpiod_flags(struct device *dev, unsigned int idx,
+				      enum gpio_lookup_flags *flags)
+{
+	struct gpio_desc *desc = ERR_PTR(-ENOENT);
+
+	if (!dev || !flags)
+		return ERR_PTR(-EINVAL);
+
+	/* Using device tree? */
+	if (IS_ENABLED(CONFIG_OF) && dev->of_node)
+		desc = of_find_gpio(dev, NULL, idx, flags);
+	else if (IS_ENABLED(CONFIG_ACPI) && ACPI_COMPANION(dev))
+		desc = acpi_get_gpiod_flags(dev, idx, flags);
+
+	return desc;
+}
+EXPORT_SYMBOL(dev_get_gpiod_flags);
+
 static struct gpiod_lookup_table *gpiod_find_lookup_table(struct device *dev)
 {
 	const char *dev_id = dev ? dev_name(dev) : NULL;
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 1a4103dd38df..a759db968e51 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -25,6 +25,7 @@ enum of_gpio_flags;
 struct acpi_gpio_info {
 	bool gpioint;
 	bool active_low;
+	u8 pin_config;
 };
 
 #ifdef CONFIG_ACPI
@@ -33,6 +34,8 @@ void acpi_gpiochip_remove(struct gpio_chip *chip);
 
 struct gpio_desc *acpi_get_gpiod_by_index(struct device *dev, int index,
 					  struct acpi_gpio_info *info);
+struct gpio_desc *acpi_get_gpiod_flags(struct device *dev, int index,
+				       enum gpio_lookup_flags *flags);
 #else
 static inline void acpi_gpiochip_add(struct gpio_chip *chip) { }
 static inline void acpi_gpiochip_remove(struct gpio_chip *chip) { }
@@ -43,6 +46,11 @@ acpi_get_gpiod_by_index(struct device *dev, int index,
 {
 	return ERR_PTR(-ENOSYS);
 }
+static struct gpio_desc *acpi_get_gpiod_flags(struct device *dev, int index,
+					      enum gpio_lookup_flags *flags)
+{
+	return ERR_PTR(-ENOSYS);
+}
 #endif
 
 int gpiochip_request_own_desc(struct gpio_desc *desc, const char *label);
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index 05e53ccb708b..53f422e4f0c9 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -7,6 +7,8 @@
 
 struct device;
 
+enum gpio_lookup_flags;
+
 /**
  * Opaque descriptor for a GPIO. These are obtained using gpiod_get() and are
  * preferable to the old integer-based handles.
@@ -73,6 +75,9 @@ int gpiod_to_irq(const struct gpio_desc *desc);
 struct gpio_desc *gpio_to_desc(unsigned gpio);
 int desc_to_gpio(const struct gpio_desc *desc);
 
+struct gpio_desc *dev_get_gpiod_flags(struct device *dev, unsigned int idx,
+				      enum gpio_lookup_flags *flags);
+
 #else /* CONFIG_GPIOLIB */
 
 static inline struct gpio_desc *__must_check gpiod_get(struct device *dev,
@@ -254,6 +259,12 @@ static inline int desc_to_gpio(const struct gpio_desc *desc)
 	return -EINVAL;
 }
 
+static inline struct gpio_desc *dev_get_gpiod_flags(struct device *dev,
+			unsigned int idx, enum gpio_lookup_flags *flags)
+{
+	return ERR_PTR(-ENOSYS);
+}
+
 
 #endif /* CONFIG_GPIOLIB */
 
-- 
2.1.0.rc1

Re: [RFC PATCH 6/9] gpiolib: add API to get gpio desc and flags

[Grant Likely]

On Sun, 17 Aug 2014 09:04:16 +0300, Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
> From: Aaron Lu <aaron.lu@intel.com>
> 
> Add a new API to get the GPIO's description pointer and its flags for
> both OF based system and ACPI based system. This is useful in drivers
> that do not need to care about the underlying firmware interface.

Hi Mika,

I've only looked at this one briefly, and noticed one problem below...

g.

> 
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> Signed-off-by: Max Eliaser <max.eliaser@intel.com>
> Signed-off-by: Darren Hart <dvhart@linux.intel.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/gpio/gpiolib-acpi.c   | 81 ++++++++++++++++++++++++++++++++-----------
>  drivers/gpio/gpiolib.c        | 18 ++++++++++
>  drivers/gpio/gpiolib.h        |  8 +++++
>  include/linux/gpio/consumer.h | 11 ++++++
>  4 files changed, 97 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index 4a987917c186..f35e88d29a47 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -293,11 +293,38 @@ static int acpi_find_gpio(struct acpi_resource *ares, void *data)
>  			agpio->connection_type == ACPI_RESOURCE_GPIO_TYPE_INT;
>  		lookup->info.active_low =
>  			agpio->polarity == ACPI_ACTIVE_LOW;
> +		lookup->info.pin_config = agpio->pin_config;
>  	}
>  
>  	return 1;
>  }
>  
> +static struct gpio_desc *
> +__acpi_get_gpiod_by_index(struct acpi_device *adev, int index,
> +			  struct acpi_gpio_info *info)
> +{
> +	struct acpi_gpio_lookup lookup;
> +	struct list_head resource_list;
> +	int ret;
> +
> +	memset(&lookup, 0, sizeof(lookup));
> +	lookup.index = index;
> +
> +	INIT_LIST_HEAD(&resource_list);
> +	ret = acpi_dev_get_resources(adev, &resource_list, acpi_find_gpio,
> +				     &lookup);
> +	if (ret < 0)
> +		return ERR_PTR(ret);
> +
> +	acpi_dev_free_resource_list(&resource_list);
> +
> +	if (lookup.desc && info)
> +		*info = lookup.info;
> +
> +	return lookup.desc ? lookup.desc : ERR_PTR(-ENOENT);
> +}
> +
> +
>  /**
>   * acpi_get_gpiod_by_index() - get a GPIO descriptor from device resources
>   * @dev: pointer to a device to get GPIO from
> @@ -318,34 +345,46 @@ static int acpi_find_gpio(struct acpi_resource *ares, void *data)
>  struct gpio_desc *acpi_get_gpiod_by_index(struct device *dev, int index,
>  					  struct acpi_gpio_info *info)
>  {
> -	struct acpi_gpio_lookup lookup;
> -	struct list_head resource_list;
> -	struct acpi_device *adev;
> -	acpi_handle handle;
> -	int ret;
> -
> -	if (!dev)
> +	if (!dev || !ACPI_COMPANION(dev))
>  		return ERR_PTR(-EINVAL);
> +	return __acpi_get_gpiod_by_index(ACPI_COMPANION(dev), index, info);
> +}
>  
> -	handle = ACPI_HANDLE(dev);
> -	if (!handle || acpi_bus_get_device(handle, &adev))
> -		return ERR_PTR(-ENODEV);
> -
> -	memset(&lookup, 0, sizeof(lookup));
> -	lookup.index = index;
> +struct gpio_desc *acpi_get_gpiod_flags(struct device *dev, int idx,
> +				       enum gpio_lookup_flags *flags)
> +{
> +	struct acpi_device *adev = ACPI_COMPANION(dev);
> +	struct acpi_reference_args args;
> +	struct acpi_gpio_info info;
> +	struct gpio_desc *desc;
> +	bool active_low;
> +	int ret;
>  
> -	INIT_LIST_HEAD(&resource_list);
> -	ret = acpi_dev_get_resources(adev, &resource_list, acpi_find_gpio,
> -				     &lookup);
> -	if (ret < 0)
> +	ret = acpi_dev_get_property_reference(adev, "gpios", NULL, idx, &args);
> +	if (ret)
>  		return ERR_PTR(ret);
>  
> -	acpi_dev_free_resource_list(&resource_list);
> +	desc = __acpi_get_gpiod_by_index(args.adev, args.args[0], &info);
> +	if (IS_ERR(desc))
> +		return desc;
>  
> -	if (lookup.desc && info)
> -		*info = lookup.info;
> +	/*
> +	 * The 3rd argument optionally specifies the pin polarity. We
> +	 * use that if it exists, otherwise we resort to the pin config.
> +	 * (Note that the first element of the "gpios" package goes into
> +	 * arg.adev, not args.args.)
> +	 */
> +	if (args.nargs >= 3)
> +		active_low = !!args.args[2];
> +	else if (info.gpioint)
> +		active_low = !!info.active_low;
> +	else
> +		active_low = !!(info.pin_config & ACPI_PIN_CONFIG_PULLUP);
>  
> -	return lookup.desc ? lookup.desc : ERR_PTR(-ENOENT);
> +	if (active_low)
> +		*flags |= GPIO_ACTIVE_LOW;
> +
> +	return desc;
>  }
>  
>  static acpi_status
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 2ebc9071e354..e6c2413a6fbf 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2644,6 +2644,24 @@ static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
>  	return desc;
>  }
>  
> +struct gpio_desc *dev_get_gpiod_flags(struct device *dev, unsigned int idx,
> +				      enum gpio_lookup_flags *flags)
> +{
> +	struct gpio_desc *desc = ERR_PTR(-ENOENT);
> +
> +	if (!dev || !flags)
> +		return ERR_PTR(-EINVAL);
> +
> +	/* Using device tree? */
> +	if (IS_ENABLED(CONFIG_OF) && dev->of_node)
> +		desc = of_find_gpio(dev, NULL, idx, flags);

of_find_gpio() doesn't exist.

> +	else if (IS_ENABLED(CONFIG_ACPI) && ACPI_COMPANION(dev))
> +		desc = acpi_get_gpiod_flags(dev, idx, flags);
> +
> +	return desc;
> +}
> +EXPORT_SYMBOL(dev_get_gpiod_flags);
> +
>  static struct gpiod_lookup_table *gpiod_find_lookup_table(struct device *dev)
>  {
>  	const char *dev_id = dev ? dev_name(dev) : NULL;
> diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
> index 1a4103dd38df..a759db968e51 100644
> --- a/drivers/gpio/gpiolib.h
> +++ b/drivers/gpio/gpiolib.h
> @@ -25,6 +25,7 @@ enum of_gpio_flags;
>  struct acpi_gpio_info {
>  	bool gpioint;
>  	bool active_low;
> +	u8 pin_config;
>  };
>  
>  #ifdef CONFIG_ACPI
> @@ -33,6 +34,8 @@ void acpi_gpiochip_remove(struct gpio_chip *chip);
>  
>  struct gpio_desc *acpi_get_gpiod_by_index(struct device *dev, int index,
>  					  struct acpi_gpio_info *info);
> +struct gpio_desc *acpi_get_gpiod_flags(struct device *dev, int index,
> +				       enum gpio_lookup_flags *flags);
>  #else
>  static inline void acpi_gpiochip_add(struct gpio_chip *chip) { }
>  static inline void acpi_gpiochip_remove(struct gpio_chip *chip) { }
> @@ -43,6 +46,11 @@ acpi_get_gpiod_by_index(struct device *dev, int index,
>  {
>  	return ERR_PTR(-ENOSYS);
>  }
> +static struct gpio_desc *acpi_get_gpiod_flags(struct device *dev, int index,
> +					      enum gpio_lookup_flags *flags)
> +{
> +	return ERR_PTR(-ENOSYS);
> +}
>  #endif
>  
>  int gpiochip_request_own_desc(struct gpio_desc *desc, const char *label);
> diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
> index 05e53ccb708b..53f422e4f0c9 100644
> --- a/include/linux/gpio/consumer.h
> +++ b/include/linux/gpio/consumer.h
> @@ -7,6 +7,8 @@
>  
>  struct device;
>  
> +enum gpio_lookup_flags;
> +
>  /**
>   * Opaque descriptor for a GPIO. These are obtained using gpiod_get() and are
>   * preferable to the old integer-based handles.
> @@ -73,6 +75,9 @@ int gpiod_to_irq(const struct gpio_desc *desc);
>  struct gpio_desc *gpio_to_desc(unsigned gpio);
>  int desc_to_gpio(const struct gpio_desc *desc);
>  
> +struct gpio_desc *dev_get_gpiod_flags(struct device *dev, unsigned int idx,
> +				      enum gpio_lookup_flags *flags);
> +
>  #else /* CONFIG_GPIOLIB */
>  
>  static inline struct gpio_desc *__must_check gpiod_get(struct device *dev,
> @@ -254,6 +259,12 @@ static inline int desc_to_gpio(const struct gpio_desc *desc)
>  	return -EINVAL;
>  }
>  
> +static inline struct gpio_desc *dev_get_gpiod_flags(struct device *dev,
> +			unsigned int idx, enum gpio_lookup_flags *flags)
> +{
> +	return ERR_PTR(-ENOSYS);
> +}
> +
>  
>  #endif /* CONFIG_GPIOLIB */
>  
> -- 
> 2.1.0.rc1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH 6/9] gpiolib: add API to get gpio desc and flags

[Darren Hart]

On 8/17/14, 6:00, "Grant Likely" <grant.likely@secretlab.ca> wrote:

>>
>>+	/* Using device tree? */
>>+	if (IS_ENABLED(CONFIG_OF) && dev->of_node)
>>+		desc = of_find_gpio(dev, NULL, idx, flags);
>
>of_find_gpio() doesn't exist.

Hrm... As of 3.16.0 (e64df3ebe8262c8203d1fe4f541e0241c3112c01)

$ git blame -L1455,1456 drivers/gpio/gpiolib.c
bae48da2 (Alexandre Courbot 2013-10-17 10:21:38 -0700 1455) static struct
gpio_desc *of_find_gpio(struct device *dev, const char *con_id,

Have we removed this in -next or something? (on the plane, will verify
upon landing)



-- 
Darren Hart					Open Source Technology Center
darren.hart@intel.com				            Intel Corporation

Re: [RFC PATCH 6/9] gpiolib: add API to get gpio desc and flags

[Rafael J. Wysocki]

On Sunday, August 17, 2014 12:43:38 PM Darren Hart wrote:
> On 8/17/14, 6:00, "Grant Likely" <grant.likely@secretlab.ca> wrote:
> 
> >>
> >>+	/* Using device tree? */
> >>+	if (IS_ENABLED(CONFIG_OF) && dev->of_node)
> >>+		desc = of_find_gpio(dev, NULL, idx, flags);
> >
> >of_find_gpio() doesn't exist.
> 
> Hrm... As of 3.16.0 (e64df3ebe8262c8203d1fe4f541e0241c3112c01)
> 
> $ git blame -L1455,1456 drivers/gpio/gpiolib.c
> bae48da2 (Alexandre Courbot 2013-10-17 10:21:38 -0700 1455) static struct
> gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
> 
> Have we removed this in -next or something? (on the plane, will verify
> upon landing)

In 3.17-rc1:

rafael@vostro:~/src/linux-pm> grep -r of_find_gpio *
drivers/gpio/gpiolib.c:static struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
drivers/gpio/gpiolib.c:         desc = of_find_gpio(dev, con_id, idx, &lookupflags);

Rafael

Re: [RFC PATCH 6/9] gpiolib: add API to get gpio desc and flags

[Aaron Lu]

On 08/18/2014 12:57 PM, Rafael J. Wysocki wrote:
> On Sunday, August 17, 2014 12:43:38 PM Darren Hart wrote:
>> On 8/17/14, 6:00, "Grant Likely" <grant.likely@secretlab.ca> wrote:
>>
>>>>
>>>> +	/* Using device tree? */
>>>> +	if (IS_ENABLED(CONFIG_OF) && dev->of_node)
>>>> +		desc = of_find_gpio(dev, NULL, idx, flags);
>>>
>>> of_find_gpio() doesn't exist.
>>
>> Hrm... As of 3.16.0 (e64df3ebe8262c8203d1fe4f541e0241c3112c01)
>>
>> $ git blame -L1455,1456 drivers/gpio/gpiolib.c
>> bae48da2 (Alexandre Courbot 2013-10-17 10:21:38 -0700 1455) static struct
>> gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
>>
>> Have we removed this in -next or something? (on the plane, will verify
>> upon landing)
> 
> In 3.17-rc1:
> 
> rafael@vostro:~/src/linux-pm> grep -r of_find_gpio *
> drivers/gpio/gpiolib.c:static struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
> drivers/gpio/gpiolib.c:         desc = of_find_gpio(dev, con_id, idx, &lookupflags);

I also verified the following branch:
git://git.secretlab.ca/git/linux devicetree/next
And of_find_gpio is there.

Regards,
Aaron

Re: [RFC PATCH 6/9] gpiolib: add API to get gpio desc and flags

[Grant Likely]

On Mon, 18 Aug 2014 06:57:41 +0200, "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:
> On Sunday, August 17, 2014 12:43:38 PM Darren Hart wrote:
> > On 8/17/14, 6:00, "Grant Likely" <grant.likely@secretlab.ca> wrote:
> > 
> > >>
> > >>+	/* Using device tree? */
> > >>+	if (IS_ENABLED(CONFIG_OF) && dev->of_node)
> > >>+		desc = of_find_gpio(dev, NULL, idx, flags);
> > >
> > >of_find_gpio() doesn't exist.
> > 
> > Hrm... As of 3.16.0 (e64df3ebe8262c8203d1fe4f541e0241c3112c01)
> > 
> > $ git blame -L1455,1456 drivers/gpio/gpiolib.c
> > bae48da2 (Alexandre Courbot 2013-10-17 10:21:38 -0700 1455) static struct
> > gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
> > 
> > Have we removed this in -next or something? (on the plane, will verify
> > upon landing)
> 
> In 3.17-rc1:
> 
> rafael@vostro:~/src/linux-pm> grep -r of_find_gpio *
> drivers/gpio/gpiolib.c:static struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
> drivers/gpio/gpiolib.c:         desc = of_find_gpio(dev, con_id, idx, &lookupflags);

Weird, I don't know why I couldn't find it. I must have been on a
different branch. Sorry for the noise.

g.