Re: [PATCH v3 4/4] ALSA: hda - Move audio component accesses to hdac_i915.c

[Vinod Koul <vinod.koul@intel.com>]

On Fri, Dec 04, 2015 at 06:12:49PM +0100, Takashi Iwai wrote:
> A couple of i915_audio_component ops have been added and accessed
> directly from patch_hdmi.c.  Ideally all these should be factored out
> into hdac_i915.c.
> 
> This patch does it, adds two new helper functions for setting N/CTS
> and fetching ELD bytes.  One bonus is that the hackish widget vs port
> mapping is also moved to hdac_i915.c, so that it can be fixed /
> enhanced more cleanly.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

Reviewed-by: Vinod Koul <vinod.koul@intel.com>

-- 
~Vinod
> ---
>  include/sound/hda_i915.h   | 14 ++++++++++
>  sound/hda/hdac_i915.c      | 66 ++++++++++++++++++++++++++++++++++++++++++++
>  sound/pci/hda/patch_hdmi.c | 69 +++++++++++++++++-----------------------------
>  3 files changed, 106 insertions(+), 43 deletions(-)
> 
> diff --git a/include/sound/hda_i915.h b/include/sound/hda_i915.h
> index 930b41e5acf4..fa341fcb5829 100644
> --- a/include/sound/hda_i915.h
> +++ b/include/sound/hda_i915.h
> @@ -10,6 +10,9 @@
>  int snd_hdac_set_codec_wakeup(struct hdac_bus *bus, bool enable);
>  int snd_hdac_display_power(struct hdac_bus *bus, bool enable);
>  int snd_hdac_get_display_clk(struct hdac_bus *bus);
> +int snd_hdac_sync_audio_rate(struct hdac_bus *bus, hda_nid_t nid, int rate);
> +int snd_hdac_acomp_get_eld(struct hdac_bus *bus, hda_nid_t nid,
> +			   bool *audio_enabled, char *buffer, int max_bytes);
>  int snd_hdac_i915_init(struct hdac_bus *bus);
>  int snd_hdac_i915_exit(struct hdac_bus *bus);
>  int snd_hdac_i915_register_notifier(const struct i915_audio_component_audio_ops *);
> @@ -26,6 +29,17 @@ static inline int snd_hdac_get_display_clk(struct hdac_bus *bus)
>  {
>  	return 0;
>  }
> +static inline int snd_hdac_sync_audio_rate(struct hdac_bus *bus, hda_nid_t nid,
> +					   int rate)
> +{
> +	return 0;
> +}
> +static inline int snd_hdac_acomp_get_eld(struct hdac_bus *bus, hda_nid_t nid,
> +					 bool *audio_enabled, char *buffer,
> +					 int max_bytes)
> +{
> +	return -ENODEV;
> +}
>  static inline int snd_hdac_i915_init(struct hdac_bus *bus)
>  {
>  	return -ENODEV;
> diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
> index 8fef1b8d1fd8..c50177fb469f 100644
> --- a/sound/hda/hdac_i915.c
> +++ b/sound/hda/hdac_i915.c
> @@ -118,6 +118,72 @@ int snd_hdac_get_display_clk(struct hdac_bus *bus)
>  }
>  EXPORT_SYMBOL_GPL(snd_hdac_get_display_clk);
>  
> +/* There is a fixed mapping between audio pin node and display port
> + * on current Intel platforms:
> + * Pin Widget 5 - PORT B (port = 1 in i915 driver)
> + * Pin Widget 6 - PORT C (port = 2 in i915 driver)
> + * Pin Widget 7 - PORT D (port = 3 in i915 driver)
> + */
> +static int pin2port(hda_nid_t pin_nid)
> +{
> +	return pin_nid - 4;
> +}
> +
> +/**
> + * snd_hdac_sync_audio_rate - Set N/CTS based on the sample rate
> + * @bus: HDA core bus
> + * @nid: the pin widget NID
> + * @rate: the sample rate to set
> + *
> + * This function is supposed to be used only by a HD-audio controller
> + * driver that needs the interaction with i915 graphics.
> + *
> + * This function sets N/CTS value based on the given sample rate.
> + * Returns zero for success, or a negative error code.
> + */
> +int snd_hdac_sync_audio_rate(struct hdac_bus *bus, hda_nid_t nid, int rate)
> +{
> +	struct i915_audio_component *acomp = bus->audio_component;
> +
> +	if (!acomp || !acomp->ops || !acomp->ops->sync_audio_rate)
> +		return -ENODEV;
> +	return acomp->ops->sync_audio_rate(acomp->dev, pin2port(nid), rate);
> +}
> +EXPORT_SYMBOL_GPL(snd_hdac_sync_audio_rate);
> +
> +/**
> + * snd_hdac_acomp_get_eld - Get the audio state and ELD via component
> + * @bus: HDA core bus
> + * @nid: the pin widget NID
> + * @audio_enabled: the pointer to store the current audio state
> + * @buffer: the buffer pointer to store ELD bytes
> + * @max_bytes: the max bytes to be stored on @buffer
> + *
> + * This function is supposed to be used only by a HD-audio controller
> + * driver that needs the interaction with i915 graphics.
> + *
> + * This function queries the current state of the audio on the given
> + * digital port and fetches the ELD bytes onto the given buffer.
> + * It returns the number of bytes for the total ELD data, zero for
> + * invalid ELD, or a negative error code.
> + *
> + * The return size is the total bytes required for the whole ELD bytes,
> + * thus it may be over @max_bytes.  If it's over @max_bytes, it implies
> + * that only a part of ELD bytes have been fetched.
> + */
> +int snd_hdac_acomp_get_eld(struct hdac_bus *bus, hda_nid_t nid,
> +			   bool *audio_enabled, char *buffer, int max_bytes)
> +{
> +	struct i915_audio_component *acomp = bus->audio_component;
> +
> +	if (!acomp || !acomp->ops || !acomp->ops->get_eld)
> +		return -ENODEV;
> +
> +	return acomp->ops->get_eld(acomp->dev, pin2port(nid), audio_enabled,
> +				   buffer, max_bytes);
> +}
> +EXPORT_SYMBOL_GPL(snd_hdac_acomp_get_eld);
> +
>  static int hdac_component_master_bind(struct device *dev)
>  {
>  	struct i915_audio_component *acomp = hdac_acomp;
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index 61f004a73590..1ef0c6959e75 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -1438,17 +1438,6 @@ static void intel_not_share_assigned_cvt(struct hda_codec *codec,
>  	}
>  }
>  
> -/* There is a fixed mapping between audio pin node and display port
> - * on current Intel platforms:
> - * Pin Widget 5 - PORT B (port = 1 in i915 driver)
> - * Pin Widget 6 - PORT C (port = 2 in i915 driver)
> - * Pin Widget 7 - PORT D (port = 3 in i915 driver)
> - */
> -static int intel_pin2port(hda_nid_t pin_nid)
> -{
> -	return pin_nid - 4;
> -}
> -
>  /*
>   * HDA PCM callbacks
>   */
> @@ -1660,38 +1649,36 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
>  static void sync_eld_via_acomp(struct hda_codec *codec,
>  			       struct hdmi_spec_per_pin *per_pin)
>  {
> -	struct i915_audio_component *acomp = codec->bus->core.audio_component;
>  	struct hdmi_spec *spec = codec->spec;
>  	struct hdmi_eld *eld = &spec->temp_eld;
>  	int size;
>  
> -	if (acomp && acomp->ops && acomp->ops->get_eld) {
> -		mutex_lock(&per_pin->lock);
> -		size = acomp->ops->get_eld(acomp->dev,
> -					   intel_pin2port(per_pin->pin_nid),
> -					   &eld->monitor_present,
> -					   eld->eld_buffer,
> -					   ELD_MAX_SIZE);
> -		if (size > 0) {
> -			size = min(size, ELD_MAX_SIZE);
> -			if (snd_hdmi_parse_eld(codec, &eld->info,
> -					       eld->eld_buffer, size) < 0)
> -				size = -EINVAL;
> -		}
> -
> -		if (size > 0) {
> -			eld->eld_valid = true;
> -			eld->eld_size = size;
> -		} else {
> -			eld->eld_valid = false;
> -			eld->eld_size = 0;
> -		}
> -
> -		update_eld(codec, per_pin, eld);
> -		snd_jack_report(per_pin->acomp_jack,
> -				eld->monitor_present ? SND_JACK_AVOUT : 0);
> -		mutex_unlock(&per_pin->lock);
> +	mutex_lock(&per_pin->lock);
> +	size = snd_hdac_acomp_get_eld(&codec->bus->core, per_pin->pin_nid,
> +				      &eld->monitor_present, eld->eld_buffer,
> +				      ELD_MAX_SIZE);
> +	if (size < 0)
> +		goto unlock;
> +	if (size > 0) {
> +		size = min(size, ELD_MAX_SIZE);
> +		if (snd_hdmi_parse_eld(codec, &eld->info,
> +				       eld->eld_buffer, size) < 0)
> +			size = -EINVAL;
> +	}
> +
> +	if (size > 0) {
> +		eld->eld_valid = true;
> +		eld->eld_size = size;
> +	} else {
> +		eld->eld_valid = false;
> +		eld->eld_size = 0;
>  	}
> +
> +	update_eld(codec, per_pin, eld);
> +	snd_jack_report(per_pin->acomp_jack,
> +			eld->monitor_present ? SND_JACK_AVOUT : 0);
> + unlock:
> +	mutex_unlock(&per_pin->lock);
>  }
>  
>  static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
> @@ -1857,7 +1844,6 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
>  	struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
>  	hda_nid_t pin_nid = per_pin->pin_nid;
>  	struct snd_pcm_runtime *runtime = substream->runtime;
> -	struct i915_audio_component *acomp = codec->bus->core.audio_component;
>  	bool non_pcm;
>  	int pinctl;
>  
> @@ -1876,10 +1862,7 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
>  
>  	/* Call sync_audio_rate to set the N/CTS/M manually if necessary */
>  	/* Todo: add DP1.2 MST audio support later */
> -	if (acomp && acomp->ops && acomp->ops->sync_audio_rate)
> -		acomp->ops->sync_audio_rate(acomp->dev,
> -				intel_pin2port(pin_nid),
> -				runtime->rate);
> +	snd_hdac_sync_audio_rate(&codec->bus->core, pin_nid, runtime->rate);
>  
>  	non_pcm = check_non_pcm_per_cvt(codec, cvt_nid);
>  	mutex_lock(&per_pin->lock);
> -- 
> 2.6.3
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx