Skip to content

[RFC DO NOT MERGE]SOF D0ix support#1154

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
keyonjie wants to merge 14 commits into from
Closed

[RFC DO NOT MERGE]SOF D0ix support #1154

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
9a64948
ASoC: SOF: add a field to store the current D0 state of DSP
keyonjie Aug 23, 2019
693ce25
ASoC: SOF: reset default d0_state at probe/suspend/resume
keyonjie Aug 23, 2019
98774ab
ASoC: SOF: add set_state() to dsp_ops for d0ix state update on platform
keyonjie Aug 23, 2019
b5f97d6
ASoC: SOF: Intel: hda-dsp: Add helper for setting dsp d0 substate
keyonjie Jul 31, 2019
809b403
ASoC: SOF: Intel: cnl: add set_state() ops
keyonjie Aug 23, 2019
eb9b775
ASoC: SOF: Intel: apl: add d0ix_update() ops
keyonjie Aug 23, 2019
25ee229
ASoC: SOF: add flag to snd_sof_pcm_stream for low power stream
keyonjie Aug 1, 2019
bfc94c1
ASoC: SOF: token: add token for low power PCM.
keyonjie Aug 1, 2019
c8ce226
ASoC: SOF: topology: parse and store d0i3_compatible flag
keyonjie Aug 1, 2019
f164f74
ASoC: SOF: ipc: introduce message for setting DSP state
keyonjie Aug 30, 2019
c0c413d
ASoC: SOF: pm: add helpers for setting state for ADSP
keyonjie Aug 23, 2019
4bfd3bf
ASoC: SOF: add a platform driver for D0ix power management
keyonjie Sep 6, 2019
0db26fd
ASoC: SOF: core: register/unregister d0ix device at probe/remove
keyonjie Sep 6, 2019
ed4cd5e
ASoC: SOF: pcm: get/put d0ix device at normal substream open/close
keyonjie Sep 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions include/sound/sof.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -48,6 +48,7 @@ struct snd_sof_pdata {

/* machine */
struct platform_device *pdev_mach;
struct platform_device *pdev_d0ix;
const struct snd_soc_acpi_mach *machine;

void *hw_pdata;
Expand Down
1 change: 1 addition & 0 deletions include/sound/sof/header.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -74,6 +74,7 @@
#define SOF_IPC_PM_CLK_GET SOF_CMD_TYPE(0x005)
#define SOF_IPC_PM_CLK_REQ SOF_CMD_TYPE(0x006)
#define SOF_IPC_PM_CORE_ENABLE SOF_CMD_TYPE(0x007)
#define SOF_IPC_PM_SET_STATE SOF_CMD_TYPE(0x008)

@plbossartplbossartSep 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SET_POWER_STATE


/* component runtime config - multiple different types */
#define SOF_IPC_COMP_SET_VALUE SOF_CMD_TYPE(0x001)
Expand Down
4 changes: 4 additions & 0 deletions include/uapi/sound/sof/tokens.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -114,4 +114,8 @@
#define SOF_TKN_IMX_ESAI_FIRST_TOKEN 1100
/* TODO: Add ESAI tokens */

/* Stream */
#define SOF_TKN_STREAM_PLAYBACK_COMPATIBLE_D0I3 1200
#define SOF_TKN_STREAM_CAPTURE_COMPATIBLE_D0I3 1201

#endif
2 changes: 1 addition & 1 deletion sound/soc/sof/Makefile
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -3,7 +3,7 @@
ccflags-y += -DDEBUG

snd-sof-objs := core.o ops.o loader.o ipc.o pcm.o pm.o debug.o topology.o\
control.o trace.o utils.o
control.o trace.o utils.o pm_d0ix.o

snd-sof-pci-objs := sof-pci-dev.o
snd-sof-acpi-objs := sof-acpi-dev.o
Expand Down
17 changes: 17 additions & 0 deletions sound/soc/sof/core.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -402,9 +402,22 @@ static int sof_probe_continue(struct snd_sof_dev *sdev)
dev_dbg(sdev->dev, "created machine %s\n",
dev_name(&plat_data->pdev_mach->dev));

plat_data->pdev_d0ix =
platform_device_register_data(sdev->dev, "sof_d0ix",
PLATFORM_DEVID_NONE, sdev,
sizeof(*sdev));

if (IS_ERR(plat_data->pdev_d0ix)) {
ret = PTR_ERR(plat_data->pdev_d0ix);
goto fw_run_err;
}

if (plat_data->sof_probe_complete)
plat_data->sof_probe_complete(sdev->dev);

/* initialize default D0 sub-state */
sdev->d0_substate = SOF_DSP_D0I0;

@plbossartplbossartSep 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would do the initialization earlier, since the sof_probe_complete is where you typically enable pm_runtime

@keyonjiekeyonjieSep 10, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense.

return 0;

#if !IS_ENABLED(CONFIG_SND_SOC_SOF_PROBE_WORK_QUEUE)
Expand DownExpand Up@@ -513,6 +526,10 @@ int snd_sof_device_remove(struct device *dev)
snd_sof_free_debug(sdev);
snd_sof_free_trace(sdev);

/* Unregister D0Ix power management device */
if (!IS_ERR_OR_NULL(pdata->pdev_d0ix))
platform_device_unregister(pdata->pdev_d0ix);

/*
* Unregister machine driver. This will unbind the snd_card which
* will remove the component driver and unload the topology
Expand Down
1 change: 1 addition & 0 deletions sound/soc/sof/intel/apl.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -97,6 +97,7 @@ const struct snd_sof_dsp_ops sof_apl_ops = {
.runtime_resume = hda_dsp_runtime_resume,
.runtime_idle = hda_dsp_runtime_idle,
.set_hw_params_upon_resume = hda_dsp_set_hw_params_upon_resume,
.set_state = hda_dsp_set_state,

@ranj063ranj063Sep 9, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@keyonjie incorrect commit title and message. Please align with cnl commit.

@keyonjiekeyonjieSep 10, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

};
EXPORT_SYMBOL(sof_apl_ops);

Expand Down
1 change: 1 addition & 0 deletions sound/soc/sof/intel/cnl.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -255,6 +255,7 @@ const struct snd_sof_dsp_ops sof_cnl_ops = {
.runtime_resume = hda_dsp_runtime_resume,
.runtime_idle = hda_dsp_runtime_idle,
.set_hw_params_upon_resume = hda_dsp_set_hw_params_upon_resume,
.set_state = hda_dsp_set_state,
};
EXPORT_SYMBOL(sof_cnl_ops);

Expand Down
46 changes: 46 additions & 0 deletions sound/soc/sof/intel/hda-dsp.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -282,6 +282,52 @@ void hda_dsp_ipc_int_disable(struct snd_sof_dev *sdev)
HDA_DSP_REG_HIPCCTL_BUSY | HDA_DSP_REG_HIPCCTL_DONE, 0);
}

static void hda_dsp_wait_d0i3c_done(struct snd_sof_dev *sdev, int *retry)

@plbossartplbossartSep 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this handles retries so should return an error if there is no change after all the retries

{
struct hdac_bus *bus = sof_to_bus(sdev);
u8 reg;

reg = snd_hdac_chip_readb(bus, VS_D0I3C);
while ((reg & SOF_HDA_VS_D0I3C_CIP) && --*retry) {
usleep_range(10, 15);
reg = snd_hdac_chip_readb(bus, VS_D0I3C);

@lyakhlyakhSep 4, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pattern occurs so often - when you have to perform a calculation to verify a condition and then repeat an action while the condition holds, so you end up with

test = calculate_test();
while (must_loop(test)) {
action(test);
test = calculate_test();
}

that maybe we have to add an exception to kernel coding style guidelines to allow

while (must_loop(test = calculate_test()))
action(test);

but for now we have to do the former...

@keyonjiekeyonjieSep 6, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what's your suggestion here @lyakh ?

@lyakhlyakhSep 10, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@keyonjie write to LKML to suggest add this exception to the Coding Style :-)

}
}

int hda_dsp_set_state(struct snd_sof_dev *sdev,

@plbossartplbossartSep 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set_power_state

enum sof_d0_substate d0_substate)
{
struct hdac_bus *bus = sof_to_bus(sdev);
u8 value;
int retry = 50;

/* Write to D0I3C after Command-In-Progress bit is cleared */
hda_dsp_wait_d0i3c_done(sdev, &retry);

@ranj063ranj063Sep 9, 2019•
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@keyonjie why do we need to pass this as a pointer? Why not just initialize retry in the other function and check the return value for hda_dsp_wait_d0i3c_done() here. It seems a bit of an overkill to pass a pointer for no good reason

@ranj063ranj063Sep 9, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@keyonjie do we need to add a check to see if the previous substate is already the requested state and return in tthat case?

@lyakhlyakhSep 10, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree, remove the parameter and return -ETIMEDOUT if retries exhausted, maybe also use a macro for the retry count


if (!retry) {
dev_err(bus->dev, "CIP timeout before update D0I3C!\n");
return -ETIMEDOUT;

@lyakhlyakhSep 10, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so here you'll just propagate the error back to the caller

}

/* Update D0I3C register */
value = d0_substate == SOF_DSP_D0I3 ? SOF_HDA_VS_D0I3C_I3 : 0;
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;
hda_dsp_wait_d0i3c_done(sdev, &retry);

if (!retry) {
dev_err(bus->dev, "CIP timeout after D0I3C updated!\n");
return -ETIMEDOUT;
}

@plbossartplbossartSep 3, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be worth having a helper to read the register, it's not very nice as is.

@keyonjiekeyonjieSep 4, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I can split it out, make something like hda_dsp_wait_d0i3c_done(u32 retry).


dev_dbg(bus->dev, "D0I3C updated, register = 0x%x\n",
snd_hdac_chip_readb(bus, VS_D0I3C));

return 0;
}

static int hda_suspend(struct snd_sof_dev *sdev, bool runtime_suspend)
{
struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata;
Expand Down
7 changes: 7 additions & 0 deletions sound/soc/sof/intel/hda.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -65,6 +65,10 @@
#define SOF_HDA_PPCTL_PIE BIT(31)
#define SOF_HDA_PPCTL_GPROCEN BIT(30)

/* D0I3C Register fields */
#define SOF_HDA_VS_D0I3C_CIP BIT(0) /* Command-In-Progress */
#define SOF_HDA_VS_D0I3C_I3 BIT(2) /* D0i3 enable bit */

/* DPIB entry size: 8 Bytes = 2 DWords */
#define SOF_HDA_DPIB_ENTRY_SIZE 0x8

Expand DownExpand Up@@ -452,6 +456,9 @@ int hda_dsp_core_reset_power_down(struct snd_sof_dev *sdev,
void hda_dsp_ipc_int_enable(struct snd_sof_dev *sdev);
void hda_dsp_ipc_int_disable(struct snd_sof_dev *sdev);

int hda_dsp_set_state(struct snd_sof_dev *sdev,

@plbossartplbossartSep 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion:
set_power_state(sdev, dstate, d0_substate).
That way in a future PR we can deprecate CTX_SAVE/RESTORE and use this set_power_state instead

@keyonjiekeyonjieSep 9, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, thanks.

@keyonjiekeyonjieSep 10, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, @plbossart you didn't get my point here. This helper is only aimed to set d0i3c register, not including sending IPC to FW, so the CTX_SAVE/RESTORE should not belong to here.

enum sof_d0_substate d0_substate);

int hda_dsp_suspend(struct snd_sof_dev *sdev);
int hda_dsp_resume(struct snd_sof_dev *sdev);
int hda_dsp_runtime_suspend(struct snd_sof_dev *sdev);
Expand Down
9 changes: 9 additions & 0 deletions sound/soc/sof/ops.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -193,6 +193,15 @@ static inline int snd_sof_dsp_set_clk(struct snd_sof_dev *sdev, u32 freq)
return 0;
}

static inline int snd_sof_dsp_set_state(struct snd_sof_dev *sdev,

@plbossartplbossartSep 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set_power_state

@keyonjiekeyonjieSep 10, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.

enum sof_d0_substate d0_substate)
{
if (sof_ops(sdev)->set_state)
return sof_ops(sdev)->set_state(sdev, d0_substate);

return 0;
}

/* debug */
static inline void snd_sof_dsp_dbg_dump(struct snd_sof_dev *sdev, u32 flags)
{
Expand Down
17 changes: 15 additions & 2 deletions sound/soc/sof/pcm.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -422,6 +422,7 @@ static int sof_pcm_open(struct snd_pcm_substream *substream)
struct snd_soc_component *component =
snd_soc_rtdcom_lookup(rtd, DRV_NAME);
struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(component);
struct snd_sof_pdata *plat_data = sdev->pdata;
struct snd_sof_pcm *spcm;
struct snd_soc_tplg_stream_caps *caps;
int ret;
Expand DownExpand Up@@ -486,10 +487,16 @@ static int sof_pcm_open(struct snd_pcm_substream *substream)
spcm->prepared[substream->stream] = false;

ret = snd_sof_pcm_platform_open(sdev, substream);
if (ret < 0)
if (ret < 0) {
dev_err(sdev->dev, "error: pcm open failed %d\n", ret);
return ret;
}

if (!spcm->stream[substream->stream].d0i3_compatible)
pm_runtime_get_sync(&plat_data->pdev_d0ix->dev);

@plbossartplbossartSep 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, this is officially horrible.

NAK NAK NAK


return 0;

return ret;
}

static int sof_pcm_close(struct snd_pcm_substream *substream)
Expand All@@ -498,6 +505,7 @@ static int sof_pcm_close(struct snd_pcm_substream *substream)
struct snd_soc_component *component =
snd_soc_rtdcom_lookup(rtd, DRV_NAME);
struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(component);
struct snd_sof_pdata *plat_data = sdev->pdata;
struct snd_sof_pcm *spcm;
int err;

Expand All@@ -522,6 +530,11 @@ static int sof_pcm_close(struct snd_pcm_substream *substream)
*/
}

if (!spcm->stream[substream->stream].d0i3_compatible) {
pm_runtime_mark_last_busy(&plat_data->pdev_d0ix->dev);
pm_runtime_put_autosuspend(&plat_data->pdev_d0ix->dev);
}

@lyakhlyakhSep 4, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually would rather comment to the commit message of this patch - it looks very much like re-inventing runtime-PM to me, can we convert this to use the standard RT-PM APIs?

@plbossartplbossartSep 4, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it feels like re-invention but no it's not. pm_runtime is only about DO-D3. When the device is in D0i3 (which is platform-specific), pm_runtime considers the device as 'active'. So indeed we need to implement a custom usage/refcount and decision making. It's not necessarily elegant but it's not generic either so the ability to rely on a common framework for stuff that's hardware/firmware specific is limited.

@keyonjiekeyonjieSep 5, 2019•
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @lyakh and @plbossart I think using RT-PM API is doable, it only define runtime_status as either suspended or active, not limit them to be D3/D0, we can make it suspended==d0i3 and active==d0i0.

I did a prototype code to have a quick try today, it works fine. What I did is:

  1. Introduce a new platform device(child of pci device) with PM ops(runtime suspend/resume).
  2. runtime get at pcm_open() and put at pcm_close() for normal substream only.
  3. do d0i3 entry at runtime_suspend(), d0i3 exit at runtime_resume().

@plbossart if you can accept this solution, I will clean up and update it to the PR tomorrow.

@lyakhlyakhSep 10, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@keyonjie I see @plbossart's point that RTPM is only about D0 / D3, but still doing what we want to do "from scratch" doesn't seem very pretty to me. At the very least I'd look at using kref instead of doing manual use-counting. But even better I'd talk to Linux PM gurus. Hint:: @rjwysocki is at Intel

return 0;
}

Expand Down
35 changes: 35 additions & 0 deletions sound/soc/sof/pm.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -326,6 +326,9 @@ static int sof_resume(struct device *dev, bool runtime_resume)
"error: ctx_restore ipc error during resume %d\n",
ret);

/* initialize default D0 sub-state */
sdev->d0_substate = SOF_DSP_D0I0;

return ret;
}

Expand DownExpand Up@@ -419,3 +422,35 @@ int snd_sof_suspend(struct device *dev)
return sof_suspend(dev, false);
}
EXPORT_SYMBOL(snd_sof_suspend);

int snd_sof_set_dsp_state(struct snd_sof_dev *sdev,
enum sof_d0_substate d0_substate)
{
int ret;

dev_dbg(sdev->dev, "setting ADSP D0 substate:%d\n", d0_substate);

/* Todo: destroy/reload unused pipelines/widgets in FW */

/* do platform specific set_state */
ret = snd_sof_dsp_set_state(sdev, d0_substate);
if (ret < 0) {
dev_err(sdev->dev,
"error: d0i3_exit ipc error %d\n", ret);

@plbossartplbossartSep 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove d0i3, this this set_power_state ipc error

return ret;
}

/* sending set_state IPC */
ret = sof_send_pm_ipc(sdev, SOF_IPC_PM_SET_STATE | d0_substate);

@plbossartplbossartSep 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and isn't it the other way around, you first send the IPC then set the D0i3C bit?

if (ret < 0) {
dev_err(sdev->dev,
"error: d0i3_exit ipc error %d\n", ret);

@plbossartplbossartSep 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

return ret;
}

/* update dsp D0 sub-state */
sdev->d0_substate = d0_substate;

return 0;
}
EXPORT_SYMBOL(snd_sof_set_dsp_state);
84 changes: 84 additions & 0 deletions sound/soc/sof/pm_d0ix.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
//
// This file is provided under a dual BSD/GPLv2 license. When using or
// redistributing this file, you may do so under either license.
//
// Copyright(c) 2019 Intel Corporation. All rights reserved.
//
// Author: Keyon Jie <yang.jie@linux.intel.com>
//

#include <linux/device.h>
#include <linux/module.h>
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
#include <sound/sof.h>
#include "ops.h"

#ifdef CONFIG_PM
static int sof_d0ix_suspend(struct device *dev)
{
struct snd_sof_dev *sdev = dev_get_platdata(dev);

dev_dbg(dev, "Suspending to D0i3...\n");

return snd_sof_set_dsp_state(sdev, SOF_DSP_D0I3);
}

static int sof_d0ix_resume(struct device *dev)
{
struct snd_sof_dev *sdev = dev_get_platdata(dev);

dev_dbg(dev, "Resuming from D0i3...\n");

return snd_sof_set_dsp_state(sdev, SOF_DSP_D0I0);
}
#else
#define sof_d0ix_suspend NULL
#define sof_d0ix_resume NULL
#endif

static int pm_d0ix_probe(struct platform_device *pdev)
{
/* allow runtime_pm */
pm_runtime_set_autosuspend_delay(&pdev->dev, SND_SOF_D0I3_DELAY_MS);
pm_runtime_use_autosuspend(&pdev->dev);

pm_runtime_allow(&pdev->dev);

pm_runtime_enable(&pdev->dev);

pm_runtime_mark_last_busy(&pdev->dev);

pm_runtime_put_noidle(&pdev->dev);

dev_dbg(&pdev->dev, "%s done.\n", __func__);
return 0;
}

static int pm_d0ix_remove(struct platform_device *pdev)
{
return 0;
}

@ranj063ranj063Sep 9, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@keyonjie is remove() really needed if you dont do anything here?


const struct dev_pm_ops d0ix_pm_ops = {
SET_RUNTIME_PM_OPS(sof_d0ix_suspend, sof_d0ix_resume, NULL)
};

static struct platform_driver sof_d0ix_driver = {
.driver = {
.name = "sof_d0ix",
.pm = &d0ix_pm_ops,
},

.probe = pm_d0ix_probe,
.remove = pm_d0ix_remove,
};

module_platform_driver(sof_d0ix_driver);

@plbossartplbossartSep 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding platform devices is not really something we want. We cannot control when the probe happens and in general it adds more issues.

I am inches away from stating 'NAK'

@lyakhlyakhSep 10, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@keyonjie I'm also not too keen on adding a platform device just for the purpose of PM. It doesn't seem to be very logical to me. I'll comment with more explanations where @plbossart has explained why RTPM isn't the right approach here.


MODULE_DESCRIPTION("SOF D0Ix driver");
MODULE_AUTHOR("Keyon Jie");
MODULE_LICENSE("Dual BSD/GPL");
MODULE_ALIAS("platform:sof_d0ix");

Loading