[PATCH 1/2] pinctrl: aspeed: Enable pass-through on GPIOE1 and GPIOE3 free

[PATCH 1/2] pinctrl: aspeed: Enable pass-through on GPIOE1 and GPIOE3 free

[Bills, Jason M]

This change adds a gpio_disable_free() implementation that checks
if the GPIO being freed is GPIOE1 (33) or GPIOE3 (35) and will
re-enable the pass-through mux.

Tested:
Requested GPIOs 33 and 35 and used devmem to check that pass-through
was disabled. Then freed them and checked that pass-through was
enabled again.

Signed-off-by: Jason M. Bills <jason.m.bills@linux.intel.com>
---
  drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c | 17 +++++++
  drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c | 17 +++++++
  drivers/pinctrl/aspeed/pinctrl-aspeed.c    | 53 ++++++++++++++++++++++
  drivers/pinctrl/aspeed/pinctrl-aspeed.h    |  3 ++
  4 files changed, 90 insertions(+)

diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c 
b/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c
index 4c0d26606b6c..6ab3473cbba6 100644
--- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c
+++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c
@@ -2785,6 +2785,22 @@ static int aspeed_g5_sig_expr_set(struct 
aspeed_pinmux_data *ctx,
  	return 0;
  }

+#define GPIOE1 33
+#define GPIOE3 35
+static void aspeed_g5_gpio_disable_free(struct pinctrl_dev *pctldev,
+			      struct pinctrl_gpio_range *range,
+			      unsigned int offset)
+{
+	/*
+	 * If we're freeing GPIOE1 (33) or GPIOE3 (35) then re-enable the
+	 * pass-through mux setting; otherwise, do nothing.
+	 */
+	if (offset != GPIOE1 && offset != GPIOE3)
+		return;
+
+	aspeed_gpio_disable_free(pctldev, range, offset);
+}
+
  static const struct aspeed_pin_config_map aspeed_g5_pin_config_map[] = {
  	{ PIN_CONFIG_BIAS_PULL_DOWN,  0, 1, BIT_MASK(0)},
  	{ PIN_CONFIG_BIAS_PULL_DOWN, -1, 0, BIT_MASK(0)},
@@ -2820,6 +2836,7 @@ static const struct pinmux_ops 
aspeed_g5_pinmux_ops = {
  	.get_function_groups = aspeed_pinmux_get_fn_groups,
  	.set_mux = aspeed_pinmux_set_mux,
  	.gpio_request_enable = aspeed_gpio_request_enable,
+	.gpio_disable_free = aspeed_g5_gpio_disable_free,
  	.strict = true,
  };

diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c 
b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
index a3fa03bcd9a3..ffc72168ef7b 100644
--- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
+++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
@@ -2693,6 +2693,22 @@ static int aspeed_g6_sig_expr_set(struct 
aspeed_pinmux_data *ctx,
  	return 0;
  }

+#define GPIOP1 121
+#define GPIOP3 123
+static void aspeed_g6_gpio_disable_free(struct pinctrl_dev *pctldev,
+			      struct pinctrl_gpio_range *range,
+			      unsigned int offset)
+{
+	/*
+	 * If we're freeing GPIOP1 (121) or GPIOP3 (123) then re-enable the
+	 * pass-through mux setting; otherwise, do nothing.
+	 */
+	if (offset != GPIOP1 && offset != GPIOP3)
+		return;
+
+	aspeed_gpio_disable_free(pctldev, range, offset);
+}
+
  static const struct aspeed_pin_config_map aspeed_g6_pin_config_map[] = {
  	{ PIN_CONFIG_BIAS_PULL_DOWN,  0,   1, BIT_MASK(0)},
  	{ PIN_CONFIG_BIAS_PULL_DOWN, -1,   0, BIT_MASK(0)},
@@ -2733,6 +2749,7 @@ static const struct pinmux_ops 
aspeed_g6_pinmux_ops = {
  	.get_function_groups = aspeed_pinmux_get_fn_groups,
  	.set_mux = aspeed_pinmux_set_mux,
  	.gpio_request_enable = aspeed_gpio_request_enable,
+	.gpio_disable_free = aspeed_g6_gpio_disable_free,
  	.strict = true,
  };

diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.c 
b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
index c94e24aadf92..ade658af580b 100644
--- a/drivers/pinctrl/aspeed/pinctrl-aspeed.c
+++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
@@ -439,6 +439,59 @@ int aspeed_gpio_request_enable(struct pinctrl_dev 
*pctldev,
  	return 0;
  }

+void aspeed_gpio_disable_free(struct pinctrl_dev *pctldev,
+			      struct pinctrl_gpio_range *range,
+			      unsigned int offset)
+{
+	struct aspeed_pinctrl_data *pdata = pinctrl_dev_get_drvdata(pctldev);
+	const struct aspeed_pin_desc *pdesc = pdata->pins[offset].drv_data;
+	const struct aspeed_sig_expr ***prios, **funcs, *expr;
+	int ret;
+
+	if (!pdesc)
+		return;
+
+	dev_dbg(pctldev->dev,
+		"Freeing pass-through pin %s (%d). Re-enabling pass-through.\n",
+		pdesc->name, offset);
+
+	prios = pdesc->prios;
+
+	if (!prios)
+		return;
+
+	/* Disable any functions of higher priority than GPIO just in case */
+	while ((funcs = *prios)) {
+		if (aspeed_gpio_in_exprs(funcs))
+			break;
+
+		ret = aspeed_disable_sig(&pdata->pinmux, funcs);
+		if (ret)
+			return;
+
+		prios++;
+	}
+
+	if (!funcs) {
+		char *signals = get_defined_signals(pdesc);
+
+		pr_warn("No GPIO signal type found on pin %s (%d). Found: %s\n",
+			pdesc->name, offset, signals);
+		kfree(signals);
+
+		return;
+	}
+
+	/*
+	 * Pass-through should be one priority higher than the GPIO function,
+	 * so decrement our prios and enable that function
+	 */
+	prios--;
+	funcs = *prios;
+	expr = *funcs;
+	aspeed_sig_expr_enable(&pdata->pinmux, expr);
+}
+
  int aspeed_pinctrl_probe(struct platform_device *pdev,
  			 struct pinctrl_desc *pdesc,
  			 struct aspeed_pinctrl_data *pdata)
diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.h 
b/drivers/pinctrl/aspeed/pinctrl-aspeed.h
index 4dcde3bc29c8..bd497c20a15f 100644
--- a/drivers/pinctrl/aspeed/pinctrl-aspeed.h
+++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.h
@@ -101,6 +101,9 @@ int aspeed_pinmux_set_mux(struct pinctrl_dev 
*pctldev, unsigned int function,
  int aspeed_gpio_request_enable(struct pinctrl_dev *pctldev,
  		struct pinctrl_gpio_range *range,
  		unsigned int offset);
+void aspeed_gpio_disable_free(struct pinctrl_dev *pctldev,
+		struct pinctrl_gpio_range *range,
+		unsigned int offset);
  int aspeed_pinctrl_probe(struct platform_device *pdev,
  		struct pinctrl_desc *pdesc,
  		struct aspeed_pinctrl_data *pdata);
-- 
2.25.1

Re: [PATCH 1/2] pinctrl: aspeed: Enable pass-through on GPIOE1 and GPIOE3 free

[Andrew Jeffery]

On Thu, 3 Feb 2022, at 06:29, Bills, Jason M wrote:
> This change adds a gpio_disable_free() implementation that checks
> if the GPIO being freed is GPIOE1 (33) or GPIOE3 (35) and will
> re-enable the pass-through mux.

Okay. So trying to pull back from the implementation for a moment:

Perhaps we can view pass-through as a property on a pair of GPIOs, rather than a mux state? I think it would be better if we could, for instance, annotate this in the devicetree?

If we did that I don't think we're require this awkward and pin-specific implementation of the free callback for GPIOs.

If pass-through is enabled it puts constraints on how the pins are used if they're requested as GPIOs, but we can add those dynamic checks in the GPIO driver.

Let me think about it some more.

Thanks for surfacing the patch.

Andrew

Re: [PATCH 1/2] pinctrl: aspeed: Enable pass-through on GPIOE1 and GPIOE3 free

[Zev Weiss]

On Wed, Feb 02, 2022 at 02:49:13PM PST, Andrew Jeffery wrote:
>
>
>On Thu, 3 Feb 2022, at 06:29, Bills, Jason M wrote:
>> This change adds a gpio_disable_free() implementation that checks
>> if the GPIO being freed is GPIOE1 (33) or GPIOE3 (35) and will
>> re-enable the pass-through mux.
>
>Okay. So trying to pull back from the implementation for a moment:
>
>Perhaps we can view pass-through as a property on a pair of GPIOs, rather than a mux state? I think it would be better if we could, for instance, annotate this in the devicetree?
>
>If we did that I don't think we're require this awkward and pin-specific implementation of the free callback for GPIOs.
>

Agreed, a way to specify this in DT would be nice -- e3c246d4i, for
example, also wants GPIO passthrough, but on GPIOD rather than GPIOE.
Hopefully whatever mechanism we arrive at for this could support either.

I don't have much expertise in the pinctrl subsystem to offer for code
review, but I'm happy to help with testing for this.


Zev

Re: [PATCH 1/2] pinctrl: aspeed: Enable pass-through on GPIOE1 and GPIOE3 free

[Joel Stanley]

On Wed, 2 Feb 2022 at 22:49, Andrew Jeffery <andrew@aj.id.au> wrote:
>
>
>
> On Thu, 3 Feb 2022, at 06:29, Bills, Jason M wrote:
> > This change adds a gpio_disable_free() implementation that checks
> > if the GPIO being freed is GPIOE1 (33) or GPIOE3 (35) and will
> > re-enable the pass-through mux.
>
> Okay. So trying to pull back from the implementation for a moment:
>
> Perhaps we can view pass-through as a property on a pair of GPIOs, rather than a mux state? I think it would be better if we could, for instance, annotate this in the devicetree?
>
> If we did that I don't think we're require this awkward and pin-specific implementation of the free callback for GPIOs.
>
> If pass-through is enabled it puts constraints on how the pins are used if they're requested as GPIOs, but we can add those dynamic checks in the GPIO driver.
>
> Let me think about it some more.
>
> Thanks for surfacing the patch.

This is for the kernel, I assume.

Jason, you should send the patch to the upstream lists (use
get_maintainers.pl) for review.

Cheers,

Joel

Re: [PATCH 1/2] pinctrl: aspeed: Enable pass-through on GPIOE1 and GPIOE3 free

[Bills, Jason M]

On 2/6/2022 11:45 PM, Joel Stanley wrote:
> On Wed, 2 Feb 2022 at 22:49, Andrew Jeffery <andrew@aj.id.au> wrote:
>>
>>
>>
>> On Thu, 3 Feb 2022, at 06:29, Bills, Jason M wrote:
>>> This change adds a gpio_disable_free() implementation that checks
>>> if the GPIO being freed is GPIOE1 (33) or GPIOE3 (35) and will
>>> re-enable the pass-through mux.
>>
>> Okay. So trying to pull back from the implementation for a moment:
>>
>> Perhaps we can view pass-through as a property on a pair of GPIOs, rather than a mux state? I think it would be better if we could, for instance, annotate this in the devicetree?
>>
>> If we did that I don't think we're require this awkward and pin-specific implementation of the free callback for GPIOs.
>>
>> If pass-through is enabled it puts constraints on how the pins are used if they're requested as GPIOs, but we can add those dynamic checks in the GPIO driver.
>>
>> Let me think about it some more.
>>
>> Thanks for surfacing the patch.
> 
> This is for the kernel, I assume.
> 
> Jason, you should send the patch to the upstream lists (use
> get_maintainers.pl) for review.
Sorry for the delay.  I found the right lists with get_maintainers.pl. 
Should I send these patches to the upstream lists as they are, or do 
they need to be tweaked?

Thanks!
-Jason

> 
> Cheers,
> 
> Joel

Re: [PATCH 1/2] pinctrl: aspeed: Enable pass-through on GPIOE1 and GPIOE3 free

[Joel Stanley]

On Thu, 24 Feb 2022 at 21:03, Bills, Jason M
<jason.m.bills@linux.intel.com> wrote:
>
> >
> > Jason, you should send the patch to the upstream lists (use
> > get_maintainers.pl) for review.
> Sorry for the delay.  I found the right lists with get_maintainers.pl.
> Should I send these patches to the upstream lists as they are, or do
> they need to be tweaked?

You've got some review comments from Andrew and Zev that you should
address. I suggest replying to them, and adding the pinctrl list on
cc. Then when you post v2 you can send them to the upstream lists.

If you'd prefer to just send a v2 to the upstream lists to restart the
discussion you could do that too. Your call.

Cheers,

Joel

Re: [PATCH 1/2] pinctrl: aspeed: Enable pass-through on GPIOE1 and GPIOE3 free

[Bills, Jason M]

On 2/24/2022 9:49 PM, Joel Stanley wrote:
> On Thu, 24 Feb 2022 at 21:03, Bills, Jason M
> <jason.m.bills@linux.intel.com> wrote:
>>
>>>
>>> Jason, you should send the patch to the upstream lists (use
>>> get_maintainers.pl) for review.
>> Sorry for the delay.  I found the right lists with get_maintainers.pl.
>> Should I send these patches to the upstream lists as they are, or do
>> they need to be tweaked?
> 
> You've got some review comments from Andrew and Zev that you should
> address. I suggest replying to them, and adding the pinctrl list on
> cc. Then when you post v2 you can send them to the upstream lists.
> 
> If you'd prefer to just send a v2 to the upstream lists to restart the
> discussion you could do that too. Your call.
Thanks! Unless I missed something, I didn't see any specific feedback 
that would require a v2, yet.

It sounded like Andrew was theorizing a better implementation and was 
going think on it some more.  So, would it be better to wait for a 
cleaner implementation of this before we send it upstream?

Thanks,
-Jason


> 
> Cheers,
> 
> Joel

Re: [PATCH 1/2] pinctrl: aspeed: Enable pass-through on GPIOE1 and GPIOE3 free

[Andrew Jeffery]

On Sat, 26 Feb 2022, at 01:52, Bills, Jason M wrote:
> On 2/24/2022 9:49 PM, Joel Stanley wrote:
>> On Thu, 24 Feb 2022 at 21:03, Bills, Jason M
>> <jason.m.bills@linux.intel.com> wrote:
>>>
>>>>
>>>> Jason, you should send the patch to the upstream lists (use
>>>> get_maintainers.pl) for review.
>>> Sorry for the delay.  I found the right lists with get_maintainers.pl.
>>> Should I send these patches to the upstream lists as they are, or do
>>> they need to be tweaked?
>> 
>> You've got some review comments from Andrew and Zev that you should
>> address. I suggest replying to them, and adding the pinctrl list on
>> cc. Then when you post v2 you can send them to the upstream lists.
>> 
>> If you'd prefer to just send a v2 to the upstream lists to restart the
>> discussion you could do that too. Your call.
> Thanks! Unless I missed something, I didn't see any specific feedback 
> that would require a v2, yet.
>
> It sounded like Andrew was theorizing a better implementation and was 
> going think on it some more.  So, would it be better to wait for a 
> cleaner implementation of this before we send it upstream?

Yeah, I'm still thinking about it in the background. I think it's on me 
to cook something up before we push this further upstream.

Andrew