On Wed, Sep 11, 2013 at 3:58 AM, Alex Courbot <[email protected]> wrote:
> On 09/10/2013 09:30 PM, Linus Walleij wrote:
>>         /* flip the beeper output */
>> -       *IXP4XX_GPIO_GPOUTR ^= (1 << (unsigned int) dev_id);
>> +       gpio_set_value(pin, ~gpio_get_value(pin));
>
>
> Don't you want
>
>         gpio_set_value(pin, !gpio_get_value(pin));
>
> instead? From the deleted line I suppose you guess to invert the GPIO value,
> but using ~ will invert all the bits of the integer returned by
> gpio_get_value(), meaning that if it returns 1 you will end up calling
> gpio_set_value(-2). In practice your gpio is very unlikely to ever be set to
> 0.

You're right, fixed this.

>>         free_irq(IRQ_IXP4XX_TIMER2, (void *)dev->id);
>> +       gpio_free(dev->id);
>
>
> Just wondering, is there a reason for not using the devm_gpio functions
> here?

I didn't want to change the style of the whole driver, but I can make
a separate patch switching the whole driver to use managed
resources at some later point.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to