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
From:   Marek Szyprowski <m.szyprowski@samsung.com>
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
Message-ID: <b7648606-b5d1-c42f-cf68-338fbc3cf4df@samsung.com>
Date:   Wed, 21 Nov 2018 10:26:30 +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: <c426f174-dc45-3a25-4c7e-018972c2ceef@samsung.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>
        <c426f174-dc45-3a25-4c7e-018972c2ceef@samsung.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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