[v3,1/5] dmaengine: sun6i-dma: Refactor to support A733 interrupt and register handling

Refactor to support the Allwinner A733 DMA controller. Currently, the
`sun6i-dma` driver has several functions related to interrupt handling
(reading/writing interrupt enable and status registers) and register
dumping that are hardcoded.

To support the A733, which has different register layouts and interrupt
handling logic, these functions are being moved into the
`sun6i_dma_config` structure as function pointers. This allows the
driver to use a polymorphic approach where the specific implementation
is determined by the hardware configuration assigned during device
probing.

Changes:
- Added function pointers to `struct sun6i_dma_config` for:
    - `dump_com_regs`
    - `read_irq_en`
    - `write_irq_en`
    - `read_irq_stat`
    - `write_irq_stat`
- Implemented generic `sun6i_read/write_irq_*` functions for existing
  hardware.
- Added a macro and updated existing `sun6i_dma_config` instances (A31,
  A23, H3, A64, A100, H6, V3S) to use these new function pointers.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Yuanshen Cao <alex.caoys@gmail.com>
---
 drivers/dma/sun6i-dma.c | 50 ++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 45 insertions(+), 5 deletions(-)
  

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Integer division truncates the channel check in the interrupt handler, causing interrupts for channels >= 8 to be completely ignored on SoCs where the number of channels is not a multiple of 8.
- [High] Incorrect calculation of the physical channel pointer in the interrupt handler aliases upper hardware channels to lower software channels, causing Use-After-Free and data corruption.
- [High] A Time-of-Check to Time-of-Use (TOCTOU) race condition in `sun6i_kill_tasklet` allows a concurrent tasklet to re-enable interrupts after they were intentionally disabled, causing an unhandled interrupt storm after the IRQ is freed.
--

commit 4e47cdc0e212cbdd5c17510fa923b572eb14fb54
Author: Yuanshen Cao <alex.caoys@gmail.com>

dmaengine: sun6i-dma: Refactor to support A733 interrupt and register handling

Refactor to support the Allwinner A733 DMA controller by moving interrupt
handling and register dumping functions into the sun6i_dma_config structure.
This allows a polymorphic approach determined by hardware configuration
during device probing.

> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index a9a254dbf8cb9..ef3052c4ab363 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -347,6 +352,26 @@ static void sun6i_set_mode_h6(u32 *p_cfg, s8 src_mode, s8 dst_mode)
[ ... ]
> +static void sun6i_write_irq_en(struct sun6i_dma_dev *sdev, u32 irq_reg, u32 irq_val)
> +{
> +	writel(irq_val, sdev->base + DMA_IRQ_EN(irq_reg));
> +}

[Severity: High]
This is a pre-existing issue, but does disabling interrupts leave a race
condition?

In sun6i_kill_tasklet(), hardware interrupts are disabled via write_irq_en(0)
before tasklet_kill() is called:

sun6i_kill_tasklet()
    for (i = 0; i < ...; i++)
        sdev->cfg->write_irq_en(sdev, i, 0);
    atomic_inc(&sdev->tasklet_shutdown);
    devm_free_irq(sdev->slave.dev, sdev->irq, sdev);
    tasklet_kill(&sdev->task);

If sun6i_dma_tasklet() runs concurrently and calls sun6i_dma_start_desc(),
could it perform a read-modify-write on the IRQ_EN register without a lock?

This could re-enable the interrupt after sun6i_kill_tasklet() explicitly
disabled it. When devm_free_irq() is called, an active DMA channel might
then assert an unhandled IRQ, leading to an interrupt storm.

[ ... ]
> @@ -549,14 +574,14 @@ static irqreturn_t sun6i_dma_interrupt(int irq, void *dev_id)
>  	u32 status;
>  
>  	for (i = 0; i < sdev->num_pchans / DMA_IRQ_CHAN_NR; i++) {

[Severity: High]
This is a pre-existing issue, but does integer division truncate the channel
check here?

If num_pchans is not a multiple of 8 (like 12 on H3), dividing by 8 yields 1.
This would mean the loop only checks channels 0-7 and ignores the second
interrupt register for channels 8-11.

Could this cause unacknowledged level-triggered interrupts from those upper
channels, leading to an interrupt storm?

> -		status = readl(sdev->base + DMA_IRQ_STAT(i));
> +		status = sdev->cfg->read_irq_stat(sdev, i);
>  		if (!status)
>  			continue;
>  
>  		dev_dbg(sdev->slave.dev, "DMA irq status %s: 0x%x\n",
>  			str_high_low(i), status);
>  
> -		writel(status, sdev->base + DMA_IRQ_STAT(i));
> +		sdev->cfg->write_irq_stat(sdev, i, status);
>  
>  		for (j = 0; (j < DMA_IRQ_CHAN_NR) && status; j++) {
>  			pchan = sdev->pchans + j;

[Severity: High]
This isn't a bug introduced by this patch, but does the physical channel
pointer calculation alias upper hardware channels to lower software channels?

The calculation sdev->pchans + j ignores the register index i.

For upper channels (where i = 1, meaning channels 8-15), would the handler
map the active hardware interrupt back to software channels 0-7?

It seems this might cause the driver to process and complete descriptors for
the wrong channels while the hardware is still writing to them, potentially
causing data corruption and use-after-free conditions.
  

On Mon, 22 Jun 2026 01:36:23 +0000
Yuanshen Cao <alex.caoys@gmail.com> wrote:

Hi,

first, many thanks for sending this, also for structuring the changes
nicely, so that they remain reviewable!

> Refactor to support the Allwinner A733 DMA controller. Currently, the
> `sun6i-dma` driver has several functions related to interrupt handling
> (reading/writing interrupt enable and status registers) and register
> dumping that are hardcoded.
> 
> To support the A733, which has different register layouts and interrupt
> handling logic, these functions are being moved into the
> `sun6i_dma_config` structure as function pointers.

So I see that this driver already makes use of per-device function
pointer, though personally I don't like this approach very much, as it
decreases the readability, and suggests significant differences between
the SoC generations that are not really there: each function just reads
or write an MMIO register, it's just the offset that differs.

So I think it's better to express the differences through data
entries in the config struct, for the IRQ enable/stat functions I think
this should be something like this:

struct sun6i_dma_config {
	...
	u32	irq_stride;
	u32	irq_en_offset;
	u32	irq_stat_offset;
	...
};

-	irq_val = readl(sdev->base + DMA_IRQ_EN(irq_reg));
+	irq_val = readl(sdev->base + sdev->cfg->irq_en_offset + irq_reg * sdev->cfg->irq_stride);

the existing configs set .stride to 0x04, and .en_offset to 0x0, the
A733 later uses .stride = 0x40 and .en_offset = 0x134.
Maybe we still move that now longish line into a helper function, but
not a config specific one. 

I think that's more readable, and avoids unnecessary redirections and
potential pipeline stalls.

dump_com_regs is a different story, since the two instances of that
function are significantly different.

What do you think?

> This allows the
> driver to use a polymorphic approach where the specific implementation
> is determined by the hardware configuration assigned during device
> probing.
> 
> Changes:
> - Added function pointers to `struct sun6i_dma_config` for:

By the way: the preferred style to list changes in commit messages in
imperative mood [1], not in past tense. Think about you ask the
code base what to change:

Add function pointers to ...
Implement generic functions ...

Cheers,
Andre

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst#n94

>     - `dump_com_regs`
>     - `read_irq_en`
>     - `write_irq_en`
>     - `read_irq_stat`
>     - `write_irq_stat`
> - Implemented generic `sun6i_read/write_irq_*` functions for existing
>   hardware.
> - Added a macro and updated existing `sun6i_dma_config` instances (A31,
>   A23, H3, A64, A100, H6, V3S) to use these new function pointers.
> 
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> Signed-off-by: Yuanshen Cao <alex.caoys@gmail.com>
> ---
>  drivers/dma/sun6i-dma.c | 50 ++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 45 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index a9a254dbf8cb..ef3052c4ab36 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -138,6 +138,11 @@ struct sun6i_dma_config {
>  	void (*set_burst_length)(u32 *p_cfg, s8 src_burst, s8 dst_burst);
>  	void (*set_drq)(u32 *p_cfg, s8 src_drq, s8 dst_drq);
>  	void (*set_mode)(u32 *p_cfg, s8 src_mode, s8 dst_mode);
> +	void (*dump_com_regs)(struct sun6i_dma_dev *sdev);
> +	u32 (*read_irq_en)(struct sun6i_dma_dev *sdev, u32 irq_reg);
> +	void (*write_irq_en)(struct sun6i_dma_dev *sdev, u32 irq_reg, u32 irq_val);
> +	u32 (*read_irq_stat)(struct sun6i_dma_dev *sdev, u32 irq_reg);
> +	void (*write_irq_stat)(struct sun6i_dma_dev *sdev, u32 irq_reg, u32 status);
>  	u32 src_burst_lengths;
>  	u32 dst_burst_lengths;
>  	u32 src_addr_widths;
> @@ -347,6 +352,26 @@ static void sun6i_set_mode_h6(u32 *p_cfg, s8 src_mode, s8 dst_mode)
>  		  DMA_CHAN_CFG_DST_MODE_H6(dst_mode);
>  }
>  
> +static u32 sun6i_read_irq_en(struct sun6i_dma_dev *sdev, u32 irq_reg)
> +{
> +	return readl(sdev->base + DMA_IRQ_EN(irq_reg));
> +}
> +
> +static void sun6i_write_irq_en(struct sun6i_dma_dev *sdev, u32 irq_reg, u32 irq_val)
> +{
> +	writel(irq_val, sdev->base + DMA_IRQ_EN(irq_reg));
> +}
> +
> +static u32 sun6i_read_irq_stat(struct sun6i_dma_dev *sdev, u32 irq_reg)
> +{
> +	return readl(sdev->base + DMA_IRQ_STAT(irq_reg));
> +}
> +
> +static void sun6i_write_irq_stat(struct sun6i_dma_dev *sdev, u32 irq_reg, u32 status)
> +{
> +	writel(status, sdev->base + DMA_IRQ_STAT(irq_reg));
> +}
> +
>  static size_t sun6i_get_chan_size(struct sun6i_pchan *pchan)
>  {
>  	struct sun6i_desc *txd = pchan->desc;
> @@ -460,16 +485,16 @@ static int sun6i_dma_start_desc(struct sun6i_vchan *vchan)
>  
>  	vchan->irq_type = vchan->cyclic ? DMA_IRQ_PKG : DMA_IRQ_QUEUE;
>  
> -	irq_val = readl(sdev->base + DMA_IRQ_EN(irq_reg));
> +	irq_val = sdev->cfg->read_irq_en(sdev, irq_reg);
>  	irq_val &= ~((DMA_IRQ_HALF | DMA_IRQ_PKG | DMA_IRQ_QUEUE) <<
>  			(irq_offset * DMA_IRQ_CHAN_WIDTH));
>  	irq_val |= vchan->irq_type << (irq_offset * DMA_IRQ_CHAN_WIDTH);
> -	writel(irq_val, sdev->base + DMA_IRQ_EN(irq_reg));
> +	sdev->cfg->write_irq_en(sdev, irq_reg, irq_val);
>  
>  	writel(pchan->desc->p_lli, pchan->base + DMA_CHAN_LLI_ADDR);
>  	writel(DMA_CHAN_ENABLE_START, pchan->base + DMA_CHAN_ENABLE);
>  
> -	sun6i_dma_dump_com_regs(sdev);
> +	sdev->cfg->dump_com_regs(sdev);
>  	sun6i_dma_dump_chan_regs(sdev, pchan);
>  
>  	return 0;
> @@ -549,14 +574,14 @@ static irqreturn_t sun6i_dma_interrupt(int irq, void *dev_id)
>  	u32 status;
>  
>  	for (i = 0; i < sdev->num_pchans / DMA_IRQ_CHAN_NR; i++) {
> -		status = readl(sdev->base + DMA_IRQ_STAT(i));
> +		status = sdev->cfg->read_irq_stat(sdev, i);
>  		if (!status)
>  			continue;
>  
>  		dev_dbg(sdev->slave.dev, "DMA irq status %s: 0x%x\n",
>  			str_high_low(i), status);
>  
> -		writel(status, sdev->base + DMA_IRQ_STAT(i));
> +		sdev->cfg->write_irq_stat(sdev, i, status);
>  
>  		for (j = 0; (j < DMA_IRQ_CHAN_NR) && status; j++) {
>  			pchan = sdev->pchans + j;
> @@ -1101,6 +1126,13 @@ static inline void sun6i_dma_free(struct sun6i_dma_dev *sdev)
>  	}
>  }
>  
> +#define SUN6I_DMA_IRQ_A31_COMMON_OPS	\
> +	.dump_com_regs    = sun6i_dma_dump_com_regs,	\
> +	.read_irq_en      = sun6i_read_irq_en,	\
> +	.write_irq_en     = sun6i_write_irq_en,	\
> +	.read_irq_stat    = sun6i_read_irq_stat,	\
> +	.write_irq_stat   = sun6i_write_irq_stat,
> +
>  /*
>   * For A31:
>   *
> @@ -1132,6 +1164,7 @@ static struct sun6i_dma_config sun6i_a31_dma_cfg = {
>  	.dst_addr_widths   = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
>  			     BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
>  			     BIT(DMA_SLAVE_BUSWIDTH_4_BYTES),
> +	SUN6I_DMA_IRQ_A31_COMMON_OPS
>  };
>  
>  /*
> @@ -1155,6 +1188,7 @@ static struct sun6i_dma_config sun8i_a23_dma_cfg = {
>  	.dst_addr_widths   = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
>  			     BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
>  			     BIT(DMA_SLAVE_BUSWIDTH_4_BYTES),
> +	SUN6I_DMA_IRQ_A31_COMMON_OPS
>  };
>  
>  static struct sun6i_dma_config sun8i_a83t_dma_cfg = {
> @@ -1173,6 +1207,7 @@ static struct sun6i_dma_config sun8i_a83t_dma_cfg = {
>  	.dst_addr_widths   = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
>  			     BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
>  			     BIT(DMA_SLAVE_BUSWIDTH_4_BYTES),
> +	SUN6I_DMA_IRQ_A31_COMMON_OPS
>  };
>  
>  /*
> @@ -1200,6 +1235,7 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = {
>  			     BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
>  			     BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
>  			     BIT(DMA_SLAVE_BUSWIDTH_8_BYTES),
> +	SUN6I_DMA_IRQ_A31_COMMON_OPS
>  };
>  
>  /*
> @@ -1221,6 +1257,7 @@ static struct sun6i_dma_config sun50i_a64_dma_cfg = {
>  			     BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
>  			     BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
>  			     BIT(DMA_SLAVE_BUSWIDTH_8_BYTES),
> +	SUN6I_DMA_IRQ_A31_COMMON_OPS
>  };
>  
>  /*
> @@ -1244,6 +1281,7 @@ static struct sun6i_dma_config sun50i_a100_dma_cfg = {
>  			     BIT(DMA_SLAVE_BUSWIDTH_8_BYTES),
>  	.has_high_addr = true,
>  	.has_mbus_clk = true,
> +	SUN6I_DMA_IRQ_A31_COMMON_OPS
>  };
>  
>  /*
> @@ -1266,6 +1304,7 @@ static struct sun6i_dma_config sun50i_h6_dma_cfg = {
>  			     BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
>  			     BIT(DMA_SLAVE_BUSWIDTH_8_BYTES),
>  	.has_mbus_clk = true,
> +	SUN6I_DMA_IRQ_A31_COMMON_OPS
>  };
>  
>  /*
> @@ -1289,6 +1328,7 @@ static struct sun6i_dma_config sun8i_v3s_dma_cfg = {
>  	.dst_addr_widths   = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
>  			     BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
>  			     BIT(DMA_SLAVE_BUSWIDTH_4_BYTES),
> +	SUN6I_DMA_IRQ_A31_COMMON_OPS
>  };
>  
>  static const struct of_device_id sun6i_dma_match[] = {
>
  

On Mon, Jun 29, 2026 at 12:35:05AM +0200, Andre Przywara wrote:
> On Mon, 22 Jun 2026 01:36:23 +0000
> Yuanshen Cao <alex.caoys@gmail.com> wrote:
> 
> Hi,
> 
> first, many thanks for sending this, also for structuring the changes
> nicely, so that they remain reviewable!
> 
> > Refactor to support the Allwinner A733 DMA controller. Currently, the
> > `sun6i-dma` driver has several functions related to interrupt handling
> > (reading/writing interrupt enable and status registers) and register
> > dumping that are hardcoded.
> > 
> > To support the A733, which has different register layouts and interrupt
> > handling logic, these functions are being moved into the
> > `sun6i_dma_config` structure as function pointers.
> 
> So I see that this driver already makes use of per-device function
> pointer, though personally I don't like this approach very much, as it
> decreases the readability, and suggests significant differences between
> the SoC generations that are not really there: each function just reads
> or write an MMIO register, it's just the offset that differs.

Yes, I considered to do this but since the original mainline code and
BSP code all use function-pointer so I impelement this the same way.

> 
> So I think it's better to express the differences through data
> entries in the config struct, for the IRQ enable/stat functions I think
> this should be something like this:
> 
> struct sun6i_dma_config {
> 	...
> 	u32	irq_stride;
> 	u32	irq_en_offset;
> 	u32	irq_stat_offset;
> 	...
> };
> 
> -	irq_val = readl(sdev->base + DMA_IRQ_EN(irq_reg));
> +	irq_val = readl(sdev->base + sdev->cfg->irq_en_offset + irq_reg * sdev->cfg->irq_stride);
> 
> the existing configs set .stride to 0x04, and .en_offset to 0x0, the
> A733 later uses .stride = 0x40 and .en_offset = 0x134.
> Maybe we still move that now longish line into a helper function, but
> not a config specific one. 
> 
> I think that's more readable, and avoids unnecessary redirections and
> potential pipeline stalls.
> 
> dump_com_regs is a different story, since the two instances of that
> function are significantly different.
> 
> What do you think?

I am okay to change this one but should we do the same for the other
function pointers as well? Just to keep them aligned. Let me know what
do you think.

> > This allows the
> > driver to use a polymorphic approach where the specific implementation
> > is determined by the hardware configuration assigned during device
> > probing.
> > 
> > Changes:
> > - Added function pointers to `struct sun6i_dma_config` for:
> 
> By the way: the preferred style to list changes in commit messages in
> imperative mood [1], not in past tense. Think about you ask the
> code base what to change:
> 
> Add function pointers to ...
> Implement generic functions ...

Thanks for the advice! I'll make sure to follow them in the future.

Best,
Alex

> Cheers,
> Andre
> 
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst#n94
> 
> >     - `dump_com_regs`
> >     - `read_irq_en`
> >     - `write_irq_en`
> >     - `read_irq_stat`
> >     - `write_irq_stat`
> > - Implemented generic `sun6i_read/write_irq_*` functions for existing
> >   hardware.
> > - Added a macro and updated existing `sun6i_dma_config` instances (A31,
> >   A23, H3, A64, A100, H6, V3S) to use these new function pointers.
> > 
> > Reviewed-by: Frank Li <Frank.Li@nxp.com>
> > Signed-off-by: Yuanshen Cao <alex.caoys@gmail.com>
> > ---
> >  drivers/dma/sun6i-dma.c | 50 ++++++++++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 45 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> > index a9a254dbf8cb..ef3052c4ab36 100644
> > --- a/drivers/dma/sun6i-dma.c
> > +++ b/drivers/dma/sun6i-dma.c
> > @@ -138,6 +138,11 @@ struct sun6i_dma_config {
> >  	void (*set_burst_length)(u32 *p_cfg, s8 src_burst, s8 dst_burst);
> >  	void (*set_drq)(u32 *p_cfg, s8 src_drq, s8 dst_drq);
> >  	void (*set_mode)(u32 *p_cfg, s8 src_mode, s8 dst_mode);
> > +	void (*dump_com_regs)(struct sun6i_dma_dev *sdev);
> > +	u32 (*read_irq_en)(struct sun6i_dma_dev *sdev, u32 irq_reg);
> > +	void (*write_irq_en)(struct sun6i_dma_dev *sdev, u32 irq_reg, u32 irq_val);
> > +	u32 (*read_irq_stat)(struct sun6i_dma_dev *sdev, u32 irq_reg);
> > +	void (*write_irq_stat)(struct sun6i_dma_dev *sdev, u32 irq_reg, u32 status);
> >  	u32 src_burst_lengths;
> >  	u32 dst_burst_lengths;
> >  	u32 src_addr_widths;
> > @@ -347,6 +352,26 @@ static void sun6i_set_mode_h6(u32 *p_cfg, s8 src_mode, s8 dst_mode)
> >  		  DMA_CHAN_CFG_DST_MODE_H6(dst_mode);
> >  }
> >  
> > +static u32 sun6i_read_irq_en(struct sun6i_dma_dev *sdev, u32 irq_reg)
> > +{
> > +	return readl(sdev->base + DMA_IRQ_EN(irq_reg));
> > +}
> > +
> > +static void sun6i_write_irq_en(struct sun6i_dma_dev *sdev, u32 irq_reg, u32 irq_val)
> > +{
> > +	writel(irq_val, sdev->base + DMA_IRQ_EN(irq_reg));
> > +}
> > +
> > +static u32 sun6i_read_irq_stat(struct sun6i_dma_dev *sdev, u32 irq_reg)
> > +{
> > +	return readl(sdev->base + DMA_IRQ_STAT(irq_reg));
> > +}
> > +
> > +static void sun6i_write_irq_stat(struct sun6i_dma_dev *sdev, u32 irq_reg, u32 status)
> > +{
> > +	writel(status, sdev->base + DMA_IRQ_STAT(irq_reg));
> > +}
> > +
> >  static size_t sun6i_get_chan_size(struct sun6i_pchan *pchan)
> >  {
> >  	struct sun6i_desc *txd = pchan->desc;
> > @@ -460,16 +485,16 @@ static int sun6i_dma_start_desc(struct sun6i_vchan *vchan)
> >  
> >  	vchan->irq_type = vchan->cyclic ? DMA_IRQ_PKG : DMA_IRQ_QUEUE;
> >  
> > -	irq_val = readl(sdev->base + DMA_IRQ_EN(irq_reg));
> > +	irq_val = sdev->cfg->read_irq_en(sdev, irq_reg);
> >  	irq_val &= ~((DMA_IRQ_HALF | DMA_IRQ_PKG | DMA_IRQ_QUEUE) <<
> >  			(irq_offset * DMA_IRQ_CHAN_WIDTH));
> >  	irq_val |= vchan->irq_type << (irq_offset * DMA_IRQ_CHAN_WIDTH);
> > -	writel(irq_val, sdev->base + DMA_IRQ_EN(irq_reg));
> > +	sdev->cfg->write_irq_en(sdev, irq_reg, irq_val);
> >  
> >  	writel(pchan->desc->p_lli, pchan->base + DMA_CHAN_LLI_ADDR);
> >  	writel(DMA_CHAN_ENABLE_START, pchan->base + DMA_CHAN_ENABLE);
> >  
> > -	sun6i_dma_dump_com_regs(sdev);
> > +	sdev->cfg->dump_com_regs(sdev);
> >  	sun6i_dma_dump_chan_regs(sdev, pchan);
> >  
> >  	return 0;
> > @@ -549,14 +574,14 @@ static irqreturn_t sun6i_dma_interrupt(int irq, void *dev_id)
> >  	u32 status;
> >  
> >  	for (i = 0; i < sdev->num_pchans / DMA_IRQ_CHAN_NR; i++) {
> > -		status = readl(sdev->base + DMA_IRQ_STAT(i));
> > +		status = sdev->cfg->read_irq_stat(sdev, i);
> >  		if (!status)
> >  			continue;
> >  
> >  		dev_dbg(sdev->slave.dev, "DMA irq status %s: 0x%x\n",
> >  			str_high_low(i), status);
> >  
> > -		writel(status, sdev->base + DMA_IRQ_STAT(i));
> > +		sdev->cfg->write_irq_stat(sdev, i, status);
> >  
> >  		for (j = 0; (j < DMA_IRQ_CHAN_NR) && status; j++) {
> >  			pchan = sdev->pchans + j;
> > @@ -1101,6 +1126,13 @@ static inline void sun6i_dma_free(struct sun6i_dma_dev *sdev)
> >  	}
> >  }
> >  
> > +#define SUN6I_DMA_IRQ_A31_COMMON_OPS	\
> > +	.dump_com_regs    = sun6i_dma_dump_com_regs,	\
> > +	.read_irq_en      = sun6i_read_irq_en,	\
> > +	.write_irq_en     = sun6i_write_irq_en,	\
> > +	.read_irq_stat    = sun6i_read_irq_stat,	\
> > +	.write_irq_stat   = sun6i_write_irq_stat,
> > +
> >  /*
> >   * For A31:
> >   *
> > @@ -1132,6 +1164,7 @@ static struct sun6i_dma_config sun6i_a31_dma_cfg = {
> >  	.dst_addr_widths   = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
> >  			     BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
> >  			     BIT(DMA_SLAVE_BUSWIDTH_4_BYTES),
> > +	SUN6I_DMA_IRQ_A31_COMMON_OPS
> >  };
> >  
> >  /*
> > @@ -1155,6 +1188,7 @@ static struct sun6i_dma_config sun8i_a23_dma_cfg = {
> >  	.dst_addr_widths   = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
> >  			     BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
> >  			     BIT(DMA_SLAVE_BUSWIDTH_4_BYTES),
> > +	SUN6I_DMA_IRQ_A31_COMMON_OPS
> >  };
> >  
> >  static struct sun6i_dma_config sun8i_a83t_dma_cfg = {
> > @@ -1173,6 +1207,7 @@ static struct sun6i_dma_config sun8i_a83t_dma_cfg = {
> >  	.dst_addr_widths   = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
> >  			     BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
> >  			     BIT(DMA_SLAVE_BUSWIDTH_4_BYTES),
> > +	SUN6I_DMA_IRQ_A31_COMMON_OPS
> >  };
> >  
> >  /*
> > @@ -1200,6 +1235,7 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = {
> >  			     BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
> >  			     BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
> >  			     BIT(DMA_SLAVE_BUSWIDTH_8_BYTES),
> > +	SUN6I_DMA_IRQ_A31_COMMON_OPS
> >  };
> >  
> >  /*
> > @@ -1221,6 +1257,7 @@ static struct sun6i_dma_config sun50i_a64_dma_cfg = {
> >  			     BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
> >  			     BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
> >  			     BIT(DMA_SLAVE_BUSWIDTH_8_BYTES),
> > +	SUN6I_DMA_IRQ_A31_COMMON_OPS
> >  };
> >  
> >  /*
> > @@ -1244,6 +1281,7 @@ static struct sun6i_dma_config sun50i_a100_dma_cfg = {
> >  			     BIT(DMA_SLAVE_BUSWIDTH_8_BYTES),
> >  	.has_high_addr = true,
> >  	.has_mbus_clk = true,
> > +	SUN6I_DMA_IRQ_A31_COMMON_OPS
> >  };
> >  
> >  /*
> > @@ -1266,6 +1304,7 @@ static struct sun6i_dma_config sun50i_h6_dma_cfg = {
> >  			     BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
> >  			     BIT(DMA_SLAVE_BUSWIDTH_8_BYTES),
> >  	.has_mbus_clk = true,
> > +	SUN6I_DMA_IRQ_A31_COMMON_OPS
> >  };
> >  
> >  /*
> > @@ -1289,6 +1328,7 @@ static struct sun6i_dma_config sun8i_v3s_dma_cfg = {
> >  	.dst_addr_widths   = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
> >  			     BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
> >  			     BIT(DMA_SLAVE_BUSWIDTH_4_BYTES),
> > +	SUN6I_DMA_IRQ_A31_COMMON_OPS
> >  };
> >  
> >  static const struct of_device_id sun6i_dma_match[] = {
> > 
>
  

Hi Yuanshen, Andre,

I applied the v3 series locally on top of dc59e4fea9d8. The
series applied cleanly, the focused DMA binding check passed, and
a focused drivers/dma/sun6i-dma.o build passed. I have not done a
hardware DMA runtime test, so this is only static review plus
build/schema validation.

On the IRQ accessor shape, I think Andre's data-driven direction is
a good fit for the enable/status register differences. The
A733-specific values look like data: enable offset 0x134, status
offset 0x138, stride 0x40. A small helper using cfg offsets/stride
would keep the call sites readable without needing per-compatible
read/write accessors. I would keep dump_com_regs separate unless
there is a clean table-driven way to express the genuinely different
dump layout.

While comparing this with the public Sun60iw2 BSP, I think the same
respin should also fix the interrupt channel decode path that Sashiko
pointed out. The series encodes the interrupt register as:

  irq_reg = pchan->idx / sdev->cfg->num_channels_per_reg;
  irq_offset = pchan->idx % sdev->cfg->num_channels_per_reg;

but the interrupt handler still decodes with:

  pchan = sdev->pchans + j;

For A733, num_channels_per_reg is 1, so j is always 0 and each IRQ
status register would map back to pchans[0]. The public Sun60iw2 BSP
uses the inverse mapping:

  pchan = sdev->pchans + (i * sdev->cfg->channum_per_reg + j);

That matches the encode path and looks like the shape needed here as
well. The register-loop bounds probably want the same treatment:
derive the number of IRQ status registers from the real channel count,
not from an implicitly exact division.