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

2011-09-21 Thread Sylwester Nawrocki
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,
-- 
Sylwester Nawrocki
Samsung Poland RD Center
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2011-09-21 Thread Sylwester Nawrocki
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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2011-09-20 Thread Laurent Pinchart
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.nawro...@samsung.com
 Signed-off-by: Kyungmin Park kyungmin.p...@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 */

-- 
Regards,

Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html