dma: xilinx: dpdma: Add the Xilinx DisplayPort DMA engine driver
PATCH 2/2 dma: xilinx: dpdma: Add the Xilinx DisplayPort DMA engine driver
Quoted 5 lines · Hyun Kwon
Quoted 2 lines
why should this be a compile option
This debugfs code is for testing / regression, and we don't want to enable it for regular users.
Right and that is why you have CONFIG_DEBUGFS, why not use that?
Quoted 6 lines
Quoted 2 lines
do you need all these headers?
I directly included all headers that are used in this driver. Some of them can be removed from indirect inclusions, and I'm fine with that. Please let me know if that's your preference.
Yes pls remove the ones that are needed
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
+#define
XILINX_DPDMA_INTR_DESC_DONE_SHIFT0you can define a common shift using ffs
I guess you mean to replace, (value & MASK) << SHIFT, with (value & MASK) << ffs(MASK). I'll change to that way. Let me know otherwise.
yes and you may use the ones in bitfield.h
Quoted 6 lines
Quoted 2 lines
what does it mean (lower 32 bit of Nth 4KB page)
Each descriptor can point up to 5 - 48bit address payloads. src_addr* fields contain lower 32bit of 48bit address. Remaining upper 16bit is programmed in addr_ext* fields.
pls document this
Quoted 10 lines
Quoted 9 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+static int xilinx_dpdma_config(struct dma_chan *dchan, + struct dma_slave_config *config) +{ + if (config->direction != DMA_MEM_TO_DEV) + return -EINVAL;?? Why are the config values not used, how else do you configure the channel?
Quoted 2 lines
There's not much configuration to be done. Channels are pretty much identical with fixed configurations.
hmmm, you do support DMA_SLAVE right?
Quoted 8 lines
Quoted 2 lines
why have these wrappers which do not do anything?
It's just my personal preference to split into different code partitions, and each section is partitioned / grouped with some comment line. :-) Ex, a partition for struct
dma_chan, and another one for
structxilinx_dpdma_chan. It gives me sort of abstracted view. But it may be just me, and it comes with extra indirections. I'll remove unnecessary wrappers.
wrapper without any logic dont help
Quoted 8 lines
Quoted 7 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+ for (i = 0; i < XILINX_DPDMA_NUM_CHAN; i++) + if (xdev->chan[i]) + xilinx_dpdma_chan_remove(xdev->chan[i]);At this point your irq is still enabled and can fire, and can schedule tasklet.. Are you sure you are okay with that?
Quoted 4 lines
Ok. You mean that an interrupt can occur right before
xilinx_dpdma_disable_intr(), and the interrupt may be handled in the middle or after removing this driver. I'll switch torequest_irq()fromdevm_request_irq(), and remove the irq when removing the driver.
yes and ensure tasklets are quiesced
Quoted 7 lines
Quoted 5 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+MODULE_AUTHOR("Xilinx, Inc."); +MODULE_DESCRIPTION("Xilinx DPDMA driver"); +MODULE_LICENSE("GPL v2");No
MODULE_ALIAS()?Is it required? Could you please elaborate, to help my understanding?
did you try builing as modules and testing this?