Re: [PATCH 03/34] brcmfmac: Split brcmf_sdiod_regrw_helper() up.

2017-08-07 Thread Ian Molton 
On 07/08/17 12:25, Arend van Spriel wrote:
> On 26-07-17 22:25, Ian Molton wrote:
>> This large function is concealing a LOT of obscure logic about
>> how the hardware functions. Time to split it up.
>>
>> This first patch splits the function into two pieces - read and write,
>> doing away with the rw flag in the process.
> 
> I really don't this it is all that obscure, but alas. Everything is in
> the eye of the beholder. The reason for having the helper was to not
> duplicate code for read and write and different access sizes. So now you
> are duplicating it. In subsequent patches you throw away pieces of this
> helper so duplication is not as bad in the net result. It would have
> been easier if those patches were done before this one.

I agree, this is a big and unwieldy patchset - I've attempted to break
the thing down in such a way that all the steps leading to the end
result are at least sane. I initially did it all in one hit and it was
utterly illegible :-(


> Fix the indent and column align to opening bracket.

I guess a few of these got through. I blame git rebase :)

-Ian

Re: [PATCH 03/34] brcmfmac: Split brcmf_sdiod_regrw_helper() up.

2017-08-07 Thread Arend van Spriel 

On 26-07-17 22:25, Ian Molton wrote:

This large function is concealing a LOT of obscure logic about
how the hardware functions. Time to split it up.

This first patch splits the function into two pieces - read and write,
doing away with the rw flag in the process.


I really don't this it is all that obscure, but alas. Everything is in
the eye of the beholder. The reason for having the helper was to not
duplicate code for read and write and different access sizes. So now you
are duplicating it. In subsequent patches you throw away pieces of this
helper so duplication is not as bad in the net result. It would have
been easier if those patches were done before this one.

Reviewed-by: Arend van Spriel 

Signed-off-by: Ian Molton 


some minor comment below


---
 .../wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c  | 94 +-
 1 file changed, 73 insertions(+), 21 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
index 2b441ce91d5f..1ee0f91b6c50 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
@@ -302,8 +302,8 @@ static int brcmf_sdiod_request_data(struct brcmf_sdio_dev 
*sdiodev, u8 fn,
return ret;
 }

-static int brcmf_sdiod_regrw_helper(struct brcmf_sdio_dev *sdiodev, u32 addr,
-  u8 regsz, void *data, bool write)
+static int brcmf_sdiod_reg_write(struct brcmf_sdio_dev *sdiodev, u32 addr,
+u8 regsz, void *data)


Fix the indent and column align to opening bracket.

Regards,
Arend

[PATCH 03/34] brcmfmac: Split brcmf_sdiod_regrw_helper() up.

2017-07-26 Thread Ian Molton 
This large function is concealing a LOT of obscure logic about
how the hardware functions. Time to split it up.

This first patch splits the function into two pieces - read and write,
doing away with the rw flag in the process.

Signed-off-by: Ian Molton 
---
 .../wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c  | 94 +-
 1 file changed, 73 insertions(+), 21 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
index 2b441ce91d5f..1ee0f91b6c50 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
@@ -302,8 +302,8 @@ static int brcmf_sdiod_request_data(struct brcmf_sdio_dev 
*sdiodev, u8 fn,
return ret;
 }
 
-static int brcmf_sdiod_regrw_helper(struct brcmf_sdio_dev *sdiodev, u32 addr,
-  u8 regsz, void *data, bool write)
+static int brcmf_sdiod_reg_write(struct brcmf_sdio_dev *sdiodev, u32 addr,
+u8 regsz, void *data)
 {
u8 func;
s32 retry = 0;
@@ -324,13 +324,66 @@ static int brcmf_sdiod_regrw_helper(struct brcmf_sdio_dev 
*sdiodev, u32 addr,
func = SDIO_FUNC_1;
 
do {
-   if (!write)
-   memset(data, 0, regsz);
/* for retry wait for 1 ms till bus get settled down */
if (retry)
usleep_range(1000, 2000);
+
+   ret = brcmf_sdiod_request_data(sdiodev, func, addr, regsz,
+  data, true);
+
+   } while (ret != 0 && ret != -ENOMEDIUM &&
+retry++ < SDIOH_API_ACCESS_RETRY_LIMIT);
+
+   if (ret == -ENOMEDIUM) {
+   brcmf_sdiod_change_state(sdiodev, BRCMF_SDIOD_NOMEDIUM);
+   } else if (ret != 0) {
+   /*
+* SleepCSR register access can fail when
+* waking up the device so reduce this noise
+* in the logs.
+*/
+   if (addr != SBSDIO_FUNC1_SLEEPCSR)
+   brcmf_err("failed to write data F%d@0x%05x, err: %d\n",
+ func, addr, ret);
+   else
+   brcmf_dbg(SDIO, "failed to write data F%d@0x%05x, err: 
%d\n",
+ func, addr, ret);
+   }
+
+   return ret;
+}
+
+static int brcmf_sdiod_reg_read(struct brcmf_sdio_dev *sdiodev, u32 addr,
+   u8 regsz, void *data)
+{
+   u8 func;
+   s32 retry = 0;
+   int ret;
+
+   if (sdiodev->state == BRCMF_SDIOD_NOMEDIUM)
+   return -ENOMEDIUM;
+
+   /*
+* figure out how to read the register based on address range
+* 0x00 ~ 0x7FF: function 0 CCCR and FBR
+* 0x1 ~ 0x1: function 1 miscellaneous registers
+* The rest: function 1 silicon backplane core registers
+*/
+   if ((addr & ~REG_F0_REG_MASK) == 0)
+   func = SDIO_FUNC_0;
+   else
+   func = SDIO_FUNC_1;
+
+   do {
+   memset(data, 0, regsz);
+
+   /* for retry wait for 1 ms till bus get settled down */
+   if (retry)
+   usleep_range(1000, 2000);
+
ret = brcmf_sdiod_request_data(sdiodev, func, addr, regsz,
-  data, write);
+  data, false);
+
} while (ret != 0 && ret != -ENOMEDIUM &&
 retry++ < SDIOH_API_ACCESS_RETRY_LIMIT);
 
@@ -343,12 +396,13 @@ static int brcmf_sdiod_regrw_helper(struct brcmf_sdio_dev 
*sdiodev, u32 addr,
 * in the logs.
 */
if (addr != SBSDIO_FUNC1_SLEEPCSR)
-   brcmf_err("failed to %s data F%d@0x%05x, err: %d\n",
- write ? "write" : "read", func, addr, ret);
+   brcmf_err("failed to read data F%d@0x%05x, err: %d\n",
+ func, addr, ret);
else
-   brcmf_dbg(SDIO, "failed to %s data F%d@0x%05x, err: 
%d\n",
- write ? "write" : "read", func, addr, ret);
+   brcmf_dbg(SDIO, "failed to read data F%d@0x%05x, err: 
%d\n",
+ func, addr, ret);
}
+
return ret;
 }
 
@@ -366,13 +420,11 @@ brcmf_sdiod_set_sbaddr_window(struct brcmf_sdio_dev 
*sdiodev, u32 address)
addr[2] = (address >> 24) & SBSDIO_SBADDRHIGH_MASK;
 
for (i = 0; i < 3; i++) {
-   err = brcmf_sdiod_regrw_helper(sdiodev,
-  SBSDIO_FUNC1_SBADDRLOW + i,
-  1, [i], true);
+   brcmf_sdiod_regwb(sdiodev, SBSDIO_FUNC1_SBADDRLOW +

[PATCH 03/34] brcmfmac: Split brcmf_sdiod_regrw_helper() up.

2017-07-19 Thread Ian Molton 
This large function is concealing a LOT of obscure logic about
how the hardware functions. Time to split it up.

This first patch splits the function into two pieces - read and write,
doing away with the rw flag in the process.

Signed-off-by: Ian Molton 
---
 .../wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c  | 94 +-
 1 file changed, 73 insertions(+), 21 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
index db983168ceba..5b9a21d6b6b2 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
@@ -300,8 +300,8 @@ static int brcmf_sdiod_request_data(struct brcmf_sdio_dev 
*sdiodev, u8 fn,
return ret;
 }
 
-static int brcmf_sdiod_regrw_helper(struct brcmf_sdio_dev *sdiodev, u32 addr,
-  u8 regsz, void *data, bool write)
+static int brcmf_sdiod_reg_write(struct brcmf_sdio_dev *sdiodev, u32 addr,
+  u8 regsz, void *data)
 {
u8 func;
s32 retry = 0;
@@ -322,13 +322,66 @@ static int brcmf_sdiod_regrw_helper(struct brcmf_sdio_dev 
*sdiodev, u32 addr,
func = SDIO_FUNC_1;
 
do {
-   if (!write)
-   memset(data, 0, regsz);
/* for retry wait for 1 ms till bus get settled down */
if (retry)
usleep_range(1000, 2000);
+
+   ret = brcmf_sdiod_request_data(sdiodev, func, addr, regsz,
+  data, true);
+
+   } while (ret != 0 && ret != -ENOMEDIUM &&
+retry++ < SDIOH_API_ACCESS_RETRY_LIMIT);
+
+   if (ret == -ENOMEDIUM)
+   brcmf_sdiod_change_state(sdiodev, BRCMF_SDIOD_NOMEDIUM);
+   else if (ret != 0) {
+   /*
+* SleepCSR register access can fail when
+* waking up the device so reduce this noise
+* in the logs.
+*/
+   if (addr != SBSDIO_FUNC1_SLEEPCSR)
+   brcmf_err("failed to write data F%d@0x%05x, err: %d\n",
+ func, addr, ret);
+   else
+   brcmf_dbg(SDIO, "failed to write data F%d@0x%05x, err: 
%d\n",
+ func, addr, ret);
+   }
+
+   return ret;
+}
+
+static int brcmf_sdiod_reg_read(struct brcmf_sdio_dev *sdiodev, u32 addr,
+  u8 regsz, void *data)
+{
+   u8 func;
+   s32 retry = 0;
+   int ret;
+
+   if (sdiodev->state == BRCMF_SDIOD_NOMEDIUM)
+   return -ENOMEDIUM;
+
+   /*
+* figure out how to read the register based on address range
+* 0x00 ~ 0x7FF: function 0 CCCR and FBR
+* 0x1 ~ 0x1: function 1 miscellaneous registers
+* The rest: function 1 silicon backplane core registers
+*/
+   if ((addr & ~REG_F0_REG_MASK) == 0)
+   func = SDIO_FUNC_0;
+   else
+   func = SDIO_FUNC_1;
+
+   do {
+   memset(data, 0, regsz);
+
+   /* for retry wait for 1 ms till bus get settled down */
+   if (retry)
+   usleep_range(1000, 2000);
+
ret = brcmf_sdiod_request_data(sdiodev, func, addr, regsz,
-  data, write);
+  data, false);
+
} while (ret != 0 && ret != -ENOMEDIUM &&
 retry++ < SDIOH_API_ACCESS_RETRY_LIMIT);
 
@@ -341,12 +394,13 @@ static int brcmf_sdiod_regrw_helper(struct brcmf_sdio_dev 
*sdiodev, u32 addr,
 * in the logs.
 */
if (addr != SBSDIO_FUNC1_SLEEPCSR)
-   brcmf_err("failed to %s data F%d@0x%05x, err: %d\n",
- write ? "write" : "read", func, addr, ret);
+   brcmf_err("failed to read data F%d@0x%05x, err: %d\n",
+ func, addr, ret);
else
-   brcmf_dbg(SDIO, "failed to %s data F%d@0x%05x, err: 
%d\n",
- write ? "write" : "read", func, addr, ret);
+   brcmf_dbg(SDIO, "failed to read data F%d@0x%05x, err: 
%d\n",
+ func, addr, ret);
}
+
return ret;
 }
 
@@ -364,13 +418,11 @@ brcmf_sdiod_set_sbaddr_window(struct brcmf_sdio_dev 
*sdiodev, u32 address)
addr[2] = (address >> 24) & SBSDIO_SBADDRHIGH_MASK;
 
for (i = 0; i < 3; i++) {
-   err = brcmf_sdiod_regrw_helper(sdiodev,
-  SBSDIO_FUNC1_SBADDRLOW + i,
-  1, [i], true);
+   brcmf_sdiod_regwb(sdiodev, SBSDIO_FUNC1_SBADDRLOW +