[PATCH 0/2] v4l: Add media bus polarity flags for HREF signal

[PATCH 0/2] v4l: Add media bus polarity flags for HREF signal

[Sylwester Nawrocki]

Hello,

The following patche adds support for HREF signal polarity configuration
through the parallel media bus flags.
The second one just converts s5p-fimc driver to use generic flags.


Sylwester Nawrocki (2):
  v4l2: Add the parallel bus HREF signal polarity flags
  s5p-fimc: Convert to use generic bus polarity flags

 drivers/media/video/s5p-fimc/fimc-reg.c |    8 ++++----
 include/media/s5p_fimc.h                |    7 +------
 include/media/v4l2-mediabus.h           |   14 ++++++++------
 3 files changed, 13 insertions(+), 16 deletions(-)


Thanks,
--
Sylwester Nawrocki
Samsung Poland R&D Center

[PATCH/RFC 1/2] v4l2: Add the parallel bus HREF signal polarity flags

[Sylwester Nawrocki]

HREF is a signal indicating valid data during single line transmission.
Add corresponding flags for this signal to the set of mediabus signal
polarity flags.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 include/media/v4l2-mediabus.h |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
index 6114007..41d8771 100644
--- a/include/media/v4l2-mediabus.h
+++ b/include/media/v4l2-mediabus.h
@@ -26,12 +26,14 @@
 /* Note: in BT.656 mode HSYNC and VSYNC are unused */
 #define V4L2_MBUS_HSYNC_ACTIVE_HIGH		(1 << 2)
 #define V4L2_MBUS_HSYNC_ACTIVE_LOW		(1 << 3)
-#define V4L2_MBUS_VSYNC_ACTIVE_HIGH		(1 << 4)
-#define V4L2_MBUS_VSYNC_ACTIVE_LOW		(1 << 5)
-#define V4L2_MBUS_PCLK_SAMPLE_RISING		(1 << 6)
-#define V4L2_MBUS_PCLK_SAMPLE_FALLING		(1 << 7)
-#define V4L2_MBUS_DATA_ACTIVE_HIGH		(1 << 8)
-#define V4L2_MBUS_DATA_ACTIVE_LOW		(1 << 9)
+#define V4L2_MBUS_HREF_ACTIVE_HIGH		(1 << 4)
+#define V4L2_MBUS_HREF_ACTIVE_LOW		(1 << 5)
+#define V4L2_MBUS_VSYNC_ACTIVE_HIGH		(1 << 6)
+#define V4L2_MBUS_VSYNC_ACTIVE_LOW		(1 << 7)
+#define V4L2_MBUS_PCLK_SAMPLE_RISING		(1 << 8)
+#define V4L2_MBUS_PCLK_SAMPLE_FALLING		(1 << 9)
+#define V4L2_MBUS_DATA_ACTIVE_HIGH		(1 << 10)
+#define V4L2_MBUS_DATA_ACTIVE_LOW		(1 << 11)
 
 /* Serial flags */
 /* How many lanes the client can use */
-- 
1.7.6

[PATCH 2/2] s5p-fimc: Convert to use generic bus polarity flags

[Sylwester Nawrocki]

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <s.nawrocki@samsung.com>
---
 drivers/media/video/s5p-fimc/fimc-reg.c |    8 ++++----
 include/media/s5p_fimc.h                |    7 +------
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/media/video/s5p-fimc/fimc-reg.c b/drivers/media/video/s5p-fimc/fimc-reg.c
index 2a1ae51..5543b1b 100644
--- a/drivers/media/video/s5p-fimc/fimc-reg.c
+++ b/drivers/media/video/s5p-fimc/fimc-reg.c
@@ -535,16 +535,16 @@ int fimc_hw_set_camera_polarity(struct fimc_dev *fimc,
 	cfg &= ~(S5P_CIGCTRL_INVPOLPCLK | S5P_CIGCTRL_INVPOLVSYNC |
 		 S5P_CIGCTRL_INVPOLHREF | S5P_CIGCTRL_INVPOLHSYNC);
 
-	if (cam->flags & FIMC_CLK_INV_PCLK)
+	if (cam->flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
 		cfg |= S5P_CIGCTRL_INVPOLPCLK;
 
-	if (cam->flags & FIMC_CLK_INV_VSYNC)
+	if (cam->flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
 		cfg |= S5P_CIGCTRL_INVPOLVSYNC;
 
-	if (cam->flags & FIMC_CLK_INV_HREF)
+	if (cam->flags & V4L2_MBUS_HREF_ACTIVE_LOW)
 		cfg |= S5P_CIGCTRL_INVPOLHREF;
 
-	if (cam->flags & FIMC_CLK_INV_HSYNC)
+	if (cam->flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)
 		cfg |= S5P_CIGCTRL_INVPOLHSYNC;
 
 	writel(cfg, fimc->regs + S5P_CIGCTRL);
diff --git a/include/media/s5p_fimc.h b/include/media/s5p_fimc.h
index 2b58904..688fb3f 100644
--- a/include/media/s5p_fimc.h
+++ b/include/media/s5p_fimc.h
@@ -19,11 +19,6 @@ enum cam_bus_type {
 	FIMC_LCD_WB, /* FIFO link from LCD mixer */
 };
 
-#define FIMC_CLK_INV_PCLK	(1 << 0)
-#define FIMC_CLK_INV_VSYNC	(1 << 1)
-#define FIMC_CLK_INV_HREF	(1 << 2)
-#define FIMC_CLK_INV_HSYNC	(1 << 3)
-
 struct i2c_board_info;
 
 /**
@@ -37,7 +32,7 @@ struct i2c_board_info;
  * @i2c_bus_num: i2c control bus id the sensor is attached to
  * @mux_id: FIMC camera interface multiplexer index (separate for MIPI and ITU)
  * @clk_id: index of the SoC peripheral clock for sensors
- * @flags: flags defining bus signals polarity inversion (High by default)
+ * @flags: the parallel bus flags defining signals polarity (V4L2_MBUS_*)
  */
 struct s5p_fimc_isp_info {
 	struct i2c_board_info *board_info;
-- 
1.7.6

Re: [PATCH/RFC 1/2] v4l2: Add the parallel bus HREF signal polarity flags

[Laurent Pinchart]

Hi Sylwester,

On Friday 16 September 2011 19:28:42 Sylwester Nawrocki wrote:
> HREF is a signal indicating valid data during single line transmission.
> Add corresponding flags for this signal to the set of mediabus signal
> polarity flags.

So that's a data valid signal that gates the pixel data ? The OMAP3 ISP has a 
similar signal called WEN, and I've seen other chips using DVAL. Your patch 
looks good to me, except maybe for the signal name that could be made a bit 
more explicit (I'm not sure what most chips use though).

> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  include/media/v4l2-mediabus.h |   14 ++++++++------
>  1 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
> index 6114007..41d8771 100644
> --- a/include/media/v4l2-mediabus.h
> +++ b/include/media/v4l2-mediabus.h
> @@ -26,12 +26,14 @@
>  /* Note: in BT.656 mode HSYNC and VSYNC are unused */
>  #define V4L2_MBUS_HSYNC_ACTIVE_HIGH		(1 << 2)
>  #define V4L2_MBUS_HSYNC_ACTIVE_LOW		(1 << 3)
> -#define V4L2_MBUS_VSYNC_ACTIVE_HIGH		(1 << 4)
> -#define V4L2_MBUS_VSYNC_ACTIVE_LOW		(1 << 5)
> -#define V4L2_MBUS_PCLK_SAMPLE_RISING		(1 << 6)
> -#define V4L2_MBUS_PCLK_SAMPLE_FALLING		(1 << 7)
> -#define V4L2_MBUS_DATA_ACTIVE_HIGH		(1 << 8)
> -#define V4L2_MBUS_DATA_ACTIVE_LOW		(1 << 9)
> +#define V4L2_MBUS_HREF_ACTIVE_HIGH		(1 << 4)
> +#define V4L2_MBUS_HREF_ACTIVE_LOW		(1 << 5)
> +#define V4L2_MBUS_VSYNC_ACTIVE_HIGH		(1 << 6)
> +#define V4L2_MBUS_VSYNC_ACTIVE_LOW		(1 << 7)
> +#define V4L2_MBUS_PCLK_SAMPLE_RISING		(1 << 8)
> +#define V4L2_MBUS_PCLK_SAMPLE_FALLING		(1 << 9)
> +#define V4L2_MBUS_DATA_ACTIVE_HIGH		(1 << 10)
> +#define V4L2_MBUS_DATA_ACTIVE_LOW		(1 << 11)
> 
>  /* Serial flags */
>  /* How many lanes the client can use */

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC 1/2] v4l2: Add the parallel bus HREF signal polarity flags

[Sylwester Nawrocki]

Hi Laurent,

thanks for your comments.

On 09/17/2011 12:54 PM, Laurent Pinchart wrote:
> Hi Sylwester,
> 
> On Friday 16 September 2011 19:28:42 Sylwester Nawrocki wrote:
>> HREF is a signal indicating valid data during single line transmission.
>> Add corresponding flags for this signal to the set of mediabus signal
>> polarity flags.
> 
> So that's a data valid signal that gates the pixel data ? The OMAP3 ISP has a

Yes, it's "horizontal window reference" signal, it's well described in this datasheet:
http://www.morninghan.com/pdf/OV2640FSL_DS_(1_3).pdf

AFAICS there can be also its vertical counterpart - VREF.

Many devices seem to use this terminology. However, I realize, not all, as you're
pointing out. So perhaps it's time for some naming contest now.. :-)

> similar signal called WEN, and I've seen other chips using DVAL. Your patch
> looks good to me, except maybe for the signal name that could be made a bit
> more explicit (I'm not sure what most chips use though).

I'm pretty OK with HREF/VREF. But I'm open to any better suggestions.

Maybe 

V4L2_MBUS_LINE_VALID_ACTIVE_HIGH
V4L2_MBUS_LINE_VALID_ACTIVE_LOW

V4L2_MBUS_FRAME_VALID_ACTIVE_HIGH
V4L2_MBUS_FRAME_VALID_ACTIVE_LOW
	
? 
Some of Aptina sensor datasheets describes those signals as LINE_VALID/FRAME_VALID,
(www.aptina.com/assets/downloadDocument.do?id=76).

> 
>> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>> ---
>>   include/media/v4l2-mediabus.h |   14 ++++++++------
>>   1 files changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
>> index 6114007..41d8771 100644
>> --- a/include/media/v4l2-mediabus.h
>> +++ b/include/media/v4l2-mediabus.h
>> @@ -26,12 +26,14 @@
>>   /* Note: in BT.656 mode HSYNC and VSYNC are unused */

I've forgotten to update this:

/* Note: in BT.656 mode HSYNC, HREF and VSYNC are unused */

>>   #define V4L2_MBUS_HSYNC_ACTIVE_HIGH		(1<<  2)
>>   #define V4L2_MBUS_HSYNC_ACTIVE_LOW		(1<<  3)
>> -#define V4L2_MBUS_VSYNC_ACTIVE_HIGH		(1<<  4)
>> -#define V4L2_MBUS_VSYNC_ACTIVE_LOW		(1<<  5)
>> -#define V4L2_MBUS_PCLK_SAMPLE_RISING		(1<<  6)
>> -#define V4L2_MBUS_PCLK_SAMPLE_FALLING		(1<<  7)
>> -#define V4L2_MBUS_DATA_ACTIVE_HIGH		(1<<  8)
>> -#define V4L2_MBUS_DATA_ACTIVE_LOW		(1<<  9)
>> +#define V4L2_MBUS_HREF_ACTIVE_HIGH		(1<<  4)
>> +#define V4L2_MBUS_HREF_ACTIVE_LOW		(1<<  5)
>> +#define V4L2_MBUS_VSYNC_ACTIVE_HIGH		(1<<  6)
>> +#define V4L2_MBUS_VSYNC_ACTIVE_LOW		(1<<  7)
>> +#define V4L2_MBUS_PCLK_SAMPLE_RISING		(1<<  8)
>> +#define V4L2_MBUS_PCLK_SAMPLE_FALLING		(1<<  9)
>> +#define V4L2_MBUS_DATA_ACTIVE_HIGH		(1<<  10)
>> +#define V4L2_MBUS_DATA_ACTIVE_LOW		(1<<  11)

--
Thanks,
Sylwester

Re: [PATCH/RFC 1/2] v4l2: Add the parallel bus HREF signal polarity flags

[Guennadi Liakhovetski]

On Sat, 17 Sep 2011, Sylwester Nawrocki wrote:

> Hi Laurent,
> 
> thanks for your comments.
> 
> On 09/17/2011 12:54 PM, Laurent Pinchart wrote:
> > Hi Sylwester,
> > 
> > On Friday 16 September 2011 19:28:42 Sylwester Nawrocki wrote:
> >> HREF is a signal indicating valid data during single line transmission.
> >> Add corresponding flags for this signal to the set of mediabus signal
> >> polarity flags.
> > 
> > So that's a data valid signal that gates the pixel data ? The OMAP3 ISP has a
> 
> Yes, it's "horizontal window reference" signal, it's well described in this datasheet:
> http://www.morninghan.com/pdf/OV2640FSL_DS_(1_3).pdf
> 
> AFAICS there can be also its vertical counterpart - VREF.
> 
> Many devices seem to use this terminology. However, I realize, not all, as you're
> pointing out. So perhaps it's time for some naming contest now.. :-)

Hi

No objections in principle, just one question though: can these signals 
actually be used simultaneously with respective *SYNC signals or only as 
an alternative? If the latter, maybe we could reuse same names by just 
making them more generic?

Thanks
Guennadi

> > similar signal called WEN, and I've seen other chips using DVAL. Your patch
> > looks good to me, except maybe for the signal name that could be made a bit
> > more explicit (I'm not sure what most chips use though).
> 
> I'm pretty OK with HREF/VREF. But I'm open to any better suggestions.
> 
> Maybe 
> 
> V4L2_MBUS_LINE_VALID_ACTIVE_HIGH
> V4L2_MBUS_LINE_VALID_ACTIVE_LOW
> 
> V4L2_MBUS_FRAME_VALID_ACTIVE_HIGH
> V4L2_MBUS_FRAME_VALID_ACTIVE_LOW
> 	
> ? 
> Some of Aptina sensor datasheets describes those signals as LINE_VALID/FRAME_VALID,
> (www.aptina.com/assets/downloadDocument.do?id=76).
> 
> > 
> >> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
> >> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> >> ---
> >>   include/media/v4l2-mediabus.h |   14 ++++++++------
> >>   1 files changed, 8 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
> >> index 6114007..41d8771 100644
> >> --- a/include/media/v4l2-mediabus.h
> >> +++ b/include/media/v4l2-mediabus.h
> >> @@ -26,12 +26,14 @@
> >>   /* Note: in BT.656 mode HSYNC and VSYNC are unused */
> 
> I've forgotten to update this:
> 
> /* Note: in BT.656 mode HSYNC, HREF and VSYNC are unused */
> 
> >>   #define V4L2_MBUS_HSYNC_ACTIVE_HIGH		(1<<  2)
> >>   #define V4L2_MBUS_HSYNC_ACTIVE_LOW		(1<<  3)
> >> -#define V4L2_MBUS_VSYNC_ACTIVE_HIGH		(1<<  4)
> >> -#define V4L2_MBUS_VSYNC_ACTIVE_LOW		(1<<  5)
> >> -#define V4L2_MBUS_PCLK_SAMPLE_RISING		(1<<  6)
> >> -#define V4L2_MBUS_PCLK_SAMPLE_FALLING		(1<<  7)
> >> -#define V4L2_MBUS_DATA_ACTIVE_HIGH		(1<<  8)
> >> -#define V4L2_MBUS_DATA_ACTIVE_LOW		(1<<  9)
> >> +#define V4L2_MBUS_HREF_ACTIVE_HIGH		(1<<  4)
> >> +#define V4L2_MBUS_HREF_ACTIVE_LOW		(1<<  5)
> >> +#define V4L2_MBUS_VSYNC_ACTIVE_HIGH		(1<<  6)
> >> +#define V4L2_MBUS_VSYNC_ACTIVE_LOW		(1<<  7)
> >> +#define V4L2_MBUS_PCLK_SAMPLE_RISING		(1<<  8)
> >> +#define V4L2_MBUS_PCLK_SAMPLE_FALLING		(1<<  9)
> >> +#define V4L2_MBUS_DATA_ACTIVE_HIGH		(1<<  10)
> >> +#define V4L2_MBUS_DATA_ACTIVE_LOW		(1<<  11)
> 
> --
> Thanks,
> Sylwester
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH/RFC 1/2] v4l2: Add the parallel bus HREF signal polarity flags

[Sylwester Nawrocki]

Hi Guennadi,

On 09/17/2011 02:34 PM, Guennadi Liakhovetski wrote:
> On Sat, 17 Sep 2011, Sylwester Nawrocki wrote:
> 
>> Hi Laurent,
>>
>> thanks for your comments.
>>
>> On 09/17/2011 12:54 PM, Laurent Pinchart wrote:
>>> Hi Sylwester,
>>>
>>> On Friday 16 September 2011 19:28:42 Sylwester Nawrocki wrote:
>>>> HREF is a signal indicating valid data during single line transmission.
>>>> Add corresponding flags for this signal to the set of mediabus signal
>>>> polarity flags.
>>>
>>> So that's a data valid signal that gates the pixel data ? The OMAP3 ISP has a
>>
>> Yes, it's "horizontal window reference" signal, it's well described in this datasheet:
>> http://www.morninghan.com/pdf/OV2640FSL_DS_(1_3).pdf
>>
>> AFAICS there can be also its vertical counterpart - VREF.
>>
>> Many devices seem to use this terminology. However, I realize, not all, as you're
>> pointing out. So perhaps it's time for some naming contest now.. :-)
> 
> Hi
> 
> No objections in principle, just one question though: can these signals
> actually be used simultaneously with respective *SYNC signals or only as
> an alternative? If the latter, maybe we could reuse same names by just
> making them more generic?

That's actually a good question. In my use cases only HREF is used as horizontal
synchronization signal, i.e. physical bus interface has this signals:

->| PCLK
->| VSYNC
->| HREF
->| DATA[0:7]
->| FIELD

For interlaced mode FIELD can be connected to the horizontal synchronization
signal. For this case there is InvPolHSYNC bit in the host interface registers
to indicate the polarity. There are 5 bits actually:

InvPolPCLK 
InvPolVSYNC (vertical sychronization)
InvPolHREF  (horizontal synchronization)
InvPolHSYNC (for interlaced mode only, FIELD port = horizontal sync. signal)
InvPolFIELD (interlaced mode,  FIELD port = FIELD signal)

IMHO keeping different names for synchronization and 'data valid' signals is more
clear.

> 
> Thanks
> Guennadi
> 
>>> similar signal called WEN, and I've seen other chips using DVAL. Your patch
>>> looks good to me, except maybe for the signal name that could be made a bit
>>> more explicit (I'm not sure what most chips use though).
>>
>> I'm pretty OK with HREF/VREF. But I'm open to any better suggestions.
>>
>> Maybe
>>
>> V4L2_MBUS_LINE_VALID_ACTIVE_HIGH
>> V4L2_MBUS_LINE_VALID_ACTIVE_LOW
>>
>> V4L2_MBUS_FRAME_VALID_ACTIVE_HIGH
>> V4L2_MBUS_FRAME_VALID_ACTIVE_LOW
>> 	
>> ?
>> Some of Aptina sensor datasheets describes those signals as LINE_VALID/FRAME_VALID,
>> (www.aptina.com/assets/downloadDocument.do?id=76).
>>
>>>
>>>> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
>>>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>>>> ---
>>>>    include/media/v4l2-mediabus.h |   14 ++++++++------
>>>>    1 files changed, 8 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
>>>> index 6114007..41d8771 100644
>>>> --- a/include/media/v4l2-mediabus.h
>>>> +++ b/include/media/v4l2-mediabus.h
>>>> @@ -26,12 +26,14 @@
>>>>    /* Note: in BT.656 mode HSYNC and VSYNC are unused */
>>
>> I've forgotten to update this:
>>
>> /* Note: in BT.656 mode HSYNC, HREF and VSYNC are unused */
>>
>>>>    #define V4L2_MBUS_HSYNC_ACTIVE_HIGH		(1<<   2)
>>>>    #define V4L2_MBUS_HSYNC_ACTIVE_LOW		(1<<   3)
>>>> -#define V4L2_MBUS_VSYNC_ACTIVE_HIGH		(1<<   4)
>>>> -#define V4L2_MBUS_VSYNC_ACTIVE_LOW		(1<<   5)
>>>> -#define V4L2_MBUS_PCLK_SAMPLE_RISING		(1<<   6)
>>>> -#define V4L2_MBUS_PCLK_SAMPLE_FALLING		(1<<   7)
>>>> -#define V4L2_MBUS_DATA_ACTIVE_HIGH		(1<<   8)
>>>> -#define V4L2_MBUS_DATA_ACTIVE_LOW		(1<<   9)
>>>> +#define V4L2_MBUS_HREF_ACTIVE_HIGH		(1<<   4)
>>>> +#define V4L2_MBUS_HREF_ACTIVE_LOW		(1<<   5)
>>>> +#define V4L2_MBUS_VSYNC_ACTIVE_HIGH		(1<<   6)
>>>> +#define V4L2_MBUS_VSYNC_ACTIVE_LOW		(1<<   7)
>>>> +#define V4L2_MBUS_PCLK_SAMPLE_RISING		(1<<   8)
>>>> +#define V4L2_MBUS_PCLK_SAMPLE_FALLING		(1<<   9)
>>>> +#define V4L2_MBUS_DATA_ACTIVE_HIGH		(1<<   10)
>>>> +#define V4L2_MBUS_DATA_ACTIVE_LOW		(1<<   11)

--
Regards,
Sylwester

Re: [PATCH/RFC 1/2] v4l2: Add the parallel bus HREF signal polarity flags

[Laurent Pinchart]

Hi Sylwester,

On Saturday 17 September 2011 14:07:30 Sylwester Nawrocki wrote:
> On 09/17/2011 12:54 PM, Laurent Pinchart wrote:
> > On Friday 16 September 2011 19:28:42 Sylwester Nawrocki wrote:
> >> HREF is a signal indicating valid data during single line transmission.
> >> Add corresponding flags for this signal to the set of mediabus signal
> >> polarity flags.
> > 
> > So that's a data valid signal that gates the pixel data ? The OMAP3 ISP
> > has a
> 
> Yes, it's "horizontal window reference" signal, it's well described in this
> datasheet: http://www.morninghan.com/pdf/OV2640FSL_DS_(1_3).pdf

In that specific case I would likely connect to HREF signal to the ISP HSYNC 
signal and ignore the sensor HSYNC signal completely :-)

> AFAICS there can be also its vertical counterpart - VREF.

OK, your HREF signal is thus completely unrelated to my DVAL signal. DVAL 
really qualifies pixel. For instance, if the sensor outputs pixels at half the 
pixel rate, DVAL would switch at every pixel clock cycle during a line.

> Many devices seem to use this terminology. However, I realize, not all, as
> you're pointing out. So perhaps it's time for some naming contest now..
> :-)
> 
> > similar signal called WEN, and I've seen other chips using DVAL. Your
> > patch looks good to me, except maybe for the signal name that could be
> > made a bit more explicit (I'm not sure what most chips use though).
> 
> I'm pretty OK with HREF/VREF. But I'm open to any better suggestions.
> 
> Maybe
> 
> V4L2_MBUS_LINE_VALID_ACTIVE_HIGH
> V4L2_MBUS_LINE_VALID_ACTIVE_LOW
> 
> V4L2_MBUS_FRAME_VALID_ACTIVE_HIGH
> V4L2_MBUS_FRAME_VALID_ACTIVE_LOW
> 
> ?
> Some of Aptina sensor datasheets describes those signals as
> LINE_VALID/FRAME_VALID, (www.aptina.com/assets/downloadDocument.do?id=76).

LINE_VALID/FRAME_VALID are HSYNC/VSYNC.

> >> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
> >> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> >> ---
> >> 
> >>   include/media/v4l2-mediabus.h |   14 ++++++++------
> >>   1 files changed, 8 insertions(+), 6 deletions(-)
> >> 
> >> diff --git a/include/media/v4l2-mediabus.h
> >> b/include/media/v4l2-mediabus.h index 6114007..41d8771 100644
> >> --- a/include/media/v4l2-mediabus.h
> >> +++ b/include/media/v4l2-mediabus.h
> >> @@ -26,12 +26,14 @@
> >> 
> >>   /* Note: in BT.656 mode HSYNC and VSYNC are unused */
> 
> I've forgotten to update this:
> 
> /* Note: in BT.656 mode HSYNC, HREF and VSYNC are unused */
> 
> >>   #define V4L2_MBUS_HSYNC_ACTIVE_HIGH		(1<<  2)
> >>   #define V4L2_MBUS_HSYNC_ACTIVE_LOW		(1<<  3)
> >> 
> >> -#define V4L2_MBUS_VSYNC_ACTIVE_HIGH		(1<<  4)
> >> -#define V4L2_MBUS_VSYNC_ACTIVE_LOW		(1<<  5)
> >> -#define V4L2_MBUS_PCLK_SAMPLE_RISING		(1<<  6)
> >> -#define V4L2_MBUS_PCLK_SAMPLE_FALLING		(1<<  7)
> >> -#define V4L2_MBUS_DATA_ACTIVE_HIGH		(1<<  8)
> >> -#define V4L2_MBUS_DATA_ACTIVE_LOW		(1<<  9)
> >> +#define V4L2_MBUS_HREF_ACTIVE_HIGH		(1<<  4)
> >> +#define V4L2_MBUS_HREF_ACTIVE_LOW		(1<<  5)
> >> +#define V4L2_MBUS_VSYNC_ACTIVE_HIGH		(1<<  6)
> >> +#define V4L2_MBUS_VSYNC_ACTIVE_LOW		(1<<  7)
> >> +#define V4L2_MBUS_PCLK_SAMPLE_RISING		(1<<  8)
> >> +#define V4L2_MBUS_PCLK_SAMPLE_FALLING		(1<<  9)
> >> +#define V4L2_MBUS_DATA_ACTIVE_HIGH		(1<<  10)
> >> +#define V4L2_MBUS_DATA_ACTIVE_LOW		(1<<  11)

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC 1/2] v4l2: Add the parallel bus HREF signal polarity flags

[Laurent Pinchart]

Hi Sylwester,

On Saturday 17 September 2011 18:06:20 Sylwester Nawrocki wrote:
> On 09/17/2011 02:34 PM, Guennadi Liakhovetski wrote:
> > On Sat, 17 Sep 2011, Sylwester Nawrocki wrote:
> >> On 09/17/2011 12:54 PM, Laurent Pinchart wrote:
> >>> On Friday 16 September 2011 19:28:42 Sylwester Nawrocki wrote:
> >>>> HREF is a signal indicating valid data during single line
> >>>> transmission. Add corresponding flags for this signal to the set of
> >>>> mediabus signal polarity flags.
> >>> 
> >>> So that's a data valid signal that gates the pixel data ? The OMAP3 ISP
> >>> has a
> >> 
> >> Yes, it's "horizontal window reference" signal, it's well described in
> >> this datasheet: http://www.morninghan.com/pdf/OV2640FSL_DS_(1_3).pdf
> >> 
> >> AFAICS there can be also its vertical counterpart - VREF.
> >> 
> >> Many devices seem to use this terminology. However, I realize, not all,
> >> as you're pointing out. So perhaps it's time for some naming contest
> >> now.. :-)
> > 
> > No objections in principle, just one question though: can these signals
> > actually be used simultaneously with respective *SYNC signals or only as
> > an alternative? If the latter, maybe we could reuse same names by just
> > making them more generic?
> 
> That's actually a good question. In my use cases only HREF is used as
> horizontal synchronization signal, i.e. physical bus interface has this
> signals:
> 
> ->| PCLK
> ->| VSYNC
> ->| HREF
> ->| DATA[0:7]
> ->| FIELD
> 
> For interlaced mode FIELD can be connected to the horizontal
> synchronization signal. For this case there is InvPolHSYNC bit in the host
> interface registers to indicate the polarity. There are 5 bits actually:
> 
> InvPolPCLK
> InvPolVSYNC (vertical sychronization)
> InvPolHREF  (horizontal synchronization)
> InvPolHSYNC (for interlaced mode only, FIELD port = horizontal sync.
> signal) InvPolFIELD (interlaced mode,  FIELD port = FIELD signal)

Shouldn't this be handled through platform data only ?

> IMHO keeping different names for synchronization and 'data valid' signals
> is more clear.
>
> >>> similar signal called WEN, and I've seen other chips using DVAL. Your
> >>> patch looks good to me, except maybe for the signal name that could be
> >>> made a bit more explicit (I'm not sure what most chips use though).
> >> 
> >> I'm pretty OK with HREF/VREF. But I'm open to any better suggestions.
> >> 
> >> Maybe
> >> 
> >> V4L2_MBUS_LINE_VALID_ACTIVE_HIGH
> >> V4L2_MBUS_LINE_VALID_ACTIVE_LOW
> >> 
> >> V4L2_MBUS_FRAME_VALID_ACTIVE_HIGH
> >> V4L2_MBUS_FRAME_VALID_ACTIVE_LOW
> >> 
> >> ?
> >> Some of Aptina sensor datasheets describes those signals as
> >> LINE_VALID/FRAME_VALID,
> >> (www.aptina.com/assets/downloadDocument.do?id=76).
> >> 
> >>>> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
> >>>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> >>>> ---
> >>>> 
> >>>>    include/media/v4l2-mediabus.h |   14 ++++++++------
> >>>>    1 files changed, 8 insertions(+), 6 deletions(-)
> >>>> 
> >>>> diff --git a/include/media/v4l2-mediabus.h
> >>>> b/include/media/v4l2-mediabus.h index 6114007..41d8771 100644
> >>>> --- a/include/media/v4l2-mediabus.h
> >>>> +++ b/include/media/v4l2-mediabus.h
> >>>> @@ -26,12 +26,14 @@
> >>>> 
> >>>>    /* Note: in BT.656 mode HSYNC and VSYNC are unused */
> >> 
> >> I've forgotten to update this:
> >> 
> >> /* Note: in BT.656 mode HSYNC, HREF and VSYNC are unused */
> >> 
> >>>>    #define V4L2_MBUS_HSYNC_ACTIVE_HIGH		(1<<   2)
> >>>>    #define V4L2_MBUS_HSYNC_ACTIVE_LOW		(1<<   3)
> >>>> 
> >>>> -#define V4L2_MBUS_VSYNC_ACTIVE_HIGH		(1<<   4)
> >>>> -#define V4L2_MBUS_VSYNC_ACTIVE_LOW		(1<<   5)
> >>>> -#define V4L2_MBUS_PCLK_SAMPLE_RISING		(1<<   6)
> >>>> -#define V4L2_MBUS_PCLK_SAMPLE_FALLING		(1<<   7)
> >>>> -#define V4L2_MBUS_DATA_ACTIVE_HIGH		(1<<   8)
> >>>> -#define V4L2_MBUS_DATA_ACTIVE_LOW		(1<<   9)
> >>>> +#define V4L2_MBUS_HREF_ACTIVE_HIGH		(1<<   4)
> >>>> +#define V4L2_MBUS_HREF_ACTIVE_LOW		(1<<   5)
> >>>> +#define V4L2_MBUS_VSYNC_ACTIVE_HIGH		(1<<   6)
> >>>> +#define V4L2_MBUS_VSYNC_ACTIVE_LOW		(1<<   7)
> >>>> +#define V4L2_MBUS_PCLK_SAMPLE_RISING		(1<<   8)
> >>>> +#define V4L2_MBUS_PCLK_SAMPLE_FALLING		(1<<   9)
> >>>> +#define V4L2_MBUS_DATA_ACTIVE_HIGH		(1<<   10)
> >>>> +#define V4L2_MBUS_DATA_ACTIVE_LOW		(1<<   11)

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC 1/2] v4l2: Add the parallel bus HREF signal polarity flags

[Sylwester Nawrocki]

On 09/19/2011 01:02 AM, Laurent Pinchart wrote:
> On Saturday 17 September 2011 14:07:30 Sylwester Nawrocki wrote:
>> On 09/17/2011 12:54 PM, Laurent Pinchart wrote:
>>> On Friday 16 September 2011 19:28:42 Sylwester Nawrocki wrote:
>>>> HREF is a signal indicating valid data during single line transmission.
>>>> Add corresponding flags for this signal to the set of mediabus signal
>>>> polarity flags.
>>>
>>> So that's a data valid signal that gates the pixel data ? The OMAP3 ISP
>>> has a
>>
>> Yes, it's "horizontal window reference" signal, it's well described in this
>> datasheet: http://www.morninghan.com/pdf/OV2640FSL_DS_(1_3).pdf
> 
> In that specific case I would likely connect to HREF signal to the ISP HSYNC 
> signal and ignore the sensor HSYNC signal completely :-)
> 
>> AFAICS there can be also its vertical counterpart - VREF.
> 
> OK, your HREF signal is thus completely unrelated to my DVAL signal. DVAL 
> really qualifies pixel. For instance, if the sensor outputs pixels at half the 
> pixel rate, DVAL would switch at every pixel clock cycle during a line.

Yeah, sounds it's entirely different.

> 
>> Many devices seem to use this terminology. However, I realize, not all, as
>> you're pointing out. So perhaps it's time for some naming contest now..
>> :-)
>>
>>> similar signal called WEN, and I've seen other chips using DVAL. Your
>>> patch looks good to me, except maybe for the signal name that could be
>>> made a bit more explicit (I'm not sure what most chips use though).
>>
>> I'm pretty OK with HREF/VREF. But I'm open to any better suggestions.
>>
>> Maybe
>>
>> V4L2_MBUS_LINE_VALID_ACTIVE_HIGH
>> V4L2_MBUS_LINE_VALID_ACTIVE_LOW
>>
>> V4L2_MBUS_FRAME_VALID_ACTIVE_HIGH
>> V4L2_MBUS_FRAME_VALID_ACTIVE_LOW
>>
>> ?
>> Some of Aptina sensor datasheets describes those signals as
>> LINE_VALID/FRAME_VALID, (www.aptina.com/assets/downloadDocument.do?id=76).
> 
> LINE_VALID/FRAME_VALID are HSYNC/VSYNC.

I would say these are rather inverted horizontal/vertical blanking signal.

> 
>>>> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
>>>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>>>> ---
>>>>
>>>>   include/media/v4l2-mediabus.h |   14 ++++++++------
>>>>   1 files changed, 8 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/include/media/v4l2-mediabus.h
>>>> b/include/media/v4l2-mediabus.h index 6114007..41d8771 100644
>>>> --- a/include/media/v4l2-mediabus.h
>>>> +++ b/include/media/v4l2-mediabus.h
>>>> @@ -26,12 +26,14 @@
>>>>
>>>>   /* Note: in BT.656 mode HSYNC and VSYNC are unused */
>>
>> I've forgotten to update this:
>>
>> /* Note: in BT.656 mode HSYNC, HREF and VSYNC are unused */
>>
>>>>   #define V4L2_MBUS_HSYNC_ACTIVE_HIGH		(1<<  2)
>>>>   #define V4L2_MBUS_HSYNC_ACTIVE_LOW		(1<<  3)
>>>>
>>>> -#define V4L2_MBUS_VSYNC_ACTIVE_HIGH		(1<<  4)
>>>> -#define V4L2_MBUS_VSYNC_ACTIVE_LOW		(1<<  5)
>>>> -#define V4L2_MBUS_PCLK_SAMPLE_RISING		(1<<  6)
>>>> -#define V4L2_MBUS_PCLK_SAMPLE_FALLING		(1<<  7)
>>>> -#define V4L2_MBUS_DATA_ACTIVE_HIGH		(1<<  8)
>>>> -#define V4L2_MBUS_DATA_ACTIVE_LOW		(1<<  9)
>>>> +#define V4L2_MBUS_HREF_ACTIVE_HIGH		(1<<  4)
>>>> +#define V4L2_MBUS_HREF_ACTIVE_LOW		(1<<  5)
>>>> +#define V4L2_MBUS_VSYNC_ACTIVE_HIGH		(1<<  6)
>>>> +#define V4L2_MBUS_VSYNC_ACTIVE_LOW		(1<<  7)
>>>> +#define V4L2_MBUS_PCLK_SAMPLE_RISING		(1<<  8)
>>>> +#define V4L2_MBUS_PCLK_SAMPLE_FALLING		(1<<  9)
>>>> +#define V4L2_MBUS_DATA_ACTIVE_HIGH		(1<<  10)
>>>> +#define V4L2_MBUS_DATA_ACTIVE_LOW		(1<<  11)
> 

Thanks
-- 
Sylwester Nawrocki
Samsung Poland R&D Center

Re: [PATCH/RFC 1/2] v4l2: Add the parallel bus HREF signal polarity flags

[Sylwester Nawrocki]

Hi Laurent,

On 09/19/2011 01:05 AM, Laurent Pinchart wrote:
> On Saturday 17 September 2011 18:06:20 Sylwester Nawrocki wrote:
>> On 09/17/2011 02:34 PM, Guennadi Liakhovetski wrote:
>>> On Sat, 17 Sep 2011, Sylwester Nawrocki wrote:
>>>> On 09/17/2011 12:54 PM, Laurent Pinchart wrote:
>>>>> On Friday 16 September 2011 19:28:42 Sylwester Nawrocki wrote:
>>>>>> HREF is a signal indicating valid data during single line
>>>>>> transmission. Add corresponding flags for this signal to the set of
>>>>>> mediabus signal polarity flags.
>>>>>
>>>>> So that's a data valid signal that gates the pixel data ? The OMAP3 ISP
>>>>> has a
>>>>
>>>> Yes, it's "horizontal window reference" signal, it's well described in
>>>> this datasheet: http://www.morninghan.com/pdf/OV2640FSL_DS_(1_3).pdf
>>>>
>>>> AFAICS there can be also its vertical counterpart - VREF.
>>>>
>>>> Many devices seem to use this terminology. However, I realize, not all,
>>>> as you're pointing out. So perhaps it's time for some naming contest
>>>> now.. :-)
>>>
>>> No objections in principle, just one question though: can these signals
>>> actually be used simultaneously with respective *SYNC signals or only as
>>> an alternative? If the latter, maybe we could reuse same names by just
>>> making them more generic?
>>
>> That's actually a good question. In my use cases only HREF is used as
>> horizontal synchronization signal, i.e. physical bus interface has this
>> signals:
>>
>> ->| PCLK
>> ->| VSYNC
>> ->| HREF
>> ->| DATA[0:7]
>> ->| FIELD
>>
>> For interlaced mode FIELD can be connected to the horizontal
>> synchronization signal. For this case there is InvPolHSYNC bit in the host
>> interface registers to indicate the polarity. There are 5 bits actually:
>>
>> InvPolPCLK
>> InvPolVSYNC (vertical sychronization)
>> InvPolHREF  (horizontal synchronization)
>> InvPolHSYNC (for interlaced mode only, FIELD port = horizontal sync.
>> signal) InvPolFIELD (interlaced mode,  FIELD port = FIELD signal)
> 
> Shouldn't this be handled through platform data only ?

Indeed, this is how it's done now and I didn't intend to change that.
I just wanted to replace driver's private signal polarity flag definitions
with the standard ones. Do you think I should rather keep these things in
driver's public header? It's of course a way of less resistance :)


To make things complete I thought about adding the FIELD flags, i.e.

>From 9bd11f9b14dffe877f9c546e068b4b4027c9472a Mon Sep 17 00:00:00 2001
From: Sylwester Nawrocki <s.nawrocki@samsung.com>
Date: Sun, 18 Sep 2011 11:28:58 +0200
Subject: [PATCH 1/2] v4l2: Add the parallel bus HREF and FIELD signal polarity
flags

HREF is a signal gating valid data during single line transmission.
FIELD is an Even/Odd field selection signal, as specified in ITU-R BT.601
standard.
Add corresponding flags for these signals to the set of media bus signal
polarity flags.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 include/media/v4l2-mediabus.h |   20 +++++++++++++-------
 1 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
index 6114007..1952d9f 100644
--- a/include/media/v4l2-mediabus.h
+++ b/include/media/v4l2-mediabus.h
@@ -23,15 +23,21 @@
 #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 */
+/* Note: in BT.656 mode HSYNC, HREF, FIELD, and VSYNC are unused */
 #define V4L2_MBUS_HSYNC_ACTIVE_HIGH		(1 << 2)
 #define V4L2_MBUS_HSYNC_ACTIVE_LOW		(1 << 3)
-#define V4L2_MBUS_VSYNC_ACTIVE_HIGH		(1 << 4)
-#define V4L2_MBUS_VSYNC_ACTIVE_LOW		(1 << 5)
-#define V4L2_MBUS_PCLK_SAMPLE_RISING		(1 << 6)
-#define V4L2_MBUS_PCLK_SAMPLE_FALLING		(1 << 7)
-#define V4L2_MBUS_DATA_ACTIVE_HIGH		(1 << 8)
-#define V4L2_MBUS_DATA_ACTIVE_LOW		(1 << 9)
+/* HREF is a horizontal window reference signal gating valid pixel data */
+#define V4L2_MBUS_HREF_ACTIVE_HIGH		(1 << 4)
+#define V4L2_MBUS_HREF_ACTIVE_LOW		(1 << 5)
+#define V4L2_MBUS_VSYNC_ACTIVE_HIGH		(1 << 6)
+#define V4L2_MBUS_VSYNC_ACTIVE_LOW		(1 << 7)
+#define V4L2_MBUS_PCLK_SAMPLE_RISING		(1 << 8)
+#define V4L2_MBUS_PCLK_SAMPLE_FALLING		(1 << 9)
+#define V4L2_MBUS_DATA_ACTIVE_HIGH		(1 << 10)
+#define V4L2_MBUS_DATA_ACTIVE_LOW		(1 << 11)
+/* Field selection signal for interlaced scan mode */
+#define V4L2_MBUS_FIELD_ACTIVE_HIGH		(1 << 12)
+#define V4L2_MBUS_FIELD_ACTIVE_LOW		(1 << 13)

 /* Serial flags */
 /* How many lanes the client can use */
-- 1.7.4.1

If there is more objection to the above changes then I'll drop the patch
and stay with driver's private definitions.


Thanks,
-- 
Sylwester Nawrocki
Samsung Poland R&D Center

[PATCH v2 0/2] v4l: Add media bus polarity flags for FIELD signal

[Sylwester Nawrocki]

Hello,

The following patch adds support for FIELD signal polarity configuration
through the parallel media bus flags.
The second one just converts s5p-fimc driver to use generic flags.

Changes since v2:
 - dropped V4L2_MBUS_HREF* definitions, added comment on usage of
   V4L2_MBUS_[HV]SYNC* flags for [HV]REF signals
 - added V4L2_MBUS_FIELD*
 - modified the second patch to use HSYNC flags only

Sylwester Nawrocki (2):
  v4l2: Add the polarity flags for parallel camera bus FIELD signal
  s5p-fimc: Convert to use generic media bus polarity flags

 drivers/media/video/s5p-fimc/fimc-reg.c  |   14 +++++++++-----
 drivers/media/video/s5p-fimc/regs-fimc.h |    1 +
 include/media/s5p_fimc.h                 |    7 +------
 include/media/v4l2-mediabus.h            |   11 +++++++++--
 4 files changed, 20 insertions(+), 13 deletions(-)


Thanks,
--
Sylwester Nawrocki
Samsung Poland R&D Center

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

[Sylwester Nawrocki]

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>
---
 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..586fc44 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 << 12)
+#define V4L2_MBUS_FIELD_ACTIVE_LOW		(1 << 13)
 
 /* Serial flags */
 /* How many lanes the client can use */
-- 
1.7.6.3

[PATCH v2 2/2] s5p-fimc: Convert to use generic media bus polarity flags

[Sylwester Nawrocki]

Switch to generic media bus signal polarity flags and allow
configuring the FIELD signal polarity.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <s.nawrocki@samsung.com>
---
 drivers/media/video/s5p-fimc/fimc-reg.c  |   14 +++++++++-----
 drivers/media/video/s5p-fimc/regs-fimc.h |    1 +
 include/media/s5p_fimc.h                 |    7 +------
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/media/video/s5p-fimc/fimc-reg.c b/drivers/media/video/s5p-fimc/fimc-reg.c
index 2a1ae51..678d7d3 100644
--- a/drivers/media/video/s5p-fimc/fimc-reg.c
+++ b/drivers/media/video/s5p-fimc/fimc-reg.c
@@ -533,20 +533,24 @@ int fimc_hw_set_camera_polarity(struct fimc_dev *fimc,
 	u32 cfg = readl(fimc->regs + S5P_CIGCTRL);
 
 	cfg &= ~(S5P_CIGCTRL_INVPOLPCLK | S5P_CIGCTRL_INVPOLVSYNC |
-		 S5P_CIGCTRL_INVPOLHREF | S5P_CIGCTRL_INVPOLHSYNC);
+		 S5P_CIGCTRL_INVPOLHREF | S5P_CIGCTRL_INVPOLHSYNC |
+		 S5P_CIGCTRL_INVPOLFIELD);
 
-	if (cam->flags & FIMC_CLK_INV_PCLK)
+	if (cam->flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
 		cfg |= S5P_CIGCTRL_INVPOLPCLK;
 
-	if (cam->flags & FIMC_CLK_INV_VSYNC)
+	if (cam->flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
 		cfg |= S5P_CIGCTRL_INVPOLVSYNC;
 
-	if (cam->flags & FIMC_CLK_INV_HREF)
+	if (cam->flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)
 		cfg |= S5P_CIGCTRL_INVPOLHREF;
 
-	if (cam->flags & FIMC_CLK_INV_HSYNC)
+	if (cam->flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)
 		cfg |= S5P_CIGCTRL_INVPOLHSYNC;
 
+	if (cam->flags & V4L2_MBUS_FIELD_ACTIVE_LOW)
+		cfg |= S5P_CIGCTRL_INVPOLFIELD;
+
 	writel(cfg, fimc->regs + S5P_CIGCTRL);
 
 	return 0;
diff --git a/drivers/media/video/s5p-fimc/regs-fimc.h b/drivers/media/video/s5p-fimc/regs-fimc.h
index 94d2302..c8e3b94 100644
--- a/drivers/media/video/s5p-fimc/regs-fimc.h
+++ b/drivers/media/video/s5p-fimc/regs-fimc.h
@@ -61,6 +61,7 @@
 #define S5P_CIGCTRL_CSC_ITU601_709	(1 << 5)
 #define S5P_CIGCTRL_INVPOLHSYNC		(1 << 4)
 #define S5P_CIGCTRL_SELCAM_MIPI		(1 << 3)
+#define S5P_CIGCTRL_INVPOLFIELD		(1 << 1)
 #define S5P_CIGCTRL_INTERLACE		(1 << 0)
 
 /* Window offset 2 */
diff --git a/include/media/s5p_fimc.h b/include/media/s5p_fimc.h
index 2b58904..688fb3f 100644
--- a/include/media/s5p_fimc.h
+++ b/include/media/s5p_fimc.h
@@ -19,11 +19,6 @@ enum cam_bus_type {
 	FIMC_LCD_WB, /* FIFO link from LCD mixer */
 };
 
-#define FIMC_CLK_INV_PCLK	(1 << 0)
-#define FIMC_CLK_INV_VSYNC	(1 << 1)
-#define FIMC_CLK_INV_HREF	(1 << 2)
-#define FIMC_CLK_INV_HSYNC	(1 << 3)
-
 struct i2c_board_info;
 
 /**
@@ -37,7 +32,7 @@ struct i2c_board_info;
  * @i2c_bus_num: i2c control bus id the sensor is attached to
  * @mux_id: FIMC camera interface multiplexer index (separate for MIPI and ITU)
  * @clk_id: index of the SoC peripheral clock for sensors
- * @flags: flags defining bus signals polarity inversion (High by default)
+ * @flags: the parallel bus flags defining signals polarity (V4L2_MBUS_*)
  */
 struct s5p_fimc_isp_info {
 	struct i2c_board_info *board_info;
-- 
1.7.6.3

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

[Sylwester Nawrocki]

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(-)

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)
 
 /* Serial flags */
 /* How many lanes the client can use */
-- 
1.7.6.3

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

[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.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 */

-- 
Regards,

Laurent Pinchart

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

[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 R&D Center

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

[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.