From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754893AbbI3HfH (ORCPT ); Wed, 30 Sep 2015 03:35:07 -0400 Received: from mailout4.w1.samsung.com ([210.118.77.14]:12773 "EHLO mailout4.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755170AbbI3Heg (ORCPT ); Wed, 30 Sep 2015 03:34:36 -0400 X-AuditID: cbfec7f4-f79c56d0000012ee-f0-560b90880175 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> 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: <560B9075.1000304@samsung.com> Date: Wed, 30 Sep 2015 16:34:13 +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: <560B8CF1.7050102@rock-chips.com> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA02Sb0zMcRzHfX/f3+93v47j6yRfWWyHoa0jPPjMzGyefJlhRloP6OQn1h+5 K9QDzr+NuDrOpi4iVDoXratFi9LyN/09HWU6Srvo0uQcm4VOjGevz/v9+nwefSSszhFCpd3J qbI+WZeoEZV8449HHREnzOOjFr29Q6HrZrMApuYnHJR5CzBkt3QiuNQwmjUV3RfhcM4lAZ5/ GRKhpqObg2xvgQDDN44r4GfPgADNH0oQnHWbebjuy1VAnruXh/7uNh4G+iMhu3cAQ2vfaRGa jngVUN7rEsBZfUGE4Tc/MeS03OPglVMFt8/VcWA+X8rD8bsNCvjq94vwuqwJQY7lvQivvk+E ZqNFsVLD7Pl2xI4dPS2yPGMbz5xZJo6NeF7w7I71tYKVFPtEVm47KbIq/xuBuU894pjj2iFm OvpRZD6bC7MRay3PsipsiFW58vEGdYxy+Q45cfc+Wb9wRaxy1+f7xTjFG3bgi79cYUQFIZko SKJkKe3qzOXGOIS2dt8SM5FSUpNCRPPOmPDY4EO0I8+pCFhTSCJtff5JCBTBxIHp1cEabsyq RvRbe+nvBpNGgXoahoXAikiWUEfxNTHAKhJOn7osOMA8mUvtI/2/eSqJpk+bBv84k+k3Szcf 4CCipWUP/KMsjR7VUndbeCDGZBZ12AexGRHrfxvWf5b1P+sywjY0VU6LSzFsj0+K1Bp0SYa0 5Hht3J6kcjT2L77b6OrDZfWISEgzQdVPxkepBd0+Q3pSPaIS1gSrKjNHI9UOXXqGrN+zTZ+W KBvq0QyJ10xTXawe2qQm8bpUOUGWU2T935aTgkKNSNxYFBv2/t1WjkV/nmk3r9y/eERbubp2 S488iK359mfn4mfHrDrrsA0Vbl4gTp5/2XjlzOFJWQkhntAolt44Ljczwud9GbE+6KAroe+x JaNCadRvJPySpZXTq0w+2l6zM7RmnrOyzN0yUx/8ds3asDkvNJZ2sm6vJ7LL0xOD6zS8YZcu MhzrDbpfg3ffRisDAAA= Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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... Best regards, Krzysztof