lkml.org 
[lkml]   [2013]   [Mar]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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.



\
 
 \ /
  Last update: 2013-03-28 16:01    [W:0.131 / U:0.120 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site