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. > > > > if (dev_id >= num_devices) > > error; > > > > And this also leads to the question. Shouldn't we return an error if > > an invalid dev_id is given? > > I'm thinking if the wrong dev_id is given, we just leave as it is before. > I will return the -EINVAL in next version. IMO, it's safer to return an error. Since non-zero dev_id is only for MST, the code path that passes such a value must handle MST properly, i.e. not through the default code path. > > > + > > > + timeout = jiffies + msecs_to_jiffies(500); > > > + > > > + /* make sure the device entry selection takes effect */ > > > + do { > > > + ret = snd_hda_codec_write(codec, nid, 0, > > > + AC_VERB_SET_DEVICE_SEL, dev_id); > > > + udelay(20); > > > + > > > + dev = snd_hda_get_dev_select(codec, nid); > > > + if (dev == dev_id) > > > + break; > > > + msleep(20); > > > + } while (time_before(jiffies, timeout)); > > > > Is this loop needed? Most verbs take effect immediately, and so far, > > the only exception is the power state. > > OK, I will remove the loop. Well, I don't mean to remove forcibly. Just want to be sure. If such a loop is needed practically, we must have it, of course. Let's check. thanks, Takashi _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
