Re: [PATCH] [RFC][POWERPC] refactor dcr code

[Stephen Rothwell <sfr@canb.auug.org.au>]

[-- Attachment #1: Type: text/plain, Size: 2587 bytes --]

Hi Stephen,

Just some trivial issues ...

On Mon, 24 Mar 2008 16:28:33 -0700 Stephen Neuendorffer <stephen.neuendorffer@xilinx.com> wrote:
>
> +++ b/arch/powerpc/sysdev/dcr.c
> @@ -23,6 +23,67 @@
>  #include <asm/prom.h>
>  #include <asm/dcr.h>
>  
> +#if defined(CONFIG_PPC_DCR_NATIVE) && defined(CONFIG_PPC_DCR_MMIO)
> +
> +bool dcr_map_ok_generic(dcr_host_t host) {
> +	if(host.type == INVALID) {
          ^
Space, please.

> +		return 0;
> +	} else if(host.type == NATIVE) {

And again.  And more below.

> +		return dcr_map_ok_native(host.host.native);
> +	} else {
> +		return dcr_map_ok_mmio(host.host.mmio);
> +	}

You don't need the braces around each if/else clause. Again, more below.

> +dcr_host_t dcr_map_generic(struct device_node *dev, unsigned int dcr_n,
> +				 unsigned int dcr_c) {
> +	dcr_host_t host;
> +	const char *prop = of_get_property(dev, "dcr-access-method", NULL);
> +       	if(!strcmp(prop, "native")) {

Indentation.  We also usually separate declarations from code with a a
blank line.

> -dcr_host_t dcr_map(struct device_node *dev, unsigned int dcr_n,
> +dcr_host_mmio_t dcr_map_mmio(struct device_node *dev, unsigned int dcr_n,
>  		   unsigned int dcr_c)
>  {
> -	dcr_host_t ret = { .token = NULL, .stride = 0, .base = dcr_n };
> +	dcr_host_mmio_t ret = { .token = NULL, .stride = 0, .base = dcr_n };
>  	u64 addr;
>  
>  	pr_debug("dcr_map(%s, 0x%x, 0x%x)\n",
>  		 dev->full_name, dcr_n, dcr_c);
>  
>  	addr = of_translate_dcr_address(dev, dcr_n, &ret.stride);
> -	pr_debug("translates to addr: 0x%lx, stride: 0x%x\n",
> +	pr_debug("translates to addr: 0x%p, stride: 0x%x\n",
>  		 addr, ret.stride);

This change should cause a warning because you are passing a u64 where
printk is expecting a pointer.  We usually use %llx and cast the u64 to
unsigned long long.

> +++ b/include/asm-powerpc/dcr-mmio.h
> @@ -27,20 +27,20 @@ typedef struct {
>  	void __iomem *token;
>  	unsigned int stride;
>  	unsigned int base;
> -} dcr_host_t;
> +} dcr_host_mmio_t;
>  
> -#define DCR_MAP_OK(host)	((host).token != NULL)
> +#define dcr_map_ok_mmio(host)	((host).token != NULL)

This could be a static inline function.

> +++ b/include/asm-powerpc/dcr-native.h
> @@ -24,14 +24,14 @@
>  
>  typedef struct {
>  	unsigned int base;
> -} dcr_host_t;
> +} dcr_host_native_t;
>  
> -#define DCR_MAP_OK(host)	(1)
> +#define dcr_map_ok_native(host)	(1)

As could this.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]