On Mon, Aug 29, 2005 at 11:39:02PM -0400, Jeff Garzik wrote:

Marcelo Tosatti wrote:

quoted

+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().

The memory map structure which contains device configuration/registers
is _always_ directly mapped with pte's (the 8xx is a chip with builtin
UART/network/etc functionality).

I don't think there is a need to use read/write acessors.

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

OK, will fix the types and address the rest of your comments.

Thanks!