[PATCH 1/2] cs5535-gpio: add AMD CS5535/CS5536 GPIO driver support

[PATCH 1/2] cs5535-gpio: add AMD CS5535/CS5536 GPIO driver support

[Andres Salomon]

I'm resubmitting this, as I actually have time to deal w/ any problems
it might have.  Randy, please do your worst (and send me a .config if
you find problems!).  Thanks.

Oh, also, I renamed it to cs5535-gpio at the request of q-funk, in
order to be consistent w/ naming standards for CS5530/CS5535/CS5536.



This creates a CS5535/CS5536 GPIO driver which uses a gpio_chip backend
(allowing GPIO users to use the generic GPIO API if desired) while also
allowing architecture-specific users directly (via the cs5535_gpio_*
functions).

Tested on an OLPC machine.  Some Leemotes also use CS5536 (with a mips
cpu), which is why this is in drivers/gpio rather than arch/x86.
Currently, it conflicts with older geode GPIO support; once MFGPT support
is reworked to also be more generic, the older geode code will be removed.

Signed-off-by: Andres Salomon <dilinger@collabora.co.uk>
---
 arch/x86/include/asm/geode.h |   28 +----
 drivers/gpio/Kconfig         |   10 ++
 drivers/gpio/Makefile        |    1 +
 drivers/gpio/cs5535-gpio.c   |  282 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/cs5535.h       |   58 +++++++++
 5 files changed, 352 insertions(+), 27 deletions(-)
 create mode 100644 drivers/gpio/cs5535-gpio.c
 create mode 100644 include/linux/cs5535.h

diff --git a/arch/x86/include/asm/geode.h b/arch/x86/include/asm/geode.h
index ad3c2ed..5716214 100644
--- a/arch/x86/include/asm/geode.h
+++ b/arch/x86/include/asm/geode.h
@@ -12,6 +12,7 @@
 
 #include <asm/processor.h>
 #include <linux/io.h>
+#include <linux/cs5535.h>
 
 /* Generic southbridge functions */
 
@@ -115,33 +116,6 @@ extern int geode_get_dev_base(unsigned int dev);
 #define VSA_VR_MEM_SIZE		0x0200
 #define AMD_VSA_SIG		0x4132	/* signature is ascii 'VSA2' */
 #define GSW_VSA_SIG		0x534d  /* General Software signature */
-/* GPIO */
-
-#define GPIO_OUTPUT_VAL		0x00
-#define GPIO_OUTPUT_ENABLE	0x04
-#define GPIO_OUTPUT_OPEN_DRAIN	0x08
-#define GPIO_OUTPUT_INVERT	0x0C
-#define GPIO_OUTPUT_AUX1	0x10
-#define GPIO_OUTPUT_AUX2	0x14
-#define GPIO_PULL_UP		0x18
-#define GPIO_PULL_DOWN		0x1C
-#define GPIO_INPUT_ENABLE	0x20
-#define GPIO_INPUT_INVERT	0x24
-#define GPIO_INPUT_FILTER	0x28
-#define GPIO_INPUT_EVENT_COUNT	0x2C
-#define GPIO_READ_BACK		0x30
-#define GPIO_INPUT_AUX1		0x34
-#define GPIO_EVENTS_ENABLE	0x38
-#define GPIO_LOCK_ENABLE	0x3C
-#define GPIO_POSITIVE_EDGE_EN	0x40
-#define GPIO_NEGATIVE_EDGE_EN	0x44
-#define GPIO_POSITIVE_EDGE_STS	0x48
-#define GPIO_NEGATIVE_EDGE_STS	0x4C
-
-#define GPIO_MAP_X		0xE0
-#define GPIO_MAP_Y		0xE4
-#define GPIO_MAP_Z		0xE8
-#define GPIO_MAP_W		0xEC
 
 static inline u32 geode_gpio(unsigned int nr)
 {
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index edb0253..185a458 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -145,6 +145,16 @@ config GPIO_TWL4030
 
 comment "PCI GPIO expanders:"
 
+config GPIO_CS5535
+	tristate "AMD CS5535/CS5536 GPIO support"
+	depends on PCI && !CS5535_GPIO && !MGEODE_LX
+	help
+	  The AMD CS5535 and CS5536 southbridges support 28 GPIO pins that
+	  can be used for quite a number of things.  The CS5535/6 is found on
+	  AMD Geode and Lemote Yeeloong devices.
+
+	  If unsure, say N.
+
 config GPIO_BT8XX
 	tristate "BT8XX GPIO abuser"
 	depends on PCI && VIDEO_BT848=n
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 49ac64e..c7bc4bd 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -11,4 +11,5 @@ obj-$(CONFIG_GPIO_PCA953X)	+= pca953x.o
 obj-$(CONFIG_GPIO_PCF857X)	+= pcf857x.o
 obj-$(CONFIG_GPIO_TWL4030)	+= twl4030-gpio.o
 obj-$(CONFIG_GPIO_XILINX)	+= xilinx_gpio.o
+obj-$(CONFIG_GPIO_CS5535)	+= cs5535-gpio.o
 obj-$(CONFIG_GPIO_BT8XX)	+= bt8xxgpio.o
diff --git a/drivers/gpio/cs5535-gpio.c b/drivers/gpio/cs5535-gpio.c
new file mode 100644
index 0000000..5613889
--- /dev/null
+++ b/drivers/gpio/cs5535-gpio.c
@@ -0,0 +1,282 @@
+/*
+ * AMD CS5535/CS5536 GPIO driver
+ * Copyright (C) 2006  Advanced Micro Devices, Inc.
+ * Copyright (C) 2007-2009  Andres Salomon <dilinger@collabora.co.uk>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of version 2 of the GNU General Public License
+ * as published by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/spinlock.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/gpio.h>
+#include <linux/io.h>
+#include <linux/cs5535.h>
+
+#define DRV_NAME "cs5535-gpio"
+#define GPIO_BAR 1
+
+static struct cs5535_gpio_chip {
+	struct gpio_chip chip;
+	resource_size_t base;
+
+	struct pci_dev *pdev;
+	spinlock_t lock;
+} cs5535_gpio_chip;
+
+/*
+ * The CS5535/CS5536 GPIOs support a number of extra features not defined
+ * by the gpio_chip API, so these are exported.  For a full list of the
+ * registers, see include/linux/cs5535.h.
+ */
+
+static void __cs5535_gpio_set(struct cs5535_gpio_chip *chip, unsigned offset,
+		unsigned int reg)
+{
+	if (offset < 16)
+		/* low bank register */
+		outl(1 << offset, chip->base + reg);
+	else
+		/* high bank register */
+		outl(1 << (offset - 16), chip->base + 0x80 + reg);
+}
+
+void cs5535_gpio_set(unsigned offset, unsigned int reg)
+{
+	struct cs5535_gpio_chip *chip = &cs5535_gpio_chip;
+	unsigned long flags;
+
+	spin_lock_irqsave(&chip->lock, flags);
+	__cs5535_gpio_set(chip, offset, reg);
+	spin_unlock_irqrestore(&chip->lock, flags);
+}
+EXPORT_SYMBOL_GPL(cs5535_gpio_set);
+
+static void __cs5535_gpio_clear(struct cs5535_gpio_chip *chip, unsigned offset,
+		unsigned int reg)
+{
+	if (offset < 16)
+		/* low bank register */
+		outl(1 << (offset + 16), chip->base + reg);
+	else
+		/* high bank register */
+		outl(1 << offset, chip->base + 0x80 + reg);
+}
+
+void cs5535_gpio_clear(unsigned offset, unsigned int reg)
+{
+	struct cs5535_gpio_chip *chip = &cs5535_gpio_chip;
+	unsigned long flags;
+
+	spin_lock_irqsave(&chip->lock, flags);
+	__cs5535_gpio_clear(chip, offset, reg);
+	spin_unlock_irqrestore(&chip->lock, flags);
+}
+EXPORT_SYMBOL_GPL(cs5535_gpio_clear);
+
+int cs5535_gpio_isset(unsigned offset, unsigned int reg)
+{
+	struct cs5535_gpio_chip *chip = &cs5535_gpio_chip;
+	unsigned long flags;
+	long val;
+
+	spin_lock_irqsave(&chip->lock, flags);
+	if (offset < 16)
+		/* low bank register */
+		val = inl(chip->base + reg);
+	else {
+		/* high bank register */
+		val = inl(chip->base + 0x80 + reg);
+		offset -= 16;
+	}
+	spin_unlock_irqrestore(&chip->lock, flags);
+
+	return (val & (1 << offset)) ? 1 : 0;
+}
+EXPORT_SYMBOL_GPL(cs5535_gpio_isset);
+
+/*
+ * Generic gpio_chip API support.
+ */
+
+static int chip_gpio_get(struct gpio_chip *chip, unsigned offset)
+{
+	return cs5535_gpio_isset(offset, GPIO_OUTPUT_VAL);
+}
+
+static void chip_gpio_set(struct gpio_chip *chip, unsigned offset, int val)
+{
+	if (val)
+		cs5535_gpio_set(offset, GPIO_OUTPUT_VAL);
+	else
+		cs5535_gpio_clear(offset, GPIO_OUTPUT_VAL);
+}
+
+static int chip_direction_input(struct gpio_chip *c, unsigned offset)
+{
+	struct cs5535_gpio_chip *chip = (struct cs5535_gpio_chip *) c;
+	unsigned long flags;
+
+	spin_lock_irqsave(&chip->lock, flags);
+	__cs5535_gpio_set(chip, offset, GPIO_INPUT_ENABLE);
+	spin_unlock_irqrestore(&chip->lock, flags);
+
+	return 0;
+}
+
+static int chip_direction_output(struct gpio_chip *c, unsigned offset, int val)
+{
+	struct cs5535_gpio_chip *chip = (struct cs5535_gpio_chip *) c;
+	unsigned long flags;
+
+	spin_lock_irqsave(&chip->lock, flags);
+
+	__cs5535_gpio_set(chip, offset, GPIO_OUTPUT_ENABLE);
+	if (val)
+		__cs5535_gpio_set(chip, offset, GPIO_OUTPUT_VAL);
+	else
+		__cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_VAL);
+
+	spin_unlock_irqrestore(&chip->lock, flags);
+
+	return 0;
+}
+
+static struct cs5535_gpio_chip cs5535_gpio_chip = {
+	.chip = {
+		.owner = THIS_MODULE,
+		.label = DRV_NAME,
+
+		.base = 0,
+		.ngpio = 28,
+
+		.get = chip_gpio_get,
+		.set = chip_gpio_set,
+
+		.direction_input = chip_direction_input,
+		.direction_output = chip_direction_output,
+	},
+};
+
+static int __init cs5535_gpio_probe(struct pci_dev *pdev,
+		const struct pci_device_id *pci_id)
+{
+	int err;
+
+	/* There are two ways to get the GPIO base address; one is by
+	 * fetching it from MSR_LBAR_GPIO, the other is by reading the
+	 * PCI BAR info.  The latter method is easier (especially across
+	 * different architectures), so we'll stick with that for now.  If
+	 * it turns out to be unreliable in the face of crappy BIOSes, we
+	 * can always go back to using MSRs.. */
+
+	err = pci_enable_device_io(pdev);
+	if (err) {
+		dev_err(&pdev->dev, "can't enable device IO\n");
+		goto done;
+	}
+
+	err = pci_request_region(pdev, GPIO_BAR, DRV_NAME);
+	if (err) {
+		dev_err(&pdev->dev, "can't alloc PCI BAR #%d\n", GPIO_BAR);
+		goto done;
+	}
+
+	/* set up the driver-specific struct */
+	cs5535_gpio_chip.base = pci_resource_start(pdev, GPIO_BAR);
+	cs5535_gpio_chip.pdev = pdev;
+	spin_lock_init(&cs5535_gpio_chip.lock);
+
+	dev_info(&pdev->dev, "allocated PCI BAR #%d: base 0x%llx\n", GPIO_BAR,
+			(unsigned long long) cs5535_gpio_chip.base);
+
+	/* finally, register with the generic GPIO API */
+	err = gpiochip_add(&cs5535_gpio_chip.chip);
+	if (err) {
+		dev_err(&pdev->dev, "failed to register gpio chip\n");
+		goto release_region;
+	}
+
+	printk(KERN_INFO DRV_NAME ": GPIO support successfully loaded.\n");
+	return 0;
+
+release_region:
+	pci_release_region(pdev, GPIO_BAR);
+done:
+	return err;
+}
+
+static void __exit cs5535_gpio_remove(struct pci_dev *pdev)
+{
+	int err;
+
+	err = gpiochip_remove(&cs5535_gpio_chip.chip);
+	if (err) {
+		/* uhh? */
+		dev_err(&pdev->dev, "unable to remove gpio_chip?\n");
+	}
+	pci_release_region(pdev, GPIO_BAR);
+}
+
+static struct pci_device_id cs5535_gpio_pci_tbl[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_NS, PCI_DEVICE_ID_NS_CS5535_ISA) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_CS5536_ISA) },
+	{ 0, },
+};
+MODULE_DEVICE_TABLE(pci, cs5535_gpio_pci_tbl);
+
+/*
+ * We can't use the standard PCI driver registration stuff here, since
+ * that allows only one driver to bind to each PCI device (and we want
+ * multiple drivers to be able to bind to the device).  Instead, manually
+ * scan for the PCI device, request a single region, and keep track of the
+ * devices that we're using.
+ */
+
+static int __init cs5535_gpio_scan_pci(void)
+{
+	struct pci_dev *pdev;
+	int err = -ENODEV;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(cs5535_gpio_pci_tbl); i++) {
+		pdev = pci_get_device(cs5535_gpio_pci_tbl[i].vendor,
+				cs5535_gpio_pci_tbl[i].device, NULL);
+		if (pdev) {
+			err = cs5535_gpio_probe(pdev, &cs5535_gpio_pci_tbl[i]);
+			if (err)
+				pci_dev_put(pdev);
+
+			/* we only support a single CS5535/6 southbridge */
+			break;
+		}
+	}
+
+	return err;
+}
+
+static void __exit cs5535_gpio_free_pci(void)
+{
+	cs5535_gpio_remove(cs5535_gpio_chip.pdev);
+	pci_dev_put(cs5535_gpio_chip.pdev);
+}
+
+static int __init cs5535_gpio_init(void)
+{
+	return cs5535_gpio_scan_pci();
+}
+
+static void __exit cs5535_gpio_exit(void)
+{
+	cs5535_gpio_free_pci();
+}
+
+module_init(cs5535_gpio_init);
+module_exit(cs5535_gpio_exit);
+
+MODULE_AUTHOR("Andres Salomon <dilinger@collabora.co.uk>");
+MODULE_DESCRIPTION("AMD CS5535/CS5536 GPIO driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/cs5535.h b/include/linux/cs5535.h
new file mode 100644
index 0000000..cfea689
--- /dev/null
+++ b/include/linux/cs5535.h
@@ -0,0 +1,58 @@
+/*
+ * AMD CS5535/CS5536 definitions
+ * Copyright (C) 2006  Advanced Micro Devices, Inc.
+ * Copyright (C) 2009  Andres Salomon <dilinger@collabora.co.uk>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of version 2 of the GNU General Public License
+ * as published by the Free Software Foundation.
+ */
+
+#ifndef _CS5535_H
+#define _CS5535_H
+
+/* MSRs */
+#define MSR_LBAR_SMB		0x5140000B
+#define MSR_LBAR_GPIO		0x5140000C
+#define MSR_LBAR_MFGPT		0x5140000D
+#define MSR_LBAR_ACPI		0x5140000E
+#define MSR_LBAR_PMS		0x5140000F
+
+/* resource sizes */
+#define LBAR_GPIO_SIZE		0xFF
+#define LBAR_MFGPT_SIZE		0x40
+#define LBAR_ACPI_SIZE		0x40
+#define LBAR_PMS_SIZE		0x80
+
+/* GPIOs */
+#define GPIO_OUTPUT_VAL		0x00
+#define GPIO_OUTPUT_ENABLE	0x04
+#define GPIO_OUTPUT_OPEN_DRAIN	0x08
+#define GPIO_OUTPUT_INVERT	0x0C
+#define GPIO_OUTPUT_AUX1	0x10
+#define GPIO_OUTPUT_AUX2	0x14
+#define GPIO_PULL_UP		0x18
+#define GPIO_PULL_DOWN		0x1C
+#define GPIO_INPUT_ENABLE	0x20
+#define GPIO_INPUT_INVERT	0x24
+#define GPIO_INPUT_FILTER	0x28
+#define GPIO_INPUT_EVENT_COUNT	0x2C
+#define GPIO_READ_BACK		0x30
+#define GPIO_INPUT_AUX1		0x34
+#define GPIO_EVENTS_ENABLE	0x38
+#define GPIO_LOCK_ENABLE	0x3C
+#define GPIO_POSITIVE_EDGE_EN	0x40
+#define GPIO_NEGATIVE_EDGE_EN	0x44
+#define GPIO_POSITIVE_EDGE_STS	0x48
+#define GPIO_NEGATIVE_EDGE_STS	0x4C
+
+#define GPIO_MAP_X		0xE0
+#define GPIO_MAP_Y		0xE4
+#define GPIO_MAP_Z		0xE8
+#define GPIO_MAP_W		0xEC
+
+void cs5535_gpio_set(unsigned offset, unsigned int reg);
+void cs5535_gpio_clear(unsigned offset, unsigned int reg);
+int cs5535_gpio_isset(unsigned offset, unsigned int reg);
+
+#endif
-- 
1.5.6.5

Re: [PATCH 1/2] cs5535-gpio: add AMD CS5535/CS5536 GPIO driver support

[Tobias Müller]

2009/6/10 Andres Salomon <dilinger@collabora.co.uk>:
> +config GPIO_CS5535
> +       tristate "AMD CS5535/CS5536 GPIO support"
> +       depends on PCI && !CS5535_GPIO && !MGEODE_LX
> +       help
> +         The AMD CS5535 and CS5536 southbridges support 28 GPIO pins that
> +         can be used for quite a number of things.  The CS5535/6 is found on
> +         AMD Geode and Lemote Yeeloong devices.

Why do you depend on !MGEODE_LX? Are there any problems?

Re: [PATCH 1/2] cs5535-gpio: add AMD CS5535/CS5536 GPIO driver support

[Andres Salomon]

On Thu, 11 Jun 2009 16:46:35 +0200
Tobias Müller <Tobias_Mueller@twam.info> wrote:

> 2009/6/10 Andres Salomon <dilinger@collabora.co.uk>:
> > +config GPIO_CS5535
> > +       tristate "AMD CS5535/CS5536 GPIO support"
> > +       depends on PCI && !CS5535_GPIO && !MGEODE_LX
> > +       help
> > +         The AMD CS5535 and CS5536 southbridges support 28 GPIO
> > pins that
> > +         can be used for quite a number of things.  The CS5535/6
> > is found on
> > +         AMD Geode and Lemote Yeeloong devices.
> 
> Why do you depend on !MGEODE_LX? Are there any problems?

Because arch/x86/kernel/{mfgpt,geode}_32.c are built if
CONFIG_MGEODE_LX is set, and they will happily clobber each other.  We
need to get a broken-out mfgpt driver completed, and then we can rework
or remove the arch/x86/kernel/{mfgpt,geode}_32.c stuff.

It's probably possible to add a separate config for that
(CONFIG_GEODE_LEGACY or something?) rather than having it depend upon
CONFIG_MGEODE_LX, but I haven't looked into it.  I'd rather spend the
time fixing it correctly than fretting about with the various config
permutations. :)

Re: [PATCH 1/2] cs5535-gpio: add AMD CS5535/CS5536 GPIO driver support

[Tobias Müller]

Here's the promised patch. :) I tried to solve all issues I had with
Andres' patch.



From: Tobias Mueller <Tobias_Mueller@twam.info>

Added mask to enable/disable some GPIO pins.
Added names for GPIO pins.
Added code on GPIO initialisation to disable output/input aux functions.

Signed-off-by: Tobias Mueller <Tobias_Mueller@twam.info>
---
diff --git a/drivers/gpio/cs5535-gpio.c b/drivers/gpio/cs5535-gpio.c
index 5613889..8a0b290 100644
--- a/drivers/gpio/cs5535-gpio.c
+++ b/drivers/gpio/cs5535-gpio.c
@@ -18,6 +18,11 @@

 #define DRV_NAME "cs5535-gpio"
 #define GPIO_BAR 1
+#define GPIO_DEFAULT_MASK 0x0B003C66
+
+static ulong mask = GPIO_DEFAULT_MASK;
+module_param_named(mask, mask, ulong, 0444);
+MODULE_PARM_DESC(mask, "GPIO channel mask.");

 static struct cs5535_gpio_chip {
 	struct gpio_chip chip;
@@ -102,6 +107,38 @@ EXPORT_SYMBOL_GPL(cs5535_gpio_isset);
  * Generic gpio_chip API support.
  */

+static int chip_gpio_request(struct gpio_chip *c, unsigned offset)
+{
+	struct cs5535_gpio_chip *chip = (struct cs5535_gpio_chip *) c;
+	unsigned long flags;
+
+	spin_lock_irqsave(&chip->lock, flags);
+
+	/* check if this pin is available */
+	if ((mask & (1 << offset)) == 0) {
+		printk(KERN_INFO DRV_NAME
+		       ": pin %u is not available (check mask)\n", offset);
+		return -EINVAL;
+	}
+
+	/* disable output aux 1 & 2 on this pin */
+	__cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_AUX1);
+	__cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_AUX2);
+
+	/* disable input aux 1 on this pin */
+	__cs5535_gpio_clear(chip, offset, GPIO_INPUT_AUX1);
+
+	/* disable output */
+	__cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_ENABLE);
+
+	/* enable input */
+	__cs5535_gpio_set(chip, offset, GPIO_INPUT_ENABLE);
+
+	spin_unlock_irqrestore(&chip->lock, flags);
+
+	return 0;
+}
+
 static int chip_gpio_get(struct gpio_chip *chip, unsigned offset)
 {
 	return cs5535_gpio_isset(offset, GPIO_OUTPUT_VAL);
@@ -145,19 +182,34 @@ static int chip_direction_output(struct
gpio_chip *c, unsigned offset, int val)
 	return 0;
 }

+static char *cs5535_gpio_names[] = {
+	"GPIO0", "GPIO1", "GPIO2", "GPIO3",
+	"GPIO4", "GPIO5", "GPIO6", "GPIO7",
+	"GPIO8", "GPIO9", "GPIO10", "GPIO11",
+	"GPIO12", "GPIO13", "GPIO14", "GPIO15",
+	"GPIO16", "GPIO17", "GPIO18", "GPIO19",
+	"GPIO20", "GPIO21", "GPIO22", NULL,
+	"GPIO24", "GPIO25", "GPIO26", "GPIO27",
+	"GPIO28", NULL, NULL, NULL,
+};
+
 static struct cs5535_gpio_chip cs5535_gpio_chip = {
 	.chip = {
 		.owner = THIS_MODULE,
 		.label = DRV_NAME,

 		.base = 0,
-		.ngpio = 28,
+		.ngpio = 32,
+		.names = cs5535_gpio_names,
+
+		.request = chip_gpio_request,

 		.get = chip_gpio_get,
 		.set = chip_gpio_set,

 		.direction_input = chip_direction_input,
 		.direction_output = chip_direction_output,
+
 	},
 };

@@ -165,6 +217,7 @@ static int __init cs5535_gpio_probe(struct pci_dev *pdev,
 		const struct pci_device_id *pci_id)
 {
 	int err;
+	ulong mask_orig = mask;

 	/* There are two ways to get the GPIO base address; one is by
 	 * fetching it from MSR_LBAR_GPIO, the other is by reading the
@@ -193,6 +246,18 @@ static int __init cs5535_gpio_probe(struct pci_dev *pdev,
 	dev_info(&pdev->dev, "allocated PCI BAR #%d: base 0x%llx\n", GPIO_BAR,
 			(unsigned long long) cs5535_gpio_chip.base);

+	/* mask out reserved pins */
+	mask &= 0x1F7FFFFF;
+
+	/* do not allow pin 28, Power Button, as there's special handling
+	 * in the PMC needed. (note 12, p. 48) */
+	mask &= ~(1 << 28);
+
+	if (mask_orig != mask)
+		printk(KERN_INFO DRV_NAME
+		       ": mask changed from 0x%08lX to 0x%08lX\n",
+		       mask_orig, mask);
+
 	/* finally, register with the generic GPIO API */
 	err = gpiochip_add(&cs5535_gpio_chip.chip);
 	if (err) {

Re: [PATCH 1/2] cs5535-gpio: add AMD CS5535/CS5536 GPIO driver support

[Andres Salomon]

Awesome.  Comments below..


On Thu, 11 Jun 2009 20:52:52 +0200
Tobias Müller <Tobias_Mueller@twam.info> wrote:

> Here's the promised patch. :) I tried to solve all issues I had with
> Andres' patch.
> 
> 
> 
> From: Tobias Mueller <Tobias_Mueller@twam.info>
> 
> Added mask to enable/disable some GPIO pins.
> Added names for GPIO pins.
> Added code on GPIO initialisation to disable output/input aux
> functions.
> 
> Signed-off-by: Tobias Mueller <Tobias_Mueller@twam.info>
> ---
> diff --git a/drivers/gpio/cs5535-gpio.c b/drivers/gpio/cs5535-gpio.c
> index 5613889..8a0b290 100644
> --- a/drivers/gpio/cs5535-gpio.c
> +++ b/drivers/gpio/cs5535-gpio.c
> @@ -18,6 +18,11 @@
> 
>  #define DRV_NAME "cs5535-gpio"
>  #define GPIO_BAR 1
> +#define GPIO_DEFAULT_MASK 0x0B003C66

Where does this mask of available GPIOs originate from?


> +
> +static ulong mask = GPIO_DEFAULT_MASK;
> +module_param_named(mask, mask, ulong, 0444);
> +MODULE_PARM_DESC(mask, "GPIO channel mask.");
> 
>  static struct cs5535_gpio_chip {
>  	struct gpio_chip chip;
> @@ -102,6 +107,38 @@ EXPORT_SYMBOL_GPL(cs5535_gpio_isset);
>   * Generic gpio_chip API support.
>   */
> 
> +static int chip_gpio_request(struct gpio_chip *c, unsigned offset)
> +{
> +	struct cs5535_gpio_chip *chip = (struct cs5535_gpio_chip *)
> c;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&chip->lock, flags);
> +
> +	/* check if this pin is available */
> +	if ((mask & (1 << offset)) == 0) {
> +		printk(KERN_INFO DRV_NAME
> +		       ": pin %u is not available (check mask)\n",
> offset);
> +		return -EINVAL;

There's a locking error here; you really want to spin_unlock_irqrestore
before returning.


> +	}
> +
> +	/* disable output aux 1 & 2 on this pin */
> +	__cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_AUX1);
> +	__cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_AUX2);
> +
> +	/* disable input aux 1 on this pin */
> +	__cs5535_gpio_clear(chip, offset, GPIO_INPUT_AUX1);
> +
> +	/* disable output */
> +	__cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_ENABLE);
> +
> +	/* enable input */
> +	__cs5535_gpio_set(chip, offset, GPIO_INPUT_ENABLE);

I don't think this is the right place for all of this.  Your earlier
email mentioned disabling OUT_AUX{1,2} for outputs, and IN_AUX for
inputs.  I'm fine with doing that here, but I don't see why you're also
disabling output and enabling input by default.

> +
> +	spin_unlock_irqrestore(&chip->lock, flags);
> +
> +	return 0;
> +}
> +
>  static int chip_gpio_get(struct gpio_chip *chip, unsigned offset)
>  {
>  	return cs5535_gpio_isset(offset, GPIO_OUTPUT_VAL);
> @@ -145,19 +182,34 @@ static int chip_direction_output(struct
> gpio_chip *c, unsigned offset, int val)
>  	return 0;
>  }
> 
> +static char *cs5535_gpio_names[] = {
> +	"GPIO0", "GPIO1", "GPIO2", "GPIO3",
> +	"GPIO4", "GPIO5", "GPIO6", "GPIO7",
> +	"GPIO8", "GPIO9", "GPIO10", "GPIO11",
> +	"GPIO12", "GPIO13", "GPIO14", "GPIO15",
> +	"GPIO16", "GPIO17", "GPIO18", "GPIO19",
> +	"GPIO20", "GPIO21", "GPIO22", NULL,
> +	"GPIO24", "GPIO25", "GPIO26", "GPIO27",
> +	"GPIO28", NULL, NULL, NULL,
> +};
> +
>  static struct cs5535_gpio_chip cs5535_gpio_chip = {
>  	.chip = {
>  		.owner = THIS_MODULE,
>  		.label = DRV_NAME,
> 
>  		.base = 0,
> -		.ngpio = 28,
> +		.ngpio = 32,

Since GPIOs 29-31 aren't externally available, and 28 is
unavailable anyways, shouldn't we just set ngpio to 28?


> +		.names = cs5535_gpio_names,
> +
> +		.request = chip_gpio_request,
> 
>  		.get = chip_gpio_get,
>  		.set = chip_gpio_set,
> 
>  		.direction_input = chip_direction_input,
>  		.direction_output = chip_direction_output,
> +
>  	},
>  };
> 
> @@ -165,6 +217,7 @@ static int __init cs5535_gpio_probe(struct
> pci_dev *pdev, const struct pci_device_id *pci_id)
>  {
>  	int err;
> +	ulong mask_orig = mask;
> 
>  	/* There are two ways to get the GPIO base address; one is by
>  	 * fetching it from MSR_LBAR_GPIO, the other is by reading
> the @@ -193,6 +246,18 @@ static int __init cs5535_gpio_probe(struct
> pci_dev *pdev, dev_info(&pdev->dev, "allocated PCI BAR #%d: base
> 0x%llx\n", GPIO_BAR, (unsigned long long) cs5535_gpio_chip.base);
> 
> +	/* mask out reserved pins */
> +	mask &= 0x1F7FFFFF;
> +
> +	/* do not allow pin 28, Power Button, as there's special
> handling
> +	 * in the PMC needed. (note 12, p. 48) */
> +	mask &= ~(1 << 28);

Nice, even a pointer to the note. :)

> +
> +	if (mask_orig != mask)
> +		printk(KERN_INFO DRV_NAME
> +		       ": mask changed from 0x%08lX to 0x%08lX\n",
> +		       mask_orig, mask);
> +
>  	/* finally, register with the generic GPIO API */
>  	err = gpiochip_add(&cs5535_gpio_chip.chip);
>  	if (err) {

Re: [PATCH 1/2] cs5535-gpio: add AMD CS5535/CS5536 GPIO driver support

[Tobias Müller]

>>  #define DRV_NAME "cs5535-gpio"
>>  #define GPIO_BAR 1
>> +#define GPIO_DEFAULT_MASK 0x0B003C66
>
> Where does this mask of available GPIOs originate from?

I had a comment in my original patch:

/**
* Some GPIO pins
*  31-29,23 : reserved (always mask out)
*  28       : Power Button
*  26       : PME#
*  22-16    : LPC
*  14,15    : SMBus
*  9,8      : UART1
*  7        : PCI INTB
*  3,4      : UART2/DDC
*  2        : IDE_IRQ0
*  1        : AC_BEEP
*  0        : PCI INTA
*
* If a mask was not specified, be conservative and only allow:
*  1,2,5,6,10-13,24,25,27
*/

I'll add this in my patch to clear it out.

>> +     spin_lock_irqsave(&chip->lock, flags);
>> +
>> +     /* check if this pin is available */
>> +     if ((mask & (1 << offset)) == 0) {
>> +             printk(KERN_INFO DRV_NAME
>> +                    ": pin %u is not available (check mask)\n",
>> offset);
>> +             return -EINVAL;
>
> There's a locking error here; you really want to spin_unlock_irqrestore
> before returning.

Thanks.

>> +     /* disable output aux 1 & 2 on this pin */
>> +     __cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_AUX1);
>> +     __cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_AUX2);
>> +
>> +     /* disable input aux 1 on this pin */
>> +     __cs5535_gpio_clear(chip, offset, GPIO_INPUT_AUX1);
>> +
>> +     /* disable output */
>> +     __cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_ENABLE);
>> +
>> +     /* enable input */
>> +     __cs5535_gpio_set(chip, offset, GPIO_INPUT_ENABLE);
>
> I don't think this is the right place for all of this.  Your earlier
> email mentioned disabling OUT_AUX{1,2} for outputs, and IN_AUX for
> inputs.  I'm fine with doing that here, but I don't see why you're also
> disabling output and enabling input by default.

I mentioned this in an ealier mail too. When I request the GPIO from
userspace the direction file always contains "in", so I thought
this is the standard direction after resetting as I should be in a
defined state after requesting. But I didn't found anything
about this in GPIO lib documentation, so I would be fine to change
this if there is any common default behavoir.

>> -             .ngpio = 28,
>> +             .ngpio = 32,
>
> Since GPIOs 29-31 aren't externally available, and 28 is
> unavailable anyways, shouldn't we just set ngpio to 28?
I thought that 32 is in consistency with the datasheet as it always
talks about 32 GPIO pins.

Re: [PATCH 1/2] cs5535-gpio: add AMD CS5535/CS5536 GPIO driver support

[Andres Salomon]

On Thu, 11 Jun 2009 22:11:58 +0200
Tobias Müller <Tobias_Mueller@twam.info> wrote:

> >>  #define DRV_NAME "cs5535-gpio"
> >>  #define GPIO_BAR 1
> >> +#define GPIO_DEFAULT_MASK 0x0B003C66
> >
> > Where does this mask of available GPIOs originate from?
> 
> I had a comment in my original patch:
> 
> /**
> * Some GPIO pins
> *  31-29,23 : reserved (always mask out)
> *  28       : Power Button
> *  26       : PME#
> *  22-16    : LPC
> *  14,15    : SMBus
> *  9,8      : UART1
> *  7        : PCI INTB
> *  3,4      : UART2/DDC
> *  2        : IDE_IRQ0
> *  1        : AC_BEEP
> *  0        : PCI INTA
> *
> * If a mask was not specified, be conservative and only allow:
> *  1,2,5,6,10-13,24,25,27
> */
> 
> I'll add this in my patch to clear it out.
> 

But why are you being conservative in the first place?  If something's
using GPIOs, unless they're unmapped, you should allow it to use them
without requiring a boot arg.

For example, OLPC uses GPIO 7 for its DCON IRQ.  With the masking
scheme, OLPC will need to set that mask from the default.  I don't see
the point of having the mask at all if other drivers in the kernel are
going to be requesting GPIOs (presumably they know what they're doing).


> >> +     /* disable output aux 1 & 2 on this pin */
> >> +     __cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_AUX1);
> >> +     __cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_AUX2);
> >> +
> >> +     /* disable input aux 1 on this pin */
> >> +     __cs5535_gpio_clear(chip, offset, GPIO_INPUT_AUX1);
> >> +
> >> +     /* disable output */
> >> +     __cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_ENABLE);
> >> +
> >> +     /* enable input */
> >> +     __cs5535_gpio_set(chip, offset, GPIO_INPUT_ENABLE);
> >
> > I don't think this is the right place for all of this.  Your earlier
> > email mentioned disabling OUT_AUX{1,2} for outputs, and IN_AUX for
> > inputs.  I'm fine with doing that here, but I don't see why you're
> > also disabling output and enabling input by default.
> 
> I mentioned this in an ealier mail too. When I request the GPIO from
> userspace the direction file always contains "in", so I thought
> this is the standard direction after resetting as I should be in a
> defined state after requesting. But I didn't found anything
> about this in GPIO lib documentation, so I would be fine to change
> this if there is any common default behavoir.

To be honest, I'd have to play around with it a bit before I
knew whether it actually breaks anything or not. I'm not sure if
it would break anything on OLPC, and I don't have any other geode
machines that do anything interesting w/ GPIOs.

Maybe David can clear up whether this behavior is correct from the
userspace GPIO usage standpoint..


> 
> >> -             .ngpio = 28,
> >> +             .ngpio = 32,
> >
> > Since GPIOs 29-31 aren't externally available, and 28 is
> > unavailable anyways, shouldn't we just set ngpio to 28?
> I thought that 32 is in consistency with the datasheet as it always
> talks about 32 GPIO pins.

Fair enough.

Re: [PATCH 1/2] cs5535-gpio: add AMD CS5535/CS5536 GPIO driver support

[Tobias Müller]

>> /**
>> * Some GPIO pins
>> *  31-29,23 : reserved (always mask out)
>> *  28       : Power Button
>> *  26       : PME#
>> *  22-16    : LPC
>> *  14,15    : SMBus
>> *  9,8      : UART1
>> *  7        : PCI INTB
>> *  3,4      : UART2/DDC
>> *  2        : IDE_IRQ0
>> *  1        : AC_BEEP
>> *  0        : PCI INTA
>> *
>> * If a mask was not specified, be conservative and only allow:
>> *  1,2,5,6,10-13,24,25,27
>> */
>>
>> I'll add this in my patch to clear it out.
>>
>
> But why are you being conservative in the first place?  If something's
> using GPIOs, unless they're unmapped, you should allow it to use them
> without requiring a boot arg.
>
> For example, OLPC uses GPIO 7 for its DCON IRQ.  With the masking
> scheme, OLPC will need to set that mask from the default.  I don't see
> the point of having the mask at all if other drivers in the kernel are
> going to be requesting GPIOs (presumably they know what they're doing).
Hmm... OK, this makes sense. So default mask allow everything exept
reserved pins and pin 28 (power button).

I think the mask is quite useful if you've critical things on GPIO pins
and they should be changeable (especially from userspace and when
non-root users are allowed to use userspace gpio).

>> >> +     /* disable output aux 1 & 2 on this pin */
>> >> +     __cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_AUX1);
>> >> +     __cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_AUX2);
>> >> +
>> >> +     /* disable input aux 1 on this pin */
>> >> +     __cs5535_gpio_clear(chip, offset, GPIO_INPUT_AUX1);
>> >> +
>> >> +     /* disable output */
>> >> +     __cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_ENABLE);
>> >> +
>> >> +     /* enable input */
>> >> +     __cs5535_gpio_set(chip, offset, GPIO_INPUT_ENABLE);
>> >
>> > I don't think this is the right place for all of this.  Your earlier
>> > email mentioned disabling OUT_AUX{1,2} for outputs, and IN_AUX for
>> > inputs.  I'm fine with doing that here, but I don't see why you're
>> > also disabling output and enabling input by default.
>>
>> I mentioned this in an ealier mail too. When I request the GPIO from
>> userspace the direction file always contains "in", so I thought
>> this is the standard direction after resetting as I should be in a
>> defined state after requesting. But I didn't found anything
>> about this in GPIO lib documentation, so I would be fine to change
>> this if there is any common default behavoir.
>
> To be honest, I'd have to play around with it a bit before I
> knew whether it actually breaks anything or not. I'm not sure if
> it would break anything on OLPC, and I don't have any other geode
> machines that do anything interesting w/ GPIOs.
>
> Maybe David can clear up whether this behavior is correct from the
> userspace GPIO usage standpoint..
This would be nice. But I wouldn't have a problem when those
two statements are removed. I just thought it's better to put them
in a defined state.

Re: [PATCH 1/2] cs5535-gpio: add AMD CS5535/CS5536 GPIO driver support

[Andres Salomon]

On Thu, 11 Jun 2009 23:35:55 +0200
Tobias Müller <Tobias_Mueller@twam.info> wrote:

> >> /**
> >> * Some GPIO pins
> >> *  31-29,23 : reserved (always mask out)
> >> *  28       : Power Button
> >> *  26       : PME#
> >> *  22-16    : LPC
> >> *  14,15    : SMBus
> >> *  9,8      : UART1
> >> *  7        : PCI INTB
> >> *  3,4      : UART2/DDC
> >> *  2        : IDE_IRQ0
> >> *  1        : AC_BEEP
> >> *  0        : PCI INTA
> >> *
> >> * If a mask was not specified, be conservative and only allow:
> >> *  1,2,5,6,10-13,24,25,27
> >> */
> >>
> >> I'll add this in my patch to clear it out.
> >>
> >
> > But why are you being conservative in the first place?  If
> > something's using GPIOs, unless they're unmapped, you should allow
> > it to use them without requiring a boot arg.
> >
> > For example, OLPC uses GPIO 7 for its DCON IRQ.  With the masking
> > scheme, OLPC will need to set that mask from the default.  I don't
> > see the point of having the mask at all if other drivers in the
> > kernel are going to be requesting GPIOs (presumably they know what
> > they're doing).
> Hmm... OK, this makes sense. So default mask allow everything exept
> reserved pins and pin 28 (power button).
> 
> I think the mask is quite useful if you've critical things on GPIO
> pins and they should be changeable (especially from userspace and when
> non-root users are allowed to use userspace gpio).

I agree that it would be useful for userspace, just not for
kernelspace.  Is there a way that you can have it only enforce the
mask if the request is coming from userspace?

Re: [PATCH 1/2] cs5535-gpio: add AMD CS5535/CS5536 GPIO driver support

[Tobias Müller]

2009/6/13 Andres Salomon <dilinger@collabora.co.uk>:
> On Thu, 11 Jun 2009 23:35:55 +0200
> Tobias Müller <Tobias_Mueller@twam.info> wrote:
>
>> >> /**
>> >> * Some GPIO pins
>> >> *  31-29,23 : reserved (always mask out)
>> >> *  28       : Power Button
>> >> *  26       : PME#
>> >> *  22-16    : LPC
>> >> *  14,15    : SMBus
>> >> *  9,8      : UART1
>> >> *  7        : PCI INTB
>> >> *  3,4      : UART2/DDC
>> >> *  2        : IDE_IRQ0
>> >> *  1        : AC_BEEP
>> >> *  0        : PCI INTA
>> >> *
>> >> * If a mask was not specified, be conservative and only allow:
>> >> *  1,2,5,6,10-13,24,25,27
>> >> */
>> >>
>> >> I'll add this in my patch to clear it out.
>> >>
>> >
>> > But why are you being conservative in the first place?  If
>> > something's using GPIOs, unless they're unmapped, you should allow
>> > it to use them without requiring a boot arg.
>> >
>> > For example, OLPC uses GPIO 7 for its DCON IRQ.  With the masking
>> > scheme, OLPC will need to set that mask from the default.  I don't
>> > see the point of having the mask at all if other drivers in the
>> > kernel are going to be requesting GPIOs (presumably they know what
>> > they're doing).
>> Hmm... OK, this makes sense. So default mask allow everything exept
>> reserved pins and pin 28 (power button).
>>
>> I think the mask is quite useful if you've critical things on GPIO
>> pins and they should be changeable (especially from userspace and when
>> non-root users are allowed to use userspace gpio).
>
> I agree that it would be useful for userspace, just not for
> kernelspace.  Is there a way that you can have it only enforce the
> mask if the request is coming from userspace?

I didn't find a way to find out if a request is coming from userspace, so at
the moment I think it's only possible to set it global.

Re: [PATCH 1/2] cs5535-gpio: add AMD CS5535/CS5536 GPIO driver support

[David Brownell]

On Thursday 11 June 2009, Andres Salomon wrote:
> 
> > I mentioned this in an ealier mail too. When I request the GPIO from
> > userspace the direction file always contains "in", so I thought
> > this is the standard direction after resetting as I should be in a
> > defined state after requesting. But I didn't found anything
> > about this in GPIO lib documentation, so I would be fine to change
> > this if there is any common default behavoir.
> 
> To be honest, I'd have to play around with it a bit before I
> knew whether it actually breaks anything or not. I'm not sure if
> it would break anything on OLPC, and I don't have any other geode
> machines that do anything interesting w/ GPIOs.
> 
> Maybe David can clear up whether this behavior is correct from the
> userspace GPIO usage standpoint..

Correct-but-annoying ... and maybe worth changing.  The
direction *displayed* may not reflect the actual hardware
until after that GPIO signal is initialized.  Boot firmware
may well have set the direction; Linux shouldn't change it
without explicit instructions to do so.

The issue is that when it first comes up, nobody has tod
Linux that direction ... so instead of defining an "unknown"
state, that code applies a heuristic.  In gpiochip_add():

                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;
                }

Now, as far as heuristics go that one seems to cover most
of the common cases.  But like all heuristics, the result
produced may be wrong.

So it would be nice to remove the heuristic.  The best
way would be to add a new method to query gpio direction.
Then that when it's available, instead of the heuristic.

- Dave

Re: [PATCH 1/2] cs5535-gpio: add AMD CS5535/CS5536 GPIO driver support

[Tobias Müller]

2009/7/2 David Brownell <david-b@pacbell.net>:
> Correct-but-annoying ... and maybe worth changing.  The
> direction *displayed* may not reflect the actual hardware
> until after that GPIO signal is initialized.  Boot firmware
> may well have set the direction; Linux shouldn't change it
> without explicit instructions to do so.
OK. If this is a know problem with all GPIO drivers, we can
skip these to two lines to set in pin a defined state.

> So it would be nice to remove the heuristic.  The best
> way would be to add a new method to query gpio direction.
> Then that when it's available, instead of the heuristic.
This can be implemented when the GPIO interface supports it.

The problem with the mask still occurs. I changed to default
mask to 0x0F7FFFFF, so that all pins except the reserved
ones and the power-pin (which needs special handling) are
enabled. Shall we keep it this way (which I prefer) or delete
it entirely?

Sorry, for the late reply, holidays were nice. :)
  Tobias

Re: [PATCH 1/2] cs5535-gpio: add AMD CS5535/CS5536 GPIO driver support

[Andres Salomon]

On Sat, 15 Aug 2009 12:59:48 +0200
Tobias Müller <Tobias_Mueller@twam.info> wrote:

> 2009/7/2 David Brownell <david-b@pacbell.net>:
> > Correct-but-annoying ... and maybe worth changing.  The
> > direction *displayed* may not reflect the actual hardware
> > until after that GPIO signal is initialized.  Boot firmware
> > may well have set the direction; Linux shouldn't change it
> > without explicit instructions to do so.
> OK. If this is a know problem with all GPIO drivers, we can
> skip these to two lines to set in pin a defined state.
> 
> > So it would be nice to remove the heuristic.  The best
> > way would be to add a new method to query gpio direction.
> > Then that when it's available, instead of the heuristic.
> This can be implemented when the GPIO interface supports it.
> 
> The problem with the mask still occurs. I changed to default
> mask to 0x0F7FFFFF, so that all pins except the reserved
> ones and the power-pin (which needs special handling) are
> enabled. Shall we keep it this way (which I prefer) or delete
> it entirely?
> 

Hi Tobias,

At this point, I've lost track of all the changes that need to be
made.  Would you mind submitting your current patch w/ all of the
updates we talked about, and we can go from there?  If possible, it'd
be nice to get the cs5535-gpio stuff into the next merge window (but
we'd need to get moving on this now in order to do that).

Re: [PATCH 1/2] cs5535-gpio: add AMD CS5535/CS5536 GPIO driver support

[Tobias Müller]

2009/8/18 Andres Salomon <dilinger@collabora.co.uk>:
> Hi Tobias,
>
> At this point, I've lost track of all the changes that need to be
> made.  Would you mind submitting your current patch w/ all of the
> updates we talked about, and we can go from there?  If possible, it'd
> be nice to get the cs5535-gpio stuff into the next merge window (but
> we'd need to get moving on this now in order to do that).

cs5535: request function, mask & names added

Changed number of gpio pins to 32 (according to datasheet)
Added mask to disable some pins
Added gpio_request for checking mask and disabling special pin functions
Added pin names

Signed-off-by: Tobias Mueller <Tobias_Mueller@twam.info>

---
 drivers/gpio/cs5535-gpio.c |   86 +++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 85 insertions(+), 1 deletions(-)

diff --git a/drivers/gpio/cs5535-gpio.c b/drivers/gpio/cs5535-gpio.c
index 5613889..d50a0bf 100644
--- a/drivers/gpio/cs5535-gpio.c
+++ b/drivers/gpio/cs5535-gpio.c
@@ -19,6 +19,29 @@
 #define DRV_NAME "cs5535-gpio"
 #define GPIO_BAR 1

+/*
+ * Some GPIO pins
+ *  31-29,23 : reserved (always mask out)
+ *  28       : Power Button
+ *  26       : PME#
+ *  22-16    : LPC
+ *  14,15    : SMBus
+ *  9,8      : UART1
+ *  7        : PCI INTB
+ *  3,4      : UART2/DDC
+ *  2        : IDE_IRQ0
+ *  1        : AC_BEEP
+ *  0        : PCI INTA
+ *
+ * If a mask was not specified, allow all except
+ * reserved and Power Button
+ */
+#define GPIO_DEFAULT_MASK 0x0F7FFFFF
+
+static ulong mask = GPIO_DEFAULT_MASK;
+module_param_named(mask, mask, ulong, 0444);
+MODULE_PARM_DESC(mask, "GPIO channel mask.");
+
 static struct cs5535_gpio_chip {
 	struct gpio_chip chip;
 	resource_size_t base;
@@ -102,6 +125,39 @@ EXPORT_SYMBOL_GPL(cs5535_gpio_isset);
  * Generic gpio_chip API support.
  */

+static int chip_gpio_request(struct gpio_chip *c, unsigned offset)
+{
+	struct cs5535_gpio_chip *chip = (struct cs5535_gpio_chip *) c;
+	unsigned long flags;
+
+	spin_lock_irqsave(&chip->lock, flags);
+
+	/* check if this pin is available */
+	if ((mask & (1 << offset)) == 0) {
+		printk(KERN_INFO DRV_NAME
+		       ": pin %u is not available (check mask)\n", offset);
+		spin_unlock_irqrestore(&chip->lock, flags);
+		return -EINVAL;
+	}
+
+	/* disable output aux 1 & 2 on this pin */
+	__cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_AUX1);
+	__cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_AUX2);
+
+	/* disable input aux 1 on this pin */
+	__cs5535_gpio_clear(chip, offset, GPIO_INPUT_AUX1);
+
+	spin_unlock_irqrestore(&chip->lock, flags);
+
+	return 0;
+}
+
 static int chip_gpio_get(struct gpio_chip *chip, unsigned offset)
 {
 	return cs5535_gpio_isset(offset, GPIO_OUTPUT_VAL);
@@ -145,19 +201,34 @@ static int chip_direction_output(struct
gpio_chip *c, unsigned offset, int val)
 	return 0;
 }

+static char *cs5535_gpio_names[] = {
+	"GPIO0", "GPIO1", "GPIO2", "GPIO3",
+	"GPIO4", "GPIO5", "GPIO6", "GPIO7",
+	"GPIO8", "GPIO9", "GPIO10", "GPIO11",
+	"GPIO12", "GPIO13", "GPIO14", "GPIO15",
+	"GPIO16", "GPIO17", "GPIO18", "GPIO19",
+	"GPIO20", "GPIO21", "GPIO22", NULL,
+	"GPIO24", "GPIO25", "GPIO26", "GPIO27",
+	"GPIO28", NULL, NULL, NULL,
+};
+
 static struct cs5535_gpio_chip cs5535_gpio_chip = {
 	.chip = {
 		.owner = THIS_MODULE,
 		.label = DRV_NAME,

 		.base = 0,
-		.ngpio = 28,
+		.ngpio = 32,
+		.names = cs5535_gpio_names,
+
+		.request = chip_gpio_request,

 		.get = chip_gpio_get,
 		.set = chip_gpio_set,

 		.direction_input = chip_direction_input,
 		.direction_output = chip_direction_output,
+
 	},
 };

@@ -165,6 +236,7 @@ static int __init cs5535_gpio_probe(struct pci_dev *pdev,
 		const struct pci_device_id *pci_id)
 {
 	int err;
+	ulong mask_orig = mask;

 	/* There are two ways to get the GPIO base address; one is by
 	 * fetching it from MSR_LBAR_GPIO, the other is by reading the
@@ -193,6 +265,18 @@ static int __init cs5535_gpio_probe(struct pci_dev *pdev,
 	dev_info(&pdev->dev, "allocated PCI BAR #%d: base 0x%llx\n", GPIO_BAR,
 			(unsigned long long) cs5535_gpio_chip.base);

+	/* mask out reserved pins */
+	mask &= 0x1F7FFFFF;
+
+	/* do not allow pin 28, Power Button, as there's special handling
+	 * in the PMC needed. (note 12, p. 48) */
+	mask &= ~(1 << 28);
+
+	if (mask_orig != mask)
+		printk(KERN_INFO DRV_NAME
+		       ": mask changed from 0x%08lX to 0x%08lX\n",
+		       mask_orig, mask);
+
 	/* finally, register with the generic GPIO API */
 	err = gpiochip_add(&cs5535_gpio_chip.chip);
 	if (err) {
-- 
1.6.4