Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1199258

Re: [PATCH v1 2/2] pinctrl: rockchip: only enable gpio clock when it setting

Show all headers | View raw

Am Montag, 3. August 2015, 13:21:27 schrieb Doug Anderson:
> hl
> 
> On Sun, Aug 2, 2015 at 8:56 PM, huang lin <hl@rock-chips.com> 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 <heiko@sntech.de>
> > Signed-off-by: huang lin <hl@rock-chips.com>
> > 
> > Signed-off-by: huang lin <hl@rock-chips.com>
> 
> 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/

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v1 1/2] clk: rockchip: add pclk_pd_pmu to the list of rk3288 critical clocks huang lin <hl@rock-chips.com> - 2015-08-03 06:00 +0200
  [PATCH v1 2/2] pinctrl: rockchip: only enable gpio clock when it setting huang lin <hl@rock-chips.com> - 2015-08-03 06:00 +0200
    Re: [PATCH v1 2/2] pinctrl: rockchip: only enable gpio clock when it setting Doug Anderson <dianders@chromium.org> - 2015-08-03 22:30 +0200
      Re: [PATCH v1 2/2] pinctrl: rockchip: only enable gpio clock when it setting Heiko Stübner <heiko@sntech.de> - 2015-08-03 23:00 +0200
  Re: [PATCH v1 1/2] clk: rockchip: add pclk_pd_pmu to the list of  rk3288 critical clocks Doug Anderson <dianders@chromium.org> - 2015-08-03 22:00 +0200

csiph-web