Re: [PATCH V9 XDMA 1/2] dmaengine: xilinx: xdma: Add xilinx xdma driver

[Lizhi Hou]

On 11/5/22 00:17, Vinod Koul wrote:
> On 04-11-22, 11:17, Lizhi Hou wrote:
> > On 11/4/22 10:57, Vinod Koul wrote:
> > > On 04-11-22, 09:57, Lizhi Hou wrote:
> > > > On 11/4/22 07:32, Vinod Koul wrote:
> > > > > On 25-10-22, 10:57, Lizhi Hou wrote:
> > > > > > +static inline int xdma_write_reg(struct xdma_device *xdev, u32 base, u32 reg,
> > > > > > +				 u32 val)
> > > > > > +{
> > > > > > +	return regmap_write(xdev->regmap, base + reg, val);
> > > > > > +}
> > > > > Do you really need one more level indirection?
> > > > Do you mean using readl / writel instead of regmap_* here?
> > > Nope, I refer to using regmap_write() intead of xdma_write_reg()
> > Ok. As you mentioned below,
> >
> >     why not move err into xdma_write_reg(), rather than adding in each
> >     helper!
> >
> > If I use regmap_write() directly, I will not be able to move err into
> > xdma_write_reg(). Having a inline function might be useful to add debug
> > code.  May I keep xdma_write_reg()?
> Okay, either way if xdma_write_reg() is doing only regmap_write() then
> no, if it has extra logic like logging on error etc then it makes sense
Got it. Thanks. :)
> > > > > > +failed:
> > > > > > +	xdma_free_desc(&sw_desc->vdesc);
> > > > > who will free sw_desc here?
> > > > sw_desc is freed by xdma_free_desc(). xdma_free_desc() is virt-dma callback
> > > > it converts struct virt_dma_desc pointer to driver sw_desc pointer and free
> > > > the whole thing.
> > > IN case of error, you are returning NULL, so allocated descriptor leaks
> > I meant the descriptor is freed inside xdma_free_desc() which is called
> > before 'return NULL'.
> >
> > xdma_free_desc(struct virt_dma_desc *vdesc)
> >
> > {
> >
> >          sw_desc = to_xdma_desc(vdesc);
> >
> >          .....
> >
> >          kfree(sw_desc);
> >
> > }
> ok
>
> > > > > > +#ifndef _PLATDATA_AMD_XDMA_H
> > > > > > +#define _PLATDATA_AMD_XDMA_H
> > > > > > +
> > > > > > +#include <linux/dmaengine.h>
> > > > > > +
> > > > > > +/**
> > > > > > + * struct xdma_chan_info - DMA channel information
> > > > > > + *	This information is used to match channel when request dma channel
> > > > > > + * @dir: Channel transfer direction
> > > > > > + */
> > > > > > +struct xdma_chan_info {
> > > > > > +	enum dma_transfer_direction dir;
> > > > > > +};
> > > > > > +
> > > > > > +#define XDMA_FILTER_PARAM(chan_info)	((void *)(chan_info))
> > > > > > +
> > > > > > +struct dma_slave_map;
> > > > > > +
> > > > > > +/**
> > > > > > + * struct xdma_platdata - platform specific data for XDMA engine
> > > > > > + * @max_dma_channels: Maximum dma channels in each direction
> > > > > > + */
> > > > > > +struct xdma_platdata {
> > > > > > +	u32 max_dma_channels;
> > > > > > +	u32 device_map_cnt;
> > > > > > +	struct dma_slave_map *device_map;
> > > > > > +};
> > > > > why do you need this plat data
> > > > max_dma_channels is used to specify the maximum dma channels will be used.
> > > What is the device mode, who creates this dma device, devicetree or
> > > something else?
> > This dma engine is on PCI device (exposed on PCI BAR). Thus, the pci device
> > driver creates this dma device.
> So it is a platform_device type? Why not make it something like auxdev?

With our FPGA device, the XDMA IP is populated by flattened device tree. 
Using platform device will support both device tree and non-dt case.


Thanks,
Lizhi

>