On 26-07-17 22:25, Ian Molton wrote:
> This introduces no functional changes, but makes the code drastically more
> readable, reducing the amount of dereferencing performed inside functions
> throughout the SDIO core.
>
> For example, reduce:
> sdio_release_host(bus->sdiodev->func1);
> to:
> sdio_release_host(func1);
>
> Fixup a few inconsistently named pointers whilst we are at it ie.
>
> sdiod -> sdiodev
Last but not least so to speak. I think "drastically" is a bit
overstated. In terms of readability it can go either way at far as I am
concerned. At least the above example does. In terms of instructions it
depends. Both dereferencing and helper variables on stack have their
price. So I (try to) use some soft limits when it comes to
dereferencing. The depth should not be more than 2, ie.:
bus->sdiodev->func1 is ok, bus->sdiodev->func1->card is not.
The number of dereferences within a particular code path in the function
should not be more than 3.
Also there are some places where you add both func1 and func2 to the
stack, but if you look at it you could do with just one of them.
>From this series I reviewed patches 1 upto and including patch 15, and
patches 29 through 34. Please rework those as requested and resubmit
them. Please also resubmit the remaining patch after that and consider
splitting it up file based. For example below could be a patch series
for chip.c related changes:
$ git log gerrit/brcm-wl-master..HEAD --reverse --oneline --
drivers/net/wireless/broadcom/brcm80211/brcmfmac/chip.c
4c75cf9 brcmfmac: Rename SOC_AI to SOC_AXI
acf9f33 brcmfmac: Get rid of chip_priv and core_priv structs
57cef44 brcmfmac: Whitespace patch
fe23a50 brcmfmac: Simplify chip probe routine
89e5bb5 brcmfmac: Rename axi functions for clarity.
a6352d7 brcmfmac: HUGE cleanup of IO access functions.
ee7ea4f brcmfmac: Rename chip.ctx -> chip.bus_priv
Reviewed-by: Arend van Spriel <arend.vanspriel@xxxxxxxxxxxx>
> Signed-off-by: Ian Molton <ian@xxxxxxxxxxxxxx>
comments below...
> # Conflicts:
> # drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> # drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> ---
> .../wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 140 +++----
> .../wireless/broadcom/brcm80211/brcmfmac/sdio.c | 409 +++++++++++----------
> 2 files changed, 294 insertions(+), 255 deletions(-)
[...]
> int brcmf_sdio_sleep(struct brcmf_sdio *bus, bool sleep)
> {
> + struct brcmf_sdio_dev *sdiodev = bus->sdiodev;
> + struct sdio_func *func1 = sdiodev->func1;
Actually only wanted to explicitly mention this one. Probably the
compiler is smart enough to keep sdiodev from the stack, but I would say
it is a variable you do not really need and also in terms of readability
it seems clear enough to do it in one assignment.
> int ret;
>
> - sdio_claim_host(bus->sdiodev->func1);
> + sdio_claim_host(func1);
> ret = brcmf_sdio_bus_sleep(bus, sleep, false);
> - sdio_release_host(bus->sdiodev->func1);
> + sdio_release_host(func1);
>
> return ret;
> }
>
