The following are based on Nick's patches, they address checkpatch
complaints, make use of pci_dev's is_pcie, and makes use of a helper
instead on the 7th patch. These need testing on pci-e cards. I've
tested it on AR5213 (MAC 0x56, PHY: 0x41), RF5211 (0x17) and RF2111
(0x23).
You can also wget them from:
http://www.kernel.org/pub/linux/kernel/people/mcgrof/patches/ath5k/2008-02-28/
Luis
2008/3/7, Luis R. Rodriguez <[email protected]>:
>
> OK -- so just to re-cap, patches 1-6 remain the same, patch 7 was
> changed to not have the helper but still respect 80-char limit,
> and patch 8 modified to apply ontop of new patch 7.
>
> Now series can be wget'd from:
>
> http://www.kernel.org/pub/linux/kernel/people/mcgrof/patches/ath5k/2008-03-07/
>
>
> Luis
>
Acked-by: Nick Kossifidis <[email protected]>
--
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick
2008/3/7, Luis R. Rodriguez <[email protected]>:
> On Thu, Feb 28, 2008 at 5:56 PM, Luis R. Rodriguez <[email protected]> wrote:
> > The following are based on Nick's patches, they address checkpatch
> > complaints, make use of pci_dev's is_pcie, and makes use of a helper
> > instead on the 7th patch. These need testing on pci-e cards. I've
> > tested it on AR5213 (MAC 0x56, PHY: 0x41), RF5211 (0x17) and RF2111
> > (0x23).
>
>
> Testing was done.
>
>
> > You can also wget them from:
> >
> > http://www.kernel.org/pub/linux/kernel/people/mcgrof/patches/ath5k/2008-02-28/
>
>
> We need to move forward with this. Any blockers?
>
>
> Luis
>
As i've told you in private, i'm not O.K. with the helper function you
introduced in 7/8 because:
a) It makes cross-reference with dumps more difficult for no reason.
b) It doesn't make sense to have a helper function for only one chip
and for only one single 5212-specific part of code. There is a lot of
chip-specific code in hw_reset, either we make a helper function for
each chip containing all chip-specific code or we leave it as it is -i
prefer the second since order there matters + it will make
cross-reference much more difficult-, grabbing just a small part of
chip-specific code and calling it a helper function for that chip
doesn't make sense and is misleading IMHO.
c) It doesn't make sense to introduce a small function that's only
called once + have the above issues just to avoid 80-column limit.
d) In general i don't think introducing a function for style-reasons
is a good practice since it's misleading and in fact makes code
readability worse.
so please resubmit that patch without the helper function and you'll
get my ACK (well it'll be my patch then + checkpatch fixes so
ACK/signed-off-by makes no difference :P).
As for the pci-e patch i've also told you what i think both public and
in private, however since a fix for these cards (2424/5424/2425) is
needed asap i'm ok with any given approach (mine + Bob's patch or
your's), so i leave it up to John to decide.
--
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick
On Thu, Feb 28, 2008 at 5:56 PM, Luis R. Rodriguez <[email protected]> wrote:
> The following are based on Nick's patches, they address checkpatch
> complaints, make use of pci_dev's is_pcie, and makes use of a helper
> instead on the 7th patch. These need testing on pci-e cards. I've
> tested it on AR5213 (MAC 0x56, PHY: 0x41), RF5211 (0x17) and RF2111
> (0x23).
Testing was done.
> You can also wget them from:
>
> http://www.kernel.org/pub/linux/kernel/people/mcgrof/patches/ath5k/2008-02-28/
We need to move forward with this. Any blockers?
Luis
On Fri, Mar 7, 2008 at 7:32 AM, Nick Kossifidis <[email protected]> wrote:
>
> As i've told you in private, i'm not O.K. with the helper function you
> introduced in 7/8 because:
>
> a) It makes cross-reference with dumps more difficult for no reason.
This is not accurate, I stated the reasons why I did it:
a. To not hit the 80-char limitb
b. To not make multiple hw_writes span two lines
c. To make ath5k_hw_reset() shorter
> b) It doesn't make sense to have a helper function for only one chip
> and for only one single 5212-specific part of code. There is a lot of
> chip-specific code in hw_reset, either we make a helper function for
> each chip containing all chip-specific code or we leave it as it is -i
> prefer the second since order there matters + it will make
> cross-reference much more difficult-, grabbing just a small part of
> chip-specific code and calling it a helper function for that chip
> doesn't make sense and is misleading IMHO.
This I agree with -- what I'll do is keep the code in the reset, some
writes will span more than two lines and then in another independent
patch I'll address re-writing reset split into different logical
components.
> c) It doesn't make sense to introduce a small function that's only
> called once + have the above issues just to avoid 80-column limit.
Actually I think it does, but moreover I think this proves we need a
re-write on ath5k_hw_reset().
> d) In general i don't think introducing a function for style-reasons
> is a good practice since it's misleading and in fact makes code
> readability worse.
I think it depends but in our case, like I said I think we need to
split ath5k_hw_reset() into different logical components to make it
more readable, even if that means taking some code out into helpers.
The benefit will be to use helpers that *do* make sense, even they are
used once. The problem with my helper is its for some writes for which
we are not sure of what they do yet.
> so please resubmit that patch without the helper function and you'll
> get my ACK (well it'll be my patch then + checkpatch fixes so
> ACK/signed-off-by makes no difference :P).
Coming up.
> As for the pci-e patch i've also told you what i think both public and
> in private, however since a fix for these cards (2424/5424/2425) is
> needed asap i'm ok with any given approach (mine + Bob's patch or
> your's), so i leave it up to John to decide.
I do think its best to go with using struct pci_dev's is_pcie member.
We can deal with ahb when we get there and I think there are more
reasonable ways to deal with it and then adding a ah_ispcie, afterall
-- the difference between ahb and other devices is what bus they're
on, not if the device is PCI-E or not.
Luis
OK -- so just to re-cap, patches 1-6 remain the same, patch 7 was
changed to not have the helper but still respect 80-char limit,
and patch 8 modified to apply ontop of new patch 7.
Now series can be wget'd from:
http://www.kernel.org/pub/linux/kernel/people/mcgrof/patches/ath5k/2008-03-07/
Luis