[alsa-devel] [PATCH 0/1 - try2] 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: {dev_name(socdev->dev)}-{codec->name} for example:
debugfs/asoc/soc-audio.0-twl4030/...
So it is easier to find the correct codec we are looking for.
Note: The patch accidentally fixes one trailing space in the soc_pcm_apply_symmetry function.
--- Peter Ujfalusi (1): ASoC: add support for multiple cards/codecs in debugfs
include/sound/soc.h | 1 + sound/soc/soc-core.c | 28 ++++++++++++++++++++-------- 2 files changed, 21 insertions(+), 8 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/{dev_name(socdev->dev)}-{codec->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 | 28 ++++++++++++++++++++-------- 2 files changed, 21 insertions(+), 8 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..ea0b8be 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -126,7 +126,7 @@ static int soc_pcm_apply_symmetry(struct snd_pcm_substream *substream)
if (codec_dai->symmetric_rates || cpu_dai->symmetric_rates || machine->symmetric_rates) { - dev_dbg(card->dev, "Symmetry forces %dHz rate\n", + dev_dbg(card->dev, "Symmetry forces %dHz rate\n", machine->rate);
ret = snd_pcm_hw_constraint_minmax(substream->runtime, @@ -1254,21 +1254,35 @@ static const struct file_operations codec_reg_fops = {
static void soc_init_codec_debugfs(struct snd_soc_codec *codec) { + char codec_root[128]; + + snprintf(codec_root, sizeof(codec_root), + "%s-%s", dev_name(codec->socdev->dev), codec->name); + + codec->debugfs_codec_root = debugfs_create_dir(codec_root, + 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 +1292,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 -- 1.6.5.rc1
On Thu, Oct 01, 2009 at 10:32:47AM +0300, Peter Ujfalusi wrote:
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/{dev_name(socdev->dev)}-{codec->name}/codec_reg
I'd rather use dev_name() for the CODEC itself if possible, that is more likely to be stable going forward and one of the immediate aims with the API refactoring is to remove socdev entirely at runtime.
That can be done in a followup, though.
@@ -126,7 +126,7 @@ static int soc_pcm_apply_symmetry(struct snd_pcm_substream *substream)
if (codec_dai->symmetric_rates || cpu_dai->symmetric_rates || machine->symmetric_rates) {
dev_dbg(card->dev, "Symmetry forces %dHz rate\n",
dev_dbg(card->dev, "Symmetry forces %dHz rate\n", machine->rate);ret = snd_pcm_hw_constraint_minmax(substream->runtime,
but this doesn't seem to accomplish that goal? I can't actually spot the difference either, I'm assuming it's just whitespace? If so I'll apply as-is.
On Thursday 01 October 2009 13:51:07 ext Mark Brown wrote:
On Thu, Oct 01, 2009 at 10:32:47AM +0300, Peter Ujfalusi wrote:
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/{dev_name(socdev->dev)}-{codec->name}/codec_reg
I'd rather use dev_name() for the CODEC itself if possible, that is more likely to be stable going forward and one of the immediate aims with the API refactoring is to remove socdev entirely at runtime.
At this point the codec->dev was NULL, that is why I have used the socdev->dev instead.
That can be done in a followup, though.
@@ -126,7 +126,7 @@ static int soc_pcm_apply_symmetry(struct snd_pcm_substream *substream)
if (codec_dai->symmetric_rates || cpu_dai->symmetric_rates || machine->symmetric_rates) {
dev_dbg(card->dev, "Symmetry forces %dHz rate\n",
dev_dbg(card->dev, "Symmetry forces %dHz rate\n", machine->rate);ret = snd_pcm_hw_constraint_minmax(substream->runtime,
but this doesn't seem to accomplish that goal? I can't actually spot the difference either, I'm assuming it's just whitespace? If so I'll apply as-is.
It is my editor, which removed the tailing space from that line, I have mentioned it in the intro mail.
On Thu, Oct 01, 2009 at 02:02:04PM +0300, Peter Ujfalusi wrote:
On Thursday 01 October 2009 13:51:07 ext Mark Brown wrote:
I'd rather use dev_name() for the CODEC itself if possible, that is more likely to be stable going forward and one of the immediate aims with the API refactoring is to remove socdev entirely at runtime.
At this point the codec->dev was NULL, that is why I have used the socdev->dev instead.
Right, your CODEC driver needs updating to current APIs. Omitting the device name is a good fallback here since you won't be able to have more than one of the same device anyway if a device isn't provided.
ret = snd_pcm_hw_constraint_minmax(substream->runtime,
but this doesn't seem to accomplish that goal? I can't actually spot the difference either, I'm assuming it's just whitespace? If so I'll apply as-is.
It is my editor, which removed the tailing space from that line, I have mentioned it in the intro mail.
Oh, right. Like I was saying yesterday it's better to not use a separate cover letter for single patches.
On Thursday 01 October 2009 14:09:19 ext Mark Brown wrote:
On Thu, Oct 01, 2009 at 02:02:04PM +0300, Peter Ujfalusi wrote:
On Thursday 01 October 2009 13:51:07 ext Mark Brown wrote:
I'd rather use dev_name() for the CODEC itself if possible, that is more likely to be stable going forward and one of the immediate aims with the API refactoring is to remove socdev entirely at runtime.
At this point the codec->dev was NULL, that is why I have used the socdev->dev instead.
Right, your CODEC driver needs updating to current APIs. Omitting the device name is a good fallback here since you won't be able to have more than one of the same device anyway if a device isn't provided.
One of the codec to be blamed is the twl4030 codec, but since that does not use any _device/_driver method to load, there is no dev to be assigned to codec-
dev...
I don't know if it would make sense to use platform_driver_register (and associates) with it in the future.
The other codec, which I'm preparing for sending is using i2c probing, so I have fixed that, so now it has codec->dev initialized correctly.
your CODEC driver needs updating to current APIs
Hmm, there are quite a bit of inconsistency among the codec drivers, so I'm not sure what is the correct way of doing this. I will use the wm8993 driver for further fine tuning, since that seams to be rather new in the tree.
I'll send a patch later to fix this implementation: if the codec->dev is valid, than it is going to use dev_name(codec->dev), if it is NULL, than it will use either the codec->name or provide the same string as the current implementation does ( {dev_name(socdev->dev)}-{codec->name} ), whichever is the preferred.
Thanks, Péter
On Thu, Oct 01, 2009 at 04:49:17PM +0300, Peter Ujfalusi wrote:
One of the codec to be blamed is the twl4030 codec, but since that does not use any _device/_driver method to load, there is no dev to be assigned to codec-
dev...
I don't know if it would make sense to use platform_driver_register (and associates) with it in the future.
Yes, it should be using a platform device and the MFD framework (as most of the other TWL4030 subdevices do already).
your CODEC driver needs updating to current APIs
Hmm, there are quite a bit of inconsistency among the codec drivers, so I'm not sure what is the correct way of doing this. I will use the wm8993 driver for further fine tuning, since that seams to be rather new in the tree.
In general look at the drivers that have been touched most recently - wm8993 is a good one, as are wm8903 and wm8731, but the main thing is to look at one that's not registering the DAIs in the module init function but is using normal device model probing and registering the DAIs only after the underlying device has come up.
I'll send a patch later to fix this implementation: if the codec->dev is valid, than it is going to use dev_name(codec->dev), if it is NULL, than it will use either the codec->name or provide the same string as the current implementation does ( {dev_name(socdev->dev)}-{codec->name} ), whichever is the preferred.
I'd just leave it as codec->name - the soc-audio device name doesn't add anything.
On Thu, Oct 01, 2009 at 02:02:04PM +0300, Peter Ujfalusi wrote:
but this doesn't seem to accomplish that goal? I can't actually spot the difference either, I'm assuming it's just whitespace? If so I'll apply as-is.
It is my editor, which removed the tailing space from that line, I have mentioned it in the intro mail.
I've now applied the patch, except for this hunk which failed to apply against for-2.6.33 so I dropped it.
On Thursday 01 October 2009 14:19:56 ext Mark Brown wrote:
On Thu, Oct 01, 2009 at 02:02:04PM +0300, Peter Ujfalusi wrote:
but this doesn't seem to accomplish that goal? I can't actually spot the difference either, I'm assuming it's just whitespace? If so I'll apply as-is.
It is my editor, which removed the tailing space from that line, I have mentioned it in the intro mail.
I've now applied the patch, except for this hunk which failed to apply against for-2.6.33 so I dropped it.
Thanks, I'll try to teach my editor to not to do that again. These things tends to be getting clever day-by-day, which is annoying sometimes.
Sorry about the hassle it caused to you.
participants (2)
-
Mark Brown -
Peter Ujfalusi