[2.6] Add command function to struct i2c_adapter
PATCH 1/1 [2.6] Add command function to struct i2c_adapter
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.
include/linux/i2c.h +5 -0
@@ -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 *);
include/linux/i2c.h line 237
Quoted 5 lines · Michael Hunold
include/linux/i2c.h +4
-0+/* 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
Hi,
Quoted 11 lines · Greg KH
include/linux/i2c.h line 237
Quoted 5 lines · Michael Hunold
include/linux/i2c.h +4
-0+/* 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.
Quoted 2 lines
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?
Quoted 2 lines
thanks,
greg k-h
Regards
Michael.
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?
Signature · 2 lines
Jon Smirl [email protected]
Hi,
Quoted 5 lines · 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.
Quoted 7 lines
EEPROM and DDC are both
algo_bitclients. When you attach a bus toalgo_bitboth 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_bitclients 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.
Quoted 6 lines
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_bitclients.
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).
Quoted 1 line
When you attach a bus to
algo_bitboth 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.
Quoted 2 lines
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.)
Quoted 3 lines
A proposal was made to implement different classes of
algo_bitclients 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.
Signature · 2 lines
Jean "Khali" Delvare http://khali.linux-fr.org/
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;
}Signature · 2 lines
Jon Smirl [email protected]
Quoted 5 lines · Michael Hunold
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.
On Wed, 22 Sep 2004 09:56:06 +0100, Adrian Cox wrote
Quoted 8 lines · Michael Hunold
Quoted 6 lines
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.
Quoted 4 lines
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.
Quoted 5 lines
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.
Quoted 2 lines
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.
Quoted 9 lines
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.
Quoted 3 lines
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.
Signature · 2 lines
Jean Delvare http://khali.linux-fr.org/
Quoted 8 lines · Jean Delvare
On Wed, 22 Sep 2004 09:56:06 +0100, Adrian Cox wrote
Quoted 5 lines
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?
Quoted 4 lines
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.
Quoted 7 lines
Quoted 7 lines
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);
Quoted 8 lines
Quoted 2 lines
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.
Quoted 4 lines
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.
On Wed, 22 Sep 2004 12:54:08 +0100, Adrian Cox wrote
Quoted 6 lines · Jean Delvare
Quoted 4 lines
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.
Quoted 11 lines
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?
Quoted 4 lines
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.
Signature · 2 lines
Jean Delvare http://khali.linux-fr.org/
Quoted 8 lines · Jean Delvare
On Wed, 22 Sep 2004 12:54:08 +0100, Adrian Cox wrote
Quoted 5 lines
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.
Quoted 9 lines
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.
Quoted 7 lines
Quoted 4 lines
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.
Quoted 9 lines · Jean Delvare
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.
Signature · 2 lines
Jon Smirl [email protected]
Quoted 3 lines · Jon Smirl
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.
Quoted 2 lines · Adrian Cox
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.
Signature · 2 lines
Jon Smirl [email protected]
Quoted 9 lines · Jon Smirl
Quoted 2 lines · Adrian Cox
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.
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
Signature · 2 lines
Jon Smirl [email protected]
Quoted 9 lines · Jean Delvare
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.
Quoted 8 lines
Quoted 5 lines
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.
Signature · 2 lines
Jean "Khali" Delvare http://khali.linux-fr.org/
Hi Adrian:
Quoted 15 lines · Adrian Cox
Quoted 9 lines · Jean Delvare
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:
Quoted 6 lines
1) The card driver defines an
i2c_adapterstructure, but never callsi2c_add_adapter(). The only extra thing it needs to do is to initialise the semaphores in the structure.
2) The frontend callsi2c_transfer()directly.
3) The i2c core never gets involved, and there is never anyi2c_clientstructure.
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.
Quoted 2 lines
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,
Signature · 2 lines
Mark M. Hoffman [email protected]
Hi,
Quoted 4 lines · Adrian Cox
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.centirely.
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.
Quoted 10 lines
Quoted 4 lines
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.
Quoted 2 lines
- 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.
Hi,
Quoted 20 lines · Adrian Cox
Quoted 9 lines
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_adapterstructure, but never callsi2c_add_adapter(). The only extra thing it needs to do is to initialise the semaphores in the structure.
2) The frontend callsi2c_transfer()directly.
3) The i2c core never gets involved, and there is never anyi2c_clientstructure.
Quoted 2 lines
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".
Quoted 2 lines
- Adrian Cox
Humboldt Solutions Ltd.
CU
Michael.
Hi,
Quoted 20 lines · Adrian Cox
Quoted 9 lines
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_adapterstructure, but never callsi2c_add_adapter(). The only extra thing it needs to do is to initialise the semaphores in the structure.
2) The frontend callsi2c_transfer()directly.
3) The i2c core never gets involved, and there is never anyi2c_clientstructure.
Quoted 2 lines
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".
Quoted 2 lines
- Adrian Cox
Humboldt Solutions Ltd.
CU
Michael.
Quoted 11 lines · Michael Hunold
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.
Hi,
Quoted 12 lines · Greg KH
include/linux/i2c.h line 237
Quoted 5 lines · Michael Hunold
include/linux/i2c.h +4
-0+/* 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 heredrv->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.
Quoted 2 lines
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.
Quoted 7 lines
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 newNO_PROBEflag to structi2c_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,
Signature · 2 lines
Jean "Khali" Delvare http://khali.linux-fr.org/
Hi,
Quoted 11 lines · Jean Delvare
Quoted 5 lines
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 newNO_PROBEflag to structi2c_adapter, so a particular
adapter is never probed by anyoneI 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.
Quoted 4 lines
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.
Quoted 1 line
Thanks,
CU
Michael.
Quoted 7 lines · Michael Hunold
Quoted 4 lines
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.
Quoted 8 lines
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_PROBEflag 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
Quoted 10 lines · Greg KH
Quoted 3 lines · Michael Hunold
If we have a PCI card where we exactly know what we are doing, we can use the
NO_PROBEflag 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.
On Fri, 01 Oct 2004 13:22:45 +0100, Adrian Cox wrote
Quoted 10 lines
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.
Quoted 3 lines
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).
Quoted 5 lines
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.
Signature · 2 lines
Jean Delvare http://khali.linux-fr.org/
Quoted 22 lines · Adrian Cox
Quoted 10 lines · Greg KH
Quoted 3 lines · Michael Hunold
If we have a PCI card where we exactly know what we are doing, we can use the
NO_PROBEflag 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.
Quoted 3 lines
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