On Thu, Sep 13, 2012 at 03:40:40AM +0200, Laurent Pinchart wrote: > Hi Sascha, > > > > +int panel_get_modes(struct panel *panel, const struct fb_videomode > > > **modes) > > > +{ > > > + if (!panel->ops || !panel->ops->get_modes) > > > + return 0; > > > + > > > + return panel->ops->get_modes(panel, modes); > > > +} > > > +EXPORT_SYMBOL_GPL(panel_get_modes); > > > > You have seen my of videomode helper proposal. One result there was that > > we want to have ranges for the margin/synclen fields. Does it make sense > > to base this new panel framework on a more sophisticated internal > > reprentation of the panel parameters? > > I think it does, yes. We need a common video mode structure, and the panel > framework should use it. I'll try to rebase my patches on top of your > proposal. Have you posted the latest version ? V2 is the newest version. I'd like to implement ranges for the display timings which then makes for a new common video mode structure, which then could be used by drm and fbdev, probably with helper functions to convert from common videomode to drm/fbdev specific variants. > > > This could then be converted to struct fb_videomode / struct > > drm_display_mode as needed. This would also make it more suitable for drm > > drivers which are not interested in struct fb_videomode. > > > > Related to this I suggest to change the API to be able to iterate over > > the different modes, like: > > > > struct videomode *panel_get_mode(struct panel *panel, int num); > > > > This was we do not have to have an array of modes in memory, which may > > be a burden for some panel drivers. > > I currently have mixed feelings about this. Both approaches have pros and > cons. Iterating over the modes would be more complex for drivers that use > panels, and would be race-prone if the modes can change at runtime (OK, this > isn't supported by the current panel API proposal). I just remember that the array approach was painful when I worked on an fbdev driver some time ago. There some possible modes came from platform_data, other modes were default modes in the driver and all had to be merged into a single array. I don't remember the situation exactly, but it would have been simpler if it had been a list instead of an array. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html