Previously, gpiolib would unconditionally flag all GPIO pins as inputs,
regardless of their true state. This resulted in all GPIO output pins
initially being incorrectly identified as "input" in the GPIO sysfs.
Since the direction of GPIOs is not known prior to having their
direction set, instead set the default direction to "unknown" to prevent
user confusion. A pin with an "unknown" direction can not be written or
read via sysfs; it must first be configured as an input or output before
it can be used.
While we're playing with the direction flag in/out defines, rename them to
a more descriptive FLAG_DIR_* format.
Cc: Alek Du <[email protected]>
Cc: Samuel Ortiz <[email protected]>
Cc: David Brownell <[email protected]>
Cc: Eric Miao <[email protected]>
Cc: Uwe Kleine-K?nig <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Joe Perches <[email protected]>
Cc: Alan Cox <[email protected]>
Cc: Grant Likely <[email protected]>
Signed-off-by: Peter Tyser <[email protected]>
---
Change since V1:
- This patch is new and is based on feedback from Alan to support a new
"unknown" direction.
- Rename FLAG_IS_OUT to FLAG_DIR_OUT, and add FLAG_DIR_IN
drivers/gpio/gpiolib.c | 82 +++++++++++++++++++++++++++--------------------
1 files changed, 47 insertions(+), 35 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 649550e..0113c10 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -49,13 +49,14 @@ struct gpio_desc {
unsigned long flags;
/* flag symbols are bit numbers */
#define FLAG_REQUESTED 0
-#define FLAG_IS_OUT 1
-#define FLAG_RESERVED 2
-#define FLAG_EXPORT 3 /* protected by sysfs_lock */
-#define FLAG_SYSFS 4 /* exported via /sys/class/gpio/control */
-#define FLAG_TRIG_FALL 5 /* trigger on falling edge */
-#define FLAG_TRIG_RISE 6 /* trigger on rising edge */
-#define FLAG_ACTIVE_LOW 7 /* sysfs value has active low */
+#define FLAG_DIR_OUT 1 /* pin is an output */
+#define FLAG_DIR_IN 2 /* pin is an input */
+#define FLAG_RESERVED 3
+#define FLAG_EXPORT 4 /* protected by sysfs_lock */
+#define FLAG_SYSFS 5 /* exported via /sys/class/gpio/control */
+#define FLAG_TRIG_FALL 6 /* trigger on falling edge */
+#define FLAG_TRIG_RISE 7 /* trigger on rising edge */
+#define FLAG_ACTIVE_LOW 8 /* sysfs value has active low */
#define ID_SHIFT 16 /* add new flags before this one */
@@ -220,15 +221,22 @@ static ssize_t gpio_direction_show(struct device *dev,
{
const struct gpio_desc *desc = dev_get_drvdata(dev);
ssize_t status;
+ const char *dir_str;
mutex_lock(&sysfs_lock);
- if (!test_bit(FLAG_EXPORT, &desc->flags))
+ if (!test_bit(FLAG_EXPORT, &desc->flags)) {
status = -EIO;
- else
- status = sprintf(buf, "%s\n",
- test_bit(FLAG_IS_OUT, &desc->flags)
- ? "out" : "in");
+ } else {
+ if (test_bit(FLAG_DIR_OUT, &desc->flags))
+ dir_str = "out";
+ else if (test_bit(FLAG_DIR_IN, &desc->flags))
+ dir_str = "in";
+ else
+ dir_str = "unknown";
+
+ status = sprintf(buf, "%s\n", dir_str);
+ }
mutex_unlock(&sysfs_lock);
return status;
@@ -272,6 +280,9 @@ static ssize_t gpio_value_show(struct device *dev,
if (!test_bit(FLAG_EXPORT, &desc->flags)) {
status = -EIO;
+ } else if ((!test_bit(FLAG_DIR_OUT, &desc->flags)) &&
+ (!test_bit(FLAG_DIR_IN, &desc->flags))) {
+ status = -EPERM;
} else {
int value;
@@ -297,7 +308,7 @@ static ssize_t gpio_value_store(struct device *dev,
if (!test_bit(FLAG_EXPORT, &desc->flags))
status = -EIO;
- else if (!test_bit(FLAG_IS_OUT, &desc->flags))
+ else if (!test_bit(FLAG_DIR_OUT, &desc->flags))
status = -EPERM;
else {
long value;
@@ -741,7 +752,7 @@ int gpio_export(unsigned gpio, bool direction_may_change)
if (!status && gpio_to_irq(gpio) >= 0
&& (direction_may_change
- || !test_bit(FLAG_IS_OUT,
+ || test_bit(FLAG_DIR_IN,
&desc->flags)))
status = device_create_file(dev,
&dev_attr_edge);
@@ -1059,22 +1070,10 @@ int gpiochip_add(struct gpio_chip *chip)
break;
}
}
- if (status == 0) {
- for (id = base; id < base + chip->ngpio; id++) {
+ if (status == 0)
+ for (id = base; id < base + chip->ngpio; id++)
gpio_desc[id].chip = chip;
- /* REVISIT: most hardware initializes GPIOs as
- * inputs (often with pullups enabled) so power
- * usage is minimized. Linux code should set the
- * gpio direction first thing; but until it does,
- * we may expose the wrong direction in sysfs.
- */
- gpio_desc[id].flags = !chip->direction_input
- ? (1 << FLAG_IS_OUT)
- : 0;
- }
- }
-
of_gpiochip_add(chip);
unlock:
@@ -1402,8 +1401,10 @@ int gpio_direction_input(unsigned gpio)
}
status = chip->direction_input(chip, gpio);
- if (status == 0)
- clear_bit(FLAG_IS_OUT, &desc->flags);
+ if (status == 0) {
+ clear_bit(FLAG_DIR_OUT, &desc->flags);
+ set_bit(FLAG_DIR_IN, &desc->flags);
+ }
lose:
return status;
fail:
@@ -1455,8 +1456,10 @@ int gpio_direction_output(unsigned gpio, int value)
}
status = chip->direction_output(chip, gpio, value);
- if (status == 0)
- set_bit(FLAG_IS_OUT, &desc->flags);
+ if (status == 0) {
+ clear_bit(FLAG_DIR_IN, &desc->flags);
+ set_bit(FLAG_DIR_OUT, &desc->flags);
+ }
lose:
return status;
fail:
@@ -1643,21 +1646,27 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)
unsigned i;
unsigned gpio = chip->base;
struct gpio_desc *gdesc = &gpio_desc[gpio];
- int is_out;
+ const char *dir_str;
+ int unknown = 0;
for (i = 0; i < chip->ngpio; i++, gpio++, gdesc++) {
if (!test_bit(FLAG_REQUESTED, &gdesc->flags))
continue;
- is_out = test_bit(FLAG_IS_OUT, &gdesc->flags);
- seq_printf(s, " gpio-%-3d (%-20.20s) %s %s",
- gpio, gdesc->label,
- is_out ? "out" : "in ",
- chip->get
- ? (chip->get(chip, i) ? "hi" : "lo")
- : "? ");
+ if (test_bit(FLAG_DIR_IN, &gdesc->flags)) {
+ dir_str = "in ";
+ } else if (test_bit(FLAG_DIR_OUT, &gdesc->flags)) {
+ dir_str = "out";
+ } else {
+ dir_str = "? ";
+ unknown = 1;
+ }
+
+ seq_printf(s, " gpio-%-3d (%-20.20s) %s %s", gpio, gdesc->label,
+ dir_str, (chip->get && !unknown) ?
+ (chip->get(chip, i) ? "hi" : "lo") : "?");
- if (!is_out) {
+ if (test_bit(FLAG_DIR_IN, &gdesc->flags)) {
int irq = gpio_to_irq(gpio);
struct irq_desc *desc = irq_to_desc(irq);
--
1.7.0.4
Add a new get_direction() function to the gpio_chip structure. This is
useful so that the direction of a pin can be determined when its
initially exported. Previously, the direction defaulted to "unknown"
regardless of the actual configuration of the GPIO pin.
If a GPIO driver implements get_direction(), it is called in
gpio_request() to set the initial direction of the pin accurately.
Cc: Alek Du <[email protected]>
Cc: Samuel Ortiz <[email protected]>
Cc: David Brownell <[email protected]>
Cc: Eric Miao <[email protected]>
Cc: Uwe Kleine-K?nig <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Joe Perches <[email protected]>
Cc: Alan Cox <[email protected]>
Cc: Grant Likely <[email protected]>
Signed-off-by: Peter Tyser <[email protected]>
---
Changes since v1:
- Add support for "unknown" direction
drivers/gpio/gpiolib.c | 20 ++++++++++++++++++++
include/asm-generic/gpio.h | 4 ++++
2 files changed, 24 insertions(+), 0 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 0113c10..7723beb 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1214,6 +1214,26 @@ int gpio_request(unsigned gpio, const char *label)
}
}
+ if (chip->get_direction) {
+ /* chip->get_direction may sleep */
+ spin_unlock_irqrestore(&gpio_lock, flags);
+ switch (chip->get_direction(chip, gpio - chip->base)) {
+ case -1:
+ clear_bit(FLAG_DIR_OUT, &desc->flags);
+ clear_bit(FLAG_DIR_IN, &desc->flags);
+ break;
+ case 0:
+ set_bit(FLAG_DIR_IN, &desc->flags);
+ clear_bit(FLAG_DIR_OUT, &desc->flags);
+ break;
+ case 1:
+ set_bit(FLAG_DIR_OUT, &desc->flags);
+ clear_bit(FLAG_DIR_IN, &desc->flags);
+ break;
+ }
+ spin_lock_irqsave(&gpio_lock, flags);
+ }
+
done:
if (status)
pr_debug("gpio_request: gpio-%d (%s) status %d\n",
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index ff5c660..6af5a3e 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -57,6 +57,8 @@ struct device_node;
* @direction_input: configures signal "offset" as input, or returns error
* @get: returns value for signal "offset"; for output signals this
* returns either the value actually sensed, or zero
+ * @get_direction: optional hook to determine if a GPIO signal is configured
+ * as an input (0) or output (1), or unknown (-1)
* @direction_output: configures signal "offset" as output, or returns error
* @set: assigns output value for signal "offset"
* @to_irq: optional hook supporting non-static gpio_to_irq() mappings;
@@ -101,6 +103,8 @@ struct gpio_chip {
unsigned offset);
int (*get)(struct gpio_chip *chip,
unsigned offset);
+ int (*get_direction)(struct gpio_chip *chip,
+ unsigned offset);
int (*direction_output)(struct gpio_chip *chip,
unsigned offset, int value);
int (*set_debounce)(struct gpio_chip *chip,
--
1.7.0.4
Adding the get_direction() function allows the accurate GPIO pin
direction data to be shown in sysfs. Previously, the direction was
was always initialized to 'unknown'.
Cc: Alek Du <[email protected]>
Cc: Samuel Ortiz <[email protected]>
Cc: David Brownell <[email protected]>
Cc: Eric Miao <[email protected]>
Cc: Uwe Kleine-K?nig <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Joe Perches <[email protected]>
Cc: Alan Cox <[email protected]>
Cc: Grant Likely <[email protected]>
Signed-off-by: Peter Tyser <[email protected]>
---
Changes since v1:
- None
drivers/gpio/pca953x.c | 11 +++++++++++
1 files changed, 11 insertions(+), 0 deletions(-)
diff --git a/drivers/gpio/pca953x.c b/drivers/gpio/pca953x.c
index b473429..66d36b1 100644
--- a/drivers/gpio/pca953x.c
+++ b/drivers/gpio/pca953x.c
@@ -168,6 +168,16 @@ exit:
return ret;
}
+static int pca953x_gpio_get_direction(struct gpio_chip *gc, unsigned off)
+{
+ struct pca953x_chip *chip;
+
+ chip = container_of(gc, struct pca953x_chip, gpio_chip);
+
+ /* return 0 if IO pin is input, 1 if its an output */
+ return chip->reg_direction & (1u << off) ? 0 : 1;
+}
+
static int pca953x_gpio_get_value(struct gpio_chip *gc, unsigned off)
{
struct pca953x_chip *chip;
@@ -221,6 +231,7 @@ static void pca953x_setup_gpio(struct pca953x_chip *chip, int gpios)
gc->direction_input = pca953x_gpio_direction_input;
gc->direction_output = pca953x_gpio_direction_output;
+ gc->get_direction = pca953x_gpio_get_direction;
gc->get = pca953x_gpio_get_value;
gc->set = pca953x_gpio_set_value;
gc->can_sleep = 1;
--
1.7.0.4
This driver works on many Intel chipsets, including the ICH6, ICH7,
ICH8, ICH9, ICH10, 3100, Series 5/3400 (Ibex Peak), Series 6/C200
(Cougar Point).
Additional Intel chipsets should be easily supported if needed, eg the
ICH1-5, EP80579, etc.
Tested on a QM57 (Ibex Peak) and 3100.
Cc: Alek Du <[email protected]>
Cc: Samuel Ortiz <[email protected]>
Cc: David Brownell <[email protected]>
Cc: Eric Miao <[email protected]>
Cc: Uwe Kleine-K?nig <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Joe Perches <[email protected]>
Cc: Alan Cox <[email protected]>
Cc: Grant Likely <[email protected]>
Signed-off-by: Peter Tyser <[email protected]>
---
Changes since v1:
- Fixed grammar in Kconfig menu entry
MAINTAINERS | 5 +
drivers/gpio/Kconfig | 11 +
drivers/gpio/Makefile | 1 +
drivers/gpio/ichx_gpio.c | 551 ++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 568 insertions(+), 0 deletions(-)
create mode 100644 drivers/gpio/ichx_gpio.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 4837907..1778396 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3108,6 +3108,11 @@ W: http://www.developer.ibm.com/welcome/netfinity/serveraid.html
S: Supported
F: drivers/scsi/ips.*
+ICHX GPIO DRIVER
+M: Peter Tyser <[email protected]>
+S: Maintained
+F: drivers/gpio/ichx_gpio.c
+
IDE SUBSYSTEM
M: "David S. Miller" <[email protected]>
L: [email protected]
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 664660e..13b7998 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -319,6 +319,17 @@ config GPIO_BT8XX
If unsure, say N.
+config GPIO_ICHX
+ tristate "Intel ICHx GPIO"
+ depends on PCI && GPIOLIB && X86
+ help
+ Say yes here to support the GPIO functionality of a number of Intel
+ ICH-based chipsets. Currently supported devices: ICH6, ICH7, ICH8
+ ICH9, ICH10, Series 5/3400 (eg Ibex Peak), Series 6/C200 (eg
+ Cougar Point), and 3100.
+
+ If unsure, say N.
+
config GPIO_LANGWELL
bool "Intel Langwell/Penwell GPIO support"
depends on PCI
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 3351cf8..8900bc6 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_GPIOLIB) += gpiolib.o
obj-$(CONFIG_GPIO_ADP5520) += adp5520-gpio.o
obj-$(CONFIG_GPIO_ADP5588) += adp5588-gpio.o
obj-$(CONFIG_GPIO_BASIC_MMIO) += basic_mmio_gpio.o
+obj-$(CONFIG_GPIO_ICHX) += ichx_gpio.o
obj-$(CONFIG_GPIO_LANGWELL) += langwell_gpio.o
obj-$(CONFIG_GPIO_MAX730X) += max730x.o
obj-$(CONFIG_GPIO_MAX7300) += max7300.o
diff --git a/drivers/gpio/ichx_gpio.c b/drivers/gpio/ichx_gpio.c
new file mode 100644
index 0000000..88f02dc
--- /dev/null
+++ b/drivers/gpio/ichx_gpio.c
@@ -0,0 +1,551 @@
+/*
+ * Intel ICH6-10, Series 5 and 6 GPIO driver
+ *
+ * Copyright (C) 2010 Extreme Engineering Solutions.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/gpio.h>
+#include <linux/platform_device.h>
+
+#define DRV_NAME "ichx_gpio"
+
+/* PCI config register offsets into LPC I/F - D31:F0 */
+#define PCI_ICHX_ACPI_BAR 0x40
+#define PCI_ICHX_GPIO_BAR 0x48
+#define PCI_ICHX_GPIO_CTRL 0x4C
+
+/*
+ * GPIO register offsets in GPIO I/O space.
+ * Each chunk of 32 GPIOs is manipulated via its own USE_SELx, IO_SELx, and
+ * LVLx registers. Logic in the read/write functions takes a register and
+ * an absolute bit number and determines the proper register offset and bit
+ * number in that register. For example, to read the value of GPIO bit 50
+ * the code would access offset ichx_regs[2(=GPIO_LVL)][1(=50/32)],
+ * bit 18 (50%32).
+ */
+enum GPIO_REG {
+ GPIO_USE_SEL = 0,
+ GPIO_IO_SEL,
+ GPIO_LVL,
+};
+static const u8 ichx_regs[3][3] = {
+ {0x00, 0x30, 0x40}, /* USE_SEL[1-3] offsets */
+ {0x04, 0x34, 0x44}, /* IO_SEL[1-3] offsets */
+ {0x0c, 0x38, 0x48}, /* LVL[1-3] offsets */
+};
+
+#define ICHX_GPIO_WRITE(val, reg) outl(val, (reg) + ichx_priv.gpio_base)
+#define ICHX_GPIO_READ(reg) inl((reg) + ichx_priv.gpio_base)
+#define ICHX_PM_WRITE(val, reg) outl(val, (reg) + ichx_priv.pm_base)
+#define ICHX_PM_READ(reg) inl((reg) + ichx_priv.pm_base)
+
+/* Convenient wrapper to make our PCI ID table */
+#define ICHX_GPIO_PCI_DEVICE(dev, data) \
+ .vendor = PCI_VENDOR_ID_INTEL, \
+ .device = dev, \
+ .subvendor = PCI_ANY_ID, \
+ .subdevice = PCI_ANY_ID, \
+ .class = 0, \
+ .class_mask = 0, \
+ .driver_data = (ulong)data
+
+struct ichx_desc {
+ /* Max GPIO pins the chipset can have */
+ uint ngpio;
+
+ /* The offset of GPE0_STS in the PM IO region, 0 if unneeded */
+ uint gpe0_sts_ofs;
+
+ /* USE_SEL is bogus on some chipsets, eg 3100 */
+ u32 use_sel_ignore[3];
+
+ /* Some chipsets have quirks, let these use their own request/get */
+ int (*request)(struct gpio_chip *chip, unsigned offset);
+ int (*get)(struct gpio_chip *chip, unsigned offset);
+};
+
+static struct {
+ spinlock_t lock;
+ struct platform_device *dev;
+ struct pci_dev *pdev;
+ struct gpio_chip chip;
+ unsigned long gpio_base;/* GPIO IO base */
+ unsigned long pm_base; /* Power Mangagment IO base */
+ struct ichx_desc *desc; /* Pointer to chipset-specific description */
+ u32 orig_gpio_ctrl; /* Orig CTRL value, used to restore on exit */
+} ichx_priv;
+
+static struct platform_device *ichx_gpio_platform_device;
+
+static int modparam_gpiobase = -1 /* dynamic */;
+module_param_named(gpiobase, modparam_gpiobase, int, 0444);
+MODULE_PARM_DESC(gpiobase, "The GPIO number base. -1 means dynamic, "
+ "which is the default.");
+
+static int ichx_write_bit(int reg, unsigned nr, int val, int verify)
+{
+ unsigned long flags;
+ u32 data;
+ int reg_nr = nr / 32;
+ int bit = nr & 0x1f;
+ int ret = 0;
+
+ spin_lock_irqsave(&ichx_priv.lock, flags);
+
+ data = ICHX_GPIO_READ(ichx_regs[reg][reg_nr]);
+ data = (data & ~(1 << bit)) | (val << bit);
+ ICHX_GPIO_WRITE(data, ichx_regs[reg][reg_nr]);
+ if (verify && (data != ICHX_GPIO_READ(ichx_regs[reg][reg_nr])))
+ ret = -EPERM;
+
+ spin_unlock_irqrestore(&ichx_priv.lock, flags);
+
+ return ret;
+}
+
+static int ichx_read_bit(int reg, unsigned nr)
+{
+ unsigned long flags;
+ u32 data;
+ int reg_nr = nr / 32;
+ int bit = nr & 0x1f;
+
+ spin_lock_irqsave(&ichx_priv.lock, flags);
+
+ data = ICHX_GPIO_READ(ichx_regs[reg][reg_nr]);
+
+ spin_unlock_irqrestore(&ichx_priv.lock, flags);
+
+ return data & (1 << bit) ? 1 : 0;
+}
+
+static int ichx_gpio_direction_input(struct gpio_chip *gpio, unsigned nr)
+{
+ /*
+ * Try setting pin as an input and verify it worked since many pins
+ * are output-only.
+ */
+ if (ichx_write_bit(GPIO_IO_SEL, nr, 1, 1))
+ return -EINVAL;
+
+ return 0;
+}
+
+static int ichx_gpio_direction_output(struct gpio_chip *gpio, unsigned nr,
+ int val)
+{
+ /* Set GPIO output value. */
+ ichx_write_bit(GPIO_LVL, nr, val, 0);
+
+ /*
+ * Try setting pin as an output and verify it worked since many pins
+ * are input-only.
+ */
+ if (ichx_write_bit(GPIO_IO_SEL, nr, 0, 1))
+ return -EINVAL;
+
+ return 0;
+}
+
+static int ichx_gpio_get_direction(struct gpio_chip *chip, unsigned nr)
+{
+ /* Return 0 if IO pin is input, 1 if its an output */
+ return !ichx_read_bit(GPIO_IO_SEL, nr);
+}
+
+static int ichx_gpio_get(struct gpio_chip *chip, unsigned nr)
+{
+ return ichx_read_bit(GPIO_LVL, nr);
+}
+
+static int ich6_gpio_get(struct gpio_chip *chip, unsigned nr)
+{
+ unsigned long flags;
+ u32 data;
+
+ /*
+ * GPI 0 - 15 need to be read from the power management registers on
+ * a ICH6/3100 bridge.
+ */
+ if (nr < 16) {
+ spin_lock_irqsave(&ichx_priv.lock, flags);
+
+ /* GPI 0 - 15 are latched, write 1 to clear*/
+ ICHX_PM_WRITE(1 << (16 + nr), ichx_priv.desc->gpe0_sts_ofs);
+
+ data = ICHX_PM_READ(ichx_priv.desc->gpe0_sts_ofs);
+
+ spin_unlock_irqrestore(&ichx_priv.lock, flags);
+
+ return (data >> 16) & (1 << nr) ? 1 : 0;
+ } else {
+ return ichx_gpio_get(chip, nr);
+ }
+}
+
+static int ichx_gpio_request(struct gpio_chip *chip, unsigned nr)
+{
+ /*
+ * Note we assume the BIOS properly set a bridge's USE value. Some
+ * chips (eg Intel 3100) have bogus USE values though, so first see if
+ * the chipset's USE value can be trusted for this specific bit.
+ * If it can't be trusted, assume that the pin can be used as a GPIO.
+ */
+ if (ichx_priv.desc->use_sel_ignore[nr / 32] & (1 << (nr & 0x1f)))
+ return 1;
+
+ return ichx_read_bit(GPIO_USE_SEL, nr) ? 0 : -ENODEV;
+}
+
+static int ich6_gpio_request(struct gpio_chip *chip, unsigned nr)
+{
+ /*
+ * Fixups for bits 16 and 17 are necessary on the Intel ICH6/3100
+ * brdige as they are controlled by USE register bits 0 and 1. See
+ * "Table 704 GPIO_USE_SEL1 register" in the i3100 datasheet for
+ * additional info.
+ */
+ if ((nr == 16) || (nr == 17))
+ nr -= 16;
+
+ return ichx_gpio_request(chip, nr);
+}
+
+static void ichx_gpio_set(struct gpio_chip *chip, unsigned nr, int val)
+{
+ ichx_write_bit(GPIO_LVL, nr, val, 0);
+}
+
+static void __devinit ichx_gpiolib_setup(struct gpio_chip *chip)
+{
+ chip->owner = THIS_MODULE;
+ chip->label = DRV_NAME;
+
+ /* Allow chip-specific overrides of request()/get() */
+ chip->request = ichx_priv.desc->request ?
+ ichx_priv.desc->request : ichx_gpio_request;
+ chip->get = ichx_priv.desc->get ?
+ ichx_priv.desc->get : ichx_gpio_get;
+
+ chip->set = ichx_gpio_set;
+ chip->get_direction = ichx_gpio_get_direction;
+ chip->direction_input = ichx_gpio_direction_input;
+ chip->direction_output = ichx_gpio_direction_output;
+ chip->base = modparam_gpiobase;
+ chip->ngpio = ichx_priv.desc->ngpio;
+ chip->can_sleep = 0;
+ chip->dbg_show = NULL;
+}
+
+static int __devinit ichx_gpio_init(struct pci_dev *pdev,
+ const struct pci_device_id *ent,
+ struct platform_device *dev)
+{
+ int err;
+
+ ichx_priv.pdev = pdev;
+ ichx_priv.dev = dev;
+ ichx_priv.desc = (struct ichx_desc *)ent->driver_data;
+
+ /* Determine I/O address of GPIO registers */
+ pci_read_config_dword(pdev, PCI_ICHX_GPIO_BAR,
+ (u32 *)&ichx_priv.gpio_base);
+ ichx_priv.gpio_base &= PCI_BASE_ADDRESS_IO_MASK;
+ if (!ichx_priv.gpio_base) {
+ printk(KERN_ERR "%s: GPIO BAR not enumerated\n", DRV_NAME);
+ return -ENODEV;
+ }
+
+ /*
+ * If necessary, determine the I/O address of ACPI/power management
+ * registers which are needed to read the the GPE0 register for GPI pins
+ * 0 - 15 on some chipsets.
+ */
+ if (ichx_priv.desc->gpe0_sts_ofs) {
+ pci_read_config_dword(pdev, PCI_ICHX_ACPI_BAR,
+ (u32 *)&ichx_priv.pm_base);
+ ichx_priv.pm_base &= PCI_BASE_ADDRESS_IO_MASK;
+ if (!ichx_priv.pm_base)
+ printk(KERN_WARNING "%s: ACPI BAR not enumerated, "
+ "GPI 0 - 15 unusable\n", DRV_NAME);
+ }
+
+ /* Enable GPIO */
+ pci_read_config_dword(pdev, PCI_ICHX_GPIO_CTRL,
+ &ichx_priv.orig_gpio_ctrl);
+ pci_write_config_dword(pdev, PCI_ICHX_GPIO_CTRL,
+ ichx_priv.orig_gpio_ctrl | 0x10);
+
+ /*
+ * Reserve GPIO I/O region. The power management region is used by other
+ * drivers thus shouldn't be reserved. The GPIO I/O region is 64
+ * bytes for older chipsets with <= 64 GPIO, 128 bytes for newer
+ * chipsets with > 64 GPIO.
+ */
+ if (!devm_request_region(&dev->dev, ichx_priv.gpio_base,
+ ichx_priv.desc->ngpio > 64 ? 128 : 64,
+ "ichxgpio")) {
+ printk(KERN_ERR "%s: Can't request iomem (0x%lx)\n",
+ DRV_NAME, ichx_priv.gpio_base);
+ return -EBUSY;
+ }
+
+ ichx_gpiolib_setup(&ichx_priv.chip);
+
+ err = gpiochip_add(&ichx_priv.chip);
+ if (err) {
+ printk(KERN_ERR "%s: Failed to register GPIOs\n", DRV_NAME);
+ return err;
+ }
+
+ printk(KERN_INFO "%s: GPIO from %d to %d on %s\n", DRV_NAME,
+ ichx_priv.chip.base,
+ ichx_priv.chip.base + ichx_priv.chip.ngpio - 1,
+ dev_name(&pdev->dev));
+
+ return 0;
+}
+
+/* ICH6-based, 631xesb-based */
+static struct ichx_desc ich6_desc = {
+ /* Bridges using the ICH6 controller need fixups for GPIO 0 - 17 */
+ .request = ich6_gpio_request,
+ .get = ich6_gpio_get,
+
+ /* GPIO 0-15 are read in the GPE0_STS PM register */
+ .gpe0_sts_ofs = 0x28,
+
+ .ngpio = 50,
+};
+
+/* Intel 3100 */
+static struct ichx_desc i3100_desc = {
+ /*
+ * Bits 16,17, 20 of USE_SEL and bit 16 of USE_SEL2 always read 0 on
+ * the Intel 3100. See "Table 712. GPIO Summary Table" of 3100
+ * Datasheet for more info.
+ */
+ .use_sel_ignore = {0x00130000, 0x00010000, 0x0},
+
+ /* The 3100 needs fixups for GPIO 0 - 17 */
+ .request = ich6_gpio_request,
+ .get = ich6_gpio_get,
+
+ /* GPIO 0-15 are read in the GPE0_STS PM register */
+ .gpe0_sts_ofs = 0x28,
+
+ .ngpio = 50,
+};
+
+/* ICH7 and ICH8-based */
+static struct ichx_desc ich7_desc = {
+ .ngpio = 50,
+};
+
+/* ICH9-based */
+static struct ichx_desc ich9_desc = {
+ .ngpio = 61,
+};
+
+/* ICH10-based - Consumer/corporate versions have different amount of GPIO */
+static struct ichx_desc ich10_cons_desc = {
+ .ngpio = 51,
+};
+static struct ichx_desc ich10_corp_desc = {
+ .ngpio = 72,
+};
+
+/* Intel 5 series, 6 series, 3400 series, and C200 series */
+static struct ichx_desc intel5_desc = {
+ .ngpio = 76,
+};
+
+/*
+ * Note that we don't register a PCI driver since the PCI device has additional
+ * functionality that is used by other drivers, eg iTCO_wdt. We use the table
+ * to probe for matching devices, then use a platform driver.
+ *
+ * Also note that some additional, old chipsets should in theory be supported
+ * by this driver (eg 82801XX chips), but they are left out due to the fact
+ * that they complicate the driver and there likely isn't much of a demand
+ * since a driver doesn't already exist for the 10+ years the chipsets have
+ * existed for.
+ */
+static DEFINE_PCI_DEVICE_TABLE(ichx_pci_tbl) = {
+ { ICHX_GPIO_PCI_DEVICE(PCI_DEVICE_ID_INTEL_ICH6_0, &ich6_desc) },
+ { ICHX_GPIO_PCI_DEVICE(PCI_DEVICE_ID_INTEL_ICH6_1, &ich6_desc) },
+ { ICHX_GPIO_PCI_DEVICE(PCI_DEVICE_ID_INTEL_ICH6_2, &ich6_desc) },
+ { ICHX_GPIO_PCI_DEVICE(PCI_DEVICE_ID_INTEL_ICH7_0, &ich7_desc) },
+ { ICHX_GPIO_PCI_DEVICE(PCI_DEVICE_ID_INTEL_ICH7_1, &ich7_desc) },
+ { ICHX_GPIO_PCI_DEVICE(PCI_DEVICE_ID_INTEL_ICH7_30, &ich7_desc) },
+ { ICHX_GPIO_PCI_DEVICE(PCI_DEVICE_ID_INTEL_ICH7_31, &ich7_desc) },
+ { ICHX_GPIO_PCI_DEVICE(PCI_DEVICE_ID_INTEL_ICH8_0, &ich7_desc) },
+ { ICHX_GPIO_PCI_DEVICE(PCI_DEVICE_ID_INTEL_ICH8_1, &ich7_desc) },
+ { ICHX_GPIO_PCI_DEVICE(PCI_DEVICE_ID_INTEL_ICH8_2, &ich7_desc) },
+ { ICHX_GPIO_PCI_DEVICE(PCI_DEVICE_ID_INTEL_ICH8_3, &ich7_desc) },
+ { ICHX_GPIO_PCI_DEVICE(PCI_DEVICE_ID_INTEL_ICH8_4, &ich7_desc) },
+ { ICHX_GPIO_PCI_DEVICE(PCI_DEVICE_ID_INTEL_ICH9_1, &ich9_desc) },
+ { ICHX_GPIO_PCI_DEVICE(PCI_DEVICE_ID_INTEL_ICH9_2, &ich9_desc) },
+ { ICHX_GPIO_PCI_DEVICE(PCI_DEVICE_ID_INTEL_ICH9_4, &ich9_desc) },
+ { ICHX_GPIO_PCI_DEVICE(PCI_DEVICE_ID_INTEL_ICH9_5, &ich9_desc) },
+ { ICHX_GPIO_PCI_DEVICE(PCI_DEVICE_ID_INTEL_ICH9_7, &ich9_desc) },
+ { ICHX_GPIO_PCI_DEVICE(PCI_DEVICE_ID_INTEL_ICH9_8, &ich9_desc) },
+ { ICHX_GPIO_PCI_DEVICE(PCI_DEVICE_ID_INTEL_ICH10_0, &ich10_corp_desc) },
+ { ICHX_GPIO_PCI_DEVICE(PCI_DEVICE_ID_INTEL_ICH10_3, &ich10_corp_desc) },
+ { ICHX_GPIO_PCI_DEVICE(PCI_DEVICE_ID_INTEL_ICH10_1, &ich10_cons_desc) },
+ { ICHX_GPIO_PCI_DEVICE(PCI_DEVICE_ID_INTEL_ICH10_2, &ich10_cons_desc) },
+ { ICHX_GPIO_PCI_DEVICE(PCI_DEVICE_ID_INTEL_ESB2_0, &i3100_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x2671, &ich6_desc) }, /* 631Xesb series */
+ { ICHX_GPIO_PCI_DEVICE(0x2672, &ich6_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x2673, &ich6_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x2674, &ich6_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x2675, &ich6_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x2676, &ich6_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x2677, &ich6_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x2678, &ich6_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x2679, &ich6_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x267a, &ich6_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x267b, &ich6_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x267c, &ich6_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x267d, &ich6_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x267e, &ich6_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x267f, &ich6_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x3b00, &intel5_desc) }, /* 5 series */
+ { ICHX_GPIO_PCI_DEVICE(0x3b01, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x3b02, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x3b03, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x3b04, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x3b05, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x3b06, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x3b07, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x3b08, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x3b09, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x3b0a, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x3b0b, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x3b0c, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x3b0d, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x3b0e, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x3b0f, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x1c41, &intel5_desc) }, /* 6 Series */
+ { ICHX_GPIO_PCI_DEVICE(0x1c42, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x1c43, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x1c44, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x1c45, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x1c46, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x1c47, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x1c48, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x1c49, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x1c4a, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x1c4b, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x1c4c, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x1c4d, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x1c4e, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x1c4f, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x1c50, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x1c51, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x1c52, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x1c53, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x1c54, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x1c55, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x1c56, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x1c57, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x1c58, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x1c59, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x1c5a, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x1c5b, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x1c5c, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x1c5d, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x1c5e, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x1c5f, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x3b12, &intel5_desc) }, /* 3400 series */
+ { ICHX_GPIO_PCI_DEVICE(0x3b14, &intel5_desc) },
+ { ICHX_GPIO_PCI_DEVICE(0x3b16, &intel5_desc) },
+ { 0, },
+};
+MODULE_DEVICE_TABLE(pci, ichx_pci_tbl);
+
+static int __devinit ichx_gpio_probe(struct platform_device *dev)
+{
+ struct pci_dev *pdev = NULL;
+ const struct pci_device_id *ent;
+
+ spin_lock_init(&ichx_priv.lock);
+
+ for_each_pci_dev(pdev) {
+ ent = pci_match_id(ichx_pci_tbl, pdev);
+ if (ent)
+ return ichx_gpio_init(pdev, ent, dev);
+ }
+
+ return -ENODEV;
+}
+
+static int __devexit ichx_gpio_remove(struct platform_device *dev)
+{
+ int ret = gpiochip_remove(&ichx_priv.chip);
+
+ /* Restore original GPIO control register value */
+ pci_write_config_dword(ichx_priv.pdev, PCI_ICHX_GPIO_CTRL,
+ ichx_priv.orig_gpio_ctrl);
+
+ return ret;
+}
+
+static struct platform_driver ichx_gpio_driver = {
+ .driver = {
+ .owner = THIS_MODULE,
+ .name = DRV_NAME,
+ },
+ .probe = ichx_gpio_probe,
+ .remove = __devexit_p(ichx_gpio_remove),
+};
+
+static int __devinit ichx_gpio_init_module(void)
+{
+ int err = platform_driver_register(&ichx_gpio_driver);
+ if (err)
+ return err;
+
+ ichx_gpio_platform_device =
+ platform_device_register_simple(DRV_NAME, -1, NULL, 0);
+
+ if (IS_ERR(ichx_gpio_platform_device)) {
+ err = PTR_ERR(ichx_gpio_platform_device);
+ goto unreg_platform_driver;
+ }
+
+ return 0;
+
+unreg_platform_driver:
+ platform_driver_unregister(&ichx_gpio_driver);
+ return err;
+}
+
+static void __devexit ichx_gpio_exit_module(void)
+{
+ platform_device_unregister(ichx_gpio_platform_device);
+ platform_driver_unregister(&ichx_gpio_driver);
+}
+
+module_init(ichx_gpio_init_module);
+module_exit(ichx_gpio_exit_module);
+
+MODULE_AUTHOR("Peter Tyser <[email protected]>");
+MODULE_DESCRIPTION("Intel ICHx series bridge GPIO driver");
+MODULE_LICENSE("GPL");
--
1.7.0.4
On Thu, Feb 17, 2011 at 8:56 AM, Peter Tyser <[email protected]> wrote:
> Previously, gpiolib would unconditionally flag all GPIO pins as inputs,
> regardless of their true state. This resulted in all GPIO output pins
> initially being incorrectly identified as "input" in the GPIO sysfs.
>
> Since the direction of GPIOs is not known prior to having their
> direction set, instead set the default direction to "unknown" to prevent
> user confusion. A pin with an "unknown" direction can not be written or
> read via sysfs; it must first be configured as an input or output before
> it can be used.
>
Hrm... that's why I don't like the original definition of gpio_request()
which is vague on the pin configurations. The pin configuration should
be clear upon requesting, otherwise it's a potential issue.
Anyway, this unknown state looks to be a good mitigation to this
underlying problem. I'm good with it.
> While we're playing with the direction flag in/out defines, rename them to
> a more descriptive FLAG_DIR_* format.
>
> Cc: Alek Du <[email protected]>
> Cc: Samuel Ortiz <[email protected]>
> Cc: David Brownell <[email protected]>
> Cc: Eric Miao <[email protected]>
> Cc: Uwe Kleine-K?nig <[email protected]>
> Cc: Mark Brown <[email protected]>
> Cc: Joe Perches <[email protected]>
> Cc: Alan Cox <[email protected]>
> Cc: Grant Likely <[email protected]>
> Signed-off-by: Peter Tyser <[email protected]>
> ---
> Change since V1:
> - This patch is new and is based on feedback from Alan to support a new
> "unknown" direction.
> - Rename FLAG_IS_OUT to FLAG_DIR_OUT, and add FLAG_DIR_IN
>
> drivers/gpio/gpiolib.c | 82 +++++++++++++++++++++++++++--------------------
> 1 files changed, 47 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 649550e..0113c10 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -49,13 +49,14 @@ struct gpio_desc {
> unsigned long flags;
> /* flag symbols are bit numbers */
> #define FLAG_REQUESTED 0
> -#define FLAG_IS_OUT 1
> -#define FLAG_RESERVED 2
> -#define FLAG_EXPORT 3 /* protected by sysfs_lock */
> -#define FLAG_SYSFS 4 /* exported via /sys/class/gpio/control */
> -#define FLAG_TRIG_FALL 5 /* trigger on falling edge */
> -#define FLAG_TRIG_RISE 6 /* trigger on rising edge */
> -#define FLAG_ACTIVE_LOW 7 /* sysfs value has active low */
> +#define FLAG_DIR_OUT 1 /* pin is an output */
> +#define FLAG_DIR_IN 2 /* pin is an input */
> +#define FLAG_RESERVED 3
> +#define FLAG_EXPORT 4 /* protected by sysfs_lock */
> +#define FLAG_SYSFS 5 /* exported via /sys/class/gpio/control */
> +#define FLAG_TRIG_FALL 6 /* trigger on falling edge */
> +#define FLAG_TRIG_RISE 7 /* trigger on rising edge */
> +#define FLAG_ACTIVE_LOW 8 /* sysfs value has active low */
>
> #define ID_SHIFT 16 /* add new flags before this one */
>
> @@ -220,15 +221,22 @@ static ssize_t gpio_direction_show(struct device *dev,
> {
> const struct gpio_desc *desc = dev_get_drvdata(dev);
> ssize_t status;
> + const char *dir_str;
>
> mutex_lock(&sysfs_lock);
>
> - if (!test_bit(FLAG_EXPORT, &desc->flags))
> + if (!test_bit(FLAG_EXPORT, &desc->flags)) {
> status = -EIO;
> - else
> - status = sprintf(buf, "%s\n",
> - test_bit(FLAG_IS_OUT, &desc->flags)
> - ? "out" : "in");
> + } else {
> + if (test_bit(FLAG_DIR_OUT, &desc->flags))
> + dir_str = "out";
> + else if (test_bit(FLAG_DIR_IN, &desc->flags))
> + dir_str = "in";
> + else
> + dir_str = "unknown";
> +
> + status = sprintf(buf, "%s\n", dir_str);
> + }
>
> mutex_unlock(&sysfs_lock);
> return status;
> @@ -272,6 +280,9 @@ static ssize_t gpio_value_show(struct device *dev,
>
> if (!test_bit(FLAG_EXPORT, &desc->flags)) {
> status = -EIO;
> + } else if ((!test_bit(FLAG_DIR_OUT, &desc->flags)) &&
> + (!test_bit(FLAG_DIR_IN, &desc->flags))) {
> + status = -EPERM;
> } else {
> int value;
>
> @@ -297,7 +308,7 @@ static ssize_t gpio_value_store(struct device *dev,
>
> if (!test_bit(FLAG_EXPORT, &desc->flags))
> status = -EIO;
> - else if (!test_bit(FLAG_IS_OUT, &desc->flags))
> + else if (!test_bit(FLAG_DIR_OUT, &desc->flags))
> status = -EPERM;
> else {
> long value;
> @@ -741,7 +752,7 @@ int gpio_export(unsigned gpio, bool direction_may_change)
>
> if (!status && gpio_to_irq(gpio) >= 0
> && (direction_may_change
> - || !test_bit(FLAG_IS_OUT,
> + || test_bit(FLAG_DIR_IN,
> &desc->flags)))
> status = device_create_file(dev,
> &dev_attr_edge);
> @@ -1059,22 +1070,10 @@ int gpiochip_add(struct gpio_chip *chip)
> break;
> }
> }
> - if (status == 0) {
> - for (id = base; id < base + chip->ngpio; id++) {
> + if (status == 0)
> + for (id = base; id < base + chip->ngpio; id++)
> gpio_desc[id].chip = chip;
>
> - /* REVISIT: most hardware initializes GPIOs as
> - * inputs (often with pullups enabled) so power
> - * usage is minimized. Linux code should set the
> - * gpio direction first thing; but until it does,
> - * we may expose the wrong direction in sysfs.
> - */
> - gpio_desc[id].flags = !chip->direction_input
> - ? (1 << FLAG_IS_OUT)
> - : 0;
> - }
> - }
> -
> of_gpiochip_add(chip);
>
> unlock:
> @@ -1402,8 +1401,10 @@ int gpio_direction_input(unsigned gpio)
> }
>
> status = chip->direction_input(chip, gpio);
> - if (status == 0)
> - clear_bit(FLAG_IS_OUT, &desc->flags);
> + if (status == 0) {
> + clear_bit(FLAG_DIR_OUT, &desc->flags);
> + set_bit(FLAG_DIR_IN, &desc->flags);
> + }
> lose:
> return status;
> fail:
> @@ -1455,8 +1456,10 @@ int gpio_direction_output(unsigned gpio, int value)
> }
>
> status = chip->direction_output(chip, gpio, value);
> - if (status == 0)
> - set_bit(FLAG_IS_OUT, &desc->flags);
> + if (status == 0) {
> + clear_bit(FLAG_DIR_IN, &desc->flags);
> + set_bit(FLAG_DIR_OUT, &desc->flags);
> + }
> lose:
> return status;
> fail:
> @@ -1643,21 +1646,27 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)
> unsigned i;
> unsigned gpio = chip->base;
> struct gpio_desc *gdesc = &gpio_desc[gpio];
> - int is_out;
> + const char *dir_str;
> + int unknown = 0;
>
> for (i = 0; i < chip->ngpio; i++, gpio++, gdesc++) {
> if (!test_bit(FLAG_REQUESTED, &gdesc->flags))
> continue;
>
> - is_out = test_bit(FLAG_IS_OUT, &gdesc->flags);
> - seq_printf(s, " gpio-%-3d (%-20.20s) %s %s",
> - gpio, gdesc->label,
> - is_out ? "out" : "in ",
> - chip->get
> - ? (chip->get(chip, i) ? "hi" : "lo")
> - : "? ");
> + if (test_bit(FLAG_DIR_IN, &gdesc->flags)) {
> + dir_str = "in ";
> + } else if (test_bit(FLAG_DIR_OUT, &gdesc->flags)) {
> + dir_str = "out";
> + } else {
> + dir_str = "? ";
> + unknown = 1;
> + }
> +
> + seq_printf(s, " gpio-%-3d (%-20.20s) %s %s", gpio, gdesc->label,
> + dir_str, (chip->get && !unknown) ?
> + (chip->get(chip, i) ? "hi" : "lo") : "?");
>
> - if (!is_out) {
> + if (test_bit(FLAG_DIR_IN, &gdesc->flags)) {
> int irq = gpio_to_irq(gpio);
> struct irq_desc *desc = irq_to_desc(irq);
>
> --
> 1.7.0.4
>
>
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Wed, Feb 16, 2011 at 06:56:54PM -0600, Peter Tyser wrote:
> Add a new get_direction() function to the gpio_chip structure. This is
> useful so that the direction of a pin can be determined when its
> initially exported. Previously, the direction defaulted to "unknown"
> regardless of the actual configuration of the GPIO pin.
>
> If a GPIO driver implements get_direction(), it is called in
> gpio_request() to set the initial direction of the pin accurately.
>
> Cc: Alek Du <[email protected]>
> Cc: Samuel Ortiz <[email protected]>
> Cc: David Brownell <[email protected]>
> Cc: Eric Miao <[email protected]>
> Cc: Uwe Kleine-K?nig <[email protected]>
> Cc: Mark Brown <[email protected]>
> Cc: Joe Perches <[email protected]>
> Cc: Alan Cox <[email protected]>
> Cc: Grant Likely <[email protected]>
> Signed-off-by: Peter Tyser <[email protected]>
> ---
> Changes since v1:
> - Add support for "unknown" direction
>
> drivers/gpio/gpiolib.c | 20 ++++++++++++++++++++
> include/asm-generic/gpio.h | 4 ++++
> 2 files changed, 24 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 0113c10..7723beb 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1214,6 +1214,26 @@ int gpio_request(unsigned gpio, const char *label)
> }
> }
>
> + if (chip->get_direction) {
> + /* chip->get_direction may sleep */
> + spin_unlock_irqrestore(&gpio_lock, flags);
> + switch (chip->get_direction(chip, gpio - chip->base)) {
> + case -1:
Use some #defines instead of hardcoded values?
> + clear_bit(FLAG_DIR_OUT, &desc->flags);
> + clear_bit(FLAG_DIR_IN, &desc->flags);
> + break;
> + case 0:
> + set_bit(FLAG_DIR_IN, &desc->flags);
> + clear_bit(FLAG_DIR_OUT, &desc->flags);
> + break;
> + case 1:
> + set_bit(FLAG_DIR_OUT, &desc->flags);
> + clear_bit(FLAG_DIR_IN, &desc->flags);
> + break;
> + }
> + spin_lock_irqsave(&gpio_lock, flags);
You are changing the flags without the lock. I think it would be good to copy
the locking-pattern of chip->request for now.
Regards,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Thu, Feb 17, 2011 at 03:33:42PM +0800, Eric Miao wrote:
> On Thu, Feb 17, 2011 at 8:56 AM, Peter Tyser <[email protected]> wrote:
> > Previously, gpiolib would unconditionally flag all GPIO pins as inputs,
> > regardless of their true state. ?This resulted in all GPIO output pins
> > initially being incorrectly identified as "input" in the GPIO sysfs.
> >
> > Since the direction of GPIOs is not known prior to having their
> > direction set, instead set the default direction to "unknown" to prevent
> > user confusion. ?A pin with an "unknown" direction can not be written or
> > read via sysfs; it must first be configured as an input or output before
> > it can be used.
> >
>
> Hrm... that's why I don't like the original definition of gpio_request()
> which is vague on the pin configurations. The pin configuration should
> be clear upon requesting, otherwise it's a potential issue.
I don't know (state: "unknown" :-) what you would prefer here. An
implicit gpio_direction_input()? Or gpio_reserve_and_direction_input()?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On 02/17/2011 08:33 AM, Eric Miao wrote:
> On Thu, Feb 17, 2011 at 8:56 AM, Peter Tyser <[email protected]> wrote:
>> Previously, gpiolib would unconditionally flag all GPIO pins as inputs,
>> regardless of their true state. This resulted in all GPIO output pins
>> initially being incorrectly identified as "input" in the GPIO sysfs.
>>
>> Since the direction of GPIOs is not known prior to having their
>> direction set, instead set the default direction to "unknown" to prevent
>> user confusion. A pin with an "unknown" direction can not be written or
>> read via sysfs; it must first be configured as an input or output before
>> it can be used.
>>
>
> Hrm... that's why I don't like the original definition of gpio_request()
> which is vague on the pin configurations.
Actually it doesn't say anything at all about the current configuration at all.
Requesting a pin grants you exclusive access to that pin, if it succeeds. So it
is solely about ownership and not about configuration.
Once you own a pin you are allowed to modify its configuration, otherwise you
should never touch it. And you shouldn't make any assumptions about its
previous configuration, if you want to use it as input you should explicitly
configure it as an input pin.
> The pin configuration should be clear upon requesting, otherwise it's a
> potential issue.
How so?
>
> Anyway, this unknown state looks to be a good mitigation to this
> underlying problem. I'm good with it.
>
- Lars
On Thursday 17 February 2011, 13:03:54 Lars-Peter Clausen wrote:
> On 02/17/2011 08:33 AM, Eric Miao wrote:
> > On Thu, Feb 17, 2011 at 8:56 AM, Peter Tyser <[email protected]> wrote:
> >> Previously, gpiolib would unconditionally flag all GPIO pins as inputs,
> >> regardless of their true state. This resulted in all GPIO output pins
> >> initially being incorrectly identified as "input" in the GPIO sysfs.
> >>
> >> Since the direction of GPIOs is not known prior to having their
> >> direction set, instead set the default direction to "unknown" to prevent
> >> user confusion. A pin with an "unknown" direction can not be written or
> >> read via sysfs; it must first be configured as an input or output before
> >> it can be used.
> >
> > Hrm... that's why I don't like the original definition of gpio_request()
> > which is vague on the pin configurations.
>
> Actually it doesn't say anything at all about the current configuration at
> all. Requesting a pin grants you exclusive access to that pin, if it
> succeeds. So it is solely about ownership and not about configuration.
Well, ownership is a bit misleading here. You must request a GPIO to change
its direction. But to set or get a value this isn't required.
In general one could expect if you requested a GPIO you are the only one to
call any function on it. On the other hand, this may be bad in some situations
where you want to read a GPIO value from different places.
Alexander
On 02/17/2011 01:21 PM, Alexander Stein wrote:
> On Thursday 17 February 2011, 13:03:54 Lars-Peter Clausen wrote:
>> On 02/17/2011 08:33 AM, Eric Miao wrote:
>>> On Thu, Feb 17, 2011 at 8:56 AM, Peter Tyser <[email protected]> wrote:
>>>> Previously, gpiolib would unconditionally flag all GPIO pins as inputs,
>>>> regardless of their true state. This resulted in all GPIO output pins
>>>> initially being incorrectly identified as "input" in the GPIO sysfs.
>>>>
>>>> Since the direction of GPIOs is not known prior to having their
>>>> direction set, instead set the default direction to "unknown" to prevent
>>>> user confusion. A pin with an "unknown" direction can not be written or
>>>> read via sysfs; it must first be configured as an input or output before
>>>> it can be used.
>>>
>>> Hrm... that's why I don't like the original definition of gpio_request()
>>> which is vague on the pin configurations.
>>
>> Actually it doesn't say anything at all about the current configuration at
>> all. Requesting a pin grants you exclusive access to that pin, if it
>> succeeds. So it is solely about ownership and not about configuration.
>
> Well, ownership is a bit misleading here. You must request a GPIO to change
> its direction. But to set or get a value this isn't required.
Well, it is not enforced in the actual code, but the GPIO API is based on
convention and I would consider it a misusage of the API to call
gpio_{set,get}_value without requesting the pin and having configured its
direction prior to it.
> In general one could expect if you requested a GPIO you are the only one to
> call any function on it. On the other hand, this may be bad in some situations
> where you want to read a GPIO value from different places.
Sharing GPIOs in read-only mode, is indeed something that is not covered by the
GPIO API. It might be worth adding a gpio_request_shared, which would only
permit setting the direction to input. Futher gpio_request_shared calls would
be allowed but gpio_request calls would fail.
- Lars
On Thursday 17 February 2011, 14:06:06 Lars-Peter Clausen wrote:
> > Well, ownership is a bit misleading here. You must request a GPIO to
> > change its direction. But to set or get a value this isn't required.
>
> Well, it is not enforced in the actual code, but the GPIO API is based on
> convention and I would consider it a misusage of the API to call
> gpio_{set,get}_value without requesting the pin and having configured its
> direction prior to it.
>
> > In general one could expect if you requested a GPIO you are the only one
> > to call any function on it. On the other hand, this may be bad in some
> > situations where you want to read a GPIO value from different places.
>
> Sharing GPIOs in read-only mode, is indeed something that is not covered by
> the GPIO API. It might be worth adding a gpio_request_shared, which would
> only permit setting the direction to input. Futher gpio_request_shared
> calls would be allowed but gpio_request calls would fail.
gpio_request_shared sounds interesting, but in this case an implicit
gpio_direction_input call has to be done in the first shared request. But the
user must/should not call gpio_direction_input himself which is quite a bit
different than using a normal gpio_requested gpio.
Alexander
> > Sharing GPIOs in read-only mode, is indeed something that is not covered by
> > the GPIO API. It might be worth adding a gpio_request_shared, which would
> > only permit setting the direction to input. Futher gpio_request_shared
> > calls would be allowed but gpio_request calls would fail.
>
> gpio_request_shared sounds interesting, but in this case an implicit
Can you name a use-case? One reason is that we won't need to implement
it if there is no user, another one is that it could potentially weaken
abstractions?
Regards,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Monday 21 February 2011, 10:19:16 Wolfram Sang wrote:
> > > Sharing GPIOs in read-only mode, is indeed something that is not
> > > covered by the GPIO API. It might be worth adding a
> > > gpio_request_shared, which would only permit setting the direction to
> > > input. Futher gpio_request_shared calls would be allowed but
> > > gpio_request calls would fail.
> >
> > gpio_request_shared sounds interesting, but in this case an implicit
>
> Can you name a use-case? One reason is that we won't need to implement
> it if there is no user, another one is that it could potentially weaken
> abstractions?
We had exported our 5V_enable gpio to sysfs to allow a user-space application
to enable/disable devices connected to 5V circuit.
But on the other hand we had to read the current status of this gpio in the
power-fail interrupt handler to distinguish between false-positive (5V
disabled) and a correct detection.
As the sysfs export requests the gpio we cannot gpio_request it in the power-
fail driver and just used gpio_get_value without request.
We set the direction already in the machine startup code and never touched it
again.
Alexander
> We had exported our 5V_enable gpio to sysfs to allow a user-space application
> to enable/disable devices connected to 5V circuit.
> But on the other hand we had to read the current status of this gpio in the
> power-fail interrupt handler to distinguish between false-positive (5V
> disabled) and a correct detection.
What about gpio_export() (description in Documentation/gpio.txt)?
Regards,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Monday 21 February 2011, 10:47:56 Wolfram Sang wrote:
> > We had exported our 5V_enable gpio to sysfs to allow a user-space
> > application to enable/disable devices connected to 5V circuit.
> > But on the other hand we had to read the current status of this gpio in
> > the power-fail interrupt handler to distinguish between false-positive
> > (5V disabled) and a correct detection.
>
> What about gpio_export() (description in Documentation/gpio.txt)?
Ah, I didn't know about this. I just expected this is only used from sysfs
part. But you have to make sure your .ko is loaded before userspace is
accessing sysfs and tries to export the GPIO.
Or is it "allowed" by the API convention to gpio_request and gpio_export (and
set direction) a GPIO in the machine startup code which will later be used in
a different place?
Alexander
On Mon, Feb 21, 2011 at 12:07:27PM +0100, Alexander Stein wrote:
> On Monday 21 February 2011, 10:47:56 Wolfram Sang wrote:
> > > We had exported our 5V_enable gpio to sysfs to allow a user-space
> > > application to enable/disable devices connected to 5V circuit.
> > > But on the other hand we had to read the current status of this gpio in
> > > the power-fail interrupt handler to distinguish between false-positive
> > > (5V disabled) and a correct detection.
> >
> > What about gpio_export() (description in Documentation/gpio.txt)?
>
> Ah, I didn't know about this. I just expected this is only used from sysfs
> part. But you have to make sure your .ko is loaded before userspace is
> accessing sysfs and tries to export the GPIO.
Eh? Userspace doesn't export the GPIO in that case.
> Or is it "allowed" by the API convention to gpio_request and gpio_export (and
> set direction) a GPIO in the machine startup code which will later be used in
> a different place?
different place = userspace? Well, that's the main intention of
gpio_export(). (I have the feeling we are missing each other here,
thoguh) I'd suggest looking a bit further in the docs/code. It should
make clear what is possible.
Regards,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Monday 21 February 2011, 12:22:40 Wolfram Sang wrote:
> On Mon, Feb 21, 2011 at 12:07:27PM +0100, Alexander Stein wrote:
> > On Monday 21 February 2011, 10:47:56 Wolfram Sang wrote:
> > > > We had exported our 5V_enable gpio to sysfs to allow a user-space
> > > > application to enable/disable devices connected to 5V circuit.
> > > > But on the other hand we had to read the current status of this gpio
> > > > in the power-fail interrupt handler to distinguish between
> > > > false-positive (5V disabled) and a correct detection.
> > >
> > > What about gpio_export() (description in Documentation/gpio.txt)?
> >
> > Ah, I didn't know about this. I just expected this is only used from
> > sysfs part. But you have to make sure your .ko is loaded before
> > userspace is accessing sysfs and tries to export the GPIO.
>
> Eh? Userspace doesn't export the GPIO in that case.
Sure, but you have to make sure you have it exported or userspace will fail.
> > Or is it "allowed" by the API convention to gpio_request and gpio_export
> > (and set direction) a GPIO in the machine startup code which will later
> > be used in a different place?
>
> different place = userspace? Well, that's the main intention of
> gpio_export(). (I have the feeling we are missing each other here,
> thoguh) I'd suggest looking a bit further in the docs/code. It should
> make clear what is possible.
No, with different place I mean a kernel module driver which will be loaded
later using insmod. In this case this module is just expecting the GPIO has
already be exported and set proper direction without requesting the GPIO
itself.
Alexander
On 02/22/2011 12:38 AM, Alexander Stein wrote:
> On Monday 21 February 2011, 12:22:40 Wolfram Sang wrote:
>> On Mon, Feb 21, 2011 at 12:07:27PM +0100, Alexander Stein wrote:
>>> On Monday 21 February 2011, 10:47:56 Wolfram Sang wrote:
>>>>> We had exported our 5V_enable gpio to sysfs to allow a user-space
>>>>> application to enable/disable devices connected to 5V circuit.
>>>>> But on the other hand we had to read the current status of this gpio
>>>>> in the power-fail interrupt handler to distinguish between
>>>>> false-positive (5V disabled) and a correct detection.
>>>>
>>>> What about gpio_export() (description in Documentation/gpio.txt)?
>>>
>>> Ah, I didn't know about this. I just expected this is only used from
>>> sysfs part. But you have to make sure your .ko is loaded before
>>> userspace is accessing sysfs and tries to export the GPIO.
>>
>> Eh? Userspace doesn't export the GPIO in that case.
>
> Sure, but you have to make sure you have it exported or userspace will fail.
>
>>> Or is it "allowed" by the API convention to gpio_request and gpio_export
>>> (and set direction) a GPIO in the machine startup code which will later
>>> be used in a different place?
>>
>> different place = userspace? Well, that's the main intention of
>> gpio_export(). (I have the feeling we are missing each other here,
>> thoguh) I'd suggest looking a bit further in the docs/code. It should
>> make clear what is possible.
>
> No, with different place I mean a kernel module driver which will be loaded
> later using insmod. In this case this module is just expecting the GPIO has
> already be exported and set proper direction without requesting the GPIO
> itself.
If the module does the gpio_request, then it should also do the
gpio_export (and gpio_unexport/gpio_free on module exit). In this case
when the module is not loaded the gpio is not in use by the kernel and
user space is able to access it via sysfs. When the module is in use the
gpio_export call will make the gpio available via sysfs.
If the gpio_request is done in your platform code, then the gpio_export
should also go in the platform code. This way the gpio is always in use
by the kernel, but also exported to userspace so it will be available
via sysfs.
~Ryan
--
Bluewater Systems Ltd - ARM Technology Solution Centre
Ryan Mallon 5 Amuri Park, 404 Barbadoes St
[email protected] PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com New Zealand
Phone: +64 3 3779127 Freecall: Australia 1800 148 751
Fax: +64 3 3779135 USA 1800 261 2934