[PATCH 0/5] pinctrl: fix some issues with new pinconfig dt parsing

[PATCH 0/5] pinctrl: fix some issues with new pinconfig dt parsing

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

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

-- 
1.7.10.4

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

[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 <heiko@sntech.de>
---
 include/linux/pinctrl/pinconf-generic.h |   18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
index d414a77..d1868bc 100644
--- a/include/linux/pinctrl/pinconf-generic.h
+++ b/include/linux/pinctrl/pinconf-generic.h
@@ -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
-- 
1.7.10.4

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

[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 <james.hogan@imgtec.com>

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 .../bindings/pinctrl/pinctrl-bindings.txt          |   22 +++++++++++++++++++-
 drivers/pinctrl/pinconf-generic.c                  |    9 ++++----
 2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
index ef7cd57..2d730e3 100644
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
@@ -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>
diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c
index d3c693c..dcf0371 100644
--- a/drivers/pinctrl/pinconf-generic.c
+++ b/drivers/pinctrl/pinconf-generic.c
@@ -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, },
 };
-- 
1.7.10.4

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

[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 <laurent.pinchart@ideasonboard.com>
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/pinctrl/pinconf-generic.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c
index dcf0371..ea9da17 100644
--- a/drivers/pinctrl/pinconf-generic.c
+++ b/drivers/pinctrl/pinconf-generic.c
@@ -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.
-- 
1.7.10.4

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

[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 <laurent.pinchart@ideasonboard.com>
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/pinctrl/pinconf-generic.c |   24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c
index ea9da17..794dad7 100644
--- a/drivers/pinctrl/pinconf-generic.c
+++ b/drivers/pinctrl/pinconf-generic.c
@@ -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
-- 
1.7.10.4

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

[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 <heiko@sntech.de>
---
 drivers/pinctrl/pinctrl-rockchip.c |   37 +++++++++++++++++++++++++++++++-----
 1 file changed, 32 insertions(+), 5 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index c605b63..2568afc 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -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;
-- 
1.7.10.4

Re: [PATCH 0/5] pinctrl: fix some issues with new pinconfig dt parsing

[James Hogan]

On 14/06/13 16:41, Heiko Stübner wrote:
> 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

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

Cheers
James

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

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

[Linus Walleij]

On Fri, Jun 14, 2013 at 5:42 PM, Heiko Stübner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> wrote:

> 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 <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>

I'm holding this patch off.

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

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

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

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

[Linus Walleij]

On Fri, Jun 14, 2013 at 5:42 PM, Heiko Stübner <heiko@sntech.de> wrote:

> 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 <james.hogan@imgtec.com>
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>

Patch applied.

Yours,
Linus Walleij

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

[Linus Walleij]

On Fri, Jun 14, 2013 at 5:43 PM, Heiko Stübner <heiko@sntech.de> wrote:

> 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 <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>

Patch applied.

Yours,
Linus Walleij

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

[Linus Walleij]

On Fri, Jun 14, 2013 at 5:43 PM, Heiko Stübner <heiko@sntech.de> wrote:

> 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 <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>

Patch applied.

Yours,
Linus Walleij

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

[Linus Walleij]

On Fri, Jun 14, 2013 at 5:44 PM, Heiko Stübner <heiko@sntech.de> wrote:

> 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 <heiko@sntech.de>

Patch applied so we can move ahead but:

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

This is what I think is counter-intuitive.

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

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

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

[Heiko Stübner]

Am Sonntag, 16. Juni 2013, 12:26:38 schrieb Linus Walleij:
> On Fri, Jun 14, 2013 at 5:42 PM, Heiko Stübner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> wrote:
> > 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 <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
> 
> I'm holding this patch off.
> 
> >   * @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?
> 
> >   * @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.
> 
> >   * @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?

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

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

[Heiko Stübner]

Am Sonntag, 16. Juni 2013, 12:35:43 schrieb Linus Walleij:
> On Fri, Jun 14, 2013 at 5:44 PM, Heiko Stübner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> wrote:
> > 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 <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
> 
> Patch applied so we can move ahead but:
> >         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;
> 
> This is what I think is counter-intuitive.
> 
> Can't you rely on the bias-disable here instead?
> 
> > -               *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

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

[Linus Walleij]

On Sun, Jun 16, 2013 at 12:45 PM, Heiko Stübner <heiko@sntech.de> wrote:
> Am Sonntag, 16. Juni 2013, 12:26:38 schrieb Linus Walleij:

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

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

> 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

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

[Linus Walleij]

On Sun, Jun 16, 2013 at 1:02 PM, Heiko Stübner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> wrote:

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

> 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

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

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

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
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(-)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index c605b63..427564f 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -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;
-- 
1.7.10.4

Re: [PATCH 0/5] pinctrl: fix some issues with new pinconfig dt parsing

[Laurent Pinchart]

Hi Heiko,

On Friday 14 June 2013 16:53:06 James Hogan wrote:
> On 14/06/13 16:41, Heiko Stübner wrote:
> > 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
> 
> patches 1-4:
> Reviewed-by: James Hogan <james.hogan@imgtec.com>

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

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

-- 
Regards,

Laurent Pinchart

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

[Linus Walleij]

On Sun, Jun 16, 2013 at 5:41 PM, Heiko Stübner <heiko@sntech.de> wrote:

> 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 <heiko@sntech.de>
> ---
> 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

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

[Stephen Warren]

On 06/14/2013 09:42 AM, Heiko Stübner wrote:
> 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 <james.hogan@imgtec.com>
> 

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

> Signed-off-by: Heiko Stuebner <heiko@sntech.de>

> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt

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

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

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

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.

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

[Linus Walleij]

On Thu, Jun 20, 2013 at 12:10 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 06/14/2013 09:42 AM, Heiko Stübner wrote:

>> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
>
>> -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.

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

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

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