[alsa-devel] [PATCH 0/1] ASoC: debugfs support improvement
Hello,
I have noticed that the support for multiple cards/codecs via the debugfs is kind of broken. The second codec, which tries to create the debugfs files would emit the following messages: ASoC: Failed to create codec register debugfs file Failed to create pop time debugfs file ...
The following patch fixes this by adding a directory per codec to host the files. The additional directory is named as codec->name, so it is easier to find the correct codec we are looking for.
--- Peter Ujfalusi (1): ASoC: add support for multiple cards/codecs in debugfs
include/sound/soc.h | 1 + sound/soc/soc-core.c | 21 ++++++++++++++------- 2 files changed, 15 insertions(+), 7 deletions(-)
In order to support multiple codecs on the same system in the debugfs the directory hierarchy need to be changed by adding directory per codec under the asoc direcorty:
debugfs/asoc/{codec1-name}/codec_reg /dapm_pop_time /dapm/{widgets} debugfs/asoc/{codec2-name}/codec_reg /dapm_pop_time /dapm/{widgets}
With the original implementation only the debugfs files are only created for the first codec, other codecs loaded later would fail to create the debugfs files (since they are already exist). Furthermore in this situation any of the codecs has been removed, would cause the debugfs entries to disappear, regardless if the codec, which created them are still loaded (the one which loaded first).
Signed-off-by: Peter Ujfalusi peter.ujfalusi@nokia.com --- include/sound/soc.h | 1 + sound/soc/soc-core.c | 21 ++++++++++++++------- 2 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h index 475cb7e..0b1f917 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -413,6 +413,7 @@ struct snd_soc_codec { unsigned int num_dai;
#ifdef CONFIG_DEBUG_FS + struct dentry *debugfs_codec_root; struct dentry *debugfs_reg; struct dentry *debugfs_pop_time; struct dentry *debugfs_dapm; diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index f5b356f..f8bb73e 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1254,21 +1254,30 @@ static const struct file_operations codec_reg_fops = {
static void soc_init_codec_debugfs(struct snd_soc_codec *codec) { + codec->debugfs_codec_root = debugfs_create_dir(codec->name, + debugfs_root); + if (!codec->debugfs_codec_root) { + printk(KERN_WARNING + "ASoC: Failed to create codec debugfs directory\n"); + return; + } + codec->debugfs_reg = debugfs_create_file("codec_reg", 0644, - debugfs_root, codec, - &codec_reg_fops); + codec->debugfs_codec_root, + codec, &codec_reg_fops); if (!codec->debugfs_reg) printk(KERN_WARNING "ASoC: Failed to create codec register debugfs file\n");
codec->debugfs_pop_time = debugfs_create_u32("dapm_pop_time", 0744, - debugfs_root, + codec->debugfs_codec_root, &codec->pop_time); if (!codec->debugfs_pop_time) printk(KERN_WARNING "Failed to create pop time debugfs file\n");
- codec->debugfs_dapm = debugfs_create_dir("dapm", debugfs_root); + codec->debugfs_dapm = debugfs_create_dir("dapm", + codec->debugfs_codec_root); if (!codec->debugfs_dapm) printk(KERN_WARNING "Failed to create DAPM debugfs directory\n"); @@ -1278,9 +1287,7 @@ static void soc_init_codec_debugfs(struct snd_soc_codec *codec)
static void soc_cleanup_codec_debugfs(struct snd_soc_codec *codec) { - debugfs_remove_recursive(codec->debugfs_dapm); - debugfs_remove(codec->debugfs_pop_time); - debugfs_remove(codec->debugfs_reg); + debugfs_remove_recursive(codec->debugfs_codec_root); }
#else
On Wed, Sep 30, 2009 at 01:24:19PM +0300, Peter Ujfalusi wrote:
With the original implementation only the debugfs files are only created for the first codec, other codecs loaded later would fail to create the debugfs files (since they are already exist). Furthermore in this situation any of the codecs has been removed, would cause the debugfs entries to disappear, regardless if the codec, which created them are still loaded (the one which loaded first).
Signed-off-by: Peter Ujfalusi peter.ujfalusi@nokia.com
There's way more problems than this with multiple CODECs and multiple cards - DAPM can't really cope at all, and the split between CODECs and cards isn't as clear as would be desirable. Looking into this is one of my big tasks for the rest of the year. With this particular problem I suspect the fix will fall out of the other reorganisations required to make the rest of the system work.
codec->debugfs_codec_root = debugfs_create_dir(codec->name,debugfs_root);
I don't think the CODEC name is what we want to use here, one of the common cases is going to be to have two or more mono DACs or speaker drivers in the system, normally of the same type and therefore with the same name set here. A dev_name() is going to be required as well, munged in with the CODEC name for usability.
On Wednesday 30 September 2009 13:46:29 ext Mark Brown wrote:
There's way more problems than this with multiple CODECs and multiple cards - DAPM can't really cope at all, and the split between CODECs and cards isn't as clear as would be desirable.
Yes, when I have moved to 2.6.31 it was kind of weird to have the snd_soc_init_card(socdev) called from the codec driver itself, which I still not feel too comfortable with...
Looking into this is one of my big tasks for the rest of the year. With this particular problem I suspect the fix will fall out of the other reorganisations required to make the rest of the system work.
I have in my board two codecs, using separate sound cards for each. One card - one codec. So far DAPM and in general ASoC behaved quite well in all possible scenarios. But not be able to use the debugfs in my setup is quite frustrating.
Btw. do you have some information about this upcoming reorganization, to see what is coming?
codec->debugfs_codec_root = debugfs_create_dir(codec->name,debugfs_root);I don't think the CODEC name is what we want to use here, one of the common cases is going to be to have two or more mono DACs or speaker drivers in the system, normally of the same type and therefore with the same name set here.
I see, these setups are going to be interesting and greatly complicates things.
A dev_name() is going to be required as well, munged in with the CODEC name for usability.
The dev_name() is used in earlier ASoC versions as debugfs directory name.
All of that being said, it would be still nice to at least fix the current situation in some way, since the current implementation is clearly not behaving correctly.
Does it help, if I use the dev_name() instead of the codec->name for the directory for now?
Later when you are doing the reorganization of the code, this could be also fixed.
Thanks, Péter
On Wed, Sep 30, 2009 at 02:21:46PM +0300, Peter Ujfalusi wrote:
Yes, when I have moved to 2.6.31 it was kind of weird to have the snd_soc_init_card(socdev) called from the codec driver itself, which I still not feel too comfortable with...
That was always happening anyway, the function just got renamed.
I have in my board two codecs, using separate sound cards for each. One card - one codec. So far DAPM and in general ASoC behaved quite well in all possible scenarios.
I can't remember off-hand what they are but there are some issues you'll run into if you look hard enough - I certainly wouldn't say that it's a supported configuration. Device probe/removal is where the issues are if I remember correctly, it can work but it's fragile.
Btw. do you have some information about this upcoming reorganization, to see what is coming?
Probably from an external point of view it'll have a dev_name added to all the strings in the DAPM maps for deduplication when used in the board drivers (which are the only things that ought to need to know about more than one device). For the cards the soc-audio device will go away and the card device will be used directly, possibly with only the DAIs being directly referenced from the machine driver.
It's much more of an upheaval internally.
All of that being said, it would be still nice to at least fix the current situation in some way, since the current implementation is clearly not behaving correctly.
Does it help, if I use the dev_name() instead of the codec->name for the directory for now?
I'd use both, the dev_name() is rather illegible by itself.
On Wednesday 30 September 2009 15:39:38 ext Mark Brown wrote:
On Wed, Sep 30, 2009 at 02:21:46PM +0300, Peter Ujfalusi wrote:
Yes, when I have moved to 2.6.31 it was kind of weird to have the snd_soc_init_card(socdev) called from the codec driver itself, which I still not feel too comfortable with...
That was always happening anyway, the function just got renamed.
Might be, but in case if you have one sound card with two (or more) codecs (or dai_link) on it than all codecs will call the snd_soc_init_card(socdev), right?
For me at least this does not sound right. I have to check the code itself what it actually does.
I have in my board two codecs, using separate sound cards for each. One card - one codec. So far DAPM and in general ASoC behaved quite well in all possible scenarios.
I can't remember off-hand what they are but there are some issues you'll run into if you look hard enough - I certainly wouldn't say that it's a supported configuration. Device probe/removal is where the issues are if I remember correctly, it can work but it's fragile.
At the moment I'm compiling things in (no problems so far), but I'll try them as modules and see how they behave.
Btw. do you have some information about this upcoming reorganization, to see what is coming?
Probably from an external point of view it'll have a dev_name added to all the strings in the DAPM maps for deduplication when used in the board drivers (which are the only things that ought to need to know about more than one device). For the cards the soc-audio device will go away and the card device will be used directly, possibly with only the DAIs being directly referenced from the machine driver.
It's much more of an upheaval internally.
I see, thanks.
All of that being said, it would be still nice to at least fix the current situation in some way, since the current implementation is clearly not behaving correctly.
Does it help, if I use the dev_name() instead of the codec->name for the directory for now?
I'd use both, the dev_name() is rather illegible by itself.
Is it good if the patch creates a directory hierarchy like this: debugfs/asoc/{dev_name()}/{codec->name}/...
Or should I use some intermediate buffer to construct a single level tree: debugfs/asoc/{dev_name()}-{codec->name}/...
From the point of what is coming the former is much more future proof, if we take into account that we can have more than one codec in one soc device (or card).
On Thursday 01 October 2009 08:41:07 Ujfalusi Peter (Nokia-D/Tampere) wrote:
Is it good if the patch creates a directory hierarchy like this: debugfs/asoc/{dev_name()}/{codec->name}/...
Or should I use some intermediate buffer to construct a single level tree: debugfs/asoc/{dev_name()}-{codec->name}/...
From the point of what is coming the former is much more future proof, if we take into account that we can have more than one codec in one soc device (or card).
After a bit of thinking, I think I will go with the later one: debugfs/asoc/{dev_name()}-{codec->name}/...
It needs some intermediate buffer to construct the string for the directory, but I think that is the better way for now.
participants (2)
-
Mark Brown -
Peter Ujfalusi