| Message ID | 20250120101329.3713017-4-Vijendar.Mukunda@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | soundwire: amd: code improvements and new platform support | expand |
On 1/20/25 4:13 AM, Vijendar Mukunda wrote: > Add a conditional check to resume SoundWire manager device, when the > SoundWire manager instance is in the suspended state. > > Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com> > --- > drivers/soundwire/amd_manager.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c > index 60be5805715e..6831b3729db5 100644 > --- a/drivers/soundwire/amd_manager.c > +++ b/drivers/soundwire/amd_manager.c > @@ -850,7 +850,10 @@ static void amd_sdw_update_slave_status(u32 status_change_0to7, u32 status_chang > static void amd_sdw_process_wake_event(struct amd_sdw_manager *amd_manager) > { > dev_dbg(amd_manager->dev, "SoundWire Wake event reported\n"); > - pm_request_resume(amd_manager->dev); > + if (pm_runtime_suspended(amd_manager->dev)) { > + dev_dbg(amd_manager->dev, "Device is in suspend state\n"); > + pm_request_resume(amd_manager->dev); > + } Is this test actually doing something useful? If the device is already active, presumably doing a pm_request_resume() is a no-op. If it's already suspended it does something. Testing the device state is risky with all the asynchronous behavior in SoundWire, best to leave the state checks to be handled inside the pm_runtime core, no? IIRC the only time when such a test in needed is in the system suspend callbacks where specific action needs to be taken if the device is in pm_runtime suspended mode with the clock_stop mode engaged.
On 20/01/25 21:00, Pierre-Louis Bossart wrote: > On 1/20/25 4:13 AM, Vijendar Mukunda wrote: >> Add a conditional check to resume SoundWire manager device, when the >> SoundWire manager instance is in the suspended state. >> >> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com> >> --- >> drivers/soundwire/amd_manager.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c >> index 60be5805715e..6831b3729db5 100644 >> --- a/drivers/soundwire/amd_manager.c >> +++ b/drivers/soundwire/amd_manager.c >> @@ -850,7 +850,10 @@ static void amd_sdw_update_slave_status(u32 status_change_0to7, u32 status_chang >> static void amd_sdw_process_wake_event(struct amd_sdw_manager *amd_manager) >> { >> dev_dbg(amd_manager->dev, "SoundWire Wake event reported\n"); >> - pm_request_resume(amd_manager->dev); >> + if (pm_runtime_suspended(amd_manager->dev)) { >> + dev_dbg(amd_manager->dev, "Device is in suspend state\n"); >> + pm_request_resume(amd_manager->dev); >> + } > Is this test actually doing something useful? > > If the device is already active, presumably doing a pm_request_resume() is a no-op. If it's already suspended it does something. > Testing the device state is risky with all the asynchronous behavior in SoundWire, best to leave the state checks to be handled inside the pm_runtime core, no? > > IIRC the only time when such a test in needed is in the system suspend callbacks where specific action needs to be taken if the device is in pm_runtime suspended mode with the clock_stop mode engaged. Soundwire device resume sequence can be invoked from multiple places same time due to different interrupt sources. We have added this extra condition check to safeguard the resume invoking sequence. Will drop this code and retest our test scenarios.
diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c index 60be5805715e..6831b3729db5 100644 --- a/drivers/soundwire/amd_manager.c +++ b/drivers/soundwire/amd_manager.c @@ -850,7 +850,10 @@ static void amd_sdw_update_slave_status(u32 status_change_0to7, u32 status_chang static void amd_sdw_process_wake_event(struct amd_sdw_manager *amd_manager) { dev_dbg(amd_manager->dev, "SoundWire Wake event reported\n"); - pm_request_resume(amd_manager->dev); + if (pm_runtime_suspended(amd_manager->dev)) { + dev_dbg(amd_manager->dev, "Device is in suspend state\n"); + pm_request_resume(amd_manager->dev); + } writel(0x00, amd_manager->acp_mmio + ACP_SW_WAKE_EN(amd_manager->instance)); writel(0x00, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_8TO11); }
Add a conditional check to resume SoundWire manager device, when the SoundWire manager instance is in the suspended state. Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com> --- drivers/soundwire/amd_manager.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)