bttv, tvaudio and ir-kbd-i2c probing conflict

bttv, tvaudio and ir-kbd-i2c probing conflict

[Hans Verkuil]

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.

It might work if you use lirc, since that uses low-level i2c access 
(right?). But I can't see how it can work with ir-kbd-i2c and tvaudio at 
the same time.

Did some googling and this seems to confirm my analysis:

http://lists.zerezo.com/video4linux/msg21328.html

Ideas on a postcard, please... :-)

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: bttv, tvaudio and ir-kbd-i2c probing conflict

[Jean Delvare]

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.

> It might work if you use lirc, since that uses low-level i2c access 
> (right?). But I can't see how it can work with ir-kbd-i2c and tvaudio at 
> the same time.

I don't know anything about lirc, so I can't comment on this.

> Did some googling and this seems to confirm my analysis:
> 
> http://lists.zerezo.com/video4linux/msg21328.html
> 
> Ideas on a postcard, please... :-)

This is the typical multifunction device problem. It isn't specifically
related to I2C, 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.

For very specific devices, it isn't necessarily a big problem. You can
simply make an all-in-one driver for that specific device. The real
problem is when the device in question is fully compatible with other
devices which only implement functionality A _and_ fully compatible with
other devices which only implement functionality B. You don't really
want to support functions A and B in the same driver if most devices
out there have either function but not both.

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.

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.

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.

But this is all said without having much knowledge of the bttv, tvaudio
and ir-kbd-i2c drivers, so I might as well be completely off track.

-- 
Jean Delvare

Re: bttv, tvaudio and ir-kbd-i2c probing conflict

[Trent Piepho]

On Sun, 15 Mar 2009, Jean Delvare wrote:
> On Sun, 15 Mar 2009 13:44:01 +0100, Hans Verkuil wrote:
> This is the typical multifunction device problem. It isn't specifically
> related to I2C, 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.
>
> For very specific devices, it isn't necessarily a big problem. You can
> simply make an all-in-one driver for that specific device. The real
> problem is when the device in question is fully compatible with other
> devices which only implement functionality A _and_ fully compatible with
> other devices which only implement functionality B. You don't really
> want to support functions A and B in the same driver if most devices
> out there have either function but not both.

You can also split the "device" into multiple devices.  Most SoCs have one
register block where all kinds of devices, from i2c controllers to network
adapters, exist.  This is shown to linux as many devices, rather than one
massive multifunction device.

Re: bttv, tvaudio and ir-kbd-i2c probing conflict

[Jean Delvare]

On Sun, 15 Mar 2009 10:42:41 -0700 (PDT), Trent Piepho wrote:
> On Sun, 15 Mar 2009, Jean Delvare wrote:
> > On Sun, 15 Mar 2009 13:44:01 +0100, Hans Verkuil wrote:
> > This is the typical multifunction device problem. It isn't specifically
> > related to I2C, 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.
> >
> > For very specific devices, it isn't necessarily a big problem. You can
> > simply make an all-in-one driver for that specific device. The real
> > problem is when the device in question is fully compatible with other
> > devices which only implement functionality A _and_ fully compatible with
> > other devices which only implement functionality B. You don't really
> > want to support functions A and B in the same driver if most devices
> > out there have either function but not both.
> 
> You can also split the "device" into multiple devices.  Most SoCs have one
> register block where all kinds of devices, from i2c controllers to network
> adapters, exist.  This is shown to linux as many devices, rather than one
> massive multifunction device.

It really depends on the device type. You can't split an I2C or PCI
device that way.

-- 
Jean Delvare

Re: bttv, tvaudio and ir-kbd-i2c probing conflict

[Mauro Carvalho Chehab]

On Sun, 15 Mar 2009 18:53:13 +0100
Jean Delvare <khali@linux-fr.org> wrote:

> On Sun, 15 Mar 2009 10:42:41 -0700 (PDT), Trent Piepho wrote:
> > On Sun, 15 Mar 2009, Jean Delvare wrote:
> > > On Sun, 15 Mar 2009 13:44:01 +0100, Hans Verkuil wrote:
> > > This is the typical multifunction device problem. It isn't specifically
> > > related to I2C, 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.
> > >
> > > For very specific devices, it isn't necessarily a big problem. You can
> > > simply make an all-in-one driver for that specific device. The real
> > > problem is when the device in question is fully compatible with other
> > > devices which only implement functionality A _and_ fully compatible with
> > > other devices which only implement functionality B. You don't really
> > > want to support functions A and B in the same driver if most devices
> > > out there have either function but not both.
> > 
> > You can also split the "device" into multiple devices.  Most SoCs have one
> > register block where all kinds of devices, from i2c controllers to network
> > adapters, exist.  This is shown to linux as many devices, rather than one
> > massive multifunction device.
> 
> It really depends on the device type. You can't split an I2C or PCI
> device that way.

In the case of PCI, you can. There are several devices where you have a PCI
bridge inside the chip. The PCI bus identifies such cases and create a PCI
sub-bus to handle this. This is what happens by default with cx88 drivers
(where we have up to 4 different PCI devices at the same chip) and with devices
with multiple bt848 chips, inside the same board.

Of course, a subdev information is attached inside the BUS address on this case.

I suspect that we may need to have something like this, in order to support
some complex devices that use I2C. It seems to be very common those days to
have a device using a subaddress to address different functions. With the
current approach, we need to bind two different things inside the same i2c
module, which is not good.


-

Getting back to the problem Hans discovered with PV951 this was introduced by 
date:        Sun Feb 22 01:59:34 2004 +0000

Yet, I'm not convinced is what Hans discovered is a board with two different
functions at the same address, or (what I think it is more likely), aboard with
two different setups. Something like:



Cheers,
Mauro

Re: bttv, tvaudio and ir-kbd-i2c probing conflict

[Mauro Carvalho Chehab]

On Sun, 15 Mar 2009 18:53:13 +0100
Jean Delvare <khali@linux-fr.org> wrote:

> On Sun, 15 Mar 2009 10:42:41 -0700 (PDT), Trent Piepho wrote:
> > On Sun, 15 Mar 2009, Jean Delvare wrote:
> > > On Sun, 15 Mar 2009 13:44:01 +0100, Hans Verkuil wrote:
> > > This is the typical multifunction device problem. It isn't specifically
> > > related to I2C, 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.
> > >
> > > For very specific devices, it isn't necessarily a big problem. You can
> > > simply make an all-in-one driver for that specific device. The real
> > > problem is when the device in question is fully compatible with other
> > > devices which only implement functionality A _and_ fully compatible with
> > > other devices which only implement functionality B. You don't really
> > > want to support functions A and B in the same driver if most devices
> > > out there have either function but not both.
> > 
> > You can also split the "device" into multiple devices.  Most SoCs have one
> > register block where all kinds of devices, from i2c controllers to network
> > adapters, exist.  This is shown to linux as many devices, rather than one
> > massive multifunction device.
> 
> It really depends on the device type. You can't split an I2C or PCI
> device that way.

In the case of PCI, you can. There are several devices where you have a PCI
bridge inside the chip. The PCI bus identifies such cases and create a PCI
sub-bus to handle this. This is what happens by default with cx88 drivers
(where we have up to 4 different PCI devices at the same chip) and with devices
with multiple bt848 chips, inside the same board.

Of course, a subdev information is attached inside the BUS address on this case.

I suspect that we may need to have something like this, in order to support
some complex devices that use I2C. It seems to be very common those days to
have a device using a subaddress to address different functions. 

With the current approach, we need to bind two completely different things
inside the same i2c module, which may result in a very poor design.

-

Getting back to the problem Hans discovered with PV951, This board was
introduced by the initial CVS commit that populates the tree, dated as Sun Feb
22 01:59:34 2004 +0000, on changeset#784.

So, this is more ancient than 2.6.11. This driver were probably added during
2.5 development cycle or even before, together with i2c introduction in kernel.
So, I really don't doubt that on that time it were possible to have two boards
sharing the same address. To be sure, when this were added, we would need to
followup the Kernel past history before -git.

By looking at tvaudio, it really seems that they used the same I2C address, but
2 different I2C subaddress for two completely independent things:

+/* the registers of 16C54, I2C sub address. */
+#define PIC16C54_REG_KEY_CODE     0x01        /* Not use. */
+#define PIC16C54_REG_MISC         0x02

It seems clear by the code that address 0x48 subaddress 0x01 is for IR and address
0x48 subaddress 0x02 is for audio.

Since the Input subsystem is something completely independent from the audio
one, and even agreeding that generic modules like tvaudio and i2c-ir-kbd are
not the proper way, we shouldn't mix the input susbystem stuff with audio at
the same driver. Those are completely unrelated things. The proper design would
be to have one different driver for each address/subaddress pair.

So, in this case, I2C should not expose this driver as:

/sys/devices/.../i2c-adapter/i2c-3/3-0096

But, instead, create a subbus, just like PCI, exposing it as:

/sys/devices/.../i2c-adapter/i2c-3/3-0096-1
/sys/devices/.../i2c-adapter/i2c-3/3-0096-2

And letting to bind one module at address 0096-1 (for IR) and another at 0096-2 (for audio).

Cheers,
Mauro

Re: bttv, tvaudio and ir-kbd-i2c probing conflict

[Jean Delvare]

Hi Mauro,

On Mon, 16 Mar 2009 06:34:02 -0300, Mauro Carvalho Chehab wrote:
> On Sun, 15 Mar 2009 18:53:13 +0100
> Jean Delvare <khali@linux-fr.org> wrote:
> > On Sun, 15 Mar 2009 10:42:41 -0700 (PDT), Trent Piepho wrote:
> > > You can also split the "device" into multiple devices.  Most SoCs have one
> > > register block where all kinds of devices, from i2c controllers to network
> > > adapters, exist.  This is shown to linux as many devices, rather than one
> > > massive multifunction device.
> > 
> > It really depends on the device type. You can't split an I2C or PCI
> > device that way.
> 
> In the case of PCI, you can. There are several devices where you have a PCI
> bridge inside the chip. The PCI bus identifies such cases and create a PCI
> sub-bus to handle this. This is what happens by default with cx88 drivers
> (where we have up to 4 different PCI devices at the same chip) and with devices
> with multiple bt848 chips, inside the same board.

Sorry for not being clear. I really meant "PCI device" as in "what
Linux considers a PCI device", which is a PCI function at the PCI bus
level. I didn't mean "PCI device" as in "TV card which plugs into a PCI
slot". I am fully aware that TV cards can show up as several entries in
the output of lspci.

Yeah, the term "device" can mean many different things and it can get
very confusing at times :(

> Of course, a subdev information is attached inside the BUS address on this case.
> 
> I suspect that we may need to have something like this, in order to support
> some complex devices that use I2C. It seems to be very common those days to
> have a device using a subaddress to address different functions. 
> 
> With the current approach, we need to bind two completely different things
> inside the same i2c module, which may result in a very poor design.

I really don't see any problem there. There are many drivers (i2c or
not) in the kernel which do exactly this and this works just fine. We
even have a directory for (some of) them: drivers/mfd. For other
drivers, we either have broader categories in Linux, or we put the
driver in the category which is the more meaningful. For example, many
hwmon drivers include several functions: voltage monitoring,
temperature monitoring, fan speed monitoring, fan speed control (either
manual or automatic), chassis intrusion detection, etc. Some even
include a watchdog. We have decided to put all these drivers under
drivers/hwmon, and this didn't cause any problem so far. Some other
chips have hardware monitoring has a secondary feature and as a result
they live in other directories but still register has a hwmon (class)
device. Again, nothing wrong with that.

While I agree that in an ideal world each device would serve just one
function so we can put each driver in the right directory and have many
small and easy to maintain drivers, in the case where we have a
multi-function device, the solutions to handle it already exist.

> Getting back to the problem Hans discovered with PV951, This board was
> introduced by the initial CVS commit that populates the tree, dated as Sun Feb
> 22 01:59:34 2004 +0000, on changeset#784.
> 
> So, this is more ancient than 2.6.11. This driver were probably added during
> 2.5 development cycle or even before, together with i2c introduction in kernel.
> So, I really don't doubt that on that time it were possible to have two boards
> sharing the same address. To be sure, when this were added, we would need to
> followup the Kernel past history before -git.

You can just look at Thomas Gleixner's excellent history tree:
http://git.kernel.org/?p=linux/kernel/git/tglx/history.git;a=summary

> By looking at tvaudio, it really seems that they used the same I2C address, but
> 2 different I2C subaddress for two completely independent things:
> 
> +/* the registers of 16C54, I2C sub address. */
> +#define PIC16C54_REG_KEY_CODE     0x01        /* Not use. */
> +#define PIC16C54_REG_MISC         0x02
> 
> It seems clear by the code that address 0x48 subaddress 0x01 is for IR and address
> 0x48 subaddress 0x02 is for audio.
> 
> Since the Input subsystem is something completely independent from the audio
> one, and even agreeding that generic modules like tvaudio and i2c-ir-kbd are
> not the proper way, we shouldn't mix the input susbystem stuff with audio at
> the same driver. Those are completely unrelated things. The proper design would
> be to have one different driver for each address/subaddress pair.

This is one possible design, yes. Having both functions handled by the
same driver (and I really wrote driver, as in struct i2c_driver, and
not module) is another possible design, and honestly I don't see any
problem with it. You are totally free to put the separate functions
into separate source files or even separate modules to keep the code
clean. Then all you need is some glue and exported functions to make
all pieces work together.

> So, in this case, I2C should not expose this driver as:
> 
> /sys/devices/.../i2c-adapter/i2c-3/3-0096
> 
> But, instead, create a subbus, just like PCI, exposing it as:

PCI doesn't do this, no. The way each device organizes its register map
(which is really what we are talking about here) has _nothing_ to do
with bus topology.

> /sys/devices/.../i2c-adapter/i2c-3/3-0096-1
> /sys/devices/.../i2c-adapter/i2c-3/3-0096-2
> 
> And letting to bind one module at address 0096-1 (for IR) and another at 0096-2 (for audio).

I fear this is never going to happen. We have a model which works
today, you propose to change it for another one which would work too.
Without an excellent reason to justify this change, I can't see why we
would change anything here at the cost of a compatibility breakage.

Also, please let's not lose focus here. The important thing here is to
finish the conversion to the new i2c device driver binding model, and
quickly.

Thanks,
-- 
Jean Delvare

Re: bttv, tvaudio and ir-kbd-i2c probing conflict

[Mauro Carvalho Chehab]

On Mon, 16 Mar 2009 12:18:01 +0100
Jean Delvare <khali@linux-fr.org> wrote:

> > With the current approach, we need to bind two completely different things
> > inside the same i2c module, which may result in a very poor design.
> 
> I really don't see any problem there. There are many drivers (i2c or
> not) in the kernel which do exactly this and this works just fine. We
> even have a directory for (some of) them: drivers/mfd. For other
> drivers, we either have broader categories in Linux, or we put the
> driver in the category which is the more meaningful. For example, many
> hwmon drivers include several functions: voltage monitoring,
> temperature monitoring, fan speed monitoring, fan speed control (either
> manual or automatic), chassis intrusion detection, etc. Some even
> include a watchdog. We have decided to put all these drivers under
> drivers/hwmon, and this didn't cause any problem so far. Some other
> chips have hardware monitoring has a secondary feature and as a result
> they live in other directories but still register has a hwmon (class)
> device. Again, nothing wrong with that.

In the case of hwmon, it is all about hardware monitoring, so, it is fine to
have all of them grouped.

This is not the case of an input device, and an audio DSP control for some TV
standard.

The input device is a handler for EVDEV events, that has their own dependencies
and needs, while the audio DSP control is part of a group of functions to setup
a certain video/audio standard. You don't need to be streaming a video to use
your IR. 

For IR to work, a driver generally just needs to setup a few GPIO pins at the
driver, and do a glue with I2C bus (or GPIO bus). 

Several designed manufacturer drivers for other O. S. use separate drivers for
IR and for video, since those are completely independent functions just sharing
a very limited set of the chipset resources (the worse case is generally
needing to share up to 4 GPIO registers, the I2C bus and a IRQ line).

It is about the same we had in the past where some audio boards that also had a CD
controller inside (those legacy Sound Blaster devices, for example). The proper 
design were to map, the audio driver to be independent of the ISA driver.

We should really isolate things. On an ideal design, I'd say that we should
have a completely independent bttv-ir driver splitted from the bttv-video,
since someone can use the board just for IR interface, or just video. 

The I2C layer need to be capable of supporting such hardware abstraction level,
instead of forcing to use the same driver for both things. In fact, it does
support the needed abstraction, if the I2C addresses of the two hardwares are
different, but it fails if the difference is at the subaddress level.

> While I agree that in an ideal world each device would serve just one
> function so we can put each driver in the right directory and have many
> small and easy to maintain drivers, in the case where we have a
> multi-function device, the solutions to handle it already exist.
> 
> > Getting back to the problem Hans discovered with PV951, This board was
> > introduced by the initial CVS commit that populates the tree, dated as Sun Feb
> > 22 01:59:34 2004 +0000, on changeset#784.
> > 
> > So, this is more ancient than 2.6.11. This driver were probably added during
> > 2.5 development cycle or even before, together with i2c introduction in kernel.
> > So, I really don't doubt that on that time it were possible to have two boards
> > sharing the same address. To be sure, when this were added, we would need to
> > followup the Kernel past history before -git.
> 
> You can just look at Thomas Gleixner's excellent history tree:
> http://git.kernel.org/?p=linux/kernel/git/tglx/history.git;a=summary

I know. 

> > By looking at tvaudio, it really seems that they used the same I2C address, but
> > 2 different I2C subaddress for two completely independent things:
> > 
> > +/* the registers of 16C54, I2C sub address. */
> > +#define PIC16C54_REG_KEY_CODE     0x01        /* Not use. */
> > +#define PIC16C54_REG_MISC         0x02
> > 
> > It seems clear by the code that address 0x48 subaddress 0x01 is for IR and address
> > 0x48 subaddress 0x02 is for audio.
> > 
> > Since the Input subsystem is something completely independent from the audio
> > one, and even agreeding that generic modules like tvaudio and i2c-ir-kbd are
> > not the proper way, we shouldn't mix the input susbystem stuff with audio at
> > the same driver. Those are completely unrelated things. The proper design would
> > be to have one different driver for each address/subaddress pair.
> 
> This is one possible design, yes. Having both functions handled by the
> same driver (and I really wrote driver, as in struct i2c_driver, and
> not module) is another possible design, and honestly I don't see any
> problem with it. You are totally free to put the separate functions
> into separate source files or even separate modules to keep the code
> clean. Then all you need is some glue and exported functions to make
> all pieces work together.

So, it seems that this is the more direct alternative for that pic processor.

> > So, in this case, I2C should not expose this driver as:
> > 
> > /sys/devices/.../i2c-adapter/i2c-3/3-0096
> > 
> > But, instead, create a subbus, just like PCI, exposing it as:
> 
> PCI doesn't do this, no. The way each device organizes its register map
> (which is really what we are talking about here) has _nothing_ to do
> with bus topology.

This is the way PCI maps the devices on a machine I have here with one saa7131
board and one 4x bt878 board:

lspci output:

01:02.0 Multimedia controller: Philips Semiconductors SAA7131/SAA7133/SAA7135 Video Broadcast Decoder (rev d1)
02:0c.0 Multimedia video controller: Brooktree Corporation Bt878 Video Capture (rev 11)
02:0c.1 Multimedia controller: Brooktree Corporation Bt878 Audio Capture (rev 11)
02:0d.0 Multimedia video controller: Brooktree Corporation Bt878 Video Capture (rev 11)
02:0d.1 Multimedia controller: Brooktree Corporation Bt878 Audio Capture (rev 11)
02:0e.0 Multimedia video controller: Brooktree Corporation Bt878 Video Capture (rev 11)
02:0e.1 Multimedia controller: Brooktree Corporation Bt878 Audio Capture (rev 11)
02:0f.0 Multimedia video controller: Brooktree Corporation Bt878 Video Capture (rev 11)
02:0f.1 Multimedia controller: Brooktree Corporation Bt878 Audio Capture (rev 11)

v4l2-apps/util/v4l2_sysfs_path output:

device     = /dev/video4
bus info   = PCI:0000:01:02.0
sysfs path = /sys/devices/pci0000:00/0000:00:1e.0/0000:01:02.0

device     = /dev/video3
bus info   = PCI:0000:02:0f.0
sysfs path = /sys/devices/pci0000:00/0000:00:1e.0/0000:01:01.0/0000:02:0f.0

device     = /dev/video2
bus info   = PCI:0000:02:0e.0
sysfs path = /sys/devices/pci0000:00/0000:00:1e.0/0000:01:01.0/0000:02:0e.0

device     = /dev/video1
bus info   = PCI:0000:02:0d.0
sysfs path = /sys/devices/pci0000:00/0000:00:1e.0/0000:01:01.0/0000:02:0d.0

device     = /dev/video0
bus info   = PCI:0000:02:0c.0
sysfs path = /sys/devices/pci0000:00/0000:00:1e.0/0000:01:01.0/0000:02:0c.0


You should notice that, even physically having all 4 bttv devices at the same
slot (physically at pci0000:00/0000:00:1e.0/0000:01:01.0), PCI has created unique
representation and sysfs names for each bttv device. The same occurs with
devices like cx88, that has 4 pci logical devices inside the plastic.

This is what I'm meaning.

> 
> > /sys/devices/.../i2c-adapter/i2c-3/3-0096-1
> > /sys/devices/.../i2c-adapter/i2c-3/3-0096-2
> > 
> > And letting to bind one module at address 0096-1 (for IR) and another at 0096-2 (for audio).
> 
> I fear this is never going to happen. We have a model which works
> today, you propose to change it for another one which would work too.
> Without an excellent reason to justify this change, I can't see why we
> would change anything here at the cost of a compatibility breakage.

Well, I could use the same argument for all those conversions done at the V4L
drivers. 

I never saw any excellent reason to migrate the existing board support to the
model you're proposing: the current model works properly for the almost all
the supported devices. 

A large amount of changes that are needed to be done, on a very short interval
will almost certainly cause compatibility breakages. I can't see any gain to
the end user of a board that it is properly supported that such change would do.

The reasons I see are in order to provide a more consistent internal model
to represent the devices, and to support devices with more complex approaches
like devices that have some I2C muxes and switches on their buses, to solve the
address problems we're currently facing with such devices.

> Also, please let's not lose focus here. The important thing here is to
> finish the conversion to the new i2c device driver binding model, and
> quickly.

For finishing the conversion, I agree that we just need some kind of workaround
to allow both IR and Audio to work, but we shouldn't loose how it would be done
in the final version.

Cheers,
Mauro

Re: bttv, tvaudio and ir-kbd-i2c probing conflict

[Jean Delvare]

Hi Mauro,

On Mon, 16 Mar 2009 09:52:37 -0300, Mauro Carvalho Chehab wrote:
> On Mon, 16 Mar 2009 12:18:01 +0100
> Jean Delvare <khali@linux-fr.org> wrote:
> > I really don't see any problem there. There are many drivers (i2c or
> > not) in the kernel which do exactly this and this works just fine. We
> > even have a directory for (some of) them: drivers/mfd. For other
> > drivers, we either have broader categories in Linux, or we put the
> > driver in the category which is the more meaningful. For example, many
> > hwmon drivers include several functions: voltage monitoring,
> > temperature monitoring, fan speed monitoring, fan speed control (either
> > manual or automatic), chassis intrusion detection, etc. Some even
> > include a watchdog. We have decided to put all these drivers under
> > drivers/hwmon, and this didn't cause any problem so far. Some other
> > chips have hardware monitoring has a secondary feature and as a result
> > they live in other directories but still register has a hwmon (class)
> > device. Again, nothing wrong with that.
> 
> In the case of hwmon, it is all about hardware monitoring, so, it is fine to
> have all of them grouped.

Absolutely not. These are totally separate functions. There's really
nothing in common between ensuring that the PSU is in good health and
controlling the speed of the fans.

> This is not the case of an input device, and an audio DSP control for some TV
> standard.

I don't make any difference. It's all multimedia stuff to me.

See how subjective drawing this kind of lines can be? ;)

> The input device is a handler for EVDEV events, that has their own dependencies
> and needs, while the audio DSP control is part of a group of functions to setup
> a certain video/audio standard. You don't need to be streaming a video to use
> your IR. 
> 
> For IR to work, a driver generally just needs to setup a few GPIO pins at the
> driver, and do a glue with I2C bus (or GPIO bus). 
> 
> Several designed manufacturer drivers for other O. S. use separate drivers for
> IR and for video, since those are completely independent functions just sharing
> a very limited set of the chipset resources (the worse case is generally
> needing to share up to 4 GPIO registers, the I2C bus and a IRQ line).
> 
> It is about the same we had in the past where some audio boards that also had a CD
> controller inside (those legacy Sound Blaster devices, for example). The proper 
> design were to map, the audio driver to be independent of the ISA driver.
> 
> We should really isolate things. On an ideal design, I'd say that we should
> have a completely independent bttv-ir driver splitted from the bttv-video,
> since someone can use the board just for IR interface, or just video.

I agree, but then again this is nothing you can't already do today.
It's not the most frequent driver design, but it can be done. There is
nothing preventing you from passing i2c_client references to other
modules, which can then do whatever they want with them.

> The I2C layer need to be capable of supporting such hardware abstraction level,
> instead of forcing to use the same driver for both things. In fact, it does
> support the needed abstraction, if the I2C addresses of the two hardwares are
> different, but it fails if the difference is at the subaddress level.

No, the I2C layer doesn't need to be capable of supporting such
hardware abstraction level. I claim it isn't the I2C layer's job to deal
with how each device maps its registers nor how many functions an I2C
device implements. This is a driver-specific issue.

The I2C layer must make it _possible_ to write drivers for such
devices, yes. And I think it already does. If there are problems,
please point me to them, and we'll address them. But this won't take a
redesign of the i2c subsystem.

> > (...)
> > PCI doesn't do this, no. The way each device organizes its register map
> > (which is really what we are talking about here) has _nothing_ to do
> > with bus topology.
> 
> This is the way PCI maps the devices on a machine I have here with one saa7131
> board and one 4x bt878 board:
> 
> lspci output:
> 
> 01:02.0 Multimedia controller: Philips Semiconductors SAA7131/SAA7133/SAA7135 Video Broadcast Decoder (rev d1)
> 02:0c.0 Multimedia video controller: Brooktree Corporation Bt878 Video Capture (rev 11)
> 02:0c.1 Multimedia controller: Brooktree Corporation Bt878 Audio Capture (rev 11)
> 02:0d.0 Multimedia video controller: Brooktree Corporation Bt878 Video Capture (rev 11)
> 02:0d.1 Multimedia controller: Brooktree Corporation Bt878 Audio Capture (rev 11)
> 02:0e.0 Multimedia video controller: Brooktree Corporation Bt878 Video Capture (rev 11)
> 02:0e.1 Multimedia controller: Brooktree Corporation Bt878 Audio Capture (rev 11)
> 02:0f.0 Multimedia video controller: Brooktree Corporation Bt878 Video Capture (rev 11)
> 02:0f.1 Multimedia controller: Brooktree Corporation Bt878 Audio Capture (rev 11)
> 
> v4l2-apps/util/v4l2_sysfs_path output:
> 
> device     = /dev/video4
> bus info   = PCI:0000:01:02.0
> sysfs path = /sys/devices/pci0000:00/0000:00:1e.0/0000:01:02.0
> 
> device     = /dev/video3
> bus info   = PCI:0000:02:0f.0
> sysfs path = /sys/devices/pci0000:00/0000:00:1e.0/0000:01:01.0/0000:02:0f.0
> 
> device     = /dev/video2
> bus info   = PCI:0000:02:0e.0
> sysfs path = /sys/devices/pci0000:00/0000:00:1e.0/0000:01:01.0/0000:02:0e.0
> 
> device     = /dev/video1
> bus info   = PCI:0000:02:0d.0
> sysfs path = /sys/devices/pci0000:00/0000:00:1e.0/0000:01:01.0/0000:02:0d.0
> 
> device     = /dev/video0
> bus info   = PCI:0000:02:0c.0
> sysfs path = /sys/devices/pci0000:00/0000:00:1e.0/0000:01:01.0/0000:02:0c.0
> 
> 
> You should notice that, even physically having all 4 bttv devices at the same
> slot (physically at pci0000:00/0000:00:1e.0/0000:01:01.0), PCI has created unique
> representation and sysfs names for each bttv device. The same occurs with
> devices like cx88, that has 4 pci logical devices inside the plastic.
> 
> This is what I'm meaning.

If you like to keep the analogy between I2C and PCI, PCI bridges (which
is what you have above) are like I2C multiplexers. Again, nothing to do
with the internal register mapping of I2C devices.

> > (...)
> > I fear this is never going to happen. We have a model which works
> > today, you propose to change it for another one which would work too.
> > Without an excellent reason to justify this change, I can't see why we
> > would change anything here at the cost of a compatibility breakage.
> 
> Well, I could use the same argument for all those conversions done at the V4L
> drivers.

This was more or less my point, but apparently I didn't stress it
enough. I am impressed by the way you ask me to basically redesign the
i2c subsystem to take care of one specific I2C device which you think
doesn't fit exactly in the current model, while OTOH you are doing all
you can to slow down an on-going subsystem redesign that has been
requested by many for years, and that solves a dozen real-world
problems, and that is already 85% done.

> I never saw any excellent reason to migrate the existing board support to the
> model you're proposing: the current model works properly for the almost all
> the supported devices.

That's it, you made my day, really.

At this point I'm not sure if you really have a bad memory (worse than
mine), or if you are trying to make fun of me, or if you are just
wasting my time.

Come on, just look at ir-kbd-i2c and tvaudio again, see how great are
these drivers which have been "designed" on top of the legacy i2c
binding model. Look at the bttv mess. Look at the zoran driver
conversion done by Hans a few weeks ago, which killed what, 3000 lines
of code? The old binding model was so bad that DVB doesn't even use it.
They decided to go with raw bus transactions instead, despite the
potential problems theses have.

The new binding model solves so many issues that I don't even know
where to start. It makes drivers load much faster. It prevents device
misdetections. It prevents undue probes which have occasionally been
reported to break hardware. It kills a lot of glue code that had been
added to workaround the weakness of the old binding model. It makes it
much easier for developers to understand the i2c code because it now
follows the standard model.

I can't believe that you claim that I am pushing the new model on you
when you don't want it, when you (not necessarily you personally, but
you video4linux developers) are one of the two groups for which the
redesign happened in the first place. You asked for it, don't you
remember? With more spare time I'd go dig my mail archive.

> A large amount of changes that are needed to be done, on a very short interval

Kernel 2.6.22 was released in July 2007. This is 20 months ago. Is that
what you call a short interval? In the Linux development time frame,
this is a long interval.

Ah yeah, now I remember. You're the one who considers 2.6.18 a bleeding
edge kernel suitable for upstream development. This explains a lot.

> will almost certainly cause compatibility breakages. I can't see any gain to
> the end user of a board that it is properly supported that such change would do.

I'm literally speechless.

> The reasons I see are in order to provide a more consistent internal model
> to represent the devices, and to support devices with more complex approaches
> like devices that have some I2C muxes and switches on their buses, to solve the
> address problems we're currently facing with such devices.

Abstracting the numerous improvements brought by the new binding model
for older devices, which by themselves justify the move IMHO, the key
issue here is that the old model and the new model do not mix properly.
The different device lifetime cycles make it pretty much impossible to
come up with a sane locking model. This is the reason why I want the
legacy model to die now.

> > Also, please let's not lose focus here. The important thing here is to
> > finish the conversion to the new i2c device driver binding model, and
> > quickly.
> 
> For finishing the conversion, I agree that we just need some kind of workaround
> to allow both IR and Audio to work, but we shouldn't loose how it would be done
> in the final version.

For finishing the conversion, we don't need to do anything. The
PIC16C54 support is already broken today if I understood Hans
correctly. We certainly want to fix that someday, but this is unrelated
to the model conversion.

-- 
Jean Delvare

Re: bttv, tvaudio and ir-kbd-i2c probing conflict

[Mauro Carvalho Chehab]

I need to be short, since I'm about to travel.

On Mon, 16 Mar 2009 15:28:02 +0100
Jean Delvare <khali@linux-fr.org> wrote:


> Ah yeah, now I remember. You're the one who considers 2.6.18 a bleeding
> edge kernel suitable for upstream development. This explains a lot.

I have 3 separated environments, being the first with the bleeding edge Fedora
rawhide core (2.6.29-rc-git + several linux-next patches), for development. For
sure, this is not stable, and an intermediate with 2.6.27 (Fedora 10), for normal tests.

But for sure I don't want to loose time or data running my production data on
an unstable environment. Then, I use RHEL5 on my main machines: it is rock solid.

On all of those environments, I need my devices properly working with the
bleeding edge v4l/dvb drivers.
> 
> > will almost certainly cause compatibility breakages. I can't see any gain to
> > the end user of a board that it is properly supported that such change would do.
> 
> I'm literally speechless.

What's the gain of the change for a user whose card already works? If
everything went ok, he will just have what he had before. The difference
affects only users of boards not supported yet (or badly supported).

> > The reasons I see are in order to provide a more consistent internal model
> > to represent the devices, and to support devices with more complex approaches
> > like devices that have some I2C muxes and switches on their buses, to solve the
> > address problems we're currently facing with such devices.
> 
> Abstracting the numerous improvements brought by the new binding model
> for older devices, which by themselves justify the move IMHO, the key
> issue here is that the old model and the new model do not mix properly.
> The different device lifetime cycles make it pretty much impossible to
> come up with a sane locking model. This is the reason why I want the
> legacy model to die now.

Ok, that's the reason why we're changing: we need the new model for a small
subset of devices (5%? maybe even less). On those devices, I2C switches do
exist and this causes troubles with the old model. Since both models don't mix,
we're migrating even the drivers that won't need such model.

> > > Also, please let's not lose focus here. The important thing here is to
> > > finish the conversion to the new i2c device driver binding model, and
> > > quickly.
> > 
> > For finishing the conversion, I agree that we just need some kind of workaround
> > to allow both IR and Audio to work, but we shouldn't loose how it would be done
> > in the final version.
> 
> For finishing the conversion, we don't need to do anything. The
> PIC16C54 support is already broken today if I understood Hans
> correctly. We certainly want to fix that someday, but this is unrelated
> to the model conversion.

It may or may not work. With the current behaviour, someone may load either
driver (IR or audio). So, both things work, but not simultaneously.

Anyway, you got me wrong: I don't think that pic16c54 is an enough reason for
justify any changes on the i2c model.

However, there are several devices that have processors connected via i2c
(including several DVB based devices using Cypress processors). IMO, we will
need sooner or later some solution for using the sub-address as a different
device, but we may postpone such discussion to another time.

Cheers,
Mauro

Re: bttv, tvaudio and ir-kbd-i2c probing conflict

[Trent Piepho]

On Mon, 16 Mar 2009, Jean Delvare wrote:
> Come on, just look at ir-kbd-i2c and tvaudio again, see how great are
> these drivers which have been "designed" on top of the legacy i2c
> binding model. Look at the bttv mess. Look at the zoran driver
> conversion done by Hans a few weeks ago, which killed what, 3000 lines
> of code? The old binding model was so bad that DVB doesn't even use it.

IIRC, the zoran patch removed more like 1000 lines.  But it also deleted
v4l1 support, highmem support, and bigphys_area support.  Maybe other
things, Hans doesn't decribe his patches, so there's really no way to know
what the zoran patch really did other than to weed through 10,000+ lines of
diff which is mostly but not entirely moving blocks of code from one space
to another and reindenting them.

If one includes the v4l1-compat module that is now providing v4l1 support
(though it doesn't work correctly for zoran), the driver and the compat
module are larger than the old driver was.  Of course one can now turn off
v4l1 support and get a smaller driver than before.  And the v4l1 compat
already existed and can be shared.  But I think it's more correct to say
the size reduction of the zoran driver was from removing features and not
from v4l2_subdev.  It seems like more of the the other subdev conversions
have overall added more code than they removed.

Re: bttv, tvaudio and ir-kbd-i2c probing conflict

[Jean Delvare]

On Mon, 16 Mar 2009 12:43:33 -0700 (PDT), Trent Piepho wrote:
> On Mon, 16 Mar 2009, Jean Delvare wrote:
> > Come on, just look at ir-kbd-i2c and tvaudio again, see how great are
> > these drivers which have been "designed" on top of the legacy i2c
> > binding model. Look at the bttv mess. Look at the zoran driver
> > conversion done by Hans a few weeks ago, which killed what, 3000 lines
> > of code? The old binding model was so bad that DVB doesn't even use it.
> 
> IIRC, the zoran patch removed more like 1000 lines.

You don't remember correctly:
 37 files changed, 4570 insertions(+), 7404 deletions(-)
That's 2834 lines. 1570 of which is for the removal of the saa7111 and
saa7114 and I agree this shouldn't count, so we're down to 1264 lines
removed. The removal of some "features" also shouldn't count but I
doubt it amounts to more than a few hundred lines.

In fact, to get a more accurate figure you can just look at the result
of my own conversion. There were two patches:
 13 files changed, 510 insertions(+), 625 deletions(-)
 11 files changed, 549 deletions(-)
This is -664 lines total. The exact number doesn't really matter. The
bottom line is that the new binding model requires significantly less
glue code than the legacy model. If you think some more about it, it
means a lot.

> But it also deleted
> v4l1 support, highmem support, and bigphys_area support.  Maybe other
> things, Hans doesn't decribe his patches, so there's really no way to know
> what the zoran patch really did other than to weed through 10,000+ lines of
> diff which is mostly but not entirely moving blocks of code from one space
> to another and reindenting them.

You are unfair. The pull request came with a short log of all the
changes.

> If one includes the v4l1-compat module that is now providing v4l1 support
> (though it doesn't work correctly for zoran), the driver and the compat
> module are larger than the old driver was.  Of course one can now turn off
> v4l1 support and get a smaller driver than before.  And the v4l1 compat
> already existed and can be shared.  But I think it's more correct to say
> the size reduction of the zoran driver was from removing features and not
> from v4l2_subdev.  It seems like more of the the other subdev conversions
> have overall added more code than they removed.

I am not familiar enough with this part of the code to say. But I guess
it doesn't really matter, as it wasn't my point anyway.

-- 
Jean Delvare

Re: bttv, tvaudio and ir-kbd-i2c probing conflict

[Hans Verkuil]

On Monday 16 March 2009 22:40:40 Jean Delvare wrote:
> On Mon, 16 Mar 2009 12:43:33 -0700 (PDT), Trent Piepho wrote:
> > On Mon, 16 Mar 2009, Jean Delvare wrote:
> > > Come on, just look at ir-kbd-i2c and tvaudio again, see how great are
> > > these drivers which have been "designed" on top of the legacy i2c
> > > binding model. Look at the bttv mess. Look at the zoran driver
> > > conversion done by Hans a few weeks ago, which killed what, 3000
> > > lines of code? The old binding model was so bad that DVB doesn't even
> > > use it.
> >
> > IIRC, the zoran patch removed more like 1000 lines.
>
> You don't remember correctly:
>  37 files changed, 4570 insertions(+), 7404 deletions(-)
> That's 2834 lines. 1570 of which is for the removal of the saa7111 and
> saa7114 and I agree this shouldn't count, so we're down to 1264 lines
> removed. The removal of some "features" also shouldn't count but I
> doubt it amounts to more than a few hundred lines.
>
> In fact, to get a more accurate figure you can just look at the result
> of my own conversion. There were two patches:
>  13 files changed, 510 insertions(+), 625 deletions(-)
>  11 files changed, 549 deletions(-)
> This is -664 lines total. The exact number doesn't really matter. The
> bottom line is that the new binding model requires significantly less
> glue code than the legacy model. If you think some more about it, it
> means a lot.
>
> > But it also deleted
> > v4l1 support, highmem support, and bigphys_area support.  Maybe other
> > things, Hans doesn't decribe his patches, so there's really no way to
> > know what the zoran patch really did other than to weed through 10,000+
> > lines of diff which is mostly but not entirely moving blocks of code
> > from one space to another and reindenting them.
>
> You are unfair. The pull request came with a short log of all the
> changes.
>
> > If one includes the v4l1-compat module that is now providing v4l1
> > support (though it doesn't work correctly for zoran), the driver and
> > the compat module are larger than the old driver was.  Of course one
> > can now turn off v4l1 support and get a smaller driver than before. 
> > And the v4l1 compat already existed and can be shared.  But I think
> > it's more correct to say the size reduction of the zoran driver was
> > from removing features and not from v4l2_subdev.  It seems like more of
> > the the other subdev conversions have overall added more code than they
> > removed.
>
> I am not familiar enough with this part of the code to say. But I guess
> it doesn't really matter, as it wasn't my point anyway.

My experience is that i2c drivers shrink in size while adapter drivers 
increase slightly in size. Of course, the intention is to use this new 
framework in the mid- to long-term to move common functionality out of the 
drivers into the core, thus shrinking the drivers (although increasing the 
core) and improving robustness, consistency and reliability.

I'm also not really concerned about increasing driver size (within reason): 
we're talking about drivers that need to allocate megabytes of framebuffer 
memory anyway. Improving code quality, robustness, consistency and 
maintainability is much more important than saving a few (or even more than 
a few) kB. Obviously, if with a little effort you can gain substantial 
memory savings, like you did with bttv, then that's definitely a good 
thing.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: bttv, tvaudio and ir-kbd-i2c probing conflict

[Trent Piepho]

On Mon, 16 Mar 2009, Jean Delvare wrote:
> On Mon, 16 Mar 2009 12:43:33 -0700 (PDT), Trent Piepho wrote:
> > On Mon, 16 Mar 2009, Jean Delvare wrote:
> > > Come on, just look at ir-kbd-i2c and tvaudio again, see how great are
> > > these drivers which have been "designed" on top of the legacy i2c
> > > binding model. Look at the bttv mess. Look at the zoran driver
> > > conversion done by Hans a few weeks ago, which killed what, 3000 lines
> > > of code? The old binding model was so bad that DVB doesn't even use it.
> >
> > IIRC, the zoran patch removed more like 1000 lines.
>
> You don't remember correctly:
>  37 files changed, 4570 insertions(+), 7404 deletions(-)
> That's 2834 lines. 1570 of which is for the removal of the saa7111 and
> saa7114 and I agree this shouldn't count, so we're down to 1264 lines
> removed. The removal of some "features" also shouldn't count but I
> doubt it amounts to more than a few hundred lines.

1264 lines is "more like 1000 lines" than it is like 3000 lines!

> In fact, to get a more accurate figure you can just look at the result
> of my own conversion. There were two patches:
>  13 files changed, 510 insertions(+), 625 deletions(-)
>  11 files changed, 549 deletions(-)
> This is -664 lines total. The exact number doesn't really matter. The
> bottom line is that the new binding model requires significantly less
> glue code than the legacy model. If you think some more about it, it
> means a lot.

Then how come the bttv patch added lines of code and a couple kB to the
compiled driver size?

> > But it also deleted
> > v4l1 support, highmem support, and bigphys_area support.  Maybe other
> > things, Hans doesn't decribe his patches, so there's really no way to know
> > what the zoran patch really did other than to weed through 10,000+ lines of
> > diff which is mostly but not entirely moving blocks of code from one space
> > to another and reindenting them.
>
> You are unfair. The pull request came with a short log of all the
> changes.

"short" log.  His entire series was decribed with fewer words than I would
use on a single patch that changes ten lines.

> > If one includes the v4l1-compat module that is now providing v4l1 support
> > (though it doesn't work correctly for zoran), the driver and the compat
> > module are larger than the old driver was.  Of course one can now turn off
> > v4l1 support and get a smaller driver than before.  And the v4l1 compat
> > already existed and can be shared.  But I think it's more correct to say
> > the size reduction of the zoran driver was from removing features and not
> > from v4l2_subdev.  It seems like more of the the other subdev conversions
> > have overall added more code than they removed.
>
> I am not familiar enough with this part of the code to say. But I guess
> it doesn't really matter, as it wasn't my point anyway.

It seems like your point was that conversions to v4l2_subdev allow drivers
to be more efficient remove lots of code.  The numbers I see just don't
support that claim.

Re: bttv, tvaudio and ir-kbd-i2c probing conflict

[Jean Delvare]

Hi Trent,

On Mon, 16 Mar 2009 15:47:17 -0700 (PDT), Trent Piepho wrote:
> On Mon, 16 Mar 2009, Jean Delvare wrote:
> > You are unfair. The pull request came with a short log of all the
> > changes.
> 
> "short" log.  His entire series was decribed with fewer words than I would
> use on a single patch that changes ten lines.

In general I tend to like detailed patch logs as much as you do. But in
this case Hans is doing almost all the work by himself and it is very
needed, and the faster completed, the better. So I am really to trade
log details for a faster conversion.

> > (...)
> > I am not familiar enough with this part of the code to say. But I guess
> > it doesn't really matter, as it wasn't my point anyway.
> 
> It seems like your point was that conversions to v4l2_subdev allow drivers
> to be more efficient remove lots of code.  The numbers I see just don't
> support that claim.

No, sorry if I didn't make it clear, but that wasn't my point. My point
was only about the change in i2c binding model. This change clearly
results in a net shrink as far as lines of code are concerned.

This doesn't have much to do with the v4l2_subdev conversion Hans is
doing in parallel (other than the fact that the former may make the
later easier, but I'm not even sure.)

-- 
Jean Delvare

Re: bttv, tvaudio and ir-kbd-i2c probing conflict

[Trent Piepho]

On Tue, 17 Mar 2009, Jean Delvare wrote:
> On Mon, 16 Mar 2009 15:47:17 -0700 (PDT), Trent Piepho wrote:
> > On Mon, 16 Mar 2009, Jean Delvare wrote:
> > > You are unfair. The pull request came with a short log of all the
> > > changes.
> >
> > "short" log.  His entire series was decribed with fewer words than I would
> > use on a single patch that changes ten lines.
>
> In general I tend to like detailed patch logs as much as you do. But in
> this case Hans is doing almost all the work by himself and it is very
> needed, and the faster completed, the better. So I am really to trade
> log details for a faster conversion.

I guess that I don't consider documentation to be optional.

> > > (...)
> > > I am not familiar enough with this part of the code to say. But I guess
> > > it doesn't really matter, as it wasn't my point anyway.
> >
> > It seems like your point was that conversions to v4l2_subdev allow drivers
> > to be more efficient remove lots of code.  The numbers I see just don't
> > support that claim.
>
> No, sorry if I didn't make it clear, but that wasn't my point. My point
> was only about the change in i2c binding model. This change clearly
> results in a net shrink as far as lines of code are concerned.

Does it?  When we can use the model as it's designed, then I think it's
clearly much better.  But when one is emulating the detection behaviour,
like it appears the bttv patches do, I don't see what's better.

Re: bttv, tvaudio and ir-kbd-i2c probing conflict

[Andy Walls]

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.

>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.


>  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?

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?

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?

> 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, 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.

The PCI subsystem handles single devices with multiple functions.
There, of course, the function number is in the logical device address.

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.


> 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.

Regards,
Andy

> But this is all said without having much knowledge of the bttv, tvaudio
> and ir-kbd-i2c drivers, so I might as well be completely off track.

Re: bttv, tvaudio and ir-kbd-i2c probing conflict

[Hans Verkuil]

On Sunday 15 March 2009 20:34:33 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.
>
> >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.
>
> >  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?
>
> 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?
>
> 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?
>
> > 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, 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.
>
> The PCI subsystem handles single devices with multiple functions.
> There, of course, the function number is in the logical device address.
>
> 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.
>
> > 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.

Actually, it seems like this used to work at one time in the past. Jean, can 
you confirm that it used to be possible to have two i2c clients at the same 
i2c address in the past? Looking at the post (see the link in my original 
mail) it apparently worked in 2.6.19 at least.

I do think that the initial approach to this is to ensure that tvaudio is at 
least working. After all, it is better to have audio and no remote than to 
have a working remote, but no audio. Until we find a proper solution I 
think that this should not stop us from going forward since this way we 
remain bug-compatible with the current situation. Or even slightly better 
since we can now ensure that audio is working at least.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: bttv, tvaudio and ir-kbd-i2c probing conflict

[Jean Delvare]

On Sun, 15 Mar 2009 23:09:05 +0100, Hans Verkuil wrote:
> Actually, it seems like this used to work at one time in the past. Jean, can 
> you confirm that it used to be possible to have two i2c clients at the same 
> i2c address in the past? Looking at the post (see the link in my original 
> mail) it apparently worked in 2.6.19 at least.

No, it has never been possible. I can't explain why it used to work (if
it really did...) Maybe the reporter was using an alternative
implementation doing raw bus transfers? You originally suggested that
lirc was doing this.

> I do think that the initial approach to this is to ensure that tvaudio is at 
> least working. After all, it is better to have audio and no remote than to 
> have a working remote, but no audio. Until we find a proper solution I 
> think that this should not stop us from going forward since this way we 
> remain bug-compatible with the current situation. Or even slightly better 
> since we can now ensure that audio is working at least.

Agreed.

-- 
Jean Delvare

Re: bttv, tvaudio and ir-kbd-i2c probing conflict

[Jean Delvare]

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

Re: bttv, tvaudio and ir-kbd-i2c probing conflict

[Andy Walls]

On Sun, 2009-03-15 at 23:26 +0100, Jean Delvare wrote:
> Hi Andy,

Hi Jean,

Thanks for the reply.

> 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,

> > > 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.

So how does the linux identify a "device" when binding, by some n-tuple
in a coordinate space right?  The last few parts of that coordinate are
dictated by I2C subsystem design for I2C devices.  I believe they are
(adapter, address) commonly displayed something 0-004b.  

If the I2C subsystem allowed a coordinate for a device to have the last
few parts be (adapter, address, client/function #), like:

0-004b-0
0-004b-1

then one could have multiple drivers bind to a single piece of hardware
and still have the locking on the i2c_adapter provided by the
i2c_subsystem.  Wouldn't this be possible once the automatic probing is
gone?

 
> > 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.

I suppose validity is subjective, in that I have implicit cost measures
and assessments of the costs that are different from yours.

For example you have a need for some threshold number of devices to
behave like the I2C device in question before the need would be valid.
One such device is not enough.

Fair enough.



>  Once a design is in
> place, you have to follow it,


I disagree.  Why does the world now have digital TV broadcast systems
instead of just being stuck with analog TV broadcast systems?  A change
in the design of the broadcast television system was needed due to
pressures to recover spectrum, etc.


> 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.

No there is always a point to an intial design.  You always start with a
design to address your current needs and perhaps try to accomodate
future needs in design considerations.  No one can know everything ahead
of time.


>  Sure, a design can evolve, but not
> for just one case.

I agree, there are costs to be weighed.  If it doesn't make sense for a
single case, due to the costs involved, then it doesn't make sense.


> 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.

I was not thinking of trying to "move the world", just accomodate
multifunction I2C devices to allow multiple driver modules to access a
device at a single (adapter, address) coordinate.  Perhaps by using an
(adapter, address, function #) coordinate.


 
> > > 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.

Thanks, I didn't understand the scope of that effort you had
mentioned.  




> > 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.

And I guess I was thinking, but didn't write down, that the I2C
subsystem could expand it's namespace by one coordinate axis.

3-004b-0
3-004b-1


> (I am curious if this is a real device, BTW, as 0x96 isn't a valid
> 7-bit I2C address.)

No, I made it up from the device in the discussion. I forgot to shift it
down to a 7 bit address of 0x4b.


> > 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.

I was under the impression that access to a bus adapter was locked by
the I2C subsystem using adapter->bus_lock.  At that point, if multiple
drivers are accessing a multifunction device at a single address, they
would just need to "play nice" and stay in different register regions of
the multifunction I2C chip.

I hadn't thought about intertransfer delays.  The I2C spec does have a
minimum turn around specification between STOP and START, 4.7 us, but I
have no idea where it is enforced currently in Linux.



> > > 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.

OK.  So it sounds like it's not going to happen at all.


> > 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.

Ah. You're right.  My analogy corresponds to an I2C chip that responds
on two addresses on the same bus.  My analogy was bad.


> > 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.

That's the current state.  So it looks like the problem is there is no
"PIC16C54" driver since it's an 8-bit general purpose microcontroller.  

It's a one off case for writing a driver to handle a PIC16C54 with the
microcode the manufacturer has programmed into the device.  So I
understand the desire to leverage existing modules.


> > > 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.

And the problem here is that one physical slave ends up as two logical
slaves, that are very similar to individual physical slave handled by
other drivers, and people want to levereage existing code.

But you've convinced me that one corner case is not worth going through
all the expense to cover.



> Instead of blaming me or the i2c subsystem,

My intent was not to blame or offend, but to discuss the issue.  I'm
sorry if my tone didn't convey properly.  I'm not the most eloquent
writer.


>  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.

I understand now.  The desire to leverage existing crappy code is not
worth the cost and risk of perturbing things for the sake of one device.

Regards,
Andy


> Good night,

Re: bttv, tvaudio and ir-kbd-i2c probing conflict

[hermann pitton]

Hi,

Am Sonntag, den 15.03.2009, 20:52 -0400 schrieb Andy Walls:
> On Sun, 2009-03-15 at 23:26 +0100, Jean Delvare wrote:
> > Hi Andy,
> 
> Hi Jean,
> 
> Thanks for the reply.
> 
> > 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,
> 
> > > > 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.
> 
> So how does the linux identify a "device" when binding, by some n-tuple
> in a coordinate space right?  The last few parts of that coordinate are
> dictated by I2C subsystem design for I2C devices.  I believe they are
> (adapter, address) commonly displayed something 0-004b.  
> 
> If the I2C subsystem allowed a coordinate for a device to have the last
> few parts be (adapter, address, client/function #), like:
> 
> 0-004b-0
> 0-004b-1
> 
> then one could have multiple drivers bind to a single piece of hardware
> and still have the locking on the i2c_adapter provided by the
> i2c_subsystem.  Wouldn't this be possible once the automatic probing is
> gone?
> 
>  
> > > 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.
> 
> I suppose validity is subjective, in that I have implicit cost measures
> and assessments of the costs that are different from yours.
> 
> For example you have a need for some threshold number of devices to
> behave like the I2C device in question before the need would be valid.
> One such device is not enough.
> 
> Fair enough.
> 
> 
> 
> >  Once a design is in
> > place, you have to follow it,
> 
> 
> I disagree.  Why does the world now have digital TV broadcast systems
> instead of just being stuck with analog TV broadcast systems?  A change
> in the design of the broadcast television system was needed due to
> pressures to recover spectrum, etc.
> 
> 
> > 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.
> 
> No there is always a point to an intial design.  You always start with a
> design to address your current needs and perhaps try to accomodate
> future needs in design considerations.  No one can know everything ahead
> of time.
> 
> 
> >  Sure, a design can evolve, but not
> > for just one case.
> 
> I agree, there are costs to be weighed.  If it doesn't make sense for a
> single case, due to the costs involved, then it doesn't make sense.
> 
> 
> > 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.
> 
> I was not thinking of trying to "move the world", just accomodate
> multifunction I2C devices to allow multiple driver modules to access a
> device at a single (adapter, address) coordinate.  Perhaps by using an
> (adapter, address, function #) coordinate.
> 
> 
>  
> > > > 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.
> 
> Thanks, I didn't understand the scope of that effort you had
> mentioned.  
> 
> 
> 
> 
> > > 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.
> 
> And I guess I was thinking, but didn't write down, that the I2C
> subsystem could expand it's namespace by one coordinate axis.
> 
> 3-004b-0
> 3-004b-1
> 
> 
> > (I am curious if this is a real device, BTW, as 0x96 isn't a valid
> > 7-bit I2C address.)
> 
> No, I made it up from the device in the discussion. I forgot to shift it
> down to a 7 bit address of 0x4b.
> 
> 
> > > 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.
> 
> I was under the impression that access to a bus adapter was locked by
> the I2C subsystem using adapter->bus_lock.  At that point, if multiple
> drivers are accessing a multifunction device at a single address, they
> would just need to "play nice" and stay in different register regions of
> the multifunction I2C chip.
> 
> I hadn't thought about intertransfer delays.  The I2C spec does have a
> minimum turn around specification between STOP and START, 4.7 us, but I
> have no idea where it is enforced currently in Linux.
> 
> 
> 
> > > > 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.
> 
> OK.  So it sounds like it's not going to happen at all.
> 
> 
> > > 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.
> 
> Ah. You're right.  My analogy corresponds to an I2C chip that responds
> on two addresses on the same bus.  My analogy was bad.
> 
> 
> > > 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.
> 
> That's the current state.  So it looks like the problem is there is no
> "PIC16C54" driver since it's an 8-bit general purpose microcontroller.  
> 
> It's a one off case for writing a driver to handle a PIC16C54 with the
> microcode the manufacturer has programmed into the device.  So I
> understand the desire to leverage existing modules.
> 
> 
> > > > 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.
> 
> And the problem here is that one physical slave ends up as two logical
> slaves, that are very similar to individual physical slave handled by
> other drivers, and people want to levereage existing code.
> 
> But you've convinced me that one corner case is not worth going through
> all the expense to cover.
> 
> 
> 
> > Instead of blaming me or the i2c subsystem,
> 
> My intent was not to blame or offend, but to discuss the issue.  I'm
> sorry if my tone didn't convey properly.  I'm not the most eloquent
> writer.
> 
> 
> >  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.
> 
> I understand now.  The desire to leverage existing crappy code is not
> worth the cost and risk of perturbing things for the sake of one device.
> 
> Regards,
> Andy
> 
> 
> > Good night,
> 

sorry, just reading the mail backlash on a first attempt.

But what comes into mind from what we really have seen, there are i2c
devices (tuners) with multiple addresses present on the bus, in fact
only one of them "works".

This was improved by new separate silicon radio tuner chips within the
even same range of possible four addresses. That for the radio tuner
address field was introduced.

The same goes for 0x86 and 0x4b demodulator stuff and the like.

Of course it is known that there is an address clash between tda988x and
tda 8290 and as well with that pic in tvaudio.

So, in general I'm with Hans and all here, it is better to have this
device specific in the card's entry.

But Trent is also right in so far, that we don't have it for sure for
all the devices around previously probed, no final list so far.

What gives me more concerns is, that such addresses shared by multiple
devices are on the other driver encoded in the eeprom, even manufacturer
specific (!), and we are more confronted with the mess not to be able to
identify the _type_ of the device then with identifying its address.

This is most visible for digital demodulators currently.

Good night,
Hermann

Re: bttv, tvaudio and ir-kbd-i2c probing conflict

[Jean Delvare]

Hi Andy,

On Sun, 15 Mar 2009 20:52:33 -0400, Andy Walls wrote:
> On Sun, 2009-03-15 at 23:26 +0100, Jean Delvare wrote:
> > 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.
> 
> So how does the linux identify a "device" when binding, by some n-tuple
> in a coordinate space right?  The last few parts of that coordinate are
> dictated by I2C subsystem design for I2C devices.  I believe they are
> (adapter, address) commonly displayed something 0-004b.  

Yes, this is correct.

> If the I2C subsystem allowed a coordinate for a device to have the last
> few parts be (adapter, address, client/function #), like:
> 
> 0-004b-0
> 0-004b-1
> 
> then one could have multiple drivers bind to a single piece of hardware
> and still have the locking on the i2c_adapter provided by the
> i2c_subsystem.  Wouldn't this be possible once the automatic probing is
> gone?

No. Locking on the i2c_adapter level is only the tip of the iceberg. As
discussed with Trent somewhere else in this thread, there are many
other cases which require additional locking at the device (not bus)
level. Executive summary: non-atomic transactions, banked registers,
read-modify-write cycles.

Of course there may be cases where neither problem exists for a given
chip (all drivers use only atomic transactions, the register space is
flat and the registers accessed by both drivers don't overlap.) But this
is the lucky case. We can hardly build a new design on top of these
assumptions.

What this means is that, in general, you will require some form of
coordination between the drivers for the separate functions. At which
point it is really not different from having a single driver with a
core part taking care of the coordination and other parts taking care
of each functions.

> > (...)
> > 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.
> 
> And I guess I was thinking, but didn't write down, that the I2C
> subsystem could expand it's namespace by one coordinate axis.
> 
> 3-004b-0
> 3-004b-1

It could, yes. This raises questions though. For example, while the bus
number and especially device address are dictated by hardware
realities, the last coordinate you propose to had would not. These
numbers would be attributed arbitrarily by Linux driver authors. How?

Oh, and of course this change would break compatibility. There are a
number of user-space tools which expect the i2c device "names" to look
like 3-004b and not 3-004b-0. We'd have to fix them all. So this kind
of change is not to be taken lightly.

But more importantly, my feeling is that the additional coordinate in
the device names doesn't solve any problem. It's moving to the bus
level something which was handled at the chip level so far, and I fail
to see any significant benefit in doing so.

> > (I am curious if this is a real device, BTW, as 0x96 isn't a valid
> > 7-bit I2C address.)
> 
> No, I made it up from the device in the discussion. I forgot to shift it
> down to a 7 bit address of 0x4b.

Ah, OK. Pffew :)

> I hadn't thought about intertransfer delays.  The I2C spec does have a
> minimum turn around specification between STOP and START, 4.7 us, but I
> have no idea where it is enforced currently in Linux.

I fear it is not enforced. I suspect this is taken care of by hardware
implementations (e.g. SMBus controller on the mainboard) but probably
i2c-algo-bit and possibly other software implementations should be
taught about it. If this really bothers someone, I accept patches ;)

But this is not what I had in mind. What I had in mind was per-chip
restrictions. For example, some chips require a delay between
consecutive writes, and will misbehave if the delay isn't respected.
This needs to be handled at the driver level. More specifically, for a
multifunction device, this likely has to be handled by the common part
of the driver.

> > (...)
> > 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.
> 
> OK.  So it sounds like it's not going to happen at all.

Well, I still hope that someday the core driver model will have proper
support for multiple bindings. But given how slow the progress has been
over the past few years, yeah, I'm not holding my breath :(

> Ah. You're right.  My analogy corresponds to an I2C chip that responds
> on two addresses on the same bus.  My analogy was bad.

A single I2C chip which responds to 2 I2C addresses can be handled by
one or two drivers, as you like. This is an easy case. This however
isn't the case of the PIC16C54.

> > (...)
> > 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.
> 
> That's the current state.  So it looks like the problem is there is no
> "PIC16C54" driver since it's an 8-bit general purpose microcontroller.

General purpose devices are indeed a special case. In general I don't
see much point in having a general driver for them. I think it's better
to consider each usage a separate chip and write per-usage driver. All
we have to come up with is an i2c device name for them (e.g.
pic16c54_pv951) so that the right driver will bind.

> > (...)
> > Instead of blaming me or the i2c subsystem,
> 
> My intent was not to blame or offend, but to discuss the issue.  I'm
> sorry if my tone didn't convey properly.  I'm not the most eloquent
> writer.

I didn't take it personally, don't worry. I'm probably not the best
writer out there either, and it was late, and my baby is ill so I can't
really concentrate on what I'm writing, which doesn't help.

> I understand now.  The desire to leverage existing crappy code is not
> worth the cost and risk of perturbing things for the sake of one device.

+1 :)

-- 
Jean Delvare

Re: bttv, tvaudio and ir-kbd-i2c probing conflict

[Trent Piepho]

On Sun, 15 Mar 2009, Andy Walls wrote:
> On Sun, 2009-03-15 at 18:12 +0100, Jean Delvare wrote:
>
> > 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.

For a lot of i2c devices, it would be difficult for two drivers to access
the device at the same time without some kind of locking.

If you take the reads and writes of one driver and then intersperse the
reads and writes of another driver, the resulting sequence from the i2c
device's point of view is completely broken.

But, I suppose there are some devices where if the drivers all use
i2c_smbus_read/write_byte/word_data or equivalent atomic transactions
with i2c_transfer(), then you could get away with two drivers talking to
the same chip.

Re: bttv, tvaudio and ir-kbd-i2c probing conflict

[Jean Delvare]

Hi Trent,

On Sun, 15 Mar 2009 16:46:36 -0700 (PDT), Trent Piepho wrote:
> On Sun, 15 Mar 2009, Andy Walls wrote:
> > 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.
> 
> For a lot of i2c devices, it would be difficult for two drivers to access
> the device at the same time without some kind of locking.
> 
> If you take the reads and writes of one driver and then intersperse the
> reads and writes of another driver, the resulting sequence from the i2c
> device's point of view is completely broken.

Correct.

> But, I suppose there are some devices where if the drivers all use
> i2c_smbus_read/write_byte/word_data or equivalent atomic transactions
> with i2c_transfer(), then you could get away with two drivers talking to
> the same chip.

Yes, this may be possible for some devices, but it can't be
generalized. Non-atomic transactions are one problem but there are
others, for example some I2C devices do not have a flat address space
(the same I2C transaction can access different registers depending on
the value of another register commonly known as bank selector.) Another
problem is read-modify-write cycles, which in my experience are often
needed for I2C devices, and which are racy if several drivers attempt
to handle the same device and want to touch the same register.

So, all in all, great care should always be taken when more than one
driver accesses the same I2C device. And even if only one driver
accesses the I2C device, all accesses that can happen in parallel (for
example triggered by sysfs attribute reads or writes) may have to be
serialized by the driver.

-- 
Jean Delvare

Re: bttv, tvaudio and ir-kbd-i2c probing conflict

[Andy Walls]

On Sun, 2009-03-15 at 13:44 +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.


Hans,

Just a thought: have you confirmed with i2cdetect that the PIC16C54
microcontroller code only responds at one I2C address?

Regards,
Andy

Re: bttv, tvaudio and ir-kbd-i2c probing conflict

[Hans Verkuil]

> On Sun, 2009-03-15 at 13:44 +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.
>
>
> Hans,
>
> Just a thought: have you confirmed with i2cdetect that the PIC16C54
> microcontroller code only responds at one I2C address?

I don't have this board, so I can't test it. Everything points to this
being a multifunction device responding to one i2c address. It's one of
those programmable devices, so that seems perfectly reasonably to me.

In general I'm not inclined to worry about this case. As Jean said, the
core problem is not so much the perceived lack of i2c support for such
devices, but that the IR and (to a lesser extent) tvaudio modules are
badly designed. The IR module should really become a v4l2_subdev as well,
loaded on command by the adapter driver, and v4l2_subdev should acquire
one or more ops to handle IR stuff. With that in place it is easy to move
the pic support from tvaudio and the IR module into a module of its own.

Simple and effective it solves this whole issue elegantly.

Regards,

       Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG