* Geert Uytterhoeven [off-list ref] [170110 06:09]:

Hi Tony,

On Tue, Dec 27, 2016 at 6:19 PM, Tony Lindgren [off-list ref] wrote:

quoted

Having the pin control framework call pin controller functions
before it's probe has finished is not nice as the pin controller
device driver does not yet have struct pinctrl_dev handle.

Let's fix this issue by adding deferred work for late init. This is
needed to be able to add pinctrl generic helper functions that expect
to know struct pinctrl_dev handle. Note that we now need to call
create_pinctrl() directly as we don't want to add the pin controller
to the list of controllers until the hogs are claimed. We also need
to pass the pinctrl_dev to the device tree parser functions as they
otherwise won't find the right controller at this point.

Signed-off-by: Tony Lindgren <tony@atomide.com>

I believe this patch causes a regression on r8a7740/armadillo, where the
pin controller is also a GPIO controller, and lcd0 needs a hog
(cfr. arch/arm/boot/dts/r8a7740-armadillo800eva.dts):

-GPIO line 176 (lcd0) hogged as output/high
-sh-pfc e6050000.pfc: r8a7740_pfc handling gpio 0 -> 211
+gpiochip_add_data: GPIOs 0..211 (r8a7740_pfc) failed to register
+sh-pfc e6050000.pfc: failed to init GPIO chip, ignoring...
 sh-pfc e6050000.pfc: r8a7740_pfc support registered

Hence all drivers using GPIOs fail to initialize because their GPIOs never
become available.

Adding debug prints to the failure paths shows that the call to
of_pinctrl_get() in of_gpiochip_add_pin_range() fails with -EPROBE_DEFER.
Adding a debug print to the top of gpiochip_add_data() makes the problem go
away, presumably because it introduces a delay that allows the delayed work
to kick in...

OK. What if we added also an optional pinctrl function that the pin
controller driver could call to initialize hogs? Then the pin controller
driver could call it during or after probe as needed. That is after
there's a valid struct pinctrl_dev handle.

Jon's fix ("pinctrl: core: Fix panic when pinctrl devices with hogs are
unregistered") doesn't help, as it affects unregistration only.

quoted

--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c

quoted

@@ -1800,32 +1847,10 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
                goto out_err;
        }

-       mutex_lock(&pinctrldev_list_mutex);
-       list_add_tail(&pctldev->node, &pinctrldev_list);
-       mutex_unlock(&pinctrldev_list_mutex);
-
-       pctldev->p = pinctrl_get(pctldev->dev);
-
-       if (!IS_ERR(pctldev->p)) {
-               pctldev->hog_default =
-                       pinctrl_lookup_state(pctldev->p, PINCTRL_STATE_DEFAULT);
-               if (IS_ERR(pctldev->hog_default)) {
-                       dev_dbg(dev, "failed to lookup the default state\n");
-               } else {
-                       if (pinctrl_select_state(pctldev->p,
-                                               pctldev->hog_default))
-                               dev_err(dev,
-                                       "failed to select default state\n");
-               }
-
-               pctldev->hog_sleep =
-                       pinctrl_lookup_state(pctldev->p,
-                                                   PINCTRL_STATE_SLEEP);
-               if (IS_ERR(pctldev->hog_sleep))
-                       dev_dbg(dev, "failed to lookup the sleep state\n");
-       }
-
-       pinctrl_init_device_debugfs(pctldev);
+       if (pinctrl_dt_has_hogs(pctldev))

Changing the above line to "if (0 && pinctrl_dt_has_hogs(pctldev))"
fixes the issue for me.

quoted

+               schedule_delayed_work(&pctldev->late_init, 0);
+       else
+               pinctrl_late_init(&pctldev->late_init.work);

        return pctldev;

We could also pass some flag if should always call pinctrl_late_init()
directly. But that does not remove the problem of struct pinctrl_dev handle
being uninitialized when the pin controller driver functionas are called.

Regards,

Tony