Hi Sylwester,
On Sunday 15 May 2011 11:33:14 Sylwester Nawrocki wrote:
> On 05/14/2011 05:29 PM, Laurent Pinchart wrote:
> > On Wednesday 11 May 2011 17:17:10 Sylwester Nawrocki wrote:
[snip]
> >> +static int s5pcsis_suspend(struct device *dev)
> >> +{
> >> + struct s5p_platform_mipi_csis *pdata = dev->platform_data;
> >> + struct platform_device *pdev = to_platform_device(dev);
> >> + struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> >> + struct csis_state *state = sd_to_csis_state(sd);
> >> + int ret;
> >> +
> >> + v4l2_dbg(1, debug, sd, "%s: flags: 0x%x\n",
> >> + __func__, state->flags);
> >> +
> >> + mutex_lock(&state->lock);
> >> + if (state->flags& ST_POWERED) {
> >> + s5pcsis_stop_stream(state);
> >> + ret = pdata->phy_enable(state->pdev, false);
> >> + if (ret)
> >> + goto unlock;
> >> +
> >> + if (state->supply&& regulator_disable(state->supply))
> >> + goto unlock;
> >> +
> >> + clk_disable(state->clock[CSIS_CLK_GATE]);
> >> + state->flags&= ~ST_POWERED;
> >> + }
> >> + state->flags |= ST_SUSPENDED;
> >> + unlock:
> >> + mutex_unlock(&state->lock);
> >> + return ret ? -EAGAIN : 0;
> >> +}
> >> +
> >> +static int s5pcsis_resume(struct device *dev)
> >> +{
> >> + struct s5p_platform_mipi_csis *pdata = dev->platform_data;
> >> + struct platform_device *pdev = to_platform_device(dev);
> >> + struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> >> + struct csis_state *state = sd_to_csis_state(sd);
> >> + int ret = 0;
> >> +
> >> + v4l2_dbg(1, debug, sd, "%s: flags: 0x%x\n",
> >> + __func__, state->flags);
> >> +
> >> + mutex_lock(&state->lock);
> >> + if (!(state->flags& ST_SUSPENDED))
> >> + goto unlock;
> >> +
> >> + if (!(state->flags& ST_POWERED)) {
> >
> > If the device wasn't powered before being suspended, it should stay
> > powered off.
>
> I'm not sure, shortly after system wide resume the device is powered off by
> PM runtime core anyway.
> There is no other means in this driver to enable power except using
> pm_runtime_* calls. The device is being powered on or off only through
> these runtime PM helpers, i.e. s5pcsis_resume/s5pcsis_suspend.
> (full source can be found here: http://tinyurl.com/6fozx34)
OK, it should be fine then.
> The pm_runtime_resume helper is guaranteed by the PM core to be called only
> on device in 'suspended' state.
>
> From Documentation/power/runtime_pm.txt:
> " ...
> * Once the subsystem-level resume callback has completed successfully, the
> PM core regards the device as fully operational, which means that the
> device _must_ be able to complete I/O operations as needed. The run-time
> PM status of the device is then 'active'.
> ..."
>
> If s5pcsis_resume would return 0 without really enabling device clocks and
> the external voltage regulator then the runtime PM core idea about the
> device's state would be wrong.
>
> I'm not a PM expert but documentation says that it's better to leave
> device fully functional after system wide driver's resume() helper returns.
>
> From Documentation/power/devices.txt:
>
> "...
> When resuming from standby or memory sleep, the phases are:
> resume_noirq, resume, complete.
> (...)
> At the end of these phases, drivers should be as functional as they were
> before suspending: I/O can be performed using DMA and IRQs, and the
> relevant clocks are gated on. Even if the device was in a low-power state
> before the system sleep because of runtime power management, afterwards it
> should be back in its full-power state. There are multiple reasons why
> it's best to do this; they are discussed in more detail in
> Documentation/power/runtime_pm.txt.
> ..."
>
> Unfortunately there seem to be no standard way to instruct the PM core to
> enable power of a few (I2C/client platform) devices forming the video
> pipeline in an arbitrary order. The parent devices of my platforms devices
> are already the power domain devices.
>
> So it might be a good idea to forbid enabling sub-device's power when
> the host driver is not using it, to have full control of the pipeline
> devices power enable sequence at any time.
>
> I perhaps need to isolate functions out from of s5pcsis_resume/suspend and
> then call that in s_power op and s5pcsis_resume/suspend. Don't really like
> this idea though... It would virtually render pm_runtime_* calls unusable
> in this sub-device driver, those would make sense only in the host driver..
I think using the pm_runtime_* calls make sense, they could replace the subdev
s_power operation in the long term. We'll need to evaluate that (I'm not sure
if runtime PM is available on all platforms targetted by V4L2 for instance).
> I just wanted to put all what is needed to control device's power in the PM
> helpers and then use pm_runtime_* calls where required.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
