Re: [PATCH v2] ALSA: hda/tegra: enable clock during probe

[Jon Hunter]

On 04/02/2019 08:45, Thierry Reding wrote:

...

> The idea was, as I was saying below, to reuse dev_pm_ops even if
> !CONFIG_PM. So pm_runtime_enable() could be something like this:
> 
> 	pm_runtime_enable(dev)
> 	{
> 		if (!CONFIG_PM)
> 			if (dev->pm_ops->resume)
> 				dev->pm_ops->resume(dev);
> 
> 		...
> 	}
> 
> But that's admittedly somewhat of a stretch. This could of course be
> made somewhat nicer by adding an explicit variant, say:
> 
> 	pm_runtime_enable_foo(dev)
> 	{
> 		if (!CONFIG_PM && dev->pm_ops->resume)
> 			return dev->pm_ops->resume(dev);
> 
> 		return 0;
> 	}
> 
> Maybe the fact that I couldn't come up with a good name is a good
> indication that this is a bad idea...

How about some new APIs called ...

pm_runtime_enable_get()
pm_runtime_enable_get_sync()
pm_runtime_put_disable() (implies a put_sync)

... and in these APIs we add ...

pm_runtime_enable_get(dev)
{
	if (!CONFIG_PM && dev->pm_ops->resume)
		return dev->pm_ops->resume(dev);

	pm_runtime_enable(dev);
	return pm_runtime_get(dev);
}

>>> This would be somewhat tricky because drivers
>>> usually use SET_RUNTIME_PM_OPS to populate the struct dev_pm_ops and
>>> that would result in an empty structure if !CONFIG_PM, but we could
>>> probably work around that by adding a __SET_RUNTIME_PM_OPS that would
>>> never be compiled out for this kind of case. Or such drivers could even
>>> manually set .runtime_suspend and .runtime_resume to make sure they're
>>> always populated.
>>>
>>> Another way out of this would be to make sure we never run into the case
>>> where runtime PM is disabled. If we always "select PM" on Tegra, then PM
>>> should always be available. But is it guaranteed that runtime PM for the
>>> devices is functional in that case? From a cursory look at the code it
>>> would seem that way.
>>
>> If you select PM, then all of the requisite code should be there.
> 
> We do this on 64-bit ARM, but there had been some pushback when we had
> proposed to do the same thing on 32-bit ARM. I think there were two
> concerns:
> 
> 	1) select PM would force the setting for all platforms on multi-
> 	   platforms builds
> 
> 	2) prevents anyone from disabling PM for debugging purposes
> 
> 1) no longer seems to be valid because Rockchip already selects PM
> unconditionally. I'm not sure if 2) is valid anymore either. I haven't
> run a build with !PM in a very long time and I wouldn't be surprised if
> that was completely broken.
> 
> Maybe we need to try this again since a couple of years have elapsed and
> runtime PM support on Tegra is much more mature at this point.
> 
>> Alternatively, you can make the driver depend on PM.
> 
> That's probably the easiest way out, but to be honest I think I'd prefer
> to just enforce PM and keep things simple.
> 
> Jon, any objections?

None, but seems overkill just for this case.

Cheers
Jon

-- 
nvpublic