Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1588801 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [REGRESSION] pinctrl, of, unable to find hogs Gary Bisson <gary.bisson@boundarydevices.com> - 2017-02-27 17:40 +0100
    Re: [REGRESSION] pinctrl, of, unable to find hogs Gary Bisson <gary.bisson@boundarydevices.com> - 2017-02-27 18:20 +0100
      Re: [REGRESSION] pinctrl, of, unable to find hogs Tony Lindgren <tony@atomide.com> - 2017-02-27 19:40 +0100
        Re: [REGRESSION] pinctrl, of, unable to find hogs Gary Bisson <gary.bisson@boundarydevices.com> - 2017-02-27 23:30 +0100
          Re: [REGRESSION] pinctrl, of, unable to find hogs Tony Lindgren <tony@atomide.com> - 2017-02-28 00:50 +0100
            Re: [REGRESSION] pinctrl, of, unable to find hogs Gary Bisson <gary.bisson@boundarydevices.com> - 2017-02-28 10:40 +0100
              Re: [REGRESSION] pinctrl, of, unable to find hogs Tony Lindgren <tony@atomide.com> - 2017-02-28 18:30 +0100
        Re: [REGRESSION] pinctrl, of, unable to find hogs Tony Lindgren <tony@atomide.com> - 2017-02-27 23:40 +0100

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

Mika, Tony, All,

On Mon, Feb 27, 2017 at 07:53:53AM -0800, Tony Lindgren wrote:
> * Mika Penttilä <mika.penttila@nextfour.com> [170226 21:46]:
> > 
> > With current linus git (pre 4.11), unable to find the pinctrl hogs :
> > 
> > 
> >  imx6q-pinctrl 20e0000.iomuxc: unable to find group for node hoggrp
> > 
> > 
> > Device is i.MX6 based.
> 
> Sorry to hear about that, maybe imx_pinctrl_probe_dt() should be
> called before devm_pinctrl_register_and_init()?
> 
> Things got moved around a bit with e566fc11ea76 ("pinctrl: imx: use
> generic pinctrl helpers for managing groups") it seems. But maybe that
> was done because we did not have commit 950b0d91dc10 ("pinctrl: core:
> Fix regression caused by delayed work for hogs") when the imx_pinctrl
> changes got merged.

Indeed the i.MX changes were made before your the rework.

The reason imx_pinctrl_probe_dt got moved around is because
devm_pinctrl_register is the one that initializes the radix trees that
are needed when probing the dt.

> Gary, are you able to reproduce this? Seems it should happen with
> any imx with hogs configured in the dts.

Yes I can reproduce the issue.

Not sure how to fix it though since we can't move the dt probing before
radix tree init.

Regards,
Gary

[toc] | [next] | [standalone]

#1588824

On Mon, Feb 27, 2017 at 05:27:47PM +0100, Gary Bisson wrote:
> Mika, Tony, All,
> 
> On Mon, Feb 27, 2017 at 07:53:53AM -0800, Tony Lindgren wrote:
> > * Mika Penttilä <mika.penttila@nextfour.com> [170226 21:46]:
> > > 
> > > With current linus git (pre 4.11), unable to find the pinctrl hogs :
> > > 
> > > 
> > >  imx6q-pinctrl 20e0000.iomuxc: unable to find group for node hoggrp
> > > 
> > > 
> > > Device is i.MX6 based.
> > 
> > Sorry to hear about that, maybe imx_pinctrl_probe_dt() should be
> > called before devm_pinctrl_register_and_init()?
> > 
> > Things got moved around a bit with e566fc11ea76 ("pinctrl: imx: use
> > generic pinctrl helpers for managing groups") it seems. But maybe that
> > was done because we did not have commit 950b0d91dc10 ("pinctrl: core:
> > Fix regression caused by delayed work for hogs") when the imx_pinctrl
> > changes got merged.
> 
> Indeed the i.MX changes were made before your the rework.

s/the/hog/

> The reason imx_pinctrl_probe_dt got moved around is because
> devm_pinctrl_register is the one that initializes the radix trees that
> are needed when probing the dt.
> 
> > Gary, are you able to reproduce this? Seems it should happen with
> > any imx with hogs configured in the dts.
> 
> Yes I can reproduce the issue.
> 
> Not sure how to fix it though since we can't move the dt probing before
> radix tree init.
> 
> Regards,
> Gary

[toc] | [prev] | [next] | [standalone]

#1588859

* Gary Bisson <gary.bisson@boundarydevices.com> [170227 08:42]:
> On Mon, Feb 27, 2017 at 05:27:47PM +0100, Gary Bisson wrote:
> > Mika, Tony, All,
> > 
> > On Mon, Feb 27, 2017 at 07:53:53AM -0800, Tony Lindgren wrote:
> > > * Mika Penttilä <mika.penttila@nextfour.com> [170226 21:46]:
> > > > 
> > > > With current linus git (pre 4.11), unable to find the pinctrl hogs :
> > > > 
> > > > 
> > > >  imx6q-pinctrl 20e0000.iomuxc: unable to find group for node hoggrp
> > > > 
> > > > 
> > > > Device is i.MX6 based.
> > > 
> > > Sorry to hear about that, maybe imx_pinctrl_probe_dt() should be
> > > called before devm_pinctrl_register_and_init()?
> > > 
> > > Things got moved around a bit with e566fc11ea76 ("pinctrl: imx: use
> > > generic pinctrl helpers for managing groups") it seems. But maybe that
> > > was done because we did not have commit 950b0d91dc10 ("pinctrl: core:
> > > Fix regression caused by delayed work for hogs") when the imx_pinctrl
> > > changes got merged.
> > 
> > Indeed the i.MX changes were made before your the rework.
> 
> s/the/hog/
> 
> > The reason imx_pinctrl_probe_dt got moved around is because
> > devm_pinctrl_register is the one that initializes the radix trees that
> > are needed when probing the dt.

OK

> > > Gary, are you able to reproduce this? Seems it should happen with
> > > any imx with hogs configured in the dts.
> > 
> > Yes I can reproduce the issue.

OK good to hear.

> > 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 :)

Regards,

Tony

[toc] | [prev] | [next] | [standalone]

#1588992

Hi Tony,

On Mon, Feb 27, 2017 at 10:45:35AM -0800, Tony Lindgren wrote:
> * 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().

I'll try that tomorrow morning and let you know.

Regards,
Gary

[toc] | [prev] | [next] | [standalone]

#1589048

* Gary Bisson <gary.bisson@boundarydevices.com> [170227 13:08]:
> Hi Tony,
> 
> On Mon, Feb 27, 2017 at 10:45:35AM -0800, Tony Lindgren wrote:
> > * 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().
> 
> 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

[toc] | [prev] | [next] | [standalone]

#1589312

Hi Tony,

On Mon, Feb 27, 2017 at 03:05:43PM -0800, Tony Lindgren wrote:
> * Gary Bisson <gary.bisson@boundarydevices.com> [170227 13:08]:
> > Hi Tony,
> > 
> > On Mon, Feb 27, 2017 at 10:45:35AM -0800, Tony Lindgren wrote:
> > > * 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().
> > 
> > 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 tested your patch and confirm it works.
Tested-by: Gary Bisson <gary.bisson@boundarydevices.com>

I made one change though, see below.

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

Indeed it doesn't strike me as really necessary. But I guess the
question is now: which option is the best/acceptable for 4.11?

> 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");

Here you need to add:
		ret = PTR_ERR(ipctl->pctl);

Otherwise the return value will be undetermined (and a warning shows
up).

Regards,
Gary

[toc] | [prev] | [next] | [standalone]

#1589645

* Gary Bisson <gary.bisson@boundarydevices.com> [170228 01:27]:
> Hi Tony,
> 
> On Mon, Feb 27, 2017 at 03:05:43PM -0800, Tony Lindgren wrote:
> > * Gary Bisson <gary.bisson@boundarydevices.com> [170227 13:08]:
> > > Hi Tony,
> > > 
> > > On Mon, Feb 27, 2017 at 10:45:35AM -0800, Tony Lindgren wrote:
> > > > * 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().
> > > 
> > > 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 tested your patch and confirm it works.
> Tested-by: Gary Bisson <gary.bisson@boundarydevices.com>

OK good to hear.

> I made one change though, see below.

OK

> > 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()?
> 
> Indeed it doesn't strike me as really necessary. But I guess the
> question is now: which option is the best/acceptable for 4.11?

Yeah.. pinctrl_register_and_init() reduces some boilerplate code, but it
still has the $subject issue with hogs. So I don't know if we want to
get stuck with supporting it.

> > --- 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");
> 
> Here you need to add:
> 		ret = PTR_ERR(ipctl->pctl);
> 
> Otherwise the return value will be undetermined (and a warning shows
> up).

Oops thanks for catching that, will fold in.

Regards,

Tony

[toc] | [prev] | [next] | [standalone]

#1589001

* 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web