diff mbox series

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

Message ID 20240620113150.83466-15-ryan@testtoast.com (mailing list archive)
State New, archived
Headers show
Series drm: sun4i: add Display Engine 3.3 (DE33) support | expand

Commit Message

Ryan Walklin June 20, 2024, 11:29 a.m. UTC
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(-)

Comments

Andre Przywara June 25, 2024, 12:28 a.m. UTC | #1
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:
Ryan Walklin June 25, 2024, 1:23 a.m. UTC | #2
Hi Andre,

On Tue, 25 Jun 2024, at 12:28 PM, Andre Przywara wrote:

Thanks for the review!


>>  		regmap_write(mixer->engine.regs, SUN50I_MIXER_CDC1_EN, 0);
>> -	} else {
>> +
>
> That seems to add an extra line, which shouldn't be here.

This was intentional to add some whitespace between the block of register writes and the next if/then block, but happy to remove delete if that is the style preference.

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

Thanks again!

Ryan
diff mbox series

Patch

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);
+
+	/* 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 {
+
+	} 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: