On Tue, Oct 29, 2019 at 3:10 AM Cezary Rojewski <cezary.rojewski@xxxxxxxxx>
wrote:
> On 2019-10-26 00:41, Pierre-Louis Bossart wrote:
> > From: Keyon Jie <yang.jie@xxxxxxxxxxxxxxx>
> >
> > Adding helper to implement setting dsp to d0i3 or d0i0 status, this will
> > be needed for driver D0ix support.
> >
> > Signed-off-by: Keyon Jie <yang.jie@xxxxxxxxxxxxxxx>
> > Signed-off-by: Pierre-Louis Bossart <
> pierre-louis.bossart@xxxxxxxxxxxxxxx>
>
> > +static int hda_dsp_wait_d0i3c_done(struct snd_sof_dev *sdev, int retry)
> > +{
> > + struct hdac_bus *bus = sof_to_bus(sdev);
> > +
> > + while (snd_hdac_chip_readb(bus, VS_D0I3C) & SOF_HDA_VS_D0I3C_CIP) {
> > + if (!retry--)
> > + return -ETIMEDOUT;
> > + usleep_range(10, 15);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +int hda_dsp_set_power_state(struct snd_sof_dev *sdev,
> > + enum sof_d0_substate d0_substate)
> > +{
> > + struct hdac_bus *bus = sof_to_bus(sdev);
> > + int retry = 50;
> > + int ret;
> > + u8 value;
> > +
> > + /* Write to D0I3C after Command-In-Progress bit is cleared */
> > + ret = hda_dsp_wait_d0i3c_done(sdev, retry);
> > + if (ret < 0) {
> > + dev_err(bus->dev, "CIP timeout before update D0I3C!\n");
> > + return ret;
> > + }
> > +
> > + /* Update D0I3C register */
> > + value = d0_substate == SOF_DSP_D0I3 ? SOF_HDA_VS_D0I3C_I3 : 0;
>
> Some indentation or parenthesis would make this cleaner.
>
> > + snd_hdac_chip_updateb(bus, VS_D0I3C, SOF_HDA_VS_D0I3C_I3, value);
> > +
> > + /* Wait for cmd in progress to be cleared before exiting the
> function */
> > + retry = 50;
> > + ret = hda_dsp_wait_d0i3c_done(sdev, retry);
> > + if (ret < 0) {
> > + dev_err(bus->dev, "CIP timeout after D0I3C updated!\n");
> > + return ret;
> > + }
> > +
> > + dev_vdbg(bus->dev, "D0I3C updated, register = 0x%x\n",
> > + snd_hdac_chip_readb(bus, VS_D0I3C));
> > +
> > + return 0;
> > +}
> > +
>
> I believe hda_dsp_wait_d0i3c_done function could have had its argument
> list simplified. "retry" is passed externally and it is always set to
> 50. One could put the "retry" right inside _done function. This or allow
> the caller to modify the sleep period. Another option is to replace
> "retry" with "timeout period" (total timeout, that is) entirely.
>
Cezary,
This has been fixed later in the series to use
the HDA_DSP_REG_POLL_RETRY_COUNT but I agree that this can further be
simplified to just use the macro inside the hda_dsp_wait_d0i3c_done()
instead of passing the argument.
>
> In regard to maintenance, both ret checks (resulting in dev_errs) assume
> -ETIMEOUT outcome on _done failure. If said function gets updated in the
> future, these implicit checks might cause problems.
>
Possibly, but at the moment -ETIMEOUT looks like the correct error to be
reported.
Thanks,
Ranjani
>
> > static int hda_suspend(struct snd_sof_dev *sdev, bool runtime_suspend)
> > {
> > struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata;
> > diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h
> > index ea02bf40cb25..0e7c366b8f71 100644
> > --- a/sound/soc/sof/intel/hda.h
> > +++ b/sound/soc/sof/intel/hda.h
> > @@ -64,6 +64,13 @@
> > #define SOF_HDA_PPCTL_PIE BIT(31)
> > #define SOF_HDA_PPCTL_GPROCEN BIT(30)
> >
> > +/*Vendor Specific Registers*/
>
> Missing spaces.
>
> > +#define SOF_HDA_VS_D0I3C 0x104A
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@xxxxxxxxxxxxxxxx
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
