Re: [PATCH v7 1/1] alsa: jack: implement software jack injection via debugfs

From: Takashi Iwai <tiwai@suse.de>
To: Hui Wang <hui.wang@canonical.com>
Cc: alsa-devel@alsa-project.org, kai.vehmanen@linux.intel.com
Subject: Re: [PATCH v7 1/1] alsa: jack: implement software jack injection via debugfs
Date: Mon, 25 Jan 2021 15:32:09 +0100	[thread overview]
Message-ID: <s5ha6sx3x5i.wl-tiwai@suse.de> (raw)
In-Reply-To: <20210125032118.13269-2-hui.wang@canonical.com>

On Mon, 25 Jan 2021 04:21:18 +0100,
Hui Wang wrote:
> 
> This change adds audio jack injection feature through debugfs, with
> this feature, we could validate alsa userspace changes by injecting
> plugin or plugout events to the non-phantom audio jacks.
> 
> With this change, the sound core will build the folders
> $debugfs_mount_dir/sound/cardN if SND_DEBUG and DEBUG_FS are enabled.
> And if users also enable the SND_JACK_INJECTION_DEBUG, the jack
> injection nodes will be built in the folder cardN like below:
> 
> $tree $debugfs_mount_dir/sound
> $debugfs_mount_dir/sound
> ├── card0
> │   ├── HDMI_DP_pcm_10_Jack
> │   │   ├── jackin_inject
> │   │   ├── kctl_id
> │   │   ├── mask_bits
> │   │   ├── status
> │   │   ├── sw_inject_enable
> │   │   └── type
> ...
> │   └── HDMI_DP_pcm_9_Jack
> │       ├── jackin_inject
> │       ├── kctl_id
> │       ├── mask_bits
> │       ├── status
> │       ├── sw_inject_enable
> │       └── type
> └── card1
>     ├── HDMI_DP_pcm_5_Jack
>     │   ├── jackin_inject
>     │   ├── kctl_id
>     │   ├── mask_bits
>     │   ├── status
>     │   ├── sw_inject_enable
>     │   └── type
>     ...
>     ├── Headphone_Jack
>     │   ├── jackin_inject
>     │   ├── kctl_id
>     │   ├── mask_bits
>     │   ├── status
>     │   ├── sw_inject_enable
>     │   └── type
>     └── Headset_Mic_Jack
>         ├── jackin_inject
>         ├── kctl_id
>         ├── mask_bits
>         ├── status
>         ├── sw_inject_enable
>         └── type
> 
> The nodes kctl_id, mask_bits, status and type are read-only, users
> could check jack or jack_kctl's information through them.
> 
> The nodes sw_inject_enable and jackin_inject are directly used for
> injection. The sw_inject_enable is read-write, users could check if
> software injection is enabled or not on this jack, and users could
> echo 1 or 0 to enable or disable software injection on this jack. Once
> the injection is enabled, the jack will not change by hardware events
> anymore, once the injection is disabled, the jack will restore the
> last reported hardware events to the jack. The jackin_inject is
> write-only, if the injection is enabled, users could echo 1 or 0 to
> this node to inject plugin or plugout events to this jack.
> 
> For the detailed usage information on these nodes, please refer to
> Documentation/sound/designs/jack-injection.rst.
> 
> Reviewed-by: Takashi Iwai <tiwai@suse.de>
> Reviewed-by: Jaroslav Kysela <perex@perex.cz>
> Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> Signed-off-by: Hui Wang <hui.wang@canonical.com>

Some minor nitpicking:

> +static int snd_jack_debugfs_add_inject_node(struct snd_jack *jack,
> +					    struct snd_jack_kctl *jack_kctl)
> +{
> +	char *tname;
> +	int i;
> +
> +	tname = kstrdup(jack_kctl->kctl->id.name, GFP_KERNEL);
> +	if (!tname)
> +		return -ENOMEM;
> +
> +	/* replace the chars which are not suitable for folder's name with _ */
> +	for (i = 0; i < strlen(tname); i++)

No need to use strlen(), just check the NUL character on tname[i].

> --- a/sound/core/sound.c
> +++ b/sound/core/sound.c
> @@ -9,6 +9,7 @@
>  #include <linux/time.h>
>  #include <linux/device.h>
>  #include <linux/module.h>
> +#include <linux/debugfs.h>
>  #include <sound/core.h>
>  #include <sound/minors.h>
>  #include <sound/info.h>
> @@ -39,6 +40,9 @@ MODULE_ALIAS_CHARDEV_MAJOR(CONFIG_SND_MAJOR);
>  int snd_ecards_limit;
>  EXPORT_SYMBOL(snd_ecards_limit);
>  
> +struct dentry *sound_debugfs_root;
> +EXPORT_SYMBOL_GPL(sound_debugfs_root);

I guess this should be wrapped with CONFIG_SND_DEBUG ifdef?

thanks,

Takashi