ALSA: hda/tegra: enable clock during probe
diff mbox series

Message ID 1548351403-1875-1-git-send-email-spujar@nvidia.com
State New
Headers show
Series
  • ALSA: hda/tegra: enable clock during probe
Related show

Commit Message

Sameer Pujar Jan. 24, 2019, 5:36 p.m. UTC
If CONFIG_PM is disabled or runtime PM calls are forbidden, the clocks
will not be ON. This could cause issue during probe, where hda init
setup is done. This patch checks whether runtime PM is enabled or not.
If disabled, clocks are enabled in probe() and disabled in remove()

This patch does following minor changes as cleanup,
  * return code check for pm_runtime_get_sync() to take care of failure
    and exit gracefully.
  * In remove path runtime PM is disabled before calling snd_card_free().
  * hda_tegra_disable_clocks() is moved out of CONFIG_PM_SLEEP check.
  * runtime PM callbacks moved out of CONFIG_PM check

Signed-off-by: Sameer Pujar <spujar@nvidia.com>
Reviewed-by: Ravindra Lokhande <rlokhande@nvidia.com>
Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
---
 sound/pci/hda/hda_tegra.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

Comments

Takashi Iwai Jan. 24, 2019, 7:08 p.m. UTC | #1
On Thu, 24 Jan 2019 18:36:43 +0100,
Sameer Pujar wrote:
> 
> If CONFIG_PM is disabled or runtime PM calls are forbidden, the clocks
> will not be ON. This could cause issue during probe, where hda init
> setup is done. This patch checks whether runtime PM is enabled or not.
> If disabled, clocks are enabled in probe() and disabled in remove()
> 
> This patch does following minor changes as cleanup,
>   * return code check for pm_runtime_get_sync() to take care of failure
>     and exit gracefully.
>   * In remove path runtime PM is disabled before calling snd_card_free().
>   * hda_tegra_disable_clocks() is moved out of CONFIG_PM_SLEEP check.
>   * runtime PM callbacks moved out of CONFIG_PM check
> 
> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
> Reviewed-by: Ravindra Lokhande <rlokhande@nvidia.com>
> Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
(snip)
> @@ -555,6 +553,13 @@ static int hda_tegra_probe(struct platform_device *pdev)
>  	if (!azx_has_pm_runtime(chip))
>  		pm_runtime_forbid(hda->dev);
>  
> +	/* explicit resume if runtime PM is disabled */
> +	if (!pm_runtime_enabled(hda->dev)) {
> +		err = hda_tegra_runtime_resume(hda->dev);
> +		if (err)
> +			goto out_free;
> +	}
> +
>  	schedule_work(&hda->probe_work);

Calling runtime_resume here is really confusing...


> @@ -571,7 +576,14 @@ static void hda_tegra_probe_work(struct work_struct *work)
>  	struct platform_device *pdev = to_platform_device(hda->dev);
>  	int err;
>  
> -	pm_runtime_get_sync(hda->dev);
> +	err = pm_runtime_get_sync(hda->dev);
> +	if (err < 0) {
> +		dev_err(hda->dev,
> +			"failed in pm_runtime_get_syc with err = %d\n",
> +			err);
> +		return;
> +	}

This pm_runtime_get_sync() is needed just because you enabled runtime
PM before probe_work.  Why not deferring the runtime PM enablement
after probing done?

That is what we need is the hda_tegra_enable_clocks() call at probe
unconditionally before enabling runtime PM.

>  	err = hda_tegra_first_init(chip, pdev);
>  	if (err < 0)
>  		goto out_free;
> @@ -599,12 +611,13 @@ static void hda_tegra_probe_work(struct work_struct *work)
>  
>  static int hda_tegra_remove(struct platform_device *pdev)
>  {
> -	int ret;
> -
> -	ret = snd_card_free(dev_get_drvdata(&pdev->dev));
>  	pm_runtime_disable(&pdev->dev);
> +	if (!pm_runtime_status_suspended(&pdev->dev)) {
> +		hda_tegra_runtime_suspend(&pdev->dev);
> +		pm_runtime_set_suspended(&pdev->dev);
> +	}
> -	return ret;
> +	return snd_card_free(dev_get_drvdata(&pdev->dev));


Forcing the suspend *before* snd_card_free() doesn't sound right.
It's the point before the disconnect and release procedure of all the
rest.  That is, the other hardware components are still active at this
point.


thanks,

Takashi
Sameer Pujar Jan. 25, 2019, 7:08 a.m. UTC | #2
On 1/25/2019 12:38 AM, Takashi Iwai wrote:
> On Thu, 24 Jan 2019 18:36:43 +0100,
> Sameer Pujar wrote:
>> If CONFIG_PM is disabled or runtime PM calls are forbidden, the clocks
>> will not be ON. This could cause issue during probe, where hda init
>> setup is done. This patch checks whether runtime PM is enabled or not.
>> If disabled, clocks are enabled in probe() and disabled in remove()
>>
>> This patch does following minor changes as cleanup,
>>    * return code check for pm_runtime_get_sync() to take care of failure
>>      and exit gracefully.
>>    * In remove path runtime PM is disabled before calling snd_card_free().
>>    * hda_tegra_disable_clocks() is moved out of CONFIG_PM_SLEEP check.
>>    * runtime PM callbacks moved out of CONFIG_PM check
>>
>> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
>> Reviewed-by: Ravindra Lokhande <rlokhande@nvidia.com>
>> Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
> (snip)
>> @@ -555,6 +553,13 @@ static int hda_tegra_probe(struct platform_device *pdev)
>>   	if (!azx_has_pm_runtime(chip))
>>   		pm_runtime_forbid(hda->dev);
>>   
>> +	/* explicit resume if runtime PM is disabled */
>> +	if (!pm_runtime_enabled(hda->dev)) {
>> +		err = hda_tegra_runtime_resume(hda->dev);
>> +		if (err)
>> +			goto out_free;
>> +	}
>> +
>>   	schedule_work(&hda->probe_work);
> Calling runtime_resume here is really confusing...
>
>
>> @@ -571,7 +576,14 @@ static void hda_tegra_probe_work(struct work_struct *work)
>>   	struct platform_device *pdev = to_platform_device(hda->dev);
>>   	int err;
>>   
>> -	pm_runtime_get_sync(hda->dev);
>> +	err = pm_runtime_get_sync(hda->dev);
>> +	if (err < 0) {
>> +		dev_err(hda->dev,
>> +			"failed in pm_runtime_get_syc with err = %d\n",
>> +			err);
>> +		return;
>> +	}
> This pm_runtime_get_sync() is needed just because you enabled runtime
> PM before probe_work.  Why not deferring the runtime PM enablement
> after probing done?
I think what you are suggesting can be done and simplify things further.
I can get rid of pm_runtime_get_sync/pm_runtime_put in probe work.
> That is what we need is the hda_tegra_enable_clocks() call at probe
> unconditionally before enabling runtime PM.
>
>>   	err = hda_tegra_first_init(chip, pdev);
>>   	if (err < 0)
>>   		goto out_free;
>> @@ -599,12 +611,13 @@ static void hda_tegra_probe_work(struct work_struct *work)
>>   
>>   static int hda_tegra_remove(struct platform_device *pdev)
>>   {
>> -	int ret;
>> -
>> -	ret = snd_card_free(dev_get_drvdata(&pdev->dev));
>>   	pm_runtime_disable(&pdev->dev);
>> +	if (!pm_runtime_status_suspended(&pdev->dev)) {
>> +		hda_tegra_runtime_suspend(&pdev->dev);
>> +		pm_runtime_set_suspended(&pdev->dev);
>> +	}
>> -	return ret;
>> +	return snd_card_free(dev_get_drvdata(&pdev->dev));
>
> Forcing the suspend *before* snd_card_free() doesn't sound right.
> It's the point before the disconnect and release procedure of all the
> rest.  That is, the other hardware components are still active at this
> point.

Ok, I will keep the sequence same and publish updated version.

Thanks,
Sameer.

>
> thanks,
>
> Takashi
Jon Hunter Jan. 25, 2019, 11:36 a.m. UTC | #3
On 24/01/2019 19:08, Takashi Iwai wrote:
> On Thu, 24 Jan 2019 18:36:43 +0100,
> Sameer Pujar wrote:
>>
>> If CONFIG_PM is disabled or runtime PM calls are forbidden, the clocks
>> will not be ON. This could cause issue during probe, where hda init
>> setup is done. This patch checks whether runtime PM is enabled or not.
>> If disabled, clocks are enabled in probe() and disabled in remove()
>>
>> This patch does following minor changes as cleanup,
>>   * return code check for pm_runtime_get_sync() to take care of failure
>>     and exit gracefully.
>>   * In remove path runtime PM is disabled before calling snd_card_free().
>>   * hda_tegra_disable_clocks() is moved out of CONFIG_PM_SLEEP check.
>>   * runtime PM callbacks moved out of CONFIG_PM check
>>
>> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
>> Reviewed-by: Ravindra Lokhande <rlokhande@nvidia.com>
>> Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
> (snip)
>> @@ -555,6 +553,13 @@ static int hda_tegra_probe(struct platform_device *pdev)
>>  	if (!azx_has_pm_runtime(chip))
>>  		pm_runtime_forbid(hda->dev);
>>  
>> +	/* explicit resume if runtime PM is disabled */
>> +	if (!pm_runtime_enabled(hda->dev)) {
>> +		err = hda_tegra_runtime_resume(hda->dev);
>> +		if (err)
>> +			goto out_free;
>> +	}
>> +
>>  	schedule_work(&hda->probe_work);
> 
> Calling runtime_resume here is really confusing...

Why? IMO it is better to have a single handler for resuming the device
and so if RPM is not enabled we call the handler directly. This is what
we have been advised to do in the past and do in other drivers. See ...

sound/soc/tegra/tegra30_i2s.c

> 
>> @@ -571,7 +576,14 @@ static void hda_tegra_probe_work(struct work_struct *work)
>>  	struct platform_device *pdev = to_platform_device(hda->dev);
>>  	int err;
>>  
>> -	pm_runtime_get_sync(hda->dev);
>> +	err = pm_runtime_get_sync(hda->dev);
>> +	if (err < 0) {
>> +		dev_err(hda->dev,
>> +			"failed in pm_runtime_get_syc with err = %d\n",
>> +			err);
>> +		return;
>> +	}
> 
> This pm_runtime_get_sync() is needed just because you enabled runtime
> PM before probe_work.  Why not deferring the runtime PM enablement
> after probing done?

That would be fine with me.

> That is what we need is the hda_tegra_enable_clocks() call at probe
> unconditionally before enabling runtime PM.

I think that calling hda_tegra_runtime_resume as above is sufficient.
The nice thing about calling the runtime_resume function is that if for
a future device there is something else in addition to clocks, say a
reset, that also needs to be handled, we just added to the
runtime_resume/suspend handlers and we are done.

Cheers
Jon
Takashi Iwai Jan. 25, 2019, 12:40 p.m. UTC | #4
On Fri, 25 Jan 2019 12:36:00 +0100,
Jon Hunter wrote:
> 
> 
> On 24/01/2019 19:08, Takashi Iwai wrote:
> > On Thu, 24 Jan 2019 18:36:43 +0100,
> > Sameer Pujar wrote:
> >>
> >> If CONFIG_PM is disabled or runtime PM calls are forbidden, the clocks
> >> will not be ON. This could cause issue during probe, where hda init
> >> setup is done. This patch checks whether runtime PM is enabled or not.
> >> If disabled, clocks are enabled in probe() and disabled in remove()
> >>
> >> This patch does following minor changes as cleanup,
> >>   * return code check for pm_runtime_get_sync() to take care of failure
> >>     and exit gracefully.
> >>   * In remove path runtime PM is disabled before calling snd_card_free().
> >>   * hda_tegra_disable_clocks() is moved out of CONFIG_PM_SLEEP check.
> >>   * runtime PM callbacks moved out of CONFIG_PM check
> >>
> >> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
> >> Reviewed-by: Ravindra Lokhande <rlokhande@nvidia.com>
> >> Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
> > (snip)
> >> @@ -555,6 +553,13 @@ static int hda_tegra_probe(struct platform_device *pdev)
> >>  	if (!azx_has_pm_runtime(chip))
> >>  		pm_runtime_forbid(hda->dev);
> >>  
> >> +	/* explicit resume if runtime PM is disabled */
> >> +	if (!pm_runtime_enabled(hda->dev)) {
> >> +		err = hda_tegra_runtime_resume(hda->dev);
> >> +		if (err)
> >> +			goto out_free;
> >> +	}
> >> +
> >>  	schedule_work(&hda->probe_work);
> > 
> > Calling runtime_resume here is really confusing...
> 
> Why? IMO it is better to have a single handler for resuming the device
> and so if RPM is not enabled we call the handler directly. This is what
> we have been advised to do in the past and do in other drivers. See ...

The point is that we're not "resuming" anything there.  It's in the
early probe stage, and the device state is uninitialized, not really
suspended.  It'd end up with just calling the same helper
(hda_tegra_enable_clocks()), though.


Takashi
Jon Hunter Jan. 25, 2019, 1:26 p.m. UTC | #5
On 25/01/2019 12:40, Takashi Iwai wrote:
> On Fri, 25 Jan 2019 12:36:00 +0100,
> Jon Hunter wrote:
>>
>>
>> On 24/01/2019 19:08, Takashi Iwai wrote:
>>> On Thu, 24 Jan 2019 18:36:43 +0100,
>>> Sameer Pujar wrote:
>>>>
>>>> If CONFIG_PM is disabled or runtime PM calls are forbidden, the clocks
>>>> will not be ON. This could cause issue during probe, where hda init
>>>> setup is done. This patch checks whether runtime PM is enabled or not.
>>>> If disabled, clocks are enabled in probe() and disabled in remove()
>>>>
>>>> This patch does following minor changes as cleanup,
>>>>   * return code check for pm_runtime_get_sync() to take care of failure
>>>>     and exit gracefully.
>>>>   * In remove path runtime PM is disabled before calling snd_card_free().
>>>>   * hda_tegra_disable_clocks() is moved out of CONFIG_PM_SLEEP check.
>>>>   * runtime PM callbacks moved out of CONFIG_PM check
>>>>
>>>> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
>>>> Reviewed-by: Ravindra Lokhande <rlokhande@nvidia.com>
>>>> Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
>>> (snip)
>>>> @@ -555,6 +553,13 @@ static int hda_tegra_probe(struct platform_device *pdev)
>>>>  	if (!azx_has_pm_runtime(chip))
>>>>  		pm_runtime_forbid(hda->dev);
>>>>  
>>>> +	/* explicit resume if runtime PM is disabled */
>>>> +	if (!pm_runtime_enabled(hda->dev)) {
>>>> +		err = hda_tegra_runtime_resume(hda->dev);
>>>> +		if (err)
>>>> +			goto out_free;
>>>> +	}
>>>> +
>>>>  	schedule_work(&hda->probe_work);
>>>
>>> Calling runtime_resume here is really confusing...
>>
>> Why? IMO it is better to have a single handler for resuming the device
>> and so if RPM is not enabled we call the handler directly. This is what
>> we have been advised to do in the past and do in other drivers. See ...
> 
> The point is that we're not "resuming" anything there.  It's in the
> early probe stage, and the device state is uninitialized, not really
> suspended.  It'd end up with just calling the same helper
> (hda_tegra_enable_clocks()), though.

Yes and you can make the same argument for every driver that calls
pm_runtime_get_sync() during probe to turn on clocks, handle resets,
etc, because at the end of the day the very first call to
pm_runtime_get_sync() invokes the runtime_resume callback, when we have
never been suspended.

Yes at the end of the day it is the same and given that we have done
this elsewhere I think it is good to be consistent if/where we can.

Furthermore, there are other various drivers in the kernel that do the
same ...

drivers/i2c/busses/i2c-img-scb.c
drivers/dma/xilinx/zynqmp_dma.c
drivers/gpu/drm/arm/malidp_drv.c

Cheers
Jon
Takashi Iwai Jan. 25, 2019, 1:58 p.m. UTC | #6
On Fri, 25 Jan 2019 14:26:27 +0100,
Jon Hunter wrote:
> 
> 
> On 25/01/2019 12:40, Takashi Iwai wrote:
> > On Fri, 25 Jan 2019 12:36:00 +0100,
> > Jon Hunter wrote:
> >>
> >>
> >> On 24/01/2019 19:08, Takashi Iwai wrote:
> >>> On Thu, 24 Jan 2019 18:36:43 +0100,
> >>> Sameer Pujar wrote:
> >>>>
> >>>> If CONFIG_PM is disabled or runtime PM calls are forbidden, the clocks
> >>>> will not be ON. This could cause issue during probe, where hda init
> >>>> setup is done. This patch checks whether runtime PM is enabled or not.
> >>>> If disabled, clocks are enabled in probe() and disabled in remove()
> >>>>
> >>>> This patch does following minor changes as cleanup,
> >>>>   * return code check for pm_runtime_get_sync() to take care of failure
> >>>>     and exit gracefully.
> >>>>   * In remove path runtime PM is disabled before calling snd_card_free().
> >>>>   * hda_tegra_disable_clocks() is moved out of CONFIG_PM_SLEEP check.
> >>>>   * runtime PM callbacks moved out of CONFIG_PM check
> >>>>
> >>>> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
> >>>> Reviewed-by: Ravindra Lokhande <rlokhande@nvidia.com>
> >>>> Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
> >>> (snip)
> >>>> @@ -555,6 +553,13 @@ static int hda_tegra_probe(struct platform_device *pdev)
> >>>>  	if (!azx_has_pm_runtime(chip))
> >>>>  		pm_runtime_forbid(hda->dev);
> >>>>  
> >>>> +	/* explicit resume if runtime PM is disabled */
> >>>> +	if (!pm_runtime_enabled(hda->dev)) {
> >>>> +		err = hda_tegra_runtime_resume(hda->dev);
> >>>> +		if (err)
> >>>> +			goto out_free;
> >>>> +	}
> >>>> +
> >>>>  	schedule_work(&hda->probe_work);
> >>>
> >>> Calling runtime_resume here is really confusing...
> >>
> >> Why? IMO it is better to have a single handler for resuming the device
> >> and so if RPM is not enabled we call the handler directly. This is what
> >> we have been advised to do in the past and do in other drivers. See ...
> > 
> > The point is that we're not "resuming" anything there.  It's in the
> > early probe stage, and the device state is uninitialized, not really
> > suspended.  It'd end up with just calling the same helper
> > (hda_tegra_enable_clocks()), though.
> 
> Yes and you can make the same argument for every driver that calls
> pm_runtime_get_sync() during probe to turn on clocks, handle resets,
> etc, because at the end of the day the very first call to
> pm_runtime_get_sync() invokes the runtime_resume callback, when we have
> never been suspended.

Although there are some magical pm_runtime_*() in some places, most of
such pm_runtime_get_sync() is for the actual runtime PM management (to
prevent the runtime suspend), while the code above is for explicitly
setting up something for non-PM cases.

And if pm_runtime_get_sync() is obviously superfluous, we should
remove such calls.  Really.

> Yes at the end of the day it is the same and given that we have done
> this elsewhere I think it is good to be consistent if/where we can.

The code becomes less readable, and that's a good reason against it :)


thanks,

Takashi
Jon Hunter Jan. 25, 2019, 2:04 p.m. UTC | #7
On 25/01/2019 13:58, Takashi Iwai wrote:
> On Fri, 25 Jan 2019 14:26:27 +0100,
> Jon Hunter wrote:
>>
>>
>> On 25/01/2019 12:40, Takashi Iwai wrote:
>>> On Fri, 25 Jan 2019 12:36:00 +0100,
>>> Jon Hunter wrote:
>>>>
>>>>
>>>> On 24/01/2019 19:08, Takashi Iwai wrote:
>>>>> On Thu, 24 Jan 2019 18:36:43 +0100,
>>>>> Sameer Pujar wrote:
>>>>>>
>>>>>> If CONFIG_PM is disabled or runtime PM calls are forbidden, the clocks
>>>>>> will not be ON. This could cause issue during probe, where hda init
>>>>>> setup is done. This patch checks whether runtime PM is enabled or not.
>>>>>> If disabled, clocks are enabled in probe() and disabled in remove()
>>>>>>
>>>>>> This patch does following minor changes as cleanup,
>>>>>>   * return code check for pm_runtime_get_sync() to take care of failure
>>>>>>     and exit gracefully.
>>>>>>   * In remove path runtime PM is disabled before calling snd_card_free().
>>>>>>   * hda_tegra_disable_clocks() is moved out of CONFIG_PM_SLEEP check.
>>>>>>   * runtime PM callbacks moved out of CONFIG_PM check
>>>>>>
>>>>>> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
>>>>>> Reviewed-by: Ravindra Lokhande <rlokhande@nvidia.com>
>>>>>> Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
>>>>> (snip)
>>>>>> @@ -555,6 +553,13 @@ static int hda_tegra_probe(struct platform_device *pdev)
>>>>>>  	if (!azx_has_pm_runtime(chip))
>>>>>>  		pm_runtime_forbid(hda->dev);
>>>>>>  
>>>>>> +	/* explicit resume if runtime PM is disabled */
>>>>>> +	if (!pm_runtime_enabled(hda->dev)) {
>>>>>> +		err = hda_tegra_runtime_resume(hda->dev);
>>>>>> +		if (err)
>>>>>> +			goto out_free;
>>>>>> +	}
>>>>>> +
>>>>>>  	schedule_work(&hda->probe_work);
>>>>>
>>>>> Calling runtime_resume here is really confusing...
>>>>
>>>> Why? IMO it is better to have a single handler for resuming the device
>>>> and so if RPM is not enabled we call the handler directly. This is what
>>>> we have been advised to do in the past and do in other drivers. See ...
>>>
>>> The point is that we're not "resuming" anything there.  It's in the
>>> early probe stage, and the device state is uninitialized, not really
>>> suspended.  It'd end up with just calling the same helper
>>> (hda_tegra_enable_clocks()), though.
>>
>> Yes and you can make the same argument for every driver that calls
>> pm_runtime_get_sync() during probe to turn on clocks, handle resets,
>> etc, because at the end of the day the very first call to
>> pm_runtime_get_sync() invokes the runtime_resume callback, when we have
>> never been suspended.
> 
> Although there are some magical pm_runtime_*() in some places, most of
> such pm_runtime_get_sync() is for the actual runtime PM management (to
> prevent the runtime suspend), while the code above is for explicitly
> setting up something for non-PM cases.
> 
> And if pm_runtime_get_sync() is obviously superfluous, we should
> remove such calls.  Really.

Yes agree.

>> Yes at the end of the day it is the same and given that we have done
>> this elsewhere I think it is good to be consistent if/where we can.
> 
> The code becomes less readable, and that's a good reason against it :)

I don't its less readable. However, I do think it is less error prone :-)

Jon
Sameer Pujar Jan. 28, 2019, 6:06 a.m. UTC | #8
On 1/25/2019 7:34 PM, Jon Hunter wrote:
> On 25/01/2019 13:58, Takashi Iwai wrote:
>> On Fri, 25 Jan 2019 14:26:27 +0100,
>> Jon Hunter wrote:
>>>
>>> On 25/01/2019 12:40, Takashi Iwai wrote:
>>>> On Fri, 25 Jan 2019 12:36:00 +0100,
>>>> Jon Hunter wrote:
>>>>>
>>>>> On 24/01/2019 19:08, Takashi Iwai wrote:
>>>>>> On Thu, 24 Jan 2019 18:36:43 +0100,
>>>>>> Sameer Pujar wrote:
>>>>>>> If CONFIG_PM is disabled or runtime PM calls are forbidden, the clocks
>>>>>>> will not be ON. This could cause issue during probe, where hda init
>>>>>>> setup is done. This patch checks whether runtime PM is enabled or not.
>>>>>>> If disabled, clocks are enabled in probe() and disabled in remove()
>>>>>>>
>>>>>>> This patch does following minor changes as cleanup,
>>>>>>>    * return code check for pm_runtime_get_sync() to take care of failure
>>>>>>>      and exit gracefully.
>>>>>>>    * In remove path runtime PM is disabled before calling snd_card_free().
>>>>>>>    * hda_tegra_disable_clocks() is moved out of CONFIG_PM_SLEEP check.
>>>>>>>    * runtime PM callbacks moved out of CONFIG_PM check
>>>>>>>
>>>>>>> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
>>>>>>> Reviewed-by: Ravindra Lokhande <rlokhande@nvidia.com>
>>>>>>> Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
>>>>>> (snip)
>>>>>>> @@ -555,6 +553,13 @@ static int hda_tegra_probe(struct platform_device *pdev)
>>>>>>>   	if (!azx_has_pm_runtime(chip))
>>>>>>>   		pm_runtime_forbid(hda->dev);
>>>>>>>   
>>>>>>> +	/* explicit resume if runtime PM is disabled */
>>>>>>> +	if (!pm_runtime_enabled(hda->dev)) {
>>>>>>> +		err = hda_tegra_runtime_resume(hda->dev);
>>>>>>> +		if (err)
>>>>>>> +			goto out_free;
>>>>>>> +	}
>>>>>>> +
>>>>>>>   	schedule_work(&hda->probe_work);
>>>>>> Calling runtime_resume here is really confusing...
>>>>> Why? IMO it is better to have a single handler for resuming the device
>>>>> and so if RPM is not enabled we call the handler directly. This is what
>>>>> we have been advised to do in the past and do in other drivers. See ...
>>>> The point is that we're not "resuming" anything there.  It's in the
>>>> early probe stage, and the device state is uninitialized, not really
>>>> suspended.  It'd end up with just calling the same helper
>>>> (hda_tegra_enable_clocks()), though.
>>> Yes and you can make the same argument for every driver that calls
>>> pm_runtime_get_sync() during probe to turn on clocks, handle resets,
>>> etc, because at the end of the day the very first call to
>>> pm_runtime_get_sync() invokes the runtime_resume callback, when we have
>>> never been suspended.
>> Although there are some magical pm_runtime_*() in some places, most of
>> such pm_runtime_get_sync() is for the actual runtime PM management (to
>> prevent the runtime suspend), while the code above is for explicitly
>> setting up something for non-PM cases.
>>
>> And if pm_runtime_get_sync() is obviously superfluous, we should
>> remove such calls.  Really.
> Yes agree.
>
>>> Yes at the end of the day it is the same and given that we have done
>>> this elsewhere I think it is good to be consistent if/where we can.
>> The code becomes less readable, and that's a good reason against it :)
> I don't its less readable. However, I do think it is less error prone :-)

Do we have a consensus here? Request others to provide opinions to help 
close on this.

Thanks,
Sameer.

> Jon
>
Jon Hunter Jan. 30, 2019, 9:35 a.m. UTC | #9
On 28/01/2019 06:06, Sameer Pujar wrote:
> 
> On 1/25/2019 7:34 PM, Jon Hunter wrote:
>> On 25/01/2019 13:58, Takashi Iwai wrote:
>>> On Fri, 25 Jan 2019 14:26:27 +0100,
>>> Jon Hunter wrote:
>>>>
>>>> On 25/01/2019 12:40, Takashi Iwai wrote:
>>>>> On Fri, 25 Jan 2019 12:36:00 +0100,
>>>>> Jon Hunter wrote:
>>>>>>
>>>>>> On 24/01/2019 19:08, Takashi Iwai wrote:
>>>>>>> On Thu, 24 Jan 2019 18:36:43 +0100,
>>>>>>> Sameer Pujar wrote:
>>>>>>>> If CONFIG_PM is disabled or runtime PM calls are forbidden, the
>>>>>>>> clocks
>>>>>>>> will not be ON. This could cause issue during probe, where hda init
>>>>>>>> setup is done. This patch checks whether runtime PM is enabled
>>>>>>>> or not.
>>>>>>>> If disabled, clocks are enabled in probe() and disabled in remove()
>>>>>>>>
>>>>>>>> This patch does following minor changes as cleanup,
>>>>>>>>    * return code check for pm_runtime_get_sync() to take care of
>>>>>>>> failure
>>>>>>>>      and exit gracefully.
>>>>>>>>    * In remove path runtime PM is disabled before calling
>>>>>>>> snd_card_free().
>>>>>>>>    * hda_tegra_disable_clocks() is moved out of CONFIG_PM_SLEEP
>>>>>>>> check.
>>>>>>>>    * runtime PM callbacks moved out of CONFIG_PM check
>>>>>>>>
>>>>>>>> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
>>>>>>>> Reviewed-by: Ravindra Lokhande <rlokhande@nvidia.com>
>>>>>>>> Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
>>>>>>> (snip)
>>>>>>>> @@ -555,6 +553,13 @@ static int hda_tegra_probe(struct
>>>>>>>> platform_device *pdev)
>>>>>>>>       if (!azx_has_pm_runtime(chip))
>>>>>>>>           pm_runtime_forbid(hda->dev);
>>>>>>>>   +    /* explicit resume if runtime PM is disabled */
>>>>>>>> +    if (!pm_runtime_enabled(hda->dev)) {
>>>>>>>> +        err = hda_tegra_runtime_resume(hda->dev);
>>>>>>>> +        if (err)
>>>>>>>> +            goto out_free;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>>       schedule_work(&hda->probe_work);
>>>>>>> Calling runtime_resume here is really confusing...
>>>>>> Why? IMO it is better to have a single handler for resuming the
>>>>>> device
>>>>>> and so if RPM is not enabled we call the handler directly. This is
>>>>>> what
>>>>>> we have been advised to do in the past and do in other drivers.
>>>>>> See ...
>>>>> The point is that we're not "resuming" anything there.  It's in the
>>>>> early probe stage, and the device state is uninitialized, not really
>>>>> suspended.  It'd end up with just calling the same helper
>>>>> (hda_tegra_enable_clocks()), though.
>>>> Yes and you can make the same argument for every driver that calls
>>>> pm_runtime_get_sync() during probe to turn on clocks, handle resets,
>>>> etc, because at the end of the day the very first call to
>>>> pm_runtime_get_sync() invokes the runtime_resume callback, when we have
>>>> never been suspended.
>>> Although there are some magical pm_runtime_*() in some places, most of
>>> such pm_runtime_get_sync() is for the actual runtime PM management (to
>>> prevent the runtime suspend), while the code above is for explicitly
>>> setting up something for non-PM cases.
>>>
>>> And if pm_runtime_get_sync() is obviously superfluous, we should
>>> remove such calls.  Really.
>> Yes agree.
>>
>>>> Yes at the end of the day it is the same and given that we have done
>>>> this elsewhere I think it is good to be consistent if/where we can.
>>> The code becomes less readable, and that's a good reason against it :)
>> I don't its less readable. However, I do think it is less error prone :-)
> 
> Do we have a consensus here? Request others to provide opinions to help
> close on this.

I am not going to block this and ultimately it is Iwai-san call.

However, I wonder if it would be appropriate to move the whole ...

 if (pm_runtime_enabled())
         ret = pm_runtime_get_sync();
 else
         ret = hda_tegra_runtime_resume();

... into the probe_work function? In other words, we are just resuming
when we really need to. Unless I am still misunderstanding Iwai-san
comment. Otherwise if Iwai-san is happy with V2 then go with that.

Cheers
Jon
Takashi Iwai Jan. 30, 2019, 10:39 a.m. UTC | #10
On Wed, 30 Jan 2019 10:35:35 +0100,
Jon Hunter wrote:
> 
> 
> On 28/01/2019 06:06, Sameer Pujar wrote:
> > 
> > On 1/25/2019 7:34 PM, Jon Hunter wrote:
> >> On 25/01/2019 13:58, Takashi Iwai wrote:
> >>> On Fri, 25 Jan 2019 14:26:27 +0100,
> >>> Jon Hunter wrote:
> >>>>
> >>>> On 25/01/2019 12:40, Takashi Iwai wrote:
> >>>>> On Fri, 25 Jan 2019 12:36:00 +0100,
> >>>>> Jon Hunter wrote:
> >>>>>>
> >>>>>> On 24/01/2019 19:08, Takashi Iwai wrote:
> >>>>>>> On Thu, 24 Jan 2019 18:36:43 +0100,
> >>>>>>> Sameer Pujar wrote:
> >>>>>>>> If CONFIG_PM is disabled or runtime PM calls are forbidden, the
> >>>>>>>> clocks
> >>>>>>>> will not be ON. This could cause issue during probe, where hda init
> >>>>>>>> setup is done. This patch checks whether runtime PM is enabled
> >>>>>>>> or not.
> >>>>>>>> If disabled, clocks are enabled in probe() and disabled in remove()
> >>>>>>>>
> >>>>>>>> This patch does following minor changes as cleanup,
> >>>>>>>>    * return code check for pm_runtime_get_sync() to take care of
> >>>>>>>> failure
> >>>>>>>>      and exit gracefully.
> >>>>>>>>    * In remove path runtime PM is disabled before calling
> >>>>>>>> snd_card_free().
> >>>>>>>>    * hda_tegra_disable_clocks() is moved out of CONFIG_PM_SLEEP
> >>>>>>>> check.
> >>>>>>>>    * runtime PM callbacks moved out of CONFIG_PM check
> >>>>>>>>
> >>>>>>>> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
> >>>>>>>> Reviewed-by: Ravindra Lokhande <rlokhande@nvidia.com>
> >>>>>>>> Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
> >>>>>>> (snip)
> >>>>>>>> @@ -555,6 +553,13 @@ static int hda_tegra_probe(struct
> >>>>>>>> platform_device *pdev)
> >>>>>>>>       if (!azx_has_pm_runtime(chip))
> >>>>>>>>           pm_runtime_forbid(hda->dev);
> >>>>>>>>   +    /* explicit resume if runtime PM is disabled */
> >>>>>>>> +    if (!pm_runtime_enabled(hda->dev)) {
> >>>>>>>> +        err = hda_tegra_runtime_resume(hda->dev);
> >>>>>>>> +        if (err)
> >>>>>>>> +            goto out_free;
> >>>>>>>> +    }
> >>>>>>>> +
> >>>>>>>>       schedule_work(&hda->probe_work);
> >>>>>>> Calling runtime_resume here is really confusing...
> >>>>>> Why? IMO it is better to have a single handler for resuming the
> >>>>>> device
> >>>>>> and so if RPM is not enabled we call the handler directly. This is
> >>>>>> what
> >>>>>> we have been advised to do in the past and do in other drivers.
> >>>>>> See ...
> >>>>> The point is that we're not "resuming" anything there.  It's in the
> >>>>> early probe stage, and the device state is uninitialized, not really
> >>>>> suspended.  It'd end up with just calling the same helper
> >>>>> (hda_tegra_enable_clocks()), though.
> >>>> Yes and you can make the same argument for every driver that calls
> >>>> pm_runtime_get_sync() during probe to turn on clocks, handle resets,
> >>>> etc, because at the end of the day the very first call to
> >>>> pm_runtime_get_sync() invokes the runtime_resume callback, when we have
> >>>> never been suspended.
> >>> Although there are some magical pm_runtime_*() in some places, most of
> >>> such pm_runtime_get_sync() is for the actual runtime PM management (to
> >>> prevent the runtime suspend), while the code above is for explicitly
> >>> setting up something for non-PM cases.
> >>>
> >>> And if pm_runtime_get_sync() is obviously superfluous, we should
> >>> remove such calls.  Really.
> >> Yes agree.
> >>
> >>>> Yes at the end of the day it is the same and given that we have done
> >>>> this elsewhere I think it is good to be consistent if/where we can.
> >>> The code becomes less readable, and that's a good reason against it :)
> >> I don't its less readable. However, I do think it is less error prone :-)
> > 
> > Do we have a consensus here? Request others to provide opinions to help
> > close on this.
> 
> I am not going to block this and ultimately it is Iwai-san call.
> 
> However, I wonder if it would be appropriate to move the whole ...
> 
>  if (pm_runtime_enabled())
>          ret = pm_runtime_get_sync();
>  else
>          ret = hda_tegra_runtime_resume();
> 
> ... into the probe_work function? In other words, we are just resuming
> when we really need to. Unless I am still misunderstanding Iwai-san
> comment. Otherwise if Iwai-san is happy with V2 then go with that.

Only from my personal taste, I find the v2 patch is better.
It like simpler, after all.  That is, the code in v1 patch

	probe() {
		....
		pm_runtime_enable();
		....
		if (!pm_runtime_enabled())
			hda_tegra_runtime_resume();
		schedule_work();
	}

	work() {
		pm_runtime_get_sync();
		....
		pm_runtime_put();
	}

becomes shorter in v2:

	probe() {
		....
		hda_tegra_enable_clocks();
		schedule_work();
	}

	work() {
		....
		pm_runtime_enable();
	}


However, the point about hda_tegra_remove() you raised in the v2 patch
is still valid.  (BTW, I guess the discussion followed in that thread
was somehow misunderstood; your argument was about hda_tegra_remove()
while Sameer discussed about the probe.)  It can be with
hda_tegra_disable_clocks() if we want more consistency.

Though, I don't mind too much about that as long as the proper comment
is given.


thanks,

Takashi
Sameer Pujar Jan. 30, 2019, 10:56 a.m. UTC | #11
On 1/30/2019 4:09 PM, Takashi Iwai wrote:
> On Wed, 30 Jan 2019 10:35:35 +0100,
> Jon Hunter wrote:
>>
>> On 28/01/2019 06:06, Sameer Pujar wrote:
>>> On 1/25/2019 7:34 PM, Jon Hunter wrote:
>>>> On 25/01/2019 13:58, Takashi Iwai wrote:
>>>>> On Fri, 25 Jan 2019 14:26:27 +0100,
>>>>> Jon Hunter wrote:
>>>>>> On 25/01/2019 12:40, Takashi Iwai wrote:
>>>>>>> On Fri, 25 Jan 2019 12:36:00 +0100,
>>>>>>> Jon Hunter wrote:
>>>>>>>> On 24/01/2019 19:08, Takashi Iwai wrote:
>>>>>>>>> On Thu, 24 Jan 2019 18:36:43 +0100,
>>>>>>>>> Sameer Pujar wrote:
>>>>>>>>>> If CONFIG_PM is disabled or runtime PM calls are forbidden, the
>>>>>>>>>> clocks
>>>>>>>>>> will not be ON. This could cause issue during probe, where hda init
>>>>>>>>>> setup is done. This patch checks whether runtime PM is enabled
>>>>>>>>>> or not.
>>>>>>>>>> If disabled, clocks are enabled in probe() and disabled in remove()
>>>>>>>>>>
>>>>>>>>>> This patch does following minor changes as cleanup,
>>>>>>>>>>     * return code check for pm_runtime_get_sync() to take care of
>>>>>>>>>> failure
>>>>>>>>>>       and exit gracefully.
>>>>>>>>>>     * In remove path runtime PM is disabled before calling
>>>>>>>>>> snd_card_free().
>>>>>>>>>>     * hda_tegra_disable_clocks() is moved out of CONFIG_PM_SLEEP
>>>>>>>>>> check.
>>>>>>>>>>     * runtime PM callbacks moved out of CONFIG_PM check
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
>>>>>>>>>> Reviewed-by: Ravindra Lokhande <rlokhande@nvidia.com>
>>>>>>>>>> Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
>>>>>>>>> (snip)
>>>>>>>>>> @@ -555,6 +553,13 @@ static int hda_tegra_probe(struct
>>>>>>>>>> platform_device *pdev)
>>>>>>>>>>        if (!azx_has_pm_runtime(chip))
>>>>>>>>>>            pm_runtime_forbid(hda->dev);
>>>>>>>>>>    +    /* explicit resume if runtime PM is disabled */
>>>>>>>>>> +    if (!pm_runtime_enabled(hda->dev)) {
>>>>>>>>>> +        err = hda_tegra_runtime_resume(hda->dev);
>>>>>>>>>> +        if (err)
>>>>>>>>>> +            goto out_free;
>>>>>>>>>> +    }
>>>>>>>>>> +
>>>>>>>>>>        schedule_work(&hda->probe_work);
>>>>>>>>> Calling runtime_resume here is really confusing...
>>>>>>>> Why? IMO it is better to have a single handler for resuming the
>>>>>>>> device
>>>>>>>> and so if RPM is not enabled we call the handler directly. This is
>>>>>>>> what
>>>>>>>> we have been advised to do in the past and do in other drivers.
>>>>>>>> See ...
>>>>>>> The point is that we're not "resuming" anything there.  It's in the
>>>>>>> early probe stage, and the device state is uninitialized, not really
>>>>>>> suspended.  It'd end up with just calling the same helper
>>>>>>> (hda_tegra_enable_clocks()), though.
>>>>>> Yes and you can make the same argument for every driver that calls
>>>>>> pm_runtime_get_sync() during probe to turn on clocks, handle resets,
>>>>>> etc, because at the end of the day the very first call to
>>>>>> pm_runtime_get_sync() invokes the runtime_resume callback, when we have
>>>>>> never been suspended.
>>>>> Although there are some magical pm_runtime_*() in some places, most of
>>>>> such pm_runtime_get_sync() is for the actual runtime PM management (to
>>>>> prevent the runtime suspend), while the code above is for explicitly
>>>>> setting up something for non-PM cases.
>>>>>
>>>>> And if pm_runtime_get_sync() is obviously superfluous, we should
>>>>> remove such calls.  Really.
>>>> Yes agree.
>>>>
>>>>>> Yes at the end of the day it is the same and given that we have done
>>>>>> this elsewhere I think it is good to be consistent if/where we can.
>>>>> The code becomes less readable, and that's a good reason against it :)
>>>> I don't its less readable. However, I do think it is less error prone :-)
>>> Do we have a consensus here? Request others to provide opinions to help
>>> close on this.
>> I am not going to block this and ultimately it is Iwai-san call.
>>
>> However, I wonder if it would be appropriate to move the whole ...
>>
>>   if (pm_runtime_enabled())
>>           ret = pm_runtime_get_sync();
>>   else
>>           ret = hda_tegra_runtime_resume();
>>
>> ... into the probe_work function? In other words, we are just resuming
>> when we really need to. Unless I am still misunderstanding Iwai-san
>> comment. Otherwise if Iwai-san is happy with V2 then go with that.
> Only from my personal taste, I find the v2 patch is better.
> It like simpler, after all.  That is, the code in v1 patch
>
> 	probe() {
> 		....
> 		pm_runtime_enable();
> 		....
> 		if (!pm_runtime_enabled())
> 			hda_tegra_runtime_resume();
> 		schedule_work();
> 	}
>
> 	work() {
> 		pm_runtime_get_sync();
> 		....
> 		pm_runtime_put();
> 	}
>
> becomes shorter in v2:
>
> 	probe() {
> 		....
> 		hda_tegra_enable_clocks();
> 		schedule_work();
> 	}
>
> 	work() {
> 		....
> 		pm_runtime_enable();
> 	}
>
>
> However, the point about hda_tegra_remove() you raised in the v2 patch
> is still valid.  (BTW, I guess the discussion followed in that thread
> was somehow misunderstood; your argument was about hda_tegra_remove()
> while Sameer discussed about the probe.)  It can be with
> hda_tegra_disable_clocks() if we want more consistency.
>
> Though, I don't mind too much about that as long as the proper comment
> is given.
We might need entire functionality of hda_tegra_runtime_suspend() 
replicated here,
if hda_tegra_disable_clocks() were to be used. Right now it takes care 
of both the
cases where runtime PM is enabled/disabled. If you all agree, we can 
move the
discussion to v2 patch.

Thanks,
Sameer.
>
> thanks,
>
> Takashi
Jon Hunter Jan. 30, 2019, 12:24 p.m. UTC | #12
On 30/01/2019 10:56, Sameer Pujar wrote:
> 
> On 1/30/2019 4:09 PM, Takashi Iwai wrote:
>> On Wed, 30 Jan 2019 10:35:35 +0100,
>> Jon Hunter wrote:
>>>
>>> On 28/01/2019 06:06, Sameer Pujar wrote:
>>>> On 1/25/2019 7:34 PM, Jon Hunter wrote:
>>>>> On 25/01/2019 13:58, Takashi Iwai wrote:
>>>>>> On Fri, 25 Jan 2019 14:26:27 +0100,
>>>>>> Jon Hunter wrote:
>>>>>>> On 25/01/2019 12:40, Takashi Iwai wrote:
>>>>>>>> On Fri, 25 Jan 2019 12:36:00 +0100,
>>>>>>>> Jon Hunter wrote:
>>>>>>>>> On 24/01/2019 19:08, Takashi Iwai wrote:
>>>>>>>>>> On Thu, 24 Jan 2019 18:36:43 +0100,
>>>>>>>>>> Sameer Pujar wrote:
>>>>>>>>>>> If CONFIG_PM is disabled or runtime PM calls are forbidden, the
>>>>>>>>>>> clocks
>>>>>>>>>>> will not be ON. This could cause issue during probe, where
>>>>>>>>>>> hda init
>>>>>>>>>>> setup is done. This patch checks whether runtime PM is enabled
>>>>>>>>>>> or not.
>>>>>>>>>>> If disabled, clocks are enabled in probe() and disabled in
>>>>>>>>>>> remove()
>>>>>>>>>>>
>>>>>>>>>>> This patch does following minor changes as cleanup,
>>>>>>>>>>>     * return code check for pm_runtime_get_sync() to take
>>>>>>>>>>> care of
>>>>>>>>>>> failure
>>>>>>>>>>>       and exit gracefully.
>>>>>>>>>>>     * In remove path runtime PM is disabled before calling
>>>>>>>>>>> snd_card_free().
>>>>>>>>>>>     * hda_tegra_disable_clocks() is moved out of CONFIG_PM_SLEEP
>>>>>>>>>>> check.
>>>>>>>>>>>     * runtime PM callbacks moved out of CONFIG_PM check
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
>>>>>>>>>>> Reviewed-by: Ravindra Lokhande <rlokhande@nvidia.com>
>>>>>>>>>>> Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
>>>>>>>>>> (snip)
>>>>>>>>>>> @@ -555,6 +553,13 @@ static int hda_tegra_probe(struct
>>>>>>>>>>> platform_device *pdev)
>>>>>>>>>>>        if (!azx_has_pm_runtime(chip))
>>>>>>>>>>>            pm_runtime_forbid(hda->dev);
>>>>>>>>>>>    +    /* explicit resume if runtime PM is disabled */
>>>>>>>>>>> +    if (!pm_runtime_enabled(hda->dev)) {
>>>>>>>>>>> +        err = hda_tegra_runtime_resume(hda->dev);
>>>>>>>>>>> +        if (err)
>>>>>>>>>>> +            goto out_free;
>>>>>>>>>>> +    }
>>>>>>>>>>> +
>>>>>>>>>>>        schedule_work(&hda->probe_work);
>>>>>>>>>> Calling runtime_resume here is really confusing...
>>>>>>>>> Why? IMO it is better to have a single handler for resuming the
>>>>>>>>> device
>>>>>>>>> and so if RPM is not enabled we call the handler directly. This is
>>>>>>>>> what
>>>>>>>>> we have been advised to do in the past and do in other drivers.
>>>>>>>>> See ...
>>>>>>>> The point is that we're not "resuming" anything there.  It's in the
>>>>>>>> early probe stage, and the device state is uninitialized, not
>>>>>>>> really
>>>>>>>> suspended.  It'd end up with just calling the same helper
>>>>>>>> (hda_tegra_enable_clocks()), though.
>>>>>>> Yes and you can make the same argument for every driver that calls
>>>>>>> pm_runtime_get_sync() during probe to turn on clocks, handle resets,
>>>>>>> etc, because at the end of the day the very first call to
>>>>>>> pm_runtime_get_sync() invokes the runtime_resume callback, when
>>>>>>> we have
>>>>>>> never been suspended.
>>>>>> Although there are some magical pm_runtime_*() in some places,
>>>>>> most of
>>>>>> such pm_runtime_get_sync() is for the actual runtime PM management
>>>>>> (to
>>>>>> prevent the runtime suspend), while the code above is for explicitly
>>>>>> setting up something for non-PM cases.
>>>>>>
>>>>>> And if pm_runtime_get_sync() is obviously superfluous, we should
>>>>>> remove such calls.  Really.
>>>>> Yes agree.
>>>>>
>>>>>>> Yes at the end of the day it is the same and given that we have done
>>>>>>> this elsewhere I think it is good to be consistent if/where we can.
>>>>>> The code becomes less readable, and that's a good reason against
>>>>>> it :)
>>>>> I don't its less readable. However, I do think it is less error
>>>>> prone :-)
>>>> Do we have a consensus here? Request others to provide opinions to help
>>>> close on this.
>>> I am not going to block this and ultimately it is Iwai-san call.
>>>
>>> However, I wonder if it would be appropriate to move the whole ...
>>>
>>>   if (pm_runtime_enabled())
>>>           ret = pm_runtime_get_sync();
>>>   else
>>>           ret = hda_tegra_runtime_resume();
>>>
>>> ... into the probe_work function? In other words, we are just resuming
>>> when we really need to. Unless I am still misunderstanding Iwai-san
>>> comment. Otherwise if Iwai-san is happy with V2 then go with that.
>> Only from my personal taste, I find the v2 patch is better.
>> It like simpler, after all.  That is, the code in v1 patch
>>
>>     probe() {
>>         ....
>>         pm_runtime_enable();
>>         ....
>>         if (!pm_runtime_enabled())
>>             hda_tegra_runtime_resume();
>>         schedule_work();
>>     }
>>
>>     work() {
>>         pm_runtime_get_sync();
>>         ....
>>         pm_runtime_put();
>>     }
>>
>> becomes shorter in v2:
>>
>>     probe() {
>>         ....
>>         hda_tegra_enable_clocks();
>>         schedule_work();
>>     }
>>
>>     work() {
>>         ....
>>         pm_runtime_enable();
>>     }
>>
>>
>> However, the point about hda_tegra_remove() you raised in the v2 patch
>> is still valid.  (BTW, I guess the discussion followed in that thread
>> was somehow misunderstood; your argument was about hda_tegra_remove()
>> while Sameer discussed about the probe.)  It can be with
>> hda_tegra_disable_clocks() if we want more consistency.
>>
>> Though, I don't mind too much about that as long as the proper comment
>> is given.
> We might need entire functionality of hda_tegra_runtime_suspend()
> replicated here,
> if hda_tegra_disable_clocks() were to be used. Right now it takes care
> of both the
> cases where runtime PM is enabled/disabled. If you all agree, we can
> move the
> discussion to v2 patch.

We should avoid replicating the function.

Jon

Patch
diff mbox series

diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c
index c8d18dc..155c2f5 100644
--- a/sound/pci/hda/hda_tegra.c
+++ b/sound/pci/hda/hda_tegra.c
@@ -219,7 +219,6 @@  static int hda_tegra_enable_clocks(struct hda_tegra *data)
 	return rc;
 }
 
-#ifdef CONFIG_PM_SLEEP
 static void hda_tegra_disable_clocks(struct hda_tegra *data)
 {
 	clk_disable_unprepare(data->hda2hdmi_clk);
@@ -227,6 +226,7 @@  static void hda_tegra_disable_clocks(struct hda_tegra *data)
 	clk_disable_unprepare(data->hda_clk);
 }
 
+#ifdef CONFIG_PM_SLEEP
 /*
  * power management
  */
@@ -257,7 +257,6 @@  static int hda_tegra_resume(struct device *dev)
 }
 #endif /* CONFIG_PM_SLEEP */
 
-#ifdef CONFIG_PM
 static int hda_tegra_runtime_suspend(struct device *dev)
 {
 	struct snd_card *card = dev_get_drvdata(dev);
@@ -283,7 +282,7 @@  static int hda_tegra_runtime_resume(struct device *dev)
 	int rc;
 
 	rc = hda_tegra_enable_clocks(hda);
-	if (rc != 0)
+	if (rc)
 		return rc;
 	if (chip && chip->running) {
 		hda_tegra_init(hda);
@@ -292,7 +291,6 @@  static int hda_tegra_runtime_resume(struct device *dev)
 
 	return 0;
 }
-#endif /* CONFIG_PM */
 
 static const struct dev_pm_ops hda_tegra_pm = {
 	SET_SYSTEM_SLEEP_PM_OPS(hda_tegra_suspend, hda_tegra_resume)
@@ -555,6 +553,13 @@  static int hda_tegra_probe(struct platform_device *pdev)
 	if (!azx_has_pm_runtime(chip))
 		pm_runtime_forbid(hda->dev);
 
+	/* explicit resume if runtime PM is disabled */
+	if (!pm_runtime_enabled(hda->dev)) {
+		err = hda_tegra_runtime_resume(hda->dev);
+		if (err)
+			goto out_free;
+	}
+
 	schedule_work(&hda->probe_work);
 
 	return 0;
@@ -571,7 +576,14 @@  static void hda_tegra_probe_work(struct work_struct *work)
 	struct platform_device *pdev = to_platform_device(hda->dev);
 	int err;
 
-	pm_runtime_get_sync(hda->dev);
+	err = pm_runtime_get_sync(hda->dev);
+	if (err < 0) {
+		dev_err(hda->dev,
+			"failed in pm_runtime_get_syc with err = %d\n",
+			err);
+		return;
+	}
+
 	err = hda_tegra_first_init(chip, pdev);
 	if (err < 0)
 		goto out_free;
@@ -599,12 +611,13 @@  static void hda_tegra_probe_work(struct work_struct *work)
 
 static int hda_tegra_remove(struct platform_device *pdev)
 {
-	int ret;
-
-	ret = snd_card_free(dev_get_drvdata(&pdev->dev));
 	pm_runtime_disable(&pdev->dev);
+	if (!pm_runtime_status_suspended(&pdev->dev)) {
+		hda_tegra_runtime_suspend(&pdev->dev);
+		pm_runtime_set_suspended(&pdev->dev);
+	}
 
-	return ret;
+	return snd_card_free(dev_get_drvdata(&pdev->dev));
 }
 
 static void hda_tegra_shutdown(struct platform_device *pdev)