[PATCH 3/4] pinctrl: Add support for additional dynamic states

[Stephen Warren]

On 07/19/2013 01:29 AM, Tony Lindgren wrote:
> * Stephen Warren <swarren at wwwdotorg.org> [130718 12:27]:
>> On 07/18/2013 01:25 AM, Tony Lindgren wrote:
>>> * Stephen Warren <swarren at wwwdotorg.org> [130717 14:21]:
>>>> On 07/16/2013 03:05 AM, Tony Lindgren 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.
>>>>>
>>>>> 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.
>>>>>
>>>>> Then with the states pre-validated, pinctrl_select_dynamic() can
>>>>> just toggle between the dynamic states without extra checks.
>>>>>
>>>>> Note that pinctr_select_state() still has valid use cases, such as
>>>>> changing states when the pins can be shared between two drivers
>>>>> and don't necessarily cover the same pingroups. For dynamic runtime
>>>>> toggling of pin states, we should eventually always use just
>>>>> pinctrl_select_dynamic().
>>
>>>>> @@ -1241,7 +1371,13 @@ static int pinctrl_pm_select_state(struct device *dev, struct pinctrl_state *sta
>>>>>  		return 0;
>>>>>  	if (IS_ERR(state))
>>>>>  		return 0; /* No such state */
>>>>> -	ret = pinctrl_select_state(pins->p, state);
>>>>> +
>>>>> +	/* Configured for proper dynamic muxing? */
>>>>> +	if (!IS_ERR(dev->pins->active_state))
>>>>> +		ret = pinctrl_select_dynamic(pins->p, state);
>>>>> +	else
>>>>> +		ret = pinctrl_select_state(pins->p, state);
>>>>
>>>> Hmmm. I'm not quite sure this is right... Surely this function should
>>>> just do nothing if the dynamic states aren't defined? The system should
>>>> just, and only, be in the "default" state and never do anything else.
>>>
>>> This keeps the existing behaviour. We should be able to drop the
>>> call to pinctrl_select_state() here after the existing use in
>>> drivers has been fixed.
>>
>> How many DT-based systems already have multiple of default/idle/sleep
>> states defined in DT? Right now, since the kernel code uses
>> pinctrl_select_state() to switch between those, the state definitions
>> need to define /all/ pins used by those states, not just the dynamic
>> ones. However, with this series in place, the default state should only
>> include the static pins, and the active/idle/sleep states should only
>> include the dynamic pins. That's a change to the DT bindings, since it
>> changes how the DT must be written, and causes older DTs to be
>> incompatible with newer kernels and vice-versa.
> 
> Well we can keep the current behaviour with pinctrl_select_state() around
> as long as needed. For the legacy cases, there is no active state defined
> and we fall back to pinctrl_select_state().
>  
>> So, do we just ignore this DT ABI change again, or insist on doing it in
>> some compatible way? DT ABI-ness is a PITA:-(
> 
> I'd vote for keeping the existing behaviour with pinctrl_select_state()
> when no active state is defined.

Yes, I think that will work, since the active state cannot exist before
this new scheme is in place.

But, this needs to be very clearly spell out in the DT binding
documentation: If you have states default/idle/sleep, they're complete
alternatives, whereas if you have states default/active/idle/sleep, the
latter 3 are alternatives that build on top of the first. I foresee mass
confusion, but perhaps I'm being pessimistic.