Re: PIXIS gpio controller and gpio flags

[Anton Vorontsov <avorontsov@ru.mvista.com>]

On Sat, Jul 19, 2008 at 02:08:01PM -0700, Trent Piepho wrote:
> 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.

Thanks, I'll fix this. But I think spinlock would be less cryptic here,
since we have one, and the xlate is called quite rarely anyway.

> 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!

Why would you want to unset the flags? It is specified in the device
tree, and can't be changed. Specifying different flags for the same GPIO
would be an error (plus, Linux forbids shared gpios, so you simply can't
specify one gpio for several devices).

> It doesn't show
> in sysfs or debugfs either.  That could be very confusing.

It is in the /proc/device-tree. But I agree, it would be great if
/sys/debug/gpio would show active-low gpios. But this would need gpiolib
changes, with which David will not agree, I suppose. But I didn't try.

> I also wonder if it's ok to have the xlate function do flag setting?

Why not? Of course, we can implement of_gpio_is_active_low(device_node,
gpion), but this is just less convenient than handling active-low gpios
transparently (i.e. for pure OF drivers you don't have to do all
this if (active_low) in the drivers themselves).

> 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.

Yes, it translates gpio value, and its flags, it also "caches" the flag
so that further gpio_ calls could use it. But the flags for the
particular gpio is constant. So each xlate must end up with the same
active-low flag, otherwise you did something wrong.

> 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.

Nothing is allocated.

> 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!

You're still assuming that something was allocated. It wasn't. The flag
was set, and it should not change. It is irrelevant if request() failed
or not.

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2