On Mon, 04 Feb 2019 11:04:50 +0100,
Jon Hunter wrote:
>
>
> On 04/02/2019 08:16, Sameer Pujar wrote:
>
> ...
>
> > Objective is to have things working with or without CONFIG_PM enabled.
> > From previous comments and discussions it appears that there is mixed
> > response
> > for calling hda_tegra_runtime_resume() or runtime PM APIs in probe()
> > call. Need
> > to have consensus regarding the best practice to be followed, which
> > would eventually
> > can be used in other drivers too.
> >
> > Rafael is suggesting to use CONFIG_PM check to do manual setup or
> > runtime PM setup in probe,
> > which would bring back the earlier above mentioned concern.
> >
> > if (IS_ENABLED(CONFIG_PM)) {
> > do setup based on pm-runtime
> > } else {
> > do manual setup
> > }
> > Both if/else might end up doing the same here.
> > Do we really need CONFIG_PM check here?
> >
> > Instead does below proposal appear fine?
> >
> > probe() {
> > hda_tegra_enable_clock();
> > }
> >
> > probe_work() {
> > /* hda setup */
> > . . .
> > pm_runtime_set_active(); /* initial state as active */
> > pm_runtime_enable();
> > return;
> > }
>
> I believe that this still does not work, because if there is a
> power-domain that needs to be turned on, this does not guarantee this.
> So I think that you need to call pm_runtime_get ...
>
> probe() {
> if (!IS_ENABLED(CONFIG_PM))
> hda_tegra_enable_clock();
> }
>
>
> probe_work() {
> /* hda setup */
> . . .
> pm_runtime_enable();
> pm_runtime_get_sync();
> return;
> }
>
> The alternative here could just be ...
>
> probe() {
> pm_runtime_enable();
> if (!pm_runtime_enabled())
> ret = hda_tegra_enable_clock();
> else
> ret = pm_runtime_get_sync();
>
> if (ret < 0) {
> ...
> }
> }
>
> Very similar to what I was saying to begin with but not call the
> pm_runtime_resume handler directly. Which I believe was Iwai-san's dislike.
Yes, exactly, what bothered me was really a nuance: calling
hda_tegra_runtime_resume() there makes the code misleading (or
confusing) as if the runtime PM were mandatory.
I hoped there could be some standard idiom for this expression, but
apparently there isn't any, so far...
Obviously the easiest option is to enforce the dependency on
CONFIG_PM. Would there be any platform that needs to run without PM,
practically seen...?
But, now after lengthy discussions and the clarification of the
current situation, I have no strong opinion on this any longer.
So I leave the decision to you guys, and I'll apply it as-is.
thanks,
Takashi
