| Message ID | 20181017180657.9410-1-natechancellor@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | isdn: hfc_{pci,sx}: Avoid empty body if statements and use proper register accessors | expand |
Hi Nathan, On Thu, Oct 18, 2018 at 3:09 AM Nathan Chancellor <natechancellor@gmail.com> wrote: > > Clang warns: > > drivers/isdn/hisax/hfc_pci.c:131:34: error: if statement has empty body > [-Werror,-Wempty-body] > if (Read_hfc(cs, HFCPCI_INT_S1)); > ^ > drivers/isdn/hisax/hfc_pci.c:131:34: note: put the semicolon on a > separate line to silence this warning > > Use the format found in drivers/isdn/hardware/mISDN/hfcpci.c of casting > the return of Read_hfc to void, instead of using an empty if statement. > > While we're at it, Masahiro Yamada pointed out that {Read,Write}_hfc > should be using a standard access method in hfc_pci.h. Use the one found > in drivers/isdn/hardware/mISDN/hfc_pci.h. > > Link: https://github.com/ClangBuiltLinux/linux/issues/66 > Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > --- > drivers/isdn/hisax/hfc_pci.c | 6 +++--- > drivers/isdn/hisax/hfc_pci.h | 4 ++-- > drivers/isdn/hisax/hfc_sx.c | 6 +++--- > 3 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/drivers/isdn/hisax/hfc_pci.c b/drivers/isdn/hisax/hfc_pci.c > index 8e5b03161b2f..a63b9155b697 100644 > --- a/drivers/isdn/hisax/hfc_pci.c > +++ b/drivers/isdn/hisax/hfc_pci.c > @@ -128,7 +128,7 @@ reset_hfcpci(struct IsdnCardState *cs) > Write_hfc(cs, HFCPCI_INT_M1, cs->hw.hfcpci.int_m1); > > /* Clear already pending ints */ > - if (Read_hfc(cs, HFCPCI_INT_S1)); > + (void) Read_hfc(cs, HFCPCI_INT_S1); Why is this '(void)' necessary? I see no warning without it.
On Thu, Oct 18, 2018 at 12:23:37PM +0900, Masahiro Yamada wrote: > Hi Nathan, > > On Thu, Oct 18, 2018 at 3:09 AM Nathan Chancellor > <natechancellor@gmail.com> wrote: > > > > Clang warns: > > > > drivers/isdn/hisax/hfc_pci.c:131:34: error: if statement has empty body > > [-Werror,-Wempty-body] > > if (Read_hfc(cs, HFCPCI_INT_S1)); > > ^ > > drivers/isdn/hisax/hfc_pci.c:131:34: note: put the semicolon on a > > separate line to silence this warning > > > > Use the format found in drivers/isdn/hardware/mISDN/hfcpci.c of casting > > the return of Read_hfc to void, instead of using an empty if statement. > > > > While we're at it, Masahiro Yamada pointed out that {Read,Write}_hfc > > should be using a standard access method in hfc_pci.h. Use the one found > > in drivers/isdn/hardware/mISDN/hfc_pci.h. > > > > Link: https://github.com/ClangBuiltLinux/linux/issues/66 > > Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com> > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > --- > > drivers/isdn/hisax/hfc_pci.c | 6 +++--- > > drivers/isdn/hisax/hfc_pci.h | 4 ++-- > > drivers/isdn/hisax/hfc_sx.c | 6 +++--- > > 3 files changed, 8 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/isdn/hisax/hfc_pci.c b/drivers/isdn/hisax/hfc_pci.c > > index 8e5b03161b2f..a63b9155b697 100644 > > --- a/drivers/isdn/hisax/hfc_pci.c > > +++ b/drivers/isdn/hisax/hfc_pci.c > > @@ -128,7 +128,7 @@ reset_hfcpci(struct IsdnCardState *cs) > > Write_hfc(cs, HFCPCI_INT_M1, cs->hw.hfcpci.int_m1); > > > > /* Clear already pending ints */ > > - if (Read_hfc(cs, HFCPCI_INT_S1)); > > + (void) Read_hfc(cs, HFCPCI_INT_S1); > > > > Why is this '(void)' necessary? > It's not. I never tested without it since last time I tried to remove the if statement, GCC complained at me but that was before the readb change like you suggested. I will go ahead and send a v2 with that cleaned up. > I see no warning without it. > > > > -- > Best Regards > Masahiro Yamada Thanks for the review! Nathan
diff --git a/drivers/isdn/hisax/hfc_pci.c b/drivers/isdn/hisax/hfc_pci.c index 8e5b03161b2f..a63b9155b697 100644 --- a/drivers/isdn/hisax/hfc_pci.c +++ b/drivers/isdn/hisax/hfc_pci.c @@ -128,7 +128,7 @@ reset_hfcpci(struct IsdnCardState *cs) Write_hfc(cs, HFCPCI_INT_M1, cs->hw.hfcpci.int_m1); /* Clear already pending ints */ - if (Read_hfc(cs, HFCPCI_INT_S1)); + (void) Read_hfc(cs, HFCPCI_INT_S1); Write_hfc(cs, HFCPCI_STATES, HFCPCI_LOAD_STATE | 2); /* HFC ST 2 */ udelay(10); @@ -158,7 +158,7 @@ reset_hfcpci(struct IsdnCardState *cs) /* Finally enable IRQ output */ cs->hw.hfcpci.int_m2 = HFCPCI_IRQ_ENABLE; Write_hfc(cs, HFCPCI_INT_M2, cs->hw.hfcpci.int_m2); - if (Read_hfc(cs, HFCPCI_INT_S1)); + (void) Read_hfc(cs, HFCPCI_INT_S1); } /***************************************************/ @@ -1537,7 +1537,7 @@ hfcpci_bh(struct work_struct *work) cs->hw.hfcpci.int_m1 &= ~HFCPCI_INTS_TIMER; Write_hfc(cs, HFCPCI_INT_M1, cs->hw.hfcpci.int_m1); /* Clear already pending ints */ - if (Read_hfc(cs, HFCPCI_INT_S1)); + (void) Read_hfc(cs, HFCPCI_INT_S1); Write_hfc(cs, HFCPCI_STATES, 4 | HFCPCI_LOAD_STATE); udelay(10); Write_hfc(cs, HFCPCI_STATES, 4); diff --git a/drivers/isdn/hisax/hfc_pci.h b/drivers/isdn/hisax/hfc_pci.h index 4e58700a3e61..4c3b3ba35726 100644 --- a/drivers/isdn/hisax/hfc_pci.h +++ b/drivers/isdn/hisax/hfc_pci.h @@ -228,8 +228,8 @@ typedef union { } fifo_area; -#define Write_hfc(a, b, c) (*(((u_char *)a->hw.hfcpci.pci_io) + b) = c) -#define Read_hfc(a, b) (*(((u_char *)a->hw.hfcpci.pci_io) + b)) +#define Write_hfc(a, b, c) (writeb(c, (a->hw.hfcpci.pci_io) + b)) +#define Read_hfc(a, b) (readb((a->hw.hfcpci.pci_io) + b)) extern void main_irq_hcpci(struct BCState *bcs); extern void releasehfcpci(struct IsdnCardState *cs); diff --git a/drivers/isdn/hisax/hfc_sx.c b/drivers/isdn/hisax/hfc_sx.c index 4d3b4b2f2612..c4f3f37adfc8 100644 --- a/drivers/isdn/hisax/hfc_sx.c +++ b/drivers/isdn/hisax/hfc_sx.c @@ -381,7 +381,7 @@ reset_hfcsx(struct IsdnCardState *cs) Write_hfc(cs, HFCSX_INT_M1, cs->hw.hfcsx.int_m1); /* Clear already pending ints */ - if (Read_hfc(cs, HFCSX_INT_S1)); + (void) Read_hfc(cs, HFCSX_INT_S1); Write_hfc(cs, HFCSX_STATES, HFCSX_LOAD_STATE | 2); /* HFC ST 2 */ udelay(10); @@ -411,7 +411,7 @@ reset_hfcsx(struct IsdnCardState *cs) /* Finally enable IRQ output */ cs->hw.hfcsx.int_m2 = HFCSX_IRQ_ENABLE; Write_hfc(cs, HFCSX_INT_M2, cs->hw.hfcsx.int_m2); - if (Read_hfc(cs, HFCSX_INT_S2)); + (void) Read_hfc(cs, HFCSX_INT_S2); } /***************************************************/ @@ -1288,7 +1288,7 @@ hfcsx_bh(struct work_struct *work) cs->hw.hfcsx.int_m1 &= ~HFCSX_INTS_TIMER; Write_hfc(cs, HFCSX_INT_M1, cs->hw.hfcsx.int_m1); /* Clear already pending ints */ - if (Read_hfc(cs, HFCSX_INT_S1)); + (void) Read_hfc(cs, HFCSX_INT_S1); Write_hfc(cs, HFCSX_STATES, 4 | HFCSX_LOAD_STATE); udelay(10);
Clang warns: drivers/isdn/hisax/hfc_pci.c:131:34: error: if statement has empty body [-Werror,-Wempty-body] if (Read_hfc(cs, HFCPCI_INT_S1)); ^ drivers/isdn/hisax/hfc_pci.c:131:34: note: put the semicolon on a separate line to silence this warning Use the format found in drivers/isdn/hardware/mISDN/hfcpci.c of casting the return of Read_hfc to void, instead of using an empty if statement. While we're at it, Masahiro Yamada pointed out that {Read,Write}_hfc should be using a standard access method in hfc_pci.h. Use the one found in drivers/isdn/hardware/mISDN/hfc_pci.h. Link: https://github.com/ClangBuiltLinux/linux/issues/66 Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> --- drivers/isdn/hisax/hfc_pci.c | 6 +++--- drivers/isdn/hisax/hfc_pci.h | 4 ++-- drivers/isdn/hisax/hfc_sx.c | 6 +++--- 3 files changed, 8 insertions(+), 8 deletions(-)