All of lore.kernel.org
 help / color / mirror / Atom feed
From: Tony Lindgren <tony@atomide.com>
To: Linus Walleij <linus.walleij@linaro.org>
Cc: Linux-OMAP <linux-omap@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	Stephen Warren <swarren@wwwdotorg.org>
Subject: Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
Date: Mon, 29 Jul 2013 02:31:25 -0700	[thread overview]
Message-ID: <20130729093125.GB7656@atomide.com> (raw)
In-Reply-To: <CACRpkdY6-f1DKfPDfCpYAX9weXMtB7koJ3VwkRgHGm_MzgFawQ@mail.gmail.com>

* Linus Walleij <linus.walleij@linaro.org> [130722 16:14]:
> On Tue, Jul 16, 2013 at 11:05 AM, Tony Lindgren <tony@atomide.com> wrote:
> 
> > To toggle dynamic states, let's add the optional active state in
> > addition to the static default state. Then if the optional active
> > state is defined, we can require that idle and sleep states cover
> > the same pingroups as the active state.
> 
> OK...
> 
> > Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic()
> > to use instead of pinctrl_select() to avoid breaking existing users.
> >
> > With pinctrl_check_dynamic() we can check that idle and sleep states
> > match the active state for pingroups during init, and don't need to
> > do it during runtime.
> 
> I do not understand why this complexity need to be exposed outside
> of the subsystem.

Unfortunately it's mostly to deal with supporting the current behaviour
of pinctrl_select_state() which is not quite suitable for runtime PM.
 
> pinctrl_select_state() and the PM accessors are enough IMO. Why
> should say a driver care about whether it is dynamic or not?

I think we can make this all transparent to the consumer drivers
for runtime PM. Basically drivers/base/pinctrl.c needs these for the
checks because of the current pinctrl_select_state().
 
> Surely the checking and different paths for static/dynamic configurations
> can be an intrinsic detail of the pinctrl subsystem, by adding flags and
> members to private structs like struct pinctrl itself in worst case.

I'll take a look if we can bury more things inside the pinctrl
subsystem.
 
> So I'm not buying into this, it looks like it is making things complicated
> for consumers outside the subsystem for no reason.

I don't think the consumer drivers eventually need to do much
anything ideally. We're missing runtime PM related set_irq_wake()
but that's a minor detail as we can initially keep the runtime
PM related wake-up events always enabled.

Regards,

Tony

WARNING: multiple messages have this Message-ID (diff)
From: tony@atomide.com (Tony Lindgren)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 3/4] pinctrl: Add support for additional dynamic states
Date: Mon, 29 Jul 2013 02:31:25 -0700	[thread overview]
Message-ID: <20130729093125.GB7656@atomide.com> (raw)
In-Reply-To: <CACRpkdY6-f1DKfPDfCpYAX9weXMtB7koJ3VwkRgHGm_MzgFawQ@mail.gmail.com>

* Linus Walleij <linus.walleij@linaro.org> [130722 16:14]:
> On Tue, Jul 16, 2013 at 11:05 AM, Tony Lindgren <tony@atomide.com> wrote:
> 
> > To toggle dynamic states, let's add the optional active state in
> > addition to the static default state. Then if the optional active
> > state is defined, we can require that idle and sleep states cover
> > the same pingroups as the active state.
> 
> OK...
> 
> > Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic()
> > to use instead of pinctrl_select() to avoid breaking existing users.
> >
> > With pinctrl_check_dynamic() we can check that idle and sleep states
> > match the active state for pingroups during init, and don't need to
> > do it during runtime.
> 
> I do not understand why this complexity need to be exposed outside
> of the subsystem.

Unfortunately it's mostly to deal with supporting the current behaviour
of pinctrl_select_state() which is not quite suitable for runtime PM.
 
> pinctrl_select_state() and the PM accessors are enough IMO. Why
> should say a driver care about whether it is dynamic or not?

I think we can make this all transparent to the consumer drivers
for runtime PM. Basically drivers/base/pinctrl.c needs these for the
checks because of the current pinctrl_select_state().
 
> Surely the checking and different paths for static/dynamic configurations
> can be an intrinsic detail of the pinctrl subsystem, by adding flags and
> members to private structs like struct pinctrl itself in worst case.

I'll take a look if we can bury more things inside the pinctrl
subsystem.
 
> So I'm not buying into this, it looks like it is making things complicated
> for consumers outside the subsystem for no reason.

I don't think the consumer drivers eventually need to do much
anything ideally. We're missing runtime PM related set_irq_wake()
but that's a minor detail as we can initially keep the runtime
PM related wake-up events always enabled.

Regards,

Tony

  reply	other threads:[~2013-07-29  9:31 UTC|newest]

Thread overview: 89+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-07-16  9:05 [PATCH 0/4] improved support for runtime muxing for pinctrl Tony Lindgren
2013-07-16  9:05 ` Tony Lindgren
2013-07-16  9:05 ` [PATCH 1/4] pinctrl: Remove duplicate code in pinctrl_pm_select_state functions Tony Lindgren
2013-07-16  9:05   ` Tony Lindgren
2013-07-16 13:15   ` Grygorii Strashko
2013-07-16 13:15     ` Grygorii Strashko
2013-07-16 13:15     ` Grygorii Strashko
2013-07-16 13:41     ` Tony Lindgren
2013-07-16 13:41       ` Tony Lindgren
2013-07-16 14:25       ` Grygorii Strashko
2013-07-16 14:25         ` Grygorii Strashko
2013-07-16 14:25         ` Grygorii Strashko
2013-07-17  6:31         ` Tony Lindgren
2013-07-17  6:31           ` Tony Lindgren
2013-07-16  9:05 ` [PATCH 2/4] pinctrl: Allow pinctrl to have multiple active states Tony Lindgren
2013-07-16  9:05   ` Tony Lindgren
2013-07-17 20:55   ` Stephen Warren
2013-07-17 20:55     ` Stephen Warren
2013-07-16  9:05 ` [PATCH 3/4] pinctrl: Add support for additional dynamic states Tony Lindgren
2013-07-16  9:05   ` Tony Lindgren
2013-07-16  9:35   ` Felipe Balbi
2013-07-16  9:35     ` Felipe Balbi
2013-07-16  9:35     ` Felipe Balbi
2013-07-16 12:06     ` Tony Lindgren
2013-07-16 12:06       ` Tony Lindgren
2013-07-17 21:14   ` Stephen Warren
2013-07-17 21:14     ` Stephen Warren
2013-07-18  7:25     ` Tony Lindgren
2013-07-18  7:25       ` Tony Lindgren
2013-07-18 10:53       ` Tony Lindgren
2013-07-18 10:53         ` Tony Lindgren
2013-07-18 19:21       ` Stephen Warren
2013-07-18 19:21         ` Stephen Warren
2013-07-19  7:29         ` Tony Lindgren
2013-07-19  7:29           ` Tony Lindgren
2013-07-19 18:52           ` Stephen Warren
2013-07-19 18:52             ` Stephen Warren
2013-07-29  9:05             ` Tony Lindgren
2013-07-29  9:05               ` Tony Lindgren
2013-07-29 22:01               ` Stephen Warren
2013-07-29 22:01                 ` Stephen Warren
2013-08-14 16:41                 ` Linus Walleij
2013-08-14 16:41                   ` Linus Walleij
2013-08-14 16:41                   ` Linus Walleij
2013-07-17 21:23   ` Stephen Warren
2013-07-17 21:23     ` Stephen Warren
2013-07-18  7:36     ` Tony Lindgren
2013-07-18  7:36       ` Tony Lindgren
2013-07-18 19:26       ` Stephen Warren
2013-07-18 19:26         ` Stephen Warren
2013-07-19  7:39         ` Tony Lindgren
2013-07-19  7:39           ` Tony Lindgren
2013-07-19 10:29           ` Grygorii Strashko
2013-07-19 10:29             ` Grygorii Strashko
2013-07-19 10:29             ` Grygorii Strashko
2013-07-19 19:03             ` Stephen Warren
2013-07-19 19:03               ` Stephen Warren
2013-07-22 23:15               ` Linus Walleij
2013-07-22 23:15                 ` Linus Walleij
2013-07-22 23:15                 ` Linus Walleij
2013-07-29  9:08               ` Tony Lindgren
2013-07-29  9:08                 ` Tony Lindgren
2013-07-19 18:58           ` Stephen Warren
2013-07-19 18:58             ` Stephen Warren
2013-07-29  9:21             ` Tony Lindgren
2013-07-29  9:21               ` Tony Lindgren
2013-07-29 22:08               ` Stephen Warren
2013-07-29 22:08                 ` Stephen Warren
2013-07-22 23:07   ` Linus Walleij
2013-07-22 23:07     ` Linus Walleij
2013-07-22 23:07     ` Linus Walleij
2013-07-29  9:31     ` Tony Lindgren [this message]
2013-07-29  9:31       ` Tony Lindgren
2013-07-29  9:31       ` Tony Lindgren
2013-07-16  9:05 ` [PATCH 4/4] drivers: Add pinctrl handling for dynamic pin states Tony Lindgren
2013-07-16  9:05   ` Tony Lindgren
2013-07-17 21:21   ` Stephen Warren
2013-07-17 21:21     ` Stephen Warren
2013-07-18  7:50     ` Tony Lindgren
2013-07-18  7:50       ` Tony Lindgren
2013-07-18 13:48       ` Tony Lindgren
2013-07-18 13:48         ` Tony Lindgren
2013-07-16  9:14 ` [PATCH 0/4] improved support for runtime muxing for pinctrl Tony Lindgren
2013-07-16  9:14   ` Tony Lindgren
2013-07-17 11:49 ` Grygorii Strashko
2013-07-17 11:49   ` Grygorii Strashko
2013-07-17 11:49   ` Grygorii Strashko
2013-07-18 15:15 [PATCHv2 " Tony Lindgren
2013-07-18 15:15 ` [PATCH 3/4] pinctrl: Add support for additional dynamic states Tony Lindgren
2013-07-18 15:15   ` Tony Lindgren

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20130729093125.GB7656@atomide.com \
    --to=tony@atomide.com \
    --cc=linus.walleij@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-omap@vger.kernel.org \
    --cc=swarren@wwwdotorg.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.