Messages in this thread Patch in this message | | | Date | Fri, 23 May 2014 10:15:03 -0600 | From | Stephen Warren <> | Subject | Re: [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.
| |