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

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 4329 bytes --]

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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]