Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756757Ab3HGRJT (ORCPT ); Wed, 7 Aug 2013 13:09:19 -0400 Received: from avon.wwwdotorg.org ([70.85.31.133]:46883 "EHLO avon.wwwdotorg.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753305Ab3HGRJS (ORCPT ); Wed, 7 Aug 2013 13:09:18 -0400 Message-ID: <52027F39.5050805@wwwdotorg.org> Date: Wed, 07 Aug 2013 11:09:13 -0600 From: Stephen Warren User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130510 Thunderbird/17.0.6 MIME-Version: 1.0 To: Linus Walleij CC: Sonic Zhang , Axel Lin , Grant Likely , Steven Miao , LKML , buildroot-devel@blackfin.uclinux.org, adi-buildroot-devel@lists.sourceforge.net, Sonic Zhang Subject: Re: [PATCH] pinctrl: pinmux: Don't free pins requested by other devices References: <1374039105-17777-1-git-send-email-sonic.adi@gmail.com> In-Reply-To: X-Enigmail-Version: 1.4.6 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2566 Lines: 73 On 08/07/2013 10:23 AM, Linus Walleij wrote: > On Wed, Jul 17, 2013 at 7:31 AM, Sonic Zhang wrote: > > I'd like Stephen and Axel to have a look at this as well... > >> From: Sonic Zhang >> >> in pinmux_disable_setting after current device fails to request >> the same pins. >> >> Signed-off-by: Sonic Zhang > > I don't quite understand the patch. Can you provide more context? Yes, the commit description needs to describe the problem this solves. I'm *guessing* the issue is: Something tries to enable a new mux setting on some pins. One of those pins is already owned by something else. So, applying the current setting fails. So, pinctrl core attempts to unapply the partially applied setting. This ends up incorrectly over-writing the conflicting ownership of the pins with NULL, and hence forgetting about it. I think a better change would be something more along the lines of: for (i = 0; i < num_pins; i++) + if (this_device_owns_pin(pins[i]) pin_free(pctldev, pins[i], NULL); ? Where this_device_owns_pin() might be someting like: desc->owning_setting == setting (which would be a new field that needed to be assigned during pinmux_enable_setting). Or perhaps the strcmp() is fine. >> diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c >> index 88cc509..9ebcf3b 100644 >> --- a/drivers/pinctrl/pinmux.c >> +++ b/drivers/pinctrl/pinmux.c >> @@ -482,13 +482,14 @@ void pinmux_disable_setting(struct pinctrl_setting const *setting) >> pins[i]); >> continue; >> } >> + /* And release the pins */ >> + if (desc->mux_usecount && >> + !strcmp(desc->mux_owner, setting->dev_name)) >> + pin_free(pctldev, pins[i], NULL); >> + >> desc->mux_setting = NULL; >> } >> >> - /* And release the pins */ >> - for (i = 0; i < num_pins; i++) >> - pin_free(pctldev, pins[i], NULL); >> - > > For pinmux_disable_setting() to inspect desc->mux_usecount seems > assymetric. This is something pin_free() should do, shouldn't it? > > Should not this codepath be kept and a change made inside pin_free() > for the check above instead? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/