[PATCH] regulator: wm8994: Don't use devres for enable GPIOs

[PATCH] regulator: wm8994: Don't use devres for enable GPIOs

[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 <m.szyprowski@samsung.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---

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(-)

diff --git a/drivers/regulator/wm8994-regulator.c b/drivers/regulator/wm8994-regulator.c
index d7fec533c403..f2592c30d0ca 100644
--- a/drivers/regulator/wm8994-regulator.c
+++ b/drivers/regulator/wm8994-regulator.c
@@ -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",
 	},
-- 
2.11.0

Re: [PATCH] regulator: wm8994: Don't use devres for enable GPIOs

[Marek Szyprowski]

Hi Charles,

On 2018-11-20 18:01, Charles Keepax wrote:
> 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 <m.szyprowski@samsung.com>
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
>
> 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.


> Thanks,
> Charles
>
>  drivers/regulator/wm8994-regulator.c | 28 +++++++++++++++++++++++-----
>  1 file changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/regulator/wm8994-regulator.c b/drivers/regulator/wm8994-regulator.c
> index d7fec533c403..f2592c30d0ca 100644
> --- a/drivers/regulator/wm8994-regulator.c
> +++ b/drivers/regulator/wm8994-regulator.c
> @@ -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",
>  	},

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] regulator: wm8994: Don't use devres for enable GPIOs

[Marek Szyprowski]

Hi,

On 2018-11-20 18:25, Marek Szyprowski wrote:
> Hi Charles,
>
> On 2018-11-20 18:01, Charles Keepax wrote:
>> 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 <m.szyprowski@samsung.com>
>> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
>> ---
>>
>> 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.

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...

>
>> Thanks,
>> Charles
>>
>>  drivers/regulator/wm8994-regulator.c | 28 +++++++++++++++++++++++-----
>>  1 file changed, 23 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/regulator/wm8994-regulator.c b/drivers/regulator/wm8994-regulator.c
>> index d7fec533c403..f2592c30d0ca 100644
>> --- a/drivers/regulator/wm8994-regulator.c
>> +++ b/drivers/regulator/wm8994-regulator.c
>> @@ -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",
>>  	},
> Best regards

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] regulator: wm8994: Don't use devres for enable GPIOs

[Charles Keepax]

On Wed, Nov 21, 2018 at 10:26:30AM +0100, Marek Szyprowski wrote:
> On 2018-11-20 18:25, Marek Szyprowski wrote:
> > 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
> 
> 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

Re: [PATCH] regulator: wm8994: Don't use devres for enable GPIOs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 327 bytes --]

On Wed, Nov 21, 2018 at 10:26:30AM +0100, Marek Szyprowski wrote:

> 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()?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regulator: wm8994: Don't use devres for enable GPIOs

[Marek Szyprowski]

Hi Mark,

On 2018-11-21 13:19, Mark Brown wrote:
> On Wed, Nov 21, 2018 at 10:26:30AM +0100, Marek Szyprowski wrote:
>
>> 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
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland