On Wed, 09 Jan 2019 09:29:36 +0100, Takashi Iwai wrote:
On Wed, 09 Jan 2019 09:16:34 +0100, Yang, Libin wrote:
-static void hdmi_codec_complete(struct device *dev) +#ifdef CONFIG_PM_SLEEP +static int hdmi_codec_resume(struct device *dev) { struct hdac_device *hdev = dev_to_hdac_dev(dev); struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
- int ret;
- /* Power up afg */
- snd_hdac_codec_read(hdev, hdev->afg, 0, AC_VERB_SET_POWER_STATE,
AC_PWRST_D0);- hdac_hdmi_skl_enable_all_pins(hdev);
- hdac_hdmi_skl_enable_dp12(hdev);
- ret = pm_runtime_force_resume(dev);
The code hopes to call hdmi pm_runtime() whenever this resume() is called? If so, I'm afraid it will not work. pm_runtime_force_resume() calls pm_runtime with conditions. Sometimes pm_runtime will not be called. In my suspend/resume test, it shows that pm_runtime() is not called. This may cause the following function hdac_hdmi_present_sense_all_pins() not work well as there is no power on.
Hm, right, maybe we should move hdac_hdmi_present_sense_all_pins() into runtime resume. It may be called unconditionally, but safer to call it conditionally only after S3 or so...
Now reading back the code again, no, it's correct to call hdac_hdmi_present_sense_all_pins() there. The function doesn't access any HD-audio codec verbs but only with the i915 audio component. Hence it's safe to call there no matter whether the device is opened (thus resumed) or not. And, it is even mandatory to call there; if a HDMI connection changed during suspend, we need to notify it right after the system resume.
thanks,
Takashi