[PATCH 0/1 - try2] ASoC: debugfs support improvement

[PATCH 0/1 - try2] ASoC: debugfs support improvement

[Peter Ujfalusi]

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(-)

[PATCH 1/1 - try2] ASoC: add support for multiple cards/codecs in debugfs

[Peter Ujfalusi]

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

Re: [PATCH 1/1 - try2] ASoC: add support for multiple cards/codecs in debugfs

[Mark Brown]

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.

Re: [PATCH 1/1 - try2] ASoC: add support for multiple cards/codecs in debugfs

[Peter Ujfalusi]

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.

> 

-- 
Péter

Re: [PATCH 1/1 - try2] ASoC: add support for multiple cards/codecs in debugfs

[Mark Brown]

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.

Re: [PATCH 1/1 - try2] ASoC: add support for multiple cards/codecs in debugfs

[Mark Brown]

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.

Re: [PATCH 1/1 - try2] ASoC: add support for multiple cards/codecs in debugfs

[Peter Ujfalusi]

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.

-- 
Péter

Re: [PATCH 1/1 - try2] ASoC: add support for multiple cards/codecs in debugfs

[Peter Ujfalusi]

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

Re: [PATCH 1/1 - try2] ASoC: add support for multiple cards/codecs in debugfs

[Mark Brown]

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.