Messages in this thread | | | Date | Thu, 28 Mar 2013 08:53:01 -0600 | From | Stephen Warren <> | Subject | Re: [PATCH 3/4] pinctrl: disable and free setting in select_state in case of error |
| |
On 03/28/2013 04:55 AM, Richard Genoud wrote: > 2013/3/28 Stephen Warren <swarren@wwwdotorg.org>: >> On 03/25/2013 08:47 AM, Richard Genoud wrote: >>> If enabling a pin fails in pinctrl_select_state_locked(), all the >>> previous enabled pins have to be disabled to get back to the previous >>> state. >> >>> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
>>> + list_for_each_entry(setting2, &state->settings, node) { >>> + if (&setting2->node == &setting->node) >>> + break; >>> + pinctrl_free_setting(true, setting2); >> >> That's clearly wrong. >> >> pinctrl_free_setting() is supposed to free any memory associated with >> the setting; the storage that holds the representation of that setting. >> >> It's only appropriate to do that in pinctrl_put(), when actually >> destroying the whole struct pinctrl object. If pinctrl_select() fails, >> we don't want to destroy/invalidate the struct pinctrl content, but >> rather keep it around in case the driver uses it again even if the face >> of previous errors. >> >> In other words, what you should be doing inside this loop body is >> exactly what the body of the first loop inside pinctrl_select_state() >> does to "undo" any previously selected state, which is to call >> pinmux_disable_setting() for each entry, or something similar to that. > > The code here tries to undo what have been done in the *second* loop > of pinctrl_select_state(). > > The "do" loop is: > > list_for_each_entry(setting, &state->settings, node) { > switch (setting->type) { > case PIN_MAP_TYPE_MUX_GROUP: > ret = pinmux_enable_setting(setting); > break; > case PIN_MAP_TYPE_CONFIGS_PIN: > case PIN_MAP_TYPE_CONFIGS_GROUP: > ret = pinconf_apply_setting(setting); > break; > default: > ret = -EINVAL; > break; > } > > if (ret < 0) > goto unapply_new_state; > }
Right, I understand that.
> And maybe I'm wrong, but I understood that to "undo" pinmux_enable_setting, > we call pinmux_disable_setting()
Yes.
> and pinmux_free_setting() (which is empty for now).
No. pinmux_free_setting() free's the storage for a setting. Right now, nothing is dynamically allocated for the setting, so there's nothing to do. However, it's still semantically wrong to try to free it at this point.
> And to undo pinconf_apply_setting() we call pinconf_free_setting() > And that's what pinctrl_free_setting() does.
There's no way to undo the application of a setting. The only way to undo it is to apply a new setting that over-writes it. Hopefully, re-applying a different state would do that, but it's not guaranteed.
Again, pinconf_free_setting() is all about freeing any dynamically allocated storage required to represent the setting itself; it's not about (un-)programming HW.
| |