From: "Jernej Škrabec" <jernej.skrabec@gmail.com>
To: wens@kernel.org
Cc: samuel@sholland.org, mripard@kernel.org,
maarten.lankhorst@linux.intel.com, tzimmermann@suse.de,
airlied@gmail.com, simona@ffwll.ch, robh@kernel.org,
krzk+dt@kernel.org, conor+dt@kernel.org, mturquette@baylibre.com,
sboyd@kernel.org, dri-devel@lists.freedesktop.org,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org,
linux-clk@vger.kernel.org
Subject: Re: [PATCH 7/7] drm/sun4i: switch DE33 to new bindings
Date: Thu, 25 Dec 2025 20:20:29 +0100 [thread overview]
Message-ID: <3684052.iIbC2pHGDl@jernej-laptop> (raw)
In-Reply-To: <CAGb2v67B_2SMsDbATVu51Ed+2o+6=FRLw7KmjeL1KYgxm=gyDw@mail.gmail.com>
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.
No, as I said in previous response, I'll merge it into sun8i-mixer.
>
> > + 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.
Good point. This can be clean up later.
>
>
> > + of_node_put(np);
> > + if (!pdev) {
> > + ret = -EPROBE_DEFER;
> > + goto err_disable_mod_clk;
> > + }
> > +
> > + data = platform_get_drvdata(pdev);
> > + if (!data) {
> > + put_device(&pdev->dev);
> > + return -EPROBE_DEFER;
>
> Should be a goto here?
Right.
Best regards,
Jernej
>
>
> ChenYu
>
> > + }
> > +
> > + mixer->planes_dev = &pdev->dev;
> > + }
> > +
> > list_add_tail(&mixer->engine.list, &drv->engine_list);
> >
> > /* Reset registers and disable unused sub-engines */
> > @@ -668,6 +684,8 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
> >
> > return 0;
> >
> > +err_disable_mod_clk:
> > + clk_disable_unprepare(mixer->mod_clk);
> > err_disable_bus_clk:
> > clk_disable_unprepare(mixer->bus_clk);
> > err_assert_reset:
> > @@ -863,16 +881,8 @@ static const struct sun8i_mixer_cfg sun50i_h6_mixer0_cfg = {
> > };
> >
> > static const struct sun8i_mixer_cfg sun50i_h616_mixer0_cfg = {
> > - .lay_cfg = {
> > - .de_type = SUN8I_MIXER_DE33,
> > - .scaler_mask = 0xf,
> > - .scanline_yuv = 4096,
> > - },
> > .de_type = SUN8I_MIXER_DE33,
> > .mod_rate = 600000000,
> > - .ui_num = 3,
> > - .vi_num = 1,
> > - .map = {0, 6, 7, 8},
> > };
> >
> > static const struct of_device_id sun8i_mixer_of_table[] = {
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > index e2f83301aae8..7abc88c898d9 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > @@ -202,7 +202,6 @@ struct sun8i_mixer_cfg {
> > int ui_num;
> > unsigned int de_type;
> > unsigned long mod_rate;
> > - unsigned int map[6];
> > };
> >
> > struct sun8i_mixer {
> > @@ -216,7 +215,7 @@ struct sun8i_mixer {
> > struct clk *mod_clk;
> >
> > struct regmap *top_regs;
> > - struct regmap *disp_regs;
> > + struct device *planes_dev;
> > };
> >
> > enum {
> > @@ -252,13 +251,6 @@ sun8i_blender_base(struct sun8i_mixer *mixer)
> > return mixer->cfg->de_type == SUN8I_MIXER_DE3 ? DE3_BLD_BASE : DE2_BLD_BASE;
> > }
> >
> > -static inline struct regmap *
> > -sun8i_blender_regmap(struct sun8i_mixer *mixer)
> > -{
> > - return mixer->cfg->de_type == SUN8I_MIXER_DE33 ?
> > - mixer->disp_regs : mixer->engine.regs;
> > -}
> > -
> > static inline u32
> > sun8i_channel_base(struct sun8i_layer *layer)
> > {
> > --
> > 2.51.2
> >
> >
>
next prev parent reply other threads:[~2025-12-25 19:20 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-11-15 14:13 [PATCH 0/7] drm/sun4i: update DE33 support Jernej Skrabec
2025-11-15 14:13 ` [PATCH 1/7] drm/sun4i: Add support for DE33 CSC Jernej Skrabec
2025-12-25 8:22 ` Chen-Yu Tsai
2025-11-15 14:13 ` [PATCH 2/7] drm/sun4i: vi_layer: Limit formats for DE33 Jernej Skrabec
2025-11-15 14:40 ` Chen-Yu Tsai
2025-11-15 14:47 ` Jernej Škrabec
2025-11-15 14:13 ` [PATCH 3/7] clk: sunxi-ng: de2: Export register regmap " Jernej Skrabec
2025-11-15 17:37 ` Chen-Yu Tsai
2025-11-15 14:13 ` [PATCH 4/7] dt-bindings: display: allwinner: Add DE33 planes Jernej Skrabec
2025-11-16 11:29 ` Krzysztof Kozlowski
2025-11-16 11:44 ` Jernej Škrabec
2025-11-16 11:49 ` Krzysztof Kozlowski
2025-11-16 12:10 ` Jernej Škrabec
2025-11-15 14:13 ` [PATCH 5/7] drm/sun4i: Add planes driver Jernej Skrabec
2025-12-25 9:29 ` Chen-Yu Tsai
2025-12-25 9:37 ` Chen-Yu Tsai
2025-12-25 19:16 ` Jernej Škrabec
2025-12-25 19:30 ` Chen-Yu Tsai
2025-12-25 19:34 ` Jernej Škrabec
2025-11-15 14:13 ` [PATCH 6/7] dt-bindings: display: allwinner: Update H616 DE33 binding Jernej Skrabec
2025-11-16 11:33 ` Krzysztof Kozlowski
2025-11-16 11:33 ` Krzysztof Kozlowski
2025-11-16 12:00 ` Jernej Škrabec
2025-11-16 12:07 ` Chen-Yu Tsai
2025-11-15 14:13 ` [PATCH 7/7] drm/sun4i: switch DE33 to new bindings Jernej Skrabec
2025-12-25 9:49 ` Chen-Yu Tsai
2025-12-25 19:20 ` Jernej Škrabec [this message]
2026-02-14 20:55 ` Jernej Škrabec
2026-02-15 7:13 ` Chen-Yu Tsai
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3684052.iIbC2pHGDl@jernej-laptop \
--to=jernej.skrabec@gmail.com \
--cc=airlied@gmail.com \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=krzk+dt@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-sunxi@lists.linux.dev \
--cc=maarten.lankhorst@linux.intel.com \
--cc=mripard@kernel.org \
--cc=mturquette@baylibre.com \
--cc=robh@kernel.org \
--cc=samuel@sholland.org \
--cc=sboyd@kernel.org \
--cc=simona@ffwll.ch \
--cc=tzimmermann@suse.de \
--cc=wens@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.