[alsa-devel] [PATCH] [ALSA] Allow setting codec register with debugfs filesystem

[Mark Brown]

On Tue, Oct 07, 2008 at 02:03:55PM -0700, Troy Kisky wrote:

Thanks for this - looks good.  A few comments below, nitpicks rather
than anything substantial.

> +#ifdef CONFIG_SND_SOC_DEBUG_FS
> +	struct dentry	*debugfs_root;
> +	struct dentry	*debugfs_codec_reg;
> +#endif

It would be better to just make this conditional on debugfs support -
this isn't sufficiently high cost to make the additional complexity
worthwhile.  This is the general idiom for debugfs.

> +static ssize_t code_reg_read_file(struct file *file, char __user *user_buf,
> +			       size_t count, loff_t *ppos)
> +{
> +	ssize_t res;

ret would be more consistent with the rest of ASoC (told you I was
picking nits).

> +	struct snd_soc_device *devdata = file->private_data;
> +	char *buf = kmalloc(PAGE_SIZE, GFP_KERNEL);

It should check for out of memory here.  If we ever get codecs with
enormous register maps we'll need to worry about PAGE_SIZE but that's an
issue for sysfs too and not an issue currently.

> +static void soc_init_debugfs(struct snd_soc_device *socdev)
> +{
> +	struct dentry *root, *debugfs_codec_reg;
> +	root = debugfs_create_dir(dev_name(socdev->dev), NULL);
> +	if (IS_ERR(root) || !root)
> +		goto exit1;

There's an existing debugfs file (the DAPM pop time configuration) which
I'd expect to be handled by this function - it should probably be
renamed to something like soc_init_codec_debugfs().

The directory should probably be a subdirectory of that created by DAPM,
or you could just put the codec register file in that directly until we
support two codecs (which will needs updates to the sysfs code anyway).

> +	debugfs_codec_reg = debugfs_create_file("codec_reg", 0644,
> +			root, socdev, &codec_reg_fops);
> +	if (!debugfs_codec_reg)
> +		goto exit2;

> +
> +	socdev->debugfs_root = root;

> +static void soc_cleanup_debugfs(struct snd_soc_device *socdev)
> +{
> +	debugfs_remove(socdev->debugfs_codec_reg);

debugfs directory removal is recursive so no need to do this (or
remember the file) unless you put it in the directory with the DAPM file.