[alsa-devel] [PATCH v2 2/5] ALSA: hda - Add DP-MST jack support

[Takashi Iwai]

On Tue, 19 Nov 2019 05:45:26 +0100,
Nikhil Mahale wrote:
> 
> On 11/15/19 6:42 PM, Takashi Iwai wrote:
> > On Fri, 15 Nov 2019 10:56:03 +0100,
> > Nikhil Mahale wrote:
> >>
> >>  static struct hda_jack_tbl *
> >> -snd_hda_jack_tbl_new(struct hda_codec *codec, hda_nid_t nid)
> >> +snd_hda_jack_tbl_new(struct hda_codec *codec, hda_nid_t nid, int dev_id)
> >>  {
> >> -	struct hda_jack_tbl *jack = snd_hda_jack_tbl_get(codec, nid);
> >> +	struct hda_jack_tbl *jack =
> >> +		snd_hda_jack_tbl_get_mst(codec, nid, dev_id);
> >> +	struct hda_jack_tbl *existing_nid_jack =
> >> +		any_jack_tbl_get_from_nid(codec, nid);
> >> +
> >> +	WARN_ON(dev_id != 0 && !codec->dp_mst);
> >> +
> >>  	if (jack)
> >>  		return jack;
> >>  	jack = snd_array_new(&codec->jacktbl);
> >>  	if (!jack)
> >>  		return NULL;
> >>  	jack->nid = nid;
> >> +	jack->dev_id = dev_id;
> >>  	jack->jack_dirty = 1;
> >> -	jack->tag = codec->jacktbl.used;
> >> +	if (!existing_nid_jack)
> >> +		jack->tag = codec->jacktbl.used;
> >> +	else
> >> +		jack->tag = existing_nid_jack->tag;
> >> +
> >>  	return jack;
> >>  }
> > 
> > In this logic, we assign the same tag to multiple jack objects, and
> > this will lead to the multiple calls of SET_UNSOLICITED_ENABLE on the
> > same pin.  But this should be only once at parsing, and the rest
> > resume init will be done from the codec regmap cache, so it's not too
> > bad, I guess.
> > 
> > Alternatively we can set 0 to jack->tag when existing_nid_jack!=NULL,
> > too, and skip SET_UNSOLICITED_ENABLE for tag==0.  But this will make
> > *_get_from_tag() won't work as you intended, hence the step will be
> > - get the jack once via snd_hda_jack_tbl_get_from_tag(tag)
> > - get the jack again with snd_hda_jack_tbl_get_mst(jack->nid, dev_id)
> > But it's more complex, and not better, either.
> 
> I am bit skeptical over this idea. One can build hypothetical case like - you create 2 jack objects, first one for (nid=1, dev_id=0) with jack->tag!=0 and second one for (nid=1, dev_id=1) with jack->tag==0. If you want free first jack object and continue working with second one then that would create problem because you lose tag with destruction of first object?
> 
> If objective is not to make multiple calls of SET_UNSOLICITED_ENABLE on the same pin, then just like jack->tag we can inherit jack->jack_detect from  existing_nid_jack. That can make sure snd_hda_jack_detect_enable_callback_mst() not to do multiple calls of SET_UNSOLICITED_ENABLE on the same pin, right?
> 
> I am suggesting code change like -
> 
> ...
> snd_hda_jack_tbl_new(...)
> {
>     [...]
>     if (!existing_nid_jack) {
>         jack->tag = codec->jacktbl.used;
>     } else {
>         jack->tag = existing_nid_jack->tag;
>         jack->jack_detect = existing_nid_jack->jack_detect;
>     }
>     [...]
> }

Yes, this looks like a good idea.


thanks,

Takashi