[PATCH 0/6] Fix tvp5150 regression with em28xx

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

I haven't been able to test this with an em28xx device as I don't own any. I
would appreciate if someone could give the series a go.

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: Don't reset device 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     | 60 +++++++++++++++++++++++++----------------
 drivers/media/i2c/tvp5150_reg.h |  9 +++++++
 2 files changed, 46 insertions(+), 23 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH 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 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 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 4/6] v4l: tvp5150: Don't reset device 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. Fix it.

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

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index 3a0fe8cc64e9..19b79028cac5 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;
 
-- 
Regards,

Laurent Pinchart

[PATCH 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 19b79028cac5..18260849fe5a 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 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 18260849fe5a..9f1104b7c2cd 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 0/6] Fix tvp5150 regression with em28xx

[Mauro Carvalho Chehab]

Em Fri,  9 Dec 2016 00:22:40 +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.
> 
> I haven't been able to test this with an em28xx device as I don't own any. I
> would appreciate if someone could give the series a go.
> 
> 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: Don't reset device 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

Tested here with an interlaced video input on S-Video and/or Composite
entries of HVR-950:

It broke Composite selection. After this series, only S-Video input works.

VBI was not affected.

Regards,
Mauro