diff mbox series

[V2] ASoC: hdac_hdmi: add device PM dependency

Message ID 1554094251-25910-1-git-send-email-libin.yang@intel.com (mailing list archive)
State New, archived
Headers show
Series [V2] ASoC: hdac_hdmi: add device PM dependency | expand

Commit Message

Yang, Libin April 1, 2019, 4:50 a.m. UTC 
From: Libin Yang <libin.yang@intel.com>

HDMI audio codec register operation depends on the display audio power
domain. The hdmi audio codec dapm event callback must be called after
display audio power is turned on.

Add hdac_hdmi_device_link_add() in HDMI audio codec driver. The customer
audio driver, such as cAVS/SST and SOF audio driver, can call the function
to set the audio power dependency.

Signed-off-by: Libin Yang <libin.yang@intel.com>
---
 sound/soc/codecs/hdac_hdmi.c | 12 ++++++++++++
 sound/soc/codecs/hdac_hdmi.h |  6 ++++++
 2 files changed, 18 insertions(+)

Comments

Pierre-Louis Bossart April 1, 2019, 12:34 p.m. UTC | #1 
On 4/1/19 12:50 AM, libin.yang@intel.com wrote:
> From: Libin Yang <libin.yang@intel.com>
> 
> HDMI audio codec register operation depends on the display audio power
> domain. The hdmi audio codec dapm event callback must be called after
> display audio power is turned on.
> 
> Add hdac_hdmi_device_link_add() in HDMI audio codec driver. The customer
> audio driver, such as cAVS/SST and SOF audio driver, can call the function
> to set the audio power dependency.
> 
> Signed-off-by: Libin Yang <libin.yang@intel.com>
> ---
>   sound/soc/codecs/hdac_hdmi.c | 12 ++++++++++++
>   sound/soc/codecs/hdac_hdmi.h |  6 ++++++
>   2 files changed, 18 insertions(+)
> 
> diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
> index 5eeb0fe..c991407 100644
> --- a/sound/soc/codecs/hdac_hdmi.c
> +++ b/sound/soc/codecs/hdac_hdmi.c
> @@ -1792,6 +1792,18 @@ int hdac_hdmi_jack_init(struct snd_soc_dai *dai, int device,
>   }
>   EXPORT_SYMBOL_GPL(hdac_hdmi_jack_init);
>   
> +struct device_link *
> +hdac_hdmi_device_link_add(struct device *consumer,
> +			  struct snd_soc_component *component,
> +			  u32 flags)
> +{
> +	struct hdac_hdmi_priv *hdmi = snd_soc_component_get_drvdata(component);
> +	struct hdac_device *hdev = hdmi->hdev;
> +
> +	return device_link_add(consumer, &hdev->dev, flags);

Don't you need a matching device_link_free() to avoid a memory leak?

Also what would be the expectations for the flags and do you really want 
the consumer to set them?

And last, without a patch of the cAVS driver it's hard to see how this 
might be used.

> +}
> +EXPORT_SYMBOL_GPL(hdac_hdmi_device_link_add);
> +
>   static void hdac_hdmi_present_sense_all_pins(struct hdac_device *hdev,
>   			struct hdac_hdmi_priv *hdmi, bool detect_pin_caps)
>   {
> diff --git a/sound/soc/codecs/hdac_hdmi.h b/sound/soc/codecs/hdac_hdmi.h
> index 4fa2fc9..9239c6b 100644
> --- a/sound/soc/codecs/hdac_hdmi.h
> +++ b/sound/soc/codecs/hdac_hdmi.h
> @@ -7,4 +7,10 @@ int hdac_hdmi_jack_init(struct snd_soc_dai *dai, int pcm,
>   
>   int hdac_hdmi_jack_port_init(struct snd_soc_component *component,
>   			struct snd_soc_dapm_context *dapm);
> +
> +struct device_link *
> +hdac_hdmi_device_link_add(struct device *consumer,
> +			  struct snd_soc_component *component,
> +			  u32 flags);
> +
>   #endif /* __HDAC_HDMI_H__ */
>
Yang, Libin April 1, 2019, 1:43 p.m. UTC | #2 
Hi Pierre,

>-----Original Message-----
>From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com]
>Sent: Monday, April 1, 2019 8:34 PM
>To: Yang, Libin <libin.yang@intel.com>; alsa-devel@alsa-project.org;
>tiwai@suse.de; broonie@kernel.org
>Subject: Re: [alsa-devel] [PATCH V2] ASoC: hdac_hdmi: add device PM
>dependency
>
>On 4/1/19 12:50 AM, libin.yang@intel.com wrote:
>> From: Libin Yang <libin.yang@intel.com>
>>
>> HDMI audio codec register operation depends on the display audio power
>> domain. The hdmi audio codec dapm event callback must be called after
>> display audio power is turned on.
>>
>> Add hdac_hdmi_device_link_add() in HDMI audio codec driver. The
>> customer audio driver, such as cAVS/SST and SOF audio driver, can call
>> the function to set the audio power dependency.
>>
>> Signed-off-by: Libin Yang <libin.yang@intel.com>
>> ---
>>   sound/soc/codecs/hdac_hdmi.c | 12 ++++++++++++
>>   sound/soc/codecs/hdac_hdmi.h |  6 ++++++
>>   2 files changed, 18 insertions(+)
>>
>> diff --git a/sound/soc/codecs/hdac_hdmi.c
>> b/sound/soc/codecs/hdac_hdmi.c index 5eeb0fe..c991407 100644
>> --- a/sound/soc/codecs/hdac_hdmi.c
>> +++ b/sound/soc/codecs/hdac_hdmi.c
>> @@ -1792,6 +1792,18 @@ int hdac_hdmi_jack_init(struct snd_soc_dai *dai,
>int device,
>>   }
>>   EXPORT_SYMBOL_GPL(hdac_hdmi_jack_init);
>>
>> +struct device_link *
>> +hdac_hdmi_device_link_add(struct device *consumer,
>> +			  struct snd_soc_component *component,
>> +			  u32 flags)
>> +{
>> +	struct hdac_hdmi_priv *hdmi =
>snd_soc_component_get_drvdata(component);
>> +	struct hdac_device *hdev = hdmi->hdev;
>> +
>> +	return device_link_add(consumer, &hdev->dev, flags);
>
>Don't you need a matching device_link_free() to avoid a memory leak?

Right. Sorry for my poor patch and thanks for pointing it out. 
I will refine it and test tomorrow.

>
>Also what would be the expectations for the flags and do you really want the
>consumer to set them?

A consumer setting the flag will be more flexible. For the flag, I'm currently thinking
to use DL_FLAG_RPM_ACTIVE. Not sure if it is a good flag.

>
>And last, without a patch of the cAVS driver it's hard to see how this might be
>used.

I tried on SOF, it worked. I will check if I have a cAVS environment and will have
a test on cAVS.

Regards,
Libin

>
>> +}
>> +EXPORT_SYMBOL_GPL(hdac_hdmi_device_link_add);
>> +
>>   static void hdac_hdmi_present_sense_all_pins(struct hdac_device *hdev,
>>   			struct hdac_hdmi_priv *hdmi, bool detect_pin_caps)
>>   {
>> diff --git a/sound/soc/codecs/hdac_hdmi.h
>> b/sound/soc/codecs/hdac_hdmi.h index 4fa2fc9..9239c6b 100644
>> --- a/sound/soc/codecs/hdac_hdmi.h
>> +++ b/sound/soc/codecs/hdac_hdmi.h
>> @@ -7,4 +7,10 @@ int hdac_hdmi_jack_init(struct snd_soc_dai *dai, int
>> pcm,
>>
>>   int hdac_hdmi_jack_port_init(struct snd_soc_component *component,
>>   			struct snd_soc_dapm_context *dapm);
>> +
>> +struct device_link *
>> +hdac_hdmi_device_link_add(struct device *consumer,
>> +			  struct snd_soc_component *component,
>> +			  u32 flags);
>> +
>>   #endif /* __HDAC_HDMI_H__ */
>>
Yang, Libin April 2, 2019, 2:45 a.m. UTC | #3 
Hi Pierre,

Thanks for pointing out my patch's defect. I think we should call 
xxx_del() instread of xxx_free().

Hi all,

I found it's a little confusing when releasing the link. I will send out
a RFC patch based on Pierre's suggestion. Please kindly help review the
logic of releasing the link.

Regards,
Libin


>-----Original Message-----
>From: Yang, Libin
>Sent: Monday, April 1, 2019 9:44 PM
>To: 'Pierre-Louis Bossart' <pierre-louis.bossart@linux.intel.com>; alsa-
>devel@alsa-project.org; tiwai@suse.de; broonie@kernel.org
>Subject: RE: [alsa-devel] [PATCH V2] ASoC: hdac_hdmi: add device PM
>dependency
>
>Hi Pierre,
>
>>-----Original Message-----
>>From: Pierre-Louis Bossart
>>[mailto:pierre-louis.bossart@linux.intel.com]
>>Sent: Monday, April 1, 2019 8:34 PM
>>To: Yang, Libin <libin.yang@intel.com>; alsa-devel@alsa-project.org;
>>tiwai@suse.de; broonie@kernel.org
>>Subject: Re: [alsa-devel] [PATCH V2] ASoC: hdac_hdmi: add device PM
>>dependency
>>
>>On 4/1/19 12:50 AM, libin.yang@intel.com wrote:
>>> From: Libin Yang <libin.yang@intel.com>
>>>
>>> HDMI audio codec register operation depends on the display audio
>>> power domain. The hdmi audio codec dapm event callback must be called
>>> after display audio power is turned on.
>>>
>>> Add hdac_hdmi_device_link_add() in HDMI audio codec driver. The
>>> customer audio driver, such as cAVS/SST and SOF audio driver, can
>>> call the function to set the audio power dependency.
>>>
>>> Signed-off-by: Libin Yang <libin.yang@intel.com>
>>> ---
>>>   sound/soc/codecs/hdac_hdmi.c | 12 ++++++++++++
>>>   sound/soc/codecs/hdac_hdmi.h |  6 ++++++
>>>   2 files changed, 18 insertions(+)
>>>
>>> diff --git a/sound/soc/codecs/hdac_hdmi.c
>>> b/sound/soc/codecs/hdac_hdmi.c index 5eeb0fe..c991407 100644
>>> --- a/sound/soc/codecs/hdac_hdmi.c
>>> +++ b/sound/soc/codecs/hdac_hdmi.c
>>> @@ -1792,6 +1792,18 @@ int hdac_hdmi_jack_init(struct snd_soc_dai
>>> *dai,
>>int device,
>>>   }
>>>   EXPORT_SYMBOL_GPL(hdac_hdmi_jack_init);
>>>
>>> +struct device_link *
>>> +hdac_hdmi_device_link_add(struct device *consumer,
>>> +			  struct snd_soc_component *component,
>>> +			  u32 flags)
>>> +{
>>> +	struct hdac_hdmi_priv *hdmi =
>>snd_soc_component_get_drvdata(component);
>>> +	struct hdac_device *hdev = hdmi->hdev;
>>> +
>>> +	return device_link_add(consumer, &hdev->dev, flags);
>>
>>Don't you need a matching device_link_free() to avoid a memory leak?
>
>Right. Sorry for my poor patch and thanks for pointing it out.
>I will refine it and test tomorrow.
>
>>
>>Also what would be the expectations for the flags and do you really
>>want the consumer to set them?
>
>A consumer setting the flag will be more flexible. For the flag, I'm currently
>thinking to use DL_FLAG_RPM_ACTIVE. Not sure if it is a good flag.
>
>>
>>And last, without a patch of the cAVS driver it's hard to see how this
>>might be used.
>
>I tried on SOF, it worked. I will check if I have a cAVS environment and will
>have a test on cAVS.
>
>Regards,
>Libin
>
>>
>>> +}
>>> +EXPORT_SYMBOL_GPL(hdac_hdmi_device_link_add);
>>> +
>>>   static void hdac_hdmi_present_sense_all_pins(struct hdac_device *hdev,
>>>   			struct hdac_hdmi_priv *hdmi, bool detect_pin_caps)
>>>   {
>>> diff --git a/sound/soc/codecs/hdac_hdmi.h
>>> b/sound/soc/codecs/hdac_hdmi.h index 4fa2fc9..9239c6b 100644
>>> --- a/sound/soc/codecs/hdac_hdmi.h
>>> +++ b/sound/soc/codecs/hdac_hdmi.h
>>> @@ -7,4 +7,10 @@ int hdac_hdmi_jack_init(struct snd_soc_dai *dai, int
>>> pcm,
>>>
>>>   int hdac_hdmi_jack_port_init(struct snd_soc_component *component,
>>>   			struct snd_soc_dapm_context *dapm);
>>> +
>>> +struct device_link *
>>> +hdac_hdmi_device_link_add(struct device *consumer,
>>> +			  struct snd_soc_component *component,
>>> +			  u32 flags);
>>> +
>>>   #endif /* __HDAC_HDMI_H__ */
>>>
diff mbox series

Patch

diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index 5eeb0fe..c991407 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -1792,6 +1792,18 @@  int hdac_hdmi_jack_init(struct snd_soc_dai *dai, int device,
 }
 EXPORT_SYMBOL_GPL(hdac_hdmi_jack_init);
 
+struct device_link *
+hdac_hdmi_device_link_add(struct device *consumer,
+			  struct snd_soc_component *component,
+			  u32 flags)
+{
+	struct hdac_hdmi_priv *hdmi = snd_soc_component_get_drvdata(component);
+	struct hdac_device *hdev = hdmi->hdev;
+
+	return device_link_add(consumer, &hdev->dev, flags);
+}
+EXPORT_SYMBOL_GPL(hdac_hdmi_device_link_add);
+
 static void hdac_hdmi_present_sense_all_pins(struct hdac_device *hdev,
 			struct hdac_hdmi_priv *hdmi, bool detect_pin_caps)
 {
diff --git a/sound/soc/codecs/hdac_hdmi.h b/sound/soc/codecs/hdac_hdmi.h
index 4fa2fc9..9239c6b 100644
--- a/sound/soc/codecs/hdac_hdmi.h
+++ b/sound/soc/codecs/hdac_hdmi.h
@@ -7,4 +7,10 @@  int hdac_hdmi_jack_init(struct snd_soc_dai *dai, int pcm,
 
 int hdac_hdmi_jack_port_init(struct snd_soc_component *component,
 			struct snd_soc_dapm_context *dapm);
+
+struct device_link *
+hdac_hdmi_device_link_add(struct device *consumer,
+			  struct snd_soc_component *component,
+			  u32 flags);
+
 #endif /* __HDAC_HDMI_H__ */