Fix more issues with generic pinconf bindings

PATCH In review Heiko Stübner 4 participants 20 messages Jun 25, 2013 – Jun 27, 2013

Subsystems (2)

Device tree bindings Pin control

lore

Share & tools

lore

Heiko Stübner

author

2013-06-25 12:55 UTC #0 lore raw

cover Fix more issues with generic pinconf bindings

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

Signature · 1 line

1.7.10.4

Heiko Stübner

author

2013-06-25 12:55 UTC #1 lore raw

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

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 <[email protected]>

---
 Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt |    5 ++---
 include/linux/pinctrl/pinconf-generic.h                        |    5 ++++-
 2 files changed, 6 insertions(+), 4 deletions(-)

Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt +2 -3

index 2d730e3..7498bdc 100644
@@ -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.

include/linux/pinctrl/pinconf-generic.h +4 -1

index 10ad996..48aa4ba 100644
@@ -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.

Signature · 1 line

1.7.10.4

Linus Walleij

2013-06-25 13:14 UTC #2 lore raw

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

Quoted 9 lines · 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 <[email protected]>

Patch applied.

Yours,
Linus Walleij

Heiko Stübner

author

2013-06-25 12:56 UTC #3 lore raw

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

In reply to 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 <[email protected]>

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

Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt +2 -1

index 7498bdc..788ab09 100644
@@ -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

drivers/pinctrl/pinconf-generic.c +1 -1

index 794dad7..2b271d5 100644
@@ -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"),

include/linux/pinctrl/pinconf-generic.h +1 -1

index 48aa4ba..bf7e989 100644
@@ -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

Signature · 1 line

1.7.10.4

Linus Walleij

2013-06-25 13:15 UTC #4 lore raw

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

Quoted 7 lines · 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 <[email protected]>

Patch applied.

Yours,
Linus Walleij

Heiko Stübner

author

2013-06-25 12:56 UTC #5 lore raw

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

In reply to 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 <[email protected]>

---
 .../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(-)

Documentation/devicetree/bindings/pinctrl/img,tz1090-pdc- renamed from Documentation/devicetree/bindings/pinctrl/img,tz1090-pdc-pinctrl.txt +0 -0

pinctrl.txt
index 9f7a85b..a186181 100644

Documentation/devicetree/bindings/pinctrl/img,tz1090-pdc-pinctrl.txt +1 -4

@@ -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

Documentation/devicetree/bindings/pinctrl/img,tz1090-pinctrl.txt +1 -6

index 39bfd9c..4b27c99 100644
@@ -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

drivers/pinctrl/pinctrl-tz1090-pdc.c +0 -5

index 12e4808..d4f12cc 100644
@@ -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;

drivers/pinctrl/pinctrl-tz1090.c +0 -5

index 02ff3a2..4edae08 100644
@@ -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;

Signature · 1 line

1.7.10.4

James Hogan

2013-06-25 13:05 UTC #6 lore raw

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

Hi Heiko,

Quoted 2 lines · 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

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

Quoted 6 lines

release.
Therefore remove it again from the only current user, tz1090.
Signed-off-by Heiko Stuebner <[email protected]>
---

<snip>

Quoted 23 lines

drivers/pinctrl/pinctrl-tz1090-pdc.c +0 -5

index 12e4808..d4f12cc 100644
@@ -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;

drivers/pinctrl/pinctrl-tz1090.c +0 -0

index 02ff3a2..4edae08 100644
@@ -1834,11 +1834,6 @@ static int tz1090_pinconf_group_reg(struct pinctrl_dev *pctldev,
 		*width = 1;

drivers/pinctrl/pinctrl-tz1090.c static int tz1090_pinconf_group_reg(struct pinctrl_dev *pctldev, · line 1839

Quoted 10 lines

drivers/pinctrl/pinctrl-tz1090.c +0 -5

 		*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

Heiko Stübner

author

2013-06-25 13:21 UTC #7 lore raw

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

Am Dienstag, 25. Juni 2013, 15:05:05 schrieb James Hogan:

Quoted 56 lines

Hi Heiko,
Quoted 2 lines · 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
s/tought/thought/
s/offical/official/
Quoted 6 lines
release.
Therefore remove it again from the only current user, tz1090.
Signed-off-by Heiko Stuebner <[email protected]>
---
<snip>
Quoted 38 lines
(quoted diff) +0 -0
b/drivers/pinctrl/pinctrl-tz1090-pdc.c index 12e4808..d4f12cc 100644
drivers/pinctrl/pinctrl-tz1090-pdc.c +0 -5
@@ -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;
drivers/pinctrl/pinctrl-tz1090-pdc.c +0 -0
 	case PIN_CONFIG_DRIVE_STRENGTH:
 		*shift = REG_GPIO_CONTROL2_PDC_DR_S;
 		*width = 2;
(quoted diff) +0 -0
b/drivers/pinctrl/pinctrl-tz1090.c index 02ff3a2..4edae08 100644
drivers/pinctrl/pinctrl-tz1090.c +0 -5
@@ -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;

Quoted 8 lines

drivers/pinctrl/pinctrl-tz1090.c static int tz1090_pinconf_group_reg(struct pinctrl_dev *pctldev, · line 1839
Quoted 3 lines
drivers/pinctrl/pinctrl-tz1090.c +0 -0
 	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

James Hogan

Acked-by

2013-06-25 13:27 UTC #8 lore raw

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

Quoted 66 lines · Heiko Stübner

Am Dienstag, 25. Juni 2013, 15:05:05 schrieb James Hogan:
Quoted 56 lines
Hi Heiko,
Quoted 2 lines · 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
s/tought/thought/
s/offical/official/
Quoted 6 lines
release.
Therefore remove it again from the only current user, tz1090.
Signed-off-by Heiko Stuebner <[email protected]>
---
<snip>
Quoted 38 lines
(quoted diff) +0 -0
b/drivers/pinctrl/pinctrl-tz1090-pdc.c index 12e4808..d4f12cc 100644
drivers/pinctrl/pinctrl-tz1090-pdc.c +0 -5
@@ -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;
drivers/pinctrl/pinctrl-tz1090-pdc.c +0 -0
 	case PIN_CONFIG_DRIVE_STRENGTH:
 		*shift = REG_GPIO_CONTROL2_PDC_DR_S;
 		*width = 2;
(quoted diff) +0 -0
b/drivers/pinctrl/pinctrl-tz1090.c index 02ff3a2..4edae08 100644
drivers/pinctrl/pinctrl-tz1090.c +0 -5
@@ -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;
Quoted 8 lines
drivers/pinctrl/pinctrl-tz1090.c static int tz1090_pinconf_group_reg(struct pinctrl_dev *pctldev, · line 1839
Quoted 3 lines
drivers/pinctrl/pinctrl-tz1090.c +0 -0
 	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).

Quoted 7 lines

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 <[email protected]>

Cheers
James

Linus Walleij

2013-06-25 13:32 UTC #9 lore raw

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

Quoted 11 lines · James Hogan

Quoted 5 lines · Heiko Stübner
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 <[email protected]>

OK applied this so we have a clean slate.

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

Yours,
Linus Walleij

James Hogan

2013-06-25 13:50 UTC #10 lore raw

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

Quoted 9 lines · Linus Walleij

Quoted 8 lines · James Hogan
Quoted 5 lines · Heiko Stübner
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.

Quoted 7 lines

Quoted 3 lines
Okay, fair enough.
Acked-by James Hogan <[email protected]>
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

Linus Walleij

2013-06-25 15:39 UTC #11 lore raw

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

Quoted 9 lines · James Hogan

Quoted 1 line · Linus Walleij
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:

Quoted 6 lines

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;

Quoted 4 lines

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

Stephen Warren

2013-06-25 21:40 UTC #12 lore raw

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

Quoted 17 lines · Linus Walleij

Quoted 9 lines · James Hogan
Quoted 1 line · Linus Walleij
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:
Quoted 6 lines
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.

Quoted 8 lines

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.

Linus Walleij

2013-06-25 13:22 UTC #13 lore raw

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

In reply to James Hogan

Quoted 48 lines · James Hogan

Hi Heiko,

Quoted 2 lines · 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

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

Quoted 6 lines

release.
Therefore remove it again from the only current user, tz1090.
Signed-off-by Heiko Stuebner <[email protected]>
---

<snip>

Quoted 30 lines

drivers/pinctrl/pinctrl-tz1090-pdc.c +0 -5

index 12e4808..d4f12cc 100644
@@ -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;

drivers/pinctrl/pinctrl-tz1090.c +0 -5

index 02ff3a2..4edae08 100644
@@ -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:

Quoted 8 lines

drivers/pinctrl/pinctrl-tz1090.c static int tz1090_pinconf_group_reg(struct pinctrl_dev *pctldev, · line 1839
Quoted 3 lines
drivers/pinctrl/pinctrl-tz1090.c +0 -0
              *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

James Hogan

2013-06-25 14:57 UTC #14 lore raw

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

Quoted 9 lines · Linus Walleij

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).

Quoted 5 lines

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

Stephen Warren

2013-06-25 21:46 UTC #15 lore raw

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

Quoted 18 lines · James Hogan

Quoted 9 lines · Linus Walleij
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).
Quoted 5 lines
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:

Quoted 7 lines

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.

Linus Walleij

2013-06-27 08:32 UTC #16 lore raw

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

Quoted 9 lines · Stephen Warren

Quoted 7 lines · James Hogan
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.

Quoted 8 lines

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

Heiko Stübner

author

2013-06-25 12:57 UTC #17 lore raw

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

In reply to 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 <[email protected]>

---
 .../devicetree/bindings/pinctrl/pinctrl-bindings.txt         |   10 ----------
 drivers/pinctrl/pinconf-generic.c                            |    3 ---
 2 files changed, 13 deletions(-)

Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt +0 ~~-10~~

index 788ab09..aeb3c99 100644
@@ -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

drivers/pinctrl/pinconf-generic.c +0 -3

index 2b271d5..8594f03 100644
@@ -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, },

Signature · 1 line

1.7.10.4

Linus Walleij

2013-06-25 13:34 UTC #18 lore raw

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

Quoted 8 lines · 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 <[email protected]>

Patch applied.

Yours,
Linus Walleij

James Hogan

Reviewed-by on the v1 series

2013-06-25 13:16 UTC #19 lore raw

In reply to Heiko Stübner

Quoted 10 lines · 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.

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 <[email protected]>

Cheers
James