[RFC][PATCH v2 1/1] alsa: jack: implement software jack injection via debugfs

[Kai Vehmanen]

Hey,

I gave a quick test spin and features seems to work as advertized. A few 
minor comments on the code. If Jaroslav you think this would be ok as an 
approach, I can give a more extensive test run on this.

On Wed, 16 Dec 2020, Hui Wang wrote:

> We want to perform remote audio auto test, need the audio jack to
> change from plugout to plugin or vice versa by software ways.
> 
> Here the design is creating a sound-core root folder in the debugfs
> dir, and each sound card will create a folder cardN under sound-core,
> then the sound jack will create folders by jack_ctrl->ctrl->id.name,
> and will create 2 file nodes jackin_inject and sw_inject_enable in
> the folder, this is the layout of folder on a machine with 2 sound
> cards:
> $tree $debugfs_mount_dir/sound-core
> sound-core/
> ├── card0
> │   ├── HDMI!DP,pcm=10 Jack

this combination of "!,= " characters in filenames is a bit non-unixy, 
but maybe in 2020 we are ready for this. 

> +static void _snd_jack_report(struct snd_jack *jack, int status, bool from_inject)
> +{
> +	struct snd_jack_kctl *jack_kctl;
> +	unsigned int mask_bits = 0;
> +#ifdef CONFIG_SND_JACK_INPUT_DEV
> +	int i;
> +#endif
> +	list_for_each_entry(jack_kctl, &jack->kctl_list, list) {
> +		if (jack_kctl->sw_inject_enable == from_inject)
> +			snd_kctl_jack_report(jack->card, jack_kctl->kctl,
> +					     status & jack_kctl->mask_bits);
> +		else if (jack_kctl->sw_inject_enable)
> +			mask_bits |= jack_kctl->mask_bits;
> +	}

I'm wondering if it would be worth the code duplication to have the 
inject-variant of this code in a separate function. I find the above code 
to set up "mask_bits" a bit hard to read and this adds a layer of 
complexity to anyone just wanting to look at the regular jack report code 
path.

> +static ssize_t sw_inject_enable_write(struct file *file,
> +				      const char __user *from, size_t count, loff_t *ppos)
> +{
> +	struct snd_jack_kctl *jack_kctl = file->private_data;
> +	char *buf;
> +	int ret, err;
> +	unsigned long enable;
> +
> +	buf = kzalloc(count, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	ret = simple_write_to_buffer(buf, count, ppos, from, count);
> +	err = kstrtoul(buf, 0, &enable);
> +	if (err) {
> +		ret = err;
> +		goto exit;
> +	}
> +
> +	jack_kctl->sw_inject_enable = !!enable;

Here it's a bit annoying that after you disable sw_inject, the kcontrol
values are not restored to reflrect actual hw state (until there are 
new jack events from hw). User-space cannot completely handle the 
save'n'restore as it cannot detect if real hw jack status changed 
during the sw-inject test. OTOH, this would require caching the most 
recent value and maybe not worth the effort.

> +static int snd_jack_debugfs_add_inject_node(struct snd_jack *jack,
> +					    struct snd_jack_kctl *jack_kctl)
> +{
> +	char *tname;
> +
> +	/* the folder's name can't contains '/', need to replace it with '!' as lib/kobject.c does */
> +	tname = kstrdup(jack_kctl->kctl->id.name, GFP_KERNEL);

This goes over 100-column limit and triggers a checkpatch complaint.

Br, Kai