On Thu, 28 Mar 2019 14:52:19 +0100, Yang, Libin wrote: > > 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? Yes, that'll make more sense, although keeping it in runtime resume would be relatively harmless in practice. > >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. Thanks! Takashi _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
