On Fri, 25 Mar 2016 03:26:06 +0100, Yang, Libin wrote:
Hi Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Thursday, March 24, 2016 5:19 PM To: Yang, Libin Cc: libin.yang@linux.intel.com; alsa-devel@alsa-project.org; Lin, Mengdong Subject: Re: [alsa-devel] [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@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 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