Hi,
Arnd Bergmann wrote on 2011-11-23:
> On Tuesday 08 November 2011, Nikolaus Voss wrote:
>
>
> > +
> > +static unsigned at91_twi_read(struct at91_twi_dev *dev, unsigned reg)
> > +{
> > + return __raw_readl(dev->base + reg);
> > +}
> > +
> > +static void at91_twi_write(struct at91_twi_dev *dev, unsigned reg, unsigned
> val)
> > +{
> > + __raw_writel(val, dev->base + reg);
> > +}
>
> Better use readl/writel or readl_relaxed/writel_relaxed.
ok, changed.
>
> > +/*
> > + * Calculate and set symmetric clock as stated in datasheet:
> > + * twi_clk = F_MAIN / (2 * cdiv * (1 << ckdiv) + 4)
> > + */
> > +static void __devinit at91_set_twi_clock(struct at91_twi_dev *dev, int
> twi_clk)
> > +{
> > + const int div = DIV_ROUND_UP(clk_get_rate(dev->clk), 2 * twi_clk) - 2;
> > + int ckdiv = fls(div >> 8);
> > + const int cdiv = div >> ckdiv;
> > +
> > + if (cpu_is_at91rm9200() && (ckdiv > 5)) {
> > + dev_warn(dev->dev, "AT91RM9200 erratum 22: using ckdiv = 5.\n");
> > + ckdiv = 5;
> > + }
>
> Don't check a global property like this locally in the driver. Instead,
> make it a property of the device that you check here and set it based on
> the the device name set by the platform, or by a device tree property.
Yes, this is a known problem and was discussed in
https://lkml.org/lkml/2011/11/8/217 . The main problem is that there a no
revisions for specific at91 IP devices but the revision is implicitly
defined by the cpu type and version. Changing this would need to add some
arch infrastructure which I think is beyond the scope of this driver.
> > diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h
> > new file mode 100644
> > index 0000000..a898159
> > --- /dev/null
> > +++ b/drivers/i2c/busses/i2c-at91.h
> > @@ -0,0 +1,80 @@
> > +
> > +#ifndef AT91_TWI_H
> > +#define AT91_TWI_H
> > +
>
> No need for a header file if all the values in it are used in only one
> source file. Just move everything to i2c_at91.c
>
> > +#define AT91_TWI_MMR 0x04 /* Master Mode Register */
> > +#define AT91_TWI_IADRSZ (3 << 8) /* Internal Device Address
> > + * Size */
> > +#define AT91_TWI_IADRSZ_NO (0 << 8)
> > +#define AT91_TWI_IADRSZ_1 (1 << 8)
> > +#define AT91_TWI_IADRSZ_2 (2 << 8)
> > +#define AT91_TWI_IADRSZ_3 (3 << 8)
> > +#define AT91_TWI_MREAD (1 << 12) /* Master Read Direction
> */
> > +#define AT91_TWI_DADR (0x7f << 16) /* Device Address */
>
> These are harder to read than just using hexadecimal bitmasks:
>
> #define AT91_TWI_MMR 0x00000004
> #define AT91_TWI_IADRSZ 0x00000300
> #define AT91_TWI_IADRSZ_NO 0x00000000
> #define AT91_TWI_IADRSZ_1 0x00000100
> ...
I agree, but this header file was already used by the old driver and
converting would add possible errors to register definitions which are
not (yet) used. That's why I've left it as is and just made it a local
include.
Thanks,
Niko
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
