From 950b0d91dc108f54bccca5a2f75bb46f2df63d29 Mon Sep 17 00:00:00 2001 From: Tony Lindgren Date: Wed, 11 Jan 2017 14:13:34 -0800 Subject: [PATCH] pinctrl: core: Fix regression caused by delayed work for hogs 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 . 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 , 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 Cc: Gary Bisson Signed-off-by: Tony Lindgren Tested-by: Geert Uytterhoeven Signed-off-by: Linus Walleij --- Documentation/pinctrl.txt | 4 +- drivers/pinctrl/core.c | 201 ++++++++++++++++-------- drivers/pinctrl/core.h | 2 - drivers/pinctrl/freescale/pinctrl-imx.c | 8 +- drivers/pinctrl/pinctrl-single.c | 5 +- drivers/pinctrl/sh-pfc/pinctrl.c | 4 +- drivers/pinctrl/ti/pinctrl-ti-iodelay.c | 5 +- include/linux/pinctrl/pinctrl.h | 15 ++ 8 files changed, 165 insertions(+), 79 deletions(-) diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt index 0d3b9ce0a0b9..54bd5faa8782 100644 --- a/Documentation/pinctrl.txt +++ b/Documentation/pinctrl.txt @@ -79,9 +79,7 @@ int __init foo_probe(void) { struct pinctrl_dev *pctl; - pctl = pinctrl_register(&foo_desc, , NULL); - if (!pctl) - pr_err("could not register foo pin driver\n"); + return pinctrl_register_and_init(&foo_desc, , NULL, &pctl); } To enable the pinctrl subsystem and the subgroups for PINMUX and PINCONF and diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c index 65c0ae0969dc..bc73a63163ee 100644 --- a/drivers/pinctrl/core.c +++ b/drivers/pinctrl/core.c @@ -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 diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h index ad812a2d7248..1c35de59a658 100644 --- a/drivers/pinctrl/core.h +++ b/drivers/pinctrl/core.h @@ -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; diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c index bccd9416d44f..a7ace9e1ad81 100644 --- a/drivers/pinctrl/freescale/pinctrl-imx.c +++ b/drivers/pinctrl/freescale/pinctrl-imx.c @@ -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; } diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index 8408263de466..d23e0e0aeb22 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -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; } diff --git a/drivers/pinctrl/sh-pfc/pinctrl.c b/drivers/pinctrl/sh-pfc/pinctrl.c index fcacfa73ef6e..08150a321be6 100644 --- a/drivers/pinctrl/sh-pfc/pinctrl.c +++ b/drivers/pinctrl/sh-pfc/pinctrl.c @@ -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); } diff --git a/drivers/pinctrl/ti/pinctrl-ti-iodelay.c b/drivers/pinctrl/ti/pinctrl-ti-iodelay.c index 3b86d3db7358..7f472f123515 100644 --- a/drivers/pinctrl/ti/pinctrl-ti-iodelay.c +++ b/drivers/pinctrl/ti/pinctrl-ti-iodelay.c @@ -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; } diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h index a42e57da270d..8ce2d87a238b 100644 --- a/include/linux/pinctrl/pinctrl.h +++ b/include/linux/pinctrl/pinctrl.h @@ -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); -- 2.20.1