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

[PATCH 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.

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: avoid custom irq_domain for gpio
  staging: mt7621-gpio: implement high level and low level irqs

 drivers/staging/mt7621-gpio/Kconfig       |   5 +-
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 375 +++++++++++++++---------------
 2 files changed, 193 insertions(+), 187 deletions(-)

-- 
2.7.4

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

[PATCH 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 | 148 ++++++++++--------------------
 2 files changed, 47 insertions(+), 102 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..cc08505 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(bank)      (((bank) * GPIO_BANK_WIDE) + 0x00)
+#define GPIO_REG_POL(bank)       (((bank) * GPIO_BANK_WIDE) + 0x10)
+#define GPIO_REG_DATA(bank)      (((bank) * GPIO_BANK_WIDE) + 0x20)
+#define GPIO_REG_DSET(bank)      (((bank) * GPIO_BANK_WIDE) + 0x30)
+#define GPIO_REG_DCLR(bank)      (((bank) * GPIO_BANK_WIDE) + 0x40)
+#define GPIO_REG_REDGE(bank)     (((bank) * GPIO_BANK_WIDE) + 0x50)
+#define GPIO_REG_FEDGE(bank)     (((bank) * GPIO_BANK_WIDE) + 0x60)
+#define GPIO_REG_HLVL(bank)      (((bank) * GPIO_BANK_WIDE) + 0x70)
+#define GPIO_REG_LLVL(bank)      (((bank) * GPIO_BANK_WIDE) + 0x80)
+#define GPIO_REG_STAT(bank)      (((bank) * GPIO_BANK_WIDE) + 0x90)
+#define GPIO_REG_EDGE(bank)      (((bank) * GPIO_BANK_WIDE) + 0xA0)
 
 struct mtk_gc {
 	struct gpio_chip chip;
@@ -55,80 +54,21 @@ 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);
+	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;
-
-	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);
+	struct gpio_chip *gc = &rg->chip;
+	struct mtk_data *gpio_data = gpiochip_get_data(gc);
 
-	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;
+	return gc->read_reg(gpio_data->gpio_membase + offset);
 }
 
 static int
@@ -156,20 +96,22 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
 	memset(rg, 0, sizeof(*rg));
 
 	spin_lock_init(&rg->lock);
+	rg->bank = be32_to_cpu(*id);
+
+	ret = bgpio_init(&rg->chip, &pdev->dev, 4,
+			 gpio_data->gpio_membase + GPIO_REG_DATA(rg->bank),
+			 gpio_data->gpio_membase + GPIO_REG_DSET(rg->bank),
+			 gpio_data->gpio_membase + GPIO_REG_DCLR(rg->bank),
+			 gpio_data->gpio_membase + GPIO_REG_CTRL(rg->bank),
+			 gpio_data->gpio_membase + GPIO_REG_CTRL(rg->bank),
+			 0);
+	if (ret) {
+		dev_err(&pdev->dev, "bgpio_init() failed\n");
+		return ret;
+	}
 
-	rg->chip.parent = &pdev->dev;
-	rg->chip.label = dev_name(&pdev->dev);
-	rg->chip.of_node = bank;
-	rg->chip.base = MTK_BANK_WIDTH * be32_to_cpu(*id);
-	rg->chip.ngpio = MTK_BANK_WIDTH;
-	rg->chip.direction_input = mediatek_gpio_direction_input;
-	rg->chip.direction_output = mediatek_gpio_direction_output;
-	rg->chip.get_direction = mediatek_gpio_get_direction;
-	rg->chip.get = mediatek_gpio_get;
-	rg->chip.set = mediatek_gpio_set;
 	if (gpio_data->gpio_irq_domain)
 		rg->chip.to_irq = mediatek_gpio_to_irq;
-	rg->bank = be32_to_cpu(*id);
 
 	ret = devm_gpiochip_add_data(&pdev->dev, &rg->chip, gpio_data);
 	if (ret < 0) {
@@ -179,7 +121,7 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
 	}
 
 	/* set polarity to low for all gpios */
-	mtk_gpio_w32(rg, GPIO_REG_POL, 0);
+	mtk_gpio_w32(rg, GPIO_REG_POL(rg->bank), 0);
 
 	dev_info(&pdev->dev, "registering %d gpios\n", rg->chip.ngpio);
 
@@ -200,14 +142,14 @@ mediatek_gpio_irq_handler(struct irq_desc *desc)
 		if (!rg)
 			continue;
 
-		pending = mtk_gpio_r32(rg, GPIO_REG_STAT);
+		pending = mtk_gpio_r32(rg, GPIO_REG_STAT(rg->bank));
 
 		for_each_set_bit(bit, &pending, MTK_BANK_WIDTH) {
 			u32 map = irq_find_mapping(gpio_data->gpio_irq_domain,
 						   (MTK_BANK_WIDTH * i) + bit);
 
 			generic_handle_irq(map);
-			mtk_gpio_w32(rg, GPIO_REG_STAT, BIT(bit));
+			mtk_gpio_w32(rg, GPIO_REG_STAT(i), BIT(bit));
 		}
 	}
 }
@@ -226,10 +168,12 @@ mediatek_gpio_irq_unmask(struct irq_data *d)
 		return;
 
 	spin_lock_irqsave(&rg->lock, flags);
-	rise = mtk_gpio_r32(rg, GPIO_REG_REDGE);
-	fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE);
-	mtk_gpio_w32(rg, GPIO_REG_REDGE, rise | (PIN_MASK(pin) & rg->rising));
-	mtk_gpio_w32(rg, GPIO_REG_FEDGE, fall | (PIN_MASK(pin) & rg->falling));
+	rise = mtk_gpio_r32(rg, GPIO_REG_REDGE(bank));
+	fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE(bank));
+	mtk_gpio_w32(rg, GPIO_REG_REDGE(bank),
+		     rise | (PIN_MASK(pin) & rg->rising));
+	mtk_gpio_w32(rg, GPIO_REG_FEDGE(bank),
+		     fall | (PIN_MASK(pin) & rg->falling));
 	spin_unlock_irqrestore(&rg->lock, flags);
 }
 
@@ -247,10 +191,10 @@ mediatek_gpio_irq_mask(struct irq_data *d)
 		return;
 
 	spin_lock_irqsave(&rg->lock, flags);
-	rise = mtk_gpio_r32(rg, GPIO_REG_REDGE);
-	fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE);
-	mtk_gpio_w32(rg, GPIO_REG_FEDGE, fall & ~PIN_MASK(pin));
-	mtk_gpio_w32(rg, GPIO_REG_REDGE, rise & ~PIN_MASK(pin));
+	rise = mtk_gpio_r32(rg, GPIO_REG_REDGE(bank));
+	fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE(bank));
+	mtk_gpio_w32(rg, GPIO_REG_FEDGE(bank), fall & ~PIN_MASK(pin));
+	mtk_gpio_w32(rg, GPIO_REG_REDGE(bank), rise & ~PIN_MASK(pin));
 	spin_unlock_irqrestore(&rg->lock, flags);
 }
 
-- 
2.7.4

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

[PATCH 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 cc08505..433b1e9 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 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 433b1e9..654c9eb 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -310,4 +310,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 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 654c9eb..2a1c506 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;
@@ -229,12 +230,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
@@ -282,6 +313,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 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 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 2a1c506..80e618f 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(bank)      (((bank) * GPIO_BANK_WIDE) + 0x90)
 #define GPIO_REG_EDGE(bank)      (((bank) * GPIO_BANK_WIDE) + 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 7/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. You only have to
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'  and 'to_mediatek_gpio' are 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       |   2 +-
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 137 +++++++++++++-----------------
 2 files changed, 58 insertions(+), 81 deletions(-)

diff --git a/drivers/staging/mt7621-gpio/Kconfig b/drivers/staging/mt7621-gpio/Kconfig
index 835998a..b6a921a 100644
--- a/drivers/staging/mt7621-gpio/Kconfig
+++ b/drivers/staging/mt7621-gpio/Kconfig
@@ -2,6 +2,6 @@ config GPIO_MT7621
 	bool "Mediatek GPIO Support"
 	depends on SOC_MT7620 || SOC_MT7621 || COMPILE_TEST
 	select GPIO_GENERIC
-	select ARCH_REQUIRE_GPIOLIB
+	select GPIOLIB_IRQCHIP
 	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 80e618f..5d082c8 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>
@@ -54,23 +53,15 @@ 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];
 };
 
-static inline struct mtk_gc *
-to_mediatek_gpio(struct gpio_chip *chip)
-{
-	return container_of(chip, struct mtk_gc, chip);
-}
-
 static inline void
 mtk_gpio_w32(struct mtk_gc *rg, u32 offset, u32 val)
 {
@@ -89,63 +80,6 @@ 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_data = dev_get_drvdata(&pdev->dev);
-	const __be32 *id = of_get_property(bank, "reg", NULL);
-	struct mtk_gc *rg;
-	int ret;
-
-	if (!id || be32_to_cpu(*id) >= MTK_BANK_CNT)
-		return -EINVAL;
-
-	rg = &gpio_data->gc_map[be32_to_cpu(*id)];
-	memset(rg, 0, sizeof(*rg));
-
-	spin_lock_init(&rg->lock);
-	rg->bank = be32_to_cpu(*id);
-
-	ret = bgpio_init(&rg->chip, &pdev->dev, 4,
-			 gpio_data->gpio_membase + GPIO_REG_DATA(rg->bank),
-			 gpio_data->gpio_membase + GPIO_REG_DSET(rg->bank),
-			 gpio_data->gpio_membase + GPIO_REG_DCLR(rg->bank),
-			 gpio_data->gpio_membase + GPIO_REG_CTRL(rg->bank),
-			 gpio_data->gpio_membase + GPIO_REG_CTRL(rg->bank),
-			 0);
-	if (ret) {
-		dev_err(&pdev->dev, "bgpio_init() failed\n");
-		return ret;
-	}
-
-	if (gpio_data->gpio_irq_domain)
-		rg->chip.to_irq = mediatek_gpio_to_irq;
-
-	ret = devm_gpiochip_add_data(&pdev->dev, &rg->chip, gpio_data);
-	if (ret < 0) {
-		dev_err(&pdev->dev, "Could not register gpio %d, ret=%d\n",
-			rg->chip.ngpio, ret);
-		return ret;
-	}
-
-	/* set polarity to low for all gpios */
-	mtk_gpio_w32(rg, GPIO_REG_POL(rg->bank), 0);
-
-	dev_info(&pdev->dev, "registering %d gpios\n", rg->chip.ngpio);
-
-	return 0;
-}
-
 static void
 mediatek_gpio_irq_handler(struct irq_desc *desc)
 {
@@ -163,7 +97,7 @@ mediatek_gpio_irq_handler(struct irq_desc *desc)
 		pending = mtk_gpio_r32(rg, GPIO_REG_STAT(rg->bank));
 
 		for_each_set_bit(bit, &pending, MTK_BANK_WIDTH) {
-			u32 map = irq_find_mapping(gpio_data->gpio_irq_domain,
+			u32 map = irq_find_mapping(rg->chip.irq.domain,
 						   (MTK_BANK_WIDTH * i) + bit);
 
 			generic_handle_irq(map);
@@ -308,6 +242,62 @@ static const struct irq_domain_ops irq_domain_ops = {
 };
 
 static int
+mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
+{
+	struct mtk_data *gpio_data = dev_get_drvdata(&pdev->dev);
+	const __be32 *id = of_get_property(bank, "reg", NULL);
+	struct mtk_gc *rg;
+	int ret;
+
+	if (!id || be32_to_cpu(*id) >= MTK_BANK_CNT)
+		return -EINVAL;
+
+	rg = &gpio_data->gc_map[be32_to_cpu(*id)];
+	memset(rg, 0, sizeof(*rg));
+
+	spin_lock_init(&rg->lock);
+	rg->bank = be32_to_cpu(*id);
+
+	ret = bgpio_init(&rg->chip, &pdev->dev, 4,
+			 gpio_data->gpio_membase + GPIO_REG_DATA(rg->bank),
+			 gpio_data->gpio_membase + GPIO_REG_DSET(rg->bank),
+			 gpio_data->gpio_membase + GPIO_REG_DCLR(rg->bank),
+			 gpio_data->gpio_membase + GPIO_REG_CTRL(rg->bank),
+			 gpio_data->gpio_membase + GPIO_REG_CTRL(rg->bank),
+			 0);
+	if (ret) {
+		dev_err(&pdev->dev, "bgpio_init() failed\n");
+		return ret;
+	}
+
+	ret = devm_gpiochip_add_data(&pdev->dev, &rg->chip, gpio_data);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Could not register gpio %d, ret=%d\n",
+			rg->chip.ngpio, ret);
+		return ret;
+	}
+
+	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_data->gpio_irq,
+				     mediatek_gpio_irq_handler);
+
+	/* set polarity to low for all gpios */
+	mtk_gpio_w32(rg, GPIO_REG_POL(rg->bank), 0);
+
+	dev_info(&pdev->dev, "registering %d gpios\n", rg->chip.ngpio);
+
+	return 0;
+}
+
+
+static int
 mediatek_gpio_probe(struct platform_device *pdev)
 {
 	struct device_node *bank, *np = pdev->dev.of_node;
@@ -323,14 +313,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);
 
@@ -338,11 +320,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 8/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 | 57 +++++++++++++++++++++++++------
 1 file changed, 46 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index 5d082c8..b0cbb30 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -37,6 +37,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;
@@ -44,6 +46,8 @@ struct mtk_gc {
 	int bank;
 	u32 rising;
 	u32 falling;
+	u32 hlevel;
+	u32 llevel;
 };
 
 /**
@@ -114,7 +118,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;
@@ -122,10 +126,16 @@ mediatek_gpio_irq_unmask(struct irq_data *d)
 	spin_lock_irqsave(&rg->lock, flags);
 	rise = mtk_gpio_r32(rg, GPIO_REG_REDGE(bank));
 	fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE(bank));
+	high = mtk_gpio_r32(rg, GPIO_REG_HLVL(bank));
+	low = mtk_gpio_r32(rg, GPIO_REG_LLVL(bank));
 	mtk_gpio_w32(rg, GPIO_REG_REDGE(bank),
 		     rise | (PIN_MASK(pin) & rg->rising));
 	mtk_gpio_w32(rg, GPIO_REG_FEDGE(bank),
 		     fall | (PIN_MASK(pin) & rg->falling));
+	mtk_gpio_w32(rg, GPIO_REG_HLVL(bank),
+		     high | (PIN_MASK(pin) & rg->hlevel));
+	mtk_gpio_w32(rg, GPIO_REG_LLVL(bank),
+		     low | (PIN_MASK(pin) & rg->llevel));
 	spin_unlock_irqrestore(&rg->lock, flags);
 }
 
@@ -137,7 +147,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;
@@ -145,8 +155,12 @@ mediatek_gpio_irq_mask(struct irq_data *d)
 	spin_lock_irqsave(&rg->lock, flags);
 	rise = mtk_gpio_r32(rg, GPIO_REG_REDGE(bank));
 	fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE(bank));
+	high = mtk_gpio_r32(rg, GPIO_REG_HLVL(bank));
+	low = mtk_gpio_r32(rg, GPIO_REG_LLVL(bank));
 	mtk_gpio_w32(rg, GPIO_REG_FEDGE(bank), fall & ~PIN_MASK(pin));
 	mtk_gpio_w32(rg, GPIO_REG_REDGE(bank), rise & ~PIN_MASK(pin));
+	mtk_gpio_w32(rg, GPIO_REG_REDGE(bank), high & ~PIN_MASK(pin));
+	mtk_gpio_w32(rg, GPIO_REG_REDGE(bank), low & ~PIN_MASK(pin));
 	spin_unlock_irqrestore(&rg->lock, flags);
 }
 
@@ -163,21 +177,42 @@ 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;
+		type = (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING |
+			IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW);
 	}
 
-	if (type & IRQ_TYPE_EDGE_RISING)
+	switch (type) {
+	case IRQ_TYPE_EDGE_RISING:
 		rg->rising |= mask;
-	else
-		rg->rising &= ~mask;
-
-	if (type & IRQ_TYPE_EDGE_FALLING)
-		rg->falling |= mask;
-	else
 		rg->falling &= ~mask;
+		rg->hlevel &= ~mask;
+		rg->llevel &= ~mask;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		rg->falling |= mask;
+		rg->rising &= ~mask;
+		rg->hlevel &= ~mask;
+		rg->llevel &= ~mask;
+		break;
+	case IRQ_TYPE_LEVEL_HIGH:
+		rg->hlevel |= mask;
+		rg->rising &= ~mask;
+		rg->falling &= mask;
+		rg->llevel &= ~mask;
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+		rg->llevel |= mask;
+		rg->rising &= ~mask;
+		rg->falling &= mask;
+		rg->hlevel &= ~mask;
+		break;
+	default:
+		return -EINVAL;
+	}
 
 	return 0;
 }
-- 
2.7.4

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

Re: [PATCH 1/8] staging: mt7621-gpio: make use 'bgpio_init' from GPIO_GENERIC

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 10238 bytes --]

On Sat, Jun 09 2018, Sergio Paracuellos wrote:

> 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.

Thanks for this.  It is a big change, and unsurprisingly there are a few
bugs.
I haven't actually got the driver working yet so there is still
something not quite right after the things listed here get fixed.

Firstly, I don't think it is useful to create the macros that you
describe above.  Every time we access a register, we have an 'rg' which
has a bank number in it so I think it is much better to do the
multiplication in the read/write functions rather than in the macro.
But that isn't a bug exactly.... read on..


>
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> ---
>  drivers/staging/mt7621-gpio/Kconfig       |   1 +
>  drivers/staging/mt7621-gpio/gpio-mt7621.c | 148 ++++++++++--------------------
>  2 files changed, 47 insertions(+), 102 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..cc08505 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(bank)      (((bank) * GPIO_BANK_WIDE) + 0x00)
> +#define GPIO_REG_POL(bank)       (((bank) * GPIO_BANK_WIDE) + 0x10)
> +#define GPIO_REG_DATA(bank)      (((bank) * GPIO_BANK_WIDE) + 0x20)
> +#define GPIO_REG_DSET(bank)      (((bank) * GPIO_BANK_WIDE) + 0x30)
> +#define GPIO_REG_DCLR(bank)      (((bank) * GPIO_BANK_WIDE) + 0x40)
> +#define GPIO_REG_REDGE(bank)     (((bank) * GPIO_BANK_WIDE) + 0x50)
> +#define GPIO_REG_FEDGE(bank)     (((bank) * GPIO_BANK_WIDE) + 0x60)
> +#define GPIO_REG_HLVL(bank)      (((bank) * GPIO_BANK_WIDE) + 0x70)
> +#define GPIO_REG_LLVL(bank)      (((bank) * GPIO_BANK_WIDE) + 0x80)
> +#define GPIO_REG_STAT(bank)      (((bank) * GPIO_BANK_WIDE) + 0x90)
> +#define GPIO_REG_EDGE(bank)      (((bank) * GPIO_BANK_WIDE) + 0xA0)
>  
>  struct mtk_gc {
>  	struct gpio_chip chip;
> @@ -55,80 +54,21 @@ 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);
> +	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;
> -
> -	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);
> +	struct gpio_chip *gc = &rg->chip;
> +	struct mtk_data *gpio_data = gpiochip_get_data(gc);
>  
> -	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;
> +	return gc->read_reg(gpio_data->gpio_membase + offset);
>  }
>  
>  static int
> @@ -156,20 +96,22 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
>  	memset(rg, 0, sizeof(*rg));
>  
>  	spin_lock_init(&rg->lock);
> +	rg->bank = be32_to_cpu(*id);
> +
> +	ret = bgpio_init(&rg->chip, &pdev->dev, 4,
> +			 gpio_data->gpio_membase + GPIO_REG_DATA(rg->bank),
> +			 gpio_data->gpio_membase + GPIO_REG_DSET(rg->bank),
> +			 gpio_data->gpio_membase + GPIO_REG_DCLR(rg->bank),
> +			 gpio_data->gpio_membase + GPIO_REG_CTRL(rg->bank),
> +			 gpio_data->gpio_membase + GPIO_REG_CTRL(rg->bank),
> +			 0);

This is the first problem I git.  You've provided both 'dirout' and
'dirin' and that is not permitted.  You've given exactly the same value
for both, which isn't really meaningful.
dirout is correct, dirin should be NULL.

> +	if (ret) {
> +		dev_err(&pdev->dev, "bgpio_init() failed\n");
> +		return ret;
> +	}
>  
> -	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);

These last two are still needed - bgpio_init doesn't set them up.

I think that is all in this patch.

Thanks,
NeilBrown

> -	rg->chip.ngpio = MTK_BANK_WIDTH;
> -	rg->chip.direction_input = mediatek_gpio_direction_input;
> -	rg->chip.direction_output = mediatek_gpio_direction_output;
> -	rg->chip.get_direction = mediatek_gpio_get_direction;
> -	rg->chip.get = mediatek_gpio_get;
> -	rg->chip.set = mediatek_gpio_set;
>  	if (gpio_data->gpio_irq_domain)
>  		rg->chip.to_irq = mediatek_gpio_to_irq;
> -	rg->bank = be32_to_cpu(*id);
>  
>  	ret = devm_gpiochip_add_data(&pdev->dev, &rg->chip, gpio_data);
>  	if (ret < 0) {
> @@ -179,7 +121,7 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
>  	}
>  
>  	/* set polarity to low for all gpios */
> -	mtk_gpio_w32(rg, GPIO_REG_POL, 0);
> +	mtk_gpio_w32(rg, GPIO_REG_POL(rg->bank), 0);
>  
>  	dev_info(&pdev->dev, "registering %d gpios\n", rg->chip.ngpio);
>  
> @@ -200,14 +142,14 @@ mediatek_gpio_irq_handler(struct irq_desc *desc)
>  		if (!rg)
>  			continue;
>  
> -		pending = mtk_gpio_r32(rg, GPIO_REG_STAT);
> +		pending = mtk_gpio_r32(rg, GPIO_REG_STAT(rg->bank));
>  
>  		for_each_set_bit(bit, &pending, MTK_BANK_WIDTH) {
>  			u32 map = irq_find_mapping(gpio_data->gpio_irq_domain,
>  						   (MTK_BANK_WIDTH * i) + bit);
>  
>  			generic_handle_irq(map);
> -			mtk_gpio_w32(rg, GPIO_REG_STAT, BIT(bit));
> +			mtk_gpio_w32(rg, GPIO_REG_STAT(i), BIT(bit));
>  		}
>  	}
>  }
> @@ -226,10 +168,12 @@ mediatek_gpio_irq_unmask(struct irq_data *d)
>  		return;
>  
>  	spin_lock_irqsave(&rg->lock, flags);
> -	rise = mtk_gpio_r32(rg, GPIO_REG_REDGE);
> -	fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE);
> -	mtk_gpio_w32(rg, GPIO_REG_REDGE, rise | (PIN_MASK(pin) & rg->rising));
> -	mtk_gpio_w32(rg, GPIO_REG_FEDGE, fall | (PIN_MASK(pin) & rg->falling));
> +	rise = mtk_gpio_r32(rg, GPIO_REG_REDGE(bank));
> +	fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE(bank));
> +	mtk_gpio_w32(rg, GPIO_REG_REDGE(bank),
> +		     rise | (PIN_MASK(pin) & rg->rising));
> +	mtk_gpio_w32(rg, GPIO_REG_FEDGE(bank),
> +		     fall | (PIN_MASK(pin) & rg->falling));
>  	spin_unlock_irqrestore(&rg->lock, flags);
>  }
>  
> @@ -247,10 +191,10 @@ mediatek_gpio_irq_mask(struct irq_data *d)
>  		return;
>  
>  	spin_lock_irqsave(&rg->lock, flags);
> -	rise = mtk_gpio_r32(rg, GPIO_REG_REDGE);
> -	fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE);
> -	mtk_gpio_w32(rg, GPIO_REG_FEDGE, fall & ~PIN_MASK(pin));
> -	mtk_gpio_w32(rg, GPIO_REG_REDGE, rise & ~PIN_MASK(pin));
> +	rise = mtk_gpio_r32(rg, GPIO_REG_REDGE(bank));
> +	fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE(bank));
> +	mtk_gpio_w32(rg, GPIO_REG_FEDGE(bank), fall & ~PIN_MASK(pin));
> +	mtk_gpio_w32(rg, GPIO_REG_REDGE(bank), rise & ~PIN_MASK(pin));
>  	spin_unlock_irqrestore(&rg->lock, flags);
>  }
>  
> -- 
> 2.7.4

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

Re: [PATCH 4/8] staging: mt7621-gpio: implement '.irq_[request|release]_resources' functions

[NeilBrown]

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

On Sat, Jun 09 2018, Sergio Paracuellos wrote:

> 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 654c9eb..2a1c506 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;
> @@ -229,12 +230,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);

By the end of the patch set, irq_data_get_irq_chip_data returns a
'struct gpio_chip' here.
You can use container_of to get the 'rg', and you don't need 'bank' at
all.

The same applies in several places.
It might not be this patch that introduces the problem, but this was the
easiest place to point to it.

Thanks,
NeilBrown


> +	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
> @@ -282,6 +313,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

[-- 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 7/8] staging: mt7621-gpio: avoid custom irq_domain for gpio

[NeilBrown]

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

On Sat, Jun 09 2018, Sergio Paracuellos wrote:

> 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. You only have to
> 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'  and 'to_mediatek_gpio' are 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       |   2 +-
>  drivers/staging/mt7621-gpio/gpio-mt7621.c | 137 +++++++++++++-----------------
>  2 files changed, 58 insertions(+), 81 deletions(-)
>
> diff --git a/drivers/staging/mt7621-gpio/Kconfig b/drivers/staging/mt7621-gpio/Kconfig
> index 835998a..b6a921a 100644
> --- a/drivers/staging/mt7621-gpio/Kconfig
> +++ b/drivers/staging/mt7621-gpio/Kconfig
> @@ -2,6 +2,6 @@ config GPIO_MT7621
>  	bool "Mediatek GPIO Support"
>  	depends on SOC_MT7620 || SOC_MT7621 || COMPILE_TEST
>  	select GPIO_GENERIC
> -	select ARCH_REQUIRE_GPIOLIB
> +	select GPIOLIB_IRQCHIP
>  	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 80e618f..5d082c8 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>
> @@ -54,23 +53,15 @@ 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];
>  };
>  
> -static inline struct mtk_gc *
> -to_mediatek_gpio(struct gpio_chip *chip)
> -{
> -	return container_of(chip, struct mtk_gc, chip);
> -}
> -
>  static inline void
>  mtk_gpio_w32(struct mtk_gc *rg, u32 offset, u32 val)
>  {
> @@ -89,63 +80,6 @@ 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_data = dev_get_drvdata(&pdev->dev);
> -	const __be32 *id = of_get_property(bank, "reg", NULL);
> -	struct mtk_gc *rg;
> -	int ret;
> -
> -	if (!id || be32_to_cpu(*id) >= MTK_BANK_CNT)
> -		return -EINVAL;
> -
> -	rg = &gpio_data->gc_map[be32_to_cpu(*id)];
> -	memset(rg, 0, sizeof(*rg));
> -
> -	spin_lock_init(&rg->lock);
> -	rg->bank = be32_to_cpu(*id);
> -
> -	ret = bgpio_init(&rg->chip, &pdev->dev, 4,
> -			 gpio_data->gpio_membase + GPIO_REG_DATA(rg->bank),
> -			 gpio_data->gpio_membase + GPIO_REG_DSET(rg->bank),
> -			 gpio_data->gpio_membase + GPIO_REG_DCLR(rg->bank),
> -			 gpio_data->gpio_membase + GPIO_REG_CTRL(rg->bank),
> -			 gpio_data->gpio_membase + GPIO_REG_CTRL(rg->bank),
> -			 0);
> -	if (ret) {
> -		dev_err(&pdev->dev, "bgpio_init() failed\n");
> -		return ret;
> -	}
> -
> -	if (gpio_data->gpio_irq_domain)
> -		rg->chip.to_irq = mediatek_gpio_to_irq;
> -
> -	ret = devm_gpiochip_add_data(&pdev->dev, &rg->chip, gpio_data);
> -	if (ret < 0) {
> -		dev_err(&pdev->dev, "Could not register gpio %d, ret=%d\n",
> -			rg->chip.ngpio, ret);
> -		return ret;
> -	}
> -
> -	/* set polarity to low for all gpios */
> -	mtk_gpio_w32(rg, GPIO_REG_POL(rg->bank), 0);
> -
> -	dev_info(&pdev->dev, "registering %d gpios\n", rg->chip.ngpio);
> -
> -	return 0;
> -}
> -
>  static void
>  mediatek_gpio_irq_handler(struct irq_desc *desc)
>  {
> @@ -163,7 +97,7 @@ mediatek_gpio_irq_handler(struct irq_desc *desc)
>  		pending = mtk_gpio_r32(rg, GPIO_REG_STAT(rg->bank));
>  
>  		for_each_set_bit(bit, &pending, MTK_BANK_WIDTH) {
> -			u32 map = irq_find_mapping(gpio_data->gpio_irq_domain,
> +			u32 map = irq_find_mapping(rg->chip.irq.domain,
>  						   (MTK_BANK_WIDTH * i) + bit);

I think you need more changes to mediatek_gpio_irq_handler.
irq_desk_get_handler_data() now returns a 'struct gpio_chip' I think.
However this was the part that doesn't work for me yet.
Does this get called once for each bank when an interrupt occurs?
Maybe.
Anyway, something else is wrong here.

Once you've clean up the rest, I'll try again and see if I can work out
what is happening.

Thanks,
NeilBrown


>  
>  			generic_handle_irq(map);
> @@ -308,6 +242,62 @@ static const struct irq_domain_ops irq_domain_ops = {
>  };
>  
>  static int
> +mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
> +{
> +	struct mtk_data *gpio_data = dev_get_drvdata(&pdev->dev);
> +	const __be32 *id = of_get_property(bank, "reg", NULL);
> +	struct mtk_gc *rg;
> +	int ret;
> +
> +	if (!id || be32_to_cpu(*id) >= MTK_BANK_CNT)
> +		return -EINVAL;
> +
> +	rg = &gpio_data->gc_map[be32_to_cpu(*id)];
> +	memset(rg, 0, sizeof(*rg));
> +
> +	spin_lock_init(&rg->lock);
> +	rg->bank = be32_to_cpu(*id);
> +
> +	ret = bgpio_init(&rg->chip, &pdev->dev, 4,
> +			 gpio_data->gpio_membase + GPIO_REG_DATA(rg->bank),
> +			 gpio_data->gpio_membase + GPIO_REG_DSET(rg->bank),
> +			 gpio_data->gpio_membase + GPIO_REG_DCLR(rg->bank),
> +			 gpio_data->gpio_membase + GPIO_REG_CTRL(rg->bank),
> +			 gpio_data->gpio_membase + GPIO_REG_CTRL(rg->bank),
> +			 0);
> +	if (ret) {
> +		dev_err(&pdev->dev, "bgpio_init() failed\n");
> +		return ret;
> +	}
> +
> +	ret = devm_gpiochip_add_data(&pdev->dev, &rg->chip, gpio_data);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Could not register gpio %d, ret=%d\n",
> +			rg->chip.ngpio, ret);
> +		return ret;
> +	}
> +
> +	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_data->gpio_irq,
> +				     mediatek_gpio_irq_handler);
> +
> +	/* set polarity to low for all gpios */
> +	mtk_gpio_w32(rg, GPIO_REG_POL(rg->bank), 0);
> +
> +	dev_info(&pdev->dev, "registering %d gpios\n", rg->chip.ngpio);
> +
> +	return 0;
> +}
> +
> +
> +static int
>  mediatek_gpio_probe(struct platform_device *pdev)
>  {
>  	struct device_node *bank, *np = pdev->dev.of_node;
> @@ -323,14 +313,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);

This is the only place that irq_domain_ops was used - currently it is
unused.
If we really don't needed it, it should be removed.

> -		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);
>  
> @@ -338,11 +320,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

[-- 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 8/8] staging: mt7621-gpio: implement high level and low level irqs

[NeilBrown]

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

On Sat, Jun 09 2018, Sergio Paracuellos wrote:

> 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 | 57 +++++++++++++++++++++++++------
>  1 file changed, 46 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> index 5d082c8..b0cbb30 100644
> --- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
> +++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> @@ -37,6 +37,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;
> @@ -44,6 +46,8 @@ struct mtk_gc {
>  	int bank;
>  	u32 rising;
>  	u32 falling;
> +	u32 hlevel;
> +	u32 llevel;
>  };
>  
>  /**
> @@ -114,7 +118,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;
> @@ -122,10 +126,16 @@ mediatek_gpio_irq_unmask(struct irq_data *d)
>  	spin_lock_irqsave(&rg->lock, flags);
>  	rise = mtk_gpio_r32(rg, GPIO_REG_REDGE(bank));
>  	fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE(bank));
> +	high = mtk_gpio_r32(rg, GPIO_REG_HLVL(bank));
> +	low = mtk_gpio_r32(rg, GPIO_REG_LLVL(bank));
>  	mtk_gpio_w32(rg, GPIO_REG_REDGE(bank),
>  		     rise | (PIN_MASK(pin) & rg->rising));
>  	mtk_gpio_w32(rg, GPIO_REG_FEDGE(bank),
>  		     fall | (PIN_MASK(pin) & rg->falling));
> +	mtk_gpio_w32(rg, GPIO_REG_HLVL(bank),
> +		     high | (PIN_MASK(pin) & rg->hlevel));
> +	mtk_gpio_w32(rg, GPIO_REG_LLVL(bank),
> +		     low | (PIN_MASK(pin) & rg->llevel));
>  	spin_unlock_irqrestore(&rg->lock, flags);
>  }
>  
> @@ -137,7 +147,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;
> @@ -145,8 +155,12 @@ mediatek_gpio_irq_mask(struct irq_data *d)
>  	spin_lock_irqsave(&rg->lock, flags);
>  	rise = mtk_gpio_r32(rg, GPIO_REG_REDGE(bank));
>  	fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE(bank));
> +	high = mtk_gpio_r32(rg, GPIO_REG_HLVL(bank));
> +	low = mtk_gpio_r32(rg, GPIO_REG_LLVL(bank));
>  	mtk_gpio_w32(rg, GPIO_REG_FEDGE(bank), fall & ~PIN_MASK(pin));
>  	mtk_gpio_w32(rg, GPIO_REG_REDGE(bank), rise & ~PIN_MASK(pin));
> +	mtk_gpio_w32(rg, GPIO_REG_REDGE(bank), high & ~PIN_MASK(pin));
> +	mtk_gpio_w32(rg, GPIO_REG_REDGE(bank), low & ~PIN_MASK(pin));
>  	spin_unlock_irqrestore(&rg->lock, flags);
>  }
>  
> @@ -163,21 +177,42 @@ 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;
> +		type = (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING |
> +			IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW);
>  	}
>  
> -	if (type & IRQ_TYPE_EDGE_RISING)
> +	switch (type) {
> +	case IRQ_TYPE_EDGE_RISING:
>  		rg->rising |= mask;
> -	else
> -		rg->rising &= ~mask;
> -
> -	if (type & IRQ_TYPE_EDGE_FALLING)
> -		rg->falling |= mask;
> -	else
>  		rg->falling &= ~mask;
> +		rg->hlevel &= ~mask;
> +		rg->llevel &= ~mask;
> +		break;
> +	case IRQ_TYPE_EDGE_FALLING:
> +		rg->falling |= mask;
> +		rg->rising &= ~mask;
> +		rg->hlevel &= ~mask;
> +		rg->llevel &= ~mask;
> +		break;
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		rg->hlevel |= mask;
> +		rg->rising &= ~mask;
> +		rg->falling &= mask;
> +		rg->llevel &= ~mask;
> +		break;
> +	case IRQ_TYPE_LEVEL_LOW:
> +		rg->llevel |= mask;
> +		rg->rising &= ~mask;
> +		rg->falling &= mask;
> +		rg->hlevel &= ~mask;
> +		break;

Changing this to a switch statement is definitely the wrong thing to do.
Various combinations of bits are possible.
It only makes sense to have HIGH *or* LOW, but it makes lots of sense to
have both RISING and FALLING.
It probably doesn't make sense to have both a LEVEL and an EDGE, but I'm
not certain.

Thanks,
NeilBrown

> +	default:
> +		return -EINVAL;
> +	}
>  
>  	return 0;
>  }
> -- 
> 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 1/8] staging: mt7621-gpio: make use 'bgpio_init' from GPIO_GENERIC

[Sergio Paracuellos]

On Sun, Jun 10, 2018 at 06:53:04PM +1000, NeilBrown wrote:
> On Sat, Jun 09 2018, Sergio Paracuellos wrote:
> 
> > 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.
> 
> Thanks for this.  It is a big change, and unsurprisingly there are a few
> bugs.
> I haven't actually got the driver working yet so there is still
> something not quite right after the things listed here get fixed.

Thanks for testing and review. See my comments below.

> 
> Firstly, I don't think it is useful to create the macros that you
> describe above.  Every time we access a register, we have an 'rg' which
> has a bank number in it so I think it is much better to do the
> multiplication in the read/write functions rather than in the macro.
> But that isn't a bug exactly.... read on..

Linus prefers to pass offsets instead bank number to that kind
of read/write functions and create the macros make easier to
handle both: offsets and bgpio_init parameters. That is why this have
been introduced. There is some similar code in 'gpio-brcmstb.c' 
driver.

> 
> 
> >
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > ---
> >  drivers/staging/mt7621-gpio/Kconfig       |   1 +
> >  drivers/staging/mt7621-gpio/gpio-mt7621.c | 148 ++++++++++--------------------
> >  2 files changed, 47 insertions(+), 102 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..cc08505 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(bank)      (((bank) * GPIO_BANK_WIDE) + 0x00)
> > +#define GPIO_REG_POL(bank)       (((bank) * GPIO_BANK_WIDE) + 0x10)
> > +#define GPIO_REG_DATA(bank)      (((bank) * GPIO_BANK_WIDE) + 0x20)
> > +#define GPIO_REG_DSET(bank)      (((bank) * GPIO_BANK_WIDE) + 0x30)
> > +#define GPIO_REG_DCLR(bank)      (((bank) * GPIO_BANK_WIDE) + 0x40)
> > +#define GPIO_REG_REDGE(bank)     (((bank) * GPIO_BANK_WIDE) + 0x50)
> > +#define GPIO_REG_FEDGE(bank)     (((bank) * GPIO_BANK_WIDE) + 0x60)
> > +#define GPIO_REG_HLVL(bank)      (((bank) * GPIO_BANK_WIDE) + 0x70)
> > +#define GPIO_REG_LLVL(bank)      (((bank) * GPIO_BANK_WIDE) + 0x80)
> > +#define GPIO_REG_STAT(bank)      (((bank) * GPIO_BANK_WIDE) + 0x90)
> > +#define GPIO_REG_EDGE(bank)      (((bank) * GPIO_BANK_WIDE) + 0xA0)
> >  
> >  struct mtk_gc {
> >  	struct gpio_chip chip;
> > @@ -55,80 +54,21 @@ 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);
> > +	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;
> > -
> > -	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);
> > +	struct gpio_chip *gc = &rg->chip;
> > +	struct mtk_data *gpio_data = gpiochip_get_data(gc);
> >  
> > -	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;
> > +	return gc->read_reg(gpio_data->gpio_membase + offset);
> >  }
> >  
> >  static int
> > @@ -156,20 +96,22 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
> >  	memset(rg, 0, sizeof(*rg));
> >  
> >  	spin_lock_init(&rg->lock);
> > +	rg->bank = be32_to_cpu(*id);
> > +
> > +	ret = bgpio_init(&rg->chip, &pdev->dev, 4,
> > +			 gpio_data->gpio_membase + GPIO_REG_DATA(rg->bank),
> > +			 gpio_data->gpio_membase + GPIO_REG_DSET(rg->bank),
> > +			 gpio_data->gpio_membase + GPIO_REG_DCLR(rg->bank),
> > +			 gpio_data->gpio_membase + GPIO_REG_CTRL(rg->bank),
> > +			 gpio_data->gpio_membase + GPIO_REG_CTRL(rg->bank),
> > +			 0);
> 
> This is the first problem I git.  You've provided both 'dirout' and
> 'dirin' and that is not permitted.  You've given exactly the same value
> for both, which isn't really meaningful.
> dirout is correct, dirin should be NULL.

True, I misunderstood the code I was looking at. Thanks for pointing this out.

> 
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "bgpio_init() failed\n");
> > +		return ret;
> > +	}
> >  
> > -	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);
> 
> These last two are still needed - bgpio_init doesn't set them up.

Mmmm chip.base is set by bgpio_init to -1 which is the correct
value to set it as Linus said.

> 
> I think that is all in this patch.
> 
> Thanks,
> NeilBrown

Best regards,
    Sergio Paracuellos

> 
> > -	rg->chip.ngpio = MTK_BANK_WIDTH;
> > -	rg->chip.direction_input = mediatek_gpio_direction_input;
> > -	rg->chip.direction_output = mediatek_gpio_direction_output;
> > -	rg->chip.get_direction = mediatek_gpio_get_direction;
> > -	rg->chip.get = mediatek_gpio_get;
> > -	rg->chip.set = mediatek_gpio_set;
> >  	if (gpio_data->gpio_irq_domain)
> >  		rg->chip.to_irq = mediatek_gpio_to_irq;
> > -	rg->bank = be32_to_cpu(*id);
> >  
> >  	ret = devm_gpiochip_add_data(&pdev->dev, &rg->chip, gpio_data);
> >  	if (ret < 0) {
> > @@ -179,7 +121,7 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
> >  	}
> >  
> >  	/* set polarity to low for all gpios */
> > -	mtk_gpio_w32(rg, GPIO_REG_POL, 0);
> > +	mtk_gpio_w32(rg, GPIO_REG_POL(rg->bank), 0);
> >  
> >  	dev_info(&pdev->dev, "registering %d gpios\n", rg->chip.ngpio);
> >  
> > @@ -200,14 +142,14 @@ mediatek_gpio_irq_handler(struct irq_desc *desc)
> >  		if (!rg)
> >  			continue;
> >  
> > -		pending = mtk_gpio_r32(rg, GPIO_REG_STAT);
> > +		pending = mtk_gpio_r32(rg, GPIO_REG_STAT(rg->bank));
> >  
> >  		for_each_set_bit(bit, &pending, MTK_BANK_WIDTH) {
> >  			u32 map = irq_find_mapping(gpio_data->gpio_irq_domain,
> >  						   (MTK_BANK_WIDTH * i) + bit);
> >  
> >  			generic_handle_irq(map);
> > -			mtk_gpio_w32(rg, GPIO_REG_STAT, BIT(bit));
> > +			mtk_gpio_w32(rg, GPIO_REG_STAT(i), BIT(bit));
> >  		}
> >  	}
> >  }
> > @@ -226,10 +168,12 @@ mediatek_gpio_irq_unmask(struct irq_data *d)
> >  		return;
> >  
> >  	spin_lock_irqsave(&rg->lock, flags);
> > -	rise = mtk_gpio_r32(rg, GPIO_REG_REDGE);
> > -	fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE);
> > -	mtk_gpio_w32(rg, GPIO_REG_REDGE, rise | (PIN_MASK(pin) & rg->rising));
> > -	mtk_gpio_w32(rg, GPIO_REG_FEDGE, fall | (PIN_MASK(pin) & rg->falling));
> > +	rise = mtk_gpio_r32(rg, GPIO_REG_REDGE(bank));
> > +	fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE(bank));
> > +	mtk_gpio_w32(rg, GPIO_REG_REDGE(bank),
> > +		     rise | (PIN_MASK(pin) & rg->rising));
> > +	mtk_gpio_w32(rg, GPIO_REG_FEDGE(bank),
> > +		     fall | (PIN_MASK(pin) & rg->falling));
> >  	spin_unlock_irqrestore(&rg->lock, flags);
> >  }
> >  
> > @@ -247,10 +191,10 @@ mediatek_gpio_irq_mask(struct irq_data *d)
> >  		return;
> >  
> >  	spin_lock_irqsave(&rg->lock, flags);
> > -	rise = mtk_gpio_r32(rg, GPIO_REG_REDGE);
> > -	fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE);
> > -	mtk_gpio_w32(rg, GPIO_REG_FEDGE, fall & ~PIN_MASK(pin));
> > -	mtk_gpio_w32(rg, GPIO_REG_REDGE, rise & ~PIN_MASK(pin));
> > +	rise = mtk_gpio_r32(rg, GPIO_REG_REDGE(bank));
> > +	fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE(bank));
> > +	mtk_gpio_w32(rg, GPIO_REG_FEDGE(bank), fall & ~PIN_MASK(pin));
> > +	mtk_gpio_w32(rg, GPIO_REG_REDGE(bank), rise & ~PIN_MASK(pin));
> >  	spin_unlock_irqrestore(&rg->lock, flags);
> >  }
> >  
> > -- 
> > 2.7.4


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

Re: [PATCH 8/8] staging: mt7621-gpio: implement high level and low level irqs

[Sergio Paracuellos]

On Sun, Jun 10, 2018 at 07:04:56PM +1000, NeilBrown wrote:
> On Sat, Jun 09 2018, Sergio Paracuellos wrote:
> 
> > 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 | 57 +++++++++++++++++++++++++------
> >  1 file changed, 46 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> > index 5d082c8..b0cbb30 100644
> > --- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
> > +++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> > @@ -37,6 +37,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;
> > @@ -44,6 +46,8 @@ struct mtk_gc {
> >  	int bank;
> >  	u32 rising;
> >  	u32 falling;
> > +	u32 hlevel;
> > +	u32 llevel;
> >  };
> >  
> >  /**
> > @@ -114,7 +118,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;
> > @@ -122,10 +126,16 @@ mediatek_gpio_irq_unmask(struct irq_data *d)
> >  	spin_lock_irqsave(&rg->lock, flags);
> >  	rise = mtk_gpio_r32(rg, GPIO_REG_REDGE(bank));
> >  	fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE(bank));
> > +	high = mtk_gpio_r32(rg, GPIO_REG_HLVL(bank));
> > +	low = mtk_gpio_r32(rg, GPIO_REG_LLVL(bank));
> >  	mtk_gpio_w32(rg, GPIO_REG_REDGE(bank),
> >  		     rise | (PIN_MASK(pin) & rg->rising));
> >  	mtk_gpio_w32(rg, GPIO_REG_FEDGE(bank),
> >  		     fall | (PIN_MASK(pin) & rg->falling));
> > +	mtk_gpio_w32(rg, GPIO_REG_HLVL(bank),
> > +		     high | (PIN_MASK(pin) & rg->hlevel));
> > +	mtk_gpio_w32(rg, GPIO_REG_LLVL(bank),
> > +		     low | (PIN_MASK(pin) & rg->llevel));
> >  	spin_unlock_irqrestore(&rg->lock, flags);
> >  }
> >  
> > @@ -137,7 +147,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;
> > @@ -145,8 +155,12 @@ mediatek_gpio_irq_mask(struct irq_data *d)
> >  	spin_lock_irqsave(&rg->lock, flags);
> >  	rise = mtk_gpio_r32(rg, GPIO_REG_REDGE(bank));
> >  	fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE(bank));
> > +	high = mtk_gpio_r32(rg, GPIO_REG_HLVL(bank));
> > +	low = mtk_gpio_r32(rg, GPIO_REG_LLVL(bank));
> >  	mtk_gpio_w32(rg, GPIO_REG_FEDGE(bank), fall & ~PIN_MASK(pin));
> >  	mtk_gpio_w32(rg, GPIO_REG_REDGE(bank), rise & ~PIN_MASK(pin));
> > +	mtk_gpio_w32(rg, GPIO_REG_REDGE(bank), high & ~PIN_MASK(pin));
> > +	mtk_gpio_w32(rg, GPIO_REG_REDGE(bank), low & ~PIN_MASK(pin));
> >  	spin_unlock_irqrestore(&rg->lock, flags);
> >  }
> >  
> > @@ -163,21 +177,42 @@ 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;
> > +		type = (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING |
> > +			IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW);
> >  	}
> >  
> > -	if (type & IRQ_TYPE_EDGE_RISING)
> > +	switch (type) {
> > +	case IRQ_TYPE_EDGE_RISING:
> >  		rg->rising |= mask;
> > -	else
> > -		rg->rising &= ~mask;
> > -
> > -	if (type & IRQ_TYPE_EDGE_FALLING)
> > -		rg->falling |= mask;
> > -	else
> >  		rg->falling &= ~mask;
> > +		rg->hlevel &= ~mask;
> > +		rg->llevel &= ~mask;
> > +		break;
> > +	case IRQ_TYPE_EDGE_FALLING:
> > +		rg->falling |= mask;
> > +		rg->rising &= ~mask;
> > +		rg->hlevel &= ~mask;
> > +		rg->llevel &= ~mask;
> > +		break;
> > +	case IRQ_TYPE_LEVEL_HIGH:
> > +		rg->hlevel |= mask;
> > +		rg->rising &= ~mask;
> > +		rg->falling &= mask;
> > +		rg->llevel &= ~mask;
> > +		break;
> > +	case IRQ_TYPE_LEVEL_LOW:
> > +		rg->llevel |= mask;
> > +		rg->rising &= ~mask;
> > +		rg->falling &= mask;
> > +		rg->hlevel &= ~mask;
> > +		break;
> 
> Changing this to a switch statement is definitely the wrong thing to do.
> Various combinations of bits are possible.
> It only makes sense to have HIGH *or* LOW, but it makes lots of sense to
> have both RISING and FALLING.

Yes, you are right. I'll fix up this to allow both RISING and FALLING as
it was before.

> It probably doesn't make sense to have both a LEVEL and an EDGE, but I'm
> not certain.

The chip support both so it seems that it must be also implemented.

> 
> Thanks,
> NeilBrown

Best regards,
    Sergio Paracuellos

> 
> > +	default:
> > +		return -EINVAL;
> > +	}
> >  
> >  	return 0;
> >  }
> > -- 
> > 2.7.4


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

Re: [PATCH 4/8] staging: mt7621-gpio: implement '.irq_[request|release]_resources' functions

[Sergio Paracuellos]

On Sun, Jun 10, 2018 at 06:56:21PM +1000, NeilBrown wrote:
> On Sat, Jun 09 2018, Sergio Paracuellos wrote:
> 
> > 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 654c9eb..2a1c506 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;
> > @@ -229,12 +230,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);
> 
> By the end of the patch set, irq_data_get_irq_chip_data returns a
> 'struct gpio_chip' here.
> You can use container_of to get the 'rg', and you don't need 'bank' at
> all.

Function 'container_of' was a little confused and only
was used in the deleted 'gpio_to_irq' so I prefer to delete it also.
Is a wrong approach to use bank instead?

> 
> The same applies in several places.
> It might not be this patch that introduces the problem, but this was the
> easiest place to point to it.
> 
> Thanks,
> NeilBrown

Best regards,
    Sergio Paracuellos
> 
> 
> > +	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
> > @@ -282,6 +313,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

Re: [PATCH 7/8] staging: mt7621-gpio: avoid custom irq_domain for gpio

[Sergio Paracuellos]

On Sun, Jun 10, 2018 at 07:02:20PM +1000, NeilBrown wrote:
> On Sat, Jun 09 2018, Sergio Paracuellos wrote:
> 
> > 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. You only have to
> > 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'  and 'to_mediatek_gpio' are 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       |   2 +-
> >  drivers/staging/mt7621-gpio/gpio-mt7621.c | 137 +++++++++++++-----------------
> >  2 files changed, 58 insertions(+), 81 deletions(-)
> >
> > diff --git a/drivers/staging/mt7621-gpio/Kconfig b/drivers/staging/mt7621-gpio/Kconfig
> > index 835998a..b6a921a 100644
> > --- a/drivers/staging/mt7621-gpio/Kconfig
> > +++ b/drivers/staging/mt7621-gpio/Kconfig
> > @@ -2,6 +2,6 @@ config GPIO_MT7621
> >  	bool "Mediatek GPIO Support"
> >  	depends on SOC_MT7620 || SOC_MT7621 || COMPILE_TEST
> >  	select GPIO_GENERIC
> > -	select ARCH_REQUIRE_GPIOLIB
> > +	select GPIOLIB_IRQCHIP
> >  	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 80e618f..5d082c8 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>
> > @@ -54,23 +53,15 @@ 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];
> >  };
> >  
> > -static inline struct mtk_gc *
> > -to_mediatek_gpio(struct gpio_chip *chip)
> > -{
> > -	return container_of(chip, struct mtk_gc, chip);
> > -}
> > -
> >  static inline void
> >  mtk_gpio_w32(struct mtk_gc *rg, u32 offset, u32 val)
> >  {
> > @@ -89,63 +80,6 @@ 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_data = dev_get_drvdata(&pdev->dev);
> > -	const __be32 *id = of_get_property(bank, "reg", NULL);
> > -	struct mtk_gc *rg;
> > -	int ret;
> > -
> > -	if (!id || be32_to_cpu(*id) >= MTK_BANK_CNT)
> > -		return -EINVAL;
> > -
> > -	rg = &gpio_data->gc_map[be32_to_cpu(*id)];
> > -	memset(rg, 0, sizeof(*rg));
> > -
> > -	spin_lock_init(&rg->lock);
> > -	rg->bank = be32_to_cpu(*id);
> > -
> > -	ret = bgpio_init(&rg->chip, &pdev->dev, 4,
> > -			 gpio_data->gpio_membase + GPIO_REG_DATA(rg->bank),
> > -			 gpio_data->gpio_membase + GPIO_REG_DSET(rg->bank),
> > -			 gpio_data->gpio_membase + GPIO_REG_DCLR(rg->bank),
> > -			 gpio_data->gpio_membase + GPIO_REG_CTRL(rg->bank),
> > -			 gpio_data->gpio_membase + GPIO_REG_CTRL(rg->bank),
> > -			 0);
> > -	if (ret) {
> > -		dev_err(&pdev->dev, "bgpio_init() failed\n");
> > -		return ret;
> > -	}
> > -
> > -	if (gpio_data->gpio_irq_domain)
> > -		rg->chip.to_irq = mediatek_gpio_to_irq;
> > -
> > -	ret = devm_gpiochip_add_data(&pdev->dev, &rg->chip, gpio_data);
> > -	if (ret < 0) {
> > -		dev_err(&pdev->dev, "Could not register gpio %d, ret=%d\n",
> > -			rg->chip.ngpio, ret);
> > -		return ret;
> > -	}
> > -
> > -	/* set polarity to low for all gpios */
> > -	mtk_gpio_w32(rg, GPIO_REG_POL(rg->bank), 0);
> > -
> > -	dev_info(&pdev->dev, "registering %d gpios\n", rg->chip.ngpio);
> > -
> > -	return 0;
> > -}
> > -
> >  static void
> >  mediatek_gpio_irq_handler(struct irq_desc *desc)
> >  {
> > @@ -163,7 +97,7 @@ mediatek_gpio_irq_handler(struct irq_desc *desc)
> >  		pending = mtk_gpio_r32(rg, GPIO_REG_STAT(rg->bank));
> >  
> >  		for_each_set_bit(bit, &pending, MTK_BANK_WIDTH) {
> > -			u32 map = irq_find_mapping(gpio_data->gpio_irq_domain,
> > +			u32 map = irq_find_mapping(rg->chip.irq.domain,
> >  						   (MTK_BANK_WIDTH * i) + bit);
> 
> I think you need more changes to mediatek_gpio_irq_handler.
> irq_desk_get_handler_data() now returns a 'struct gpio_chip' I think.
> However this was the part that doesn't work for me yet.
> Does this get called once for each bank when an interrupt occurs?
> Maybe.
> Anyway, something else is wrong here.

Interrupt line must be marked as IRQF_SHARED to make this working
properly.

> 
> Once you've clean up the rest, I'll try again and see if I can work out
> what is happening.
> 
> Thanks,
> NeilBrown

Best regards,
    Sergio Paracuellos
> 
> 
> >  
> >  			generic_handle_irq(map);
> > @@ -308,6 +242,62 @@ static const struct irq_domain_ops irq_domain_ops = {
> >  };
> >  
> >  static int
> > +mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
> > +{
> > +	struct mtk_data *gpio_data = dev_get_drvdata(&pdev->dev);
> > +	const __be32 *id = of_get_property(bank, "reg", NULL);
> > +	struct mtk_gc *rg;
> > +	int ret;
> > +
> > +	if (!id || be32_to_cpu(*id) >= MTK_BANK_CNT)
> > +		return -EINVAL;
> > +
> > +	rg = &gpio_data->gc_map[be32_to_cpu(*id)];
> > +	memset(rg, 0, sizeof(*rg));
> > +
> > +	spin_lock_init(&rg->lock);
> > +	rg->bank = be32_to_cpu(*id);
> > +
> > +	ret = bgpio_init(&rg->chip, &pdev->dev, 4,
> > +			 gpio_data->gpio_membase + GPIO_REG_DATA(rg->bank),
> > +			 gpio_data->gpio_membase + GPIO_REG_DSET(rg->bank),
> > +			 gpio_data->gpio_membase + GPIO_REG_DCLR(rg->bank),
> > +			 gpio_data->gpio_membase + GPIO_REG_CTRL(rg->bank),
> > +			 gpio_data->gpio_membase + GPIO_REG_CTRL(rg->bank),
> > +			 0);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "bgpio_init() failed\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = devm_gpiochip_add_data(&pdev->dev, &rg->chip, gpio_data);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "Could not register gpio %d, ret=%d\n",
> > +			rg->chip.ngpio, ret);
> > +		return ret;
> > +	}
> > +
> > +	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_data->gpio_irq,
> > +				     mediatek_gpio_irq_handler);
> > +
> > +	/* set polarity to low for all gpios */
> > +	mtk_gpio_w32(rg, GPIO_REG_POL(rg->bank), 0);
> > +
> > +	dev_info(&pdev->dev, "registering %d gpios\n", rg->chip.ngpio);
> > +
> > +	return 0;
> > +}
> > +
> > +
> > +static int
> >  mediatek_gpio_probe(struct platform_device *pdev)
> >  {
> >  	struct device_node *bank, *np = pdev->dev.of_node;
> > @@ -323,14 +313,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);
> 
> This is the only place that irq_domain_ops was used - currently it is
> unused.
> If we really don't needed it, it should be removed.
> 
> > -		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);
> >  
> > @@ -338,11 +320,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