[PATCH v1 0/3]

[PATCH v1 0/3]

[Andy Shevchenko]

Here is a third [1] version of the SFI GPIO helpers. This series contains SFI
GPIO helpers along with the update of GPIO library to lookup for SFI resources.

It has been tested on Medfield device on top of v3.13-rc2 + Alexandre's GPIO
descriptor API update [2], Mika's ACPI gpio patches [3] and my GPIO library
clean up [4].

[1] https://lkml.org/lkml/2013/6/5/316
[2] http://www.spinics.net/lists/kernel/msg1645907.html
[3] http://www.spinics.net/lists/linux-acpi/msg47572.html
[4] https://www.mail-archive.com/linux-gpio@vger.kernel.org/msg01515.html

Andy Shevchenko (3):
  SFI: fix compiler warnings
  SFI: store GPIO table and export lookup function
  gpiolib: append SFI helpers for GPIO API

 drivers/gpio/Kconfig       |   4 ++
 drivers/gpio/Makefile      |   1 +
 drivers/gpio/gpiolib-sfi.c |  28 +++++++++++
 drivers/gpio/gpiolib.c     |   3 ++
 drivers/gpio/gpiolib.h     |  13 +++++
 drivers/sfi/Makefile       |   2 +-
 drivers/sfi/sfi_acpi.c     |   9 ++--
 drivers/sfi/sfi_core.c     |  10 +++-
 drivers/sfi/sfi_core.h     |   3 ++
 drivers/sfi/sfi_gpio.c     | 123 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/sfi.h        |   8 +++
 11 files changed, 197 insertions(+), 7 deletions(-)
 create mode 100644 drivers/gpio/gpiolib-sfi.c
 create mode 100644 drivers/sfi/sfi_gpio.c

-- 
1.8.4.4

[PATCH v1 1/3] SFI: fix compiler warnings

[Andy Shevchenko]

This fixes the following compiler warnings when build is done by make W=1.

drivers/sfi/sfi_acpi.c:154:5: warning: no previous prototype for ‘sfi_acpi_table_parse’ [-Wmissing-prototypes]

drivers/sfi/sfi_core.c:164:26: warning: no previous prototype for ‘sfi_map_table’ [-Wmissing-prototypes]
drivers/sfi/sfi_core.c:192:6: warning: no previous prototype for ‘sfi_unmap_table’ [-Wmissing-prototypes]

Indentation fixes are also included.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/sfi/sfi_acpi.c | 9 +++++----
 drivers/sfi/sfi_core.c | 4 ++--
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/sfi/sfi_acpi.c b/drivers/sfi/sfi_acpi.c
index f5b4ca5..1332546 100644
--- a/drivers/sfi/sfi_acpi.c
+++ b/drivers/sfi/sfi_acpi.c
@@ -63,6 +63,8 @@
 #include <acpi/acpi.h>
 
 #include <linux/sfi.h>
+#include <linux/sfi_acpi.h>
+
 #include "sfi_core.h"
 
 /*
@@ -152,7 +154,7 @@ static void sfi_acpi_put_table(struct acpi_table_header *table)
  * Find specified table in XSDT, run handler on it and return its return value
  */
 int sfi_acpi_table_parse(char *signature, char *oem_id, char *oem_table_id,
-			int(*handler)(struct acpi_table_header *))
+			 int(*handler)(struct acpi_table_header *))
 {
 	struct acpi_table_header *table = NULL;
 	struct sfi_table_key key;
@@ -175,8 +177,8 @@ int sfi_acpi_table_parse(char *signature, char *oem_id, char *oem_table_id,
 }
 
 static ssize_t sfi_acpi_table_show(struct file *filp, struct kobject *kobj,
-			       struct bin_attribute *bin_attr, char *buf,
-			       loff_t offset, size_t count)
+				   struct bin_attribute *bin_attr, char *buf,
+				   loff_t offset, size_t count)
 {
 	struct sfi_table_attr *tbl_attr =
 	    container_of(bin_attr, struct sfi_table_attr, attr);
@@ -199,7 +201,6 @@ static ssize_t sfi_acpi_table_show(struct file *filp, struct kobject *kobj,
 	return cnt;
 }
 
-
 void __init sfi_acpi_sysfs_init(void)
 {
 	u32 tbl_cnt, i;
diff --git a/drivers/sfi/sfi_core.c b/drivers/sfi/sfi_core.c
index 1e824fb..296db7a 100644
--- a/drivers/sfi/sfi_core.c
+++ b/drivers/sfi/sfi_core.c
@@ -161,7 +161,7 @@ static int sfi_verify_table(struct sfi_table_header *table)
  * Check for common case that we can re-use mapping to SYST,
  * which requires syst_pa, syst_va to be initialized.
  */
-struct sfi_table_header *sfi_map_table(u64 pa)
+static struct sfi_table_header *sfi_map_table(u64 pa)
 {
 	struct sfi_table_header *th;
 	u32 length;
@@ -189,7 +189,7 @@ struct sfi_table_header *sfi_map_table(u64 pa)
  * Undoes effect of sfi_map_table() by unmapping table
  * if it did not completely fit on same page as SYST.
  */
-void sfi_unmap_table(struct sfi_table_header *th)
+static void sfi_unmap_table(struct sfi_table_header *th)
 {
 	if (!TABLE_ON_PAGE(syst_va, th, th->len))
 		sfi_unmap_memory(th, TABLE_ON_PAGE(th, th, th->len) ?
-- 
1.8.4.4

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 2/3] SFI: store GPIO table and export lookup function

[Andy Shevchenko]

We have to provide a mechanism to retrive GPIO information from SFI. For this
we store SFI GPIO table and provide the lookup function
sfi_gpio_get_entry_by_name() that will be used later in GPIO framework.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/sfi/Makefile   |   2 +-
 drivers/sfi/sfi_core.c |   6 +++
 drivers/sfi/sfi_core.h |   3 ++
 drivers/sfi/sfi_gpio.c | 123 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/sfi.h    |   8 ++++
 5 files changed, 141 insertions(+), 1 deletion(-)
 create mode 100644 drivers/sfi/sfi_gpio.c

diff --git a/drivers/sfi/Makefile b/drivers/sfi/Makefile
index 2343732..dc011db 100644
--- a/drivers/sfi/Makefile
+++ b/drivers/sfi/Makefile
@@ -1,3 +1,3 @@
 obj-y	+= sfi_acpi.o
 obj-y	+= sfi_core.o
-
+obj-y	+= sfi_gpio.o
diff --git a/drivers/sfi/sfi_core.c b/drivers/sfi/sfi_core.c
index 296db7a..e9ff6f0 100644
--- a/drivers/sfi/sfi_core.c
+++ b/drivers/sfi/sfi_core.c
@@ -512,6 +512,12 @@ void __init sfi_init_late(void)
 	syst_va = sfi_map_memory(syst_pa, length);
 
 	sfi_acpi_init();
+
+	/*
+	 * Parsing GPIO table first, since the DEVS table will need this table
+	 * to map the pin name to the actual pin.
+	 */
+	sfi_gpio_init();
 }
 
 /*
diff --git a/drivers/sfi/sfi_core.h b/drivers/sfi/sfi_core.h
index 1d5cfe8..18c663d 100644
--- a/drivers/sfi/sfi_core.h
+++ b/drivers/sfi/sfi_core.h
@@ -79,3 +79,6 @@ struct sfi_table_header *sfi_get_table(struct sfi_table_key *key);
 extern void sfi_put_table(struct sfi_table_header *table);
 extern struct sfi_table_attr __init *sfi_sysfs_install_table(u64 pa);
 extern void __init sfi_acpi_sysfs_init(void);
+
+/* sfi_gpio.c */
+int sfi_gpio_init(void);
diff --git a/drivers/sfi/sfi_gpio.c b/drivers/sfi/sfi_gpio.c
new file mode 100644
index 0000000..677368d
--- /dev/null
+++ b/drivers/sfi/sfi_gpio.c
@@ -0,0 +1,123 @@
+/* sfi_gpio.c Simple Firmware Interface - GPIO extensions */
+
+/*
+
+  This file is provided under a dual BSD/GPLv2 license.  When using or
+  redistributing this file, you may do so under either license.
+
+  GPL LICENSE SUMMARY
+
+  Copyright(c) 2013 Intel Corporation. All rights reserved.
+
+  This program is free software; you can redistribute it and/or modify
+  it under the terms of version 2 of the GNU General Public License as
+  published by the Free Software Foundation.
+
+  This program is distributed in the hope that it will be useful, but
+  WITHOUT ANY WARRANTY; without even the implied warranty of
+  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+  General Public License for more details.
+
+  You should have received a copy of the GNU General Public License
+  along with this program; if not, write to the Free Software
+  Foundation, Inc., 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
+  The full GNU General Public License is included in this distribution
+  in the file called LICENSE.GPL.
+
+  BSD LICENSE
+
+  Copyright(c) 2013 Intel Corporation. All rights reserved.
+
+  Redistribution and use in source and binary forms, with or without
+  modification, are permitted provided that the following conditions
+  are met:
+
+    * Redistributions of source code must retain the above copyright
+      notice, this list of conditions and the following disclaimer.
+    * Redistributions in binary form must reproduce the above copyright
+      notice, this list of conditions and the following disclaimer in
+      the documentation and/or other materials provided with the
+      distribution.
+    * Neither the name of Intel Corporation nor the names of its
+      contributors may be used to endorse or promote products derived
+      from this software without specific prior written permission.
+
+  THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+  "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+  LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+  A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+  OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+  SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+  LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+  DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+  THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+  (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+  OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+*/
+
+#define KMSG_COMPONENT "SFI"
+#define pr_fmt(fmt) KMSG_COMPONENT ": " fmt
+
+#include <linux/errno.h>
+#include <linux/kernel.h>
+#include <linux/string.h>
+#include <linux/slab.h>
+#include <linux/export.h>
+#include <linux/sfi.h>
+
+#include "sfi_core.h"
+
+static struct sfi_gpio_table_entry *sfi_gpio_table;
+static int sfi_gpio_num_entry;
+
+struct sfi_gpio_table_entry *sfi_gpio_get_entry_by_name(const char *name)
+{
+	struct sfi_gpio_table_entry *pentry = sfi_gpio_table;
+	int i;
+
+	if (!pentry)
+		return NULL;
+
+	for (i = 0; i < sfi_gpio_num_entry; i++, pentry++) {
+		if (!strncmp(name, pentry->pin_name, SFI_NAME_LEN))
+			return pentry;
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(sfi_gpio_get_entry_by_name);
+
+static int __init sfi_gpio_parse(struct sfi_table_header *table)
+{
+	struct sfi_table_simple *sb;
+	struct sfi_gpio_table_entry *pentry;
+	int num, i;
+
+	if (sfi_gpio_table)
+		return -EBUSY;
+
+	sb = container_of(table, struct sfi_table_simple, header);
+
+	num = SFI_GET_NUM_ENTRIES(sb, struct sfi_gpio_table_entry);
+	pentry = (struct sfi_gpio_table_entry *)sb->pentry;
+
+	sfi_gpio_table = kmemdup(pentry, num * sizeof(*pentry), GFP_KERNEL);
+	if (!sfi_gpio_table)
+		return -ENOMEM;
+
+	sfi_gpio_num_entry = num;
+
+	pr_debug("GPIO pin info:\n");
+	for (i = 0; i < num; i++, pentry++)
+		pr_debug("GPIO [%2d] chip = %16.16s, name = %16.16s, pin=%d\n",
+			 i, pentry->controller_name, pentry->pin_name,
+			 pentry->pin_no);
+
+	return 0;
+}
+
+int __init sfi_gpio_init(void)
+{
+	return sfi_table_parse(SFI_SIG_GPIO, NULL, NULL, sfi_gpio_parse);
+}
diff --git a/include/linux/sfi.h b/include/linux/sfi.h
index d9b436f..510e74d 100644
--- a/include/linux/sfi.h
+++ b/include/linux/sfi.h
@@ -185,6 +185,8 @@ static inline void disable_sfi(void)
 	sfi_disabled = 1;
 }
 
+struct sfi_gpio_table_entry *sfi_gpio_get_entry_by_name(const char *name);
+
 #else /* !CONFIG_SFI */
 
 static inline void sfi_init(void)
@@ -204,6 +206,12 @@ static inline int sfi_table_parse(char *signature, char *oem_id,
 	return -1;
 }
 
+static inline
+struct sfi_gpio_table_entry *sfi_gpio_get_entry_by_name(const char *name)
+{
+	return NULL;
+}
+
 #endif /* !CONFIG_SFI */
 
 #endif /*_LINUX_SFI_H*/
-- 
1.8.4.4

[PATCH v1 3/3] gpiolib: append SFI helpers for GPIO API

[Andy Shevchenko]

To support some (legacy) firmwares and platforms let's make life easier for
their customers.

This patch provides a function which converts sfi_gpio_table_entry to
gpio_desc. The use of it is integrated into GPIO library to enable generic
access to the SFI GPIO resources.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/Kconfig       |  4 ++++
 drivers/gpio/Makefile      |  1 +
 drivers/gpio/gpiolib-sfi.c | 28 ++++++++++++++++++++++++++++
 drivers/gpio/gpiolib.c     |  3 +++
 drivers/gpio/gpiolib.h     | 13 +++++++++++++
 5 files changed, 49 insertions(+)
 create mode 100644 drivers/gpio/gpiolib-sfi.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index ae3682d..a12752a 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -51,6 +51,10 @@ config OF_GPIO
 	def_bool y
 	depends on OF
 
+config GPIO_SFI
+	def_bool y
+	depends on SFI
+
 config GPIO_ACPI
 	def_bool y
 	depends on ACPI
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index ee95154..5373e3a 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -5,6 +5,7 @@ ccflags-$(CONFIG_DEBUG_GPIO)	+= -DDEBUG
 obj-$(CONFIG_GPIO_DEVRES)	+= devres.o
 obj-$(CONFIG_GPIOLIB)		+= gpiolib.o
 obj-$(CONFIG_OF_GPIO)		+= gpiolib-of.o
+obj-$(CONFIG_GPIO_SFI)		+= gpiolib-sfi.o
 obj-$(CONFIG_GPIO_ACPI)		+= gpiolib-acpi.o
 
 # Device drivers. Generally keep list sorted alphabetically
diff --git a/drivers/gpio/gpiolib-sfi.c b/drivers/gpio/gpiolib-sfi.c
new file mode 100644
index 0000000..c804314
--- /dev/null
+++ b/drivers/gpio/gpiolib-sfi.c
@@ -0,0 +1,28 @@
+/*
+ * Simple Firmware Interface (SFI) helpers for GPIO API
+ *
+ * Copyright (C) 2013, Intel Corporation
+ * Author: Andy Shevchenko <andriy.shevchenko@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/gpio/consumer.h>
+#include <linux/sfi.h>
+#include <linux/errno.h>
+#include <linux/err.h>
+
+#include "gpiolib.h"
+
+struct gpio_desc *sfi_get_gpiod_by_name(const char *name)
+{
+	struct sfi_gpio_table_entry *pentry;
+
+	pentry = sfi_gpio_get_entry_by_name(name);
+	if (!pentry)
+		return ERR_PTR(-ENODEV);
+
+	return gpio_to_desc(pentry->pin_no);
+}
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index bad400c..789ae1c 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2451,6 +2451,9 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
 	} else if (IS_ENABLED(CONFIG_ACPI) && dev && ACPI_HANDLE(dev)) {
 		dev_dbg(dev, "using ACPI for GPIO lookup\n");
 		desc = acpi_find_gpio(dev, con_id, idx, &flags);
+	} else if (IS_ENABLED(CONFIG_SFI)) {
+		dev_dbg(dev, "using SFI for GPIO lookup\n");
+		desc = sfi_get_gpiod_by_name(con_id);
 	}
 
 	/*
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 82be586..ca0615a 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -43,4 +43,17 @@ acpi_get_gpiod_by_index(struct device *dev, int index,
 }
 #endif
 
+#ifdef CONFIG_GPIO_SFI
+
+struct gpio_desc *sfi_get_gpiod_by_name(const char *name);
+
+#else /* !CONFIG_GPIO_SFI */
+
+static inline struct gpio_desc *sfi_get_gpiod_by_name(const char *name)
+{
+	return ERR_PTR(-ENODEV);
+}
+
+#endif /* !CONFIG_GPIO_SFI */
+
 #endif /* GPIOLIB_H */
-- 
1.8.4.4

Re: [PATCH v1 0/3]

[David Cohen]

Hi Andy,

On 12/05/2013 08:36 AM, Andy Shevchenko wrote:
> Here is a third [1] version of the SFI GPIO helpers. This series contains SFI

Your patch set subject says it's the first version :)

> GPIO helpers along with the update of GPIO library to lookup for SFI resources.
>
> It has been tested on Medfield device on top of v3.13-rc2 + Alexandre's GPIO
> descriptor API update [2], Mika's ACPI gpio patches [3] and my GPIO library
> clean up [4].

Good test case. I'm able to extend it to Merrifield.

Br, David

>
> [1] https://lkml.org/lkml/2013/6/5/316
> [2] http://www.spinics.net/lists/kernel/msg1645907.html
> [3] http://www.spinics.net/lists/linux-acpi/msg47572.html
> [4] https://www.mail-archive.com/linux-gpio@vger.kernel.org/msg01515.html
>
> Andy Shevchenko (3):
>   SFI: fix compiler warnings
>   SFI: store GPIO table and export lookup function
>   gpiolib: append SFI helpers for GPIO API
>
>  drivers/gpio/Kconfig       |   4 ++
>  drivers/gpio/Makefile      |   1 +
>  drivers/gpio/gpiolib-sfi.c |  28 +++++++++++
>  drivers/gpio/gpiolib.c     |   3 ++
>  drivers/gpio/gpiolib.h     |  13 +++++
>  drivers/sfi/Makefile       |   2 +-
>  drivers/sfi/sfi_acpi.c     |   9 ++--
>  drivers/sfi/sfi_core.c     |  10 +++-
>  drivers/sfi/sfi_core.h     |   3 ++
>  drivers/sfi/sfi_gpio.c     | 123 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/sfi.h        |   8 +++
>  11 files changed, 197 insertions(+), 7 deletions(-)
>  create mode 100644 drivers/gpio/gpiolib-sfi.c
>  create mode 100644 drivers/sfi/sfi_gpio.c
>

Re: [PATCH v1 1/3] SFI: fix compiler warnings

[David Cohen]

On 12/05/2013 08:36 AM, Andy Shevchenko wrote:
> This fixes the following compiler warnings when build is done by make W=1.
>
> drivers/sfi/sfi_acpi.c:154:5: warning: no previous prototype for ‘sfi_acpi_table_parse’ [-Wmissing-prototypes]
>
> drivers/sfi/sfi_core.c:164:26: warning: no previous prototype for ‘sfi_map_table’ [-Wmissing-prototypes]
> drivers/sfi/sfi_core.c:192:6: warning: no previous prototype for ‘sfi_unmap_table’ [-Wmissing-prototypes]
>
> Indentation fixes are also included.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

IMO this patch could be accepted regardless of this series feedback.
It's a simple fix without much dependence with the other patches.

Br, David
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 2/3] SFI: store GPIO table and export lookup function

[David Cohen]

Hi Andy,

On 12/05/2013 08:36 AM, Andy Shevchenko wrote:
> We have to provide a mechanism to retrive GPIO information from SFI. For this
> we store SFI GPIO table and provide the lookup function
> sfi_gpio_get_entry_by_name() that will be used later in GPIO framework.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/sfi/Makefile   |   2 +-
>  drivers/sfi/sfi_core.c |   6 +++
>  drivers/sfi/sfi_core.h |   3 ++
>  drivers/sfi/sfi_gpio.c | 123 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/sfi.h    |   8 ++++
>  5 files changed, 141 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/sfi/sfi_gpio.c
>
> diff --git a/drivers/sfi/Makefile b/drivers/sfi/Makefile
> index 2343732..dc011db 100644
> --- a/drivers/sfi/Makefile
> +++ b/drivers/sfi/Makefile
> @@ -1,3 +1,3 @@
>  obj-y	+= sfi_acpi.o
>  obj-y	+= sfi_core.o
> -
> +obj-y	+= sfi_gpio.o
> diff --git a/drivers/sfi/sfi_core.c b/drivers/sfi/sfi_core.c
> index 296db7a..e9ff6f0 100644
> --- a/drivers/sfi/sfi_core.c
> +++ b/drivers/sfi/sfi_core.c
> @@ -512,6 +512,12 @@ void __init sfi_init_late(void)
>  	syst_va = sfi_map_memory(syst_pa, length);
>  
>  	sfi_acpi_init();
> +
> +	/*
> +	 * Parsing GPIO table first, since the DEVS table will need this table
> +	 * to map the pin name to the actual pin.
> +	 */
> +	sfi_gpio_init();
>  }
>  
>  /*
> diff --git a/drivers/sfi/sfi_core.h b/drivers/sfi/sfi_core.h
> index 1d5cfe8..18c663d 100644
> --- a/drivers/sfi/sfi_core.h
> +++ b/drivers/sfi/sfi_core.h
> @@ -79,3 +79,6 @@ struct sfi_table_header *sfi_get_table(struct sfi_table_key *key);
>  extern void sfi_put_table(struct sfi_table_header *table);
>  extern struct sfi_table_attr __init *sfi_sysfs_install_table(u64 pa);
>  extern void __init sfi_acpi_sysfs_init(void);
> +
> +/* sfi_gpio.c */
> +int sfi_gpio_init(void);
> diff --git a/drivers/sfi/sfi_gpio.c b/drivers/sfi/sfi_gpio.c
> new file mode 100644
> index 0000000..677368d
> --- /dev/null
> +++ b/drivers/sfi/sfi_gpio.c
> @@ -0,0 +1,123 @@
> +/* sfi_gpio.c Simple Firmware Interface - GPIO extensions */
> +
> +/*
> +
> +  This file is provided under a dual BSD/GPLv2 license.  When using or
> +  redistributing this file, you may do so under either license.
> +
> +  GPL LICENSE SUMMARY
> +
> +  Copyright(c) 2013 Intel Corporation. All rights reserved.
> +
> +  This program is free software; you can redistribute it and/or modify
> +  it under the terms of version 2 of the GNU General Public License as
> +  published by the Free Software Foundation.
> +
> +  This program is distributed in the hope that it will be useful, but
> +  WITHOUT ANY WARRANTY; without even the implied warranty of
> +  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +  General Public License for more details.
> +
> +  You should have received a copy of the GNU General Public License
> +  along with this program; if not, write to the Free Software
> +  Foundation, Inc., 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
> +  The full GNU General Public License is included in this distribution
> +  in the file called LICENSE.GPL.
> +
> +  BSD LICENSE
> +
> +  Copyright(c) 2013 Intel Corporation. All rights reserved.
> +
> +  Redistribution and use in source and binary forms, with or without
> +  modification, are permitted provided that the following conditions
> +  are met:
> +
> +    * Redistributions of source code must retain the above copyright
> +      notice, this list of conditions and the following disclaimer.
> +    * Redistributions in binary form must reproduce the above copyright
> +      notice, this list of conditions and the following disclaimer in
> +      the documentation and/or other materials provided with the
> +      distribution.
> +    * Neither the name of Intel Corporation nor the names of its
> +      contributors may be used to endorse or promote products derived
> +      from this software without specific prior written permission.
> +
> +  THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> +  "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> +  LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> +  A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> +  OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> +  SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> +  LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> +  DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> +  THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> +  (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> +  OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +
> +*/
> +
> +#define KMSG_COMPONENT "SFI"
> +#define pr_fmt(fmt) KMSG_COMPONENT ": " fmt
> +
> +#include <linux/errno.h>
> +#include <linux/kernel.h>
> +#include <linux/string.h>
> +#include <linux/slab.h>
> +#include <linux/export.h>
> +#include <linux/sfi.h>
> +
> +#include "sfi_core.h"
> +
> +static struct sfi_gpio_table_entry *sfi_gpio_table;
> +static int sfi_gpio_num_entry;
> +
> +struct sfi_gpio_table_entry *sfi_gpio_get_entry_by_name(const char *name)
> +{
> +	struct sfi_gpio_table_entry *pentry = sfi_gpio_table;
> +	int i;
> +
> +	if (!pentry)
> +		return NULL;

If this function is called prior to sfi_gpio_table initialization, it
will have
same result as if the gpio name doesn't exist.
For development sanity, how about return ERR_PTR(-EBUSY) or
ERR_PTR(-EAGAIN) if it's too early?

> +
> +	for (i = 0; i < sfi_gpio_num_entry; i++, pentry++) {
> +		if (!strncmp(name, pentry->pin_name, SFI_NAME_LEN))
> +			return pentry;
> +	}
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(sfi_gpio_get_entry_by_name);
> +
> +static int __init sfi_gpio_parse(struct sfi_table_header *table)
> +{
> +	struct sfi_table_simple *sb;
> +	struct sfi_gpio_table_entry *pentry;
> +	int num, i;
> +
> +	if (sfi_gpio_table)
> +		return -EBUSY;

It can't ever happen. This is a static function called once in this file.
IMO kernel doesn't need such kind of tests :)

> +
> +	sb = container_of(table, struct sfi_table_simple, header);
> +
> +	num = SFI_GET_NUM_ENTRIES(sb, struct sfi_gpio_table_entry);
> +	pentry = (struct sfi_gpio_table_entry *)sb->pentry;
> +
> +	sfi_gpio_table = kmemdup(pentry, num * sizeof(*pentry), GFP_KERNEL);
> +	if (!sfi_gpio_table)
> +		return -ENOMEM;
> +
> +	sfi_gpio_num_entry = num;
> +
> +	pr_debug("GPIO pin info:\n");
> +	for (i = 0; i < num; i++, pentry++)
> +		pr_debug("GPIO [%2d] chip = %16.16s, name = %16.16s, pin=%d\n",
> +			 i, pentry->controller_name, pentry->pin_name,
> +			 pentry->pin_no);
> +
> +	return 0;
> +}
> +
> +int __init sfi_gpio_init(void)
> +{
> +	return sfi_table_parse(SFI_SIG_GPIO, NULL, NULL, sfi_gpio_parse);
> +}
> diff --git a/include/linux/sfi.h b/include/linux/sfi.h
> index d9b436f..510e74d 100644
> --- a/include/linux/sfi.h
> +++ b/include/linux/sfi.h
> @@ -185,6 +185,8 @@ static inline void disable_sfi(void)
>  	sfi_disabled = 1;
>  }
>  
> +struct sfi_gpio_table_entry *sfi_gpio_get_entry_by_name(const char *name);
> +
>  #else /* !CONFIG_SFI */
>  
>  static inline void sfi_init(void)
> @@ -204,6 +206,12 @@ static inline int sfi_table_parse(char *signature, char *oem_id,
>  	return -1;
>  }
>  
> +static inline
> +struct sfi_gpio_table_entry *sfi_gpio_get_entry_by_name(const char *name)
> +{
> +	return NULL;

I'd suggest something more meaningful like ERR_PTR(-ENODEV).
Again, for development sanity.

Br, David Cohen

> +}
> +
>  #endif /* !CONFIG_SFI */
>  
>  #endif /*_LINUX_SFI_H*/

Re: [PATCH v1 3/3] gpiolib: append SFI helpers for GPIO API

[David Cohen]

On 12/05/2013 08:36 AM, Andy Shevchenko wrote:
> To support some (legacy) firmwares and platforms let's make life easier for
> their customers.
>
> This patch provides a function which converts sfi_gpio_table_entry to
> gpio_desc. The use of it is integrated into GPIO library to enable generic
> access to the SFI GPIO resources.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/gpio/Kconfig       |  4 ++++
>  drivers/gpio/Makefile      |  1 +
>  drivers/gpio/gpiolib-sfi.c | 28 ++++++++++++++++++++++++++++
>  drivers/gpio/gpiolib.c     |  3 +++
>  drivers/gpio/gpiolib.h     | 13 +++++++++++++
>  5 files changed, 49 insertions(+)
>  create mode 100644 drivers/gpio/gpiolib-sfi.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index ae3682d..a12752a 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -51,6 +51,10 @@ config OF_GPIO
>  	def_bool y
>  	depends on OF
>  
> +config GPIO_SFI
> +	def_bool y
> +	depends on SFI
> +
>  config GPIO_ACPI
>  	def_bool y
>  	depends on ACPI
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index ee95154..5373e3a 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -5,6 +5,7 @@ ccflags-$(CONFIG_DEBUG_GPIO)	+= -DDEBUG
>  obj-$(CONFIG_GPIO_DEVRES)	+= devres.o
>  obj-$(CONFIG_GPIOLIB)		+= gpiolib.o
>  obj-$(CONFIG_OF_GPIO)		+= gpiolib-of.o
> +obj-$(CONFIG_GPIO_SFI)		+= gpiolib-sfi.o
>  obj-$(CONFIG_GPIO_ACPI)		+= gpiolib-acpi.o
>  
>  # Device drivers. Generally keep list sorted alphabetically
> diff --git a/drivers/gpio/gpiolib-sfi.c b/drivers/gpio/gpiolib-sfi.c
> new file mode 100644
> index 0000000..c804314
> --- /dev/null
> +++ b/drivers/gpio/gpiolib-sfi.c
> @@ -0,0 +1,28 @@
> +/*
> + * Simple Firmware Interface (SFI) helpers for GPIO API
> + *
> + * Copyright (C) 2013, Intel Corporation
> + * Author: Andy Shevchenko <andriy.shevchenko@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/gpio/consumer.h>
> +#include <linux/sfi.h>
> +#include <linux/errno.h>
> +#include <linux/err.h>
> +
> +#include "gpiolib.h"
> +
> +struct gpio_desc *sfi_get_gpiod_by_name(const char *name)
> +{
> +	struct sfi_gpio_table_entry *pentry;
> +
> +	pentry = sfi_gpio_get_entry_by_name(name);
> +	if (!pentry)
> +		return ERR_PTR(-ENODEV);

With my comments on patch 2/3, I'd suggest here:
if (!pentry)
     return ERR_PTR(-EINVAL);
if (IS_ERR(pentry))
     return pentry;

!pentry == could not find
IS_ERR(pentry) == something else went wrong

Br, David


> +
> +	return gpio_to_desc(pentry->pin_no);
> +}
>

Re: [PATCH v1 2/3] SFI: store GPIO table and export lookup function

[Alex Courbot]

On 12/06/2013 01:36 AM, Andy Shevchenko wrote:
> We have to provide a mechanism to retrive GPIO information from SFI. For this
> we store SFI GPIO table and provide the lookup function
> sfi_gpio_get_entry_by_name() that will be used later in GPIO framework.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>   drivers/sfi/Makefile   |   2 +-
>   drivers/sfi/sfi_core.c |   6 +++
>   drivers/sfi/sfi_core.h |   3 ++
>   drivers/sfi/sfi_gpio.c | 123 +++++++++++++++++++++++++++++++++++++++++++++++++
>   include/linux/sfi.h    |   8 ++++
>   5 files changed, 141 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/sfi/sfi_gpio.c
>
> diff --git a/drivers/sfi/Makefile b/drivers/sfi/Makefile
> index 2343732..dc011db 100644
> --- a/drivers/sfi/Makefile
> +++ b/drivers/sfi/Makefile
> @@ -1,3 +1,3 @@
>   obj-y	+= sfi_acpi.o
>   obj-y	+= sfi_core.o
> -
> +obj-y	+= sfi_gpio.o
> diff --git a/drivers/sfi/sfi_core.c b/drivers/sfi/sfi_core.c
> index 296db7a..e9ff6f0 100644
> --- a/drivers/sfi/sfi_core.c
> +++ b/drivers/sfi/sfi_core.c
> @@ -512,6 +512,12 @@ void __init sfi_init_late(void)
>   	syst_va = sfi_map_memory(syst_pa, length);
>
>   	sfi_acpi_init();
> +
> +	/*
> +	 * Parsing GPIO table first, since the DEVS table will need this table
> +	 * to map the pin name to the actual pin.
> +	 */
> +	sfi_gpio_init();
>   }
>
>   /*
> diff --git a/drivers/sfi/sfi_core.h b/drivers/sfi/sfi_core.h
> index 1d5cfe8..18c663d 100644
> --- a/drivers/sfi/sfi_core.h
> +++ b/drivers/sfi/sfi_core.h
> @@ -79,3 +79,6 @@ struct sfi_table_header *sfi_get_table(struct sfi_table_key *key);
>   extern void sfi_put_table(struct sfi_table_header *table);
>   extern struct sfi_table_attr __init *sfi_sysfs_install_table(u64 pa);
>   extern void __init sfi_acpi_sysfs_init(void);
> +
> +/* sfi_gpio.c */
> +int sfi_gpio_init(void);
> diff --git a/drivers/sfi/sfi_gpio.c b/drivers/sfi/sfi_gpio.c
> new file mode 100644
> index 0000000..677368d
> --- /dev/null
> +++ b/drivers/sfi/sfi_gpio.c
> @@ -0,0 +1,123 @@
> +/* sfi_gpio.c Simple Firmware Interface - GPIO extensions */
> +
> +/*
> +
> +  This file is provided under a dual BSD/GPLv2 license.  When using or
> +  redistributing this file, you may do so under either license.
> +
> +  GPL LICENSE SUMMARY
> +
> +  Copyright(c) 2013 Intel Corporation. All rights reserved.
> +
> +  This program is free software; you can redistribute it and/or modify
> +  it under the terms of version 2 of the GNU General Public License as
> +  published by the Free Software Foundation.
> +
> +  This program is distributed in the hope that it will be useful, but
> +  WITHOUT ANY WARRANTY; without even the implied warranty of
> +  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +  General Public License for more details.
> +
> +  You should have received a copy of the GNU General Public License
> +  along with this program; if not, write to the Free Software
> +  Foundation, Inc., 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
> +  The full GNU General Public License is included in this distribution
> +  in the file called LICENSE.GPL.

As I've been told many times in the past, this last paragraph should not 
be included unless you are willing to update all your patches that 
include it should the FSF move to another address. ;)

Otherwise I'm not familiar with SFI, but this looks ok to me.

Re: [PATCH v1 3/3] gpiolib: append SFI helpers for GPIO API

[Alex Courbot]

On 12/06/2013 01:36 AM, Andy Shevchenko wrote:
> To support some (legacy) firmwares and platforms let's make life easier for
> their customers.
>
> This patch provides a function which converts sfi_gpio_table_entry to
> gpio_desc. The use of it is integrated into GPIO library to enable generic
> access to the SFI GPIO resources.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>   drivers/gpio/Kconfig       |  4 ++++
>   drivers/gpio/Makefile      |  1 +
>   drivers/gpio/gpiolib-sfi.c | 28 ++++++++++++++++++++++++++++
>   drivers/gpio/gpiolib.c     |  3 +++
>   drivers/gpio/gpiolib.h     | 13 +++++++++++++
>   5 files changed, 49 insertions(+)
>   create mode 100644 drivers/gpio/gpiolib-sfi.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index ae3682d..a12752a 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -51,6 +51,10 @@ config OF_GPIO
>   	def_bool y
>   	depends on OF
>
> +config GPIO_SFI
> +	def_bool y
> +	depends on SFI
> +
>   config GPIO_ACPI
>   	def_bool y
>   	depends on ACPI
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index ee95154..5373e3a 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -5,6 +5,7 @@ ccflags-$(CONFIG_DEBUG_GPIO)	+= -DDEBUG
>   obj-$(CONFIG_GPIO_DEVRES)	+= devres.o
>   obj-$(CONFIG_GPIOLIB)		+= gpiolib.o
>   obj-$(CONFIG_OF_GPIO)		+= gpiolib-of.o
> +obj-$(CONFIG_GPIO_SFI)		+= gpiolib-sfi.o
>   obj-$(CONFIG_GPIO_ACPI)		+= gpiolib-acpi.o
>
>   # Device drivers. Generally keep list sorted alphabetically
> diff --git a/drivers/gpio/gpiolib-sfi.c b/drivers/gpio/gpiolib-sfi.c
> new file mode 100644
> index 0000000..c804314
> --- /dev/null
> +++ b/drivers/gpio/gpiolib-sfi.c
> @@ -0,0 +1,28 @@
> +/*
> + * Simple Firmware Interface (SFI) helpers for GPIO API
> + *
> + * Copyright (C) 2013, Intel Corporation
> + * Author: Andy Shevchenko <andriy.shevchenko@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/gpio/consumer.h>
> +#include <linux/sfi.h>
> +#include <linux/errno.h>
> +#include <linux/err.h>
> +
> +#include "gpiolib.h"
> +
> +struct gpio_desc *sfi_get_gpiod_by_name(const char *name)
> +{
> +	struct sfi_gpio_table_entry *pentry;
> +
> +	pentry = sfi_gpio_get_entry_by_name(name);
> +	if (!pentry)
> +		return ERR_PTR(-ENODEV);
> +
> +	return gpio_to_desc(pentry->pin_no);
> +}
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index bad400c..789ae1c 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2451,6 +2451,9 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
>   	} else if (IS_ENABLED(CONFIG_ACPI) && dev && ACPI_HANDLE(dev)) {
>   		dev_dbg(dev, "using ACPI for GPIO lookup\n");
>   		desc = acpi_find_gpio(dev, con_id, idx, &flags);
> +	} else if (IS_ENABLED(CONFIG_SFI)) {
> +		dev_dbg(dev, "using SFI for GPIO lookup\n");
> +		desc = sfi_get_gpiod_by_name(con_id);

Your lookup function is ignoring the dev argument. Are SFI GPIOs always 
supposed to be system-global? In this case, your if condition should 
likely be

	} else if (IS_ENABLED(CONFIG_SFI) && !dev) {

So that a global SFI GPIO does not get mistakenly assigned to a device 
that has, say, a more suited platform mapping on the same con_id.

Re: [PATCH v1 1/3] SFI: fix compiler warnings

[Andy Shevchenko]

On Thu, 2013-12-05 at 14:48 -0800, David Cohen wrote:
> On 12/05/2013 08:36 AM, Andy Shevchenko wrote:
> > This fixes the following compiler warnings when build is done by make W=1.
> >
> > drivers/sfi/sfi_acpi.c:154:5: warning: no previous prototype for ‘sfi_acpi_table_parse’ [-Wmissing-prototypes]
> >
> > drivers/sfi/sfi_core.c:164:26: warning: no previous prototype for ‘sfi_map_table’ [-Wmissing-prototypes]
> > drivers/sfi/sfi_core.c:192:6: warning: no previous prototype for ‘sfi_unmap_table’ [-Wmissing-prototypes]
> >
> > Indentation fixes are also included.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> IMO this patch could be accepted regardless of this series feedback.
> It's a simple fix without much dependence with the other patches.

Yes, but it was ignored all times. Do you know a maintainer who can
upstream this one?


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 0/3]

[Andy Shevchenko]

On Thu, 2013-12-05 at 14:46 -0800, David Cohen wrote:
> Hi Andy,
> 
> On 12/05/2013 08:36 AM, Andy Shevchenko wrote:
> > Here is a third [1] version of the SFI GPIO helpers. This series contains SFI
> 
> Your patch set subject says it's the first version :)

Will fix in next iteration.

> 
> > GPIO helpers along with the update of GPIO library to lookup for SFI resources.
> >
> > It has been tested on Medfield device on top of v3.13-rc2 + Alexandre's GPIO
> > descriptor API update [2], Mika's ACPI gpio patches [3] and my GPIO library
> > clean up [4].
> 
> Good test case. I'm able to extend it to Merrifield.

Be informed, that no driver is modified by this patch series. To test
you have to update one or more drivers. I've tested against gpio_keys,
but I'm going to make that solution upstreamable (it requires to update
gpio_keys driver itself first).

> 
> Br, David
> 
> >
> > [1] https://lkml.org/lkml/2013/6/5/316
> > [2] http://www.spinics.net/lists/kernel/msg1645907.html
> > [3] http://www.spinics.net/lists/linux-acpi/msg47572.html
> > [4] https://www.mail-archive.com/linux-gpio@vger.kernel.org/msg01515.html
> >
> > Andy Shevchenko (3):
> >   SFI: fix compiler warnings
> >   SFI: store GPIO table and export lookup function
> >   gpiolib: append SFI helpers for GPIO API
> >
> >  drivers/gpio/Kconfig       |   4 ++
> >  drivers/gpio/Makefile      |   1 +
> >  drivers/gpio/gpiolib-sfi.c |  28 +++++++++++
> >  drivers/gpio/gpiolib.c     |   3 ++
> >  drivers/gpio/gpiolib.h     |  13 +++++
> >  drivers/sfi/Makefile       |   2 +-
> >  drivers/sfi/sfi_acpi.c     |   9 ++--
> >  drivers/sfi/sfi_core.c     |  10 +++-
> >  drivers/sfi/sfi_core.h     |   3 ++
> >  drivers/sfi/sfi_gpio.c     | 123 +++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/sfi.h        |   8 +++
> >  11 files changed, 197 insertions(+), 7 deletions(-)
> >  create mode 100644 drivers/gpio/gpiolib-sfi.c
> >  create mode 100644 drivers/sfi/sfi_gpio.c
> >
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1 2/3] SFI: store GPIO table and export lookup function

[Andy Shevchenko]

On Fri, 2013-12-06 at 10:51 +0900, Alex Courbot wrote:
> On 12/06/2013 01:36 AM, Andy Shevchenko wrote:
> > We have to provide a mechanism to retrive GPIO information from SFI. For this
> > we store SFI GPIO table and provide the lookup function
> > sfi_gpio_get_entry_by_name() that will be used later in GPIO framework.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >   drivers/sfi/Makefile   |   2 +-
> >   drivers/sfi/sfi_core.c |   6 +++
> >   drivers/sfi/sfi_core.h |   3 ++
> >   drivers/sfi/sfi_gpio.c | 123 +++++++++++++++++++++++++++++++++++++++++++++++++
> >   include/linux/sfi.h    |   8 ++++
> >   5 files changed, 141 insertions(+), 1 deletion(-)
> >   create mode 100644 drivers/sfi/sfi_gpio.c
> >
> > diff --git a/drivers/sfi/Makefile b/drivers/sfi/Makefile
> > index 2343732..dc011db 100644
> > --- a/drivers/sfi/Makefile
> > +++ b/drivers/sfi/Makefile
> > @@ -1,3 +1,3 @@
> >   obj-y	+= sfi_acpi.o
> >   obj-y	+= sfi_core.o
> > -
> > +obj-y	+= sfi_gpio.o
> > diff --git a/drivers/sfi/sfi_core.c b/drivers/sfi/sfi_core.c
> > index 296db7a..e9ff6f0 100644
> > --- a/drivers/sfi/sfi_core.c
> > +++ b/drivers/sfi/sfi_core.c
> > @@ -512,6 +512,12 @@ void __init sfi_init_late(void)
> >   	syst_va = sfi_map_memory(syst_pa, length);
> >
> >   	sfi_acpi_init();
> > +
> > +	/*
> > +	 * Parsing GPIO table first, since the DEVS table will need this table
> > +	 * to map the pin name to the actual pin.
> > +	 */
> > +	sfi_gpio_init();
> >   }
> >
> >   /*
> > diff --git a/drivers/sfi/sfi_core.h b/drivers/sfi/sfi_core.h
> > index 1d5cfe8..18c663d 100644
> > --- a/drivers/sfi/sfi_core.h
> > +++ b/drivers/sfi/sfi_core.h
> > @@ -79,3 +79,6 @@ struct sfi_table_header *sfi_get_table(struct sfi_table_key *key);
> >   extern void sfi_put_table(struct sfi_table_header *table);
> >   extern struct sfi_table_attr __init *sfi_sysfs_install_table(u64 pa);
> >   extern void __init sfi_acpi_sysfs_init(void);
> > +
> > +/* sfi_gpio.c */
> > +int sfi_gpio_init(void);
> > diff --git a/drivers/sfi/sfi_gpio.c b/drivers/sfi/sfi_gpio.c
> > new file mode 100644
> > index 0000000..677368d
> > --- /dev/null
> > +++ b/drivers/sfi/sfi_gpio.c
> > @@ -0,0 +1,123 @@
> > +/* sfi_gpio.c Simple Firmware Interface - GPIO extensions */
> > +
> > +/*
> > +
> > +  This file is provided under a dual BSD/GPLv2 license.  When using or
> > +  redistributing this file, you may do so under either license.
> > +
> > +  GPL LICENSE SUMMARY
> > +
> > +  Copyright(c) 2013 Intel Corporation. All rights reserved.
> > +
> > +  This program is free software; you can redistribute it and/or modify
> > +  it under the terms of version 2 of the GNU General Public License as
> > +  published by the Free Software Foundation.
> > +
> > +  This program is distributed in the hope that it will be useful, but
> > +  WITHOUT ANY WARRANTY; without even the implied warranty of
> > +  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > +  General Public License for more details.
> > +
> > +  You should have received a copy of the GNU General Public License
> > +  along with this program; if not, write to the Free Software
> > +  Foundation, Inc., 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
> > +  The full GNU General Public License is included in this distribution
> > +  in the file called LICENSE.GPL.
> 
> As I've been told many times in the past, this last paragraph should not 
> be included unless you are willing to update all your patches that 
> include it should the FSF move to another address. ;)
> 
> Otherwise I'm not familiar with SFI, but this looks ok to me.

I have just copied the top from other file in that folder (to keep
licensing the same). So, it probably requires to be updated in all
related files at once.

Meanwhile I'll prepare that patch and reduce this one.


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1 2/3] SFI: store GPIO table and export lookup function

[Andy Shevchenko]

On Thu, 2013-12-05 at 15:07 -0800, David Cohen wrote:
> Hi Andy,
> 
> On 12/05/2013 08:36 AM, Andy Shevchenko wrote:
> > We have to provide a mechanism to retrive GPIO information from SFI. For this
> > we store SFI GPIO table and provide the lookup function
> > sfi_gpio_get_entry_by_name() that will be used later in GPIO framework.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  drivers/sfi/Makefile   |   2 +-
> >  drivers/sfi/sfi_core.c |   6 +++
> >  drivers/sfi/sfi_core.h |   3 ++
> >  drivers/sfi/sfi_gpio.c | 123 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/sfi.h    |   8 ++++
> >  5 files changed, 141 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/sfi/sfi_gpio.c
> >
> > diff --git a/drivers/sfi/Makefile b/drivers/sfi/Makefile
> > index 2343732..dc011db 100644
> > --- a/drivers/sfi/Makefile
> > +++ b/drivers/sfi/Makefile
> > @@ -1,3 +1,3 @@
> >  obj-y	+= sfi_acpi.o
> >  obj-y	+= sfi_core.o
> > -
> > +obj-y	+= sfi_gpio.o
> > diff --git a/drivers/sfi/sfi_core.c b/drivers/sfi/sfi_core.c
> > index 296db7a..e9ff6f0 100644
> > --- a/drivers/sfi/sfi_core.c
> > +++ b/drivers/sfi/sfi_core.c
> > @@ -512,6 +512,12 @@ void __init sfi_init_late(void)
> >  	syst_va = sfi_map_memory(syst_pa, length);
> >  
> >  	sfi_acpi_init();
> > +
> > +	/*
> > +	 * Parsing GPIO table first, since the DEVS table will need this table
> > +	 * to map the pin name to the actual pin.
> > +	 */
> > +	sfi_gpio_init();
> >  }
> >  
> >  /*
> > diff --git a/drivers/sfi/sfi_core.h b/drivers/sfi/sfi_core.h
> > index 1d5cfe8..18c663d 100644
> > --- a/drivers/sfi/sfi_core.h
> > +++ b/drivers/sfi/sfi_core.h
> > @@ -79,3 +79,6 @@ struct sfi_table_header *sfi_get_table(struct sfi_table_key *key);
> >  extern void sfi_put_table(struct sfi_table_header *table);
> >  extern struct sfi_table_attr __init *sfi_sysfs_install_table(u64 pa);
> >  extern void __init sfi_acpi_sysfs_init(void);
> > +
> > +/* sfi_gpio.c */
> > +int sfi_gpio_init(void);
> > diff --git a/drivers/sfi/sfi_gpio.c b/drivers/sfi/sfi_gpio.c
> > new file mode 100644
> > index 0000000..677368d
> > --- /dev/null
> > +++ b/drivers/sfi/sfi_gpio.c
> > @@ -0,0 +1,123 @@
> > +/* sfi_gpio.c Simple Firmware Interface - GPIO extensions */
> > +
> > +/*
> > +
> > +  This file is provided under a dual BSD/GPLv2 license.  When using or
> > +  redistributing this file, you may do so under either license.
> > +
> > +  GPL LICENSE SUMMARY
> > +
> > +  Copyright(c) 2013 Intel Corporation. All rights reserved.
> > +
> > +  This program is free software; you can redistribute it and/or modify
> > +  it under the terms of version 2 of the GNU General Public License as
> > +  published by the Free Software Foundation.
> > +
> > +  This program is distributed in the hope that it will be useful, but
> > +  WITHOUT ANY WARRANTY; without even the implied warranty of
> > +  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > +  General Public License for more details.
> > +
> > +  You should have received a copy of the GNU General Public License
> > +  along with this program; if not, write to the Free Software
> > +  Foundation, Inc., 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
> > +  The full GNU General Public License is included in this distribution
> > +  in the file called LICENSE.GPL.
> > +
> > +  BSD LICENSE
> > +
> > +  Copyright(c) 2013 Intel Corporation. All rights reserved.
> > +
> > +  Redistribution and use in source and binary forms, with or without
> > +  modification, are permitted provided that the following conditions
> > +  are met:
> > +
> > +    * Redistributions of source code must retain the above copyright
> > +      notice, this list of conditions and the following disclaimer.
> > +    * Redistributions in binary form must reproduce the above copyright
> > +      notice, this list of conditions and the following disclaimer in
> > +      the documentation and/or other materials provided with the
> > +      distribution.
> > +    * Neither the name of Intel Corporation nor the names of its
> > +      contributors may be used to endorse or promote products derived
> > +      from this software without specific prior written permission.
> > +
> > +  THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> > +  "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> > +  LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> > +  A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> > +  OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> > +  SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> > +  LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> > +  DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> > +  THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> > +  (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> > +  OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> > +
> > +*/
> > +
> > +#define KMSG_COMPONENT "SFI"
> > +#define pr_fmt(fmt) KMSG_COMPONENT ": " fmt
> > +
> > +#include <linux/errno.h>
> > +#include <linux/kernel.h>
> > +#include <linux/string.h>
> > +#include <linux/slab.h>
> > +#include <linux/export.h>
> > +#include <linux/sfi.h>
> > +
> > +#include "sfi_core.h"
> > +
> > +static struct sfi_gpio_table_entry *sfi_gpio_table;
> > +static int sfi_gpio_num_entry;
> > +
> > +struct sfi_gpio_table_entry *sfi_gpio_get_entry_by_name(const char *name)
> > +{
> > +	struct sfi_gpio_table_entry *pentry = sfi_gpio_table;
> > +	int i;
> > +
> > +	if (!pentry)
> > +		return NULL;
> 
> If this function is called prior to sfi_gpio_table initialization, it
> will have
> same result as if the gpio name doesn't exist.
> For development sanity, how about return ERR_PTR(-EBUSY) or
> ERR_PTR(-EAGAIN) if it's too early?

Okay, I switch to error pointer.

> 
> > +
> > +	for (i = 0; i < sfi_gpio_num_entry; i++, pentry++) {
> > +		if (!strncmp(name, pentry->pin_name, SFI_NAME_LEN))
> > +			return pentry;
> > +	}
> > +
> > +	return NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(sfi_gpio_get_entry_by_name);
> > +
> > +static int __init sfi_gpio_parse(struct sfi_table_header *table)
> > +{
> > +	struct sfi_table_simple *sb;
> > +	struct sfi_gpio_table_entry *pentry;
> > +	int num, i;
> > +
> > +	if (sfi_gpio_table)
> > +		return -EBUSY;
> 
> It can't ever happen. This is a static function called once in this file.
> IMO kernel doesn't need such kind of tests :)

Will fix it.

> 
> > +
> > +	sb = container_of(table, struct sfi_table_simple, header);
> > +
> > +	num = SFI_GET_NUM_ENTRIES(sb, struct sfi_gpio_table_entry);
> > +	pentry = (struct sfi_gpio_table_entry *)sb->pentry;
> > +
> > +	sfi_gpio_table = kmemdup(pentry, num * sizeof(*pentry), GFP_KERNEL);
> > +	if (!sfi_gpio_table)
> > +		return -ENOMEM;
> > +
> > +	sfi_gpio_num_entry = num;
> > +
> > +	pr_debug("GPIO pin info:\n");
> > +	for (i = 0; i < num; i++, pentry++)
> > +		pr_debug("GPIO [%2d] chip = %16.16s, name = %16.16s, pin=%d\n",
> > +			 i, pentry->controller_name, pentry->pin_name,
> > +			 pentry->pin_no);
> > +
> > +	return 0;
> > +}
> > +
> > +int __init sfi_gpio_init(void)
> > +{
> > +	return sfi_table_parse(SFI_SIG_GPIO, NULL, NULL, sfi_gpio_parse);
> > +}
> > diff --git a/include/linux/sfi.h b/include/linux/sfi.h
> > index d9b436f..510e74d 100644
> > --- a/include/linux/sfi.h
> > +++ b/include/linux/sfi.h
> > @@ -185,6 +185,8 @@ static inline void disable_sfi(void)
> >  	sfi_disabled = 1;
> >  }
> >  
> > +struct sfi_gpio_table_entry *sfi_gpio_get_entry_by_name(const char *name);
> > +
> >  #else /* !CONFIG_SFI */
> >  
> >  static inline void sfi_init(void)
> > @@ -204,6 +206,12 @@ static inline int sfi_table_parse(char *signature, char *oem_id,
> >  	return -1;
> >  }
> >  
> > +static inline
> > +struct sfi_gpio_table_entry *sfi_gpio_get_entry_by_name(const char *name)
> > +{
> > +	return NULL;
> 
> I'd suggest something more meaningful like ERR_PTR(-ENODEV).
> Again, for development sanity.

Okay.

> 
> Br, David Cohen
> 
> > +}
> > +
> >  #endif /* !CONFIG_SFI */
> >  
> >  #endif /*_LINUX_SFI_H*/
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1 3/3] gpiolib: append SFI helpers for GPIO API

[Andy Shevchenko]

On Fri, 2013-12-06 at 10:52 +0900, Alex Courbot wrote:
> On 12/06/2013 01:36 AM, Andy Shevchenko wrote:
> > To support some (legacy) firmwares and platforms let's make life easier for
> > their customers.
> >
> > This patch provides a function which converts sfi_gpio_table_entry to
> > gpio_desc. The use of it is integrated into GPIO library to enable generic
> > access to the SFI GPIO resources.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >   drivers/gpio/Kconfig       |  4 ++++
> >   drivers/gpio/Makefile      |  1 +
> >   drivers/gpio/gpiolib-sfi.c | 28 ++++++++++++++++++++++++++++
> >   drivers/gpio/gpiolib.c     |  3 +++
> >   drivers/gpio/gpiolib.h     | 13 +++++++++++++
> >   5 files changed, 49 insertions(+)
> >   create mode 100644 drivers/gpio/gpiolib-sfi.c
> >
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index ae3682d..a12752a 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -51,6 +51,10 @@ config OF_GPIO
> >   	def_bool y
> >   	depends on OF
> >
> > +config GPIO_SFI
> > +	def_bool y
> > +	depends on SFI
> > +
> >   config GPIO_ACPI
> >   	def_bool y
> >   	depends on ACPI
> > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> > index ee95154..5373e3a 100644
> > --- a/drivers/gpio/Makefile
> > +++ b/drivers/gpio/Makefile
> > @@ -5,6 +5,7 @@ ccflags-$(CONFIG_DEBUG_GPIO)	+= -DDEBUG
> >   obj-$(CONFIG_GPIO_DEVRES)	+= devres.o
> >   obj-$(CONFIG_GPIOLIB)		+= gpiolib.o
> >   obj-$(CONFIG_OF_GPIO)		+= gpiolib-of.o
> > +obj-$(CONFIG_GPIO_SFI)		+= gpiolib-sfi.o
> >   obj-$(CONFIG_GPIO_ACPI)		+= gpiolib-acpi.o
> >
> >   # Device drivers. Generally keep list sorted alphabetically
> > diff --git a/drivers/gpio/gpiolib-sfi.c b/drivers/gpio/gpiolib-sfi.c
> > new file mode 100644
> > index 0000000..c804314
> > --- /dev/null
> > +++ b/drivers/gpio/gpiolib-sfi.c
> > @@ -0,0 +1,28 @@
> > +/*
> > + * Simple Firmware Interface (SFI) helpers for GPIO API
> > + *
> > + * Copyright (C) 2013, Intel Corporation
> > + * Author: Andy Shevchenko <andriy.shevchenko@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/gpio/consumer.h>
> > +#include <linux/sfi.h>
> > +#include <linux/errno.h>
> > +#include <linux/err.h>
> > +
> > +#include "gpiolib.h"
> > +
> > +struct gpio_desc *sfi_get_gpiod_by_name(const char *name)
> > +{
> > +	struct sfi_gpio_table_entry *pentry;
> > +
> > +	pentry = sfi_gpio_get_entry_by_name(name);
> > +	if (!pentry)
> > +		return ERR_PTR(-ENODEV);
> > +
> > +	return gpio_to_desc(pentry->pin_no);
> > +}
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index bad400c..789ae1c 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -2451,6 +2451,9 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
> >   	} else if (IS_ENABLED(CONFIG_ACPI) && dev && ACPI_HANDLE(dev)) {
> >   		dev_dbg(dev, "using ACPI for GPIO lookup\n");
> >   		desc = acpi_find_gpio(dev, con_id, idx, &flags);
> > +	} else if (IS_ENABLED(CONFIG_SFI)) {
> > +		dev_dbg(dev, "using SFI for GPIO lookup\n");
> > +		desc = sfi_get_gpiod_by_name(con_id);
> 
> Your lookup function is ignoring the dev argument. Are SFI GPIOs always 
> supposed to be system-global?

It's not clear. It could be device related, though SFI itself has
probably wrong design. I rather prefer to avoid a dev parameter check at
all.

>  In this case, your if condition should 
> likely be
> 
> 	} else if (IS_ENABLED(CONFIG_SFI) && !dev) {
> 
> So that a global SFI GPIO does not get mistakenly assigned to a device 
> that has, say, a more suited platform mapping on the same con_id.

So, for example in the driver that could be enumerated from SFI, DT, and
via platform data you suggest to have something like


desc = gpiod_get(dev, "con_id_device_tree");
...

if (IS_ERR(desc))
 desc = gpiod_get(NULL, "con_id_sfi");

if (IS_ERR(desc))
 desc = gpiod_get(???, "con_id_from_platdata");

Correct?

>From my point of view, the SFI, platform data, and DT cases should have
the same con_id name. And I don't see a problem here, if we use dev
parameter != NULL for SFI case. Since you proposed to distinguish no
entity case vs. others, it would be easy to return it from SFI gpio
helper.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1 3/3] gpiolib: append SFI helpers for GPIO API

[Andy Shevchenko]

On Thu, 2013-12-05 at 15:20 -0800, David Cohen wrote:
> On 12/05/2013 08:36 AM, Andy Shevchenko wrote:
> > To support some (legacy) firmwares and platforms let's make life easier for
> > their customers.
> >
> > This patch provides a function which converts sfi_gpio_table_entry to
> > gpio_desc. The use of it is integrated into GPIO library to enable generic
> > access to the SFI GPIO resources.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  drivers/gpio/Kconfig       |  4 ++++
> >  drivers/gpio/Makefile      |  1 +
> >  drivers/gpio/gpiolib-sfi.c | 28 ++++++++++++++++++++++++++++
> >  drivers/gpio/gpiolib.c     |  3 +++
> >  drivers/gpio/gpiolib.h     | 13 +++++++++++++
> >  5 files changed, 49 insertions(+)
> >  create mode 100644 drivers/gpio/gpiolib-sfi.c
> >
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index ae3682d..a12752a 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -51,6 +51,10 @@ config OF_GPIO
> >  	def_bool y
> >  	depends on OF
> >  
> > +config GPIO_SFI
> > +	def_bool y
> > +	depends on SFI
> > +
> >  config GPIO_ACPI
> >  	def_bool y
> >  	depends on ACPI
> > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> > index ee95154..5373e3a 100644
> > --- a/drivers/gpio/Makefile
> > +++ b/drivers/gpio/Makefile
> > @@ -5,6 +5,7 @@ ccflags-$(CONFIG_DEBUG_GPIO)	+= -DDEBUG
> >  obj-$(CONFIG_GPIO_DEVRES)	+= devres.o
> >  obj-$(CONFIG_GPIOLIB)		+= gpiolib.o
> >  obj-$(CONFIG_OF_GPIO)		+= gpiolib-of.o
> > +obj-$(CONFIG_GPIO_SFI)		+= gpiolib-sfi.o
> >  obj-$(CONFIG_GPIO_ACPI)		+= gpiolib-acpi.o
> >  
> >  # Device drivers. Generally keep list sorted alphabetically
> > diff --git a/drivers/gpio/gpiolib-sfi.c b/drivers/gpio/gpiolib-sfi.c
> > new file mode 100644
> > index 0000000..c804314
> > --- /dev/null
> > +++ b/drivers/gpio/gpiolib-sfi.c
> > @@ -0,0 +1,28 @@
> > +/*
> > + * Simple Firmware Interface (SFI) helpers for GPIO API
> > + *
> > + * Copyright (C) 2013, Intel Corporation
> > + * Author: Andy Shevchenko <andriy.shevchenko@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/gpio/consumer.h>
> > +#include <linux/sfi.h>
> > +#include <linux/errno.h>
> > +#include <linux/err.h>
> > +
> > +#include "gpiolib.h"
> > +
> > +struct gpio_desc *sfi_get_gpiod_by_name(const char *name)
> > +{
> > +	struct sfi_gpio_table_entry *pentry;
> > +
> > +	pentry = sfi_gpio_get_entry_by_name(name);
> > +	if (!pentry)
> > +		return ERR_PTR(-ENODEV);
> 
> With my comments on patch 2/3, I'd suggest here:
> if (!pentry)
>      return ERR_PTR(-EINVAL);

It should never be here. We have to avoid tristate return pointers.

> if (IS_ERR(pentry))
>      return pentry;

This one correct, I agree.

> !pentry == could not find

Rather -ENOENT.

> IS_ERR(pentry) == something else went wrong
> 
> Br, David
> 
> 
> > +
> > +	return gpio_to_desc(pentry->pin_no);
> > +}
> >
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1 3/3] gpiolib: append SFI helpers for GPIO API

[Alex Courbot]

On 12/09/2013 07:11 PM, Andy Shevchenko wrote:
> On Fri, 2013-12-06 at 10:52 +0900, Alex Courbot wrote:
>> On 12/06/2013 01:36 AM, Andy Shevchenko wrote:
>>> To support some (legacy) firmwares and platforms let's make life easier for
>>> their customers.
>>>
>>> This patch provides a function which converts sfi_gpio_table_entry to
>>> gpio_desc. The use of it is integrated into GPIO library to enable generic
>>> access to the SFI GPIO resources.
>>>
>>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>> ---
>>>    drivers/gpio/Kconfig       |  4 ++++
>>>    drivers/gpio/Makefile      |  1 +
>>>    drivers/gpio/gpiolib-sfi.c | 28 ++++++++++++++++++++++++++++
>>>    drivers/gpio/gpiolib.c     |  3 +++
>>>    drivers/gpio/gpiolib.h     | 13 +++++++++++++
>>>    5 files changed, 49 insertions(+)
>>>    create mode 100644 drivers/gpio/gpiolib-sfi.c
>>>
>>> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
>>> index ae3682d..a12752a 100644
>>> --- a/drivers/gpio/Kconfig
>>> +++ b/drivers/gpio/Kconfig
>>> @@ -51,6 +51,10 @@ config OF_GPIO
>>>    	def_bool y
>>>    	depends on OF
>>>
>>> +config GPIO_SFI
>>> +	def_bool y
>>> +	depends on SFI
>>> +
>>>    config GPIO_ACPI
>>>    	def_bool y
>>>    	depends on ACPI
>>> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
>>> index ee95154..5373e3a 100644
>>> --- a/drivers/gpio/Makefile
>>> +++ b/drivers/gpio/Makefile
>>> @@ -5,6 +5,7 @@ ccflags-$(CONFIG_DEBUG_GPIO)	+= -DDEBUG
>>>    obj-$(CONFIG_GPIO_DEVRES)	+= devres.o
>>>    obj-$(CONFIG_GPIOLIB)		+= gpiolib.o
>>>    obj-$(CONFIG_OF_GPIO)		+= gpiolib-of.o
>>> +obj-$(CONFIG_GPIO_SFI)		+= gpiolib-sfi.o
>>>    obj-$(CONFIG_GPIO_ACPI)		+= gpiolib-acpi.o
>>>
>>>    # Device drivers. Generally keep list sorted alphabetically
>>> diff --git a/drivers/gpio/gpiolib-sfi.c b/drivers/gpio/gpiolib-sfi.c
>>> new file mode 100644
>>> index 0000000..c804314
>>> --- /dev/null
>>> +++ b/drivers/gpio/gpiolib-sfi.c
>>> @@ -0,0 +1,28 @@
>>> +/*
>>> + * Simple Firmware Interface (SFI) helpers for GPIO API
>>> + *
>>> + * Copyright (C) 2013, Intel Corporation
>>> + * Author: Andy Shevchenko <andriy.shevchenko@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/gpio/consumer.h>
>>> +#include <linux/sfi.h>
>>> +#include <linux/errno.h>
>>> +#include <linux/err.h>
>>> +
>>> +#include "gpiolib.h"
>>> +
>>> +struct gpio_desc *sfi_get_gpiod_by_name(const char *name)
>>> +{
>>> +	struct sfi_gpio_table_entry *pentry;
>>> +
>>> +	pentry = sfi_gpio_get_entry_by_name(name);
>>> +	if (!pentry)
>>> +		return ERR_PTR(-ENODEV);
>>> +
>>> +	return gpio_to_desc(pentry->pin_no);
>>> +}
>>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>>> index bad400c..789ae1c 100644
>>> --- a/drivers/gpio/gpiolib.c
>>> +++ b/drivers/gpio/gpiolib.c
>>> @@ -2451,6 +2451,9 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
>>>    	} else if (IS_ENABLED(CONFIG_ACPI) && dev && ACPI_HANDLE(dev)) {
>>>    		dev_dbg(dev, "using ACPI for GPIO lookup\n");
>>>    		desc = acpi_find_gpio(dev, con_id, idx, &flags);
>>> +	} else if (IS_ENABLED(CONFIG_SFI)) {
>>> +		dev_dbg(dev, "using SFI for GPIO lookup\n");
>>> +		desc = sfi_get_gpiod_by_name(con_id);
>>
>> Your lookup function is ignoring the dev argument. Are SFI GPIOs always
>> supposed to be system-global?
>
> It's not clear. It could be device related, though SFI itself has
> probably wrong design. I rather prefer to avoid a dev parameter check at
> all.
>
>>   In this case, your if condition should
>> likely be
>>
>> 	} else if (IS_ENABLED(CONFIG_SFI) && !dev) {
>>
>> So that a global SFI GPIO does not get mistakenly assigned to a device
>> that has, say, a more suited platform mapping on the same con_id.
>
> So, for example in the driver that could be enumerated from SFI, DT, and
> via platform data you suggest to have something like
>
>
> desc = gpiod_get(dev, "con_id_device_tree");
> ...
>
> if (IS_ERR(desc))
>   desc = gpiod_get(NULL, "con_id_sfi");
>
> if (IS_ERR(desc))
>   desc = gpiod_get(???, "con_id_from_platdata");
>
> Correct?

No, this is not what I'm suggesting. Device drivers should not care who 
provides the GPIO, they should just ask for it, and obtain it (or not).

The scope of the problem actually depends on what SFI GPIOs are used 
for. For instance, let's say you have two devices each using an enabling 
GPIO which happens to be provided by SFI. Both drivers for these devices 
obtain the enable GPIO as follows:

	enable_gpio = gpiod_get(dev, "enable");

Here you actually have two problems:

1) Since you only look for con_id, how to you discriminate the enable 
GPIOs for these devices?

2) The device drivers are the one to decide which GPIO name they 
request. How are SFI GPIO names decided? If you don't have any kind of 
flexibility for their naming, and want to use them with devices drivers, 
you will very likely need another naming layer that associates a 
(device_name, con_id) pair to the right SFI GPIO, similarly to what is 
done with platform GPIOs. If the only consumer of SFI GPIOs is platform 
code, and SFI GPIOs are all uniquely named, then you may as well request 
the device to be NULL for their lookup so that they don't interfere with 
more precisely-mapped GPIOs.

I don't know anything about SFI GPIOs, how they are defined, where their 
name comes from, and how they are used so my vision may be incomplete. 
But AFAICT it all comes down to one of these two scenarios:

1) SFI GPIOs are only used in platform code -> using their pin name is 
ok, device argument should be assumed to be NULL for their matching
2) SFI GPIOs are also consumed by device drivers -> you need a way to 
match a (device, con_id) pair to your SFI GPIOs so they can be matched 
exactly and through the names drivers will request.

Re: [PATCH v1 3/3] gpiolib: append SFI helpers for GPIO API

[Andy Shevchenko]

On Tue, 2013-12-10 at 12:00 +0900, Alex Courbot wrote:
> On 12/09/2013 07:11 PM, Andy Shevchenko wrote:
> > On Fri, 2013-12-06 at 10:52 +0900, Alex Courbot wrote:
> >> On 12/06/2013 01:36 AM, Andy Shevchenko wrote:
> >>> To support some (legacy) firmwares and platforms let's make life easier for
> >>> their customers.
> >>>
> >>> This patch provides a function which converts sfi_gpio_table_entry to
> >>> gpio_desc. The use of it is integrated into GPIO library to enable generic
> >>> access to the SFI GPIO resources.
> >>>
> >>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >>> ---
> >>>    drivers/gpio/Kconfig       |  4 ++++
> >>>    drivers/gpio/Makefile      |  1 +
> >>>    drivers/gpio/gpiolib-sfi.c | 28 ++++++++++++++++++++++++++++
> >>>    drivers/gpio/gpiolib.c     |  3 +++
> >>>    drivers/gpio/gpiolib.h     | 13 +++++++++++++
> >>>    5 files changed, 49 insertions(+)
> >>>    create mode 100644 drivers/gpio/gpiolib-sfi.c
> >>>
> >>> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> >>> index ae3682d..a12752a 100644
> >>> --- a/drivers/gpio/Kconfig
> >>> +++ b/drivers/gpio/Kconfig
> >>> @@ -51,6 +51,10 @@ config OF_GPIO
> >>>    	def_bool y
> >>>    	depends on OF
> >>>
> >>> +config GPIO_SFI
> >>> +	def_bool y
> >>> +	depends on SFI
> >>> +
> >>>    config GPIO_ACPI
> >>>    	def_bool y
> >>>    	depends on ACPI
> >>> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> >>> index ee95154..5373e3a 100644
> >>> --- a/drivers/gpio/Makefile
> >>> +++ b/drivers/gpio/Makefile
> >>> @@ -5,6 +5,7 @@ ccflags-$(CONFIG_DEBUG_GPIO)	+= -DDEBUG
> >>>    obj-$(CONFIG_GPIO_DEVRES)	+= devres.o
> >>>    obj-$(CONFIG_GPIOLIB)		+= gpiolib.o
> >>>    obj-$(CONFIG_OF_GPIO)		+= gpiolib-of.o
> >>> +obj-$(CONFIG_GPIO_SFI)		+= gpiolib-sfi.o
> >>>    obj-$(CONFIG_GPIO_ACPI)		+= gpiolib-acpi.o
> >>>
> >>>    # Device drivers. Generally keep list sorted alphabetically
> >>> diff --git a/drivers/gpio/gpiolib-sfi.c b/drivers/gpio/gpiolib-sfi.c
> >>> new file mode 100644
> >>> index 0000000..c804314
> >>> --- /dev/null
> >>> +++ b/drivers/gpio/gpiolib-sfi.c
> >>> @@ -0,0 +1,28 @@
> >>> +/*
> >>> + * Simple Firmware Interface (SFI) helpers for GPIO API
> >>> + *
> >>> + * Copyright (C) 2013, Intel Corporation
> >>> + * Author: Andy Shevchenko <andriy.shevchenko@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/gpio/consumer.h>
> >>> +#include <linux/sfi.h>
> >>> +#include <linux/errno.h>
> >>> +#include <linux/err.h>
> >>> +
> >>> +#include "gpiolib.h"
> >>> +
> >>> +struct gpio_desc *sfi_get_gpiod_by_name(const char *name)
> >>> +{
> >>> +	struct sfi_gpio_table_entry *pentry;
> >>> +
> >>> +	pentry = sfi_gpio_get_entry_by_name(name);
> >>> +	if (!pentry)
> >>> +		return ERR_PTR(-ENODEV);
> >>> +
> >>> +	return gpio_to_desc(pentry->pin_no);
> >>> +}
> >>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> >>> index bad400c..789ae1c 100644
> >>> --- a/drivers/gpio/gpiolib.c
> >>> +++ b/drivers/gpio/gpiolib.c
> >>> @@ -2451,6 +2451,9 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
> >>>    	} else if (IS_ENABLED(CONFIG_ACPI) && dev && ACPI_HANDLE(dev)) {
> >>>    		dev_dbg(dev, "using ACPI for GPIO lookup\n");
> >>>    		desc = acpi_find_gpio(dev, con_id, idx, &flags);
> >>> +	} else if (IS_ENABLED(CONFIG_SFI)) {
> >>> +		dev_dbg(dev, "using SFI for GPIO lookup\n");
> >>> +		desc = sfi_get_gpiod_by_name(con_id);
> >>
> >> Your lookup function is ignoring the dev argument. Are SFI GPIOs always
> >> supposed to be system-global?
> >
> > It's not clear. It could be device related, though SFI itself has
> > probably wrong design. I rather prefer to avoid a dev parameter check at
> > all.
> >
> >>   In this case, your if condition should
> >> likely be
> >>
> >> 	} else if (IS_ENABLED(CONFIG_SFI) && !dev) {
> >>
> >> So that a global SFI GPIO does not get mistakenly assigned to a device
> >> that has, say, a more suited platform mapping on the same con_id.
> >
> > So, for example in the driver that could be enumerated from SFI, DT, and
> > via platform data you suggest to have something like
> >
> >
> > desc = gpiod_get(dev, "con_id_device_tree");
> > ...
> >
> > if (IS_ERR(desc))
> >   desc = gpiod_get(NULL, "con_id_sfi");
> >
> > if (IS_ERR(desc))
> >   desc = gpiod_get(???, "con_id_from_platdata");
> >
> > Correct?
> 
> No, this is not what I'm suggesting. Device drivers should not care who 
> provides the GPIO, they should just ask for it, and obtain it (or not).
> 
> The scope of the problem actually depends on what SFI GPIOs are used 
> for. For instance, let's say you have two devices each using an enabling 
> GPIO which happens to be provided by SFI. Both drivers for these devices 
> obtain the enable GPIO as follows:
> 
> 	enable_gpio = gpiod_get(dev, "enable");

You rather can't do that in sfi case. Yes, the structure has (stringy)
reference to gpio chip that provides a line, but in practice the gpio
line name should be unique. Consider this is misdesign of SFI as I
mentioned earlier.

> 
> Here you actually have two problems:
> 
> 1) Since you only look for con_id, how to you discriminate the enable 
> GPIOs for these devices?

> 2) The device drivers are the one to decide which GPIO name they 
> request. How are SFI GPIO names decided? If you don't have any kind of 
> flexibility for their naming, and want to use them with devices drivers, 
> you will very likely need another naming layer that associates a 
> (device_name, con_id) pair to the right SFI GPIO, similarly to what is 
> done with platform GPIOs. If the only consumer of SFI GPIOs is platform 
> code, and SFI GPIOs are all uniquely named, then you may as well request 
> the device to be NULL for their lookup so that they don't interfere with 
> more precisely-mapped GPIOs.

So, you insist to have !dev there?

> 
> I don't know anything about SFI GPIOs, how they are defined, where their 
> name comes from, and how they are used so my vision may be incomplete. 
> But AFAICT it all comes down to one of these two scenarios:
> 
> 1) SFI GPIOs are only used in platform code -> using their pin name is 
> ok, device argument should be assumed to be NULL for their matching

Let's stick to this.

> 2) SFI GPIOs are also consumed by device drivers -> you need a way to 
> match a (device, con_id) pair to your SFI GPIOs so they can be matched 
> exactly and through the names drivers will request.

Mostly unlikely we go this way. It would mean we don't need SFI at all.


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1 3/3] gpiolib: append SFI helpers for GPIO API

[David Cohen]

Hi,

[snip]

>> I don't know anything about SFI GPIOs, how they are defined, where their 
>> name comes from, and how they are used so my vision may be incomplete. 
>> But AFAICT it all comes down to one of these two scenarios:
>>
>> 1) SFI GPIOs are only used in platform code -> using their pin name is 
>> ok, device argument should be assumed to be NULL for their matching
> Let's stick to this.
>
>> 2) SFI GPIOs are also consumed by device drivers -> you need a way to 
>> match a (device, con_id) pair to your SFI GPIOs so they can be matched 
>> exactly and through the names drivers will request.
> Mostly unlikely we go this way. It would mean we don't need SFI at all.

I'm currently upstreaming Intel-MID's SFI users. This is a forbidden
situation which I'd change before submit the patch.

Br, David Cohen

Re: [PATCH v1 3/3] gpiolib: append SFI helpers for GPIO API

[Andy Shevchenko]

On Tue, 2013-12-10 at 07:21 -0800, David Cohen wrote:
> Hi,
> 
> [snip]
> 
> >> I don't know anything about SFI GPIOs, how they are defined, where their 
> >> name comes from, and how they are used so my vision may be incomplete. 
> >> But AFAICT it all comes down to one of these two scenarios:
> >>
> >> 1) SFI GPIOs are only used in platform code -> using their pin name is 
> >> ok, device argument should be assumed to be NULL for their matching
> > Let's stick to this.
> >
> >> 2) SFI GPIOs are also consumed by device drivers -> you need a way to 
> >> match a (device, con_id) pair to your SFI GPIOs so they can be matched 
> >> exactly and through the names drivers will request.
> > Mostly unlikely we go this way. It would mean we don't need SFI at all.
> 
> I'm currently upstreaming Intel-MID's SFI users. This is a forbidden
> situation which I'd change before submit the patch.

I'm sorry I didn't get this. You mean we have to have another mapping,
or you agree with system-global GPIOs in SFI case?

> 
> Br, David Cohen

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1 3/3] gpiolib: append SFI helpers for GPIO API

[Alex Courbot]

On 12/10/2013 10:15 PM, Andy Shevchenko wrote:
> On Tue, 2013-12-10 at 12:00 +0900, Alex Courbot wrote:
>> On 12/09/2013 07:11 PM, Andy Shevchenko wrote:
>>> On Fri, 2013-12-06 at 10:52 +0900, Alex Courbot wrote:
>>>> On 12/06/2013 01:36 AM, Andy Shevchenko wrote:
>>>>> To support some (legacy) firmwares and platforms let's make life easier for
>>>>> their customers.
>>>>>
>>>>> This patch provides a function which converts sfi_gpio_table_entry to
>>>>> gpio_desc. The use of it is integrated into GPIO library to enable generic
>>>>> access to the SFI GPIO resources.
>>>>>
>>>>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>>>> ---
>>>>>     drivers/gpio/Kconfig       |  4 ++++
>>>>>     drivers/gpio/Makefile      |  1 +
>>>>>     drivers/gpio/gpiolib-sfi.c | 28 ++++++++++++++++++++++++++++
>>>>>     drivers/gpio/gpiolib.c     |  3 +++
>>>>>     drivers/gpio/gpiolib.h     | 13 +++++++++++++
>>>>>     5 files changed, 49 insertions(+)
>>>>>     create mode 100644 drivers/gpio/gpiolib-sfi.c
>>>>>
>>>>> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
>>>>> index ae3682d..a12752a 100644
>>>>> --- a/drivers/gpio/Kconfig
>>>>> +++ b/drivers/gpio/Kconfig
>>>>> @@ -51,6 +51,10 @@ config OF_GPIO
>>>>>     	def_bool y
>>>>>     	depends on OF
>>>>>
>>>>> +config GPIO_SFI
>>>>> +	def_bool y
>>>>> +	depends on SFI
>>>>> +
>>>>>     config GPIO_ACPI
>>>>>     	def_bool y
>>>>>     	depends on ACPI
>>>>> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
>>>>> index ee95154..5373e3a 100644
>>>>> --- a/drivers/gpio/Makefile
>>>>> +++ b/drivers/gpio/Makefile
>>>>> @@ -5,6 +5,7 @@ ccflags-$(CONFIG_DEBUG_GPIO)	+= -DDEBUG
>>>>>     obj-$(CONFIG_GPIO_DEVRES)	+= devres.o
>>>>>     obj-$(CONFIG_GPIOLIB)		+= gpiolib.o
>>>>>     obj-$(CONFIG_OF_GPIO)		+= gpiolib-of.o
>>>>> +obj-$(CONFIG_GPIO_SFI)		+= gpiolib-sfi.o
>>>>>     obj-$(CONFIG_GPIO_ACPI)		+= gpiolib-acpi.o
>>>>>
>>>>>     # Device drivers. Generally keep list sorted alphabetically
>>>>> diff --git a/drivers/gpio/gpiolib-sfi.c b/drivers/gpio/gpiolib-sfi.c
>>>>> new file mode 100644
>>>>> index 0000000..c804314
>>>>> --- /dev/null
>>>>> +++ b/drivers/gpio/gpiolib-sfi.c
>>>>> @@ -0,0 +1,28 @@
>>>>> +/*
>>>>> + * Simple Firmware Interface (SFI) helpers for GPIO API
>>>>> + *
>>>>> + * Copyright (C) 2013, Intel Corporation
>>>>> + * Author: Andy Shevchenko <andriy.shevchenko@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/gpio/consumer.h>
>>>>> +#include <linux/sfi.h>
>>>>> +#include <linux/errno.h>
>>>>> +#include <linux/err.h>
>>>>> +
>>>>> +#include "gpiolib.h"
>>>>> +
>>>>> +struct gpio_desc *sfi_get_gpiod_by_name(const char *name)
>>>>> +{
>>>>> +	struct sfi_gpio_table_entry *pentry;
>>>>> +
>>>>> +	pentry = sfi_gpio_get_entry_by_name(name);
>>>>> +	if (!pentry)
>>>>> +		return ERR_PTR(-ENODEV);
>>>>> +
>>>>> +	return gpio_to_desc(pentry->pin_no);
>>>>> +}
>>>>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>>>>> index bad400c..789ae1c 100644
>>>>> --- a/drivers/gpio/gpiolib.c
>>>>> +++ b/drivers/gpio/gpiolib.c
>>>>> @@ -2451,6 +2451,9 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
>>>>>     	} else if (IS_ENABLED(CONFIG_ACPI) && dev && ACPI_HANDLE(dev)) {
>>>>>     		dev_dbg(dev, "using ACPI for GPIO lookup\n");
>>>>>     		desc = acpi_find_gpio(dev, con_id, idx, &flags);
>>>>> +	} else if (IS_ENABLED(CONFIG_SFI)) {
>>>>> +		dev_dbg(dev, "using SFI for GPIO lookup\n");
>>>>> +		desc = sfi_get_gpiod_by_name(con_id);
>>>>
>>>> Your lookup function is ignoring the dev argument. Are SFI GPIOs always
>>>> supposed to be system-global?
>>>
>>> It's not clear. It could be device related, though SFI itself has
>>> probably wrong design. I rather prefer to avoid a dev parameter check at
>>> all.
>>>
>>>>    In this case, your if condition should
>>>> likely be
>>>>
>>>> 	} else if (IS_ENABLED(CONFIG_SFI) && !dev) {
>>>>
>>>> So that a global SFI GPIO does not get mistakenly assigned to a device
>>>> that has, say, a more suited platform mapping on the same con_id.
>>>
>>> So, for example in the driver that could be enumerated from SFI, DT, and
>>> via platform data you suggest to have something like
>>>
>>>
>>> desc = gpiod_get(dev, "con_id_device_tree");
>>> ...
>>>
>>> if (IS_ERR(desc))
>>>    desc = gpiod_get(NULL, "con_id_sfi");
>>>
>>> if (IS_ERR(desc))
>>>    desc = gpiod_get(???, "con_id_from_platdata");
>>>
>>> Correct?
>>
>> No, this is not what I'm suggesting. Device drivers should not care who
>> provides the GPIO, they should just ask for it, and obtain it (or not).
>>
>> The scope of the problem actually depends on what SFI GPIOs are used
>> for. For instance, let's say you have two devices each using an enabling
>> GPIO which happens to be provided by SFI. Both drivers for these devices
>> obtain the enable GPIO as follows:
>>
>> 	enable_gpio = gpiod_get(dev, "enable");
>
> You rather can't do that in sfi case. Yes, the structure has (stringy)
> reference to gpio chip that provides a line, but in practice the gpio
> line name should be unique. Consider this is misdesign of SFI as I
> mentioned earlier.

Ok. Note that this is not necessarily a bad thing, it just means that 
SFI GPIOs should only be used by platform-specific code that is aware of 
their existence (and particular naming).

Device drivers on the other hand cannot make any assumption on who will 
provide the GPIO, so if you want them to be able to use SFI GPIOs, you 
will need an actual way to map then to (device, function) pairs. If this 
use-case is out of the equation, then I'm glad with the v6 of your patch.

But David raised a point that kind of sounds like he might want to use 
them that way, so I'm holding off my ack until this is solved.

Alex.

Re: [PATCH v1 3/3] gpiolib: append SFI helpers for GPIO API

[David Cohen]

On 12/10/2013 06:47 PM, Alex Courbot wrote:
> On 12/10/2013 10:15 PM, Andy Shevchenko wrote:
>> On Tue, 2013-12-10 at 12:00 +0900, Alex Courbot wrote:
>>> On 12/09/2013 07:11 PM, Andy Shevchenko wrote:
>>>> On Fri, 2013-12-06 at 10:52 +0900, Alex Courbot wrote:
>>>>> On 12/06/2013 01:36 AM, Andy Shevchenko wrote:
>>>>>> To support some (legacy) firmwares and platforms let's make life
>>>>>> easier for
>>>>>> their customers.
>>>>>>
>>>>>> This patch provides a function which converts
>>>>>> sfi_gpio_table_entry to
>>>>>> gpio_desc. The use of it is integrated into GPIO library to
>>>>>> enable generic
>>>>>> access to the SFI GPIO resources.
>>>>>>
>>>>>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>>>>> ---
>>>>>>     drivers/gpio/Kconfig       |  4 ++++
>>>>>>     drivers/gpio/Makefile      |  1 +
>>>>>>     drivers/gpio/gpiolib-sfi.c | 28 ++++++++++++++++++++++++++++
>>>>>>     drivers/gpio/gpiolib.c     |  3 +++
>>>>>>     drivers/gpio/gpiolib.h     | 13 +++++++++++++
>>>>>>     5 files changed, 49 insertions(+)
>>>>>>     create mode 100644 drivers/gpio/gpiolib-sfi.c
>>>>>>
>>>>>> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
>>>>>> index ae3682d..a12752a 100644
>>>>>> --- a/drivers/gpio/Kconfig
>>>>>> +++ b/drivers/gpio/Kconfig
>>>>>> @@ -51,6 +51,10 @@ config OF_GPIO
>>>>>>         def_bool y
>>>>>>         depends on OF
>>>>>>
>>>>>> +config GPIO_SFI
>>>>>> +    def_bool y
>>>>>> +    depends on SFI
>>>>>> +
>>>>>>     config GPIO_ACPI
>>>>>>         def_bool y
>>>>>>         depends on ACPI
>>>>>> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
>>>>>> index ee95154..5373e3a 100644
>>>>>> --- a/drivers/gpio/Makefile
>>>>>> +++ b/drivers/gpio/Makefile
>>>>>> @@ -5,6 +5,7 @@ ccflags-$(CONFIG_DEBUG_GPIO)    += -DDEBUG
>>>>>>     obj-$(CONFIG_GPIO_DEVRES)    += devres.o
>>>>>>     obj-$(CONFIG_GPIOLIB)        += gpiolib.o
>>>>>>     obj-$(CONFIG_OF_GPIO)        += gpiolib-of.o
>>>>>> +obj-$(CONFIG_GPIO_SFI)        += gpiolib-sfi.o
>>>>>>     obj-$(CONFIG_GPIO_ACPI)        += gpiolib-acpi.o
>>>>>>
>>>>>>     # Device drivers. Generally keep list sorted alphabetically
>>>>>> diff --git a/drivers/gpio/gpiolib-sfi.c b/drivers/gpio/gpiolib-sfi.c
>>>>>> new file mode 100644
>>>>>> index 0000000..c804314
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/gpio/gpiolib-sfi.c
>>>>>> @@ -0,0 +1,28 @@
>>>>>> +/*
>>>>>> + * Simple Firmware Interface (SFI) helpers for GPIO API
>>>>>> + *
>>>>>> + * Copyright (C) 2013, Intel Corporation
>>>>>> + * Author: Andy Shevchenko <andriy.shevchenko@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/gpio/consumer.h>
>>>>>> +#include <linux/sfi.h>
>>>>>> +#include <linux/errno.h>
>>>>>> +#include <linux/err.h>
>>>>>> +
>>>>>> +#include "gpiolib.h"
>>>>>> +
>>>>>> +struct gpio_desc *sfi_get_gpiod_by_name(const char *name)
>>>>>> +{
>>>>>> +    struct sfi_gpio_table_entry *pentry;
>>>>>> +
>>>>>> +    pentry = sfi_gpio_get_entry_by_name(name);
>>>>>> +    if (!pentry)
>>>>>> +        return ERR_PTR(-ENODEV);
>>>>>> +
>>>>>> +    return gpio_to_desc(pentry->pin_no);
>>>>>> +}
>>>>>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>>>>>> index bad400c..789ae1c 100644
>>>>>> --- a/drivers/gpio/gpiolib.c
>>>>>> +++ b/drivers/gpio/gpiolib.c
>>>>>> @@ -2451,6 +2451,9 @@ struct gpio_desc *__must_check
>>>>>> gpiod_get_index(struct device *dev,
>>>>>>         } else if (IS_ENABLED(CONFIG_ACPI) && dev &&
>>>>>> ACPI_HANDLE(dev)) {
>>>>>>             dev_dbg(dev, "using ACPI for GPIO lookup\n");
>>>>>>             desc = acpi_find_gpio(dev, con_id, idx, &flags);
>>>>>> +    } else if (IS_ENABLED(CONFIG_SFI)) {
>>>>>> +        dev_dbg(dev, "using SFI for GPIO lookup\n");
>>>>>> +        desc = sfi_get_gpiod_by_name(con_id);
>>>>>
>>>>> Your lookup function is ignoring the dev argument. Are SFI GPIOs
>>>>> always
>>>>> supposed to be system-global?
>>>>
>>>> It's not clear. It could be device related, though SFI itself has
>>>> probably wrong design. I rather prefer to avoid a dev parameter
>>>> check at
>>>> all.
>>>>
>>>>>    In this case, your if condition should
>>>>> likely be
>>>>>
>>>>>     } else if (IS_ENABLED(CONFIG_SFI) && !dev) {
>>>>>
>>>>> So that a global SFI GPIO does not get mistakenly assigned to a
>>>>> device
>>>>> that has, say, a more suited platform mapping on the same con_id.
>>>>
>>>> So, for example in the driver that could be enumerated from SFI,
>>>> DT, and
>>>> via platform data you suggest to have something like
>>>>
>>>>
>>>> desc = gpiod_get(dev, "con_id_device_tree");
>>>> ...
>>>>
>>>> if (IS_ERR(desc))
>>>>    desc = gpiod_get(NULL, "con_id_sfi");
>>>>
>>>> if (IS_ERR(desc))
>>>>    desc = gpiod_get(???, "con_id_from_platdata");
>>>>
>>>> Correct?
>>>
>>> No, this is not what I'm suggesting. Device drivers should not care who
>>> provides the GPIO, they should just ask for it, and obtain it (or not).
>>>
>>> The scope of the problem actually depends on what SFI GPIOs are used
>>> for. For instance, let's say you have two devices each using an
>>> enabling
>>> GPIO which happens to be provided by SFI. Both drivers for these
>>> devices
>>> obtain the enable GPIO as follows:
>>>
>>>     enable_gpio = gpiod_get(dev, "enable");
>>
>> You rather can't do that in sfi case. Yes, the structure has (stringy)
>> reference to gpio chip that provides a line, but in practice the gpio
>> line name should be unique. Consider this is misdesign of SFI as I
>> mentioned earlier.
>
> Ok. Note that this is not necessarily a bad thing, it just means that
> SFI GPIOs should only be used by platform-specific code that is aware
> of their existence (and particular naming).
>
> Device drivers on the other hand cannot make any assumption on who
> will provide the GPIO, so if you want them to be able to use SFI
> GPIOs, you will need an actual way to map then to (device, function)
> pairs. If this use-case is out of the equation, then I'm glad with the
> v6 of your patch.
>
> But David raised a point that kind of sounds like he might want to use
> them that way, so I'm holding off my ack until this is solved.

Intel-MID needs this approach on platform code inside
arch/x86/platform/intel-mid/device_libs/ directory. But I agree it
should not
be found on device drivers in general.

Br, David

>
> Alex.
>

Re: [PATCH v1 3/3] gpiolib: append SFI helpers for GPIO API

[Alex Courbot]

On 12/12/2013 09:45 AM, David Cohen wrote:
> On 12/10/2013 06:47 PM, Alex Courbot wrote:
>> On 12/10/2013 10:15 PM, Andy Shevchenko wrote:
>>> On Tue, 2013-12-10 at 12:00 +0900, Alex Courbot wrote:
>>>> On 12/09/2013 07:11 PM, Andy Shevchenko wrote:
>>>>> On Fri, 2013-12-06 at 10:52 +0900, Alex Courbot wrote:
>>>>>> On 12/06/2013 01:36 AM, Andy Shevchenko wrote:
>>>>>>> To support some (legacy) firmwares and platforms let's make life
>>>>>>> easier for
>>>>>>> their customers.
>>>>>>>
>>>>>>> This patch provides a function which converts
>>>>>>> sfi_gpio_table_entry to
>>>>>>> gpio_desc. The use of it is integrated into GPIO library to
>>>>>>> enable generic
>>>>>>> access to the SFI GPIO resources.
>>>>>>>
>>>>>>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>>>>>> ---
>>>>>>>      drivers/gpio/Kconfig       |  4 ++++
>>>>>>>      drivers/gpio/Makefile      |  1 +
>>>>>>>      drivers/gpio/gpiolib-sfi.c | 28 ++++++++++++++++++++++++++++
>>>>>>>      drivers/gpio/gpiolib.c     |  3 +++
>>>>>>>      drivers/gpio/gpiolib.h     | 13 +++++++++++++
>>>>>>>      5 files changed, 49 insertions(+)
>>>>>>>      create mode 100644 drivers/gpio/gpiolib-sfi.c
>>>>>>>
>>>>>>> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
>>>>>>> index ae3682d..a12752a 100644
>>>>>>> --- a/drivers/gpio/Kconfig
>>>>>>> +++ b/drivers/gpio/Kconfig
>>>>>>> @@ -51,6 +51,10 @@ config OF_GPIO
>>>>>>>          def_bool y
>>>>>>>          depends on OF
>>>>>>>
>>>>>>> +config GPIO_SFI
>>>>>>> +    def_bool y
>>>>>>> +    depends on SFI
>>>>>>> +
>>>>>>>      config GPIO_ACPI
>>>>>>>          def_bool y
>>>>>>>          depends on ACPI
>>>>>>> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
>>>>>>> index ee95154..5373e3a 100644
>>>>>>> --- a/drivers/gpio/Makefile
>>>>>>> +++ b/drivers/gpio/Makefile
>>>>>>> @@ -5,6 +5,7 @@ ccflags-$(CONFIG_DEBUG_GPIO)    += -DDEBUG
>>>>>>>      obj-$(CONFIG_GPIO_DEVRES)    += devres.o
>>>>>>>      obj-$(CONFIG_GPIOLIB)        += gpiolib.o
>>>>>>>      obj-$(CONFIG_OF_GPIO)        += gpiolib-of.o
>>>>>>> +obj-$(CONFIG_GPIO_SFI)        += gpiolib-sfi.o
>>>>>>>      obj-$(CONFIG_GPIO_ACPI)        += gpiolib-acpi.o
>>>>>>>
>>>>>>>      # Device drivers. Generally keep list sorted alphabetically
>>>>>>> diff --git a/drivers/gpio/gpiolib-sfi.c b/drivers/gpio/gpiolib-sfi.c
>>>>>>> new file mode 100644
>>>>>>> index 0000000..c804314
>>>>>>> --- /dev/null
>>>>>>> +++ b/drivers/gpio/gpiolib-sfi.c
>>>>>>> @@ -0,0 +1,28 @@
>>>>>>> +/*
>>>>>>> + * Simple Firmware Interface (SFI) helpers for GPIO API
>>>>>>> + *
>>>>>>> + * Copyright (C) 2013, Intel Corporation
>>>>>>> + * Author: Andy Shevchenko <andriy.shevchenko@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/gpio/consumer.h>
>>>>>>> +#include <linux/sfi.h>
>>>>>>> +#include <linux/errno.h>
>>>>>>> +#include <linux/err.h>
>>>>>>> +
>>>>>>> +#include "gpiolib.h"
>>>>>>> +
>>>>>>> +struct gpio_desc *sfi_get_gpiod_by_name(const char *name)
>>>>>>> +{
>>>>>>> +    struct sfi_gpio_table_entry *pentry;
>>>>>>> +
>>>>>>> +    pentry = sfi_gpio_get_entry_by_name(name);
>>>>>>> +    if (!pentry)
>>>>>>> +        return ERR_PTR(-ENODEV);
>>>>>>> +
>>>>>>> +    return gpio_to_desc(pentry->pin_no);
>>>>>>> +}
>>>>>>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>>>>>>> index bad400c..789ae1c 100644
>>>>>>> --- a/drivers/gpio/gpiolib.c
>>>>>>> +++ b/drivers/gpio/gpiolib.c
>>>>>>> @@ -2451,6 +2451,9 @@ struct gpio_desc *__must_check
>>>>>>> gpiod_get_index(struct device *dev,
>>>>>>>          } else if (IS_ENABLED(CONFIG_ACPI) && dev &&
>>>>>>> ACPI_HANDLE(dev)) {
>>>>>>>              dev_dbg(dev, "using ACPI for GPIO lookup\n");
>>>>>>>              desc = acpi_find_gpio(dev, con_id, idx, &flags);
>>>>>>> +    } else if (IS_ENABLED(CONFIG_SFI)) {
>>>>>>> +        dev_dbg(dev, "using SFI for GPIO lookup\n");
>>>>>>> +        desc = sfi_get_gpiod_by_name(con_id);
>>>>>>
>>>>>> Your lookup function is ignoring the dev argument. Are SFI GPIOs
>>>>>> always
>>>>>> supposed to be system-global?
>>>>>
>>>>> It's not clear. It could be device related, though SFI itself has
>>>>> probably wrong design. I rather prefer to avoid a dev parameter
>>>>> check at
>>>>> all.
>>>>>
>>>>>>     In this case, your if condition should
>>>>>> likely be
>>>>>>
>>>>>>      } else if (IS_ENABLED(CONFIG_SFI) && !dev) {
>>>>>>
>>>>>> So that a global SFI GPIO does not get mistakenly assigned to a
>>>>>> device
>>>>>> that has, say, a more suited platform mapping on the same con_id.
>>>>>
>>>>> So, for example in the driver that could be enumerated from SFI,
>>>>> DT, and
>>>>> via platform data you suggest to have something like
>>>>>
>>>>>
>>>>> desc = gpiod_get(dev, "con_id_device_tree");
>>>>> ...
>>>>>
>>>>> if (IS_ERR(desc))
>>>>>     desc = gpiod_get(NULL, "con_id_sfi");
>>>>>
>>>>> if (IS_ERR(desc))
>>>>>     desc = gpiod_get(???, "con_id_from_platdata");
>>>>>
>>>>> Correct?
>>>>
>>>> No, this is not what I'm suggesting. Device drivers should not care who
>>>> provides the GPIO, they should just ask for it, and obtain it (or not).
>>>>
>>>> The scope of the problem actually depends on what SFI GPIOs are used
>>>> for. For instance, let's say you have two devices each using an
>>>> enabling
>>>> GPIO which happens to be provided by SFI. Both drivers for these
>>>> devices
>>>> obtain the enable GPIO as follows:
>>>>
>>>>      enable_gpio = gpiod_get(dev, "enable");
>>>
>>> You rather can't do that in sfi case. Yes, the structure has (stringy)
>>> reference to gpio chip that provides a line, but in practice the gpio
>>> line name should be unique. Consider this is misdesign of SFI as I
>>> mentioned earlier.
>>
>> Ok. Note that this is not necessarily a bad thing, it just means that
>> SFI GPIOs should only be used by platform-specific code that is aware
>> of their existence (and particular naming).
>>
>> Device drivers on the other hand cannot make any assumption on who
>> will provide the GPIO, so if you want them to be able to use SFI
>> GPIOs, you will need an actual way to map then to (device, function)
>> pairs. If this use-case is out of the equation, then I'm glad with the
>> v6 of your patch.
>>
>> But David raised a point that kind of sounds like he might want to use
>> them that way, so I'm holding off my ack until this is solved.
>
> Intel-MID needs this approach on platform code inside
> arch/x86/platform/intel-mid/device_libs/ directory. But I agree it
> should not
> be found on device drivers in general.

That was my only concern. If these GPIOs are only used by code that 
expects them to come from SFI, then I'm fine with Andy's latest version.