On Fri, 25 Mar 2016 03:26:06 +0100, Yang, Libin wrote: > > Hi Takashi, > > > -----Original Message----- > > From: Takashi Iwai [mailto:tiwai@xxxxxxx] > > Sent: Thursday, March 24, 2016 5:19 PM > > To: Yang, Libin > > Cc: libin.yang@xxxxxxxxxxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx; Lin, > > Mengdong > > Subject: Re: [RFC PATCH v2 1/3] ALSA: hda - add DP mst verb > > support > > > > On Thu, 24 Mar 2016 09:44:32 +0100, > > Yang, Libin wrote: > > > > > > Hi Takashi, > > > > > > > -----Original Message----- > > > > From: Takashi Iwai [mailto:tiwai@xxxxxxx] > > > > Sent: Tuesday, March 22, 2016 7:17 PM > > > > To: libin.yang@xxxxxxxxxxxxxxx > > > > Cc: alsa-devel@xxxxxxxxxxxxxxxx; Lin, Mengdong; Yang, Libin > > > > Subject: Re: [RFC PATCH v2 1/3] ALSA: hda - add DP mst > > verb > > > > support > > > > > > > > On Mon, 21 Mar 2016 09:37:54 +0100, > > > > libin.yang@xxxxxxxxxxxxxxx wrote: > > > > > > > > > > From: Libin Yang <libin.yang@xxxxxxxxxxxxxxx> > > > > > > > > > > Add snd_hda_get_dev_select() and snd_hda_set_dev_select() > > > > functions > > > > > for DP MST audio support. > > > > > > > > > > Signed-off-by: Libin Yang <libin.yang@xxxxxxxxxxxxxxx> > > > > > --- > > > > > sound/pci/hda/hda_codec.c | 83 > > > > ++++++++++++++++++++++++++++++++++++++++++++--- > > > > > sound/pci/hda/hda_codec.h | 3 ++ > > > > > 2 files changed, 82 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/sound/pci/hda/hda_codec.c > > b/sound/pci/hda/hda_codec.c > > > > > index d17e38e..db94a66 100644 > > > > > --- a/sound/pci/hda/hda_codec.c > > > > > +++ b/sound/pci/hda/hda_codec.c > > > > > @@ -325,9 +325,16 @@ int snd_hda_get_conn_index(struct > > > > hda_codec *codec, hda_nid_t mux, > > > > > } > > > > > EXPORT_SYMBOL_GPL(snd_hda_get_conn_index); > > > > > > > > > > - > > > > > -/* return DEVLIST_LEN parameter of the given widget */ > > > > > -static unsigned int get_num_devices(struct hda_codec *codec, > > > > hda_nid_t nid) > > > > > +/** > > > > > + * snd_hda_get_num_devices - get DEVLIST_LEN parameter of the > > > > given widget > > > > > + * @codec: the HDA codec > > > > > + * @nid: NID of the pin to parse > > > > > + * > > > > > + * Get the device entry number on the given widget. > > > > > + * This is a feature of DP MST audio. Each pin can > > > > > + * have several device entries in it. > > > > > + */ > > > > > +unsigned int snd_hda_get_num_devices(struct hda_codec *codec, > > > > hda_nid_t nid) > > > > > { > > > > > unsigned int wcaps = get_wcaps(codec, nid); > > > > > unsigned int parm; > > > > > @@ -341,6 +348,7 @@ static unsigned int get_num_devices(struct > > > > hda_codec *codec, hda_nid_t nid) > > > > > parm = 0; > > > > > return parm & AC_DEV_LIST_LEN_MASK; > > > > > } > > > > > +EXPORT_SYMBOL_GPL(snd_hda_get_num_devices); > > > > > > > > > > /** > > > > > * snd_hda_get_devices - copy device list without cache > > > > > @@ -358,7 +366,7 @@ int snd_hda_get_devices(struct hda_codec > > > > *codec, hda_nid_t nid, > > > > > unsigned int parm; > > > > > int i, dev_len, devices; > > > > > > > > > > - parm = get_num_devices(codec, nid); > > > > > + parm = snd_hda_get_num_devices(codec, nid); > > > > > if (!parm) /* not multi-stream capable */ > > > > > return 0; > > > > > > > > > > @@ -382,6 +390,73 @@ int snd_hda_get_devices(struct > > hda_codec > > > > *codec, hda_nid_t nid, > > > > > return devices; > > > > > } > > > > > > > > > > +/** > > > > > + * snd_hda_get_dev_select - get device entry select on the pin > > > > > + * @codec: the HDA codec > > > > > + * @nid: NID of the pin to get device entry select > > > > > + * > > > > > + * Get the devcie entry select on the pin. Return the device entry > > > > > + * id selected on the pin. Return 0 means the first device entry > > > > > + * is selected or MST is not supported. > > > > > + */ > > > > > +int snd_hda_get_dev_select(struct hda_codec *codec, hda_nid_t > > nid) > > > > > +{ > > > > > + /* not support dp_mst will always return 0, using first dev_entry > > > > */ > > > > > + if (!codec->dp_mst) > > > > > + return 0; > > > > > + > > > > > + return snd_hda_codec_read(codec, nid, 0, > > > > AC_VERB_GET_DEVICE_SEL, 0); > > > > > +} > > > > > +EXPORT_SYMBOL_GPL(snd_hda_get_dev_select); > > > > > + > > > > > +/** > > > > > + * snd_hda_set_dev_select - set device entry select on the pin > > > > > + * @codec: the HDA codec > > > > > + * @nid: NID of the pin to set device entry select > > > > > + * @dev_id: device entry id to be set > > > > > + * > > > > > + * Set the device entry select on the pin nid. > > > > > + */ > > > > > +int snd_hda_set_dev_select(struct hda_codec *codec, hda_nid_t > > nid, > > > > int dev_id) > > > > > +{ > > > > > + int ret, dev, num_devices; > > > > > + unsigned long timeout; > > > > > + > > > > > + /* not support dp_mst will always return 0, using first dev_entry > > > > */ > > > > > + if (!codec->dp_mst) > > > > > + return 0; > > > > > + > > > > > + /* AC_PAR_DEVLIST_LEN is 0 based. */ > > > > > + num_devices = snd_hda_get_num_devices(codec, nid); > > > > > + /* If Device List Length is 0, the pin is not multi stream capable. > > > > > + */ > > > > > + if (num_devices == 0) > > > > > + return 0; > > > > > + > > > > > + /* Behavior of setting index being equal to or greater than > > > > > + * Device List Length is not predictable > > > > > + */ > > > > > + if (num_devices + 1 <= dev_id) > > > > > + return 0; > > > > > > > > Is this correct? Doesn't num_devices=1 mean that there is only a > > > > single device? If dev_id=1 must be invalid, but the check above > > > > passes. The standard form of such index check is like > > > > > > num_device is the return value of the verb AC_PAR_DEVLIST_LEN. > > > device number is AC_PAR_DEVLIST_LEN + 1. > > > > > > >From the spec: > > > Device List Length is a 0 based integer value indicating the number > > > of sink device that a multi stream capable Digital Display Pin > > > Widget can support. If Device List Length is value is 0, there is > > > only one sink device connection possible indicating the Pin Widget > > > is not multi stream capable, and there is no Device Select control > > > > > > And dev_id is 0 based. > > > > > > If dev_id = 1, it means set the second device entry, and > > > num_device + 1 (the real number of device entries) > > > must be equal to or greater than 2. > > > (num_device + 1 > dev_id ) > > > > > > So (num_device + 1 <= dev_id) is wrong situation. > > > > If so, the term "num_devices" is counter-intuitive. It should be > > corrected to the natural value, i.e. based on 1, or renamed to a > > different one. Readers would assume that the number of devices is 1 > > if there is a single element, not 0. If this isn't true, it just > > confuses readers. > > Get it. So what do you think if using: > > if (dev_id > num_device) //both is 0 based. num_device is still a confusing name. If you really want to keep it zero-based, use max_device_id or such. Or, just return +1 from *_get_num_devices() instead. Takashi _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
