Re: [PATCH][3/5] RapidIO support: core enum

On Mon, Jun 06, 2005 at 11:19:50PM -0600, Zwane Mwaikambo wrote:

On Mon, 6 Jun 2005, Matt Porter wrote:

quoted

+spinlock_t rio_global_list_lock = SPIN_LOCK_UNLOCKED;

spin_lock_init?

How about DEFINE_SPINLOCK() as they do the same thing (except the 
difference in amount of babble)? This is what PCI is doing, for
example. I use the same init method in the static locks found
in rio-access.c, FWIW.

quoted

+extern struct rio_route_ops __start_rio_route_ops[];
+extern struct rio_route_ops __end_rio_route_ops[];

rio.h?

Yep, will move.

quoted

+static void
+rio_set_device_id(struct rio_mport *port, u16 destid, u8 hopcount, u16 did)

Shouldn't those be on the same line?

Yes, will fix.
 

quoted

+static int rio_device_has_destid(struct rio_mport *port, int src_ops,
+				 int dst_ops)
+{
+	if (((src_ops & RIO_SRC_OPS_READ) ||
+	     (src_ops & RIO_SRC_OPS_WRITE) ||
+	     (src_ops & RIO_SRC_OPS_ATOMIC_TST_SWP) ||
+	     (src_ops & RIO_SRC_OPS_ATOMIC_INC) ||
+	     (src_ops & RIO_SRC_OPS_ATOMIC_DEC) ||
+	     (src_ops & RIO_SRC_OPS_ATOMIC_SET) ||
+	     (src_ops & RIO_SRC_OPS_ATOMIC_CLR)) &&
+	    ((dst_ops & RIO_DST_OPS_READ) ||
+	     (dst_ops & RIO_DST_OPS_WRITE) ||
+	     (dst_ops & RIO_DST_OPS_ATOMIC_TST_SWP) ||
+	     (dst_ops & RIO_DST_OPS_ATOMIC_INC) ||
+	     (dst_ops & RIO_DST_OPS_ATOMIC_DEC) ||
+	     (dst_ops & RIO_DST_OPS_ATOMIC_SET) ||
+	     (dst_ops & RIO_DST_OPS_ATOMIC_CLR))) {
+		return 1;

Why not just;

mask = (RIO_DST_OPS_READ | RIO_DST_OPS_WRITE....)
return !!((dst_ops & mask) && (src_ops & mask))

Yes, that's good. I already had that comment from an IRC discussion
and neglected to address it in the last updates. Will do.

quoted

+	rdev->dev.dma_mask = (u64 *) 0xffffffff;
+	rdev->dev.coherent_dma_mask = 0xffffffffULL;

Shouldn't that be dma_set_mask?

There is a problem there, yes, but it shouldn't use dma_set_mask().
dma_set_mask() needs [struct device]->dma_mask to be non-zero
(initialized to something) to do anything in all the copied
implementations I've seen.  I need to do something like the
following to initialize the struct device dma_mask properly:

	rdev->dma_mask = 0xffffffffULL;
	rdev->dev.dma_mask = &rdev->dma_mask;

-Matt