<html> <head> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> </head> <body bgcolor="#FFFFFF" text="#000000"> Hi Krzysztof, <div class="moz-cite-prefix">On 09/30/2015 03:34 PM, Krzysztof Kozlowski wrote: </div> <blockquote cite="mid:560B9075.1000304@samsung.com" type="cite"> <pre wrap="">On 30.09.2015 16:19, Yakir Yang wrote: </pre> <blockquote type="cite"> <pre wrap="">Hi Krzysztof, On 09/30/2015 01:32 PM, Krzysztof Kozlowski wrote: </pre> <blockquote type="cite"> <pre wrap="">On 22.09.2015 16:37, Yakir Yang wrote: </pre> <blockquote type="cite"> <pre wrap="">Both hsync/vsync polarity and interlace mode can be parsed from drm display mode, and dynamic_range and ycbcr_coeff can be judge by the video code. But presumably Exynos still relies on the DT properties, so take good use of mode_fixup() in to achieve the compatibility hacks. Signed-off-by: Yakir Yang <a class="moz-txt-link-rfc2396E" href="mailto:ykk@rock-chips.com"><ykk@rock-chips.com></a> --- Changes in v5: - Switch video timing type to "u32", so driver could use "of_property_read_u32" to get the backword timing values. </pre> </blockquote> <pre wrap="">Okay </pre> <blockquote type="cite"> <pre wrap="">Krzysztof suggest me that driver could use the "of_property_read_bool" to get backword timing values, but that interfacs would modify the original drm_display_mode timing directly (whether those properties exists or not). </pre> </blockquote> <pre wrap="">Hmm, I don't understand. You have a: struct video_info { bool h_sync_polarity; bool v_sync_polarity; bool interlaced; }; so what is wrong with: dp_video_config->h_sync_polarity = of_property_read_bool(dp_node, "hsync-active-high"); Is it exactly the same binding as previously? </pre> </blockquote> <pre wrap=""> Yes, it is the same binding as previously. But just a note that we already mark those DT binding as deprecated. +-interlaced: deprecated prop that can parsed frm drm_display_mode. +-vsync-active-high: deprecated prop that can parsed frm drm_display_mode. +-hsync-active-high: deprecated prop that can parsed frm drm_display_mode. For now those values should come from "struct drm_display_mode", and we already parsed them out from "drm_display_mode" before driver provide the backward compatibility. Let's used the "hsync-active-high" example: As for now the code would like: static void analogix_dp_bridge_mode_set(...) { // Parsed timing value from "drm_display_mode" video->h_sync_polarity = !!(mode->flags & DRM_MODE_FLAG_NHSYNC); // Try to detect the deprecated property, providing // the backward compatibility of_property_read_u32(dp_node, "hsync-active-high", &video->h_sync_polarity); /* * In this case, if "hsync-active-high" property haven't been * found, then the video timing "h_sync_polarity" would keep * no change, keeping the parsed value from "drm_display_mode" */ } But if keep the "of_property_read_bool", then code would like: static void analogix_dp_bridge_mode_set(...) { // Parsed timing value from "drm_display_mode" video->h_sync_polarity = !!(mode->flags & DRM_MODE_FLAG_NHSYNC); // Try to detect the deprecated property, providing // the backward compatibility video->h_sync_polarity = of_property_read_bool(dp_node, "hsync-active-high"); /* * In this case, if "hsync-active-high" property haven't been * found, then the video timing "h_sync_polarity" would just * modify to "false". That is the place we don't want, cause * it would always modify the timing value parsed from * "drm_display_mode" */ } </pre> </blockquote> <pre wrap=""> OK, I see the point of overwriting values from drm_display_mode. However I think you changed the binding. I believe the of_property_read_u32() will behave differently for such DTS: exynos_dp { ... hsync-active-high; } It will return -EOVERFLOW which means it would be broken now... </pre> </blockquote> Whoops, thanks for your remind, after try that, I do see over flow error. static void *of_find_property_value_of_size(const struct device_node *np, const char *propname, u32 len) { .... if (len > prop->length) return ERR_PTR(-EOVERFLOW); ... } So I though code should be: if (of_property_read_bool(dp_node, "hsync-active-high")) video->h_sync_polarity = true; And we can't provide full backward compatibility for this property, cause the previous exynos_dp driver would set this timing value to "false" when property not defined, but analogix_dp driver keep this timing value corresponding to "drm_display_mode" when property not found. Thanks, - Yakir <blockquote cite="mid:560B9075.1000304@samsung.com" type="cite"> <pre wrap=""> Best regards, Krzysztof </pre> </blockquote> </body> </html>