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

[Tony Lindgren]

* Gary Bisson <gary.bisson@xxxxxxxxxxxxxxxxxxx> [170227 13:08]:
> Hi Tony,
> 
> On Mon, Feb 27, 2017 at 10:45:35AM -0800, Tony Lindgren wrote:
> > * Tony Lindgren <tony@xxxxxxxxxxx> [170227 09:37]:
> > > * Gary Bisson <gary.bisson@xxxxxxxxxxxxxxxxxxx> [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().
> 
> I'll try that tomorrow morning and let you know.

Actually that one is not considering that it's OK to return -ENODEV
for hogs if not found. Below is what I think is a better version,
compile tested only for imx6 though. I boot tested the similar changes
with pinctrl-single with an additional patch.

It just splits up things further and exports these for pin controller
drivers to use:

pinctrl_init_controller
pinctrl_claim_hogs
pinctrl_enable

Splitting things that way allows parsing the pins dynamically like
you do. And that can be then used later on for other pin controller
drivers to simplify things further.

I wonder if we should drop the pinctrl_register_and_init() we recently
introduced in favor of init + claim_hogs + enable. Linus, what's your
preference, keep or drop pinctrl_register_and_init()?

Regards,

Tony

8< ---------------------
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -2009,32 +2009,50 @@ struct pinctrl_dev *pinctrl_init_controller(struct pinctrl_desc *pctldesc,
 	kfree(pctldev);
 	return ERR_PTR(ret);
 }
+EXPORT_SYMBOL_GPL(pinctrl_init_controller);
 
-static int pinctrl_create_and_start(struct pinctrl_dev *pctldev)
+int pinctrl_claim_hogs(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");
-		}
+	if (PTR_ERR(pctldev->p) == -ENODEV) {
+		dev_dbg(pctldev->dev, "no hogs found\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;
+	}
+
+	if (IS_ERR(pctldev->p)) {
+		dev_err(pctldev->dev, "error claiming hogs: %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);
+
+void pinctrl_enable(struct pinctrl_dev *pctldev)
+{
 	mutex_lock(&pinctrldev_list_mutex);
 	list_add_tail(&pctldev->node, &pinctrldev_list);
 	mutex_unlock(&pinctrldev_list_mutex);
@@ -2043,6 +2061,24 @@ static int pinctrl_create_and_start(struct pinctrl_dev *pctldev)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(pinctrl_enable);
+
+static int pinctrl_create_and_start(struct pinctrl_dev *pctldev)
+{
+	int error;
+
+	error = pinctrl_claim_hogs(pctldev);
+	if (error) {
+		dev_err(pctldev->dev, "could not claim hogs: %i\n",
+			error);
+
+		return error;
+	}
+
+	pinctrl_enable(pctldev);
+
+	return 0;
+}
 
 /**
  * pinctrl_register() - register a pin controller device
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
@@ -774,10 +774,10 @@ int imx_pinctrl_probe(struct platform_device *pdev,
 	ipctl->info = info;
 	ipctl->dev = info->dev;
 	platform_set_drvdata(pdev, ipctl);
-	ret = devm_pinctrl_register_and_init(&pdev->dev,
-					     imx_pinctrl_desc, ipctl,
-					     &ipctl->pctl);
-	if (ret) {
+
+	ipctl->pctl = pinctrl_init_controller(imx_pinctrl_desc, &pdev->dev,
+					      ipctl);
+	if (IS_ERR(ipctl->pctl)) {
 		dev_err(&pdev->dev, "could not register IMX pinctrl driver\n");
 		goto free;
 	}
@@ -788,8 +788,16 @@ 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");
 
+	pinctrl_enable(ipctl->pctl);
+
 	return 0;
 
 free:
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
@@ -146,7 +146,14 @@ 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 */
+/* Use these if dynamically allocating pins */
+extern struct pinctrl_dev *
+pinctrl_init_controller(struct pinctrl_desc *pctldesc, struct device *dev,
+			void *driver_data);
+extern int pinctrl_claim_hogs(struct pinctrl_dev *pctldev);
+extern void pinctrl_enable(struct pinctrl_dev *pctldev);
+
+/* Please use pinctrl_register_and_init() or the above instead */
 extern struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
 				struct device *dev, void *driver_data);
 
-- 
2.11.1