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.
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).
Yes, that's true. I'll set the numbers anyway, for clarity.
Archit
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
