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

2016-12-08 Thread 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

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

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

2016-12-08 Thread Mauro Carvalho Chehab 
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.

Thanks,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-12-08 Thread 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 
> Cc: Javier Martinez Canillas 
> Cc: Laurent Pinchart 
> Cc: sta...@vger.kernel.org
> Signed-off-by: Mauro Carvalho Chehab 
> ---
> 
> 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

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

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

2016-12-08 Thread kbuild test robot 
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 infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

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

2016-12-08 Thread Mauro Carvalho Chehab 
Em Thu, 8 Dec 2016 17:51:02 -0200
Mauro Carvalho Chehab  escreveu:

> Em Thu,  8 Dec 2016 17:46:53 -0200
> Mauro Carvalho Chehab  escreveu:
> 
> > 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 
> > Cc: Javier Martinez Canillas 
> > Cc: Laurent Pinchart 
> > Cc: sta...@vger.kernel.org
> > Signed-off-by: Mauro Carvalho Chehab 
> > ---
> > 
> > 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  
> 
> Tested with HVR-950 (USB ID 2040:6513, Hauppauge model 65201, rev A1C0):
>   - both S-Video and composite entries are working.

Devin,

Btw, if you're willing to test it against the latest Kernel, I recommend
you to also apply the three em28xx patches I just sent upstream, as they
fix a regression with the conversion to dev_foo() print on em28xx driver,
reported by Antti, with happens when removing the em28xx driver from memory.

Such regression happened only at the 4.9-rc development cycle, so it 
shouldn't affect any earlier versions of em28xx.

I'm placing all 4 patches under this branch:
https://git.linuxtv.org/mchehab/experimental.git/log/?h=em28xx-fixes

Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-12-08 Thread Mauro Carvalho Chehab 
Em Thu,  8 Dec 2016 17:46:53 -0200
Mauro Carvalho Chehab  escreveu:

> 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 
> Cc: Javier Martinez Canillas 
> Cc: Laurent Pinchart 
> Cc: sta...@vger.kernel.org
> Signed-off-by: Mauro Carvalho Chehab 
> ---
> 
> 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

Tested with HVR-950 (USB ID 2040:6513, Hauppauge model 65201, rev A1C0):
- both S-Video and composite entries are working.

Thanks,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html