Re: [PATCH 1/9] gpiolib: Fix buggy flag detection code

[Andrey Smirnov <andrew.smirnov@gmail.com>]

On Mon, Jul 24, 2017 at 8:36 AM, Sam Ravnborg <sam@ravnborg.org> wrote:
> Hi Andrey.
>
> On Mon, Jul 24, 2017 at 07:53:52AM -0700, Andrey Smirnov wrote:
>> Both GPIOF_ACTIVE_LOW and GPIOF_INIT_ACTIVE are multi-bit constants so
>> detecting their assertion using simple bit-wise and is incorrect and
>> would lead to false positives.
>>
>> Fixes: bbc499914 ("gpiolib: Add code to support "active low" GPIOs")
>>
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>
> When looking at the resulting code there is now two ways to check
> flags in gpiolib.
> One where assume the flags are one bit values, and one where
> we make sure to check the resuting value for > 1 bit values.
> When revisitng this code in years from now this will look
> inconsistent and only if you look up gpio.h you will see
> why this is done in different ways.
>
> I dunno if this should be consistent, but as I noted this when
> reading the code I commented on this.
>
> The use of "const bool" also baffeled me, but it makes
> good sense to say that this bool value are not changed
> after it is initialised.
> And we use "const bool" in a few other places already.
> This was just the first time I noticed.
>
> So despite the comments:
> Acked-by: Sam Ravnborg <sam@ravnborg.org>
>
>         Sam
>
>
> The two ways flags are tested:
>> +     const bool active_low = (flags & GPIOF_ACTIVE_LOW) == GPIOF_ACTIVE_LOW;
>
>>       if (flags & GPIOF_DIR_IN) {

I can convert all of the code to use "(flags & mask) == mask" idiom
and add a comment to the code explaining it. Hopefully this will make
it less confusing and more "future proof".

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox