Re: [PATCH v10 1/6] video: add display_timing and videomode

[Grant Likely <grant.likely@secretlab.ca>]

On Thu, 15 Nov 2012 17:00:57 +0100, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> Hi Grant,
> 
> On Thursday 15 November 2012 15:47:53 Grant Likely wrote:
> > On Thu, 15 Nov 2012 10:23:52 +0100, Steffen Trumtrar wrote:
> > > Add display_timing structure and the according helper functions. This
> > > allows the description of a display via its supported timing parameters.
> > > 
> > > Every timing parameter can be specified as a single value or a range
> > > <min typ max>.
> > > 
> > > Also, add helper functions to convert from display timings to a generic
> > > videomode structure. This videomode can then be converted to the
> > > corresponding subsystem mode representation (e.g. fb_videomode).
> > > 
> > > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> > 
> > Hmmm... here's my thoughts as an outside reviewer. Correct me if I'm
> > making an incorrect assumption.
> > 
> > It looks to me that the purpose of this entire series is to decode video
> > timings from the device tree and (eventually) provide the data in the
> > form 'struct videomode'. Correct?
> > 
> > If so, then it looks over engineered. Creating new infrastructure to
> > allocate, maintain, and free a new 'struct display_timings' doesn't make
> > any sense when it is an intermediary data format that will never be used
> > by drivers.
> > 
> > Can the DT parsing code instead return a table of struct videomode?
> > 
> > But, wait... struct videomode is also a new structure. So it looks like
> > this series creates two new intermediary data structures;
> > display_timings and videomode. And at least as far as I can see in this
> > series struct fb_videomode is the only user.
> 
> struct videomode is supposed to slowly replace the various video mode 
> structures we currently have in the kernel (struct drm_mode_modeinfo, struct 
> fb_videomode and struct v4l2_bt_timings), at least where possible (userspace 
> APIs can't be broken). This will make it possible to reuse code across the 
> DRM, FB and V4L2 subsystems, such as the EDID parser or HDMI encoder drivers. 
> This rationale might not be clearly explained in the commit message, but 
> having a shared video mode structure is pretty important.

Okay that make sense. What about struct display_timings?

g.