Re: staging driver s626 clashes with philips SAA7146 media/dvb based cards

[Manu Abraham]

Hi Herton,

On Thu, Jun 18, 2009 at 9:23 PM, Herton Ronaldo
Krzesinski<herton@xxxxxxxxxxxxxxx> wrote:
> Em Quinta-feira 18 Junho 2009, às 04:28:04, Frank Mori Hess escreveu:
>> On Wednesday 17 June 2009, Herton Ronaldo Krzesinski wrote:
>> > >
>> > > Your patch breaks configuration of the board unless the bus and slot
>> > > are explicitly specified.  Just make a minimal patch that replaces
>> > > pci_get_device with pci_get_subsys and fixes the problem that was
>> > > reported.
>> >
>> > Hmm that's not what the patch does, it doesn't break configuration,
>> > keeps the same logic as before (I was wrong in my last email replying to
>> > myself), check it, if it->options[0] and it->options[1] isn't specified,
>> > the pdev is valid so the for loop exits (see !pdev check).
>>
>> Your right.  However, it also turns a loop over pci devices into a loop
>> over pci ids, which appears to break the case of multiple s626 boards
>> where the bus/slot of the second s626 board is specified.  If you're not
>> willing to provide a minimal patch that just fixes the reported problem,
>> just say so.  It would have been less effort for me to do it myself than
>> analyze what your changes are breaking.
>
> That's part of reviewing process, I just wanted to enhance it in case more pci
> ids are added in the future along with the switch to pci_get_subsys, I don't
> know why you act like that, you don't want the code to be enhanced? Other
> comedi driver loop over ids, for example gsc_hpdi
>
> And indeed what you say it's true, there is a bug in the patch now that you
> checked it out properly, it has a problem with this multiple s626 case, fixed
> that now. Pointing out the problem instead just saying it's broken helps :)
>
> I also removed the obligation to add sub ids and re-factored the patch, that
> was too much and not good, just a simpler loop over pci id array is used now,
> and a comment was added in pci id list telling people that they should add
> subvendor:subdevice ids for boards with vendor:device == 0x1131:0x7146,
> in case new boards with this id appear.


Since there is valid subsystem information: why do you have to loop
over the ID's
manually in the driver ?

Can't the ID's be set into the PCI ID table and a normal pci_probe be
used instead ?
Not that i am familiar with the comedi stuff though ...

We use the probe method directly (rather than pci_get_ ) for most of
the multimedia drivers. As an example this is what i do:
http://jusst.de/hg/saa716x/file/59dd985d4473/linux/drivers/media/dvb/saa716x/saa716x_budget.c

Maybe it helps you in some way.

Regards,
Manu
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/