Again on virtual i2c adapter support.

Again on virtual i2c adapter support.

[Rodolfo Giometti]

Hello Jean,

I'm working again on virtual i2c adapter support :) since I received
several letters asking for this support. That's why I'd like to have
from you some advice in order to propose a working code.

Looking at latest i2c code I suppose we can modify the
i2c_del_driver() function as follow:

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index b1c9abe..3454878 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -799,10 +799,10 @@ void i2c_del_driver(struct i2c_driver *driver)
        class_for_each_device(&i2c_adapter_class, NULL, driver,
                              __detach_adapter);

+       mutex_unlock(&core_lock);
+
        driver_unregister(&driver->driver);
        pr_debug("i2c-core: driver [%s] unregistered\n",
        driver->driver.name);
-
-       mutex_unlock(&core_lock);
 }
 EXPORT_SYMBOL(i2c_del_driver);

Unluckely I'm not an i2c guru so I'd like to ask to you. If such
modification is correct I can provide a simple support with no
modifications to the i2c core! Apart this one, of course... :)

If you agree I can send a patchset, but everything is based on the
above change...

Thanks for your suggestions,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail: giometti-AVVDYK/kqiJWk0Htik3J/w@public.gmane.org
Linux Device Driver                          giometti-k2GhghHVRtY@public.gmane.org
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti

Re: Again on virtual i2c adapter support.

[Jean Delvare]

Hi Rodolfo,

On Thu, 22 Jan 2009 16:02:30 +0100, Rodolfo Giometti wrote:
> Hello Jean,
> 
> I'm working again on virtual i2c adapter support :) since I received
> several letters asking for this support. That's why I'd like to have
> from you some advice in order to propose a working code.

Thanks for keeping up with this despite my lack of support. I really
hope that we can get this done this year.

Did you speak with Dave Miller already? Some months ago he proposed an
horrible hack to implement support for I2C bus multiplexing, which I
nacked and I pointed him to you.

Last year I have been working with David Brownell of cleaning up
i2c-core so that adding things like multiplexing support is easier, in
particular when it comes to the locking model. It's not completely done
yet but I think we're on the right track. My hope was that we could
finish the cleanups first and add multiplexing support only then, but
maybe I was too optimistic, the cleanups take longer than I expected.

> Looking at latest i2c code I suppose we can modify the
> i2c_del_driver() function as follow:
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index b1c9abe..3454878 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -799,10 +799,10 @@ void i2c_del_driver(struct i2c_driver *driver)
>         class_for_each_device(&i2c_adapter_class, NULL, driver,
>                               __detach_adapter);
> 
> +       mutex_unlock(&core_lock);
> +
>         driver_unregister(&driver->driver);
>         pr_debug("i2c-core: driver [%s] unregistered\n",
>         driver->driver.name);
> -
> -       mutex_unlock(&core_lock);
>  }
>  EXPORT_SYMBOL(i2c_del_driver);
> 
> Unluckely I'm not an i2c guru so I'd like to ask to you. If such
> modification is correct I can provide a simple support with no
> modifications to the i2c core! Apart this one, of course... :)

Honestly, I am not sure either. The locking model isn't completely
cleared yet, we still need to get rid of the 3 dozen remaining legacy
i2c chip drivers before we can finish it. I _think_ the above it
correct, after all we do not hold the lock when calling
driver_register() so I can't see why we would need to hold it when
calling driver_unregister()... unless either i2c_client_release() or
i2c_client_dev_release() needs this.

David should know better, as he designed the thing in the first place.
David, can you please comment on the proposed change?

> If you agree I can send a patchset, but everything is based on the
> above change...

I would like to propose that you create yourself an account on
i2c.wiki.kernel.org. I've just added a "Work in progress" section on
the main page so that other interested developers can keep track of
what is happening. You could create a page describing your progress and
things that need to be done. And I should do the same for my part.

Oh, and please stop calling the thing "virtual i2c adapter support".
These adapters are very real. What you are working on is better
described as "i2c bus multiplexing support".

Thanks,
-- 
Jean Delvare

Re: Again on virtual i2c adapter support.

[Peter Korsgaard]

>>>>> "Jean" == Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> writes:

Hi,

 Jean> Oh, and please stop calling the thing "virtual i2c adapter support".
 Jean> These adapters are very real. What you are working on is better
 Jean> described as "i2c bus multiplexing support".

Not only that, it's afaik about doing i2c bus multiplexing where the
multiplexers are themselves i2c devices - E.G. stuff where you might
need to recursively call into the i2c core.

I have been doing i2c multiplexers where the multiplexers themselves
are not controlled through i2c for years without problems.

I afaik even posted an example driver back when Dave posted his
multiplex hack.

-- 
Bye, Peter Korsgaard

Re: Again on virtual i2c adapter support.

[Jean Delvare]

Hi Peter,

On Fri, 23 Jan 2009 10:23:05 +0100, Peter Korsgaard wrote:
> >>>>> "Jean" == Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> writes:
> 
> Hi,
> 
>  Jean> Oh, and please stop calling the thing "virtual i2c adapter support".
>  Jean> These adapters are very real. What you are working on is better
>  Jean> described as "i2c bus multiplexing support".
> 
> Not only that, it's afaik about doing i2c bus multiplexing where the
> multiplexers are themselves i2c devices - E.G. stuff where you might
> need to recursively call into the i2c core.
> 
> I have been doing i2c multiplexers where the multiplexers themselves
> are not controlled through i2c for years without problems.
> 
> I afaik even posted an example driver back when Dave posted his
> multiplex hack.

There are still a few caveats when doing this. In particular, if you
use drivers which probe the I2C bus for devices, you must make sure
that the devices will be properly found on either the trunk or one of
the multiplexer branches but not both. In fact this is with this
specific case in mind that I decided to wait for the i2c device driver
binding model to be cleaned up before going on with full multiplexing
support.

That being said, yes, you're correct, multiplexing where the
multiplexer chip isn't on the bus itself is a lot easier and can
already be done today in some cases.

-- 
Jean Delvare

Re: Again on virtual i2c adapter support.

[Peter Korsgaard]

>>>>> "Jean" == Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> writes:

Hi,

 Jean> There are still a few caveats when doing this. In particular,
 Jean> if you use drivers which probe the I2C bus for devices, you
 Jean> must make sure that the devices will be properly found on
 Jean> either the trunk or one of the multiplexer branches but not
 Jean> both. In fact this is with this specific case in mind that I
 Jean> decided to wait for the i2c device driver binding model to be
 Jean> cleaned up before going on with full multiplexing support.

True. I unsually solve this by making sure the multiplexer starts in
an unconnected state so the trunk probe doesn't find anything, or
simply not use the old style probing.

-- 
Bye, Peter Korsgaard

Re: Again on virtual i2c adapter support.

[Jean Delvare]

On Fri, 23 Jan 2009 10:49:41 +0100, Peter Korsgaard wrote:
> >>>>> "Jean" == Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> writes:
> 
> Hi,
> 
>  Jean> There are still a few caveats when doing this. In particular,
>  Jean> if you use drivers which probe the I2C bus for devices, you
>  Jean> must make sure that the devices will be properly found on
>  Jean> either the trunk or one of the multiplexer branches but not
>  Jean> both. In fact this is with this specific case in mind that I
>  Jean> decided to wait for the i2c device driver binding model to be
>  Jean> cleaned up before going on with full multiplexing support.
> 
> True. I unsually solve this by making sure the multiplexer starts in
> an unconnected state so the trunk probe doesn't find anything, or
> simply not use the old style probing.

Please keep in mind that the difficulty here is with probing itself,
not just with old-style. The new binding model also has a detection
mode, which is also affected. Making sure that the multiplexer is in an
unconnected state initially isn't sufficient, as you can load I2C chip
drivers at any later point in time and this will trigger a new
detection cycle. And not all multiplexers can be disabled that way,
some have always one outer channel enabled.

I've been thinking about it a bit and my conclusion is that detection
is simply not compatible with multiplexed I2C buses in general. In
specific cases (for example if there is no chip on the trunk) you can
get it to work but I couldn't come up with a logic that always works.
If anyone has ideas, these are welcome.

-- 
Jean Delvare

Re: Again on virtual i2c adapter support.

[Peter Korsgaard]

>>>>> "Jean" == Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> writes:

 >> True. I unsually solve this by making sure the multiplexer starts in
 >> an unconnected state so the trunk probe doesn't find anything, or
 >> simply not use the old style probing.

 Jean> Please keep in mind that the difficulty here is with probing itself,
 Jean> not just with old-style. The new binding model also has a detection
 Jean> mode, which is also affected. Making sure that the multiplexer is in an
 Jean> unconnected state initially isn't sufficient, as you can load I2C chip
 Jean> drivers at any later point in time and this will trigger a new
 Jean> detection cycle. And not all multiplexers can be disabled that way,
 Jean> some have always one outer channel enabled.

Yes, but my code
(http://www.mail-archive.com/i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org/msg01539.html) does:

lock parent mutex
enable mux
do transfer on parent bus
disable mux
unlock parent

So that afaik shouldn't be a problem.

If your mux doesn't have an enable control and there's no unused
connection you can use instead, then that ofcourse doesn't work.

-- 
Bye, Peter Korsgaard

Re: Again on virtual i2c adapter support.

[Rodolfo Giometti]

On Fri, Jan 23, 2009 at 10:23:05AM +0100, Peter Korsgaard wrote:
> >>>>> "Jean" == Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> writes:
> 
> Hi,
> 
>  Jean> Oh, and please stop calling the thing "virtual i2c adapter support".
>  Jean> These adapters are very real. What you are working on is better
>  Jean> described as "i2c bus multiplexing support".
> 
> Not only that, it's afaik about doing i2c bus multiplexing where the
> multiplexers are themselves i2c devices - E.G. stuff where you might
> need to recursively call into the i2c core.

I did exactly as you say. I have a custom board with a PCA9540 which
is a i2c multiplexer controlled by i2c bus.

> I have been doing i2c multiplexers where the multiplexers themselves
> are not controlled through i2c for years without problems.
> 
> I afaik even posted an example driver back when Dave posted his
> multiplex hack.

Did you post the code on this list?

However I'm going to publish my job on i2c multiplexers ASAP, maybe
you can take a look at it and verify if it fixes your needs.

Ciao,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail: giometti-AVVDYK/kqiJWk0Htik3J/w@public.gmane.org
Linux Device Driver                          giometti-k2GhghHVRtY@public.gmane.org
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti

Re: Again on virtual i2c adapter support.

[Peter Korsgaard]

>>>>> "Rodolfo" == Rodolfo Giometti <giometti-AVVDYK/kqiJWk0Htik3J/w@public.gmane.org> writes:

Hi,

 >> I have been doing i2c multiplexers where the multiplexers themselves
 >> are not controlled through i2c for years without problems.
 >> 
 >> I afaik even posted an example driver back when Dave posted his
 >> multiplex hack.

 Rodolfo> Did you post the code on this list?

Yes, but notice that it was for a multiplexer NOT controlled over I2C.

http://www.mail-archive.com/i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org/msg01539.html

-- 
Bye, Peter Korsgaard

Re: Again on virtual i2c adapter support.

[Michelle Konzack]

[-- Attachment #1: Type: text/plain, Size: 2319 bytes --]

Am 2009-01-23 14:48:33, schrieb Rodolfo Giometti:
> On Fri, Jan 23, 2009 at 10:23:05AM +0100, Peter Korsgaard wrote:
> > Not only that, it's afaik about doing i2c bus multiplexing where the
> > multiplexers are themselves i2c devices - E.G. stuff where you might
> > need to recursively call into the i2c core.
> 
> I did exactly as you say. I have a custom board with a PCA9540 which
> is a i2c multiplexer controlled by i2c bus.

Here too, but using a Maxim MAX7367 and MAX7369 and here  you  will  run
into trouble, because the first one  is  a  Switch/Multiplexer  and  the
second one a Multiplexer only.

Runing an Atmel AT91SAM7S and doing RAW coding is no problem, but using
my Atmel AT91SAM7SE (64MB NAND, 32 MB SDRAM) using Linux is (currently)
the hell.

> > I have been doing i2c multiplexers where the multiplexers themselves
> > are not controlled through i2c for years without problems.

So you use those Multiplexer only for  level  switching  where  even  on
different ports the Address is unique?  This is working with  the  Maxim
chips too.

But in my "24V DC modular ATX PSU" (like a striped down UPS) I  have  on
the first Port the standard ATX outputs and the other  three  Ports  are
connected to Expansion slots...

Since no one know, which Expansion modules are connected, I can not have
unique addresses for the I²C devices...

Used Devices are:

DS1780		Hardware Monitor
DS28CZ04	4kbit EEPROM	(stores module infos)
MAX1036		4-Channel 8-Bit ADC
MAX1038		12-Channel 8-Bit ADC

but not limited too.

> However I'm going to publish my job on i2c multiplexers ASAP, maybe
> you can take a look at it and verify if it fixes your needs.

I hope I hear you soon...

Thanks, Greetings and nice Day/Evening
    Michelle Konzack
    Systemadministrator
    24V Electronic Engineer
    Tamay Dogan Network
    Debian GNU/Linux Consultant


-- 
Linux-User #280138 with the Linux Counter, http://counter.li.org/
##################### Debian GNU/Linux Consultant #####################
<http://www.tamay-dogan.net/>               <http://www.can4linux.org/>
Michelle Konzack   Apt. 917                  ICQ #328449886
+49/177/9351947    50, rue de Soultz         MSN LinuxMichi
+33/6/61925193     67100 Strasbourg/France   IRC #Debian (irc.icq.com)

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Again on virtual i2c adapter support.

[Peter Korsgaard]

>>>>> "Michelle" == Michelle Konzack <linux4michelle-qCYv2r+mE47ltxMxsNcu6w@public.gmane.org> writes:

Hi,

 >> > I have been doing i2c multiplexers where the multiplexers themselves
 >> > are not controlled through i2c for years without problems.

 Michelle> So you use those Multiplexer only for level switching where
 Michelle> even on different ports the Address is unique?  This is
 Michelle> working with the Maxim chips too.

No, there can (and there are) multiple chips with the same
address. What I mean is simply that the mux is't controlled by writing
I2C commands to it, but through something else (in this case a
sram-like bus on a fpga).

-- 
Bye, Peter Korsgaard

Re: Again on virtual i2c adapter support.

[David Brownell]

On Friday 23 January 2009, Jean Delvare wrote:
> > --- a/drivers/i2c/i2c-core.c
> > +++ b/drivers/i2c/i2c-core.c
> > @@ -799,10 +799,10 @@ void i2c_del_driver(struct i2c_driver *driver)
> >         class_for_each_device(&i2c_adapter_class, NULL, driver,
> >                               __detach_adapter);
> > 
> > +       mutex_unlock(&core_lock);
> > +
> >         driver_unregister(&driver->driver);
> >         pr_debug("i2c-core: driver [%s] unregistered\n",
> >         driver->driver.name);
> > -
> > -       mutex_unlock(&core_lock);
> >  }
> >  EXPORT_SYMBOL(i2c_del_driver);
> > 
> > Unluckely I'm not an i2c guru so I'd like to ask to you. If such
> > modification is correct I can provide a simple support with no
> > modifications to the i2c core! Apart this one, of course... :)
> 
> Honestly, I am not sure either. The locking model isn't completely
> cleared yet,

All legacy locking in the I2C core is problematic -- courtesy
of the original (lack-of-)design.  The core_lock is the worst
offender; it's mostly there to support some wierdness when
removing legacy drivers, and I recall thinking that it works
more by luck (and lack of heavy use!) than correctness.


> we still need to get rid of the 3 dozen remaining legacy 
> i2c chip drivers before we can finish it. I _think_ the above it
> correct, after all we do not hold the lock when calling
> driver_register() so I can't see why we would need to hold it when
> calling driver_unregister()... unless either i2c_client_release() or
> i2c_client_dev_release() needs this.
> 
> David should know better, as he designed the thing in the first place.

No I didn't ... I worked on making it play better with the
driver model, and simplifying a bunch of the rest.  I gave
up on fixing the legacy locking ... other than that patch
to remove i2c_adapter.clist_lock, a while back, I think the
only real fix is getting rid of those legacy calls (thus
using only the driver-model internal locks).


> David, can you please comment on the proposed change?

If it's a legacy driver, it will already have been decoupled
from devices, so the nastiest bit of stuff will have gotten
the protection it needs.  New-style drivers shouldn't care
about the legacy stuff at all.  So that much looks plausible.

But I don't have the patience to really analyse the other
locking implications; there were some rude and un-obvious
things lurking there, including abuse of complete() for
the legacy i2c_client lifecycle.

- Dave

Re: Again on virtual i2c adapter support.

[Jean Delvare]

Hi David,

On Fri, 23 Jan 2009 01:39:38 -0800, David Brownell wrote:
> On Friday 23 January 2009, Jean Delvare wrote:
> > David, can you please comment on the proposed change?
> 
> If it's a legacy driver, it will already have been decoupled
> from devices, so the nastiest bit of stuff will have gotten
> the protection it needs.  New-style drivers shouldn't care
> about the legacy stuff at all.  So that much looks plausible.
> 
> But I don't have the patience to really analyse the other
> locking implications; there were some rude and un-obvious
> things lurking there, including abuse of complete() for
> the legacy i2c_client lifecycle.

This was my conclusion as well.

I think I'll try Rodoflo's patch locally and see if anything breaks. If
things appear to work, I am ready to push the patch upstream, but
obviously if anyone reports a locking issue after that, it would have
to be reverted.

In the meantime I will try to find time again to convert more legacy
drivers. My feeling is that we won't do much progress on multiplexing
before we get rid of legacy drivers and their custom locking model.

-- 
Jean Delvare

Re: Again on virtual i2c adapter support.

[Rodolfo Giometti]

On Fri, Jan 23, 2009 at 09:51:10AM +0100, Jean Delvare wrote:

> Thanks for keeping up with this despite my lack of support. I really
> hope that we can get this done this year.
> 
> Did you speak with Dave Miller already? Some months ago he proposed an
> horrible hack to implement support for I2C bus multiplexing, which I
> nacked and I pointed him to you.

No... I don't remember about it...

> Last year I have been working with David Brownell of cleaning up
> i2c-core so that adding things like multiplexing support is easier, in
> particular when it comes to the locking model. It's not completely done
> yet but I think we're on the right track. My hope was that we could
> finish the cleanups first and add multiplexing support only then, but
> maybe I was too optimistic, the cleanups take longer than I expected.

I see.

> Honestly, I am not sure either. The locking model isn't completely
> cleared yet, we still need to get rid of the 3 dozen remaining legacy
> i2c chip drivers before we can finish it. I _think_ the above it
> correct, after all we do not hold the lock when calling
> driver_register() so I can't see why we would need to hold it when
> calling driver_unregister()... unless either i2c_client_release() or
> i2c_client_dev_release() needs this.

That is exactly what I supposed to be. :)

> I would like to propose that you create yourself an account on
> i2c.wiki.kernel.org. I've just added a "Work in progress" section on
> the main page so that other interested developers can keep track of
> what is happening. You could create a page describing your progress and
> things that need to be done. And I should do the same for my part.

Ok. Thanks.

> Oh, and please stop calling the thing "virtual i2c adapter support".
> These adapters are very real. What you are working on is better
> described as "i2c bus multiplexing support".

I do.

Ciao,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail: giometti-AVVDYK/kqiJWk0Htik3J/w@public.gmane.org
Linux Device Driver                          giometti-k2GhghHVRtY@public.gmane.org
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti

Re: Again on virtual i2c adapter support.

[Jean Delvare]

On Thu, 22 Jan 2009 16:02:30 +0100, Rodolfo Giometti wrote:
> Hello Jean,
> 
> I'm working again on virtual i2c adapter support :) since I received
> several letters asking for this support. That's why I'd like to have
> from you some advice in order to propose a working code.
> 
> Looking at latest i2c code I suppose we can modify the
> i2c_del_driver() function as follow:
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index b1c9abe..3454878 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -799,10 +799,10 @@ void i2c_del_driver(struct i2c_driver *driver)
>         class_for_each_device(&i2c_adapter_class, NULL, driver,
>                               __detach_adapter);
> 
> +       mutex_unlock(&core_lock);
> +
>         driver_unregister(&driver->driver);
>         pr_debug("i2c-core: driver [%s] unregistered\n",
>         driver->driver.name);
> -
> -       mutex_unlock(&core_lock);
>  }
>  EXPORT_SYMBOL(i2c_del_driver);

What tree is this patch against? I can't get it to apply on top of
2.6.29-rc2.

-- 
Jean Delvare

Re: Again on virtual i2c adapter support.

[Rodolfo Giometti]

[-- Attachment #1: Type: text/plain, Size: 1193 bytes --]

On Fri, Jan 23, 2009 at 03:24:34PM +0100, Jean Delvare wrote:

> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > index b1c9abe..3454878 100644
> > --- a/drivers/i2c/i2c-core.c
> > +++ b/drivers/i2c/i2c-core.c
> > @@ -799,10 +799,10 @@ void i2c_del_driver(struct i2c_driver *driver)
> >         class_for_each_device(&i2c_adapter_class, NULL, driver,
> >                               __detach_adapter);
> > 
> > +       mutex_unlock(&core_lock);
> > +
> >         driver_unregister(&driver->driver);
> >         pr_debug("i2c-core: driver [%s] unregistered\n",
> >         driver->driver.name);
> > -
> > -       mutex_unlock(&core_lock);
> >  }
> >  EXPORT_SYMBOL(i2c_del_driver);
> 
> What tree is this patch against? I can't get it to apply on top of
> 2.6.29-rc2.

It was against 2.6.29-rc2... let me attach a patch file, maybe I did
cut&paste. :)

Ciao,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail: giometti-AVVDYK/kqiJWk0Htik3J/w@public.gmane.org
Linux Device Driver                          giometti-k2GhghHVRtY@public.gmane.org
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti

[-- Attachment #2: PATCH --]
[-- Type: text/plain, Size: 515 bytes --]

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index b1c9abe..3454878 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -799,10 +799,10 @@ void i2c_del_driver(struct i2c_driver *driver)
 	class_for_each_device(&i2c_adapter_class, NULL, driver,
 			      __detach_adapter);
 
+	mutex_unlock(&core_lock);
+
 	driver_unregister(&driver->driver);
 	pr_debug("i2c-core: driver [%s] unregistered\n", driver->driver.name);
-
-	mutex_unlock(&core_lock);
 }
 EXPORT_SYMBOL(i2c_del_driver);