Hello Joakim,
Joakim Tjernlund wrote:
> Heiko Schocher <h...@denx.de> wrote on 17/09/2009 08:00:34:
>
>> Hello Joakim,
>
> Hi Heiko
>
>> Joakim Tjernlund wrote:
>>> The latest AN2819 has changed the way FDR/DFSR should be calculated.
>>> Update the driver according to spec. However, Condition 2
>>> is not accounted for as it is not clear how to do so.
>> Thanks for your work, just some minor Codingstyle comments:
>>
>>> Signed-off-by: Joakim Tjernlund <joakim.tjernl...@transmode.se>
>>> ---
>>> drivers/i2c/fsl_i2c.c | 88
>>> +++++++++++++++++++++++++++++-------------------
>>> 1 files changed, 53 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/drivers/i2c/fsl_i2c.c b/drivers/i2c/fsl_i2c.c
>>> index 0c5f6be..0491a69 100644
>>> --- a/drivers/i2c/fsl_i2c.c
>>> +++ b/drivers/i2c/fsl_i2c.c
>>> @@ -100,29 +100,9 @@ static const struct fsl_i2c *i2c_dev[2] = {
>
>>> #ifdef __PPC__
>>> - u8 dfsr;
>>> + u8 dfsr, fdr = 0x31; /* Default if no FDR found */
>>> + unsigned short A, B, Ga, Gb;
>> Please do not use mixed-case variables, thanks.
>
> A and B are from the AN2819 spec and I used the same names to ease
> identify with the spec. I rather keep them.
I am fine with that, just please do not mix upper and
lower case in one variable name. So please use "ga" and
"gb" ...
>>> + unsigned long c_div, est_div;
>>> +
>>> #ifdef CONFIG_FSL_I2C_CUSTOM_DFSR
>>> - dfsr = CONFIG_FSL_I2C_CUSTOM_DFSR;
>>> + dfsr = CONFIG_FSL_I2C_CUSTOM_DFSR;
>>> #else
>>> - dfsr = fsl_i2c_speed_map[i].dfsr;
>>> -#endif
>>> - writeb(dfsr, &dev->dfsrr); /* set default filter */
>>> + /* Condition 1: dfsr <= 50/T */
>>> + dfsr = (5*(i2c_clk/1000))/(100000);
>> Please use one space around (on each side of) most binary
>> and ternary operators.
>
> Like so?
> dfsr = (5 * (i2c_clk / 1000)) / 100000);
Yep.
>>> #endif
>>> #ifdef CONFIG_FSL_I2C_CUSTOM_FDR
>>> - fdr = CONFIG_FSL_I2C_CUSTOM_FDR;
>>> - speed = i2c_clk / divider; /* Fake something */
>>> + fdr = CONFIG_FSL_I2C_CUSTOM_FDR;
>>> + speed = i2c_clk / divider; /* Fake something */
>>> #else
>>> + debug("Requested speed:%d, i2c_clk:%d\n", speed, i2c_clk);
>>> + if (!dfsr)
>>> + dfsr = 1;
>>> +
>>> + est_div = ~0;
>>> + for(Ga=0x4, A=10; A<=30; Ga++, A+=2) {
>> spaces her too.
> Like so?
> for(Ga = 0x4, A = 10; A <= 30; Ga++, A += 2) {
for (Ga = 0x4, A = 10; A <= 30; Ga++, A += 2) {
>>> + for (Gb=0; Gb<8; Gb++) {
>> and here too. Please check the whole patch.
>>
>>> + B = 16 << Gb;
>>> + c_div = B * (A + ((3*dfsr)/B)*2);
>>> + if (c_div > divider && c_div < est_div) {
>> Can we make
>>
>> if ((c_div > divider) && (c_div < est_div)) {
>
> Sure.
Thanks!
bye
Heiko
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
