Well, the full path to this directory is os/linux-2.6/arch/cris, starting from devboard-R2_01.
I forgot to add "os/linux-2.6" in my explanation(s), maybe that was misleading you? Best rgds, --Geert --- In [email protected], "J R" <[EMAIL PROTECTED]> wrote: > > hello , > i don't have /arch/cris on my debian with fox sdk intalled ! > what is the problem ? > thank a lot ! > > > 2006/11/23, Geert Vancompernolle <[EMAIL PROTECTED]>: > > > > Sure John, > > > > You are lucky, because I was about to delete this email in my email > > box, knowing it was anyhow on the FoxBoard website... :-) > > > > Here is the email content you sent me. > > > > And thanks very much for the time you spent on this. It was > > absolutely not my intention to give critics, I just studied the code > > quite a bit and wanted to share my findings with the community. This > > way, I also learn from others how to do things right. > > > > And as you said, nobody is perfect, even not me... ;-) > > > > I think I will "develop" my own I2C driver, but then heavily based on > > yours. Without the LCD stuff, however, because I want it really > > separated. > > > > It might be I'm making a driver for every I2C device too, at least if > > it's a "special" case. > > > > Best rgds, > > > > --Geert > > > > > 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)? > > > > because i was lazy > > > > > > 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? > > > > because the rtc does not work otherwise > > > > > > 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? > > > > the last bit is 0 on reading and 1 on writing..... every device has 2 > > seperate adds ... one for read ops and one for write ops > > > > > > 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. > > > > i'll check it... possible you found a bug :) > > > > > > 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? > > > > because i use a hd44780 attached to a pcf8574... i could have made a > > seperate driver... (see answer to 1.) > > > > > > 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. > > > > this is how i2c works.... check out the philips site for more detailed > > info > > > > > > 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... > > > > most of the drivers use there own delay.... you know, nobody is perfect > > :) > > > > oh my god.... how many more ?? :-) > > > > 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. > > i cant remember why, but there was a reason.... has to do with 1-n cases > > using the same functions with different parameters... summin like > > that... but i remember that i did it for a reason > > > > > > 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()... > > > > no idea without looking at the code... it basically a macro used to tell > > the kernel that its the init function > > > > > > Feel free to react! > > i just did :) > > > > John > > > > --- In [email protected] <foxboard%40yahoogroups.com>, "blogic79" > > <john@> wrote: > > > > > > 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] <foxboard%40yahoogroups.com>, "Geert > > Vancompernolle" > > > <geert.vancompernolle@> 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 > > > > > > > > > > > > > >
