From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751700AbdB1R0R (ORCPT ); Tue, 28 Feb 2017 12:26:17 -0500 Received: from muru.com ([72.249.23.125]:36870 "EHLO muru.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751647AbdB1R0O (ORCPT ); Tue, 28 Feb 2017 12:26:14 -0500 Date: Tue, 28 Feb 2017 09:24:39 -0800 From: Tony Lindgren To: Gary Bisson Cc: Mika =?utf-8?B?UGVudHRpbMOk?= , LKML , linus.walleij@linaro.org Subject: Re: [REGRESSION] pinctrl, of, unable to find hogs Message-ID: <20170228172439.GD20572@atomide.com> References: <4b93bdb8-2894-4c27-6256-21499110f08e@nextfour.com> <20170227155352.GL21809@atomide.com> <20170227162747.kxgkyqzjximtou4u@t450s.lan> <20170227164033.3ogiyrt2hqlsnfqn@t450s.lan> <20170227173718.GO21809@atomide.com> <20170227184535.GP21809@atomide.com> <20170227210639.pxkcafpitacb6uwj@t450s.lan> <20170227230542.GQ21809@atomide.com> <20170228092538.gne33rswfnlhasrk@t450s.lan> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170228092538.gne33rswfnlhasrk@t450s.lan> User-Agent: Mutt/1.7.2 (2016-11-26) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Gary Bisson [170228 01:27]: > Hi Tony, > > On Mon, Feb 27, 2017 at 03:05:43PM -0800, Tony Lindgren wrote: > > * Gary Bisson [170227 13:08]: > > > Hi Tony, > > > > > > On Mon, Feb 27, 2017 at 10:45:35AM -0800, Tony Lindgren wrote: > > > > * Tony Lindgren [170227 09:37]: > > > > > * Gary Bisson [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 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