linux-wireless.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Ian Molton <ian@mnementh.co.uk>
To: Arend van Spriel <arend.vanspriel@broadcom.com>,
	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 18:40:07 +0100	[thread overview]
Message-ID: <3f51e5bb-c0ea-6c56-66e2-f5fd170bc7c4@mnementh.co.uk> (raw)
In-Reply-To: <97ac668c-fef4-37be-06e1-0135dea77527@mnementh.co.uk>

On 17/07/17 16:56, Ian Molton wrote:
> The two structures *always* exist together. And yes, preventing access
> to the members that we don't want accessed from the wrong layer would
> be *possibly* worthwhile, however the driver code as it stands
> *constantly* short circuits that with the use of offsetof().

inb4: misthought there - it doesnt use offsetof() for that, but my point
stands.

Look, for example, at the core structs:

struct brcmf_core_priv {
	struct brcmf_core pub;
	u32 wrapbase;
	struct list_head list;
	struct brcmf_chip_priv *chip;
};

and

struct brcmf_core {
 	u16 id;
 	u16 rev;
 	u32 base;
}


What is the useful separation being achieved here? both base and
wrapbase are pointers to core address space.

Apart from that, nothing but the core ID and revision are separated from
the other data.

so we have two address space pointers, which are separated for no good
reason, and two derived values which its pretty much irrelevant if
they're changed.

The only other information in there is the list management structures,
which are used in public structures throughout the linux kernel - its
common practice.

A quick grep shows that the struct brcmf_core does not get used outside
chip.c, and struct brcmf_chip is used purely as a pointer in a less than
a handful of places in the bus drivers, which are barely interested in
their internal data (chiprev, etc.).

For these tiny number of cases do we really want to make all of chip.c
and chip.h illegible?

eg:

-	chip->ops->activate(chip->ctx, &chip->pub, rstvec);
+	chip->ops->activate(chip->ctx, chip, rstvec);

Which I was planning to submit a further patch to convert it to simply:

chip->ops->activate(chip, rstvec);

Which is *so* much more readable.

-Ian

  reply	other threads:[~2017-07-17 17:40 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
2017-07-17 15:56   ` Ian Molton
2017-07-17 17:40     ` Ian Molton [this message]
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=3f51e5bb-c0ea-6c56-66e2-f5fd170bc7c4@mnementh.co.uk \
    --to=ian@mnementh.co.uk \
    --cc=arend.vanspriel@broadcom.com \
    --cc=franky.lin@broadcom.com \
    --cc=hante.meuleman@broadcom.com \
    --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).