gpio: add a driver for GPIO pins found on AMD-8111 south bridge chips

PATCH Superseded Dmitry Eremin-Solenikov 3 participants 4 messages Apr 27, 2012 – May 18, 2012 GPIO

lore

Share & tools

lore

Dmitry Eremin-Solenikov

author

2012-04-27 10:09 UTC #0 lore raw

PATCH 1/1 gpio: add a driver for GPIO pins found on AMD-8111 south bridge chips

Add a driver to use GPIO pins available on several AMD south bridges (currently only AMD 8111 is supported).

Signed-off-by Dmitry Eremin-Solenikov <[email protected]>

---
 drivers/gpio/Kconfig        |   12 +++
 drivers/gpio/Makefile       |    1 +
 drivers/gpio/gpio-amd8111.c |  218 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 231 insertions(+)
 create mode 100644 drivers/gpio/gpio-amd8111.c

drivers/gpio/Kconfig +12 -0

index d0c4118..b05902f 100644
@@ -379,6 +379,18 @@ config GPIO_BT8XX
 
 	  If unsure, say N.
 
+config GPIO_AMD8111
+	tristate "AMD 8111 GPIO driver"
+	depends on PCI
+	help
+	  The AMD 8111 south bridge contains 32 GPIO pins which can be used.
+
+	  Note, that usually system firmware/ACPI handles GPIO pins on their
+	  own and users might easily break their systems with uncarefull usage
+	  of this driver!
+
+	  If unsure, say N
+
 config GPIO_LANGWELL
 	bool "Intel Langwell/Penwell GPIO support"
 	depends on PCI && X86

drivers/gpio/Makefile +1 -0

index fa10df6..a6d2ac7 100644
@@ -11,6 +11,7 @@ obj-$(CONFIG_GPIO_74X164)	+= gpio-74x164.o
 obj-$(CONFIG_GPIO_AB8500)	+= gpio-ab8500.o
 obj-$(CONFIG_GPIO_ADP5520)	+= gpio-adp5520.o
 obj-$(CONFIG_GPIO_ADP5588)	+= gpio-adp5588.o
+obj-$(CONFIG_GPIO_AMD8111)	+= gpio-amd8111.o
 obj-$(CONFIG_GPIO_BT8XX)	+= gpio-bt8xx.o
 obj-$(CONFIG_GPIO_CS5535)	+= gpio-cs5535.o
 obj-$(CONFIG_GPIO_DA9052)	+= gpio-da9052.o

drivers/gpio/gpio-amd8111.c new file +218 -0

218 lines not on this page · raw message

Signature · 1 line

1.7.10

Linus Walleij

2012-04-27 10:52 UTC #1 lore raw

Quoted 4 lines · Dmitry Eremin-Solenikov

Add a driver to use GPIO pins available on several AMD south bridges (currently only AMD 8111 is supported).
Signed-off-by Dmitry Eremin-Solenikov <[email protected]>

Mainly nitpicks, it's looking good overall...

Quoted 3 lines

+ spin_lock_irqsave(&agp->lock, flags);
+ temp = ioread8(agp->pm - PMBASE_OFFSET + PMBASE_GPIO(offset));
+ temp = (temp & 0x1c) | (!!value);

This is a bit idomatic but I think I understand what's going on. However this 0x1C magic could be better of with some #define AMD_8111_GPIO_FOO don't you think?

drivers/gpio/gpio-amd8111.c line 107

Quoted 10 lines

drivers/gpio/gpio-amd8111.c new file +10 -0

+static int amd_gpio_get(struct gpio_chip *chip, unsigned offset)
+{
+       struct amd_gpio *agp = to_agp(chip);
+       u8 temp;
+
+       temp = ioread8(agp->pm - PMBASE_OFFSET + PMBASE_GPIO(offset));
+
+       dev_dbg(&agp->pdev->dev, "Getting gpio %d, reg=%02x\n", offset, temp);
+
+       return !!(temp & 0x20);

And this could be #define AMD_8111_GPIO_INVAL or so (I bet this bit has a name in the datasheet, right?)

drivers/gpio/gpio-amd8111.c line 118

Quoted 9 lines

drivers/gpio/gpio-amd8111.c new file +9 -0

+static int amd_gpio_dirout(struct gpio_chip *chip, unsigned offset, int value)
+{
+       struct amd_gpio *agp = to_agp(chip);
+       u8 temp;
+       unsigned long flags;
+
+       spin_lock_irqsave(&agp->lock, flags);
+       temp = ioread8(agp->pm - PMBASE_OFFSET + PMBASE_GPIO(offset));
+       temp = (temp & 0x10) | 0x4 | (!!value);

And so on...

drivers/gpio/gpio-amd8111.c line 159

Quoted 9 lines

+ iowrite8(temp, agp->pm - PMBASE_OFFSET + PMBASE_GPIO(offset));
+ spin_unlock_irqrestore(&agp->lock, flags);
drivers/gpio/gpio-amd8111.c new file +6 -0
+
+       dev_dbg(&agp->pdev->dev, "Dirout gpio %d, value %d, reg=%02x\n", offset, !!value, temp);
+
+       return 0;
+}
 
+static int __init mod_init(void)

amd_gpio_init() maybe? These names are nice for e.g. ftrace.

Quoted 1 line

+ printk(KERN_INFO "AMD-8111 GPIO detected\n");

pr_info() & friends are nice shorthands for this.

Quoted 1 line

+static void __exit mod_exit(void)

amd_gpio_mod_exit()?

Quoted 1 line

+MODULE_AUTHOR("The Linux Kernel team");

Don't be shy, put in your own name :-)

Yours,
Linus Walleij

Grant Likely

2012-05-18 22:35 UTC #2 lore raw

Quoted 9 lines · Linus Walleij

Quoted 4 lines · Dmitry Eremin-Solenikov
Add a driver to use GPIO pins available on several AMD south bridges (currently only AMD 8111 is supported).
Signed-off-by Dmitry Eremin-Solenikov <[email protected]>
Mainly nitpicks, it's looking good overall...

Hi Dimtry,

Do you have an updated version of this patch to address Linus' comments?

Quoted 61 lines

Quoted 4 lines
+ spin_lock_irqsave(&agp->lock, flags);
+ temp = ioread8(agp->pm - PMBASE_OFFSET + PMBASE_GPIO(offset));
+ temp = (temp & 0x1c) | (!!value);
This is a bit idomatic but I think I understand what's going on. However this 0x1C magic could be better of with some #define AMD_8111_GPIO_FOO don't you think?
drivers/gpio/gpio-amd8111.c line 107
Quoted 10 lines
drivers/gpio/gpio-amd8111.c new file +10 -0
+static int amd_gpio_get(struct gpio_chip *chip, unsigned offset)
+{
+       struct amd_gpio *agp = to_agp(chip);
+       u8 temp;
+
+       temp = ioread8(agp->pm - PMBASE_OFFSET + PMBASE_GPIO(offset));
+
+       dev_dbg(&agp->pdev->dev, "Getting gpio %d, reg=%02x\n", offset, temp);
+
+       return !!(temp & 0x20);
And this could be #define AMD_8111_GPIO_INVAL or so (I bet this bit has a name in the datasheet, right?)
drivers/gpio/gpio-amd8111.c line 118
Quoted 9 lines
drivers/gpio/gpio-amd8111.c new file +9 -0
+static int amd_gpio_dirout(struct gpio_chip *chip, unsigned offset, int value)
+{
+       struct amd_gpio *agp = to_agp(chip);
+       u8 temp;
+       unsigned long flags;
+
+       spin_lock_irqsave(&agp->lock, flags);
+       temp = ioread8(agp->pm - PMBASE_OFFSET + PMBASE_GPIO(offset));
+       temp = (temp & 0x10) | 0x4 | (!!value);
And so on...
drivers/gpio/gpio-amd8111.c line 159
Quoted 9 lines
+ iowrite8(temp, agp->pm - PMBASE_OFFSET + PMBASE_GPIO(offset));
+ spin_unlock_irqrestore(&agp->lock, flags);
drivers/gpio/gpio-amd8111.c new file +6 -0
+
+       dev_dbg(&agp->pdev->dev, "Dirout gpio %d, value %d, reg=%02x\n", offset, !!value, temp);
+
+       return 0;
+}
 
+static int __init mod_init(void)
amd_gpio_init() maybe? These names are nice for e.g. ftrace.
Quoted 1 line
+ printk(KERN_INFO "AMD-8111 GPIO detected\n");
pr_info() & friends are nice shorthands for this.
Quoted 1 line
+static void __exit mod_exit(void)
amd_gpio_mod_exit()?
Quoted 1 line
+MODULE_AUTHOR("The Linux Kernel team");
Don't be shy, put in your own name :-)
Yours,
Linus Walleij

Signature · 2 lines

Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

Dmitry Eremin-Solenikov

author

2012-05-18 22:43 UTC #3 lore raw

Quoted 7 lines · Grant Likely

Quoted 6 lines · Linus Walleij
Quoted 3 lines · Dmitry Eremin-Solenikov
Add a driver to use GPIO pins available on several AMD south bridges (currently only AMD 8111 is supported).

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
Signed-off-by Dmitry Eremin-Solenikov <[email protected]>
Mainly nitpicks, it's looking good overall...
Hi Dimtry,
Do you have an updated version of this patch to address Linus' comments?

Colleagues,

I was a little bit busy with job & private topics. I plan to post updated version
either during the weekend, or on Mon-Wed.

Quoted 68 lines

g.
Quoted 53 lines
Quoted 4 lines
+ spin_lock_irqsave(&agp->lock, flags);
+ temp = ioread8(agp->pm - PMBASE_OFFSET + PMBASE_GPIO(offset));
+ temp = (temp & 0x1c) | (!!value);
This is a bit idomatic but I think I understand what's going on. However this 0x1C magic could be better of with some #define AMD_8111_GPIO_FOO don't you think?
drivers/gpio/gpio-amd8111.c line 107
Quoted 10 lines
drivers/gpio/gpio-amd8111.c new file +10 -0
+static int amd_gpio_get(struct gpio_chip *chip, unsigned offset)
+{
+       struct amd_gpio *agp = to_agp(chip);
+       u8 temp;
+
+       temp = ioread8(agp->pm - PMBASE_OFFSET + PMBASE_GPIO(offset));
+
+       dev_dbg(&agp->pdev->dev, "Getting gpio %d, reg=%02x\n", offset, temp);
+
+       return !!(temp & 0x20);
And this could be #define AMD_8111_GPIO_INVAL or so (I bet this bit has a name in the datasheet, right?)
drivers/gpio/gpio-amd8111.c line 118
Quoted 9 lines
drivers/gpio/gpio-amd8111.c new file +9 -0
+static int amd_gpio_dirout(struct gpio_chip *chip, unsigned offset, int value)
+{
+       struct amd_gpio *agp = to_agp(chip);
+       u8 temp;
+       unsigned long flags;
+
+       spin_lock_irqsave(&agp->lock, flags);
+       temp = ioread8(agp->pm - PMBASE_OFFSET + PMBASE_GPIO(offset));
+       temp = (temp & 0x10) | 0x4 | (!!value);
And so on...
drivers/gpio/gpio-amd8111.c line 159
Quoted 9 lines
+ iowrite8(temp, agp->pm - PMBASE_OFFSET + PMBASE_GPIO(offset));
+ spin_unlock_irqrestore(&agp->lock, flags);
drivers/gpio/gpio-amd8111.c new file +6 -0
+
+       dev_dbg(&agp->pdev->dev, "Dirout gpio %d, value %d, reg=%02x\n", offset, !!value, temp);
+
+       return 0;
+}
 
+static int __init mod_init(void)
amd_gpio_init() maybe? These names are nice for e.g. ftrace.
Quoted 1 line
+ printk(KERN_INFO "AMD-8111 GPIO detected\n");
pr_info() & friends are nice shorthands for this.
Quoted 1 line
+static void __exit mod_exit(void)
Quoted 8 lines
amd_gpio_mod_exit()?
Quoted 1 line
+MODULE_AUTHOR("The Linux Kernel team");
Don't be shy, put in your own name :-)
Yours,
Linus Walleij
--
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

Signature · 2 lines

With best wishes
Dmitry