[PATCH 7/7] drm/sun4i: switch DE33 to new bindings
Jernej Škrabec
jernej.skrabec at gmail.com
Sat Feb 14 12:55:00 PST 2026
Hi Chen-Yu,
Dne četrtek, 25. december 2025 ob 10:49:47 Srednjeevropski standardni čas je Chen-Yu Tsai napisal(a):
> On Sat, Nov 15, 2025 at 10:14 PM Jernej Skrabec
> <jernej.skrabec at gmail.com> wrote:
> >
> > Now that everything is in place, switch DE33 to new bindings.
> >
> > Signed-off-by: Jernej Skrabec <jernej.skrabec at gmail.com>
> > ---
> > drivers/gpu/drm/sun4i/sun8i_mixer.c | 130 +++++++++++++++-------------
> > drivers/gpu/drm/sun4i/sun8i_mixer.h | 10 +--
> > 2 files changed, 71 insertions(+), 69 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > index fde3b677e925..da213e54e653 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > @@ -13,6 +13,7 @@
> > #include <linux/of.h>
> > #include <linux/of_device.h>
> > #include <linux/of_graph.h>
> > +#include <linux/of_platform.h>
> > #include <linux/platform_device.h>
> > #include <linux/reset.h>
> >
> > @@ -24,6 +25,7 @@
> > #include <drm/drm_probe_helper.h>
> >
> > #include "sun4i_drv.h"
> > +#include "sun50i_planes.h"
> > #include "sun8i_mixer.h"
> > #include "sun8i_ui_layer.h"
> > #include "sun8i_vi_layer.h"
> > @@ -256,7 +258,6 @@ static void sun8i_mixer_commit(struct sunxi_engine *engine,
> > {
> > struct sun8i_mixer *mixer = engine_to_sun8i_mixer(engine);
> > u32 bld_base = sun8i_blender_base(mixer);
> > - struct regmap *bld_regs = sun8i_blender_regmap(mixer);
> > struct drm_plane_state *plane_state;
> > struct drm_plane *plane;
> > u32 route = 0, pipe_en = 0;
> > @@ -293,16 +294,16 @@ static void sun8i_mixer_commit(struct sunxi_engine *engine,
> > route |= layer->index << SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos);
> > pipe_en |= SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);
> >
> > - regmap_write(bld_regs,
> > + regmap_write(engine->regs,
> > SUN8I_MIXER_BLEND_ATTR_COORD(bld_base, zpos),
> > SUN8I_MIXER_COORD(x, y));
> > - regmap_write(bld_regs,
> > + regmap_write(engine->regs,
> > SUN8I_MIXER_BLEND_ATTR_INSIZE(bld_base, zpos),
> > SUN8I_MIXER_SIZE(w, h));
> > }
> >
> > - regmap_write(bld_regs, SUN8I_MIXER_BLEND_ROUTE(bld_base), route);
> > - regmap_write(bld_regs, SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
> > + regmap_write(engine->regs, SUN8I_MIXER_BLEND_ROUTE(bld_base), route);
> > + regmap_write(engine->regs, SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
> > pipe_en | SUN8I_MIXER_BLEND_PIPE_CTL_FC_EN(0));
> >
> > if (mixer->cfg->de_type != SUN8I_MIXER_DE33)
> > @@ -317,7 +318,6 @@ static struct drm_plane **sun8i_layers_init(struct drm_device *drm,
> > struct sun8i_mixer *mixer = engine_to_sun8i_mixer(engine);
> > int plane_cnt = mixer->cfg->ui_num + mixer->cfg->vi_num;
> > enum drm_plane_type type;
> > - unsigned int phy_index;
> > int i;
> >
> > planes = devm_kcalloc(drm->dev, plane_cnt, sizeof(*planes), GFP_KERNEL);
> > @@ -332,12 +332,8 @@ static struct drm_plane **sun8i_layers_init(struct drm_device *drm,
> > else
> > type = DRM_PLANE_TYPE_OVERLAY;
> >
> > - phy_index = i;
> > - if (mixer->cfg->de_type == SUN8I_MIXER_DE33)
> > - phy_index = mixer->cfg->map[i];
> > -
> > layer = sun8i_vi_layer_init_one(drm, type, mixer->engine.regs,
> > - i, phy_index, plane_cnt,
> > + i, i, plane_cnt,
> > &mixer->cfg->lay_cfg);
> > if (IS_ERR(layer)) {
> > dev_err(drm->dev,
> > @@ -357,12 +353,8 @@ static struct drm_plane **sun8i_layers_init(struct drm_device *drm,
> > else
> > type = DRM_PLANE_TYPE_OVERLAY;
> >
> > - phy_index = index;
> > - if (mixer->cfg->de_type == SUN8I_MIXER_DE33)
> > - phy_index = mixer->cfg->map[index];
> > -
> > layer = sun8i_ui_layer_init_one(drm, type, mixer->engine.regs,
> > - index, phy_index, plane_cnt,
> > + index, index, plane_cnt,
> > &mixer->cfg->lay_cfg);
> > if (IS_ERR(layer)) {
> > dev_err(drm->dev, "Couldn't initialize %s plane\n",
> > @@ -376,16 +368,25 @@ static struct drm_plane **sun8i_layers_init(struct drm_device *drm,
> > return planes;
> > }
> >
> > +static struct drm_plane **sun50i_layers_init(struct drm_device *drm,
> > + struct sunxi_engine *engine)
> > +{
> > + struct sun8i_mixer *mixer = engine_to_sun8i_mixer(engine);
> > +
> > + if (IS_ENABLED(CONFIG_DRM_SUN50I_PLANES))
> > + return sun50i_planes_setup(mixer->planes_dev, drm, engine->id);
> > +
> > + return NULL;
> > +}
> > +
> > static void sun8i_mixer_mode_set(struct sunxi_engine *engine,
> > const struct drm_display_mode *mode)
> > {
> > struct sun8i_mixer *mixer = engine_to_sun8i_mixer(engine);
> > - struct regmap *bld_regs;
> > u32 bld_base, size, val;
> > bool interlaced;
> >
> > bld_base = sun8i_blender_base(mixer);
> > - bld_regs = sun8i_blender_regmap(mixer);
> > interlaced = !!(mode->flags & DRM_MODE_FLAG_INTERLACE);
> > size = SUN8I_MIXER_SIZE(mode->hdisplay, mode->vdisplay);
> >
> > @@ -397,14 +398,14 @@ static void sun8i_mixer_mode_set(struct sunxi_engine *engine,
> > else
> > regmap_write(mixer->engine.regs, SUN8I_MIXER_GLOBAL_SIZE, size);
> >
> > - regmap_write(bld_regs, SUN8I_MIXER_BLEND_OUTSIZE(bld_base), size);
> > + regmap_write(engine->regs, SUN8I_MIXER_BLEND_OUTSIZE(bld_base), size);
> >
> > if (interlaced)
> > val = SUN8I_MIXER_BLEND_OUTCTL_INTERLACED;
> > else
> > val = 0;
> >
> > - regmap_update_bits(bld_regs, SUN8I_MIXER_BLEND_OUTCTL(bld_base),
> > + regmap_update_bits(engine->regs, SUN8I_MIXER_BLEND_OUTCTL(bld_base),
> > SUN8I_MIXER_BLEND_OUTCTL_INTERLACED, val);
> >
> > DRM_DEBUG_DRIVER("Switching display mixer interlaced mode %s\n",
> > @@ -417,8 +418,14 @@ static const struct sunxi_engine_ops sun8i_engine_ops = {
> > .mode_set = sun8i_mixer_mode_set,
> > };
> >
> > +static const struct sunxi_engine_ops sun50i_engine_ops = {
> > + .commit = sun8i_mixer_commit,
> > + .layers_init = sun50i_layers_init,
> > + .mode_set = sun8i_mixer_mode_set,
> > +};
> > +
> > static const struct regmap_config sun8i_mixer_regmap_config = {
> > - .name = "layers",
> > + .name = "display",
> > .reg_bits = 32,
> > .val_bits = 32,
> > .reg_stride = 4,
> > @@ -433,14 +440,6 @@ static const struct regmap_config sun8i_top_regmap_config = {
> > .max_register = 0x3c,
> > };
> >
> > -static const struct regmap_config sun8i_disp_regmap_config = {
> > - .name = "display",
> > - .reg_bits = 32,
> > - .val_bits = 32,
> > - .reg_stride = 4,
> > - .max_register = 0x20000,
> > -};
> > -
> > static int sun8i_mixer_of_get_id(struct device_node *node)
> > {
> > struct device_node *ep, *remote;
> > @@ -463,17 +462,14 @@ static int sun8i_mixer_of_get_id(struct device_node *node)
> >
> > static void sun8i_mixer_init(struct sun8i_mixer *mixer)
> > {
> > - struct regmap *top_regs, *disp_regs;
> > unsigned int base = sun8i_blender_base(mixer);
> > + struct regmap *top_regs;
> > int plane_cnt, i;
> >
> > - if (mixer->cfg->de_type == SUN8I_MIXER_DE33) {
> > + if (mixer->cfg->de_type == SUN8I_MIXER_DE33)
> > top_regs = mixer->top_regs;
> > - disp_regs = mixer->disp_regs;
> > - } else {
> > + else
> > top_regs = mixer->engine.regs;
> > - disp_regs = mixer->engine.regs;
> > - }
> >
> > /* Enable the mixer */
> > regmap_write(top_regs, SUN8I_MIXER_GLOBAL_CTL,
> > @@ -483,25 +479,25 @@ static void sun8i_mixer_init(struct sun8i_mixer *mixer)
> > regmap_write(top_regs, SUN50I_MIXER_GLOBAL_CLK, 1);
> >
> > /* Set background color to black */
> > - regmap_write(disp_regs, SUN8I_MIXER_BLEND_BKCOLOR(base),
> > + 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(disp_regs, SUN8I_MIXER_BLEND_PIPE_CTL(base),
> > + regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_PIPE_CTL(base),
> > SUN8I_MIXER_BLEND_PIPE_CTL_FC_EN(0));
> > - regmap_write(disp_regs, SUN8I_MIXER_BLEND_ATTR_FCOLOR(base, 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(disp_regs,
> > + regmap_write(mixer->engine.regs,
> > SUN8I_MIXER_BLEND_MODE(base, i),
> > SUN8I_MIXER_BLEND_MODE_DEF);
> >
> > - regmap_update_bits(disp_regs, SUN8I_MIXER_BLEND_PIPE_CTL(base),
> > + regmap_update_bits(mixer->engine.regs, SUN8I_MIXER_BLEND_PIPE_CTL(base),
> > SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK, 0);
> > }
> >
> > @@ -532,7 +528,6 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
> > if (!mixer)
> > return -ENOMEM;
> > dev_set_drvdata(dev, mixer);
> > - mixer->engine.ops = &sun8i_engine_ops;
> > mixer->engine.node = dev->of_node;
> >
> > if (of_property_present(dev->of_node, "iommus")) {
> > @@ -562,6 +557,11 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
> > if (!mixer->cfg)
> > return -EINVAL;
> >
> > + if (mixer->cfg->de_type == SUN8I_MIXER_DE33)
> > + mixer->engine.ops = &sun50i_engine_ops;
>
> You're missing an IS_ENABLED() clause here if you wanted to make the DE 3.3
> planes driver optional. Though as I mentioned in the other patch, splittig
> the two modules might not work.
>
> > + else
> > + mixer->engine.ops = &sun8i_engine_ops;
> > +
> > regs = devm_platform_ioremap_resource(pdev, 0);
> > if (IS_ERR(regs))
> > return PTR_ERR(regs);
> > @@ -584,17 +584,6 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
> > dev_err(dev, "Couldn't create the top regmap\n");
> > return PTR_ERR(mixer->top_regs);
> > }
> > -
> > - regs = devm_platform_ioremap_resource_byname(pdev, "display");
> > - if (IS_ERR(regs))
> > - return PTR_ERR(regs);
> > -
> > - mixer->disp_regs = devm_regmap_init_mmio(dev, regs,
> > - &sun8i_disp_regmap_config);
> > - if (IS_ERR(mixer->disp_regs)) {
> > - dev_err(dev, "Couldn't create the disp regmap\n");
> > - return PTR_ERR(mixer->disp_regs);
> > - }
> > }
> >
> > mixer->reset = devm_reset_control_get(dev, NULL);
> > @@ -634,6 +623,33 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
> >
> > clk_prepare_enable(mixer->mod_clk);
> >
> > + if (mixer->cfg->de_type == SUN8I_MIXER_DE33) {
> > + struct platform_device *pdev;
> > + struct device_node *np;
> > + void *data;
> > +
> > + np = of_parse_phandle(dev->of_node, "allwinner,planes", 0);
> > + if (!np) {
> > + ret = -ENODEV;
> > + goto err_disable_mod_clk;
> > + }
> > +
> > + pdev = of_find_device_by_node(np);
>
> You need to add a matching put_device() in the unbind function.
>
> Side note:
>
> This bind function is using a lot of devm_ functions. These have the wrong
> lifetime. I think it would be better if we could move resource acquisition
> into the probe function.
Looking a bit more into this, this requires a bit more work. For example, clocks
can be provided by tcon-top, which are created only in bind callback. Basically,
whole sun4i-drm driver depends on devm_* calls in bind functions. This would
need careful analysis of all driver calls and then refactoring drivers one by one.
IMO tcon-top driver needs to be refactored to plain clock driver without component
bind/unbind functions. Although this may cause slightly higher power consumption
if device doesn't have display but driver is loaded nevertheless.
What do you think?
Best regards,
Jernej
More information about the linux-arm-kernel
mailing list