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"
<[EMAIL PROTECTED]> 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
> >
>
