[PATCH][2.6] Add command function to struct i2c_adapter

[PATCH][2.6] Add command function to struct i2c_adapter

[Michael Hunold]

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

Hi there,

the attached patch adds a command function to struct i2c_adapter, 
similar to the command function that is already there for struct i2c_client.

The reason behind this is as follows:
In the DVB subsystem, the tuners are accessed via normal kernel i2c 
drivers. One big problem is, that tuners can be wired very differently 
depending on the surrounding hardware. Currently, we need to keep 
specific settings which are unique to a hardware design in the 
independent i2c tuner driver. This is both very ugly and makes it 
impossible to support DVB drivers which are not in the official Linux 
kernel tree, but could otherwise use in-kernel frontend driver.

If the struct i2c_adapter has a command function, it would be possible 
to get additional informations from the adapter in the i2c client's 
attach_adapter() function *before* probing the device and guessing 
things like i2c addresses or other hardware settings.

This patch is non-intrusive. It doesn't affect any current drivers, but 
only future developments.

Are there any objections against this patch?

Regards
Michael.

[-- Attachment #2: i2c-add-command.diff --]
[-- Type: text/plain, Size: 633 bytes --]

diff -ura linux-2.6.9-rc2-mm1/include/linux/i2c.h b/include/linux/i2c.h
--- linux-2.6.9-rc2-mm1/include/linux/i2c.h	2004-09-20 12:38:24.000000000 +0200
+++ b/include/linux/i2c.h	2004-09-20 18:53:32.000000000 +0200
@@ -231,6 +231,11 @@
 	struct i2c_algorithm *algo;/* the algorithm to access the bus	*/
 	void *algo_data;
 
+	/* a ioctl like command that can be used to perform specific functions
+	 * with the adapter.
+	 */
+	int (*command)(struct i2c_adapter *adapter, unsigned int cmd, void *arg);
+
 	/* --- administration stuff. */
 	int (*client_register)(struct i2c_client *);
 	int (*client_unregister)(struct i2c_client *);

Re: [PATCH][2.6] Add command function to struct i2c_adapter

[Greg KH]

On Mon, Sep 20, 2004 at 07:19:24PM +0200, Michael Hunold wrote:
>  
> +	/* a ioctl like command that can be used to perform specific functions
> +	 * with the adapter.
> +	 */
> +	int (*command)(struct i2c_adapter *adapter, unsigned int cmd, void *arg);

Ick ick ick.  We don't like ioctls for the very reason they aren't type
safe, and you can pretty much stick anything in there you want.  So
let's not try to add the same type of interface to another subsystem.

How about we add the exact explicit functionality that you want, one
function per "type" of operation, like all other subsystems have.

thanks,

greg k-h

Re: [PATCH][2.6] Add command function to struct i2c_adapter

[Michael Hunold]

Hi,

On 21.09.2004 17:41, Greg KH wrote:
> On Mon, Sep 20, 2004 at 07:19:24PM +0200, Michael Hunold wrote:
> 
>> 
>>+	/* a ioctl like command that can be used to perform specific functions
>>+	 * with the adapter.
>>+	 */
>>+	int (*command)(struct i2c_adapter *adapter, unsigned int cmd, void *arg);

> Ick ick ick.  We don't like ioctls for the very reason they aren't type
> safe, and you can pretty much stick anything in there you want.  So
> let's not try to add the same type of interface to another subsystem.

Ok.

> How about we add the exact explicit functionality that you want, one
> function per "type" of operation, like all other subsystems have.

Hm, but the functionality depends heavily on the types of clients and 
adapters.

For the dvb subsystem, for example, if we know that the i2c adapter is 
some sort of dvb device, we might need to set the pll from the frontend 
i2c client if the user wants to tune to some frequency. The pll settings 
are very h/w specific, so they should be in the driver implementing the 
i2c adapter. So from the dvb frontend dvb i2c client we would call 
adapter->command(adapter, DVB_FE_SET_PLL, &arg).

You don't mean to add a int(*dvb_fe_set_pll)(...) function to the struct 
i2c_adapter instead, don't you?

Isn't this is a general problem? Isn't there the need to have some 
abstraction to allow message passing between i2c clients and i2c 
adapters in both ways, because i2c clients are never that independend 
from the i2c adapter and have always some h/w dependend parts inside?

> thanks,
> greg k-h

Regards
Michael.

Re: [PATCH][2.6] Add command function to struct i2c_adapter

[Jon Smirl]

There is a related I2C problem with EEPROMs and DDC monitors. DDC
monitors look just like EEPROMs, the EEPROM driver can even read most
of them. But there are DDC monitors that need special wakeup sequences
before their ROMs will appear.

EEPROM and DDC are both algo_bit clients. When you attach a bus to
algo_bit both clients will run. There is concern that sending the
special DDC wake up sequence down non-DDC buses might mess up the bus.

A proposal was made to implement different classes of algo_bit clients
but this was never implemented. Would a class solution help with the
dvb problem too?


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH][2.6] Add command function to struct i2c_adapter

[Michael Hunold]

Hi,

On 21.09.2004 19:39, Jon Smirl wrote:
> There is a related I2C problem with EEPROMs and DDC monitors. DDC
> monitors look just like EEPROMs, the EEPROM driver can even read most
> of them. But there are DDC monitors that need special wakeup sequences
> before their ROMs will appear.
> 
> EEPROM and DDC are both algo_bit clients. When you attach a bus to
> algo_bit both clients will run. There is concern that sending the
> special DDC wake up sequence down non-DDC buses might mess up the bus.
> 
> A proposal was made to implement different classes of algo_bit clients
> but this was never implemented. Would a class solution help with the
> dvb problem too?

It would help to separate dvb clients and dvb busses. I just posted 
another mail titled "Adding .class field to struct i2c_client (was Re: 
[PATCH][2.6] Add command function to struct i2c_adapter" that adds a 
.class entry to the struct i2c_client.

With that addition, it's possible for the i2c core to check if the 
.class entries of the adapter and the client match. If they don't then 
there is no need to probe a driver. This will help to keep non-i2c 
drivers to be probed on dvb i2c busses (and screw them up accidently). 
Currently it's up to the driver to decide wheter to probe a bus or not.

CU
Michael.

Re: [PATCH][2.6] Add command function to struct i2c_adapter

[Adrian Cox]

On Tue, 2004-09-21 at 19:05, Michael Hunold wrote:

> With that addition, it's possible for the i2c core to check if the 
> .class entries of the adapter and the client match. If they don't then 
> there is no need to probe a driver. This will help to keep non-i2c 
> drivers to be probed on dvb i2c busses (and screw them up accidently). 
> Currently it's up to the driver to decide wheter to probe a bus or not.

I've said it before, but:
This is all the wrong way round. I2C probing is a solution for the
problem of finding sensors on a pre-ACPI PC. We'd never have invented it
if all we had was DVB cards and monitor detection. 

These .class entries are workarounds that shouldn't be required. For DVB
cards, TV capture cards, monitor detection, and embedded systems the
required behaviour is normally known in advance. Why should the top
level driver have to use these workarounds to steer the result of
probing when it already has all the information?

My rough proposal would be:
1) One by one, disable probing on these I2C adapters.

2) In the pci probe function of the DVB or capture card, do a sequence
like this:
my_dev_priv->i2c_adapter = i2c_adapter_create(...);
my_dev_priv->tea6415 = tea6415_create(my_dev_priv->i2c_adapter,
                                      &my_tea6415_parameters);
my_dev_priv->saa7111 = saa7111_create(my_dev_priv->i2c_adapter);

3) Then to use the i2c client:
tea6415_switch(my_dev_priv->tea6415, &vm);

This is type safe, it allows out of tree DVB and capture drivers, and it
never ever sends an unexpected event down an I2C bus. It doesn't even
need to change the I2C core very much.

- Adrian Cox
Humboldt Solutions Ltd.

Re: [PATCH][2.6] Add command function to struct i2c_adapter

[Jean Delvare]

On Wed, 22 Sep 2004 09:56:06 +0100, Adrian Cox wrote
> On Tue, 2004-09-21 at 19:05, Michael Hunold wrote:
> 
> > With that addition, it's possible for the i2c core to check if the
> > .class entries of the adapter and the client match. If they don't then
> > there is no need to probe a driver. This will help to keep non-i2c
> > drivers to be probed on dvb i2c busses (and screw them up accidently).
> > Currently it's up to the driver to decide wheter to probe a bus or not.
> 
> I've said it before, but:
> This is all the wrong way round. I2C probing is a solution for the
> problem of finding sensors on a pre-ACPI PC. We'd never have 
> invented it if all we had was DVB cards and monitor detection.

Agreed, the sensors case is different from the other I2C bus users. However I
don't get the "pre-ACPI PC" part. Detecting the hardware monitoring chip is
still needed even in ACPI-enabled PCs. I've almost never seen the ACPI
subsystem handle hardware monitoring (except on my laptop where it presents a
single temperature value and no limits). Instead, the hardware monitoring
chips are still there sitting on the I2C or ISA bus, waiting for us to probe
them. I'm not familiar with ACPI, but if it is supposed to handle hardware
monitoring, it looks like at least the current BIOS and/or Linux
implementations don't handle it yet.

> These .class entries are workarounds that shouldn't be required. For 
> DVB cards, TV capture cards, monitor detection, and embedded systems 
> the required behaviour is normally known in advance. Why should the top
> level driver have to use these workarounds to steer the result of
> probing when it already has all the information?

Well, I don't quite follow you here. On the one hand you agree that sensors
and video/embedded stuff should be handled differently, but then you don't
want us to tag them according to their function in order to actually behave
differently.

Of course we could limit the difference to a simple "do probe or do not probe"
flag. This would probably be sufficent. While we are at it though, clearly
labeling adapters with a class promises a wider usability. Another superiority
with the classes is that the check is done by the core instead of relying on
the client's good will. OK, the client could lie on its class(es) to bypass
the check, but this is quite different from simply omitting to check the "do
not probe" flag and less likely to happen by accident too.

> My rough proposal would be:
> 1) One by one, disable probing on these I2C adapters.

The clients are probing the adapters, not the other way around, so there is no
way to "disable probing on these I2C adapters". The only way is to tag the
adapters as "do not probe" and have either the core or the clients respect
this, just as I described above.

> 2) In the pci probe function of the DVB or capture card, do a 
> sequence like this: my_dev_priv->i2c_adapter = 
> i2c_adapter_create(...); my_dev_priv->tea6415 = 
> tea6415_create(my_dev_priv->i2c_adapter,                             
>          &my_tea6415_parameters); my_dev_priv->saa7111 = 
> saa7111_create(my_dev_priv->i2c_adapter);
> 
> 3) Then to use the i2c client:
> tea6415_switch(my_dev_priv->tea6415, &vm);

As far as I know, this is exactly what video folks already do. The whole issue
is not with video folks probing adapters, but with them not wanting us (the
sensors clients) to arbitrarily probe their video i2c busses in search of
hardware monitoring chips. Michael's proposal is meant to give us a way not to
do this anymore.

> This is type safe, it allows out of tree DVB and capture drivers,
>  and it never ever sends an unexpected event down an I2C bus. It 
> doesn't even need to change the I2C core very much.

The change in the i2c-core will be really simple (comparing two bitfields
isn't that hard), although I agree it'll almost certainly cause troube in the
early days if .class fields are not properly filled in some adapters or clients.

All in all I don't see how we can solve the problem without either a "do not
probe" flag in the adapter structure or a class bitfield in both the adapter
and the client structures. I would be fine with either option unless someone
explains how one is better than the other in any particular case.

Thanks.

-- 
Jean Delvare
http://khali.linux-fr.org/

Re: [PATCH][2.6] Add command function to struct i2c_adapter

[Adrian Cox]

On Wed, 2004-09-22 at 13:08, Jean Delvare wrote:
> On Wed, 22 Sep 2004 09:56:06 +0100, Adrian Cox wrote

> > These .class entries are workarounds that shouldn't be required. For 
> > DVB cards, TV capture cards, monitor detection, and embedded systems 
> > the required behaviour is normally known in advance. Why should the top
> > level driver have to use these workarounds to steer the result of
> > probing when it already has all the information?
> 
> Well, I don't quite follow you here. On the one hand you agree that sensors
> and video/embedded stuff should be handled differently, but then you don't
> want us to tag them according to their function in order to actually behave
> differently.

I don't want them tagged because I don't want them to ever appear on a
system-wide list. They're an internal detail of a particular card, and
don't even need to be in sysfs. The only reason to have any shared I2C
code at all for these cards is to avoid duplicating the implementation
of bit-banging.

> > 2) In the pci probe function of the DVB or capture card, do a 
> > sequence like this: my_dev_priv->i2c_adapter = 
> > i2c_adapter_create(...); my_dev_priv->tea6415 = 
> > tea6415_create(my_dev_priv->i2c_adapter,                             
> >          &my_tea6415_parameters); my_dev_priv->saa7111 = 
> > saa7111_create(my_dev_priv->i2c_adapter);
> > 
> > 3) Then to use the i2c client:
> > tea6415_switch(my_dev_priv->tea6415, &vm);
> 
> As far as I know, this is exactly what video folks already do. The whole issue
> is not with video folks probing adapters, but with them not wanting us (the
> sensors clients) to arbitrarily probe their video i2c busses in search of
> hardware monitoring chips. Michael's proposal is meant to give us a way not to
> do this anymore.

Not in the current Linux DVB code. A frontend driver registers itself
onto a list, and whenever a DVB card registers its I2C adapter the
available frontends are probed. My solution would throw away all the
list handling in dvb_i2c.c entirely.

> All in all I don't see how we can solve the problem without either a "do not
> probe" flag in the adapter structure or a class bitfield in both the adapter
> and the client structures. I would be fine with either option unless someone
> explains how one is better than the other in any particular case.

What I want is a way for a card driver to create a private I2C adapter,
and private instances of I2C clients, for purposes of code reuse. The
card driver would be responsible for attaching those clients to the bus
and cleaning up the objects on removal. The bus wouldn't be visible in
sysfs, or accessible from user-mode.

Some USB webcams have internal I2C busses to connect the sensor to the
USB chip. The drivers for these ignore the I2C core completely, and
invent their own system for reading and writing the sensor registers.
Maybe that's actually the best way of dealing with this.

- Adrian Cox
Humboldt Solutions Ltd.

Re: [PATCH][2.6] Add command function to struct i2c_adapter

[Jean Delvare]

On Wed, 22 Sep 2004 12:54:08 +0100, Adrian Cox wrote
> On Wed, 2004-09-22 at 13:08, Jean Delvare wrote:
> > Well, I don't quite follow you here. On the one hand you agree that
> > sensors and video/embedded stuff should be handled differently, but
> > then you don't want us to tag them according to their function in order
> > to actually behave differently.
> 
> I don't want them tagged because I don't want them to ever appear on 
> a system-wide list. They're an internal detail of a particular card, 
> and don't even need to be in sysfs. The only reason to have any 
> shared I2C code at all for these cards is to avoid duplicating the
> implementation of bit-banging.
> (...)
> What I want is a way for a card driver to create a private I2C 
> adapter, and private instances of I2C clients, for purposes of code 
> reuse. The card driver would be responsible for attaching those 
> clients to the bus and cleaning up the objects on removal. The bus 
> wouldn't be visible in sysfs, or accessible from user-mode.

Aha, this is an interesting point (which was missing from your previous
explanation). The base of your proposal would be to have several small i2c
"trees" (where a tree is a list of adapters and a list of clients) instead of
a larger, unique one. This would indeed solve a number of problems, and I
admit that it is somehow equivalent to Michael's classes in that it
efficiently prevents the hardware monitoring clients from probing the video
stuff. The rest is just details internal to each "tree". As I understand it,
each video device would be a tree on itself, while the whole hardware
monitoring stuff would constitute one (bigger) tree. Correct?

> Some USB webcams have internal I2C busses to connect the sensor to 
> the USB chip. The drivers for these ignore the I2C core completely, and
> invent their own system for reading and writing the sensor registers.
> Maybe that's actually the best way of dealing with this.

With your proposal, these drivers could use the common code again while still
being completely separated from the other i2c busses, right?

Thanks.

-- 
Jean Delvare
http://khali.linux-fr.org/

Re: [PATCH][2.6] Add command function to struct i2c_adapter

[Adrian Cox]

On Wed, 2004-09-22 at 14:38, Jean Delvare wrote:
> On Wed, 22 Sep 2004 12:54:08 +0100, Adrian Cox wrote

> > What I want is a way for a card driver to create a private I2C 
> > adapter, and private instances of I2C clients, for purposes of code 
> > reuse. The card driver would be responsible for attaching those 
> > clients to the bus and cleaning up the objects on removal. The bus 
> > wouldn't be visible in sysfs, or accessible from user-mode.
> 
> Aha, this is an interesting point (which was missing from your previous
> explanation). The base of your proposal would be to have several small i2c
> "trees" (where a tree is a list of adapters and a list of clients) instead of
> a larger, unique one. This would indeed solve a number of problems, and I
> admit that it is somehow equivalent to Michael's classes in that it
> efficiently prevents the hardware monitoring clients from probing the video
> stuff. The rest is just details internal to each "tree". As I understand it,
> each video device would be a tree on itself, while the whole hardware
> monitoring stuff would constitute one (bigger) tree. Correct?

Yes. It took me an extra cup of coffee to explain the idea. 

An important detail is that frontends would no longer contain
module_init() and module_exit() functions. Instead, the card driver
would call a function in the frontend module to attach the frontend
client to the tree and simultaneously set the correct parameters for
that specific card. This provides type safety, and a guarantee that the
client parameters match the correct card when mixing cards from
different manufacturers.

> > Some USB webcams have internal I2C busses to connect the sensor to 
> > the USB chip. The drivers for these ignore the I2C core completely, and
> > invent their own system for reading and writing the sensor registers.
> > Maybe that's actually the best way of dealing with this.
> 
> With your proposal, these drivers could use the common code again while still
> being completely separated from the other i2c busses, right?

Possibly, though they tend to have very limited I2C controllers which
makes it more awkward.

- Adrian Cox
Humboldt Solutions Ltd.

Re: [PATCH][2.6] Add command function to struct i2c_adapter

[Jon Smirl]

On Wed, 22 Sep 2004 14:38:46 +0100, Jean Delvare <khali@linux-fr.org> wrote:
> Aha, this is an interesting point (which was missing from your previous
> explanation). The base of your proposal would be to have several small i2c
> "trees" (where a tree is a list of adapters and a list of clients) instead of
> a larger, unique one. This would indeed solve a number of problems, and I
> admit that it is somehow equivalent to Michael's classes in that it
> efficiently prevents the hardware monitoring clients from probing the video
> stuff. The rest is just details internal to each "tree". As I understand it,
> each video device would be a tree on itself, while the whole hardware
> monitoring stuff would constitute one (bigger) tree. Correct?

Any DDC solution needs to leave the data visible in sysfs and
accessible from user space. I'm trying to move the EDID parsing code
out of the kernel.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH][2.6] Add command function to struct i2c_adapter

[Adrian Cox]

On Wed, 2004-09-22 at 16:40, Jon Smirl wrote:

> Any DDC solution needs to leave the data visible in sysfs and
> accessible from user space. I'm trying to move the EDID parsing code
> out of the kernel.

Would it do for a display device to expose read-only EDID data through
sysfs, or do you need I2C level access to DDC from userspace?

- Adrian Cox
Humboldt Solutions Ltd.

Re: [PATCH][2.6] Add command function to struct i2c_adapter

[Jon Smirl]

On Wed, 22 Sep 2004 16:56:19 +0100, Adrian Cox <adrian@humboldt.co.uk> wrote:
> Would it do for a display device to expose read-only EDID data through
> sysfs, or do you need I2C level access to DDC from userspace?

For my purpose of decoding the EDID read only access is fine. 

But I do know there are programs that use the user space I2C drivers
to control extended monitor functions. Some monitors let you set
brightnesss, contrast, on/off via the I2C link.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH][2.6] Add command function to struct i2c_adapter

[Adrian Cox]

On Wed, 2004-09-22 at 17:07, Jon Smirl wrote:
> On Wed, 22 Sep 2004 16:56:19 +0100, Adrian Cox <adrian@humboldt.co.uk> wrote:
> > Would it do for a display device to expose read-only EDID data through
> > sysfs, or do you need I2C level access to DDC from userspace?
> 
> For my purpose of decoding the EDID read only access is fine. 
> 
> But I do know there are programs that use the user space I2C drivers
> to control extended monitor functions. Some monitors let you set
> brightnesss, contrast, on/off via the I2C link.

In which case you will need the current mechanism, with the class
mechanism to stop sensor drivers probing the bus.

- Adrian Cox
Humboldt Solutions Ltd.

Re: [PATCH][2.6] Add command function to struct i2c_adapter

[Jon Smirl]

Can this code be implemented in the current framework? Don't I need
the attach/detach_adapter hook which I can't get to from the algo_bit
code?

http://lkml.org/lkml/2004/9/21/181

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH][2.6] Add command function to struct i2c_adapter

[Jean Delvare]

> > For my purpose of decoding the EDID read only access is fine. 
> > 
> > But I do know there are programs that use the user space I2C drivers
> > to control extended monitor functions. Some monitors let you set
> > brightnesss, contrast, on/off via the I2C link.
> 
> In which case you will need the current mechanism, with the class
> mechanism to stop sensor drivers probing the bus.

There is no absolute necessity to do that as far as I can see. Usually,
there is nothing on the DDC bus but the DDC EEPROM itself, so the probes
won't happen except for the EEPROM itself, for which it is safe.

-- 
Jean "Khali" Delvare
http://khali.linux-fr.org/

Re: [PATCH][2.6] Add command function to struct i2c_adapter

[Adrian Cox]

On Wed, 2004-09-22 at 14:38, Jean Delvare wrote:

> Aha, this is an interesting point (which was missing from your previous
> explanation). The base of your proposal would be to have several small i2c
> "trees" (where a tree is a list of adapters and a list of clients) instead of
> a larger, unique one. This would indeed solve a number of problems, and I
> admit that it is somehow equivalent to Michael's classes in that it
> efficiently prevents the hardware monitoring clients from probing the video
> stuff. The rest is just details internal to each "tree". As I understand it,
> each video device would be a tree on itself, while the whole hardware
> monitoring stuff would constitute one (bigger) tree. Correct?

I've been rereading the code, and it could be even simpler. How about
this:

1) The card driver defines an i2c_adapter structure, but never calls
i2c_add_adapter(). The only extra thing it needs to do is to initialise
the semaphores in the structure.
2) The frontend calls i2c_transfer() directly.
3) The i2c core never gets involved, and there is never any i2c_client
structure.

This gives us the required reuse of the I2C algo-bit code, without any
of the list walking or device probing being required.

- Adrian Cox
Humboldt Solutions Ltd.

Re: [PATCH][2.6] Add command function to struct i2c_adapter

[Mark M. Hoffman]

Hi Adrian:

* Adrian Cox <adrian@humboldt.co.uk> [2004-09-22 19:32:31 +0100]:
> On Wed, 2004-09-22 at 14:38, Jean Delvare wrote:
> 
> > Aha, this is an interesting point (which was missing from your previous
> > explanation). The base of your proposal would be to have several small i2c
> > "trees" (where a tree is a list of adapters and a list of clients) instead of
> > a larger, unique one. This would indeed solve a number of problems, and I
> > admit that it is somehow equivalent to Michael's classes in that it
> > efficiently prevents the hardware monitoring clients from probing the video
> > stuff. The rest is just details internal to each "tree". As I understand it,
> > each video device would be a tree on itself, while the whole hardware
> > monitoring stuff would constitute one (bigger) tree. Correct?
> 
> I've been rereading the code, and it could be even simpler. How about
> this:
> 
> 1) The card driver defines an i2c_adapter structure, but never calls
> i2c_add_adapter(). The only extra thing it needs to do is to initialise
> the semaphores in the structure.
> 2) The frontend calls i2c_transfer() directly.
> 3) The i2c core never gets involved, and there is never any i2c_client
> structure.

Yes, almost...

Why force your card driver to re-implement i2c_smbus_read_byte() and all
its relatives?  Go ahead and define the i2c_client structure(s) as well,
but don't i2c_attach_client().  Sensors drivers do their probing before
attaching the client, so I know that works.

> This gives us the required reuse of the I2C algo-bit code, without any
> of the list walking or device probing being required.

Ditto, *plus* you can still reuse all the i2c_core helper routines that
require a client structure.

Regards,

-- 
Mark M. Hoffman
mhoffman@lightlink.com

Re: [PATCH][2.6] Add command function to struct i2c_adapter

[Michael Hunold]

Hi,

On 22.09.2004 20:32, Adrian Cox wrote:
>>Aha, this is an interesting point (which was missing from your previous
>>explanation). The base of your proposal would be to have several small i2c
>>"trees" (where a tree is a list of adapters and a list of clients) instead of
>>a larger, unique one. This would indeed solve a number of problems, and I
>>admit that it is somehow equivalent to Michael's classes in that it
>>efficiently prevents the hardware monitoring clients from probing the video
>>stuff. The rest is just details internal to each "tree". As I understand it,
>>each video device would be a tree on itself, while the whole hardware
>>monitoring stuff would constitute one (bigger) tree. Correct?

> I've been rereading the code, and it could be even simpler. How about
> this:
> 
> 1) The card driver defines an i2c_adapter structure, but never calls
> i2c_add_adapter(). The only extra thing it needs to do is to initialise
> the semaphores in the structure.
> 2) The frontend calls i2c_transfer() directly.
> 3) The i2c core never gets involved, and there is never any i2c_client
> structure.
> 
> This gives us the required reuse of the I2C algo-bit code, without any
> of the list walking or device probing being required.

This is the way the linux-dvb guys are currently favouring.

In some cases there should be an adapter that has been registered with 
i2c_add_adapter() because there can always be other drivers (audio/video 
multiplexes and the like) that you cannot get your hands on because they 
are provided by the manufacturer of some embedded platform.

For some cards (mostly bt8x8 based, ie. the core parts are driven by the 
"bttv" driver) and hybrid cards (ie. dvb cards that have a separate 
analog video input) the common i2c bus cannot go away, but it should be 
protected by proper .class entries in both the clients and adapters.

We think about splitting the frontend drivers into library parts, ie. 
library functions for demodulators (the current frontend drivers) and 
h/w dependent plls (or tuners).

Because of the fact the pci device knows which devices are present on 
the bus, it can register and configure the demod, pll and other specific 
dvb devices with direct i2c_transfer()s directly. If there are multiple 
combinations possible, it can use the library to probe as well.

The question we are currently facing is: are the frontend or demod i2c 
drivers real and independent i2c clients like thermal sensors or are 
they part of a h/w dependent design that should better be configured on 
a library basis?

If they are independend, then we need i2c bus types and type-safe 
client<=>adapter communication to exchange configuration and control 
informations, because the ioctl interface currently works from the 
adapter to the client and is not type safe.
Another question is, if this is probably too bloated. Think of simple 
matrix switch chipsets: the only "interface" they have is "set input a 
to output b".

> - Adrian Cox
> Humboldt Solutions Ltd.

CU
Michael.

Re: [PATCH][2.6] Add command function to struct i2c_adapter

[Michael Hunold]

Hi,

On 22.09.2004 20:32, Adrian Cox wrote:
>>Aha, this is an interesting point (which was missing from your previous
>>explanation). The base of your proposal would be to have several small i2c
>>"trees" (where a tree is a list of adapters and a list of clients) instead of
>>a larger, unique one. This would indeed solve a number of problems, and I
>>admit that it is somehow equivalent to Michael's classes in that it
>>efficiently prevents the hardware monitoring clients from probing the video
>>stuff. The rest is just details internal to each "tree". As I understand it,
>>each video device would be a tree on itself, while the whole hardware
>>monitoring stuff would constitute one (bigger) tree. Correct?

> I've been rereading the code, and it could be even simpler. How about
> this:
> 
> 1) The card driver defines an i2c_adapter structure, but never calls
> i2c_add_adapter(). The only extra thing it needs to do is to initialise
> the semaphores in the structure.
> 2) The frontend calls i2c_transfer() directly.
> 3) The i2c core never gets involved, and there is never any i2c_client
> structure.
> 
> This gives us the required reuse of the I2C algo-bit code, without any
> of the list walking or device probing being required.

This is the way the linux-dvb guys are currently favouring.

In some cases there should be an adapter that has been registered with 
i2c_add_adapter() because there can always be other drivers (audio/video 
multiplexes and the like) that you cannot get your hands on because they 
are provided by the manufacturer of some embedded platform.

For some cards (mostly bt8x8 based, ie. the core parts are driven by the 
"bttv" driver) and hybrid cards (ie. dvb cards that have a separate 
analog video input) the common i2c bus cannot go away, but it should be 
protected by proper .class entries in both the clients and adapters.

We think about splitting the frontend drivers into library parts, ie. 
library functions for demodulators (the current frontend drivers) and 
h/w dependent plls (or tuners).

Because of the fact the pci device knows which devices are present on 
the bus, it can register and configure the demod, pll and other specific 
dvb devices with direct i2c_transfer()s directly. If there are multiple 
combinations possible, it can use the library to probe as well.

The question we are currently facing is: are the frontend or demod i2c 
drivers real and independent i2c clients like thermal sensors or are 
they part of a h/w dependent design that should better be configured on 
a library basis?

If they are independend, then we need i2c bus types and type-safe 
client<=>adapter communication to exchange configuration and control 
informations, because the ioctl interface currently works from the 
adapter to the client and is not type safe.
Another question is, if this is probably too bloated. Think of simple 
matrix switch chipsets: the only "interface" they have is "set input a 
to output b".

> - Adrian Cox
> Humboldt Solutions Ltd.

CU
Michael.

Re: [PATCH][2.6] Add command function to struct i2c_adapter

[Michael Hunold]

Hi,

On 22.09.2004 13:54, Adrian Cox wrote:
> Not in the current Linux DVB code. A frontend driver registers itself
> onto a list, and whenever a DVB card registers its I2C adapter the
> available frontends are probed. My solution would throw away all the
> list handling in dvb_i2c.c entirely.

Kernels including 2.6.9-rc2-mm1 have the proprietary dvb_i2c 
implementation inside, ie. no kernel i2c at all. I have recently sent a 
patch to Andrew that converts all dvb drivers and frontends to fully use 
kernel i2c. The current discussion is completely about the 
not-yet-officially-released dvb subsystem using kernel-i2c.

>>All in all I don't see how we can solve the problem without either a "do not
>>probe" flag in the adapter structure or a class bitfield in both the adapter
>>and the client structures. I would be fine with either option unless someone
>>explains how one is better than the other in any particular case.

> What I want is a way for a card driver to create a private I2C adapter,
> and private instances of I2C clients, for purposes of code reuse. The
> card driver would be responsible for attaching those clients to the bus
> and cleaning up the objects on removal. The bus wouldn't be visible in
> sysfs, or accessible from user-mode.

We're having a similar discussion on the linux-dvb mailing list and I 
have made a similar suggestion. There shouldn't be such a thing as a 
generic i2c bus at all - at least not for specific PCI or AGP cards 
having an i2c bus, because you really now what's there.

The adapters should be able to create a specific i2c bus. This bus then 
should have a well-defined client<->adapter interface. The adapter 
provides an interface clients can use for example to query h/w dependent 
informations, like "Is it possible to have chipset XY on your bus?". For 
DVB this question can be answered using pci subvendor/subdevice 
informations. This avoids the need to add a command() function to struct 
i2c_adapter.

If the adapter wants a client to join the bus and the client is really 
found, then the client must provide a well-defined set of functions as 
well. The adapter can then control the device in a type-safe manner and 
doesn't have to control it using the current ioctl interface.

> - Adrian Cox
> Humboldt Solutions Ltd.

We need to keep in mind, that the adapter interface must be a per-client 
interface. On PCI devices it's simple: you have a i2c bus bound to a dvb 
card and know which chipsets can be there. The bus is dvb specific.

On embedded platforms, however, you usually have one one i2c bus, where 
everything is present: dvb frontends, audio/video multiplexers, 
digital/analog audio converters, stuff like that.

So if you create *the* i2c bus and invite i2c client to participate at 
the party, you need to provide different interfaces to the different 
chipsets.

CU
Michael.

Re: [PATCH][2.6] Add command function to struct i2c_adapter

[Adrian Cox]

On Thu, 2004-09-23 at 08:09, Michael Hunold wrote:
> We need to keep in mind, that the adapter interface must be a per-client 
> interface. On PCI devices it's simple: you have a i2c bus bound to a dvb 
> card and know which chipsets can be there. The bus is dvb specific.
> 
> On embedded platforms, however, you usually have one one i2c bus, where 
> everything is present: dvb frontends, audio/video multiplexers, 
> digital/analog audio converters, stuff like that.
> 
> So if you create *the* i2c bus and invite i2c client to participate at 
> the party, you need to provide different interfaces to the different 
> chipsets.

I may have to solve a similar problem when connecting an image sensor
directly to an embedded processor. My current idea looks a bit like
this:

1) The I2C bus is a platform device, created at boot time, independent
of my video capture module.
2) My module contains a dummy I2C driver, which exists solely to grab an
i2c_adapter pointer for the platform I2C device.

My underlying libraries don't have to worry whether the i2c_adapter came
from a parent PCI device or a platform device. My only worry is that
power management may still provide us with a headache, as we'll have to
cope with platform devices being suspended in any order.

- Adrian Cox
Humboldt Solutions Ltd.

Re: [PATCH][2.6] Add command function to struct i2c_adapter

[Jean Delvare]

> There is a related I2C problem with EEPROMs and DDC monitors. DDC
> monitors look just like EEPROMs, the EEPROM driver can even read most
> of them. But there are DDC monitors that need special wakeup sequences
> before their ROMs will appear.
> 
> EEPROM and DDC are both algo_bit clients.

Not true. algo-bit refers to i2c bus implementations, not i2c clients.
It happens that all DDC monitors are accessed through bit-banging I2C
busses (real I2C busses, as opposed to SMBus), but other EEPROMs do not.
Most EEPROMs are from memory modules and hang off the motherboard SMBus
(which by definition does not depend on algo-bit).

> When you attach a bus to algo_bit both clients will run.

FYI, there is no ddcmon driver in Linux 2.6 and I believe there won't
be. The eeprom driver should do the job. Converting the EEPROM data to
significant info belongs to user-space.

> There is concern that sending the
> special DDC wake up sequence down non-DDC buses might mess up the bus.

This is however true, and I agree that Michael's proposal could help in
this respect. And I actually believe that his I2C classes implementation
would help more than the "command" callback. After all, you need the DDC
data to identify the monitor, and (in some cases) need to know the
monitor to properly access the DDC data...

(BTW, I am not certain at all that we should add support for these
broken monitors. As far as I can see, they do not support the DDC
standard, too bad for them. I would hate to break standard-compliant
monitors by implementing tricks to support non-standard ones.)

> A proposal was made to implement different classes of algo_bit clients
> but this was never implemented. Would a class solution help with the
> dvb problem too?

As a side note, the classes apply to all I2C adapters, not just
bit-banging ones. And classes apply to the busses purposes (video, ddc,
sensors etc...), not the technical details such as the bus being I2C
compatible or only SMBus.

I have the feeling that we should go on implementing these classes
first, and see what else is needed only after that.

-- 
Jean "Khali" Delvare
http://khali.linux-fr.org/

Re: [PATCH][2.6] Add command function to struct i2c_adapter

[Jon Smirl]

This is the special wake code for older monitors.  ATI supplied it and
it is in the radeon driver.

My understanding that this is a generic problem with old DDC monitors
so the code should be in a DDC I2C driver instead of being added to
all of the video drivers. I don't want the DDC driver to decode the
EDID data, it just needs to handle this problem.

I can say that even my current monitors don't always return the EDID
data without being woken up. The only way to see this is to have
multiple video cards in your PC. The secondary cards won't be reset by
the kernel. Reseting the card will run the BIOS which wakes the
monitors up. If you try to get the EDID from the secondary cards
before they are reset you won't be able to reliably read it.

int radeon_probe_i2c_connector(struct radeonfb_info *rinfo, int conn,
u8 **out_edid)
{
	u32 reg = rinfo->i2c[conn-1].ddc_reg;
	u8 *edid = NULL;
	int i, j;

	OUTREG(reg, INREG(reg) & 
			~(VGA_DDC_DATA_OUTPUT | VGA_DDC_CLK_OUTPUT));

	OUTREG(reg, INREG(reg) & ~(VGA_DDC_CLK_OUT_EN));
	(void)INREG(reg);

	for (i = 0; i < 3; i++) {
		/* For some old monitors we need the
		 * following process to initialize/stop DDC
		 */
		OUTREG(reg, INREG(reg) & ~(VGA_DDC_DATA_OUT_EN));
		(void)INREG(reg);
		msleep(13);

		OUTREG(reg, INREG(reg) & ~(VGA_DDC_CLK_OUT_EN));
		(void)INREG(reg);
		for (j = 0; j < 5; j++) {
			msleep(10);
			if (INREG(reg) & VGA_DDC_CLK_INPUT)
				break;
		}
		if (j == 5)
			continue;

		OUTREG(reg, INREG(reg) | VGA_DDC_DATA_OUT_EN);
		(void)INREG(reg);
		msleep(15);
		OUTREG(reg, INREG(reg) | VGA_DDC_CLK_OUT_EN);
		(void)INREG(reg);
		msleep(15);
		OUTREG(reg, INREG(reg) & ~(VGA_DDC_DATA_OUT_EN));
		(void)INREG(reg);
		msleep(15);

		/* Do the real work */
		edid = radeon_do_probe_i2c_edid(&rinfo->i2c[conn-1]);

		OUTREG(reg, INREG(reg) | 
				(VGA_DDC_DATA_OUT_EN | VGA_DDC_CLK_OUT_EN));
		(void)INREG(reg);
		msleep(15);
		
		OUTREG(reg, INREG(reg) & ~(VGA_DDC_CLK_OUT_EN));
		(void)INREG(reg);
		for (j = 0; j < 10; j++) {
			msleep(10);
			if (INREG(reg) & VGA_DDC_CLK_INPUT)
				break;
		}

		OUTREG(reg, INREG(reg) & ~(VGA_DDC_DATA_OUT_EN));
		(void)INREG(reg);
		msleep(15);
		OUTREG(reg, INREG(reg) |
				(VGA_DDC_DATA_OUT_EN | VGA_DDC_CLK_OUT_EN));
		(void)INREG(reg);
		if (edid)
			break;
	}
	if (out_edid)
		*out_edid = edid;
	if (!edid) {
		RTRACE("radeonfb: I2C (port %d) ... not found\n", conn);
		return MT_NONE;
	}
	if (edid[0x14] & 0x80) {
		/* Fix detection using BIOS tables */
		if (rinfo->is_mobility /*&& conn == ddc_dvi*/ &&
		    (INREG(LVDS_GEN_CNTL) & LVDS_ON)) {
			RTRACE("radeonfb: I2C (port %d) ... found LVDS panel\n", conn);
			return MT_LCD;
		} else {
			RTRACE("radeonfb: I2C (port %d) ... found TMDS panel\n", conn);
			return MT_DFP;
		}
	}
       	RTRACE("radeonfb: I2C (port %d) ... found CRT display\n", conn);
	return MT_CRT;
}

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH][2.6] Add command function to struct i2c_adapter

[Michael Hunold]

Hi,

On 21.09.2004 17:41, Greg KH wrote:
> On Mon, Sep 20, 2004 at 07:19:24PM +0200, Michael Hunold wrote:
> 
>> 
>>+	/* a ioctl like command that can be used to perform specific functions
>>+	 * with the adapter.
>>+	 */
>>+	int (*command)(struct i2c_adapter *adapter, unsigned int cmd, void *arg);
> 
> 
> Ick ick ick.  We don't like ioctls for the very reason they aren't type
> safe, and you can pretty much stick anything in there you want.  So
> let's not try to add the same type of interface to another subsystem.

Ok, Gerd Knorr and I have been discussing this and we have come up with 
the following idea.

We like to have an completly isolated i2c adapter, where the device 
driver can invite i2c drivers to connect an i2c client to. When the 
connection is made, an "interface" pointer with client-specific data or 
function pointers can be provided.

- i2c adapter and i2c clients register themselves as usual

- add a new NO_PROBE flag to struct i2c_adapter, so a particular adapter 
is never probed by anyone

- add these two functions to struct i2c_driver:
int (*connect)(struct i2c_adapter *adapter, void *interface, struct 
i2c_client **client);
int (*disconnect)(struct i2c_client *client);

- add new generic i2c functions:
struct i2c_driver* i2c_driver_get(char *name);
void i2c_driver_put(struct i2c_driver *drv);

- the dvb-ttpci driver now can do the following:

struct stv0299_interface {
	struct dvb_adapter *dvb_adap;
	int tuner_addr;
};

struct stv0299_interface s_if;
struct i2c_driver *drv;
struct i2c_client *clt;

request_module("stv0299");
drv = i2c_driver_get("stv0299");
// fill s_if here
drv->connect(adap, &s_if, &clt);

Now inside the "connect" function of the stv0299 demodulator i2c driver 
the device is probed, registered to the adapter and the pointer to the 
interface with the h/w dependend information is stored.

> thanks,
> greg k-h

The crucial point is probably the void * interface pointer, isn't it? Is 
this a no-no in this situation?

The dvb-ttpci driver is inviting a very specific driver, no probing 
involved. The driver interface is well-defined and will only be hidden 
behind the void * pointer to avoid a functional reference to the 
demodulator driver.

We have discussed simply adding a type-safe stv0299-specific connect 
function as well, but that would introduce a functional dependency. The 
dvb-ttpci driver can be used with about 8 different frontend drivers, so 
loading the driver would cause unnecessary frontend drivers to be loaded 
as well.

CU
Michael.

Re: [PATCH][2.6] Add command function to struct i2c_adapter

[Jean Delvare]

> We like to have an completly isolated i2c adapter, where the device 
> driver can invite i2c drivers to connect an i2c client to. When the 
> connection is made, an "interface" pointer with client-specific data
> or function pointers can be provided.
> (...)
> - add a new NO_PROBE flag to struct i2c_adapter, so a particular
> adapter is never probed by anyone

I don't get it. If the adapter is isolated, there is no way the i2c-core
will probe it anyway. As Adrian Cox underlined, it should be far easier
and more efficient to separate these adapters from the main i2c adapters
list from the beginning than leaving them in the main list and then try
and prevent future probings using a flag.

Also, how does this proposal interact with the work on the i2c classes?
Although the classes carry more information than a simple flag or a
complete separation, both were/may be introduced to achieve the same
goal, isn't it?

Thanks,

-- 
Jean "Khali" Delvare
http://khali.linux-fr.org/

Re: [PATCH][2.6] Add command function to struct i2c_adapter

[Michael Hunold]

Hi,

On 24.09.2004 20:05, Jean Delvare wrote:
>>We like to have an completly isolated i2c adapter, where the device 
>>driver can invite i2c drivers to connect an i2c client to. When the 
>>connection is made, an "interface" pointer with client-specific data
>>or function pointers can be provided.
>>(...)
>>- add a new NO_PROBE flag to struct i2c_adapter, so a particular
>>adapter is never probed by anyone

> I don't get it. If the adapter is isolated, there is no way the i2c-core
> will probe it anyway. As Adrian Cox underlined, it should be far easier
> and more efficient to separate these adapters from the main i2c adapters
> list from the beginning than leaving them in the main list and then try
> and prevent future probings using a flag.

There a two scenarios, where you don't have full control over the i2c 
adapter or you don't want to fully isolate the bus:

1) Some dvb drivers are bttv-sub-drivers, ie. bttv does the pci and i2c 
handling by itself. You have the struct i2c_adapter pointer, but it has 
been registered by bttv, most likely as an analog and/or digital tv i2c bus.

2) Embedded platforms have usually a system-wide i2c bus, so you cannot 
isolate the bus here.

It's useful to register both adapters and drivers to the i2c-core, so 
you keep the door open.

> Also, how does this proposal interact with the work on the i2c classes?
> Although the classes carry more information than a simple flag or a
> complete separation, both were/may be introduced to achieve the same
> goal, isn't it?

Partly, yes.

The .class approach is necessary to have a finer grained access control 
by the i2c-core regarding bus classes, ie. not the client drivers have 
to check if the bus should be probed (for example dcc drivers on a dvb 
bus). This is useful in general.

If we have a PCI card where we exactly know what we are doing, we can 
use the NO_PROBE flag to effectively block any probing and can use the 
proposed interface to manually connect the clients.

> Thanks,

CU
Michael.

Re: [PATCH][2.6] Add command function to struct i2c_adapter

[Greg KH]

On Fri, Sep 24, 2004 at 10:21:08PM +0200, Michael Hunold wrote:
> >Also, how does this proposal interact with the work on the i2c classes?
> >Although the classes carry more information than a simple flag or a
> >complete separation, both were/may be introduced to achieve the same
> >goal, isn't it?
> 
> Partly, yes.
> 
> The .class approach is necessary to have a finer grained access control 
> by the i2c-core regarding bus classes, ie. not the client drivers have 
> to check if the bus should be probed (for example dcc drivers on a dvb 
> bus). This is useful in general.
> 
> If we have a PCI card where we exactly know what we are doing, we can 
> use the NO_PROBE flag to effectively block any probing and can use the 
> proposed interface to manually connect the clients.

But why?  The .class feature can accomplish this too.  Just create a new
class for this type of adapter and device.  Then only that device will
be able to be connected to that adapter, just like you want to have
happen, right?

thanks,

greg k-h

Re: [PATCH][2.6] Add command function to struct i2c_adapter

[Adrian Cox]

On Fri, 2004-10-01 at 07:52, Greg KH wrote:
> On Fri, Sep 24, 2004 at 10:21:08PM +0200, Michael Hunold wrote:

> > If we have a PCI card where we exactly know what we are doing, we can 
> > use the NO_PROBE flag to effectively block any probing and can use the 
> > proposed interface to manually connect the clients.
> 
> But why?  The .class feature can accomplish this too.  Just create a new
> class for this type of adapter and device.  Then only that device will
> be able to be connected to that adapter, just like you want to have
> happen, right?

Either the i2c devices need to be able to support a list of permitted
adapters, or the i2c adapters need a list of permitted clients. A single
class isn't adequate. Consider the following scenario:

The FooTV123 has multiplexor MX3R0K3 and frontend XYZZY, the TVMatic3000
has frontend XYZZY and multiplexor MX31337, and the FooTV124 has
multiplexor MX31337 and frontend FR012. All three cards are installed in
the same machine. In the worst case the probe code for MX31337 puts
MX3R0K3 into a state that requires a hard reset.

Manual connection of clients makes it easier to develop a driver outside
the kernel tree, then merge it when ready, without having to allocate a
number from a central authority.

Creating the adapter with a list of permitted clients is also an
adequate solution for a bus like I2C which doesn't properly support
probing. The OCP bus on PowerPC has no explicit probing, and uses a
similar approach of creating the bus with a list of the devices possible
for that PowerPC model.

- Adrian Cox
Humboldt Solutions Ltd.

Re: [PATCH][2.6] Add command function to struct i2c_adapter

[Jean Delvare]

On Fri, 01 Oct 2004 13:22:45 +0100, Adrian Cox wrote
> Either the i2c devices need to be able to support a list of permitted
> adapters, or the i2c adapters need a list of permitted clients. A single
> class isn't adequate. Consider the following scenario:
> 
> The FooTV123 has multiplexor MX3R0K3 and frontend XYZZY, the TVMatic3000
> has frontend XYZZY and multiplexor MX31337, and the FooTV124 has
> multiplexor MX31337 and frontend FR012. All three cards are 
> installed in the same machine. In the worst case the probe code for 
> MX31337 puts MX3R0K3 into a state that requires a hard reset.
> 
> Manual connection of clients makes it easier to develop a driver outside
> the kernel tree, then merge it when ready, without having to 
> allocate a number from a central authority.

Agreed. Greg's proposal would somehow mean one class per video device adapter,
which doesn't sound good. We have room for only 32 classes anyway. However, I
agree with Greg that a "no probe" flag isn't needed if we already have
well-defined classes.

Basically, probing is something only hardware monitoring chip drivers do (+
eeprom, so we can include the DDC world). Video clients don't probe as far as
I know (because probing just doesn't work here). So, if we were to have a "no
probe" flag, it would always be clear when class is VIDEO and always set when
class is HWMON (I voluntarily simplify, there are a couple more classes), so
it means that the "no probe" flag is redundant. To put it short, simply not
declaring an adapter as class HWMON means it won't be probed ever (the eeprom
case is to be considered separately and may require a class of its own).

> Creating the adapter with a list of permitted clients is also an
> adequate solution for a bus like I2C which doesn't properly support
> probing. The OCP bus on PowerPC has no explicit probing, and uses a
> similar approach of creating the bus with a list of the devices possible
> for that PowerPC model.

This may be fine for the PPC world, but in the i386 world it wont work. To
give you an idea, the MBM database has 1155 motherboards listed, most of which
have hardware monitoring chips on-board. It happens quite often that I don't
find motherboard models I am looking for therein, so a complete base would
have maybe 1500 or 2000 entries. We certainly don't want to have such a base
in the linux kernel tree. It would be unmaintainable, and also would ensure
that new hardware has no chance to work until us developers know about it.
Probing is the way to go there, even if the I2C bus was obviously not designed
for this.

So I2C bus probing won't go away. Classes or flags are what we need (unless we
go for separated I2C adapter lists, but that's a completely different
approach, and obviously we are not interested until someone comes with a solid
patch that really does this.) A working classes system is really next door,
Michael did most of the work already, half of which is already merged into the
kernel tree.

Thanks.

-- 
Jean Delvare
http://khali.linux-fr.org/

Re: [PATCH][2.6] Add command function to struct i2c_adapter

[Greg KH]

On Fri, Oct 01, 2004 at 01:22:45PM +0100, Adrian Cox wrote:
> On Fri, 2004-10-01 at 07:52, Greg KH wrote:
> > On Fri, Sep 24, 2004 at 10:21:08PM +0200, Michael Hunold wrote:
> 
> > > If we have a PCI card where we exactly know what we are doing, we can 
> > > use the NO_PROBE flag to effectively block any probing and can use the 
> > > proposed interface to manually connect the clients.
> > 
> > But why?  The .class feature can accomplish this too.  Just create a new
> > class for this type of adapter and device.  Then only that device will
> > be able to be connected to that adapter, just like you want to have
> > happen, right?
> 
> Either the i2c devices need to be able to support a list of permitted
> adapters, or the i2c adapters need a list of permitted clients. A single
> class isn't adequate. Consider the following scenario:
> 
> The FooTV123 has multiplexor MX3R0K3 and frontend XYZZY, the TVMatic3000
> has frontend XYZZY and multiplexor MX31337, and the FooTV124 has
> multiplexor MX31337 and frontend FR012. All three cards are installed in
> the same machine. In the worst case the probe code for MX31337 puts
> MX3R0K3 into a state that requires a hard reset.
> 
> Manual connection of clients makes it easier to develop a driver outside
> the kernel tree, then merge it when ready, without having to allocate a
> number from a central authority.

Ok, I now understand better, thanks for putting up with me :)

So, got a patch to do this?

thanks,

greg k-h

Re: [PATCH][2.6] Add command function to struct i2c_adapter

[Jean Delvare]

Hi Michael,

See my various questions inline.

> the attached patch adds a command function to struct i2c_adapter,
> similar to the command function that is already there for struct
> i2c_client.

I didne't even know that. Is this command function in struct i2c_client used
somewhere? What for?

> The reason behind this is as follows:
> In the DVB subsystem, the tuners are accessed via normal kernel i2c
> drivers. One big problem is, that tuners can be wired very differently
> depending on the surrounding hardware. Currently, we need to keep
> specific settings which are unique to a hardware design in the
> independent i2c tuner driver. This is both very ugly and makes it
> impossible to support DVB drivers which are not in the official Linux
> kernel tree, but could otherwise use in-kernel frontend driver.

What is the i2c tuner driver "independent" of? Do you simply mean that it
holds the common code for all DVB adapters?

> If the struct i2c_adapter has a command function, it would be possible
> to get additional informations from the adapter in the i2c client's
> attach_adapter() function *before* probing the device and guessing
> things like i2c addresses or other hardware settings.

I see what you want to do, but I don't exactly see how it solves your problem.
Your new command callback function will provide an adapter-specific interface
to adapter internal data. You will have to fill the data before you can access
it, so there still is hardware-specific data involved. Is your point that this
data would be located in (and split among) adapter drivers instead of the
common tuner driver as is now?

I am interested in your proposal because we (the sensors side of the i2c bus)
are facing a similar problem with hardware monitoring chips on motherboads.
The same chip can be wired differently on various motherboads, and most of the
time there is no way to guess (either because the hardware just doesn't
provide any way, or because the BIOS failed to do its job properly). We have
not come to any solution yet, and most of the time users have to use
user-space tools (such as i2cset) prior to loading the drivers. Looks like the
problem you are dealing with is of similar nature.

My initial thoughts about this were that we could use DMI data and/or PCI
information to identify systems, and trigger motherboard-specific code
whenever needed. It's really just a wild guess, I don't know if it is the
correct way. Wouldn't it work for you as well?

I would appreciate if you could show us some (pseudo)code with explanations on
what you have for now, and what you would have after the change. I have some
difficulties to understand what the change would actually change, most
probably because the video and sensors sides of the i2c subsystem work rather
differently.

On a completely different matter, what about the i2c subsystem class flags and
checking mechanism? You had come with a first step some months ago (adding a
class member to i2c clients), and your proposal (generalized check of i2c
client class against adapter class) looked very good to me, but then we did
not finish the job. What about it? I am willing to help on the sensors front
if you are going into this again someday. If I remember correctly, step 2 was
about clearing manual class checks and filling all class members, and step 3
was about implementing the generalized check in i2c-core. Am I right?

Thanks.

-- 
Jean Delvare
http://khali.linux-fr.org/

Re: [PATCH][2.6] Add command function to struct i2c_adapter

[Michael Hunold]

Hi,

(because I think this will be an in-depth discussion I trimmed the CC 
list a litte bit)

On 21.09.2004 15:28, Jean Delvare wrote:
>>the attached patch adds a command function to struct i2c_adapter,
>>similar to the command function that is already there for struct
>>i2c_client.

> I didne't even know that. Is this command function in struct i2c_client used
> somewhere? What for?

Please have a look at Video4Linux driver drivers/media/video/mxb.c. This 
driver needs various helper chipsets: tuner, tea6145c, tea6420, tda9840, 
saa7111.

For example the tea6415c is a simple video matrix chipset. If the user 
request a change of the input, the video matrix needs to be re-programmed.

For this, the tea6415 driver has the TEA6415C_SWITCH ioctl function, 
which is called through the i2c_client command() function.
 > mxb->tea6415c->driver->command(mxb->tea6415c,TEA6415C_SWITCH, &vm);

Otherwise there would be no way the mxb driver could tell the tea6415c 
that the configuration should be changed.

>>The reason behind this is as follows:
>>In the DVB subsystem, the tuners are accessed via normal kernel i2c
>>drivers. One big problem is, that tuners can be wired very differently
>>depending on the surrounding hardware. Currently, we need to keep
>>specific settings which are unique to a hardware design in the
>>independent i2c tuner driver. This is both very ugly and makes it
>>impossible to support DVB drivers which are not in the official Linux
>>kernel tree, but could otherwise use in-kernel frontend driver.

> What is the i2c tuner driver "independent" of? Do you simply mean that it
> holds the common code for all DVB adapters?

The tuner is called frontend in dvb terms, because it holds more that 
just a simple tuner.

Some parts are hardware-independent and are in the right place in the 
frontend drivers. But manufacturers are free to use different plls (the 
actual tuners). Unfortunately, they nearly all sit on the same i2c 
address, are really dumb, write only and therefore not very distinguishable.

Currently the frontend drivers do a strcmp() on the adapter name. If it 
is "SkyStar2" for example, then it is a TechniSat card with a  Samsung 
TBMU24112IMB pll.

With that solution, it's not possible to support dvb drivers outside of 
the kernel which could theoretically use the in-kernel frontend drivers, 
because h/w dependend things need to coded into every frontend driver.

>>If the struct i2c_adapter has a command function, it would be possible
>>to get additional informations from the adapter in the i2c client's
>>attach_adapter() function *before* probing the device and guessing
>>things like i2c addresses or other hardware settings.

> I see what you want to do, but I don't exactly see how it solves your problem.
> Your new command callback function will provide an adapter-specific interface
> to adapter internal data.

Not adapter internal data, but frontend specific data, that only the 
manufacturer knows (which can be distinguished by subsystem pci ids).

 > You will have to fill the data before you can access
 > it, so there still is hardware-specific data involved. Is your point
 > that this
 > data would be located in (and split among) adapter drivers instead of
 > the
 > common tuner driver as is now?

Yes. With that approach, it's possible to support dvb drivers outside of 
the kernel, because all necessary information about the frontends is 
inside the dvb driver, not the frontend driver.

> I am interested in your proposal because we (the sensors side of the i2c bus)
> are facing a similar problem with hardware monitoring chips on motherboads.
> The same chip can be wired differently on various motherboads, and most of the
> time there is no way to guess (either because the hardware just doesn't
> provide any way, or because the BIOS failed to do its job properly). We have
> not come to any solution yet, and most of the time users have to use
> user-space tools (such as i2cset) prior to loading the drivers. Looks like the
> problem you are dealing with is of similar nature.

Yes, it's basically the same.

> My initial thoughts about this were that we could use DMI data and/or PCI
> information to identify systems, and trigger motherboard-specific code
> whenever needed. It's really just a wild guess, I don't know if it is the
> correct way. Wouldn't it work for you as well?

This is how it will work. In our case, we only need pci ids, but it's 
possible to use other information sources as well.

Let's say the user loads the stv0299 frontend driver and later the 
dvb-ttpci device driver.

The dvb-ttpci driver exports an i2c_adapter, the stv0299 
i2c_attach_adapter() function will be called. Ideally, if this isn't a 
bus from the digitial tv subsystem, then bail out (This needs to be 
implemented, see below).

If we are on a digital tv i2c bus the stv0299 uses the i2c adapter 
command() callback and asks the i2c adapter, if this frontend can be on 
that bus at all and asks for further informations.

dvb-ttpci knows the pci ids and can tell different hardware versions. 
For example, cable dvb cards have different pci ids than satellite dvb 
cards. The stv0299 is a satellite frontend, so if the bus is on a dvb 
cable card then dvb-ttpci can simply tell the stv0299 driver not to do 
anything.

If the stv0299 can be found on that system in theory, either the correct 
hardware stv0299 dependent informations are supplied completely or 
probing informations are supplied.

> I would appreciate if you could show us some (pseudo)code with explanations on
> what you have for now, and what you would have after the change. I have some
> difficulties to understand what the change would actually change, most
> probably because the video and sensors sides of the i2c subsystem work rather
> differently.

Please have a look at drivers/media/dvb/frontends/stv0299.c

At the top you'll notice a lot of defines:
#define PHILIPS_SU1278_TUA      3 // SU1278 with TUA6100 synth

In lots of functions there are switch/case statements to distinguish 
different plls, for example in pll_set_tv_freq().  stv0299_init() is 
ugly, too. The init-data for different hardware is hardcoded into the 
frontend driver. *shiver*

With the command() function, probe_tuner() with the ugly strcmp() stuff 
and i2c probing can be killed completely.

In other places, common i2c_adapter commands can be used to pull 
frontend device specific data from the dvb device (for example frequency 
boundaries) or trigger h/w dependend thinks that only the dvb driver 
knows, like setting a frequency in the pll with command(SET_PLL).

Sorry, I just have some proof-of-concept code, that doesn't use all the 
things I explained above. Some dvb people would like to give up the 
strong separation due to the kernel i2c interface and have something 
more "dvbish".

> On a completely different matter, what about the i2c subsystem class flags and
> checking mechanism? You had come with a first step some months ago (adding a
> class member to i2c clients), and your proposal (generalized check of i2c
> client class against adapter class) looked very good to me, but then we did
> not finish the job. What about it? I am willing to help on the sensors front
> if you are going into this again someday. If I remember correctly, step 2 was
> about clearing manual class checks and filling all class members, and step 3
> was about implementing the generalized check in i2c-core. Am I right?

I'll answer that in a separate mail.

> Thanks.

CU
Michael.