On Thu, Jan 30, 2020 at 7:07 AM Pierre-Louis Bossart < pierre-louis.bossart@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
