Hi Laurent

On Sun, 28 Aug 2011, Laurent Pinchart wrote:

[snip]

quoted

@@ -593,8 +639,7 @@ static struct ov5642 *to_ov5642(const struct i2c_client

*client) }

 /* Find a data format by a pixel code in an array */
-static const struct ov5642_datafmt
-			*ov5642_find_datafmt(enum v4l2_mbus_pixelcode code)
+static const struct ov5642_datafmt *ov5642_find_datafmt(enum
v4l2_mbus_pixelcode code) {

checkpatch.pl won't be happy.

Since the lift of the hard 80-char limit, I often find lines of 86 characters 
more acceptable than their split versions.

[snip]

quoted

@@ -774,17 +839,27 @@ static int ov5642_s_fmt(struct v4l2_subdev *sd,

 	ov5642_try_fmt(sd, mf);

+	priv->out_size.width		= mf->width;
+	priv->out_size.height		= mf->height;

It looks like to me (but I may be wrong) that you achieve different 
resolutions using cropping, not scaling. If that's correct you should 
implement s_crop support and refuse changing the resolution through s_fmt.

As the patch explains (I think) on several occasions, currently only the 
1:1 scale is supported, and it was our deliberate choice to implement this 
using the scaling API

quoted

@@ -793,10 +868,12 @@ static int ov5642_g_fmt(struct v4l2_subdev *sd,

 	mf->code	= fmt->code;
 	mf->colorspace	= fmt->colorspace;
-	mf->width	= OV5642_WIDTH;
-	mf->height	= OV5642_HEIGHT;
+	mf->width	= priv->out_size.width;
+	mf->height	= priv->out_size.height;
 	mf->field	= V4L2_FIELD_NONE;

+	dev_dbg(sd->v4l2_dev->dev, "%s return width: %u heigth: %u\n", __func__,
+			mf->width, mf->height);

Isn't that a bit too verbose ? Printing the format in a debug message in the 
s_fmt handler is useful, but maybe doing it in g_fmt is a bit too much.

This is a dev_dbg()... Personally, as long as they don't clutter the source 
code needlessly, compile without warnings and have their typos fixed (;-)) 
I don't have problems with an odd instance, even if I don't really perceive 
its output as particularly useful:-)

[snip]

quoted

+	dev_dbg(sd->v4l2_dev->dev, "%s crop width: %u heigth: %u\n", __func__,
+			rect->width, rect->height);

Same comment as for g_fmt.

same reply:-)

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