From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mga11.intel.com ([192.55.52.93]:58711 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751191Ab2GOXRs (ORCPT ); Sun, 15 Jul 2012 19:17:48 -0400 Message-ID: <50034F97.9060208@linux.intel.com> Date: Mon, 16 Jul 2012 02:17:43 +0300 From: David Cohen MIME-Version: 1.0 To: Laurent Pinchart CC: Guennadi Liakhovetski , linux-media@vger.kernel.org Subject: Re: [PATCH v2 8/9] soc-camera: Add and use soc_camera_power_[on|off]() helper functions References: <1341520728-2707-1-git-send-email-laurent.pinchart@ideasonboard.com> <1341520728-2707-9-git-send-email-laurent.pinchart@ideasonboard.com> In-Reply-To: <1341520728-2707-9-git-send-email-laurent.pinchart@ideasonboard.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: Hi Laurent, Few more comments below :) On 07/05/2012 11:38 PM, Laurent Pinchart wrote: > Instead of forcing all soc-camera drivers to go through the mid-layer to > handle power management, create soc_camera_power_[on|off]() functions > that can be called from the subdev .s_power() operation to manage > regulators and platform-specific power handling. This allows non > soc-camera hosts to use soc-camera-aware clients. > > Signed-off-by: Laurent Pinchart > --- > drivers/media/video/imx074.c | 9 +++ > drivers/media/video/mt9m001.c | 9 +++ > drivers/media/video/mt9m111.c | 52 +++++++++++++----- > drivers/media/video/mt9t031.c | 11 +++- > drivers/media/video/mt9t112.c | 9 +++ > drivers/media/video/mt9v022.c | 9 +++ > drivers/media/video/ov2640.c | 9 +++ > drivers/media/video/ov5642.c | 10 +++- > drivers/media/video/ov6650.c | 9 +++ > drivers/media/video/ov772x.c | 9 +++ > drivers/media/video/ov9640.c | 10 +++- > drivers/media/video/ov9740.c | 15 +++++- > drivers/media/video/rj54n1cb0c.c | 9 +++ > drivers/media/video/soc_camera.c | 83 +++++++++++++++++------------ > drivers/media/video/soc_camera_platform.c | 11 ++++- > drivers/media/video/tw9910.c | 9 +++ > include/media/soc_camera.h | 10 ++++ > 17 files changed, 225 insertions(+), 58 deletions(-) > [snip] > diff --git a/drivers/media/video/ov9740.c b/drivers/media/video/ov9740.c > index 3eb07c2..effd0f1 100644 > --- a/drivers/media/video/ov9740.c > +++ b/drivers/media/video/ov9740.c > @@ -786,16 +786,29 @@ static int ov9740_g_chip_ident(struct v4l2_subdev *sd, > > static int ov9740_s_power(struct v4l2_subdev *sd, int on) > { > + struct i2c_client *client = v4l2_get_subdevdata(sd); > + struct soc_camera_link *icl = soc_camera_i2c_to_link(client); > struct ov9740_priv *priv = to_ov9740(sd); > + int ret; > > - if (!priv->current_enable) > + if (on) { > + ret = soc_camera_power_on(&client->dev, icl); > + if (ret < 0) > + return ret; > + } > + > + if (!priv->current_enable) { > + if (!on) > + soc_camera_power_off(&client->dev, icl); After your changes, this function has 3 if's (one nested) where all of them checks "on" variable due to you need to mix "on" and "priv->current_enable" checks. However, code's traceability is not so trivial. How about if you nest "priv->current_enable" into last "if" and keep only that one? See an incomplete code below: > return 0; > + } > > if (on) { soc_camera_power_on(); if (!priv->current_enable) return; > ov9740_s_fmt(sd, &priv->current_mf); > ov9740_s_stream(sd, priv->current_enable); > } else { > ov9740_s_stream(sd, 0); Execute ov9740_s_stream() conditionally: if (priv->current_enable) { ov9740_s_stream(); priv->current_enable = true; } > + soc_camera_power_off(&client->dev, icl); > priv->current_enable = true; priv->current_enable is set to false when ov9740_s_stream(0) is called then this function sets it back to true afterwards. So, in case you want to have no functional change, it seems to me you should call soc_camera_power_off() after that variable has its original value set back. In this case, even if you don't like my suggestion, you still need to swap those 2 lines above. :) Br, David Cohen > } > > diff --git a/drivers/media/video/rj54n1cb0c.c b/drivers/media/video/rj54n1cb0c.c > index f6419b2..ca1cee7 100644 > --- a/drivers/media/video/rj54n1cb0c.c > +++ b/drivers/media/video/rj54n1cb0c.c > @@ -1180,6 +1180,14 @@ static int rj54n1_s_register(struct v4l2_subdev *sd, > } > #endif > > +static int rj54n1_s_power(struct v4l2_subdev *sd, int on) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(sd); > + struct soc_camera_link *icl = soc_camera_i2c_to_link(client); > + > + return soc_camera_set_power(&client->dev, icl, on); > +} > + > static int rj54n1_s_ctrl(struct v4l2_ctrl *ctrl) > { > struct rj54n1 *rj54n1 = container_of(ctrl->handler, struct rj54n1, hdl); > @@ -1230,6 +1238,7 @@ static struct v4l2_subdev_core_ops rj54n1_subdev_core_ops = { > .g_register = rj54n1_g_register, > .s_register = rj54n1_s_register, > #endif > + .s_power = rj54n1_s_power, > }; > > static int rj54n1_g_mbus_config(struct v4l2_subdev *sd, > diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c > index bbd518f..576146e 100644 > --- a/drivers/media/video/soc_camera.c > +++ b/drivers/media/video/soc_camera.c > @@ -50,63 +50,72 @@ static LIST_HEAD(hosts); > static LIST_HEAD(devices); > static DEFINE_MUTEX(list_lock); /* Protects the list of hosts */ > > -static int soc_camera_power_on(struct soc_camera_device *icd, > - struct soc_camera_link *icl) > +int soc_camera_power_on(struct device *dev, struct soc_camera_link *icl) > { > - struct v4l2_subdev *sd = soc_camera_to_subdev(icd); > int ret = regulator_bulk_enable(icl->num_regulators, > icl->regulators); > if (ret < 0) { > - dev_err(icd->pdev, "Cannot enable regulators\n"); > + dev_err(dev, "Cannot enable regulators\n"); > return ret; > } > > if (icl->power) { > - ret = icl->power(icd->control, 1); > + ret = icl->power(dev, 1); > if (ret < 0) { > - dev_err(icd->pdev, > + dev_err(dev, > "Platform failed to power-on the camera.\n"); > - goto elinkpwr; > + regulator_bulk_disable(icl->num_regulators, > + icl->regulators); > } > } > > - ret = v4l2_subdev_call(sd, core, s_power, 1); > - if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV) > - goto esdpwr; > - > - return 0; > - > -esdpwr: > - if (icl->power) > - icl->power(icd->control, 0); > -elinkpwr: > - regulator_bulk_disable(icl->num_regulators, > - icl->regulators); > return ret; > } > +EXPORT_SYMBOL(soc_camera_power_on); > > -static int soc_camera_power_off(struct soc_camera_device *icd, > - struct soc_camera_link *icl) > +int soc_camera_power_off(struct device *dev, struct soc_camera_link *icl) > { > - struct v4l2_subdev *sd = soc_camera_to_subdev(icd); > int ret; > > - v4l2_subdev_call(sd, core, s_power, 0); > - > if (icl->power) { > - ret = icl->power(icd->control, 0); > + ret = icl->power(dev, 0); > if (ret < 0) > - dev_err(icd->pdev, > + dev_err(dev, > "Platform failed to power-off the camera.\n"); > } > > ret = regulator_bulk_disable(icl->num_regulators, > icl->regulators); > if (ret < 0) > - dev_err(icd->pdev, "Cannot disable regulators\n"); > + dev_err(dev, "Cannot disable regulators\n"); > > return ret; > } > +EXPORT_SYMBOL(soc_camera_power_off); > + > +static int __soc_camera_power_on(struct soc_camera_device *icd) > +{ > + struct v4l2_subdev *sd = soc_camera_to_subdev(icd); > + int ret; > + > + ret = v4l2_subdev_call(sd, core, s_power, 1); > + if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV) > + return ret; > + > + return 0; > +} > + > +static int __soc_camera_power_off(struct soc_camera_device *icd) > +{ > + struct v4l2_subdev *sd = soc_camera_to_subdev(icd); > + int ret; > + > + ret = v4l2_subdev_call(sd, core, s_power, 0); > + if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV) > + return ret; > + > + return 0; > +} > > const struct soc_camera_format_xlate *soc_camera_xlate_by_fourcc( > struct soc_camera_device *icd, unsigned int fourcc) > @@ -539,7 +548,7 @@ static int soc_camera_open(struct file *file) > goto eiciadd; > } > > - ret = soc_camera_power_on(icd, icl); > + ret = __soc_camera_power_on(icd); > if (ret < 0) > goto epower; > > @@ -581,7 +590,7 @@ einitvb: > esfmt: > pm_runtime_disable(&icd->vdev->dev); > eresume: > - soc_camera_power_off(icd, icl); > + __soc_camera_power_off(icd); > epower: > ici->ops->remove(icd); > eiciadd: > @@ -598,8 +607,6 @@ static int soc_camera_close(struct file *file) > > icd->use_count--; > if (!icd->use_count) { > - struct soc_camera_link *icl = to_soc_camera_link(icd); > - > pm_runtime_suspend(&icd->vdev->dev); > pm_runtime_disable(&icd->vdev->dev); > > @@ -607,7 +614,7 @@ static int soc_camera_close(struct file *file) > vb2_queue_release(&icd->vb2_vidq); > ici->ops->remove(icd); > > - soc_camera_power_off(icd, icl); > + __soc_camera_power_off(icd); > } > > if (icd->streamer == file) > @@ -1066,8 +1073,14 @@ static int soc_camera_probe(struct soc_camera_device *icd) > * subdevice has not been initialised yet. We'll have to call it once > * again after initialisation, even though it shouldn't be needed, we > * don't do any IO here. > + * > + * The device pointer passed to soc_camera_power_on(), and ultimately to > + * the platform callback, should be the subdev physical device. However, > + * we have no way to retrieve a pointer to that device here. This isn't > + * a real issue, as no platform currently uses the device pointer, and > + * this soc_camera_power_on() call will be removed in the next commit. > */ > - ret = soc_camera_power_on(icd, icl); > + ret = soc_camera_power_on(icd->pdev, icl); > if (ret < 0) > goto epower; > > @@ -1140,7 +1153,7 @@ static int soc_camera_probe(struct soc_camera_device *icd) > > ici->ops->remove(icd); > > - soc_camera_power_off(icd, icl); > + __soc_camera_power_off(icd); > > mutex_unlock(&icd->video_lock); > > @@ -1162,7 +1175,7 @@ eadddev: > video_device_release(icd->vdev); > icd->vdev = NULL; > evdc: > - soc_camera_power_off(icd, icl); > + __soc_camera_power_off(icd); > epower: > ici->ops->remove(icd); > eadd: > diff --git a/drivers/media/video/soc_camera_platform.c b/drivers/media/video/soc_camera_platform.c > index f59ccad..7cf7fd1 100644 > --- a/drivers/media/video/soc_camera_platform.c > +++ b/drivers/media/video/soc_camera_platform.c > @@ -50,7 +50,16 @@ static int soc_camera_platform_fill_fmt(struct v4l2_subdev *sd, > return 0; > } > > -static struct v4l2_subdev_core_ops platform_subdev_core_ops; > +static int soc_camera_platform_s_power(struct v4l2_subdev *sd, int on) > +{ > + struct soc_camera_platform_info *p = v4l2_get_subdevdata(sd); > + > + return soc_camera_set_power(p->icd->control, p->icd->link, on); > +} > + > +static struct v4l2_subdev_core_ops platform_subdev_core_ops = { > + .s_power = soc_camera_platform_s_power, > +}; > > static int soc_camera_platform_enum_fmt(struct v4l2_subdev *sd, unsigned int index, > enum v4l2_mbus_pixelcode *code) > diff --git a/drivers/media/video/tw9910.c b/drivers/media/video/tw9910.c > index 9f53eac..f283650 100644 > --- a/drivers/media/video/tw9910.c > +++ b/drivers/media/video/tw9910.c > @@ -566,6 +566,14 @@ static int tw9910_s_register(struct v4l2_subdev *sd, > } > #endif > > +static int tw9910_s_power(struct v4l2_subdev *sd, int on) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(sd); > + struct soc_camera_link *icl = soc_camera_i2c_to_link(client); > + > + return soc_camera_set_power(&client->dev, icl, on); > +} > + > static int tw9910_set_frame(struct v4l2_subdev *sd, u32 *width, u32 *height) > { > struct i2c_client *client = v4l2_get_subdevdata(sd); > @@ -814,6 +822,7 @@ static struct v4l2_subdev_core_ops tw9910_subdev_core_ops = { > .g_register = tw9910_g_register, > .s_register = tw9910_s_register, > #endif > + .s_power = tw9910_s_power, > }; > > static int tw9910_enum_fmt(struct v4l2_subdev *sd, unsigned int index, > diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h > index d865dcf..982bfc9 100644 > --- a/include/media/soc_camera.h > +++ b/include/media/soc_camera.h > @@ -254,6 +254,16 @@ unsigned long soc_camera_apply_sensor_flags(struct soc_camera_link *icl, > unsigned long soc_camera_apply_board_flags(struct soc_camera_link *icl, > const struct v4l2_mbus_config *cfg); > > +int soc_camera_power_on(struct device *dev, struct soc_camera_link *icl); > +int soc_camera_power_off(struct device *dev, struct soc_camera_link *icl); > + > +static inline int soc_camera_set_power(struct device *dev, > + struct soc_camera_link *icl, bool on) > +{ > + return on ? soc_camera_power_on(dev, icl) > + : soc_camera_power_off(dev, icl); > +} > + > /* This is only temporary here - until v4l2-subdev begins to link to video_device */ > #include > static inline struct video_device *soc_camera_i2c_to_vdev(const struct i2c_client *client) >