pinctrl: fix some issues with new pinconfig dt parsing

PATCH In review Heiko Stübner 5 participants 21 messages Jun 14, 2013 – Jun 24, 2013

Subsystems (3)

Device tree bindings Arm Rockchip Pin control

lore

Share & tools

lore

Heiko Stübner

author maintainer

2013-06-14 15:41 UTC #0 lore raw

cover pinctrl: fix some issues with new pinconfig dt parsing

Some issues with the recently submitted generic pinconfig parsing from dt came up, so fix these in this follow-up series.

Hopefully I did catch all of them.

Tested on my rk3066 device.

Heiko Stuebner (5):
pinctrl: update the documentation for some pinconfig params
pinctrl: clarify some dt pinconfig options
pinctrl: handle zero found dt pinconfig properties better
pinctrl: dynamically alloc temp array when parsing dt pinconf options
pinctrl: rockchip: correctly handle arguments of pinconf options

 .../bindings/pinctrl/pinctrl-bindings.txt          |   22 +++++++++++-
 drivers/pinctrl/pinconf-generic.c                  |   38 +++++++++++++++-----
 drivers/pinctrl/pinctrl-rockchip.c                 |   37 ++++++++++++++++---
 include/linux/pinctrl/pinconf-generic.h            |   18 ++++++----
 4 files changed, 93 insertions(+), 22 deletions(-)

Signature · 1 line

1.7.10.4

Heiko Stübner

author maintainer

2013-06-14 15:42 UTC #1 lore raw

PATCH 1/5 pinctrl: update the documentation for some pinconfig params

The BIAS_DISABLE and BIAS_HIGH_IMPEDANCE generic pinconfig options were missing information about their argument - which should be ignored.

Also the BIAS_PULL_* options may have the pull strength as argument when they are activated, while simpler hardware can use any non-0 value for it.

Update the kerneldoc to reflect this.

Signed-off-by Heiko Stuebner <[email protected]>

---
 include/linux/pinctrl/pinconf-generic.h |   18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

include/linux/pinctrl/pinconf-generic.h +11 -7

index d414a77..d1868bc 100644
@@ -23,27 +23,31 @@
  * @PIN_CONFIG_BIAS_DISABLE: disable any pin bias on the pin, a
  *	transition from say pull-up to pull-down implies that you disable
  *	pull-up in the process, this setting disables all biasing.
+ *	The argument is ignored.
  * @PIN_CONFIG_BIAS_HIGH_IMPEDANCE: the pin will be set to a high impedance
  *	mode, also know as "third-state" (tristate) or "high-Z" or "floating".
  *	On output pins this effectively disconnects the pin, which is useful
  *	if for example some other pin is going to drive the signal connected
  *	to it for a while. Pins used for input are usually always high
- *	impedance.
+ *	impedance. The argument is ignored.
  * @PIN_CONFIG_BIAS_BUS_HOLD: the pin will be set to weakly latch so that it
  *	weakly drives the last value on a tristate bus, also known as a "bus
  *	holder", "bus keeper" or "repeater". This allows another device on the
  *	bus to change the value by driving the bus high or low and switching to
  *	tristate. The argument is ignored.
  * @PIN_CONFIG_BIAS_PULL_UP: the pin will be pulled up (usually with high
- *	impedance to VDD). If the argument is != 0 pull-up is enabled,
- *	if it is 0, pull-up is disabled.
+ *	impedance to VDD). If the argument is != 0 pull-up is enabled. On
+ *	hardware supporting this, the argument should contain the strength of
+ *	the pull in Ohm. If it is 0, pull-up is disabled.
  * @PIN_CONFIG_BIAS_PULL_DOWN: the pin will be pulled down (usually with high
- *	impedance to GROUND). If the argument is != 0 pull-down is enabled,
- *	if it is 0, pull-down is disabled.
+ *	impedance to GROUND). If the argument is != 0 pull-down is enabled. On
+ *	hardware supporting this, the argument should contain the strength of
+ *	the pull in Ohm. If it is 0, pull-down is disabled.
  * @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.
- *	If the argument is != 0 pull up/down is enabled, if it is 0,
- *	the pull is disabled.
+ *	If the argument is != 0 pull up/down is enabled. On hardware supporting
+ *	this, the argument should contain the strength of the pull in Ohm.
+ *	If it is 0, pull is disabled.
  * @PIN_CONFIG_DRIVE_PUSH_PULL: the pin will be driven actively high and
  *	low, this is the most typical case and is typically achieved with two
  *	active transistors on the output. Setting this config will enable

Signature · 1 line

1.7.10.4

Linus Walleij

2013-06-16 10:26 UTC #2 lore raw

Re: [PATCH 1/5] pinctrl: update the documentation for some pinconfig params

Quoted 10 lines · Heiko St?bner

The BIAS_DISABLE and BIAS_HIGH_IMPEDANCE generic pinconfig options were missing information about their argument - which should be ignored.
Also the BIAS_PULL_* options may have the pull strength as argument when they are activated, while simpler hardware can use any non-0 value for it.
Update the kerneldoc to reflect this.
Signed-off-by Heiko Stuebner <[email protected]>

I'm holding this patch off.

Quoted 6 lines

* @PIN_CONFIG_BIAS_PULL_UP: the pin will be pulled up (usually with high

- *     impedance to VDD). If the argument is != 0 pull-up is enabled,
- *     if it is 0, pull-up is disabled.
+ *     impedance to VDD). If the argument is != 0 pull-up is enabled. On
+ *     hardware supporting this, the argument should contain the strength of
+ *     the pull in Ohm. If it is 0, pull-up is disabled.

As noted by Laurent, a pull-up of 0 Ohm is a short-circuit (bascially the TOTAL pull-up) and it is pretty counter-intuitive to have that mean "disable pull-up". Can we avoid this?

Quoted 6 lines

* @PIN_CONFIG_BIAS_PULL_DOWN: the pin will be pulled down (usually with high

- *     impedance to GROUND). If the argument is != 0 pull-down is enabled,
- *     if it is 0, pull-down is disabled.
+ *     impedance to GROUND). If the argument is != 0 pull-down is enabled. On
+ *     hardware supporting this, the argument should contain the strength of
+ *     the pull in Ohm. If it is 0, pull-down is disabled.

Dito.

Quoted 7 lines

* @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.
- *     If the argument is != 0 pull up/down is enabled, if it is 0,
- *     the pull is disabled.
+ *     If the argument is != 0 pull up/down is enabled. On hardware supporting
+ *     this, the argument should contain the strength of the pull in Ohm.
+ *     If it is 0, pull is disabled.

Dito.

Can't we rely on PIN_CONFIG_BIAS_DISABLE for all this?

Yours,
Linus Walleij

Heiko Stübner

author maintainer

2013-06-16 10:45 UTC #3 lore raw

Re: [PATCH 1/5] pinctrl: update the documentation for some pinconfig params

Am Sonntag, 16. Juni 2013, 12:26:38 schrieb Linus Walleij:

Quoted 49 lines · Heiko St?bner

Quoted 10 lines
The BIAS_DISABLE and BIAS_HIGH_IMPEDANCE generic pinconfig options were missing information about their argument - which should be ignored.
Also the BIAS_PULL_* options may have the pull strength as argument when they are activated, while simpler hardware can use any non-0 value for it.
Update the kerneldoc to reflect this.
Signed-off-by Heiko Stuebner <[email protected]>
I'm holding this patch off.
Quoted 8 lines
* @PIN_CONFIG_BIAS_PULL_UP: the pin will be pulled up (usually with high
- *     impedance to VDD). If the argument is != 0 pull-up is enabled,
- *     if it is 0, pull-up is disabled.
+ *     impedance to VDD). If the argument is != 0 pull-up is enabled. On
+ *     hardware supporting this, the argument should contain the
strength of + * the pull in Ohm. If it is 0, pull-up is disabled.
As noted by Laurent, a pull-up of 0 Ohm is a short-circuit (bascially the TOTAL pull-up) and it is pretty counter-intuitive to have that mean "disable pull-up". Can we avoid this?
Quoted 9 lines
* @PIN_CONFIG_BIAS_PULL_DOWN: the pin will be pulled down (usually with high
- * impedance to GROUND). If the argument is != 0 pull-down is
enabled, - * if it is 0, pull-down is disabled.
+ * impedance to GROUND). If the argument is != 0 pull-down is
enabled. On + * hardware supporting this, the argument should
contain the strength of + * the pull in Ohm. If it is 0, pull-down
is disabled.
Dito.
Quoted 9 lines
* @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.
- *     If the argument is != 0 pull up/down is enabled, if it is 0,
- *     the pull is disabled.
+ *     If the argument is != 0 pull up/down is enabled. On hardware
supporting + * this, the argument should contain the strength of the
pull in Ohm. + * If it is 0, pull is disabled.

Quoted 3 lines

Dito.
Can't we rely on PIN_CONFIG_BIAS_DISABLE for all this?

you're the boss on this, I'll do whatever you say :-)

I was going after the existing documentation of "bias-pull-X = <0>" being there to turn off this specific bias. So when reading the existing doc I was assuming somebody had needed this in the past.

Actually I'm too more comfortable with only using bias-disable.

So I'll redo this patch to remove the disabling of a pull via this method, if that's the right way to go.

Same with the rockchip driver.

Heiko

Linus Walleij

2013-06-16 12:26 UTC #4 lore raw

Re: [PATCH 1/5] pinctrl: update the documentation for some pinconfig params

Quoted 5 lines · Heiko St?bner

Am Sonntag, 16. Juni 2013, 12:26:38 schrieb Linus Walleij:
Quoted 1 line
Can't we rely on PIN_CONFIG_BIAS_DISABLE for all this?
you're the boss on this, I'll do whatever you say :-)

Oh that's not good. As can be seen from past experience I am often avoiding to do horrible design mistakes by other smart people telling me how wrong I am ...

Quoted 4 lines

Actually I'm too more comfortable with only using bias-disable.
So I'll redo this patch to remove the disabling of a pull via this method, if that's the right way to go.

I sent some patch on how I'd like it defined, please check it.

Quoted 1 line

Same with the rockchip driver.

Hm I merged that patch actually, but you can send a v2 if you want me to replace it. Or a patch on top, if preferred.

Yours,
Linus Walleij

Heiko Stübner

author maintainer

2013-06-14 15:42 UTC #5 lore raw

PATCH 2/5 pinctrl: clarify some dt pinconfig options

In reply to Heiko Stübner

The bias-pull-* options use values > 0 to indicate that the pull should be activated and optionally also indicate the strength of the pull. Therefore use an default value of 1 for these options.

Split the low-power-mode option into low-power-enable and -disable.

Update the documentation to describe the param arguments better.

Wrong default options

Reported-by James Hogan <[email protected]>

Signed-off-by Heiko Stuebner <[email protected]>

---
 .../bindings/pinctrl/pinctrl-bindings.txt          |   22 +++++++++++++++++++-
 drivers/pinctrl/pinconf-generic.c                  |    9 ++++----
 2 files changed, 26 insertions(+), 5 deletions(-)

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

index ef7cd57..2d730e3 100644
@@ -158,9 +158,29 @@ 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-mode		- low power mode
+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
 
+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.
+
+- 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 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
 <include/linux/pinctrl/pinconfig-generic.h>

drivers/pinctrl/pinconf-generic.c +5 -4

index d3c693c..dcf0371 100644
@@ -152,9 +152,9 @@ static struct pinconf_generic_dt_params dt_params[] = {
 	{ "bias-disable", PIN_CONFIG_BIAS_DISABLE, 0 },
 	{ "bias-high-impedance", PIN_CONFIG_BIAS_HIGH_IMPEDANCE, 0 },
 	{ "bias-bus-hold", PIN_CONFIG_BIAS_BUS_HOLD, 0 },
-	{ "bias-pull-up", PIN_CONFIG_BIAS_PULL_UP, 0 },
-	{ "bias-pull-down", PIN_CONFIG_BIAS_PULL_DOWN, 0 },
-	{ "bias-pull-pin-default", PIN_CONFIG_BIAS_PULL_PIN_DEFAULT, 0 },
+	{ "bias-pull-up", PIN_CONFIG_BIAS_PULL_UP, 1 },
+	{ "bias-pull-down", PIN_CONFIG_BIAS_PULL_DOWN, 1 },
+	{ "bias-pull-pin-default", PIN_CONFIG_BIAS_PULL_PIN_DEFAULT, 1 },
 	{ "drive-push-pull", PIN_CONFIG_DRIVE_PUSH_PULL, 0 },
 	{ "drive-open-drain", PIN_CONFIG_DRIVE_OPEN_DRAIN, 0 },
 	{ "drive-open-source", PIN_CONFIG_DRIVE_OPEN_SOURCE, 0 },

@@ -165,7 +165,8 @@ static struct pinconf_generic_dt_params dt_params[] = {
 	{ "input-debounce", PIN_CONFIG_INPUT_DEBOUNCE, 0 },
 	{ "power-source", PIN_CONFIG_POWER_SOURCE, 0 },
 	{ "slew-rate", PIN_CONFIG_SLEW_RATE, 0 },
-	{ "low-power-mode", PIN_CONFIG_LOW_POWER_MODE, 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, },
 	{ "output-high", PIN_CONFIG_OUTPUT, 1, },
 };

Signature · 1 line

1.7.10.4

Linus Walleij

2013-06-16 10:28 UTC #6 lore raw

Re: [PATCH 2/5] pinctrl: clarify some dt pinconfig options

Quoted 4 lines · Heiko St?bner

The bias-pull-* options use values > 0 to indicate that the pull should be activated and optionally also indicate the strength of the pull. Therefore use an default value of 1 for these options.

Quoted 8 lines

Split the low-power-mode option into low-power-enable and -disable.
Update the documentation to describe the param arguments better.
Wrong default options
Reported-by James Hogan <[email protected]>
Signed-off-by Heiko Stuebner <[email protected]>

Patch applied.

Yours,
Linus Walleij

Stephen Warren

2013-06-19 22:10 UTC #7 lore raw

Re: [PATCH 2/5] pinctrl: clarify some dt pinconfig options

In reply to Heiko Stübner

Quoted 11 lines · Heiko St?bner

The bias-pull-* options use values > 0 to indicate that the pull should be activated and optionally also indicate the strength of the pull. Therefore use an default value of 1 for these options.
Split the low-power-mode option into low-power-enable and -disable.
Update the documentation to describe the param arguments better.
Wrong default options
Reported-by James Hogan <[email protected]>

That blank line should be before the Reported-by not after it.

Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt input-schmitt - run in schmitt-trigger mode with hysteresis X · line 162

Quoted 7 lines

Signed-off-by Heiko Stuebner <[email protected]>
Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt +2 -1
 
-low-power-mode		- low power mode
+low-power-enable	- enable low power mode
+low-power-disable	- disable low power mode

Hmmm. That's changing the binding definition. What if somebody already wrote their device tree according previous definition?

It seems to be that tri-states are preferable for pinctrl DT:

no entry: do nothing
= 0: disable
= 1: enable

Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt input-schmitt - run in schmitt-trigger mode with hysteresis X · line 170

Quoted 5 lines

Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt +5 -0

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

Does this agree with the latest definition of the kernel-internal meaning of 0 for pull-up/down?

Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt input-schmitt - run in schmitt-trigger mode with hysteresis X · line 181

Quoted 8 lines

+- input-schmitt takes as argument the adjustable hysteresis in a
Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt +7 -0
+  driver-specific format
+
+- input-debounce takes the debounce time 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

If those things have driver-specific (note: should be DT-binding-specific, not driver-specific) values, then I'm not convinced that having a generic parameter name for them is a good idea; it makes things look the same when they aren't. By forcing each binding to include the vendor prefix on those properties and hence define a custom property name, you're making it clear that the semantics may be different.

Linus Walleij

2013-06-24 09:51 UTC #8 lore raw

Re: [PATCH 2/5] pinctrl: clarify some dt pinconfig options

Quoted 10 lines · Stephen Warren

Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt input-schmitt - run in schmitt-trigger mode with hysteresis X · line 162
Quoted 5 lines · Heiko St?bner
Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt +2 -1
 
-low-power-mode               - low power mode
+low-power-enable     - enable low power mode
+low-power-disable    - disable low power mode
Hmmm. That's changing the binding definition. What if somebody already wrote their device tree according previous definition?

It's not merged so see it as alterations to a WIP in the turners workshop or something.

Quoted 5 lines

It seems to be that tri-states are preferable for pinctrl DT:
no entry: do nothing
= 0: disable
= 1: enable

Better with explict enable/disable strings instead of <0> or <1> I think, but the semantic effect would be the same I guess, the upside with *enable/*disable strings is that we do not have to handle cases like tristate = <2>; ...

Quoted 8 lines

Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt input-schmitt - run in schmitt-trigger mode with hysteresis X · line 170
Quoted 5 lines
Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt +5 -0
+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.
Does this agree with the latest definition of the kernel-internal meaning of 0 for pull-up/down?

No that is wrong. Heiko, care to fix this binding doc?

Quoted 7 lines

Quoted 7 lines
+- input-schmitt takes as argument the adjustable hysteresis in a
Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt +6 -0
+  driver-specific format
+
+- input-debounce takes the debounce time as argument or 0 to disable debouncing
+
+- power-source argument is the custom value describing the source to select
+

Quoted 8 lines

Quoted 1 line
+- slew-rate takes as argument the target rate in a driver-specific format
If those things have driver-specific (note: should be DT-binding-specific, not driver-specific) values, then I'm not convinced that having a generic parameter name for them is a good idea; it makes things look the same when they aren't. By forcing each binding to include the vendor prefix on those properties and hence define a custom property name, you're making it clear that the semantics may be different.

Hmmm I don't think they're used right now, let's deal with them when we have something to showcase them with. Patches to delete the unclear bindings will be considered...

Yours,
Linus Walleij

Heiko Stübner

author maintainer

2013-06-14 15:43 UTC #9 lore raw

PATCH 3/5 pinctrl: handle zero found dt pinconfig properties better

In reply to Heiko Stübner

This adds a shortcut when no valid pinconf properties are found in the parsed dt node, to set the values immediately and return.

Signed-off-by Heiko Stuebner <[email protected]>

---
 drivers/pinctrl/pinconf-generic.c |    7 +++++++
 1 file changed, 7 insertions(+)

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

index dcf0371..ea9da17 100644
@@ -208,6 +208,13 @@ int pinconf_generic_parse_dt_config(struct device_node *np,
 		ncfg++;
 	}
 
+	/* no configs found at all */
+	if (ncfg == 0) {
+		*configs = NULL;
+		*nconfigs = 0;
+		return 0;
+	}
+
 	/*
 	 * Now limit the number of configs to the real number of
 	 * found properties.

Signature · 1 line

1.7.10.4

Linus Walleij

2013-06-16 10:29 UTC #10 lore raw

Re: [PATCH 3/5] pinctrl: handle zero found dt pinconfig properties better

Quoted 5 lines · Heiko St?bner

This adds a shortcut when no valid pinconf properties are found in the parsed dt node, to set the values immediately and return.
Suggested-by Laurent Pinchart <[email protected]>
Signed-off-by Heiko Stuebner <[email protected]>

Patch applied.

Yours,
Linus Walleij

Heiko Stübner

author maintainer

2013-06-14 15:43 UTC #11 lore raw

PATCH 4/5 pinctrl: dynamically alloc temp array when parsing dt pinconf options

In reply to Heiko Stübner

Allocating the temorary array in pinconf_generic_parse_dt_config on stack might cause problems later on, when the number of options grows over time. Therefore also allocate this array dynamically to be on the safe side.

Signed-off-by Heiko Stuebner <[email protected]>

---
 drivers/pinctrl/pinconf-generic.c |   24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

drivers/pinctrl/pinconf-generic.c +18 -6

index ea9da17..794dad7 100644
@@ -182,7 +182,7 @@ int pinconf_generic_parse_dt_config(struct device_node *np,
 				    unsigned long **configs,
 				    unsigned int *nconfigs)
 {
-	unsigned long cfg[ARRAY_SIZE(dt_params)];
+	unsigned long *cfg;
 	unsigned int ncfg = 0;
 	int ret;
 	int i;

@@ -191,6 +191,11 @@ int pinconf_generic_parse_dt_config(struct device_node *np,
 	if (!np)
 		return -EINVAL;
 
+	/* allocate a temporary array big enough to hold one of each option */
+	cfg = kzalloc(sizeof(*cfg) * ARRAY_SIZE(dt_params), GFP_KERNEL);
+	if (!cfg)
+		return -ENOMEM;
+
 	for (i = 0; i < ARRAY_SIZE(dt_params); i++) {
 		struct pinconf_generic_dt_params *par = &dt_params[i];
 		ret = of_property_read_u32(np, par->property, &val);

@@ -208,11 +213,13 @@ int pinconf_generic_parse_dt_config(struct device_node *np,
 		ncfg++;
 	}
 
+	ret = 0;
+
 	/* no configs found at all */
 	if (ncfg == 0) {
 		*configs = NULL;
 		*nconfigs = 0;
-		return 0;
+		goto out;
 	}
 
 	/*

@@ -220,11 +227,16 @@ int pinconf_generic_parse_dt_config(struct device_node *np,
 	 * found properties.
 	 */
 	*configs = kzalloc(ncfg * sizeof(unsigned long), GFP_KERNEL);
-	if (!*configs)
-		return -ENOMEM;
+	if (!*configs) {
+		ret = -ENOMEM;
+		goto out;
+	}
 
-	memcpy(*configs, &cfg, ncfg * sizeof(unsigned long));
+	memcpy(*configs, cfg, ncfg * sizeof(unsigned long));
 	*nconfigs = ncfg;
-	return 0;
+
+out:
+	kfree(cfg);
+	return ret;
 }
 #endif

Signature · 1 line

1.7.10.4

Linus Walleij

2013-06-16 10:31 UTC #12 lore raw

Re: [PATCH 4/5] pinctrl: dynamically alloc temp array when parsing dt pinconf options

Quoted 6 lines · Heiko St?bner

Allocating the temorary array in pinconf_generic_parse_dt_config on stack might cause problems later on, when the number of options grows over time. Therefore also allocate this array dynamically to be on the safe side.
Suggested-by Laurent Pinchart <[email protected]>
Signed-off-by Heiko Stuebner <[email protected]>

Patch applied.

Yours,
Linus Walleij

Heiko Stübner

author maintainer

2013-06-14 15:44 UTC #13 lore raw

PATCH 5/5 pinctrl: rockchip: correctly handle arguments of pinconf options

In reply to Heiko Stübner

Change the rockchip pinctrl driver to handle the arguments of 0 or 1 to the pull pinconfig options correctly, so that the pull gets disabled when either the bias_disable options is set or the pull option has the argument 0.

Signed-off-by Heiko Stuebner <[email protected]>

---
 drivers/pinctrl/pinctrl-rockchip.c |   37 +++++++++++++++++++++++++++++++-----
 1 file changed, 32 insertions(+), 5 deletions(-)

drivers/pinctrl/pinctrl-rockchip.c +32 -5

index c605b63..2568afc 100644
@@ -563,6 +563,25 @@ static const struct pinmux_ops rockchip_pmx_ops = {
  * Pinconf_ops handling
  */
 
+static bool rockchip_pinconf_pull_valid(struct rockchip_pin_ctrl *ctrl,
+					enum pin_config_param pull)
+{
+	/* rk3066b does support any pulls */
+	if (!ctrl->pull_offset)
+		return pull ? false : true;
+
+	if (ctrl->pull_auto) {
+		if (pull != PIN_CONFIG_BIAS_PULL_PIN_DEFAULT &&
+					pull != PIN_CONFIG_BIAS_DISABLE)
+			return false;
+	} else {
+		if (pull == PIN_CONFIG_BIAS_PULL_PIN_DEFAULT)
+			return false;
+	}
+
+	return true;
+}
+
 /* set the pin config settings for a specified pin */
 static int rockchip_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
 							unsigned long config)

@@ -570,12 +589,21 @@ static int rockchip_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
 	struct rockchip_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
 	struct rockchip_pin_bank *bank = pin_to_bank(info, pin);
 	enum pin_config_param param = pinconf_to_config_param(config);
+	u16 arg = pinconf_to_config_argument(config);
 
 	switch (param) {
 	case PIN_CONFIG_BIAS_DISABLE:
+		return rockchip_set_pull(bank, pin - bank->pin_base, param);
+		break;
 	case PIN_CONFIG_BIAS_PULL_UP:
 	case PIN_CONFIG_BIAS_PULL_DOWN:
 	case PIN_CONFIG_BIAS_PULL_PIN_DEFAULT:
+		if (!rockchip_pinconf_pull_valid(info->ctrl, param))
+			return -ENOTSUPP;
+
+		if (!arg)
+			param = PIN_CONFIG_BIAS_DISABLE;
+
 		return rockchip_set_pull(bank, pin - bank->pin_base, param);
 		break;
 	default:

@@ -600,12 +628,11 @@ static int rockchip_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
 	case PIN_CONFIG_BIAS_PULL_UP:
 	case PIN_CONFIG_BIAS_PULL_DOWN:
 	case PIN_CONFIG_BIAS_PULL_PIN_DEFAULT:
-		pull = rockchip_get_pull(bank, pin - bank->pin_base);
-
-		if (pull != param)
-			return -EINVAL;
+		if (!rockchip_pinconf_pull_valid(info->ctrl, param))
+			return -ENOTSUPP;
 
-		*config = 0;
+		pull = rockchip_get_pull(bank, pin - bank->pin_base);
+		*config = (pull == param) ? 1 : 0;
 		break;
 	default:
 		return -ENOTSUPP;

Signature · 1 line

1.7.10.4

Linus Walleij

2013-06-16 10:35 UTC #14 lore raw

Re: [PATCH 5/5] pinctrl: rockchip: correctly handle arguments of pinconf options

Quoted 5 lines · Heiko St?bner

Change the rockchip pinctrl driver to handle the arguments of 0 or 1 to the pull pinconfig options correctly, so that the pull gets disabled when either the bias_disable options is set or the pull option has the argument 0.
Signed-off-by Heiko Stuebner <[email protected]>

Patch applied so we can move ahead but:

drivers/pinctrl/pinctrl-rockchip.c static int rockchip_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin, · line 605

Quoted 8 lines

case PIN_CONFIG_BIAS_PULL_UP:
case PIN_CONFIG_BIAS_PULL_DOWN:
case PIN_CONFIG_BIAS_PULL_PIN_DEFAULT:

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

+               if (!rockchip_pinconf_pull_valid(info->ctrl, param))
+                       return -ENOTSUPP;
+
+               if (!arg)
+                       param = PIN_CONFIG_BIAS_DISABLE;

This is what I think is counter-intuitive.

Can't you rely on the bias-disable here instead?

drivers/pinctrl/pinctrl-rockchip.c static int rockchip_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin, · line 635

Quoted 3 lines

drivers/pinctrl/pinctrl-rockchip.c +2 -1

-               *config = 0;
+               pull = rockchip_get_pull(bank, pin - bank->pin_base);
+               *config = (pull == param) ? 1 : 0;

And then I guess you should emit PIN_CONFIG_BIAS_DISABLE here as well.

Yours,
Linus Walleij

Heiko Stübner

author maintainer

2013-06-16 11:02 UTC #15 lore raw

Re: [PATCH 5/5] pinctrl: rockchip: correctly handle arguments of pinconf options

Am Sonntag, 16. Juni 2013, 12:35:43 schrieb Linus Walleij:

Quoted 21 lines · Heiko St?bner

Quoted 6 lines
Change the rockchip pinctrl driver to handle the arguments of 0 or 1 to the pull pinconfig options correctly, so that the pull gets disabled when either the bias_disable options is set or the pull option has the argument 0.
Signed-off-by Heiko Stuebner <[email protected]>
Patch applied so we can move ahead but:
drivers/pinctrl/pinctrl-rockchip.c static int rockchip_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin, · line 605
Quoted 9 lines
case PIN_CONFIG_BIAS_PULL_UP:
case PIN_CONFIG_BIAS_PULL_DOWN:
case PIN_CONFIG_BIAS_PULL_PIN_DEFAULT:
drivers/pinctrl/pinctrl-rockchip.c +5 -0
+               if (!rockchip_pinconf_pull_valid(info->ctrl, param))
+                       return -ENOTSUPP;
+
+               if (!arg)
+                       param = PIN_CONFIG_BIAS_DISABLE;
This is what I think is counter-intuitive.

Quoted 8 lines

Can't you rely on the bias-disable here instead?
drivers/pinctrl/pinctrl-rockchip.c static int rockchip_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin, · line 635
Quoted 3 lines
drivers/pinctrl/pinctrl-rockchip.c +2 -1
-               *config = 0;
+               pull = rockchip_get_pull(bank, pin - bank->pin_base);
+               *config = (pull == param) ? 1 : 0;
And then I guess you should emit PIN_CONFIG_BIAS_DISABLE here as well.

rockchip_get_pull does emit PIN_CONFIG_BIAS_DISABLE when the pull is disabled, only wrongly does add the argument here.

But I'm actually not quite sure what the expected behaviour is here at all :-)

Say the pin is in the "pin-default" pull state and the query in config_get is for "bias-disable", what would be the expected bahviour/return value in this case?

Thanks
Heiko

Linus Walleij

2013-06-16 12:35 UTC #16 lore raw

Re: [PATCH 5/5] pinctrl: rockchip: correctly handle arguments of pinconf options

Quoted 9 lines · Heiko St?bner

Quoted 6 lines
drivers/pinctrl/pinctrl-rockchip.c static int rockchip_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin, · line 635
Quoted 3 lines
drivers/pinctrl/pinctrl-rockchip.c +2 -1
-               *config = 0;
+               pull = rockchip_get_pull(bank, pin - bank->pin_base);
+               *config = (pull == param) ? 1 : 0;
And then I guess you should emit PIN_CONFIG_BIAS_DISABLE here as well.
rockchip_get_pull does emit PIN_CONFIG_BIAS_DISABLE when the pull is disabled, only wrongly does add the argument here.

Ah, right.

Quoted 5 lines

But I'm actually not quite sure what the expected behaviour is here at all :-)
Say the pin is in the "pin-default" pull state and the query in config_get is for "bias-disable", what would be the expected bahviour/return value in this case?

return -EINVAL; ?

Hm it was some time since I designed this...

Yours,
Linus Walleij

Heiko Stübner

author maintainer

2013-06-16 15:41 UTC #17 lore raw

PATCH v2 1/1 pinctrl: rockchip: correctly handle arguments of pinconf options

Change the rockchip pinctrl driver to handle the arguments to the pull pinconfig options correctly. So only accept non-0 values for the pull options as the rockchip pin-controller can only turn pulls on and off (this via BIAS_DISABLE).

Signed-off-by Heiko Stuebner <[email protected]>

---
changes since v1:
really respect the values for arguments of the pull options (aka 0 is not meant to turn off a pull).
Replacing the patch also makes sure that no wrong knowledge stays in the kernel via the old commit message, least somebody else reads it and takes it as true.

 drivers/pinctrl/pinctrl-rockchip.c      |   41 ++++++++++++++++++++++++++++---
 1 file changed, 37 insertions(+), 4 deletions(-)

drivers/pinctrl/pinctrl-rockchip.c +37 -4

index c605b63..427564f 100644
@@ -563,6 +563,25 @@ static const struct pinmux_ops rockchip_pmx_ops = {
  * Pinconf_ops handling
  */
 
+static bool rockchip_pinconf_pull_valid(struct rockchip_pin_ctrl *ctrl,
+					enum pin_config_param pull)
+{
+	/* rk3066b does support any pulls */
+	if (!ctrl->pull_offset)
+		return pull ? false : true;
+
+	if (ctrl->pull_auto) {
+		if (pull != PIN_CONFIG_BIAS_PULL_PIN_DEFAULT &&
+					pull != PIN_CONFIG_BIAS_DISABLE)
+			return false;
+	} else {
+		if (pull == PIN_CONFIG_BIAS_PULL_PIN_DEFAULT)
+			return false;
+	}
+
+	return true;
+}
+
 /* set the pin config settings for a specified pin */
 static int rockchip_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
 							unsigned long config)

@@ -570,12 +589,21 @@ static int rockchip_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
 	struct rockchip_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
 	struct rockchip_pin_bank *bank = pin_to_bank(info, pin);
 	enum pin_config_param param = pinconf_to_config_param(config);
+	u16 arg = pinconf_to_config_argument(config);
 
 	switch (param) {
 	case PIN_CONFIG_BIAS_DISABLE:
+		return rockchip_set_pull(bank, pin - bank->pin_base, param);
+		break;
 	case PIN_CONFIG_BIAS_PULL_UP:
 	case PIN_CONFIG_BIAS_PULL_DOWN:
 	case PIN_CONFIG_BIAS_PULL_PIN_DEFAULT:
+		if (!rockchip_pinconf_pull_valid(info->ctrl, param))
+			return -ENOTSUPP;
+
+		if (!arg)
+			return -EINVAL;
+
 		return rockchip_set_pull(bank, pin - bank->pin_base, param);
 		break;
 	default:

@@ -593,19 +621,24 @@ static int rockchip_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
 	struct rockchip_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
 	struct rockchip_pin_bank *bank = pin_to_bank(info, pin);
 	enum pin_config_param param = pinconf_to_config_param(*config);
-	unsigned int pull;
 
 	switch (param) {
 	case PIN_CONFIG_BIAS_DISABLE:
+		if (rockchip_get_pull(bank, pin - bank->pin_base) != param)
+			return -EINVAL;
+
+		*config = 0;
+		break;
 	case PIN_CONFIG_BIAS_PULL_UP:
 	case PIN_CONFIG_BIAS_PULL_DOWN:
 	case PIN_CONFIG_BIAS_PULL_PIN_DEFAULT:
-		pull = rockchip_get_pull(bank, pin - bank->pin_base);
+		if (!rockchip_pinconf_pull_valid(info->ctrl, param))
+			return -ENOTSUPP;
 
-		if (pull != param)
+		if (rockchip_get_pull(bank, pin - bank->pin_base) != param)
 			return -EINVAL;
 
-		*config = 0;
+		*config = 1;
 		break;
 	default:
 		return -ENOTSUPP;

Signature · 1 line

1.7.10.4

Linus Walleij

2013-06-17 15:48 UTC #18 lore raw

Re: [PATCH v2] pinctrl: rockchip: correctly handle arguments of pinconf options

Quoted 5 lines · Heiko St?bner

Change the rockchip pinctrl driver to handle the arguments to the pull pinconfig options correctly. So only accept non-0 values for the pull options as the rockchip pin-controller can only turn pulls on and off (this via BIAS_DISABLE).

Quoted 8 lines

Signed-off-by Heiko Stuebner <[email protected]>
---
changes since v1:
really respect the values for arguments of the pull options (aka 0 is not meant to turn off a pull).
Replacing the patch also makes sure that no wrong knowledge stays in the kernel via the old commit message, least somebody else reads it and takes it as true.

OK took out the v1 and put in this v2 version instead...

Thanks!
Linus Walleij

James Hogan

Reviewed-by on the v1 series

2013-06-14 15:53 UTC #19 lore raw

In reply to Heiko Stübner

Quoted 5 lines · Heiko St?bner

Some issues with the recently submitted generic pinconfig parsing from dt came up, so fix these in this follow-up series.
Hopefully I did catch all of them.

Quoted 7 lines

Tested on my rk3066 device.
Heiko Stuebner (5):
pinctrl: update the documentation for some pinconfig params
pinctrl: clarify some dt pinconfig options
pinctrl: handle zero found dt pinconfig properties better
pinctrl: dynamically alloc temp array when parsing dt pinconf options

patches 1-4:

Reviewed-by James Hogan <[email protected]>

Cheers
James

Quoted 8 lines

pinctrl: rockchip: correctly handle arguments of pinconf options

 .../bindings/pinctrl/pinctrl-bindings.txt          |   22 +++++++++++-
 drivers/pinctrl/pinconf-generic.c                  |   38 +++++++++++++++-----
 drivers/pinctrl/pinctrl-rockchip.c                 |   37 ++++++++++++++++---
 include/linux/pinctrl/pinconf-generic.h            |   18 ++++++----
 4 files changed, 93 insertions(+), 22 deletions(-)

Laurent Pinchart

Acked-by

2013-06-17 03:03 UTC #20 lore raw

Hi Heiko,

Quoted 8 lines · On Friday 14 June 2013 16:53:06 James Hogan wrote:

Quoted 7 lines · Heiko Stübner
Some issues with the recently submitted generic pinconfig parsing from dt came up, so fix these in this follow-up series.
Hopefully I did catch all of them.
Tested on my rk3066 device.

Quoted 8 lines

Quoted 5 lines
Heiko Stuebner (5):
pinctrl: update the documentation for some pinconfig params
pinctrl: clarify some dt pinconfig options
pinctrl: handle zero found dt pinconfig properties better
pinctrl: dynamically alloc temp array when parsing dt pinconf options
patches 1-4:
Reviewed-by James Hogan <[email protected]>

Probably too late as Linus has already applied the patches, but for 2, 3 and 4

Acked-by Laurent Pinchart <[email protected]>

Signature · 3 lines

Regards,

Laurent Pinchart