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

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.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
Resending with proper bit assignment.

---
 include/media/v4l2-mediabus.h |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)
  

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.

> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> Resending with proper bit assignment.
> 
> ---
>  include/media/v4l2-mediabus.h |   11 +++++++++--
>  1 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
> index 6114007..f3a61ab 100644
> --- a/include/media/v4l2-mediabus.h
> +++ b/include/media/v4l2-mediabus.h
> @@ -22,8 +22,12 @@
>   */
>  #define V4L2_MBUS_MASTER			(1 << 0)
>  #define V4L2_MBUS_SLAVE				(1 << 1)
> -/* Which signal polarities it supports */
> -/* Note: in BT.656 mode HSYNC and VSYNC are unused */
> +/*
> + * Signal polarity flags
> + * Note: in BT.656 mode HSYNC, FIELD, and VSYNC are unused
> + * V4L2_MBUS_[HV]SYNC_* flags should be also used for specifying
> + * configuration of hardware that uses [HV]REF signals
> + */
>  #define V4L2_MBUS_HSYNC_ACTIVE_HIGH		(1 << 2)
>  #define V4L2_MBUS_HSYNC_ACTIVE_LOW		(1 << 3)
>  #define V4L2_MBUS_VSYNC_ACTIVE_HIGH		(1 << 4)
> @@ -32,6 +36,9 @@
>  #define V4L2_MBUS_PCLK_SAMPLE_FALLING		(1 << 7)
>  #define V4L2_MBUS_DATA_ACTIVE_HIGH		(1 << 8)
>  #define V4L2_MBUS_DATA_ACTIVE_LOW		(1 << 9)
> +/* 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).

>  /* Serial flags */
>  /* How many lanes the client can use */
  

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. 


Thanks,
  

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html