[v2] gpio: Add Tegra186 support

Submitted by Thierry Reding on March 10, 2017, 4:26 p.m.

Details

Message ID 20170310162629.31455-1-thierry.reding@gmail.com
State Superseded
Headers show

Commit Message

Thierry Reding March 10, 2017, 4:26 p.m.
From: Thierry Reding <treding@nvidia.com>

Tegra186 has two GPIO controllers that are largely register compatible
between one another but are completely different from the controller
found on earlier generations.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v2:
- add pin names to allow easy lookup using the chardev interface
- distinguish AON and main GPIO controllers by label
- use gpiochip_get_data() instead of container_of()
- use C99 initializers

Hi Linus,

This addresses all comments I got on the last revision, except for your
request to use gpiolib's IRQ chip helpers. I investigated using it with
the driver but still can't figure out how it's supposed to work given
our requirement to service multiple parent interrupts.

Thierry

 drivers/gpio/Kconfig         |   8 +
 drivers/gpio/Makefile        |   1 +
 drivers/gpio/gpio-tegra186.c | 636 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 645 insertions(+)
 create mode 100644 drivers/gpio/gpio-tegra186.c

Comments

Thierry Reding March 13, 2017, 7:06 a.m.
On Fri, Mar 10, 2017 at 05:26:29PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Tegra186 has two GPIO controllers that are largely register compatible
> between one another but are completely different from the controller
> found on earlier generations.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v2:
> - add pin names to allow easy lookup using the chardev interface

By the way, I tested this using Bartosz Gołaszewski's excellent libgpiod
that provides a really nice set of userspace utilities to detect and
manipulate GPIO chips:

	https://github.com/brgl/libgpiod

So for userspace access to the Tegra GPIOs we can now run something like
this in a shell script:

	#!/bin/sh
	gpio=$(gpiofind PCC.00)
	gpioget $gpio

There's a bunch of other features that the library supports. I'm glad we
finally have a standard set of tools to deal with GPIOs in userspace. It
seems the internet is full of libraries that deal with the GPIO sysfs
interface, but hopefully we can standardize on one library and the tools
for the chardev interface.

Thierry
Laxman Dewangan March 13, 2017, 4:22 p.m.
On Friday 10 March 2017 09:56 PM, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> Tegra186 has two GPIO controllers that are largely register compatible
> between one another but are completely different from the controller
> found on earlier generations.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v2:
> - add pin names to allow easy lookup using the chardev interface
> - distinguish AON and main GPIO controllers by label
> - use gpiochip_get_data() instead of container_of()
> - use C99 initializers
>

Overall it looks fine to me.
Only thing I did not like is the of_xlate manipulation where the gpio 
number is translated to another number for register access.
The reason is that it complicate in debugging when we instrument the 
callbacks for some gpios and this is very common in our debugging.

I still request here to use simple mapping instead of complicating.

Otherwise:
Acked-by: Laxman Dewangan <ldewangan@nvidia.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thierry Reding March 13, 2017, 5:44 p.m.
On Mon, Mar 13, 2017 at 09:52:04PM +0530, Laxman Dewangan wrote:
> 
> On Friday 10 March 2017 09:56 PM, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Tegra186 has two GPIO controllers that are largely register compatible
> > between one another but are completely different from the controller
> > found on earlier generations.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > Changes in v2:
> > - add pin names to allow easy lookup using the chardev interface
> > - distinguish AON and main GPIO controllers by label
> > - use gpiochip_get_data() instead of container_of()
> > - use C99 initializers
> > 
> 
> Overall it looks fine to me.
> Only thing I did not like is the of_xlate manipulation where the gpio number
> is translated to another number for register access.
> The reason is that it complicate in debugging when we instrument the
> callbacks for some gpios and this is very common in our debugging.
> 
> I still request here to use simple mapping instead of complicating.

I fail to see how this is complicating things. Yes, you can no longer
directly map from an offset to the port/index, but since we have a name
associated with each pin, we can simply print the name if we want to
know what pin we're dealing with.

So you could simply do something like this:

	static int tegra186_gpio_get_direction(struct gpio_chip *chip,
					       unsigned int offset)
	{
		struct gpio_desc *desc = gpiochip_get_desc(chip, offset);
		...
		dev_dbg(chip->parent, "GPIO %s (%s)\n", desc->name, desc->label);
		...
	}

The alternative that you're talking about would be something like this:

	static int tegra186_gpio_get_direction(struct gpio_chip *chip,
					       unsigned int offset)
	{
		struct tegra_gpio_port_soc_info *info;
		unsigned int port = GPIO_PORT(offset);
		unsigned int pin = GPIO_PIN(offset);
		...
		info = tgi->soc->port[port];
		...
		dev_dbg(chip->parent, "GPIO P%s.%02u\n", info->port_name, pin);
		...
	}

And with the above you'd either duplicate the logic that constructs the
pin name: once in the instrumentation code and once in the code that
sets up the pin names. Or you could decide not to set up any pin names
at all, in which case userspace would need to deal with the numbering
schemes itself, which makes things really complicated, especially when
we start supporting multiple SoC families.

The proposal in this patch is really quite elegant, if I may say so, in
that individual pins are identified by their name. Userspace can query
the list and get only the ones that actually exist. The wholes in the
GPIO range are not there, so there's not even a chance of userspace
messing with non-existing GPIOs.

Similarly the kernel has access to the name of these GPIOs, so the same
"string representation" can be used as for userspace.

I think this makes the driver very consistent across all use-cases. The
only remaining downside I see is that we're moving away from the 8-pins
per port from earlier chips. But I actually see that as a feature, even
something that we should backport to older chips.

The only reason why the driver for older chips works is because the
registers are laid out such that the pins of a port share one register
block, so if a non-existing GPIO is accessed, we'd be accessing a non-
existing field in an existing register, which seems to be less of an
issue than accessing a non-existing register.

Thierry
Linus Walleij March 14, 2017, 3:20 p.m.
On Mon, Mar 13, 2017 at 8:06 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:

> By the way, I tested this using Bartosz Gołaszewski's excellent libgpiod
> that provides a really nice set of userspace utilities to detect and
> manipulate GPIO chips:
>
>         https://github.com/brgl/libgpiod
>
> So for userspace access to the Tegra GPIOs we can now run something like
> this in a shell script:
>
>         #!/bin/sh
>         gpio=$(gpiofind PCC.00)
>         gpioget $gpio
>
> There's a bunch of other features that the library supports. I'm glad we
> finally have a standard set of tools to deal with GPIOs in userspace.

I share your high appraisal of this library, Bartosz work is really important
for consolidating this mess to something we can contain.

> It
> seems the internet is full of libraries that deal with the GPIO sysfs
> interface,

I do not advocate Internet censorship, but ... ;)

The world should be purged from them all.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thierry Reding March 31, 2017, 1:10 p.m.
On Fri, Mar 10, 2017 at 05:26:29PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Tegra186 has two GPIO controllers that are largely register compatible
> between one another but are completely different from the controller
> found on earlier generations.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v2:
> - add pin names to allow easy lookup using the chardev interface
> - distinguish AON and main GPIO controllers by label
> - use gpiochip_get_data() instead of container_of()
> - use C99 initializers
> 
> Hi Linus,
> 
> This addresses all comments I got on the last revision, except for your
> request to use gpiolib's IRQ chip helpers. I investigated using it with
> the driver but still can't figure out how it's supposed to work given
> our requirement to service multiple parent interrupts.
> 
> Thierry
> 
>  drivers/gpio/Kconfig         |   8 +
>  drivers/gpio/Makefile        |   1 +
>  drivers/gpio/gpio-tegra186.c | 636 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 645 insertions(+)
>  create mode 100644 drivers/gpio/gpio-tegra186.c

Hi Linus,

any objections to merging this for v4.12? I know Laxman has some
reservations about the internal numbering, but I hope my explanation in
the other subthread shows that this is the simplest way to achieve what
we want with the least amount of surprises.

Thierry

> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 05043071fc98..f74b61cfd113 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -431,6 +431,14 @@ config GPIO_TEGRA
>  	help
>  	  Say yes here to support GPIO pins on NVIDIA Tegra SoCs.
>  
> +config GPIO_TEGRA186
> +	tristate "NVIDIA Tegra186 GPIO support"
> +	default ARCH_TEGRA_186_SOC
> +	depends on ARCH_TEGRA_186_SOC || COMPILE_TEST
> +	depends on OF_GPIO
> +	help
> +	  Say yes here to support GPIO pins on NVIDIA Tegra186 SoCs.
> +
>  config GPIO_TS4800
>  	tristate "TS-4800 DIO blocks and compatibles"
>  	depends on OF_GPIO
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index becb96c724fe..cad0eb2c3531 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -111,6 +111,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 000000000000..87e02d9165d8
> --- /dev/null
> +++ b/drivers/gpio/gpio-tegra186.c
> @@ -0,0 +1,636 @@
> +/*
> + * Copyright (c) 2016-2017 NVIDIA Corporation
> + *
> + * Author: Thierry Reding <treding@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/gpio/driver.h>
> +#include <linux/gpio.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +
> +#include <dt-bindings/gpio/tegra186-gpio.h>
> +
> +#define TEGRA186_GPIO_ENABLE_CONFIG 0x00
> +#define  TEGRA186_GPIO_ENABLE_CONFIG_ENABLE BIT(0)
> +#define  TEGRA186_GPIO_ENABLE_CONFIG_OUT BIT(1)
> +#define  TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_TYPE_NONE (0x0 << 2)
> +#define  TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_TYPE_LEVEL (0x1 << 2)
> +#define  TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_TYPE_SINGLE_EDGE (0x2 << 2)
> +#define  TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_TYPE_DOUBLE_EDGE (0x3 << 2)
> +#define  TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_TYPE_MASK (0x3 << 2)
> +#define  TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_LEVEL BIT(4)
> +#define  TEGRA186_GPIO_ENABLE_CONFIG_INTERRUPT BIT(6)
> +
> +#define TEGRA186_GPIO_DEBOUNCE_CONTROL 0x04
> +#define  TEGRA186_GPIO_DEBOUNCE_CONTROL_THRESHOLD(x) ((x) & 0xff)
> +
> +#define TEGRA186_GPIO_INPUT 0x08
> +#define  TEGRA186_GPIO_INPUT_HIGH BIT(0)
> +
> +#define TEGRA186_GPIO_OUTPUT_CONTROL 0x0c
> +#define  TEGRA186_GPIO_OUTPUT_CONTROL_FLOATED BIT(0)
> +
> +#define TEGRA186_GPIO_OUTPUT_VALUE 0x10
> +#define  TEGRA186_GPIO_OUTPUT_VALUE_HIGH BIT(0)
> +
> +#define TEGRA186_GPIO_INTERRUPT_CLEAR 0x14
> +
> +#define TEGRA186_GPIO_INTERRUPT_STATUS(x) (0x100 + (x) * 4)
> +
> +struct tegra_gpio_port {
> +	const char *name;
> +	unsigned int offset;
> +	unsigned int pins;
> +};
> +
> +struct tegra_gpio_soc {
> +	const struct tegra_gpio_port *ports;
> +	unsigned int num_ports;
> +	const char *name;
> +};
> +
> +struct tegra_gpio {
> +	struct gpio_chip gpio;
> +	struct irq_chip intc;
> +	unsigned int num_irq;
> +	unsigned int *irq;
> +
> +	const struct tegra_gpio_soc *soc;
> +
> +	void __iomem *base;
> +
> +	struct irq_domain *domain;
> +};
> +
> +static const struct tegra_gpio_port *
> +tegra186_gpio_get_port(struct tegra_gpio *gpio, unsigned int *pin)
> +{
> +	unsigned int start = 0, i;
> +
> +	for (i = 0; i < gpio->soc->num_ports; i++) {
> +		const struct tegra_gpio_port *port = &gpio->soc->ports[i];
> +
> +		if (*pin >= start && *pin < start + port->pins) {
> +			*pin -= start;
> +			return port;
> +		}
> +
> +		start += port->pins;
> +	}
> +
> +	return NULL;
> +}
> +
> +static void __iomem *tegra186_gpio_get_base(struct tegra_gpio *gpio,
> +					    unsigned int pin)
> +{
> +	const struct tegra_gpio_port *port;
> +
> +	port = tegra186_gpio_get_port(gpio, &pin);
> +	if (!port)
> +		return NULL;
> +
> +	return gpio->base + port->offset + pin * 0x20;
> +}
> +
> +static int tegra186_gpio_get_direction(struct gpio_chip *chip,
> +				       unsigned int offset)
> +{
> +	struct tegra_gpio *gpio = gpiochip_get_data(chip);
> +	void __iomem *base;
> +	u32 value;
> +
> +	base = tegra186_gpio_get_base(gpio, offset);
> +	if (WARN_ON(base == NULL))
> +		return -ENODEV;
> +
> +	value = readl(base + TEGRA186_GPIO_ENABLE_CONFIG);
> +	if (value & TEGRA186_GPIO_ENABLE_CONFIG_OUT)
> +		return GPIOF_DIR_OUT;
> +
> +	return GPIOF_DIR_IN;
> +}
> +
> +static int tegra186_gpio_direction_input(struct gpio_chip *chip,
> +					 unsigned int offset)
> +{
> +	struct tegra_gpio *gpio = gpiochip_get_data(chip);
> +	void __iomem *base;
> +	u32 value;
> +
> +	base = tegra186_gpio_get_base(gpio, offset);
> +	if (WARN_ON(base == NULL))
> +		return -ENODEV;
> +
> +	value = readl(base + TEGRA186_GPIO_OUTPUT_CONTROL);
> +	value |= TEGRA186_GPIO_OUTPUT_CONTROL_FLOATED;
> +	writel(value, base + TEGRA186_GPIO_OUTPUT_CONTROL);
> +
> +	value = readl(base + TEGRA186_GPIO_ENABLE_CONFIG);
> +	value |= TEGRA186_GPIO_ENABLE_CONFIG_ENABLE;
> +	value &= ~TEGRA186_GPIO_ENABLE_CONFIG_OUT;
> +	writel(value, base + TEGRA186_GPIO_ENABLE_CONFIG);
> +
> +	return 0;
> +}
> +
> +static int tegra186_gpio_direction_output(struct gpio_chip *chip,
> +					  unsigned int offset, int level)
> +{
> +	struct tegra_gpio *gpio = gpiochip_get_data(chip);
> +	void __iomem *base;
> +	u32 value;
> +
> +	/* configure output level first */
> +	chip->set(chip, offset, level);
> +
> +	base = tegra186_gpio_get_base(gpio, offset);
> +	if (WARN_ON(base == NULL))
> +		return -EINVAL;
> +
> +	/* set the direction */
> +	value = readl(base + TEGRA186_GPIO_OUTPUT_CONTROL);
> +	value &= ~TEGRA186_GPIO_OUTPUT_CONTROL_FLOATED;
> +	writel(value, base + TEGRA186_GPIO_OUTPUT_CONTROL);
> +
> +	value = readl(base + TEGRA186_GPIO_ENABLE_CONFIG);
> +	value |= TEGRA186_GPIO_ENABLE_CONFIG_ENABLE;
> +	value |= TEGRA186_GPIO_ENABLE_CONFIG_OUT;
> +	writel(value, base + TEGRA186_GPIO_ENABLE_CONFIG);
> +
> +	return 0;
> +}
> +
> +static int tegra186_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct tegra_gpio *gpio = gpiochip_get_data(chip);
> +	void __iomem *base;
> +	u32 value;
> +
> +	base = tegra186_gpio_get_base(gpio, offset);
> +	if (WARN_ON(base == NULL))
> +		return -ENODEV;
> +
> +	value = readl(base + TEGRA186_GPIO_ENABLE_CONFIG);
> +	if (value & TEGRA186_GPIO_ENABLE_CONFIG_OUT)
> +		value = readl(base + TEGRA186_GPIO_OUTPUT_VALUE);
> +	else
> +		value = readl(base + TEGRA186_GPIO_INPUT);
> +
> +	return value & BIT(0);
> +}
> +
> +static void tegra186_gpio_set(struct gpio_chip *chip, unsigned int offset,
> +			      int level)
> +{
> +	struct tegra_gpio *gpio = gpiochip_get_data(chip);
> +	void __iomem *base;
> +	u32 value;
> +
> +	base = tegra186_gpio_get_base(gpio, offset);
> +	if (WARN_ON(base == NULL))
> +		return;
> +
> +	value = readl(base + TEGRA186_GPIO_OUTPUT_VALUE);
> +	if (level == 0)
> +		value &= ~TEGRA186_GPIO_OUTPUT_VALUE_HIGH;
> +	else
> +		value |= TEGRA186_GPIO_OUTPUT_VALUE_HIGH;
> +
> +	writel(value, base + TEGRA186_GPIO_OUTPUT_VALUE);
> +}
> +
> +static int tegra186_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct tegra_gpio *gpio = gpiochip_get_data(chip);
> +
> +	return irq_find_mapping(gpio->domain, offset);
> +}
> +
> +static int tegra186_gpio_of_xlate(struct gpio_chip *chip,
> +				  const struct of_phandle_args *spec,
> +				  u32 *flags)
> +{
> +	struct tegra_gpio *gpio = gpiochip_get_data(chip);
> +	unsigned int port, pin, i, offset = 0;
> +
> +	if (WARN_ON(chip->of_gpio_n_cells < 2))
> +		return -EINVAL;
> +
> +	if (WARN_ON(spec->args_count < chip->of_gpio_n_cells))
> +		return -EINVAL;
> +
> +	port = spec->args[0] / 8;
> +	pin = spec->args[0] % 8;
> +
> +	if (port >= gpio->soc->num_ports) {
> +		dev_err(chip->parent, "invalid port number: %u\n", port);
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < port; i++)
> +		offset += gpio->soc->ports[i].pins;
> +
> +	if (flags)
> +		*flags = spec->args[1];
> +
> +	return offset + pin;
> +}
> +
> +static void tegra186_irq_ack(struct irq_data *data)
> +{
> +	struct tegra_gpio *gpio = irq_data_get_irq_chip_data(data);
> +	void __iomem *base;
> +
> +	base = tegra186_gpio_get_base(gpio, data->hwirq);
> +	if (WARN_ON(base == NULL))
> +		return;
> +
> +	writel(1, base + TEGRA186_GPIO_INTERRUPT_CLEAR);
> +}
> +
> +static void tegra186_irq_mask(struct irq_data *data)
> +{
> +	struct tegra_gpio *gpio = irq_data_get_irq_chip_data(data);
> +	void __iomem *base;
> +	u32 value;
> +
> +	base = tegra186_gpio_get_base(gpio, data->hwirq);
> +	if (WARN_ON(base == NULL))
> +		return;
> +
> +	value = readl(base + TEGRA186_GPIO_ENABLE_CONFIG);
> +	value &= ~TEGRA186_GPIO_ENABLE_CONFIG_INTERRUPT;
> +	writel(value, base + TEGRA186_GPIO_ENABLE_CONFIG);
> +}
> +
> +static void tegra186_irq_unmask(struct irq_data *data)
> +{
> +	struct tegra_gpio *gpio = irq_data_get_irq_chip_data(data);
> +	void __iomem *base;
> +	u32 value;
> +
> +	base = tegra186_gpio_get_base(gpio, data->hwirq);
> +	if (WARN_ON(base == NULL))
> +		return;
> +
> +	value = readl(base + TEGRA186_GPIO_ENABLE_CONFIG);
> +	value |= TEGRA186_GPIO_ENABLE_CONFIG_INTERRUPT;
> +	writel(value, base + TEGRA186_GPIO_ENABLE_CONFIG);
> +}
> +
> +static int tegra186_irq_set_type(struct irq_data *data, unsigned int flow)
> +{
> +	struct tegra_gpio *gpio = irq_data_get_irq_chip_data(data);
> +	void __iomem *base;
> +	u32 value;
> +
> +	base = tegra186_gpio_get_base(gpio, data->hwirq);
> +	if (WARN_ON(base == NULL))
> +		return -ENODEV;
> +
> +	value = readl(base + TEGRA186_GPIO_ENABLE_CONFIG);
> +	value &= ~TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_TYPE_MASK;
> +	value &= ~TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_LEVEL;
> +
> +	switch (flow & IRQ_TYPE_SENSE_MASK) {
> +	case IRQ_TYPE_NONE:
> +		break;
> +
> +	case IRQ_TYPE_EDGE_RISING:
> +		value |= TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_TYPE_SINGLE_EDGE;
> +		value |= TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_LEVEL;
> +		break;
> +
> +	case IRQ_TYPE_EDGE_FALLING:
> +		value |= TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_TYPE_SINGLE_EDGE;
> +		break;
> +
> +	case IRQ_TYPE_EDGE_BOTH:
> +		value |= TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_TYPE_DOUBLE_EDGE;
> +		break;
> +
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		value |= TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_TYPE_LEVEL;
> +		value |= TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_LEVEL;
> +		break;
> +
> +	case IRQ_TYPE_LEVEL_LOW:
> +		value |= TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_TYPE_LEVEL;
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	writel(value, base + TEGRA186_GPIO_ENABLE_CONFIG);
> +
> +	if ((flow & IRQ_TYPE_EDGE_BOTH) == 0)
> +		irq_set_handler_locked(data, handle_level_irq);
> +	else
> +		irq_set_handler_locked(data, handle_edge_irq);
> +
> +	return 0;
> +}
> +
> +static void tegra186_gpio_irq(struct irq_desc *desc)
> +{
> +	struct tegra_gpio *gpio = irq_desc_get_handler_data(desc);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	unsigned int i, offset = 0;
> +
> +	chained_irq_enter(chip, desc);
> +
> +	for (i = 0; i < gpio->soc->num_ports; i++) {
> +		const struct tegra_gpio_port *port = &gpio->soc->ports[i];
> +		void __iomem *base = gpio->base + port->offset;
> +		unsigned int pin, irq;
> +		unsigned long value;
> +
> +		value = readl(base + TEGRA186_GPIO_INTERRUPT_STATUS(1));
> +
> +		for_each_set_bit(pin, &value, port->pins) {
> +			irq = irq_find_mapping(gpio->domain, offset + pin);
> +			if (WARN_ON(irq == 0))
> +				continue;
> +
> +			generic_handle_irq(irq);
> +		}
> +
> +		offset += port->pins;
> +	}
> +
> +	chained_irq_exit(chip, desc);
> +}
> +
> +static int tegra186_gpio_irq_domain_xlate(struct irq_domain *domain,
> +					  struct device_node *np,
> +					  const u32 *spec, unsigned int size,
> +					  unsigned long *hwirq,
> +					  unsigned int *type)
> +{
> +	struct tegra_gpio *gpio = domain->host_data;
> +	unsigned int port, pin, i, offset = 0;
> +
> +	if (size < 2)
> +		return -EINVAL;
> +
> +	port = spec[0] / 8;
> +	pin = spec[0] % 8;
> +
> +	if (port >= gpio->soc->num_ports) {
> +		dev_err(gpio->gpio.parent, "invalid port number: %u\n", port);
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < port; i++)
> +		offset += gpio->soc->ports[i].pins;
> +
> +	*type = spec[1] & IRQ_TYPE_SENSE_MASK;
> +	*hwirq = offset + pin;
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops tegra186_gpio_irq_domain_ops = {
> +	.xlate = tegra186_gpio_irq_domain_xlate,
> +};
> +
> +static struct lock_class_key tegra186_gpio_lock_class;
> +
> +static int tegra186_gpio_probe(struct platform_device *pdev)
> +{
> +	unsigned int i, j, irq, offset = 0;
> +	struct tegra_gpio *gpio;
> +	struct resource *res;
> +	char **names;
> +	int err;
> +
> +	gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
> +	if (!gpio)
> +		return -ENOMEM;
> +
> +	gpio->soc = of_device_get_match_data(&pdev->dev);
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "gpio");
> +	gpio->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(gpio->base))
> +		return PTR_ERR(gpio->base);
> +
> +	err = of_irq_count(pdev->dev.of_node);
> +	if (err < 0)
> +		return err;
> +
> +	gpio->num_irq = err;
> +
> +	gpio->irq = devm_kcalloc(&pdev->dev, gpio->num_irq, sizeof(*gpio->irq),
> +				 GFP_KERNEL);
> +	if (!gpio->irq)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < gpio->num_irq; i++) {
> +		err = platform_get_irq(pdev, i);
> +		if (err < 0)
> +			return err;
> +
> +		gpio->irq[i] = err;
> +	}
> +
> +	gpio->gpio.label = gpio->soc->name;
> +	gpio->gpio.parent = &pdev->dev;
> +
> +	gpio->gpio.get_direction = tegra186_gpio_get_direction;
> +	gpio->gpio.direction_input = tegra186_gpio_direction_input;
> +	gpio->gpio.direction_output = tegra186_gpio_direction_output;
> +	gpio->gpio.get = tegra186_gpio_get,
> +	gpio->gpio.set = tegra186_gpio_set;
> +	gpio->gpio.to_irq = tegra186_gpio_to_irq;
> +
> +	gpio->gpio.base = -1;
> +
> +	for (i = 0; i < gpio->soc->num_ports; i++)
> +		gpio->gpio.ngpio += gpio->soc->ports[i].pins;
> +
> +	names = devm_kcalloc(gpio->gpio.parent, gpio->gpio.ngpio,
> +			     sizeof(*names), GFP_KERNEL);
> +	if (!names)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < gpio->soc->num_ports; i++) {
> +		const struct tegra_gpio_port *port = &gpio->soc->ports[i];
> +		char *name;
> +
> +		for (j = 0; j < port->pins; j++) {
> +			name = devm_kasprintf(gpio->gpio.parent, GFP_KERNEL,
> +					      "P%s.%02x", port->name, j);
> +			if (!name)
> +				return -ENOMEM;
> +
> +			names[offset + j] = name;
> +		}
> +
> +		offset += port->pins;
> +	}
> +
> +	gpio->gpio.names = (const char * const *)names;
> +
> +	gpio->gpio.of_node = pdev->dev.of_node;
> +	gpio->gpio.of_gpio_n_cells = 2;
> +	gpio->gpio.of_xlate = tegra186_gpio_of_xlate;
> +
> +	gpio->intc.name = pdev->dev.of_node->name;
> +	gpio->intc.irq_ack = tegra186_irq_ack;
> +	gpio->intc.irq_mask = tegra186_irq_mask;
> +	gpio->intc.irq_unmask = tegra186_irq_unmask;
> +	gpio->intc.irq_set_type = tegra186_irq_set_type;
> +
> +	platform_set_drvdata(pdev, gpio);
> +
> +	err = devm_gpiochip_add_data(&pdev->dev, &gpio->gpio, gpio);
> +	if (err < 0)
> +		return err;
> +
> +	gpio->domain = irq_domain_add_linear(pdev->dev.of_node,
> +					     gpio->gpio.ngpio,
> +					     &tegra186_gpio_irq_domain_ops,
> +					     gpio);
> +	if (!gpio->domain)
> +		return -ENODEV;
> +
> +	for (i = 0; i < gpio->gpio.ngpio; i++) {
> +		irq = irq_create_mapping(gpio->domain, i);
> +		if (irq == 0) {
> +			dev_err(&pdev->dev,
> +				"failed to create IRQ mapping for GPIO#%u\n",
> +				i);
> +			continue;
> +		}
> +
> +		irq_set_lockdep_class(irq, &tegra186_gpio_lock_class);
> +		irq_set_chip_data(irq, gpio);
> +		irq_set_chip_and_handler(irq, &gpio->intc, handle_simple_irq);
> +	}
> +
> +	for (i = 0; i < gpio->num_irq; i++)
> +		irq_set_chained_handler_and_data(gpio->irq[i],
> +						 tegra186_gpio_irq,
> +						 gpio);
> +
> +	return 0;
> +}
> +
> +static int tegra186_gpio_remove(struct platform_device *pdev)
> +{
> +	struct tegra_gpio *gpio = platform_get_drvdata(pdev);
> +	unsigned int i, irq;
> +
> +	for (i = 0; i < gpio->num_irq; i++)
> +		irq_set_chained_handler_and_data(gpio->irq[i], NULL, NULL);
> +
> +	for (i = 0; i < gpio->gpio.ngpio; i++) {
> +		irq = irq_find_mapping(gpio->domain, i);
> +		irq_dispose_mapping(irq);
> +	}
> +
> +	irq_domain_remove(gpio->domain);
> +
> +	return 0;
> +}
> +
> +#define TEGRA_MAIN_GPIO_PORT(port, base, count)	\
> +	[TEGRA_MAIN_GPIO_PORT_##port] = {	\
> +		.name = #port,			\
> +		.offset = base,			\
> +		.pins = count,			\
> +	}
> +
> +static const struct tegra_gpio_port tegra186_main_ports[] = {
> +	TEGRA_MAIN_GPIO_PORT( A, 0x2000, 7),
> +	TEGRA_MAIN_GPIO_PORT( B, 0x3000, 7),
> +	TEGRA_MAIN_GPIO_PORT( C, 0x3200, 7),
> +	TEGRA_MAIN_GPIO_PORT( D, 0x3400, 6),
> +	TEGRA_MAIN_GPIO_PORT( E, 0x2200, 8),
> +	TEGRA_MAIN_GPIO_PORT( F, 0x2400, 6),
> +	TEGRA_MAIN_GPIO_PORT( G, 0x4200, 6),
> +	TEGRA_MAIN_GPIO_PORT( H, 0x1000, 7),
> +	TEGRA_MAIN_GPIO_PORT( I, 0x0800, 8),
> +	TEGRA_MAIN_GPIO_PORT( J, 0x5000, 8),
> +	TEGRA_MAIN_GPIO_PORT( K, 0x5200, 1),
> +	TEGRA_MAIN_GPIO_PORT( L, 0x1200, 8),
> +	TEGRA_MAIN_GPIO_PORT( M, 0x5600, 6),
> +	TEGRA_MAIN_GPIO_PORT( N, 0x0000, 7),
> +	TEGRA_MAIN_GPIO_PORT( O, 0x0200, 4),
> +	TEGRA_MAIN_GPIO_PORT( P, 0x4000, 7),
> +	TEGRA_MAIN_GPIO_PORT( Q, 0x0400, 6),
> +	TEGRA_MAIN_GPIO_PORT( R, 0x0a00, 6),
> +	TEGRA_MAIN_GPIO_PORT( T, 0x0600, 4),
> +	TEGRA_MAIN_GPIO_PORT( X, 0x1400, 8),
> +	TEGRA_MAIN_GPIO_PORT( Y, 0x1600, 7),
> +	TEGRA_MAIN_GPIO_PORT(BB, 0x2600, 2),
> +	TEGRA_MAIN_GPIO_PORT(CC, 0x5400, 4),
> +};
> +
> +static const struct tegra_gpio_soc tegra186_main_soc = {
> +	.num_ports = ARRAY_SIZE(tegra186_main_ports),
> +	.ports = tegra186_main_ports,
> +	.name = "tegra186-gpio",
> +};
> +
> +#define TEGRA_AON_GPIO_PORT(port, base, count)	\
> +	[TEGRA_AON_GPIO_PORT_##port] = {	\
> +		.name = #port,			\
> +		.offset = base,			\
> +		.pins = count,			\
> +	}
> +
> +static const struct tegra_gpio_port tegra186_aon_ports[] = {
> +	TEGRA_AON_GPIO_PORT( S, 0x0200, 5),
> +	TEGRA_AON_GPIO_PORT( U, 0x0400, 6),
> +	TEGRA_AON_GPIO_PORT( V, 0x0800, 8),
> +	TEGRA_AON_GPIO_PORT( W, 0x0a00, 8),
> +	TEGRA_AON_GPIO_PORT( Z, 0x0e00, 4),
> +	TEGRA_AON_GPIO_PORT(AA, 0x0c00, 8),
> +	TEGRA_AON_GPIO_PORT(EE, 0x0600, 3),
> +	TEGRA_AON_GPIO_PORT(FF, 0x0000, 5),
> +};
> +
> +static const struct tegra_gpio_soc tegra186_aon_soc = {
> +	.num_ports = ARRAY_SIZE(tegra186_aon_ports),
> +	.ports = tegra186_aon_ports,
> +	.name = "tegra186-gpio-aon",
> +};
> +
> +static const struct of_device_id tegra186_gpio_of_match[] = {
> +	{
> +		.compatible = "nvidia,tegra186-gpio",
> +		.data = &tegra186_main_soc
> +	}, {
> +		.compatible = "nvidia,tegra186-gpio-aon",
> +		.data = &tegra186_aon_soc
> +	}, {
> +		/* sentinel */
> +	}
> +};
> +
> +static struct platform_driver tegra186_gpio_driver = {
> +	.driver = {
> +		.name = "tegra186-gpio",
> +		.of_match_table = tegra186_gpio_of_match,
> +	},
> +	.probe = tegra186_gpio_probe,
> +	.remove = tegra186_gpio_remove,
> +};
> +module_platform_driver(tegra186_gpio_driver);
> +
> +MODULE_DESCRIPTION("NVIDIA Tegra186 GPIO controller driver");
> +MODULE_AUTHOR("Thierry Reding <treding@nvidia.com>");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.12.0
>
Linus Walleij March 31, 2017, 1:36 p.m.
On Fri, Mar 31, 2017 at 3:10 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:

> any objections to merging this for v4.12?

Sorry I should have done a proper review. I'll look at it now.

>I know Laxman has some
> reservations about the internal numbering, but I hope my explanation in
> the other subthread shows that this is the simplest way to achieve what
> we want with the least amount of surprises.

As long as he's not NACKing it it's fine I guess.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij March 31, 2017, 1:59 p.m.
On Fri, Mar 10, 2017 at 5:26 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:

> From: Thierry Reding <treding@nvidia.com>
>
> Tegra186 has two GPIO controllers that are largely register compatible
> between one another but are completely different from the controller
> found on earlier generations.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v2:

So this still doesn't use GPIOLIB_IRQCHIP even though I pointed
out that you can assign several parent interrupts to the same
irqchip.

For a recent example see:
http://marc.info/?l=devicetree&m=149012117004066&w=2
(Notice Gregory's elegant manipulation of the mask.)

My earlier reply here:

----------------------
>> I would prefer if you could try to convert this driver to use
>> CONFIG_GPIOLIB_IRQCHIP and install a chained interrupt
>> handler with gpiochip_irqchio_add() + gpiolib_set_chained_irqchip().
>> It would save us so much trouble and so much complicated
>> code to maintain for this custom irqdomain.
>>
>> I suggest you to look into the mechanisms mentioned in my
>> previous mail for how to poke holes in the single linear
>> irqdomain used by this mechanism.
>>
>> As it seems, you only have one parent interrupt with all
>> these IRQs cascading off it as far as I can tell.
>
> Like I said in other email, I don't think this will work because the
> GPIOLIB_IRQCHIP support seems to be limited to cases where a single
> parent interrupt is used. We could possibly live with a single IRQ
> handler and data, but we definitely need to request multiple IRQs if
> we want to be able to use all GPIOs as interrupts.

Sorry if I missed that.

Actually it works.

If you look at gpiochip_set_chained_irqchip() it just calls
irq_set_chained_handler_and_data() for the parent interrupt.
------------------------

Just call gpiochip_set_chained_irqchip() for all the irqs, and
figure out a way to get the right interrupt hardware offset in the
interrupt handler.

(...)

> +config GPIO_TEGRA186
> +       tristate "NVIDIA Tegra186 GPIO support"
> +       default ARCH_TEGRA_186_SOC
> +       depends on ARCH_TEGRA_186_SOC || COMPILE_TEST
> +       depends on OF_GPIO

select GPIOLIB_IRQCHIP

> +#include <linux/gpio/driver.h>
> +#include <linux/gpio.h>

Only <linux/gpio/driver.h>

You should not use <linux/gpio.h> in drivers, then you are
doing something wrong.

> +struct tegra_gpio {
> +       struct gpio_chip gpio;
> +       struct irq_chip intc;
> +       unsigned int num_irq;
> +       unsigned int *irq;

Why are you keeping the irqs around after requesting?
Use devm_*

> +
> +       const struct tegra_gpio_soc *soc;
> +
> +       void __iomem *base;
> +
> +       struct irq_domain *domain;

I don't like custom irq domains it is messy.
Please do your best to try to use GPIOLIB_IRQCHIP

If you still decide to go with a custom irqdomain, there is
stuff missing, especially the gpiochip_lock_as_irq()
calls from the irqchip .irq_request/release_resources()
callbacks, see the gpiolib.c implementation in the
helpers there for reference.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thierry Reding March 31, 2017, 2:54 p.m.
On Fri, Mar 31, 2017 at 03:59:40PM +0200, Linus Walleij wrote:
> On Fri, Mar 10, 2017 at 5:26 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> 
> > From: Thierry Reding <treding@nvidia.com>
> >
> > Tegra186 has two GPIO controllers that are largely register compatible
> > between one another but are completely different from the controller
> > found on earlier generations.
> >
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > Changes in v2:
> 
> So this still doesn't use GPIOLIB_IRQCHIP even though I pointed
> out that you can assign several parent interrupts to the same
> irqchip.
> 
> For a recent example see:
> http://marc.info/?l=devicetree&m=149012117004066&w=2
> (Notice Gregory's elegant manipulation of the mask.)
> 
> My earlier reply here:
> 
> ----------------------
> >> I would prefer if you could try to convert this driver to use
> >> CONFIG_GPIOLIB_IRQCHIP and install a chained interrupt
> >> handler with gpiochip_irqchio_add() + gpiolib_set_chained_irqchip().
> >> It would save us so much trouble and so much complicated
> >> code to maintain for this custom irqdomain.
> >>
> >> I suggest you to look into the mechanisms mentioned in my
> >> previous mail for how to poke holes in the single linear
> >> irqdomain used by this mechanism.
> >>
> >> As it seems, you only have one parent interrupt with all
> >> these IRQs cascading off it as far as I can tell.
> >
> > Like I said in other email, I don't think this will work because the
> > GPIOLIB_IRQCHIP support seems to be limited to cases where a single
> > parent interrupt is used. We could possibly live with a single IRQ
> > handler and data, but we definitely need to request multiple IRQs if
> > we want to be able to use all GPIOs as interrupts.
> 
> Sorry if I missed that.
> 
> Actually it works.
> 
> If you look at gpiochip_set_chained_irqchip() it just calls
> irq_set_chained_handler_and_data() for the parent interrupt.
> ------------------------
> 
> Just call gpiochip_set_chained_irqchip() for all the irqs, and
> figure out a way to get the right interrupt hardware offset in the
> interrupt handler.

Okay, let me quote the gpiochip_set_chained_irqchip() function here and
explain why I think it's not a proper fit here. This is actually quoting
gpiochip_set_cascaded_irqchip(), but that's what ends up being called.

> static void gpiochip_set_cascaded_irqchip(struct gpio_chip *gpiochip,
> 					  struct irq_chip *irqchip,
> 					  int parent_irq,
> 					  irq_flow_handler_t parent_handler)
> {
> 	unsigned int offset;
> 
> 	if (!gpiochip->irqdomain) {
> 		chip_err(gpiochip, "called %s before setting up irqchip\n",
> 			 __func__);
> 		return;
> 	}
> 
> 	if (parent_handler) {
> 		if (gpiochip->can_sleep) {
> 			chip_err(gpiochip,
> 				 "you cannot have chained interrupts on a "
> 				 "chip that may sleep\n");
> 			return;
> 		}
> 		/*
> 		 * The parent irqchip is already using the chip_data for this
> 		 * irqchip, so our callbacks simply use the handler_data.
> 		 */
> 		irq_set_chained_handler_and_data(parent_irq, parent_handler,
> 						 gpiochip);
> 
> 		gpiochip->irq_chained_parent = parent_irq;

If I call this multiple times with different parent_irq parameters, then
only the last one will be stored in gpiochip->irq_chained_parent. Later
on, this is used to disconnect the chained handler and data upon GPIO
chip removal, which means that handler and data for all but the last one
end up dangling.

> 	}
> 
> 	/* Set the parent IRQ for all affected IRQs */
> 	for (offset = 0; offset < gpiochip->ngpio; offset++) {
> 		if (!gpiochip_irqchip_irq_valid(gpiochip, offset))
> 			continue;
> 		irq_set_parent(irq_find_mapping(gpiochip->irqdomain, offset),
> 			       parent_irq);
> 	}
> }

Similarly here, we'd be setting the parent IRQ of all associated GPIOs
to the first, second... last parent IRQ. This is completely unnecessary
work and it's also setting the wrong parent. Note that we don't actually
care about that in the driver right now, but it's still wrong.

> > +config GPIO_TEGRA186
> > +       tristate "NVIDIA Tegra186 GPIO support"
> > +       default ARCH_TEGRA_186_SOC
> > +       depends on ARCH_TEGRA_186_SOC || COMPILE_TEST
> > +       depends on OF_GPIO
> 
> select GPIOLIB_IRQCHIP
> 
> > +#include <linux/gpio/driver.h>
> > +#include <linux/gpio.h>
> 
> Only <linux/gpio/driver.h>
> 
> You should not use <linux/gpio.h> in drivers, then you are
> doing something wrong.

Yeah, I don't seem to need this at all.

> > +struct tegra_gpio {
> > +       struct gpio_chip gpio;
> > +       struct irq_chip intc;
> > +       unsigned int num_irq;
> > +       unsigned int *irq;
> 
> Why are you keeping the irqs around after requesting?
> Use devm_*

First I prefer to keep resource request and driver initialization
separate because it makes error recovery more robust. So this is used to
first store the results from platform_get_irq() and later on to iterate
over when installing the chained handlers.

Also, devm_* doesn't exist for irq_set_chained_handler_and_data(). So I
need to keep these around in order to properly uninstall the handlers
again on removal.

> > +
> > +       const struct tegra_gpio_soc *soc;
> > +
> > +       void __iomem *base;
> > +
> > +       struct irq_domain *domain;
> 
> I don't like custom irq domains it is messy.
> Please do your best to try to use GPIOLIB_IRQCHIP

Like I explained above, I don't think it works the way it is supposed to
for this case. The armada-37xx patch that you linked to earlier seems to
suffer from the same issues in that all but the last parent IRQ handlers
will be left dangling after removal, which would cause the kernel to run
non-existing code if by any chance the interrupts were to still fire for
some reason.

> If you still decide to go with a custom irqdomain, there is
> stuff missing, especially the gpiochip_lock_as_irq()
> calls from the irqchip .irq_request/release_resources()
> callbacks, see the gpiolib.c implementation in the
> helpers there for reference.

Good catch. I will add those.

Thierry
Laxman Dewangan April 1, 2017, 7:45 a.m.
On Friday 31 March 2017 07:06 PM, Linus Walleij wrote:
> On Fri, Mar 31, 2017 at 3:10 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
>
>> any objections to merging this for v4.12?
> Sorry I should have done a proper review. I'll look at it now.
>
>> I know Laxman has some
>> reservations about the internal numbering, but I hope my explanation in
>> the other subthread shows that this is the simplest way to achieve what
>> we want with the least amount of surprises.
> As long as he's not NACKing it it's fine I guess.
I am fine with patch.

You can consider my ack.

Acked-by: Laxman Dewangan<ldewangan@nvidia.com>

thanks,
Laxman
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij April 1, 2017, 5:46 p.m.
On Fri, Mar 31, 2017 at 4:54 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Fri, Mar 31, 2017 at 03:59:40PM +0200, Linus Walleij wrote:

> If I call this multiple times with different parent_irq parameters, then
> only the last one will be stored in gpiochip->irq_chained_parent. Later
> on, this is used to disconnect the chained handler and data upon GPIO
> chip removal, which means that handler and data for all but the last one
> end up dangling.

People are using this code already for chained handlers with several
parents.

I guess it works so nicely because gpiochips are often just added
and seldom removed.

Overall that means this is a bug and should be fixed, not that new
drivers should avoid using this approach, or that we should discourage
new drivers to use this API.

>>       /* Set the parent IRQ for all affected IRQs */
>>       for (offset = 0; offset < gpiochip->ngpio; offset++) {
>>               if (!gpiochip_irqchip_irq_valid(gpiochip, offset))
>>                       continue;
>>               irq_set_parent(irq_find_mapping(gpiochip->irqdomain, offset),
>>                              parent_irq);
>>       }
>> }
>
> Similarly here, we'd be setting the parent IRQ of all associated GPIOs
> to the first, second... last parent IRQ. This is completely unnecessary
> work and it's also setting the wrong parent. Note that we don't actually
> care about that in the driver right now, but it's still wrong.

Is that a way of saying there are bugs in the gpiolib?

I know there are, maintainers working on core code are always
lacking. I wrote this code and I admit I make mistakes, please help
me fix them instead of trying to make this into a reason not to
use this code.

>> Why are you keeping the irqs around after requesting?
>> Use devm_*
>
> First I prefer to keep resource request and driver initialization
> separate because it makes error recovery more robust. So this is used to
> first store the results from platform_get_irq() and later on to iterate
> over when installing the chained handlers.

OK no big deal. But it could be in a local variable rather than
in the driver state container, right?

> Also, devm_* doesn't exist for irq_set_chained_handler_and_data(). So I
> need to keep these around in order to properly uninstall the handlers
> again on removal.

OK is this something that should be fixed in the irqchip code?

> Like I explained above, I don't think it works the way it is supposed to
> for this case. The armada-37xx patch that you linked to earlier seems to
> suffer from the same issues in that all but the last parent IRQ handlers
> will be left dangling after removal, which would cause the kernel to run
> non-existing code if by any chance the interrupts were to still fire for
> some reason.

Again, bugs is not a reason not to use library code. We need to fix
those bugs and centralize more, or we get too much strange code
to maintain.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thierry Reding April 3, 2017, 5:28 a.m.
On Sat, Apr 01, 2017 at 07:46:24PM +0200, Linus Walleij wrote:
> On Fri, Mar 31, 2017 at 4:54 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Fri, Mar 31, 2017 at 03:59:40PM +0200, Linus Walleij wrote:
> 
> > If I call this multiple times with different parent_irq parameters, then
> > only the last one will be stored in gpiochip->irq_chained_parent. Later
> > on, this is used to disconnect the chained handler and data upon GPIO
> > chip removal, which means that handler and data for all but the last one
> > end up dangling.
> 
> People are using this code already for chained handlers with several
> parents.
> 
> I guess it works so nicely because gpiochips are often just added
> and seldom removed.
> 
> Overall that means this is a bug and should be fixed, not that new
> drivers should avoid using this approach, or that we should discourage
> new drivers to use this API.
> 
> >>       /* Set the parent IRQ for all affected IRQs */
> >>       for (offset = 0; offset < gpiochip->ngpio; offset++) {
> >>               if (!gpiochip_irqchip_irq_valid(gpiochip, offset))
> >>                       continue;
> >>               irq_set_parent(irq_find_mapping(gpiochip->irqdomain, offset),
> >>                              parent_irq);
> >>       }
> >> }
> >
> > Similarly here, we'd be setting the parent IRQ of all associated GPIOs
> > to the first, second... last parent IRQ. This is completely unnecessary
> > work and it's also setting the wrong parent. Note that we don't actually
> > care about that in the driver right now, but it's still wrong.
> 
> Is that a way of saying there are bugs in the gpiolib?
> 
> I know there are, maintainers working on core code are always
> lacking. I wrote this code and I admit I make mistakes, please help
> me fix them instead of trying to make this into a reason not to
> use this code.

Fair enough. I'll see if I can turn this into something that I can use
for this particular driver. I suspect it'll be a bit cumbersome because
of the parent/child relationship, but I have a couple of ideas that I'd
like to try out.

> >> Why are you keeping the irqs around after requesting?
> >> Use devm_*
> >
> > First I prefer to keep resource request and driver initialization
> > separate because it makes error recovery more robust. So this is used to
> > first store the results from platform_get_irq() and later on to iterate
> > over when installing the chained handlers.
> 
> OK no big deal. But it could be in a local variable rather than
> in the driver state container, right?

Well, I also need the same array of interrupts to remove the handlers
again, so I don't think local would be enough. But...

> 
> > Also, devm_* doesn't exist for irq_set_chained_handler_and_data(). So I
> > need to keep these around in order to properly uninstall the handlers
> > again on removal.
> 
> OK is this something that should be fixed in the irqchip code?

... with this in place it wouldn't be necessary. Cross-subsystem series
are unlikely to make it for v4.12 at this point, so here go my hopes to
get this merged sometime soon...

Thierry

Patch hide | download patch | download mbox

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 05043071fc98..f74b61cfd113 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -431,6 +431,14 @@  config GPIO_TEGRA
 	help
 	  Say yes here to support GPIO pins on NVIDIA Tegra SoCs.
 
+config GPIO_TEGRA186
+	tristate "NVIDIA Tegra186 GPIO support"
+	default ARCH_TEGRA_186_SOC
+	depends on ARCH_TEGRA_186_SOC || COMPILE_TEST
+	depends on OF_GPIO
+	help
+	  Say yes here to support GPIO pins on NVIDIA Tegra186 SoCs.
+
 config GPIO_TS4800
 	tristate "TS-4800 DIO blocks and compatibles"
 	depends on OF_GPIO
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index becb96c724fe..cad0eb2c3531 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -111,6 +111,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 000000000000..87e02d9165d8
--- /dev/null
+++ b/drivers/gpio/gpio-tegra186.c
@@ -0,0 +1,636 @@ 
+/*
+ * Copyright (c) 2016-2017 NVIDIA Corporation
+ *
+ * Author: Thierry Reding <treding@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/gpio/driver.h>
+#include <linux/gpio.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+
+#include <dt-bindings/gpio/tegra186-gpio.h>
+
+#define TEGRA186_GPIO_ENABLE_CONFIG 0x00
+#define  TEGRA186_GPIO_ENABLE_CONFIG_ENABLE BIT(0)
+#define  TEGRA186_GPIO_ENABLE_CONFIG_OUT BIT(1)
+#define  TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_TYPE_NONE (0x0 << 2)
+#define  TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_TYPE_LEVEL (0x1 << 2)
+#define  TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_TYPE_SINGLE_EDGE (0x2 << 2)
+#define  TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_TYPE_DOUBLE_EDGE (0x3 << 2)
+#define  TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_TYPE_MASK (0x3 << 2)
+#define  TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_LEVEL BIT(4)
+#define  TEGRA186_GPIO_ENABLE_CONFIG_INTERRUPT BIT(6)
+
+#define TEGRA186_GPIO_DEBOUNCE_CONTROL 0x04
+#define  TEGRA186_GPIO_DEBOUNCE_CONTROL_THRESHOLD(x) ((x) & 0xff)
+
+#define TEGRA186_GPIO_INPUT 0x08
+#define  TEGRA186_GPIO_INPUT_HIGH BIT(0)
+
+#define TEGRA186_GPIO_OUTPUT_CONTROL 0x0c
+#define  TEGRA186_GPIO_OUTPUT_CONTROL_FLOATED BIT(0)
+
+#define TEGRA186_GPIO_OUTPUT_VALUE 0x10
+#define  TEGRA186_GPIO_OUTPUT_VALUE_HIGH BIT(0)
+
+#define TEGRA186_GPIO_INTERRUPT_CLEAR 0x14
+
+#define TEGRA186_GPIO_INTERRUPT_STATUS(x) (0x100 + (x) * 4)
+
+struct tegra_gpio_port {
+	const char *name;
+	unsigned int offset;
+	unsigned int pins;
+};
+
+struct tegra_gpio_soc {
+	const struct tegra_gpio_port *ports;
+	unsigned int num_ports;
+	const char *name;
+};
+
+struct tegra_gpio {
+	struct gpio_chip gpio;
+	struct irq_chip intc;
+	unsigned int num_irq;
+	unsigned int *irq;
+
+	const struct tegra_gpio_soc *soc;
+
+	void __iomem *base;
+
+	struct irq_domain *domain;
+};
+
+static const struct tegra_gpio_port *
+tegra186_gpio_get_port(struct tegra_gpio *gpio, unsigned int *pin)
+{
+	unsigned int start = 0, i;
+
+	for (i = 0; i < gpio->soc->num_ports; i++) {
+		const struct tegra_gpio_port *port = &gpio->soc->ports[i];
+
+		if (*pin >= start && *pin < start + port->pins) {
+			*pin -= start;
+			return port;
+		}
+
+		start += port->pins;
+	}
+
+	return NULL;
+}
+
+static void __iomem *tegra186_gpio_get_base(struct tegra_gpio *gpio,
+					    unsigned int pin)
+{
+	const struct tegra_gpio_port *port;
+
+	port = tegra186_gpio_get_port(gpio, &pin);
+	if (!port)
+		return NULL;
+
+	return gpio->base + port->offset + pin * 0x20;
+}
+
+static int tegra186_gpio_get_direction(struct gpio_chip *chip,
+				       unsigned int offset)
+{
+	struct tegra_gpio *gpio = gpiochip_get_data(chip);
+	void __iomem *base;
+	u32 value;
+
+	base = tegra186_gpio_get_base(gpio, offset);
+	if (WARN_ON(base == NULL))
+		return -ENODEV;
+
+	value = readl(base + TEGRA186_GPIO_ENABLE_CONFIG);
+	if (value & TEGRA186_GPIO_ENABLE_CONFIG_OUT)
+		return GPIOF_DIR_OUT;
+
+	return GPIOF_DIR_IN;
+}
+
+static int tegra186_gpio_direction_input(struct gpio_chip *chip,
+					 unsigned int offset)
+{
+	struct tegra_gpio *gpio = gpiochip_get_data(chip);
+	void __iomem *base;
+	u32 value;
+
+	base = tegra186_gpio_get_base(gpio, offset);
+	if (WARN_ON(base == NULL))
+		return -ENODEV;
+
+	value = readl(base + TEGRA186_GPIO_OUTPUT_CONTROL);
+	value |= TEGRA186_GPIO_OUTPUT_CONTROL_FLOATED;
+	writel(value, base + TEGRA186_GPIO_OUTPUT_CONTROL);
+
+	value = readl(base + TEGRA186_GPIO_ENABLE_CONFIG);
+	value |= TEGRA186_GPIO_ENABLE_CONFIG_ENABLE;
+	value &= ~TEGRA186_GPIO_ENABLE_CONFIG_OUT;
+	writel(value, base + TEGRA186_GPIO_ENABLE_CONFIG);
+
+	return 0;
+}
+
+static int tegra186_gpio_direction_output(struct gpio_chip *chip,
+					  unsigned int offset, int level)
+{
+	struct tegra_gpio *gpio = gpiochip_get_data(chip);
+	void __iomem *base;
+	u32 value;
+
+	/* configure output level first */
+	chip->set(chip, offset, level);
+
+	base = tegra186_gpio_get_base(gpio, offset);
+	if (WARN_ON(base == NULL))
+		return -EINVAL;
+
+	/* set the direction */
+	value = readl(base + TEGRA186_GPIO_OUTPUT_CONTROL);
+	value &= ~TEGRA186_GPIO_OUTPUT_CONTROL_FLOATED;
+	writel(value, base + TEGRA186_GPIO_OUTPUT_CONTROL);
+
+	value = readl(base + TEGRA186_GPIO_ENABLE_CONFIG);
+	value |= TEGRA186_GPIO_ENABLE_CONFIG_ENABLE;
+	value |= TEGRA186_GPIO_ENABLE_CONFIG_OUT;
+	writel(value, base + TEGRA186_GPIO_ENABLE_CONFIG);
+
+	return 0;
+}
+
+static int tegra186_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct tegra_gpio *gpio = gpiochip_get_data(chip);
+	void __iomem *base;
+	u32 value;
+
+	base = tegra186_gpio_get_base(gpio, offset);
+	if (WARN_ON(base == NULL))
+		return -ENODEV;
+
+	value = readl(base + TEGRA186_GPIO_ENABLE_CONFIG);
+	if (value & TEGRA186_GPIO_ENABLE_CONFIG_OUT)
+		value = readl(base + TEGRA186_GPIO_OUTPUT_VALUE);
+	else
+		value = readl(base + TEGRA186_GPIO_INPUT);
+
+	return value & BIT(0);
+}
+
+static void tegra186_gpio_set(struct gpio_chip *chip, unsigned int offset,
+			      int level)
+{
+	struct tegra_gpio *gpio = gpiochip_get_data(chip);
+	void __iomem *base;
+	u32 value;
+
+	base = tegra186_gpio_get_base(gpio, offset);
+	if (WARN_ON(base == NULL))
+		return;
+
+	value = readl(base + TEGRA186_GPIO_OUTPUT_VALUE);
+	if (level == 0)
+		value &= ~TEGRA186_GPIO_OUTPUT_VALUE_HIGH;
+	else
+		value |= TEGRA186_GPIO_OUTPUT_VALUE_HIGH;
+
+	writel(value, base + TEGRA186_GPIO_OUTPUT_VALUE);
+}
+
+static int tegra186_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
+{
+	struct tegra_gpio *gpio = gpiochip_get_data(chip);
+
+	return irq_find_mapping(gpio->domain, offset);
+}
+
+static int tegra186_gpio_of_xlate(struct gpio_chip *chip,
+				  const struct of_phandle_args *spec,
+				  u32 *flags)
+{
+	struct tegra_gpio *gpio = gpiochip_get_data(chip);
+	unsigned int port, pin, i, offset = 0;
+
+	if (WARN_ON(chip->of_gpio_n_cells < 2))
+		return -EINVAL;
+
+	if (WARN_ON(spec->args_count < chip->of_gpio_n_cells))
+		return -EINVAL;
+
+	port = spec->args[0] / 8;
+	pin = spec->args[0] % 8;
+
+	if (port >= gpio->soc->num_ports) {
+		dev_err(chip->parent, "invalid port number: %u\n", port);
+		return -EINVAL;
+	}
+
+	for (i = 0; i < port; i++)
+		offset += gpio->soc->ports[i].pins;
+
+	if (flags)
+		*flags = spec->args[1];
+
+	return offset + pin;
+}
+
+static void tegra186_irq_ack(struct irq_data *data)
+{
+	struct tegra_gpio *gpio = irq_data_get_irq_chip_data(data);
+	void __iomem *base;
+
+	base = tegra186_gpio_get_base(gpio, data->hwirq);
+	if (WARN_ON(base == NULL))
+		return;
+
+	writel(1, base + TEGRA186_GPIO_INTERRUPT_CLEAR);
+}
+
+static void tegra186_irq_mask(struct irq_data *data)
+{
+	struct tegra_gpio *gpio = irq_data_get_irq_chip_data(data);
+	void __iomem *base;
+	u32 value;
+
+	base = tegra186_gpio_get_base(gpio, data->hwirq);
+	if (WARN_ON(base == NULL))
+		return;
+
+	value = readl(base + TEGRA186_GPIO_ENABLE_CONFIG);
+	value &= ~TEGRA186_GPIO_ENABLE_CONFIG_INTERRUPT;
+	writel(value, base + TEGRA186_GPIO_ENABLE_CONFIG);
+}
+
+static void tegra186_irq_unmask(struct irq_data *data)
+{
+	struct tegra_gpio *gpio = irq_data_get_irq_chip_data(data);
+	void __iomem *base;
+	u32 value;
+
+	base = tegra186_gpio_get_base(gpio, data->hwirq);
+	if (WARN_ON(base == NULL))
+		return;
+
+	value = readl(base + TEGRA186_GPIO_ENABLE_CONFIG);
+	value |= TEGRA186_GPIO_ENABLE_CONFIG_INTERRUPT;
+	writel(value, base + TEGRA186_GPIO_ENABLE_CONFIG);
+}
+
+static int tegra186_irq_set_type(struct irq_data *data, unsigned int flow)
+{
+	struct tegra_gpio *gpio = irq_data_get_irq_chip_data(data);
+	void __iomem *base;
+	u32 value;
+
+	base = tegra186_gpio_get_base(gpio, data->hwirq);
+	if (WARN_ON(base == NULL))
+		return -ENODEV;
+
+	value = readl(base + TEGRA186_GPIO_ENABLE_CONFIG);
+	value &= ~TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_TYPE_MASK;
+	value &= ~TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_LEVEL;
+
+	switch (flow & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_NONE:
+		break;
+
+	case IRQ_TYPE_EDGE_RISING:
+		value |= TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_TYPE_SINGLE_EDGE;
+		value |= TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_LEVEL;
+		break;
+
+	case IRQ_TYPE_EDGE_FALLING:
+		value |= TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_TYPE_SINGLE_EDGE;
+		break;
+
+	case IRQ_TYPE_EDGE_BOTH:
+		value |= TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_TYPE_DOUBLE_EDGE;
+		break;
+
+	case IRQ_TYPE_LEVEL_HIGH:
+		value |= TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_TYPE_LEVEL;
+		value |= TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_LEVEL;
+		break;
+
+	case IRQ_TYPE_LEVEL_LOW:
+		value |= TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_TYPE_LEVEL;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	writel(value, base + TEGRA186_GPIO_ENABLE_CONFIG);
+
+	if ((flow & IRQ_TYPE_EDGE_BOTH) == 0)
+		irq_set_handler_locked(data, handle_level_irq);
+	else
+		irq_set_handler_locked(data, handle_edge_irq);
+
+	return 0;
+}
+
+static void tegra186_gpio_irq(struct irq_desc *desc)
+{
+	struct tegra_gpio *gpio = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	unsigned int i, offset = 0;
+
+	chained_irq_enter(chip, desc);
+
+	for (i = 0; i < gpio->soc->num_ports; i++) {
+		const struct tegra_gpio_port *port = &gpio->soc->ports[i];
+		void __iomem *base = gpio->base + port->offset;
+		unsigned int pin, irq;
+		unsigned long value;
+
+		value = readl(base + TEGRA186_GPIO_INTERRUPT_STATUS(1));
+
+		for_each_set_bit(pin, &value, port->pins) {
+			irq = irq_find_mapping(gpio->domain, offset + pin);
+			if (WARN_ON(irq == 0))
+				continue;
+
+			generic_handle_irq(irq);
+		}
+
+		offset += port->pins;
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+static int tegra186_gpio_irq_domain_xlate(struct irq_domain *domain,
+					  struct device_node *np,
+					  const u32 *spec, unsigned int size,
+					  unsigned long *hwirq,
+					  unsigned int *type)
+{
+	struct tegra_gpio *gpio = domain->host_data;
+	unsigned int port, pin, i, offset = 0;
+
+	if (size < 2)
+		return -EINVAL;
+
+	port = spec[0] / 8;
+	pin = spec[0] % 8;
+
+	if (port >= gpio->soc->num_ports) {
+		dev_err(gpio->gpio.parent, "invalid port number: %u\n", port);
+		return -EINVAL;
+	}
+
+	for (i = 0; i < port; i++)
+		offset += gpio->soc->ports[i].pins;
+
+	*type = spec[1] & IRQ_TYPE_SENSE_MASK;
+	*hwirq = offset + pin;
+
+	return 0;
+}
+
+static const struct irq_domain_ops tegra186_gpio_irq_domain_ops = {
+	.xlate = tegra186_gpio_irq_domain_xlate,
+};
+
+static struct lock_class_key tegra186_gpio_lock_class;
+
+static int tegra186_gpio_probe(struct platform_device *pdev)
+{
+	unsigned int i, j, irq, offset = 0;
+	struct tegra_gpio *gpio;
+	struct resource *res;
+	char **names;
+	int err;
+
+	gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
+	if (!gpio)
+		return -ENOMEM;
+
+	gpio->soc = of_device_get_match_data(&pdev->dev);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "gpio");
+	gpio->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(gpio->base))
+		return PTR_ERR(gpio->base);
+
+	err = of_irq_count(pdev->dev.of_node);
+	if (err < 0)
+		return err;
+
+	gpio->num_irq = err;
+
+	gpio->irq = devm_kcalloc(&pdev->dev, gpio->num_irq, sizeof(*gpio->irq),
+				 GFP_KERNEL);
+	if (!gpio->irq)
+		return -ENOMEM;
+
+	for (i = 0; i < gpio->num_irq; i++) {
+		err = platform_get_irq(pdev, i);
+		if (err < 0)
+			return err;
+
+		gpio->irq[i] = err;
+	}
+
+	gpio->gpio.label = gpio->soc->name;
+	gpio->gpio.parent = &pdev->dev;
+
+	gpio->gpio.get_direction = tegra186_gpio_get_direction;
+	gpio->gpio.direction_input = tegra186_gpio_direction_input;
+	gpio->gpio.direction_output = tegra186_gpio_direction_output;
+	gpio->gpio.get = tegra186_gpio_get,
+	gpio->gpio.set = tegra186_gpio_set;
+	gpio->gpio.to_irq = tegra186_gpio_to_irq;
+
+	gpio->gpio.base = -1;
+
+	for (i = 0; i < gpio->soc->num_ports; i++)
+		gpio->gpio.ngpio += gpio->soc->ports[i].pins;
+
+	names = devm_kcalloc(gpio->gpio.parent, gpio->gpio.ngpio,
+			     sizeof(*names), GFP_KERNEL);
+	if (!names)
+		return -ENOMEM;
+
+	for (i = 0; i < gpio->soc->num_ports; i++) {
+		const struct tegra_gpio_port *port = &gpio->soc->ports[i];
+		char *name;
+
+		for (j = 0; j < port->pins; j++) {
+			name = devm_kasprintf(gpio->gpio.parent, GFP_KERNEL,
+					      "P%s.%02x", port->name, j);
+			if (!name)
+				return -ENOMEM;
+
+			names[offset + j] = name;
+		}
+
+		offset += port->pins;
+	}
+
+	gpio->gpio.names = (const char * const *)names;
+
+	gpio->gpio.of_node = pdev->dev.of_node;
+	gpio->gpio.of_gpio_n_cells = 2;
+	gpio->gpio.of_xlate = tegra186_gpio_of_xlate;
+
+	gpio->intc.name = pdev->dev.of_node->name;
+	gpio->intc.irq_ack = tegra186_irq_ack;
+	gpio->intc.irq_mask = tegra186_irq_mask;
+	gpio->intc.irq_unmask = tegra186_irq_unmask;
+	gpio->intc.irq_set_type = tegra186_irq_set_type;
+
+	platform_set_drvdata(pdev, gpio);
+
+	err = devm_gpiochip_add_data(&pdev->dev, &gpio->gpio, gpio);
+	if (err < 0)
+		return err;
+
+	gpio->domain = irq_domain_add_linear(pdev->dev.of_node,
+					     gpio->gpio.ngpio,
+					     &tegra186_gpio_irq_domain_ops,
+					     gpio);
+	if (!gpio->domain)
+		return -ENODEV;
+
+	for (i = 0; i < gpio->gpio.ngpio; i++) {
+		irq = irq_create_mapping(gpio->domain, i);
+		if (irq == 0) {
+			dev_err(&pdev->dev,
+				"failed to create IRQ mapping for GPIO#%u\n",
+				i);
+			continue;
+		}
+
+		irq_set_lockdep_class(irq, &tegra186_gpio_lock_class);
+		irq_set_chip_data(irq, gpio);
+		irq_set_chip_and_handler(irq, &gpio->intc, handle_simple_irq);
+	}
+
+	for (i = 0; i < gpio->num_irq; i++)
+		irq_set_chained_handler_and_data(gpio->irq[i],
+						 tegra186_gpio_irq,
+						 gpio);
+
+	return 0;
+}
+
+static int tegra186_gpio_remove(struct platform_device *pdev)
+{
+	struct tegra_gpio *gpio = platform_get_drvdata(pdev);
+	unsigned int i, irq;
+
+	for (i = 0; i < gpio->num_irq; i++)
+		irq_set_chained_handler_and_data(gpio->irq[i], NULL, NULL);
+
+	for (i = 0; i < gpio->gpio.ngpio; i++) {
+		irq = irq_find_mapping(gpio->domain, i);
+		irq_dispose_mapping(irq);
+	}
+
+	irq_domain_remove(gpio->domain);
+
+	return 0;
+}
+
+#define TEGRA_MAIN_GPIO_PORT(port, base, count)	\
+	[TEGRA_MAIN_GPIO_PORT_##port] = {	\
+		.name = #port,			\
+		.offset = base,			\
+		.pins = count,			\
+	}
+
+static const struct tegra_gpio_port tegra186_main_ports[] = {
+	TEGRA_MAIN_GPIO_PORT( A, 0x2000, 7),
+	TEGRA_MAIN_GPIO_PORT( B, 0x3000, 7),
+	TEGRA_MAIN_GPIO_PORT( C, 0x3200, 7),
+	TEGRA_MAIN_GPIO_PORT( D, 0x3400, 6),
+	TEGRA_MAIN_GPIO_PORT( E, 0x2200, 8),
+	TEGRA_MAIN_GPIO_PORT( F, 0x2400, 6),
+	TEGRA_MAIN_GPIO_PORT( G, 0x4200, 6),
+	TEGRA_MAIN_GPIO_PORT( H, 0x1000, 7),
+	TEGRA_MAIN_GPIO_PORT( I, 0x0800, 8),
+	TEGRA_MAIN_GPIO_PORT( J, 0x5000, 8),
+	TEGRA_MAIN_GPIO_PORT( K, 0x5200, 1),
+	TEGRA_MAIN_GPIO_PORT( L, 0x1200, 8),
+	TEGRA_MAIN_GPIO_PORT( M, 0x5600, 6),
+	TEGRA_MAIN_GPIO_PORT( N, 0x0000, 7),
+	TEGRA_MAIN_GPIO_PORT( O, 0x0200, 4),
+	TEGRA_MAIN_GPIO_PORT( P, 0x4000, 7),
+	TEGRA_MAIN_GPIO_PORT( Q, 0x0400, 6),
+	TEGRA_MAIN_GPIO_PORT( R, 0x0a00, 6),
+	TEGRA_MAIN_GPIO_PORT( T, 0x0600, 4),
+	TEGRA_MAIN_GPIO_PORT( X, 0x1400, 8),
+	TEGRA_MAIN_GPIO_PORT( Y, 0x1600, 7),
+	TEGRA_MAIN_GPIO_PORT(BB, 0x2600, 2),
+	TEGRA_MAIN_GPIO_PORT(CC, 0x5400, 4),
+};
+
+static const struct tegra_gpio_soc tegra186_main_soc = {
+	.num_ports = ARRAY_SIZE(tegra186_main_ports),
+	.ports = tegra186_main_ports,
+	.name = "tegra186-gpio",
+};
+
+#define TEGRA_AON_GPIO_PORT(port, base, count)	\
+	[TEGRA_AON_GPIO_PORT_##port] = {	\
+		.name = #port,			\
+		.offset = base,			\
+		.pins = count,			\
+	}
+
+static const struct tegra_gpio_port tegra186_aon_ports[] = {
+	TEGRA_AON_GPIO_PORT( S, 0x0200, 5),
+	TEGRA_AON_GPIO_PORT( U, 0x0400, 6),
+	TEGRA_AON_GPIO_PORT( V, 0x0800, 8),
+	TEGRA_AON_GPIO_PORT( W, 0x0a00, 8),
+	TEGRA_AON_GPIO_PORT( Z, 0x0e00, 4),
+	TEGRA_AON_GPIO_PORT(AA, 0x0c00, 8),
+	TEGRA_AON_GPIO_PORT(EE, 0x0600, 3),
+	TEGRA_AON_GPIO_PORT(FF, 0x0000, 5),
+};
+
+static const struct tegra_gpio_soc tegra186_aon_soc = {
+	.num_ports = ARRAY_SIZE(tegra186_aon_ports),
+	.ports = tegra186_aon_ports,
+	.name = "tegra186-gpio-aon",
+};
+
+static const struct of_device_id tegra186_gpio_of_match[] = {
+	{
+		.compatible = "nvidia,tegra186-gpio",
+		.data = &tegra186_main_soc
+	}, {
+		.compatible = "nvidia,tegra186-gpio-aon",
+		.data = &tegra186_aon_soc
+	}, {
+		/* sentinel */
+	}
+};
+
+static struct platform_driver tegra186_gpio_driver = {
+	.driver = {
+		.name = "tegra186-gpio",
+		.of_match_table = tegra186_gpio_of_match,
+	},
+	.probe = tegra186_gpio_probe,
+	.remove = tegra186_gpio_remove,
+};
+module_platform_driver(tegra186_gpio_driver);
+
+MODULE_DESCRIPTION("NVIDIA Tegra186 GPIO controller driver");
+MODULE_AUTHOR("Thierry Reding <treding@nvidia.com>");
+MODULE_LICENSE("GPL v2");