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