Hi Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, March 22, 2016 7:17 PM To: libin.yang@linux.intel.com Cc: alsa-devel@alsa-project.org; Lin, Mengdong; Yang, Libin Subject: Re: [alsa-devel] [RFC PATCH v2 1/3] ALSA: hda - add DP mst verb support
On Mon, 21 Mar 2016 09:37:54 +0100, libin.yang@linux.intel.com wrote:
From: Libin Yang libin.yang@linux.intel.com
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@linux.intel.com
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 (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.
- 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.
Regards, Libin
Takashi