[PATCH v2 0/6] Fix tvp5150 regression with em28xx

[PATCH v2 0/6] Fix tvp5150 regression with em28xx

[Laurent Pinchart]

Hello,

This patch series fixes a regression reported by Devin Heitmueller that
affects a large number of em28xx. The problem was introduced by

commit 13d52fe40f1f7bbad49128e8ee6a2fe5e13dd18d
Author: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
Date:   Tue Jan 26 06:59:39 2016 -0200

    [media] em28xx: fix implementation of s_stream

that started calling s_stream(1) in the em28xx driver when enabling the
stream, resulting in the tvp5150 s_stream() operation writing several
registers with values fit for other platforms (namely OMAP3, possibly others)
but not for em28xx.

The series starts with two unrelated drive-by cleanups and an unrelated bug
fix. It then continues with a patch to remove an unneeded and armful call to
tvp5150_reset() when getting the format from the subdevice (4/6), an update of
an invalid comment and the addition of macros for register bits in order to
make the code more readable (5/6) and actually allow following the incorrect
code flow, and finally a rework of the s_stream() operation to fix the
problem.

Compared to v1,

- Patch 4/5 now calls tvp5150_reset() at probe time
- Patch 5/6 is fixed with an extra ~ removed

I haven't been able to test this with an em28xx device as I don't own any that
contains a tvp5150, but Mauro reported that the series fixes the issue with
his device.

I also haven't been able to test anything on an OMAP3 platform, as the tvp5150
driver go broken on DT-based systems by

commit f7b4b54e63643b740c598e044874c4bffa0f04f2
Author: Javier Martinez Canillas <javier@osg.samsung.com>
Date:   Fri Feb 5 17:09:58 2016 -0200

    [media] tvp5150: add HW input connectors support

Fixing it will be the topic of another patch series.

Laurent Pinchart (6):
  v4l: tvp5150: Compile tvp5150_link_setup out if
    !CONFIG_MEDIA_CONTROLLER
  v4l: tvp5150: Don't inline the tvp5150_selmux() function
  v4l: tvp5150: Add missing break in set control handler
  v4l: tvp5150: Reset device at probe time, not in get/set format
    handlers
  v4l: tvp5150: Fix comment regarding output pin muxing
  v4l: tvp5150: Don't override output pinmuxing at stream on/off time

 drivers/media/i2c/tvp5150.c     | 63 +++++++++++++++++++++++++----------------
 drivers/media/i2c/tvp5150_reg.h |  9 ++++++
 2 files changed, 48 insertions(+), 24 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH v2 1/6] v4l: tvp5150: Compile tvp5150_link_setup out if !CONFIG_MEDIA_CONTROLLER

[Laurent Pinchart]

The function is only referenced as a handler in the tvp5150_sd_media_ops
structure, which is only used when CONFIG_MEDIA_CONTROLLER is set. Don't
define the function and the structure when the configuration option is
unset to avoid an unused function warning.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/tvp5150.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index 6737685d5be5..08384951c9e5 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -1013,11 +1013,11 @@ static int tvp5150_enum_frame_size(struct v4l2_subdev *sd,
 			Media entity ops
  ****************************************************************************/
 
+#ifdef CONFIG_MEDIA_CONTROLLER
 static int tvp5150_link_setup(struct media_entity *entity,
 			      const struct media_pad *local,
 			      const struct media_pad *remote, u32 flags)
 {
-#ifdef CONFIG_MEDIA_CONTROLLER
 	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
 	struct tvp5150 *decoder = to_tvp5150(sd);
 	int i;
@@ -1034,7 +1034,6 @@ static int tvp5150_link_setup(struct media_entity *entity,
 	decoder->input = i;
 
 	tvp5150_selmux(sd);
-#endif
 
 	return 0;
 }
@@ -1042,6 +1041,7 @@ static int tvp5150_link_setup(struct media_entity *entity,
 static const struct media_entity_operations tvp5150_sd_media_ops = {
 	.link_setup = tvp5150_link_setup,
 };
+#endif
 
 /****************************************************************************
 			I2C Command
-- 
Regards,

Laurent Pinchart

[PATCH v2 2/6] v4l: tvp5150: Don't inline the tvp5150_selmux() function

[Laurent Pinchart]

The function is large and called in several places, don't inline it.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/tvp5150.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index 08384951c9e5..febe6833a504 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -258,7 +258,7 @@ static int tvp5150_log_status(struct v4l2_subdev *sd)
 			Basic functions
  ****************************************************************************/
 
-static inline void tvp5150_selmux(struct v4l2_subdev *sd)
+static void tvp5150_selmux(struct v4l2_subdev *sd)
 {
 	int opmode = 0;
 	struct tvp5150 *decoder = to_tvp5150(sd);
-- 
Regards,

Laurent Pinchart

[PATCH v2 3/6] v4l: tvp5150: Add missing break in set control handler

[Laurent Pinchart]

A break is missing resulting in the hue control enabling or disabling
the decode completely. Fix it.

Fixes: c43875f66140 ("[media] tvp5150: replace MEDIA_ENT_F_CONN_TEST by a control")
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/tvp5150.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index febe6833a504..3a0fe8cc64e9 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -818,6 +818,7 @@ static int tvp5150_s_ctrl(struct v4l2_ctrl *ctrl)
 		return 0;
 	case V4L2_CID_HUE:
 		tvp5150_write(sd, TVP5150_HUE_CTL, ctrl->val);
+		break;
 	case V4L2_CID_TEST_PATTERN:
 		decoder->enable = ctrl->val ? false : true;
 		tvp5150_selmux(sd);
-- 
Regards,

Laurent Pinchart

[PATCH v2 4/6] v4l: tvp5150: Reset device at probe time, not in get/set format handlers

[Laurent Pinchart]

The tvp5150 doesn't support format setting through the subdev pad API
and thus implements the set format handler as a get format operation.
The single handler, tvp5150_fill_fmt(), resets the device by calling
tvp5150_reset(). This causes malfunction as the device can be reset at
will, possibly from userspace when the subdev userspace API is enabled.

The reset call was added in commit ec2c4f3f93cb ("[media] media:
tvp5150: Add mbus_fmt callbacks"), probably as an attempt to set the
device to a known state before detecting the current TV standard.
However, the get format handler doesn't access the hardware to get the
TV standard since commit 963ddc63e20d ("[media] media: tvp5150: Add
cropping support"). There is thus no need to reset the device when
getting the format.

However, removing the tvp5150_reset() from the get/set format handlers
results in the function not being called at all if the bridge driver
doesn't use the .reset() operation. The operation is nowadays abused and
shouldn't be used, so shouldn't expect bridge drivers to call it. To
make sure the device is properly initialize, move the reset call from
the format handlers to the probe function.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/tvp5150.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index 3a0fe8cc64e9..a30bfcb4eec6 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -861,8 +861,6 @@ 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 / 2;
 
@@ -1524,7 +1522,6 @@ static int tvp5150_probe(struct i2c_client *c,
 		res = core->hdl.error;
 		goto err;
 	}
-	v4l2_ctrl_handler_setup(&core->hdl);
 
 	/* Default is no cropping */
 	core->rect.top = 0;
@@ -1535,6 +1532,8 @@ static int tvp5150_probe(struct i2c_client *c,
 	core->rect.left = 0;
 	core->rect.width = TVP5150_H_MAX;
 
+	tvp5150_reset(sd, 0);	/* Calls v4l2_ctrl_handler_setup() */
+
 	res = v4l2_async_register_subdev(sd);
 	if (res < 0)
 		goto err;
-- 
Regards,

Laurent Pinchart

[PATCH v2 5/6] v4l: tvp5150: Fix comment regarding output pin muxing

[Laurent Pinchart]

The FID/GLCO/VLK/HVLK and INTREQ/GPCL/VBLK pins are muxed differently
depending on whether the input is an S-Video or composite signal. The
comment that explains the logic doesn't reflect the code. It appears
that the comment is incorrect, as disabling the output data bus in
composite mode makes no sense. Update the comment to match the code.

While at it define macros for the MISC_CTL register bits, the code is
too confusing with numerical values.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/tvp5150.c     | 24 +++++++++++++++++-------
 drivers/media/i2c/tvp5150_reg.h |  9 +++++++++
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index a30bfcb4eec6..8852fa8c957b 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -291,8 +291,12 @@ static void tvp5150_selmux(struct v4l2_subdev *sd)
 	tvp5150_write(sd, TVP5150_OP_MODE_CTL, opmode);
 	tvp5150_write(sd, TVP5150_VD_IN_SRC_SEL_1, input);
 
-	/* Svideo should enable YCrCb output and disable GPCL output
-	 * For Composite and TV, it should be the reverse
+	/*
+	 * Setup the FID/GLCO/VLK/HVLK and INTREQ/GPCL/VBLK output signals. For
+	 * S-Video we output the vertical lock (VLK) signal on FID/GLCO/VLK/HVLK
+	 * and set INTREQ/GPCL/VBLK to logic 0. For composite we output the
+	 * field indicator (FID) signal on FID/GLCO/VLK/HVLK and set
+	 * INTREQ/GPCL/VBLK to logic 1.
 	 */
 	val = tvp5150_read(sd, TVP5150_MISC_CTL);
 	if (val < 0) {
@@ -301,9 +305,9 @@ static void tvp5150_selmux(struct v4l2_subdev *sd)
 	}
 
 	if (decoder->input == TVP5150_SVIDEO)
-		val = (val & ~0x40) | 0x10;
+		val = (val & ~TVP5150_MISC_CTL_GPCL) | TVP5150_MISC_CTL_HVLK;
 	else
-		val = (val & ~0x10) | 0x40;
+		val = (val & ~TVP5150_MISC_CTL_HVLK) | TVP5150_MISC_CTL_GPCL;
 	tvp5150_write(sd, TVP5150_MISC_CTL, val);
 };
 
@@ -455,7 +459,12 @@ static const struct i2c_reg_value tvp5150_init_enable[] = {
 	},{	/* Automatic offset and AGC enabled */
 		TVP5150_ANAL_CHL_CTL, 0x15
 	},{	/* Activate YCrCb output 0x9 or 0xd ? */
-		TVP5150_MISC_CTL, 0x6f
+		TVP5150_MISC_CTL, TVP5150_MISC_CTL_GPCL |
+				  TVP5150_MISC_CTL_INTREQ_OE |
+				  TVP5150_MISC_CTL_YCBCR_OE |
+				  TVP5150_MISC_CTL_SYNC_OE |
+				  TVP5150_MISC_CTL_VBLANK |
+				  TVP5150_MISC_CTL_CLOCK_OE,
 	},{	/* Activates video std autodetection for all standards */
 		TVP5150_AUTOSW_MSK, 0x0
 	},{	/* Default format: 0x47. For 4:2:2: 0x40 */
@@ -1050,11 +1059,12 @@ static int tvp5150_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct tvp5150 *decoder = to_tvp5150(sd);
 	/* Output format: 8-bit ITU-R BT.656 with embedded syncs */
-	int val = 0x09;
+	int val = TVP5150_MISC_CTL_YCBCR_OE | TVP5150_MISC_CTL_CLOCK_OE;
 
 	/* Output format: 8-bit 4:2:2 YUV with discrete sync */
 	if (decoder->mbus_type == V4L2_MBUS_PARALLEL)
-		val = 0x0d;
+		val = TVP5150_MISC_CTL_YCBCR_OE | TVP5150_MISC_CTL_SYNC_OE
+		    | TVP5150_MISC_CTL_CLOCK_OE;
 
 	/* Initializes TVP5150 to its default values */
 	/* # set PCLK (27MHz) */
diff --git a/drivers/media/i2c/tvp5150_reg.h b/drivers/media/i2c/tvp5150_reg.h
index 25a994944918..30a48c28d05a 100644
--- a/drivers/media/i2c/tvp5150_reg.h
+++ b/drivers/media/i2c/tvp5150_reg.h
@@ -9,6 +9,15 @@
 #define TVP5150_ANAL_CHL_CTL         0x01 /* Analog channel controls */
 #define TVP5150_OP_MODE_CTL          0x02 /* Operation mode controls */
 #define TVP5150_MISC_CTL             0x03 /* Miscellaneous controls */
+#define TVP5150_MISC_CTL_VBLK_GPCL	BIT(7)
+#define TVP5150_MISC_CTL_GPCL		BIT(6)
+#define TVP5150_MISC_CTL_INTREQ_OE	BIT(5)
+#define TVP5150_MISC_CTL_HVLK		BIT(4)
+#define TVP5150_MISC_CTL_YCBCR_OE	BIT(3)
+#define TVP5150_MISC_CTL_SYNC_OE	BIT(2)
+#define TVP5150_MISC_CTL_VBLANK		BIT(1)
+#define TVP5150_MISC_CTL_CLOCK_OE	BIT(0)
+
 #define TVP5150_AUTOSW_MSK           0x04 /* Autoswitch mask: TVP5150A / TVP5150AM */
 
 /* Reserved 05h */
-- 
Regards,

Laurent Pinchart

[PATCH v2 6/6] v4l: tvp5150: Don't override output pinmuxing at stream on/off time

[Laurent Pinchart]

The s_stream() handler incorrectly writes the whole MISC_CTL register to
enable or disable the outputs, overriding the output pinmuxing
configuration. Fix it to only touch the output enable bits.

The CONF_SHARED_PIN register is also written by the same function,
resulting in muxing the INTREQ signal instead of the VBLK/GPCL signal on
the INTREQ/GPCL/VBLK pin. As the driver doesn't support interrupts this
is obviously incorrect, and breaks operation on other devices. Fix it by
removing the write.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/tvp5150.c | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index 8852fa8c957b..48646a7f3fb0 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -1058,22 +1058,27 @@ static const struct media_entity_operations tvp5150_sd_media_ops = {
 static int tvp5150_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct tvp5150 *decoder = to_tvp5150(sd);
-	/* Output format: 8-bit ITU-R BT.656 with embedded syncs */
-	int val = TVP5150_MISC_CTL_YCBCR_OE | TVP5150_MISC_CTL_CLOCK_OE;
-
-	/* Output format: 8-bit 4:2:2 YUV with discrete sync */
-	if (decoder->mbus_type == V4L2_MBUS_PARALLEL)
-		val = TVP5150_MISC_CTL_YCBCR_OE | TVP5150_MISC_CTL_SYNC_OE
-		    | TVP5150_MISC_CTL_CLOCK_OE;
+	int val;
 
-	/* Initializes TVP5150 to its default values */
-	/* # set PCLK (27MHz) */
-	tvp5150_write(sd, TVP5150_CONF_SHARED_PIN, 0x00);
+	/* Enable or disable the video output signals. */
+	val = tvp5150_read(sd, TVP5150_MISC_CTL);
+	if (val < 0)
+		return val;
+
+	val &= ~(TVP5150_MISC_CTL_YCBCR_OE | TVP5150_MISC_CTL_SYNC_OE |
+		 TVP5150_MISC_CTL_CLOCK_OE);
+
+	if (enable) {
+		/*
+		 * Enable the YCbCr and clock outputs. In discrete sync mode
+		 * (non-BT.656) additionally enable the the sync outputs.
+		 */
+		val |= TVP5150_MISC_CTL_YCBCR_OE | TVP5150_MISC_CTL_CLOCK_OE;
+		if (decoder->mbus_type == V4L2_MBUS_PARALLEL)
+			val |= TVP5150_MISC_CTL_SYNC_OE;
+	}
 
-	if (enable)
-		tvp5150_write(sd, TVP5150_MISC_CTL, val);
-	else
-		tvp5150_write(sd, TVP5150_MISC_CTL, 0x00);
+	tvp5150_write(sd, TVP5150_MISC_CTL, val);
 
 	return 0;
 }
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 0/6] Fix tvp5150 regression with em28xx

[Mauro Carvalho Chehab]

Hi Laurent,

Em Fri,  9 Dec 2016 13:47:13 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hello,
> 
> This patch series fixes a regression reported by Devin Heitmueller that
> affects a large number of em28xx. The problem was introduced by
> 
> commit 13d52fe40f1f7bbad49128e8ee6a2fe5e13dd18d
> Author: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> Date:   Tue Jan 26 06:59:39 2016 -0200
> 
>     [media] em28xx: fix implementation of s_stream
> 
> that started calling s_stream(1) in the em28xx driver when enabling the
> stream, resulting in the tvp5150 s_stream() operation writing several
> registers with values fit for other platforms (namely OMAP3, possibly others)
> but not for em28xx.
> 
> The series starts with two unrelated drive-by cleanups and an unrelated bug
> fix. It then continues with a patch to remove an unneeded and armful call to
> tvp5150_reset() when getting the format from the subdevice (4/6), an update of
> an invalid comment and the addition of macros for register bits in order to
> make the code more readable (5/6) and actually allow following the incorrect
> code flow, and finally a rework of the s_stream() operation to fix the
> problem.
> 
> Compared to v1,
> 
> - Patch 4/5 now calls tvp5150_reset() at probe time
> - Patch 5/6 is fixed with an extra ~ removed
> 
> I haven't been able to test this with an em28xx device as I don't own any that
> contains a tvp5150, but Mauro reported that the series fixes the issue with
> his device.
> 
> I also haven't been able to test anything on an OMAP3 platform, as the tvp5150
> driver go broken on DT-based systems by

I applied today patches 1 to 3, as I don't see any risk of regressions
there. Stable was c/c on patch 3.

I want to do more tests on patches 4-6, with both progressive video and RF. 
It would also be nice if someone could test it on OMAP3, to be sure that no
regressions happened there.

So, my goal is to send those patches for -rc2, assuming that we can do
such tests, as it is likely too late for -rc1, as we'll have a short merge
window this time.

Regards,
Mauro

> commit f7b4b54e63643b740c598e044874c4bffa0f04f2
> Author: Javier Martinez Canillas <javier@osg.samsung.com>
> Date:   Fri Feb 5 17:09:58 2016 -0200
> 
>     [media] tvp5150: add HW input connectors support
> 
> Fixing it will be the topic of another patch series.
> 
> Laurent Pinchart (6):
>   v4l: tvp5150: Compile tvp5150_link_setup out if
>     !CONFIG_MEDIA_CONTROLLER
>   v4l: tvp5150: Don't inline the tvp5150_selmux() function
>   v4l: tvp5150: Add missing break in set control handler
>   v4l: tvp5150: Reset device at probe time, not in get/set format
>     handlers
>   v4l: tvp5150: Fix comment regarding output pin muxing
>   v4l: tvp5150: Don't override output pinmuxing at stream on/off time
> 
>  drivers/media/i2c/tvp5150.c     | 63 +++++++++++++++++++++++++----------------
>  drivers/media/i2c/tvp5150_reg.h |  9 ++++++
>  2 files changed, 48 insertions(+), 24 deletions(-)
> 



Thanks,
Mauro

Re: [PATCH v2 0/6] Fix tvp5150 regression with em28xx

[Javier Martinez Canillas]

Hello Mauro,

On 12/12/2016 06:51 AM, Mauro Carvalho Chehab wrote:
> Hi Laurent,
> 
> Em Fri,  9 Dec 2016 13:47:13 +0200
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:
> 
>> Hello,
>>
>> This patch series fixes a regression reported by Devin Heitmueller that
>> affects a large number of em28xx. The problem was introduced by
>>
>> commit 13d52fe40f1f7bbad49128e8ee6a2fe5e13dd18d
>> Author: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
>> Date:   Tue Jan 26 06:59:39 2016 -0200
>>
>>     [media] em28xx: fix implementation of s_stream
>>
>> that started calling s_stream(1) in the em28xx driver when enabling the
>> stream, resulting in the tvp5150 s_stream() operation writing several
>> registers with values fit for other platforms (namely OMAP3, possibly others)
>> but not for em28xx.
>>
>> The series starts with two unrelated drive-by cleanups and an unrelated bug
>> fix. It then continues with a patch to remove an unneeded and armful call to
>> tvp5150_reset() when getting the format from the subdevice (4/6), an update of
>> an invalid comment and the addition of macros for register bits in order to
>> make the code more readable (5/6) and actually allow following the incorrect
>> code flow, and finally a rework of the s_stream() operation to fix the
>> problem.
>>
>> Compared to v1,
>>
>> - Patch 4/5 now calls tvp5150_reset() at probe time
>> - Patch 5/6 is fixed with an extra ~ removed
>>
>> I haven't been able to test this with an em28xx device as I don't own any that
>> contains a tvp5150, but Mauro reported that the series fixes the issue with
>> his device.
>>
>> I also haven't been able to test anything on an OMAP3 platform, as the tvp5150
>> driver go broken on DT-based systems by
> 
> I applied today patches 1 to 3, as I don't see any risk of regressions
> there. Stable was c/c on patch 3.
> 
> I want to do more tests on patches 4-6, with both progressive video and RF. 
> It would also be nice if someone could test it on OMAP3, to be sure that no
> regressions happened there.
>

I've tested patches 4-6 on a IGEPv2 and video capture is still working for both
composite input AIP1A (after changing the hardcoded selected input) and AIP1B.

The patches also look good to me, so please feel free to add my Reviewed-by and
Tested-by tags on these.

I wasn't able to test S-Video since my S-Video source broke (an old DVD player)
but this never worked for me anyways with this board.

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

Re: [PATCH v2 0/6] Fix tvp5150 regression with em28xx

[Laurent Pinchart]

Hello,

On Monday 12 Dec 2016 13:22:50 Javier Martinez Canillas wrote:
> On 12/12/2016 06:51 AM, Mauro Carvalho Chehab wrote:
> > Em Fri,  9 Dec 2016 13:47:13 +0200 Laurent Pinchart escreveu:
> >> Hello,
> >> 
> >> This patch series fixes a regression reported by Devin Heitmueller that
> >> affects a large number of em28xx. The problem was introduced by
> >> 
> >> commit 13d52fe40f1f7bbad49128e8ee6a2fe5e13dd18d
> >> Author: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> >> Date:   Tue Jan 26 06:59:39 2016 -0200
> >> 
> >>     [media] em28xx: fix implementation of s_stream
> >> 
> >> that started calling s_stream(1) in the em28xx driver when enabling the
> >> stream, resulting in the tvp5150 s_stream() operation writing several
> >> registers with values fit for other platforms (namely OMAP3, possibly
> >> others) but not for em28xx.
> >> 
> >> The series starts with two unrelated drive-by cleanups and an unrelated
> >> bug fix. It then continues with a patch to remove an unneeded and armful
> >> call to tvp5150_reset() when getting the format from the subdevice (4/6),
> >> an update of an invalid comment and the addition of macros for register
> >> bits in order to make the code more readable (5/6) and actually allow
> >> following the incorrect code flow, and finally a rework of the
> >> s_stream() operation to fix the problem.
> >> 
> >> Compared to v1,
> >> 
> >> - Patch 4/5 now calls tvp5150_reset() at probe time
> >> - Patch 5/6 is fixed with an extra ~ removed
> >> 
> >> I haven't been able to test this with an em28xx device as I don't own any
> >> that contains a tvp5150, but Mauro reported that the series fixes the
> >> issue with his device.
> >> 
> >> I also haven't been able to test anything on an OMAP3 platform, as the
> >> tvp5150 driver go broken on DT-based systems by
> > 
> > I applied today patches 1 to 3, as I don't see any risk of regressions
> > there. Stable was c/c on patch 3.
> > 
> > I want to do more tests on patches 4-6, with both progressive video and
> > RF. It would also be nice if someone could test it on OMAP3, to be sure
> > that no regressions happened there.
> 
> I've tested patches 4-6 on a IGEPv2 and video capture is still working for
> both composite input AIP1A (after changing the hardcoded selected input)
> and AIP1B.
> 
> The patches also look good to me, so please feel free to add my Reviewed-by
> and Tested-by tags on these.
> 
> I wasn't able to test S-Video since my S-Video source broke (an old DVD
> player) but this never worked for me anyways with this board.

I've tested the patches too, in composite mode only as my hardware has a 
single input. The image quality isn't very good, but I believe that's due to 
my source. It shouldn't be related to this patch series at least.

I tried both BT.656 and parallel bus modes. The latter didn't work properly, 
but it wasn't supported when I worked on TVP5151 + OMAP3 support in the first 
place anyway, so it's not a regression, just something to eventually fix (if I 
have too much free time).

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 0/6] Fix tvp5150 regression with em28xx

[Mauro Carvalho Chehab]

Em Mon, 12 Dec 2016 18:37:01 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hello,
> 
> On Monday 12 Dec 2016 13:22:50 Javier Martinez Canillas wrote:
> > On 12/12/2016 06:51 AM, Mauro Carvalho Chehab wrote:  
> > > Em Fri,  9 Dec 2016 13:47:13 +0200 Laurent Pinchart escreveu:  
> > >> Hello,
> > >> 
> > >> This patch series fixes a regression reported by Devin Heitmueller that
> > >> affects a large number of em28xx. The problem was introduced by
> > >> 
> > >> commit 13d52fe40f1f7bbad49128e8ee6a2fe5e13dd18d
> > >> Author: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> > >> Date:   Tue Jan 26 06:59:39 2016 -0200
> > >> 
> > >>     [media] em28xx: fix implementation of s_stream
> > >> 
> > >> that started calling s_stream(1) in the em28xx driver when enabling the
> > >> stream, resulting in the tvp5150 s_stream() operation writing several
> > >> registers with values fit for other platforms (namely OMAP3, possibly
> > >> others) but not for em28xx.
> > >> 
> > >> The series starts with two unrelated drive-by cleanups and an unrelated
> > >> bug fix. It then continues with a patch to remove an unneeded and armful
> > >> call to tvp5150_reset() when getting the format from the subdevice (4/6),
> > >> an update of an invalid comment and the addition of macros for register
> > >> bits in order to make the code more readable (5/6) and actually allow
> > >> following the incorrect code flow, and finally a rework of the
> > >> s_stream() operation to fix the problem.
> > >> 
> > >> Compared to v1,
> > >> 
> > >> - Patch 4/5 now calls tvp5150_reset() at probe time
> > >> - Patch 5/6 is fixed with an extra ~ removed
> > >> 
> > >> I haven't been able to test this with an em28xx device as I don't own any
> > >> that contains a tvp5150, but Mauro reported that the series fixes the
> > >> issue with his device.
> > >> 
> > >> I also haven't been able to test anything on an OMAP3 platform, as the
> > >> tvp5150 driver go broken on DT-based systems by  
> > > 
> > > I applied today patches 1 to 3, as I don't see any risk of regressions
> > > there. Stable was c/c on patch 3.
> > > 
> > > I want to do more tests on patches 4-6, with both progressive video and
> > > RF. It would also be nice if someone could test it on OMAP3, to be sure
> > > that no regressions happened there.  
> > 
> > I've tested patches 4-6 on a IGEPv2 and video capture is still working for
> > both composite input AIP1A (after changing the hardcoded selected input)
> > and AIP1B.
> > 
> > The patches also look good to me, so please feel free to add my Reviewed-by
> > and Tested-by tags on these.
> > 
> > I wasn't able to test S-Video since my S-Video source broke (an old DVD
> > player) but this never worked for me anyways with this board.  
> 
> I've tested the patches too, in composite mode only as my hardware has a 
> single input. The image quality isn't very good, but I believe that's due to 
> my source. It shouldn't be related to this patch series at least.

Yesterday, I was able to make my device that generates 480p to work again,
and bought a RF modulator.

I used HVR-350 and Hauppauge MediaMVP as image sources producing NTSC output,
and Kernel 4.9 + media patches for 4.10 + tvp5150 v2 patch series.

With that, I completed the tests on HVR-950. My tests covered:
- S-Video, Composite, TV
- 480i and 480p
- Closed Captions (with HVR-350 - it seems that MediaMVP doesn't
  produce NTSC CC).

PS.: I did some tests with PAL output too, with HVR-350.

> I tried both BT.656 and parallel bus modes. The latter didn't work properly, 
> but it wasn't supported when I worked on TVP5151 + OMAP3 support in the first 
> place anyway, so it's not a regression, just something to eventually fix (if I 
> have too much free time).

With that, it seems that BT.656 is working fine. So, I'm merging the
patches and will send them on the next pull request.

Thanks,
Mauro

Re: [PATCH v2 0/6] Fix tvp5150 regression with em28xx

[Devin Heitmueller]

Hi Mauro,

> With that, I completed the tests on HVR-950. My tests covered:
> - S-Video, Composite, TV
> - 480i and 480p
> - Closed Captions (with HVR-350 - it seems that MediaMVP doesn't
>   produce NTSC CC).

FYI:  the MediaMVP HD can be configured to output NTSC CC over VBI.
If you want that functionality, I can dig out the script.  In fact
I've got an alternate GUI which just plays a clip on boot and lets you
select all the different resolutions/framerates available for
composte/component/HDMI (for both PAL and NTSC) just by hitting
buttons on the remote.  If you're interested, let me know and I'll dig
it up.  It's a great tool to have, especially when doing work with
HDMI where there are many more possible combinations to choose from.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH v2 0/6] Fix tvp5150 regression with em28xx

[Mauro Carvalho Chehab]

Em Wed, 21 Dec 2016 09:11:36 -0500
Hi Devin,

Devin Heitmueller <dheitmueller@kernellabs.com> escreveu:

> Hi Mauro,
> 
> > With that, I completed the tests on HVR-950. My tests covered:
> > - S-Video, Composite, TV
> > - 480i and 480p
> > - Closed Captions (with HVR-350 - it seems that MediaMVP doesn't
> >   produce NTSC CC).  
> 
> FYI:  the MediaMVP HD can be configured to output NTSC CC over VBI.
> If you want that functionality, I can dig out the script.  In fact
> I've got an alternate GUI which just plays a clip on boot and lets you
> select all the different resolutions/framerates available for
> composte/component/HDMI (for both PAL and NTSC) just by hitting
> buttons on the remote.  If you're interested, let me know and I'll dig
> it up.  It's a great tool to have, especially when doing work with
> HDMI where there are many more possible combinations to choose from.

Yeah, both things are interesting! My plan is to use MediaMVP as a
test device for analog TV, as it provides a way better output than
HVR-350 and it is easier to use.


Thanks,
Mauro