i2c handling in nouveau driver

[Jean Delvare]

Hi Ben,

I see in commit f553b79c03f0dbd52f6f03abe8233a2bef8cbd0d that you just 
changed the nouveau driver to use an internal i2c bit-banging 
implementation instead of i2c-algo-bit. Let me say here publicly that I 
disapprove this change. I believe this is a move in the wrong direction. 
Just because doing so fixed one bug you were seeing doesn't make it 
right.

If i2c-algo-bit has problems, please report them to the maintainer (i.e. 
me) and have them fixed. If you have problems with it, others may do as 
well. If everyone stops using common pieces of code as soon as they have 
a problem, we will end up with a lot of duplicated code in the kernel, 
which is bad in many respects.

In the commit message, you complain about the use of cond_resched() in 
i2c-algo-bit. You might as well be right, maybe it should be removed. 
It's been there pretty much forever (February 2002 at least) and while I 
can understand why it was put there, I would agree it isn't necessarily 
a good idea. Did you try removing it to see if it solved your problem?

There is also a call to yield() somewhere else in the i2c-algo-bit 
driver. I remember having a discussion about it long ago, someone 
proposed to change it to cond_resched(), but it never happened. I admit 
I don't know the difference between cond_resched() and yield() so I 
can't really make a decision until someone explains it to me.

I see that your reimplementation uses:

#define T_HOLD     5000

which basically means you're running the I2C bus at ~100 kHz, while the 
original code had:

port->bit.udelay = 40;

which basically ran the bus at ~12.5 kHz, i.e. only a tad faster than 
the low limit allowed by SMBus. I warned some times ago that such low 
speeds could be problematic:

http://lists.freedesktop.org/archives/dri-devel/2011-October/015512.html

I even posted a patch:

http://lists.freedesktop.org/archives/dri-devel/2011-October/015504.html

Despite positive comments from the i915 and radeon driver maintainers, 
it was never applied. Did you try lowering the udelay value with i2c-
algo-bit to see if it would solve your problem?

David, any reason why you did not pick this patch? Should I resend it? 
In a split form maybe?

Ben, again, I would really have appreciated if you had contacted me 
while investigating your NVS 300 issue, instead of silently running away 
from i2c-algo-bit. If the code is broken, let's fix it for the benefit 
of all users. Duplicating code around isn't going to help any.

Thanks,
-- 
Jean Delvare
Suse L3