* [PATCH] media: i2c: tvp5150: fix color burst lock instability on some hardware @ 2018-03-25 22:56 Nasser Afshin 2018-03-26 9:43 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 7+ messages in thread From: Nasser Afshin @ 2018-03-25 22:56 UTC (permalink / raw) To: mchehab Cc: p.zabel, sakari.ailus, hans.verkuil, bparrot, garsilva, linux-media, linux-kernel, Nasser Afshin According to the datasheet, INTREQ/GPCL/VBLK should have a pull-up/down resistor if it's been disabled. On hardware that does not have such resistor, we should use the default output enable value. This prevents the color burst lock instability problem. Signed-off-by: Nasser Afshin <Afshin.Nasser@gmail.com> --- drivers/media/i2c/tvp5150.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c index 2476d812f669..0e9713814816 100644 --- a/drivers/media/i2c/tvp5150.c +++ b/drivers/media/i2c/tvp5150.c @@ -328,7 +328,7 @@ static const struct i2c_reg_value tvp5150_init_default[] = { TVP5150_OP_MODE_CTL,0x00 }, { /* 0x03 */ - TVP5150_MISC_CTL,0x01 + TVP5150_MISC_CTL,0x21 }, { /* 0x06 */ TVP5150_COLOR_KIL_THSH_CTL,0x10 @@ -1072,7 +1072,8 @@ static int tvp5150_s_stream(struct v4l2_subdev *sd, int 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; + val |= TVP5150_MISC_CTL_YCBCR_OE | TVP5150_MISC_CTL_CLOCK_OE | + TVP5150_MISC_CTL_INTREQ_OE; if (decoder->mbus_type == V4L2_MBUS_PARALLEL) val |= TVP5150_MISC_CTL_SYNC_OE; } -- 2.15.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] media: i2c: tvp5150: fix color burst lock instability on some hardware 2018-03-25 22:56 [PATCH] media: i2c: tvp5150: fix color burst lock instability on some hardware Nasser Afshin @ 2018-03-26 9:43 ` Mauro Carvalho Chehab 2018-03-26 22:29 ` Nasser 0 siblings, 1 reply; 7+ messages in thread From: Mauro Carvalho Chehab @ 2018-03-26 9:43 UTC (permalink / raw) To: Nasser Afshin Cc: p.zabel, sakari.ailus, hans.verkuil, bparrot, garsilva, linux-media, linux-kernel Hi Nasser, Em Mon, 26 Mar 2018 03:26:33 +0430 Nasser Afshin <afshin.nasser@gmail.com> escreveu: > According to the datasheet, INTREQ/GPCL/VBLK should have a pull-up/down > resistor if it's been disabled. On hardware that does not have such > resistor, we should use the default output enable value. > This prevents the color burst lock instability problem. If this is hardware-dependent, you should instead store it at OF (for SoC) or pass via platform_data (for PCI/USB devices). > > Signed-off-by: Nasser Afshin <Afshin.Nasser@gmail.com> > --- > drivers/media/i2c/tvp5150.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c > index 2476d812f669..0e9713814816 100644 > --- a/drivers/media/i2c/tvp5150.c > +++ b/drivers/media/i2c/tvp5150.c > @@ -328,7 +328,7 @@ static const struct i2c_reg_value tvp5150_init_default[] = { > TVP5150_OP_MODE_CTL,0x00 > }, > { /* 0x03 */ > - TVP5150_MISC_CTL,0x01 > + TVP5150_MISC_CTL,0x21 > }, > { /* 0x06 */ > TVP5150_COLOR_KIL_THSH_CTL,0x10 > @@ -1072,7 +1072,8 @@ static int tvp5150_s_stream(struct v4l2_subdev *sd, int 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; > + val |= TVP5150_MISC_CTL_YCBCR_OE | TVP5150_MISC_CTL_CLOCK_OE | > + TVP5150_MISC_CTL_INTREQ_OE; > if (decoder->mbus_type == V4L2_MBUS_PARALLEL) > val |= TVP5150_MISC_CTL_SYNC_OE; > } Thanks, Mauro ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] media: i2c: tvp5150: fix color burst lock instability on some hardware 2018-03-26 9:43 ` Mauro Carvalho Chehab @ 2018-03-26 22:29 ` Nasser 2018-03-29 14:34 ` Nasser 0 siblings, 1 reply; 7+ messages in thread From: Nasser @ 2018-03-26 22:29 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: p.zabel, sakari.ailus, hans.verkuil, bparrot, garsilva, linux-media, linux-kernel On Mon, Mar 26, 2018 at 06:43:53AM -0300, Mauro Carvalho Chehab wrote: > Hi Nasser, > > Em Mon, 26 Mar 2018 03:26:33 +0430 > Nasser Afshin <afshin.nasser@gmail.com> escreveu: > > > According to the datasheet, INTREQ/GPCL/VBLK should have a pull-up/down > > resistor if it's been disabled. On hardware that does not have such > > resistor, we should use the default output enable value. > > This prevents the color burst lock instability problem. > Color burst lock instability is just a side effect of not using the recommended value for this bit. If we use the recommended setting, we will support more hardware while not breaking anything. > If this is hardware-dependent, you should instead store it at > OF (for SoC) or pass via platform_data (for PCI/USB devices). > We have used the recommended value for this bit (as the datasheet suggests) while we are in tvp5150_init_enable but in tvp5150_s_stream we are using the wrong value. Also we have this comment at line 319: /* Default values as sugested at TVP5150AM1 datasheet */ But as you see, TVP5150_MISC_CTL is not set to its suggested default value. > > > > Signed-off-by: Nasser Afshin <Afshin.Nasser@gmail.com> > > --- > > drivers/media/i2c/tvp5150.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c > > index 2476d812f669..0e9713814816 100644 > > --- a/drivers/media/i2c/tvp5150.c > > +++ b/drivers/media/i2c/tvp5150.c > > @@ -328,7 +328,7 @@ static const struct i2c_reg_value tvp5150_init_default[] = { > > TVP5150_OP_MODE_CTL,0x00 > > }, > > { /* 0x03 */ > > - TVP5150_MISC_CTL,0x01 > > + TVP5150_MISC_CTL,0x21 > > }, > > { /* 0x06 */ > > TVP5150_COLOR_KIL_THSH_CTL,0x10 > > @@ -1072,7 +1072,8 @@ static int tvp5150_s_stream(struct v4l2_subdev *sd, int 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; > > + val |= TVP5150_MISC_CTL_YCBCR_OE | TVP5150_MISC_CTL_CLOCK_OE | > > + TVP5150_MISC_CTL_INTREQ_OE; > > if (decoder->mbus_type == V4L2_MBUS_PARALLEL) > > val |= TVP5150_MISC_CTL_SYNC_OE; > > } > > > > Thanks, > Mauro ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] media: i2c: tvp5150: fix color burst lock instability on some hardware 2018-03-26 22:29 ` Nasser @ 2018-03-29 14:34 ` Nasser 2018-03-29 15:02 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 7+ messages in thread From: Nasser @ 2018-03-29 14:34 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: p.zabel, sakari.ailus, hans.verkuil, bparrot, garsilva, linux-media, linux-kernel On Tue, Mar 27, 2018 at 02:59:21AM +0430, Nasser wrote: Hi Mauro, Thank you for taking time to review my patch. May be I should rephrase the commit message to something like: Use the default register values as suggested in TVP5150AM1 datasheet As this is not a hardware-dependent issue. Am I missing something? > On Mon, Mar 26, 2018 at 06:43:53AM -0300, Mauro Carvalho Chehab wrote: > > Hi Nasser, > > > > Em Mon, 26 Mar 2018 03:26:33 +0430 > > Nasser Afshin <afshin.nasser@gmail.com> escreveu: > > > > > According to the datasheet, INTREQ/GPCL/VBLK should have a pull-up/down > > > resistor if it's been disabled. On hardware that does not have such > > > resistor, we should use the default output enable value. > > > This prevents the color burst lock instability problem. > > > > Color burst lock instability is just a side effect of not using the > recommended value for this bit. If we use the recommended setting, we > will support more hardware while not breaking anything. > > > If this is hardware-dependent, you should instead store it at > > OF (for SoC) or pass via platform_data (for PCI/USB devices). > > > > We have used the recommended value for this bit (as the datasheet > suggests) while we are in tvp5150_init_enable but in tvp5150_s_stream > we are using the wrong value. > > Also we have this comment at line 319: > /* Default values as sugested at TVP5150AM1 datasheet */ > But as you see, TVP5150_MISC_CTL is not set to its suggested default > value. > > > > > > > Signed-off-by: Nasser Afshin <Afshin.Nasser@gmail.com> > > > --- > > > drivers/media/i2c/tvp5150.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c > > > index 2476d812f669..0e9713814816 100644 > > > --- a/drivers/media/i2c/tvp5150.c > > > +++ b/drivers/media/i2c/tvp5150.c > > > @@ -328,7 +328,7 @@ static const struct i2c_reg_value tvp5150_init_default[] = { > > > TVP5150_OP_MODE_CTL,0x00 > > > }, > > > { /* 0x03 */ > > > - TVP5150_MISC_CTL,0x01 > > > + TVP5150_MISC_CTL,0x21 > > > }, > > > { /* 0x06 */ > > > TVP5150_COLOR_KIL_THSH_CTL,0x10 > > > @@ -1072,7 +1072,8 @@ static int tvp5150_s_stream(struct v4l2_subdev *sd, int 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; > > > + val |= TVP5150_MISC_CTL_YCBCR_OE | TVP5150_MISC_CTL_CLOCK_OE | > > > + TVP5150_MISC_CTL_INTREQ_OE; > > > if (decoder->mbus_type == V4L2_MBUS_PARALLEL) > > > val |= TVP5150_MISC_CTL_SYNC_OE; > > > } > > > > > > > > Thanks, > > Mauro Thanks, Nasser ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] media: i2c: tvp5150: fix color burst lock instability on some hardware 2018-03-29 14:34 ` Nasser @ 2018-03-29 15:02 ` Mauro Carvalho Chehab 2018-03-29 18:07 ` Nasser 0 siblings, 1 reply; 7+ messages in thread From: Mauro Carvalho Chehab @ 2018-03-29 15:02 UTC (permalink / raw) To: Nasser Cc: p.zabel, sakari.ailus, hans.verkuil, bparrot, garsilva, linux-media, linux-kernel Em Thu, 29 Mar 2018 19:04:35 +0430 Nasser <afshin.nasser@gmail.com> escreveu: > On Tue, Mar 27, 2018 at 02:59:21AM +0430, Nasser wrote: > Hi Mauro, > > Thank you for taking time to review my patch. > > May be I should rephrase the commit message to something like: > Use the default register values as suggested in TVP5150AM1 datasheet > > As this is not a hardware-dependent issue. Am I missing something? It is not a matter of rephasing, but, instead, to be sure that it won't cause regressions on existing hardware. Yet, it would worth if you could describe at the patch what hardware did you test it, and if VBI was tested too. Anyway, I'll try to find some time to run some tests on the hardware I have with tvp5150 too. Regards, Mauro > > > On Mon, Mar 26, 2018 at 06:43:53AM -0300, Mauro Carvalho Chehab wrote: > > > Hi Nasser, > > > > > > Em Mon, 26 Mar 2018 03:26:33 +0430 > > > Nasser Afshin <afshin.nasser@gmail.com> escreveu: > > > > > > > According to the datasheet, INTREQ/GPCL/VBLK should have a pull-up/down > > > > resistor if it's been disabled. On hardware that does not have such > > > > resistor, we should use the default output enable value. > > > > This prevents the color burst lock instability problem. > > > > > > > Color burst lock instability is just a side effect of not using the > > recommended value for this bit. If we use the recommended setting, we > > will support more hardware while not breaking anything. > > > > > If this is hardware-dependent, you should instead store it at > > > OF (for SoC) or pass via platform_data (for PCI/USB devices). > > > > > > > We have used the recommended value for this bit (as the datasheet > > suggests) while we are in tvp5150_init_enable but in tvp5150_s_stream > > we are using the wrong value. > > > > Also we have this comment at line 319: > > /* Default values as sugested at TVP5150AM1 datasheet */ > > But as you see, TVP5150_MISC_CTL is not set to its suggested default > > value. > > > > > > > > > > Signed-off-by: Nasser Afshin <Afshin.Nasser@gmail.com> > > > > --- > > > > drivers/media/i2c/tvp5150.c | 5 +++-- > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c > > > > index 2476d812f669..0e9713814816 100644 > > > > --- a/drivers/media/i2c/tvp5150.c > > > > +++ b/drivers/media/i2c/tvp5150.c > > > > @@ -328,7 +328,7 @@ static const struct i2c_reg_value tvp5150_init_default[] = { > > > > TVP5150_OP_MODE_CTL,0x00 > > > > }, > > > > { /* 0x03 */ > > > > - TVP5150_MISC_CTL,0x01 > > > > + TVP5150_MISC_CTL,0x21 > > > > }, > > > > { /* 0x06 */ > > > > TVP5150_COLOR_KIL_THSH_CTL,0x10 > > > > @@ -1072,7 +1072,8 @@ static int tvp5150_s_stream(struct v4l2_subdev *sd, int 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; > > > > + val |= TVP5150_MISC_CTL_YCBCR_OE | TVP5150_MISC_CTL_CLOCK_OE | > > > > + TVP5150_MISC_CTL_INTREQ_OE; > > > > if (decoder->mbus_type == V4L2_MBUS_PARALLEL) > > > > val |= TVP5150_MISC_CTL_SYNC_OE; > > > > } > > > > > > > > > > > > Thanks, > > > Mauro > > Thanks, > Nasser Thanks, Mauro ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] media: i2c: tvp5150: fix color burst lock instability on some hardware 2018-03-29 15:02 ` Mauro Carvalho Chehab @ 2018-03-29 18:07 ` Nasser 2018-03-29 19:19 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 7+ messages in thread From: Nasser @ 2018-03-29 18:07 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: p.zabel, sakari.ailus, hans.verkuil, bparrot, garsilva, linux-media, linux-kernel On Thu, Mar 29, 2018 at 12:02:40PM -0300, Mauro Carvalho Chehab wrote: > Em Thu, 29 Mar 2018 19:04:35 +0430 > Nasser <afshin.nasser@gmail.com> escreveu: > > > On Tue, Mar 27, 2018 at 02:59:21AM +0430, Nasser wrote: > > Hi Mauro, > > > > Thank you for taking time to review my patch. > > > > May be I should rephrase the commit message to something like: > > Use the default register values as suggested in TVP5150AM1 datasheet > > > > As this is not a hardware-dependent issue. Am I missing something? > > It is not a matter of rephasing, but, instead, to be sure that it won't > cause regressions on existing hardware. > > Yet, it would worth if you could describe at the patch what hardware > did you test it, and if VBI was tested too. > Does this means that I should resend the patch with this additional info? Sorry for not being clear about that. This was a custom board based on ARM. The VBI was not used. > Anyway, I'll try to find some time to run some tests on the hardware > I have with tvp5150 too. It sounds great. > > Regards, > Mauro > > > > > > On Mon, Mar 26, 2018 at 06:43:53AM -0300, Mauro Carvalho Chehab wrote: > > > > Hi Nasser, > > > > > > > > Em Mon, 26 Mar 2018 03:26:33 +0430 > > > > Nasser Afshin <afshin.nasser@gmail.com> escreveu: > > > > > > > > > According to the datasheet, INTREQ/GPCL/VBLK should have a pull-up/down > > > > > resistor if it's been disabled. On hardware that does not have such > > > > > resistor, we should use the default output enable value. > > > > > This prevents the color burst lock instability problem. > > > > > > > > > > Color burst lock instability is just a side effect of not using the > > > recommended value for this bit. If we use the recommended setting, we > > > will support more hardware while not breaking anything. > > > > > > > If this is hardware-dependent, you should instead store it at > > > > OF (for SoC) or pass via platform_data (for PCI/USB devices). > > > > > > > > > > We have used the recommended value for this bit (as the datasheet > > > suggests) while we are in tvp5150_init_enable but in tvp5150_s_stream > > > we are using the wrong value. > > > > > > Also we have this comment at line 319: > > > /* Default values as sugested at TVP5150AM1 datasheet */ > > > But as you see, TVP5150_MISC_CTL is not set to its suggested default > > > value. > > > Any way the assignment to tvp5150_init_default after the above comment seems not to be correct according to the "3.21.4 Miscellaneous Controls Register" part in the datasheet. While following the same comment phrase on line 455, we see the correct assignment as the default values to tvp5150_init_enable. Sorry this is so lengthy. Thank you, Nasser > > > > > > > > > > Signed-off-by: Nasser Afshin <Afshin.Nasser@gmail.com> > > > > > --- > > > > > drivers/media/i2c/tvp5150.c | 5 +++-- > > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c > > > > > index 2476d812f669..0e9713814816 100644 > > > > > --- a/drivers/media/i2c/tvp5150.c > > > > > +++ b/drivers/media/i2c/tvp5150.c > > > > > @@ -328,7 +328,7 @@ static const struct i2c_reg_value tvp5150_init_default[] = { > > > > > TVP5150_OP_MODE_CTL,0x00 > > > > > }, > > > > > { /* 0x03 */ > > > > > - TVP5150_MISC_CTL,0x01 > > > > > + TVP5150_MISC_CTL,0x21 > > > > > }, > > > > > { /* 0x06 */ > > > > > TVP5150_COLOR_KIL_THSH_CTL,0x10 > > > > > @@ -1072,7 +1072,8 @@ static int tvp5150_s_stream(struct v4l2_subdev *sd, int 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; > > > > > + val |= TVP5150_MISC_CTL_YCBCR_OE | TVP5150_MISC_CTL_CLOCK_OE | > > > > > + TVP5150_MISC_CTL_INTREQ_OE; > > > > > if (decoder->mbus_type == V4L2_MBUS_PARALLEL) > > > > > val |= TVP5150_MISC_CTL_SYNC_OE; > > > > > } > > > > > > > > > > > > > > > > Thanks, > > > > Mauro > > > > Thanks, > > Nasser > > > > Thanks, > Mauro ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] media: i2c: tvp5150: fix color burst lock instability on some hardware 2018-03-29 18:07 ` Nasser @ 2018-03-29 19:19 ` Mauro Carvalho Chehab 0 siblings, 0 replies; 7+ messages in thread From: Mauro Carvalho Chehab @ 2018-03-29 19:19 UTC (permalink / raw) To: Nasser Cc: p.zabel, sakari.ailus, hans.verkuil, bparrot, garsilva, linux-media, linux-kernel Em Thu, 29 Mar 2018 22:37:57 +0430 Nasser <afshin.nasser@gmail.com> escreveu: > On Thu, Mar 29, 2018 at 12:02:40PM -0300, Mauro Carvalho Chehab wrote: > > Em Thu, 29 Mar 2018 19:04:35 +0430 > > Nasser <afshin.nasser@gmail.com> escreveu: > > > > > On Tue, Mar 27, 2018 at 02:59:21AM +0430, Nasser wrote: > > > Hi Mauro, > > > > > > Thank you for taking time to review my patch. > > > > > > May be I should rephrase the commit message to something like: > > > Use the default register values as suggested in TVP5150AM1 datasheet > > > > > > As this is not a hardware-dependent issue. Am I missing something? > > > > It is not a matter of rephasing, but, instead, to be sure that it won't > > cause regressions on existing hardware. > > > > Yet, it would worth if you could describe at the patch what hardware > > did you test it, and if VBI was tested too. > > > > Does this means that I should resend the patch with this additional info? > Sorry for not being clear about that. This was a custom board based on > ARM. The VBI was not used. That's what I suspected when you send your patch :-) The tvp5150 is used by several USB and PCI devices for analog TV and for video stream capture, with may have different requirements from what you're doing on your ARM board. Changing a register setting global wide without enough care will very likely break support for existing boards. That's why I said that the right thing to do is to pass a parameter to the driver specifying what "extra" features are needed. Without looking at tvp5150/tvp5151/tvp5150am datasheets nor testing, I'd risk to say that, at the specific case of your patch, touching at VBLK settings have a potential risk of affecting VBI handling. > > Anyway, I'll try to find some time to run some tests on the hardware > > I have with tvp5150 too. > > It sounds great. Yes, but I won't be able to do it on the next couple of weeks. We're approaching the merge window for Kernel 4.17. Thanks, Mauro ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-03-29 19:19 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-03-25 22:56 [PATCH] media: i2c: tvp5150: fix color burst lock instability on some hardware Nasser Afshin 2018-03-26 9:43 ` Mauro Carvalho Chehab 2018-03-26 22:29 ` Nasser 2018-03-29 14:34 ` Nasser 2018-03-29 15:02 ` Mauro Carvalho Chehab 2018-03-29 18:07 ` Nasser 2018-03-29 19:19 ` Mauro Carvalho Chehab
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).