[PATCH 0/1] gpio: add Lynxpoint chipset gpio driver

[PATCH 0/1] gpio: add Lynxpoint chipset gpio driver

[Mathias Nyman]

This gpio controller driver for the Intel Lynxpoint LP PCH chipset is
designed to work with the new ACPI 5 enumeration model and ACPI 5 GpioIo/GpioInt 
resource translation recently added to linux-acpi.

Mathias Nyman (1):
  gpio: add Lynxpoint chipset gpio driver.

 drivers/gpio/Kconfig          |    7 +
 drivers/gpio/Makefile         |    1 +
 drivers/gpio/gpio-lynxpoint.c |  406 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 414 insertions(+), 0 deletions(-)
 create mode 100644 drivers/gpio/gpio-lynxpoint.c

-- 
1.7.4.1

[PATCH 1/1] gpio: add Lynxpoint chipset gpio driver.

[Mathias Nyman]

Add gpio support for Intel Lynxpoint chipset.
Lynxpoint supports 94 gpio pins which can generate interrupts.
Driver will fail requests for pins that are marked as owned by ACPI, or
set in an alternate mode (non-gpio).

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

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index f165576..e144bfa 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -267,6 +267,13 @@ config GPIO_GE_FPGA
 	  and write pin state) for GPIO implemented in a number of GE single
 	  board computers.
 
+config GPIO_LYNXPOINT
+	bool "Intel Lynxpoint GPIO support"
+	select IRQ_DOMAIN
+	help
+	  driver for GPIO functionality on Intel Lynxpoint PCH chipset
+	  Requires platform or ACPI code to set up a platform device.
+
 comment "I2C GPIO expanders:"
 
 config GPIO_ARIZONA
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 420dbac..e4af917 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_GPIO_JANZ_TTL)	+= gpio-janz-ttl.o
 obj-$(CONFIG_ARCH_KS8695)	+= gpio-ks8695.o
 obj-$(CONFIG_GPIO_LANGWELL)	+= gpio-langwell.o
 obj-$(CONFIG_ARCH_LPC32XX)	+= gpio-lpc32xx.o
+obj-$(CONFIG_GPIO_LYNXPOINT)	+= gpio-lynxpoint.o
 obj-$(CONFIG_GPIO_MAX730X)	+= gpio-max730x.o
 obj-$(CONFIG_GPIO_MAX7300)	+= gpio-max7300.o
 obj-$(CONFIG_GPIO_MAX7301)	+= gpio-max7301.o
diff --git a/drivers/gpio/gpio-lynxpoint.c b/drivers/gpio/gpio-lynxpoint.c
new file mode 100644
index 0000000..6c7f351
--- /dev/null
+++ b/drivers/gpio/gpio-lynxpoint.c
@@ -0,0 +1,406 @@
+/*
+ * GPIO controller driver for Intel Lynxpoint PCH chipset>
+ * Copyright (c) 2012, 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/slab.h>
+#include <linux/acpi.h>
+#include <linux/platform_device.h>
+
+/* LynxPoint chipset has support for 94 gpio pins */
+
+#define LP_NUM_GPIO	94
+
+/* Register offsets */
+#define LP_ACPI_OWNED	0x00 /* Bitmap, set by bios, 0: pin reserved for ACPI */
+#define LP_GC		0x7C /* set APIC IRQ to IRQ14 or IRQ15 for all pins */
+#define LP_INT_STAT	0x80
+#define LP_INT_ENABLE	0x90
+
+/* Each pin has two 32 bit config registers, starting at 0x100 */
+#define LP_CONFIG1	0x100
+#define LP_CONFIG2	0x104
+
+/* LP_CONFIG1 reg bits */
+#define OUT_LVL_BIT	BIT(31)
+#define IN_LVL_BIT	BIT(30)
+#define TRIG_SEL_BIT	BIT(4) /* 0: Edge, 1: Level */
+#define INT_INV_BIT	BIT(3) /* Invert interrupt triggering */
+#define DIR_BIT		BIT(2) /* 0: Output, 1: Input */
+#define USE_SEL_BIT	BIT(0) /* 0: Native, 1: GPIO */
+
+/* LP_CONFIG2 reg bits */
+#define GPINDIS_BIT	BIT(2) /* disable input sensing */
+#define GPIWP_BIT	(BIT(0) | BIT(1)) /* weak pull options */
+
+struct lp_gpio {
+	struct gpio_chip	chip;
+	struct irq_domain	*domain;
+	struct platform_device	*pdev;
+	spinlock_t		lock;
+	unsigned		hwirq;
+	unsigned long		reg_base;
+	unsigned long		reg_len;
+};
+
+static unsigned long gpio_reg(struct gpio_chip *chip, unsigned gpio, int reg)
+{
+	struct lp_gpio *lg = container_of(chip, struct lp_gpio, chip);
+	int offset;
+
+	if (reg == LP_CONFIG1 || reg == LP_CONFIG2)
+		offset = gpio * 8;
+	else
+		offset = (gpio / 32) * 4;
+
+	return lg->reg_base + reg + offset;
+}
+
+static int lp_gpio_request(struct gpio_chip *chip, unsigned gpio)
+{
+	struct lp_gpio *lg = container_of(chip, struct lp_gpio, chip);
+	unsigned long reg = gpio_reg(chip, gpio, LP_CONFIG1);
+	unsigned long conf2 = gpio_reg(chip, gpio, LP_CONFIG2);
+	unsigned long acpi_use = gpio_reg(chip, gpio, LP_ACPI_OWNED);
+
+	/* Fail if BIOS reserved pin for ACPI use */
+	if (!(inl(acpi_use) & BIT(gpio % 32))) {
+		dev_err(&lg->pdev->dev, "gpio %d reserved for ACPI\n", gpio);
+		return -EBUSY;
+	}
+	/* Fail if pin is in alternate function mode (not GPIO mode) */
+	if (!(inl(reg) & USE_SEL_BIT))
+		return -ENODEV;
+
+	/* enable input sensing */
+	outl(inl(conf2) & ~GPINDIS_BIT, conf2);
+
+	return 0;
+}
+
+static int lp_irq_type(struct irq_data *d, unsigned type)
+{
+	struct lp_gpio *lg = irq_data_get_irq_chip_data(d);
+	u32 gpio = irqd_to_hwirq(d);
+	unsigned long flags;
+	u32 value;
+	unsigned long reg = gpio_reg(&lg->chip, gpio, LP_CONFIG1);
+
+	if (gpio >= lg->chip.ngpio)
+		return -EINVAL;
+
+	spin_lock_irqsave(&lg->lock, flags);
+	value = inl(reg);
+
+	/* set both TRIG_SEL and INV bits to 0 for rising edge */
+	if (type & IRQ_TYPE_EDGE_RISING)
+		value &= ~(TRIG_SEL_BIT | INT_INV_BIT);
+
+	/* TRIG_SEL bit 0, INV bit 1 for falling edge */
+	if (type & IRQ_TYPE_EDGE_FALLING)
+		value = (value | INT_INV_BIT) & ~TRIG_SEL_BIT;
+
+	/* TRIG_SEL bit 1, INV bit 0 for level low */
+	if (type & IRQ_TYPE_LEVEL_LOW)
+		value = (value | TRIG_SEL_BIT) & ~INT_INV_BIT;
+
+	/* TRIG_SEL bit 1, INV bit 1 for level high */
+	if (type & IRQ_TYPE_LEVEL_HIGH)
+		value |= TRIG_SEL_BIT | INT_INV_BIT;
+
+	outl(value, reg);
+	spin_unlock_irqrestore(&lg->lock, flags);
+
+	return 0;
+}
+
+static int lp_gpio_get(struct gpio_chip *chip, unsigned gpio)
+{
+	unsigned long reg = gpio_reg(chip, gpio, LP_CONFIG1);
+	return inl(reg) & IN_LVL_BIT;
+}
+
+static void lp_gpio_set(struct gpio_chip *chip, unsigned gpio, int value)
+{
+	struct lp_gpio *lg = container_of(chip, struct lp_gpio, chip);
+	unsigned long reg = gpio_reg(chip, gpio, LP_CONFIG1);
+	unsigned long flags;
+
+	spin_lock_irqsave(&lg->lock, flags);
+
+	if (value)
+		outl(inl(reg) | OUT_LVL_BIT, reg);
+	else
+		outl(inl(reg) & ~OUT_LVL_BIT, reg);
+
+	spin_unlock_irqrestore(&lg->lock, flags);
+}
+
+static int lp_gpio_direction_input(struct gpio_chip *chip, unsigned gpio)
+{
+	struct lp_gpio *lg = container_of(chip, struct lp_gpio, chip);
+	unsigned long reg = gpio_reg(chip, gpio, LP_CONFIG1);
+	unsigned long flags;
+
+	spin_lock_irqsave(&lg->lock, flags);
+	outl(inl(reg) | DIR_BIT, reg);
+	spin_unlock_irqrestore(&lg->lock, flags);
+
+	return 0;
+}
+
+static int lp_gpio_direction_output(struct gpio_chip *chip,
+				      unsigned gpio, int value)
+{
+	struct lp_gpio *lg = container_of(chip, struct lp_gpio, chip);
+	unsigned long reg = gpio_reg(chip, gpio, LP_CONFIG1);
+	unsigned long flags;
+
+	lp_gpio_set(chip, gpio, value);
+
+	spin_lock_irqsave(&lg->lock, flags);
+	outl(inl(reg) & ~DIR_BIT, reg);
+	spin_unlock_irqrestore(&lg->lock, flags);
+
+	return 0;
+}
+
+static int lp_gpio_to_irq(struct gpio_chip *chip, unsigned gpio)
+{
+	struct lp_gpio *lg = container_of(chip, struct lp_gpio, chip);
+	return irq_create_mapping(lg->domain, gpio);
+}
+
+static void lp_gpio_irq_handler(unsigned irq, struct irq_desc *desc)
+{
+	struct irq_data *data = irq_desc_get_irq_data(desc);
+	struct lp_gpio *lg = irq_data_get_irq_handler_data(data);
+	struct irq_chip *chip = irq_data_get_irq_chip(data);
+	u32 base, pin, mask;
+	unsigned long reg, pending;
+	unsigned virq;
+
+	/* check from GPIO controller which pin triggered the interrupt */
+	for (base = 0; base < lg->chip.ngpio; base += 32) {
+		reg = gpio_reg(&lg->chip, base, LP_INT_STAT);
+
+		while ((pending = inl(reg))) {
+			pin = __ffs(pending);
+			mask = BIT(pin);
+			/* Clear before handling so we don't lose an edge */
+			outl(mask, reg);
+			virq = irq_find_mapping(lg->domain, base + pin);
+			generic_handle_irq(virq);
+		}
+	}
+	chip->irq_eoi(data);
+}
+
+static void lp_irq_unmask(struct irq_data *d)
+{
+}
+
+static void lp_irq_mask(struct irq_data *d)
+{
+}
+
+static void lp_irq_enable(struct irq_data *d)
+{
+	struct lp_gpio *lg = irq_data_get_irq_chip_data(d);
+	u32 gpio = irqd_to_hwirq(d);
+	unsigned long reg = gpio_reg(&lg->chip, gpio, LP_INT_ENABLE);
+	unsigned long flags;
+
+	spin_lock_irqsave(&lg->lock, flags);
+	outl(inl(reg) | BIT(gpio % 32), reg);
+	spin_unlock_irqrestore(&lg->lock, flags);
+}
+
+static void lp_irq_disable(struct irq_data *d)
+{
+	struct lp_gpio *lg = irq_data_get_irq_chip_data(d);
+	u32 gpio = irqd_to_hwirq(d);
+	unsigned long reg = gpio_reg(&lg->chip, gpio, LP_INT_ENABLE);
+	unsigned long flags;
+
+	spin_lock_irqsave(&lg->lock, flags);
+	outl(inl(reg) & ~BIT(gpio % 32), reg);
+	spin_unlock_irqrestore(&lg->lock, flags);
+}
+
+static struct irq_chip lp_irqchip = {
+	.name = "LP-GPIO",
+	.irq_mask = lp_irq_mask,
+	.irq_unmask = lp_irq_unmask,
+	.irq_enable = lp_irq_enable,
+	.irq_disable = lp_irq_disable,
+	.irq_set_type = lp_irq_type,
+	.flags = IRQCHIP_SKIP_SET_WAKE,
+};
+
+static void lp_gpio_irq_init_hw(struct lp_gpio *lg)
+{
+	unsigned long reg;
+	unsigned base;
+
+	for (base = 0; base < lg->chip.ngpio; base += 32) {
+		/* disable gpio pin interrupts */
+		reg = gpio_reg(&lg->chip, base, LP_INT_ENABLE);
+		outl(0, reg);
+		/* Clear interrupt status register */
+		reg = gpio_reg(&lg->chip, base, LP_INT_STAT);
+		outl(0xffffffff, reg);
+	}
+}
+
+static int lp_gpio_irq_map(struct irq_domain *d, unsigned int virq,
+			    irq_hw_number_t hw)
+{
+	struct lp_gpio *lg = d->host_data;
+
+	irq_set_chip_and_handler_name(virq, &lp_irqchip, handle_simple_irq,
+				      "demux");
+	irq_set_chip_data(virq, lg);
+	irq_set_irq_type(virq, IRQ_TYPE_NONE);
+
+	return 0;
+}
+
+static const struct irq_domain_ops lp_gpio_irq_ops = {
+	.map = lp_gpio_irq_map,
+};
+
+static int __devinit lp_gpio_probe(struct platform_device *pdev)
+{
+	struct lp_gpio *lg;
+	struct gpio_chip *gc;
+	struct resource *io_rc, *irq_rc;
+	struct device *dev = &pdev->dev;
+	int ret = -ENODEV;
+
+	lg = devm_kzalloc(dev, sizeof(struct lp_gpio), GFP_KERNEL);
+	if (!lg) {
+		dev_err(dev, "can't allocate lp_gpio chip data\n");
+		return -ENOMEM;
+	}
+
+	lg->pdev = pdev;
+	platform_set_drvdata(pdev, lg);
+
+	io_rc = platform_get_resource(pdev, IORESOURCE_IO, 0);
+	irq_rc = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+
+	if (!io_rc) {
+		dev_err(dev, "missing IO resources\n");
+		return -EINVAL;
+	}
+
+	lg->reg_base = io_rc->start;
+	lg->reg_len = resource_size(io_rc);
+
+	if (!devm_request_region(dev, lg->reg_base, lg->reg_len, "lp-gpio")) {
+		dev_err(dev, "failed requesting IO region 0x%x\n",
+			(unsigned int)lg->reg_base);
+		return -EBUSY;
+	}
+
+	spin_lock_init(&lg->lock);
+
+	gc = &lg->chip;
+	gc->label = dev_name(dev);
+	gc->owner = THIS_MODULE;
+	gc->request = lp_gpio_request;
+	gc->direction_input = lp_gpio_direction_input;
+	gc->direction_output = lp_gpio_direction_output;
+	gc->get = lp_gpio_get;
+	gc->set = lp_gpio_set;
+	gc->base = -1;
+	gc->ngpio = LP_NUM_GPIO;
+	gc->can_sleep = 0;
+	gc->dev = dev;
+
+	/* set up interrupts  */
+	if (irq_rc && irq_rc->start) {
+		lg->hwirq = irq_rc->start;
+		gc->to_irq = lp_gpio_to_irq;
+
+		lg->domain = irq_domain_add_linear(NULL, LP_NUM_GPIO,
+						   &lp_gpio_irq_ops, lg);
+		if (!lg->domain)
+			return -ENXIO;
+
+		lp_gpio_irq_init_hw(lg);
+
+		irq_set_handler_data(lg->hwirq, lg);
+		irq_set_chained_handler(lg->hwirq, lp_gpio_irq_handler);
+	}
+
+	ret = gpiochip_add(gc);
+	if (ret) {
+		dev_err(dev, "failed adding lp-gpio chip\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id lynxpoint_gpio_acpi_match[] = {
+	{ "INT33C7", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, lynxpoint_gpio_acpi_match);
+#endif
+
+static int __devexit lp_gpio_remove(struct platform_device *pdev)
+{
+	struct lp_gpio *lg = platform_get_drvdata(pdev);
+	int err;
+	err = gpiochip_remove(&lg->chip);
+	if (err)
+		dev_warn(&pdev->dev, "failed to remove gpio_chip.\n");
+	platform_set_drvdata(pdev, NULL);
+	return 0;
+}
+
+static struct platform_driver lp_gpio_driver = {
+	.probe          = lp_gpio_probe,
+	.remove         = __devexit_p(lp_gpio_remove),
+	.driver         = {
+		.name   = "lp_gpio",
+		.owner  = THIS_MODULE,
+		.acpi_match_table = ACPI_PTR(lynxpoint_gpio_acpi_match),
+	},
+};
+
+static int __init lp_gpio_init(void)
+{
+	return platform_driver_register(&lp_gpio_driver);
+}
+
+subsys_initcall(lp_gpio_init);
-- 
1.7.4.1

Re: [PATCH 1/1] gpio: add Lynxpoint chipset gpio driver.

[Linus Walleij]

On Fri, Dec 7, 2012 at 3:01 PM, Mathias Nyman
<mathias.nyman@linux.intel.com> wrote:

> Add gpio support for Intel Lynxpoint chipset.
> Lynxpoint supports 94 gpio pins which can generate interrupts.
> Driver will fail requests for pins that are marked as owned by ACPI, or
> set in an alternate mode (non-gpio).
>
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>

Nice shape on this patch, makes it easy to focus the review on the
important stuff, thanks Magnus!

> +config GPIO_LYNXPOINT
> +       bool "Intel Lynxpoint GPIO support"

So you don't want to be able to build it as module?
(OK just odd on Intel systems.)

> +       select IRQ_DOMAIN
> +       help
> +         driver for GPIO functionality on Intel Lynxpoint PCH chipset
> +         Requires platform or ACPI code to set up a platform device.

(...)
> +++ b/drivers/gpio/gpio-lynxpoint.c
(...)
> +/* LynxPoint chipset has support for 94 gpio pins */
> +
> +#define LP_NUM_GPIO    94
> +
> +/* Register offsets */
> +#define LP_ACPI_OWNED  0x00 /* Bitmap, set by bios, 0: pin reserved for ACPI */
> +#define LP_GC          0x7C /* set APIC IRQ to IRQ14 or IRQ15 for all pins */
> +#define LP_INT_STAT    0x80
> +#define LP_INT_ENABLE  0x90
> +
> +/* Each pin has two 32 bit config registers, starting at 0x100 */
> +#define LP_CONFIG1     0x100
> +#define LP_CONFIG2     0x104
> +
> +/* LP_CONFIG1 reg bits */
> +#define OUT_LVL_BIT    BIT(31)
> +#define IN_LVL_BIT     BIT(30)
> +#define TRIG_SEL_BIT   BIT(4) /* 0: Edge, 1: Level */
> +#define INT_INV_BIT    BIT(3) /* Invert interrupt triggering */
> +#define DIR_BIT                BIT(2) /* 0: Output, 1: Input */
> +#define USE_SEL_BIT    BIT(0) /* 0: Native, 1: GPIO */
> +
> +/* LP_CONFIG2 reg bits */
> +#define GPINDIS_BIT    BIT(2) /* disable input sensing */
> +#define GPIWP_BIT      (BIT(0) | BIT(1)) /* weak pull options */

Kudos for using BIT() macros, very readable, thanks!

> +struct lp_gpio {
> +       struct gpio_chip        chip;
> +       struct irq_domain       *domain;
> +       struct platform_device  *pdev;
> +       spinlock_t              lock;
> +       unsigned                hwirq;

This struct member is only used in probe() so make it a local variable there
and cut it from the struct.

> +       unsigned long           reg_base;
> +       unsigned long           reg_len;

Same comment for reg_len.

> +};
> +
> +static unsigned long gpio_reg(struct gpio_chip *chip, unsigned gpio, int reg)

Rename lp_gpio_reg() so as to match the family.

Rename the argument "gpio" to "offset" since it's a local number,
not in the global GPIO numberspace. (Please change this everywhere
a local index is used.)

> +{
> +       struct lp_gpio *lg = container_of(chip, struct lp_gpio, chip);
> +       int offset;
> +
> +       if (reg == LP_CONFIG1 || reg == LP_CONFIG2)
> +               offset = gpio * 8;
> +       else
> +               offset = (gpio / 32) * 4;

Put in some comment above this explaining the layout of the offsets
for these two cases so we understand what's happening here.

> +       return lg->reg_base + reg + offset;
> +}
> +
> +static int lp_gpio_request(struct gpio_chip *chip, unsigned gpio)
> +{
> +       struct lp_gpio *lg = container_of(chip, struct lp_gpio, chip);
> +       unsigned long reg = gpio_reg(chip, gpio, LP_CONFIG1);
> +       unsigned long conf2 = gpio_reg(chip, gpio, LP_CONFIG2);
> +       unsigned long acpi_use = gpio_reg(chip, gpio, LP_ACPI_OWNED);
> +
> +       /* Fail if BIOS reserved pin for ACPI use */
> +       if (!(inl(acpi_use) & BIT(gpio % 32))) {
> +               dev_err(&lg->pdev->dev, "gpio %d reserved for ACPI\n", gpio);
> +               return -EBUSY;
> +       }

This %32 magic only seems to consider the latter part of the if()
statement in the gpio_reg() function. It's like you assume only the
(gpio / 32) * 4 path to be taken. It seems that for the two models
handled there this should be %8 or something. Or I'm getting it
wrong, which is an indication that something is pretty unclear here...

> +       /* Fail if pin is in alternate function mode (not GPIO mode) */
> +       if (!(inl(reg) & USE_SEL_BIT))
> +               return -ENODEV;
> +
> +       /* enable input sensing */
> +       outl(inl(conf2) & ~GPINDIS_BIT, conf2);
> +
> +       return 0;
> +}

(...)
> +static void lp_irq_enable(struct irq_data *d)
> +{
> +       struct lp_gpio *lg = irq_data_get_irq_chip_data(d);
> +       u32 gpio = irqd_to_hwirq(d);

That variable is confusingly named. It's not a global gpio number,
it's a local offset, so please rename it "offset".

> +       unsigned long reg = gpio_reg(&lg->chip, gpio, LP_INT_ENABLE);
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&lg->lock, flags);
> +       outl(inl(reg) | BIT(gpio % 32), reg);
> +       spin_unlock_irqrestore(&lg->lock, flags);
> +}
> +
> +static void lp_irq_disable(struct irq_data *d)
> +{
> +       struct lp_gpio *lg = irq_data_get_irq_chip_data(d);
> +       u32 gpio = irqd_to_hwirq(d);

Dito.

> +       unsigned long reg = gpio_reg(&lg->chip, gpio, LP_INT_ENABLE);
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&lg->lock, flags);
> +       outl(inl(reg) & ~BIT(gpio % 32), reg);
> +       spin_unlock_irqrestore(&lg->lock, flags);
> +}
(...)
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id lynxpoint_gpio_acpi_match[] = {
> +       { "INT33C7", 0 },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(acpi, lynxpoint_gpio_acpi_match);
> +#endif

Aha so how many users are there of this driver which are not
using ACPI?

If zero, why not just depend on ACPI in Kconfig?

> +static int __devexit lp_gpio_remove(struct platform_device *pdev)

All __devinit/__devexit macros are going away in v3.8 so delete them
everywhere.

> +{
> +       struct lp_gpio *lg = platform_get_drvdata(pdev);
> +       int err;
> +       err = gpiochip_remove(&lg->chip);
> +       if (err)
> +               dev_warn(&pdev->dev, "failed to remove gpio_chip.\n");
> +       platform_set_drvdata(pdev, NULL);
> +       return 0;
> +}
> +
> +static struct platform_driver lp_gpio_driver = {
> +       .probe          = lp_gpio_probe,
> +       .remove         = __devexit_p(lp_gpio_remove),

Delete that __devexit_p() too I suppose.

> +       .driver         = {
> +               .name   = "lp_gpio",
> +               .owner  = THIS_MODULE,
> +               .acpi_match_table = ACPI_PTR(lynxpoint_gpio_acpi_match),
> +       },
> +};
> +
> +static int __init lp_gpio_init(void)
> +{
> +       return platform_driver_register(&lp_gpio_driver);
> +}
> +
> +subsys_initcall(lp_gpio_init);

Yours,
Linus Walleij

Re: [PATCH 1/1] gpio: add Lynxpoint chipset gpio driver.

[Mathias Nyman]

On 12/10/2012 11:48 AM, Linus Walleij wrote:
> On Fri, Dec 7, 2012 at 3:01 PM, Mathias Nyman
> <mathias.nyman@linux.intel.com>  wrote:
>
>> Add gpio support for Intel Lynxpoint chipset.
>> Lynxpoint supports 94 gpio pins which can generate interrupts.
>> Driver will fail requests for pins that are marked as owned by ACPI, or
>> set in an alternate mode (non-gpio).
>>
>> Signed-off-by: Mathias Nyman<mathias.nyman@linux.intel.com>
>
> Nice shape on this patch, makes it easy to focus the review on the
> important stuff, thanks Magnus!
>
>> +config GPIO_LYNXPOINT
>> +       bool "Intel Lynxpoint GPIO support"
>
> So you don't want to be able to build it as module?
> (OK just odd on Intel systems.)

Reason for having it as bool was the subsys_initcall() got degraded
to something like module_init() if built as module. This was not good 
because the IO port ranges used for gpios were specified in ACPI tables 
both in the gpio device, and as a part of a motherboard device. Pnpacpi 
code would then reserve all the IO port ranges in the motherboard device 
before this gpio driver had a chance to take reserve them.

I have to re-check if this is still the case.

>> +struct lp_gpio {
>> +       struct gpio_chip        chip;
>> +       struct irq_domain       *domain;
>> +       struct platform_device  *pdev;
>> +       spinlock_t              lock;
>> +       unsigned                hwirq;
>
> This struct member is only used in probe() so make it a local variable there
> and cut it from the struct.
>
>> +       unsigned long           reg_base;
>> +       unsigned long           reg_len;
>
> Same comment for reg_len.

Will fix

>
>> +};
>> +
>> +static unsigned long gpio_reg(struct gpio_chip *chip, unsigned gpio, int reg)
>
> Rename lp_gpio_reg() so as to match the family.
>
> Rename the argument "gpio" to "offset" since it's a local number,
> not in the global GPIO numberspace. (Please change this everywhere
> a local index is used.)

Sure.

>
>> +{
>> +       struct lp_gpio *lg = container_of(chip, struct lp_gpio, chip);
>> +       int offset;
>> +
>> +       if (reg == LP_CONFIG1 || reg == LP_CONFIG2)
>> +               offset = gpio * 8;
>> +       else
>> +               offset = (gpio / 32) * 4;
>
> Put in some comment above this explaining the layout of the offsets
> for these two cases so we understand what's happening here.
>
>> +       return lg->reg_base + reg + offset;
>> +}
>> +
>> +static int lp_gpio_request(struct gpio_chip *chip, unsigned gpio)
>> +{
>> +       struct lp_gpio *lg = container_of(chip, struct lp_gpio, chip);
>> +       unsigned long reg = gpio_reg(chip, gpio, LP_CONFIG1);
>> +       unsigned long conf2 = gpio_reg(chip, gpio, LP_CONFIG2);
>> +       unsigned long acpi_use = gpio_reg(chip, gpio, LP_ACPI_OWNED);
>> +
>> +       /* Fail if BIOS reserved pin for ACPI use */
>> +       if (!(inl(acpi_use)&  BIT(gpio % 32))) {
>> +               dev_err(&lg->pdev->dev, "gpio %d reserved for ACPI\n", gpio);
>> +               return -EBUSY;
>> +       }
>
> This %32 magic only seems to consider the latter part of the if()
> statement in the gpio_reg() function. It's like you assume only the
> (gpio / 32) * 4 path to be taken. It seems that for the two models
> handled there this should be %8 or something. Or I'm getting it
> wrong, which is an indication that something is pretty unclear here...
>

I should document the register layout better. It's a bit messy because 
in some cases a register represents a functionality where each bit 
stands for a gpio (bitmapped) (each register is 32 bit, so to cover all 
94 gpios three 32bit registers are needed).

In other cases a register represent a gpio, and each bit a 
functionality. This is the case with LP_CONFIGx registers. So we got
94 LP_CONFIG1 registers and 94 LP_CONFIG2 registers.

In the case of acpi_use & BIT(gpio % 32) I know that acpi_used is 
pointing to one of the three LP_ACPI_OWNED bitmapped registers, and the 
(gpio / 32 * 4) path is taken, and % 32 will work.

I'll add better description of the register layout as a comment

>> +static void lp_irq_enable(struct irq_data *d)
>> +{
>> +       struct lp_gpio *lg = irq_data_get_irq_chip_data(d);
>> +       u32 gpio = irqd_to_hwirq(d);
>
> That variable is confusingly named. It's not a global gpio number,
> it's a local offset, so please rename it "offset".

sure, (is "pin" ok?  "offset" is already used in may places)

>> +#ifdef CONFIG_ACPI
>> +static const struct acpi_device_id lynxpoint_gpio_acpi_match[] = {
>> +       { "INT33C7", 0 },
>> +       { }
>> +};
>> +MODULE_DEVICE_TABLE(acpi, lynxpoint_gpio_acpi_match);
>> +#endif
>
> Aha so how many users are there of this driver which are not
> using ACPI?
>
> If zero, why not just depend on ACPI in Kconfig?
>

Only ACPI support for now.
Will remove ifdefs and add dependency.

>> +static int __devexit lp_gpio_remove(struct platform_device *pdev)
>
> All __devinit/__devexit macros are going away in v3.8 so delete them
> everywhere.

Ah, Ok.

>
> Yours,
> Linus Walleij

Thanks for taking the time to review this, I'll make the suggested changes.

I also got some offline comments about adding runtime pm/system suspend 
code to this driver.

-Mathias

Re: [PATCH 1/1] gpio: add Lynxpoint chipset gpio driver.

[Linus Walleij]

On Mon, Dec 10, 2012 at 3:06 PM, Mathias Nyman
<mathias.nyman@linux.intel.com> wrote:

>>> +static void lp_irq_enable(struct irq_data *d)
>>> +{
>>> +       struct lp_gpio *lg = irq_data_get_irq_chip_data(d);
>>> +       u32 gpio = irqd_to_hwirq(d);
>>
>>
>> That variable is confusingly named. It's not a global gpio number,
>> it's a local offset, so please rename it "offset".
>
> sure, (is "pin" ok?  "offset" is already used in may places)

pin will confuse things to the pinctrl subsystem ... offset is really nice.
But no super-big deal.

Yours,
Linus Walleij

Re: [PATCH 1/1] gpio: add Lynxpoint chipset gpio driver.

[Grant Likely]

On Fri,  7 Dec 2012 16:01:39 +0200, Mathias Nyman <mathias.nyman@linux.intel.com> wrote:
> Add gpio support for Intel Lynxpoint chipset.
> Lynxpoint supports 94 gpio pins which can generate interrupts.
> Driver will fail requests for pins that are marked as owned by ACPI, or
> set in an alternate mode (non-gpio).
> 
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> ---
> +static void lp_gpio_set(struct gpio_chip *chip, unsigned gpio, int value)
> +{
> +	struct lp_gpio *lg = container_of(chip, struct lp_gpio, chip);
> +	unsigned long reg = gpio_reg(chip, gpio, LP_CONFIG1);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&lg->lock, flags);
> +
> +	if (value)
> +		outl(inl(reg) | OUT_LVL_BIT, reg);
> +	else
> +		outl(inl(reg) & ~OUT_LVL_BIT, reg);
> +
> +	spin_unlock_irqrestore(&lg->lock, flags);
> +}

A *lot* of drivers implement their own GPIO ops like this, and they all
end up looking the same. Please take a look at
drivers/gpio/gpio-generic.c and see if you can use the stock operations
provided there.

g.

Re: [PATCH 1/1] gpio: add Lynxpoint chipset gpio driver.

[Linus Walleij]

On Tue, Dec 11, 2012 at 12:07 AM, Grant Likely
<grant.likely@secretlab.ca> wrote:
> On Fri,  7 Dec 2012 16:01:39 +0200, Mathias Nyman <mathias.nyman@linux.intel.com> wrote:
>> Add gpio support for Intel Lynxpoint chipset.
>> Lynxpoint supports 94 gpio pins which can generate interrupts.
>> Driver will fail requests for pins that are marked as owned by ACPI, or
>> set in an alternate mode (non-gpio).
>>
>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>> ---
>> +static void lp_gpio_set(struct gpio_chip *chip, unsigned gpio, int value)
>> +{
>> +     struct lp_gpio *lg = container_of(chip, struct lp_gpio, chip);
>> +     unsigned long reg = gpio_reg(chip, gpio, LP_CONFIG1);
>> +     unsigned long flags;
>> +
>> +     spin_lock_irqsave(&lg->lock, flags);
>> +
>> +     if (value)
>> +             outl(inl(reg) | OUT_LVL_BIT, reg);
>> +     else
>> +             outl(inl(reg) & ~OUT_LVL_BIT, reg);
>> +
>> +     spin_unlock_irqrestore(&lg->lock, flags);
>> +}
>
> A *lot* of drivers implement their own GPIO ops like this, and they all
> end up looking the same. Please take a look at
> drivers/gpio/gpio-generic.c and see if you can use the stock operations
> provided there.

I was under the impression that the generic code could not be
used off-the-shelf for drivers doing irqchips (like this one).

But maybe I'm mistaken or it's trivial to fix?

Yours,
Linus Walleij

Re: [PATCH 1/1] gpio: add Lynxpoint chipset gpio driver.

[Grant Likely]

On Tue, 11 Dec 2012 01:34:04 +0100, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Dec 11, 2012 at 12:07 AM, Grant Likely
> <grant.likely@secretlab.ca> wrote:
> > On Fri,  7 Dec 2012 16:01:39 +0200, Mathias Nyman <mathias.nyman@linux.intel.com> wrote:
> >> Add gpio support for Intel Lynxpoint chipset.
> >> Lynxpoint supports 94 gpio pins which can generate interrupts.
> >> Driver will fail requests for pins that are marked as owned by ACPI, or
> >> set in an alternate mode (non-gpio).
> >>
> >> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> >> ---
> >> +static void lp_gpio_set(struct gpio_chip *chip, unsigned gpio, int value)
> >> +{
> >> +     struct lp_gpio *lg = container_of(chip, struct lp_gpio, chip);
> >> +     unsigned long reg = gpio_reg(chip, gpio, LP_CONFIG1);
> >> +     unsigned long flags;
> >> +
> >> +     spin_lock_irqsave(&lg->lock, flags);
> >> +
> >> +     if (value)
> >> +             outl(inl(reg) | OUT_LVL_BIT, reg);
> >> +     else
> >> +             outl(inl(reg) & ~OUT_LVL_BIT, reg);
> >> +
> >> +     spin_unlock_irqrestore(&lg->lock, flags);
> >> +}
> >
> > A *lot* of drivers implement their own GPIO ops like this, and they all
> > end up looking the same. Please take a look at
> > drivers/gpio/gpio-generic.c and see if you can use the stock operations
> > provided there.
> 
> I was under the impression that the generic code could not be
> used off-the-shelf for drivers doing irqchips (like this one).
> 
> But maybe I'm mistaken or it's trivial to fix?

There's two parts to the generic code, a library of helper ops and a
full fledged platform driver. The library can be used without the
platform driver bit.

g.

Re: [PATCH 1/1] gpio: add Lynxpoint chipset gpio driver.

[Mathias Nyman]

On 12/11/2012 01:07 AM, Grant Likely wrote:
> On Fri,  7 Dec 2012 16:01:39 +0200, Mathias Nyman<mathias.nyman@linux.intel.com>  wrote:
>> Add gpio support for Intel Lynxpoint chipset.
>> Lynxpoint supports 94 gpio pins which can generate interrupts.
>> Driver will fail requests for pins that are marked as owned by ACPI, or
>> set in an alternate mode (non-gpio).
>>
>> Signed-off-by: Mathias Nyman<mathias.nyman@linux.intel.com>
>> ---
>> +static void lp_gpio_set(struct gpio_chip *chip, unsigned gpio, int value)
>> +{
>> +	struct lp_gpio *lg = container_of(chip, struct lp_gpio, chip);
>> +	unsigned long reg = gpio_reg(chip, gpio, LP_CONFIG1);
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&lg->lock, flags);
>> +
>> +	if (value)
>> +		outl(inl(reg) | OUT_LVL_BIT, reg);
>> +	else
>> +		outl(inl(reg)&  ~OUT_LVL_BIT, reg);
>> +
>> +	spin_unlock_irqrestore(&lg->lock, flags);
>> +}
>
> A *lot* of drivers implement their own GPIO ops like this, and they all
> end up looking the same. Please take a look at
> drivers/gpio/gpio-generic.c and see if you can use the stock operations
> provided there.
>
> g.
>

Look quite similar but turns out the only potential generic ones which 
could be used are the bgpio_set() and bgpio_get() funtions. Using them 
would require custom pin2mask, write_reg, read_reg, etc functions.
bgpio_set_direction

I guess using gpio-generic code fits memory mapped devices better. 
Trying to use it for io port devices would require a lot of code mangling.

-Mathias

Re: [PATCH 1/1] gpio: add Lynxpoint chipset gpio driver.

[Grant Likely]

On Tue, 11 Dec 2012 13:40:51 +0200, Mathias Nyman <mathias.nyman@linux.intel.com> wrote:
> On 12/11/2012 01:07 AM, Grant Likely wrote:
> > On Fri,  7 Dec 2012 16:01:39 +0200, Mathias Nyman<mathias.nyman@linux.intel.com>  wrote:
> >> Add gpio support for Intel Lynxpoint chipset.
> >> Lynxpoint supports 94 gpio pins which can generate interrupts.
> >> Driver will fail requests for pins that are marked as owned by ACPI, or
> >> set in an alternate mode (non-gpio).
> >>
> >> Signed-off-by: Mathias Nyman<mathias.nyman@linux.intel.com>
> >> ---
> >> +static void lp_gpio_set(struct gpio_chip *chip, unsigned gpio, int value)
> >> +{
> >> +	struct lp_gpio *lg = container_of(chip, struct lp_gpio, chip);
> >> +	unsigned long reg = gpio_reg(chip, gpio, LP_CONFIG1);
> >> +	unsigned long flags;
> >> +
> >> +	spin_lock_irqsave(&lg->lock, flags);
> >> +
> >> +	if (value)
> >> +		outl(inl(reg) | OUT_LVL_BIT, reg);
> >> +	else
> >> +		outl(inl(reg)&  ~OUT_LVL_BIT, reg);
> >> +
> >> +	spin_unlock_irqrestore(&lg->lock, flags);
> >> +}
> >
> > A *lot* of drivers implement their own GPIO ops like this, and they all
> > end up looking the same. Please take a look at
> > drivers/gpio/gpio-generic.c and see if you can use the stock operations
> > provided there.
> >
> > g.
> >
> 
> Look quite similar but turns out the only potential generic ones which 
> could be used are the bgpio_set() and bgpio_get() funtions. Using them 
> would require custom pin2mask, write_reg, read_reg, etc functions.
> bgpio_set_direction

You would need new write_reg and read_reg, but they'd just become part
of the library. Ah, but I see now that each GPIO has a separate
register, and not a bitmask for multiple gpio lines. Fair enough.

g.