Re: [PATCH 14/23] drm: sun4i: de2/de3: refactor mixer initialisation

[Andre Przywara <andre.przywara@arm.com>]

On Thu, 20 Jun 2024 23:29:52 +1200
Ryan Walklin <ryan@testtoast.com> wrote:

Hi,

> From: Jernej Skrabec <jernej.skrabec@gmail.com>
> 
> Now that the DE variant can be selected by enum, take the oppportunity
> to factor out some common initialisation code to a separate function.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> Signed-off-by: Ryan Walklin <ryan@testtoast.com>
> ---
>  drivers/gpu/drm/sun4i/sun8i_mixer.c | 69 ++++++++++++++++-------------
>  1 file changed, 38 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> index 7874b68786eee..533aa93d2a30e 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> @@ -404,6 +404,40 @@ static int sun8i_mixer_of_get_id(struct device_node *node)
>  	return of_ep.id;
>  }
>  
> +static void sun8i_mixer_init(struct sun8i_mixer *mixer)
> +{
> +	unsigned int base;
> +	int plane_cnt, i;
> +
> +	base = sun8i_blender_base(mixer);

Please move the initialisation up into the line with the declaration.

> +
> +	/* Enable the mixer */
> +	regmap_write(mixer->engine.regs, SUN8I_MIXER_GLOBAL_CTL,
> +		     SUN8I_MIXER_GLOBAL_CTL_RT_EN);
> +
> +	/* Set background color to black */
> +	regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_BKCOLOR(base),
> +		     SUN8I_MIXER_BLEND_COLOR_BLACK);
> +
> +	/*
> +	 * Set fill color of bottom plane to black. Generally not needed
> +	 * except when VI plane is at bottom (zpos = 0) and enabled.
> +	 */
> +	regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_PIPE_CTL(base),
> +		     SUN8I_MIXER_BLEND_PIPE_CTL_FC_EN(0));
> +	regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_ATTR_FCOLOR(base, 0),
> +		     SUN8I_MIXER_BLEND_COLOR_BLACK);
> +
> +	plane_cnt = mixer->cfg->vi_num + mixer->cfg->ui_num;
> +	for (i = 0; i < plane_cnt; i++)
> +		regmap_write(mixer->engine.regs,
> +			     SUN8I_MIXER_BLEND_MODE(base, i),
> +			     SUN8I_MIXER_BLEND_MODE_DEF);
> +
> +	regmap_update_bits(mixer->engine.regs, SUN8I_MIXER_BLEND_PIPE_CTL(base),
> +			   SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK, 0);
> +}
> +
>  static int sun8i_mixer_bind(struct device *dev, struct device *master,
>  			      void *data)
>  {
> @@ -412,8 +446,6 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
>  	struct sun4i_drv *drv = drm->dev_private;
>  	struct sun8i_mixer *mixer;
>  	void __iomem *regs;
> -	unsigned int base;
> -	int plane_cnt;
>  	int i, ret;
>  
>  	/*
> @@ -517,8 +549,6 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
>  
>  	list_add_tail(&mixer->engine.list, &drv->engine_list);
>  
> -	base = sun8i_blender_base(mixer);
> -
>  	/* Reset registers and disable unused sub-engines */
>  	if (mixer->cfg->de_type == sun8i_mixer_de3) {
>  		for (i = 0; i < DE3_MIXER_UNIT_SIZE; i += 4)
> @@ -534,7 +564,8 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
>  		regmap_write(mixer->engine.regs, SUN50I_MIXER_FMT_EN, 0);
>  		regmap_write(mixer->engine.regs, SUN50I_MIXER_CDC0_EN, 0);
>  		regmap_write(mixer->engine.regs, SUN50I_MIXER_CDC1_EN, 0);
> -	} else {
> +

That seems to add an extra line, which shouldn't be here.

Verified that the rest is indeed just a code move, from below into a
separate function. So with the two minor bits above fixed:

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> +	} else if (mixer->cfg->de_type == sun8i_mixer_de2) {
>  		for (i = 0; i < DE2_MIXER_UNIT_SIZE; i += 4)
>  			regmap_write(mixer->engine.regs, i, 0);
>  
> @@ -547,32 +578,8 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
>  		regmap_write(mixer->engine.regs, SUN8I_MIXER_DCSC_EN, 0);
>  	}
>  
> -	/* Enable the mixer */
> -	regmap_write(mixer->engine.regs, SUN8I_MIXER_GLOBAL_CTL,
> -		     SUN8I_MIXER_GLOBAL_CTL_RT_EN);
> -
> -	/* Set background color to black */
> -	regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_BKCOLOR(base),
> -		     SUN8I_MIXER_BLEND_COLOR_BLACK);
> -
> -	/*
> -	 * Set fill color of bottom plane to black. Generally not needed
> -	 * except when VI plane is at bottom (zpos = 0) and enabled.
> -	 */
> -	regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_PIPE_CTL(base),
> -		     SUN8I_MIXER_BLEND_PIPE_CTL_FC_EN(0));
> -	regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_ATTR_FCOLOR(base, 0),
> -		     SUN8I_MIXER_BLEND_COLOR_BLACK);
> -
> -	plane_cnt = mixer->cfg->vi_num + mixer->cfg->ui_num;
> -	for (i = 0; i < plane_cnt; i++)
> -		regmap_write(mixer->engine.regs,
> -			     SUN8I_MIXER_BLEND_MODE(base, i),
> -			     SUN8I_MIXER_BLEND_MODE_DEF);
> -
> -	regmap_update_bits(mixer->engine.regs, SUN8I_MIXER_BLEND_PIPE_CTL(base),
> -			   SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK, 0);
> -
> +	sun8i_mixer_init(mixer);
> +	
>  	return 0;
>  
>  err_disable_bus_clk: