On Fri, 18 Jul 2008, Anton Vorontsov wrote:
> +int px_gpio_xlate(struct of_gpio_chip *of_gc, struct device_node *np,
> + const void *gpio_spec)
> +{
> + if (gpio[1] & PX_GPIO_FLAG_ACTIVE_LOW)
> + px_gc->active_low |= pin2mask(*gpio);
You have a race here. What if px_gpio_xlate() is called at the same time
for different gpios? This is an easy one to fix, since you can use the
atomic bitops.
It doesn't look like you have any way to unset the active low flag. What if
I unload the leds-gpio driver (or another gpio user) and then try to use the
gpio with something else? The active low flag is stuck on! It doesn't show
in sysfs or debugfs either. That could be very confusing.
I also wonder if it's ok to have the xlate function do flag setting?
of_get_property() just gets the property, it doesn't allocate it. Same with
of_get_address() and of_get_pci_address(), they don't actually allocate or
map an address, they just get the value. of_get_gpio() doesn't allocate the
gpio, that gets done later with gpio_request(). It seems like what it's
supposed to do is just get the translated value of the gpio property.
Except, your pixis gpio xlate function sets the gpio's flags. What if one
wants to just look up a gpio number, but not allocate it? The flags will
still get set.
Most gpio users, including leds-gpio, can handle gpios being busy. If
leds-gpio can't get one of the gpios, it rolls back all the leds it did
create, doesn't drive the device and returns EBUSY. Except with
of_get_gpio() setting the flags, it will change the flags out from under
whatever had the gpio already allocated!
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
