| Message ID | 1555036832-2850-1-git-send-email-libin.yang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: codec: hdac_hdmi add device_link to card device | expand |
On 4/11/19 9:40 PM, libin.yang@intel.com wrote: > From: Libin Yang <libin.yang@intel.com> > > In resume from S3, HDAC HDMI codec driver dapm event callback may be > operated before HDMI codec driver turns on the display audio power > domain because of the contest between display driver and hdmi codec driver. > > This patch adds the device_link between soc card device (consumer) and > hdmi codec device (supplier) to make sure the sequence is always correct. > > Signed-off-by: Libin Yang <libin.yang@intel.com> > --- > sound/soc/codecs/hdac_hdmi.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c > index 5eeb0fe..8bb883e 100644 > --- a/sound/soc/codecs/hdac_hdmi.c > +++ b/sound/soc/codecs/hdac_hdmi.c > @@ -1855,6 +1855,15 @@ static int hdmi_codec_probe(struct snd_soc_component *component) > hdmi->card = dapm->card->snd_card; > > /* > + * Setup a device_link between card device and HDMI codec device. > + * The card device is the consumer and the HDMI codec device is > + * the supplier. With this setting, we can make sure that the audio > + * domain in display power will be always turned on before operating > + * on the HDMI audio codec registers. > + */ > + device_link_add(component->card->dev, &hdev->dev, DL_FLAG_RPM_ACTIVE | > + DL_FLAG_AUTOREMOVE_CONSUMER); Should device_link_free() be added to hdmi_codec_remove then? Thanks!
Hi Pierre, >-----Original Message----- >From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com] >Sent: Friday, April 12, 2019 11:00 AM >To: Yang, Libin <libin.yang@intel.com>; alsa-devel@alsa-project.org; >tiwai@suse.de; broonie@kernel.org >Subject: Re: [alsa-devel] [PATCH] ASoC: codec: hdac_hdmi add device_link to >card device > >On 4/11/19 9:40 PM, libin.yang@intel.com wrote: >> From: Libin Yang <libin.yang@intel.com> >> >> In resume from S3, HDAC HDMI codec driver dapm event callback may be >> operated before HDMI codec driver turns on the display audio power >> domain because of the contest between display driver and hdmi codec >driver. >> >> This patch adds the device_link between soc card device (consumer) and >> hdmi codec device (supplier) to make sure the sequence is always correct. >> >> Signed-off-by: Libin Yang <libin.yang@intel.com> >> --- >> sound/soc/codecs/hdac_hdmi.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/sound/soc/codecs/hdac_hdmi.c >> b/sound/soc/codecs/hdac_hdmi.c index 5eeb0fe..8bb883e 100644 >> --- a/sound/soc/codecs/hdac_hdmi.c >> +++ b/sound/soc/codecs/hdac_hdmi.c >> @@ -1855,6 +1855,15 @@ static int hdmi_codec_probe(struct >snd_soc_component *component) >> hdmi->card = dapm->card->snd_card; >> >> /* >> + * Setup a device_link between card device and HDMI codec device. >> + * The card device is the consumer and the HDMI codec device is >> + * the supplier. With this setting, we can make sure that the audio >> + * domain in display power will be always turned on before operating >> + * on the HDMI audio codec registers. >> + */ >> + device_link_add(component->card->dev, &hdev->dev, >DL_FLAG_RPM_ACTIVE | >> + DL_FLAG_AUTOREMOVE_CONSUMER); > >Should device_link_free() be added to hdmi_codec_remove then? As Takashi suggested, I add the DL_FLAG_AUTOREMOVE_CONSUMER flag. This will make sure the link will be freed when machine driver are removed. And as machine driver depends on the hdac_hdmi module, when hdmi_codec_remove() is called, the link is freed already. Regards, Libin >Thanks!
>>> + device_link_add(component->card->dev, &hdev->dev, >> DL_FLAG_RPM_ACTIVE | >>> + DL_FLAG_AUTOREMOVE_CONSUMER); >> >> Should device_link_free() be added to hdmi_codec_remove then? > > As Takashi suggested, I add the DL_FLAG_AUTOREMOVE_CONSUMER flag. > This will make sure the link will be freed when machine driver are removed. > And as machine driver depends on the hdac_hdmi module, when > hdmi_codec_remove() is called, the link is freed already. ok, maybe adding a comment would help dummies like me who didn't know about this flag? Thanks!
Hi Pierre, >-----Original Message----- >From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com] >Sent: Friday, April 12, 2019 9:52 PM >To: Yang, Libin <libin.yang@intel.com>; alsa-devel@alsa-project.org; >tiwai@suse.de; broonie@kernel.org >Subject: Re: [alsa-devel] [PATCH] ASoC: codec: hdac_hdmi add device_link to >card device > > >>>> + device_link_add(component->card->dev, &hdev->dev, >>> DL_FLAG_RPM_ACTIVE | >>>> + DL_FLAG_AUTOREMOVE_CONSUMER); >>> >>> Should device_link_free() be added to hdmi_codec_remove then? >> >> As Takashi suggested, I add the DL_FLAG_AUTOREMOVE_CONSUMER flag. >> This will make sure the link will be freed when machine driver are removed. >> And as machine driver depends on the hdac_hdmi module, when >> hdmi_codec_remove() is called, the link is freed already. > >ok, maybe adding a comment would help dummies like me who didn't know >about this flag? Thanks! Thanks for suggestion. I will add the comment. Regards, Libin
>> >>>>> + device_link_add(component->card->dev, &hdev->dev, >>>> DL_FLAG_RPM_ACTIVE | >>>>> + DL_FLAG_AUTOREMOVE_CONSUMER); >>>> >>>> Should device_link_free() be added to hdmi_codec_remove then? >>> >>> As Takashi suggested, I add the DL_FLAG_AUTOREMOVE_CONSUMER flag. >>> This will make sure the link will be freed when machine driver are removed. >>> And as machine driver depends on the hdac_hdmi module, when >>> hdmi_codec_remove() is called, the link is freed already. >> >>ok, maybe adding a comment would help dummies like me who didn't know >>about this flag? Thanks! > >Thanks for suggestion. I will add the comment. After a second thought, is there any possibility that the machine driver does not depend on the hdac_hdmi module? I checked our current driver, there is the dependency between machine driver and hdac_hdmi module. Regards, Libin
On 4/12/19 9:33 AM, Yang, Libin wrote: >>> >>>>>> + device_link_add(component->card->dev, &hdev->dev, >>>>> DL_FLAG_RPM_ACTIVE | >>>>>> + DL_FLAG_AUTOREMOVE_CONSUMER); >>>>> >>>>> Should device_link_free() be added to hdmi_codec_remove then? >>>> >>>> As Takashi suggested, I add the DL_FLAG_AUTOREMOVE_CONSUMER flag. >>>> This will make sure the link will be freed when machine driver are removed. >>>> And as machine driver depends on the hdac_hdmi module, when >>>> hdmi_codec_remove() is called, the link is freed already. >>> >>> ok, maybe adding a comment would help dummies like me who didn't know >>> about this flag? Thanks! >> >> Thanks for suggestion. I will add the comment. > > After a second thought, is there any possibility that the machine driver does > not depend on the hdac_hdmi module? I checked our current driver, > there is the dependency between machine driver and hdac_hdmi module. I don't get your question. All machine drivers supporting the iDISP link explicitly use a select HDAC_HDMI in the Kconfig and refer to DAIs created/exposed by the hdac_hdmi module. It's possible that the hdac_hdmi codec is probed as a result of the platform driver configuration but the machine driver does not support HDMI. In that case, is there any issue with your solution?
Hi Pierre, >-----Original Message----- >From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com] >Sent: Friday, April 12, 2019 11:03 PM >To: Yang, Libin <libin.yang@intel.com>; alsa-devel@alsa-project.org; >tiwai@suse.de; broonie@kernel.org >Subject: Re: [alsa-devel] [PATCH] ASoC: codec: hdac_hdmi add device_link to >card device > > > >On 4/12/19 9:33 AM, Yang, Libin wrote: >>>> >>>>>>> + device_link_add(component->card->dev, &hdev->dev, >>>>>> DL_FLAG_RPM_ACTIVE | >>>>>>> + DL_FLAG_AUTOREMOVE_CONSUMER); >>>>>> >>>>>> Should device_link_free() be added to hdmi_codec_remove then? >>>>> >>>>> As Takashi suggested, I add the DL_FLAG_AUTOREMOVE_CONSUMER >flag. >>>>> This will make sure the link will be freed when machine driver are >removed. >>>>> And as machine driver depends on the hdac_hdmi module, when >>>>> hdmi_codec_remove() is called, the link is freed already. >>>> >>>> ok, maybe adding a comment would help dummies like me who didn't >>>> know about this flag? Thanks! >>> >>> Thanks for suggestion. I will add the comment. >> >> After a second thought, is there any possibility that the machine >> driver does not depend on the hdac_hdmi module? I checked our current >> driver, there is the dependency between machine driver and hdac_hdmi >module. > >I don't get your question. >All machine drivers supporting the iDISP link explicitly use a select >HDAC_HDMI in the Kconfig and refer to DAIs created/exposed by the >hdac_hdmi module. So the dependency should always exists. I have checked all the machine drivers and they all depends on the hdac_hdmi if they are using it. >It's possible that the hdac_hdmi codec is probed as a result of the platform >driver configuration but the machine driver does not support HDMI. In that >case, is there any issue with your solution? If machine driver doesn't support HDMI, component will never be called. It is safe. Hi Takashi, My patch doesn't use component->init() as you suggested. But they should be similar. Are you OK with this patch? Regards, Libin
On Sat, 13 Apr 2019 01:00:38 +0200, Yang, Libin wrote: > > Hi Pierre, > > >-----Original Message----- > >From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com] > >Sent: Friday, April 12, 2019 11:03 PM > >To: Yang, Libin <libin.yang@intel.com>; alsa-devel@alsa-project.org; > >tiwai@suse.de; broonie@kernel.org > >Subject: Re: [alsa-devel] [PATCH] ASoC: codec: hdac_hdmi add device_link to > >card device > > > > > > > >On 4/12/19 9:33 AM, Yang, Libin wrote: > >>>> > >>>>>>> + device_link_add(component->card->dev, &hdev->dev, > >>>>>> DL_FLAG_RPM_ACTIVE | > >>>>>>> + DL_FLAG_AUTOREMOVE_CONSUMER); > >>>>>> > >>>>>> Should device_link_free() be added to hdmi_codec_remove then? > >>>>> > >>>>> As Takashi suggested, I add the DL_FLAG_AUTOREMOVE_CONSUMER > >flag. > >>>>> This will make sure the link will be freed when machine driver are > >removed. > >>>>> And as machine driver depends on the hdac_hdmi module, when > >>>>> hdmi_codec_remove() is called, the link is freed already. > >>>> > >>>> ok, maybe adding a comment would help dummies like me who didn't > >>>> know about this flag? Thanks! > >>> > >>> Thanks for suggestion. I will add the comment. > >> > >> After a second thought, is there any possibility that the machine > >> driver does not depend on the hdac_hdmi module? I checked our current > >> driver, there is the dependency between machine driver and hdac_hdmi > >module. > > > >I don't get your question. > >All machine drivers supporting the iDISP link explicitly use a select > >HDAC_HDMI in the Kconfig and refer to DAIs created/exposed by the > >hdac_hdmi module. > > So the dependency should always exists. I have checked all the machine > drivers and they all depends on the hdac_hdmi if they are using it. > > >It's possible that the hdac_hdmi codec is probed as a result of the platform > >driver configuration but the machine driver does not support HDMI. In that > >case, is there any issue with your solution? > > If machine driver doesn't support HDMI, component will never be called. > It is safe. > > Hi Takashi, > > My patch doesn't use component->init() as you suggested. But they should > be similar. Are you OK with this patch? Yes. Takashi
diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c index 5eeb0fe..8bb883e 100644 --- a/sound/soc/codecs/hdac_hdmi.c +++ b/sound/soc/codecs/hdac_hdmi.c @@ -1855,6 +1855,15 @@ static int hdmi_codec_probe(struct snd_soc_component *component) hdmi->card = dapm->card->snd_card; /* + * Setup a device_link between card device and HDMI codec device. + * The card device is the consumer and the HDMI codec device is + * the supplier. With this setting, we can make sure that the audio + * domain in display power will be always turned on before operating + * on the HDMI audio codec registers. + */ + device_link_add(component->card->dev, &hdev->dev, DL_FLAG_RPM_ACTIVE | + DL_FLAG_AUTOREMOVE_CONSUMER); + /* * hdac_device core already sets the state to active and calls * get_noresume. So enable runtime and set the device to suspend. */