* gpio: introduce gpio-fch driver for AMD A45/A50M/A55E Fusion Controller Hub
@ 2013-07-04 12:41 Denis Turischev
2013-07-21 15:13 ` Linus Walleij
0 siblings, 1 reply; 2+ messages in thread
From: Denis Turischev @ 2013-07-04 12:41 UTC (permalink / raw)
To: linux-kernel; +Cc: Grant Likely, Linus Walleij
Signed-off-by: Denis Turischev <denis.turischev@compulab.co.il>
diff -uprN linux-3.10.orig/drivers/gpio/gpio-fch.c linux-3.10/drivers/gpio/gpio-fch.c
--- linux-3.10.orig/drivers/gpio/gpio-fch.c 1970-01-01 02:00:00.000000000 +0200
+++ linux-3.10/drivers/gpio/gpio-fch.c 2013-07-04 15:35:50.020672040 +0300
@@ -0,0 +1,247 @@
+/*
+ * gpio-fch.c - GPIO interface for AMD Fusion Controller Hub
+ *
+ * Copyright (c) 2013 CompuLab Ltd
+ * Author: Denis Turischev <denis.turischev@compulab.co.il>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License 2 as published
+ * by the Free Software Foundation.
+ *
+ * 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; see the file COPYING. If not, write to
+ * the Free Software Foundation, 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/errno.h>
+#include <linux/pci.h>
+#include <linux/gpio.h>
+
+static void __iomem *gpio_ba;
+u32 acpimmioaddr;
+
+#define GPIO_SPACE_OFFSET 0x100
+#define GPIO_SPACE_SIZE 0x100
+
+static DEFINE_SPINLOCK(gpio_lock);
+
+static DEFINE_PCI_DEVICE_TABLE(gpio_fch_tbl) = {
+ { PCI_DEVICE(PCI_VENDOR_ID_ATI, PCI_DEVICE_ID_ATI_SBX00_SMBUS) },
+ { 0, }
+};
+MODULE_DEVICE_TABLE(pci, gpio_fch_tbl);
+
+static int gpio_fch_direction_in(struct gpio_chip *gc, unsigned gpio_num)
+{
+ u8 curr_state;
+
+ spin_lock(&gpio_lock);
+
+ curr_state = ioread8(gpio_ba + gpio_num);
+ if (!(curr_state & (1 << 5)))
+ iowrite8(curr_state | (1 << 5), gpio_ba + gpio_num);
+
+ spin_unlock(&gpio_lock);
+ return 0;
+}
+
+static int gpio_fch_get(struct gpio_chip *gc, unsigned gpio_num)
+{
+ u8 curr_state, bit;
+ int res;
+
+ curr_state = ioread8(gpio_ba + gpio_num);
+
+ bit = 6;
+ if (curr_state & (1 << 5))
+ bit = 7;
+
+ res = !!((curr_state) & (1 << bit));
+ return res;
+}
+
+static void gpio_fch_set(struct gpio_chip *gc, unsigned gpio_num, int val)
+{
+ u8 curr_state;
+
+ spin_lock(&gpio_lock);
+
+ curr_state = ioread8(gpio_ba + gpio_num);
+ iowrite8((curr_state & ~(1 << 5)), gpio_ba + gpio_num);
+
+ if (val)
+ iowrite8(curr_state | (1 << 6), gpio_ba + gpio_num);
+ else
+ iowrite8(curr_state & ~(1 << 6), gpio_ba + gpio_num);
+
+ spin_unlock(&gpio_lock);
+}
+
+static int gpio_fch_direction_out(struct gpio_chip *gc,
+ unsigned gpio_num, int val)
+{
+ u8 curr_state;
+
+ spin_lock(&gpio_lock);
+
+ curr_state = ioread8(gpio_ba + gpio_num);
+
+ if (curr_state & (1 << 5))
+ iowrite8(curr_state & ~(1 << 5), gpio_ba + gpio_num);
+
+ spin_unlock(&gpio_lock);
+ return 0;
+}
+
+u32 read_pm_reg(u8 port)
+{
+ u32 res;
+
+ outb(port + 3, 0xCD6);
+ res = inb(0xCD7);
+ res = res << 8;
+
+ outb(port + 2, 0xCD6);
+ res = res + inb(0xCD7);
+ res = res << 8;
+
+ outb(port + 1, 0xCD6);
+ res = res + inb(0xCD7);
+ res = res << 8;
+
+ outb(port, 0xCD6);
+ res = res + inb(0xCD7);
+
+ return res;
+}
+
+static struct gpio_chip fch_gpio_chip0 = {
+ .label = "gpio_fch",
+ .owner = THIS_MODULE,
+ .get = gpio_fch_get,
+ .direction_input = gpio_fch_direction_in,
+ .set = gpio_fch_set,
+ .direction_output = gpio_fch_direction_out,
+};
+
+static struct gpio_chip fch_gpio_chip128 = {
+ .label = "gpio_fch",
+ .owner = THIS_MODULE,
+ .get = gpio_fch_get,
+ .direction_input = gpio_fch_direction_in,
+ .set = gpio_fch_set,
+ .direction_output = gpio_fch_direction_out,
+};
+
+static struct gpio_chip fch_gpio_chip160 = {
+ .label = "gpio_fch",
+ .owner = THIS_MODULE,
+ .get = gpio_fch_get,
+ .direction_input = gpio_fch_direction_in,
+ .set = gpio_fch_set,
+ .direction_output = gpio_fch_direction_out,
+};
+
+static int gpio_fch_probe(struct pci_dev *pdev, const struct pci_device_id *id)
+{
+ int ret;
+
+ acpimmioaddr = read_pm_reg(0x24) & 0xFFFFF000;
+
+ if (!request_mem_region(acpimmioaddr + GPIO_SPACE_OFFSET,
+ GPIO_SPACE_SIZE, fch_gpio_chip0.label))
+ return -EBUSY;
+
+ gpio_ba = ioremap(acpimmioaddr + GPIO_SPACE_OFFSET, GPIO_SPACE_SIZE);
+ if (!gpio_ba) {
+ ret = -ENOMEM;
+ goto err_no_ioremap;
+ }
+
+ fch_gpio_chip0.base = 0;
+ fch_gpio_chip0.ngpio = 68;
+
+ fch_gpio_chip128.base = 128;
+ fch_gpio_chip128.ngpio = 23;
+
+ fch_gpio_chip160.base = 160;
+ fch_gpio_chip160.ngpio = 69;
+
+ ret = gpiochip_add(&fch_gpio_chip0);
+ if (ret < 0)
+ goto err_no_gpiochip0_add;
+
+ ret = gpiochip_add(&fch_gpio_chip128);
+ if (ret < 0)
+ goto err_no_gpiochip128_add;
+
+ ret = gpiochip_add(&fch_gpio_chip160);
+ if (ret < 0)
+ goto err_no_gpiochip160_add;
+
+ return 0;
+
+err_no_gpiochip160_add:
+ ret = gpiochip_remove(&fch_gpio_chip128);
+ if (ret)
+ dev_err(&pdev->dev, "%s failed, %d\n",
+ "gpiochip_remove()", ret);
+
+err_no_gpiochip128_add:
+ ret = gpiochip_remove(&fch_gpio_chip0);
+ if (ret)
+ dev_err(&pdev->dev, "%s failed, %d\n",
+ "gpiochip_remove()", ret);
+
+err_no_gpiochip0_add:
+ iounmap(gpio_ba);
+
+err_no_ioremap:
+ release_mem_region(acpimmioaddr + GPIO_SPACE_OFFSET, GPIO_SPACE_SIZE);
+ return ret;
+}
+
+static void gpio_fch_remove(struct pci_dev *pdev)
+{
+ int ret;
+
+ ret = gpiochip_remove(&fch_gpio_chip160);
+ if (ret < 0)
+ dev_err(&pdev->dev, "%s failed, %d\n",
+ "gpiochip_remove()", ret);
+
+ ret = gpiochip_remove(&fch_gpio_chip128);
+ if (ret < 0)
+ dev_err(&pdev->dev, "%s failed, %d\n",
+ "gpiochip_remove()", ret);
+
+ ret = gpiochip_remove(&fch_gpio_chip0);
+ if (ret < 0)
+ dev_err(&pdev->dev, "%s failed, %d\n",
+ "gpiochip_remove()", ret);
+
+ iounmap(gpio_ba);
+ release_mem_region(acpimmioaddr + GPIO_SPACE_OFFSET, GPIO_SPACE_SIZE);
+}
+
+static struct pci_driver gpio_fch_driver = {
+ .name = "gpio_fch",
+ .id_table = gpio_fch_tbl,
+ .probe = gpio_fch_probe,
+ .remove = gpio_fch_remove,
+};
+
+module_pci_driver(gpio_fch_driver);
+
+MODULE_AUTHOR("Denis Turischev <denis@compulab.co.il>");
+MODULE_DESCRIPTION("GPIO interface for AMD Fusion Controller Hub");
+MODULE_LICENSE("GPL");
+
diff -uprN linux-3.10.orig/drivers/gpio/Kconfig linux-3.10/drivers/gpio/Kconfig
--- linux-3.10.orig/drivers/gpio/Kconfig 2013-07-01 01:13:29.000000000 +0300
+++ linux-3.10/drivers/gpio/Kconfig 2013-07-04 14:12:15.412806610 +0300
@@ -135,6 +135,17 @@ config GPIO_EP93XX
depends on ARCH_EP93XX
select GPIO_GENERIC
+config GPIO_FCH
+ tristate "AMD A45/A50M/A55E Fusion Controller Hub GPIO"
+ depends on PCI && X86
+ default m
+ help
+ GPIO interface for AMD A45/A50M/A55E Fusion Controller Hub.
+ Available GPIOs are 0-67, 128-150, 160-228.
+
+ Part of GPIOs is usually occupied by BIOS for it's internal needs, so
+ use them with care.
+
config GPIO_MM_LANTIQ
bool "Lantiq Memory mapped GPIOs"
depends on LANTIQ && SOC_XWAY
diff -uprN linux-3.10.orig/drivers/gpio/Makefile linux-3.10/drivers/gpio/Makefile
--- linux-3.10.orig/drivers/gpio/Makefile 2013-07-01 01:13:29.000000000 +0300
+++ linux-3.10/drivers/gpio/Makefile 2013-07-04 09:39:38.605245556 +0300
@@ -24,6 +24,7 @@ obj-$(CONFIG_GPIO_DA9055) += gpio-da9055
obj-$(CONFIG_ARCH_DAVINCI) += gpio-davinci.o
obj-$(CONFIG_GPIO_EM) += gpio-em.o
obj-$(CONFIG_GPIO_EP93XX) += gpio-ep93xx.o
+obj-$(CONFIG_GPIO_FCH) += gpio-fch.o
obj-$(CONFIG_GPIO_GE_FPGA) += gpio-ge.o
obj-$(CONFIG_GPIO_GRGPIO) += gpio-grgpio.o
obj-$(CONFIG_GPIO_ICH) += gpio-ich.o
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: gpio: introduce gpio-fch driver for AMD A45/A50M/A55E Fusion Controller Hub
2013-07-04 12:41 gpio: introduce gpio-fch driver for AMD A45/A50M/A55E Fusion Controller Hub Denis Turischev
@ 2013-07-21 15:13 ` Linus Walleij
0 siblings, 0 replies; 2+ messages in thread
From: Linus Walleij @ 2013-07-21 15:13 UTC (permalink / raw)
To: Denis Turischev; +Cc: linux-kernel@vger.kernel.org, Grant Likely
On Thu, Jul 4, 2013 at 2:41 PM, Denis Turischev
<denis.turischev@compulab.co.il> wrote:
> Signed-off-by: Denis Turischev <denis.turischev@compulab.co.il>
Insert some commit message above describing what this is all about.
What kind of hardware this is, which arch/machine it is found in, etc.
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/errno.h>
> +#include <linux/pci.h>
> +#include <linux/gpio.h>
#include <linux/bitops.h>
> +static void __iomem *gpio_ba;
Just call this "base" per convention.
> +u32 acpimmioaddr;
> +
> +#define GPIO_SPACE_OFFSET 0x100
> +#define GPIO_SPACE_SIZE 0x100
> +
> +static DEFINE_SPINLOCK(gpio_lock);
So gpio_ba, acpimmioaddr and gpio_lock are static locals.
What happens if a user has two such PCI devices in their machine?
Even if this does not appear in practice, the code is more readable
if you assume that.
I think you need to move these into a devm_kzalloc():ed struct
that get instantiated for every device in probe().
Follow the pattern found in many other drivers such as
gpio-pch.c
(...)
> +static int gpio_fch_direction_in(struct gpio_chip *gc, unsigned gpio_num)
> +{
> + u8 curr_state;
> +
> + spin_lock(&gpio_lock);
> +
> + curr_state = ioread8(gpio_ba + gpio_num);
> + if (!(curr_state & (1 << 5)))
> + iowrite8(curr_state | (1 << 5), gpio_ba + gpio_num);
Use BIT(5) instead of (1 << 5) above, easier for me to read.
Repeat that pattern everywhere applicable.
(Some think I'm picky for this but I really like it.)
> +static void gpio_fch_set(struct gpio_chip *gc, unsigned gpio_num, int val)
> +{
> + u8 curr_state;
> +
> + spin_lock(&gpio_lock);
> +
> + curr_state = ioread8(gpio_ba + gpio_num);
> + iowrite8((curr_state & ~(1 << 5)), gpio_ba + gpio_num);
> +
> + if (val)
> + iowrite8(curr_state | (1 << 6), gpio_ba + gpio_num);
> + else
> + iowrite8(curr_state & ~(1 << 6), gpio_ba + gpio_num);
> +
> + spin_unlock(&gpio_lock);
> +}
Hm this looks a bit familiar. I had a quick look around
but couldn't quite find siblings ... but make sure you are
not reimplementing something now.
> +u32 read_pm_reg(u8 port)
> +{
> + u32 res;
> +
> + outb(port + 3, 0xCD6);
> + res = inb(0xCD7);
#define these magic offsets.
> + res = res << 8;
> +
> + outb(port + 2, 0xCD6);
> + res = res + inb(0xCD7);
> + res = res << 8;
> +
> + outb(port + 1, 0xCD6);
> + res = res + inb(0xCD7);
> + res = res << 8;
> +
> + outb(port, 0xCD6);
> + res = res + inb(0xCD7);
> +
> + return res;
> +}
On the whole, explain what this function is doing with
some kerneldoc.
> +static int gpio_fch_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> + int ret;
> +
> + acpimmioaddr = read_pm_reg(0x24) & 0xFFFFF000;
#define these magic things as well. I can see it is reading out the
base address.
What happens on some random system if you start reading
on port 0x24 like this?
But "ACPImmioaddr", what does this really mean? Where is this address
coming from? What does it have to do with ACPI?
This seems like some ISA-style reading around in regs and probing
(that is actually why the probe() function is named probe())
but with ACPI I was under the impression that you were to query
ACPI for things like base addresses?
> + if (!request_mem_region(acpimmioaddr + GPIO_SPACE_OFFSET,
> + GPIO_SPACE_SIZE, fch_gpio_chip0.label))
> + return -EBUSY;
> +
> + gpio_ba = ioremap(acpimmioaddr + GPIO_SPACE_OFFSET, GPIO_SPACE_SIZE);
> + if (!gpio_ba) {
> + ret = -ENOMEM;
> + goto err_no_ioremap;
> + }
Stuff the resulting address into a struct resource and then use
devm_ioremap_resource() to remap.
> +
> + fch_gpio_chip0.base = 0;
> + fch_gpio_chip0.ngpio = 68;
> +
> + fch_gpio_chip128.base = 128;
> + fch_gpio_chip128.ngpio = 23;
> +
> + fch_gpio_chip160.base = 160;
> + fch_gpio_chip160.ngpio = 69;
This is also characteristics that seem like they could vary with the
PCI ID for newer versions. Isn't it better to make this a per-pci-id
table from the beginning? It also simplifies adding these
chips as you have 3x the same code adding these chips.
> +err_no_gpiochip160_add:
> + ret = gpiochip_remove(&fch_gpio_chip128);
> + if (ret)
> + dev_err(&pdev->dev, "%s failed, %d\n",
> + "gpiochip_remove()", ret);
> +
> +err_no_gpiochip128_add:
> + ret = gpiochip_remove(&fch_gpio_chip0);
> + if (ret)
> + dev_err(&pdev->dev, "%s failed, %d\n",
> + "gpiochip_remove()", ret);
> +
> +err_no_gpiochip0_add:
> + iounmap(gpio_ba);
Not needed with devm_* managed resources.
> +
> +err_no_ioremap:
> + release_mem_region(acpimmioaddr + GPIO_SPACE_OFFSET, GPIO_SPACE_SIZE);
Dito.
> + return ret;
> +}
> +
> +static void gpio_fch_remove(struct pci_dev *pdev)
> +{
> + int ret;
> +
> + ret = gpiochip_remove(&fch_gpio_chip160);
> + if (ret < 0)
> + dev_err(&pdev->dev, "%s failed, %d\n",
> + "gpiochip_remove()", ret);
> +
> + ret = gpiochip_remove(&fch_gpio_chip128);
> + if (ret < 0)
> + dev_err(&pdev->dev, "%s failed, %d\n",
> + "gpiochip_remove()", ret);
> +
> + ret = gpiochip_remove(&fch_gpio_chip0);
> + if (ret < 0)
> + dev_err(&pdev->dev, "%s failed, %d\n",
> + "gpiochip_remove()", ret);
Here you see the simplification that can be achieved by using
a table. just iterate over it.
> +
> + iounmap(gpio_ba);
> + release_mem_region(acpimmioaddr + GPIO_SPACE_OFFSET, GPIO_SPACE_SIZE);
Dito.
> +config GPIO_FCH
> + tristate "AMD A45/A50M/A55E Fusion Controller Hub GPIO"
> + depends on PCI && X86
> + default m
> + help
> + GPIO interface for AMD A45/A50M/A55E Fusion Controller Hub.
> + Available GPIOs are 0-67, 128-150, 160-228.
> +
> + Part of GPIOs is usually occupied by BIOS for it's internal needs, so
> + use them with care.
So is it impossible to get that information out of the BIOS
so we can avoid poking them altogether?
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2013-07-21 15:13 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-07-04 12:41 gpio: introduce gpio-fch driver for AMD A45/A50M/A55E Fusion Controller Hub Denis Turischev
2013-07-21 15:13 ` Linus Walleij
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox