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

[Gary Bisson]

Hi Tony,

On Mon, Feb 27, 2017 at 03:05:43PM -0800, Tony Lindgren wrote:
> * 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 tested your patch and confirm it works.
Tested-by: Gary Bisson <gary.bisson@xxxxxxxxxxxxxxxxxxx>

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