[PATCH 0/2] gpio: mediatek: driver for gpio chip in MT7621 SoC

[PATCH 0/2] gpio: mediatek: driver for gpio chip in MT7621 SoC

[Sergio Paracuellos]

This patch series add support for gpio driver in mediatek MT7621 
SoC. This driver has been in staging for a while and after some
cleanups cycles we consider to git it a try to get mainlined.

The functionality is presented as a single irq-chip but 3 separate
gpio-chips, as this seemed simplest. Is this acceptable, or does it
need to be changed? We'd like to have review comments and feedback 
specially for this.

Hope this helps and thanks in advance.

Best regards,
    Sergio Paracuellos

Sergio Paracuellos (2):
  gpio: mediatek: add driver for MT7621
  dt-bindings: document gpio-mt7621 bindings

 .../bindings/gpio/mediatek,mt7621-gpio.txt         |  68 ++++
 drivers/gpio/Kconfig                               |   7 +
 drivers/gpio/Makefile                              |   1 +
 drivers/gpio/gpio-mt7621.c                         | 370 +++++++++++++++++++++
 4 files changed, 446 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/mediatek,mt7621-gpio.txt
 create mode 100644 drivers/gpio/gpio-mt7621.c

-- 
2.7.4

[PATCH 1/2] gpio: mediatek: add driver for MT7621

[Sergio Paracuellos]

Add driver support for gpio of MT7621 SoC.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
Reviewed-by: NeilBrown <neil@brown.name>
---
 drivers/gpio/Kconfig       |   7 +
 drivers/gpio/Makefile      |   1 +
 drivers/gpio/gpio-mt7621.c | 370 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 378 insertions(+)
 create mode 100644 drivers/gpio/gpio-mt7621.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 71c0ab4..2646949 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -359,6 +359,13 @@ config GPIO_MPC8XXX
 	  Say Y here if you're going to use hardware that connects to the
 	  MPC512x/831x/834x/837x/8572/8610/QorIQ GPIOs.
 
+config GPIO_MT7621
+	bool "Mediatek GPIO Support"
+	depends on SOC_MT7620 || SOC_MT7621
+	select GPIOLIB_IRQCHIP
+	help
+	  Say yes here to support the Mediatek SoC GPIO device
+
 config GPIO_MVEBU
 	def_bool y
 	depends on PLAT_ORION || ARCH_MVEBU
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 1324c8f..03647ae 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -88,6 +88,7 @@ obj-$(CONFIG_GPIO_MOCKUP)      += gpio-mockup.o
 obj-$(CONFIG_GPIO_MPC5200)	+= gpio-mpc5200.o
 obj-$(CONFIG_GPIO_MPC8XXX)	+= gpio-mpc8xxx.o
 obj-$(CONFIG_GPIO_MSIC)		+= gpio-msic.o
+obj-$(CONFIG_GPIO_MT7621)	+= gpio-mt7621.o
 obj-$(CONFIG_GPIO_MVEBU)        += gpio-mvebu.o
 obj-$(CONFIG_GPIO_MXC)		+= gpio-mxc.o
 obj-$(CONFIG_GPIO_MXS)		+= gpio-mxs.o
diff --git a/drivers/gpio/gpio-mt7621.c b/drivers/gpio/gpio-mt7621.c
new file mode 100644
index 0000000..0c4fb4a
--- /dev/null
+++ b/drivers/gpio/gpio-mt7621.c
@@ -0,0 +1,370 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2009-2011 Gabor Juhos <juhosg@openwrt.org>
+ * Copyright (C) 2013 John Crispin <blogic@openwrt.org>
+ */
+
+#include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/gpio/driver.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irqdomain.h>
+#include <linux/module.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+
+#define MTK_BANK_CNT		3
+#define MTK_BANK_WIDTH		32
+#define PIN_MASK(nr)		(1UL << ((nr % MTK_BANK_WIDTH)))
+
+enum mediatek_gpio_reg {
+	GPIO_REG_CTRL = 0,
+	GPIO_REG_POL,
+	GPIO_REG_DATA,
+	GPIO_REG_DSET,
+	GPIO_REG_DCLR,
+	GPIO_REG_REDGE,
+	GPIO_REG_FEDGE,
+	GPIO_REG_HLVL,
+	GPIO_REG_LLVL,
+	GPIO_REG_STAT,
+	GPIO_REG_EDGE,
+};
+
+struct mtk_gc {
+	struct gpio_chip chip;
+	spinlock_t lock;
+	int bank;
+	u32 rising;
+	u32 falling;
+};
+
+struct mtk_data {
+	void __iomem *gpio_membase;
+	int gpio_irq;
+	struct irq_domain *gpio_irq_domain;
+	struct mtk_gc gc_map[MTK_BANK_CNT];
+};
+
+static inline struct mtk_gc *
+to_mediatek_gpio(struct gpio_chip *chip)
+{
+	return container_of(chip, struct mtk_gc, chip);
+}
+
+static inline void
+mtk_gpio_w32(struct mtk_gc *rg, u8 reg, u32 val)
+{
+	struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
+	u32 offset = (reg * 0x10) + (rg->bank * 0x4);
+
+	iowrite32(val, gpio_data->gpio_membase + offset);
+}
+
+static inline u32
+mtk_gpio_r32(struct mtk_gc *rg, u8 reg)
+{
+	struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
+	u32 offset = (reg * 0x10) + (rg->bank * 0x4);
+
+	return ioread32(gpio_data->gpio_membase + offset);
+}
+
+static void
+mediatek_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
+{
+	struct mtk_gc *rg = to_mediatek_gpio(chip);
+
+	mtk_gpio_w32(rg, (value) ? GPIO_REG_DSET : GPIO_REG_DCLR, BIT(offset));
+}
+
+static int
+mediatek_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct mtk_gc *rg = to_mediatek_gpio(chip);
+
+	return !!(mtk_gpio_r32(rg, GPIO_REG_DATA) & BIT(offset));
+}
+
+static int
+mediatek_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
+{
+	struct mtk_gc *rg = to_mediatek_gpio(chip);
+	unsigned long flags;
+	u32 t;
+
+	spin_lock_irqsave(&rg->lock, flags);
+	t = mtk_gpio_r32(rg, GPIO_REG_CTRL);
+	t &= ~BIT(offset);
+	mtk_gpio_w32(rg, GPIO_REG_CTRL, t);
+	spin_unlock_irqrestore(&rg->lock, flags);
+
+	return 0;
+}
+
+static int
+mediatek_gpio_direction_output(struct gpio_chip *chip,
+					unsigned int offset, int value)
+{
+	struct mtk_gc *rg = to_mediatek_gpio(chip);
+	unsigned long flags;
+	u32 t;
+
+	spin_lock_irqsave(&rg->lock, flags);
+	t = mtk_gpio_r32(rg, GPIO_REG_CTRL);
+	t |= BIT(offset);
+	mtk_gpio_w32(rg, GPIO_REG_CTRL, t);
+	mediatek_gpio_set(chip, offset, value);
+	spin_unlock_irqrestore(&rg->lock, flags);
+
+	return 0;
+}
+
+static int
+mediatek_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
+{
+	struct mtk_gc *rg = to_mediatek_gpio(chip);
+	u32 t = mtk_gpio_r32(rg, GPIO_REG_CTRL);
+
+	return (t & BIT(offset)) ? GPIOF_DIR_OUT : GPIOF_DIR_IN;
+}
+
+static int
+mediatek_gpio_to_irq(struct gpio_chip *chip, unsigned int pin)
+{
+	struct mtk_data *gpio_data = gpiochip_get_data(chip);
+	struct mtk_gc *rg = to_mediatek_gpio(chip);
+
+	return irq_create_mapping(gpio_data->gpio_irq_domain,
+				  pin + (rg->bank * MTK_BANK_WIDTH));
+}
+
+static int
+mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
+{
+	struct mtk_data *gpio_data = dev_get_drvdata(&pdev->dev);
+	const __be32 *id = of_get_property(bank, "reg", NULL);
+	struct mtk_gc *rg;
+	int ret;
+
+	if (!id || be32_to_cpu(*id) >= MTK_BANK_CNT)
+		return -EINVAL;
+
+	rg = &gpio_data->gc_map[be32_to_cpu(*id)];
+	memset(rg, 0, sizeof(*rg));
+
+	spin_lock_init(&rg->lock);
+
+	rg->chip.parent = &pdev->dev;
+	rg->chip.label = dev_name(&pdev->dev);
+	rg->chip.of_node = bank;
+	rg->chip.base = MTK_BANK_WIDTH * be32_to_cpu(*id);
+	rg->chip.ngpio = MTK_BANK_WIDTH;
+	rg->chip.direction_input = mediatek_gpio_direction_input;
+	rg->chip.direction_output = mediatek_gpio_direction_output;
+	rg->chip.get_direction = mediatek_gpio_get_direction;
+	rg->chip.get = mediatek_gpio_get;
+	rg->chip.set = mediatek_gpio_set;
+	if (gpio_data->gpio_irq_domain)
+		rg->chip.to_irq = mediatek_gpio_to_irq;
+	rg->bank = be32_to_cpu(*id);
+
+	ret = devm_gpiochip_add_data(&pdev->dev, &rg->chip, gpio_data);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Could not register gpio %d, ret=%d\n",
+			rg->chip.ngpio, ret);
+		return ret;
+	}
+
+	/* set polarity to low for all gpios */
+	mtk_gpio_w32(rg, GPIO_REG_POL, 0);
+
+	dev_info(&pdev->dev, "registering %d gpios\n", rg->chip.ngpio);
+
+	return 0;
+}
+
+static void
+mediatek_gpio_irq_handler(struct irq_desc *desc)
+{
+	struct mtk_data *gpio_data = irq_desc_get_handler_data(desc);
+	int i;
+
+	for (i = 0; i < MTK_BANK_CNT; i++) {
+		struct mtk_gc *rg = &gpio_data->gc_map[i];
+		unsigned long pending;
+		int bit;
+
+		if (!rg)
+			continue;
+
+		pending = mtk_gpio_r32(rg, GPIO_REG_STAT);
+
+		for_each_set_bit(bit, &pending, MTK_BANK_WIDTH) {
+			u32 map = irq_find_mapping(gpio_data->gpio_irq_domain,
+						   (MTK_BANK_WIDTH * i) + bit);
+
+			generic_handle_irq(map);
+			mtk_gpio_w32(rg, GPIO_REG_STAT, BIT(bit));
+		}
+	}
+}
+
+static void
+mediatek_gpio_irq_unmask(struct irq_data *d)
+{
+	struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
+	int pin = d->hwirq;
+	int bank = pin / MTK_BANK_WIDTH;
+	struct mtk_gc *rg = &gpio_data->gc_map[bank];
+	unsigned long flags;
+	u32 rise, fall;
+
+	if (!rg)
+		return;
+
+	spin_lock_irqsave(&rg->lock, flags);
+	rise = mtk_gpio_r32(rg, GPIO_REG_REDGE);
+	fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE);
+	mtk_gpio_w32(rg, GPIO_REG_REDGE, rise | (PIN_MASK(pin) & rg->rising));
+	mtk_gpio_w32(rg, GPIO_REG_FEDGE, fall | (PIN_MASK(pin) & rg->falling));
+	spin_unlock_irqrestore(&rg->lock, flags);
+}
+
+static void
+mediatek_gpio_irq_mask(struct irq_data *d)
+{
+	struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
+	int pin = d->hwirq;
+	int bank = pin / MTK_BANK_WIDTH;
+	struct mtk_gc *rg = &gpio_data->gc_map[bank];
+	unsigned long flags;
+	u32 rise, fall;
+
+	if (!rg)
+		return;
+
+	spin_lock_irqsave(&rg->lock, flags);
+	rise = mtk_gpio_r32(rg, GPIO_REG_REDGE);
+	fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE);
+	mtk_gpio_w32(rg, GPIO_REG_FEDGE, fall & ~PIN_MASK(pin));
+	mtk_gpio_w32(rg, GPIO_REG_REDGE, rise & ~PIN_MASK(pin));
+	spin_unlock_irqrestore(&rg->lock, flags);
+}
+
+static int
+mediatek_gpio_irq_type(struct irq_data *d, unsigned int type)
+{
+	struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
+	int pin = d->hwirq;
+	int bank = pin / MTK_BANK_WIDTH;
+	struct mtk_gc *rg = &gpio_data->gc_map[bank];
+	u32 mask = PIN_MASK(pin);
+
+	if (!rg)
+		return -1;
+
+	if (type == IRQ_TYPE_PROBE) {
+		if ((rg->rising | rg->falling) & mask)
+			return 0;
+
+		type = IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING;
+	}
+
+	if (type & IRQ_TYPE_EDGE_RISING)
+		rg->rising |= mask;
+	else
+		rg->rising &= ~mask;
+
+	if (type & IRQ_TYPE_EDGE_FALLING)
+		rg->falling |= mask;
+	else
+		rg->falling &= ~mask;
+
+	return 0;
+}
+
+static struct irq_chip mediatek_gpio_irq_chip = {
+	.name		= "GPIO",
+	.irq_unmask	= mediatek_gpio_irq_unmask,
+	.irq_mask	= mediatek_gpio_irq_mask,
+	.irq_mask_ack	= mediatek_gpio_irq_mask,
+	.irq_set_type	= mediatek_gpio_irq_type,
+};
+
+static int
+mediatek_gpio_gpio_map(struct irq_domain *d, unsigned int irq,
+		       irq_hw_number_t hw)
+{
+	int ret;
+
+	ret = irq_set_chip_data(irq, d->host_data);
+	if (ret < 0)
+		return ret;
+	irq_set_chip_and_handler(irq, &mediatek_gpio_irq_chip,
+				 handle_level_irq);
+	irq_set_handler_data(irq, d);
+
+	return 0;
+}
+
+static const struct irq_domain_ops irq_domain_ops = {
+	.xlate = irq_domain_xlate_twocell,
+	.map = mediatek_gpio_gpio_map,
+};
+
+static int
+mediatek_gpio_probe(struct platform_device *pdev)
+{
+	struct device_node *bank, *np = pdev->dev.of_node;
+	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	struct mtk_data *gpio_data;
+
+	gpio_data = devm_kzalloc(&pdev->dev, sizeof(*gpio_data), GFP_KERNEL);
+	if (!gpio_data)
+		return -ENOMEM;
+
+	gpio_data->gpio_membase = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(gpio_data->gpio_membase))
+		return PTR_ERR(gpio_data->gpio_membase);
+
+	gpio_data->gpio_irq = irq_of_parse_and_map(np, 0);
+	if (gpio_data->gpio_irq) {
+		gpio_data->gpio_irq_domain = irq_domain_add_linear(np,
+			MTK_BANK_CNT * MTK_BANK_WIDTH,
+			&irq_domain_ops, gpio_data);
+		if (!gpio_data->gpio_irq_domain)
+			dev_err(&pdev->dev, "irq_domain_add_linear failed\n");
+	}
+
+	platform_set_drvdata(pdev, gpio_data);
+
+	for_each_child_of_node(np, bank)
+		if (of_device_is_compatible(bank, "mediatek,mt7621-gpio-bank"))
+			mediatek_gpio_bank_probe(pdev, bank);
+
+	if (gpio_data->gpio_irq_domain)
+		irq_set_chained_handler_and_data(gpio_data->gpio_irq,
+						 mediatek_gpio_irq_handler,
+						 gpio_data);
+
+	return 0;
+}
+
+static const struct of_device_id mediatek_gpio_match[] = {
+	{ .compatible = "mediatek,mt7621-gpio" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, mediatek_gpio_match);
+
+static struct platform_driver mediatek_gpio_driver = {
+	.probe = mediatek_gpio_probe,
+	.driver = {
+		.name = "mt7621_gpio",
+		.of_match_table = mediatek_gpio_match,
+	},
+};
+
+module_platform_driver(mediatek_gpio_driver);
-- 
2.7.4

Re: [PATCH 1/2] gpio: mediatek: add driver for MT7621

[Linus Walleij]

Hi Sergio!

Thanks for your patch!

Given that we have combined pin control and GPIO drivers for
almost all Mediatek chips in drivers/pinctrl/mediatek/*
I would ideally like to have some input from the Mediatek
maintainers (especially Sean Wang) on this, especially:

- Is MT7621 a non-pincontrol GPIO controller, or can it
  eventually use pin control as a back-end? Will a separate
  pin control driver appear later for this SoC?

- Would it make sense to have a combined driver just like
  for the other Mediatek SoCs in drivers/pinctrl/mediatek?
  If this GPIO controller does not do pin control I understand
  why it is submitted as a GPIO driver only.

drivers/pinctrl/mediatek/pinctrl-mt7622.c is suspiciously
similarly named. Is this a relative or just as different as
night and day?

Also you can see that this driver has a built-in GPIO driver,
using an external interrupt.

On Sat, Jun 2, 2018 at 9:30 AM, Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:

> Add driver support for gpio of MT7621 SoC.
>
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> Reviewed-by: NeilBrown <neil@brown.name>

> +config GPIO_MT7621
> +       bool "Mediatek GPIO Support"

Specify in the option that it is for MT7621 as there are so
many of these now.

> +       depends on SOC_MT7620 || SOC_MT7621

Can we enable COMPILE_TEST?

> +       select GPIOLIB_IRQCHIP

You are not using this so I guess remove that line.

> +       help
> +         Say yes here to support the Mediatek SoC GPIO device

Elaborate on SoC type please.

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

This should not be included in new code, just remove it
(should be fine).

> +#include <linux/gpio/driver.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/irqdomain.h>
> +#include <linux/module.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +
> +#define MTK_BANK_CNT           3
> +#define MTK_BANK_WIDTH         32
> +#define PIN_MASK(nr)           (1UL << ((nr % MTK_BANK_WIDTH)))
> +
> +enum mediatek_gpio_reg {
> +       GPIO_REG_CTRL = 0,
> +       GPIO_REG_POL,
> +       GPIO_REG_DATA,
> +       GPIO_REG_DSET,
> +       GPIO_REG_DCLR,
> +       GPIO_REG_REDGE,
> +       GPIO_REG_FEDGE,
> +       GPIO_REG_HLVL,
> +       GPIO_REG_LLVL,
> +       GPIO_REG_STAT,
> +       GPIO_REG_EDGE,
> +};

So these are all registers? I usually prefer some #defines
for each offset.

In this case I definately think you should define them all
relative the memory base:

#define GPIO_REG_CTRL0 0x00
#define GPIO_REG_CTRL1 0x04
(...)

as that makes it easier to use GPIO_GENERIC as described
below.

> +struct mtk_gc {
> +       struct gpio_chip chip;
> +       spinlock_t lock;
> +       int bank;
> +       u32 rising;
> +       u32 falling;
> +};

> +struct mtk_data {
> +       void __iomem *gpio_membase;
> +       int gpio_irq;
> +       struct irq_domain *gpio_irq_domain;
> +       struct mtk_gc gc_map[MTK_BANK_CNT];
> +};

These two state containers make it a bit confusing, maybe it
can be alleviated with a bit of comments explaining what is
going on?

> +static inline struct mtk_gc *
> +to_mediatek_gpio(struct gpio_chip *chip)
> +{
> +       return container_of(chip, struct mtk_gc, chip);
> +}

This is a bit confusing as the other state container comes
out of the gpio_chip, but that is a member of the mtk_gc.
But maybe this is the only way to do it.

> +static inline void
> +mtk_gpio_w32(struct mtk_gc *rg, u8 reg, u32 val)
> +{
> +       struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
> +       u32 offset = (reg * 0x10) + (rg->bank * 0x4);

So the register stride is 0x10 and the bank is 0x4 wide?

Following this gets terse so I guess that is why I prefer to
just #define the registers and have them relate directly
to membase and no reader/writer functions like this.
But it's not a strong opinion, maybe you have
good reasons for having it like this.

> +       iowrite32(val, gpio_data->gpio_membase + offset);
> +}
> +
> +static inline u32
> +mtk_gpio_r32(struct mtk_gc *rg, u8 reg)
> +{
> +       struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
> +       u32 offset = (reg * 0x10) + (rg->bank * 0x4);
> +
> +       return ioread32(gpio_data->gpio_membase + offset);
> +}

These two could possibly be wrapped into a custom regmap
as well, that makes for nice abstraction. Just an idea.

> +static void
> +mediatek_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
> +{
> +       struct mtk_gc *rg = to_mediatek_gpio(chip);
> +
> +       mtk_gpio_w32(rg, (value) ? GPIO_REG_DSET : GPIO_REG_DCLR, BIT(offset));
> +}
> +
> +static int
> +mediatek_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct mtk_gc *rg = to_mediatek_gpio(chip);
> +
> +       return !!(mtk_gpio_r32(rg, GPIO_REG_DATA) & BIT(offset));
> +}
> +
> +static int
> +mediatek_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct mtk_gc *rg = to_mediatek_gpio(chip);
> +       unsigned long flags;
> +       u32 t;
> +
> +       spin_lock_irqsave(&rg->lock, flags);
> +       t = mtk_gpio_r32(rg, GPIO_REG_CTRL);
> +       t &= ~BIT(offset);
> +       mtk_gpio_w32(rg, GPIO_REG_CTRL, t);
> +       spin_unlock_irqrestore(&rg->lock, flags);
> +
> +       return 0;
> +}
> +
> +static int
> +mediatek_gpio_direction_output(struct gpio_chip *chip,
> +                                       unsigned int offset, int value)
> +{
> +       struct mtk_gc *rg = to_mediatek_gpio(chip);
> +       unsigned long flags;
> +       u32 t;
> +
> +       spin_lock_irqsave(&rg->lock, flags);
> +       t = mtk_gpio_r32(rg, GPIO_REG_CTRL);
> +       t |= BIT(offset);
> +       mtk_gpio_w32(rg, GPIO_REG_CTRL, t);
> +       mediatek_gpio_set(chip, offset, value);
> +       spin_unlock_irqrestore(&rg->lock, flags);
> +
> +       return 0;
> +}
> +
> +static int
> +mediatek_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct mtk_gc *rg = to_mediatek_gpio(chip);
> +       u32 t = mtk_gpio_r32(rg, GPIO_REG_CTRL);
> +
> +       return (t & BIT(offset)) ? GPIOF_DIR_OUT : GPIOF_DIR_IN;
> +}

How do these calls end up?

If this complexity is just masking the fact that offset is always
0..n and writes to bits 0..n of some memory address, this whole
thing can just me converted to use GPIO_GENERIC and replace all
the code from mtk_gpio_w32() to here.

Given that MTK_BANK_WIDTH is 32, I think it's very likely that
you can just use GPIO_GENERIC and send in the memory
offsets for all these registers to bgpio_init().

As bonus you get a proven implementation supporting
.get/set_multiple() at the same time.

> +static int
> +mediatek_gpio_to_irq(struct gpio_chip *chip, unsigned int pin)
> +{
> +       struct mtk_data *gpio_data = gpiochip_get_data(chip);
> +       struct mtk_gc *rg = to_mediatek_gpio(chip);
> +
> +       return irq_create_mapping(gpio_data->gpio_irq_domain,
> +                                 pin + (rg->bank * MTK_BANK_WIDTH));
> +}

So this is the result of a custom IRQdomain because you
can't use the generic GPIO IRQ lib.  Oh well, we have to live
with it I guess.

> +static int
> +mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
> +{
> +       struct mtk_data *gpio_data = dev_get_drvdata(&pdev->dev);
> +       const __be32 *id = of_get_property(bank, "reg", NULL);
> +       struct mtk_gc *rg;
> +       int ret;
> +
> +       if (!id || be32_to_cpu(*id) >= MTK_BANK_CNT)
> +               return -EINVAL;
> +
> +       rg = &gpio_data->gc_map[be32_to_cpu(*id)];
> +       memset(rg, 0, sizeof(*rg));
> +
> +       spin_lock_init(&rg->lock);
> +
> +       rg->chip.parent = &pdev->dev;
> +       rg->chip.label = dev_name(&pdev->dev);
> +       rg->chip.of_node = bank;
> +       rg->chip.base = MTK_BANK_WIDTH * be32_to_cpu(*id);

Nope, do not ever assign base in a new driver. Set this to -1.

New systems should use dynamic IRQ base assignment, and
userspace should use the character device to access GPIOs
if need be.

> +       rg->chip.ngpio = MTK_BANK_WIDTH;
> +       rg->chip.direction_input = mediatek_gpio_direction_input;
> +       rg->chip.direction_output = mediatek_gpio_direction_output;
> +       rg->chip.get_direction = mediatek_gpio_get_direction;
> +       rg->chip.get = mediatek_gpio_get;
> +       rg->chip.set = mediatek_gpio_set;
> +       if (gpio_data->gpio_irq_domain)
> +               rg->chip.to_irq = mediatek_gpio_to_irq;
> +       rg->bank = be32_to_cpu(*id);

As stated I think this can be replaced with bgpio_init() and
selecting GPIOCHIP_GENERIC.

> +static void
> +mediatek_gpio_irq_handler(struct irq_desc *desc)
> +{
> +       struct mtk_data *gpio_data = irq_desc_get_handler_data(desc);
> +       int i;
> +
> +       for (i = 0; i < MTK_BANK_CNT; i++) {
> +               struct mtk_gc *rg = &gpio_data->gc_map[i];
> +               unsigned long pending;
> +               int bit;
> +
> +               if (!rg)
> +                       continue;
> +
> +               pending = mtk_gpio_r32(rg, GPIO_REG_STAT);
> +
> +               for_each_set_bit(bit, &pending, MTK_BANK_WIDTH) {
> +                       u32 map = irq_find_mapping(gpio_data->gpio_irq_domain,
> +                                                  (MTK_BANK_WIDTH * i) + bit);
> +
> +                       generic_handle_irq(map);
> +                       mtk_gpio_w32(rg, GPIO_REG_STAT, BIT(bit));
> +               }
> +       }
> +}
> +
> +static void
> +mediatek_gpio_irq_unmask(struct irq_data *d)
> +{
> +       struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
> +       int pin = d->hwirq;
> +       int bank = pin / MTK_BANK_WIDTH;
> +       struct mtk_gc *rg = &gpio_data->gc_map[bank];
> +       unsigned long flags;
> +       u32 rise, fall;
> +
> +       if (!rg)
> +               return;
> +
> +       spin_lock_irqsave(&rg->lock, flags);
> +       rise = mtk_gpio_r32(rg, GPIO_REG_REDGE);
> +       fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE);
> +       mtk_gpio_w32(rg, GPIO_REG_REDGE, rise | (PIN_MASK(pin) & rg->rising));
> +       mtk_gpio_w32(rg, GPIO_REG_FEDGE, fall | (PIN_MASK(pin) & rg->falling));
> +       spin_unlock_irqrestore(&rg->lock, flags);
> +}
> +
> +static void
> +mediatek_gpio_irq_mask(struct irq_data *d)
> +{
> +       struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
> +       int pin = d->hwirq;
> +       int bank = pin / MTK_BANK_WIDTH;
> +       struct mtk_gc *rg = &gpio_data->gc_map[bank];
> +       unsigned long flags;
> +       u32 rise, fall;
> +
> +       if (!rg)
> +               return;
> +
> +       spin_lock_irqsave(&rg->lock, flags);
> +       rise = mtk_gpio_r32(rg, GPIO_REG_REDGE);
> +       fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE);
> +       mtk_gpio_w32(rg, GPIO_REG_FEDGE, fall & ~PIN_MASK(pin));
> +       mtk_gpio_w32(rg, GPIO_REG_REDGE, rise & ~PIN_MASK(pin));
> +       spin_unlock_irqrestore(&rg->lock, flags);
> +}


Looks OK.

> +static int
> +mediatek_gpio_irq_type(struct irq_data *d, unsigned int type)
> +{
> +       struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
> +       int pin = d->hwirq;
> +       int bank = pin / MTK_BANK_WIDTH;
> +       struct mtk_gc *rg = &gpio_data->gc_map[bank];
> +       u32 mask = PIN_MASK(pin);
> +
> +       if (!rg)
> +               return -1;
> +
> +       if (type == IRQ_TYPE_PROBE) {
> +               if ((rg->rising | rg->falling) & mask)
> +                       return 0;
> +
> +               type = IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING;
> +       }
> +
> +       if (type & IRQ_TYPE_EDGE_RISING)
> +               rg->rising |= mask;
> +       else
> +               rg->rising &= ~mask;
> +
> +       if (type & IRQ_TYPE_EDGE_FALLING)
> +               rg->falling |= mask;
> +       else
> +               rg->falling &= ~mask;

I don't understand this, the register map contains:
GPIO_REG_HLVL, GPIO_REG_LLVL, yet high/low level
interrupts are not implemented? Why not? Can't be that hard
now that you fixed everything else!

> +static struct irq_chip mediatek_gpio_irq_chip = {
> +       .name           = "GPIO",
> +       .irq_unmask     = mediatek_gpio_irq_unmask,
> +       .irq_mask       = mediatek_gpio_irq_mask,
> +       .irq_mask_ack   = mediatek_gpio_irq_mask,
> +       .irq_set_type   = mediatek_gpio_irq_type,
> +};

When implementing custom irqchips it is important to also
implement .irq_request_resources() and
.irq_release_resources() and make sure these call
gpiochip_[un]lock_as_irq().

See for example
drivers/gpio/gpio-dwapb.c

for an example.

> +static const struct of_device_id mediatek_gpio_match[] = {
> +       { .compatible = "mediatek,mt7621-gpio" },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(of, mediatek_gpio_match);
> +
> +static struct platform_driver mediatek_gpio_driver = {
> +       .probe = mediatek_gpio_probe,
> +       .driver = {
> +               .name = "mt7621_gpio",
> +               .of_match_table = mediatek_gpio_match,
> +       },
> +};
> +
> +module_platform_driver(mediatek_gpio_driver);

If you're not implementing .remove() I don't think this will
really work fine as a module. Also the Kconfig is a bool.

I guess you want to use
builtin_platform_driver()?

Yours,
Linus Walleij

Re: [PATCH 1/2] gpio: mediatek: add driver for MT7621

[Linus Walleij]

A second thought:

On Fri, Jun 8, 2018 at 1:59 PM, Linus Walleij <linus.walleij@linaro.org> wrote:

>> +       select GPIOLIB_IRQCHIP
>
> You are not using this so I guess remove that line.
(...)
>> +static int
>> +mediatek_gpio_to_irq(struct gpio_chip *chip, unsigned int pin)
>> +{
>> +       struct mtk_data *gpio_data = gpiochip_get_data(chip);
>> +       struct mtk_gc *rg = to_mediatek_gpio(chip);
>> +
>> +       return irq_create_mapping(gpio_data->gpio_irq_domain,
>> +                                 pin + (rg->bank * MTK_BANK_WIDTH));
>> +}
>
> So this is the result of a custom IRQdomain because you
> can't use the generic GPIO IRQ lib.  Oh well, we have to live
> with it I guess.

I think maybe you can actually use the generic GPIO IRQCHIP.

It is OK to call gpiochip_set_chained_irqchip() several times.
If you just mark the line with IRQF_SHARED then the handler
will just loop over all three banks until you find a hit, provided
you code it up properly.

There were some problems with removing an irqchip like
that but your driver is anyway a bool so I think it might work
just fine.

Yours,
Linus Walleij

Re: [PATCH 1/2] gpio: mediatek: add driver for MT7621

[Sergio Paracuellos]

On Fri, Jun 8, 2018 at 2:17 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> A second thought:

Second thought is always a good thing :-)

>
> On Fri, Jun 8, 2018 at 1:59 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>
>>> +       select GPIOLIB_IRQCHIP
>>
>> You are not using this so I guess remove that line.
> (...)
>>> +static int
>>> +mediatek_gpio_to_irq(struct gpio_chip *chip, unsigned int pin)
>>> +{
>>> +       struct mtk_data *gpio_data = gpiochip_get_data(chip);
>>> +       struct mtk_gc *rg = to_mediatek_gpio(chip);
>>> +
>>> +       return irq_create_mapping(gpio_data->gpio_irq_domain,
>>> +                                 pin + (rg->bank * MTK_BANK_WIDTH));
>>> +}
>>
>> So this is the result of a custom IRQdomain because you
>> can't use the generic GPIO IRQ lib.  Oh well, we have to live
>> with it I guess.
>
> I think maybe you can actually use the generic GPIO IRQCHIP.
>
> It is OK to call gpiochip_set_chained_irqchip() several times.
> If you just mark the line with IRQF_SHARED then the handler
> will just loop over all three banks until you find a hit, provided
> you code it up properly.

Ok, I'll give it a try to use this also.

>
> There were some problems with removing an irqchip like
> that but your driver is anyway a bool so I think it might work
> just fine.
>
> Yours,
> Linus Walleij

Thanks again.

Best regards,
    Sergio Paracuellos

Re: [PATCH 1/2] gpio: mediatek: add driver for MT7621

[Sergio Paracuellos]

On Fri, Jun 8, 2018 at 1:59 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> Hi Sergio!
>
> Thanks for your patch!

Hi Linus,

First of all, thanks for your feedback and effort reviewing this.

>
> On Sat, Jun 2, 2018 at 9:30 AM, Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
>
>> Add driver support for gpio of MT7621 SoC.
>>
>> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
>> Reviewed-by: NeilBrown <neil@brown.name>
>
>> +config GPIO_MT7621
>> +       bool "Mediatek GPIO Support"
>
> Specify in the option that it is for MT7621 as there are so
> many of these now.

I see. I will.

>
>> +       depends on SOC_MT7620 || SOC_MT7621
>
> Can we enable COMPILE_TEST?

Sure.

>
>> +       select GPIOLIB_IRQCHIP
>
> You are not using this so I guess remove that line.
>
>> +       help
>> +         Say yes here to support the Mediatek SoC GPIO device
>
> Elaborate on SoC type please.
>
>> +#include <linux/err.h>
>> +#include <linux/gpio.h>
>
> This should not be included in new code, just remove it
> (should be fine).

If I am correct this header contains GPIOF_DIR_OUT and GPIOF_DIR_IN
definitions and mediatek_gpio_set function was using them. That's the
reason why this was included.

>
>> +#include <linux/gpio/driver.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/module.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/spinlock.h>
>> +
>> +#define MTK_BANK_CNT           3
>> +#define MTK_BANK_WIDTH         32
>> +#define PIN_MASK(nr)           (1UL << ((nr % MTK_BANK_WIDTH)))
>> +
>> +enum mediatek_gpio_reg {
>> +       GPIO_REG_CTRL = 0,
>> +       GPIO_REG_POL,
>> +       GPIO_REG_DATA,
>> +       GPIO_REG_DSET,
>> +       GPIO_REG_DCLR,
>> +       GPIO_REG_REDGE,
>> +       GPIO_REG_FEDGE,
>> +       GPIO_REG_HLVL,
>> +       GPIO_REG_LLVL,
>> +       GPIO_REG_STAT,
>> +       GPIO_REG_EDGE,
>> +};
>
> So these are all registers? I usually prefer some #defines
> for each offset.
>
> In this case I definately think you should define them all
> relative the memory base:
>
> #define GPIO_REG_CTRL0 0x00
> #define GPIO_REG_CTRL1 0x04
> (...)
>
> as that makes it easier to use GPIO_GENERIC as described
> below.

I see. Agreed. Will change them.

>
>> +struct mtk_gc {
>> +       struct gpio_chip chip;
>> +       spinlock_t lock;
>> +       int bank;
>> +       u32 rising;
>> +       u32 falling;
>> +};
>
>> +struct mtk_data {
>> +       void __iomem *gpio_membase;
>> +       int gpio_irq;
>> +       struct irq_domain *gpio_irq_domain;
>> +       struct mtk_gc gc_map[MTK_BANK_CNT];
>> +};
>
> These two state containers make it a bit confusing, maybe it
> can be alleviated with a bit of comments explaining what is
> going on?

Sure.

>
>> +static inline struct mtk_gc *
>> +to_mediatek_gpio(struct gpio_chip *chip)
>> +{
>> +       return container_of(chip, struct mtk_gc, chip);
>> +}
>
> This is a bit confusing as the other state container comes
> out of the gpio_chip, but that is a member of the mtk_gc.
> But maybe this is the only way to do it.

With the comments in the state containers should be more clear.

>
>> +static inline void
>> +mtk_gpio_w32(struct mtk_gc *rg, u8 reg, u32 val)
>> +{
>> +       struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
>> +       u32 offset = (reg * 0x10) + (rg->bank * 0x4);
>
> So the register stride is 0x10 and the bank is 0x4 wide?
>
> Following this gets terse so I guess that is why I prefer to
> just #define the registers and have them relate directly
> to membase and no reader/writer functions like this.
> But it's not a strong opinion, maybe you have
> good reasons for having it like this.

Not really. The original code was as it is here and with my
cleanups I didn't make that kind of changes. And it make sense
to change them because readability gets improve in the way you are
pointing out here.

>
>> +       iowrite32(val, gpio_data->gpio_membase + offset);
>> +}
>> +
>> +static inline u32
>> +mtk_gpio_r32(struct mtk_gc *rg, u8 reg)
>> +{
>> +       struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
>> +       u32 offset = (reg * 0x10) + (rg->bank * 0x4);
>> +
>> +       return ioread32(gpio_data->gpio_membase + offset);
>> +}
>
> These two could possibly be wrapped into a custom regmap
> as well, that makes for nice abstraction. Just an idea.
>
>> +static void
>> +mediatek_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
>> +{
>> +       struct mtk_gc *rg = to_mediatek_gpio(chip);
>> +
>> +       mtk_gpio_w32(rg, (value) ? GPIO_REG_DSET : GPIO_REG_DCLR, BIT(offset));
>> +}
>> +
>> +static int
>> +mediatek_gpio_get(struct gpio_chip *chip, unsigned int offset)
>> +{
>> +       struct mtk_gc *rg = to_mediatek_gpio(chip);
>> +
>> +       return !!(mtk_gpio_r32(rg, GPIO_REG_DATA) & BIT(offset));
>> +}
>> +
>> +static int
>> +mediatek_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
>> +{
>> +       struct mtk_gc *rg = to_mediatek_gpio(chip);
>> +       unsigned long flags;
>> +       u32 t;
>> +
>> +       spin_lock_irqsave(&rg->lock, flags);
>> +       t = mtk_gpio_r32(rg, GPIO_REG_CTRL);
>> +       t &= ~BIT(offset);
>> +       mtk_gpio_w32(rg, GPIO_REG_CTRL, t);
>> +       spin_unlock_irqrestore(&rg->lock, flags);
>> +
>> +       return 0;
>> +}
>> +
>> +static int
>> +mediatek_gpio_direction_output(struct gpio_chip *chip,
>> +                                       unsigned int offset, int value)
>> +{
>> +       struct mtk_gc *rg = to_mediatek_gpio(chip);
>> +       unsigned long flags;
>> +       u32 t;
>> +
>> +       spin_lock_irqsave(&rg->lock, flags);
>> +       t = mtk_gpio_r32(rg, GPIO_REG_CTRL);
>> +       t |= BIT(offset);
>> +       mtk_gpio_w32(rg, GPIO_REG_CTRL, t);
>> +       mediatek_gpio_set(chip, offset, value);
>> +       spin_unlock_irqrestore(&rg->lock, flags);
>> +
>> +       return 0;
>> +}
>> +
>> +static int
>> +mediatek_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
>> +{
>> +       struct mtk_gc *rg = to_mediatek_gpio(chip);
>> +       u32 t = mtk_gpio_r32(rg, GPIO_REG_CTRL);
>> +
>> +       return (t & BIT(offset)) ? GPIOF_DIR_OUT : GPIOF_DIR_IN;
>> +}
>
> How do these calls end up?
>
> If this complexity is just masking the fact that offset is always
> 0..n and writes to bits 0..n of some memory address, this whole
> thing can just me converted to use GPIO_GENERIC and replace all
> the code from mtk_gpio_w32() to here.
>
> Given that MTK_BANK_WIDTH is 32, I think it's very likely that
> you can just use GPIO_GENERIC and send in the memory
> offsets for all these registers to bgpio_init().

I'll give it a try using this.

>
> As bonus you get a proven implementation supporting
> .get/set_multiple() at the same time.
>
>> +static int
>> +mediatek_gpio_to_irq(struct gpio_chip *chip, unsigned int pin)
>> +{
>> +       struct mtk_data *gpio_data = gpiochip_get_data(chip);
>> +       struct mtk_gc *rg = to_mediatek_gpio(chip);
>> +
>> +       return irq_create_mapping(gpio_data->gpio_irq_domain,
>> +                                 pin + (rg->bank * MTK_BANK_WIDTH));
>> +}
>
> So this is the result of a custom IRQdomain because you
> can't use the generic GPIO IRQ lib.  Oh well, we have to live
> with it I guess.
>
>> +static int
>> +mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
>> +{
>> +       struct mtk_data *gpio_data = dev_get_drvdata(&pdev->dev);
>> +       const __be32 *id = of_get_property(bank, "reg", NULL);
>> +       struct mtk_gc *rg;
>> +       int ret;
>> +
>> +       if (!id || be32_to_cpu(*id) >= MTK_BANK_CNT)
>> +               return -EINVAL;
>> +
>> +       rg = &gpio_data->gc_map[be32_to_cpu(*id)];
>> +       memset(rg, 0, sizeof(*rg));
>> +
>> +       spin_lock_init(&rg->lock);
>> +
>> +       rg->chip.parent = &pdev->dev;
>> +       rg->chip.label = dev_name(&pdev->dev);
>> +       rg->chip.of_node = bank;
>> +       rg->chip.base = MTK_BANK_WIDTH * be32_to_cpu(*id);
>
> Nope, do not ever assign base in a new driver. Set this to -1.
>
> New systems should use dynamic IRQ base assignment, and
> userspace should use the character device to access GPIOs
> if need be.
>

Thanks for your explanation in this.

>> +       rg->chip.ngpio = MTK_BANK_WIDTH;
>> +       rg->chip.direction_input = mediatek_gpio_direction_input;
>> +       rg->chip.direction_output = mediatek_gpio_direction_output;
>> +       rg->chip.get_direction = mediatek_gpio_get_direction;
>> +       rg->chip.get = mediatek_gpio_get;
>> +       rg->chip.set = mediatek_gpio_set;
>> +       if (gpio_data->gpio_irq_domain)
>> +               rg->chip.to_irq = mediatek_gpio_to_irq;
>> +       rg->bank = be32_to_cpu(*id);
>
> As stated I think this can be replaced with bgpio_init() and
> selecting GPIOCHIP_GENERIC.
>
If it works much more clean with that, yes. Thanks.

>> +static void
>> +mediatek_gpio_irq_handler(struct irq_desc *desc)
>> +{
>> +       struct mtk_data *gpio_data = irq_desc_get_handler_data(desc);
>> +       int i;
>> +
>> +       for (i = 0; i < MTK_BANK_CNT; i++) {
>> +               struct mtk_gc *rg = &gpio_data->gc_map[i];
>> +               unsigned long pending;
>> +               int bit;
>> +
>> +               if (!rg)
>> +                       continue;
>> +
>> +               pending = mtk_gpio_r32(rg, GPIO_REG_STAT);
>> +
>> +               for_each_set_bit(bit, &pending, MTK_BANK_WIDTH) {
>> +                       u32 map = irq_find_mapping(gpio_data->gpio_irq_domain,
>> +                                                  (MTK_BANK_WIDTH * i) + bit);
>> +
>> +                       generic_handle_irq(map);
>> +                       mtk_gpio_w32(rg, GPIO_REG_STAT, BIT(bit));
>> +               }
>> +       }
>> +}
>> +
>> +static void
>> +mediatek_gpio_irq_unmask(struct irq_data *d)
>> +{
>> +       struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
>> +       int pin = d->hwirq;
>> +       int bank = pin / MTK_BANK_WIDTH;
>> +       struct mtk_gc *rg = &gpio_data->gc_map[bank];
>> +       unsigned long flags;
>> +       u32 rise, fall;
>> +
>> +       if (!rg)
>> +               return;
>> +
>> +       spin_lock_irqsave(&rg->lock, flags);
>> +       rise = mtk_gpio_r32(rg, GPIO_REG_REDGE);
>> +       fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE);
>> +       mtk_gpio_w32(rg, GPIO_REG_REDGE, rise | (PIN_MASK(pin) & rg->rising));
>> +       mtk_gpio_w32(rg, GPIO_REG_FEDGE, fall | (PIN_MASK(pin) & rg->falling));
>> +       spin_unlock_irqrestore(&rg->lock, flags);
>> +}
>> +
>> +static void
>> +mediatek_gpio_irq_mask(struct irq_data *d)
>> +{
>> +       struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
>> +       int pin = d->hwirq;
>> +       int bank = pin / MTK_BANK_WIDTH;
>> +       struct mtk_gc *rg = &gpio_data->gc_map[bank];
>> +       unsigned long flags;
>> +       u32 rise, fall;
>> +
>> +       if (!rg)
>> +               return;
>> +
>> +       spin_lock_irqsave(&rg->lock, flags);
>> +       rise = mtk_gpio_r32(rg, GPIO_REG_REDGE);
>> +       fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE);
>> +       mtk_gpio_w32(rg, GPIO_REG_FEDGE, fall & ~PIN_MASK(pin));
>> +       mtk_gpio_w32(rg, GPIO_REG_REDGE, rise & ~PIN_MASK(pin));
>> +       spin_unlock_irqrestore(&rg->lock, flags);
>> +}
>
>
> Looks OK.
>
>> +static int
>> +mediatek_gpio_irq_type(struct irq_data *d, unsigned int type)
>> +{
>> +       struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
>> +       int pin = d->hwirq;
>> +       int bank = pin / MTK_BANK_WIDTH;
>> +       struct mtk_gc *rg = &gpio_data->gc_map[bank];
>> +       u32 mask = PIN_MASK(pin);
>> +
>> +       if (!rg)
>> +               return -1;
>> +
>> +       if (type == IRQ_TYPE_PROBE) {
>> +               if ((rg->rising | rg->falling) & mask)
>> +                       return 0;
>> +
>> +               type = IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING;
>> +       }
>> +
>> +       if (type & IRQ_TYPE_EDGE_RISING)
>> +               rg->rising |= mask;
>> +       else
>> +               rg->rising &= ~mask;
>> +
>> +       if (type & IRQ_TYPE_EDGE_FALLING)
>> +               rg->falling |= mask;
>> +       else
>> +               rg->falling &= ~mask;
>
> I don't understand this, the register map contains:
> GPIO_REG_HLVL, GPIO_REG_LLVL, yet high/low level
> interrupts are not implemented? Why not? Can't be that hard
> now that you fixed everything else!

The original code didn't has support for them so I
didn't add them also. But if they are supported in the chip,
I think those two should be added also.

>
>> +static struct irq_chip mediatek_gpio_irq_chip = {
>> +       .name           = "GPIO",
>> +       .irq_unmask     = mediatek_gpio_irq_unmask,
>> +       .irq_mask       = mediatek_gpio_irq_mask,
>> +       .irq_mask_ack   = mediatek_gpio_irq_mask,
>> +       .irq_set_type   = mediatek_gpio_irq_type,
>> +};
>
> When implementing custom irqchips it is important to also
> implement .irq_request_resources() and
> .irq_release_resources() and make sure these call
> gpiochip_[un]lock_as_irq().
>
> See for example
> drivers/gpio/gpio-dwapb.c
>
> for an example.

I will, thanks.

>
>> +static const struct of_device_id mediatek_gpio_match[] = {
>> +       { .compatible = "mediatek,mt7621-gpio" },
>> +       {},
>> +};
>> +MODULE_DEVICE_TABLE(of, mediatek_gpio_match);
>> +
>> +static struct platform_driver mediatek_gpio_driver = {
>> +       .probe = mediatek_gpio_probe,
>> +       .driver = {
>> +               .name = "mt7621_gpio",
>> +               .of_match_table = mediatek_gpio_match,
>> +       },
>> +};
>> +
>> +module_platform_driver(mediatek_gpio_driver);
>
> If you're not implementing .remove() I don't think this will
> really work fine as a module. Also the Kconfig is a bool.
>
> I guess you want to use
> builtin_platform_driver()?

Yes, That should be definitely the one to use :-).

>
> Yours,
> Linus Walleij

I think the best way to achieve this is make all of this changes in
the actual staging driver sending a new cleanups patch series
in order to let NeilBrown to can test them and when we think this is
clean enough again send you v2 with all of the changes pointed out here.
So... it is up to you, Neil the way to go here. Just let me know.

Thanks in advance.

Best regards,
    Sergio Paracuellos

Re: [PATCH 1/2] gpio: mediatek: add driver for MT7621

[Sean Wang]

Hi,

On Fri, 2018-06-08 at 13:59 +0200, Linus Walleij wrote:
> Hi Sergio!
> 
> Thanks for your patch!
> 
> Given that we have combined pin control and GPIO drivers for
> almost all Mediatek chips in drivers/pinctrl/mediatek/*
> I would ideally like to have some input from the Mediatek
> maintainers (especially Sean Wang) on this, especially:
> 
> - Is MT7621 a non-pincontrol GPIO controller, or can it
>   eventually use pin control as a back-end? Will a separate
>   pin control driver appear later for this SoC?
> 

MT7621 also have the circuit for pad setup tweaking Tx driving and mux
setup switching to either gpio mode or specific hardware mode, but the
circuit for all of them is being accessed in a different register range
from gpio controller being implemented here.

the part of pad or mux control for MT7621 I thought that is really
simple, so it seems worth joining pinmux and pinconf together into a
single driver to become a full function about pin setup. 

> - Would it make sense to have a combined driver just like
>   for the other Mediatek SoCs in drivers/pinctrl/mediatek?
>   If this GPIO controller does not do pin control I understand
>   why it is submitted as a GPIO driver only.
> 
> drivers/pinctrl/mediatek/pinctrl-mt7622.c is suspiciously
> similarly named. Is this a relative or just as different as
> night and day?
> 

The MT7621 is just as completely different as night and day from MT7622.

MT7622 pinctrl originate from MediaTek IPs but MT7621 pinctrl originate
from Ralink IPs and even MT7621 should be the last one machine using the
Ralink pinctrl IPs. And for these machine MT762x appearing later MT7622,
they all will be developed based on pinctrl-mt7622 architecture .

> Also you can see that this driver has a built-in GPIO driver,
> using an external interrupt.
> 
> On Sat, Jun 2, 2018 at 9:30 AM, Sergio Paracuellos

[ ... ]

> I guess you want to use
> builtin_platform_driver()?
> 
> Yours,
> Linus Walleij

[PATCH 2/2] dt-bindings: document gpio-mt7621 bindings

[Sergio Paracuellos]

Add a devicetree binding documentation for the mt7621 driver.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
Reviewed-by: NeilBrown <neil@brown.name>
---
 .../bindings/gpio/mediatek,mt7621-gpio.txt         | 68 ++++++++++++++++++++++
 1 file changed, 68 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/mediatek,mt7621-gpio.txt

diff --git a/Documentation/devicetree/bindings/gpio/mediatek,mt7621-gpio.txt b/Documentation/devicetree/bindings/gpio/mediatek,mt7621-gpio.txt
new file mode 100644
index 0000000..30d8a02
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/mediatek,mt7621-gpio.txt
@@ -0,0 +1,68 @@
+Mediatek SoC GPIO controller bindings
+
+The IP core used inside these SoCs has 3 banks of 32 GPIOs each.
+The registers of all the banks are interwoven inside one single IO range.
+We load one GPIO controller instance per bank. To make this possible
+we support 2 types of nodes. The parent node defines the memory I/O range and
+has 3 children each describing a single bank. Also the GPIO controller can receive
+interrupts on any of the GPIOs, either edge or level. It then interrupts the CPU
+using GIC INT12.
+
+Required properties for the top level node:
+- compatible:
+  - "mediatek,mt7621-gpio" for Mediatek controllers
+- reg : Physical base address and length of the controller's registers
+- interrupt-parent : phandle of the parent interrupt controller.
+- interrupts : Interrupt specifier for the controllers interrupt.
+- interrupt-controller : Mark the device node as an interrupt controller.
+- #interrupt-cells : Should be 2. The first cell defines the interrupt number.
+   The second cell bits[3:0] is used to specify trigger type as follows:
+	- 1 = low-to-high edge triggered.
+	- 2 = high-to-low edge triggered.
+	- 4 = active high level-sensitive.
+	- 8 = active low level-sensitive.
+
+
+Required properties for the GPIO bank node:
+- compatible:
+  - "mediatek,mt7621-gpio-bank" for Mediatek banks
+- #gpio-cells : Should be two. The first cell is the GPIO pin number and the
+   second cell specifies GPIO flags, as defined in <dt-bindings/gpio/gpio.h>.
+   Only the GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags are supported.
+- gpio-controller : Marks the device node as a GPIO controller.
+- reg : The id of the bank that the node describes.
+
+Example:
+	gpio@600 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		compatible = "mediatek,mt7621-gpio";
+		reg = <0x600 0x100>;
+
+		interrupt-parent = <&gic>;
+		interrupts = <GIC_SHARED 12 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
+
+		gpio0: bank@0 {
+			reg = <0>;
+			compatible = "mediatek,mt7621-gpio-bank";
+			gpio-controller;
+			#gpio-cells = <2>;
+		};
+
+		gpio1: bank@1 {
+			reg = <1>;
+			compatible = "mediatek,mt7621-gpio-bank";
+			gpio-controller;
+			#gpio-cells = <2>;
+		};
+
+		gpio2: bank@2 {
+			reg = <2>;
+			compatible = "mediatek,mt7621-gpio-bank";
+			gpio-controller;
+			#gpio-cells = <2>;
+		};
+	};
-- 
2.7.4

Re: [PATCH 2/2] dt-bindings: document gpio-mt7621 bindings

[Rob Herring]

On Sat, Jun 02, 2018 at 09:30:10AM +0200, Sergio Paracuellos wrote:
> Add a devicetree binding documentation for the mt7621 driver.

Bindings are for h/w, not a driver.

> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> Reviewed-by: NeilBrown <neil@brown.name>

Space              ^

> ---
>  .../bindings/gpio/mediatek,mt7621-gpio.txt         | 68 ++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/mediatek,mt7621-gpio.txt
> 
> diff --git a/Documentation/devicetree/bindings/gpio/mediatek,mt7621-gpio.txt b/Documentation/devicetree/bindings/gpio/mediatek,mt7621-gpio.txt
> new file mode 100644
> index 0000000..30d8a02
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/mediatek,mt7621-gpio.txt
> @@ -0,0 +1,68 @@
> +Mediatek SoC GPIO controller bindings
> +
> +The IP core used inside these SoCs has 3 banks of 32 GPIOs each.
> +The registers of all the banks are interwoven inside one single IO range.
> +We load one GPIO controller instance per bank. To make this possible
> +we support 2 types of nodes. The parent node defines the memory I/O range and
> +has 3 children each describing a single bank. Also the GPIO controller can receive
> +interrupts on any of the GPIOs, either edge or level. It then interrupts the CPU
> +using GIC INT12.
> +
> +Required properties for the top level node:
> +- compatible:
> +  - "mediatek,mt7621-gpio" for Mediatek controllers
> +- reg : Physical base address and length of the controller's registers
> +- interrupt-parent : phandle of the parent interrupt controller.
> +- interrupts : Interrupt specifier for the controllers interrupt.
> +- interrupt-controller : Mark the device node as an interrupt controller.
> +- #interrupt-cells : Should be 2. The first cell defines the interrupt number.
> +   The second cell bits[3:0] is used to specify trigger type as follows:
> +	- 1 = low-to-high edge triggered.
> +	- 2 = high-to-low edge triggered.
> +	- 4 = active high level-sensitive.
> +	- 8 = active low level-sensitive.

Just refer to the common definition.

> +
> +
> +Required properties for the GPIO bank node:
> +- compatible:
> +  - "mediatek,mt7621-gpio-bank" for Mediatek banks
> +- #gpio-cells : Should be two. The first cell is the GPIO pin number and the

So interrupt numbers and gpio numbers are different? 0-95 and 3x 0-31

That doesn't seem ideal.

> +   second cell specifies GPIO flags, as defined in <dt-bindings/gpio/gpio.h>.
> +   Only the GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags are supported.
> +- gpio-controller : Marks the device node as a GPIO controller.
> +- reg : The id of the bank that the node describes.

I'd prefer to not have banks defined in DT. Do you have a variable 
number or resources that are per bank? If not, then you don't need them.

> +
> +Example:
> +	gpio@600 {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		compatible = "mediatek,mt7621-gpio";
> +		reg = <0x600 0x100>;
> +
> +		interrupt-parent = <&gic>;
> +		interrupts = <GIC_SHARED 12 IRQ_TYPE_LEVEL_HIGH>;
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +
> +		gpio0: bank@0 {
> +			reg = <0>;
> +			compatible = "mediatek,mt7621-gpio-bank";
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +		};
> +
> +		gpio1: bank@1 {
> +			reg = <1>;
> +			compatible = "mediatek,mt7621-gpio-bank";
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +		};
> +
> +		gpio2: bank@2 {
> +			reg = <2>;
> +			compatible = "mediatek,mt7621-gpio-bank";
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +		};
> +	};
> -- 
> 2.7.4
> 

Re: [PATCH 2/2] dt-bindings: document gpio-mt7621 bindings

[Dan Carpenter]

On Tue, Jun 12, 2018 at 02:56:38PM -0600, Rob Herring wrote:
> Bindings are for h/w, not a driver.
> 
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > Reviewed-by: NeilBrown <neil@brown.name>
> 
> Space              ^

Pretty sure that was deliberate...  Otherwise he's been making that
same mistake for over a decade now.

regards,
dan carpenter

Re: [PATCH 2/2] dt-bindings: document gpio-mt7621 bindings

[Sergio Paracuellos]

Hi Rob,

Thanks for your time in reviewing this.

On Tue, Jun 12, 2018 at 10:56 PM, Rob Herring <robh@kernel.org> wrote:
> On Sat, Jun 02, 2018 at 09:30:10AM +0200, Sergio Paracuellos wrote:
>> Add a devicetree binding documentation for the mt7621 driver.
>
> Bindings are for h/w, not a driver.

You are totally right, my english is not my best as you can see :-).
I'll fix this
for v2.

>
>> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
>> Reviewed-by: NeilBrown <neil@brown.name>
>
> Space              ^
>

Actually, this is deliberate, so it will not change.

>> ---
>>  .../bindings/gpio/mediatek,mt7621-gpio.txt         | 68 ++++++++++++++++++++++
>>  1 file changed, 68 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/gpio/mediatek,mt7621-gpio.txt
>>
>> diff --git a/Documentation/devicetree/bindings/gpio/mediatek,mt7621-gpio.txt b/Documentation/devicetree/bindings/gpio/mediatek,mt7621-gpio.txt
>> new file mode 100644
>> index 0000000..30d8a02
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpio/mediatek,mt7621-gpio.txt
>> @@ -0,0 +1,68 @@
>> +Mediatek SoC GPIO controller bindings
>> +
>> +The IP core used inside these SoCs has 3 banks of 32 GPIOs each.
>> +The registers of all the banks are interwoven inside one single IO range.
>> +We load one GPIO controller instance per bank. To make this possible
>> +we support 2 types of nodes. The parent node defines the memory I/O range and
>> +has 3 children each describing a single bank. Also the GPIO controller can receive
>> +interrupts on any of the GPIOs, either edge or level. It then interrupts the CPU
>> +using GIC INT12.
>> +
>> +Required properties for the top level node:
>> +- compatible:
>> +  - "mediatek,mt7621-gpio" for Mediatek controllers
>> +- reg : Physical base address and length of the controller's registers
>> +- interrupt-parent : phandle of the parent interrupt controller.
>> +- interrupts : Interrupt specifier for the controllers interrupt.
>> +- interrupt-controller : Mark the device node as an interrupt controller.
>> +- #interrupt-cells : Should be 2. The first cell defines the interrupt number.
>> +   The second cell bits[3:0] is used to specify trigger type as follows:
>> +     - 1 = low-to-high edge triggered.
>> +     - 2 = high-to-low edge triggered.
>> +     - 4 = active high level-sensitive.
>> +     - 8 = active low level-sensitive.
>
> Just refer to the common definition.

ok, thanks. I will.

>
>> +
>> +
>> +Required properties for the GPIO bank node:
>> +- compatible:
>> +  - "mediatek,mt7621-gpio-bank" for Mediatek banks
>> +- #gpio-cells : Should be two. The first cell is the GPIO pin number and the
>
> So interrupt numbers and gpio numbers are different? 0-95 and 3x 0-31
>
> That doesn't seem ideal.

Yes, that's true. Actually this is one of the things that has been
changed for v2.

>
>> +   second cell specifies GPIO flags, as defined in <dt-bindings/gpio/gpio.h>.
>> +   Only the GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags are supported.
>> +- gpio-controller : Marks the device node as a GPIO controller.
>> +- reg : The id of the bank that the node describes.
>
> I'd prefer to not have banks defined in DT. Do you have a variable
> number or resources that are per bank? If not, then you don't need them.

Mmmm, That's what I understood from documentation:

"Some system-on-chips (SoCs) use the concept of GPIO banks. ...
Usually each such bank is
exposed in the device tree as an individual gpio-controller node. ..."

If this is not a good approach, could you please me point me out to a
device tree example where
the correct approach is being used?

Thanks in advance.

Best regards,
    Sergio Paracuellos
>> +
>> +Example:
>> +     gpio@600 {
>> +             #address-cells = <1>;
>> +             #size-cells = <0>;
>> +
>> +             compatible = "mediatek,mt7621-gpio";
>> +             reg = <0x600 0x100>;
>> +
>> +             interrupt-parent = <&gic>;
>> +             interrupts = <GIC_SHARED 12 IRQ_TYPE_LEVEL_HIGH>;
>> +             interrupt-controller;
>> +             #interrupt-cells = <2>;
>> +
>> +             gpio0: bank@0 {
>> +                     reg = <0>;
>> +                     compatible = "mediatek,mt7621-gpio-bank";
>> +                     gpio-controller;
>> +                     #gpio-cells = <2>;
>> +             };
>> +
>> +             gpio1: bank@1 {
>> +                     reg = <1>;
>> +                     compatible = "mediatek,mt7621-gpio-bank";
>> +                     gpio-controller;
>> +                     #gpio-cells = <2>;
>> +             };
>> +
>> +             gpio2: bank@2 {
>> +                     reg = <2>;
>> +                     compatible = "mediatek,mt7621-gpio-bank";
>> +                     gpio-controller;
>> +                     #gpio-cells = <2>;
>> +             };
>> +     };
>> --
>> 2.7.4
>>

Re: [PATCH 2/2] dt-bindings: document gpio-mt7621 bindings

[Rob Herring]

On Wed, Jun 13, 2018 at 10:23 AM, Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
> Hi Rob,
>
> Thanks for your time in reviewing this.
>
> On Tue, Jun 12, 2018 at 10:56 PM, Rob Herring <robh@kernel.org> wrote:
>> On Sat, Jun 02, 2018 at 09:30:10AM +0200, Sergio Paracuellos wrote:
>>> Add a devicetree binding documentation for the mt7621 driver.
>>

>>> +   second cell specifies GPIO flags, as defined in <dt-bindings/gpio/gpio.h>.
>>> +   Only the GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags are supported.
>>> +- gpio-controller : Marks the device node as a GPIO controller.
>>> +- reg : The id of the bank that the node describes.
>>
>> I'd prefer to not have banks defined in DT. Do you have a variable
>> number or resources that are per bank? If not, then you don't need them.
>
> Mmmm, That's what I understood from documentation:
>
> "Some system-on-chips (SoCs) use the concept of GPIO banks. ...
> Usually each such bank is
> exposed in the device tree as an individual gpio-controller node. ..."

This should be conditioned on being able to divide up the registers by
bank which seems like you can't. Or there's the case like the DW GPIO
block and the number of banks is configurable.

> If this is not a good approach, could you please me point me out to a
> device tree example where
> the correct approach is being used?

I'm not sure offhand. There are lots of examples of single nodes I'm
sure. Which ones have banks I haven't a clue. IIRC, there were some
cases where the bank # was part of the GPIO cells, but I seem to
recall Linus prefers not having 3 cells.

Rob

Re: [PATCH 2/2] dt-bindings: document gpio-mt7621 bindings

[Sergio Paracuellos]

On Wed, Jun 13, 2018 at 01:28:35PM -0600, Rob Herring wrote:
> On Wed, Jun 13, 2018 at 10:23 AM, Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> > Hi Rob,
> >
> > Thanks for your time in reviewing this.
> >
> > On Tue, Jun 12, 2018 at 10:56 PM, Rob Herring <robh@kernel.org> wrote:
> >> On Sat, Jun 02, 2018 at 09:30:10AM +0200, Sergio Paracuellos wrote:
> >>> Add a devicetree binding documentation for the mt7621 driver.
> >>
> 
> >>> +   second cell specifies GPIO flags, as defined in <dt-bindings/gpio/gpio.h>.
> >>> +   Only the GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags are supported.
> >>> +- gpio-controller : Marks the device node as a GPIO controller.
> >>> +- reg : The id of the bank that the node describes.
> >>
> >> I'd prefer to not have banks defined in DT. Do you have a variable
> >> number or resources that are per bank? If not, then you don't need them.
> >
> > Mmmm, That's what I understood from documentation:
> >
> > "Some system-on-chips (SoCs) use the concept of GPIO banks. ...
> > Usually each such bank is
> > exposed in the device tree as an individual gpio-controller node. ..."
> 
> This should be conditioned on being able to divide up the registers by
> bank which seems like you can't. Or there's the case like the DW GPIO
> block and the number of banks is configurable.

I see. Thanks for clarifing this.

> 
> > If this is not a good approach, could you please me point me out to a
> > device tree example where
> > the correct approach is being used?
> 
> I'm not sure offhand. There are lots of examples of single nodes I'm
> sure. Which ones have banks I haven't a clue. IIRC, there were some
> cases where the bank # was part of the GPIO cells, but I seem to
> recall Linus prefers not having 3 cells.

Ok, so... does the following single node sounds acceptable?

gpio: gpio@600 {
  #gpio-cells = <2>;
  #interrupt-cells = <2>;
  compatible = "mediatek,mt7621-gpio";
  gpio-controller;
  interrupt-controller;
  reg = <0x600 0x60>;
  interrupt-parent = <&gic>;
  interrupts = <GIC_SHARED 12 IRQ_TYPE_LEVEL_HIGH>;
  mediatek,gpio-bank-widths = <32 32 32>;
}

Changing definition for "reg" and adding a new one for 
"mediatek,gpio-bank-widths" as follows:

reg: 
  Define the base and range of the address space containing
  the mediatek GPIO controller registers

mediatek,gpio-bank-widths:
  Number of GPIO lines for each bank.  Number of elements must
  correspond to number of banks suggested by the 'reg' property.

Thanks in advance.
> 
> Rob

Best regards,
    Sergio Paracuellos

Re: [PATCH 2/2] dt-bindings: document gpio-mt7621 bindings

[Linus Walleij]

On Thu, Jun 14, 2018 at 6:45 AM, Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:

> Ok, so... does the following single node sounds acceptable?
>
> gpio: gpio@600 {
>   #gpio-cells = <2>;
>   #interrupt-cells = <2>;
>   compatible = "mediatek,mt7621-gpio";
>   gpio-controller;
>   interrupt-controller;
>   reg = <0x600 0x60>;
>   interrupt-parent = <&gic>;
>   interrupts = <GIC_SHARED 12 IRQ_TYPE_LEVEL_HIGH>;
>   mediatek,gpio-bank-widths = <32 32 32>;

Why would you need this? It is pretty obvious from the
compatible string isn't it? Just use that to tell that
"aha, it is mediatek,mt7321-gpio, so it has 3x32 GPIOs
indexed from 0..95".

No need of overspecifying stuff.

Yours,
Linus Walleij

Re: [PATCH 2/2] dt-bindings: document gpio-mt7621 bindings

[Sergio Paracuellos]

On Thu, Jun 14, 2018 at 4:17 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Jun 14, 2018 at 6:45 AM, Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
>
>> Ok, so... does the following single node sounds acceptable?
>>
>> gpio: gpio@600 {
>>   #gpio-cells = <2>;
>>   #interrupt-cells = <2>;
>>   compatible = "mediatek,mt7621-gpio";
>>   gpio-controller;
>>   interrupt-controller;
>>   reg = <0x600 0x60>;
>>   interrupt-parent = <&gic>;
>>   interrupts = <GIC_SHARED 12 IRQ_TYPE_LEVEL_HIGH>;
>>   mediatek,gpio-bank-widths = <32 32 32>;
>
> Why would you need this? It is pretty obvious from the
> compatible string isn't it? Just use that to tell that
> "aha, it is mediatek,mt7321-gpio, so it has 3x32 GPIOs
> indexed from 0..95".
>
> No need of overspecifying stuff.

Ok, So just one node with no extra stuff.

gpio: gpio@600 {
   #gpio-cells = <2>;
   #interrupt-cells = <2>;
   compatible = "mediatek,mt7621-gpio";
   gpio-controller;
   interrupt-controller;
   reg = <0x600 0x60>;
   interrupt-parent = <&gic>;
   interrupts = <GIC_SHARED 12 IRQ_TYPE_LEVEL_HIGH>;

Thanks for your answers!

>
> Yours,
> Linus Walleij

Best regards,
    Sergio Paracuellos

Re: [PATCH 2/2] dt-bindings: document gpio-mt7621 bindings

[Linus Walleij]

On Wed, Jun 13, 2018 at 9:28 PM, Rob Herring <robh@kernel.org> wrote:

>> "Some system-on-chips (SoCs) use the concept of GPIO banks. ...
>> Usually each such bank is
>> exposed in the device tree as an individual gpio-controller node. ..."
>
> This should be conditioned on being able to divide up the registers by
> bank which seems like you can't. Or there's the case like the DW GPIO
> block and the number of banks is configurable.

If it is possible to create one device per bank I usually prefer that
approach, as it also (often) makes it possible to use the
generic GPIO library, i.e. the hardware abstraction start to
share more with other GPIO controllers.

>> If this is not a good approach, could you please me point me out to a
>> device tree example where
>> the correct approach is being used?
>
> I'm not sure offhand. There are lots of examples of single nodes I'm
> sure. Which ones have banks I haven't a clue. IIRC, there were some
> cases where the bank # was part of the GPIO cells, but I seem to
> recall Linus prefers not having 3 cells.

I don't like 3 cells, stuff is complicated enough as it is already.

Better in that case to concatenate the offsets and instead of
having an extra cell 0, 1 and offsets 0-31, 0-31
have two cells and offsets 0-63.

My reasoning is that since it is represented by a single device
we are indexing into that one device from 0-n.

Yours,
Linus Walleij

Re: [PATCH 2/2] dt-bindings: document gpio-mt7621 bindings

[Rob Herring]

On Thu, Jun 14, 2018 at 8:14 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Jun 13, 2018 at 9:28 PM, Rob Herring <robh@kernel.org> wrote:
>
>>> "Some system-on-chips (SoCs) use the concept of GPIO banks. ...
>>> Usually each such bank is
>>> exposed in the device tree as an individual gpio-controller node. ..."
>>
>> This should be conditioned on being able to divide up the registers by
>> bank which seems like you can't. Or there's the case like the DW GPIO
>> block and the number of banks is configurable.
>
> If it is possible to create one device per bank I usually prefer that
> approach, as it also (often) makes it possible to use the
> generic GPIO library, i.e. the hardware abstraction start to
> share more with other GPIO controllers.

But that should be possible whether there are banks in DT or not, right?

Rob

Re: [PATCH 2/2] dt-bindings: document gpio-mt7621 bindings

[Linus Walleij]

On Thu, Jun 14, 2018 at 4:33 PM, Rob Herring <robh@kernel.org> wrote:
> On Thu, Jun 14, 2018 at 8:14 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Wed, Jun 13, 2018 at 9:28 PM, Rob Herring <robh@kernel.org> wrote:
>>
>>>> "Some system-on-chips (SoCs) use the concept of GPIO banks. ...
>>>> Usually each such bank is
>>>> exposed in the device tree as an individual gpio-controller node. ..."
>>>
>>> This should be conditioned on being able to divide up the registers by
>>> bank which seems like you can't. Or there's the case like the DW GPIO
>>> block and the number of banks is configurable.
>>
>> If it is possible to create one device per bank I usually prefer that
>> approach, as it also (often) makes it possible to use the
>> generic GPIO library, i.e. the hardware abstraction start to
>> share more with other GPIO controllers.
>
> But that should be possible whether there are banks in DT or not, right?

Possible yes, but more complex, requireing a bigger and more complex
chunk of code to get it right. Which we don't have for Linux (I don't know
about $OS).

For 1 bank = 1 device, all callbacks etc naturally offsets to something
like 0..31 landing in (1 << offset) which makes for a simple support library.

If there is more complex calculations, more complex helper libs are
required and maybe not even worth it, ending up with more duplicated
or slightly-different code.

Yours,
Linus Walleij