[alsa-devel] [PATCH v2 0/9] ARM: dts: tegra: last minute fixes
This series comes with some last minutes fixes and further clean-up.
Changes in v2: - Dropped "[PATCH v1 3/8] ARM: tegra: apalis/colibri_t30: fix hdmi regulator" as suggested by Russell et. al. - Added 2 new patches improving/fixing audio on Apalis TK1.
Changes in v1: - Remove simple-panel compatible as suggested by Rob.
Marcel Ziswiler (9): ARM: tegra: fix simple-panel compatibles ARM: tegra: apalis-tk1/colibri_t20/t30: eval/iris: fix regulator gpio enable ARM: tegra: colibri_t20: reorder pmic properties ARM: tegra: apalis-tk1: further regulator clean-up ARM: tegra: apalis_t30/tk1: annotate power I2C being on-module ARM: tegra: colibri_t30: further regulator clean-up ARM: tegra: apalis_t30: further regulator clean-up ASoC: tegra_sgtl5000: fix device_node refcounting ASoC: tegra_sgtl5000: fix platform name vs. of_node assignement
arch/arm/boot/dts/tegra114-dalmore.dts | 3 +- arch/arm/boot/dts/tegra124-apalis-eval.dts | 4 +- arch/arm/boot/dts/tegra124-apalis-v1.2-eval.dts | 4 +- arch/arm/boot/dts/tegra124-apalis-v1.2.dtsi | 17 +++--- arch/arm/boot/dts/tegra124-apalis.dtsi | 17 +++--- arch/arm/boot/dts/tegra124-venice2.dts | 2 +- arch/arm/boot/dts/tegra20-colibri-eval-v3.dts | 4 +- arch/arm/boot/dts/tegra20-colibri-iris.dts | 4 +- arch/arm/boot/dts/tegra20-colibri.dtsi | 4 +- arch/arm/boot/dts/tegra20-harmony.dts | 2 +- arch/arm/boot/dts/tegra20-medcom-wide.dts | 2 +- arch/arm/boot/dts/tegra20-paz00.dts | 2 +- arch/arm/boot/dts/tegra20-seaboard.dts | 2 +- arch/arm/boot/dts/tegra20-ventana.dts | 2 +- arch/arm/boot/dts/tegra30-apalis-eval.dts | 2 +- arch/arm/boot/dts/tegra30-apalis-v1.1-eval.dts | 4 +- arch/arm/boot/dts/tegra30-apalis-v1.1.dtsi | 72 +++++++++++-------------- arch/arm/boot/dts/tegra30-apalis.dtsi | 70 ++++++++++-------------- arch/arm/boot/dts/tegra30-cardhu.dtsi | 2 +- arch/arm/boot/dts/tegra30-colibri-eval-v3.dts | 4 +- arch/arm/boot/dts/tegra30-colibri.dtsi | 45 ++++++++-------- sound/soc/tegra/tegra_sgtl5000.c | 22 +++++++- 22 files changed, 143 insertions(+), 147 deletions(-)
From: Marcel Ziswiler marcel.ziswiler@toradex.com
Similar to the following:
commit 4321723648b0 ("ASoC: tegra_alc5632: fix device_node refcounting")
commit 7c5dfd549617 ("ASoC: tegra: fix device_node refcounting")
Signed-off-by: Marcel Ziswiler marcel.ziswiler@toradex.com
---
Changes in v2: New patch
sound/soc/tegra/tegra_sgtl5000.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/sound/soc/tegra/tegra_sgtl5000.c b/sound/soc/tegra/tegra_sgtl5000.c index 45a4aa9d2a47..901457da25ec 100644 --- a/sound/soc/tegra/tegra_sgtl5000.c +++ b/sound/soc/tegra/tegra_sgtl5000.c @@ -149,14 +149,14 @@ static int tegra_sgtl5000_driver_probe(struct platform_device *pdev) dev_err(&pdev->dev, "Property 'nvidia,i2s-controller' missing/invalid\n"); ret = -EINVAL; - goto err; + goto err_put_codec_of_node; }
tegra_sgtl5000_dai.platform_of_node = tegra_sgtl5000_dai.cpu_of_node;
ret = tegra_asoc_utils_init(&machine->util_data, &pdev->dev); if (ret) - goto err; + goto err_put_cpu_of_node;
ret = snd_soc_register_card(card); if (ret) { @@ -169,6 +169,13 @@ static int tegra_sgtl5000_driver_probe(struct platform_device *pdev)
err_fini_utils: tegra_asoc_utils_fini(&machine->util_data); +err_put_cpu_of_node: + of_node_put(tegra_sgtl5000_dai.cpu_of_node); + tegra_sgtl5000_dai.cpu_of_node = NULL; + tegra_sgtl5000_dai.platform_of_node = NULL; +err_put_codec_of_node: + of_node_put(tegra_sgtl5000_dai.codec_of_node); + tegra_sgtl5000_dai.codec_of_node = NULL; err: return ret; } @@ -183,6 +190,12 @@ static int tegra_sgtl5000_driver_remove(struct platform_device *pdev)
tegra_asoc_utils_fini(&machine->util_data);
+ of_node_put(tegra_sgtl5000_dai.cpu_of_node); + tegra_sgtl5000_dai.cpu_of_node = NULL; + tegra_sgtl5000_dai.platform_of_node = NULL; + of_node_put(tegra_sgtl5000_dai.codec_of_node); + tegra_sgtl5000_dai.codec_of_node = NULL; + return ret; }
On 16/10/2018 11:47, Marcel Ziswiler wrote:
From: Marcel Ziswiler marcel.ziswiler@toradex.com
Similar to the following:
commit 4321723648b0 ("ASoC: tegra_alc5632: fix device_node refcounting")
commit 7c5dfd549617 ("ASoC: tegra: fix device_node refcounting")
Signed-off-by: Marcel Ziswiler marcel.ziswiler@toradex.com
Changes in v2: New patch
sound/soc/tegra/tegra_sgtl5000.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/sound/soc/tegra/tegra_sgtl5000.c b/sound/soc/tegra/tegra_sgtl5000.c index 45a4aa9d2a47..901457da25ec 100644 --- a/sound/soc/tegra/tegra_sgtl5000.c +++ b/sound/soc/tegra/tegra_sgtl5000.c @@ -149,14 +149,14 @@ static int tegra_sgtl5000_driver_probe(struct platform_device *pdev) dev_err(&pdev->dev, "Property 'nvidia,i2s-controller' missing/invalid\n"); ret = -EINVAL;
goto err;
goto err_put_codec_of_node;}
tegra_sgtl5000_dai.platform_of_node = tegra_sgtl5000_dai.cpu_of_node;
ret = tegra_asoc_utils_init(&machine->util_data, &pdev->dev); if (ret)
goto err;
goto err_put_cpu_of_node;ret = snd_soc_register_card(card); if (ret) {
@@ -169,6 +169,13 @@ static int tegra_sgtl5000_driver_probe(struct platform_device *pdev)
err_fini_utils: tegra_asoc_utils_fini(&machine->util_data); +err_put_cpu_of_node:
- of_node_put(tegra_sgtl5000_dai.cpu_of_node);
- tegra_sgtl5000_dai.cpu_of_node = NULL;
- tegra_sgtl5000_dai.platform_of_node = NULL;
+err_put_codec_of_node:
- of_node_put(tegra_sgtl5000_dai.codec_of_node);
- tegra_sgtl5000_dai.codec_of_node = NULL;
err: return ret; } @@ -183,6 +190,12 @@ static int tegra_sgtl5000_driver_remove(struct platform_device *pdev)
tegra_asoc_utils_fini(&machine->util_data);
- of_node_put(tegra_sgtl5000_dai.cpu_of_node);
- tegra_sgtl5000_dai.cpu_of_node = NULL;
- tegra_sgtl5000_dai.platform_of_node = NULL;
- of_node_put(tegra_sgtl5000_dai.codec_of_node);
- tegra_sgtl5000_dai.codec_of_node = NULL;
- return ret;
}
Acked-by: Jon Hunter jonathanh@nvidia.com
Cheers Jon
From: Marcel Ziswiler marcel.ziswiler@toradex.com
This fixes the following error as seen post commit daecf46ee0e5 ("ASoC: soc-core: use snd_soc_dai_link_component for platform"):
tegra-snd-sgtl5000 sound: ASoC: Both platform name/of_node are set for sgtl5000 tegra-snd-sgtl5000 sound: ASoC: failed to init link sgtl5000 tegra-snd-sgtl5000 sound: snd_soc_register_card failed (-22) tegra-snd-sgtl5000: probe of sound failed with error -22
Signed-off-by: Marcel Ziswiler marcel.ziswiler@toradex.com
---
Changes in v2: New patch
sound/soc/tegra/tegra_sgtl5000.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/sound/soc/tegra/tegra_sgtl5000.c b/sound/soc/tegra/tegra_sgtl5000.c index 901457da25ec..eb702925cac3 100644 --- a/sound/soc/tegra/tegra_sgtl5000.c +++ b/sound/soc/tegra/tegra_sgtl5000.c @@ -168,6 +168,11 @@ static int tegra_sgtl5000_driver_probe(struct platform_device *pdev) return 0;
err_fini_utils: + if (tegra_sgtl5000_dai.platform) { + devm_kfree(&pdev->dev, tegra_sgtl5000_dai.platform); + tegra_sgtl5000_dai.platform = NULL; + } + tegra_asoc_utils_fini(&machine->util_data); err_put_cpu_of_node: of_node_put(tegra_sgtl5000_dai.cpu_of_node);
On 16/10/2018 11:47, Marcel Ziswiler wrote:
From: Marcel Ziswiler marcel.ziswiler@toradex.com
This fixes the following error as seen post commit daecf46ee0e5 ("ASoC: soc-core: use snd_soc_dai_link_component for platform"):
tegra-snd-sgtl5000 sound: ASoC: Both platform name/of_node are set for sgtl5000 tegra-snd-sgtl5000 sound: ASoC: failed to init link sgtl5000 tegra-snd-sgtl5000 sound: snd_soc_register_card failed (-22) tegra-snd-sgtl5000: probe of sound failed with error -22
Signed-off-by: Marcel Ziswiler marcel.ziswiler@toradex.com
Changes in v2: New patch
sound/soc/tegra/tegra_sgtl5000.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/sound/soc/tegra/tegra_sgtl5000.c b/sound/soc/tegra/tegra_sgtl5000.c index 901457da25ec..eb702925cac3 100644 --- a/sound/soc/tegra/tegra_sgtl5000.c +++ b/sound/soc/tegra/tegra_sgtl5000.c @@ -168,6 +168,11 @@ static int tegra_sgtl5000_driver_probe(struct platform_device *pdev) return 0;
err_fini_utils:
- if (tegra_sgtl5000_dai.platform) {
devm_kfree(&pdev->dev, tegra_sgtl5000_dai.platform);tegra_sgtl5000_dai.platform = NULL;- }
- tegra_asoc_utils_fini(&machine->util_data);
err_put_cpu_of_node: of_node_put(tegra_sgtl5000_dai.cpu_of_node);
Where is the above allocated? I don't see it allocated in this driver AFAICT. If it is not then it does not seem right to free something that we have not allocated in this driver. I would have assumed it was allocated by snd_soc_init_platform() in which case it should not be necessary to free because that function uses devm_kzalloc(). What am I missing here?
Cheers Jon
On Wed, 2018-10-17 at 13:32 +0100, Jon Hunter wrote:
On 16/10/2018 11:47, Marcel Ziswiler wrote:
From: Marcel Ziswiler marcel.ziswiler@toradex.com
This fixes the following error as seen post commit daecf46ee0e5 ("ASoC: soc-core: use snd_soc_dai_link_component for platform"):
tegra-snd-sgtl5000 sound: ASoC: Both platform name/of_node are set for sgtl5000 tegra-snd-sgtl5000 sound: ASoC: failed to init link sgtl5000 tegra-snd-sgtl5000 sound: snd_soc_register_card failed (-22) tegra-snd-sgtl5000: probe of sound failed with error -22
Signed-off-by: Marcel Ziswiler marcel.ziswiler@toradex.com
Changes in v2: New patch
sound/soc/tegra/tegra_sgtl5000.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/sound/soc/tegra/tegra_sgtl5000.c b/sound/soc/tegra/tegra_sgtl5000.c index 901457da25ec..eb702925cac3 100644 --- a/sound/soc/tegra/tegra_sgtl5000.c +++ b/sound/soc/tegra/tegra_sgtl5000.c @@ -168,6 +168,11 @@ static int tegra_sgtl5000_driver_probe(struct platform_device *pdev) return 0;
err_fini_utils:
- if (tegra_sgtl5000_dai.platform) {
devm_kfree(&pdev->dev,tegra_sgtl5000_dai.platform);
tegra_sgtl5000_dai.platform = NULL;- }
- tegra_asoc_utils_fini(&machine->util_data);
err_put_cpu_of_node: of_node_put(tegra_sgtl5000_dai.cpu_of_node);
Where is the above allocated?
snd_soc_init_platform() in sound/soc/soc-core.c
I don't see it allocated in this driver AFAICT. If it is not then it does not seem right to free something that we have not allocated in this driver. I would have assumed it was allocated by snd_soc_init_platform() in which case it should not be necessary to free because that function uses devm_kzalloc().
That is kind of what I assumed as well.
What am I missing here?
That is actually a very very good question. Unfortunately, since above mentioned commit which is part of the bigger multi-platform (or whatever one may call it) rework done by folks on CC things start falling apart.
I should maybe rather have phrased this one as an RFC.
Some facts from my humble investigation so far:
- The issue does not exhibit itself on Apalis/Colibri T30 where probably the order of things being initialised is slightly different.
- Bisecting points to the above mentioned commit being to blame. However there is no way to just revert that commit as it is part of the bigger multi-platform rework.
- Somehow it has to do with probe deferral. Basically, platform gets allocated in snd_soc_init_platform() but due to GPIO/I2C whatever not being ready the SGTL5000 codec aka dai_link can not yet be found and therefore it probe defers as follows:
[ 2.166517] tegra30-i2s 70301200.i2s: DMA channels sourced from device 70300000.ahub [ 2.176043] tegra-snd-sgtl5000 sound: ASoC: CODEC DAI sgtl5000 not registered [ 2.183241] tegra-snd-sgtl5000 sound: snd_soc_register_card failed (-517)
- Somewhere thereafter platform seems to get stumped onto (e.g. its name rather than being null now is bogus. Unfortunately, it is not just the name as clearing just that did not really help (sgtl5000 codec gets instantiated but trying to play audio always returned -EINVAL).
- The second time around snd_soc_init_platform() re-uses previously allocated platform now corrupt triggering a check in soc-core concerning platform name and of_node both being set (as noted in above commit message).
Some questions:
- How exactly are devm allocations supposed to work concerning probe deferrals?
- Does or should the platform get cleared during a probe deferral cycle?
- If so, why does that not work?
- Or is some special implicit probe deferral handling missing in soc- core?
I'm happy to try more things and/or provide more debugging output if needed. Just let me know.
Thanks Jon!
Cheers Jon
Cheers
Marcel
On Wed, Oct 17, 2018 at 02:28:22PM +0000, Marcel Ziswiler wrote:
Some questions:
- How exactly are devm allocations supposed to work concerning probe
deferrals?
Probe deferrals are just normal probe errors, any devm_ allocated stuff gets unwound.
- Does or should the platform get cleared during a probe deferral
cycle?
- If so, why does that not work?
Is something writing to static data when it should be writing to dynamically allocated data? That's what this sounds like, we shouldn't be modifying any static data and any data dynamically allocated during probe ought to be being discarded.
- Or is some special implicit probe deferral handling missing in soc-
core?
Like I say a probe deferral is just a normal error.
On 17/10/2018 20:16, Mark Brown wrote:
On Wed, Oct 17, 2018 at 02:28:22PM +0000, Marcel Ziswiler wrote:
Some questions:
- How exactly are devm allocations supposed to work concerning probe
deferrals?
Probe deferrals are just normal probe errors, any devm_ allocated stuff gets unwound.
- Does or should the platform get cleared during a probe deferral
cycle?
- If so, why does that not work?
Is something writing to static data when it should be writing to dynamically allocated data? That's what this sounds like, we shouldn't be modifying any static data and any data dynamically allocated during probe ought to be being discarded.
Marcel, it maybe worth looking at what happens in the 2nd call to snd_soc_init_platform() following the probe deferral. Also pay attention to the value of 'dai_link->platform_name' in snd_soc_init_platform() on both 1st and 2nd call. Seems that on the 2nd time the function is called the value is not NULL???
Cheers Jon
On Wed, 2018-10-17 at 20:16 +0100, Mark Brown wrote:
On Wed, Oct 17, 2018 at 02:28:22PM +0000, Marcel Ziswiler wrote:
Some questions:
- How exactly are devm allocations supposed to work concerning
probe deferrals?
Probe deferrals are just normal probe errors, any devm_ allocated stuff gets unwound.
OK, that is where my understanding was weary. So you are saying anything should really get allocated again upon the second time.
- Does or should the platform get cleared during a probe deferral
cycle?
- If so, why does that not work?
Is something writing to static data when it should be writing to dynamically allocated data? That's what this sounds like, we shouldn't be modifying any static data and any data dynamically allocated during probe ought to be being discarded.
OK, I believe I start to see what you are saying. I guess the bug lays in soc-core then not properly discarding the platform and the second time around it is using a stale pointer to that which now of course points to whatever happens to be there! Let me try a few more things and I will cook up a proper patch to fix that...
- Or is some special implicit probe deferral handling missing in
soc- core?
Like I say a probe deferral is just a normal error.
Yeah, right. Thanks!
On Tue, Oct 16, 2018 at 12:47:21PM +0200, Marcel Ziswiler wrote:
Marcel Ziswiler (9): ARM: tegra: fix simple-panel compatibles ARM: tegra: apalis-tk1/colibri_t20/t30: eval/iris: fix regulator gpio enable ARM: tegra: colibri_t20: reorder pmic properties ARM: tegra: apalis-tk1: further regulator clean-up ARM: tegra: apalis_t30/tk1: annotate power I2C being on-module ARM: tegra: colibri_t30: further regulator clean-up ARM: tegra: apalis_t30: further regulator clean-up ASoC: tegra_sgtl5000: fix device_node refcounting ASoC: tegra_sgtl5000: fix platform name vs. of_node assignement
These ASoC patches appear to have absolutely no relationship with the rest of the series. Is that the case or is there some dependency I'm missing?
On Tue, 2018-10-16 at 12:58 +0100, Mark Brown wrote:
On Tue, Oct 16, 2018 at 12:47:21PM +0200, Marcel Ziswiler wrote:
Marcel Ziswiler (9): ARM: tegra: fix simple-panel compatibles ARM: tegra: apalis-tk1/colibri_t20/t30: eval/iris: fix regulator gpio enable ARM: tegra: colibri_t20: reorder pmic properties ARM: tegra: apalis-tk1: further regulator clean-up ARM: tegra: apalis_t30/tk1: annotate power I2C being on-module ARM: tegra: colibri_t30: further regulator clean-up ARM: tegra: apalis_t30: further regulator clean-up ASoC: tegra_sgtl5000: fix device_node refcounting ASoC: tegra_sgtl5000: fix platform name vs. of_node assignement
These ASoC patches appear to have absolutely no relationship with the rest of the series. Is that the case or is there some dependency I'm missing?
Not outside of all being Tegra specific stuff, no. If you prefer I may send them as a separate series.
On Wed, Oct 17, 2018 at 10:51:30AM +0000, Marcel Ziswiler wrote:
On Tue, 2018-10-16 at 12:58 +0100, Mark Brown wrote:
These ASoC patches appear to have absolutely no relationship with the rest of the series. Is that the case or is there some dependency I'm missing?
Not outside of all being Tegra specific stuff, no. If you prefer I may send them as a separate series.
It's fine this time but do try to avoid doing that please - if things aren't related just send them separately. Bundling them up into a series creates confusion about dependencis and can result in things getting held up due to problems in the unrelated bits of the series.
participants (4)
-
Jon Hunter -
Marcel Ziswiler -
Marcel Ziswiler -
Mark Brown