Re: [PATCH 1/2] [POWERPC] 8xx: mpc885ads pcmcia support

[olof@lixom.net (Olof Johansson)]

Hi,

Some minor nitpicks below. Overall it looks good.


-Olof

On Sun, May 06, 2007 at 05:26:33AM +0400, Vitaly Bordug wrote:
> @@ -322,6 +334,70 @@ void init_smc_ioports(struct fs_uart_platform_info *data)
>  	}
>  }
>  
> +#ifdef CONFIG_PCMCIA_M8XX
> +static void pcmcia_hw_setup(int slot, int enable)
> +{
> +	unsigned *bcsr_io;
> +
> +	bcsr_io = ioremap(BCSR1, sizeof(unsigned long));
> +	if (enable)
> +		clrbits32(bcsr_io, BCSR1_PCCEN);
> +	else
> +		setbits32(bcsr_io, BCSR1_PCCEN);
> +
> +	iounmap(bcsr_io);
> +}

If you move this (and next) function up, you don't have to do the
prototypes in the beginning of the file. One less #ifdef that way.

> +
> +static int pcmcia_set_voltage(int slot, int vcc, int vpp)
> +{
> +	u32 reg = 0;
> +	unsigned *bcsr_io;
> +
> +	bcsr_io = ioremap(BCSR1, sizeof(unsigned long));
> +
> +	switch(vcc) {
> +	case 0:
> +		break;
> +	case 33:
> +		reg |= BCSR1_PCCVCC0;
> +		break;
> +	case 50:
> +		reg |= BCSR1_PCCVCC1;
> +		break;
> +	default:
> +		return 1;
> +	}
> +
> +	switch(vpp) {
> +	case 0:
> +       	break;
> +	case 33:
> +	case 50:
> +       	if(vcc == vpp)
> +			reg |= BCSR1_PCCVPP1;
> +		else
> +			return 1;
> +		break;
> +	case 120:
> +	if ((vcc == 33) || (vcc == 50))
> +		reg |= BCSR1_PCCVPP0;
> +	else
> +		return 1;
> +	default:
> +		return 1;
> +	}
> +
> +	/* first, turn off all power */
> +	clrbits32(bcsr_io, 0x00610000);
> +
> +	/* enable new powersettings */
> +	setbits32(bcsr_io, reg);
> +
> +	iounmap(bcsr_io);
> +	return 0;
> +}
> +#endif
> +
>  int platform_device_skip(const char *model, int id)
>  {
>  #ifdef CONFIG_MPC8xx_SECOND_ETH_SCC3
> diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
> index 8a123c7..880e45f 100644
> --- a/arch/powerpc/sysdev/fsl_soc.c
> +++ b/arch/powerpc/sysdev/fsl_soc.c
> @@ -1028,6 +1028,19 @@ err:
>  
>  arch_initcall(fs_enet_of_init);
>  
> +static int __init fsl_pcmcia_of_init(void)
> +{
> +	struct device_node *np = NULL;
> +	/*
> +	 * Register all the devices which type is "pcmcia"
> +	 */
> +	while ((np = of_find_compatible_node(np, 
> +			"pcmcia", "fsl,pq-pcmcia")) != NULL)
> +			    of_platform_device_create(np, "m8xx-pcmcia", NULL);

Do you really need this now that you have the device table in the driver?

> +	return 0;
> +}
> +
> +arch_initcall(fsl_pcmcia_of_init);
>  
>  static const char *smc_regs = "regs";
>  static const char *smc_pram = "pram";
> diff --git a/arch/powerpc/sysdev/mpc8xx_pic.h b/arch/powerpc/sysdev/mpc8xx_pic.h
> index afa2ee6..07be061 100644
> --- a/arch/powerpc/sysdev/mpc8xx_pic.h
> +++ b/arch/powerpc/sysdev/mpc8xx_pic.h
> @@ -4,7 +4,7 @@
>  #include <linux/irq.h>
>  #include <linux/interrupt.h>
>  
> -extern struct hw_interrupt_type mpc8xx_pic;
> +/*extern struct hw_interrupt_type mpc8xx_pic;*/

Take it out or leave it in, but don't comment it out, please

>  
>  int mpc8xx_pic_init(void);
>  unsigned int mpc8xx_get_irq(void);
> diff --git a/drivers/pcmcia/Kconfig b/drivers/pcmcia/Kconfig
> index 35f8864..c3fd55d 100644
> --- a/drivers/pcmcia/Kconfig
> +++ b/drivers/pcmcia/Kconfig
> @@ -183,6 +183,7 @@ config PCMCIA_M8XX
>          tristate "MPC8xx PCMCIA support"
>          depends on PCMCIA && PPC && 8xx 
>          select PCCARD_IODYN
> +	select PCCARD_NONSTATIC

I was going to say "whitespace!", but seems like this added line is the only one
that's actually using tabs. :-) (Yes, I saw 2/2 that fixes this).

>          help
>          Say Y here to include support for PowerPC 8xx series PCMCIA
>          controller.