Re: [PATCH v2 3/3] drm/nuvoton: add MA35D1 display controller driver

[Joey Lu]

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 <a0987203069@xxxxxxxxx>
> > ---
> >   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