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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> 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@xxxxxxxxx>
> > > > > >
> > > > > > 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