Re: [PATCH 2/2] dmaengine: ma35d1-pdma: Add MA35D1 PDMA driver

[Shan-Chun Hung <shanchun1218@gmail.com>]

Hi Krzysztof,

Sorry, I missed this comment.


On 11/5/2026 12:11 PM, Shan-Chun Hung wrote:
> Hi Krzysztof,
> 
> Thanks for your review.
> 
> On 11/4/2026 10:06 PM, Krzysztof Kozlowski wrote:
>> On Tue, Nov 03, 2026 at 10:22:02AM +0800, Shan-Chun Hung wrote:
>>> +#include <linux/align.h>
>>> +#include <linux/bitfield.h>
>>> +#include <linux/bits.h>
>>> +#include <linux/clk.h>
>>> +#include <linux/compiler_types.h>
>>> +#include <linux/container_of.h>
>>> +#include <linux/dev_printk.h>
>>> +#include <linux/device.h>
>>> +#include <linux/dma-direction.h>
>>> +#include <linux/dma-mapping.h>
>>> +#include <linux/dma/ma35d1-pdma.h>
>>> +#include <linux/dmaengine.h>
>>> +#include <linux/err.h>
>>> +#include <linux/errno.h>
>>> +#include <linux/gfp_types.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/io.h>
>>> +#include <linux/irqreturn.h>
>>> +#include <linux/limits.h>
>>> +#include <linux/list.h>
>>> +#include <linux/math.h>
>>> +#include <linux/minmax.h>
>>> +#include <linux/mod_devicetable.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_dma.h>
>>> +#include <linux/overflow.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/scatterlist.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/spinlock.h>
>>> +#include <linux/spinlock_types.h>
>>> +#include <linux/stddef.h>
>>> +#include <linux/string.h>
>>> +#include <linux/types.h>
>>
>> Do you use all these defines?
I'll double-check the defines.>>
>> I don't see usage of header with bindings, though.
The bindings header is indeed unused and will be removed in the next 
revision.
>>
>>> +
>>> +#include "dmaengine.h"
>>> +#include "virt-dma.h"
>>> +
>>
>> ...
>>
>>> +static int ma35d1_pdma_probe(struct platform_device *pdev)
>>> +{
>>> +    int i, irq, nr_chans, ret;
>>> +    struct clk *pdma_clk;
>>> +    struct device *dev = &pdev->dev;
>>> +    struct device_node *node = dev->of_node;
>>> +    struct ma35d1_chan *ch;
>>> +    struct ma35d1_pdmadev *pdmadev;
>>> +    void __iomem *reg_base;
>>> +
>>> +    pdma_clk = devm_clk_get_enabled(dev, NULL);
>>> +    if (IS_ERR(pdma_clk))
>>> +        return dev_err_probe(dev, PTR_ERR(pdma_clk), "Failed to get 
>>> clk\n");
>>> +
>>> +    ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    irq = platform_get_irq(pdev, 0);
>>> +    if (irq < 0)
>>> +        return irq;
>>> +
>>> +    ret = of_property_read_u32(node, "dma-channels", &nr_chans);
>>> +    if (ret)
>>> +        return dev_err_probe(dev, ret, "Failed to get dma-channels\n");
>>> +
>>> +    if (nr_chans > PDMA_MAX_CHANS) {
>>> +        dev_warn(dev, "dma-channels %d exceeds max %d, clamping\n",
>>> +             nr_chans, PDMA_MAX_CHANS);
>>> +        nr_chans = PDMA_MAX_CHANS;
>>> +    }
>>> +
>>> +    pdmadev = devm_kzalloc(dev, struct_size(pdmadev, channels, 
>>> nr_chans), GFP_KERNEL);
>>> +    if (!pdmadev)
>>> +        return -ENOMEM;
>>> +
>>> +    pdmadev->nr_chans = nr_chans;
>>> +
>>> +    reg_base = devm_platform_ioremap_resource(pdev, 0);
>>> +    if (IS_ERR(reg_base))
>>> +        return dev_err_probe(dev, PTR_ERR(reg_base),
>>> +                     "Failed to map pdma global registers\n");
>>> +
>>> +    dma_cap_zero(pdmadev->ddev.cap_mask);
>>> +    dma_cap_set(DMA_SLAVE, pdmadev->ddev.cap_mask);
>>> +    dma_cap_set(DMA_PRIVATE, pdmadev->ddev.cap_mask);
>>> +    dma_cap_set(DMA_MEMCPY, pdmadev->ddev.cap_mask);
>>> +    dma_cap_set(DMA_CYCLIC, pdmadev->ddev.cap_mask);
>>> +
>>> +    pdmadev->ddev.dev = dev;
>>> +    pdmadev->ddev.src_addr_widths = PDMA_SLAVE_BUSWIDTHS;
>>> +    pdmadev->ddev.dst_addr_widths = PDMA_SLAVE_BUSWIDTHS;
>>> +    pdmadev->ddev.directions = BIT(DMA_MEM_TO_DEV) | 
>>> BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_MEM);
>>> +    pdmadev->ddev.device_prep_dma_memcpy = ma35d1_prep_dma_memcpy;
>>> +    pdmadev->ddev.device_prep_slave_sg = ma35d1_prep_slave_sg;
>>> +    pdmadev->ddev.device_prep_dma_cyclic = ma35d1_prep_dma_cyclic;
>>> +    pdmadev->ddev.device_free_chan_resources = 
>>> ma35d1_free_chan_resources;
>>> +    pdmadev->ddev.device_issue_pending = ma35d1_issue_pending;
>>> +    pdmadev->ddev.device_tx_status = ma35d1_tx_status;
>>> +    pdmadev->ddev.device_config = ma35d1_slave_config;
>>> +    pdmadev->ddev.device_terminate_all = ma35d1_terminate_all;
>>> +    pdmadev->ddev.device_synchronize = ma35d1_synchronize;
>>> +
>>> +    spin_lock_init(&pdmadev->lock);
>>> +    INIT_LIST_HEAD(&pdmadev->ddev.channels);
>>> +    for (i = 0; i < nr_chans; i++) {
>>> +        ch = &pdmadev->channels[i];
>>> +        ch->dev = dev;
>>> +        ch->pdmadev = pdmadev;
>>> +        ch->id = i;
>>> +        ch->base = reg_base;
>>> +        ch->vc.desc_free = ma35d1_pdma_desc_free;
>>> +        vchan_init(&ch->vc, &pdmadev->ddev);
>>> +    }
>>> +
>>> +    ret = devm_request_irq(dev, irq, ma35d1_pdma_interrupt, 0, pdev- 
>>> >name, pdmadev);
>>> +    if (ret)
>>> +        return dev_err_probe(dev, ret, "Failed to request IRQ\n");
>>> +
>>> +    ret = dmaenginem_async_device_register(&pdmadev->ddev);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    ret = of_dma_controller_register(node, ma35d1_of_xlate, pdmadev);
>>> +    if (ret)
>>> +        return dev_err_probe(dev, ret, "of_dma_controller_register 
>>> failed\n");
>>> +
>>> +    platform_set_drvdata(pdev, pdmadev);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static void ma35d1_pdma_remove(struct platform_device *pdev)
>>> +{
>>> +    struct ma35d1_pdmadev *pdmadev = platform_get_drvdata(pdev);
>>> +    int i;
>>> +
>>> +    of_dma_controller_free(pdev->dev.of_node);
>>> +    writel(0, pdmadev->channels[0].base + PDMA_INTEN);
>>> +
>>> +    for (i = 0; i < pdmadev->nr_chans; i++)
>>> +        tasklet_kill(&pdmadev->channels[i].vc.task);
>>> +}
>>> +
>>> +static const struct of_device_id ma35d1_pdma_match[] = {
>>> +    { .compatible = "nuvoton,ma35d1-pdma" },
>>> +    { /* sentinel */ }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, ma35d1_pdma_match);
>>> +
>>> +static struct platform_driver ma35d1_pdma_driver = {
>>> +    .probe = ma35d1_pdma_probe,
>>> +    .remove = ma35d1_pdma_remove,
>>> +    .driver = {
>>> +        .name = "ma35d1-pdma",
>>> +        .of_match_table = ma35d1_pdma_match,
>>> +    },
>>> +};
>>> +module_platform_driver(ma35d1_pdma_driver);
>>> +
>>> +MODULE_DESCRIPTION("PDMA controller driver for Nuvoton MA35D1");
>>> +MODULE_AUTHOR("Shan-Chun Hung <shanchun1218@gmail.com>");
>>> +MODULE_LICENSE("GPL");
>>> diff --git a/include/linux/dma/ma35d1-pdma.h b/include/linux/dma/ 
>>> ma35d1-pdma.h
>>> new file mode 100644
>>> index 000000000000..13f1410d74da
>>> --- /dev/null
>>> +++ b/include/linux/dma/ma35d1-pdma.h
>>> @@ -0,0 +1,24 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>> +/*
>>> + * Header file for the MA35D1 PDMA Controller driver
>>> + *
>>> + * Copyright (C) 2026 Nuvoton Technology Corp.
>>> + */
>>> +#ifndef _MA35D1_PDMA_H
>>> +#define _MA35D1_PDMA_H
>>> +
>>> +/**
>>> + * struct ma35d1_peripheral - PDMA peripheral configuration
>>> + * @prescaler: timeout prescaler value
>>> + * @counter: timeout counter value
>>> + *
>>> + * Slave device drivers fill in this structure and pass it via
>>> + * dma_slave_config.peripheral_config/peripheral_size to configure
>>> + * the MA35D1 PDMA engine's per-channel timeout detection block.
>>
>> Hm? Confusing, how is it supposed to be used? Do you just reimplement
>> board files?
> 
> Sorry, the comment did not clearly explain the intended usage.
> 
> For UART RX DMA, the UART driver calculates the PDMA timeout prescaler 
> and counter values based on the current baud rate, then passes them 
> through dma_slave_config.peripheral_config to configure the channel's 
> timeout detection. These are runtime settings.
> 
> However, I understand that passing raw prescaler and counter values 
> couples the UART driver to the MA35D1 PDMA register layout. I will 
> consider having the UART driver specify the required timeout duration 
> and letting the PDMA driver convert it into the hardware settings.
> 
> In the next revision, I will remove this timeout configuration interface 
> and revisit it together with the UART DMA support patches, so the 
> intended usage and client implementation can be reviewed together.
>>
>>> + */
>>> +struct ma35d1_peripheral {
>>> +    unsigned int prescaler;
>>> +    unsigned int counter;
>>> +};
>>> +
>>> +#endif /* _MA35D1_PDMA_H */
>>> -- 
>>> 2.25.1
>>>
> 
> Best Regards,
> Shan-Chun

Best Regards,
Shan-Chun