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

[Andrzej Hajda]

On 15.01.2018 15:40, Philippe CORNU wrote:
> 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?

No strong feelings, but I would slightly prefer current version: error
is reported but the driver tries to do the best to continue work.
On the other side it increases risk that the error will be ignored and
potential bug not fixed.
Choice between robustness and strictness.

Regards
Andrzej

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