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 <[email protected]>
---
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;
+ }
+
/*
* Note that the reset was not defined in the initial device tree, so
* we have to be prepared for it not being found.
--
2.15.1
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 <[email protected]>
> ---
> 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 <[email protected]>
--
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.
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 <[email protected]>
>> ---
>> 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 <[email protected]>
>
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.
>
>
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 <[email protected]>
>>> ---
>>> 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 <[email protected]>
>>
> 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.
Hi Brian,
On 01/15/2018 06:11 PM, Andrzej Hajda wrote:
> 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 <[email protected]>
>>>> ---
>>>> 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 <[email protected]>
>>>
>> 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
Before sending a v3 with Andrzej comments, may I ask you please to do a
short review of this patch, particularly the
dw_mipi_dsi_bridge_mode_set() function with the use of the optional
pixel clock.
Many thanks for your support,
Philippe :-)
>
>>
>>
>> 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.
>
>