[PATCH v2 0/4] MediaTek DRM - DSI driver cleanups

[PATCH v2 0/4] MediaTek DRM - DSI driver cleanups

[AngeloGioacchino Del Regno]

Changes in v2:
 - Rebased over next-20231213

This series performs some cleanups for mtk_dsi, enhancing human
readability, using kernel provided macros where possible and
also reducing code size.

Tested on MT8173 and MT8192 Chromebooks (using a DSI<->DP bridge)
and on MT6795 Sony Xperia M5 (DSI video mode panel).

AngeloGioacchino Del Regno (4):
  drm/mediatek: dsi: Use GENMASK() for register mask definitions
  drm/mediatek: dsi: Cleanup functions mtk_dsi_ps_control{_vact}()
  drm/mediatek: dsi: Use bitfield macros where useful
  drm/mediatek: dsi: Replace open-coded instance of HZ_PER_MHZ

 drivers/gpu/drm/mediatek/mtk_dsi.c | 198 +++++++++++++----------------
 1 file changed, 88 insertions(+), 110 deletions(-)

-- 
2.43.0

[PATCH v2 1/4] drm/mediatek: dsi: Use GENMASK() for register mask definitions

[AngeloGioacchino Del Regno]

Change magic numerical masks with usage of the GENMASK() macro
to improve readability.

This commit brings no functional changes.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dsi.c | 46 ++++++++++++++++--------------
 1 file changed, 24 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index a2fdfc8ddb15..23d2c5be8dbb 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -58,18 +58,18 @@
 
 #define DSI_TXRX_CTRL		0x18
 #define VC_NUM				BIT(1)
-#define LANE_NUM			(0xf << 2)
+#define LANE_NUM			GENMASK(5, 2)
 #define DIS_EOT				BIT(6)
 #define NULL_EN				BIT(7)
 #define TE_FREERUN			BIT(8)
 #define EXT_TE_EN			BIT(9)
 #define EXT_TE_EDGE			BIT(10)
-#define MAX_RTN_SIZE			(0xf << 12)
+#define MAX_RTN_SIZE			GENMASK(15, 12)
 #define HSTX_CKLP_EN			BIT(16)
 
 #define DSI_PSCTRL		0x1c
-#define DSI_PS_WC			0x3fff
-#define DSI_PS_SEL			(3 << 16)
+#define DSI_PS_WC			GENMASK(14, 0)
+#define DSI_PS_SEL			GENMASK(19, 16)
 #define PACKED_PS_16BIT_RGB565		(0 << 16)
 #define LOOSELY_PS_18BIT_RGB666		(1 << 16)
 #define PACKED_PS_18BIT_RGB666		(2 << 16)
@@ -109,26 +109,27 @@
 #define LD0_WAKEUP_EN			BIT(2)
 
 #define DSI_PHY_TIMECON0	0x110
-#define LPX				(0xff << 0)
-#define HS_PREP				(0xff << 8)
-#define HS_ZERO				(0xff << 16)
-#define HS_TRAIL			(0xff << 24)
+#define LPX				GENMASK(7, 0)
+#define HS_PREP				GENMASK(15, 8)
+#define HS_ZERO				GENMASK(23, 16)
+#define HS_TRAIL			GENMASK(31, 24)
 
 #define DSI_PHY_TIMECON1	0x114
-#define TA_GO				(0xff << 0)
-#define TA_SURE				(0xff << 8)
-#define TA_GET				(0xff << 16)
-#define DA_HS_EXIT			(0xff << 24)
+#define TA_GO				GENMASK(7, 0)
+#define TA_SURE				GENMASK(15, 8)
+#define TA_GET				GENMASK(23, 16)
+#define DA_HS_EXIT			GENMASK(31, 24)
 
 #define DSI_PHY_TIMECON2	0x118
-#define CONT_DET			(0xff << 0)
-#define CLK_ZERO			(0xff << 16)
-#define CLK_TRAIL			(0xff << 24)
+#define CONT_DET			GENMASK(7, 0)
+#define DA_HS_SYNC			GENMASK(15, 8)
+#define CLK_ZERO			GENMASK(23, 16)
+#define CLK_TRAIL			GENMASK(31, 24)
 
 #define DSI_PHY_TIMECON3	0x11c
-#define CLK_HS_PREP			(0xff << 0)
-#define CLK_HS_POST			(0xff << 8)
-#define CLK_HS_EXIT			(0xff << 16)
+#define CLK_HS_PREP			GENMASK(7, 0)
+#define CLK_HS_POST			GENMASK(15, 8)
+#define CLK_HS_EXIT			GENMASK(23, 16)
 
 #define DSI_VM_CMD_CON		0x130
 #define VM_CMD_EN			BIT(0)
@@ -138,13 +139,14 @@
 #define FORCE_COMMIT			BIT(0)
 #define BYPASS_SHADOW			BIT(1)
 
-#define CONFIG				(0xff << 0)
+/* CMDQ related bits */
+#define CONFIG				GENMASK(7, 0)
 #define SHORT_PACKET			0
 #define LONG_PACKET			2
 #define BTA				BIT(2)
-#define DATA_ID				(0xff << 8)
-#define DATA_0				(0xff << 16)
-#define DATA_1				(0xff << 24)
+#define DATA_ID				GENMASK(15, 8)
+#define DATA_0				GENMASK(23, 16)
+#define DATA_1				GENMASK(31, 24)
 
 #define NS_TO_CYCLE(n, c)    ((n) / (c) + (((n) % (c)) ? 1 : 0))
 
-- 
2.43.0

[PATCH v2 2/4] drm/mediatek: dsi: Cleanup functions mtk_dsi_ps_control{_vact}()

[AngeloGioacchino Del Regno]

Function mtk_dsi_ps_control() is a subset of mtk_dsi_ps_control_vact():
merge the two in one mtk_dsi_ps_control() function by adding one
function parameter `config_vact` which, when true, writes the VACT
related registers.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dsi.c | 76 +++++++++---------------------
 1 file changed, 23 insertions(+), 53 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index 23d2c5be8dbb..b618e2e31022 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -352,40 +352,6 @@ static void mtk_dsi_set_vm_cmd(struct mtk_dsi *dsi)
 	mtk_dsi_mask(dsi, DSI_VM_CMD_CON, TS_VFP_EN, TS_VFP_EN);
 }
 
-static void mtk_dsi_ps_control_vact(struct mtk_dsi *dsi)
-{
-	struct videomode *vm = &dsi->vm;
-	u32 dsi_buf_bpp, ps_wc;
-	u32 ps_bpp_mode;
-
-	if (dsi->format == MIPI_DSI_FMT_RGB565)
-		dsi_buf_bpp = 2;
-	else
-		dsi_buf_bpp = 3;
-
-	ps_wc = vm->hactive * dsi_buf_bpp;
-	ps_bpp_mode = ps_wc;
-
-	switch (dsi->format) {
-	case MIPI_DSI_FMT_RGB888:
-		ps_bpp_mode |= PACKED_PS_24BIT_RGB888;
-		break;
-	case MIPI_DSI_FMT_RGB666:
-		ps_bpp_mode |= PACKED_PS_18BIT_RGB666;
-		break;
-	case MIPI_DSI_FMT_RGB666_PACKED:
-		ps_bpp_mode |= LOOSELY_PS_18BIT_RGB666;
-		break;
-	case MIPI_DSI_FMT_RGB565:
-		ps_bpp_mode |= PACKED_PS_16BIT_RGB565;
-		break;
-	}
-
-	writel(vm->vactive, dsi->regs + DSI_VACT_NL);
-	writel(ps_bpp_mode, dsi->regs + DSI_PSCTRL);
-	writel(ps_wc, dsi->regs + DSI_HSTX_CKL_WC);
-}
-
 static void mtk_dsi_rxtx_control(struct mtk_dsi *dsi)
 {
 	u32 tmp_reg;
@@ -417,36 +383,40 @@ static void mtk_dsi_rxtx_control(struct mtk_dsi *dsi)
 	writel(tmp_reg, dsi->regs + DSI_TXRX_CTRL);
 }
 
-static void mtk_dsi_ps_control(struct mtk_dsi *dsi)
+static void mtk_dsi_ps_control(struct mtk_dsi *dsi, bool config_vact)
 {
-	u32 dsi_tmp_buf_bpp;
-	u32 tmp_reg;
+	struct videomode *vm = &dsi->vm;
+	u32 dsi_buf_bpp, ps_wc;
+	u32 ps_bpp_mode;
+
+	if (dsi->format == MIPI_DSI_FMT_RGB565)
+		dsi_buf_bpp = 2;
+	else
+		dsi_buf_bpp = 3;
+
+	ps_wc = vm->hactive * dsi_buf_bpp;
+	ps_bpp_mode = ps_wc;
 
 	switch (dsi->format) {
 	case MIPI_DSI_FMT_RGB888:
-		tmp_reg = PACKED_PS_24BIT_RGB888;
-		dsi_tmp_buf_bpp = 3;
+		ps_bpp_mode |= PACKED_PS_24BIT_RGB888;
 		break;
 	case MIPI_DSI_FMT_RGB666:
-		tmp_reg = LOOSELY_PS_18BIT_RGB666;
-		dsi_tmp_buf_bpp = 3;
+		ps_bpp_mode |= PACKED_PS_18BIT_RGB666;
 		break;
 	case MIPI_DSI_FMT_RGB666_PACKED:
-		tmp_reg = PACKED_PS_18BIT_RGB666;
-		dsi_tmp_buf_bpp = 3;
+		ps_bpp_mode |= LOOSELY_PS_18BIT_RGB666;
 		break;
 	case MIPI_DSI_FMT_RGB565:
-		tmp_reg = PACKED_PS_16BIT_RGB565;
-		dsi_tmp_buf_bpp = 2;
-		break;
-	default:
-		tmp_reg = PACKED_PS_24BIT_RGB888;
-		dsi_tmp_buf_bpp = 3;
+		ps_bpp_mode |= PACKED_PS_16BIT_RGB565;
 		break;
 	}
 
-	tmp_reg += dsi->vm.hactive * dsi_tmp_buf_bpp & DSI_PS_WC;
-	writel(tmp_reg, dsi->regs + DSI_PSCTRL);
+	if (config_vact) {
+		writel(vm->vactive, dsi->regs + DSI_VACT_NL);
+		writel(ps_wc, dsi->regs + DSI_HSTX_CKL_WC);
+	}
+	writel(ps_bpp_mode, dsi->regs + DSI_PSCTRL);
 }
 
 static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
@@ -522,7 +492,7 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
 	writel(horizontal_backporch_byte, dsi->regs + DSI_HBP_WC);
 	writel(horizontal_frontporch_byte, dsi->regs + DSI_HFP_WC);
 
-	mtk_dsi_ps_control(dsi);
+	mtk_dsi_ps_control(dsi, false);
 }
 
 static void mtk_dsi_start(struct mtk_dsi *dsi)
@@ -667,7 +637,7 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
 	mtk_dsi_reset_engine(dsi);
 	mtk_dsi_phy_timconfig(dsi);
 
-	mtk_dsi_ps_control_vact(dsi);
+	mtk_dsi_ps_control(dsi, true);
 	mtk_dsi_set_vm_cmd(dsi);
 	mtk_dsi_config_vdo_timing(dsi);
 	mtk_dsi_set_interrupt_enable(dsi);
-- 
2.43.0

[PATCH v2 3/4] drm/mediatek: dsi: Use bitfield macros where useful

[AngeloGioacchino Del Regno]

Instead of open coding bitshifting for various register fields,
use the bitfield macro FIELD_PREP(): this allows to enhance the
human readability, decrease likeliness of mistakes (and register
field overflowing) and also to simplify the code.
The latter is especially seen in mtk_dsi_rxtx_control(), where
it was possible to change a switch to a short for loop and to
also remove the need to check for maximum DSI lanes == 4 thanks
to the FIELD_PREP macro masking the value.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dsi.c | 95 ++++++++++++++++--------------
 1 file changed, 50 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index b618e2e31022..2ba6cd129150 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -70,16 +70,19 @@
 #define DSI_PSCTRL		0x1c
 #define DSI_PS_WC			GENMASK(14, 0)
 #define DSI_PS_SEL			GENMASK(19, 16)
-#define PACKED_PS_16BIT_RGB565		(0 << 16)
-#define LOOSELY_PS_18BIT_RGB666		(1 << 16)
-#define PACKED_PS_18BIT_RGB666		(2 << 16)
-#define PACKED_PS_24BIT_RGB888		(3 << 16)
+#define PACKED_PS_16BIT_RGB565		0
+#define LOOSELY_PS_18BIT_RGB666		1
+#define PACKED_PS_18BIT_RGB666		2
+#define PACKED_PS_24BIT_RGB888		3
 
 #define DSI_VSA_NL		0x20
 #define DSI_VBP_NL		0x24
 #define DSI_VFP_NL		0x28
 #define DSI_VACT_NL		0x2C
+#define VACT_NL				GENMASK(14, 0)
 #define DSI_SIZE_CON		0x38
+#define DSI_HEIGHT				GENMASK(30, 16)
+#define DSI_WIDTH				GENMASK(14, 0)
 #define DSI_HSA_WC		0x50
 #define DSI_HBP_WC		0x54
 #define DSI_HFP_WC		0x58
@@ -254,14 +257,23 @@ static void mtk_dsi_phy_timconfig(struct mtk_dsi *dsi)
 	timing->clk_hs_zero = timing->clk_hs_trail * 4;
 	timing->clk_hs_exit = 2 * timing->clk_hs_trail;
 
-	timcon0 = timing->lpx | timing->da_hs_prepare << 8 |
-		  timing->da_hs_zero << 16 | timing->da_hs_trail << 24;
-	timcon1 = timing->ta_go | timing->ta_sure << 8 |
-		  timing->ta_get << 16 | timing->da_hs_exit << 24;
-	timcon2 = 1 << 8 | timing->clk_hs_zero << 16 |
-		  timing->clk_hs_trail << 24;
-	timcon3 = timing->clk_hs_prepare | timing->clk_hs_post << 8 |
-		  timing->clk_hs_exit << 16;
+	timcon0 = FIELD_PREP(LPX, timing->lpx) |
+		  FIELD_PREP(HS_PREP, timing->da_hs_prepare) |
+		  FIELD_PREP(HS_ZERO, timing->da_hs_zero) |
+		  FIELD_PREP(HS_TRAIL, timing->da_hs_trail);
+
+	timcon1 = FIELD_PREP(TA_GO, timing->ta_go) |
+		  FIELD_PREP(TA_SURE, timing->ta_sure) |
+		  FIELD_PREP(TA_GET, timing->ta_get) |
+		  FIELD_PREP(DA_HS_EXIT, timing->da_hs_exit);
+
+	timcon2 = FIELD_PREP(DA_HS_SYNC, 1) |
+		  FIELD_PREP(CLK_ZERO, timing->clk_hs_zero) |
+		  FIELD_PREP(CLK_TRAIL, timing->clk_hs_trail);
+
+	timcon3 = FIELD_PREP(CLK_HS_PREP, timing->clk_hs_prepare) |
+		  FIELD_PREP(CLK_HS_POST, timing->clk_hs_post) |
+		  FIELD_PREP(CLK_HS_EXIT, timing->clk_hs_exit);
 
 	writel(timcon0, dsi->regs + DSI_PHY_TIMECON0);
 	writel(timcon1, dsi->regs + DSI_PHY_TIMECON1);
@@ -354,69 +366,61 @@ static void mtk_dsi_set_vm_cmd(struct mtk_dsi *dsi)
 
 static void mtk_dsi_rxtx_control(struct mtk_dsi *dsi)
 {
-	u32 tmp_reg;
+	u32 regval, tmp_reg = 0;
+	u8 i;
 
-	switch (dsi->lanes) {
-	case 1:
-		tmp_reg = 1 << 2;
-		break;
-	case 2:
-		tmp_reg = 3 << 2;
-		break;
-	case 3:
-		tmp_reg = 7 << 2;
-		break;
-	case 4:
-		tmp_reg = 0xf << 2;
-		break;
-	default:
-		tmp_reg = 0xf << 2;
-		break;
-	}
+	/* Number of DSI lanes (max 4 lanes), each bit enables one DSI lane. */
+	for (i = 0; i < dsi->lanes; i++)
+		tmp_reg |= BIT(i);
+
+	regval = FIELD_PREP(LANE_NUM, tmp_reg);
 
 	if (dsi->mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS)
-		tmp_reg |= HSTX_CKLP_EN;
+		regval |= HSTX_CKLP_EN;
 
 	if (dsi->mode_flags & MIPI_DSI_MODE_NO_EOT_PACKET)
-		tmp_reg |= DIS_EOT;
+		regval |= DIS_EOT;
 
-	writel(tmp_reg, dsi->regs + DSI_TXRX_CTRL);
+	writel(regval, dsi->regs + DSI_TXRX_CTRL);
 }
 
 static void mtk_dsi_ps_control(struct mtk_dsi *dsi, bool config_vact)
 {
-	struct videomode *vm = &dsi->vm;
-	u32 dsi_buf_bpp, ps_wc;
-	u32 ps_bpp_mode;
+	u32 dsi_buf_bpp, ps_val, ps_wc, vact_nl;
 
 	if (dsi->format == MIPI_DSI_FMT_RGB565)
 		dsi_buf_bpp = 2;
 	else
 		dsi_buf_bpp = 3;
 
-	ps_wc = vm->hactive * dsi_buf_bpp;
-	ps_bpp_mode = ps_wc;
+	/* Word count */
+	ps_wc = FIELD_PREP(DSI_PS_WC, dsi->vm.hactive * dsi_buf_bpp);
+	ps_val = ps_wc;
 
+	/* Pixel Stream type */
 	switch (dsi->format) {
+	default:
+		/* fallthrough */
 	case MIPI_DSI_FMT_RGB888:
-		ps_bpp_mode |= PACKED_PS_24BIT_RGB888;
+		ps_val |= FIELD_PREP(DSI_PS_SEL, PACKED_PS_24BIT_RGB888);
 		break;
 	case MIPI_DSI_FMT_RGB666:
-		ps_bpp_mode |= PACKED_PS_18BIT_RGB666;
+		ps_val |= FIELD_PREP(DSI_PS_SEL, PACKED_PS_18BIT_RGB666);
 		break;
 	case MIPI_DSI_FMT_RGB666_PACKED:
-		ps_bpp_mode |= LOOSELY_PS_18BIT_RGB666;
+		ps_val |= FIELD_PREP(DSI_PS_SEL, LOOSELY_PS_18BIT_RGB666);
 		break;
 	case MIPI_DSI_FMT_RGB565:
-		ps_bpp_mode |= PACKED_PS_16BIT_RGB565;
+		ps_val |= FIELD_PREP(DSI_PS_SEL, PACKED_PS_16BIT_RGB565);
 		break;
 	}
 
 	if (config_vact) {
-		writel(vm->vactive, dsi->regs + DSI_VACT_NL);
+		vact_nl = FIELD_PREP(VACT_NL, dsi->vm.vactive);
+		writel(vact_nl, dsi->regs + DSI_VACT_NL);
 		writel(ps_wc, dsi->regs + DSI_HSTX_CKL_WC);
 	}
-	writel(ps_bpp_mode, dsi->regs + DSI_PSCTRL);
+	writel(ps_val, dsi->regs + DSI_PSCTRL);
 }
 
 static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
@@ -443,7 +447,8 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
 	writel(vm->vactive, dsi->regs + DSI_VACT_NL);
 
 	if (dsi->driver_data->has_size_ctl)
-		writel(vm->vactive << 16 | vm->hactive,
+		writel(FIELD_PREP(DSI_HEIGHT, vm->vactive) |
+		       FIELD_PREP(DSI_WIDTH, vm->hactive),
 		       dsi->regs + DSI_SIZE_CON);
 
 	horizontal_sync_active_byte = (vm->hsync_len * dsi_tmp_buf_bpp - 10);
-- 
2.43.0

[PATCH v2 4/4] drm/mediatek: dsi: Replace open-coded instance of HZ_PER_MHZ

[AngeloGioacchino Del Regno]

In mtk_dsi_phy_timconfig(), we're dividing the `data_rate` variable,
expressed in Hz to retrieve a value in MHz: instead of open-coding,
use the HZ_PER_MHZ definition, available in linux/units.h.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dsi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index 2ba6cd129150..b9a37407f3b4 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -12,6 +12,7 @@
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
 #include <linux/reset.h>
+#include <linux/units.h>
 
 #include <video/mipi_display.h>
 #include <video/videomode.h>
@@ -237,7 +238,7 @@ static void mtk_dsi_mask(struct mtk_dsi *dsi, u32 offset, u32 mask, u32 data)
 static void mtk_dsi_phy_timconfig(struct mtk_dsi *dsi)
 {
 	u32 timcon0, timcon1, timcon2, timcon3;
-	u32 data_rate_mhz = DIV_ROUND_UP(dsi->data_rate, 1000000);
+	u32 data_rate_mhz = DIV_ROUND_UP(dsi->data_rate, HZ_PER_MHZ);
 	struct mtk_phy_timing *timing = &dsi->phy_timing;
 
 	timing->lpx = (60 * data_rate_mhz / (8 * 1000)) + 1;
-- 
2.43.0

Re: [PATCH v2 3/4] drm/mediatek: dsi: Use bitfield macros where useful

[kernel test robot]

Hi AngeloGioacchino,

kernel test robot noticed the following build errors:

[auto build test ERROR on drm-misc/drm-misc-next]
[also build test ERROR on drm/drm-next drm-exynos/exynos-drm-next drm-intel/for-linux-next-fixes drm-tip/drm-tip linus/master v6.7-rc6 next-20231222]
[cannot apply to drm-intel/for-linux-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/AngeloGioacchino-Del-Regno/drm-mediatek-dsi-Use-GENMASK-for-register-mask-definitions/20231222-164513
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20231220135722.192080-4-angelogioacchino.delregno%40collabora.com
patch subject: [PATCH v2 3/4] drm/mediatek: dsi: Use bitfield macros where useful
config: arm-allmodconfig (https://download.01.org/0day-ci/archive/20231223/202312231512.ioiD48LA-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231223/202312231512.ioiD48LA-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312231512.ioiD48LA-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/gpu/drm/mediatek/mtk_dsi.c: In function 'mtk_dsi_phy_timconfig':
>> drivers/gpu/drm/mediatek/mtk_dsi.c:260:19: error: implicit declaration of function 'FIELD_PREP' [-Werror=implicit-function-declaration]
     260 |         timcon0 = FIELD_PREP(LPX, timing->lpx) |
         |                   ^~~~~~~~~~
   cc1: some warnings being treated as errors


vim +/FIELD_PREP +260 drivers/gpu/drm/mediatek/mtk_dsi.c

   236	
   237	static void mtk_dsi_phy_timconfig(struct mtk_dsi *dsi)
   238	{
   239		u32 timcon0, timcon1, timcon2, timcon3;
   240		u32 data_rate_mhz = DIV_ROUND_UP(dsi->data_rate, 1000000);
   241		struct mtk_phy_timing *timing = &dsi->phy_timing;
   242	
   243		timing->lpx = (60 * data_rate_mhz / (8 * 1000)) + 1;
   244		timing->da_hs_prepare = (80 * data_rate_mhz + 4 * 1000) / 8000;
   245		timing->da_hs_zero = (170 * data_rate_mhz + 10 * 1000) / 8000 + 1 -
   246				     timing->da_hs_prepare;
   247		timing->da_hs_trail = timing->da_hs_prepare + 1;
   248	
   249		timing->ta_go = 4 * timing->lpx - 2;
   250		timing->ta_sure = timing->lpx + 2;
   251		timing->ta_get = 4 * timing->lpx;
   252		timing->da_hs_exit = 2 * timing->lpx + 1;
   253	
   254		timing->clk_hs_prepare = 70 * data_rate_mhz / (8 * 1000);
   255		timing->clk_hs_post = timing->clk_hs_prepare + 8;
   256		timing->clk_hs_trail = timing->clk_hs_prepare;
   257		timing->clk_hs_zero = timing->clk_hs_trail * 4;
   258		timing->clk_hs_exit = 2 * timing->clk_hs_trail;
   259	
 > 260		timcon0 = FIELD_PREP(LPX, timing->lpx) |
   261			  FIELD_PREP(HS_PREP, timing->da_hs_prepare) |
   262			  FIELD_PREP(HS_ZERO, timing->da_hs_zero) |
   263			  FIELD_PREP(HS_TRAIL, timing->da_hs_trail);
   264	
   265		timcon1 = FIELD_PREP(TA_GO, timing->ta_go) |
   266			  FIELD_PREP(TA_SURE, timing->ta_sure) |
   267			  FIELD_PREP(TA_GET, timing->ta_get) |
   268			  FIELD_PREP(DA_HS_EXIT, timing->da_hs_exit);
   269	
   270		timcon2 = FIELD_PREP(DA_HS_SYNC, 1) |
   271			  FIELD_PREP(CLK_ZERO, timing->clk_hs_zero) |
   272			  FIELD_PREP(CLK_TRAIL, timing->clk_hs_trail);
   273	
   274		timcon3 = FIELD_PREP(CLK_HS_PREP, timing->clk_hs_prepare) |
   275			  FIELD_PREP(CLK_HS_POST, timing->clk_hs_post) |
   276			  FIELD_PREP(CLK_HS_EXIT, timing->clk_hs_exit);
   277	
   278		writel(timcon0, dsi->regs + DSI_PHY_TIMECON0);
   279		writel(timcon1, dsi->regs + DSI_PHY_TIMECON1);
   280		writel(timcon2, dsi->regs + DSI_PHY_TIMECON2);
   281		writel(timcon3, dsi->regs + DSI_PHY_TIMECON3);
   282	}
   283	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 1/4] drm/mediatek: dsi: Use GENMASK() for register mask definitions

[Fei Shao]

Hi Angelo,

On Wed, Dec 20, 2023 at 9:57 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Change magic numerical masks with usage of the GENMASK() macro
> to improve readability.
>
> This commit brings no functional changes.
>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dsi.c | 46 ++++++++++++++++--------------
>  1 file changed, 24 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index a2fdfc8ddb15..23d2c5be8dbb 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -58,18 +58,18 @@
>
>  #define DSI_TXRX_CTRL          0x18
>  #define VC_NUM                         BIT(1)
> -#define LANE_NUM                       (0xf << 2)
> +#define LANE_NUM                       GENMASK(5, 2)
>  #define DIS_EOT                                BIT(6)
>  #define NULL_EN                                BIT(7)
>  #define TE_FREERUN                     BIT(8)
>  #define EXT_TE_EN                      BIT(9)
>  #define EXT_TE_EDGE                    BIT(10)
> -#define MAX_RTN_SIZE                   (0xf << 12)
> +#define MAX_RTN_SIZE                   GENMASK(15, 12)
>  #define HSTX_CKLP_EN                   BIT(16)
>
>  #define DSI_PSCTRL             0x1c
> -#define DSI_PS_WC                      0x3fff
> -#define DSI_PS_SEL                     (3 << 16)
> +#define DSI_PS_WC                      GENMASK(14, 0)
> +#define DSI_PS_SEL                     GENMASK(19, 16)

GENMASK(17, 16)
>
>  #define PACKED_PS_16BIT_RGB565         (0 << 16)
>  #define LOOSELY_PS_18BIT_RGB666                (1 << 16)
>  #define PACKED_PS_18BIT_RGB666         (2 << 16)
> @@ -109,26 +109,27 @@
>  #define LD0_WAKEUP_EN                  BIT(2)
>
>  #define DSI_PHY_TIMECON0       0x110
> -#define LPX                            (0xff << 0)
> -#define HS_PREP                                (0xff << 8)
> -#define HS_ZERO                                (0xff << 16)
> -#define HS_TRAIL                       (0xff << 24)
> +#define LPX                            GENMASK(7, 0)
> +#define HS_PREP                                GENMASK(15, 8)
> +#define HS_ZERO                                GENMASK(23, 16)
> +#define HS_TRAIL                       GENMASK(31, 24)
>
>  #define DSI_PHY_TIMECON1       0x114
> -#define TA_GO                          (0xff << 0)
> -#define TA_SURE                                (0xff << 8)
> -#define TA_GET                         (0xff << 16)
> -#define DA_HS_EXIT                     (0xff << 24)
> +#define TA_GO                          GENMASK(7, 0)
> +#define TA_SURE                                GENMASK(15, 8)
> +#define TA_GET                         GENMASK(23, 16)
> +#define DA_HS_EXIT                     GENMASK(31, 24)
>
>  #define DSI_PHY_TIMECON2       0x118
> -#define CONT_DET                       (0xff << 0)
> -#define CLK_ZERO                       (0xff << 16)
> -#define CLK_TRAIL                      (0xff << 24)
> +#define CONT_DET                       GENMASK(7, 0)
> +#define DA_HS_SYNC                     GENMASK(15, 8)

This is new, so please introduce it in a separate patch if intended.

The rest looks good to me.

Regards,
Fei


>
> +#define CLK_ZERO                       GENMASK(23, 16)
> +#define CLK_TRAIL                      GENMASK(31, 24)
>
>  #define DSI_PHY_TIMECON3       0x11c
> -#define CLK_HS_PREP                    (0xff << 0)
> -#define CLK_HS_POST                    (0xff << 8)
> -#define CLK_HS_EXIT                    (0xff << 16)
> +#define CLK_HS_PREP                    GENMASK(7, 0)
> +#define CLK_HS_POST                    GENMASK(15, 8)
> +#define CLK_HS_EXIT                    GENMASK(23, 16)
>
>  #define DSI_VM_CMD_CON         0x130
>  #define VM_CMD_EN                      BIT(0)
> @@ -138,13 +139,14 @@
>  #define FORCE_COMMIT                   BIT(0)
>  #define BYPASS_SHADOW                  BIT(1)
>
> -#define CONFIG                         (0xff << 0)
> +/* CMDQ related bits */
> +#define CONFIG                         GENMASK(7, 0)
>  #define SHORT_PACKET                   0
>  #define LONG_PACKET                    2
>  #define BTA                            BIT(2)
> -#define DATA_ID                                (0xff << 8)
> -#define DATA_0                         (0xff << 16)
> -#define DATA_1                         (0xff << 24)
> +#define DATA_ID                                GENMASK(15, 8)
> +#define DATA_0                         GENMASK(23, 16)
> +#define DATA_1                         GENMASK(31, 24)
>
>  #define NS_TO_CYCLE(n, c)    ((n) / (c) + (((n) % (c)) ? 1 : 0))
>
> --
> 2.43.0
>
>

Re: [PATCH v2 2/4] drm/mediatek: dsi: Cleanup functions mtk_dsi_ps_control{_vact}()

[Fei Shao]

Hi Angelo,

On Wed, Dec 20, 2023 at 9:57 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Function mtk_dsi_ps_control() is a subset of mtk_dsi_ps_control_vact():
> merge the two in one mtk_dsi_ps_control() function by adding one
> function parameter `config_vact` which, when true, writes the VACT
> related registers.
>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dsi.c | 76 +++++++++---------------------
>  1 file changed, 23 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index 23d2c5be8dbb..b618e2e31022 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -352,40 +352,6 @@ static void mtk_dsi_set_vm_cmd(struct mtk_dsi *dsi)
>         mtk_dsi_mask(dsi, DSI_VM_CMD_CON, TS_VFP_EN, TS_VFP_EN);
>  }
>
> -static void mtk_dsi_ps_control_vact(struct mtk_dsi *dsi)
> -{
> -       struct videomode *vm = &dsi->vm;
> -       u32 dsi_buf_bpp, ps_wc;
> -       u32 ps_bpp_mode;
> -
> -       if (dsi->format == MIPI_DSI_FMT_RGB565)
> -               dsi_buf_bpp = 2;
> -       else
> -               dsi_buf_bpp = 3;
> -
> -       ps_wc = vm->hactive * dsi_buf_bpp;
> -       ps_bpp_mode = ps_wc;
> -
> -       switch (dsi->format) {
> -       case MIPI_DSI_FMT_RGB888:
> -               ps_bpp_mode |= PACKED_PS_24BIT_RGB888;
> -               break;
> -       case MIPI_DSI_FMT_RGB666:
> -               ps_bpp_mode |= PACKED_PS_18BIT_RGB666;
> -               break;
> -       case MIPI_DSI_FMT_RGB666_PACKED:
> -               ps_bpp_mode |= LOOSELY_PS_18BIT_RGB666;
> -               break;
> -       case MIPI_DSI_FMT_RGB565:
> -               ps_bpp_mode |= PACKED_PS_16BIT_RGB565;
> -               break;
> -       }
> -
> -       writel(vm->vactive, dsi->regs + DSI_VACT_NL);
> -       writel(ps_bpp_mode, dsi->regs + DSI_PSCTRL);
> -       writel(ps_wc, dsi->regs + DSI_HSTX_CKL_WC);
> -}
> -
>  static void mtk_dsi_rxtx_control(struct mtk_dsi *dsi)
>  {
>         u32 tmp_reg;
> @@ -417,36 +383,40 @@ static void mtk_dsi_rxtx_control(struct mtk_dsi *dsi)
>         writel(tmp_reg, dsi->regs + DSI_TXRX_CTRL);
>  }
>
> -static void mtk_dsi_ps_control(struct mtk_dsi *dsi)
> +static void mtk_dsi_ps_control(struct mtk_dsi *dsi, bool config_vact)
>  {
> -       u32 dsi_tmp_buf_bpp;
> -       u32 tmp_reg;
> +       struct videomode *vm = &dsi->vm;
> +       u32 dsi_buf_bpp, ps_wc;
> +       u32 ps_bpp_mode;
> +
> +       if (dsi->format == MIPI_DSI_FMT_RGB565)
> +               dsi_buf_bpp = 2;
> +       else
> +               dsi_buf_bpp = 3;

The same is also in mtk_dsi_config_vdo_timing(). Given this is a
cleanup series, I think it'd be a good chance to add another patch
and integrate those usages. Just a thought.  :)
>
> +
> +       ps_wc = vm->hactive * dsi_buf_bpp;

I noticed the "& DSI_PS_WC" part was dropped (but perhaps with awareness?).

While the outcome seems to always fall within the range of
DSI_PS_WC so we should be fine in practice, I think it doesn't hurt to
keep the value masked to save readers some time to check if this would
ever be possible to overflow and write undesired bits down the line.
WDYT?

Regardless of that, I didn't find obvious issue in this patch, so

Reviewed-by: Fei Shao <fshao@chromium.org>

Regards,
Fei




>
> +       ps_bpp_mode = ps_wc;
>
>         switch (dsi->format) {
>         case MIPI_DSI_FMT_RGB888:
> -               tmp_reg = PACKED_PS_24BIT_RGB888;
> -               dsi_tmp_buf_bpp = 3;
> +               ps_bpp_mode |= PACKED_PS_24BIT_RGB888;
>                 break;
>         case MIPI_DSI_FMT_RGB666:
> -               tmp_reg = LOOSELY_PS_18BIT_RGB666;
> -               dsi_tmp_buf_bpp = 3;
> +               ps_bpp_mode |= PACKED_PS_18BIT_RGB666;
>                 break;
>         case MIPI_DSI_FMT_RGB666_PACKED:
> -               tmp_reg = PACKED_PS_18BIT_RGB666;
> -               dsi_tmp_buf_bpp = 3;
> +               ps_bpp_mode |= LOOSELY_PS_18BIT_RGB666;
>                 break;
>         case MIPI_DSI_FMT_RGB565:
> -               tmp_reg = PACKED_PS_16BIT_RGB565;
> -               dsi_tmp_buf_bpp = 2;
> -               break;
> -       default:
>
> -               tmp_reg = PACKED_PS_24BIT_RGB888;
> -               dsi_tmp_buf_bpp = 3;
> +               ps_bpp_mode |= PACKED_PS_16BIT_RGB565;
>                 break;
>         }
>
> -       tmp_reg += dsi->vm.hactive * dsi_tmp_buf_bpp & DSI_PS_WC;
>
> -       writel(tmp_reg, dsi->regs + DSI_PSCTRL);
> +       if (config_vact) {
> +               writel(vm->vactive, dsi->regs + DSI_VACT_NL);
> +               writel(ps_wc, dsi->regs + DSI_HSTX_CKL_WC);
> +       }
> +       writel(ps_bpp_mode, dsi->regs + DSI_PSCTRL);
>  }
>
>  static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
> @@ -522,7 +492,7 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
>         writel(horizontal_backporch_byte, dsi->regs + DSI_HBP_WC);
>         writel(horizontal_frontporch_byte, dsi->regs + DSI_HFP_WC);
>
> -       mtk_dsi_ps_control(dsi);
> +       mtk_dsi_ps_control(dsi, false);
>  }
>
>  static void mtk_dsi_start(struct mtk_dsi *dsi)
> @@ -667,7 +637,7 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
>         mtk_dsi_reset_engine(dsi);
>         mtk_dsi_phy_timconfig(dsi);
>
> -       mtk_dsi_ps_control_vact(dsi);
> +       mtk_dsi_ps_control(dsi, true);
>         mtk_dsi_set_vm_cmd(dsi);
>         mtk_dsi_config_vdo_timing(dsi);
>         mtk_dsi_set_interrupt_enable(dsi);
> --
> 2.43.0
>
>

Re: [PATCH v2 1/4] drm/mediatek: dsi: Use GENMASK() for register mask definitions

[AngeloGioacchino Del Regno]

Il 26/12/23 11:46, Fei Shao ha scritto:
> Hi Angelo,
> 
> On Wed, Dec 20, 2023 at 9:57 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Change magic numerical masks with usage of the GENMASK() macro
>> to improve readability.
>>
>> This commit brings no functional changes.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   drivers/gpu/drm/mediatek/mtk_dsi.c | 46 ++++++++++++++++--------------
>>   1 file changed, 24 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
>> index a2fdfc8ddb15..23d2c5be8dbb 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
>> @@ -58,18 +58,18 @@
>>
>>   #define DSI_TXRX_CTRL          0x18
>>   #define VC_NUM                         BIT(1)
>> -#define LANE_NUM                       (0xf << 2)
>> +#define LANE_NUM                       GENMASK(5, 2)
>>   #define DIS_EOT                                BIT(6)
>>   #define NULL_EN                                BIT(7)
>>   #define TE_FREERUN                     BIT(8)
>>   #define EXT_TE_EN                      BIT(9)
>>   #define EXT_TE_EDGE                    BIT(10)
>> -#define MAX_RTN_SIZE                   (0xf << 12)
>> +#define MAX_RTN_SIZE                   GENMASK(15, 12)
>>   #define HSTX_CKLP_EN                   BIT(16)
>>
>>   #define DSI_PSCTRL             0x1c
>> -#define DSI_PS_WC                      0x3fff
>> -#define DSI_PS_SEL                     (3 << 16)
>> +#define DSI_PS_WC                      GENMASK(14, 0)
>> +#define DSI_PS_SEL                     GENMASK(19, 16)
> 
> GENMASK(17, 16)

That was intended, and depends on the SoC - I should effectively advertise that
in the commit description and I will.

As per the datasheets:

MT8192 - DSI_PS_SEL[18:16]
MT8195 - DSI_PS_SEL[19:16]

...on SoCs that don't have those additional bits, the higher ones are reserved
(unused), so it is safe to have this as 19, 16.

>>
>>   #define PACKED_PS_16BIT_RGB565         (0 << 16)
>>   #define LOOSELY_PS_18BIT_RGB666                (1 << 16)
>>   #define PACKED_PS_18BIT_RGB666         (2 << 16)
>> @@ -109,26 +109,27 @@
>>   #define LD0_WAKEUP_EN                  BIT(2)
>>
>>   #define DSI_PHY_TIMECON0       0x110
>> -#define LPX                            (0xff << 0)
>> -#define HS_PREP                                (0xff << 8)
>> -#define HS_ZERO                                (0xff << 16)
>> -#define HS_TRAIL                       (0xff << 24)
>> +#define LPX                            GENMASK(7, 0)
>> +#define HS_PREP                                GENMASK(15, 8)
>> +#define HS_ZERO                                GENMASK(23, 16)
>> +#define HS_TRAIL                       GENMASK(31, 24)
>>
>>   #define DSI_PHY_TIMECON1       0x114
>> -#define TA_GO                          (0xff << 0)
>> -#define TA_SURE                                (0xff << 8)
>> -#define TA_GET                         (0xff << 16)
>> -#define DA_HS_EXIT                     (0xff << 24)
>> +#define TA_GO                          GENMASK(7, 0)
>> +#define TA_SURE                                GENMASK(15, 8)
>> +#define TA_GET                         GENMASK(23, 16)
>> +#define DA_HS_EXIT                     GENMASK(31, 24)
>>
>>   #define DSI_PHY_TIMECON2       0x118
>> -#define CONT_DET                       (0xff << 0)
>> -#define CLK_ZERO                       (0xff << 16)
>> -#define CLK_TRAIL                      (0xff << 24)
>> +#define CONT_DET                       GENMASK(7, 0)
>> +#define DA_HS_SYNC                     GENMASK(15, 8)
> 
> This is new, so please introduce it in a separate patch if intended.
> 

I wouldn't really be comfortable doing so: this would mean that I'd
have to first write a constant and then fix that in a later patch...

P.S.: Sorry for working again on this one whole month after .... :-)

Regards,
Angelo

> The rest looks good to me.
> 
> Regards,
> Fei
> 
> 
>>
>> +#define CLK_ZERO                       GENMASK(23, 16)
>> +#define CLK_TRAIL                      GENMASK(31, 24)
>>
>>   #define DSI_PHY_TIMECON3       0x11c
>> -#define CLK_HS_PREP                    (0xff << 0)
>> -#define CLK_HS_POST                    (0xff << 8)
>> -#define CLK_HS_EXIT                    (0xff << 16)
>> +#define CLK_HS_PREP                    GENMASK(7, 0)
>> +#define CLK_HS_POST                    GENMASK(15, 8)
>> +#define CLK_HS_EXIT                    GENMASK(23, 16)
>>
>>   #define DSI_VM_CMD_CON         0x130
>>   #define VM_CMD_EN                      BIT(0)
>> @@ -138,13 +139,14 @@
>>   #define FORCE_COMMIT                   BIT(0)
>>   #define BYPASS_SHADOW                  BIT(1)
>>
>> -#define CONFIG                         (0xff << 0)
>> +/* CMDQ related bits */
>> +#define CONFIG                         GENMASK(7, 0)
>>   #define SHORT_PACKET                   0
>>   #define LONG_PACKET                    2
>>   #define BTA                            BIT(2)
>> -#define DATA_ID                                (0xff << 8)
>> -#define DATA_0                         (0xff << 16)
>> -#define DATA_1                         (0xff << 24)
>> +#define DATA_ID                                GENMASK(15, 8)
>> +#define DATA_0                         GENMASK(23, 16)
>> +#define DATA_1                         GENMASK(31, 24)
>>
>>   #define NS_TO_CYCLE(n, c)    ((n) / (c) + (((n) % (c)) ? 1 : 0))
>>
>> --
>> 2.43.0
>>
>>

Re: [PATCH v2 2/4] drm/mediatek: dsi: Cleanup functions mtk_dsi_ps_control{_vact}()

[AngeloGioacchino Del Regno]

Il 26/12/23 11:48, Fei Shao ha scritto:
> Hi Angelo,
> 
> On Wed, Dec 20, 2023 at 9:57 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Function mtk_dsi_ps_control() is a subset of mtk_dsi_ps_control_vact():
>> merge the two in one mtk_dsi_ps_control() function by adding one
>> function parameter `config_vact` which, when true, writes the VACT
>> related registers.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   drivers/gpu/drm/mediatek/mtk_dsi.c | 76 +++++++++---------------------
>>   1 file changed, 23 insertions(+), 53 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
>> index 23d2c5be8dbb..b618e2e31022 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
>> @@ -352,40 +352,6 @@ static void mtk_dsi_set_vm_cmd(struct mtk_dsi *dsi)
>>          mtk_dsi_mask(dsi, DSI_VM_CMD_CON, TS_VFP_EN, TS_VFP_EN);
>>   }
>>
>> -static void mtk_dsi_ps_control_vact(struct mtk_dsi *dsi)
>> -{
>> -       struct videomode *vm = &dsi->vm;
>> -       u32 dsi_buf_bpp, ps_wc;
>> -       u32 ps_bpp_mode;
>> -
>> -       if (dsi->format == MIPI_DSI_FMT_RGB565)
>> -               dsi_buf_bpp = 2;
>> -       else
>> -               dsi_buf_bpp = 3;
>> -
>> -       ps_wc = vm->hactive * dsi_buf_bpp;
>> -       ps_bpp_mode = ps_wc;
>> -
>> -       switch (dsi->format) {
>> -       case MIPI_DSI_FMT_RGB888:
>> -               ps_bpp_mode |= PACKED_PS_24BIT_RGB888;
>> -               break;
>> -       case MIPI_DSI_FMT_RGB666:
>> -               ps_bpp_mode |= PACKED_PS_18BIT_RGB666;
>> -               break;
>> -       case MIPI_DSI_FMT_RGB666_PACKED:
>> -               ps_bpp_mode |= LOOSELY_PS_18BIT_RGB666;
>> -               break;
>> -       case MIPI_DSI_FMT_RGB565:
>> -               ps_bpp_mode |= PACKED_PS_16BIT_RGB565;
>> -               break;
>> -       }
>> -
>> -       writel(vm->vactive, dsi->regs + DSI_VACT_NL);
>> -       writel(ps_bpp_mode, dsi->regs + DSI_PSCTRL);
>> -       writel(ps_wc, dsi->regs + DSI_HSTX_CKL_WC);
>> -}
>> -
>>   static void mtk_dsi_rxtx_control(struct mtk_dsi *dsi)
>>   {
>>          u32 tmp_reg;
>> @@ -417,36 +383,40 @@ static void mtk_dsi_rxtx_control(struct mtk_dsi *dsi)
>>          writel(tmp_reg, dsi->regs + DSI_TXRX_CTRL);
>>   }
>>
>> -static void mtk_dsi_ps_control(struct mtk_dsi *dsi)
>> +static void mtk_dsi_ps_control(struct mtk_dsi *dsi, bool config_vact)
>>   {
>> -       u32 dsi_tmp_buf_bpp;
>> -       u32 tmp_reg;
>> +       struct videomode *vm = &dsi->vm;
>> +       u32 dsi_buf_bpp, ps_wc;
>> +       u32 ps_bpp_mode;
>> +
>> +       if (dsi->format == MIPI_DSI_FMT_RGB565)
>> +               dsi_buf_bpp = 2;
>> +       else
>> +               dsi_buf_bpp = 3;
> 
> The same is also in mtk_dsi_config_vdo_timing(). Given this is a
> cleanup series, I think it'd be a good chance to add another patch
> and integrate those usages. Just a thought.  :)

Checking that right now.

>>
>> +
>> +       ps_wc = vm->hactive * dsi_buf_bpp;
> 
> I noticed the "& DSI_PS_WC" part was dropped (but perhaps with awareness?).
> 
> While the outcome seems to always fall within the range of
> DSI_PS_WC so we should be fine in practice, I think it doesn't hurt to
> keep the value masked to save readers some time to check if this would
> ever be possible to overflow and write undesired bits down the line.
> WDYT?
> 

Masking a pre-masked value doesn't look right.

Besides, as for the concern of overflowing, if we masked that we'd still end up
with broken functionality, as if the value is invalid... well, it's invalid.
Masked or not. :-)

Thanks for the R-b, sending a v3 soon with some fixes.

Regards,
Angelo


> Regardless of that, I didn't find obvious issue in this patch, so
> 
> Reviewed-by: Fei Shao <fshao@chromium.org>
> 
> Regards,
> Fei
> 
> 
> 
> 
>>
>> +       ps_bpp_mode = ps_wc;
>>
>>          switch (dsi->format) {
>>          case MIPI_DSI_FMT_RGB888:
>> -               tmp_reg = PACKED_PS_24BIT_RGB888;
>> -               dsi_tmp_buf_bpp = 3;
>> +               ps_bpp_mode |= PACKED_PS_24BIT_RGB888;
>>                  break;
>>          case MIPI_DSI_FMT_RGB666:
>> -               tmp_reg = LOOSELY_PS_18BIT_RGB666;
>> -               dsi_tmp_buf_bpp = 3;
>> +               ps_bpp_mode |= PACKED_PS_18BIT_RGB666;
>>                  break;
>>          case MIPI_DSI_FMT_RGB666_PACKED:
>> -               tmp_reg = PACKED_PS_18BIT_RGB666;
>> -               dsi_tmp_buf_bpp = 3;
>> +               ps_bpp_mode |= LOOSELY_PS_18BIT_RGB666;
>>                  break;
>>          case MIPI_DSI_FMT_RGB565:
>> -               tmp_reg = PACKED_PS_16BIT_RGB565;
>> -               dsi_tmp_buf_bpp = 2;
>> -               break;
>> -       default:
>>
>> -               tmp_reg = PACKED_PS_24BIT_RGB888;
>> -               dsi_tmp_buf_bpp = 3;
>> +               ps_bpp_mode |= PACKED_PS_16BIT_RGB565;
>>                  break;
>>          }
>>
>> -       tmp_reg += dsi->vm.hactive * dsi_tmp_buf_bpp & DSI_PS_WC;
>>
>> -       writel(tmp_reg, dsi->regs + DSI_PSCTRL);
>> +       if (config_vact) {
>> +               writel(vm->vactive, dsi->regs + DSI_VACT_NL);
>> +               writel(ps_wc, dsi->regs + DSI_HSTX_CKL_WC);
>> +       }
>> +       writel(ps_bpp_mode, dsi->regs + DSI_PSCTRL);
>>   }
>>
>>   static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
>> @@ -522,7 +492,7 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
>>          writel(horizontal_backporch_byte, dsi->regs + DSI_HBP_WC);
>>          writel(horizontal_frontporch_byte, dsi->regs + DSI_HFP_WC);
>>
>> -       mtk_dsi_ps_control(dsi);
>> +       mtk_dsi_ps_control(dsi, false);
>>   }
>>
>>   static void mtk_dsi_start(struct mtk_dsi *dsi)
>> @@ -667,7 +637,7 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
>>          mtk_dsi_reset_engine(dsi);
>>          mtk_dsi_phy_timconfig(dsi);
>>
>> -       mtk_dsi_ps_control_vact(dsi);
>> +       mtk_dsi_ps_control(dsi, true);
>>          mtk_dsi_set_vm_cmd(dsi);
>>          mtk_dsi_config_vdo_timing(dsi);
>>          mtk_dsi_set_interrupt_enable(dsi);
>> --
>> 2.43.0
>>
>>