On Tue, Aug 28, 2007 at 12:01:30PM -0400, Jiri Slaby wrote:
> +config ATH5K_AR5210
> + bool "Support AR5210"
> + depends on ATH5K
> + default y
> +
> +config ATH5K_AR5211
> + bool "Support AR5211"
> + depends on ATH5K
> + default y
> +
> +config ATH5K_AR5212
> + bool "Support AR5212"
> + depends on ATH5K
> + default y
Please don't add more default statements.
Also this whole patch seems rather pointless. It saves only
very little and turns the driver into a complete ifdef maze.
2007/8/30, John W. Linville <[email protected]>:
> On Thu, Aug 30, 2007 at 04:38:09AM +0300, Nick Kossifidis wrote:
> > 2007/8/28, Christoph Hellwig <[email protected]>:
>
> > > Also this whole patch seems rather pointless. It saves only
> > > very little and turns the driver into a complete ifdef maze.
>
> > Also most
> > people will use 5212 code only, 5211 cards are on some old laptops and
> > 5210, well i couldn't even find a 5210 for actual testing :P
>
> FWIW, I'd bet dollars to donuts that distros will enable them all
> together.
>
> Is saving code space the only reason to turn these off? How much
> space do you save?
>
> Is there some way you can isolate and/or limit the number of ifdef
> blocks further? If so, we might consider a version of this patch
> that depends on EMBEDDED or somesuch...?
>
> John
O.K. as a first step i'll limit 5210 code only then, just an option
like "support older 5210 chipsets" which is going to be off by default
instead of 3 options. It's not just saving space, it's also saving
some runtime checks. It's not really a gain in performance though,
most checks are done during initialization and dfs setup, i just
thought it would be usefull to save as much cpu as possible.
--
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick
On Thu, 2007-08-30 at 08:36 -0400, John W. Linville wrote:
> On Thu, Aug 30, 2007 at 04:38:09AM +0300, Nick Kossifidis wrote:
> > 2007/8/28, Christoph Hellwig <[email protected]>:
>
> > > Also this whole patch seems rather pointless. It saves only
> > > very little and turns the driver into a complete ifdef maze.
>
> > Also most
> > people will use 5212 code only, 5211 cards are on some old laptops and
> > 5210, well i couldn't even find a 5210 for actual testing :P
>
> FWIW, I'd bet dollars to donuts that distros will enable them all
> together.
I would certainly _hope_ that distros enable everything -that is in the
kernel- that they can get their hands on, otherwise when you stick a
card in, it doesn't just work.
Dan
> Is saving code space the only reason to turn these off? How much
> space do you save?
>
> Is there some way you can isolate and/or limit the number of ifdef
> blocks further? If so, we might consider a version of this patch
> that depends on EMBEDDED or somesuch...?
>
> John
On Thu, Aug 30, 2007 at 04:38:09AM +0300, Nick Kossifidis wrote:
> It saves big chunks of code (not only initial register settings
> arrays) and we'll extend it's use more inside ath5k_hw.c Trust me this
> is a very useful step, eg. check out descriptor processing / setup or
> PHY functions (calibrate/channel set etc). For example AR5210 mac chip
> only comes with RF5110 phy chip so we can get rid of RF5111/RF5112
> code, AR5211 comes with RF5111 so we can get rid of RF5110 and RF5112
> code and AR5212 comes with RF5111 or RF5112 so we get rid of RF5110.
> This thing also saves lots of checks during runtime (some of them
> happen verry frequently, eg. durring descriptor processing). Also most
> people will use 5212 code only, 5211 cards are on some old laptops and
> 5210, well i couldn't even find a 5210 for actual testing :P
If you're doing these checks in a hotpath something is badly wrong with
your architecture.
2007/8/28, Christoph Hellwig <[email protected]>:
> On Tue, Aug 28, 2007 at 12:01:30PM -0400, Jiri Slaby wrote:
> > +config ATH5K_AR5210
> > + bool "Support AR5210"
> > + depends on ATH5K
> > + default y
> > +
> > +config ATH5K_AR5211
> > + bool "Support AR5211"
> > + depends on ATH5K
> > + default y
> > +
> > +config ATH5K_AR5212
> > + bool "Support AR5212"
> > + depends on ATH5K
> > + default y
>
> Please don't add more default statements.
>
> Also this whole patch seems rather pointless. It saves only
> very little and turns the driver into a complete ifdef maze.
It saves big chunks of code (not only initial register settings
arrays) and we'll extend it's use more inside ath5k_hw.c Trust me this
is a very useful step, eg. check out descriptor processing / setup or
PHY functions (calibrate/channel set etc). For example AR5210 mac chip
only comes with RF5110 phy chip so we can get rid of RF5111/RF5112
code, AR5211 comes with RF5111 so we can get rid of RF5110 and RF5112
code and AR5212 comes with RF5111 or RF5112 so we get rid of RF5110.
This thing also saves lots of checks during runtime (some of them
happen verry frequently, eg. durring descriptor processing). Also most
people will use 5212 code only, 5211 cards are on some old laptops and
5210, well i couldn't even find a 5210 for actual testing :P
--
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick
On Thu, Aug 30, 2007 at 04:38:09AM +0300, Nick Kossifidis wrote:
> 2007/8/28, Christoph Hellwig <[email protected]>:
> > Also this whole patch seems rather pointless. It saves only
> > very little and turns the driver into a complete ifdef maze.
> Also most
> people will use 5212 code only, 5211 cards are on some old laptops and
> 5210, well i couldn't even find a 5210 for actual testing :P
FWIW, I'd bet dollars to donuts that distros will enable them all
together.
Is saving code space the only reason to turn these off? How much
space do you save?
Is there some way you can isolate and/or limit the number of ifdef
blocks further? If so, we might consider a version of this patch
that depends on EMBEDDED or somesuch...?
John
--
John W. Linville
[email protected]
Dan Williams wrote:
> On Thu, 2007-08-30 at 08:36 -0400, John W. Linville wrote:
>> On Thu, Aug 30, 2007 at 04:38:09AM +0300, Nick Kossifidis wrote:
>>> 2007/8/28, Christoph Hellwig <[email protected]>:
>>>> Also this whole patch seems rather pointless. It saves only
>>>> very little and turns the driver into a complete ifdef maze.
>>> Also most
>>> people will use 5212 code only, 5211 cards are on some old laptops and
>>> 5210, well i couldn't even find a 5210 for actual testing :P
>> FWIW, I'd bet dollars to donuts that distros will enable them all
>> together.
>
> I would certainly _hope_ that distros enable everything -that is in the
> kernel- that they can get their hands on, otherwise when you stick a
> card in, it doesn't just work.
Distros definitely -do not- do this. Plenty of ancient ISA drivers are
disabled at build time, for example, in many distros.
Jeff
On Fri, 2007-08-31 at 09:30 -0400, Jeff Garzik wrote:
> Dan Williams wrote:
> > On Thu, 2007-08-30 at 08:36 -0400, John W. Linville wrote:
> >> On Thu, Aug 30, 2007 at 04:38:09AM +0300, Nick Kossifidis wrote:
> >>> 2007/8/28, Christoph Hellwig <[email protected]>:
> >>>> Also this whole patch seems rather pointless. It saves only
> >>>> very little and turns the driver into a complete ifdef maze.
> >>> Also most
> >>> people will use 5212 code only, 5211 cards are on some old laptops and
> >>> 5210, well i couldn't even find a 5210 for actual testing :P
> >> FWIW, I'd bet dollars to donuts that distros will enable them all
> >> together.
> >
> > I would certainly _hope_ that distros enable everything -that is in the
> > kernel- that they can get their hands on, otherwise when you stick a
> > card in, it doesn't just work.
>
> Distros definitely -do not- do this. Plenty of ancient ISA drivers are
> disabled at build time, for example, in many distros.
Ok, so let me qualify to "within reason". All 802.11-compliant wireless
cards would fall within the "within reason" IMHO, but, for example,
older non 802.11 wireless cards (early Aironet products for example)
probably don't. ISA clearly does not for mainstream distros.
Dan
2007/8/31, Nick Kossifidis <[email protected]>:
> 2007/8/30, John W. Linville <[email protected]>:
> > On Thu, Aug 30, 2007 at 04:38:09AM +0300, Nick Kossifidis wrote:
> > > 2007/8/28, Christoph Hellwig <[email protected]>:
> >
> > > > Also this whole patch seems rather pointless. It saves only
> > > > very little and turns the driver into a complete ifdef maze.
> >
> > > Also most
> > > people will use 5212 code only, 5211 cards are on some old laptops and
> > > 5210, well i couldn't even find a 5210 for actual testing :P
> >
> > FWIW, I'd bet dollars to donuts that distros will enable them all
> > together.
> >
> > Is saving code space the only reason to turn these off? How much
> > space do you save?
> >
> > Is there some way you can isolate and/or limit the number of ifdef
> > blocks further? If so, we might consider a version of this patch
> > that depends on EMBEDDED or somesuch...?
> >
> > John
>
> O.K. as a first step i'll limit 5210 code only then, just an option
> like "support older 5210 chipsets" which is going to be off by default
> instead of 3 options. It's not just saving space, it's also saving
> some runtime checks. It's not really a gain in performance though,
> most checks are done during initialization and dfs setup, i just
> thought it would be usefull to save as much cpu as possible.
>
Well after some thought i removed them all, there is no real gain from
this in most cases (that ppl will use newer 5212 chips and
combatibles).
--
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick