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.shevche...@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.shevche...@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.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
