[PATCH 0/7] smiapp binning limit cleanups, runtime PM fix and a omap3isp fix

[PATCH 0/7] smiapp binning limit cleanups, runtime PM fix and a omap3isp fix

[Sakari Ailus]

Hi folks,

This set cleans up binning limit handling and fixes runtime PM use by
enabling runtime PM *before* any functionality requiring runtime PM may be
used.

Also a small omap3isp fix is included.

Sakari Ailus (7):
  smiapp: Don't get binning limits dynamically
  smiapp: Move binning configuration to streaming start
  smiapp: Don't update sensor configuration during power-on init
  smiapp: Use non-binned and binned limits correctly
  smiapp: Register sensor after enabling runtime PM on the device
  smiapp: Rename update_mode as pll_blanking_update
  omap3isp: Ignore failure of stopping streaming on external subdev

 drivers/media/i2c/smiapp/smiapp-core.c | 157 +++++++++----------------
 drivers/media/platform/omap3isp/isp.c  |   8 +-
 2 files changed, 57 insertions(+), 108 deletions(-)

-- 
2.20.1

[PATCH 1/7] smiapp: Don't get binning limits dynamically

[Sakari Ailus]

The driver implementation assumed the binning limits could change
dynamically based on the binning configuration. This is not actually the
case; these limits are static and suitable to be used with all binning
configurations but possibly not optimal limit for many of those
configurations.

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

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index a274527987b8..bf054b2e8e8b 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -682,66 +682,6 @@ static int smiapp_get_all_limits(struct smiapp_sensor *sensor)
 	return 0;
 }
 
-static int smiapp_get_limits_binning(struct smiapp_sensor *sensor)
-{
-	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
-	static u32 const limits[] = {
-		SMIAPP_LIMIT_MIN_FRAME_LENGTH_LINES_BIN,
-		SMIAPP_LIMIT_MAX_FRAME_LENGTH_LINES_BIN,
-		SMIAPP_LIMIT_MIN_LINE_LENGTH_PCK_BIN,
-		SMIAPP_LIMIT_MAX_LINE_LENGTH_PCK_BIN,
-		SMIAPP_LIMIT_MIN_LINE_BLANKING_PCK_BIN,
-		SMIAPP_LIMIT_FINE_INTEGRATION_TIME_MIN_BIN,
-		SMIAPP_LIMIT_FINE_INTEGRATION_TIME_MAX_MARGIN_BIN,
-	};
-	static u32 const limits_replace[] = {
-		SMIAPP_LIMIT_MIN_FRAME_LENGTH_LINES,
-		SMIAPP_LIMIT_MAX_FRAME_LENGTH_LINES,
-		SMIAPP_LIMIT_MIN_LINE_LENGTH_PCK,
-		SMIAPP_LIMIT_MAX_LINE_LENGTH_PCK,
-		SMIAPP_LIMIT_MIN_LINE_BLANKING_PCK,
-		SMIAPP_LIMIT_FINE_INTEGRATION_TIME_MIN,
-		SMIAPP_LIMIT_FINE_INTEGRATION_TIME_MAX_MARGIN,
-	};
-	unsigned int i;
-	int rval;
-
-	if (sensor->limits[SMIAPP_LIMIT_BINNING_CAPABILITY] ==
-	    SMIAPP_BINNING_CAPABILITY_NO) {
-		for (i = 0; i < ARRAY_SIZE(limits); i++)
-			sensor->limits[limits[i]] =
-				sensor->limits[limits_replace[i]];
-
-		return 0;
-	}
-
-	rval = smiapp_get_limits(sensor, limits, ARRAY_SIZE(limits));
-	if (rval < 0)
-		return rval;
-
-	/*
-	 * Sanity check whether the binning limits are valid. If not,
-	 * use the non-binning ones.
-	 */
-	if (sensor->limits[SMIAPP_LIMIT_MIN_FRAME_LENGTH_LINES_BIN]
-	    && sensor->limits[SMIAPP_LIMIT_MIN_LINE_LENGTH_PCK_BIN]
-	    && sensor->limits[SMIAPP_LIMIT_MIN_LINE_BLANKING_PCK_BIN])
-		return 0;
-
-	for (i = 0; i < ARRAY_SIZE(limits); i++) {
-		dev_dbg(&client->dev,
-			"replace limit 0x%8.8x \"%s\" = %d, 0x%x\n",
-			smiapp_reg_limits[limits[i]].addr,
-			smiapp_reg_limits[limits[i]].what,
-			sensor->limits[limits_replace[i]],
-			sensor->limits[limits_replace[i]]);
-		sensor->limits[limits[i]] =
-			sensor->limits[limits_replace[i]];
-	}
-
-	return 0;
-}
-
 static int smiapp_get_mbus_formats(struct smiapp_sensor *sensor)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
@@ -940,11 +880,6 @@ static int smiapp_update_mode(struct smiapp_sensor *sensor)
 	if (rval < 0)
 		return rval;
 
-	/* Get updated limits due to binning */
-	rval = smiapp_get_limits_binning(sensor);
-	if (rval < 0)
-		return rval;
-
 	rval = smiapp_pll_update(sensor);
 	if (rval < 0)
 		return rval;
-- 
2.20.1

[PATCH 2/7] smiapp: Move binning configuration to streaming start

[Sakari Ailus]

Only write the binning configuration at stream start. It has no effect
otherwise.

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

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index bf054b2e8e8b..edaeebaada79 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -857,29 +857,8 @@ static void smiapp_update_blanking(struct smiapp_sensor *sensor)
 static int smiapp_update_mode(struct smiapp_sensor *sensor)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
-	unsigned int binning_mode;
 	int rval;
 
-	/* Binning has to be set up here; it affects limits */
-	if (sensor->binning_horizontal == 1 &&
-	    sensor->binning_vertical == 1) {
-		binning_mode = 0;
-	} else {
-		u8 binning_type =
-			(sensor->binning_horizontal << 4)
-			| sensor->binning_vertical;
-
-		rval = smiapp_write(
-			sensor, SMIAPP_REG_U8_BINNING_TYPE, binning_type);
-		if (rval < 0)
-			return rval;
-
-		binning_mode = 1;
-	}
-	rval = smiapp_write(sensor, SMIAPP_REG_U8_BINNING_MODE, binning_mode);
-	if (rval < 0)
-		return rval;
-
 	rval = smiapp_pll_update(sensor);
 	if (rval < 0)
 		return rval;
@@ -1351,6 +1330,7 @@ static int smiapp_power_off(struct device *dev)
 static int smiapp_start_streaming(struct smiapp_sensor *sensor)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
+	unsigned int binning_mode;
 	int rval;
 
 	mutex_lock(&sensor->mutex);
@@ -1361,6 +1341,27 @@ static int smiapp_start_streaming(struct smiapp_sensor *sensor)
 	if (rval)
 		goto out;
 
+	/* Binning configuration */
+	if (sensor->binning_horizontal == 1 &&
+	    sensor->binning_vertical == 1) {
+		binning_mode = 0;
+	} else {
+		u8 binning_type =
+			(sensor->binning_horizontal << 4)
+			| sensor->binning_vertical;
+
+		rval = smiapp_write(
+			sensor, SMIAPP_REG_U8_BINNING_TYPE, binning_type);
+		if (rval < 0)
+			return rval;
+
+		binning_mode = 1;
+	}
+	rval = smiapp_write(sensor, SMIAPP_REG_U8_BINNING_MODE, binning_mode);
+	if (rval < 0)
+		return rval;
+
+	/* Set up PLL */
 	rval = smiapp_pll_configure(sensor);
 	if (rval)
 		goto out;
-- 
2.20.1

[PATCH 3/7] smiapp: Don't update sensor configuration during power-on init

[Sakari Ailus]

The sensor configuration since it was previously powered off was not
changed, so no need to update the PLL configuration etc. What is necessary
though is to re-apply the configuration to the sensor's registers.

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

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index edaeebaada79..5fc3bcc6def0 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -1267,10 +1267,6 @@ static int smiapp_power_on(struct device *dev)
 		rval = __v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler);
 		if (rval)
 			goto out_cci_addr_fail;
-
-		rval = smiapp_update_mode(sensor);
-		if (rval < 0)
-			goto out_cci_addr_fail;
 	}
 
 	mutex_unlock(&sensor->mutex);
-- 
2.20.1

[PATCH 4/7] smiapp: Use non-binned and binned limits correctly

[Sakari Ailus]

Use non-binned limits when binning is disabled and binned when they're
enabled.

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

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 5fc3bcc6def0..30830878d069 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -831,23 +831,36 @@ static void smiapp_update_blanking(struct smiapp_sensor *sensor)
 {
 	struct v4l2_ctrl *vblank = sensor->vblank;
 	struct v4l2_ctrl *hblank = sensor->hblank;
+	uint16_t min_fll, max_fll, min_llp, max_llp, min_lbp;
 	int min, max;
 
+	if (sensor->binning_vertical > 1 || sensor->binning_horizontal > 1) {
+		min_fll = sensor->limits[SMIAPP_LIMIT_MIN_FRAME_LENGTH_LINES_BIN];
+		max_fll = sensor->limits[SMIAPP_LIMIT_MAX_FRAME_LENGTH_LINES_BIN];
+		min_llp = sensor->limits[SMIAPP_LIMIT_MIN_LINE_LENGTH_PCK_BIN];
+		max_llp = sensor->limits[SMIAPP_LIMIT_MAX_LINE_LENGTH_PCK_BIN];
+		min_lbp = sensor->limits[SMIAPP_LIMIT_MIN_LINE_BLANKING_PCK_BIN];
+	} else {
+		min_fll = sensor->limits[SMIAPP_LIMIT_MIN_FRAME_LENGTH_LINES];
+		max_fll = sensor->limits[SMIAPP_LIMIT_MAX_FRAME_LENGTH_LINES];
+		min_llp = sensor->limits[SMIAPP_LIMIT_MIN_LINE_LENGTH_PCK];
+		max_llp = sensor->limits[SMIAPP_LIMIT_MAX_LINE_LENGTH_PCK];
+		min_lbp = sensor->limits[SMIAPP_LIMIT_MIN_LINE_BLANKING_PCK];
+	}
+
 	min = max_t(int,
 		    sensor->limits[SMIAPP_LIMIT_MIN_FRAME_BLANKING_LINES],
-		    sensor->limits[SMIAPP_LIMIT_MIN_FRAME_LENGTH_LINES_BIN] -
+		    min_fll -
 		    sensor->pixel_array->crop[SMIAPP_PA_PAD_SRC].height);
-	max = sensor->limits[SMIAPP_LIMIT_MAX_FRAME_LENGTH_LINES_BIN] -
-		sensor->pixel_array->crop[SMIAPP_PA_PAD_SRC].height;
+	max = max_fll -	sensor->pixel_array->crop[SMIAPP_PA_PAD_SRC].height;
 
 	__v4l2_ctrl_modify_range(vblank, min, max, vblank->step, min);
 
 	min = max_t(int,
-		    sensor->limits[SMIAPP_LIMIT_MIN_LINE_LENGTH_PCK_BIN] -
+		    min_llp -
 		    sensor->pixel_array->crop[SMIAPP_PA_PAD_SRC].width,
-		    sensor->limits[SMIAPP_LIMIT_MIN_LINE_BLANKING_PCK_BIN]);
-	max = sensor->limits[SMIAPP_LIMIT_MAX_LINE_LENGTH_PCK_BIN] -
-		sensor->pixel_array->crop[SMIAPP_PA_PAD_SRC].width;
+		    min_lbp);
+	max = max_llp - sensor->pixel_array->crop[SMIAPP_PA_PAD_SRC].width;
 
 	__v4l2_ctrl_modify_range(hblank, min, max, hblank->step, min);
 
-- 
2.20.1

[PATCH 5/7] smiapp: Register sensor after enabling runtime PM on the device

[Sakari Ailus]

Earlier it was possible that the parts of the driver that assumed runtime
PM was enabled were being called before runtime PM was enabled in the
driver's probe function. So enable runtime PM before registering the
sub-device.

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

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 30830878d069..174a965c17b8 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -3058,19 +3058,23 @@ static int smiapp_probe(struct i2c_client *client)
 	if (rval < 0)
 		goto out_media_entity_cleanup;
 
-	rval = v4l2_async_register_subdev_sensor_common(&sensor->src->sd);
-	if (rval < 0)
-		goto out_media_entity_cleanup;
-
 	pm_runtime_set_active(&client->dev);
 	pm_runtime_get_noresume(&client->dev);
 	pm_runtime_enable(&client->dev);
+
+	rval = v4l2_async_register_subdev_sensor_common(&sensor->src->sd);
+	if (rval < 0)
+		goto out_disable_runtime_pm;
+
 	pm_runtime_set_autosuspend_delay(&client->dev, 1000);
 	pm_runtime_use_autosuspend(&client->dev);
 	pm_runtime_put_autosuspend(&client->dev);
 
 	return 0;
 
+out_disable_runtime_pm:
+	pm_runtime_disable(&client->dev);
+
 out_media_entity_cleanup:
 	media_entity_cleanup(&sensor->src->sd.entity);
 
-- 
2.20.1

[PATCH 6/7] smiapp: Rename update_mode as pll_blanking_update

[Sakari Ailus]

Rename the confusingly named smiapp_update_mode() function as
smiapp_pll_blanking_update(). The function is used to calculate new PLL
and blanking configuration after binning or scaling configuration has been
changed.

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

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 174a965c17b8..2af796abb6b8 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -867,7 +867,7 @@ static void smiapp_update_blanking(struct smiapp_sensor *sensor)
 	__smiapp_update_exposure_limits(sensor);
 }
 
-static int smiapp_update_mode(struct smiapp_sensor *sensor)
+static int smiapp_pll_blanking_update(struct smiapp_sensor *sensor)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
 	int rval;
@@ -2047,7 +2047,7 @@ static int smiapp_set_compose(struct v4l2_subdev *subdev,
 	smiapp_propagate(subdev, cfg, sel->which, V4L2_SEL_TGT_COMPOSE);
 
 	if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE)
-		return smiapp_update_mode(sensor);
+		return smiapp_pll_blanking_update(sensor);
 
 	return 0;
 }
@@ -3043,7 +3043,7 @@ static int smiapp_probe(struct i2c_client *client)
 	}
 
 	mutex_lock(&sensor->mutex);
-	rval = smiapp_update_mode(sensor);
+	rval = smiapp_pll_blanking_update(sensor);
 	mutex_unlock(&sensor->mutex);
 	if (rval) {
 		dev_err(&client->dev, "update mode failed\n");
-- 
2.20.1

[PATCH 7/7] omap3isp: Ignore failure of stopping streaming on external subdev

[Sakari Ailus]

The isp was marked to have failed to stop if stopping streaming on an
external subdev failed. The return value from the external subdev should
be ignored instead as it is not part of the ISP and thus the ISP does not
need to be reset for that reason.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/platform/omap3isp/isp.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 327c5716922a..a4ee6b86663e 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -810,6 +810,10 @@ static int isp_pipeline_disable(struct isp_pipeline *pipe)
 
 		ret = v4l2_subdev_call(subdev, video, s_stream, 0);
 
+		/* Stop at the first external sub-device. */
+		if (subdev->dev != isp->dev)
+			break;
+
 		if (subdev == &isp->isp_res.subdev)
 			ret |= isp_pipeline_wait(isp, isp_pipeline_wait_resizer);
 		else if (subdev == &isp->isp_prev.subdev)
@@ -837,10 +841,6 @@ static int isp_pipeline_disable(struct isp_pipeline *pipe)
 						      &subdev->entity);
 			failure = -ETIMEDOUT;
 		}
-
-		/* Stop at the first external sub-device. */
-		if (subdev->dev != isp->dev)
-			break;
 	}
 
 	return failure;
-- 
2.20.1