regulator: wm8994: Don't use devres for enable GPIOs

PATCH Superseded Charles Keepax 3 participants 6 messages Nov 20, 2018 – Nov 21, 2018

Subsystems (2)

Voltage and current regulator framework Wolfson microelectronics drivers

lore

Share & tools

lore

Charles Keepax

author

2018-11-20 17:02 UTC #0 lore raw

PATCH 1/1 regulator: wm8994: Don't use devres for enable GPIOs

We need to manage the life time of the enable GPIO against the regulator device but the OF node lives on the parent MFD device. As such we can't use the devm functions which assume the same device will be used for both.

Reported-by Marek Szyprowski <[email protected]>

Signed-off-by Charles Keepax <[email protected]>

---

This patch is only build tested so it would be good to get a tested by from Marek before applying.

Thanks,
Charles

 drivers/regulator/wm8994-regulator.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

drivers/regulator/wm8994-regulator.c +23 -5

index d7fec533c403..f2592c30d0ca 100644
@@ -31,6 +31,7 @@ struct wm8994_ldo {
 	struct wm8994 *wm8994;
 	struct regulator_consumer_supply supply;
 	struct regulator_init_data init_data;
+	struct gpio_desc *ena_gpiod;
 };
 
 #define WM8994_LDO1_MAX_SELECTOR 0x7

@@ -147,14 +148,19 @@ static int wm8994_ldo_probe(struct platform_device *pdev)
 	config.regmap = wm8994->regmap;
 	config.init_data = &ldo->init_data;
 
-	/* Look up LDO enable GPIO from the parent device node */
-	gpiod = devm_gpiod_get_optional(pdev->dev.parent,
-					id ? "wlf,ldo2ena" : "wlf,ldo1ena",
-					GPIOD_OUT_LOW |
-					GPIOD_FLAGS_BIT_NONEXCLUSIVE);
+	/*
+	 * Look up LDO enable GPIO from the parent device node, we can't
+	 * use devm because it assumes the device we want to allocate
+	 * against is the same one that holds the OF node
+	 */
+	gpiod = gpiod_get_optional(pdev->dev.parent,
+				   id ? "wlf,ldo2ena" : "wlf,ldo1ena",
+				   GPIOD_OUT_LOW |
+				   GPIOD_FLAGS_BIT_NONEXCLUSIVE);
 	if (IS_ERR(gpiod))
 		return PTR_ERR(gpiod);
 	config.ena_gpiod = gpiod;
+	ldo->ena_gpiod = gpiod;
 
 	/* Use default constraints if none set up */
 	if (!pdata || !pdata->ldo[id].init_data || wm8994->dev->of_node) {

@@ -184,11 +190,23 @@ static int wm8994_ldo_probe(struct platform_device *pdev)
 	return 0;
 
 err:
+	gpiod_put(gpiod);
 	return ret;
 }
 
+static int wm8994_ldo_remove(struct platform_device *pdev)
+{
+	struct wm8994_ldo *ldo = platform_get_drvdata(pdev);
+
+	if (ldo->ena_gpiod)
+		gpiod_put(ldo->ena_gpiod);
+
+	return 0;
+}
+
 static struct platform_driver wm8994_ldo_driver = {
 	.probe = wm8994_ldo_probe,
+	.remove = wm8994_ldo_remove,
 	.driver		= {
 		.name	= "wm8994-ldo",
 	},

Signature · 1 line

2.11.0

Marek Szyprowski

2018-11-20 17:26 UTC #1 lore raw

Hi Charles,

Quoted 11 lines · Charles Keepax

We need to manage the life time of the enable GPIO against the regulator device but the OF node lives on the parent MFD device. As such we can't use the devm functions which assume the same device will be used for both.
Reported-by Marek Szyprowski <[email protected]>
Signed-off-by Charles Keepax <[email protected]>
---
This patch is only build tested so it would be good to get a tested by from Marek before applying.

Sadly it looks that it is not enough. The issue is still there:

wm8994 4-001a: Failed to get supply 'DBVDD1': -517 wm8994 4-001a: Failed to get supplies: -517

------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at drivers/gpio/gpiolib.c:2421
regulator_ena_gpio_free+0x70/0xa0
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted
4.20.0-rc3-next-20181120-00001-g330f37c7fb0c #5090
Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[<c0111a54>] (unwind_backtrace) from [<c010d900>] (show_stack+0x10/0x14)
[<c010d900>] (show_stack) from [<c09e5878>] (dump_stack+0x90/0xc8)
[<c09e5878>] (dump_stack) from [<c0125998>] (__warn+0xf8/0x124)
[<c0125998>] (__warn) from [<c0125a04>] (warn_slowpath_null+0x40/0x48)

[<c0125a04>] (warn_slowpath_null) from [<c04a5b68>] (regulator_ena_gpio_free+0x70/0xa0)
[<c04a5b68>] (regulator_ena_gpio_free) from [<c04aa03c>] (regulator_unregister+0x134/0x17c)
[<c04aa03c>] (regulator_unregister) from [<c054b5f8>] (release_nodes+0x164/0x1dc)
[<c054b5f8>] (release_nodes) from [<c054810c>] (device_release_driver_internal+0x18c/0x21c)
[<c054810c>] (device_release_driver_internal) from [<c054698c>] (bus_remove_device+0xdc/0x12c)
[<c054698c>] (bus_remove_device) from [<c05434bc>] (device_del+0x108/0x2e0) [<c05434bc>] (device_del) from [<c0549c38>] (platform_device_del+0x20/0x8c) [<c0549c38>] (platform_device_del) from [<c0549cb0>] (platform_device_unregister+0xc/0x18)
[<c0549cb0>] (platform_device_unregister) from [<c0575120>] (mfd_remove_devices_fn+0x58/0x64)
[<c0575120>] (mfd_remove_devices_fn) from [<c05425a8>] (device_for_each_child_reverse+0x34/0x74)
[<c05425a8>] (device_for_each_child_reverse) from [<c0574f18>] (mfd_remove_devices+0x20/0x30)
[<c0574f18>] (mfd_remove_devices) from [<c0573b1c>] (wm8994_i2c_probe+0x2dc/0x8d0)
[<c0573b1c>] (wm8994_i2c_probe) from [<c068bce4>] (i2c_device_probe+0x234/0x2b0)
[<c068bce4>] (i2c_device_probe) from [<c0547774>] (really_probe+0x224/0x3f4) [<c0547774>] (really_probe) from [<c0547b98>] (driver_probe_device+0x70/0x1c4)
[<c0547b98>] (driver_probe_device) from [<c0547e0c>] (__driver_attach+0x120/0x144)
[<c0547e0c>] (__driver_attach) from [<c05459f0>] (bus_for_each_dev+0x58/0x7c)
[<c05459f0>] (bus_for_each_dev) from [<c0546b50>] (bus_add_driver+0x174/0x250)
[<c0546b50>] (bus_add_driver) from [<c0548db8>] (driver_register+0x7c/0x110) [<c0548db8>] (driver_register) from [<c068ddcc>] (i2c_register_driver+0x38/0xa8)
[<c068ddcc>] (i2c_register_driver) from [<c01030fc>] (do_one_initcall+0x80/0x408)
[<c01030fc>] (do_one_initcall) from [<c0f011e4>] (kernel_init_freeable+0x3a0/0x4fc)
[<c0f011e4>] (kernel_init_freeable) from [<c09fdd6c>] (kernel_init+0x8/0x118)
[<c09fdd6c>] (kernel_init) from [<c01010b4>] (ret_from_fork+0x14/0x20)

Exception stack(0xef0e3fb0 to 0xef0e3ff8)

3fa0: 00000000 00000000 00000000 00000000
3fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
3fe0: 00000000 00000000 00000000 00000000 00000013 00000000

irq event stamp: 176913
hardirqs last  enabled at (176921): [<c018b1c0>] console_unlock+0x460/0x6ec
hardirqs last disabled at (176940): [<c018ae18>] console_unlock+0xb8/0x6ec
softirqs last  enabled at (176956): [<c01024e4>] __do_softirq+0x3a4/0x66c
softirqs last disabled at (176967): [<c012d618>] irq_exit+0x140/0x168

I will check more tomorrow.

Quoted 57 lines

Thanks,
Charles

 drivers/regulator/wm8994-regulator.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

drivers/regulator/wm8994-regulator.c +18 -5

index d7fec533c403..f2592c30d0ca 100644
@@ -31,6 +31,7 @@ struct wm8994_ldo {
 	struct wm8994 *wm8994;
 	struct regulator_consumer_supply supply;
 	struct regulator_init_data init_data;
+	struct gpio_desc *ena_gpiod;
 };
 
 #define WM8994_LDO1_MAX_SELECTOR 0x7

@@ -147,14 +148,19 @@ static int wm8994_ldo_probe(struct platform_device *pdev)
 	config.regmap = wm8994->regmap;
 	config.init_data = &ldo->init_data;
 
-	/* Look up LDO enable GPIO from the parent device node */
-	gpiod = devm_gpiod_get_optional(pdev->dev.parent,
-					id ? "wlf,ldo2ena" : "wlf,ldo1ena",
-					GPIOD_OUT_LOW |
-					GPIOD_FLAGS_BIT_NONEXCLUSIVE);
+	/*
+	 * Look up LDO enable GPIO from the parent device node, we can't
+	 * use devm because it assumes the device we want to allocate
+	 * against is the same one that holds the OF node
+	 */
+	gpiod = gpiod_get_optional(pdev->dev.parent,
+				   id ? "wlf,ldo2ena" : "wlf,ldo1ena",
+				   GPIOD_OUT_LOW |
+				   GPIOD_FLAGS_BIT_NONEXCLUSIVE);
 	if (IS_ERR(gpiod))
 		return PTR_ERR(gpiod);
 	config.ena_gpiod = gpiod;
+	ldo->ena_gpiod = gpiod;
 
 	/* Use default constraints if none set up */
 	if (!pdata || !pdata->ldo[id].init_data || wm8994->dev->of_node) {

@@ -184,11 +190,23 @@ static int wm8994_ldo_probe(struct platform_device *pdev)
 	return 0;
 
 err:
+	gpiod_put(gpiod);
 	return ret;
 }
 
+static int wm8994_ldo_remove(struct platform_device *pdev)
+{
+	struct wm8994_ldo *ldo = platform_get_drvdata(pdev);
+
+	if (ldo->ena_gpiod)
+		gpiod_put(ldo->ena_gpiod);

drivers/regulator/wm8994-regulator.c static int wm8994_ldo_probe(struct platform_device *pdev) · line 212

Quoted 10 lines

drivers/regulator/wm8994-regulator.c +5 -0

+
+	return 0;
+}
+
 static struct platform_driver wm8994_ldo_driver = {
 	.probe = wm8994_ldo_probe,
+	.remove = wm8994_ldo_remove,
 	.driver		= {
 		.name	= "wm8994-ldo",
 	},

Best regards

Signature · 2 lines

Marek Szyprowski, PhD
Samsung R&D Institute Poland

Marek Szyprowski

2018-11-21 09:26 UTC #2 lore raw

Hi,

Quoted 78 lines · Marek Szyprowski

Hi Charles,
Quoted 11 lines · Charles Keepax
We need to manage the life time of the enable GPIO against the regulator device but the OF node lives on the parent MFD device. As such we can't use the devm functions which assume the same device will be used for both.
Reported-by Marek Szyprowski <[email protected]>
Signed-off-by Charles Keepax <[email protected]>
---
This patch is only build tested so it would be good to get a tested by from Marek before applying.
Sadly it looks that it is not enough. The issue is still there:
wm8994 4-001a: Failed to get supply 'DBVDD1': -517 wm8994 4-001a: Failed to get supplies: -517
------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at drivers/gpio/gpiolib.c:2421
regulator_ena_gpio_free+0x70/0xa0
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted
4.20.0-rc3-next-20181120-00001-g330f37c7fb0c #5090
Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[<c0111a54>] (unwind_backtrace) from [<c010d900>] (show_stack+0x10/0x14)
[<c010d900>] (show_stack) from [<c09e5878>] (dump_stack+0x90/0xc8)
[<c09e5878>] (dump_stack) from [<c0125998>] (__warn+0xf8/0x124)
[<c0125998>] (__warn) from [<c0125a04>] (warn_slowpath_null+0x40/0x48)
[<c0125a04>] (warn_slowpath_null) from [<c04a5b68>] (regulator_ena_gpio_free+0x70/0xa0)
[<c04a5b68>] (regulator_ena_gpio_free) from [<c04aa03c>] (regulator_unregister+0x134/0x17c)
[<c04aa03c>] (regulator_unregister) from [<c054b5f8>] (release_nodes+0x164/0x1dc)
[<c054b5f8>] (release_nodes) from [<c054810c>] (device_release_driver_internal+0x18c/0x21c)
[<c054810c>] (device_release_driver_internal) from [<c054698c>] (bus_remove_device+0xdc/0x12c)
[<c054698c>] (bus_remove_device) from [<c05434bc>] (device_del+0x108/0x2e0) [<c05434bc>] (device_del) from [<c0549c38>] (platform_device_del+0x20/0x8c) [<c0549c38>] (platform_device_del) from [<c0549cb0>] (platform_device_unregister+0xc/0x18)
[<c0549cb0>] (platform_device_unregister) from [<c0575120>] (mfd_remove_devices_fn+0x58/0x64)
[<c0575120>] (mfd_remove_devices_fn) from [<c05425a8>] (device_for_each_child_reverse+0x34/0x74)
[<c05425a8>] (device_for_each_child_reverse) from [<c0574f18>] (mfd_remove_devices+0x20/0x30)
[<c0574f18>] (mfd_remove_devices) from [<c0573b1c>] (wm8994_i2c_probe+0x2dc/0x8d0)
[<c0573b1c>] (wm8994_i2c_probe) from [<c068bce4>] (i2c_device_probe+0x234/0x2b0)
[<c068bce4>] (i2c_device_probe) from [<c0547774>] (really_probe+0x224/0x3f4) [<c0547774>] (really_probe) from [<c0547b98>] (driver_probe_device+0x70/0x1c4)
[<c0547b98>] (driver_probe_device) from [<c0547e0c>] (__driver_attach+0x120/0x144)
[<c0547e0c>] (__driver_attach) from [<c05459f0>] (bus_for_each_dev+0x58/0x7c)
[<c05459f0>] (bus_for_each_dev) from [<c0546b50>] (bus_add_driver+0x174/0x250)
[<c0546b50>] (bus_add_driver) from [<c0548db8>] (driver_register+0x7c/0x110) [<c0548db8>] (driver_register) from [<c068ddcc>] (i2c_register_driver+0x38/0xa8)
[<c068ddcc>] (i2c_register_driver) from [<c01030fc>] (do_one_initcall+0x80/0x408)
[<c01030fc>] (do_one_initcall) from [<c0f011e4>] (kernel_init_freeable+0x3a0/0x4fc)
[<c0f011e4>] (kernel_init_freeable) from [<c09fdd6c>] (kernel_init+0x8/0x118)
[<c09fdd6c>] (kernel_init) from [<c01010b4>] (ret_from_fork+0x14/0x20)
Exception stack(0xef0e3fb0 to 0xef0e3ff8)
3fa0: 00000000 00000000 00000000 00000000
3fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
3fe0: 00000000 00000000 00000000 00000000 00000013 00000000

Quoted 7 lines

irq event stamp: 176913
hardirqs last  enabled at (176921): [<c018b1c0>] console_unlock+0x460/0x6ec
hardirqs last disabled at (176940): [<c018ae18>] console_unlock+0xb8/0x6ec
softirqs last  enabled at (176956): [<c01024e4>] __do_softirq+0x3a4/0x66c
softirqs last disabled at (176967): [<c012d618>] irq_exit+0x140/0x168

I will check more tomorrow.

I've checked a bit more and it looks that the issue is caused by gpiod_put() in regulator_ena_gpio_free(). I've removed gpiod_put() call from wm8994_ldo_remove(), switched to regulator_register() in wm8994_ldo_probe() and added regulator_unregister() in wm8994_ldo_remove(). This way the warning is gone. If there is any other, simpler way to fix it, let me know. When regulator_register() fails, gpiod will be freed only in some cases, depending what caused failure. It looks that this needs some clarification...

Quoted 61 lines

Thanks,
Charles

 drivers/regulator/wm8994-regulator.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

drivers/regulator/wm8994-regulator.c +21 -5

index d7fec533c403..f2592c30d0ca 100644
@@ -31,6 +31,7 @@ struct wm8994_ldo {
 	struct wm8994 *wm8994;
 	struct regulator_consumer_supply supply;
 	struct regulator_init_data init_data;
+	struct gpio_desc *ena_gpiod;
 };
 
 #define WM8994_LDO1_MAX_SELECTOR 0x7

@@ -147,14 +148,19 @@ static int wm8994_ldo_probe(struct platform_device *pdev)
 	config.regmap = wm8994->regmap;
 	config.init_data = &ldo->init_data;
 
-	/* Look up LDO enable GPIO from the parent device node */
-	gpiod = devm_gpiod_get_optional(pdev->dev.parent,
-					id ? "wlf,ldo2ena" : "wlf,ldo1ena",
-					GPIOD_OUT_LOW |
-					GPIOD_FLAGS_BIT_NONEXCLUSIVE);
+	/*
+	 * Look up LDO enable GPIO from the parent device node, we can't
+	 * use devm because it assumes the device we want to allocate
+	 * against is the same one that holds the OF node
+	 */
+	gpiod = gpiod_get_optional(pdev->dev.parent,
+				   id ? "wlf,ldo2ena" : "wlf,ldo1ena",
+				   GPIOD_OUT_LOW |
+				   GPIOD_FLAGS_BIT_NONEXCLUSIVE);
 	if (IS_ERR(gpiod))
 		return PTR_ERR(gpiod);
 	config.ena_gpiod = gpiod;
+	ldo->ena_gpiod = gpiod;
 
 	/* Use default constraints if none set up */
 	if (!pdata || !pdata->ldo[id].init_data || wm8994->dev->of_node) {

@@ -184,11 +190,23 @@ static int wm8994_ldo_probe(struct platform_device *pdev)
 	return 0;
 
 err:
+	gpiod_put(gpiod);
 	return ret;
 }
 
+static int wm8994_ldo_remove(struct platform_device *pdev)
+{
+	struct wm8994_ldo *ldo = platform_get_drvdata(pdev);
+
+	if (ldo->ena_gpiod)
+		gpiod_put(ldo->ena_gpiod);
+
+	return 0;
+}

Quoted 8 lines

drivers/regulator/wm8994-regulator.c static int wm8994_ldo_probe(struct platform_device *pdev) · line 212
Quoted 7 lines
drivers/regulator/wm8994-regulator.c +2 -0
+
 static struct platform_driver wm8994_ldo_driver = {
 	.probe = wm8994_ldo_probe,
+	.remove = wm8994_ldo_remove,
 	.driver		= {
 		.name	= "wm8994-ldo",
 	},
Best regards

Best regards

Signature · 2 lines

Marek Szyprowski, PhD
Samsung R&D Institute Poland

Charles Keepax

author

2018-11-21 10:07 UTC #3 lore raw

Quoted 6 lines · Marek Szyprowski

Quoted 4 lines · Marek Szyprowski
On 2018-11-20 18:01, Charles Keepax wrote:
------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at drivers/gpio/gpiolib.c:2421
regulator_ena_gpio_free+0x70/0xa0

Quoted 9 lines

I've checked a bit more and it looks that the issue is caused by gpiod_put() in regulator_ena_gpio_free(). I've removed gpiod_put() call from wm8994_ldo_remove(), switched to regulator_register() in wm8994_ldo_probe() and added regulator_unregister() in wm8994_ldo_remove(). This way the warning is gone. If there is any other, simpler way to fix it, let me know. When regulator_register() fails, gpiod will be freed only in some cases, depending what caused failure. It looks that this needs some clarification...

Ah.. ok, so is perhaps what is happening here that both the devm_gpio_get and the regualtor_ena_gpio_free are both trying to free the GPIO? I assume the regulator core does this to handle the case of shared GPIOs, so presumably the correct fix would just be to switch from devm_gpiod_get to just plain gpiod_get and not add the additional gpiod_put in wm8994_ldo_remove, as the regulator core will handle freeing the GPIO for us.

I will respin the patch and lets see where that gets us to.

Thanks,
Charles

Mark Brown

maintainer

2018-11-21 12:19 UTC #4 lore raw

In reply to Marek Szyprowski

Quoted 3 lines · Marek Szyprowski

from wm8994_ldo_remove(), switched to regulator_register() in wm8994_ldo_probe() and added regulator_unregister() in wm8994_ldo_remove(). This way the warning is gone. If there is any

I don't understand why you'd need to switch to regulator_register()?

Marek Szyprowski

2018-11-21 12:24 UTC #5 lore raw

Hi Mark,

Quoted 6 lines · Mark Brown

Quoted 3 lines · Marek Szyprowski
from wm8994_ldo_remove(), switched to regulator_register() in wm8994_ldo_probe() and added regulator_unregister() in wm8994_ldo_remove(). This way the warning is gone. If there is any
I don't understand why you'd need to switch to regulator_register()?

I mixed a few things. This was just a first version of my experiments, which worked without the warning. In the final solution it was not needed, see https://lkml.org/lkml/2018/11/21/542

Best regards

Signature · 2 lines

Marek Szyprowski, PhD
Samsung R&D Institute Poland