[PATCH v3] adv7180: add more subdev video ops

[PATCH v3] adv7180: add more subdev video ops

[Sergei Shtylyov]

From: Vladimir Barinov <vladimir.barinov@cogentembedded.com>

Add subdev video ops for ADV7180 video decoder.  This makes decoder usable on
the soc-camera drivers.

Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
This patch is against the 'media_tree.git' repo.

Changes from version 2:
- set the field format depending on video standard in try_mbus_fmt() method;
- removed querystd() method calls from try_mbus_fmt() and cropcap() methods;
- removed g_crop() method.

 drivers/media/i2c/adv7180.c |   86 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)

Index: media_tree/drivers/media/i2c/adv7180.c
=================================--- media_tree.orig/drivers/media/i2c/adv7180.c
+++ media_tree/drivers/media/i2c/adv7180.c
@@ -1,6 +1,8 @@
 /*
  * adv7180.c Analog Devices ADV7180 video decoder driver
  * Copyright (c) 2009 Intel Corporation
+ * Copyright (C) 2013 Cogent Embedded, Inc.
+ * Copyright (C) 2013 Renesas Solutions Corp.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -128,6 +130,7 @@ struct adv7180_state {
 	v4l2_std_id		curr_norm;
 	bool			autodetect;
 	u8			input;
+	struct v4l2_mbus_framefmt fmt;
 };
 #define to_adv7180_sd(_ctrl) (&container_of(_ctrl->handler,		\
 					    struct adv7180_state,	\
@@ -397,10 +400,93 @@ static void adv7180_exit_controls(struct
 	v4l2_ctrl_handler_free(&state->ctrl_hdl);
 }
 
+static int adv7180_enum_mbus_fmt(struct v4l2_subdev *sd, unsigned int index,
+				 enum v4l2_mbus_pixelcode *code)
+{
+	if (index > 0)
+		return -EINVAL;
+
+	*code = V4L2_MBUS_FMT_YUYV8_2X8;
+
+	return 0;
+}
+
+static int adv7180_try_mbus_fmt(struct v4l2_subdev *sd,
+				struct v4l2_mbus_framefmt *fmt)
+{
+	struct adv7180_state *state = to_state(sd);
+
+	fmt->code = V4L2_MBUS_FMT_YUYV8_2X8;
+	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
+	fmt->field = state->curr_norm & V4L2_STD_525_60 ?
+		     V4L2_FIELD_INTERLACED_BT : V4L2_FIELD_INTERLACED_TB;
+	fmt->width = 720;
+	fmt->height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
+
+	return 0;
+}
+
+static int adv7180_g_mbus_fmt(struct v4l2_subdev *sd,
+			      struct v4l2_mbus_framefmt *fmt)
+{
+	struct adv7180_state *state = to_state(sd);
+
+	*fmt = state->fmt;
+
+	return 0;
+}
+
+static int adv7180_s_mbus_fmt(struct v4l2_subdev *sd,
+			      struct v4l2_mbus_framefmt *fmt)
+{
+	struct adv7180_state *state = to_state(sd);
+
+	adv7180_try_mbus_fmt(sd, fmt);
+	state->fmt = *fmt;
+
+	return 0;
+}
+
+static int adv7180_cropcap(struct v4l2_subdev *sd, struct v4l2_cropcap *a)
+{
+	struct adv7180_state *state = to_state(sd);
+
+	a->bounds.left = 0;
+	a->bounds.top = 0;
+	a->bounds.width = 720;
+	a->bounds.height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
+	a->defrect = a->bounds;
+	a->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+	a->pixelaspect.numerator = 1;
+	a->pixelaspect.denominator = 1;
+
+	return 0;
+}
+
+static int adv7180_g_mbus_config(struct v4l2_subdev *sd,
+				 struct v4l2_mbus_config *cfg)
+{
+	/*
+	 * The ADV7180 sensor supports BT.601/656 output modes.
+	 * The BT.656 is default and not yet configurable by s/w.
+	 */
+	cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING |
+		     V4L2_MBUS_DATA_ACTIVE_HIGH;
+	cfg->type = V4L2_MBUS_BT656;
+
+	return 0;
+}
+
 static const struct v4l2_subdev_video_ops adv7180_video_ops = {
 	.querystd = adv7180_querystd,
 	.g_input_status = adv7180_g_input_status,
 	.s_routing = adv7180_s_routing,
+	.enum_mbus_fmt = adv7180_enum_mbus_fmt,
+	.try_mbus_fmt = adv7180_try_mbus_fmt,
+	.g_mbus_fmt = adv7180_g_mbus_fmt,
+	.s_mbus_fmt = adv7180_s_mbus_fmt,
+	.cropcap = adv7180_cropcap,
+	.g_mbus_config = adv7180_g_mbus_config,
 };
 
 static const struct v4l2_subdev_core_ops adv7180_core_ops = {

[PATCH v3] adv7180: add more subdev video ops

[Sergei Shtylyov]

From: Vladimir Barinov <vladimir.barinov@cogentembedded.com>

Add subdev video ops for ADV7180 video decoder.  This makes decoder usable on
the soc-camera drivers.

Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
This patch is against the 'media_tree.git' repo.

Changes from version 2:
- set the field format depending on video standard in try_mbus_fmt() method;
- removed querystd() method calls from try_mbus_fmt() and cropcap() methods;
- removed g_crop() method.

 drivers/media/i2c/adv7180.c |   86 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)

Index: media_tree/drivers/media/i2c/adv7180.c
===================================================================
--- media_tree.orig/drivers/media/i2c/adv7180.c
+++ media_tree/drivers/media/i2c/adv7180.c
@@ -1,6 +1,8 @@
 /*
  * adv7180.c Analog Devices ADV7180 video decoder driver
  * Copyright (c) 2009 Intel Corporation
+ * Copyright (C) 2013 Cogent Embedded, Inc.
+ * Copyright (C) 2013 Renesas Solutions Corp.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -128,6 +130,7 @@ struct adv7180_state {
 	v4l2_std_id		curr_norm;
 	bool			autodetect;
 	u8			input;
+	struct v4l2_mbus_framefmt fmt;
 };
 #define to_adv7180_sd(_ctrl) (&container_of(_ctrl->handler,		\
 					    struct adv7180_state,	\
@@ -397,10 +400,93 @@ static void adv7180_exit_controls(struct
 	v4l2_ctrl_handler_free(&state->ctrl_hdl);
 }
 
+static int adv7180_enum_mbus_fmt(struct v4l2_subdev *sd, unsigned int index,
+				 enum v4l2_mbus_pixelcode *code)
+{
+	if (index > 0)
+		return -EINVAL;
+
+	*code = V4L2_MBUS_FMT_YUYV8_2X8;
+
+	return 0;
+}
+
+static int adv7180_try_mbus_fmt(struct v4l2_subdev *sd,
+				struct v4l2_mbus_framefmt *fmt)
+{
+	struct adv7180_state *state = to_state(sd);
+
+	fmt->code = V4L2_MBUS_FMT_YUYV8_2X8;
+	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
+	fmt->field = state->curr_norm & V4L2_STD_525_60 ?
+		     V4L2_FIELD_INTERLACED_BT : V4L2_FIELD_INTERLACED_TB;
+	fmt->width = 720;
+	fmt->height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
+
+	return 0;
+}
+
+static int adv7180_g_mbus_fmt(struct v4l2_subdev *sd,
+			      struct v4l2_mbus_framefmt *fmt)
+{
+	struct adv7180_state *state = to_state(sd);
+
+	*fmt = state->fmt;
+
+	return 0;
+}
+
+static int adv7180_s_mbus_fmt(struct v4l2_subdev *sd,
+			      struct v4l2_mbus_framefmt *fmt)
+{
+	struct adv7180_state *state = to_state(sd);
+
+	adv7180_try_mbus_fmt(sd, fmt);
+	state->fmt = *fmt;
+
+	return 0;
+}
+
+static int adv7180_cropcap(struct v4l2_subdev *sd, struct v4l2_cropcap *a)
+{
+	struct adv7180_state *state = to_state(sd);
+
+	a->bounds.left = 0;
+	a->bounds.top = 0;
+	a->bounds.width = 720;
+	a->bounds.height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
+	a->defrect = a->bounds;
+	a->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+	a->pixelaspect.numerator = 1;
+	a->pixelaspect.denominator = 1;
+
+	return 0;
+}
+
+static int adv7180_g_mbus_config(struct v4l2_subdev *sd,
+				 struct v4l2_mbus_config *cfg)
+{
+	/*
+	 * The ADV7180 sensor supports BT.601/656 output modes.
+	 * The BT.656 is default and not yet configurable by s/w.
+	 */
+	cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING |
+		     V4L2_MBUS_DATA_ACTIVE_HIGH;
+	cfg->type = V4L2_MBUS_BT656;
+
+	return 0;
+}
+
 static const struct v4l2_subdev_video_ops adv7180_video_ops = {
 	.querystd = adv7180_querystd,
 	.g_input_status = adv7180_g_input_status,
 	.s_routing = adv7180_s_routing,
+	.enum_mbus_fmt = adv7180_enum_mbus_fmt,
+	.try_mbus_fmt = adv7180_try_mbus_fmt,
+	.g_mbus_fmt = adv7180_g_mbus_fmt,
+	.s_mbus_fmt = adv7180_s_mbus_fmt,
+	.cropcap = adv7180_cropcap,
+	.g_mbus_config = adv7180_g_mbus_config,
 };
 
 static const struct v4l2_subdev_core_ops adv7180_core_ops = {

Re: [PATCH v3] adv7180: add more subdev video ops

[Sergei Shtylyov]

Hello.

On 05/13/2013 11:21 PM, Sergei Shtylyov wrote:

> From: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
>
> Add subdev video ops for ADV7180 video decoder.  This makes decoder usable on
> the soc-camera drivers.
>
> Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

     Hans, what's your opinion of this version? Mauro said I should ask 
you first...

> ---
> This patch is against the 'media_tree.git' repo.
>
> Changes from version 2:
> - set the field format depending on video standard in try_mbus_fmt() method;
> - removed querystd() method calls from try_mbus_fmt() and cropcap() methods;
> - removed g_crop() method.
>
>   drivers/media/i2c/adv7180.c |   86 ++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 86 insertions(+)
>
> Index: media_tree/drivers/media/i2c/adv7180.c
> =================================> --- media_tree.orig/drivers/media/i2c/adv7180.c
> +++ media_tree/drivers/media/i2c/adv7180.c
> @@ -1,6 +1,8 @@
>   /*
>    * adv7180.c Analog Devices ADV7180 video decoder driver
>    * Copyright (c) 2009 Intel Corporation
> + * Copyright (C) 2013 Cogent Embedded, Inc.
> + * Copyright (C) 2013 Renesas Solutions Corp.
>    *
>    * This program is free software; you can redistribute it and/or modify
>    * it under the terms of the GNU General Public License version 2 as
> @@ -128,6 +130,7 @@ struct adv7180_state {
>   	v4l2_std_id		curr_norm;
>   	bool			autodetect;
>   	u8			input;
> +	struct v4l2_mbus_framefmt fmt;
>   };
>   #define to_adv7180_sd(_ctrl) (&container_of(_ctrl->handler,		\
>   					    struct adv7180_state,	\
> @@ -397,10 +400,93 @@ static void adv7180_exit_controls(struct
>   	v4l2_ctrl_handler_free(&state->ctrl_hdl);
>   }
>   
> +static int adv7180_enum_mbus_fmt(struct v4l2_subdev *sd, unsigned int index,
> +				 enum v4l2_mbus_pixelcode *code)
> +{
> +	if (index > 0)
> +		return -EINVAL;
> +
> +	*code = V4L2_MBUS_FMT_YUYV8_2X8;
> +
> +	return 0;
> +}
> +
> +static int adv7180_try_mbus_fmt(struct v4l2_subdev *sd,
> +				struct v4l2_mbus_framefmt *fmt)
> +{
> +	struct adv7180_state *state = to_state(sd);
> +
> +	fmt->code = V4L2_MBUS_FMT_YUYV8_2X8;
> +	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
> +	fmt->field = state->curr_norm & V4L2_STD_525_60 ?
> +		     V4L2_FIELD_INTERLACED_BT : V4L2_FIELD_INTERLACED_TB;
> +	fmt->width = 720;
> +	fmt->height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
> +
> +	return 0;
> +}
> +
> +static int adv7180_g_mbus_fmt(struct v4l2_subdev *sd,
> +			      struct v4l2_mbus_framefmt *fmt)
> +{
> +	struct adv7180_state *state = to_state(sd);
> +
> +	*fmt = state->fmt;
> +
> +	return 0;
> +}
> +
> +static int adv7180_s_mbus_fmt(struct v4l2_subdev *sd,
> +			      struct v4l2_mbus_framefmt *fmt)
> +{
> +	struct adv7180_state *state = to_state(sd);
> +
> +	adv7180_try_mbus_fmt(sd, fmt);
> +	state->fmt = *fmt;
> +
> +	return 0;
> +}
> +
> +static int adv7180_cropcap(struct v4l2_subdev *sd, struct v4l2_cropcap *a)
> +{
> +	struct adv7180_state *state = to_state(sd);
> +
> +	a->bounds.left = 0;
> +	a->bounds.top = 0;
> +	a->bounds.width = 720;
> +	a->bounds.height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
> +	a->defrect = a->bounds;
> +	a->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> +	a->pixelaspect.numerator = 1;
> +	a->pixelaspect.denominator = 1;
> +
> +	return 0;
> +}
> +
> +static int adv7180_g_mbus_config(struct v4l2_subdev *sd,
> +				 struct v4l2_mbus_config *cfg)
> +{
> +	/*
> +	 * The ADV7180 sensor supports BT.601/656 output modes.
> +	 * The BT.656 is default and not yet configurable by s/w.
> +	 */
> +	cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING |
> +		     V4L2_MBUS_DATA_ACTIVE_HIGH;
> +	cfg->type = V4L2_MBUS_BT656;
> +
> +	return 0;
> +}
> +
>   static const struct v4l2_subdev_video_ops adv7180_video_ops = {
>   	.querystd = adv7180_querystd,
>   	.g_input_status = adv7180_g_input_status,
>   	.s_routing = adv7180_s_routing,
> +	.enum_mbus_fmt = adv7180_enum_mbus_fmt,
> +	.try_mbus_fmt = adv7180_try_mbus_fmt,
> +	.g_mbus_fmt = adv7180_g_mbus_fmt,
> +	.s_mbus_fmt = adv7180_s_mbus_fmt,
> +	.cropcap = adv7180_cropcap,
> +	.g_mbus_config = adv7180_g_mbus_config,
>   };
>   
>   static const struct v4l2_subdev_core_ops adv7180_core_ops = {

WBR, Sergei

Re: [PATCH v3] adv7180: add more subdev video ops

[Sergei Shtylyov]

Hello.

On 05/13/2013 11:21 PM, Sergei Shtylyov wrote:

> From: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
>
> Add subdev video ops for ADV7180 video decoder.  This makes decoder usable on
> the soc-camera drivers.
>
> Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

     Hans, what's your opinion of this version? Mauro said I should ask 
you first...

> ---
> This patch is against the 'media_tree.git' repo.
>
> Changes from version 2:
> - set the field format depending on video standard in try_mbus_fmt() method;
> - removed querystd() method calls from try_mbus_fmt() and cropcap() methods;
> - removed g_crop() method.
>
>   drivers/media/i2c/adv7180.c |   86 ++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 86 insertions(+)
>
> Index: media_tree/drivers/media/i2c/adv7180.c
> ===================================================================
> --- media_tree.orig/drivers/media/i2c/adv7180.c
> +++ media_tree/drivers/media/i2c/adv7180.c
> @@ -1,6 +1,8 @@
>   /*
>    * adv7180.c Analog Devices ADV7180 video decoder driver
>    * Copyright (c) 2009 Intel Corporation
> + * Copyright (C) 2013 Cogent Embedded, Inc.
> + * Copyright (C) 2013 Renesas Solutions Corp.
>    *
>    * This program is free software; you can redistribute it and/or modify
>    * it under the terms of the GNU General Public License version 2 as
> @@ -128,6 +130,7 @@ struct adv7180_state {
>   	v4l2_std_id		curr_norm;
>   	bool			autodetect;
>   	u8			input;
> +	struct v4l2_mbus_framefmt fmt;
>   };
>   #define to_adv7180_sd(_ctrl) (&container_of(_ctrl->handler,		\
>   					    struct adv7180_state,	\
> @@ -397,10 +400,93 @@ static void adv7180_exit_controls(struct
>   	v4l2_ctrl_handler_free(&state->ctrl_hdl);
>   }
>   
> +static int adv7180_enum_mbus_fmt(struct v4l2_subdev *sd, unsigned int index,
> +				 enum v4l2_mbus_pixelcode *code)
> +{
> +	if (index > 0)
> +		return -EINVAL;
> +
> +	*code = V4L2_MBUS_FMT_YUYV8_2X8;
> +
> +	return 0;
> +}
> +
> +static int adv7180_try_mbus_fmt(struct v4l2_subdev *sd,
> +				struct v4l2_mbus_framefmt *fmt)
> +{
> +	struct adv7180_state *state = to_state(sd);
> +
> +	fmt->code = V4L2_MBUS_FMT_YUYV8_2X8;
> +	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
> +	fmt->field = state->curr_norm & V4L2_STD_525_60 ?
> +		     V4L2_FIELD_INTERLACED_BT : V4L2_FIELD_INTERLACED_TB;
> +	fmt->width = 720;
> +	fmt->height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
> +
> +	return 0;
> +}
> +
> +static int adv7180_g_mbus_fmt(struct v4l2_subdev *sd,
> +			      struct v4l2_mbus_framefmt *fmt)
> +{
> +	struct adv7180_state *state = to_state(sd);
> +
> +	*fmt = state->fmt;
> +
> +	return 0;
> +}
> +
> +static int adv7180_s_mbus_fmt(struct v4l2_subdev *sd,
> +			      struct v4l2_mbus_framefmt *fmt)
> +{
> +	struct adv7180_state *state = to_state(sd);
> +
> +	adv7180_try_mbus_fmt(sd, fmt);
> +	state->fmt = *fmt;
> +
> +	return 0;
> +}
> +
> +static int adv7180_cropcap(struct v4l2_subdev *sd, struct v4l2_cropcap *a)
> +{
> +	struct adv7180_state *state = to_state(sd);
> +
> +	a->bounds.left = 0;
> +	a->bounds.top = 0;
> +	a->bounds.width = 720;
> +	a->bounds.height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
> +	a->defrect = a->bounds;
> +	a->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> +	a->pixelaspect.numerator = 1;
> +	a->pixelaspect.denominator = 1;
> +
> +	return 0;
> +}
> +
> +static int adv7180_g_mbus_config(struct v4l2_subdev *sd,
> +				 struct v4l2_mbus_config *cfg)
> +{
> +	/*
> +	 * The ADV7180 sensor supports BT.601/656 output modes.
> +	 * The BT.656 is default and not yet configurable by s/w.
> +	 */
> +	cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING |
> +		     V4L2_MBUS_DATA_ACTIVE_HIGH;
> +	cfg->type = V4L2_MBUS_BT656;
> +
> +	return 0;
> +}
> +
>   static const struct v4l2_subdev_video_ops adv7180_video_ops = {
>   	.querystd = adv7180_querystd,
>   	.g_input_status = adv7180_g_input_status,
>   	.s_routing = adv7180_s_routing,
> +	.enum_mbus_fmt = adv7180_enum_mbus_fmt,
> +	.try_mbus_fmt = adv7180_try_mbus_fmt,
> +	.g_mbus_fmt = adv7180_g_mbus_fmt,
> +	.s_mbus_fmt = adv7180_s_mbus_fmt,
> +	.cropcap = adv7180_cropcap,
> +	.g_mbus_config = adv7180_g_mbus_config,
>   };
>   
>   static const struct v4l2_subdev_core_ops adv7180_core_ops = {

WBR, Sergei

Re: [PATCH v3] adv7180: add more subdev video ops

[Hans Verkuil]

Hi Sergei,

Sorry for the delay, I had planned to work on this during the weekend, but
I became ill.

On Mon 13 May 2013 21:21:39 Sergei Shtylyov wrote:
> From: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
> 
> Add subdev video ops for ADV7180 video decoder.  This makes decoder usable on
> the soc-camera drivers.
> 
> Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> ---
> This patch is against the 'media_tree.git' repo.
> 
> Changes from version 2:
> - set the field format depending on video standard in try_mbus_fmt() method;
> - removed querystd() method calls from try_mbus_fmt() and cropcap() methods;
> - removed g_crop() method.

OK, it looks good with one exception: since no cropping is supported, cropcap
can be dropped as well. cropcap is only needed in combination with cropping
support.

Regards,

	Hans

> 
>  drivers/media/i2c/adv7180.c |   86 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 86 insertions(+)
> 
> Index: media_tree/drivers/media/i2c/adv7180.c
> =================================> --- media_tree.orig/drivers/media/i2c/adv7180.c
> +++ media_tree/drivers/media/i2c/adv7180.c
> @@ -1,6 +1,8 @@
>  /*
>   * adv7180.c Analog Devices ADV7180 video decoder driver
>   * Copyright (c) 2009 Intel Corporation
> + * Copyright (C) 2013 Cogent Embedded, Inc.
> + * Copyright (C) 2013 Renesas Solutions Corp.
>   *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License version 2 as
> @@ -128,6 +130,7 @@ struct adv7180_state {
>  	v4l2_std_id		curr_norm;
>  	bool			autodetect;
>  	u8			input;
> +	struct v4l2_mbus_framefmt fmt;
>  };
>  #define to_adv7180_sd(_ctrl) (&container_of(_ctrl->handler,		\
>  					    struct adv7180_state,	\
> @@ -397,10 +400,93 @@ static void adv7180_exit_controls(struct
>  	v4l2_ctrl_handler_free(&state->ctrl_hdl);
>  }
>  
> +static int adv7180_enum_mbus_fmt(struct v4l2_subdev *sd, unsigned int index,
> +				 enum v4l2_mbus_pixelcode *code)
> +{
> +	if (index > 0)
> +		return -EINVAL;
> +
> +	*code = V4L2_MBUS_FMT_YUYV8_2X8;
> +
> +	return 0;
> +}
> +
> +static int adv7180_try_mbus_fmt(struct v4l2_subdev *sd,
> +				struct v4l2_mbus_framefmt *fmt)
> +{
> +	struct adv7180_state *state = to_state(sd);
> +
> +	fmt->code = V4L2_MBUS_FMT_YUYV8_2X8;
> +	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
> +	fmt->field = state->curr_norm & V4L2_STD_525_60 ?
> +		     V4L2_FIELD_INTERLACED_BT : V4L2_FIELD_INTERLACED_TB;
> +	fmt->width = 720;
> +	fmt->height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
> +
> +	return 0;
> +}
> +
> +static int adv7180_g_mbus_fmt(struct v4l2_subdev *sd,
> +			      struct v4l2_mbus_framefmt *fmt)
> +{
> +	struct adv7180_state *state = to_state(sd);
> +
> +	*fmt = state->fmt;
> +
> +	return 0;
> +}
> +
> +static int adv7180_s_mbus_fmt(struct v4l2_subdev *sd,
> +			      struct v4l2_mbus_framefmt *fmt)
> +{
> +	struct adv7180_state *state = to_state(sd);
> +
> +	adv7180_try_mbus_fmt(sd, fmt);
> +	state->fmt = *fmt;
> +
> +	return 0;
> +}
> +
> +static int adv7180_cropcap(struct v4l2_subdev *sd, struct v4l2_cropcap *a)
> +{
> +	struct adv7180_state *state = to_state(sd);
> +
> +	a->bounds.left = 0;
> +	a->bounds.top = 0;
> +	a->bounds.width = 720;
> +	a->bounds.height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
> +	a->defrect = a->bounds;
> +	a->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> +	a->pixelaspect.numerator = 1;
> +	a->pixelaspect.denominator = 1;
> +
> +	return 0;
> +}
> +
> +static int adv7180_g_mbus_config(struct v4l2_subdev *sd,
> +				 struct v4l2_mbus_config *cfg)
> +{
> +	/*
> +	 * The ADV7180 sensor supports BT.601/656 output modes.
> +	 * The BT.656 is default and not yet configurable by s/w.
> +	 */
> +	cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING |
> +		     V4L2_MBUS_DATA_ACTIVE_HIGH;
> +	cfg->type = V4L2_MBUS_BT656;
> +
> +	return 0;
> +}
> +
>  static const struct v4l2_subdev_video_ops adv7180_video_ops = {
>  	.querystd = adv7180_querystd,
>  	.g_input_status = adv7180_g_input_status,
>  	.s_routing = adv7180_s_routing,
> +	.enum_mbus_fmt = adv7180_enum_mbus_fmt,
> +	.try_mbus_fmt = adv7180_try_mbus_fmt,
> +	.g_mbus_fmt = adv7180_g_mbus_fmt,
> +	.s_mbus_fmt = adv7180_s_mbus_fmt,
> +	.cropcap = adv7180_cropcap,
> +	.g_mbus_config = adv7180_g_mbus_config,
>  };
>  
>  static const struct v4l2_subdev_core_ops adv7180_core_ops = {
> --
> 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 v3] adv7180: add more subdev video ops

[Hans Verkuil]

Hi Sergei,

Sorry for the delay, I had planned to work on this during the weekend, but
I became ill.

On Mon 13 May 2013 21:21:39 Sergei Shtylyov wrote:
> From: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
> 
> Add subdev video ops for ADV7180 video decoder.  This makes decoder usable on
> the soc-camera drivers.
> 
> Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> ---
> This patch is against the 'media_tree.git' repo.
> 
> Changes from version 2:
> - set the field format depending on video standard in try_mbus_fmt() method;
> - removed querystd() method calls from try_mbus_fmt() and cropcap() methods;
> - removed g_crop() method.

OK, it looks good with one exception: since no cropping is supported, cropcap
can be dropped as well. cropcap is only needed in combination with cropping
support.

Regards,

	Hans

> 
>  drivers/media/i2c/adv7180.c |   86 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 86 insertions(+)
> 
> Index: media_tree/drivers/media/i2c/adv7180.c
> ===================================================================
> --- media_tree.orig/drivers/media/i2c/adv7180.c
> +++ media_tree/drivers/media/i2c/adv7180.c
> @@ -1,6 +1,8 @@
>  /*
>   * adv7180.c Analog Devices ADV7180 video decoder driver
>   * Copyright (c) 2009 Intel Corporation
> + * Copyright (C) 2013 Cogent Embedded, Inc.
> + * Copyright (C) 2013 Renesas Solutions Corp.
>   *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License version 2 as
> @@ -128,6 +130,7 @@ struct adv7180_state {
>  	v4l2_std_id		curr_norm;
>  	bool			autodetect;
>  	u8			input;
> +	struct v4l2_mbus_framefmt fmt;
>  };
>  #define to_adv7180_sd(_ctrl) (&container_of(_ctrl->handler,		\
>  					    struct adv7180_state,	\
> @@ -397,10 +400,93 @@ static void adv7180_exit_controls(struct
>  	v4l2_ctrl_handler_free(&state->ctrl_hdl);
>  }
>  
> +static int adv7180_enum_mbus_fmt(struct v4l2_subdev *sd, unsigned int index,
> +				 enum v4l2_mbus_pixelcode *code)
> +{
> +	if (index > 0)
> +		return -EINVAL;
> +
> +	*code = V4L2_MBUS_FMT_YUYV8_2X8;
> +
> +	return 0;
> +}
> +
> +static int adv7180_try_mbus_fmt(struct v4l2_subdev *sd,
> +				struct v4l2_mbus_framefmt *fmt)
> +{
> +	struct adv7180_state *state = to_state(sd);
> +
> +	fmt->code = V4L2_MBUS_FMT_YUYV8_2X8;
> +	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
> +	fmt->field = state->curr_norm & V4L2_STD_525_60 ?
> +		     V4L2_FIELD_INTERLACED_BT : V4L2_FIELD_INTERLACED_TB;
> +	fmt->width = 720;
> +	fmt->height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
> +
> +	return 0;
> +}
> +
> +static int adv7180_g_mbus_fmt(struct v4l2_subdev *sd,
> +			      struct v4l2_mbus_framefmt *fmt)
> +{
> +	struct adv7180_state *state = to_state(sd);
> +
> +	*fmt = state->fmt;
> +
> +	return 0;
> +}
> +
> +static int adv7180_s_mbus_fmt(struct v4l2_subdev *sd,
> +			      struct v4l2_mbus_framefmt *fmt)
> +{
> +	struct adv7180_state *state = to_state(sd);
> +
> +	adv7180_try_mbus_fmt(sd, fmt);
> +	state->fmt = *fmt;
> +
> +	return 0;
> +}
> +
> +static int adv7180_cropcap(struct v4l2_subdev *sd, struct v4l2_cropcap *a)
> +{
> +	struct adv7180_state *state = to_state(sd);
> +
> +	a->bounds.left = 0;
> +	a->bounds.top = 0;
> +	a->bounds.width = 720;
> +	a->bounds.height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
> +	a->defrect = a->bounds;
> +	a->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> +	a->pixelaspect.numerator = 1;
> +	a->pixelaspect.denominator = 1;
> +
> +	return 0;
> +}
> +
> +static int adv7180_g_mbus_config(struct v4l2_subdev *sd,
> +				 struct v4l2_mbus_config *cfg)
> +{
> +	/*
> +	 * The ADV7180 sensor supports BT.601/656 output modes.
> +	 * The BT.656 is default and not yet configurable by s/w.
> +	 */
> +	cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING |
> +		     V4L2_MBUS_DATA_ACTIVE_HIGH;
> +	cfg->type = V4L2_MBUS_BT656;
> +
> +	return 0;
> +}
> +
>  static const struct v4l2_subdev_video_ops adv7180_video_ops = {
>  	.querystd = adv7180_querystd,
>  	.g_input_status = adv7180_g_input_status,
>  	.s_routing = adv7180_s_routing,
> +	.enum_mbus_fmt = adv7180_enum_mbus_fmt,
> +	.try_mbus_fmt = adv7180_try_mbus_fmt,
> +	.g_mbus_fmt = adv7180_g_mbus_fmt,
> +	.s_mbus_fmt = adv7180_s_mbus_fmt,
> +	.cropcap = adv7180_cropcap,
> +	.g_mbus_config = adv7180_g_mbus_config,
>  };
>  
>  static const struct v4l2_subdev_core_ops adv7180_core_ops = {
> --
> 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 v3] adv7180: add more subdev video ops

[Hans Verkuil]

On Mon 13 May 2013 21:21:39 Sergei Shtylyov wrote:
> From: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
> 
> Add subdev video ops for ADV7180 video decoder.  This makes decoder usable on
> the soc-camera drivers.
> 
> Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> ---
> This patch is against the 'media_tree.git' repo.
> 
> Changes from version 2:
> - set the field format depending on video standard in try_mbus_fmt() method;
> - removed querystd() method calls from try_mbus_fmt() and cropcap() methods;
> - removed g_crop() method.
> 
>  drivers/media/i2c/adv7180.c |   86 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 86 insertions(+)
> 
> Index: media_tree/drivers/media/i2c/adv7180.c
> =================================> --- media_tree.orig/drivers/media/i2c/adv7180.c
> +++ media_tree/drivers/media/i2c/adv7180.c


> +
> +static int adv7180_try_mbus_fmt(struct v4l2_subdev *sd,
> +				struct v4l2_mbus_framefmt *fmt)
> +{
> +	struct adv7180_state *state = to_state(sd);
> +
> +	fmt->code = V4L2_MBUS_FMT_YUYV8_2X8;
> +	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
> +	fmt->field = state->curr_norm & V4L2_STD_525_60 ?
> +		     V4L2_FIELD_INTERLACED_BT : V4L2_FIELD_INTERLACED_TB;

Just noticed this: use V4L2_FIELD_INTERLACED as that does the right thing.
No need to split in _BT and _TB.

> +	fmt->width = 720;
> +	fmt->height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
> +
> +	return 0;
> +}

Actually, all this code can be simplified substantially: the try/g/s_mbus_fmt
functions are really all identical since the data returned is only dependent
on the current standard. So this means you can use just a single function for
all three ops, and you can do away with adding struct v4l2_mbus_framefmt to
adv7180_state.

Regards,

	Hans

Re: [PATCH v3] adv7180: add more subdev video ops

[Hans Verkuil]

On Mon 13 May 2013 21:21:39 Sergei Shtylyov wrote:
> From: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
> 
> Add subdev video ops for ADV7180 video decoder.  This makes decoder usable on
> the soc-camera drivers.
> 
> Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> ---
> This patch is against the 'media_tree.git' repo.
> 
> Changes from version 2:
> - set the field format depending on video standard in try_mbus_fmt() method;
> - removed querystd() method calls from try_mbus_fmt() and cropcap() methods;
> - removed g_crop() method.
> 
>  drivers/media/i2c/adv7180.c |   86 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 86 insertions(+)
> 
> Index: media_tree/drivers/media/i2c/adv7180.c
> ===================================================================
> --- media_tree.orig/drivers/media/i2c/adv7180.c
> +++ media_tree/drivers/media/i2c/adv7180.c


> +
> +static int adv7180_try_mbus_fmt(struct v4l2_subdev *sd,
> +				struct v4l2_mbus_framefmt *fmt)
> +{
> +	struct adv7180_state *state = to_state(sd);
> +
> +	fmt->code = V4L2_MBUS_FMT_YUYV8_2X8;
> +	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
> +	fmt->field = state->curr_norm & V4L2_STD_525_60 ?
> +		     V4L2_FIELD_INTERLACED_BT : V4L2_FIELD_INTERLACED_TB;

Just noticed this: use V4L2_FIELD_INTERLACED as that does the right thing.
No need to split in _BT and _TB.

> +	fmt->width = 720;
> +	fmt->height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
> +
> +	return 0;
> +}

Actually, all this code can be simplified substantially: the try/g/s_mbus_fmt
functions are really all identical since the data returned is only dependent
on the current standard. So this means you can use just a single function for
all three ops, and you can do away with adding struct v4l2_mbus_framefmt to
adv7180_state.

Regards,

	Hans

Re: [PATCH v3] adv7180: add more subdev video ops

[Sergei Shtylyov]

Hello.

On 21-05-2013 13:35, Hans Verkuil wrote:

>> From: Vladimir Barinov <vladimir.barinov@cogentembedded.com>

>> Add subdev video ops for ADV7180 video decoder.  This makes decoder usable on
>> the soc-camera drivers.

>> Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

>> ---
>> This patch is against the 'media_tree.git' repo.

>> Changes from version 2:
>> - set the field format depending on video standard in try_mbus_fmt() method;
>> - removed querystd() method calls from try_mbus_fmt() and cropcap() methods;
>> - removed g_crop() method.

>>   drivers/media/i2c/adv7180.c |   86 ++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 86 insertions(+)
>>
>> Index: media_tree/drivers/media/i2c/adv7180.c
>> =================================>> --- media_tree.orig/drivers/media/i2c/adv7180.c
>> +++ media_tree/drivers/media/i2c/adv7180.c


>> +
>> +static int adv7180_try_mbus_fmt(struct v4l2_subdev *sd,
>> +				struct v4l2_mbus_framefmt *fmt)
>> +{
>> +	struct adv7180_state *state = to_state(sd);
>> +
>> +	fmt->code = V4L2_MBUS_FMT_YUYV8_2X8;
>> +	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
>> +	fmt->field = state->curr_norm & V4L2_STD_525_60 ?
>> +		     V4L2_FIELD_INTERLACED_BT : V4L2_FIELD_INTERLACED_TB;

> Just noticed this: use V4L2_FIELD_INTERLACED as that does the right thing.
> No need to split in _BT and _TB.

    Hm, testers have reported that _BT vs _TB do make a difference. I'll 
try to look into how V4L2 handles interlacing for different standards.

>> +	fmt->width = 720;
>> +	fmt->height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
>> +
>> +	return 0;
>> +}

> Actually, all this code can be simplified substantially: the try/g/s_mbus_fmt
> functions are really all identical since the data returned is only dependent
> on the current standard. So this means you can use just a single function for
> all three ops, and you can do away with adding struct v4l2_mbus_framefmt to
> adv7180_state.

    Hm, I wonder how "get" and "set" methods can be identical... I'll 
look into this some more.

> Regards,

> 	Hans

WBR, Sergei

Re: [PATCH v3] adv7180: add more subdev video ops

[Sergei Shtylyov]

Hello.

On 21-05-2013 13:35, Hans Verkuil wrote:

>> From: Vladimir Barinov <vladimir.barinov@cogentembedded.com>

>> Add subdev video ops for ADV7180 video decoder.  This makes decoder usable on
>> the soc-camera drivers.

>> Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

>> ---
>> This patch is against the 'media_tree.git' repo.

>> Changes from version 2:
>> - set the field format depending on video standard in try_mbus_fmt() method;
>> - removed querystd() method calls from try_mbus_fmt() and cropcap() methods;
>> - removed g_crop() method.

>>   drivers/media/i2c/adv7180.c |   86 ++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 86 insertions(+)
>>
>> Index: media_tree/drivers/media/i2c/adv7180.c
>> ===================================================================
>> --- media_tree.orig/drivers/media/i2c/adv7180.c
>> +++ media_tree/drivers/media/i2c/adv7180.c


>> +
>> +static int adv7180_try_mbus_fmt(struct v4l2_subdev *sd,
>> +				struct v4l2_mbus_framefmt *fmt)
>> +{
>> +	struct adv7180_state *state = to_state(sd);
>> +
>> +	fmt->code = V4L2_MBUS_FMT_YUYV8_2X8;
>> +	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
>> +	fmt->field = state->curr_norm & V4L2_STD_525_60 ?
>> +		     V4L2_FIELD_INTERLACED_BT : V4L2_FIELD_INTERLACED_TB;

> Just noticed this: use V4L2_FIELD_INTERLACED as that does the right thing.
> No need to split in _BT and _TB.

    Hm, testers have reported that _BT vs _TB do make a difference. I'll 
try to look into how V4L2 handles interlacing for different standards.

>> +	fmt->width = 720;
>> +	fmt->height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
>> +
>> +	return 0;
>> +}

> Actually, all this code can be simplified substantially: the try/g/s_mbus_fmt
> functions are really all identical since the data returned is only dependent
> on the current standard. So this means you can use just a single function for
> all three ops, and you can do away with adding struct v4l2_mbus_framefmt to
> adv7180_state.

    Hm, I wonder how "get" and "set" methods can be identical... I'll 
look into this some more.

> Regards,

> 	Hans

WBR, Sergei

Re: [PATCH v3] adv7180: add more subdev video ops

[Hans Verkuil]

On Tue 21 May 2013 16:28:08 Sergei Shtylyov wrote:
> Hello.
> 
> On 21-05-2013 13:35, Hans Verkuil wrote:
> 
> >> From: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
> 
> >> Add subdev video ops for ADV7180 video decoder.  This makes decoder usable on
> >> the soc-camera drivers.
> 
> >> Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
> >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> >> ---
> >> This patch is against the 'media_tree.git' repo.
> 
> >> Changes from version 2:
> >> - set the field format depending on video standard in try_mbus_fmt() method;
> >> - removed querystd() method calls from try_mbus_fmt() and cropcap() methods;
> >> - removed g_crop() method.
> 
> >>   drivers/media/i2c/adv7180.c |   86 ++++++++++++++++++++++++++++++++++++++++++++
> >>   1 file changed, 86 insertions(+)
> >>
> >> Index: media_tree/drivers/media/i2c/adv7180.c
> >> =================================> >> --- media_tree.orig/drivers/media/i2c/adv7180.c
> >> +++ media_tree/drivers/media/i2c/adv7180.c
> 
> 
> >> +
> >> +static int adv7180_try_mbus_fmt(struct v4l2_subdev *sd,
> >> +				struct v4l2_mbus_framefmt *fmt)
> >> +{
> >> +	struct adv7180_state *state = to_state(sd);
> >> +
> >> +	fmt->code = V4L2_MBUS_FMT_YUYV8_2X8;
> >> +	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
> >> +	fmt->field = state->curr_norm & V4L2_STD_525_60 ?
> >> +		     V4L2_FIELD_INTERLACED_BT : V4L2_FIELD_INTERLACED_TB;
> 
> > Just noticed this: use V4L2_FIELD_INTERLACED as that does the right thing.
> > No need to split in _BT and _TB.
> 
>     Hm, testers have reported that _BT vs _TB do make a difference. I'll 
> try to look into how V4L2 handles interlacing for different standards.

When using V4L2_FIELD_INTERLACED the BT vs TB is implicit (i.e. the application
is supposed to know that the order will be different depending on the standard).
Explicitly using BT/TB is only useful if you want to override the default, e.g.
if a video was encoded using the wrong temporal order.
 
> >> +	fmt->width = 720;
> >> +	fmt->height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
> >> +
> >> +	return 0;
> >> +}
> 
> > Actually, all this code can be simplified substantially: the try/g/s_mbus_fmt
> > functions are really all identical since the data returned is only dependent
> > on the current standard. So this means you can use just a single function for
> > all three ops, and you can do away with adding struct v4l2_mbus_framefmt to
> > adv7180_state.
> 
>     Hm, I wonder how "get" and "set" methods can be identical... I'll 
> look into this some more.

Normally they aren't, but in this case there is only one possible format, so
any values you attempt to set are ignored completely.

Regards,

	Hans

Re: [PATCH v3] adv7180: add more subdev video ops

[Hans Verkuil]

On Tue 21 May 2013 16:28:08 Sergei Shtylyov wrote:
> Hello.
> 
> On 21-05-2013 13:35, Hans Verkuil wrote:
> 
> >> From: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
> 
> >> Add subdev video ops for ADV7180 video decoder.  This makes decoder usable on
> >> the soc-camera drivers.
> 
> >> Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
> >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> >> ---
> >> This patch is against the 'media_tree.git' repo.
> 
> >> Changes from version 2:
> >> - set the field format depending on video standard in try_mbus_fmt() method;
> >> - removed querystd() method calls from try_mbus_fmt() and cropcap() methods;
> >> - removed g_crop() method.
> 
> >>   drivers/media/i2c/adv7180.c |   86 ++++++++++++++++++++++++++++++++++++++++++++
> >>   1 file changed, 86 insertions(+)
> >>
> >> Index: media_tree/drivers/media/i2c/adv7180.c
> >> ===================================================================
> >> --- media_tree.orig/drivers/media/i2c/adv7180.c
> >> +++ media_tree/drivers/media/i2c/adv7180.c
> 
> 
> >> +
> >> +static int adv7180_try_mbus_fmt(struct v4l2_subdev *sd,
> >> +				struct v4l2_mbus_framefmt *fmt)
> >> +{
> >> +	struct adv7180_state *state = to_state(sd);
> >> +
> >> +	fmt->code = V4L2_MBUS_FMT_YUYV8_2X8;
> >> +	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
> >> +	fmt->field = state->curr_norm & V4L2_STD_525_60 ?
> >> +		     V4L2_FIELD_INTERLACED_BT : V4L2_FIELD_INTERLACED_TB;
> 
> > Just noticed this: use V4L2_FIELD_INTERLACED as that does the right thing.
> > No need to split in _BT and _TB.
> 
>     Hm, testers have reported that _BT vs _TB do make a difference. I'll 
> try to look into how V4L2 handles interlacing for different standards.

When using V4L2_FIELD_INTERLACED the BT vs TB is implicit (i.e. the application
is supposed to know that the order will be different depending on the standard).
Explicitly using BT/TB is only useful if you want to override the default, e.g.
if a video was encoded using the wrong temporal order.
 
> >> +	fmt->width = 720;
> >> +	fmt->height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
> >> +
> >> +	return 0;
> >> +}
> 
> > Actually, all this code can be simplified substantially: the try/g/s_mbus_fmt
> > functions are really all identical since the data returned is only dependent
> > on the current standard. So this means you can use just a single function for
> > all three ops, and you can do away with adding struct v4l2_mbus_framefmt to
> > adv7180_state.
> 
>     Hm, I wonder how "get" and "set" methods can be identical... I'll 
> look into this some more.

Normally they aren't, but in this case there is only one possible format, so
any values you attempt to set are ignored completely.

Regards,

	Hans

Re: [PATCH v3] adv7180: add more subdev video ops

[Sergei Shtylyov]

Hello.

On 05/21/2013 06:45 PM, Hans Verkuil wrote:

>>>> From: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
>>>> Add subdev video ops for ADV7180 video decoder.  This makes decoder usable on
>>>> the soc-camera drivers.
>>>> Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>>> ---
>>>> This patch is against the 'media_tree.git' repo.
>>>> Changes from version 2:
>>>> - set the field format depending on video standard in try_mbus_fmt() method;
>>>> - removed querystd() method calls from try_mbus_fmt() and cropcap() methods;
>>>> - removed g_crop() method.
>>>>    drivers/media/i2c/adv7180.c |   86 ++++++++++++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 86 insertions(+)
>>>>
>>>> Index: media_tree/drivers/media/i2c/adv7180.c
>>>> =================================>>>> --- media_tree.orig/drivers/media/i2c/adv7180.c
>>>> +++ media_tree/drivers/media/i2c/adv7180.c
>>
>>>> +
>>>> +static int adv7180_try_mbus_fmt(struct v4l2_subdev *sd,
>>>> +				struct v4l2_mbus_framefmt *fmt)
>>>> +{
>>>> +	struct adv7180_state *state = to_state(sd);
>>>> +
>>>> +	fmt->code = V4L2_MBUS_FMT_YUYV8_2X8;
>>>> +	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
>>>> +	fmt->field = state->curr_norm & V4L2_STD_525_60 ?
>>>> +		     V4L2_FIELD_INTERLACED_BT : V4L2_FIELD_INTERLACED_TB;
>>> Just noticed this: use V4L2_FIELD_INTERLACED as that does the right thing.
>>> No need to split in _BT and _TB.
>>      Hm, testers have reported that _BT vs _TB do make a difference. I'll
>> try to look into how V4L2 handles interlacing for different standards.
> When using V4L2_FIELD_INTERLACED the BT vs TB is implicit (i.e. the application
> is supposed to know that the order will be different depending on the standard).
> Explicitly using BT/TB is only useful if you want to override the default, e.g.
> if a video was encoded using the wrong temporal order.

     We have used V4L2_FIELD_INTERLACED before and people reported
incorrect field ordering -- that's why we changed to what it is now. So this
might be an application failure?

WBR, Sergei

Re: [PATCH v3] adv7180: add more subdev video ops

[Sergei Shtylyov]

Hello.

On 05/21/2013 06:45 PM, Hans Verkuil wrote:

>>>> From: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
>>>> Add subdev video ops for ADV7180 video decoder.  This makes decoder usable on
>>>> the soc-camera drivers.
>>>> Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>>> ---
>>>> This patch is against the 'media_tree.git' repo.
>>>> Changes from version 2:
>>>> - set the field format depending on video standard in try_mbus_fmt() method;
>>>> - removed querystd() method calls from try_mbus_fmt() and cropcap() methods;
>>>> - removed g_crop() method.
>>>>    drivers/media/i2c/adv7180.c |   86 ++++++++++++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 86 insertions(+)
>>>>
>>>> Index: media_tree/drivers/media/i2c/adv7180.c
>>>> ===================================================================
>>>> --- media_tree.orig/drivers/media/i2c/adv7180.c
>>>> +++ media_tree/drivers/media/i2c/adv7180.c
>>
>>>> +
>>>> +static int adv7180_try_mbus_fmt(struct v4l2_subdev *sd,
>>>> +				struct v4l2_mbus_framefmt *fmt)
>>>> +{
>>>> +	struct adv7180_state *state = to_state(sd);
>>>> +
>>>> +	fmt->code = V4L2_MBUS_FMT_YUYV8_2X8;
>>>> +	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
>>>> +	fmt->field = state->curr_norm & V4L2_STD_525_60 ?
>>>> +		     V4L2_FIELD_INTERLACED_BT : V4L2_FIELD_INTERLACED_TB;
>>> Just noticed this: use V4L2_FIELD_INTERLACED as that does the right thing.
>>> No need to split in _BT and _TB.
>>      Hm, testers have reported that _BT vs _TB do make a difference. I'll
>> try to look into how V4L2 handles interlacing for different standards.
> When using V4L2_FIELD_INTERLACED the BT vs TB is implicit (i.e. the application
> is supposed to know that the order will be different depending on the standard).
> Explicitly using BT/TB is only useful if you want to override the default, e.g.
> if a video was encoded using the wrong temporal order.

     We have used V4L2_FIELD_INTERLACED before and people reported
incorrect field ordering -- that's why we changed to what it is now. So this
might be an application failure?

WBR, Sergei

Re: [PATCH v3] adv7180: add more subdev video ops

[Hans Verkuil]

On Tue May 21 2013 19:17:09 Sergei Shtylyov wrote:
> Hello.
> 
> On 05/21/2013 06:45 PM, Hans Verkuil wrote:
> 
> >>>> From: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
> >>>> Add subdev video ops for ADV7180 video decoder.  This makes decoder usable on
> >>>> the soc-camera drivers.
> >>>> Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
> >>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> >>>> ---
> >>>> This patch is against the 'media_tree.git' repo.
> >>>> Changes from version 2:
> >>>> - set the field format depending on video standard in try_mbus_fmt() method;
> >>>> - removed querystd() method calls from try_mbus_fmt() and cropcap() methods;
> >>>> - removed g_crop() method.
> >>>>    drivers/media/i2c/adv7180.c |   86 ++++++++++++++++++++++++++++++++++++++++++++
> >>>>    1 file changed, 86 insertions(+)
> >>>>
> >>>> Index: media_tree/drivers/media/i2c/adv7180.c
> >>>> =================================> >>>> --- media_tree.orig/drivers/media/i2c/adv7180.c
> >>>> +++ media_tree/drivers/media/i2c/adv7180.c
> >>
> >>>> +
> >>>> +static int adv7180_try_mbus_fmt(struct v4l2_subdev *sd,
> >>>> +				struct v4l2_mbus_framefmt *fmt)
> >>>> +{
> >>>> +	struct adv7180_state *state = to_state(sd);
> >>>> +
> >>>> +	fmt->code = V4L2_MBUS_FMT_YUYV8_2X8;
> >>>> +	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
> >>>> +	fmt->field = state->curr_norm & V4L2_STD_525_60 ?
> >>>> +		     V4L2_FIELD_INTERLACED_BT : V4L2_FIELD_INTERLACED_TB;
> >>> Just noticed this: use V4L2_FIELD_INTERLACED as that does the right thing.
> >>> No need to split in _BT and _TB.
> >>      Hm, testers have reported that _BT vs _TB do make a difference. I'll
> >> try to look into how V4L2 handles interlacing for different standards.
> > When using V4L2_FIELD_INTERLACED the BT vs TB is implicit (i.e. the application
> > is supposed to know that the order will be different depending on the standard).
> > Explicitly using BT/TB is only useful if you want to override the default, e.g.
> > if a video was encoded using the wrong temporal order.
> 
>      We have used V4L2_FIELD_INTERLACED before and people reported
> incorrect field ordering -- that's why we changed to what it is now. So this
> might be an application failure?

I've fairly certain it is, yes. The spec says this about FIELD_INTERLACED:

"Images contain both fields, interleaved line by line. The temporal order of the
 fields (whether the top or bottom field is first transmitted) depends on the
 current video standard. M/NTSC transmits the bottom field first, all other
 standards the top field first."

So if the application needs to know the temporal order, it will have to look
at the current video standard.

Regards,

	Hans

Re: [PATCH v3] adv7180: add more subdev video ops

[Hans Verkuil]

On Tue May 21 2013 19:17:09 Sergei Shtylyov wrote:
> Hello.
> 
> On 05/21/2013 06:45 PM, Hans Verkuil wrote:
> 
> >>>> From: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
> >>>> Add subdev video ops for ADV7180 video decoder.  This makes decoder usable on
> >>>> the soc-camera drivers.
> >>>> Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
> >>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> >>>> ---
> >>>> This patch is against the 'media_tree.git' repo.
> >>>> Changes from version 2:
> >>>> - set the field format depending on video standard in try_mbus_fmt() method;
> >>>> - removed querystd() method calls from try_mbus_fmt() and cropcap() methods;
> >>>> - removed g_crop() method.
> >>>>    drivers/media/i2c/adv7180.c |   86 ++++++++++++++++++++++++++++++++++++++++++++
> >>>>    1 file changed, 86 insertions(+)
> >>>>
> >>>> Index: media_tree/drivers/media/i2c/adv7180.c
> >>>> ===================================================================
> >>>> --- media_tree.orig/drivers/media/i2c/adv7180.c
> >>>> +++ media_tree/drivers/media/i2c/adv7180.c
> >>
> >>>> +
> >>>> +static int adv7180_try_mbus_fmt(struct v4l2_subdev *sd,
> >>>> +				struct v4l2_mbus_framefmt *fmt)
> >>>> +{
> >>>> +	struct adv7180_state *state = to_state(sd);
> >>>> +
> >>>> +	fmt->code = V4L2_MBUS_FMT_YUYV8_2X8;
> >>>> +	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
> >>>> +	fmt->field = state->curr_norm & V4L2_STD_525_60 ?
> >>>> +		     V4L2_FIELD_INTERLACED_BT : V4L2_FIELD_INTERLACED_TB;
> >>> Just noticed this: use V4L2_FIELD_INTERLACED as that does the right thing.
> >>> No need to split in _BT and _TB.
> >>      Hm, testers have reported that _BT vs _TB do make a difference. I'll
> >> try to look into how V4L2 handles interlacing for different standards.
> > When using V4L2_FIELD_INTERLACED the BT vs TB is implicit (i.e. the application
> > is supposed to know that the order will be different depending on the standard).
> > Explicitly using BT/TB is only useful if you want to override the default, e.g.
> > if a video was encoded using the wrong temporal order.
> 
>      We have used V4L2_FIELD_INTERLACED before and people reported
> incorrect field ordering -- that's why we changed to what it is now. So this
> might be an application failure?

I've fairly certain it is, yes. The spec says this about FIELD_INTERLACED:

"Images contain both fields, interleaved line by line. The temporal order of the
 fields (whether the top or bottom field is first transmitted) depends on the
 current video standard. M/NTSC transmits the bottom field first, all other
 standards the top field first."

So if the application needs to know the temporal order, it will have to look
at the current video standard.

Regards,

	Hans

Re: [PATCH v3] adv7180: add more subdev video ops

[Sergei Shtylyov]

Hello.

On 22-05-2013 10:55, Hans Verkuil wrote:

>>>>>> From: Vladimir Barinov <vladimir.barinov@cogentembedded.com>

>>>>>> Add subdev video ops for ADV7180 video decoder.  This makes decoder usable on
>>>>>> the soc-camera drivers.

>>>>>> Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
>>>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

>>>>>> ---
>>>>>> This patch is against the 'media_tree.git' repo.

>>>>>> Changes from version 2:
>>>>>> - set the field format depending on video standard in try_mbus_fmt() method;
>>>>>> - removed querystd() method calls from try_mbus_fmt() and cropcap() methods;
>>>>>> - removed g_crop() method.

>>>>>>     drivers/media/i2c/adv7180.c |   86 ++++++++++++++++++++++++++++++++++++++++++++
>>>>>>     1 file changed, 86 insertions(+)

>>>>>> Index: media_tree/drivers/media/i2c/adv7180.c
>>>>>> =================================>>>>>> --- media_tree.orig/drivers/media/i2c/adv7180.c
>>>>>> +++ media_tree/drivers/media/i2c/adv7180.c

>>>>>> +
>>>>>> +static int adv7180_try_mbus_fmt(struct v4l2_subdev *sd,
>>>>>> +				struct v4l2_mbus_framefmt *fmt)
>>>>>> +{
>>>>>> +	struct adv7180_state *state = to_state(sd);
>>>>>> +
>>>>>> +	fmt->code = V4L2_MBUS_FMT_YUYV8_2X8;
>>>>>> +	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
>>>>>> +	fmt->field = state->curr_norm & V4L2_STD_525_60 ?
>>>>>> +		     V4L2_FIELD_INTERLACED_BT : V4L2_FIELD_INTERLACED_TB;

>>>>> Just noticed this: use V4L2_FIELD_INTERLACED as that does the right thing.
>>>>> No need to split in _BT and _TB.

>>>>       Hm, testers have reported that _BT vs _TB do make a difference. I'll
>>>> try to look into how V4L2 handles interlacing for different standards.
>>> When using V4L2_FIELD_INTERLACED the BT vs TB is implicit (i.e. the application
>>> is supposed to know that the order will be different depending on the standard).
>>> Explicitly using BT/TB is only useful if you want to override the default, e.g.
>>> if a video was encoded using the wrong temporal order.

>>       We have used V4L2_FIELD_INTERLACED before and people reported
>> incorrect field ordering -- that's why we changed to what it is now. So this
>> might be an application failure?

> I've fairly certain it is, yes. The spec says this about FIELD_INTERLACED:

> "Images contain both fields, interleaved line by line. The temporal order of the
>   fields (whether the top or bottom field is first transmitted) depends on the
>   current video standard. M/NTSC transmits the bottom field first, all other
>   standards the top field first."

> So if the application needs to know the temporal order, it will have to look
> at the current video standard.

    It appeared that it's our R-Car VIN soc_camera driver that misses 
this video standard check. It just always treats V4L2_FIELD_INTERLACED 
the same as V4L2_FIELD_INTERLACED_TB when programming the video mode 
control register.

> Regards,

> 	Hans

WBR, Sergei

Re: [PATCH v3] adv7180: add more subdev video ops

[Sergei Shtylyov]

Hello.

On 22-05-2013 10:55, Hans Verkuil wrote:

>>>>>> From: Vladimir Barinov <vladimir.barinov@cogentembedded.com>

>>>>>> Add subdev video ops for ADV7180 video decoder.  This makes decoder usable on
>>>>>> the soc-camera drivers.

>>>>>> Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
>>>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

>>>>>> ---
>>>>>> This patch is against the 'media_tree.git' repo.

>>>>>> Changes from version 2:
>>>>>> - set the field format depending on video standard in try_mbus_fmt() method;
>>>>>> - removed querystd() method calls from try_mbus_fmt() and cropcap() methods;
>>>>>> - removed g_crop() method.

>>>>>>     drivers/media/i2c/adv7180.c |   86 ++++++++++++++++++++++++++++++++++++++++++++
>>>>>>     1 file changed, 86 insertions(+)

>>>>>> Index: media_tree/drivers/media/i2c/adv7180.c
>>>>>> ===================================================================
>>>>>> --- media_tree.orig/drivers/media/i2c/adv7180.c
>>>>>> +++ media_tree/drivers/media/i2c/adv7180.c

>>>>>> +
>>>>>> +static int adv7180_try_mbus_fmt(struct v4l2_subdev *sd,
>>>>>> +				struct v4l2_mbus_framefmt *fmt)
>>>>>> +{
>>>>>> +	struct adv7180_state *state = to_state(sd);
>>>>>> +
>>>>>> +	fmt->code = V4L2_MBUS_FMT_YUYV8_2X8;
>>>>>> +	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
>>>>>> +	fmt->field = state->curr_norm & V4L2_STD_525_60 ?
>>>>>> +		     V4L2_FIELD_INTERLACED_BT : V4L2_FIELD_INTERLACED_TB;

>>>>> Just noticed this: use V4L2_FIELD_INTERLACED as that does the right thing.
>>>>> No need to split in _BT and _TB.

>>>>       Hm, testers have reported that _BT vs _TB do make a difference. I'll
>>>> try to look into how V4L2 handles interlacing for different standards.
>>> When using V4L2_FIELD_INTERLACED the BT vs TB is implicit (i.e. the application
>>> is supposed to know that the order will be different depending on the standard).
>>> Explicitly using BT/TB is only useful if you want to override the default, e.g.
>>> if a video was encoded using the wrong temporal order.

>>       We have used V4L2_FIELD_INTERLACED before and people reported
>> incorrect field ordering -- that's why we changed to what it is now. So this
>> might be an application failure?

> I've fairly certain it is, yes. The spec says this about FIELD_INTERLACED:

> "Images contain both fields, interleaved line by line. The temporal order of the
>   fields (whether the top or bottom field is first transmitted) depends on the
>   current video standard. M/NTSC transmits the bottom field first, all other
>   standards the top field first."

> So if the application needs to know the temporal order, it will have to look
> at the current video standard.

    It appeared that it's our R-Car VIN soc_camera driver that misses 
this video standard check. It just always treats V4L2_FIELD_INTERLACED 
the same as V4L2_FIELD_INTERLACED_TB when programming the video mode 
control register.

> Regards,

> 	Hans

WBR, Sergei