在 2025-08-17星期日的 01:55 +0800，Icenowy Zheng写道：
> 在 2025-08-16星期六的 20:45 +0300，Dmitry Baryshkov写道：
> > On Sun, Aug 17, 2025 at 12:48:42AM +0800, Icenowy Zheng wrote:
> > > 在 2025-08-16星期六的 19:18 +0300，Dmitry Baryshkov写道：
> > > > On Fri, Aug 15, 2025 at 12:40:43AM +0800, Icenowy Zheng wrote:
> > > > >
8< Some contents got cut here ============
> > > > >
> >
> > Please don't follow that pattern. It breaks as soon as userspace
> > submits
> > an DRM_MODE_ATOMIC_TEST_ONLY commit. It's hard for encoders since
> > they
> > don't have a state, but bridges have proper drm_bridge_state.
> > Please
> > use
> > it.
>
> Yes I thought it have proper drm_bridge_state, but I cannot
> understand
> why I got always 0 when accessing bridge_state->output_bus_cfg.format
> in atomic_enable().
>
> If I follow this pattern, then when this problem is fixed, my driver
> can get fixed along with ingenic and meson.
Well I check the codebase, and the other encoder (root bridge) driver
that really sets output format, mtk_dpi, does the same thing by saving
the format in struct mtk_dpi.
I now have totally no idea about why this happened...
>
> >
> > >
> > > >
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +static void vs_bridge_atomic_enable(struct drm_bridge
> > > > > *bridge,
> > > > > + struct drm_atomic_state
> > > > > *state)
> > > > > +{
> > > > > + struct vs_bridge *vbridge =
> > > > > drm_bridge_to_vs_bridge(bridge);
> > > > > + struct drm_bridge_state *br_state =
> > > > > drm_atomic_get_bridge_state(state,
> > > > > +
> > > > >
> > > > >
> > > > > bridge);
> > > > > + struct vs_crtc *crtc = vbridge->crtc;
> > > > > + struct vs_dc *dc = crtc->dc;
> > > > > + unsigned int output = crtc->id;
> > > > > + u32 dp_fmt;
> > > > > + unsigned int i;
> > > > > +
> > > > > + DRM_DEBUG_DRIVER("Enabling output %u\n", output);
> > > > > +
> > > > > + switch (vbridge->intf) {
> > > > > + case VSDC_OUTPUT_INTERFACE_DPI:
> > > > > + regmap_clear_bits(dc->regs,
> > > > > VSDC_DISP_DP_CONFIG(output),
> > > > > + VSDC_DISP_DP_CONFIG_DP_EN);
> > > > > + break;
> > > > > + case VSDC_OUTPUT_INTERFACE_DP:
> > > > > + for (i = 0; i <
> > > > > ARRAY_SIZE(vsdc_dp_supported_fmts);
> > > > > i++) {
> > > > > + if
> > > > > (vsdc_dp_supported_fmts[i].linux_fmt
> > > > > ==
> > > > > + vbridge->output_bus_fmt)
> > > > > + break;
> > > > > + }
> > > > > + WARN_ON_ONCE(i ==
> > > > > ARRAY_SIZE(vsdc_dp_supported_fmts));
> > > > > + dp_fmt = vsdc_dp_supported_fmts[i].vsdc_fmt;
> > > >
> > > > This might trigger all static checkers in the universe. It's
> > > > not
> > > > really
> > > > possible, since you've checked it in the atomic_check(), but...
> > >
> > > Sigh I don't know how to properly describe it...
> > >
> > > I can only say something really bad happens if the previous
> > > WARN_ON_ONCE is triggered.
> >
> >
> > if (WARN_ON_ONCE())
> > return;
>
> Sounds reasonable, as it's a UB here.
>
> >
> > >
> > > >
> > > > > + dp_fmt |= VSDC_DISP_DP_CONFIG_DP_EN;
> > > > > + regmap_write(dc->regs,
> > > > > VSDC_DISP_DP_CONFIG(output),
> > > > > dp_fmt);
> > > > > + regmap_assign_bits(dc->regs,
> > > > > +
> > > > > VSDC_DISP_PANEL_CONFIG(output),
> > > > > +
> > > > > VSDC_DISP_PANEL_CONFIG_YUV,
> > > > > +
> > > > > vsdc_dp_supported_fmts[i].is_yuv);
> > > > > + break;
> > > > > + }
> > > > > +
> > > > > + regmap_clear_bits(dc->regs,
> > > > > VSDC_DISP_PANEL_CONFIG(output),
> > > > > + VSDC_DISP_PANEL_CONFIG_DAT_POL);
> > > > > + regmap_assign_bits(dc->regs,
> > > > > VSDC_DISP_PANEL_CONFIG(output),
> > > > > + VSDC_DISP_PANEL_CONFIG_DE_POL,
> > > > > + br_state->output_bus_cfg.flags &
> > > > > + DRM_BUS_FLAG_DE_LOW);
> > > > > + regmap_assign_bits(dc->regs,
> > > > > VSDC_DISP_PANEL_CONFIG(output),
> > > > > + VSDC_DISP_PANEL_CONFIG_CLK_POL,
> > > > > + br_state->output_bus_cfg.flags &
> > > > > +
> > > > > DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE);
> > > > > + regmap_set_bits(dc->regs,
> > > > > VSDC_DISP_PANEL_CONFIG(output),
> > > > > + VSDC_DISP_PANEL_CONFIG_DE_EN |
> > > > > + VSDC_DISP_PANEL_CONFIG_DAT_EN |
> > > > > + VSDC_DISP_PANEL_CONFIG_CLK_EN);
> > > > > + regmap_set_bits(dc->regs,
> > > > > VSDC_DISP_PANEL_CONFIG(output),
> > > > > + VSDC_DISP_PANEL_CONFIG_RUNNING);
> > > > > + regmap_clear_bits(dc->regs, VSDC_DISP_PANEL_START,
> > > > > +
> > > > > VSDC_DISP_PANEL_START_MULTI_DISP_SYNC);
> > > > > + regmap_set_bits(dc->regs, VSDC_DISP_PANEL_START,
> > > > > + VSDC_DISP_PANEL_START_RUNNING(output)
> > > > > );
> > > > > +
> > > > > + regmap_set_bits(dc->regs,
> > > > > VSDC_DISP_PANEL_CONFIG_EX(crtc-
> > > > > > id),
> > > > > + VSDC_DISP_PANEL_CONFIG_EX_COMMIT);
> > > > > +}
> > > > > +
> > > > > +static void vs_bridge_atomic_disable(struct drm_bridge
> > > > > *bridge,
> > > > > + struct drm_atomic_state
> > > > > *state)
> > > > > +{
> > > > > + struct vs_bridge *vbridge =
> > > > > drm_bridge_to_vs_bridge(bridge);
> > > > > + struct vs_crtc *crtc = vbridge->crtc;
> > > > > + struct vs_dc *dc = crtc->dc;
> > > > > + unsigned int output = crtc->id;
> > > > > +
> > > > > + DRM_DEBUG_DRIVER("Disabling output %u\n", output);
> > > > > +
> > > > > + regmap_clear_bits(dc->regs, VSDC_DISP_PANEL_START,
> > > > > +
> > > > > VSDC_DISP_PANEL_START_MULTI_DISP_SYNC
> > > > > >
> > > > > +
> > > > > VSDC_DISP_PANEL_START_RUNNING(output));
> > > > > + regmap_clear_bits(dc->regs,
> > > > > VSDC_DISP_PANEL_CONFIG(output),
> > > > > + VSDC_DISP_PANEL_CONFIG_RUNNING);
> > > > > +
> > > > > + regmap_set_bits(dc->regs,
> > > > > VSDC_DISP_PANEL_CONFIG_EX(crtc-
> > > > > > id),
> > > > > + VSDC_DISP_PANEL_CONFIG_EX_COMMIT);
> > > > > +}
> > > > > +
> > > > > +static const struct drm_bridge_funcs vs_bridge_funcs = {
> > > > > + .attach = vs_bridge_attach,
> > > > > + .atomic_enable = vs_bridge_atomic_enable,
> > > > > + .atomic_disable = vs_bridge_atomic_disable,
> > > > > + .atomic_check = vs_bridge_atomic_check,
> > > > > + .atomic_get_input_bus_fmts =
> > > > > vs_bridge_atomic_get_input_bus_fmts,
> > > > > + .atomic_get_output_bus_fmts =
> > > > > vs_bridge_atomic_get_output_bus_fmts,
> > > > > + .atomic_duplicate_state =
> > > > > drm_atomic_helper_bridge_duplicate_state,
> > > > > + .atomic_destroy_state =
> > > > > drm_atomic_helper_bridge_destroy_state,
> > > > > + .atomic_reset = drm_atomic_helper_bridge_reset,
> > > > > +};
> > > > > +
> > > > > +static int vs_bridge_detect_output_interface(struct
> > > > > device_node
> > > > > *of_node,
> > > > > + unsigned int
> > > > > output)
> > > > > +{
> > > > > + int ret;
> > > > > + struct device_node *remote;
> > > > > +
> > > > > + remote = of_graph_get_remote_node(of_node, output,
> > > > > +
> > > > > VSDC_OUTPUT_INTERFACE_DPI);
> > > >
> > > > This deserves a comment in the source file.
> > > >
> > > > > + if (remote) {
> > > > > + ret = VSDC_OUTPUT_INTERFACE_DPI;
> > > >
> > > > return here, drop else{}
> > >
> > > Well a of_node_put() is missing before the final return, and Yao
> > > Zi
> > > noted me of it.
> >
> > You can put the node right after of_graph_get_remote_node(); You
> > don't
> > use any props from it.
> >
> > >
> > > >
> > > > > + } else {
> > > > > + remote = of_graph_get_remote_node(of_node,
> > > > > output,
> > > > > +
> > > > > VSDC_OUTPUT_INTERFACE_DP);
> > > > > + if (remote)
> > > > > + ret = VSDC_OUTPUT_INTERFACE_DP;
> > > >
> > > > return
> > > >
> > > > > + else
> > > > > + ret = -ENODEV;
> > > > > + }
> > > > > +
> > > > > + return ret;
> > > > > +}
> > > > > +
> > > > > +struct vs_bridge *vs_bridge_init(struct drm_device *drm_dev,
> > > > > + struct vs_crtc *crtc)
> > > > > +{
> > > > > + unsigned int output = crtc->id;
> > > > > + struct vs_bridge *bridge;
> > > > > + struct drm_bridge *next;
> > > > > + enum vs_bridge_output_interface intf;
> > > > > + int ret;
> > > > > +
> > > > > + intf = vs_bridge_detect_output_interface(drm_dev-
> > > > > >dev-
> > > > > > of_node,
> > > > > + output);
> > > > > + if (intf == -ENODEV) {
> > > > > + dev_info(drm_dev->dev, "Skipping output
> > > > > %u\n",
> > > > > output);
> > > > > + return NULL;
> > > > > + }
> > > > > +
> > > > > + bridge = devm_kzalloc(drm_dev->dev, sizeof(*bridge),
> > > > > GFP_KERNEL);
> > > >
> > > > devm_drm_bridge_alloc()
> > > >
> > > > > + if (!bridge)
> > > > > + return ERR_PTR(-ENOMEM);
> > > > > +
> > > > > + bridge->crtc = crtc;
> > > > > + bridge->intf = intf;
> > > > > + bridge->base.funcs = &vs_bridge_funcs;
> > > > > +
> > > > > + next = devm_drm_of_get_bridge(drm_dev->dev, drm_dev-
> > > > > > dev-
> > > > > > of_node,
> > > > > + output, intf);
> > > > > + if (IS_ERR(next)) {
> > > > > + ret = PTR_ERR(next);
> > > > > + goto err_free_bridge;
> > > > > + }
> > > > > +
> > > > > + bridge->next = next;
> > > > > +
> > > > > + ret = drm_simple_encoder_init(drm_dev, &bridge->enc,
> > > >
> > > > Oh, so there is an encoder... Please drop drm_simple_encoder,
> > > > it's
> > > > deprecated, and try moving all the ifs to the encoder funcs.
> > >
> > > Ah? Is it really deprecated? I can find no source of this
> > > deprecation.
> >
> > https://lore.kernel.org/dri-devel/20250401094056.32904-3-tzimmerm...@suse.de/
> >
> > > In addition, I think many drivers here are using a bridge as a
> > > "better
> > > encoder" because of the restriction of current encoder
> > > implementation,
> > > and I am doing the same thing. Either encoder functionality
> > > should
> > > be
> > > improved to on par with bridge, or such dummy encoders with a
> > > bridge
> > > should exist, and some helper for creating them should exist. It
> > > might
> > > be not drm_simple_encoder_init (because I can understand the
> > > deprecation of other parts of the simple-kms routines, although I
> > > see
> > > no formal documentation mentioning it's deprecated, maybe I
> > > missed
> > > some
> > > newspaper?), but it should exist.
> >
> > Maybe we should explicitly document the status.
> >
> > Also, if you use non-simple encoders, you can actually have
> > functionality there.
>
> Yes I tried it, but then I realized that it's not a good pattern if a
> root bridge is present -- they represent the same thing in the
> hardware.
>
> >
> > >
> > > >
> > > > > + (intf ==
> > > > > VSDC_OUTPUT_INTERFACE_DPI) ?
> > > > > + DRM_MODE_ENCODER_DPI :
> > > > > + DRM_MODE_ENCODER_NONE);
> > > > > + if (ret) {
> > > > > + dev_err(drm_dev->dev,
> > > > > + "Cannot initialize encoder for output
> > > > > %u\n", output);
> > > > > + goto err_free_bridge;
> > > > > + }
> > > > > +
> > > > > + bridge->enc.possible_crtcs = drm_crtc_mask(&crtc-
> > > > > > base);
> > > > > +
> > > > > + ret = drm_bridge_attach(&bridge->enc, &bridge->base,
> > > > > NULL,
> > > > > + DRM_BRIDGE_ATTACH_NO_CONNECTO
> > > > > R)
> > > > > ;
> > > > > + if (ret) {
> > > > > + dev_err(drm_dev->dev,
> > > > > + "Cannot attach bridge for output
> > > > > %u\n",
> > > > > output);
> > > > > + goto err_cleanup_encoder;
> > > > > + }
> > > > > +
> > > > > + bridge->conn = drm_bridge_connector_init(drm_dev,
> > > > > &bridge-
> > > > > > enc);
> > > > > + if (IS_ERR(bridge->conn)) {
> > > > > + dev_err(drm_dev->dev,
> > > > > + "Cannot create connector for output
> > > > > %u\n",
> > > > > output);
> > > > > + ret = PTR_ERR(bridge->conn);
> > > > > + goto err_cleanup_encoder;
> > > > > + }
> > > > > + drm_connector_attach_encoder(bridge->conn, &bridge-
> > > > > > enc);
> > > > > +
> > > > > + return bridge;
> > > > > +
> > > > > +err_cleanup_encoder:
> > > > > + drm_encoder_cleanup(&bridge->enc);
> > > > > +err_free_bridge:
> > > > > + devm_kfree(drm_dev->dev, bridge);
> > > > > +
> > > > > + return ERR_PTR(ret);
> > > > > +}
> > > > > diff --git a/drivers/gpu/drm/verisilicon/vs_bridge.h
> > > > > b/drivers/gpu/drm/verisilicon/vs_bridge.h
> > > > > new file mode 100644
> > > > > index 0000000000000..4a8a9eeb739f2
> > > > > --- /dev/null
> > > > > +++ b/drivers/gpu/drm/verisilicon/vs_bridge.h
> > > > > @@ -0,0 +1,40 @@
> > > > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > > > +/*
> > > > > + * Copyright (C) 2025 Icenowy Zheng <u...@icenowy.me>
> > > > > + */
> > > > > +
> > > > > +#ifndef _VS_BRIDGE_H_
> > > > > +#define _VS_BRIDGE_H_
> > > > > +
> > > > > +#include <linux/types.h>
> > > > > +
> > > > > +#include <drm/drm_bridge.h>
> > > > > +#include <drm/drm_connector.h>
> > > > > +#include <drm/drm_encoder.h>
> > > > > +
> > > > > +struct vs_crtc;
> > > > > +
> > > > > +enum vs_bridge_output_interface {
> > > > > + VSDC_OUTPUT_INTERFACE_DPI = 0,
> > > > > + VSDC_OUTPUT_INTERFACE_DP = 1
> > > > > +};
> > > > > +
> > > > > +struct vs_bridge {
> > > > > + struct drm_bridge base;
> > > > > + struct drm_encoder enc;
> > > > > + struct drm_connector *conn;
> > > > > +
> > > > > + struct vs_crtc *crtc;
> > > > > + struct drm_bridge *next;
> > > > > + enum vs_bridge_output_interface intf;
> > > > > + u32 output_bus_fmt;
> > > > > +};
> > > > > +
> > > > > +static inline struct vs_bridge
> > > > > *drm_bridge_to_vs_bridge(struct
> > > > > drm_bridge *bridge)
> > > > > +{
> > > > > + return container_of(bridge, struct vs_bridge, base);
> > > > > +}
> > > > > +
> > > > > +struct vs_bridge *vs_bridge_init(struct drm_device *drm_dev,
> > > > > + struct vs_crtc *crtc);
> > > > > +#endif /* _VS_BRIDGE_H_ */
> > > > > diff --git a/drivers/gpu/drm/verisilicon/vs_bridge_regs.h
> > > > > b/drivers/gpu/drm/verisilicon/vs_bridge_regs.h
> > > > > new file mode 100644
> > > > > index 0000000000000..d1c91dd1354b4
> > > > > --- /dev/null
> > > > > +++ b/drivers/gpu/drm/verisilicon/vs_bridge_regs.h
> > > > > @@ -0,0 +1,47 @@
> > > > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > > > +/*
> > > > > + * Copyright (C) 2025 Icenowy Zheng <u...@icenowy.me>
> > > > > + *
> > > > > + * Based on vs_dc_hw.h, which is:
> > > > > + * Copyright (C) 2023 VeriSilicon Holdings Co., Ltd.
> > > > > + */
> > > > > +
> > > > > +#ifndef _VS_BRIDGE_REGS_H_
> > > > > +#define _VS_BRIDGE_REGS_H_
> > > > > +
> > > > > +#include <linux/bits.h>
> > > > > +
> > > > > +#define VSDC_DISP_PANEL_CONFIG(n) (0x1418 + 0x4
> > > > > *
> > > > > (n))
> > > > > +#define VSDC_DISP_PANEL_CONFIG_DE_EN BIT(0)
> > > > > +#define VSDC_DISP_PANEL_CONFIG_DE_POL BIT(1)
> > > > > +#define VSDC_DISP_PANEL_CONFIG_DAT_EN BIT(4)
> > > > > +#define VSDC_DISP_PANEL_CONFIG_DAT_POL BIT(5)
> > > > > +#define VSDC_DISP_PANEL_CONFIG_CLK_EN BIT(8)
> > > > > +#define VSDC_DISP_PANEL_CONFIG_CLK_POL BIT(9)
> > > > > +#define VSDC_DISP_PANEL_CONFIG_RUNNING BIT(12)
> > > > > +#define VSDC_DISP_PANEL_CONFIG_GAMMA BIT(13)
> > > > > +#define VSDC_DISP_PANEL_CONFIG_YUV BIT(16)
> > > > > +
> > > > > +#define VSDC_DISP_PANEL_START 0x1CCC
> > > > > +#define VSDC_DISP_PANEL_START_RUNNING(n) BIT(n)
> > > > > +#define VSDC_DISP_PANEL_START_MULTI_DISP_SYNC BIT(3)
> > > > > +
> > > > > +#define VSDC_DISP_DP_CONFIG(n) (0x1CD0 + 0x4
> > > > > *
> > > > > (n))
> > > > > +#define VSDC_DISP_DP_CONFIG_DP_EN BIT(3)
> > > > > +#define VSDC_DISP_DP_CONFIG_FMT_MASK GENMASK(2, 0)
> > > > > +#define VSDC_DISP_DP_CONFIG_FMT_RGB565 (0)
> > > > > +#define VSDC_DISP_DP_CONFIG_FMT_RGB666 (1)
> > > > > +#define VSDC_DISP_DP_CONFIG_FMT_RGB888 (2)
> > > > > +#define VSDC_DISP_DP_CONFIG_FMT_RGB101010 (3)
> > > > > +#define VSDC_DISP_DP_CONFIG_YUV_FMT_MASK GENMASK(7, 4)
> > > > > +#define VSDC_DISP_DP_CONFIG_YUV_FMT_UYVY8 (2 << 4)
> > > > > +#define VSDC_DISP_DP_CONFIG_YUV_FMT_YUV8 (4 << 4)
> > > > > +#define VSDC_DISP_DP_CONFIG_YUV_FMT_UYVY10 (8 << 4)
> > > > > +#define VSDC_DISP_DP_CONFIG_YUV_FMT_YUV10 (10 << 4)
> > > > > +#define VSDC_DISP_DP_CONFIG_YUV_FMT_UYYVYY8 (12 << 4)
> > > > > +#define VSDC_DISP_DP_CONFIG_YUV_FMT_UYYVYY10 (13 << 4)
> > > > > +
> > > > > +#define VSDC_DISP_PANEL_CONFIG_EX(n) (0x2518 + 0x4
> > > > > *
> > > > > (n))
> > > > > +#define VSDC_DISP_PANEL_CONFIG_EX_COMMIT BIT(0)
> > > > > +
> > > > > +#endif /* _VS_BRIDGE_REGS_H_ */
> > > > > diff --git a/drivers/gpu/drm/verisilicon/vs_crtc.c
> > > > > b/drivers/gpu/drm/verisilicon/vs_crtc.c
> > > > > new file mode 100644
> > > > > index 0000000000000..46c4191b82f49
> > > > > --- /dev/null
> > > > > +++ b/drivers/gpu/drm/verisilicon/vs_crtc.c
> > > > > @@ -0,0 +1,217 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > > +/*
> > > > > + * Copyright (C) 2025 Icenowy Zheng <u...@icenowy.me>
> > > > > + */
> > > > > +
> > > > > +#include <linux/clk.h>
> > > > > +#include <linux/regmap.h>
> > > > > +
> > > > > +#include <drm/drm_atomic.h>
> > > > > +#include <drm/drm_atomic_helper.h>
> > > > > +#include <drm/drm_print.h>
> > > > > +
> > > > > +#include "vs_crtc_regs.h"
> > > > > +#include "vs_crtc.h"
> > > > > +#include "vs_dc.h"
> > > > > +#include "vs_dc_top_regs.h"
> > > > > +#include "vs_plane.h"
> > > > > +
> > > > > +static void vs_crtc_atomic_flush(struct drm_crtc *crtc,
> > > > > + struct drm_atomic_state
> > > > > *state)
> > > > > +{
> > > > > + struct vs_crtc *vcrtc = drm_crtc_to_vs_crtc(crtc);
> > > > > + struct drm_crtc_state *crtc_state =
> > > > > drm_atomic_get_new_crtc_state(state,
> > > > > +
> > > > >
> > > > >
> > > > > crtc);
> > > > > + struct drm_pending_vblank_event *event = crtc_state-
> > > > > > event;
> > > > > +
> > > > > + DRM_DEBUG_DRIVER("Flushing CRTC %u vblank events\n",
> > > > > vcrtc-
> > > > > > id);
> > > > > +
> > > > > + if (event) {
> > > > > + crtc_state->event = NULL;
> > > > > +
> > > > > + spin_lock_irq(&crtc->dev->event_lock);
> > > > > + if (drm_crtc_vblank_get(crtc) == 0)
> > > > > + drm_crtc_arm_vblank_event(crtc,
> > > > > event);
> > > > > + else
> > > > > + drm_crtc_send_vblank_event(crtc,
> > > > > event);
> > > > > + spin_unlock_irq(&crtc->dev->event_lock);
> > > > > + }
> > > > > +}
> > > > > +
> > > > > +static void vs_crtc_atomic_disable(struct drm_crtc *crtc,
> > > > > + struct drm_atomic_state
> > > > > *state)
> > > > > +{
> > > > > + struct vs_crtc *vcrtc = drm_crtc_to_vs_crtc(crtc);
> > > > > + struct vs_dc *dc = vcrtc->dc;
> > > > > + unsigned int output = vcrtc->id;
> > > > > +
> > > > > + DRM_DEBUG_DRIVER("Disabling CRTC %u\n", output);
> > > > > +
> > > > > + drm_crtc_vblank_off(crtc);
> > > > > +
> > > > > + clk_disable_unprepare(dc->pix_clk[output]);
> > > > > +}
> > > > > +
> > > > > +static void vs_crtc_atomic_enable(struct drm_crtc *crtc,
> > > > > + struct drm_atomic_state
> > > > > *state)
> > > > > +{
> > > > > + struct vs_crtc *vcrtc = drm_crtc_to_vs_crtc(crtc);
> > > > > + struct vs_dc *dc = vcrtc->dc;
> > > > > + unsigned int output = vcrtc->id;
> > > > > +
> > > > > + DRM_DEBUG_DRIVER("Enabling CRTC %u\n", output);
> > > > > +
> > > > > + WARN_ON(clk_prepare_enable(dc->pix_clk[output]));
> > > > > +
> > > > > + drm_crtc_vblank_on(crtc);
> > > > > +}
> > > > > +
> > > > > +static void vs_crtc_mode_set_nofb(struct drm_crtc *crtc)
> > > > > +{
> > > > > + struct drm_display_mode *mode = &crtc->state-
> > > > > > adjusted_mode;
> > > > > + struct vs_crtc *vcrtc = drm_crtc_to_vs_crtc(crtc);
> > > > > + struct vs_dc *dc = vcrtc->dc;
> > > > > + unsigned int output = vcrtc->id;
> > > > > +
> > > > > + DRM_DEBUG_DRIVER("Setting mode on CRTC %u\n",
> > > > > output);
> > > > > +
> > > > > + regmap_write(dc->regs, VSDC_DISP_HSIZE(output),
> > > > > + VSDC_DISP_HSIZE_DISP(mode->hdisplay) |
> > > > > + VSDC_DISP_HSIZE_TOTAL(mode->htotal));
> > > > > + regmap_write(dc->regs, VSDC_DISP_VSIZE(output),
> > > > > + VSDC_DISP_VSIZE_DISP(mode->vdisplay) |
> > > > > + VSDC_DISP_VSIZE_TOTAL(mode->vtotal));
> > > > > + regmap_write(dc->regs, VSDC_DISP_HSYNC(output),
> > > > > + VSDC_DISP_HSYNC_START(mode->hsync_start)
> > > > > |
> > > > > + VSDC_DISP_HSYNC_END(mode->hsync_end) |
> > > > > + VSDC_DISP_HSYNC_EN);
> > > > > + if (!(mode->flags & DRM_MODE_FLAG_PHSYNC))
> > > > > + regmap_set_bits(dc->regs,
> > > > > VSDC_DISP_HSYNC(output),
> > > > > + VSDC_DISP_HSYNC_POL);
> > > > > + regmap_write(dc->regs, VSDC_DISP_VSYNC(output),
> > > > > + VSDC_DISP_VSYNC_START(mode->vsync_start)
> > > > > |
> > > > > + VSDC_DISP_VSYNC_END(mode->vsync_end) |
> > > > > + VSDC_DISP_VSYNC_EN);
> > > > > + if (!(mode->flags & DRM_MODE_FLAG_PVSYNC))
> > > > > + regmap_set_bits(dc->regs,
> > > > > VSDC_DISP_VSYNC(output),
> > > > > + VSDC_DISP_VSYNC_POL);
> > > > > +
> > > > > + WARN_ON(clk_set_rate(dc->pix_clk[output], mode-
> > > > > > crtc_clock
> > > > > * 1000));
> > > > > +}
> > > > > +
> > > > > +static enum drm_mode_status
> > > > > +vs_crtc_mode_valid(struct drm_crtc *crtc, const struct
> > > > > drm_display_mode *mode)
> > > > > +{
> > > > > + struct vs_crtc *vcrtc = drm_crtc_to_vs_crtc(crtc);
> > > > > + struct vs_dc *dc = vcrtc->dc;
> > > > > + unsigned int output = vcrtc->id;
> > > > > + long rate;
> > > > > +
> > > > > + if (mode->htotal > 0x7FFF)
> > > >
> > > > lowercase hex, please.
> > >
> > > Why? I didn't see any document enforces this.
> >
> > I think, it's a generic suggestion for the sake of readability.
> >
> > >
> > > >
> > > > > + return MODE_BAD_HVALUE;
> > > > > + if (mode->vtotal > 0x7FFF)
> > > > > + return MODE_BAD_VVALUE;
> > > > > +
> > > > > + rate = clk_round_rate(dc->pix_clk[output], mode-
> > > > > >clock
> > > > > *
> > > > > 1000);
> > > > > + if (rate <= 0)
> > > > > + return MODE_CLOCK_RANGE;
> > > > > +
> > > > > + return MODE_OK;
> > > > > +}
> > > > > +
> > > > > +static bool vs_crtc_mode_fixup(struct drm_crtc *crtc,
> > > > > + const struct drm_display_mode
> > > > > *m,
> > > > > + struct drm_display_mode
> > > > > *adjusted_mode)
> > > > > +{
> > > > > + struct vs_crtc *vcrtc = drm_crtc_to_vs_crtc(crtc);
> > > > > + struct vs_dc *dc = vcrtc->dc;
> > > > > + unsigned int output = vcrtc->id;
> > > > > + long clk_rate;
> > > > > +
> > > > > + drm_mode_set_crtcinfo(adjusted_mode, 0);
> > > > > +
> > > > > + /* Feedback the pixel clock to crtc_clock */
> > > > > + clk_rate = adjusted_mode->crtc_clock * 1000;
> > > > > + clk_rate = clk_round_rate(dc->pix_clk[output],
> > > > > clk_rate);
> > > > > + if (clk_rate <= 0)
> > > > > + return false;
> > > > > +
> > > > > + adjusted_mode->crtc_clock = clk_rate / 1000;
> > > > > +
> > > > > + return true;
> > > > > +}
> > > > > +
> > > > > +static const struct drm_crtc_helper_funcs
> > > > > vs_crtc_helper_funcs
> > > > > = {
> > > > > + .atomic_flush = vs_crtc_atomic_flush,
> > > > > + .atomic_enable = vs_crtc_atomic_enable,
> > > > > + .atomic_disable = vs_crtc_atomic_disable,
> > > > > + .mode_set_nofb = vs_crtc_mode_set_nofb,
> > > > > + .mode_valid = vs_crtc_mode_valid,
> > > > > + .mode_fixup = vs_crtc_mode_fixup,
> > > > > +};
> > > > > +
> > > > > +static int vs_crtc_enable_vblank(struct drm_crtc *crtc)
> > > > > +{
> > > > > + struct vs_crtc *vcrtc = drm_crtc_to_vs_crtc(crtc);
> > > > > + struct vs_dc *dc = vcrtc->dc;
> > > > > +
> > > > > + DRM_DEBUG_DRIVER("Enabling VBLANK on CRTC %u\n",
> > > > > vcrtc-
> > > > > > id);
> > > > > + regmap_set_bits(dc->regs, VSDC_TOP_IRQ_EN,
> > > > > VSDC_TOP_IRQ_VSYNC(vcrtc->id));
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +static void vs_crtc_disable_vblank(struct drm_crtc *crtc)
> > > > > +{
> > > > > + struct vs_crtc *vcrtc = drm_crtc_to_vs_crtc(crtc);
> > > > > + struct vs_dc *dc = vcrtc->dc;
> > > > > +
> > > > > + DRM_DEBUG_DRIVER("Disabling VBLANK on CRTC %u\n",
> > > > > vcrtc-
> > > > > > id);
> > > > > + regmap_clear_bits(dc->regs, VSDC_TOP_IRQ_EN,
> > > > > VSDC_TOP_IRQ_VSYNC(vcrtc->id));
> > > > > +}
> > > > > +
> > > > > +static const struct drm_crtc_funcs vs_crtc_funcs = {
> > > > > + .atomic_destroy_state =
> > > > > drm_atomic_helper_crtc_destroy_state,
> > > > > + .atomic_duplicate_state =
> > > > > drm_atomic_helper_crtc_duplicate_state,
> > > > > + .destroy = drm_crtc_cleanup,
> > > > > + .page_flip =
> > > > > drm_atomic_helper_page_flip,
> > > > > + .reset =
> > > > > drm_atomic_helper_crtc_reset,
> > > > > + .set_config =
> > > > > drm_atomic_helper_set_config,
> > > > > + .enable_vblank = vs_crtc_enable_vblank,
> > > > > + .disable_vblank = vs_crtc_disable_vblank,
> > > > > +};
> > > > > +
> > > > > +struct vs_crtc *vs_crtc_init(struct drm_device *drm_dev,
> > > > > struct
> > > > > vs_dc *dc,
> > > > > + unsigned int output)
> > > > > +{
> > > > > + struct vs_crtc *vcrtc;
> > > > > + struct drm_plane *primary;
> > > > > + int ret;
> > > > > +
> > > > > + vcrtc = devm_kzalloc(drm_dev->dev, sizeof(*vcrtc),
> > > > > GFP_KERNEL);
> > > > > + if (!vcrtc)
> > > > > + return ERR_PTR(-ENOMEM);
> > > > > + vcrtc->dc = dc;
> > > > > + vcrtc->id = output;
> > > > > +
> > > > > + /* Create our primary plane */
> > > > > + primary = vs_primary_plane_init(drm_dev, dc);
> > > > > + if (IS_ERR(primary)) {
> > > > > + dev_err(drm_dev->dev, "Couldn't create the
> > > > > primary
> > > > > plane\n");
> > > > > + return ERR_PTR(PTR_ERR(primary));
> > > > > + }
> > > > > +
> > > > > + ret = drm_crtc_init_with_planes(drm_dev, &vcrtc-
> > > > > >base,
> > > > > + primary,
> > > > > + NULL,
> > > > > + &vs_crtc_funcs,
> > > > > + NULL);
> > > > > + if (ret) {
> > > > > + dev_err(drm_dev->dev, "Couldn't initialize
> > > > > CRTC\n");
> > > > > + return ERR_PTR(ret);
> > > > > + }
> > > > > +
> > > > > + drm_crtc_helper_add(&vcrtc->base,
> > > > > &vs_crtc_helper_funcs);
> > > > > +
> > > > > + return vcrtc;
> > > > > +}
> > > > > diff --git a/drivers/gpu/drm/verisilicon/vs_crtc.h
> > > > > b/drivers/gpu/drm/verisilicon/vs_crtc.h
> > > > > new file mode 100644
> > > > > index 0000000000000..6f862d609b984
> > > > > --- /dev/null
> > > > > +++ b/drivers/gpu/drm/verisilicon/vs_crtc.h
> > > > > @@ -0,0 +1,29 @@
> > > > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > > > +/*
> > > > > + * Copyright (C) 2025 Icenowy Zheng <u...@icenowy.me>
> > > > > + */
> > > > > +
> > > > > +#ifndef _VS_CRTC_H_
> > > > > +#define _VS_CRTC_H_
> > > > > +
> > > > > +#include <drm/drm_crtc.h>
> > > > > +#include <drm/drm_vblank.h>
> > > > > +
> > > > > +struct vs_dc;
> > > > > +
> > > > > +struct vs_crtc {
> > > > > + struct drm_crtc base;
> > > > > +
> > > > > + struct vs_dc *dc;
> > > > > + unsigned int id;
> > > > > +};
> > > > > +
> > > > > +static inline struct vs_crtc *drm_crtc_to_vs_crtc(struct
> > > > > drm_crtc
> > > > > *crtc)
> > > > > +{
> > > > > + return container_of(crtc, struct vs_crtc, base);
> > > > > +}
> > > > > +
> > > > > +struct vs_crtc *vs_crtc_init(struct drm_device *drm_dev,
> > > > > struct
> > > > > vs_dc *dc,
> > > > > + unsigned int output);
> > > > > +
> > > > > +#endif /* _VS_CRTC_H_ */
> > > > > diff --git a/drivers/gpu/drm/verisilicon/vs_crtc_regs.h
> > > > > b/drivers/gpu/drm/verisilicon/vs_crtc_regs.h
> > > > > new file mode 100644
> > > > > index 0000000000000..c7930e817635c
> > > > > --- /dev/null
> > > > > +++ b/drivers/gpu/drm/verisilicon/vs_crtc_regs.h
> > > > > @@ -0,0 +1,60 @@
> > > > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > > > +/*
> > > > > + * Copyright (C) 2025 Icenowy Zheng <u...@icenowy.me>
> > > > > + *
> > > > > + * Based on vs_dc_hw.h, which is:
> > > > > + * Copyright (C) 2023 VeriSilicon Holdings Co., Ltd.
> > > > > + */
> > > > > +
> > > > > +#ifndef _VS_CRTC_REGS_H_
> > > > > +#define _VS_CRTC_REGS_H_
> > > > > +
> > > > > +#include <linux/bits.h>
> > > > > +
> > > > > +#define VSDC_DISP_DITHER_CONFIG(n) (0x1410 + 0x4
> > > > > *
> > > > > (n))
> > > > > +
> > > > > +#define VSDC_DISP_DITHER_TABLE_LOW(n) (0x1420 + 0x4
> > > > > *
> > > > > (n))
> > > > > +#define VSDC_DISP_DITHER_TABLE_LOW_DEFAULT 0x7B48F3C0
> > > > > +
> > > > > +#define VSDC_DISP_DITHER_TABLE_HIGH(n) (0x1428 + 0x4
> > > > > *
> > > > > (n))
> > > > > +#define VSDC_DISP_DITHER_TABLE_HIGH_DEFAULT 0x596AD1E2
> > > > > +
> > > > > +#define VSDC_DISP_HSIZE(n) (0x1430 + 0x4
> > > > > *
> > > > > (n))
> > > > > +#define VSDC_DISP_HSIZE_DISP_MASK GENMASK(14,
> > > > > 0)
> > > > > +#define VSDC_DISP_HSIZE_DISP(v) ((v)
> > > > > <<
> > > > > 0)
> > > > > +#define VSDC_DISP_HSIZE_TOTAL_MASK GENMASK(30,
> > > > > 16)
> > > > > +#define VSDC_DISP_HSIZE_TOTAL(v) ((v) << 16)
> > > > > +
> > > > > +#define VSDC_DISP_HSYNC(n) (0x1438 + 0x4
> > > > > *
> > > > > (n))
> > > > > +#define VSDC_DISP_HSYNC_START_MASK GENMASK(14,
> > > > > 0)
> > > > > +#define VSDC_DISP_HSYNC_START(v) ((v) << 0)
> > > > > +#define VSDC_DISP_HSYNC_END_MASK GENMASK(29,
> > > > > 15)
> > > > > +#define VSDC_DISP_HSYNC_END(v) ((v) << 15)
> > > > > +#define VSDC_DISP_HSYNC_EN BIT(30)
> > > > > +#define VSDC_DISP_HSYNC_POL BIT(31)
> > > > > +
> > > > > +#define VSDC_DISP_VSIZE(n) (0x1440 + 0x4
> > > > > *
> > > > > (n))
> > > > > +#define VSDC_DISP_VSIZE_DISP_MASK GENMASK(14,
> > > > > 0)
> > > > > +#define VSDC_DISP_VSIZE_DISP(v) ((v)
> > > > > <<
> > > > > 0)
> > > > > +#define VSDC_DISP_VSIZE_TOTAL_MASK GENMASK(30,
> > > > > 16)
> > > > > +#define VSDC_DISP_VSIZE_TOTAL(v) ((v) << 16)
> > > > > +
> > > > > +#define VSDC_DISP_VSYNC(n) (0x1448 + 0x4
> > > > > *
> > > > > (n))
> > > > > +#define VSDC_DISP_VSYNC_START_MASK GENMASK(14,
> > > > > 0)
> > > > > +#define VSDC_DISP_VSYNC_START(v) ((v) << 0)
> > > > > +#define VSDC_DISP_VSYNC_END_MASK GENMASK(29,
> > > > > 15)
> > > > > +#define VSDC_DISP_VSYNC_END(v) ((v) << 15)
> > > > > +#define VSDC_DISP_VSYNC_EN BIT(30)
> > > > > +#define VSDC_DISP_VSYNC_POL BIT(31)
> > > > > +
> > > > > +#define VSDC_DISP_CURRENT_LOCATION(n) (0x1450 + 0x4
> > > > > *
> > > > > (n))
> > > > > +
> > > > > +#define VSDC_DISP_GAMMA_INDEX(n) (0x1458 + 0x4
> > > > > *
> > > > > (n))
> > > > > +
> > > > > +#define
> > > > > VSDC_DISP_GAMMA_DATA(n) (0x1460
> > > > > +
> > > > > 0x4 * (n))
> > > > > +
> > > > > +#define VSDC_DISP_IRQ_STA 0x147C
> > > > > +
> > > > > +#define VSDC_DISP_IRQ_EN 0x1480
> > > > > +
> > > > > +#endif /* _VS_CRTC_REGS_H_ */
> > > > > diff --git a/drivers/gpu/drm/verisilicon/vs_dc.c
> > > > > b/drivers/gpu/drm/verisilicon/vs_dc.c
> > > > > new file mode 100644
> > > > > index 0000000000000..98384559568c4
> > > > > --- /dev/null
> > > > > +++ b/drivers/gpu/drm/verisilicon/vs_dc.c
> > > > > @@ -0,0 +1,233 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > > +/*
> > > > > + * Copyright (C) 2025 Icenowy Zheng <u...@icenowy.me>
> > > > > + */
> > > > > +
> > > > > +#include <linux/dma-mapping.h>
> > > > > +#include <linux/of.h>
> > > > > +#include <linux/of_graph.h>
> > > > > +
> > > > > +#include "vs_crtc.h"
> > > > > +#include "vs_dc.h"
> > > > > +#include "vs_dc_top_regs.h"
> > > > > +#include "vs_drm.h"
> > > > > +#include "vs_hwdb.h"
> > > > > +
> > > > > +static const struct regmap_config vs_dc_regmap_cfg = {
> > > > > + .reg_bits = 32,
> > > > > + .val_bits = 32,
> > > > > + .reg_stride = sizeof(u32),
> > > > > + /* VSDC_OVL_CONFIG_EX(1) */
> > > > > + .max_register = 0x2544,
> > > > > + .cache_type = REGCACHE_NONE,
> > > > > +};
> > > > > +
> > > > > +static const struct of_device_id vs_dc_driver_dt_match[] = {
> > > > > + { .compatible = "verisilicon,dc" },
> > > > > + {},
> > > > > +};
> > > > > +MODULE_DEVICE_TABLE(of, vs_dc_driver_dt_match);
> > > > > +
> > > > > +static irqreturn_t vs_dc_irq_handler(int irq, void *private)
> > > > > +{
> > > > > + struct vs_dc *dc = private;
> > > > > + u32 irqs;
> > > > > +
> > > > > + regmap_read(dc->regs, VSDC_TOP_IRQ_ACK, &irqs);
> > > > > +
> > > > > + return vs_drm_handle_irq(dc, irqs);
> > > > > +}
> > > > > +
> > > > > +static int vs_dc_probe(struct platform_device *pdev)
> > > > > +{
> > > > > + struct device *dev = &pdev->dev;
> > > > > + struct vs_dc *dc;
> > > > > + void __iomem *regs;
> > > > > + unsigned int outputs, i;
> > > > > + /* pix0/pix1 */
> > > > > + char pixclk_name[5];
> > > > > + int irq, ret;
> > > > > +
> > > > > + if (!dev->of_node) {
> > > > > + dev_err(dev, "can't find DC devices\n");
> > > > > + return -ENODEV;
> > > > > + }
> > > > > +
> > > > > + outputs = of_graph_get_port_count(dev->of_node);
> > > > > + if (!outputs) {
> > > > > + dev_err(dev, "can't find DC downstream
> > > > > ports\n");
> > > > > + return -ENODEV;
> > > > > + }
> > > > > + if (outputs > VSDC_MAX_OUTPUTS) {
> > > > > + dev_err(dev, "too many DC downstream ports
> > > > > than
> > > > > possible\n");
> > > > > + return -EINVAL;
> > > > > + }
> > > > > +
> > > > > + ret = dma_set_mask_and_coherent(&pdev->dev,
> > > > > DMA_BIT_MASK(32));
> > > > > + if (ret) {
> > > > > + dev_err(dev, "No suitable DMA available\n");
> > > > > + return ret;
> > > > > + }
> > > > > +
> > > > > + dc = devm_kzalloc(dev, sizeof(*dc), GFP_KERNEL);
> > > > > + if (!dc)
> > > > > + return -ENOMEM;
> > > > > +
> > > > > + dc->outputs = outputs;
> > > > > +
> > > > > + dc->rsts[0].id = "core";
> > > > > + dc->rsts[1].id = "axi";
> > > > > + dc->rsts[0].id = "ahb";
> > > > > +
> > > > > + ret =
> > > > > devm_reset_control_bulk_get_optional_shared(dev,
> > > > > VSDC_RESET_COUNT,
> > > > > + dc-
> > > > > > rsts);
> > > > > + if (ret) {
> > > > > + dev_err(dev, "can't get reset lines\n");
> > > > > + return ret;
> > > > > + }
> > > > > +
> > > > > + dc->core_clk = devm_clk_get(dev, "core");
> > > > > + if (IS_ERR(dc->core_clk)) {
> > > > > + dev_err(dev, "can't get core clock\n");
> > > > > + return PTR_ERR(dc->core_clk);
> > > > > + }
> > > > > +
> > > > > + dc->axi_clk = devm_clk_get(dev, "axi");
> > > > > + if (IS_ERR(dc->axi_clk)) {
> > > > > + dev_err(dev, "can't get axi clock\n");
> > > > > + return PTR_ERR(dc->axi_clk);
> > > > > + }
> > > > > +
> > > > > + dc->ahb_clk = devm_clk_get(dev, "ahb");
> > > >
> > > > devm_clk_get_enabled() ?
> > > >
> > > > > + if (IS_ERR(dc->ahb_clk)) {
> > > > > + dev_err(dev, "can't get ahb clock\n");
> > > > > + return PTR_ERR(dc->ahb_clk);
> > > > > + }
> > > > > +
> > > > > + for (i = 0; i < outputs; i++) {
> > > > > + snprintf(pixclk_name, sizeof(pixclk_name),
> > > > > "pix%u",
> > > > > i);
> > > > > + dc->pix_clk[i] = devm_clk_get(dev,
> > > > > pixclk_name);
> > > > > + if (IS_ERR(dc->pix_clk[i])) {
> > > > > + dev_err(dev, "can't get pixel clk
> > > > > %u\n",
> > > > > i);
> > > > > + return PTR_ERR(dc->pix_clk[i]);
> > > > > + }
> > > > > + }
> > > > > +
> > > > > + irq = platform_get_irq(pdev, 0);
> > > > > + if (irq < 0) {
> > > > > + dev_err(dev, "can't get irq\n");
> > > > > + return irq;
> > > > > + }
> > > > > +
> > > > > + ret = reset_control_bulk_deassert(VSDC_RESET_COUNT,
> > > > > dc-
> > > > > > rsts);
> > > > > + if (ret) {
> > > > > + dev_err(dev, "can't deassert reset lines\n");
> > > > > + return ret;
> > > > > + }
> > > > > +
> > > > > + ret = clk_prepare_enable(dc->core_clk);
> > > > > + if (ret) {
> > > > > + dev_err(dev, "can't enable core clock\n");
> > > > > + goto err_rst_assert;
> > > > > + }
> > > > > +
> > > > > + ret = clk_prepare_enable(dc->axi_clk);
> > > > > + if (ret) {
> > > > > + dev_err(dev, "can't enable axi clock\n");
> > > > > + goto err_core_clk_disable;
> > > > > + }
> > > > > +
> > > > > + ret = clk_prepare_enable(dc->ahb_clk);
> > > > > + if (ret) {
> > > > > + dev_err(dev, "can't enable ahb clock\n");
> > > > > + goto err_axi_clk_disable;
> > > > > + }
> > > > > +
> > > > > + regs = devm_platform_ioremap_resource(pdev, 0);
> > > > > + if (IS_ERR(regs)) {
> > > > > + dev_err(dev, "can't map registers");
> > > > > + ret = PTR_ERR(regs);
> > > > > + goto err_ahb_clk_disable;
> > > > > + }
> > > > > +
> > > > > + dc->regs = devm_regmap_init_mmio(dev, regs,
> > > > > &vs_dc_regmap_cfg);
> > > > > + if (IS_ERR(dc->regs)) {
> > > > > + ret = PTR_ERR(dc->regs);
> > > > > + goto err_ahb_clk_disable;
> > > > > + }
> > > > > +
> > > > > + ret = vs_fill_chip_identity(dc->regs, &dc->identity);
> > > >
> > > > I'd say, this should be a part of the DT bindings.
> > > >
> > > > > + if (ret)
> > > > > + goto err_ahb_clk_disable;
> > > > > +
> > > > > + dev_info(dev, "DC%x rev %x customer %x\n", dc-
> > > > > > identity.model,
> > > > > + dc->identity.revision, dc-
> > > > > > identity.customer_id);
> > > > > +
> > > > > + if (outputs > dc->identity.display_count) {
> > > > > + dev_err(dev, "too many downstream ports than
> > > > > HW
> > > > > capability\n");
> > > > > + ret = -EINVAL;
> > > > > + goto err_ahb_clk_disable;
> > > > > + }
> > > > > +
> > > > > + ret = devm_request_irq(dev, irq, vs_dc_irq_handler,
> > > > > 0,
> > > > > + dev_name(dev), dc);
> > > >
> > > > Are we ready to handle the IRQ here?
> > > >
> > > > > + if (ret) {
> > > > > + dev_err(dev, "can't request irq\n");
> > > > > + goto err_ahb_clk_disable;
> > > > > + }
> > > > > +
> > > > > + dev_set_drvdata(dev, dc);
> > > > > +
> > > > > + ret = vs_drm_initialize(dc, pdev);
> > > > > + if (ret)
> > > > > + goto err_ahb_clk_disable;
> > > > > +
> > > > > + return 0;
> > > > > +
> > > > > +err_ahb_clk_disable:
> > > > > + clk_disable_unprepare(dc->ahb_clk);
> > > > > +err_axi_clk_disable:
> > > > > + clk_disable_unprepare(dc->axi_clk);
> > > > > +err_core_clk_disable:
> > > > > + clk_disable_unprepare(dc->core_clk);
> > > > > +err_rst_assert:
> > > > > + reset_control_bulk_assert(VSDC_RESET_COUNT, dc-
> > > > > >rsts);
> > > > > + return ret;
> > > > > +}
> > > > > +
> > > > > +static void vs_dc_remove(struct platform_device *pdev)
> > > > > +{
> > > > > + struct vs_dc *dc = dev_get_drvdata(&pdev->dev);
> > > > > +
> > > > > + vs_drm_finalize(dc);
> > > > > +
> > > > > + dev_set_drvdata(&pdev->dev, NULL);
> > > > > +
> > > > > + clk_disable_unprepare(dc->ahb_clk);
> > > > > + clk_disable_unprepare(dc->axi_clk);
> > > > > + clk_disable_unprepare(dc->core_clk);
> > > > > + reset_control_bulk_assert(VSDC_RESET_COUNT, dc-
> > > > > >rsts);
> > > > > +}
> > > > > +
> > > > > +static void vs_dc_shutdown(struct platform_device *pdev)
> > > > > +{
> > > > > + struct vs_dc *dc = dev_get_drvdata(&pdev->dev);
> > > > > +
> > > > > + vs_drm_shutdown_handler(dc);
> > > >
> > > > I'd suggest inlining simple wrappers.
> > >
> > > Well I am going to divider the code to non-DRM things and DRM
> > > things
> > > here, so vs_drm_shutdown_handler is in the DRM things part
> > > instead.
> >
> > Ack. It might be my personal preference not to have extra wrappers.
> >
>
