[PATCH 0/9] soc-camera sensor improvements

[PATCH 0/9] soc-camera sensor improvements

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

As per Guennadi's suggestions, improve the code of various soc-camera sensor
drivers to avoid duplicating code.

Note that the mt9t112 issue is something I discovered and that has been there
from before my recent patches that removed the old video ops.

Also note that that is the only driver that Guennadi commented upon ("Now
mt9t112_frame_check() will be called twice in the .s_mbus_fmt() case.") that
I did not change: this driver is more complex than the others and I did not
see an easy way of changing this.

I might tackle that one later as I actually have hardware with this sensor.

Regards,

	Hans

Hans Verkuil (9):
  imx074: don't call imx074_find_datafmt() twice
  mt9m001: avoid calling mt9m001_find_datafmt() twice
  mt9v022: avoid calling mt9v022_find_datafmt() twice
  ov2640: avoid calling ov2640_select_win() twice
  ov5642: avoid calling ov5642_find_datafmt() twice
  ov772x: avoid calling ov772x_select_params() twice
  ov9640: avoid calling ov9640_res_roundup() twice
  ov9740: avoid calling ov9740_res_roundup() twice
  mt9t112: initialize left and top

 drivers/media/i2c/soc_camera/imx074.c  |  7 +++---
 drivers/media/i2c/soc_camera/mt9m001.c |  8 +++----
 drivers/media/i2c/soc_camera/mt9t112.c |  3 ++-
 drivers/media/i2c/soc_camera/mt9v022.c |  8 +++----
 drivers/media/i2c/soc_camera/ov2640.c  | 21 +++++++++--------
 drivers/media/i2c/soc_camera/ov5642.c  |  7 +++---
 drivers/media/i2c/soc_camera/ov772x.c  | 41 +++++++++++-----------------------
 drivers/media/i2c/soc_camera/ov9640.c  | 24 +++-----------------
 drivers/media/i2c/soc_camera/ov9740.c  | 18 +--------------
 9 files changed, 45 insertions(+), 92 deletions(-)

-- 
2.1.4

[PATCH 1/9] imx074: don't call imx074_find_datafmt() twice

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Simplify imx074_set_fmt().

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Reported-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/media/i2c/soc_camera/imx074.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/soc_camera/imx074.c b/drivers/media/i2c/soc_camera/imx074.c
index f68c235..4226f06 100644
--- a/drivers/media/i2c/soc_camera/imx074.c
+++ b/drivers/media/i2c/soc_camera/imx074.c
@@ -171,8 +171,9 @@ static int imx074_set_fmt(struct v4l2_subdev *sd,
 		/* MIPI CSI could have changed the format, double-check */
 		if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
 			return -EINVAL;
-		mf->code	= imx074_colour_fmts[0].code;
-		mf->colorspace	= imx074_colour_fmts[0].colorspace;
+		fmt = imx074_colour_fmts;
+		mf->code = fmt->code;
+		mf->colorspace = fmt->colorspace;
 	}
 
 	mf->width	= IMX074_WIDTH;
@@ -180,7 +181,7 @@ static int imx074_set_fmt(struct v4l2_subdev *sd,
 	mf->field	= V4L2_FIELD_NONE;
 
 	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
-		priv->fmt = imx074_find_datafmt(mf->code);
+		priv->fmt = fmt;
 	else
 		cfg->try_fmt = *mf;
 
-- 
2.1.4

[PATCH 2/9] mt9m001: avoid calling mt9m001_find_datafmt() twice

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Simplify mt9m001_s_fmt and mt9m001_set_fmt.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Reported-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/media/i2c/soc_camera/mt9m001.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/soc_camera/mt9m001.c b/drivers/media/i2c/soc_camera/mt9m001.c
index 4fbdd1e..1f49140 100644
--- a/drivers/media/i2c/soc_camera/mt9m001.c
+++ b/drivers/media/i2c/soc_camera/mt9m001.c
@@ -271,6 +271,7 @@ static int mt9m001_get_fmt(struct v4l2_subdev *sd,
 }
 
 static int mt9m001_s_fmt(struct v4l2_subdev *sd,
+			 const struct mt9m001_datafmt *fmt,
 			 struct v4l2_mbus_framefmt *mf)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
@@ -290,9 +291,8 @@ static int mt9m001_s_fmt(struct v4l2_subdev *sd,
 	if (!ret) {
 		mf->width	= mt9m001->rect.width;
 		mf->height	= mt9m001->rect.height;
-		mt9m001->fmt	= mt9m001_find_datafmt(mf->code,
-					mt9m001->fmts, mt9m001->num_fmts);
-		mf->colorspace	= mt9m001->fmt->colorspace;
+		mt9m001->fmt	= fmt;
+		mf->colorspace	= fmt->colorspace;
 	}
 
 	return ret;
@@ -328,7 +328,7 @@ static int mt9m001_set_fmt(struct v4l2_subdev *sd,
 	mf->colorspace	= fmt->colorspace;
 
 	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
-		return mt9m001_s_fmt(sd, mf);
+		return mt9m001_s_fmt(sd, fmt, mf);
 	cfg->try_fmt = *mf;
 	return 0;
 }
-- 
2.1.4

[PATCH 3/9] mt9v022: avoid calling mt9v022_find_datafmt() twice

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Simplify mt9v022_s_fmt and mt9v022_set_fmt.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Reported-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/media/i2c/soc_camera/mt9v022.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/soc_camera/mt9v022.c b/drivers/media/i2c/soc_camera/mt9v022.c
index f313774..00516bf 100644
--- a/drivers/media/i2c/soc_camera/mt9v022.c
+++ b/drivers/media/i2c/soc_camera/mt9v022.c
@@ -396,6 +396,7 @@ static int mt9v022_get_fmt(struct v4l2_subdev *sd,
 }
 
 static int mt9v022_s_fmt(struct v4l2_subdev *sd,
+			 const struct mt9v022_datafmt *fmt,
 			 struct v4l2_mbus_framefmt *mf)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
@@ -434,9 +435,8 @@ static int mt9v022_s_fmt(struct v4l2_subdev *sd,
 	if (!ret) {
 		mf->width	= mt9v022->rect.width;
 		mf->height	= mt9v022->rect.height;
-		mt9v022->fmt	= mt9v022_find_datafmt(mf->code,
-					mt9v022->fmts, mt9v022->num_fmts);
-		mf->colorspace	= mt9v022->fmt->colorspace;
+		mt9v022->fmt	= fmt;
+		mf->colorspace	= fmt->colorspace;
 	}
 
 	return ret;
@@ -471,7 +471,7 @@ static int mt9v022_set_fmt(struct v4l2_subdev *sd,
 	mf->colorspace	= fmt->colorspace;
 
 	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
-		return mt9v022_s_fmt(sd, mf);
+		return mt9v022_s_fmt(sd, fmt, mf);
 	cfg->try_fmt = *mf;
 	return 0;
 }
-- 
2.1.4

[PATCH 4/9] ov2640: avoid calling ov2640_select_win() twice

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Simplify ov2640_set_params and ov2640_set_fmt.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Reported-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/media/i2c/soc_camera/ov2640.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/media/i2c/soc_camera/ov2640.c b/drivers/media/i2c/soc_camera/ov2640.c
index 9b4f5de..5dcaf24 100644
--- a/drivers/media/i2c/soc_camera/ov2640.c
+++ b/drivers/media/i2c/soc_camera/ov2640.c
@@ -769,15 +769,15 @@ static const struct ov2640_win_size *ov2640_select_win(u32 *width, u32 *height)
 	return &ov2640_supported_win_sizes[default_size];
 }
 
-static int ov2640_set_params(struct i2c_client *client, u32 *width, u32 *height,
-			     u32 code)
+static int ov2640_set_params(struct i2c_client *client,
+			     const struct ov2640_win_size *win, u32 code)
 {
 	struct ov2640_priv       *priv = to_ov2640(client);
 	const struct regval_list *selected_cfmt_regs;
 	int ret;
 
 	/* select win */
-	priv->win = ov2640_select_win(width, height);
+	priv->win = win;
 
 	/* select format */
 	priv->cfmt_code = 0;
@@ -798,6 +798,7 @@ static int ov2640_set_params(struct i2c_client *client, u32 *width, u32 *height,
 	case MEDIA_BUS_FMT_UYVY8_2X8:
 		dev_dbg(&client->dev, "%s: Selected cfmt UYVY", __func__);
 		selected_cfmt_regs = ov2640_uyvy_regs;
+		break;
 	}
 
 	/* reset hardware */
@@ -832,8 +833,6 @@ static int ov2640_set_params(struct i2c_client *client, u32 *width, u32 *height,
 		goto err;
 
 	priv->cfmt_code = code;
-	*width = priv->win->width;
-	*height = priv->win->height;
 
 	return 0;
 
@@ -887,14 +886,13 @@ static int ov2640_set_fmt(struct v4l2_subdev *sd,
 {
 	struct v4l2_mbus_framefmt *mf = &format->format;
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	const struct ov2640_win_size *win;
 
 	if (format->pad)
 		return -EINVAL;
 
-	/*
-	 * select suitable win, but don't store it
-	 */
-	ov2640_select_win(&mf->width, &mf->height);
+	/* select suitable win */
+	win = ov2640_select_win(&mf->width, &mf->height);
 
 	mf->field	= V4L2_FIELD_NONE;
 
@@ -905,14 +903,15 @@ static int ov2640_set_fmt(struct v4l2_subdev *sd,
 		break;
 	default:
 		mf->code = MEDIA_BUS_FMT_UYVY8_2X8;
+		/* fall through */
 	case MEDIA_BUS_FMT_YUYV8_2X8:
 	case MEDIA_BUS_FMT_UYVY8_2X8:
 		mf->colorspace = V4L2_COLORSPACE_JPEG;
+		break;
 	}
 
 	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
-		return ov2640_set_params(client, &mf->width,
-					 &mf->height, mf->code);
+		return ov2640_set_params(client, win, mf->code);
 	cfg->try_fmt = *mf;
 	return 0;
 }
-- 
2.1.4

[PATCH 5/9] ov5642: avoid calling ov5642_find_datafmt() twice

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Simplify ov5642_set_fmt().

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Reported-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/media/i2c/soc_camera/ov5642.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/soc_camera/ov5642.c b/drivers/media/i2c/soc_camera/ov5642.c
index bab9ac0..061fca3 100644
--- a/drivers/media/i2c/soc_camera/ov5642.c
+++ b/drivers/media/i2c/soc_camera/ov5642.c
@@ -804,14 +804,15 @@ static int ov5642_set_fmt(struct v4l2_subdev *sd,
 	if (!fmt) {
 		if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
 			return -EINVAL;
-		mf->code	= ov5642_colour_fmts[0].code;
-		mf->colorspace	= ov5642_colour_fmts[0].colorspace;
+		fmt = ov5642_colour_fmts;
+		mf->code = fmt->code;
+		mf->colorspace = fmt->colorspace;
 	}
 
 	mf->field	= V4L2_FIELD_NONE;
 
 	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
-		priv->fmt = ov5642_find_datafmt(mf->code);
+		priv->fmt = fmt;
 	else
 		cfg->try_fmt = *mf;
 	return 0;
-- 
2.1.4

[PATCH 6/9] ov772x: avoid calling ov772x_select_params() twice

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Merge ov772x_s_fmt into ov772x_set_fmt.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Reported-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/media/i2c/soc_camera/ov772x.c | 41 +++++++++++------------------------
 1 file changed, 13 insertions(+), 28 deletions(-)

diff --git a/drivers/media/i2c/soc_camera/ov772x.c b/drivers/media/i2c/soc_camera/ov772x.c
index f150a8b..aa32bc5 100644
--- a/drivers/media/i2c/soc_camera/ov772x.c
+++ b/drivers/media/i2c/soc_camera/ov772x.c
@@ -895,38 +895,15 @@ static int ov772x_get_fmt(struct v4l2_subdev *sd,
 	return 0;
 }
 
-static int ov772x_s_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf)
-{
-	struct ov772x_priv *priv = to_ov772x(sd);
-	const struct ov772x_color_format *cfmt;
-	const struct ov772x_win_size *win;
-	int ret;
-
-	ov772x_select_params(mf, &cfmt, &win);
-
-	ret = ov772x_set_params(priv, cfmt, win);
-	if (ret < 0)
-		return ret;
-
-	priv->win = win;
-	priv->cfmt = cfmt;
-
-	mf->code = cfmt->code;
-	mf->width = win->rect.width;
-	mf->height = win->rect.height;
-	mf->field = V4L2_FIELD_NONE;
-	mf->colorspace = cfmt->colorspace;
-
-	return 0;
-}
-
 static int ov772x_set_fmt(struct v4l2_subdev *sd,
 		struct v4l2_subdev_pad_config *cfg,
 		struct v4l2_subdev_format *format)
 {
+	struct ov772x_priv *priv = to_ov772x(sd);
 	struct v4l2_mbus_framefmt *mf = &format->format;
 	const struct ov772x_color_format *cfmt;
 	const struct ov772x_win_size *win;
+	int ret;
 
 	if (format->pad)
 		return -EINVAL;
@@ -939,9 +916,17 @@ static int ov772x_set_fmt(struct v4l2_subdev *sd,
 	mf->field = V4L2_FIELD_NONE;
 	mf->colorspace = cfmt->colorspace;
 
-	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
-		return ov772x_s_fmt(sd, mf);
-	cfg->try_fmt = *mf;
+	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
+		cfg->try_fmt = *mf;
+		return 0;
+	}
+
+	ret = ov772x_set_params(priv, cfmt, win);
+	if (ret < 0)
+		return ret;
+
+	priv->win = win;
+	priv->cfmt = cfmt;
 	return 0;
 }
 
-- 
2.1.4

[PATCH 7/9] ov9640: avoid calling ov9640_res_roundup() twice

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Simplify ov9640_s_fmt and ov9640_set_fmt

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Reported-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/media/i2c/soc_camera/ov9640.c | 24 +++---------------------
 1 file changed, 3 insertions(+), 21 deletions(-)

diff --git a/drivers/media/i2c/soc_camera/ov9640.c b/drivers/media/i2c/soc_camera/ov9640.c
index 8caae1c..c8ac41e 100644
--- a/drivers/media/i2c/soc_camera/ov9640.c
+++ b/drivers/media/i2c/soc_camera/ov9640.c
@@ -486,11 +486,8 @@ static int ov9640_s_fmt(struct v4l2_subdev *sd,
 {
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	struct ov9640_reg_alt alts = {0};
-	enum v4l2_colorspace cspace;
-	u32 code = mf->code;
 	int ret;
 
-	ov9640_res_roundup(&mf->width, &mf->height);
 	ov9640_alter_regs(mf->code, &alts);
 
 	ov9640_reset(client);
@@ -499,24 +496,7 @@ static int ov9640_s_fmt(struct v4l2_subdev *sd,
 	if (ret)
 		return ret;
 
-	switch (code) {
-	case MEDIA_BUS_FMT_RGB555_2X8_PADHI_LE:
-	case MEDIA_BUS_FMT_RGB565_2X8_LE:
-		cspace = V4L2_COLORSPACE_SRGB;
-		break;
-	default:
-		code = MEDIA_BUS_FMT_UYVY8_2X8;
-	case MEDIA_BUS_FMT_UYVY8_2X8:
-		cspace = V4L2_COLORSPACE_JPEG;
-	}
-
-	ret = ov9640_write_regs(client, mf->width, code, &alts);
-	if (!ret) {
-		mf->code	= code;
-		mf->colorspace	= cspace;
-	}
-
-	return ret;
+	return ov9640_write_regs(client, mf->width, mf->code, &alts);
 }
 
 static int ov9640_set_fmt(struct v4l2_subdev *sd,
@@ -539,8 +519,10 @@ static int ov9640_set_fmt(struct v4l2_subdev *sd,
 		break;
 	default:
 		mf->code = MEDIA_BUS_FMT_UYVY8_2X8;
+		/* fall through */
 	case MEDIA_BUS_FMT_UYVY8_2X8:
 		mf->colorspace = V4L2_COLORSPACE_JPEG;
+		break;
 	}
 
 	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
-- 
2.1.4

[PATCH 8/9] ov9740: avoid calling ov9740_res_roundup() twice

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Simplify ov9740_s_fmt.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Reported-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/media/i2c/soc_camera/ov9740.c | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/drivers/media/i2c/soc_camera/ov9740.c b/drivers/media/i2c/soc_camera/ov9740.c
index 03a7fc7..61a8e18 100644
--- a/drivers/media/i2c/soc_camera/ov9740.c
+++ b/drivers/media/i2c/soc_camera/ov9740.c
@@ -673,20 +673,8 @@ static int ov9740_s_fmt(struct v4l2_subdev *sd,
 {
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	struct ov9740_priv *priv = to_ov9740(sd);
-	enum v4l2_colorspace cspace;
-	u32 code = mf->code;
 	int ret;
 
-	ov9740_res_roundup(&mf->width, &mf->height);
-
-	switch (code) {
-	case MEDIA_BUS_FMT_YUYV8_2X8:
-		cspace = V4L2_COLORSPACE_SRGB;
-		break;
-	default:
-		return -EINVAL;
-	}
-
 	ret = ov9740_reg_write_array(client, ov9740_defaults,
 				     ARRAY_SIZE(ov9740_defaults));
 	if (ret < 0)
@@ -696,11 +684,7 @@ static int ov9740_s_fmt(struct v4l2_subdev *sd,
 	if (ret < 0)
 		return ret;
 
-	mf->code	= code;
-	mf->colorspace	= cspace;
-
-	memcpy(&priv->current_mf, mf, sizeof(struct v4l2_mbus_framefmt));
-
+	priv->current_mf = *mf;
 	return ret;
 }
 
-- 
2.1.4

[PATCH 9/9] mt9t112: initialize left and top

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

The left and top variables were uninitialized, leading to unexpected
results.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/i2c/soc_camera/mt9t112.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/soc_camera/mt9t112.c b/drivers/media/i2c/soc_camera/mt9t112.c
index de10a76..02190d6 100644
--- a/drivers/media/i2c/soc_camera/mt9t112.c
+++ b/drivers/media/i2c/soc_camera/mt9t112.c
@@ -952,7 +952,8 @@ static int mt9t112_set_fmt(struct v4l2_subdev *sd,
 	struct v4l2_mbus_framefmt *mf = &format->format;
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	struct mt9t112_priv *priv = to_mt9t112(client);
-	unsigned int top, left;
+	unsigned int top = priv->frame.top;
+	unsigned int left = priv->frame.left;
 	int i;
 
 	if (format->pad)
-- 
2.1.4

Re: [PATCH 1/9] imx074: don't call imx074_find_datafmt() twice

[Guennadi Liakhovetski]

Hi Hans,

Thanks for fixing the drivers!

On Sun, 3 May 2015, Hans Verkuil wrote:

> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Simplify imx074_set_fmt().
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> Reported-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>  drivers/media/i2c/soc_camera/imx074.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/soc_camera/imx074.c b/drivers/media/i2c/soc_camera/imx074.c
> index f68c235..4226f06 100644
> --- a/drivers/media/i2c/soc_camera/imx074.c
> +++ b/drivers/media/i2c/soc_camera/imx074.c
> @@ -171,8 +171,9 @@ static int imx074_set_fmt(struct v4l2_subdev *sd,
>  		/* MIPI CSI could have changed the format, double-check */
>  		if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
>  			return -EINVAL;
> -		mf->code	= imx074_colour_fmts[0].code;
> -		mf->colorspace	= imx074_colour_fmts[0].colorspace;
> +		fmt = imx074_colour_fmts;
> +		mf->code = fmt->code;
> +		mf->colorspace = fmt->colorspace;

Uhm, why this change? I understand, that this is equivalent code, but (1) 
is it at all related to the change? and (2) imx074_colour_fmts is an 
array, so, I'd prefer to keep it as is. I do use pointer arithmetics for 
array, but then I'd do something like

+		fmt = imx074_colour_fmts + 0;
+		mf->code = fmt->code;
+		mf->colorspace = fmt->colorspace;

which looks silly:) And then - even more importantly - you overwrite the 
fmt variable, which is then used below instead of calling 
imx074_find_datafmt() again. So, now you assign a (theoretically) 
different value to priv->fmt. I know that array only has one element and 
imx074_find_datafmt() will anyway just return it, but, I don't see why 
this change is needed?

Thanks
Guennadi

>  	}
>  
>  	mf->width	= IMX074_WIDTH;
> @@ -180,7 +181,7 @@ static int imx074_set_fmt(struct v4l2_subdev *sd,
>  	mf->field	= V4L2_FIELD_NONE;
>  
>  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> -		priv->fmt = imx074_find_datafmt(mf->code);
> +		priv->fmt = fmt;
>  	else
>  		cfg->try_fmt = *mf;
>  
> -- 
> 2.1.4
> 

Re: [PATCH 2/9] mt9m001: avoid calling mt9m001_find_datafmt() twice

[Guennadi Liakhovetski]

On Sun, 3 May 2015, Hans Verkuil wrote:

> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Simplify mt9m001_s_fmt and mt9m001_set_fmt.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> Reported-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Thanks
Guennadi

> ---
>  drivers/media/i2c/soc_camera/mt9m001.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/soc_camera/mt9m001.c b/drivers/media/i2c/soc_camera/mt9m001.c
> index 4fbdd1e..1f49140 100644
> --- a/drivers/media/i2c/soc_camera/mt9m001.c
> +++ b/drivers/media/i2c/soc_camera/mt9m001.c
> @@ -271,6 +271,7 @@ static int mt9m001_get_fmt(struct v4l2_subdev *sd,
>  }
>  
>  static int mt9m001_s_fmt(struct v4l2_subdev *sd,
> +			 const struct mt9m001_datafmt *fmt,
>  			 struct v4l2_mbus_framefmt *mf)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> @@ -290,9 +291,8 @@ static int mt9m001_s_fmt(struct v4l2_subdev *sd,
>  	if (!ret) {
>  		mf->width	= mt9m001->rect.width;
>  		mf->height	= mt9m001->rect.height;
> -		mt9m001->fmt	= mt9m001_find_datafmt(mf->code,
> -					mt9m001->fmts, mt9m001->num_fmts);
> -		mf->colorspace	= mt9m001->fmt->colorspace;
> +		mt9m001->fmt	= fmt;
> +		mf->colorspace	= fmt->colorspace;
>  	}
>  
>  	return ret;
> @@ -328,7 +328,7 @@ static int mt9m001_set_fmt(struct v4l2_subdev *sd,
>  	mf->colorspace	= fmt->colorspace;
>  
>  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> -		return mt9m001_s_fmt(sd, mf);
> +		return mt9m001_s_fmt(sd, fmt, mf);
>  	cfg->try_fmt = *mf;
>  	return 0;
>  }
> -- 
> 2.1.4
> 

Re: [PATCH 3/9] mt9v022: avoid calling mt9v022_find_datafmt() twice

[Guennadi Liakhovetski]

On Sun, 3 May 2015, Hans Verkuil wrote:

> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Simplify mt9v022_s_fmt and mt9v022_set_fmt.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> Reported-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Thanks
Guennadi

> ---
>  drivers/media/i2c/soc_camera/mt9v022.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/soc_camera/mt9v022.c b/drivers/media/i2c/soc_camera/mt9v022.c
> index f313774..00516bf 100644
> --- a/drivers/media/i2c/soc_camera/mt9v022.c
> +++ b/drivers/media/i2c/soc_camera/mt9v022.c
> @@ -396,6 +396,7 @@ static int mt9v022_get_fmt(struct v4l2_subdev *sd,
>  }
>  
>  static int mt9v022_s_fmt(struct v4l2_subdev *sd,
> +			 const struct mt9v022_datafmt *fmt,
>  			 struct v4l2_mbus_framefmt *mf)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> @@ -434,9 +435,8 @@ static int mt9v022_s_fmt(struct v4l2_subdev *sd,
>  	if (!ret) {
>  		mf->width	= mt9v022->rect.width;
>  		mf->height	= mt9v022->rect.height;
> -		mt9v022->fmt	= mt9v022_find_datafmt(mf->code,
> -					mt9v022->fmts, mt9v022->num_fmts);
> -		mf->colorspace	= mt9v022->fmt->colorspace;
> +		mt9v022->fmt	= fmt;
> +		mf->colorspace	= fmt->colorspace;
>  	}
>  
>  	return ret;
> @@ -471,7 +471,7 @@ static int mt9v022_set_fmt(struct v4l2_subdev *sd,
>  	mf->colorspace	= fmt->colorspace;
>  
>  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> -		return mt9v022_s_fmt(sd, mf);
> +		return mt9v022_s_fmt(sd, fmt, mf);
>  	cfg->try_fmt = *mf;
>  	return 0;
>  }
> -- 
> 2.1.4
> 

Re: [PATCH 4/9] ov2640: avoid calling ov2640_select_win() twice

[Guennadi Liakhovetski]

Hi Hans,

On Sun, 3 May 2015, Hans Verkuil wrote:

> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Simplify ov2640_set_params and ov2640_set_fmt.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> Reported-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>  drivers/media/i2c/soc_camera/ov2640.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/i2c/soc_camera/ov2640.c b/drivers/media/i2c/soc_camera/ov2640.c
> index 9b4f5de..5dcaf24 100644
> --- a/drivers/media/i2c/soc_camera/ov2640.c
> +++ b/drivers/media/i2c/soc_camera/ov2640.c
> @@ -769,15 +769,15 @@ static const struct ov2640_win_size *ov2640_select_win(u32 *width, u32 *height)
>  	return &ov2640_supported_win_sizes[default_size];
>  }
>  
> -static int ov2640_set_params(struct i2c_client *client, u32 *width, u32 *height,
> -			     u32 code)
> +static int ov2640_set_params(struct i2c_client *client,
> +			     const struct ov2640_win_size *win, u32 code)
>  {
>  	struct ov2640_priv       *priv = to_ov2640(client);
>  	const struct regval_list *selected_cfmt_regs;
>  	int ret;
>  
>  	/* select win */
> -	priv->win = ov2640_select_win(width, height);
> +	priv->win = win;
>  
>  	/* select format */
>  	priv->cfmt_code = 0;
> @@ -798,6 +798,7 @@ static int ov2640_set_params(struct i2c_client *client, u32 *width, u32 *height,
>  	case MEDIA_BUS_FMT_UYVY8_2X8:
>  		dev_dbg(&client->dev, "%s: Selected cfmt UYVY", __func__);
>  		selected_cfmt_regs = ov2640_uyvy_regs;
> +		break;

Hm, IIRC, some versions of gcc complain like "break at the end of a switch 
statement is deprecated." Why did you add this at two locations? Are you 
seeing a warning? If not, maybe better not do that.

Otherwise

Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Thanks
Guennadi

>  	}
>  
>  	/* reset hardware */
> @@ -832,8 +833,6 @@ static int ov2640_set_params(struct i2c_client *client, u32 *width, u32 *height,
>  		goto err;
>  
>  	priv->cfmt_code = code;
> -	*width = priv->win->width;
> -	*height = priv->win->height;
>  
>  	return 0;
>  
> @@ -887,14 +886,13 @@ static int ov2640_set_fmt(struct v4l2_subdev *sd,
>  {
>  	struct v4l2_mbus_framefmt *mf = &format->format;
>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	const struct ov2640_win_size *win;
>  
>  	if (format->pad)
>  		return -EINVAL;
>  
> -	/*
> -	 * select suitable win, but don't store it
> -	 */
> -	ov2640_select_win(&mf->width, &mf->height);
> +	/* select suitable win */
> +	win = ov2640_select_win(&mf->width, &mf->height);
>  
>  	mf->field	= V4L2_FIELD_NONE;
>  
> @@ -905,14 +903,15 @@ static int ov2640_set_fmt(struct v4l2_subdev *sd,
>  		break;
>  	default:
>  		mf->code = MEDIA_BUS_FMT_UYVY8_2X8;
> +		/* fall through */
>  	case MEDIA_BUS_FMT_YUYV8_2X8:
>  	case MEDIA_BUS_FMT_UYVY8_2X8:
>  		mf->colorspace = V4L2_COLORSPACE_JPEG;
> +		break;
>  	}
>  
>  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> -		return ov2640_set_params(client, &mf->width,
> -					 &mf->height, mf->code);
> +		return ov2640_set_params(client, win, mf->code);
>  	cfg->try_fmt = *mf;
>  	return 0;
>  }
> -- 
> 2.1.4
> 

Re: [PATCH 5/9] ov5642: avoid calling ov5642_find_datafmt() twice

[Guennadi Liakhovetski]

Hi Hans,

On Sun, 3 May 2015, Hans Verkuil wrote:

> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Simplify ov5642_set_fmt().
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> Reported-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>  drivers/media/i2c/soc_camera/ov5642.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/soc_camera/ov5642.c b/drivers/media/i2c/soc_camera/ov5642.c
> index bab9ac0..061fca3 100644
> --- a/drivers/media/i2c/soc_camera/ov5642.c
> +++ b/drivers/media/i2c/soc_camera/ov5642.c
> @@ -804,14 +804,15 @@ static int ov5642_set_fmt(struct v4l2_subdev *sd,
>  	if (!fmt) {
>  		if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
>  			return -EINVAL;
> -		mf->code	= ov5642_colour_fmts[0].code;
> -		mf->colorspace	= ov5642_colour_fmts[0].colorspace;
> +		fmt = ov5642_colour_fmts;
> +		mf->code = fmt->code;
> +		mf->colorspace = fmt->colorspace;

Again - I still don't see why this is needed.

Thanks
Guennadi

>  	}
>  
>  	mf->field	= V4L2_FIELD_NONE;
>  
>  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> -		priv->fmt = ov5642_find_datafmt(mf->code);
> +		priv->fmt = fmt;
>  	else
>  		cfg->try_fmt = *mf;
>  	return 0;
> -- 
> 2.1.4
> 

Re: [PATCH 6/9] ov772x: avoid calling ov772x_select_params() twice

[Guennadi Liakhovetski]

On Sun, 3 May 2015, Hans Verkuil wrote:

> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Merge ov772x_s_fmt into ov772x_set_fmt.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> Reported-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Thanks
Guennadi

> ---
>  drivers/media/i2c/soc_camera/ov772x.c | 41 +++++++++++------------------------
>  1 file changed, 13 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/media/i2c/soc_camera/ov772x.c b/drivers/media/i2c/soc_camera/ov772x.c
> index f150a8b..aa32bc5 100644
> --- a/drivers/media/i2c/soc_camera/ov772x.c
> +++ b/drivers/media/i2c/soc_camera/ov772x.c
> @@ -895,38 +895,15 @@ static int ov772x_get_fmt(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> -static int ov772x_s_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf)
> -{
> -	struct ov772x_priv *priv = to_ov772x(sd);
> -	const struct ov772x_color_format *cfmt;
> -	const struct ov772x_win_size *win;
> -	int ret;
> -
> -	ov772x_select_params(mf, &cfmt, &win);
> -
> -	ret = ov772x_set_params(priv, cfmt, win);
> -	if (ret < 0)
> -		return ret;
> -
> -	priv->win = win;
> -	priv->cfmt = cfmt;
> -
> -	mf->code = cfmt->code;
> -	mf->width = win->rect.width;
> -	mf->height = win->rect.height;
> -	mf->field = V4L2_FIELD_NONE;
> -	mf->colorspace = cfmt->colorspace;
> -
> -	return 0;
> -}
> -
>  static int ov772x_set_fmt(struct v4l2_subdev *sd,
>  		struct v4l2_subdev_pad_config *cfg,
>  		struct v4l2_subdev_format *format)
>  {
> +	struct ov772x_priv *priv = to_ov772x(sd);
>  	struct v4l2_mbus_framefmt *mf = &format->format;
>  	const struct ov772x_color_format *cfmt;
>  	const struct ov772x_win_size *win;
> +	int ret;
>  
>  	if (format->pad)
>  		return -EINVAL;
> @@ -939,9 +916,17 @@ static int ov772x_set_fmt(struct v4l2_subdev *sd,
>  	mf->field = V4L2_FIELD_NONE;
>  	mf->colorspace = cfmt->colorspace;
>  
> -	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> -		return ov772x_s_fmt(sd, mf);
> -	cfg->try_fmt = *mf;
> +	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> +		cfg->try_fmt = *mf;
> +		return 0;
> +	}
> +
> +	ret = ov772x_set_params(priv, cfmt, win);
> +	if (ret < 0)
> +		return ret;
> +
> +	priv->win = win;
> +	priv->cfmt = cfmt;
>  	return 0;
>  }
>  
> -- 
> 2.1.4
> 

Re: [PATCH 7/9] ov9640: avoid calling ov9640_res_roundup() twice

[Guennadi Liakhovetski]

Hi Hans,

On Sun, 3 May 2015, Hans Verkuil wrote:

> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Simplify ov9640_s_fmt and ov9640_set_fmt
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> Reported-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>  drivers/media/i2c/soc_camera/ov9640.c | 24 +++---------------------
>  1 file changed, 3 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/media/i2c/soc_camera/ov9640.c b/drivers/media/i2c/soc_camera/ov9640.c
> index 8caae1c..c8ac41e 100644
> --- a/drivers/media/i2c/soc_camera/ov9640.c
> +++ b/drivers/media/i2c/soc_camera/ov9640.c
> @@ -486,11 +486,8 @@ static int ov9640_s_fmt(struct v4l2_subdev *sd,
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
>  	struct ov9640_reg_alt alts = {0};
> -	enum v4l2_colorspace cspace;
> -	u32 code = mf->code;
>  	int ret;
>  
> -	ov9640_res_roundup(&mf->width, &mf->height);
>  	ov9640_alter_regs(mf->code, &alts);
>  
>  	ov9640_reset(client);
> @@ -499,24 +496,7 @@ static int ov9640_s_fmt(struct v4l2_subdev *sd,
>  	if (ret)
>  		return ret;
>  
> -	switch (code) {
> -	case MEDIA_BUS_FMT_RGB555_2X8_PADHI_LE:
> -	case MEDIA_BUS_FMT_RGB565_2X8_LE:
> -		cspace = V4L2_COLORSPACE_SRGB;
> -		break;
> -	default:
> -		code = MEDIA_BUS_FMT_UYVY8_2X8;
> -	case MEDIA_BUS_FMT_UYVY8_2X8:
> -		cspace = V4L2_COLORSPACE_JPEG;
> -	}
> -
> -	ret = ov9640_write_regs(client, mf->width, code, &alts);
> -	if (!ret) {
> -		mf->code	= code;
> -		mf->colorspace	= cspace;
> -	}
> -
> -	return ret;
> +	return ov9640_write_regs(client, mf->width, mf->code, &alts);
>  }
>  
>  static int ov9640_set_fmt(struct v4l2_subdev *sd,
> @@ -539,8 +519,10 @@ static int ov9640_set_fmt(struct v4l2_subdev *sd,
>  		break;
>  	default:
>  		mf->code = MEDIA_BUS_FMT_UYVY8_2X8;
> +		/* fall through */
>  	case MEDIA_BUS_FMT_UYVY8_2X8:
>  		mf->colorspace = V4L2_COLORSPACE_JPEG;
> +		break;

Again, not sure this is a good idea. Otherwise

Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Thanks
Guennadi

>  	}
>  
>  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> -- 
> 2.1.4
> 

Re: [PATCH 8/9] ov9740: avoid calling ov9740_res_roundup() twice

[Guennadi Liakhovetski]

Hi Hans,

On Sun, 3 May 2015, Hans Verkuil wrote:

> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Simplify ov9740_s_fmt.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> Reported-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>  drivers/media/i2c/soc_camera/ov9740.c | 18 +-----------------
>  1 file changed, 1 insertion(+), 17 deletions(-)
> 
> diff --git a/drivers/media/i2c/soc_camera/ov9740.c b/drivers/media/i2c/soc_camera/ov9740.c
> index 03a7fc7..61a8e18 100644
> --- a/drivers/media/i2c/soc_camera/ov9740.c
> +++ b/drivers/media/i2c/soc_camera/ov9740.c
> @@ -673,20 +673,8 @@ static int ov9740_s_fmt(struct v4l2_subdev *sd,
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
>  	struct ov9740_priv *priv = to_ov9740(sd);
> -	enum v4l2_colorspace cspace;
> -	u32 code = mf->code;
>  	int ret;
>  
> -	ov9740_res_roundup(&mf->width, &mf->height);
> -
> -	switch (code) {
> -	case MEDIA_BUS_FMT_YUYV8_2X8:
> -		cspace = V4L2_COLORSPACE_SRGB;
> -		break;
> -	default:
> -		return -EINVAL;
> -	}
> -

ov9740_s_fmt() is also called from ov9740_s_power(), so, don't we have to 
do this simplification the other way round - remove redundant code from 
ov9740_set_fmt() instead?

Thanks
Guennadi

>  	ret = ov9740_reg_write_array(client, ov9740_defaults,
>  				     ARRAY_SIZE(ov9740_defaults));
>  	if (ret < 0)
> @@ -696,11 +684,7 @@ static int ov9740_s_fmt(struct v4l2_subdev *sd,
>  	if (ret < 0)
>  		return ret;
>  
> -	mf->code	= code;
> -	mf->colorspace	= cspace;
> -
> -	memcpy(&priv->current_mf, mf, sizeof(struct v4l2_mbus_framefmt));
> -
> +	priv->current_mf = *mf;
>  	return ret;
>  }
>  
> -- 
> 2.1.4
> 

Re: [PATCH 9/9] mt9t112: initialize left and top

[Guennadi Liakhovetski]

Hi Hans,

On Sun, 3 May 2015, Hans Verkuil wrote:

> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> The left and top variables were uninitialized, leading to unexpected
> results.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/i2c/soc_camera/mt9t112.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/soc_camera/mt9t112.c b/drivers/media/i2c/soc_camera/mt9t112.c
> index de10a76..02190d6 100644
> --- a/drivers/media/i2c/soc_camera/mt9t112.c
> +++ b/drivers/media/i2c/soc_camera/mt9t112.c
> @@ -952,7 +952,8 @@ static int mt9t112_set_fmt(struct v4l2_subdev *sd,
>  	struct v4l2_mbus_framefmt *mf = &format->format;
>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
>  	struct mt9t112_priv *priv = to_mt9t112(client);
> -	unsigned int top, left;
> +	unsigned int top = priv->frame.top;
> +	unsigned int left = priv->frame.left;

I don't think this is needed. We don't care about left and top in 
mt9t112_set_fmt().

How about my comment about a duplicated call to mt9t112_set_params()? Can 
we have it fixed too?

Thanks
Guennadi

>  	int i;
>  
>  	if (format->pad)
> -- 
> 2.1.4
> 

Re: [PATCH 9/9] mt9t112: initialize left and top

[Guennadi Liakhovetski]

On Sun, 3 May 2015, Guennadi Liakhovetski wrote:

> Hi Hans,
> 
> On Sun, 3 May 2015, Hans Verkuil wrote:
> 
> > From: Hans Verkuil <hans.verkuil@cisco.com>
> > 
> > The left and top variables were uninitialized, leading to unexpected
> > results.
> > 
> > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > ---
> >  drivers/media/i2c/soc_camera/mt9t112.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/i2c/soc_camera/mt9t112.c b/drivers/media/i2c/soc_camera/mt9t112.c
> > index de10a76..02190d6 100644
> > --- a/drivers/media/i2c/soc_camera/mt9t112.c
> > +++ b/drivers/media/i2c/soc_camera/mt9t112.c
> > @@ -952,7 +952,8 @@ static int mt9t112_set_fmt(struct v4l2_subdev *sd,
> >  	struct v4l2_mbus_framefmt *mf = &format->format;
> >  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> >  	struct mt9t112_priv *priv = to_mt9t112(client);
> > -	unsigned int top, left;
> > +	unsigned int top = priv->frame.top;
> > +	unsigned int left = priv->frame.left;
> 
> I don't think this is needed. We don't care about left and top in 
> mt9t112_set_fmt().
> 
> How about my comment about a duplicated call to mt9t112_set_params()? Can 
> we have it fixed too?

Sorry, just saw your comment in patch #0:) Np, let's postpone this then.

Thanks
Guennadi

Re: [PATCH 1/9] imx074: don't call imx074_find_datafmt() twice

[Hans Verkuil]

On 05/03/2015 07:54 PM, Guennadi Liakhovetski wrote:
> Hi Hans,
> 
> Thanks for fixing the drivers!
> 
> On Sun, 3 May 2015, Hans Verkuil wrote:
> 
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> Simplify imx074_set_fmt().
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> Reported-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> ---
>>  drivers/media/i2c/soc_camera/imx074.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/i2c/soc_camera/imx074.c b/drivers/media/i2c/soc_camera/imx074.c
>> index f68c235..4226f06 100644
>> --- a/drivers/media/i2c/soc_camera/imx074.c
>> +++ b/drivers/media/i2c/soc_camera/imx074.c
>> @@ -171,8 +171,9 @@ static int imx074_set_fmt(struct v4l2_subdev *sd,
>>  		/* MIPI CSI could have changed the format, double-check */
>>  		if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
>>  			return -EINVAL;
>> -		mf->code	= imx074_colour_fmts[0].code;
>> -		mf->colorspace	= imx074_colour_fmts[0].colorspace;
>> +		fmt = imx074_colour_fmts;
>> +		mf->code = fmt->code;
>> +		mf->colorspace = fmt->colorspace;
> 
> Uhm, why this change? I understand, that this is equivalent code, but (1) 
> is it at all related to the change? and (2) imx074_colour_fmts is an 
> array, so, I'd prefer to keep it as is. I do use pointer arithmetics for 
> array, but then I'd do something like

Ah, I missed the FORMAT_ACTIVE check just before these lines. You are right,
this change is not needed.

Regards,

	Hans

> 
> +		fmt = imx074_colour_fmts + 0;
> +		mf->code = fmt->code;
> +		mf->colorspace = fmt->colorspace;
> 
> which looks silly:) And then - even more importantly - you overwrite the 
> fmt variable, which is then used below instead of calling 
> imx074_find_datafmt() again. So, now you assign a (theoretically) 
> different value to priv->fmt. I know that array only has one element and 
> imx074_find_datafmt() will anyway just return it, but, I don't see why 
> this change is needed?
> 
> Thanks
> Guennadi
> 
>>  	}
>>  
>>  	mf->width	= IMX074_WIDTH;
>> @@ -180,7 +181,7 @@ static int imx074_set_fmt(struct v4l2_subdev *sd,
>>  	mf->field	= V4L2_FIELD_NONE;
>>  
>>  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
>> -		priv->fmt = imx074_find_datafmt(mf->code);
>> +		priv->fmt = fmt;
>>  	else
>>  		cfg->try_fmt = *mf;
>>  
>> -- 
>> 2.1.4
>>

Re: [PATCH 4/9] ov2640: avoid calling ov2640_select_win() twice

[Hans Verkuil]

On 05/03/2015 08:19 PM, Guennadi Liakhovetski wrote:
> Hi Hans,
> 
> On Sun, 3 May 2015, Hans Verkuil wrote:
> 
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> Simplify ov2640_set_params and ov2640_set_fmt.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> Reported-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> ---
>>  drivers/media/i2c/soc_camera/ov2640.c | 21 ++++++++++-----------
>>  1 file changed, 10 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/media/i2c/soc_camera/ov2640.c b/drivers/media/i2c/soc_camera/ov2640.c
>> index 9b4f5de..5dcaf24 100644
>> --- a/drivers/media/i2c/soc_camera/ov2640.c
>> +++ b/drivers/media/i2c/soc_camera/ov2640.c
>> @@ -769,15 +769,15 @@ static const struct ov2640_win_size *ov2640_select_win(u32 *width, u32 *height)
>>  	return &ov2640_supported_win_sizes[default_size];
>>  }
>>  
>> -static int ov2640_set_params(struct i2c_client *client, u32 *width, u32 *height,
>> -			     u32 code)
>> +static int ov2640_set_params(struct i2c_client *client,
>> +			     const struct ov2640_win_size *win, u32 code)
>>  {
>>  	struct ov2640_priv       *priv = to_ov2640(client);
>>  	const struct regval_list *selected_cfmt_regs;
>>  	int ret;
>>  
>>  	/* select win */
>> -	priv->win = ov2640_select_win(width, height);
>> +	priv->win = win;
>>  
>>  	/* select format */
>>  	priv->cfmt_code = 0;
>> @@ -798,6 +798,7 @@ static int ov2640_set_params(struct i2c_client *client, u32 *width, u32 *height,
>>  	case MEDIA_BUS_FMT_UYVY8_2X8:
>>  		dev_dbg(&client->dev, "%s: Selected cfmt UYVY", __func__);
>>  		selected_cfmt_regs = ov2640_uyvy_regs;
>> +		break;
> 
> Hm, IIRC, some versions of gcc complain like "break at the end of a switch 
> statement is deprecated." Why did you add this at two locations? Are you 
> seeing a warning? If not, maybe better not do that.

I have never seen such a warning in 20 odd years of using gcc. It is bad practice
not to add a break at the end in case someone adds a new case later or shuffles
cases around and misses that there was no break.

And since 99% of all switch statements in the kernel have a break at the end,
I would say that any gcc issues with that would have been spotted ages ago.

Regards,

	Hans

> 
> Otherwise
> 
> Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> 
> Thanks
> Guennadi
> 
>>  	}
>>  
>>  	/* reset hardware */
>> @@ -832,8 +833,6 @@ static int ov2640_set_params(struct i2c_client *client, u32 *width, u32 *height,
>>  		goto err;
>>  
>>  	priv->cfmt_code = code;
>> -	*width = priv->win->width;
>> -	*height = priv->win->height;
>>  
>>  	return 0;
>>  
>> @@ -887,14 +886,13 @@ static int ov2640_set_fmt(struct v4l2_subdev *sd,
>>  {
>>  	struct v4l2_mbus_framefmt *mf = &format->format;
>>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +	const struct ov2640_win_size *win;
>>  
>>  	if (format->pad)
>>  		return -EINVAL;
>>  
>> -	/*
>> -	 * select suitable win, but don't store it
>> -	 */
>> -	ov2640_select_win(&mf->width, &mf->height);
>> +	/* select suitable win */
>> +	win = ov2640_select_win(&mf->width, &mf->height);
>>  
>>  	mf->field	= V4L2_FIELD_NONE;
>>  
>> @@ -905,14 +903,15 @@ static int ov2640_set_fmt(struct v4l2_subdev *sd,
>>  		break;
>>  	default:
>>  		mf->code = MEDIA_BUS_FMT_UYVY8_2X8;
>> +		/* fall through */
>>  	case MEDIA_BUS_FMT_YUYV8_2X8:
>>  	case MEDIA_BUS_FMT_UYVY8_2X8:
>>  		mf->colorspace = V4L2_COLORSPACE_JPEG;
>> +		break;
>>  	}
>>  
>>  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
>> -		return ov2640_set_params(client, &mf->width,
>> -					 &mf->height, mf->code);
>> +		return ov2640_set_params(client, win, mf->code);
>>  	cfg->try_fmt = *mf;
>>  	return 0;
>>  }
>> -- 
>> 2.1.4
>>

Re: [PATCH 5/9] ov5642: avoid calling ov5642_find_datafmt() twice

[Hans Verkuil]

On 05/03/2015 10:24 PM, Guennadi Liakhovetski wrote:
> Hi Hans,
> 
> On Sun, 3 May 2015, Hans Verkuil wrote:
> 
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> Simplify ov5642_set_fmt().
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> Reported-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> ---
>>  drivers/media/i2c/soc_camera/ov5642.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/i2c/soc_camera/ov5642.c b/drivers/media/i2c/soc_camera/ov5642.c
>> index bab9ac0..061fca3 100644
>> --- a/drivers/media/i2c/soc_camera/ov5642.c
>> +++ b/drivers/media/i2c/soc_camera/ov5642.c
>> @@ -804,14 +804,15 @@ static int ov5642_set_fmt(struct v4l2_subdev *sd,
>>  	if (!fmt) {
>>  		if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
>>  			return -EINVAL;
>> -		mf->code	= ov5642_colour_fmts[0].code;
>> -		mf->colorspace	= ov5642_colour_fmts[0].colorspace;
>> +		fmt = ov5642_colour_fmts;
>> +		mf->code = fmt->code;
>> +		mf->colorspace = fmt->colorspace;
> 
> Again - I still don't see why this is needed.

Same thing, missed the if statement just before these lines.

Will fix.

	Hans

> 
> Thanks
> Guennadi
> 
>>  	}
>>  
>>  	mf->field	= V4L2_FIELD_NONE;
>>  
>>  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
>> -		priv->fmt = ov5642_find_datafmt(mf->code);
>> +		priv->fmt = fmt;
>>  	else
>>  		cfg->try_fmt = *mf;
>>  	return 0;
>> -- 
>> 2.1.4
>>

Re: [PATCH 9/9] mt9t112: initialize left and top

[Hans Verkuil]

On 05/03/2015 11:02 PM, Guennadi Liakhovetski wrote:
> Hi Hans,
> 
> On Sun, 3 May 2015, Hans Verkuil wrote:
> 
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> The left and top variables were uninitialized, leading to unexpected
>> results.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>  drivers/media/i2c/soc_camera/mt9t112.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/i2c/soc_camera/mt9t112.c b/drivers/media/i2c/soc_camera/mt9t112.c
>> index de10a76..02190d6 100644
>> --- a/drivers/media/i2c/soc_camera/mt9t112.c
>> +++ b/drivers/media/i2c/soc_camera/mt9t112.c
>> @@ -952,7 +952,8 @@ static int mt9t112_set_fmt(struct v4l2_subdev *sd,
>>  	struct v4l2_mbus_framefmt *mf = &format->format;
>>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
>>  	struct mt9t112_priv *priv = to_mt9t112(client);
>> -	unsigned int top, left;
>> +	unsigned int top = priv->frame.top;
>> +	unsigned int left = priv->frame.left;
> 
> I don't think this is needed. We don't care about left and top in 
> mt9t112_set_fmt().

On further analysis you are correct, it will work with random left/top
values. But I think it is 1) very unexpected and 2) bad form to leave it
with random values.

I prefer to keep this patch, unless you disagree.

Regards,

	Hans

> 
> How about my comment about a duplicated call to mt9t112_set_params()? Can 
> we have it fixed too?
> 
> Thanks
> Guennadi
> 
>>  	int i;
>>  
>>  	if (format->pad)
>> -- 
>> 2.1.4
>>

Re: [PATCH 9/9] mt9t112: initialize left and top

[Guennadi Liakhovetski]

On Mon, 4 May 2015, Hans Verkuil wrote:

> On 05/03/2015 11:02 PM, Guennadi Liakhovetski wrote:
> > Hi Hans,
> > 
> > On Sun, 3 May 2015, Hans Verkuil wrote:
> > 
> >> From: Hans Verkuil <hans.verkuil@cisco.com>
> >>
> >> The left and top variables were uninitialized, leading to unexpected
> >> results.
> >>
> >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >> ---
> >>  drivers/media/i2c/soc_camera/mt9t112.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/media/i2c/soc_camera/mt9t112.c b/drivers/media/i2c/soc_camera/mt9t112.c
> >> index de10a76..02190d6 100644
> >> --- a/drivers/media/i2c/soc_camera/mt9t112.c
> >> +++ b/drivers/media/i2c/soc_camera/mt9t112.c
> >> @@ -952,7 +952,8 @@ static int mt9t112_set_fmt(struct v4l2_subdev *sd,
> >>  	struct v4l2_mbus_framefmt *mf = &format->format;
> >>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> >>  	struct mt9t112_priv *priv = to_mt9t112(client);
> >> -	unsigned int top, left;
> >> +	unsigned int top = priv->frame.top;
> >> +	unsigned int left = priv->frame.left;
> > 
> > I don't think this is needed. We don't care about left and top in 
> > mt9t112_set_fmt().
> 
> On further analysis you are correct, it will work with random left/top
> values. But I think it is 1) very unexpected and 2) bad form to leave it
> with random values.
> 
> I prefer to keep this patch, unless you disagree.

Sorry, but I do. Assigning those specific values to left and top makes the 
code even more confusing, it makes it look like that makes any sense, 
whereas it doesn't. If anything we can add a comment there. Or we can pass 
NULL and make sure to catch it somewhere down the line.

Thanks
Guennadi

> Regards,
> 
> 	Hans
> 
> > 
> > How about my comment about a duplicated call to mt9t112_set_params()? Can 
> > we have it fixed too?
> > 
> > Thanks
> > Guennadi
> > 
> >>  	int i;
> >>  
> >>  	if (format->pad)
> >> -- 
> >> 2.1.4
> >>
> 

Re: [PATCH 9/9] mt9t112: initialize left and top

[Hans Verkuil]

On 05/04/2015 09:40 AM, Guennadi Liakhovetski wrote:
> On Mon, 4 May 2015, Hans Verkuil wrote:
> 
>> On 05/03/2015 11:02 PM, Guennadi Liakhovetski wrote:
>>> Hi Hans,
>>>
>>> On Sun, 3 May 2015, Hans Verkuil wrote:
>>>
>>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>>
>>>> The left and top variables were uninitialized, leading to unexpected
>>>> results.
>>>>
>>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>>> ---
>>>>  drivers/media/i2c/soc_camera/mt9t112.c | 3 ++-
>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/media/i2c/soc_camera/mt9t112.c b/drivers/media/i2c/soc_camera/mt9t112.c
>>>> index de10a76..02190d6 100644
>>>> --- a/drivers/media/i2c/soc_camera/mt9t112.c
>>>> +++ b/drivers/media/i2c/soc_camera/mt9t112.c
>>>> @@ -952,7 +952,8 @@ static int mt9t112_set_fmt(struct v4l2_subdev *sd,
>>>>  	struct v4l2_mbus_framefmt *mf = &format->format;
>>>>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
>>>>  	struct mt9t112_priv *priv = to_mt9t112(client);
>>>> -	unsigned int top, left;
>>>> +	unsigned int top = priv->frame.top;
>>>> +	unsigned int left = priv->frame.left;
>>>
>>> I don't think this is needed. We don't care about left and top in 
>>> mt9t112_set_fmt().
>>
>> On further analysis you are correct, it will work with random left/top
>> values. But I think it is 1) very unexpected and 2) bad form to leave it
>> with random values.
>>
>> I prefer to keep this patch, unless you disagree.
> 
> Sorry, but I do. Assigning those specific values to left and top makes the 
> code even more confusing, it makes it look like that makes any sense, 
> whereas it doesn't. If anything we can add a comment there. Or we can pass 
> NULL and make sure to catch it somewhere down the line.
> 

What about this:

	unsigned int top = 0; /* don't care */
	unsigned int left = 0; /* don't care */

Regards,

	Hans

> Thanks
> Guennadi
> 
>> Regards,
>>
>> 	Hans
>>
>>>
>>> How about my comment about a duplicated call to mt9t112_set_params()? Can 
>>> we have it fixed too?
>>>
>>> Thanks
>>> Guennadi
>>>
>>>>  	int i;
>>>>  
>>>>  	if (format->pad)
>>>> -- 
>>>> 2.1.4
>>>>
>>

Re: [PATCH 9/9] mt9t112: initialize left and top

[Guennadi Liakhovetski]

On Mon, 4 May 2015, Hans Verkuil wrote:

> On 05/04/2015 09:40 AM, Guennadi Liakhovetski wrote:
> > On Mon, 4 May 2015, Hans Verkuil wrote:
> > 
> >> On 05/03/2015 11:02 PM, Guennadi Liakhovetski wrote:
> >>> Hi Hans,
> >>>
> >>> On Sun, 3 May 2015, Hans Verkuil wrote:
> >>>
> >>>> From: Hans Verkuil <hans.verkuil@cisco.com>
> >>>>
> >>>> The left and top variables were uninitialized, leading to unexpected
> >>>> results.
> >>>>
> >>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >>>> ---
> >>>>  drivers/media/i2c/soc_camera/mt9t112.c | 3 ++-
> >>>>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/media/i2c/soc_camera/mt9t112.c b/drivers/media/i2c/soc_camera/mt9t112.c
> >>>> index de10a76..02190d6 100644
> >>>> --- a/drivers/media/i2c/soc_camera/mt9t112.c
> >>>> +++ b/drivers/media/i2c/soc_camera/mt9t112.c
> >>>> @@ -952,7 +952,8 @@ static int mt9t112_set_fmt(struct v4l2_subdev *sd,
> >>>>  	struct v4l2_mbus_framefmt *mf = &format->format;
> >>>>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> >>>>  	struct mt9t112_priv *priv = to_mt9t112(client);
> >>>> -	unsigned int top, left;
> >>>> +	unsigned int top = priv->frame.top;
> >>>> +	unsigned int left = priv->frame.left;
> >>>
> >>> I don't think this is needed. We don't care about left and top in 
> >>> mt9t112_set_fmt().
> >>
> >> On further analysis you are correct, it will work with random left/top
> >> values. But I think it is 1) very unexpected and 2) bad form to leave it
> >> with random values.
> >>
> >> I prefer to keep this patch, unless you disagree.
> > 
> > Sorry, but I do. Assigning those specific values to left and top makes the 
> > code even more confusing, it makes it look like that makes any sense, 
> > whereas it doesn't. If anything we can add a comment there. Or we can pass 
> > NULL and make sure to catch it somewhere down the line.
> > 
> 
> What about this:
> 
> 	unsigned int top = 0; /* don't care */
> 	unsigned int left = 0; /* don't care */

This would be my third preference. My first preference is just a comment. 
My second preference is adding

	if (!start)
		return;

in the middle of soc_camera_limit_side() and using NULL in 
mt9t112_set_fmt(). I really dislike meaningless operations, also when they 
fix compiler warnings, but here even the compiler is happy:)

Thanks
Guennadi

> >>>>  	int i;
> >>>>  
> >>>>  	if (format->pad)
> >>>> -- 
> >>>> 2.1.4
> >>>>
> >>
> 

Re: [PATCH 8/9] ov9740: avoid calling ov9740_res_roundup() twice

[Hans Verkuil]

On 05/03/2015 10:47 PM, Guennadi Liakhovetski wrote:
> Hi Hans,
> 
> On Sun, 3 May 2015, Hans Verkuil wrote:
> 
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> Simplify ov9740_s_fmt.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> Reported-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> ---
>>  drivers/media/i2c/soc_camera/ov9740.c | 18 +-----------------
>>  1 file changed, 1 insertion(+), 17 deletions(-)
>>
>> diff --git a/drivers/media/i2c/soc_camera/ov9740.c b/drivers/media/i2c/soc_camera/ov9740.c
>> index 03a7fc7..61a8e18 100644
>> --- a/drivers/media/i2c/soc_camera/ov9740.c
>> +++ b/drivers/media/i2c/soc_camera/ov9740.c
>> @@ -673,20 +673,8 @@ static int ov9740_s_fmt(struct v4l2_subdev *sd,
>>  {
>>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
>>  	struct ov9740_priv *priv = to_ov9740(sd);
>> -	enum v4l2_colorspace cspace;
>> -	u32 code = mf->code;
>>  	int ret;
>>  
>> -	ov9740_res_roundup(&mf->width, &mf->height);
>> -
>> -	switch (code) {
>> -	case MEDIA_BUS_FMT_YUYV8_2X8:
>> -		cspace = V4L2_COLORSPACE_SRGB;
>> -		break;
>> -	default:
>> -		return -EINVAL;
>> -	}
>> -
> 
> ov9740_s_fmt() is also called from ov9740_s_power(), so, don't we have to 
> do this simplification the other way round - remove redundant code from 
> ov9740_set_fmt() instead?

Yes, but s_power is also calling ov9740_res_roundup() and it sets mf->code and
mf->colorspace to the correct values. So this code in s_fmt is a duplicate
for both s_power and for set_fmt. It can't be removed from set_fmt either
since it is needed for the TRY_FORMAT case anyway.

So IMHO it makes sense to remove it from s_fmt.

Regards,

	Hans

> 
> Thanks
> Guennadi
> 
>>  	ret = ov9740_reg_write_array(client, ov9740_defaults,
>>  				     ARRAY_SIZE(ov9740_defaults));
>>  	if (ret < 0)
>> @@ -696,11 +684,7 @@ static int ov9740_s_fmt(struct v4l2_subdev *sd,
>>  	if (ret < 0)
>>  		return ret;
>>  
>> -	mf->code	= code;
>> -	mf->colorspace	= cspace;
>> -
>> -	memcpy(&priv->current_mf, mf, sizeof(struct v4l2_mbus_framefmt));
>> -
>> +	priv->current_mf = *mf;
>>  	return ret;
>>  }
>>  
>> -- 
>> 2.1.4
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>