From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754912AbbI3I0m (ORCPT ); Wed, 30 Sep 2015 04:26:42 -0400 Received: from mailout2.w1.samsung.com ([210.118.77.12]:64032 "EHLO mailout2.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754314AbbI3I0f (ORCPT ); Wed, 30 Sep 2015 04:26:35 -0400 X-AuditID: cbfec7f5-f794b6d000001495-42-560b9cb79651 Subject: Re: [PATCH v5 05/17] drm: bridge: analogix/dp: dynamic parse sync_pol & interlace & dynamic_range To: Yakir Yang , Inki Dae , Andrzej Hajda , Joonyoung Shim , Seung-Woo Kim , Kyungmin Park , Jingoo Han , Heiko Stuebner , Mark Yao , Thierry Reding , joe@perches.com, Rob Herring References: <1442906428-2609-1-git-send-email-ykk@rock-chips.com> <1442907477-3283-1-git-send-email-ykk@rock-chips.com> <560B73D4.30109@samsung.com> <560B8CF1.7050102@rock-chips.com> <560B9075.1000304@samsung.com> <560B9B33.2060409@rock-chips.com> Cc: David Airlie , Russell King , djkurtz@chromium.org, dianders@chromium.org, Sean Paul , Kukjin Kim , Kumar Gala , emil.l.velikov@gmail.com, Ian Campbell , Gustavo Padovan , Kishon Vijay Abraham I , Pawel Moll , ajaynumb@gmail.com, robherring2@gmail.com, Andy Yan , dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-samsung-soc@vger.kernel.org, linux-rockchip@lists.infradead.org, linux-arm-kernel@lists.infradead.org From: Krzysztof Kozlowski Message-id: <560B9CAA.9080109@samsung.com> Date: Wed, 30 Sep 2015 17:26:18 +0900 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-version: 1.0 In-reply-to: <560B9B33.2060409@rock-chips.com> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA02SW0iTYRjHe7/3O01bfS0PL1YEg0IG2fHiKSqKLvxugg4iaVBN/VLJ6dpU MqhM80LLU5PUzTxQZq6VooImKw+spNKleYgSp2SheVaWhWXm0si73//5//53D48VuYwPHxkd K+mi1VFK1o1+87ulc1ttgXvgjiS7D3x8Ymcg3f6KgsqxEgyZbz8gKLIt3toeNLFwPa+Iga5v kyxYux0UZI6VMDDzKIWDhU+jDNhHyhHc7s+i4aEznwNT/yANw44OGkaHd0Lm4CiG9i+3WGhL GuOgarCHgc76AhZmBhYw5L19TkFvpxzqchopyMp9TEPKMxsH32dnWeirbEOQZ/jKQu/PNWBP NHCHlKKl0ILEG8m3WNGU2EGLnRnplDg/9J4Wnxr7OLG8zMmKVeZUVqydHWDE/pstlFh9/5qY njzBik5zDxbnjQ20mFFjRmJtTyE+pgh22x8mRUXGS7rtB8+5ReTeTaO1c1sv9VSMcImoYVMa kvFE2EMsjufUEnuRdkcFm4bceIVQioj1TvZycCKSUdP+11ovRJH2rmnGVXgI1ZjcG7dSS9Y8 Ijmp77ArYOENQ4ZsM4xrwgq7SXXZfdbFckFFHGYL7WJa2EKKW1uxiz2FU+R12/iys478MDgW HZ6XCX6kO+myC/Ei9neoXAYWNpNqyzjOQoJxxcD43zKusIoRNiNPKS5Uqw8J1+zy06s1+rjo cL/QGE0VWnqXb3Wo9OW+ZiTwSLlaPiy4ByoYdbw+QdOMCI+VHvKtWYsneZg64bKkizmri4uS 9M1oA08rveX59ZMBCiFcHStdkCStpPvXUrzMJxGZs/2PD/n6Hm3c4nXxRW9KU/Lc1Jls26/Z 03u9g/1LQmrooU8eWuvxZtxi4h4FhWOtTGXfOPVds6qYC2i8meY3eSXT092QwKyeMFk8Tybw hwemHl6/GrxB/i7fdoJe2z3dOuE0pOakDB4o6DU9OxIEc+VKWeTnl6/dz8ccmGGPKWl9hHqn Cuv06j/l8vMTKgMAAA== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 30.09.2015 17:20, Yakir Yang wrote: > Hi Krzysztof, > > On 09/30/2015 03:34 PM, Krzysztof Kozlowski wrote: >> On 30.09.2015 16:19, Yakir Yang wrote: >>> Hi Krzysztof, >>> >>> On 09/30/2015 01:32 PM, Krzysztof Kozlowski wrote: >>>> On 22.09.2015 16:37, Yakir Yang wrote: >>>>> 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 >>>>> --- >>>>> Changes in v5: >>>>> - Switch video timing type to "u32", so driver could use "of_property_read_u32" >>>>> to get the backword timing values. >>>> Okay >>>> >>>>> 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). >>>> 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? >>> 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" >>> */ >>> } >>> >> 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... > > 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; Looks good. > > 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. Indeed, the behaviour changes. I don't know if this is important issue... Best regards, Krzysztof