[PATCH 0/4] Fix more issues with generic pinconf bindings

[PATCH 0/4] Fix more issues with generic pinconf bindings

[Heiko Stübner]

As Stephen Warren pointed out, some pinconf bindings had deficits
either in documentation or in the whole thought process.

Therefore this series tries to fix these issues by improving
documentation and removing the real controversial bindings for now.

Of the changed pinconf bindings only the slew-rate had an acutal
current user (since yesterday). James I hope it's ok to simply
remove the slew-rate handling from the tz1090 driver until we can
handle it correctly in dt.


Heiko Stuebner (4):
  pinctrl: more clarifications for generic pull configs
  pinctrl: set unit for debounce time pinconfig to usec
  pinctrl: remove slew-rate parameter from tz1090
  pinctrl: remove bindings for pinconf options needing more thought

 .../bindings/pinctrl/img,tz1090-pdc-pinctrl.txt      |    5 +----
 .../bindings/pinctrl/img,tz1090-pinctrl.txt          |    7 +------
 .../devicetree/bindings/pinctrl/pinctrl-bindings.txt |   18 ++++--------------
 drivers/pinctrl/pinconf-generic.c                    |    5 +----
 drivers/pinctrl/pinctrl-tz1090-pdc.c                 |    5 -----
 drivers/pinctrl/pinctrl-tz1090.c                     |    5 -----
 include/linux/pinctrl/pinconf-generic.h              |    7 +++++--
 7 files changed, 12 insertions(+), 40 deletions(-)

-- 
1.7.10.4

[PATCH 1/4] pinctrl: more clarifications for generic pull configs

[Heiko Stübner]

PULL_PIN_DEFAULT is meant for hardware completely hiding any pull
settings from the driver, so that it's really only possible to turn
the pull on or off, but it not being possible to determine any
pull settings from software.

Also the binding-documentation for the pull arguments did not match
the changes to the expected values.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt |    5 ++---
 include/linux/pinctrl/pinconf-generic.h                        |    5 ++++-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
index 2d730e3..7498bdc 100644
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
@@ -165,9 +165,8 @@ output-high		- set the pin to output mode with high level
 
 Arguments for parameters:
 
-- bias-pull-up, -down and -pin-default take as optional argument 0 to disable
-  the pull, on hardware supporting it the pull strength in Ohm. bias-disable
-  will also disable any active pull.
+- bias-pull-up, -down and -pin-default take as optional argument on hardware
+  supporting it the pull strength in Ohm. bias-disable will disable the pull.
 
 - drive-strength takes as argument the target strength in mA.
 
diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
index 10ad996..48aa4ba 100644
--- a/include/linux/pinctrl/pinconf-generic.h
+++ b/include/linux/pinctrl/pinconf-generic.h
@@ -41,7 +41,10 @@
  *	impedance to GROUND). If the argument is != 0 pull-down is enabled,
  *	if it is 0, pull-down is total, i.e. the pin is connected to GROUND.
  * @PIN_CONFIG_BIAS_PULL_PIN_DEFAULT: the pin will be pulled up or down based
- *	on embedded knowledge of the controller, like current mux function.
+ *	on embedded knowledge of the controller hardware, like current mux
+ *	function. The pull direction and possibly strength too will normally
+ *	be decided completely inside the hardware block and not be readable
+ *	from the kernel side.
  *	If the argument is != 0 pull up/down is enabled, if it is 0, the
  *	configuration is ignored. The proper way to disable it is to use
  *	@PIN_CONFIG_BIAS_DISABLE.
-- 
1.7.10.4

Re: [PATCH 1/4] pinctrl: more clarifications for generic pull configs

[Linus Walleij]

On Tue, Jun 25, 2013 at 2:55 PM, Heiko Stübner <heiko@sntech.de> wrote:

> PULL_PIN_DEFAULT is meant for hardware completely hiding any pull
> settings from the driver, so that it's really only possible to turn
> the pull on or off, but it not being possible to determine any
> pull settings from software.
>
> Also the binding-documentation for the pull arguments did not match
> the changes to the expected values.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>

Patch applied.

Yours,
Linus Walleij

[PATCH 2/4] pinctrl: set unit for debounce time pinconfig to usec

[Heiko Stübner]

Currently the debounce time pinconfig option uses an unspecified
"time units" unit. As pinconfig options should use SI units and a
real unit is also necessary for generic dt bindings, change it
to usec. Currently no driver is using the generic pinconfig option
for this, so the unit change is safe to do.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt |    3 ++-
 drivers/pinctrl/pinconf-generic.c                              |    2 +-
 include/linux/pinctrl/pinconf-generic.h                        |    2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
index 7498bdc..788ab09 100644
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
@@ -173,7 +173,8 @@ Arguments for parameters:
 - input-schmitt takes as argument the adjustable hysteresis in a
   driver-specific format
 
-- input-debounce takes the debounce time as argument or 0 to disable debouncing
+- input-debounce takes the debounce time in usec as argument
+  or 0 to disable debouncing
 
 - power-source argument is the custom value describing the source to select
 
diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c
index 794dad7..2b271d5 100644
--- a/drivers/pinctrl/pinconf-generic.c
+++ b/drivers/pinctrl/pinconf-generic.c
@@ -49,7 +49,7 @@ static struct pin_config_item conf_items[] = {
 	PCONFDUMP(PIN_CONFIG_DRIVE_STRENGTH, "output drive strength", "mA"),
 	PCONFDUMP(PIN_CONFIG_INPUT_SCHMITT_ENABLE, "input schmitt enabled", NULL),
 	PCONFDUMP(PIN_CONFIG_INPUT_SCHMITT, "input schmitt trigger", NULL),
-	PCONFDUMP(PIN_CONFIG_INPUT_DEBOUNCE, "input debounce", "time units"),
+	PCONFDUMP(PIN_CONFIG_INPUT_DEBOUNCE, "input debounce", "usec"),
 	PCONFDUMP(PIN_CONFIG_POWER_SOURCE, "pin power source", "selector"),
 	PCONFDUMP(PIN_CONFIG_SLEW_RATE, "slew rate", NULL),
 	PCONFDUMP(PIN_CONFIG_LOW_POWER_MODE, "pin low power", "mode"),
diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
index 48aa4ba..bf7e989 100644
--- a/include/linux/pinctrl/pinconf-generic.h
+++ b/include/linux/pinctrl/pinconf-generic.h
@@ -70,7 +70,7 @@
  *	setting pins to this mode.
  * @PIN_CONFIG_INPUT_DEBOUNCE: this will configure the pin to debounce mode,
  *	which means it will wait for signals to settle when reading inputs. The
- *	argument gives the debounce time on a custom format. Setting the
+ *	argument gives the debounce time in usecs. Setting the
  *	argument to zero turns debouncing off.
  * @PIN_CONFIG_POWER_SOURCE: if the pin can select between different power
  *	supplies, the argument to this parameter (on a custom format) tells
-- 
1.7.10.4

Re: [PATCH 2/4] pinctrl: set unit for debounce time pinconfig to usec

[Linus Walleij]

On Tue, Jun 25, 2013 at 2:56 PM, Heiko Stübner <heiko@sntech.de> wrote:

> Currently the debounce time pinconfig option uses an unspecified
> "time units" unit. As pinconfig options should use SI units and a
> real unit is also necessary for generic dt bindings, change it
> to usec. Currently no driver is using the generic pinconfig option
> for this, so the unit change is safe to do.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>

Patch applied.

Yours,
Linus Walleij

[PATCH 3/4] pinctrl: remove slew-rate parameter from tz1090

[Heiko Stübner]

As the binding for slew-rate is under discussion and seems to need
more tought it will get removed for now, so it doesn't get an offical
release.

Therefore remove it again from the only current user, tz1090.

Signed-off-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
---
 .../devicetree/bindings/pinctrl/img,tz1090-pdc-pinctrl.txt      |    5 +----
 .../devicetree/bindings/pinctrl/img,tz1090-pinctrl.txt          |    7 +------
 drivers/pinctrl/pinctrl-tz1090-pdc.c                            |    5 -----
 drivers/pinctrl/pinctrl-tz1090.c                                |    5 -----
 4 files changed, 2 insertions(+), 20 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/img,tz1090-pdc-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/img,tz1090-pdc-
pinctrl.txt
index 9f7a85b..a186181 100644
--- a/Documentation/devicetree/bindings/pinctrl/img,tz1090-pdc-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/img,tz1090-pdc-pinctrl.txt
@@ -44,9 +44,6 @@ Optional subnode-properties:
   - bias-pull-down
   - input-schmitt-enable
   - input-schmitt-disable
-  - slew-rate: Integer, control slew rate of pins.
-      0: slow (half frequency)
-      1: fast
   - drive-strength: Integer, control drive strength of pins in mA.
       2: 2mA
       4: 4mA
@@ -83,7 +80,7 @@ Valid values for pin and group names are:
 
   drive groups:
 
-    These support input-schmitt-enable, input-schmitt-disable, slew-rate,
+    These support input-schmitt-enable, input-schmitt-disable,
     drive-strength, low-power-enable, and low-power-disable.
 
     pdc
diff --git a/Documentation/devicetree/bindings/pinctrl/img,tz1090-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/img,tz1090-pinctrl.txt
index 39bfd9c..4b27c99 100644
--- a/Documentation/devicetree/bindings/pinctrl/img,tz1090-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/img,tz1090-pinctrl.txt
@@ -44,9 +44,6 @@ Optional subnode-properties:
   - bias-pull-down
   - input-schmitt-enable
   - input-schmitt-disable
-  - slew-rate: Integer, control slew rate of pins.
-      0: slow (half frequency)
-      1: fast
   - drive-strength: Integer, control drive strength of pins in mA.
       2: 2mA
       4: 4mA
@@ -124,7 +121,6 @@ Valid values for pin and group names are:
         function:              afe, ts_out_0.
         input-schmitt-enable:  supported.
         input-schmitt-disable: supported.
-        slew-rate:             supported.
         drive-strength:        supported.
     pdm_d
         pins:                  pdm_d.
@@ -153,12 +149,11 @@ Valid values for pin and group names are:
                                lcd_trace, phy_ringosc.
         input-schmitt-enable:  supported.
         input-schmitt-disable: supported.
-        slew-rate:             supported.
         drive-strength:        supported.
 
   drive groups:
 
-    These all support input-schmitt-enable, input-schmitt-disable, slew-rate,
+    These all support input-schmitt-enable, input-schmitt-disable,
     and drive-strength.
 
     jtag
diff --git a/drivers/pinctrl/pinctrl-tz1090-pdc.c b/drivers/pinctrl/pinctrl-tz1090-pdc.c
index 12e4808..d4f12cc 100644
--- a/drivers/pinctrl/pinctrl-tz1090-pdc.c
+++ b/drivers/pinctrl/pinctrl-tz1090-pdc.c
@@ -809,11 +809,6 @@ static int tz1090_pdc_pinconf_group_reg(struct pinctrl_dev *pctldev,
 		*width = 1;
 		*map = tz1090_pdc_boolean_map;
 		break;
-	case PIN_CONFIG_SLEW_RATE:
-		*shift = REG_GPIO_CONTROL2_PDC_SR_S;
-		*width = 1;
-		*map = tz1090_pdc_boolean_map;
-		break;
 	case PIN_CONFIG_DRIVE_STRENGTH:
 		*shift = REG_GPIO_CONTROL2_PDC_DR_S;
 		*width = 2;
diff --git a/drivers/pinctrl/pinctrl-tz1090.c b/drivers/pinctrl/pinctrl-tz1090.c
index 02ff3a2..4edae08 100644
--- a/drivers/pinctrl/pinctrl-tz1090.c
+++ b/drivers/pinctrl/pinctrl-tz1090.c
@@ -1834,11 +1834,6 @@ static int tz1090_pinconf_group_reg(struct pinctrl_dev *pctldev,
 		*width = 1;
 		*map = tz1090_boolean_map;
 		break;
-	case PIN_CONFIG_SLEW_RATE:
-		*reg = REG_PINCTRL_SR;
-		*width = 1;
-		*map = tz1090_boolean_map;
-		break;
 	case PIN_CONFIG_DRIVE_STRENGTH:
 		*reg = REG_PINCTRL_DR;
 		*width = 2;
-- 
1.7.10.4

Re: [PATCH 3/4] pinctrl: remove slew-rate parameter from tz1090

[James Hogan]

Hi Heiko,

On 25/06/13 13:56, Heiko Stübner wrote:
> As the binding for slew-rate is under discussion and seems to need
> more tought it will get removed for now, so it doesn't get an offical

s/tought/thought/
s/offical/official/

> release.
> 
> Therefore remove it again from the only current user, tz1090.
> 
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---

<snip>

> diff --git a/drivers/pinctrl/pinctrl-tz1090-pdc.c b/drivers/pinctrl/pinctrl-tz1090-pdc.c
> index 12e4808..d4f12cc 100644
> --- a/drivers/pinctrl/pinctrl-tz1090-pdc.c
> +++ b/drivers/pinctrl/pinctrl-tz1090-pdc.c
> @@ -809,11 +809,6 @@ static int tz1090_pdc_pinconf_group_reg(struct pinctrl_dev *pctldev,
>  		*width = 1;
>  		*map = tz1090_pdc_boolean_map;
>  		break;
> -	case PIN_CONFIG_SLEW_RATE:
> -		*shift = REG_GPIO_CONTROL2_PDC_SR_S;
> -		*width = 1;
> -		*map = tz1090_pdc_boolean_map;
> -		break;
>  	case PIN_CONFIG_DRIVE_STRENGTH:
>  		*shift = REG_GPIO_CONTROL2_PDC_DR_S;
>  		*width = 2;
> diff --git a/drivers/pinctrl/pinctrl-tz1090.c b/drivers/pinctrl/pinctrl-tz1090.c
> index 02ff3a2..4edae08 100644
> --- a/drivers/pinctrl/pinctrl-tz1090.c
> +++ b/drivers/pinctrl/pinctrl-tz1090.c
> @@ -1834,11 +1834,6 @@ static int tz1090_pinconf_group_reg(struct pinctrl_dev *pctldev,
>  		*width = 1;
>  		*map = tz1090_boolean_map;
>  		break;
> -	case PIN_CONFIG_SLEW_RATE:
> -		*reg = REG_PINCTRL_SR;
> -		*width = 1;
> -		*map = tz1090_boolean_map;
> -		break;
>  	case PIN_CONFIG_DRIVE_STRENGTH:
>  		*reg = REG_PINCTRL_DR;
>  		*width = 2;
> 

I don't see the harm in keeping the handling of PIN_CONFIG_SLEW_RATE,
since PIN_CONFIG_SLEW_RATE is still present and you only seem to be
removing the device tree bindings (which is the only important bit from
the DT ABI point of view).

Cheers
James

Re: [PATCH 3/4] pinctrl: remove slew-rate parameter from tz1090

[Heiko Stübner]

Am Dienstag, 25. Juni 2013, 15:05:05 schrieb James Hogan:
> Hi Heiko,
> 
> On 25/06/13 13:56, Heiko Stübner wrote:
> > As the binding for slew-rate is under discussion and seems to need
> > more tought it will get removed for now, so it doesn't get an offical
> 
> s/tought/thought/
> s/offical/official/
> 
> > release.
> > 
> > Therefore remove it again from the only current user, tz1090.
> > 
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > ---
> 
> <snip>
> 
> > diff --git a/drivers/pinctrl/pinctrl-tz1090-pdc.c
> > b/drivers/pinctrl/pinctrl-tz1090-pdc.c index 12e4808..d4f12cc 100644
> > --- a/drivers/pinctrl/pinctrl-tz1090-pdc.c
> > +++ b/drivers/pinctrl/pinctrl-tz1090-pdc.c
> > @@ -809,11 +809,6 @@ static int tz1090_pdc_pinconf_group_reg(struct
> > pinctrl_dev *pctldev,
> > 
> >  		*width = 1;
> >  		*map = tz1090_pdc_boolean_map;
> >  		break;
> > 
> > -	case PIN_CONFIG_SLEW_RATE:
> > -		*shift = REG_GPIO_CONTROL2_PDC_SR_S;
> > -		*width = 1;
> > -		*map = tz1090_pdc_boolean_map;
> > -		break;
> > 
> >  	case PIN_CONFIG_DRIVE_STRENGTH:
> >  		*shift = REG_GPIO_CONTROL2_PDC_DR_S;
> >  		*width = 2;
> > 
> > diff --git a/drivers/pinctrl/pinctrl-tz1090.c
> > b/drivers/pinctrl/pinctrl-tz1090.c index 02ff3a2..4edae08 100644
> > --- a/drivers/pinctrl/pinctrl-tz1090.c
> > +++ b/drivers/pinctrl/pinctrl-tz1090.c
> > @@ -1834,11 +1834,6 @@ static int tz1090_pinconf_group_reg(struct
> > pinctrl_dev *pctldev,
> > 
> >  		*width = 1;
> >  		*map = tz1090_boolean_map;
> >  		break;
> > 
> > -	case PIN_CONFIG_SLEW_RATE:
> > -		*reg = REG_PINCTRL_SR;
> > -		*width = 1;
> > -		*map = tz1090_boolean_map;
> > -		break;
> > 
> >  	case PIN_CONFIG_DRIVE_STRENGTH:
> >  		*reg = REG_PINCTRL_DR;
> >  		*width = 2;
> 
> I don't see the harm in keeping the handling of PIN_CONFIG_SLEW_RATE,
> since PIN_CONFIG_SLEW_RATE is still present and you only seem to be
> removing the device tree bindings (which is the only important bit from
> the DT ABI point of view).

I'm partial to this :-)

My thoughts were that this code would never be reached when the parsing was 
removed and to not cause confusion to the driver when an acceptable binding 
was found for slew-rate.

But it of course also doesn't hurt to stay in.


Heiko

Re: [PATCH 3/4] pinctrl: remove slew-rate parameter from tz1090

[James Hogan]

On 25/06/13 14:21, Heiko Stübner wrote:
> Am Dienstag, 25. Juni 2013, 15:05:05 schrieb James Hogan:
>> Hi Heiko,
>>
>> On 25/06/13 13:56, Heiko Stübner wrote:
>>> As the binding for slew-rate is under discussion and seems to need
>>> more tought it will get removed for now, so it doesn't get an offical
>>
>> s/tought/thought/
>> s/offical/official/
>>
>>> release.
>>>
>>> Therefore remove it again from the only current user, tz1090.
>>>
>>> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
>>> ---
>>
>> <snip>
>>
>>> diff --git a/drivers/pinctrl/pinctrl-tz1090-pdc.c
>>> b/drivers/pinctrl/pinctrl-tz1090-pdc.c index 12e4808..d4f12cc 100644
>>> --- a/drivers/pinctrl/pinctrl-tz1090-pdc.c
>>> +++ b/drivers/pinctrl/pinctrl-tz1090-pdc.c
>>> @@ -809,11 +809,6 @@ static int tz1090_pdc_pinconf_group_reg(struct
>>> pinctrl_dev *pctldev,
>>>
>>>  		*width = 1;
>>>  		*map = tz1090_pdc_boolean_map;
>>>  		break;
>>>
>>> -	case PIN_CONFIG_SLEW_RATE:
>>> -		*shift = REG_GPIO_CONTROL2_PDC_SR_S;
>>> -		*width = 1;
>>> -		*map = tz1090_pdc_boolean_map;
>>> -		break;
>>>
>>>  	case PIN_CONFIG_DRIVE_STRENGTH:
>>>  		*shift = REG_GPIO_CONTROL2_PDC_DR_S;
>>>  		*width = 2;
>>>
>>> diff --git a/drivers/pinctrl/pinctrl-tz1090.c
>>> b/drivers/pinctrl/pinctrl-tz1090.c index 02ff3a2..4edae08 100644
>>> --- a/drivers/pinctrl/pinctrl-tz1090.c
>>> +++ b/drivers/pinctrl/pinctrl-tz1090.c
>>> @@ -1834,11 +1834,6 @@ static int tz1090_pinconf_group_reg(struct
>>> pinctrl_dev *pctldev,
>>>
>>>  		*width = 1;
>>>  		*map = tz1090_boolean_map;
>>>  		break;
>>>
>>> -	case PIN_CONFIG_SLEW_RATE:
>>> -		*reg = REG_PINCTRL_SR;
>>> -		*width = 1;
>>> -		*map = tz1090_boolean_map;
>>> -		break;
>>>
>>>  	case PIN_CONFIG_DRIVE_STRENGTH:
>>>  		*reg = REG_PINCTRL_DR;
>>>  		*width = 2;
>>
>> I don't see the harm in keeping the handling of PIN_CONFIG_SLEW_RATE,
>> since PIN_CONFIG_SLEW_RATE is still present and you only seem to be
>> removing the device tree bindings (which is the only important bit from
>> the DT ABI point of view).
> 
> I'm partial to this :-)
> 
> My thoughts were that this code would never be reached when the parsing was 
> removed and to not cause confusion to the driver when an acceptable binding 
> was found for slew-rate.
> 
> But it of course also doesn't hurt to stay in.

Okay, fair enough.

Acked-by: James Hogan <james.hogan@imgtec.com>

Cheers
James

Re: [PATCH 3/4] pinctrl: remove slew-rate parameter from tz1090

[Linus Walleij]

On Tue, Jun 25, 2013 at 3:27 PM, James Hogan <james.hogan@imgtec.com> wrote:
> On 25/06/13 14:21, Heiko Stübner wrote:

>> My thoughts were that this code would never be reached when the parsing was
>> removed and to not cause confusion to the driver when an acceptable binding
>> was found for slew-rate.
>>
>> But it of course also doesn't hurt to stay in.
>
> Okay, fair enough.
>
> Acked-by: James Hogan <james.hogan@imgtec.com>

OK applied this so we have a clean slate.

Patches adding it back in with SI measures will be welcomed!

Yours,
Linus Walleij

Re: [PATCH 3/4] pinctrl: remove slew-rate parameter from tz1090

[James Hogan]

On 25/06/13 14:32, Linus Walleij wrote:
> On Tue, Jun 25, 2013 at 3:27 PM, James Hogan <james.hogan@imgtec.com> wrote:
>> On 25/06/13 14:21, Heiko Stübner wrote:
> 
>>> My thoughts were that this code would never be reached when the parsing was
>>> removed and to not cause confusion to the driver when an acceptable binding
>>> was found for slew-rate.
>>>
>>> But it of course also doesn't hurt to stay in.
>>
>> Okay, fair enough.
>>
>> Acked-by: James Hogan <james.hogan@imgtec.com>
> 
> OK applied this so we have a clean slate.
> 
> Patches adding it back in with SI measures will be welcomed!

Okay.

So the most descriptive documentation I can find for the slew rate in
the TZ1090 pin config hardware is basically:

0: slow (half frequency)
1: fast

Sounds like this pretty much precludes it from having a generic DT
binding unless it can be mapped to some physical value, so I'll add a
tz1090,slew-rate and use PIN_CONFIG_END+1 instead of PIN_CONFIG_SLEW_RATE.

Anybody object to me adding an argument to
pinconf_generic_parse_dt_config() so that an additional
pinconf_generic_dt_params array can be optionally passed in? That way I
can share the generic pinconf dt parsing code.

Cheers
James

Re: [PATCH 3/4] pinctrl: remove slew-rate parameter from tz1090

[Linus Walleij]

On Tue, Jun 25, 2013 at 3:50 PM, James Hogan <james.hogan@imgtec.com> wrote:
> On 25/06/13 14:32, Linus Walleij wrote:

>> Patches adding it back in with SI measures will be welcomed!
>
> Okay.
>
> So the most descriptive documentation I can find for the slew rate in
> the TZ1090 pin config hardware is basically:
>
> 0: slow (half frequency)
> 1: fast
>
> Sounds like this pretty much precludes it from having a generic DT
> binding unless it can be mapped to some physical value, so I'll add a
> tz1090,slew-rate and use PIN_CONFIG_END+1 instead of PIN_CONFIG_SLEW_RATE.

Okay... ut the Nomadik pin controller incidentally have a
similar definition: NMK_GPIO_LOWEMI. By slashing the
slew rate in half the EMI is of course reduced so that was
another name for the same thing.

Maybe this is something that should just be boolean?

slewrate-reduced-slope;

?

> Anybody object to me adding an argument to
> pinconf_generic_parse_dt_config() so that an additional
> pinconf_generic_dt_params array can be optionally passed in? That way I
> can share the generic pinconf dt parsing code.

Hm hm hm, that sounds more like it could be a separate
utility function. Then it could help to parse any such array
of custom bindings.

Yours,
Linus Walleij

Re: [PATCH 3/4] pinctrl: remove slew-rate parameter from tz1090

[Stephen Warren]

On 06/25/2013 09:39 AM, Linus Walleij wrote:
> On Tue, Jun 25, 2013 at 3:50 PM, James Hogan <james.hogan@imgtec.com> wrote:
>> On 25/06/13 14:32, Linus Walleij wrote:
> 
>>> Patches adding it back in with SI measures will be welcomed!
>>
>> Okay.
>>
>> So the most descriptive documentation I can find for the slew rate in
>> the TZ1090 pin config hardware is basically:
>>
>> 0: slow (half frequency)
>> 1: fast
>>
>> Sounds like this pretty much precludes it from having a generic DT
>> binding unless it can be mapped to some physical value, so I'll add a
>> tz1090,slew-rate and use PIN_CONFIG_END+1 instead of PIN_CONFIG_SLEW_RATE.
> 
> Okay... ut the Nomadik pin controller incidentally have a
> similar definition: NMK_GPIO_LOWEMI. By slashing the
> slew rate in half the EMI is of course reduced so that was
> another name for the same thing.
> 
> Maybe this is something that should just be boolean?
> 
> slewrate-reduced-slope;

Tegra has, IIRC, 4 different slew rates. I'm not sure that a
Boolean/dual-state value would be appropriate here; it'd be limited to
supporting some arbitrary cases rather than being something generic.

Re: [PATCH 3/4] pinctrl: remove slew-rate parameter from tz1090

[Linus Walleij]

On Tue, Jun 25, 2013 at 3:05 PM, James Hogan <james.hogan@imgtec.com> wrote:
> Hi Heiko,
>
> On 25/06/13 13:56, Heiko Stübner wrote:
>> As the binding for slew-rate is under discussion and seems to need
>> more tought it will get removed for now, so it doesn't get an offical
>
> s/tought/thought/
> s/offical/official/
>
>> release.
>>
>> Therefore remove it again from the only current user, tz1090.
>>
>> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
>> ---
>
> <snip>
>
>> diff --git a/drivers/pinctrl/pinctrl-tz1090-pdc.c b/drivers/pinctrl/pinctrl-tz1090-pdc.c
>> index 12e4808..d4f12cc 100644
>> --- a/drivers/pinctrl/pinctrl-tz1090-pdc.c
>> +++ b/drivers/pinctrl/pinctrl-tz1090-pdc.c
>> @@ -809,11 +809,6 @@ static int tz1090_pdc_pinconf_group_reg(struct pinctrl_dev *pctldev,
>>               *width = 1;
>>               *map = tz1090_pdc_boolean_map;
>>               break;
>> -     case PIN_CONFIG_SLEW_RATE:
>> -             *shift = REG_GPIO_CONTROL2_PDC_SR_S;
>> -             *width = 1;
>> -             *map = tz1090_pdc_boolean_map;
>> -             break;
>>       case PIN_CONFIG_DRIVE_STRENGTH:
>>               *shift = REG_GPIO_CONTROL2_PDC_DR_S;
>>               *width = 2;
>> diff --git a/drivers/pinctrl/pinctrl-tz1090.c b/drivers/pinctrl/pinctrl-tz1090.c
>> index 02ff3a2..4edae08 100644
>> --- a/drivers/pinctrl/pinctrl-tz1090.c
>> +++ b/drivers/pinctrl/pinctrl-tz1090.c
>> @@ -1834,11 +1834,6 @@ static int tz1090_pinconf_group_reg(struct pinctrl_dev *pctldev,
>>               *width = 1;
>>               *map = tz1090_boolean_map;
>>               break;
>> -     case PIN_CONFIG_SLEW_RATE:
>> -             *reg = REG_PINCTRL_SR;
>> -             *width = 1;
>> -             *map = tz1090_boolean_map;
>> -             break;
>>       case PIN_CONFIG_DRIVE_STRENGTH:
>>               *reg = REG_PINCTRL_DR;
>>               *width = 2;
>>
>
> I don't see the harm in keeping the handling of PIN_CONFIG_SLEW_RATE,
> since PIN_CONFIG_SLEW_RATE is still present and you only seem to be
> removing the device tree bindings (which is the only important bit from
> the DT ABI point of view).

I would actually like to be pretty strict about the kernel-internal meaning of
these parameters as well.

Can't we just try to come up with a patch that nails down the meaning of
slew rate in some meaningful manner then?

So according to:
http://en.wikipedia.org/wiki/Slew_rate
a proper expression for slew rate would be dV/dt i.e.
something like microvolts per microsecond (which then just
becomes volts/second).

What we need to figure out is what range will be applicable within
reasonable doubt for current scenarios and the next few years.

What are your datasheets specifying here, and what would be
a proper measure?

Yours,
Linus Walleij

Re: [PATCH 3/4] pinctrl: remove slew-rate parameter from tz1090

[James Hogan]

On 25/06/13 14:22, Linus Walleij wrote:
> Can't we just try to come up with a patch that nails down the meaning of
> slew rate in some meaningful manner then?
> 
> So according to:
> http://en.wikipedia.org/wiki/Slew_rate
> a proper expression for slew rate would be dV/dt i.e.
> something like microvolts per microsecond (which then just
> becomes volts/second).
> 
> What we need to figure out is what range will be applicable within
> reasonable doubt for current scenarios and the next few years.
> 
> What are your datasheets specifying here, and what would be
> a proper measure?

My datasheet says:

0: slow (half frequency)
1: fast

I just got a reply back from a hardware engineer, who said that the
relationship with the actual volts/usec will depend on both the drive
strength and the load on the pad, and that a definite answer probably
requires running a simulation.

Cheers
James

Re: [PATCH 3/4] pinctrl: remove slew-rate parameter from tz1090

[Stephen Warren]

On 06/25/2013 08:57 AM, James Hogan wrote:
> On 25/06/13 14:22, Linus Walleij wrote:
>> Can't we just try to come up with a patch that nails down the meaning of
>> slew rate in some meaningful manner then?
>>
>> So according to:
>> http://en.wikipedia.org/wiki/Slew_rate
>> a proper expression for slew rate would be dV/dt i.e.
>> something like microvolts per microsecond (which then just
>> becomes volts/second).
>>
>> What we need to figure out is what range will be applicable within
>> reasonable doubt for current scenarios and the next few years.
>>
>> What are your datasheets specifying here, and what would be
>> a proper measure?
> 
> My datasheet says:
> 
> 0: slow (half frequency)
> 1: fast
> 
> I just got a reply back from a hardware engineer, who said that the
> relationship with the actual volts/usec will depend on both the drive
> strength and the load on the pad, and that a definite answer probably
> requires running a simulation.

Tegra is similar here. The docs just say (for a 2-bit field expressed in
binary) "Code 11 is the least slewing of the signal, code 00 is the
highest slewing of the signal".

I'm not sure that a generic parameter actually needs specific units. Why
can't we simply specify the units as HW-defined, even while using a
standardized DT property name and kernel-internal enum to represent the
concept of slew rate? Even the order of whether 0 or 3 is highest or
lowest need not be mandated by the spec?

Note also that Tegra has separate rising and falling slew-rate
configuration.

And the slew rate is influenced by a "low-power mode" setting.

And as for James, I imagine the actual dV/dT is influenced by the
voltage on the IO rail for a particular board, since I'm pretty sure we
have some IOs that can operate at multiple different voltages, simply
based on whatever voltage is supplied for that pin/block's VDD.

Re: [PATCH 3/4] pinctrl: remove slew-rate parameter from tz1090

[Linus Walleij]

On Tue, Jun 25, 2013 at 11:46 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 06/25/2013 08:57 AM, James Hogan wrote:
>> 0: slow (half frequency)
>> 1: fast
>>
>> I just got a reply back from a hardware engineer, who said that the
>> relationship with the actual volts/usec will depend on both the drive
>> strength and the load on the pad, and that a definite answer probably
>> requires running a simulation.
>
> Tegra is similar here. The docs just say (for a 2-bit field expressed in
> binary) "Code 11 is the least slewing of the signal, code 00 is the
> highest slewing of the signal".
>
> I'm not sure that a generic parameter actually needs specific units. Why
> can't we simply specify the units as HW-defined, even while using a
> standardized DT property name and kernel-internal enum to represent the
> concept of slew rate?

That is fine I guess, that is how it is currently defined for the
kernel internals in
<linux/pinconf/pinconf-generic.h>:

 * @PIN_CONFIG_SLEW_RATE: if the pin can select slew rate, the argument to
 *      this parameter (on a custom format) tells the driver which alternative
 *      slew rate to use.

It's just that the very phrase "device tree" bring out the
standards body slow-and-impersonal considering all aspects
of the world side of me.

Unless someone from the DT camp think it is horrible to have
this value vendor-specific I'm all game.

But I guess it needs some binding doc for each and every pin
controller in that case, so referring to the generic binding will
not work. (And the driver needs to do some range checking
etc I guess.)

Yours,
Linus Walleij

[PATCH 4/4] pinctrl: remove bindings for pinconf options needing more thought

[Heiko Stübner]

Some options currently take arguments in unspecified driver-specific units.
As pointed out by Stephen Warren, driver specific values should not be part
of generic devicetree bindings describing the hardware.

Therefore remove the critical bindings again, before they become part of
an official release.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 .../devicetree/bindings/pinctrl/pinctrl-bindings.txt         |   10 ----------
 drivers/pinctrl/pinconf-generic.c                            |    3 ---
 2 files changed, 13 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
index 788ab09..aeb3c99 100644
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
@@ -154,10 +154,7 @@ drive-open-source	- drive with open source
 drive-strength		- sink or source at most X mA
 input-schmitt-enable	- enable schmitt-trigger mode
 input-schmitt-disable	- disable schmitt-trigger mode
-input-schmitt		- run in schmitt-trigger mode with hysteresis X
 input-debounce		- debounce mode with debound time X
-power-source		- select power source X
-slew-rate		- use slew-rate X
 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
@@ -170,16 +167,9 @@ Arguments for parameters:
 
 - drive-strength takes as argument the target strength in mA.
 
-- input-schmitt takes as argument the adjustable hysteresis in a
-  driver-specific format
-
 - input-debounce takes the debounce time in usec as argument
   or 0 to disable debouncing
 
-- power-source argument is the custom value describing the source to select
-
-- slew-rate takes as argument the target rate in a driver-specific format
-
 All parameters not listed here, do not take an argument.
 
 More in-depth documentation on these parameters can be found in
diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c
index 2b271d5..8594f03 100644
--- a/drivers/pinctrl/pinconf-generic.c
+++ b/drivers/pinctrl/pinconf-generic.c
@@ -161,10 +161,7 @@ static struct pinconf_generic_dt_params dt_params[] = {
 	{ "drive-strength", PIN_CONFIG_DRIVE_STRENGTH, 0 },
 	{ "input-schmitt-enable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 1 },
 	{ "input-schmitt-disable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 0 },
-	{ "input-schmitt", PIN_CONFIG_INPUT_SCHMITT, 0 },
 	{ "input-debounce", PIN_CONFIG_INPUT_DEBOUNCE, 0 },
-	{ "power-source", PIN_CONFIG_POWER_SOURCE, 0 },
-	{ "slew-rate", PIN_CONFIG_SLEW_RATE, 0 },
 	{ "low-power-enable", PIN_CONFIG_LOW_POWER_MODE, 1 },
 	{ "low-power-disable", PIN_CONFIG_LOW_POWER_MODE, 0 },
 	{ "output-low", PIN_CONFIG_OUTPUT, 0, },
-- 
1.7.10.4

Re: [PATCH 4/4] pinctrl: remove bindings for pinconf options needing more thought

[Linus Walleij]

On Tue, Jun 25, 2013 at 2:57 PM, Heiko Stübner <heiko@sntech.de> wrote:

> Some options currently take arguments in unspecified driver-specific units.
> As pointed out by Stephen Warren, driver specific values should not be part
> of generic devicetree bindings describing the hardware.
>
> Therefore remove the critical bindings again, before they become part of
> an official release.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH 0/4] Fix more issues with generic pinconf bindings

[James Hogan]

On 25/06/13 13:55, Heiko Stübner wrote:
> As Stephen Warren pointed out, some pinconf bindings had deficits
> either in documentation or in the whole thought process.
> 
> Therefore this series tries to fix these issues by improving
> documentation and removing the real controversial bindings for now.
> 
> Of the changed pinconf bindings only the slew-rate had an acutal
> current user (since yesterday). James I hope it's ok to simply
> remove the slew-rate handling from the tz1090 driver until we can
> handle it correctly in dt.

Other than my comment about patch 3 (which since it's the only case I
don't mind too much, it can easily be added back again in the future):

Reviewed-by: James Hogan <james.hogan@imgtec.com>

Cheers
James