hi geert, i think i accidentally answered to your email not to the list last night... can you forward my answer to the list please ?
--- In [email protected], "Geert Vancompernolle" <[EMAIL PROTECTED]> wrote: > > Hi, > > I'm currently studying the driver written for I2C, located into > arch/cris/arch-v10/drivers (version of the Phrozen SDK, Linux kernel 2.6) > > So far, I have the following questions/remarks: > > 1. There's a macro, D(x), which is void while in the code there are > references to D(x) to do kernel prints. Why is the macro defined > being empty? Why has there not been made a "debug" and a "release" > version of D(x)? > > 2. I see that there's a special CONFIG_ETRAX_I2C_USES_PB_NOT_PB_I2C > definition. > Why is this? Is the HW wrt the I2C pins not reliable? Or is it just > in case those dedicated I2C pins are taken for other functionality? > > 3. It was not clear to me why in the readreg(_n) functions the first > slave address was send with the last bit of the address forcefully set > to 0 by means of the mask 0x0fe. > Later on, I realised that the name of the function was indeed telling > what it did: reading a certain register. > Because of that, you first have to write to the device which register > you want to read (hence, the "& 0xfe" mask that filtered out the > lowest bit of the slave address byte), followed by the register itself > and then a repeated start condition with the read bit set (hence, the > "| 0x01" mask that filtered "in" the lowest bit of the slave address). > However, is this explanation correct? > > 4. I've seen a mistake in the i2c_readreg_n() function. When the last > byte is read, the master should tell the slave that it doesn't want > any further bytes by not acknowledging the last byte. > However, this is the piece of code used for that: > > if ( length ) > { > i2c_sendnack(); > } > else > { > i2c_sendnack(); > }; > > If length != 0, then an ACK should be given, not a NACK. To me, this > is wrong. > > 5. Why is there a "mix" with LCD functionality in this driver? Anyone > any idea? Would it not have been better to separate those two > functionalities? > > 6. In the i2c_read_data() function, there's first a write of the slave > address with the RW bit cleared (so, writing), then followed by a > repeated start condition and then the slave address is again sent, but > now with the RW bit set (so, reading). > Why is this necessary? Isn't it enough to just send the slave address > with the RW bit set, without the repeated start condition? > Why the restart condition? > There's anyhow no extra byte sent between the first time the slave > address is sent and the repeated start condition, so to me this is > useless. > > 7. Since I addressed already the LCD part, here's one for the LCD: > There's a function called lcdDelay(), where some very bizarre things > are done. > However, there's a function available called mdelay() in > include/linux/delay.h. > Can't this function be used instead? Would be more transparent then > what is coded now. Just a suggestion... > > 8. In the function i2c_ioctl(), I see lots of "fall through" case > statements. Is this on purpose? Can hardly think it's the intention, > but rather something that's forgotten. > It's starting from "case I2C_WRITEDATA" till the end of this switch > construction. > > 9. The function i2c_init() has __init in front of it. > Since i2c_init() is called from i2c_register() and since this function > is registered to be called during boot (via module_init( > i2c_register)), is there a need to have this __init in front of > i2c_init()? The __init is, however, needed in front of i2c_register()... > > So far my remarks. > > Some of them might be wrong, some of them might be correct. Hence, my > proposal to discuss it in this user forum. > > Feel free to react! > > Best rgds, > > --Geert >
