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

From: Marek Szyprowski
Date: Wed Nov 21 2018 - 04:26:37 EST

Next message: Christophe Leroy: "[PATCH v3] powerpc/mm: dump block address translation on book3s/32"
Previous message: Aneesh Kumar K.V: "[PATCH V4 3/3] powerpc/mm/iommu: Allow large IOMMU page size only for hugetlb backing"
In reply to: Marek Szyprowski: "Re: [PATCH] regulator: wm8994: Don't use devres for enable GPIOs"
Next in thread: Charles Keepax: "Re: [PATCH] regulator: wm8994: Don't use devres for enable GPIOs"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxxx>
>> Signed-off-by: Charles Keepax <ckeepax@xxxxxxxxxxxxxxxxxxxxx>
>> ---
>>
>> 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

Next message: Christophe Leroy: "[PATCH v3] powerpc/mm: dump block address translation on book3s/32"
Previous message: Aneesh Kumar K.V: "[PATCH V4 3/3] powerpc/mm/iommu: Allow large IOMMU page size only for hugetlb backing"
In reply to: Marek Szyprowski: "Re: [PATCH] regulator: wm8994: Don't use devres for enable GPIOs"
Next in thread: Charles Keepax: "Re: [PATCH] regulator: wm8994: Don't use devres for enable GPIOs"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]