Messages in this thread | | | Date | Wed, 29 Feb 2012 14:46:34 +0800 | From | Dong Aisheng <> | Subject | Re: [PATCH V2 4/6] pinctrl: API changes to support multiple states per device |
| |
On Tue, Feb 28, 2012 at 01:27:30PM -0700, Stephen Warren wrote: > The API model is changed from: > > p = pinctrl_get(dev, "state1"); > pinctrl_enable(p); > ... > pinctrl_disable(p); > pinctrl_put(p); > p = pinctrl_get(dev, "state2"); > pinctrl_enable(p); > ... > pinctrl_disable(p); > pinctrl_put(p); > > to this: > > p = pinctrl_get(dev); > s1 = pinctrl_lookup_state(p, "state1"); > s2 = pinctrl_lookup_state(p, "state2"); > pinctrl_select_state(p, s1); > ... > pinctrl_select_state(p, s2); > ... > pinctrl_put(p); > > This allows devices to directly transition between states without > disabling the pin controller programming and put()/get()ing the > configuration data each time. This model will also better suit pinconf > programming, which doesn't have a concept of "disable". > > The special-case hogging feature of pin controllers is re-written to use > the regular APIs instead of special-case code. Hence, the pinmux-hogs > debugfs file is removed; see the top-level pinctrl-handles files for > equivalent data. > > Signed-off-by: Stephen Warren <swarren@nvidia.com> > --- > v2: Make use of PINCTRL_STATE_DEFAULT, split out some documentation > cleanup into an earlier patch. Various minor fixes. Fixes due to > rebasing on updated earlier patches. Remove usecount field from struct > pinctrl; allow only one concurrent pinctrl_get() per device. > Glad to hear this.
The whole patch looks pretty good to me. A few trivial comments, else Acked-by: Dong Aisheng <dong.aisheng@linaro.org>
> +static struct pinctrl *find_pinctrl(struct device *dev) > +{ > + struct pinctrl *p; > + > + list_for_each_entry(p, &pinctrldev_list, node) s/pinctrldev_list/pinctrl_list ?
> -static void pinctrl_disable_locked(struct pinctrl *p) > +static int pinctrl_select_state_locked(struct pinctrl *p, > + struct pinctrl_state *state) > { > - struct pinctrl_setting *setting; > + struct pinctrl_setting *setting, *setting2; > + int ret; > > - if (p == NULL) > - return; > + if (p->state == state) > + return 0; > > - if (--p->usecount == 0) { > - list_for_each_entry(setting, &p->settings, node) > - pinmux_disable_setting(setting); > + if (p->state) { > + /* > + * The set of groups with a mux configuration in the old state > + * may not be identical to the set of groups with a mux setting > + * in the new state. While this might be unusual, it's entirely > + * possible for the "user"-supplied mapping table to be written > + * that way. For each group that was configured in the old state > + * but not in the new state, this code puts that group into a > + * safe/disabled state. It means the old state function of some groups may not have been disabled before enabling the new function mode. May it be a little error-prone? Maybe we can not depend on user to disable a function before enable another for a group when doing pinmux_enable_setting considering they may be different registers.
> + */ > + list_for_each_entry(setting, &p->state->settings, node) { > + bool found = false; > + list_for_each_entry(setting2, &state->settings, node) { > + if (setting2->group_selector == > + setting->group_selector) { > + found = true; > + break; > + } > + } > + if (!found) > + pinmux_disable_setting(setting); > + } > + } > + > + p->state = state; > + > + /* Apply all the settings for the new state */ > + list_for_each_entry(setting, &state->settings, node) { > + ret = pinmux_enable_setting(setting); > + if (ret < 0) { > + /* FIXME: Difficult to return to prev state */ > + return ret; > + } > } > + > + return 0; > } > > /**
Regards Dong Aisheng
| |