[PATCH 0/3] [media] tvp5150: Migrate to media-controller framework and add video format detection

[PATCH 0/3] [media] tvp5150: Migrate to media-controller framework and add video format detection

[Javier Martinez Canillas]

Hello,

The tvp5150 video decoder is usually used on a video pipeline with several
video processing integrated circuits. So the driver has to be migrated to
the new media device API to reflect this at the software level.

Also the tvp5150 is able to detect what is the video standard at which
the device is currently operating, so it makes sense to add video format
detection in the driver as well as.

This patch-set migrates the tvp5150 driver to the MCF and adds video format detection.

It is composed of the following patches:

[PATCH 1/3] [media] tvp5150: Add constants for PAL and NTSC video standards

[Javier Martinez Canillas]

Signed-off-by: Javier Martinez Canillas <martinez.javier@gmail.com>
---
 include/media/tvp5150.h |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/include/media/tvp5150.h b/include/media/tvp5150.h
index 72bd2a2..ccd4ed0 100644
--- a/include/media/tvp5150.h
+++ b/include/media/tvp5150.h
@@ -30,5 +30,11 @@
 #define TVP5150_NORMAL       0
 #define TVP5150_BLACK_SCREEN 1
 
+/* Number of pixels and number of lines per frame for different standards */
+#define NTSC_NUM_ACTIVE_PIXELS		(720)
+#define NTSC_NUM_ACTIVE_LINES		(480)
+#define PAL_NUM_ACTIVE_PIXELS		(720)
+#define PAL_NUM_ACTIVE_LINES		(576)
+
 #endif
 
-- 
1.7.4.1

[PATCH 2/3] [media] tvp5150: Add video format registers configuration values

[Javier Martinez Canillas]

The tvp5150 video decoder has two operation modes to configure the video
standard used.

If auto-switch mode is enable, the device can sense the signal and detect which
video standard the device is operating. Also the device can be forced to use a
user defined video standard.

Each operation mode uses a different register and the bitmask values to
represent each standard is different.

So we add video standard constants for both autoswitch and no-autoswitch mode.

Signed-off-by: Javier Martinez Canillas <martinez.javier@gmail.com>
---
 drivers/media/video/tvp5150_reg.h |   17 ++++++++++++++++-
 1 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/tvp5150_reg.h b/drivers/media/video/tvp5150_reg.h
index 4240043..25a9949 100644
--- a/drivers/media/video/tvp5150_reg.h
+++ b/drivers/media/video/tvp5150_reg.h
@@ -45,7 +45,22 @@
 
 /* Reserved 1Fh-27h */
 
-#define TVP5150_VIDEO_STD           0x28 /* Video standard */
+#define VIDEO_STD_MASK			 (0x07 >> 1)
+#define TVP5150_VIDEO_STD                0x28 /* Video standard */
+#define VIDEO_STD_AUTO_SWITCH_BIT	 0x00
+#define VIDEO_STD_NTSC_MJ_BIT		 0x02
+#define VIDEO_STD_PAL_BDGHIN_BIT	 0x04
+#define VIDEO_STD_PAL_M_BIT		 0x06
+#define VIDEO_STD_PAL_COMBINATION_N_BIT	 0x08
+#define VIDEO_STD_NTSC_4_43_BIT		 0x0a
+#define VIDEO_STD_SECAM_BIT		 0x0c
+
+#define VIDEO_STD_NTSC_MJ_BIT_AS                 0x01
+#define VIDEO_STD_PAL_BDGHIN_BIT_AS              0x03
+#define VIDEO_STD_PAL_M_BIT_AS		         0x05
+#define VIDEO_STD_PAL_COMBINATION_N_BIT_AS	 0x07
+#define VIDEO_STD_NTSC_4_43_BIT_AS		 0x09
+#define VIDEO_STD_SECAM_BIT_AS		         0x0b
 
 /* Reserved 29h-2bh */
 
-- 
1.7.4.1

[PATCH 3/3] [media] tvp5150: Migrate to media-controller framework and add video format detection

[Javier Martinez Canillas]

The tvp5150 video decoder is usually used on a video pipeline with several
video processing integrated circuits. So the driver has to be migrated to
the new media device API to reflect this at the software level.

Also the tvp5150 is able to detect what is the video standard at which
the device is currently operating, so it makes sense to add video format
detection in the driver.

Signed-off-by: Javier Martinez Canillas <martinez.javier@gmail.com>
---
 drivers/media/video/tvp5150.c |  400 +++++++++++++++++++++++++++++++++++++++--
 1 files changed, 389 insertions(+), 11 deletions(-)

diff --git a/drivers/media/video/tvp5150.c b/drivers/media/video/tvp5150.c
index e927d25..1c771f9 100644
--- a/drivers/media/video/tvp5150.c
+++ b/drivers/media/video/tvp5150.c
@@ -13,6 +13,7 @@
 #include <media/tvp5150.h>
 #include <media/v4l2-chip-ident.h>
 #include <media/v4l2-ctrls.h>
+#include <media/v4l2-subdev.h>
 
 #include "tvp5150_reg.h"
 
@@ -25,11 +26,79 @@ static int debug;
 module_param(debug, int, 0);
 MODULE_PARM_DESC(debug, "Debug level (0-2)");
 
+/* enum tvp515x_std - enum for supported standards */
+enum tvp515x_std {
+	STD_PAL_BDGHIN = 0,
+	STD_NTSC_MJ,
+	STD_INVALID
+};
+
+/**
+ * struct tvp515x_std_info - Structure to store standard informations
+ * @width: Line width in pixels
+ * @height:Number of active lines
+ * @video_std: Value to write in REG_VIDEO_STD register
+ * @standard: v4l2 standard structure information
+ */
+struct tvp515x_std_info {
+	u8 video_std;
+	struct v4l2_standard standard;
+	struct v4l2_mbus_framefmt format;
+};
+
+/**
+ * Supported standards -
+ *
+ * Currently supports two standards only, need to add support for rest of the
+ * modes, like SECAM, etc...
+ */
+static struct tvp515x_std_info tvp515x_std_list[] = {
+	/* Standard: STD_NTSC_MJ */
+	/* Standard: STD_PAL_BDGHIN */
+	[STD_PAL_BDGHIN] = {
+		.video_std = VIDEO_STD_PAL_BDGHIN_BIT,
+		.standard = {
+			.index = 1,
+			.id = V4L2_STD_PAL,
+			.name = "PAL",
+			.frameperiod = {1, 25},
+			.framelines = 625
+		},
+		.format = {
+			.width = PAL_NUM_ACTIVE_PIXELS,
+			.height = PAL_NUM_ACTIVE_LINES,
+			.code = V4L2_MBUS_FMT_UYVY8_2X8,
+			.field = V4L2_FIELD_INTERLACED,
+			.colorspace = V4L2_COLORSPACE_SMPTE170M,
+		},
+	},
+	[STD_NTSC_MJ] = {
+		.video_std = VIDEO_STD_NTSC_MJ_BIT,
+		.standard = {
+			.index = 0,
+			.id = V4L2_STD_NTSC,
+			.name = "NTSC",
+			.frameperiod = {1001, 30000},
+			.framelines = 525
+		},
+		.format = {
+			.width = NTSC_NUM_ACTIVE_PIXELS,
+			.height = NTSC_NUM_ACTIVE_LINES,
+			.code = V4L2_MBUS_FMT_UYVY8_2X8,
+			.field = V4L2_FIELD_INTERLACED,
+			.colorspace = V4L2_COLORSPACE_SMPTE170M,
+		},
+	},
+	/* Standard: need to add for additional standard */
+};
+
 struct tvp5150 {
 	struct v4l2_subdev sd;
 	struct v4l2_ctrl_handler hdl;
-
-	v4l2_std_id norm;	/* Current set standard */
+	struct media_pad pad;
+	struct v4l2_mbus_framefmt *format;
+	v4l2_std_id std_idx;
+	int norm;
 	u32 input;
 	u32 output;
 	int enable;
@@ -692,6 +761,45 @@ static int tvp5150_get_vbi(struct v4l2_subdev *sd,
 	return type;
 }
 
+/**
+ * tvp515x_query_current_std() : Query the current standard detected by TVP5151
+ * @sd: ptr to v4l2_subdev struct
+ *
+ * Returns the current standard detected by TVP5151, STD_INVALID if there is no
+ * standard detected.
+ */
+static int tvp515x_query_current_std(struct v4l2_subdev *sd)
+{
+	u8 std, std_status;
+
+	std = tvp5150_read(sd, TVP5150_VIDEO_STD);
+	if ((std & VIDEO_STD_MASK) == VIDEO_STD_AUTO_SWITCH_BIT)
+		/* use the standard status register */
+		std_status = tvp5150_read(sd, TVP5150_STATUS_REG_5);
+	else
+		/* use the standard register itself */
+		std_status = std;
+
+	switch (std_status & VIDEO_STD_MASK) {
+	case VIDEO_STD_NTSC_MJ_BIT:
+	case VIDEO_STD_NTSC_MJ_BIT_AS:
+		return STD_NTSC_MJ;
+
+	case VIDEO_STD_PAL_BDGHIN_BIT:
+	case VIDEO_STD_PAL_BDGHIN_BIT_AS:
+		return STD_PAL_BDGHIN;
+
+	default:
+		return STD_INVALID;
+	}
+
+	return STD_INVALID;
+}
+
+/****************************************************************************
+			V4L2 subdev video operations
+ ****************************************************************************/
+
 static int tvp5150_set_std(struct v4l2_subdev *sd, v4l2_std_id std)
 {
 	struct tvp5150 *decoder = to_tvp5150(sd);
@@ -704,19 +812,19 @@ static int tvp5150_set_std(struct v4l2_subdev *sd, v4l2_std_id std)
 	if (std == V4L2_STD_ALL) {
 		fmt = 0;	/* Autodetect mode */
 	} else if (std & V4L2_STD_NTSC_443) {
-		fmt = 0xa;
+		fmt = VIDEO_STD_NTSC_4_43_BIT;
 	} else if (std & V4L2_STD_PAL_M) {
-		fmt = 0x6;
+		fmt = VIDEO_STD_PAL_M_BIT;
 	} else if (std & (V4L2_STD_PAL_N | V4L2_STD_PAL_Nc)) {
-		fmt = 0x8;
+		fmt = VIDEO_STD_PAL_COMBINATION_N_BIT;
 	} else {
 		/* Then, test against generic ones */
 		if (std & V4L2_STD_NTSC)
-			fmt = 0x2;
+			fmt = VIDEO_STD_NTSC_MJ_BIT;
 		else if (std & V4L2_STD_PAL)
-			fmt = 0x4;
+			fmt = VIDEO_STD_PAL_BDGHIN_BIT;
 		else if (std & V4L2_STD_SECAM)
-			fmt = 0xc;
+			fmt = VIDEO_STD_SECAM_BIT;
 	}
 
 	v4l2_dbg(1, debug, sd, "Set video std register to %d.\n", fmt);
@@ -727,11 +835,26 @@ static int tvp5150_set_std(struct v4l2_subdev *sd, v4l2_std_id std)
 static int tvp5150_s_std(struct v4l2_subdev *sd, v4l2_std_id std)
 {
 	struct tvp5150 *decoder = to_tvp5150(sd);
+	int i;
+	int num_stds = ARRAY_SIZE(tvp515x_std_list);
 
 	if (decoder->norm == std)
 		return 0;
 
-	return tvp5150_set_std(sd, std);
+	for (i = 0; i < num_stds; i++)
+		if (std & tvp515x_std_list[i].standard.id)
+			break;
+
+	if ((i == num_stds) || (i == STD_INVALID))
+		return -EINVAL;
+
+	tvp5150_write(sd, TVP5150_VIDEO_STD, tvp515x_std_list[i].video_std);
+
+	decoder->norm = i;
+	decoder->norm = std;
+
+/*	return tvp5150_set_std(sd, std); */
+	return 0;
 }
 
 static int tvp5150_reset(struct v4l2_subdev *sd, u32 val)
@@ -778,6 +901,177 @@ static int tvp5150_s_ctrl(struct v4l2_ctrl *ctrl)
 	return -EINVAL;
 }
 
+static struct v4l2_mbus_framefmt *
+__tvp5150_get_pad_format(struct tvp5150 *tvp5150, struct v4l2_subdev_fh *fh,
+			 unsigned int pad, enum v4l2_subdev_format_whence which)
+{
+	switch (which) {
+	case V4L2_SUBDEV_FORMAT_TRY:
+		return v4l2_subdev_get_try_format(fh, pad);
+	case V4L2_SUBDEV_FORMAT_ACTIVE:
+		return tvp5150->format;
+	default:
+		return NULL;
+	}
+}
+
+static int tvp5150_get_pad_format(struct v4l2_subdev *subdev,
+			      struct v4l2_subdev_fh *fh,
+			      struct v4l2_subdev_format *format)
+{
+	struct tvp5150 *tvp5150 = to_tvp5150(subdev);
+
+	format->format = *__tvp5150_get_pad_format(tvp5150, fh, format->pad,
+						   format->which);
+
+	return 0;
+}
+
+static int tvp5150_set_pad_format(struct v4l2_subdev *subdev,
+			      struct v4l2_subdev_fh *fh,
+			      struct v4l2_subdev_format *format)
+{
+	struct tvp5150 *tvp5150 = to_tvp5150(subdev);
+	tvp5150->std_idx = STD_INVALID;
+
+	tvp5150->std_idx = tvp515x_query_current_std(subdev);
+	if (tvp5150->std_idx == STD_INVALID) {
+		v4l2_err(subdev, "Unable to query std\n");
+		return 0;
+	}
+
+	tvp5150->norm = tvp515x_std_list[tvp5150->std_idx].standard.id;
+
+	tvp5150->format = &tvp515x_std_list[tvp5150->std_idx].format;
+
+	format->format = *__tvp5150_get_pad_format(tvp5150, fh, format->pad,
+	format->which);
+
+	v4l2_info(subdev, "code=x%x width=%u height=%u colorspace=0x%x\n",
+			format->format.code, format->format.width,
+			format->format.height, format->format.colorspace);
+
+	return 0;
+}
+
+/**
+ * tvp515x_mbus_fmt_cap() - V4L2 decoder interface handler for try/s/g_mbus_fmt
+ * @sd: pointer to standard V4L2 sub-device structure
+ * @f: pointer to the mediabus format structure
+ *
+ * Negotiates the image capture size and mediabus format.
+ */
+static int
+tvp515x_mbus_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *f)
+{
+	struct tvp5150 *decoder = to_tvp5150(sd);
+
+	if (f == NULL)
+		return -EINVAL;
+
+	f = decoder->format;
+
+	v4l2_dbg(1, debug, sd, "MBUS_FMT: Width - %d, Height - %d\n",
+			f->width, f->height);
+	return 0;
+}
+
+/*
+ * tvp515x_s_stream() - V4L2 decoder i/f handler for s_stream
+ * @sd: pointer to standard V4L2 sub-device structure
+ * @enable: streaming enable or disable
+ *
+ * Sets streaming to enable or disable, if possible.
+ */
+static int tvp515x_s_stream(struct v4l2_subdev *subdev, int enable)
+{
+
+	/* Initializes TVP5150 to its default values */
+	/* # set PCLK (27MHz) */
+	tvp5150_write(subdev, TVP5150_CONF_SHARED_PIN, 0x00);
+
+	/* Output format: 8-bit ITU-R BT.656 with embedded syncs */
+	if (enable)
+		tvp5150_write(subdev, TVP5150_MISC_CTL, 0x09);
+	else
+		tvp5150_write(subdev, TVP5150_MISC_CTL, 0x00);
+
+	return 0;
+}
+
+
+/**
+ * tvp515x_enum_mbus_fmt() - V4L2 decoder interface handler for enum_mbus_fmt
+ * @sd: pointer to standard V4L2 sub-device structure
+ * @index: index of pixelcode to retrieve
+ * @code: receives the pixelcode
+ *
+ * Enumerates supported mediabus formats
+ */
+static int
+tvp515x_enum_mbus_fmt(struct v4l2_subdev *sd, unsigned index,
+					enum v4l2_mbus_pixelcode *code)
+{
+	if (index)
+		return -EINVAL;
+
+	*code = V4L2_MBUS_FMT_UYVY8_2X8;
+	return 0;
+}
+
+/**
+ * tvp515x_g_parm() - V4L2 decoder interface handler for g_parm
+ * @sd: pointer to standard V4L2 sub-device structure
+ * @a: pointer to standard V4L2 VIDIOC_G_PARM ioctl structure
+ *
+ * Returns the decoder's video CAPTURE parameters.
+ */
+static int
+tvp515x_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *a)
+{
+	struct v4l2_captureparm *cparm;
+
+	if (a == NULL)
+		return -EINVAL;
+
+	if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+		/* only capture is supported */
+		return -EINVAL;
+
+	cparm = &a->parm.capture;
+	cparm->capability = V4L2_CAP_TIMEPERFRAME;
+	cparm->timeperframe = a->parm.capture.timeperframe;
+
+	return 0;
+}
+
+/**
+ * tvp515x_s_parm() - V4L2 decoder interface handler for s_parm
+ * @sd: pointer to standard V4L2 sub-device structure
+ * @a: pointer to standard V4L2 VIDIOC_S_PARM ioctl structure
+ *
+ * Configures the decoder to use the input parameters, if possible. If
+ * not possible, returns the appropriate error code.
+ */
+static int
+tvp515x_s_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *a)
+{
+	struct v4l2_fract *timeperframe;
+
+	if (a == NULL)
+		return -EINVAL;
+
+	if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+		/* only capture is supported */
+		return -EINVAL;
+
+	timeperframe = &a->parm.capture.timeperframe;
+
+	return 0;
+}
+
+
+
 /****************************************************************************
 			I2C Command
  ****************************************************************************/
@@ -900,7 +1194,28 @@ static int tvp5150_g_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
 	return 0;
 }
 
-/* ----------------------------------------------------------------------- */
+/****************************************************************************
+		    V4L2 subdev core operations
+ ****************************************************************************/
+
+static int tvp5150_open(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
+{
+	struct tvp5150 *decoder = to_tvp5150(subdev);
+
+	decoder->std_idx = STD_INVALID;
+
+	decoder->std_idx = tvp515x_query_current_std(subdev);
+
+	if (decoder->std_idx == STD_INVALID) {
+		v4l2_err(subdev, "Unable to query std\n");
+		return 0;
+	}
+
+	decoder->format = (&(tvp515x_std_list[decoder->std_idx].format));
+	decoder->norm = tvp515x_std_list[decoder->std_idx].standard.id;
+
+	return 0;
+}
 
 static const struct v4l2_ctrl_ops tvp5150_ctrl_ops = {
 	.s_ctrl = tvp5150_s_ctrl,
@@ -924,12 +1239,24 @@ static const struct v4l2_subdev_core_ops tvp5150_core_ops = {
 #endif
 };
 
+static struct v4l2_subdev_file_ops tvp5150_subdev_file_ops = {
+	.open		= tvp5150_open,
+};
+
 static const struct v4l2_subdev_tuner_ops tvp5150_tuner_ops = {
 	.g_tuner = tvp5150_g_tuner,
 };
 
 static const struct v4l2_subdev_video_ops tvp5150_video_ops = {
 	.s_routing = tvp5150_s_routing,
+	.s_stream = tvp515x_s_stream,
+	.enum_mbus_fmt = tvp515x_enum_mbus_fmt,
+	.g_mbus_fmt = tvp515x_mbus_fmt,
+	.try_mbus_fmt = tvp515x_mbus_fmt,
+	.s_mbus_fmt = tvp515x_mbus_fmt,
+	.g_parm = tvp515x_g_parm,
+	.s_parm = tvp515x_s_parm,
+	.s_std_output = tvp5150_s_std,
 };
 
 static const struct v4l2_subdev_vbi_ops tvp5150_vbi_ops = {
@@ -939,14 +1266,57 @@ static const struct v4l2_subdev_vbi_ops tvp5150_vbi_ops = {
 	.s_raw_fmt = tvp5150_s_raw_fmt,
 };
 
+static int tvp515x_enum_mbus_code(struct v4l2_subdev *subdev,
+				  struct v4l2_subdev_fh *fh,
+				  struct v4l2_subdev_mbus_code_enum *code)
+{
+	if (code->index >= ARRAY_SIZE(tvp515x_std_list))
+		return -EINVAL;
+
+	code->code = V4L2_MBUS_FMT_UYVY8_2X8;
+
+	return 0;
+}
+
+static int tvp515x_enum_frame_size(struct v4l2_subdev *subdev,
+				   struct v4l2_subdev_fh *fh,
+				   struct v4l2_subdev_frame_size_enum *fse)
+{
+	int current_std = STD_INVALID;
+
+	if (fse->code != V4L2_MBUS_FMT_UYVY8_2X8)
+		return -EINVAL;
+
+	/* query the current standard */
+	current_std = tvp515x_query_current_std(subdev);
+	if (current_std == STD_INVALID) {
+		v4l2_err(subdev, "Unable to query std\n");
+		return 0;
+	}
+
+	fse->min_width = tvp515x_std_list[current_std].format.width;
+	fse->min_height = tvp515x_std_list[current_std].format.height;
+	fse->max_width = fse->min_width;
+	fse->max_height = fse->min_height;
+	return 0;
+}
+
+static struct v4l2_subdev_pad_ops tvp5150_pad_ops = {
+	.enum_mbus_code = tvp515x_enum_mbus_code,
+	.enum_frame_size = tvp515x_enum_frame_size,
+	.get_fmt = tvp5150_get_pad_format,
+	.set_fmt = tvp5150_set_pad_format,
+};
+
 static const struct v4l2_subdev_ops tvp5150_ops = {
 	.core = &tvp5150_core_ops,
+	.file	= &tvp5150_subdev_file_ops,
 	.tuner = &tvp5150_tuner_ops,
 	.video = &tvp5150_video_ops,
 	.vbi = &tvp5150_vbi_ops,
+	.pad = &tvp5150_pad_ops,
 };
 
-
 /****************************************************************************
 			I2C Client & Driver
  ****************************************************************************/
@@ -957,6 +1327,7 @@ static int tvp5150_probe(struct i2c_client *c,
 	struct tvp5150 *core;
 	struct v4l2_subdev *sd;
 	u8 msb_id, lsb_id, msb_rom, lsb_rom;
+	int ret;
 
 	/* Check if the adapter supports the needed features */
 	if (!i2c_check_functionality(c->adapter,
@@ -992,6 +1363,7 @@ static int tvp5150_probe(struct i2c_client *c,
 		}
 	}
 
+	core->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 	core->norm = V4L2_STD_ALL;	/* Default is autodetect */
 	core->input = TVP5150_COMPOSITE1;
 	core->enable = 1;
@@ -1017,6 +1389,12 @@ static int tvp5150_probe(struct i2c_client *c,
 
 	if (debug > 1)
 		tvp5150_log_status(sd);
+
+	core->pad.flags = MEDIA_PAD_FLAG_OUTPUT;
+	ret = media_entity_init(&core->sd.entity, 1, &core->pad, 0);
+	if (ret < 0)
+		kfree(core);
+
 	return 0;
 }
 
-- 
1.7.4.1

Re: [PATCH 3/3] [media] tvp5150: Migrate to media-controller framework and add video format detection

[Sakari Ailus]

Hi Javier,

Thanks for the patch! It's very interesting to see a driver for a video
decoder using the MC interface. Before this we've had just image sensors.

Javier Martinez Canillas wrote:
> The tvp5150 video decoder is usually used on a video pipeline with several
> video processing integrated circuits. So the driver has to be migrated to
> the new media device API to reflect this at the software level.
> 
> Also the tvp5150 is able to detect what is the video standard at which
> the device is currently operating, so it makes sense to add video format
> detection in the driver.
> 
> Signed-off-by: Javier Martinez Canillas <martinez.javier@gmail.com>
> ---
>  drivers/media/video/tvp5150.c |  400 +++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 389 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/video/tvp5150.c b/drivers/media/video/tvp5150.c
> index e927d25..1c771f9 100644
> --- a/drivers/media/video/tvp5150.c
> +++ b/drivers/media/video/tvp5150.c
> @@ -13,6 +13,7 @@
>  #include <media/tvp5150.h>
>  #include <media/v4l2-chip-ident.h>
>  #include <media/v4l2-ctrls.h>
> +#include <media/v4l2-subdev.h>
>  
>  #include "tvp5150_reg.h"
>  
> @@ -25,11 +26,79 @@ static int debug;
>  module_param(debug, int, 0);
>  MODULE_PARM_DESC(debug, "Debug level (0-2)");
>  
> +/* enum tvp515x_std - enum for supported standards */
> +enum tvp515x_std {
> +	STD_PAL_BDGHIN = 0,
> +	STD_NTSC_MJ,
> +	STD_INVALID
> +};
> +
> +/**
> + * struct tvp515x_std_info - Structure to store standard informations
> + * @width: Line width in pixels
> + * @height:Number of active lines
> + * @video_std: Value to write in REG_VIDEO_STD register
> + * @standard: v4l2 standard structure information
> + */
> +struct tvp515x_std_info {
> +	u8 video_std;
> +	struct v4l2_standard standard;
> +	struct v4l2_mbus_framefmt format;
> +};
> +
> +/**
> + * Supported standards -
> + *
> + * Currently supports two standards only, need to add support for rest of the
> + * modes, like SECAM, etc...
> + */
> +static struct tvp515x_std_info tvp515x_std_list[] = {
> +	/* Standard: STD_NTSC_MJ */
> +	/* Standard: STD_PAL_BDGHIN */
> +	[STD_PAL_BDGHIN] = {
> +		.video_std = VIDEO_STD_PAL_BDGHIN_BIT,
> +		.standard = {
> +			.index = 1,
> +			.id = V4L2_STD_PAL,
> +			.name = "PAL",
> +			.frameperiod = {1, 25},
> +			.framelines = 625
> +		},
> +		.format = {
> +			.width = PAL_NUM_ACTIVE_PIXELS,
> +			.height = PAL_NUM_ACTIVE_LINES,
> +			.code = V4L2_MBUS_FMT_UYVY8_2X8,
> +			.field = V4L2_FIELD_INTERLACED,
> +			.colorspace = V4L2_COLORSPACE_SMPTE170M,
> +		},
> +	},
> +	[STD_NTSC_MJ] = {
> +		.video_std = VIDEO_STD_NTSC_MJ_BIT,
> +		.standard = {
> +			.index = 0,
> +			.id = V4L2_STD_NTSC,
> +			.name = "NTSC",
> +			.frameperiod = {1001, 30000},
> +			.framelines = 525
> +		},
> +		.format = {
> +			.width = NTSC_NUM_ACTIVE_PIXELS,
> +			.height = NTSC_NUM_ACTIVE_LINES,
> +			.code = V4L2_MBUS_FMT_UYVY8_2X8,
> +			.field = V4L2_FIELD_INTERLACED,
> +			.colorspace = V4L2_COLORSPACE_SMPTE170M,
> +		},
> +	},
> +	/* Standard: need to add for additional standard */
> +};
> +
>  struct tvp5150 {
>  	struct v4l2_subdev sd;
>  	struct v4l2_ctrl_handler hdl;
> -
> -	v4l2_std_id norm;	/* Current set standard */
> +	struct media_pad pad;
> +	struct v4l2_mbus_framefmt *format;
> +	v4l2_std_id std_idx;
> +	int norm;
>  	u32 input;
>  	u32 output;
>  	int enable;
> @@ -692,6 +761,45 @@ static int tvp5150_get_vbi(struct v4l2_subdev *sd,
>  	return type;
>  }
>  
> +/**
> + * tvp515x_query_current_std() : Query the current standard detected by TVP5151
> + * @sd: ptr to v4l2_subdev struct
> + *
> + * Returns the current standard detected by TVP5151, STD_INVALID if there is no
> + * standard detected.
> + */
> +static int tvp515x_query_current_std(struct v4l2_subdev *sd)
> +{
> +	u8 std, std_status;
> +
> +	std = tvp5150_read(sd, TVP5150_VIDEO_STD);
> +	if ((std & VIDEO_STD_MASK) == VIDEO_STD_AUTO_SWITCH_BIT)
> +		/* use the standard status register */
> +		std_status = tvp5150_read(sd, TVP5150_STATUS_REG_5);
> +	else
> +		/* use the standard register itself */
> +		std_status = std;

Braces would be nice here.

> +	switch (std_status & VIDEO_STD_MASK) {
> +	case VIDEO_STD_NTSC_MJ_BIT:
> +	case VIDEO_STD_NTSC_MJ_BIT_AS:
> +		return STD_NTSC_MJ;
> +
> +	case VIDEO_STD_PAL_BDGHIN_BIT:
> +	case VIDEO_STD_PAL_BDGHIN_BIT_AS:
> +		return STD_PAL_BDGHIN;
> +
> +	default:
> +		return STD_INVALID;
> +	}
> +
> +	return STD_INVALID;

This return won't do anything.

> +}
> +
> +/****************************************************************************
> +			V4L2 subdev video operations
> + ****************************************************************************/
> +
>  static int tvp5150_set_std(struct v4l2_subdev *sd, v4l2_std_id std)
>  {
>  	struct tvp5150 *decoder = to_tvp5150(sd);
> @@ -704,19 +812,19 @@ static int tvp5150_set_std(struct v4l2_subdev *sd, v4l2_std_id std)
>  	if (std == V4L2_STD_ALL) {
>  		fmt = 0;	/* Autodetect mode */
>  	} else if (std & V4L2_STD_NTSC_443) {
> -		fmt = 0xa;
> +		fmt = VIDEO_STD_NTSC_4_43_BIT;
>  	} else if (std & V4L2_STD_PAL_M) {
> -		fmt = 0x6;
> +		fmt = VIDEO_STD_PAL_M_BIT;
>  	} else if (std & (V4L2_STD_PAL_N | V4L2_STD_PAL_Nc)) {
> -		fmt = 0x8;
> +		fmt = VIDEO_STD_PAL_COMBINATION_N_BIT;
>  	} else {
>  		/* Then, test against generic ones */
>  		if (std & V4L2_STD_NTSC)
> -			fmt = 0x2;
> +			fmt = VIDEO_STD_NTSC_MJ_BIT;
>  		else if (std & V4L2_STD_PAL)
> -			fmt = 0x4;
> +			fmt = VIDEO_STD_PAL_BDGHIN_BIT;
>  		else if (std & V4L2_STD_SECAM)
> -			fmt = 0xc;
> +			fmt = VIDEO_STD_SECAM_BIT;
>  	}

Excellent! Less magic numbers...

>  	v4l2_dbg(1, debug, sd, "Set video std register to %d.\n", fmt);
> @@ -727,11 +835,26 @@ static int tvp5150_set_std(struct v4l2_subdev *sd, v4l2_std_id std)
>  static int tvp5150_s_std(struct v4l2_subdev *sd, v4l2_std_id std)
>  {
>  	struct tvp5150 *decoder = to_tvp5150(sd);
> +	int i;
> +	int num_stds = ARRAY_SIZE(tvp515x_std_list);
>  
>  	if (decoder->norm == std)
>  		return 0;
>  
> -	return tvp5150_set_std(sd, std);
> +	for (i = 0; i < num_stds; i++)
> +		if (std & tvp515x_std_list[i].standard.id)
> +			break;
> +
> +	if ((i == num_stds) || (i == STD_INVALID))
> +		return -EINVAL;
> +
> +	tvp5150_write(sd, TVP5150_VIDEO_STD, tvp515x_std_list[i].video_std);
> +
> +	decoder->norm = i;
> +	decoder->norm = std;
> +
> +/*	return tvp5150_set_std(sd, std); */
> +	return 0;
>  }
>  
>  static int tvp5150_reset(struct v4l2_subdev *sd, u32 val)
> @@ -778,6 +901,177 @@ static int tvp5150_s_ctrl(struct v4l2_ctrl *ctrl)
>  	return -EINVAL;
>  }
>  
> +static struct v4l2_mbus_framefmt *
> +__tvp5150_get_pad_format(struct tvp5150 *tvp5150, struct v4l2_subdev_fh *fh,
> +			 unsigned int pad, enum v4l2_subdev_format_whence which)
> +{
> +	switch (which) {
> +	case V4L2_SUBDEV_FORMAT_TRY:
> +		return v4l2_subdev_get_try_format(fh, pad);
> +	case V4L2_SUBDEV_FORMAT_ACTIVE:
> +		return tvp5150->format;
> +	default:
> +		return NULL;

Hmm. This will never happen, but is returning NULL the right thing to
do? An easy alternative is to just replace this with if (which may only
have either of the two values).

> +	}
> +}
> +
> +static int tvp5150_get_pad_format(struct v4l2_subdev *subdev,
> +			      struct v4l2_subdev_fh *fh,
> +			      struct v4l2_subdev_format *format)
> +{
> +	struct tvp5150 *tvp5150 = to_tvp5150(subdev);
> +
> +	format->format = *__tvp5150_get_pad_format(tvp5150, fh, format->pad,
> +						   format->which);
> +
> +	return 0;
> +}
> +
> +static int tvp5150_set_pad_format(struct v4l2_subdev *subdev,
> +			      struct v4l2_subdev_fh *fh,
> +			      struct v4l2_subdev_format *format)
> +{
> +	struct tvp5150 *tvp5150 = to_tvp5150(subdev);
> +	tvp5150->std_idx = STD_INVALID;

The above assignment will always be overwritten immediately.

> +	tvp5150->std_idx = tvp515x_query_current_std(subdev);
> +	if (tvp5150->std_idx == STD_INVALID) {
> +		v4l2_err(subdev, "Unable to query std\n");
> +		return 0;

Isn't this an error?

> +	}
> +
> +	tvp5150->norm = tvp515x_std_list[tvp5150->std_idx].standard.id;
> +
> +	tvp5150->format = &tvp515x_std_list[tvp5150->std_idx].format;
> +
> +	format->format = *__tvp5150_get_pad_format(tvp5150, fh, format->pad,
> +	format->which);
> +
> +	v4l2_info(subdev, "code=x%x width=%u height=%u colorspace=0x%x\n",
> +			format->format.code, format->format.width,
> +			format->format.height, format->format.colorspace);
> +
> +	return 0;
> +}
> +
> +/**
> + * tvp515x_mbus_fmt_cap() - V4L2 decoder interface handler for try/s/g_mbus_fmt

The name of the function is different.

> + * @sd: pointer to standard V4L2 sub-device structure
> + * @f: pointer to the mediabus format structure
> + *
> + * Negotiates the image capture size and mediabus format.
> + */
> +static int
> +tvp515x_mbus_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *f)
> +{
> +	struct tvp5150 *decoder = to_tvp5150(sd);
> +
> +	if (f == NULL)
> +		return -EINVAL;
> +
> +	f = decoder->format;
> +
> +	v4l2_dbg(1, debug, sd, "MBUS_FMT: Width - %d, Height - %d\n",
> +			f->width, f->height);
> +	return 0;
> +}
> +
> +/*
> + * tvp515x_s_stream() - V4L2 decoder i/f handler for s_stream
> + * @sd: pointer to standard V4L2 sub-device structure
> + * @enable: streaming enable or disable
> + *
> + * Sets streaming to enable or disable, if possible.
> + */
> +static int tvp515x_s_stream(struct v4l2_subdev *subdev, int enable)
> +{
> +
> +	/* Initializes TVP5150 to its default values */
> +	/* # set PCLK (27MHz) */
> +	tvp5150_write(subdev, TVP5150_CONF_SHARED_PIN, 0x00);
> +
> +	/* Output format: 8-bit ITU-R BT.656 with embedded syncs */
> +	if (enable)
> +		tvp5150_write(subdev, TVP5150_MISC_CTL, 0x09);
> +	else
> +		tvp5150_write(subdev, TVP5150_MISC_CTL, 0x00);
> +
> +	return 0;
> +}
> +
> +
> +/**
> + * tvp515x_enum_mbus_fmt() - V4L2 decoder interface handler for enum_mbus_fmt
> + * @sd: pointer to standard V4L2 sub-device structure
> + * @index: index of pixelcode to retrieve
> + * @code: receives the pixelcode
> + *
> + * Enumerates supported mediabus formats
> + */
> +static int
> +tvp515x_enum_mbus_fmt(struct v4l2_subdev *sd, unsigned index,
> +					enum v4l2_mbus_pixelcode *code)
> +{
> +	if (index)
> +		return -EINVAL;
> +
> +	*code = V4L2_MBUS_FMT_UYVY8_2X8;
> +	return 0;
> +}
> +
> +/**
> + * tvp515x_g_parm() - V4L2 decoder interface handler for g_parm
> + * @sd: pointer to standard V4L2 sub-device structure
> + * @a: pointer to standard V4L2 VIDIOC_G_PARM ioctl structure
> + *
> + * Returns the decoder's video CAPTURE parameters.
> + */
> +static int
> +tvp515x_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *a)
> +{
> +	struct v4l2_captureparm *cparm;
> +
> +	if (a == NULL)
> +		return -EINVAL;
> +
> +	if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> +		/* only capture is supported */
> +		return -EINVAL;
> +
> +	cparm = &a->parm.capture;
> +	cparm->capability = V4L2_CAP_TIMEPERFRAME;
> +	cparm->timeperframe = a->parm.capture.timeperframe;
> +
> +	return 0;
> +}
> +
> +/**
> + * tvp515x_s_parm() - V4L2 decoder interface handler for s_parm
> + * @sd: pointer to standard V4L2 sub-device structure
> + * @a: pointer to standard V4L2 VIDIOC_S_PARM ioctl structure
> + *
> + * Configures the decoder to use the input parameters, if possible. If
> + * not possible, returns the appropriate error code.
> + */
> +static int
> +tvp515x_s_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *a)
> +{
> +	struct v4l2_fract *timeperframe;
> +
> +	if (a == NULL)
> +		return -EINVAL;
> +
> +	if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> +		/* only capture is supported */
> +		return -EINVAL;
> +
> +	timeperframe = &a->parm.capture.timeperframe;
> +
> +	return 0;
> +}
> +
> +
> +
>  /****************************************************************************
>  			I2C Command
>   ****************************************************************************/
> @@ -900,7 +1194,28 @@ static int tvp5150_g_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
>  	return 0;
>  }
>  
> -/* ----------------------------------------------------------------------- */
> +/****************************************************************************
> +		    V4L2 subdev core operations
> + ****************************************************************************/
> +
> +static int tvp5150_open(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
> +{
> +	struct tvp5150 *decoder = to_tvp5150(subdev);
> +
> +	decoder->std_idx = STD_INVALID;
> +
> +	decoder->std_idx = tvp515x_query_current_std(subdev);
> +
> +	if (decoder->std_idx == STD_INVALID) {
> +		v4l2_err(subdev, "Unable to query std\n");
> +		return 0;
> +	}
> +
> +	decoder->format = (&(tvp515x_std_list[decoder->std_idx].format));
> +	decoder->norm = tvp515x_std_list[decoder->std_idx].standard.id;
> +
> +	return 0;
> +}
>  
>  static const struct v4l2_ctrl_ops tvp5150_ctrl_ops = {
>  	.s_ctrl = tvp5150_s_ctrl,
> @@ -924,12 +1239,24 @@ static const struct v4l2_subdev_core_ops tvp5150_core_ops = {
>  #endif
>  };
>  
> +static struct v4l2_subdev_file_ops tvp5150_subdev_file_ops = {
> +	.open		= tvp5150_open,
> +};
> +
>  static const struct v4l2_subdev_tuner_ops tvp5150_tuner_ops = {
>  	.g_tuner = tvp5150_g_tuner,
>  };
>  
>  static const struct v4l2_subdev_video_ops tvp5150_video_ops = {
>  	.s_routing = tvp5150_s_routing,
> +	.s_stream = tvp515x_s_stream,
> +	.enum_mbus_fmt = tvp515x_enum_mbus_fmt,
> +	.g_mbus_fmt = tvp515x_mbus_fmt,
> +	.try_mbus_fmt = tvp515x_mbus_fmt,
> +	.s_mbus_fmt = tvp515x_mbus_fmt,
> +	.g_parm = tvp515x_g_parm,
> +	.s_parm = tvp515x_s_parm,
> +	.s_std_output = tvp5150_s_std,

Do we really need both video and pad format ops?

s_std should be added to pad ops so it would be available on the subdev
node.

>  };
>  
>  static const struct v4l2_subdev_vbi_ops tvp5150_vbi_ops = {
> @@ -939,14 +1266,57 @@ static const struct v4l2_subdev_vbi_ops tvp5150_vbi_ops = {
>  	.s_raw_fmt = tvp5150_s_raw_fmt,
>  };
>  
> +static int tvp515x_enum_mbus_code(struct v4l2_subdev *subdev,
> +				  struct v4l2_subdev_fh *fh,
> +				  struct v4l2_subdev_mbus_code_enum *code)
> +{
> +	if (code->index >= ARRAY_SIZE(tvp515x_std_list))
> +		return -EINVAL;
> +
> +	code->code = V4L2_MBUS_FMT_UYVY8_2X8;

If there's just one supported mbus code, non-zero code->index must
return -EINVAL.

> +	return 0;
> +}
> +
> +static int tvp515x_enum_frame_size(struct v4l2_subdev *subdev,
> +				   struct v4l2_subdev_fh *fh,
> +				   struct v4l2_subdev_frame_size_enum *fse)
> +{
> +	int current_std = STD_INVALID;

current_std is overwritten before it gets used.

> +	if (fse->code != V4L2_MBUS_FMT_UYVY8_2X8)
> +		return -EINVAL;
> +
> +	/* query the current standard */
> +	current_std = tvp515x_query_current_std(subdev);
> +	if (current_std == STD_INVALID) {
> +		v4l2_err(subdev, "Unable to query std\n");
> +		return 0;
> +	}

I wonder how the enum_frame_size and s_std are supposed to interact,
especially that I understand, after reading the discussion, the chip may
be used to force certain standard while the actual signal is different.

> +	fse->min_width = tvp515x_std_list[current_std].format.width;
> +	fse->min_height = tvp515x_std_list[current_std].format.height;
> +	fse->max_width = fse->min_width;
> +	fse->max_height = fse->min_height;
> +	return 0;
> +}
> +
> +static struct v4l2_subdev_pad_ops tvp5150_pad_ops = {
> +	.enum_mbus_code = tvp515x_enum_mbus_code,
> +	.enum_frame_size = tvp515x_enum_frame_size,
> +	.get_fmt = tvp5150_get_pad_format,
> +	.set_fmt = tvp5150_set_pad_format,
> +};
> +
>  static const struct v4l2_subdev_ops tvp5150_ops = {
>  	.core = &tvp5150_core_ops,
> +	.file	= &tvp5150_subdev_file_ops,
>  	.tuner = &tvp5150_tuner_ops,
>  	.video = &tvp5150_video_ops,
>  	.vbi = &tvp5150_vbi_ops,
> +	.pad = &tvp5150_pad_ops,
>  };
>  
> -
>  /****************************************************************************
>  			I2C Client & Driver
>   ****************************************************************************/
> @@ -957,6 +1327,7 @@ static int tvp5150_probe(struct i2c_client *c,
>  	struct tvp5150 *core;
>  	struct v4l2_subdev *sd;
>  	u8 msb_id, lsb_id, msb_rom, lsb_rom;
> +	int ret;
>  
>  	/* Check if the adapter supports the needed features */
>  	if (!i2c_check_functionality(c->adapter,
> @@ -992,6 +1363,7 @@ static int tvp5150_probe(struct i2c_client *c,
>  		}
>  	}
>  
> +	core->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>  	core->norm = V4L2_STD_ALL;	/* Default is autodetect */
>  	core->input = TVP5150_COMPOSITE1;
>  	core->enable = 1;
> @@ -1017,6 +1389,12 @@ static int tvp5150_probe(struct i2c_client *c,
>  
>  	if (debug > 1)
>  		tvp5150_log_status(sd);
> +
> +	core->pad.flags = MEDIA_PAD_FLAG_OUTPUT;
> +	ret = media_entity_init(&core->sd.entity, 1, &core->pad, 0);
> +	if (ret < 0)
> +		kfree(core);
> +
>  	return 0;
>  }
>  

-- 
Sakari Ailus
sakari.ailus@iki.fi