Re: [PATCH v10 14/21] drm/mediatek: dpi: Add dpintf support

[Christophe JAILLET]

Le 23/05/2022 à 12:47, Guillaume Ranquet a écrit :
> dpintf is the displayport interface hardware unit. This unit is similar
> to dpi and can reuse most of the code.
>
> This patch adds support for mt8195-dpintf to this dpi driver. Main
> differences are:
>   - Some features/functional components are not available for dpintf
>     which are now excluded from code execution once is_dpintf is set
>   - dpintf can and needs to choose between different clockdividers based
>     on the clockspeed. This is done by choosing a different clock parent.
>   - There are two additional clocks that need to be managed. These are
>     only set for dpintf and will be set to NULL if not supplied. The
>     clk_* calls handle these as normal clocks then.
>   - Some register contents differ slightly between the two components. To
>     work around this I added register bits/masks with a DPINTF_ prefix
>     and use them where different.
>
> Based on a separate driver for dpintf created by
> Jason-JH.Lin <jason-jh.lin-NuS5LvNUpcJWk0Htik3J/w@xxxxxxxxxxxxxxxx>.
>
> Signed-off-by: Markus Schneider-Pargmann <msp-rdvid1DuHRBWk0Htik3J/w@xxxxxxxxxxxxxxxx>
> Signed-off-by: Guillaume Ranquet <granquet-rdvid1DuHRBWk0Htik3J/w@xxxxxxxxxxxxxxxx>
> ---
>   drivers/gpu/drm/mediatek/mtk_dpi.c          | 126 +++++++++++++++++---
>   drivers/gpu/drm/mediatek/mtk_dpi_regs.h     |  35 ++++++
>   drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c |   8 ++
>   drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h |   1 +
>   drivers/gpu/drm/mediatek/mtk_drm_drv.c      |   5 +-
>   include/linux/soc/mediatek/mtk-mmsys.h      |   4 +-
>   6 files changed, 159 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c
> index eb969c5c5c2e..763bfb700135 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dpi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
> @@ -71,6 +71,7 @@ struct mtk_dpi {
>   	void __iomem *regs;
>   	struct device *dev;
>   	struct clk *engine_clk;
> +	struct clk *dpi_ck_cg;
>   	struct clk *pixel_clk;
>   	struct clk *tvd_clk;
>   	int irq;
> @@ -126,6 +127,7 @@ struct mtk_dpi_conf {
>   	const u32 *output_fmts;
>   	u32 num_output_fmts;
>   	bool is_ck_de_pol;
> +	bool is_dpintf;
>   	bool swap_input_support;
>   	/* Mask used for HWIDTH, HPORCH, VSYNC_WIDTH and VSYNC_PORCH (no shift) */
>   	u32 dimension_mask;
> @@ -438,6 +440,8 @@ static void mtk_dpi_power_off(struct mtk_dpi *dpi)
>   	mtk_dpi_disable(dpi);
>   	clk_disable_unprepare(dpi->pixel_clk);
>   	clk_disable_unprepare(dpi->engine_clk);
> +	clk_disable_unprepare(dpi->dpi_ck_cg);
> +	clk_disable_unprepare(dpi->tvd_clk);
>   }
>   
>   static int mtk_dpi_power_on(struct mtk_dpi *dpi)
> @@ -447,12 +451,24 @@ static int mtk_dpi_power_on(struct mtk_dpi *dpi)
>   	if (++dpi->refcount != 1)
>   		return 0;
>   

Hi,

belwo the error handling path looks odd. (both where we goto, and the 
order of the clk_disable_unprepare() in the error handling path.

just my 2c,

CJ

> +	ret = clk_prepare_enable(dpi->tvd_clk);
> +	if (ret) {
> +		dev_err(dpi->dev, "Failed to enable tvd pll: %d\n", ret);
> +		goto err_pixel;
> +	}
> +
>   	ret = clk_prepare_enable(dpi->engine_clk);
>   	if (ret) {
>   		dev_err(dpi->dev, "Failed to enable engine clock: %d\n", ret);
>   		goto err_refcount;
>   	}
>   
> +	ret = clk_prepare_enable(dpi->dpi_ck_cg);
> +	if (ret) {
> +		dev_err(dpi->dev, "Failed to enable dpi_ck_cg clock: %d\n", ret);
> +		goto err_ck_cg;
> +	}
> +
>   	ret = clk_prepare_enable(dpi->pixel_clk);
>   	if (ret) {
>   		dev_err(dpi->dev, "Failed to enable pixel clock: %d\n", ret);
> @@ -466,6 +482,8 @@ static int mtk_dpi_power_on(struct mtk_dpi *dpi)
>   	return 0;
>   
>   err_pixel:
> +	clk_disable_unprepare(dpi->dpi_ck_cg);
> +err_ck_cg:
>   	clk_disable_unprepare(dpi->engine_clk);
>   err_refcount:
>   	dpi->refcount--;

[...]