[PATCH 00/10] [media] tvp5150: add MC and DT support

[PATCH 00/10] [media] tvp5150: add MC and DT support

[Javier Martinez Canillas]

Hello,

One of my testing platforms for the MC next gen [0] work has been an OMAP3
board (IGEPv2) with a tvp5151 video decoder attached to the OMAP3ISP block.

I've been using some patches from Laurent Pinchart that adds MC support to
the tvp5150 driver. The patches were never posted to the list and it seems
he doesn't have time to continue working on this so I have taken them from
his personal tree [1] and submitting now for review.

The series also contains patches that adds DT support to the driver so it
can be used in DT based platforms.

To test, the following media pipeline was used:

$ media-ctl -r -l '"tvp5150 1-005c":0->"OMAP3 ISP CCDC":0[1], "OMAP3 ISP CCDC":1->"OMAP3 ISP CCDC output":0[1]'
$ media-ctl -v --set-format '"OMAP3 ISP CCDC":0 [UYVY2X8 720x240 field:alternate]'
$ media-ctl -v --set-format '"OMAP3 ISP CCDC":1 [UYVY2X8 720x240 field:interlaced-tb]'

And frames captured with the yavta tool:

$ yavta -f UYVY -s 720x480 -n 1 --field interlaced-tb --capture=1 -F /dev/video2
$ raw2rgbpnm -f UYVY -s 720x480 frame-000000.bin frame-000000.pnm

The patches are on top of [0] not because is a depedency but just to avoid
merge conflicts and I don't expect them to be picked before that anyways.

Best regards,
Javier

[0]: http://lists.infradead.org/pipermail/linux-arm-kernel/2015-August/367109.html
[1]: http://git.linuxtv.org/pinchartl/media.git/log/?h=omap3isp/tvp5151


Eduard Gavin (1):
  [media] tvp5150: Add OF match table

Javier Martinez Canillas (3):
  [media] tvp5150: Add device tree binding document
  [media] tvp5150: Initialize the chip on probe
  [media] tvp5150: Configure data interface via pdata or DT

Laurent Pinchart (6):
  [media] tvp5150: Restructure version detection
  [media] tvp5150: Add tvp5151 support
  [media] tvp5150: Add pad-level subdev operations
  [media] tvp5150: Add pixel rate control support
  [media] tvp5150: Add s_stream subdev operation support
  [media] tvp5150: Add g_mbus_config subdev operation support

 .../devicetree/bindings/media/i2c/tvp5150.txt      |  35 +++
 drivers/media/i2c/tvp5150.c                        | 272 +++++++++++++++++----
 include/media/i2c/tvp5150.h                        |   5 +
 3 files changed, 263 insertions(+), 49 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/tvp5150.txt

-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 01/10] [media] tvp5150: Restructure version detection

[Javier Martinez Canillas]

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Move the version detection code to a separate function and restructure
it to prepare for TVP5151 support.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---

 drivers/media/i2c/tvp5150.c | 79 ++++++++++++++++++++++++++-------------------
 1 file changed, 45 insertions(+), 34 deletions(-)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index dda8b3c000cc..9e953e5a7ec9 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -1105,13 +1105,53 @@ static const struct v4l2_subdev_ops tvp5150_ops = {
 			I2C Client & Driver
  ****************************************************************************/
 
+static int tvp5150_detect_version(struct tvp5150 *core)
+{
+	struct v4l2_subdev *sd = &core->sd;
+	struct i2c_client *c = v4l2_get_subdevdata(sd);
+	unsigned int i;
+	u16 dev_id;
+	u16 rom_ver;
+	u8 regs[4];
+	int res;
+
+	/*
+	 * Read consequent registers - TVP5150_MSB_DEV_ID, TVP5150_LSB_DEV_ID,
+	 * TVP5150_ROM_MAJOR_VER, TVP5150_ROM_MINOR_VER
+	 */
+	for (i = 0; i < 4; i++) {
+		res = tvp5150_read(sd, TVP5150_MSB_DEV_ID + i);
+		if (res < 0)
+			return res;
+		regs[i] = res;
+	}
+
+	dev_id = (regs[0] << 8) | regs[1];
+	rom_ver = (regs[2] << 8) | regs[3];
+
+	v4l2_info(sd, "tvp%04x (%u.%u) chip found @ 0x%02x (%s)\n",
+		  dev_id, regs[2], regs[3], c->addr << 1, c->adapter->name);
+
+	if (dev_id == 0x5150 && rom_ver == 0x0321) { /* TVP51510A */
+		v4l2_info(sd, "tvp5150a detected.\n");
+	} else if (dev_id == 0x5150 && rom_ver == 0x0400) { /* TVP5150AM1 */
+		v4l2_info(sd, "tvp5150am1 detected.\n");
+
+		/* ITU-T BT.656.4 timing */
+		tvp5150_write(sd, TVP5150_REV_SELECT, 0);
+	} else {
+		v4l2_info(sd, "*** unknown tvp%04x chip detected.\n", dev_id);
+	}
+
+	return 0;
+}
+
 static int tvp5150_probe(struct i2c_client *c,
 			 const struct i2c_device_id *id)
 {
 	struct tvp5150 *core;
 	struct v4l2_subdev *sd;
-	int tvp5150_id[4];
-	int i, res;
+	int res;
 
 	/* Check if the adapter supports the needed features */
 	if (!i2c_check_functionality(c->adapter,
@@ -1124,38 +1164,9 @@ static int tvp5150_probe(struct i2c_client *c,
 	sd = &core->sd;
 	v4l2_i2c_subdev_init(sd, c, &tvp5150_ops);
 
-	/* 
-	 * Read consequent registers - TVP5150_MSB_DEV_ID, TVP5150_LSB_DEV_ID,
-	 * TVP5150_ROM_MAJOR_VER, TVP5150_ROM_MINOR_VER 
-	 */
-	for (i = 0; i < 4; i++) {
-		res = tvp5150_read(sd, TVP5150_MSB_DEV_ID + i);
-		if (res < 0)
-			return res;
-		tvp5150_id[i] = res;
-	}
-
-	v4l_info(c, "chip found @ 0x%02x (%s)\n",
-		 c->addr << 1, c->adapter->name);
-
-	if (tvp5150_id[2] == 4 && tvp5150_id[3] == 0) { /* Is TVP5150AM1 */
-		v4l2_info(sd, "tvp%02x%02xam1 detected.\n",
-			  tvp5150_id[0], tvp5150_id[1]);
-
-		/* ITU-T BT.656.4 timing */
-		tvp5150_write(sd, TVP5150_REV_SELECT, 0);
-	} else {
-		/* Is TVP5150A */
-		if (tvp5150_id[2] == 3 || tvp5150_id[3] == 0x21) {
-			v4l2_info(sd, "tvp%02x%02xa detected.\n",
-				  tvp5150_id[0], tvp5150_id[1]);
-		} else {
-			v4l2_info(sd, "*** unknown tvp%02x%02x chip detected.\n",
-				  tvp5150_id[0], tvp5150_id[1]);
-			v4l2_info(sd, "*** Rom ver is %d.%d\n",
-				  tvp5150_id[2], tvp5150_id[3]);
-		}
-	}
+	res = tvp5150_detect_version(core);
+	if (res < 0)
+		return res;
 
 	core->norm = V4L2_STD_ALL;	/* Default is autodetect */
 	core->input = TVP5150_COMPOSITE1;
-- 
2.4.3

[PATCH 02/10] [media] tvp5150: Add tvp5151 support

[Javier Martinez Canillas]

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Expand the version detection code to identity the tvp5151.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---

 drivers/media/i2c/tvp5150.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index 9e953e5a7ec9..b3b34e24db13 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -1139,6 +1139,8 @@ static int tvp5150_detect_version(struct tvp5150 *core)
 
 		/* ITU-T BT.656.4 timing */
 		tvp5150_write(sd, TVP5150_REV_SELECT, 0);
+	} else if (dev_id == 0x5151 && rom_ver == 0x0100) { /* TVP5151 */
+		v4l2_info(sd, "tvp5151 detected.\n");
 	} else {
 		v4l2_info(sd, "*** unknown tvp%04x chip detected.\n", dev_id);
 	}
-- 
2.4.3

[PATCH 03/10] [media] tvp5150: Add pad-level subdev operations

[Javier Martinez Canillas]

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

This patch enables the tvp5150 decoder driver to be used with the media
controller framework by adding pad-level subdev operations and init the
media entity pad.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---

 drivers/media/i2c/tvp5150.c | 60 +++++++++++++++++++++++++++++++++------------
 1 file changed, 45 insertions(+), 15 deletions(-)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index b3b34e24db13..82fba9d46f30 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -35,6 +35,7 @@ MODULE_PARM_DESC(debug, "Debug level (0-2)");
 
 struct tvp5150 {
 	struct v4l2_subdev sd;
+	struct media_pad pad;
 	struct v4l2_ctrl_handler hdl;
 	struct v4l2_rect rect;
 
@@ -818,17 +819,6 @@ static v4l2_std_id tvp5150_read_std(struct v4l2_subdev *sd)
 	}
 }
 
-static int tvp5150_enum_mbus_code(struct v4l2_subdev *sd,
-		struct v4l2_subdev_pad_config *cfg,
-		struct v4l2_subdev_mbus_code_enum *code)
-{
-	if (code->pad || code->index)
-		return -EINVAL;
-
-	code->code = MEDIA_BUS_FMT_UYVY8_2X8;
-	return 0;
-}
-
 static int tvp5150_fill_fmt(struct v4l2_subdev *sd,
 		struct v4l2_subdev_pad_config *cfg,
 		struct v4l2_subdev_format *format)
@@ -841,13 +831,11 @@ static int tvp5150_fill_fmt(struct v4l2_subdev *sd,
 
 	f = &format->format;
 
-	tvp5150_reset(sd, 0);
-
 	f->width = decoder->rect.width;
-	f->height = decoder->rect.height;
+	f->height = decoder->rect.height / 2;
 
 	f->code = MEDIA_BUS_FMT_UYVY8_2X8;
-	f->field = V4L2_FIELD_SEQ_TB;
+	f->field = V4L2_FIELD_ALTERNATE;
 	f->colorspace = V4L2_COLORSPACE_SMPTE170M;
 
 	v4l2_dbg(1, debug, sd, "width = %d, height = %d\n", f->width,
@@ -948,6 +936,39 @@ static int tvp5150_cropcap(struct v4l2_subdev *sd, struct v4l2_cropcap *a)
 	return 0;
 }
 
+ /****************************************************************************
+			V4L2 subdev pad ops
+ ****************************************************************************/
+
+static int tvp5150_enum_mbus_code(struct v4l2_subdev *sd,
+				  struct v4l2_subdev_pad_config *cfg,
+				  struct v4l2_subdev_mbus_code_enum *code)
+{
+	if (code->index)
+		return -EINVAL;
+
+	code->code = MEDIA_BUS_FMT_UYVY8_2X8;
+	return 0;
+}
+
+static int tvp5150_enum_frame_size(struct v4l2_subdev *sd,
+				   struct v4l2_subdev_pad_config *cfg,
+				   struct v4l2_subdev_frame_size_enum *fse)
+{
+	struct tvp5150 *decoder = to_tvp5150(sd);
+
+	if (fse->index >= 8 || fse->code != MEDIA_BUS_FMT_UYVY8_2X8)
+		return -EINVAL;
+
+	fse->code = MEDIA_BUS_FMT_UYVY8_2X8;
+	fse->min_width = decoder->rect.width;
+	fse->max_width = decoder->rect.width;
+	fse->min_height = decoder->rect.height / 2;
+	fse->max_height = decoder->rect.height / 2;
+
+	return 0;
+}
+
 /****************************************************************************
 			I2C Command
  ****************************************************************************/
@@ -1088,6 +1109,7 @@ static const struct v4l2_subdev_vbi_ops tvp5150_vbi_ops = {
 
 static const struct v4l2_subdev_pad_ops tvp5150_pad_ops = {
 	.enum_mbus_code = tvp5150_enum_mbus_code,
+	.enum_frame_size = tvp5150_enum_frame_size,
 	.set_fmt = tvp5150_fill_fmt,
 	.get_fmt = tvp5150_fill_fmt,
 };
@@ -1165,6 +1187,14 @@ static int tvp5150_probe(struct i2c_client *c,
 		return -ENOMEM;
 	sd = &core->sd;
 	v4l2_i2c_subdev_init(sd, c, &tvp5150_ops);
+	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	core->pad.flags = MEDIA_PAD_FL_SOURCE;
+	res = media_entity_pads_init(&sd->entity, 1, &core->pad);
+	if (res < 0)
+		return res;
+#endif
 
 	res = tvp5150_detect_version(core);
 	if (res < 0)
-- 
2.4.3

[PATCH 04/10] [media] tvp5150: Add pixel rate control support

[Javier Martinez Canillas]

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

This patch adds support for the V4L2_CID_PIXEL_RATE control.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---

 drivers/media/i2c/tvp5150.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index 82fba9d46f30..71473cec236a 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -1204,7 +1204,7 @@ static int tvp5150_probe(struct i2c_client *c,
 	core->input = TVP5150_COMPOSITE1;
 	core->enable = 1;
 
-	v4l2_ctrl_handler_init(&core->hdl, 4);
+	v4l2_ctrl_handler_init(&core->hdl, 5);
 	v4l2_ctrl_new_std(&core->hdl, &tvp5150_ctrl_ops,
 			V4L2_CID_BRIGHTNESS, 0, 255, 1, 128);
 	v4l2_ctrl_new_std(&core->hdl, &tvp5150_ctrl_ops,
@@ -1213,6 +1213,9 @@ static int tvp5150_probe(struct i2c_client *c,
 			V4L2_CID_SATURATION, 0, 255, 1, 128);
 	v4l2_ctrl_new_std(&core->hdl, &tvp5150_ctrl_ops,
 			V4L2_CID_HUE, -128, 127, 1, 0);
+	v4l2_ctrl_new_std(&core->hdl, &tvp5150_ctrl_ops,
+			V4L2_CID_PIXEL_RATE, 27000000,
+			27000000, 1, 27000000);
 	sd->ctrl_handler = &core->hdl;
 	if (core->hdl.error) {
 		res = core->hdl.error;
-- 
2.4.3

[PATCH 05/10] [media] tvp5150: Add s_stream subdev operation support

[Javier Martinez Canillas]

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

This patch adds the .s_stream subdev operation to the driver.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---

 drivers/media/i2c/tvp5150.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index 71473cec236a..fb7a4ddff1fe 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -973,6 +973,21 @@ static int tvp5150_enum_frame_size(struct v4l2_subdev *sd,
 			I2C Command
  ****************************************************************************/
 
+static int tvp5150_s_stream(struct v4l2_subdev *sd, int enable)
+{
+	/* Initializes TVP5150 to its default values */
+	/* # set PCLK (27MHz) */
+	tvp5150_write(sd, TVP5150_CONF_SHARED_PIN, 0x00);
+
+	/* Output format: 8-bit ITU-R BT.656 with embedded syncs */
+	if (enable)
+		tvp5150_write(sd, TVP5150_MISC_CTL, 0x09);
+	else
+		tvp5150_write(sd, TVP5150_MISC_CTL, 0x00);
+
+	return 0;
+}
+
 static int tvp5150_s_routing(struct v4l2_subdev *sd,
 			     u32 input, u32 output, u32 config)
 {
@@ -1094,6 +1109,7 @@ static const struct v4l2_subdev_tuner_ops tvp5150_tuner_ops = {
 
 static const struct v4l2_subdev_video_ops tvp5150_video_ops = {
 	.s_std = tvp5150_s_std,
+	.s_stream = tvp5150_s_stream,
 	.s_routing = tvp5150_s_routing,
 	.s_crop = tvp5150_s_crop,
 	.g_crop = tvp5150_g_crop,
-- 
2.4.3

[PATCH 06/10] [media] tvp5150: Add g_mbus_config subdev operation support

[Javier Martinez Canillas]

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

This patch adds the .g_mbus_config subdev operation to the driver.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---

 drivers/media/i2c/tvp5150.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index fb7a4ddff1fe..105bd1c6b17f 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -936,6 +936,16 @@ static int tvp5150_cropcap(struct v4l2_subdev *sd, struct v4l2_cropcap *a)
 	return 0;
 }
 
+static int tvp5150_g_mbus_config(struct v4l2_subdev *sd,
+				 struct v4l2_mbus_config *cfg)
+{
+	cfg->type = V4L2_MBUS_BT656;
+	cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING
+		   | V4L2_MBUS_FIELD_EVEN_LOW | V4L2_MBUS_DATA_ACTIVE_HIGH;
+
+	return 0;
+}
+
  /****************************************************************************
 			V4L2 subdev pad ops
  ****************************************************************************/
@@ -1114,6 +1124,7 @@ static const struct v4l2_subdev_video_ops tvp5150_video_ops = {
 	.s_crop = tvp5150_s_crop,
 	.g_crop = tvp5150_g_crop,
 	.cropcap = tvp5150_cropcap,
+	.g_mbus_config = tvp5150_g_mbus_config,
 };
 
 static const struct v4l2_subdev_vbi_ops tvp5150_vbi_ops = {
-- 
2.4.3

[PATCH 07/10] [media] tvp5150: Add device tree binding document

[Javier Martinez Canillas]

Add a Device Tree binding document for the TVP5150 video decoder.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---

 .../devicetree/bindings/media/i2c/tvp5150.txt      | 35 ++++++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/tvp5150.txt

diff --git a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
new file mode 100644
index 000000000000..bf0b3f3128ce
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
@@ -0,0 +1,35 @@
+* Texas Instruments TVP5150 and TVP5151 video decoders
+
+The TVP5150 and TVP5151 are video decoders that convert baseband NTSC and PAL
+(and also SECAM in the TVP5151 case) video signals to either 8-bit 4:2:2 YUV
+with discrete syncs or 8-bit ITU-R BT.656 with embedded syncs output formats.
+
+Required Properties:
+- compatible: value must be "ti,tvp5150"
+- reg: I2C slave address
+
+Optional Properties:
+- powerdown-gpios: phandle for the GPIO connected to the PDN pin, if any.
+- reset-gpios: phandle for the GPIO connected to the RESETB pin, if any.
+
+The device node must contain one 'port' child node for its digital output
+video port, in accordance with the video interface bindings defined in
+Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+Example:
+
+&i2c2 {
+	...
+	tvp5150@5c {
+			compatible = "ti,tvp5150";
+			reg = <0x5c>;
+			powerdown-gpios = <&gpio4 30 GPIO_ACTIVE_LOW>;
+			reset-gpios = <&gpio6 7 GPIO_ACTIVE_LOW>;
+
+			port {
+				tvp5150_1: endpoint {
+					remote-endpoint = <&ccdc_ep>;
+				};
+			};
+	};
+};
-- 
2.4.3

Re: [PATCH 07/10] [media] tvp5150: Add device tree binding document

[Rob Herring]

On Mon, Jan 04, 2016 at 09:25:29AM -0300, Javier Martinez Canillas wrote:
> Add a Device Tree binding document for the TVP5150 video decoder.
> 
> Signed-off-by: Javier Martinez Canillas <javier-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>
> ---
> 
>  .../devicetree/bindings/media/i2c/tvp5150.txt      | 35 ++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/tvp5150.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
> new file mode 100644
> index 000000000000..bf0b3f3128ce
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
> @@ -0,0 +1,35 @@
> +* Texas Instruments TVP5150 and TVP5151 video decoders
> +
> +The TVP5150 and TVP5151 are video decoders that convert baseband NTSC and PAL
> +(and also SECAM in the TVP5151 case) video signals to either 8-bit 4:2:2 YUV
> +with discrete syncs or 8-bit ITU-R BT.656 with embedded syncs output formats.
> +
> +Required Properties:
> +- compatible: value must be "ti,tvp5150"

What about the 5151? The driver never needs to know if SECAM is 
supported or not?

> +- reg: I2C slave address
> +
> +Optional Properties:
> +- powerdown-gpios: phandle for the GPIO connected to the PDN pin, if any.
> +- reset-gpios: phandle for the GPIO connected to the RESETB pin, if any.
> +
> +The device node must contain one 'port' child node for its digital output
> +video port, in accordance with the video interface bindings defined in
> +Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +Example:
> +
> +&i2c2 {
> +	...
> +	tvp5150@5c {
> +			compatible = "ti,tvp5150";

Too much indentation here.

> +			reg = <0x5c>;
> +			powerdown-gpios = <&gpio4 30 GPIO_ACTIVE_LOW>;
> +			reset-gpios = <&gpio6 7 GPIO_ACTIVE_LOW>;
> +
> +			port {
> +				tvp5150_1: endpoint {
> +					remote-endpoint = <&ccdc_ep>;
> +				};
> +			};
> +	};
> +};
> -- 
> 2.4.3
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/10] [media] tvp5150: Add device tree binding document

[Javier Martinez Canillas]

Hello Rob,

Thanks a lot for your feedback.

On 01/04/2016 11:07 AM, Rob Herring wrote:
> On Mon, Jan 04, 2016 at 09:25:29AM -0300, Javier Martinez Canillas wrote:
>> Add a Device Tree binding document for the TVP5150 video decoder.
>>
>> Signed-off-by: Javier Martinez Canillas <javier-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>
>> ---
>>
>>  .../devicetree/bindings/media/i2c/tvp5150.txt      | 35 ++++++++++++++++++++++
>>  1 file changed, 35 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/tvp5150.txt
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
>> new file mode 100644
>> index 000000000000..bf0b3f3128ce
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
>> @@ -0,0 +1,35 @@
>> +* Texas Instruments TVP5150 and TVP5151 video decoders
>> +
>> +The TVP5150 and TVP5151 are video decoders that convert baseband NTSC and PAL
>> +(and also SECAM in the TVP5151 case) video signals to either 8-bit 4:2:2 YUV
>> +with discrete syncs or 8-bit ITU-R BT.656 with embedded syncs output formats.
>> +
>> +Required Properties:
>> +- compatible: value must be "ti,tvp5150"
> 
> What about the 5151? The driver never needs to know if SECAM is 
> supported or not?
>

The device ID can be detected at runtime by reading the TVP5150_MSB_DEV_ID
and TVP5150_LSB_DEV_ID registers in both tvp5150 and tvp5151 (see patch #2
in this series). So I thought there was no need to have another compatible
string for "ti,tvp5151".
 
>> +- reg: I2C slave address
>> +
>> +Optional Properties:
>> +- powerdown-gpios: phandle for the GPIO connected to the PDN pin, if any.
>> +- reset-gpios: phandle for the GPIO connected to the RESETB pin, if any.
>> +
>> +The device node must contain one 'port' child node for its digital output
>> +video port, in accordance with the video interface bindings defined in
>> +Documentation/devicetree/bindings/media/video-interfaces.txt.
>> +
>> +Example:
>> +
>> +&i2c2 {
>> +	...
>> +	tvp5150@5c {
>> +			compatible = "ti,tvp5150";
> 
> Too much indentation here.
>

Right, I copied the example from a DTS that had another level of indentation
but it seems that I forgot to fix all lines, sorry about that. I will in v2.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/10] [media] tvp5150: Add device tree binding document

[Laurent Pinchart]

Hi Javier,

Thank you for the patch.

On Monday 04 January 2016 09:25:29 Javier Martinez Canillas wrote:
> Add a Device Tree binding document for the TVP5150 video decoder.
> 
> Signed-off-by: Javier Martinez Canillas <javier-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>
> ---
> 
>  .../devicetree/bindings/media/i2c/tvp5150.txt      | 35 +++++++++++++++++++
>  1 file changed, 35 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/tvp5150.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
> b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt new file mode
> 100644
> index 000000000000..bf0b3f3128ce
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
> @@ -0,0 +1,35 @@
> +* Texas Instruments TVP5150 and TVP5151 video decoders
> +
> +The TVP5150 and TVP5151 are video decoders that convert baseband NTSC and
> PAL
> +(and also SECAM in the TVP5151 case) video signals to either 8-bit 4:2:2
> YUV
> +with discrete syncs or 8-bit ITU-R BT.656 with embedded syncs output
> formats.
> +
> +Required Properties:
> +- compatible: value must be "ti,tvp5150"
> +- reg: I2C slave address
> +
> +Optional Properties:
> +- powerdown-gpios: phandle for the GPIO connected to the PDN pin, if any.

The signal is called PDN in the datasheet, so it might make sense to call this 
pdn-gpios. I have no strong opinion on this, I'll let you decide what you 
think is best.

Apart from that (and the indentation issue pointed out by Rob),

Reviewed-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>

> +- reset-gpios: phandle for the GPIO connected to the RESETB pin, if any.
> +
> +The device node must contain one 'port' child node for its digital output
> +video port, in accordance with the video interface bindings defined in
> +Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +Example:
> +
> +&i2c2 {
> +	...
> +	tvp5150@5c {
> +			compatible = "ti,tvp5150";
> +			reg = <0x5c>;
> +			powerdown-gpios = <&gpio4 30 GPIO_ACTIVE_LOW>;
> +			reset-gpios = <&gpio6 7 GPIO_ACTIVE_LOW>;
> +
> +			port {
> +				tvp5150_1: endpoint {
> +					remote-endpoint = <&ccdc_ep>;
> +				};
> +			};
> +	};
> +};

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/10] [media] tvp5150: Add device tree binding document

[Javier Martinez Canillas]

Hello Laurent,

On 01/06/2016 07:39 AM, Laurent Pinchart wrote:
> Hi Javier,
> 
> Thank you for the patch.
>

Thanks a lot for your feedback.

[snip]

>> +
>> +Optional Properties:
>> +- powerdown-gpios: phandle for the GPIO connected to the PDN pin, if any.
> 
> The signal is called PDN in the datasheet, so it might make sense to call this 
> pdn-gpios. I have no strong opinion on this, I'll let you decide what you 
> think is best.
>

Yes, I wondered if the convention was to use a descriptive name or the one
used in the datasheet but Documentation/devicetree/bindings/gpio/gpio.txt
says nothing about it.

I'll change it to pdn-gpios since it could be easier to match with the doc.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 07/10] [media] tvp5150: Add device tree binding document

[Laurent Pinchart]

On Wednesday 06 January 2016 07:46:35 Javier Martinez Canillas wrote:
> On 01/06/2016 07:39 AM, Laurent Pinchart wrote:
> > Hi Javier,
> > 
> > Thank you for the patch.
> 
> Thanks a lot for your feedback.
> 
> [snip]
> 
> >> +
> >> +Optional Properties:
> >> +- powerdown-gpios: phandle for the GPIO connected to the PDN pin, if
> >> any.
> > 
> > The signal is called PDN in the datasheet, so it might make sense to call
> > this pdn-gpios. I have no strong opinion on this, I'll let you decide
> > what you think is best.
> 
> Yes, I wondered if the convention was to use a descriptive name or the one
> used in the datasheet but Documentation/devicetree/bindings/gpio/gpio.txt
> says nothing about it.

The device tree maintainers might want to comment on that :-)

> I'll change it to pdn-gpios since it could be easier to match with the doc.

-- 
Regards,

Laurent Pinchart

[PATCH 08/10] [media] tvp5150: Add OF match table

[Javier Martinez Canillas]

From: Eduard Gavin <egavinc@gmail.com>

The Documentation/devicetree/bindings/media/i2c/tvp5150.txt DT binding doc
lists "ti,tvp5150" as the device compatible string but the driver does not
have an OF match table. Add the table to the driver so the I2C core can do
an OF style match.

Signed-off-by: Eduard Gavin <egavinc@gmail.com>
Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---

 drivers/media/i2c/tvp5150.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index 105bd1c6b17f..caac96a577f8 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -1295,8 +1295,17 @@ static const struct i2c_device_id tvp5150_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, tvp5150_id);
 
+#if IS_ENABLED(CONFIG_OF)
+static const struct of_device_id tvp5150_of_match[] = {
+	{ .compatible = "ti,tvp5150", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, tvp5150_of_match);
+#endif
+
 static struct i2c_driver tvp5150_driver = {
 	.driver = {
+		.of_match_table = of_match_ptr(tvp5150_of_match),
 		.name	= "tvp5150",
 	},
 	.probe		= tvp5150_probe,
-- 
2.4.3

Re: [PATCH 08/10] [media] tvp5150: Add OF match table

[Laurent Pinchart]

Hi Javier,

Thank you for the patch.

On Monday 04 January 2016 09:25:30 Javier Martinez Canillas wrote:
> From: Eduard Gavin <egavinc@gmail.com>
> 
> The Documentation/devicetree/bindings/media/i2c/tvp5150.txt DT binding doc
> lists "ti,tvp5150" as the device compatible string but the driver does not
> have an OF match table. Add the table to the driver so the I2C core can do
> an OF style match.
> 
> Signed-off-by: Eduard Gavin <egavinc@gmail.com>
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> 
>  drivers/media/i2c/tvp5150.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> index 105bd1c6b17f..caac96a577f8 100644
> --- a/drivers/media/i2c/tvp5150.c
> +++ b/drivers/media/i2c/tvp5150.c
> @@ -1295,8 +1295,17 @@ static const struct i2c_device_id tvp5150_id[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, tvp5150_id);
> 
> +#if IS_ENABLED(CONFIG_OF)
> +static const struct of_device_id tvp5150_of_match[] = {
> +	{ .compatible = "ti,tvp5150", },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, tvp5150_of_match);
> +#endif
> +
>  static struct i2c_driver tvp5150_driver = {
>  	.driver = {
> +		.of_match_table = of_match_ptr(tvp5150_of_match),
>  		.name	= "tvp5150",
>  	},
>  	.probe		= tvp5150_probe,

-- 
Regards,

Laurent Pinchart

[PATCH 09/10] [media] tvp5150: Initialize the chip on probe

[Javier Martinez Canillas]

After power-up, the tvp5150 decoder is in a unknown state until the
RESETB pin is driven LOW which reset all the registers and restarts
the chip's internal state machine.

The init sequence has some timing constraints and the RESETB signal
can only be used if the PDN (Power-down) pin is first released.

So, the initialization sequence is as follows:

1- PDN (active-low) is driven HIGH so the chip is power-up
2- A 20 ms delay is needed before sending a RESETB (active-low) signal.
3- The RESETB pulse duration is 500 ns.
4- A 200 us delay is needed for the I2C client to be active after reset.

This patch used as a reference the logic in the IGEPv2 board file from
the ISEE 2.6.37 vendor tree.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---

 drivers/media/i2c/tvp5150.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index caac96a577f8..fed89a811ab7 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -5,6 +5,7 @@
  * This code is placed under the terms of the GNU General Public License v2
  */
 
+#include <linux/of_gpio.h>
 #include <linux/i2c.h>
 #include <linux/slab.h>
 #include <linux/videodev2.h>
@@ -1197,6 +1198,36 @@ static int tvp5150_detect_version(struct tvp5150 *core)
 	return 0;
 }
 
+static inline int tvp5150_init(struct i2c_client *c)
+{
+	struct gpio_desc *pdn_gpio;
+	struct gpio_desc *reset_gpio;
+
+	pdn_gpio = devm_gpiod_get_optional(&c->dev, "powerdown", GPIOD_OUT_HIGH);
+	if (IS_ERR(pdn_gpio))
+		return PTR_ERR(pdn_gpio);
+
+	if (pdn_gpio) {
+		gpiod_set_value_cansleep(pdn_gpio, 0);
+		/* Delay time between power supplies active and reset */
+		msleep(20);
+	}
+
+	reset_gpio = devm_gpiod_get_optional(&c->dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(reset_gpio))
+		return PTR_ERR(reset_gpio);
+
+	if (reset_gpio) {
+		/* RESETB pulse duration */
+		ndelay(500);
+		gpiod_set_value_cansleep(reset_gpio, 0);
+		/* Delay time between end of reset to I2C active */
+		usleep_range(200, 250);
+	}
+
+	return 0;
+}
+
 static int tvp5150_probe(struct i2c_client *c,
 			 const struct i2c_device_id *id)
 {
@@ -1209,6 +1240,10 @@ static int tvp5150_probe(struct i2c_client *c,
 	     I2C_FUNC_SMBUS_READ_BYTE | I2C_FUNC_SMBUS_WRITE_BYTE_DATA))
 		return -EIO;
 
+	res = tvp5150_init(c);
+	if (res)
+		return res;
+
 	core = devm_kzalloc(&c->dev, sizeof(*core), GFP_KERNEL);
 	if (!core)
 		return -ENOMEM;
-- 
2.4.3

Re: [PATCH 09/10] [media] tvp5150: Initialize the chip on probe

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2207 bytes --]

Hi Javier,

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v4.4-rc8 next-20151231]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Javier-Martinez-Canillas/tvp5150-add-MC-and-DT-support/20160104-203224
base:   git://linuxtv.org/media_tree.git master
config: x86_64-randconfig-x008-01040711 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/media/i2c/tvp5150.c: In function 'tvp5150_init':
>> drivers/media/i2c/tvp5150.c:1206:13: error: implicit declaration of function 'devm_gpiod_get_optional' [-Werror=implicit-function-declaration]
     pdn_gpio = devm_gpiod_get_optional(&c->dev, "powerdown", GPIOD_OUT_HIGH);
                ^
>> drivers/media/i2c/tvp5150.c:1206:59: error: 'GPIOD_OUT_HIGH' undeclared (first use in this function)
     pdn_gpio = devm_gpiod_get_optional(&c->dev, "powerdown", GPIOD_OUT_HIGH);
                                                              ^
   drivers/media/i2c/tvp5150.c:1206:59: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/media/i2c/tvp5150.c:1211:3: error: implicit declaration of function 'gpiod_set_value_cansleep' [-Werror=implicit-function-declaration]
      gpiod_set_value_cansleep(pdn_gpio, 0);
      ^
   cc1: some warnings being treated as errors

vim +/devm_gpiod_get_optional +1206 drivers/media/i2c/tvp5150.c

  1200	
  1201	static inline int tvp5150_init(struct i2c_client *c)
  1202	{
  1203		struct gpio_desc *pdn_gpio;
  1204		struct gpio_desc *reset_gpio;
  1205	
> 1206		pdn_gpio = devm_gpiod_get_optional(&c->dev, "powerdown", GPIOD_OUT_HIGH);
  1207		if (IS_ERR(pdn_gpio))
  1208			return PTR_ERR(pdn_gpio);
  1209	
  1210		if (pdn_gpio) {
> 1211			gpiod_set_value_cansleep(pdn_gpio, 0);
  1212			/* Delay time between power supplies active and reset */
  1213			msleep(20);
  1214		}

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 21835 bytes --]

Re: [PATCH 09/10] [media] tvp5150: Initialize the chip on probe

[Javier Martinez Canillas]

Hello,

On 01/04/2016 09:40 AM, kbuild test robot wrote:
> Hi Javier,
> 
> [auto build test ERROR on linuxtv-media/master]
> [also build test ERROR on v4.4-rc8 next-20151231]
> [if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Javier-Martinez-Canillas/tvp5150-add-MC-and-DT-support/20160104-203224
> base:   git://linuxtv.org/media_tree.git master
> config: x86_64-randconfig-x008-01040711 (attached as .config)
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=x86_64 
> 
> All errors (new ones prefixed by >>):
> 
>    drivers/media/i2c/tvp5150.c: In function 'tvp5150_init':
>>> drivers/media/i2c/tvp5150.c:1206:13: error: implicit declaration of function 'devm_gpiod_get_optional' [-Werror=implicit-function-declaration]
>      pdn_gpio = devm_gpiod_get_optional(&c->dev, "powerdown", GPIOD_OUT_HIGH);
>                 ^
>>> drivers/media/i2c/tvp5150.c:1206:59: error: 'GPIOD_OUT_HIGH' undeclared (first use in this function)
>      pdn_gpio = devm_gpiod_get_optional(&c->dev, "powerdown", GPIOD_OUT_HIGH);
>                                                               ^
>    drivers/media/i2c/tvp5150.c:1206:59: note: each undeclared identifier is reported only once for each function it appears in
>>> drivers/media/i2c/tvp5150.c:1211:3: error: implicit declaration of function 'gpiod_set_value_cansleep' [-Werror=implicit-function-declaration]
>       gpiod_set_value_cansleep(pdn_gpio, 0);
>

Sigh, it's caused by a missing include for the <linux/gpio/consumer.h> header.

Thanks for reporting, I'll wait a couple of days to see if I get more feedback
and then post a v2 fixing this.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/10] [media] tvp5150: Initialize the chip on probe

[Laurent Pinchart]

Hi Javier,

Thankk you for the patch.

On Monday 04 January 2016 09:25:31 Javier Martinez Canillas wrote:
> After power-up, the tvp5150 decoder is in a unknown state until the
> RESETB pin is driven LOW which reset all the registers and restarts
> the chip's internal state machine.
> 
> The init sequence has some timing constraints and the RESETB signal
> can only be used if the PDN (Power-down) pin is first released.
> 
> So, the initialization sequence is as follows:
> 
> 1- PDN (active-low) is driven HIGH so the chip is power-up
> 2- A 20 ms delay is needed before sending a RESETB (active-low) signal.
> 3- The RESETB pulse duration is 500 ns.
> 4- A 200 us delay is needed for the I2C client to be active after reset.
> 
> This patch used as a reference the logic in the IGEPv2 board file from
> the ISEE 2.6.37 vendor tree.
> 
> Signed-off-by: Javier Martinez Canillas <javier-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>
> ---
> 
>  drivers/media/i2c/tvp5150.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> index caac96a577f8..fed89a811ab7 100644
> --- a/drivers/media/i2c/tvp5150.c
> +++ b/drivers/media/i2c/tvp5150.c
> @@ -5,6 +5,7 @@
>   * This code is placed under the terms of the GNU General Public License v2
> */
> 
> +#include <linux/of_gpio.h>

Let's keep the headers sorted alphabetically if you don't mind :-)

>  #include <linux/i2c.h>
>  #include <linux/slab.h>
>  #include <linux/videodev2.h>
> @@ -1197,6 +1198,36 @@ static int tvp5150_detect_version(struct tvp5150
> *core) return 0;
>  }
> 
> +static inline int tvp5150_init(struct i2c_client *c)
> +{
> +	struct gpio_desc *pdn_gpio;
> +	struct gpio_desc *reset_gpio;
> +
> +	pdn_gpio = devm_gpiod_get_optional(&c->dev, "powerdown", GPIOD_OUT_HIGH);
> +	if (IS_ERR(pdn_gpio))
> +		return PTR_ERR(pdn_gpio);
> +
> +	if (pdn_gpio) {
> +		gpiod_set_value_cansleep(pdn_gpio, 0);
> +		/* Delay time between power supplies active and reset */
> +		msleep(20);

How about usleep_range() instead ?

> +	}
> +
> +	reset_gpio = devm_gpiod_get_optional(&c->dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(reset_gpio))
> +		return PTR_ERR(reset_gpio);
> +
> +	if (reset_gpio) {
> +		/* RESETB pulse duration */
> +		ndelay(500);

Is the timing so sensitive that we need a delay, or could we use 
usleep_range() ?

> +		gpiod_set_value_cansleep(reset_gpio, 0);
> +		/* Delay time between end of reset to I2C active */
> +		usleep_range(200, 250);
> +	}
> +
> +	return 0;
> +}
> +
>  static int tvp5150_probe(struct i2c_client *c,
>  			 const struct i2c_device_id *id)
>  {
> @@ -1209,6 +1240,10 @@ static int tvp5150_probe(struct i2c_client *c,
>  	     I2C_FUNC_SMBUS_READ_BYTE | I2C_FUNC_SMBUS_WRITE_BYTE_DATA))
>  		return -EIO;
> 
> +	res = tvp5150_init(c);
> +	if (res)
> +		return res;
> +
>  	core = devm_kzalloc(&c->dev, sizeof(*core), GFP_KERNEL);
>  	if (!core)
>  		return -ENOMEM;

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/10] [media] tvp5150: Initialize the chip on probe

[Javier Martinez Canillas]

Hello Laurent,

On 01/06/2016 07:59 AM, Laurent Pinchart wrote:
> Hi Javier,
> 
> Thankk you for the patch.
>

Thanks a lot for your feedback.
 
> On Monday 04 January 2016 09:25:31 Javier Martinez Canillas wrote:
>> After power-up, the tvp5150 decoder is in a unknown state until the
>> RESETB pin is driven LOW which reset all the registers and restarts
>> the chip's internal state machine.
>>
>> The init sequence has some timing constraints and the RESETB signal
>> can only be used if the PDN (Power-down) pin is first released.
>>
>> So, the initialization sequence is as follows:
>>
>> 1- PDN (active-low) is driven HIGH so the chip is power-up
>> 2- A 20 ms delay is needed before sending a RESETB (active-low) signal.
>> 3- The RESETB pulse duration is 500 ns.
>> 4- A 200 us delay is needed for the I2C client to be active after reset.
>>
>> This patch used as a reference the logic in the IGEPv2 board file from
>> the ISEE 2.6.37 vendor tree.
>>
>> Signed-off-by: Javier Martinez Canillas <javier-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>
>> ---
>>
>>  drivers/media/i2c/tvp5150.c | 35 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 35 insertions(+)
>>
>> diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
>> index caac96a577f8..fed89a811ab7 100644
>> --- a/drivers/media/i2c/tvp5150.c
>> +++ b/drivers/media/i2c/tvp5150.c
>> @@ -5,6 +5,7 @@
>>   * This code is placed under the terms of the GNU General Public License v2
>> */
>>
>> +#include <linux/of_gpio.h>
> 
> Let's keep the headers sorted alphabetically if you don't mind :-)
>

Right, sorry about that.
 
>>  #include <linux/i2c.h>
>>  #include <linux/slab.h>
>>  #include <linux/videodev2.h>
>> @@ -1197,6 +1198,36 @@ static int tvp5150_detect_version(struct tvp5150
>> *core) return 0;
>>  }
>>
>> +static inline int tvp5150_init(struct i2c_client *c)
>> +{
>> +	struct gpio_desc *pdn_gpio;
>> +	struct gpio_desc *reset_gpio;
>> +
>> +	pdn_gpio = devm_gpiod_get_optional(&c->dev, "powerdown", GPIOD_OUT_HIGH);
>> +	if (IS_ERR(pdn_gpio))
>> +		return PTR_ERR(pdn_gpio);
>> +
>> +	if (pdn_gpio) {
>> +		gpiod_set_value_cansleep(pdn_gpio, 0);
>> +		/* Delay time between power supplies active and reset */
>> +		msleep(20);
> 
> How about usleep_range() instead ?
>

Documentation/timers/timers-howto.txt says that usleep_range() should be
used for 1ms - 20ms and msleep() for 20ms+ so since this was a boundary
value, I chose for msleep() but I can use usleep_range() if you want.

I've no strong opinion on this.

>> +	}
>> +
>> +	reset_gpio = devm_gpiod_get_optional(&c->dev, "reset", GPIOD_OUT_HIGH);
>> +	if (IS_ERR(reset_gpio))
>> +		return PTR_ERR(reset_gpio);
>> +
>> +	if (reset_gpio) {
>> +		/* RESETB pulse duration */
>> +		ndelay(500);
> 
> Is the timing so sensitive that we need a delay, or could we use 
> usleep_range() ?
>

According to my tests, it is a minimum value but I chose to do a delay since
the time is very short and also Documentation/timers/timers-howto.txt says
that using usleep for very short periods may not be worth it due the overhead
of setting up the hrtimers for usleep.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 10/10] [media] tvp5150: Configure data interface via pdata or DT

[Javier Martinez Canillas]

The video decoder supports either 8-bit 4:2:2 YUV with discrete syncs
or 8-bit ITU-R BT.656 with embedded syncs output format but currently
BT.656 it's always reported. Allow to configure the format to use via
either platform data or a device tree definition.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>

---

 drivers/media/i2c/tvp5150.c | 61 +++++++++++++++++++++++++++++++++++++++++++--
 include/media/i2c/tvp5150.h |  5 ++++
 2 files changed, 64 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index fed89a811ab7..8bce45a6e264 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/of_gpio.h>
+#include <linux/of_graph.h>
 #include <linux/i2c.h>
 #include <linux/slab.h>
 #include <linux/videodev2.h>
@@ -15,6 +16,7 @@
 #include <media/v4l2-device.h>
 #include <media/i2c/tvp5150.h>
 #include <media/v4l2-ctrls.h>
+#include <media/v4l2-of.h>
 
 #include "tvp5150_reg.h"
 
@@ -39,6 +41,7 @@ struct tvp5150 {
 	struct media_pad pad;
 	struct v4l2_ctrl_handler hdl;
 	struct v4l2_rect rect;
+	struct tvp5150_platform_data *pdata;
 
 	v4l2_std_id norm;	/* Current set standard */
 	u32 input;
@@ -757,6 +760,7 @@ static int tvp5150_s_std(struct v4l2_subdev *sd, v4l2_std_id std)
 static int tvp5150_reset(struct v4l2_subdev *sd, u32 val)
 {
 	struct tvp5150 *decoder = to_tvp5150(sd);
+	struct tvp5150_platform_data *pdata = decoder->pdata;
 
 	/* Initializes TVP5150 to its default values */
 	tvp5150_write_inittab(sd, tvp5150_init_default);
@@ -774,6 +778,10 @@ static int tvp5150_reset(struct v4l2_subdev *sd, u32 val)
 	v4l2_ctrl_handler_setup(&decoder->hdl);
 
 	tvp5150_set_std(sd, decoder->norm);
+
+	if (pdata && pdata->bus_type == V4L2_MBUS_PARALLEL)
+		tvp5150_write(sd, TVP5150_DATA_RATE_SEL, 0x40);
+
 	return 0;
 };
 
@@ -940,6 +948,16 @@ static int tvp5150_cropcap(struct v4l2_subdev *sd, struct v4l2_cropcap *a)
 static int tvp5150_g_mbus_config(struct v4l2_subdev *sd,
 				 struct v4l2_mbus_config *cfg)
 {
+	struct tvp5150_platform_data *pdata = to_tvp5150(sd)->pdata;
+
+	if (pdata) {
+		cfg->type = pdata->bus_type;
+		cfg->flags = pdata->parallel_flags;
+
+		return 0;
+	}
+
+	/* Default values if no platform data was provided */
 	cfg->type = V4L2_MBUS_BT656;
 	cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING
 		   | V4L2_MBUS_FIELD_EVEN_LOW | V4L2_MBUS_DATA_ACTIVE_HIGH;
@@ -986,13 +1004,20 @@ static int tvp5150_enum_frame_size(struct v4l2_subdev *sd,
 
 static int tvp5150_s_stream(struct v4l2_subdev *sd, int enable)
 {
+	struct tvp5150_platform_data *pdata = to_tvp5150(sd)->pdata;
+	/* Output format: 8-bit ITU-R BT.656 with embedded syncs */
+	int val = 0x09;
+
+	/* Output format: 8-bit 4:2:2 YUV with discrete sync */
+	if (pdata && pdata->bus_type == V4L2_MBUS_PARALLEL)
+		val = 0x0d;
+
 	/* Initializes TVP5150 to its default values */
 	/* # set PCLK (27MHz) */
 	tvp5150_write(sd, TVP5150_CONF_SHARED_PIN, 0x00);
 
-	/* Output format: 8-bit ITU-R BT.656 with embedded syncs */
 	if (enable)
-		tvp5150_write(sd, TVP5150_MISC_CTL, 0x09);
+		tvp5150_write(sd, TVP5150_MISC_CTL, val);
 	else
 		tvp5150_write(sd, TVP5150_MISC_CTL, 0x00);
 
@@ -1228,11 +1253,42 @@ static inline int tvp5150_init(struct i2c_client *c)
 	return 0;
 }
 
+static struct tvp5150_platform_data *tvp5150_get_pdata(struct device *dev)
+{
+	struct tvp5150_platform_data *pdata = dev_get_platdata(dev);
+	struct v4l2_of_endpoint bus_cfg;
+	struct device_node *ep;
+
+	if (pdata)
+		return pdata;
+
+	if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
+		pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+		if (!pdata)
+			return NULL;
+
+		ep = of_graph_get_next_endpoint(dev->of_node, NULL);
+		if (!ep)
+			return NULL;
+
+		v4l2_of_parse_endpoint(ep, &bus_cfg);
+
+		pdata->bus_type = bus_cfg.bus_type;
+		pdata->parallel_flags = bus_cfg.bus.parallel.flags;
+
+		of_node_put(ep);
+		return pdata;
+	}
+
+	return NULL;
+}
+
 static int tvp5150_probe(struct i2c_client *c,
 			 const struct i2c_device_id *id)
 {
 	struct tvp5150 *core;
 	struct v4l2_subdev *sd;
+	struct tvp5150_platform_data *pdata = tvp5150_get_pdata(&c->dev);
 	int res;
 
 	/* Check if the adapter supports the needed features */
@@ -1262,6 +1318,7 @@ static int tvp5150_probe(struct i2c_client *c,
 	if (res < 0)
 		return res;
 
+	core->pdata = pdata;
 	core->norm = V4L2_STD_ALL;	/* Default is autodetect */
 	core->input = TVP5150_COMPOSITE1;
 	core->enable = 1;
diff --git a/include/media/i2c/tvp5150.h b/include/media/i2c/tvp5150.h
index 649908a25605..e4cda0c843df 100644
--- a/include/media/i2c/tvp5150.h
+++ b/include/media/i2c/tvp5150.h
@@ -30,4 +30,9 @@
 #define TVP5150_NORMAL       0
 #define TVP5150_BLACK_SCREEN 1
 
+struct tvp5150_platform_data {
+	enum v4l2_mbus_type bus_type;
+	unsigned int parallel_flags;
+};
+
 #endif
-- 
2.4.3

Re: [PATCH 10/10] [media] tvp5150: Configure data interface via pdata or DT

[Laurent Pinchart]

Hi Javier,

Thank you for the patch.

On Monday 04 January 2016 09:25:32 Javier Martinez Canillas wrote:
> The video decoder supports either 8-bit 4:2:2 YUV with discrete syncs
> or 8-bit ITU-R BT.656 with embedded syncs output format but currently
> BT.656 it's always reported. Allow to configure the format to use via
> either platform data or a device tree definition.
> 
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> ---
>  drivers/media/i2c/tvp5150.c | 61 ++++++++++++++++++++++++++++++++++++++++--
>  include/media/i2c/tvp5150.h |  5 ++++
>  2 files changed, 64 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> index fed89a811ab7..8bce45a6e264 100644
> --- a/drivers/media/i2c/tvp5150.c
> +++ b/drivers/media/i2c/tvp5150.c
> @@ -6,6 +6,7 @@
>   */
> 
>  #include <linux/of_gpio.h>
> +#include <linux/of_graph.h>
>  #include <linux/i2c.h>
>  #include <linux/slab.h>
>  #include <linux/videodev2.h>
> @@ -15,6 +16,7 @@
>  #include <media/v4l2-device.h>
>  #include <media/i2c/tvp5150.h>
>  #include <media/v4l2-ctrls.h>
> +#include <media/v4l2-of.h>
> 
>  #include "tvp5150_reg.h"
> 
> @@ -39,6 +41,7 @@ struct tvp5150 {
>  	struct media_pad pad;
>  	struct v4l2_ctrl_handler hdl;
>  	struct v4l2_rect rect;
> +	struct tvp5150_platform_data *pdata;

How about embedding tvp5150_platform_data instead of pointing to it ? It would 
save an allocation and you could get rid of the pdata != NULL checks.

>  	v4l2_std_id norm;	/* Current set standard */
>  	u32 input;
> @@ -757,6 +760,7 @@ static int tvp5150_s_std(struct v4l2_subdev *sd,
> v4l2_std_id std) static int tvp5150_reset(struct v4l2_subdev *sd, u32 val)
>  {
>  	struct tvp5150 *decoder = to_tvp5150(sd);
> +	struct tvp5150_platform_data *pdata = decoder->pdata;
> 
>  	/* Initializes TVP5150 to its default values */
>  	tvp5150_write_inittab(sd, tvp5150_init_default);
> @@ -774,6 +778,10 @@ static int tvp5150_reset(struct v4l2_subdev *sd, u32
> val) v4l2_ctrl_handler_setup(&decoder->hdl);
> 
>  	tvp5150_set_std(sd, decoder->norm);
> +
> +	if (pdata && pdata->bus_type == V4L2_MBUS_PARALLEL)
> +		tvp5150_write(sd, TVP5150_DATA_RATE_SEL, 0x40);
> +
>  	return 0;
>  };
> 
> @@ -940,6 +948,16 @@ static int tvp5150_cropcap(struct v4l2_subdev *sd,
> struct v4l2_cropcap *a) static int tvp5150_g_mbus_config(struct v4l2_subdev
> *sd,
>  				 struct v4l2_mbus_config *cfg)
>  {
> +	struct tvp5150_platform_data *pdata = to_tvp5150(sd)->pdata;
> +
> +	if (pdata) {
> +		cfg->type = pdata->bus_type;
> +		cfg->flags = pdata->parallel_flags;

The clock and sync signals polarity don't seem configurable, shouldn't they 
just be hardcoded as currently done ?

> +		return 0;
> +	}
> +
> +	/* Default values if no platform data was provided */
>  	cfg->type = V4L2_MBUS_BT656;
>  	cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING
> 
>  		   | V4L2_MBUS_FIELD_EVEN_LOW | V4L2_MBUS_DATA_ACTIVE_HIGH;
> 
> @@ -986,13 +1004,20 @@ static int tvp5150_enum_frame_size(struct v4l2_subdev
> *sd,
> 
>  static int tvp5150_s_stream(struct v4l2_subdev *sd, int enable)
>  {
> +	struct tvp5150_platform_data *pdata = to_tvp5150(sd)->pdata;
> +	/* Output format: 8-bit ITU-R BT.656 with embedded syncs */
> +	int val = 0x09;
> +
> +	/* Output format: 8-bit 4:2:2 YUV with discrete sync */
> +	if (pdata && pdata->bus_type == V4L2_MBUS_PARALLEL)
> +		val = 0x0d;
> +
>  	/* Initializes TVP5150 to its default values */
>  	/* # set PCLK (27MHz) */
>  	tvp5150_write(sd, TVP5150_CONF_SHARED_PIN, 0x00);
> 
> -	/* Output format: 8-bit ITU-R BT.656 with embedded syncs */
>  	if (enable)
> -		tvp5150_write(sd, TVP5150_MISC_CTL, 0x09);
> +		tvp5150_write(sd, TVP5150_MISC_CTL, val);
>  	else
>  		tvp5150_write(sd, TVP5150_MISC_CTL, 0x00);
> 
> @@ -1228,11 +1253,42 @@ static inline int tvp5150_init(struct i2c_client *c)
> return 0;
>  }
> 
> +static struct tvp5150_platform_data *tvp5150_get_pdata(struct device *dev)
> +{
> +	struct tvp5150_platform_data *pdata = dev_get_platdata(dev);
> +	struct v4l2_of_endpoint bus_cfg;
> +	struct device_node *ep;
> +
> +	if (pdata)
> +		return pdata;

Nobody uses platform data today, I wonder whether we shouldn't postpone adding 
support for it until we have a use case. Embedded systems (at least the ARM-
based ones) should use DT.

> +	if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
> +		pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +		if (!pdata)
> +			return NULL;
> +
> +		ep = of_graph_get_next_endpoint(dev->of_node, NULL);
> +		if (!ep)
> +			return NULL;
> +
> +		v4l2_of_parse_endpoint(ep, &bus_cfg);

Shouldn't you check the return value of the function ?

> +
> +		pdata->bus_type = bus_cfg.bus_type;
> +		pdata->parallel_flags = bus_cfg.bus.parallel.flags;

The V4L2_MBUS_DATA_ACTIVE_HIGH flags set returned by tvp5150_g_mbus_config() 
when pdata is NULL is never set by v4l2_of_parse_endpoint(), should you add it 
unconditionally ?

> +		of_node_put(ep);
> +		return pdata;
> +	}
> +
> +	return NULL;
> +}
> +
>  static int tvp5150_probe(struct i2c_client *c,
>  			 const struct i2c_device_id *id)
>  {
>  	struct tvp5150 *core;
>  	struct v4l2_subdev *sd;
> +	struct tvp5150_platform_data *pdata = tvp5150_get_pdata(&c->dev);
>  	int res;
> 
>  	/* Check if the adapter supports the needed features */
> @@ -1262,6 +1318,7 @@ static int tvp5150_probe(struct i2c_client *c,
>  	if (res < 0)
>  		return res;
> 
> +	core->pdata = pdata;
>  	core->norm = V4L2_STD_ALL;	/* Default is autodetect */
>  	core->input = TVP5150_COMPOSITE1;
>  	core->enable = 1;
> diff --git a/include/media/i2c/tvp5150.h b/include/media/i2c/tvp5150.h
> index 649908a25605..e4cda0c843df 100644
> --- a/include/media/i2c/tvp5150.h
> +++ b/include/media/i2c/tvp5150.h
> @@ -30,4 +30,9 @@
>  #define TVP5150_NORMAL       0
>  #define TVP5150_BLACK_SCREEN 1
> 
> +struct tvp5150_platform_data {
> +	enum v4l2_mbus_type bus_type;
> +	unsigned int parallel_flags;
> +};
> +
>  #endif

-- 
Regards,

Laurent Pinchart

Re: [PATCH 10/10] [media] tvp5150: Configure data interface via pdata or DT

[Javier Martinez Canillas]

Hello Laurent,

Thanks a lot for your feedback.

On 01/06/2016 07:56 AM, Laurent Pinchart wrote:
> Hi Javier,
> 
> Thank you for the patch.
> 
> On Monday 04 January 2016 09:25:32 Javier Martinez Canillas wrote:
>> The video decoder supports either 8-bit 4:2:2 YUV with discrete syncs
>> or 8-bit ITU-R BT.656 with embedded syncs output format but currently
>> BT.656 it's always reported. Allow to configure the format to use via
>> either platform data or a device tree definition.
>>
>> Signed-off-by: Javier Martinez Canillas <javier-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>
>> ---
>>  drivers/media/i2c/tvp5150.c | 61 ++++++++++++++++++++++++++++++++++++++++--
>>  include/media/i2c/tvp5150.h |  5 ++++
>>  2 files changed, 64 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
>> index fed89a811ab7..8bce45a6e264 100644
>> --- a/drivers/media/i2c/tvp5150.c
>> +++ b/drivers/media/i2c/tvp5150.c
>> @@ -6,6 +6,7 @@
>>   */
>>
>>  #include <linux/of_gpio.h>
>> +#include <linux/of_graph.h>
>>  #include <linux/i2c.h>
>>  #include <linux/slab.h>
>>  #include <linux/videodev2.h>
>> @@ -15,6 +16,7 @@
>>  #include <media/v4l2-device.h>
>>  #include <media/i2c/tvp5150.h>
>>  #include <media/v4l2-ctrls.h>
>> +#include <media/v4l2-of.h>
>>
>>  #include "tvp5150_reg.h"
>>
>> @@ -39,6 +41,7 @@ struct tvp5150 {
>>  	struct media_pad pad;
>>  	struct v4l2_ctrl_handler hdl;
>>  	struct v4l2_rect rect;
>> +	struct tvp5150_platform_data *pdata;
> 
> How about embedding tvp5150_platform_data instead of pointing to it ? It would 
> save an allocation and you could get rid of the pdata != NULL checks.
>

Agreed.
 
>>  	v4l2_std_id norm;	/* Current set standard */
>>  	u32 input;
>> @@ -757,6 +760,7 @@ static int tvp5150_s_std(struct v4l2_subdev *sd,
>> v4l2_std_id std) static int tvp5150_reset(struct v4l2_subdev *sd, u32 val)
>>  {
>>  	struct tvp5150 *decoder = to_tvp5150(sd);
>> +	struct tvp5150_platform_data *pdata = decoder->pdata;
>>
>>  	/* Initializes TVP5150 to its default values */
>>  	tvp5150_write_inittab(sd, tvp5150_init_default);
>> @@ -774,6 +778,10 @@ static int tvp5150_reset(struct v4l2_subdev *sd, u32
>> val) v4l2_ctrl_handler_setup(&decoder->hdl);
>>
>>  	tvp5150_set_std(sd, decoder->norm);
>> +
>> +	if (pdata && pdata->bus_type == V4L2_MBUS_PARALLEL)
>> +		tvp5150_write(sd, TVP5150_DATA_RATE_SEL, 0x40);
>> +
>>  	return 0;
>>  };
>>
>> @@ -940,6 +948,16 @@ static int tvp5150_cropcap(struct v4l2_subdev *sd,
>> struct v4l2_cropcap *a) static int tvp5150_g_mbus_config(struct v4l2_subdev
>> *sd,
>>  				 struct v4l2_mbus_config *cfg)
>>  {
>> +	struct tvp5150_platform_data *pdata = to_tvp5150(sd)->pdata;
>> +
>> +	if (pdata) {
>> +		cfg->type = pdata->bus_type;
>> +		cfg->flags = pdata->parallel_flags;
> 
> The clock and sync signals polarity don't seem configurable, shouldn't they 
> just be hardcoded as currently done ?
>

That's a very good question, I added the flags because according to
Documentation/devicetree/bindings/media/video-interfaces.txt, the way
to define that the output format will be BT.656 is to avoid defining
{hsync,vsync,field-even}-active properties.

IOW, if parallel sync is used, then these properties have to be defined
and it felt strange to not use in the driver what is defined in the DT.

>> +		return 0;
>> +	}
>> +
>> +	/* Default values if no platform data was provided */
>>  	cfg->type = V4L2_MBUS_BT656;
>>  	cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING
>>
>>  		   | V4L2_MBUS_FIELD_EVEN_LOW | V4L2_MBUS_DATA_ACTIVE_HIGH;
>>
>> @@ -986,13 +1004,20 @@ static int tvp5150_enum_frame_size(struct v4l2_subdev
>> *sd,
>>
>>  static int tvp5150_s_stream(struct v4l2_subdev *sd, int enable)
>>  {
>> +	struct tvp5150_platform_data *pdata = to_tvp5150(sd)->pdata;
>> +	/* Output format: 8-bit ITU-R BT.656 with embedded syncs */
>> +	int val = 0x09;
>> +
>> +	/* Output format: 8-bit 4:2:2 YUV with discrete sync */
>> +	if (pdata && pdata->bus_type == V4L2_MBUS_PARALLEL)
>> +		val = 0x0d;
>> +
>>  	/* Initializes TVP5150 to its default values */
>>  	/* # set PCLK (27MHz) */
>>  	tvp5150_write(sd, TVP5150_CONF_SHARED_PIN, 0x00);
>>
>> -	/* Output format: 8-bit ITU-R BT.656 with embedded syncs */
>>  	if (enable)
>> -		tvp5150_write(sd, TVP5150_MISC_CTL, 0x09);
>> +		tvp5150_write(sd, TVP5150_MISC_CTL, val);
>>  	else
>>  		tvp5150_write(sd, TVP5150_MISC_CTL, 0x00);
>>
>> @@ -1228,11 +1253,42 @@ static inline int tvp5150_init(struct i2c_client *c)
>> return 0;
>>  }
>>
>> +static struct tvp5150_platform_data *tvp5150_get_pdata(struct device *dev)
>> +{
>> +	struct tvp5150_platform_data *pdata = dev_get_platdata(dev);
>> +	struct v4l2_of_endpoint bus_cfg;
>> +	struct device_node *ep;
>> +
>> +	if (pdata)
>> +		return pdata;
> 
> Nobody uses platform data today, I wonder whether we shouldn't postpone adding 
> support for it until we have a use case. Embedded systems (at least the ARM-
> based ones) should use DT.
>

Yes, I just added it for completeness since in other subsystems I've been
yelled in the past that not all the world is DT and that I needed a pdata :)

But I'll gladly remove it since it means less code which is always good.

>> +	if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
>> +		pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> +		if (!pdata)
>> +			return NULL;
>> +
>> +		ep = of_graph_get_next_endpoint(dev->of_node, NULL);
>> +		if (!ep)
>> +			return NULL;
>> +
>> +		v4l2_of_parse_endpoint(ep, &bus_cfg);
> 
> Shouldn't you check the return value of the function ?
>

Right, the v4l2_of_parse_endpoint() kernel doc says "Return: 0." and most
drivers are not checking the return value so I thought that it couldn't
fail. But now looking at the implementation I see that's not true so I'll
add a check in v2.

I'll also post patches to update v4l2_of_parse_endpoint() kernel-doc and
the drivers that are not currently checking for this return value.

>> +
>> +		pdata->bus_type = bus_cfg.bus_type;
>> +		pdata->parallel_flags = bus_cfg.bus.parallel.flags;
> 
> The V4L2_MBUS_DATA_ACTIVE_HIGH flags set returned by tvp5150_g_mbus_config() 
> when pdata is NULL is never set by v4l2_of_parse_endpoint(), should you add it 
> unconditionally ?
>

But v4l2_of_parse_endpoint() calls v4l2_of_parse_parallel_bus() which does
it or did I read the code incorrectly?

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 10/10] [media] tvp5150: Configure data interface via pdata or DT

[Laurent Pinchart]

Hi Javier,

On Wednesday 06 January 2016 08:27:26 Javier Martinez Canillas wrote:
> On 01/06/2016 07:56 AM, Laurent Pinchart wrote:
> > On Monday 04 January 2016 09:25:32 Javier Martinez Canillas wrote:
> >> The video decoder supports either 8-bit 4:2:2 YUV with discrete syncs
> >> or 8-bit ITU-R BT.656 with embedded syncs output format but currently
> >> BT.656 it's always reported. Allow to configure the format to use via
> >> either platform data or a device tree definition.
> >> 
> >> Signed-off-by: Javier Martinez Canillas <javier-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>
> >> ---
> >> 
> >>  drivers/media/i2c/tvp5150.c | 61 +++++++++++++++++++++++++++++++++++++--
> >>  include/media/i2c/tvp5150.h |  5 ++++
> >>  2 files changed, 64 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> >> index fed89a811ab7..8bce45a6e264 100644
> >> --- a/drivers/media/i2c/tvp5150.c
> >> +++ b/drivers/media/i2c/tvp5150.c

[snip]

> >> @@ -940,6 +948,16 @@ static int tvp5150_cropcap(struct v4l2_subdev *sd,
> >> struct v4l2_cropcap *a)
> >>  static int tvp5150_g_mbus_config(struct v4l2_subdev *sd,
> >>  				 struct v4l2_mbus_config *cfg)
> >>  {
> >> +	struct tvp5150_platform_data *pdata = to_tvp5150(sd)->pdata;
> >> +
> >> +	if (pdata) {
> >> +		cfg->type = pdata->bus_type;
> >> +		cfg->flags = pdata->parallel_flags;
> > 
> > The clock and sync signals polarity don't seem configurable, shouldn't
> > they just be hardcoded as currently done ?
> 
> That's a very good question, I added the flags because according to
> Documentation/devicetree/bindings/media/video-interfaces.txt, the way
> to define that the output format will be BT.656 is to avoid defining
> {hsync,vsync,field-even}-active properties.
> 
> IOW, if parallel sync is used, then these properties have to be defined
> and it felt strange to not use in the driver what is defined in the DT.

In that case we should restrict the values of the properties to what the 
hardware actually supports. I would hardcode the flags here, and check them 
when parsing the endpoint to make sure they're valid.

If you find a register I have missed in the documentation with which 
polarities could be configured then please also feel free to prove me wrong 
:-)

> >> +		return 0;
> >> +	}
> >> +
> >> +	/* Default values if no platform data was provided */
> >>  	cfg->type = V4L2_MBUS_BT656;
> >>  	cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING
> >>  		   | V4L2_MBUS_FIELD_EVEN_LOW | V4L2_MBUS_DATA_ACTIVE_HIGH;

[snip]

> >> @@ -1228,11 +1253,42 @@ static inline int tvp5150_init(struct i2c_client
> >> *c) return 0;
> >> 
> >>  }
> >> 
> >> +static struct tvp5150_platform_data *tvp5150_get_pdata(struct device
> >> *dev)
> >> +{
> >> +	struct tvp5150_platform_data *pdata = dev_get_platdata(dev);
> >> +	struct v4l2_of_endpoint bus_cfg;
> >> +	struct device_node *ep;
> >> +
> >> +	if (pdata)
> >> +		return pdata;
> > 
> > Nobody uses platform data today, I wonder whether we shouldn't postpone
> > adding support for it until we have a use case. Embedded systems (at
> > least the ARM- based ones) should use DT.
> 
> Yes, I just added it for completeness since in other subsystems I've been
> yelled in the past that not all the world is DT and that I needed a pdata :)
> 
> But I'll gladly remove it since it means less code which is always good.
> 
> >> +	if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
> >> +		pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> >> +		if (!pdata)
> >> +			return NULL;
> >> +
> >> +		ep = of_graph_get_next_endpoint(dev->of_node, NULL);
> >> +		if (!ep)
> >> +			return NULL;
> >> +
> >> +		v4l2_of_parse_endpoint(ep, &bus_cfg);
> > 
> > Shouldn't you check the return value of the function ?
> 
> Right, the v4l2_of_parse_endpoint() kernel doc says "Return: 0." and most
> drivers are not checking the return value so I thought that it couldn't
> fail. But now looking at the implementation I see that's not true so I'll
> add a check in v2.
> 
> I'll also post patches to update v4l2_of_parse_endpoint() kernel-doc and
> the drivers that are not currently checking for this return value.

Thank you for that.

> >> +
> >> +		pdata->bus_type = bus_cfg.bus_type;
> >> +		pdata->parallel_flags = bus_cfg.bus.parallel.flags;
> > 
> > The V4L2_MBUS_DATA_ACTIVE_HIGH flags set returned by
> > tvp5150_g_mbus_config() when pdata is NULL is never set by
> > v4l2_of_parse_endpoint(), should you add it unconditionally ?
> 
> But v4l2_of_parse_endpoint() calls v4l2_of_parse_parallel_bus() which does
> it or did I read the code incorrectly?

No, you're right, I had overlooked the V4L2_MBUS_DATA_ACTIVE_HIGH flag when 
reading v4l2_of_parse_parallel_bus(), probably a typo when searching. Please 
ignore that comment.

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 10/10] [media] tvp5150: Configure data interface via pdata or DT

[Javier Martinez Canillas]

Hello Laurent,

On 01/06/2016 10:48 AM, Laurent Pinchart wrote:

[snip]

>>>> @@ -940,6 +948,16 @@ static int tvp5150_cropcap(struct v4l2_subdev *sd,
>>>> struct v4l2_cropcap *a)
>>>>  static int tvp5150_g_mbus_config(struct v4l2_subdev *sd,
>>>>  				 struct v4l2_mbus_config *cfg)
>>>>  {
>>>> +	struct tvp5150_platform_data *pdata = to_tvp5150(sd)->pdata;
>>>> +
>>>> +	if (pdata) {
>>>> +		cfg->type = pdata->bus_type;
>>>> +		cfg->flags = pdata->parallel_flags;
>>>
>>> The clock and sync signals polarity don't seem configurable, shouldn't
>>> they just be hardcoded as currently done ?
>>
>> That's a very good question, I added the flags because according to
>> Documentation/devicetree/bindings/media/video-interfaces.txt, the way
>> to define that the output format will be BT.656 is to avoid defining
>> {hsync,vsync,field-even}-active properties.
>>
>> IOW, if parallel sync is used, then these properties have to be defined
>> and it felt strange to not use in the driver what is defined in the DT.
> 
> In that case we should restrict the values of the properties to what the 
> hardware actually supports. I would hardcode the flags here, and check them 
> when parsing the endpoint to make sure they're valid.
>

That's a good idea, I'll also mention the supported values in the binding doc.
 
> If you find a register I have missed in the documentation with which 
> polarities could be configured then please also feel free to prove me wrong 
> :-)
>

I didn't find either when reading the datasheet to prepare this patch-set
so I think you are correct on that.

[snip]

>>>> +
>>>> +		pdata->bus_type = bus_cfg.bus_type;
>>>> +		pdata->parallel_flags = bus_cfg.bus.parallel.flags;
>>>
>>> The V4L2_MBUS_DATA_ACTIVE_HIGH flags set returned by
>>> tvp5150_g_mbus_config() when pdata is NULL is never set by
>>> v4l2_of_parse_endpoint(), should you add it unconditionally ?
>>
>> But v4l2_of_parse_endpoint() calls v4l2_of_parse_parallel_bus() which does
>> it or did I read the code incorrectly?
> 
> No, you're right, I had overlooked the V4L2_MBUS_DATA_ACTIVE_HIGH flag when 
> reading v4l2_of_parse_parallel_bus(), probably a typo when searching. Please 
> ignore that comment.
> 

Ok, thanks for the clarification.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html