Re: [PATCH v2] pinctrl: add params in disable_setting for different usage

[Stephen Warren <swarren@wwwdotorg.org>]

On 05/22/2014 07:54 PM, FanWu wrote:
> On 05/23/2014 07:13 AM, Stephen Warren wrote:
>> On 05/21/2014 09:10 PM, 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.
>>>
>> ...
>>> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
>>> index c0fe609..c97491a 100644
>>> --- a/drivers/pinctrl/core.c
>>> +++ b/drivers/pinctrl/core.c
>>> @@ -993,25 +993,13 @@ int pinctrl_select_state(struct pinctrl *p,
>>> struct pinctrl_state *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.
>>> +         * that way. This code is used for each group that was
>>> +         * configured in the old state but not in the new state
>>
>>
>> Looking at the code, it's run for every group in the state, not "each
>> group that was configured in the old state but not in the new state"

> For you question 1:
> The disable_pinmux_setting is for the all of the setting in old state.
> This is what we really need to do, ahead of enable setting in new state.
> In the first patch I filed, which still includes the HW ops in
> disable_pinmux_setting, to disable each setting in old state and then to
> enable the setting in new state will introduce HW glitch.
> But in the current solution, the glitch will not be there, because there
> is no HW ops in disable_pinmux_setting.
> And please notice the patch is mainly used to avoid the duplicated
> enable operation for the same pin.

I think you missed the point of my comment. I think the new comment text
is incorrect. Instead, how about replacing the entire comment with:

/*
 * 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.
 */

>>> @@ -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);
>>>   }
>>
>> Should that op be removed from the header file and all drivers too?

> For your question 2:
> the pinctrl-single driver is still using ops->disable, if I remove the
> "disable" in ops, there will be build error in the vendor's code base
> who is using pinctrl-single driver.

I thought Tony said it was fine to simply remove pinctrl-single's
ops->disable code completeley.

> Just as I said in the last mail,
> the next plan for this topic:
> 
> 1. To remove the disable ops registration when defining the
> "include/linux/pinctrl/pinmux.h" in inctrl-single driver.
> Meanwhile, the related things, like "pinctrl-single,function-off"
> property and corresponding flag in "pcs_device", will be also removed.
> 
> 2. To remove the disable ops in "pinmux_ops" in the file of
> include/linux/pinctrl/pinmux.h
> 
> Are you OK for this ?

I guess splitting that into separate patches is fine.