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

Jon Hunter
2019-02-04T09:53:43Z

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