Re: [PATCH 7/7] drm/sun4i: switch DE33 to new bindings

Chen-Yu Tsai
2026-02-15T07:13:14Z

On Sun, Feb 15, 2026 at 3:03 PM Jernej Škrabec <jernej.skrabec@gmail.com> wrote:
>
> 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@gmail.com> wrote:
> > >
> > > Now that everything is in place, switch DE33 to new bindings.
> > >
> > > Signed-off-by: Jernej Skrabec <jernej.skrabec@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.

Unfortunately so. However most of them just require moving the initial
context memory allocation and whatever resource acquisition over. It
shouldn't be that messy.

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

It's just the bus clock that's enabled all the time, so I think that's fine.
However since it sits in the middle of the whole OF graph, you either need
to make sun4i-drm know to skip that when adding all the components, or just
keep a dummy bind callback and component to make it happy.

Thanks
ChenYu