lkml.org 
[lkml]   [2014]   [May]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH v2] pinctrl: add params in disable_setting for different usage
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.


\
 
 \ /
  Last update: 2014-05-23 19:01    [W:0.068 / U:0.132 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site