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
