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 <ykk@xxxxxxxxxxxxxx>
>>> ---
>>> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
