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