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

Tomi Valkeinen
2012-06-27T12:42:19Z

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

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.

Also, related thing, I see you're writing enum values directly to the
registers. If you do that, it'd be good to explicitly set the number
value of the enums. Otherwise it's quite easy to add a new enum value
later between the old ones, breaking everything (perhaps not a problem
here, though).

Tomi

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