Re: [REGRESSION] pinctrl, of, unable to find hogs

[Tony Lindgren <tony@atomide.com>]

* Tony Lindgren <tony@atomide.com> [170227 09:37]:
> * Gary Bisson <gary.bisson@boundarydevices.com> [170227 08:42]:
> > > Not sure how to fix it though since we can't move the dt probing before
> > > radix tree init.
> 
> Yup looks like we still have an issue with pinctrl driver functions
> getting called before driver probe has completed.
> 
> How about we introduce something like:
> 
> int pinctrl_claim_hogs(struct pinctrl_dev *pctldev);
> 
> Then the drivers can call that at the end of the probe after
> the pins have been parsed?
> 
> This should be safe as no other driver can claim the pins either
> before the pins have been parsed :)

Below is an initial take on this solution. I've only briefly tested
it so far but maybe give it a try and see if it helps.

I'll take a look if we can make the error handling better for
pinctrl_register_and_init().

Regards,

Tony

8< ---------------------------
>From tony Mon Sep 17 00:00:00 2001
From: Tony Lindgren <tony@atomide.com>
Date: Mon, 27 Feb 2017 10:15:22 -0800
Subject: [PATCH] Fix imx hogs

---
 drivers/pinctrl/core.c                  | 90 ++++++++++++++++++++++++---------
 drivers/pinctrl/freescale/pinctrl-imx.c |  6 +++
 drivers/pinctrl/pinctrl-single.c        |  6 +++
 drivers/pinctrl/sh-pfc/pinctrl.c        | 12 ++++-
 drivers/pinctrl/ti/pinctrl-ti-iodelay.c |  4 +-
 include/linux/pinctrl/pinctrl.h         |  1 +
 6 files changed, 93 insertions(+), 26 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -2010,30 +2010,14 @@ struct pinctrl_dev *pinctrl_init_controller(struct pinctrl_desc *pctldesc,
 	return ERR_PTR(ret);
 }
 
-static int pinctrl_create_and_start(struct pinctrl_dev *pctldev)
+static int pinctrl_create(struct pinctrl_dev *pctldev)
 {
+	/* REVISIT: Can we bail out on errors here? */
 	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");
-	}
+	if (IS_ERR(pctldev->p))
+		dev_warn(pctldev->dev,
+			 "creating incomplete pin controller: %li\n",
+			 PTR_ERR(pctldev->p));
 
 	mutex_lock(&pinctrldev_list_mutex);
 	list_add_tail(&pctldev->node, &pinctrldev_list);
@@ -2044,6 +2028,66 @@ static int pinctrl_create_and_start(struct pinctrl_dev *pctldev)
 	return 0;
 }
 
+int pinctrl_claim_hogs(struct pinctrl_dev *pctldev)
+{
+	if (IS_ERR(pctldev->p)) {
+		dev_err(pctldev->dev,
+			"claim hogs for incomplete pin controller?: %li\n",
+			PTR_ERR(pctldev->p));
+
+		return PTR_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");
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pinctrl_claim_hogs);
+
+/* REVISIT: Can we bail out on errors here? */
+static int pinctrl_create_and_start(struct pinctrl_dev *pctldev)
+{
+	int error;
+
+	error = pinctrl_create(pctldev);
+	if (error) {
+		dev_warn(pctldev->dev,
+			 "pin controller not claiming hogs: %i\n",
+			 error);
+
+		goto out_warn_only;
+	}
+
+	error = pinctrl_claim_hogs(pctldev);
+	if (!error)
+		return 0;
+
+out_warn_only:
+	dev_warn(pctldev->dev,
+		 "pin controller incomplete, hogs not claimed: %i\n",
+		 error);
+
+	return 0;
+}
+
 /**
  * pinctrl_register() - register a pin controller device
  * @pctldesc: descriptor for this pin controller
@@ -2097,7 +2141,7 @@ int pinctrl_register_and_init(struct pinctrl_desc *pctldesc,
 	 */
 	*pctldev = p;
 
-	error = pinctrl_create_and_start(p);
+	error = pinctrl_create(p);
 	if (error) {
 		mutex_destroy(&p->mutex);
 		kfree(p);
diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c
--- a/drivers/pinctrl/freescale/pinctrl-imx.c
+++ b/drivers/pinctrl/freescale/pinctrl-imx.c
@@ -788,6 +788,12 @@ int imx_pinctrl_probe(struct platform_device *pdev,
 		goto free;
 	}
 
+	ret = pinctrl_claim_hogs(ipctl->pctl);
+	if (ret) {
+		dev_err(&pdev->dev, "could not claim hogs: %i\n", ret);
+		goto free;
+	}
+
 	dev_info(&pdev->dev, "initialized IMX pinctrl driver\n");
 
 	return 0;
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -1765,6 +1765,12 @@ static int pcs_probe(struct platform_device *pdev)
 		goto free;
 	}
 
+	ret = pinctrl_claim_hogs(pcs->pctl);
+	if (ret) {
+		dev_err(pcs->dev, "could not claim hogs\n");
+		goto free;
+	}
+
 	ret = pcs_add_gpio_func(np, pcs);
 	if (ret < 0)
 		goto free;
diff --git a/drivers/pinctrl/sh-pfc/pinctrl.c b/drivers/pinctrl/sh-pfc/pinctrl.c
--- a/drivers/pinctrl/sh-pfc/pinctrl.c
+++ b/drivers/pinctrl/sh-pfc/pinctrl.c
@@ -816,6 +816,14 @@ int sh_pfc_register_pinctrl(struct sh_pfc *pfc)
 	pmx->pctl_desc.pins = pmx->pins;
 	pmx->pctl_desc.npins = pfc->info->nr_pins;
 
-	return devm_pinctrl_register_and_init(pfc->dev, &pmx->pctl_desc, pmx,
-					      &pmx->pctl);
+	ret = devm_pinctrl_register_and_init(pfc->dev, &pmx->pctl_desc, pmx,
+					     &pmx->pctl);
+	if (ret) {
+		dev_error(pcf->dev, "could not claim hogs: %i\n", ret);
+
+		return ret;
+
+	}
+
+	return pinctrl_claim_hogs(pmx->pctl);
 }
diff --git a/drivers/pinctrl/ti/pinctrl-ti-iodelay.c b/drivers/pinctrl/ti/pinctrl-ti-iodelay.c
--- a/drivers/pinctrl/ti/pinctrl-ti-iodelay.c
+++ b/drivers/pinctrl/ti/pinctrl-ti-iodelay.c
@@ -885,13 +885,15 @@ static int ti_iodelay_probe(struct platform_device *pdev)
 	iod->desc.name = dev_name(dev);
 	iod->desc.owner = THIS_MODULE;
 
+	platform_set_drvdata(pdev, iod);
+
 	ret = pinctrl_register_and_init(&iod->desc, dev, iod, &iod->pctl);
 	if (ret) {
 		dev_err(dev, "Failed to register pinctrl\n");
 		goto exit_out;
 	}
 
-	platform_set_drvdata(pdev, iod);
+	return pinctrl_claim_hogs(iod->pctl);
 
 exit_out:
 	of_node_put(np);
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -145,6 +145,7 @@ struct pinctrl_desc {
 extern int pinctrl_register_and_init(struct pinctrl_desc *pctldesc,
 				     struct device *dev, void *driver_data,
 				     struct pinctrl_dev **pctldev);
+extern int pinctrl_claim_hogs(struct pinctrl_dev *pctldev);
 
 /* Please use pinctrl_register_and_init() instead */
 extern struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
-- 
2.11.1