[PATCH v3 0/8] staging: mt7621-gpio: last cleanups

[PATCH v3 0/8] 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 main of Linus Walleij:

http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-June/121742.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 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'. It should be working now??

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 (8):
  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

 drivers/staging/mt7621-gpio/Kconfig       |   4 +-
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 369 +++++++++++++-----------------
 2 files changed, 168 insertions(+), 205 deletions(-)

-- 
2.7.4

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

[PATCH v3 1/8] 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 v3 2/8] 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 v3 3/8] 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 v3 4/8] 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 v3 5/8] 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 v3 6/8] 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 v3 7/8] 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 v3 8/8] 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

Re: [PATCH v3 0/8] staging: mt7621-gpio: last cleanups

[NeilBrown]

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

On Mon, Jun 11 2018, Sergio Paracuellos wrote:

> After submiting this driver to try to get mainlined and get
> out of staging some new cleanups seems to be necessary.
> According to this main of Linus Walleij:
>
> http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-June/121742.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 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'. It should be working now??

Yes, it is working now - thanks.
With this series, the driver again works for all the tests I can
perform - except that some names aren't unique, as I've mentioned
separately.

Looking over the new code:

- I don't think we need PIN_MASK() any more.  We needed that
  when we had 1 irq_chip which handled 96 irqs.  Now we have
  3 irq_chips with 32 irqs each.

- documentation for 'struct mtk_data' says it is a single
  irqchip, but I don't think it is any more - there is one
  per gpio chip.
  Related: doco for 'struct mtk_gc' contains data for both
   the gpio_chip and the irq_chip.  I don't know if that
   needs to be spelled out.

- In
	if (pending) {
		for_each_set_bit(bit, &pending, MTK_BANK_WIDTH) {
  I wouldn't bother with the "if (pending)".
  If pending is zero, then find_each_set_bit() won't find anything.
  It is at most a minor optimization.
  This is a personal preference and if you like it that way, leave it.
  Though if you are keen to optimize, then instead of calling
  mtk_gpio_w32(...BIT(bit)) for every found bit, just call
  mtk_gpio_w32(... pending) once at the top.
  
- to_mediatek_gpio() cannot return NULL, so testing "if (!rg)" in
  several places is pointless.

- If the dts file doesn't specify an irq, the irq_of_parse_and_map()
  will return -1 (I think).  This might deserve a warning and probably
  shouldn't cause the probe to fail, but it should cause
  mediatek_gpio_bank_probe to avoid trying to set up interrupts.


Nothing serious, but some might be worth fixing.

Thanks a lot,
NeilBrown

[-- 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 v3 0/8] staging: mt7621-gpio: last cleanups

[Sergio Paracuellos]

On Tue, Jun 12, 2018 at 12:33:42PM +1000, NeilBrown wrote:
> On Mon, Jun 11 2018, Sergio Paracuellos wrote:
> 
> > After submiting this driver to try to get mainlined and get
> > out of staging some new cleanups seems to be necessary.
> > According to this main of Linus Walleij:
> >
> > http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-June/121742.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 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'. It should be working now??
> 
> Yes, it is working now - thanks.
> With this series, the driver again works for all the tests I can
> perform - except that some names aren't unique, as I've mentioned
> separately.

Awesome. Thanks for testing a review! Now that al is working it is 
time for small cleanups.

> 
> Looking over the new code:
> 
> - I don't think we need PIN_MASK() any more.  We needed that
>   when we had 1 irq_chip which handled 96 irqs.  Now we have
>   3 irq_chips with 32 irqs each.
> 
> - documentation for 'struct mtk_data' says it is a single
>   irqchip, but I don't think it is any more - there is one
>   per gpio chip.
>   Related: doco for 'struct mtk_gc' contains data for both
>    the gpio_chip and the irq_chip.  I don't know if that
>    needs to be spelled out.
> 
> - In
> 	if (pending) {
> 		for_each_set_bit(bit, &pending, MTK_BANK_WIDTH) {
>   I wouldn't bother with the "if (pending)".
>   If pending is zero, then find_each_set_bit() won't find anything.
>   It is at most a minor optimization.
>   This is a personal preference and if you like it that way, leave it.
>   Though if you are keen to optimize, then instead of calling
>   mtk_gpio_w32(...BIT(bit)) for every found bit, just call
>   mtk_gpio_w32(... pending) once at the top.
>   
> - to_mediatek_gpio() cannot return NULL, so testing "if (!rg)" in
>   several places is pointless.
> 
> - If the dts file doesn't specify an irq, the irq_of_parse_and_map()
>   will return -1 (I think).  This might deserve a warning and probably
>   shouldn't cause the probe to fail, but it should cause
>   mediatek_gpio_bank_probe to avoid trying to set up interrupts.
> 
> 
> Nothing serious, but some might be worth fixing.

Thanks for pointing out these in this mail also.

Hopefully I'll resend a new cleanups series in reply to this
mail.

> 
> Thanks a lot,
> NeilBrown

Best regards,
    Sergio Paracuellos


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

[PATCH 0/7] staging: mt7621-gpio: last minor cleanups

[Sergio Paracuellos]

Now that all seems to be working these are the last minor
cleanups in order to make a second try to get this out
of staging.

Sergio Paracuellos (7):
  staging: mt7621-gpio: remove no more necessary PIN_MASK macro
  staging: mt7621-gpio: update kerneldoc for state containers
  staging: mtk7621-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

 drivers/staging/mt7621-gpio/gpio-mt7621.c | 113 ++++++++++++++----------------
 1 file changed, 52 insertions(+), 61 deletions(-)

-- 
2.7.4

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

[PATCH 1/8] 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 2/8] 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 3/8] 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 4/8] 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 5/8] 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 6/8] 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 7/8] 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 8/8] 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