[PULL] http://linuxtv.org/hg/~tap/v4l-dvb | |
| [Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] | |
Trent Piepho wrote:
> On Thu, 1 Nov 2007, Oliver Endriss wrote:
> > Oliver Endriss wrote:
> > > Trent Piepho wrote:
> > > > Mauro,
> > > >
> > > > Please pull from http://linuxtv.org/hg/~tap/v4l-dvb
> > > > it's been updated since the last request
> > > > ...
> > > >
> > > > 03/09: ttpci: Rework Kconfig menus and Makefile
> > > > http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=5320c2571183
> > > > ...
> > >
> > > Guys,
> > >
> > > I would appreciate if you would give me a chance to review non-trivial
> > > patches, which touch the ttpci family, _before_ they are committed.
> > > Thanks!
> >
> > Meanwhile I've reviewed this changeset:
> > It does not make sense to separate av7110 and budget cards.
>
> They've always been separated before, and you've never done anything about
> it.
Wrong. Before your patch they were grouped together:
| | --- DVB/ATSC adapters ? ?
? ? --- Supported SAA7146 based PCI Adapters ? ?
? ? <M> AV7110 cards ? ?
? ? [ ] Compile AV7110 firmware into the driver ? ?
? ? [*] AV7110 OSD support ? ?
? ? < > Budget cards ? ?
? ? < > Budget cards with onboard CI connector ? ?
? ? < > Budget cards with analog video inputs ? ?
? ? < > AV7110 cards with Budget Patch ? ?
? ? --- Supported USB Adapters ? ?
You put the budget-stuff under "SAA7146 DVB cards (aka Budget, Nova-PCI)"
? ? <M> AV7110 cards ? ?
? ? [ ] Compile AV7110 firmware into the driver ? ?
? ? [*] AV7110 OSD support ? ?
? ? < > SAA7146 DVB cards (aka Budget, Nova-PCI) ? ?
? ? --- Supported USB Adapters ? ?
The av7110 is also saa7146-based, so this does not make much sense.
> The issue with non-working kconfig settings for av7110 and budget kept
> coming up over and over, and I kept saying how to fix it over and over,
> but nothing happened. I got tired of repeating myself and went and fixed
> it.
You could have sent me a mail. Bypassing a maintainer is very bad style.
I don't accept this! If you don't respect the rules you can take over
maintainership of ttpci and do with the drivers whatever you like.
Have a lot of fun. :-(
> The AV7710 driver and the budget drivers are different modules. They way
> you've done it, it's not possible to compile one as a module and one into
> the kernel, which is something that is possible to do, as was possible
> before your patch.
Wrong, it is possible as it was before. For example:
? ? <*> Supported SAA7146-based PCI Adapters ? ?
? ? <M> AV7110 cards ? ?
? ? [ ] Compile AV7110 firmware into the driver (NEW) ? ?
? ? [*] AV7110 OSD support (NEW) ? ?
? ? <*> Budget cards ? ?
? ? <M> Budget cards with onboard CI connector ? ?
? ? <*> Budget cards with analog video inputs ? ?
? ? <M> AV7110 cards with Budget Patch (DEPRECATED) ?
And, by the way, budget/budget-ci/budget-av/budget-patch and dvb-tpci
are 5 different modules. So what?
> You've removed the dependencies for DVB_BUDGET_CORE. It does depend on
> those things. Just because kconfig currently ignores dependencies for
> select is no reason to remove dependency information.
Wrong. DVB_BUDGET_CORE is within the 'if' block and inherits the
dependencies from DVB_SAA7146:
<snip>
config DVB_SAA7146
tristate "Supported SAA7146-based PCI Adapters"
depends on DVB_CORE && PCI && I2C
help
...
if DVB_SAA7146
...
endif
</snip>
Basically I do exactly the same as you did, except that DVB_AV7110 is
also controlled by DVB_SAA7146.
> You're using select _more_ that it was before. From kconfig-language.txt,
> "select is evil.... use select ... for symbols with no dependencies."
I use select for DVB_BUDGET_CORE and TTPCI_EEPROM, you used it for
TTPCI_EEPROM only. No problem, that's what select was designed for.
> Putting the drivers under an "if/endif" block isn't necessary. You don't
> see other drivers doing this. Just making the drivers depend on the symbol
> you want them under is enough, if you've organized your file correctly.
If you prefer I can remove it, but I'd rather remove the DVB_SAA7146
dependency from all symbols within the if/endif block. See attachment.
> If there is a if/endif block, it isn't necessary for symbols inside it to
> depend on DVB_SAA7146, since they gain an implicit dependency by virtue of
> being in the block that depends on DVB_SAA7146. Some of the symbols in
> ttpci get explicit dependencies on DVB_SAA7146 and some only have the
> implicit ones, and it doesn't not appear there is any good reason for this
> difference.
True, fixed.
> The new symbol DVB_SAA7146 is a tristate that doesn't control any code.
> This doesn't make sense, how can something that is only for controlling
> presentation of the options be a module or compiled in?
It propagates its setting to the symbols below. It can also be used to
force all drivers below to be compiled as modules.
If you prefer I make it a menu. Tristate menus are used very often in
the kernel. I think it's overkill but I don't care. See attachment.
> It's just a menu,
> you can't compile a menu. If you want to group the SAA7146 cards together
> under a menu, use a menu. But IMO, creating a new menu just for two cards
> is unnecessary.
We have _5_ drivers controlled by this selection:
dvb-ttpci, budget, budget-ci, budget-av, budget-patch.
> The goal isn't to create as many menus as possible, but
> only when it helps.
If you prefer we can use a menu. I don't care.
Again: I did exactly the same as you did, except that all cards are
grouped in a slightly different way.
> > budget-patch is a driver for a (modded) full-featured card, not a budget
> > card. I suggest to keep all saa7146 cards together. See attachment.
>
> budget-patch uses the budget-core driver, and NOT the AV7110 driver, so
> it's more a budget driver than a AV7110 driver.
It uses av7110 hardware, not budget hardware. It's a special driver
for modded full-featured cards.
> It does say that it needs
> the AV7110 driver to be loaded first in order to load the firmware, which I
> guess is why the budget-patch driver used to select AV7110. select is
> evil, so I changed it to a dependency as it should have been done. But it
> sounds like one still needs the AV7110 driver around to use the
> budget-patch driver.
The budgetpatch driver is crap by design. A typical use requires to
- load the av7110 (just to load the firmware)
- unload the av7110 driver
- load budget-patch
I marked it DEPRECATED, so we may remove it in the future.
Agreed, It should not select av7110. That's fine.
But I don't want to make it dependent of DVB_AV7110 either.
We should not make it hard for people to find a driver.
The module compiles standalone without any problems.
As I want to get rid of it, I don't want to spend much effort. I just
want to make sure that all users (if any) have time to migrate to the
av7110 driver.
CU
Oliver
--
----------------------------------------------------------------
VDR Remote Plugin 0.4.0: http://www.escape-edv.de/endriss/vdr/
----------------------------------------------------------------
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ttpci-kconfig2.diff
Type: text/x-diff
Size: 5153 bytes
Desc: not available
Url : http://www.linuxtv.org/pipermail/v4l-dvb-maintainer/attachments/20071101/5b288f45/attachment.diff
|
|
-->
