Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1513798 > unrolled thread

[PATCH] gpio: tegra186: Add support for T186 GPIO

Back to article view | Back to linux.kernel

Contents

  [PATCH] gpio: tegra186: Add support for T186 GPIO Suresh Mangipudi <smangipudi@nvidia.com> - 2016-11-02 11:50 +0100
    Re: [PATCH] gpio: tegra186: Add support for T186 GPIO Linus Walleij <linus.walleij@linaro.org> - 2016-11-07 09:00 +0100
      Re: [PATCH] gpio: tegra186: Add support for T186 GPIO Thierry Reding <thierry.reding@gmail.com> - 2016-11-07 14:30 +0100
        Re: [PATCH] gpio: tegra186: Add support for T186 GPIO Olof Johansson <olof@lixom.net> - 2016-11-08 02:50 +0100
          Re: [PATCH] gpio: tegra186: Add support for T186 GPIO Thierry Reding <thierry.reding@gmail.com> - 2016-11-08 17:00 +0100
            Re: [PATCH] gpio: tegra186: Add support for T186 GPIO Stephen Warren <swarren@wwwdotorg.org> - 2016-11-08 17:50 +0100
              Re: [PATCH] gpio: tegra186: Add support for T186 GPIO Thierry Reding <thierry.reding@gmail.com> - 2016-11-08 19:00 +0100
    Re: [PATCH] gpio: tegra186: Add support for T186 GPIO Stephen Warren <swarren@wwwdotorg.org> - 2016-11-08 20:10 +0100

#1513798 — [PATCH] gpio: tegra186: Add support for T186 GPIO

Add GPIO driver for T186 based platforms.
Adds support for MAIN and AON GPIO's from T186.

Signed-off-by: Suresh Mangipudi <smangipudi@nvidia.com>
---
 drivers/gpio/Kconfig         |   8 +
 drivers/gpio/Makefile        |   1 +
 drivers/gpio/gpio-tegra186.c | 647 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 656 insertions(+)
 create mode 100644 drivers/gpio/gpio-tegra186.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index a9a1c8a..99aeded 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -409,6 +409,14 @@ config GPIO_TB10X
 	select GENERIC_IRQ_CHIP
 	select OF_GPIO
 
+config GPIO_TEGRA186
+	bool "NVIDIA Tegra186 GPIO support"
+	default ARCH_TEGRA
+	depends on ARCH_TEGRA || COMPILE_TEST
+	depends on OF_GPIO
+	help
+	 Support for the NVIDIA Tegra186 GPIO controller driver.
+
 config GPIO_TEGRA
 	bool "NVIDIA Tegra GPIO support"
 	default ARCH_TEGRA
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 8043a95..35ccc47 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -108,6 +108,7 @@ obj-$(CONFIG_GPIO_SYSCON)	+= gpio-syscon.o
 obj-$(CONFIG_GPIO_TB10X)	+= gpio-tb10x.o
 obj-$(CONFIG_GPIO_TC3589X)	+= gpio-tc3589x.o
 obj-$(CONFIG_GPIO_TEGRA)	+= gpio-tegra.o
+obj-$(CONFIG_GPIO_TEGRA186)	+= gpio-tegra186.o
 obj-$(CONFIG_GPIO_TIMBERDALE)	+= gpio-timberdale.o
 obj-$(CONFIG_GPIO_PALMAS)	+= gpio-palmas.o
 obj-$(CONFIG_GPIO_TPIC2810)	+= gpio-tpic2810.o
diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
new file mode 100644
index 0000000..c66600d
--- /dev/null
+++ b/drivers/gpio/gpio-tegra186.c
@@ -0,0 +1,647 @@
+/*
+ * GPIO driver for NVIDIA Tegra186
+ *
+ * Copyright (c) 2016, NVIDIA CORPORATION. All rights reserved.
+ *
+ * Author: Suresh Mangipudi <smangipudi@nvidia.com>
+ * Author: Laxman Dewangan <ldewangan@nvidia.com>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ */
+
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/irq.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/gpio.h>
+#include <linux/of_device.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/irqdomain.h>
+#include <linux/irqchip/chained_irq.h>
+#include <dt-bindings/gpio/tegra186-gpio.h>
+
+/* GPIO control registers */
+#define GPIO_ENB_CONFIG_REG			0x00
+#define GPIO_DBC_THRES_REG			0x04
+#define GPIO_INPUT_REG				0x08
+#define GPIO_OUT_CTRL_REG			0x0c
+#define GPIO_OUT_VAL_REG			0x10
+#define GPIO_INT_CLEAR_REG			0x14
+#define GPIO_REG_DIFF				0x20
+#define GPIO_INT_STATUS_OFFSET			0x100
+
+/* GPIO SCR registers */
+#define GPIO_SCR_REG				0x04
+#define GPIO_SCR_DIFF				0x08
+
+#define GPIO_INOUT_BIT				BIT(1)
+#define GPIO_TRG_TYPE_BIT(x)			((x) & 0x3)
+#define GPIO_TRG_TYPE_BIT_OFFSET		0x2
+#define GPIO_TRG_LVL_BIT			BIT(4)
+#define GPIO_DEB_FUNC_BIT			BIT(5)
+#define GPIO_INT_FUNC_BIT			BIT(6)
+
+#define GPIO_SCR_SEC_WEN			BIT(28)
+#define GPIO_SCR_SEC_REN			BIT(27)
+#define GPIO_SCR_SEC_G1W			BIT(9)
+#define GPIO_SCR_SEC_G1R			BIT(1)
+#define GPIO_FULL_ACCESS			(GPIO_SCR_SEC_WEN | \
+						 GPIO_SCR_SEC_REN | \
+						 GPIO_SCR_SEC_G1R | \
+						 GPIO_SCR_SEC_G1W)
+
+#define GPIO_INT_LVL_LEVEL_TRIGGER		0x1
+#define GPIO_INT_LVL_SINGLE_EDGE_TRIGGER	0x2
+#define GPIO_INT_LVL_BOTH_EDGE_TRIGGER		0x3
+
+#define TRIGGER_LEVEL_LOW			0x0
+#define TRIGGER_LEVEL_HIGH			0x1
+
+#define GPIO_STATUS_G1				0x04
+
+#define MAX_GPIO_CONTROLLERS			7
+#define MAX_GPIO_PORTS				8
+
+#define GPIO_PORT(g)				((g) >> 3)
+#define GPIO_PIN(g)				((g) & 0x7)
+
+struct tegra_gpio_port_soc_info {
+	const char *port_name;
+	int cont_id;
+	int port_index;
+	int valid_pins;
+	int scr_offset;
+	u32 reg_offset;
+};
+
+#define TEGRA_MAIN_GPIO_PORT_INFO(port, cid, cind, npins)	\
+[TEGRA_MAIN_GPIO_PORT_##port] = {				\
+		.port_name = #port,				\
+		.cont_id = cid,					\
+		.port_index = cind,				\
+		.valid_pins = npins,				\
+		.scr_offset = cid * 0x1000 + cind * 0x40,	\
+		.reg_offset = cid * 0x1000 + cind * 0x200,	\
+}
+
+#define TEGRA_AON_GPIO_PORT_INFO(port, cid, cind, npins)	\
+[TEGRA_AON_GPIO_PORT_##port] = {				\
+		.port_name = #port,				\
+		.cont_id = cid,					\
+		.port_index = cind,				\
+		.valid_pins = npins,				\
+		.scr_offset = cind * 0x40,			\
+		.reg_offset = cind * 0x200,			\
+}
+
+static struct tegra_gpio_port_soc_info tegra_main_gpio_cinfo[] = {
+	TEGRA_MAIN_GPIO_PORT_INFO(A, 2, 0, 7),
+	TEGRA_MAIN_GPIO_PORT_INFO(B, 3, 0, 7),
+	TEGRA_MAIN_GPIO_PORT_INFO(C, 3, 1, 7),
+	TEGRA_MAIN_GPIO_PORT_INFO(D, 3, 2, 6),
+	TEGRA_MAIN_GPIO_PORT_INFO(E, 2, 1, 8),
+	TEGRA_MAIN_GPIO_PORT_INFO(F, 2, 2, 6),
+	TEGRA_MAIN_GPIO_PORT_INFO(G, 4, 1, 6),
+	TEGRA_MAIN_GPIO_PORT_INFO(H, 1, 0, 7),
+	TEGRA_MAIN_GPIO_PORT_INFO(I, 0, 4, 8),
+	TEGRA_MAIN_GPIO_PORT_INFO(J, 5, 0, 8),
+	TEGRA_MAIN_GPIO_PORT_INFO(K, 5, 1, 1),
+	TEGRA_MAIN_GPIO_PORT_INFO(L, 1, 1, 8),
+	TEGRA_MAIN_GPIO_PORT_INFO(M, 5, 3, 6),
+	TEGRA_MAIN_GPIO_PORT_INFO(N, 0, 0, 7),
+	TEGRA_MAIN_GPIO_PORT_INFO(O, 0, 1, 4),
+	TEGRA_MAIN_GPIO_PORT_INFO(P, 4, 0, 7),
+	TEGRA_MAIN_GPIO_PORT_INFO(Q, 0, 2, 6),
+	TEGRA_MAIN_GPIO_PORT_INFO(R, 0, 5, 6),
+	TEGRA_MAIN_GPIO_PORT_INFO(T, 0, 3, 4),
+	TEGRA_MAIN_GPIO_PORT_INFO(X, 1, 2, 8),
+	TEGRA_MAIN_GPIO_PORT_INFO(Y, 1, 3, 7),
+	TEGRA_MAIN_GPIO_PORT_INFO(BB, 2, 3, 2),
+	TEGRA_MAIN_GPIO_PORT_INFO(CC, 5, 2, 4),
+};
+
+static struct tegra_gpio_port_soc_info tegra_aon_gpio_cinfo[] = {
+	TEGRA_AON_GPIO_PORT_INFO(S, 0, 1, 5),
+	TEGRA_AON_GPIO_PORT_INFO(U, 0, 2, 6),
+	TEGRA_AON_GPIO_PORT_INFO(V, 0, 4, 8),
+	TEGRA_AON_GPIO_PORT_INFO(W, 0, 5, 8),
+	TEGRA_AON_GPIO_PORT_INFO(Z, 0, 7, 4),
+	TEGRA_AON_GPIO_PORT_INFO(AA, 0, 6, 8),
+	TEGRA_AON_GPIO_PORT_INFO(EE, 0, 3, 3),
+	TEGRA_AON_GPIO_PORT_INFO(FF, 0, 0, 5),
+};
+
+struct tegra_gpio_info;
+
+struct tegra_gpio_soc_info {
+	const char *name;
+	const struct tegra_gpio_port_soc_info *port;
+	int nports;
+};
+
+struct tegra_gpio_controller {
+	int controller;
+	int irq;
+	struct tegra_gpio_info *tgi;
+};
+
+struct tegra_gpio_info {
+	struct device *dev;
+	int nbanks;
+	void __iomem *gpio_regs;
+	void __iomem *scr_regs;
+	struct irq_domain *irq_domain;
+	const struct tegra_gpio_soc_info *soc;
+	struct tegra_gpio_controller tg_contrlr[MAX_GPIO_CONTROLLERS];
+	struct gpio_chip gc;
+	struct irq_chip ic;
+};
+
+#define GPIO_CNTRL_REG(tgi, gpio, roffset)				    \
+	((tgi)->gpio_regs + (tgi)->soc->port[GPIO_PORT(gpio)].reg_offset + \
+	(GPIO_REG_DIFF * GPIO_PIN(gpio)) + (roffset))
+
+static u32 tegra_gpio_readl(struct tegra_gpio_info *tgi, u32 gpio,
+				   u32 reg_offset)
+{
+	return __raw_readl(GPIO_CNTRL_REG(tgi, gpio, reg_offset));
+}
+
+static void tegra_gpio_writel(struct tegra_gpio_info *tgi, u32 val,
+				     u32 gpio, u32 reg_offset)
+{
+	__raw_writel(val, GPIO_CNTRL_REG(tgi, gpio, reg_offset));
+}
+
+static void tegra_gpio_update(struct tegra_gpio_info *tgi, u32 gpio,
+				     u32 reg_offset,	u32 mask, u32 val)
+{
+	u32 rval;
+
+	rval = __raw_readl(GPIO_CNTRL_REG(tgi, gpio, reg_offset));
+	rval = (rval & ~mask) | (val & mask);
+	__raw_writel(rval, GPIO_CNTRL_REG(tgi, gpio, reg_offset));
+}
+
+/* This function will return if the GPIO is accessible by CPU */
+static bool gpio_is_accessible(struct tegra_gpio_info *tgi, u32 offset)
+{
+	int port = GPIO_PORT(offset);
+	int pin = GPIO_PIN(offset);
+	u32 val;
+	int cont_id;
+	u32 scr_offset = tgi->soc->port[port].scr_offset;
+
+	if (pin >= tgi->soc->port[port].valid_pins)
+		return false;
+
+	cont_id = tgi->soc->port[port].cont_id;
+	if (cont_id  < 0)
+		return false;
+
+	val = __raw_readl(tgi->scr_regs + scr_offset +
+			(pin * GPIO_SCR_DIFF) + GPIO_SCR_REG);
+
+	if ((val & GPIO_FULL_ACCESS) == GPIO_FULL_ACCESS)
+		return true;
+
+	return false;
+}
+
+static void tegra_gpio_enable(struct tegra_gpio_info *tgi, int gpio)
+{
+	tegra_gpio_update(tgi, gpio, GPIO_ENB_CONFIG_REG, 0x1, 0x1);
+}
+
+static void tegra_gpio_disable(struct tegra_gpio_info *tgi, int gpio)
+{
+	tegra_gpio_update(tgi, gpio, GPIO_ENB_CONFIG_REG, 0x1, 0x0);
+}
+
+static void tegra_gpio_free(struct gpio_chip *chip, unsigned int offset)
+{
+	struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
+
+	tegra_gpio_disable(tgi, offset);
+}
+
+static void tegra_gpio_set(struct gpio_chip *chip, unsigned int offset,
+			   int value)
+{
+	struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
+	u32 val = (value) ? 0x1 : 0x0;
+
+	tegra_gpio_writel(tgi, val, offset, GPIO_OUT_VAL_REG);
+	tegra_gpio_writel(tgi, 0, offset, GPIO_OUT_CTRL_REG);
+}
+
+static int tegra_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
+	u32 val;
+
+	val = tegra_gpio_readl(tgi, offset, GPIO_ENB_CONFIG_REG);
+	if (val & GPIO_INOUT_BIT)
+		return tegra_gpio_readl(tgi, offset, GPIO_OUT_VAL_REG) & 0x1;
+
+	return tegra_gpio_readl(tgi, offset, GPIO_INPUT_REG) & 0x1;
+}
+
+static void set_gpio_direction_mode(struct gpio_chip *chip, u32 offset,
+				    bool mode)
+{
+	struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
+	u32 val;
+
+	val = tegra_gpio_readl(tgi, offset, GPIO_ENB_CONFIG_REG);
+	if (mode)
+		val |= GPIO_INOUT_BIT;
+	else
+		val &= ~GPIO_INOUT_BIT;
+	tegra_gpio_writel(tgi, val, offset, GPIO_ENB_CONFIG_REG);
+}
+
+static int tegra_gpio_direction_input(struct gpio_chip *chip,
+				      unsigned int offset)
+{
+	struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
+
+	set_gpio_direction_mode(chip, offset, 0);
+	tegra_gpio_enable(tgi, offset);
+
+	return 0;
+}
+
+static int tegra_gpio_direction_output(struct gpio_chip *chip,
+				       unsigned int offset, int value)
+{
+	struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
+
+	tegra_gpio_set(chip, offset, value);
+	set_gpio_direction_mode(chip, offset, 1);
+	tegra_gpio_enable(tgi, offset);
+
+	return 0;
+}
+
+static int tegra_gpio_set_debounce(struct gpio_chip *chip, unsigned int offset,
+				   unsigned int debounce)
+{
+	struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
+	unsigned int dbc_ms = DIV_ROUND_UP(debounce, 1000);
+
+	tegra_gpio_update(tgi, offset, GPIO_ENB_CONFIG_REG, 0x1, 0x1);
+	tegra_gpio_update(tgi, offset, GPIO_DEB_FUNC_BIT, 0x5, 0x1);
+
+	/* Update debounce threshold */
+	tegra_gpio_writel(tgi, dbc_ms, offset, GPIO_DBC_THRES_REG);
+
+	return 0;
+}
+
+static int tegra_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
+{
+	struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
+	u32 val;
+
+	if (!gpio_is_accessible(tgi, offset))
+		return 0;
+
+	val = tegra_gpio_readl(tgi, offset, GPIO_OUT_CTRL_REG);
+
+	return (val & 0x1);
+}
+
+static int tegra_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
+{
+	struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
+
+	return irq_find_mapping(tgi->irq_domain, offset);
+}
+
+static void tegra_gpio_irq_ack(struct irq_data *d)
+{
+	struct tegra_gpio_controller *ctrlr = irq_data_get_irq_chip_data(d);
+
+	tegra_gpio_writel(ctrlr->tgi, 1, d->hwirq, GPIO_INT_CLEAR_REG);
+}
+
+static void tegra_gpio_irq_mask(struct irq_data *d)
+{
+	struct tegra_gpio_controller *c = irq_data_get_irq_chip_data(d);
+
+	tegra_gpio_update(c->tgi, d->hwirq, GPIO_ENB_CONFIG_REG,
+			  GPIO_INT_FUNC_BIT, 0);
+}
+
+static void tegra_gpio_irq_unmask(struct irq_data *d)
+{
+	struct tegra_gpio_controller *c = irq_data_get_irq_chip_data(d);
+
+	tegra_gpio_update(c->tgi, d->hwirq, GPIO_ENB_CONFIG_REG,
+			  GPIO_INT_FUNC_BIT, GPIO_INT_FUNC_BIT);
+}
+
+static int tegra_gpio_irq_set_type(struct irq_data *d, unsigned int type)
+{
+	struct tegra_gpio_controller *ctrlr = irq_data_get_irq_chip_data(d);
+	int gpio = d->hwirq;
+	u32 lvl_type;
+	u32 trg_type;
+	u32 val;
+
+	switch (type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_EDGE_RISING:
+		trg_type = TRIGGER_LEVEL_HIGH;
+		lvl_type = GPIO_INT_LVL_SINGLE_EDGE_TRIGGER;
+		break;
+
+	case IRQ_TYPE_EDGE_FALLING:
+		trg_type = TRIGGER_LEVEL_LOW;
+		lvl_type = GPIO_INT_LVL_SINGLE_EDGE_TRIGGER;
+		break;
+
+	case IRQ_TYPE_EDGE_BOTH:
+		lvl_type = GPIO_INT_LVL_BOTH_EDGE_TRIGGER;
+		trg_type = 0;
+		break;
+
+	case IRQ_TYPE_LEVEL_HIGH:
+		trg_type = TRIGGER_LEVEL_HIGH;
+		lvl_type = GPIO_INT_LVL_LEVEL_TRIGGER;
+		break;
+
+	case IRQ_TYPE_LEVEL_LOW:
+		trg_type = TRIGGER_LEVEL_LOW;
+		lvl_type = GPIO_INT_LVL_LEVEL_TRIGGER;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	trg_type = trg_type << 0x4;
+	lvl_type = lvl_type << 0x2;
+
+	/* Clear and Program the values */
+	val = tegra_gpio_readl(ctrlr->tgi, gpio, GPIO_ENB_CONFIG_REG);
+	val &= ~((0x3 << GPIO_TRG_TYPE_BIT_OFFSET) | (GPIO_TRG_LVL_BIT));
+	val |= trg_type | lvl_type;
+	tegra_gpio_writel(ctrlr->tgi, val, gpio, GPIO_ENB_CONFIG_REG);
+
+	tegra_gpio_enable(ctrlr->tgi, gpio);
+
+	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
+		irq_set_handler_locked(d, handle_level_irq);
+	else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
+		irq_set_handler_locked(d, handle_edge_irq);
+
+	return 0;
+}
+
+static void tegra_gpio_irq_handler(struct irq_desc *desc)
+{
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	struct tegra_gpio_controller *tg_cont = irq_desc_get_handler_data(desc);
+	struct tegra_gpio_info *tgi = tg_cont->tgi;
+	int pin;
+	int port;
+	u32 i;
+	unsigned long val;
+	u32 gpio;
+	u32 addr;
+	int port_map[MAX_GPIO_PORTS];
+
+	for (i = 0; i < MAX_GPIO_PORTS; ++i)
+		port_map[i] = -1;
+
+	for (i = 0; i < tgi->soc->nports; ++i) {
+		if (tgi->soc->port[i].cont_id == tg_cont->controller)
+			port_map[tgi->soc->port[i].port_index] = i;
+	}
+
+	chained_irq_enter(chip, desc);
+	for (i = 0; i < MAX_GPIO_PORTS; i++) {
+		port = port_map[i];
+		if (port == -1)
+			continue;
+
+		addr = tgi->soc->port[port].reg_offset;
+		val = __raw_readl(tg_cont->tgi->gpio_regs + addr +
+				GPIO_INT_STATUS_OFFSET + GPIO_STATUS_G1);
+		gpio = tgi->gc.base + (port * 8);
+		for_each_set_bit(pin, &val, 8)
+			generic_handle_irq(gpio_to_irq(gpio + pin));
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+#ifdef CONFIG_DEBUG_FS
+
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
+
+static int dbg_gpio_show(struct seq_file *s, void *unused)
+{
+	struct tegra_gpio_info *tgi = s->private;
+	int i;
+
+	seq_puts(s, "Port:Pin:ENB DBC IN OUT_CTRL OUT_VAL INT_CLR\n");
+	for (i = 0; i < tgi->gc.ngpio; i++) {
+		if (!gpio_is_accessible(tgi, i))
+			continue;
+		seq_printf(s, "%s:%d 0x%x 0x%x 0x%x 0x%x 0x%x 0x%x\n",
+			   tgi->soc->port[GPIO_PORT(i)].port_name, i % 8,
+			   tegra_gpio_readl(tgi, i, GPIO_ENB_CONFIG_REG),
+			   tegra_gpio_readl(tgi, i, GPIO_DBC_THRES_REG),
+			   tegra_gpio_readl(tgi, i, GPIO_INPUT_REG),
+			   tegra_gpio_readl(tgi, i, GPIO_OUT_CTRL_REG),
+			   tegra_gpio_readl(tgi, i, GPIO_OUT_VAL_REG),
+			   tegra_gpio_readl(tgi, i, GPIO_INT_CLEAR_REG));
+	}
+
+	return 0;
+}
+
+static int dbg_gpio_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, dbg_gpio_show, inode->i_private);
+}
+
+static const struct file_operations debug_fops = {
+	.open		= dbg_gpio_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
+static int __init tegra_gpio_debuginit(struct tegra_gpio_info *tgi)
+{
+	(void)debugfs_create_file(tgi->soc->name, 0444, NULL, tgi, &debug_fops);
+
+	return 0;
+}
+#else
+static int tegra_gpio_debuginit(struct tegra_gpio_info *tgi)
+{
+	return 0;
+}
+#endif
+
+static int tegra_gpio_probe(struct platform_device *pdev)
+{
+	struct tegra_gpio_info *tgi;
+	struct resource *res;
+	int bank;
+	int gpio;
+	int ret;
+
+	for (bank = 0;; bank++) {
+		res = platform_get_resource(pdev, IORESOURCE_IRQ, bank);
+		if (!res)
+			break;
+	}
+	if (!bank) {
+		dev_err(&pdev->dev, "No GPIO Controller found\n");
+		return -ENODEV;
+	}
+
+	tgi = devm_kzalloc(&pdev->dev, sizeof(*tgi), GFP_KERNEL);
+	if (!tgi)
+		return -ENOMEM;
+	tgi->dev = &pdev->dev;
+	tgi->nbanks = bank;
+	tgi->soc = of_device_get_match_data(&pdev->dev);
+
+	tgi->gc.label			= tgi->soc->name;
+	tgi->gc.free			= tegra_gpio_free;
+	tgi->gc.direction_input		= tegra_gpio_direction_input;
+	tgi->gc.get			= tegra_gpio_get;
+	tgi->gc.direction_output	= tegra_gpio_direction_output;
+	tgi->gc.set			= tegra_gpio_set;
+	tgi->gc.get_direction		= tegra_gpio_get_direction;
+	tgi->gc.to_irq			= tegra_gpio_to_irq;
+	tgi->gc.set_debounce		= tegra_gpio_set_debounce;
+	tgi->gc.base			= -1;
+	tgi->gc.ngpio			= tgi->soc->nports * 8;
+	tgi->gc.parent			= &pdev->dev;
+	tgi->gc.of_node			= pdev->dev.of_node;
+
+	tgi->ic.name			= tgi->soc->name;
+	tgi->ic.irq_ack			= tegra_gpio_irq_ack;
+	tgi->ic.irq_mask		= tegra_gpio_irq_mask;
+	tgi->ic.irq_unmask		= tegra_gpio_irq_unmask;
+	tgi->ic.irq_set_type		= tegra_gpio_irq_set_type;
+	tgi->ic.irq_shutdown		= tegra_gpio_irq_mask;
+	tgi->ic.irq_disable		= tegra_gpio_irq_mask;
+
+	platform_set_drvdata(pdev, tgi);
+
+	for (bank = 0; bank < tgi->nbanks; bank++) {
+		res = platform_get_resource(pdev, IORESOURCE_IRQ, bank);
+		tgi->tg_contrlr[bank].controller = bank;
+		tgi->tg_contrlr[bank].irq = res->start;
+		tgi->tg_contrlr[bank].tgi = tgi;
+	}
+
+	tgi->irq_domain = irq_domain_add_linear(pdev->dev.of_node,
+						tgi->gc.ngpio,
+						&irq_domain_simple_ops, NULL);
+	if (!tgi->irq_domain)
+		return -ENODEV;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "security");
+	if (!res) {
+		dev_err(&pdev->dev, "Missing security MEM resource\n");
+		return -ENODEV;
+	}
+	tgi->scr_regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(tgi->scr_regs)) {
+		ret = PTR_ERR(tgi->scr_regs);
+		dev_err(&pdev->dev, "Failed to iomap for security: %d\n", ret);
+		return ret;
+	}
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "gpio");
+	if (!res) {
+		dev_err(&pdev->dev, "Missing gpio MEM resource\n");
+		return -ENODEV;
+	}
+	tgi->gpio_regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(tgi->gpio_regs)) {
+		ret = PTR_ERR(tgi->gpio_regs);
+		dev_err(&pdev->dev, "Failed to iomap for gpio: %d\n", ret);
+		return ret;
+	}
+
+	ret = devm_gpiochip_add_data(&pdev->dev, &tgi->gc, tgi);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Could not register gpiochip, %d\n", ret);
+		return ret;
+	}
+
+	for (gpio = 0; gpio < tgi->gc.ngpio; gpio++) {
+		int irq = irq_create_mapping(tgi->irq_domain, gpio);
+		int cont_id = tgi->soc->port[GPIO_PORT(gpio)].cont_id;
+
+		if (gpio_is_accessible(tgi, gpio))
+			/* mask interrupts for this GPIO */
+			tegra_gpio_update(tgi, gpio, GPIO_ENB_CONFIG_REG,
+					  GPIO_INT_FUNC_BIT, 0);
+
+		irq_set_chip_data(irq, &tgi->tg_contrlr[cont_id]);
+		irq_set_chip_and_handler(irq, &tgi->ic, handle_simple_irq);
+	}
+
+	for (bank = 0; bank < tgi->nbanks; bank++)
+		irq_set_chained_handler_and_data(tgi->tg_contrlr[bank].irq,
+						 tegra_gpio_irq_handler,
+						 &tgi->tg_contrlr[bank]);
+
+	tegra_gpio_debuginit(tgi);
+
+	return 0;
+}
+
+static const struct tegra_gpio_soc_info t186_main_gpio_soc = {
+	.name = "tegra-main-gpio",
+	.port = tegra_main_gpio_cinfo,
+	.nports = ARRAY_SIZE(tegra_main_gpio_cinfo),
+};
+
+static const struct tegra_gpio_soc_info t186_aon_gpio_soc = {
+	.name = "tegra-aon-gpio",
+	.port = tegra_aon_gpio_cinfo,
+	.nports = ARRAY_SIZE(tegra_aon_gpio_cinfo),
+};
+
+static const struct of_device_id tegra_gpio_of_match[] = {
+	{ .compatible = "nvidia,tegra186-gpio", .data = &t186_main_gpio_soc},
+	{ .compatible = "nvidia,tegra186-gpio-aon", .data = &t186_aon_gpio_soc},
+	{ },
+};
+
+static struct platform_driver tegra_gpio_driver = {
+	.driver		= {
+		.name	= "tegra186-gpio",
+		.of_match_table = tegra_gpio_of_match,
+	},
+	.probe		= tegra_gpio_probe,
+};
+
+static int __init tegra_gpio_init(void)
+{
+	return platform_driver_register(&tegra_gpio_driver);
+}
+postcore_initcall(tegra_gpio_init);
+
+MODULE_AUTHOR("Suresh Mangipudi <smangipudi@nvidia.com>");
+MODULE_AUTHOR("Laxman Dewangan <ldewangan@nvidia.com>");
+MODULE_DESCRIPTION("NVIDIA Tegra186 GPIO driver");
+MODULE_LICENSE("GPL v2");
-- 
2.1.4

[toc] | [next] | [standalone]

#1515891

On Wed, Nov 2, 2016 at 11:48 AM, Suresh Mangipudi <smangipudi@nvidia.com> wrote:

> Add GPIO driver for T186 based platforms.
> Adds support for MAIN and AON GPIO's from T186.
>
> Signed-off-by: Suresh Mangipudi <smangipudi@nvidia.com>

Stephen/Thierry/Alexandre:
Can I get your review on this Tegra thing?

Yours,
Linus Walleij

[toc] | [prev] | [next] | [standalone]

#1516174

[Multipart message — attachments visible in raw view] — view raw

On Mon, Nov 07, 2016 at 08:53:37AM +0100, Linus Walleij wrote:
> On Wed, Nov 2, 2016 at 11:48 AM, Suresh Mangipudi <smangipudi@nvidia.com> wrote:
> 
> > Add GPIO driver for T186 based platforms.
> > Adds support for MAIN and AON GPIO's from T186.
> >
> > Signed-off-by: Suresh Mangipudi <smangipudi@nvidia.com>
> 
> Stephen/Thierry/Alexandre:
> Can I get your review on this Tegra thing?

Can we hold off on this for a bit? I've got my own implementation of
this in my Tegra186 tree because I thought nobody else was working on
it. From a brief look they seem mostly similar but there are a couple
of differences that I need to take a closer look at (and do some more
intensive testing on).

Thanks,
Thierry

[toc] | [prev] | [next] | [standalone]

#1516724

On Mon, Nov 7, 2016 at 5:21 AM, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Mon, Nov 07, 2016 at 08:53:37AM +0100, Linus Walleij wrote:
>> On Wed, Nov 2, 2016 at 11:48 AM, Suresh Mangipudi <smangipudi@nvidia.com> wrote:
>>
>> > Add GPIO driver for T186 based platforms.
>> > Adds support for MAIN and AON GPIO's from T186.
>> >
>> > Signed-off-by: Suresh Mangipudi <smangipudi@nvidia.com>
>>
>> Stephen/Thierry/Alexandre:
>> Can I get your review on this Tegra thing?
>
> Can we hold off on this for a bit? I've got my own implementation of
> this in my Tegra186 tree because I thought nobody else was working on
> it. From a brief look they seem mostly similar but there are a couple
> of differences that I need to take a closer look at (and do some more
> intensive testing on).

Be careful about discouraging other developers internally from
participating upstream, Thierry.

Please don't become a bottleneck for your company's contributions.
Rewriting stuff on your own isn't a scalable model.


-Olof

[toc] | [prev] | [next] | [standalone]

#1517314

[Multipart message — attachments visible in raw view] — view raw

On Mon, Nov 07, 2016 at 05:42:33PM -0800, Olof Johansson wrote:
> On Mon, Nov 7, 2016 at 5:21 AM, Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Mon, Nov 07, 2016 at 08:53:37AM +0100, Linus Walleij wrote:
> >> On Wed, Nov 2, 2016 at 11:48 AM, Suresh Mangipudi <smangipudi@nvidia.com> wrote:
> >>
> >> > Add GPIO driver for T186 based platforms.
> >> > Adds support for MAIN and AON GPIO's from T186.
> >> >
> >> > Signed-off-by: Suresh Mangipudi <smangipudi@nvidia.com>
> >>
> >> Stephen/Thierry/Alexandre:
> >> Can I get your review on this Tegra thing?
> >
> > Can we hold off on this for a bit? I've got my own implementation of
> > this in my Tegra186 tree because I thought nobody else was working on
> > it. From a brief look they seem mostly similar but there are a couple
> > of differences that I need to take a closer look at (and do some more
> > intensive testing on).
> 
> Be careful about discouraging other developers internally from
> participating upstream, Thierry.

That was certainly not my intention. I do welcome any contributions,
internal or external.

> Please don't become a bottleneck for your company's contributions.
> Rewriting stuff on your own isn't a scalable model.

Like I said, I didn't rewrite this, I merely wrote the GPIO driver
because there wasn't one at the time and I wasn't aware of anyone else
working on one. Waiting for a GPIO driver to emerge would've prevented
me from making progress on other fronts.

Thierry

[toc] | [prev] | [next] | [standalone]

#1517378

On 11/08/2016 08:55 AM, Thierry Reding wrote:
> On Mon, Nov 07, 2016 at 05:42:33PM -0800, Olof Johansson wrote:
>> On Mon, Nov 7, 2016 at 5:21 AM, Thierry Reding <thierry.reding@gmail.com> wrote:
>>> On Mon, Nov 07, 2016 at 08:53:37AM +0100, Linus Walleij wrote:
>>>> On Wed, Nov 2, 2016 at 11:48 AM, Suresh Mangipudi <smangipudi@nvidia.com> wrote:
>>>>
>>>>> Add GPIO driver for T186 based platforms.
>>>>> Adds support for MAIN and AON GPIO's from T186.
>>>>>
>>>>> Signed-off-by: Suresh Mangipudi <smangipudi@nvidia.com>
>>>>
>>>> Stephen/Thierry/Alexandre:
>>>> Can I get your review on this Tegra thing?
>>>
>>> Can we hold off on this for a bit? I've got my own implementation of
>>> this in my Tegra186 tree because I thought nobody else was working on
>>> it. From a brief look they seem mostly similar but there are a couple
>>> of differences that I need to take a closer look at (and do some more
>>> intensive testing on).
>>
>> Be careful about discouraging other developers internally from
>> participating upstream, Thierry.
>
> That was certainly not my intention. I do welcome any contributions,
> internal or external.
>
>> Please don't become a bottleneck for your company's contributions.
>> Rewriting stuff on your own isn't a scalable model.
>
> Like I said, I didn't rewrite this, I merely wrote the GPIO driver
> because there wasn't one at the time and I wasn't aware of anyone else
> working on one. Waiting for a GPIO driver to emerge would've prevented
> me from making progress on other fronts.

One issue here is that there are lots of patches destined for upstream 
in your own personal tree, but they aren't actually upstream yet. I 
think pushing more of your work into linux-next (and further upstream) 
faster would help people out. linux-next and other "standard" repos 
should be easily discoverable by anyway following any upstreaming 
HOWTOs, but those HOWTOs aren't going to mention your personal trees, so 
patches there effectively don't exist. Making the already extant work 
more discoverable will help prevent people duplicating work.

Related to this, I've been waiting rather a while for the Tegra186 DT 
binding patches I sent to be applied. I'd love to see them go in ASAP 
even if there's no kernel driver behind them. The bindings have been 
reviewed, ack'd, and they're in use in U-Boot already.

A thought on Tegra186: For a older supported SoCs, we obviously don't 
want to push changes upstream that aren't too well baked. However, for a 
new SoC that doesn't work yet, I'm tend to prioritize getting as much 
early work upstream as fast as possible (to try and unblock people 
working in other areas) over getting those patches perfect first. 
Release early, release often will help unblock people and parallelize 
work. Pipeline your own work rather than batching it all up to release 
at once.

P.S. don't take this email too personally or anything; I'm not trying to 
be complaining/critical or anything like that. It's just a few mild 
thoughts.

[toc] | [prev] | [next] | [standalone]

#1517431

[Multipart message — attachments visible in raw view] — view raw

On Tue, Nov 08, 2016 at 09:49:27AM -0700, Stephen Warren wrote:
> On 11/08/2016 08:55 AM, Thierry Reding wrote:
> > On Mon, Nov 07, 2016 at 05:42:33PM -0800, Olof Johansson wrote:
> > > On Mon, Nov 7, 2016 at 5:21 AM, Thierry Reding <thierry.reding@gmail.com> wrote:
> > > > On Mon, Nov 07, 2016 at 08:53:37AM +0100, Linus Walleij wrote:
> > > > > On Wed, Nov 2, 2016 at 11:48 AM, Suresh Mangipudi <smangipudi@nvidia.com> wrote:
> > > > > 
> > > > > > Add GPIO driver for T186 based platforms.
> > > > > > Adds support for MAIN and AON GPIO's from T186.
> > > > > > 
> > > > > > Signed-off-by: Suresh Mangipudi <smangipudi@nvidia.com>
> > > > > 
> > > > > Stephen/Thierry/Alexandre:
> > > > > Can I get your review on this Tegra thing?
> > > > 
> > > > Can we hold off on this for a bit? I've got my own implementation of
> > > > this in my Tegra186 tree because I thought nobody else was working on
> > > > it. From a brief look they seem mostly similar but there are a couple
> > > > of differences that I need to take a closer look at (and do some more
> > > > intensive testing on).
> > > 
> > > Be careful about discouraging other developers internally from
> > > participating upstream, Thierry.
> > 
> > That was certainly not my intention. I do welcome any contributions,
> > internal or external.
> > 
> > > Please don't become a bottleneck for your company's contributions.
> > > Rewriting stuff on your own isn't a scalable model.
> > 
> > Like I said, I didn't rewrite this, I merely wrote the GPIO driver
> > because there wasn't one at the time and I wasn't aware of anyone else
> > working on one. Waiting for a GPIO driver to emerge would've prevented
> > me from making progress on other fronts.
> 
> One issue here is that there are lots of patches destined for upstream in
> your own personal tree, but they aren't actually upstream yet. I think
> pushing more of your work into linux-next (and further upstream) faster
> would help people out. linux-next and other "standard" repos should be
> easily discoverable by anyway following any upstreaming HOWTOs, but those
> HOWTOs aren't going to mention your personal trees, so patches there
> effectively don't exist. Making the already extant work more discoverable
> will help prevent people duplicating work.

I had assumed that I had properly communicated what the canonical
temporary location for Tegra186 patches would be, but you're right that
it's probably easy to miss, and linux-next would be a more obvious
target.

> Related to this, I've been waiting rather a while for the Tegra186 DT
> binding patches I sent to be applied. I'd love to see them go in ASAP even
> if there's no kernel driver behind them. The bindings have been reviewed,
> ack'd, and they're in use in U-Boot already.

It's true, I've been promising this for weeks now. I'll get around to it
within the week. Please do prod me in case I don't. I promise I won't
get mad =)

> A thought on Tegra186: For a older supported SoCs, we obviously don't want
> to push changes upstream that aren't too well baked. However, for a new SoC
> that doesn't work yet, I'm tend to prioritize getting as much early work
> upstream as fast as possible (to try and unblock people working in other
> areas) over getting those patches perfect first. Release early, release
> often will help unblock people and parallelize work. Pipeline your own work
> rather than batching it all up to release at once.

I'm always hesitant to merge code that isn't functional or tested, but
perhaps I'm being a little overzealous here, and I'm evidently not doing
so great, so let me try to be more aggressive.

> P.S. don't take this email too personally or anything; I'm not trying to be
> complaining/critical or anything like that. It's just a few mild thoughts.

No offense taken, thanks for being constructive about it.

Thierry

[toc] | [prev] | [next] | [standalone]

#1517497

On 11/02/2016 04:48 AM, Suresh Mangipudi wrote:
> Add GPIO driver for T186 based platforms.
> Adds support for MAIN and AON GPIO's from T186.

I'm not sure how you/Thierry will approach merging this with the other 
GPIO driver he has, but here's a very quick review of this one in case 
it's useful.

> diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c

> +#define TEGRA_MAIN_GPIO_PORT_INFO(port, cid, cind, npins)	\

A comment indicating what "cid" and "cind" mean (and perhaps the other 
parameters too) would be useful.

A brief description of the overall register layout and structure and 
differences between the MAIN/AON controllers would be useful.

> +[TEGRA_MAIN_GPIO_PORT_##port] = {				\
> +		.port_name = #port,				\
> +		.cont_id = cid,					\
> +		.port_index = cind,				\

Why not make the parameter names match the field names they're assigned to?

> +		.valid_pins = npins,				\
> +		.scr_offset = cid * 0x1000 + cind * 0x40,	\
> +		.reg_offset = cid * 0x1000 + cind * 0x200,	\

While C does define operator precedence rules that make that expression 
OK, I personally prefer using () to make it explict:

+		.reg_offset = (cid * 0x1000) + (cind * 0x200),	\

That way, the reader doesn't have to think/remember so much.

Also, if these values can be calculated based on .cont_id and 
.port_index, I wonder why we need to pre-calculate them here and/or what 
else could be pre-calculated from .cont_id/.port_index? I'm tend to 
either (a) just store .cont_id and .port_index and calculate everything 
from them always, or (b) store just derived data and not both storing 
.cont_id/.port_index.

> +static struct tegra_gpio_port_soc_info tegra_main_gpio_cinfo[] = {
> +	TEGRA_MAIN_GPIO_PORT_INFO(A, 2, 0, 7),
> +	TEGRA_MAIN_GPIO_PORT_INFO(B, 3, 0, 7),

I assume the entries in this file must be in the same order as the DT 
binding port IDs? A comment to that effect would be useful.

> +struct tegra_gpio_info;
> +
> +struct tegra_gpio_soc_info {
> +	const char *name;
> +	const struct tegra_gpio_port_soc_info *port;
> +	int nports;
> +};

This isn't information about an SoC; it's information about a 
controller, and there are 2 controllers within Tegra186. Rename to 
tegra_gpio_ctlr_info?

> +struct tegra_gpio_controller {
> +	int controller;
> +	int irq;
> +	struct tegra_gpio_info *tgi;
> +};
> +
> +struct tegra_gpio_info {

Is this structure per-bank/-port? Also, "info" seems to be used above 
for static configuration info/data. I think this should be called 
"tegra_gpio_port"?

> +	struct device *dev;
> +	int nbanks;
> +	void __iomem *gpio_regs;
> +	void __iomem *scr_regs;
> +	struct irq_domain *irq_domain;
> +	const struct tegra_gpio_soc_info *soc;
> +	struct tegra_gpio_controller tg_contrlr[MAX_GPIO_CONTROLLERS];
> +	struct gpio_chip gc;
> +	struct irq_chip ic;
> +};

> +#define GPIO_CNTRL_REG(tgi, gpio, roffset)				    \
> +	((tgi)->gpio_regs + (tgi)->soc->port[GPIO_PORT(gpio)].reg_offset + \
> +	(GPIO_REG_DIFF * GPIO_PIN(gpio)) + (roffset))

Writing a static inline function would make formatting and type safety 
easier.

> +static void tegra_gpio_update(struct tegra_gpio_info *tgi, u32 gpio,
> +				     u32 reg_offset,	u32 mask, u32 val)
> +{
> +	u32 rval;
> +
> +	rval = __raw_readl(GPIO_CNTRL_REG(tgi, gpio, reg_offset));
> +	rval = (rval & ~mask) | (val & mask);
> +	__raw_writel(rval, GPIO_CNTRL_REG(tgi, gpio, reg_offset));
> +}

If you use a regmap object rather than a raw MMIO pointer, I believe 
there's already a function that does this read-modify-write.

> +/* This function will return if the GPIO is accessible by CPU */
> +static bool gpio_is_accessible(struct tegra_gpio_info *tgi, u32 offset)

I'd prefer all functions use the same name prefix. "tegra_gpio" seems to 
be used so far. Actually, given there's already an existing Tegra GPIO 
driver, and this driver covers the new controller(s) in Tegra186, I'd 
prefer everything be named tegra186_gpio_xxx.

> +       if (cont_id  < 0)
> +               return false;

That should trigger a checkpatch error due to the presence of two spaces 
in the expression. Try running checkpatch and fixing any issues.

> +	val = __raw_readl(tgi->scr_regs + scr_offset +
> +			(pin * GPIO_SCR_DIFF) + GPIO_SCR_REG);
> +
> +	if ((val & GPIO_FULL_ACCESS) == GPIO_FULL_ACCESS)
> +		return true;

I'm not entirely convinced about this. It's possible to have only read 
or only write access. I believe the CPU can be assigned to an arbitrary 
bus master group, whereas the value of GPIO_FULL_ACCESS assumes it's on 
group 1. Do we actually need this function at all except for debug? I'd 
be tempted to just drop it and let all GPIO accesses be attempted. If 
the SCR is configured such that the CPU doesn't have access, writes 
should be ignored and reads return valid dummy data. That seems fine to me.

Also, this function isn't consistently used by all client-callable APIs. 
I'd expect it to be used from every function that's accessible via a 
function pointer in struct gpio_chip, if it's useful.

> +static void tegra_gpio_set(struct gpio_chip *chip, unsigned int offset,
> +			   int value)

> +	tegra_gpio_writel(tgi, val, offset, GPIO_OUT_VAL_REG);
> +	tegra_gpio_writel(tgi, 0, offset, GPIO_OUT_CTRL_REG);

Shouldn't this function *just* set the output value; any other setup 
should be done solely as part of direction_input/direction_output?

> +static int tegra_gpio_irq_set_type(struct irq_data *d, unsigned int type)

> +	tegra_gpio_enable(ctrlr->tgi, gpio);

Shouldn't this only happen when the client actually calls enable/disable?

> +	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
> +		irq_set_handler_locked(d, handle_level_irq);
> +	else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
> +		irq_set_handler_locked(d, handle_edge_irq);

Shouldn't the handler be set before the IRQ is enabled?

> +static void tegra_gpio_irq_handler(struct irq_desc *desc)

> +	for (i = 0; i < MAX_GPIO_PORTS; ++i)
> +		port_map[i] = -1;
> +
> +	for (i = 0; i < tgi->soc->nports; ++i) {
> +		if (tgi->soc->port[i].cont_id == tg_cont->controller)
> +			port_map[tgi->soc->port[i].port_index] = i;
> +	}

I would have expected the code to use simple math here (iterate over all 
ports in the controller) rather than creating some kind of 
lookup/mapping table.

> +	chained_irq_enter(chip, desc);
> +	for (i = 0; i < MAX_GPIO_PORTS; i++) {
> +		port = port_map[i];
> +		if (port == -1)
> +			continue;
> +
> +		addr = tgi->soc->port[port].reg_offset;
> +		val = __raw_readl(tg_cont->tgi->gpio_regs + addr +
> +				GPIO_INT_STATUS_OFFSET + GPIO_STATUS_G1);
> +		gpio = tgi->gc.base + (port * 8);
> +		for_each_set_bit(pin, &val, 8)
> +			generic_handle_irq(gpio_to_irq(gpio + pin));

For edge-sensitive IRQs, doesn't the status need to be cleared before 
calling the handler, so that (a) the latched status is cleared, (b) if a 
new edge occurs after this point, that fact is recorded and the new IRQ 
handled?

> +#ifdef CONFIG_DEBUG_FS

Using a regmap might give you some of the debug logic for free.

> +static int tegra_gpio_probe(struct platform_device *pdev)
> +{
> +	struct tegra_gpio_info *tgi;
> +	struct resource *res;
> +	int bank;
> +	int gpio;
> +	int ret;
> +
> +	for (bank = 0;; bank++) {
> +		res = platform_get_resource(pdev, IORESOURCE_IRQ, bank);
> +		if (!res)
> +			break;
> +	}
> +	if (!bank) {
> +		dev_err(&pdev->dev, "No GPIO Controller found\n");
> +		return -ENODEV;
> +	}
...
 > +	tgi->nbanks = bank;

There should be a fixed number of IRQs in DT based on the controller 
definition; the value shouldn't be variable.

See Documentation/devicetree/bindings/gpio/nvidia,tegra186-gpio.txt

> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/gpio/nvidia,tegra186-gpio.txt

The U-Boot Tegra186 GPIO driver might also be useful as a reference to 
how to use the DT binding and structure the driver:

> http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/gpio/tegra186_gpio.c;h=1c681514db9f50e61a9db041ac2067c084db494c;hb=HEAD

> +	tgi->gc.ngpio			= tgi->soc->nports * 8;

This will leave some gaps in the GPIO numbering, since not all ports 
have 8 GPIOs. I think this is the correct thing to do, but IIRC Thierry 
found this caused some issues in the GPIO core since it attempts to 
query initial status of each GPIO. Did you see this issue during testing?

> +static int __init tegra_gpio_init(void)
> +{
> +	return platform_driver_register(&tegra_gpio_driver);
> +}
> +postcore_initcall(tegra_gpio_init);

I would have expected everything to use module_initcall() these days.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web