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]>
Cc: Lars-Peter Clausen <[email protected]>
Cc: Alexander Stein <[email protected]>
Cc: Ryan Mallon <[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
Change since V3:
- None
Change since V4:
- 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
Add a new get_direction() function to the gpio_chip structure. This is
useful so that the direction of a GPIO can be determined when its
initially exported. Previously, the direction defaulted to "unknown"
regardless of the actual configuration of the GPIO.
If a GPIO driver implements get_direction(), it is called in
gpio_request() to set the initial direction of the GPIO 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
Changes since v3:
- Removed "pin" references from the commit message
- Added might_sleep() call
- Made the direction always be set in gpio_request()
Change since V4:
- None
drivers/gpio/gpiolib.c | 25 +++++++++++++++++++++++++
include/asm-generic/gpio.h | 4 ++++
2 files changed, 29 insertions(+), 0 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index eb74311..106164f 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 = -1; /* Default to unknown direction */
unsigned long flags;
spin_lock_irqsave(&gpio_lock, flags);
@@ -1214,6 +1215,30 @@ 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;
+ }
+
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]>
Signed-off-by: Vincent Palatin <[email protected]>
Tested-by: Vincent Palatin <[email protected]>
---
Changes since v1:
- Fixed grammar in Kconfig menu entry
Changes since v2:
- Use GPIOF_DIR_* defines
Changes since v3:
- Added Vincent's signed-off/tested-by
- Added Tigerpoint PCI ID
Changes since v4:
- Integrated feedback from Joe:
- Fixed comment before ;
- Fixed bridge typo
- Updated to use pr_fmt and pr_<level>
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..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..b841a1a
--- /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.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#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
+ * bridge 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) {
+ pr_err("GPIO BAR not enumerated\n");
+ 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)
+ pr_warn("ACPI BAR not enumerated, GPI 0 - 15 "
+ "unusable\n");
+ }
+
+ /* 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")) {
+ pr_err("Can't request iomem (0x%lx)\n", ichx_priv.gpio_base);
+ return -EBUSY;
+ }
+
+ ichx_gpiolib_setup(&ichx_priv.chip);
+
+ err = gpiochip_add(&ichx_priv.chip);
+ if (err) {
+ pr_err("Failed to register GPIOs\n");
+ return err;
+ }
+
+ pr_info("GPIO from %d to %d on %s\n", 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_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) },
+ { 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
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
Changes since v3:
- None
Change since V4:
- None
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
On Tue, Mar 01, 2011 at 05:28:17PM -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.
It's well past time for me to weigh in on this debate...
So, basically, adding an 'unknown' state to gpio pins is all about the
sysfs interface. All of the in-kernel gpio users should already be
setting the pin direction before using the gpio. The sysfs interface
is currently the only interface that has a method for querying the
direction.
The thing about gpios is that how they are used is entirely dependant
on what they are wired up to. There is no avoiding the fact that you
*absolutely* must understand the usage model before even considering
fiddling with a gpio (ignoring the "I wonder what this knob does"
use-case such as when reverse engineer a board). So, while it is nice
to have an 'unknown' state for sysfs to report, it is certainly not
required. The model still remains that the pin direction must be set
before (or at the same time as) reading/writing the pin.
I'm *not* talking about gpio_request() either. A gpio request must
not change the state of the pin, and any gpio driver that does is
simply buggy. gpio_request() must only be used to ensure exclusive
access to the pin.
I'm also not talking about alt_func or pin muxing, or anything
like that. The gpio api is only concerned about n gpios on a gpio
controller. Any munging of pin routing to get the gpio to the outside
world is beyond the scope of the GPIO API which doesn't even attempt
to handle or express it. IMNSHO, that is completely by design. If a
pin is configured for an alternate function, then it is conceptually
equivalent to the pin being physically disconnected from the gpio
controller. The gpio is still present and can be twiddled, but it
isn't actually connected to anything, and the gpio controller driver
may or may not be able to know anything about it. I am not interested
in trying to abstract all the complexity of pin mux into the gpio api.
Back to sysfs: The sysfs gpio interface is useful for many reasons,
but it is dangerous much in the same way that /dev/mem is dangerous.
It gives userspace unfettered access to gpio resources without any
clues about how it should be used. I consider it bad practice to
depend on the gpio sysfs interface for any real system/application.
gpio numbers could easily change or become unavailable if a kernel
driver decides to use them (heck, I'd like to be rid of gpio
numbers entirely, but that's a separate debate). I'd much rather see
real device drivers be written for gpio interfaces instead of bodging
things together from userspace. Since the addition of an 'unknown'
direction is solely for benefit of the sysfs interface, I don't (yet)
see a valid argument for adding it. *Who cares* if sysfs might report
direction as 'input' inaccurately? It may be mildly inconvenient to a
human reader, but it doesn't change the usage model one iota because
the direction still must be explicitly set before using the gpio.
So, I'm going to say no to this patch.
g.
>
> 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]>
> Cc: Lars-Peter Clausen <[email protected]>
> Cc: Alexander Stein <[email protected]>
> Cc: Ryan Mallon <[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
>
> Change since V3:
> - None
>
> Change since V4:
> - 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
>
On Tue, Mar 01, 2011 at 05:28:18PM -0600, Peter Tyser wrote:
> Add a new get_direction() function to the gpio_chip structure. This is
> useful so that the direction of a GPIO can be determined when its
> initially exported. Previously, the direction defaulted to "unknown"
> regardless of the actual configuration of the GPIO.
>
> If a GPIO driver implements get_direction(), it is called in
> gpio_request() to set the initial direction of the GPIO accurately.
I don't like this approach. It effectively creates two methods for
determining the direction of a gpio; either ask the driver, or read
the flags value. Currently, only gpio_request() actually uses the
first option, but it still leaves the ambiguity.
Instead, the driver should be able to preload the direction flag at or
shortly after gpiochip_add() time. No need for a new callback hook
from what I can see.
g.
>
> 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
>
> Changes since v3:
> - Removed "pin" references from the commit message
> - Added might_sleep() call
> - Made the direction always be set in gpio_request()
>
> Change since V4:
> - None
>
> drivers/gpio/gpiolib.c | 25 +++++++++++++++++++++++++
> include/asm-generic/gpio.h | 4 ++++
> 2 files changed, 29 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index eb74311..106164f 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 = -1; /* Default to unknown direction */
> unsigned long flags;
>
> spin_lock_irqsave(&gpio_lock, flags);
> @@ -1214,6 +1215,30 @@ 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;
> + }
> +
> 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
>
On 03/06/2011 08:25 PM, Grant Likely wrote:
> Back to sysfs: The sysfs gpio interface is useful for many reasons,
> but it is dangerous much in the same way that /dev/mem is dangerous.
> It gives userspace unfettered access to gpio resources without any
> clues about how it should be used. I consider it bad practice to
> depend on the gpio sysfs interface for any real system/application.
In an embedded system, where both the kernel and the userspace are
provided by the hardware vendor, it can be completely sensible to rely
on gpio numbers in userspace (though I would also like to see the sysfs
interface able to use named access). There are some existing kernel
interfaces for common gpio functions such as leds and input devices, but
there are also many other valid uses for gpios. There are many reasons
for the access code to be in userspace too: It may be easier to write
and debug, depending on the setup of the device it may be easier to do
firmware upgrades of userspace than it is for the kernel, etc.
> gpio numbers could easily change or become unavailable if a kernel
> driver decides to use them (heck, I'd like to be rid of gpio
> numbers entirely, but that's a separate debate). I'd much rather see
> real device drivers be written for gpio interfaces instead of bodging
> things together from userspace. Since the addition of an 'unknown'
> direction is solely for benefit of the sysfs interface, I don't (yet)
> see a valid argument for adding it. *Who cares* if sysfs might report
> direction as 'input' inaccurately?
Because it is incorrect? It may also be useful for a userspace debug
tool to request all available gpios and show the current direction and
value of them.
> It may be mildly inconvenient to a
> human reader, but it doesn't change the usage model one iota because
> the direction still must be explicitly set before using the gpio.
I agree that the usage model should be to request and then explicitly
set the direction before use, but that isn't really an argument for
exporting incorrect information to userspace. The ABI should attempt to
prevent abuse of itself so that crappy userspace apps cannot be written.
Either exporting the direction as "unknown" or making the direction file
unreadable and the value file unreadable/unwritable until the direction
has been explicitly set?
> So, I'm going to say no to this patch.
The patch is small (it adds 9 lines) and fixes an incorrect value being
exported to userspace. By your argument we should actually remove the
ability to read the direction file in sysfs, since userspace must
explicitly set it, and therefore knows what the direction is?
~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 Mon, 2011-03-07 at 09:19 +1300, Ryan Mallon wrote:
> On 03/06/2011 08:25 PM, Grant Likely wrote:
>
> > Back to sysfs: The sysfs gpio interface is useful for many reasons,
> > but it is dangerous much in the same way that /dev/mem is dangerous.
> > It gives userspace unfettered access to gpio resources without any
> > clues about how it should be used. I consider it bad practice to
> > depend on the gpio sysfs interface for any real system/application.
>
> In an embedded system, where both the kernel and the userspace are
> provided by the hardware vendor, it can be completely sensible to rely
> on gpio numbers in userspace (though I would also like to see the sysfs
> interface able to use named access). There are some existing kernel
> interfaces for common gpio functions such as leds and input devices, but
> there are also many other valid uses for gpios. There are many reasons
> for the access code to be in userspace too: It may be easier to write
> and debug, depending on the setup of the device it may be easier to do
> firmware upgrades of userspace than it is for the kernel, etc.
>
> > gpio numbers could easily change or become unavailable if a kernel
> > driver decides to use them (heck, I'd like to be rid of gpio
> > numbers entirely, but that's a separate debate). I'd much rather see
> > real device drivers be written for gpio interfaces instead of bodging
> > things together from userspace. Since the addition of an 'unknown'
> > direction is solely for benefit of the sysfs interface, I don't (yet)
> > see a valid argument for adding it. *Who cares* if sysfs might report
> > direction as 'input' inaccurately?
>
> Because it is incorrect? It may also be useful for a userspace debug
> tool to request all available gpios and show the current direction and
> value of them.
>
> > It may be mildly inconvenient to a
> > human reader, but it doesn't change the usage model one iota because
> > the direction still must be explicitly set before using the gpio.
>
> I agree that the usage model should be to request and then explicitly
> set the direction before use, but that isn't really an argument for
> exporting incorrect information to userspace. The ABI should attempt to
> prevent abuse of itself so that crappy userspace apps cannot be written.
> Either exporting the direction as "unknown" or making the direction file
> unreadable and the value file unreadable/unwritable until the direction
> has been explicitly set?
>
> > So, I'm going to say no to this patch.
>
> The patch is small (it adds 9 lines) and fixes an incorrect value being
> exported to userspace. By your argument we should actually remove the
> ability to read the direction file in sysfs, since userspace must
> explicitly set it, and therefore knows what the direction is?
+1 to all Ryan's points. I hope this patch, or something like it, will
be accepted.
On a related note, I just noticed I need to update
Documentation/gpio.txt to document the 'unknown' direction, if this
patch is accepted. I'll resubmit after waiting for feedback.
Best,
Peter
> The thing about gpios is that how they are used is entirely dependant
> on what they are wired up to. There is no avoiding the fact that you
> *absolutely* must understand the usage model before even considering
> fiddling with a gpio (ignoring the "I wonder what this knob does"
> use-case such as when reverse engineer a board). So, while it is nice
> to have an 'unknown' state for sysfs to report, it is certainly not
> required. The model still remains that the pin direction must be set
> before (or at the same time as) reading/writing the pin.
As is, even if someone knows about the GPIO wiring on their board, they
have to know Linux has a "rule" that "before I can use a GPIO, I have to
explicitly set its direction, even if the current reported direction is
what I want". A number of our customers have tried to use a GPIO which
states its an 'input' as an input after exporting it, which is
completely logical. But it doesn't work, because its not really an
input... Why not set the direction accurately as 'unknown' so users
intuitively know they have to set the direction before using it? You
also mention that it would be a nice feature above, so why not include
it?
> Since the addition of an 'unknown'
> direction is solely for benefit of the sysfs interface, I don't (yet)
> see a valid argument for adding it. *Who cares* if sysfs might report
> direction as 'input' inaccurately? It may be mildly inconvenient to a
> human reader, but it doesn't change the usage model one iota because
> the direction still must be explicitly set before using the gpio.
Adding an 'unknown' direction *forces* people to use the recommended
usage model. As is, I'd guess most end users don't know about the
recommended usage model, and they are misled by the incorrect info in
sysfs to boot.
Best,
Peter
On Sun, 2011-03-06 at 00:30 -0700, Grant Likely wrote:
> On Tue, Mar 01, 2011 at 05:28:18PM -0600, Peter Tyser wrote:
> > Add a new get_direction() function to the gpio_chip structure. This is
> > useful so that the direction of a GPIO can be determined when its
> > initially exported. Previously, the direction defaulted to "unknown"
> > regardless of the actual configuration of the GPIO.
> >
> > If a GPIO driver implements get_direction(), it is called in
> > gpio_request() to set the initial direction of the GPIO accurately.
>
> I don't like this approach. It effectively creates two methods for
> determining the direction of a gpio; either ask the driver, or read
> the flags value. Currently, only gpio_request() actually uses the
> first option, but it still leaves the ambiguity.
>
> Instead, the driver should be able to preload the direction flag at or
> shortly after gpiochip_add() time. No need for a new callback hook
> from what I can see.
The callback hook was used because the gpio_desc[] structure was local
to gpiolib.c. The code would have to be restructured a bit to allow
drivers to set the flags themselves. I can do that if you'd prefer, but
don't think it'd be all that much cleaner.
Also, I thought it made sense to read the direction of the GPIO in
gpio_request() because that's the point that the GPIO comes under the
GPIO subsystem's control. Prior to that, there's a chance the direction
could be changed by platform code, or another driver, etc, so reading
the direction in gpiochip_add() may result in out-of-date directions.
The reading of the direction could also be put in chip drivers'
request() function. That would get rid of the callback and still ensure
the direction is up-to-date.
I think the current patch seems cleaner than the alternatives, but will
change if you'd prefer.
Best,
Peter
On Sun, Mar 06, 2011 at 09:07:21PM -0600, Peter Tyser wrote:
> On Sun, 2011-03-06 at 00:30 -0700, Grant Likely wrote:
> > On Tue, Mar 01, 2011 at 05:28:18PM -0600, Peter Tyser wrote:
> > > Add a new get_direction() function to the gpio_chip structure. This is
> > > useful so that the direction of a GPIO can be determined when its
> > > initially exported. Previously, the direction defaulted to "unknown"
> > > regardless of the actual configuration of the GPIO.
> > >
> > > If a GPIO driver implements get_direction(), it is called in
> > > gpio_request() to set the initial direction of the GPIO accurately.
> >
> > I don't like this approach. It effectively creates two methods for
> > determining the direction of a gpio; either ask the driver, or read
> > the flags value. Currently, only gpio_request() actually uses the
> > first option, but it still leaves the ambiguity.
> >
> > Instead, the driver should be able to preload the direction flag at or
> > shortly after gpiochip_add() time. No need for a new callback hook
> > from what I can see.
>
> The callback hook was used because the gpio_desc[] structure was local
> to gpiolib.c. The code would have to be restructured a bit to allow
> drivers to set the flags themselves. I can do that if you'd prefer, but
> don't think it'd be all that much cleaner.
Make it part of the gpiochip_add function, or add a function for
explicitly setting gpio directions. Should not require any
reorganization at all.
> Also, I thought it made sense to read the direction of the GPIO in
> gpio_request() because that's the point that the GPIO comes under the
> GPIO subsystem's control. Prior to that, there's a chance the direction
> could be changed by platform code, or another driver, etc, so reading
> the direction in gpiochip_add() may result in out-of-date directions.
That's the problem with gpiolib having sole responsibility of the
cache. It doesn't give the driver any recourse for changing things if
it needs to. The best solution might very well be to eliminate the
direction state flag entirely and force all gpiochip drivers to
populate a gpio_get_direction() callback, but that is a lot more work.
> The reading of the direction could also be put in chip drivers'
> request() function. That would get rid of the callback and still ensure
> the direction is up-to-date.
I don't object to a callback hook. My objection is how it is bodged
on to work around limitations to the direction being cached in the
flags variable. I want to see a solution that either depends entirely
on the callback, or completely fixes the problems with the cached
value by allowing the driver to update it.
> I think the current patch seems cleaner than the alternatives, but will
> change if you'd prefer.
Thanks,
g.
On Sun, Mar 06, 2011 at 08:43:05PM -0600, Peter Tyser wrote:
> > The thing about gpios is that how they are used is entirely dependant
> > on what they are wired up to. There is no avoiding the fact that you
> > *absolutely* must understand the usage model before even considering
> > fiddling with a gpio (ignoring the "I wonder what this knob does"
> > use-case such as when reverse engineer a board). So, while it is nice
> > to have an 'unknown' state for sysfs to report, it is certainly not
> > required. The model still remains that the pin direction must be set
> > before (or at the same time as) reading/writing the pin.
>
> As is, even if someone knows about the GPIO wiring on their board, they
> have to know Linux has a "rule" that "before I can use a GPIO, I have to
> explicitly set its direction, even if the current reported direction is
> what I want". A number of our customers have tried to use a GPIO which
> states its an 'input' as an input after exporting it, which is
> completely logical. But it doesn't work, because its not really an
> input... Why not set the direction accurately as 'unknown' so users
> intuitively know they have to set the direction before using it? You
> also mention that it would be a nice feature above, so why not include
> it?
I don't like what it does to the implementation, and I'd rather make
drivers provide accurate data at the outset.
g.
On Mon, Mar 07, 2011 at 09:19:21AM +1300, Ryan Mallon wrote:
> On 03/06/2011 08:25 PM, Grant Likely wrote:
>
> > Back to sysfs: The sysfs gpio interface is useful for many reasons,
> > but it is dangerous much in the same way that /dev/mem is dangerous.
> > It gives userspace unfettered access to gpio resources without any
> > clues about how it should be used. I consider it bad practice to
> > depend on the gpio sysfs interface for any real system/application.
>
> In an embedded system, where both the kernel and the userspace are
> provided by the hardware vendor, it can be completely sensible to rely
> on gpio numbers in userspace (though I would also like to see the sysfs
> interface able to use named access). There are some existing kernel
> interfaces for common gpio functions such as leds and input devices, but
> there are also many other valid uses for gpios. There are many reasons
> for the access code to be in userspace too: It may be easier to write
> and debug, depending on the setup of the device it may be easier to do
> firmware upgrades of userspace than it is for the kernel, etc.
I don't dispute any of the above points. It is the sysfs interface
itself that I see as problematic. It provides no discoverability and
it does not change the fact that once gpios are actually attached to
something they become specific purpose and users *absolutely must*
understand exactly what they are doing.
> > gpio numbers could easily change or become unavailable if a kernel
> > driver decides to use them (heck, I'd like to be rid of gpio
> > numbers entirely, but that's a separate debate). I'd much rather see
> > real device drivers be written for gpio interfaces instead of bodging
> > things together from userspace. Since the addition of an 'unknown'
> > direction is solely for benefit of the sysfs interface, I don't (yet)
> > see a valid argument for adding it. *Who cares* if sysfs might report
> > direction as 'input' inaccurately?
>
> Because it is incorrect? It may also be useful for a userspace debug
> tool to request all available gpios and show the current direction and
> value of them.
If it was a simple & correct fix, then I'd have no problem with it.
The current proposed solution doesn't meet that criteria for me (more
below)
>
> > It may be mildly inconvenient to a
> > human reader, but it doesn't change the usage model one iota because
> > the direction still must be explicitly set before using the gpio.
>
> I agree that the usage model should be to request and then explicitly
> set the direction before use, but that isn't really an argument for
> exporting incorrect information to userspace. The ABI should attempt to
> prevent abuse of itself so that crappy userspace apps cannot be written.
> Either exporting the direction as "unknown" or making the direction file
> unreadable and the value file unreadable/unwritable until the direction
> has been explicitly set?
I'm far more interested in a solution that puts the onus on the GPIO
driver to populate the starting state at registration time, and
possibly to update it if it changes due to unrelated events.
Another consideration is that adding an 'unknown' value changes the
user space ABI, which must always be approached carefully.
>
> > So, I'm going to say no to this patch.
>
> The patch is small (it adds 9 lines) and fixes an incorrect value being
> exported to userspace.
It solves the problem at the wrong level. Rather than having an
'unknown' state, I'm far more interested in a solution that pushed
back on the drivers to correctly initialize the gpio starting state
at gpiochip_add() time. This would be both simpler and more correct
from my perspective.
And, yes, I do realize that some controllers cannot be probed for the
starting state, but I strongly suspect them to be a minority and
starting state can be handled with pdata in those cases.
> By your argument we should actually remove the
> ability to read the direction file in sysfs, since userspace must
> explicitly set it, and therefore knows what the direction is?
No, I'm arguing that the driver should provide good data from the
outset. If it does not, then defaulting to 'input' is both a
reasonable assumption, and is safe.
g.
On Sun, 2011-03-06 at 23:52 -0700, Grant Likely wrote:
> On Sun, Mar 06, 2011 at 08:43:05PM -0600, Peter Tyser wrote:
> > > The thing about gpios is that how they are used is entirely dependant
> > > on what they are wired up to. There is no avoiding the fact that you
> > > *absolutely* must understand the usage model before even considering
> > > fiddling with a gpio (ignoring the "I wonder what this knob does"
> > > use-case such as when reverse engineer a board). So, while it is nice
> > > to have an 'unknown' state for sysfs to report, it is certainly not
> > > required. The model still remains that the pin direction must be set
> > > before (or at the same time as) reading/writing the pin.
> >
> > As is, even if someone knows about the GPIO wiring on their board, they
> > have to know Linux has a "rule" that "before I can use a GPIO, I have to
> > explicitly set its direction, even if the current reported direction is
> > what I want". A number of our customers have tried to use a GPIO which
> > states its an 'input' as an input after exporting it, which is
> > completely logical. But it doesn't work, because its not really an
> > input... Why not set the direction accurately as 'unknown' so users
> > intuitively know they have to set the direction before using it? You
> > also mention that it would be a nice feature above, so why not include
> > it?
>
> I don't like what it does to the implementation, and I'd rather make
> drivers provide accurate data at the outset.
Agreed that ideally drivers should provide accurate data at the outset.
The original patch attempted to move in that direction by making it
optional, and didn't touch the concept of "unknown" directions. Alan
and others argued we should add support for the "unknown" direction,
which spawned this patch.
To be clear, are you saying you won't accept a patch adding an "unknown"
direction? Or would you be OK with under circumstance X? If so, what
is circumstance X?
Best,
Peter
On Mon, 2011-03-07 at 00:08 -0700, Grant Likely wrote:
> On Sun, Mar 06, 2011 at 09:07:21PM -0600, Peter Tyser wrote:
> > On Sun, 2011-03-06 at 00:30 -0700, Grant Likely wrote:
> > > On Tue, Mar 01, 2011 at 05:28:18PM -0600, Peter Tyser wrote:
> > > > Add a new get_direction() function to the gpio_chip structure. This is
> > > > useful so that the direction of a GPIO can be determined when its
> > > > initially exported. Previously, the direction defaulted to "unknown"
> > > > regardless of the actual configuration of the GPIO.
> > > >
> > > > If a GPIO driver implements get_direction(), it is called in
> > > > gpio_request() to set the initial direction of the GPIO accurately.
> > >
> > > I don't like this approach. It effectively creates two methods for
> > > determining the direction of a gpio; either ask the driver, or read
> > > the flags value. Currently, only gpio_request() actually uses the
> > > first option, but it still leaves the ambiguity.
> > >
> > > Instead, the driver should be able to preload the direction flag at or
> > > shortly after gpiochip_add() time. No need for a new callback hook
> > > from what I can see.
> >
> > The callback hook was used because the gpio_desc[] structure was local
> > to gpiolib.c. The code would have to be restructured a bit to allow
> > drivers to set the flags themselves. I can do that if you'd prefer, but
> > don't think it'd be all that much cleaner.
>
> Make it part of the gpiochip_add function, or add a function for
> explicitly setting gpio directions. Should not require any
> reorganization at all.
>
> > Also, I thought it made sense to read the direction of the GPIO in
> > gpio_request() because that's the point that the GPIO comes under the
> > GPIO subsystem's control. Prior to that, there's a chance the direction
> > could be changed by platform code, or another driver, etc, so reading
> > the direction in gpiochip_add() may result in out-of-date directions.
>
> That's the problem with gpiolib having sole responsibility of the
> cache. It doesn't give the driver any recourse for changing things if
> it needs to. The best solution might very well be to eliminate the
> direction state flag entirely and force all gpiochip drivers to
> populate a gpio_get_direction() callback, but that is a lot more work.
>
> > The reading of the direction could also be put in chip drivers'
> > request() function. That would get rid of the callback and still ensure
> > the direction is up-to-date.
>
> I don't object to a callback hook. My objection is how it is bodged
> on to work around limitations to the direction being cached in the
> flags variable. I want to see a solution that either depends entirely
> on the callback, or completely fixes the problems with the cached
> value by allowing the driver to update it.
I agree it'd be nice to have each driver implement a get_direction()
function and remove the FLAG_DIR_* flags altogether, but didn't want to
do the work to 20 drivers which I don't use... Would you accept a
series that:
- Added "unknown" direction support (eg patch 1)
- Added ability to get GPIO direction (eg patch 2 in this series)
- Sequence of patches to add get_direction() to all GPIO drivers (most
of which I couldn't test).
- Replace checking of FLAG_DIR_* in gpiolib.c with calls to new
get_direction() function and remove FLAG_DIR_* flags all together.
It should be relatively easy to add get_direction() to most drivers, and
I'm willing to do it if all parties would agree it was the proper
direction to take. Or do you have any other suggestions about an easier
migration path?
Best,
Peter
> I don't object to a callback hook. My objection is how it is bodged
> on to work around limitations to the direction being cached in the
> flags variable. I want to see a solution that either depends entirely
> on the callback, or completely fixes the problems with the cached
> value by allowing the driver to update it.
Doing it all by callback might actually fix a lot of the problems because
it can handle all kinds of 'unknowns'. If the callbacks for set/get
optionally pass a char buffer as well even the /proc interface just comes
out in the wash as the device can return a string to populate the status
or to be parsed (obviously with most h/w using the default method which
is in/out)
However who then does the enforcement of gpio_foo calls if the flag is
not cached, does that end up in each driver or is there still a cache of
some form ?
Not sure updating the interface is that hard either - we've done it
before with other layers simply by starting off with
if (foo->ops->method)
foo->ops->method(foo, bar);
else {
old fixed method + glue
}
(or by forcing foo->ops->method on init to point to a default handler,
but that breaks making ops const)
Alan
On Tue, 2011-03-08 at 12:13 +0000, Alan Cox wrote:
> > I don't object to a callback hook. My objection is how it is bodged
> > on to work around limitations to the direction being cached in the
> > flags variable. I want to see a solution that either depends entirely
> > on the callback, or completely fixes the problems with the cached
> > value by allowing the driver to update it.
>
> Doing it all by callback might actually fix a lot of the problems because
> it can handle all kinds of 'unknowns'. If the callbacks for set/get
> optionally pass a char buffer as well even the /proc interface just comes
> out in the wash as the device can return a string to populate the status
> or to be parsed (obviously with most h/w using the default method which
> is in/out)
I like the callback-only implementation also.
> However who then does the enforcement of gpio_foo calls if the flag is
> not cached, does that end up in each driver or is there still a cache of
> some form ?
What enforcement are you referring to? I was envisioning no cached
directions at the gpiolib level, eg replace all the current references
that check desc->flag for direction with calls to chip->get_direction().
If a GPIO chip's hardware didn't support reading the direction of a pin
(eg the pcf857x chip), it would have to use its own caching to maintain
an accurate representation of its pins directions. Is this concept in
line with what you were picturing?
Best,
Peter
On Mon, 2011-03-07 at 18:38 -0600, Peter Tyser wrote:
> On Mon, 2011-03-07 at 00:08 -0700, Grant Likely wrote:
> > On Sun, Mar 06, 2011 at 09:07:21PM -0600, Peter Tyser wrote:
> > > On Sun, 2011-03-06 at 00:30 -0700, Grant Likely wrote:
> > > > On Tue, Mar 01, 2011 at 05:28:18PM -0600, Peter Tyser wrote:
> > > > > Add a new get_direction() function to the gpio_chip structure. This is
> > > > > useful so that the direction of a GPIO can be determined when its
> > > > > initially exported. Previously, the direction defaulted to "unknown"
> > > > > regardless of the actual configuration of the GPIO.
> > > > >
> > > > > If a GPIO driver implements get_direction(), it is called in
> > > > > gpio_request() to set the initial direction of the GPIO accurately.
> > > >
> > > > I don't like this approach. It effectively creates two methods for
> > > > determining the direction of a gpio; either ask the driver, or read
> > > > the flags value. Currently, only gpio_request() actually uses the
> > > > first option, but it still leaves the ambiguity.
> > > >
> > > > Instead, the driver should be able to preload the direction flag at or
> > > > shortly after gpiochip_add() time. No need for a new callback hook
> > > > from what I can see.
> > >
> > > The callback hook was used because the gpio_desc[] structure was local
> > > to gpiolib.c. The code would have to be restructured a bit to allow
> > > drivers to set the flags themselves. I can do that if you'd prefer, but
> > > don't think it'd be all that much cleaner.
> >
> > Make it part of the gpiochip_add function, or add a function for
> > explicitly setting gpio directions. Should not require any
> > reorganization at all.
> >
> > > Also, I thought it made sense to read the direction of the GPIO in
> > > gpio_request() because that's the point that the GPIO comes under the
> > > GPIO subsystem's control. Prior to that, there's a chance the direction
> > > could be changed by platform code, or another driver, etc, so reading
> > > the direction in gpiochip_add() may result in out-of-date directions.
> >
> > That's the problem with gpiolib having sole responsibility of the
> > cache. It doesn't give the driver any recourse for changing things if
> > it needs to. The best solution might very well be to eliminate the
> > direction state flag entirely and force all gpiochip drivers to
> > populate a gpio_get_direction() callback, but that is a lot more work.
> >
> > > The reading of the direction could also be put in chip drivers'
> > > request() function. That would get rid of the callback and still ensure
> > > the direction is up-to-date.
> >
> > I don't object to a callback hook. My objection is how it is bodged
> > on to work around limitations to the direction being cached in the
> > flags variable. I want to see a solution that either depends entirely
> > on the callback, or completely fixes the problems with the cached
> > value by allowing the driver to update it.
>
> I agree it'd be nice to have each driver implement a get_direction()
> function and remove the FLAG_DIR_* flags altogether, but didn't want to
> do the work to 20 drivers which I don't use... Would you accept a
> series that:
> - Added "unknown" direction support (eg patch 1)
> - Added ability to get GPIO direction (eg patch 2 in this series)
> - Sequence of patches to add get_direction() to all GPIO drivers (most
> of which I couldn't test).
> - Replace checking of FLAG_DIR_* in gpiolib.c with calls to new
> get_direction() function and remove FLAG_DIR_* flags all together.
>
>
> It should be relatively easy to add get_direction() to most drivers, and
> I'm willing to do it if all parties would agree it was the proper
> direction to take. Or do you have any other suggestions about an easier
> migration path?
Ping. Are you OK with this plan Grant? Just want to make sure we're on
the same page before starting any more work.
Thanks,
Peter
On Mon, Mar 07, 2011 at 06:38:49PM -0600, Peter Tyser wrote:
> On Mon, 2011-03-07 at 00:08 -0700, Grant Likely wrote:
> > On Sun, Mar 06, 2011 at 09:07:21PM -0600, Peter Tyser wrote:
> > > On Sun, 2011-03-06 at 00:30 -0700, Grant Likely wrote:
> > > > On Tue, Mar 01, 2011 at 05:28:18PM -0600, Peter Tyser wrote:
> > > > > Add a new get_direction() function to the gpio_chip structure. This is
> > > > > useful so that the direction of a GPIO can be determined when its
> > > > > initially exported. Previously, the direction defaulted to "unknown"
> > > > > regardless of the actual configuration of the GPIO.
> > > > >
> > > > > If a GPIO driver implements get_direction(), it is called in
> > > > > gpio_request() to set the initial direction of the GPIO accurately.
> > > >
> > > > I don't like this approach. It effectively creates two methods for
> > > > determining the direction of a gpio; either ask the driver, or read
> > > > the flags value. Currently, only gpio_request() actually uses the
> > > > first option, but it still leaves the ambiguity.
> > > >
> > > > Instead, the driver should be able to preload the direction flag at or
> > > > shortly after gpiochip_add() time. No need for a new callback hook
> > > > from what I can see.
> > >
> > > The callback hook was used because the gpio_desc[] structure was local
> > > to gpiolib.c. The code would have to be restructured a bit to allow
> > > drivers to set the flags themselves. I can do that if you'd prefer, but
> > > don't think it'd be all that much cleaner.
> >
> > Make it part of the gpiochip_add function, or add a function for
> > explicitly setting gpio directions. Should not require any
> > reorganization at all.
> >
> > > Also, I thought it made sense to read the direction of the GPIO in
> > > gpio_request() because that's the point that the GPIO comes under the
> > > GPIO subsystem's control. Prior to that, there's a chance the direction
> > > could be changed by platform code, or another driver, etc, so reading
> > > the direction in gpiochip_add() may result in out-of-date directions.
> >
> > That's the problem with gpiolib having sole responsibility of the
> > cache. It doesn't give the driver any recourse for changing things if
> > it needs to. The best solution might very well be to eliminate the
> > direction state flag entirely and force all gpiochip drivers to
> > populate a gpio_get_direction() callback, but that is a lot more work.
> >
> > > The reading of the direction could also be put in chip drivers'
> > > request() function. That would get rid of the callback and still ensure
> > > the direction is up-to-date.
> >
> > I don't object to a callback hook. My objection is how it is bodged
> > on to work around limitations to the direction being cached in the
> > flags variable. I want to see a solution that either depends entirely
> > on the callback, or completely fixes the problems with the cached
> > value by allowing the driver to update it.
>
> I agree it'd be nice to have each driver implement a get_direction()
> function and remove the FLAG_DIR_* flags altogether, but didn't want to
> do the work to 20 drivers which I don't use... Would you accept a
> series that:
> - Added "unknown" direction support (eg patch 1)
> - Added ability to get GPIO direction (eg patch 2 in this series)
> - Sequence of patches to add get_direction() to all GPIO drivers (most
> of which I couldn't test).
> - Replace checking of FLAG_DIR_* in gpiolib.c with calls to new
> get_direction() function and remove FLAG_DIR_* flags all together.
Yes, this sounds perfect to me.
g.
On Wed, Mar 09, 2011 at 04:53:56PM -0600, Peter Tyser wrote:
> On Tue, 2011-03-08 at 12:13 +0000, Alan Cox wrote:
> > > I don't object to a callback hook. My objection is how it is bodged
> > > on to work around limitations to the direction being cached in the
> > > flags variable. I want to see a solution that either depends entirely
> > > on the callback, or completely fixes the problems with the cached
> > > value by allowing the driver to update it.
> >
> > Doing it all by callback might actually fix a lot of the problems because
> > it can handle all kinds of 'unknowns'. If the callbacks for set/get
> > optionally pass a char buffer as well even the /proc interface just comes
> > out in the wash as the device can return a string to populate the status
> > or to be parsed (obviously with most h/w using the default method which
> > is in/out)
>
> I like the callback-only implementation also.
>
> > However who then does the enforcement of gpio_foo calls if the flag is
> > not cached, does that end up in each driver or is there still a cache of
> > some form ?
>
> What enforcement are you referring to? I was envisioning no cached
> directions at the gpiolib level, eg replace all the current references
> that check desc->flag for direction with calls to chip->get_direction().
> If a GPIO chip's hardware didn't support reading the direction of a pin
> (eg the pcf857x chip), it would have to use its own caching to maintain
> an accurate representation of its pins directions. Is this concept in
> line with what you were picturing?
I agree. Trash the cache in gpiolib. Make drivers do it (or use a
stock library function if they don't have the facility).
g.
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), and NM10 (Tiger Point).
Additional Intel chipsets should be easily supported if needed, eg the
ICH1-5, EP80579, etc.
Tested on a QM57 (Ibex Peak), 3100 (Whitmore Lake) , and
NM10 (Tiger Point).
Cc: Alek Du <[email protected]>
Cc: Samuel Ortiz <[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]>
Cc: Syed S Azam <[email protected]>
Signed-off-by: Peter Tyser <[email protected]>
Signed-off-by: Vincent Palatin <[email protected]>
Tested-by: Vincent Palatin <[email protected]>
---
Changes since v1:
- Fixed grammar in Kconfig menu entry
Changes since v2:
- Use GPIOF_DIR_* defines
Changes since v3:
- Added Vincent's signed-off/tested-by
- Added Tigerpoint PCI ID
Changes since v4:
- Integrated feedback from Joe:
- Fixed comment before ;
- Fixed bridge typo
- Updated to use pr_fmt and pr_<level>
Changes since v5:
- Listed NM10 in Kconfig description
- Removed get_direction() support
- The get_direction() support was a new feature that is
not in the mainline kernel and I didn't want it to
gate adding this driver as a number of people have
asked about it off-list
I still plan on submitting support for get_direction(), just
been busy at work recently.
MAINTAINERS | 5 +
drivers/gpio/Kconfig | 11 +
drivers/gpio/Makefile | 1 +
drivers/gpio/ichx_gpio.c | 545 ++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 562 insertions(+), 0 deletions(-)
create mode 100644 drivers/gpio/ichx_gpio.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 1e2724e..29fd490 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3135,6 +3135,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 d3b2953..dc80605 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -322,6 +322,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), NM10 (Tiger Point), and 3100 (Whitmore Lake).
+
+ If unsure, say N.
+
config GPIO_LANGWELL
bool "Intel Langwell/Penwell GPIO support"
depends on PCI && X86
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index becef59..23d8bf6 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..0f815c5
--- /dev/null
+++ b/drivers/gpio/ichx_gpio.c
@@ -0,0 +1,545 @@
+/*
+ * 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.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#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(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
+ * bridge 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->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) {
+ pr_err("GPIO BAR not enumerated\n");
+ 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)
+ pr_warn("ACPI BAR not enumerated, GPI 0 - 15 "
+ "unusable\n");
+ }
+
+ /* 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")) {
+ pr_err("Can't request iomem (0x%lx)\n", ichx_priv.gpio_base);
+ return -EBUSY;
+ }
+
+ ichx_gpiolib_setup(&ichx_priv.chip);
+
+ err = gpiochip_add(&ichx_priv.chip);
+ if (err) {
+ pr_err("Failed to register GPIOs\n");
+ return err;
+ }
+
+ pr_info("GPIO from %d to %d on %s\n", 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_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) },
+ { 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
> > > I don't object to a callback hook. My objection is how it is bodged
> > > on to work around limitations to the direction being cached in the
> > > flags variable. I want to see a solution that either depends entirely
> > > on the callback, or completely fixes the problems with the cached
> > > value by allowing the driver to update it.
> >
> > I agree it'd be nice to have each driver implement a get_direction()
> > function and remove the FLAG_DIR_* flags altogether, but didn't want to
> > do the work to 20 drivers which I don't use... Would you accept a
> > series that:
> > - Added "unknown" direction support (eg patch 1)
> > - Added ability to get GPIO direction (eg patch 2 in this series)
> > - Sequence of patches to add get_direction() to all GPIO drivers (most
> > of which I couldn't test).
> > - Replace checking of FLAG_DIR_* in gpiolib.c with calls to new
> > get_direction() function and remove FLAG_DIR_* flags all together.
>
> Yes, this sounds perfect to me.
I had a chance to look into this today, but had some follow up questions
and comments. There are a lot more GPIO drivers than expected... How
do the 94 driver changes get tested and applied? I can compile test the
common drivers in drivers/gpio and under arch/powerpc but I don't have
cross compile environments for other architectures and wading through
the config options to enable each driver would be very time consuming to
do manually. I also can only run-test a handful of the changes. Most
of the changes are pretty small and straightforward, but I likely missed
a semi-colon here, misspelled a define there, etc.
- Is there a kernel build test server that could be used to weed out any
compile issues?
- Should I post the patches to a website and email each driver's
maintainer for an ACK? Spamming the lkml with 100 trivial patches seems
like a bad idea.
- If the last patch "gpiolib: Get rid of direction flag" isn't applied,
all drivers would maintain their current functionality as a fallback,
which means the 94 driver-specific changes wouldn't need to be applied
right away.
Any input on how to proceed would be appreciated. Below is a list of
how the patches are broken up:
gpiolib: Add gpio_get_direction()
Consolidate direction detection into one function which is globally
visible. A GPIO's direction is currently still stored in as an
input/output flag, but this change lays the groundwork to allow GPIO
chips to accurately report their direction in realtime.
The new gpio_get_direction() function returns GPIOF_DIR_IN,
GPIOF_DIR_OUT, or a negative errno if the direction can't be determined.
---
gpiolib: Add chip->get_direction() support
Add a new get_direction() function to the gpio_chip structure. This is
useful so that the direction of a GPIO can always be acurately
determined. Previously incorrect directions could be reported prior
to explicitly setting a GPIO's direction, or if its direction was
changed elsewhere, eg in platform code.
At this point it is optional for a chip driver to implement
get_direction(). If get_direction() is not implemented for a GPIO, the
previous method of using a software cached direction will be utilized to
determine its direction.
---
gpio: tnetv107x: Add get_direction()
gpio: scoop: Add get_direction()
gpio: at91: Add get_direction()
gpio: via: Add get_direction()
gpio: max3107: Add get_direction()
gpio: pfc: Add get_direction()
gpio: intel_pmic: Add get_direction()
gpio: ucb1x00: Add get_direction()
gpio: tps65010: Add get_direction()
gpio: tc6393xb: Add get_direction()
gpio: davinci: Add get_direction()
gpio: ep93xx: Add get_direction()
gpio: gemini: Add get_direction()
gpio: ks8695: Add get_direction()
gpio: lpc32xx: Add get_direction()
gpio: msm/trout: Add get_direction()
gpio: msm/gpio-v2: Add get_direction()
gpio: msm/gpio: Add get_direction()
gpio: mxs/gpio: Add get_direction()
gpio: s5p64x0: Add get_direction()
gpio: sa1100: Add get_direction()
gpio: tegra: Add get_direction()
gpio: w90x900: Add get_direction()
gpio: iop: Add get_direction()
gpio: mxc: Add get_direction()
gpio: nomadik: Add get_direction()
gpio: omap: Add get_direction()
gpio: orion: Add get_direction()
gpio: pxa: Add get_direction()
gpio: samsung gpio: Add get_direction()
gpio: samsung gpiolib: Add get_direction()
gpio: stmp3xxx: Add get_direction()
gpio: at32ap: Add get_direction()
gpio: bfin: Add get_direction()
gpio: bf538: Add get_direction()
gpio: coldfire: Add get_direction()
gpio: au1000: Add get_direction()
gpio: ar7: Add get_direction()
gpio: ath79: Add get_direction()
gpio: bcm63xx: Add get_direction()
gpio: jz4740: Add get_direction()
gpio: txx9: Add get_direction()
gpio: loongson: Add get_direction()
gpio: msp71xx: Add get_direction()
gpio: msp71xx-ext: Add get_direction()
gpio: rb532: Add get_direction()
gpio: mpc52xx_gpio: Add get_direction()
gpio: mpc52xx_gpt: Add get_direction()
gpio: gef_gpio: Add get_direction()
gpio: cpm1: Add get_direction()
gpio: cpm_common: Add get_direction()
gpio: mpc8xxx_gpio: Add get_direction()
gpio: ppc4xx_gpio: Add get_direction()
gpio: qe: Add get_direction()
gpio: simple_gpio: Add get_direction()
gpio: s6000: Add get_direction()
gpio: adp5520: Add get_direction()
gpio: adp5588: Add get_direction()
gpio: basic_mmio: Add get_direction()
gpio: bt8xx: Add get_direction()
gpio: cs5535: Add get_direction()
gpio: ichx_gpio: Add get_direction()
gpio: it8761e: Add get_direction()
gpio: langwell: Add get_direction()
gpio: max730x: Add get_direction()
gpio: max732x: Add get_direction()
gpio: mcp23s08: Add get_direction()
gpio: ml_ioh: Add get_direction()
gpio: pca953x: Add get_direction()
gpio: pcf857x: Add get_direction()
gpio: pch: Add get_direction()
gpio: pl061: Add get_direction()
gpio: rdc321x: Add get_direction()
gpio: sch: Add get_direction()
gpio: stmpe: Add get_direction()
gpio: sx150x: Add get_direction()
gpio: tc3589x: Add get_direction()
gpio: timbgpio: Add get_direction()
gpio: twl4030: Add get_direction()
gpio: ucb1400: Add get_direction()
gpio: vr41xx_giu: Add get_direction()
gpio: vx855: Add get_direction()
gpio: wm831x: Add get_direction()
gpio: wm8350: Add get_direction()
gpio: wm8994: Add get_direction()
gpio: xilinx: Add get_direction()
gpio: adp5588-keys: Add get_direction()
gpio: ad7879: Add get_direction()
gpio: asic3: Add get_direction()
gpio: dm355evm_msp: Add get_direction()
gpio: htc-egpio: Add get_direction()
gpio: htc-i2cpld: Add get_direction()
gpio: sm501: Add get_direction()
gpiolib: Get rid of direction flag
Previously gpiolib used a cached flag (FLAG_IS_OUT) which was used to
determine the direction of a given GPIO. There were a few shortcomings
of this method:
- The initial direction of a GPIO could not be determined and was often
incorrectly reported for GPIOs exported via sysfs.
- If platform code changed the direction of a GPIO its flag would not be
updated and would contain false information.
The updated logic to determine a GPIO's direction is now:
- If possible, call the GPIO chip driver's get_direction() function to
dynamically determine pin direction.
- Else try and determine the direction of the pin based on the
presence/lack of the GPIO chip driver's direction_output() and
direction_input() functions.
- Else report an invalid/unknown GPIO direction. A GPIO that reports
an unknown direction can stil be used like a normal GPIO, but its
current direction can't be determined.
If a GPIO chip will be exported via sysfs it is highly recommended that
the chip driver implement get_direction() as some of the sysfs
functionality relies on being able to determine a GPIO's direction.
On Wed, Apr 20, 2011 at 18:17, Peter Tyser wrote:
> I had a chance to look into this today, but had some follow up questions
> and comments. There are a lot more GPIO drivers than expected... How
> do the 94 driver changes get tested and applied?
one possibility:
- one commit to add core functionality
- one commit to convert all of the drivers under drivers/gpio/
- one commit per arch/<arch>/
the first one you'd cc all arch maintainers, and the last one you can
cc each arch maintainer. for the 2nd one, you can cc all the relevant
gpio people.
-mike
On Wed, 2011-04-20 at 11:35 -0500, Peter Tyser wrote:
> 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), and NM10 (Tiger Point).
>
> Additional Intel chipsets should be easily supported if needed, eg the
> ICH1-5, EP80579, etc.
>
> Tested on a QM57 (Ibex Peak), 3100 (Whitmore Lake) , and
> NM10 (Tiger Point).
>
> Cc: Alek Du <[email protected]>
> Cc: Samuel Ortiz <[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]>
> Cc: Syed S Azam <[email protected]>
> Signed-off-by: Peter Tyser <[email protected]>
> Signed-off-by: Vincent Palatin <[email protected]>
> Tested-by: Vincent Palatin <[email protected]>
Ping...
On Wed, Apr 20, 2011 at 11:35:54AM -0500, Peter Tyser wrote:
> 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), and NM10 (Tiger Point).
>
> Additional Intel chipsets should be easily supported if needed, eg the
> ICH1-5, EP80579, etc.
>
> Tested on a QM57 (Ibex Peak), 3100 (Whitmore Lake) , and
> NM10 (Tiger Point).
>
> Cc: Alek Du <[email protected]>
> Cc: Samuel Ortiz <[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]>
> Cc: Syed S Azam <[email protected]>
> Signed-off-by: Peter Tyser <[email protected]>
> Signed-off-by: Vincent Palatin <[email protected]>
> Tested-by: Vincent Palatin <[email protected]>
Hmmm, I merged a patch from Jean Delvare adding support for Intel
82801 gpio pins[1]. Does this driver support the same hardware? I see
the same PCI ids.
[1] https://lkml.org/lkml/2011/4/19/170
Also, another comment below about module init.
> ---
> Changes since v1:
> - Fixed grammar in Kconfig menu entry
>
> Changes since v2:
> - Use GPIOF_DIR_* defines
>
> Changes since v3:
> - Added Vincent's signed-off/tested-by
> - Added Tigerpoint PCI ID
>
> Changes since v4:
> - Integrated feedback from Joe:
> - Fixed comment before ;
> - Fixed bridge typo
> - Updated to use pr_fmt and pr_<level>
>
> Changes since v5:
> - Listed NM10 in Kconfig description
> - Removed get_direction() support
> - The get_direction() support was a new feature that is
> not in the mainline kernel and I didn't want it to
> gate adding this driver as a number of people have
> asked about it off-list
>
> I still plan on submitting support for get_direction(), just
> been busy at work recently.
>
> MAINTAINERS | 5 +
> drivers/gpio/Kconfig | 11 +
> drivers/gpio/Makefile | 1 +
> drivers/gpio/ichx_gpio.c | 545 ++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 562 insertions(+), 0 deletions(-)
> create mode 100644 drivers/gpio/ichx_gpio.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1e2724e..29fd490 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3135,6 +3135,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 d3b2953..dc80605 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -322,6 +322,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), NM10 (Tiger Point), and 3100 (Whitmore Lake).
> +
> + If unsure, say N.
> +
> config GPIO_LANGWELL
> bool "Intel Langwell/Penwell GPIO support"
> depends on PCI && X86
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index becef59..23d8bf6 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..0f815c5
> --- /dev/null
> +++ b/drivers/gpio/ichx_gpio.c
> @@ -0,0 +1,545 @@
> +/*
> + * 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.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#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(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
> + * bridge 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->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) {
> + pr_err("GPIO BAR not enumerated\n");
> + 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)
> + pr_warn("ACPI BAR not enumerated, GPI 0 - 15 "
> + "unusable\n");
> + }
> +
> + /* 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")) {
> + pr_err("Can't request iomem (0x%lx)\n", ichx_priv.gpio_base);
> + return -EBUSY;
> + }
> +
> + ichx_gpiolib_setup(&ichx_priv.chip);
> +
> + err = gpiochip_add(&ichx_priv.chip);
> + if (err) {
> + pr_err("Failed to register GPIOs\n");
> + return err;
> + }
> +
> + pr_info("GPIO from %d to %d on %s\n", 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_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) },
> + { 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;
> + }
Ugh, this always makes me nervous. Very rarely should drivers be
registering their own devices in the module_init routine.
It /looks/ like the driver should be a pci_driver, and bind against
a pci device.
> +
> + 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
>
Hi Grant,
On Fri, 27 May 2011 00:42:38 -0600, Grant Likely wrote:
> On Wed, Apr 20, 2011 at 11:35:54AM -0500, Peter Tyser wrote:
> > 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), and NM10 (Tiger Point).
> >
> > Additional Intel chipsets should be easily supported if needed, eg the
> > ICH1-5, EP80579, etc.
> >
> > Tested on a QM57 (Ibex Peak), 3100 (Whitmore Lake) , and
> > NM10 (Tiger Point).
> >
> > Cc: Alek Du <[email protected]>
> > Cc: Samuel Ortiz <[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]>
> > Cc: Syed S Azam <[email protected]>
> > Signed-off-by: Peter Tyser <[email protected]>
> > Signed-off-by: Vincent Palatin <[email protected]>
> > Tested-by: Vincent Palatin <[email protected]>
>
> Hmmm, I merged a patch from Jean Delvare adding support for Intel
> 82801 gpio pins[1]. Does this driver support the same hardware? I see
> the same PCI ids.
>
> [1] https://lkml.org/lkml/2011/4/19/170
There is indeed a common range in the supported devices: ICH6 to ICH10.
My driver also supports older ICH chips (ICH to ICH5), while Peter's
support newer devices my driver does not (basically everything after
the ICH10).
Another key difference is that my driver is a simple PCI driver, while
Peter's is a platform driver. It makes some sense to have a platform
driver because the PCI device is a multifunction device so other
drivers may want to bind to it. However, I suspect that the other
functions (ACPI?) will never need a driver (not in the Linux device
driver binding model sense of the term at least) which is why I did not
bother. Peter, what was you reason to go for a platform driver? If you
really want to it go that route, you'll have to follow the standard MFD
model (see drivers/mfd/lpc_sch.c for an example.)
The only device I really care to see supported at the moment is the
ICH10, and it is supported by both drivers, so I don't care too much
which driver is picked.
--
Jean Delvare
On Fri, 2011-05-27 at 11:01 +0200, Jean Delvare wrote:
> Hi Grant,
>
> On Fri, 27 May 2011 00:42:38 -0600, Grant Likely wrote:
> > On Wed, Apr 20, 2011 at 11:35:54AM -0500, Peter Tyser wrote:
> > > 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), and NM10 (Tiger Point).
> > >
> > > Additional Intel chipsets should be easily supported if needed, eg the
> > > ICH1-5, EP80579, etc.
> > >
> > > Tested on a QM57 (Ibex Peak), 3100 (Whitmore Lake) , and
> > > NM10 (Tiger Point).
> > >
> > > Cc: Alek Du <[email protected]>
> > > Cc: Samuel Ortiz <[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]>
> > > Cc: Syed S Azam <[email protected]>
> > > Signed-off-by: Peter Tyser <[email protected]>
> > > Signed-off-by: Vincent Palatin <[email protected]>
> > > Tested-by: Vincent Palatin <[email protected]>
> >
> > Hmmm, I merged a patch from Jean Delvare adding support for Intel
> > 82801 gpio pins[1]. Does this driver support the same hardware? I see
> > the same PCI ids.
> >
> > [1] https://lkml.org/lkml/2011/4/19/170
>
> There is indeed a common range in the supported devices: ICH6 to ICH10.
> My driver also supports older ICH chips (ICH to ICH5), while Peter's
> support newer devices my driver does not (basically everything after
> the ICH10).
>
> Another key difference is that my driver is a simple PCI driver, while
> Peter's is a platform driver. It makes some sense to have a platform
> driver because the PCI device is a multifunction device so other
> drivers may want to bind to it. However, I suspect that the other
> functions (ACPI?) will never need a driver (not in the Linux device
> driver binding model sense of the term at least) which is why I did not
> bother. Peter, what was you reason to go for a platform driver? If you
> really want to it go that route, you'll have to follow the standard MFD
> model (see drivers/mfd/lpc_sch.c for an example.)
>
> The only device I really care to see supported at the moment is the
> ICH10, and it is supported by both drivers, so I don't care too much
> which driver is picked.
I went with a platform driver because the PCI device IDs we're searching
for don't contain the "GPIO registers" - they just have pointer to the
GPIO registers which reside in IO space, in addition to other non-GPIO
registers as Jean mentioned. So conceptually a platform driver made
more sense to me since we're not claiming or using the PCI device, we're
just using it to get a pointer.
More importantly, the PCI device that contains the pointer to the GPIO
IO space also has other uses that prevent it from being claimed. Eg the
drivers/mtd/maps/esb2rom.c driver uses the same PCI device to configure
firmware hub registers. The esb2rom.c driver isn't a PCI driver either,
so it wouldn't cause a conflict, but it seemed to support the idea that
we shouldn't be using a PCI driver.
The drivers/wdt/iTCO_wdt.c file also uses the same PCI device to get the
ACPI base (similar to us getting the GPIO base), and it is implemented
as a platform driver.
What determines if the driver should use the mfd or platform model? Eg
the iTCO_wdt.c that I used as an example doesn't use the mfd model
despite using the PCI device the same as we are.
My own (completely biased) preference would be to use my driver as a
starting point, primarily because it supports newer chips that require
different access methods, as well as the older-style chips Jean's
supports. Its also been hanging out for about 6 months without many
complaints, a tested-by, etc.
Best,
Peter
* The original work was done on an Intel 3100 with PCI device ID
PCI_DEVICE_ID_INTEL_ESB2_0, datasheet at
http://download.intel.com/design/intarch/datashts/31345803.pdf, see
section 16.2.
On Fri, May 27, 2011 at 09:26:42AM -0500, Peter Tyser wrote:
> On Fri, 2011-05-27 at 11:01 +0200, Jean Delvare wrote:
> > Hi Grant,
> >
> > On Fri, 27 May 2011 00:42:38 -0600, Grant Likely wrote:
> > > On Wed, Apr 20, 2011 at 11:35:54AM -0500, Peter Tyser wrote:
> > > > 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), and NM10 (Tiger Point).
> > > >
> > > > Additional Intel chipsets should be easily supported if needed, eg the
> > > > ICH1-5, EP80579, etc.
> > > >
> > > > Tested on a QM57 (Ibex Peak), 3100 (Whitmore Lake) , and
> > > > NM10 (Tiger Point).
> > > >
> > > > Cc: Alek Du <[email protected]>
> > > > Cc: Samuel Ortiz <[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]>
> > > > Cc: Syed S Azam <[email protected]>
> > > > Signed-off-by: Peter Tyser <[email protected]>
> > > > Signed-off-by: Vincent Palatin <[email protected]>
> > > > Tested-by: Vincent Palatin <[email protected]>
> > >
> > > Hmmm, I merged a patch from Jean Delvare adding support for Intel
> > > 82801 gpio pins[1]. Does this driver support the same hardware? I see
> > > the same PCI ids.
> > >
> > > [1] https://lkml.org/lkml/2011/4/19/170
> >
> > There is indeed a common range in the supported devices: ICH6 to ICH10.
> > My driver also supports older ICH chips (ICH to ICH5), while Peter's
> > support newer devices my driver does not (basically everything after
> > the ICH10).
> >
> > Another key difference is that my driver is a simple PCI driver, while
> > Peter's is a platform driver. It makes some sense to have a platform
> > driver because the PCI device is a multifunction device so other
> > drivers may want to bind to it. However, I suspect that the other
> > functions (ACPI?) will never need a driver (not in the Linux device
> > driver binding model sense of the term at least) which is why I did not
> > bother. Peter, what was you reason to go for a platform driver? If you
> > really want to it go that route, you'll have to follow the standard MFD
> > model (see drivers/mfd/lpc_sch.c for an example.)
> >
> > The only device I really care to see supported at the moment is the
> > ICH10, and it is supported by both drivers, so I don't care too much
> > which driver is picked.
>
> I went with a platform driver because the PCI device IDs we're searching
> for don't contain the "GPIO registers" - they just have pointer to the
> GPIO registers which reside in IO space, in addition to other non-GPIO
> registers as Jean mentioned. So conceptually a platform driver made
> more sense to me since we're not claiming or using the PCI device, we're
> just using it to get a pointer.
>
> More importantly, the PCI device that contains the pointer to the GPIO
> IO space also has other uses that prevent it from being claimed. Eg the
> drivers/mtd/maps/esb2rom.c driver uses the same PCI device to configure
> firmware hub registers. The esb2rom.c driver isn't a PCI driver either,
> so it wouldn't cause a conflict, but it seemed to support the idea that
> we shouldn't be using a PCI driver.
>
> The drivers/wdt/iTCO_wdt.c file also uses the same PCI device to get the
> ACPI base (similar to us getting the GPIO base), and it is implemented
> as a platform driver.
>
> What determines if the driver should use the mfd or platform model? Eg
> the iTCO_wdt.c that I used as an example doesn't use the mfd model
> despite using the PCI device the same as we are.
An mfd device is merely a bag of platform devices. It's fine to use a
platform_device, but it should be registered in the context of the pci
probe hook, not from the context of the module_init() hook.
> My own (completely biased) preference would be to use my driver as a
> starting point, primarily because it supports newer chips that require
> different access methods, as well as the older-style chips Jean's
> supports. Its also been hanging out for about 6 months without many
> complaints, a tested-by, etc.
I like the /structure/ of Jean's driver better, particularly that it doesn't
play games with the device model by registering devices at module load
time. If one device performs more than one function, then it should
register all the interfaces from a single probe, or go in the
direction of MFD devices and register a bunch of child
platform_devices.
I'm going to merge Jean's driver and leave this one out for now
(sorry), but I reserve the right to replace Jean's with your driver in
the next merge window. This is pretty much an arbitrary decision, but
I may as well merge at least one of them now instead of kicking both
out for another cycle. It really seams to me like there is still
a few architectural issues to sort out in both drivers and to make
sure that one driver will be useful for both of you.
g.
<snip>
> > More importantly, the PCI device that contains the pointer to the GPIO
> > IO space also has other uses that prevent it from being claimed. Eg the
> > drivers/mtd/maps/esb2rom.c driver uses the same PCI device to configure
> > firmware hub registers. The esb2rom.c driver isn't a PCI driver either,
> > so it wouldn't cause a conflict, but it seemed to support the idea that
> > we shouldn't be using a PCI driver.
> >
> > The drivers/wdt/iTCO_wdt.c file also uses the same PCI device to get the
> > ACPI base (similar to us getting the GPIO base), and it is implemented
> > as a platform driver.
> >
> > What determines if the driver should use the mfd or platform model? Eg
> > the iTCO_wdt.c that I used as an example doesn't use the mfd model
> > despite using the PCI device the same as we are.
>
> An mfd device is merely a bag of platform devices. It's fine to use a
> platform_device, but it should be registered in the context of the pci
> probe hook, not from the context of the module_init() hook.
>
> > My own (completely biased) preference would be to use my driver as a
> > starting point, primarily because it supports newer chips that require
> > different access methods, as well as the older-style chips Jean's
> > supports. Its also been hanging out for about 6 months without many
> > complaints, a tested-by, etc.
>
> I like the /structure/ of Jean's driver better, particularly that it doesn't
> play games with the device model by registering devices at module load
> time.
But the driver shouldn't be a PCI driver - that's worse in my opinion
than a platform driver. Multiple drivers currently use the same PCI
device ID as Jean's - they can't all claim the PCI device, so why should
Jean's GPIO driver? It seems incorrect, and likely to break things in
the future. Mine has its warts too, but it doesn't have the possibility
of negatively affect other drivers at least, as well as supports more
hardware.
> If one device performs more than one function, then it should
> register all the interfaces from a single probe, or go in the
> direction of MFD devices and register a bunch of child
> platform_devices.
In this case its not really "one device performing more than one
function", its one device specifying the location of other devices. The
device being claimed is used for FWH and IRQ configuration and happens
to have a pointer to a GPIO device (among other things). Claiming it
for a GPIO device seems very wrong - it has very little to do with GPIO
at all.
> I'm going to merge Jean's driver and leave this one out for now
> (sorry), but I reserve the right to replace Jean's with your driver in
> the next merge window. This is pretty much an arbitrary decision, but
> I may as well merge at least one of them now instead of kicking both
> out for another cycle. It really seams to me like there is still
> a few architectural issues to sort out in both drivers and to make
> sure that one driver will be useful for both of you.
I disagree with the logic, and its a bit frustrating to have a patch
hanging out for 6 months to be superseded by a more recent change that
seems to be more likely to cause conflicts.
Best,
Peter
On Fri, May 27, 2011 at 3:29 PM, Peter Tyser <[email protected]> wrote:
>> > My own (completely biased) preference would be to use my driver as a
>> > starting point, primarily because it supports newer chips that require
>> > different access methods, as well as the older-style chips Jean's
>> > supports. ?Its also been hanging out for about 6 months without many
>> > complaints, a tested-by, etc.
>>
>> I like the /structure/ of Jean's driver better, particularly that it doesn't
>> play games with the device model by registering devices at module load
>> time.
>
> But the driver shouldn't be a PCI driver - that's worse in my opinion
> than a platform driver. ?Multiple drivers currently use the same PCI
> device ID as Jean's - they can't all claim the PCI device, so why should
> Jean's GPIO driver? ?It seems incorrect, and likely to break things in
> the future. ?Mine has its warts too, but it doesn't have the possibility
> of negatively affect other drivers at least, as well as supports more
> hardware.
Does it conflict with other drivers in the kernel right now? If so,
then I'll drop it.
Also, you're directly describing the use case for an MFD type device.
I certainly expect either this driver or Jean's driver to be reworked
into an MFD style device in the next cycle.
>> If one device performs more than one function, then it should
>> register all the interfaces from a single probe, or go in the
>> direction of MFD devices and register a bunch of child
>> platform_devices.
>
> In this case its not really "one device performing more than one
> function", its one device specifying the location of other devices. ?The
> device being claimed is used for FWH and IRQ configuration and happens
> to have a pointer to a GPIO device (among other things). ?Claiming it
> for a GPIO device seems very wrong - it has very little to do with GPIO
> at all.
Are you sure? My reading of the ICH10 manual is that the GPIO
controller is very much behind the LPC bridge, and that it is entirely
appropriate for the GPIO controller device to be registered by the PCI
driver as a child of the PCI device, or to be registered directly by
the PCI driver.
>
>> I'm going to merge Jean's driver and leave this one out for now
>> (sorry), but I reserve the right to replace Jean's with your driver in
>> the next merge window. ?This is pretty much an arbitrary decision, but
>> I may as well merge at least one of them now instead of kicking both
>> out for another cycle. ?It really seams to me like there is still
>> a few architectural issues to sort out in both drivers and to make
>> sure that one driver will be useful for both of you.
>
> I disagree with the logic, and its a bit frustrating to have a patch
> hanging out for 6 months to be superseded by a more recent change that
> seems to be more likely to cause conflicts.
Think about it this way; your patch wasn't going to go in this cycle
anyway due to the current concerns I have about structure, and I
haven't said no to it either in that I reserve the right to replace
Jean's version with a rework of this one in the next cycle if it makes
more sense to do so. I'm sorry that I wasn't able to look at the
patch before now, but that is unfortunately how things happen
sometimes. Believe me, I understand your frustration, but I cannot
merge this driver as is, and there isn't time to rework it before the
window closes.
I'm okay with Jean's patch because I like the structure and it looked
to be low risk. However, if I'm wrong about it and it will indeed
conflict with another driver then let me know and I'll remove it.
g.
On Fri, 2011-05-27 at 17:54 -0600, Grant Likely wrote:
> On Fri, May 27, 2011 at 3:29 PM, Peter Tyser <[email protected]> wrote:
> >> > My own (completely biased) preference would be to use my driver as a
> >> > starting point, primarily because it supports newer chips that require
> >> > different access methods, as well as the older-style chips Jean's
> >> > supports. Its also been hanging out for about 6 months without many
> >> > complaints, a tested-by, etc.
> >>
> >> I like the /structure/ of Jean's driver better, particularly that it doesn't
> >> play games with the device model by registering devices at module load
> >> time.
> >
> > But the driver shouldn't be a PCI driver - that's worse in my opinion
> > than a platform driver. Multiple drivers currently use the same PCI
> > device ID as Jean's - they can't all claim the PCI device, so why should
> > Jean's GPIO driver? It seems incorrect, and likely to break things in
> > the future. Mine has its warts too, but it doesn't have the possibility
> > of negatively affect other drivers at least, as well as supports more
> > hardware.
>
> Does it conflict with other drivers in the kernel right now? If so,
> then I'll drop it.
It'd take some searching of where all the PCI vendor IDs in the new
patch are used. I did a quick search of ICH7 and ESB2 IDs, and it
looked like:
Conflict:
- drivers/leds/leds-ss4200.c (also uses a PCI driver, also uses the GPIO
functionality of the chip)
Uses the same device, but no conflict:
- drivers/char/sonypi.c (uses platform driver)
- drivers/watchdog/iTCO_wdt.c (uses platform driver)
- drivers/mtd/maps/esb2rom.c (has PCI driver support commented out)
The same search would have to be performed for the other IDs to
determine if there are conflicts. So there is one conflict, but its
probably not a huge deal because someone using leds-ss4200.c likely
wouldn't be using the ich GPIO driver as well. leds-ss4200.c should
probably be converted to use the standard GPIO driver in the future.
However, if the above non-conflicting drivers did use the same PCI
driver model as the accepted GPIO patch, there would be conflicts -
that's what I don't care for. The drivers above are considerate of the
fact that the device being used is a MFD while the Jean's driver is not.
I followed the iTCO_wdt.c and sonypi.c models for my driver in an
attempt to play nice with other drivers that might require the same PCI
device.
> Also, you're directly describing the use case for an MFD type device.
> I certainly expect either this driver or Jean's driver to be reworked
> into an MFD style device in the next cycle.
I was following the very "popular" iTCO_wdt.c driver model. I'd hope
the current driver could be merged as it follows the current ICHx usage
pattern, and then they (and possibly other drivers) could all be
reworked at the same time into an MFD style driver.
> >> If one device performs more than one function, then it should
> >> register all the interfaces from a single probe, or go in the
> >> direction of MFD devices and register a bunch of child
> >> platform_devices.
> >
> > In this case its not really "one device performing more than one
> > function", its one device specifying the location of other devices. The
> > device being claimed is used for FWH and IRQ configuration and happens
> > to have a pointer to a GPIO device (among other things). Claiming it
> > for a GPIO device seems very wrong - it has very little to do with GPIO
> > at all.
>
> Are you sure? My reading of the ICH10 manual is that the GPIO
> controller is very much behind the LPC bridge, and that it is entirely
> appropriate for the GPIO controller device to be registered by the PCI
> driver as a child of the PCI device, or to be registered directly by
> the PCI driver.
I was a bit overzealous in my comment - you are correct that the GPIO
controller is behind the LPC bridge. However, there is a lot of other
stuff in the same device, from the Intel series 5 datasheet : "The LPC
bridge function of the PCH resides in PCI Device 31:Function 0. This
function contains many other functional units, such as DMA and Interrupt
controllers, Timers, Power Management, System Management, GPIO, RTC, and
LPC Configuration Registers."
Thus I still don't think its correct to claim the device as only a GPIO
device - its much more, as can be seen by the current drivers which use
the same device. I'm having a tough time seeing how an monopolistic PCI
driver is considered better than a platform driver that follows the
structure of other drivers in use.
> >
> >> I'm going to merge Jean's driver and leave this one out for now
> >> (sorry), but I reserve the right to replace Jean's with your driver in
> >> the next merge window. This is pretty much an arbitrary decision, but
> >> I may as well merge at least one of them now instead of kicking both
> >> out for another cycle. It really seams to me like there is still
> >> a few architectural issues to sort out in both drivers and to make
> >> sure that one driver will be useful for both of you.
> >
> > I disagree with the logic, and its a bit frustrating to have a patch
> > hanging out for 6 months to be superseded by a more recent change that
> > seems to be more likely to cause conflicts.
>
> Think about it this way; your patch wasn't going to go in this cycle
> anyway due to the current concerns I have about structure,
I attempted to follow the driver model of the existing iTCO_wdt.c file,
which at a minimum will not negatively affect other drivers unlike the
potential of the accepted driver. Any comments about the platform
driver GPIO approach can also be made to the iTCO_wdt.c driver, as well
as the sonypi.c driver. Since cleanup needs to be done anyway on those
drivers, my hope would be that the platform GPIO driver would be
accepted since it follows the status quo, and then cleanup on them all
would occur.
Best,
Peter
On Mon, May 30, 2011 at 11:27 AM, Peter Tyser <[email protected]> wrote:
> On Fri, 2011-05-27 at 17:54 -0600, Grant Likely wrote:
>> On Fri, May 27, 2011 at 3:29 PM, Peter Tyser <[email protected]> wrote:
>> >> > My own (completely biased) preference would be to use my driver as a
>> >> > starting point, primarily because it supports newer chips that require
>> >> > different access methods, as well as the older-style chips Jean's
>> >> > supports. ?Its also been hanging out for about 6 months without many
>> >> > complaints, a tested-by, etc.
>> >>
>> >> I like the /structure/ of Jean's driver better, particularly that it doesn't
>> >> play games with the device model by registering devices at module load
>> >> time.
>> >
>> > But the driver shouldn't be a PCI driver - that's worse in my opinion
>> > than a platform driver. ?Multiple drivers currently use the same PCI
>> > device ID as Jean's - they can't all claim the PCI device, so why should
>> > Jean's GPIO driver? ?It seems incorrect, and likely to break things in
>> > the future. ?Mine has its warts too, but it doesn't have the possibility
>> > of negatively affect other drivers at least, as well as supports more
>> > hardware.
>>
>> Does it conflict with other drivers in the kernel right now? ?If so,
>> then I'll drop it.
>
> It'd take some searching of where all the PCI vendor IDs in the new
> patch are used. ?I did a quick search of ICH7 and ESB2 IDs, and it
> looked like:
> Conflict:
> - drivers/leds/leds-ss4200.c (also uses a PCI driver, also uses the GPIO
> functionality of the chip)
>
> Uses the same device, but no conflict:
> - drivers/char/sonypi.c (uses platform driver)
> - drivers/watchdog/iTCO_wdt.c (uses platform driver)
> - drivers/mtd/maps/esb2rom.c (has PCI driver support commented out)
>
> The same search would have to be performed for the other IDs to
> determine if there are conflicts. ?So there is one conflict, but its
> probably not a huge deal because someone using leds-ss4200.c likely
> wouldn't be using the ich GPIO driver as well. ?leds-ss4200.c should
> probably be converted to use the standard GPIO driver in the future.
>
> However, if the above non-conflicting drivers did use the same PCI
> driver model as the accepted GPIO patch, there would be conflicts -
> that's what I don't care for. ?The drivers above are considerate of the
> fact that the device being used is a MFD while the Jean's driver is not.
>
> I followed the iTCO_wdt.c and sonypi.c models for my driver in an
> attempt to play nice with other drivers that might require the same PCI
> device.
>
>> Also, you're directly describing the use case for an MFD type device.
>> I certainly expect either this driver or Jean's driver to be reworked
>> into an MFD style device in the next cycle.
>
> I was following the very "popular" iTCO_wdt.c driver model. ?I'd hope
> the current driver could be merged as it follows the current ICHx usage
> pattern, and then they (and possibly other drivers) could all be
> reworked at the same time into an MFD style driver.
>
>> >> If one device performs more than one function, then it should
>> >> register all the interfaces from a single probe, or go in the
>> >> direction of MFD devices and register a bunch of child
>> >> platform_devices.
>> >
>> > In this case its not really "one device performing more than one
>> > function", its one device specifying the location of other devices. ?The
>> > device being claimed is used for FWH and IRQ configuration and happens
>> > to have a pointer to a GPIO device (among other things). ?Claiming it
>> > for a GPIO device seems very wrong - it has very little to do with GPIO
>> > at all.
>>
>> Are you sure? ?My reading of the ICH10 manual is that the GPIO
>> controller is very much behind the LPC bridge, and that it is entirely
>> appropriate for the GPIO controller device to be registered by the PCI
>> driver as a child of the PCI device, or to be registered directly by
>> the PCI driver.
>
> I was a bit overzealous in my comment - you are correct that the GPIO
> controller is behind the LPC bridge. ?However, there is a lot of other
> stuff in the same device, from the Intel series 5 datasheet : "The LPC
> bridge function of the PCH resides in PCI Device 31:Function 0. This
> function contains many other functional units, such as DMA and Interrupt
> controllers, Timers, Power Management, System Management, GPIO, RTC, and
> LPC Configuration Registers."
>
> Thus I still don't think its correct to claim the device as only a GPIO
> device - its much more, as can be seen by the current drivers which use
> the same device. ?I'm having a tough time seeing how an monopolistic PCI
> driver is considered better than a platform driver that follows the
> structure of other drivers in use.
>
>> >
>> >> I'm going to merge Jean's driver and leave this one out for now
>> >> (sorry), but I reserve the right to replace Jean's with your driver in
>> >> the next merge window. ?This is pretty much an arbitrary decision, but
>> >> I may as well merge at least one of them now instead of kicking both
>> >> out for another cycle. ?It really seams to me like there is still
>> >> a few architectural issues to sort out in both drivers and to make
>> >> sure that one driver will be useful for both of you.
>> >
>> > I disagree with the logic, and its a bit frustrating to have a patch
>> > hanging out for 6 months to be superseded by a more recent change that
>> > seems to be more likely to cause conflicts.
>>
>> Think about it this way; your patch wasn't going to go in this cycle
>> anyway due to the current concerns I have about structure,
>
> I attempted to follow the driver model of the existing iTCO_wdt.c file,
> which at a minimum will not negatively affect other drivers unlike the
> potential of the accepted driver. ?Any comments about the platform
> driver GPIO approach can also be made to the iTCO_wdt.c driver, as well
> as the sonypi.c driver.
Correct, those drivers are using a bad model and should also be fixed.
> ?Since cleanup needs to be done anyway on those
> drivers, my hope would be that the platform GPIO driver would be
> accepted since it follows the status quo, and then cleanup on them all
> would occur.
Ah, but those drivers are already merged, whereas this new driver is
not. It is perfectly reasonable for me to require that new drivers
follow good practices before getting merged. I'm not even asking you
to do something hard. Instead of registering both the device and the
driver in the same code, I'm asking you to create a new pci_driver in
drivers/mfd that registers the a child platform_device for each
embedded device, including the GPIO controller. Take a look at
drivers/mfd/timberdate.c as an example (although much more complicated
than you need, your version will probably be somewhere around 50-100
lines of code top to bottom including comments).
BTW, I also held off on asking Linus to merge Jean's version because
of the possibility of conflict (Sorry Jean). After the concern was
raised, I just didn't feel confident enough to push it on to Linus.
g.