[PATCH v5 00/18] staging: mt7621-gpio: last cleanups

[PATCH v5 00/18] staging: mt7621-gpio: last cleanups

[Sergio Paracuellos]

After submiting this driver to try to get mainlined and get
out of staging some new cleanups seems to be necessary.
According to this mail of Linus Walleij:

http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-June/121742.html

and this mail os Rob Herring:

http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-June/121974.html

this series tries to fix all of the issues in order to send
v2 and give it a new try. Because I don't have to hardware to
test my changes I send new cleanups first in staging to make
easier to NeilBrown test it and get a feedback about them.

Changes in v5:
    - PATCH 18 pass 'np' instead of 'bank' which was NULL and
      was wrong. Bank is no more neccesary because now we 
      have only one node in the DT.

Changes in v4:
    - Add "last minor cleanups" series to the same patch series.
    - Update Kconfig being more specific in description and help
      about the SoC.
    - Update dt-bindings, device tree and code to don't use
      banks in the device tree assuming this SoC has 3 banks
      with 32 gpios each.

Changes in v3:
    - PATCH 7: refactor irq_type to make better code.
    - Add PATCH 8 avoiding the use of custom domain and requesting
      manually a 'IRQF_SHARED'.

Changes in v2:
    - Patch where GPIOLIB_IRQCHIP was used avoiding
      the use of a custom irq domain has been dropped to
      be sure after this changes all is working properly.
      (This was PATCH 7 in previous series)
    - PATCH 1:
         * avoid introducing new macros and use 'bank'
           field of mtk_gc with register offset.
         * Make correct use of bgpio_init passing new
           void __iomem pointers instead of use the
           macros.
    - Previous series PATCH 8 now is PATCH 7. Avoid the
      use of a switch-case statement which was wrong and
      distinc if we have RISSING AND FALLING EDGE interrupt
      or HIGH LOW level ones. This last two are exclusive and
      cannot be generated at the same time.

Hope this helps.

Thanks in advance.

Best regards,
    Sergio Paracuellos


Sergio Paracuellos (18):
  staging: mt7621-gpio: make use 'bgpio_init' from GPIO_GENERIC
  staging: mt7621-gpio: avoid including 'gpio.h'
  staging: mt7621-gpio: make use of 'builtin_platform_driver'
  staging: mt7621-gpio: implement '.irq_[request|release]_resources'
    functions
  staging: mt7621-gpio: add COMPILE_TEST
  staging: mt7621-gpio: add kerneldoc for state data containers
  staging: mt7621-gpio: implement high level and low level irqs
  staging: mt7621-gpio: avoid custom irq_domain for gpio
  staging: mt7621-gpio: remove no more necessary PIN_MASK macro
  staging: mt7621-gpio: update kerneldoc for state containers
  staging: mt7621-gpio: align indentation for all defines
  staging: mt7621-gpio: avoid check for NULL in 'to_mediatek_gpio' calls
  staging: mt7621-gpio: avoid to set up irqs if not defined in dts
  staging: mt7621-gpio: avoid one level indentation in interrupt handler
  staging: mt7621-gpio: set different names for each gpio_chip and
    irq_chip
  staging: mt7621-gpio: avoid long line in a comment
  staging: mt7621-gpio: update Kconfig with SoC details
  staging: mt7621-gpio: avoid use banks in device tree

 drivers/staging/mt7621-dts/gbpc1.dts               |  10 +-
 drivers/staging/mt7621-dts/mt7621.dtsi             |  31 +-
 drivers/staging/mt7621-gpio/Kconfig                |   8 +-
 drivers/staging/mt7621-gpio/gpio-mt7621.c          | 406 +++++++++------------
 .../staging/mt7621-gpio/mediatek,mt7621-gpio.txt   |  59 +--
 5 files changed, 205 insertions(+), 309 deletions(-)

-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v5 01/18] staging: mt7621-gpio: make use 'bgpio_init' from GPIO_GENERIC

[Sergio Paracuellos]

Gpio complexity is just masking the fact that offset is always
0..n and writes to bits 0..n of some memory address. Because
of this whole thing can just me converted to use GPIO_GENERIC
and avoid duplications of a lot of driver custom functions.
So use bgpio_init instead of custom code adding GPIO_GENERIC
dependency to the Kconfig file. Also to make easier using
bgpio_init function offset for each gpio bank, enumeration
where register were defined has been replaced in favour of
some macros which handle each gpio offset taking into account
the bank where are located. Because of this change write and
read functions which are being used for remaining irq handling
stuff have been updated also as well as its dependencies along
the code.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-gpio/Kconfig       |   1 +
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 136 +++++++++---------------------
 2 files changed, 42 insertions(+), 95 deletions(-)

diff --git a/drivers/staging/mt7621-gpio/Kconfig b/drivers/staging/mt7621-gpio/Kconfig
index c741ec3..f4453e8 100644
--- a/drivers/staging/mt7621-gpio/Kconfig
+++ b/drivers/staging/mt7621-gpio/Kconfig
@@ -1,6 +1,7 @@
 config GPIO_MT7621
 	bool "Mediatek GPIO Support"
 	depends on SOC_MT7620 || SOC_MT7621
+	select GPIO_GENERIC
 	select ARCH_REQUIRE_GPIOLIB
 	help
 	  Say yes here to support the Mediatek SoC GPIO device
diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index 0c4fb4a..eb60dd4 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -19,19 +19,18 @@
 #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,
-};
+#define GPIO_BANK_WIDE	0x04
+#define GPIO_REG_CTRL	0x00
+#define GPIO_REG_POL	0x10
+#define GPIO_REG_DATA	0x20
+#define GPIO_REG_DSET	0x30
+#define GPIO_REG_DCLR	0x40
+#define GPIO_REG_REDGE	0x50
+#define GPIO_REG_FEDGE	0x60
+#define GPIO_REG_HLVL	0x70
+#define GPIO_REG_LLVL	0x80
+#define GPIO_REG_STAT	0x90
+#define GPIO_REG_EDGE	0xA0
 
 struct mtk_gc {
 	struct gpio_chip chip;
@@ -55,80 +54,23 @@ to_mediatek_gpio(struct gpio_chip *chip)
 }
 
 static inline void
-mtk_gpio_w32(struct mtk_gc *rg, u8 reg, u32 val)
+mtk_gpio_w32(struct mtk_gc *rg, u32 offset, u32 val)
 {
-	struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
-	u32 offset = (reg * 0x10) + (rg->bank * 0x4);
+	struct gpio_chip *gc = &rg->chip;
+	struct mtk_data *gpio_data = gpiochip_get_data(gc);
 
-	iowrite32(val, gpio_data->gpio_membase + offset);
+	offset = (rg->bank * GPIO_BANK_WIDE) + offset;
+	gc->write_reg(gpio_data->gpio_membase + offset, val);
 }
 
 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)
+mtk_gpio_r32(struct mtk_gc *rg, u32 offset)
 {
-	struct mtk_gc *rg = to_mediatek_gpio(chip);
-	unsigned long flags;
-	u32 t;
+	struct gpio_chip *gc = &rg->chip;
+	struct mtk_data *gpio_data = gpiochip_get_data(gc);
 
-	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;
+	offset = (rg->bank * GPIO_BANK_WIDE) + offset;
+	return gc->read_reg(gpio_data->gpio_membase + offset);
 }
 
 static int
@@ -144,34 +86,38 @@ mediatek_gpio_to_irq(struct gpio_chip *chip, unsigned int pin)
 static int
 mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
 {
-	struct mtk_data *gpio_data = dev_get_drvdata(&pdev->dev);
+	struct mtk_data *gpio = dev_get_drvdata(&pdev->dev);
 	const __be32 *id = of_get_property(bank, "reg", NULL);
 	struct mtk_gc *rg;
+	void __iomem *dat, *set, *ctrl, *diro;
 	int ret;
 
 	if (!id || be32_to_cpu(*id) >= MTK_BANK_CNT)
 		return -EINVAL;
 
-	rg = &gpio_data->gc_map[be32_to_cpu(*id)];
+	rg = &gpio->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);
+	dat = gpio->gpio_membase + GPIO_REG_DATA + (rg->bank * GPIO_BANK_WIDE);
+	set = gpio->gpio_membase + GPIO_REG_DSET + (rg->bank * GPIO_BANK_WIDE);
+	ctrl = gpio->gpio_membase + GPIO_REG_DCLR + (rg->bank * GPIO_BANK_WIDE);
+	diro = gpio->gpio_membase + GPIO_REG_CTRL + (rg->bank * GPIO_BANK_WIDE);
+
+	ret = bgpio_init(&rg->chip, &pdev->dev, 4,
+			 dat, set, ctrl, diro, NULL, 0);
+	if (ret) {
+		dev_err(&pdev->dev, "bgpio_init() failed\n");
+		return ret;
+	}
+
+	if (gpio->gpio_irq_domain)
+		rg->chip.to_irq = mediatek_gpio_to_irq;
+
+	ret = devm_gpiochip_add_data(&pdev->dev, &rg->chip, gpio);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "Could not register gpio %d, ret=%d\n",
 			rg->chip.ngpio, ret);
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v5 02/18] staging: mt7621-gpio: avoid including 'gpio.h'

[Sergio Paracuellos]

Including file '<linux/gpio.h>' should be avoided in
new drivers code, so just remove it because it is
no necessary at all.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index eb60dd4..f95310c 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -5,7 +5,6 @@
  */
 
 #include <linux/err.h>
-#include <linux/gpio.h>
 #include <linux/gpio/driver.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v5 03/18] staging: mt7621-gpio: make use of 'builtin_platform_driver'

[Sergio Paracuellos]

This driver was being registered using 'module_platform_driver'
but it is not a module at all. Instead of this use
'builtin_platform_driver' which seems to be the correct one.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index f95310c..6a31f60 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -312,4 +312,4 @@ static struct platform_driver mediatek_gpio_driver = {
 	},
 };
 
-module_platform_driver(mediatek_gpio_driver);
+builtin_platform_driver(mediatek_gpio_driver);
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v5 04/18] staging: mt7621-gpio: implement '.irq_[request|release]_resources' functions

[Sergio Paracuellos]

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().
Add those two for this driver. Also store struct device pointer
in global state structure to be able to use 'dev_err' with the
device from 'mediatek_request_resources' function.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 42 +++++++++++++++++++++++++++----
 1 file changed, 37 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index 6a31f60..e5a7979 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -40,6 +40,7 @@ struct mtk_gc {
 };
 
 struct mtk_data {
+	struct device *dev;
 	void __iomem *gpio_membase;
 	int gpio_irq;
 	struct irq_domain *gpio_irq_domain;
@@ -231,12 +232,42 @@ mediatek_gpio_irq_type(struct irq_data *d, unsigned int type)
 	return 0;
 }
 
+static int mediatek_irq_reqres(struct irq_data *d)
+{
+	struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
+	int bank = irqd_to_hwirq(d) / MTK_BANK_WIDTH;
+	struct mtk_gc *rg = &gpio_data->gc_map[bank];
+	struct gpio_chip *gc = &rg->chip;
+	int ret;
+
+	ret = gpiochip_lock_as_irq(gc, irqd_to_hwirq(d));
+	if (ret) {
+		dev_err(gpio_data->dev, "unable to lock HW IRQ %lu for IRQ\n",
+			irqd_to_hwirq(d));
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void mediatek_irq_relres(struct irq_data *d)
+{
+	struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
+	int bank = irqd_to_hwirq(d) / MTK_BANK_WIDTH;
+	struct mtk_gc *rg = &gpio_data->gc_map[bank];
+	struct gpio_chip *gc = &rg->chip;
+
+	gpiochip_unlock_as_irq(gc, irqd_to_hwirq(d));
+}
+
 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,
+	.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,
+	.irq_request_resources	= mediatek_irq_reqres,
+	.irq_release_resources	= mediatek_irq_relres,
 };
 
 static int
@@ -284,6 +315,7 @@ mediatek_gpio_probe(struct platform_device *pdev)
 			dev_err(&pdev->dev, "irq_domain_add_linear failed\n");
 	}
 
+	gpio_data->dev = &pdev->dev;
 	platform_set_drvdata(pdev, gpio_data);
 
 	for_each_child_of_node(np, bank)
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v5 05/18] staging: mt7621-gpio: add COMPILE_TEST

[Sergio Paracuellos]

This driver is actually platform-agnostic.  Add COMPILE_TEST for
the compilation test coverage.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-gpio/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/mt7621-gpio/Kconfig b/drivers/staging/mt7621-gpio/Kconfig
index f4453e8..835998a 100644
--- a/drivers/staging/mt7621-gpio/Kconfig
+++ b/drivers/staging/mt7621-gpio/Kconfig
@@ -1,6 +1,6 @@
 config GPIO_MT7621
 	bool "Mediatek GPIO Support"
-	depends on SOC_MT7620 || SOC_MT7621
+	depends on SOC_MT7620 || SOC_MT7621 || COMPILE_TEST
 	select GPIO_GENERIC
 	select ARCH_REQUIRE_GPIOLIB
 	help
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v5 06/18] staging: mt7621-gpio: add kerneldoc for state data containers

[Sergio Paracuellos]

This commit adds kerneldoc for the two data containers in
order to better understanding of its existence.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index e5a7979..54c18c1 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -31,6 +31,14 @@
 #define GPIO_REG_STAT	0x90
 #define GPIO_REG_EDGE	0xA0
 
+/**
+ * struct mtk_gc - data for a single gpio-chip
+ * @chip: gpio chip instance
+ * @lock: spinlock to protect the chip
+ * @bank: gpio bank number for the chip
+ * @rising: mask for rising irqs
+ * @falling: mask for falling irqs
+ */
 struct mtk_gc {
 	struct gpio_chip chip;
 	spinlock_t lock;
@@ -39,6 +47,16 @@ struct mtk_gc {
 	u32 falling;
 };
 
+/**
+ * struct mtk_data - state container for
+ * data of the platform driver. It is a
+ * single irq-chip but 3 separate gpio-chip.
+ * @dev: device instance
+ * @gpio_membase: memory base address
+ * @gpio_irq: irq number from the device tree
+ * @gpio_irq_domain: irq domain for this chip
+ * @gc_map: array of the gpio chips
+ */
 struct mtk_data {
 	struct device *dev;
 	void __iomem *gpio_membase;
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v5 07/18] staging: mt7621-gpio: implement high level and low level irqs

[Sergio Paracuellos]

This chip support high level and low level interrupts. Those
have to be implemented also to get a complete and clean driver.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 47 ++++++++++++++++++++++++-------
 1 file changed, 37 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index 54c18c1..0c344a5 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -38,6 +38,8 @@
  * @bank: gpio bank number for the chip
  * @rising: mask for rising irqs
  * @falling: mask for falling irqs
+ * @hlevel: mask for high level irqs
+ * @llevel: mask for low level irqs
  */
 struct mtk_gc {
 	struct gpio_chip chip;
@@ -45,6 +47,8 @@ struct mtk_gc {
 	int bank;
 	u32 rising;
 	u32 falling;
+	u32 hlevel;
+	u32 llevel;
 };
 
 /**
@@ -184,7 +188,7 @@ mediatek_gpio_irq_unmask(struct irq_data *d)
 	int bank = pin / MTK_BANK_WIDTH;
 	struct mtk_gc *rg = &gpio_data->gc_map[bank];
 	unsigned long flags;
-	u32 rise, fall;
+	u32 rise, fall, high, low;
 
 	if (!rg)
 		return;
@@ -192,8 +196,12 @@ mediatek_gpio_irq_unmask(struct irq_data *d)
 	spin_lock_irqsave(&rg->lock, flags);
 	rise = mtk_gpio_r32(rg, GPIO_REG_REDGE);
 	fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE);
+	high = mtk_gpio_r32(rg, GPIO_REG_HLVL);
+	low = mtk_gpio_r32(rg, GPIO_REG_LLVL);
 	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));
+	mtk_gpio_w32(rg, GPIO_REG_HLVL, high | (PIN_MASK(pin) & rg->hlevel));
+	mtk_gpio_w32(rg, GPIO_REG_LLVL, low | (PIN_MASK(pin) & rg->llevel));
 	spin_unlock_irqrestore(&rg->lock, flags);
 }
 
@@ -205,7 +213,7 @@ mediatek_gpio_irq_mask(struct irq_data *d)
 	int bank = pin / MTK_BANK_WIDTH;
 	struct mtk_gc *rg = &gpio_data->gc_map[bank];
 	unsigned long flags;
-	u32 rise, fall;
+	u32 rise, fall, high, low;
 
 	if (!rg)
 		return;
@@ -213,8 +221,12 @@ mediatek_gpio_irq_mask(struct irq_data *d)
 	spin_lock_irqsave(&rg->lock, flags);
 	rise = mtk_gpio_r32(rg, GPIO_REG_REDGE);
 	fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE);
+	high = mtk_gpio_r32(rg, GPIO_REG_HLVL);
+	low = mtk_gpio_r32(rg, GPIO_REG_LLVL);
 	mtk_gpio_w32(rg, GPIO_REG_FEDGE, fall & ~PIN_MASK(pin));
 	mtk_gpio_w32(rg, GPIO_REG_REDGE, rise & ~PIN_MASK(pin));
+	mtk_gpio_w32(rg, GPIO_REG_HLVL, high & ~PIN_MASK(pin));
+	mtk_gpio_w32(rg, GPIO_REG_LLVL, low & ~PIN_MASK(pin));
 	spin_unlock_irqrestore(&rg->lock, flags);
 }
 
@@ -231,21 +243,36 @@ mediatek_gpio_irq_type(struct irq_data *d, unsigned int type)
 		return -1;
 
 	if (type == IRQ_TYPE_PROBE) {
-		if ((rg->rising | rg->falling) & mask)
+		if ((rg->rising | rg->falling |
+		     rg->hlevel | rg->llevel) & 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;
+	rg->rising &= ~mask;
+	rg->falling &= ~mask;
+	rg->hlevel &= ~mask;
+	rg->llevel &= ~mask;
 
-	if (type & IRQ_TYPE_EDGE_FALLING)
+	switch (type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_EDGE_BOTH:
+		rg->rising |= mask;
 		rg->falling |= mask;
-	else
-		rg->falling &= ~mask;
+		break;
+	case IRQ_TYPE_EDGE_RISING:
+		rg->rising |= mask;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		rg->falling |= mask;
+		break;
+	case IRQ_TYPE_LEVEL_HIGH:
+		rg->hlevel |= mask;
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+		rg->llevel |= mask;
+		break;
+	}
 
 	return 0;
 }
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v5 08/18] staging: mt7621-gpio: avoid custom irq_domain for gpio

[Sergio Paracuellos]

Instead of create a custom irq_domain for this chip, use
'gpiochip_set_chained_irqchip' from GPIOLIB_IRQCHIP. It
is ok to call this function several times. We have to
manually mark the line with 'IRQF_SHARED' and then loop
over the three banks until you find a hit. There were
some problems with removing an irqchip like that but this
driver is a bool so it might work just fine. After this
changes the functions 'mediatek_gpio_to_irq' is not needed
anymore and also the 'gpio_irq_domain' field from the state
container. Instead of use the custom irq domain in the irq
handler use the associated domain from the gpio_chip in
'irq_find_mapping' function. Function 'mediatek_gpio_bank_probe'
has been moved a it to the botton to have all the irq related
functions together and avoid some forward declarations to resolve
some symbols along the code.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-gpio/Kconfig       |   1 +
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 221 +++++++++++-------------------
 2 files changed, 81 insertions(+), 141 deletions(-)

diff --git a/drivers/staging/mt7621-gpio/Kconfig b/drivers/staging/mt7621-gpio/Kconfig
index 835998a..e32facb 100644
--- a/drivers/staging/mt7621-gpio/Kconfig
+++ b/drivers/staging/mt7621-gpio/Kconfig
@@ -2,6 +2,7 @@ config GPIO_MT7621
 	bool "Mediatek GPIO Support"
 	depends on SOC_MT7620 || SOC_MT7621 || COMPILE_TEST
 	select GPIO_GENERIC
+	select GPIOLIB_IRQCHIP
 	select ARCH_REQUIRE_GPIOLIB
 	help
 	  Say yes here to support the Mediatek SoC GPIO device
diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index 0c344a5..d2a7512 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -8,7 +8,6 @@
 #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>
@@ -58,14 +57,12 @@ struct mtk_gc {
  * @dev: device instance
  * @gpio_membase: memory base address
  * @gpio_irq: irq number from the device tree
- * @gpio_irq_domain: irq domain for this chip
  * @gc_map: array of the gpio chips
  */
 struct mtk_data {
 	struct device *dev;
 	void __iomem *gpio_membase;
 	int gpio_irq;
-	struct irq_domain *gpio_irq_domain;
 	struct mtk_gc gc_map[MTK_BANK_CNT];
 };
 
@@ -95,98 +92,36 @@ mtk_gpio_r32(struct mtk_gc *rg, u32 offset)
 	return gc->read_reg(gpio_data->gpio_membase + offset);
 }
 
-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 = dev_get_drvdata(&pdev->dev);
-	const __be32 *id = of_get_property(bank, "reg", NULL);
-	struct mtk_gc *rg;
-	void __iomem *dat, *set, *ctrl, *diro;
-	int ret;
-
-	if (!id || be32_to_cpu(*id) >= MTK_BANK_CNT)
-		return -EINVAL;
-
-	rg = &gpio->gc_map[be32_to_cpu(*id)];
-	memset(rg, 0, sizeof(*rg));
-
-	spin_lock_init(&rg->lock);
-	rg->chip.of_node = bank;
-	rg->bank = be32_to_cpu(*id);
-
-	dat = gpio->gpio_membase + GPIO_REG_DATA + (rg->bank * GPIO_BANK_WIDE);
-	set = gpio->gpio_membase + GPIO_REG_DSET + (rg->bank * GPIO_BANK_WIDE);
-	ctrl = gpio->gpio_membase + GPIO_REG_DCLR + (rg->bank * GPIO_BANK_WIDE);
-	diro = gpio->gpio_membase + GPIO_REG_CTRL + (rg->bank * GPIO_BANK_WIDE);
-
-	ret = bgpio_init(&rg->chip, &pdev->dev, 4,
-			 dat, set, ctrl, diro, NULL, 0);
-	if (ret) {
-		dev_err(&pdev->dev, "bgpio_init() failed\n");
-		return ret;
-	}
-
-	if (gpio->gpio_irq_domain)
-		rg->chip.to_irq = mediatek_gpio_to_irq;
-
-	ret = devm_gpiochip_add_data(&pdev->dev, &rg->chip, gpio);
-	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)
+static irqreturn_t
+mediatek_gpio_irq_handler(int irq, void *data)
 {
-	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;
+	struct gpio_chip *gc = data;
+	struct mtk_gc *rg = to_mediatek_gpio(gc);
+	irqreturn_t ret = IRQ_NONE;
+	unsigned long pending;
+	int bit;
 
-		pending = mtk_gpio_r32(rg, GPIO_REG_STAT);
+	pending = mtk_gpio_r32(rg, GPIO_REG_STAT);
 
+	if (pending) {
 		for_each_set_bit(bit, &pending, MTK_BANK_WIDTH) {
-			u32 map = irq_find_mapping(gpio_data->gpio_irq_domain,
-						   (MTK_BANK_WIDTH * i) + bit);
+			u32 map = irq_find_mapping(gc->irq.domain, bit);
 
 			generic_handle_irq(map);
 			mtk_gpio_w32(rg, GPIO_REG_STAT, BIT(bit));
+			ret |= IRQ_HANDLED;
 		}
 	}
+
+	return ret;
 }
 
 static void
 mediatek_gpio_irq_unmask(struct irq_data *d)
 {
-	struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct mtk_gc *rg = to_mediatek_gpio(gc);
 	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, high, low;
 
@@ -208,10 +143,9 @@ mediatek_gpio_irq_unmask(struct irq_data *d)
 static void
 mediatek_gpio_irq_mask(struct irq_data *d)
 {
-	struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct mtk_gc *rg = to_mediatek_gpio(gc);
 	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, high, low;
 
@@ -233,10 +167,9 @@ mediatek_gpio_irq_mask(struct irq_data *d)
 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);
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct mtk_gc *rg = to_mediatek_gpio(gc);
 	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)
@@ -277,65 +210,84 @@ mediatek_gpio_irq_type(struct irq_data *d, unsigned int type)
 	return 0;
 }
 
-static int mediatek_irq_reqres(struct irq_data *d)
-{
-	struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
-	int bank = irqd_to_hwirq(d) / MTK_BANK_WIDTH;
-	struct mtk_gc *rg = &gpio_data->gc_map[bank];
-	struct gpio_chip *gc = &rg->chip;
-	int ret;
-
-	ret = gpiochip_lock_as_irq(gc, irqd_to_hwirq(d));
-	if (ret) {
-		dev_err(gpio_data->dev, "unable to lock HW IRQ %lu for IRQ\n",
-			irqd_to_hwirq(d));
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
-static void mediatek_irq_relres(struct irq_data *d)
-{
-	struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
-	int bank = irqd_to_hwirq(d) / MTK_BANK_WIDTH;
-	struct mtk_gc *rg = &gpio_data->gc_map[bank];
-	struct gpio_chip *gc = &rg->chip;
-
-	gpiochip_unlock_as_irq(gc, irqd_to_hwirq(d));
-}
-
 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,
-	.irq_request_resources	= mediatek_irq_reqres,
-	.irq_release_resources	= mediatek_irq_relres,
 };
 
 static int
-mediatek_gpio_gpio_map(struct irq_domain *d, unsigned int irq,
-		       irq_hw_number_t hw)
+mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
 {
+	struct mtk_data *gpio = dev_get_drvdata(&pdev->dev);
+	const __be32 *id = of_get_property(bank, "reg", NULL);
+	struct mtk_gc *rg;
+	void __iomem *dat, *set, *ctrl, *diro;
 	int ret;
 
-	ret = irq_set_chip_data(irq, d->host_data);
-	if (ret < 0)
+	if (!id || be32_to_cpu(*id) >= MTK_BANK_CNT)
+		return -EINVAL;
+
+	rg = &gpio->gc_map[be32_to_cpu(*id)];
+	memset(rg, 0, sizeof(*rg));
+
+	spin_lock_init(&rg->lock);
+	rg->chip.of_node = bank;
+	rg->bank = be32_to_cpu(*id);
+
+	dat = gpio->gpio_membase + GPIO_REG_DATA + (rg->bank * GPIO_BANK_WIDE);
+	set = gpio->gpio_membase + GPIO_REG_DSET + (rg->bank * GPIO_BANK_WIDE);
+	ctrl = gpio->gpio_membase + GPIO_REG_DCLR + (rg->bank * GPIO_BANK_WIDE);
+	diro = gpio->gpio_membase + GPIO_REG_CTRL + (rg->bank * GPIO_BANK_WIDE);
+
+	ret = bgpio_init(&rg->chip, &pdev->dev, 4,
+			 dat, set, ctrl, diro, NULL, 0);
+	if (ret) {
+		dev_err(&pdev->dev, "bgpio_init() failed\n");
+		return ret;
+	}
+
+	ret = devm_gpiochip_add_data(&pdev->dev, &rg->chip, gpio);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Could not register gpio %d, ret=%d\n",
+			rg->chip.ngpio, ret);
+		return ret;
+	}
+
+	/*
+	 * Manually request the irq here instead of passing a flow-handler
+	 * to gpiochip_set_chained_irqchip, because the irq is shared.
+	 */
+	ret = devm_request_irq(&pdev->dev, gpio->gpio_irq,
+			       mediatek_gpio_irq_handler, IRQF_SHARED,
+			       "mt7621", &rg->chip);
+
+	if (ret) {
+		dev_err(&pdev->dev, "Error requesting IRQ %d: %d\n",
+			gpio->gpio_irq, ret);
 		return ret;
-	irq_set_chip_and_handler(irq, &mediatek_gpio_irq_chip,
-				 handle_level_irq);
-	irq_set_handler_data(irq, d);
+	}
+
+	ret = gpiochip_irqchip_add(&rg->chip, &mediatek_gpio_irq_chip,
+				   0, handle_simple_irq, IRQ_TYPE_NONE);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to add gpiochip_irqchip\n");
+		return ret;
+	}
+
+	gpiochip_set_chained_irqchip(&rg->chip, &mediatek_gpio_irq_chip,
+				     gpio->gpio_irq, NULL);
+
+	/* 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 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)
 {
@@ -352,14 +304,6 @@ mediatek_gpio_probe(struct platform_device *pdev)
 		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");
-	}
-
 	gpio_data->dev = &pdev->dev;
 	platform_set_drvdata(pdev, gpio_data);
 
@@ -367,11 +311,6 @@ mediatek_gpio_probe(struct platform_device *pdev)
 		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;
 }
 
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v5 09/18] staging: mt7621-gpio: remove no more necessary PIN_MASK macro

[Sergio Paracuellos]

PIN_MASK macro was being used because of the fact we were only
using one interrupt controller for all of the gpio chips. This
has been changed to use one per gpio chip and each has 32 irqs.
Because of this this macro is not needed anymore. Use BIT macro
instead.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index d2a7512..a8893e8 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -15,7 +15,6 @@
 
 #define MTK_BANK_CNT		3
 #define MTK_BANK_WIDTH		32
-#define PIN_MASK(nr)		(1UL << ((nr % MTK_BANK_WIDTH)))
 
 #define GPIO_BANK_WIDE	0x04
 #define GPIO_REG_CTRL	0x00
@@ -133,10 +132,10 @@ mediatek_gpio_irq_unmask(struct irq_data *d)
 	fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE);
 	high = mtk_gpio_r32(rg, GPIO_REG_HLVL);
 	low = mtk_gpio_r32(rg, GPIO_REG_LLVL);
-	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));
-	mtk_gpio_w32(rg, GPIO_REG_HLVL, high | (PIN_MASK(pin) & rg->hlevel));
-	mtk_gpio_w32(rg, GPIO_REG_LLVL, low | (PIN_MASK(pin) & rg->llevel));
+	mtk_gpio_w32(rg, GPIO_REG_REDGE, rise | (BIT(pin) & rg->rising));
+	mtk_gpio_w32(rg, GPIO_REG_FEDGE, fall | (BIT(pin) & rg->falling));
+	mtk_gpio_w32(rg, GPIO_REG_HLVL, high | (BIT(pin) & rg->hlevel));
+	mtk_gpio_w32(rg, GPIO_REG_LLVL, low | (BIT(pin) & rg->llevel));
 	spin_unlock_irqrestore(&rg->lock, flags);
 }
 
@@ -157,10 +156,10 @@ mediatek_gpio_irq_mask(struct irq_data *d)
 	fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE);
 	high = mtk_gpio_r32(rg, GPIO_REG_HLVL);
 	low = mtk_gpio_r32(rg, GPIO_REG_LLVL);
-	mtk_gpio_w32(rg, GPIO_REG_FEDGE, fall & ~PIN_MASK(pin));
-	mtk_gpio_w32(rg, GPIO_REG_REDGE, rise & ~PIN_MASK(pin));
-	mtk_gpio_w32(rg, GPIO_REG_HLVL, high & ~PIN_MASK(pin));
-	mtk_gpio_w32(rg, GPIO_REG_LLVL, low & ~PIN_MASK(pin));
+	mtk_gpio_w32(rg, GPIO_REG_FEDGE, fall & ~BIT(pin));
+	mtk_gpio_w32(rg, GPIO_REG_REDGE, rise & ~BIT(pin));
+	mtk_gpio_w32(rg, GPIO_REG_HLVL, high & ~BIT(pin));
+	mtk_gpio_w32(rg, GPIO_REG_LLVL, low & ~BIT(pin));
 	spin_unlock_irqrestore(&rg->lock, flags);
 }
 
@@ -170,7 +169,7 @@ mediatek_gpio_irq_type(struct irq_data *d, unsigned int type)
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct mtk_gc *rg = to_mediatek_gpio(gc);
 	int pin = d->hwirq;
-	u32 mask = PIN_MASK(pin);
+	u32 mask = BIT(pin);
 
 	if (!rg)
 		return -1;
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v5 10/18] staging: mt7621-gpio: update kerneldoc for state containers

[Sergio Paracuellos]

Update kernel doc for mtk_data and also remove no needed
documentation for mtk_gc which is clear enough to don't
need it.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index a8893e8..5f7d524c 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -29,16 +29,6 @@
 #define GPIO_REG_STAT	0x90
 #define GPIO_REG_EDGE	0xA0
 
-/**
- * struct mtk_gc - data for a single gpio-chip
- * @chip: gpio chip instance
- * @lock: spinlock to protect the chip
- * @bank: gpio bank number for the chip
- * @rising: mask for rising irqs
- * @falling: mask for falling irqs
- * @hlevel: mask for high level irqs
- * @llevel: mask for low level irqs
- */
 struct mtk_gc {
 	struct gpio_chip chip;
 	spinlock_t lock;
@@ -51,8 +41,9 @@ struct mtk_gc {
 
 /**
  * struct mtk_data - state container for
- * data of the platform driver. It is a
- * single irq-chip but 3 separate gpio-chip.
+ * data of the platform driver. It is 3
+ * separate gpio-chip each one with its
+ * own irq_chip.
  * @dev: device instance
  * @gpio_membase: memory base address
  * @gpio_irq: irq number from the device tree
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v5 11/18] staging: mt7621-gpio: align indentation for all defines

[Sergio Paracuellos]

There was two remaining defines which weren't properly
aligned with the rest. Align them improving readability.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index 5f7d524c..853817a 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -13,8 +13,8 @@
 #include <linux/platform_device.h>
 #include <linux/spinlock.h>
 
-#define MTK_BANK_CNT		3
-#define MTK_BANK_WIDTH		32
+#define MTK_BANK_CNT	3
+#define MTK_BANK_WIDTH	32
 
 #define GPIO_BANK_WIDE	0x04
 #define GPIO_REG_CTRL	0x00
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v5 12/18] staging: mt7621-gpio: avoid check for NULL in 'to_mediatek_gpio' calls

[Sergio Paracuellos]

Function 'to_mediatek_gpio' cannot return NULL, so this NULL
checkings are pointless.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index 853817a..1318003 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -115,9 +115,6 @@ mediatek_gpio_irq_unmask(struct irq_data *d)
 	unsigned long flags;
 	u32 rise, fall, high, low;
 
-	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);
@@ -139,9 +136,6 @@ mediatek_gpio_irq_mask(struct irq_data *d)
 	unsigned long flags;
 	u32 rise, fall, high, low;
 
-	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);
@@ -162,9 +156,6 @@ mediatek_gpio_irq_type(struct irq_data *d, unsigned int type)
 	int pin = d->hwirq;
 	u32 mask = BIT(pin);
 
-	if (!rg)
-		return -1;
-
 	if (type == IRQ_TYPE_PROBE) {
 		if ((rg->rising | rg->falling |
 		     rg->hlevel | rg->llevel) & mask)
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v5 13/18] staging: mt7621-gpio: avoid to set up irqs if not defined in dts

[Sergio Paracuellos]

If there is no interrupt defined in the dts 'irq_of_parse_and_map'
returns 0 and we should't set up interrupts for each gpio chip in
that case.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 42 ++++++++++++++++---------------
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index 1318003..96dee10 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -237,30 +237,32 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
 		return ret;
 	}
 
-	/*
-	 * Manually request the irq here instead of passing a flow-handler
-	 * to gpiochip_set_chained_irqchip, because the irq is shared.
-	 */
-	ret = devm_request_irq(&pdev->dev, gpio->gpio_irq,
-			       mediatek_gpio_irq_handler, IRQF_SHARED,
-			       "mt7621", &rg->chip);
+	if (gpio->gpio_irq) {
+		/*
+		 * Manually request the irq here instead of passing a flow-handler
+		 * to gpiochip_set_chained_irqchip, because the irq is shared.
+		 */
+		ret = devm_request_irq(&pdev->dev, gpio->gpio_irq,
+				       mediatek_gpio_irq_handler, IRQF_SHARED,
+				       "mt7621", &rg->chip);
+
+		if (ret) {
+			dev_err(&pdev->dev, "Error requesting IRQ %d: %d\n",
+				gpio->gpio_irq, ret);
+			return ret;
+		}
 
-	if (ret) {
-		dev_err(&pdev->dev, "Error requesting IRQ %d: %d\n",
-			gpio->gpio_irq, ret);
-		return ret;
-	}
+		ret = gpiochip_irqchip_add(&rg->chip, &mediatek_gpio_irq_chip,
+					   0, handle_simple_irq, IRQ_TYPE_NONE);
+		if (ret) {
+			dev_err(&pdev->dev, "failed to add gpiochip_irqchip\n");
+			return ret;
+		}
 
-	ret = gpiochip_irqchip_add(&rg->chip, &mediatek_gpio_irq_chip,
-				   0, handle_simple_irq, IRQ_TYPE_NONE);
-	if (ret) {
-		dev_err(&pdev->dev, "failed to add gpiochip_irqchip\n");
-		return ret;
+		gpiochip_set_chained_irqchip(&rg->chip, &mediatek_gpio_irq_chip,
+					     gpio->gpio_irq, NULL);
 	}
 
-	gpiochip_set_chained_irqchip(&rg->chip, &mediatek_gpio_irq_chip,
-				     gpio->gpio_irq, NULL);
-
 	/* set polarity to low for all gpios */
 	mtk_gpio_w32(rg, GPIO_REG_POL, 0);
 
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v5 14/18] staging: mt7621-gpio: avoid one level indentation in interrupt handler

[Sergio Paracuellos]

There is no need to check for 'pending' before loop over the
interrupts using 'for_each_set_bit' if nothing is set the
return values will be the same so just avoid this check avoiding
also one level intentation and improving readability.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index 96dee10..698a95d 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -93,14 +93,12 @@ mediatek_gpio_irq_handler(int irq, void *data)
 
 	pending = mtk_gpio_r32(rg, GPIO_REG_STAT);
 
-	if (pending) {
-		for_each_set_bit(bit, &pending, MTK_BANK_WIDTH) {
-			u32 map = irq_find_mapping(gc->irq.domain, bit);
+	for_each_set_bit(bit, &pending, MTK_BANK_WIDTH) {
+		u32 map = irq_find_mapping(gc->irq.domain, bit);
 
-			generic_handle_irq(map);
-			mtk_gpio_w32(rg, GPIO_REG_STAT, BIT(bit));
-			ret |= IRQ_HANDLED;
-		}
+		generic_handle_irq(map);
+		mtk_gpio_w32(rg, GPIO_REG_STAT, BIT(bit));
+		ret |= IRQ_HANDLED;
 	}
 
 	return ret;
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v5 15/18] staging: mt7621-gpio: set different names for each gpio_chip and irq_chip

[Sergio Paracuellos]

Currently the driver defines 3 gpiochips, one for each bank.

/sys/class/gpio/gpiochip416/label:1e000600.gpio
/sys/class/gpio/gpiochip448/label:1e000600.gpio
/sys/class/gpio/gpiochip480/label:1e000600.gpio

Unfortunately they all have the same label

Interrupts from /proc/interrupt show the same name which is
confusing:

/proc/interrupts:

17: 0  0  0  0  MIPS GIC  19  mt7621, mt7621, mt7621

which is the interrupt from the GPIO controller.
It is a little weird that all three banks are named "mt7621"
here. We also have:

26: 0  0  0  0  GPIO  18  reset

which is the interrupt from GPIO which provides the "reset"
button. I suspect that if I had interrupts form two different
banks they would both be called "GPIO" which would be a little
confusing.

In order to unify all of this set different names for each chip
Use a 'bank-based' name instead the same for all: 'mt7621-bank[0-2]'.
Create a new 'mediatek_gpio_bank_name' function which return the
name depending on the bank number. This function is allways called
with a valid index 0, 1 or 2.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index 698a95d..63fb5a1 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -190,13 +190,21 @@ mediatek_gpio_irq_type(struct irq_data *d, unsigned int type)
 }
 
 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 inline const char * const mediatek_gpio_bank_name(int bank)
+{
+	static const char * const bank_names[] = {
+		"mt7621-bank0", "mt7621-bank1", "mt7621-bank2",
+	};
+
+	return bank_names[bank];
+}
+
 static int
 mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
 {
@@ -215,6 +223,7 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
 	spin_lock_init(&rg->lock);
 	rg->chip.of_node = bank;
 	rg->bank = be32_to_cpu(*id);
+	rg->chip.label = mediatek_gpio_bank_name(rg->bank);
 
 	dat = gpio->gpio_membase + GPIO_REG_DATA + (rg->bank * GPIO_BANK_WIDE);
 	set = gpio->gpio_membase + GPIO_REG_DSET + (rg->bank * GPIO_BANK_WIDE);
@@ -242,7 +251,7 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
 		 */
 		ret = devm_request_irq(&pdev->dev, gpio->gpio_irq,
 				       mediatek_gpio_irq_handler, IRQF_SHARED,
-				       "mt7621", &rg->chip);
+				       rg->chip.label, &rg->chip);
 
 		if (ret) {
 			dev_err(&pdev->dev, "Error requesting IRQ %d: %d\n",
@@ -250,6 +259,7 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
 			return ret;
 		}
 
+		mediatek_gpio_irq_chip.name = rg->chip.label;
 		ret = gpiochip_irqchip_add(&rg->chip, &mediatek_gpio_irq_chip,
 					   0, handle_simple_irq, IRQ_TYPE_NONE);
 		if (ret) {
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v5 16/18] staging: mt7621-gpio: avoid long line in a comment

[Sergio Paracuellos]

Checkpatch script is complaining about a comment line
which exceeds 80 characteres. Just silence it.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index 63fb5a1..9a4a12f 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -246,8 +246,9 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
 
 	if (gpio->gpio_irq) {
 		/*
-		 * Manually request the irq here instead of passing a flow-handler
-		 * to gpiochip_set_chained_irqchip, because the irq is shared.
+		 * Manually request the irq here instead of passing
+		 * a flow-handler to gpiochip_set_chained_irqchip,
+		 * because the irq is shared.
 		 */
 		ret = devm_request_irq(&pdev->dev, gpio->gpio_irq,
 				       mediatek_gpio_irq_handler, IRQF_SHARED,
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v5 17/18] staging: mt7621-gpio: update Kconfig with SoC details

[Sergio Paracuellos]

Kconfig is using a generic 'Mediatek GPIO Support' in
description and help which is not specific at all about
the current SoC which is MT7621. Update it.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-gpio/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/mt7621-gpio/Kconfig b/drivers/staging/mt7621-gpio/Kconfig
index e32facb..5485dd2 100644
--- a/drivers/staging/mt7621-gpio/Kconfig
+++ b/drivers/staging/mt7621-gpio/Kconfig
@@ -1,8 +1,8 @@
 config GPIO_MT7621
-	bool "Mediatek GPIO Support"
+	bool "Mediatek MT7621 GPIO Support"
 	depends on SOC_MT7620 || SOC_MT7621 || COMPILE_TEST
 	select GPIO_GENERIC
 	select GPIOLIB_IRQCHIP
 	select ARCH_REQUIRE_GPIOLIB
 	help
-	  Say yes here to support the Mediatek SoC GPIO device
+	  Say yes here to support the Mediatek MT7621 SoC GPIO device
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v5 18/18] staging: mt7621-gpio: avoid use banks in device tree

[Sergio Paracuellos]

Banks shouldn't be defined in DT if number of resources
per bank is not variable. We actually know that this SoC
has three banks so take that into account in order to don't
overspecify the device tree. Device tree will only have one
node making it simple. Update device tree, binding doc and
code accordly.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-dts/gbpc1.dts               | 10 ++--
 drivers/staging/mt7621-dts/mt7621.dtsi             | 31 ++----------
 drivers/staging/mt7621-gpio/gpio-mt7621.c          | 21 ++++----
 .../staging/mt7621-gpio/mediatek,mt7621-gpio.txt   | 59 +++++-----------------
 4 files changed, 31 insertions(+), 90 deletions(-)

diff --git a/drivers/staging/mt7621-dts/gbpc1.dts b/drivers/staging/mt7621-dts/gbpc1.dts
index 6b13d85..352945d 100644
--- a/drivers/staging/mt7621-dts/gbpc1.dts
+++ b/drivers/staging/mt7621-dts/gbpc1.dts
@@ -32,7 +32,7 @@
 
 		reset {
 			label = "reset";
-			gpios = <&gpio0 18 GPIO_ACTIVE_HIGH>;
+			gpios = <&gpio 18 GPIO_ACTIVE_HIGH>;
 			linux,code = <KEY_RESTART>;
 		};
 	};
@@ -42,22 +42,22 @@
 
 		system {
 			label = "gb-pc1:green:system";
-			gpios = <&gpio0 6 GPIO_ACTIVE_LOW>;
+			gpios = <&gpio 6 GPIO_ACTIVE_LOW>;
 		};
 
 		status {
 			label = "gb-pc1:green:status";
-			gpios = <&gpio0 8 GPIO_ACTIVE_LOW>;
+			gpios = <&gpio 8 GPIO_ACTIVE_LOW>;
 		};
 
 		lan1 {
 			label = "gb-pc1:green:lan1";
-			gpios = <&gpio0 24 GPIO_ACTIVE_LOW>;
+			gpios = <&gpio 24 GPIO_ACTIVE_LOW>;
 		};
 
 		lan2 {
 			label = "gb-pc1:green:lan2";
-			gpios = <&gpio0 25 GPIO_ACTIVE_LOW>;
+			gpios = <&gpio 25 GPIO_ACTIVE_LOW>;
 		};
 	};
 };
diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi b/drivers/staging/mt7621-dts/mt7621.dtsi
index acb6b8c..02746af 100644
--- a/drivers/staging/mt7621-dts/mt7621.dtsi
+++ b/drivers/staging/mt7621-dts/mt7621.dtsi
@@ -61,37 +61,14 @@
 		};
 
 		gpio: gpio@600 {
-			#address-cells = <1>;
-			#size-cells = <0>;
-
+			#gpio-cells = <2>;
+			#interrupt-cells = <2>;
 			compatible = "mediatek,mt7621-gpio";
+			gpio-controller;
+			interrupt-controller;
 			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>;
-			};
 		};
 
 		i2c: i2c@900 {
diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index 9a4a12f..281e621 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -206,23 +206,20 @@ static inline const char * const mediatek_gpio_bank_name(int bank)
 }
 
 static int
-mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
+mediatek_gpio_bank_probe(struct platform_device *pdev,
+			 struct device_node *node, int bank)
 {
 	struct mtk_data *gpio = dev_get_drvdata(&pdev->dev);
-	const __be32 *id = of_get_property(bank, "reg", NULL);
 	struct mtk_gc *rg;
 	void __iomem *dat, *set, *ctrl, *diro;
 	int ret;
 
-	if (!id || be32_to_cpu(*id) >= MTK_BANK_CNT)
-		return -EINVAL;
-
-	rg = &gpio->gc_map[be32_to_cpu(*id)];
+	rg = &gpio->gc_map[bank];
 	memset(rg, 0, sizeof(*rg));
 
 	spin_lock_init(&rg->lock);
-	rg->chip.of_node = bank;
-	rg->bank = be32_to_cpu(*id);
+	rg->chip.of_node = node;
+	rg->bank = bank;
 	rg->chip.label = mediatek_gpio_bank_name(rg->bank);
 
 	dat = gpio->gpio_membase + GPIO_REG_DATA + (rg->bank * GPIO_BANK_WIDE);
@@ -283,9 +280,10 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
 static int
 mediatek_gpio_probe(struct platform_device *pdev)
 {
-	struct device_node *bank, *np = pdev->dev.of_node;
+	struct device_node *np = pdev->dev.of_node;
 	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	struct mtk_data *gpio_data;
+	int i;
 
 	gpio_data = devm_kzalloc(&pdev->dev, sizeof(*gpio_data), GFP_KERNEL);
 	if (!gpio_data)
@@ -299,9 +297,8 @@ mediatek_gpio_probe(struct platform_device *pdev)
 	gpio_data->dev = &pdev->dev;
 	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);
+	for (i = 0; i < MTK_BANK_CNT; i++)
+		mediatek_gpio_bank_probe(pdev, np, i);
 
 	return 0;
 }
diff --git a/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt b/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
index 30d8a02..ba45558 100644
--- a/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
+++ b/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
@@ -1,68 +1,35 @@
-Mediatek SoC GPIO controller bindings
+Mediatek MT7621 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
+We load one GPIO controller instance per 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:
+- #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.
+- #interrupt-cells : Specifies the number of cells needed to encode an
+   interrupt. Should be 2. The first cell defines the interrupt number,
+   the second encodes the triger flags encoded as described in
+   Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
 - 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>;
-
+		#gpio-cells = <2>;
+		#interrupt-cells = <2>;
 		compatible = "mediatek,mt7621-gpio";
+		gpio-controller;
+		interrupt-controller;
 		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

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 18/18] staging: mt7621-gpio: avoid use banks in device tree

[NeilBrown]

[-- Attachment #1.1: Type: text/plain, Size: 9115 bytes --]

On Mon, Jun 18 2018, Sergio Paracuellos wrote:

> Banks shouldn't be defined in DT if number of resources
> per bank is not variable. We actually know that this SoC
> has three banks so take that into account in order to don't
> overspecify the device tree. Device tree will only have one
> node making it simple. Update device tree, binding doc and
> code accordly.
>
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>

I'm sorry that I've been silent one these for a while - busy any all
that.

This last patch doesn't work.  My test case works with all but this one
applied, but this breaks it.  I haven't had a chance to look into why
yet.  Sorry.

NeilBrown

> ---
>  drivers/staging/mt7621-dts/gbpc1.dts               | 10 ++--
>  drivers/staging/mt7621-dts/mt7621.dtsi             | 31 ++----------
>  drivers/staging/mt7621-gpio/gpio-mt7621.c          | 21 ++++----
>  .../staging/mt7621-gpio/mediatek,mt7621-gpio.txt   | 59 +++++-----------------
>  4 files changed, 31 insertions(+), 90 deletions(-)
>
> diff --git a/drivers/staging/mt7621-dts/gbpc1.dts b/drivers/staging/mt7621-dts/gbpc1.dts
> index 6b13d85..352945d 100644
> --- a/drivers/staging/mt7621-dts/gbpc1.dts
> +++ b/drivers/staging/mt7621-dts/gbpc1.dts
> @@ -32,7 +32,7 @@
>  
>  		reset {
>  			label = "reset";
> -			gpios = <&gpio0 18 GPIO_ACTIVE_HIGH>;
> +			gpios = <&gpio 18 GPIO_ACTIVE_HIGH>;
>  			linux,code = <KEY_RESTART>;
>  		};
>  	};
> @@ -42,22 +42,22 @@
>  
>  		system {
>  			label = "gb-pc1:green:system";
> -			gpios = <&gpio0 6 GPIO_ACTIVE_LOW>;
> +			gpios = <&gpio 6 GPIO_ACTIVE_LOW>;
>  		};
>  
>  		status {
>  			label = "gb-pc1:green:status";
> -			gpios = <&gpio0 8 GPIO_ACTIVE_LOW>;
> +			gpios = <&gpio 8 GPIO_ACTIVE_LOW>;
>  		};
>  
>  		lan1 {
>  			label = "gb-pc1:green:lan1";
> -			gpios = <&gpio0 24 GPIO_ACTIVE_LOW>;
> +			gpios = <&gpio 24 GPIO_ACTIVE_LOW>;
>  		};
>  
>  		lan2 {
>  			label = "gb-pc1:green:lan2";
> -			gpios = <&gpio0 25 GPIO_ACTIVE_LOW>;
> +			gpios = <&gpio 25 GPIO_ACTIVE_LOW>;
>  		};
>  	};
>  };
> diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi b/drivers/staging/mt7621-dts/mt7621.dtsi
> index acb6b8c..02746af 100644
> --- a/drivers/staging/mt7621-dts/mt7621.dtsi
> +++ b/drivers/staging/mt7621-dts/mt7621.dtsi
> @@ -61,37 +61,14 @@
>  		};
>  
>  		gpio: gpio@600 {
> -			#address-cells = <1>;
> -			#size-cells = <0>;
> -
> +			#gpio-cells = <2>;
> +			#interrupt-cells = <2>;
>  			compatible = "mediatek,mt7621-gpio";
> +			gpio-controller;
> +			interrupt-controller;
>  			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>;
> -			};
>  		};
>  
>  		i2c: i2c@900 {
> diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> index 9a4a12f..281e621 100644
> --- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
> +++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> @@ -206,23 +206,20 @@ static inline const char * const mediatek_gpio_bank_name(int bank)
>  }
>  
>  static int
> -mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
> +mediatek_gpio_bank_probe(struct platform_device *pdev,
> +			 struct device_node *node, int bank)
>  {
>  	struct mtk_data *gpio = dev_get_drvdata(&pdev->dev);
> -	const __be32 *id = of_get_property(bank, "reg", NULL);
>  	struct mtk_gc *rg;
>  	void __iomem *dat, *set, *ctrl, *diro;
>  	int ret;
>  
> -	if (!id || be32_to_cpu(*id) >= MTK_BANK_CNT)
> -		return -EINVAL;
> -
> -	rg = &gpio->gc_map[be32_to_cpu(*id)];
> +	rg = &gpio->gc_map[bank];
>  	memset(rg, 0, sizeof(*rg));
>  
>  	spin_lock_init(&rg->lock);
> -	rg->chip.of_node = bank;
> -	rg->bank = be32_to_cpu(*id);
> +	rg->chip.of_node = node;
> +	rg->bank = bank;
>  	rg->chip.label = mediatek_gpio_bank_name(rg->bank);
>  
>  	dat = gpio->gpio_membase + GPIO_REG_DATA + (rg->bank * GPIO_BANK_WIDE);
> @@ -283,9 +280,10 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
>  static int
>  mediatek_gpio_probe(struct platform_device *pdev)
>  {
> -	struct device_node *bank, *np = pdev->dev.of_node;
> +	struct device_node *np = pdev->dev.of_node;
>  	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	struct mtk_data *gpio_data;
> +	int i;
>  
>  	gpio_data = devm_kzalloc(&pdev->dev, sizeof(*gpio_data), GFP_KERNEL);
>  	if (!gpio_data)
> @@ -299,9 +297,8 @@ mediatek_gpio_probe(struct platform_device *pdev)
>  	gpio_data->dev = &pdev->dev;
>  	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);
> +	for (i = 0; i < MTK_BANK_CNT; i++)
> +		mediatek_gpio_bank_probe(pdev, np, i);
>  
>  	return 0;
>  }
> diff --git a/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt b/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
> index 30d8a02..ba45558 100644
> --- a/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
> +++ b/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
> @@ -1,68 +1,35 @@
> -Mediatek SoC GPIO controller bindings
> +Mediatek MT7621 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
> +We load one GPIO controller instance per 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:
> +- #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.
> +- #interrupt-cells : Specifies the number of cells needed to encode an
> +   interrupt. Should be 2. The first cell defines the interrupt number,
> +   the second encodes the triger flags encoded as described in
> +   Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
>  - 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>;
> -
> +		#gpio-cells = <2>;
> +		#interrupt-cells = <2>;
>  		compatible = "mediatek,mt7621-gpio";
> +		gpio-controller;
> +		interrupt-controller;
>  		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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 169 bytes --]

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 18/18] staging: mt7621-gpio: avoid use banks in device tree

[Sergio Paracuellos]

On Sat, Jun 30, 2018 at 7:47 AM, NeilBrown <neil@brown.name> wrote:
> On Mon, Jun 18 2018, Sergio Paracuellos wrote:
>
>> Banks shouldn't be defined in DT if number of resources
>> per bank is not variable. We actually know that this SoC
>> has three banks so take that into account in order to don't
>> overspecify the device tree. Device tree will only have one
>> node making it simple. Update device tree, binding doc and
>> code accordly.
>>
>> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
>
> I'm sorry that I've been silent one these for a while - busy any all
> that.
>
> This last patch doesn't work.  My test case works with all but this one
> applied, but this breaks it.  I haven't had a chance to look into why
> yet.  Sorry.

Thanks for pointing this out. All of them were applied so I though
there were no problem at all with any of them.
We should revert the last one or wait to your feedback about what is
breaking it and send a new patch fixing the problem.

Thanks in advance.

> NeilBrown

Best regards,
    Sergio Paracuellos

>
>> ---
>>  drivers/staging/mt7621-dts/gbpc1.dts               | 10 ++--
>>  drivers/staging/mt7621-dts/mt7621.dtsi             | 31 ++----------
>>  drivers/staging/mt7621-gpio/gpio-mt7621.c          | 21 ++++----
>>  .../staging/mt7621-gpio/mediatek,mt7621-gpio.txt   | 59 +++++-----------------
>>  4 files changed, 31 insertions(+), 90 deletions(-)
>>
>> diff --git a/drivers/staging/mt7621-dts/gbpc1.dts b/drivers/staging/mt7621-dts/gbpc1.dts
>> index 6b13d85..352945d 100644
>> --- a/drivers/staging/mt7621-dts/gbpc1.dts
>> +++ b/drivers/staging/mt7621-dts/gbpc1.dts
>> @@ -32,7 +32,7 @@
>>
>>               reset {
>>                       label = "reset";
>> -                     gpios = <&gpio0 18 GPIO_ACTIVE_HIGH>;
>> +                     gpios = <&gpio 18 GPIO_ACTIVE_HIGH>;
>>                       linux,code = <KEY_RESTART>;
>>               };
>>       };
>> @@ -42,22 +42,22 @@
>>
>>               system {
>>                       label = "gb-pc1:green:system";
>> -                     gpios = <&gpio0 6 GPIO_ACTIVE_LOW>;
>> +                     gpios = <&gpio 6 GPIO_ACTIVE_LOW>;
>>               };
>>
>>               status {
>>                       label = "gb-pc1:green:status";
>> -                     gpios = <&gpio0 8 GPIO_ACTIVE_LOW>;
>> +                     gpios = <&gpio 8 GPIO_ACTIVE_LOW>;
>>               };
>>
>>               lan1 {
>>                       label = "gb-pc1:green:lan1";
>> -                     gpios = <&gpio0 24 GPIO_ACTIVE_LOW>;
>> +                     gpios = <&gpio 24 GPIO_ACTIVE_LOW>;
>>               };
>>
>>               lan2 {
>>                       label = "gb-pc1:green:lan2";
>> -                     gpios = <&gpio0 25 GPIO_ACTIVE_LOW>;
>> +                     gpios = <&gpio 25 GPIO_ACTIVE_LOW>;
>>               };
>>       };
>>  };
>> diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi b/drivers/staging/mt7621-dts/mt7621.dtsi
>> index acb6b8c..02746af 100644
>> --- a/drivers/staging/mt7621-dts/mt7621.dtsi
>> +++ b/drivers/staging/mt7621-dts/mt7621.dtsi
>> @@ -61,37 +61,14 @@
>>               };
>>
>>               gpio: gpio@600 {
>> -                     #address-cells = <1>;
>> -                     #size-cells = <0>;
>> -
>> +                     #gpio-cells = <2>;
>> +                     #interrupt-cells = <2>;
>>                       compatible = "mediatek,mt7621-gpio";
>> +                     gpio-controller;
>> +                     interrupt-controller;
>>                       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>;
>> -                     };
>>               };
>>
>>               i2c: i2c@900 {
>> diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
>> index 9a4a12f..281e621 100644
>> --- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
>> +++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
>> @@ -206,23 +206,20 @@ static inline const char * const mediatek_gpio_bank_name(int bank)
>>  }
>>
>>  static int
>> -mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
>> +mediatek_gpio_bank_probe(struct platform_device *pdev,
>> +                      struct device_node *node, int bank)
>>  {
>>       struct mtk_data *gpio = dev_get_drvdata(&pdev->dev);
>> -     const __be32 *id = of_get_property(bank, "reg", NULL);
>>       struct mtk_gc *rg;
>>       void __iomem *dat, *set, *ctrl, *diro;
>>       int ret;
>>
>> -     if (!id || be32_to_cpu(*id) >= MTK_BANK_CNT)
>> -             return -EINVAL;
>> -
>> -     rg = &gpio->gc_map[be32_to_cpu(*id)];
>> +     rg = &gpio->gc_map[bank];
>>       memset(rg, 0, sizeof(*rg));
>>
>>       spin_lock_init(&rg->lock);
>> -     rg->chip.of_node = bank;
>> -     rg->bank = be32_to_cpu(*id);
>> +     rg->chip.of_node = node;
>> +     rg->bank = bank;
>>       rg->chip.label = mediatek_gpio_bank_name(rg->bank);
>>
>>       dat = gpio->gpio_membase + GPIO_REG_DATA + (rg->bank * GPIO_BANK_WIDE);
>> @@ -283,9 +280,10 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
>>  static int
>>  mediatek_gpio_probe(struct platform_device *pdev)
>>  {
>> -     struct device_node *bank, *np = pdev->dev.of_node;
>> +     struct device_node *np = pdev->dev.of_node;
>>       struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>       struct mtk_data *gpio_data;
>> +     int i;
>>
>>       gpio_data = devm_kzalloc(&pdev->dev, sizeof(*gpio_data), GFP_KERNEL);
>>       if (!gpio_data)
>> @@ -299,9 +297,8 @@ mediatek_gpio_probe(struct platform_device *pdev)
>>       gpio_data->dev = &pdev->dev;
>>       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);
>> +     for (i = 0; i < MTK_BANK_CNT; i++)
>> +             mediatek_gpio_bank_probe(pdev, np, i);
>>
>>       return 0;
>>  }
>> diff --git a/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt b/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
>> index 30d8a02..ba45558 100644
>> --- a/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
>> +++ b/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
>> @@ -1,68 +1,35 @@
>> -Mediatek SoC GPIO controller bindings
>> +Mediatek MT7621 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
>> +We load one GPIO controller instance per 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:
>> +- #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.
>> +- #interrupt-cells : Specifies the number of cells needed to encode an
>> +   interrupt. Should be 2. The first cell defines the interrupt number,
>> +   the second encodes the triger flags encoded as described in
>> +   Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
>>  - 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>;
>> -
>> +             #gpio-cells = <2>;
>> +             #interrupt-cells = <2>;
>>               compatible = "mediatek,mt7621-gpio";
>> +             gpio-controller;
>> +             interrupt-controller;
>>               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 v5 18/18] staging: mt7621-gpio: avoid use banks in device tree

[NeilBrown]

[-- Attachment #1.1: Type: text/plain, Size: 4334 bytes --]

On Sat, Jun 30 2018, Sergio Paracuellos wrote:

> On Sat, Jun 30, 2018 at 7:47 AM, NeilBrown <neil@brown.name> wrote:
>> On Mon, Jun 18 2018, Sergio Paracuellos wrote:
>>
>>> Banks shouldn't be defined in DT if number of resources
>>> per bank is not variable. We actually know that this SoC
>>> has three banks so take that into account in order to don't
>>> overspecify the device tree. Device tree will only have one
>>> node making it simple. Update device tree, binding doc and
>>> code accordly.
>>>
>>> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
>>
>> I'm sorry that I've been silent one these for a while - busy any all
>> that.
>>
>> This last patch doesn't work.  My test case works with all but this one
>> applied, but this breaks it.  I haven't had a chance to look into why
>> yet.  Sorry.
>
> Thanks for pointing this out. All of them were applied so I though
> there were no problem at all with any of them.
> We should revert the last one or wait to your feedback about what is
> breaking it and send a new patch fixing the problem.
>

OK, I finally made time to dig into this.
The problem is that the default gpio.of_xlate function assumes there is
one gpio chip for each devicetree node.  With this patch applied, the
one device tree node corresponds to 3 different gpio chips.  For that to
work we need an xlate function.
See below for what I wrote to get it working.
With this in place:
 /sys/class/gpio still contains:
    export	gpiochip416  gpiochip448  gpiochip480  unexport

which is a little annoying, but unavoidable I guess.
The labels on these are:

# grep . /sys/class/gpio/gpiochip4*/label
/sys/class/gpio/gpiochip416/label:1e000600.gpio
/sys/class/gpio/gpiochip448/label:1e000600.gpio
/sys/class/gpio/gpiochip480/label:1e000600.gpio

.. all the same, which is not ideal.

Your attempt to change the names doesn't work because bgpio_init() sets
the names itself.  If you move the assignment to rg->chip.label to
*after* the call to bgpio_init(), the new names work:

#  grep . /sys/class/gpio/gpiochip4*/label
/sys/class/gpio/gpiochip416/label:mt7621-bank2
/sys/class/gpio/gpiochip448/label:mt7621-bank1
/sys/class/gpio/gpiochip480/label:mt7621-bank0

Also with that change /proc/interrupts contains:

 17:          0          0          0          0  MIPS GIC  19  mt7621-bank0, mt7621-bank1, mt7621-bank2

and

 26:          0          0          0          0  mt7621-bank2  18  reset

The first line looks good - though having "gpio" in the name might be
good. Should they be 1e000600.gpio-bankN" ??

The second is a bit weird, as this isn't bank2.
It probably makes sense to have "1e000600.gpio  18  reset" there...

There is only 1 irq chip, compared with 3 gpio chips, so a different
name is appropriate.  I changed the irq chip name:

		mediatek_gpio_irq_chip.name = dev_name(&pdev->dev);

though maybe that assignment should go elsewhere - maybe in
mediatek_gpio_probe() as it is only needed once.


Thanks,
NeilBrown





diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index 281e6214d543..814af9342d25 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -205,6 +205,22 @@ static inline const char * const mediatek_gpio_bank_name(int bank)
 	return bank_names[bank];
 }
 
+static int mediatek_gpio_xlate(struct gpio_chip *chip,
+			       const struct of_phandle_args *spec,
+			       u32 *flags)
+{
+	int gpio = spec->args[0];
+	struct mtk_gc *rq = container_of(chip, struct mtk_gc, chip);
+
+	if (rq->bank != gpio / MTK_BANK_WIDTH)
+		return -EINVAL;
+
+	if (flags)
+		*flags = spec->args[1];
+
+	return gpio % MTK_BANK_WIDTH;
+}
+
 static int
 mediatek_gpio_bank_probe(struct platform_device *pdev,
 			 struct device_node *node, int bank)
@@ -221,6 +237,8 @@ mediatek_gpio_bank_probe(struct platform_device *pdev,
 	rg->chip.of_node = node;
 	rg->bank = bank;
 	rg->chip.label = mediatek_gpio_bank_name(rg->bank);
+	rg->chip.of_gpio_n_cells = 2;
+	rg->chip.of_xlate = mediatek_gpio_xlate;
 
 	dat = gpio->gpio_membase + GPIO_REG_DATA + (rg->bank * GPIO_BANK_WIDE);
 	set = gpio->gpio_membase + GPIO_REG_DSET + (rg->bank * GPIO_BANK_WIDE);

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 169 bytes --]

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 18/18] staging: mt7621-gpio: avoid use banks in device tree

[Sergio Paracuellos]

On Thu, Jul 05, 2018 at 10:51:39AM +1000, NeilBrown wrote:
> On Sat, Jun 30 2018, Sergio Paracuellos wrote:
> 
> > On Sat, Jun 30, 2018 at 7:47 AM, NeilBrown <neil@brown.name> wrote:
> >> On Mon, Jun 18 2018, Sergio Paracuellos wrote:
> >>
> >>> Banks shouldn't be defined in DT if number of resources
> >>> per bank is not variable. We actually know that this SoC
> >>> has three banks so take that into account in order to don't
> >>> overspecify the device tree. Device tree will only have one
> >>> node making it simple. Update device tree, binding doc and
> >>> code accordly.
> >>>
> >>> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> >>
> >> I'm sorry that I've been silent one these for a while - busy any all
> >> that.
> >>
> >> This last patch doesn't work.  My test case works with all but this one
> >> applied, but this breaks it.  I haven't had a chance to look into why
> >> yet.  Sorry.
> >
> > Thanks for pointing this out. All of them were applied so I though
> > there were no problem at all with any of them.
> > We should revert the last one or wait to your feedback about what is
> > breaking it and send a new patch fixing the problem.
> >
> 
> OK, I finally made time to dig into this.
> The problem is that the default gpio.of_xlate function assumes there is
> one gpio chip for each devicetree node.  With this patch applied, the
> one device tree node corresponds to 3 different gpio chips.  For that to
> work we need an xlate function.
> See below for what I wrote to get it working.
> With this in place:
>  /sys/class/gpio still contains:
>     export	gpiochip416  gpiochip448  gpiochip480  unexport
> 
> which is a little annoying, but unavoidable I guess.
> The labels on these are:
> 
> # grep . /sys/class/gpio/gpiochip4*/label
> /sys/class/gpio/gpiochip416/label:1e000600.gpio
> /sys/class/gpio/gpiochip448/label:1e000600.gpio
> /sys/class/gpio/gpiochip480/label:1e000600.gpio
> 
> .. all the same, which is not ideal.
> 
> Your attempt to change the names doesn't work because bgpio_init() sets
> the names itself.  If you move the assignment to rg->chip.label to
> *after* the call to bgpio_init(), the new names work:
> 
> #  grep . /sys/class/gpio/gpiochip4*/label
> /sys/class/gpio/gpiochip416/label:mt7621-bank2
> /sys/class/gpio/gpiochip448/label:mt7621-bank1
> /sys/class/gpio/gpiochip480/label:mt7621-bank0
> 
> Also with that change /proc/interrupts contains:
> 
>  17:          0          0          0          0  MIPS GIC  19  mt7621-bank0, mt7621-bank1, mt7621-bank2
> 
> and
> 
>  26:          0          0          0          0  mt7621-bank2  18  reset
> 
> The first line looks good - though having "gpio" in the name might be
> good. Should they be 1e000600.gpio-bankN" ??
> 
> The second is a bit weird, as this isn't bank2.
> It probably makes sense to have "1e000600.gpio  18  reset" there...
> 
> There is only 1 irq chip, compared with 3 gpio chips, so a different
> name is appropriate.  I changed the irq chip name:
> 
> 		mediatek_gpio_irq_chip.name = dev_name(&pdev->dev);
> 
> though maybe that assignment should go elsewhere - maybe in
> mediatek_gpio_probe() as it is only needed once.
> 
> 
> Thanks,
> NeilBrown
> 

Thanks for your time in looking into this and the explanation about
what is happening there, Neil.

I will send a new patch series including all the stuff pointed out here.

Best regards,
    Sergio Paracuellos
> 
> 
> 
> 
> diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> index 281e6214d543..814af9342d25 100644
> --- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
> +++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> @@ -205,6 +205,22 @@ static inline const char * const mediatek_gpio_bank_name(int bank)
>  	return bank_names[bank];
>  }
>  
> +static int mediatek_gpio_xlate(struct gpio_chip *chip,
> +			       const struct of_phandle_args *spec,
> +			       u32 *flags)
> +{
> +	int gpio = spec->args[0];
> +	struct mtk_gc *rq = container_of(chip, struct mtk_gc, chip);
> +
> +	if (rq->bank != gpio / MTK_BANK_WIDTH)
> +		return -EINVAL;
> +
> +	if (flags)
> +		*flags = spec->args[1];
> +
> +	return gpio % MTK_BANK_WIDTH;
> +}
> +
>  static int
>  mediatek_gpio_bank_probe(struct platform_device *pdev,
>  			 struct device_node *node, int bank)
> @@ -221,6 +237,8 @@ mediatek_gpio_bank_probe(struct platform_device *pdev,
>  	rg->chip.of_node = node;
>  	rg->bank = bank;
>  	rg->chip.label = mediatek_gpio_bank_name(rg->bank);
> +	rg->chip.of_gpio_n_cells = 2;
> +	rg->chip.of_xlate = mediatek_gpio_xlate;
>  
>  	dat = gpio->gpio_membase + GPIO_REG_DATA + (rg->bank * GPIO_BANK_WIDE);
>  	set = gpio->gpio_membase + GPIO_REG_DSET + (rg->bank * GPIO_BANK_WIDE);