| Message ID | 1414529835-16207-1-git-send-email-dianders@chromium.org |
|---|---|
| State | Needs Review / ACK, archived |
| Headers | show |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 1 errors, 0 warnings, 0 lines checked |
| robh/patch-applied | success |
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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, };
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(+)