[PATCH] gpio/pinctrl: Add pin ranges before gpiochip

[PATCH] gpio/pinctrl: Add pin ranges before gpiochip

[Linus Walleij]

This fixes a semantic ordering issue as we need to add
pin ranges before adding gpiochips when gpiochips use
pin control as a backend: as it needs to talk to the
pin control backend during initialization, the backend
needs to be there already.

Other drivers in the tree using pincontrol as backend do
not necessarily have this problem, as they might not need
to access the pincontrol portions during initialization.

Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Reported-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Hans: would be great if you could test this. I can't
even compiletest right now because of a slow machine
as workhorse...
---
 drivers/gpio/gpio-merrifield.c             | 18 +++++++++++-------
 drivers/pinctrl/intel/pinctrl-baytrail.c   | 14 +++++++++-----
 drivers/pinctrl/intel/pinctrl-cherryview.c | 16 ++++++++++------
 drivers/pinctrl/intel/pinctrl-intel.c      | 16 ++++++++++------
 drivers/pinctrl/pinctrl-amd.c              | 14 +++++++++-----
 5 files changed, 49 insertions(+), 29 deletions(-)

diff --git a/drivers/gpio/gpio-merrifield.c b/drivers/gpio/gpio-merrifield.c
index 2f1e9da81c1e..195e253cb561 100644
--- a/drivers/gpio/gpio-merrifield.c
+++ b/drivers/gpio/gpio-merrifield.c
@@ -463,13 +463,10 @@ static int mrfld_gpio_probe(struct pci_dev *pdev, const struct pci_device_id *id
 	girq->default_type = IRQ_TYPE_NONE;
 	girq->handler = handle_bad_irq;
 
-	pci_set_drvdata(pdev, priv);
-	retval = devm_gpiochip_add_data(&pdev->dev, &priv->chip, priv);
-	if (retval) {
-		dev_err(&pdev->dev, "gpiochip_add error %d\n", retval);
-		return retval;
-	}
-
+	/*
+	 * Needs to happen first since the gpiochip is using pin
+	 * control as back-end.
+	 */
 	pinctrl_dev_name = mrfld_gpio_get_pinctrl_dev_name(priv);
 	for (i = 0; i < ARRAY_SIZE(mrfld_gpio_ranges); i++) {
 		range = &mrfld_gpio_ranges[i];
@@ -484,6 +481,13 @@ static int mrfld_gpio_probe(struct pci_dev *pdev, const struct pci_device_id *id
 		}
 	}
 
+	pci_set_drvdata(pdev, priv);
+	retval = devm_gpiochip_add_data(&pdev->dev, &priv->chip, priv);
+	if (retval) {
+		dev_err(&pdev->dev, "gpiochip_add error %d\n", retval);
+		return retval;
+	}
+
 	return 0;
 }
 
diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
index beb26550c25f..b308567c5153 100644
--- a/drivers/pinctrl/intel/pinctrl-baytrail.c
+++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
@@ -1549,16 +1549,20 @@ static int byt_gpio_probe(struct byt_gpio *vg)
 		girq->handler = handle_bad_irq;
 	}
 
-	ret = devm_gpiochip_add_data(&vg->pdev->dev, gc, vg);
+	/*
+	 * Needs to happen first since the gpiochip is using pin
+	 * control as back-end.
+	 */
+	ret = gpiochip_add_pin_range(gc, dev_name(&vg->pdev->dev),
+				     0, 0, vg->soc_data->npins);
 	if (ret) {
-		dev_err(&vg->pdev->dev, "failed adding byt-gpio chip\n");
+		dev_err(&vg->pdev->dev, "failed to add GPIO pin range\n");
 		return ret;
 	}
 
-	ret = gpiochip_add_pin_range(&vg->chip, dev_name(&vg->pdev->dev),
-				     0, 0, vg->soc_data->npins);
+	ret = devm_gpiochip_add_data(&vg->pdev->dev, gc, vg);
 	if (ret) {
-		dev_err(&vg->pdev->dev, "failed to add GPIO pin range\n");
+		dev_err(&vg->pdev->dev, "failed adding byt-gpio chip\n");
 		return ret;
 	}
 
diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index dff2a81250b6..13144e192c1f 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -1572,12 +1572,10 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
 	if (need_valid_mask)
 		chip->irq.init_valid_mask = chv_init_irq_valid_mask;
 
-	ret = devm_gpiochip_add_data(pctrl->dev, chip, pctrl);
-	if (ret) {
-		dev_err(pctrl->dev, "Failed to register gpiochip\n");
-		return ret;
-	}
-
+	/*
+	 * Needs to happen first since the gpiochip is using pin
+	 * control as back-end.
+	 */
 	for (i = 0; i < community->ngpio_ranges; i++) {
 		range = &community->gpio_ranges[i];
 		ret = gpiochip_add_pin_range(chip, dev_name(pctrl->dev),
@@ -1589,6 +1587,12 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
 		}
 	}
 
+	ret = devm_gpiochip_add_data(pctrl->dev, chip, pctrl);
+	if (ret) {
+		dev_err(pctrl->dev, "Failed to register gpiochip\n");
+		return ret;
+	}
+
 	/*
 	 * The same set of machines in chv_no_valid_mask[] have incorrectly
 	 * configured GPIOs that generate spurious interrupts so we use
diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index b54b27228ad9..78afcf13c444 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -1225,12 +1225,10 @@ static int intel_gpio_probe(struct intel_pinctrl *pctrl, int irq)
 	pctrl->irqchip.irq_set_wake = intel_gpio_irq_wake;
 	pctrl->irqchip.flags = IRQCHIP_MASK_ON_SUSPEND;
 
-	ret = devm_gpiochip_add_data(pctrl->dev, &pctrl->chip, pctrl);
-	if (ret) {
-		dev_err(pctrl->dev, "failed to register gpiochip\n");
-		return ret;
-	}
-
+	/*
+	 * Needs to happen first since the gpiochip is using pin
+	 * control as back-end.
+	 */
 	for (i = 0; i < pctrl->ncommunities; i++) {
 		struct intel_community *community = &pctrl->communities[i];
 
@@ -1241,6 +1239,12 @@ static int intel_gpio_probe(struct intel_pinctrl *pctrl, int irq)
 		}
 	}
 
+	ret = devm_gpiochip_add_data(pctrl->dev, &pctrl->chip, pctrl);
+	if (ret) {
+		dev_err(pctrl->dev, "failed to register gpiochip\n");
+		return ret;
+	}
+
 	/*
 	 * We need to request the interrupt here (instead of providing chip
 	 * to the irq directly) because on some platforms several GPIO
diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index 2c61141519f8..3637059083ff 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -912,17 +912,21 @@ static int amd_gpio_probe(struct platform_device *pdev)
 		return PTR_ERR(gpio_dev->pctrl);
 	}
 
-	ret = gpiochip_add_data(&gpio_dev->gc, gpio_dev);
-	if (ret)
-		return ret;
-
+	/*
+	 * Needs to happen first since the gpiochip is using pin
+	 * control as back-end.
+	 */
 	ret = gpiochip_add_pin_range(&gpio_dev->gc, dev_name(&pdev->dev),
 				0, 0, gpio_dev->gc.ngpio);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to add pin range\n");
-		goto out2;
+		return ret;
 	}
 
+	ret = gpiochip_add_data(&gpio_dev->gc, gpio_dev);
+	if (ret)
+		return ret;
+
 	ret = gpiochip_irqchip_add(&gpio_dev->gc,
 				&amd_gpio_irqchip,
 				0,
-- 
2.21.0

Re: [PATCH] gpio/pinctrl: Add pin ranges before gpiochip

[Hans de Goede]

Hi,

On 30-10-2019 15:49, Linus Walleij wrote:
> This fixes a semantic ordering issue as we need to add
> pin ranges before adding gpiochips when gpiochips use
> pin control as a backend: as it needs to talk to the
> pin control backend during initialization, the backend
> needs to be there already.
> 
> Other drivers in the tree using pincontrol as backend do
> not necessarily have this problem, as they might not need
> to access the pincontrol portions during initialization.
> 
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Reported-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Hans: would be great if you could test this. I can't
> even compiletest right now because of a slow machine
> as workhorse...

Thanks, I will give this a test-run asap.

Regards,

Hans


> ---
>   drivers/gpio/gpio-merrifield.c             | 18 +++++++++++-------
>   drivers/pinctrl/intel/pinctrl-baytrail.c   | 14 +++++++++-----
>   drivers/pinctrl/intel/pinctrl-cherryview.c | 16 ++++++++++------
>   drivers/pinctrl/intel/pinctrl-intel.c      | 16 ++++++++++------
>   drivers/pinctrl/pinctrl-amd.c              | 14 +++++++++-----
>   5 files changed, 49 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-merrifield.c b/drivers/gpio/gpio-merrifield.c
> index 2f1e9da81c1e..195e253cb561 100644
> --- a/drivers/gpio/gpio-merrifield.c
> +++ b/drivers/gpio/gpio-merrifield.c
> @@ -463,13 +463,10 @@ static int mrfld_gpio_probe(struct pci_dev *pdev, const struct pci_device_id *id
>   	girq->default_type = IRQ_TYPE_NONE;
>   	girq->handler = handle_bad_irq;
>   
> -	pci_set_drvdata(pdev, priv);
> -	retval = devm_gpiochip_add_data(&pdev->dev, &priv->chip, priv);
> -	if (retval) {
> -		dev_err(&pdev->dev, "gpiochip_add error %d\n", retval);
> -		return retval;
> -	}
> -
> +	/*
> +	 * Needs to happen first since the gpiochip is using pin
> +	 * control as back-end.
> +	 */
>   	pinctrl_dev_name = mrfld_gpio_get_pinctrl_dev_name(priv);
>   	for (i = 0; i < ARRAY_SIZE(mrfld_gpio_ranges); i++) {
>   		range = &mrfld_gpio_ranges[i];
> @@ -484,6 +481,13 @@ static int mrfld_gpio_probe(struct pci_dev *pdev, const struct pci_device_id *id
>   		}
>   	}
>   
> +	pci_set_drvdata(pdev, priv);
> +	retval = devm_gpiochip_add_data(&pdev->dev, &priv->chip, priv);
> +	if (retval) {
> +		dev_err(&pdev->dev, "gpiochip_add error %d\n", retval);
> +		return retval;
> +	}
> +
>   	return 0;
>   }
>   
> diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
> index beb26550c25f..b308567c5153 100644
> --- a/drivers/pinctrl/intel/pinctrl-baytrail.c
> +++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
> @@ -1549,16 +1549,20 @@ static int byt_gpio_probe(struct byt_gpio *vg)
>   		girq->handler = handle_bad_irq;
>   	}
>   
> -	ret = devm_gpiochip_add_data(&vg->pdev->dev, gc, vg);
> +	/*
> +	 * Needs to happen first since the gpiochip is using pin
> +	 * control as back-end.
> +	 */
> +	ret = gpiochip_add_pin_range(gc, dev_name(&vg->pdev->dev),
> +				     0, 0, vg->soc_data->npins);
>   	if (ret) {
> -		dev_err(&vg->pdev->dev, "failed adding byt-gpio chip\n");
> +		dev_err(&vg->pdev->dev, "failed to add GPIO pin range\n");
>   		return ret;
>   	}
>   
> -	ret = gpiochip_add_pin_range(&vg->chip, dev_name(&vg->pdev->dev),
> -				     0, 0, vg->soc_data->npins);
> +	ret = devm_gpiochip_add_data(&vg->pdev->dev, gc, vg);
>   	if (ret) {
> -		dev_err(&vg->pdev->dev, "failed to add GPIO pin range\n");
> +		dev_err(&vg->pdev->dev, "failed adding byt-gpio chip\n");
>   		return ret;
>   	}
>   
> diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
> index dff2a81250b6..13144e192c1f 100644
> --- a/drivers/pinctrl/intel/pinctrl-cherryview.c
> +++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
> @@ -1572,12 +1572,10 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
>   	if (need_valid_mask)
>   		chip->irq.init_valid_mask = chv_init_irq_valid_mask;
>   
> -	ret = devm_gpiochip_add_data(pctrl->dev, chip, pctrl);
> -	if (ret) {
> -		dev_err(pctrl->dev, "Failed to register gpiochip\n");
> -		return ret;
> -	}
> -
> +	/*
> +	 * Needs to happen first since the gpiochip is using pin
> +	 * control as back-end.
> +	 */
>   	for (i = 0; i < community->ngpio_ranges; i++) {
>   		range = &community->gpio_ranges[i];
>   		ret = gpiochip_add_pin_range(chip, dev_name(pctrl->dev),
> @@ -1589,6 +1587,12 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
>   		}
>   	}
>   
> +	ret = devm_gpiochip_add_data(pctrl->dev, chip, pctrl);
> +	if (ret) {
> +		dev_err(pctrl->dev, "Failed to register gpiochip\n");
> +		return ret;
> +	}
> +
>   	/*
>   	 * The same set of machines in chv_no_valid_mask[] have incorrectly
>   	 * configured GPIOs that generate spurious interrupts so we use
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index b54b27228ad9..78afcf13c444 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -1225,12 +1225,10 @@ static int intel_gpio_probe(struct intel_pinctrl *pctrl, int irq)
>   	pctrl->irqchip.irq_set_wake = intel_gpio_irq_wake;
>   	pctrl->irqchip.flags = IRQCHIP_MASK_ON_SUSPEND;
>   
> -	ret = devm_gpiochip_add_data(pctrl->dev, &pctrl->chip, pctrl);
> -	if (ret) {
> -		dev_err(pctrl->dev, "failed to register gpiochip\n");
> -		return ret;
> -	}
> -
> +	/*
> +	 * Needs to happen first since the gpiochip is using pin
> +	 * control as back-end.
> +	 */
>   	for (i = 0; i < pctrl->ncommunities; i++) {
>   		struct intel_community *community = &pctrl->communities[i];
>   
> @@ -1241,6 +1239,12 @@ static int intel_gpio_probe(struct intel_pinctrl *pctrl, int irq)
>   		}
>   	}
>   
> +	ret = devm_gpiochip_add_data(pctrl->dev, &pctrl->chip, pctrl);
> +	if (ret) {
> +		dev_err(pctrl->dev, "failed to register gpiochip\n");
> +		return ret;
> +	}
> +
>   	/*
>   	 * We need to request the interrupt here (instead of providing chip
>   	 * to the irq directly) because on some platforms several GPIO
> diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
> index 2c61141519f8..3637059083ff 100644
> --- a/drivers/pinctrl/pinctrl-amd.c
> +++ b/drivers/pinctrl/pinctrl-amd.c
> @@ -912,17 +912,21 @@ static int amd_gpio_probe(struct platform_device *pdev)
>   		return PTR_ERR(gpio_dev->pctrl);
>   	}
>   
> -	ret = gpiochip_add_data(&gpio_dev->gc, gpio_dev);
> -	if (ret)
> -		return ret;
> -
> +	/*
> +	 * Needs to happen first since the gpiochip is using pin
> +	 * control as back-end.
> +	 */
>   	ret = gpiochip_add_pin_range(&gpio_dev->gc, dev_name(&pdev->dev),
>   				0, 0, gpio_dev->gc.ngpio);
>   	if (ret) {
>   		dev_err(&pdev->dev, "Failed to add pin range\n");
> -		goto out2;
> +		return ret;
>   	}
>   
> +	ret = gpiochip_add_data(&gpio_dev->gc, gpio_dev);
> +	if (ret)
> +		return ret;
> +
>   	ret = gpiochip_irqchip_add(&gpio_dev->gc,
>   				&amd_gpio_irqchip,
>   				0,
> 

Re: [PATCH] gpio/pinctrl: Add pin ranges before gpiochip

[Andy Shevchenko]

On Wed, Oct 30, 2019 at 03:49:40PM +0100, Linus Walleij wrote:
> This fixes a semantic ordering issue as we need to add
> pin ranges before adding gpiochips when gpiochips use
> pin control as a backend: as it needs to talk to the
> pin control backend during initialization, the backend
> needs to be there already.
> 
> Other drivers in the tree using pincontrol as backend do
> not necessarily have this problem, as they might not need
> to access the pincontrol portions during initialization.

Thanks for the fix!
One nit below.

> 
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Reported-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Hans: would be great if you could test this. I can't
> even compiletest right now because of a slow machine
> as workhorse...
> ---
>  drivers/gpio/gpio-merrifield.c             | 18 +++++++++++-------
>  drivers/pinctrl/intel/pinctrl-baytrail.c   | 14 +++++++++-----
>  drivers/pinctrl/intel/pinctrl-cherryview.c | 16 ++++++++++------
>  drivers/pinctrl/intel/pinctrl-intel.c      | 16 ++++++++++------
>  drivers/pinctrl/pinctrl-amd.c              | 14 +++++++++-----
>  5 files changed, 49 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-merrifield.c b/drivers/gpio/gpio-merrifield.c
> index 2f1e9da81c1e..195e253cb561 100644
> --- a/drivers/gpio/gpio-merrifield.c
> +++ b/drivers/gpio/gpio-merrifield.c
> @@ -463,13 +463,10 @@ static int mrfld_gpio_probe(struct pci_dev *pdev, const struct pci_device_id *id
>  	girq->default_type = IRQ_TYPE_NONE;
>  	girq->handler = handle_bad_irq;
>  
> -	pci_set_drvdata(pdev, priv);
> -	retval = devm_gpiochip_add_data(&pdev->dev, &priv->chip, priv);
> -	if (retval) {
> -		dev_err(&pdev->dev, "gpiochip_add error %d\n", retval);
> -		return retval;
> -	}
> -
> +	/*
> +	 * Needs to happen first since the gpiochip is using pin
> +	 * control as back-end.
> +	 */
>  	pinctrl_dev_name = mrfld_gpio_get_pinctrl_dev_name(priv);
>  	for (i = 0; i < ARRAY_SIZE(mrfld_gpio_ranges); i++) {
>  		range = &mrfld_gpio_ranges[i];
> @@ -484,6 +481,13 @@ static int mrfld_gpio_probe(struct pci_dev *pdev, const struct pci_device_id *id
>  		}
>  	}

> +	pci_set_drvdata(pdev, priv);

I think we may move it after the call before last return.

> +	retval = devm_gpiochip_add_data(&pdev->dev, &priv->chip, priv);
> +	if (retval) {
> +		dev_err(&pdev->dev, "gpiochip_add error %d\n", retval);
> +		return retval;
> +	}
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
> index beb26550c25f..b308567c5153 100644
> --- a/drivers/pinctrl/intel/pinctrl-baytrail.c
> +++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
> @@ -1549,16 +1549,20 @@ static int byt_gpio_probe(struct byt_gpio *vg)
>  		girq->handler = handle_bad_irq;
>  	}
>  
> -	ret = devm_gpiochip_add_data(&vg->pdev->dev, gc, vg);
> +	/*
> +	 * Needs to happen first since the gpiochip is using pin
> +	 * control as back-end.
> +	 */
> +	ret = gpiochip_add_pin_range(gc, dev_name(&vg->pdev->dev),
> +				     0, 0, vg->soc_data->npins);
>  	if (ret) {
> -		dev_err(&vg->pdev->dev, "failed adding byt-gpio chip\n");
> +		dev_err(&vg->pdev->dev, "failed to add GPIO pin range\n");
>  		return ret;
>  	}
>  
> -	ret = gpiochip_add_pin_range(&vg->chip, dev_name(&vg->pdev->dev),
> -				     0, 0, vg->soc_data->npins);
> +	ret = devm_gpiochip_add_data(&vg->pdev->dev, gc, vg);
>  	if (ret) {
> -		dev_err(&vg->pdev->dev, "failed to add GPIO pin range\n");
> +		dev_err(&vg->pdev->dev, "failed adding byt-gpio chip\n");
>  		return ret;
>  	}
>  
> diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
> index dff2a81250b6..13144e192c1f 100644
> --- a/drivers/pinctrl/intel/pinctrl-cherryview.c
> +++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
> @@ -1572,12 +1572,10 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
>  	if (need_valid_mask)
>  		chip->irq.init_valid_mask = chv_init_irq_valid_mask;
>  
> -	ret = devm_gpiochip_add_data(pctrl->dev, chip, pctrl);
> -	if (ret) {
> -		dev_err(pctrl->dev, "Failed to register gpiochip\n");
> -		return ret;
> -	}
> -
> +	/*
> +	 * Needs to happen first since the gpiochip is using pin
> +	 * control as back-end.
> +	 */
>  	for (i = 0; i < community->ngpio_ranges; i++) {
>  		range = &community->gpio_ranges[i];
>  		ret = gpiochip_add_pin_range(chip, dev_name(pctrl->dev),
> @@ -1589,6 +1587,12 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
>  		}
>  	}
>  
> +	ret = devm_gpiochip_add_data(pctrl->dev, chip, pctrl);
> +	if (ret) {
> +		dev_err(pctrl->dev, "Failed to register gpiochip\n");
> +		return ret;
> +	}
> +
>  	/*
>  	 * The same set of machines in chv_no_valid_mask[] have incorrectly
>  	 * configured GPIOs that generate spurious interrupts so we use
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index b54b27228ad9..78afcf13c444 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -1225,12 +1225,10 @@ static int intel_gpio_probe(struct intel_pinctrl *pctrl, int irq)
>  	pctrl->irqchip.irq_set_wake = intel_gpio_irq_wake;
>  	pctrl->irqchip.flags = IRQCHIP_MASK_ON_SUSPEND;
>  
> -	ret = devm_gpiochip_add_data(pctrl->dev, &pctrl->chip, pctrl);
> -	if (ret) {
> -		dev_err(pctrl->dev, "failed to register gpiochip\n");
> -		return ret;
> -	}
> -
> +	/*
> +	 * Needs to happen first since the gpiochip is using pin
> +	 * control as back-end.
> +	 */
>  	for (i = 0; i < pctrl->ncommunities; i++) {
>  		struct intel_community *community = &pctrl->communities[i];
>  
> @@ -1241,6 +1239,12 @@ static int intel_gpio_probe(struct intel_pinctrl *pctrl, int irq)
>  		}
>  	}
>  
> +	ret = devm_gpiochip_add_data(pctrl->dev, &pctrl->chip, pctrl);
> +	if (ret) {
> +		dev_err(pctrl->dev, "failed to register gpiochip\n");
> +		return ret;
> +	}
> +
>  	/*
>  	 * We need to request the interrupt here (instead of providing chip
>  	 * to the irq directly) because on some platforms several GPIO
> diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
> index 2c61141519f8..3637059083ff 100644
> --- a/drivers/pinctrl/pinctrl-amd.c
> +++ b/drivers/pinctrl/pinctrl-amd.c
> @@ -912,17 +912,21 @@ static int amd_gpio_probe(struct platform_device *pdev)
>  		return PTR_ERR(gpio_dev->pctrl);
>  	}
>  
> -	ret = gpiochip_add_data(&gpio_dev->gc, gpio_dev);
> -	if (ret)
> -		return ret;
> -
> +	/*
> +	 * Needs to happen first since the gpiochip is using pin
> +	 * control as back-end.
> +	 */
>  	ret = gpiochip_add_pin_range(&gpio_dev->gc, dev_name(&pdev->dev),
>  				0, 0, gpio_dev->gc.ngpio);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Failed to add pin range\n");
> -		goto out2;
> +		return ret;
>  	}
>  
> +	ret = gpiochip_add_data(&gpio_dev->gc, gpio_dev);
> +	if (ret)
> +		return ret;
> +
>  	ret = gpiochip_irqchip_add(&gpio_dev->gc,
>  				&amd_gpio_irqchip,
>  				0,
> -- 
> 2.21.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpio/pinctrl: Add pin ranges before gpiochip

[Mika Westerberg]

On Wed, Oct 30, 2019 at 03:49:40PM +0100, Linus Walleij wrote:
> This fixes a semantic ordering issue as we need to add
> pin ranges before adding gpiochips when gpiochips use
> pin control as a backend: as it needs to talk to the
> pin control backend during initialization, the backend
> needs to be there already.
> 
> Other drivers in the tree using pincontrol as backend do
> not necessarily have this problem, as they might not need
> to access the pincontrol portions during initialization.
> 
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Reported-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Looks good to me as well. Are you going to take this?

In that case

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH] gpio/pinctrl: Add pin ranges before gpiochip

[Hans de Goede]

Hi,

On 30-10-2019 15:49, Linus Walleij wrote:
> This fixes a semantic ordering issue as we need to add
> pin ranges before adding gpiochips when gpiochips use
> pin control as a backend: as it needs to talk to the
> pin control backend during initialization, the backend
> needs to be there already.
> 
> Other drivers in the tree using pincontrol as backend do
> not necessarily have this problem, as they might not need
> to access the pincontrol portions during initialization.
> 
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Reported-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Hans: would be great if you could test this. I can't
> even compiletest right now because of a slow machine
> as workhorse...

This does not seem to work I've tested in both a Bay Trail and
a Cherry Trail device and neither will boot the kernel after
these changes I'm afraid.

I think it might be best to pass in an array of ranges (*) to
gpiochip_add_data and have it register the ranges before
doing the irq-chip setup.

Regards,

Hans



*) Terminated with a range from 0 to 0

> ---
>   drivers/gpio/gpio-merrifield.c             | 18 +++++++++++-------
>   drivers/pinctrl/intel/pinctrl-baytrail.c   | 14 +++++++++-----
>   drivers/pinctrl/intel/pinctrl-cherryview.c | 16 ++++++++++------
>   drivers/pinctrl/intel/pinctrl-intel.c      | 16 ++++++++++------
>   drivers/pinctrl/pinctrl-amd.c              | 14 +++++++++-----
>   5 files changed, 49 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-merrifield.c b/drivers/gpio/gpio-merrifield.c
> index 2f1e9da81c1e..195e253cb561 100644
> --- a/drivers/gpio/gpio-merrifield.c
> +++ b/drivers/gpio/gpio-merrifield.c
> @@ -463,13 +463,10 @@ static int mrfld_gpio_probe(struct pci_dev *pdev, const struct pci_device_id *id
>   	girq->default_type = IRQ_TYPE_NONE;
>   	girq->handler = handle_bad_irq;
>   
> -	pci_set_drvdata(pdev, priv);
> -	retval = devm_gpiochip_add_data(&pdev->dev, &priv->chip, priv);
> -	if (retval) {
> -		dev_err(&pdev->dev, "gpiochip_add error %d\n", retval);
> -		return retval;
> -	}
> -
> +	/*
> +	 * Needs to happen first since the gpiochip is using pin
> +	 * control as back-end.
> +	 */
>   	pinctrl_dev_name = mrfld_gpio_get_pinctrl_dev_name(priv);
>   	for (i = 0; i < ARRAY_SIZE(mrfld_gpio_ranges); i++) {
>   		range = &mrfld_gpio_ranges[i];
> @@ -484,6 +481,13 @@ static int mrfld_gpio_probe(struct pci_dev *pdev, const struct pci_device_id *id
>   		}
>   	}
>   
> +	pci_set_drvdata(pdev, priv);
> +	retval = devm_gpiochip_add_data(&pdev->dev, &priv->chip, priv);
> +	if (retval) {
> +		dev_err(&pdev->dev, "gpiochip_add error %d\n", retval);
> +		return retval;
> +	}
> +
>   	return 0;
>   }
>   
> diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
> index beb26550c25f..b308567c5153 100644
> --- a/drivers/pinctrl/intel/pinctrl-baytrail.c
> +++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
> @@ -1549,16 +1549,20 @@ static int byt_gpio_probe(struct byt_gpio *vg)
>   		girq->handler = handle_bad_irq;
>   	}
>   
> -	ret = devm_gpiochip_add_data(&vg->pdev->dev, gc, vg);
> +	/*
> +	 * Needs to happen first since the gpiochip is using pin
> +	 * control as back-end.
> +	 */
> +	ret = gpiochip_add_pin_range(gc, dev_name(&vg->pdev->dev),
> +				     0, 0, vg->soc_data->npins);
>   	if (ret) {
> -		dev_err(&vg->pdev->dev, "failed adding byt-gpio chip\n");
> +		dev_err(&vg->pdev->dev, "failed to add GPIO pin range\n");
>   		return ret;
>   	}
>   
> -	ret = gpiochip_add_pin_range(&vg->chip, dev_name(&vg->pdev->dev),
> -				     0, 0, vg->soc_data->npins);
> +	ret = devm_gpiochip_add_data(&vg->pdev->dev, gc, vg);
>   	if (ret) {
> -		dev_err(&vg->pdev->dev, "failed to add GPIO pin range\n");
> +		dev_err(&vg->pdev->dev, "failed adding byt-gpio chip\n");
>   		return ret;
>   	}
>   
> diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
> index dff2a81250b6..13144e192c1f 100644
> --- a/drivers/pinctrl/intel/pinctrl-cherryview.c
> +++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
> @@ -1572,12 +1572,10 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
>   	if (need_valid_mask)
>   		chip->irq.init_valid_mask = chv_init_irq_valid_mask;
>   
> -	ret = devm_gpiochip_add_data(pctrl->dev, chip, pctrl);
> -	if (ret) {
> -		dev_err(pctrl->dev, "Failed to register gpiochip\n");
> -		return ret;
> -	}
> -
> +	/*
> +	 * Needs to happen first since the gpiochip is using pin
> +	 * control as back-end.
> +	 */
>   	for (i = 0; i < community->ngpio_ranges; i++) {
>   		range = &community->gpio_ranges[i];
>   		ret = gpiochip_add_pin_range(chip, dev_name(pctrl->dev),
> @@ -1589,6 +1587,12 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
>   		}
>   	}
>   
> +	ret = devm_gpiochip_add_data(pctrl->dev, chip, pctrl);
> +	if (ret) {
> +		dev_err(pctrl->dev, "Failed to register gpiochip\n");
> +		return ret;
> +	}
> +
>   	/*
>   	 * The same set of machines in chv_no_valid_mask[] have incorrectly
>   	 * configured GPIOs that generate spurious interrupts so we use
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index b54b27228ad9..78afcf13c444 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -1225,12 +1225,10 @@ static int intel_gpio_probe(struct intel_pinctrl *pctrl, int irq)
>   	pctrl->irqchip.irq_set_wake = intel_gpio_irq_wake;
>   	pctrl->irqchip.flags = IRQCHIP_MASK_ON_SUSPEND;
>   
> -	ret = devm_gpiochip_add_data(pctrl->dev, &pctrl->chip, pctrl);
> -	if (ret) {
> -		dev_err(pctrl->dev, "failed to register gpiochip\n");
> -		return ret;
> -	}
> -
> +	/*
> +	 * Needs to happen first since the gpiochip is using pin
> +	 * control as back-end.
> +	 */
>   	for (i = 0; i < pctrl->ncommunities; i++) {
>   		struct intel_community *community = &pctrl->communities[i];
>   
> @@ -1241,6 +1239,12 @@ static int intel_gpio_probe(struct intel_pinctrl *pctrl, int irq)
>   		}
>   	}
>   
> +	ret = devm_gpiochip_add_data(pctrl->dev, &pctrl->chip, pctrl);
> +	if (ret) {
> +		dev_err(pctrl->dev, "failed to register gpiochip\n");
> +		return ret;
> +	}
> +
>   	/*
>   	 * We need to request the interrupt here (instead of providing chip
>   	 * to the irq directly) because on some platforms several GPIO
> diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
> index 2c61141519f8..3637059083ff 100644
> --- a/drivers/pinctrl/pinctrl-amd.c
> +++ b/drivers/pinctrl/pinctrl-amd.c
> @@ -912,17 +912,21 @@ static int amd_gpio_probe(struct platform_device *pdev)
>   		return PTR_ERR(gpio_dev->pctrl);
>   	}
>   
> -	ret = gpiochip_add_data(&gpio_dev->gc, gpio_dev);
> -	if (ret)
> -		return ret;
> -
> +	/*
> +	 * Needs to happen first since the gpiochip is using pin
> +	 * control as back-end.
> +	 */
>   	ret = gpiochip_add_pin_range(&gpio_dev->gc, dev_name(&pdev->dev),
>   				0, 0, gpio_dev->gc.ngpio);
>   	if (ret) {
>   		dev_err(&pdev->dev, "Failed to add pin range\n");
> -		goto out2;
> +		return ret;
>   	}
>   
> +	ret = gpiochip_add_data(&gpio_dev->gc, gpio_dev);
> +	if (ret)
> +		return ret;
> +
>   	ret = gpiochip_irqchip_add(&gpio_dev->gc,
>   				&amd_gpio_irqchip,
>   				0,
> 

Re: [PATCH] gpio/pinctrl: Add pin ranges before gpiochip

[Hans de Goede]

Hi,

On 30-10-2019 16:48, Hans de Goede wrote:
> Hi,
> 
> On 30-10-2019 15:49, Linus Walleij wrote:
>> This fixes a semantic ordering issue as we need to add
>> pin ranges before adding gpiochips when gpiochips use
>> pin control as a backend: as it needs to talk to the
>> pin control backend during initialization, the backend
>> needs to be there already.
>>
>> Other drivers in the tree using pincontrol as backend do
>> not necessarily have this problem, as they might not need
>> to access the pincontrol portions during initialization.
>>
>> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
>> Reported-by: Hans de Goede <hdegoede@redhat.com>
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>> ---
>> Hans: would be great if you could test this. I can't
>> even compiletest right now because of a slow machine
>> as workhorse...
> 
> This does not seem to work I've tested in both a Bay Trail and
> a Cherry Trail device and neither will boot the kernel after
> these changes I'm afraid.
> 
> I think it might be best to pass in an array of ranges (*) to
> gpiochip_add_data and have it register the ranges before
> doing the irq-chip setup.

p.s.

For 5.4 we should probably revert
"gpio: merrifield: Pass irqchip when adding gpiochip"
and the fixes added on top of it, since AFAICT _AEI handling
will be broken on merrifield after this change too.

So I suggest that we revert the following commits (in revert order):

4c87540940cbc7ddbe9674087919c605fd5c2ef1 "gpio: merrifield: Move hardware initialization to callback"
6658f87f219427ee776c498e07c878eb5cad1be2 "gpio: merrifield: Restore use of irq_base"
8f86a5b4ad679e4836733b47414226074eee4e4d "gpio: merrifield: Pass irqchip when adding gpiochip"

That seems like the safest thing to do at this point in the cycle.

Regards,

Hans


> *) Terminated with a range from 0 to 0
> 
>> ---
>>   drivers/gpio/gpio-merrifield.c             | 18 +++++++++++-------
>>   drivers/pinctrl/intel/pinctrl-baytrail.c   | 14 +++++++++-----
>>   drivers/pinctrl/intel/pinctrl-cherryview.c | 16 ++++++++++------
>>   drivers/pinctrl/intel/pinctrl-intel.c      | 16 ++++++++++------
>>   drivers/pinctrl/pinctrl-amd.c              | 14 +++++++++-----
>>   5 files changed, 49 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-merrifield.c b/drivers/gpio/gpio-merrifield.c
>> index 2f1e9da81c1e..195e253cb561 100644
>> --- a/drivers/gpio/gpio-merrifield.c
>> +++ b/drivers/gpio/gpio-merrifield.c
>> @@ -463,13 +463,10 @@ static int mrfld_gpio_probe(struct pci_dev *pdev, const struct pci_device_id *id
>>       girq->default_type = IRQ_TYPE_NONE;
>>       girq->handler = handle_bad_irq;
>> -    pci_set_drvdata(pdev, priv);
>> -    retval = devm_gpiochip_add_data(&pdev->dev, &priv->chip, priv);
>> -    if (retval) {
>> -        dev_err(&pdev->dev, "gpiochip_add error %d\n", retval);
>> -        return retval;
>> -    }
>> -
>> +    /*
>> +     * Needs to happen first since the gpiochip is using pin
>> +     * control as back-end.
>> +     */
>>       pinctrl_dev_name = mrfld_gpio_get_pinctrl_dev_name(priv);
>>       for (i = 0; i < ARRAY_SIZE(mrfld_gpio_ranges); i++) {
>>           range = &mrfld_gpio_ranges[i];
>> @@ -484,6 +481,13 @@ static int mrfld_gpio_probe(struct pci_dev *pdev, const struct pci_device_id *id
>>           }
>>       }
>> +    pci_set_drvdata(pdev, priv);
>> +    retval = devm_gpiochip_add_data(&pdev->dev, &priv->chip, priv);
>> +    if (retval) {
>> +        dev_err(&pdev->dev, "gpiochip_add error %d\n", retval);
>> +        return retval;
>> +    }
>> +
>>       return 0;
>>   }
>> diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
>> index beb26550c25f..b308567c5153 100644
>> --- a/drivers/pinctrl/intel/pinctrl-baytrail.c
>> +++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
>> @@ -1549,16 +1549,20 @@ static int byt_gpio_probe(struct byt_gpio *vg)
>>           girq->handler = handle_bad_irq;
>>       }
>> -    ret = devm_gpiochip_add_data(&vg->pdev->dev, gc, vg);
>> +    /*
>> +     * Needs to happen first since the gpiochip is using pin
>> +     * control as back-end.
>> +     */
>> +    ret = gpiochip_add_pin_range(gc, dev_name(&vg->pdev->dev),
>> +                     0, 0, vg->soc_data->npins);
>>       if (ret) {
>> -        dev_err(&vg->pdev->dev, "failed adding byt-gpio chip\n");
>> +        dev_err(&vg->pdev->dev, "failed to add GPIO pin range\n");
>>           return ret;
>>       }
>> -    ret = gpiochip_add_pin_range(&vg->chip, dev_name(&vg->pdev->dev),
>> -                     0, 0, vg->soc_data->npins);
>> +    ret = devm_gpiochip_add_data(&vg->pdev->dev, gc, vg);
>>       if (ret) {
>> -        dev_err(&vg->pdev->dev, "failed to add GPIO pin range\n");
>> +        dev_err(&vg->pdev->dev, "failed adding byt-gpio chip\n");
>>           return ret;
>>       }
>> diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
>> index dff2a81250b6..13144e192c1f 100644
>> --- a/drivers/pinctrl/intel/pinctrl-cherryview.c
>> +++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
>> @@ -1572,12 +1572,10 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
>>       if (need_valid_mask)
>>           chip->irq.init_valid_mask = chv_init_irq_valid_mask;
>> -    ret = devm_gpiochip_add_data(pctrl->dev, chip, pctrl);
>> -    if (ret) {
>> -        dev_err(pctrl->dev, "Failed to register gpiochip\n");
>> -        return ret;
>> -    }
>> -
>> +    /*
>> +     * Needs to happen first since the gpiochip is using pin
>> +     * control as back-end.
>> +     */
>>       for (i = 0; i < community->ngpio_ranges; i++) {
>>           range = &community->gpio_ranges[i];
>>           ret = gpiochip_add_pin_range(chip, dev_name(pctrl->dev),
>> @@ -1589,6 +1587,12 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
>>           }
>>       }
>> +    ret = devm_gpiochip_add_data(pctrl->dev, chip, pctrl);
>> +    if (ret) {
>> +        dev_err(pctrl->dev, "Failed to register gpiochip\n");
>> +        return ret;
>> +    }
>> +
>>       /*
>>        * The same set of machines in chv_no_valid_mask[] have incorrectly
>>        * configured GPIOs that generate spurious interrupts so we use
>> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
>> index b54b27228ad9..78afcf13c444 100644
>> --- a/drivers/pinctrl/intel/pinctrl-intel.c
>> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
>> @@ -1225,12 +1225,10 @@ static int intel_gpio_probe(struct intel_pinctrl *pctrl, int irq)
>>       pctrl->irqchip.irq_set_wake = intel_gpio_irq_wake;
>>       pctrl->irqchip.flags = IRQCHIP_MASK_ON_SUSPEND;
>> -    ret = devm_gpiochip_add_data(pctrl->dev, &pctrl->chip, pctrl);
>> -    if (ret) {
>> -        dev_err(pctrl->dev, "failed to register gpiochip\n");
>> -        return ret;
>> -    }
>> -
>> +    /*
>> +     * Needs to happen first since the gpiochip is using pin
>> +     * control as back-end.
>> +     */
>>       for (i = 0; i < pctrl->ncommunities; i++) {
>>           struct intel_community *community = &pctrl->communities[i];
>> @@ -1241,6 +1239,12 @@ static int intel_gpio_probe(struct intel_pinctrl *pctrl, int irq)
>>           }
>>       }
>> +    ret = devm_gpiochip_add_data(pctrl->dev, &pctrl->chip, pctrl);
>> +    if (ret) {
>> +        dev_err(pctrl->dev, "failed to register gpiochip\n");
>> +        return ret;
>> +    }
>> +
>>       /*
>>        * We need to request the interrupt here (instead of providing chip
>>        * to the irq directly) because on some platforms several GPIO
>> diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
>> index 2c61141519f8..3637059083ff 100644
>> --- a/drivers/pinctrl/pinctrl-amd.c
>> +++ b/drivers/pinctrl/pinctrl-amd.c
>> @@ -912,17 +912,21 @@ static int amd_gpio_probe(struct platform_device *pdev)
>>           return PTR_ERR(gpio_dev->pctrl);
>>       }
>> -    ret = gpiochip_add_data(&gpio_dev->gc, gpio_dev);
>> -    if (ret)
>> -        return ret;
>> -
>> +    /*
>> +     * Needs to happen first since the gpiochip is using pin
>> +     * control as back-end.
>> +     */
>>       ret = gpiochip_add_pin_range(&gpio_dev->gc, dev_name(&pdev->dev),
>>                   0, 0, gpio_dev->gc.ngpio);
>>       if (ret) {
>>           dev_err(&pdev->dev, "Failed to add pin range\n");
>> -        goto out2;
>> +        return ret;
>>       }
>> +    ret = gpiochip_add_data(&gpio_dev->gc, gpio_dev);
>> +    if (ret)
>> +        return ret;
>> +
>>       ret = gpiochip_irqchip_add(&gpio_dev->gc,
>>                   &amd_gpio_irqchip,
>>                   0,
>>

Re: [PATCH] gpio/pinctrl: Add pin ranges before gpiochip

[Linus Walleij]

On Wed, Oct 30, 2019 at 5:01 PM Hans de Goede <hdegoede@redhat.com> wrote:

> For 5.4 we should probably revert
> "gpio: merrifield: Pass irqchip when adding gpiochip"
> and the fixes added on top of it, since AFAICT _AEI handling
> will be broken on merrifield after this change too.
>
> So I suggest that we revert the following commits (in revert order):
>
> 4c87540940cbc7ddbe9674087919c605fd5c2ef1 "gpio: merrifield: Move hardware initialization to callback"
> 6658f87f219427ee776c498e07c878eb5cad1be2 "gpio: merrifield: Restore use of irq_base"
> 8f86a5b4ad679e4836733b47414226074eee4e4d "gpio: merrifield: Pass irqchip when adding gpiochip"
>
> That seems like the safest thing to do at this point in the cycle.

OK are the Intel people OK with this?

If so I'll go and revert them.

Mika: will any of the pin control fixes you sent collide with
this? (I guess not...)

Yours,
Linus Walleij

Re: [PATCH] gpio/pinctrl: Add pin ranges before gpiochip

[Linus Walleij]

On Wed, Oct 30, 2019 at 4:48 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 30-10-2019 15:49, Linus Walleij wrote:

> This does not seem to work I've tested in both a Bay Trail and
> a Cherry Trail device and neither will boot the kernel after
> these changes I'm afraid.
>
> I think it might be best to pass in an array of ranges (*) to
> gpiochip_add_data and have it register the ranges before
> doing the irq-chip setup.

Yeah this new way of populating GPIO irqchips seems to be
pretty imperialistic and pull the whole world into the gpiolib.
I don't mind, once we have the semantic in one place we
can get it right. (As with the previous ordering issues.)
Hopefully it saves us from other problems.

Thierry: is this approach for pin control ranges in line with your
initial design of the new way of registering gpio irqchips?

I guess I can draft something to test.

Yours,
Linus Walleij

Re: [PATCH] gpio/pinctrl: Add pin ranges before gpiochip

[Mika Westerberg]

On Wed, Oct 30, 2019 at 05:04:21PM +0100, Linus Walleij wrote:
> On Wed, Oct 30, 2019 at 5:01 PM Hans de Goede <hdegoede@redhat.com> wrote:
> 
> > For 5.4 we should probably revert
> > "gpio: merrifield: Pass irqchip when adding gpiochip"
> > and the fixes added on top of it, since AFAICT _AEI handling
> > will be broken on merrifield after this change too.
> >
> > So I suggest that we revert the following commits (in revert order):
> >
> > 4c87540940cbc7ddbe9674087919c605fd5c2ef1 "gpio: merrifield: Move hardware initialization to callback"
> > 6658f87f219427ee776c498e07c878eb5cad1be2 "gpio: merrifield: Restore use of irq_base"
> > 8f86a5b4ad679e4836733b47414226074eee4e4d "gpio: merrifield: Pass irqchip when adding gpiochip"
> >
> > That seems like the safest thing to do at this point in the cycle.
> 
> OK are the Intel people OK with this?

I'm fine but I'll leave this to Andy since that's his stuff.

> If so I'll go and revert them.
> 
> Mika: will any of the pin control fixes you sent collide with
> this? (I guess not...)

No they should not, they don't touch the merrifield driver.

Re: [PATCH] gpio/pinctrl: Add pin ranges before gpiochip

[Thierry Reding]

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

On Wed, Oct 30, 2019 at 05:07:17PM +0100, Linus Walleij wrote:
> On Wed, Oct 30, 2019 at 4:48 PM Hans de Goede <hdegoede@redhat.com> wrote:
> > On 30-10-2019 15:49, Linus Walleij wrote:
> 
> > This does not seem to work I've tested in both a Bay Trail and
> > a Cherry Trail device and neither will boot the kernel after
> > these changes I'm afraid.
> >
> > I think it might be best to pass in an array of ranges (*) to
> > gpiochip_add_data and have it register the ranges before
> > doing the irq-chip setup.
> 
> Yeah this new way of populating GPIO irqchips seems to be
> pretty imperialistic and pull the whole world into the gpiolib.
> I don't mind, once we have the semantic in one place we
> can get it right. (As with the previous ordering issues.)
> Hopefully it saves us from other problems.
> 
> Thierry: is this approach for pin control ranges in line with your
> initial design of the new way of registering gpio irqchips?

I'm not sure I fully understand what the pin control ranges have to do
with the embedded IRQ chip. The embedded IRQ chip represents a different
way of using the GPIO lines, so anything that impacts the IRQ chip would
presumably also impact the GPIO lines.

Generally speaking, though, I don't think there's anything wrong with
adding ranges to a GPIO chip descriptor before registering it. In fact I
think that's really the only way to make it work if we've got a
dependency on these ranges being available.

So technically at any point after gpiochip_add_data() somebody could
start using the GPIO chip, which means that technically there could be a
race between the consumers and the call to gpiochip_add_pin_range(). If
there's a dependency on the pin control ranges, they must be registered
before the GPIO chip becomes available to consumers.

Again, I don't think this has anything to do with IRQ chip. If you want
to provide pin control ranges as part of adding the GPIO chip, they
should be specified as part of the GPIO chip structure.

Regarding the imperialistic nature of this: back at the time I think
your argument had been that you wanted to move as much boiler plate as
possible into gpiolib. Seems like this is very much in line with that.

Thierry

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

Re: [PATCH] gpio/pinctrl: Add pin ranges before gpiochip

[Andy Shevchenko]

On Wed, Oct 30, 2019 at 06:10:41PM +0200, Mika Westerberg wrote:
> On Wed, Oct 30, 2019 at 05:04:21PM +0100, Linus Walleij wrote:
> > On Wed, Oct 30, 2019 at 5:01 PM Hans de Goede <hdegoede@redhat.com> wrote:
> > 
> > > For 5.4 we should probably revert
> > > "gpio: merrifield: Pass irqchip when adding gpiochip"
> > > and the fixes added on top of it, since AFAICT _AEI handling
> > > will be broken on merrifield after this change too.
> > >
> > > So I suggest that we revert the following commits (in revert order):
> > >
> > > 4c87540940cbc7ddbe9674087919c605fd5c2ef1 "gpio: merrifield: Move hardware initialization to callback"
> > > 6658f87f219427ee776c498e07c878eb5cad1be2 "gpio: merrifield: Restore use of irq_base"
> > > 8f86a5b4ad679e4836733b47414226074eee4e4d "gpio: merrifield: Pass irqchip when adding gpiochip"
> > >
> > > That seems like the safest thing to do at this point in the cycle.
> > 
> > OK are the Intel people OK with this?
> 
> I'm fine but I'll leave this to Andy since that's his stuff.

Ack.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpio/pinctrl: Add pin ranges before gpiochip

[Linus Walleij]

On Wed, Oct 30, 2019 at 5:01 PM Hans de Goede <hdegoede@redhat.com> wrote:

> So I suggest that we revert the following commits (in revert order):
>
> 4c87540940cbc7ddbe9674087919c605fd5c2ef1 "gpio: merrifield: Move hardware initialization to callback"
> 6658f87f219427ee776c498e07c878eb5cad1be2 "gpio: merrifield: Restore use of irq_base"
> 8f86a5b4ad679e4836733b47414226074eee4e4d "gpio: merrifield: Pass irqchip when adding gpiochip"

I reverted these on my fixes branch and hope we can figure our stuff out in the
v5.5 or v5.6 kernels.

Yours,
Linus Walleij