diff mbox series

gpio: aspeed: Pass irqchip when adding gpiochip

Message ID 20190809125515.19094-1-linus.walleij@linaro.org
State Accepted
Commit 8512ee31926296fd7d65c1b3c97bb20d67e7eb2a
Headers show
Series gpio: aspeed: Pass irqchip when adding gpiochip | expand

Commit Message

Linus Walleij Aug. 9, 2019, 12:55 p.m. UTC 
We need to convert all old gpio irqchips to pass the irqchip
setup along when adding the gpio_chip. For more info see
drivers/gpio/TODO.

For chained irqchips this is a pretty straight-forward
conversion.

Cc: Joel Stanley <joel@jms.id.au>
Cc: Andrew Jeffery <andrew@aj.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Thierry Reding <treding@nvidia.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
Hi Aspeed folks, it'd be great if you could test/review this
patch.
---
 drivers/gpio/gpio-aspeed.c | 60 ++++++++++++++++++--------------------
 1 file changed, 29 insertions(+), 31 deletions(-)

-- 
2.21.0

Comments

Andrew Jeffery Aug. 13, 2019, 4:36 a.m. UTC | #1 
On Fri, 9 Aug 2019, at 22:25, Linus Walleij wrote:
> We need to convert all old gpio irqchips to pass the irqchip

> setup along when adding the gpio_chip. For more info see

> drivers/gpio/TODO.

> 

> For chained irqchips this is a pretty straight-forward

> conversion.

> 

> Cc: Joel Stanley <joel@jms.id.au>

> Cc: Andrew Jeffery <andrew@aj.id.au>

> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>

> Cc: Thierry Reding <treding@nvidia.com>

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> ---

> Hi Aspeed folks, it'd be great if you could test/review this

> patch.


Used it to exercise our WIP GPIO model for qemu. After fixing the qemu bugs it
worked fine there. Booted it on a Witherspoon and things worked as expected
there too. From a brief inspection it looks fine to me.

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

Tested-by: Andrew Jeffery <andrew@aj.id.au>


> ---

>  drivers/gpio/gpio-aspeed.c | 60 ++++++++++++++++++--------------------

>  1 file changed, 29 insertions(+), 31 deletions(-)

> 

> diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c

> index 13d80bfbc3b6..9defe25d4721 100644

> --- a/drivers/gpio/gpio-aspeed.c

> +++ b/drivers/gpio/gpio-aspeed.c

> @@ -711,32 +711,6 @@ static void set_irq_valid_mask(struct aspeed_gpio *gpio)

>  	}

>  }

>  

> -static int aspeed_gpio_setup_irqs(struct aspeed_gpio *gpio,

> -		struct platform_device *pdev)

> -{

> -	int rc;

> -

> -	rc = platform_get_irq(pdev, 0);

> -	if (rc < 0)

> -		return rc;

> -

> -	gpio->irq = rc;

> -

> -	set_irq_valid_mask(gpio);

> -

> -	rc = gpiochip_irqchip_add(&gpio->chip, &aspeed_gpio_irqchip,

> -			0, handle_bad_irq, IRQ_TYPE_NONE);

> -	if (rc) {

> -		dev_info(&pdev->dev, "Could not add irqchip\n");

> -		return rc;

> -	}

> -

> -	gpiochip_set_chained_irqchip(&gpio->chip, &aspeed_gpio_irqchip,

> -				     gpio->irq, aspeed_gpio_irq_handler);

> -

> -	return 0;

> -}

> -

>  static int aspeed_gpio_reset_tolerance(struct gpio_chip *chip,

>  					unsigned int offset, bool enable)

>  {

> @@ -1189,7 +1163,6 @@ static int __init aspeed_gpio_probe(struct 

> platform_device *pdev)

>  	gpio->chip.set_config = aspeed_gpio_set_config;

>  	gpio->chip.label = dev_name(&pdev->dev);

>  	gpio->chip.base = -1;

> -	gpio->chip.irq.need_valid_mask = true;

>  

>  	/* Allocate a cache of the output registers */

>  	banks = gpio->config->nr_gpios >> 5;

> @@ -1212,16 +1185,41 @@ static int __init aspeed_gpio_probe(struct 

> platform_device *pdev)

>  		aspeed_gpio_change_cmd_source(gpio, bank, 3, GPIO_CMDSRC_ARM);

>  	}

>  

> -	rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);

> -	if (rc < 0)

> -		return rc;

> +	/* Optionally set up an irqchip if there is an IRQ */

> +	rc = platform_get_irq(pdev, 0);

> +	if (rc > 0) {

> +		struct gpio_irq_chip *girq;

> +

> +		gpio->irq = rc;

> +		girq = &gpio->chip.irq;

> +		girq->chip = &aspeed_gpio_irqchip;

> +		girq->parent_handler = aspeed_gpio_irq_handler;

> +		girq->num_parents = 1;

> +		girq->parents = devm_kcalloc(&pdev->dev, 1,

> +					     sizeof(*girq->parents),

> +					     GFP_KERNEL);

> +		if (!girq->parents)

> +			return -ENOMEM;

> +		girq->parents[0] = gpio->irq;

> +		girq->default_type = IRQ_TYPE_NONE;

> +		girq->handler = handle_bad_irq;

> +		girq->need_valid_mask = true;

> +	}

>  

>  	gpio->offset_timer =

>  		devm_kzalloc(&pdev->dev, gpio->chip.ngpio, GFP_KERNEL);

>  	if (!gpio->offset_timer)

>  		return -ENOMEM;

>  

> -	return aspeed_gpio_setup_irqs(gpio, pdev);

> +	rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);

> +	if (rc < 0)

> +		return rc;

> +

> +	/* Now the valid mask is allocated */

> +	if (gpio->irq)

> +		set_irq_valid_mask(gpio);

> +

> +	return 0;

>  }

>  

>  static struct platform_driver aspeed_gpio_driver = {

> -- 

> 2.21.0

> 

>
Joel Stanley Aug. 13, 2019, 4:44 a.m. UTC | #2 
On Fri, 9 Aug 2019 at 12:55, Linus Walleij <linus.walleij@linaro.org> wrote:
>

> We need to convert all old gpio irqchips to pass the irqchip

> setup along when adding the gpio_chip. For more info see

> drivers/gpio/TODO.

>

> For chained irqchips this is a pretty straight-forward

> conversion.

>

> Cc: Joel Stanley <joel@jms.id.au>

> Cc: Andrew Jeffery <andrew@aj.id.au>

> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>

> Cc: Thierry Reding <treding@nvidia.com>

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> ---

> Hi Aspeed folks, it'd be great if you could test/review this

> patch.

> ---

>  drivers/gpio/gpio-aspeed.c | 60 ++++++++++++++++++--------------------

>  1 file changed, 29 insertions(+), 31 deletions(-)

>

> diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c

> index 13d80bfbc3b6..9defe25d4721 100644

> --- a/drivers/gpio/gpio-aspeed.c

> +++ b/drivers/gpio/gpio-aspeed.c

> @@ -711,32 +711,6 @@ static void set_irq_valid_mask(struct aspeed_gpio *gpio)

>         }

>  }

>

> -static int aspeed_gpio_setup_irqs(struct aspeed_gpio *gpio,

> -               struct platform_device *pdev)

> -{

> -       int rc;

> -

> -       rc = platform_get_irq(pdev, 0);

> -       if (rc < 0)

> -               return rc;

> -

> -       gpio->irq = rc;

> -

> -       set_irq_valid_mask(gpio);

> -

> -       rc = gpiochip_irqchip_add(&gpio->chip, &aspeed_gpio_irqchip,

> -                       0, handle_bad_irq, IRQ_TYPE_NONE);

> -       if (rc) {

> -               dev_info(&pdev->dev, "Could not add irqchip\n");

> -               return rc;

> -       }

> -

> -       gpiochip_set_chained_irqchip(&gpio->chip, &aspeed_gpio_irqchip,

> -                                    gpio->irq, aspeed_gpio_irq_handler);

> -

> -       return 0;

> -}

> -

>  static int aspeed_gpio_reset_tolerance(struct gpio_chip *chip,

>                                         unsigned int offset, bool enable)

>  {

> @@ -1189,7 +1163,6 @@ static int __init aspeed_gpio_probe(struct platform_device *pdev)

>         gpio->chip.set_config = aspeed_gpio_set_config;

>         gpio->chip.label = dev_name(&pdev->dev);

>         gpio->chip.base = -1;

> -       gpio->chip.irq.need_valid_mask = true;

>

>         /* Allocate a cache of the output registers */

>         banks = gpio->config->nr_gpios >> 5;

> @@ -1212,16 +1185,41 @@ static int __init aspeed_gpio_probe(struct platform_device *pdev)

>                 aspeed_gpio_change_cmd_source(gpio, bank, 3, GPIO_CMDSRC_ARM);

>         }

>

> -       rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);

> -       if (rc < 0)

> -               return rc;

> +       /* Optionally set up an irqchip if there is an IRQ */

> +       rc = platform_get_irq(pdev, 0);

> +       if (rc > 0) {

> +               struct gpio_irq_chip *girq;

> +

> +               gpio->irq = rc;

> +               girq = &gpio->chip.irq;

> +               girq->chip = &aspeed_gpio_irqchip;

> +               girq->parent_handler = aspeed_gpio_irq_handler;

> +               girq->num_parents = 1;

> +               girq->parents = devm_kcalloc(&pdev->dev, 1,

> +                                            sizeof(*girq->parents),

> +                                            GFP_KERNEL);


Is there a reason this is kcalloc and not kzalloc?

Other than that,

Acked-by: Joel Stanley <joel@jms.id.au>


Cheers,

Joel

> +               if (!girq->parents)

> +                       return -ENOMEM;

> +               girq->parents[0] = gpio->irq;

> +               girq->default_type = IRQ_TYPE_NONE;

> +               girq->handler = handle_bad_irq;

> +               girq->need_valid_mask = true;

> +       }

>

>         gpio->offset_timer =

>                 devm_kzalloc(&pdev->dev, gpio->chip.ngpio, GFP_KERNEL);

>         if (!gpio->offset_timer)

>                 return -ENOMEM;

>

> -       return aspeed_gpio_setup_irqs(gpio, pdev);

> +       rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);

> +       if (rc < 0)

> +               return rc;

> +

> +       /* Now the valid mask is allocated */

> +       if (gpio->irq)

> +               set_irq_valid_mask(gpio);

> +

> +       return 0;

>  }

>

>  static struct platform_driver aspeed_gpio_driver = {

> --

> 2.21.0

>
Linus Walleij Aug. 15, 2019, 8:01 a.m. UTC | #3 
On Tue, Aug 13, 2019 at 6:44 AM Joel Stanley <joel@jms.id.au> wrote:
> On Fri, 9 Aug 2019 at 12:55, Linus Walleij <linus.walleij@linaro.org> wrote:


> > +               girq->parents = devm_kcalloc(&pdev->dev, 1,

> > +                                            sizeof(*girq->parents),

> > +                                            GFP_KERNEL);

>

> Is there a reason this is kcalloc and not kzalloc?


Following the pattern that we have one item of sizeof().
The parents can be several, so we try to follow this
pattern so people get it right.

> Acked-by: Joel Stanley <joel@jms.id.au>


Thanks!

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
index 13d80bfbc3b6..9defe25d4721 100644
--- a/drivers/gpio/gpio-aspeed.c
+++ b/drivers/gpio/gpio-aspeed.c
@@ -711,32 +711,6 @@  static void set_irq_valid_mask(struct aspeed_gpio *gpio)
 	}
 }
 
-static int aspeed_gpio_setup_irqs(struct aspeed_gpio *gpio,
-		struct platform_device *pdev)
-{
-	int rc;
-
-	rc = platform_get_irq(pdev, 0);
-	if (rc < 0)
-		return rc;
-
-	gpio->irq = rc;
-
-	set_irq_valid_mask(gpio);
-
-	rc = gpiochip_irqchip_add(&gpio->chip, &aspeed_gpio_irqchip,
-			0, handle_bad_irq, IRQ_TYPE_NONE);
-	if (rc) {
-		dev_info(&pdev->dev, "Could not add irqchip\n");
-		return rc;
-	}
-
-	gpiochip_set_chained_irqchip(&gpio->chip, &aspeed_gpio_irqchip,
-				     gpio->irq, aspeed_gpio_irq_handler);
-
-	return 0;
-}
-
 static int aspeed_gpio_reset_tolerance(struct gpio_chip *chip,
 					unsigned int offset, bool enable)
 {
@@ -1189,7 +1163,6 @@  static int __init aspeed_gpio_probe(struct platform_device *pdev)
 	gpio->chip.set_config = aspeed_gpio_set_config;
 	gpio->chip.label = dev_name(&pdev->dev);
 	gpio->chip.base = -1;
-	gpio->chip.irq.need_valid_mask = true;
 
 	/* Allocate a cache of the output registers */
 	banks = gpio->config->nr_gpios >> 5;
@@ -1212,16 +1185,41 @@  static int __init aspeed_gpio_probe(struct platform_device *pdev)
 		aspeed_gpio_change_cmd_source(gpio, bank, 3, GPIO_CMDSRC_ARM);
 	}
 
-	rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);
-	if (rc < 0)
-		return rc;
+	/* Optionally set up an irqchip if there is an IRQ */
+	rc = platform_get_irq(pdev, 0);
+	if (rc > 0) {
+		struct gpio_irq_chip *girq;
+
+		gpio->irq = rc;
+		girq = &gpio->chip.irq;
+		girq->chip = &aspeed_gpio_irqchip;
+		girq->parent_handler = aspeed_gpio_irq_handler;
+		girq->num_parents = 1;
+		girq->parents = devm_kcalloc(&pdev->dev, 1,
+					     sizeof(*girq->parents),
+					     GFP_KERNEL);
+		if (!girq->parents)
+			return -ENOMEM;
+		girq->parents[0] = gpio->irq;
+		girq->default_type = IRQ_TYPE_NONE;
+		girq->handler = handle_bad_irq;
+		girq->need_valid_mask = true;
+	}
 
 	gpio->offset_timer =
 		devm_kzalloc(&pdev->dev, gpio->chip.ngpio, GFP_KERNEL);
 	if (!gpio->offset_timer)
 		return -ENOMEM;
 
-	return aspeed_gpio_setup_irqs(gpio, pdev);
+	rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);
+	if (rc < 0)
+		return rc;
+
+	/* Now the valid mask is allocated */
+	if (gpio->irq)
+		set_irq_valid_mask(gpio);
+
+	return 0;
 }
 
 static struct platform_driver aspeed_gpio_driver = {