linux-kernel - Re: [PATCH v3 2/7] drm/mediatek: dsi: Cleanup functions mtk_dsi_ps_control{

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <14e124f02d82ff151974f99d042c4197e4dd5dd7.camel@mediatek.com>
Date: Tue, 6 Feb 2024 09:50:41 +0000
From: CK Hu (胡俊光) <ck.hu@...iatek.com>
To: "angelogioacchino.delregno@...labora.com"
	<angelogioacchino.delregno@...labora.com>, "chunkuang.hu@...nel.org"
	<chunkuang.hu@...nel.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
	"kernel@...labora.com" <kernel@...labora.com>, "daniel@...ll.ch"
	<daniel@...ll.ch>, "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
	"airlied@...il.com" <airlied@...il.com>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "matthias.bgg@...il.com"
	<matthias.bgg@...il.com>
Subject: Re: [PATCH v3 2/7] drm/mediatek: dsi: Cleanup functions
 mtk_dsi_ps_control{_vact}()

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@...omium.org>
> Signed-off-by: AngeloGioacchino Del Regno <
> angelogioacchino.delregno@...labora.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);