Re: [PATCH v3] pinctrl: to avoid duplicated calling enable_pinmux_setting for a pin

[FanWu <fwu@marvell.com>]

On 05/26/2014 10:52 AM, FanWu wrote:
> On 05/26/2014 10:43 AM, fwu@marvell.com wrote:
>> From: Fan Wu <fwu@marvell.com>
>>
>> What the patch did:
>> 1.To call pinmux_disable_setting ahead of pinmux_enable_setting in
>> each time of
>>    calling pinctrl_select_state
>> 2.Remove the HW disable operation in in pinmux_disable_setting function.
>>
>> The reason why to do this is that:
>> 1.To avoid duplicated calling enable_setting operation without disabling
>>    operation which will let Pin's desc->mux_usecount keep being added.
>> 2.The HW pin disable operation is not useful for most of the vendors'
>> platform.
>>    And this can be used to avoid the HW glitch after using the item 1#
>>    modification.
>>
>> In the following case, the issue can be reproduced:
>> 1)There is a driver need to switch Pin state dynamicly, E.g. b/t
>> "sleep" and
>> "default" state
>> 2)The Pin setting configuration in DTS node may be like the following
>> one:
>> component a {
>>     pinctrl-names = "default", "sleep";
>>     pinctrl-0 = <&a_grp_setting &c_grp_setting>;
>>     pinctrl-1 = <&b_grp_setting &c_grp_setting>;
>> }
>> The "c_grp_setting" config node is totaly same, maybe like following one:
>> c_grp_setting: c_grp_setting {
>>     pinctrl-single,pins = <GPIO48 AF6>;
>>     MFP_DEFAULT;
>> }
>> 3)When switching the Pin state in the following official Pinctrl
>> sequence:
>>     pin = pinctrl_get();
>>     state = pinctrl_lookup_state(wanted_state);
>>     pinctrl_select_state(state);
>>     pinctrl_put();
>>
>> Test Result:
>> 1)The switch is completed as expectation, that is: component's
>> Pins configuration are changed according to the description in the
>> "wanted_state" group setting
>> 2)The "desc->mux_usecount" of corresponding Pins in "c_group" is added
>> without being
>> decreased, because the "desc" is for each physical pin while the
>> "setting" is
>> for each setting node in the DTS.
>> Thus, if the "c_grp_setting" in pinctrl-0 is not disabled ahead of
>> enabling
>> "c_grp_setting" in pinctrl-1, the desc->mux_usecount will be kept
>> added without
>> any chance to be decreased.
>>
>> According to the comments in the original code, only the setting, in
>> old state
>> but not in new state, will be "disable"(calling
>> pinmux_disable_setting), which
>> is correct logic but not intact. We still need consider case that the
>> setting
>> is in both old state and new state.
>> We can do this in the following two ways:
>> 1) Avoid "enable"(calling pinmux_enable_setting) the Same Pins setting
>> repeatedly.
>> 2) "Disable"(calling pinmux_disable_setting) the "Same Pins setting",
>> actually
>> two setting instance, ahead of enabling them.
>>
>> Analysis:
>> 1.The solution 2# is better because it can avoid too much iteration.
>> 2.If we disable all of the setting in the old state and one/ones of
>> the setting(s) is/are
>> existed in the new state, the Pin's mux function change may happen when
>> some SoC vendors defined the "pinctrl-single,function-off" in their
>> DTS file.
>> old_setting=>disabled_setting=>new_setting.
>> 3.In the pinmux framework, when Pin state is switched, the setting in
>> the old state should be
>> marked as "disabled".
>>
>> Conclusion:
>> 1.To Remove the HW disabling operation to above the glitch mentioned
>> above.
>> 2.Handle the issue mentioned above by disabling all of the settings in
>> old
>> state and then enable the all of the settings in new state.
>>
>> Signed-off-by: Fan Wu <fwu@marvell.com>
>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>> ---
>>   drivers/pinctrl/core.c   |   24 +++++-------------------
>>   drivers/pinctrl/pinmux.c |    4 ----
>>   2 files changed, 5 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
>> index c0fe609..4445a67 100644
>> --- a/drivers/pinctrl/core.c
>> +++ b/drivers/pinctrl/core.c
>> @@ -989,29 +989,15 @@ int pinctrl_select_state(struct pinctrl *p,
>> struct pinctrl_state *state)
>>
>>       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.
>> +         * For each pinmux setting in the old state, forget SW's record
>> +         * of mux owner for that pingroup. Any pingroups which are
>> +         * still owned by the new state will be re-acquired by the call
>> +         * to pinmux_enable_setting() in the loop below.
>>            */
>>           list_for_each_entry(setting, &p->state->settings, node) {
>> -            bool found = false;
>>               if (setting->type != PIN_MAP_TYPE_MUX_GROUP)
>>                   continue;
>> -            list_for_each_entry(setting2, &state->settings, node) {
>> -                if (setting2->type != PIN_MAP_TYPE_MUX_GROUP)
>> -                    continue;
>> -                if (setting2->data.mux.group ==
>> -                        setting->data.mux.group) {
>> -                    found = true;
>> -                    break;
>> -                }
>> -            }
>> -            if (!found)
>> -                pinmux_disable_setting(setting);
>> +            pinmux_disable_setting(setting);
>>           }
>>       }
>>
>> diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
>> index 9248ce4..c2c4aff 100644
>> --- a/drivers/pinctrl/pinmux.c
>> +++ b/drivers/pinctrl/pinmux.c
>> @@ -469,7 +469,6 @@ void pinmux_disable_setting(struct pinctrl_setting
>> const *setting)
>>   {
>>       struct pinctrl_dev *pctldev = setting->pctldev;
>>       const struct pinctrl_ops *pctlops = pctldev->desc->pctlops;
>> -    const struct pinmux_ops *ops = pctldev->desc->pmxops;
>>       int ret;
>>       const unsigned *pins;
>>       unsigned num_pins;
>> @@ -515,9 +514,6 @@ void pinmux_disable_setting(struct pinctrl_setting
>> const *setting)
>>                    pins[i], desc->name, gname);
>>           }
>>       }
>> -
>> -    if (ops->disable)
>> -        ops->disable(pctldev, setting->data.mux.func,
>> setting->data.mux.group);
>>   }
>>
>>   #ifdef CONFIG_DEBUG_FS
>>
>
>
> Dear Stephen,
>
> Great thanks for your suggestion about the inline comments in the patch.
> I have updated the part you mentioned and the patch title.
> Please help to review again.
>
> Great to see that we almost get the goal of the long term discussion! :)
>
> After this patch is acknowledged by you Guys, I will submit the
> following two patches for you to review:
> 1.to remove the ops->disable registration in pinctrl-single
> 2.to remove the ops->disable function phandle in pinmux struct.
>
> Great thanks for this !
>

Dear Guys,

Do you have any comments about the new patch ?  Any suggestion is 
welcomed. :)

Looking forward your reply !
Great thanks for this !