2013/3/28 Stephen Warren <swar...@wwwdotorg.org>:
> 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.
Ok, I understand now.
>
>> 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.
>
got it.
I'll change that
thanks !
--
for me, ck means con kolivas and not calvin klein... does it mean I'm a geek ?
--
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/
