Re: [PATCH 1/5] pinctrl: core: Use delayed work for hogs

[Geert Uytterhoeven]

Hi Tony,

On Tue, Dec 27, 2016 at 6:19 PM, Tony Lindgren <tony@xxxxxxxxxxx> wrote:
> 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@xxxxxxxxxxx>

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...

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

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

> @@ -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.

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

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds