Re: [PATCH] MPC8xx PCMCIA driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

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


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.

There are multiple reasons:

* Easier reviewing. One cannot easily distinguish between writing to normal kernel virtual memory and "magic" memory that produces magicaly side effects such as initiating DMA of a net packet.

* Compiler safety. As the code is written now, you have no guarantees that the compiler won't combine two stores to the same location, etc. Accessor macros are a convenient place to add compiler barriers or 'volatile' notations that the MPC8xx code lacks.

* Maintainable. foo_read[bwl] or foo_read{8,16,32} are preferred because that's the way other bus accessors look like -- yes even embedded SoC buses benefit from these code patterns. You want your driver to look like other drivers as much as possible.

* Convenience. The accessors can be a zero overhead memory read/write at a minimum. But they can also be convenient places to use special memory read/write instructions that specify uncached memop, compiler barriers, memory barriers, etc.

Regards,

	Jeff


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

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]
  Powered by Linux