On 03/28/2013 04:55 AM, Richard Genoud wrote:
> 2013/3/28 Stephen Warren <swar...@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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
