Re: [PATCH] MPC8xx PCMCIA driver

[Jeff Garzik <jgarzik@pobox.com>]

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/