[RFC][PATCH v2 0/1] design a way to change audio Jack state by software

[RFC][PATCH v2 0/1] design a way to change audio Jack state by software

[Hui Wang]

the changes in the V2:
 - using debugfs instead of sysfs
 - using jack_ctrl to create a folder instead of snd_jack, since ASoC drivers
   could create multi jack_ctrls within a snd_jack
 - create a folder for each jack_ctrl instead for all jack_ctrls

This is the layout of folder on a machine with 2 sound cards:
root@hwang4-ThinkPad-P1-Gen-3:/sys/kernel/debug# tree /sys/kernel/debug/sound-core/
/sys/kernel/debug/sound-core/
├── card0
│   ├── HDMI!DP,pcm=10 Jack
│   │   ├── jackin_inject
│   │   └── sw_inject_enable
│   ├── HDMI!DP,pcm=11 Jack
│   │   ├── jackin_inject
│   │   └── sw_inject_enable
│   ├── HDMI!DP,pcm=3 Jack
│   │   ├── jackin_inject
│   │   └── sw_inject_enable
│   ├── HDMI!DP,pcm=7 Jack
│   │   ├── jackin_inject
│   │   └── sw_inject_enable
│   ├── HDMI!DP,pcm=8 Jack
│   │   ├── jackin_inject
│   │   └── sw_inject_enable
│   └── HDMI!DP,pcm=9 Jack
│       ├── jackin_inject
│       └── sw_inject_enable
└── card1
    ├── HDMI!DP,pcm=3 Jack
    │   ├── jackin_inject
    │   └── sw_inject_enable
    ├── HDMI!DP,pcm=4 Jack
    │   ├── jackin_inject
    │   └── sw_inject_enable
    ├── HDMI!DP,pcm=5 Jack
    │   ├── jackin_inject
    │   └── sw_inject_enable
    ├── Headphone Jack
    │   ├── jackin_inject
    │   └── sw_inject_enable
    ├── Headset Jack
    │   ├── jackin_inject
    │   └── sw_inject_enable
    └── Mic Jack
        ├── jackin_inject
        └── sw_inject_enable

The sw_inject_enable has rw mode, the jackin_inject has write_only mode.

If users want to inject a plugin to Headphone Jack, they could run:
echo 1 > 'Headphone Jack'/sw_inject_enable  /* now, this jack state can't be changed by hw events */
cat 'Headphone Jack'/sw_inject_enable
ack: Headphone Jack		Inject Enabled: 1
echo 1 > 'Headphone Jack'/jackin_inject /* this will inject a plugin to Headphone Jack */

After testing, run
echo 0 > 'Headphone Jack'/sw_inject_enable /* the hw events will change the jack state */

Hui Wang (1):
  alsa: jack: implement software jack injection via debugfs

 include/sound/core.h |   2 +
 sound/core/init.c    |   7 ++
 sound/core/jack.c    | 202 ++++++++++++++++++++++++++++++++++++-------
 sound/core/sound.c   |   8 ++
 4 files changed, 188 insertions(+), 31 deletions(-)

-- 
2.25.1

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

[Hui Wang]

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
│   │   ├── jackin_inject
│   │   └── sw_inject_enable
│   ├── HDMI!DP,pcm=11 Jack
│   │   ├── jackin_inject
│   │   └── sw_inject_enable
│   ├── HDMI!DP,pcm=3 Jack
│   │   ├── jackin_inject
│   │   └── sw_inject_enable
│   ├── HDMI!DP,pcm=7 Jack
│   │   ├── jackin_inject
│   │   └── sw_inject_enable
│   ├── HDMI!DP,pcm=8 Jack
│   │   ├── jackin_inject
│   │   └── sw_inject_enable
│   └── HDMI!DP,pcm=9 Jack
│       ├── jackin_inject
│       └── sw_inject_enable
└── card1
    ├── HDMI!DP,pcm=3 Jack
    │   ├── jackin_inject
    │   └── sw_inject_enable
    ├── HDMI!DP,pcm=4 Jack
    │   ├── jackin_inject
    │   └── sw_inject_enable
    ├── HDMI!DP,pcm=5 Jack
    │   ├── jackin_inject
    │   └── sw_inject_enable
    ├── Headphone Jack
    │   ├── jackin_inject
    │   └── sw_inject_enable
    ├── Headset Jack
    │   ├── jackin_inject
    │   └── sw_inject_enable
    └── Mic Jack
        ├── jackin_inject
        └── sw_inject_enable

Suppose users want to enable jack injection for Headphone, they need
to run $sudo sh -c 'echo 1 > 'Headphone Jack'/sw_inject_enable', then
users could change the Headphone Jack state through jackin_inject and
this Jack's state will not be changed by non-injection ways anymore
until users echo 0 to sw_inject_enable.

Users could run $sudo sh -c 'echo 1 > 'Headphone Jack'/jackin_inject'
to trigger the Headphone jack to plugin or echo 0 to trigger it to
plugout.

If users finish their test, they could run
$sudo sh -c 'echo 0 > 'Headphone Jack'/sw_inject_enable' to disable
injection and let non-injection ways control this Jack.

Signed-off-by: Hui Wang <hui.wang@canonical.com>
---
 include/sound/core.h |   2 +
 sound/core/init.c    |   7 ++
 sound/core/jack.c    | 202 ++++++++++++++++++++++++++++++++++++-------
 sound/core/sound.c   |   8 ++
 4 files changed, 188 insertions(+), 31 deletions(-)

diff --git a/include/sound/core.h b/include/sound/core.h
index 0462c577d7a3..11d61e4248ee 100644
--- a/include/sound/core.h
+++ b/include/sound/core.h
@@ -122,6 +122,7 @@ struct snd_card {
 
 	size_t total_pcm_alloc_bytes;	/* total amount of allocated buffers */
 	struct mutex memory_mutex;	/* protection for the above */
+	struct dentry *debugfs_root;    /* debugfs root for card */
 
 #ifdef CONFIG_PM
 	unsigned int power_state;	/* power state */
@@ -180,6 +181,7 @@ static inline struct device *snd_card_get_device_link(struct snd_card *card)
 extern int snd_major;
 extern int snd_ecards_limit;
 extern struct class *sound_class;
+extern struct dentry *sound_core_debugfs_root;
 
 void snd_request_card(int card);
 
diff --git a/sound/core/init.c b/sound/core/init.c
index 764dbe673d48..a9ceaf788019 100644
--- a/sound/core/init.c
+++ b/sound/core/init.c
@@ -13,6 +13,7 @@
 #include <linux/time.h>
 #include <linux/ctype.h>
 #include <linux/pm.h>
+#include <linux/debugfs.h>
 #include <linux/completion.h>
 
 #include <sound/core.h>
@@ -163,6 +164,7 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
 {
 	struct snd_card *card;
 	int err;
+	char name[8];
 
 	if (snd_BUG_ON(!card_ret))
 		return -EINVAL;
@@ -246,6 +248,10 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
 		dev_err(parent, "unable to create card info\n");
 		goto __error_ctl;
 	}
+
+	sprintf(name, "card%d", idx);
+	card->debugfs_root = debugfs_create_dir(name, sound_core_debugfs_root);
+
 	*card_ret = card;
 	return 0;
 
@@ -418,6 +424,7 @@ int snd_card_disconnect(struct snd_card *card)
 	/* notify all devices that we are disconnected */
 	snd_device_disconnect_all(card);
 
+	debugfs_remove(card->debugfs_root);
 	snd_info_card_disconnect(card);
 	if (card->registered) {
 		device_del(&card->card_dev);
diff --git a/sound/core/jack.c b/sound/core/jack.c
index 503c8af79d55..fa6b3cdc0431 100644
--- a/sound/core/jack.c
+++ b/sound/core/jack.c
@@ -8,6 +8,8 @@
 #include <linux/input.h>
 #include <linux/slab.h>
 #include <linux/module.h>
+#include <linux/ctype.h>
+#include <linux/debugfs.h>
 #include <sound/jack.h>
 #include <sound/core.h>
 #include <sound/control.h>
@@ -16,6 +18,9 @@ struct snd_jack_kctl {
 	struct snd_kcontrol *kctl;
 	struct list_head list;  /* list of controls belong to the same jack */
 	unsigned int mask_bits; /* only masked status bits are reported via kctl */
+	struct snd_jack *jack;  /* pointer to struct snd_jack */
+	bool sw_inject_enable;  /* allow to inject plug event via debugfs */
+	struct dentry *jack_debugfs_root; /* jack_kctl debugfs root */
 };
 
 #ifdef CONFIG_SND_JACK_INPUT_DEV
@@ -109,12 +114,174 @@ static int snd_jack_dev_register(struct snd_device *device)
 }
 #endif /* CONFIG_SND_JACK_INPUT_DEV */
 
+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;
+	}
+
+#ifdef CONFIG_SND_JACK_INPUT_DEV
+	if (!jack->input_dev)
+		return;
+
+	for (i = 0; i < ARRAY_SIZE(jack->key); i++) {
+		int testbit = SND_JACK_BTN_0 >> i;
+
+		if (!from_inject)
+			testbit &= ~mask_bits;
+		if (jack->type & testbit)
+			input_report_key(jack->input_dev, jack->key[i],
+					 status & testbit);
+	}
+
+	for (i = 0; i < ARRAY_SIZE(jack_switch_types); i++) {
+		int testbit = 1 << i;
+
+		if (!from_inject)
+			testbit &= ~mask_bits;
+		if (jack->type & testbit)
+			input_report_switch(jack->input_dev,
+					    jack_switch_types[i],
+					    status & testbit);
+	}
+
+	input_sync(jack->input_dev);
+#endif /* CONFIG_SND_JACK_INPUT_DEV */
+}
+
+#ifdef CONFIG_DEBUG_FS
+static ssize_t sw_inject_enable_read(struct file *file,
+				     char __user *to, size_t count, loff_t *ppos)
+{
+	struct snd_jack_kctl *jack_kctl = file->private_data;
+	char *buf;
+	int len, ret;
+
+	buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	len = scnprintf(buf, PAGE_SIZE, "%s: %s\t\t%s: %i\n", "Jack", jack_kctl->kctl->id.name,
+			"Inject Enabled", jack_kctl->sw_inject_enable);
+	ret = simple_read_from_buffer(to, count, ppos, buf, len);
+
+	kfree(buf);
+	return ret;
+}
+
+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;
+
+ exit:
+	kfree(buf);
+	return ret;
+}
+
+static ssize_t jackin_inject_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;
+
+	if (!jack_kctl->sw_inject_enable)
+		return -EINVAL;
+
+	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;
+	}
+
+	_snd_jack_report(jack_kctl->jack, !!enable ? jack_kctl->mask_bits : 0, true);
+
+ exit:
+	kfree(buf);
+	return ret;
+}
+
+static const struct file_operations sw_inject_enable_fops = {
+	.open = simple_open,
+	.read = sw_inject_enable_read,
+	.write = sw_inject_enable_write,
+	.llseek = default_llseek,
+};
+
+static const struct file_operations jackin_inject_fops = {
+	.open = simple_open,
+	.write = jackin_inject_write,
+	.llseek = default_llseek,
+};
+
+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);
+	if (!tname)
+		return -ENOMEM;
+	strreplace(tname, '/', '!');
+	jack_kctl->jack_debugfs_root = debugfs_create_dir(tname, jack->card->debugfs_root);
+	kfree(tname);
+
+	debugfs_create_file("sw_inject_enable", 0644, jack_kctl->jack_debugfs_root, jack_kctl,
+			    &sw_inject_enable_fops);
+
+	debugfs_create_file("jackin_inject", 0200, jack_kctl->jack_debugfs_root, jack_kctl,
+			    &jackin_inject_fops);
+
+	return 0;
+}
+#else
+static int snd_jack_debugfs_add_inject_node(struct snd_jack *jack,
+					    struct snd_jack_kctl *jack_kctl)
+{
+	return 0;
+}
+#endif
+
 static void snd_jack_kctl_private_free(struct snd_kcontrol *kctl)
 {
 	struct snd_jack_kctl *jack_kctl;
 
 	jack_kctl = kctl->private_data;
 	if (jack_kctl) {
+		debugfs_remove(jack_kctl->jack_debugfs_root);
 		list_del(&jack_kctl->list);
 		kfree(jack_kctl);
 	}
@@ -122,7 +289,10 @@ static void snd_jack_kctl_private_free(struct snd_kcontrol *kctl)
 
 static void snd_jack_kctl_add(struct snd_jack *jack, struct snd_jack_kctl *jack_kctl)
 {
+	jack_kctl->jack = jack;
 	list_add_tail(&jack_kctl->list, &jack->kctl_list);
+	if (!strstr(jack_kctl->kctl->id.name, "Phantom"))
+		snd_jack_debugfs_add_inject_node(jack, jack_kctl);
 }
 
 static struct snd_jack_kctl * snd_jack_kctl_new(struct snd_card *card, const char *name, unsigned int mask)
@@ -339,39 +509,9 @@ EXPORT_SYMBOL(snd_jack_set_key);
  */
 void snd_jack_report(struct snd_jack *jack, int status)
 {
-	struct snd_jack_kctl *jack_kctl;
-#ifdef CONFIG_SND_JACK_INPUT_DEV
-	int i;
-#endif
-
 	if (!jack)
 		return;
 
-	list_for_each_entry(jack_kctl, &jack->kctl_list, list)
-		snd_kctl_jack_report(jack->card, jack_kctl->kctl,
-					    status & jack_kctl->mask_bits);
-
-#ifdef CONFIG_SND_JACK_INPUT_DEV
-	if (!jack->input_dev)
-		return;
-
-	for (i = 0; i < ARRAY_SIZE(jack->key); i++) {
-		int testbit = SND_JACK_BTN_0 >> i;
-
-		if (jack->type & testbit)
-			input_report_key(jack->input_dev, jack->key[i],
-					 status & testbit);
-	}
-
-	for (i = 0; i < ARRAY_SIZE(jack_switch_types); i++) {
-		int testbit = 1 << i;
-		if (jack->type & testbit)
-			input_report_switch(jack->input_dev,
-					    jack_switch_types[i],
-					    status & testbit);
-	}
-
-	input_sync(jack->input_dev);
-#endif /* CONFIG_SND_JACK_INPUT_DEV */
+	return _snd_jack_report(jack, status, false);
 }
 EXPORT_SYMBOL(snd_jack_report);
diff --git a/sound/core/sound.c b/sound/core/sound.c
index b75f78f2c4b8..3039e317df93 100644
--- 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_core_debugfs_root;
+EXPORT_SYMBOL_GPL(sound_core_debugfs_root);
+
 static struct snd_minor *snd_minors[SNDRV_OS_MINORS];
 static DEFINE_MUTEX(sound_mutex);
 
@@ -395,6 +399,9 @@ static int __init alsa_sound_init(void)
 		unregister_chrdev(major, "alsa");
 		return -ENOMEM;
 	}
+
+	sound_core_debugfs_root = debugfs_create_dir("sound-core", NULL);
+
 #ifndef MODULE
 	pr_info("Advanced Linux Sound Architecture Driver Initialized.\n");
 #endif
@@ -403,6 +410,7 @@ static int __init alsa_sound_init(void)
 
 static void __exit alsa_sound_exit(void)
 {
+	debugfs_remove(sound_core_debugfs_root);
 	snd_info_done();
 	unregister_chrdev(major, "alsa");
 }
-- 
2.25.1

Re: [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

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

[Takashi Iwai]

On Thu, 17 Dec 2020 17:45:05 +0100,
Kai Vehmanen wrote:
> 
> 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.

The tree representation looks better than the previous one, IMO.
The exact contents would need more brush up, though; e.g. the content
of each jack could be shown in a debugfs node as well as the
injection.  Or the type and the mask-to-be-injected can be shown
there, too.

> > +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.

Yes, that's my impression, too.  The logic is hard to follow.


> > +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.

Right, but I guess this can be ignored.

Or, as I mentioned in the above, we may expose the current value in
each node instead, and writing a value to this node is treated as
injection.  Then the rest requirement is rather masking from the
hardware update.


thanks,

Takashi

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

[Hui Wang]

On 12/18/20 12:45 AM, Kai Vehmanen wrote:
> 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.
[snip]
> 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.
OK, will try to remove those characters.
>> +static void _snd_jack_report(struct snd_jack *jack, int status, bool from_inject)
>> +{
>> +	struct snd_jack_kctl *jack_kctl;
[snip]
>> +	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.

Got it, will fix it.

Thanks.

>
> Br, Kai

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

[Hui Wang]

On 12/18/20 11:17 PM, Takashi Iwai wrote:
> On Thu, 17 Dec 2020 17:45:05 +0100,
> Kai Vehmanen wrote:
>> 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.
> The tree representation looks better than the previous one, IMO.
> The exact contents would need more brush up, though; e.g. the content
> of each jack could be shown in a debugfs node as well as the
> injection.  Or the type and the mask-to-be-injected can be shown
> there, too.
OK, got it, will add more nodes for a jack, the nodes will bring more 
info of the jack to the userspace.
>
>>> +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.
> Yes, that's my impression, too.  The logic is hard to follow.

I think it is really complicated,  That is my design:

  - If a jack_ctrl's sw_inject is enabled,  the jack_report will only 
report status from injection (block hw events), if it is disabled,  the 
jack_report will only report status from hw events (block injection). 
That is why I have to add a parameter from_inject

  - A snd_jack could contain multi jack_ctrls, the 
snd_jack_report(status) is based on snd_jack instead of jack_ctrls, but 
sw_inject_enable is based on jack_ctrls instead of snd_jack. Suppose a 
snd_jack has 2 jack_ctrls A and B, A's sw_inject is enabled. Suppose 
Jack of A triggers a hw events and snd_jack_report() is called, the 
status should be blocked since A's sw_inject is enabled,  also the 
input_dev's event of this jack_ctrl should be blocked too, I added 
mask_bits |= jack_kctl->mask_bits just for blocking the input-dev's report.

So far, I could not design a cleaner and simpler function to implement 
the idea above.


>
>
>>> +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.
> Right, but I guess this can be ignored.
>
> Or, as I mentioned in the above, we may expose the current value in
> each node instead, and writing a value to this node is treated as
> injection.  Then the rest requirement is rather masking from the
> hardware update.

Also, I could add a hw_status_cache in the snd_jack_kctl{}, and use it 
to implement save-and-restore for the jack's state.

Thanks.

>
>
> thanks,
>
> Takashi