Re: [PATCH 05/17] OMAPDSS: Add some new fields to omap_video_timings

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

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

On Wed, 2012-06-27 at 18:17 +0530, Archit Taneja wrote:
> On Wednesday 27 June 2012 06:12 PM, Tomi Valkeinen wrote:
> > On Wed, 2012-06-27 at 17:56 +0530, Archit Taneja wrote:
> >> On Wednesday 27 June 2012 05:18 PM, Tomi Valkeinen wrote:
> >>> On Tue, 2012-06-26 at 15:06 +0530, Archit Taneja wrote:
> >>>> Some panel timing related fields are contained in omap_panel_config in the form
> >>>> of flags. The fields are:
> >>>>
> >>>> - Hsync logic level
> >>>> - Vsync logic level
> >>>> - Data driven on rising/falling edge of pixel clock
> >>>> - Output enable/Data enable logic level
> >>>> - HSYNC/VSYNC driven on rising/falling edge of pixel clock
> >>>>
> >>>> Out of these parameters, Hsync and Vsync logic levels are a part of the timings
> >>>> in the Xorg modeline configuration. So it makes sense to move the to
> >>>> omap_video_timings. The rest aren't a part of modeline, but it still makes
> >>>> sense to move these since they are related to panel timings.
> >>>>
> >>>> These fields stored in omap_panel_config in dssdev are configured for LCD
> >>>> panels, and the corresponding LCD managers in the DISPC_POL_FREQo registers.
> >>>>
> >>>> Add the above fields in omap_video_timings. Represent their state via new enums.
> >>>> The parameter pclk_edge is configured via omap_dss_signal_level, however it
> >>>> actually configures whether data is driven on the rising or falling edge. This
> >>>> is a bit unclean, but it prevents us from creating another enum.
> >>>
> >>> Hmm, why can't omap_dss_signal_edge be used for pclk_edge? I think it'd
> >>> fit fine, except OMAPDSS_DRIVE_SIG_OPPOSITE_EDGES would be an illegal
> >>> value for it.
> >>
> >> I think my paragraph is a bit misleading. The issue is more about the
> >> default value. For hsync_vsync_edge(which programs ONOFF and RF), the
> >> default value is OMAPDSS_DRIVE_SIG_OPPOSITE_EDGES, and is the case for
> >> most panels. But for pclk_edge, the value for most panels is
> >> OMAPDSS_DRIVE_SIG_ACTIVE_HIGH.
> >>
> >> So if I use the same enum for both, I'll need to populate either
> >> pclk_edge or hsync_vsync_edge for almost every panel. With my approach,
> >> I only need to populate these fields for panels having the non-default
> >> requirements. The negative thing is that it's a bit misleading to
> >> represent pclk_edge with the omap_dss_signal_level enum.
> >
> > Ah, I see.
> >
> > I really think using the ACTIVE_LOW/HIGH for pclk_edge is quite ugly =).
> >
> > Well, one option is to add new entry for the enums, "UNDEFINED" or
> > perhaps "DEFAULT", which would have value 0. Then omapdss would know
> > that it should use the default value, whatever that is.
> >
> > Another option is to have all panels define the pclk_edge, so that
> > there's no default value needed.
> 
> I think I'll go with this.

Ok. But if you do this, then perhaps you should fill all the other
properties also. It looks a bit strange to require only one particular
to be always entered, while the others have default values.

But I guess that's not a bad thing at all. If the panel doesn't define
what kind of signal it wants, and trusts that the defaults are always
the same, it may break later or perhaps with other platforms.

So I think the panel drivers should define all the signal properties,
and not leave them to default values. That's the safest option, although
a bit verbose.

 Tomi


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