On Tue, Jan 23, 2018 at 05:03:19PM +0000, Hyun Kwon wrote:
> > 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?
> > 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
> > > +#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
> > 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
> > > +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?
> > 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
> > > + 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
> > > +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?
--
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
