[i2c] [PATCH] AT91RM9200 I2C bus driver

[Mark M. Hoffman]

Hi Andrew:

* Andrew Victor <andrew at sanpeople.com> [2006-07-04 17:40:54 +0200]:
> hi Mark,
> 
> > 2) The correct way to prepare for memory-mapped device IO is ioremap().
> 
> Eventually, but the core AT91RM9200 SoC code (in mainline) already has a
> static mapping for these peripherals.

Well, you take your chances with Greg KH on this one.  Unfortunately, I
don't know enough yet to tell you exactly what needs to change.

> > > +	at91_twi_write(AT91_TWI_IDR, 0x1ff);		/* Disable all interrupts */
> > 
> > Get rid of the magic number 0x1ff.  According to the datasheet it's
> > wrong anyway.
> 
> It's actually correct according to the original AT91RM9200 datasheet -
> that included support for Slave mode.  The newly release AT91SAM9260
> should also use this driver, and that does support Slave mode.
> 
> It's an Interrupt Disable register, so I will change it to 0xffffffff -
> that will "disable all interrupts" as the comment says.

OK, I guess.  I usually don't like to poke reserved bits like that, but
in this case it makes sense.

> > > +	BUG_ON(ckdiv > 5);	/* AT91RM9200 Errata #22 */
> > 
> > No:  return status so that you can fail the driver load.  Write something to
> > the kernel log while you're at it... but BUG_ON() is way too harsh for this.
> 
> I disagree.  It means they've manually edited the code to modify the
> TWI_CLOCK value, and have also specified a *very low* value.  It can't
> trigger any other way.

So you cause the driver load to fail, and that's it.  I haven't looked at
the errata, but either...

A) It's a system-wide errata, in which case it should be handled
elsewhere... i.e. you can't depend on this BUG_ON to catch it because
the user might not even load this driver.  OR...

B) The errata is specific to this peripheral, in which case killing the
whole system is obviously too harsh.  Just fail the driver load.

Is there some other possibility that I'm just not getting?

> > > +	clk_enable(twi_clk);			/* enable peripheral clock */
> > > +	at91_twi_hwinit();			/* initialize TWI controller */
> > 
> > Wouldn't it make more sense to move the clock enable into the hwinit?
> 
> Not really - there was discussion about having the ability to
> re-initialize the hardware on certain errors.

OK.

Regards,

-- 
Mark M. Hoffman
mhoffman at lightlink.com