From: Arend van Spriel <arend.vanspriel@broadcom.com>
To: Ian Molton <ian@mnementh.co.uk>, linux-wireless@vger.kernel.org
Cc: franky.lin@broadcom.com, hante.meuleman@broadcom.com
Subject: Re: RFC: Broadcom fmac wireless driver cleanup
Date: Mon, 17 Jul 2017 14:41:46 +0200 [thread overview]
Message-ID: <72fae7cb-7db0-4979-8e4b-cf26c557634a@broadcom.com> (raw)
In-Reply-To: <20170716112129.10206-1-ian@mnementh.co.uk>
On 16-07-17 13:21, Ian Molton wrote:
> Hi folks,
>
> I've been doing some cleanups in the broadcome brcmfmac driver, trying to
> understand how it works.
>
> I think this makes the driver a *lot* more readable.
Everyone is entitled to his own opinion. While skimming the patches
terms like horrid, abomination, brain damaged does not really get me
motivated in reviewing it let alone accepting it. Especially when I just
start my vacation, but you could not have known ;-)
> Of note:
>
> * Gets rid of the arbitrary and completely 1:1 mapping of
> struct brcmf_{core,chip}_priv/struct brcmf_{core,chip} structures
> which add unreadability whilst offering no real seperation.
It is maybe 1:1 but it assures that whatever is in the priv structures
in only accessible in chip.c. Why would that not offer any real separation.
> * The patch titled HACK - stabilise the value of ->sbwad in use
> highlights an issue that is either bizarre undocumented behaviour or a
> genuine bug - without datasheets, I dont know. Essentially the value of sbwad
> is taken as the base address in several functions, even though it is subject
> to change should other IO occur that moves the window offset
Ok. This needs to be looked at, but as I recall sbwad is only changing
when needed. All IO occurs under claimed host lock so I don't expect
there is a bug here, but will look at it more closely.
> Obviously this is a first cut at this and needs substantial cleanup itself,
> however I wanted to get some idea of wether I should continue working on this.
Now this is where a commit message would really help to explain your
train of thought. What is obvious to you, may not be to others. For
instance changing the order of parameters in a function is simply absurd
although that is my opinion.
> I only have a 43430 SDIO WiFi / BT chip to test on, but other chips *should*
> be unaffected by these changes.
I have a decent amount of SDIO chipsets so count on it that I will run
this patch series when I return from my vacation.
I just noticed some earlier reactions and it seems your are easily
offended starting to use *star* quotes. No need to start shouting. There
is stuff in there that I am happy to acknowledge, but some I think is a
matter of taste. Although mostly there are no arguments given to get the
discussion going. I am sure you have them in mind so don't qualm and
just spew them.
Regards,
Arend
next prev parent reply other threads:[~2017-07-17 12:41 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-07-16 11:21 RFC: Broadcom fmac wireless driver cleanup Ian Molton
2017-07-16 11:21 ` [PATCH 01/21] net: brcmfmac: Write function depends on size of regs not types Ian Molton
2017-07-16 11:21 ` [PATCH 02/21] net: brcmfmac: Fix brain damaged fn parameter order Ian Molton
2017-07-16 11:21 ` [PATCH 03/21] bcmfmac: simplify brcmf_sdiod_abort Ian Molton
2017-07-16 11:21 ` [PATCH 04/21] brcmfmac: Do away with this abomination: Ian Molton
2017-07-16 11:21 ` [PATCH 05/21] brcmfmac: its ALWAYS 4 Ian Molton
2017-07-16 11:21 ` [PATCH 06/21] brcmfmac: Clean up SDIO IO functions Ian Molton
2017-07-16 11:21 ` [PATCH 07/21] brcmfmac: Split buff_rw function up + cleanup annoying bcmerror variable Ian Molton
2017-07-16 11:21 ` [PATCH 08/21] brcmfmac: Sanitise all byte-wise IO Ian Molton
2017-07-16 11:21 ` [PATCH 09/21] brcmfmac: tidy header a bit Ian Molton
2017-07-16 11:21 ` [PATCH 10/21] brcmfmac: Further SDIO addressing cleanup Ian Molton
2017-07-16 11:21 ` [PATCH 11/21] brcmfmac: cleanup horrid offsetof() mess Ian Molton
2017-07-16 11:21 ` [PATCH 12/21] brcmfmac: Fix awfully named #define and crap multi-stage if...elseif clause Ian Molton
2017-07-16 11:21 ` [PATCH 13/21] brcmfmac: HACK - stabilise the value of ->sbwad in use for some xfer routines Ian Molton
2017-07-16 11:21 ` [PATCH 14/21] brcmfmac: Get rid of hideous chip_priv and core_priv structs Ian Molton
2017-07-16 11:21 ` [PATCH 15/21] brcmfmac: Simplify chip probe routine Ian Molton
2017-07-16 11:21 ` [PATCH 16/21] brcmfmac: rename axi functions for clarity Ian Molton
2017-07-16 11:21 ` [PATCH 17/21] brcmfmac: HUGE cleanup of IO access functions Ian Molton
2017-07-16 15:16 ` kbuild test robot
2017-07-16 15:16 ` [PATCH] brcmfmac: fix boolreturn.cocci warnings kbuild test robot
2017-07-16 11:21 ` [PATCH 18/21] brcmfmac: rename ctx -> bus_priv Ian Molton
2017-07-16 11:21 ` [PATCH 19/21] brcmfmac: Remove repeated and annoying calls to brcmf_chip_get_core() Ian Molton
2017-07-16 11:21 ` [PATCH 20/21] brcmfmac: general cleanup Ian Molton
2017-07-16 11:21 ` [PATCH 21/21] brcmfmac: rename horridly named IO functions Ian Molton
2017-07-17 4:53 ` RFC: Broadcom fmac wireless driver cleanup Rafał Miłecki
2017-07-17 9:13 ` James Hughes
2017-07-17 9:45 ` Ian Molton
[not found] ` <707b8832-a09e-9d8a-d4fc-6f9b73306680@mnementh.co.uk>
2017-07-17 10:38 ` Rafał Miłecki
2017-07-21 15:29 ` Kalle Valo
2017-07-17 12:41 ` Arend van Spriel [this message]
2017-07-17 15:56 ` Ian Molton
2017-07-17 17:40 ` Ian Molton
2017-07-17 16:18 ` Ian Molton
2017-07-17 16:20 ` Ian Molton
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=72fae7cb-7db0-4979-8e4b-cf26c557634a@broadcom.com \
--to=arend.vanspriel@broadcom.com \
--cc=franky.lin@broadcom.com \
--cc=hante.meuleman@broadcom.com \
--cc=ian@mnementh.co.uk \
--cc=linux-wireless@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).