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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxx>
Cc: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
Reported-by: Hans de Goede <hdegoede@xxxxxxxxxx>
Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
---
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,






[Index of Archives]     [Linux SPI]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]

  Powered by Linux