[PATCH 3/4] soundwire: intel: exit clock stop mode on system suspend

[Vinod Koul]

On 02-08-21, 11:28, Pierre-Louis Bossart wrote:
> 
> 
> 
> >> 1. In above you are calling resume of child devices first and then intel
> >> device, which sounds reverse, should you not resume intel device first
> >> and then child (codec devices) ?
> >>
> >> 2. What about when resume is invoked by the core for the child devices.
> >> That would be called in the PM resume flow, so why do it here?
> > 
> > I realize it's a complicated sequence, it took us multiple phases to get
> > it right. There are multiple layers between power domain, bus and driver.
> > 
> > The .prepare phase happens before the system suspend phase. Unlike
> > suspend, which progresses from children to parents, the .prepare is
> > handled parent first.
> > 
> > When we do a request_resume of the child device, by construction that
> > also resumes the parent. In other words, if we have multiple codecs on a
> > link, the first iteration of device_for_each_child() will already resume
> > the parent and the first device, and the second iteration will only
> > resume the second device.
> > 
> > What this step does is make sure than when the codec .suspend routine is
> > invoked, the entire bus is already back to full power. I did check
> > privately with Rafael (CC:ed) if this sequence was legit.
> > 
> > We did consider modifying the system suspend callback in codec drivers,
> > so that we would do a pm_runtime resume(). This is functionally
> > equivalent to what we are suggesting here, but we decided not to do so
> > for two main reasons
> > 
> > a) lots of code changes across all codecs for an Intel-specific issue
> > 
> > b) we would need to add a flag so that codec drivers would know in which
> > Intel-specific clock-stop mode the bus was configured. That's not so
> > good either.
> > 
> > It seemed simpler to use to add this .prepare step and test on the Intel
> > clock stop mode before doing a pm_runtime_resume for all codecs.

Ack, the code looks neat. But glancing at it, reader might get confused
about the sequencing done here.. It is not very obvious, so consider
adding this to changelog or driver comments. It will be helpful

> 
> Note that we could invert the two parts and do a parent resume first,
> and a loop for all children second. It's completely equivalent, but
> might be less convoluted to understand without any implicit behavior
> assumed.

Agree, it would be redundant as PM core would take care of it. maybe
add a comment so that it is explicit

-- 
~Vinod