Re: [PATCH] MPC8xx PCMCIA driver

[Jeff Garzik <jgarzik@pobox.com>]

Marcelo Tosatti wrote:
> +static int voltage_set(int slot, int vcc, int vpp)
> +{
> +	u_int reg = 0;
> +
> +	switch(vcc) {
> +	case 0: break;
> +	case 33:
> +		reg |= BCSR1_PCVCTL4;
> +		break;
> +	case 50: 
> +		reg |= BCSR1_PCVCTL5;
> +		break;
> +	default: 
> +		return 1;
> +	}
> +
> +	switch(vpp) {
> +	case 0: break;
> +	case 33: 
> +	case 50:
> +		if(vcc == vpp)
> +			reg |= BCSR1_PCVCTL6;
> +		else
> +			return 1;
> +		break;
> +	case 120: 
> +		reg |= BCSR1_PCVCTL7;
> +	default:
> +		return 1;
> +	}
> +
> +	if(!((vcc == 50) || (vcc == 0)))
> +		return 1;
> +
> +	/* first, turn off all power */
> +
> +	*((uint *)RPX_CSR_ADDR) &= ~(BCSR1_PCVCTL4 | BCSR1_PCVCTL5
> +				     | BCSR1_PCVCTL6 | BCSR1_PCVCTL7);
> +
> +	/* enable new powersettings */
> +
> +	*((uint *)RPX_CSR_ADDR) |= reg;

Should use bus read/write functions, such as foo_readl() or iowrite32().

Don't use weird types in kernel code such as 'uint'.  Use the more 
explicitly-sized u32.


> +	return 0;
> +}
> +
> +#define socket_get(_slot_) PCMCIA_SOCKET_KEY_5V
> +#define hardware_enable(_slot_)  /* No hardware to enable */
> +#define hardware_disable(_slot_) /* No hardware to disable */
> +
> +#endif /* CONFIG_RPXCLASSIC */
> +
> +/* FADS Boards from Motorola                                               */
> +
> +#if defined(CONFIG_FADS)
> +
> +#define PCMCIA_BOARD_MSG "FADS"
> +
> +static int voltage_set(int slot, int vcc, int vpp)
> +{
> +	uint reg = 0;
> +
> +	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 */
> +	*((uint *)BCSR1) &= ~(BCSR1_PCCVCC_MASK | BCSR1_PCCVPP_MASK);
> +
> +	/* enable new powersettings */
> +	*((uint *)BCSR1) |= reg;

ditto

> +	return 0;
> +}
> +
> +#define socket_get(_slot_) PCMCIA_SOCKET_KEY_5V
> +
> +static void hardware_enable(int slot)
> +{
> +	*((uint *)BCSR1) &= ~BCSR1_PCCEN;
> +}

ditto

> +static void hardware_disable(int slot)
> +{
> +	*((uint *)BCSR1) |=  BCSR1_PCCEN;
> +}

etc.


> +/* ------------------------------------------------------------------------- */
> +/* Motorola MBX860                                                           */
> +
> +#if defined(CONFIG_MBX)
> +
> +#define PCMCIA_BOARD_MSG "MBX"
> +
> +static int voltage_set(int slot, int vcc, int vpp)
> +{
> +	unsigned char reg = 0;
> +
> +	switch(vcc) {
> +		case 0:
> +			break;
> +		case 33:
> +			reg |= CSR2_VCC_33;
> +			break;
> +		case 50:
> +			reg |= CSR2_VCC_50;
> +			break;
> +		default:
> +			return 1;
> +	}
> +
> +	switch(vpp) {
> +		case 0:
> +			break;
> +		case 33:
> +		case 50:
> +			if(vcc == vpp)
> +				reg |= CSR2_VPP_VCC;
> +			else
> +				return 1;
> +			break;
> +		case 120:
> +			if ((vcc == 33) || (vcc == 50))
> +				reg |= CSR2_VPP_12;
> +			else
> +				return 1;
> +		default:
> +			return 1;
> +	}
> +
> +	/* first, turn off all power */
> +	*((unsigned char *)MBX_CSR2_ADDR) &= ~(CSR2_VCC_MASK | CSR2_VPP_MASK);
> +
> +	/* enable new powersettings */
> +	*((unsigned char *)MBX_CSR2_ADDR) |= reg;

ditto.

also, use u8 not unsigned char.


> +	return 0;
> +}
> +
> +#define socket_get(_slot_) PCMCIA_SOCKET_KEY_5V
> +#define hardware_enable(_slot_)  /* No hardware to enable */
> +#define hardware_disable(_slot_) /* No hardware to disable */
> +
> +#endif /* CONFIG_MBX */
> +
> +#if defined(CONFIG_PRxK)
> +#include <asm/cpld.h>
> +extern volatile fpga_pc_regs *fpga_pc;
> +
> +#define PCMCIA_BOARD_MSG "MPC855T"
> +
> +static int voltage_set(int slot, int vcc, int vpp)
> +{
> +	unsigned char reg = 0;
> +	unsigned char regread;
> +	cpld_regs *ccpld = get_cpld();
> +
> +	switch(vcc) {
> +		case 0:
> +			break;
> +		case 33:
> +			reg |= PCMCIA_VCC_33;
> +			break;
> +		case 50:
> +			reg |= PCMCIA_VCC_50;
> +			break;
> +		default:
> +			return 1;
> +	}
> +
> +	switch(vpp) {
> +		case 0:
> +			break;
> +		case 33:
> +		case 50:
> +			if(vcc == vpp)
> +				reg |= PCMCIA_VPP_VCC;
> +			else
> +				return 1;
> +			break;
> +		case 120:
> +			if ((vcc == 33) || (vcc == 50))
> +				reg |= PCMCIA_VPP_12;
> +			else
> +				return 1;
> +		default:
> +			return 1;
> +	}
> +
> +	reg = reg >> (slot << 2);
> +	regread = ccpld->fpga_pc_ctl;
> +	if (reg != (regread & ((PCMCIA_VCC_MASK | PCMCIA_VPP_MASK) >> (slot << 2)))) {
> +		/* enable new powersettings */
> +		regread = regread & ~((PCMCIA_VCC_MASK | PCMCIA_VPP_MASK) >> (slot << 2));
> +		ccpld->fpga_pc_ctl = reg | regread;
> +		mdelay(100);

should be msleep() AFAICS


> +	return 0;
> +}
> +
> +#define socket_get(_slot_) PCMCIA_SOCKET_KEY_LV
> +#define hardware_enable(_slot_)  /* No hardware to enable */
> +#define hardware_disable(_slot_) /* No hardware to disable */
> +
> +#endif /* CONFIG_PRxK */
> +
> +static void m8xx_shutdown(void)
> +{
> +	u_int m, i;
> +	pcmcia_win_t *w;
> +
> +	for(i = 0; i < PCMCIA_SOCKETS_NO; i++){
> +		w = (void *) &((immap_t *)IMAP_ADDR)->im_pcmcia.pcmc_pbr0;
> +
> +		((immap_t *)IMAP_ADDR)->im_pcmcia.pcmc_pscr = 
> +			M8XX_PCMCIA_MASK(i); 
> +		((immap_t *)IMAP_ADDR)->im_pcmcia.pcmc_per 
> +			&= ~M8XX_PCMCIA_MASK(i);
> + 
> +		/* turn off interrupt and disable CxOE */
> +		M8XX_PGCRX(i) = M8XX_PGCRX_CXOE;
> +		
> +		/* turn off memory windows */
> +		for(m = 0; m < PCMCIA_MEM_WIN_NO; m++) {
> +			w->or = 0;  /* set to not valid */
> +			w++;
> +		}
> +		
> +		/* turn off voltage */
> +		voltage_set(i, 0, 0);
> +		
> +		/* disable external hardware */
> +		hardware_disable(i);
> +	}
> +
> +	free_irq(pcmcia_schlvl, NULL);

don't you want to free_irq() first?



> +}
> +
> +/* copied from tcic.c */
> +
> +static int m8xx_drv_suspend(struct device *dev, u32 state, u32 level)
> +{
> +        int ret = 0;
> +        if (level == SUSPEND_SAVE_STATE)
> +                ret = pcmcia_socket_dev_suspend(dev, state);
> +        return ret;
> +}
> +                                                                                       
> +static int m8xx_drv_resume(struct device *dev, u32 level)
> +{
> +        int ret = 0;
> +        if (level == RESUME_RESTORE_STATE)
> +                ret = pcmcia_socket_dev_resume(dev);
> +        return ret;
> +}
> +                                                                                       
> +static struct device_driver m8xx_driver = {
> +        .name = "m8xx-pcmcia",
> +        .bus = &platform_bus_type,
> +        .suspend = m8xx_drv_suspend,
> +        .resume = m8xx_drv_resume,
> +};
> +
> +static struct platform_device m8xx_device = {
> +        .name = "m8xx-pcmcia",
> +        .id = 0,
> +};
> +
> +static u_int pending_events[PCMCIA_SOCKETS_NO];
> +static spinlock_t pending_event_lock = SPIN_LOCK_UNLOCKED;
> +
> +static irqreturn_t m8xx_interrupt(int irq, void *dev, struct pt_regs *regs)
> +{
> +	socket_info_t *s;
> +	event_table_t *e;
> +	u_int i, events, pscr, pipr, per;
> +
> +	dprintk("Interrupt!\n");
> +	/* get interrupt sources */
> +
> +	pscr = ((immap_t *)IMAP_ADDR)->im_pcmcia.pcmc_pscr;
> +	pipr = ((immap_t *)IMAP_ADDR)->im_pcmcia.pcmc_pipr;
> +	per = ((immap_t *)IMAP_ADDR)->im_pcmcia.pcmc_per;
> +
> +	for(i = 0; i < PCMCIA_SOCKETS_NO; i++) {
> +		s = &socket[i];
> +		e = &s->events[0]; 
> +		events = 0;
> +
> +		while(e->regbit) {
> +			if(pscr & e->regbit)
> +				events |= e->eventbit;
> +	    
> +				e++;
> +		}
> +
> +		/* 
> +		 * report only if both card detect signals are the same 
> +		 * not too nice done, 
> +		 * we depend on that CD2 is the bit to the left of CD1...
> +		 */
> +		if(events & SS_DETECT)
> +			if(((pipr & M8XX_PCMCIA_CD2(i)) >> 1) ^
> +				(pipr & M8XX_PCMCIA_CD1(i)))
> +			{
> +				events &= ~SS_DETECT;
> +			}
> +
> +#ifdef PCMCIA_GLITCHY_CD
> +		/*
> +		 * I've experienced CD problems with my ADS board.
> +		 * We make an extra check to see if there was a
> +		 * real change of Card detection.
> +		 */
> +	  
> +		if((events & SS_DETECT) && 
> +		   ((pipr &
> +		     (M8XX_PCMCIA_CD2(i) | M8XX_PCMCIA_CD1(i))) == 0) &&
> +		   (s->state.Vcc | s->state.Vpp)) {
> +			events &= ~SS_DETECT;
> +			/*printk( "CD glitch workaround - CD = 0x%08x!\n",
> +				(pipr & (M8XX_PCMCIA_CD2(i) 
> +					 | M8XX_PCMCIA_CD1(i))));*/
> +		}
> +#endif
> +	  
> +		/* call the handler */
> +
> +		dprintk("slot %u: events = 0x%02x, pscr = 0x%08x, "
> +			"pipr = 0x%08x\n", 
> +			i, events, pscr, pipr);
> +  
> +		if(events) {
> +			spin_lock(&pending_event_lock);
> +			pending_events[i] |= events;
> +			spin_unlock(&pending_event_lock);
> +			/* 
> +			 * Turn off RDY_L bits in the PER mask on 
> +			 * CD interrupt receival.
> +			 *
> +			 * They can generate bad interrupts on the
> +			 * ACS4,8,16,32.   - marcelo
> +			 */
> +			per &= ~M8XX_PCMCIA_RDY_L(0);
> +			per &= ~M8XX_PCMCIA_RDY_L(1);
> +
> +			((immap_t *)IMAP_ADDR)->im_pcmcia.pcmc_per = per;
> +
> +			if (events)
> +				pcmcia_parse_events(&socket[i].socket, events);
> +		}
> +	}
> +	
> +	/* clear the interrupt sources */
> +	((immap_t *)IMAP_ADDR)->im_pcmcia.pcmc_pscr = pscr;

more direct memory writes which should be foo_writel()


> +	dprintk("Interrupt done.\n");
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int m8xx_suspend(struct pcmcia_socket *sock)
> +{
> +	return m8xx_set_socket(sock, &dead_socket);
> +}
> +
> +static struct pccard_operations m8xx_services = {
> +	.init	= m8xx_sock_init,	
> +	.suspend = m8xx_suspend,
> +	.get_status = m8xx_get_status,
> +	.get_socket = m8xx_get_socket,
> +	.set_socket = m8xx_set_socket,
> +	.set_io_map = m8xx_set_io_map,
> +	.set_mem_map = m8xx_set_mem_map,
> +};
> +
> +static int __init m8xx_init(void)
> +{
> +	pcmcia_win_t *w;
> +	u_int i,m;
> +
> +	pcmcia_info("%s\n", version);
> +
> +	if (driver_register(&m8xx_driver))
> +		return -1;
> +
> +	pcmcia_info(PCMCIA_BOARD_MSG " using " PCMCIA_SLOT_MSG 
> +		    " with IRQ %u.\n", pcmcia_schlvl); 
> +
> +	/* Configure Status change interrupt */
> +
> +	if(request_irq(pcmcia_schlvl, m8xx_interrupt, 0, 
> +			  "m8xx_pcmcia", NULL)) {
> +		pcmcia_error("Cannot allocate IRQ %u for SCHLVL!\n", 
> +			     pcmcia_schlvl);
> +		return -1;
> +	}
> +
> +	w = (void *) &((immap_t *)IMAP_ADDR)->im_pcmcia.pcmc_pbr0;
> +
> +	((immap_t *)IMAP_ADDR)->im_pcmcia.pcmc_pscr = 
> +		M8XX_PCMCIA_MASK(0)| M8XX_PCMCIA_MASK(1); 
> +	((immap_t *)IMAP_ADDR)->im_pcmcia.pcmc_per 
> +		&= ~(M8XX_PCMCIA_MASK(0)| M8XX_PCMCIA_MASK(1));

ditto etc.