Modified for kernel coding style rule of hfs-s+/sp device driver .
. reference: ./Documentation/CodingStyle
ex)
60 Don't put multiple statements on a single line unless you have
61 something to hide:
62
63 if (condition) do_this;
64 do_something_everytime;
Signed-off-by: Geunsik Lim <[email protected]>
---
drivers/isdn/hisax/hfc_sx.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/isdn/hisax/hfc_sx.c b/drivers/isdn/hisax/hfc_sx.c
index 156d7c6..7c41bd8 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));
+ 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));
+ Read_hfc(cs, HFCSX_INT_S2);
}
/***************************************************/
@@ -1286,7 +1286,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));
+ Read_hfc(cs, HFCSX_INT_S1);
Write_hfc(cs, HFCSX_STATES, 4 | HFCSX_LOAD_STATE);
udelay(10);
--
1.7.8.1
From: Geunsik Lim <[email protected]>
Date: Wed, 1 Feb 2012 15:59:53 +0900
> Modified for kernel coding style rule of hfs-s+/sp device driver .
> . reference: ./Documentation/CodingStyle
>
> ex)
> 60 Don't put multiple statements on a single line unless you have
> 61 something to hide:
> 62
> 63 if (condition) do_this;
> 64 do_something_everytime;
>
> Signed-off-by: Geunsik Lim <[email protected]>
This was probably there to eliminate compiler warnings or avoid the
code being eliminated completely, because the result of the register
read is unused.
Have you verified that neither is the case here?
To be honest I very strongly dislike patches like this. You're
patching a driver that very few people use, and changes like this can
only risk possible regressions that will be hard to notice and it's
not like people scour over this driver often and thus will be upset by
frequently seeing some minor coding style infraction.
I'm not applying this patch.
From: Geunsik Lim <[email protected]>
Date: Wed, 1 Feb 2012 16:45:09 +0900
> On Wed, Feb 1, 2012 at 4:06 PM, David Miller <[email protected]> wrote:
>
>> From: Geunsik Lim <[email protected]>
>> Date: Wed, 1 Feb 2012 15:59:53 +0900
>>
>> > Modified for kernel coding style rule of hfs-s+/sp device driver .
>> > . reference: ./Documentation/CodingStyle
>> >
>> > ex)
>> > 60 Don't put multiple statements on a single line unless you have
>> > 61 something to hide:
>> > 62
>> > 63 if (condition) do_this;
>> > 64 do_something_everytime;
>> >
>> > Signed-off-by: Geunsik Lim <[email protected]>
>>
>> This was probably there to eliminate compiler warnings or avoid the
>>
> Thank you for your opinion.
> It's strange. I did not meet compiler warnings you replied.
> What is the version of GCC that You tried to compile Linux kernel source
> in your experience?
I'm saying the code was likely written this way "because" of that
reason, I didn't say I saw such a warning. Where did I say I saw
the warning in question?
I stated a very likely reason why the original code was written the
way that it was.
>> code being eliminated completely, because the result of the register
>> read is unused.
>>
> Are you mean that the result of the register read is used if we append
> "if statement" of c language?
I was saying that without some kind of use of the interface's return
value, without some other control such as a volatile asm statement or
a reference to a volatile memory location, the compiler might legally
be able to remove the I/O register read completely.
You need to make sure that the assembler code does not change due to
your changes, and in particular these I/O register read calls do not
get eliminated.
On Wed, Feb 01, 2012 at 02:48:43AM -0500, David Miller wrote:
> From: Geunsik Lim <[email protected]>
> Date: Wed, 1 Feb 2012 16:45:09 +0900
>
> > On Wed, Feb 1, 2012 at 4:06 PM, David Miller <[email protected]> wrote:
> >
> >> From: Geunsik Lim <[email protected]>
> >> Date: Wed, 1 Feb 2012 15:59:53 +0900
> >>
> >> > Modified for kernel coding style rule of hfs-s+/sp device driver .
> >> > . reference: ./Documentation/CodingStyle
> >> >
> >> > ex)
> >> > 60 Don't put multiple statements on a single line unless you have
> >> > 61 something to hide:
> >> > 62
> >> > 63 if (condition) do_this;
> >> > 64 do_something_everytime;
> >> >
> >> > Signed-off-by: Geunsik Lim <[email protected]>
> >>
> >> This was probably there to eliminate compiler warnings or avoid the
Yes it was.
> >>
> > Thank you for your opinion.
> > It's strange. I did not meet compiler warnings you replied.
I did not remember which version it was, it must been arround the time
when that code was developed. I did not like this method, but this was
at this time the suggested workaround from the GCC guys the problem is,
even when you read the value into a register, it makes no difference, you
cannot do anything with it.
I agree with David, such patches are not really needed, the danger that
something gets wrong is too high.
I think in this case a coding style violation is minor to a warning or
potencial miscompiling.
Do not misunderstand me that I do not like to make the code better and more
readable, but such small style violations should be only fixed when here is
a strong need or the driver is reworked in bigger parts and full testing
is done.
Best Regards
Karsten
On Thu, 2012-02-02 at 20:01 +0100, Karsten Keil wrote:
> > >> This was probably there to eliminate compiler warnings or avoid the
> Yes it was.
[]
> I think in this case a coding style violation is minor to a warning or
> potencial miscompiling.
> Do not misunderstand me that I do not like to make the code better and more
> readable, but such small style violations should be only fixed when here is
> a strong need or the driver is reworked in bigger parts and full testing
> is done.
I agree, but in the future (or perhaps even today)
it's better to mark these odd coding style uses with
macros to indicate the reason for their use.
Perhaps something akin to the ACCESS_ONCE macro like
#define PERFORM_ONCE(expr) \
do { \
if ((expr)) \
; \
} while (0)