[alsa-devel] [PATCH 5/7] ASoC: wm5100: remove bitwise operations involving GPIO level value

[Trent Piepho]

On Tue, Jun 2, 2015 at 1:23 PM, Vladimir Zapolskiy <vz at 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?