Re: [PATCH] pinctrl: document the pinctrl PM states

[Tony Lindgren]

* Stephen Warren <swarren@xxxxxxxxxxxxx> [130624 11:15]:
> On 06/24/2013 04:10 AM, Tony Lindgren wrote:
> > * Stephen Warren <swarren@xxxxxxxxxxxxx> [130621 12:18]:
> >> On 06/21/2013 12:25 AM, Tony Lindgren wrote:
> >>> * Stephen Warren <swarren@xxxxxxxxxxxxx> [130620 12:32]:
> >>>>
> >>>> I assume you mean there shouldn't be any issue *modifying* the pinctrl
> >>>> API to allow multiple states to be active at once? And where you're
> >>>> talking about having multiple sets active at once already, you're
> >>>> talking about some other API?
> >>>
> >>> Nope, the standard pinctrl API. At least I have not seen issues with
> >>> having multiple states active the same time in a single driver.
> >>
> >> Please take a look at the implementation of pinctrl_select_state(). It
> >> very explicitly performs the following steps:
> >>
> >> 1) Find all pins(groups) that are used in the current state but not the
> >> new state, and execute pinctrl_disable_setting() on them. (For mux
> >> settings only, not pin config, since the core doesn't have any idea how
> >> to reverse config settings).
> >>
> >> 2) For all settings in the new state, apply those settings.
> >>
> >> So, it very explicitly only allows a single state to be set at a time.
> >> Equally, p->state (the field which stores the currently selected state)
> >> is a single item, not a set/list/array.
> > 
> > OK thanks I get now what you're saying. I did not see the p->state
> > issue as the disable function won't do anything for the SoCs that I
> > mostly deal with.
> > 
> >> So, this code needs rework if you want the core to support the concept
> >> of having multiple states active at once, since it needs separate
> >> pinctrl_activate_state() and pinctrl_deactivate_state() APIs, in order
> >> to avoid step (1) above. And of course, p->state would need to be a
> >> set/list/array.
> > 
> > I'll think about it a bit and do a patch to fix this. It seems that
> > that we need just two entries in the p->state array: static (default),
> > and dynamic. Then the dynamic would be typically one of: active, idle,
> > rx, tx.
> 
> I'm not entirely convinced that "2" is the right number. If we start
> allowing drivers to "piece together" multiple different state names, why
> wouldn't you allow 3 (or n) different state names to be active at once?
> Off-hand, I don't have specific use-cases in mind for more than 2 state
> (or even 1 in my case I suspect) - it just seems like expecting to
> arbitrarily restrict the number of co-active states is unlikely to last
> for long, and it'll end up being a slippery slope.

Yes let's set it up so we can expand it if needed.

We probably don't want the consumer drivers to piece together various
named states directly though as that will lead to custom code in every
driver..

I think we can make it happen automaticall for the cases we've discussed.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/