Message-ID: <53797279.1010109@marvell.com>
Date:	Mon, 19 May 2014 10:54:49 +0800
From:	FanWu <fwu@...vell.com>
To:	Stephen Warren <swarren@...dotorg.org>
CC:	Linus Walleij <linus.walleij@...aro.org>,
	ext Tony Lindgren <tony@...mide.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Stephen Warren <swarren@...dia.com>,
	Chao Xie <cxie4@...vell.com>, Yilu Mao <ylmao@...vell.com>,
	Ning Jiang <njiang1@...vell.com>,
	Xiaofan Tian <tianxf@...vell.com>,
	Fangsuo Wu <fswu@...vell.com>
Subject: Re: [PATCH] pinctrl: add params in disable_setting for different
 usage

On 05/17/2014 03:53 AM, Stephen Warren wrote:
> On 05/16/2014 10:21 AM, Linus Walleij wrote:
>> On Wed, May 14, 2014 at 4:01 AM,  <fwu@...vell.com> wrote:
>>
>>> From: Fan Wu <fwu@...vell.com>
>>>
>>> The patch added params in disable_setting to differ the two possible usage,
>>> 1.Only want to disable the pin setting in SW aspect, param can be set to "0"
>>> 2.Want to disable the pin setting in both HW and SW aspect, param can be set to "1";
>>>
>>> The reason why to do this is that:
>>> To avoid duplicated enable_setting operation without disabling operation which will
>>> let Pin's desc->mux_usecount keep being added.
>>>
>>> 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 the two state is same, 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:
>>
>> Hm this is a quite interesting thing if we can get it in place, but
>> I need Stephen's consent, also Tony should have a look at this as
>> I know he's had the same problem as you in pinctrl-single.
>
> I only briefly looked at the patch, but it probably solves/hides the
> immediate problem.
>
> However, rather than doing this, why not just remove
> pinmux_disable_setting() completely. It doesn't make sense to "disable a
> mux selection" (some value is always selected in the mux register field)
> any more than it does to "disable a drive strength selection". We don't
> have a pinconf_disable_setting(), and couldn't really add one if we
> wanted. For consistency, let's just remove pinmux_disable_setting(). Do
> you agree?
>

Dear, Stephen and Guys,

Sorry for late due to some personal affairs in Weekend time.

I don't think it is a proper way to remove pinmux_disable_setting 
directly without changing any other code, like no change on the code in 
pinmux_enable_setting.

Talking about the pinmux_disable_operation, in SW aspect, we also need 
to consider the "pinmux_enable_setting" operation.
For the "pinmux_enable_setting" operation, there is some SW level code 
logic, like pin_request.
Do you think it is a acceptable way to remove the SW level code logic 
from the "pinmux_enable_setting" path, because there will be no 
corresponding operation in reverse order in pinmux_disable_setting after 
applying our possible change?
At least, I think this way may be a considerable change in Pinmux 
framework, right?

Talking about HW aspect,
I think the solution you mentioned is indeed a good way to solve the 
problem for some HW vendor's SoC chip, but may be not that intact solution.

In my understanding, the pinmux operation, like enabling and disabling, 
is to change pin's(pins') multi-function from funcion_M to function_N.
And, the "pinconf" enabling function is used to change the attributes of 
the pin, like Pull_Up/Down, DriveStrength(Low/Medium/Fast) and etc.

The pinmux disabling operation will be called in the following case in 
current pinmux framework:
1. when pin(s) is/are freed or error handler when configure it(them) and 
finally the pin will be changed to a disabled/safe state if defined by 
vendor.
2. in the pinctrl_select_state function

The item 2# is just the thing we talked about in this loop and we reach 
agreement that the item 2# is not useful.

I think the item 1# is still useful for some vendor if they defined the 
disabled/safe multi-function for a pin. They may expect the pin is 
changed to the disabled/safe state for saving power or some security 
reason.

Thus, I think we should keep the disable_pinmux_setting in pinmux code.

Do you think what I mentioned is an acceptable and not that aggressive 
solution?

Please correct me if anything wrong.



For another topics:
1. There is no disable_pinconf: I think this is a issue. When the pin's 
mux setting is changed, the pinconf setting should also be changed, at 
least, the pinmux code here should offer the user a chance(interface) to 
decide whether to change the pinconf setting. Thus, we may need to add 
pinconf disable function.
2. If the vendor use pinctrl-single driver, the 
"pinctrl-single,function-off" implementation is not useful in practical 
usage. The "pinctrl-single,function-off" is parsed when pinctrl-single 
driver probe phase and the instance setting of 
"pinctrl-single,function-off" will be used for all pins setting. 
Practically, I think different pins may have different disabled/safe.

Great thanks for this long term discussion :)



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists