Hi Andy, On Sun, 15 Mar 2009 15:34:33 -0400, Andy Walls wrote: > On Sun, 2009-03-15 at 18:12 +0100, Jean Delvare wrote: > > Hi Hans, > > > > On Sun, 15 Mar 2009 13:44:01 +0100, Hans Verkuil wrote: > > > Hi Mauro, Jean, > > > > > > When converting the bttv driver to v4l2_subdev I found one probing conflict > > > between tvaudio and ir-kbd-i2c: address 0x96 (or 0x4b in 7-bit notation). > > > > > > It turns out that this is one and the same PIC16C54 device used on the > > > ProVideo PV951 board. This chip is used for both audio input selection and > > > for IR handling. > > > > > > But the tvaudio module does the audio part and the ir-kbd-i2c module does > > > the IR part. I have truly no idea how this should be handled in the new > > > situation. For that matter, I wonder whether it ever worked at all since my > > > understanding is that once you called i2c_attach_client for a particular > > > address, you cannot do that a second time. So depending on which module > > > happens to register itself first, you either have working audio or working > > > IR, but not both. > > > > You are right. > > > > > This is the typical multifunction device problem. It isn't specifically > > related to I2C, > > But the specific problem that Hans' brings up is precisely a Linux > kernel I2C subsystem *software* prohibition on two i2c_clients binding > to the same address on the same adapter. No. Once again: Linux doesn't support binding more than one driver to a device. This has _nothing_ to do with any choice done in i2c-core. > >From linux/drivers/i2c/i2c-core.c: > > static int __i2c_check_addr(struct device *dev, void *addrp) > { > struct i2c_client *client = i2c_verify_client(dev); > int addr = *(int *)addrp; > > if (client && client->addr == addr) > return -EBUSY; > return 0; > } > [...] > int i2c_attach_client(struct i2c_client *client) > { > struct i2c_adapter *adapter = client->adapter; > int res; > > /* Check for address business */ > res = i2c_check_addr(adapter, client->addr); > if (res) > return res; > [...] > > > It seems like an artificial restriction: intended for safety, but > getting in the way when something like that is a valid need. I think we could remove the check. But then the driver core would fail for us, so it wouldn't change anything in practice. You claim that the need is valid, but I disagree. Once a design is in place, you have to follow it, even if you come up with a case which doesn't fit in said design perfectly. Otherwise there is no point in having a design in the first place. Sure, a design can evolve, but not for just one case. And, in Linux' case, not for just one subsystem. The strength of the Linux driver model is that it is shared by all subsystems. That's not something which is going to change. > > the exact same problem happens for other devices, for > > example a PCI south bridge including hardware monitoring and SMBus, or > > a Super-I/O chip including hardware monitoring, parallel port, > > infrared, watchdog, etc. Linux currently only allows one driver to bind > > to a given device, so it becomes very difficult to make per-function > > drivers for such devices. > > > > I know that there was some work in progress to allow multiple drivers > > to bind to the same device. However it seems to be very slow because it > > is fundamentally incompatible with the device driver model as it was > > originally designed. > > The driver model outside of the I2C subsystem? Again this was in no way specific to the I2C subsystem. This was meant as an extension to the core device driver model. > Looking at the rest of i2c_attach_client() (that I didn't paste in > above), I dont' see how the call to device_register(&client->dev) would > care, as each i2c_client has it's own dev. Although I guess you might > get duplicately named sysfs directory entries like > > /sys/devices/.../i2c-adapter/i2c-3/3-0096 > > Which could be a problem for accessing via the sysfs filesystem. But > that could be fixed in i2c_attach_client? That's not just duplicated sysfs directories. That's duplicated device names. The "3-0096" above is how the driver core uniquely identifies the device in question withing the i2c subsystem name space. By definition you can't have two devices with the same identifier. (I am curious if this is a real device, BTW, as 0x96 isn't a valid 7-bit I2C address.) > Then there's a matter of accessing the I2C device only by the address > which means the wrong client might be used. But since they both point > to the same address on the same device, does that really matter? Of course it does matter. The whole point of having only one driver that can bind to a device is that said driver can control who accesses the device, and how, and take care of any needed locking or delays. Having multiple "clients" (software devices) accessing the same (physical) device is equivalent to not having any client at all. This would be a significant step backwards. > > In the meantime, one workaround is to list the multifunction device as > > supported by several drivers, and make the probe functions for this > > device fail, while still keeping a reference to the device. The > > reference lets you access the device, and is freed when you remove the > > drivers. See for example the via686a, vt8231 and i2c-viapro drivers. > > This approach may or may not be suitable for the ir-kbd-i2c and tvaudio > > drivers. One drawback is that you can't do power management on the > > device. > > To me it would be more forward looking to add support in the I2C > subsystem for allowing multiple client drivers to use the same address > on the same adapter, No, that's not going to happen in i2c-core, sorry. If anything like this happens, it will be implemented as part of the core driver model. > instead of adding non-intuitive behavior to module > probe routines as a workaround. Integration of discrete I2C chip cores > into multifunction devices is likely to be a continuing trend. I sure hope not. > The PCI subsystem handles single devices with multiple functions. In the good cases, yes. In some cases (which I have quoted already) it doesn't. But anyway PCI functions are an implementation detail irrelevant to this discussion: as far as Linux is concerned, each PCI function is a separate device, exactly as separate PCI devices. > There, of course, the function number is in the logical device address. Not sure what you mean here. As said above, Linux doesn't give a damn to PCI functions. They are an implementation detail. > For an single I2C chip with multiple functions, I've seen two types of > functional block separation provided: a separate I2C address per > functional block, and functions are separated by register address > ranges. The CX25843 leaps to mind as being of the second type. There > are register blocks for the basic device, the analog front end, the > consumer IR device, the video decoding, the broadcast audio decoding, > and AC97 interface functions. And there's nothing preventing you from handling these separate functions as you wish in your driver. Having a single function per i2c_client is not a requirement in general. > > As far as the PIC16C54 is concerned, another possibility would be to > > move support to a dedicated driver. Depending on how much code is common > > between the PIC16C54 and the other supported devices, the new driver > > may either be standalone, or rely on functions exported by the > > ir-kbd-i2c and tvaudio modules. > > I'll guess that solution is probably the path of least resistance for > the problem at hand. It seems like a workaround for design decision > made in the I2C subsystem long ago though. Third (and last) time: this is how the Linux device driver model was designed (and quite rightly so.) Not my decision and not i2c subsystem specific. The only i2c-specific thing here (and not my choice either) are the bus identifiers (e.g. 3-0096) but I believe it was a pretty natural choice given that a given I2C address can indeed only be used by one (physical) I2C slave on a given I2C segment. Instead of blaming me or the i2c subsystem, please look carefully at what the problem is in the first place: the ir-kbd-i2c and tvaudio drivers are horrible design errors (for which there are certainly historical errors, but still.) They support many devices each, on the basis that these devices have the same functionality. Can you imagine a single driver for all SATA controllers out there? Or a single driver for all RTC chips out there? This is the design mistake you are looking for. Good night, -- Jean Delvare -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html