On Thu, 20 Jun 2024 23:29:52 +1200
Ryan Walklin <ryan@xxxxxxxxxxxxx> wrote:
Hi,
> From: Jernej Skrabec <jernej.skrabec@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> Signed-off-by: Ryan Walklin <ryan@xxxxxxxxxxxxx>
> ---
> 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@xxxxxxx>
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:
