[PATCH 0/5] smiapp: Runtime PM support

[PATCH 0/5] smiapp: Runtime PM support

[Sakari Ailus]

Hi,

This set adds runtime PM support for the smiapp driver. The old s_power()
callback is made redundant and removed as a result.

These patches go on top of my other patches in the "[PATCH v2 00/17] More
smiapp cleanups, fixes" which I just sent to linux-media.

The patches can also be found here:

<URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=smiapp-runtime-pm>

-- 
Kind regards,
Sakari

[PATCH 1/5] smiapp: Drop BUG_ON() in suspend path

[Sakari Ailus]

Checking that the mutex is not acquired is unnecessary for user processes
are stopped by this point. Drop the check.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp/smiapp-core.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 61827fd..bdc5d1b 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -2687,8 +2687,6 @@ static int smiapp_suspend(struct device *dev)
 	struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
 	bool streaming;
 
-	BUG_ON(mutex_is_locked(&sensor->mutex));
-
 	if (sensor->power_count == 0)
 		return 0;
 
-- 
2.1.4

[PATCH 2/5] smiapp: Set device for pixel array and binner

[Sakari Ailus]

The dev field of the v4l2_subdev was left NULL for the pixel array and
binner sub-devices. Fix this.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp/smiapp-core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index bdc5d1b..fe1f738 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -2589,6 +2589,7 @@ static void smiapp_create_subdev(struct smiapp_sensor *sensor,
 
 	ssd->sd.internal_ops = &smiapp_internal_ops;
 	ssd->sd.owner = THIS_MODULE;
+	ssd->sd.dev = &client->dev;
 	v4l2_set_subdevdata(&ssd->sd, client);
 }
 
-- 
2.1.4

Re: [PATCH 2/5] smiapp: Set device for pixel array and binner

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 310 bytes --]

Hi,

On Thu, Sep 15, 2016 at 02:29:18PM +0300, Sakari Ailus wrote:
> The dev field of the v4l2_subdev was left NULL for the pixel array and
> binner sub-devices. Fix this.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-By: Sebastian Reichel <sre@kernel.org>

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 3/5] smiapp: Set use suspend and resume ops for other functions

[Sakari Ailus]

Use the suspend and resume ops for freeze, thaw, poweroff and restore
callbacks as well.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp/smiapp-core.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index fe1f738..e1d1459 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -3090,8 +3090,7 @@ static const struct i2c_device_id smiapp_id_table[] = {
 MODULE_DEVICE_TABLE(i2c, smiapp_id_table);
 
 static const struct dev_pm_ops smiapp_pm_ops = {
-	.suspend	= smiapp_suspend,
-	.resume		= smiapp_resume,
+	SET_SYSTEM_SLEEP_PM_OPS(smiapp_suspend, smiapp_resume)
 };
 
 static struct i2c_driver smiapp_i2c_driver = {
-- 
2.1.4

Re: [PATCH 3/5] smiapp: Set use suspend and resume ops for other functions

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 298 bytes --]

Hi,

On Thu, Sep 15, 2016 at 02:29:19PM +0300, Sakari Ailus wrote:
> Use the suspend and resume ops for freeze, thaw, poweroff and restore
> callbacks as well.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-By: Sebastian Reichel <sre@kernel.org>

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 4/5] smiapp: Use runtime PM

[Sakari Ailus]

Use runtime PM to manage power. The s_power() core sub-device callback is
removed as it is no longer needed.

The power management of the sensor is changed so that it is no longer
dependent on open file descriptors on sub-device or use_count in the media
entity but solely will be powered on as needed for probing and streaming.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp/smiapp-core.c | 139 +++++++++++++++++----------------
 drivers/media/i2c/smiapp/smiapp.h      |  11 +--
 2 files changed, 71 insertions(+), 79 deletions(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index e1d1459..ba5ad36 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -26,6 +26,7 @@
 #include <linux/gpio.h>
 #include <linux/gpio/consumer.h>
 #include <linux/module.h>
+#include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <linux/smiapp.h>
@@ -415,10 +416,14 @@ static int smiapp_set_ctrl(struct v4l2_ctrl *ctrl)
 	struct smiapp_sensor *sensor =
 		container_of(ctrl->handler, struct smiapp_subdev, ctrl_handler)
 			->sensor;
+	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
 	u32 orient = 0;
 	int exposure;
 	int rval;
 
+	if (pm_runtime_suspended(&client->dev))
+		return 0;
+
 	switch (ctrl->id) {
 	case V4L2_CID_ANALOGUE_GAIN:
 		return smiapp_write(
@@ -922,6 +927,9 @@ static int smiapp_update_mode(struct smiapp_sensor *sensor)
 	unsigned int binning_mode;
 	int rval;
 
+	if (pm_runtime_suspended(&client->dev))
+		return 0;
+
 	/* Binning has to be set up here; it affects limits */
 	if (sensor->binning_horizontal == 1 &&
 	    sensor->binning_vertical == 1) {
@@ -1198,9 +1206,17 @@ out:
  * Power management
  */
 
-static int smiapp_power_on(struct smiapp_sensor *sensor)
+static int smiapp_power_on(struct device *dev)
 {
-	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
+	struct smiapp_subdev *ssd = to_smiapp_subdev(subdev);
+	/*
+	 * The sub-device related to the I2C device is always the
+	 * source one, i.e. ssds[0].
+	 */
+	struct smiapp_sensor *sensor =
+		container_of(ssd, struct smiapp_sensor, ssds[0]);
 	unsigned int sleep;
 	int rval;
 
@@ -1326,6 +1342,7 @@ static int smiapp_power_on(struct smiapp_sensor *sensor)
 	return 0;
 
 out_cci_addr_fail:
+
 	gpiod_set_value(sensor->xshutdown, 0);
 	clk_disable_unprepare(sensor->ext_clk);
 
@@ -1334,8 +1351,14 @@ out_xclk_fail:
 	return rval;
 }
 
-static void smiapp_power_off(struct smiapp_sensor *sensor)
+static int smiapp_power_off(struct device *dev)
 {
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
+	struct smiapp_subdev *ssd = to_smiapp_subdev(subdev);
+	struct smiapp_sensor *sensor =
+		container_of(ssd, struct smiapp_sensor, ssds[0]);
+
 	/*
 	 * Currently power/clock to lens are enable/disabled separately
 	 * but they are essentially the same signals. So if the sensor is
@@ -1353,31 +1376,8 @@ static void smiapp_power_off(struct smiapp_sensor *sensor)
 	usleep_range(5000, 5000);
 	regulator_disable(sensor->vana);
 	sensor->streaming = false;
-}
-
-static int smiapp_set_power(struct v4l2_subdev *subdev, int on)
-{
-	struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
-	int ret = 0;
-
-	mutex_lock(&sensor->power_mutex);
-
-	if (on && !sensor->power_count) {
-		/* Power on and perform initialisation. */
-		ret = smiapp_power_on(sensor);
-		if (ret < 0)
-			goto out;
-	} else if (!on && sensor->power_count == 1) {
-		smiapp_power_off(sensor);
-	}
-
-	/* Update the power count. */
-	sensor->power_count += on ? 1 : -1;
-	WARN_ON(sensor->power_count < 0);
 
-out:
-	mutex_unlock(&sensor->power_mutex);
-	return ret;
+	return 0;
 }
 
 /* -----------------------------------------------------------------------------
@@ -1535,6 +1535,7 @@ out:
 
 static int smiapp_set_stream(struct v4l2_subdev *subdev, int enable)
 {
+	struct i2c_client *client = v4l2_get_subdevdata(subdev);
 	struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
 	int rval;
 
@@ -1542,13 +1543,23 @@ static int smiapp_set_stream(struct v4l2_subdev *subdev, int enable)
 		return 0;
 
 	if (enable) {
+		rval = pm_runtime_get_sync(&client->dev);
+		if (rval < 0) {
+			pm_runtime_put(&client->dev);
+			return rval;
+		}
+
 		sensor->streaming = true;
 		rval = smiapp_start_streaming(sensor);
-		if (rval < 0)
+		if (rval < 0) {
+			pm_runtime_put(&client->dev);
 			sensor->streaming = false;
+		}
 	} else {
 		rval = smiapp_stop_streaming(sensor);
 		sensor->streaming = false;
+
+		pm_runtime_put(&client->dev);
 	}
 
 	return rval;
@@ -2308,13 +2319,15 @@ smiapp_sysfs_nvm_read(struct device *dev, struct device_attribute *attr,
 	if (!sensor->nvm_size) {
 		/* NVM not read yet - read it now */
 		sensor->nvm_size = sensor->hwcfg->nvm_size;
-		if (smiapp_set_power(subdev, 1) < 0)
+		if (pm_runtime_get_sync(&client->dev) < 0) {
+			pm_runtime_put(&client->dev);
 			return -ENODEV;
+		}
 		if (smiapp_read_nvm(sensor, sensor->nvm)) {
 			dev_err(&client->dev, "nvm read failed\n");
 			return -ENODEV;
 		}
-		smiapp_set_power(subdev, 0);
+		pm_runtime_put(&client->dev);
 	}
 	/*
 	 * NVM is still way below a PAGE_SIZE, so we can safely
@@ -2624,22 +2637,13 @@ static int smiapp_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 
 	mutex_unlock(&sensor->mutex);
 
-	return smiapp_set_power(sd, 1);
-}
-
-static int smiapp_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
-{
-	return smiapp_set_power(sd, 0);
+	return 0;
 }
 
 static const struct v4l2_subdev_video_ops smiapp_video_ops = {
 	.s_stream = smiapp_set_stream,
 };
 
-static const struct v4l2_subdev_core_ops smiapp_core_ops = {
-	.s_power = smiapp_set_power,
-};
-
 static const struct v4l2_subdev_pad_ops smiapp_pad_ops = {
 	.enum_mbus_code = smiapp_enum_mbus_code,
 	.get_fmt = smiapp_get_format,
@@ -2654,7 +2658,6 @@ static const struct v4l2_subdev_sensor_ops smiapp_sensor_ops = {
 };
 
 static const struct v4l2_subdev_ops smiapp_ops = {
-	.core = &smiapp_core_ops,
 	.video = &smiapp_video_ops,
 	.pad = &smiapp_pad_ops,
 	.sensor = &smiapp_sensor_ops,
@@ -2667,12 +2670,10 @@ static const struct media_entity_operations smiapp_entity_ops = {
 static const struct v4l2_subdev_internal_ops smiapp_internal_src_ops = {
 	.registered = smiapp_registered,
 	.open = smiapp_open,
-	.close = smiapp_close,
 };
 
 static const struct v4l2_subdev_internal_ops smiapp_internal_ops = {
 	.open = smiapp_open,
-	.close = smiapp_close,
 };
 
 /* -----------------------------------------------------------------------------
@@ -2686,18 +2687,18 @@ static int smiapp_suspend(struct device *dev)
 	struct i2c_client *client = to_i2c_client(dev);
 	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
 	struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
-	bool streaming;
+	bool streaming = sensor->streaming;
+	int rval;
 
-	if (sensor->power_count == 0)
-		return 0;
+	rval = pm_runtime_get_sync(dev);
+	if (rval < 0) {
+		pm_runtime_put(dev);
+		return -EAGAIN;
+	}
 
 	if (sensor->streaming)
 		smiapp_stop_streaming(sensor);
 
-	streaming = sensor->streaming;
-
-	smiapp_power_off(sensor);
-
 	/* save state for resume */
 	sensor->streaming = streaming;
 
@@ -2709,14 +2710,9 @@ static int smiapp_resume(struct device *dev)
 	struct i2c_client *client = to_i2c_client(dev);
 	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
 	struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
-	int rval;
-
-	if (sensor->power_count == 0)
-		return 0;
+	int rval = 0;
 
-	rval = smiapp_power_on(sensor);
-	if (rval)
-		return rval;
+	pm_runtime_put(dev);
 
 	if (sensor->streaming)
 		rval = smiapp_start_streaming(sensor);
@@ -2823,7 +2819,6 @@ static int smiapp_probe(struct i2c_client *client,
 
 	sensor->hwcfg = hwcfg;
 	mutex_init(&sensor->mutex);
-	mutex_init(&sensor->power_mutex);
 	sensor->src = &sensor->ssds[sensor->ssds_used];
 
 	v4l2_i2c_subdev_init(&sensor->src->sd, client, &smiapp_ops);
@@ -2855,9 +2850,13 @@ static int smiapp_probe(struct i2c_client *client,
 	if (IS_ERR(sensor->xshutdown))
 		return PTR_ERR(sensor->xshutdown);
 
-	rval = smiapp_power_on(sensor);
-	if (rval)
-		return -ENODEV;
+	pm_runtime_enable(&client->dev);
+
+	rval = pm_runtime_get_sync(&client->dev);
+	if (rval < 0) {
+		rval = -ENODEV;
+		goto out_power_off;
+	}
 
 	rval = smiapp_identify_module(sensor);
 	if (rval) {
@@ -3030,8 +3029,6 @@ static int smiapp_probe(struct i2c_client *client,
 	sensor->streaming = false;
 	sensor->dev_init_done = true;
 
-	smiapp_power_off(sensor);
-
 	rval = media_entity_pads_init(&sensor->src->sd.entity, 2,
 				 sensor->src->pads);
 	if (rval < 0)
@@ -3041,6 +3038,8 @@ static int smiapp_probe(struct i2c_client *client,
 	if (rval < 0)
 		goto out_media_entity_cleanup;
 
+	pm_runtime_put(&client->dev);
+
 	return 0;
 
 out_media_entity_cleanup:
@@ -3050,7 +3049,9 @@ out_cleanup:
 	smiapp_cleanup(sensor);
 
 out_power_off:
-	smiapp_power_off(sensor);
+	pm_runtime_put(&client->dev);
+	pm_runtime_disable(&client->dev);
+
 	return rval;
 }
 
@@ -3062,11 +3063,7 @@ static int smiapp_remove(struct i2c_client *client)
 
 	v4l2_async_unregister_subdev(subdev);
 
-	if (sensor->power_count) {
-		gpiod_set_value(sensor->xshutdown, 0);
-		clk_disable_unprepare(sensor->ext_clk);
-		sensor->power_count = 0;
-	}
+	smiapp_set_stream(subdev, 0);
 
 	for (i = 0; i < sensor->ssds_used; i++) {
 		v4l2_device_unregister_subdev(&sensor->ssds[i].sd);
@@ -3074,6 +3071,9 @@ static int smiapp_remove(struct i2c_client *client)
 	}
 	smiapp_cleanup(sensor);
 
+	pm_runtime_suspend(&client->dev);
+	pm_runtime_disable(&client->dev);
+
 	return 0;
 }
 
@@ -3091,6 +3091,7 @@ MODULE_DEVICE_TABLE(i2c, smiapp_id_table);
 
 static const struct dev_pm_ops smiapp_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(smiapp_suspend, smiapp_resume)
+	SET_RUNTIME_PM_OPS(smiapp_power_off, smiapp_power_on, NULL)
 };
 
 static struct i2c_driver smiapp_i2c_driver = {
diff --git a/drivers/media/i2c/smiapp/smiapp.h b/drivers/media/i2c/smiapp/smiapp.h
index d7b52a6..f74d695 100644
--- a/drivers/media/i2c/smiapp/smiapp.h
+++ b/drivers/media/i2c/smiapp/smiapp.h
@@ -176,16 +176,9 @@ struct smiapp_sensor {
 	 * "mutex" is used to serialise access to all fields here
 	 * except v4l2_ctrls at the end of the struct. "mutex" is also
 	 * used to serialise access to file handle specific
-	 * information. The exception to this rule is the power_mutex
-	 * below.
+	 * information.
 	 */
 	struct mutex mutex;
-	/*
-	 * power_mutex is used to serialise power management related
-	 * activities. Acquiring "mutex" at that time isn't necessary
-	 * since there are no other users anyway.
-	 */
-	struct mutex power_mutex;
 	struct smiapp_subdev ssds[SMIAPP_SUBDEVS];
 	u32 ssds_used;
 	struct smiapp_subdev *src;
@@ -218,8 +211,6 @@ struct smiapp_sensor {
 	u16 image_start; /* image data start line */
 	u16 visible_pixel_start; /* start pixel of the visible image */
 
-	int power_count;
-
 	bool streaming;
 	bool dev_init_done;
 	u8 compressed_min_bpp;
-- 
2.1.4

[PATCH v1.1 4/5] smiapp: Use runtime PM

[Sakari Ailus]

Use runtime PM to manage power. The s_power() core sub-device callback is
removed as it is no longer needed.

The power management of the sensor is changed so that it is no longer
dependent on open file descriptors on sub-device or use_count in the media
entity but solely will be powered on as needed for probing and streaming.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
since v1:

- Both smiapp_set_ctrl() and smiapp_update_mode() perform work which is
  unrelated to the power state of the device. Instead, check the power
  state in smiapp_write() which is more appropriate.

- Don't explicitly disable streaming in smiapp_remove(). It'd be an
  unrelated change.

 drivers/media/i2c/smiapp/smiapp-core.c | 130 ++++++++++++++++-----------------
 drivers/media/i2c/smiapp/smiapp-regs.c |   5 ++
 drivers/media/i2c/smiapp/smiapp.h      |  11 +--
 3 files changed, 67 insertions(+), 79 deletions(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index e1d1459..d7f3738 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -26,6 +26,7 @@
 #include <linux/gpio.h>
 #include <linux/gpio/consumer.h>
 #include <linux/module.h>
+#include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <linux/smiapp.h>
@@ -1198,9 +1199,17 @@ out:
  * Power management
  */
 
-static int smiapp_power_on(struct smiapp_sensor *sensor)
+static int smiapp_power_on(struct device *dev)
 {
-	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
+	struct smiapp_subdev *ssd = to_smiapp_subdev(subdev);
+	/*
+	 * The sub-device related to the I2C device is always the
+	 * source one, i.e. ssds[0].
+	 */
+	struct smiapp_sensor *sensor =
+		container_of(ssd, struct smiapp_sensor, ssds[0]);
 	unsigned int sleep;
 	int rval;
 
@@ -1326,6 +1335,7 @@ static int smiapp_power_on(struct smiapp_sensor *sensor)
 	return 0;
 
 out_cci_addr_fail:
+
 	gpiod_set_value(sensor->xshutdown, 0);
 	clk_disable_unprepare(sensor->ext_clk);
 
@@ -1334,8 +1344,14 @@ out_xclk_fail:
 	return rval;
 }
 
-static void smiapp_power_off(struct smiapp_sensor *sensor)
+static int smiapp_power_off(struct device *dev)
 {
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
+	struct smiapp_subdev *ssd = to_smiapp_subdev(subdev);
+	struct smiapp_sensor *sensor =
+		container_of(ssd, struct smiapp_sensor, ssds[0]);
+
 	/*
 	 * Currently power/clock to lens are enable/disabled separately
 	 * but they are essentially the same signals. So if the sensor is
@@ -1353,31 +1369,8 @@ static void smiapp_power_off(struct smiapp_sensor *sensor)
 	usleep_range(5000, 5000);
 	regulator_disable(sensor->vana);
 	sensor->streaming = false;
-}
-
-static int smiapp_set_power(struct v4l2_subdev *subdev, int on)
-{
-	struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
-	int ret = 0;
-
-	mutex_lock(&sensor->power_mutex);
-
-	if (on && !sensor->power_count) {
-		/* Power on and perform initialisation. */
-		ret = smiapp_power_on(sensor);
-		if (ret < 0)
-			goto out;
-	} else if (!on && sensor->power_count == 1) {
-		smiapp_power_off(sensor);
-	}
-
-	/* Update the power count. */
-	sensor->power_count += on ? 1 : -1;
-	WARN_ON(sensor->power_count < 0);
 
-out:
-	mutex_unlock(&sensor->power_mutex);
-	return ret;
+	return 0;
 }
 
 /* -----------------------------------------------------------------------------
@@ -1535,6 +1528,7 @@ out:
 
 static int smiapp_set_stream(struct v4l2_subdev *subdev, int enable)
 {
+	struct i2c_client *client = v4l2_get_subdevdata(subdev);
 	struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
 	int rval;
 
@@ -1542,13 +1536,23 @@ static int smiapp_set_stream(struct v4l2_subdev *subdev, int enable)
 		return 0;
 
 	if (enable) {
+		rval = pm_runtime_get_sync(&client->dev);
+		if (rval < 0) {
+			pm_runtime_put(&client->dev);
+			return rval;
+		}
+
 		sensor->streaming = true;
 		rval = smiapp_start_streaming(sensor);
-		if (rval < 0)
+		if (rval < 0) {
+			pm_runtime_put(&client->dev);
 			sensor->streaming = false;
+		}
 	} else {
 		rval = smiapp_stop_streaming(sensor);
 		sensor->streaming = false;
+
+		pm_runtime_put(&client->dev);
 	}
 
 	return rval;
@@ -2308,13 +2312,15 @@ smiapp_sysfs_nvm_read(struct device *dev, struct device_attribute *attr,
 	if (!sensor->nvm_size) {
 		/* NVM not read yet - read it now */
 		sensor->nvm_size = sensor->hwcfg->nvm_size;
-		if (smiapp_set_power(subdev, 1) < 0)
+		if (pm_runtime_get_sync(&client->dev) < 0) {
+			pm_runtime_put(&client->dev);
 			return -ENODEV;
+		}
 		if (smiapp_read_nvm(sensor, sensor->nvm)) {
 			dev_err(&client->dev, "nvm read failed\n");
 			return -ENODEV;
 		}
-		smiapp_set_power(subdev, 0);
+		pm_runtime_put(&client->dev);
 	}
 	/*
 	 * NVM is still way below a PAGE_SIZE, so we can safely
@@ -2624,22 +2630,13 @@ static int smiapp_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 
 	mutex_unlock(&sensor->mutex);
 
-	return smiapp_set_power(sd, 1);
-}
-
-static int smiapp_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
-{
-	return smiapp_set_power(sd, 0);
+	return 0;
 }
 
 static const struct v4l2_subdev_video_ops smiapp_video_ops = {
 	.s_stream = smiapp_set_stream,
 };
 
-static const struct v4l2_subdev_core_ops smiapp_core_ops = {
-	.s_power = smiapp_set_power,
-};
-
 static const struct v4l2_subdev_pad_ops smiapp_pad_ops = {
 	.enum_mbus_code = smiapp_enum_mbus_code,
 	.get_fmt = smiapp_get_format,
@@ -2654,7 +2651,6 @@ static const struct v4l2_subdev_sensor_ops smiapp_sensor_ops = {
 };
 
 static const struct v4l2_subdev_ops smiapp_ops = {
-	.core = &smiapp_core_ops,
 	.video = &smiapp_video_ops,
 	.pad = &smiapp_pad_ops,
 	.sensor = &smiapp_sensor_ops,
@@ -2667,12 +2663,10 @@ static const struct media_entity_operations smiapp_entity_ops = {
 static const struct v4l2_subdev_internal_ops smiapp_internal_src_ops = {
 	.registered = smiapp_registered,
 	.open = smiapp_open,
-	.close = smiapp_close,
 };
 
 static const struct v4l2_subdev_internal_ops smiapp_internal_ops = {
 	.open = smiapp_open,
-	.close = smiapp_close,
 };
 
 /* -----------------------------------------------------------------------------
@@ -2686,18 +2680,18 @@ static int smiapp_suspend(struct device *dev)
 	struct i2c_client *client = to_i2c_client(dev);
 	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
 	struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
-	bool streaming;
+	bool streaming = sensor->streaming;
+	int rval;
 
-	if (sensor->power_count == 0)
-		return 0;
+	rval = pm_runtime_get_sync(dev);
+	if (rval < 0) {
+		pm_runtime_put(dev);
+		return -EAGAIN;
+	}
 
 	if (sensor->streaming)
 		smiapp_stop_streaming(sensor);
 
-	streaming = sensor->streaming;
-
-	smiapp_power_off(sensor);
-
 	/* save state for resume */
 	sensor->streaming = streaming;
 
@@ -2709,14 +2703,9 @@ static int smiapp_resume(struct device *dev)
 	struct i2c_client *client = to_i2c_client(dev);
 	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
 	struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
-	int rval;
-
-	if (sensor->power_count == 0)
-		return 0;
+	int rval = 0;
 
-	rval = smiapp_power_on(sensor);
-	if (rval)
-		return rval;
+	pm_runtime_put(dev);
 
 	if (sensor->streaming)
 		rval = smiapp_start_streaming(sensor);
@@ -2823,7 +2812,6 @@ static int smiapp_probe(struct i2c_client *client,
 
 	sensor->hwcfg = hwcfg;
 	mutex_init(&sensor->mutex);
-	mutex_init(&sensor->power_mutex);
 	sensor->src = &sensor->ssds[sensor->ssds_used];
 
 	v4l2_i2c_subdev_init(&sensor->src->sd, client, &smiapp_ops);
@@ -2855,9 +2843,13 @@ static int smiapp_probe(struct i2c_client *client,
 	if (IS_ERR(sensor->xshutdown))
 		return PTR_ERR(sensor->xshutdown);
 
-	rval = smiapp_power_on(sensor);
-	if (rval)
-		return -ENODEV;
+	pm_runtime_enable(&client->dev);
+
+	rval = pm_runtime_get_sync(&client->dev);
+	if (rval < 0) {
+		rval = -ENODEV;
+		goto out_power_off;
+	}
 
 	rval = smiapp_identify_module(sensor);
 	if (rval) {
@@ -3030,8 +3022,6 @@ static int smiapp_probe(struct i2c_client *client,
 	sensor->streaming = false;
 	sensor->dev_init_done = true;
 
-	smiapp_power_off(sensor);
-
 	rval = media_entity_pads_init(&sensor->src->sd.entity, 2,
 				 sensor->src->pads);
 	if (rval < 0)
@@ -3041,6 +3031,8 @@ static int smiapp_probe(struct i2c_client *client,
 	if (rval < 0)
 		goto out_media_entity_cleanup;
 
+	pm_runtime_put(&client->dev);
+
 	return 0;
 
 out_media_entity_cleanup:
@@ -3050,7 +3042,9 @@ out_cleanup:
 	smiapp_cleanup(sensor);
 
 out_power_off:
-	smiapp_power_off(sensor);
+	pm_runtime_put(&client->dev);
+	pm_runtime_disable(&client->dev);
+
 	return rval;
 }
 
@@ -3062,11 +3056,8 @@ static int smiapp_remove(struct i2c_client *client)
 
 	v4l2_async_unregister_subdev(subdev);
 
-	if (sensor->power_count) {
-		gpiod_set_value(sensor->xshutdown, 0);
-		clk_disable_unprepare(sensor->ext_clk);
-		sensor->power_count = 0;
-	}
+	pm_runtime_suspend(&client->dev);
+	pm_runtime_disable(&client->dev);
 
 	for (i = 0; i < sensor->ssds_used; i++) {
 		v4l2_device_unregister_subdev(&sensor->ssds[i].sd);
@@ -3091,6 +3082,7 @@ MODULE_DEVICE_TABLE(i2c, smiapp_id_table);
 
 static const struct dev_pm_ops smiapp_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(smiapp_suspend, smiapp_resume)
+	SET_RUNTIME_PM_OPS(smiapp_power_off, smiapp_power_on, NULL)
 };
 
 static struct i2c_driver smiapp_i2c_driver = {
diff --git a/drivers/media/i2c/smiapp/smiapp-regs.c b/drivers/media/i2c/smiapp/smiapp-regs.c
index 1e501c0..a9c7baf 100644
--- a/drivers/media/i2c/smiapp/smiapp-regs.c
+++ b/drivers/media/i2c/smiapp/smiapp-regs.c
@@ -18,6 +18,7 @@
 
 #include <linux/delay.h>
 #include <linux/i2c.h>
+#include <linux/pm_runtime.h>
 
 #include "smiapp.h"
 #include "smiapp-regs.h"
@@ -288,8 +289,12 @@ int smiapp_write_no_quirk(struct smiapp_sensor *sensor, u32 reg, u32 val)
  */
 int smiapp_write(struct smiapp_sensor *sensor, u32 reg, u32 val)
 {
+	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
 	int rval;
 
+	if (pm_runtime_suspended(&client->dev))
+		return 0;
+
 	rval = smiapp_call_quirk(sensor, reg_access, true, &reg, &val);
 	if (rval == -ENOIOCTLCMD)
 		return 0;
diff --git a/drivers/media/i2c/smiapp/smiapp.h b/drivers/media/i2c/smiapp/smiapp.h
index d7b52a6..f74d695 100644
--- a/drivers/media/i2c/smiapp/smiapp.h
+++ b/drivers/media/i2c/smiapp/smiapp.h
@@ -176,16 +176,9 @@ struct smiapp_sensor {
 	 * "mutex" is used to serialise access to all fields here
 	 * except v4l2_ctrls at the end of the struct. "mutex" is also
 	 * used to serialise access to file handle specific
-	 * information. The exception to this rule is the power_mutex
-	 * below.
+	 * information.
 	 */
 	struct mutex mutex;
-	/*
-	 * power_mutex is used to serialise power management related
-	 * activities. Acquiring "mutex" at that time isn't necessary
-	 * since there are no other users anyway.
-	 */
-	struct mutex power_mutex;
 	struct smiapp_subdev ssds[SMIAPP_SUBDEVS];
 	u32 ssds_used;
 	struct smiapp_subdev *src;
@@ -218,8 +211,6 @@ struct smiapp_sensor {
 	u16 image_start; /* image data start line */
 	u16 visible_pixel_start; /* start pixel of the visible image */
 
-	int power_count;
-
 	bool streaming;
 	bool dev_init_done;
 	u8 compressed_min_bpp;
-- 
2.1.4

Re: [PATCH v1.1 4/5] smiapp: Use runtime PM

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1353 bytes --]

Hi,

On Fri, Sep 16, 2016 at 01:53:29AM +0300, Sakari Ailus wrote:
> [...]
>
> diff --git a/drivers/media/i2c/smiapp/smiapp-regs.c b/drivers/media/i2c/smiapp/smiapp-regs.c
> index 1e501c0..a9c7baf 100644
> --- a/drivers/media/i2c/smiapp/smiapp-regs.c
> +++ b/drivers/media/i2c/smiapp/smiapp-regs.c
> @@ -18,6 +18,7 @@
>  
>  #include <linux/delay.h>
>  #include <linux/i2c.h>
> +#include <linux/pm_runtime.h>
>  
>  #include "smiapp.h"
>  #include "smiapp-regs.h"
> @@ -288,8 +289,12 @@ int smiapp_write_no_quirk(struct smiapp_sensor *sensor, u32 reg, u32 val)
>   */
>  int smiapp_write(struct smiapp_sensor *sensor, u32 reg, u32 val)
>  {
> +	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
>  	int rval;
>  
> +	if (pm_runtime_suspended(&client->dev))
> +		return 0;
> +

This looks racy. What if idle countdown runs out immediately after
this check? If you can't call get_sync in this function you can
call pm_runtime_get() before the suspend check and pm_runtime_put
before returning from the function, so that the device keeps being
enabled.

Also I would expect some error code instead of success for early
return due to device being suspended?

>  	rval = smiapp_call_quirk(sensor, reg_access, true, &reg, &val);
>  	if (rval == -ENOIOCTLCMD)
>  		return 0;
>
> [...]

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v1.1 4/5] smiapp: Use runtime PM

[Sakari Ailus]

[-- Attachment #1.1: Type: text/plain, Size: 1726 bytes --]

Hi Sebastian,

Thank you for the review.

On 09/20/16 01:51, Sebastian Reichel wrote:
> Hi,
> 
> On Fri, Sep 16, 2016 at 01:53:29AM +0300, Sakari Ailus wrote:
>> [...]
>>
>> diff --git a/drivers/media/i2c/smiapp/smiapp-regs.c b/drivers/media/i2c/smiapp/smiapp-regs.c
>> index 1e501c0..a9c7baf 100644
>> --- a/drivers/media/i2c/smiapp/smiapp-regs.c
>> +++ b/drivers/media/i2c/smiapp/smiapp-regs.c
>> @@ -18,6 +18,7 @@
>>  
>>  #include <linux/delay.h>
>>  #include <linux/i2c.h>
>> +#include <linux/pm_runtime.h>
>>  
>>  #include "smiapp.h"
>>  #include "smiapp-regs.h"
>> @@ -288,8 +289,12 @@ int smiapp_write_no_quirk(struct smiapp_sensor *sensor, u32 reg, u32 val)
>>   */
>>  int smiapp_write(struct smiapp_sensor *sensor, u32 reg, u32 val)
>>  {
>> +	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
>>  	int rval;
>>  
>> +	if (pm_runtime_suspended(&client->dev))
>> +		return 0;
>> +
> 
> This looks racy. What if idle countdown runs out immediately after
> this check? If you can't call get_sync in this function you can
> call pm_runtime_get() before the suspend check and pm_runtime_put
> before returning from the function, so that the device keeps being
> enabled.

Good point. It was probably late when I wrote the patch. X-)

I guess I need to put pm_runtime_get_noresume() before that, and then
put_autosuspend() it later on.

> 
> Also I would expect some error code instead of success for early
> return due to device being suspended?

That's by design.

If the sensor is off, there's no need to write anything there. The
configuration is re-applied to the sensor when it's powered on.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 213 bytes --]

Re: [PATCH v1.1 4/5] smiapp: Use runtime PM

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1124 bytes --]

Hi,

On Fri, Sep 16, 2016 at 01:53:29AM +0300, Sakari Ailus wrote:
> Use runtime PM to manage power. The s_power() core sub-device callback is
> removed as it is no longer needed.
> 
> The power management of the sensor is changed so that it is no longer
> dependent on open file descriptors on sub-device or use_count in the media
> entity but solely will be powered on as needed for probing and streaming.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> since v1:
> 
> - Both smiapp_set_ctrl() and smiapp_update_mode() perform work which is
>   unrelated to the power state of the device. Instead, check the power
>   state in smiapp_write() which is more appropriate.
> 
> - Don't explicitly disable streaming in smiapp_remove(). It'd be an
>   unrelated change.
> 
>  drivers/media/i2c/smiapp/smiapp-core.c | 130 ++++++++++++++++-----------------
>  drivers/media/i2c/smiapp/smiapp-regs.c |   5 ++
>  drivers/media/i2c/smiapp/smiapp.h      |  11 +--
>  3 files changed, 67 insertions(+), 79 deletions(-)

Reviewed-By: Sebastian Reichel <sre@kernel.org>

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[PATCH v1.2 4/5] smiapp: Use runtime PM

[Sakari Ailus]

Switch to runtime PM in sensor power management. The internal power count
is thus removed.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
since v1.1:

- Keep the behaviour the same, i.e. the sensor is powered on through open
  file descriptors or s_power() callback.

 drivers/media/i2c/smiapp/smiapp-core.c | 131 +++++++++++++++++++++------------
 drivers/media/i2c/smiapp/smiapp.h      |  11 +--
 2 files changed, 83 insertions(+), 59 deletions(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 88ad4b9..68adc1b 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -26,6 +26,7 @@
 #include <linux/gpio.h>
 #include <linux/gpio/consumer.h>
 #include <linux/module.h>
+#include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <linux/smiapp.h>
@@ -1202,9 +1203,17 @@ out:
  * Power management
  */
 
-static int smiapp_power_on(struct smiapp_sensor *sensor)
+static int smiapp_power_on(struct device *dev)
 {
-	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
+	struct smiapp_subdev *ssd = to_smiapp_subdev(subdev);
+	/*
+	 * The sub-device related to the I2C device is always the
+	 * source one, i.e. ssds[0].
+	 */
+	struct smiapp_sensor *sensor =
+		container_of(ssd, struct smiapp_sensor, ssds[0]);
 	unsigned int sleep;
 	int rval;
 
@@ -1330,16 +1339,24 @@ static int smiapp_power_on(struct smiapp_sensor *sensor)
 	return 0;
 
 out_cci_addr_fail:
+
 	gpiod_set_value(sensor->xshutdown, 0);
 	clk_disable_unprepare(sensor->ext_clk);
 
 out_xclk_fail:
 	regulator_disable(sensor->vana);
+
 	return rval;
 }
 
-static void smiapp_power_off(struct smiapp_sensor *sensor)
+static int smiapp_power_off(struct device *dev)
 {
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
+	struct smiapp_subdev *ssd = to_smiapp_subdev(subdev);
+	struct smiapp_sensor *sensor =
+		container_of(ssd, struct smiapp_sensor, ssds[0]);
+
 	/*
 	 * Currently power/clock to lens are enable/disabled separately
 	 * but they are essentially the same signals. So if the sensor is
@@ -1357,31 +1374,26 @@ static void smiapp_power_off(struct smiapp_sensor *sensor)
 	usleep_range(5000, 5000);
 	regulator_disable(sensor->vana);
 	sensor->streaming = false;
+
+	return 0;
 }
 
 static int smiapp_set_power(struct v4l2_subdev *subdev, int on)
 {
-	struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
-	int ret = 0;
+	int rval = 0;
 
-	mutex_lock(&sensor->power_mutex);
+	if (on) {
+		rval = pm_runtime_get_sync(subdev->dev);
+		if (rval >= 0)
+			return 0;
 
-	if (on && !sensor->power_count) {
-		/* Power on and perform initialisation. */
-		ret = smiapp_power_on(sensor);
-		if (ret < 0)
-			goto out;
-	} else if (!on && sensor->power_count == 1) {
-		smiapp_power_off(sensor);
+		if (rval != -EBUSY && rval != -EAGAIN)
+			pm_runtime_set_active(subdev->dev);
 	}
 
-	/* Update the power count. */
-	sensor->power_count += on ? 1 : -1;
-	WARN_ON(sensor->power_count < 0);
+	pm_runtime_put(subdev->dev);
 
-out:
-	mutex_unlock(&sensor->power_mutex);
-	return ret;
+	return rval;
 }
 
 /* -----------------------------------------------------------------------------
@@ -2310,15 +2322,25 @@ smiapp_sysfs_nvm_read(struct device *dev, struct device_attribute *attr,
 		return -EBUSY;
 
 	if (!sensor->nvm_size) {
+		int rval;
+
 		/* NVM not read yet - read it now */
 		sensor->nvm_size = sensor->hwcfg->nvm_size;
-		if (smiapp_set_power(subdev, 1) < 0)
+
+		rval = pm_runtime_get_sync(&client->dev);
+		if (rval < 0) {
+			if (rval != -EBUSY && rval != -EAGAIN)
+				pm_runtime_set_active(&client->dev);
+			pm_runtime_put(&client->dev);
 			return -ENODEV;
+		}
+
 		if (smiapp_read_nvm(sensor, sensor->nvm)) {
 			dev_err(&client->dev, "nvm read failed\n");
 			return -ENODEV;
 		}
-		smiapp_set_power(subdev, 0);
+
+		pm_runtime_put(&client->dev);
 	}
 	/*
 	 * NVM is still way below a PAGE_SIZE, so we can safely
@@ -2619,6 +2641,7 @@ static int smiapp_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 	struct smiapp_subdev *ssd = to_smiapp_subdev(sd);
 	struct smiapp_sensor *sensor = ssd->sensor;
 	unsigned int i;
+	int rval;
 
 	mutex_lock(&sensor->mutex);
 
@@ -2645,12 +2668,22 @@ static int smiapp_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 
 	mutex_unlock(&sensor->mutex);
 
-	return smiapp_set_power(sd, 1);
+	rval = pm_runtime_get_sync(sd->dev);
+	if (rval >= 0)
+		return 0;
+
+	if (rval != -EBUSY && rval != -EAGAIN)
+		pm_runtime_set_active(sd->dev);
+	pm_runtime_put(sd->dev);
+
+	return rval;
 }
 
 static int smiapp_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 {
-	return smiapp_set_power(sd, 0);
+	pm_runtime_put(sd->dev);
+
+	return 0;
 }
 
 static const struct v4l2_subdev_video_ops smiapp_video_ops = {
@@ -2708,18 +2741,20 @@ static int smiapp_suspend(struct device *dev)
 	struct i2c_client *client = to_i2c_client(dev);
 	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
 	struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
-	bool streaming;
+	bool streaming = sensor->streaming;
+	int rval;
 
-	if (sensor->power_count == 0)
-		return 0;
+	rval = pm_runtime_get_sync(dev);
+	if (rval < 0) {
+		if (rval != -EBUSY && rval != -EAGAIN)
+			pm_runtime_set_active(&client->dev);
+		pm_runtime_put(dev);
+		return -EAGAIN;
+	}
 
 	if (sensor->streaming)
 		smiapp_stop_streaming(sensor);
 
-	streaming = sensor->streaming;
-
-	smiapp_power_off(sensor);
-
 	/* save state for resume */
 	sensor->streaming = streaming;
 
@@ -2731,14 +2766,9 @@ static int smiapp_resume(struct device *dev)
 	struct i2c_client *client = to_i2c_client(dev);
 	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
 	struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
-	int rval;
-
-	if (sensor->power_count == 0)
-		return 0;
+	int rval = 0;
 
-	rval = smiapp_power_on(sensor);
-	if (rval)
-		return rval;
+	pm_runtime_put(dev);
 
 	if (sensor->streaming)
 		rval = smiapp_start_streaming(sensor);
@@ -2845,7 +2875,6 @@ static int smiapp_probe(struct i2c_client *client,
 
 	sensor->hwcfg = hwcfg;
 	mutex_init(&sensor->mutex);
-	mutex_init(&sensor->power_mutex);
 	sensor->src = &sensor->ssds[sensor->ssds_used];
 
 	v4l2_i2c_subdev_init(&sensor->src->sd, client, &smiapp_ops);
@@ -2877,9 +2906,13 @@ static int smiapp_probe(struct i2c_client *client,
 	if (IS_ERR(sensor->xshutdown))
 		return PTR_ERR(sensor->xshutdown);
 
-	rval = smiapp_power_on(sensor);
-	if (rval)
-		return -ENODEV;
+	pm_runtime_enable(&client->dev);
+
+	rval = pm_runtime_get_sync(&client->dev);
+	if (rval < 0) {
+		rval = -ENODEV;
+		goto out_power_off;
+	}
 
 	rval = smiapp_identify_module(sensor);
 	if (rval) {
@@ -3051,8 +3084,6 @@ static int smiapp_probe(struct i2c_client *client,
 	sensor->streaming = false;
 	sensor->dev_init_done = true;
 
-	smiapp_power_off(sensor);
-
 	rval = media_entity_pads_init(&sensor->src->sd.entity, 2,
 				 sensor->src->pads);
 	if (rval < 0)
@@ -3062,6 +3093,8 @@ static int smiapp_probe(struct i2c_client *client,
 	if (rval < 0)
 		goto out_media_entity_cleanup;
 
+	pm_runtime_put(&client->dev);
+
 	return 0;
 
 out_media_entity_cleanup:
@@ -3071,7 +3104,9 @@ out_cleanup:
 	smiapp_cleanup(sensor);
 
 out_power_off:
-	smiapp_power_off(sensor);
+	pm_runtime_put(&client->dev);
+	pm_runtime_disable(&client->dev);
+
 	return rval;
 }
 
@@ -3083,11 +3118,8 @@ static int smiapp_remove(struct i2c_client *client)
 
 	v4l2_async_unregister_subdev(subdev);
 
-	if (sensor->power_count) {
-		gpiod_set_value(sensor->xshutdown, 0);
-		clk_disable_unprepare(sensor->ext_clk);
-		sensor->power_count = 0;
-	}
+	pm_runtime_suspend(&client->dev);
+	pm_runtime_disable(&client->dev);
 
 	for (i = 0; i < sensor->ssds_used; i++) {
 		v4l2_device_unregister_subdev(&sensor->ssds[i].sd);
@@ -3112,6 +3144,7 @@ MODULE_DEVICE_TABLE(i2c, smiapp_id_table);
 
 static const struct dev_pm_ops smiapp_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(smiapp_suspend, smiapp_resume)
+	SET_RUNTIME_PM_OPS(smiapp_power_off, smiapp_power_on, NULL)
 };
 
 static struct i2c_driver smiapp_i2c_driver = {
diff --git a/drivers/media/i2c/smiapp/smiapp.h b/drivers/media/i2c/smiapp/smiapp.h
index d7b52a6..f74d695 100644
--- a/drivers/media/i2c/smiapp/smiapp.h
+++ b/drivers/media/i2c/smiapp/smiapp.h
@@ -176,16 +176,9 @@ struct smiapp_sensor {
 	 * "mutex" is used to serialise access to all fields here
 	 * except v4l2_ctrls at the end of the struct. "mutex" is also
 	 * used to serialise access to file handle specific
-	 * information. The exception to this rule is the power_mutex
-	 * below.
+	 * information.
 	 */
 	struct mutex mutex;
-	/*
-	 * power_mutex is used to serialise power management related
-	 * activities. Acquiring "mutex" at that time isn't necessary
-	 * since there are no other users anyway.
-	 */
-	struct mutex power_mutex;
 	struct smiapp_subdev ssds[SMIAPP_SUBDEVS];
 	u32 ssds_used;
 	struct smiapp_subdev *src;
@@ -218,8 +211,6 @@ struct smiapp_sensor {
 	u16 image_start; /* image data start line */
 	u16 visible_pixel_start; /* start pixel of the visible image */
 
-	int power_count;
-
 	bool streaming;
 	bool dev_init_done;
 	u8 compressed_min_bpp;
-- 
2.1.4

[PATCH 5/5] smiapp: Implement support for autosuspend

[Sakari Ailus]

Delay suspending the device by 1000 ms by default.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp/smiapp-core.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index ba5ad36..313f037 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -1559,7 +1559,8 @@ static int smiapp_set_stream(struct v4l2_subdev *subdev, int enable)
 		rval = smiapp_stop_streaming(sensor);
 		sensor->streaming = false;
 
-		pm_runtime_put(&client->dev);
+		pm_runtime_mark_last_busy(&client->dev);
+		pm_runtime_put_autosuspend(&client->dev);
 	}
 
 	return rval;
@@ -2327,7 +2328,8 @@ smiapp_sysfs_nvm_read(struct device *dev, struct device_attribute *attr,
 			dev_err(&client->dev, "nvm read failed\n");
 			return -ENODEV;
 		}
-		pm_runtime_put(&client->dev);
+		pm_runtime_mark_last_busy(&client->dev);
+		pm_runtime_put_autosuspend(&client->dev);
 	}
 	/*
 	 * NVM is still way below a PAGE_SIZE, so we can safely
@@ -3038,7 +3040,9 @@ static int smiapp_probe(struct i2c_client *client,
 	if (rval < 0)
 		goto out_media_entity_cleanup;
 
-	pm_runtime_put(&client->dev);
+	pm_runtime_set_autosuspend_delay(&client->dev, 1000);
+	pm_runtime_use_autosuspend(&client->dev);
+	pm_runtime_put_autosuspend(&client->dev);
 
 	return 0;
 
-- 
2.1.4

Re: [PATCH 5/5] smiapp: Implement support for autosuspend

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 257 bytes --]

Hi,

On Thu, Sep 15, 2016 at 02:29:21PM +0300, Sakari Ailus wrote:
> Delay suspending the device by 1000 ms by default.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-By: Sebastian Reichel <sre@kernel.org>

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v1.1 5/5] smiapp: Implement support for autosuspend

[Sakari Ailus]

Delay suspending the device by 1000 ms by default.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---

since v1:

- Increment usage count before register write using
  pm_runtime_get_noresume(), and decrement it before returning. This
  avoids a serialisation problem with autosuspend.

 drivers/media/i2c/smiapp/smiapp-core.c | 10 +++++++---
 drivers/media/i2c/smiapp/smiapp-regs.c | 21 +++++++++++++++------
 2 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index f1c95bf..77c0a26 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -1556,7 +1556,8 @@ static int smiapp_set_stream(struct v4l2_subdev *subdev, int enable)
 		rval = smiapp_stop_streaming(sensor);
 		sensor->streaming = false;
 
-		pm_runtime_put(&client->dev);
+		pm_runtime_mark_last_busy(&client->dev);
+		pm_runtime_put_autosuspend(&client->dev);
 	}
 
 	return rval;
@@ -2324,7 +2325,8 @@ smiapp_sysfs_nvm_read(struct device *dev, struct device_attribute *attr,
 			dev_err(&client->dev, "nvm read failed\n");
 			return -ENODEV;
 		}
-		pm_runtime_put(&client->dev);
+		pm_runtime_mark_last_busy(&client->dev);
+		pm_runtime_put_autosuspend(&client->dev);
 	}
 	/*
 	 * NVM is still way below a PAGE_SIZE, so we can safely
@@ -3052,7 +3054,9 @@ static int smiapp_probe(struct i2c_client *client,
 	if (rval < 0)
 		goto out_media_entity_cleanup;
 
-	pm_runtime_put(&client->dev);
+	pm_runtime_set_autosuspend_delay(&client->dev, 1000);
+	pm_runtime_use_autosuspend(&client->dev);
+	pm_runtime_put_autosuspend(&client->dev);
 
 	return 0;
 
diff --git a/drivers/media/i2c/smiapp/smiapp-regs.c b/drivers/media/i2c/smiapp/smiapp-regs.c
index a9c7baf..ef15478 100644
--- a/drivers/media/i2c/smiapp/smiapp-regs.c
+++ b/drivers/media/i2c/smiapp/smiapp-regs.c
@@ -290,16 +290,25 @@ int smiapp_write_no_quirk(struct smiapp_sensor *sensor, u32 reg, u32 val)
 int smiapp_write(struct smiapp_sensor *sensor, u32 reg, u32 val)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
-	int rval;
+	int rval = 0;
+
+	pm_runtime_get_noresume(&client->dev);
 
 	if (pm_runtime_suspended(&client->dev))
-		return 0;
+		goto out;
 
 	rval = smiapp_call_quirk(sensor, reg_access, true, &reg, &val);
-	if (rval == -ENOIOCTLCMD)
-		return 0;
+	if (rval == -ENOIOCTLCMD) {
+		rval = 0;
+		goto out;
+	}
 	if (rval < 0)
-		return rval;
+		goto out;
+
+	rval = smiapp_write_no_quirk(sensor, reg, val);
+
+out:
+	pm_runtime_put_autosuspend(&client->dev);
 
-	return smiapp_write_no_quirk(sensor, reg, val);
+	return rval;
 }
-- 
2.7.4

Re: [PATCH v1.1 5/5] smiapp: Implement support for autosuspend

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 653 bytes --]

Hi,

On Tue, Sep 20, 2016 at 03:29:58PM +0300, Sakari Ailus wrote:
> Delay suspending the device by 1000 ms by default.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> 
> since v1:
> 
> - Increment usage count before register write using
>   pm_runtime_get_noresume(), and decrement it before returning. This
>   avoids a serialisation problem with autosuspend.
> 
>  drivers/media/i2c/smiapp/smiapp-core.c | 10 +++++++---
>  drivers/media/i2c/smiapp/smiapp-regs.c | 21 +++++++++++++++------
>  2 files changed, 22 insertions(+), 9 deletions(-)

Reviewed-By: Sebastian Reichel <sre@kernel.org>

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v1.1 5/5] smiapp: Implement support for autosuspend

[Sakari Ailus]

Sebastian Reichel wrote:
> Hi,
>
> On Tue, Sep 20, 2016 at 03:29:58PM +0300, Sakari Ailus wrote:
>> Delay suspending the device by 1000 ms by default.
>>
>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>> ---
>>
>> since v1:
>>
>> - Increment usage count before register write using
>>    pm_runtime_get_noresume(), and decrement it before returning. This
>>    avoids a serialisation problem with autosuspend.
>>
>>   drivers/media/i2c/smiapp/smiapp-core.c | 10 +++++++---
>>   drivers/media/i2c/smiapp/smiapp-regs.c | 21 +++++++++++++++------
>>   2 files changed, 22 insertions(+), 9 deletions(-)
>
> Reviewed-By: Sebastian Reichel <sre@kernel.org>

Danke schön! :-)

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

[PATCH v1.2 5/5] smiapp: Implement support for autosuspend

[Sakari Ailus]

Delay suspending the device by 1000 ms by default. This is done on
explicit power off through s_power() callback, through releasing the
file descriptor, NVM read or when the probe finishes.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
since v1.1:

- More or less just rebased. The previous patch changed quite a bit so
  this one did as well.

 drivers/media/i2c/smiapp/smiapp-core.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 68adc1b..44e32cf 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -1380,17 +1380,22 @@ static int smiapp_power_off(struct device *dev)
 
 static int smiapp_set_power(struct v4l2_subdev *subdev, int on)
 {
-	int rval = 0;
+	int rval;
 
-	if (on) {
-		rval = pm_runtime_get_sync(subdev->dev);
-		if (rval >= 0)
-			return 0;
+	if (!on) {
+		pm_runtime_mark_last_busy(subdev->dev);
+		pm_runtime_put_autosuspend(subdev->dev);
 
-		if (rval != -EBUSY && rval != -EAGAIN)
-			pm_runtime_set_active(subdev->dev);
+		return 0;
 	}
 
+	rval = pm_runtime_get_sync(subdev->dev);
+	if (rval >= 0)
+		return 0;
+
+	if (rval != -EBUSY && rval != -EAGAIN)
+		pm_runtime_set_active(subdev->dev);
+
 	pm_runtime_put(subdev->dev);
 
 	return rval;
@@ -2340,7 +2345,8 @@ smiapp_sysfs_nvm_read(struct device *dev, struct device_attribute *attr,
 			return -ENODEV;
 		}
 
-		pm_runtime_put(&client->dev);
+		pm_runtime_mark_last_busy(&client->dev);
+		pm_runtime_put_autosuspend(&client->dev);
 	}
 	/*
 	 * NVM is still way below a PAGE_SIZE, so we can safely
@@ -2681,7 +2687,8 @@ static int smiapp_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 
 static int smiapp_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 {
-	pm_runtime_put(sd->dev);
+	pm_runtime_mark_last_busy(&client->dev);
+	pm_runtime_put_autosuspend(&client->dev);
 
 	return 0;
 }
@@ -3093,7 +3100,9 @@ static int smiapp_probe(struct i2c_client *client,
 	if (rval < 0)
 		goto out_media_entity_cleanup;
 
-	pm_runtime_put(&client->dev);
+	pm_runtime_set_autosuspend_delay(&client->dev, 1000);
+	pm_runtime_use_autosuspend(&client->dev);
+	pm_runtime_put_autosuspend(&client->dev);
 
 	return 0;
 
-- 
2.1.4

[PATCH v1.3 5/5] smiapp: Implement support for autosuspend

[Sakari Ailus]

Delay suspending the device by 1000 ms by default. This is done on
explicit power off through s_power() callback, through releasing the
file descriptor, NVM read or when the probe finishes.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
since v1.2:

- Fix copy & paste issue in smiapp_close().

 drivers/media/i2c/smiapp/smiapp-core.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 68adc1b..59872b3 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -1380,17 +1380,22 @@ static int smiapp_power_off(struct device *dev)
 
 static int smiapp_set_power(struct v4l2_subdev *subdev, int on)
 {
-	int rval = 0;
+	int rval;
 
-	if (on) {
-		rval = pm_runtime_get_sync(subdev->dev);
-		if (rval >= 0)
-			return 0;
+	if (!on) {
+		pm_runtime_mark_last_busy(subdev->dev);
+		pm_runtime_put_autosuspend(subdev->dev);
 
-		if (rval != -EBUSY && rval != -EAGAIN)
-			pm_runtime_set_active(subdev->dev);
+		return 0;
 	}
 
+	rval = pm_runtime_get_sync(subdev->dev);
+	if (rval >= 0)
+		return 0;
+
+	if (rval != -EBUSY && rval != -EAGAIN)
+		pm_runtime_set_active(subdev->dev);
+
 	pm_runtime_put(subdev->dev);
 
 	return rval;
@@ -2340,7 +2345,8 @@ smiapp_sysfs_nvm_read(struct device *dev, struct device_attribute *attr,
 			return -ENODEV;
 		}
 
-		pm_runtime_put(&client->dev);
+		pm_runtime_mark_last_busy(&client->dev);
+		pm_runtime_put_autosuspend(&client->dev);
 	}
 	/*
 	 * NVM is still way below a PAGE_SIZE, so we can safely
@@ -2681,7 +2687,8 @@ static int smiapp_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 
 static int smiapp_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 {
-	pm_runtime_put(sd->dev);
+	pm_runtime_mark_last_busy(sd->dev);
+	pm_runtime_put_autosuspend(sd->dev);
 
 	return 0;
 }
@@ -3093,7 +3100,9 @@ static int smiapp_probe(struct i2c_client *client,
 	if (rval < 0)
 		goto out_media_entity_cleanup;
 
-	pm_runtime_put(&client->dev);
+	pm_runtime_set_autosuspend_delay(&client->dev, 1000);
+	pm_runtime_use_autosuspend(&client->dev);
+	pm_runtime_put_autosuspend(&client->dev);
 
 	return 0;
 
-- 
2.1.4