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

Tony Lindgren committed 9 years ago

Commit 950b0d91 parent 56d9e4a8 files · +165 −79Browse files

Unified · Split

8 files

Documentation/pinctrl.txt +1 −3
drivers/pinctrl/core.c +139 −62
drivers/pinctrl/core.h +0 −2
drivers/pinctrl/freescale/pinctrl-imx.c +4 −4
drivers/pinctrl/pinctrl-single.c +2 −3
drivers/pinctrl/sh-pfc/pinctrl.c +2 −2
drivers/pinctrl/ti/pinctrl-ti-iodelay.c +2 −3
include/linux/pinctrl/pinctrl.h +15 −0

Documentation/pinctrl.txt+1−3

@@ -79,9 +79,7 @@ int __init foo_probe(void) · 78 unmodified lines
79	79	{
80	80	struct pinctrl_dev *pctl;
81	81
82		− pctl = pinctrl_register(&foo_desc, <PARENT>, NULL);
83		− if (!pctl)
84		− pr_err("could not register foo pin driver\n");
	82	+ return pinctrl_register_and_init(&foo_desc, <PARENT>, NULL, &pctl);
85	83	}
86	84
87	85	To enable the pinctrl subsystem and the subgroups for PINMUX and PINCONF and

drivers/pinctrl/core.c+139−62

@@ -1905,59 +1905,14 @@ static int pinctrl_check_ops(struct pinctrl_dev *pctldev) · 1904 unmodified lines
1905	1905	}
1906	1906
1907	1907	/**
1908		− * pinctrl_late_init() - finish pin controller device registration
1909		− * @work: work struct
1910		− */
1911		−static void pinctrl_late_init(struct work_struct *work)
1912		−{
1913		− struct pinctrl_dev *pctldev;
1914		−
1915		− pctldev = container_of(work, struct pinctrl_dev, late_init.work);
1916		−
1917		− /*
1918		− * If the pin controller does NOT have hogs, this will report an
1919		− * error and we skip over this entire branch. This is why we can
1920		− * call this function directly when we do not have hogs on the
1921		− * device.
1922		− */
1923		− pctldev->p = create_pinctrl(pctldev->dev, pctldev);
1924		− if (!IS_ERR(pctldev->p)) {
1925		− kref_get(&pctldev->p->users);
1926		− pctldev->hog_default =
1927		− pinctrl_lookup_state(pctldev->p, PINCTRL_STATE_DEFAULT);
1928		− if (IS_ERR(pctldev->hog_default)) {
1929		− dev_dbg(pctldev->dev,
1930		− "failed to lookup the default state\n");
1931		− } else {
1932		− if (pinctrl_select_state(pctldev->p,
1933		− pctldev->hog_default))
1934		− dev_err(pctldev->dev,
1935		− "failed to select default state\n");
1936		− }
1937		−
1938		− pctldev->hog_sleep =
1939		− pinctrl_lookup_state(pctldev->p,
1940		− PINCTRL_STATE_SLEEP);
1941		− if (IS_ERR(pctldev->hog_sleep))
1942		− dev_dbg(pctldev->dev,
1943		− "failed to lookup the sleep state\n");
1944		− }
1945		−
1946		− mutex_lock(&pinctrldev_list_mutex);
1947		− list_add_tail(&pctldev->node, &pinctrldev_list);
1948		− mutex_unlock(&pinctrldev_list_mutex);
1949		−
1950		− pinctrl_init_device_debugfs(pctldev);
1951		−}
1952		−
1953		−/**
1954		− * pinctrl_register() - register a pin controller device
	1908	+ * pinctrl_init_controller() - init a pin controller device
1955	1909	* @pctldesc: descriptor for this pin controller
1956	1910	* @dev: parent device for this pin controller
1957	1911	* @driver_data: private pin controller data for this pin controller
1958	1912	*/
1959		−struct pinctrl_dev pinctrl_register(struct pinctrl_desc pctldesc,
1960		− struct device dev, void driver_data)
	1913	+struct pinctrl_dev pinctrl_init_controller(struct pinctrl_desc pctldesc,
	1914	+ struct device *dev,
	1915	+ void *driver_data)
1961	1916	{
1962	1917	struct pinctrl_dev *pctldev;
1963	1918	int ret;
@@ -1983,7 +1938,6 @@ struct pinctrl_dev pinctrl_register(struct pinctrl_desc pctldesc, · 19 unmodified lines
1983	1938	INIT_RADIX_TREE(&pctldev->pin_function_tree, GFP_KERNEL);
1984	1939	#endif
1985	1940	INIT_LIST_HEAD(&pctldev->gpio_ranges);
1986		− INIT_DELAYED_WORK(&pctldev->late_init, pinctrl_late_init);
1987	1941	pctldev->dev = dev;
1988	1942	mutex_init(&pctldev->mutex);
1989	1943
@@ -2018,17 +1972,6 @@ struct pinctrl_dev pinctrl_register(struct pinctrl_desc pctldesc, · 28 unmodified lines
2018	1972	goto out_err;
2019	1973	}
2020	1974
2021		− /*
2022		− * If the device has hogs we want the probe() function of the driver
2023		− * to complete before we go in and hog them and add the pin controller
2024		− * to the list of controllers. If it has no hogs, we can just complete
2025		− * the registration immediately.
2026		− */
2027		− if (pinctrl_dt_has_hogs(pctldev))
2028		− schedule_delayed_work(&pctldev->late_init, 0);
2029		− else
2030		− pinctrl_late_init(&pctldev->late_init.work);
2031		−
2032	1975	return pctldev;
2033	1976
2034	1977	out_err:
@@ -2036,8 +1979,107 @@ out_err: · 1 unmodified line
2036	1979	kfree(pctldev);
2037	1980	return ERR_PTR(ret);
2038	1981	}
	1982	+
	1983	+static int pinctrl_create_and_start(struct pinctrl_dev *pctldev)
	1984	+{
	1985	+ pctldev->p = create_pinctrl(pctldev->dev, pctldev);
	1986	+ if (!IS_ERR(pctldev->p)) {
	1987	+ kref_get(&pctldev->p->users);
	1988	+ pctldev->hog_default =
	1989	+ pinctrl_lookup_state(pctldev->p, PINCTRL_STATE_DEFAULT);
	1990	+ if (IS_ERR(pctldev->hog_default)) {
	1991	+ dev_dbg(pctldev->dev,
	1992	+ "failed to lookup the default state\n");
	1993	+ } else {
	1994	+ if (pinctrl_select_state(pctldev->p,
	1995	+ pctldev->hog_default))
	1996	+ dev_err(pctldev->dev,
	1997	+ "failed to select default state\n");
	1998	+ }
	1999	+
	2000	+ pctldev->hog_sleep =
	2001	+ pinctrl_lookup_state(pctldev->p,
	2002	+ PINCTRL_STATE_SLEEP);
	2003	+ if (IS_ERR(pctldev->hog_sleep))
	2004	+ dev_dbg(pctldev->dev,
	2005	+ "failed to lookup the sleep state\n");
	2006	+ }
	2007	+
	2008	+ mutex_lock(&pinctrldev_list_mutex);
	2009	+ list_add_tail(&pctldev->node, &pinctrldev_list);
	2010	+ mutex_unlock(&pinctrldev_list_mutex);
	2011	+
	2012	+ pinctrl_init_device_debugfs(pctldev);
	2013	+
	2014	+ return 0;
	2015	+}
	2016	+
	2017	+/**
	2018	+ * pinctrl_register() - register a pin controller device
	2019	+ * @pctldesc: descriptor for this pin controller
	2020	+ * @dev: parent device for this pin controller
	2021	+ * @driver_data: private pin controller data for this pin controller
	2022	+ *
	2023	+ * Note that pinctrl_register() is known to have problems as the pin
	2024	+ * controller driver functions are called before the driver has a
	2025	+ * struct pinctrl_dev handle. To avoid issues later on, please use the
	2026	+ * new pinctrl_register_and_init() below instead.
	2027	+ */
	2028	+struct pinctrl_dev pinctrl_register(struct pinctrl_desc pctldesc,
	2029	+ struct device dev, void driver_data)
	2030	+{
	2031	+ struct pinctrl_dev *pctldev;
	2032	+ int error;
	2033	+
	2034	+ pctldev = pinctrl_init_controller(pctldesc, dev, driver_data);
	2035	+ if (IS_ERR(pctldev))
	2036	+ return pctldev;
	2037	+
	2038	+ error = pinctrl_create_and_start(pctldev);
	2039	+ if (error) {
	2040	+ mutex_destroy(&pctldev->mutex);
	2041	+ kfree(pctldev);
	2042	+
	2043	+ return ERR_PTR(error);
	2044	+ }
	2045	+
	2046	+ return pctldev;
	2047	+
	2048	+}
2039	2049	EXPORT_SYMBOL_GPL(pinctrl_register);
2040	2050
	2051	+int pinctrl_register_and_init(struct pinctrl_desc *pctldesc,
	2052	+ struct device dev, void driver_data,
	2053	+ struct pinctrl_dev **pctldev)
	2054	+{
	2055	+ struct pinctrl_dev *p;
	2056	+ int error;
	2057	+
	2058	+ p = pinctrl_init_controller(pctldesc, dev, driver_data);
	2059	+ if (IS_ERR(p))
	2060	+ return PTR_ERR(p);
	2061	+
	2062	+ /*
	2063	+ * We have pinctrl_start() call functions in the pin controller
	2064	+ * driver with create_pinctrl() for at least dt_node_to_map(). So
	2065	+ * let's make sure pctldev is properly initialized for the
	2066	+ * pin controller driver before we do anything.
	2067	+ */
	2068	+ *pctldev = p;
	2069	+
	2070	+ error = pinctrl_create_and_start(p);
	2071	+ if (error) {
	2072	+ mutex_destroy(&p->mutex);
	2073	+ kfree(p);
	2074	+ *pctldev = NULL;
	2075	+
	2076	+ return error;
	2077	+ }
	2078	+
	2079	+ return 0;
	2080	+}
	2081	+EXPORT_SYMBOL_GPL(pinctrl_register_and_init);
	2082	+
2041	2083	/**
2042	2084	* pinctrl_unregister() - unregister pinmux
2043	2085	* @pctldev: pin controller to unregister
@@ -2052,7 +2094,6 @@ void pinctrl_unregister(struct pinctrl_dev *pctldev) · 8 unmodified lines
2052	2094	if (pctldev == NULL)
2053	2095	return;
2054	2096
2055		− cancel_delayed_work_sync(&pctldev->late_init);
2056	2097	mutex_lock(&pctldev->mutex);
2057	2098	pinctrl_remove_device_debugfs(pctldev);
2058	2099	mutex_unlock(&pctldev->mutex);
@@ -2133,6 +2174,42 @@ struct pinctrl_dev devm_pinctrl_register(struct device dev, · 74 unmodified lines
2133	2174	}
2134	2175	EXPORT_SYMBOL_GPL(devm_pinctrl_register);
2135	2176
	2177	+/**
	2178	+ * devm_pinctrl_register_and_init() - Resource managed pinctrl register and init
	2179	+ * @dev: parent device for this pin controller
	2180	+ * @pctldesc: descriptor for this pin controller
	2181	+ * @driver_data: private pin controller data for this pin controller
	2182	+ *
	2183	+ * Returns an error pointer if pincontrol register failed. Otherwise
	2184	+ * it returns valid pinctrl handle.
	2185	+ *
	2186	+ * The pinctrl device will be automatically released when the device is unbound.
	2187	+ */
	2188	+int devm_pinctrl_register_and_init(struct device *dev,
	2189	+ struct pinctrl_desc *pctldesc,
	2190	+ void *driver_data,
	2191	+ struct pinctrl_dev **pctldev)
	2192	+{
	2193	+ struct pinctrl_dev **ptr;
	2194	+ int error;
	2195	+
	2196	+ ptr = devres_alloc(devm_pinctrl_dev_release, sizeof(*ptr), GFP_KERNEL);
	2197	+ if (!ptr)
	2198	+ return -ENOMEM;
	2199	+
	2200	+ error = pinctrl_register_and_init(pctldesc, dev, driver_data, pctldev);
	2201	+ if (error) {
	2202	+ devres_free(ptr);
	2203	+ return error;
	2204	+ }
	2205	+
	2206	+ ptr = pctldev;
	2207	+ devres_add(dev, ptr);
	2208	+
	2209	+ return 0;
	2210	+}
	2211	+EXPORT_SYMBOL_GPL(devm_pinctrl_register_and_init);
	2212	+
2136	2213	/**
2137	2214	* devm_pinctrl_unregister() - Resource managed version of pinctrl_unregister().
2138	2215	* @dev: device for which which resource was allocated

drivers/pinctrl/core.h+0−2

@@ -37,7 +37,6 @@ struct pinctrl_gpio_range; · 36 unmodified lines
37	37	* @p: result of pinctrl_get() for this device
38	38	* @hog_default: default state for pins hogged by this device
39	39	* @hog_sleep: sleep state for pins hogged by this device
40		− * @late_init: delayed work for pin controller to finish registration
41	40	* @mutex: mutex taken on each pin controller specific action
42	41	* @device_root: debugfs root for this device
43	42	*/
@@ -60,7 +59,6 @@ struct pinctrl_dev { · 16 unmodified lines
60	59	struct pinctrl *p;
61	60	struct pinctrl_state *hog_default;
62	61	struct pinctrl_state *hog_sleep;
63		− struct delayed_work late_init;
64	62	struct mutex mutex;
65	63	#ifdef CONFIG_DEBUG_FS
66	64	struct dentry *device_root;

drivers/pinctrl/freescale/pinctrl-imx.c+4−4

@@ -774,11 +774,11 @@ int imx_pinctrl_probe(struct platform_device *pdev, · 773 unmodified lines
774	774	ipctl->info = info;
775	775	ipctl->dev = info->dev;
776	776	platform_set_drvdata(pdev, ipctl);
777		− ipctl->pctl = devm_pinctrl_register(&pdev->dev,
778		− imx_pinctrl_desc, ipctl);
779		− if (IS_ERR(ipctl->pctl)) {
	777	+ ret = devm_pinctrl_register_and_init(&pdev->dev,
	778	+ imx_pinctrl_desc, ipctl,
	779	+ &ipctl->pctl);
	780	+ if (ret) {
780	781	dev_err(&pdev->dev, "could not register IMX pinctrl driver\n");
781		− ret = PTR_ERR(ipctl->pctl);
782	782	goto free;
783	783	}
784	784

drivers/pinctrl/pinctrl-single.c+2−3

@@ -1747,10 +1747,9 @@ static int pcs_probe(struct platform_device *pdev) · 1746 unmodified lines
1747	1747	if (ret < 0)
1748	1748	goto free;
1749	1749
1750		− pcs->pctl = pinctrl_register(&pcs->desc, pcs->dev, pcs);
1751		− if (IS_ERR(pcs->pctl)) {
	1750	+ ret = pinctrl_register_and_init(&pcs->desc, pcs->dev, pcs, &pcs->pctl);
	1751	+ if (ret) {
1752	1752	dev_err(pcs->dev, "could not register single pinctrl driver\n");
1753		− ret = PTR_ERR(pcs->pctl);
1754	1753	goto free;
1755	1754	}
1756	1755

drivers/pinctrl/sh-pfc/pinctrl.c+2−2

@@ -816,6 +816,6 @@ int sh_pfc_register_pinctrl(struct sh_pfc *pfc) · 815 unmodified lines
816	816	pmx->pctl_desc.pins = pmx->pins;
817	817	pmx->pctl_desc.npins = pfc->info->nr_pins;
818	818
819		− pmx->pctl = devm_pinctrl_register(pfc->dev, &pmx->pctl_desc, pmx);
820		− return PTR_ERR_OR_ZERO(pmx->pctl);
	819	+ return devm_pinctrl_register_and_init(pfc->dev, &pmx->pctl_desc, pmx,
	820	+ &pmx->pctl);
821	821	}

drivers/pinctrl/ti/pinctrl-ti-iodelay.c+2−3

@@ -891,10 +891,9 @@ static int ti_iodelay_probe(struct platform_device *pdev) · 890 unmodified lines
891	891	iod->desc.name = dev_name(dev);
892	892	iod->desc.owner = THIS_MODULE;
893	893
894		− iod->pctl = pinctrl_register(&iod->desc, dev, iod);
895		− if (!iod->pctl) {
	894	+ ret = pinctrl_register_and_init(&iod->desc, dev, iod, &iod->pctl);
	895	+ if (ret) {
896	896	dev_err(dev, "Failed to register pinctrl\n");
897		− ret = -ENODEV;
898	897	goto exit_out;
899	898	}
900	899

include/linux/pinctrl/pinctrl.h+15−0

@@ -141,12 +141,27 @@ struct pinctrl_desc { · 140 unmodified lines
141	141	};
142	142
143	143	/* External interface to pin controller */
	144	+
	145	+extern int pinctrl_register_and_init(struct pinctrl_desc *pctldesc,
	146	+ struct device dev, void driver_data,
	147	+ struct pinctrl_dev **pctldev);
	148	+
	149	+/* Please use pinctrl_register_and_init() instead */
144	150	extern struct pinctrl_dev pinctrl_register(struct pinctrl_desc pctldesc,
145	151	struct device dev, void driver_data);
	152	+
146	153	extern void pinctrl_unregister(struct pinctrl_dev *pctldev);
	154	+
	155	+extern int devm_pinctrl_register_and_init(struct device *dev,
	156	+ struct pinctrl_desc *pctldesc,
	157	+ void *driver_data,
	158	+ struct pinctrl_dev **pctldev);
	159	+
	160	+/* Please use devm_pinctrl_register_and_init() instead */
147	161	extern struct pinctrl_dev devm_pinctrl_register(struct device dev,
148	162	struct pinctrl_desc *pctldesc,
149	163	void *driver_data);
	164	+
150	165	extern void devm_pinctrl_unregister(struct device *dev,
151	166	struct pinctrl_dev *pctldev);
152	167

pinctrl: core: Fix regression caused by delayed work for hogs

Keyboard shortcuts

General

Issues and pull requests

Issues, pull requests and Inbox

Repository code