All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/9] Miscellaneous soc-camera patches
@ 2012-07-05 20:38 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
                   ` (8 more replies)
  0 siblings, 9 replies; 19+ messages in thread
From: Laurent Pinchart @ 2012-07-05 20:38 UTC (permalink / raw)
  To: Guennadi Liakhovetski; +Cc: linux-media

Here's the second version of the miscellaneous soc-camera patches that try to
improve the interoperability between soc-camera clients and non soc-camera
hosts.

As with the previous version, all patches have been compile-tested but not
runtime-tested as I lack the necessary hardware.

Changes compared to v1:

- Set priv->cfmt_code in ov2640 driver (3/9)
- Split the soc-camera power off sequence change to its own patch (7/9)
- Added an inline soc_camera_set_power() function (8/9)
- Make sure the mt9m111 gets powered off completely if power on fails (8/9)
- Simplify the s_power implementation in the ov5642 driver (8/9)
- Don't modify power handling in the sh_mobile_csi2 driver
- Don't pass a NULL device pointer to soc_camera_power_on() at probe time (8/9)
- Fix physical device retrieval in soc_camera_platform_s_power() (8/9)
- Don't loose return codes in mt9m111_video_probe() (9/9)
- Remove unneeded enable/idle/disable calls in mt9p031 probe (9/9)

Laurent Pinchart (9):
  soc-camera: Don't fail at module init time if no device is present
  soc-camera: Pass the physical device to the power operation
  ov2640: Don't access the device in the g_mbus_fmt operation
  ov772x: Don't access the device in the g_mbus_fmt operation
  tw9910: Don't access the device in the g_mbus_fmt operation
  soc_camera: Don't call .s_power() during probe
  soc-camera: Continue the power off sequence if one of the steps fails
  soc-camera: Add and use soc_camera_power_[on|off]() helper functions
  soc-camera: Push probe-time power management to drivers

 drivers/media/video/imx074.c              |   30 ++++++-
 drivers/media/video/mt9m001.c             |   26 ++++++-
 drivers/media/video/mt9m111.c             |  116 ++++++++++++++++++----------
 drivers/media/video/mt9t031.c             |   48 ++++++------
 drivers/media/video/mt9t112.c             |   21 +++++-
 drivers/media/video/mt9v022.c             |   14 ++++
 drivers/media/video/ov2640.c              |   26 +++++--
 drivers/media/video/ov5642.c              |   31 ++++++--
 drivers/media/video/ov6650.c              |   28 ++++++--
 drivers/media/video/ov772x.c              |   31 ++++++--
 drivers/media/video/ov9640.c              |   27 ++++++-
 drivers/media/video/ov9740.c              |   38 +++++++---
 drivers/media/video/rj54n1cb0c.c          |   27 ++++++-
 drivers/media/video/soc_camera.c          |  101 +++++++++++--------------
 drivers/media/video/soc_camera_platform.c |   11 +++-
 drivers/media/video/tw9910.c              |   29 ++++++--
 include/media/soc_camera.h                |   10 +++
 17 files changed, 423 insertions(+), 191 deletions(-)

-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 19+ messages in thread
* [PATCH v2 1/9] soc-camera: Don't fail at module init time if no device is present
  2012-07-05 20:38 [PATCH v2 0/9] Miscellaneous soc-camera patches Laurent Pinchart
@ 2012-07-05 20:38 ` Laurent Pinchart
  2012-07-05 20:38 ` [PATCH v2 2/9] soc-camera: Pass the physical device to the power operation Laurent Pinchart
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 19+ messages in thread
From: Laurent Pinchart @ 2012-07-05 20:38 UTC (permalink / raw)
  To: Guennadi Liakhovetski; +Cc: linux-media

The soc-camera module exports functions that are needed by soc-camera
client drivers even when not running in soc-camera mode. Replace the
platform_driver_probe() with a platform_driver_register() call to avoid
module load failures if no soc-camera device is present.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/soc_camera.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
index 0421bf9..e7c6809 100644
--- a/drivers/media/video/soc_camera.c
+++ b/drivers/media/video/soc_camera.c
@@ -1518,6 +1518,7 @@ static int __devexit soc_camera_pdrv_remove(struct platform_device *pdev)
 }
 
 static struct platform_driver __refdata soc_camera_pdrv = {
+	.probe = soc_camera_pdrv_probe,
 	.remove  = __devexit_p(soc_camera_pdrv_remove),
 	.driver  = {
 		.name	= "soc-camera-pdrv",
@@ -1527,7 +1528,7 @@ static struct platform_driver __refdata soc_camera_pdrv = {
 
 static int __init soc_camera_init(void)
 {
-	return platform_driver_probe(&soc_camera_pdrv, soc_camera_pdrv_probe);
+	return platform_driver_register(&soc_camera_pdrv);
 }
 
 static void __exit soc_camera_exit(void)
-- 
1.7.8.6


^ permalink raw reply related	[flat|nested] 19+ messages in thread
* [PATCH v2 2/9] soc-camera: Pass the physical device to the power operation
  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 ` 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
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 19+ messages in thread
From: Laurent Pinchart @ 2012-07-05 20:38 UTC (permalink / raw)
  To: Guennadi Liakhovetski; +Cc: linux-media

There will be no soc_camera_device instance with a soc-camera device is
used with a non soc-camera host, so we won't be able to pass the
soc_camera_device fake platform device to board code. Pass the physical
device instead.

The argument is currently not used by any board file so this is safe.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/soc_camera.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
index e7c6809..b03ffec 100644
--- a/drivers/media/video/soc_camera.c
+++ b/drivers/media/video/soc_camera.c
@@ -62,7 +62,7 @@ static int soc_camera_power_on(struct soc_camera_device *icd,
 	}
 
 	if (icl->power) {
-		ret = icl->power(icd->pdev, 1);
+		ret = icl->power(icd->control, 1);
 		if (ret < 0) {
 			dev_err(icd->pdev,
 				"Platform failed to power-on the camera.\n");
@@ -78,7 +78,7 @@ static int soc_camera_power_on(struct soc_camera_device *icd,
 
 esdpwr:
 	if (icl->power)
-		icl->power(icd->pdev, 0);
+		icl->power(icd->control, 0);
 elinkpwr:
 	regulator_bulk_disable(icl->num_regulators,
 			       icl->regulators);
@@ -95,7 +95,7 @@ static int soc_camera_power_off(struct soc_camera_device *icd,
 		return ret;
 
 	if (icl->power) {
-		ret = icl->power(icd->pdev, 0);
+		ret = icl->power(icd->control, 0);
 		if (ret < 0) {
 			dev_err(icd->pdev,
 				"Platform failed to power-off the camera.\n");
-- 
1.7.8.6


^ permalink raw reply related	[flat|nested] 19+ messages in thread
* [PATCH v2 3/9] ov2640: Don't access the device in the g_mbus_fmt operation
  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 ` Laurent Pinchart
  2012-07-05 20:38 ` [PATCH v2 4/9] ov772x: " Laurent Pinchart
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 19+ messages in thread
From: Laurent Pinchart @ 2012-07-05 20:38 UTC (permalink / raw)
  To: Guennadi Liakhovetski; +Cc: linux-media

The g_mbus_fmt operation only needs to return the current mbus frame
format and doesn't need to configure the hardware to do so. Fix it to
avoid requiring the chip to be powered on when calling the operation.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/ov2640.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/media/video/ov2640.c b/drivers/media/video/ov2640.c
index 3c2c5d3..7c44d1f 100644
--- a/drivers/media/video/ov2640.c
+++ b/drivers/media/video/ov2640.c
@@ -837,10 +837,8 @@ static int ov2640_g_fmt(struct v4l2_subdev *sd,
 
 	if (!priv->win) {
 		u32 width = W_SVGA, height = H_SVGA;
-		int ret = ov2640_set_params(client, &width, &height,
-					    V4L2_MBUS_FMT_UYVY8_2X8);
-		if (ret < 0)
-			return ret;
+		priv->win = ov2640_select_win(&width, &height);
+		priv->cfmt_code = V4L2_MBUS_FMT_UYVY8_2X8;
 	}
 
 	mf->width	= priv->win->width;
-- 
1.7.8.6


^ permalink raw reply related	[flat|nested] 19+ messages in thread
* [PATCH v2 4/9] ov772x: Don't access the device in the g_mbus_fmt operation
  2012-07-05 20:38 [PATCH v2 0/9] Miscellaneous soc-camera patches Laurent Pinchart
                   ` (2 preceding siblings ...)
  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 ` Laurent Pinchart
  2012-07-05 20:38 ` [PATCH v2 5/9] tw9910: " Laurent Pinchart
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 19+ messages in thread
From: Laurent Pinchart @ 2012-07-05 20:38 UTC (permalink / raw)
  To: Guennadi Liakhovetski; +Cc: linux-media

The g_mbus_fmt operation only needs to return the current mbus frame
format and doesn't need to configure the hardware to do so. Fix it to
avoid requiring the chip to be powered on when calling the operation.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/ov772x.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/media/video/ov772x.c b/drivers/media/video/ov772x.c
index 74e77d3..6d79b89 100644
--- a/drivers/media/video/ov772x.c
+++ b/drivers/media/video/ov772x.c
@@ -880,15 +880,11 @@ static int ov772x_cropcap(struct v4l2_subdev *sd, struct v4l2_cropcap *a)
 static int ov772x_g_fmt(struct v4l2_subdev *sd,
 			struct v4l2_mbus_framefmt *mf)
 {
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	struct ov772x_priv *priv = container_of(sd, struct ov772x_priv, subdev);
 
 	if (!priv->win || !priv->cfmt) {
-		u32 width = VGA_WIDTH, height = VGA_HEIGHT;
-		int ret = ov772x_set_params(client, &width, &height,
-					    V4L2_MBUS_FMT_YUYV8_2X8);
-		if (ret < 0)
-			return ret;
+		priv->cfmt = &ov772x_cfmts[0];
+		priv->win = ov772x_select_win(VGA_WIDTH, VGA_HEIGHT);
 	}
 
 	mf->width	= priv->win->width;
-- 
1.7.8.6


^ permalink raw reply related	[flat|nested] 19+ messages in thread
* [PATCH v2 5/9] tw9910: Don't access the device in the g_mbus_fmt operation
  2012-07-05 20:38 [PATCH v2 0/9] Miscellaneous soc-camera patches Laurent Pinchart
                   ` (3 preceding siblings ...)
  2012-07-05 20:38 ` [PATCH v2 4/9] ov772x: " Laurent Pinchart
@ 2012-07-05 20:38 ` Laurent Pinchart
  2012-07-05 20:38 ` [PATCH v2 6/9] soc_camera: Don't call .s_power() during probe Laurent Pinchart
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 19+ messages in thread
From: Laurent Pinchart @ 2012-07-05 20:38 UTC (permalink / raw)
  To: Guennadi Liakhovetski; +Cc: linux-media

The g_mbus_fmt operation only needs to return the current mbus frame
format and doesn't need to configure the hardware to do so. Fix it to
avoid requiring the chip to be powered on when calling the operation.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/tw9910.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/media/video/tw9910.c b/drivers/media/video/tw9910.c
index 8768efb..9f53eac 100644
--- a/drivers/media/video/tw9910.c
+++ b/drivers/media/video/tw9910.c
@@ -699,11 +699,9 @@ static int tw9910_g_fmt(struct v4l2_subdev *sd,
 	struct tw9910_priv *priv = to_tw9910(client);
 
 	if (!priv->scale) {
-		int ret;
-		u32 width = 640, height = 480;
-		ret = tw9910_set_frame(sd, &width, &height);
-		if (ret < 0)
-			return ret;
+		priv->scale = tw9910_select_norm(priv->norm, 640, 480);
+		if (!priv->scale)
+			return -EINVAL;
 	}
 
 	mf->width	= priv->scale->width;
-- 
1.7.8.6


^ permalink raw reply related	[flat|nested] 19+ messages in thread
* [PATCH v2 6/9] soc_camera: Don't call .s_power() during probe
  2012-07-05 20:38 [PATCH v2 0/9] Miscellaneous soc-camera patches Laurent Pinchart
                   ` (4 preceding siblings ...)
  2012-07-05 20:38 ` [PATCH v2 5/9] tw9910: " Laurent Pinchart
@ 2012-07-05 20:38 ` 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
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 19+ messages in thread
From: Laurent Pinchart @ 2012-07-05 20:38 UTC (permalink / raw)
  To: Guennadi Liakhovetski; +Cc: linux-media

The .s_power() call only covers the .g_mbus_fmt() operation call.
Several clients required to be powered on to retrieve the current mbus
format but have now been fixed. The .s_power() call is thus not needed
anymore and can be removed.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/soc_camera.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
index b03ffec..55b981f 100644
--- a/drivers/media/video/soc_camera.c
+++ b/drivers/media/video/soc_camera.c
@@ -1133,10 +1133,6 @@ static int soc_camera_probe(struct soc_camera_device *icd)
 	if (ret < 0)
 		goto evidstart;
 
-	ret = v4l2_subdev_call(sd, core, s_power, 1);
-	if (ret < 0 && ret != -ENOIOCTLCMD)
-		goto esdpwr;
-
 	/* Try to improve our guess of a reasonable window format */
 	if (!v4l2_subdev_call(sd, video, g_mbus_fmt, &mf)) {
 		icd->user_width		= mf.width;
@@ -1153,8 +1149,6 @@ static int soc_camera_probe(struct soc_camera_device *icd)
 
 	return 0;
 
-esdpwr:
-	video_unregister_device(icd->vdev);
 evidstart:
 	mutex_unlock(&icd->video_lock);
 	soc_camera_free_user_formats(icd);
-- 
1.7.8.6


^ permalink raw reply related	[flat|nested] 19+ messages in thread
* [PATCH v2 7/9] soc-camera: Continue the power off sequence if one of the steps fails
  2012-07-05 20:38 [PATCH v2 0/9] Miscellaneous soc-camera patches Laurent Pinchart
                   ` (5 preceding siblings ...)
  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 ` Laurent Pinchart
  2012-07-15 22:24   ` 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-05 20:38 ` [PATCH v2 9/9] soc-camera: Push probe-time power management to drivers Laurent Pinchart
  8 siblings, 1 reply; 19+ messages in thread
From: Laurent Pinchart @ 2012-07-05 20:38 UTC (permalink / raw)
  To: Guennadi Liakhovetski; +Cc: linux-media

Powering off a device is a "best effort" task: failure to execute one of
the steps should not prevent the next steps to be executed. For
instance, an I2C communication error when putting the chip in stand-by
mode should not prevent the more agressive next step of turning the
chip's power supply off.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/soc_camera.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
index 55b981f..bbd518f 100644
--- a/drivers/media/video/soc_camera.c
+++ b/drivers/media/video/soc_camera.c
@@ -89,18 +89,15 @@ static int soc_camera_power_off(struct soc_camera_device *icd,
 				struct soc_camera_link *icl)
 {
 	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
-	int ret = v4l2_subdev_call(sd, core, s_power, 0);
+	int ret;
 
-	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
-		return ret;
+	v4l2_subdev_call(sd, core, s_power, 0);
 
 	if (icl->power) {
 		ret = icl->power(icd->control, 0);
-		if (ret < 0) {
+		if (ret < 0)
 			dev_err(icd->pdev,
 				"Platform failed to power-off the camera.\n");
-			return ret;
-		}
 	}
 
 	ret = regulator_bulk_disable(icl->num_regulators,
-- 
1.7.8.6


^ permalink raw reply related	[flat|nested] 19+ messages in thread
* [PATCH v2 8/9] soc-camera: Add and use soc_camera_power_[on|off]() helper functions
  2012-07-05 20:38 [PATCH v2 0/9] Miscellaneous soc-camera patches Laurent Pinchart
                   ` (6 preceding siblings ...)
  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-05 20:38 ` Laurent Pinchart
  2012-07-15 23:17   ` David Cohen
  2012-07-05 20:38 ` [PATCH v2 9/9] soc-camera: Push probe-time power management to drivers Laurent Pinchart
  8 siblings, 1 reply; 19+ messages in thread
From: Laurent Pinchart @ 2012-07-05 20:38 UTC (permalink / raw)
  To: Guennadi Liakhovetski; +Cc: linux-media

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(-)

diff --git a/drivers/media/video/imx074.c b/drivers/media/video/imx074.c
index 351e9ba..ade1987 100644
--- a/drivers/media/video/imx074.c
+++ b/drivers/media/video/imx074.c
@@ -268,6 +268,14 @@ static int imx074_g_chip_ident(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int imx074_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 imx074_g_mbus_config(struct v4l2_subdev *sd,
 				struct v4l2_mbus_config *cfg)
 {
@@ -292,6 +300,7 @@ static struct v4l2_subdev_video_ops imx074_subdev_video_ops = {
 
 static struct v4l2_subdev_core_ops imx074_subdev_core_ops = {
 	.g_chip_ident	= imx074_g_chip_ident,
+	.s_power	= imx074_s_power,
 };
 
 static struct v4l2_subdev_ops imx074_subdev_ops = {
diff --git a/drivers/media/video/mt9m001.c b/drivers/media/video/mt9m001.c
index 00583f5..cd71230 100644
--- a/drivers/media/video/mt9m001.c
+++ b/drivers/media/video/mt9m001.c
@@ -377,6 +377,14 @@ static int mt9m001_s_register(struct v4l2_subdev *sd,
 }
 #endif
 
+static int mt9m001_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 mt9m001_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct mt9m001 *mt9m001 = container_of(ctrl->handler,
@@ -566,6 +574,7 @@ static struct v4l2_subdev_core_ops mt9m001_subdev_core_ops = {
 	.g_register	= mt9m001_g_register,
 	.s_register	= mt9m001_s_register,
 #endif
+	.s_power	= mt9m001_s_power,
 };
 
 static int mt9m001_enum_fmt(struct v4l2_subdev *sd, unsigned int index,
diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
index b0c5299..a7a649a 100644
--- a/drivers/media/video/mt9m111.c
+++ b/drivers/media/video/mt9m111.c
@@ -832,10 +832,37 @@ static int mt9m111_video_probe(struct i2c_client *client)
 	return v4l2_ctrl_handler_setup(&mt9m111->hdl);
 }
 
+static int mt9m111_power_on(struct mt9m111 *mt9m111)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
+	struct soc_camera_link *icl = soc_camera_i2c_to_link(client);
+	int ret;
+
+	ret = soc_camera_power_on(&client->dev, icl);
+	if (ret < 0)
+		return ret;
+
+	ret = mt9m111_resume(mt9m111);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to resume the sensor: %d\n", ret);
+		soc_camera_power_off(&client->dev, icl);
+	}
+
+	return ret;
+}
+
+static void mt9m111_power_off(struct mt9m111 *mt9m111)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
+	struct soc_camera_link *icl = soc_camera_i2c_to_link(client);
+
+	mt9m111_suspend(mt9m111);
+	soc_camera_power_off(&client->dev, icl);
+}
+
 static int mt9m111_s_power(struct v4l2_subdev *sd, int on)
 {
 	struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev);
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	int ret = 0;
 
 	mutex_lock(&mt9m111->power_lock);
@@ -845,23 +872,18 @@ static int mt9m111_s_power(struct v4l2_subdev *sd, int on)
 	 * update the power state.
 	 */
 	if (mt9m111->power_count == !on) {
-		if (on) {
-			ret = mt9m111_resume(mt9m111);
-			if (ret) {
-				dev_err(&client->dev,
-					"Failed to resume the sensor: %d\n", ret);
-				goto out;
-			}
-		} else {
-			mt9m111_suspend(mt9m111);
-		}
+		if (on)
+			ret = mt9m111_power_on(mt9m111);
+		else
+			mt9m111_power_off(mt9m111);
 	}
 
-	/* Update the power count. */
-	mt9m111->power_count += on ? 1 : -1;
-	WARN_ON(mt9m111->power_count < 0);
+	if (!ret) {
+		/* Update the power count. */
+		mt9m111->power_count += on ? 1 : -1;
+		WARN_ON(mt9m111->power_count < 0);
+	}
 
-out:
 	mutex_unlock(&mt9m111->power_lock);
 	return ret;
 }
diff --git a/drivers/media/video/mt9t031.c b/drivers/media/video/mt9t031.c
index 1415074..9666e20 100644
--- a/drivers/media/video/mt9t031.c
+++ b/drivers/media/video/mt9t031.c
@@ -616,12 +616,19 @@ static struct device_type mt9t031_dev_type = {
 static int mt9t031_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 video_device *vdev = soc_camera_i2c_to_vdev(client);
+	int ret;
 
-	if (on)
+	if (on) {
+		ret = soc_camera_power_on(&client->dev, icl);
+		if (ret < 0)
+			return ret;
 		vdev->dev.type = &mt9t031_dev_type;
-	else
+	} else {
 		vdev->dev.type = NULL;
+		soc_camera_power_off(&client->dev, icl);
+	}
 
 	return 0;
 }
diff --git a/drivers/media/video/mt9t112.c b/drivers/media/video/mt9t112.c
index e1ae46a..624ceec 100644
--- a/drivers/media/video/mt9t112.c
+++ b/drivers/media/video/mt9t112.c
@@ -776,12 +776,21 @@ static int mt9t112_s_register(struct v4l2_subdev *sd,
 }
 #endif
 
+static int mt9t112_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 struct v4l2_subdev_core_ops mt9t112_subdev_core_ops = {
 	.g_chip_ident	= mt9t112_g_chip_ident,
 #ifdef CONFIG_VIDEO_ADV_DEBUG
 	.g_register	= mt9t112_g_register,
 	.s_register	= mt9t112_s_register,
 #endif
+	.s_power	= mt9t112_s_power,
 };
 
 
diff --git a/drivers/media/video/mt9v022.c b/drivers/media/video/mt9v022.c
index 7247924..5f09cb7 100644
--- a/drivers/media/video/mt9v022.c
+++ b/drivers/media/video/mt9v022.c
@@ -445,6 +445,14 @@ static int mt9v022_s_register(struct v4l2_subdev *sd,
 }
 #endif
 
+static int mt9v022_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 mt9v022_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct mt9v022 *mt9v022 = container_of(ctrl->handler,
@@ -664,6 +672,7 @@ static struct v4l2_subdev_core_ops mt9v022_subdev_core_ops = {
 	.g_register	= mt9v022_g_register,
 	.s_register	= mt9v022_s_register,
 #endif
+	.s_power	= mt9v022_s_power,
 };
 
 static int mt9v022_enum_fmt(struct v4l2_subdev *sd, unsigned int index,
diff --git a/drivers/media/video/ov2640.c b/drivers/media/video/ov2640.c
index 7c44d1f..16ed091 100644
--- a/drivers/media/video/ov2640.c
+++ b/drivers/media/video/ov2640.c
@@ -742,6 +742,14 @@ static int ov2640_s_register(struct v4l2_subdev *sd,
 }
 #endif
 
+static int ov2640_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);
+}
+
 /* Select the nearest higher resolution for capture */
 static const struct ov2640_win_size *ov2640_select_win(u32 *width, u32 *height)
 {
@@ -988,6 +996,7 @@ static struct v4l2_subdev_core_ops ov2640_subdev_core_ops = {
 	.g_register	= ov2640_g_register,
 	.s_register	= ov2640_s_register,
 #endif
+	.s_power	= ov2640_s_power,
 };
 
 static int ov2640_g_mbus_config(struct v4l2_subdev *sd,
diff --git a/drivers/media/video/ov5642.c b/drivers/media/video/ov5642.c
index 0bc9331..61824c6 100644
--- a/drivers/media/video/ov5642.c
+++ b/drivers/media/video/ov5642.c
@@ -933,13 +933,17 @@ static int ov5642_g_mbus_config(struct v4l2_subdev *sd,
 
 static int ov5642_s_power(struct v4l2_subdev *sd, int on)
 {
-	struct i2c_client *client;
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct soc_camera_link *icl = soc_camera_i2c_to_link(client);
 	int ret;
 
 	if (!on)
-		return 0;
+		return soc_camera_power_off(&client->dev, icl);
+
+	ret = soc_camera_power_on(&client->dev, icl);
+	if (ret < 0)
+		return ret;
 
-	client = v4l2_get_subdevdata(sd);
 	ret = ov5642_write_array(client, ov5642_default_regs_init);
 	if (!ret)
 		ret = ov5642_set_resolution(sd);
diff --git a/drivers/media/video/ov6650.c b/drivers/media/video/ov6650.c
index 3e028b1..12d57a5 100644
--- a/drivers/media/video/ov6650.c
+++ b/drivers/media/video/ov6650.c
@@ -432,6 +432,14 @@ static int ov6650_set_register(struct v4l2_subdev *sd,
 }
 #endif
 
+static int ov6650_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 ov6650_g_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
@@ -866,6 +874,7 @@ static struct v4l2_subdev_core_ops ov6650_core_ops = {
 	.g_register		= ov6650_get_register,
 	.s_register		= ov6650_set_register,
 #endif
+	.s_power		= ov6650_s_power,
 };
 
 /* Request bus settings on camera side */
diff --git a/drivers/media/video/ov772x.c b/drivers/media/video/ov772x.c
index 6d79b89..a022662 100644
--- a/drivers/media/video/ov772x.c
+++ b/drivers/media/video/ov772x.c
@@ -683,6 +683,14 @@ static int ov772x_s_register(struct v4l2_subdev *sd,
 }
 #endif
 
+static int ov772x_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 const struct ov772x_win_size *ov772x_select_win(u32 width, u32 height)
 {
 	__u32 diff;
@@ -996,6 +1004,7 @@ static struct v4l2_subdev_core_ops ov772x_subdev_core_ops = {
 	.g_register	= ov772x_g_register,
 	.s_register	= ov772x_s_register,
 #endif
+	.s_power	= ov772x_s_power,
 };
 
 static int ov772x_enum_fmt(struct v4l2_subdev *sd, unsigned int index,
diff --git a/drivers/media/video/ov9640.c b/drivers/media/video/ov9640.c
index 23412de..8c13b3b 100644
--- a/drivers/media/video/ov9640.c
+++ b/drivers/media/video/ov9640.c
@@ -333,6 +333,14 @@ static int ov9640_set_register(struct v4l2_subdev *sd,
 }
 #endif
 
+static int ov9640_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);
+}
+
 /* select nearest higher resolution for capture */
 static void ov9640_res_roundup(u32 *width, u32 *height)
 {
@@ -631,7 +639,7 @@ static struct v4l2_subdev_core_ops ov9640_core_ops = {
 	.g_register		= ov9640_get_register,
 	.s_register		= ov9640_set_register,
 #endif
-
+	.s_power		= ov9640_s_power,
 };
 
 /* Request bus settings on camera side */
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);
 		return 0;
+	}
 
 	if (on) {
 		ov9740_s_fmt(sd, &priv->current_mf);
 		ov9740_s_stream(sd, priv->current_enable);
 	} else {
 		ov9740_s_stream(sd, 0);
+		soc_camera_power_off(&client->dev, icl);
 		priv->current_enable = true;
 	}
 
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)
-- 
1.7.8.6


^ permalink raw reply related	[flat|nested] 19+ messages in thread
* [PATCH v2 9/9] soc-camera: Push probe-time power management to drivers
  2012-07-05 20:38 [PATCH v2 0/9] Miscellaneous soc-camera patches Laurent Pinchart
                   ` (7 preceding siblings ...)
  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-05 20:38 ` Laurent Pinchart
  2012-07-10 12:06   ` Guennadi Liakhovetski
  8 siblings, 1 reply; 19+ messages in thread
From: Laurent Pinchart @ 2012-07-05 20:38 UTC (permalink / raw)
  To: Guennadi Liakhovetski; +Cc: linux-media

Several client drivers access the hardware at probe time, for instance
to read the probe chip ID. Such chips need to be powered up when being
probed.

soc-camera handles this by powering chips up in the soc-camera probe
implementation. However, this will break with non soc-camera hosts that
don't perform the same operations.

Fix the problem by pushing the power up/down from the soc-camera core
down to individual drivers on a needs basis.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/imx074.c     |   21 ++++++++--
 drivers/media/video/mt9m001.c    |   17 +++++++-
 drivers/media/video/mt9m111.c    |   80 +++++++++++++++++++++----------------
 drivers/media/video/mt9t031.c    |   37 +++++++----------
 drivers/media/video/mt9t112.c    |   12 +++++-
 drivers/media/video/mt9v022.c    |    5 ++
 drivers/media/video/ov2640.c     |   11 ++++-
 drivers/media/video/ov5642.c     |   21 ++++++++--
 drivers/media/video/ov6650.c     |   19 ++++++---
 drivers/media/video/ov772x.c     |   14 ++++++-
 drivers/media/video/ov9640.c     |   17 ++++++--
 drivers/media/video/ov9740.c     |   23 +++++++----
 drivers/media/video/rj54n1cb0c.c |   18 ++++++--
 drivers/media/video/soc_camera.c |   20 ---------
 drivers/media/video/tw9910.c     |   12 +++++-
 15 files changed, 204 insertions(+), 123 deletions(-)

diff --git a/drivers/media/video/imx074.c b/drivers/media/video/imx074.c
index ade1987..f8534ee 100644
--- a/drivers/media/video/imx074.c
+++ b/drivers/media/video/imx074.c
@@ -310,26 +310,33 @@ static struct v4l2_subdev_ops imx074_subdev_ops = {
 
 static int imx074_video_probe(struct i2c_client *client)
 {
+	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
 	int ret;
 	u16 id;
 
+	ret = imx074_s_power(subdev, 1);
+	if (ret < 0)
+		return ret;
+
 	/* Read sensor Model ID */
 	ret = reg_read(client, 0);
 	if (ret < 0)
-		return ret;
+		goto done;
 
 	id = ret << 8;
 
 	ret = reg_read(client, 1);
 	if (ret < 0)
-		return ret;
+		goto done;
 
 	id |= ret;
 
 	dev_info(&client->dev, "Chip ID 0x%04x detected\n", id);
 
-	if (id != 0x74)
-		return -ENODEV;
+	if (id != 0x74) {
+		ret = -ENODEV;
+		goto done;
+	}
 
 	/* PLL Setting EXTCLK=24MHz, 22.5times */
 	reg_write(client, PLL_MULTIPLIER, 0x2D);
@@ -411,7 +418,11 @@ static int imx074_video_probe(struct i2c_client *client)
 
 	reg_write(client, GROUPED_PARAMETER_HOLD, 0x00);	/* off */
 
-	return 0;
+	ret = 0;
+
+done:
+	imx074_s_power(subdev, 0);
+	return ret;
 }
 
 static int imx074_probe(struct i2c_client *client,
diff --git a/drivers/media/video/mt9m001.c b/drivers/media/video/mt9m001.c
index cd71230..d85be41 100644
--- a/drivers/media/video/mt9m001.c
+++ b/drivers/media/video/mt9m001.c
@@ -490,6 +490,10 @@ static int mt9m001_video_probe(struct soc_camera_link *icl,
 	unsigned long flags;
 	int ret;
 
+	ret = mt9m001_s_power(&mt9m001->subdev, 1);
+	if (ret < 0)
+		return ret;
+
 	/* Enable the chip */
 	data = reg_write(client, MT9M001_CHIP_ENABLE, 1);
 	dev_dbg(&client->dev, "write: %d\n", data);
@@ -511,7 +515,8 @@ static int mt9m001_video_probe(struct soc_camera_link *icl,
 	default:
 		dev_err(&client->dev,
 			"No MT9M001 chip detected, register read %x\n", data);
-		return -ENODEV;
+		ret = -ENODEV;
+		goto done;
 	}
 
 	mt9m001->num_fmts = 0;
@@ -540,11 +545,17 @@ static int mt9m001_video_probe(struct soc_camera_link *icl,
 		 data == 0x8431 ? "C12STM" : "C12ST");
 
 	ret = mt9m001_init(client);
-	if (ret < 0)
+	if (ret < 0) {
 		dev_err(&client->dev, "Failed to initialise the camera\n");
+		goto done;
+	}
 
 	/* mt9m001_init() has reset the chip, returning registers to defaults */
-	return v4l2_ctrl_handler_setup(&mt9m001->hdl);
+	ret = v4l2_ctrl_handler_setup(&mt9m001->hdl);
+
+done:
+	mt9m001_s_power(&mt9m001->subdev, 0);
+	return ret;
 }
 
 static void mt9m001_video_remove(struct soc_camera_link *icl)
diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
index a7a649a..8a10729 100644
--- a/drivers/media/video/mt9m111.c
+++ b/drivers/media/video/mt9m111.c
@@ -797,41 +797,6 @@ static int mt9m111_init(struct mt9m111 *mt9m111)
 	return ret;
 }
 
-/*
- * Interface active, can use i2c. If it fails, it can indeed mean, that
- * this wasn't our capture interface, so, we wait for the right one
- */
-static int mt9m111_video_probe(struct i2c_client *client)
-{
-	struct mt9m111 *mt9m111 = to_mt9m111(client);
-	s32 data;
-	int ret;
-
-	data = reg_read(CHIP_VERSION);
-
-	switch (data) {
-	case 0x143a: /* MT9M111 or MT9M131 */
-		mt9m111->model = V4L2_IDENT_MT9M111;
-		dev_info(&client->dev,
-			"Detected a MT9M111/MT9M131 chip ID %x\n", data);
-		break;
-	case 0x148c: /* MT9M112 */
-		mt9m111->model = V4L2_IDENT_MT9M112;
-		dev_info(&client->dev, "Detected a MT9M112 chip ID %x\n", data);
-		break;
-	default:
-		dev_err(&client->dev,
-			"No MT9M111/MT9M112/MT9M131 chip detected register read %x\n",
-			data);
-		return -ENODEV;
-	}
-
-	ret = mt9m111_init(mt9m111);
-	if (ret)
-		return ret;
-	return v4l2_ctrl_handler_setup(&mt9m111->hdl);
-}
-
 static int mt9m111_power_on(struct mt9m111 *mt9m111)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
@@ -942,6 +907,51 @@ static struct v4l2_subdev_ops mt9m111_subdev_ops = {
 	.video	= &mt9m111_subdev_video_ops,
 };
 
+/*
+ * Interface active, can use i2c. If it fails, it can indeed mean, that
+ * this wasn't our capture interface, so, we wait for the right one
+ */
+static int mt9m111_video_probe(struct i2c_client *client)
+{
+	struct mt9m111 *mt9m111 = to_mt9m111(client);
+	s32 data;
+	int ret;
+
+	ret = mt9m111_s_power(&mt9m111->subdev, 1);
+	if (ret < 0)
+		return ret;
+
+	data = reg_read(CHIP_VERSION);
+
+	switch (data) {
+	case 0x143a: /* MT9M111 or MT9M131 */
+		mt9m111->model = V4L2_IDENT_MT9M111;
+		dev_info(&client->dev,
+			"Detected a MT9M111/MT9M131 chip ID %x\n", data);
+		break;
+	case 0x148c: /* MT9M112 */
+		mt9m111->model = V4L2_IDENT_MT9M112;
+		dev_info(&client->dev, "Detected a MT9M112 chip ID %x\n", data);
+		break;
+	default:
+		dev_err(&client->dev,
+			"No MT9M111/MT9M112/MT9M131 chip detected register read %x\n",
+			data);
+		ret = -ENODEV;
+		goto done;
+	}
+
+	ret = mt9m111_init(mt9m111);
+	if (ret)
+		goto done;
+
+	ret = v4l2_ctrl_handler_setup(&mt9m111->hdl);
+
+done:
+	mt9m111_s_power(&mt9m111->subdev, 0);
+	return ret;
+}
+
 static int mt9m111_probe(struct i2c_client *client,
 			 const struct i2c_device_id *did)
 {
diff --git a/drivers/media/video/mt9t031.c b/drivers/media/video/mt9t031.c
index 9666e20..4f12177 100644
--- a/drivers/media/video/mt9t031.c
+++ b/drivers/media/video/mt9t031.c
@@ -161,14 +161,6 @@ static int mt9t031_idle(struct i2c_client *client)
 	return ret >= 0 ? 0 : -EIO;
 }
 
-static int mt9t031_disable(struct i2c_client *client)
-{
-	/* Disable the chip */
-	reg_clear(client, MT9T031_OUTPUT_CONTROL, 2);
-
-	return 0;
-}
-
 static int mt9t031_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
@@ -643,9 +635,15 @@ static int mt9t031_video_probe(struct i2c_client *client)
 	s32 data;
 	int ret;
 
-	/* Enable the chip */
-	data = reg_write(client, MT9T031_CHIP_ENABLE, 1);
-	dev_dbg(&client->dev, "write: %d\n", data);
+	ret = mt9t031_s_power(&mt9t031->subdev, 1);
+	if (ret < 0)
+		return ret;
+
+	ret = mt9t031_idle(client);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to initialise the camera\n");
+		return ret;
+	}
 
 	/* Read out the chip version register */
 	data = reg_read(client, MT9T031_CHIP_VERSION);
@@ -657,16 +655,16 @@ static int mt9t031_video_probe(struct i2c_client *client)
 	default:
 		dev_err(&client->dev,
 			"No MT9T031 chip detected, register read %x\n", data);
-		return -ENODEV;
+		ret = -ENODEV;
+		goto done;
 	}
 
 	dev_info(&client->dev, "Detected a MT9T031 chip ID %x\n", data);
 
-	ret = mt9t031_idle(client);
-	if (ret < 0)
-		dev_err(&client->dev, "Failed to initialise the camera\n");
-	else
-		v4l2_ctrl_handler_setup(&mt9t031->hdl);
+	ret = v4l2_ctrl_handler_setup(&mt9t031->hdl);
+
+done:
+	mt9t031_s_power(&mt9t031->subdev, 0);
 
 	return ret;
 }
@@ -817,12 +815,7 @@ static int mt9t031_probe(struct i2c_client *client,
 	mt9t031->xskip = 1;
 	mt9t031->yskip = 1;
 
-	mt9t031_idle(client);
-
 	ret = mt9t031_video_probe(client);
-
-	mt9t031_disable(client);
-
 	if (ret) {
 		v4l2_ctrl_handler_free(&mt9t031->hdl);
 		kfree(mt9t031);
diff --git a/drivers/media/video/mt9t112.c b/drivers/media/video/mt9t112.c
index 624ceec..9ba428e 100644
--- a/drivers/media/video/mt9t112.c
+++ b/drivers/media/video/mt9t112.c
@@ -1041,6 +1041,11 @@ static int mt9t112_camera_probe(struct i2c_client *client)
 	struct mt9t112_priv *priv = to_mt9t112(client);
 	const char          *devname;
 	int                  chipid;
+	int		     ret;
+
+	ret = mt9t112_s_power(&priv->subdev, 1);
+	if (ret < 0)
+		return ret;
 
 	/*
 	 * check and show chip ID
@@ -1058,12 +1063,15 @@ static int mt9t112_camera_probe(struct i2c_client *client)
 		break;
 	default:
 		dev_err(&client->dev, "Product ID error %04x\n", chipid);
-		return -ENODEV;
+		ret = -ENODEV;
+		goto done;
 	}
 
 	dev_info(&client->dev, "%s chip ID %04x\n", devname, chipid);
 
-	return 0;
+done:
+	mt9t112_s_power(&priv->subdev, 0);
+	return ret;
 }
 
 static int mt9t112_probe(struct i2c_client *client,
diff --git a/drivers/media/video/mt9v022.c b/drivers/media/video/mt9v022.c
index 5f09cb7..2edea84 100644
--- a/drivers/media/video/mt9v022.c
+++ b/drivers/media/video/mt9v022.c
@@ -578,6 +578,10 @@ static int mt9v022_video_probe(struct i2c_client *client)
 	int ret;
 	unsigned long flags;
 
+	ret = mt9v022_s_power(&mt9v022->subdev, 1);
+	if (ret < 0)
+		return ret;
+
 	/* Read out the chip version register */
 	data = reg_read(client, MT9V022_CHIP_VERSION);
 
@@ -648,6 +652,7 @@ static int mt9v022_video_probe(struct i2c_client *client)
 		dev_err(&client->dev, "Failed to initialise the camera\n");
 
 ei2c:
+	mt9v022_s_power(&mt9v022->subdev, 0);
 	return ret;
 }
 
diff --git a/drivers/media/video/ov2640.c b/drivers/media/video/ov2640.c
index 16ed091..78ac574 100644
--- a/drivers/media/video/ov2640.c
+++ b/drivers/media/video/ov2640.c
@@ -955,6 +955,10 @@ static int ov2640_video_probe(struct i2c_client *client)
 	const char *devname;
 	int ret;
 
+	ret = ov2640_s_power(&priv->subdev, 1);
+	if (ret < 0)
+		return ret;
+
 	/*
 	 * check and show product ID and manufacturer ID
 	 */
@@ -973,16 +977,17 @@ static int ov2640_video_probe(struct i2c_client *client)
 		dev_err(&client->dev,
 			"Product ID error %x:%x\n", pid, ver);
 		ret = -ENODEV;
-		goto err;
+		goto done;
 	}
 
 	dev_info(&client->dev,
 		 "%s Product ID %0x:%0x Manufacturer ID %x:%x\n",
 		 devname, pid, ver, midh, midl);
 
-	return v4l2_ctrl_handler_setup(&priv->hdl);
+	ret = v4l2_ctrl_handler_setup(&priv->hdl);
 
-err:
+done:
+	ov2640_s_power(&priv->subdev, 0);
 	return ret;
 }
 
diff --git a/drivers/media/video/ov5642.c b/drivers/media/video/ov5642.c
index 61824c6..d886c0b 100644
--- a/drivers/media/video/ov5642.c
+++ b/drivers/media/video/ov5642.c
@@ -980,29 +980,40 @@ static struct v4l2_subdev_ops ov5642_subdev_ops = {
 
 static int ov5642_video_probe(struct i2c_client *client)
 {
+	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
 	int ret;
 	u8 id_high, id_low;
 	u16 id;
 
+	ret = ov5642_s_power(subdev, 1);
+	if (ret < 0)
+		return ret;
+
 	/* Read sensor Model ID */
 	ret = reg_read(client, REG_CHIP_ID_HIGH, &id_high);
 	if (ret < 0)
-		return ret;
+		goto done;
 
 	id = id_high << 8;
 
 	ret = reg_read(client, REG_CHIP_ID_LOW, &id_low);
 	if (ret < 0)
-		return ret;
+		goto done;
 
 	id |= id_low;
 
 	dev_info(&client->dev, "Chip ID 0x%04x detected\n", id);
 
-	if (id != 0x5642)
-		return -ENODEV;
+	if (id != 0x5642) {
+		ret = -ENODEV;
+		goto done;
+	}
 
-	return 0;
+	ret = 0;
+
+done:
+	ov5642_s_power(subdev, 0);
+	return ret;
 }
 
 static int ov5642_probe(struct i2c_client *client,
diff --git a/drivers/media/video/ov6650.c b/drivers/media/video/ov6650.c
index 12d57a5..65b031f 100644
--- a/drivers/media/video/ov6650.c
+++ b/drivers/media/video/ov6650.c
@@ -829,8 +829,13 @@ static int ov6650_prog_dflt(struct i2c_client *client)
 
 static int ov6650_video_probe(struct i2c_client *client)
 {
+	struct ov6650 *priv = to_ov6650(client);
 	u8		pidh, pidl, midh, midl;
-	int		ret = 0;
+	int		ret;
+
+	ret = ov6650_s_power(&priv->subdev, 1);
+	if (ret < 0)
+		return ret;
 
 	/*
 	 * check and show product ID and manufacturer ID
@@ -844,12 +849,13 @@ static int ov6650_video_probe(struct i2c_client *client)
 		ret = ov6650_reg_read(client, REG_MIDL, &midl);
 
 	if (ret)
-		return ret;
+		goto done;
 
 	if ((pidh != OV6650_PIDH) || (pidl != OV6650_PIDL)) {
 		dev_err(&client->dev, "Product ID error 0x%02x:0x%02x\n",
 				pidh, pidl);
-		return -ENODEV;
+		ret = -ENODEV;
+		goto done;
 	}
 
 	dev_info(&client->dev,
@@ -859,7 +865,11 @@ static int ov6650_video_probe(struct i2c_client *client)
 	ret = ov6650_reset(client);
 	if (!ret)
 		ret = ov6650_prog_dflt(client);
+	if (!ret)
+		ret = v4l2_ctrl_handler_setup(&priv->hdl);
 
+done:
+	ov6650_s_power(&priv->subdev, 0);
 	return ret;
 }
 
@@ -1019,9 +1029,6 @@ static int ov6650_probe(struct i2c_client *client,
 	priv->colorspace  = V4L2_COLORSPACE_JPEG;
 
 	ret = ov6650_video_probe(client);
-	if (!ret)
-		ret = v4l2_ctrl_handler_setup(&priv->hdl);
-
 	if (ret) {
 		v4l2_ctrl_handler_free(&priv->hdl);
 		kfree(priv);
diff --git a/drivers/media/video/ov772x.c b/drivers/media/video/ov772x.c
index a022662..641f6f4 100644
--- a/drivers/media/video/ov772x.c
+++ b/drivers/media/video/ov772x.c
@@ -962,6 +962,11 @@ static int ov772x_video_probe(struct i2c_client *client)
 	struct ov772x_priv *priv = to_ov772x(client);
 	u8                  pid, ver;
 	const char         *devname;
+	int		    ret;
+
+	ret = ov772x_s_power(&priv->subdev, 1);
+	if (ret < 0)
+		return ret;
 
 	/*
 	 * check and show product ID and manufacturer ID
@@ -981,7 +986,8 @@ static int ov772x_video_probe(struct i2c_client *client)
 	default:
 		dev_err(&client->dev,
 			"Product ID error %x:%x\n", pid, ver);
-		return -ENODEV;
+		ret = -ENODEV;
+		goto done;
 	}
 
 	dev_info(&client->dev,
@@ -991,7 +997,11 @@ static int ov772x_video_probe(struct i2c_client *client)
 		 ver,
 		 i2c_smbus_read_byte_data(client, MIDH),
 		 i2c_smbus_read_byte_data(client, MIDL));
-	return v4l2_ctrl_handler_setup(&priv->hdl);
+	ret = v4l2_ctrl_handler_setup(&priv->hdl);
+
+done:
+	ov772x_s_power(&priv->subdev, 0);
+	return ret;
 }
 
 static const struct v4l2_ctrl_ops ov772x_ctrl_ops = {
diff --git a/drivers/media/video/ov9640.c b/drivers/media/video/ov9640.c
index 8c13b3b..8f3ef23 100644
--- a/drivers/media/video/ov9640.c
+++ b/drivers/media/video/ov9640.c
@@ -592,7 +592,11 @@ static int ov9640_video_probe(struct i2c_client *client)
 	struct ov9640_priv *priv = to_ov9640_sensor(sd);
 	u8		pid, ver, midh, midl;
 	const char	*devname;
-	int		ret = 0;
+	int		ret;
+
+	ret = ov9640_s_power(&priv->subdev, 1);
+	if (ret < 0)
+		return ret;
 
 	/*
 	 * check and show product ID and manufacturer ID
@@ -606,7 +610,7 @@ static int ov9640_video_probe(struct i2c_client *client)
 	if (!ret)
 		ret = ov9640_reg_read(client, OV9640_MIDL, &midl);
 	if (ret)
-		return ret;
+		goto done;
 
 	switch (VERSION(pid, ver)) {
 	case OV9640_V2:
@@ -620,13 +624,18 @@ static int ov9640_video_probe(struct i2c_client *client)
 		break;
 	default:
 		dev_err(&client->dev, "Product ID error %x:%x\n", pid, ver);
-		return -ENODEV;
+		ret = -ENODEV;
+		goto done;
 	}
 
 	dev_info(&client->dev, "%s Product ID %0x:%0x Manufacturer ID %x:%x\n",
 		 devname, pid, ver, midh, midl);
 
-	return v4l2_ctrl_handler_setup(&priv->hdl);
+	ret = v4l2_ctrl_handler_setup(&priv->hdl);
+
+done:
+	ov9640_s_power(&priv->subdev, 0);
+	return ret;
 }
 
 static const struct v4l2_ctrl_ops ov9640_ctrl_ops = {
diff --git a/drivers/media/video/ov9740.c b/drivers/media/video/ov9740.c
index effd0f1..a3b02f7 100644
--- a/drivers/media/video/ov9740.c
+++ b/drivers/media/video/ov9740.c
@@ -856,34 +856,38 @@ static int ov9740_video_probe(struct i2c_client *client)
 	u8 modelhi, modello;
 	int ret;
 
+	ret = ov9740_s_power(&priv->subdev, 1);
+	if (ret < 0)
+		return ret;
+
 	/*
 	 * check and show product ID and manufacturer ID
 	 */
 	ret = ov9740_reg_read(client, OV9740_MODEL_ID_HI, &modelhi);
 	if (ret < 0)
-		goto err;
+		goto done;
 
 	ret = ov9740_reg_read(client, OV9740_MODEL_ID_LO, &modello);
 	if (ret < 0)
-		goto err;
+		goto done;
 
 	priv->model = (modelhi << 8) | modello;
 
 	ret = ov9740_reg_read(client, OV9740_REVISION_NUMBER, &priv->revision);
 	if (ret < 0)
-		goto err;
+		goto done;
 
 	ret = ov9740_reg_read(client, OV9740_MANUFACTURER_ID, &priv->manid);
 	if (ret < 0)
-		goto err;
+		goto done;
 
 	ret = ov9740_reg_read(client, OV9740_SMIA_VERSION, &priv->smiaver);
 	if (ret < 0)
-		goto err;
+		goto done;
 
 	if (priv->model != 0x9740) {
 		ret = -ENODEV;
-		goto err;
+		goto done;
 	}
 
 	priv->ident = V4L2_IDENT_OV9740;
@@ -892,7 +896,10 @@ static int ov9740_video_probe(struct i2c_client *client)
 		 "Manufacturer 0x%02x, SMIA Version 0x%02x\n",
 		 priv->model, priv->revision, priv->manid, priv->smiaver);
 
-err:
+	ret = v4l2_ctrl_handler_setup(&priv->hdl);
+
+done:
+	ov9740_s_power(&priv->subdev, 0);
 	return ret;
 }
 
@@ -976,8 +983,6 @@ static int ov9740_probe(struct i2c_client *client,
 	}
 
 	ret = ov9740_video_probe(client);
-	if (!ret)
-		ret = v4l2_ctrl_handler_setup(&priv->hdl);
 	if (ret < 0) {
 		v4l2_ctrl_handler_free(&priv->hdl);
 		kfree(priv);
diff --git a/drivers/media/video/rj54n1cb0c.c b/drivers/media/video/rj54n1cb0c.c
index ca1cee7..32226c9 100644
--- a/drivers/media/video/rj54n1cb0c.c
+++ b/drivers/media/video/rj54n1cb0c.c
@@ -1296,9 +1296,14 @@ static struct v4l2_subdev_ops rj54n1_subdev_ops = {
 static int rj54n1_video_probe(struct i2c_client *client,
 			      struct rj54n1_pdata *priv)
 {
+	struct rj54n1 *rj54n1 = to_rj54n1(client);
 	int data1, data2;
 	int ret;
 
+	ret = rj54n1_s_power(&rj54n1->subdev, 1);
+	if (ret < 0)
+		return ret;
+
 	/* Read out the chip version register */
 	data1 = reg_read(client, RJ54N1_DEV_CODE);
 	data2 = reg_read(client, RJ54N1_DEV_CODE2);
@@ -1307,18 +1312,21 @@ static int rj54n1_video_probe(struct i2c_client *client,
 		ret = -ENODEV;
 		dev_info(&client->dev, "No RJ54N1CB0C found, read 0x%x:0x%x\n",
 			 data1, data2);
-		goto ei2c;
+		goto done;
 	}
 
 	/* Configure IOCTL polarity from the platform data: 0 or 1 << 7. */
 	ret = reg_write(client, RJ54N1_IOC, priv->ioctl_high << 7);
 	if (ret < 0)
-		goto ei2c;
+		goto done;
 
 	dev_info(&client->dev, "Detected a RJ54N1CB0C chip ID 0x%x:0x%x\n",
 		 data1, data2);
 
-ei2c:
+	ret = v4l2_ctrl_handler_setup(&rj54n1->hdl);
+
+done:
+	rj54n1_s_power(&rj54n1->subdev, 0);
 	return ret;
 }
 
@@ -1382,9 +1390,9 @@ static int rj54n1_probe(struct i2c_client *client,
 	if (ret < 0) {
 		v4l2_ctrl_handler_free(&rj54n1->hdl);
 		kfree(rj54n1);
-		return ret;
 	}
-	return v4l2_ctrl_handler_setup(&rj54n1->hdl);
+
+	return ret;
 }
 
 static int rj54n1_remove(struct i2c_client *client)
diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
index 576146e..d485d32 100644
--- a/drivers/media/video/soc_camera.c
+++ b/drivers/media/video/soc_camera.c
@@ -1068,22 +1068,6 @@ static int soc_camera_probe(struct soc_camera_device *icd)
 	if (ret < 0)
 		goto eadd;
 
-	/*
-	 * This will not yet call v4l2_subdev_core_ops::s_power(1), because the
-	 * 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->pdev, icl);
-	if (ret < 0)
-		goto epower;
-
 	/* Must have icd->vdev before registering the device */
 	ret = video_dev_create(icd);
 	if (ret < 0)
@@ -1153,8 +1137,6 @@ static int soc_camera_probe(struct soc_camera_device *icd)
 
 	ici->ops->remove(icd);
 
-	__soc_camera_power_off(icd);
-
 	mutex_unlock(&icd->video_lock);
 
 	return 0;
@@ -1175,8 +1157,6 @@ eadddev:
 	video_device_release(icd->vdev);
 	icd->vdev = NULL;
 evdc:
-	__soc_camera_power_off(icd);
-epower:
 	ici->ops->remove(icd);
 eadd:
 	regulator_bulk_free(icl->num_regulators, icl->regulators);
diff --git a/drivers/media/video/tw9910.c b/drivers/media/video/tw9910.c
index f283650..140716e 100644
--- a/drivers/media/video/tw9910.c
+++ b/drivers/media/video/tw9910.c
@@ -780,6 +780,7 @@ static int tw9910_video_probe(struct i2c_client *client)
 {
 	struct tw9910_priv *priv = to_tw9910(client);
 	s32 id;
+	int ret;
 
 	/*
 	 * tw9910 only use 8 or 16 bit bus width
@@ -790,6 +791,10 @@ static int tw9910_video_probe(struct i2c_client *client)
 		return -ENODEV;
 	}
 
+	ret = tw9910_s_power(&priv->subdev, 1);
+	if (ret < 0)
+		return ret;
+
 	/*
 	 * check and show Product ID
 	 * So far only revisions 0 and 1 have been seen
@@ -803,7 +808,8 @@ static int tw9910_video_probe(struct i2c_client *client)
 		dev_err(&client->dev,
 			"Product ID error %x:%x\n",
 			id, priv->revision);
-		return -ENODEV;
+		ret = -ENODEV;
+		goto done;
 	}
 
 	dev_info(&client->dev,
@@ -811,7 +817,9 @@ static int tw9910_video_probe(struct i2c_client *client)
 
 	priv->norm = V4L2_STD_NTSC;
 
-	return 0;
+done:
+	tw9910_s_power(&priv->subdev, 0);
+	return ret;
 }
 
 static struct v4l2_subdev_core_ops tw9910_subdev_core_ops = {
-- 
1.7.8.6


^ permalink raw reply related	[flat|nested] 19+ messages in thread
* Re: [PATCH v2 9/9] soc-camera: Push probe-time power management to drivers
  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
  0 siblings, 1 reply; 19+ messages in thread
From: Guennadi Liakhovetski @ 2012-07-10 12:06 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: linux-media

Hi Laurent

On Thu, 5 Jul 2012, Laurent Pinchart wrote:

> Several client drivers access the hardware at probe time, for instance
> to read the probe chip ID. Such chips need to be powered up when being
> probed.
> 
> soc-camera handles this by powering chips up in the soc-camera probe
> implementation. However, this will break with non soc-camera hosts that
> don't perform the same operations.
> 
> Fix the problem by pushing the power up/down from the soc-camera core
> down to individual drivers on a needs basis.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/video/imx074.c     |   21 ++++++++--
>  drivers/media/video/mt9m001.c    |   17 +++++++-
>  drivers/media/video/mt9m111.c    |   80 +++++++++++++++++++++----------------
>  drivers/media/video/mt9t031.c    |   37 +++++++----------
>  drivers/media/video/mt9t112.c    |   12 +++++-
>  drivers/media/video/mt9v022.c    |    5 ++
>  drivers/media/video/ov2640.c     |   11 ++++-
>  drivers/media/video/ov5642.c     |   21 ++++++++--
>  drivers/media/video/ov6650.c     |   19 ++++++---
>  drivers/media/video/ov772x.c     |   14 ++++++-
>  drivers/media/video/ov9640.c     |   17 ++++++--
>  drivers/media/video/ov9740.c     |   23 +++++++----
>  drivers/media/video/rj54n1cb0c.c |   18 ++++++--
>  drivers/media/video/soc_camera.c |   20 ---------
>  drivers/media/video/tw9910.c     |   12 +++++-
>  15 files changed, 204 insertions(+), 123 deletions(-)

[snip]

> diff --git a/drivers/media/video/mt9t031.c b/drivers/media/video/mt9t031.c
> index 9666e20..4f12177 100644
> --- a/drivers/media/video/mt9t031.c
> +++ b/drivers/media/video/mt9t031.c
> @@ -161,14 +161,6 @@ static int mt9t031_idle(struct i2c_client *client)
>  	return ret >= 0 ? 0 : -EIO;
>  }
>  
> -static int mt9t031_disable(struct i2c_client *client)
> -{
> -	/* Disable the chip */
> -	reg_clear(client, MT9T031_OUTPUT_CONTROL, 2);
> -
> -	return 0;
> -}
> -
>  static int mt9t031_s_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> @@ -643,9 +635,15 @@ static int mt9t031_video_probe(struct i2c_client *client)
>  	s32 data;
>  	int ret;
>  
> -	/* Enable the chip */
> -	data = reg_write(client, MT9T031_CHIP_ENABLE, 1);
> -	dev_dbg(&client->dev, "write: %d\n", data);
> +	ret = mt9t031_s_power(&mt9t031->subdev, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = mt9t031_idle(client);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "Failed to initialise the camera\n");
> +		return ret;

grm... don't you have to "goto done" here instead to disable the power again?

> +	}
>  
>  	/* Read out the chip version register */
>  	data = reg_read(client, MT9T031_CHIP_VERSION);
> @@ -657,16 +655,16 @@ static int mt9t031_video_probe(struct i2c_client *client)
>  	default:
>  		dev_err(&client->dev,
>  			"No MT9T031 chip detected, register read %x\n", data);
> -		return -ENODEV;
> +		ret = -ENODEV;
> +		goto done;
>  	}
>  
>  	dev_info(&client->dev, "Detected a MT9T031 chip ID %x\n", data);
>  
> -	ret = mt9t031_idle(client);
> -	if (ret < 0)
> -		dev_err(&client->dev, "Failed to initialise the camera\n");
> -	else
> -		v4l2_ctrl_handler_setup(&mt9t031->hdl);
> +	ret = v4l2_ctrl_handler_setup(&mt9t031->hdl);
> +
> +done:
> +	mt9t031_s_power(&mt9t031->subdev, 0);
>  
>  	return ret;
>  }
> @@ -817,12 +815,7 @@ static int mt9t031_probe(struct i2c_client *client,
>  	mt9t031->xskip = 1;
>  	mt9t031->yskip = 1;
>  
> -	mt9t031_idle(client);
> -
>  	ret = mt9t031_video_probe(client);
> -
> -	mt9t031_disable(client);
> -
>  	if (ret) {
>  		v4l2_ctrl_handler_free(&mt9t031->hdl);
>  		kfree(mt9t031);

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [PATCH v2 9/9] soc-camera: Push probe-time power management to drivers
  2012-07-10 12:06   ` Guennadi Liakhovetski
@ 2012-07-15 13:31     ` Laurent Pinchart
  0 siblings, 0 replies; 19+ messages in thread
From: Laurent Pinchart @ 2012-07-15 13:31 UTC (permalink / raw)
  To: Guennadi Liakhovetski; +Cc: linux-media

Hi Guennadi,

On Tuesday 10 July 2012 14:06:51 Guennadi Liakhovetski wrote:
> On Thu, 5 Jul 2012, Laurent Pinchart wrote:
> > Several client drivers access the hardware at probe time, for instance
> > to read the probe chip ID. Such chips need to be powered up when being
> > probed.
> > 
> > soc-camera handles this by powering chips up in the soc-camera probe
> > implementation. However, this will break with non soc-camera hosts that
> > don't perform the same operations.
> > 
> > Fix the problem by pushing the power up/down from the soc-camera core
> > down to individual drivers on a needs basis.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  drivers/media/video/imx074.c     |   21 ++++++++--
> >  drivers/media/video/mt9m001.c    |   17 +++++++-
> >  drivers/media/video/mt9m111.c    |   80  ++++++++++++++++++--------------
> >  drivers/media/video/mt9t031.c    |   37 +++++++----------
> >  drivers/media/video/mt9t112.c    |   12 +++++-
> >  drivers/media/video/mt9v022.c    |    5 ++
> >  drivers/media/video/ov2640.c     |   11 ++++-
> >  drivers/media/video/ov5642.c     |   21 ++++++++--
> >  drivers/media/video/ov6650.c     |   19 ++++++---
> >  drivers/media/video/ov772x.c     |   14 ++++++-
> >  drivers/media/video/ov9640.c     |   17 ++++++--
> >  drivers/media/video/ov9740.c     |   23 +++++++----
> >  drivers/media/video/rj54n1cb0c.c |   18 ++++++--
> >  drivers/media/video/soc_camera.c |   20 ---------
> >  drivers/media/video/tw9910.c     |   12 +++++-
> >  15 files changed, 204 insertions(+), 123 deletions(-)
> 
> [snip]
> 
> > diff --git a/drivers/media/video/mt9t031.c b/drivers/media/video/mt9t031.c
> > index 9666e20..4f12177 100644
> > --- a/drivers/media/video/mt9t031.c
> > +++ b/drivers/media/video/mt9t031.c
> > @@ -161,14 +161,6 @@ static int mt9t031_idle(struct i2c_client *client)
> > 
> >  	return ret >= 0 ? 0 : -EIO;
> >  
> >  }
> > 
> > -static int mt9t031_disable(struct i2c_client *client)
> > -{
> > -	/* Disable the chip */
> > -	reg_clear(client, MT9T031_OUTPUT_CONTROL, 2);
> > -
> > -	return 0;
> > -}
> > -
> > 
> >  static int mt9t031_s_stream(struct v4l2_subdev *sd, int enable)
> >  {
> >  
> >  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> > 
> > @@ -643,9 +635,15 @@ static int mt9t031_video_probe(struct i2c_client
> > *client)> 
> >  	s32 data;
> >  	int ret;
> > 
> > -	/* Enable the chip */
> > -	data = reg_write(client, MT9T031_CHIP_ENABLE, 1);
> > -	dev_dbg(&client->dev, "write: %d\n", data);
> > +	ret = mt9t031_s_power(&mt9t031->subdev, 1);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = mt9t031_idle(client);
> > +	if (ret < 0) {
> > +		dev_err(&client->dev, "Failed to initialise the camera\n");
> > +		return ret;
> 
> grm... don't you have to "goto done" here instead to disable the power
> again?

Sorry about that one. Will fix.

> > +	}
> > 
> >  	/* Read out the chip version register */
> >  	data = reg_read(client, MT9T031_CHIP_VERSION);
> > 
> > @@ -657,16 +655,16 @@ static int mt9t031_video_probe(struct i2c_client
> > *client)> 
> >  	default:
> >  		dev_err(&client->dev,
> >  		
> >  			"No MT9T031 chip detected, register read %x\n", data);
> > 
> > -		return -ENODEV;
> > +		ret = -ENODEV;
> > +		goto done;
> > 
> >  	}
> >  	
> >  	dev_info(&client->dev, "Detected a MT9T031 chip ID %x\n", data);
> > 
> > -	ret = mt9t031_idle(client);
> > -	if (ret < 0)
> > -		dev_err(&client->dev, "Failed to initialise the camera\n");
> > -	else
> > -		v4l2_ctrl_handler_setup(&mt9t031->hdl);
> > +	ret = v4l2_ctrl_handler_setup(&mt9t031->hdl);
> > +
> > +done:
> > +	mt9t031_s_power(&mt9t031->subdev, 0);
> > 
> >  	return ret;
> >  
> >  }
> > 

-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [PATCH v2 7/9] soc-camera: Continue the power off sequence if one of the steps fails
  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
  0 siblings, 1 reply; 19+ messages in thread
From: David Cohen @ 2012-07-15 22:24 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: Guennadi Liakhovetski, linux-media

Hi Laurent,

Few comments below.

On 07/05/2012 11:38 PM, Laurent Pinchart wrote:
> Powering off a device is a "best effort" task: failure to execute one of
> the steps should not prevent the next steps to be executed. For
> instance, an I2C communication error when putting the chip in stand-by
> mode should not prevent the more agressive next step of turning the
> chip's power supply off.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>   drivers/media/video/soc_camera.c |    9 +++------
>   1 files changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
> index 55b981f..bbd518f 100644
> --- a/drivers/media/video/soc_camera.c
> +++ b/drivers/media/video/soc_camera.c
> @@ -89,18 +89,15 @@ static int soc_camera_power_off(struct soc_camera_device *icd,
>   				struct soc_camera_link *icl)
>   {
>   	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> -	int ret = v4l2_subdev_call(sd, core, s_power, 0);
> +	int ret;
>
> -	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
> -		return ret;
> +	v4l2_subdev_call(sd, core, s_power, 0);

Fair enough. I agree we should not prevent power off because of failure
in this step. But IMO we should not silently bypass it too. How about
an error message?

>
>   	if (icl->power) {
>   		ret = icl->power(icd->control, 0);
> -		if (ret < 0) {
> +		if (ret < 0)
>   			dev_err(icd->pdev,
>   				"Platform failed to power-off the camera.\n");
> -			return ret;
> -		}
>   	}
>
>   	ret = regulator_bulk_disable(icl->num_regulators,

One more comment. Should this function's return value being based fully
on last action? If any earlier error happened but this last step is
fine, IMO we should not return 0.

Kind regards,

David Cohen


^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [PATCH v2 8/9] soc-camera: Add and use soc_camera_power_[on|off]() helper functions
  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
  2012-07-15 23:25     ` David Cohen
  2012-07-17  1:24     ` Laurent Pinchart
  0 siblings, 2 replies; 19+ messages in thread
From: David Cohen @ 2012-07-15 23:17 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: Guennadi Liakhovetski, linux-media

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)
>



^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [PATCH v2 8/9] soc-camera: Add and use soc_camera_power_[on|off]() helper functions
  2012-07-15 23:17   ` David Cohen
@ 2012-07-15 23:25     ` David Cohen
  2012-07-17  1:24     ` Laurent Pinchart
  1 sibling, 0 replies; 19+ messages in thread
From: David Cohen @ 2012-07-15 23:25 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: Guennadi Liakhovetski, linux-media

On 07/16/2012 02:17 AM, David Cohen wrote:
> 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

Let me just correct my sentence:
s/no functional change/no potential functional change/

Br,

David

> 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)
>>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [PATCH v2 7/9] soc-camera: Continue the power off sequence if one of the steps fails
  2012-07-15 22:24   ` David Cohen
@ 2012-07-16 23:45     ` Laurent Pinchart
  2012-07-17 11:03       ` David Cohen
  0 siblings, 1 reply; 19+ messages in thread
From: Laurent Pinchart @ 2012-07-16 23:45 UTC (permalink / raw)
  To: David Cohen; +Cc: Guennadi Liakhovetski, linux-media

Hi David,

Thank you for the review.

On Monday 16 July 2012 01:24:37 David Cohen wrote:
> On 07/05/2012 11:38 PM, Laurent Pinchart wrote:
> > Powering off a device is a "best effort" task: failure to execute one of
> > the steps should not prevent the next steps to be executed. For
> > instance, an I2C communication error when putting the chip in stand-by
> > mode should not prevent the more agressive next step of turning the
> > chip's power supply off.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >   drivers/media/video/soc_camera.c |    9 +++------
> >   1 files changed, 3 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/media/video/soc_camera.c
> > b/drivers/media/video/soc_camera.c index 55b981f..bbd518f 100644
> > --- a/drivers/media/video/soc_camera.c
> > +++ b/drivers/media/video/soc_camera.c
> > @@ -89,18 +89,15 @@ static int soc_camera_power_off(struct
> > soc_camera_device *icd,> 
> >   				struct soc_camera_link *icl)
> >   {
> >   	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> > -	int ret = v4l2_subdev_call(sd, core, s_power, 0);
> > +	int ret;
> > 
> > -	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
> > -		return ret;
> > +	v4l2_subdev_call(sd, core, s_power, 0);
> 
> Fair enough. I agree we should not prevent power off because of failure
> in this step. But IMO we should not silently bypass it too. How about
> an error message?

I'll add that.

> >   	if (icl->power) {
> >   	
> >   		ret = icl->power(icd->control, 0);
> > 
> > -		if (ret < 0) {
> > +		if (ret < 0)
> > 
> >   			dev_err(icd->pdev,
> >   			
> >   				"Platform failed to power-off the camera.\n");
> > 
> > -			return ret;
> > -		}
> > 
> >   	}
> >   	
> >   	ret = regulator_bulk_disable(icl->num_regulators,
> 
> One more comment. Should this function's return value being based fully
> on last action? If any earlier error happened but this last step is
> fine, IMO we should not return 0.

Good point. What about this (on top of the current patch) ?

diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
index bbd518f..7bf21da 100644
--- a/drivers/media/video/soc_camera.c
+++ b/drivers/media/video/soc_camera.c
@@ -89,21 +89,30 @@ static int soc_camera_power_off(struct soc_camera_device *icd,
                                struct soc_camera_link *icl)
 {
        struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
-       int ret;
+       int ret = 0;
+       int err;
 
-       v4l2_subdev_call(sd, core, s_power, 0);
+       err = v4l2_subdev_call(sd, core, s_power, 0);
+       if (err < 0 && err != -ENOIOCTLCMD && err != -ENODEV) {
+               dev_err(icd->pdev, "Subdev failed to power-off the camera.\n");
+               ret = err;
+       }
 
        if (icl->power) {
-               ret = icl->power(icd->control, 0);
-               if (ret < 0)
+               err = icl->power(icd->control, 0);
+               if (err < 0) {
                        dev_err(icd->pdev,
                                "Platform failed to power-off the camera.\n");
+                       ret = ret ? : err;
+               }
        }
 
-       ret = regulator_bulk_disable(icl->num_regulators,
+       err = regulator_bulk_disable(icl->num_regulators,
                                     icl->regulators);
-       if (ret < 0)
+       if (err < 0) {
                dev_err(icd->pdev, "Cannot disable regulators\n");
+               ret = ret ? : err;
+       }
 
        return ret;
 }

-- 
Regards,

Laurent Pinchart


^ permalink raw reply related	[flat|nested] 19+ messages in thread
* Re: [PATCH v2 8/9] soc-camera: Add and use soc_camera_power_[on|off]() helper functions
  2012-07-15 23:17   ` David Cohen
  2012-07-15 23:25     ` David Cohen
@ 2012-07-17  1:24     ` Laurent Pinchart
  2012-07-17 10:40       ` David Cohen
  1 sibling, 1 reply; 19+ messages in thread
From: Laurent Pinchart @ 2012-07-17  1:24 UTC (permalink / raw)
  To: David Cohen; +Cc: Guennadi Liakhovetski, linux-media

Hi David,

Thanks for the review.

On Monday 16 July 2012 02:17:43 David Cohen wrote:
> 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. :)

What do you think of

diff --git a/drivers/media/video/ov9740.c b/drivers/media/video/ov9740.c
index 3eb07c2..10c0ba9 100644
--- a/drivers/media/video/ov9740.c
+++ b/drivers/media/video/ov9740.c
@@ -786,17 +786,27 @@ 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);
-
-       if (!priv->current_enable)
-               return 0;
+       int ret;
 
        if (on) {
-               ov9740_s_fmt(sd, &priv->current_mf);
-               ov9740_s_stream(sd, priv->current_enable);
+               ret = soc_camera_power_on(&client->dev, icl);
+               if (ret < 0)
+                       return ret;
+
+               if (priv->current_enable) {
+                       ov9740_s_fmt(sd, &priv->current_mf);
+                       ov9740_s_stream(sd, 1);
+               }
        } else {
-               ov9740_s_stream(sd, 0);
-               priv->current_enable = true;
+               if (priv->current_enable) {
+                       ov9740_s_stream(sd, 0);
+                       priv->current_enable = true;
+               }
+
+               soc_camera_power_off(&client->dev, icl);
        }
 
        return 0;

-- 
Regards,

Laurent Pinchart


^ permalink raw reply related	[flat|nested] 19+ messages in thread
* Re: [PATCH v2 8/9] soc-camera: Add and use soc_camera_power_[on|off]() helper functions
  2012-07-17  1:24     ` Laurent Pinchart
@ 2012-07-17 10:40       ` David Cohen
  0 siblings, 0 replies; 19+ messages in thread
From: David Cohen @ 2012-07-17 10:40 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: Guennadi Liakhovetski, linux-media

Hi Laurent,

On 07/17/2012 04:24 AM, Laurent Pinchart wrote:
> Hi David,
>
> Thanks for the review.

You're welcome.

>
> On Monday 16 July 2012 02:17:43 David Cohen wrote:
>> 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. :)
>
> What do you think of

Sounds good to me :)

Br,

David Cohen

>
> diff --git a/drivers/media/video/ov9740.c b/drivers/media/video/ov9740.c
> index 3eb07c2..10c0ba9 100644
> --- a/drivers/media/video/ov9740.c
> +++ b/drivers/media/video/ov9740.c
> @@ -786,17 +786,27 @@ 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);
> -
> -       if (!priv->current_enable)
> -               return 0;
> +       int ret;
>
>          if (on) {
> -               ov9740_s_fmt(sd, &priv->current_mf);
> -               ov9740_s_stream(sd, priv->current_enable);
> +               ret = soc_camera_power_on(&client->dev, icl);
> +               if (ret < 0)
> +                       return ret;
> +
> +               if (priv->current_enable) {
> +                       ov9740_s_fmt(sd, &priv->current_mf);
> +                       ov9740_s_stream(sd, 1);
> +               }
>          } else {
> -               ov9740_s_stream(sd, 0);
> -               priv->current_enable = true;
> +               if (priv->current_enable) {
> +                       ov9740_s_stream(sd, 0);
> +                       priv->current_enable = true;
> +               }
> +
> +               soc_camera_power_off(&client->dev, icl);
>          }
>
>          return 0;
>



^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [PATCH v2 7/9] soc-camera: Continue the power off sequence if one of the steps fails
  2012-07-16 23:45     ` Laurent Pinchart
@ 2012-07-17 11:03       ` David Cohen
  0 siblings, 0 replies; 19+ messages in thread
From: David Cohen @ 2012-07-17 11:03 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: Guennadi Liakhovetski, linux-media

Hi Laurent,

On 07/17/2012 02:45 AM, Laurent Pinchart wrote:
> Hi David,
>
> Thank you for the review.

You're welcome.

>
> On Monday 16 July 2012 01:24:37 David Cohen wrote:
>> On 07/05/2012 11:38 PM, Laurent Pinchart wrote:
>>> Powering off a device is a "best effort" task: failure to execute one of
>>> the steps should not prevent the next steps to be executed. For
>>> instance, an I2C communication error when putting the chip in stand-by
>>> mode should not prevent the more agressive next step of turning the
>>> chip's power supply off.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> ---
>>>
>>>    drivers/media/video/soc_camera.c |    9 +++------
>>>    1 files changed, 3 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/media/video/soc_camera.c
>>> b/drivers/media/video/soc_camera.c index 55b981f..bbd518f 100644
>>> --- a/drivers/media/video/soc_camera.c
>>> +++ b/drivers/media/video/soc_camera.c
>>> @@ -89,18 +89,15 @@ static int soc_camera_power_off(struct
>>> soc_camera_device *icd,>
>>>    				struct soc_camera_link *icl)
>>>    {
>>>    	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
>>> -	int ret = v4l2_subdev_call(sd, core, s_power, 0);
>>> +	int ret;
>>>
>>> -	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
>>> -		return ret;
>>> +	v4l2_subdev_call(sd, core, s_power, 0);
>>
>> Fair enough. I agree we should not prevent power off because of failure
>> in this step. But IMO we should not silently bypass it too. How about
>> an error message?
>
> I'll add that.
>
>>>    	if (icl->power) {
>>>    	
>>>    		ret = icl->power(icd->control, 0);
>>>
>>> -		if (ret < 0) {
>>> +		if (ret < 0)
>>>
>>>    			dev_err(icd->pdev,
>>>    			
>>>    				"Platform failed to power-off the camera.\n");
>>>
>>> -			return ret;
>>> -		}
>>>
>>>    	}
>>>    	
>>>    	ret = regulator_bulk_disable(icl->num_regulators,
>>
>> One more comment. Should this function's return value being based fully
>> on last action? If any earlier error happened but this last step is
>> fine, IMO we should not return 0.
>
> Good point. What about this (on top of the current patch) ?

That sounds nice to me :)

Regards,

David Cohen

>
> diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
> index bbd518f..7bf21da 100644
> --- a/drivers/media/video/soc_camera.c
> +++ b/drivers/media/video/soc_camera.c
> @@ -89,21 +89,30 @@ static int soc_camera_power_off(struct soc_camera_device *icd,
>                                  struct soc_camera_link *icl)
>   {
>          struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> -       int ret;
> +       int ret = 0;
> +       int err;
>
> -       v4l2_subdev_call(sd, core, s_power, 0);
> +       err = v4l2_subdev_call(sd, core, s_power, 0);
> +       if (err < 0 && err != -ENOIOCTLCMD && err != -ENODEV) {
> +               dev_err(icd->pdev, "Subdev failed to power-off the camera.\n");
> +               ret = err;
> +       }
>
>          if (icl->power) {
> -               ret = icl->power(icd->control, 0);
> -               if (ret < 0)
> +               err = icl->power(icd->control, 0);
> +               if (err < 0) {
>                          dev_err(icd->pdev,
>                                  "Platform failed to power-off the camera.\n");
> +                       ret = ret ? : err;
> +               }
>          }
>
> -       ret = regulator_bulk_disable(icl->num_regulators,
> +       err = regulator_bulk_disable(icl->num_regulators,
>                                       icl->regulators);
> -       if (ret < 0)
> +       if (err < 0) {
>                  dev_err(icd->pdev, "Cannot disable regulators\n");
> +               ret = ret ? : err;
> +       }
>
>          return ret;
>   }
>



^ permalink raw reply	[flat|nested] 19+ messages in thread
end of thread, other threads:[~2012-07-17 11:03 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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

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.