[PATCH] drm/bridge/synopsys: dsi: add optional pixel clock

[PATCH] drm/bridge/synopsys: dsi: add optional pixel clock

[Philippe Cornu]

The pixel clock is optional. When available, it offers a better
preciseness for timing computations.

Signed-off-by: Philippe Cornu <philippe.cornu@st.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 24 ++++++++++++++++++------
 1 file changed, 18 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 d9cca4f..8b3787d 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,13 @@ static int dw_mipi_dsi_bridge_attach(struct drm_bridge *bridge)
 		return ERR_PTR(ret);
 	}
 
+	dsi->px_clk = devm_clk_get(dev, "px_clk");
+	if (IS_ERR(dsi->px_clk)) {
+		ret = PTR_ERR(dsi->px_clk);
+		dev_dbg(dev, "Unable to get optional px_clk: %d\n", ret);
+		dsi->px_clk = NULL;
+	}
+
 	/*
 	 * Note that the reset was not defined in the initial device tree, so
 	 * we have to be prepared for it not being found.
-- 
1.9.1

[PATCH] drm/bridge/synopsys: dsi: add optional pixel clock

[Philippe Cornu]

The pixel clock is optional. When available, it offers a better
preciseness for timing computations.

Signed-off-by: Philippe Cornu <philippe.cornu@st.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 24 ++++++++++++++++++------
 1 file changed, 18 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 d9cca4f..8b3787d 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,13 @@ static int dw_mipi_dsi_bridge_attach(struct drm_bridge *bridge)
 		return ERR_PTR(ret);
 	}
 
+	dsi->px_clk = devm_clk_get(dev, "px_clk");
+	if (IS_ERR(dsi->px_clk)) {
+		ret = PTR_ERR(dsi->px_clk);
+		dev_dbg(dev, "Unable to get optional px_clk: %d\n", ret);
+		dsi->px_clk = NULL;
+	}
+
 	/*
 	 * Note that the reset was not defined in the initial device tree, so
 	 * we have to be prepared for it not being found.
-- 
1.9.1

Re: [PATCH] drm/bridge/synopsys: dsi: add optional pixel clock

[Andrzej Hajda]

On 26.10.2017 18:09, Philippe Cornu wrote:
> The pixel clock is optional. When available, it offers a better
> preciseness for timing computations.
>
> Signed-off-by: Philippe Cornu <philippe.cornu@st.com>
> ---
>  drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 24 ++++++++++++++++++------
>  1 file changed, 18 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 d9cca4f..8b3787d 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);

Just small suggestion: if pixel clock rate matters, maybe better is to
fix it in adjusted_mode in mode_fixup callback.

>  	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,13 @@ static int dw_mipi_dsi_bridge_attach(struct drm_bridge *bridge)
>  		return ERR_PTR(ret);
>  	}
>  
> +	dsi->px_clk = devm_clk_get(dev, "px_clk");
> +	if (IS_ERR(dsi->px_clk)) {
> +		ret = PTR_ERR(dsi->px_clk);
> +		dev_dbg(dev, "Unable to get optional px_clk: %d\n", ret);
> +		dsi->px_clk = NULL;
> +	}
> +

devm_clk_get is called from bridge::attach callback, do we have
guarantee that in ::detach callback the clock will be removed?
And what if the clock is defined in dts, but it cannot be get due to
other reasons? I guess the code should fail then, ie you should have
different paths for -ENOENT and for other errors.

 --
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.

Re: [PATCH] drm/bridge/synopsys: dsi: add optional pixel clock

[Andrzej Hajda]

On 26.10.2017 18:09, Philippe Cornu wrote:
> The pixel clock is optional. When available, it offers a better
> preciseness for timing computations.
>
> Signed-off-by: Philippe Cornu <philippe.cornu@st.com>
> ---
>  drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 24 ++++++++++++++++++------
>  1 file changed, 18 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 d9cca4f..8b3787d 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);

Just small suggestion: if pixel clock rate matters, maybe better is to
fix it in adjusted_mode in mode_fixup callback.

>  	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,13 @@ static int dw_mipi_dsi_bridge_attach(struct drm_bridge *bridge)
>  		return ERR_PTR(ret);
>  	}
>  
> +	dsi->px_clk = devm_clk_get(dev, "px_clk");
> +	if (IS_ERR(dsi->px_clk)) {
> +		ret = PTR_ERR(dsi->px_clk);
> +		dev_dbg(dev, "Unable to get optional px_clk: %d\n", ret);
> +		dsi->px_clk = NULL;
> +	}
> +

devm_clk_get is called from bridge::attach callback, do we have
guarantee that in ::detach callback the clock will be removed?
And what if the clock is defined in dts, but it cannot be get due to
other reasons? I guess the code should fail then, ie you should have
different paths for -ENOENT and for other errors.

 --
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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/bridge/synopsys: dsi: add optional pixel clock

[Philipp Zabel]

Hi Philippe,

On Thu, 2017-10-26 at 18:09 +0200, Philippe Cornu wrote:
> The pixel clock is optional. When available, it offers a better
> preciseness for timing computations.
> 
> Signed-off-by: Philippe Cornu <philippe.cornu@st.com>
> ---
>  drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 24 ++++++++++++++++++------
>  1 file changed, 18 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 d9cca4f..8b3787d 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;

I don't understand why is the pixel clock rate divided by 1000 different
from the mode clock in the first place? If px_clk is just the DPI input
pixel clock signal from the display controller, I'd expect the mode to
be adjusted correctly.

> +
> +	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,13 @@ static int dw_mipi_dsi_bridge_attach(struct drm_bridge *bridge)
>  		return ERR_PTR(ret);
>  	}
>  
> +	dsi->px_clk = devm_clk_get(dev, "px_clk");

Is "px_clk" what the pixel clock is called in the datasheet?

If this is probed from DT, the new optional property must be documented
in the binding docs.

> +	if (IS_ERR(dsi->px_clk)) {
> +		ret = PTR_ERR(dsi->px_clk);

Better leave ret == 0 for -ENOENT, but return the error for all others.

> +		dev_dbg(dev, "Unable to get optional px_clk: %d\n", ret);
> +		dsi->px_clk = NULL;
> +	}
> +
>  	/*
>  	 * Note that the reset was not defined in the initial device tree, so
>  	 * we have to be prepared for it not being found.

regards
Philipp

Re: [PATCH] drm/bridge/synopsys: dsi: add optional pixel clock

[Philipp Zabel]

Hi Philippe,

On Thu, 2017-10-26 at 18:09 +0200, Philippe Cornu wrote:
> The pixel clock is optional. When available, it offers a better
> preciseness for timing computations.
> 
> Signed-off-by: Philippe Cornu <philippe.cornu@st.com>
> ---
>  drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 24 ++++++++++++++++++------
>  1 file changed, 18 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 d9cca4f..8b3787d 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;

I don't understand why is the pixel clock rate divided by 1000 different
from the mode clock in the first place? If px_clk is just the DPI input
pixel clock signal from the display controller, I'd expect the mode to
be adjusted correctly.

> +
> +	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,13 @@ static int dw_mipi_dsi_bridge_attach(struct drm_bridge *bridge)
>  		return ERR_PTR(ret);
>  	}
>  
> +	dsi->px_clk = devm_clk_get(dev, "px_clk");

Is "px_clk" what the pixel clock is called in the datasheet?

If this is probed from DT, the new optional property must be documented
in the binding docs.

> +	if (IS_ERR(dsi->px_clk)) {
> +		ret = PTR_ERR(dsi->px_clk);

Better leave ret == 0 for -ENOENT, but return the error for all others.

> +		dev_dbg(dev, "Unable to get optional px_clk: %d\n", ret);
> +		dsi->px_clk = NULL;
> +	}
> +
>  	/*
>  	 * Note that the reset was not defined in the initial device tree, so
>  	 * we have to be prepared for it not being found.

regards
Philipp
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/bridge/synopsys: dsi: add optional pixel clock

[Philippe CORNU]

Hi Andrzej,

On 10/27/2017 08:41 AM, Andrzej Hajda wrote:
> On 26.10.2017 18:09, Philippe Cornu wrote:
>> The pixel clock is optional. When available, it offers a better
>> preciseness for timing computations.
>>
>> Signed-off-by: Philippe Cornu <philippe.cornu@st.com>
>> ---
>>   drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 24 ++++++++++++++++++------
>>   1 file changed, 18 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 d9cca4f..8b3787d 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);
> 
> Just small suggestion: if pixel clock rate matters, maybe better is to
> fix it in adjusted_mode in mode_fixup callback.
> 

Well, it was my first idea but unfortunately, "_adjusted_" stuffs are 
not used by many drivers, and it means that other drivers using this 
generic dw dsi driver will have to handle the "_adjusted_" stuffs too in 
their own crtc or dsi specific parts...

So that is why I have chosen to use the "optional" pixel clock (as 
defined in the documentation): as it is optional, it is up to the 
"specific dt part" to use it or not, whatever the "crtc" driver (often 
handling the set rate, like in stm32 ltdc.c), avoiding then more patches 
in the specific parts and more importantly in the crtc parts...

What is your opinion then?

>>   	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,13 @@ static int dw_mipi_dsi_bridge_attach(struct drm_bridge *bridge)
>>   		return ERR_PTR(ret);
>>   	}
>>   
>> +	dsi->px_clk = devm_clk_get(dev, "px_clk");
>> +	if (IS_ERR(dsi->px_clk)) {
>> +		ret = PTR_ERR(dsi->px_clk);
>> +		dev_dbg(dev, "Unable to get optional px_clk: %d\n", ret);
>> +		dsi->px_clk = NULL;
>> +	}
>> +
> 
> devm_clk_get is called from bridge::attach callback, do we have
> guarantee that in ::detach callback the clock will be removed?
> And what if the clock is defined in dts, but it cannot be get due to
> other reasons? I guess the code should fail then, ie you should have
> different paths for -ENOENT and for other errors.
> 

I will do more tests related to your above comments,

Many thanks for your comments and support,
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.
> 
> 

Re: [PATCH] drm/bridge/synopsys: dsi: add optional pixel clock

[Philippe CORNU]

Hi Andrzej,

On 10/27/2017 08:41 AM, Andrzej Hajda wrote:
> On 26.10.2017 18:09, Philippe Cornu wrote:
>> The pixel clock is optional. When available, it offers a better
>> preciseness for timing computations.
>>
>> Signed-off-by: Philippe Cornu <philippe.cornu@st.com>
>> ---
>>   drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 24 ++++++++++++++++++------
>>   1 file changed, 18 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 d9cca4f..8b3787d 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);
> 
> Just small suggestion: if pixel clock rate matters, maybe better is to
> fix it in adjusted_mode in mode_fixup callback.
> 

Well, it was my first idea but unfortunately, "_adjusted_" stuffs are 
not used by many drivers, and it means that other drivers using this 
generic dw dsi driver will have to handle the "_adjusted_" stuffs too in 
their own crtc or dsi specific parts...

So that is why I have chosen to use the "optional" pixel clock (as 
defined in the documentation): as it is optional, it is up to the 
"specific dt part" to use it or not, whatever the "crtc" driver (often 
handling the set rate, like in stm32 ltdc.c), avoiding then more patches 
in the specific parts and more importantly in the crtc parts...

What is your opinion then?

>>   	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,13 @@ static int dw_mipi_dsi_bridge_attach(struct drm_bridge *bridge)
>>   		return ERR_PTR(ret);
>>   	}
>>   
>> +	dsi->px_clk = devm_clk_get(dev, "px_clk");
>> +	if (IS_ERR(dsi->px_clk)) {
>> +		ret = PTR_ERR(dsi->px_clk);
>> +		dev_dbg(dev, "Unable to get optional px_clk: %d\n", ret);
>> +		dsi->px_clk = NULL;
>> +	}
>> +
> 
> devm_clk_get is called from bridge::attach callback, do we have
> guarantee that in ::detach callback the clock will be removed?
> And what if the clock is defined in dts, but it cannot be get due to
> other reasons? I guess the code should fail then, ie you should have
> different paths for -ENOENT and for other errors.
> 

I will do more tests related to your above comments,

Many thanks for your comments and support,
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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/bridge/synopsys: dsi: add optional pixel clock

[Philippe CORNU]

Hi Philipp,

On 10/27/2017 10:06 AM, Philipp Zabel wrote:
> Hi Philippe,
> 
> On Thu, 2017-10-26 at 18:09 +0200, Philippe Cornu wrote:
>> The pixel clock is optional. When available, it offers a better
>> preciseness for timing computations.
>>
>> Signed-off-by: Philippe Cornu <philippe.cornu@st.com>
>> ---
>>   drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 24 ++++++++++++++++++------
>>   1 file changed, 18 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 d9cca4f..8b3787d 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;
> 
> I don't understand why is the pixel clock rate divided by 1000 different
> from the mode clock in the first place? If px_clk is just the DPI input
> pixel clock signal from the display controller, I'd expect the mode to
> be adjusted correctly.
> 

The display panel requests a given px clk frequency. Then, the set rate 
is performed by the "crtc" (ie. the display controller). Depending of 
the hw platform, the real frequency is different from the requested one 
as pll work with dividers (and sometimes pll are shared between several 
IPs adding more constraints...) so preciseness is not "perfect".

This preciseness is "hw platform dependent" that is why this px clock is 
used here only if the platform populates the related dt.

>> +
>> +	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,13 @@ static int dw_mipi_dsi_bridge_attach(struct drm_bridge *bridge)
>>   		return ERR_PTR(ret);
>>   	}
>>   
>> +	dsi->px_clk = devm_clk_get(dev, "px_clk");
> 
> Is "px_clk" what the pixel clock is called in the datasheet?
> 
> If this is probed from DT, the new optional property must be documented
> in the binding docs.
> 

The px clk (and "px_clk") are already documented in 
Documentation/devicetree/bindings/display/bridge/dw_mipi_dsi.txt as 
optional.

   - "px_clk" is the pixel clock for the DPI/RGB input. (optional)

Do you think bindings is clear enough?

>> +	if (IS_ERR(dsi->px_clk)) {
>> +		ret = PTR_ERR(dsi->px_clk);
> 
> Better leave ret == 0 for -ENOENT, but return the error for all others.
> 

Ok, I will do more test here.

Many thanks for your comments,
Philippe :-)

>> +		dev_dbg(dev, "Unable to get optional px_clk: %d\n", ret);
>> +		dsi->px_clk = NULL;
>> +	}
>> +
>>   	/*
>>   	 * Note that the reset was not defined in the initial device tree, so
>>   	 * we have to be prepared for it not being found.
> 
> regards
> Philipp
> 

Re: [PATCH] drm/bridge/synopsys: dsi: add optional pixel clock

[Philippe CORNU]

Hi Philipp,

On 10/27/2017 10:06 AM, Philipp Zabel wrote:
> Hi Philippe,
> 
> On Thu, 2017-10-26 at 18:09 +0200, Philippe Cornu wrote:
>> The pixel clock is optional. When available, it offers a better
>> preciseness for timing computations.
>>
>> Signed-off-by: Philippe Cornu <philippe.cornu@st.com>
>> ---
>>   drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 24 ++++++++++++++++++------
>>   1 file changed, 18 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 d9cca4f..8b3787d 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;
> 
> I don't understand why is the pixel clock rate divided by 1000 different
> from the mode clock in the first place? If px_clk is just the DPI input
> pixel clock signal from the display controller, I'd expect the mode to
> be adjusted correctly.
> 

The display panel requests a given px clk frequency. Then, the set rate 
is performed by the "crtc" (ie. the display controller). Depending of 
the hw platform, the real frequency is different from the requested one 
as pll work with dividers (and sometimes pll are shared between several 
IPs adding more constraints...) so preciseness is not "perfect".

This preciseness is "hw platform dependent" that is why this px clock is 
used here only if the platform populates the related dt.

>> +
>> +	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,13 @@ static int dw_mipi_dsi_bridge_attach(struct drm_bridge *bridge)
>>   		return ERR_PTR(ret);
>>   	}
>>   
>> +	dsi->px_clk = devm_clk_get(dev, "px_clk");
> 
> Is "px_clk" what the pixel clock is called in the datasheet?
> 
> If this is probed from DT, the new optional property must be documented
> in the binding docs.
> 

The px clk (and "px_clk") are already documented in 
Documentation/devicetree/bindings/display/bridge/dw_mipi_dsi.txt as 
optional.

   - "px_clk" is the pixel clock for the DPI/RGB input. (optional)

Do you think bindings is clear enough?

>> +	if (IS_ERR(dsi->px_clk)) {
>> +		ret = PTR_ERR(dsi->px_clk);
> 
> Better leave ret == 0 for -ENOENT, but return the error for all others.
> 

Ok, I will do more test here.

Many thanks for your comments,
Philippe :-)

>> +		dev_dbg(dev, "Unable to get optional px_clk: %d\n", ret);
>> +		dsi->px_clk = NULL;
>> +	}
>> +
>>   	/*
>>   	 * Note that the reset was not defined in the initial device tree, so
>>   	 * we have to be prepared for it not being found.
> 
> regards
> Philipp
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel