| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <0a1a8f92-860f-4f4e-aad0-f743f099efc1@gmail.com>
Date: Fri, 6 Feb 2026 15:23:59 +0800
From: Joey Lu <a0987203069@...il.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: airlied@...il.com, simona@...ll.ch, maarten.lankhorst@...ux.intel.com,
mripard@...nel.org, tzimmermann@...e.de, robh@...nel.org,
krzk+dt@...nel.org, conor+dt@...nel.org, ychuang3@...oton.com,
schung@...oton.com, yclu4@...oton.com, linux-arm-kernel@...ts.infradead.org,
dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/3] drm/nuvoton: add MA35D1 display controller driver
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@...il.com>
>> ---
>> 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
Powered by blists - more mailing lists