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

["CK Hu (胡俊光)" <ck.hu@mediatek.com>]

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);