Hi Takashi, >-----Original Message----- >From: Takashi Iwai [mailto:tiwai@xxxxxxx] >Sent: Thursday, March 28, 2019 5:23 PM >To: Yang, Libin <libin.yang@xxxxxxxxx> >Cc: alsa-devel@xxxxxxxxxxxxxxxx; broonie@xxxxxxxxxx >Subject: Re: [PATCH] ASoC: hdmi: implement component >supsend/resume callback > >On Thu, 28 Mar 2019 09:40:05 +0100, >libin.yang@xxxxxxxxx wrote: >> >> From: Libin Yang <libin.yang@xxxxxxxxx> >> >> 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@xxxxxxxxx> >> --- >> 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. Right. So move it into component resume? > >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. OK. I will try to use device_link_add() and have a test tomorrow. Regards, Libin > > >thanks, > >Takashi _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
