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

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

[Bastian Hecht]

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 <hechtb@gmail.com>
---
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
@@ -14,8 +14,10 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/bitops.h>
 #include <linux/delay.h>
 #include <linux/i2c.h>
+#include <linux/kernel.h>
 #include <linux/slab.h>
 #include <linux/videodev2.h>
 
@@ -34,7 +36,7 @@
 #define REG_WINDOW_START_Y_LOW		0x3803
 #define REG_WINDOW_WIDTH_HIGH		0x3804
 #define REG_WINDOW_WIDTH_LOW		0x3805
-#define REG_WINDOW_HEIGHT_HIGH 		0x3806
+#define REG_WINDOW_HEIGHT_HIGH		0x3806
 #define REG_WINDOW_HEIGHT_LOW		0x3807
 #define REG_OUT_WIDTH_HIGH		0x3808
 #define REG_OUT_WIDTH_LOW		0x3809
@@ -44,19 +46,44 @@
 #define REG_OUT_TOTAL_WIDTH_LOW		0x380d
 #define REG_OUT_TOTAL_HEIGHT_HIGH	0x380e
 #define REG_OUT_TOTAL_HEIGHT_LOW	0x380f
+#define REG_OUTPUT_FORMAT		0x4300
+#define REG_ISP_CTRL_01			0x5001
+#define REG_AVG_WINDOW_END_X_HIGH	0x5682
+#define REG_AVG_WINDOW_END_X_LOW	0x5683
+#define REG_AVG_WINDOW_END_Y_HIGH	0x5686
+#define REG_AVG_WINDOW_END_Y_LOW	0x5687
+
+/* active pixel array size */
+#define OV5642_SENSOR_SIZE_X	2592
+#define OV5642_SENSOR_SIZE_Y	1944
 
 /*
- * define standard resolution.
- * Works currently only for up to 720 lines
- * eg. 320x240, 640x480, 800x600, 1280x720, 2048x720
+ * About OV5642 resolution, cropping and binning:
+ * This sensor supports it all, at least in the feature description.
+ * Unfortunately, no combination of appropriate registers settings could make
+ * the chip work the intended way. As it works with predefined register lists,
+ * some undocumented registers are presumably changed there to achieve their
+ * goals.
+ * This driver currently only works for resolutions up to 720 lines with a
+ * 1:1 scale. Hopefully these restrictions will be removed in the future.
  */
+#define OV5642_MAX_WIDTH	OV5642_SENSOR_SIZE_X
+#define OV5642_MAX_HEIGHT	720
 
-#define OV5642_WIDTH		1280
-#define OV5642_HEIGHT		720
-#define OV5642_TOTAL_WIDTH	3200
-#define OV5642_TOTAL_HEIGHT	2000
-#define OV5642_SENSOR_SIZE_X	2592
-#define OV5642_SENSOR_SIZE_Y	1944
+/* default sizes */
+#define OV5642_DEFAULT_WIDTH	1280
+#define OV5642_DEFAULT_HEIGHT	OV5642_MAX_HEIGHT
+
+/* minimum extra blanking */
+#define BLANKING_EXTRA_WIDTH		500
+#define BLANKING_EXTRA_HEIGHT		20
+
+/*
+ * the sensor's autoexposure is buggy when setting total_height low.
+ * It tries to expose longer than 1 frame period without taking care of it
+ * and this leads to weird output. So we set 1000 lines as minimum.
+ */
+#define BLANKING_MIN_HEIGHT		1000
 
 struct regval_list {
 	u16 reg_num;
@@ -578,9 +605,20 @@ struct ov5642_datafmt {
 	enum v4l2_colorspace		colorspace;
 };
 
+/* the output resolution and blanking information */
+struct ov5642_out_size {
+	int width;
+	int height;
+	int total_width;
+	int total_height;
+};
+
 struct ov5642 {
 	struct v4l2_subdev		subdev;
+
 	const struct ov5642_datafmt	*fmt;
+	struct v4l2_rect                crop_rect;
+	struct ov5642_out_size		out_size;
 };
 
 static const struct ov5642_datafmt ov5642_colour_fmts[] = {
@@ -641,6 +679,21 @@ static int reg_write(struct i2c_client *client, u16 reg, u8 val)
 
 	return 0;
 }
+
+/*
+ * convenience function to write 16 bit register values that are split up
+ * into two consecutive high and low parts
+ */
+static int reg_write16(struct i2c_client *client, u16 reg, u16 val16)
+{
+	int ret;
+
+	ret = reg_write(client, reg, val16 >> 8);
+	if (ret)
+		return ret;
+	return reg_write(client, reg + 1, val16 & 0x00ff);
+}
+
 #ifdef CONFIG_VIDEO_ADV_DEBUG
 static int ov5642_get_register(struct v4l2_subdev *sd, struct v4l2_dbg_register *reg)
 {
@@ -684,107 +737,101 @@ static int ov5642_write_array(struct i2c_client *client,
 	return 0;
 }
 
-static int ov5642_set_resolution(struct i2c_client *client)
+static int ov5642_set_resolution(struct v4l2_subdev *sd)
 {
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct ov5642 *priv = to_ov5642(client);
+	int width = priv->out_size.width;
+	int height = priv->out_size.height;
+	int total_width = priv->out_size.total_width;
+	int total_height = priv->out_size.total_height;
+	int start_x = (OV5642_SENSOR_SIZE_X - width) / 2;
+	int start_y = (OV5642_SENSOR_SIZE_Y - height) / 2;
 	int ret;
-	u8 start_x_high = ((OV5642_SENSOR_SIZE_X - OV5642_WIDTH) / 2) >> 8;
-	u8 start_x_low  = ((OV5642_SENSOR_SIZE_X - OV5642_WIDTH) / 2) & 0xff;
-	u8 start_y_high = ((OV5642_SENSOR_SIZE_Y - OV5642_HEIGHT) / 2) >> 8;
-	u8 start_y_low  = ((OV5642_SENSOR_SIZE_Y - OV5642_HEIGHT) / 2) & 0xff;
-
-	u8 width_high	= OV5642_WIDTH  >> 8;
-	u8 width_low	= OV5642_WIDTH  & 0xff;
-	u8 height_high	= OV5642_HEIGHT >> 8;
-	u8 height_low	= OV5642_HEIGHT & 0xff;
-
-	u8 total_width_high  = OV5642_TOTAL_WIDTH  >> 8;
-	u8 total_width_low   = OV5642_TOTAL_WIDTH  & 0xff;
-	u8 total_height_high = OV5642_TOTAL_HEIGHT >> 8;
-	u8 total_height_low  = OV5642_TOTAL_HEIGHT & 0xff;
-
-	ret = reg_write(client, REG_WINDOW_START_X_HIGH, start_x_high);
-	if (!ret)
-		ret = reg_write(client, REG_WINDOW_START_X_LOW, start_x_low);
-	if (!ret)
-		ret = reg_write(client, REG_WINDOW_START_Y_HIGH, start_y_high);
-	if (!ret)
-		ret = reg_write(client, REG_WINDOW_START_Y_LOW, start_y_low);
 
+	/* 
+	 * This should set the starting point for cropping.
+	 * Doesn't work so far.
+	 */ 
+	ret = reg_write16(client, REG_WINDOW_START_X_HIGH, start_x);
 	if (!ret)
-		ret = reg_write(client, REG_WINDOW_WIDTH_HIGH, width_high);
-	if (!ret)
-		ret = reg_write(client, REG_WINDOW_WIDTH_LOW , width_low);
-	if (!ret)
-		ret = reg_write(client, REG_WINDOW_HEIGHT_HIGH, height_high);
-	if (!ret)
-		ret = reg_write(client, REG_WINDOW_HEIGHT_LOW,  height_low);
+		ret = reg_write16(client, REG_WINDOW_START_Y_HIGH, start_y);
+	if (!ret) {
+		priv->crop_rect.left = start_x;
+		priv->crop_rect.top = start_y;
+	}
 
 	if (!ret)
-		ret = reg_write(client, REG_OUT_WIDTH_HIGH, width_high);
-	if (!ret)
-		ret = reg_write(client, REG_OUT_WIDTH_LOW , width_low);
+		ret = reg_write16(client, REG_WINDOW_WIDTH_HIGH, width);
 	if (!ret)
-		ret = reg_write(client, REG_OUT_HEIGHT_HIGH, height_high);
+		ret = reg_write16(client, REG_WINDOW_HEIGHT_HIGH, height);
+	if (ret)
+		return ret;
+	priv->crop_rect.width = width;
+	priv->crop_rect.height = height;
+
+	/* Set the output window size. Only 1:1 scale is supported so far. */
+	ret = reg_write16(client, REG_OUT_WIDTH_HIGH, width);
 	if (!ret)
-		ret = reg_write(client, REG_OUT_HEIGHT_LOW,  height_low);
+		ret = reg_write16(client, REG_OUT_HEIGHT_HIGH, height);
 
+	/* Total width = output size + blanking */
 	if (!ret)
-		ret = reg_write(client, REG_OUT_TOTAL_WIDTH_HIGH, total_width_high);
+		ret = reg_write16(client, REG_OUT_TOTAL_WIDTH_HIGH, total_width);
 	if (!ret)
-		ret = reg_write(client, REG_OUT_TOTAL_WIDTH_LOW, total_width_low);
+		ret = reg_write16(client, REG_OUT_TOTAL_HEIGHT_HIGH, total_height);
+
+	/* Sets the window for AWB calculations */
 	if (!ret)
-		ret = reg_write(client, REG_OUT_TOTAL_HEIGHT_HIGH, total_height_high);
+		ret = reg_write16(client, REG_AVG_WINDOW_END_X_HIGH, width);
 	if (!ret)
-		ret = reg_write(client, REG_OUT_TOTAL_HEIGHT_LOW,  total_height_low);
+		ret = reg_write16(client, REG_AVG_WINDOW_END_Y_HIGH, height);
 
 	return ret;
 }
 
-static int ov5642_try_fmt(struct v4l2_subdev *sd,
-			  struct v4l2_mbus_framefmt *mf)
+static int ov5642_try_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf)
 {
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct ov5642 *priv = to_ov5642(client);
 	const struct ov5642_datafmt *fmt = ov5642_find_datafmt(mf->code);
 
-	dev_dbg(sd->v4l2_dev->dev, "%s(%u) width: %u heigth: %u\n",
-			__func__, mf->code, mf->width, mf->height);
+	mf->width = priv->out_size.width;
+	mf->height = priv->out_size.height;
 
 	if (!fmt) {
 		mf->code	= ov5642_colour_fmts[0].code;
 		mf->colorspace	= ov5642_colour_fmts[0].colorspace;
 	}
 
-	mf->width	= OV5642_WIDTH;
-	mf->height	= OV5642_HEIGHT;
 	mf->field	= V4L2_FIELD_NONE;
 
 	return 0;
 }
 
-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);
 
-	return 0;
+	return ret;
 }
 
-static int ov5642_g_fmt(struct v4l2_subdev *sd,
-			struct v4l2_mbus_framefmt *mf)
+static int ov5642_g_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	struct ov5642 *priv = to_ov5642(client);
@@ -793,8 +840,8 @@ 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;
 
 	return 0;
@@ -827,15 +874,42 @@ static int ov5642_g_chip_ident(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int ov5642_s_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;
+	int ret;
+
+	v4l_bound_align_image(&rect->width, 48, OV5642_MAX_WIDTH, 1,
+			      &rect->height, 32, OV5642_MAX_HEIGHT, 1, 0);
+
+	priv->out_size.width		= rect->width;
+	priv->out_size.height		= rect->height;
+	priv->out_size.total_width	= rect->width + BLANKING_EXTRA_WIDTH;
+	priv->out_size.total_height	= max_t(int, rect->height +
+							BLANKING_EXTRA_HEIGHT,
+							BLANKING_MIN_HEIGHT);
+	priv->crop_rect.width		= rect->width;
+	priv->crop_rect.height		= rect->height;
+
+	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);
+
+	return ret;
+}
+
 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;
+	*rect = priv->crop_rect;
 
 	return 0;
 }
@@ -844,8 +918,8 @@ static int ov5642_cropcap(struct v4l2_subdev *sd, struct v4l2_cropcap *a)
 {
 	a->bounds.left			= 0;
 	a->bounds.top			= 0;
-	a->bounds.width			= OV5642_WIDTH;
-	a->bounds.height		= OV5642_HEIGHT;
+	a->bounds.width			= OV5642_MAX_WIDTH;
+	a->bounds.height		= OV5642_MAX_HEIGHT;
 	a->defrect			= a->bounds;
 	a->type				= V4L2_BUF_TYPE_VIDEO_CAPTURE;
 	a->pixelaspect.numerator	= 1;
@@ -858,9 +932,8 @@ static int ov5642_g_mbus_config(struct v4l2_subdev *sd,
 				struct v4l2_mbus_config *cfg)
 {
 	cfg->type = V4L2_MBUS_CSI2;
-	cfg->flags = V4L2_MBUS_CSI2_2_LANE |
-		V4L2_MBUS_CSI2_CHANNEL_0 |
-		V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
+	cfg->flags = V4L2_MBUS_CSI2_2_LANE | V4L2_MBUS_CSI2_CHANNEL_0 |
+					V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
 
 	return 0;
 }
@@ -870,6 +943,7 @@ static struct v4l2_subdev_video_ops ov5642_subdev_video_ops = {
 	.g_mbus_fmt	= ov5642_g_fmt,
 	.try_mbus_fmt	= ov5642_try_fmt,
 	.enum_mbus_fmt	= ov5642_enum_fmt,
+	.s_crop		= ov5642_s_crop,
 	.g_crop		= ov5642_g_crop,
 	.cropcap	= ov5642_cropcap,
 	.g_mbus_config	= ov5642_g_mbus_config,
@@ -941,8 +1015,17 @@ static int ov5642_probe(struct i2c_client *client,
 
 	v4l2_i2c_subdev_init(&priv->subdev, client, &ov5642_subdev_ops);
 
-	icd->ops	= NULL;
-	priv->fmt	= &ov5642_colour_fmts[0];
+	icd->ops		= NULL;
+	priv->fmt		= &ov5642_colour_fmts[0];
+
+	priv->crop_rect.width	= OV5642_DEFAULT_WIDTH;
+	priv->crop_rect.height	= OV5642_DEFAULT_HEIGHT;
+	priv->crop_rect.left	= (OV5642_MAX_WIDTH - OV5642_DEFAULT_WIDTH) / 2;
+	priv->crop_rect.top	= (OV5642_MAX_HEIGHT - OV5642_DEFAULT_HEIGHT) / 2;
+	priv->out_size.width	= OV5642_DEFAULT_WIDTH;
+	priv->out_size.height	= OV5642_DEFAULT_HEIGHT;
+	priv->out_size.total_width = OV5642_DEFAULT_WIDTH + BLANKING_EXTRA_WIDTH;
+	priv->out_size.total_height = BLANKING_MIN_HEIGHT;
 
 	ret = ov5642_video_probe(icd, client);
 	if (ret < 0)
@@ -951,6 +1034,7 @@ static int ov5642_probe(struct i2c_client *client,
 	return 0;
 
 error:
+	icd->ops = NULL;
 	kfree(priv);
 	return ret;
 }
@@ -961,6 +1045,7 @@ static int ov5642_remove(struct i2c_client *client)
 	struct soc_camera_device *icd = client->dev.platform_data;
 	struct soc_camera_link *icl = to_soc_camera_link(icd);
 
+	icd->ops = NULL;
 	if (icl->free_bus)
 		icl->free_bus(icl);
 	kfree(priv);

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

[Laurent Pinchart]

Hi Bastian,

Thanks for the patch.

On Wednesday 31 August 2011 17:05:52 Bastian Hecht wrote:
> 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 <hechtb@gmail.com>
> ---
> 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]

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

[snip]

> -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.

> -	return 0;
> +	return ret;
>  }

[snip]

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

[snip]

>  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 ?

> +	*rect = priv->crop_rect;
> 
>  	return 0;
>  }

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Bastian,

A second comment.

On Wednesday 31 August 2011 17:05:52 Bastian Hecht wrote:
> 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 <hechtb@gmail.com>
> ---
> 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]

> @@ -578,9 +605,20 @@ struct ov5642_datafmt {
>  	enum v4l2_colorspace		colorspace;
>  };
> 
> +/* the output resolution and blanking information */
> +struct ov5642_out_size {
> +	int width;
> +	int height;
> +	int total_width;
> +	int total_height;
> +};
> +
>  struct ov5642 {
>  	struct v4l2_subdev		subdev;
> +
>  	const struct ov5642_datafmt	*fmt;
> +	struct v4l2_rect                crop_rect;
> +	struct ov5642_out_size		out_size;
>  };

If I'm not mistaken you store the exact same width/height in crop_rect and 
out_size. If that's right, you should remove width/height from struct 
ov5642_out_size (or maybe better move the total_width and total_height 
directly to the ov5642 structure).

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

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:
> > 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 <hechtb@gmail.com>
> > ---
> > 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]
> 
> > @@ -684,107 +737,101 @@ static int ov5642_write_array(struct i2c_client
> 
> [snip]
> 
> > -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.
> 
> > -	return 0;
> > +	return ret;
> > 
> >  }
> 
> [snip]
> 
> > @@ -827,15 +874,42 @@ static int ov5642_g_chip_ident(struct v4l2_subdev
> 
> [snip]
> 
> >  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 ?
> 
> > +	*rect = priv->crop_rect;
> > 
> >  	return 0;
> >  
> >  }

-- 
Regards,

Laurent Pinchart

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

[Bastian Hecht]

Hello Laurent,

2011/8/31 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> 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"

I think that you think I should do the right thing. I removed
out_sizes and repost v3 in a moment :)


> 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:
>> > 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 <hechtb@gmail.com>
>> > ---
>> > 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]
>>
>> > @@ -684,107 +737,101 @@ static int ov5642_write_array(struct i2c_client
>>
>> [snip]
>>
>> > -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.

We talked about it in the ov5642 controls thread. I need to initialize
the sensor at some point and it doesn't work to divide the calls
between different locations.

>> > -   return 0;
>> > +   return ret;
>> >
>> >  }
>>
>> [snip]
>>
>> > @@ -827,15 +874,42 @@ static int ov5642_g_chip_ident(struct v4l2_subdev
>>
>> [snip]
>>
>> >  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 ?

No idea, but if you say so, I'll change it.

>> > +   *rect = priv->crop_rect;
>> >
>> >     return 0;
>> >
>> >  }
>
> --
> Regards,
>
> Laurent Pinchart
>

best,

 Bastian

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

[Laurent Pinchart]

Hi Bastian,

On Monday 05 September 2011 11:10:48 Bastian Hecht wrote:
> 2011/8/31 Laurent Pinchart:
> > 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"
> 
> I think that you think I should do the right thing. I removed out_sizes and
> repost v3 in a moment :)

Thanks :-)

> > 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:
> >> > 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 <hechtb@gmail.com>
> >> > ---
> >> > 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]
> >> 
> >> > @@ -684,107 +737,101 @@ static int ov5642_write_array(struct
> >> > i2c_client
> >> 
> >> [snip]
> >> 
> >> > -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.
> 
> We talked about it in the ov5642 controls thread. I need to initialize
> the sensor at some point and it doesn't work to divide the calls
> between different locations.

Sure, but calling s_fmt isn't mandatory for hosts/bridges. What about moving 
sensor initialization to s_stream() ?

> >> > -   return 0;
> >> > +   return ret;
> >> > 
> >> >  }
> >> 
> >> [snip]
> >> 
> >> > @@ -827,15 +874,42 @@ static int ov5642_g_chip_ident(struct
> >> > v4l2_subdev
> >> 
> >> [snip]
> >> 
> >> >  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 ?
> 
> No idea, but if you say so, I'll change it.

VIDIOC_G_FMT documentation states that

"When the requested buffer type is not supported drivers return an EINVAL 
error code."

I thought VIDIOC_G_CROP documentation did as well, but it doesn't. However I 
believe the above should apply to VIDIOC_G_CROP as well. There is no explicit 
documentation about error codes for subdev operations, but I think it makes 
sense to follow what the V4L2 ioctls do.

-- 
Regards,

Laurent Pinchart

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

[Bastian Hecht]

2011/9/5 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> Hi Bastian,
>
> On Monday 05 September 2011 11:10:48 Bastian Hecht wrote:
>> 2011/8/31 Laurent Pinchart:
>> > 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"
>>
>> I think that you think I should do the right thing. I removed out_sizes and
>> repost v3 in a moment :)
>
> Thanks :-)
>
>> > 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:
>> >> > 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 <hechtb@gmail.com>
>> >> > ---
>> >> > 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]
>> >>
>> >> > @@ -684,107 +737,101 @@ static int ov5642_write_array(struct
>> >> > i2c_client
>> >>
>> >> [snip]
>> >>
>> >> > -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.
>>
>> We talked about it in the ov5642 controls thread. I need to initialize
>> the sensor at some point and it doesn't work to divide the calls
>> between different locations.
>
> Sure, but calling s_fmt isn't mandatory for hosts/bridges. What about moving
> sensor initialization to s_stream() ?
>
>> >> > -   return 0;
>> >> > +   return ret;
>> >> >
>> >> >  }
>> >>
>> >> [snip]
>> >>
>> >> > @@ -827,15 +874,42 @@ static int ov5642_g_chip_ident(struct
>> >> > v4l2_subdev
>> >>
>> >> [snip]
>> >>
>> >> >  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 ?
>>
>> No idea, but if you say so, I'll change it.
>
> VIDIOC_G_FMT documentation states that
>
> "When the requested buffer type is not supported drivers return an EINVAL
> error code."
>
> I thought VIDIOC_G_CROP documentation did as well, but it doesn't. However I
> believe the above should apply to VIDIOC_G_CROP as well. There is no explicit
> documentation about error codes for subdev operations, but I think it makes
> sense to follow what the V4L2 ioctls do.

And these ioctl calls go straight through to my driver? Or is there
some intermediate work by the subdev architecture? I'm asking because
I don't check the buffer type in g_fmt as well. If so, I have to
change that too.

best,

 Bastian

> --
> Regards,
>
> Laurent Pinchart
>

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

[Laurent Pinchart]

Hi Bastian,

On Monday 05 September 2011 11:41:28 Bastian Hecht wrote:
> 2011/9/5 Laurent Pinchart:
> > On Monday 05 September 2011 11:10:48 Bastian Hecht wrote:
> >> 2011/8/31 Laurent Pinchart:
> >> >> >  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 ?
> >> 
> >> No idea, but if you say so, I'll change it.
> > 
> > VIDIOC_G_FMT documentation states that
> > 
> > "When the requested buffer type is not supported drivers return an EINVAL
> > error code."
> > 
> > I thought VIDIOC_G_CROP documentation did as well, but it doesn't.
> > However I believe the above should apply to VIDIOC_G_CROP as well. There
> > is no explicit documentation about error codes for subdev operations,
> > but I think it makes sense to follow what the V4L2 ioctls do.
> 
> And these ioctl calls go straight through to my driver? Or is there
> some intermediate work by the subdev architecture? I'm asking because
> I don't check the buffer type in g_fmt as well. If so, I have to
> change that too.

The ioctls go to the host/bridge driver, which then decides when and how to 
call g/s_fmt and g/s_crop. I would add the same check to g_fmt.

-- 
Regards,

Laurent Pinchart

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

[Bastian Hecht]

Hello Laurent,

2011/9/5 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> Hi Bastian,
>
> On Monday 05 September 2011 11:41:28 Bastian Hecht wrote:
>> 2011/9/5 Laurent Pinchart:
>> > On Monday 05 September 2011 11:10:48 Bastian Hecht wrote:
>> >> 2011/8/31 Laurent Pinchart:
>> >> >> >  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 ?
>> >>
>> >> No idea, but if you say so, I'll change it.
>> >
>> > VIDIOC_G_FMT documentation states that
>> >
>> > "When the requested buffer type is not supported drivers return an EINVAL
>> > error code."
>> >
>> > I thought VIDIOC_G_CROP documentation did as well, but it doesn't.
>> > However I believe the above should apply to VIDIOC_G_CROP as well. There
>> > is no explicit documentation about error codes for subdev operations,
>> > but I think it makes sense to follow what the V4L2 ioctls do.
>>
>> And these ioctl calls go straight through to my driver? Or is there
>> some intermediate work by the subdev architecture? I'm asking because
>> I don't check the buffer type in g_fmt as well. If so, I have to
>> change that too.
>
> The ioctls go to the host/bridge driver, which then decides when and how to
> call g/s_fmt and g/s_crop. I would add the same check to g_fmt.

Next time I will work in the media section of the kernel, I must study
the docs a bit better I guess :/
To the g_fmt() thing... is there the buffer type given at all? The
argument of type struct v4l2_mbus_framefmt doesn't contain it.

best,

 Bastian


> --
> Regards,
>
> Laurent Pinchart
> --
> 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
>

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

[Guennadi Liakhovetski]

On Mon, 5 Sep 2011, Laurent Pinchart wrote:

> Hi Bastian,
> 
> On Monday 05 September 2011 11:10:48 Bastian Hecht wrote:
> > 2011/8/31 Laurent Pinchart:
> > > 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"
> > 
> > I think that you think I should do the right thing. I removed out_sizes and
> > repost v3 in a moment :)
> 
> Thanks :-)
> 
> > > 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:
> > >> > 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 <hechtb@gmail.com>
> > >> > ---
> > >> > 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]
> > >> 
> > >> > @@ -684,107 +737,101 @@ static int ov5642_write_array(struct
> > >> > i2c_client
> > >> 
> > >> [snip]
> > >> 
> > >> > -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.
> > 
> > We talked about it in the ov5642 controls thread. I need to initialize
> > the sensor at some point and it doesn't work to divide the calls
> > between different locations.
> 
> Sure, but calling s_fmt isn't mandatory for hosts/bridges. What about moving 
> sensor initialization to s_stream() ?

Throughout the development of this driver, I was opposing the "delayed 
configuration" approach. I.e., the approach, in which all the ioctl()s, 
like S_FMT, S_CROP, etc. only store user values internally, and the actual 
hardware configuration is only performed at STREAMON time. There are 
several reasons to this: the spec says "the driver may program the 
hardware, allocate resources and generally prepare for data exchange" 
(yes, "may" != "must"), most drivers seem to do the same, the possibility 
to check and return any hardware errors, returned by this operation, I 
probably have forgotten something. But if we ignore all these reasons as 
insufficiently important, then yes, doing the actualy hardware 
configuration in .s_stream() brings a couple of advantages with it, 
especially for drivers / devices like this one.

So, if there are no strong objections, maybe indeed move this back to 
.s_stream() would be the better solution here.

Thanks
Guennadi

> 
> > >> > -   return 0;
> > >> > +   return ret;
> > >> > 
> > >> >  }
> > >> 
> > >> [snip]
> > >> 
> > >> > @@ -827,15 +874,42 @@ static int ov5642_g_chip_ident(struct
> > >> > v4l2_subdev
> > >> 
> > >> [snip]
> > >> 
> > >> >  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 ?
> > 
> > No idea, but if you say so, I'll change it.
> 
> VIDIOC_G_FMT documentation states that
> 
> "When the requested buffer type is not supported drivers return an EINVAL 
> error code."
> 
> I thought VIDIOC_G_CROP documentation did as well, but it doesn't. However I 
> believe the above should apply to VIDIOC_G_CROP as well. There is no explicit 
> documentation about error codes for subdev operations, but I think it makes 
> sense to follow what the V4L2 ioctls do.
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

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

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

[Guennadi Liakhovetski]

On Mon, 5 Sep 2011, Bastian Hecht wrote:

[snip]

> And these ioctl calls go straight through to my driver? Or is there
> some intermediate work by the subdev architecture? I'm asking because
> I don't check the buffer type in g_fmt as well. If so, I have to
> change that too.

With soc-camera all ioctl()s first enter in soc_camera.c (check 
soc_camera_ioctl_ops), then they are typically dispatched to the camera 
host driver, which then calls respective subdev methods. Check respective 
ioctl() implementations for details.

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

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

[Laurent Pinchart]

Hi Bastian,

On Monday 05 September 2011 11:51:52 Bastian Hecht wrote:
> 2011/9/5 Laurent Pinchart:
> > On Monday 05 September 2011 11:41:28 Bastian Hecht wrote:
> >> 2011/9/5 Laurent Pinchart:
> >> > On Monday 05 September 2011 11:10:48 Bastian Hecht wrote:
> >> >> 2011/8/31 Laurent Pinchart:
> >> >> >> >  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 ?
> >> >> 
> >> >> No idea, but if you say so, I'll change it.
> >> > 
> >> > VIDIOC_G_FMT documentation states that
> >> > 
> >> > "When the requested buffer type is not supported drivers return an
> >> > EINVAL error code."
> >> > 
> >> > I thought VIDIOC_G_CROP documentation did as well, but it doesn't.
> >> > However I believe the above should apply to VIDIOC_G_CROP as well.
> >> > There is no explicit documentation about error codes for subdev
> >> > operations, but I think it makes sense to follow what the V4L2 ioctls
> >> > do.
> >> 
> >> And these ioctl calls go straight through to my driver? Or is there
> >> some intermediate work by the subdev architecture? I'm asking because
> >> I don't check the buffer type in g_fmt as well. If so, I have to
> >> change that too.
> > 
> > The ioctls go to the host/bridge driver, which then decides when and how
> > to call g/s_fmt and g/s_crop. I would add the same check to g_fmt.
> 
> Next time I will work in the media section of the kernel, I must study
> the docs a bit better I guess :/

Studying the docs isn't very difficult, the real problem is understanding what 
is not written in the documentation :-)

> To the g_fmt() thing... is there the buffer type given at all? The
> argument of type struct v4l2_mbus_framefmt doesn't contain it.

Oops, my bad. You're right, there's no buffer type argument to the mbus 
g/s_fmt calls. Sorry for the mistake.

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Guennadi,

On Monday 05 September 2011 11:51:57 Guennadi Liakhovetski wrote:
> On Mon, 5 Sep 2011, Laurent Pinchart wrote:
> > On Monday 05 September 2011 11:10:48 Bastian Hecht wrote:
> > > 2011/8/31 Laurent Pinchart:
> > > > On Wednesday 31 August 2011 19:06:25 Laurent Pinchart wrote:
> > > >> On Wednesday 31 August 2011 17:05:52 Bastian Hecht wrote:
> > > >> > 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 <hechtb@gmail.com>
> > > >> > ---
> > > >> > 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]
> > > >> 
> > > >> > @@ -684,107 +737,101 @@ static int ov5642_write_array(struct
> > > >> > i2c_client
> > > >> 
> > > >> [snip]
> > > >> 
> > > >> > -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.
> > > 
> > > We talked about it in the ov5642 controls thread. I need to initialize
> > > the sensor at some point and it doesn't work to divide the calls
> > > between different locations.
> > 
> > Sure, but calling s_fmt isn't mandatory for hosts/bridges. What about
> > moving sensor initialization to s_stream() ?
> 
> Throughout the development of this driver, I was opposing the "delayed
> configuration" approach. I.e., the approach, in which all the ioctl()s,
> like S_FMT, S_CROP, etc. only store user values internally, and the actual
> hardware configuration is only performed at STREAMON time. There are
> several reasons to this: the spec says "the driver may program the
> hardware, allocate resources and generally prepare for data exchange"
> (yes, "may" != "must"), most drivers seem to do the same, the possibility
> to check and return any hardware errors, returned by this operation, I
> probably have forgotten something. But if we ignore all these reasons as
> insufficiently important, then yes, doing the actualy hardware
> configuration in .s_stream() brings a couple of advantages with it,
> especially for drivers / devices like this one.
> 
> So, if there are no strong objections, maybe indeed move this back to
> .s_stream() would be the better solution here.

I have no strong opinion here. Your points are certainly valid. I'm fine with 
performing direct hardware setup in .s_crop(), but doing it in .s_fmt() looks 
weird to me as .s_fmt() doesn't perform any operation now that the driver 
moved to using .s_crop(). Without delayed initialization I believe the device 
should be initialized when powered up, and have its crop rectangle altered in 
.s_crop().

-- 
Regards,

Laurent Pinchart

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

[Bastian Hecht]

2011/9/5 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> Hi Guennadi,
>
> On Monday 05 September 2011 11:51:57 Guennadi Liakhovetski wrote:
>> On Mon, 5 Sep 2011, Laurent Pinchart wrote:
>> > On Monday 05 September 2011 11:10:48 Bastian Hecht wrote:
>> > > 2011/8/31 Laurent Pinchart:
>> > > > On Wednesday 31 August 2011 19:06:25 Laurent Pinchart wrote:
>> > > >> On Wednesday 31 August 2011 17:05:52 Bastian Hecht wrote:
>> > > >> > 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 <hechtb@gmail.com>
>> > > >> > ---
>> > > >> > 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]
>> > > >>
>> > > >> > @@ -684,107 +737,101 @@ static int ov5642_write_array(struct
>> > > >> > i2c_client
>> > > >>
>> > > >> [snip]
>> > > >>
>> > > >> > -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.
>> > >
>> > > We talked about it in the ov5642 controls thread. I need to initialize
>> > > the sensor at some point and it doesn't work to divide the calls
>> > > between different locations.
>> >
>> > Sure, but calling s_fmt isn't mandatory for hosts/bridges. What about
>> > moving sensor initialization to s_stream() ?
>>
>> Throughout the development of this driver, I was opposing the "delayed
>> configuration" approach. I.e., the approach, in which all the ioctl()s,
>> like S_FMT, S_CROP, etc. only store user values internally, and the actual
>> hardware configuration is only performed at STREAMON time. There are
>> several reasons to this: the spec says "the driver may program the
>> hardware, allocate resources and generally prepare for data exchange"
>> (yes, "may" != "must"), most drivers seem to do the same, the possibility
>> to check and return any hardware errors, returned by this operation, I
>> probably have forgotten something. But if we ignore all these reasons as
>> insufficiently important, then yes, doing the actualy hardware
>> configuration in .s_stream() brings a couple of advantages with it,
>> especially for drivers / devices like this one.
>>
>> So, if there are no strong objections, maybe indeed move this back to
>> .s_stream() would be the better solution here.
>
> I have no strong opinion here. Your points are certainly valid. I'm fine with
> performing direct hardware setup in .s_crop(), but doing it in .s_fmt() looks
> weird to me as .s_fmt() doesn't perform any operation now that the driver
> moved to using .s_crop(). Without delayed initialization I believe the device
> should be initialized when powered up, and have its crop rectangle altered in
> .s_crop().

Ok, it is moved to s_power and s_crop now. This approach sounds clean indeed.

best,

 Bastian


> --
> Regards,
>
> Laurent Pinchart
>