diff mbox series

[v3,2/7] drm/mediatek: dsi: Cleanup functions mtk_dsi_ps_control{_vact}()

Message ID 20240131113434.241929-3-angelogioacchino.delregno@collabora.com (mailing list archive)
State New, archived
Headers show
Series MediaTek DRM - DSI driver cleanups | expand

Commit Message

AngeloGioacchino Del Regno Jan. 31, 2024, 11:34 a.m. UTC 
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.

Reviewed-by: Fei Shao <fshao@chromium.org>
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(-)

Comments

CK Hu (胡俊光) Feb. 6, 2024, 9:50 a.m. UTC | #1 
Hi, Angelo:

On Wed, 2024-01-31 at 12:34 +0100, AngeloGioacchino Del Regno 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.
> 
> Reviewed-by: Fei Shao <fshao@chromium.org>
> 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 3b7392c03b4d..8414ce73ce9f 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -351,40 +351,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;
> @@ -416,36 +382,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;

You change the original logic here. If it is a fixup, separate to a
independent patch not hiding in a clean up patch. So we could backport
the fixup patch.

Regards,
CK

>  		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)
> @@ -521,7 +491,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)
> @@ -666,7 +636,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);
AngeloGioacchino Del Regno Feb. 6, 2024, 11:24 a.m. UTC | #2 
Il 06/02/24 10:50, CK Hu (胡俊光) ha scritto:
> Hi, Angelo:
> 
> On Wed, 2024-01-31 at 12:34 +0100, AngeloGioacchino Del Regno 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.
>>
>> Reviewed-by: Fei Shao <fshao@chromium.org>
>> 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 3b7392c03b4d..8414ce73ce9f 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
>> @@ -351,40 +351,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;
>> @@ -416,36 +382,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;
> 
> You change the original logic here. If it is a fixup, separate to a
> independent patch not hiding in a clean up patch. So we could backport
> the fixup patch.
> 

Thank you CK! Thanks to you noticing that, I've discovered that actually
besides the two functions not agreeing on the bit to set, the definitions
of those bits are actually wrong (as you can verify on datasheets for
multiple SoCs, including MT8195, MT8188, MT8186, MT8183).

v4 will fix that by adding a fixup commit to rectify the whole thing.

Cheers,
Angelo

> Regards,
> CK
> 
>>   		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)
>> @@ -521,7 +491,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)
>> @@ -666,7 +636,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);
diff mbox series

Patch

diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index 3b7392c03b4d..8414ce73ce9f 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -351,40 +351,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;
@@ -416,36 +382,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)
@@ -521,7 +491,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)
@@ -666,7 +636,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);