[PATCH] staging: mt7621-gpio: avoid use of globals and use a drvdata allocated structure

[PATCH] staging: mt7621-gpio: avoid use of globals and use a drvdata allocated structure

[Sergio Paracuellos]

Gpio driver have a some globals which can be avoided just
using platform_data in a proper form. This commit adds a new
struct mtk_data which includes all of those globals setting them
using platform_set_drvdata and devm_gpiochip_add_data functions.
With this properly set we are able to retrieve driver data along
the code using kernel api's so globals are not needed anymore.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---

I have not tested this because I don't hardware to do it but I
have compile it just to be sure it compiles. This patch needs 
to be applied after the previous series where bindings have been 
changed to use existant 'mediatek' instead of custom 'mtk'.
After this changes (if nothing is broken and this is accepted) 
I think only make sure gpio interrupts works is remaining in
order to get this out of staging.
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 82 +++++++++++++++++++++----------
 1 file changed, 57 insertions(+), 25 deletions(-)

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index 7d17949..3362ed8 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -31,17 +31,20 @@ enum mediatek_gpio_reg {
 	GPIO_REG_EDGE,
 };
 
-static void __iomem *mediatek_gpio_membase;
-static int mediatek_gpio_irq;
-static struct irq_domain *mediatek_gpio_irq_domain;
-
-static struct mtk_gc {
+struct mtk_gc {
 	struct gpio_chip chip;
 	spinlock_t lock;
 	int bank;
 	u32 rising;
 	u32 falling;
-} *gc_map[MTK_MAX_BANK];
+};
+
+struct mtk_data {
+	void __iomem *gpio_membase;
+	int gpio_irq;
+	struct irq_domain *gpio_irq_domain;
+	struct mtk_gc *gc_map[MTK_MAX_BANK];
+};
 
 static inline struct mtk_gc
 *to_mediatek_gpio(struct gpio_chip *chip)
@@ -56,15 +59,19 @@ static inline struct mtk_gc
 static inline void
 mtk_gpio_w32(struct mtk_gc *rg, u8 reg, u32 val)
 {
-	iowrite32(val, mediatek_gpio_membase + (reg * 0x10) + (rg->bank * 0x4));
+	struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
+	u32 offset = (reg * 0x10) + (rg->bank * 0x4);
+
+	iowrite32(val, gpio_data->gpio_membase + offset);
 }
 
 static inline u32
 mtk_gpio_r32(struct mtk_gc *rg, u8 reg)
 {
+	struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
 	u32 offset = (reg * 0x10) + (rg->bank * 0x4);
 
-	return ioread32(mediatek_gpio_membase + offset);
+	return ioread32(gpio_data->gpio_membase + offset);
 }
 
 static void
@@ -137,23 +144,26 @@ mediatek_gpio_get_direction(struct gpio_chip *chip, unsigned int 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(mediatek_gpio_irq_domain,
+	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 = devm_kzalloc(&pdev->dev,
 				sizeof(struct mtk_gc), GFP_KERNEL);
+	int ret;
 
 	if (!rg || !id || be32_to_cpu(*id) > MTK_MAX_BANK)
 		return -ENOMEM;
 
-	gc_map[be32_to_cpu(*id)] = rg;
+	gpio_data->gc_map[be32_to_cpu(*id)] = rg;
 
 	memset(rg, 0, sizeof(struct mtk_gc));
 
@@ -169,13 +179,20 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
 	rg->chip.get_direction = mediatek_gpio_get_direction;
 	rg->chip.get = mediatek_gpio_get;
 	rg->chip.set = mediatek_gpio_set;
-	if (mediatek_gpio_irq_domain)
+	if (gpio_data->gpio_irq_domain)
 		rg->chip.to_irq = mediatek_gpio_to_irq;
 	rg->bank = be32_to_cpu(*id);
 
 	/* set polarity to low for all gpios */
 	mtk_gpio_w32(rg, GPIO_REG_POL, 0);
 
+	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;
+	}
+
 	dev_info(&pdev->dev, "registering %d gpios\n", rg->chip.ngpio);
 
 	return gpiochip_add(&rg->chip);
@@ -184,10 +201,12 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
 static void
 mediatek_gpio_irq_handler(struct irq_desc *desc)
 {
+	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
+	struct mtk_data *gpio_data = gpiochip_get_data(gc);
 	int i;
 
 	for (i = 0; i < MTK_MAX_BANK; i++) {
-		struct mtk_gc *rg = gc_map[i];
+		struct mtk_gc *rg = gpio_data->gc_map[i];
 		unsigned long pending;
 		int bit;
 
@@ -197,7 +216,7 @@ mediatek_gpio_irq_handler(struct irq_desc *desc)
 		pending = mtk_gpio_r32(rg, GPIO_REG_STAT);
 
 		for_each_set_bit(bit, &pending, MTK_BANK_WIDTH) {
-			u32 map = irq_find_mapping(mediatek_gpio_irq_domain,
+			u32 map = irq_find_mapping(gpio_data->gpio_irq_domain,
 						   (MTK_BANK_WIDTH * i) + bit);
 
 			generic_handle_irq(map);
@@ -209,9 +228,11 @@ mediatek_gpio_irq_handler(struct irq_desc *desc)
 static void
 mediatek_gpio_irq_unmask(struct irq_data *d)
 {
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct mtk_data *gpio_data = gpiochip_get_data(gc);
 	int pin = d->hwirq;
 	int bank = pin / 32;
-	struct mtk_gc *rg = gc_map[bank];
+	struct mtk_gc *rg = gpio_data->gc_map[bank];
 	unsigned long flags;
 	u32 rise, fall;
 
@@ -230,9 +251,11 @@ mediatek_gpio_irq_unmask(struct irq_data *d)
 static void
 mediatek_gpio_irq_mask(struct irq_data *d)
 {
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct mtk_data *gpio_data = gpiochip_get_data(gc);
 	int pin = d->hwirq;
 	int bank = pin / 32;
-	struct mtk_gc *rg = gc_map[bank];
+	struct mtk_gc *rg = gpio_data->gc_map[bank];
 	unsigned long flags;
 	u32 rise, fall;
 
@@ -251,9 +274,11 @@ mediatek_gpio_irq_mask(struct irq_data *d)
 static int
 mediatek_gpio_irq_type(struct irq_data *d, unsigned int type)
 {
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct mtk_data *gpio_data = gpiochip_get_data(gc);
 	int pin = d->hwirq;
 	int bank = pin / 32;
-	struct mtk_gc *rg = gc_map[bank];
+	struct mtk_gc *rg = gpio_data->gc_map[bank];
 	u32 mask = BIT(d->hwirq);
 
 	if (!rg)
@@ -308,26 +333,33 @@ mediatek_gpio_probe(struct platform_device *pdev)
 {
 	struct device_node *bank, *np = pdev->dev.of_node;
 	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	struct mtk_data *gpio_data;
 
-	mediatek_gpio_membase = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(mediatek_gpio_membase))
-		return PTR_ERR(mediatek_gpio_membase);
+	gpio_data = devm_kzalloc(&pdev->dev, sizeof(*gpio_data), GFP_KERNEL);
+	if (!gpio_data)
+		return -ENOMEM;
 
-	mediatek_gpio_irq = irq_of_parse_and_map(np, 0);
-	if (mediatek_gpio_irq) {
-		mediatek_gpio_irq_domain = irq_domain_add_linear(np,
+	gpio_data->gpio_membase = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(gpio_data->gpio_membase))
+		return PTR_ERR(gpio_data->gpio_membase);
+
+	gpio_data->gpio_irq = irq_of_parse_and_map(np, 0);
+	if (gpio_data->gpio_irq) {
+		gpio_data->gpio_irq_domain = irq_domain_add_linear(np,
 			MTK_MAX_BANK * MTK_BANK_WIDTH,
 			&irq_domain_ops, NULL);
-		if (!mediatek_gpio_irq_domain)
+		if (!gpio_data->gpio_irq_domain)
 			dev_err(&pdev->dev, "irq_domain_add_linear failed\n");
 	}
 
+	platform_set_drvdata(pdev, gpio_data);
+
 	for_each_child_of_node(np, bank)
 		if (of_device_is_compatible(bank, "mediatek,mt7621-gpio-bank"))
 			mediatek_gpio_bank_probe(pdev, bank);
 
-	if (mediatek_gpio_irq_domain)
-		irq_set_chained_handler(mediatek_gpio_irq,
+	if (gpio_data->gpio_irq_domain)
+		irq_set_chained_handler(gpio_data->gpio_irq,
 					mediatek_gpio_irq_handler);
 
 	return 0;
-- 
2.7.4

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

Re: [PATCH] staging: mt7621-gpio: avoid use of globals and use a drvdata allocated structure

[Dan Carpenter]

Not related to your patch...

On Tue, May 15, 2018 at 02:14:02PM +0200, Sergio Paracuellos wrote:
>  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 = devm_kzalloc(&pdev->dev,
>  				sizeof(struct mtk_gc), GFP_KERNEL);
> +	int ret;

>  
>  	if (!rg || !id || be32_to_cpu(*id) > MTK_MAX_BANK)
>  		return -ENOMEM;
>  
> -	gc_map[be32_to_cpu(*id)] = rg;
> +	gpio_data->gc_map[be32_to_cpu(*id)] = rg;
>  
>  	memset(rg, 0, sizeof(struct mtk_gc));
>  

I hate that devm_kzalloc() hidden in the allocations and now the
allocation is even further from the NULL check.  Allocations inside the
declaration block are likelier to be leaked.  We see that trend looking
at static checker output.  In this case we're using managed memory so
it's not an issue, but it still makes me itche.

If *id is MTK_MAX_BANK then we end up writing one element beyond the end
of the gc_map[] array.  Normally I would say just change > to >= but
we're not using the first element of the gc_map[] array so perhaps we
intended to subtract one?  I don't know.  Probably changing > to >= is
correct.

Also this the "memset(rg, 0, sizeof(struct mtk_gc));" line is not
required since we allocated zeroed memory.

In other words it maybe should be:

	struct mtk_gc *rg;
	int ret;

	if (!id || be32_to_cpu(*id) >= MTK_MAX_BANK)
		return -EINVAL;
	rg = devm_kzalloc(&pdev->dev, sizeof(struct mtk_gc), GFP_KERNEL);
	if (!rg)
		return -ENOMEM;

	gc_map[be32_to_cpu(*id)] = rg;


regards,
dan carpenter

Re: [PATCH] staging: mt7621-gpio: avoid use of globals and use a drvdata allocated structure

[Dan Carpenter]

On Tue, May 15, 2018 at 05:45:03PM +0300, Dan Carpenter wrote:
> Normally I would say just change > to >= but
> we're not using the first element of the gc_map[] array so perhaps we
> intended to subtract one?

Oh.  We do use the first element.  My bad.  I got confused between
"id" and "*id".  My bad.

regards,
dan carpenter


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

Re: [PATCH] staging: mt7621-gpio: avoid use of globals and use a drvdata allocated structure

[Sergio Paracuellos]

On Tue, May 15, 2018 at 4:45 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> Not related to your patch...
>
> On Tue, May 15, 2018 at 02:14:02PM +0200, Sergio Paracuellos wrote:
>>  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 = devm_kzalloc(&pdev->dev,
>>                               sizeof(struct mtk_gc), GFP_KERNEL);
>> +     int ret;
>
>>
>>       if (!rg || !id || be32_to_cpu(*id) > MTK_MAX_BANK)
>>               return -ENOMEM;
>>
>> -     gc_map[be32_to_cpu(*id)] = rg;
>> +     gpio_data->gc_map[be32_to_cpu(*id)] = rg;
>>
>>       memset(rg, 0, sizeof(struct mtk_gc));
>>
>
> I hate that devm_kzalloc() hidden in the allocations and now the
> allocation is even further from the NULL check.  Allocations inside the
> declaration block are likelier to be leaked.  We see that trend looking
> at static checker output.  In this case we're using managed memory so
> it's not an issue, but it still makes me itche.
>
> If *id is MTK_MAX_BANK then we end up writing one element beyond the end
> of the gc_map[] array.  Normally I would say just change > to >= but
> we're not using the first element of the gc_map[] array so perhaps we
> intended to subtract one?  I don't know.  Probably changing > to >= is
> correct.
>
> Also this the "memset(rg, 0, sizeof(struct mtk_gc));" line is not
> required since we allocated zeroed memory.

I'll clean this up a bit in next series.

>
> In other words it maybe should be:
>
>         struct mtk_gc *rg;
>         int ret;
>
>         if (!id || be32_to_cpu(*id) >= MTK_MAX_BANK)
>                 return -EINVAL;
>         rg = devm_kzalloc(&pdev->dev, sizeof(struct mtk_gc), GFP_KERNEL);
>         if (!rg)
>                 return -ENOMEM;
>
>         gc_map[be32_to_cpu(*id)] = rg;
>
>
> regards,
> dan carpenter

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

Re: [PATCH] staging: mt7621-gpio: avoid use of globals and use a drvdata allocated structure

[NeilBrown]

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

On Tue, May 15 2018, Sergio Paracuellos wrote:

> Gpio driver have a some globals which can be avoided just
> using platform_data in a proper form. This commit adds a new
> struct mtk_data which includes all of those globals setting them
> using platform_set_drvdata and devm_gpiochip_add_data functions.
> With this properly set we are able to retrieve driver data along
> the code using kernel api's so globals are not needed anymore.
>
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> ---
>
> I have not tested this because I don't hardware to do it but I
> have compile it just to be sure it compiles. This patch needs 

I've tested it.  Doesn't work :-(  Close though.
mtk_gpio_w32() needs the gpio_data, so it cannot be called before
devm_gpiochip_add_data() is called.
This:

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index be27cbdcbfa8..48141d979059 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -185,9 +185,6 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
 		rg->chip.to_irq = mediatek_gpio_to_irq;
 	rg->bank = be32_to_cpu(*id);
 
-	/* set polarity to low for all gpios */
-	mtk_gpio_w32(rg, GPIO_REG_POL, 0);
-
 	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",
@@ -195,6 +192,9 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
 		return ret;
 	}
 
+	/* set polarity to low for all gpios */
+	mtk_gpio_w32(rg, GPIO_REG_POL, 0);
+
 	dev_info(&pdev->dev, "registering %d gpios\n", rg->chip.ngpio);
 
 	return gpiochip_add(&rg->chip);


makes it work.  It probably won't work once I try interrupts as
mediatek_gpio_to_irq() also needs gpio_data, and that is currently
called early.

Also:

>  
>  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(mediatek_gpio_membase + offset);
> +	return ioread32(gpio_data->gpio_membase + offset);
>  }
>  

This hunk doesn't apply.
The existing code is:

static inline u32
mtk_gpio_r32(struct mtk_gc *rg, u8 reg)
{
       return ioread32(mediatek_gpio_membase + (reg * 0x10) + (rg->bank * 0x4));
}

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

Re: [PATCH] staging: mt7621-gpio: avoid use of globals and use a drvdata allocated structure

[Sergio Paracuellos]

On Wed, May 16, 2018 at 09:59:09AM +1000, NeilBrown wrote:
> On Tue, May 15 2018, Sergio Paracuellos wrote:
> 
> > Gpio driver have a some globals which can be avoided just
> > using platform_data in a proper form. This commit adds a new
> > struct mtk_data which includes all of those globals setting them
> > using platform_set_drvdata and devm_gpiochip_add_data functions.
> > With this properly set we are able to retrieve driver data along
> > the code using kernel api's so globals are not needed anymore.
> >
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > ---
> >
> > I have not tested this because I don't hardware to do it but I
> > have compile it just to be sure it compiles. This patch needs 
> 
> I've tested it.  Doesn't work :-(  Close though.
> mtk_gpio_w32() needs the gpio_data, so it cannot be called before
> devm_gpiochip_add_data() is called.
> This:
> 
> diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> index be27cbdcbfa8..48141d979059 100644
> --- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
> +++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> @@ -185,9 +185,6 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
>  		rg->chip.to_irq = mediatek_gpio_to_irq;
>  	rg->bank = be32_to_cpu(*id);
>  
> -	/* set polarity to low for all gpios */
> -	mtk_gpio_w32(rg, GPIO_REG_POL, 0);
> -
>  	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",
> @@ -195,6 +192,9 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
>  		return ret;
>  	}
>  
> +	/* set polarity to low for all gpios */
> +	mtk_gpio_w32(rg, GPIO_REG_POL, 0);
> +
>  	dev_info(&pdev->dev, "registering %d gpios\n", rg->chip.ngpio);
>  
>  	return gpiochip_add(&rg->chip);
> 
> 
> makes it work.  It probably won't work once I try interrupts as
> mediatek_gpio_to_irq() also needs gpio_data, and that is currently
> called early.

True. I'll change this and send v2.

> 
> Also:
> 
> >  
> >  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(mediatek_gpio_membase + offset);
> > +	return ioread32(gpio_data->gpio_membase + offset);
> >  }
> >  
> 
> This hunk doesn't apply.
> The existing code is:
> 
> static inline u32
> mtk_gpio_r32(struct mtk_gc *rg, u8 reg)
> {
>        return ioread32(mediatek_gpio_membase + (reg * 0x10) + (rg->bank * 0x4));
> }

Mmmm... This is rebased on the top of staging-next and the existing code there
is not the one you are pointing out here. The only different code between
this patch and the existing code is the use of 'mediatek' instead of 'mtk' for
the bindings which are in my previous patch series which haven't be applied
yet. Should I join all of them with this stuff fixed and send them all again for
your better review?

Best regards,
    Sergio Paracuellos
> 
> Thanks!
> 
> NeilBrown


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

Re: [PATCH] staging: mt7621-gpio: avoid use of globals and use a drvdata allocated structure

[Sergio Paracuellos]

On Wed, May 16, 2018 at 09:59:09AM +1000, NeilBrown wrote:
> On Tue, May 15 2018, Sergio Paracuellos wrote:
> 
> > Gpio driver have a some globals which can be avoided just
> > using platform_data in a proper form. This commit adds a new
> > struct mtk_data which includes all of those globals setting them
> > using platform_set_drvdata and devm_gpiochip_add_data functions.
> > With this properly set we are able to retrieve driver data along
> > the code using kernel api's so globals are not needed anymore.
> >
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > ---
> >
> > I have not tested this because I don't hardware to do it but I
> > have compile it just to be sure it compiles. This patch needs 
> 
> I've tested it.  Doesn't work :-(  Close though.
> mtk_gpio_w32() needs the gpio_data, so it cannot be called before
> devm_gpiochip_add_data() is called.
> This:
> 
> diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> index be27cbdcbfa8..48141d979059 100644
> --- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
> +++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> @@ -185,9 +185,6 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
>  		rg->chip.to_irq = mediatek_gpio_to_irq;
>  	rg->bank = be32_to_cpu(*id);
>  
> -	/* set polarity to low for all gpios */
> -	mtk_gpio_w32(rg, GPIO_REG_POL, 0);
> -
>  	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",
> @@ -195,6 +192,9 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
>  		return ret;
>  	}
>  
> +	/* set polarity to low for all gpios */
> +	mtk_gpio_w32(rg, GPIO_REG_POL, 0);
> +
>  	dev_info(&pdev->dev, "registering %d gpios\n", rg->chip.ngpio);
>  
>  	return gpiochip_add(&rg->chip);
> 
> 
> makes it work.  It probably won't work once I try interrupts as
> mediatek_gpio_to_irq() also needs gpio_data, and that is currently
> called early.
> 
> Also:
> 
> >  
> >  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(mediatek_gpio_membase + offset);
> > +	return ioread32(gpio_data->gpio_membase + offset);
> >  }
> >  
> 
> This hunk doesn't apply.
> The existing code is:
> 
> static inline u32
> mtk_gpio_r32(struct mtk_gc *rg, u8 reg)
> {
>        return ioread32(mediatek_gpio_membase + (reg * 0x10) + (rg->bank * 0x4));
> }
> 
> Thanks!
> 
> NeilBrown

I have just sent v2 with things you pointed out here probably fixed and other remaining
cleanups pointed in this thread.

Hope this helps,

Best regards,
    Sergio Paracuellos