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

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

[Andres Salomon]

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 96dda81..ff34b2c 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -157,6 +157,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 9244c6f..3ad6b40 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -12,5 +12,6 @@ obj-$(CONFIG_GPIO_PCF857X)	+= pcf857x.o
 obj-$(CONFIG_GPIO_PL061)	+= pl061.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
 obj-$(CONFIG_GPIO_VR41XX)	+= vr41xx_giu.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.6.3.3

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

[Andres Salomon]

Hi Andrew,

BTW, this patch series includes fixes for symbol build errors (it
should fix all build issues that I'm aware of), and Tobias's changes.
When you get the chance, can you please include it in your tree so it
can get some testing?  Thanks!


On Tue, 18 Aug 2009 17:53:14 -0400
Andres Salomon <dilinger@collabora.co.uk> wrote:

> 
> 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>
[...]

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

[Takashi Iwai]

At Tue, 18 Aug 2009 17:53:14 -0400,
Andres Salomon wrote:
> 
> 
> 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).

Will be any user of cs5535_gpio_*() expected?  If not, it'd be better
not to export stuff, IMO.

> 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.

... or you can rewrite the old driver to use the functions above?
Then the ugly Kconfig check can be dropped, too.  (yeah that's an
answer to my own question above.)

BTW CONFIG_GPIO_CS5536 and CONFIG_CS5536_GPIO are so confusing... :)

> Signed-off-by: Andres Salomon <dilinger@collabora.co.uk>

I can happily merge this patch series to sound GIT tree if no one
objects, since it's basically for OLPC sound stuff.


thanks,

Takashi

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

[Andres Salomon]

On Thu, 20 Aug 2009 17:21:20 +0200
Takashi Iwai <tiwai@suse.de> wrote:

> At Tue, 18 Aug 2009 17:53:14 -0400,
> Andres Salomon wrote:
> > 
> > 
> > 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).
> 
> Will be any user of cs5535_gpio_*() expected?  If not, it'd be better
> not to export stuff, IMO.
> 

The olpc-dcon driver, for one.  If desired, I can remove the exports
for now.


> > 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.
> 
> .... or you can rewrite the old driver to use the functions above?
> Then the ugly Kconfig check can be dropped, too.  (yeah that's an
> answer to my own question above.)

I guess in theory I could.. 


> 
> BTW CONFIG_GPIO_CS5536 and CONFIG_CS5536_GPIO are so confusing... :)
> 

Agreed.  I'd really like CONFIG_CS5535_GPIO to go away, but first there
needs to be a replacement.  CONFIG_CS5535_GPIO will probably also need
to be marked DEPRECATED for a while, too, since it's a
userspace-visible change.  I can follow up w/ a patch for that once
people are happy w/ this current set of patches.

> > Signed-off-by: Andres Salomon <dilinger@collabora.co.uk>
> 
> I can happily merge this patch series to sound GIT tree if no one
> objects, since it's basically for OLPC sound stuff.
> 
> 
> thanks,
> 
> Takashi

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

[Martin-Éric Racine]

 On Thu, 20 Aug 2009 17:21:20 +0200 Takashi Iwai <tiwai@suse.de> wrote:
> BTW CONFIG_GPIO_CS5536 and CONFIG_CS5536_GPIO are so confusing... :)

That's because too many people who write drivers lack a bird's eye
view on the whole Geode platform's history and end up inventing driver
names that only further add to the confusion.  Ages ago, on the Geode
list (I cannot remember if was the AMD PCS maintained list or the
X.org Geode list), we came to the following nomenclature:

gx1:  driver that only affects the first-generation GX from Cyrix or
NSC and the NSC SC system on chip's CPU+FPU components.
gx:  driver that only affects the NSC/AMD Media GX (a.k.a. GX2 Red
Cloud) CPU+FPU components.
lx:  driver that only affect the AMD LX CPU+FPU components.
sc1100:  driver that only affect the sc1100 system on chip's peripheral drivers.
scx200:  driver that affects the sc1200, sc2200 and sc3200 systems on
chip's peripheral drivers.
cs5510:  driver that only affect the cs5510 companion chip.
cs5520:  driver that only affect the cs5520 companion chip.
cs5530:  driver that affects the cs5530 (or cs5530a) companion chip alone.
cs5535:  driver that affects the cs5535 and cs5536 companion chips.
cs5536:  driver that affects the cs5536 specifically (same as cs5535,
plus USB 2.0 support and a different PATA implementation).
cs553x:  driver that affects the WHOLE cs553x series. Should not be
used since the cs5530 and cs553[5|6] are different beasts.
geode:  driver that affects the whole Geode series, such as the SM bus.

Sadly, some drivers don't respect this nomenclature, the most common
mistake being to use cs553x for a driver that only works on cs5535 and
cs5536. Another mistake was the naming of the ALSA driver, which came
as snd-cs5535audio instead of just snd-cs5535.

Martin-Éric

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

[Andres Salomon]

On Thu, 20 Aug 2009 20:03:14 +0300
Martin-Éric Racine <q-funk@iki.fi> wrote:

>  On Thu, 20 Aug 2009 17:21:20 +0200 Takashi Iwai <tiwai@suse.de>
> wrote:
> > BTW CONFIG_GPIO_CS5536 and CONFIG_CS5536_GPIO are so confusing... :)
> 
> That's because too many people who write drivers lack a bird's eye
> view on the whole Geode platform's history and end up inventing driver
> names that only further add to the confusion.  Ages ago, on the Geode

Uh, I'm assuming he meant the GPIO_CS* vs CS*_GPIO bit.  They're both
CS5535 in Kconfig.

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

[Takashi Iwai]

At Thu, 20 Aug 2009 13:27:34 -0400,
Andres Salomon wrote:
> 
> On Thu, 20 Aug 2009 20:03:14 +0300
> Martin-Éric Racine <q-funk@iki.fi> wrote:
> 
> >  On Thu, 20 Aug 2009 17:21:20 +0200 Takashi Iwai <tiwai@suse.de>
> > wrote:
> > > BTW CONFIG_GPIO_CS5536 and CONFIG_CS5536_GPIO are so confusing... :)
> > 
> > That's because too many people who write drivers lack a bird's eye
> > view on the whole Geode platform's history and end up inventing driver
> > names that only further add to the confusion.  Ages ago, on the Geode
> 
> Uh, I'm assuming he meant the GPIO_CS* vs CS*_GPIO bit.  They're both
> CS5535 in Kconfig.

Yes, sorry for typo.


Takashi

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

[Takashi Iwai]

At Thu, 20 Aug 2009 12:40:02 -0400,
Andres Salomon wrote:
> 
> On Thu, 20 Aug 2009 17:21:20 +0200
> Takashi Iwai <tiwai@suse.de> wrote:
> 
> > At Tue, 18 Aug 2009 17:53:14 -0400,
> > Andres Salomon wrote:
> > > 
> > > 
> > > 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).
> > 
> > Will be any user of cs5535_gpio_*() expected?  If not, it'd be better
> > not to export stuff, IMO.
> > 
> 
> The olpc-dcon driver, for one.  If desired, I can remove the exports
> for now.

Ah, OK.  If this is going to be merged soonish, it should be fine
to have the exports from the beginning.


> > > 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.
> > 
> > .... or you can rewrite the old driver to use the functions above?
> > Then the ugly Kconfig check can be dropped, too.  (yeah that's an
> > answer to my own question above.)
> 
> I guess in theory I could.. 
> 
> 
> > 
> > BTW CONFIG_GPIO_CS5536 and CONFIG_CS5536_GPIO are so confusing... :)
> > 
> 
> Agreed.  I'd really like CONFIG_CS5535_GPIO to go away, but first there
> needs to be a replacement.  CONFIG_CS5535_GPIO will probably also need
> to be marked DEPRECATED for a while, too, since it's a
> userspace-visible change.  I can follow up w/ a patch for that once
> people are happy w/ this current set of patches.

Agreed.  Unless the compatible feature is given, some deprecation
period would be safer.


thanks,

Takashi

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

[Alessandro Zummo]

On Tue, 18 Aug 2009 17:53:14 -0400
Andres Salomon <dilinger@collabora.co.uk> wrote:

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

 fwiw,

 Reviewed-by: Alessandro Zummo <a.zummo@towertech.it>

> +	dev_info(&pdev->dev, "allocated PCI BAR #%d: base 0x%llx\n", GPIO_BAR,
> +			(unsigned long long) cs5535_gpio_chip.base);

 shouldn't this be dev_dbg ?

> 
> +	/* 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");

 gpiochip_add already uses pr_err for error conditions, there's no need
 to report twice.

> +		goto release_region;
> +	}
> +
> +	printk(KERN_INFO DRV_NAME ": GPIO support successfully loaded.\n");

 I'd use dev_xxx here, maybe it's worth to have a generic one
 in gpiochip_add .


-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Torino, Italy

  http://www.towertech.it

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

[Andres Salomon]

On Sun, 23 Aug 2009 11:22:14 +0200
Alessandro Zummo <alessandro.zummo@towertech.it> wrote:

> On Tue, 18 Aug 2009 17:53:14 -0400
> Andres Salomon <dilinger@collabora.co.uk> wrote:
> 
> > 
> > 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>
> 
>  fwiw,
> 
>  Reviewed-by: Alessandro Zummo <a.zummo@towertech.it>
> 

Thanks!

> > +	dev_info(&pdev->dev, "allocated PCI BAR #%d: base
> > 0x%llx\n", GPIO_BAR,
> > +			(unsigned long long)
> > cs5535_gpio_chip.base);
> 
>  shouldn't this be dev_dbg ?
> 

I prefer to see it when the driver is loaded (as info).

> > 
> > +	/* 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");
> 
>  gpiochip_add already uses pr_err for error conditions, there's no
> need to report twice.
> 

Hm, you're right.   Andrew, can you please add/merge the patch below?


> > +		goto release_region;
> > +	}
> > +
> > +	printk(KERN_INFO DRV_NAME ": GPIO support successfully
> > loaded.\n");
> 
>  I'd use dev_xxx here, maybe it's worth to have a generic one
>  in gpiochip_add .
> 
> 

Noted and changed (along w/ a few other printks).  Thanks!



From: Andres Salomon <dilinger@collabora.co.uk>
Subject: [PATCH] cs5535-gpio: make printk usage consistent

Clean up places where we were using printk()s instead of dev_*.

Signed-off-by: Andres Salomon <dilinger@collabora.co.uk>
---
 drivers/gpio/cs5535-gpio.c |   13 +++++--------
 1 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/cs5535-gpio.c b/drivers/gpio/cs5535-gpio.c
index ec1f0e8..0fdbe94 100644
--- a/drivers/gpio/cs5535-gpio.c
+++ b/drivers/gpio/cs5535-gpio.c
@@ -134,8 +134,8 @@ static int chip_gpio_request(struct gpio_chip *c, unsigned offset)
 
 	/* 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);
+		dev_info(&chip->pdev->dev,
+			"pin %u is not available (check mask)\n", offset);
 		spin_unlock_irqrestore(&chip->lock, flags);
 		return -EINVAL;
 	}
@@ -265,18 +265,15 @@ static int __init cs5535_gpio_probe(struct pci_dev *pdev,
 	mask &= ~(1 << 28);
 
 	if (mask_orig != mask)
-		printk(KERN_INFO DRV_NAME
-				": mask changed from 0x%08lX to 0x%08lX\n",
+		dev_info(&pdev->dev, "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) {
-		dev_err(&pdev->dev, "failed to register gpio chip\n");
+	if (err)
 		goto release_region;
-	}
 
-	printk(KERN_INFO DRV_NAME ": GPIO support successfully loaded.\n");
+	dev_info(&pdev->dev, DRV_NAME ": GPIO support successfully loaded.\n");
 	return 0;
 
 release_region:
-- 
1.5.6.5