[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