| Message ID | 1354779918-4028-2-git-send-email-acourbot@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 6, 2012 at 8:45 AM, Alexandre Courbot <acourbot@nvidia.com> wrote: > With the current API, GPIOs are manipulated through an integer which > represents their unique number across the system. This poses problems in > terms of portability, scalability and flexibility: for instance, the > number of valid GPIOs for a given system is fixed at system time, and a > large array of that size is statically allocated to hold the GPIO > descriptors. Worse, GPIOs can be used without being properly allocated. > > In order to improve the situation, the integer namespace must first get > away. This patch introduces an alternative GPIO API that uses opaque > handlers and refactor gpiolib's internals to work with these handlers > instead of GPIO numbers. The former integer-based API is still available > as a light wrapper around this new API. > > This first step will then us to build more improvements for gpiolib, > like proper GPIO lookup functions per device and provider, and getting > rid of the static GPIO array and the ARCH_NR_GPIO configuration option. > > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> I really like this. It is looking very good. Especially it's nice that it makes GPIO descriptors the norm rather than a bolt-on, and the old linear GPIO space is turned into a comaptibility layer using static inlines. I'd like to see some love for Documentation/gpio.txt to reflect the changes, plus considering Grants note, and I will be happy to add Reviewed-by on this. Especially the lookups will be nice to see going forward! Yours, Linus Walleij
On Thursday 06 December 2012 22:42:59 Grant Likely wrote: > how about "gpio_chip_hwnum()" to somewhat match irqdomain convention? Sure, matching existing interfaces is better. "git grep hwnum" does not seem to reveal much related to irqdomain though? > I've only lightly scanned this patch, but I like what I see. I would > keep going with it. Fantastic then - I just wanted to make sure things were not completely broken from the beginning. Since you and Linus W. seem to approve this, I will flush what I have in my head and publish a proper series for review. Thanks! Alex.
On Thu, Dec 06, 2012 at 04:45:18PM +0900, Alexandre Courbot wrote: > With the current API, GPIOs are manipulated through an integer which > represents their unique number across the system. This poses problems in > terms of portability, scalability and flexibility: for instance, the > number of valid GPIOs for a given system is fixed at system time, and a > large array of that size is statically allocated to hold the GPIO > descriptors. Worse, GPIOs can be used without being properly allocated. > > In order to improve the situation, the integer namespace must first get > away. This patch introduces an alternative GPIO API that uses opaque > handlers and refactor gpiolib's internals to work with these handlers > instead of GPIO numbers. The former integer-based API is still available > as a light wrapper around this new API. > > This first step will then us to build more improvements for gpiolib, > like proper GPIO lookup functions per device and provider, and getting > rid of the static GPIO array and the ARCH_NR_GPIO configuration option. > > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> I like the idea, especially getting rid of ARCH_NR_GPIO. I have a system with 1000+ GPIO pins, so that will come handy. My own idea for a solution was to keep integer based handles, but replace gpio_desc[] with a hash table. But ultimately I don't really care how it is done. Do you have a solution for gpiochip_find_base() in mind, and how to handle reservations ? I had thought about using bit maps, but maybe there is a better solution. If/when you have some code to test, please let me know. Couple of comments below. Guenter > --- > drivers/gpio/gpiolib.c | 302 ++++++++++++++++++++++-------------------- > include/asm-generic/gpio.h | 74 ++++++++--- > include/linux/gpio/consumer.h | 45 +++++++ > 3 files changed, 261 insertions(+), 160 deletions(-) > create mode 100644 include/linux/gpio/consumer.h > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 1c8d9e3..bf32511 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -83,6 +83,32 @@ static inline void desc_set_label(struct gpio_desc *d, const char *label) > #endif > } > > +/** > + * 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 inline int desc_to_gpio(struct gpio_desc *desc) > +{ > + return desc - &gpio_desc[0]; > +} > + > +/** > + * Return the GPIO number of the passed descriptor relative to its chip > + */ > +int gpio_chip_offset(struct gpio_desc *desc) > +{ > + return (desc - &gpio_desc[0]) - desc->chip->base; > +} > + > +/** > + * Convert a GPIO number to its descriptor > + */ > +struct gpio_desc *gpio_to_desc(unsigned gpio) > +{ > + return &gpio_desc[gpio]; > +} > + > /* 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 > @@ -94,10 +120,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)) { > @@ -116,9 +142,9 @@ static int gpio_ensure_requested(struct gpio_desc *desc, unsigned offset) > } > > /* caller holds gpio_lock *OR* gpio is marked as requested */ > -struct gpio_chip *gpio_to_chip(unsigned gpio) > +struct gpio_chip *gpiod_to_chip(struct gpio_desc *desc) > { > - return gpio_desc[gpio].chip; > + return desc->chip; > } > > /* dynamic allocation of GPIOs, e.g. on a hotplugged device */ > @@ -684,10 +710,9 @@ static struct class gpio_class = { > .class_attrs = gpio_class_attrs, > }; > > - > /** > - * gpio_export - export a GPIO through sysfs > - * @gpio: gpio to make available, already requested > + * gpiod_export - export a GPIO through sysfs > + * @desc: descriptor of gpio to make available, already requested > * @direction_may_change: true if userspace may change gpio direction > * Context: arch_initcall or later > * > @@ -700,12 +725,17 @@ static struct class gpio_class = { > * > * Returns zero on success, else an error. > */ > -int gpio_export(unsigned gpio, bool direction_may_change) > +int gpiod_export(struct gpio_desc *desc, bool direction_may_change) > { > unsigned long flags; > - struct gpio_desc *desc; > int status = -EINVAL; > const char *ioname = NULL; > + int gpio; > + > + if (!desc) > + goto done; > + > + mutex_lock(&sysfs_lock); > > /* can't export until sysfs is available ... */ > if (!gpio_class.p) { > @@ -713,13 +743,7 @@ int gpio_export(unsigned gpio, bool direction_may_change) > return -ENOENT; mutex is not released here. > } > > - if (!gpio_is_valid(gpio)) > - goto done; > - > - 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)) { > status = 0; > @@ -729,6 +753,7 @@ int gpio_export(unsigned gpio, bool direction_may_change) > } > spin_unlock_irqrestore(&gpio_lock, flags); > > + gpio = desc_to_gpio(desc); > if (desc->chip->names && desc->chip->names[gpio - desc->chip->base]) > ioname = desc->chip->names[gpio - desc->chip->base]; > > @@ -768,7 +793,7 @@ done: > > return status; > } > -EXPORT_SYMBOL_GPL(gpio_export); > +EXPORT_SYMBOL_GPL(gpiod_export); > > static int match_export(struct device *dev, void *data) > { > @@ -776,28 +801,26 @@ static int match_export(struct device *dev, void *data) > } > > /** > - * gpio_export_link - create a sysfs link to an exported GPIO node > + * gpiod_export_link - create a sysfs link to an exported GPIO node > * @dev: device under which to create symlink > * @name: name of the symlink > - * @gpio: gpio to create symlink to, already exported > + * @desc: descriptor of gpio to create symlink to, already exported > * > * Set up a symlink from /sys/.../dev/name to /sys/class/gpio/gpioN > * node. Caller is responsible for unlinking. > * > * Returns zero on success, else an error. > */ > -int gpio_export_link(struct device *dev, const char *name, unsigned gpio) > +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; > > @@ -814,16 +837,16 @@ 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); > - > +EXPORT_SYMBOL_GPL(gpiod_export_link); > > /** > - * gpio_sysfs_set_active_low - set the polarity of gpio sysfs value > - * @gpio: gpio to change > + * gpiod_sysfs_set_active_low - set the polarity of gpio sysfs value > + * @desc: descriptor of gpio to change > * @value: non-zero to use active low, i.e. inverted values > * > * Set the polarity of /sys/class/gpio/gpioN/value sysfs attribute. > @@ -833,19 +856,16 @@ EXPORT_SYMBOL_GPL(gpio_export_link); > * > * Returns zero on success, else an error. > */ > -int gpio_sysfs_set_active_low(unsigned gpio, int value) > +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) { > @@ -861,33 +881,31 @@ 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; > } > -EXPORT_SYMBOL_GPL(gpio_sysfs_set_active_low); > +EXPORT_SYMBOL_GPL(gpiod_sysfs_set_active_low); > > /** > - * gpio_unexport - reverse effect of gpio_export() > - * @gpio: gpio to make unavailable > + * gpiod_unexport - reverse effect of gpio_export() > + * @desc: descriptor of gpio to make unavailable > * > * This is implicit on gpio_free(). > */ > -void gpio_unexport(unsigned gpio) > +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); > @@ -899,15 +917,17 @@ 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); > } > -EXPORT_SYMBOL_GPL(gpio_unexport); > +EXPORT_SYMBOL_GPL(gpiod_unexport); > > static int gpiochip_export(struct gpio_chip *chip) > { > @@ -1386,7 +1406,6 @@ const char *gpiochip_is_requested(struct gpio_chip *chip, unsigned offset) > } > EXPORT_SYMBOL_GPL(gpiochip_is_requested); > > - > /* Drivers MUST set GPIO direction before making get/set calls. In > * some cases this is done in early boot, before IRQs are enabled. > * > @@ -1396,24 +1415,19 @@ EXPORT_SYMBOL_GPL(gpiochip_is_requested); > * rely on gpio_request() having been called beforehand. > */ > > -int gpio_direction_input(unsigned gpio) > +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 gpio; > > 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; > > @@ -1423,6 +1437,8 @@ int gpio_direction_input(unsigned gpio) > > might_sleep_if(chip->can_sleep); > > + gpio = gpio_chip_offset(desc); > + > if (status) { > status = chip->request(chip, gpio); > if (status < 0) { > @@ -1449,34 +1465,31 @@ fail: > __func__, gpio, status); > return status; > } > -EXPORT_SYMBOL_GPL(gpio_direction_input); > +EXPORT_SYMBOL_GPL(gpiod_direction_input); > > -int gpio_direction_output(unsigned gpio, int value) > +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 gpio; > > /* 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; > > @@ -1485,6 +1498,7 @@ int gpio_direction_output(unsigned gpio, int value) > spin_unlock_irqrestore(&gpio_lock, flags); > > might_sleep_if(chip->can_sleep); > + gpio = gpio_chip_offset(desc); > > if (status) { > status = chip->request(chip, gpio); > @@ -1515,28 +1529,26 @@ fail: > EXPORT_SYMBOL_GPL(gpio_direction_output); > > /** > - * gpio_set_debounce - sets @debounce time for a @gpio > - * @gpio: the gpio to set debounce time > + * gpiod_set_debounce - sets @debounce time for a gpio > + * @desc: descriptor to the gpio to set debounce time > * @debounce: debounce time is microseconds > */ > -int gpio_set_debounce(unsigned gpio, unsigned debounce) > +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 gpio; > > 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; > > @@ -1546,6 +1558,7 @@ int gpio_set_debounce(unsigned gpio, unsigned debounce) > > might_sleep_if(chip->can_sleep); > > + gpio = gpio_chip_offset(desc); > return chip->set_debounce(chip, gpio, debounce); > > fail: > @@ -1556,7 +1569,7 @@ fail: > > return status; > } > -EXPORT_SYMBOL_GPL(gpio_set_debounce); > +EXPORT_SYMBOL_GPL(gpiod_set_debounce); > > /* I/O calls are only valid after configuration completed; the relevant > * "is this a valid GPIO" error checks should already have been done. > @@ -1581,27 +1594,28 @@ EXPORT_SYMBOL_GPL(gpio_set_debounce); > */ > > /** > - * __gpio_get_value() - return a gpio's value > - * @gpio: gpio whose value will be returned > + * gpiod_get_value() - return a gpio's value > + * @desc: descriptor of gpio whose value will be returned > * Context: any > * > - * This is used directly or indirectly to implement gpio_get_value(). > - * It returns the zero or nonzero value provided by the associated > + * 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) > +int gpiod_get_value(struct gpio_desc *desc) > { > struct gpio_chip *chip; > int value; > + int gpio; > > - chip = gpio_to_chip(gpio); > + chip = desc->chip; > + gpio = gpio_chip_offset(desc); Might be better to use a separate variable named 'offset' when dealing with the offset, to avoid confusion and accidential bugs. You are doing it below, so you might as well do it everywhere. This would also simplify the code in a couple of places where gpio is first converted into an offset only to use "chip->base + gpio" later on. > /* 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, gpio) : 0; > + trace_gpio_value(gpio + chip->base, 1, value); > return value; > } > -EXPORT_SYMBOL_GPL(__gpio_get_value); > +EXPORT_SYMBOL_GPL(gpiod_get_value); > > /* > * _gpio_set_open_drain_value() - Set the open drain gpio's value. > @@ -1609,23 +1623,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_offset(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(offset + chip->base, value, err); > if (err < 0) > pr_err("%s: Error in set_value for open drain gpio%d err %d\n", > - __func__, gpio, err); > + __func__, offset + chip->base, err); I know I am nitpicking, but everywhere in the existing code it is "chip->base + offset/gpio". > } > > /* > @@ -1634,124 +1650,120 @@ 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_offset(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(offset + chip->base, !value, err); > if (err < 0) > pr_err("%s: Error in set_value for open source gpio%d err %d\n", > - __func__, gpio, err); > + __func__, offset + chip->base, err); > } > > - > /** > - * __gpio_set_value() - assign a gpio's value > - * @gpio: gpio whose value will be assigned > + * gpiod_set_value() - assign a gpio's value > + * @desc: descriptor of gpio whose value will be assigned > * @value: value to assign > * Context: any > * > - * This is used directly or indirectly to implement gpio_set_value(). > - * It invokes the associated gpio_chip.set() method. > + * Invokes the associated gpio_chip.set() method. > */ > -void __gpio_set_value(unsigned gpio, int value) > +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_offset(desc), value); > } > -EXPORT_SYMBOL_GPL(__gpio_set_value); > +EXPORT_SYMBOL_GPL(gpiod_set_value); > > /** > - * __gpio_cansleep() - report whether gpio value access will sleep > - * @gpio: gpio in question > + * gpiod_cansleep() - report whether gpio value access will sleep > + * @desc: descriptor of gpio in question > * Context: any > * > - * This is used directly or indirectly to implement gpio_cansleep(). It > - * returns nonzero if access reading or writing the GPIO value can sleep. > + * Returns nonzero if access reading or writing the GPIO value can sleep. > */ > -int __gpio_cansleep(unsigned gpio) > +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 chip->can_sleep; > + return desc->chip->can_sleep; > } > -EXPORT_SYMBOL_GPL(__gpio_cansleep); > +EXPORT_SYMBOL_GPL(gpiod_cansleep); > > /** > - * __gpio_to_irq() - return the IRQ corresponding to a GPIO > - * @gpio: gpio whose IRQ will be returned (already requested) > + * gpiod_to_irq() - return the IRQ corresponding to a GPIO > + * @desc: descriptor of gpio whose IRQ will be returned (already requested) > * Context: any > * > - * This is used directly or indirectly to implement gpio_to_irq(). > - * It returns the number of the IRQ signaled by this (input) GPIO, > + * Returns the number of the IRQ signaled by this (input) GPIO, > * or a negative errno. > */ > -int __gpio_to_irq(unsigned gpio) > +int gpiod_to_irq(struct gpio_desc *desc) > { > struct gpio_chip *chip; > + int gpio; > > - chip = gpio_to_chip(gpio); > + chip = desc->chip; > + gpio = desc_to_gpio(desc); > return chip->to_irq ? chip->to_irq(chip, gpio - chip->base) : -ENXIO; s/gpio - chip->base/gpio_chip_offset(desc)/ then you don't need gpio at all. > } > -EXPORT_SYMBOL_GPL(__gpio_to_irq); > - > - > +EXPORT_SYMBOL_GPL(gpiod_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) > +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_offset(desc); > + value = chip->get ? chip->get(chip, offset) : 0; > + trace_gpio_value(offset + chip->base, 1, value); > return value; > } > -EXPORT_SYMBOL_GPL(gpio_get_value_cansleep); > +EXPORT_SYMBOL_GPL(gpiod_get_value_cansleep); > > -void gpio_set_value_cansleep(unsigned gpio, int value) > +void gpiod_set_value_cansleep(struct gpio_desc *desc, int value) > { > struct gpio_chip *chip; > + int gpio; > > might_sleep_if(extra_checks); > - chip = gpio_to_chip(gpio); > + chip = desc->chip; > + gpio = desc_to_gpio(desc); > trace_gpio_value(gpio, 0, value); > if (test_bit(FLAG_OPEN_DRAIN, &gpio_desc[gpio].flags)) Use desc directly. > - _gpio_set_open_drain_value(gpio, chip, value); > + _gpio_set_open_drain_value(desc, value); > else if (test_bit(FLAG_OPEN_SOURCE, &gpio_desc[gpio].flags)) Use desc directly. > - _gpio_set_open_source_value(gpio, chip, value); > + _gpio_set_open_source_value(desc, value); > else > chip->set(chip, gpio - chip->base, value); > } > -EXPORT_SYMBOL_GPL(gpio_set_value_cansleep); > - > +EXPORT_SYMBOL_GPL(gpiod_set_value_cansleep); > > #ifdef CONFIG_DEBUG_FS > > diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h > index a9432fc..30e1a91 100644 > --- a/include/asm-generic/gpio.h > +++ b/include/asm-generic/gpio.h > @@ -9,6 +9,7 @@ > #ifdef CONFIG_GPIOLIB > > #include <linux/compiler.h> > +#include <linux/gpio/consumer.h> > > /* Platforms may implement their GPIO interface with library code, > * at a small performance cost for non-inlined operations and some > @@ -138,7 +139,10 @@ struct gpio_chip { > > extern const char *gpiochip_is_requested(struct gpio_chip *chip, > unsigned offset); > -extern struct gpio_chip *gpio_to_chip(unsigned gpio); > +static inline struct gpio_chip *gpio_to_chip(unsigned gpio) > +{ > + return gpiod_to_chip(gpio_to_desc(gpio)); > +} > extern int __must_check gpiochip_reserve(int start, int ngpio); > > /* add/remove chips */ > @@ -155,25 +159,52 @@ extern struct gpio_chip *gpiochip_find(void *data, > extern int gpio_request(unsigned gpio, const char *label); > extern void gpio_free(unsigned gpio); > > -extern int gpio_direction_input(unsigned gpio); > -extern int gpio_direction_output(unsigned gpio, int value); > +static inline int gpio_direction_input(unsigned gpio) > +{ > + return gpiod_direction_input(gpio_to_desc(gpio)); > +} > +static inline int gpio_direction_output(unsigned gpio, int value) > +{ > + return gpiod_direction_output(gpio_to_desc(gpio), value); > +} > > -extern int gpio_set_debounce(unsigned gpio, unsigned debounce); > +static inline int gpio_set_debounce(unsigned gpio, unsigned debounce) > +{ > + return gpiod_set_debounce(gpio_to_desc(gpio), debounce); > +} > > -extern int gpio_get_value_cansleep(unsigned gpio); > -extern void gpio_set_value_cansleep(unsigned gpio, int value); > +static inline int gpio_get_value_cansleep(unsigned gpio) > +{ > + return gpiod_get_value_cansleep(gpio_to_desc(gpio)); > +} > +static inline void gpio_set_value_cansleep(unsigned gpio, int value) > +{ > + return gpiod_set_value_cansleep(gpio_to_desc(gpio), value); > +} > > > /* A platform's <asm/gpio.h> code may want to inline the I/O calls when > * the GPIO is constant and refers to some always-present controller, > * giving direct access to chip registers and tight bitbanging loops. > */ > -extern int __gpio_get_value(unsigned gpio); > -extern void __gpio_set_value(unsigned gpio, int value); > +static inline int __gpio_get_value(unsigned gpio) > +{ > + return gpiod_get_value(gpio_to_desc(gpio)); > +} > +static inline void __gpio_set_value(unsigned gpio, int value) > +{ > + gpiod_set_value(gpio_to_desc(gpio), value); > +} > > -extern int __gpio_cansleep(unsigned gpio); > +static inline int __gpio_cansleep(unsigned gpio) > +{ > + return gpiod_cansleep(gpio_to_desc(gpio)); > +} > > -extern int __gpio_to_irq(unsigned gpio); > +static inline int __gpio_to_irq(unsigned gpio) > +{ > + return gpiod_to_irq(gpio_to_desc(gpio)); > +} > > extern int gpio_request_one(unsigned gpio, unsigned long flags, const char *label); > extern int gpio_request_array(const struct gpio *array, size_t num); > @@ -191,11 +222,24 @@ void devm_gpio_free(struct device *dev, unsigned int gpio); > * A sysfs interface can be exported by individual drivers if they want, > * but more typically is configured entirely from userspace. > */ > -extern int gpio_export(unsigned gpio, bool direction_may_change); > -extern int gpio_export_link(struct device *dev, const char *name, > - unsigned gpio); > -extern int gpio_sysfs_set_active_low(unsigned gpio, int value); > -extern void gpio_unexport(unsigned gpio); > + > +static inline int gpio_export(unsigned gpio, bool direction_may_change) > +{ > + return gpiod_export(gpio_to_desc(gpio), direction_may_change); > +} > +static inline int gpio_export_link(struct device *dev, const char *name, > + unsigned gpio) > +{ > + return gpiod_export_link(dev, name, gpio_to_desc(gpio)); > +} > +static inline int gpio_sysfs_set_active_low(unsigned gpio, int value) > +{ > + return gpiod_sysfs_set_active_low(gpio_to_desc(gpio), value); > +} > +static inline void gpio_unexport(unsigned gpio) > +{ > + gpiod_unexport(gpio_to_desc(gpio)); > +} > > #endif /* CONFIG_GPIO_SYSFS */ > > diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h > new file mode 100644 > index 0000000..483be0c > --- /dev/null > +++ b/include/linux/gpio/consumer.h > @@ -0,0 +1,45 @@ > +#ifndef __LINUX_GPIO_CONSUMER_H > +#define __LINUX_GPIO_CONSUMER_H > + > +#ifdef CONFIG_GPIOLIB > + > +struct device; > +struct gpio_chip; > + > +/** > + * Opaque descriptor for a GPIO. These are obtained and manipulated through > + * the gpiod_* API and are preferable to the old integer-based handles. > + * > + * Contrary to integers, a pointer to a gpio_desc is guaranteed to be valid > + * until the GPIO is released. > + */ > +struct gpio_desc; > + > +extern struct gpio_desc *gpio_to_desc(unsigned gpio); > +extern int gpio_chip_offset(struct gpio_desc *desc); > +extern struct gpio_chip *gpiod_to_chip(struct gpio_desc *desc); > + > +extern int gpiod_direction_input(struct gpio_desc *desc); > +extern int gpiod_direction_output(struct gpio_desc *desc, int value); > +extern int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce); > +extern int gpiod_get_value_cansleep(struct gpio_desc *desc); > +extern void gpiod_set_value_cansleep(struct gpio_desc *desc, int value); > + > +extern int gpiod_get_value(struct gpio_desc *desc); > +extern void gpiod_set_value(struct gpio_desc *desc, int value); > +extern int gpiod_cansleep(struct gpio_desc *desc); > +extern int gpiod_to_irq(struct gpio_desc *desc); > + > +#ifdef CONFIG_GPIO_SYSFS > + > +extern int gpiod_export(struct gpio_desc *desc, bool direction_may_change); > +extern int gpiod_export_link(struct device *dev, const char *name, > + struct gpio_desc *desc); > +extern int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value); > +extern void gpiod_unexport(struct gpio_desc *desc); > + > +#endif /* CONFIG_GPIO_SYSFS */ > + > +#endif /* CONFIG_GPIOLIB */ > + > +#endif > -- > 1.8.0.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > > >
Hi Guenter, On Friday 07 December 2012 10:49:47 Guenter Roeck wrote: > My own idea for a solution was to keep integer based handles, but replace > gpio_desc[] with a hash table. But ultimately I don't really care how > it is done. > > Do you have a solution for gpiochip_find_base() in mind, and how to handle > reservations ? I had thought about using bit maps, but maybe there is > a better solution. My plan so far is to use a sorted linked list of gpio_chips. Each chip contains its base address and size, so this will make it possible to find usable areas through a single parse. Current gpiochip_find_base() start from ARCH_NR_GPIOS and look backwards in the integer space to find a free range, a similar behavior can also be done if this is deemed better (GPIO numbers might become high, but since we want to hide them this should not matter). The counterpart of the list is that fetching the descriptor corresponding to a GPIO number is going to be linear instead of constant, but (1) the number of gpio_chips on the system should never grow very high and (2) this is a good incentive to use the descriptor-based API instead. :) Existing code could easily be converted - once a GPIO is acquired, its number should be converted immediatly to a descriptor and the gpiod_* functions used from them on. We can probably write a sed or Coccinelle rule to do that through the whole kernel. gpiochip_reserve() will require some more thinking using this model, but something like a dummy chip can probably be introduced in the list. It will need to be statically allocated however since memory allocation cannot be used there. Actually the only user of gpiochip_reserve() seems to be Tosa, so I wonder if there would be no way to simply get rid of it? > If/when you have some code to test, please let me know. Sure! > > + mutex_lock(&sysfs_lock); > > > > /* can't export until sysfs is available ... */ > > if (!gpio_class.p) { > > > > @@ -713,13 +743,7 @@ int gpio_export(unsigned gpio, bool > > direction_may_change)> > > return -ENOENT; > > mutex is not released here. Oops. > > - chip = gpio_to_chip(gpio); > > + chip = desc->chip; > > + gpio = gpio_chip_offset(desc); > > Might be better to use a separate variable named 'offset' when dealing with > the offset, to avoid confusion and accidential bugs. You are doing it > below, so you might as well do it everywhere. This would also simplify the > code in a couple of places where gpio is first converted into an offset > only to use "chip->base + gpio" later on. Makes sense. Fixed, thanks. > > - __func__, gpio, err); > > + __func__, offset + chip->base, err); > > I know I am nitpicking, but everywhere in the existing code it is > "chip->base + offset/gpio". Fixed. > > return chip->to_irq ? chip->to_irq(chip, gpio - chip->base) : > > -ENXIO; > > s/gpio - chip->base/gpio_chip_offset(desc)/ > > then you don't need gpio at all. Done, thanks. > > - chip = gpio_to_chip(gpio); > > + chip = desc->chip; > > + gpio = desc_to_gpio(desc); > > > > trace_gpio_value(gpio, 0, value); > > if (test_bit(FLAG_OPEN_DRAIN, &gpio_desc[gpio].flags)) > > Use desc directly. > > > - _gpio_set_open_drain_value(gpio, chip, value); > > + _gpio_set_open_drain_value(desc, value); > > > > else if (test_bit(FLAG_OPEN_SOURCE, &gpio_desc[gpio].flags)) > > Use desc directly. Right. Thanks, Alex.
On Fri, Dec 7, 2012 at 3:06 AM, Alex Courbot <acourbot@nvidia.com> wrote: > On Thursday 06 December 2012 22:42:59 Grant Likely wrote: >> how about "gpio_chip_hwnum()" to somewhat match irqdomain convention? > > Sure, matching existing interfaces is better. "git grep hwnum" does not seem > to reveal much related to irqdomain though? I think Grant is referring to the similarity with hwirq. Check: include/linux/irqdomain.h For example GPIO drivers using irqdomain often look up the trigging local GPIO pin offset from the irqdomain descriptor passed with the interrupt like this: static void foo_gpio_irq_unmask(struct irq_data *d) int offset = d->hwirq; (...) Maybe "hwgpio" is an even better choice for the name of this. Yours, Linus Walleij
On Fri, Dec 07, 2012 at 04:02:02PM +0900, Alex Courbot wrote: > Hi Guenter, > > On Friday 07 December 2012 10:49:47 Guenter Roeck wrote: > > My own idea for a solution was to keep integer based handles, but replace > > gpio_desc[] with a hash table. But ultimately I don't really care how > > it is done. > > > > Do you have a solution for gpiochip_find_base() in mind, and how to handle > > reservations ? I had thought about using bit maps, but maybe there is > > a better solution. > > My plan so far is to use a sorted linked list of gpio_chips. Each chip > contains its base address and size, so this will make it possible to find > usable areas through a single parse. Current gpiochip_find_base() start from Excellent idea. > ARCH_NR_GPIOS and look backwards in the integer space to find a free range, a > similar behavior can also be done if this is deemed better (GPIO numbers might > become high, but since we want to hide them this should not matter). > You can not completely hide them, I would guess - you'd still want to export them. Anyway, a simpler method would be to keep the list sorted in increasing order and simply search for a large enough gap. If there is none, add the new chip to the end of the list. > The counterpart of the list is that fetching the descriptor corresponding to a > GPIO number is going to be linear instead of constant, but (1) the number of > gpio_chips on the system should never grow very high and (2) this is a good > incentive to use the descriptor-based API instead. :) Existing code could > easily be converted - once a GPIO is acquired, its number should be converted > immediatly to a descriptor and the gpiod_* functions used from them on. We can > probably write a sed or Coccinelle rule to do that through the whole kernel. > Since the current approach loops through all gpio pins, it is still much better than that - the complexity would be O(chips), not O(ngpios). As you said, there won't be many chips, so that should scale for a long time. > gpiochip_reserve() will require some more thinking using this model, but > something like a dummy chip can probably be introduced in the list. It will > need to be statically allocated however since memory allocation cannot be used > there. > Introducing a dummy chip sounds like a good idea, and would have very low overhead. Thanks, Guenter
On Fri, 7 Dec 2012 11:06:34 +0900, Alex Courbot <acourbot@nvidia.com> wrote: > On Thursday 06 December 2012 22:42:59 Grant Likely wrote: > > how about "gpio_chip_hwnum()" to somewhat match irqdomain convention? > > Sure, matching existing interfaces is better. "git grep hwnum" does not seem > to reveal much related to irqdomain though? irqdomain actually uses "hwirq". "hwgpio" would be a closer analog than 'hwnum'. At first I though hwgpio to be too goofy, but on further reflection it probably has the least likelyhood of getting used incorrectly. g.
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 1c8d9e3..bf32511 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -83,6 +83,32 @@ static inline void desc_set_label(struct gpio_desc *d, const char *label) #endif } +/** + * 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 inline int desc_to_gpio(struct gpio_desc *desc) +{ + return desc - &gpio_desc[0]; +} + +/** + * Return the GPIO number of the passed descriptor relative to its chip + */ +int gpio_chip_offset(struct gpio_desc *desc) +{ + return (desc - &gpio_desc[0]) - desc->chip->base; +} + +/** + * Convert a GPIO number to its descriptor + */ +struct gpio_desc *gpio_to_desc(unsigned gpio) +{ + return &gpio_desc[gpio]; +} + /* 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 @@ -94,10 +120,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)) { @@ -116,9 +142,9 @@ static int gpio_ensure_requested(struct gpio_desc *desc, unsigned offset) } /* caller holds gpio_lock *OR* gpio is marked as requested */ -struct gpio_chip *gpio_to_chip(unsigned gpio) +struct gpio_chip *gpiod_to_chip(struct gpio_desc *desc) { - return gpio_desc[gpio].chip; + return desc->chip; } /* dynamic allocation of GPIOs, e.g. on a hotplugged device */ @@ -684,10 +710,9 @@ static struct class gpio_class = { .class_attrs = gpio_class_attrs, }; - /** - * gpio_export - export a GPIO through sysfs - * @gpio: gpio to make available, already requested + * gpiod_export - export a GPIO through sysfs + * @desc: descriptor of gpio to make available, already requested * @direction_may_change: true if userspace may change gpio direction * Context: arch_initcall or later * @@ -700,12 +725,17 @@ static struct class gpio_class = { * * Returns zero on success, else an error. */ -int gpio_export(unsigned gpio, bool direction_may_change) +int gpiod_export(struct gpio_desc *desc, bool direction_may_change) { unsigned long flags; - struct gpio_desc *desc; int status = -EINVAL; const char *ioname = NULL; + int gpio; + + if (!desc) + goto done; + + mutex_lock(&sysfs_lock); /* can't export until sysfs is available ... */ if (!gpio_class.p) { @@ -713,13 +743,7 @@ int gpio_export(unsigned gpio, bool direction_may_change) return -ENOENT; } - if (!gpio_is_valid(gpio)) - goto done; - - 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)) { status = 0; @@ -729,6 +753,7 @@ int gpio_export(unsigned gpio, bool direction_may_change) } spin_unlock_irqrestore(&gpio_lock, flags); + gpio = desc_to_gpio(desc); if (desc->chip->names && desc->chip->names[gpio - desc->chip->base]) ioname = desc->chip->names[gpio - desc->chip->base]; @@ -768,7 +793,7 @@ done: return status; } -EXPORT_SYMBOL_GPL(gpio_export); +EXPORT_SYMBOL_GPL(gpiod_export); static int match_export(struct device *dev, void *data) { @@ -776,28 +801,26 @@ static int match_export(struct device *dev, void *data) } /** - * gpio_export_link - create a sysfs link to an exported GPIO node + * gpiod_export_link - create a sysfs link to an exported GPIO node * @dev: device under which to create symlink * @name: name of the symlink - * @gpio: gpio to create symlink to, already exported + * @desc: descriptor of gpio to create symlink to, already exported * * Set up a symlink from /sys/.../dev/name to /sys/class/gpio/gpioN * node. Caller is responsible for unlinking. * * Returns zero on success, else an error. */ -int gpio_export_link(struct device *dev, const char *name, unsigned gpio) +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; @@ -814,16 +837,16 @@ 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); - +EXPORT_SYMBOL_GPL(gpiod_export_link); /** - * gpio_sysfs_set_active_low - set the polarity of gpio sysfs value - * @gpio: gpio to change + * gpiod_sysfs_set_active_low - set the polarity of gpio sysfs value + * @desc: descriptor of gpio to change * @value: non-zero to use active low, i.e. inverted values * * Set the polarity of /sys/class/gpio/gpioN/value sysfs attribute. @@ -833,19 +856,16 @@ EXPORT_SYMBOL_GPL(gpio_export_link); * * Returns zero on success, else an error. */ -int gpio_sysfs_set_active_low(unsigned gpio, int value) +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) { @@ -861,33 +881,31 @@ 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; } -EXPORT_SYMBOL_GPL(gpio_sysfs_set_active_low); +EXPORT_SYMBOL_GPL(gpiod_sysfs_set_active_low); /** - * gpio_unexport - reverse effect of gpio_export() - * @gpio: gpio to make unavailable + * gpiod_unexport - reverse effect of gpio_export() + * @desc: descriptor of gpio to make unavailable * * This is implicit on gpio_free(). */ -void gpio_unexport(unsigned gpio) +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); @@ -899,15 +917,17 @@ 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); } -EXPORT_SYMBOL_GPL(gpio_unexport); +EXPORT_SYMBOL_GPL(gpiod_unexport); static int gpiochip_export(struct gpio_chip *chip) { @@ -1386,7 +1406,6 @@ const char *gpiochip_is_requested(struct gpio_chip *chip, unsigned offset) } EXPORT_SYMBOL_GPL(gpiochip_is_requested); - /* Drivers MUST set GPIO direction before making get/set calls. In * some cases this is done in early boot, before IRQs are enabled. * @@ -1396,24 +1415,19 @@ EXPORT_SYMBOL_GPL(gpiochip_is_requested); * rely on gpio_request() having been called beforehand. */ -int gpio_direction_input(unsigned gpio) +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 gpio; 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; @@ -1423,6 +1437,8 @@ int gpio_direction_input(unsigned gpio) might_sleep_if(chip->can_sleep); + gpio = gpio_chip_offset(desc); + if (status) { status = chip->request(chip, gpio); if (status < 0) { @@ -1449,34 +1465,31 @@ fail: __func__, gpio, status); return status; } -EXPORT_SYMBOL_GPL(gpio_direction_input); +EXPORT_SYMBOL_GPL(gpiod_direction_input); -int gpio_direction_output(unsigned gpio, int value) +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 gpio; /* 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; @@ -1485,6 +1498,7 @@ int gpio_direction_output(unsigned gpio, int value) spin_unlock_irqrestore(&gpio_lock, flags); might_sleep_if(chip->can_sleep); + gpio = gpio_chip_offset(desc); if (status) { status = chip->request(chip, gpio); @@ -1515,28 +1529,26 @@ fail: EXPORT_SYMBOL_GPL(gpio_direction_output); /** - * gpio_set_debounce - sets @debounce time for a @gpio - * @gpio: the gpio to set debounce time + * gpiod_set_debounce - sets @debounce time for a gpio + * @desc: descriptor to the gpio to set debounce time * @debounce: debounce time is microseconds */ -int gpio_set_debounce(unsigned gpio, unsigned debounce) +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 gpio; 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; @@ -1546,6 +1558,7 @@ int gpio_set_debounce(unsigned gpio, unsigned debounce) might_sleep_if(chip->can_sleep); + gpio = gpio_chip_offset(desc); return chip->set_debounce(chip, gpio, debounce); fail: @@ -1556,7 +1569,7 @@ fail: return status; } -EXPORT_SYMBOL_GPL(gpio_set_debounce); +EXPORT_SYMBOL_GPL(gpiod_set_debounce); /* I/O calls are only valid after configuration completed; the relevant * "is this a valid GPIO" error checks should already have been done. @@ -1581,27 +1594,28 @@ EXPORT_SYMBOL_GPL(gpio_set_debounce); */ /** - * __gpio_get_value() - return a gpio's value - * @gpio: gpio whose value will be returned + * gpiod_get_value() - return a gpio's value + * @desc: descriptor of gpio whose value will be returned * Context: any * - * This is used directly or indirectly to implement gpio_get_value(). - * It returns the zero or nonzero value provided by the associated + * 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) +int gpiod_get_value(struct gpio_desc *desc) { struct gpio_chip *chip; int value; + int gpio; - chip = gpio_to_chip(gpio); + chip = desc->chip; + gpio = gpio_chip_offset(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, gpio) : 0; + trace_gpio_value(gpio + chip->base, 1, value); return value; } -EXPORT_SYMBOL_GPL(__gpio_get_value); +EXPORT_SYMBOL_GPL(gpiod_get_value); /* * _gpio_set_open_drain_value() - Set the open drain gpio's value. @@ -1609,23 +1623,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_offset(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(offset + chip->base, value, err); if (err < 0) pr_err("%s: Error in set_value for open drain gpio%d err %d\n", - __func__, gpio, err); + __func__, offset + chip->base, err); } /* @@ -1634,124 +1650,120 @@ 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_offset(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(offset + chip->base, !value, err); if (err < 0) pr_err("%s: Error in set_value for open source gpio%d err %d\n", - __func__, gpio, err); + __func__, offset + chip->base, err); } - /** - * __gpio_set_value() - assign a gpio's value - * @gpio: gpio whose value will be assigned + * gpiod_set_value() - assign a gpio's value + * @desc: descriptor of gpio whose value will be assigned * @value: value to assign * Context: any * - * This is used directly or indirectly to implement gpio_set_value(). - * It invokes the associated gpio_chip.set() method. + * Invokes the associated gpio_chip.set() method. */ -void __gpio_set_value(unsigned gpio, int value) +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_offset(desc), value); } -EXPORT_SYMBOL_GPL(__gpio_set_value); +EXPORT_SYMBOL_GPL(gpiod_set_value); /** - * __gpio_cansleep() - report whether gpio value access will sleep - * @gpio: gpio in question + * gpiod_cansleep() - report whether gpio value access will sleep + * @desc: descriptor of gpio in question * Context: any * - * This is used directly or indirectly to implement gpio_cansleep(). It - * returns nonzero if access reading or writing the GPIO value can sleep. + * Returns nonzero if access reading or writing the GPIO value can sleep. */ -int __gpio_cansleep(unsigned gpio) +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 chip->can_sleep; + return desc->chip->can_sleep; } -EXPORT_SYMBOL_GPL(__gpio_cansleep); +EXPORT_SYMBOL_GPL(gpiod_cansleep); /** - * __gpio_to_irq() - return the IRQ corresponding to a GPIO - * @gpio: gpio whose IRQ will be returned (already requested) + * gpiod_to_irq() - return the IRQ corresponding to a GPIO + * @desc: descriptor of gpio whose IRQ will be returned (already requested) * Context: any * - * This is used directly or indirectly to implement gpio_to_irq(). - * It returns the number of the IRQ signaled by this (input) GPIO, + * Returns the number of the IRQ signaled by this (input) GPIO, * or a negative errno. */ -int __gpio_to_irq(unsigned gpio) +int gpiod_to_irq(struct gpio_desc *desc) { struct gpio_chip *chip; + int gpio; - chip = gpio_to_chip(gpio); + chip = desc->chip; + gpio = desc_to_gpio(desc); return chip->to_irq ? chip->to_irq(chip, gpio - chip->base) : -ENXIO; } -EXPORT_SYMBOL_GPL(__gpio_to_irq); - - +EXPORT_SYMBOL_GPL(gpiod_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) +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_offset(desc); + value = chip->get ? chip->get(chip, offset) : 0; + trace_gpio_value(offset + chip->base, 1, value); return value; } -EXPORT_SYMBOL_GPL(gpio_get_value_cansleep); +EXPORT_SYMBOL_GPL(gpiod_get_value_cansleep); -void gpio_set_value_cansleep(unsigned gpio, int value) +void gpiod_set_value_cansleep(struct gpio_desc *desc, int value) { struct gpio_chip *chip; + int gpio; might_sleep_if(extra_checks); - chip = gpio_to_chip(gpio); + chip = desc->chip; + gpio = desc_to_gpio(desc); trace_gpio_value(gpio, 0, value); if (test_bit(FLAG_OPEN_DRAIN, &gpio_desc[gpio].flags)) - _gpio_set_open_drain_value(gpio, chip, value); + _gpio_set_open_drain_value(desc, value); else if (test_bit(FLAG_OPEN_SOURCE, &gpio_desc[gpio].flags)) - _gpio_set_open_source_value(gpio, chip, value); + _gpio_set_open_source_value(desc, value); else chip->set(chip, gpio - chip->base, value); } -EXPORT_SYMBOL_GPL(gpio_set_value_cansleep); - +EXPORT_SYMBOL_GPL(gpiod_set_value_cansleep); #ifdef CONFIG_DEBUG_FS diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h index a9432fc..30e1a91 100644 --- a/include/asm-generic/gpio.h +++ b/include/asm-generic/gpio.h @@ -9,6 +9,7 @@ #ifdef CONFIG_GPIOLIB #include <linux/compiler.h> +#include <linux/gpio/consumer.h> /* Platforms may implement their GPIO interface with library code, * at a small performance cost for non-inlined operations and some @@ -138,7 +139,10 @@ struct gpio_chip { extern const char *gpiochip_is_requested(struct gpio_chip *chip, unsigned offset); -extern struct gpio_chip *gpio_to_chip(unsigned gpio); +static inline struct gpio_chip *gpio_to_chip(unsigned gpio) +{ + return gpiod_to_chip(gpio_to_desc(gpio)); +} extern int __must_check gpiochip_reserve(int start, int ngpio); /* add/remove chips */ @@ -155,25 +159,52 @@ extern struct gpio_chip *gpiochip_find(void *data, extern int gpio_request(unsigned gpio, const char *label); extern void gpio_free(unsigned gpio); -extern int gpio_direction_input(unsigned gpio); -extern int gpio_direction_output(unsigned gpio, int value); +static inline int gpio_direction_input(unsigned gpio) +{ + return gpiod_direction_input(gpio_to_desc(gpio)); +} +static inline int gpio_direction_output(unsigned gpio, int value) +{ + return gpiod_direction_output(gpio_to_desc(gpio), value); +} -extern int gpio_set_debounce(unsigned gpio, unsigned debounce); +static inline int gpio_set_debounce(unsigned gpio, unsigned debounce) +{ + return gpiod_set_debounce(gpio_to_desc(gpio), debounce); +} -extern int gpio_get_value_cansleep(unsigned gpio); -extern void gpio_set_value_cansleep(unsigned gpio, int value); +static inline int gpio_get_value_cansleep(unsigned gpio) +{ + return gpiod_get_value_cansleep(gpio_to_desc(gpio)); +} +static inline void gpio_set_value_cansleep(unsigned gpio, int value) +{ + return gpiod_set_value_cansleep(gpio_to_desc(gpio), value); +} /* A platform's <asm/gpio.h> code may want to inline the I/O calls when * the GPIO is constant and refers to some always-present controller, * giving direct access to chip registers and tight bitbanging loops. */ -extern int __gpio_get_value(unsigned gpio); -extern void __gpio_set_value(unsigned gpio, int value); +static inline int __gpio_get_value(unsigned gpio) +{ + return gpiod_get_value(gpio_to_desc(gpio)); +} +static inline void __gpio_set_value(unsigned gpio, int value) +{ + gpiod_set_value(gpio_to_desc(gpio), value); +} -extern int __gpio_cansleep(unsigned gpio); +static inline int __gpio_cansleep(unsigned gpio) +{ + return gpiod_cansleep(gpio_to_desc(gpio)); +} -extern int __gpio_to_irq(unsigned gpio); +static inline int __gpio_to_irq(unsigned gpio) +{ + return gpiod_to_irq(gpio_to_desc(gpio)); +} extern int gpio_request_one(unsigned gpio, unsigned long flags, const char *label); extern int gpio_request_array(const struct gpio *array, size_t num); @@ -191,11 +222,24 @@ void devm_gpio_free(struct device *dev, unsigned int gpio); * A sysfs interface can be exported by individual drivers if they want, * but more typically is configured entirely from userspace. */ -extern int gpio_export(unsigned gpio, bool direction_may_change); -extern int gpio_export_link(struct device *dev, const char *name, - unsigned gpio); -extern int gpio_sysfs_set_active_low(unsigned gpio, int value); -extern void gpio_unexport(unsigned gpio); + +static inline int gpio_export(unsigned gpio, bool direction_may_change) +{ + return gpiod_export(gpio_to_desc(gpio), direction_may_change); +} +static inline int gpio_export_link(struct device *dev, const char *name, + unsigned gpio) +{ + return gpiod_export_link(dev, name, gpio_to_desc(gpio)); +} +static inline int gpio_sysfs_set_active_low(unsigned gpio, int value) +{ + return gpiod_sysfs_set_active_low(gpio_to_desc(gpio), value); +} +static inline void gpio_unexport(unsigned gpio) +{ + gpiod_unexport(gpio_to_desc(gpio)); +} #endif /* CONFIG_GPIO_SYSFS */ diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h new file mode 100644 index 0000000..483be0c --- /dev/null +++ b/include/linux/gpio/consumer.h @@ -0,0 +1,45 @@ +#ifndef __LINUX_GPIO_CONSUMER_H +#define __LINUX_GPIO_CONSUMER_H + +#ifdef CONFIG_GPIOLIB + +struct device; +struct gpio_chip; + +/** + * Opaque descriptor for a GPIO. These are obtained and manipulated through + * the gpiod_* API and are preferable to the old integer-based handles. + * + * Contrary to integers, a pointer to a gpio_desc is guaranteed to be valid + * until the GPIO is released. + */ +struct gpio_desc; + +extern struct gpio_desc *gpio_to_desc(unsigned gpio); +extern int gpio_chip_offset(struct gpio_desc *desc); +extern struct gpio_chip *gpiod_to_chip(struct gpio_desc *desc); + +extern int gpiod_direction_input(struct gpio_desc *desc); +extern int gpiod_direction_output(struct gpio_desc *desc, int value); +extern int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce); +extern int gpiod_get_value_cansleep(struct gpio_desc *desc); +extern void gpiod_set_value_cansleep(struct gpio_desc *desc, int value); + +extern int gpiod_get_value(struct gpio_desc *desc); +extern void gpiod_set_value(struct gpio_desc *desc, int value); +extern int gpiod_cansleep(struct gpio_desc *desc); +extern int gpiod_to_irq(struct gpio_desc *desc); + +#ifdef CONFIG_GPIO_SYSFS + +extern int gpiod_export(struct gpio_desc *desc, bool direction_may_change); +extern int gpiod_export_link(struct device *dev, const char *name, + struct gpio_desc *desc); +extern int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value); +extern void gpiod_unexport(struct gpio_desc *desc); + +#endif /* CONFIG_GPIO_SYSFS */ + +#endif /* CONFIG_GPIOLIB */ + +#endif
With the current API, GPIOs are manipulated through an integer which represents their unique number across the system. This poses problems in terms of portability, scalability and flexibility: for instance, the number of valid GPIOs for a given system is fixed at system time, and a large array of that size is statically allocated to hold the GPIO descriptors. Worse, GPIOs can be used without being properly allocated. In order to improve the situation, the integer namespace must first get away. This patch introduces an alternative GPIO API that uses opaque handlers and refactor gpiolib's internals to work with these handlers instead of GPIO numbers. The former integer-based API is still available as a light wrapper around this new API. This first step will then us to build more improvements for gpiolib, like proper GPIO lookup functions per device and provider, and getting rid of the static GPIO array and the ARCH_NR_GPIO configuration option. Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> --- drivers/gpio/gpiolib.c | 302 ++++++++++++++++++++++-------------------- include/asm-generic/gpio.h | 74 ++++++++--- include/linux/gpio/consumer.h | 45 +++++++ 3 files changed, 261 insertions(+), 160 deletions(-) create mode 100644 include/linux/gpio/consumer.h