[PATCH] media: i2c: adv7180: fix adv7280 BT.656-4 compatibility

[PATCH] media: i2c: adv7180: fix adv7280 BT.656-4 compatibility

[Matthew Michilot]

From: Matthew Michilot <matthew.michilot@gmail.com>

Captured video would be out of sync when using the adv7280 with
the BT.656-4 protocol. Certain registers (0x04, 0x31, 0xE6) had to
be configured properly to ensure BT.656-4 compatibility.

An error in the adv7280 reference manual suggested that EAV/SAV mode
was enabled by default, however upon inspecting register 0x31, it was
determined to be disabled by default.

Signed-off-by: Matthew Michilot <matthew.michilot@gmail.com>
Reviewed-by: Tim Harvey <tharvey@gateworks.com>
---
 drivers/media/i2c/adv7180.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 99697baad2ea..27da424dce76 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -94,6 +94,7 @@
 #define ADV7180_REG_SHAP_FILTER_CTL_1	0x0017
 #define ADV7180_REG_CTRL_2		0x001d
 #define ADV7180_REG_VSYNC_FIELD_CTL_1	0x0031
+#define ADV7180_VSYNC_FIELD_CTL_1_NEWAV 0x12
 #define ADV7180_REG_MANUAL_WIN_CTL_1	0x003d
 #define ADV7180_REG_MANUAL_WIN_CTL_2	0x003e
 #define ADV7180_REG_MANUAL_WIN_CTL_3	0x003f
@@ -935,10 +936,20 @@ static int adv7182_init(struct adv7180_state *state)
 		adv7180_write(state, ADV7180_REG_EXTENDED_OUTPUT_CONTROL, 0x57);
 		adv7180_write(state, ADV7180_REG_CTRL_2, 0xc0);
 	} else {
-		if (state->chip_info->flags & ADV7180_FLAG_V2)
+		if (state->chip_info->flags & ADV7180_FLAG_V2) {
+			/* ITU-R BT.656-4 compatible */
 			adv7180_write(state,
 				      ADV7180_REG_EXTENDED_OUTPUT_CONTROL,
-				      0x17);
+				      ADV7180_EXTENDED_OUTPUT_CONTROL_NTSCDIS);
+			/* Manually set NEWAVMODE */
+			adv7180_write(state,
+				      ADV7180_REG_VSYNC_FIELD_CTL_1,
+				      ADV7180_VSYNC_FIELD_CTL_1_NEWAV);
+			/* Manually set V bit end position in NTSC mode */
+			adv7180_write(state,
+				      ADV7180_REG_NTSC_V_BIT_END,
+				      ADV7180_NTSC_V_BIT_END_MANUAL_NVEND);
+		}
 		else
 			adv7180_write(state,
 				      ADV7180_REG_EXTENDED_OUTPUT_CONTROL,
-- 
2.17.1

Re: [PATCH] media: i2c: adv7180: fix adv7280 BT.656-4 compatibility

[Hans Verkuil]

Adding Niklas.

Niklas, can you take a look at this?

Regards,

	Hans

On 8/27/19 11:55 PM, Matthew Michilot wrote:
> From: Matthew Michilot <matthew.michilot@gmail.com>
> 
> Captured video would be out of sync when using the adv7280 with
> the BT.656-4 protocol. Certain registers (0x04, 0x31, 0xE6) had to
> be configured properly to ensure BT.656-4 compatibility.
> 
> An error in the adv7280 reference manual suggested that EAV/SAV mode
> was enabled by default, however upon inspecting register 0x31, it was
> determined to be disabled by default.
> 
> Signed-off-by: Matthew Michilot <matthew.michilot@gmail.com>
> Reviewed-by: Tim Harvey <tharvey@gateworks.com>
> ---
>  drivers/media/i2c/adv7180.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> index 99697baad2ea..27da424dce76 100644
> --- a/drivers/media/i2c/adv7180.c
> +++ b/drivers/media/i2c/adv7180.c
> @@ -94,6 +94,7 @@
>  #define ADV7180_REG_SHAP_FILTER_CTL_1	0x0017
>  #define ADV7180_REG_CTRL_2		0x001d
>  #define ADV7180_REG_VSYNC_FIELD_CTL_1	0x0031
> +#define ADV7180_VSYNC_FIELD_CTL_1_NEWAV 0x12
>  #define ADV7180_REG_MANUAL_WIN_CTL_1	0x003d
>  #define ADV7180_REG_MANUAL_WIN_CTL_2	0x003e
>  #define ADV7180_REG_MANUAL_WIN_CTL_3	0x003f
> @@ -935,10 +936,20 @@ static int adv7182_init(struct adv7180_state *state)
>  		adv7180_write(state, ADV7180_REG_EXTENDED_OUTPUT_CONTROL, 0x57);
>  		adv7180_write(state, ADV7180_REG_CTRL_2, 0xc0);
>  	} else {
> -		if (state->chip_info->flags & ADV7180_FLAG_V2)
> +		if (state->chip_info->flags & ADV7180_FLAG_V2) {
> +			/* ITU-R BT.656-4 compatible */
>  			adv7180_write(state,
>  				      ADV7180_REG_EXTENDED_OUTPUT_CONTROL,
> -				      0x17);
> +				      ADV7180_EXTENDED_OUTPUT_CONTROL_NTSCDIS);
> +			/* Manually set NEWAVMODE */
> +			adv7180_write(state,
> +				      ADV7180_REG_VSYNC_FIELD_CTL_1,
> +				      ADV7180_VSYNC_FIELD_CTL_1_NEWAV);
> +			/* Manually set V bit end position in NTSC mode */
> +			adv7180_write(state,
> +				      ADV7180_REG_NTSC_V_BIT_END,
> +				      ADV7180_NTSC_V_BIT_END_MANUAL_NVEND);
> +		}
>  		else
>  			adv7180_write(state,
>  				      ADV7180_REG_EXTENDED_OUTPUT_CONTROL,
> 

Re: [PATCH] media: i2c: adv7180: fix adv7280 BT.656-4 compatibility

[Niklas Söderlund]

Hi,

On 2019-08-29 13:43:49 +0200, Hans Verkuil wrote:
> Adding Niklas.
> 
> Niklas, can you take a look at this?

I'm happy to have a look at this. I'm currently moving so all my boards 
are in a box somewhere. I hope to have my lab up and running next week, 
so if this is not urgent I will look at it then.

> 
> Regards,
> 
> 	Hans
> 
> On 8/27/19 11:55 PM, Matthew Michilot wrote:
> > From: Matthew Michilot <matthew.michilot@gmail.com>
> > 
> > Captured video would be out of sync when using the adv7280 with
> > the BT.656-4 protocol. Certain registers (0x04, 0x31, 0xE6) had to
> > be configured properly to ensure BT.656-4 compatibility.
> > 
> > An error in the adv7280 reference manual suggested that EAV/SAV mode
> > was enabled by default, however upon inspecting register 0x31, it was
> > determined to be disabled by default.
> > 
> > Signed-off-by: Matthew Michilot <matthew.michilot@gmail.com>
> > Reviewed-by: Tim Harvey <tharvey@gateworks.com>
> > ---
> >  drivers/media/i2c/adv7180.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> > index 99697baad2ea..27da424dce76 100644
> > --- a/drivers/media/i2c/adv7180.c
> > +++ b/drivers/media/i2c/adv7180.c
> > @@ -94,6 +94,7 @@
> >  #define ADV7180_REG_SHAP_FILTER_CTL_1	0x0017
> >  #define ADV7180_REG_CTRL_2		0x001d
> >  #define ADV7180_REG_VSYNC_FIELD_CTL_1	0x0031
> > +#define ADV7180_VSYNC_FIELD_CTL_1_NEWAV 0x12
> >  #define ADV7180_REG_MANUAL_WIN_CTL_1	0x003d
> >  #define ADV7180_REG_MANUAL_WIN_CTL_2	0x003e
> >  #define ADV7180_REG_MANUAL_WIN_CTL_3	0x003f
> > @@ -935,10 +936,20 @@ static int adv7182_init(struct adv7180_state *state)
> >  		adv7180_write(state, ADV7180_REG_EXTENDED_OUTPUT_CONTROL, 0x57);
> >  		adv7180_write(state, ADV7180_REG_CTRL_2, 0xc0);
> >  	} else {
> > -		if (state->chip_info->flags & ADV7180_FLAG_V2)
> > +		if (state->chip_info->flags & ADV7180_FLAG_V2) {
> > +			/* ITU-R BT.656-4 compatible */
> >  			adv7180_write(state,
> >  				      ADV7180_REG_EXTENDED_OUTPUT_CONTROL,
> > -				      0x17);
> > +				      ADV7180_EXTENDED_OUTPUT_CONTROL_NTSCDIS);
> > +			/* Manually set NEWAVMODE */
> > +			adv7180_write(state,
> > +				      ADV7180_REG_VSYNC_FIELD_CTL_1,
> > +				      ADV7180_VSYNC_FIELD_CTL_1_NEWAV);
> > +			/* Manually set V bit end position in NTSC mode */
> > +			adv7180_write(state,
> > +				      ADV7180_REG_NTSC_V_BIT_END,
> > +				      ADV7180_NTSC_V_BIT_END_MANUAL_NVEND);
> > +		}
> >  		else
> >  			adv7180_write(state,
> >  				      ADV7180_REG_EXTENDED_OUTPUT_CONTROL,
> > 
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH] media: i2c: adv7180: fix adv7280 BT.656-4 compatibility

[Tim Harvey]

On Thu, Aug 29, 2019 at 7:29 AM Niklas Söderlund
<niklas.soderlund@ragnatech.se> wrote:
>
> Hi,
>
> On 2019-08-29 13:43:49 +0200, Hans Verkuil wrote:
> > Adding Niklas.
> >
> > Niklas, can you take a look at this?
>
> I'm happy to have a look at this. I'm currently moving so all my boards
> are in a box somewhere. I hope to have my lab up and running next week,
> so if this is not urgent I will look at it then.
>

Niklas,

Have you looked at this yet? Without this patch the ADV7280A does not
output proper BT.656. We tested this on a Gateworks Ventana GW5404-G
which uses the ADV7280A connected to the IMX6 CSI parallel bus. I'm
hoping to see this get merged and perhaps backported to older kernels.

Regards,

Tim

> >
> > Regards,
> >
> >       Hans
> >
> > On 8/27/19 11:55 PM, Matthew Michilot wrote:
> > > From: Matthew Michilot <matthew.michilot@gmail.com>
> > >
> > > Captured video would be out of sync when using the adv7280 with
> > > the BT.656-4 protocol. Certain registers (0x04, 0x31, 0xE6) had to
> > > be configured properly to ensure BT.656-4 compatibility.
> > >
> > > An error in the adv7280 reference manual suggested that EAV/SAV mode
> > > was enabled by default, however upon inspecting register 0x31, it was
> > > determined to be disabled by default.
> > >
> > > Signed-off-by: Matthew Michilot <matthew.michilot@gmail.com>
> > > Reviewed-by: Tim Harvey <tharvey@gateworks.com>
> > > ---
> > >  drivers/media/i2c/adv7180.c | 15 +++++++++++++--
> > >  1 file changed, 13 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> > > index 99697baad2ea..27da424dce76 100644
> > > --- a/drivers/media/i2c/adv7180.c
> > > +++ b/drivers/media/i2c/adv7180.c
> > > @@ -94,6 +94,7 @@
> > >  #define ADV7180_REG_SHAP_FILTER_CTL_1      0x0017
> > >  #define ADV7180_REG_CTRL_2         0x001d
> > >  #define ADV7180_REG_VSYNC_FIELD_CTL_1      0x0031
> > > +#define ADV7180_VSYNC_FIELD_CTL_1_NEWAV 0x12
> > >  #define ADV7180_REG_MANUAL_WIN_CTL_1       0x003d
> > >  #define ADV7180_REG_MANUAL_WIN_CTL_2       0x003e
> > >  #define ADV7180_REG_MANUAL_WIN_CTL_3       0x003f
> > > @@ -935,10 +936,20 @@ static int adv7182_init(struct adv7180_state *state)
> > >             adv7180_write(state, ADV7180_REG_EXTENDED_OUTPUT_CONTROL, 0x57);
> > >             adv7180_write(state, ADV7180_REG_CTRL_2, 0xc0);
> > >     } else {
> > > -           if (state->chip_info->flags & ADV7180_FLAG_V2)
> > > +           if (state->chip_info->flags & ADV7180_FLAG_V2) {
> > > +                   /* ITU-R BT.656-4 compatible */
> > >                     adv7180_write(state,
> > >                                   ADV7180_REG_EXTENDED_OUTPUT_CONTROL,
> > > -                                 0x17);
> > > +                                 ADV7180_EXTENDED_OUTPUT_CONTROL_NTSCDIS);
> > > +                   /* Manually set NEWAVMODE */
> > > +                   adv7180_write(state,
> > > +                                 ADV7180_REG_VSYNC_FIELD_CTL_1,
> > > +                                 ADV7180_VSYNC_FIELD_CTL_1_NEWAV);
> > > +                   /* Manually set V bit end position in NTSC mode */
> > > +                   adv7180_write(state,
> > > +                                 ADV7180_REG_NTSC_V_BIT_END,
> > > +                                 ADV7180_NTSC_V_BIT_END_MANUAL_NVEND);
> > > +           }
> > >             else
> > >                     adv7180_write(state,
> > >                                   ADV7180_REG_EXTENDED_OUTPUT_CONTROL,
> > >
> >
>
> --
> Regards,
> Niklas Söderlund

Re: [PATCH] media: i2c: adv7180: fix adv7280 BT.656-4 compatibility

[Niklas Söderlund]

Hi Tim,

Sorry for taking to so long to look at this.

On 2019-09-23 15:04:47 -0700, Tim Harvey wrote:
> On Thu, Aug 29, 2019 at 7:29 AM Niklas Söderlund
> <niklas.soderlund@ragnatech.se> wrote:
> >
> > Hi,
> >
> > On 2019-08-29 13:43:49 +0200, Hans Verkuil wrote:
> > > Adding Niklas.
> > >
> > > Niklas, can you take a look at this?
> >
> > I'm happy to have a look at this. I'm currently moving so all my boards
> > are in a box somewhere. I hope to have my lab up and running next week,
> > so if this is not urgent I will look at it then.
> >
> 
> Niklas,
> 
> Have you looked at this yet? Without this patch the ADV7280A does not
> output proper BT.656. We tested this on a Gateworks Ventana GW5404-G
> which uses the ADV7280A connected to the IMX6 CSI parallel bus. I'm
> hoping to see this get merged and perhaps backported to older kernels.

I only have access to an adv7180 so I was unable to test this patch.  
After reviewing the documentation I think the patch is OK if what you 
want is to unconditionally switch the driver from outputting BT.656-3 to 
outputting BT.656-4.

As this change would effect a large number of compat strings (adv7280, 
adv7280-m, adv7281, adv7281-m, adv7281-ma, adv7282, adv7282-m) and the 
goal is to back port it I'm a bit reluctant to adding my tag to this 
patch as I'm not sure if this will break other setups.

From the documentation about the BT.656-4 register (address 0x04 bit 7):

    Between Revision 3 and Revision 4 of the ITU-R BT.656 standards,
    the ITU has changed the toggling position for the V bit within
    the SAV EAV codes for NTSC. The ITU-R BT.656-4 standard
    bit allows the user to select an output mode that is compliant
    with either the previous or new standard. For further information,
    visit the International Telecommunication Union website.

    Note that the standard change only affects NTSC and has no
    bearing on PAL.

    When ITU-R BT.656-4 is 0 (default), the ITU-R BT.656-3
    specification is used. The V bit goes low at EAV of Line 10
    and Line 273.

    When ITU-R BT.656-4 is 1, the ITU-R BT.656-4 specification is
    used. The V bit goes low at EAV of Line 20 and Line 283.

Do you know what effects such a change would bring? Looking at the 
driver BT.656-4 seems to be set unconditionally for some adv7180 chips.

> 
> Regards,
> 
> Tim
> 
> > >
> > > Regards,
> > >
> > >       Hans
> > >
> > > On 8/27/19 11:55 PM, Matthew Michilot wrote:
> > > > From: Matthew Michilot <matthew.michilot@gmail.com>
> > > >
> > > > Captured video would be out of sync when using the adv7280 with
> > > > the BT.656-4 protocol. Certain registers (0x04, 0x31, 0xE6) had to
> > > > be configured properly to ensure BT.656-4 compatibility.
> > > >
> > > > An error in the adv7280 reference manual suggested that EAV/SAV mode
> > > > was enabled by default, however upon inspecting register 0x31, it was
> > > > determined to be disabled by default.

The manual I have [1] states that NEWAVMODE is switched off by default.  
I'm only asking as I would like to know if there is an error in that 
datasheet or not.

1. https://www.analog.com/media/en/technical-documentation/user-guides/ADV7280_7281_7282_7283_UG-637.pdf

> > > >
> > > > Signed-off-by: Matthew Michilot <matthew.michilot@gmail.com>
> > > > Reviewed-by: Tim Harvey <tharvey@gateworks.com>
> > > > ---
> > > >  drivers/media/i2c/adv7180.c | 15 +++++++++++++--
> > > >  1 file changed, 13 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> > > > index 99697baad2ea..27da424dce76 100644
> > > > --- a/drivers/media/i2c/adv7180.c
> > > > +++ b/drivers/media/i2c/adv7180.c
> > > > @@ -94,6 +94,7 @@
> > > >  #define ADV7180_REG_SHAP_FILTER_CTL_1      0x0017
> > > >  #define ADV7180_REG_CTRL_2         0x001d
> > > >  #define ADV7180_REG_VSYNC_FIELD_CTL_1      0x0031
> > > > +#define ADV7180_VSYNC_FIELD_CTL_1_NEWAV 0x12
> > > >  #define ADV7180_REG_MANUAL_WIN_CTL_1       0x003d
> > > >  #define ADV7180_REG_MANUAL_WIN_CTL_2       0x003e
> > > >  #define ADV7180_REG_MANUAL_WIN_CTL_3       0x003f
> > > > @@ -935,10 +936,20 @@ static int adv7182_init(struct adv7180_state *state)
> > > >             adv7180_write(state, ADV7180_REG_EXTENDED_OUTPUT_CONTROL, 0x57);
> > > >             adv7180_write(state, ADV7180_REG_CTRL_2, 0xc0);
> > > >     } else {
> > > > -           if (state->chip_info->flags & ADV7180_FLAG_V2)
> > > > +           if (state->chip_info->flags & ADV7180_FLAG_V2) {
> > > > +                   /* ITU-R BT.656-4 compatible */
> > > >                     adv7180_write(state,
> > > >                                   ADV7180_REG_EXTENDED_OUTPUT_CONTROL,
> > > > -                                 0x17);
> > > > +                                 ADV7180_EXTENDED_OUTPUT_CONTROL_NTSCDIS);
> > > > +                   /* Manually set NEWAVMODE */
> > > > +                   adv7180_write(state,
> > > > +                                 ADV7180_REG_VSYNC_FIELD_CTL_1,
> > > > +                                 ADV7180_VSYNC_FIELD_CTL_1_NEWAV);
> > > > +                   /* Manually set V bit end position in NTSC mode */
> > > > +                   adv7180_write(state,
> > > > +                                 ADV7180_REG_NTSC_V_BIT_END,
> > > > +                                 ADV7180_NTSC_V_BIT_END_MANUAL_NVEND);
> > > > +           }
> > > >             else
> > > >                     adv7180_write(state,
> > > >                                   ADV7180_REG_EXTENDED_OUTPUT_CONTROL,
> > > >
> > >
> >
> > --
> > Regards,
> > Niklas Söderlund

-- 
Regards,
Niklas Söderlund

Re: [PATCH] media: i2c: adv7180: fix adv7280 BT.656-4 compatibility

[Fabio Estevam]

Hi Tim,

On Wed, Sep 25, 2019 at 9:22 PM Tim Harvey <tharvey@gateworks.com> wrote:

> Have you looked at this yet? Without this patch the ADV7280A does not
> output proper BT.656. We tested this on a Gateworks Ventana GW5404-G
> which uses the ADV7280A connected to the IMX6 CSI parallel bus. I'm
> hoping to see this get merged and perhaps backported to older kernels.

In order to be backported to older kernels, please add a Fixes tag.

Thanks

Re: [PATCH] media: i2c: adv7180: fix adv7280 BT.656-4 compatibility

[Tim Harvey]

On Fri, Sep 27, 2019 at 12:04 PM Niklas Söderlund
<niklas.soderlund@ragnatech.se> wrote:
>
> Hi Tim,
>
> Sorry for taking to so long to look at this.
>
> On 2019-09-23 15:04:47 -0700, Tim Harvey wrote:
> > On Thu, Aug 29, 2019 at 7:29 AM Niklas Söderlund
> > <niklas.soderlund@ragnatech.se> wrote:
> > >
> > > Hi,
> > >
> > > On 2019-08-29 13:43:49 +0200, Hans Verkuil wrote:
> > > > Adding Niklas.
> > > >
> > > > Niklas, can you take a look at this?
> > >
> > > I'm happy to have a look at this. I'm currently moving so all my boards
> > > are in a box somewhere. I hope to have my lab up and running next week,
> > > so if this is not urgent I will look at it then.
> > >
> >
> > Niklas,
> >
> > Have you looked at this yet? Without this patch the ADV7280A does not
> > output proper BT.656. We tested this on a Gateworks Ventana GW5404-G
> > which uses the ADV7280A connected to the IMX6 CSI parallel bus. I'm
> > hoping to see this get merged and perhaps backported to older kernels.
>
> I only have access to an adv7180 so I was unable to test this patch.
> After reviewing the documentation I think the patch is OK if what you
> want is to unconditionally switch the driver from outputting BT.656-3 to
> outputting BT.656-4.
>
> As this change would effect a large number of compat strings (adv7280,
> adv7280-m, adv7281, adv7281-m, adv7281-ma, adv7282, adv7282-m) and the
> goal is to back port it I'm a bit reluctant to adding my tag to this
> patch as I'm not sure if this will break other setups.
>
> From the documentation about the BT.656-4 register (address 0x04 bit 7):
>
>     Between Revision 3 and Revision 4 of the ITU-R BT.656 standards,
>     the ITU has changed the toggling position for the V bit within
>     the SAV EAV codes for NTSC. The ITU-R BT.656-4 standard
>     bit allows the user to select an output mode that is compliant
>     with either the previous or new standard. For further information,
>     visit the International Telecommunication Union website.
>
>     Note that the standard change only affects NTSC and has no
>     bearing on PAL.
>
>     When ITU-R BT.656-4 is 0 (default), the ITU-R BT.656-3
>     specification is used. The V bit goes low at EAV of Line 10
>     and Line 273.
>
>     When ITU-R BT.656-4 is 1, the ITU-R BT.656-4 specification is
>     used. The V bit goes low at EAV of Line 20 and Line 283.
>
> Do you know what effects such a change would bring? Looking at the
> driver BT.656-4 seems to be set unconditionally for some adv7180 chips.
>

Niklas,

Quite simply, we have a board that has an ADV7180 attached to the
parallel CSI of an IMX6 that worked fine with mainline drivers then
when we revised this board to attach an ADV7280A in the same way
capture failed to sync. Investigation showed that the NEWAVMODE
differed between the two.

So if the point of the driver is to configure the variants in the same
way, this patch needs to be applied.

I would maintain that the adv7180 comes up with NEWAVMODE enabled and
in order to be compatible we must configure the adv7282 the same.

The same argument can be made for setting the V bit end position in
NTSC mode - its done for the adv7180 so for compatible output it
should be done for the adv7282.

> >
> > Regards,
> >
> > Tim
> >
> > > >
> > > > Regards,
> > > >
> > > >       Hans
> > > >
> > > > On 8/27/19 11:55 PM, Matthew Michilot wrote:
> > > > > From: Matthew Michilot <matthew.michilot@gmail.com>
> > > > >
> > > > > Captured video would be out of sync when using the adv7280 with
> > > > > the BT.656-4 protocol. Certain registers (0x04, 0x31, 0xE6) had to
> > > > > be configured properly to ensure BT.656-4 compatibility.
> > > > >
> > > > > An error in the adv7280 reference manual suggested that EAV/SAV mode
> > > > > was enabled by default, however upon inspecting register 0x31, it was
> > > > > determined to be disabled by default.
>
> The manual I have [1] states that NEWAVMODE is switched off by default.
> I'm only asking as I would like to know if there is an error in that
> datasheet or not.
>
> 1. https://www.analog.com/media/en/technical-documentation/user-guides/ADV7280_7281_7282_7283_UG-637.pdf
>

Table 99 in that document shows NEVAVMODE disabled on power-up
(0x31=0x02) yet Page 77 shows it enabled at power-up. Looking at an
actual device we find it is indeed disabled on powerup (0x31=0x02) so
Table 99 is correct, and Page 77 is not.

If you look at the ADV7180 datasheet
(https://www.analog.com/media/en/technical-documentation/data-sheets/ADV7180.pdf)
Table 105 shows NEWAVMODE enabled by default which is also reflected
in the register details on Page 91 and is what you find on an actual
device.

Regards,

Tim

-- 


CONFIDENTIALITY NOTICE: This email constitutes an electronic 
communication within the meaning of the Electronic Communications Privacy 
Act, 18 U.S.C. 2510, and its disclosure is strictly limited to the named 
recipient(s) intended by the sender of this message. This email, and any 
attachments, may contain confidential and/or proprietary information. If 
you are not a named recipient, any copying, using, disclosing or 
distributing to others the information in this email and attachments is 
STRICTLY PROHIBITED. If you have received this email in error, please 
notify the sender immediately and permanently delete the email, any 
attachments, and all copies thereof from any drives or storage media and 
destroy any printouts or hard copies of the email and attachments.

 


EXPORT COMPLIANCE NOTICE: This email and any attachments may contain 
technical data subject to U.S export restrictions under the International 
Traffic in Arms Regulations (ITAR) or the Export Administration Regulations 
(EAR). Export or transfer of this technical data and/or related information 
to any foreign person(s) or entity(ies), either within the U.S. or outside 
of the U.S., may require advance export authorization by the appropriate 
U.S. Government agency prior to export or transfer. In addition, technical 
data may not be exported or transferred to certain countries or specified 
designated nationals identified by U.S. embargo controls without prior 
export authorization. By accepting this email and any attachments, all 
recipients confirm that they understand and will comply with all applicable 
ITAR, EAR and embargo compliance requirements.

Re: [PATCH] media: i2c: adv7180: fix adv7280 BT.656-4 compatibility

[Niklas Söderlund]

Hi Tim,

On 2019-09-27 12:26:40 -0700, Tim Harvey wrote:
> On Fri, Sep 27, 2019 at 12:04 PM Niklas Söderlund
> <niklas.soderlund@ragnatech.se> wrote:
> >
> > Hi Tim,
> >
> > Sorry for taking to so long to look at this.
> >
> > On 2019-09-23 15:04:47 -0700, Tim Harvey wrote:
> > > On Thu, Aug 29, 2019 at 7:29 AM Niklas Söderlund
> > > <niklas.soderlund@ragnatech.se> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On 2019-08-29 13:43:49 +0200, Hans Verkuil wrote:
> > > > > Adding Niklas.
> > > > >
> > > > > Niklas, can you take a look at this?
> > > >
> > > > I'm happy to have a look at this. I'm currently moving so all my boards
> > > > are in a box somewhere. I hope to have my lab up and running next week,
> > > > so if this is not urgent I will look at it then.
> > > >
> > >
> > > Niklas,
> > >
> > > Have you looked at this yet? Without this patch the ADV7280A does not
> > > output proper BT.656. We tested this on a Gateworks Ventana GW5404-G
> > > which uses the ADV7280A connected to the IMX6 CSI parallel bus. I'm
> > > hoping to see this get merged and perhaps backported to older kernels.
> >
> > I only have access to an adv7180 so I was unable to test this patch.
> > After reviewing the documentation I think the patch is OK if what you
> > want is to unconditionally switch the driver from outputting BT.656-3 to
> > outputting BT.656-4.
> >
> > As this change would effect a large number of compat strings (adv7280,
> > adv7280-m, adv7281, adv7281-m, adv7281-ma, adv7282, adv7282-m) and the
> > goal is to back port it I'm a bit reluctant to adding my tag to this
> > patch as I'm not sure if this will break other setups.
> >
> > From the documentation about the BT.656-4 register (address 0x04 bit 7):
> >
> >     Between Revision 3 and Revision 4 of the ITU-R BT.656 standards,
> >     the ITU has changed the toggling position for the V bit within
> >     the SAV EAV codes for NTSC. The ITU-R BT.656-4 standard
> >     bit allows the user to select an output mode that is compliant
> >     with either the previous or new standard. For further information,
> >     visit the International Telecommunication Union website.
> >
> >     Note that the standard change only affects NTSC and has no
> >     bearing on PAL.
> >
> >     When ITU-R BT.656-4 is 0 (default), the ITU-R BT.656-3
> >     specification is used. The V bit goes low at EAV of Line 10
> >     and Line 273.
> >
> >     When ITU-R BT.656-4 is 1, the ITU-R BT.656-4 specification is
> >     used. The V bit goes low at EAV of Line 20 and Line 283.
> >
> > Do you know what effects such a change would bring? Looking at the
> > driver BT.656-4 seems to be set unconditionally for some adv7180 chips.
> >
> 
> Niklas,
> 
> Quite simply, we have a board that has an ADV7180 attached to the
> parallel CSI of an IMX6 that worked fine with mainline drivers then
> when we revised this board to attach an ADV7280A in the same way
> capture failed to sync. Investigation showed that the NEWAVMODE
> differed between the two.

I understand your problem, the driver configures adv7180 and adv7280 
differently.

> 
> So if the point of the driver is to configure the variants in the same
> way, this patch needs to be applied.

I'm not sure that is the point of the driver. As the driver today 
configures different compatible strings differently. Some as ITU-R 
BT.656-3 and some as ITU-R BT.656-4, I can only assume there is a reason 
for that.

> 
> I would maintain that the adv7180 comes up with NEWAVMODE enabled and
> in order to be compatible we must configure the adv7282 the same.
> 
> The same argument can be made for setting the V bit end position in
> NTSC mode - its done for the adv7180 so for compatible output it
> should be done for the adv7282.

I understand that this is needed to make it a drop-in replacement for 
the adv7180 in your use-case. But I'm not sure it is a good idea for 
other users of the driver. What if someone is already using a adv7282 on 
a board and depends on it providing ITU-R BT.656-3 and the old settings?
If this patch is picked up there use-cases may break.

I'm not sure what the best way forward is I'm afraid. Looking at 
video-interfaces.txt we have a device tree property bus-type which is 
used to describe the bus is a BT.656 bus but not which revision of it.

I'm not really found of driver specific bus descriptions, but maybe this 
is a case where one might consider adding one? Hans what do you think?

> 
> > >
> > > Regards,
> > >
> > > Tim
> > >
> > > > >
> > > > > Regards,
> > > > >
> > > > >       Hans
> > > > >
> > > > > On 8/27/19 11:55 PM, Matthew Michilot wrote:
> > > > > > From: Matthew Michilot <matthew.michilot@gmail.com>
> > > > > >
> > > > > > Captured video would be out of sync when using the adv7280 with
> > > > > > the BT.656-4 protocol. Certain registers (0x04, 0x31, 0xE6) had to
> > > > > > be configured properly to ensure BT.656-4 compatibility.
> > > > > >
> > > > > > An error in the adv7280 reference manual suggested that EAV/SAV mode
> > > > > > was enabled by default, however upon inspecting register 0x31, it was
> > > > > > determined to be disabled by default.
> >
> > The manual I have [1] states that NEWAVMODE is switched off by default.
> > I'm only asking as I would like to know if there is an error in that
> > datasheet or not.
> >
> > 1. https://www.analog.com/media/en/technical-documentation/user-guides/ADV7280_7281_7282_7283_UG-637.pdf
> >
> 
> Table 99 in that document shows NEVAVMODE disabled on power-up
> (0x31=0x02) yet Page 77 shows it enabled at power-up. Looking at an
> actual device we find it is indeed disabled on powerup (0x31=0x02) so
> Table 99 is correct, and Page 77 is not.
> 
> If you look at the ADV7180 datasheet
> (https://www.analog.com/media/en/technical-documentation/data-sheets/ADV7180.pdf)
> Table 105 shows NEWAVMODE enabled by default which is also reflected
> in the register details on Page 91 and is what you find on an actual
> device.
> 
> Regards,
> 
> Tim
> 
> -- 
> 
> 
> CONFIDENTIALITY NOTICE: This email constitutes an electronic 
> communication within the meaning of the Electronic Communications Privacy 
> Act, 18 U.S.C. 2510, and its disclosure is strictly limited to the named 
> recipient(s) intended by the sender of this message. This email, and any 
> attachments, may contain confidential and/or proprietary information. If 
> you are not a named recipient, any copying, using, disclosing or 
> distributing to others the information in this email and attachments is 
> STRICTLY PROHIBITED. If you have received this email in error, please 
> notify the sender immediately and permanently delete the email, any 
> attachments, and all copies thereof from any drives or storage media and 
> destroy any printouts or hard copies of the email and attachments.
> 
>  
> 
> 
> EXPORT COMPLIANCE NOTICE: This email and any attachments may contain 
> technical data subject to U.S export restrictions under the International 
> Traffic in Arms Regulations (ITAR) or the Export Administration Regulations 
> (EAR). Export or transfer of this technical data and/or related information 
> to any foreign person(s) or entity(ies), either within the U.S. or outside 
> of the U.S., may require advance export authorization by the appropriate 
> U.S. Government agency prior to export or transfer. In addition, technical 
> data may not be exported or transferred to certain countries or specified 
> designated nationals identified by U.S. embargo controls without prior 
> export authorization. By accepting this email and any attachments, all 
> recipients confirm that they understand and will comply with all applicable 
> ITAR, EAR and embargo compliance requirements.

-- 
Regards,
Niklas Söderlund

Re: [PATCH] media: i2c: adv7180: fix adv7280 BT.656-4 compatibility

[Tim Harvey]

On Fri, Sep 27, 2019 at 1:43 PM Niklas Söderlund
<niklas.soderlund@ragnatech.se> wrote:
>
> Hi Tim,
>
> On 2019-09-27 12:26:40 -0700, Tim Harvey wrote:
> > On Fri, Sep 27, 2019 at 12:04 PM Niklas Söderlund
> > <niklas.soderlund@ragnatech.se> wrote:
> > >
> > > Hi Tim,
> > >
> > > Sorry for taking to so long to look at this.
> > >
> > > On 2019-09-23 15:04:47 -0700, Tim Harvey wrote:
> > > > On Thu, Aug 29, 2019 at 7:29 AM Niklas Söderlund
> > > > <niklas.soderlund@ragnatech.se> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > On 2019-08-29 13:43:49 +0200, Hans Verkuil wrote:
> > > > > > Adding Niklas.
> > > > > >
> > > > > > Niklas, can you take a look at this?
> > > > >
> > > > > I'm happy to have a look at this. I'm currently moving so all my boards
> > > > > are in a box somewhere. I hope to have my lab up and running next week,
> > > > > so if this is not urgent I will look at it then.
> > > > >
> > > >
> > > > Niklas,
> > > >
> > > > Have you looked at this yet? Without this patch the ADV7280A does not
> > > > output proper BT.656. We tested this on a Gateworks Ventana GW5404-G
> > > > which uses the ADV7280A connected to the IMX6 CSI parallel bus. I'm
> > > > hoping to see this get merged and perhaps backported to older kernels.
> > >
> > > I only have access to an adv7180 so I was unable to test this patch.
> > > After reviewing the documentation I think the patch is OK if what you
> > > want is to unconditionally switch the driver from outputting BT.656-3 to
> > > outputting BT.656-4.
> > >
> > > As this change would effect a large number of compat strings (adv7280,
> > > adv7280-m, adv7281, adv7281-m, adv7281-ma, adv7282, adv7282-m) and the
> > > goal is to back port it I'm a bit reluctant to adding my tag to this
> > > patch as I'm not sure if this will break other setups.
> > >
> > > From the documentation about the BT.656-4 register (address 0x04 bit 7):
> > >
> > >     Between Revision 3 and Revision 4 of the ITU-R BT.656 standards,
> > >     the ITU has changed the toggling position for the V bit within
> > >     the SAV EAV codes for NTSC. The ITU-R BT.656-4 standard
> > >     bit allows the user to select an output mode that is compliant
> > >     with either the previous or new standard. For further information,
> > >     visit the International Telecommunication Union website.
> > >
> > >     Note that the standard change only affects NTSC and has no
> > >     bearing on PAL.
> > >
> > >     When ITU-R BT.656-4 is 0 (default), the ITU-R BT.656-3
> > >     specification is used. The V bit goes low at EAV of Line 10
> > >     and Line 273.
> > >
> > >     When ITU-R BT.656-4 is 1, the ITU-R BT.656-4 specification is
> > >     used. The V bit goes low at EAV of Line 20 and Line 283.
> > >
> > > Do you know what effects such a change would bring? Looking at the
> > > driver BT.656-4 seems to be set unconditionally for some adv7180 chips.
> > >
> >
> > Niklas,
> >
> > Quite simply, we have a board that has an ADV7180 attached to the
> > parallel CSI of an IMX6 that worked fine with mainline drivers then
> > when we revised this board to attach an ADV7280A in the same way
> > capture failed to sync. Investigation showed that the NEWAVMODE
> > differed between the two.
>
> I understand your problem, the driver configures adv7180 and adv7280
> differently.
>
> >
> > So if the point of the driver is to configure the variants in the same
> > way, this patch needs to be applied.
>
> I'm not sure that is the point of the driver. As the driver today
> configures different compatible strings differently. Some as ITU-R
> BT.656-3 and some as ITU-R BT.656-4, I can only assume there is a reason
> for that.
>
> >
> > I would maintain that the adv7180 comes up with NEWAVMODE enabled and
> > in order to be compatible we must configure the adv7282 the same.
> >
> > The same argument can be made for setting the V bit end position in
> > NTSC mode - its done for the adv7180 so for compatible output it
> > should be done for the adv7282.
>
> I understand that this is needed to make it a drop-in replacement for
> the adv7180 in your use-case. But I'm not sure it is a good idea for
> other users of the driver. What if someone is already using a adv7282 on
> a board and depends on it providing ITU-R BT.656-3 and the old settings?
> If this patch is picked up there use-cases may break.
>
> I'm not sure what the best way forward is I'm afraid. Looking at
> video-interfaces.txt we have a device tree property bus-type which is
> used to describe the bus is a BT.656 bus but not which revision of it.
>
> I'm not really found of driver specific bus descriptions, but maybe this
> is a case where one might consider adding one? Hans what do you think?
>

Niklas / Hans,

Thanks for the feedback. I thought that the goal of any 'compatible'
device should be to be configured identically. If that's not the case
then we need more discussion for sure.

There are 3 registers being changed by this patch which do the
following for the adv7182/adv7280/adv7181/adv7282:
- change output from BT656-3 to BT656-4 (as the driver does this for adv7180)
- enable NEWAVMODE meaning EAV/SAV codes are configurable (new code
but adv7180 enables this by power-on default and adv7280 does not)
- configure V bit end count (to be what adv7180 uses; this isn't used
if NEWAVMODE is disabled)

So its not only the question of how to decide to configure BT656-3 vs
BT656-4 but how to deal with differences in the EAV/SAV codes. I'm not
an expert so I don't know what configuration is BT656 compliant and of
course without knowing who is using these devices we can't tell what
would break even if we fix something that may be misconfigured already
(or even completely unused).

I'm cc'ing Steve Longerbeam as well as at one point he was suggesting
adding a 'newavmode' property to the adv7180 bindings and likely
recalls the discussions there.

Tim

Re: [PATCH] media: i2c: adv7180: fix adv7280 BT.656-4 compatibility

[Steve Longerbeam]

On 10/2/19 2:31 PM, Tim Harvey wrote:
> On Fri, Sep 27, 2019 at 1:43 PM Niklas Söderlund
> <niklas.soderlund@ragnatech.se>  wrote:
>> Hi Tim,
>>
>> On 2019-09-27 12:26:40 -0700, Tim Harvey wrote:
>>> On Fri, Sep 27, 2019 at 12:04 PM Niklas Söderlund
>>> <niklas.soderlund@ragnatech.se>  wrote:
>>>> Hi Tim,
>>>>
>>>> Sorry for taking to so long to look at this.
>>>>
>>>> On 2019-09-23 15:04:47 -0700, Tim Harvey wrote:
>>>>> On Thu, Aug 29, 2019 at 7:29 AM Niklas Söderlund
>>>>> <niklas.soderlund@ragnatech.se>  wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 2019-08-29 13:43:49 +0200, Hans Verkuil wrote:
>>>>>>> Adding Niklas.
>>>>>>>
>>>>>>> Niklas, can you take a look at this?
>>>>>> I'm happy to have a look at this. I'm currently moving so all my boards
>>>>>> are in a box somewhere. I hope to have my lab up and running next week,
>>>>>> so if this is not urgent I will look at it then.
>>>>>>
>>>>> Niklas,
>>>>>
>>>>> Have you looked at this yet? Without this patch the ADV7280A does not
>>>>> output proper BT.656. We tested this on a Gateworks Ventana GW5404-G
>>>>> which uses the ADV7280A connected to the IMX6 CSI parallel bus. I'm
>>>>> hoping to see this get merged and perhaps backported to older kernels.
>>>> I only have access to an adv7180 so I was unable to test this patch.
>>>> After reviewing the documentation I think the patch is OK if what you
>>>> want is to unconditionally switch the driver from outputting BT.656-3 to
>>>> outputting BT.656-4.
>>>>
>>>> As this change would effect a large number of compat strings (adv7280,
>>>> adv7280-m, adv7281, adv7281-m, adv7281-ma, adv7282, adv7282-m) and the
>>>> goal is to back port it I'm a bit reluctant to adding my tag to this
>>>> patch as I'm not sure if this will break other setups.
>>>>
>>>>  From the documentation about the BT.656-4 register (address 0x04 bit 7):
>>>>
>>>>      Between Revision 3 and Revision 4 of the ITU-R BT.656 standards,
>>>>      the ITU has changed the toggling position for the V bit within
>>>>      the SAV EAV codes for NTSC. The ITU-R BT.656-4 standard
>>>>      bit allows the user to select an output mode that is compliant
>>>>      with either the previous or new standard. For further information,
>>>>      visit the International Telecommunication Union website.
>>>>
>>>>      Note that the standard change only affects NTSC and has no
>>>>      bearing on PAL.
>>>>
>>>>      When ITU-R BT.656-4 is 0 (default), the ITU-R BT.656-3
>>>>      specification is used. The V bit goes low at EAV of Line 10
>>>>      and Line 273.
>>>>
>>>>      When ITU-R BT.656-4 is 1, the ITU-R BT.656-4 specification is
>>>>      used. The V bit goes low at EAV of Line 20 and Line 283.
>>>>
>>>> Do you know what effects such a change would bring? Looking at the
>>>> driver BT.656-4 seems to be set unconditionally for some adv7180 chips.
>>>>
>>> Niklas,
>>>
>>> Quite simply, we have a board that has an ADV7180 attached to the
>>> parallel CSI of an IMX6 that worked fine with mainline drivers then
>>> when we revised this board to attach an ADV7280A in the same way
>>> capture failed to sync. Investigation showed that the NEWAVMODE
>>> differed between the two.
>> I understand your problem, the driver configures adv7180 and adv7280
>> differently.
>>
>>> So if the point of the driver is to configure the variants in the same
>>> way, this patch needs to be applied.
>> I'm not sure that is the point of the driver. As the driver today
>> configures different compatible strings differently. Some as ITU-R
>> BT.656-3 and some as ITU-R BT.656-4, I can only assume there is a reason
>> for that.
>>
>>> I would maintain that the adv7180 comes up with NEWAVMODE enabled and
>>> in order to be compatible we must configure the adv7282 the same.
>>>
>>> The same argument can be made for setting the V bit end position in
>>> NTSC mode - its done for the adv7180 so for compatible output it
>>> should be done for the adv7282.
>> I understand that this is needed to make it a drop-in replacement for
>> the adv7180 in your use-case. But I'm not sure it is a good idea for
>> other users of the driver. What if someone is already using a adv7282 on
>> a board and depends on it providing ITU-R BT.656-3 and the old settings?
>> If this patch is picked up there use-cases may break.
>>
>> I'm not sure what the best way forward is I'm afraid. Looking at
>> video-interfaces.txt we have a device tree property bus-type which is
>> used to describe the bus is a BT.656 bus but not which revision of it.
>>
>> I'm not really found of driver specific bus descriptions, but maybe this
>> is a case where one might consider adding one? Hans what do you think?
>>
> Niklas / Hans,
>
> Thanks for the feedback. I thought that the goal of any 'compatible'
> device should be to be configured identically. If that's not the case
> then we need more discussion for sure.
>
> There are 3 registers being changed by this patch which do the
> following for the adv7182/adv7280/adv7181/adv7282:
> - change output from BT656-3 to BT656-4 (as the driver does this for adv7180)
> - enable NEWAVMODE meaning EAV/SAV codes are configurable (new code
> but adv7180 enables this by power-on default and adv7280 does not)
> - configure V bit end count (to be what adv7180 uses; this isn't used
> if NEWAVMODE is disabled)
>
> So its not only the question of how to decide to configure BT656-3 vs
> BT656-4 but how to deal with differences in the EAV/SAV codes. I'm not
> an expert so I don't know what configuration is BT656 compliant and of
> course without knowing who is using these devices we can't tell what
> would break even if we fix something that may be misconfigured already
> (or even completely unused).
>
> I'm cc'ing Steve Longerbeam as well as at one point he was suggesting
> adding a 'newavmode' property to the adv7180 bindings and likely
> recalls the discussions there.

I've never understood why the adv7180 driver configures a non-standard 
V-bit end position (ADV7180_NTSC_V_BIT_END_MANUAL_NVEND), maybe because 
the driver was introduced along with a new bridge driver that assumes 
that V-bit position. The parallel CSI interface in imx6 driver is also 
configuring its crop window to work with this non-standard V-bit position.

The most straight-forward approach to decouple these adv7180 
dependencies would be to remove the non-standard V-bit setting in the 
adv7180 driver, and it should output standard bt.656-3 or bt.656-4 
SAV/EAV codes for all compatible chips. Then expand on the bus-type DT 
bindings to distinguish between bt.656-3 and bt.656-4. And finally all 
bridge drivers that use adv7180 would have to be fixed to work only with 
standard bt.656-3/4 codes. But I realize that last part isn't so easy, 
and it's even possible some bridge drivers may not be able to cope with 
the standard V-bit positions.


Steve