On Sun, 10 Aug 2008 23:50:54 +0900, Paul Mundt wrote: > On Sun, Aug 10, 2008 at 04:29:31PM +0200, Jean Delvare wrote: > > On Thu, 31 Jul 2008 19:43:54 +0900, Paul Mundt wrote: > > > +static int iic_force_poll, iic_force_normal; > > > +static int iic_timeout = 1000, iic_read_delay = 0; > > > > There's a comment in the driver that says that this delay is needed for > > several devices. So I'm a bit surprised that the default value is 0. > > Shouldn't the default be such that the driver works on all devices by > > default? > > > The main issue is that it's highly device specific, not just platform > specific. On most of the references boards we've shipped to customers, > there's no need for a delay, so the default is simply to leave that off, > and to allow people to extend it if they start witnessing problems. > > Internally we have many different variations both of the board and the > FPGA, a lot of which have very differing timing characteristics. For the > general case a 0 or close to 0 read delay should be sufficient. > Originally I was going to just leave the delay at 250 or something like > that, but that failed to work for the boards that had problems, and > needlessly penalized the ones that didn't (and we have no way of > detecting from the platform if we're on a troublesome board or not). For > customer references that use a silicon option for the same controller, > there's no need for the delay. > > Obviously one thing we could try to do is do a few dummy reads and see if > any of them are aborted to try and auto-calculate a meaningful default > delay, but in general it's pretty much all over the place. Given that, I > think just leaving it configurable and documenting the problem cases > should be fine. I'll be the first person to get an email when there's a > problem anyways :-) > > > > + strlcpy(adap->name, "HL FPGA I2C adapter", I2C_NAME_SIZE); > > > > I2C_NAME_SIZE isn't the size of i2c_adapter.name. Use > > sizeof(adap->name) instead (as your original code was doing.) > > > Thanks, I'm not sure why that got changed, I'll switch it back. > > > > +MODULE_PARM_DESC(iic_force_poll, "Force polling mode"); > > > +MODULE_PARM_DESC(iic_force_normal, "Force normal mode (100 kHz), default > > > is fast mode (400 kHz)"); > > > +MODULE_PARM_DESC(iic_timeout, "Set timeout value in msecs (default 1000 > > > ms)"); > > > +MODULE_PARM_DESC(iic_read_delay, "Delay between data read cycles > > > (default 0 ms)"); > > > > In general I am curious why all these are module parameters instead of > > platform device attributes. Shouldn't the platform code know the > > correct values? If so, that would be less error prone than leaving it > > on the user to pass the right parameters to the module. > > > Hopefully that's answered above. Let me know if you have any other > concerns or suggestions.
OK. I've fixed the strlcpy size issue myself, together with a couple checkpatch warnings. The resulting patch is there: http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/i2c-renesas-highlander-fpga-smbus-support.patch It is queued for 2.6.28. -- Jean Delvare _______________________________________________ i2c mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/i2c
