[PATCH 0/8] Miscellaneous soc-camera patches

[PATCH 0/8] Miscellaneous soc-camera patches

[Laurent Pinchart]

Hi Guennadi,

Here's a set of miscellaneous soc-camera patches that I wrote as part of an
effort to improve the interoperability between soc-camera clients and non
soc-camera hosts (namely the ov772x and the OMAP3 ISP in this case).

All patches have been compile-tested but not runtime-tested as I lack the
necessary hardware. I'd like to first validate the approach, I can then of
course fix any small (or not-so-small) issue you will point out.

Laurent Pinchart (8):
  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: Add and use soc_camera_power_[on|off]() helper functions
  soc-camera: Push probe-time power management to drivers

 drivers/media/video/imx074.c              |   33 +++++++-
 drivers/media/video/mt9m001.c             |   29 +++++++-
 drivers/media/video/mt9m111.c             |  116 ++++++++++++++++++-----------
 drivers/media/video/mt9t031.c             |   29 +++++--
 drivers/media/video/mt9t112.c             |   24 ++++++-
 drivers/media/video/mt9v022.c             |   17 ++++
 drivers/media/video/ov2640.c              |   28 +++++--
 drivers/media/video/ov5642.c              |   32 ++++++--
 drivers/media/video/ov6650.c              |   31 ++++++--
 drivers/media/video/ov772x.c              |   34 +++++++--
 drivers/media/video/ov9640.c              |   30 ++++++-
 drivers/media/video/ov9740.c              |   38 +++++++---
 drivers/media/video/rj54n1cb0c.c          |   30 ++++++-
 drivers/media/video/sh_mobile_csi2.c      |   10 ++-
 drivers/media/video/soc_camera.c          |  107 ++++++++++++---------------
 drivers/media/video/soc_camera_platform.c |   15 ++++-
 drivers/media/video/tw9910.c              |   32 ++++++--
 include/media/soc_camera.h                |    2 +
 18 files changed, 459 insertions(+), 178 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH 1/8] soc-camera: Don't fail at module init time if no device is present

[Laurent Pinchart]

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.3.4

[PATCH 2/8] soc-camera: Pass the physical device to the power operation

[Laurent Pinchart]

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.3.4

[PATCH 3/8] ov2640: Don't access the device in the g_mbus_fmt operation

[Laurent Pinchart]

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 |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/drivers/media/video/ov2640.c b/drivers/media/video/ov2640.c
index 3c2c5d3..d9a427c 100644
--- a/drivers/media/video/ov2640.c
+++ b/drivers/media/video/ov2640.c
@@ -837,10 +837,7 @@ 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);
 	}
 
 	mf->width	= priv->win->width;
-- 
1.7.3.4

[PATCH 4/8] ov772x: Don't access the device in the g_mbus_fmt operation

[Laurent Pinchart]

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.3.4

[PATCH 5/8] tw9910: Don't access the device in the g_mbus_fmt operation

[Laurent Pinchart]

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.3.4

[PATCH 6/8] soc_camera: Don't call .s_power() during probe

[Laurent Pinchart]

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.3.4

[PATCH 7/8] soc-camera: Add and use soc_camera_power_[on|off]() helper functions

[Laurent Pinchart]

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              |   12 ++++
 drivers/media/video/mt9m001.c             |   12 ++++
 drivers/media/video/mt9m111.c             |   50 +++++++++++-----
 drivers/media/video/mt9t031.c             |   11 +++-
 drivers/media/video/mt9t112.c             |   12 ++++
 drivers/media/video/mt9v022.c             |   12 ++++
 drivers/media/video/ov2640.c              |   12 ++++
 drivers/media/video/ov5642.c              |   11 +++-
 drivers/media/video/ov6650.c              |   12 ++++
 drivers/media/video/ov772x.c              |   12 ++++
 drivers/media/video/ov9640.c              |   13 ++++-
 drivers/media/video/ov9740.c              |   15 +++++-
 drivers/media/video/rj54n1cb0c.c          |   12 ++++
 drivers/media/video/sh_mobile_csi2.c      |   10 +++-
 drivers/media/video/soc_camera.c          |   90 +++++++++++++++-------------
 drivers/media/video/soc_camera_platform.c |   15 +++++-
 drivers/media/video/tw9910.c              |   12 ++++
 include/media/soc_camera.h                |    2 +
 18 files changed, 260 insertions(+), 65 deletions(-)

diff --git a/drivers/media/video/imx074.c b/drivers/media/video/imx074.c
index 351e9ba..1166c89 100644
--- a/drivers/media/video/imx074.c
+++ b/drivers/media/video/imx074.c
@@ -268,6 +268,17 @@ 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);
+
+	if (on)
+		return soc_camera_power_on(&client->dev, icl);
+	else
+		return soc_camera_power_off(&client->dev, icl);
+}
+
 static int imx074_g_mbus_config(struct v4l2_subdev *sd,
 				struct v4l2_mbus_config *cfg)
 {
@@ -292,6 +303,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 7e64818..cf2aa00 100644
--- a/drivers/media/video/mt9m001.c
+++ b/drivers/media/video/mt9m001.c
@@ -377,6 +377,17 @@ 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);
+
+	if (on)
+		return soc_camera_power_on(&client->dev, icl);
+	else
+		return soc_camera_power_off(&client->dev, icl);
+}
+
 static int mt9m001_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct mt9m001 *mt9m001 = container_of(ctrl->handler,
@@ -566,6 +577,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..b9bfb4f 100644
--- a/drivers/media/video/mt9m111.c
+++ b/drivers/media/video/mt9m111.c
@@ -832,10 +832,35 @@ 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);
+
+	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 +870,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..a78242a 100644
--- a/drivers/media/video/mt9t112.c
+++ b/drivers/media/video/mt9t112.c
@@ -776,12 +776,24 @@ 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);
+
+	if (on)
+		return soc_camera_power_on(&client->dev, icl);
+	else
+		return soc_camera_power_off(&client->dev, icl);
+}
+
 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 bf63417..5715231 100644
--- a/drivers/media/video/mt9v022.c
+++ b/drivers/media/video/mt9v022.c
@@ -445,6 +445,17 @@ 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);
+
+	if (on)
+		return soc_camera_power_on(&client->dev, icl);
+	else
+		return soc_camera_power_off(&client->dev, icl);
+}
+
 static int mt9v022_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct mt9v022 *mt9v022 = container_of(ctrl->handler,
@@ -664,6 +675,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 d9a427c..cc3da57 100644
--- a/drivers/media/video/ov2640.c
+++ b/drivers/media/video/ov2640.c
@@ -742,6 +742,17 @@ 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);
+
+	if (on)
+		return soc_camera_power_on(&client->dev, icl);
+	else
+		return soc_camera_power_off(&client->dev, icl);
+}
+
 /* Select the nearest higher resolution for capture */
 static const struct ov2640_win_size *ov2640_select_win(u32 *width, u32 *height)
 {
@@ -987,6 +998,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..98de102 100644
--- a/drivers/media/video/ov5642.c
+++ b/drivers/media/video/ov5642.c
@@ -933,13 +933,20 @@ 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)
+		ret = soc_camera_power_on(&client->dev, icl);
+	else
+		ret = soc_camera_power_off(&client->dev, icl);
+	if (ret < 0)
+		return ret;
+
 	if (!on)
 		return 0;
 
-	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..2264c8f 100644
--- a/drivers/media/video/ov6650.c
+++ b/drivers/media/video/ov6650.c
@@ -432,6 +432,17 @@ 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);
+
+	if (on)
+		return soc_camera_power_on(&client->dev, icl);
+	else
+		return soc_camera_power_off(&client->dev, icl);
+}
+
 static int ov6650_g_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
@@ -866,6 +877,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..3f37369 100644
--- a/drivers/media/video/ov772x.c
+++ b/drivers/media/video/ov772x.c
@@ -683,6 +683,17 @@ 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);
+
+	if (on)
+		return soc_camera_power_on(&client->dev, icl);
+	else
+		return soc_camera_power_off(&client->dev, icl);
+}
+
 static const struct ov772x_win_size *ov772x_select_win(u32 width, u32 height)
 {
 	__u32 diff;
@@ -996,6 +1007,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..712cf6f 100644
--- a/drivers/media/video/ov9640.c
+++ b/drivers/media/video/ov9640.c
@@ -333,6 +333,17 @@ 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);
+
+	if (on)
+		return soc_camera_power_on(&client->dev, icl);
+	else
+		return soc_camera_power_off(&client->dev, icl);
+}
+
 /* select nearest higher resolution for capture */
 static void ov9640_res_roundup(u32 *width, u32 *height)
 {
@@ -631,7 +642,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..f0c3c64 100644
--- a/drivers/media/video/rj54n1cb0c.c
+++ b/drivers/media/video/rj54n1cb0c.c
@@ -1180,6 +1180,17 @@ 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);
+
+	if (on)
+		return soc_camera_power_on(&client->dev, icl);
+	else
+		return soc_camera_power_off(&client->dev, icl);
+}
+
 static int rj54n1_s_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct rj54n1 *rj54n1 = container_of(ctrl->handler, struct rj54n1, hdl);
@@ -1230,6 +1241,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/sh_mobile_csi2.c b/drivers/media/video/sh_mobile_csi2.c
index 0528650..88c6911 100644
--- a/drivers/media/video/sh_mobile_csi2.c
+++ b/drivers/media/video/sh_mobile_csi2.c
@@ -276,12 +276,20 @@ static void sh_csi2_client_disconnect(struct sh_csi2 *priv)
 
 static int sh_csi2_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 sh_csi2 *priv = container_of(sd, struct sh_csi2, subdev);
+	int ret;
 
-	if (on)
+	if (on) {
+		ret = soc_camera_power_on(&client->dev, icl);
+		if (ret < 0)
+			return ret;
 		return sh_csi2_client_connect(priv);
+	}
 
 	sh_csi2_client_disconnect(priv);
+	soc_camera_power_off(&client->dev, icl);
 	return 0;
 }
 
diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
index 55b981f..6c50032 100644
--- a/drivers/media/video/soc_camera.c
+++ b/drivers/media/video/soc_camera.c
@@ -50,66 +50,74 @@ 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);
+	int ret;
+
+	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 (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
-		return ret;
+	int ret;
 
 	if (icl->power) {
-		ret = icl->power(icd->control, 0);
-		if (ret < 0) {
-			dev_err(icd->pdev,
+		ret = icl->power(dev, 0);
+		if (ret < 0)
+			dev_err(dev,
 				"Platform failed to power-off the camera.\n");
-			return ret;
-		}
 	}
 
 	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)
@@ -542,7 +550,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;
 
@@ -584,7 +592,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:
@@ -601,8 +609,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);
 
@@ -610,7 +616,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)
@@ -1070,7 +1076,7 @@ static int soc_camera_probe(struct soc_camera_device *icd)
 	 * again after initialisation, even though it shouldn't be needed, we
 	 * don't do any IO here.
 	 */
-	ret = soc_camera_power_on(icd, icl);
+	ret = soc_camera_power_on(NULL, icl);
 	if (ret < 0)
 		goto epower;
 
@@ -1143,7 +1149,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);
 
@@ -1165,7 +1171,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..efd85e7 100644
--- a/drivers/media/video/soc_camera_platform.c
+++ b/drivers/media/video/soc_camera_platform.c
@@ -50,7 +50,20 @@ 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 i2c_client *client = v4l2_get_subdevdata(sd);
+	struct soc_camera_link *icl = soc_camera_i2c_to_link(client);
+
+	if (on)
+		return soc_camera_power_on(&client->dev, icl);
+	else
+		return soc_camera_power_off(&client->dev, icl);
+}
+
+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..61a445f 100644
--- a/drivers/media/video/tw9910.c
+++ b/drivers/media/video/tw9910.c
@@ -566,6 +566,17 @@ 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);
+
+	if (on)
+		return soc_camera_power_on(&client->dev, icl);
+	else
+		return soc_camera_power_off(&client->dev, icl);
+}
+
 static int tw9910_set_frame(struct v4l2_subdev *sd, u32 *width, u32 *height)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
@@ -814,6 +825,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..0b61d85 100644
--- a/include/media/soc_camera.h
+++ b/include/media/soc_camera.h
@@ -253,6 +253,8 @@ unsigned long soc_camera_apply_sensor_flags(struct soc_camera_link *icl,
 					    unsigned long flags);
 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);
 
 /* This is only temporary here - until v4l2-subdev begins to link to video_device */
 #include <linux/i2c.h>
-- 
1.7.3.4

[PATCH 8/8] soc-camera: Push probe-time power management to drivers

[Laurent Pinchart]

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    |   18 ++++++---
 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 |   14 -------
 drivers/media/video/tw9910.c     |   12 +++++-
 15 files changed, 201 insertions(+), 101 deletions(-)

diff --git a/drivers/media/video/imx074.c b/drivers/media/video/imx074.c
index 1166c89..fc86e68 100644
--- a/drivers/media/video/imx074.c
+++ b/drivers/media/video/imx074.c
@@ -313,26 +313,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);
@@ -414,7 +421,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 cf2aa00..b69bb91 100644
--- a/drivers/media/video/mt9m001.c
+++ b/drivers/media/video/mt9m001.c
@@ -493,6 +493,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);
@@ -514,7 +518,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;
@@ -543,11 +548,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 b9bfb4f..36dd39f 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);
@@ -940,6 +905,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:
+	ret = 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..56dd31c 100644
--- a/drivers/media/video/mt9t031.c
+++ b/drivers/media/video/mt9t031.c
@@ -643,6 +643,12 @@ static int mt9t031_video_probe(struct i2c_client *client)
 	s32 data;
 	int ret;
 
+	ret = mt9t031_s_power(&mt9t031->subdev, 1);
+	if (ret < 0)
+		return ret;
+
+	mt9t031_idle(client);
+
 	/* Enable the chip */
 	data = reg_write(client, MT9T031_CHIP_ENABLE, 1);
 	dev_dbg(&client->dev, "write: %d\n", data);
@@ -657,7 +663,8 @@ 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);
@@ -668,6 +675,10 @@ static int mt9t031_video_probe(struct i2c_client *client)
 	else
 		v4l2_ctrl_handler_setup(&mt9t031->hdl);
 
+done:
+	mt9t031_disable(client);
+	mt9t031_s_power(&mt9t031->subdev, 0);
+
 	return ret;
 }
 
@@ -817,12 +828,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 a78242a..ef15375 100644
--- a/drivers/media/video/mt9t112.c
+++ b/drivers/media/video/mt9t112.c
@@ -1044,6 +1044,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
@@ -1061,12 +1066,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 5715231..0698754 100644
--- a/drivers/media/video/mt9v022.c
+++ b/drivers/media/video/mt9v022.c
@@ -581,6 +581,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);
 
@@ -651,6 +655,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 cc3da57..d948367 100644
--- a/drivers/media/video/ov2640.c
+++ b/drivers/media/video/ov2640.c
@@ -957,6 +957,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
 	 */
@@ -975,16 +979,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 98de102..f5acc87 100644
--- a/drivers/media/video/ov5642.c
+++ b/drivers/media/video/ov5642.c
@@ -983,29 +983,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 2264c8f..3c0a711 100644
--- a/drivers/media/video/ov6650.c
+++ b/drivers/media/video/ov6650.c
@@ -832,8 +832,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
@@ -847,12 +852,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,
@@ -862,7 +868,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;
 }
 
@@ -1022,9 +1032,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 3f37369..300ae73 100644
--- a/drivers/media/video/ov772x.c
+++ b/drivers/media/video/ov772x.c
@@ -965,6 +965,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
@@ -984,7 +989,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,
@@ -994,7 +1000,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 712cf6f..22fb14e 100644
--- a/drivers/media/video/ov9640.c
+++ b/drivers/media/video/ov9640.c
@@ -595,7 +595,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
@@ -609,7 +613,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:
@@ -623,13 +627,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 f0c3c64..ab74764 100644
--- a/drivers/media/video/rj54n1cb0c.c
+++ b/drivers/media/video/rj54n1cb0c.c
@@ -1299,9 +1299,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);
@@ -1310,18 +1315,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;
 }
 
@@ -1385,9 +1393,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 6c50032..1ca5642 100644
--- a/drivers/media/video/soc_camera.c
+++ b/drivers/media/video/soc_camera.c
@@ -1070,16 +1070,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.
-	 */
-	ret = soc_camera_power_on(NULL, icl);
-	if (ret < 0)
-		goto epower;
-
 	/* Must have icd->vdev before registering the device */
 	ret = video_dev_create(icd);
 	if (ret < 0)
@@ -1149,8 +1139,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;
@@ -1171,8 +1159,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 61a445f..9cad41e 100644
--- a/drivers/media/video/tw9910.c
+++ b/drivers/media/video/tw9910.c
@@ -783,6 +783,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
@@ -793,6 +794,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
@@ -806,7 +811,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,
@@ -814,7 +820,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.3.4

Re: [PATCH 3/8] ov2640: Don't access the device in the g_mbus_fmt operation

[Guennadi Liakhovetski]

Hi Laurent

Thanks for the patch

On Wed, 23 May 2012, Laurent Pinchart wrote:

> 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 |    5 +----
>  1 files changed, 1 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/video/ov2640.c b/drivers/media/video/ov2640.c
> index 3c2c5d3..d9a427c 100644
> --- a/drivers/media/video/ov2640.c
> +++ b/drivers/media/video/ov2640.c
> @@ -837,10 +837,7 @@ 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);

I think you also have to set

		priv->cfmt_code = V4L2_MBUS_FMT_UYVY8_2X8;

Thanks
Guennadi

>  	}
>  
>  	mf->width	= priv->win->width;
> -- 
> 1.7.3.4
> 

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

Re: [PATCH 7/8] soc-camera: Add and use soc_camera_power_[on|off]() helper functions

[Guennadi Liakhovetski]

Hi Laurent

On Wed, 23 May 2012, 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              |   12 ++++
>  drivers/media/video/mt9m001.c             |   12 ++++
>  drivers/media/video/mt9m111.c             |   50 +++++++++++-----
>  drivers/media/video/mt9t031.c             |   11 +++-
>  drivers/media/video/mt9t112.c             |   12 ++++
>  drivers/media/video/mt9v022.c             |   12 ++++
>  drivers/media/video/ov2640.c              |   12 ++++
>  drivers/media/video/ov5642.c              |   11 +++-
>  drivers/media/video/ov6650.c              |   12 ++++
>  drivers/media/video/ov772x.c              |   12 ++++
>  drivers/media/video/ov9640.c              |   13 ++++-
>  drivers/media/video/ov9740.c              |   15 +++++-
>  drivers/media/video/rj54n1cb0c.c          |   12 ++++
>  drivers/media/video/sh_mobile_csi2.c      |   10 +++-
>  drivers/media/video/soc_camera.c          |   90 +++++++++++++++-------------
>  drivers/media/video/soc_camera_platform.c |   15 +++++-
>  drivers/media/video/tw9910.c              |   12 ++++
>  include/media/soc_camera.h                |    2 +
>  18 files changed, 260 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/media/video/imx074.c b/drivers/media/video/imx074.c
> index 351e9ba..1166c89 100644
> --- a/drivers/media/video/imx074.c
> +++ b/drivers/media/video/imx074.c
> @@ -268,6 +268,17 @@ 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);
> +
> +	if (on)
> +		return soc_camera_power_on(&client->dev, icl);
> +	else
> +		return soc_camera_power_off(&client->dev, icl);

I think an inline function would be nice for these to just write in most 
of your converted drivers

	return soc_camera_s_power(&client->dev, icl, on);

> +}
> +
>  static int imx074_g_mbus_config(struct v4l2_subdev *sd,
>  				struct v4l2_mbus_config *cfg)
>  {
> @@ -292,6 +303,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 7e64818..cf2aa00 100644
> --- a/drivers/media/video/mt9m001.c
> +++ b/drivers/media/video/mt9m001.c
> @@ -377,6 +377,17 @@ 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);
> +
> +	if (on)
> +		return soc_camera_power_on(&client->dev, icl);
> +	else
> +		return soc_camera_power_off(&client->dev, icl);
> +}
> +
>  static int mt9m001_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
>  {
>  	struct mt9m001 *mt9m001 = container_of(ctrl->handler,
> @@ -566,6 +577,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..b9bfb4f 100644
> --- a/drivers/media/video/mt9m111.c
> +++ b/drivers/media/video/mt9m111.c
> @@ -832,10 +832,35 @@ 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);

What do you think, don't we have to soc_camera_power_off() if the above 
resume fails? At least I was doing that in the original 
soc_camera_power_on() and you also preserve it that way.

> +
> +	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 +870,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..a78242a 100644
> --- a/drivers/media/video/mt9t112.c
> +++ b/drivers/media/video/mt9t112.c
> @@ -776,12 +776,24 @@ 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);
> +
> +	if (on)
> +		return soc_camera_power_on(&client->dev, icl);
> +	else
> +		return soc_camera_power_off(&client->dev, icl);
> +}
> +
>  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 bf63417..5715231 100644
> --- a/drivers/media/video/mt9v022.c
> +++ b/drivers/media/video/mt9v022.c
> @@ -445,6 +445,17 @@ 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);
> +
> +	if (on)
> +		return soc_camera_power_on(&client->dev, icl);
> +	else
> +		return soc_camera_power_off(&client->dev, icl);
> +}
> +
>  static int mt9v022_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
>  {
>  	struct mt9v022 *mt9v022 = container_of(ctrl->handler,
> @@ -664,6 +675,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 d9a427c..cc3da57 100644
> --- a/drivers/media/video/ov2640.c
> +++ b/drivers/media/video/ov2640.c
> @@ -742,6 +742,17 @@ 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);
> +
> +	if (on)
> +		return soc_camera_power_on(&client->dev, icl);
> +	else
> +		return soc_camera_power_off(&client->dev, icl);
> +}
> +
>  /* Select the nearest higher resolution for capture */
>  static const struct ov2640_win_size *ov2640_select_win(u32 *width, u32 *height)
>  {
> @@ -987,6 +998,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..98de102 100644
> --- a/drivers/media/video/ov5642.c
> +++ b/drivers/media/video/ov5642.c
> @@ -933,13 +933,20 @@ 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)
> +		ret = soc_camera_power_on(&client->dev, icl);
> +	else
> +		ret = soc_camera_power_off(&client->dev, icl);
> +	if (ret < 0)
> +		return ret;
> +
>  	if (!on)
>  		return 0;

How about

	if (!on)
		return soc_camera_power_off(&client->dev, icl);

	ret = soc_camera_power_on(&client->dev, icl);
	if (ret < 0)
		...

>  
> -	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..2264c8f 100644
> --- a/drivers/media/video/ov6650.c
> +++ b/drivers/media/video/ov6650.c
> @@ -432,6 +432,17 @@ 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);
> +
> +	if (on)
> +		return soc_camera_power_on(&client->dev, icl);
> +	else
> +		return soc_camera_power_off(&client->dev, icl);
> +}
> +
>  static int ov6650_g_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> @@ -866,6 +877,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..3f37369 100644
> --- a/drivers/media/video/ov772x.c
> +++ b/drivers/media/video/ov772x.c
> @@ -683,6 +683,17 @@ 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);
> +
> +	if (on)
> +		return soc_camera_power_on(&client->dev, icl);
> +	else
> +		return soc_camera_power_off(&client->dev, icl);
> +}
> +
>  static const struct ov772x_win_size *ov772x_select_win(u32 width, u32 height)
>  {
>  	__u32 diff;
> @@ -996,6 +1007,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..712cf6f 100644
> --- a/drivers/media/video/ov9640.c
> +++ b/drivers/media/video/ov9640.c
> @@ -333,6 +333,17 @@ 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);
> +
> +	if (on)
> +		return soc_camera_power_on(&client->dev, icl);
> +	else
> +		return soc_camera_power_off(&client->dev, icl);
> +}
> +
>  /* select nearest higher resolution for capture */
>  static void ov9640_res_roundup(u32 *width, u32 *height)
>  {
> @@ -631,7 +642,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..f0c3c64 100644
> --- a/drivers/media/video/rj54n1cb0c.c
> +++ b/drivers/media/video/rj54n1cb0c.c
> @@ -1180,6 +1180,17 @@ 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);
> +
> +	if (on)
> +		return soc_camera_power_on(&client->dev, icl);
> +	else
> +		return soc_camera_power_off(&client->dev, icl);
> +}
> +
>  static int rj54n1_s_ctrl(struct v4l2_ctrl *ctrl)
>  {
>  	struct rj54n1 *rj54n1 = container_of(ctrl->handler, struct rj54n1, hdl);
> @@ -1230,6 +1241,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/sh_mobile_csi2.c b/drivers/media/video/sh_mobile_csi2.c
> index 0528650..88c6911 100644
> --- a/drivers/media/video/sh_mobile_csi2.c
> +++ b/drivers/media/video/sh_mobile_csi2.c
> @@ -276,12 +276,20 @@ static void sh_csi2_client_disconnect(struct sh_csi2 *priv)
>  
>  static int sh_csi2_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 sh_csi2 *priv = container_of(sd, struct sh_csi2, subdev);
> +	int ret;
>  
> -	if (on)
> +	if (on) {
> +		ret = soc_camera_power_on(&client->dev, icl);
> +		if (ret < 0)
> +			return ret;
>  		return sh_csi2_client_connect(priv);
> +	}
>  
>  	sh_csi2_client_disconnect(priv);
> +	soc_camera_power_off(&client->dev, icl);
>  	return 0;

Actually, I might have confused you on this one, sorry. This is not an 
external device, and as such it cannot have platform .s_power() callbacks 
or regulators to switch its power. So, maybe we don't have to patch this 
one at all?

>  }
>  
> diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
> index 55b981f..6c50032 100644
> --- a/drivers/media/video/soc_camera.c
> +++ b/drivers/media/video/soc_camera.c
> @@ -50,66 +50,74 @@ 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);
> +	int ret;
> +
> +	ret = regulator_bulk_enable(icl->num_regulators,
> +				    icl->regulators);

Let's avoid purely stylistic changes ;-)

>  	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 (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
> -		return ret;
> +	int ret;
>  
>  	if (icl->power) {
> -		ret = icl->power(icd->control, 0);
> -		if (ret < 0) {
> -			dev_err(icd->pdev,
> +		ret = icl->power(dev, 0);
> +		if (ret < 0)
> +			dev_err(dev,
>  				"Platform failed to power-off the camera.\n");
> -			return ret;
> -		}
>  	}
>  
>  	ret = regulator_bulk_disable(icl->num_regulators,
>  				     icl->regulators);

This is also a change in behaviour: currently if any of power-off stages 
fails we bail out. With this patch you change it to continue with the next 
stage. I'm not sure which one is more correct, but at least I wouldn't 
silently change it under the counter ;-)

>  	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)
> @@ -542,7 +550,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;
>  
> @@ -584,7 +592,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:
> @@ -601,8 +609,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);
>  
> @@ -610,7 +616,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)
> @@ -1070,7 +1076,7 @@ static int soc_camera_probe(struct soc_camera_device *icd)
>  	 * again after initialisation, even though it shouldn't be needed, we
>  	 * don't do any IO here.
>  	 */
> -	ret = soc_camera_power_on(icd, icl);
> +	ret = soc_camera_power_on(NULL, icl);

Oops, no good... I think, at least dev_err() et al. don't like being 
called with NULL dev... But even without that, I think, this looks so bad, 
that it defeats the whole conversion... IIRC, you didn't like keeping the 
original soc-camera platform device pointer for icl->power(), because non 
soc-camera hosts and platforms don't have that device and would have to 
either pass NULL or some other device pointer. Now, we're switching all 
icl->power() calls to point to the physical device, which is largely 
useless also for soc-camera platforms (they'd have to go back to the 
original platform device if they wish to use that pointer at all), all - 
except one - in which we pass NULL... So, what's the point? :) I really 
would keep the soc-camera platform device pointer for icl->power().

There would be a slight difficulty to get to that pointer in 
soc_camera_power_*(). Just passing a subdevice pointer to it and using 
v4l2_get_subdev_hostdata(sd) isn't sufficient, because that would only 
work in the soc-camera environment. If the same client driver is running 
outside of it, v4l2_get_subdev_hostdata(sd) might well point to something 
completely different. So, we need a way to distinguish, whether this 
client is running on an soc-camera host or not. It is possible to have a 
system with two cameras - one connected to the SoC interface and another 
one to USB with both sensors using soc-camera services. Only one of them 
is associated with an soc-camera device, and another one is not. One way 
to distinguish this would be to scan the devices list in soc_camera.c to 
see, whether any icd->link == icl. If the client is found - we pass its 
platform device pointer to icl->power(). If not - pass NULL. The wrapper 
inline function would then become

	return soc_camera_s_power(icl, on);

Do you see any problems with this approach?

>  	if (ret < 0)
>  		goto epower;
>  
> @@ -1143,7 +1149,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);
>  
> @@ -1165,7 +1171,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..efd85e7 100644
> --- a/drivers/media/video/soc_camera_platform.c
> +++ b/drivers/media/video/soc_camera_platform.c
> @@ -50,7 +50,20 @@ 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 i2c_client *client = v4l2_get_subdevdata(sd);

Nnnnoo... soc_camera_platform is special - it doesn't (have to) have an 
i2c device associated with it. You're only guaranteed to have a subdevice 
and an struct soc_camera_platform_priv instance (the latter actually 
contains the former as its only member). But you have the advantage, that 
this driver always only runs under soc-camera. At least I hope noone ever 
comes up with an idea to use it outside of soc-camera ;-) So, if we accept 
my above proposal to use the platform device for soc_camera_power_*(), 
then you can use something like

	struct soc_camera_platform_info *p = v4l2_get_subdevdata(sd);
	soc_camera_s_power(p->icd->link, on);

> +	struct soc_camera_link *icl = soc_camera_i2c_to_link(client);
> +
> +	if (on)
> +		return soc_camera_power_on(&client->dev, icl);
> +	else
> +		return soc_camera_power_off(&client->dev, icl);
> +}
> +
> +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..61a445f 100644
> --- a/drivers/media/video/tw9910.c
> +++ b/drivers/media/video/tw9910.c
> @@ -566,6 +566,17 @@ 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);
> +
> +	if (on)
> +		return soc_camera_power_on(&client->dev, icl);
> +	else
> +		return soc_camera_power_off(&client->dev, icl);
> +}
> +
>  static int tw9910_set_frame(struct v4l2_subdev *sd, u32 *width, u32 *height)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> @@ -814,6 +825,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..0b61d85 100644
> --- a/include/media/soc_camera.h
> +++ b/include/media/soc_camera.h
> @@ -253,6 +253,8 @@ unsigned long soc_camera_apply_sensor_flags(struct soc_camera_link *icl,
>  					    unsigned long flags);
>  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);
>  
>  /* This is only temporary here - until v4l2-subdev begins to link to video_device */
>  #include <linux/i2c.h>
> -- 
> 1.7.3.4

Let me know if you find any loose ends in my proposals.

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

Re: [PATCH 8/8] soc-camera: Push probe-time power management to drivers

[Guennadi Liakhovetski]

On Wed, 23 May 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    |   18 ++++++---
>  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 |   14 -------
>  drivers/media/video/tw9910.c     |   12 +++++-
>  15 files changed, 201 insertions(+), 101 deletions(-)
> 
> diff --git a/drivers/media/video/imx074.c b/drivers/media/video/imx074.c
> index 1166c89..fc86e68 100644
> --- a/drivers/media/video/imx074.c
> +++ b/drivers/media/video/imx074.c
> @@ -313,26 +313,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);
> @@ -414,7 +421,11 @@ static int imx074_video_probe(struct i2c_client *client)
>  
>  	reg_write(client, GROUPED_PARAMETER_HOLD, 0x00);	/* off */

Looking at this and other soc-camera sensor drivers, most of them do some 
initialisation during probe(), that is not automatically re-applied at any 
point during operation. This means, the current power switching in 
soc-camera core, turning clients off directly after probe() and after each 
last close() only works with "soft" power off variants, e.g., where the 
board only switches off analog parts of a camera sensor and preserves 
register contents. There are indeed multiple boards currently in the 
mainline, implementing the soc_camera_link::power() callback. This means, 
they all either only do the soft power-off, or have been lucky to not need 
any of the lost register contents.

The v4l2_subdev_core_ops::s_power() operation is documented as

   s_power: puts subdevice in power saving mode (on == 0) or normal operation
	mode (on == 1).

"power saving mode" means pretty much the same to me - switch off power 
consuming parts, but keep register contents. So, I think, we're fine here 
just "mechanically" bringing over the power switching functionality into 
client drivers, we might only want to improve struct soc_camera_link 
documentation :-)

>  
> -	return 0;
> +	ret = 0;
> +
> +done:
> +	imx074_s_power(subdev, 0);
> +	return ret;
>  }
>  
>  static int imx074_probe(struct i2c_client *client,

[snip]

> @@ -940,6 +905,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);

You're losing this return code...

> +
> +done:
> +	ret = mt9m111_s_power(&mt9m111->subdev, 0);
> +	return ret;

	return mt9m111_s_power(&mt9m111->subdev, 0);

But in mt9m001 you discard return code from *_s_power(0) and return the 
error from v4l2_ctrl_handler_setup(). Better be consistent, IMHO.

> +}
> +
>  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..56dd31c 100644
> --- a/drivers/media/video/mt9t031.c
> +++ b/drivers/media/video/mt9t031.c
> @@ -643,6 +643,12 @@ static int mt9t031_video_probe(struct i2c_client *client)
>  	s32 data;
>  	int ret;
>  
> +	ret = mt9t031_s_power(&mt9t031->subdev, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	mt9t031_idle(client);
> +

There's one more call to mt9t031_idle() a couple of lines down in 
mt9t031_video_probe()... I think, the latter one can be dropped 
together with...

>  	/* Enable the chip */
>  	data = reg_write(client, MT9T031_CHIP_ENABLE, 1);

the one above - it starts the read-out, which we don't necessarily want 
immediately after probe(), and it is anyway disabled again a few lines 
further down in mt9t031_disable().

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

Re: [PATCH 3/8] ov2640: Don't access the device in the g_mbus_fmt operation

[Laurent Pinchart]

Hi Guennadi,

Thanks for the review.

On Thursday 21 June 2012 14:28:04 Guennadi Liakhovetski wrote:
> On Wed, 23 May 2012, Laurent Pinchart wrote:
> > 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 |    5 +----
> >  1 files changed, 1 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/media/video/ov2640.c b/drivers/media/video/ov2640.c
> > index 3c2c5d3..d9a427c 100644
> > --- a/drivers/media/video/ov2640.c
> > +++ b/drivers/media/video/ov2640.c
> > @@ -837,10 +837,7 @@ 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);
> 
> I think you also have to set
> 
> 		priv->cfmt_code = V4L2_MBUS_FMT_UYVY8_2X8;

You're right. I'll fix that.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 7/8] soc-camera: Add and use soc_camera_power_[on|off]() helper functions

[Laurent Pinchart]

Hi Guennadi,

Thanks for the review.

On Thursday 21 June 2012 23:15:14 Guennadi Liakhovetski wrote:
> On Wed, 23 May 2012, 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>

[snip]

> > diff --git a/drivers/media/video/imx074.c b/drivers/media/video/imx074.c
> > index 351e9ba..1166c89 100644
> > --- a/drivers/media/video/imx074.c
> > +++ b/drivers/media/video/imx074.c
> > @@ -268,6 +268,17 @@ static int imx074_g_chip_ident(struct v4l2_subdev

[snip]

> > +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);
> > +
> > +	if (on)
> > +		return soc_camera_power_on(&client->dev, icl);
> > +	else
> > +		return soc_camera_power_off(&client->dev, icl);
> 
> I think an inline function would be nice for these to just write in most
> of your converted drivers
> 
> 	return soc_camera_s_power(&client->dev, icl, on);

OK.

> > +}

[snip]

> > diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
> > index b0c5299..b9bfb4f 100644
> > --- a/drivers/media/video/mt9m111.c
> > +++ b/drivers/media/video/mt9m111.c
> > @@ -832,10 +832,35 @@ 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);
> 
> What do you think, don't we have to soc_camera_power_off() if the above
> resume fails? At least I was doing that in the original
> soc_camera_power_on() and you also preserve it that way.

I agree. I'll fix that.

> > +
> > +	return ret;
> > +}

[snip]

> > diff --git a/drivers/media/video/ov5642.c b/drivers/media/video/ov5642.c
> > index 0bc9331..98de102 100644
> > --- a/drivers/media/video/ov5642.c
> > +++ b/drivers/media/video/ov5642.c
> > @@ -933,13 +933,20 @@ 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)
> > +		ret = soc_camera_power_on(&client->dev, icl);
> > +	else
> > +		ret = soc_camera_power_off(&client->dev, icl);
> > +	if (ret < 0)
> > +		return ret;
> > +
> >  	if (!on)
> >  		return 0;
> 
> How about
> 
> 	if (!on)
> 		return soc_camera_power_off(&client->dev, icl);
> 
> 	ret = soc_camera_power_on(&client->dev, icl);
> 	if (ret < 0)
> 		...

That looks good to me.

> > -	client = v4l2_get_subdevdata(sd);
> > 
> >  	ret = ov5642_write_array(client, ov5642_default_regs_init);
> >  	if (!ret)
> >  		ret = ov5642_set_resolution(sd);
> > 

[snip]

> > diff --git a/drivers/media/video/sh_mobile_csi2.c
> > b/drivers/media/video/sh_mobile_csi2.c index 0528650..88c6911 100644
> > --- a/drivers/media/video/sh_mobile_csi2.c
> > +++ b/drivers/media/video/sh_mobile_csi2.c
> > @@ -276,12 +276,20 @@ static void sh_csi2_client_disconnect(struct sh_csi2
> > *priv)> 
> >  static int sh_csi2_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 sh_csi2 *priv = container_of(sd, struct sh_csi2, subdev);
> > +	int ret;
> > 
> > -	if (on)
> > +	if (on) {
> > +		ret = soc_camera_power_on(&client->dev, icl);
> > +		if (ret < 0)
> > +			return ret;
> > 
> >  		return sh_csi2_client_connect(priv);
> > +	}
> > 
> >  	sh_csi2_client_disconnect(priv);
> > 
> > +	soc_camera_power_off(&client->dev, icl);
> > 
> >  	return 0;
> 
> Actually, I might have confused you on this one, sorry. This is not an
> external device, and as such it cannot have platform .s_power() callbacks
> or regulators to switch its power. So, maybe we don't have to patch this
> one at all?

Good, I'll skip it :-)

> >  }
> > 
> > diff --git a/drivers/media/video/soc_camera.c
> > b/drivers/media/video/soc_camera.c index 55b981f..6c50032 100644
> > --- a/drivers/media/video/soc_camera.c
> > +++ b/drivers/media/video/soc_camera.c

[snip]

> > -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);
> > +	int ret;
> > +
> > +	ret = regulator_bulk_enable(icl->num_regulators,
> > +				    icl->regulators);
> 
> Let's avoid purely stylistic changes ;-)

Next time I'll try to hide them better ;-)

[snip]

> > -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 (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
> > -		return ret;
> > +	int ret;
> > 
> >  	if (icl->power) {
> > -		ret = icl->power(icd->control, 0);
> > -		if (ret < 0) {
> > -			dev_err(icd->pdev,
> > +		ret = icl->power(dev, 0);
> > +		if (ret < 0)
> > +			dev_err(dev,
> >  				"Platform failed to power-off the camera.\n");
> > -			return ret;
> > -		}
> >  	}
> >  	
> >  	ret = regulator_bulk_disable(icl->num_regulators,
> >  				     icl->regulators);
> 
> This is also a change in behaviour: currently if any of power-off stages
> fails we bail out. With this patch you change it to continue with the next
> stage. I'm not sure which one is more correct, but at least I wouldn't
> silently change it under the counter ;-)

During power off I think it's better to continue to the next stage. Would you 
like me to split this to another patch, or to mention it in the commit message 
?

> >  	if (ret < 0)
> > -		dev_err(icd->pdev, "Cannot disable regulators\n");
> > +		dev_err(dev, "Cannot disable regulators\n");
> > 
> >  	return ret;
> >  }

[snip]

> > @@ -1070,7 +1076,7 @@ static int soc_camera_probe(struct soc_camera_device
> > *icd)> 
> >  	 * again after initialisation, even though it shouldn't be needed, we
> >  	 * don't do any IO here.
> >  	 */
> > 
> > -	ret = soc_camera_power_on(icd, icl);
> > +	ret = soc_camera_power_on(NULL, icl);
> 
> Oops, no good... I think, at least dev_err() et al. don't like being
> called with NULL dev... But even without that, I think, this looks so bad,
> that it defeats the whole conversion... IIRC, you didn't like keeping the
> original soc-camera platform device pointer for icl->power(), because non
> soc-camera hosts and platforms don't have that device and would have to
> either pass NULL or some other device pointer. Now, we're switching all
> icl->power() calls to point to the physical device, which is largely
> useless also for soc-camera platforms (they'd have to go back to the
> original platform device if they wish to use that pointer at all), all -
> except one - in which we pass NULL... So, what's the point? :) I really
> would keep the soc-camera platform device pointer for icl->power().
> 
> There would be a slight difficulty to get to that pointer in
> soc_camera_power_*(). Just passing a subdevice pointer to it and using
> v4l2_get_subdev_hostdata(sd) isn't sufficient, because that would only
> work in the soc-camera environment. If the same client driver is running
> outside of it, v4l2_get_subdev_hostdata(sd) might well point to something
> completely different. So, we need a way to distinguish, whether this
> client is running on an soc-camera host or not. It is possible to have a
> system with two cameras - one connected to the SoC interface and another
> one to USB with both sensors using soc-camera services. Only one of them
> is associated with an soc-camera device, and another one is not. One way
> to distinguish this would be to scan the devices list in soc_camera.c to
> see, whether any icd->link == icl. If the client is found - we pass its
> platform device pointer to icl->power(). If not - pass NULL. The wrapper
> inline function would then become
> 
> 	return soc_camera_s_power(icl, on);
> 
> Do you see any problems with this approach?

If I had proposed such a hack I would have sworn you would have rejected it 
;-)

My first idea was to get rid of the device pointer completely. None of the 
platform callbacks use it. You didn't really like that, as it would make 
implementation of a future platform that would require the device pointer more 
complex. I've thus decided to switch to pass the physical struct device 
associated with the device to the power functions, down to the board code. I 
don't think board code would need to go back to the original platform device 
to discriminate between devices, checking the physical device properties (such 
as the I2C address or bus number for instance) should be enough.

With your approach we would pass a different device pointer to platform code 
depending on whether we use a soc-camera host or not. With non soc-camera 
hosts the device pointer would always be NULL, which wouldn't let board code 
discrimate between devices. That's not a good situation either.

I'm really beginning to wonder whether we're not trying to fix a problem that 
will go away in the future, as we'll need to find a way to remove the platform 
callbacks when switching to the device tree anyway. Wouldn't it be better to 
remove the device pointer completely ?

(As for the dev_* macros, we could pass the platform device pointer here, and 
the physical device pointer when the power functions are called from devices).

And now I've just remembered that patch 8/8 removes this soc_camera_power_on() 
completely, so the above argument just got pointless, hasn't it ? :-/

[snip]

> > diff --git a/drivers/media/video/soc_camera_platform.c
> > b/drivers/media/video/soc_camera_platform.c index f59ccad..efd85e7 100644
> > --- a/drivers/media/video/soc_camera_platform.c
> > +++ b/drivers/media/video/soc_camera_platform.c
> > @@ -50,7 +50,20 @@ 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 i2c_client *client = v4l2_get_subdevdata(sd);
> 
> Nnnnoo... soc_camera_platform is special - it doesn't (have to) have an
> i2c device associated with it.

Oops. You're right. This might be the result of copy and paste late at night.

> You're only guaranteed to have a subdevice and an struct
> soc_camera_platform_priv instance (the latter actually contains the former
> as its only member). But you have the advantage, that this driver always
> only runs under soc-camera. At least I hope noone ever comes up with an idea
> to use it outside of soc-camera ;-) So, if we accept my above proposal to
> use the platform device for soc_camera_power_*(), then you can use something
> like
> 
> 	struct soc_camera_platform_info *p = v4l2_get_subdevdata(sd);
> 	soc_camera_s_power(p->icd->link, on);

A pointer to the physical device is stored at probe time in p->icd->control. 
What about

        struct soc_camera_platform_info *p = v4l2_get_subdevdata(sd);

        return soc_camera_set_power(p->icd->control, p->icd->link, on);

> > +	struct soc_camera_link *icl = soc_camera_i2c_to_link(client);
> > +
> > +	if (on)
> > +		return soc_camera_power_on(&client->dev, icl);
> > +	else
> > +		return soc_camera_power_off(&client->dev, icl);
> > +}

[snip]

> Let me know if you find any loose ends in my proposals.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 8/8] soc-camera: Push probe-time power management to drivers

[Laurent Pinchart]

Hi Guennadi,

Thanks for the review.

On Friday 22 June 2012 13:23:23 Guennadi Liakhovetski wrote:
> On Wed, 23 May 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    |   18 ++++++---
> >  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 |   14 -------
> >  drivers/media/video/tw9910.c     |   12 +++++-
> >  15 files changed, 201 insertions(+), 101 deletions(-)
> > 
> > diff --git a/drivers/media/video/imx074.c b/drivers/media/video/imx074.c
> > index 1166c89..fc86e68 100644
> > --- a/drivers/media/video/imx074.c
> > +++ b/drivers/media/video/imx074.c

[snip]

> > @@ -414,7 +421,11 @@ static int imx074_video_probe(struct i2c_client
> > *client)
> >  	reg_write(client, GROUPED_PARAMETER_HOLD, 0x00);	/* off */
> 
> Looking at this and other soc-camera sensor drivers, most of them do some
> initialisation during probe(), that is not automatically re-applied at any
> point during operation. This means, the current power switching in
> soc-camera core, turning clients off directly after probe() and after each
> last close() only works with "soft" power off variants, e.g., where the
> board only switches off analog parts of a camera sensor and preserves
> register contents. There are indeed multiple boards currently in the
> mainline, implementing the soc_camera_link::power() callback. This means,
> they all either only do the soft power-off, or have been lucky to not need
> any of the lost register contents.
> 
> The v4l2_subdev_core_ops::s_power() operation is documented as
> 
>    s_power: puts subdevice in power saving mode (on == 0) or normal
> operation mode (on == 1).
> 
> "power saving mode" means pretty much the same to me - switch off power
> consuming parts, but keep register contents. So, I think, we're fine here
> just "mechanically" bringing over the power switching functionality into
> client drivers, we might only want to improve struct soc_camera_link
> documentation :-)

That should be doable :-)

> > -	return 0;
> > +	ret = 0;
> > +
> > +done:
> > +	imx074_s_power(subdev, 0);
> > +	return ret;

[snip]

> > +/*
> > + * 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);
> 
> You're losing this return code...
> 
> > +
> > +done:
> > +	ret = mt9m111_s_power(&mt9m111->subdev, 0);
> > +	return ret;
> 
> 	return mt9m111_s_power(&mt9m111->subdev, 0);
> 
> But in mt9m001 you discard return code from *_s_power(0) and return the
> error from v4l2_ctrl_handler_setup(). Better be consistent, IMHO.

My bad, I'll fix that.

> > +}
> > +
> > 
> >  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..56dd31c 100644
> > --- a/drivers/media/video/mt9t031.c
> > +++ b/drivers/media/video/mt9t031.c
> > @@ -643,6 +643,12 @@ static int mt9t031_video_probe(struct i2c_client
> > *client)> 
> >  	s32 data;
> >  	int ret;
> > 
> > +	ret = mt9t031_s_power(&mt9t031->subdev, 1);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	mt9t031_idle(client);
> > +
> 
> There's one more call to mt9t031_idle() a couple of lines down in
> mt9t031_video_probe()... I think, the latter one can be dropped
> together with...
> 
> >  	/* Enable the chip */
> >  	data = reg_write(client, MT9T031_CHIP_ENABLE, 1);
> 
> the one above - it starts the read-out, which we don't necessarily want
> immediately after probe(), and it is anyway disabled again a few lines
> further down in mt9t031_disable().

I'll remove the chip enable, the second call to mt9t031_idle(), and the call 
to mt9t031_disable() as the first mt9t031_idle() has already disabled the 
chip.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 7/8] soc-camera: Add and use soc_camera_power_[on|off]() helper functions

[Guennadi Liakhovetski]

On Thu, 28 Jun 2012, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> Thanks for the review.
> 
> On Thursday 21 June 2012 23:15:14 Guennadi Liakhovetski wrote:
> > On Wed, 23 May 2012, Laurent Pinchart wrote:

> > > -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 (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
> > > -		return ret;
> > > +	int ret;
> > > 
> > >  	if (icl->power) {
> > > -		ret = icl->power(icd->control, 0);
> > > -		if (ret < 0) {
> > > -			dev_err(icd->pdev,
> > > +		ret = icl->power(dev, 0);
> > > +		if (ret < 0)
> > > +			dev_err(dev,
> > >  				"Platform failed to power-off the camera.\n");
> > > -			return ret;
> > > -		}
> > >  	}
> > >  	
> > >  	ret = regulator_bulk_disable(icl->num_regulators,
> > >  				     icl->regulators);
> > 
> > This is also a change in behaviour: currently if any of power-off stages
> > fails we bail out. With this patch you change it to continue with the next
> > stage. I'm not sure which one is more correct, but at least I wouldn't
> > silently change it under the counter ;-)
> 
> During power off I think it's better to continue to the next stage. Would you 
> like me to split this to another patch, or to mention it in the commit message 
> ?

Ok, since this patch is so big and touches so mane other drivers, I think 
it'd be cleaner splitting this into a separate patch, preferably - before 
this one.

> > > @@ -1070,7 +1076,7 @@ static int soc_camera_probe(struct soc_camera_device
> > > *icd)> 
> > >  	 * again after initialisation, even though it shouldn't be needed, we
> > >  	 * don't do any IO here.
> > >  	 */
> > > 
> > > -	ret = soc_camera_power_on(icd, icl);
> > > +	ret = soc_camera_power_on(NULL, icl);
> > 
> > Oops, no good... I think, at least dev_err() et al. don't like being
> > called with NULL dev... But even without that, I think, this looks so bad,
> > that it defeats the whole conversion... IIRC, you didn't like keeping the
> > original soc-camera platform device pointer for icl->power(), because non
> > soc-camera hosts and platforms don't have that device and would have to
> > either pass NULL or some other device pointer. Now, we're switching all
> > icl->power() calls to point to the physical device, which is largely
> > useless also for soc-camera platforms (they'd have to go back to the
> > original platform device if they wish to use that pointer at all), all -
> > except one - in which we pass NULL... So, what's the point? :) I really
> > would keep the soc-camera platform device pointer for icl->power().
> > 
> > There would be a slight difficulty to get to that pointer in
> > soc_camera_power_*(). Just passing a subdevice pointer to it and using
> > v4l2_get_subdev_hostdata(sd) isn't sufficient, because that would only
> > work in the soc-camera environment. If the same client driver is running
> > outside of it, v4l2_get_subdev_hostdata(sd) might well point to something
> > completely different. So, we need a way to distinguish, whether this
> > client is running on an soc-camera host or not. It is possible to have a
> > system with two cameras - one connected to the SoC interface and another
> > one to USB with both sensors using soc-camera services. Only one of them
> > is associated with an soc-camera device, and another one is not. One way
> > to distinguish this would be to scan the devices list in soc_camera.c to
> > see, whether any icd->link == icl. If the client is found - we pass its
> > platform device pointer to icl->power(). If not - pass NULL. The wrapper
> > inline function would then become
> > 
> > 	return soc_camera_s_power(icl, on);
> > 
> > Do you see any problems with this approach?
> 
> If I had proposed such a hack I would have sworn you would have rejected it 
> ;-)

Of course, what did you expect? ;-)

> My first idea was to get rid of the device pointer completely. None of the 
> platform callbacks use it. You didn't really like that, as it would make 
> implementation of a future platform that would require the device pointer more 
> complex.

Not only this. Pushing a patch that touches drivers, arch/sh and arch/arm 
is something, that doesn't seem to be favoured very highly these days. 
You'd need something as ugly as - (1) add a new callback without the 
device pointer and convert soc-camera to check-and-call both, (2) port all 
platforms to the new one, (3) remove the old one. All this for no 
practical gain, especially since they'll be gone soon with DT:-)

> I've thus decided to switch to pass the physical struct device 
> associated with the device to the power functions, down to the board code. I 
> don't think board code would need to go back to the original platform device 
> to discriminate between devices, checking the physical device properties (such 
> as the I2C address or bus number for instance) should be enough.
> 
> With your approach we would pass a different device pointer to platform code 
> depending on whether we use a soc-camera host or not. With non soc-camera 
> hosts the device pointer would always be NULL, which wouldn't let board code 
> discrimate between devices. That's not a good situation either.

Remember that those would normally be different platforms, you won't need 
to distinguish between valid-pointer / NULL-pointer in one function. 
Those, that know they'll be getting NULL will just ignore it. A case, like 
in my example, with a USB camera and an SoC camera and both using the same 
.power() function is rather unlikely, I think.

> I'm really beginning to wonder whether we're not trying to fix a problem that 
> will go away in the future, as we'll need to find a way to remove the platform 
> callbacks when switching to the device tree anyway. Wouldn't it be better to 
> remove the device pointer completely ?

See above :)

> (As for the dev_* macros, we could pass the platform device pointer here, and 
> the physical device pointer when the power functions are called from devices).
> 
> And now I've just remembered that patch 8/8 removes this soc_camera_power_on() 
> completely, so the above argument just got pointless, hasn't it ? :-/

Almost. Ok, you can choose any of the following 3 options (or propose 
more, of course:-)): (1) follow my idea with list scanning, (2) swap 
patches 8/9 and 9/9 to first remove power managing in probe(), then you 
don't have the NULL problem, (3) keep everything as is, only use the 
platform device pointer in this single call instead of NULL and put a 
comment, saying like - yes, this is a dirty hack, will go with the next 
commit.

> [snip]
> 
> > > diff --git a/drivers/media/video/soc_camera_platform.c
> > > b/drivers/media/video/soc_camera_platform.c index f59ccad..efd85e7 100644
> > > --- a/drivers/media/video/soc_camera_platform.c
> > > +++ b/drivers/media/video/soc_camera_platform.c
> > > @@ -50,7 +50,20 @@ 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 i2c_client *client = v4l2_get_subdevdata(sd);
> > 
> > Nnnnoo... soc_camera_platform is special - it doesn't (have to) have an
> > i2c device associated with it.
> 
> Oops. You're right. This might be the result of copy and paste late at night.
> 
> > You're only guaranteed to have a subdevice and an struct
> > soc_camera_platform_priv instance (the latter actually contains the former
> > as its only member). But you have the advantage, that this driver always
> > only runs under soc-camera. At least I hope noone ever comes up with an idea
> > to use it outside of soc-camera ;-) So, if we accept my above proposal to
> > use the platform device for soc_camera_power_*(), then you can use something
> > like
> > 
> > 	struct soc_camera_platform_info *p = v4l2_get_subdevdata(sd);
> > 	soc_camera_s_power(p->icd->link, on);
> 
> A pointer to the physical device is stored at probe time in p->icd->control. 
> What about
> 
>         struct soc_camera_platform_info *p = v4l2_get_subdevdata(sd);
> 
>         return soc_camera_set_power(p->icd->control, p->icd->link, on);

Yep, seems ok.

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