| Message ID | 20240620113150.83466-15-ryan@testtoast.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm: sun4i: add Display Engine 3.3 (DE33) support | expand |
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:
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 --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: