From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Warren Date: Fri, 03 Aug 2012 18:30:57 +0000 Subject: Re: [PATCH v2] of: Add videomode helper Message-Id: <501C18E1.3020901@wwwdotorg.org> List-Id: References: <1341388595-30672-1-git-send-email-s.hauer@pengutronix.de> <501AD68C.1000904@wwwdotorg.org> <20120803073844.GK1451@pengutronix.de> In-Reply-To: <20120803073844.GK1451-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Sascha Hauer Cc: "linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org" , "dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org" , Laurent Pinchart , "kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org" On 08/03/2012 01:38 AM, Sascha Hauer wrote: > Hi Stephen, > > On Thu, Aug 02, 2012 at 01:35:40PM -0600, Stephen Warren wrote: >> On 07/04/2012 01:56 AM, Sascha Hauer wrote: >>> This patch adds a helper function for parsing videomodes from the devicetree. >>> The videomode can be either converted to a struct drm_display_mode or a >>> struct fb_videomode. >> >>> diff --git a/Documentation/devicetree/bindings/video/displaymode b/Documentation/devicetree/bindings/video/displaymode >> >>> +Required properties: >>> + - xres, yres: Display resolution >>> + - left-margin, right-margin, hsync-len: Horizontal Display timing parameters >>> + in pixels >>> + upper-margin, lower-margin, vsync-len: Vertical display timing parameters in >>> + lines >> >> Perhaps bike-shedding, but... >> >> For the margin property names, wouldn't it be better to use terminology >> more commonly used for video timings rather than Linux FB naming. In >> other words naming like: >> >> hactive, hsync-len, hfront-porch, hback-porch? > > Can do. Just to make sure: > > hactive = xres > hsync-len = hsync-len > hfront-porch = right-margin > hback-porch = left-margin I believe so yes. >> a) They're already standardized and very common. > > Indeed, that's a big plus for EDID. I have no intention of removing EDID > data from the devicetree. There are situations where EDID is handy, for > example when you get EDID data provided by your vendor. > >> >> b) They allow other information such as a display's HDMI audio >> capabilities to be represented, which can then feed into an ALSA driver. >> >> c) The few LCD panel datasheets I've seen actually quote their >> specification as an EDID already, so deriving the EDID may actually be easy. >> >> d) People familiar with displays are almost certainly familiar with >> EDID's mode representation. There are many ways of representing display >> modes (sync position vs. porch widths, htotal specified rather than >> specifying all the components and hence htotal being calculated etc.). >> Not everyone will be familiar with all representations. Conversion >> errors are less likely if the target is EDID's familiar format. > > You seem to think about a different class of displays for which EDID > indeed is a better way to handle. What I have to deal with here mostly > are dumb displays which: > > - can only handle their native resolution > - Have no audio capabilities at all > - come with a datasheet which specify a min/typ/max triplet for > xres,hsync,..., often enough they are scanned to pdf from some previously > printed paper. > > These displays are very common on embedded devices, probably that's the > reason I did not even think about the possibility that a single display > might have different modes. That's true, but as I mentioned, there are at least some dumb panels (both I've seen recently) whose specification provides the EDID. I don't know how common that is though, I must admit. >> e) You'll end up with exactly the same data as if you have a DDC-based >> external monitor rather than an internal panel, so you end up getting to >> a common path in display handling code much more quickly. > > All we have in our display driver currently is: > > edidp = of_get_property(np, "edid", &imxpd->edid_len); > if (edidp) { > imxpd->edid = kmemdup(edidp, imxpd->edid_len, GFP_KERNEL); > } else { > ret = of_get_video_mode(np, &imxpd->mode, NULL); > if (!ret) > imxpd->mode_valid = 1; > } Presumably there's more to it though; something later checks imxpd->mode_valid and if false, parses the EDID and sets up imxpd->mode, etc.