[PATCH v2 0/1] gpio driver for Intel Baytrail platforms

[PATCH v2 0/1] gpio driver for Intel Baytrail platforms

[Mathias Nyman]

This is the second version of the Intel baytrail GPIO driver.

After looking at the pinctrl subsystem that Linus W. suggested I think 
pinctrl suits platforms that don't have firmware configuring the pins
before the operating system is started, or when pins need to be configured
on the fly.

I'd still keep this driver under GPIO. Adding it to pinctrl
feels like adding more complexity without any bigger use for the features.

We expect BIOS to set all pin configurations correctly. 
The comments about pin muxing capabilities are removed from the driver.
The same firmware is anyway listing gpio resources in ACPI tables, so pin 
configurations should be correct. (The previous indication in the driver
about the need to configure pins was mostly because we're working with early 
develpment stage firmwares which still have small hickups)  

Changes since v1:
- generic cleanups suggested by Andy S.
- removed text about pin muxing
- added missing 44:th pin to SUS controller
- added level triggering option
- prevent "forever loop" in case pin is stuck in "interrupt triggered" status,     

Mathias Nyman (1):
  gpio: add Intel BayTrail gpio driver

 drivers/gpio/Kconfig         |   12 +
 drivers/gpio/Makefile        |    1 +
 drivers/gpio/gpio-baytrail.c |  547 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 560 insertions(+), 0 deletions(-)
 create mode 100644 drivers/gpio/gpio-baytrail.c

-- 
1.7.4.1

[PATCH v2 1/1] gpio: add Intel BayTrail gpio driver

[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 44 gpio pins.
Supports gpio interrupts and ACPI gpio events.

Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/gpio/Kconfig         |   12 +
 drivers/gpio/Makefile        |    1 +
 drivers/gpio/gpio-baytrail.c |  547 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 560 insertions(+), 0 deletions(-)
 create mode 100644 drivers/gpio/gpio-baytrail.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 573c449..b712006 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 44 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..55efbdc
--- /dev/null
+++ b/drivers/gpio/gpio-baytrail.c
@@ -0,0 +1,547 @@
+/*
+ * 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		44
+
+/*
+ * 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, 7, 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);
+
+	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);
+	void __iomem *reg = byt_gpio_reg(&vg->chip, offset, BYT_CONF0_REG);
+	u32 value;
+
+	/* clear interrupt triggering */
+	value = readl(reg);
+	value &= ~(BYT_TRIG_POS | BYT_TRIG_NEG | BYT_TRIG_LVL);
+	writel(value, reg);
+
+	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);
+
+	/* For level trigges the BYT_TRIG_POS and BYT_TRIG_NEG bits
+	 * are used to indicate high and low level triggering
+	 */
+	value &= ~(BYT_TRIG_POS | BYT_TRIG_NEG | BYT_TRIG_LVL);
+
+	switch (type) {
+	case IRQ_TYPE_LEVEL_HIGH:
+		value |= BYT_TRIG_LVL;
+	case IRQ_TYPE_EDGE_RISING:
+		value |= BYT_TRIG_POS;
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+		value |= BYT_TRIG_LVL;
+	case IRQ_TYPE_EDGE_FALLING:
+		value |= BYT_TRIG_NEG;
+		break;
+	case IRQ_TYPE_EDGE_BOTH:
+		value |= (BYT_TRIG_NEG | BYT_TRIG_POS);
+		break;
+	}
+	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;
+	int looplimit = 0;
+
+	/* 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);
+
+			/* In case bios or user sets triggering incorretly a pin
+			 * might remain in "interrupt triggered" state.
+			 */
+			if (looplimit++ > 32) {
+				dev_err(&vg->pdev->dev,
+					"Gpio %d interrupt flood, disabling\n",
+					base + pin);
+
+				reg = byt_gpio_reg(&vg->chip, base + pin,
+						   BYT_CONF0_REG);
+				mask = readl(reg);
+				mask &= ~(BYT_TRIG_NEG | BYT_TRIG_POS |
+					  BYT_TRIG_LVL);
+				writel(mask, reg);
+				mask = readl(reg); /* flush */
+				break;
+			}
+		}
+	}
+	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, value;
+
+	/* 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);
+		/* make sure trigger bits are cleared, if not then a pin
+		   might be misconfigured in bios */
+		value = readl(reg);
+		if (value)
+			dev_err(&vg->pdev->dev,
+				"GPIO interrupt flood, pins misconfigured\n");
+	}
+}
+
+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);
+	vg->reg_base = devm_ioremap_resource(dev, mem_rc);
+	if (IS_ERR(vg->reg_base))
+		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  */
+	irq_rc = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	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");
+
+	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

Re: [PATCH v2 0/1] gpio driver for Intel Baytrail platforms

[Linus Walleij]

On Thu, Jun 13, 2013 at 5:06 PM, Mathias Nyman
<mathias.nyman@linux.intel.com> wrote:

> After looking at the pinctrl subsystem that Linus W. suggested I think
> pinctrl suits platforms that don't have firmware configuring the pins
> before the operating system is started, or when pins need to be configured
> on the fly.
>
> I'd still keep this driver under GPIO. Adding it to pinctrl
> feels like adding more complexity without any bigger use for the features.
>
> We expect BIOS to set all pin configurations correctly.
> The comments about pin muxing capabilities are removed from the driver.
> The same firmware is anyway listing gpio resources in ACPI tables, so pin
> configurations should be correct. (The previous indication in the driver
> about the need to configure pins was mostly because we're working with early
> develpment stage firmwares which still have small hickups)

This does not address the issue that you're reimplementing
the GPIO ranges from the pinctrl subsystem, and just hours ago
on the mailing list Christian Ruppert sent a patch making these
more flexible I think.

Subject "Add pin-list based GPIO ranges", please check this
patch, isn't that exactly the helper infrastructure you need?

Of course you can make an argument that is is a good idea to
duplicate this, but I want that to be explicit. To me it is still
quite obvious that these gpio to pad mappings are laid out
according to the actual hardware registers, and that the actual
hardware registers pertain to pads rather than what we call
"GPIOs", which in kernel terms are only some line.

I would still vote to put the thing in drivers/pinctrl anyway,
I am perfectly happy to house pure GPIO drivers there,
especially if they're obviously masking something more
pinctrl-like in reality, it will be way more flexible the day that
you just want to add "this one little quirk for this pin config
thing", then it'll fit just fine.

Yours,
Linus Walleij

Re: [PATCH v2 0/1] gpio driver for Intel Baytrail platforms

[Mathias Nyman]

On 06/13/2013 06:45 PM, Linus Walleij wrote:
> On Thu, Jun 13, 2013 at 5:06 PM, Mathias Nyman
> <mathias.nyman@linux.intel.com>  wrote:
>
>> After looking at the pinctrl subsystem that Linus W. suggested I think
>> pinctrl suits platforms that don't have firmware configuring the pins
>> before the operating system is started, or when pins need to be configured
>> on the fly.
>>
>> I'd still keep this driver under GPIO. Adding it to pinctrl
>> feels like adding more complexity without any bigger use for the features.
>>
>> We expect BIOS to set all pin configurations correctly.
>> The comments about pin muxing capabilities are removed from the driver.
>> The same firmware is anyway listing gpio resources in ACPI tables, so pin
>> configurations should be correct. (The previous indication in the driver
>> about the need to configure pins was mostly because we're working with early
>> develpment stage firmwares which still have small hickups)
>
> This does not address the issue that you're reimplementing
> the GPIO ranges from the pinctrl subsystem, and just hours ago
> on the mailing list Christian Ruppert sent a patch making these
> more flexible I think.
>
> Subject "Add pin-list based GPIO ranges", please check this
> patch, isn't that exactly the helper infrastructure you need?

It fits better yes, with that patch I could use
struct pinctrl_gpio_range instead of the custom struct gpio_bank.
The .name entry can be used for acpi_id to identify the range.

Also the gpio_to_pad() function is usable.

>
> Of course you can make an argument that is is a good idea to
> duplicate this, but I want that to be explicit. To me it is still
> quite obvious that these gpio to pad mappings are laid out
> according to the actual hardware registers, and that the actual
> hardware registers pertain to pads rather than what we call
> "GPIOs", which in kernel terms are only some line.
>

That is true, it's about mapping between layout of hw registers.
I guess it's a tradeoff between more

> I would still vote to put the thing in drivers/pinctrl anyway,
> I am perfectly happy to house pure GPIO drivers there,
> especially if they're obviously masking something more
> pinctrl-like in reality, it will be way more flexible the day that
> you just want to add "this one little quirk for this pin config
> thing", then it'll fit just fine.
>

I'm fine with having it under drivers/pinctrl as a GPIO driver, either 
just as it is, or by using the pinctrl_gpio_range structure and helper 
functions such as gpio_to_pad(), once Christian Rupperts patch is accepted.

any naming preference?
gpio-baytrail.c
pinctrl-gpio-baytrail.c
pinctrl-baytrail.c

-Mathias

Re: [PATCH v2 0/1] gpio driver for Intel Baytrail platforms

[Linus Walleij]

On Fri, Jun 14, 2013 at 2:14 PM, Mathias Nyman
<mathias.nyman@linux.intel.com> wrote:
> [Me]
>> I would still vote to put the thing in drivers/pinctrl anyway,
>> I am perfectly happy to house pure GPIO drivers there,
>> especially if they're obviously masking something more
>> pinctrl-like in reality, it will be way more flexible the day that
>> you just want to add "this one little quirk for this pin config
>> thing", then it'll fit just fine.
>>
>
> I'm fine with having it under drivers/pinctrl as a GPIO driver, either just
> as it is, or by using the pinctrl_gpio_range structure and helper functions
> such as gpio_to_pad(), once Christian Rupperts patch is accepted.

Christian's patch is merged into the pinctrl tree for v3.11.

> any naming preference?

> pinctrl-baytrail.c

Bingo!

Because the hardware is a pin controller after all.

I know there is some BIOS or ROM very much wanting it to look
simpler than it is, but it sure is a pin controller :-)

Thanks,
Linus Walleij