Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932256AbbHCUuk (ORCPT ); Mon, 3 Aug 2015 16:50:40 -0400 Received: from gloria.sntech.de ([95.129.55.99]:60644 "EHLO gloria.sntech.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754533AbbHCUuj (ORCPT ); Mon, 3 Aug 2015 16:50:39 -0400 From: Heiko =?ISO-8859-1?Q?St=FCbner?= To: Doug Anderson Cc: huang lin , "open list:ARM/Rockchip SoC..." , Linus Walleij , "linux-gpio@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH v1 2/2] pinctrl: rockchip: only enable gpio clock when it setting Date: Mon, 03 Aug 2015 22:50:33 +0200 Message-ID: <2472708.6oReK8jXV2@diego> User-Agent: KMail/4.14.1 (Linux/3.16.0-4-amd64; KDE/4.14.2; x86_64; ; ) In-Reply-To: References: <1438574211-4887-1-git-send-email-hl@rock-chips.com> <1438574211-4887-2-git-send-email-hl@rock-chips.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2461 Lines: 66 Am Montag, 3. August 2015, 13:21:27 schrieb Doug Anderson: > hl > > On Sun, Aug 2, 2015 at 8:56 PM, huang lin wrote: > > gpio can keep state even the clock disable, for save power > > consumption, only enable gpio clock when it setting > > > > Signed-off-by: Heiko Stuebner > > Signed-off-by: huang lin > > > > Signed-off-by: huang lin > > Your "Signed-off-by"s are a little wonky here... Can you fix up? > > > --- > > > > drivers/pinctrl/pinctrl-rockchip.c | 60 > > ++++++++++++++++++++++++++++++++++---- 1 file changed, 54 insertions(+), > > 6 deletions(-) > > > > diff --git a/drivers/pinctrl/pinctrl-rockchip.c > > b/drivers/pinctrl/pinctrl-rockchip.c index cc2843a..445829f 100644 > > --- a/drivers/pinctrl/pinctrl-rockchip.c > > +++ b/drivers/pinctrl/pinctrl-rockchip.c > > @@ -945,17 +945,20 @@ static int _rockchip_pmx_gpio_set_direction(struct > > gpio_chip *chip,> > > if (ret < 0) > > > > return ret; > > > > + clk_enable(bank->clk); > > > > spin_lock_irqsave(&bank->slock, flags); > > > > - data = readl_relaxed(bank->reg_base + GPIO_SWPORT_DDR); > > + data = readl(bank->reg_base + GPIO_SWPORT_DDR); > > I am a little curious why you need to change the readl_relaxed() to a > read(). Are you trying to ensure that the clock was on before the > read happened? If so, I think this won't help. I see: > > #define readl(c) ({ u32 __v = readl_relaxed(c); __iormb(); __v; }) > > ...so that means that the iormb() is _after_ the readl. > > ...but I would believe that the clk_enable() call itself would be > guaranteeing that the clock was enabled in time. ...and if not then > grabbing the spinlock is another barrier, right? I think you do this > in a few places... > > Other than that this patch looks good to me.... I think that was my fault ... looking at stuff before figuring out that we're actually loosing the pd_pmu clock, and then forgetting to take this out again, before getting it to hl. In retospect it also seems silly to have changed them in the first place ;-) . So yes, they should be changed back to their original. Heiko -- 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/