[alsa-devel] [PATCH] ASoC: hdmi: implement component supsend/resume callback

[Takashi Iwai]

On Thu, 28 Mar 2019 09:40:05 +0100,
libin.yang at intel.com wrote:
> 
> From: Libin Yang <libin.yang at intel.com>
> 
> Implement the hdmi codec component driver's suspend/resume callback.
> This can make sure that the dapm event is triggered after the display
> audio power domain is turned on if bus_control is set.
> 
> Also this patch removes the codec device PM suspend/resume.
> 
> Signed-off-by: Libin Yang <libin.yang at intel.com>
> ---
>  sound/soc/codecs/hdac_hdmi.c | 51 +++++++++++++++++++++++---------------------
>  1 file changed, 27 insertions(+), 24 deletions(-)
> 
> diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
> index 5eeb0fe..478c6f2 100644
> --- a/sound/soc/codecs/hdac_hdmi.c
> +++ b/sound/soc/codecs/hdac_hdmi.c
> @@ -1873,36 +1873,27 @@ static void hdmi_codec_remove(struct snd_soc_component *component)
>  	pm_runtime_disable(&hdev->dev);
>  }
>  
> -#ifdef CONFIG_PM_SLEEP
> -static int hdmi_codec_resume(struct device *dev)
> +static int hdmi_component_suspend(struct snd_soc_component *component)
>  {
> -	struct hdac_device *hdev = dev_to_hdac_dev(dev);
> -	struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
> -	int ret;
> +	struct hdac_hdmi_priv *hdmi = snd_soc_component_get_drvdata(component);
> +	struct hdac_device *hdev = hdmi->hdev;
>  
> -	ret = pm_runtime_force_resume(dev);
> -	if (ret < 0)
> -		return ret;
> -	/*
> -	 * As the ELD notify callback request is not entertained while the
> -	 * device is in suspend state. Need to manually check detection of
> -	 * all pins here. pin capablity change is not support, so use the
> -	 * already set pin caps.
> -	 *
> -	 * NOTE: this is safe to call even if the codec doesn't actually resume.
> -	 * The pin check involves only with DRM audio component hooks, so it
> -	 * works even if the HD-audio side is still dreaming peacefully.
> -	 */
> -	hdac_hdmi_present_sense_all_pins(hdev, hdmi, false);
> -	return 0;
> +	return pm_runtime_force_suspend(&hdev->dev);
> +}
> +
> +static int hdmi_component_resume(struct snd_soc_component *component)
> +{
> +	struct hdac_hdmi_priv *hdmi = snd_soc_component_get_drvdata(component);
> +	struct hdac_device *hdev = hdmi->hdev;
> +
> +	return pm_runtime_force_resume(&hdev->dev);
>  }
> -#else
> -#define hdmi_codec_resume NULL
> -#endif
>  
>  static const struct snd_soc_component_driver hdmi_hda_codec = {
>  	.probe			= hdmi_codec_probe,
>  	.remove			= hdmi_codec_remove,
> +	.suspend		= hdmi_component_suspend,
> +	.resume			= hdmi_component_resume,
>  	.use_pmdown_time	= 1,
>  	.endianness		= 1,
>  	.non_legacy_dai_naming	= 1,
> @@ -2104,6 +2095,7 @@ static int hdac_hdmi_runtime_resume(struct device *dev)
>  	struct hdac_device *hdev = dev_to_hdac_dev(dev);
>  	struct hdac_bus *bus = hdev->bus;
>  	struct hdac_ext_link *hlink = NULL;
> +	struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
>  
>  	dev_dbg(dev, "Enter: %s\n", __func__);
>  
> @@ -2128,6 +2120,18 @@ static int hdac_hdmi_runtime_resume(struct device *dev)
>  	snd_hdac_codec_read(hdev, hdev->afg, 0,	AC_VERB_SET_POWER_STATE,
>  							AC_PWRST_D0);
>  
> +	/*
> +	 * As the ELD notify callback request is not entertained while the
> +	 * device is in suspend state. Need to manually check detection of
> +	 * all pins here. pin capablity change is not support, so use the
> +	 * already set pin caps.
> +	 *
> +	 * NOTE: this is safe to call even if the codec doesn't actually resume.
> +	 * The pin check involves only with DRM audio component hooks, so it
> +	 * works even if the HD-audio side is still dreaming peacefully.
> +	 */
> +	hdac_hdmi_present_sense_all_pins(hdev, hdmi, false);

Do we need to move this into runtime_resume?  The forced detection is
needed basically only for the system resume, i.e. the detection is
performed while the system is running.

Other than that, one thing I overlooked is that this approach might
hit another race when another problem is calling a path that includes
pm_runtime_get*().  Since the whole ASoC resume is done in an async
work, this would conflict.  That's rather a fundamental bug in ASoC
resume framework, though...

So, I'm interested in whether another approach really works: namely
defining the PM dependency via device_link_add().  This should be
safer.


thanks,

Takashi