linux-gpio.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Douglas Anderson <dianders@chromium.org>
To: Bjorn Andersson <andersson@kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Mark Brown <broonie@kernel.org>,
	Linus Walleij <linus.walleij@linaro.org>
Cc: Matthias Kaehlcke <mka@chromium.org>,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	linux-gpio@vger.kernel.org, Stephen Boyd <swboyd@chromium.org>,
	devicetree@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	linux-spi@vger.kernel.org,
	Douglas Anderson <dianders@chromium.org>,
	Andy Gross <agross@kernel.org>,
	linux-kernel@vger.kernel.org
Subject: [PATCH 08/14] pinctrl: qcom: Support OUTPUT_ENABLE; deprecate INPUT_ENABLE
Date: Thu, 23 Mar 2023 10:30:12 -0700	[thread overview]
Message-ID: <20230323102605.8.Id740ae6a993f9313b58add6b10f6a92795d510d4@changeid> (raw)
In-Reply-To: <20230323173019.3706069-1-dianders@chromium.org>

The Qualcomm pinctrl driver has been violating the documented meaning
of PIN_CONFIG_INPUT_ENABLE. That documentation says:

  Note that this does not affect the pin's ability to drive output.

...yet the Qualcomm driver's sole action when asked to "enable input"
on a pin is to disable its output.

The Qualcomm driver's implementation stems from the fact that
"output-disable" is a "new" property from 2017. It was introduced in
commit 425562429d4f ("pinctrl: generic: Add output-enable
property"). The "input-enable" handling in Qualcomm drivers is from
2015 introduced in commit 407f5e392f9c ("pinctrl: qcom: handle
input-enable pinconf property").

Let's change the Qualcomm driver to move us in the right direction. As
part of this:
1. We'll now support PIN_CONFIG_OUTPUT_ENABLE
2. We'll still support using PIN_CONFIG_INPUT_ENABLE to disable a
   pin's output (in violation of the docs) with a big comment in the
   code. This is needed because old device trees have "input-enable"
   in them and, in some cases, people might need the old
   behavior. While we could programmatically change all old device
   trees, it doesn't really hurt to keep supporting the old behavior
   and we're _supposed_ to try to be compatible with old device trees
   anyway.

It can also be noted that the PIN_CONFIG_INPUT_ENABLE handling code
seems to have purposefully ignored its argument. That means that old
boards that had _either_ "input-disable" or "input-enable" in them
would have had the effect of disabling a pin's output. While we could
change this behavior, since we're only leaving the
PIN_CONFIG_INPUT_ENABLE there for backward compatibility we might as
well be fully backward compatible.

NOTE: despite the fact that we'll still support
PIN_CONFIG_INPUT_ENABLE for _setting_ config, we take it away from
msm_config_group_get(). This appears to be only used for populating
debugfs and fixing debugfs to "output enabled" where relevant instead
of "input enabled" makes more sense and has more truthiness.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/pinctrl/qcom/pinctrl-msm.c | 36 +++++++++++++++++++++++++-----
 1 file changed, 31 insertions(+), 5 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index daeb79a9a602..4515f375c5e8 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -323,6 +323,7 @@ static int msm_config_reg(struct msm_pinctrl *pctrl,
 		break;
 	case PIN_CONFIG_OUTPUT:
 	case PIN_CONFIG_INPUT_ENABLE:
+	case PIN_CONFIG_OUTPUT_ENABLE:
 		*bit = g->oe_bit;
 		*mask = 1;
 		break;
@@ -414,11 +415,9 @@ static int msm_config_group_get(struct pinctrl_dev *pctldev,
 		val = msm_readl_io(pctrl, g);
 		arg = !!(val & BIT(g->in_bit));
 		break;
-	case PIN_CONFIG_INPUT_ENABLE:
-		/* Pin is output */
-		if (arg)
+	case PIN_CONFIG_OUTPUT_ENABLE:
+		if (!arg)
 			return -EINVAL;
-		arg = 1;
 		break;
 	default:
 		return -ENOTSUPP;
@@ -502,9 +501,36 @@ static int msm_config_group_set(struct pinctrl_dev *pctldev,
 			arg = 1;
 			break;
 		case PIN_CONFIG_INPUT_ENABLE:
-			/* disable output */
+			/*
+			 * According to pinctrl documentation this should
+			 * actually be a no-op.
+			 *
+			 * The docs are explicit that "this does not affect
+			 * the pin's ability to drive output" but what we do
+			 * here is to modify the output enable bit. Thus, to
+			 * follow the docs we should remove that.
+			 *
+			 * The docs say that we should enable any relevant
+			 * input buffer, but TLMM there is no input buffer that
+			 * can be enabled/disabled. It's always on.
+			 *
+			 * The points above, explain why this _should_ be a
+			 * no-op. However, for historical reasons and to
+			 * support old device trees, we'll violate the docs
+			 * still affect the output.
+			 *
+			 * It should further be noted that this old historical
+			 * behavior actually overrides arg to 0. That means
+			 * that "input-enable" and "input-disable" in a device
+			 * tree would _both_ disable the output. We'll
+			 * continue to preserve this behavior as well since
+			 * we have no other use for this attribute.
+			 */
 			arg = 0;
 			break;
+		case PIN_CONFIG_OUTPUT_ENABLE:
+			arg = !!arg;
+			break;
 		default:
 			dev_err(pctrl->dev, "Unsupported config parameter: %x\n",
 				param);
-- 
2.40.0.348.gf938b09366-goog


  parent reply	other threads:[~2023-03-23 17:32 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-03-23 17:30 [PATCH 00/14] Control Quad SPI pinctrl better on Qualcomm Chromebooks Douglas Anderson
2023-03-23 17:30 ` [PATCH 01/14] arm64: dts: sc7180: Rename qspi data12 as data23 Douglas Anderson
2023-03-23 17:30 ` [PATCH 02/14] arm64: dts: sc7280: " Douglas Anderson
2023-03-23 17:30 ` [PATCH 03/14] arm64: dts: sdm845: " Douglas Anderson
2023-03-23 17:30 ` [PATCH 04/14] arm64: dts: qcom: sc7180: Annotate l13a on trogdor to always-on Douglas Anderson
2023-03-23 17:30 ` [PATCH 05/14] spi: spi-qcom-qspi: Support pinctrl sleep states Douglas Anderson
2023-03-23 17:30 ` [PATCH 06/14] dt-bindings: pinctrl: qcom: tlmm should use output-disable, not input-enable Douglas Anderson
2023-03-27  7:21   ` Krzysztof Kozlowski
2023-03-23 17:30 ` [PATCH 07/14] dt-bindings: pinctrl: qcom: Add output-enable Douglas Anderson
2023-03-27  7:22   ` Krzysztof Kozlowski
2023-03-23 17:30 ` Douglas Anderson [this message]
2023-03-24 13:55   ` [PATCH 08/14] pinctrl: qcom: Support OUTPUT_ENABLE; deprecate INPUT_ENABLE Bjorn Andersson
2023-03-23 17:30 ` [PATCH 09/14] arm64: dts: qcom: sc7180: Remove superfluous "input-enable"s from trogdor Douglas Anderson
2023-03-23 17:30 ` [PATCH 10/14] arm64: dts: qcom: sc7280: Remove superfluous "input-enable"s from idp-ec-h1 Douglas Anderson
2023-03-23 17:30 ` [PATCH 11/14] arm64: dts: qcom: sdm845: Remove superfluous "input-enable"s from cheza Douglas Anderson
2023-03-23 17:30 ` [PATCH 12/14] arm64: dts: qcom: sc7180: Fix trogdor qspi pin config Douglas Anderson
2023-04-07 18:11   ` Krzysztof Kozlowski
2023-04-07 19:53     ` Doug Anderson
2023-04-08 10:52       ` Krzysztof Kozlowski
2023-03-23 17:30 ` [PATCH 13/14] arm64: dts: qcom: sc7280: Fix " Douglas Anderson
2023-03-23 17:30 ` [PATCH 14/14] arm64: dts: qcom: sdm845: Fix cheza " Douglas Anderson
2023-03-27 21:44 ` [PATCH 00/14] Control Quad SPI pinctrl better on Qualcomm Chromebooks Linus Walleij
2023-03-27 21:51   ` Doug Anderson
2023-03-29  8:50     ` Linus Walleij
2023-04-07 17:55       ` Bjorn Andersson
2023-03-28 13:06 ` (subset) " Mark Brown
2023-04-07 18:00 ` Bjorn Andersson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230323102605.8.Id740ae6a993f9313b58add6b10f6a92795d510d4@changeid \
    --to=dianders@chromium.org \
    --cc=agross@kernel.org \
    --cc=andersson@kernel.org \
    --cc=broonie@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=konrad.dybcio@linaro.org \
    --cc=krzysztof.kozlowski+dt@linaro.org \
    --cc=linus.walleij@linaro.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-spi@vger.kernel.org \
    --cc=mka@chromium.org \
    --cc=robh+dt@kernel.org \
    --cc=swboyd@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).