linux-kernel - Re: [PATCH v3 18/35] media: camss: Add basic runtime PM support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180725122411.bgemjrp577lfmje4@paasikivi.fi.intel.com>
Date:   Wed, 25 Jul 2018 15:24:11 +0300
From:   Sakari Ailus <sakari.ailus@...ux.intel.com>
To:     Todor Tomov <todor.tomov@...aro.org>
Cc:     mchehab@...nel.org, hans.verkuil@...co.com,
        laurent.pinchart+renesas@...asonboard.com,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 18/35] media: camss: Add basic runtime PM support

Hi Todor,

On Wed, Jul 25, 2018 at 01:01:31PM +0300, Todor Tomov wrote:
> Hi Sakari,
> 
> Thank you for review.
> 
> On 24.07.2018 15:49, Sakari Ailus wrote:
> > Hi Todor,
> > 
> > On Mon, Jul 23, 2018 at 02:02:35PM +0300, Todor Tomov wrote:
> >> There is a PM domain for each of the VFE hardware modules. Add
> >> support for basic runtime PM support to be able to control the
> >> PM domains. When a PM domain needs to be powered on - a device
> >> link is created. When a PM domain needs to be powered off -
> >> its device link is removed. This allows separate and
> >> independent control of the PM domains.
> >>
> >> Suspend/Resume is still not supported.
> >>
> >> Signed-off-by: Todor Tomov <todor.tomov@...aro.org>
> >> ---
> >>  drivers/media/platform/qcom/camss/camss-csid.c   |  4 ++
> >>  drivers/media/platform/qcom/camss/camss-csiphy.c |  5 ++
> >>  drivers/media/platform/qcom/camss/camss-ispif.c  | 19 ++++++-
> >>  drivers/media/platform/qcom/camss/camss-vfe.c    | 13 +++++
> >>  drivers/media/platform/qcom/camss/camss.c        | 63 ++++++++++++++++++++++++
> >>  drivers/media/platform/qcom/camss/camss.h        | 11 +++++
> >>  6 files changed, 113 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c
> >> index 627ef44..ea2b0ba 100644
> >> --- a/drivers/media/platform/qcom/camss/camss-csid.c
> >> +++ b/drivers/media/platform/qcom/camss/camss-csid.c
> >> @@ -13,6 +13,7 @@
> >>  #include <linux/kernel.h>
> >>  #include <linux/of.h>
> >>  #include <linux/platform_device.h>
> >> +#include <linux/pm_runtime.h>
> >>  #include <linux/regulator/consumer.h>
> >>  #include <media/media-entity.h>
> >>  #include <media/v4l2-device.h>
> >> @@ -316,6 +317,8 @@ static int csid_set_power(struct v4l2_subdev *sd, int on)
> >>  	if (on) {
> >>  		u32 hw_version;
> >>  
> >> +		pm_runtime_get_sync(dev);
> >> +
> >>  		ret = regulator_enable(csid->vdda);
> > 
> > Shouldn't the regulator be enabled in the runtime_resume callback instead?
> 
> Ideally - yes, but it becomes more complex (different pipelines are possible
> and we have only one callback) so (at least for now) I have left it as it is
> and stated in the commit message that suspend/resume is still not supported.

Ack.

-- 
Sakari Ailus
sakari.ailus@...ux.intel.com