Messages in this thread | | | From | David Brownell <> | Subject | Re: [PATCH v3] Add bt8xxgpio driver | Date | Thu, 10 Jul 2008 13:02:12 -0700 |
| |
On Thursday 10 July 2008, Jiri Slaby wrote: > > --- /dev/null 1970-01-01 00:00:00.000000000 +0000 > > +++ linux-next/drivers/gpio/bt8xxgpio.c 2008-07-10 19:05:56.000000000 +0200 > > @@ -0,0 +1,348 @@ > [...] > > +static int bt8xxgpio_gpio_direction_input(struct gpio_chip *gpio, unsigned nr) > > +{ > > + struct bt8xxgpio *bg = container_of(gpio, struct bt8xxgpio, gpio); > > + unsigned long flags; > > + u32 outen, data; > > + > > + spin_lock_irqsave(&bg->lock, flags); > > Why all those irq variants? I can't see interrupts anywhere. May gpio call this > from irq?
Not that routine (see Documentation/gpio.txt where that's specified) ... but other using the same lock.
When setting GPIO direction, spin_lock_irq() style calls are appropriate (but this isn't wrong).
The gpio_{get,set}_value() accessors may be called from IRQ context, so they need to save/restor the IRQ flags.
> some flushing of posted values here?
See Documentation/gpio.txt:
+ Note that these operations include I/O barriers on platforms + which need to use them; drivers don't need to add them explicitly.
That's the key thing: drivers using I/O calls should not need to insert bus or platform specific calls to make sure the calls take effect.
Also:
> + return !!(val & (1 << nr));
GPIO values are zero/nonzero, not zero/one. So the "!!" can be removed.
- Dave
| |