diff mbox series

[4/4] gpio/rockchip: fetch deferred output settings on probe

Message ID 20210913224926.1260726-5-heiko@sntech.de (mailing list archive)
State New
Headers show
Series gpio/pinctrl-rockchip: Fixes for the recently separated gpio/pinctrl driver | expand

Commit Message

Heiko Stuebner Sept. 13, 2021, 10:49 p.m. UTC
Fetch the output settings the pinctrl driver may have created
for pinctrl hogs and set the relevant pins as requested.

Fixes: 9ce9a02039de ("pinctrl/rockchip: drop the gpio related codes")
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/gpio/gpio-rockchip.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Comments

Linus Walleij Sept. 17, 2021, 11:38 p.m. UTC | #1
On Tue, Sep 14, 2021 at 12:49 AM Heiko Stuebner <heiko@sntech.de> wrote:

> Fetch the output settings the pinctrl driver may have created
> for pinctrl hogs and set the relevant pins as requested.
>
> Fixes: 9ce9a02039de ("pinctrl/rockchip: drop the gpio related codes")
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>

Since this patch depends on patch 4/4 I applied this to the pinctrl
tree as well.

I still think this looks a bit kludgy but can't think of anything better
right now and we need a fix for the problem so this goes in.

But we need to think of something better,

Yours,
Linus Walleij
Heiko Stuebner Sept. 18, 2021, midnight UTC | #2
Hi Linus,

Am Samstag, 18. September 2021, 01:38:08 CEST schrieb Linus Walleij:
> On Tue, Sep 14, 2021 at 12:49 AM Heiko Stuebner <heiko@sntech.de> wrote:
> 
> > Fetch the output settings the pinctrl driver may have created
> > for pinctrl hogs and set the relevant pins as requested.
> >
> > Fixes: 9ce9a02039de ("pinctrl/rockchip: drop the gpio related codes")
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> 
> Since this patch depends on patch 4/4 I applied this to the pinctrl
> tree as well.
> 
> I still think this looks a bit kludgy but can't think of anything better
> right now and we need a fix for the problem so this goes in.
> 
> But we need to think of something better,

I'm all ears :-) . And yes I do agree with you that this is not very
elegant right now.

The issue is that the pinconf part for PIN_CONFIG_OUTPUT is actually
using the gpio controller to realize this setting. So when this ends up
in a pinctrl-hog, stuff explodes while probing the first pinctrl part.

I guess one way would be to somehow only do the pinctrl-hogs
_after_ all parts have probed.


Thinking about this, the component framework may be one option?
And then adding a pinctr-register / init+enable variant where the
pinctrl hogs can be aquired separately, not as part of pinctrl_enable?

Or maybe I'm thinking way too complex and a way easier solution
is around the corner ;-) .


Heiko
Linus Walleij Sept. 19, 2021, 2:47 p.m. UTC | #3
On Sat, Sep 18, 2021 at 2:00 AM Heiko Stübner <heiko@sntech.de> wrote:

> The issue is that the pinconf part for PIN_CONFIG_OUTPUT is actually
> using the gpio controller to realize this setting. So when this ends up
> in a pinctrl-hog, stuff explodes while probing the first pinctrl part.

The Nomadik driver has something similar, I came up with a solution
ages ago which isn't elegant either, so it's not like I'm any better :/

commit ab4a936247561cd998913bab5f15e3d3eaed1f9e
"pinctrl: nomadik: assure GPIO chips are populated"

> Thinking about this, the component framework may be one option?
> And then adding a pinctr-register / init+enable variant where the
> pinctrl hogs can be aquired separately, not as part of pinctrl_enable?

Check out my commit, but the component framework is what we
should ideally use (IMO) when drivers depend on each other
so I think you are right.

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
index 3335bd57761d..cf1b465db8c3 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c
@@ -689,6 +689,7 @@  static int rockchip_gpio_probe(struct platform_device *pdev)
 	struct device_node *pctlnp = of_get_parent(np);
 	struct pinctrl_dev *pctldev = NULL;
 	struct rockchip_pin_bank *bank = NULL;
+	struct rockchip_pin_output_deferred *cfg;
 	static int gpio;
 	int id, ret;
 
@@ -716,12 +717,33 @@  static int rockchip_gpio_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	/*
+	 * Prevent clashes with a deferred output setting
+	 * being added right at this moment.
+	 */
+	mutex_lock(&bank->deferred_lock);
+
 	ret = rockchip_gpiolib_register(bank);
 	if (ret) {
 		clk_disable_unprepare(bank->clk);
+		mutex_unlock(&bank->deferred_lock);
 		return ret;
 	}
 
+	while (!list_empty(&bank->deferred_output)) {
+		cfg = list_first_entry(&bank->deferred_output,
+				       struct rockchip_pin_output_deferred, head);
+		list_del(&cfg->head);
+
+		ret = rockchip_gpio_direction_output(&bank->gpio_chip, cfg->pin, cfg->arg);
+		if (ret)
+			dev_warn(dev, "setting output pin %u to %u failed\n", cfg->pin, cfg->arg);
+
+		kfree(cfg);
+	}
+
+	mutex_unlock(&bank->deferred_lock);
+
 	platform_set_drvdata(pdev, bank);
 	dev_info(dev, "probed %pOF\n", np);