Message-ID: <20170627120626.GA1067@spreadtrum.com>
Date:   Tue, 27 Jun 2017 20:06:26 +0800
From:   Baolin Wang <baolin.wang@...eadtrum.com>
To:     Rob Herring <robh@...nel.org>, <linus.walleij@...aro.org>,
        <mark.rutland@....com>, <linux-gpio@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <broonie@...nel.org>, <baolin.wang@...aro.org>
Subject: Re: [PATCH v4 1/3] pinctrl: Add sleep related configuration

Hi,

On 二,  6月 27, 2017 at 04:21:32下午 +0800, Baolin Wang wrote:
> On 一,  6月 26, 2017 at 11:13:48上午 -0500, Rob Herring wrote:
> > On Wed, Jun 21, 2017 at 07:55:37PM +0800, Baolin Wang wrote:
> > > In some scenarios, we should set some pins as input/output/pullup/pulldown
> > > when the specified system goes into deep sleep mode, then when the system
> > > goes into deep sleep mode, these pins will be set automatically by hardware.
> > > 
> > > Usually we can set the "sleep" state to set sleep related config, but one SoC
> > > usually has not only one system (especially for mobile SoC), some systems on
> > > the SoC which did not run linux kernel, they can not select the "sleep" state
> > > when they go into deep sleep mode.
> > 
> > The wording here is not very clear. I think what you mean is some pins 
> > are not controlled by any specific driver in the OS, but need to be 
> > controlled when entering sleep mode.
> 
> Yes, that is what I meaning, sorry for confusing.
> 
> > 
> > > Thus we introduce some sleep related config into pinconf-generic for users to
> > > configure.
> > > 
> > > Signed-off-by: Baolin Wang <baolin.wang@...eadtrum.com>
> > > ---
> > >  - Add this patch since v4.
> > > ---
> > >  .../bindings/pinctrl/pinctrl-bindings.txt          |   12 ++++++++++++
> > >  drivers/pinctrl/pinconf-generic.c                  |   10 ++++++++++
> > >  include/linux/pinctrl/pinconf-generic.h            |   14 ++++++++++++++
> > >  3 files changed, 36 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> > > index bf3f7b0..e098059 100644
> > > --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> > > +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> > > @@ -236,6 +236,18 @@ low-power-enable	- enable low power mode
> > >  low-power-disable	- disable low power mode
> > >  output-low		- set the pin to output mode with low level
> > >  output-high		- set the pin to output mode with high level
> > > +sleep-bias-pull-up	- pull up the pin when the specified system goes into
> > > +			  deep sleep mode
> > > +sleep-bias-pull-down	- pull down the pin when the specified system goes into
> > > +			  deep sleep mode
> > > +sleep-input-enable	- enable input on pin when the specified system goes
> > > +			  into deep sleep mode (no effect on output)
> > > +sleep-intput-disable	- disable input on pin when the specified system goes
> > > +			  into deep sleep mode (no effect on output)
> > > +sleep-output-low	- set the pin to output mode with low level when the
> > > +			  specified system goes into deep sleep mode
> > > +sleep-output-high	- set the pin to output mode with high level when the
> > > +			  specified system goes into deep sleep mode
> > >  slew-rate		- set the slew rate
> > 
> > I don't really like having 2 ways to define pin setup and this doesn't 
> > scale if I need to define 3 states. Couldn't we create pin state 
> > definitions and have a pinctrl-n property within the pin controller 
> > node to handle all the unhandled pins?
> 
> As LinusW also suggest we can create one "sleep" state and program them
> into registers at early point (like: after probing pinctrl driver). So I
> think I can introduce one called "early-sleep" state which need select it
> after initializing pinctrl driver.

After more investigation, I do not think it is a good solution to create one
state containing sleep related configs and select it at early point.

First these sleep related configs are pins' attributes, they should be set
depanding on users situation, which means the state containing sleep related
configs should be selected by users. We can not create one "sleep-xxx" state
containing all pins' sleep related configs, and select it when initializing
pinctrl driver.

Second if we create one "sleep-xxx" state containing sleep related configs,
which means we should set one pin's configuration in 2 places.

If we introduce "sleep-input-enable" config, we can set the pin's config
as below:
vio_sd0_ms_3: regctrl3 {
	pins = "SC9860_RFCTL30", "SC9860_RFCTL31", "SC9860_RFCTL32";
	function = "func1";
	sprd,sleep-mode = <0x3>;
	sleep-input-enable;
};

But If we create one extra "sleep-xxx" state for sleep-related configs,
it will be like:
grp1: regctrl3 {
	pins = "SC9860_RFCTL30", "SC9860_RFCTL31";
	function = "func1";
	sprd,sleep-mode = <0x3>;
};

sleep-input: input_grp {
	pins = "SC9860_RFCTL30", "SC9860_RFCTL31", "SC9860_RFCTL32";
	input-enable;
};

pinctrl-names = "sleep-input";
pinctrl-0 = <&sleep-input>;

"sleep-input" state will be selected when initializing pinctrl driver, "grp1"
will be selected by user to set other pin configuration.

Then we need config "SC9860_RFCTL30" pin in 2 different places, which is
more inconvenient for users.

Accoring to above explanation, I think we should introduce these standard
sleep related configs for users, but if you still have strong objection for
it, I think I should introduce some SoC-specific attributes (something like
"sprd,sleep-input") for our driver. LinusW and Rob, do you have any good
suggestion? Thanks.

> 
> > 
> > Rob

Powered by blists - more mailing lists