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

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

[fwu]

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.
3.Remove the disable ops in struct pinmux_ops

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>;
}
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();

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 choose 2# solution, we need to
1) Should mark all the setting in disable state as disabled ahead of enabling.
2) Avoid the possible HW glitch by removing the HW disable ops in
in the pinmux_disable_setting
If we disable all of the settings 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.(HW glitch)

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 ----
 include/linux/pinctrl/pinmux.h |    2 --
 3 files changed, 5 insertions(+), 25 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
diff --git a/include/linux/pinctrl/pinmux.h b/include/linux/pinctrl/pinmux.h
index c153950..3097aaf 100644
--- a/include/linux/pinctrl/pinmux.h
+++ b/include/linux/pinctrl/pinmux.h
@@ -70,8 +70,6 @@ struct pinmux_ops {
 				  unsigned * const num_groups);
 	int (*enable) (struct pinctrl_dev *pctldev, unsigned func_selector,
 		       unsigned group_selector);
-	void (*disable) (struct pinctrl_dev *pctldev, unsigned func_selector,
-			 unsigned group_selector);
 	int (*gpio_request_enable) (struct pinctrl_dev *pctldev,
 				    struct pinctrl_gpio_range *range,
 				    unsigned offset);
-- 
1.7.9.5

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

[FanWu]

On 06/03/2014 03:37 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.
> 3.Remove the disable ops in struct pinmux_ops
>
> 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>;
> }
> 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();
>
> 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 choose 2# solution, we need to
> 1) Should mark all the setting in disable state as disabled ahead of enabling.
> 2) Avoid the possible HW glitch by removing the HW disable ops in
> in the pinmux_disable_setting
> If we disable all of the settings 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.(HW glitch)
>
> 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 ----
>   include/linux/pinctrl/pinmux.h |    2 --
>   3 files changed, 5 insertions(+), 25 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
> diff --git a/include/linux/pinctrl/pinmux.h b/include/linux/pinctrl/pinmux.h
> index c153950..3097aaf 100644
> --- a/include/linux/pinctrl/pinmux.h
> +++ b/include/linux/pinctrl/pinmux.h
> @@ -70,8 +70,6 @@ struct pinmux_ops {
>   				  unsigned * const num_groups);
>   	int (*enable) (struct pinctrl_dev *pctldev, unsigned func_selector,
>   		       unsigned group_selector);
> -	void (*disable) (struct pinctrl_dev *pctldev, unsigned func_selector,
> -			 unsigned group_selector);
>   	int (*gpio_request_enable) (struct pinctrl_dev *pctldev,
>   				    struct pinctrl_gpio_range *range,
>   				    unsigned offset);
>

Dear All,

I have updated the patch and the main modification includes:
1.To remove the disable ops in the struct pinmux_ops.
2.To short the patch comments.

Please help to review.

For the "signed-off" part, I added the "Stephen" because I change the 
code inline comments according to the suggestion from Stephen.

Please share your suggestion about the new version patch.

Great thanks for this!

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

[Stephen Warren]

On 06/03/2014 01:48 AM, FanWu wrote:
> On 06/03/2014 03:37 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.
>> 3.Remove the disable ops in struct pinmux_ops
...
> For the "signed-off" part, I added the "Stephen" because I change the
> code inline comments according to the suggestion from Stephen.

The point is that you can NEVER write S-o-b for someone else, only for
yourself. Given the small contribution from me in this patch, the
following would be better ways to credit me:

1) Not bother since it's a tiny comment

2) In patch description:
(includes comment fixes from Stephen Warren)

3) In patch description final paragraph, before your S-o-b:
Based-on-work-by: Stephen Warren <swarren@nvidia.com>

(although (3) isn't really appropriate here, since my contribution was
tiny compared to the overall patch, and came after the original patch
rather than being inspiration for it)

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

[Stephen Warren]

On 06/03/2014 01:37 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.
> 3.Remove the disable ops in struct pinmux_ops
...
> Signed-off-by: Fan Wu <fwu@marvell.com>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>

As I mentioned in my previous email, I didn't sign this off. I made some
suggestions for a better alternative in that email.

If I *had* written that s-o-b, then it should be before yours in the
patch description since you handled the patch last.

> diff --git a/include/linux/pinctrl/pinmux.h b/include/linux/pinctrl/pinmux.h

> @@ -70,8 +70,6 @@ struct pinmux_ops {
>  				  unsigned * const num_groups);
>  	int (*enable) (struct pinctrl_dev *pctldev, unsigned func_selector,
>  		       unsigned group_selector);
> -	void (*disable) (struct pinctrl_dev *pctldev, unsigned func_selector,
> -			 unsigned group_selector);

This will cause a compile failure, since many drivers still set the
.disable function pointer. You need to update all the driver files to
remove those functions too. There's quite a bit of code in some of those
functions, so you'd need the relevant driver maintainers to confirm it's
OK to remove it. I think only the owners of pinctrl-egra and
pinctrl-single have ack'd this concept so far.

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

[FanWu]

On 06/04/2014 12:49 AM, Stephen Warren wrote:
> On 06/03/2014 01:37 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.
>> 3.Remove the disable ops in struct pinmux_ops
> ...
>> Signed-off-by: Fan Wu <fwu@marvell.com>
>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>
> As I mentioned in my previous email, I didn't sign this off. I made some
> suggestions for a better alternative in that email.
>
> If I *had* written that s-o-b, then it should be before yours in the
> patch description since you handled the patch last.
>

The Signed-off didn't bother me.
I will Choose your option 2# and thanks for your suggestion about this :)


>> diff --git a/include/linux/pinctrl/pinmux.h b/include/linux/pinctrl/pinmux.h
>
>> @@ -70,8 +70,6 @@ struct pinmux_ops {
>>   				  unsigned * const num_groups);
>>   	int (*enable) (struct pinctrl_dev *pctldev, unsigned func_selector,
>>   		       unsigned group_selector);
>> -	void (*disable) (struct pinctrl_dev *pctldev, unsigned func_selector,
>> -			 unsigned group_selector);
>
> This will cause a compile failure, since many drivers still set the
> .disable function pointer. You need to update all the driver files to
> remove those functions too. There's quite a bit of code in some of those
> functions, so you'd need the relevant driver maintainers to confirm it's
> OK to remove it. I think only the owners of pinctrl-egra and
> pinctrl-single have ack'd this concept so far.
>

For this part, I think I mentioned this before, simply removing disable 
ops will introduce the compiling error.
I think there are several ways to handle this:
1. Don't remove the disable ops in struct pinmux_ops in this patch but 
to remove the disable ops in struct pinmux_ops after the another patch 
is merged, which is used to remove all of the disable ops user in all 
drivers.
2. Just remove the disable ops in pinmux_ops in this patch, and make a 
another patch ASAP to remove all the disable ops user in all drivers.
3. Remove the disable ops in struct pinmux_ops and remove all the 
disable ops user in all drivers, all in this patch.


For the solution 2, I just think it may be not a good way to include so 
much content in a patch, which are not in a same code level.

I am just inclined to use solution 1# or 3#.

Please share your comments.

Great thanks for this !

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

[Stephen Warren]

On 06/03/2014 07:28 PM, FanWu wrote:
> On 06/04/2014 12:49 AM, Stephen Warren wrote:
>> On 06/03/2014 01:37 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.
>>> 3.Remove the disable ops in struct pinmux_ops
>> ...
>>> Signed-off-by: Fan Wu <fwu@marvell.com>
>>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>>
>> As I mentioned in my previous email, I didn't sign this off. I made some
>> suggestions for a better alternative in that email.
>>
>> If I *had* written that s-o-b, then it should be before yours in the
>> patch description since you handled the patch last.
>>
> 
> The Signed-off didn't bother me.
> I will Choose your option 2# and thanks for your suggestion about this :)
> 
> 
>>> diff --git a/include/linux/pinctrl/pinmux.h
>>> b/include/linux/pinctrl/pinmux.h
>>
>>> @@ -70,8 +70,6 @@ struct pinmux_ops {
>>>                     unsigned * const num_groups);
>>>       int (*enable) (struct pinctrl_dev *pctldev, unsigned
>>> func_selector,
>>>                  unsigned group_selector);
>>> -    void (*disable) (struct pinctrl_dev *pctldev, unsigned
>>> func_selector,
>>> -             unsigned group_selector);
>>
>> This will cause a compile failure, since many drivers still set the
>> .disable function pointer. You need to update all the driver files to
>> remove those functions too. There's quite a bit of code in some of those
>> functions, so you'd need the relevant driver maintainers to confirm it's
>> OK to remove it. I think only the owners of pinctrl-egra and
>> pinctrl-single have ack'd this concept so far.
>>
> 
> For this part, I think I mentioned this before, simply removing disable
> ops will introduce the compiling error.
> I think there are several ways to handle this:
> 1. Don't remove the disable ops in struct pinmux_ops in this patch but
> to remove the disable ops in struct pinmux_ops after the another patch
> is merged, which is used to remove all of the disable ops user in all
> drivers.
> 2. Just remove the disable ops in pinmux_ops in this patch, and make a
> another patch ASAP to remove all the disable ops user in all drivers.
> 3. Remove the disable ops in struct pinmux_ops and remove all the
> disable ops user in all drivers, all in this patch.
> 
> 
> For the solution 2, I just think it may be not a good way to include so
> much content in a patch, which are not in a same code level.
> 
> I am just inclined to use solution 1# or 3#.

I would expect option 3.