Re: [alsa-devel] [PATCH 5/9] ASoC: SOF: Move DSP power state transitions to platform-specific ops

From: "Sridharan, Ranjani" <ranjani.sridharan@intel.com>
To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Cc: "Takashi Iwai" <tiwai@suse.de>,
	Linux-ALSA <alsa-devel@alsa-project.org>,
	"Mark Brown" <broonie@kernel.org>,
	"Ranjani Sridharan" <ranjani.sridharan@linux.intel.com>,
	"Amadeusz Sławiński" <amadeuszx.slawinski@linux.intel.com>
Subject: Re: [alsa-devel] [PATCH 5/9] ASoC: SOF: Move DSP power state transitions to platform-specific ops
Date: Thu, 30 Jan 2020 07:33:07 -0800	[thread overview]
Message-ID: <CAFQqKeV5grgKvObGf-9vYb-9M4ddq+vK=VCrPjbV9kKc1s_-Cg@mail.gmail.com> (raw)
In-Reply-To: <615262ec-2515-ef6f-1ed4-8c42170490c4@linux.intel.com>

On Thu, Jan 30, 2020 at 7:07 AM Pierre-Louis Bossart <
pierre-louis.bossart@linux.intel.com> wrote:

>
> >> @@ -495,13 +645,27 @@ int hda_dsp_resume(struct snd_sof_dev *sdev)
> >>       }
> >>       /* init hda controller. DSP cores will be powered up during fw
> >> boot */
> >> -    return hda_resume(sdev, false);
> >> +    ret = hda_resume(sdev, false);
> >> +    if (ret < 0)
> >> +        return ret;
> >> +
> >> +    hda_dsp_set_power_state(sdev, &target_state);
> >
> > Return value  of hda_dsp_set_power_state() seems to be checked or
> > directly returned in other functions, any reason to not do it here?
>
> Good point Amadeusz, not sure why. And looking at the code, I am not
> sure either why we don't use the abstraction w/ .set_power_state() ?
>
> intel/apl.c:    .set_power_state        = hda_dsp_set_power_state,
> intel/cnl.c:    .set_power_state        = hda_dsp_set_power_state,
>
>
> git grep snd_sof_dsp_set_power_state
> sof/ipc.c:      ret = snd_sof_dsp_set_power_state(ipc->sdev,
> &target_state);
> sof/ops.h:snd_sof_dsp_set_power_state(struct snd_sof_dev *sdev,
>
> If the code can be platform-specific, we shouldn't make a direct call
> but go through the platform indirection. it's fine for now since the
> same routine is used in all cases but it's not scalable/future-proof.
>
> Ranjani?
>
Hi Amadeusz/Pierre,

This seems like a miss. We should have returned the value of
hda_set_power_state() directly here.

And to address your point, Pierre. This is the platform-specific code, so
the use of hda_dsp_set_power_state() should be permitted no? Whereas, the
part that uses this function in ipc.c is not platform-specific.
Thanks,
Ranjani
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel