All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/7] staging: mt7621-gpio: cleanups to move this driver out of staging
@ 2018-05-25 16:54 Sergio Paracuellos
  2018-05-25 16:54 ` [PATCH 1/7] staging: mt7621-gpio: rename MTK_MAX_BANK into MTK_BANK_CNT Sergio Paracuellos
                   ` (7 more replies)
  0 siblings, 8 replies; 30+ messages in thread
From: Sergio Paracuellos @ 2018-05-25 16:54 UTC (permalink / raw)
  To: gregkh; +Cc: neil, driverdev-devel

This patch series review all the probably missing stuff
in order to get this driver out of staging..

All of these are described in the following mail by NeilBrown:

- https://www.mail-archive.com/driverdev-devel@linuxdriverproject.org/msg76202.html

I don't move the driver yet because I think is better to
review and test this before and if all is likely to be
alright just move all this stuff afterwards.

Hope this helps.

Best regards,
    Sergio Paracuellos

Sergio Paracuellos (7):
  staging: mt7621-gpio: rename MTK_MAX_BANK into MTK_BANK_CNT
  staging: mt7621-gpio: use module_platform_driver() instead subsys
    initcall
  staging: mt7621-gpio: fix masks for gpio pin
  staging: mt7621-gpio: avoid locking in mediatek_gpio_get_direction
  staging: mt7621-gpio: change lock place in irq mask and unmask
    functions
  staging: mt7621-dts: add missing properties to gpio node
  staging: mt7621-gpio: dt-bindings: complete documentation for the gpio

 drivers/staging/mt7621-dts/mt7621.dtsi             |  2 ++
 drivers/staging/mt7621-gpio/gpio-mt7621.c          | 42 ++++++++--------------
 .../staging/mt7621-gpio/mediatek,mt7621-gpio.txt   | 14 +++++---
 3 files changed, 26 insertions(+), 32 deletions(-)

-- 
2.7.4

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH 1/7] staging: mt7621-gpio: rename MTK_MAX_BANK into MTK_BANK_CNT
  2018-05-25 16:54 [PATCH 0/7] staging: mt7621-gpio: cleanups to move this driver out of staging Sergio Paracuellos
@ 2018-05-25 16:54 ` Sergio Paracuellos
  2018-05-25 16:54 ` [PATCH 2/7] staging: mt7621-gpio: use module_platform_driver() instead subsys initcall Sergio Paracuellos
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 30+ messages in thread
From: Sergio Paracuellos @ 2018-05-25 16:54 UTC (permalink / raw)
  To: gregkh; +Cc: neil, driverdev-devel

There are 3 banks of gpios numbered '0' and '1' and '2'. So
the maximum bank number is "2". "3" is the count of banks.
In order to make the code looks and be correct on checking
max allowed gpio's id it makes sense to change the name of
this definition. Also there is another definitions which
start with the same prefix MKK_BANK_ of the new name so
having those with the same prefix makes all preprocessor
structure to be the same. This improves readability.

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

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index 5d923a7..6416936 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -14,7 +14,7 @@
 #include <linux/interrupt.h>
 #include <linux/platform_device.h>
 
-#define MTK_MAX_BANK		3
+#define MTK_BANK_CNT		3
 #define MTK_BANK_WIDTH		32
 
 enum mediatek_gpio_reg {
@@ -43,7 +43,7 @@ struct mtk_data {
 	void __iomem *gpio_membase;
 	int gpio_irq;
 	struct irq_domain *gpio_irq_domain;
-	struct mtk_gc *gc_map[MTK_MAX_BANK];
+	struct mtk_gc *gc_map[MTK_BANK_CNT];
 };
 
 static inline struct mtk_gc
@@ -156,7 +156,7 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
 	struct mtk_gc *rg;
 	int ret;
 
-	if (!id || be32_to_cpu(*id) >= MTK_MAX_BANK)
+	if (!id || be32_to_cpu(*id) >= MTK_BANK_CNT)
 		return -EINVAL;
 
 	rg = devm_kzalloc(&pdev->dev, sizeof(struct mtk_gc), GFP_KERNEL);
@@ -202,7 +202,7 @@ mediatek_gpio_irq_handler(struct irq_desc *desc)
 	struct mtk_data *gpio_data = irq_desc_get_handler_data(desc);
 	int i;
 
-	for (i = 0; i < MTK_MAX_BANK; i++) {
+	for (i = 0; i < MTK_BANK_CNT; i++) {
 		struct mtk_gc *rg = gpio_data->gc_map[i];
 		unsigned long pending;
 		int bit;
@@ -345,7 +345,7 @@ mediatek_gpio_probe(struct platform_device *pdev)
 	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_MAX_BANK * MTK_BANK_WIDTH,
+			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");
-- 
2.7.4

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 2/7] staging: mt7621-gpio: use module_platform_driver() instead subsys initcall
  2018-05-25 16:54 [PATCH 0/7] staging: mt7621-gpio: cleanups to move this driver out of staging Sergio Paracuellos
  2018-05-25 16:54 ` [PATCH 1/7] staging: mt7621-gpio: rename MTK_MAX_BANK into MTK_BANK_CNT Sergio Paracuellos
@ 2018-05-25 16:54 ` Sergio Paracuellos
  2018-05-25 16:54 ` [PATCH 3/7] staging: mt7621-gpio: fix masks for gpio pin Sergio Paracuellos
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 30+ messages in thread
From: Sergio Paracuellos @ 2018-05-25 16:54 UTC (permalink / raw)
  To: gregkh; +Cc: neil, driverdev-devel

The driver's init function don't do anything besides registering the platform
driver, and the exit function which is not included in the driver should only
do driver unregister. Because of this module_platform_driver() macro could
just be used instead of having separate functions.

Currently the macro is not being used because the driver is initialized at
subsys init call level but this isn't necessary since platform devices are
defined in the DT as dependencies so there's no need for init calls order.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
Reviewed-by: NeilBrown <neil@brown.name>
---
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index 6416936..d41cc3e 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -379,10 +379,4 @@ static struct platform_driver mediatek_gpio_driver = {
 	},
 };
 
-static int __init
-mediatek_gpio_init(void)
-{
-	return platform_driver_register(&mediatek_gpio_driver);
-}
-
-subsys_initcall(mediatek_gpio_init);
+module_platform_driver(mediatek_gpio_driver);
-- 
2.7.4

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 3/7] staging: mt7621-gpio: fix masks for gpio pin
  2018-05-25 16:54 [PATCH 0/7] staging: mt7621-gpio: cleanups to move this driver out of staging Sergio Paracuellos
  2018-05-25 16:54 ` [PATCH 1/7] staging: mt7621-gpio: rename MTK_MAX_BANK into MTK_BANK_CNT Sergio Paracuellos
  2018-05-25 16:54 ` [PATCH 2/7] staging: mt7621-gpio: use module_platform_driver() instead subsys initcall Sergio Paracuellos
@ 2018-05-25 16:54 ` Sergio Paracuellos
  2018-05-25 16:54 ` [PATCH 4/7] staging: mt7621-gpio: avoid locking in mediatek_gpio_get_direction Sergio Paracuellos
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 30+ messages in thread
From: Sergio Paracuellos @ 2018-05-25 16:54 UTC (permalink / raw)
  To: gregkh; +Cc: neil, driverdev-devel

BIT macro is being used to get mask for gpio's pin
which is retrieved using 'hwirq' from struct irq_data.
The problem here is that 'hwirq' can be as large as 95,
and 1UL << 95 is unlikely to work well. Instead of using
BIT macro use a new PIN_MASK macro which takes into account
pin and WIDTH of the bank in order to make a proper mask for
the gpio pin. Also 'd->hwirq' has been replaced by 'pin' in
some places because there was a 'pin' variable in changed
functions with the proper value. This improves readability.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
Reviewed-by: NeilBrown <neil@brown.name>
---
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index d41cc3e..79452eb 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -16,6 +16,7 @@
 
 #define MTK_BANK_CNT		3
 #define MTK_BANK_WIDTH		32
+#define PIN_MASK(nr)		(1UL << ((nr % MTK_BANK_WIDTH)))
 
 enum mediatek_gpio_reg {
 	GPIO_REG_CTRL = 0,
@@ -239,8 +240,8 @@ mediatek_gpio_irq_unmask(struct irq_data *d)
 	fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE);
 
 	spin_lock_irqsave(&rg->lock, flags);
-	mtk_gpio_w32(rg, GPIO_REG_REDGE, rise | (BIT(d->hwirq) & rg->rising));
-	mtk_gpio_w32(rg, GPIO_REG_FEDGE, fall | (BIT(d->hwirq) & rg->falling));
+	mtk_gpio_w32(rg, GPIO_REG_REDGE, rise | (PIN_MASK(pin) & rg->rising));
+	mtk_gpio_w32(rg, GPIO_REG_FEDGE, fall | (PIN_MASK(pin) & rg->falling));
 	spin_unlock_irqrestore(&rg->lock, flags);
 }
 
@@ -261,8 +262,8 @@ mediatek_gpio_irq_mask(struct irq_data *d)
 	fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE);
 
 	spin_lock_irqsave(&rg->lock, flags);
-	mtk_gpio_w32(rg, GPIO_REG_FEDGE, fall & ~BIT(d->hwirq));
-	mtk_gpio_w32(rg, GPIO_REG_REDGE, rise & ~BIT(d->hwirq));
+	mtk_gpio_w32(rg, GPIO_REG_FEDGE, fall & ~PIN_MASK(pin));
+	mtk_gpio_w32(rg, GPIO_REG_REDGE, rise & ~PIN_MASK(pin));
 	spin_unlock_irqrestore(&rg->lock, flags);
 }
 
@@ -273,7 +274,7 @@ mediatek_gpio_irq_type(struct irq_data *d, unsigned int type)
 	int pin = d->hwirq;
 	int bank = pin / MTK_BANK_WIDTH;
 	struct mtk_gc *rg = gpio_data->gc_map[bank];
-	u32 mask = BIT(d->hwirq);
+	u32 mask = PIN_MASK(pin);
 
 	if (!rg)
 		return -1;
-- 
2.7.4

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 4/7] staging: mt7621-gpio: avoid locking in mediatek_gpio_get_direction
  2018-05-25 16:54 [PATCH 0/7] staging: mt7621-gpio: cleanups to move this driver out of staging Sergio Paracuellos
                   ` (2 preceding siblings ...)
  2018-05-25 16:54 ` [PATCH 3/7] staging: mt7621-gpio: fix masks for gpio pin Sergio Paracuellos
@ 2018-05-25 16:54 ` Sergio Paracuellos
  2018-05-25 16:54 ` [PATCH 5/7] staging: mt7621-gpio: change lock place in irq mask and unmask functions Sergio Paracuellos
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 30+ messages in thread
From: Sergio Paracuellos @ 2018-05-25 16:54 UTC (permalink / raw)
  To: gregkh; +Cc: neil, driverdev-devel

mediatek_gpio_get_direction function is holding across a simple read
which it seems to be not neccessary at all. Just remove this locking
cleaning code of this function a bit.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
Reviewed-by: NeilBrown <neil@brown.name>
---
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index 79452eb..143268a 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -129,12 +129,7 @@ static int
 mediatek_gpio_get_direction(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);
-	spin_unlock_irqrestore(&rg->lock, flags);
+	u32 t = mtk_gpio_r32(rg, GPIO_REG_CTRL);
 
 	return (t & BIT(offset)) ? 0 : 1;
 }
-- 
2.7.4

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 5/7] staging: mt7621-gpio: change lock place in irq mask and unmask functions
  2018-05-25 16:54 [PATCH 0/7] staging: mt7621-gpio: cleanups to move this driver out of staging Sergio Paracuellos
                   ` (3 preceding siblings ...)
  2018-05-25 16:54 ` [PATCH 4/7] staging: mt7621-gpio: avoid locking in mediatek_gpio_get_direction Sergio Paracuellos
@ 2018-05-25 16:54 ` Sergio Paracuellos
  2018-05-25 16:54 ` [PATCH 6/7] staging: mt7621-dts: add missing properties to gpio node Sergio Paracuellos
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 30+ messages in thread
From: Sergio Paracuellos @ 2018-05-25 16:54 UTC (permalink / raw)
  To: gregkh; +Cc: neil, driverdev-devel

Functions mediatek_gpio_irq_umask mediatek_gpio_irq_unmask are
reading and modifying registers but only the write is being hold.
It should be a complete lock instead for those which are type of
"read-modify-write". This makes more sense.

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

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index 143268a..c96ae67 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -231,10 +231,9 @@ mediatek_gpio_irq_unmask(struct irq_data *d)
 	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);
-
-	spin_lock_irqsave(&rg->lock, flags);
 	mtk_gpio_w32(rg, GPIO_REG_REDGE, rise | (PIN_MASK(pin) & rg->rising));
 	mtk_gpio_w32(rg, GPIO_REG_FEDGE, fall | (PIN_MASK(pin) & rg->falling));
 	spin_unlock_irqrestore(&rg->lock, flags);
@@ -253,10 +252,9 @@ mediatek_gpio_irq_mask(struct irq_data *d)
 	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);
-
-	spin_lock_irqsave(&rg->lock, flags);
 	mtk_gpio_w32(rg, GPIO_REG_FEDGE, fall & ~PIN_MASK(pin));
 	mtk_gpio_w32(rg, GPIO_REG_REDGE, rise & ~PIN_MASK(pin));
 	spin_unlock_irqrestore(&rg->lock, flags);
-- 
2.7.4

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 6/7] staging: mt7621-dts: add missing properties to gpio node
  2018-05-25 16:54 [PATCH 0/7] staging: mt7621-gpio: cleanups to move this driver out of staging Sergio Paracuellos
                   ` (4 preceding siblings ...)
  2018-05-25 16:54 ` [PATCH 5/7] staging: mt7621-gpio: change lock place in irq mask and unmask functions Sergio Paracuellos
@ 2018-05-25 16:54 ` Sergio Paracuellos
  2018-05-25 16:54 ` [PATCH 7/7] staging: mt7621-gpio: dt-bindings: complete documentation for the gpio Sergio Paracuellos
  2018-05-27 23:09 ` [PATCH 0/7] staging: mt7621-gpio: cleanups to move this driver out of staging NeilBrown
  7 siblings, 0 replies; 30+ messages in thread
From: Sergio Paracuellos @ 2018-05-25 16:54 UTC (permalink / raw)
  To: gregkh; +Cc: neil, driverdev-devel

In order to let other devices reference the GPIO interrupts
if necessary properties 'interrupt-controller' and
'#interrupt-cells' becomes necessary. Add both of them to
complete gpio device tree node.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
Reviewed-by: NeilBrown <neil@brown.name>
---
 drivers/staging/mt7621-dts/mt7621.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi b/drivers/staging/mt7621-dts/mt7621.dtsi
index 240d396..d7e4981 100644
--- a/drivers/staging/mt7621-dts/mt7621.dtsi
+++ b/drivers/staging/mt7621-dts/mt7621.dtsi
@@ -69,6 +69,8 @@
 
 			interrupt-parent = <&gic>;
 			interrupts = <GIC_SHARED 12 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-controller;
+			#interrupt-cells = <1>;
 
 			gpio0: bank@0 {
 				reg = <0>;
-- 
2.7.4

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 7/7] staging: mt7621-gpio: dt-bindings: complete documentation for the gpio
  2018-05-25 16:54 [PATCH 0/7] staging: mt7621-gpio: cleanups to move this driver out of staging Sergio Paracuellos
                   ` (5 preceding siblings ...)
  2018-05-25 16:54 ` [PATCH 6/7] staging: mt7621-dts: add missing properties to gpio node Sergio Paracuellos
@ 2018-05-25 16:54 ` Sergio Paracuellos
  2018-05-27 23:09 ` [PATCH 0/7] staging: mt7621-gpio: cleanups to move this driver out of staging NeilBrown
  7 siblings, 0 replies; 30+ messages in thread
From: Sergio Paracuellos @ 2018-05-25 16:54 UTC (permalink / raw)
  To: gregkh; +Cc: neil, driverdev-devel

This commit reviews and complete documentation for gpio related stuff
in the mt7621 device. It should be complete now.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
Reviewed-by: NeilBrown <neil@brown.name>
---
 drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt b/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
index af64092..94caba7 100644
--- a/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
+++ b/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
@@ -13,15 +13,17 @@ Required properties for the top level node:
   - "mediatek,mt7621-gpio" for Mediatek controllers
 - reg : Physical base address and length of the controller's registers
 - interrupt-parent : phandle of the parent interrupt controller.
-- interrupts = Interrupt specifier for the controllers interrupt
+- interrupts : Interrupt specifier for the controllers interrupt.
+- interrupt-controller : Mark the device node as an interrupt controller.
+- #interrupt-cells : Should be 1. The first cell is the GPIO number.
 
 Required properties for the GPIO bank node:
 - compatible:
   - "mediatek,mt7621-gpio-bank" for Mediatek banks
-- #gpio-cells : Should be two.
-  - first cell is the pin number
-  - second cell is used to specify optional parameters (unused)
-- gpio-controller : Marks the device node as a GPIO controller
+- #gpio-cells : Should be two. The first cell is the GPIO pin number and the
+   second cell specifies GPIO flags, as defined in <dt-bindings/gpio/gpio.h>.
+   Only the GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags are supported.
+- gpio-controller : Marks the device node as a GPIO controller.
 - reg : The id of the bank that the node describes.
 
 Example:
@@ -34,6 +36,8 @@ Example:
 
 		interrupt-parent = <&gic>;
 		interrupts = <GIC_SHARED 12 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-controller;
+		#interrupt-cells = <1>;
 
 		gpio0: bank@0 {
 			reg = <0>;
-- 
2.7.4

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: [PATCH 0/7] staging: mt7621-gpio: cleanups to move this driver out of staging
  2018-05-25 16:54 [PATCH 0/7] staging: mt7621-gpio: cleanups to move this driver out of staging Sergio Paracuellos
                   ` (6 preceding siblings ...)
  2018-05-25 16:54 ` [PATCH 7/7] staging: mt7621-gpio: dt-bindings: complete documentation for the gpio Sergio Paracuellos
@ 2018-05-27 23:09 ` NeilBrown
  2018-05-28  4:55   ` Sergio Paracuellos
  2018-05-29  4:58   ` [PATCH 0/6] staging: mt7621-gpio: last cleanups Sergio Paracuellos
  7 siblings, 2 replies; 30+ messages in thread
From: NeilBrown @ 2018-05-27 23:09 UTC (permalink / raw)
  To: Sergio Paracuellos, gregkh; +Cc: driverdev-devel


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

On Fri, May 25 2018, Sergio Paracuellos wrote:

> This patch series review all the probably missing stuff
> in order to get this driver out of staging..
>
> All of these are described in the following mail by NeilBrown:
>
> - https://www.mail-archive.com/driverdev-devel@linuxdriverproject.org/msg76202.html
>
> I don't move the driver yet because I think is better to
> review and test this before and if all is likely to be
> alright just move all this stuff afterwards.
>
> Hope this helps.

It certainly does - thanks.
All:
  Reviewed-by: NeilBrown <neil@brown.name>

except... I'm wondering about
   #interrupt-cells = <1>;

There really need to be 2 cells - one to identify the interrupt and one
to request a trigger (rising,falling,high,low).
I think I copied the <1> from a poor example.  Most gpio chips have
   #interrupt-cells = <2>;

Sorry about that.

Otherwise they look good - I compiled and tested and it gpios still work :-)

I went through the code again -- there is always something else.  These
a really trivial though:

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

I don't think there is any gain in having gc_map be pointers.  We may
as well just allocate all 3 at once.
so
-	struct mtk_gc *gc_map[MTK_BANK_CNT];
+	struct mtk_gc gc_map[MTK_BANK_CNT];

and several consequent changes in the code.

-------------
static inline struct mtk_gc
*to_mediatek_gpio(struct gpio_chip *chip)
{
	struct mtk_gc *mgc;

	mgc = container_of(chip, struct mtk_gc, chip);

	return mgc;
}

The '*' should be at the end of the first line, not the start of the
second.  Also the body of the function can

	return container_of(chip, struct mtk_gc, chip);

----------
	return (t & BIT(offset)) ? 0 : 1;

I think this would read better as

	return (t & BIT(offset)) ? GPIOF_DIR_OUT : GPIOF_DIR_IN;


Everything else looks perfect.
Thanks,
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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 0/7] staging: mt7621-gpio: cleanups to move this driver out of staging
  2018-05-27 23:09 ` [PATCH 0/7] staging: mt7621-gpio: cleanups to move this driver out of staging NeilBrown
@ 2018-05-28  4:55   ` Sergio Paracuellos
  2018-05-29  4:58   ` [PATCH 0/6] staging: mt7621-gpio: last cleanups Sergio Paracuellos
  1 sibling, 0 replies; 30+ messages in thread
From: Sergio Paracuellos @ 2018-05-28  4:55 UTC (permalink / raw)
  To: NeilBrown; +Cc: Greg KH, driverdev-devel

On Mon, May 28, 2018 at 1:09 AM, NeilBrown <neil@brown.name> wrote:
> On Fri, May 25 2018, Sergio Paracuellos wrote:
>
>> This patch series review all the probably missing stuff
>> in order to get this driver out of staging..
>>
>> All of these are described in the following mail by NeilBrown:
>>
>> - https://www.mail-archive.com/driverdev-devel@linuxdriverproject.org/msg76202.html
>>
>> I don't move the driver yet because I think is better to
>> review and test this before and if all is likely to be
>> alright just move all this stuff afterwards.
>>
>> Hope this helps.
>
> It certainly does - thanks.
> All:
>   Reviewed-by: NeilBrown <neil@brown.name>
>

Thanks, Neil.

> except... I'm wondering about
>    #interrupt-cells = <1>;
>
> There really need to be 2 cells - one to identify the interrupt and one
> to request a trigger (rising,falling,high,low).
> I think I copied the <1> from a poor example.  Most gpio chips have
>    #interrupt-cells = <2>;

I was thinking in this for a while looking through other drivers code but
finally I ended up with your suggestion :-).

>
> Sorry about that.
>
> Otherwise they look good - I compiled and tested and it gpios still work :-)

Good news for me, I haven't broken anything :-).

>
> I went through the code again -- there is always something else.  These
> a really trivial though:
>
> -------------
> struct mtk_data {
>         void __iomem *gpio_membase;
>         int gpio_irq;
>         struct irq_domain *gpio_irq_domain;
>         struct mtk_gc *gc_map[MTK_BANK_CNT];
> };
>
> I don't think there is any gain in having gc_map be pointers.  We may
> as well just allocate all 3 at once.
> so
> -       struct mtk_gc *gc_map[MTK_BANK_CNT];
> +       struct mtk_gc gc_map[MTK_BANK_CNT];
>
> and several consequent changes in the code.
>
> -------------
> static inline struct mtk_gc
> *to_mediatek_gpio(struct gpio_chip *chip)
> {
>         struct mtk_gc *mgc;
>
>         mgc = container_of(chip, struct mtk_gc, chip);
>
>         return mgc;
> }
>
> The '*' should be at the end of the first line, not the start of the
> second.  Also the body of the function can
>
>         return container_of(chip, struct mtk_gc, chip);
>
> ----------
>         return (t & BIT(offset)) ? 0 : 1;
>
> I think this would read better as
>
>         return (t & BIT(offset)) ? GPIOF_DIR_OUT : GPIOF_DIR_IN;
>
>
> Everything else looks perfect.
> Thanks,
> NeilBrown
>

Let's apply these patches first as they are if they are ok and I will send last
changes written here in a new series during this week. Hopefully
tonight if nothing happens.

Best regards,
    Sergio Paracuellos
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH 0/6] staging: mt7621-gpio: last cleanups
  2018-05-27 23:09 ` [PATCH 0/7] staging: mt7621-gpio: cleanups to move this driver out of staging NeilBrown
  2018-05-28  4:55   ` Sergio Paracuellos
@ 2018-05-29  4:58   ` Sergio Paracuellos
  2018-05-29  4:58     ` [PATCH 1/6] staging: mt7621-dts: fix property #interrupt-cells for the gpio node Sergio Paracuellos
                       ` (5 more replies)
  1 sibling, 6 replies; 30+ messages in thread
From: Sergio Paracuellos @ 2018-05-29  4:58 UTC (permalink / raw)
  To: gregkh; +Cc: neil, driverdev-devel

The following series performs last cleanups pointed out in this mail 
by NeilBrown:

http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-May/121282.html

If this is ok and after testing this driver should be ready to go
out of staging.

Please, note that this has to eb applied after previous series which are
before in this mail thread.

Hope this helps.

Best regards,
    Sergio Paracuellos

Sergio Paracuellos (6):
  staging: mt7621-dts: fix property #interrupt-cells for the gpio node
  staging: mt7621-gpio: dt-bindings: update documentation for
    #interrupt-cells property
  staging: mt7621-gpio: change 'to_mediatek_gpio' to make just a one
    line return
  staging: mt7621-gpio: use GPIOF_DIR_OUT and GPIOF_DIR_IN macros
    instead of custom values
  staging: mt7621-gpio: change gc_map to don't use pointers
  staging: mt7621-gpio: reorder includes alphabetically

 drivers/staging/mt7621-dts/mt7621.dtsi             |  2 +-
 drivers/staging/mt7621-gpio/gpio-mt7621.c          | 38 +++++++++-------------
 .../staging/mt7621-gpio/mediatek,mt7621-gpio.txt   | 10 ++++--
 3 files changed, 25 insertions(+), 25 deletions(-)

-- 
2.7.4

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH 1/6] staging: mt7621-dts: fix property #interrupt-cells for the gpio node
  2018-05-29  4:58   ` [PATCH 0/6] staging: mt7621-gpio: last cleanups Sergio Paracuellos
@ 2018-05-29  4:58     ` Sergio Paracuellos
  2018-05-29 23:34       ` NeilBrown
  2018-05-29  4:58     ` [PATCH 2/6] staging: mt7621-gpio: dt-bindings: update documentation for #interrupt-cells property Sergio Paracuellos
                       ` (4 subsequent siblings)
  5 siblings, 1 reply; 30+ messages in thread
From: Sergio Paracuellos @ 2018-05-29  4:58 UTC (permalink / raw)
  To: gregkh; +Cc: neil, driverdev-devel

Most gpio chips have two cells for interrupts and this should be also.
Set this property accordly fixing this up.

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

diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi b/drivers/staging/mt7621-dts/mt7621.dtsi
index d7e4981..bce6029 100644
--- a/drivers/staging/mt7621-dts/mt7621.dtsi
+++ b/drivers/staging/mt7621-dts/mt7621.dtsi
@@ -70,7 +70,7 @@
 			interrupt-parent = <&gic>;
 			interrupts = <GIC_SHARED 12 IRQ_TYPE_LEVEL_HIGH>;
 			interrupt-controller;
-			#interrupt-cells = <1>;
+			#interrupt-cells = <2>;
 
 			gpio0: bank@0 {
 				reg = <0>;
-- 
2.7.4

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 2/6] staging: mt7621-gpio: dt-bindings: update documentation for #interrupt-cells property
  2018-05-29  4:58   ` [PATCH 0/6] staging: mt7621-gpio: last cleanups Sergio Paracuellos
  2018-05-29  4:58     ` [PATCH 1/6] staging: mt7621-dts: fix property #interrupt-cells for the gpio node Sergio Paracuellos
@ 2018-05-29  4:58     ` Sergio Paracuellos
  2018-05-29  4:58     ` [PATCH 3/6] staging: mt7621-gpio: change 'to_mediatek_gpio' to make just a one line return Sergio Paracuellos
                       ` (3 subsequent siblings)
  5 siblings, 0 replies; 30+ messages in thread
From: Sergio Paracuellos @ 2018-05-29  4:58 UTC (permalink / raw)
  To: gregkh; +Cc: neil, driverdev-devel

This commit update documentation for #interrupt-cells property in
the gpio node which has been changed from '1' to '2'.

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

diff --git a/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt b/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
index 94caba7..30d8a02 100644
--- a/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
+++ b/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
@@ -15,7 +15,13 @@ Required properties for the top level node:
 - interrupt-parent : phandle of the parent interrupt controller.
 - interrupts : Interrupt specifier for the controllers interrupt.
 - interrupt-controller : Mark the device node as an interrupt controller.
-- #interrupt-cells : Should be 1. The first cell is the GPIO number.
+- #interrupt-cells : Should be 2. The first cell defines the interrupt number.
+   The second cell bits[3:0] is used to specify trigger type as follows:
+	- 1 = low-to-high edge triggered.
+	- 2 = high-to-low edge triggered.
+	- 4 = active high level-sensitive.
+	- 8 = active low level-sensitive.
+
 
 Required properties for the GPIO bank node:
 - compatible:
@@ -37,7 +43,7 @@ Example:
 		interrupt-parent = <&gic>;
 		interrupts = <GIC_SHARED 12 IRQ_TYPE_LEVEL_HIGH>;
 		interrupt-controller;
-		#interrupt-cells = <1>;
+		#interrupt-cells = <2>;
 
 		gpio0: bank@0 {
 			reg = <0>;
-- 
2.7.4

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 3/6] staging: mt7621-gpio: change 'to_mediatek_gpio' to make just a one line return
  2018-05-29  4:58   ` [PATCH 0/6] staging: mt7621-gpio: last cleanups Sergio Paracuellos
  2018-05-29  4:58     ` [PATCH 1/6] staging: mt7621-dts: fix property #interrupt-cells for the gpio node Sergio Paracuellos
  2018-05-29  4:58     ` [PATCH 2/6] staging: mt7621-gpio: dt-bindings: update documentation for #interrupt-cells property Sergio Paracuellos
@ 2018-05-29  4:58     ` Sergio Paracuellos
  2018-05-29  4:58     ` [PATCH 4/6] staging: mt7621-gpio: use GPIOF_DIR_OUT and GPIOF_DIR_IN macros instead of custom values Sergio Paracuellos
                       ` (2 subsequent siblings)
  5 siblings, 0 replies; 30+ messages in thread
From: Sergio Paracuellos @ 2018-05-29  4:58 UTC (permalink / raw)
  To: gregkh; +Cc: neil, driverdev-devel

Function to_mediatek_gpio can directly return without declaring
anything else in its body improving readability. Also change
pointer '*' declaration to be with return type in the upper line.

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

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index c96ae67..9132963 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -47,14 +47,10 @@ struct mtk_data {
 	struct mtk_gc *gc_map[MTK_BANK_CNT];
 };
 
-static inline struct mtk_gc
-*to_mediatek_gpio(struct gpio_chip *chip)
+static inline struct mtk_gc *
+to_mediatek_gpio(struct gpio_chip *chip)
 {
-	struct mtk_gc *mgc;
-
-	mgc = container_of(chip, struct mtk_gc, chip);
-
-	return mgc;
+	return container_of(chip, struct mtk_gc, chip);
 }
 
 static inline void
-- 
2.7.4

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 4/6] staging: mt7621-gpio: use GPIOF_DIR_OUT and GPIOF_DIR_IN macros instead of custom values
  2018-05-29  4:58   ` [PATCH 0/6] staging: mt7621-gpio: last cleanups Sergio Paracuellos
                       ` (2 preceding siblings ...)
  2018-05-29  4:58     ` [PATCH 3/6] staging: mt7621-gpio: change 'to_mediatek_gpio' to make just a one line return Sergio Paracuellos
@ 2018-05-29  4:58     ` Sergio Paracuellos
  2018-05-29  4:58     ` [PATCH 5/6] staging: mt7621-gpio: change gc_map to don't use pointers Sergio Paracuellos
  2018-05-29  4:58     ` [PATCH 6/6] staging: mt7621-gpio: reorder includes alphabetically Sergio Paracuellos
  5 siblings, 0 replies; 30+ messages in thread
From: Sergio Paracuellos @ 2018-05-29  4:58 UTC (permalink / raw)
  To: gregkh; +Cc: neil, driverdev-devel

There are macros in gpio kernel's headers to define direction
of a gpio. Use them instead of return custom '0' and '1' values.

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

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index 9132963..e31ed67 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -6,6 +6,7 @@
 
 #include <linux/io.h>
 #include <linux/err.h>
+#include <linux/gpio.h>
 #include <linux/gpio/driver.h>
 #include <linux/module.h>
 #include <linux/of_irq.h>
@@ -127,7 +128,7 @@ 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)) ? 0 : 1;
+	return (t & BIT(offset)) ? GPIOF_DIR_OUT : GPIOF_DIR_IN;
 }
 
 static int
-- 
2.7.4

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 5/6] staging: mt7621-gpio: change gc_map to don't use pointers
  2018-05-29  4:58   ` [PATCH 0/6] staging: mt7621-gpio: last cleanups Sergio Paracuellos
                       ` (3 preceding siblings ...)
  2018-05-29  4:58     ` [PATCH 4/6] staging: mt7621-gpio: use GPIOF_DIR_OUT and GPIOF_DIR_IN macros instead of custom values Sergio Paracuellos
@ 2018-05-29  4:58     ` Sergio Paracuellos
  2018-05-29  4:58     ` [PATCH 6/6] staging: mt7621-gpio: reorder includes alphabetically Sergio Paracuellos
  5 siblings, 0 replies; 30+ messages in thread
From: Sergio Paracuellos @ 2018-05-29  4:58 UTC (permalink / raw)
  To: gregkh; +Cc: neil, driverdev-devel

There is no special gain in using pointers for 'gc_map' inside
'mtk_data' structure. We know the number of banks which is fixed
to MTK_BANK_CNT and we can just statically allocate them without
using kernel allocators.

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

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index e31ed67..0ae6082 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -45,7 +45,7 @@ struct mtk_data {
 	void __iomem *gpio_membase;
 	int gpio_irq;
 	struct irq_domain *gpio_irq_domain;
-	struct mtk_gc *gc_map[MTK_BANK_CNT];
+	struct mtk_gc gc_map[MTK_BANK_CNT];
 };
 
 static inline struct mtk_gc *
@@ -152,11 +152,8 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
 	if (!id || be32_to_cpu(*id) >= MTK_BANK_CNT)
 		return -EINVAL;
 
-	rg = devm_kzalloc(&pdev->dev, sizeof(struct mtk_gc), GFP_KERNEL);
-	if (!rg)
-		return -ENOMEM;
-
-	gpio_data->gc_map[be32_to_cpu(*id)] = rg;
+	rg = &gpio_data->gc_map[be32_to_cpu(*id)];
+	memset(rg, 0, sizeof(*rg));
 
 	spin_lock_init(&rg->lock);
 
@@ -196,7 +193,7 @@ mediatek_gpio_irq_handler(struct irq_desc *desc)
 	int i;
 
 	for (i = 0; i < MTK_BANK_CNT; i++) {
-		struct mtk_gc *rg = gpio_data->gc_map[i];
+		struct mtk_gc *rg = &gpio_data->gc_map[i];
 		unsigned long pending;
 		int bit;
 
@@ -221,7 +218,7 @@ mediatek_gpio_irq_unmask(struct irq_data *d)
 	struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
 	int pin = d->hwirq;
 	int bank = pin / MTK_BANK_WIDTH;
-	struct mtk_gc *rg = gpio_data->gc_map[bank];
+	struct mtk_gc *rg = &gpio_data->gc_map[bank];
 	unsigned long flags;
 	u32 rise, fall;
 
@@ -242,7 +239,7 @@ mediatek_gpio_irq_mask(struct irq_data *d)
 	struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
 	int pin = d->hwirq;
 	int bank = pin / MTK_BANK_WIDTH;
-	struct mtk_gc *rg = gpio_data->gc_map[bank];
+	struct mtk_gc *rg = &gpio_data->gc_map[bank];
 	unsigned long flags;
 	u32 rise, fall;
 
@@ -263,7 +260,7 @@ mediatek_gpio_irq_type(struct irq_data *d, unsigned int type)
 	struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
 	int pin = d->hwirq;
 	int bank = pin / MTK_BANK_WIDTH;
-	struct mtk_gc *rg = gpio_data->gc_map[bank];
+	struct mtk_gc *rg = &gpio_data->gc_map[bank];
 	u32 mask = PIN_MASK(pin);
 
 	if (!rg)
-- 
2.7.4

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 6/6] staging: mt7621-gpio: reorder includes alphabetically
  2018-05-29  4:58   ` [PATCH 0/6] staging: mt7621-gpio: last cleanups Sergio Paracuellos
                       ` (4 preceding siblings ...)
  2018-05-29  4:58     ` [PATCH 5/6] staging: mt7621-gpio: change gc_map to don't use pointers Sergio Paracuellos
@ 2018-05-29  4:58     ` Sergio Paracuellos
  5 siblings, 0 replies; 30+ messages in thread
From: Sergio Paracuellos @ 2018-05-29  4:58 UTC (permalink / raw)
  To: gregkh; +Cc: neil, driverdev-devel

This commit reviews driver includes reordering them in
alphabetic order. This improves readability.

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

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index 0ae6082..7884b3c 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -4,16 +4,16 @@
  * Copyright (C) 2013 John Crispin <blogic@openwrt.org>
  */
 
-#include <linux/io.h>
 #include <linux/err.h>
 #include <linux/gpio.h>
 #include <linux/gpio/driver.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irqdomain.h>
 #include <linux/module.h>
 #include <linux/of_irq.h>
-#include <linux/spinlock.h>
-#include <linux/irqdomain.h>
-#include <linux/interrupt.h>
 #include <linux/platform_device.h>
+#include <linux/spinlock.h>
 
 #define MTK_BANK_CNT		3
 #define MTK_BANK_WIDTH		32
-- 
2.7.4

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/6] staging: mt7621-dts: fix property #interrupt-cells for the gpio node
  2018-05-29  4:58     ` [PATCH 1/6] staging: mt7621-dts: fix property #interrupt-cells for the gpio node Sergio Paracuellos
@ 2018-05-29 23:34       ` NeilBrown
  2018-05-30  4:50         ` [PATCH v2] staging: mt7621-gpio: update " Sergio Paracuellos
  2018-05-30  4:54         ` [PATCH 1/6] staging: mt7621-dts: fix property " Sergio Paracuellos
  0 siblings, 2 replies; 30+ messages in thread
From: NeilBrown @ 2018-05-29 23:34 UTC (permalink / raw)
  To: Sergio Paracuellos, gregkh; +Cc: driverdev-devel

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

On Tue, May 29 2018, Sergio Paracuellos wrote:

> Most gpio chips have two cells for interrupts and this should be also.
> Set this property accordly fixing this up.
>
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> ---
>  drivers/staging/mt7621-dts/mt7621.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi b/drivers/staging/mt7621-dts/mt7621.dtsi
> index d7e4981..bce6029 100644
> --- a/drivers/staging/mt7621-dts/mt7621.dtsi
> +++ b/drivers/staging/mt7621-dts/mt7621.dtsi
> @@ -70,7 +70,7 @@
>  			interrupt-parent = <&gic>;
>  			interrupts = <GIC_SHARED 12 IRQ_TYPE_LEVEL_HIGH>;
>  			interrupt-controller;
> -			#interrupt-cells = <1>;
> +			#interrupt-cells = <2>;

Thanks for this ongoing effort.

I thought I should test this change.  It didn't quite go as I expected.
My board has one GPIO wired to a push-button so it is normally
configured with

	gpio-keys {
		compatible = "gpio-keys";

		reset {
			label = "reset";
			gpios = <&gpio0 18 GPIO_ACTIVE_HIGH>;
          ...

(though in upstream it still uses the old gpio-keys-polled).
I removed the gpios line and replaced with

			interrupt-parent = <&gpio>;
			interrupts = <18 IRQ_TYPE_EDGE_FALLING>;

which should produce a key-press event whenever the button is pressed.
It didn't.

The reason is

       .xlate = irq_domain_xlate_onecell,

in irq_domain_ops in gpio-mt7621.c.
"onecell" obviously correlated with #interrupt-cells = <1>.
I changed it to
       .xlate = irq_domain_xlate_twocell,

and now it works as expected.  So we need to combine that change with
the change to #interrupt-cells.  I'm certain that we do really want 2
cells here, as it is possible to change the trigger type.

You might have noticed that I added
			interrupt-parent = <&gpio>;

even though there is no 'gpio:' tag in the devicetree.  I had to add
one.

-               gpio@600 {
+               gpio: gpio@600 {

so that I could refer to the gpio interrupts.
This feels a bit untidy.  The gpios are grouped into banks of 32:
 gpio0 gpio1 grio2
but the interrupts are just a single bank of 96 interrupts.
I don't know that this is a problem and I'm not advocating that we "fix"
it.  But it might be something that will be queried when we
submit to linux-gpio - I really don't know.

So if you could redo this patch to added the gpio: label and change
the xlate function, that would be excellent.
For all the rest:
  Reviewed-by: NeilBrown <neil@brown.name>

Thanks a lot,
NeilBrown

>  
>  			gpio0: bank@0 {
>  				reg = <0>;
> -- 
> 2.7.4

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v2] staging: mt7621-gpio: update #interrupt-cells for the gpio node
  2018-05-29 23:34       ` NeilBrown
@ 2018-05-30  4:50         ` Sergio Paracuellos
  2018-05-31  5:27           ` NeilBrown
  2018-05-30  4:54         ` [PATCH 1/6] staging: mt7621-dts: fix property " Sergio Paracuellos
  1 sibling, 1 reply; 30+ messages in thread
From: Sergio Paracuellos @ 2018-05-30  4:50 UTC (permalink / raw)
  To: gregkh; +Cc: neil, driverdev-devel

Most gpio chips have two cells for interrupts and this should be also.
Set this property in the device tree accordly fixing this up. In order
to make this working properly the xlate function for the irq_domain must
be updated to use the  'irq_domain_xlate_twocell' one in the driver.
One more minimal change is needed two refer gpio's interrupt-parent from
other nodes which is to add new 'gpio' label in the device tree.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
Changes in v2:
    - commit message has been changed with more proper one
    - new label to refer gpio from other nodes added to the DT
    - use 'irq_domain_xlate_twocell'

 drivers/staging/mt7621-dts/mt7621.dtsi    | 4 ++--
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi b/drivers/staging/mt7621-dts/mt7621.dtsi
index d7e4981..eb3966b 100644
--- a/drivers/staging/mt7621-dts/mt7621.dtsi
+++ b/drivers/staging/mt7621-dts/mt7621.dtsi
@@ -60,7 +60,7 @@
 			reg = <0x100 0x100>;
 		};
 
-		gpio@600 {
+		gpio: gpio@600 {
 			#address-cells = <1>;
 			#size-cells = <0>;
 
@@ -70,7 +70,7 @@
 			interrupt-parent = <&gic>;
 			interrupts = <GIC_SHARED 12 IRQ_TYPE_LEVEL_HIGH>;
 			interrupt-controller;
-			#interrupt-cells = <1>;
+			#interrupt-cells = <2>;
 
 			gpio0: bank@0 {
 				reg = <0>;
diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index c96ae67..79b8c58 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -317,7 +317,7 @@ mediatek_gpio_gpio_map(struct irq_domain *d, unsigned int irq,
 }
 
 static const struct irq_domain_ops irq_domain_ops = {
-	.xlate = irq_domain_xlate_onecell,
+	.xlate = irq_domain_xlate_twocell,
 	.map = mediatek_gpio_gpio_map,
 };
 
-- 
2.7.4

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/6] staging: mt7621-dts: fix property #interrupt-cells for the gpio node
  2018-05-29 23:34       ` NeilBrown
  2018-05-30  4:50         ` [PATCH v2] staging: mt7621-gpio: update " Sergio Paracuellos
@ 2018-05-30  4:54         ` Sergio Paracuellos
  1 sibling, 0 replies; 30+ messages in thread
From: Sergio Paracuellos @ 2018-05-30  4:54 UTC (permalink / raw)
  To: NeilBrown; +Cc: gregkh, driverdev-devel

On Wed, May 30, 2018 at 09:34:39AM +1000, NeilBrown wrote:
> On Tue, May 29 2018, Sergio Paracuellos wrote:
> 
> > Most gpio chips have two cells for interrupts and this should be also.
> > Set this property accordly fixing this up.
> >
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > ---
> >  drivers/staging/mt7621-dts/mt7621.dtsi | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi b/drivers/staging/mt7621-dts/mt7621.dtsi
> > index d7e4981..bce6029 100644
> > --- a/drivers/staging/mt7621-dts/mt7621.dtsi
> > +++ b/drivers/staging/mt7621-dts/mt7621.dtsi
> > @@ -70,7 +70,7 @@
> >  			interrupt-parent = <&gic>;
> >  			interrupts = <GIC_SHARED 12 IRQ_TYPE_LEVEL_HIGH>;
> >  			interrupt-controller;
> > -			#interrupt-cells = <1>;
> > +			#interrupt-cells = <2>;
> 
> Thanks for this ongoing effort.
> 
> I thought I should test this change.  It didn't quite go as I expected.
> My board has one GPIO wired to a push-button so it is normally
> configured with
> 
> 	gpio-keys {
> 		compatible = "gpio-keys";
> 
> 		reset {
> 			label = "reset";
> 			gpios = <&gpio0 18 GPIO_ACTIVE_HIGH>;
>           ...
> 
> (though in upstream it still uses the old gpio-keys-polled).
> I removed the gpios line and replaced with
> 
> 			interrupt-parent = <&gpio>;
> 			interrupts = <18 IRQ_TYPE_EDGE_FALLING>;
> 
> which should produce a key-press event whenever the button is pressed.
> It didn't.
> 
> The reason is
> 
>        .xlate = irq_domain_xlate_onecell,
> 
> in irq_domain_ops in gpio-mt7621.c.
> "onecell" obviously correlated with #interrupt-cells = <1>.
> I changed it to
>        .xlate = irq_domain_xlate_twocell,
> 
> and now it works as expected.  So we need to combine that change with
> the change to #interrupt-cells.  I'm certain that we do really want 2
> cells here, as it is possible to change the trigger type.
> 
> You might have noticed that I added
> 			interrupt-parent = <&gpio>;
> 
> even though there is no 'gpio:' tag in the devicetree.  I had to add
> one.
> 
> -               gpio@600 {
> +               gpio: gpio@600 {
> 
> so that I could refer to the gpio interrupts.
> This feels a bit untidy.  The gpios are grouped into banks of 32:
>  gpio0 gpio1 grio2
> but the interrupts are just a single bank of 96 interrupts.
> I don't know that this is a problem and I'm not advocating that we "fix"
> it.  But it might be something that will be queried when we
> submit to linux-gpio - I really don't know.
> 
> So if you could redo this patch to added the gpio: label and change
> the xlate function, that would be excellent.
> For all the rest:
>   Reviewed-by: NeilBrown <neil@brown.name>
> 
> Thanks a lot,
> NeilBrown

Thanks for your review and clear explanation, Neil. That really helps.

I have just send v2 version for this patch with the changes you are
pointing out here.

Hope this helps.

Best regards,
    Sergio Paracuellos

> 
> >  
> >  			gpio0: bank@0 {
> >  				reg = <0>;
> > -- 
> > 2.7.4


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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] staging: mt7621-gpio: update #interrupt-cells for the gpio node
  2018-05-30  4:50         ` [PATCH v2] staging: mt7621-gpio: update " Sergio Paracuellos
@ 2018-05-31  5:27           ` NeilBrown
  2018-05-31 12:20             ` Sergio Paracuellos
  0 siblings, 1 reply; 30+ messages in thread
From: NeilBrown @ 2018-05-31  5:27 UTC (permalink / raw)
  To: Sergio Paracuellos, gregkh; +Cc: driverdev-devel


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

On Wed, May 30 2018, Sergio Paracuellos wrote:

> Most gpio chips have two cells for interrupts and this should be also.
> Set this property in the device tree accordly fixing this up. In order
> to make this working properly the xlate function for the irq_domain must
> be updated to use the  'irq_domain_xlate_twocell' one in the driver.
> One more minimal change is needed two refer gpio's interrupt-parent from
> other nodes which is to add new 'gpio' label in the device tree.
>
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>

Reviewed-by: NeilBrown <neil@brown.name>

Thanks,
NeilBrown


> ---
> Changes in v2:
>     - commit message has been changed with more proper one
>     - new label to refer gpio from other nodes added to the DT
>     - use 'irq_domain_xlate_twocell'
>
>  drivers/staging/mt7621-dts/mt7621.dtsi    | 4 ++--
>  drivers/staging/mt7621-gpio/gpio-mt7621.c | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi b/drivers/staging/mt7621-dts/mt7621.dtsi
> index d7e4981..eb3966b 100644
> --- a/drivers/staging/mt7621-dts/mt7621.dtsi
> +++ b/drivers/staging/mt7621-dts/mt7621.dtsi
> @@ -60,7 +60,7 @@
>  			reg = <0x100 0x100>;
>  		};
>  
> -		gpio@600 {
> +		gpio: gpio@600 {
>  			#address-cells = <1>;
>  			#size-cells = <0>;
>  
> @@ -70,7 +70,7 @@
>  			interrupt-parent = <&gic>;
>  			interrupts = <GIC_SHARED 12 IRQ_TYPE_LEVEL_HIGH>;
>  			interrupt-controller;
> -			#interrupt-cells = <1>;
> +			#interrupt-cells = <2>;
>  
>  			gpio0: bank@0 {
>  				reg = <0>;
> diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> index c96ae67..79b8c58 100644
> --- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
> +++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> @@ -317,7 +317,7 @@ mediatek_gpio_gpio_map(struct irq_domain *d, unsigned int irq,
>  }
>  
>  static const struct irq_domain_ops irq_domain_ops = {
> -	.xlate = irq_domain_xlate_onecell,
> +	.xlate = irq_domain_xlate_twocell,
>  	.map = mediatek_gpio_gpio_map,
>  };
>  
> -- 
> 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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] staging: mt7621-gpio: update #interrupt-cells for the gpio node
  2018-05-31  5:27           ` NeilBrown
@ 2018-05-31 12:20             ` Sergio Paracuellos
  2018-06-01  9:18               ` Greg KH
  0 siblings, 1 reply; 30+ messages in thread
From: Sergio Paracuellos @ 2018-05-31 12:20 UTC (permalink / raw)
  To: NeilBrown; +Cc: gregkh, driverdev-devel

On Thu, May 31, 2018 at 03:27:46PM +1000, NeilBrown wrote:
> On Wed, May 30 2018, Sergio Paracuellos wrote:
> 
> > Most gpio chips have two cells for interrupts and this should be also.
> > Set this property in the device tree accordly fixing this up. In order
> > to make this working properly the xlate function for the irq_domain must
> > be updated to use the  'irq_domain_xlate_twocell' one in the driver.
> > One more minimal change is needed two refer gpio's interrupt-parent from
> > other nodes which is to add new 'gpio' label in the device tree.
> >
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> 
> Reviewed-by: NeilBrown <neil@brown.name>
> 
> Thanks,
> NeilBrown

Thank you very much for all of these series review, Neil.

Best regards,
    Sergio Paracuellos
> 
> 
> > ---
> > Changes in v2:
> >     - commit message has been changed with more proper one
> >     - new label to refer gpio from other nodes added to the DT
> >     - use 'irq_domain_xlate_twocell'
> >
> >  drivers/staging/mt7621-dts/mt7621.dtsi    | 4 ++--
> >  drivers/staging/mt7621-gpio/gpio-mt7621.c | 2 +-
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi b/drivers/staging/mt7621-dts/mt7621.dtsi
> > index d7e4981..eb3966b 100644
> > --- a/drivers/staging/mt7621-dts/mt7621.dtsi
> > +++ b/drivers/staging/mt7621-dts/mt7621.dtsi
> > @@ -60,7 +60,7 @@
> >  			reg = <0x100 0x100>;
> >  		};
> >  
> > -		gpio@600 {
> > +		gpio: gpio@600 {
> >  			#address-cells = <1>;
> >  			#size-cells = <0>;
> >  
> > @@ -70,7 +70,7 @@
> >  			interrupt-parent = <&gic>;
> >  			interrupts = <GIC_SHARED 12 IRQ_TYPE_LEVEL_HIGH>;
> >  			interrupt-controller;
> > -			#interrupt-cells = <1>;
> > +			#interrupt-cells = <2>;
> >  
> >  			gpio0: bank@0 {
> >  				reg = <0>;
> > diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> > index c96ae67..79b8c58 100644
> > --- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
> > +++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> > @@ -317,7 +317,7 @@ mediatek_gpio_gpio_map(struct irq_domain *d, unsigned int irq,
> >  }
> >  
> >  static const struct irq_domain_ops irq_domain_ops = {
> > -	.xlate = irq_domain_xlate_onecell,
> > +	.xlate = irq_domain_xlate_twocell,
> >  	.map = mediatek_gpio_gpio_map,
> >  };
> >  
> > -- 
> > 2.7.4

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] staging: mt7621-gpio: update #interrupt-cells for the gpio node
  2018-05-31 12:20             ` Sergio Paracuellos
@ 2018-06-01  9:18               ` Greg KH
  2018-06-01  9:30                 ` [RESEND PATCH 1/6] " Sergio Paracuellos
  2018-06-01  9:36                 ` [PATCH v2] staging: mt7621-gpio: update #interrupt-cells for the gpio node Sergio Paracuellos
  0 siblings, 2 replies; 30+ messages in thread
From: Greg KH @ 2018-06-01  9:18 UTC (permalink / raw)
  To: Sergio Paracuellos; +Cc: NeilBrown, driverdev-devel

On Thu, May 31, 2018 at 02:20:49PM +0200, Sergio Paracuellos wrote:
> On Thu, May 31, 2018 at 03:27:46PM +1000, NeilBrown wrote:
> > On Wed, May 30 2018, Sergio Paracuellos wrote:
> > 
> > > Most gpio chips have two cells for interrupts and this should be also.
> > > Set this property in the device tree accordly fixing this up. In order
> > > to make this working properly the xlate function for the irq_domain must
> > > be updated to use the  'irq_domain_xlate_twocell' one in the driver.
> > > One more minimal change is needed two refer gpio's interrupt-parent from
> > > other nodes which is to add new 'gpio' label in the device tree.
> > >
> > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > 
> > Reviewed-by: NeilBrown <neil@brown.name>
> > 
> > Thanks,
> > NeilBrown
> 
> Thank you very much for all of these series review, Neil.

I'm really confused about what patches in this series I should, and
should not apply.  Can you resend it in one series, with Neil's
reviewed-by added so I know what to apply?

thanks,

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [RESEND PATCH 1/6] staging: mt7621-gpio: update #interrupt-cells for the gpio node
  2018-06-01  9:18               ` Greg KH
@ 2018-06-01  9:30                 ` Sergio Paracuellos
  2018-06-01  9:30                   ` [RESEND PATCH 2/6] staging: mt7621-gpio: dt-bindings: update documentation for #interrupt-cells property Sergio Paracuellos
                                     ` (4 more replies)
  2018-06-01  9:36                 ` [PATCH v2] staging: mt7621-gpio: update #interrupt-cells for the gpio node Sergio Paracuellos
  1 sibling, 5 replies; 30+ messages in thread
From: Sergio Paracuellos @ 2018-06-01  9:30 UTC (permalink / raw)
  To: gregkh; +Cc: neil, driverdev-devel

Most gpio chips have two cells for interrupts and this should be also.
Set this property in the device tree accordly fixing this up. In order
to make this working properly the xlate function for the irq_domain must
be updated to use the  'irq_domain_xlate_twocell' one in the driver.
One more minimal change is needed two refer gpio's interrupt-parent from
other nodes which is to add new 'gpio' label in the device tree.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
Reviewed-by: NeilBrown <neil@brown.name>
---
 drivers/staging/mt7621-dts/mt7621.dtsi    | 4 ++--
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi b/drivers/staging/mt7621-dts/mt7621.dtsi
index d7e4981..eb3966b 100644
--- a/drivers/staging/mt7621-dts/mt7621.dtsi
+++ b/drivers/staging/mt7621-dts/mt7621.dtsi
@@ -60,7 +60,7 @@
 			reg = <0x100 0x100>;
 		};
 
-		gpio@600 {
+		gpio: gpio@600 {
 			#address-cells = <1>;
 			#size-cells = <0>;
 
@@ -70,7 +70,7 @@
 			interrupt-parent = <&gic>;
 			interrupts = <GIC_SHARED 12 IRQ_TYPE_LEVEL_HIGH>;
 			interrupt-controller;
-			#interrupt-cells = <1>;
+			#interrupt-cells = <2>;
 
 			gpio0: bank@0 {
 				reg = <0>;
diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index c96ae67..79b8c58 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -317,7 +317,7 @@ mediatek_gpio_gpio_map(struct irq_domain *d, unsigned int irq,
 }
 
 static const struct irq_domain_ops irq_domain_ops = {
-	.xlate = irq_domain_xlate_onecell,
+	.xlate = irq_domain_xlate_twocell,
 	.map = mediatek_gpio_gpio_map,
 };
 
-- 
2.7.4

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [RESEND PATCH 2/6] staging: mt7621-gpio: dt-bindings: update documentation for #interrupt-cells property
  2018-06-01  9:30                 ` [RESEND PATCH 1/6] " Sergio Paracuellos
@ 2018-06-01  9:30                   ` Sergio Paracuellos
  2018-06-01  9:30                   ` [RESEND PATCH 3/6] staging: mt7621-gpio: change 'to_mediatek_gpio' to make just a one line return Sergio Paracuellos
                                     ` (3 subsequent siblings)
  4 siblings, 0 replies; 30+ messages in thread
From: Sergio Paracuellos @ 2018-06-01  9:30 UTC (permalink / raw)
  To: gregkh; +Cc: neil, driverdev-devel

This commit update documentation for #interrupt-cells property in
the gpio node which has been changed from '1' to '2'.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
Reviewed-by: NeilBrown <neil@brown.name>
---
 drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt b/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
index 94caba7..30d8a02 100644
--- a/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
+++ b/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
@@ -15,7 +15,13 @@ Required properties for the top level node:
 - interrupt-parent : phandle of the parent interrupt controller.
 - interrupts : Interrupt specifier for the controllers interrupt.
 - interrupt-controller : Mark the device node as an interrupt controller.
-- #interrupt-cells : Should be 1. The first cell is the GPIO number.
+- #interrupt-cells : Should be 2. The first cell defines the interrupt number.
+   The second cell bits[3:0] is used to specify trigger type as follows:
+	- 1 = low-to-high edge triggered.
+	- 2 = high-to-low edge triggered.
+	- 4 = active high level-sensitive.
+	- 8 = active low level-sensitive.
+
 
 Required properties for the GPIO bank node:
 - compatible:
@@ -37,7 +43,7 @@ Example:
 		interrupt-parent = <&gic>;
 		interrupts = <GIC_SHARED 12 IRQ_TYPE_LEVEL_HIGH>;
 		interrupt-controller;
-		#interrupt-cells = <1>;
+		#interrupt-cells = <2>;
 
 		gpio0: bank@0 {
 			reg = <0>;
-- 
2.7.4

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [RESEND PATCH 3/6] staging: mt7621-gpio: change 'to_mediatek_gpio' to make just a one line return
  2018-06-01  9:30                 ` [RESEND PATCH 1/6] " Sergio Paracuellos
  2018-06-01  9:30                   ` [RESEND PATCH 2/6] staging: mt7621-gpio: dt-bindings: update documentation for #interrupt-cells property Sergio Paracuellos
@ 2018-06-01  9:30                   ` Sergio Paracuellos
  2018-06-01  9:30                   ` [RESEND PATCH 4/6] staging: mt7621-gpio: use GPIOF_DIR_OUT and GPIOF_DIR_IN macros instead of custom values Sergio Paracuellos
                                     ` (2 subsequent siblings)
  4 siblings, 0 replies; 30+ messages in thread
From: Sergio Paracuellos @ 2018-06-01  9:30 UTC (permalink / raw)
  To: gregkh; +Cc: neil, driverdev-devel

Function to_mediatek_gpio can directly return without declaring
anything else in its body improving readability. Also change
pointer '*' declaration to be with return type in the upper line.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
Reviewed-by: NeilBrown <neil@brown.name>
---
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index 79b8c58..a8adee3 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -47,14 +47,10 @@ struct mtk_data {
 	struct mtk_gc *gc_map[MTK_BANK_CNT];
 };
 
-static inline struct mtk_gc
-*to_mediatek_gpio(struct gpio_chip *chip)
+static inline struct mtk_gc *
+to_mediatek_gpio(struct gpio_chip *chip)
 {
-	struct mtk_gc *mgc;
-
-	mgc = container_of(chip, struct mtk_gc, chip);
-
-	return mgc;
+	return container_of(chip, struct mtk_gc, chip);
 }
 
 static inline void
-- 
2.7.4

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [RESEND PATCH 4/6] staging: mt7621-gpio: use GPIOF_DIR_OUT and GPIOF_DIR_IN macros instead of custom values
  2018-06-01  9:30                 ` [RESEND PATCH 1/6] " Sergio Paracuellos
  2018-06-01  9:30                   ` [RESEND PATCH 2/6] staging: mt7621-gpio: dt-bindings: update documentation for #interrupt-cells property Sergio Paracuellos
  2018-06-01  9:30                   ` [RESEND PATCH 3/6] staging: mt7621-gpio: change 'to_mediatek_gpio' to make just a one line return Sergio Paracuellos
@ 2018-06-01  9:30                   ` Sergio Paracuellos
  2018-06-01  9:30                   ` [RESEND PATCH 5/6] staging: mt7621-gpio: change gc_map to don't use pointers Sergio Paracuellos
  2018-06-01  9:30                   ` [RESEND PATCH 6/6] staging: mt7621-gpio: reorder includes alphabetically Sergio Paracuellos
  4 siblings, 0 replies; 30+ messages in thread
From: Sergio Paracuellos @ 2018-06-01  9:30 UTC (permalink / raw)
  To: gregkh; +Cc: neil, driverdev-devel

There are macros in gpio kernel's headers to define direction
of a gpio. Use them instead of return custom '0' and '1' values.

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

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index a8adee3..390cc56 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -6,6 +6,7 @@
 
 #include <linux/io.h>
 #include <linux/err.h>
+#include <linux/gpio.h>
 #include <linux/gpio/driver.h>
 #include <linux/module.h>
 #include <linux/of_irq.h>
@@ -127,7 +128,7 @@ 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)) ? 0 : 1;
+	return (t & BIT(offset)) ? GPIOF_DIR_OUT : GPIOF_DIR_IN;
 }
 
 static int
-- 
2.7.4

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [RESEND PATCH 5/6] staging: mt7621-gpio: change gc_map to don't use pointers
  2018-06-01  9:30                 ` [RESEND PATCH 1/6] " Sergio Paracuellos
                                     ` (2 preceding siblings ...)
  2018-06-01  9:30                   ` [RESEND PATCH 4/6] staging: mt7621-gpio: use GPIOF_DIR_OUT and GPIOF_DIR_IN macros instead of custom values Sergio Paracuellos
@ 2018-06-01  9:30                   ` Sergio Paracuellos
  2018-06-01  9:30                   ` [RESEND PATCH 6/6] staging: mt7621-gpio: reorder includes alphabetically Sergio Paracuellos
  4 siblings, 0 replies; 30+ messages in thread
From: Sergio Paracuellos @ 2018-06-01  9:30 UTC (permalink / raw)
  To: gregkh; +Cc: neil, driverdev-devel

There is no special gain in using pointers for 'gc_map' inside
'mtk_data' structure. We know the number of banks which is fixed
to MTK_BANK_CNT and we can just statically allocate them without
using kernel allocators.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
Reviewed-by: NeilBrown <neil@brown.name>
---
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index 390cc56..3192fc8 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -45,7 +45,7 @@ struct mtk_data {
 	void __iomem *gpio_membase;
 	int gpio_irq;
 	struct irq_domain *gpio_irq_domain;
-	struct mtk_gc *gc_map[MTK_BANK_CNT];
+	struct mtk_gc gc_map[MTK_BANK_CNT];
 };
 
 static inline struct mtk_gc *
@@ -152,11 +152,8 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
 	if (!id || be32_to_cpu(*id) >= MTK_BANK_CNT)
 		return -EINVAL;
 
-	rg = devm_kzalloc(&pdev->dev, sizeof(struct mtk_gc), GFP_KERNEL);
-	if (!rg)
-		return -ENOMEM;
-
-	gpio_data->gc_map[be32_to_cpu(*id)] = rg;
+	rg = &gpio_data->gc_map[be32_to_cpu(*id)];
+	memset(rg, 0, sizeof(*rg));
 
 	spin_lock_init(&rg->lock);
 
@@ -196,7 +193,7 @@ mediatek_gpio_irq_handler(struct irq_desc *desc)
 	int i;
 
 	for (i = 0; i < MTK_BANK_CNT; i++) {
-		struct mtk_gc *rg = gpio_data->gc_map[i];
+		struct mtk_gc *rg = &gpio_data->gc_map[i];
 		unsigned long pending;
 		int bit;
 
@@ -221,7 +218,7 @@ mediatek_gpio_irq_unmask(struct irq_data *d)
 	struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
 	int pin = d->hwirq;
 	int bank = pin / MTK_BANK_WIDTH;
-	struct mtk_gc *rg = gpio_data->gc_map[bank];
+	struct mtk_gc *rg = &gpio_data->gc_map[bank];
 	unsigned long flags;
 	u32 rise, fall;
 
@@ -242,7 +239,7 @@ mediatek_gpio_irq_mask(struct irq_data *d)
 	struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
 	int pin = d->hwirq;
 	int bank = pin / MTK_BANK_WIDTH;
-	struct mtk_gc *rg = gpio_data->gc_map[bank];
+	struct mtk_gc *rg = &gpio_data->gc_map[bank];
 	unsigned long flags;
 	u32 rise, fall;
 
@@ -263,7 +260,7 @@ mediatek_gpio_irq_type(struct irq_data *d, unsigned int type)
 	struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
 	int pin = d->hwirq;
 	int bank = pin / MTK_BANK_WIDTH;
-	struct mtk_gc *rg = gpio_data->gc_map[bank];
+	struct mtk_gc *rg = &gpio_data->gc_map[bank];
 	u32 mask = PIN_MASK(pin);
 
 	if (!rg)
-- 
2.7.4

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [RESEND PATCH 6/6] staging: mt7621-gpio: reorder includes alphabetically
  2018-06-01  9:30                 ` [RESEND PATCH 1/6] " Sergio Paracuellos
                                     ` (3 preceding siblings ...)
  2018-06-01  9:30                   ` [RESEND PATCH 5/6] staging: mt7621-gpio: change gc_map to don't use pointers Sergio Paracuellos
@ 2018-06-01  9:30                   ` Sergio Paracuellos
  4 siblings, 0 replies; 30+ messages in thread
From: Sergio Paracuellos @ 2018-06-01  9:30 UTC (permalink / raw)
  To: gregkh; +Cc: neil, driverdev-devel

This commit reviews driver includes reordering them in
alphabetic order. This improves readability.

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

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index 3192fc8..0c4fb4a 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -4,16 +4,16 @@
  * Copyright (C) 2013 John Crispin <blogic@openwrt.org>
  */
 
-#include <linux/io.h>
 #include <linux/err.h>
 #include <linux/gpio.h>
 #include <linux/gpio/driver.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irqdomain.h>
 #include <linux/module.h>
 #include <linux/of_irq.h>
-#include <linux/spinlock.h>
-#include <linux/irqdomain.h>
-#include <linux/interrupt.h>
 #include <linux/platform_device.h>
+#include <linux/spinlock.h>
 
 #define MTK_BANK_CNT		3
 #define MTK_BANK_WIDTH		32
-- 
2.7.4

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] staging: mt7621-gpio: update #interrupt-cells for the gpio node
  2018-06-01  9:18               ` Greg KH
  2018-06-01  9:30                 ` [RESEND PATCH 1/6] " Sergio Paracuellos
@ 2018-06-01  9:36                 ` Sergio Paracuellos
  1 sibling, 0 replies; 30+ messages in thread
From: Sergio Paracuellos @ 2018-06-01  9:36 UTC (permalink / raw)
  To: Greg KH; +Cc: NeilBrown, driverdev-devel

On Fri, Jun 01, 2018 at 11:18:07AM +0200, Greg KH wrote:
> On Thu, May 31, 2018 at 02:20:49PM +0200, Sergio Paracuellos wrote:
> > On Thu, May 31, 2018 at 03:27:46PM +1000, NeilBrown wrote:
> > > On Wed, May 30 2018, Sergio Paracuellos wrote:
> > > 
> > > > Most gpio chips have two cells for interrupts and this should be also.
> > > > Set this property in the device tree accordly fixing this up. In order
> > > > to make this working properly the xlate function for the irq_domain must
> > > > be updated to use the  'irq_domain_xlate_twocell' one in the driver.
> > > > One more minimal change is needed two refer gpio's interrupt-parent from
> > > > other nodes which is to add new 'gpio' label in the device tree.
> > > >
> > > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > > 
> > > Reviewed-by: NeilBrown <neil@brown.name>
> > > 
> > > Thanks,
> > > NeilBrown
> > 
> > Thank you very much for all of these series review, Neil.
> 
> I'm really confused about what patches in this series I should, and
> should not apply.  Can you resend it in one series, with Neil's
> reviewed-by added so I know what to apply?

For sure, Greg. I have just sent those.

> 
> thanks,
> 
> greg k-h

Best regards,
    Sergio Paracuellos
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 30+ messages in thread

end of thread, other threads:[~2018-06-01  9:36 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-25 16:54 [PATCH 0/7] staging: mt7621-gpio: cleanups to move this driver out of staging Sergio Paracuellos
2018-05-25 16:54 ` [PATCH 1/7] staging: mt7621-gpio: rename MTK_MAX_BANK into MTK_BANK_CNT Sergio Paracuellos
2018-05-25 16:54 ` [PATCH 2/7] staging: mt7621-gpio: use module_platform_driver() instead subsys initcall Sergio Paracuellos
2018-05-25 16:54 ` [PATCH 3/7] staging: mt7621-gpio: fix masks for gpio pin Sergio Paracuellos
2018-05-25 16:54 ` [PATCH 4/7] staging: mt7621-gpio: avoid locking in mediatek_gpio_get_direction Sergio Paracuellos
2018-05-25 16:54 ` [PATCH 5/7] staging: mt7621-gpio: change lock place in irq mask and unmask functions Sergio Paracuellos
2018-05-25 16:54 ` [PATCH 6/7] staging: mt7621-dts: add missing properties to gpio node Sergio Paracuellos
2018-05-25 16:54 ` [PATCH 7/7] staging: mt7621-gpio: dt-bindings: complete documentation for the gpio Sergio Paracuellos
2018-05-27 23:09 ` [PATCH 0/7] staging: mt7621-gpio: cleanups to move this driver out of staging NeilBrown
2018-05-28  4:55   ` Sergio Paracuellos
2018-05-29  4:58   ` [PATCH 0/6] staging: mt7621-gpio: last cleanups Sergio Paracuellos
2018-05-29  4:58     ` [PATCH 1/6] staging: mt7621-dts: fix property #interrupt-cells for the gpio node Sergio Paracuellos
2018-05-29 23:34       ` NeilBrown
2018-05-30  4:50         ` [PATCH v2] staging: mt7621-gpio: update " Sergio Paracuellos
2018-05-31  5:27           ` NeilBrown
2018-05-31 12:20             ` Sergio Paracuellos
2018-06-01  9:18               ` Greg KH
2018-06-01  9:30                 ` [RESEND PATCH 1/6] " Sergio Paracuellos
2018-06-01  9:30                   ` [RESEND PATCH 2/6] staging: mt7621-gpio: dt-bindings: update documentation for #interrupt-cells property Sergio Paracuellos
2018-06-01  9:30                   ` [RESEND PATCH 3/6] staging: mt7621-gpio: change 'to_mediatek_gpio' to make just a one line return Sergio Paracuellos
2018-06-01  9:30                   ` [RESEND PATCH 4/6] staging: mt7621-gpio: use GPIOF_DIR_OUT and GPIOF_DIR_IN macros instead of custom values Sergio Paracuellos
2018-06-01  9:30                   ` [RESEND PATCH 5/6] staging: mt7621-gpio: change gc_map to don't use pointers Sergio Paracuellos
2018-06-01  9:30                   ` [RESEND PATCH 6/6] staging: mt7621-gpio: reorder includes alphabetically Sergio Paracuellos
2018-06-01  9:36                 ` [PATCH v2] staging: mt7621-gpio: update #interrupt-cells for the gpio node Sergio Paracuellos
2018-05-30  4:54         ` [PATCH 1/6] staging: mt7621-dts: fix property " Sergio Paracuellos
2018-05-29  4:58     ` [PATCH 2/6] staging: mt7621-gpio: dt-bindings: update documentation for #interrupt-cells property Sergio Paracuellos
2018-05-29  4:58     ` [PATCH 3/6] staging: mt7621-gpio: change 'to_mediatek_gpio' to make just a one line return Sergio Paracuellos
2018-05-29  4:58     ` [PATCH 4/6] staging: mt7621-gpio: use GPIOF_DIR_OUT and GPIOF_DIR_IN macros instead of custom values Sergio Paracuellos
2018-05-29  4:58     ` [PATCH 5/6] staging: mt7621-gpio: change gc_map to don't use pointers Sergio Paracuellos
2018-05-29  4:58     ` [PATCH 6/6] staging: mt7621-gpio: reorder includes alphabetically Sergio Paracuellos

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.