[alsa-devel] [RFC PATCH v2 1/3] ALSA: hda - add DP mst verb support

[Takashi Iwai]

On Mon, 21 Mar 2016 09:37:54 +0100,
libin.yang at linux.intel.com wrote:
> 
> From: Libin Yang <libin.yang at 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 at 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

	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?

> +
> +	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.


Takashi