* [PATCH] gpio: add Intel BayTrail gpio driver
@ 2013-05-29 8:01 Mathias Nyman
2013-05-29 19:29 ` Andy Shevchenko
2013-05-30 19:26 ` Linus Walleij
0 siblings, 2 replies; 4+ messages in thread
From: Mathias Nyman @ 2013-05-29 8:01 UTC (permalink / raw)
To: Linus; +Cc: grant.likely, linux-kernel, Mathias Nyman
Add support for gpio on Intel BayTrail platforms. BayTrail supports 3 banks
of gpios called SCORE, NCORE ans SUS with 102, 28 and 43 gpio pins.
Supports gpio interrupts
Pins may be muxed to alternate function instead of gpio by firmware.
This driver does not touch the pin muxing and expect firmare
to set pin muxing and pullup/down properties properly.
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
drivers/gpio/Kconfig | 12 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-baytrail.c | 539 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 552 insertions(+), 0 deletions(-)
create mode 100644 drivers/gpio/gpio-baytrail.c
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 573c449..77d7f35 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -108,6 +108,18 @@ config GPIO_MAX730X
comment "Memory mapped GPIO drivers:"
+config GPIO_BAYTRAIL
+ bool "Intel Baytrail GPIO support"
+ depends on ACPI && X86
+ select IRQ_DOMAIN
+ help
+ driver for memory mapped GPIO functionality on Intel Baytrail
+ platforms. Supports 3 banks with 102, 28 and 43 gpios.
+ Most pins are usually muxed to some other functionality by firmware,
+ so only a small amount is available for gpio use.
+
+ Requires ACPI device enumeration code to set up a platform device.
+
config GPIO_CLPS711X
def_bool y
depends on ARCH_CLPS711X
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 0cb2d65..bc1f6b3 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_GPIO_ADP5520) += gpio-adp5520.o
obj-$(CONFIG_GPIO_ADP5588) += gpio-adp5588.o
obj-$(CONFIG_GPIO_AMD8111) += gpio-amd8111.o
obj-$(CONFIG_GPIO_ARIZONA) += gpio-arizona.o
+obj-$(CONFIG_GPIO_BAYTRAIL) += gpio-baytrail.o
obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o
obj-$(CONFIG_GPIO_CLPS711X) += gpio-clps711x.o
obj-$(CONFIG_GPIO_CS5535) += gpio-cs5535.o
diff --git a/drivers/gpio/gpio-baytrail.c b/drivers/gpio/gpio-baytrail.c
new file mode 100644
index 0000000..afe64cb
--- /dev/null
+++ b/drivers/gpio/gpio-baytrail.c
@@ -0,0 +1,539 @@
+/*
+ * GPIO driver for Intel Baytrail
+ * Copyright (c) 2012-2013, Intel Corporation.
+ *
+ * Author: Mathias Nyman <mathias.nyman@linux.intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/types.h>
+#include <linux/bitops.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/gpio.h>
+#include <linux/irqdomain.h>
+#include <linux/acpi.h>
+#include <linux/acpi_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/seq_file.h>
+#include <linux/io.h>
+#include <linux/pm_runtime.h>
+
+/* memory mapped register offsets */
+#define BYT_CONF0_REG 0x000
+#define BYT_CONF1_REG 0x004
+#define BYT_VAL_REG 0x008
+#define BYT_DFT_REG 0x00c
+#define BYT_INT_STAT_REG 0x800
+
+/* BYT_CONF0_REG register bits */
+#define BYT_TRIG_NEG BIT(26)
+#define BYT_TRIG_POS BIT(25)
+#define BYT_TRIG_LVL BIT(24)
+#define BYT_PIN_MUX 0x07
+
+/* BYT_VAL_REG register bits */
+#define BYT_INPUT_EN BIT(2) /* 0: input enabled (active low)*/
+#define BYT_OUTPUT_EN BIT(1) /* 0: output enabled (active low)*/
+#define BYT_LEVEL BIT(0)
+
+#define BYT_DIR_MASK (BIT(1) | BIT(2))
+#define BYT_TRIG_MASK (BIT(26) | BIT(25) | BIT(24))
+
+#define BYT_NGPIO_SCORE 102
+#define BYT_NGPIO_NCORE 28
+#define BYT_NGPIO_SUS 43
+
+/*
+ * Baytrail gpio controller consist of three separate sub-controllers called
+ * SCORE, NCORE and SUS. The sub-controllers are identified by their acpi UID.
+ *
+ * GPIO numbering is _not_ ordered meaning that gpio # 0 in ACPI namespace does
+ * _not_ correspond to the first gpio register at controller's gpio base.
+ * There is no logic or pattern in mapping gpio numbers to registers (pads) so
+ * each sub-controller needs to have its own mapping table
+ */
+
+static unsigned score_gpio_to_pad[BYT_NGPIO_SCORE] = {
+ 85, 89, 93, 96, 99, 102, 98, 101, 34, 37,
+ 36, 38, 39, 35, 40, 84, 62, 61, 64, 59,
+ 54, 56, 60, 55, 63, 57, 51, 50, 53, 47,
+ 52, 49, 48, 43, 46, 41, 45, 42, 58, 44,
+ 95, 105, 70, 68, 67, 66, 69, 71, 65, 72,
+ 86, 90, 88, 92, 103, 77, 79, 83, 78, 81,
+ 80, 82, 13, 12, 15, 14, 17, 18, 19, 16,
+ 2, 1, 0, 4, 6, 7, 9, 8, 33, 32,
+ 31, 30, 29, 27, 25, 28, 26, 23, 21, 20,
+ 24, 22, 5, 3, 10, 11, 106, 87, 91, 104,
+ 97, 100,
+};
+
+static unsigned ncore_gpio_to_pad[BYT_NGPIO_NCORE] = {
+ 19, 18, 17, 20, 21, 22, 24, 25, 23, 16,
+ 14, 15, 12, 26, 27, 1, 4, 8, 11, 0,
+ 3, 6, 10, 13, 2, 5, 9, 7,
+};
+
+static unsigned sus_gpio_to_pad[BYT_NGPIO_SUS] = {
+ 29, 33, 30, 31, 32, 34, 36, 35, 38, 37,
+ 18, 11, 20, 17, 1, 8, 10, 19, 12, 0,
+ 2, 23, 39, 28, 27, 22, 21, 24, 25, 26,
+ 51, 56, 54, 49, 55, 48, 57, 50, 58, 52,
+ 53, 59, 40,
+};
+
+struct gpio_bank {
+ char *uid; /* acpi _UID */
+ int ngpio;
+ unsigned *to_pad;
+};
+
+static struct gpio_bank byt_banks[] = {
+ {
+ .uid = "1",
+ .ngpio = BYT_NGPIO_SCORE,
+ .to_pad = score_gpio_to_pad,
+ },
+ {
+ .uid = "2",
+ .ngpio = BYT_NGPIO_NCORE,
+ .to_pad = ncore_gpio_to_pad,
+ },
+ {
+ .uid = "3",
+ .ngpio = BYT_NGPIO_SUS,
+ .to_pad = sus_gpio_to_pad,
+ },
+ {
+ },
+};
+
+struct byt_gpio {
+ struct gpio_chip chip;
+ struct irq_domain *domain;
+ struct platform_device *pdev;
+ spinlock_t lock;
+ void __iomem *reg_base;
+ unsigned *gpio_to_pad;
+};
+
+static void __iomem *byt_gpio_reg(struct gpio_chip *chip, unsigned offset,
+ int reg)
+{
+ struct byt_gpio *vg = container_of(chip, struct byt_gpio, chip);
+ u32 reg_offset;
+ void __iomem *ptr;
+
+ if (reg == BYT_INT_STAT_REG)
+ reg_offset = (offset / 32) * 4;
+ else
+ reg_offset = vg->gpio_to_pad[offset] * 16;
+
+ ptr = (void __iomem *) (vg->reg_base + reg_offset + reg);
+ return ptr;
+}
+
+static int byt_gpio_request(struct gpio_chip *chip, unsigned offset)
+{
+ struct byt_gpio *vg = container_of(chip, struct byt_gpio, chip);
+
+/* Policy about what should be done when requesting a gpio is unclear.
+ * In most cases PIN MUX 000 means gpio function, with the exception of SUS
+ * core pins 11-21 where gpio is mux 001.
+ *
+ * Some pins are set by bios to a non-gpio mux, but still marked as gpio
+ * resource in acpi tables, and they work just as they should when not touching
+ * the pin muxing. (For example mmc card detect switch)
+ *
+ * option 1, check pin mux is "gpio", else fail (FIXME gpio SUS pins 11-21):
+ * void __iomem *reg = byt_gpio_reg(chip, offset, BYT_CONF0_REG);
+ * u32 value;
+ * value = readl(reg) & BYT_PIN_MUX;
+ * if (value)
+ * return -EINVAL;
+ *
+ * option 2, force pin mux to gpio mode (FIXME gpio SUS pins 11-21):
+ * void __iomem *reg = byt_gpio_reg(chip, offset, BYT_CONF0_REG);
+ * u32 value;
+ * value = readl(reg) & BYT_PIN_MUX;
+ * writel(value & ~BYT_PIN_MUX, reg);
+ *
+ * option 3: don't touch the pinmuxing at all, let BIOS handle it
+ */
+ pm_runtime_get(&vg->pdev->dev);
+
+ return 0;
+}
+
+static void byt_gpio_free(struct gpio_chip *chip, unsigned offset)
+{
+ struct byt_gpio *vg = container_of(chip, struct byt_gpio, chip);
+
+ pm_runtime_put(&vg->pdev->dev);
+}
+
+static int byt_irq_type(struct irq_data *d, unsigned type)
+{
+ struct byt_gpio *vg = irq_data_get_irq_chip_data(d);
+ u32 offset = irqd_to_hwirq(d);
+ u32 value;
+ unsigned long flags;
+ void __iomem *reg = byt_gpio_reg(&vg->chip, offset, BYT_CONF0_REG);
+
+ if (offset >= vg->chip.ngpio)
+ return -EINVAL;
+
+ spin_lock_irqsave(&vg->lock, flags);
+ value = readl(reg);
+
+ if (type & IRQ_TYPE_EDGE_RISING)
+ value |= BYT_TRIG_POS;
+ else
+ value &= ~BYT_TRIG_POS;
+
+ if (type & IRQ_TYPE_EDGE_FALLING)
+ value |= BYT_TRIG_NEG;
+ else
+ value &= ~BYT_TRIG_NEG;
+
+ writel(value, reg);
+
+ spin_unlock_irqrestore(&vg->lock, flags);
+
+ return 0;
+}
+
+static int byt_gpio_get(struct gpio_chip *chip, unsigned offset)
+{
+ void __iomem *reg = byt_gpio_reg(chip, offset, BYT_VAL_REG);
+ return readl(reg) & BYT_LEVEL;
+}
+
+static void byt_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
+{
+ struct byt_gpio *vg = container_of(chip, struct byt_gpio, chip);
+ void __iomem *reg = byt_gpio_reg(chip, offset, BYT_VAL_REG);
+ unsigned long flags;
+ u32 old_val;
+
+ spin_lock_irqsave(&vg->lock, flags);
+
+ old_val = readl(reg);
+
+ if (value)
+ writel(old_val | BYT_LEVEL, reg);
+ else
+ writel(old_val & ~BYT_LEVEL, reg);
+
+ spin_unlock_irqrestore(&vg->lock, flags);
+}
+
+static int byt_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
+{
+ struct byt_gpio *vg = container_of(chip, struct byt_gpio, chip);
+ void __iomem *reg = byt_gpio_reg(chip, offset, BYT_VAL_REG);
+ unsigned long flags;
+ u32 value;
+
+ spin_lock_irqsave(&vg->lock, flags);
+
+ value = readl(reg) | BYT_DIR_MASK;
+ value = value & (~BYT_INPUT_EN); /* active low */
+ writel(value, reg);
+
+ spin_unlock_irqrestore(&vg->lock, flags);
+
+ return 0;
+}
+
+static int byt_gpio_direction_output(struct gpio_chip *chip,
+ unsigned gpio, int value)
+{
+ struct byt_gpio *vg = container_of(chip, struct byt_gpio, chip);
+ void __iomem *reg = byt_gpio_reg(chip, gpio, BYT_VAL_REG);
+ unsigned long flags;
+ u32 reg_val;
+
+ spin_lock_irqsave(&vg->lock, flags);
+
+ reg_val = readl(reg) | (BYT_DIR_MASK | !!value);
+ reg_val &= ~(BYT_OUTPUT_EN | !value);
+ writel(reg_val, reg);
+
+ spin_unlock_irqrestore(&vg->lock, flags);
+
+ return 0;
+}
+
+static void byt_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
+{
+ struct byt_gpio *vg = container_of(chip, struct byt_gpio, chip);
+ int i;
+ unsigned long flags;
+ u32 conf0, val, offs;
+
+ spin_lock_irqsave(&vg->lock, flags);
+
+ for (i = 0; i < vg->chip.ngpio; i++) {
+
+ offs = vg->gpio_to_pad[i] * 16;
+ conf0 = readl(vg->reg_base + offs + BYT_CONF0_REG);
+ val = readl(vg->reg_base + offs + BYT_VAL_REG);
+
+ seq_printf(s,
+ " gpio-%-3d %s %s %s pad-%-3d offset:0x%03x mux:%d %s %s %s\n",
+ i,
+ val & BYT_INPUT_EN ? " " : "in",
+ val & BYT_OUTPUT_EN ? " " : "out",
+ val & BYT_LEVEL ? "hi" : "lo",
+ vg->gpio_to_pad[i], offs,
+ conf0 & 0x7,
+ conf0 & BYT_TRIG_NEG ? "fall" : "",
+ conf0 & BYT_TRIG_POS ? "rise" : "",
+ conf0 & BYT_TRIG_LVL ? "lvl " : "");
+
+ }
+ spin_unlock_irqrestore(&vg->lock, flags);
+}
+
+static int byt_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
+{
+ struct byt_gpio *vg = container_of(chip, struct byt_gpio, chip);
+ return irq_create_mapping(vg->domain, offset);
+}
+
+static void byt_gpio_irq_handler(unsigned irq, struct irq_desc *desc)
+{
+ struct irq_data *data = irq_desc_get_irq_data(desc);
+ struct byt_gpio *vg = irq_data_get_irq_handler_data(data);
+ struct irq_chip *chip = irq_data_get_irq_chip(data);
+ u32 base, pin, mask;
+ void __iomem *reg;
+ u32 pending;
+ unsigned virq;
+
+ /* check from GPIO controller which pin triggered the interrupt */
+ for (base = 0; base < vg->chip.ngpio; base += 32) {
+
+ reg = byt_gpio_reg(&vg->chip, base, BYT_INT_STAT_REG);
+
+ while ((pending = readl(reg))) {
+ pin = __ffs(pending);
+ mask = BIT(pin);
+ /* Clear before handling so we can't lose an edge */
+ writel(mask, reg);
+
+ virq = irq_find_mapping(vg->domain, base + pin);
+ generic_handle_irq(virq);
+ }
+ }
+ chip->irq_eoi(data);
+}
+
+static void byt_irq_unmask(struct irq_data *d)
+{
+}
+
+static void byt_irq_mask(struct irq_data *d)
+{
+}
+
+static struct irq_chip byt_irqchip = {
+ .name = "BYT-GPIO",
+ .irq_mask = byt_irq_mask,
+ .irq_unmask = byt_irq_unmask,
+ .irq_set_type = byt_irq_type,
+};
+
+static void byt_gpio_irq_init_hw(struct byt_gpio *vg)
+{
+ void __iomem *reg;
+ u32 base;
+
+ /* clear interrupt status trigger registers */
+ for (base = 0; base < vg->chip.ngpio; base += 32) {
+ reg = byt_gpio_reg(&vg->chip, base, BYT_INT_STAT_REG);
+ writel(0xffffffff, reg);
+ }
+}
+
+static int byt_gpio_irq_map(struct irq_domain *d, unsigned int virq,
+ irq_hw_number_t hw)
+{
+ struct byt_gpio *vg = d->host_data;
+
+ irq_set_chip_and_handler_name(virq, &byt_irqchip, handle_simple_irq,
+ "demux");
+ irq_set_chip_data(virq, vg);
+ irq_set_irq_type(virq, IRQ_TYPE_NONE);
+
+ return 0;
+}
+
+static const struct irq_domain_ops byt_gpio_irq_ops = {
+ .map = byt_gpio_irq_map,
+};
+
+static int byt_gpio_probe(struct platform_device *pdev)
+{
+ struct byt_gpio *vg;
+ struct gpio_chip *gc;
+ struct resource *mem_rc, *irq_rc;
+ struct device *dev = &pdev->dev;
+ struct acpi_device *acpi_dev;
+ struct gpio_bank *bank;
+ acpi_handle handle = ACPI_HANDLE(dev);
+ unsigned hwirq;
+ int ret;
+
+ if (acpi_bus_get_device(handle, &acpi_dev))
+ return -ENODEV;
+
+ vg = devm_kzalloc(dev, sizeof(struct byt_gpio), GFP_KERNEL);
+ if (!vg) {
+ dev_err(&pdev->dev, "can't allocate byt_gpio chip data\n");
+ return -ENOMEM;
+ }
+
+ for (bank = byt_banks; bank->uid; bank++) {
+ if (!strcmp(acpi_dev->pnp.unique_id, bank->uid)) {
+ vg->chip.ngpio = bank->ngpio;
+ vg->gpio_to_pad = bank->to_pad;
+ break;
+ }
+ }
+
+ if (!vg->chip.ngpio || !vg->gpio_to_pad)
+ return -ENODEV;
+
+ vg->pdev = pdev;
+ platform_set_drvdata(pdev, vg);
+
+ mem_rc = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ irq_rc = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+
+ if (!mem_rc) {
+ dev_err(&pdev->dev, "missing MEM resource\n");
+ return -EINVAL;
+ }
+
+ vg->reg_base = devm_ioremap_resource(dev, mem_rc);
+
+ if (IS_ERR(vg->reg_base)) {
+ dev_err(&pdev->dev, "error mapping resource\n");
+ return PTR_ERR(vg->reg_base);
+ }
+
+ spin_lock_init(&vg->lock);
+
+ gc = &vg->chip;
+ gc->label = dev_name(&pdev->dev);
+ gc->owner = THIS_MODULE;
+ gc->request = byt_gpio_request;
+ gc->free = byt_gpio_free;
+ gc->direction_input = byt_gpio_direction_input;
+ gc->direction_output = byt_gpio_direction_output;
+ gc->get = byt_gpio_get;
+ gc->set = byt_gpio_set;
+ gc->dbg_show = byt_gpio_dbg_show;
+ gc->base = -1;
+ gc->can_sleep = 0;
+ gc->dev = dev;
+
+ ret = gpiochip_add(gc);
+ if (ret) {
+ dev_err(&pdev->dev, "failed adding byt-gpio chip\n");
+ return ret;
+ }
+
+ /* set up interrupts */
+ if (irq_rc && irq_rc->start) {
+ hwirq = irq_rc->start;
+ gc->to_irq = byt_gpio_to_irq;
+
+ vg->domain = irq_domain_add_linear(NULL, gc->ngpio,
+ &byt_gpio_irq_ops, vg);
+ if (!vg->domain)
+ return -ENXIO;
+
+ byt_gpio_irq_init_hw(vg);
+
+ irq_set_handler_data(hwirq, vg);
+ irq_set_chained_handler(hwirq, byt_gpio_irq_handler);
+ }
+
+ /* Register interrupt handlers for gpio signaled acpi events */
+ acpi_gpiochip_request_interrupts(gc);
+
+ pm_runtime_enable(dev);
+
+ return 0;
+}
+
+static int byt_gpio_runtime_suspend(struct device *dev)
+{
+ return 0;
+}
+
+static int byt_gpio_runtime_resume(struct device *dev)
+{
+ return 0;
+}
+
+static const struct dev_pm_ops byt_gpio_pm_ops = {
+ .runtime_suspend = byt_gpio_runtime_suspend,
+ .runtime_resume = byt_gpio_runtime_resume,
+};
+
+static const struct acpi_device_id byt_gpio_acpi_match[] = {
+ { "INT33B2", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(acpi, byt_gpio_acpi_match);
+
+static int byt_gpio_remove(struct platform_device *pdev)
+{
+ struct byt_gpio *vg = platform_get_drvdata(pdev);
+ int err;
+ pm_runtime_disable(&pdev->dev);
+ err = gpiochip_remove(&vg->chip);
+ if (err)
+ dev_warn(&pdev->dev, "failed to remove gpio_chip.\n");
+
+ platform_set_drvdata(pdev, NULL);
+ return 0;
+}
+
+static struct platform_driver byt_gpio_driver = {
+ .probe = byt_gpio_probe,
+ .remove = byt_gpio_remove,
+ .driver = {
+ .name = "byt_gpio",
+ .owner = THIS_MODULE,
+ .pm = &byt_gpio_pm_ops,
+ .acpi_match_table = ACPI_PTR(byt_gpio_acpi_match),
+ },
+};
+
+static int __init byt_gpio_init(void)
+{
+ return platform_driver_register(&byt_gpio_driver);
+}
+
+subsys_initcall(byt_gpio_init);
--
1.7.4.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] gpio: add Intel BayTrail gpio driver
2013-05-29 8:01 [PATCH] gpio: add Intel BayTrail gpio driver Mathias Nyman
@ 2013-05-29 19:29 ` Andy Shevchenko
2013-05-30 19:26 ` Linus Walleij
1 sibling, 0 replies; 4+ messages in thread
From: Andy Shevchenko @ 2013-05-29 19:29 UTC (permalink / raw)
To: Mathias Nyman; +Cc: Linus, Grant Likely, linux-kernel
On Wed, May 29, 2013 at 11:01 AM, Mathias Nyman
<mathias.nyman@linux.intel.com> wrote:
> Add support for gpio on Intel BayTrail platforms. BayTrail supports 3 banks
> of gpios called SCORE, NCORE ans SUS with 102, 28 and 43 gpio pins.
> Supports gpio interrupts
>
> Pins may be muxed to alternate function instead of gpio by firmware.
> This driver does not touch the pin muxing and expect firmare
> to set pin muxing and pullup/down properties properly.
Few comments below (it looks like you sent previous version of patch
than that we discussed early)
> --- /dev/null
> +++ b/drivers/gpio/gpio-baytrail.c
> @@ -0,0 +1,539 @@
> +static int byt_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
> +{
> + struct byt_gpio *vg = container_of(chip, struct byt_gpio, chip);
> + void __iomem *reg = byt_gpio_reg(chip, offset, BYT_VAL_REG);
> + unsigned long flags;
> + u32 value;
> +
> + spin_lock_irqsave(&vg->lock, flags);
> +
> + value = readl(reg) | BYT_DIR_MASK;
> + value = value & (~BYT_INPUT_EN); /* active low */
Just value &= ~BYT_INPUT_EN;
> +static void byt_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
> +{
> + struct byt_gpio *vg = container_of(chip, struct byt_gpio, chip);
> + int i;
> + unsigned long flags;
> + u32 conf0, val, offs;
Two spaces instead of one.
> +
> + spin_lock_irqsave(&vg->lock, flags);
> +
> + for (i = 0; i < vg->chip.ngpio; i++) {
> +
Seems redundant empty line.
> + offs = vg->gpio_to_pad[i] * 16;
> + conf0 = readl(vg->reg_base + offs + BYT_CONF0_REG);
> + val = readl(vg->reg_base + offs + BYT_VAL_REG);
> +
> + seq_printf(s,
> + " gpio-%-3d %s %s %s pad-%-3d offset:0x%03x mux:%d %s %s %s\n",
> + i,
> + val & BYT_INPUT_EN ? " " : "in",
> + val & BYT_OUTPUT_EN ? " " : "out",
> + val & BYT_LEVEL ? "hi" : "lo",
> + vg->gpio_to_pad[i], offs,
> + conf0 & 0x7,
> + conf0 & BYT_TRIG_NEG ? "fall" : "",
> + conf0 & BYT_TRIG_POS ? "rise" : "",
> + conf0 & BYT_TRIG_LVL ? "lvl " : "");
> +
Perhaps this one will go after curle brace.
> + }
> + spin_unlock_irqrestore(&vg->lock, flags);
> +}
> +static int byt_gpio_probe(struct platform_device *pdev)
> +{
> + struct byt_gpio *vg;
> + struct gpio_chip *gc;
> + struct resource *mem_rc, *irq_rc;
> + struct device *dev = &pdev->dev;
> + struct acpi_device *acpi_dev;
> + struct gpio_bank *bank;
> + acpi_handle handle = ACPI_HANDLE(dev);
> + unsigned hwirq;
> + int ret;
> +
> + mem_rc = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + irq_rc = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
Perhaps this one should go close to place where it is used
> + if (!mem_rc) {
> + dev_err(&pdev->dev, "missing MEM resource\n");
> + return -EINVAL;
> + }
> +
Just put
mem_rc = platform_get_resource(pdev, IORESOURCE_MEM, 0);
here and remove above check.
> + vg->reg_base = devm_ioremap_resource(dev, mem_rc);
> +
No need to have this empty line.
> + if (IS_ERR(vg->reg_base)) {
> + dev_err(&pdev->dev, "error mapping resource\n");
No need to have this message.
> + return PTR_ERR(vg->reg_base);
> + }
> + ret = gpiochip_add(gc);
> + if (ret) {
> + dev_err(&pdev->dev, "failed adding byt-gpio chip\n");
> + return ret;
> + }
> +
> + /* set up interrupts */
> + if (irq_rc && irq_rc->start) {
> + hwirq = irq_rc->start;
> + gc->to_irq = byt_gpio_to_irq;
> +
> + vg->domain = irq_domain_add_linear(NULL, gc->ngpio,
> + &byt_gpio_irq_ops, vg);
> + if (!vg->domain)
Have you to call gpiochip_remove() or something like this here?
> + return -ENXIO;
> +
> + byt_gpio_irq_init_hw(vg);
> +
> + irq_set_handler_data(hwirq, vg);
> + irq_set_chained_handler(hwirq, byt_gpio_irq_handler);
> + }
> +
> + /* Register interrupt handlers for gpio signaled acpi events */
> + acpi_gpiochip_request_interrupts(gc);
> +
> + pm_runtime_enable(dev);
> +
> + return 0;
> +}
> +static int byt_gpio_remove(struct platform_device *pdev)
> +{
> + struct byt_gpio *vg = platform_get_drvdata(pdev);
> + int err;
+ empty line here
> + pm_runtime_disable(&pdev->dev);
> + err = gpiochip_remove(&vg->chip);
> + if (err)
> + dev_warn(&pdev->dev, "failed to remove gpio_chip.\n");
> +
> + platform_set_drvdata(pdev, NULL);
No need to set this.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] gpio: add Intel BayTrail gpio driver
2013-05-29 8:01 [PATCH] gpio: add Intel BayTrail gpio driver Mathias Nyman
2013-05-29 19:29 ` Andy Shevchenko
@ 2013-05-30 19:26 ` Linus Walleij
2013-06-03 9:56 ` Mathias Nyman
1 sibling, 1 reply; 4+ messages in thread
From: Linus Walleij @ 2013-05-30 19:26 UTC (permalink / raw)
To: Mathias Nyman; +Cc: Grant Likely, linux-kernel
On Wed, May 29, 2013 at 10:01 AM, Mathias Nyman
<mathias.nyman@linux.intel.com> wrote:
> Add support for gpio on Intel BayTrail platforms. BayTrail supports 3 banks
> of gpios called SCORE, NCORE ans SUS with 102, 28 and 43 gpio pins.
> Supports gpio interrupts
>
> Pins may be muxed to alternate function instead of gpio by firmware.
> This driver does not touch the pin muxing and expect firmare
> to set pin muxing and pullup/down properties properly.
>
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
Even though it is not talking to the firmware about changing the state
of pins etc, can we expect it to do so at a later point? Some
comments in the code make me pretty sure that this is the case.
I think there may be a good incentive to put this driver into
drivers/pinctrl/* instead and implement parts of the pinctrl
interfaces.
Pls consult Documentation/pinctrl.txt.
> +/*
> + * Baytrail gpio controller consist of three separate sub-controllers called
> + * SCORE, NCORE and SUS. The sub-controllers are identified by their acpi UID.
> + *
> + * GPIO numbering is _not_ ordered meaning that gpio # 0 in ACPI namespace does
> + * _not_ correspond to the first gpio register at controller's gpio base.
> + * There is no logic or pattern in mapping gpio numbers to registers (pads) so
> + * each sub-controller needs to have its own mapping table
> + */
> +
> +static unsigned score_gpio_to_pad[BYT_NGPIO_SCORE] = {
> + 85, 89, 93, 96, 99, 102, 98, 101, 34, 37,
> + 36, 38, 39, 35, 40, 84, 62, 61, 64, 59,
> + 54, 56, 60, 55, 63, 57, 51, 50, 53, 47,
> + 52, 49, 48, 43, 46, 41, 45, 42, 58, 44,
> + 95, 105, 70, 68, 67, 66, 69, 71, 65, 72,
> + 86, 90, 88, 92, 103, 77, 79, 83, 78, 81,
> + 80, 82, 13, 12, 15, 14, 17, 18, 19, 16,
> + 2, 1, 0, 4, 6, 7, 9, 8, 33, 32,
> + 31, 30, 29, 27, 25, 28, 26, 23, 21, 20,
> + 24, 22, 5, 3, 10, 11, 106, 87, 91, 104,
> + 97, 100,
> +};
> +
> +static unsigned ncore_gpio_to_pad[BYT_NGPIO_NCORE] = {
> + 19, 18, 17, 20, 21, 22, 24, 25, 23, 16,
> + 14, 15, 12, 26, 27, 1, 4, 8, 11, 0,
> + 3, 6, 10, 13, 2, 5, 9, 7,
> +};
> +
> +static unsigned sus_gpio_to_pad[BYT_NGPIO_SUS] = {
> + 29, 33, 30, 31, 32, 34, 36, 35, 38, 37,
> + 18, 11, 20, 17, 1, 8, 10, 19, 12, 0,
> + 2, 23, 39, 28, 27, 22, 21, 24, 25, 26,
> + 51, 56, 54, 49, 55, 48, 57, 50, 58, 52,
> + 53, 59, 40,
> +};
This is duplicating the functionality of mapping pins to GPIO ranges
which already exist in the pinctrl subsystem.
gpiochip_add_pin_range(chip, "foo", offset, pin_base, N);
This can be reused by instatiating a shallow pinctrl driver which only
populates the .name, .pins, .owner, .get_groups_count(),
.get_group_name(), and . get_group_pins() of a struct pinctrl_desc
(we can discuss about making the group functions optional)
and then using the GPIO ranges to cross reference pins to
GPIOs.
Then you could use from <linux/pinctrl/pinctrl.h>
pinctrl_find_gpio_range_from_pin(struct pinctrl_dev *pctldev,
unsigned int pin);
And other helpers to cross-reference pins and GPIOs.
(...)
> +static void __iomem *byt_gpio_reg(struct gpio_chip *chip, unsigned offset,
> + int reg)
> +{
> + struct byt_gpio *vg = container_of(chip, struct byt_gpio, chip);
> + u32 reg_offset;
> + void __iomem *ptr;
> +
> + if (reg == BYT_INT_STAT_REG)
> + reg_offset = (offset / 32) * 4;
> + else
> + reg_offset = vg->gpio_to_pad[offset] * 16;
> + ptr = (void __iomem *) (vg->reg_base + reg_offset + reg);
> + return ptr;
> +}
> +
> +static int byt_gpio_request(struct gpio_chip *chip, unsigned offset)
> +{
> + struct byt_gpio *vg = container_of(chip, struct byt_gpio, chip);
> +
> +/* Policy about what should be done when requesting a gpio is unclear.
> + * In most cases PIN MUX 000 means gpio function, with the exception of SUS
> + * core pins 11-21 where gpio is mux 001.
This also looks suspicious. If it is "unclear" I am reading here:
"we will sooner or later have to deal with pin control somekindof".
> + * Some pins are set by bios to a non-gpio mux, but still marked as gpio
> + * resource in acpi tables, and they work just as they should when not touching
> + * the pin muxing. (For example mmc card detect switch)
> + *
> + * option 1, check pin mux is "gpio", else fail (FIXME gpio SUS pins 11-21):
> + * void __iomem *reg = byt_gpio_reg(chip, offset, BYT_CONF0_REG);
> + * u32 value;
> + * value = readl(reg) & BYT_PIN_MUX;
> + * if (value)
> + * return -EINVAL;
> + *
> + * option 2, force pin mux to gpio mode (FIXME gpio SUS pins 11-21):
> + * void __iomem *reg = byt_gpio_reg(chip, offset, BYT_CONF0_REG);
> + * u32 value;
> + * value = readl(reg) & BYT_PIN_MUX;
> + * writel(value & ~BYT_PIN_MUX, reg);
> + *
> + * option 3: don't touch the pinmuxing at all, let BIOS handle it
> + */
> + pm_runtime_get(&vg->pdev->dev);
> +
> + return 0;
> +}
If you ever want to do option 1 or 2, you should use pinctrl.
It also has (when implemented) the upside of exposing all these
gory details through debugfs for nice debugging, so developers
can see the state of all pads here.
I would recommend putting this in drivers/pinctrl, and
use a single probe() function, but call both gpiochip_add()
and pinctrl_register() then use gpiochip_add_pin_range()
to map pins to GPIOs. Compare e.g.
drivers/pinctrl/pinctrl-abx500.c
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] gpio: add Intel BayTrail gpio driver
2013-05-30 19:26 ` Linus Walleij
@ 2013-06-03 9:56 ` Mathias Nyman
0 siblings, 0 replies; 4+ messages in thread
From: Mathias Nyman @ 2013-06-03 9:56 UTC (permalink / raw)
To: Linus Walleij; +Cc: Grant Likely, linux-kernel
On 05/30/2013 10:26 PM, Linus Walleij wrote:
> On Wed, May 29, 2013 at 10:01 AM, Mathias Nyman
> <mathias.nyman@linux.intel.com> wrote:
>
>> Add support for gpio on Intel BayTrail platforms. BayTrail supports 3 banks
>> of gpios called SCORE, NCORE ans SUS with 102, 28 and 43 gpio pins.
>> Supports gpio interrupts
>>
>> Pins may be muxed to alternate function instead of gpio by firmware.
>> This driver does not touch the pin muxing and expect firmare
>> to set pin muxing and pullup/down properties properly.
>>
>> Signed-off-by: Mathias Nyman<mathias.nyman@linux.intel.com>
>
> Even though it is not talking to the firmware about changing the state
> of pins etc, can we expect it to do so at a later point? Some
> comments in the code make me pretty sure that this is the case.
>
> I think there may be a good incentive to put this driver into
> drivers/pinctrl/* instead and implement parts of the pinctrl
> interfaces.
>
> Pls consult Documentation/pinctrl.txt.
>
I'll have a look at pinctrl and see if it fits.
My understanding at the moment is that this driver shouldn't need to
modify pin muxing. BIOS is responsible for adding GPIO resource entries
to ACPI tables for devices that use gpios, and BIOS should also set all
muxings correctly for those pins(also pull-ups/downs etc.)
>> +/*
>> + * Baytrail gpio controller consist of three separate sub-controllers called
>> + * SCORE, NCORE and SUS. The sub-controllers are identified by their acpi UID.
>> + *
>> + * GPIO numbering is _not_ ordered meaning that gpio # 0 in ACPI namespace does
>> + * _not_ correspond to the first gpio register at controller's gpio base.
>> + * There is no logic or pattern in mapping gpio numbers to registers (pads) so
>> + * each sub-controller needs to have its own mapping table
>> + */
>> +
>> +static unsigned score_gpio_to_pad[BYT_NGPIO_SCORE] = {
>> + 85, 89, 93, 96, 99, 102, 98, 101, 34, 37,
>> + 36, 38, 39, 35, 40, 84, 62, 61, 64, 59,
>> + 54, 56, 60, 55, 63, 57, 51, 50, 53, 47,
>> + 52, 49, 48, 43, 46, 41, 45, 42, 58, 44,
>> + 95, 105, 70, 68, 67, 66, 69, 71, 65, 72,
>> + 86, 90, 88, 92, 103, 77, 79, 83, 78, 81,
>> + 80, 82, 13, 12, 15, 14, 17, 18, 19, 16,
>> + 2, 1, 0, 4, 6, 7, 9, 8, 33, 32,
>> + 31, 30, 29, 27, 25, 28, 26, 23, 21, 20,
>> + 24, 22, 5, 3, 10, 11, 106, 87, 91, 104,
>> + 97, 100,
>> +};
>> +
>> +static unsigned ncore_gpio_to_pad[BYT_NGPIO_NCORE] = {
>> + 19, 18, 17, 20, 21, 22, 24, 25, 23, 16,
>> + 14, 15, 12, 26, 27, 1, 4, 8, 11, 0,
>> + 3, 6, 10, 13, 2, 5, 9, 7,
>> +};
>> +
>> +static unsigned sus_gpio_to_pad[BYT_NGPIO_SUS] = {
>> + 29, 33, 30, 31, 32, 34, 36, 35, 38, 37,
>> + 18, 11, 20, 17, 1, 8, 10, 19, 12, 0,
>> + 2, 23, 39, 28, 27, 22, 21, 24, 25, 26,
>> + 51, 56, 54, 49, 55, 48, 57, 50, 58, 52,
>> + 53, 59, 40,
>> +};
>
> This is duplicating the functionality of mapping pins to GPIO ranges
> which already exist in the pinctrl subsystem.
>
> gpiochip_add_pin_range(chip, "foo", offset, pin_base, N);
>
> This can be reused by instatiating a shallow pinctrl driver which only
> populates the .name, .pins, .owner, .get_groups_count(),
> .get_group_name(), and . get_group_pins() of a struct pinctrl_desc
> (we can discuss about making the group functions optional)
> and then using the GPIO ranges to cross reference pins to
> GPIOs.
>
> Then you could use from<linux/pinctrl/pinctrl.h>
> pinctrl_find_gpio_range_from_pin(struct pinctrl_dev *pctldev,
> unsigned int pin);
>
> And other helpers to cross-reference pins and GPIOs.
>
> (...)
The pin-to-gpio mapping is quite random here, there are no long
consecutive ranges, I tried to find some pattern but it seems to be
just random.
I haven't yet looked into pinctrl yet and see what it offers
>> +static void __iomem *byt_gpio_reg(struct gpio_chip *chip, unsigned offset,
>> + int reg)
>> +{
>> + struct byt_gpio *vg = container_of(chip, struct byt_gpio, chip);
>> + u32 reg_offset;
>> + void __iomem *ptr;
>> +
>> + if (reg == BYT_INT_STAT_REG)
>> + reg_offset = (offset / 32) * 4;
>> + else
>> + reg_offset = vg->gpio_to_pad[offset] * 16;
>> + ptr = (void __iomem *) (vg->reg_base + reg_offset + reg);
>> + return ptr;
>> +}
>> +
>> +static int byt_gpio_request(struct gpio_chip *chip, unsigned offset)
>> +{
>> + struct byt_gpio *vg = container_of(chip, struct byt_gpio, chip);
>> +
>
>
>> +/* Policy about what should be done when requesting a gpio is unclear.
>> + * In most cases PIN MUX 000 means gpio function, with the exception of SUS
>> + * core pins 11-21 where gpio is mux 001.
>
> This also looks suspicious. If it is "unclear" I am reading here:
> "we will sooner or later have to deal with pin control somekindof".
>
It should evolve in the other direction with BIOS configuring the pins,
but while writing the driver the BIOSes were not mature enough and could
not be trusted.
So I think I'll either remove the muxing options and keep it as gpio
driver, or then if it turns out we need to do do pinmuxing I'll use pinctrl.
Thanks for reviewing and for the pinctrl explanations
-Mathias
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-06-03 9:53 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-05-29 8:01 [PATCH] gpio: add Intel BayTrail gpio driver Mathias Nyman
2013-05-29 19:29 ` Andy Shevchenko
2013-05-30 19:26 ` Linus Walleij
2013-06-03 9:56 ` Mathias Nyman
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.