Re: [PATCH v3 1/2] v4l2: Add the polarity flags for parallel camera bus FIELD signal

[Sylwester Nawrocki <s.nawrocki@samsung.com>]

On 09/21/2011 03:24 PM, Sylwester Nawrocki wrote:
> Hi Laurent,
> 
> On 09/21/2011 01:12 AM, Laurent Pinchart wrote:
>> Hi Sylwester,
>>
>> Thanks for the patch.
>>
>> On Monday 19 September 2011 19:07:55 Sylwester Nawrocki wrote:
>>> FIELD is an Even/Odd field selection signal, as specified in ITU-R BT.601
>>> standard. Add corresponding flag for configuring the FIELD signal polarity.
>>> Also add a comment about usage of V4L2_MBUS_[HV]SYNC* flags for the
>>> hardware that uses [HV]REF signals.
>>
>> I like this approach better.
>>
> ...
>>> +/* Field selection signal for interlaced scan mode */
>>> +#define V4L2_MBUS_FIELD_ACTIVE_HIGH		(1 << 10)
>>> +#define V4L2_MBUS_FIELD_ACTIVE_LOW		(1 << 11)
>>
>> What does this mean ? The FIELD signal is used to select between odd and even 
>> fields. Does "active high" mean that the field is odd or even when the signal 
>> has a high level ? The comment should make it explicit, or we could even 
>> rename those two constants to FIELD_ODD_HIGH/FIELD_ODD_LOW (or 
>> FIELD_EVEN_HIGH/FIELD_EVEN_LOW).
> 
> Yes, certainly I didn't think enough about this. I silently assumed that for
> V4L2_MBUS_FIELD_ACTIVE_HIGH FIELD = 0 selects Field1 (odd) and FIELD = 1 selects
> Field2 (even).
> I think it would be good to construct the macro so it is possibly self-explanatory,
> rather than requiring often to dig in the documentation.
> 
> So I would go for V4L2_MBUS_FIELD_ODD_LOW/V4L2_MBUS_FIELD_ODD_HIGH.
> Unless someone proposes something different/better I'll send an amended version
> tomorrow. 

Thinking some more of it, V4L2_MBUS_FIELD_EVEN_HIGH/V4L2_MBUS_FIELD_EVEN_LOW
is perhaps more in line with other defines where *HIGH means standard,
non-inverted case. So it seems better to me.