| Message ID | 20170726202557.15632-2-ian@mnementh.co.uk (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
Hi Ian, So it took me a while to get to this patch series. It is indeed way to big and not well structured making it difficult to review the patches on a per-patch basis. I decided to limit myself to looking at the patches involving bcmsdh.c doing a 'git log bcmsdh.c' on the applied patches. Still twenty patches to review. I reviewed the first 15 patches of the series and skipped patch [13/34] as it did not touch bcmsdh.c. A few response already slipped to the mailing list so I will post the rest shortly. Here is the first one. On 26-07-17 22:25, Ian Molton wrote: > All the other IO functions are the other way round in this > driver. Make this one match. Core SDIO functions are indeed the other way around, but the IO functions in this file use (addr, data) pattern. So should we aim to get it all consistent with core SDIO or just consistent on its own. > Signed-off-by: Ian Molton <ian@mnementh.co.uk> > --- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > index 984c1d0560b1..f585dfd89453 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > @@ -230,8 +230,8 @@ void brcmf_sdiod_change_state(struct brcmf_sdio_dev *sdiodev, > sdiodev->state = state; > } > > -static inline int brcmf_sdiod_f0_writeb(struct sdio_func *func, > - uint regaddr, u8 byte) > +static inline int brcmf_sdiod_f0_writeb(struct sdio_func *func, u8 byte, > + uint regaddr) The second line needs to be aligned on same column as the opening bracket. Regards, Arend
On 07/08/17 12:25, Arend van Spriel wrote: >> All the other IO functions are the other way round in this >> driver. Make this one match. > > Core SDIO functions are indeed the other way around, but the IO > functions in this file use (addr, data) pattern. So should we aim to get > it all consistent with core SDIO or just consistent on its own. This is preparatory for a later patch that removes this abstraction altogether, so yes, it should match the SDIO order. -Ian
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c index 984c1d0560b1..f585dfd89453 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c @@ -230,8 +230,8 @@ void brcmf_sdiod_change_state(struct brcmf_sdio_dev *sdiodev, sdiodev->state = state; } -static inline int brcmf_sdiod_f0_writeb(struct sdio_func *func, - uint regaddr, u8 byte) +static inline int brcmf_sdiod_f0_writeb(struct sdio_func *func, u8 byte, + uint regaddr) { int err_ret; @@ -269,8 +269,8 @@ static int brcmf_sdiod_request_data(struct brcmf_sdio_dev *sdiodev, u8 fn, if (fn) sdio_writeb(func, *(u8 *)data, addr, &ret); else - ret = brcmf_sdiod_f0_writeb(func, addr, - *(u8 *)data); + ret = brcmf_sdiod_f0_writeb(func, *(u8 *)data, + addr); } else { if (fn) *(u8 *)data = sdio_readb(func, addr, &ret);
All the other IO functions are the other way round in this driver. Make this one match. Signed-off-by: Ian Molton <ian@mnementh.co.uk> --- drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)