* Stephen Warren <swarren@xxxxxxxxxxxxx> [130718 12:27]: > On 07/18/2013 01:25 AM, Tony Lindgren wrote: > > * Stephen Warren <swarren@xxxxxxxxxxxxx> [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. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
