[alsa-devel] [PATCH] ASoC: imx-spdif: Use module_init() to handle platform_device_register()
This is a quick fix for the below two issues when building spdif as modules.
1) When modprobing modules in order: snd-soc-fsl-spdif -> snd-soc-imx-spdif -> snd-soc-spdif-tx/rx, we will fail to create imx-spdif card and dai link unless we rmmod snd-soc-imx-spdif and modprobe it again due to inappropriate condition of platform_driver_unregister() in probe().
2) After "imx-spdif sound-spdif.17: dit-hifi <-> 2004000.spdif mapping ok", doing rmmod the imx-spdif would cause kernel Oops: ------------[ cut here ]------------ WARNING: CPU: 0 PID: 1301 at /home/rmk/git/linux-rmk/fs/sysfs/dir.c:915 sysfs_hash_and_remove+0x84/0x90() sysfs: can not remove 'dapm_widget', no directory Modules linked in: snd_soc_imx_spdif(-) snd_soc_fsl_spdif imx_pcm_dma fuse bnep rfcomm bluetooth imx_sdma imx_thermal imx2_wdt snd_soc_spdif_tx hid_cypress [last unloaded: imx_pcm_dma] CPU: 0 PID: 1301 Comm: rmmod Not tainted 3.13.0-rc4+ #387 Backtrace: [<c0012640>] (dump_backtrace) from [<c00127dc>] (show_stack+0x18/0x1c) r6:00000393 r5:c016bfa4 r4:00000000 r3:00000000 [<c00127c4>] (show_stack) from [<c0683a18>] (dump_stack+0x70/0x90) [<c06839a8>] (dump_stack) from [<c00244e0>] (warn_slowpath_common+0x74/0x94) r4:dab89d30 r3:00000000 [<c002446c>] (warn_slowpath_common) from [<c00245a4>] (warn_slowpath_fmt+0x38/0x40) r8:c689ce00 r7:00000000 r6:00000000 r5:c077c04c r4:dba2a318 [<c0024570>] (warn_slowpath_fmt) from [<c016bfa4>] (sysfs_hash_and_remove+0x84/0x90) r3:c077c04c r2:c06b3c38 [<c016bf20>] (sysfs_hash_and_remove) from [<c016a00c>] (sysfs_remove_file_ns+0x18/0x1c) r7:da916900 r6:00000000 r5:db2c900c r4:dba2a318 [<c0169ff4>] (sysfs_remove_file_ns) from [<c03687d4>] (device_remove_file+0x20/0x24) [<c03687b4>] (device_remove_file) from [<c04f532c>] (snd_soc_dapm_free+0x1c/0x22c) [<c04f5310>] (snd_soc_dapm_free) from [<c04f0c98>] (soc_remove_codec+0x34/0x98) r10:dabae59c r9:00000000 r8:c689ce00 r7:da916900 r6:00000000 r5:db2c900c r4:dba2a200 r3:00000000 [<c04f0c64>] (soc_remove_codec) from [<c04f3188>] (soc_remove_dai_links.clone.28+0x3c0/0x3ec) r4:00000000 r3:00000000 [<c04f2dc8>] (soc_remove_dai_links.clone.28) from [<c04f3248>] (snd_soc_unregister_card+0x94/0xc8) r10:d8435000 r9:db924204 r8:d8435000 r7:d079d480 r6:00000001 r5:00000608 r4:dabae4b8 [<c04f31b4>] (snd_soc_unregister_card) from [<c04ffa70>] (devm_card_release+0x14/0x18) r6:00000003 r5:db924010 r4:dab89e60 r3:c04ffa5c [<c04ffa5c>] (devm_card_release) from [<c036ec74>] (release_nodes+0x190/0x1f8) [<c036eae4>] (release_nodes) from [<c036ed94>] (devres_release_all+0x38/0x54) r10:00000000 r9:dab88000 r8:c000eb44 r7:bef87600 r6:db924044 r5:bf131014 r4:db924010 [<c036ed5c>] (devres_release_all) from [<c036b7a8>] (__device_release_driver+0x80/0xd4) r4:db924010 r3:bf12f000 [<c036b728>] (__device_release_driver) from [<c036bf28>] (driver_detach+0xbc/0xc0) r5:bf131014 r4:db924010 [<c036be6c>] (driver_detach) from [<c036b490>] (bus_remove_driver+0x54/0x98) r6:dab89f3c r5:bf131058 r4:bf131014 r3:db0c8000 [<c036b43c>] (bus_remove_driver) from [<c036c588>] (driver_unregister+0x30/0x4c) r4:bf131014 r3:c68a7a80 [<c036c558>] (driver_unregister) from [<c036d21c>] (platform_driver_unregister+0x14/0x18) r4:00000000 r3:bf12f2d0 [<c036d208>] (platform_driver_unregister) from [<bf12f2e4>] (imx_spdif_driver_exit+0x14/0xd30 [snd_soc_imx_spdif]) [<bf12f2d0>] (imx_spdif_driver_exit [snd_soc_imx_spdif]) from [<c008d764>] (SyS_delete_module+0x140/0x190) [<c008d624>] (SyS_delete_module) from [<c000e980>] (ret_fast_syscall+0x0/0x48) r7:00000081 r6:000120a8 r5:bef87600 r4:00000880 ---[ end trace 09423e64ab60df46 ]--- Unable to handle kernel NULL pointer dereference at virtual address 00000008 pgd = dab9c000 [00000008] *pgd=2062f831, *pte=00000000, *ppte=00000000 Internal error: Oops: 17 [#1] SMP ARM Modules linked in: snd_soc_imx_spdif(-) snd_soc_fsl_spdif imx_pcm_dma fuse bnep rfcomm bluetooth imx_sdma imx_thermal imx2_wdt snd_soc_spdif_tx hid_cypress [last unloaded: imx_pcm_dma] CPU: 0 PID: 1301 Comm: rmmod Tainted: G W 3.13.0-rc4+ #387 task: db0c8000 ti: dab88000 task.ti: dab88000 PC is at soc_remove_codec+0x78/0x98 LR is at trace_hardirqs_on+0x14/0x18 pc : [<c04f0cdc>] lr : [<c0066514>] psr: 600f0013 sp : dab89dc8 ip : 00000020 fp : dab89ddc r10: dabae59c r9 : 00000000 r8 : c689ce00 r7 : da916900 r6 : 00000000 r5 : db2c900c r4 : dba2a200 r3 : 00000000 r2 : 00100100 r1 : dabac810 r0 : dabae5f8 Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user Control: 10c53c7d Table: 2ab9c059 DAC: 00000015 Process rmmod (pid: 1301, stack limit = 0xdab88240) Stack: (0xdab89dc8 to 0xdab8a000) 9dc0: 00000000 00000000 dab89e24 dab89de0 c04f3188 c04f0c70 9de0: d84355bc dabae5a0 00200200 00100100 00200200 00100100 d079d480 dabae4b8 9e00: 00000608 00000001 d079d480 d8435000 db924204 d8435000 dab89e44 dab89e28 9e20: c04f3248 c04f2dd4 c04ffa5c dab89e60 db924010 00000003 dab89e54 dab89e48 9e40: c04ffa70 c04f31c0 dab89e94 dab89e58 c036ec74 c04ffa68 dab89e94 a00f0013 9e60: dabae400 d079d480 00000002 db924010 bf131014 db924044 bef87600 c000eb44 9e80: dab88000 00000000 dab89eac dab89e98 c036ed94 c036eaf0 bf12f000 db924010 9ea0: dab89ec4 dab89eb0 c036b7a8 c036ed68 db924010 bf131014 dab89ee4 dab89ec8 9ec0: c036bf28 c036b734 db0c8000 bf131014 bf131058 dab89f3c dab89efc dab89ee8 9ee0: c036b490 c036be78 c68a7a80 bf131014 dab89f14 dab89f00 c036c588 c036b448 9f00: bf12f2d0 00000000 dab89f24 dab89f18 c036d21c c036c564 dab89f34 dab89f28 9f20: bf12f2e4 c036d214 dab89fa4 dab89f38 c008d764 bf12f2dc 00000000 5f646e73 9f40: 5f636f73 5f786d69 69647073 bef80066 dab89f74 dab89f60 c005f0d4 c0069020 9f60: 0077a008 00000000 00000880 bef87600 000120a8 00000081 bf131058 00000880 9f80: dab89f84 00000000 00000880 bef87600 000120a8 00000081 00000000 dab89fa8 9fa0: c000e980 c008d630 00000880 bef87600 bef87600 00000880 00009778 bef875f4 9fc0: 00000880 bef87600 000120a8 00000081 00000001 000120bc 00000001 00000000 9fe0: b6f03840 bef875fc 00008f75 b6f0384c 800f0010 bef87600 00000000 00000000 Backtrace: [<c04f0c64>] (soc_remove_codec) from [<c04f3188>] (soc_remove_dai_links.clone.28+0x3c0/0x3ec) r4:00000000 r3:00000000 [<c04f2dc8>] (soc_remove_dai_links.clone.28) from [<c04f3248>] (snd_soc_unregister_card+0x94/0xc8) r10:d8435000 r9:db924204 r8:d8435000 r7:d079d480 r6:00000001 r5:00000608 r4:dabae4b8 [<c04f31b4>] (snd_soc_unregister_card) from [<c04ffa70>] (devm_card_release+0x14/0x18) r6:00000003 r5:db924010 r4:dab89e60 r3:c04ffa5c [<c04ffa5c>] (devm_card_release) from [<c036ec74>] (release_nodes+0x190/0x1f8) [<c036eae4>] (release_nodes) from [<c036ed94>] (devres_release_all+0x38/0x54) r10:00000000 r9:dab88000 r8:c000eb44 r7:bef87600 r6:db924044 r5:bf131014 r4:db924010 [<c036ed5c>] (devres_release_all) from [<c036b7a8>] (__device_release_driver+0x80/0xd4) r4:db924010 r3:bf12f000 [<c036b728>] (__device_release_driver) from [<c036bf28>] (driver_detach+0xbc/0xc0) r5:bf131014 r4:db924010 [<c036be6c>] (driver_detach) from [<c036b490>] (bus_remove_driver+0x54/0x98) r6:dab89f3c r5:bf131058 r4:bf131014 r3:db0c8000 [<c036b43c>] (bus_remove_driver) from [<c036c588>] (driver_unregister+0x30/0x4c) r4:bf131014 r3:c68a7a80 [<c036c558>] (driver_unregister) from [<c036d21c>] (platform_driver_unregister+0x14/0x18) r4:00000000 r3:bf12f2d0 [<c036d208>] (platform_driver_unregister) from [<bf12f2e4>] (imx_spdif_driver_exit+0x14/0xd30 [snd_soc_imx_spdif]) [<bf12f2d0>] (imx_spdif_driver_exit [snd_soc_imx_spdif]) from [<c008d764>] (SyS_delete_module+0x140/0x190) [<c008d624>] (SyS_delete_module) from [<c000e980>] (ret_fast_syscall+0x0/0x48) r7:00000081 r6:000120a8 r5:bef87600 r4:00000880 Code: e594100c e5842068 e584306c e5913080 (e5930008) ---[ end trace 09423e64ab60df47 ]---
Ideally, we should bypass the device_unregister() if getting EPROBE_DEFER, and find the root cause of "can not remove 'dapm_widget', no directory". But the root cause seems to be beyond the imx-spdif driver level, which might be related to the disordered resource releasing of the whole link.
Thus this patch can provide a quick fix, if acceptable, to these two bugs so that the driver can finely work when being built as modules. And it's also safe enough to avoid duplicate regisitering/unregistering since we don't handle them in the probe() and remove() any more. At the meantime, we can trace the root cause of the Oops without pressure.
Test report:
root@freescale ~$ modprobe snd-soc-spdif-rx imx-spdif sound-spdif.10: dir-hifi <-> 2004000.spdif mapping ok
root@freescale ~$ rmmod snd-soc-imx-spdif root@freescale ~$ modprobe snd-soc-imx-spdif imx-spdif sound-spdif.10: dir-hifi <-> 2004000.spdif mapping ok
root@freescale ~$ rmmod snd-soc-imx-spdif root@freescale ~$ rmmod snd-soc-fsl-spdif root@freescale ~$ modprobe snd-soc-imx-spdif imx-spdif sound-spdif.10: ASoC: CPU DAI (null) not registered imx-spdif sound-spdif.10: snd_soc_register_card failed: -517 platform sound-spdif.10: Driver imx-spdif requests probe deferral root@freescale ~$ modprobe snd-soc-fsl-spdif imx-spdif sound-spdif.10: dir-hifi <-> 2004000.spdif mapping ok
root@freescale ~$ rmmod snd-soc-imx-spdif root@freescale ~$ rmmod snd-soc-spdif-rx root@freescale ~$ modprobe snd-soc-imx-spdif imx-spdif sound-spdif.10: ASoC: CODEC spdif-dir not registered imx-spdif sound-spdif.10: snd_soc_register_card failed: -517 platform sound-spdif.10: Driver imx-spdif requests probe deferral root@freescale ~$ modprobe snd-soc-spdif-rx imx-spdif sound-spdif.10: dir-hifi <-> 2004000.spdif mapping ok
Reported-by: Russell King linux@arm.linux.org.uk Signed-off-by: Nicolin Chen Guangyu.Chen@freescale.com --- sound/soc/fsl/imx-spdif.c | 84 ++++++++++++++++++++++++----------------------- 1 file changed, 43 insertions(+), 41 deletions(-)
diff --git a/sound/soc/fsl/imx-spdif.c b/sound/soc/fsl/imx-spdif.c index 980dd1f..c4b9894 100644 --- a/sound/soc/fsl/imx-spdif.c +++ b/sound/soc/fsl/imx-spdif.c @@ -16,8 +16,6 @@ struct imx_spdif_data { struct snd_soc_dai_link dai[2]; struct snd_soc_card card; - struct platform_device *txdev; - struct platform_device *rxdev; };
static int imx_spdif_audio_probe(struct platform_device *pdev) @@ -47,13 +45,6 @@ static int imx_spdif_audio_probe(struct platform_device *pdev) data->dai[num_links].cpu_of_node = spdif_np; data->dai[num_links].platform_of_node = spdif_np; num_links++; - - data->txdev = platform_device_register_simple("spdif-dit", -1, NULL, 0); - if (IS_ERR(data->txdev)) { - ret = PTR_ERR(data->txdev); - dev_err(&pdev->dev, "register dit failed: %d\n", ret); - goto end; - } }
if (of_property_read_bool(np, "spdif-in")) { @@ -64,18 +55,11 @@ static int imx_spdif_audio_probe(struct platform_device *pdev) data->dai[num_links].cpu_of_node = spdif_np; data->dai[num_links].platform_of_node = spdif_np; num_links++; - - data->rxdev = platform_device_register_simple("spdif-dir", -1, NULL, 0); - if (IS_ERR(data->rxdev)) { - ret = PTR_ERR(data->rxdev); - dev_err(&pdev->dev, "register dir failed: %d\n", ret); - goto error_dit; - } }
if (!num_links) { dev_err(&pdev->dev, "no enabled S/PDIF DAI link\n"); - goto error_dir; + goto end; }
data->card.dev = &pdev->dev; @@ -84,24 +68,16 @@ static int imx_spdif_audio_probe(struct platform_device *pdev)
ret = snd_soc_of_parse_card_name(&data->card, "model"); if (ret) - goto error_dir; + goto end;
ret = devm_snd_soc_register_card(&pdev->dev, &data->card); if (ret) { dev_err(&pdev->dev, "snd_soc_register_card failed: %d\n", ret); - goto error_dir; + goto end; }
platform_set_drvdata(pdev, data);
- goto end; - -error_dir: - if (data->rxdev) - platform_device_unregister(data->rxdev); -error_dit: - if (data->txdev) - platform_device_unregister(data->txdev); end: if (spdif_np) of_node_put(spdif_np); @@ -109,18 +85,6 @@ end: return ret; }
-static int imx_spdif_audio_remove(struct platform_device *pdev) -{ - struct imx_spdif_data *data = platform_get_drvdata(pdev); - - if (data->rxdev) - platform_device_unregister(data->rxdev); - if (data->txdev) - platform_device_unregister(data->txdev); - - return 0; -} - static const struct of_device_id imx_spdif_dt_ids[] = { { .compatible = "fsl,imx-audio-spdif", }, { /* sentinel */ } @@ -134,10 +98,48 @@ static struct platform_driver imx_spdif_driver = { .of_match_table = imx_spdif_dt_ids, }, .probe = imx_spdif_audio_probe, - .remove = imx_spdif_audio_remove, };
-module_platform_driver(imx_spdif_driver); +static struct platform_device *txdev; +static struct platform_device *rxdev; + +static int __init imx_spdif_init(void) +{ + int ret; + + txdev = platform_device_register_simple("spdif-dit", -1, NULL, 0); + if (IS_ERR(txdev)) + return PTR_ERR(txdev); + + rxdev = platform_device_register_simple("spdif-dir", -1, NULL, 0); + if (IS_ERR(rxdev)) { + ret = PTR_ERR(rxdev); + goto err; + } + + ret = platform_driver_register(&imx_spdif_driver); + if (ret) + goto err1; + + return 0; + +err1: + platform_device_unregister(rxdev); +err: + platform_device_unregister(txdev); + + return ret; +} +module_init(imx_spdif_init); + +static void __exit imx_spdif_exit(void) +{ + platform_driver_unregister(&imx_spdif_driver); + + platform_device_unregister(rxdev); + platform_device_unregister(txdev); +} +module_exit(imx_spdif_exit);
MODULE_AUTHOR("Freescale Semiconductor, Inc."); MODULE_DESCRIPTION("Freescale i.MX S/PDIF machine driver");
On Mon, Dec 16, 2013 at 06:36:09PM +0800, Nicolin Chen wrote:
- When modprobing modules in order: snd-soc-fsl-spdif -> snd-soc-imx-spdif
-> snd-soc-spdif-tx/rx, we will fail to create imx-spdif card and dai link unless we rmmod snd-soc-imx-spdif and modprobe it again due to inappropriate condition of platform_driver_unregister() in probe().
The crucial bit of information here is what the "inappropriate condition" is - what is this trying to fix? The code doesn't look obviously wrong though it does rely on the platform device registration taking effect immediately to actually probe which is a bit of an assumption and is probably the thing that messes up the first time around as the S/PDIF CODEC drivers probably aren't loaded when the card is trying to probe. This probably causes us to hit a case where deferred probe stalls as it's not making any progress as one of the deferred devices essentially has a dependency on itself.
What should work right now is for the module to ensure that the S/PDIF CODEC drivers are loaded before it is by linking to some symbol from there. This is a total hack though. Nicer would be for the machine driver to either directly register S/PDIF DAIs (rather than devices that then register the DAIs) or to create a card subdevice in parallel with the S/PDIF ones and hook the card registration off that.
- After "imx-spdif sound-spdif.17: dit-hifi <-> 2004000.spdif mapping ok",
doing rmmod the imx-spdif would cause kernel Oops: ------------[ cut here ]------------ WARNING: CPU: 0 PID: 1301 at /home/rmk/git/linux-rmk/fs/sysfs/dir.c:915 sysfs_hash_and_remove+0x84/0x90()
Please don't paste entire backtraces into commit messages, they're hard to read and very long - this is over 100 lines long and doesn't have anything like that much information. Either edit them to pull out the important details or (better yet) explain in words what's going wrong.
I *suspect* this is triggered by removing the CODECs prior to removing the card which should work but doesn't entirely (we lock modules in to prevent it triggering normally unless you go and really try by using unregistration). Reverting back to normal card registration and doing that prior to unregistering the CODEC devices ought to fix the unload case at least.
Ideally, we should bypass the device_unregister() if getting EPROBE_DEFER,
This won't work, it'll cause the next probe to fail as the driver tries to reregister an already registered device. I'm also not sure why you'd want to do that...
+static int __init imx_spdif_init(void) +{
- int ret;
- txdev = platform_device_register_simple("spdif-dit", -1, NULL, 0);
- if (IS_ERR(txdev))
return PTR_ERR(txdev);- rxdev = platform_device_register_simple("spdif-dir", -1, NULL, 0);
- if (IS_ERR(rxdev)) {
ret = PTR_ERR(rxdev);goto err;- }
This is going to cause these devices to be loaded on unrelated systems if they have the module compiled in or it gets loaded for some other reason - we shouldn't do that, it'll cause errors in their registration paths which may stop them working at all.
On Mon, Dec 16, 2013 at 07:47:17PM +0000, Mark Brown wrote:
On Mon, Dec 16, 2013 at 06:36:09PM +0800, Nicolin Chen wrote:
- When modprobing modules in order: snd-soc-fsl-spdif -> snd-soc-imx-spdif
-> snd-soc-spdif-tx/rx, we will fail to create imx-spdif card and dai link unless we rmmod snd-soc-imx-spdif and modprobe it again due to inappropriate condition of platform_driver_unregister() in probe().
The crucial bit of information here is what the "inappropriate condition" is - what is this trying to fix? The code doesn't look obviously wrong though it does rely on the platform device registration taking effect immediately to actually probe which is a bit of an
It's an exact copy-n-paste of the same problem which Kirkwood stuff had when I first looked at spdif there (presumably because Kirkwood had copied it from somewhere.) The basic problem is this:
- No spdif codec loaded - Card probes - Card creates a platform device - Creation of the platform device triggers a uevent - Card is attempted to be registered, but fails because no spdif codec - Card removes platform device - Card exits with -EPROBE_DEFER - SPDIF codec is loaded, no device to bind to - Since no device has been bound, no deferred probing is done.
This is a fundamental problem with any ASoC card driver which tries to self-declare platform devices - you can't do this with deferred probing. The struct device for the codec really really needs to not go away _even_ if the card exits with -EPROBE_DEFER.
What should work right now is for the module to ensure that the S/PDIF CODEC drivers are loaded before it is by linking to some symbol from there. This is a total hack though. Nicer would be for the machine driver to either directly register S/PDIF DAIs (rather than devices that then register the DAIs) or to create a card subdevice in parallel with the S/PDIF ones and hook the card registration off that.
That creates much more complexity though, and adds yet more possibility for unreliability into this. "Keep it simple" is well worth following.
The simple thing here is to declare the codec device in the module init, before registering the card driver, and cleaning it up at the appropriate moment. That way, the card platform driver gets registered and it can be probed, and all the time that the card platform driver is registered, the codec device is also present, ready to hook into the codec driver when it becomes available, and thus triggering the deferred probing of any unbound card.
On Mon, Dec 16, 2013 at 09:19:58PM +0000, Russell King - ARM Linux wrote:
On Mon, Dec 16, 2013 at 07:47:17PM +0000, Mark Brown wrote:
The crucial bit of information here is what the "inappropriate condition" is - what is this trying to fix? The code doesn't look obviously wrong though it does rely on the platform device registration taking effect immediately to actually probe which is a bit of an
It's an exact copy-n-paste of the same problem which Kirkwood stuff had when I first looked at spdif there (presumably because Kirkwood had copied it from somewhere.) The basic problem is this:
Indeed - that was a comment on the qualty of the changelog as much as anything.
What should work right now is for the module to ensure that the S/PDIF CODEC drivers are loaded before it is by linking to some symbol from there. This is a total hack though. Nicer would be for the machine driver to either directly register S/PDIF DAIs (rather than devices that then register the DAIs) or to create a card subdevice in parallel with the S/PDIF ones and hook the card registration off that.
That creates much more complexity though, and adds yet more possibility for unreliability into this. "Keep it simple" is well worth following.
I don't see registering the DAIs directly as adding complexity here; it's essentially what the current code is trying to do and doesn't change the normal device registration flow at all. I do agree that the card subdevice is more complex, I don't particularly like that idea myself.
The simple thing here is to declare the codec device in the module init, before registering the card driver, and cleaning it up at the appropriate moment. That way, the card platform driver gets registered and it can
It's certainly simple and it's definitely what I'd recommend for non-DT systems. For DT systems it'll cause issues if two drivers both try to register the same device on module load and the second one to get loaded checks for errors. Another way is just to do that in the core so we don't need to worry about individual drivers conflicting and convert all the drivers that do this to reference the core copies.
On Mon, Dec 16, 2013 at 10:17:10PM +0000, Mark Brown wrote:
On Mon, Dec 16, 2013 at 09:19:58PM +0000, Russell King - ARM Linux wrote:
On Mon, Dec 16, 2013 at 07:47:17PM +0000, Mark Brown wrote:
What should work right now is for the module to ensure that the S/PDIF CODEC drivers are loaded before it is by linking to some symbol from there. This is a total hack though. Nicer would be for the machine driver to either directly register S/PDIF DAIs (rather than devices that then register the DAIs) or to create a card subdevice in parallel with the S/PDIF ones and hook the card registration off that.
That creates much more complexity though, and adds yet more possibility for unreliability into this. "Keep it simple" is well worth following.
I don't see registering the DAIs directly as adding complexity here; it's essentially what the current code is trying to do and doesn't change the normal device registration flow at all. I do agree that the card subdevice is more complex, I don't particularly like that idea myself.
Thank you for the detailed advices. I'll first try to register the CODEC DAI directly.
The simple thing here is to declare the codec device in the module init, before registering the card driver, and cleaning it up at the appropriate moment. That way, the card platform driver gets registered and it can
It's certainly simple and it's definitely what I'd recommend for non-DT systems. For DT systems it'll cause issues if two drivers both try to register the same device on module load and the second one to get loaded checks for errors. Another way is just to do that in the core so we
And this might be another topic that we wouldn't have this problem if we could use DT nodes for the S/PDIF CODEC driver as well. Since we are now using DT systems, mixing up a non-DT way might have this kinda unexpected result, although I admit this should be the responsibility driver owners and here is mine. But I'm still wondering is there any possibility for us to find a balanced way to enroll the S/PDIF CODEC driver in DT?
Thank you, Nicolin Chen
On Mon, Dec 16, 2013 at 10:17:10PM +0000, Mark Brown wrote:
I don't see registering the DAIs directly as adding complexity here; it's essentially what the current code is trying to do and doesn't change the normal device registration flow at all. I do agree that the
Sir, I just tried to register the CODEC DAI in machine driver and it works perfectly for module-loading. But when unloading it.... [...] root@freescale ~$ modprobe snd-soc-imx-spdif imx-spdif sound-spdif.10: S/PDIF-RX <-> 2004000.spdif mapping ok root@freescale ~$ lsmod Module Size Used by snd_soc_imx_spdif 3207 1 snd_soc_fsl_spdif 9604 2 imx_pcm_dma 1140 1 snd_soc_fsl_spdif imx_sdma 10781 0 [permanent] root@freescale ~$ rmmod snd-soc-imx-spdif rmmod: can't unload 'snd_soc_imx_spdif': Resource temporarily unavailable [...] It looks like registering the CODEC DAI to the pdev->dev of machine driver wasn't a good idea since it would lock itself up.
Then I also tried to register it to the pdev->dev of CPU DAI driver -- fsl-spdif, which is also the platform driver. The result is that there's nothing wrong with module loading/unloading except one failure: [...] root@freescale ~$ rmmod snd-soc-imx-spdif root@freescale ~$ modprobe snd-soc-imx-spdif debugfs: creating file 'imx-spdif' debugfs: creating file 'dapm_pop_time' debugfs: creating file 'dapm' debugfs: creating file 'bias_level' debugfs: creating file '2004000.spdif' debugfs: creating file 'cache_sync' debugfs: creating file 'cache_only' debugfs: creating file 'codec_reg' debugfs: creating file 'dapm' debugfs: creating file 'bias_level' debugfs: creating file '2004000.spdif' fsl-spdif-dai 2004000.spdif: ASoC: Failed to create platform debugfs directory imx-spdif sound-spdif.10: S/PDIF-RX <-> 2004000.spdif mapping ok debugfs: creating file 'Capture' debugfs: creating file 'spdif-in' [...] It should be caused by the naming conflict since they are two exact same names.
Although I'll continue finding another way to register it tomorrow, I think it's better to make a simple report for this solution.
And if you have any idea, please guide me.
Thank you, Nicolin Chen
On Tue, Dec 17, 2013 at 06:31:05PM +0800, Nicolin Chen wrote:
On Mon, Dec 16, 2013 at 10:17:10PM +0000, Mark Brown wrote:
I don't see registering the DAIs directly as adding complexity here; it's essentially what the current code is trying to do and doesn't change the normal device registration flow at all. I do agree that the
root@freescale ~$ rmmod snd-soc-imx-spdif rmmod: can't unload 'snd_soc_imx_spdif': Resource temporarily unavailable [...] It looks like registering the CODEC DAI to the pdev->dev of machine driver wasn't a good idea since it would lock itself up.
OK, that's the module loading bodge kicking in. We could always remove that since it's not that effective anyway I suppose but it's going to make things a bit more error prone.
Although I'll continue finding another way to register it tomorrow, I think it's better to make a simple report for this solution.
And if you have any idea, please guide me.
There's still the options of either registering the dummy devices in the core (rather than in a specific module where two modules could conflict with each other) or registering a subdevice for the card that I mentioned the other day. Of those two registering in the core seems better - it's basically the same as your current idea but means that two card drivers that need this won't conflict with each other.
On Wed, Dec 18, 2013 at 01:04:15PM +0000, Mark Brown wrote:
Although I'll continue finding another way to register it tomorrow, I think it's better to make a simple report for this solution.
And if you have any idea, please guide me.
There's still the options of either registering the dummy devices in the core (rather than in a specific module where two modules could conflict with each other) or registering a subdevice for the card that I mentioned the other day. Of those two registering in the core seems better - it's basically the same as your current idea but means that two card drivers that need this won't conflict with each other.
I'd love to try these two even though it should be a bit tougher.
Thank you for the ideas. Nicolin Chen
participants (3)
-
Mark Brown -
Nicolin Chen -
Russell King - ARM Linux