Re: [PATCH v2] of: Add videomode helper

[Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@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.