diff mbox

[6/6] OMAPDSS: VENC: Maintian copy of video output polarity in private data

Message ID 1345102594-6222-7-git-send-email-archit@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

archit taneja Aug. 16, 2012, 7:36 a.m. UTC
The VENC driver currently relies on the omap_dss_device struct to configure the
video output polarity. This makes the VENC interface driver dependent on the
omap_dss_device struct.

Make the VENC driver data maintain it's own polarity field. A panel driver
is expected to call omapdss_venc_set_vid_out_polarity() before enabling the
interface.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/video/omap2/dss/dss.h        |    2 ++
 drivers/video/omap2/dss/venc.c       |   13 ++++++++++++-
 drivers/video/omap2/dss/venc_panel.c |    6 ++++++
 3 files changed, 20 insertions(+), 1 deletion(-)

Comments

Tomi Valkeinen Aug. 16, 2012, 11:38 a.m. UTC | #1
On Thu, 2012-08-16 at 13:06 +0530, Archit Taneja wrote:
> The VENC driver currently relies on the omap_dss_device struct to configure the
> video output polarity. This makes the VENC interface driver dependent on the
> omap_dss_device struct.
> 
> Make the VENC driver data maintain it's own polarity field. A panel driver
> is expected to call omapdss_venc_set_vid_out_polarity() before enabling the
> interface.
> 
> Signed-off-by: Archit Taneja <archit@ti.com>
> ---
>  drivers/video/omap2/dss/dss.h        |    2 ++
>  drivers/video/omap2/dss/venc.c       |   13 ++++++++++++-
>  drivers/video/omap2/dss/venc_panel.c |    6 ++++++
>  3 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
> index c17d298..b2cf5530 100644
> --- a/drivers/video/omap2/dss/dss.h
> +++ b/drivers/video/omap2/dss/dss.h
> @@ -479,6 +479,8 @@ u32 omapdss_venc_get_wss(struct omap_dss_device *dssdev);
>  int omapdss_venc_set_wss(struct omap_dss_device *dssdev, u32 wss);
>  void omapdss_venc_set_type(struct omap_dss_device *dssdev,
>  		enum omap_dss_venc_type type);
> +void omapdss_venc_set_vid_out_polarity(struct omap_dss_device *dssdev,
> +		enum omap_dss_signal_level vid_out_pol);
>  int venc_panel_init(void);
>  void venc_panel_exit(void);
>  
> diff --git a/drivers/video/omap2/dss/venc.c b/drivers/video/omap2/dss/venc.c
> index 2d90fcf..8cb372f 100644
> --- a/drivers/video/omap2/dss/venc.c
> +++ b/drivers/video/omap2/dss/venc.c
> @@ -303,6 +303,7 @@ static struct {
>  
>  	struct omap_video_timings timings;
>  	enum omap_dss_venc_type type;
> +	enum omap_dss_signal_level vid_out_pol;
>  } venc;
>  
>  static inline void venc_write_reg(int idx, u32 val)
> @@ -447,7 +448,7 @@ static int venc_power_on(struct omap_dss_device *dssdev)
>  	else /* S-Video */
>  		l |= (1 << 0) | (1 << 2);
>  
> -	if (dssdev->phy.venc.invert_polarity == false)
> +	if (venc.vid_out_pol == OMAPDSS_SIG_ACTIVE_HIGH)
>  		l |= 1 << 3;

Are you sure this is correct? I know practically nothing about analog
TV, but the TRM doesn't seem to say much about that bit, except it can
be used to "invert the video output". It doesn't say there's an
active/inactive level for the signal.

 Tomi
Archit Taneja Aug. 16, 2012, 12:27 p.m. UTC | #2
On Thursday 16 August 2012 05:08 PM, Tomi Valkeinen wrote:
> On Thu, 2012-08-16 at 13:06 +0530, Archit Taneja wrote:
>> The VENC driver currently relies on the omap_dss_device struct to configure the
>> video output polarity. This makes the VENC interface driver dependent on the
>> omap_dss_device struct.
>>
>> Make the VENC driver data maintain it's own polarity field. A panel driver
>> is expected to call omapdss_venc_set_vid_out_polarity() before enabling the
>> interface.
>>
>> Signed-off-by: Archit Taneja <archit@ti.com>
>> ---
>>   drivers/video/omap2/dss/dss.h        |    2 ++
>>   drivers/video/omap2/dss/venc.c       |   13 ++++++++++++-
>>   drivers/video/omap2/dss/venc_panel.c |    6 ++++++
>>   3 files changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
>> index c17d298..b2cf5530 100644
>> --- a/drivers/video/omap2/dss/dss.h
>> +++ b/drivers/video/omap2/dss/dss.h
>> @@ -479,6 +479,8 @@ u32 omapdss_venc_get_wss(struct omap_dss_device *dssdev);
>>   int omapdss_venc_set_wss(struct omap_dss_device *dssdev, u32 wss);
>>   void omapdss_venc_set_type(struct omap_dss_device *dssdev,
>>   		enum omap_dss_venc_type type);
>> +void omapdss_venc_set_vid_out_polarity(struct omap_dss_device *dssdev,
>> +		enum omap_dss_signal_level vid_out_pol);
>>   int venc_panel_init(void);
>>   void venc_panel_exit(void);
>>
>> diff --git a/drivers/video/omap2/dss/venc.c b/drivers/video/omap2/dss/venc.c
>> index 2d90fcf..8cb372f 100644
>> --- a/drivers/video/omap2/dss/venc.c
>> +++ b/drivers/video/omap2/dss/venc.c
>> @@ -303,6 +303,7 @@ static struct {
>>
>>   	struct omap_video_timings timings;
>>   	enum omap_dss_venc_type type;
>> +	enum omap_dss_signal_level vid_out_pol;
>>   } venc;
>>
>>   static inline void venc_write_reg(int idx, u32 val)
>> @@ -447,7 +448,7 @@ static int venc_power_on(struct omap_dss_device *dssdev)
>>   	else /* S-Video */
>>   		l |= (1 << 0) | (1 << 2);
>>
>> -	if (dssdev->phy.venc.invert_polarity == false)
>> +	if (venc.vid_out_pol == OMAPDSS_SIG_ACTIVE_HIGH)
>>   		l |= 1 << 3;
>
> Are you sure this is correct? I know practically nothing about analog
> TV, but the TRM doesn't seem to say much about that bit, except it can
> be used to "invert the video output". It doesn't say there's an
> active/inactive level for the signal.

Well, the code works :), I'm also not totally sure about whether it 
should be represented as a 2-level signal, it seemed like it would be 
nicer to give it a signal level rather than keeping it as a bool, which 
could vary for other SoC's?

I am considering not to pass this via the panel driver for now, I don't 
know if all VENC IPs needs to do this, maybe it's okay to leave this 
dssdev reference for now?

Archit

>
>   Tomi
>

--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomi Valkeinen Aug. 16, 2012, 1:09 p.m. UTC | #3
On Thu, 2012-08-16 at 17:57 +0530, Archit Taneja wrote:
> On Thursday 16 August 2012 05:08 PM, Tomi Valkeinen wrote:
> > On Thu, 2012-08-16 at 13:06 +0530, Archit Taneja wrote:
> >> The VENC driver currently relies on the omap_dss_device struct to configure the
> >> video output polarity. This makes the VENC interface driver dependent on the
> >> omap_dss_device struct.
> >>
> >> Make the VENC driver data maintain it's own polarity field. A panel driver
> >> is expected to call omapdss_venc_set_vid_out_polarity() before enabling the
> >> interface.
> >>
> >> Signed-off-by: Archit Taneja <archit@ti.com>
> >> ---
> >>   drivers/video/omap2/dss/dss.h        |    2 ++
> >>   drivers/video/omap2/dss/venc.c       |   13 ++++++++++++-
> >>   drivers/video/omap2/dss/venc_panel.c |    6 ++++++
> >>   3 files changed, 20 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
> >> index c17d298..b2cf5530 100644
> >> --- a/drivers/video/omap2/dss/dss.h
> >> +++ b/drivers/video/omap2/dss/dss.h
> >> @@ -479,6 +479,8 @@ u32 omapdss_venc_get_wss(struct omap_dss_device *dssdev);
> >>   int omapdss_venc_set_wss(struct omap_dss_device *dssdev, u32 wss);
> >>   void omapdss_venc_set_type(struct omap_dss_device *dssdev,
> >>   		enum omap_dss_venc_type type);
> >> +void omapdss_venc_set_vid_out_polarity(struct omap_dss_device *dssdev,
> >> +		enum omap_dss_signal_level vid_out_pol);
> >>   int venc_panel_init(void);
> >>   void venc_panel_exit(void);
> >>
> >> diff --git a/drivers/video/omap2/dss/venc.c b/drivers/video/omap2/dss/venc.c
> >> index 2d90fcf..8cb372f 100644
> >> --- a/drivers/video/omap2/dss/venc.c
> >> +++ b/drivers/video/omap2/dss/venc.c
> >> @@ -303,6 +303,7 @@ static struct {
> >>
> >>   	struct omap_video_timings timings;
> >>   	enum omap_dss_venc_type type;
> >> +	enum omap_dss_signal_level vid_out_pol;
> >>   } venc;
> >>
> >>   static inline void venc_write_reg(int idx, u32 val)
> >> @@ -447,7 +448,7 @@ static int venc_power_on(struct omap_dss_device *dssdev)
> >>   	else /* S-Video */
> >>   		l |= (1 << 0) | (1 << 2);
> >>
> >> -	if (dssdev->phy.venc.invert_polarity == false)
> >> +	if (venc.vid_out_pol == OMAPDSS_SIG_ACTIVE_HIGH)
> >>   		l |= 1 << 3;
> >
> > Are you sure this is correct? I know practically nothing about analog
> > TV, but the TRM doesn't seem to say much about that bit, except it can
> > be used to "invert the video output". It doesn't say there's an
> > active/inactive level for the signal.
> 
> Well, the code works :), I'm also not totally sure about whether it 

You could put there APPLE and ORANGE enum values, and it'd still work =)

> should be represented as a 2-level signal, it seemed like it would be 
> nicer to give it a signal level rather than keeping it as a bool, which 
> could vary for other SoC's?

It may be really a bool. TRM says "This may be used to
correct for inversion in an external video amplifier". I don't think
there are any digital on/off signals in analog video output, so I'm
guessing it's really inverting (some parts of) the analog signal. If so,
a boolean invert field sounds correct to me.

Actually, check this out:

http://books.google.fi/books?id=P6BlcWaizHUC&pg=PT81&lpg=PT81&dq=composite+video+polarity&source=bl&ots=-gsl0Exv5R&sig=gMylEnoV9ozRwM4RX2iQFqhRpP8&hl=en&sa=X&ei=0u8sUIe3KYXh4QTf9YDQBA&redir_esc=y#v=onepage&q=composite%20video%20polarity&f=false

A monster url, here's a tinyurl version: http://tinyurl.com/clceb6t

2.16 section there shows signal polarities. I'm not sure if it's the
same one that we're discussing, but sounds like it.

I don't think ACTIVE_LOW/HIGH fits into that kind of polarity. Perhaps a
bool is not quite right for it either, as I'm not sure there's a
"normal" polarity. But I'd go forward with out current bool, and fix it
when somebody who understands this stuff tells us what to do =).

> I am considering not to pass this via the panel driver for now, I don't 
> know if all VENC IPs needs to do this, maybe it's okay to leave this 
> dssdev reference for now?

I don't know if other VENC IPs support this or not, but the TRM refers
to external amplifier, so it sounds like a thing that would exist on
other IPs also.

 Tomi
diff mbox

Patch

diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index c17d298..b2cf5530 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -479,6 +479,8 @@  u32 omapdss_venc_get_wss(struct omap_dss_device *dssdev);
 int omapdss_venc_set_wss(struct omap_dss_device *dssdev, u32 wss);
 void omapdss_venc_set_type(struct omap_dss_device *dssdev,
 		enum omap_dss_venc_type type);
+void omapdss_venc_set_vid_out_polarity(struct omap_dss_device *dssdev,
+		enum omap_dss_signal_level vid_out_pol);
 int venc_panel_init(void);
 void venc_panel_exit(void);
 
diff --git a/drivers/video/omap2/dss/venc.c b/drivers/video/omap2/dss/venc.c
index 2d90fcf..8cb372f 100644
--- a/drivers/video/omap2/dss/venc.c
+++ b/drivers/video/omap2/dss/venc.c
@@ -303,6 +303,7 @@  static struct {
 
 	struct omap_video_timings timings;
 	enum omap_dss_venc_type type;
+	enum omap_dss_signal_level vid_out_pol;
 } venc;
 
 static inline void venc_write_reg(int idx, u32 val)
@@ -447,7 +448,7 @@  static int venc_power_on(struct omap_dss_device *dssdev)
 	else /* S-Video */
 		l |= (1 << 0) | (1 << 2);
 
-	if (dssdev->phy.venc.invert_polarity == false)
+	if (venc.vid_out_pol == OMAPDSS_SIG_ACTIVE_HIGH)
 		l |= 1 << 3;
 
 	venc_write_reg(VENC_OUTPUT_CONTROL, l);
@@ -639,6 +640,16 @@  void omapdss_venc_set_type(struct omap_dss_device *dssdev,
 	mutex_unlock(&venc.venc_lock);
 }
 
+void omapdss_venc_set_vid_out_polarity(struct omap_dss_device *dssdev,
+		enum omap_dss_signal_level vid_out_pol)
+{
+	mutex_lock(&venc.venc_lock);
+
+	venc.vid_out_pol = vid_out_pol;
+
+	mutex_unlock(&venc.venc_lock);
+}
+
 static int __init venc_init_display(struct omap_dss_device *dssdev)
 {
 	DSSDBG("init_display\n");
diff --git a/drivers/video/omap2/dss/venc_panel.c b/drivers/video/omap2/dss/venc_panel.c
index ef21361..a8117d0 100644
--- a/drivers/video/omap2/dss/venc_panel.c
+++ b/drivers/video/omap2/dss/venc_panel.c
@@ -118,6 +118,7 @@  static void venc_panel_remove(struct omap_dss_device *dssdev)
 static int venc_panel_enable(struct omap_dss_device *dssdev)
 {
 	int r;
+	enum omap_dss_signal_level vid_out_pol;
 
 	dev_dbg(&dssdev->dev, "venc_panel_enable\n");
 
@@ -131,6 +132,11 @@  static int venc_panel_enable(struct omap_dss_device *dssdev)
 	omapdss_venc_set_timings(dssdev, &dssdev->panel.timings);
 	omapdss_venc_set_type(dssdev, dssdev->phy.venc.type);
 
+	vid_out_pol = dssdev->phy.venc.invert_polarity ?
+			OMAPDSS_SIG_ACTIVE_LOW : OMAPDSS_SIG_ACTIVE_HIGH;
+
+	omapdss_venc_set_vid_out_polarity(dssdev, vid_out_pol);
+
 	r = omapdss_venc_display_enable(dssdev);
 	if (r)
 		goto err;