Re: [PATCH 2/2] dma: xilinx: dpdma: Add the Xilinx DisplayPort DMA engine driver

[Vinod Koul <vinod.koul-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html