Messages in this thread | | | Date | Tue, 28 Feb 2017 10:25:38 +0100 | From | Gary Bisson <> | Subject | Re: [REGRESSION] pinctrl, of, unable to find hogs |
| |
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
| |