[PATCH v3] [media] tvp5150: don't touch register TVP5150_CONF_SHARED_PIN if not needed

[PATCH v3] [media] tvp5150: don't touch register TVP5150_CONF_SHARED_PIN if not needed

[Mauro Carvalho Chehab]

commit 460b6c0831cb ("[media] tvp5150: Add s_stream subdev operation
support") added a logic that overrides TVP5150_CONF_SHARED_PIN setting,
depending on the type of bus set via the .set_fmt() subdev callback.

This is known to cause trobules on devices that don't use a V4L2
subdev devnode, and a fix for it was made by commit 47de9bf8931e
("[media] tvp5150: Fix breakage for serial usage"). Unfortunately,
such fix doesn't consider the case of progressive video inputs,
causing chroma decoding issues on such videos, as it overrides not
only the type of video output, but also other unrelated bits.

So, instead of trying to guess, let's detect if the device configuration
is set via Device Tree. If not, just ignore the new logic, restoring
the original behavior.

Fixes: 460b6c0831cb ("[media] tvp5150: Add s_stream subdev operation support")
Cc: Devin Heitmueller <dheitmueller@kernellabs.com>
Cc: Javier Martinez Canillas <javier@osg.samsung.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: stable@vger.kernel.org
Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---

changes since version 2: 
  - fixed settings for register 0x0d
  - tested on WinTV USB2 with S-Video input

I'll do an extra test with HVR-950 on both S-Video and composite soon enough

changes since version 1: added a notice about what's broken at the
tvp5150_stream() logic, and improved patch's description.

changes since RFC: don't touch at enum v4l2_mbus_type.

 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 6737685d5be5..8b9d2fad17df 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -57,6 +57,7 @@ struct tvp5150 {
 	u16 rom_ver;
 
 	enum v4l2_mbus_type mbus_type;
+	bool has_dt;
 };
 
 static inline struct tvp5150 *to_tvp5150(struct v4l2_subdev *sd)
@@ -1053,6 +1054,20 @@ static int tvp5150_s_stream(struct v4l2_subdev *sd, int enable)
 	/* Output format: 8-bit ITU-R BT.656 with embedded syncs */
 	int val = 0x09;
 
+	if (!decoder->has_dt)
+		return 0;
+
+	/*
+	 * FIXME: the logic below is hardcoded to work with some OMAP3
+	 * hardware with tvp5151. As such, it hardcodes values for
+	 * both TVP5150_CONF_SHARED_PIN and TVP5150_MISC_CTL, and ignores
+	 * what was set before at the driver. Ideally, we should have
+	 * DT nodes describing the setup, instead of hardcoding those
+	 * values, and doing a read before writing values to
+	 * TVP5150_MISC_CTL, but any patch adding support for it should
+	 * keep DT backward-compatible.
+	 */
+
 	/* Output format: 8-bit 4:2:2 YUV with discrete sync */
 	if (decoder->mbus_type == V4L2_MBUS_PARALLEL)
 		val = 0x0d;
@@ -1471,6 +1486,7 @@ static int tvp5150_probe(struct i2c_client *c,
 			dev_err(sd->dev, "DT parsing error: %d\n", res);
 			return res;
 		}
+		decoder->has_dt = true;
 	} else {
 		/* Default to BT.656 embedded sync */
 		core->mbus_type = V4L2_MBUS_BT656;
-- 
2.9.3

Re: [PATCH v3] [media] tvp5150: don't touch register TVP5150_CONF_SHARED_PIN if not needed

[kbuild test robot]

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

Hi Mauro,

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

url:    https://github.com/0day-ci/linux/commits/Mauro-Carvalho-Chehab/tvp5150-don-t-touch-register-TVP5150_CONF_SHARED_PIN-if-not-needed/20161209-040023
base:   git://linuxtv.org/media_tree.git master
config: x86_64-randconfig-x005-201649 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
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_probe':
>> drivers/media/i2c/tvp5150.c:1489:3: error: 'decoder' undeclared (first use in this function)
      decoder->has_dt = true;
      ^~~~~~~
   drivers/media/i2c/tvp5150.c:1489:3: note: each undeclared identifier is reported only once for each function it appears in

vim +/decoder +1489 drivers/media/i2c/tvp5150.c

  1483		if (IS_ENABLED(CONFIG_OF) && np) {
  1484			res = tvp5150_parse_dt(core, np);
  1485			if (res) {
  1486				dev_err(sd->dev, "DT parsing error: %d\n", res);
  1487				return res;
  1488			}
> 1489			decoder->has_dt = true;
  1490		} else {
  1491			/* Default to BT.656 embedded sync */
  1492			core->mbus_type = V4L2_MBUS_BT656;

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28576 bytes --]

Re: [PATCH v3] [media] tvp5150: don't touch register TVP5150_CONF_SHARED_PIN if not needed

[Laurent Pinchart]

Hi Mauro,

I've just sent a series of patches ("[PATCH 0/6] Fix tvp5150 regression with 
em28xx") that should fix this problem properly. I unfortunately haven't been 
able to test it with an em28xx device as I don't own any.

On Thursday 08 Dec 2016 17:46:53 Mauro Carvalho Chehab wrote:
> commit 460b6c0831cb ("[media] tvp5150: Add s_stream subdev operation
> support") added a logic that overrides TVP5150_CONF_SHARED_PIN setting,
> depending on the type of bus set via the .set_fmt() subdev callback.
> 
> This is known to cause trobules on devices that don't use a V4L2
> subdev devnode, and a fix for it was made by commit 47de9bf8931e
> ("[media] tvp5150: Fix breakage for serial usage"). Unfortunately,
> such fix doesn't consider the case of progressive video inputs,
> causing chroma decoding issues on such videos, as it overrides not
> only the type of video output, but also other unrelated bits.
> 
> So, instead of trying to guess, let's detect if the device configuration
> is set via Device Tree. If not, just ignore the new logic, restoring
> the original behavior.
> 
> Fixes: 460b6c0831cb ("[media] tvp5150: Add s_stream subdev operation
> support") Cc: Devin Heitmueller <dheitmueller@kernellabs.com>
> Cc: Javier Martinez Canillas <javier@osg.samsung.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> ---
> 
> changes since version 2:
>   - fixed settings for register 0x0d
>   - tested on WinTV USB2 with S-Video input
> 
> I'll do an extra test with HVR-950 on both S-Video and composite soon enough
> 
> changes since version 1: added a notice about what's broken at the
> tvp5150_stream() logic, and improved patch's description.
> 
> changes since RFC: don't touch at enum v4l2_mbus_type.
> 
>  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 6737685d5be5..8b9d2fad17df 100644
> --- a/drivers/media/i2c/tvp5150.c
> +++ b/drivers/media/i2c/tvp5150.c
> @@ -57,6 +57,7 @@ struct tvp5150 {
>  	u16 rom_ver;
> 
>  	enum v4l2_mbus_type mbus_type;
> +	bool has_dt;
>  };
> 
>  static inline struct tvp5150 *to_tvp5150(struct v4l2_subdev *sd)
> @@ -1053,6 +1054,20 @@ static int tvp5150_s_stream(struct v4l2_subdev *sd,
> int enable) /* Output format: 8-bit ITU-R BT.656 with embedded syncs */
>  	int val = 0x09;
> 
> +	if (!decoder->has_dt)
> +		return 0;
> +
> +	/*
> +	 * FIXME: the logic below is hardcoded to work with some OMAP3
> +	 * hardware with tvp5151. As such, it hardcodes values for
> +	 * both TVP5150_CONF_SHARED_PIN and TVP5150_MISC_CTL, and ignores
> +	 * what was set before at the driver. Ideally, we should have
> +	 * DT nodes describing the setup, instead of hardcoding those
> +	 * values, and doing a read before writing values to
> +	 * TVP5150_MISC_CTL, but any patch adding support for it should
> +	 * keep DT backward-compatible.
> +	 */
> +
>  	/* Output format: 8-bit 4:2:2 YUV with discrete sync */
>  	if (decoder->mbus_type == V4L2_MBUS_PARALLEL)
>  		val = 0x0d;
> @@ -1471,6 +1486,7 @@ static int tvp5150_probe(struct i2c_client *c,
>  			dev_err(sd->dev, "DT parsing error: %d\n", res);
>  			return res;
>  		}
> +		decoder->has_dt = true;
>  	} else {
>  		/* Default to BT.656 embedded sync */
>  		core->mbus_type = V4L2_MBUS_BT656;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3] [media] tvp5150: don't touch register TVP5150_CONF_SHARED_PIN if not needed

[Mauro Carvalho Chehab]

Em Fri, 09 Dec 2016 00:33:22 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Mauro,
> 
> I've just sent a series of patches ("[PATCH 0/6] Fix tvp5150 regression with 
> em28xx") that should fix this problem properly. I unfortunately haven't been 
> able to test it with an em28xx device as I don't own any.

I'll try to test it tomorrow, with interlaced video. I guess I can
test also VBI, but I need to double-check. I'm currently missing some
way to test progressive video, though.

Thanks,
Mauro

Re: [PATCH v3] [media] tvp5150: don't touch register TVP5150_CONF_SHARED_PIN if not needed

[Laurent Pinchart]

Hi Mauro,

On Thursday 08 Dec 2016 21:16:07 Mauro Carvalho Chehab wrote:
> Em Fri, 09 Dec 2016 00:33:22 +0200 Laurent Pinchart escreveu:
> > Hi Mauro,
> > 
> > I've just sent a series of patches ("[PATCH 0/6] Fix tvp5150 regression
> > with em28xx") that should fix this problem properly. I unfortunately
> > haven't been able to test it with an em28xx device as I don't own any.
> 
> I'll try to test it tomorrow, with interlaced video. I guess I can
> test also VBI, but I need to double-check. I'm currently missing some
> way to test progressive video, though.

Thank you. I dug up an em28xx device I got years ago (had nearly forgotten 
about it) but it doesn't have a tvp5150, so I confirm I can't test this 
myself.

-- 
Regards,

Laurent Pinchart