[PATCH RESEND 0/4] Minor WM8994 MFD/codec fixes
Hi! This is a resend of the minor WM8994 MFD/codec driver fixes posted in last days of the February 2020: https://lore.kernel.org/patchwork/project/lkml/list/?series=431296 https://lore.kernel.org/patchwork/project/lkml/list/?series=431721 I hope this time the patches will find their way to mainline. Best regards, Marek Szyprowski Patch summary: Marek Szyprowski (4): mfd: wm8994: Fix driver operation if loaded as modules mfd: wm8994: Fix unbalanced calls to regulator_bulk_disable() mfd: wm8994: Silence warning about supplies during deferred probe ASoC: wm8994: Silence warnings during deferred probe drivers/mfd/wm8994-core.c | 8 +++++++- sound/soc/codecs/wm8994.c | 3 ++- 2 files changed, 9 insertions(+), 2 deletions(-) -- 2.17.1
WM8994 chip has built-in regulators, which might be used for chip operation. They are controlled by a separate wm8994-regulator driver, which should be loaded before this driver calls regulator_get(), because that driver also provides consumer-supply mapping for the them. If that driver is not yet loaded, regulator core substitute them with dummy regulator, what breaks chip operation, because the built-in regulators are never enabled. Fix this by annotating this driver with MODULE_SOFTDEP() "pre" dependency to "wm8994_regulator" module. Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> Acked-by: Charles Keepax <ckeepax@opensource.cirrus.com> --- drivers/mfd/wm8994-core.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/mfd/wm8994-core.c b/drivers/mfd/wm8994-core.c index 1e9fe7d92597..737dede4a95c 100644 --- a/drivers/mfd/wm8994-core.c +++ b/drivers/mfd/wm8994-core.c @@ -690,3 +690,4 @@ module_i2c_driver(wm8994_i2c_driver); MODULE_DESCRIPTION("Core support for the WM8994 audio CODEC"); MODULE_LICENSE("GPL"); MODULE_AUTHOR("Mark Brown <broonie@opensource.wolfsonmicro.com>"); +MODULE_SOFTDEP("pre: wm8994_regulator"); -- 2.17.1
On Mon, 27 Apr 2020, Marek Szyprowski wrote:
WM8994 chip has built-in regulators, which might be used for chip operation. They are controlled by a separate wm8994-regulator driver, which should be loaded before this driver calls regulator_get(), because that driver also provides consumer-supply mapping for the them. If that driver is not yet loaded, regulator core substitute them with dummy regulator, what breaks chip operation, because the built-in regulators are never enabled. Fix this by annotating this driver with MODULE_SOFTDEP() "pre" dependency to "wm8994_regulator" module.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> Acked-by: Charles Keepax <ckeepax@opensource.cirrus.com> --- drivers/mfd/wm8994-core.c | 1 + 1 file changed, 1 insertion(+)
For my own reference (apply this as-is to your sign-off block): Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org> -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
On Mon, 27 Apr 2020, Marek Szyprowski wrote:
WM8994 chip has built-in regulators, which might be used for chip operation. They are controlled by a separate wm8994-regulator driver, which should be loaded before this driver calls regulator_get(), because that driver also provides consumer-supply mapping for the them. If that driver is not yet loaded, regulator core substitute them with dummy regulator, what breaks chip operation, because the built-in regulators are never enabled. Fix this by annotating this driver with MODULE_SOFTDEP() "pre" dependency to "wm8994_regulator" module.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> Acked-by: Charles Keepax <ckeepax@opensource.cirrus.com> --- drivers/mfd/wm8994-core.c | 1 + 1 file changed, 1 insertion(+)
Applied, thanks. -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
When runtime PM is enabled, regulators are being controlled by the driver's suspend and resume callbacks. They are also unconditionally enabled at driver's probe(), and disabled in remove() functions. Add more calls to runtime PM framework to ensure that the device's runtime PM state matches the regulators state: 1. at the end of probe() function: set runtime PM state to active, so there will be no spurious call to resume(); 2. in remove(), ensure that resume() is called before disabling runtime PM management and unconditionally disabling the regulators. Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> Acked-by: Charles Keepax <ckeepax@opensource.cirrus.com> --- drivers/mfd/wm8994-core.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/mfd/wm8994-core.c b/drivers/mfd/wm8994-core.c index 737dede4a95c..69d973ec42bf 100644 --- a/drivers/mfd/wm8994-core.c +++ b/drivers/mfd/wm8994-core.c @@ -584,6 +584,7 @@ static int wm8994_device_init(struct wm8994 *wm8994, int irq) goto err_irq; } + pm_runtime_set_active(wm8994->dev); pm_runtime_enable(wm8994->dev); pm_runtime_idle(wm8994->dev); @@ -603,7 +604,9 @@ static int wm8994_device_init(struct wm8994 *wm8994, int irq) static void wm8994_device_exit(struct wm8994 *wm8994) { + pm_runtime_get_sync(wm8994->dev); pm_runtime_disable(wm8994->dev); + pm_runtime_put_noidle(wm8994->dev); wm8994_irq_exit(wm8994); regulator_bulk_disable(wm8994->num_supplies, wm8994->supplies); regulator_bulk_free(wm8994->num_supplies, wm8994->supplies); -- 2.17.1
On Mon, 27 Apr 2020, Marek Szyprowski wrote:
When runtime PM is enabled, regulators are being controlled by the driver's suspend and resume callbacks. They are also unconditionally enabled at driver's probe(), and disabled in remove() functions. Add more calls to runtime PM framework to ensure that the device's runtime PM state matches the regulators state: 1. at the end of probe() function: set runtime PM state to active, so there will be no spurious call to resume(); 2. in remove(), ensure that resume() is called before disabling runtime PM management and unconditionally disabling the regulators.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> Acked-by: Charles Keepax <ckeepax@opensource.cirrus.com> --- drivers/mfd/wm8994-core.c | 3 +++ 1 file changed, 3 insertions(+)
For my own reference (apply this as-is to your sign-off block): Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org> -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
On Mon, 27 Apr 2020, Marek Szyprowski wrote:
When runtime PM is enabled, regulators are being controlled by the driver's suspend and resume callbacks. They are also unconditionally enabled at driver's probe(), and disabled in remove() functions. Add more calls to runtime PM framework to ensure that the device's runtime PM state matches the regulators state: 1. at the end of probe() function: set runtime PM state to active, so there will be no spurious call to resume(); 2. in remove(), ensure that resume() is called before disabling runtime PM management and unconditionally disabling the regulators.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> Acked-by: Charles Keepax <ckeepax@opensource.cirrus.com> --- drivers/mfd/wm8994-core.c | 3 +++ 1 file changed, 3 insertions(+)
Applied, thanks. -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
Don't confuse user with meaningless warning about the failure in getting supplies in case of deferred probe. Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- drivers/mfd/wm8994-core.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/mfd/wm8994-core.c b/drivers/mfd/wm8994-core.c index 69d973ec42bf..3b2b93c5bbcb 100644 --- a/drivers/mfd/wm8994-core.c +++ b/drivers/mfd/wm8994-core.c @@ -393,7 +393,9 @@ static int wm8994_device_init(struct wm8994 *wm8994, int irq) ret = regulator_bulk_get(wm8994->dev, wm8994->num_supplies, wm8994->supplies); if (ret != 0) { - dev_err(wm8994->dev, "Failed to get supplies: %d\n", ret); + if (ret != -EPROBE_DEFER) + dev_err(wm8994->dev, "Failed to get supplies: %d\n", + ret); goto err; } -- 2.17.1
On Mon, Apr 27, 2020 at 09:48:31AM +0200, Marek Szyprowski wrote:
Don't confuse user with meaningless warning about the failure in getting supplies in case of deferred probe.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> ---
Acked-by: Charles Keepax <ckeepax@opensource.cirrus.com> Thanks, Charles
On Mon, 27 Apr 2020, Marek Szyprowski wrote:
Don't confuse user with meaningless warning about the failure in getting supplies in case of deferred probe.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- drivers/mfd/wm8994-core.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
Applied, thanks. -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
Don't confuse user with meaningless warning about the failure in getting clocks in case of deferred probe. Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- sound/soc/codecs/wm8994.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sound/soc/codecs/wm8994.c b/sound/soc/codecs/wm8994.c index 55d0b9be6ff0..7426df1f806c 100644 --- a/sound/soc/codecs/wm8994.c +++ b/sound/soc/codecs/wm8994.c @@ -4593,7 +4593,8 @@ static int wm8994_probe(struct platform_device *pdev) ret = devm_clk_bulk_get_optional(pdev->dev.parent, ARRAY_SIZE(wm8994->mclk), wm8994->mclk); if (ret < 0) { - dev_err(&pdev->dev, "Failed to get clocks: %d\n", ret); + if (ret != -EPROBE_DEFER) + dev_err(&pdev->dev, "Failed to get clocks: %d\n", ret); return ret; } -- 2.17.1
On Mon, Apr 27, 2020 at 09:48:32AM +0200, Marek Szyprowski wrote:
Don't confuse user with meaningless warning about the failure in getting clocks in case of deferred probe.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> ---
Acked-by: Charles Keepax <ckeepax@opensource.cirrus.com> Thanks, Charles
On Mon, Apr 27, 2020 at 09:48:32AM +0200, Marek Szyprowski wrote:
Don't confuse user with meaningless warning about the failure in getting clocks in case of deferred probe.
if (ret < 0) { - dev_err(&pdev->dev, "Failed to get clocks: %d\n", ret); + if (ret != -EPROBE_DEFER) + dev_err(&pdev->dev, "Failed to get clocks: %d\n", ret);
This completely eliminates the diagnostics which means that if the clock isn't there the user is a bit stuck trying to work out what's missing. There should still be a diagnostic.
On Mon, 27 Apr 2020, Mark Brown wrote:
On Mon, Apr 27, 2020 at 09:48:32AM +0200, Marek Szyprowski wrote:
Don't confuse user with meaningless warning about the failure in getting clocks in case of deferred probe.
if (ret < 0) { - dev_err(&pdev->dev, "Failed to get clocks: %d\n", ret); + if (ret != -EPROBE_DEFER) + dev_err(&pdev->dev, "Failed to get clocks: %d\n", ret);
This completely eliminates the diagnostics which means that if the clock isn't there the user is a bit stuck trying to work out what's missing. There should still be a diagnostic.
The driver won't defer forever though. The final pass should fail with a different error. At which point the error will be released to the system log, no? -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
On Tue, Apr 28, 2020 at 11:36:38AM +0100, Lee Jones wrote:
On Mon, 27 Apr 2020, Mark Brown wrote:
This completely eliminates the diagnostics which means that if the clock isn't there the user is a bit stuck trying to work out what's missing. There should still be a diagnostic.
The driver won't defer forever though. The final pass should fail with a different error. At which point the error will be released to the system log, no?
One of the really common cases is that someone forgot to build the driver for the dependency so it'll just defer forever waiting for something that never loads.
On Tue, 28 Apr 2020, Mark Brown wrote:
On Tue, Apr 28, 2020 at 11:36:38AM +0100, Lee Jones wrote:
On Mon, 27 Apr 2020, Mark Brown wrote:
This completely eliminates the diagnostics which means that if the clock isn't there the user is a bit stuck trying to work out what's missing. There should still be a diagnostic.
The driver won't defer forever though. The final pass should fail with a different error. At which point the error will be released to the system log, no?
One of the really common cases is that someone forgot to build the driver for the dependency so it'll just defer forever waiting for something that never loads.
Need to find another way to identify these failures. There are 10's if not 100's of cases of silently returning if -EPROBE_DEFER is caught. -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
On Wed, Apr 29, 2020 at 08:15:53AM +0100, Lee Jones wrote:
On Tue, 28 Apr 2020, Mark Brown wrote:
One of the really common cases is that someone forgot to build the driver for the dependency so it'll just defer forever waiting for something that never loads.
Need to find another way to identify these failures. There are 10's if not 100's of cases of silently returning if -EPROBE_DEFER is caught.
Or someone could go through and improve the diagnostics on those cases.
Hi All, On 27.04.2020 09:48, Marek Szyprowski wrote:
This is a resend of the minor WM8994 MFD/codec driver fixes posted in last days of the February 2020:
https://lore.kernel.org/patchwork/project/lkml/list/?series=431296 https://lore.kernel.org/patchwork/project/lkml/list/?series=431721
I hope this time the patches will find their way to mainline.
Gentle ping. Lee: could you queue at least the first 2 patches if you consider the latter 2 a bit controversial? Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland
participants (4)
-
Charles Keepax -
Lee Jones -
Marek Szyprowski -
Mark Brown