Messages in this thread | | | Date | Tue, 30 Aug 2005 00:33:00 -0400 | From | Jeff Garzik <> | Subject | Re: [PATCH] MPC8xx PCMCIA driver |
| |
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |