Jean Delvare wrote:
>
>>2) i2c-virtual and pca954x are checked in but not in the Makefiles,
>>pending i2c-core changes required.
>
>
> Strange order, huh?
>
>
I thought I did exactly as we discussed on IRC a few weeks ago.
You didn't want i2c-core changes at that time because you were working on 2.9.0.
I said I'd work on the i2c-virtual code received from Brian.
I checked it in to sensors for review and comment.
I thought that's what we agreed to.
>>I can do the core changes before or after 2.9.0. Attached is the i2c-core
>>patch, slimmed down from that received from Brian Kuschak.
>>It's really only two changes:
>> - creating xxx_nolock versions of some calls
>> - working up the bus tree for busy checks, using i2c_virt_parent()
>
>
> Irk! How exactly is i2c-core supposed to know i2c_virt_parent when it's
> defined by lm_sensors? How is it even supposed to know about
> CONFIG_I2C_VIRTUAL (and what is CONFIG_I2C_VIRTUAL_MODULE?)? Looks like
> a circular dependency issue to me.
>
i2c-core needs to include i2c-virtual.h to get the i2c_virt_parent() definition -
but that wasn't in the patch I posted.
> Also, i2c-core should NOT export nolock functions. These functions (much
> like
> __i2c_check_addr) are meant for internal use only (thus the leading
> underscores in the name, which should be used for the new functions as
> well). Locks are implementation details not supposed to be even known by
> the external users.
>
What Brian discovered is that an i2c bus mux implementation requires a nolock version
of some functions. Think about how the bootstrap works:
Bus gets added
Mux chip found
Sub busses get added (but can't because lock still held - hang)
So that's what the (first half of) i2c-core patch does. Get rid of the leading '__', rename to xxx_nolock,
and export them so that the virtual driver can implement the bootstrap.
It's not a big change. Maybe there's a way to avoid it, though... I don't know.
>
>>Not sure I like the i2c_virt_parent() hack (see
>>kernel/include/i2c-virtual.h in sensors for the definition) and trying
>>to think of a better way to do it.
>
>
> How exactly is it a hack?
>
Well, it's either a "hack" or "icky". You said "ick" which I can agree with.
>
>>Anyway, comments on the patch and on the timing (before or after 2.9.0)
>>welcome.
>
>
> Definitely after. Doesn't sound correct to me at all, to say the truth.
> It looks like things that would need to belong to the i2c-core were
> added in lm_sensors instead.
>
It could have gone either place, what's the difference? You were in the middle
of i2c changes so I put it in sensors. Again, I thought this was what
we agreed to - I wouldn't make any i2c changes. We can move i2c-virtual.c to i2c
if you want. pca954x.c can't move because it's a chip driver.
> Could you please summarize the approach that you are following, or point
> me to an up-to-date document covering this if such a document exists? I
> would like to understand how it would be implemented at the i2c-core
> level, what interface it would expose to the other kernel drivers, and
> if possible a real-world use case (an equivalent of i2c-amd756-s4882
> using i2c-virtual would be great, for example.)
>
Brian's document is checked in doc/busses/i2c-virtual. He also has extensive
comments in i2c-virtual.c and pca954x.c. So that's a start.
I'll try and come up with additional documentation.
Your other questions:
- the implentation at the i2c-core level is simply the patch I posted yesterday
- The code in sensors CVS _is_ the equivalent of i2c-amd756-4882. The
"real-world use case" is "modprobe i2c-amd756; modprobe i2c-virtual; modprobe pca9556 [params]",
where pca9556 looks alot like pca954x.
> I'm suspicious about how this approach is better than what I did for the
> S4882 motherboard, which didn't require any change to the i2c-core. As
> I understand it, even with Brian's virtual busses, one would still have
> to write a pseudo bus driver for each specific multiplexing design
> (since such designs cannot be easily guessed and multiplexer chips are
> hard to detect anyway). The only drawback of my approach is that I had
> to export the original bus driver's i2c_adapter structure. Even that
> might be worked around by simply providing a search-adapter-by-id
> function in i2c-core (although with a slight loss of performance.)
>
> Remember that bus multiplexing is only needed for a very limited number
> of motherboards and a couple proprietary designs. This accounts for a
> very limited total number of users and as such isn't worth breaking
> working concepts or compatibility. I'm sorry to insist but I fear that
> what you (and Brian) are trying to implement would belong to Linux 2.6,
> not 2.4.
>
Recall from our IRC discussion that you would work on xxxx_4882 and submit the patches,
I would work on Brian's implementation. Don't be 'suspicious' :)
My brief summary is that Brian's implementation separates the generic part
of bus muxing into a bus algorithm driver (i2c-virtual) and puts the chip-specific
part into a combination bus adapter driver and chip driver (pca954x).
This parallels i2c-algo-bit and the numerous big-banging bus adapters,
and fits well into our architecture.
The code I got from Brian was for 2.4. So that's where I put it.
Not sure what you are "insisting" - what's wrong with getting it working
on 2.4 first before porting to 2.6?
Anyway, happy to discuss further on IRC if you like, let me know.
mds
