Hi Komal,
Your post ended up in my spam box once again... I really think you
should send your patches as text attachements (or inline) rather than
binary attachements. Using real names in To: and Cc: fields might help
as well.
> I have attached the updated patch, which addresses the most of review
> comments.
I'll review that new version later today, or tomorrow.
> >The comment is confusing, as this address is usually known as the
> >"slave address" in the I2C world. Masters don't need no address on an
> >I2C bus. "own" is not a very explicit parameter name, what about
> >"slave_addr"? Ideally this should be retrieved from platform_data too,
> >else you can't be sure you won't collide with a device on the bus.
>
> >"0 for default" doesn't make sense, as the default is, by definition,
> >when the user doesn't speficiy anything. That this is internally coded
> >as 0 is an implementation detail user-space doesn't need to know.
>
> Updated the comment and changed to slave_addr , and default is changed to "3".
Slightly better, though I still don't get why you worry setting an
address that will never be used.
> >> + if (armxor_rate > 16000000)
> >> + psc = (armxor_rate + 8000000) / 12000000;
> >> + else
> >> + psc = 0;
>
> >Can you please explain this formula?
>
> The OMAP core uses 8-bit value to divide the system clock (SCLK) and
> generates its own sampling clock (ICLK), and the core logic is sampled
> at clock rate of the system clock for the module, divided by (prescaler value + 1)
I should have been more precise, I guess. What surprises me are the
numbers themselves. It's frequent to see forumlae of the form
"a = (b + c/2) / c" to divide with proper rounding, but here you have
2c/3 instread of c/2. My question was more like: is it intentional, or a
typo? Also, with the code above, psc will never have value 1. The "if"
part will always compute to at least 2, and the "else" part to 0. Is
this OK?
> I think it is better to remove those lines and return error if length is zero.
> Is that ok?
Yes. This can be revisited later when/if someone finds a hack to work
around the problem.
> >> + /* We have an error */
> >> + if (dev->cmd_err & OMAP_I2C_STAT_NACK) {
> >> + if (msg->flags & I2C_M_IGNORE_NAK)
> >> + return 0;
>
> >Couldn't you have other error bits set as well? I2C_M_IGNORE_NAK means
> >you can ignore OMAP_I2C_STAT_NACK, not other errors.
>
> This is now being handled by first checking remaining errors first and then
> NACK. Is that ok?
Yes.
> >> + r = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG) & 0xff;
> >> + dev_info(dev->dev, "bus %d rev%d.%d at %d kHz\n",
> >> + pdev->id - 1, r >> 4, r & 0xf, clock);
>
> >This "- 1" is error prone IMHO.
>
> Only if omap devices.c maintainer pushes the values less than one in device
> structure ;)
No, what I meant was rather that printing a bus number which differs
from the internal numbering might confuse the user at some point.
--
Jean Delvare
Hi,
* Jean Delvare <[email protected]> [060805 11:31]:
>
> > >> + if (armxor_rate > 16000000)
> > >> + psc = (armxor_rate + 8000000) / 12000000;
> > >> + else
> > >> + psc = 0;
> >
> > >Can you please explain this formula?
> >
> > The OMAP core uses 8-bit value to divide the system clock (SCLK) and
> > generates its own sampling clock (ICLK), and the core logic is sampled
> > at clock rate of the system clock for the module, divided by (prescaler value + 1)
>
> I should have been more precise, I guess. What surprises me are the
> numbers themselves. It's frequent to see forumlae of the form
> "a = (b + c/2) / c" to divide with proper rounding, but here you have
> 2c/3 instread of c/2. My question was more like: is it intentional, or a
> typo? Also, with the code above, psc will never have value 1. The "if"
> part will always compute to at least 2, and the "else" part to 0. Is
> this OK?
>
Hmmm, this sounds like a bug somewhere. TRM for 5912 says the I2C clock
must be prescaled to be between 7 - 12 MHz [1]. The XOR input clock is
typically 12, 13 or 19.2 MHz. So we should have code that produces:
XOR Mhz Divider Prescaler
12 1 0
13 2 1
19.2 2 1
Then again the original old code produces something different too [2]...
I suspect the original code had some hw workarounds and and later code
may have a conversion bug somewhere :)
I suggest we keep the code as is for now since it's known to work on
all omaps, and then submit a follow-up patch later once we have
verified that that code based on the TRM works on all omaps.
Regards,
Tony
[1] http://focus.ti.com/general/docs/lit/getliterature.tsp?baseLiteratureNumber=spru681
[2] http://linux-omap.bkbits.net:8080/main/diffs/drivers/i2c/busses/[email protected]?nav=index.html|src/|src/drivers|src/drivers/i2c|src/drivers/i2c/busses|hist/drivers/i2c/busses/i2c-omap.c
--- Tony Lindgren <[email protected]> wrote:
> Hi,
>
> * Jean Delvare <[email protected]> [060805 11:31]:
> >
> > > >> + if (armxor_rate > 16000000)
> > > >> + psc = (armxor_rate + 8000000) / 12000000;
> > > >> + else
> > > >> + psc = 0;
> > >
> > > >Can you please explain this formula?
> > >
> > > The OMAP core uses 8-bit value to divide the system clock (SCLK)
> and
> > > generates its own sampling clock (ICLK), and the core logic is
> sampled
> > > at clock rate of the system clock for the module, divided by
> (prescaler value + 1)
> >
> > I should have been more precise, I guess. What surprises me are the
> > numbers themselves. It's frequent to see forumlae of the form
> > "a = (b + c/2) / c" to divide with proper rounding, but here you
> have
> > 2c/3 instread of c/2. My question was more like: is it intentional,
> or a
> > typo? Also, with the code above, psc will never have value 1. The
> "if"
> > part will always compute to at least 2, and the "else" part to 0.
> Is
> > this OK?
> >
>
> Hmmm, this sounds like a bug somewhere. TRM for 5912 says the I2C
> clock
> must be prescaled to be between 7 - 12 MHz [1]. The XOR input clock
> is
> typically 12, 13 or 19.2 MHz. So we should have code that produces:
>
> XOR Mhz Divider Prescaler
> 12 1 0
> 13 2 1
> 19.2 2 1
>
> Then again the original old code produces something different too
> [2]...
>
> I suspect the original code had some hw workarounds and and later
> code
> may have a conversion bug somewhere :)
>
> I suggest we keep the code as is for now since it's known to work on
> all omaps, and then submit a follow-up patch later once we have
> verified that that code based on the TRM works on all omaps.
I have updated the driver with all other review comments except comment
on formula above by Jean. I can some part of description from Tony's
explanation along with removing the following code line
/* Set Own Address */
omap_i2c_write_reg(dev, OMAP_I2C_OA_REG, dev->own_address);
As it is not needed. Please confirm so that I can send the revised
patch soon.
---Komal Shah
http://komalshah.blogspot.com/
__________________________________________________
Do You Yahoo!?
Tired of spam? Yahoo! Mail has the best spam protection around
http://mail.yahoo.com
Hi Komal,
> > Hmmm, this sounds like a bug somewhere. TRM for 5912 says the I2C
> > clock must be prescaled to be between 7 - 12 MHz [1]. The XOR input
> > clock is typically 12, 13 or 19.2 MHz. So we should have code that
> > produces:
> >
> > XOR Mhz Divider Prescaler
> > 12 1 0
> > 13 2 1
> > 19.2 2 1
> >
> > Then again the original old code produces something different too
> > [2]...
> >
> > I suspect the original code had some hw workarounds and and later
> > code
> > may have a conversion bug somewhere :)
> >
> > I suggest we keep the code as is for now since it's known to work on
> > all omaps, and then submit a follow-up patch later once we have
> > verified that that code based on the TRM works on all omaps.
>
> I have updated the driver with all other review comments except comment
> on formula above by Jean. I can some part of description from Tony's
> explanation along with removing the following code line
>
> /* Set Own Address */
> omap_i2c_write_reg(dev, OMAP_I2C_OA_REG, dev->own_address);
>
> As it is not needed. Please confirm so that I can send the revised
> patch soon.
Fine with me. If you remove that line, there are a few others you
should be able to remove as well (declaration of slave_add module
parameter, definition of DEFAULT_OWN, etc.)
Waiting for your updated patch :) Thanks for all the changes you
accepted to make, BTW. I believe the driver is in a better shape now.
--
Jean Delvare
Hi Tony, Komal,
> Hmmm, this sounds like a bug somewhere. TRM for 5912 says the I2C clock
> must be prescaled to be between 7 - 12 MHz [1]. The XOR input clock is
> typically 12, 13 or 19.2 MHz. So we should have code that produces:
>
> XOR Mhz Divider Prescaler
> 12 1 0
> 13 2 1
> 19.2 2 1
Not that 13 MHz cannot actually be prescaled between 7 and 12 MHz, no
matter how you look at it.
> Then again the original old code produces something different too [2]...
>
> I suspect the original code had some hw workarounds and and later code
> may have a conversion bug somewhere :)
>
> I suggest we keep the code as is for now since it's known to work on
> all omaps, and then submit a follow-up patch later once we have
> verified that that code based on the TRM works on all omaps.
I've now taken Komal's patch (#4). Here is a proposed patch which brings
the prescaler computation formula in line with your comment and table
above. It could be applied on top of Komal's patch unless it causes a
problem on some of the OMAP systems. For XOR = 13 MHz, it changes the
prescaler from 0 to 1. For XOR = 19.2 MHz it changes the prescaler from
2 to 1.
I don't have any hardware to test it, though. If it happens to be
better to be slightly over 12 MHz than slightly below 7 MHz, the
"> 12000000" condition below can be replaced with "> 14000000".
i2c: Fix OMAP clock prescaler to match the comment
Signed-off-by: Jean Delvare <[email protected]>
---
drivers/i2c/busses/i2c-omap.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
--- linux-2.6.18-rc4.orig/drivers/i2c/busses/i2c-omap.c 2006-08-10 09:56:54.000000000 +0200
+++ linux-2.6.18-rc4/drivers/i2c/busses/i2c-omap.c 2006-08-10 10:12:03.000000000 +0200
@@ -231,8 +231,8 @@
* 13 2 1
* 19.2 2 1
*/
- if (fclk_rate > 16000000)
- psc = (fclk_rate + 8000000) / 12000000;
+ if (fclk_rate > 12000000)
+ psc = fclk_rate / 12000000;
}
/* Setup clock prescaler to obtain approx 12MHz I2C module clock: */
--
Jean Delvare
* Jean Delvare <[email protected]> [060810 11:30]:
> Hi Tony, Komal,
>
> > Hmmm, this sounds like a bug somewhere. TRM for 5912 says the I2C clock
> > must be prescaled to be between 7 - 12 MHz [1]. The XOR input clock is
> > typically 12, 13 or 19.2 MHz. So we should have code that produces:
> >
> > XOR Mhz Divider Prescaler
> > 12 1 0
> > 13 2 1
> > 19.2 2 1
>
> Not that 13 MHz cannot actually be prescaled between 7 and 12 MHz, no
> matter how you look at it.
True :) But that's what the docs say..
> > Then again the original old code produces something different too [2]...
> >
> > I suspect the original code had some hw workarounds and and later code
> > may have a conversion bug somewhere :)
> >
> > I suggest we keep the code as is for now since it's known to work on
> > all omaps, and then submit a follow-up patch later once we have
> > verified that that code based on the TRM works on all omaps.
>
> I've now taken Komal's patch (#4). Here is a proposed patch which brings
> the prescaler computation formula in line with your comment and table
> above. It could be applied on top of Komal's patch unless it causes a
> problem on some of the OMAP systems. For XOR = 13 MHz, it changes the
> prescaler from 0 to 1. For XOR = 19.2 MHz it changes the prescaler from
> 2 to 1.
OK cool. As far as I'm concerned, I'm fine with it too:
Signed-off-by: Tony Lindgren <[email protected]>
> I don't have any hardware to test it, though. If it happens to be
> better to be slightly over 12 MHz than slightly below 7 MHz, the
> "> 12000000" condition below can be replaced with "> 14000000".
Thanks, we'll test it on various omaps and let you know if it works.
Tony
Hi Tony, all,
On Thu, 10 Aug 2006 16:19:26 +0300, Tony Lindgren wrote:
> * Jean Delvare <[email protected]> [060810 11:30]:
> > I've now taken Komal's patch (#4). Here is a proposed patch which brings
> > the prescaler computation formula in line with your comment and table
> > above. It could be applied on top of Komal's patch unless it causes a
> > problem on some of the OMAP systems. For XOR = 13 MHz, it changes the
> > prescaler from 0 to 1. For XOR = 19.2 MHz it changes the prescaler from
> > 2 to 1.
>
> OK cool. As far as I'm concerned, I'm fine with it too:
> Signed-off-by: Tony Lindgren <[email protected]>
>
> > I don't have any hardware to test it, though. If it happens to be
> > better to be slightly over 12 MHz than slightly below 7 MHz, the
> > "> 12000000" condition below can be replaced with "> 14000000".
>
> Thanks, we'll test it on various omaps and let you know if it works.
Any news on this? I still have this patch in my local tree. Should I
push it into Linux 2.6.20?
i2c: Fix OMAP clock prescaler to match the comment
Signed-off-by: Tony Lindgren <[email protected]>
Signed-off-by: Jean Delvare <[email protected]>
---
drivers/i2c/busses/i2c-omap.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
--- linux-2.6.18-rc4.orig/drivers/i2c/busses/i2c-omap.c 2006-08-10 09:56:54.000000000 +0200
+++ linux-2.6.18-rc4/drivers/i2c/busses/i2c-omap.c 2006-08-10 10:12:03.000000000 +0200
@@ -231,8 +231,8 @@
* 13 2 1
* 19.2 2 1
*/
- if (fclk_rate > 16000000)
- psc = (fclk_rate + 8000000) / 12000000;
+ if (fclk_rate > 12000000)
+ psc = fclk_rate / 12000000;
}
/* Setup clock prescaler to obtain approx 12MHz I2C module clock: */
--
Jean Delvare
Hi,
* Jean Delvare <[email protected]> [061204 09:49]:
> Hi Tony, all,
>
> On Thu, 10 Aug 2006 16:19:26 +0300, Tony Lindgren wrote:
> > * Jean Delvare <[email protected]> [060810 11:30]:
> > > I've now taken Komal's patch (#4). Here is a proposed patch which brings
> > > the prescaler computation formula in line with your comment and table
> > > above. It could be applied on top of Komal's patch unless it causes a
> > > problem on some of the OMAP systems. For XOR = 13 MHz, it changes the
> > > prescaler from 0 to 1. For XOR = 19.2 MHz it changes the prescaler from
> > > 2 to 1.
> >
> > OK cool. As far as I'm concerned, I'm fine with it too:
> > Signed-off-by: Tony Lindgren <[email protected]>
> >
> > > I don't have any hardware to test it, though. If it happens to be
> > > better to be slightly over 12 MHz than slightly below 7 MHz, the
> > > "> 12000000" condition below can be replaced with "> 14000000".
> >
> > Thanks, we'll test it on various omaps and let you know if it works.
>
> Any news on this? I still have this patch in my local tree. Should I
> push it into Linux 2.6.20?
>
> i2c: Fix OMAP clock prescaler to match the comment
>
> Signed-off-by: Tony Lindgren <[email protected]>
> Signed-off-by: Jean Delvare <[email protected]>
> ---
> drivers/i2c/busses/i2c-omap.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> --- linux-2.6.18-rc4.orig/drivers/i2c/busses/i2c-omap.c 2006-08-10 09:56:54.000000000 +0200
> +++ linux-2.6.18-rc4/drivers/i2c/busses/i2c-omap.c 2006-08-10 10:12:03.000000000 +0200
> @@ -231,8 +231,8 @@
> * 13 2 1
> * 19.2 2 1
> */
> - if (fclk_rate > 16000000)
> - psc = (fclk_rate + 8000000) / 12000000;
> + if (fclk_rate > 12000000)
> + psc = fclk_rate / 12000000;
> }
>
> /* Setup clock prescaler to obtain approx 12MHz I2C module clock: */
Sorry for the delay in replying. Yes, it's safe to push. When the original
code was done, the max limit of 12MHz was ignored.
Regards,
Tony