Hi,
Arnd Bergmann wrote on 2011-11-24:
> On Thursday 24 November 2011, Voss, Nikolaus wrote:
> > >
> > > > +/*
> > > > + * 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.
>
> Aside from the flamewar in that thread, my impression is that in general
> people (me certainly) prefer to have driver-local workarounds be expressed
> in a driver specific way, not in a platform or architecture specific way
> because that makes the driver less portable.
I guess I see your point now. So you want something like pdev.has_bugX to be
set by mach setup and later check this flag in the 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.
>
> But you are presenting the driver as a new one, so you should be
> prepared to get review comments like any other new code.
>
> Please at least move the data into the main driver file to get rid of
> the header file.
I didn't want to appear ignorant about this, I actually appreciate your
comment. I just wanted to point out that there might be a reason to keep
the old file which you weren't aware of (because I presented this as a new
driver). So, I will move the register definitions to the main driver.
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
