* Gary Bisson <gary.bis...@boundarydevices.com> [170228 01:27]: > Hi Tony, > > On Mon, Feb 27, 2017 at 03:05:43PM -0800, Tony Lindgren wrote: > > * Gary Bisson <gary.bis...@boundarydevices.com> [170227 13:08]: > > > Hi Tony, > > > > > > On Mon, Feb 27, 2017 at 10:45:35AM -0800, Tony Lindgren wrote: > > > > * Tony Lindgren <t...@atomide.com> [170227 09:37]: > > > > > * Gary Bisson <gary.bis...@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.bis...@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