Re: [PATCH 1/2 v2] media: Add support for arbitrary resolution for the ov5642 camera driver

Hi Bastian,

Guennadi pointed out that "should" can sound a bit harsh, so please read my 
reviews as if

#define "you should" "I think you should"

was prepended to all of them :-)

On Wednesday 31 August 2011 19:06:25 Laurent Pinchart wrote:

On Wednesday 31 August 2011 17:05:52 Bastian Hecht wrote:

quoted

This patch adds the ability to get arbitrary resolutions with a width
up to 2592 and a height up to 720 pixels instead of the standard 1280x720
only.

Signed-off-by: Bastian Hecht <redacted>
---

diff --git a/drivers/media/video/ov5642.c b/drivers/media/video/ov5642.c
index 6410bda..87b432e 100644
--- a/drivers/media/video/ov5642.c
+++ b/drivers/media/video/ov5642.c

[snip]

quoted

@@ -684,107 +737,101 @@ static int ov5642_write_array(struct i2c_client

[snip]

quoted

-static int ov5642_s_fmt(struct v4l2_subdev *sd,
-			struct v4l2_mbus_framefmt *mf)
+static int ov5642_s_fmt(struct v4l2_subdev *sd, struct
v4l2_mbus_framefmt *mf) {

 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	struct ov5642 *priv = to_ov5642(client);

-
-	dev_dbg(sd->v4l2_dev->dev, "%s(%u)\n", __func__, mf->code);
+	int ret;

 	/* MIPI CSI could have changed the format, double-check */
 	if (!ov5642_find_datafmt(mf->code))
 	
 		return -EINVAL;
 	
 	ov5642_try_fmt(sd, mf);

-

 	priv->fmt = ov5642_find_datafmt(mf->code);

-	ov5642_write_array(client, ov5642_default_regs_init);
-	ov5642_set_resolution(client);
-	ov5642_write_array(client, ov5642_default_regs_finalise);
+	ret = ov5642_write_array(client, ov5642_default_regs_init);
+	if (!ret)
+		ret = ov5642_set_resolution(sd);
+	if (!ret)
+		ret = ov5642_write_array(client, ov5642_default_regs_finalise);

You shouldn't write anything to the sensor here. As only .s_crop can
currently change the format, .s_fmt should just return the current format
without performing any change or writing anything to the device.

quoted

-	return 0;
+	return ret;

 }

[snip]

quoted

@@ -827,15 +874,42 @@ static int ov5642_g_chip_ident(struct v4l2_subdev

[snip]

quoted

 static int ov5642_g_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
 {

+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct ov5642 *priv = to_ov5642(client);

 	struct v4l2_rect *rect = &a->c;

-	a->type		= V4L2_BUF_TYPE_VIDEO_CAPTURE;
-	rect->top	= 0;
-	rect->left	= 0;
-	rect->width	= OV5642_WIDTH;
-	rect->height	= OV5642_HEIGHT;
+	a->type	= V4L2_BUF_TYPE_VIDEO_CAPTURE;

Shouldn't you return an error instead when a->type is not
V4L2_BUF_TYPE_VIDEO_CAPTURE ?

quoted

+	*rect = priv->crop_rect;

 	return 0;
 
 }

-- 
Regards,

Laurent Pinchart