Hi Andrzej, On 01/15/2018 02:52 PM, Andrzej Hajda wrote: > On 12.01.2018 17:25, Philippe Cornu wrote: >> The pixel clock is optional. When available, it offers a better >> preciseness for timing computations and allows to reduce the extra dsi >> bandwidth in burst mode (from ~20% to ~10-12%, hw platform dependent). >> >> Signed-off-by: Philippe Cornu <philippe.cornu@xxxxxx> >> --- >> Changes in v2: Improve px_clk probing in case of ENOENT dt returned value >> (thanks to Philipp Zabel & Andrzej Hajda comments) >> >> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 26 ++++++++++++++++++++------ >> 1 file changed, 20 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >> index c39c7dce20ed..62fcff881b98 100644 >> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >> @@ -225,6 +225,7 @@ struct dw_mipi_dsi { >> void __iomem *base; >> >> struct clk *pclk; >> + struct clk *px_clk; >> >> unsigned int lane_mbps; /* per lane */ >> u32 channel; >> @@ -753,24 +754,28 @@ void dw_mipi_dsi_bridge_mode_set(struct drm_bridge *bridge, >> struct dw_mipi_dsi *dsi = bridge_to_dsi(bridge); >> const struct dw_mipi_dsi_phy_ops *phy_ops = dsi->plat_data->phy_ops; >> void *priv_data = dsi->plat_data->priv_data; >> + struct drm_display_mode px_clk_mode = *mode; >> int ret; >> >> clk_prepare_enable(dsi->pclk); >> >> - ret = phy_ops->get_lane_mbps(priv_data, mode, dsi->mode_flags, >> + if (dsi->px_clk) >> + px_clk_mode.clock = clk_get_rate(dsi->px_clk) / 1000; >> + >> + ret = phy_ops->get_lane_mbps(priv_data, &px_clk_mode, dsi->mode_flags, >> dsi->lanes, dsi->format, &dsi->lane_mbps); >> if (ret) >> DRM_DEBUG_DRIVER("Phy get_lane_mbps() failed\n"); >> >> pm_runtime_get_sync(dsi->dev); >> dw_mipi_dsi_init(dsi); >> - dw_mipi_dsi_dpi_config(dsi, mode); >> + dw_mipi_dsi_dpi_config(dsi, &px_clk_mode); >> dw_mipi_dsi_packet_handler_config(dsi); >> dw_mipi_dsi_video_mode_config(dsi); >> - dw_mipi_dsi_video_packet_config(dsi, mode); >> + dw_mipi_dsi_video_packet_config(dsi, &px_clk_mode); >> dw_mipi_dsi_command_mode_config(dsi); >> - dw_mipi_dsi_line_timer_config(dsi, mode); >> - dw_mipi_dsi_vertical_timing_config(dsi, mode); >> + dw_mipi_dsi_line_timer_config(dsi, &px_clk_mode); >> + dw_mipi_dsi_vertical_timing_config(dsi, &px_clk_mode); >> >> dw_mipi_dsi_dphy_init(dsi); >> dw_mipi_dsi_dphy_timing_config(dsi); >> @@ -784,7 +789,7 @@ void dw_mipi_dsi_bridge_mode_set(struct drm_bridge *bridge, >> >> dw_mipi_dsi_dphy_enable(dsi); >> >> - dw_mipi_dsi_wait_for_two_frames(mode); >> + dw_mipi_dsi_wait_for_two_frames(&px_clk_mode); >> >> /* Switch to cmd mode for panel-bridge pre_enable & panel prepare */ >> dw_mipi_dsi_set_mode(dsi, 0); >> @@ -878,6 +883,15 @@ __dw_mipi_dsi_probe(struct platform_device *pdev, >> return ERR_PTR(ret); >> } >> >> + dsi->px_clk = devm_clk_get(dev, "px_clk"); >> + if (PTR_ERR(dsi->px_clk) == -ENOENT) { >> + dsi->px_clk = NULL; >> + } else if (IS_ERR(dsi->px_clk)) { >> + ret = PTR_ERR(dsi->px_clk); >> + dev_err(dev, "Unable to get optional px_clk: %d\n", ret); >> + dsi->px_clk = NULL; >> + } >> + > As I understand on fail you just log an error and continue? > The code could be slightly simplified, for example: > dsi->px_clk = devm_clk_get(dev, "px_clk"); > if (IS_ERR(dsi->px_clk)) { > ret = PTR_ERR(dsi->px_clk); > if (ret != ENOENT) > dev_err(dev, "Unable to get optional px_clk: %d\n", ret); > dsi->px_clk = NULL; > } > > With or without this change: > > Reviewed-by: Andrzej Hajda <a.hajda@xxxxxxxxxxx> > Thanks for your review. Yes in this version, on fail, I just log an error and continue, especially because this px_clk is "optional" in the documentation. Then your proposal is much better than mine : ) Nevertheless, I wonder now if it could be better to "return" in case of error as we do for others mandatory clocks... So then, the code could be: dsi->px_clk = devm_clk_get(dev, "px_clk"); if (IS_ERR(dsi->px_clk)) { dsi->px_clk = NULL; ret = PTR_ERR(dsi->px_clk); if (ret != ENOENT) { dev_err(dev, "Unable to get optional px_clk: %d\n", ret); return ERR_PTR(ret); } } Do you (or someone else) have a preferred choice? Many thanks, Philippe :-) > -- > Regards > Andrzej > > >> /* >> * Note that the reset was not defined in the initial device tree, so >> * we have to be prepared for it not being found. > > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel