Re: [PATCH 18/23] gpio: nomadik: support mobileye,eyeq5-gpio

[Linus Walleij <linus.walleij@linaro.org>]

On Wed, Feb 21, 2024 at 5:16 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote:

> > Trying to figure it out...
>
> Can I help in the debugging process?

Nah, I found it :)

> Reading the code once again I'd guess
> of_device_get_match_data(&gpio_pdev->dev) could be the root cause. We
> are accessing match data for the GPIO device while probing the pinctrl
> device. Maybe something isn't initialised properly yet? The rest looks
> rather harmless, I've checked all conditional expressions.

Yep spot on. The nmk_gpio_populate_chip() is sometimes called from
the pinctrl driver before the gpio probe() has been called, so the match
data is NULL and we crash.

This looks like it does for historical reasons and there could be better
ways to fix it now that Saravana Kannan has fixed up the probe ordering
code.

The following is one way to fix it for now using device_is_compatible()
(illustrating some other changes I did as well):

diff --git a/drivers/gpio/gpio-nomadik.c b/drivers/gpio/gpio-nomadik.c
index 21bb6d6363fc..11071a982ebb 100644
--- a/drivers/gpio/gpio-nomadik.c
+++ b/drivers/gpio/gpio-nomadik.c
@@ -27,6 +27,7 @@
 #include <linux/of_platform.h>
 #include <linux/pinctrl/pinctrl.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
 #include <linux/reset.h>
 #include <linux/seq_file.h>
 #include <linux/types.h>
@@ -37,15 +38,13 @@
 static DEFINE_SPINLOCK(nmk_gpio_slpm_lock);
 #endif

-#define NMK_GPIO_FLAG_QUIRK_MBLY    BIT(0)
-
 void __nmk_gpio_set_slpm(struct nmk_gpio_chip *nmk_chip, unsigned int offset,
              enum nmk_gpio_slpm mode)
 {
     u32 slpm;

     /* We should NOT have been called. */
-    if (WARN_ON(nmk_chip->quirk_mbly))
+    if (WARN_ON(nmk_chip->is_mobileye_soc))
         return;

     slpm = readl(nmk_chip->addr + NMK_GPIO_SLPC);
@@ -105,7 +104,7 @@ static void __nmk_gpio_irq_modify(struct
nmk_gpio_chip *nmk_chip,
         fimscval = &nmk_chip->fimsc;
     } else  {
         /* We should NOT have been called. */
-        if (WARN_ON(nmk_chip->quirk_mbly))
+        if (WARN_ON(nmk_chip->is_mobileye_soc))
             return;
         rimscreg = NMK_GPIO_RWIMSC;
         fimscreg = NMK_GPIO_FWIMSC;
@@ -134,7 +133,7 @@ static void __nmk_gpio_set_wake(struct
nmk_gpio_chip *nmk_chip,
                 int offset, bool on)
 {
     /* We should NOT have been called. */
-    if (WARN_ON(nmk_chip->quirk_mbly))
+    if (WARN_ON(nmk_chip->is_mobileye_soc))
         return;

     /*
@@ -161,7 +160,7 @@ static void nmk_gpio_irq_maskunmask(struct
nmk_gpio_chip *nmk_chip,

     __nmk_gpio_irq_modify(nmk_chip, d->hwirq, NORMAL, enable);

-    if (!nmk_chip->quirk_mbly && !(nmk_chip->real_wake & BIT(d->hwirq)))
+    if (!nmk_chip->is_mobileye_soc && !(nmk_chip->real_wake & BIT(d->hwirq)))
         __nmk_gpio_set_wake(nmk_chip, d->hwirq, enable);

     spin_unlock(&nmk_chip->lock);
@@ -194,7 +193,7 @@ static int nmk_gpio_irq_set_wake(struct irq_data
*d, unsigned int on)
     unsigned long flags;

     /* Handler is registered in all cases. */
-    if (nmk_chip->quirk_mbly)
+    if (nmk_chip->is_mobileye_soc)
         return -ENXIO;

     clk_enable(nmk_chip->clk);
@@ -235,7 +234,7 @@ static int nmk_gpio_irq_set_type(struct irq_data
*d, unsigned int type)
     if (enabled)
         __nmk_gpio_irq_modify(nmk_chip, d->hwirq, NORMAL, false);

-    if (!nmk_chip->quirk_mbly && (enabled || wake))
+    if (!nmk_chip->is_mobileye_soc && (enabled || wake))
         __nmk_gpio_irq_modify(nmk_chip, d->hwirq, WAKE, false);

     nmk_chip->edge_rising &= ~BIT(d->hwirq);
@@ -249,7 +248,7 @@ static int nmk_gpio_irq_set_type(struct irq_data
*d, unsigned int type)
     if (enabled)
         __nmk_gpio_irq_modify(nmk_chip, d->hwirq, NORMAL, true);

-    if (!nmk_chip->quirk_mbly && (enabled || wake))
+    if (!nmk_chip->is_mobileye_soc && (enabled || wake))
         __nmk_gpio_irq_modify(nmk_chip, d->hwirq, WAKE, true);

     spin_unlock_irqrestore(&nmk_chip->lock, flags);
@@ -383,7 +382,7 @@ static int nmk_gpio_get_mode(struct nmk_gpio_chip
*nmk_chip, int offset)
     u32 afunc, bfunc;

     /* We don't support modes. */
-    if (nmk_chip->quirk_mbly)
+    if (nmk_chip->is_mobileye_soc)
         return NMK_GPIO_ALT_GPIO;

     clk_enable(nmk_chip->clk);
@@ -517,7 +516,6 @@ struct nmk_gpio_chip
*nmk_gpio_populate_chip(struct device_node *np,
     struct resource *res;
     struct clk *clk;
     void __iomem *base;
-    uintptr_t flags;
     u32 id, ngpio;

     gpio_pdev = of_find_device_by_node(np);
@@ -551,8 +549,7 @@ struct nmk_gpio_chip
*nmk_gpio_populate_chip(struct device_node *np,
         dev_dbg(&pdev->dev, "populate: using default ngpio (%d)\n", ngpio);
     }

-    flags = (uintptr_t)of_device_get_match_data(&gpio_pdev->dev);
-    nmk_chip->quirk_mbly = !!(flags & NMK_GPIO_FLAG_QUIRK_MBLY);
+    nmk_chip->is_mobileye_soc = device_is_compatible(&gpio_pdev->dev,
"mobileye,eyeq5-gpio");
     nmk_chip->bank = id;
     chip = &nmk_chip->chip;
     chip->base = -1;
@@ -667,7 +664,7 @@ static int nmk_gpio_probe(struct platform_device *pdev)
         return ret;
     }

-    if (!nmk_chip->quirk_mbly) {
+    if (!nmk_chip->is_mobileye_soc) {
         clk_enable(nmk_chip->clk);
         nmk_chip->lowemi = readl_relaxed(nmk_chip->addr + NMK_GPIO_LOWEMI);
         clk_disable(nmk_chip->clk);
@@ -690,7 +687,6 @@ static const struct of_device_id nmk_gpio_match[] = {
     },
     {
         .compatible = "mobileye,eyeq5-gpio",
-        .data = (void*)NMK_GPIO_FLAG_QUIRK_MBLY,
     },
     {}
 };
diff --git a/include/linux/gpio/gpio-nomadik.h
b/include/linux/gpio/gpio-nomadik.h
index 8d0134dd3771..ede16cdaa920 100644
--- a/include/linux/gpio/gpio-nomadik.h
+++ b/include/linux/gpio/gpio-nomadik.h
@@ -51,6 +51,7 @@ enum nmk_gpio_slpm {

 struct nmk_gpio_chip {
     struct gpio_chip chip;
+    bool is_mobileye_soc;
     void __iomem *addr;
     struct clk *clk;
     unsigned int bank;

Yours,
Linus Walleij