pinctrl: core: Fix regression caused by delayed work for hogs
authorTony Lindgren <tony@atomide.com>
Wed, 11 Jan 2017 22:13:34 +0000 (14:13 -0800)
committerLinus Walleij <linus.walleij@linaro.org>
Fri, 13 Jan 2017 15:25:17 +0000 (16:25 +0100)
Commit df61b366af26 ("pinctrl: core: Use delayed work for hogs") caused a
regression at least with sh-pfc that is also a GPIO controller as
noted by Geert Uytterhoeven <geert@linux-m68k.org>.

As the original pinctrl_register() has issues calling pin controller
driver functions early before the controller has finished registering,
we can't just revert commit df61b366af26. That would break the drivers
using GENERIC_PINCTRL_GROUPS or GENERIC_PINMUX_FUNCTIONS.

So let's fix the issue with the following steps as a single patch:

1. Revert the late_init parts of commit df61b366af26.

   The late_init clearly won't work and we have to just give up
   on fixing pinctrl_register() for GENERIC_PINCTRL_GROUPS and
   GENERIC_PINMUX_FUNCTIONS.

2. Split pinctrl_register() into two parts

   By splitting pinctrl_register() into pinctrl_init_controller()
   and pinctrl_create_and_start() we have better control over when
   it's safe to call pinctrl_create().

3. Introduce a new pinctrl_register_and_init() function

   As suggested by Linus Walleij <linus.walleij@linaro.org>, we
   can just introduce a new function for the controllers that need
   pinctrl_create() called later.

4. Convert the four known problem cases to use new function

   Let's convert pinctrl-imx, pinctrl-single, sh-pfc and ti-iodelay
   to use the new function to fix the issues. The rest of the drivers
   can be converted later. Let's also update Documentation/pinctrl.txt
   accordingly because of the known issues with pinctrl_register().

Fixes: df61b366af26 ("pinctrl: core: Use delayed work for hogs")
Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Gary Bisson <gary.bisson@boundarydevices.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
Documentation/pinctrl.txt
drivers/pinctrl/core.c
drivers/pinctrl/core.h
drivers/pinctrl/freescale/pinctrl-imx.c
drivers/pinctrl/pinctrl-single.c
drivers/pinctrl/sh-pfc/pinctrl.c
drivers/pinctrl/ti/pinctrl-ti-iodelay.c
include/linux/pinctrl/pinctrl.h

index 0d3b9ce0a0b90575fbeff83bec518139ae609154..54bd5faa8782258f72b134f1aaca684c15a00eb6 100644 (file)
@@ -79,9 +79,7 @@ int __init foo_probe(void)
 {
        struct pinctrl_dev *pctl;
 
-       pctl = pinctrl_register(&foo_desc, <PARENT>, NULL);
-       if (!pctl)
-               pr_err("could not register foo pin driver\n");
+       return pinctrl_register_and_init(&foo_desc, <PARENT>, NULL, &pctl);
 }
 
 To enable the pinctrl subsystem and the subgroups for PINMUX and PINCONF and
index 65c0ae0969dc8d004c9600785a09353f10fddc09..bc73a63163ee2fb66d280f71cc2bac8005e716e1 100644 (file)
@@ -1905,59 +1905,14 @@ static int pinctrl_check_ops(struct pinctrl_dev *pctldev)
 }
 
 /**
- * pinctrl_late_init() - finish pin controller device registration
- * @work: work struct
- */
-static void pinctrl_late_init(struct work_struct *work)
-{
-       struct pinctrl_dev *pctldev;
-
-       pctldev = container_of(work, struct pinctrl_dev, late_init.work);
-
-       /*
-        * If the pin controller does NOT have hogs, this will report an
-        * error and we skip over this entire branch. This is why we can
-        * call this function directly when we do not have hogs on the
-        * device.
-        */
-       pctldev->p = create_pinctrl(pctldev->dev, pctldev);
-       if (!IS_ERR(pctldev->p)) {
-               kref_get(&pctldev->p->users);
-               pctldev->hog_default =
-                       pinctrl_lookup_state(pctldev->p, PINCTRL_STATE_DEFAULT);
-               if (IS_ERR(pctldev->hog_default)) {
-                       dev_dbg(pctldev->dev,
-                               "failed to lookup the default state\n");
-               } else {
-                       if (pinctrl_select_state(pctldev->p,
-                                                pctldev->hog_default))
-                               dev_err(pctldev->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(pctldev->dev,
-                               "failed to lookup the sleep state\n");
-       }
-
-       mutex_lock(&pinctrldev_list_mutex);
-       list_add_tail(&pctldev->node, &pinctrldev_list);
-       mutex_unlock(&pinctrldev_list_mutex);
-
-       pinctrl_init_device_debugfs(pctldev);
-}
-
-/**
- * pinctrl_register() - register a pin controller device
+ * pinctrl_init_controller() - init a pin controller device
  * @pctldesc: descriptor for this pin controller
  * @dev: parent device for this pin controller
  * @driver_data: private pin controller data for this pin controller
  */
-struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
-                                   struct device *dev, void *driver_data)
+struct pinctrl_dev *pinctrl_init_controller(struct pinctrl_desc *pctldesc,
+                                           struct device *dev,
+                                           void *driver_data)
 {
        struct pinctrl_dev *pctldev;
        int ret;
@@ -1983,7 +1938,6 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
        INIT_RADIX_TREE(&pctldev->pin_function_tree, GFP_KERNEL);
 #endif
        INIT_LIST_HEAD(&pctldev->gpio_ranges);
-       INIT_DELAYED_WORK(&pctldev->late_init, pinctrl_late_init);
        pctldev->dev = dev;
        mutex_init(&pctldev->mutex);
 
@@ -2018,17 +1972,6 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
                goto out_err;
        }
 
-       /*
-        * If the device has hogs we want the probe() function of the driver
-        * to complete before we go in and hog them and add the pin controller
-        * to the list of controllers. If it has no hogs, we can just complete
-        * the registration immediately.
-        */
-       if (pinctrl_dt_has_hogs(pctldev))
-               schedule_delayed_work(&pctldev->late_init, 0);
-       else
-               pinctrl_late_init(&pctldev->late_init.work);
-
        return pctldev;
 
 out_err:
@@ -2036,8 +1979,107 @@ out_err:
        kfree(pctldev);
        return ERR_PTR(ret);
 }
+
+static int pinctrl_create_and_start(struct pinctrl_dev *pctldev)
+{
+       pctldev->p = create_pinctrl(pctldev->dev, pctldev);
+       if (!IS_ERR(pctldev->p)) {
+               kref_get(&pctldev->p->users);
+               pctldev->hog_default =
+                       pinctrl_lookup_state(pctldev->p, PINCTRL_STATE_DEFAULT);
+               if (IS_ERR(pctldev->hog_default)) {
+                       dev_dbg(pctldev->dev,
+                               "failed to lookup the default state\n");
+               } else {
+                       if (pinctrl_select_state(pctldev->p,
+                                               pctldev->hog_default))
+                               dev_err(pctldev->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(pctldev->dev,
+                               "failed to lookup the sleep state\n");
+       }
+
+       mutex_lock(&pinctrldev_list_mutex);
+       list_add_tail(&pctldev->node, &pinctrldev_list);
+       mutex_unlock(&pinctrldev_list_mutex);
+
+       pinctrl_init_device_debugfs(pctldev);
+
+       return 0;
+}
+
+/**
+ * pinctrl_register() - register a pin controller device
+ * @pctldesc: descriptor for this pin controller
+ * @dev: parent device for this pin controller
+ * @driver_data: private pin controller data for this pin controller
+ *
+ * Note that pinctrl_register() is known to have problems as the pin
+ * controller driver functions are called before the driver has a
+ * struct pinctrl_dev handle. To avoid issues later on, please use the
+ * new pinctrl_register_and_init() below instead.
+ */
+struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
+                                   struct device *dev, void *driver_data)
+{
+       struct pinctrl_dev *pctldev;
+       int error;
+
+       pctldev = pinctrl_init_controller(pctldesc, dev, driver_data);
+       if (IS_ERR(pctldev))
+               return pctldev;
+
+       error = pinctrl_create_and_start(pctldev);
+       if (error) {
+               mutex_destroy(&pctldev->mutex);
+               kfree(pctldev);
+
+               return ERR_PTR(error);
+       }
+
+       return pctldev;
+
+}
 EXPORT_SYMBOL_GPL(pinctrl_register);
 
+int pinctrl_register_and_init(struct pinctrl_desc *pctldesc,
+                             struct device *dev, void *driver_data,
+                             struct pinctrl_dev **pctldev)
+{
+       struct pinctrl_dev *p;
+       int error;
+
+       p = pinctrl_init_controller(pctldesc, dev, driver_data);
+       if (IS_ERR(p))
+               return PTR_ERR(p);
+
+       /*
+        * We have pinctrl_start() call functions in the pin controller
+        * driver with create_pinctrl() for at least dt_node_to_map(). So
+        * let's make sure pctldev is properly initialized for the
+        * pin controller driver before we do anything.
+        */
+       *pctldev = p;
+
+       error = pinctrl_create_and_start(p);
+       if (error) {
+               mutex_destroy(&p->mutex);
+               kfree(p);
+               *pctldev = NULL;
+
+               return error;
+       }
+
+       return 0;
+}
+EXPORT_SYMBOL_GPL(pinctrl_register_and_init);
+
 /**
  * pinctrl_unregister() - unregister pinmux
  * @pctldev: pin controller to unregister
@@ -2052,7 +2094,6 @@ void pinctrl_unregister(struct pinctrl_dev *pctldev)
        if (pctldev == NULL)
                return;
 
-       cancel_delayed_work_sync(&pctldev->late_init);
        mutex_lock(&pctldev->mutex);
        pinctrl_remove_device_debugfs(pctldev);
        mutex_unlock(&pctldev->mutex);
@@ -2133,6 +2174,42 @@ struct pinctrl_dev *devm_pinctrl_register(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_pinctrl_register);
 
+/**
+ * devm_pinctrl_register_and_init() - Resource managed pinctrl register and init
+ * @dev: parent device for this pin controller
+ * @pctldesc: descriptor for this pin controller
+ * @driver_data: private pin controller data for this pin controller
+ *
+ * Returns an error pointer if pincontrol register failed. Otherwise
+ * it returns valid pinctrl handle.
+ *
+ * The pinctrl device will be automatically released when the device is unbound.
+ */
+int devm_pinctrl_register_and_init(struct device *dev,
+                                  struct pinctrl_desc *pctldesc,
+                                  void *driver_data,
+                                  struct pinctrl_dev **pctldev)
+{
+       struct pinctrl_dev **ptr;
+       int error;
+
+       ptr = devres_alloc(devm_pinctrl_dev_release, sizeof(*ptr), GFP_KERNEL);
+       if (!ptr)
+               return -ENOMEM;
+
+       error = pinctrl_register_and_init(pctldesc, dev, driver_data, pctldev);
+       if (error) {
+               devres_free(ptr);
+               return error;
+       }
+
+       *ptr = *pctldev;
+       devres_add(dev, ptr);
+
+       return 0;
+}
+EXPORT_SYMBOL_GPL(devm_pinctrl_register_and_init);
+
 /**
  * devm_pinctrl_unregister() - Resource managed version of pinctrl_unregister().
  * @dev: device for which which resource was allocated
index ad812a2d724880c86316c6a53ed434ccee4b42fd..1c35de59a658d44a880ff8ffec50c89c18d7466f 100644 (file)
@@ -37,7 +37,6 @@ struct pinctrl_gpio_range;
  * @p: result of pinctrl_get() for this device
  * @hog_default: default state for pins hogged by this device
  * @hog_sleep: sleep state for pins hogged by this device
- * @late_init: delayed work for pin controller to finish registration
  * @mutex: mutex taken on each pin controller specific action
  * @device_root: debugfs root for this device
  */
@@ -60,7 +59,6 @@ struct pinctrl_dev {
        struct pinctrl *p;
        struct pinctrl_state *hog_default;
        struct pinctrl_state *hog_sleep;
-       struct delayed_work late_init;
        struct mutex mutex;
 #ifdef CONFIG_DEBUG_FS
        struct dentry *device_root;
index bccd9416d44fca3974f907e533bda6d3ece6a396..a7ace9e1ad81f24f571d34ca155ba5d03c5f9a36 100644 (file)
@@ -774,11 +774,11 @@ int imx_pinctrl_probe(struct platform_device *pdev,
        ipctl->info = info;
        ipctl->dev = info->dev;
        platform_set_drvdata(pdev, ipctl);
-       ipctl->pctl = devm_pinctrl_register(&pdev->dev,
-                                           imx_pinctrl_desc, ipctl);
-       if (IS_ERR(ipctl->pctl)) {
+       ret = devm_pinctrl_register_and_init(&pdev->dev,
+                                            imx_pinctrl_desc, ipctl,
+                                            &ipctl->pctl);
+       if (ret) {
                dev_err(&pdev->dev, "could not register IMX pinctrl driver\n");
-               ret = PTR_ERR(ipctl->pctl);
                goto free;
        }
 
index 8408263de46617b0c8a8dc26e8b02cdff6fc8f9a..d23e0e0aeb224376c28e7dfbb81404ebe693cc3c 100644 (file)
@@ -1747,10 +1747,9 @@ static int pcs_probe(struct platform_device *pdev)
        if (ret < 0)
                goto free;
 
-       pcs->pctl = pinctrl_register(&pcs->desc, pcs->dev, pcs);
-       if (IS_ERR(pcs->pctl)) {
+       ret = pinctrl_register_and_init(&pcs->desc, pcs->dev, pcs, &pcs->pctl);
+       if (ret) {
                dev_err(pcs->dev, "could not register single pinctrl driver\n");
-               ret = PTR_ERR(pcs->pctl);
                goto free;
        }
 
index fcacfa73ef6e9b657fb043186fcd6c9cd0d2dfeb..08150a321be6f1e2df11fce59745430b1ead1213 100644 (file)
@@ -816,6 +816,6 @@ int sh_pfc_register_pinctrl(struct sh_pfc *pfc)
        pmx->pctl_desc.pins = pmx->pins;
        pmx->pctl_desc.npins = pfc->info->nr_pins;
 
-       pmx->pctl = devm_pinctrl_register(pfc->dev, &pmx->pctl_desc, pmx);
-       return PTR_ERR_OR_ZERO(pmx->pctl);
+       return devm_pinctrl_register_and_init(pfc->dev, &pmx->pctl_desc, pmx,
+                                             &pmx->pctl);
 }
index 3b86d3db7358f01f5822316a8d49d4b87c4af959..7f472f123515c8b3b611fbd50b12dad29b0e0694 100644 (file)
@@ -891,10 +891,9 @@ static int ti_iodelay_probe(struct platform_device *pdev)
        iod->desc.name = dev_name(dev);
        iod->desc.owner = THIS_MODULE;
 
-       iod->pctl = pinctrl_register(&iod->desc, dev, iod);
-       if (!iod->pctl) {
+       ret = pinctrl_register_and_init(&iod->desc, dev, iod, &iod->pctl);
+       if (ret) {
                dev_err(dev, "Failed to register pinctrl\n");
-               ret = -ENODEV;
                goto exit_out;
        }
 
index a42e57da270dcf267e6fce79bb2a35b1a7f01b38..8ce2d87a238b84d432abca342f9920e42a8d0c42 100644 (file)
@@ -141,12 +141,27 @@ struct pinctrl_desc {
 };
 
 /* External interface to pin controller */
+
+extern int pinctrl_register_and_init(struct pinctrl_desc *pctldesc,
+                                    struct device *dev, void *driver_data,
+                                    struct pinctrl_dev **pctldev);
+
+/* Please use pinctrl_register_and_init() instead */
 extern struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
                                struct device *dev, void *driver_data);
+
 extern void pinctrl_unregister(struct pinctrl_dev *pctldev);
+
+extern int devm_pinctrl_register_and_init(struct device *dev,
+                               struct pinctrl_desc *pctldesc,
+                               void *driver_data,
+                               struct pinctrl_dev **pctldev);
+
+/* Please use devm_pinctrl_register_and_init() instead */
 extern struct pinctrl_dev *devm_pinctrl_register(struct device *dev,
                                struct pinctrl_desc *pctldesc,
                                void *driver_data);
+
 extern void devm_pinctrl_unregister(struct device *dev,
                                struct pinctrl_dev *pctldev);