On Sun, 20 Jan 2008 15:26:20 -0800 (PST)
Trent Piepho <xyzzy@xxxxxxxxxxxxx> wrote:
> On Sun, 20 Jan 2008, Mauro Carvalho Chehab wrote:
> > On Sat, 19 Jan 2008 05:28:49 -0800 (PST)
> > Trent Piepho <xyzzy@xxxxxxxxxxxxx> wrote:
> >
> > > I wish people would use the patch creating system from Hg, since it would
> > > the format mistakes evident in this patch.
> > >
> > > 1. no patch title
> > > 2. s-o-b's in wrong order
> > > 3. diffstat included
> > > 4. patch against git source and not hg
> > >
> > > On Sat, 19 Jan 2008, Akinobu Mita wrote:
> > > > From: Akinobu Mita <akinobu.mita@xxxxxxxxx>
> > > >
> > > > This patch moves the subsystem ID and subsystem vendor ID check from probing
> > > > function to the PCI generic function by describing subsystem IDs in
> > > > pci_device_id table. This enables to add new PCI IDs to a device driver pci_ids
> > > > table at runtime by new_id file in sysfs pci driver tree.
> > >
> > > This patch also changes the driver from having a pci alias to auto-load for
> > > all bt878 cards to instead only auto-load for specific cards. Some
> > > distributions will probably want to adjust their modprobe rules. I know
> > > some have delt with both snd-bt87x and bt878 loading for the same devices
> > > in various ways, e.g. adding one or the other to their modprobe blacklist
> > > files.
> >
> > The proper solution here seems to be moving snd-bt87x to our tree, and create
> > some shared code to be used by snd-bt87x, dvb-bt8xx, to avoid the
> > driver conflicts.
>
> That doesn't solve the problem. It's inherent in the bt878 design, as it
> uses the same pci function for either sound or dvb. So a bt878 chip
> function 1 can load one and only one driver from ALSA, OSS, or DVB. For
> most chips, there is only one driver that applies to a given pci device and
> function.
>
> In some cases it is possible to tell which driver to use via
> sub-vendor/device data, but not always. The ALSA driver only auto-loads
> for known good subdata, and has a module option to force loading when that
> won't work. The DVB driver has the wildcard sub-vendor/device in the alias
> table, IMHO a mistake, and will try to load for for all bt878 chips.
>
> The DVB driver used to load for cards with sub-vendor/device 0x0000,0x0000,
> but a previous patch changed that. An effect that that _should_ have been
> mentioned in the description of the patch responsible, but was not.
>
> This patch changes the DVB driver to not auto-load for all bt878 chips. A
> rather significant change that should be described in the patch
> description, but is not.
>
> > > > +static const char * __devinit card_name(const struct pci_device_id *id)
> > > > +{
> > > > + if (id >= bt878_pci_tbl &&
> > > > + id < bt878_pci_tbl + ARRAY_SIZE(bt878_pci_tbl) - 1)
> > > > + return (const char *)id->driver_data;
> > >
> > > Are you sure this is safe? I don't remember reading that the pci_device_id
> > > passed to the pci driver probe method must be from the device id table.
> > > Couldn't the device id be a local variable from whatever calls probe, as
> > > long as driver_data is set to the correct value? I looked at the current
> > > pci bus code, and it does always pass a pointer into the driver's pci id
> > > table. But that could change.
> > >
> > > It seems like a better method would just be to check if driver_data is 0,
> > > which will be the case if the id was added dynamically.
> >
> > This data can't be changed by pci core, since driver_data is a priv element. It
> > is used as a pointer on some drivers, while others use it as an integer value to
> > an array (like most V4L/DVB and ALSA drivers). The above code should work
> > properly. If pci touches on this value, it would break the support for the rest
> > of v4l/dvb drivers, so I think we don't need to worry about this.
>
> I don't think you understand. That code checks if the pci_device_id
> pointer passed to the probe function points to a spot in the driver's pci
> id table. It has nothing to do with the driver_data field being modified.
I think you didn't understand me ;)
IMO, the check is a little overkill, but I don't think that any future changes
on PCI internal code would affect, since the usage of driver_data is private to
PCI clients. The core shouldn't touch it. The check for driver_data == NULL
seems enough, but both ways should work fine.
About bttv alsa and dvb drivers, what I tried to say is that we can do some
improvements, creating a clean way for they to use the same info.
As bttv already knows that a board has DVB (.has_dvb field at board
description table). It shouldn't be hard to add a similar field for digital
audio support (.has_alsa or .has_digital_audio). We may create some functions
at bttv driver to help -dvb and -alsa to know if a device has or not support
for alsa and/or dvb. It shouldn't be hard to have a resource locking facility
to allow loading both alsa and dvb for those devices that are capable of
providing both functions.
Anyway, IMO, we should go to Akinobu approach for now, later thinking on a
better approach.
Cheers,
Mauro
_______________________________________________
linux-dvb mailing list
linux-dvb@xxxxxxxxxxx
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
