On Tue, Jan 23, 2018 at 05:03:19PM +0000, Hyun Kwon wrote:

quoted

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

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

quoted

+#define XILINX_DPDMA_INTR_DESC_DONE_SHIFT		0

you 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

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

quoted

+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?

There's not much configuration to be done. Channels are pretty much identical
with fixed configurations.

hmmm, you do support DMA_SLAVE right?

quoted

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
struct xilinx_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

quoted

+	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?

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 to request_irq() from
devm_request_irq(), and remove the irq when removing the driver.

yes and ensure tasklets are quiesced

quoted

quoted

+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?