[Cbe-oss-dev] [patch 02/16] powerpc and x86_64: Initial port of PCIe endpoint device driver for Axon

[Arnd Bergmann]

On Thursday 13 March 2008, Murali Iyer wrote:
> On Tue, 2008-03-11 at 04:38 +0100, Arnd Bergmann wrote:

> > The Signed-off-by: from the person sending the mail should always
> > come last, and you don't need to have S-o-b from each of the authors,
> > but it is usually a nice guesture to list all co-authors in the
> > changelog.
> Yep. these are manually entered. we didn't use any tool. is there a
> easy way to do this automatically?

When working with git, these happen more or less automatically,
but it's also more complicated to use than just quilt

> > > +enum message_type	{
> > > +	D2D_MSG_TYPE	     = 0, /* base driver to driver notification */
> > > +	APNET_MSG_TYPE	     = 1, /* apnet ethernet notification */
> > > +	DACS_MSG_TYPE	     = 2, /* DACS interface notification */
> > > +	DMA_LOCAL_MSG_TYPE   = 3, /* notification to system that initiated */
> > > +				  /*  a DMA */
> > > +	DMA2_LOCAL_MSG_TYPE  = 4, /* notification to system that initiated */
> > > +				  /*  a DMA */
> > > +	DMA_REMOTE_MSG_TYPE  = 5, /* notification to system that didn't */
> > > +				  /*  initiate the DMA */
> > > +	DMA2_REMOTE_MSG_TYPE = 6  /* notification to system that didn't */
> > > +				  /* initiate the DMA */
> > > +	/* type 7 reserved for other remote notifications */
> > > +	/* types 8 - 15 reserved for local callbacks */
> > > +};
> > 
> > It may be good to mark the parts of the header that are defining the
> > interface between the two sides, so you will know if you break the
> > communication or can avoid doing it.
> I don't get it. all of these are valid for both sides...

The point is that you might have to add another message type, in which
case it won't work if you only change one side. If you change the numbers
in some way, all of them will break unless you change both.

Other changes should be transparent to the remote end, so it's good to know
if a particular change breaks the communication and requires rebuilding
both sides.

> > Since the enum is in the global namespace, please qualify it with an
> > appropriate prefix, e.g. axon_message_type.
> you mean change enum message_type to axon_message_type?

yes.

> > > +
> > > +
> > > +/* helper mapping functions for apnet */
> > > +int axon_dma_map_single(int dev_num, void *addr,
> > > +			size_t size, int dir, dma_addr_t *handle);
> > > +
> > > +void axon_dma_unmap_single(int dev_num, dma_addr_t handle,
> > > +			   size_t size, int dir);
> > 
> > I think these should not be needed if you supply the right
> > dma-API implementation for device behind the PLB5.
> not really. need a proper device registration for apnet to go away.
> apnet driver needs to map/unmap dma directly.. since it has not
> registered properly this function is needed.

I think that is what I was saying: once you register apnet properly
and change the DMA accessors to handle it correctly, you can change
apnet and dacs to just call dma_map_single instead of axon_dma_map_single.

> > > Index: linux-2.6.23.1/include/linux/triblade/axon_ioctl.h
> > > ===================================================================
> > > --- /dev/null
> > > +++ linux-2.6.23.1/include/linux/triblade/axon_ioctl.h
> > > @@ -0,0 +1,149 @@
> > 
> > Please don't mix high-level driver stuff with the common plumbing
> > layer. The ioctl definitions need to go into the patch that
> > provides the user interface, and it would be good to keep that
> > patch separate from the rest of the series.
> ok. one extra patch file.. and makes sense.
> 

actually, no extra patch file, just move the header to the same patch
that implements the ioctl function.