Re: [PATCH][1/3] RapidIO support: core

[Matt Porter <mporter@kernel.crashing.org>]

On Wed, Jun 01, 2005 at 01:37:57PM -0700, Stephen Hemminger wrote:
> Quick review comments:
 
Thanks for taking the time to look at this.
 
> 1. Why hack up vmlinux.lds.h? wouldn't declaring rio_route_ops
> to be a constant do what you want?

There are more switch devices to be added. That is, each vendor has
their own way to set/get route table entries.  This approach makes it
easy to add a fooswitch.c without adding an entry to a separate constant
table as we support additional switches (at least 5 vendors so far). I
originally  looked at how PCI was using a section for quirks and figured
that was OK to copy. I can see how PCI might justify it a bit more since
it is allowing arch-specific quirks to be registered without modifying
a generic PCI table of quirks.  If it's a concern, I can make a constant
table but it's not as nice IMHO.

FWIW, I was going to expand usage of RIO-specific sections a bit more,
so I'd like to see what the consensus is on using sections to generate
tables at build time.  Specifically, support for MMIO regions requires
node specific operations (specific to each vendor device) as RIO
neglected to standardize something like BARs a la PCI. My intention
was to put some device-specific ops to get the size of active MMIO
regions etc. with the same approach as the switch ops.

> 2. Unnecessary initializers
> 
> +struct bus_type rio_bus_type = {
> +	.name = "rapidio",
> +	.match = rio_match_bus,
> +	.suspend = NULL,
> +	.resume = NULL,
> 
> 	NULL is unnecessary here.

I'll drop those.

> 3. Use existing macro's
> 
> +#define DEBUG
> +
> +#ifdef DEBUG
> +#define DBG(x...) printk(x)
> +#else
> +#define DBG(x...)
> +#endif
> +
> 
> 	Use pr_debug() and isn't this debugged yet?

That was to control debug info per-file, but can be changed.  It's
debugged to a certain level, but some network configuration are
going to expose corner cases that I haven't noticed while testing
with 2-3 boards on the 1st gen hardware. Same reason there are still
debug statements riddled throughout the PCI code...there's always
more bugs. :)

> 4. Lists should be static if possible
> 
> +
> +LIST_HEAD(rio_mports);
> 	static LIST_HEAD(rio_mports)

Ok, will do.
 
> 5. rio_nets defined but not used (if it is used later then
>    make it visible to only that code.)

That was supposed to be removed as I don't yet have a use for it.
I'll remove it.

> 6. rio_net_lock does it need to be global.
>    also maybe a name change to rio_global_list_lock

It is taken outside of rio-scan.c while searching lists so it can't
be static. The name does suck, though. I'll change it to something
like that.

-Matt
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/