Re: [PATCH] spi: pxa2xx_spi introduce chip select gpio to simplify the common cases

[David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>]

> > +     /* NOTE: setup() can be called multiple times, possibly with
> > +      * different chip_info, previously requested GPIO shall be
> > +      * released, stumped :(
> > +      */

No; chip_info should be regarded as immutable between the time the
initializing setup() call is invoked and, should it ever happen,
the cleanup() is invoked.  Anyone mucking around with that controller
data deserves the chaos they will have started.


> > +     if (gpio_is_valid(chip_info->gpio_cs)) {
> > +             err = gpio_request(chip_info->gpio_cs, "SPI_CS");

I'd actually pass dev_name(&spi->dev) not "SPI_CS", since I
happen to like seeing /sys/kernel/debug/gpio be informative.

Seeing half a dozen "SPI_CS" (uppercase, yeech!) labels is
not informative; seeing "spi1.0", "spi1.1", "spi2.5" etc is
more useful ... you can probably cross-check them with the
board schematics to detect errors, instead of just omissions.


> > +             if (err) {
> > +                     dev_err(&spi->dev, "failed to request CS GPIO%d\n",
> > +                                     chip_info->gpio_cs);
> 
> I do not agree that this is an error, though David may argue that it is.

It's an error all right.  If that signal is in use by some
other driver, this one should never touch it.


>  Feel free to reduce this to KERN_INFO and eliminate the following
> return.  It is not illegal to omit the chip select on a one-chip bus,
> when the chip does not use CS; there is no need to allocate and waste a
> pin if it is not connected to anything.

But such "not illegal" cases wouldn't hit this "if (err) ... " branch...


> > +     if (gpio_is_valid(chip->gpio_cs))
> > +             gpio_free(chip->gpio_cs);
>
> Again, this does not belong in the master driver, IMHO; the resource
> should have been allocated outside the master.

No; drivers do request_resource(), and gpio_request() is the same
kind of thing.  And they need to clean up the resources which they
requested on various paths -- even when those resources are GPIOs.

Of course, if the cs_control() route is used, then pxa2xx_spi isn't
going to be allocating any underlying GPIOs, so it won't have to
free them either.

- Dave


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/