[i2c] [PATCH] AT91RM9200 I2C bus driver

[Andrew Victor]

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.


> > +	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.


> > +	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.


> > +	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.


I'll prepare a new patch.


Regards,
  Andrew Victor