Re: PATCH v3 brcmfmac driver cleanup

2017-07-26 Thread Ian Molton
On 24/07/17 20:47, Arend van Spriel wrote:
> On 19-07-17 21:07, Ian Molton wrote:
>> Hi all,
>>
>> Please find the 3rd revision of my cleanup patchset for brcmfmac.
>>
>> I've done some further cleanup and it now handles SDIO the ay the MMC 
>> subsystem
>> was designed to.
>>
>> I've also taken the liberty of greatly reducing the amount of indirection
>> used thoughout the SDIO code, which I think has improved readability quite a
>> lot.
>>
>> Hope this finds you all well.
> After only one week of vacation there was quite some catching up to do.
> So I decided to get an easy start and run 'checkpatch.pl --strict' over
> the series. Attached the output of that. Might be stuff that was already
> wrong before.

Some of it will be. I can see a couple of lines have crept over 80 chars.

Some lines are ~81-83 chars - I tend to leave those as its usually quite
apparent what is missing off the end. Longer ones I agree should be split.

The original source has some inconsistent whitespace too, which
complicates matters.

> I did only a quick check on patch 1/34 and it is really
> introduced by that patch. So can you please check the output.

Will have a run through and resubmit.

> FWIW, I tend to ignore the warning about block comments, ie.:

Yeah, I tend to agree. /* Blah as the first line of a multiline comment
just looks unbalanced to me.

-Ian


Re: PATCH v3 brcmfmac driver cleanup

2017-07-24 Thread Arend van Spriel
On 19-07-17 21:07, Ian Molton wrote:
> Hi all,
> 
> Please find the 3rd revision of my cleanup patchset for brcmfmac.
> 
> I've done some further cleanup and it now handles SDIO the ay the MMC 
> subsystem
> was designed to.
> 
> I've also taken the liberty of greatly reducing the amount of indirection
> used thoughout the SDIO code, which I think has improved readability quite a
> lot.
> 
> Hope this finds you all well.

After only one week of vacation there was quite some catching up to do.
So I decided to get an easy start and run 'checkpatch.pl --strict' over
the series. Attached the output of that. Might be stuff that was already
wrong before. I did only a quick check on patch 1/34 and it is really
introduced by that patch. So can you please check the output.

FWIW, I tend to ignore the warning about block comments, ie.:

/*
 * bla
 */

is fine by me. At least I prefer it over:

/* bla
 */

Regards,
Arend
branch: fmac-molton-patches
checking patch: a6d80ee brcmfmac: Fix parameter order in brcmf_sdiod_f0_writeb()
WARNING: line over 80 characters
#24: FILE: drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c:233:
+static inline int brcmf_sdiod_f0_writeb(struct sdio_func *func, u8 byte, uint 
regaddr)

WARNING: line over 80 characters
#34: FILE: drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c:271:
+   ret = brcmf_sdiod_f0_writeb(func, *(u8 *)data, 
addr);

total: 0 errors, 2 warnings, 0 checks, 18 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
  mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: If any of the errors are false positives, please report
  them to the maintainer, see CHECKPATCH in MAINTAINERS.
checking patch: f86e392 brcmfmac: Register sizes on hardware are not dependent 
on compiler types
total: 0 errors, 0 warnings, 0 checks, 79 lines checked

Your patch has no obvious style problems and is ready for submission.
checking patch: 700bd76 brcmfmac: Split brcmf_sdiod_regrw_helper() up.
CHECK: Alignment should match open parenthesis
#28: FILE: drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c:304:
+static int brcmf_sdiod_reg_write(struct brcmf_sdio_dev *sdiodev, u32 addr,
+  u8 regsz, void *data)

CHECK: braces {} should be used on all arms of this statement
#48: FILE: drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c:335:
+   if (ret == -ENOMEDIUM)
[...]
+   else if (ret != 0) {
[...]

WARNING: networking block comments don't use an empty /* line, use /* Comment...
#52: FILE: drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c:339:
+   /*
+* SleepCSR register access can fail when

CHECK: Alignment should match open parenthesis
#68: FILE: drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c:355:
+static int brcmf_sdiod_reg_read(struct brcmf_sdio_dev *sdiodev, u32 addr,
+  u8 regsz, void *data)

WARNING: networking block comments don't use an empty /* line, use /* Comment...
#78: FILE: drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c:365:
+   /*
+* figure out how to read the register based on address range

CHECK: Alignment should match open parenthesis
#128: FILE: drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c:422:
+   brcmf_sdiod_regwb(sdiodev, SBSDIO_FUNC1_SBADDRLOW + i, addr[i],
+   );

total: 0 errors, 2 warnings, 4 checks, 151 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
  mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: If any of the errors are false positives, please report
  them to the maintainer, see CHECKPATCH in MAINTAINERS.
checking patch: cc14ab9 brcmfmac: Clean up brcmf_sdiod_set_sbaddr_window()
ERROR: space prohibited after that open parenthesis '('
#39: FILE: drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c:418:
+   for ( i = 0 ; i < 3 && !err ; i++, addr >>= 8 )

ERROR: space prohibited before that close parenthesis ')'
#39: FILE: drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c:418:
+   for ( i = 0 ; i < 3 && !err ; i++, addr >>= 8 )

WARNING: line over 80 characters
#40: FILE: drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c:419:
+   brcmf_sdiod_regwb(sdiodev, SBSDIO_FUNC1_SBADDRLOW + i, addr & 
0xff,

CHECK: Alignment should match open parenthesis
#41: FILE: drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c:420:
+   brcmf_sdiod_regwb(sdiodev, SBSDIO_FUNC1_SBADDRLOW + i, addr & 
0xff,
);

total: 2 errors, 1 warnings, 1 checks, 36 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
  mechanically convert to the typical style using --fix or --fix-inplace.


PATCH v3 brcmfmac driver cleanup

2017-07-19 Thread Ian Molton
Hi all,

Please find the 3rd revision of my cleanup patchset for brcmfmac.

I've done some further cleanup and it now handles SDIO the ay the MMC subsystem
was designed to.

I've also taken the liberty of greatly reducing the amount of indirection
used thoughout the SDIO code, which I think has improved readability quite a
lot.

Hope this finds you all well.

-Ian