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
Change since V2:
- None
drivers/gpio/gpiolib.c | 91 ++++++++++++++++++++++++++---------------------
1 files changed, 50 insertions(+), 41 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 649550e..eb74311 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
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
Changes since v2:
- Use GPIOF_DIR_* defines
drivers/gpio/pca953x.c | 10 ++++++++++
1 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/drivers/gpio/pca953x.c b/drivers/gpio/pca953x.c
index b473429..b93611f 100644
--- a/drivers/gpio/pca953x.c
+++ b/drivers/gpio/pca953x.c
@@ -168,6 +168,15 @@ 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 chip->reg_direction & (1u << off) ? GPIOF_DIR_IN : GPIOF_DIR_OUT;
+}
+
static int pca953x_gpio_get_value(struct gpio_chip *gc, unsigned off)
{
struct pca953x_chip *chip;
@@ -221,6 +230,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
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
Changes since v2:
Based on Wolfram's feedback:
- Use GPIOF_DIR_* flags as returns from get_direction()
- Call spin_lock_irqsave() to before setting flags
drivers/gpio/gpiolib.c | 23 +++++++++++++++++++++++
include/asm-generic/gpio.h | 4 ++++
2 files changed, 27 insertions(+), 0 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index eb74311..a656a2c 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1174,6 +1174,7 @@ int gpio_request(unsigned gpio, const char *label)
struct gpio_desc *desc;
struct gpio_chip *chip;
int status = -EINVAL;
+ int dir;
unsigned long flags;
spin_lock_irqsave(&gpio_lock, flags);
@@ -1214,6 +1215,28 @@ int gpio_request(unsigned gpio, const char *label)
}
}
+ if (chip->get_direction) {
+ /* chip->get_direction may sleep */
+ spin_unlock_irqrestore(&gpio_lock, flags);
+ dir = chip->get_direction(chip, gpio - chip->base);
+ spin_lock_irqsave(&gpio_lock, flags);
+ switch (dir) {
+ case GPIOF_DIR_OUT:
+ set_bit(FLAG_DIR_OUT, &desc->flags);
+ clear_bit(FLAG_DIR_IN, &desc->flags);
+ break;
+ case GPIOF_DIR_IN:
+ set_bit(FLAG_DIR_IN, &desc->flags);
+ clear_bit(FLAG_DIR_OUT, &desc->flags);
+ break;
+ default:
+ /* Direction isn't known */
+ clear_bit(FLAG_DIR_OUT, &desc->flags);
+ clear_bit(FLAG_DIR_IN, &desc->flags);
+ break;
+ }
+ }
+
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, output, or unknown
* @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
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
Changes since v2:
- Use GPIOF_DIR_* defines
MAINTAINERS | 5 +
drivers/gpio/Kconfig | 11 +
drivers/gpio/Makefile | 1 +
drivers/gpio/ichx_gpio.c | 550 ++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 567 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..7f54f63 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..4c7aec6
--- /dev/null
+++ b/drivers/gpio/ichx_gpio.c
@@ -0,0 +1,550 @@
+/*
+ * 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 ichx_read_bit(GPIO_IO_SEL, nr) ? GPIOF_DIR_IN : GPIOF_DIR_OUT;
+}
+
+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 02/18/2011 12:03 PM, Peter Tyser 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.
>
> 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
>
> Change since V2:
> - None
>
> drivers/gpio/gpiolib.c | 91 ++++++++++++++++++++++++++---------------------
> 1 files changed, 50 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 649550e..eb74311 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 */
This patch really shouldn't renumber all (and rename some) of the flags.
Patches should ideally do one thing only, so if you want to rename
FLAG_IS_OUT to FLAG_DIR_OUT that should be done in a separate patch.
Same goes for the renumbering, it should be a separate patch to adding
the support for "unknown" direction.
~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
On Fri, 2011-02-18 at 13:07 +1300, Ryan Mallon wrote:
> On 02/18/2011 12:03 PM, Peter Tyser 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.
> >
> > 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
> >
> > Change since V2:
> > - None
> >
> > drivers/gpio/gpiolib.c | 91 ++++++++++++++++++++++++++---------------------
> > 1 files changed, 50 insertions(+), 41 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index 649550e..eb74311 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 */
>
> This patch really shouldn't renumber all (and rename some) of the flags.
> Patches should ideally do one thing only, so if you want to rename
> FLAG_IS_OUT to FLAG_DIR_OUT that should be done in a separate patch.
> Same goes for the renumbering, it should be a separate patch to adding
> the support for "unknown" direction.
As far as the renaming of FLAG_IS_* to FLAG_DIR_*, I can see it either
way. I had to touch nearly all the flags with the patch anyway, so
reasoned it was OK to tweak the name at the same time since they would
be reviewed as part of the larger change. There's only once instance
(the chunk at line 297) where the change consisted only of a rename -
all other locations had functional changes as well.
For the renumbering, it seems pretty trivial and didn't think it was
worth a unique patch just for it.
Anyway, doesn't matter too much to me. If the powers that be want this
to be split up (or changed to an enum?) let me know.
Best,
Peter
Hello,
On Thu, Feb 17, 2011 at 05:03:16PM -0600, Peter Tyser 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.
>
> While we're playing with the direction flag in/out defines, rename them to
> a more descriptive FLAG_DIR_* format.
Would it make sense to reset the state to unknown on gpio_free?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Thu, Feb 17, 2011 at 05:03:17PM -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.
IMHO the commit log is conceptually wrong, because it talks about a
"pin". Better use gpio here.
> 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
>
> Changes since v2:
> Based on Wolfram's feedback:
> - Use GPIOF_DIR_* flags as returns from get_direction()
> - Call spin_lock_irqsave() to before setting flags
>
> drivers/gpio/gpiolib.c | 23 +++++++++++++++++++++++
> include/asm-generic/gpio.h | 4 ++++
> 2 files changed, 27 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index eb74311..a656a2c 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1174,6 +1174,7 @@ int gpio_request(unsigned gpio, const char *label)
> struct gpio_desc *desc;
> struct gpio_chip *chip;
> int status = -EINVAL;
> + int dir;
> unsigned long flags;
>
> spin_lock_irqsave(&gpio_lock, flags);
> @@ -1214,6 +1215,28 @@ int gpio_request(unsigned gpio, const char *label)
> }
> }
>
> + if (chip->get_direction) {
> + /* chip->get_direction may sleep */
> + spin_unlock_irqrestore(&gpio_lock, flags);
might_sleep_if(chip->can_sleep) ?
> + dir = chip->get_direction(chip, gpio - chip->base);
> + spin_lock_irqsave(&gpio_lock, flags);
> + switch (dir) {
> + case GPIOF_DIR_OUT:
> + set_bit(FLAG_DIR_OUT, &desc->flags);
> + clear_bit(FLAG_DIR_IN, &desc->flags);
> + break;
> + case GPIOF_DIR_IN:
> + set_bit(FLAG_DIR_IN, &desc->flags);
> + clear_bit(FLAG_DIR_OUT, &desc->flags);
> + break;
> + default:
> + /* Direction isn't known */
> + clear_bit(FLAG_DIR_OUT, &desc->flags);
> + clear_bit(FLAG_DIR_IN, &desc->flags);
> + break;
> + }
Alternatively to my suggestion for patch1:
} else {
clear_bit(FLAG_DIR_OUT, &desc->flags);
clear_bit(FLAG_DIR_IN, &desc->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, output, or unknown
> * @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
>
>
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Fri, 2011-02-18 at 09:57 +0100, Uwe Kleine-König wrote:
> On Thu, Feb 17, 2011 at 05:03:17PM -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.
> IMHO the commit log is conceptually wrong, because it talks about a
> "pin". Better use gpio here.
I don't follow. I used "pin" to make it clear that the get_direction()
function operates on a pin-by-pin basis, and to help reduce any
ambiguity about if a gpio chip or gpio pin is being referred to. Would
you prefer that the "pin" references are clarified to be "GPIO pin"?
> > 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
> >
> > Changes since v2:
> > Based on Wolfram's feedback:
> > - Use GPIOF_DIR_* flags as returns from get_direction()
> > - Call spin_lock_irqsave() to before setting flags
> >
> > drivers/gpio/gpiolib.c | 23 +++++++++++++++++++++++
> > include/asm-generic/gpio.h | 4 ++++
> > 2 files changed, 27 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index eb74311..a656a2c 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -1174,6 +1174,7 @@ int gpio_request(unsigned gpio, const char *label)
> > struct gpio_desc *desc;
> > struct gpio_chip *chip;
> > int status = -EINVAL;
> > + int dir;
> > unsigned long flags;
> >
> > spin_lock_irqsave(&gpio_lock, flags);
> > @@ -1214,6 +1215,28 @@ int gpio_request(unsigned gpio, const char *label)
> > }
> > }
> >
> > + if (chip->get_direction) {
> > + /* chip->get_direction may sleep */
> > + spin_unlock_irqrestore(&gpio_lock, flags);
> might_sleep_if(chip->can_sleep) ?
Makes sense. I was following the lead of chip->request() in the same
function, which doesn't use might_sleep_if(). I assume might_sleep_if()
should be added to it as well in a separate patch?
> > + dir = chip->get_direction(chip, gpio - chip->base);
> > + spin_lock_irqsave(&gpio_lock, flags);
> > + switch (dir) {
> > + case GPIOF_DIR_OUT:
> > + set_bit(FLAG_DIR_OUT, &desc->flags);
> > + clear_bit(FLAG_DIR_IN, &desc->flags);
> > + break;
> > + case GPIOF_DIR_IN:
> > + set_bit(FLAG_DIR_IN, &desc->flags);
> > + clear_bit(FLAG_DIR_OUT, &desc->flags);
> > + break;
> > + default:
> > + /* Direction isn't known */
> > + clear_bit(FLAG_DIR_OUT, &desc->flags);
> > + clear_bit(FLAG_DIR_IN, &desc->flags);
> > + break;
> > + }
> Alternatively to my suggestion for patch1:
> } else {
> clear_bit(FLAG_DIR_OUT, &desc->flags);
> clear_bit(FLAG_DIR_IN, &desc->flags);
I like this way better too. I'll initialize dir = -1 and pull the
switch statement out of the conditional, like:
if (chip->get_direction) {
/* chip->get_direction may sleep */
spin_unlock_irqrestore(&gpio_lock, flags);
might_sleep_if(chip->can_sleep);
dir = chip->get_direction(chip, gpio - chip->base);
spin_lock_irqsave(&gpio_lock, flags);
}
switch (dir) {
case GPIOF_DIR_OUT:
set_bit(FLAG_DIR_OUT, &desc->flags);
clear_bit(FLAG_DIR_IN, &desc->flags);
break;
case GPIOF_DIR_IN:
set_bit(FLAG_DIR_IN, &desc->flags);
clear_bit(FLAG_DIR_OUT, &desc->flags);
break;
default:
/* Direction isn't known */
clear_bit(FLAG_DIR_OUT, &desc->flags);
clear_bit(FLAG_DIR_IN, &desc->flags);
break;
}
Thanks for the comments,
Peter
Hi,
I have tested it on a Pineview / Tigerpoint based board.
It works well given you add the PCI ids for the tigerpoint/NM10 chipset
(cf patch below).
The Tigerpoint/NM10 is ICH7 based for the GPIO part.
Tested-by: Vincent Palatin <[email protected]>
Signed-off-by: Vincent Palatin <[email protected]>
---
drivers/gpio/ichx_gpio.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/gpio/ichx_gpio.c b/drivers/gpio/ichx_gpio.c
index 4c7aec6..4bba9af 100644
--- a/drivers/gpio/ichx_gpio.c
+++ b/drivers/gpio/ichx_gpio.c
@@ -394,6 +394,7 @@ static DEFINE_PCI_DEVICE_TABLE(ichx_pci_tbl) = {
{ 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_TGP_LPC, &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) },
--
1.7.3.1
Hello Peter,
On Fri, Feb 18, 2011 at 11:36:26AM -0600, Peter Tyser wrote:
> On Fri, 2011-02-18 at 09:57 +0100, Uwe Kleine-K?nig wrote:
> > On Thu, Feb 17, 2011 at 05:03:17PM -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.
> > IMHO the commit log is conceptually wrong, because it talks about a
> > "pin". Better use gpio here.
>
> I don't follow. I used "pin" to make it clear that the get_direction()
> function operates on a pin-by-pin basis, and to help reduce any
> ambiguity about if a gpio chip or gpio pin is being referred to. Would
> you prefer that the "pin" references are clarified to be "GPIO pin"?
Maybe it's just that we use different terms. For me a "pin" is an
entry/exit point into/from a cpu or other chip. A gpio is (hmm, how
should I say) a concept how to drive a pin. A gpio might or might not
be "connected" to a pin at a given time.
> > > 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
> > >
> > > Changes since v2:
> > > Based on Wolfram's feedback:
> > > - Use GPIOF_DIR_* flags as returns from get_direction()
> > > - Call spin_lock_irqsave() to before setting flags
> > >
> > > drivers/gpio/gpiolib.c | 23 +++++++++++++++++++++++
> > > include/asm-generic/gpio.h | 4 ++++
> > > 2 files changed, 27 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > index eb74311..a656a2c 100644
> > > --- a/drivers/gpio/gpiolib.c
> > > +++ b/drivers/gpio/gpiolib.c
> > > @@ -1174,6 +1174,7 @@ int gpio_request(unsigned gpio, const char *label)
> > > struct gpio_desc *desc;
> > > struct gpio_chip *chip;
> > > int status = -EINVAL;
> > > + int dir;
> > > unsigned long flags;
> > >
> > > spin_lock_irqsave(&gpio_lock, flags);
> > > @@ -1214,6 +1215,28 @@ int gpio_request(unsigned gpio, const char *label)
> > > }
> > > }
> > >
> > > + if (chip->get_direction) {
> > > + /* chip->get_direction may sleep */
> > > + spin_unlock_irqrestore(&gpio_lock, flags);
> > might_sleep_if(chip->can_sleep) ?
>
> Makes sense. I was following the lead of chip->request() in the same
> function, which doesn't use might_sleep_if(). I assume might_sleep_if()
> should be added to it as well in a separate patch?
I was there first :-)
http://mid.gmane.org/[email protected]
> > > + dir = chip->get_direction(chip, gpio - chip->base);
> > > + spin_lock_irqsave(&gpio_lock, flags);
> > > + switch (dir) {
> > > + case GPIOF_DIR_OUT:
> > > + set_bit(FLAG_DIR_OUT, &desc->flags);
> > > + clear_bit(FLAG_DIR_IN, &desc->flags);
> > > + break;
> > > + case GPIOF_DIR_IN:
> > > + set_bit(FLAG_DIR_IN, &desc->flags);
> > > + clear_bit(FLAG_DIR_OUT, &desc->flags);
> > > + break;
> > > + default:
> > > + /* Direction isn't known */
> > > + clear_bit(FLAG_DIR_OUT, &desc->flags);
> > > + clear_bit(FLAG_DIR_IN, &desc->flags);
> > > + break;
> > > + }
> > Alternatively to my suggestion for patch1:
> > } else {
> > clear_bit(FLAG_DIR_OUT, &desc->flags);
> > clear_bit(FLAG_DIR_IN, &desc->flags);
>
> I like this way better too. I'll initialize dir = -1 and pull the
> switch statement out of the conditional, like:
>
> if (chip->get_direction) {
> /* chip->get_direction may sleep */
> spin_unlock_irqrestore(&gpio_lock, flags);
> might_sleep_if(chip->can_sleep);
> dir = chip->get_direction(chip, gpio - chip->base);
> spin_lock_irqsave(&gpio_lock, flags);
> }
>
> switch (dir) {
> case GPIOF_DIR_OUT:
> set_bit(FLAG_DIR_OUT, &desc->flags);
> clear_bit(FLAG_DIR_IN, &desc->flags);
> break;
> case GPIOF_DIR_IN:
> set_bit(FLAG_DIR_IN, &desc->flags);
> clear_bit(FLAG_DIR_OUT, &desc->flags);
> break;
> default:
> /* Direction isn't known */
> clear_bit(FLAG_DIR_OUT, &desc->flags);
> clear_bit(FLAG_DIR_IN, &desc->flags);
> break;
> }
fine
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Fri, 2011-02-18 at 19:49 +0100, Uwe Kleine-König wrote:
> Hello Peter,
>
> On Fri, Feb 18, 2011 at 11:36:26AM -0600, Peter Tyser wrote:
> > On Fri, 2011-02-18 at 09:57 +0100, Uwe Kleine-König wrote:
> > > On Thu, Feb 17, 2011 at 05:03:17PM -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.
> > > IMHO the commit log is conceptually wrong, because it talks about a
> > > "pin". Better use gpio here.
> >
> > I don't follow. I used "pin" to make it clear that the get_direction()
> > function operates on a pin-by-pin basis, and to help reduce any
> > ambiguity about if a gpio chip or gpio pin is being referred to. Would
> > you prefer that the "pin" references are clarified to be "GPIO pin"?
> Maybe it's just that we use different terms. For me a "pin" is an
> entry/exit point into/from a cpu or other chip. A gpio is (hmm, how
> should I say) a concept how to drive a pin. A gpio might or might not
> be "connected" to a pin at a given time.
Ahh, I see... The GPIO subsystem doesn't have the capability to
multiplex a GPIO to different physical pins at the moment, so I always
equate a GPIO with a specific pin, even if its not "connected" all the
time. I'll remove the "pin" references when I resubmit if you think it
is more accurate.
Peter
On Fri, 2011-02-18 at 12:58 -0500, Vincent Palatin wrote:
> Hi,
>
> I have tested it on a Pineview / Tigerpoint based board.
> It works well given you add the PCI ids for the tigerpoint/NM10 chipset
> (cf patch below).
> The Tigerpoint/NM10 is ICH7 based for the GPIO part.
>
> Tested-by: Vincent Palatin <[email protected]>
>
> Signed-off-by: Vincent Palatin <[email protected]>
Thanks for testing. I'll add the Tigerpoint and your tested/signoff
when I resubmit.
Peter