From: David Cohen <david.a.cohen@linux.intel.com>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>,
linux-media@vger.kernel.org
Subject: Re: [PATCH v2 8/9] soc-camera: Add and use soc_camera_power_[on|off]() helper functions
Date: Mon, 16 Jul 2012 02:17:43 +0300 [thread overview]
Message-ID: <50034F97.9060208@linux.intel.com> (raw)
In-Reply-To: <1341520728-2707-9-git-send-email-laurent.pinchart@ideasonboard.com>
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 <laurent.pinchart@ideasonboard.com>
> ---
> 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 <linux/i2c.h>
> static inline struct video_device *soc_camera_i2c_to_vdev(const struct i2c_client *client)
>
next prev parent reply other threads:[~2012-07-15 23:17 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-07-05 20:38 [PATCH v2 0/9] Miscellaneous soc-camera patches Laurent Pinchart
2012-07-05 20:38 ` [PATCH v2 1/9] soc-camera: Don't fail at module init time if no device is present Laurent Pinchart
2012-07-05 20:38 ` [PATCH v2 2/9] soc-camera: Pass the physical device to the power operation Laurent Pinchart
2012-07-05 20:38 ` [PATCH v2 3/9] ov2640: Don't access the device in the g_mbus_fmt operation Laurent Pinchart
2012-07-05 20:38 ` [PATCH v2 4/9] ov772x: " Laurent Pinchart
2012-07-05 20:38 ` [PATCH v2 5/9] tw9910: " Laurent Pinchart
2012-07-05 20:38 ` [PATCH v2 6/9] soc_camera: Don't call .s_power() during probe Laurent Pinchart
2012-07-05 20:38 ` [PATCH v2 7/9] soc-camera: Continue the power off sequence if one of the steps fails Laurent Pinchart
2012-07-15 22:24 ` David Cohen
2012-07-16 23:45 ` Laurent Pinchart
2012-07-17 11:03 ` David Cohen
2012-07-05 20:38 ` [PATCH v2 8/9] soc-camera: Add and use soc_camera_power_[on|off]() helper functions Laurent Pinchart
2012-07-15 23:17 ` David Cohen [this message]
2012-07-15 23:25 ` David Cohen
2012-07-17 1:24 ` Laurent Pinchart
2012-07-17 10:40 ` David Cohen
2012-07-05 20:38 ` [PATCH v2 9/9] soc-camera: Push probe-time power management to drivers Laurent Pinchart
2012-07-10 12:06 ` Guennadi Liakhovetski
2012-07-15 13:31 ` Laurent Pinchart
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=50034F97.9060208@linux.intel.com \
--to=david.a.cohen@linux.intel.com \
--cc=g.liakhovetski@gmx.de \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-media@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.