| Message ID | 1359822572-26009-8-git-send-email-acourbot@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Feb 2, 2013 at 5:29 PM, Alexandre Courbot <acourbot@nvidia.com> wrote: > Make sure gpiolib works internally with descriptors and (chip, offset) > pairs instead of using the global integer namespace. This prepares the Its a numberspace not a namespace right? > ground for the removal of the global gpio_desc[] array and the > introduction of the descriptor-based GPIO API. > > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> > +/* > + * Internal gpiod_* API using descriptors instead of the integer namespace. > + * Most of this should eventually go public. > + */ > +static int gpiod_request(struct gpio_desc *desc, const char *label); > +static void gpiod_free(struct gpio_desc *desc); > +static int gpiod_direction_input(struct gpio_desc *desc); > +static int gpiod_direction_output(struct gpio_desc *desc, int value); > +static int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce); > +static int gpiod_get_value_cansleep(struct gpio_desc *desc); > +static void gpiod_set_value_cansleep(struct gpio_desc *desc, int value); > +static int gpiod_get_value(struct gpio_desc *desc); > +static void gpiod_set_value(struct gpio_desc *desc, int value); > +static int gpiod_cansleep(struct gpio_desc *desc); > +static int gpiod_to_irq(struct gpio_desc *desc); > +static int gpiod_export(struct gpio_desc *desc, bool direction_may_change); > +static int gpiod_export_link(struct device *dev, const char *name, > + struct gpio_desc *desc); > +static int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value); > +static void gpiod_unexport(struct gpio_desc *desc); Usually I don't like these upfrons forward-declarations, but in this case I *do* because they are here for a reason, to later become extern. So I like it! > +/* > + * Return the GPIO number of the passed descriptor relative to its chip > + */ > +static int gpio_chip_hwgpio(const struct gpio_desc *desc) > +{ > + return (desc - &gpio_desc[0]) - desc->chip->base; > +} That was a scary method. But it works as long as the descriptors are in a static array I guess... > +/** > + * Convert a GPIO number to its descriptor > + */ > +static struct gpio_desc *gpio_to_desc(unsigned gpio) > +{ > + if (WARN(!gpio_is_valid(gpio), "invalid GPIO %d\n", gpio)) > + return NULL; Don't we want to return ERR_PTR(-EINVAL); here? Then you can use IS_ERR() on the pointers later. This is the approach taken by the external API for clk and pins. > +/** > + * Convert a GPIO descriptor to the integer namespace. > + * This should disappear in the future but is needed since we still > + * use GPIO numbers for error messages and sysfs nodes > + */ > +static int desc_to_gpio(const struct gpio_desc *desc) > +{ > + return desc - &gpio_desc[0]; > +} Aha OK the scary stuff goes away. Good... > + > + You can never get enough whitespace ;-) > /* caller holds gpio_lock *OR* gpio is marked as requested */ That comment should be above the *next* function right? Strictly speaking it does not apply to gpiod_to_chip() if I read it right. > +static struct gpio_chip *gpiod_to_chip(struct gpio_desc *desc) > +{ > + return desc->chip; > +} > + > struct gpio_chip *gpio_to_chip(unsigned gpio) > { > - return gpio_desc[gpio].chip; > + return gpiod_to_chip(gpio_to_desc(gpio)); > } ...Then follows lots of nice stuff... (...) > static ssize_t gpio_direction_show(struct device *dev, > struct device_attribute *attr, char *buf) > { > - const struct gpio_desc *desc = dev_get_drvdata(dev); > - unsigned gpio = desc - gpio_desc; > + struct gpio_desc *desc = dev_get_drvdata(dev); Why not const anymore? (Applies to all these similar cases in the patch.) consting is nice. Especially in subsystem code. I know it is hard to get compilation right without warnings at times. But it pays off. In the pinctrl subsystem I spend endless hours reading this wiki page: http://en.wikipedia.org/wiki/Const-correctness I still don't quite get it. And it also wears off. But I like to use it. > @@ -594,29 +647,32 @@ static ssize_t export_store(struct class *class, > + desc = gpio_to_desc(gpio); I hope you have tested this hunk of the patch from userspace? > @@ -637,17 +694,18 @@ static ssize_t unexport_store(struct class *class, This too? (etc for the userspace interfaces) > @@ -1538,48 +1622,54 @@ int gpio_direction_input(unsigned gpio) > } > } > > - status = chip->direction_input(chip, gpio); > + status = chip->direction_input(chip, offset); > if (status == 0) > clear_bit(FLAG_IS_OUT, &desc->flags); > > - trace_gpio_direction(chip->base + gpio, 1, status); > + trace_gpio_direction(desc_to_gpio(desc), 1, status); > lose: > return status; > fail: > spin_unlock_irqrestore(&gpio_lock, flags); > - if (status) > + if (status) { > + int gpio = -1; > + if (desc) > + gpio = desc_to_gpio(desc); > pr_debug("%s: gpio-%d status %d\n", > __func__, gpio, status); > + } > return status; > } So using ERR_PTR/PTR_ERR helps you propagate errors in situations like these. Just: if (IS_ERR(desc)) status = PTR_ERR(desc); > -int gpio_direction_output(unsigned gpio, int value) > +static int gpiod_direction_output(struct gpio_desc *desc, int value) > { > unsigned long flags; > struct gpio_chip *chip; > - struct gpio_desc *desc = &gpio_desc[gpio]; > int status = -EINVAL; > + int offset; Use hwgpio? > /* Open drain pin should not be driven to 1 */ > if (value && test_bit(FLAG_OPEN_DRAIN, &desc->flags)) > - return gpio_direction_input(gpio); > + return gpiod_direction_input(desc); > > /* Open source pin should not be driven to 0 */ > if (!value && test_bit(FLAG_OPEN_SOURCE, &desc->flags)) > - return gpio_direction_input(gpio); > + return gpiod_direction_input(desc); > > spin_lock_irqsave(&gpio_lock, flags); > > - if (!gpio_is_valid(gpio)) > + if (!desc) if (IS_ERR(desc)) ? > @@ -1589,11 +1679,12 @@ int gpio_direction_output(unsigned gpio, int value) > > might_sleep_if(chip->can_sleep); > > + offset = gpio_chip_hwgpio(desc); Maybe rename to hwgpio? Or is that done later? We should stick with either hwgpio or offset everywhere or it will be a mess. (...) > fail: > spin_unlock_irqrestore(&gpio_lock, flags); > - if (status) > + if (status) { > + int gpio = -1; > + if (desc) > + gpio = desc_to_gpio(desc); > pr_debug("%s: gpio-%d status %d\n", > __func__, gpio, status); > + } > return status; Again IS_ERR()/ERR_PTR(). -1 is not nice. > /** > @@ -1622,24 +1722,22 @@ EXPORT_SYMBOL_GPL(gpio_direction_output); > * @gpio: the gpio to set debounce time > * @debounce: debounce time is microseconds > */ > -int gpio_set_debounce(unsigned gpio, unsigned debounce) > +static int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce) > { > unsigned long flags; > struct gpio_chip *chip; > - struct gpio_desc *desc = &gpio_desc[gpio]; > int status = -EINVAL; > + int offset; hwgpio? (then follows some repating cases) (then some nice code) > -int gpio_get_value_cansleep(unsigned gpio) > +static int gpiod_get_value_cansleep(struct gpio_desc *desc) > { > struct gpio_chip *chip; > int value; > + int offset; hwgpio? Basically the patch is very nice but I'd like you to iron out and proofread as per above so we have strong consistency, strong const:ing, and stringent naming (offset vs hwgpio come to mind). Yours, Linus Walleij
On Wed, Feb 6, 2013 at 2:53 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Sat, Feb 2, 2013 at 5:29 PM, Alexandre Courbot <acourbot@nvidia.com> wrote: > >> Make sure gpiolib works internally with descriptors and (chip, offset) >> pairs instead of using the global integer namespace. This prepares the > > Its a numberspace not a namespace right? Yes, absolutely. >> +/* >> + * Internal gpiod_* API using descriptors instead of the integer namespace. >> + * Most of this should eventually go public. >> + */ >> +static int gpiod_request(struct gpio_desc *desc, const char *label); >> +static void gpiod_free(struct gpio_desc *desc); >> +static int gpiod_direction_input(struct gpio_desc *desc); >> +static int gpiod_direction_output(struct gpio_desc *desc, int value); >> +static int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce); >> +static int gpiod_get_value_cansleep(struct gpio_desc *desc); >> +static void gpiod_set_value_cansleep(struct gpio_desc *desc, int value); >> +static int gpiod_get_value(struct gpio_desc *desc); >> +static void gpiod_set_value(struct gpio_desc *desc, int value); >> +static int gpiod_cansleep(struct gpio_desc *desc); >> +static int gpiod_to_irq(struct gpio_desc *desc); >> +static int gpiod_export(struct gpio_desc *desc, bool direction_may_change); >> +static int gpiod_export_link(struct device *dev, const char *name, >> + struct gpio_desc *desc); >> +static int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value); >> +static void gpiod_unexport(struct gpio_desc *desc); > > Usually I don't like these upfrons forward-declarations, but in this > case I *do* because they are here for a reason, to later become > extern. So I like it! Yeah, you know how the movie is going to end already. ;) Maybe it would make sense to append the gpiod patches to this series and not end up with these declarations? >> +/* >> + * Return the GPIO number of the passed descriptor relative to its chip >> + */ >> +static int gpio_chip_hwgpio(const struct gpio_desc *desc) >> +{ >> + return (desc - &gpio_desc[0]) - desc->chip->base; >> +} > > That was a scary method. But it works as long as the > descriptors are in a static array I guess... Yes, and it's becomes less scary when the static array goes away. >> +/** >> + * Convert a GPIO number to its descriptor >> + */ >> +static struct gpio_desc *gpio_to_desc(unsigned gpio) >> +{ >> + if (WARN(!gpio_is_valid(gpio), "invalid GPIO %d\n", gpio)) >> + return NULL; > > Don't we want to return ERR_PTR(-EINVAL); here? > > Then you can use IS_ERR() on the pointers later. > > This is the approach taken by the external API for clk > and pins. Yes, that completely makes sense. >> /* caller holds gpio_lock *OR* gpio is marked as requested */ > > That comment should be above the *next* function right? > > Strictly speaking it does not apply to gpiod_to_chip() if > I read it right. That's right. On a related topic there are actually a whole set of comments that are not in the right place (because the function that follows them has been switched to the gpiod prefix). I left them here because once the gpiod interface becomes public they will be updated to apply to them, and moving comments back and forth in such a short time would only make the patches confusing. >> static ssize_t gpio_direction_show(struct device *dev, >> struct device_attribute *attr, char *buf) >> { >> - const struct gpio_desc *desc = dev_get_drvdata(dev); >> - unsigned gpio = desc - gpio_desc; >> + struct gpio_desc *desc = dev_get_drvdata(dev); > > Why not const anymore? > > (Applies to all these similar cases in the patch.) > > consting is nice. Especially in subsystem code. > I know it is hard to get compilation right without warnings > at times. But it pays off. > > In the pinctrl subsystem I spend endless hours reading this > wiki page: > > http://en.wikipedia.org/wiki/Const-correctness > > I still don't quite get it. And it also wears off. But I like to use it. Oh I do share your love for const-correctness (look at the parameters for gpio_chip_hwgpio() and desc_to_gpio()). Only here I could not preserve it AFAICT. The previous version of the function was only using desc locally and relied on GPIO numbers to do the dirty job. Here however, we pass desc to gpiod_get_direction(). So you will tell me, that as it only returns the direction its parameter could also be const and that's true - excepted when it tries to clear some bits in desc->flags, there we definitely cannot claim it is const anymore. Maybe we could cast the pointer given to clear_bit/set_bit, but I'm not sure that's more elegant. Ideally gpiod_get_direction() should not modify its parameter at all. >> @@ -594,29 +647,32 @@ static ssize_t export_store(struct class *class, >> + desc = gpio_to_desc(gpio); > > I hope you have tested this hunk of the patch from userspace? > >> @@ -637,17 +694,18 @@ static ssize_t unexport_store(struct class *class, > > This too? > > (etc for the userspace interfaces) Yes. Side-question: should I explicitly add my "Tested-by" in this cases? I thought that the author's Signed-off implied that the patch has been properly tested, but your question tend to suggest it's not *that* obvious... ;) >> + if (status) { >> + int gpio = -1; >> + if (desc) >> + gpio = desc_to_gpio(desc); >> pr_debug("%s: gpio-%d status %d\n", >> __func__, gpio, status); >> + } >> return status; >> } > > So using ERR_PTR/PTR_ERR helps you propagate > errors in situations like these. > > Just: > > if (IS_ERR(desc)) > status = PTR_ERR(desc); Yes, that would make these blocks look much better. >> /* Open drain pin should not be driven to 1 */ >> if (value && test_bit(FLAG_OPEN_DRAIN, &desc->flags)) >> - return gpio_direction_input(gpio); >> + return gpiod_direction_input(desc); >> >> /* Open source pin should not be driven to 0 */ >> if (!value && test_bit(FLAG_OPEN_SOURCE, &desc->flags)) >> - return gpio_direction_input(gpio); >> + return gpiod_direction_input(desc); >> >> spin_lock_irqsave(&gpio_lock, flags); >> >> - if (!gpio_is_valid(gpio)) >> + if (!desc) > > if (IS_ERR(desc)) ? It's worse actually, we potentially access desc right before checking it it's not NULL... Makes no sense at all. Thanks for pointing this. >> + offset = gpio_chip_hwgpio(desc); > > Maybe rename to hwgpio? Or is that done later? > > We should stick with either hwgpio or offset everywhere or > it will be a mess. Will replace all these "offset" by "hwgpio" for consistency. > Basically the patch is very nice but I'd like you to iron out and proofread > as per above so we have strong consistency, strong const:ing, > and stringent naming (offset vs hwgpio come to mind). Yes, there are some inconsistencies (and "inconstistencies") that remains, e.g. automatically converting calls to gpio_is_valid() into a check that the descriptor is not NULL was not the best idea. I will fix the patch according to your comments and then check the whole file to make sure I have not missed anything. Might do some random unrelated catches in the process. Thanks for the review! Alex.
On Thu, 7 Feb 2013 15:57:32 +0900, Alexandre Courbot <acourbot@nvidia.com> wrote: > On Wed, Feb 6, 2013 at 2:53 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > > On Sat, Feb 2, 2013 at 5:29 PM, Alexandre Courbot <acourbot@nvidia.com> wrote: > >> +/** > >> + * Convert a GPIO number to its descriptor > >> + */ > >> +static struct gpio_desc *gpio_to_desc(unsigned gpio) > >> +{ > >> + if (WARN(!gpio_is_valid(gpio), "invalid GPIO %d\n", gpio)) > >> + return NULL; > > > > Don't we want to return ERR_PTR(-EINVAL); here? > > > > Then you can use IS_ERR() on the pointers later. > > > > This is the approach taken by the external API for clk > > and pins. > > Yes, that completely makes sense. > No, it does not. The ERR_PTR()/IS_ERR() is a horrible pattern for code readability because it breaks the expectations that programmers have for what is and is not a bad pointer. There are decades of history where the test for a bad pointer is 'if (!ptr)'. Not only does ERR_PTR make make that test not work, but the compiler won't tell you when you get it wrong. There are places where ERR_PTR makes sense. Particularly when communicating with userspace where error codes have very specific meanings, but I don't want it in the GPIO subsystem. g.
On Sun, 3 Feb 2013 01:29:29 +0900, Alexandre Courbot <acourbot@nvidia.com> wrote: > Make sure gpiolib works internally with descriptors and (chip, offset) > pairs instead of using the global integer namespace. This prepares the > ground for the removal of the global gpio_desc[] array and the > introduction of the descriptor-based GPIO API. > > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> > --- > drivers/gpio/gpiolib.c | 493 +++++++++++++++++++++++++++++++------------------ > 1 file changed, 317 insertions(+), 176 deletions(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index af4c350..82c40bd 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -78,6 +78,28 @@ static LIST_HEAD(gpio_chips); > static DEFINE_IDR(dirent_idr); > #endif > > +/* > + * Internal gpiod_* API using descriptors instead of the integer namespace. > + * Most of this should eventually go public. > + */ > +static int gpiod_request(struct gpio_desc *desc, const char *label); > +static void gpiod_free(struct gpio_desc *desc); > +static int gpiod_direction_input(struct gpio_desc *desc); > +static int gpiod_direction_output(struct gpio_desc *desc, int value); > +static int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce); > +static int gpiod_get_value_cansleep(struct gpio_desc *desc); > +static void gpiod_set_value_cansleep(struct gpio_desc *desc, int value); > +static int gpiod_get_value(struct gpio_desc *desc); > +static void gpiod_set_value(struct gpio_desc *desc, int value); > +static int gpiod_cansleep(struct gpio_desc *desc); > +static int gpiod_to_irq(struct gpio_desc *desc); > +static int gpiod_export(struct gpio_desc *desc, bool direction_may_change); > +static int gpiod_export_link(struct device *dev, const char *name, > + struct gpio_desc *desc); > +static int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value); > +static void gpiod_unexport(struct gpio_desc *desc); I've been thinking about the adding of a new API to supplant the old, and I'm wondering if we're going about this the wrong way around; at least for the public api. We've moved to a model where device drivers are really supposed to tread GPIO numbers as opaque handles. Most device drivers are well behaved on this point. (Others of course are not so well behaved and either hard code or interpret what a number means, but those users already need to be fixed before they will work with the device tree) Rather than creating a new API with a long-term-plan to move all old users to the new API - touching a lot of code all over the kernel in the process - what if we repurposed the existing API in a backwards compatible way. I think the maximum GPIO number I've seen in use is 65535, and that was based on dynamic allocation. What if we did the following: typedef unsigned long gpio_handle_t; struct gpio_desc *gpio_handle_to_desc(gpio_handle_t gpio) { if (gpio <= NR_GPIOS) return &gpio_desc[gpio]; return (struct gpio_desc *)gpio; } int gpio_get_direction(gpio_handle_t gpio) { struct gpio_desc *desc = gpio_handle_to_desc(gpio); if (!desc) return -EEXIST; return gpiod_get_direction(desc); } And do the same for all the other hooks. That will make the gpio changes much lower impact across the kernel, allow legacy gpio users to keep doing what they are doing, and encourage driver authors to keep out of the gpio_desc internals. Thoughts? I'm probably going to apply this patch anyway because it only affects the gpiolib internals, I need to take one more look over it before I make a decision. I want to get as much of this as possible queued up for v3.9 to make future work easier. g.
On Sun, 3 Feb 2013 01:29:29 +0900, Alexandre Courbot <acourbot@nvidia.com> wrote: > Make sure gpiolib works internally with descriptors and (chip, offset) > pairs instead of using the global integer namespace. This prepares the > ground for the removal of the global gpio_desc[] array and the > introduction of the descriptor-based GPIO API. > > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> Despite Linus' comments, I've gone ahead and applied this. I think it is a worthwhile change on its own and it feeds into the future work than needs to be done. Any of the stuff he brought up can be addressed in follow-on patches. g.
On Sat, Feb 9, 2013 at 10:11 PM, Grant Likely <grant.likely@secretlab.ca> wrote: > I've been thinking about the adding of a new API to supplant the old, > and I'm wondering if we're going about this the wrong way around; at > least for the public api. We've moved to a model where device drivers > are really supposed to tread GPIO numbers as opaque handles. Most device > drivers are well behaved on this point. (Others of course are not so > well behaved and either hard code or interpret what a number means, but > those users already need to be fixed before they will work with the > device tree) > > Rather than creating a new API with a long-term-plan to move all old > users to the new API - touching a lot of code all over the kernel in the > process - what if we repurposed the existing API in a backwards > compatible way. > > I think the maximum GPIO number I've seen in use is 65535, and that was > based on dynamic allocation. What if we did the following: > > typedef unsigned long gpio_handle_t; > > struct gpio_desc *gpio_handle_to_desc(gpio_handle_t gpio) > { > if (gpio <= NR_GPIOS) > return &gpio_desc[gpio]; > return (struct gpio_desc *)gpio; > } > > int gpio_get_direction(gpio_handle_t gpio) > { > struct gpio_desc *desc = gpio_handle_to_desc(gpio); > if (!desc) > return -EEXIST; > return gpiod_get_direction(desc); > } > > And do the same for all the other hooks. That will make the gpio changes > much lower impact across the kernel, allow legacy gpio users to keep > doing what they are doing, and encourage driver authors to keep out of > the gpio_desc internals. So if I understand correctly, this means reserving the range [0..65535] for integer GPIOs, assuming that anything bigger is a descriptor (that have been acquired properly using some gpio_get() function), and keep using the same interface for both cases? I'm not sure what to think yet, in a way I find the idea elegant and less traumatic to current GPIO users. On the other hand, I wonder if we will not run into some walls. Here is one I can think of right now: what should gpio_is_valid() return? If the parameter is in the integer range, no problem, it's valid. If it's bigger, we could check the validity of the pointer if we use the gpio_desc[] static array that currently exist, but if we eventually manage to get rid of it things would become harder. Also, let's say we do gpio_is_valid(0). Is it the GPIO number 0, which is valid, or a NULL pointer which is invalid? It might also bring some confusion that the nature of GPIOs depends on the way they are acquired (integers for gpio_request(), pointers for gpio_get()). Some users might use gpio_get(), expect to get a number, and be puzzled. I am not particularly fond of the idea of introducing a new API that is nearly identical to one that already exists, but it has the merit of keeping things clear. I also think we should consider this state as a transition towards the elimination of the GPIO integer API. I might be misguided to wish for that, but right now I can only see two things that would prevent us from achieving this: 1) The fast-path that some platforms implemented in their definition of gpio_get_value() and gpio_set_value() where they directly invoke their controller's function inline for GPIOs which values are known at compile time (can't do that with pointers AFAICT) 2) The sysfs interface. But we could still keep the integer space for that sole purpose and not use it for our in-kernel APIs. Actually only 1) is a real blocker. Also in the case we cannot/do not want to get rid of it, keeping the legacy gpio API as a wrapper around gpiod as I proposed in an earlier RFC should probably not be too painful (only a single .h file to maintain). Alex.
On Sat, Feb 9, 2013 at 10:24 PM, Grant Likely <grant.likely@secretlab.ca> wrote: > On Sun, 3 Feb 2013 01:29:29 +0900, Alexandre Courbot <acourbot@nvidia.com> wrote: >> Make sure gpiolib works internally with descriptors and (chip, offset) >> pairs instead of using the global integer namespace. This prepares the >> ground for the removal of the global gpio_desc[] array and the >> introduction of the descriptor-based GPIO API. >> >> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> > > Despite Linus' comments, I've gone ahead and applied this. I think it is > a worthwhile change on its own and it feeds into the future work than > needs to be done. Any of the stuff he brought up can be addressed in > follow-on patches. Ah, I actually wanted to ask you for a couple of days to submit a new version of this. On top of Linus' comments, there are a few more sanity checks I'd like to add. If you don't want to rebase on the new version, I can also send a new patch on top of this one, whichever way you prefer - but I think rebasing would be cleaner, if you don't mind. Alex.
On Sat, Feb 9, 2013 at 10:17 AM, Grant Likely <grant.likely@secretlab.ca> wrote: > The ERR_PTR()/IS_ERR() is a horrible pattern for code > readability because it breaks the expectations that programmers have for > what is and is not a bad pointer. There are decades of history where the > test for a bad pointer is 'if (!ptr)'. Not only does ERR_PTR make make > that test not work, but the compiler won't tell you when you get it > wrong. > > There are places where ERR_PTR makes sense. Particularly when > communicating with userspace where error codes have very specific > meanings, but I don't want it in the GPIO subsystem. OK I disagree but you get to decide. However if you take this all the way to the descriptor API it will make the consumer (driver) API for GPIO descriptors deviate from what is today used for clocks, regulators and pins. With all the resulting confusion for users. I've seen worse subsystem deviations though. Yours, Linus Walleij
On Mon, Feb 11, 2013 at 03:09:21PM +0100, Linus Walleij wrote: > On Sat, Feb 9, 2013 at 10:17 AM, Grant Likely <grant.likely@secretlab.ca> wrote: > > > The ERR_PTR()/IS_ERR() is a horrible pattern for code > > readability because it breaks the expectations that programmers have for > > what is and is not a bad pointer. There are decades of history where the > > test for a bad pointer is 'if (!ptr)'. Not only does ERR_PTR make make > > that test not work, but the compiler won't tell you when you get it > > wrong. > > > > There are places where ERR_PTR makes sense. Particularly when > > communicating with userspace where error codes have very specific > > meanings, but I don't want it in the GPIO subsystem. > > OK I disagree but you get to decide. > > However if you take this all the way to the descriptor API > it will make the consumer (driver) API for GPIO descriptors deviate > from what is today used for clocks, regulators and pins. > > With all the resulting confusion for users. > I've seen worse subsystem deviations though. > I've always considered it to be more of a complexity issue. If an interface can fail for half a dozen different reasons, it's useful to be able to encapsulate this and pass it down the line to the consumer (particularly in cases where no useful debug output is provided, and the first thing someone is going to do is go and instrument the registration path with printks to figure out where things went wrong). In the case where the work to do is relatively straightforward and there's not much complexity, NULL is clearly the way to go. There are already quite a few cases today converting NULLs over to arbitrary ERR_PTR values or IS_ERR cases wrapping back to NULL, so it's clear that both have to be supported interacting with one another regardless. Then there's that whole IS_ERR_OR_NULL case which everyone seems to get wrong, but that's another issue entirely.
On 02/11/2013 07:09 AM, Linus Walleij wrote: > On Sat, Feb 9, 2013 at 10:17 AM, Grant Likely <grant.likely@secretlab.ca> wrote: > >> The ERR_PTR()/IS_ERR() is a horrible pattern for code >> readability because it breaks the expectations that programmers have for >> what is and is not a bad pointer. There are decades of history where the >> test for a bad pointer is 'if (!ptr)'. Not only does ERR_PTR make make >> that test not work, but the compiler won't tell you when you get it >> wrong. >> >> There are places where ERR_PTR makes sense. Particularly when >> communicating with userspace where error codes have very specific >> meanings, but I don't want it in the GPIO subsystem. > > OK I disagree but you get to decide. > > However if you take this all the way to the descriptor API > it will make the consumer (driver) API for GPIO descriptors deviate > from what is today used for clocks, regulators and pins. > > With all the resulting confusion for users. > I've seen worse subsystem deviations though. Sorry I haven't looked at the specific APIs this discussion refers to, but clients of the GPIO descriptor API are going to need to distinguish "fail" from "deferred probe", so at least some initial get-like API will need to pass back some error detail...
On Mon, Feb 11, 2013 at 6:39 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 02/11/2013 07:09 AM, Linus Walleij wrote: >> However if you take this all the way to the descriptor API >> it will make the consumer (driver) API for GPIO descriptors deviate >> from what is today used for clocks, regulators and pins. >> >> With all the resulting confusion for users. >> I've seen worse subsystem deviations though. > > Sorry I haven't looked at the specific APIs this discussion refers to, > but clients of the GPIO descriptor API are going to need to distinguish > "fail" from "deferred probe", so at least some initial get-like API will > need to pass back some error detail... Right, so in some other patch I stated that this would lead to a GPIO descriptor fetch interface such as this: int gpiod_get(struct gpiod_desc **gpiod, struct device *dev, const char *name); Rather than the more established: struct gpio_desc *gpiod_get(struct device *dev, const char *name); And I'm worried about the lack of consistency. While I do get the point... I chatted with Grant about it and I want to talk to some toolchain people about this to see if pointers containing potential error codes can somehow be "flagged" by the compiler so we can enforce error checking on them. (It may sound a bit utopic...) Yours, Linus Walleij
On Tue, Feb 12, 2013 at 01:29:10PM +0100, Linus Walleij wrote: > On Mon, Feb 11, 2013 at 6:39 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > > On 02/11/2013 07:09 AM, Linus Walleij wrote: > > >> However if you take this all the way to the descriptor API > >> it will make the consumer (driver) API for GPIO descriptors deviate > >> from what is today used for clocks, regulators and pins. > >> > >> With all the resulting confusion for users. > >> I've seen worse subsystem deviations though. > > > > Sorry I haven't looked at the specific APIs this discussion refers to, > > but clients of the GPIO descriptor API are going to need to distinguish > > "fail" from "deferred probe", so at least some initial get-like API will > > need to pass back some error detail... > > Right, so in some other patch I stated that this would lead > to a GPIO descriptor fetch interface such as this: > > int gpiod_get(struct gpiod_desc **gpiod, struct device *dev, const char *name); > > Rather than the more established: > > struct gpio_desc *gpiod_get(struct device *dev, const char *name); > > And I'm worried about the lack of consistency. > > While I do get the point... I chatted with Grant about it and > I want to talk to some toolchain people about this to see if > pointers containing potential error codes can somehow be > "flagged" by the compiler so we can enforce error checking on > them. (It may sound a bit utopic...) > At the very least you can __must_check annotate, although that's probably still a bit coarser grained than what you're after.
On Tuesday 12 February 2013, Paul Mundt wrote: > > While I do get the point... I chatted with Grant about it and > > I want to talk to some toolchain people about this to see if > > pointers containing potential error codes can somehow be > > "flagged" by the compiler so we can enforce error checking on > > them. (It may sound a bit utopic...) > > > At the very least you can __must_check annotate, although that's probably > still a bit coarser grained than what you're after. There is an idea: we could have variants of __must_check for different classes of returns: NULL error, ERR_PTR, negative integer on error, ... Arnd
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index af4c350..82c40bd 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -78,6 +78,28 @@ static LIST_HEAD(gpio_chips); static DEFINE_IDR(dirent_idr); #endif +/* + * Internal gpiod_* API using descriptors instead of the integer namespace. + * Most of this should eventually go public. + */ +static int gpiod_request(struct gpio_desc *desc, const char *label); +static void gpiod_free(struct gpio_desc *desc); +static int gpiod_direction_input(struct gpio_desc *desc); +static int gpiod_direction_output(struct gpio_desc *desc, int value); +static int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce); +static int gpiod_get_value_cansleep(struct gpio_desc *desc); +static void gpiod_set_value_cansleep(struct gpio_desc *desc, int value); +static int gpiod_get_value(struct gpio_desc *desc); +static void gpiod_set_value(struct gpio_desc *desc, int value); +static int gpiod_cansleep(struct gpio_desc *desc); +static int gpiod_to_irq(struct gpio_desc *desc); +static int gpiod_export(struct gpio_desc *desc, bool direction_may_change); +static int gpiod_export_link(struct device *dev, const char *name, + struct gpio_desc *desc); +static int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value); +static void gpiod_unexport(struct gpio_desc *desc); + + static inline void desc_set_label(struct gpio_desc *d, const char *label) { #ifdef CONFIG_DEBUG_FS @@ -85,6 +107,36 @@ static inline void desc_set_label(struct gpio_desc *d, const char *label) #endif } +/* + * Return the GPIO number of the passed descriptor relative to its chip + */ +static int gpio_chip_hwgpio(const struct gpio_desc *desc) +{ + return (desc - &gpio_desc[0]) - desc->chip->base; +} + +/** + * Convert a GPIO number to its descriptor + */ +static struct gpio_desc *gpio_to_desc(unsigned gpio) +{ + if (WARN(!gpio_is_valid(gpio), "invalid GPIO %d\n", gpio)) + return NULL; + else + return &gpio_desc[gpio]; +} + +/** + * Convert a GPIO descriptor to the integer namespace. + * This should disappear in the future but is needed since we still + * use GPIO numbers for error messages and sysfs nodes + */ +static int desc_to_gpio(const struct gpio_desc *desc) +{ + return desc - &gpio_desc[0]; +} + + /* Warn when drivers omit gpio_request() calls -- legal but ill-advised * when setting direction, and otherwise illegal. Until board setup code * and drivers use explicit requests everywhere (which won't happen when @@ -96,10 +148,10 @@ static inline void desc_set_label(struct gpio_desc *d, const char *label) * only "legal" in the sense that (old) code using it won't break yet, * but instead only triggers a WARN() stack dump. */ -static int gpio_ensure_requested(struct gpio_desc *desc, unsigned offset) +static int gpio_ensure_requested(struct gpio_desc *desc) { const struct gpio_chip *chip = desc->chip; - const int gpio = chip->base + offset; + const int gpio = desc_to_gpio(desc); if (WARN(test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0, "autorequest GPIO-%d\n", gpio)) { @@ -118,9 +170,14 @@ static int gpio_ensure_requested(struct gpio_desc *desc, unsigned offset) } /* caller holds gpio_lock *OR* gpio is marked as requested */ +static struct gpio_chip *gpiod_to_chip(struct gpio_desc *desc) +{ + return desc->chip; +} + struct gpio_chip *gpio_to_chip(unsigned gpio) { - return gpio_desc[gpio].chip; + return gpiod_to_chip(gpio_to_desc(gpio)); } /* dynamic allocation of GPIOs, e.g. on a hotplugged device */ @@ -148,19 +205,19 @@ static int gpiochip_find_base(int ngpio) } /* caller ensures gpio is valid and requested, chip->get_direction may sleep */ -static int gpio_get_direction(unsigned gpio) +static int gpiod_get_direction(struct gpio_desc *desc) { struct gpio_chip *chip; - struct gpio_desc *desc = &gpio_desc[gpio]; + unsigned offset; int status = -EINVAL; - chip = gpio_to_chip(gpio); - gpio -= chip->base; + chip = gpiod_to_chip(desc); + offset = gpio_chip_hwgpio(desc); if (!chip->get_direction) return status; - status = chip->get_direction(chip, gpio); + status = chip->get_direction(chip, offset); if (status > 0) { /* GPIOF_DIR_IN, or other positive */ status = 1; @@ -204,8 +261,7 @@ static DEFINE_MUTEX(sysfs_lock); static ssize_t gpio_direction_show(struct device *dev, struct device_attribute *attr, char *buf) { - const struct gpio_desc *desc = dev_get_drvdata(dev); - unsigned gpio = desc - gpio_desc; + struct gpio_desc *desc = dev_get_drvdata(dev); ssize_t status; mutex_lock(&sysfs_lock); @@ -213,7 +269,7 @@ static ssize_t gpio_direction_show(struct device *dev, if (!test_bit(FLAG_EXPORT, &desc->flags)) status = -EIO; else - gpio_get_direction(gpio); + gpiod_get_direction(desc); status = sprintf(buf, "%s\n", test_bit(FLAG_IS_OUT, &desc->flags) ? "out" : "in"); @@ -225,8 +281,7 @@ static ssize_t gpio_direction_show(struct device *dev, static ssize_t gpio_direction_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t size) { - const struct gpio_desc *desc = dev_get_drvdata(dev); - unsigned gpio = desc - gpio_desc; + struct gpio_desc *desc = dev_get_drvdata(dev); ssize_t status; mutex_lock(&sysfs_lock); @@ -234,11 +289,11 @@ static ssize_t gpio_direction_store(struct device *dev, if (!test_bit(FLAG_EXPORT, &desc->flags)) status = -EIO; else if (sysfs_streq(buf, "high")) - status = gpio_direction_output(gpio, 1); + status = gpiod_direction_output(desc, 1); else if (sysfs_streq(buf, "out") || sysfs_streq(buf, "low")) - status = gpio_direction_output(gpio, 0); + status = gpiod_direction_output(desc, 0); else if (sysfs_streq(buf, "in")) - status = gpio_direction_input(gpio); + status = gpiod_direction_input(desc); else status = -EINVAL; @@ -252,8 +307,7 @@ static /* const */ DEVICE_ATTR(direction, 0644, static ssize_t gpio_value_show(struct device *dev, struct device_attribute *attr, char *buf) { - const struct gpio_desc *desc = dev_get_drvdata(dev); - unsigned gpio = desc - gpio_desc; + struct gpio_desc *desc = dev_get_drvdata(dev); ssize_t status; mutex_lock(&sysfs_lock); @@ -263,7 +317,7 @@ static ssize_t gpio_value_show(struct device *dev, } else { int value; - value = !!gpio_get_value_cansleep(gpio); + value = !!gpiod_get_value_cansleep(desc); if (test_bit(FLAG_ACTIVE_LOW, &desc->flags)) value = !value; @@ -277,8 +331,7 @@ static ssize_t gpio_value_show(struct device *dev, static ssize_t gpio_value_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t size) { - const struct gpio_desc *desc = dev_get_drvdata(dev); - unsigned gpio = desc - gpio_desc; + struct gpio_desc *desc = dev_get_drvdata(dev); ssize_t status; mutex_lock(&sysfs_lock); @@ -294,7 +347,7 @@ static ssize_t gpio_value_store(struct device *dev, if (status == 0) { if (test_bit(FLAG_ACTIVE_LOW, &desc->flags)) value = !value; - gpio_set_value_cansleep(gpio, value != 0); + gpiod_set_value_cansleep(desc, value != 0); status = size; } } @@ -324,7 +377,7 @@ static int gpio_setup_irq(struct gpio_desc *desc, struct device *dev, if ((desc->flags & GPIO_TRIGGER_MASK) == gpio_flags) return 0; - irq = gpio_to_irq(desc - gpio_desc); + irq = gpiod_to_irq(desc); if (irq < 0) return -EIO; @@ -594,29 +647,32 @@ static ssize_t export_store(struct class *class, struct class_attribute *attr, const char *buf, size_t len) { - long gpio; - int status; + long gpio; + struct gpio_desc *desc; + int status; status = strict_strtol(buf, 0, &gpio); if (status < 0) goto done; + desc = gpio_to_desc(gpio); + /* No extra locking here; FLAG_SYSFS just signifies that the * request and export were done by on behalf of userspace, so * they may be undone on its behalf too. */ - status = gpio_request(gpio, "sysfs"); + status = gpiod_request(desc, "sysfs"); if (status < 0) { if (status == -EPROBE_DEFER) status = -ENODEV; goto done; } - status = gpio_export(gpio, true); + status = gpiod_export(desc, true); if (status < 0) - gpio_free(gpio); + gpiod_free(desc); else - set_bit(FLAG_SYSFS, &gpio_desc[gpio].flags); + set_bit(FLAG_SYSFS, &desc->flags); done: if (status) @@ -628,8 +684,9 @@ static ssize_t unexport_store(struct class *class, struct class_attribute *attr, const char *buf, size_t len) { - long gpio; - int status; + long gpio; + struct gpio_desc *desc; + int status; status = strict_strtol(buf, 0, &gpio); if (status < 0) @@ -637,17 +694,18 @@ static ssize_t unexport_store(struct class *class, status = -EINVAL; + desc = gpio_to_desc(gpio); /* reject bogus commands (gpio_unexport ignores them) */ - if (!gpio_is_valid(gpio)) + if (!desc) goto done; /* No extra locking here; FLAG_SYSFS just signifies that the * request and export were done by on behalf of userspace, so * they may be undone on its behalf too. */ - if (test_and_clear_bit(FLAG_SYSFS, &gpio_desc[gpio].flags)) { + if (test_and_clear_bit(FLAG_SYSFS, &desc->flags)) { status = 0; - gpio_free(gpio); + gpiod_free(desc); } done: if (status) @@ -684,13 +742,13 @@ static struct class gpio_class = { * * Returns zero on success, else an error. */ -int gpio_export(unsigned gpio, bool direction_may_change) +static int gpiod_export(struct gpio_desc *desc, bool direction_may_change) { unsigned long flags; - struct gpio_desc *desc; int status; const char *ioname = NULL; struct device *dev; + int offset; /* can't export until sysfs is available ... */ if (!gpio_class.p) { @@ -698,20 +756,19 @@ int gpio_export(unsigned gpio, bool direction_may_change) return -ENOENT; } - if (!gpio_is_valid(gpio)) { - pr_debug("%s: gpio %d is not valid\n", __func__, gpio); + if (!desc) { + pr_debug("%s: invalid gpio descriptor\n", __func__); return -EINVAL; } mutex_lock(&sysfs_lock); spin_lock_irqsave(&gpio_lock, flags); - desc = &gpio_desc[gpio]; if (!test_bit(FLAG_REQUESTED, &desc->flags) || test_bit(FLAG_EXPORT, &desc->flags)) { spin_unlock_irqrestore(&gpio_lock, flags); pr_debug("%s: gpio %d unavailable (requested=%d, exported=%d)\n", - __func__, gpio, + __func__, desc_to_gpio(desc), test_bit(FLAG_REQUESTED, &desc->flags), test_bit(FLAG_EXPORT, &desc->flags)); status = -EPERM; @@ -722,11 +779,13 @@ int gpio_export(unsigned gpio, bool direction_may_change) direction_may_change = false; spin_unlock_irqrestore(&gpio_lock, flags); - if (desc->chip->names && desc->chip->names[gpio - desc->chip->base]) - ioname = desc->chip->names[gpio - desc->chip->base]; + offset = gpio_chip_hwgpio(desc); + if (desc->chip->names && desc->chip->names[offset]) + ioname = desc->chip->names[offset]; dev = device_create(&gpio_class, desc->chip->dev, MKDEV(0, 0), - desc, ioname ? ioname : "gpio%u", gpio); + desc, ioname ? ioname : "gpio%u", + desc_to_gpio(desc)); if (IS_ERR(dev)) { status = PTR_ERR(dev); goto fail_unlock; @@ -742,7 +801,7 @@ int gpio_export(unsigned gpio, bool direction_may_change) goto fail_unregister_device; } - if (gpio_to_irq(gpio) >= 0 && (direction_may_change || + if (gpiod_to_irq(desc) >= 0 && (direction_may_change || !test_bit(FLAG_IS_OUT, &desc->flags))) { status = device_create_file(dev, &dev_attr_edge); if (status) @@ -757,9 +816,15 @@ fail_unregister_device: device_unregister(dev); fail_unlock: mutex_unlock(&sysfs_lock); - pr_debug("%s: gpio%d status %d\n", __func__, gpio, status); + pr_debug("%s: gpio%d status %d\n", __func__, desc_to_gpio(desc), + status); return status; } + +int gpio_export(unsigned gpio, bool direction_may_change) +{ + return gpiod_export(gpio_to_desc(gpio), direction_may_change); +} EXPORT_SYMBOL_GPL(gpio_export); static int match_export(struct device *dev, void *data) @@ -778,18 +843,16 @@ static int match_export(struct device *dev, void *data) * * Returns zero on success, else an error. */ -int gpio_export_link(struct device *dev, const char *name, unsigned gpio) +static int gpiod_export_link(struct device *dev, const char *name, + struct gpio_desc *desc) { - struct gpio_desc *desc; int status = -EINVAL; - if (!gpio_is_valid(gpio)) + if (!desc) goto done; mutex_lock(&sysfs_lock); - desc = &gpio_desc[gpio]; - if (test_bit(FLAG_EXPORT, &desc->flags)) { struct device *tdev; @@ -806,12 +869,17 @@ int gpio_export_link(struct device *dev, const char *name, unsigned gpio) done: if (status) - pr_debug("%s: gpio%d status %d\n", __func__, gpio, status); + pr_debug("%s: gpio%d status %d\n", __func__, desc_to_gpio(desc), + status); return status; } -EXPORT_SYMBOL_GPL(gpio_export_link); +int gpio_export_link(struct device *dev, const char *name, unsigned gpio) +{ + return gpiod_export_link(dev, name, gpio_to_desc(gpio)); +} +EXPORT_SYMBOL_GPL(gpio_export_link); /** * gpio_sysfs_set_active_low - set the polarity of gpio sysfs value @@ -825,19 +893,16 @@ EXPORT_SYMBOL_GPL(gpio_export_link); * * Returns zero on success, else an error. */ -int gpio_sysfs_set_active_low(unsigned gpio, int value) +static int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value) { - struct gpio_desc *desc; struct device *dev = NULL; int status = -EINVAL; - if (!gpio_is_valid(gpio)) + if (!desc) goto done; mutex_lock(&sysfs_lock); - desc = &gpio_desc[gpio]; - if (test_bit(FLAG_EXPORT, &desc->flags)) { dev = class_find_device(&gpio_class, NULL, desc, match_export); if (dev == NULL) { @@ -853,10 +918,16 @@ unlock: done: if (status) - pr_debug("%s: gpio%d status %d\n", __func__, gpio, status); + pr_debug("%s: gpio%d status %d\n", __func__, desc_to_gpio(desc), + status); return status; } + +int gpio_sysfs_set_active_low(unsigned gpio, int value) +{ + return gpiod_sysfs_set_active_low(gpio_to_desc(gpio), value); +} EXPORT_SYMBOL_GPL(gpio_sysfs_set_active_low); /** @@ -865,21 +936,18 @@ EXPORT_SYMBOL_GPL(gpio_sysfs_set_active_low); * * This is implicit on gpio_free(). */ -void gpio_unexport(unsigned gpio) +static void gpiod_unexport(struct gpio_desc *desc) { - struct gpio_desc *desc; int status = 0; struct device *dev = NULL; - if (!gpio_is_valid(gpio)) { + if (!desc) { status = -EINVAL; goto done; } mutex_lock(&sysfs_lock); - desc = &gpio_desc[gpio]; - if (test_bit(FLAG_EXPORT, &desc->flags)) { dev = class_find_device(&gpio_class, NULL, desc, match_export); @@ -891,13 +959,20 @@ void gpio_unexport(unsigned gpio) } mutex_unlock(&sysfs_lock); + if (dev) { device_unregister(dev); put_device(dev); } done: if (status) - pr_debug("%s: gpio%d status %d\n", __func__, gpio, status); + pr_debug("%s: gpio%d status %d\n", __func__, desc_to_gpio(desc), + status); +} + +void gpio_unexport(unsigned gpio) +{ + gpiod_unexport(gpio_to_desc(gpio)); } EXPORT_SYMBOL_GPL(gpio_unexport); @@ -1281,20 +1356,18 @@ EXPORT_SYMBOL_GPL(gpiochip_remove_pin_ranges); * on each other, and help provide better diagnostics in debugfs. * They're called even less than the "set direction" calls. */ -int gpio_request(unsigned gpio, const char *label) +static int gpiod_request(struct gpio_desc *desc, const char *label) { - struct gpio_desc *desc; struct gpio_chip *chip; int status = -EPROBE_DEFER; unsigned long flags; spin_lock_irqsave(&gpio_lock, flags); - if (!gpio_is_valid(gpio)) { + if (!desc) { status = -EINVAL; goto done; } - desc = &gpio_desc[gpio]; chip = desc->chip; if (chip == NULL) goto done; @@ -1318,7 +1391,7 @@ int gpio_request(unsigned gpio, const char *label) if (chip->request) { /* chip->request may sleep */ spin_unlock_irqrestore(&gpio_lock, flags); - status = chip->request(chip, gpio - chip->base); + status = chip->request(chip, gpio_chip_hwgpio(desc)); spin_lock_irqsave(&gpio_lock, flags); if (status < 0) { @@ -1331,42 +1404,46 @@ int gpio_request(unsigned gpio, const char *label) if (chip->get_direction) { /* chip->get_direction may sleep */ spin_unlock_irqrestore(&gpio_lock, flags); - gpio_get_direction(gpio); + gpiod_get_direction(desc); spin_lock_irqsave(&gpio_lock, flags); } done: if (status) - pr_debug("gpio_request: gpio-%d (%s) status %d\n", - gpio, label ? : "?", status); + pr_debug("_gpio_request: gpio-%d (%s) status %d\n", + desc ? desc_to_gpio(desc) : -1, + label ? : "?", status); spin_unlock_irqrestore(&gpio_lock, flags); return status; } + +int gpio_request(unsigned gpio, const char *label) +{ + return gpiod_request(gpio_to_desc(gpio), label); +} EXPORT_SYMBOL_GPL(gpio_request); -void gpio_free(unsigned gpio) +static void gpiod_free(struct gpio_desc *desc) { unsigned long flags; - struct gpio_desc *desc; struct gpio_chip *chip; might_sleep(); - if (!gpio_is_valid(gpio)) { + if (!desc) { WARN_ON(extra_checks); return; } - gpio_unexport(gpio); + gpiod_unexport(desc); spin_lock_irqsave(&gpio_lock, flags); - desc = &gpio_desc[gpio]; chip = desc->chip; if (chip && test_bit(FLAG_REQUESTED, &desc->flags)) { if (chip->free) { spin_unlock_irqrestore(&gpio_lock, flags); might_sleep_if(chip->can_sleep); - chip->free(chip, gpio - chip->base); + chip->free(chip, gpio_chip_hwgpio(desc)); spin_lock_irqsave(&gpio_lock, flags); } desc_set_label(desc, NULL); @@ -1380,6 +1457,11 @@ void gpio_free(unsigned gpio) spin_unlock_irqrestore(&gpio_lock, flags); } + +void gpio_free(unsigned gpio) +{ + gpiod_free(gpio_to_desc(gpio)); +} EXPORT_SYMBOL_GPL(gpio_free); /** @@ -1390,29 +1472,32 @@ EXPORT_SYMBOL_GPL(gpio_free); */ int gpio_request_one(unsigned gpio, unsigned long flags, const char *label) { + struct gpio_desc *desc; int err; - err = gpio_request(gpio, label); + desc = gpio_to_desc(gpio); + + err = gpiod_request(desc, label); if (err) return err; if (flags & GPIOF_OPEN_DRAIN) - set_bit(FLAG_OPEN_DRAIN, &gpio_desc[gpio].flags); + set_bit(FLAG_OPEN_DRAIN, &desc->flags); if (flags & GPIOF_OPEN_SOURCE) - set_bit(FLAG_OPEN_SOURCE, &gpio_desc[gpio].flags); + set_bit(FLAG_OPEN_SOURCE, &desc->flags); if (flags & GPIOF_DIR_IN) - err = gpio_direction_input(gpio); + err = gpiod_direction_input(desc); else - err = gpio_direction_output(gpio, + err = gpiod_direction_output(desc, (flags & GPIOF_INIT_HIGH) ? 1 : 0); if (err) goto free_gpio; if (flags & GPIOF_EXPORT) { - err = gpio_export(gpio, flags & GPIOF_EXPORT_CHANGEABLE); + err = gpiod_export(desc, flags & GPIOF_EXPORT_CHANGEABLE); if (err) goto free_gpio; } @@ -1420,7 +1505,7 @@ int gpio_request_one(unsigned gpio, unsigned long flags, const char *label) return 0; free_gpio: - gpio_free(gpio); + gpiod_free(desc); return err; } EXPORT_SYMBOL_GPL(gpio_request_one); @@ -1476,13 +1561,14 @@ EXPORT_SYMBOL_GPL(gpio_free_array); const char *gpiochip_is_requested(struct gpio_chip *chip, unsigned offset) { unsigned gpio = chip->base + offset; + struct gpio_desc *desc = &gpio_desc[gpio]; - if (!gpio_is_valid(gpio) || gpio_desc[gpio].chip != chip) + if (!gpio_is_valid(gpio) || desc->chip != chip) return NULL; - if (test_bit(FLAG_REQUESTED, &gpio_desc[gpio].flags) == 0) + if (test_bit(FLAG_REQUESTED, &desc->flags) == 0) return NULL; #ifdef CONFIG_DEBUG_FS - return gpio_desc[gpio].label; + return desc->label; #else return "?"; #endif @@ -1499,24 +1585,21 @@ EXPORT_SYMBOL_GPL(gpiochip_is_requested); * rely on gpio_request() having been called beforehand. */ -int gpio_direction_input(unsigned gpio) +static int gpiod_direction_input(struct gpio_desc *desc) { unsigned long flags; struct gpio_chip *chip; - struct gpio_desc *desc = &gpio_desc[gpio]; int status = -EINVAL; + int offset; spin_lock_irqsave(&gpio_lock, flags); - if (!gpio_is_valid(gpio)) + if (!desc) goto fail; chip = desc->chip; if (!chip || !chip->get || !chip->direction_input) goto fail; - gpio -= chip->base; - if (gpio >= chip->ngpio) - goto fail; - status = gpio_ensure_requested(desc, gpio); + status = gpio_ensure_requested(desc); if (status < 0) goto fail; @@ -1526,11 +1609,12 @@ int gpio_direction_input(unsigned gpio) might_sleep_if(chip->can_sleep); + offset = gpio_chip_hwgpio(desc); if (status) { - status = chip->request(chip, gpio); + status = chip->request(chip, offset); if (status < 0) { pr_debug("GPIO-%d: chip request fail, %d\n", - chip->base + gpio, status); + desc_to_gpio(desc), status); /* and it's not available to anyone else ... * gpio_request() is the fully clean solution. */ @@ -1538,48 +1622,54 @@ int gpio_direction_input(unsigned gpio) } } - status = chip->direction_input(chip, gpio); + status = chip->direction_input(chip, offset); if (status == 0) clear_bit(FLAG_IS_OUT, &desc->flags); - trace_gpio_direction(chip->base + gpio, 1, status); + trace_gpio_direction(desc_to_gpio(desc), 1, status); lose: return status; fail: spin_unlock_irqrestore(&gpio_lock, flags); - if (status) + if (status) { + int gpio = -1; + if (desc) + gpio = desc_to_gpio(desc); pr_debug("%s: gpio-%d status %d\n", __func__, gpio, status); + } return status; } + +int gpio_direction_input(unsigned gpio) +{ + return gpiod_direction_input(gpio_to_desc(gpio)); +} EXPORT_SYMBOL_GPL(gpio_direction_input); -int gpio_direction_output(unsigned gpio, int value) +static int gpiod_direction_output(struct gpio_desc *desc, int value) { unsigned long flags; struct gpio_chip *chip; - struct gpio_desc *desc = &gpio_desc[gpio]; int status = -EINVAL; + int offset; /* Open drain pin should not be driven to 1 */ if (value && test_bit(FLAG_OPEN_DRAIN, &desc->flags)) - return gpio_direction_input(gpio); + return gpiod_direction_input(desc); /* Open source pin should not be driven to 0 */ if (!value && test_bit(FLAG_OPEN_SOURCE, &desc->flags)) - return gpio_direction_input(gpio); + return gpiod_direction_input(desc); spin_lock_irqsave(&gpio_lock, flags); - if (!gpio_is_valid(gpio)) + if (!desc) goto fail; chip = desc->chip; if (!chip || !chip->set || !chip->direction_output) goto fail; - gpio -= chip->base; - if (gpio >= chip->ngpio) - goto fail; - status = gpio_ensure_requested(desc, gpio); + status = gpio_ensure_requested(desc); if (status < 0) goto fail; @@ -1589,11 +1679,12 @@ int gpio_direction_output(unsigned gpio, int value) might_sleep_if(chip->can_sleep); + offset = gpio_chip_hwgpio(desc); if (status) { - status = chip->request(chip, gpio); + status = chip->request(chip, offset); if (status < 0) { pr_debug("GPIO-%d: chip request fail, %d\n", - chip->base + gpio, status); + desc_to_gpio(desc), status); /* and it's not available to anyone else ... * gpio_request() is the fully clean solution. */ @@ -1601,20 +1692,29 @@ int gpio_direction_output(unsigned gpio, int value) } } - status = chip->direction_output(chip, gpio, value); + status = chip->direction_output(chip, offset, value); if (status == 0) set_bit(FLAG_IS_OUT, &desc->flags); - trace_gpio_value(chip->base + gpio, 0, value); - trace_gpio_direction(chip->base + gpio, 0, status); + trace_gpio_value(desc_to_gpio(desc), 0, value); + trace_gpio_direction(desc_to_gpio(desc), 0, status); lose: return status; fail: spin_unlock_irqrestore(&gpio_lock, flags); - if (status) + if (status) { + int gpio = -1; + if (desc) + gpio = desc_to_gpio(desc); pr_debug("%s: gpio-%d status %d\n", __func__, gpio, status); + } return status; } + +int gpio_direction_output(unsigned gpio, int value) +{ + return gpiod_direction_output(gpio_to_desc(gpio), value); +} EXPORT_SYMBOL_GPL(gpio_direction_output); /** @@ -1622,24 +1722,22 @@ EXPORT_SYMBOL_GPL(gpio_direction_output); * @gpio: the gpio to set debounce time * @debounce: debounce time is microseconds */ -int gpio_set_debounce(unsigned gpio, unsigned debounce) +static int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce) { unsigned long flags; struct gpio_chip *chip; - struct gpio_desc *desc = &gpio_desc[gpio]; int status = -EINVAL; + int offset; spin_lock_irqsave(&gpio_lock, flags); - if (!gpio_is_valid(gpio)) + if (!desc) goto fail; chip = desc->chip; if (!chip || !chip->set || !chip->set_debounce) goto fail; - gpio -= chip->base; - if (gpio >= chip->ngpio) - goto fail; - status = gpio_ensure_requested(desc, gpio); + + status = gpio_ensure_requested(desc); if (status < 0) goto fail; @@ -1649,16 +1747,26 @@ int gpio_set_debounce(unsigned gpio, unsigned debounce) might_sleep_if(chip->can_sleep); - return chip->set_debounce(chip, gpio, debounce); + offset = gpio_chip_hwgpio(desc); + return chip->set_debounce(chip, offset, debounce); fail: spin_unlock_irqrestore(&gpio_lock, flags); - if (status) + if (status) { + int gpio = -1; + if (desc) + gpio = desc_to_gpio(desc); pr_debug("%s: gpio-%d status %d\n", __func__, gpio, status); + } return status; } + +int gpio_set_debounce(unsigned gpio, unsigned debounce) +{ + return gpiod_set_debounce(gpio_to_desc(gpio), debounce); +} EXPORT_SYMBOL_GPL(gpio_set_debounce); /* I/O calls are only valid after configuration completed; the relevant @@ -1692,18 +1800,25 @@ EXPORT_SYMBOL_GPL(gpio_set_debounce); * It returns the zero or nonzero value provided by the associated * gpio_chip.get() method; or zero if no such method is provided. */ -int __gpio_get_value(unsigned gpio) +static int gpiod_get_value(struct gpio_desc *desc) { struct gpio_chip *chip; int value; + int offset; - chip = gpio_to_chip(gpio); + chip = desc->chip; + offset = gpio_chip_hwgpio(desc); /* Should be using gpio_get_value_cansleep() */ WARN_ON(chip->can_sleep); - value = chip->get ? chip->get(chip, gpio - chip->base) : 0; - trace_gpio_value(gpio, 1, value); + value = chip->get ? chip->get(chip, offset) : 0; + trace_gpio_value(desc_to_gpio(desc), 1, value); return value; } + +int __gpio_get_value(unsigned gpio) +{ + return gpiod_get_value(gpio_to_desc(gpio)); +} EXPORT_SYMBOL_GPL(__gpio_get_value); /* @@ -1712,23 +1827,25 @@ EXPORT_SYMBOL_GPL(__gpio_get_value); * @chip: Gpio chip. * @value: Non-zero for setting it HIGH otherise it will set to LOW. */ -static void _gpio_set_open_drain_value(unsigned gpio, - struct gpio_chip *chip, int value) +static void _gpio_set_open_drain_value(struct gpio_desc *desc, int value) { int err = 0; + struct gpio_chip *chip = desc->chip; + int offset = gpio_chip_hwgpio(desc); + if (value) { - err = chip->direction_input(chip, gpio - chip->base); + err = chip->direction_input(chip, offset); if (!err) - clear_bit(FLAG_IS_OUT, &gpio_desc[gpio].flags); + clear_bit(FLAG_IS_OUT, &desc->flags); } else { - err = chip->direction_output(chip, gpio - chip->base, 0); + err = chip->direction_output(chip, offset, 0); if (!err) - set_bit(FLAG_IS_OUT, &gpio_desc[gpio].flags); + set_bit(FLAG_IS_OUT, &desc->flags); } - trace_gpio_direction(gpio, value, err); + trace_gpio_direction(desc_to_gpio(desc), value, err); if (err < 0) pr_err("%s: Error in set_value for open drain gpio%d err %d\n", - __func__, gpio, err); + __func__, desc_to_gpio(desc), err); } /* @@ -1737,26 +1854,27 @@ static void _gpio_set_open_drain_value(unsigned gpio, * @chip: Gpio chip. * @value: Non-zero for setting it HIGH otherise it will set to LOW. */ -static void _gpio_set_open_source_value(unsigned gpio, - struct gpio_chip *chip, int value) +static void _gpio_set_open_source_value(struct gpio_desc *desc, int value) { int err = 0; + struct gpio_chip *chip = desc->chip; + int offset = gpio_chip_hwgpio(desc); + if (value) { - err = chip->direction_output(chip, gpio - chip->base, 1); + err = chip->direction_output(chip, offset, 1); if (!err) - set_bit(FLAG_IS_OUT, &gpio_desc[gpio].flags); + set_bit(FLAG_IS_OUT, &desc->flags); } else { - err = chip->direction_input(chip, gpio - chip->base); + err = chip->direction_input(chip, offset); if (!err) - clear_bit(FLAG_IS_OUT, &gpio_desc[gpio].flags); + clear_bit(FLAG_IS_OUT, &desc->flags); } - trace_gpio_direction(gpio, !value, err); + trace_gpio_direction(desc_to_gpio(desc), !value, err); if (err < 0) pr_err("%s: Error in set_value for open source gpio%d err %d\n", - __func__, gpio, err); + __func__, desc_to_gpio(desc), err); } - /** * __gpio_set_value() - assign a gpio's value * @gpio: gpio whose value will be assigned @@ -1766,20 +1884,25 @@ static void _gpio_set_open_source_value(unsigned gpio, * This is used directly or indirectly to implement gpio_set_value(). * It invokes the associated gpio_chip.set() method. */ -void __gpio_set_value(unsigned gpio, int value) +static void gpiod_set_value(struct gpio_desc *desc, int value) { struct gpio_chip *chip; - chip = gpio_to_chip(gpio); + chip = desc->chip; /* Should be using gpio_set_value_cansleep() */ WARN_ON(chip->can_sleep); - trace_gpio_value(gpio, 0, value); - if (test_bit(FLAG_OPEN_DRAIN, &gpio_desc[gpio].flags)) - _gpio_set_open_drain_value(gpio, chip, value); - else if (test_bit(FLAG_OPEN_SOURCE, &gpio_desc[gpio].flags)) - _gpio_set_open_source_value(gpio, chip, value); + trace_gpio_value(desc_to_gpio(desc), 0, value); + if (test_bit(FLAG_OPEN_DRAIN, &desc->flags)) + _gpio_set_open_drain_value(desc, value); + else if (test_bit(FLAG_OPEN_SOURCE, &desc->flags)) + _gpio_set_open_source_value(desc, value); else - chip->set(chip, gpio - chip->base, value); + chip->set(chip, gpio_chip_hwgpio(desc), value); +} + +void __gpio_set_value(unsigned gpio, int value) +{ + return gpiod_set_value(gpio_to_desc(gpio), value); } EXPORT_SYMBOL_GPL(__gpio_set_value); @@ -1791,14 +1914,15 @@ EXPORT_SYMBOL_GPL(__gpio_set_value); * This is used directly or indirectly to implement gpio_cansleep(). It * returns nonzero if access reading or writing the GPIO value can sleep. */ -int __gpio_cansleep(unsigned gpio) +static int gpiod_cansleep(struct gpio_desc *desc) { - struct gpio_chip *chip; - /* only call this on GPIOs that are valid! */ - chip = gpio_to_chip(gpio); + return desc->chip->can_sleep; +} - return chip->can_sleep; +int __gpio_cansleep(unsigned gpio) +{ + return gpiod_cansleep(gpio_to_desc(gpio)); } EXPORT_SYMBOL_GPL(__gpio_cansleep); @@ -1811,50 +1935,67 @@ EXPORT_SYMBOL_GPL(__gpio_cansleep); * It returns the number of the IRQ signaled by this (input) GPIO, * or a negative errno. */ -int __gpio_to_irq(unsigned gpio) +static int gpiod_to_irq(struct gpio_desc *desc) { struct gpio_chip *chip; + int offset; - chip = gpio_to_chip(gpio); - return chip->to_irq ? chip->to_irq(chip, gpio - chip->base) : -ENXIO; + chip = desc->chip; + offset = gpio_chip_hwgpio(desc); + return chip->to_irq ? chip->to_irq(chip, offset) : -ENXIO; } -EXPORT_SYMBOL_GPL(__gpio_to_irq); +int __gpio_to_irq(unsigned gpio) +{ + return gpiod_to_irq(gpio_to_desc(gpio)); +} +EXPORT_SYMBOL_GPL(__gpio_to_irq); /* There's no value in making it easy to inline GPIO calls that may sleep. * Common examples include ones connected to I2C or SPI chips. */ -int gpio_get_value_cansleep(unsigned gpio) +static int gpiod_get_value_cansleep(struct gpio_desc *desc) { struct gpio_chip *chip; int value; + int offset; might_sleep_if(extra_checks); - chip = gpio_to_chip(gpio); - value = chip->get ? chip->get(chip, gpio - chip->base) : 0; - trace_gpio_value(gpio, 1, value); + chip = desc->chip; + offset = gpio_chip_hwgpio(desc); + value = chip->get ? chip->get(chip, offset) : 0; + trace_gpio_value(desc_to_gpio(desc), 1, value); return value; } + +int gpio_get_value_cansleep(unsigned gpio) +{ + return gpiod_get_value_cansleep(gpio_to_desc(gpio)); +} EXPORT_SYMBOL_GPL(gpio_get_value_cansleep); -void gpio_set_value_cansleep(unsigned gpio, int value) +static void gpiod_set_value_cansleep(struct gpio_desc *desc, int value) { struct gpio_chip *chip; might_sleep_if(extra_checks); - chip = gpio_to_chip(gpio); - trace_gpio_value(gpio, 0, value); - if (test_bit(FLAG_OPEN_DRAIN, &gpio_desc[gpio].flags)) - _gpio_set_open_drain_value(gpio, chip, value); - else if (test_bit(FLAG_OPEN_SOURCE, &gpio_desc[gpio].flags)) - _gpio_set_open_source_value(gpio, chip, value); + chip = desc->chip; + trace_gpio_value(desc_to_gpio(desc), 0, value); + if (test_bit(FLAG_OPEN_DRAIN, &desc->flags)) + _gpio_set_open_drain_value(desc, value); + else if (test_bit(FLAG_OPEN_SOURCE, &desc->flags)) + _gpio_set_open_source_value(desc, value); else - chip->set(chip, gpio - chip->base, value); + chip->set(chip, gpio_chip_hwgpio(desc), value); } -EXPORT_SYMBOL_GPL(gpio_set_value_cansleep); +void gpio_set_value_cansleep(unsigned gpio, int value) +{ + return gpiod_set_value_cansleep(gpio_to_desc(gpio), value); +} +EXPORT_SYMBOL_GPL(gpio_set_value_cansleep); #ifdef CONFIG_DEBUG_FS @@ -1869,7 +2010,7 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip) if (!test_bit(FLAG_REQUESTED, &gdesc->flags)) continue; - gpio_get_direction(gpio); + gpiod_get_direction(gdesc); is_out = test_bit(FLAG_IS_OUT, &gdesc->flags); seq_printf(s, " gpio-%-3d (%-20.20s) %s %s", gpio, gdesc->label,
Make sure gpiolib works internally with descriptors and (chip, offset) pairs instead of using the global integer namespace. This prepares the ground for the removal of the global gpio_desc[] array and the introduction of the descriptor-based GPIO API. Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> --- drivers/gpio/gpiolib.c | 493 +++++++++++++++++++++++++++++++------------------ 1 file changed, 317 insertions(+), 176 deletions(-)