On 2/5/2026 9:22 PM, Krzysztof Kozlowski wrote:
On Thu, Jan 29, 2026 at 12:05:32PM +0800, Joey Lu wrote:
Add DRM driver support for the Display Control Unit (DCU)
found in Nuvoton MA35D1 SoCs.
Signed-off-by: Joey Lu <[email protected]>
---
drivers/gpu/drm/Kconfig | 1 +
drivers/gpu/drm/Makefile | 1 +
drivers/gpu/drm/nuvoton/Kconfig | 21 +
drivers/gpu/drm/nuvoton/Makefile | 7 +
drivers/gpu/drm/nuvoton/ma35_crtc.c | 372 ++++++++++++++
drivers/gpu/drm/nuvoton/ma35_crtc.h | 67 +++
drivers/gpu/drm/nuvoton/ma35_drm.c | 371 ++++++++++++++
drivers/gpu/drm/nuvoton/ma35_drm.h | 48 ++
drivers/gpu/drm/nuvoton/ma35_interface.c | 193 ++++++++
drivers/gpu/drm/nuvoton/ma35_interface.h | 30 ++
drivers/gpu/drm/nuvoton/ma35_plane.c | 603 +++++++++++++++++++++++
drivers/gpu/drm/nuvoton/ma35_plane.h | 115 +++++
drivers/gpu/drm/nuvoton/ma35_regs.h | 88 ++++
No maintainers? Why would we want to take unmaintained code?
I'll add an entry in MAINTAINERS file.
+static void ma35_mode_fini(struct ma35_drm *priv)
+{
+ struct drm_device *drm_dev = &priv->drm_dev;
+
+ drm_kms_helper_poll_fini(drm_dev);
+}
+
+static int ma35_clocks_prepare(struct ma35_drm *priv)
+{
+ struct drm_device *drm_dev = &priv->drm_dev;
+ struct device *dev = drm_dev->dev;
+ int ret;
+
+ priv->dcuclk = devm_clk_get(dev, "dcu_gate");
+ if (IS_ERR(priv->dcuclk)) {
+ dev_err(dev, "Failed to get display core clock\n");
Don't spam logs on defers. Syntax is in entire probe path: return
dev_err_probe
+ return PTR_ERR(priv->dcuclk);
+ }
+
+ ret = clk_prepare_enable(priv->dcuclk);
Why this cannot be devm_clk_get_enabled?
I'll fix it.
+ if (ret) {
+ dev_err(dev, "Failed to enable display core clock\n");
+ return ret;
+ }
+
+ priv->dcupclk = devm_clk_get(dev, "dcup_div");
+ if (IS_ERR(priv->dcupclk)) {
+ dev_err(dev, "Failed to get display pixel clock\n");
+ return PTR_ERR(priv->dcupclk);
+ }
+
+ ret = clk_prepare_enable(priv->dcupclk);
+ if (ret) {
+ dev_err(dev, "Failed to enable display pixel clock\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+static int ma35_clocks_unprepare(struct ma35_drm *priv)
+{
+ struct clk **clocks[] = {
+ &priv->dcuclk,
+ &priv->dcupclk,
+ };
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(clocks); i++) {
+ if (!*clocks[i])
+ continue;
+
+ clk_disable_unprepare(*clocks[i]);
+ *clocks[i] = NULL;
Huh, pretty complicated and pointless code. This should be devm and bulk
API...
I'll use memory safe helpers instead.
+ }
+
+ return 0;
+}
+
+static int ma35_drm_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct ma35_drm *priv;
+ struct drm_device *drm_dev;
+ void __iomem *base;
+ struct regmap *regmap = NULL;
+ int irq;
+ int ret;
+
+ ret = of_reserved_mem_device_init(dev);
+ if (ret && ret != -ENODEV) {
+ dev_err(dev, "Failed to get optional reserved memory: %d\n",
ret);
+ return ret;
+ }
+
+ base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(base)) {
+ dev_err(dev, "Failed to map I/O base\n");
Why aren't you using dev_err_probe?
+ ret = PTR_ERR(base);
+ goto error_reserved_mem;
+ }
+ regmap = devm_regmap_init_mmio(dev, base, &ma35_drm_regmap_config);
+ if (IS_ERR(regmap)) {
+ dev_err(dev, "Failed to create regmap for I/O\n");
+ ret = PTR_ERR(regmap);
+ goto error_reserved_mem;
+ }
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0) {
+ ret = -ENODEV;
+ goto error_reserved_mem;
+ }
+
+ priv = devm_drm_dev_alloc(dev, &ma35_drm_driver,
+ struct ma35_drm, drm_dev);
+ if (IS_ERR(priv)) {
+ ret = PTR_ERR(priv);
+ goto error_reserved_mem;
+ }
+
+ platform_set_drvdata(pdev, priv);
+ drm_dev = &priv->drm_dev;
+ priv->regmap = regmap;
+ INIT_LIST_HEAD(&priv->layers_list);
+
+ ret = ma35_clocks_prepare(priv);
+ if (ret) {
+ drm_err(drm_dev, "Failed to prepare clocks\n");
Why do you print error twice? Once in the function, second time here?
+ goto error_reserved_mem;
+ }
+
+ ret = devm_request_irq(dev, irq, ma35_drm_irq_handler, 0,
+ dev_name(dev), priv);
+ if (ret) {
+ drm_err(drm_dev, "Failed to request IRQ\n");
+ goto error_clocks;
+ }
+
+ /* modeset */
+ ret = ma35_mode_init(priv);
+ if (ret) {
+ drm_err(drm_dev, "Failed to initialize KMS\n");
+ goto error_clocks;
+ }
+
+ /* plane */
+ ret = ma35_plane_init(priv);
+ if (ret) {
+ drm_err(drm_dev, "Failed to initialize layers\n");
+ goto error_clocks;
+ }
+
+ /* crtc */
+ ret = ma35_crtc_init(priv);
+ if (ret) {
+ drm_err(drm_dev, "Failed to initialize CRTC\n");
+ goto error_clocks;
+ }
+
+ /* interface */
+ ret = ma35_interface_init(priv);
+ if (ret) {
+ if (ret != -EPROBE_DEFER)
+ drm_err(drm_dev, "Failed to initialize interface\n");
+
+ goto error_clocks;
+ }
+
+ drm_mode_config_reset(drm_dev);
+
+ ret = drm_dev_register(drm_dev, 0);
+ if (ret) {
+ drm_err(drm_dev, "Failed to register DRM device\n");
+ goto error_mode;
+ }
+
+ drm_client_setup(drm_dev, NULL);
Best regards,
Krzysztof
I'll return raw error codes and let probe wrap them with dev_err_probe().
Thanks for the review.
Best regards,
Joey
