| Message ID | 1442907477-3283-1-git-send-email-ykk@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@rock-chips.com> > --- > 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? Best regards, Krzysztof > > Changes in v4: > - Provide backword compatibility with samsung. (Krzysztof) > > Changes in v3: > - Dynamic parse video timing info from struct drm_display_mode and > struct drm_display_info. (Thierry) > > Changes in v2: None > > drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 144 +++++++++++++-------- > drivers/gpu/drm/bridge/analogix/analogix_dp_core.h | 8 +- > drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c | 14 +- > 3 files changed, 104 insertions(+), 62 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > index 1e3c8d3..6be139b 100644 > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > @@ -890,8 +890,8 @@ static void analogix_dp_commit(struct analogix_dp_device *dp) > return; > } > > - ret = analogix_dp_set_link_train(dp, dp->video_info->lane_count, > - dp->video_info->link_rate); > + ret = analogix_dp_set_link_train(dp, dp->video_info.lane_count, > + dp->video_info.link_rate); > if (ret) { > dev_err(dp->dev, "unable to do link train\n"); > return; > @@ -1030,6 +1030,85 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge) > dp->dpms_mode = DRM_MODE_DPMS_OFF; > } > > +static void analogix_dp_bridge_mode_set(struct drm_bridge *bridge, > + struct drm_display_mode *orig_mode, > + struct drm_display_mode *mode) > +{ > + struct analogix_dp_device *dp = bridge->driver_private; > + struct drm_display_info *display_info = &dp->connector->display_info; > + struct video_info *video = &dp->video_info; > + struct device_node *dp_node = dp->dev->of_node; > + int vic; > + > + /* Input video interlaces & hsync pol & vsync pol */ > + video->interlaced = !!(mode->flags & DRM_MODE_FLAG_INTERLACE); > + video->v_sync_polarity = !!(mode->flags & DRM_MODE_FLAG_NVSYNC); > + video->h_sync_polarity = !!(mode->flags & DRM_MODE_FLAG_NHSYNC); > + > + /* Input video dynamic_range & colorimetry */ > + vic = drm_match_cea_mode(mode); > + if ((vic == 6) || (vic == 7) || (vic == 21) || (vic == 22) || > + (vic == 2) || (vic == 3) || (vic == 17) || (vic == 18)) { > + video->dynamic_range = CEA; > + video->ycbcr_coeff = COLOR_YCBCR601; > + } else if (vic) { > + video->dynamic_range = CEA; > + video->ycbcr_coeff = COLOR_YCBCR709; > + } else { > + video->dynamic_range = VESA; > + video->ycbcr_coeff = COLOR_YCBCR709; > + } > + > + /* Input vide bpc and color_formats */ > + switch (display_info->bpc) { > + case 12: > + video->color_depth = COLOR_12; > + break; > + case 10: > + video->color_depth = COLOR_10; > + break; > + case 8: > + video->color_depth = COLOR_8; > + break; > + case 6: > + video->color_depth = COLOR_6; > + break; > + default: > + video->color_depth = COLOR_8; > + break; > + } > + if (display_info->color_formats & DRM_COLOR_FORMAT_YCRCB444) > + video->color_space = COLOR_YCBCR444; > + else if (display_info->color_formats & DRM_COLOR_FORMAT_YCRCB422) > + video->color_space = COLOR_YCBCR422; > + else if (display_info->color_formats & DRM_COLOR_FORMAT_RGB444) > + video->color_space = COLOR_RGB; > + else > + video->color_space = COLOR_RGB; > + > + /* > + * NOTE: those property parsing code is used for providing backward > + * compatibility for samsung platform. > + * Due to we used the "of_property_read_u32" interfaces, when this > + * property isn't present, the "video_info" can keep the original > + * values and wouldn't be modified. > + */ > + of_property_read_u32(dp_node, "samsung,color-space", > + &video->color_space); > + of_property_read_u32(dp_node, "samsung,dynamic-range", > + &video->dynamic_range); > + of_property_read_u32(dp_node, "samsung,ycbcr-coeff", > + &video->ycbcr_coeff); > + of_property_read_u32(dp_node, "samsung,color-depth", > + &video->color_depth); > + of_property_read_u32(dp_node, "hsync-active-high", > + &video->h_sync_polarity); > + of_property_read_u32(dp_node, "vsync-active-high", > + &video->v_sync_polarity); > + of_property_read_u32(dp_node, "interlaced", > + &video->interlaced); > +} > + > static void analogix_dp_bridge_nop(struct drm_bridge *bridge) > { > /* do nothing */ > @@ -1040,6 +1119,7 @@ static const struct drm_bridge_funcs analogix_dp_bridge_funcs = { > .disable = analogix_dp_bridge_disable, > .pre_enable = analogix_dp_bridge_nop, > .post_disable = analogix_dp_bridge_nop, > + .mode_set = analogix_dp_bridge_mode_set, > .attach = analogix_dp_bridge_attach, > }; > > @@ -1070,62 +1150,24 @@ static int analogix_dp_create_bridge(struct drm_device *drm_dev, > return 0; > } > > -static struct video_info *analogix_dp_dt_parse_pdata(struct device *dev) > +static int analogix_dp_dt_parse_pdata(struct analogix_dp_device *dp) > { > - struct device_node *dp_node = dev->of_node; > - struct video_info *dp_video_config; > - > - dp_video_config = devm_kzalloc(dev, sizeof(*dp_video_config), > - GFP_KERNEL); > - if (!dp_video_config) > - return ERR_PTR(-ENOMEM); > - > - dp_video_config->h_sync_polarity = > - of_property_read_bool(dp_node, "hsync-active-high"); > - > - dp_video_config->v_sync_polarity = > - of_property_read_bool(dp_node, "vsync-active-high"); > - > - dp_video_config->interlaced = > - of_property_read_bool(dp_node, "interlaced"); > - > - if (of_property_read_u32(dp_node, "samsung,color-space", > - &dp_video_config->color_space)) { > - dev_err(dev, "failed to get color-space\n"); > - return ERR_PTR(-EINVAL); > - } > - > - if (of_property_read_u32(dp_node, "samsung,dynamic-range", > - &dp_video_config->dynamic_range)) { > - dev_err(dev, "failed to get dynamic-range\n"); > - return ERR_PTR(-EINVAL); > - } > - > - if (of_property_read_u32(dp_node, "samsung,ycbcr-coeff", > - &dp_video_config->ycbcr_coeff)) { > - dev_err(dev, "failed to get ycbcr-coeff\n"); > - return ERR_PTR(-EINVAL); > - } > - > - if (of_property_read_u32(dp_node, "samsung,color-depth", > - &dp_video_config->color_depth)) { > - dev_err(dev, "failed to get color-depth\n"); > - return ERR_PTR(-EINVAL); > - } > + struct device_node *dp_node = dp->dev->of_node; > + struct video_info *video_info = &dp->video_info > > if (of_property_read_u32(dp_node, "samsung,link-rate", > - &dp_video_config->link_rate)) { > + &video_info->link_rate)) { > dev_err(dev, "failed to get link-rate\n"); > - return ERR_PTR(-EINVAL); > + return -EINVAL; > } > > if (of_property_read_u32(dp_node, "samsung,lane-count", > - &dp_video_config->lane_count)) { > + &video_info->lane_count)) { > dev_err(dev, "failed to get lane-count\n"); > - return ERR_PTR(-EINVAL); > + return -EINVAL; > } > > - return dp_video_config; > + return 0; > } > > int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev, > @@ -1158,9 +1200,9 @@ int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev, > */ > dp->plat_data = plat_data; > > - dp->video_info = analogix_dp_dt_parse_pdata(&pdev->dev); > - if (IS_ERR(dp->video_info)) > - return PTR_ERR(dp->video_info); > + ret = analogix_dp_dt_parse_pdata(dp); > + if (ret) > + return ret; > > dp->phy = devm_phy_get(dp->dev, "dp"); > if (IS_ERR(dp->phy)) { > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h > index 9a90a18..730486d 100644 > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h > @@ -120,9 +120,9 @@ enum dp_irq_type { > struct video_info { > char *name; > > - bool h_sync_polarity; > - bool v_sync_polarity; > - bool interlaced; > + u32 h_sync_polarity; > + u32 v_sync_polarity; > + u32 interlaced; > > enum color_space color_space; > enum dynamic_range dynamic_range; > @@ -154,7 +154,7 @@ struct analogix_dp_device { > unsigned int irq; > void __iomem *reg_base; > > - struct video_info *video_info; > + struct video_info video_info; > struct link_train link_train; > struct work_struct hotplug_work; > struct phy *phy; > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c > index a388c0a..861097a 100644 > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c > @@ -1084,15 +1084,15 @@ void analogix_dp_set_video_color_format(struct analogix_dp_device *dp) > u32 reg; > > /* Configure the input color depth, color space, dynamic range */ > - reg = (dp->video_info->dynamic_range << IN_D_RANGE_SHIFT) | > - (dp->video_info->color_depth << IN_BPC_SHIFT) | > - (dp->video_info->color_space << IN_COLOR_F_SHIFT); > + reg = (dp->video_info.dynamic_range << IN_D_RANGE_SHIFT) | > + (dp->video_info.color_depth << IN_BPC_SHIFT) | > + (dp->video_info.color_space << IN_COLOR_F_SHIFT); > writel(reg, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_2); > > /* Set Input Color YCbCr Coefficients to ITU601 or ITU709 */ > reg = readl(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_3); > reg &= ~IN_YC_COEFFI_MASK; > - if (dp->video_info->ycbcr_coeff) > + if (dp->video_info.ycbcr_coeff) > reg |= IN_YC_COEFFI_ITU709; > else > reg |= IN_YC_COEFFI_ITU601; > @@ -1229,17 +1229,17 @@ void analogix_dp_config_video_slave_mode(struct analogix_dp_device *dp) > > reg = readl(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_10); > reg &= ~INTERACE_SCAN_CFG; > - reg |= (dp->video_info->interlaced << 2); > + reg |= (dp->video_info.interlaced << 2); > writel(reg, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_10); > > reg = readl(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_10); > reg &= ~VSYNC_POLARITY_CFG; > - reg |= (dp->video_info->v_sync_polarity << 1); > + reg |= (dp->video_info.v_sync_polarity << 1); > writel(reg, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_10); > > reg = readl(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_10); > reg &= ~HSYNC_POLARITY_CFG; > - reg |= (dp->video_info->h_sync_polarity << 0); > + reg |= (dp->video_info.h_sync_polarity << 0); > writel(reg, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_10); > > reg = AUDIO_MODE_SPDIF_MODE | VIDEO_MODE_SLAVE_MODE; >
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@rock-chips.com> >> --- >> 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" */ } Thanks, - Yakir > Best regards, > Krzysztof > >> Changes in v4: >> - Provide backword compatibility with samsung. (Krzysztof) >> >> Changes in v3: >> - Dynamic parse video timing info from struct drm_display_mode and >> struct drm_display_info. (Thierry) >> >> Changes in v2: None >> >> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 144 +++++++++++++-------- >> drivers/gpu/drm/bridge/analogix/analogix_dp_core.h | 8 +- >> drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c | 14 +- >> 3 files changed, 104 insertions(+), 62 deletions(-) >> >> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c >> index 1e3c8d3..6be139b 100644 >> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c >> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c >> @@ -890,8 +890,8 @@ static void analogix_dp_commit(struct analogix_dp_device *dp) >> return; >> } >> >> - ret = analogix_dp_set_link_train(dp, dp->video_info->lane_count, >> - dp->video_info->link_rate); >> + ret = analogix_dp_set_link_train(dp, dp->video_info.lane_count, >> + dp->video_info.link_rate); >> if (ret) { >> dev_err(dp->dev, "unable to do link train\n"); >> return; >> @@ -1030,6 +1030,85 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge) >> dp->dpms_mode = DRM_MODE_DPMS_OFF; >> } >> >> +static void analogix_dp_bridge_mode_set(struct drm_bridge *bridge, >> + struct drm_display_mode *orig_mode, >> + struct drm_display_mode *mode) >> +{ >> + struct analogix_dp_device *dp = bridge->driver_private; >> + struct drm_display_info *display_info = &dp->connector->display_info; >> + struct video_info *video = &dp->video_info; >> + struct device_node *dp_node = dp->dev->of_node; >> + int vic; >> + >> + /* Input video interlaces & hsync pol & vsync pol */ >> + video->interlaced = !!(mode->flags & DRM_MODE_FLAG_INTERLACE); >> + video->v_sync_polarity = !!(mode->flags & DRM_MODE_FLAG_NVSYNC); >> + video->h_sync_polarity = !!(mode->flags & DRM_MODE_FLAG_NHSYNC); >> + >> + /* Input video dynamic_range & colorimetry */ >> + vic = drm_match_cea_mode(mode); >> + if ((vic == 6) || (vic == 7) || (vic == 21) || (vic == 22) || >> + (vic == 2) || (vic == 3) || (vic == 17) || (vic == 18)) { >> + video->dynamic_range = CEA; >> + video->ycbcr_coeff = COLOR_YCBCR601; >> + } else if (vic) { >> + video->dynamic_range = CEA; >> + video->ycbcr_coeff = COLOR_YCBCR709; >> + } else { >> + video->dynamic_range = VESA; >> + video->ycbcr_coeff = COLOR_YCBCR709; >> + } >> + >> + /* Input vide bpc and color_formats */ >> + switch (display_info->bpc) { >> + case 12: >> + video->color_depth = COLOR_12; >> + break; >> + case 10: >> + video->color_depth = COLOR_10; >> + break; >> + case 8: >> + video->color_depth = COLOR_8; >> + break; >> + case 6: >> + video->color_depth = COLOR_6; >> + break; >> + default: >> + video->color_depth = COLOR_8; >> + break; >> + } >> + if (display_info->color_formats & DRM_COLOR_FORMAT_YCRCB444) >> + video->color_space = COLOR_YCBCR444; >> + else if (display_info->color_formats & DRM_COLOR_FORMAT_YCRCB422) >> + video->color_space = COLOR_YCBCR422; >> + else if (display_info->color_formats & DRM_COLOR_FORMAT_RGB444) >> + video->color_space = COLOR_RGB; >> + else >> + video->color_space = COLOR_RGB; >> + >> + /* >> + * NOTE: those property parsing code is used for providing backward >> + * compatibility for samsung platform. >> + * Due to we used the "of_property_read_u32" interfaces, when this >> + * property isn't present, the "video_info" can keep the original >> + * values and wouldn't be modified. >> + */ >> + of_property_read_u32(dp_node, "samsung,color-space", >> + &video->color_space); >> + of_property_read_u32(dp_node, "samsung,dynamic-range", >> + &video->dynamic_range); >> + of_property_read_u32(dp_node, "samsung,ycbcr-coeff", >> + &video->ycbcr_coeff); >> + of_property_read_u32(dp_node, "samsung,color-depth", >> + &video->color_depth); >> + of_property_read_u32(dp_node, "hsync-active-high", >> + &video->h_sync_polarity); >> + of_property_read_u32(dp_node, "vsync-active-high", >> + &video->v_sync_polarity); >> + of_property_read_u32(dp_node, "interlaced", >> + &video->interlaced); >> +} >> + >> static void analogix_dp_bridge_nop(struct drm_bridge *bridge) >> { >> /* do nothing */ >> @@ -1040,6 +1119,7 @@ static const struct drm_bridge_funcs analogix_dp_bridge_funcs = { >> .disable = analogix_dp_bridge_disable, >> .pre_enable = analogix_dp_bridge_nop, >> .post_disable = analogix_dp_bridge_nop, >> + .mode_set = analogix_dp_bridge_mode_set, >> .attach = analogix_dp_bridge_attach, >> }; >> >> @@ -1070,62 +1150,24 @@ static int analogix_dp_create_bridge(struct drm_device *drm_dev, >> return 0; >> } >> >> -static struct video_info *analogix_dp_dt_parse_pdata(struct device *dev) >> +static int analogix_dp_dt_parse_pdata(struct analogix_dp_device *dp) >> { >> - struct device_node *dp_node = dev->of_node; >> - struct video_info *dp_video_config; >> - >> - dp_video_config = devm_kzalloc(dev, sizeof(*dp_video_config), >> - GFP_KERNEL); >> - if (!dp_video_config) >> - return ERR_PTR(-ENOMEM); >> - >> - dp_video_config->h_sync_polarity = >> - of_property_read_bool(dp_node, "hsync-active-high"); >> - >> - dp_video_config->v_sync_polarity = >> - of_property_read_bool(dp_node, "vsync-active-high"); >> - >> - dp_video_config->interlaced = >> - of_property_read_bool(dp_node, "interlaced"); >> - >> - if (of_property_read_u32(dp_node, "samsung,color-space", >> - &dp_video_config->color_space)) { >> - dev_err(dev, "failed to get color-space\n"); >> - return ERR_PTR(-EINVAL); >> - } >> - >> - if (of_property_read_u32(dp_node, "samsung,dynamic-range", >> - &dp_video_config->dynamic_range)) { >> - dev_err(dev, "failed to get dynamic-range\n"); >> - return ERR_PTR(-EINVAL); >> - } >> - >> - if (of_property_read_u32(dp_node, "samsung,ycbcr-coeff", >> - &dp_video_config->ycbcr_coeff)) { >> - dev_err(dev, "failed to get ycbcr-coeff\n"); >> - return ERR_PTR(-EINVAL); >> - } >> - >> - if (of_property_read_u32(dp_node, "samsung,color-depth", >> - &dp_video_config->color_depth)) { >> - dev_err(dev, "failed to get color-depth\n"); >> - return ERR_PTR(-EINVAL); >> - } >> + struct device_node *dp_node = dp->dev->of_node; >> + struct video_info *video_info = &dp->video_info >> >> if (of_property_read_u32(dp_node, "samsung,link-rate", >> - &dp_video_config->link_rate)) { >> + &video_info->link_rate)) { >> dev_err(dev, "failed to get link-rate\n"); >> - return ERR_PTR(-EINVAL); >> + return -EINVAL; >> } >> >> if (of_property_read_u32(dp_node, "samsung,lane-count", >> - &dp_video_config->lane_count)) { >> + &video_info->lane_count)) { >> dev_err(dev, "failed to get lane-count\n"); >> - return ERR_PTR(-EINVAL); >> + return -EINVAL; >> } >> >> - return dp_video_config; >> + return 0; >> } >> >> int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev, >> @@ -1158,9 +1200,9 @@ int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev, >> */ >> dp->plat_data = plat_data; >> >> - dp->video_info = analogix_dp_dt_parse_pdata(&pdev->dev); >> - if (IS_ERR(dp->video_info)) >> - return PTR_ERR(dp->video_info); >> + ret = analogix_dp_dt_parse_pdata(dp); >> + if (ret) >> + return ret; >> >> dp->phy = devm_phy_get(dp->dev, "dp"); >> if (IS_ERR(dp->phy)) { >> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h >> index 9a90a18..730486d 100644 >> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h >> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h >> @@ -120,9 +120,9 @@ enum dp_irq_type { >> struct video_info { >> char *name; >> >> - bool h_sync_polarity; >> - bool v_sync_polarity; >> - bool interlaced; >> + u32 h_sync_polarity; >> + u32 v_sync_polarity; >> + u32 interlaced; >> >> enum color_space color_space; >> enum dynamic_range dynamic_range; >> @@ -154,7 +154,7 @@ struct analogix_dp_device { >> unsigned int irq; >> void __iomem *reg_base; >> >> - struct video_info *video_info; >> + struct video_info video_info; >> struct link_train link_train; >> struct work_struct hotplug_work; >> struct phy *phy; >> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c >> index a388c0a..861097a 100644 >> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c >> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c >> @@ -1084,15 +1084,15 @@ void analogix_dp_set_video_color_format(struct analogix_dp_device *dp) >> u32 reg; >> >> /* Configure the input color depth, color space, dynamic range */ >> - reg = (dp->video_info->dynamic_range << IN_D_RANGE_SHIFT) | >> - (dp->video_info->color_depth << IN_BPC_SHIFT) | >> - (dp->video_info->color_space << IN_COLOR_F_SHIFT); >> + reg = (dp->video_info.dynamic_range << IN_D_RANGE_SHIFT) | >> + (dp->video_info.color_depth << IN_BPC_SHIFT) | >> + (dp->video_info.color_space << IN_COLOR_F_SHIFT); >> writel(reg, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_2); >> >> /* Set Input Color YCbCr Coefficients to ITU601 or ITU709 */ >> reg = readl(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_3); >> reg &= ~IN_YC_COEFFI_MASK; >> - if (dp->video_info->ycbcr_coeff) >> + if (dp->video_info.ycbcr_coeff) >> reg |= IN_YC_COEFFI_ITU709; >> else >> reg |= IN_YC_COEFFI_ITU601; >> @@ -1229,17 +1229,17 @@ void analogix_dp_config_video_slave_mode(struct analogix_dp_device *dp) >> >> reg = readl(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_10); >> reg &= ~INTERACE_SCAN_CFG; >> - reg |= (dp->video_info->interlaced << 2); >> + reg |= (dp->video_info.interlaced << 2); >> writel(reg, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_10); >> >> reg = readl(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_10); >> reg &= ~VSYNC_POLARITY_CFG; >> - reg |= (dp->video_info->v_sync_polarity << 1); >> + reg |= (dp->video_info.v_sync_polarity << 1); >> writel(reg, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_10); >> >> reg = readl(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_10); >> reg &= ~HSYNC_POLARITY_CFG; >> - reg |= (dp->video_info->h_sync_polarity << 0); >> + reg |= (dp->video_info.h_sync_polarity << 0); >> writel(reg, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_10); >> >> reg = AUDIO_MODE_SPDIF_MODE | VIDEO_MODE_SLAVE_MODE; >> > > >
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@rock-chips.com> >>> --- >>> 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
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 <ykk@rock-chips.com> >>>> --- >>>> 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; 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 > Best regards, > Krzysztof > > > > >
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 <ykk@rock-chips.com> >>>>> --- >>>>> 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
Hi Krzysztof, On 09/30/2015 04:26 PM, Krzysztof Kozlowski wrote: > 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 <ykk@rock-chips.com> >>>>>> --- >>>>>> 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... Hmm... as I know the timing polarity would influence something like: - CTS test - HDCP function But I though it's more likely that driver would made those functions failed if hard code the timing polarity. And I think it would be better to get timing polarity from "drm_display_mode". Caused the analogix_dp driver have called the drm_add_edid_modes() that function would parse the EDID "detailed timing" block which contained the correct timing message that panel request. Besides I see the exynos_fmid driver already setup the timing polarity from "drm_display_mode", and there is no doubt that exynos dp should set the same polarity with fmid driver (I guess, just notice that fmid is a kind of CTRC driver). That's to say parsing timing polarity dynamically would give more chances to make those functions works. Thanks, - Yakir > Best regards, > Krzysztof > > > > "
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c index 1e3c8d3..6be139b 100644 --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c @@ -890,8 +890,8 @@ static void analogix_dp_commit(struct analogix_dp_device *dp) return; } - ret = analogix_dp_set_link_train(dp, dp->video_info->lane_count, - dp->video_info->link_rate); + ret = analogix_dp_set_link_train(dp, dp->video_info.lane_count, + dp->video_info.link_rate); if (ret) { dev_err(dp->dev, "unable to do link train\n"); return; @@ -1030,6 +1030,85 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge) dp->dpms_mode = DRM_MODE_DPMS_OFF; } +static void analogix_dp_bridge_mode_set(struct drm_bridge *bridge, + struct drm_display_mode *orig_mode, + struct drm_display_mode *mode) +{ + struct analogix_dp_device *dp = bridge->driver_private; + struct drm_display_info *display_info = &dp->connector->display_info; + struct video_info *video = &dp->video_info; + struct device_node *dp_node = dp->dev->of_node; + int vic; + + /* Input video interlaces & hsync pol & vsync pol */ + video->interlaced = !!(mode->flags & DRM_MODE_FLAG_INTERLACE); + video->v_sync_polarity = !!(mode->flags & DRM_MODE_FLAG_NVSYNC); + video->h_sync_polarity = !!(mode->flags & DRM_MODE_FLAG_NHSYNC); + + /* Input video dynamic_range & colorimetry */ + vic = drm_match_cea_mode(mode); + if ((vic == 6) || (vic == 7) || (vic == 21) || (vic == 22) || + (vic == 2) || (vic == 3) || (vic == 17) || (vic == 18)) { + video->dynamic_range = CEA; + video->ycbcr_coeff = COLOR_YCBCR601; + } else if (vic) { + video->dynamic_range = CEA; + video->ycbcr_coeff = COLOR_YCBCR709; + } else { + video->dynamic_range = VESA; + video->ycbcr_coeff = COLOR_YCBCR709; + } + + /* Input vide bpc and color_formats */ + switch (display_info->bpc) { + case 12: + video->color_depth = COLOR_12; + break; + case 10: + video->color_depth = COLOR_10; + break; + case 8: + video->color_depth = COLOR_8; + break; + case 6: + video->color_depth = COLOR_6; + break; + default: + video->color_depth = COLOR_8; + break; + } + if (display_info->color_formats & DRM_COLOR_FORMAT_YCRCB444) + video->color_space = COLOR_YCBCR444; + else if (display_info->color_formats & DRM_COLOR_FORMAT_YCRCB422) + video->color_space = COLOR_YCBCR422; + else if (display_info->color_formats & DRM_COLOR_FORMAT_RGB444) + video->color_space = COLOR_RGB; + else + video->color_space = COLOR_RGB; + + /* + * NOTE: those property parsing code is used for providing backward + * compatibility for samsung platform. + * Due to we used the "of_property_read_u32" interfaces, when this + * property isn't present, the "video_info" can keep the original + * values and wouldn't be modified. + */ + of_property_read_u32(dp_node, "samsung,color-space", + &video->color_space); + of_property_read_u32(dp_node, "samsung,dynamic-range", + &video->dynamic_range); + of_property_read_u32(dp_node, "samsung,ycbcr-coeff", + &video->ycbcr_coeff); + of_property_read_u32(dp_node, "samsung,color-depth", + &video->color_depth); + of_property_read_u32(dp_node, "hsync-active-high", + &video->h_sync_polarity); + of_property_read_u32(dp_node, "vsync-active-high", + &video->v_sync_polarity); + of_property_read_u32(dp_node, "interlaced", + &video->interlaced); +} + static void analogix_dp_bridge_nop(struct drm_bridge *bridge) { /* do nothing */ @@ -1040,6 +1119,7 @@ static const struct drm_bridge_funcs analogix_dp_bridge_funcs = { .disable = analogix_dp_bridge_disable, .pre_enable = analogix_dp_bridge_nop, .post_disable = analogix_dp_bridge_nop, + .mode_set = analogix_dp_bridge_mode_set, .attach = analogix_dp_bridge_attach, }; @@ -1070,62 +1150,24 @@ static int analogix_dp_create_bridge(struct drm_device *drm_dev, return 0; } -static struct video_info *analogix_dp_dt_parse_pdata(struct device *dev) +static int analogix_dp_dt_parse_pdata(struct analogix_dp_device *dp) { - struct device_node *dp_node = dev->of_node; - struct video_info *dp_video_config; - - dp_video_config = devm_kzalloc(dev, sizeof(*dp_video_config), - GFP_KERNEL); - if (!dp_video_config) - return ERR_PTR(-ENOMEM); - - dp_video_config->h_sync_polarity = - of_property_read_bool(dp_node, "hsync-active-high"); - - dp_video_config->v_sync_polarity = - of_property_read_bool(dp_node, "vsync-active-high"); - - dp_video_config->interlaced = - of_property_read_bool(dp_node, "interlaced"); - - if (of_property_read_u32(dp_node, "samsung,color-space", - &dp_video_config->color_space)) { - dev_err(dev, "failed to get color-space\n"); - return ERR_PTR(-EINVAL); - } - - if (of_property_read_u32(dp_node, "samsung,dynamic-range", - &dp_video_config->dynamic_range)) { - dev_err(dev, "failed to get dynamic-range\n"); - return ERR_PTR(-EINVAL); - } - - if (of_property_read_u32(dp_node, "samsung,ycbcr-coeff", - &dp_video_config->ycbcr_coeff)) { - dev_err(dev, "failed to get ycbcr-coeff\n"); - return ERR_PTR(-EINVAL); - } - - if (of_property_read_u32(dp_node, "samsung,color-depth", - &dp_video_config->color_depth)) { - dev_err(dev, "failed to get color-depth\n"); - return ERR_PTR(-EINVAL); - } + struct device_node *dp_node = dp->dev->of_node; + struct video_info *video_info = &dp->video_info if (of_property_read_u32(dp_node, "samsung,link-rate", - &dp_video_config->link_rate)) { + &video_info->link_rate)) { dev_err(dev, "failed to get link-rate\n"); - return ERR_PTR(-EINVAL); + return -EINVAL; } if (of_property_read_u32(dp_node, "samsung,lane-count", - &dp_video_config->lane_count)) { + &video_info->lane_count)) { dev_err(dev, "failed to get lane-count\n"); - return ERR_PTR(-EINVAL); + return -EINVAL; } - return dp_video_config; + return 0; } int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev, @@ -1158,9 +1200,9 @@ int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev, */ dp->plat_data = plat_data; - dp->video_info = analogix_dp_dt_parse_pdata(&pdev->dev); - if (IS_ERR(dp->video_info)) - return PTR_ERR(dp->video_info); + ret = analogix_dp_dt_parse_pdata(dp); + if (ret) + return ret; dp->phy = devm_phy_get(dp->dev, "dp"); if (IS_ERR(dp->phy)) { diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h index 9a90a18..730486d 100644 --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h @@ -120,9 +120,9 @@ enum dp_irq_type { struct video_info { char *name; - bool h_sync_polarity; - bool v_sync_polarity; - bool interlaced; + u32 h_sync_polarity; + u32 v_sync_polarity; + u32 interlaced; enum color_space color_space; enum dynamic_range dynamic_range; @@ -154,7 +154,7 @@ struct analogix_dp_device { unsigned int irq; void __iomem *reg_base; - struct video_info *video_info; + struct video_info video_info; struct link_train link_train; struct work_struct hotplug_work; struct phy *phy; diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c index a388c0a..861097a 100644 --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c @@ -1084,15 +1084,15 @@ void analogix_dp_set_video_color_format(struct analogix_dp_device *dp) u32 reg; /* Configure the input color depth, color space, dynamic range */ - reg = (dp->video_info->dynamic_range << IN_D_RANGE_SHIFT) | - (dp->video_info->color_depth << IN_BPC_SHIFT) | - (dp->video_info->color_space << IN_COLOR_F_SHIFT); + reg = (dp->video_info.dynamic_range << IN_D_RANGE_SHIFT) | + (dp->video_info.color_depth << IN_BPC_SHIFT) | + (dp->video_info.color_space << IN_COLOR_F_SHIFT); writel(reg, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_2); /* Set Input Color YCbCr Coefficients to ITU601 or ITU709 */ reg = readl(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_3); reg &= ~IN_YC_COEFFI_MASK; - if (dp->video_info->ycbcr_coeff) + if (dp->video_info.ycbcr_coeff) reg |= IN_YC_COEFFI_ITU709; else reg |= IN_YC_COEFFI_ITU601; @@ -1229,17 +1229,17 @@ void analogix_dp_config_video_slave_mode(struct analogix_dp_device *dp) reg = readl(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_10); reg &= ~INTERACE_SCAN_CFG; - reg |= (dp->video_info->interlaced << 2); + reg |= (dp->video_info.interlaced << 2); writel(reg, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_10); reg = readl(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_10); reg &= ~VSYNC_POLARITY_CFG; - reg |= (dp->video_info->v_sync_polarity << 1); + reg |= (dp->video_info.v_sync_polarity << 1); writel(reg, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_10); reg = readl(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_10); reg &= ~HSYNC_POLARITY_CFG; - reg |= (dp->video_info->h_sync_polarity << 0); + reg |= (dp->video_info.h_sync_polarity << 0); writel(reg, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_10); reg = AUDIO_MODE_SPDIF_MODE | VIDEO_MODE_SLAVE_MODE;
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@rock-chips.com> --- Changes in v5: - Switch video timing type to "u32", so driver could use "of_property_read_u32" to get the backword timing values. 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). Changes in v4: - Provide backword compatibility with samsung. (Krzysztof) Changes in v3: - Dynamic parse video timing info from struct drm_display_mode and struct drm_display_info. (Thierry) Changes in v2: None drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 144 +++++++++++++-------- drivers/gpu/drm/bridge/analogix/analogix_dp_core.h | 8 +- drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c | 14 +- 3 files changed, 104 insertions(+), 62 deletions(-)