Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] regulator: wm8994: Don't use devres for enable GPIOs
To:     Charles Keepax <ckeepax@opensource.cirrus.com>, broonie@kernel.org
Cc:     linus.walleij@linaro.org, lgirdwood@gmail.com,
        linux-kernel@vger.kernel.org, patches@opensource.cirrus.com
From:   Marek Szyprowski <m.szyprowski@samsung.com>
Message-ID: <c426f174-dc45-3a25-4c7e-018972c2ceef@samsung.com>
Date:   Tue, 20 Nov 2018 18:25:38 +0100
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:60.0) Gecko/20100101
        Thunderbird/60.3.1
MIME-Version: 1.0
In-Reply-To: <20181120170152.24652-1-ckeepax@opensource.cirrus.com>
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 201P
References: <CGME20181120170248epcas1p2a93ec9ff7f74ad4908166590ed548386@epcas1p2.samsung.com>
        <20181120170152.24652-1-ckeepax@opensource.cirrus.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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