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

2013-05-30 Thread Linus Walleij
On Thu, May 30, 2013 at 4:45 AM, Sathyanarayanan Kuppuswamy
 wrote:

> From: Andy Shevchenko 
>
> To support some (legacy) firmwares and platforms let's make life easier for
> their customers.

I'd like Grant to have a look at this too, if possible.

> Change-Id: I39a0e3e0ed4e04a0d733801f59d46d76189195e8

We don't put Google Gerrit change-IDs into the kernel log.
But fun to know you're using it...

(...)
> +++ b/drivers/gpio/gpiolib-sfi.c
> @@ -0,0 +1,76 @@
> +/*
> + * SFI helpers for GPIO API

Spell out "Simple Firmware Interface helpers..." it's not like
everyone knows.

> +static struct sfi_gpio_table_entry *sfi_gpio_table;
> +static int sfi_gpio_num_entry;
> +
> +int sfi_get_gpio_by_name(const char *name)
> +{
> +   struct sfi_gpio_table_entry *pentry = sfi_gpio_table;
> +   int i;
> +
> +   if (!pentry)
> +   return -1;

Don't you want to return a serious error code?

> +
> +   for (i = 0; i < sfi_gpio_num_entry; i++, pentry++) {
> +   if (!strncmp(name, pentry->pin_name, SFI_NAME_LEN))
> +   return pentry->pin_no;
> +   }
> +
> +   return -1;

Dito.

> +}
> +EXPORT_SYMBOL_GPL(sfi_get_gpio_by_name);

(...)
> diff --git a/include/linux/sfi_gpio.h b/include/linux/sfi_gpio.h
> new file mode 100644
> index 000..ef7e497
> --- /dev/null
> +++ b/include/linux/sfi_gpio.h
> @@ -0,0 +1,27 @@
> +#ifndef _LINUX_SFI_GPIO_H_
> +#define _LINUX_SFI_GPIO_H_
> +
> +#include 
> +#include 
> +#include 
> +
> +#ifdef CONFIG_GPIO_SFI
> +
> +int sfi_get_gpio_by_name(const char *name);
> +int __init sfi_gpio_init(void);

IIRC you do not need to tag things with __init in header files.

> +
> +#else /* CONFIG_GPIO_SFI */
> +
> +static inline int sfi_get_gpio_by_name(const char *name);
> +{
> +   return -1;

Why -1? -ENOTSUPP? Or some other error code?

> +}
> +
> +static inline int __init sfi_gpio_init(void)

Do not tag inline functions with __init.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2013-05-30 Thread Linus Walleij
On Thu, May 30, 2013 at 4:45 AM, Sathyanarayanan Kuppuswamy
sathyanarayanan.kuppusw...@intel.com wrote:

 From: Andy Shevchenko andriy.shevche...@linux.intel.com

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

I'd like Grant to have a look at this too, if possible.

 Change-Id: I39a0e3e0ed4e04a0d733801f59d46d76189195e8

We don't put Google Gerrit change-IDs into the kernel log.
But fun to know you're using it...

(...)
 +++ b/drivers/gpio/gpiolib-sfi.c
 @@ -0,0 +1,76 @@
 +/*
 + * SFI helpers for GPIO API

Spell out Simple Firmware Interface helpers... it's not like
everyone knows.

 +static struct sfi_gpio_table_entry *sfi_gpio_table;
 +static int sfi_gpio_num_entry;
 +
 +int sfi_get_gpio_by_name(const char *name)
 +{
 +   struct sfi_gpio_table_entry *pentry = sfi_gpio_table;
 +   int i;
 +
 +   if (!pentry)
 +   return -1;

Don't you want to return a serious error code?

 +
 +   for (i = 0; i  sfi_gpio_num_entry; i++, pentry++) {
 +   if (!strncmp(name, pentry-pin_name, SFI_NAME_LEN))
 +   return pentry-pin_no;
 +   }
 +
 +   return -1;

Dito.

 +}
 +EXPORT_SYMBOL_GPL(sfi_get_gpio_by_name);

(...)
 diff --git a/include/linux/sfi_gpio.h b/include/linux/sfi_gpio.h
 new file mode 100644
 index 000..ef7e497
 --- /dev/null
 +++ b/include/linux/sfi_gpio.h
 @@ -0,0 +1,27 @@
 +#ifndef _LINUX_SFI_GPIO_H_
 +#define _LINUX_SFI_GPIO_H_
 +
 +#include linux/errno.h
 +#include linux/gpio.h
 +#include linux/sfi.h
 +
 +#ifdef CONFIG_GPIO_SFI
 +
 +int sfi_get_gpio_by_name(const char *name);
 +int __init sfi_gpio_init(void);

IIRC you do not need to tag things with __init in header files.

 +
 +#else /* CONFIG_GPIO_SFI */
 +
 +static inline int sfi_get_gpio_by_name(const char *name);
 +{
 +   return -1;

Why -1? -ENOTSUPP? Or some other error code?

 +}
 +
 +static inline int __init sfi_gpio_init(void)

Do not tag inline functions with __init.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2013-05-29 Thread Sathyanarayanan Kuppuswamy
From: Andy Shevchenko 

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

Change-Id: I39a0e3e0ed4e04a0d733801f59d46d76189195e8
Signed-off-by: Andy Shevchenko 
---
 drivers/gpio/Kconfig   |4 +++
 drivers/gpio/Makefile  |1 +
 drivers/gpio/gpiolib-sfi.c |   76 
 drivers/sfi/sfi_core.c |7 
 include/linux/sfi_gpio.h   |   27 
 5 files changed, 115 insertions(+)
 create mode 100644 drivers/gpio/gpiolib-sfi.c
 create mode 100644 include/linux/sfi_gpio.h

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 573c449..8e4b0b8 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 0cb2d65..63d2abd 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 000..2f15a81
--- /dev/null
+++ b/drivers/gpio/gpiolib-sfi.c
@@ -0,0 +1,76 @@
+/*
+ * SFI helpers for GPIO API
+ *
+ * Copyright (C) 2013, Intel Corporation
+ * Author: Andy Shevchenko 
+ *
+ * Based on work done for Intel MID platforms (mrst.c)
+ *
+ * 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.
+ */
+
+#define pr_fmt(fmt) "SFI: GPIO: " fmt
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+static struct sfi_gpio_table_entry *sfi_gpio_table;
+static int sfi_gpio_num_entry;
+
+int sfi_get_gpio_by_name(const char *name)
+{
+   struct sfi_gpio_table_entry *pentry = sfi_gpio_table;
+   int i;
+
+   if (!pentry)
+   return -1;
+
+   for (i = 0; i < sfi_gpio_num_entry; i++, pentry++) {
+   if (!strncmp(name, pentry->pin_name, SFI_NAME_LEN))
+   return pentry->pin_no;
+   }
+
+   return -1;
+}
+EXPORT_SYMBOL_GPL(sfi_get_gpio_by_name);
+
+static int __init sfi_gpio_parse(struct sfi_table_header *table)
+{
+   struct sfi_table_simple *sb = (struct sfi_table_simple *)table;
+   struct sfi_gpio_table_entry *pentry;
+   int num, i;
+
+   if (sfi_gpio_table)
+   return 0;
+
+   num = SFI_GET_NUM_ENTRIES(sb, struct sfi_gpio_table_entry);
+   pentry = (struct sfi_gpio_table_entry *)sb->pentry;
+
+   sfi_gpio_table = kmalloc(num * sizeof(*pentry), GFP_KERNEL);
+   if (!sfi_gpio_table)
+   return -ENOMEM;
+
+   memcpy(sfi_gpio_table, pentry, num * sizeof(*pentry));
+   sfi_gpio_num_entry = num;
+
+   pr_debug("Pin info:\n");
+   for (i = 0; i < num; i++, pentry++)
+   pr_debug("[%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/drivers/sfi/sfi_core.c b/drivers/sfi/sfi_core.c
index 296db7a..2828da0 100644
--- a/drivers/sfi/sfi_core.c
+++ b/drivers/sfi/sfi_core.c
@@ -67,6 +67,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include "sfi_core.h"
@@ -512,6 +513,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/include/linux/sfi_gpio.h b/include/linux/sfi_gpio.h
new file mode 100644
index 000..ef7e497
--- /dev/null
+++ b/include/linux/sfi_gpio.h
@@ -0,0 +1,27 @@
+#ifndef _LINUX_SFI_GPIO_H_
+#define _LINUX_SFI_GPIO_H_
+
+#include 
+#include 
+#include 
+
+#ifdef CONFIG_GPIO_SFI
+
+int sfi_get_gpio_by_name(const char *name);
+int __init sfi_gpio_init(void);
+
+#else /* CONFIG_GPIO_SFI */
+
+static inline int sfi_get_gpio_by_name(const char *name);
+{
+   return -1;
+}
+
+static inline int __init sfi_gpio_init(void)
+{
+   return -ENODEV;
+}
+
+#endif /* CONFIG_GPIO_SFI */
+
+#endif /* _LINUX_SFI_GPIO_H_ */
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More 

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

2013-05-29 Thread Sathyanarayanan Kuppuswamy
From: Andy Shevchenko andriy.shevche...@linux.intel.com

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

Change-Id: I39a0e3e0ed4e04a0d733801f59d46d76189195e8
Signed-off-by: Andy Shevchenko andriy.shevche...@linux.intel.com
---
 drivers/gpio/Kconfig   |4 +++
 drivers/gpio/Makefile  |1 +
 drivers/gpio/gpiolib-sfi.c |   76 
 drivers/sfi/sfi_core.c |7 
 include/linux/sfi_gpio.h   |   27 
 5 files changed, 115 insertions(+)
 create mode 100644 drivers/gpio/gpiolib-sfi.c
 create mode 100644 include/linux/sfi_gpio.h

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 573c449..8e4b0b8 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 0cb2d65..63d2abd 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 000..2f15a81
--- /dev/null
+++ b/drivers/gpio/gpiolib-sfi.c
@@ -0,0 +1,76 @@
+/*
+ * SFI helpers for GPIO API
+ *
+ * Copyright (C) 2013, Intel Corporation
+ * Author: Andy Shevchenko andriy.shevche...@linux.intel.com
+ *
+ * Based on work done for Intel MID platforms (mrst.c)
+ *
+ * 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.
+ */
+
+#define pr_fmt(fmt) SFI: GPIO:  fmt
+
+#include linux/errno.h
+#include linux/kernel.h
+#include linux/string.h
+#include linux/slab.h
+#include linux/gpio.h
+#include linux/export.h
+#include linux/sfi_gpio.h
+#include linux/sfi.h
+
+static struct sfi_gpio_table_entry *sfi_gpio_table;
+static int sfi_gpio_num_entry;
+
+int sfi_get_gpio_by_name(const char *name)
+{
+   struct sfi_gpio_table_entry *pentry = sfi_gpio_table;
+   int i;
+
+   if (!pentry)
+   return -1;
+
+   for (i = 0; i  sfi_gpio_num_entry; i++, pentry++) {
+   if (!strncmp(name, pentry-pin_name, SFI_NAME_LEN))
+   return pentry-pin_no;
+   }
+
+   return -1;
+}
+EXPORT_SYMBOL_GPL(sfi_get_gpio_by_name);
+
+static int __init sfi_gpio_parse(struct sfi_table_header *table)
+{
+   struct sfi_table_simple *sb = (struct sfi_table_simple *)table;
+   struct sfi_gpio_table_entry *pentry;
+   int num, i;
+
+   if (sfi_gpio_table)
+   return 0;
+
+   num = SFI_GET_NUM_ENTRIES(sb, struct sfi_gpio_table_entry);
+   pentry = (struct sfi_gpio_table_entry *)sb-pentry;
+
+   sfi_gpio_table = kmalloc(num * sizeof(*pentry), GFP_KERNEL);
+   if (!sfi_gpio_table)
+   return -ENOMEM;
+
+   memcpy(sfi_gpio_table, pentry, num * sizeof(*pentry));
+   sfi_gpio_num_entry = num;
+
+   pr_debug(Pin info:\n);
+   for (i = 0; i  num; i++, pentry++)
+   pr_debug([%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/drivers/sfi/sfi_core.c b/drivers/sfi/sfi_core.c
index 296db7a..2828da0 100644
--- a/drivers/sfi/sfi_core.c
+++ b/drivers/sfi/sfi_core.c
@@ -67,6 +67,7 @@
 #include linux/acpi.h
 #include linux/init.h
 #include linux/sfi.h
+#include linux/sfi_gpio.h
 #include linux/slab.h
 
 #include sfi_core.h
@@ -512,6 +513,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/include/linux/sfi_gpio.h b/include/linux/sfi_gpio.h
new file mode 100644
index 000..ef7e497
--- /dev/null
+++ b/include/linux/sfi_gpio.h
@@ -0,0 +1,27 @@
+#ifndef _LINUX_SFI_GPIO_H_
+#define _LINUX_SFI_GPIO_H_
+
+#include linux/errno.h
+#include linux/gpio.h
+#include linux/sfi.h
+
+#ifdef CONFIG_GPIO_SFI
+
+int sfi_get_gpio_by_name(const char *name);
+int __init sfi_gpio_init(void);
+
+#else /* CONFIG_GPIO_SFI */
+
+static inline int sfi_get_gpio_by_name(const char *name);
+{
+   return -1;
+}
+