[RFC PATCH 1/2] pinctrl: Add output-disable

[RFC PATCH 1/2] pinctrl: Add output-disable

[Doug Anderson]

The pinctrl bindings / API allow you to specify that:
- a pin should be an output
- a pin should have its input path enabled / disabled

...but they don't allow you to tell a pin to stop outputting.  Lets
add a new setting for that just in case the bootloader (or the default
state) left a pin as an output and we don't want it that way anymore.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
 Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt | 1 +
 drivers/pinctrl/pinconf-generic.c                              | 1 +
 include/linux/pinctrl/pinconf-generic.h                        | 3 +++
 3 files changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
index 98eb94d..9ac8591 100644
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
@@ -183,6 +183,7 @@ 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
+output-disable		- disable output to the pin
 slew-rate		- set the slew rate
 
 For example:
diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c
index 29ff77f..ec4d95f 100644
--- a/drivers/pinctrl/pinconf-generic.c
+++ b/drivers/pinctrl/pinconf-generic.c
@@ -171,6 +171,7 @@ static struct pinconf_generic_dt_params dt_params[] = {
 	{ "low-power-disable", PIN_CONFIG_LOW_POWER_MODE, 0 },
 	{ "output-low", PIN_CONFIG_OUTPUT, 0, },
 	{ "output-high", PIN_CONFIG_OUTPUT, 1, },
+	{ "output-disable", PIN_CONFIG_OUTPUT_DISABLE, 1, },
 	{ "slew-rate", PIN_CONFIG_SLEW_RATE, 0},
 };
 
diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
index d578a60..52b0429 100644
--- a/include/linux/pinctrl/pinconf-generic.h
+++ b/include/linux/pinctrl/pinconf-generic.h
@@ -89,6 +89,8 @@
  *	1 to indicate high level, argument 0 to indicate low level. (Please
  *	see Documentation/pinctrl.txt, section "GPIO mode pitfalls" for a
  *	discussion around this parameter.)
+ * @PIN_CONFIG_OUTPUT_DISABLE: this will configure the pin _not_ to output.
+ *	Parameter should be 1.
  * @PIN_CONFIG_END: this is the last enumerator for pin configurations, if
  *	you need to pass in custom configurations to the pin controller, use
  *	PIN_CONFIG_END+1 as the base offset.
@@ -112,6 +114,7 @@ enum pin_config_param {
 	PIN_CONFIG_SLEW_RATE,
 	PIN_CONFIG_LOW_POWER_MODE,
 	PIN_CONFIG_OUTPUT,
+	PIN_CONFIG_OUTPUT_DISABLE,
 	PIN_CONFIG_END = 0x7FFF,
 };
 
-- 
2.1.0.rc2.206.gedb03e5

[RFC PATCH 2/2] pinctrl: rockchip: Implement PIN_CONFIG_OUTPUT_DISABLE

[Doug Anderson]

If someone requests us to disable output for a pin, we'll configure it
as an input.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
 drivers/pinctrl/pinctrl-rockchip.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index 40970c3..c1ac14c 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -971,6 +971,12 @@ static int rockchip_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
 			if (rc)
 				return rc;
 			break;
+		case PIN_CONFIG_OUTPUT_DISABLE:
+			rc = _rockchip_pmx_gpio_set_direction(&bank->gpio_chip,
+					  pin - bank->pin_base, true);
+			if (rc)
+				return rc;
+			break;
 		case PIN_CONFIG_OUTPUT:
 			rockchip_gpio_set(&bank->gpio_chip,
 					  pin - bank->pin_base, arg);
-- 
2.1.0.rc2.206.gedb03e5

[RFC PATCH 1/2] pinctrl: Add output-disable

[Linus Walleij]

On Tue, Oct 28, 2014 at 9:57 PM, Doug Anderson <dianders@chromium.org> wrote:
> The pinctrl bindings / API allow you to specify that:
> - a pin should be an output
> - a pin should have its input path enabled / disabled
>
> ...but they don't allow you to tell a pin to stop outputting.  Lets
> add a new setting for that just in case the bootloader (or the default
> state) left a pin as an output and we don't want it that way anymore.
>
> Signed-off-by: Doug Anderson <dianders@chromium.org>
(...)
> + * @PIN_CONFIG_OUTPUT_DISABLE: this will configure the pin _not_ to output.
> + *     Parameter should be 1.

This doesn't make sense. The pin is either low, high, some analog mode
or tristate/high impedance.

It does *not* stop existing.

Figure out the exact electronic meaning of what happens when you do
"output disable" in your hardware, I think it is very likely that
PIN_CONFIG_BIAS_HIGH_IMPEDANCE is what you are really
after here.

Yours,
Linus Walleij

[RFC PATCH 1/2] pinctrl: Add output-disable

[Doug Anderson]

Linus,

On Fri, Oct 31, 2014 at 1:49 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Oct 28, 2014 at 9:57 PM, Doug Anderson <dianders@chromium.org> wrote:
>> The pinctrl bindings / API allow you to specify that:
>> - a pin should be an output
>> - a pin should have its input path enabled / disabled
>>
>> ...but they don't allow you to tell a pin to stop outputting.  Lets
>> add a new setting for that just in case the bootloader (or the default
>> state) left a pin as an output and we don't want it that way anymore.
>>
>> Signed-off-by: Doug Anderson <dianders@chromium.org>
> (...)
>> + * @PIN_CONFIG_OUTPUT_DISABLE: this will configure the pin _not_ to output.
>> + *     Parameter should be 1.
>
> This doesn't make sense. The pin is either low, high, some analog mode
> or tristate/high impedance.
>
> It does *not* stop existing.
>
> Figure out the exact electronic meaning of what happens when you do
> "output disable" in your hardware, I think it is very likely that
> PIN_CONFIG_BIAS_HIGH_IMPEDANCE is what you are really
> after here.

OK, that seems plausible.  ...so it is OK to set both
"bias-high-impedance" _and_ "bias-pull-up" on a pin?  That seemed
weird to me but if that's right I can do that and try to implement
"bias-high-impedance" on Rockchip...


Maybe better to explain the problem.  I have a pin that I wish to
drive high weakly (using a pullup rather than actually pushing
output-high to the pin).  The firmware has left the pin configured as
an output with no pull.

I can configure the pin like this:
  rockchip,pins = <7 12 RK_FUNC_GPIO &pcfg_pull_up>;

  pcfg_pull_up: pcfg-pull-up {
    bias-pull-up;
  };

When I do this the current Rockchip pinctrl driver will _not_
configure me as an input.  It will happily flip the "pullup" bit in
hardware and leave the pin configured as an output.

I could certainly reach into the GPIO controller part of things
whenever I see "bias-pull-up" and configure the pin as an input.  I
guess that wouldn't actually hurt to do even if the pin wasn't
configured as a GPIO...  In other words, if someone has:
  rockchip,pins = <6 22 RK_FUNC_1 &pcfg_pull_up>;

...it doesn't hurt to set the GPIO controller for this pin to be an
input because it's not used when RK_FUNC_1 is used.


A third option that would work in my case (and actually be sorta
clean) would be to implement a faux open-drain output.  The hardware
itself doesn't have a concept of open drain (unlike some SoCs) but I
could implement it by swapping between "input pulled up" and "output
driven low".


-Doug

[RFC PATCH 1/2] pinctrl: Add output-disable

[Linus Walleij]

On Mon, Nov 3, 2014 at 10:09 PM, Doug Anderson <dianders@chromium.org> wrote:
> On Fri, Oct 31, 2014 at 1:49 AM, Linus Walleij <linus.walleij@linaro.org> wrote:

>> Figure out the exact electronic meaning of what happens when you do
>> "output disable" in your hardware, I think it is very likely that
>> PIN_CONFIG_BIAS_HIGH_IMPEDANCE is what you are really
>> after here.
>
> OK, that seems plausible.  ...so it is OK to set both
> "bias-high-impedance" _and_ "bias-pull-up" on a pin?  That seemed
> weird to me but if that's right I can do that and try to implement
> "bias-high-impedance" on Rockchip...

I don't quite get the question. What this patch does is add something
called "output disable" which I think is analogous to high impedance.

If you enable both as the same time something is likely wrong
because it doesn't make electronic sense, high impedance and
pull up are kind of mutually exclusive. High impedance is also
know as "tristate" and means the line will assume the level of
whatever it is connected to, not try to pull things up.

So either:

- This is the wrong terminology, "output disable" means something
  else. Like "disconnect the driver stages" or something, so pull-up
  can be enabled.

- Stuff has been programmed in a bad way all the time.

Understanding the hardware is paramount...

> Maybe better to explain the problem.  I have a pin that I wish to
> drive high weakly (using a pullup rather than actually pushing
> output-high to the pin).  The firmware has left the pin configured as
> an output with no pull.
>
> I can configure the pin like this:
>   rockchip,pins = <7 12 RK_FUNC_GPIO &pcfg_pull_up>;
>
>   pcfg_pull_up: pcfg-pull-up {
>     bias-pull-up;
>   };

This makes perfect sense.

> When I do this the current Rockchip pinctrl driver will _not_
> configure me as an input.  It will happily flip the "pullup" bit in
> hardware and leave the pin configured as an output.

You seem to imply that the pins registers work such that
input needs to be enabled for pull-up to work.

This is a hardware pecularity. Just enable input in the hardware
whenever pull-up is requested then, as that is how that hardware
has to work. If you want to be explicit there is always
PIN_CONFIG_INPUT_ENABLE (input-enable;)

> I could certainly reach into the GPIO controller part of things
> whenever I see "bias-pull-up" and configure the pin as an input.

I think you should. This is a side effect of combined hardware
and expected.

>  I
> guess that wouldn't actually hurt to do even if the pin wasn't
> configured as a GPIO...

This is why we have the callbacks:

pinctrl_request_gpio()
pinctrl_free_gpio()
pinctrl_gpio_direction_input()
pinctrl_gpio_direction_output()

By letting the GPIO driver side call these to set direction, we
can leave the control over direction status and incommensurable
states to the pin control driver.

Not that I have all things in my head, but if you're not using
the above calls in your GPIO driver, I suggest doing so and leaving
the pin direction status for the pinctrl side to handle.

> A third option that would work in my case (and actually be sorta
> clean) would be to implement a faux open-drain output.  The hardware
> itself doesn't have a concept of open drain (unlike some SoCs) but I
> could implement it by swapping between "input pulled up" and "output
> driven low".

Uhm. Not following this. Probably you are better at electronics than
I am.

Anyway, I think the answers above is what you actually need to
keep working on this.

Yours,
Linus Walleij