On Thu, 2022-03-17 at 20:02 +0800, Rex-BC Chen wrote:
Hello Xinlei,
On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
From: Jitao Shi jitao.shi@mediatek.com
In order to match the changes of "Use the drm_panel_bridge API", the poweron/poweroff of dsi is extracted from enable/disable and defined as new funcs (pre_enable/post_disable).
Fixes: 2dd8075d2185 ("drm/mediatek: mtk_dsi: Use the drm_panel_bridge API")
Signed-off-by: Jitao Shi jitao.shi@mediatek.com Signed-off-by: Xinlei Lee xinlei.lee@mediatek.com
drivers/gpu/drm/mediatek/mtk_dsi.c | 45 +++++++++++++++++---------
-- 1 file changed, 26 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c index 262c027d8c2f..e33caaca11a7 100644 --- a/drivers/gpu/drm/mediatek/mtk_dsi.c +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c @@ -679,16 +679,6 @@ static void mtk_dsi_poweroff(struct mtk_dsi *dsi) if (--dsi->refcount != 0) return;
- /*
* mtk_dsi_stop() and mtk_dsi_start() is asymmetric, since* mtk_dsi_stop() should be called aftermtk_drm_crtc_atomic_disable(),
* which needs irq for vblank, and mtk_dsi_stop() will disableirq.
* mtk_dsi_start() needs to be called inmtk_output_dsi_enable(),
* after dsi is fully set.*/- mtk_dsi_stop(dsi);
- mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500); mtk_dsi_reset_engine(dsi); mtk_dsi_lane0_ulp_mode_enter(dsi); mtk_dsi_clk_ulp_mode_enter(dsi);
@@ -703,17 +693,9 @@ static void mtk_dsi_poweroff(struct mtk_dsi *dsi)
static void mtk_output_dsi_enable(struct mtk_dsi *dsi) {
int ret;
if (dsi->enabled) return;
ret = mtk_dsi_poweron(dsi);
if (ret < 0) {
DRM_ERROR("failed to power on dsi\n");return;}
mtk_dsi_set_mode(dsi); mtk_dsi_clk_hs_mode(dsi, 1);
@@ -727,7 +709,16 @@ static void mtk_output_dsi_disable(struct mtk_dsi *dsi) if (!dsi->enabled) return;
- mtk_dsi_poweroff(dsi);
- /*
* mtk_dsi_stop() and mtk_dsi_start() is asymmetric, sinceWhy they are asymmetric?
* mtk_dsi_stop() should be called aftermtk_drm_crtc_atomic_disable(),
* which needs irq for vblank, and mtk_dsi_stop() will disableirq.
* mtk_dsi_start() needs to be called inmtk_output_dsi_enable(),
* after dsi is fully set.*/mtk_dsi_stop(dsi);
mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
dsi->enabled = false;
} @@ -765,10 +756,26 @@ static void mtk_dsi_bridge_enable(struct drm_bridge *bridge) mtk_output_dsi_enable(dsi); }
+static void mtk_dsi_bridge_pre_enable(struct drm_bridge *bridge) +{
- struct mtk_dsi *dsi = bridge_to_dsi(bridge);
- mtk_dsi_poweron(dsi);
Should you handle the error of mtk_dsi_poweron? If you failed to mtk_dsi_bridge_pre_enable and do mtk_dsi_bridge_enable, what will happend?
+}
+static void mtk_dsi_bridge_post_disable(struct drm_bridge *bridge) +{
- struct mtk_dsi *dsi = bridge_to_dsi(bridge);
- mtk_dsi_poweroff(dsi);
If you failed to mtk_dsi_bridge_disable and you do mtk_dsi_bridge_post_disable, what will happend? Do you need to handle this?
BRs, Rex
+}
static const struct drm_bridge_funcs mtk_dsi_bridge_funcs = { .attach = mtk_dsi_bridge_attach, .disable = mtk_dsi_bridge_disable, .enable = mtk_dsi_bridge_enable,
- .pre_enable = mtk_dsi_bridge_pre_enable,
- .post_disable = mtk_dsi_bridge_post_disable, .mode_set = mtk_dsi_bridge_mode_set,
};
Hi Rex:
Thanks for your review!
1.Why they are asymmetric? =>My understanding mtk_dsi_stop() and mtk_dsi_start() is to make dsi switch from LP11 and HS mode.DSI has two working modes: If it is cmd mode, the data sent is sent by LP11, and dsi_start is just a signal. In this mode, dsi_stop is not required after sending cmd. If it is video mode, because the data needs to be sent in HS mode, dsi_start is required to make dsi enter HS mode from LP11. After suspend, drm will call dsi_disable, and call dsi_stop to make dsi return from HS mode to LP11 state. Therefore mtk_dsi_stop() and mtk_dsi_start() are asymmetric. For example, in the dsi_host_transfer function, only dsi_stop has no dsi_start operation.
2. Because the return type of pre_enable & post_disable in common code is void type. If there is an error, it will be processed in poweron/poweroff, and the error message will be printed. Do you mean that pre_enable & post_disable needs to accept the poweron/poweroff error return value and then print the error log?
3. If pre_enable fails, there is only a problem with the dsi module, and it does not affect the execution of other modules and enable funcs under drm. Same goes for post_disable & disable.
Best Regrds! xinlei