| Message ID | 1433200158-6890-5-git-send-email-vz@mleia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jun 02, 2015 at 02:09:16AM +0300, Vladimir Zapolskiy wrote: > The main intention of the change is to remove bitwise operations on > GPIO level value as a preceding change before updating gpiolib > callbacks to utilize bool type to represent GPIO level. > > No functional change. > > Signed-off-by: Vladimir Zapolskiy <vz@mleia.com> > Cc: Charles Keepax <ckeepax@opensource.wolfsonmicro.com> > Cc: Lars-Peter Clausen <lars@metafoo.de> > Cc: Axel Lin <axel.lin@ingics.com> > Cc: patches@opensource.wolfsonmicro.com > --- Acked-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com> Thanks, Charles
On Tue, Jun 02, 2015 at 02:09:16AM +0300, Vladimir Zapolskiy wrote: > @@ -2244,26 +2244,27 @@ static inline struct wm5100_priv *gpio_to_wm5100(struct gpio_chip *chip) > static void wm5100_gpio_set(struct gpio_chip *chip, unsigned offset, int value) > { > struct wm5100_priv *wm5100 = gpio_to_wm5100(chip); > + unsigned int val = 0; > + > + if (value) > + val = 0x1 << WM5100_GP1_LVL_SHIFT; Write this as an if/else so the reader doesn't have to wonder why you've missed the handling of the false case.
Hello Mark, On 02.06.2015 22:45, Mark Brown wrote: > On Tue, Jun 02, 2015 at 02:09:16AM +0300, Vladimir Zapolskiy wrote: > >> @@ -2244,26 +2244,27 @@ static inline struct wm5100_priv *gpio_to_wm5100(struct gpio_chip *chip) >> static void wm5100_gpio_set(struct gpio_chip *chip, unsigned offset, int value) >> { >> struct wm5100_priv *wm5100 = gpio_to_wm5100(chip); >> + unsigned int val = 0; >> + >> + if (value) >> + val = 0x1 << WM5100_GP1_LVL_SHIFT; > > Write this as an if/else so the reader doesn't have to wonder why you've > missed the handling of the false case. > the only objection I have is that the resulting code will be two lines longer. If you think this code is not clear enough (is "val" vs. "value" misleading?), I'll change the rest of my patches, which contain the same logic structure, please let me know. -- With best wishes, Vladimir
On Tue, Jun 02, 2015 at 11:23:41PM +0300, Vladimir Zapolskiy wrote: > On 02.06.2015 22:45, Mark Brown wrote: > > On Tue, Jun 02, 2015 at 02:09:16AM +0300, Vladimir Zapolskiy wrote: > >> + unsigned int val = 0; > >> + > >> + if (value) > >> + val = 0x1 << WM5100_GP1_LVL_SHIFT; > > Write this as an if/else so the reader doesn't have to wonder why you've > > missed the handling of the false case. > the only objection I have is that the resulting code will be two lines > longer. If you think this code is not clear enough (is "val" vs. "value" > misleading?), I'll change the rest of my patches, which contain the same > logic structure, please let me know. Especially after the unrelated style change thing earlier on (which meant I was reading things more carefully than usual) it'd be good to make things as clear as possible - you're right that the val vs value thing isn't helping either. Like I say I am a bit surprised that the int/bool conversion doesn't do the right thing without any code changes other than the type of the parameter but ICBW, I didn't actually check.
Hello Mark, On 02.06.2015 23:36, Mark Brown wrote: > On Tue, Jun 02, 2015 at 11:23:41PM +0300, Vladimir Zapolskiy wrote: >> On 02.06.2015 22:45, Mark Brown wrote: >>> On Tue, Jun 02, 2015 at 02:09:16AM +0300, Vladimir Zapolskiy wrote: > >>>> + unsigned int val = 0; >>>> + >>>> + if (value) >>>> + val = 0x1 << WM5100_GP1_LVL_SHIFT; > >>> Write this as an if/else so the reader doesn't have to wonder why you've >>> missed the handling of the false case. > >> the only objection I have is that the resulting code will be two lines >> longer. If you think this code is not clear enough (is "val" vs. "value" >> misleading?), I'll change the rest of my patches, which contain the same >> logic structure, please let me know. > > Especially after the unrelated style change thing earlier on (which > meant I was reading things more carefully than usual) it'd be good to > make things as clear as possible - you're right that the val vs value > thing isn't helping either. got your concern, will send an update. > Like I say I am a bit surprised that the int/bool conversion doesn't do > the right thing without any code changes other than the type of the > parameter but ICBW, I didn't actually check. > My tested compilers do the work right, but I can not be sure about the whole variety of compilers, well, probably I should just follow the standard, which in turn is expected to be quite volatile in future revisions regarding usage of "_Bool" or future "bool" type. If we assume the preferred Linux kernel style to use true/false constants for boolean variables only (see the reference to bool{init,return}.cocci), then even if bitwise and arithmetic operations on bool type are understandable to a compiler, but probably "false << true" (or whatever is hidden under a variable name) is not quite aesthetic for a human eye. This is not a technical argument though, and I'm not sure if any clear code style policy related to the case is agreed. -- With best wishes, Vladimir
On Tue, Jun 2, 2015 at 1:23 PM, Vladimir Zapolskiy <vz@mleia.com> wrote: > Hello Mark, > > On 02.06.2015 22:45, Mark Brown wrote: > > On Tue, Jun 02, 2015 at 02:09:16AM +0300, Vladimir Zapolskiy wrote: > > > >> @@ -2244,26 +2244,27 @@ static inline struct wm5100_priv > *gpio_to_wm5100(struct gpio_chip *chip) > >> static void wm5100_gpio_set(struct gpio_chip *chip, unsigned offset, > int value) > >> { > >> struct wm5100_priv *wm5100 = gpio_to_wm5100(chip); > >> + unsigned int val = 0; > >> + > >> + if (value) > >> + val = 0x1 << WM5100_GP1_LVL_SHIFT; > > > > Write this as an if/else so the reader doesn't have to wonder why you've > > missed the handling of the false case. > > > > the only objection I have is that the resulting code will be two lines > longer. If you think this code is not clear enough (is "val" vs. "value" > misleading?), I'll change the rest of my patches, which contain the same > logic structure, please let me know. > const unsigned int val = value ? (0x1 << WM5100_GP1_LVL_SHIFT) : 0; Two lines shorter.... Have you measured the effect of going from int to bool on code size and speed? Clearly, the C code is getting longer as '!!' is transformed into an if-then-else to set a new scratch variable. But the compiler likely converts both to a conditional branch or cmov type instruction. What I wonder is if converting gpiolib to use bools instead of ints is better just because someone likes it more that way or if there are objective metrics that show improvement. BTW, with a little C algebra: const unsigned int val = value ? 0x1 << WM5100_GP1_LVL_SHIFT : 0; const unsigned int val = (value ? 0x1 : 0) << WM5100_GP1_LVL_SHIFT; const unsigned int val = (!!value) << WM5100_GP1_LVL_SHIFT; // definition of ! operator And now we're back to where we started, so I don't really see why this is even necessary. The semantics of the ! operator will be changed in a future C version?
On Wed, Jun 03, 2015 at 03:50:04AM -0700, Trent Piepho wrote: > BTW, with a little C algebra: > const unsigned int val = value ? 0x1 << WM5100_GP1_LVL_SHIFT : 0; > const unsigned int val = (value ? 0x1 : 0) << WM5100_GP1_LVL_SHIFT; > const unsigned int val = (!!value) << WM5100_GP1_LVL_SHIFT; // definition > of ! operator > And now we're back to where we started, so I don't really see why this is > even necessary. The semantics of the ! operator will be changed in a > future C version? Yes, this is exactly the point I was trying to make.
Hello Mark, Trent, thank you for review. On 03.06.2015 13:50, Trent Piepho wrote: > On Tue, Jun 2, 2015 at 1:23 PM, Vladimir Zapolskiy <vz@mleia.com > <mailto:vz@mleia.com>> wrote: > > Hello Mark, > > On 02.06.2015 22:45, Mark Brown wrote: > > On Tue, Jun 02, 2015 at 02:09:16AM +0300, Vladimir Zapolskiy wrote: > > > >> @@ -2244,26 +2244,27 @@ static inline struct wm5100_priv > *gpio_to_wm5100(struct gpio_chip *chip) > >> static void wm5100_gpio_set(struct gpio_chip *chip, unsigned > offset, int value) > >> { > >> struct wm5100_priv *wm5100 = gpio_to_wm5100(chip); > >> + unsigned int val = 0; > >> + > >> + if (value) > >> + val = 0x1 << WM5100_GP1_LVL_SHIFT; > > > > Write this as an if/else so the reader doesn't have to wonder why > you've > > missed the handling of the false case. > > > > the only objection I have is that the resulting code will be two lines > longer. If you think this code is not clear enough (is "val" vs. "value" > misleading?), I'll change the rest of my patches, which contain the same > logic structure, please let me know. > > > const unsigned int val = value ? (0x1 << WM5100_GP1_LVL_SHIFT) : 0; > > Two lines shorter.... > > Have you measured the effect of going from int to bool on code size and > speed? Clearly, the C code is getting longer as '!!' is transformed > into an if-then-else to set a new scratch variable. But the compiler > likely converts both to a conditional branch or cmov type instruction. > What I wonder is if converting gpiolib to use bools instead of ints is > better just because someone likes it more that way or if there are > objective metrics that show improvement. > > BTW, with a little C algebra: > const unsigned int val = value ? 0x1 << WM5100_GP1_LVL_SHIFT : 0; > const unsigned int val = (value ? 0x1 : 0) << WM5100_GP1_LVL_SHIFT; > const unsigned int val = (!!value) << WM5100_GP1_LVL_SHIFT; // > definition of ! operator > > And now we're back to where we started, so I don't really see why this > is even necessary. The semantics of the ! operator will be changed in a > future C version? I don't think it makes sense to discuss technical aspects of the proposed change, as I mentioned in the discussion yesterday I see no technical issues at this point, and my changes are described as nonfunctional. The _nontechnical_ problem I see (well, may be I'm just blowing it up) is a Linux kernel code style policy problem, to my knowledge the policy of using bitwise and arithmetic operations on bool type variables and constants is not defined, and since the policy is not yet defined I'm glad to take part in its discussion now. As an example of one related policy is change of 0/1 constants to false/true, when they are attended by bool type, see c2b49ae678b , 5c216cc3f37 , 7eef08554ca , bool{init,return}.cocci etc. One may say that the changes above has no value (however it might be related to ABI treatment of _Bool/bool), personally I agree with this accepted code policy. Another code policy question is to which degree arithmetic and bitwise operations on bool variables and constants are acceptable. In my personal opinion arithmetic and bitwise operations on bool variables are quite undesired, that's why I attempt to replace them in advance in some sound/soc/codecs/* functions before changing the type of input variable representing GPIO level from int to bool. I guess in your personal opinion this proposed change has no technical sense, I respect your opinion and do not insist on my answer to the policy. My little deal is to let you know that there is another opinion on the subject and send you the changes for review and ack/nak verdict. -- With best wishes, Vladimir
On Wed, Jun 3, 2015 at 12:13 PM, Vladimir Zapolskiy <vz@mleia.com> wrote: >> >> BTW, with a little C algebra: >> const unsigned int val = value ? 0x1 << WM5100_GP1_LVL_SHIFT : 0; >> const unsigned int val = (value ? 0x1 : 0) << WM5100_GP1_LVL_SHIFT; >> const unsigned int val = (!!value) << WM5100_GP1_LVL_SHIFT; // >> definition of ! operator >> >> And now we're back to where we started, so I don't really see why this >> is even necessary. The semantics of the ! operator will be changed in a >> future C version? > > I don't think it makes sense to discuss technical aspects of the > proposed change, as I mentioned in the discussion yesterday I see no > technical issues at this point, and my changes are described as > nonfunctional. > > The _nontechnical_ problem I see (well, may be I'm just blowing it up) > is a Linux kernel code style policy problem, to my knowledge the policy > of using bitwise and arithmetic operations on bool type variables and > constants is not defined, and since the policy is not yet defined I'm > glad to take part in its discussion now. I don't believe this is the case. From C99, section 6.5.3.3, paragraph 5: The result of the logical negation operator ! is 0 if the value of its operand compares unequal to 0, 1 if the value of its operand compares equal to 0. The result has type int. Thus, it's defined that !!(bool_var) and !!(int_var) will return an int that is either 0 or 1. This has always been part of the C standard and I have a very hard time believing that a future standard will change that. It would break so much code to do so and I don't the case for what it will improve. So I don't see the point in changing: !!value << SHIFT; into: unsigned int temp_val; if (value) temp_val = 1 << SHIFT; else temp_val = 0; The former is shorter and simpler and completely defined by the C standard.
Hello Trent, On 04.06.2015 00:51, Trent Piepho wrote: > On Wed, Jun 3, 2015 at 12:13 PM, Vladimir Zapolskiy <vz@mleia.com> wrote: >>> >>> BTW, with a little C algebra: >>> const unsigned int val = value ? 0x1 << WM5100_GP1_LVL_SHIFT : 0; >>> const unsigned int val = (value ? 0x1 : 0) << WM5100_GP1_LVL_SHIFT; >>> const unsigned int val = (!!value) << WM5100_GP1_LVL_SHIFT; // >>> definition of ! operator >>> >>> And now we're back to where we started, so I don't really see why this >>> is even necessary. The semantics of the ! operator will be changed in a >>> future C version? >> >> I don't think it makes sense to discuss technical aspects of the >> proposed change, as I mentioned in the discussion yesterday I see no >> technical issues at this point, and my changes are described as >> nonfunctional. >> >> The _nontechnical_ problem I see (well, may be I'm just blowing it up) >> is a Linux kernel code style policy problem, to my knowledge the policy >> of using bitwise and arithmetic operations on bool type variables and >> constants is not defined, and since the policy is not yet defined I'm >> glad to take part in its discussion now. > > I don't believe this is the case. please excuse me for possible misunderstanding, you don't believe that I'm interested in discussing a _nontechnical_ problem stated above? > From C99, section 6.5.3.3, paragraph 5: C99 is a strict superset over Linux coding style (e.g. remember // comments or "register" specifier), so let's forget about. -- With best wishes, Vladimir
diff --git a/sound/soc/codecs/wm5100.c b/sound/soc/codecs/wm5100.c index 4c10cd8..fae9d13 100644 --- a/sound/soc/codecs/wm5100.c +++ b/sound/soc/codecs/wm5100.c @@ -2244,26 +2244,27 @@ static inline struct wm5100_priv *gpio_to_wm5100(struct gpio_chip *chip) static void wm5100_gpio_set(struct gpio_chip *chip, unsigned offset, int value) { struct wm5100_priv *wm5100 = gpio_to_wm5100(chip); + unsigned int val = 0; + + if (value) + val = 0x1 << WM5100_GP1_LVL_SHIFT; regmap_update_bits(wm5100->regmap, WM5100_GPIO_CTRL_1 + offset, - WM5100_GP1_LVL, !!value << WM5100_GP1_LVL_SHIFT); + WM5100_GP1_LVL, val); } static int wm5100_gpio_direction_out(struct gpio_chip *chip, unsigned offset, int value) { struct wm5100_priv *wm5100 = gpio_to_wm5100(chip); - int val, ret; + unsigned int val = 0x1 << WM5100_GP1_FN_SHIFT; - val = (1 << WM5100_GP1_FN_SHIFT) | (!!value << WM5100_GP1_LVL_SHIFT); + if (value) + val |= 0x1 << WM5100_GP1_LVL_SHIFT; - ret = regmap_update_bits(wm5100->regmap, WM5100_GPIO_CTRL_1 + offset, - WM5100_GP1_FN_MASK | WM5100_GP1_DIR | - WM5100_GP1_LVL, val); - if (ret < 0) - return ret; - else - return 0; + return regmap_update_bits(wm5100->regmap, WM5100_GPIO_CTRL_1 + offset, + WM5100_GP1_FN_MASK | WM5100_GP1_DIR | + WM5100_GP1_LVL, val); } static int wm5100_gpio_get(struct gpio_chip *chip, unsigned offset)
The main intention of the change is to remove bitwise operations on GPIO level value as a preceding change before updating gpiolib callbacks to utilize bool type to represent GPIO level. No functional change. Signed-off-by: Vladimir Zapolskiy <vz@mleia.com> Cc: Charles Keepax <ckeepax@opensource.wolfsonmicro.com> Cc: Lars-Peter Clausen <lars@metafoo.de> Cc: Axel Lin <axel.lin@ingics.com> Cc: patches@opensource.wolfsonmicro.com --- sound/soc/codecs/wm5100.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)