[PATCH 0/2 v2] gpio: msm7200a: Add gpiolib support for MSM chips.

[PATCH 0/2 v2] gpio: msm7200a: Add gpiolib support for MSM chips.

[Gregory Bean]

Hi folks:

Here is a proposed driver which adds gpio and irq support to all 
currently-supported arm-msm SoCs. Feedback is greatly appreciated.

Revision history:
v2: Cleaned up some housekeeping noise in probe and remove methods.

Gregory Bean (2):
  gpio: msm7200a: Add gpiolib support for MSM chips.
  gpio: msm7200a: Add irq support to msm-gpiolib.

 MAINTAINERS                   |    2 +
 drivers/gpio/Kconfig          |    8 +
 drivers/gpio/Makefile         |    1 +
 drivers/gpio/msm7200a-gpio.c  |  397 +++++++++++++++++++++++++++++++++++++++++
 include/linux/msm7200a-gpio.h |   51 ++++++
 5 files changed, 459 insertions(+), 0 deletions(-)
 create mode 100644 drivers/gpio/msm7200a-gpio.c
 create mode 100644 include/linux/msm7200a-gpio.h

--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 1/2 v2] gpio: msm7200a: Add gpiolib support for MSM chips.

[Gregory Bean]

Add support for uniprocessor MSM chips whose TLMM/GPIO design
is the same as the MSM7200A.
This includes, but is not necessarily limited to, the:
MSM7200A, MSM7x25, MSM7x27, MSM7x30, QSD8x50, QSD8x50A

Signed-off-by: Gregory Bean <gbean@codeaurora.org>
---
 MAINTAINERS                   |    2 +
 drivers/gpio/Kconfig          |    8 ++
 drivers/gpio/Makefile         |    1 +
 drivers/gpio/msm7200a-gpio.c  |  196 +++++++++++++++++++++++++++++++++++++++++
 include/linux/msm7200a-gpio.h |   44 +++++++++
 5 files changed, 251 insertions(+), 0 deletions(-)
 create mode 100644 drivers/gpio/msm7200a-gpio.c
 create mode 100644 include/linux/msm7200a-gpio.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 6d119c9..bdfd31d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -819,6 +819,8 @@ F:	drivers/mmc/host/msm_sdcc.c
 F:	drivers/mmc/host/msm_sdcc.h
 F:	drivers/serial/msm_serial.h
 F:	drivers/serial/msm_serial.c
+F:	drivers/gpio/msm7200a-gpio.c
+F:	include/linux/msm7200a-gpio.h
 T:	git git://codeaurora.org/quic/kernel/dwalker/linux-msm.git
 S:	Maintained
 
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 724038d..557738a 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -76,6 +76,14 @@ config GPIO_IT8761E
 	help
 	  Say yes here to support GPIO functionality of IT8761E super I/O chip.
 
+config GPIO_MSM7200A
+	tristate "Qualcomm MSM7200A SoC GPIO support"
+	depends on GPIOLIB
+	help
+	  Say yes here to support GPIO functionality on Qualcomm's
+	  MSM chipsets which descend from the MSM7200a:
+	  MSM7x01(a), MSM7x25, MSM7x27, MSM7x30, QSD8x50(a).
+
 config GPIO_PL061
 	bool "PrimeCell PL061 GPIO support"
 	depends on ARM_AMBA
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 51c3cdd..2389c29 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_GPIO_MAX7301)	+= max7301.o
 obj-$(CONFIG_GPIO_MAX732X)	+= max732x.o
 obj-$(CONFIG_GPIO_MC33880)	+= mc33880.o
 obj-$(CONFIG_GPIO_MCP23S08)	+= mcp23s08.o
+obj-$(CONFIG_GPIO_MSM7200A)	+= msm7200a-gpio.o
 obj-$(CONFIG_GPIO_PCA953X)	+= pca953x.o
 obj-$(CONFIG_GPIO_PCF857X)	+= pcf857x.o
 obj-$(CONFIG_GPIO_PL061)	+= pl061.o
diff --git a/drivers/gpio/msm7200a-gpio.c b/drivers/gpio/msm7200a-gpio.c
new file mode 100644
index 0000000..62db753
--- /dev/null
+++ b/drivers/gpio/msm7200a-gpio.c
@@ -0,0 +1,196 @@
+/*
+ * Driver for Qualcomm MSM7200a and related SoC GPIO.
+ * Supported chipset families include:
+ * MSM7x01(a), MSM7x25, MSM7x27, MSM7x30, QSD8x50(a)
+ *
+ * Copyright (C) 2007 Google, Inc.
+ * Copyright (c) 2010, Code Aurora Forum. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 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; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+ * 02110-1301, USA.
+ */
+#include <linux/kernel.h>
+#include <linux/gpio.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/msm7200a-gpio.h>
+
+struct msm_gpio_dev {
+	struct gpio_chip		gpio_chip;
+	spinlock_t			lock;
+	struct msm7200a_gpio_regs	regs;
+};
+
+#define TO_MSM_GPIO_DEV(c) container_of(c, struct msm_gpio_dev, gpio_chip)
+
+static inline unsigned bit(unsigned offset)
+{
+	BUG_ON(offset >= sizeof(unsigned) * 8);
+	return 1U << offset;
+}
+
+/*
+ * This function assumes that msm_gpio_dev::lock is held.
+ */
+static inline void set_gpio_bit(unsigned n, void __iomem *reg)
+{
+	writel(readl(reg) | bit(n), reg);
+}
+
+/*
+ * This function assumes that msm_gpio_dev::lock is held.
+ */
+static inline void clr_gpio_bit(unsigned n, void __iomem *reg)
+{
+	writel(readl(reg) & ~bit(n), reg);
+}
+
+/*
+ * This function assumes that msm_gpio_dev::lock is held.
+ */
+static inline void
+msm_gpio_write(struct msm_gpio_dev *dev, unsigned n, unsigned on)
+{
+	if (on)
+		set_gpio_bit(n, dev->regs.out);
+	else
+		clr_gpio_bit(n, dev->regs.out);
+}
+
+static int gpio_chip_direction_input(struct gpio_chip *chip, unsigned offset)
+{
+	struct msm_gpio_dev *msm_gpio = TO_MSM_GPIO_DEV(chip);
+	unsigned long irq_flags;
+
+	spin_lock_irqsave(&msm_gpio->lock, irq_flags);
+	clr_gpio_bit(offset, msm_gpio->regs.oe);
+	spin_unlock_irqrestore(&msm_gpio->lock, irq_flags);
+
+	return 0;
+}
+
+static int
+gpio_chip_direction_output(struct gpio_chip *chip, unsigned offset, int value)
+{
+	struct msm_gpio_dev *msm_gpio = TO_MSM_GPIO_DEV(chip);
+	unsigned long irq_flags;
+
+	spin_lock_irqsave(&msm_gpio->lock, irq_flags);
+
+	msm_gpio_write(msm_gpio, offset, value);
+	set_gpio_bit(offset, msm_gpio->regs.oe);
+	spin_unlock_irqrestore(&msm_gpio->lock, irq_flags);
+
+	return 0;
+}
+
+static int gpio_chip_get(struct gpio_chip *chip, unsigned offset)
+{
+	struct msm_gpio_dev *msm_gpio = TO_MSM_GPIO_DEV(chip);
+	unsigned long irq_flags;
+	int ret;
+
+	spin_lock_irqsave(&msm_gpio->lock, irq_flags);
+	ret = readl(msm_gpio->regs.in) & bit(offset) ? 1 : 0;
+	spin_unlock_irqrestore(&msm_gpio->lock, irq_flags);
+
+	return ret;
+}
+
+static void gpio_chip_set(struct gpio_chip *chip, unsigned offset, int value)
+{
+	struct msm_gpio_dev *msm_gpio = TO_MSM_GPIO_DEV(chip);
+	unsigned long irq_flags;
+
+	spin_lock_irqsave(&msm_gpio->lock, irq_flags);
+	msm_gpio_write(msm_gpio, offset, value);
+	spin_unlock_irqrestore(&msm_gpio->lock, irq_flags);
+}
+
+static int msm_gpio_probe(struct platform_device *dev)
+{
+	struct msm_gpio_dev *msm_gpio;
+	struct msm7200a_gpio_platform_data *pdata =
+		(struct msm7200a_gpio_platform_data *)dev->dev.platform_data;
+	int ret;
+
+	if (!pdata)
+		return -EINVAL;
+
+	msm_gpio = kzalloc(sizeof(struct msm_gpio_dev), GFP_KERNEL);
+	if (!msm_gpio)
+		return -ENOMEM;
+
+	spin_lock_init(&msm_gpio->lock);
+	platform_set_drvdata(dev, msm_gpio);
+	memcpy(&msm_gpio->regs,
+	       &pdata->regs,
+	       sizeof(struct msm7200a_gpio_regs));
+
+	msm_gpio->gpio_chip.label            = dev->name;
+	msm_gpio->gpio_chip.base             = pdata->gpio_base;
+	msm_gpio->gpio_chip.ngpio            = pdata->ngpio;
+	msm_gpio->gpio_chip.direction_input  = gpio_chip_direction_input;
+	msm_gpio->gpio_chip.direction_output = gpio_chip_direction_output;
+	msm_gpio->gpio_chip.get              = gpio_chip_get;
+	msm_gpio->gpio_chip.set              = gpio_chip_set;
+
+	ret = gpiochip_add(&msm_gpio->gpio_chip);
+	if (ret < 0)
+		goto err_post_malloc;
+
+	return ret;
+err_post_malloc:
+	kfree(msm_gpio);
+	return ret;
+}
+
+static int msm_gpio_remove(struct platform_device *dev)
+{
+	struct msm_gpio_dev *msm_gpio = platform_get_drvdata(dev);
+	int ret = gpiochip_remove(&msm_gpio->gpio_chip);
+
+	if (ret < 0)
+		return ret;
+
+	kfree(msm_gpio);
+
+	return 0;
+}
+
+static struct platform_driver msm_gpio_driver = {
+	.probe = msm_gpio_probe,
+	.remove = msm_gpio_remove,
+	.driver = {
+		.name = "msm7200a-gpio",
+		.owner = THIS_MODULE,
+	},
+};
+
+static int __init msm_gpio_init(void)
+{
+	return platform_driver_register(&msm_gpio_driver);
+}
+
+static void __exit msm_gpio_exit(void)
+{
+	platform_driver_unregister(&msm_gpio_driver);
+}
+
+postcore_initcall(msm_gpio_init);
+module_exit(msm_gpio_exit);
+
+MODULE_DESCRIPTION("Driver for Qualcomm MSM 7200a-family SoC GPIOs");
+MODULE_LICENSE("GPLv2");
diff --git a/include/linux/msm7200a-gpio.h b/include/linux/msm7200a-gpio.h
new file mode 100644
index 0000000..3f1ef38
--- /dev/null
+++ b/include/linux/msm7200a-gpio.h
@@ -0,0 +1,44 @@
+/* Copyright (c) 2010, Code Aurora Forum. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are
+ * met:
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above
+ *       copyright notice, this list of conditions and the following
+ *       disclaimer in the documentation and/or other materials provided
+ *       with the distribution.
+ *     * Neither the name of Code Aurora Forum, Inc. nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED "AS IS" AND ANY EXPRESS OR IMPLIED
+ * WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT
+ * ARE DISCLAIMED.  IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS
+ * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+ * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
+ * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE
+ * OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN
+ * IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ */
+#ifndef __LINUX_MSM7200A_GPIO_H
+#define __LINUX_MSM7200A_GPIO_H
+
+struct msm7200a_gpio_regs {
+	void __iomem *in;
+	void __iomem *out;
+	void __iomem *oe;
+};
+
+struct msm7200a_gpio_platform_data {
+	unsigned gpio_base;
+	unsigned ngpio;
+	struct msm7200a_gpio_regs regs;
+};
+
+#endif
-- 
1.7.0.4

--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 2/2 v2] gpio: msm7200a: Add irq support to msm-gpiolib.

[Gregory Bean]

Signed-off-by: Gregory Bean <gbean@codeaurora.org>
---
 drivers/gpio/msm7200a-gpio.c  |  203 ++++++++++++++++++++++++++++++++++++++++-
 include/linux/msm7200a-gpio.h |    7 ++
 2 files changed, 209 insertions(+), 1 deletions(-)

diff --git a/drivers/gpio/msm7200a-gpio.c b/drivers/gpio/msm7200a-gpio.c
index 62db753..05c367d 100644
--- a/drivers/gpio/msm7200a-gpio.c
+++ b/drivers/gpio/msm7200a-gpio.c
@@ -23,13 +23,25 @@
 #include <linux/kernel.h>
 #include <linux/gpio.h>
 #include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/msm7200a-gpio.h>
 
+/*
+ * The INT_STATUS register latches both edge- and level-detection events,
+ * which is atypical.  Turning on DONT_LATCH_LEVEL_IRQS causes level irq
+ * triggers to be forgotten across mask/unmask calls, emulating a more
+ * traditional setup.
+ */
+#define MSM_GPIO_DONT_LATCH_LEVEL_IRQS 1
+
 struct msm_gpio_dev {
 	struct gpio_chip		gpio_chip;
 	spinlock_t			lock;
+	unsigned			irq_base;
+	unsigned			irq_summary;
 	struct msm7200a_gpio_regs	regs;
 };
 
@@ -119,12 +131,160 @@ static void gpio_chip_set(struct gpio_chip *chip, unsigned offset, int value)
 	spin_unlock_irqrestore(&msm_gpio->lock, irq_flags);
 }
 
+static int gpio_chip_to_irq(struct gpio_chip *chip, unsigned offset)
+{
+	struct msm_gpio_dev *msm_gpio = TO_MSM_GPIO_DEV(chip);
+	return msm_gpio->irq_base + offset;
+}
+
+#if MSM_GPIO_DONT_LATCH_LEVEL_IRQS
+static inline void forget_level_irq(struct msm_gpio_dev *msm_gpio,
+				unsigned offset)
+{
+	unsigned v = readl(msm_gpio->regs.int_edge);
+	unsigned b = bit(offset);
+
+	if (!(v & b))
+		writel(b, msm_gpio->regs.int_clear);
+
+}
+#else
+static inline void forget_level_irq(struct msm_gpio_dev *msm, unsigned off)
+{
+}
+#endif
+
+static void msm_gpio_irq_mask(unsigned int irq)
+{
+	unsigned long irq_flags;
+	struct msm_gpio_dev *msm_gpio = get_irq_chip_data(irq);
+	unsigned offset = irq - msm_gpio->irq_base;
+
+	spin_lock_irqsave(&msm_gpio->lock, irq_flags);
+	forget_level_irq(msm_gpio, offset);
+	clr_gpio_bit(offset, msm_gpio->regs.int_en);
+	spin_unlock_irqrestore(&msm_gpio->lock, irq_flags);
+}
+
+static void msm_gpio_irq_unmask(unsigned int irq)
+{
+	unsigned long irq_flags;
+	struct msm_gpio_dev *msm_gpio = get_irq_chip_data(irq);
+	unsigned offset = irq - msm_gpio->irq_base;
+
+	spin_lock_irqsave(&msm_gpio->lock, irq_flags);
+	forget_level_irq(msm_gpio, offset);
+	set_gpio_bit(offset, msm_gpio->regs.int_en);
+	spin_unlock_irqrestore(&msm_gpio->lock, irq_flags);
+}
+
+static int msm_gpio_irq_set_type(unsigned int irq, unsigned int flow_type)
+{
+	unsigned long irq_flags;
+	struct msm_gpio_dev *msm_gpio = get_irq_chip_data(irq);
+	unsigned offset = irq - msm_gpio->irq_base;
+
+	if ((flow_type & (IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING)) ==
+		(IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING))
+		return -ENOTSUPP;
+
+	if ((flow_type & (IRQF_TRIGGER_HIGH | IRQF_TRIGGER_LOW)) ==
+		(IRQF_TRIGGER_HIGH | IRQF_TRIGGER_LOW))
+		return -ENOTSUPP;
+
+	spin_lock_irqsave(&msm_gpio->lock, irq_flags);
+
+	if (flow_type & (IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING)) {
+		set_gpio_bit(offset, msm_gpio->regs.int_edge);
+		irq_desc[irq].handle_irq = handle_edge_irq;
+	} else {
+		clr_gpio_bit(offset, msm_gpio->regs.int_edge);
+		irq_desc[irq].handle_irq = handle_level_irq;
+	}
+
+	if (flow_type & (IRQF_TRIGGER_HIGH | IRQF_TRIGGER_RISING))
+		set_gpio_bit(offset, msm_gpio->regs.int_pos);
+	else
+		clr_gpio_bit(offset, msm_gpio->regs.int_pos);
+
+	spin_unlock_irqrestore(&msm_gpio->lock, irq_flags);
+
+	return 0;
+}
+
+static void msm_gpio_irq_mask_ack(unsigned int irq)
+{
+	msm_gpio_irq_mask(irq);
+}
+
+static int msm_gpio_irq_set_affinity(unsigned int irq,
+				const struct cpumask *dest)
+{
+	return -ENOTSUPP;
+}
+
+static int msm_gpio_irq_retrigger(unsigned int irq)
+{
+	return -ENOTSUPP;
+}
+
+static int msm_gpio_irq_set_wake(unsigned int irq, unsigned int on)
+{
+	return -ENOTSUPP;
+}
+
+static irqreturn_t msm_gpio_irq_handler(int irq, void *dev)
+{
+	unsigned long irq_flags;
+	int b, m;
+	unsigned e, s, v;
+
+	struct msm_gpio_dev *msm_gpio = (struct msm_gpio_dev *)dev;
+
+	/*
+	 * The int_status register latches trigger events whether or not
+	 * the gpio line is enabled as an interrupt source.  Therefore,
+	 * the set of pins which defines the interrupts which need to fire
+	 * is the intersection of int_status and int_en - int_status
+	 * alone provides an incomplete picture.
+	 */
+	spin_lock_irqsave(&msm_gpio->lock, irq_flags);
+	s = readl(msm_gpio->regs.int_status);
+	e = readl(msm_gpio->regs.int_en);
+	v = s & e;
+	if (v)
+		writel(v, msm_gpio->regs.int_clear);
+	spin_unlock_irqrestore(&msm_gpio->lock, irq_flags);
+
+	if (!v)
+		return IRQ_NONE;
+
+	while (v) {
+		m = v & -v;
+		b = fls(m) - 1;
+		v &= ~m;
+		generic_handle_irq(msm_gpio->irq_base + b);
+	}
+	return IRQ_HANDLED;
+}
+
+static struct irq_chip msm_gpio_irq_chip = {
+	.name			= "msm_gpio",
+	.mask			= msm_gpio_irq_mask,
+	.mask_ack		= msm_gpio_irq_mask_ack,
+	.unmask			= msm_gpio_irq_unmask,
+	.set_affinity		= msm_gpio_irq_set_affinity,
+	.retrigger		= msm_gpio_irq_retrigger,
+	.set_type		= msm_gpio_irq_set_type,
+	.set_wake		= msm_gpio_irq_set_wake,
+};
+
 static int msm_gpio_probe(struct platform_device *dev)
 {
 	struct msm_gpio_dev *msm_gpio;
 	struct msm7200a_gpio_platform_data *pdata =
 		(struct msm7200a_gpio_platform_data *)dev->dev.platform_data;
-	int ret;
+	int i, irq, ret;
 
 	if (!pdata)
 		return -EINVAL;
@@ -146,12 +306,52 @@ static int msm_gpio_probe(struct platform_device *dev)
 	msm_gpio->gpio_chip.direction_output = gpio_chip_direction_output;
 	msm_gpio->gpio_chip.get              = gpio_chip_get;
 	msm_gpio->gpio_chip.set              = gpio_chip_set;
+	msm_gpio->gpio_chip.to_irq           = gpio_chip_to_irq;
+	msm_gpio->irq_base                   = pdata->irq_base;
+	msm_gpio->irq_summary                = pdata->irq_summary;
 
 	ret = gpiochip_add(&msm_gpio->gpio_chip);
 	if (ret < 0)
 		goto err_post_malloc;
 
+	for (i = 0; i < msm_gpio->gpio_chip.ngpio; ++i) {
+		irq = msm_gpio->irq_base + i;
+		set_irq_chip_data(irq, msm_gpio);
+		set_irq_chip(irq, &msm_gpio_irq_chip);
+		set_irq_handler(irq, handle_level_irq);
+		set_irq_flags(irq, IRQF_VALID);
+	}
+
+	/*
+	 * We use a level-triggered interrupt because of the nature
+	 * of the shared GPIO-group interrupt.
+	 *
+	 * Many GPIO chips may be sharing the same group IRQ line, and
+	 * it is possible for GPIO interrupt to re-occur while the system
+	 * is still servicing the group interrupt associated with it.
+	 * The group IRQ line would not de-assert and re-assert, and
+	 * we'd get no second edge to cause the group IRQ to be handled again.
+	 *
+	 * Using a level interrupt guarantees that the group IRQ handlers
+	 * will continue to be called as long as any GPIO chip in the group
+	 * is asserting, even if the condition began while the group
+	 * handler was in mid-pass.
+	 */
+	ret = request_irq(msm_gpio->irq_summary,
+			  msm_gpio_irq_handler,
+			  IRQF_SHARED | IRQF_TRIGGER_HIGH,
+			  dev->name,
+			  msm_gpio);
+	if (ret < 0)
+		goto err_post_gpiochip_add;
+
 	return ret;
+err_post_gpiochip_add:
+	/*
+	 * Under no circumstances should a line be held on a gpiochip
+	 * which hasn't finished probing.
+	 */
+	BUG_ON(gpiochip_remove(&msm_gpio->gpio_chip) < 0);
 err_post_malloc:
 	kfree(msm_gpio);
 	return ret;
@@ -165,6 +365,7 @@ static int msm_gpio_remove(struct platform_device *dev)
 	if (ret < 0)
 		return ret;
 
+	free_irq(msm_gpio->irq_summary, msm_gpio);
 	kfree(msm_gpio);
 
 	return 0;
diff --git a/include/linux/msm7200a-gpio.h b/include/linux/msm7200a-gpio.h
index 3f1ef38..7af4dd6 100644
--- a/include/linux/msm7200a-gpio.h
+++ b/include/linux/msm7200a-gpio.h
@@ -33,11 +33,18 @@ struct msm7200a_gpio_regs {
 	void __iomem *in;
 	void __iomem *out;
 	void __iomem *oe;
+	void __iomem *int_status;
+	void __iomem *int_clear;
+	void __iomem *int_en;
+	void __iomem *int_edge;
+	void __iomem *int_pos;
 };
 
 struct msm7200a_gpio_platform_data {
 	unsigned gpio_base;
 	unsigned ngpio;
+	unsigned irq_base;
+	unsigned irq_summary;
 	struct msm7200a_gpio_regs regs;
 };
 
-- 
1.7.0.4

--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 1/2 v2] gpio: msm7200a: Add gpiolib support for MSM chips.

[Andrew Morton]

On Fri, 11 Jun 2010 17:19:28 -0700
Gregory Bean <gbean@codeaurora.org> wrote:

> Add support for uniprocessor MSM chips whose TLMM/GPIO design
> is the same as the MSM7200A.
> This includes, but is not necessarily limited to, the:
> MSM7200A, MSM7x25, MSM7x27, MSM7x30, QSD8x50, QSD8x50A
> 
>
> ...
>
> +static inline unsigned bit(unsigned offset)
> +{
> +	BUG_ON(offset >= sizeof(unsigned) * 8);
> +	return 1U << offset;
> +}

Could use bitops.h's BIT(), but it hardly matters.

> +/*
> + * This function assumes that msm_gpio_dev::lock is held.
> + */
> +static inline void set_gpio_bit(unsigned n, void __iomem *reg)
> +{
> +	writel(readl(reg) | bit(n), reg);
> +}
> +
> +/*
> + * This function assumes that msm_gpio_dev::lock is held.
> + */
> +static inline void clr_gpio_bit(unsigned n, void __iomem *reg)
> +{
> +	writel(readl(reg) & ~bit(n), reg);
> +}
> +
> +/*
> + * This function assumes that msm_gpio_dev::lock is held.
> + */
> +static inline void
> +msm_gpio_write(struct msm_gpio_dev *dev, unsigned n, unsigned on)
> +{
> +	if (on)
> +		set_gpio_bit(n, dev->regs.out);
> +	else
> +		clr_gpio_bit(n, dev->regs.out);
> +}

Recent gcc's often uninline things whcih were marked inline.  Doesn't
matter much.

>
> ...
>
> +static int gpio_chip_get(struct gpio_chip *chip, unsigned offset)
> +{
> +	struct msm_gpio_dev *msm_gpio = TO_MSM_GPIO_DEV(chip);
> +	unsigned long irq_flags;
> +	int ret;
> +
> +	spin_lock_irqsave(&msm_gpio->lock, irq_flags);
> +	ret = readl(msm_gpio->regs.in) & bit(offset) ? 1 : 0;
> +	spin_unlock_irqrestore(&msm_gpio->lock, irq_flags);
> +
> +	return ret;
> +}

The locking here is actually unneeded, I expect.

> +static void gpio_chip_set(struct gpio_chip *chip, unsigned offset, int value)
> +{
> +	struct msm_gpio_dev *msm_gpio = TO_MSM_GPIO_DEV(chip);
> +	unsigned long irq_flags;
> +
> +	spin_lock_irqsave(&msm_gpio->lock, irq_flags);
> +	msm_gpio_write(msm_gpio, offset, value);
> +	spin_unlock_irqrestore(&msm_gpio->lock, irq_flags);
> +}
> +
> +static int msm_gpio_probe(struct platform_device *dev)
> +{
> +	struct msm_gpio_dev *msm_gpio;
> +	struct msm7200a_gpio_platform_data *pdata =
> +		(struct msm7200a_gpio_platform_data *)dev->dev.platform_data;

The the typecast of a void* is unneeded and undesirable because it
defeats typechecking.

> +	int ret;
> +
> +	if (!pdata)
> +		return -EINVAL;
> +
> +	msm_gpio = kzalloc(sizeof(struct msm_gpio_dev), GFP_KERNEL);
> +	if (!msm_gpio)
> +		return -ENOMEM;
> +
> +	spin_lock_init(&msm_gpio->lock);
> +	platform_set_drvdata(dev, msm_gpio);
> +	memcpy(&msm_gpio->regs,
> +	       &pdata->regs,
> +	       sizeof(struct msm7200a_gpio_regs));
> +
> +	msm_gpio->gpio_chip.label            = dev->name;
> +	msm_gpio->gpio_chip.base             = pdata->gpio_base;
> +	msm_gpio->gpio_chip.ngpio            = pdata->ngpio;
> +	msm_gpio->gpio_chip.direction_input  = gpio_chip_direction_input;
> +	msm_gpio->gpio_chip.direction_output = gpio_chip_direction_output;
> +	msm_gpio->gpio_chip.get              = gpio_chip_get;
> +	msm_gpio->gpio_chip.set              = gpio_chip_set;
> +
> +	ret = gpiochip_add(&msm_gpio->gpio_chip);
> +	if (ret < 0)
> +		goto err_post_malloc;
> +
> +	return ret;
> +err_post_malloc:
> +	kfree(msm_gpio);

Maybe undo platform_set_drvdata() here?

> +	return ret;
> +}
> +
>
> ...
>

Re: [PATCH 2/2 v2] gpio: msm7200a: Add irq support to msm-gpiolib.

[Andrew Morton]

On Fri, 11 Jun 2010 17:19:29 -0700
Gregory Bean <gbean@codeaurora.org> wrote:

>
> ...
>
> +/*
> + * The INT_STATUS register latches both edge- and level-detection events,
> + * which is atypical.  Turning on DONT_LATCH_LEVEL_IRQS causes level irq
> + * triggers to be forgotten across mask/unmask calls, emulating a more
> + * traditional setup.
> + */
> +#define MSM_GPIO_DONT_LATCH_LEVEL_IRQS 1

It's unusual to require a source-code edit to enable a compile-time
feature.

If this knob is actually useful then I'd suggest making it a Kconfig
thing.  Or, much much better, a module parameter settable at modprobe
time.  Or, much much better, a /sys knob (or whatever) which can be set
at runtime.  Or, much much better, just autodetect the desired
behaviour and don't hassle the nice users ;)

>
> ...
>
> +static int msm_gpio_irq_set_type(unsigned int irq, unsigned int flow_type)
> +{
> +	unsigned long irq_flags;
> +	struct msm_gpio_dev *msm_gpio = get_irq_chip_data(irq);
> +	unsigned offset = irq - msm_gpio->irq_base;
> +
> +	if ((flow_type & (IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING)) ==
> +		(IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING))
> +		return -ENOTSUPP;
> +
> +	if ((flow_type & (IRQF_TRIGGER_HIGH | IRQF_TRIGGER_LOW)) ==
> +		(IRQF_TRIGGER_HIGH | IRQF_TRIGGER_LOW))
> +		return -ENOTSUPP;

According to the code comments, ENOTSUPP is "Defined for the NFSv3
protocol".  I'd imagine that cellphone software developers who haven't
even configured nfs3 into their builds will get a bit puzzled if this
comes out.

I'd suggest using something simple and generic: EIO, EINVAL, etc.

>
> ...
>
> +static void msm_gpio_irq_mask_ack(unsigned int irq)
> +{
> +	msm_gpio_irq_mask(irq);
> +}
> +
> +static int msm_gpio_irq_set_affinity(unsigned int irq,
> +				const struct cpumask *dest)
> +{
> +	return -ENOTSUPP;
> +}
> +
> +static int msm_gpio_irq_retrigger(unsigned int irq)
> +{
> +	return -ENOTSUPP;
> +}
> +
> +static int msm_gpio_irq_set_wake(unsigned int irq, unsigned int on)
> +{
> +	return -ENOTSUPP;
> +}

dittoes.

> +static irqreturn_t msm_gpio_irq_handler(int irq, void *dev)
> +{
> +	unsigned long irq_flags;
> +	int b, m;
> +	unsigned e, s, v;
> +
> +	struct msm_gpio_dev *msm_gpio = (struct msm_gpio_dev *)dev;

Didn't need that blank line there.

Unneeded cast.

> +	/*
> +	 * The int_status register latches trigger events whether or not
> +	 * the gpio line is enabled as an interrupt source.  Therefore,
> +	 * the set of pins which defines the interrupts which need to fire
> +	 * is the intersection of int_status and int_en - int_status
> +	 * alone provides an incomplete picture.
> +	 */
> +	spin_lock_irqsave(&msm_gpio->lock, irq_flags);
> +	s = readl(msm_gpio->regs.int_status);
> +	e = readl(msm_gpio->regs.int_en);
> +	v = s & e;
> +	if (v)
> +		writel(v, msm_gpio->regs.int_clear);
> +	spin_unlock_irqrestore(&msm_gpio->lock, irq_flags);

Plain old spin_lock() is probably OK in the IRQ handler.  It won't be
interrupting itself.

> +	if (!v)
> +		return IRQ_NONE;
> +
> +	while (v) {
> +		m = v & -v;
> +		b = fls(m) - 1;
> +		v &= ~m;
> +		generic_handle_irq(msm_gpio->irq_base + b);
> +	}
> +	return IRQ_HANDLED;
> +}
> +
>
> ...
>