Still in order to prepare for the ability to share one GPIO between
several consumers, this series of mostly unrelated patches fixes a
few minor issues. Most of the patches should be no-brainers ; maybe
patch 2 should be looked more closely in order to understand why this
code was there in the first place. Patch 4 is not only a simplification
of the API, but a hard requirement if we are to allow several GPIO
descriptors to manipulate the same GPIO, as no driver function should
require a descriptor to perform properly.
This series has been tested on Raspberry Pi and Jetson TK1 without any
problem being noticed.
Alexandre Courbot (5):
gpio: remove export of private of_get_named_gpio_flags()
gpio: simplify gpiochip_export()
gpio: make gpiochip_get_desc() gpiolib-private
gpio: remove gpiod_lock/unlock_as_irq()
gpio: move gpio_ensure_requested() into legacy C file
Documentation/gpio/driver.txt | 4 +-
drivers/gpio/gpiolib-acpi.c | 6 +-
drivers/gpio/gpiolib-legacy.c | 106 ++++++++++++++++++++++++++--
drivers/gpio/gpiolib-of.c | 3 +-
drivers/gpio/gpiolib-sysfs.c | 24 +++----
drivers/gpio/gpiolib.c | 160 ++++++------------------------------------
drivers/gpio/gpiolib.h | 2 +
include/asm-generic/gpio.h | 18 +----
include/linux/gpio/driver.h | 7 +-
9 files changed, 144 insertions(+), 186 deletions(-)
--
2.0.1
of_get_named_gpio_flags() has been made gpiolib-private by commit
f01d907582, but its EXPORT statement has not been removed. Fix this.
Signed-off-by: Alexandre Courbot <[email protected]>
---
drivers/gpio/gpiolib-of.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index e60cdab1d15e..3e2fae205bee 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -95,7 +95,6 @@ struct gpio_desc *of_get_named_gpiod_flags(struct device_node *np,
PTR_ERR_OR_ZERO(gg_data.out_gpio));
return gg_data.out_gpio;
}
-EXPORT_SYMBOL(of_get_named_gpiod_flags);
int of_get_named_gpio_flags(struct device_node *np, const char *list_name,
int index, enum of_gpio_flags *flags)
--
2.0.1
As GPIO descriptors are not going to remain unique anymore, having this
function public is not safe. Restrain its use to gpiolib since we have
no user outside of it.
Signed-off-by: Alexandre Courbot <[email protected]>
---
drivers/gpio/gpiolib-of.c | 2 +-
drivers/gpio/gpiolib.c | 1 -
drivers/gpio/gpiolib.h | 2 ++
include/linux/gpio/driver.h | 3 ---
4 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 3e2fae205bee..7cfdc2278905 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -23,7 +23,7 @@
#include <linux/pinctrl/pinctrl.h>
#include <linux/slab.h>
-struct gpio_desc;
+#include "gpiolib.h"
/* Private data structure for of_gpiochip_find_and_xlate */
struct gg_data {
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index c5509359ba88..38d176e31379 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -82,7 +82,6 @@ struct gpio_desc *gpiochip_get_desc(struct gpio_chip *chip,
return &chip->desc[hwnum];
}
-EXPORT_SYMBOL_GPL(gpiochip_get_desc);
/**
* Convert a GPIO descriptor to the integer namespace.
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 98020c393eb3..acbb9335f08c 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -51,6 +51,8 @@ void gpiochip_free_own_desc(struct gpio_desc *desc);
struct gpio_desc *of_get_named_gpiod_flags(struct device_node *np,
const char *list_name, int index, enum of_gpio_flags *flags);
+struct gpio_desc *gpiochip_get_desc(struct gpio_chip *chip, u16 hwnum);
+
extern struct spinlock gpio_lock;
extern struct list_head gpio_chips;
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 573e4f3243d0..4dc79714c136 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -151,9 +151,6 @@ void gpiod_unlock_as_irq(struct gpio_desc *desc);
struct gpio_chip *gpiod_to_chip(const struct gpio_desc *desc);
-struct gpio_desc *gpiochip_get_desc(struct gpio_chip *chip,
- u16 hwnum);
-
enum gpio_lookup_flags {
GPIO_ACTIVE_HIGH = (0 << 0),
GPIO_ACTIVE_LOW = (1 << 0),
--
2.0.1
For some reason gpiochip_export() would invalidate all the descriptors
of a chip if exporting it to sysfs failed. This does not appear as
necessary. Remove that part of the code.
While we are at it, add a note about the non-safety of temporarily
releasing a spinlock in the middle of the loop that protects its
iterator, and explain why this is done.
Signed-off-by: Alexandre Courbot <[email protected]>
---
drivers/gpio/gpiolib-sysfs.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 3516502059f2..f150aa288fa1 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -760,18 +760,8 @@ int gpiochip_export(struct gpio_chip *chip)
chip->exported = (status == 0);
mutex_unlock(&sysfs_lock);
- if (status) {
- unsigned long flags;
- unsigned gpio;
-
- spin_lock_irqsave(&gpio_lock, flags);
- gpio = 0;
- while (gpio < chip->ngpio)
- chip->desc[gpio++].chip = NULL;
- spin_unlock_irqrestore(&gpio_lock, flags);
-
+ if (status)
chip_dbg(chip, "%s: status %d\n", __func__, status);
- }
return status;
}
@@ -817,6 +807,14 @@ static int __init gpiolib_sysfs_init(void)
if (!chip || chip->exported)
continue;
+ /*
+ * TODO we yield gpio_lock here because gpiochip_export()
+ * acquires a mutex. This is unsafe and needs to be fixed.
+ *
+ * Also it would be nice to use gpiochip_find() here so we
+ * can keep gpio_chips local to gpiolib.c, but the yield of
+ * gpio_lock prevents us from doing this.
+ */
spin_unlock_irqrestore(&gpio_lock, flags);
status = gpiochip_export(chip);
spin_lock_irqsave(&gpio_lock, flags);
--
2.0.1
gpio_lock/unlock_as_irq() are working with (chip, offset) arguments and
are thus not using the old integer namespace. Therefore, there is no
reason to have gpiod variants of these functions working with
descriptors, especially since the (chip, offset) tuple is more suitable
to the users of these functions (GPIO drivers, whereas GPIO descriptors
are targeted at GPIO consumers).
Signed-off-by: Alexandre Courbot <[email protected]>
---
Documentation/gpio/driver.txt | 4 ++--
drivers/gpio/gpiolib-acpi.c | 6 +++---
drivers/gpio/gpiolib-legacy.c | 12 ------------
drivers/gpio/gpiolib-sysfs.c | 4 ++--
drivers/gpio/gpiolib.c | 30 ++++++++++++++++--------------
include/asm-generic/gpio.h | 3 ---
include/linux/gpio/driver.h | 4 ++--
7 files changed, 25 insertions(+), 38 deletions(-)
diff --git a/Documentation/gpio/driver.txt b/Documentation/gpio/driver.txt
index fa9a0a8b3734..224dbbcd1804 100644
--- a/Documentation/gpio/driver.txt
+++ b/Documentation/gpio/driver.txt
@@ -157,12 +157,12 @@ Locking IRQ usage
Input GPIOs can be used as IRQ signals. When this happens, a driver is requested
to mark the GPIO as being used as an IRQ:
- int gpiod_lock_as_irq(struct gpio_desc *desc)
+ int gpio_lock_as_irq(struct gpio_chip *chip, unsigned int offset)
This will prevent the use of non-irq related GPIO APIs until the GPIO IRQ lock
is released:
- void gpiod_unlock_as_irq(struct gpio_desc *desc)
+ void gpio_unlock_as_irq(struct gpio_chip *chip, unsigned int offset)
When implementing an irqchip inside a GPIO driver, these two functions should
typically be called in the .startup() and .shutdown() callbacks from the
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 4a987917c186..d2e8600df02c 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -157,7 +157,7 @@ static acpi_status acpi_gpiochip_request_interrupt(struct acpi_resource *ares,
gpiod_direction_input(desc);
- ret = gpiod_lock_as_irq(desc);
+ ret = gpio_lock_as_irq(chip, pin);
if (ret) {
dev_err(chip->dev, "Failed to lock GPIO as interrupt\n");
goto fail_free_desc;
@@ -212,7 +212,7 @@ static acpi_status acpi_gpiochip_request_interrupt(struct acpi_resource *ares,
fail_free_event:
kfree(event);
fail_unlock_irq:
- gpiod_unlock_as_irq(desc);
+ gpio_unlock_as_irq(chip, pin);
fail_free_desc:
gpiochip_free_own_desc(desc);
@@ -263,7 +263,7 @@ static void acpi_gpiochip_free_interrupts(struct acpi_gpio_chip *acpi_gpio)
desc = gpiochip_get_desc(chip, event->pin);
if (WARN_ON(IS_ERR(desc)))
continue;
- gpiod_unlock_as_irq(desc);
+ gpio_unlock_as_irq(chip, event->pin);
gpiochip_free_own_desc(desc);
list_del(&event->node);
kfree(event);
diff --git a/drivers/gpio/gpiolib-legacy.c b/drivers/gpio/gpiolib-legacy.c
index eb5a4e2cee85..c3e3823a40b9 100644
--- a/drivers/gpio/gpiolib-legacy.c
+++ b/drivers/gpio/gpiolib-legacy.c
@@ -97,15 +97,3 @@ void gpio_free_array(const struct gpio *array, size_t num)
gpio_free((array++)->gpio);
}
EXPORT_SYMBOL_GPL(gpio_free_array);
-
-int gpio_lock_as_irq(struct gpio_chip *chip, unsigned int offset)
-{
- return gpiod_lock_as_irq(gpiochip_get_desc(chip, offset));
-}
-EXPORT_SYMBOL_GPL(gpio_lock_as_irq);
-
-void gpio_unlock_as_irq(struct gpio_chip *chip, unsigned int offset)
-{
- return gpiod_unlock_as_irq(gpiochip_get_desc(chip, offset));
-}
-EXPORT_SYMBOL_GPL(gpio_unlock_as_irq);
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index f150aa288fa1..be45a9283c28 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -161,7 +161,7 @@ static int gpio_setup_irq(struct gpio_desc *desc, struct device *dev,
desc->flags &= ~GPIO_TRIGGER_MASK;
if (!gpio_flags) {
- gpiod_unlock_as_irq(desc);
+ gpio_unlock_as_irq(desc->chip, gpio_chip_hwgpio(desc));
ret = 0;
goto free_id;
}
@@ -200,7 +200,7 @@ static int gpio_setup_irq(struct gpio_desc *desc, struct device *dev,
if (ret < 0)
goto free_id;
- ret = gpiod_lock_as_irq(desc);
+ ret = gpio_lock_as_irq(desc->chip, gpio_chip_hwgpio(desc));
if (ret < 0) {
gpiod_warn(desc, "failed to flag the GPIO for IRQ\n");
goto free_id;
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 38d176e31379..7582207c92e7 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1428,44 +1428,46 @@ int gpiod_to_irq(const struct gpio_desc *desc)
EXPORT_SYMBOL_GPL(gpiod_to_irq);
/**
- * gpiod_lock_as_irq() - lock a GPIO to be used as IRQ
- * @gpio: the GPIO line to lock as used for IRQ
+ * gpio_lock_as_irq() - lock a GPIO to be used as IRQ
+ * @chip: the chip the GPIO to lock belongs to
+ * @offset: the offset of the GPIO to lock as IRQ
*
* This is used directly by GPIO drivers that want to lock down
* a certain GPIO line to be used for IRQs.
*/
-int gpiod_lock_as_irq(struct gpio_desc *desc)
+int gpio_lock_as_irq(struct gpio_chip *chip, unsigned int offset)
{
- if (!desc)
+ if (offset >= chip->ngpio)
return -EINVAL;
- if (test_bit(FLAG_IS_OUT, &desc->flags)) {
- gpiod_err(desc,
+ if (test_bit(FLAG_IS_OUT, &chip->desc[offset].flags)) {
+ chip_err(chip,
"%s: tried to flag a GPIO set as output for IRQ\n",
__func__);
return -EIO;
}
- set_bit(FLAG_USED_AS_IRQ, &desc->flags);
+ set_bit(FLAG_USED_AS_IRQ, &chip->desc[offset].flags);
return 0;
}
-EXPORT_SYMBOL_GPL(gpiod_lock_as_irq);
+EXPORT_SYMBOL_GPL(gpio_lock_as_irq);
/**
- * gpiod_unlock_as_irq() - unlock a GPIO used as IRQ
- * @gpio: the GPIO line to unlock from IRQ usage
+ * gpio_unlock_as_irq() - unlock a GPIO used as IRQ
+ * @chip: the chip the GPIO to lock belongs to
+ * @offset: the offset of the GPIO to lock as IRQ
*
* This is used directly by GPIO drivers that want to indicate
* that a certain GPIO is no longer used exclusively for IRQ.
*/
-void gpiod_unlock_as_irq(struct gpio_desc *desc)
+void gpio_unlock_as_irq(struct gpio_chip *chip, unsigned int offset)
{
- if (!desc)
+ if (offset >= chip->ngpio)
return;
- clear_bit(FLAG_USED_AS_IRQ, &desc->flags);
+ clear_bit(FLAG_USED_AS_IRQ, &chip->desc[offset].flags);
}
-EXPORT_SYMBOL_GPL(gpiod_unlock_as_irq);
+EXPORT_SYMBOL_GPL(gpio_unlock_as_irq);
/**
* gpiod_get_raw_value_cansleep() - return a gpio's raw value
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index 23e364538ab5..c1d4105e1c1d 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -110,9 +110,6 @@ static inline int __gpio_to_irq(unsigned gpio)
return gpiod_to_irq(gpio_to_desc(gpio));
}
-extern int gpio_lock_as_irq(struct gpio_chip *chip, unsigned int offset);
-extern void gpio_unlock_as_irq(struct gpio_chip *chip, unsigned int offset);
-
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);
extern void gpio_free_array(const struct gpio *array, size_t num);
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 4dc79714c136..7b8b00235d29 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -146,8 +146,8 @@ extern struct gpio_chip *gpiochip_find(void *data,
int (*match)(struct gpio_chip *chip, void *data));
/* lock/unlock as IRQ */
-int gpiod_lock_as_irq(struct gpio_desc *desc);
-void gpiod_unlock_as_irq(struct gpio_desc *desc);
+int gpio_lock_as_irq(struct gpio_chip *chip, unsigned int offset);
+void gpio_unlock_as_irq(struct gpio_chip *chip, unsigned int offset);
struct gpio_chip *gpiod_to_chip(const struct gpio_desc *desc);
--
2.0.1
gpio_ensure_requested() only makes sense when using the integer-based
GPIO API, so make sure it is called from there instead of the gpiod
API which we know cannot be called with a non-requested GPIO anyway.
The uses of gpio_ensure_requested() in the gpiod API were kind of
out-of-place anyway, so putting them in gpio-legacy.c helps clearing the
code.
Actually, considering the time this ensure_requested mechanism has been
around, maybe we should just turn this patch into "remove
gpio_ensure_requested()" if we know for sure that no user depend on it
anymore?
Signed-off-by: Alexandre Courbot <[email protected]>
---
drivers/gpio/gpiolib-legacy.c | 106 ++++++++++++++++++++++++++++++++++
drivers/gpio/gpiolib.c | 129 ++----------------------------------------
include/asm-generic/gpio.h | 15 +----
3 files changed, 113 insertions(+), 137 deletions(-)
diff --git a/drivers/gpio/gpiolib-legacy.c b/drivers/gpio/gpiolib-legacy.c
index c3e3823a40b9..9da5deee716c 100644
--- a/drivers/gpio/gpiolib-legacy.c
+++ b/drivers/gpio/gpiolib-legacy.c
@@ -5,6 +5,64 @@
#include "gpiolib.h"
+/* 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
+ * those calls have no teeth) we can't avoid autorequesting. This nag
+ * message should motivate switching to explicit requests... so should
+ * the weaker cleanup after faults, compared to gpio_request().
+ *
+ * NOTE: the autorequest mechanism is going away; at this point it's
+ * 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)
+{
+ struct gpio_chip *chip = desc->chip;
+ unsigned long flags;
+ bool request = false;
+ int err = 0;
+
+ spin_lock_irqsave(&gpio_lock, flags);
+
+ if (WARN(test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0,
+ "autorequest GPIO-%d\n", desc_to_gpio(desc))) {
+ if (!try_module_get(chip->owner)) {
+ gpiod_err(desc, "%s: module can't be gotten\n",
+ __func__);
+ clear_bit(FLAG_REQUESTED, &desc->flags);
+ /* lose */
+ err = -EIO;
+ goto end;
+ }
+ desc->label = "[auto]";
+ /* caller must chip->request() w/o spinlock */
+ if (chip->request)
+ request = true;
+ }
+
+end:
+ spin_unlock_irqrestore(&gpio_lock, flags);
+
+ if (request) {
+ might_sleep_if(chip->can_sleep);
+ err = chip->request(chip, gpio_chip_hwgpio(desc));
+
+ if (err < 0) {
+ gpiod_dbg(desc, "%s: chip request fail, %d\n",
+ __func__, err);
+ spin_lock_irqsave(&gpio_lock, flags);
+
+ desc->label = NULL;
+ clear_bit(FLAG_REQUESTED, &desc->flags);
+
+ spin_unlock_irqrestore(&gpio_lock, flags);
+ }
+ }
+
+ return err;
+}
+
void gpio_free(unsigned gpio)
{
gpiod_free(gpio_to_desc(gpio));
@@ -97,3 +155,51 @@ void gpio_free_array(const struct gpio *array, size_t num)
gpio_free((array++)->gpio);
}
EXPORT_SYMBOL_GPL(gpio_free_array);
+
+int gpio_direction_input(unsigned gpio)
+{
+ struct gpio_desc *desc = gpio_to_desc(gpio);
+ int err;
+
+ if (!desc)
+ return -EINVAL;
+
+ err = gpio_ensure_requested(desc);
+ if (err < 0)
+ return err;
+
+ return gpiod_direction_input(desc);
+}
+EXPORT_SYMBOL_GPL(gpio_direction_input);
+
+int gpio_direction_output(unsigned gpio, int value)
+{
+ struct gpio_desc *desc = gpio_to_desc(gpio);
+ int err;
+
+ if (!desc)
+ return -EINVAL;
+
+ err = gpio_ensure_requested(desc);
+ if (err < 0)
+ return err;
+
+ return gpiod_direction_output_raw(desc, value);
+}
+EXPORT_SYMBOL_GPL(gpio_direction_output);
+
+int gpio_set_debounce(unsigned gpio, unsigned debounce)
+{
+ struct gpio_desc *desc = gpio_to_desc(gpio);
+ int err;
+
+ if (!desc)
+ return -EINVAL;
+
+ err = gpio_ensure_requested(desc);
+ if (err < 0)
+ return err;
+
+ return gpiod_set_debounce(desc, debounce);
+}
+EXPORT_SYMBOL_GPL(gpio_set_debounce);
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 7582207c92e7..412d64e93cfb 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -95,39 +95,6 @@ int desc_to_gpio(const struct gpio_desc *desc)
EXPORT_SYMBOL_GPL(desc_to_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
- * those calls have no teeth) we can't avoid autorequesting. This nag
- * message should motivate switching to explicit requests... so should
- * the weaker cleanup after faults, compared to gpio_request().
- *
- * NOTE: the autorequest mechanism is going away; at this point it's
- * 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)
-{
- const struct gpio_chip *chip = desc->chip;
- const int gpio = desc_to_gpio(desc);
-
- if (WARN(test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0,
- "autorequest GPIO-%d\n", gpio)) {
- if (!try_module_get(chip->owner)) {
- gpiod_err(desc, "%s: module can't be gotten\n",
- __func__);
- clear_bit(FLAG_REQUESTED, &desc->flags);
- /* lose */
- return -EIO;
- }
- desc_set_label(desc, "[auto]");
- /* caller must chip->request() w/o spinlock */
- if (chip->request)
- return 1;
- }
- return 0;
-}
-
/**
* gpiod_to_chip - Return the GPIO chip to which a GPIO descriptor belongs
* @desc: descriptor to return the chip of
@@ -964,10 +931,8 @@ void gpiochip_free_own_desc(struct gpio_desc *desc)
*/
int gpiod_direction_input(struct gpio_desc *desc)
{
- unsigned long flags;
struct gpio_chip *chip;
int status = -EINVAL;
- int offset;
if (!desc || !desc->chip) {
pr_warn("%s: invalid GPIO\n", __func__);
@@ -982,52 +947,20 @@ int gpiod_direction_input(struct gpio_desc *desc)
return -EIO;
}
- spin_lock_irqsave(&gpio_lock, flags);
-
- status = gpio_ensure_requested(desc);
- if (status < 0)
- goto fail;
-
- /* now we know the gpio is valid and chip won't vanish */
-
- spin_unlock_irqrestore(&gpio_lock, flags);
-
- might_sleep_if(chip->can_sleep);
-
- offset = gpio_chip_hwgpio(desc);
- if (status) {
- status = chip->request(chip, offset);
- if (status < 0) {
- gpiod_dbg(desc, "%s: chip request fail, %d\n",
- __func__, status);
- /* and it's not available to anyone else ...
- * gpio_request() is the fully clean solution.
- */
- goto lose;
- }
- }
-
- status = chip->direction_input(chip, offset);
+ status = chip->direction_input(chip, gpio_chip_hwgpio(desc));
if (status == 0)
clear_bit(FLAG_IS_OUT, &desc->flags);
trace_gpio_direction(desc_to_gpio(desc), 1, status);
-lose:
- return status;
-fail:
- spin_unlock_irqrestore(&gpio_lock, flags);
- if (status)
- gpiod_dbg(desc, "%s: status %d\n", __func__, status);
+
return status;
}
EXPORT_SYMBOL_GPL(gpiod_direction_input);
static int _gpiod_direction_output_raw(struct gpio_desc *desc, int value)
{
- unsigned long flags;
struct gpio_chip *chip;
int status = -EINVAL;
- int offset;
/* GPIOs used for IRQs shall not be set as output */
if (test_bit(FLAG_USED_AS_IRQ, &desc->flags)) {
@@ -1053,42 +986,11 @@ static int _gpiod_direction_output_raw(struct gpio_desc *desc, int value)
return -EIO;
}
- spin_lock_irqsave(&gpio_lock, flags);
-
- status = gpio_ensure_requested(desc);
- if (status < 0)
- goto fail;
-
- /* now we know the gpio is valid and chip won't vanish */
-
- spin_unlock_irqrestore(&gpio_lock, flags);
-
- might_sleep_if(chip->can_sleep);
-
- offset = gpio_chip_hwgpio(desc);
- if (status) {
- status = chip->request(chip, offset);
- if (status < 0) {
- gpiod_dbg(desc, "%s: chip request fail, %d\n",
- __func__, status);
- /* and it's not available to anyone else ...
- * gpio_request() is the fully clean solution.
- */
- goto lose;
- }
- }
-
- status = chip->direction_output(chip, offset, value);
+ status = chip->direction_output(chip, gpio_chip_hwgpio(desc), value);
if (status == 0)
set_bit(FLAG_IS_OUT, &desc->flags);
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)
- gpiod_dbg(desc, "%s: gpio status %d\n", __func__, status);
return status;
}
@@ -1147,10 +1049,7 @@ EXPORT_SYMBOL_GPL(gpiod_direction_output);
*/
int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
{
- unsigned long flags;
struct gpio_chip *chip;
- int status = -EINVAL;
- int offset;
if (!desc || !desc->chip) {
pr_warn("%s: invalid GPIO\n", __func__);
@@ -1165,27 +1064,7 @@ int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
return -ENOTSUPP;
}
- spin_lock_irqsave(&gpio_lock, flags);
-
- status = gpio_ensure_requested(desc);
- if (status < 0)
- goto fail;
-
- /* now we know the gpio is valid and chip won't vanish */
-
- spin_unlock_irqrestore(&gpio_lock, flags);
-
- might_sleep_if(chip->can_sleep);
-
- offset = gpio_chip_hwgpio(desc);
- return chip->set_debounce(chip, offset, debounce);
-
-fail:
- spin_unlock_irqrestore(&gpio_lock, flags);
- if (status)
- gpiod_dbg(desc, "%s: status %d\n", __func__, status);
-
- return status;
+ return chip->set_debounce(chip, gpio_chip_hwgpio(desc), debounce);
}
EXPORT_SYMBOL_GPL(gpiod_set_debounce);
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index c1d4105e1c1d..39a1d06950d9 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -63,19 +63,10 @@ static inline struct gpio_chip *gpio_to_chip(unsigned gpio)
extern int gpio_request(unsigned gpio, const char *label);
extern void gpio_free(unsigned gpio);
-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_raw(gpio_to_desc(gpio), value);
-}
+extern int gpio_direction_input(unsigned gpio);
+extern int gpio_direction_output(unsigned gpio, int value);
-static inline int gpio_set_debounce(unsigned gpio, unsigned debounce)
-{
- return gpiod_set_debounce(gpio_to_desc(gpio), debounce);
-}
+extern int gpio_set_debounce(unsigned gpio, unsigned debounce);
static inline int gpio_get_value_cansleep(unsigned gpio)
{
--
2.0.1
On 07/22/2014 12:47 PM, Alexandre Courbot wrote:
(...)
> + if (WARN(test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0,
> + "autorequest GPIO-%d\n", desc_to_gpio(desc))) {
> + if (!try_module_get(chip->owner)) {
> + gpiod_err(desc, "%s: module can't be gotten\n",
> + __func__);
Should match open parenthesis '('
gpiod_err(desc, "%s: module can't be gotten\n",
__func__);
> + clear_bit(FLAG_REQUESTED, &desc->flags);
> + /* lose */
> + err = -EIO;
> + goto end;
> + }
> + desc->label = "[auto]";
> + /* caller must chip->request() w/o spinlock */
> + if (chip->request)
> + request = true;
> + }
> +
> +end:
> + spin_unlock_irqrestore(&gpio_lock, flags);
> +
> + if (request) {
> + might_sleep_if(chip->can_sleep);
> + err = chip->request(chip, gpio_chip_hwgpio(desc));
> +
> + if (err < 0) {
> + gpiod_dbg(desc, "%s: chip request fail, %d\n",
> + __func__, err);
Dto..
(...)
--
Regards,
Varka Bhadram.
On Tue, Jul 22, 2014 at 04:17:41PM +0900, Alexandre Courbot wrote:
> As GPIO descriptors are not going to remain unique anymore, having this
> function public is not safe. Restrain its use to gpiolib since we have
> no user outside of it.
>
If I implement a gpio chip driver built as module, and I want to use
gpiochip_request_own_desc(), how am I supposed to get desc ?
I understand that there is still gpio_to_desc(), but I would have thought
that
desc = gpiochip_get_desc(chip, pin);
would be better than
desc = gpio_to_desc(chip->base + pin);
Not that it makes much of a difference for me, just asking.
Thanks,
Guenter
On Wed, Jul 23, 2014 at 5:17 AM, Guenter Roeck <[email protected]> wrote:
> On Tue, Jul 22, 2014 at 04:17:41PM +0900, Alexandre Courbot wrote:
>> As GPIO descriptors are not going to remain unique anymore, having this
>> function public is not safe. Restrain its use to gpiolib since we have
>> no user outside of it.
>>
> If I implement a gpio chip driver built as module, and I want to use
> gpiochip_request_own_desc(), how am I supposed to get desc ?
>
> I understand that there is still gpio_to_desc(), but I would have thought
> that
> desc = gpiochip_get_desc(chip, pin);
> would be better than
> desc = gpio_to_desc(chip->base + pin);
>
> Not that it makes much of a difference for me, just asking.
Actually I was thinking of changing the prototype of
gpiochip_request_own_desc(), and your comment definitely strenghtens
that idea. gpiochip functions should not work with descriptors,
especially since we are going to switch to a multiple-consumer scheme
where there won't be any canonical descriptor anymore. Thus, how about
turning gpiochip_request_own_desc() into this:
struct gpio_desc *
gpiochip_request_own_desc(struct gpio_chip *chip, u16 hwnum, const char *label);
which would basically do both the gpiochip_get_desc() and former
gpiochip_request_own_desc() in one call. I think it should satisfy
everybody and removes the need to have gpiochip_get_desc() (a not very
useful function by itself) exposed out of gpiolib.
I will send a patch taking care of this if you agree that makes sense.
On 07/22/2014 08:10 PM, Alexandre Courbot wrote:
> On Wed, Jul 23, 2014 at 5:17 AM, Guenter Roeck <[email protected]> wrote:
>> On Tue, Jul 22, 2014 at 04:17:41PM +0900, Alexandre Courbot wrote:
>>> As GPIO descriptors are not going to remain unique anymore, having this
>>> function public is not safe. Restrain its use to gpiolib since we have
>>> no user outside of it.
>>>
>> If I implement a gpio chip driver built as module, and I want to use
>> gpiochip_request_own_desc(), how am I supposed to get desc ?
>>
>> I understand that there is still gpio_to_desc(), but I would have thought
>> that
>> desc = gpiochip_get_desc(chip, pin);
>> would be better than
>> desc = gpio_to_desc(chip->base + pin);
>>
>> Not that it makes much of a difference for me, just asking.
>
> Actually I was thinking of changing the prototype of
> gpiochip_request_own_desc(), and your comment definitely strenghtens
> that idea. gpiochip functions should not work with descriptors,
> especially since we are going to switch to a multiple-consumer scheme
> where there won't be any canonical descriptor anymore. Thus, how about
> turning gpiochip_request_own_desc() into this:
>
> struct gpio_desc *
> gpiochip_request_own_desc(struct gpio_chip *chip, u16 hwnum, const char *label);
>
> which would basically do both the gpiochip_get_desc() and former
> gpiochip_request_own_desc() in one call. I think it should satisfy
> everybody and removes the need to have gpiochip_get_desc() (a not very
> useful function by itself) exposed out of gpiolib.
>
> I will send a patch taking care of this if you agree that makes sense.
>
I think you also plan to remove the capability to retrieve the chip
pointer, don't you ? If so, I won't be able to use the function from
the pca953x platform init function, since I won't be able to get the
pointer to the gpio chip. Even if you don't remove gpiod_to_chip(),
things would become a bit messy, since I would first have to convert
a pin to a desc and then to the chip pointer. Anyway, that change
would mean that exporting gpiochip_request_own_desc or its replacement
won't solve one of the problems addressed by my patch anymore, leaving
me more or less in the dark :-(.
I was thinking about implementing a separate platform driver which
would enable me to auto-export (or initialize, if needed) gpio pins
from arbitrary gpio drivers to user space. I could make this work
with both devicetree data and platform data. Sure, that driver
would not have a chance to get accepted upstream, since it would use
devicetree data to, in a sense, configure the system, but on the
upside it would be independent of gpio API changes, and it would
work for _all_ gpio chips, not just for the ones with gpio driver
support. Unfortunately this approach doesn't really work either,
since exported pin names need to be configured with the chip driver,
and can not be selected afterwards when a pin is actually exported.
On the other side, would you agree to adding something like
gpiod_export_name(), which would export a gpio pin with given name,
not using the default or chip->names ? That might help solving
at least some of my problems, and I would no longer depend on
gpiochip_request_own_desc or any of the related functions.
For reference, I currently need the ability to auto-export
gpio pins to user space for pca953x, ich, and for various
to-be-published gpio drivers used by my employer.
Thanks,
Guenter
On Wed, Jul 23, 2014 at 12:47 PM, Guenter Roeck <[email protected]> wrote:
> On 07/22/2014 08:10 PM, Alexandre Courbot wrote:
>>
>> On Wed, Jul 23, 2014 at 5:17 AM, Guenter Roeck <[email protected]> wrote:
>>>
>>> On Tue, Jul 22, 2014 at 04:17:41PM +0900, Alexandre Courbot wrote:
>>>>
>>>> As GPIO descriptors are not going to remain unique anymore, having this
>>>> function public is not safe. Restrain its use to gpiolib since we have
>>>> no user outside of it.
>>>>
>>> If I implement a gpio chip driver built as module, and I want to use
>>> gpiochip_request_own_desc(), how am I supposed to get desc ?
>>>
>>> I understand that there is still gpio_to_desc(), but I would have thought
>>> that
>>> desc = gpiochip_get_desc(chip, pin);
>>> would be better than
>>> desc = gpio_to_desc(chip->base + pin);
>>>
>>> Not that it makes much of a difference for me, just asking.
>>
>>
>> Actually I was thinking of changing the prototype of
>> gpiochip_request_own_desc(), and your comment definitely strenghtens
>> that idea. gpiochip functions should not work with descriptors,
>> especially since we are going to switch to a multiple-consumer scheme
>> where there won't be any canonical descriptor anymore. Thus, how about
>> turning gpiochip_request_own_desc() into this:
>>
>> struct gpio_desc *
>> gpiochip_request_own_desc(struct gpio_chip *chip, u16 hwnum, const char
>> *label);
>>
>> which would basically do both the gpiochip_get_desc() and former
>> gpiochip_request_own_desc() in one call. I think it should satisfy
>> everybody and removes the need to have gpiochip_get_desc() (a not very
>> useful function by itself) exposed out of gpiolib.
>>
>> I will send a patch taking care of this if you agree that makes sense.
>>
>
> I think you also plan to remove the capability to retrieve the chip
> pointer, don't you ? If so, I won't be able to use the function from
> the pca953x platform init function, since I won't be able to get the
> pointer to the gpio chip. Even if you don't remove gpiod_to_chip(),
> things would become a bit messy, since I would first have to convert
> a pin to a desc and then to the chip pointer. Anyway, that change
> would mean that exporting gpiochip_request_own_desc or its replacement
> won't solve one of the problems addressed by my patch anymore, leaving
> me more or less in the dark :-(.
Here is why this change is taking place: right now you have a clear
descriptor/pin mapping, i.e. there is only one descriptor per pin,
anytime. For various reasons this is going to change soon, and
descriptors will be allocated the provided to GPIO consumers as an
abstraction of the pin. Meaning that you cannot really "get the
descriptor for that pin" anymore. Since gpiochip_request_own_desc()'s
purpose is precisely to request one descriptor for drivers to use, the
new prototype makes much more sense IMHO.
Another reason to have it work on a gpio_chip is that the gpio_chip
pointer is a token to doing certain "priviledged" operations. Like
obtaining an arbitrary descriptor. If consumers can get a pointer to
the gpio_chip of a descriptor, this means they can basically do
anything.
Being in the board code, it seems to be that you are in a good
position to obtain a pointer to the gpio_chip, and without knowing
better I'd say that's what you should try to do. But maybe I would
understand your problem better if you could post a small patch of what
you want to achieve here.
>
> I was thinking about implementing a separate platform driver which
> would enable me to auto-export (or initialize, if needed) gpio pins
> from arbitrary gpio drivers to user space. I could make this work
> with both devicetree data and platform data. Sure, that driver
> would not have a chance to get accepted upstream, since it would use
> devicetree data to, in a sense, configure the system, but on the
> upside it would be independent of gpio API changes, and it would
> work for _all_ gpio chips, not just for the ones with gpio driver
> support. Unfortunately this approach doesn't really work either,
> since exported pin names need to be configured with the chip driver,
> and can not be selected afterwards when a pin is actually exported.
>
> On the other side, would you agree to adding something like
> gpiod_export_name(), which would export a gpio pin with given name,
> not using the default or chip->names ? That might help solving
> at least some of my problems, and I would no longer depend on
> gpiochip_request_own_desc or any of the related functions.
Isn't that what gpiod_export_link() does?
>
> For reference, I currently need the ability to auto-export
> gpio pins to user space for pca953x, ich, and for various
> to-be-published gpio drivers used by my employer.
Under which criteria are the GPIOs auto-exported? Can't you have the
board code simply request all the GPIOs as a regular consumer and
export them to user-space?
On 07/22/2014 10:39 PM, Alexandre Courbot wrote:
> On Wed, Jul 23, 2014 at 12:47 PM, Guenter Roeck <[email protected]> wrote:
>> On 07/22/2014 08:10 PM, Alexandre Courbot wrote:
>>>
>>> On Wed, Jul 23, 2014 at 5:17 AM, Guenter Roeck <[email protected]> wrote:
>>>>
>>>> On Tue, Jul 22, 2014 at 04:17:41PM +0900, Alexandre Courbot wrote:
>>>>>
>>>>> As GPIO descriptors are not going to remain unique anymore, having this
>>>>> function public is not safe. Restrain its use to gpiolib since we have
>>>>> no user outside of it.
>>>>>
>>>> If I implement a gpio chip driver built as module, and I want to use
>>>> gpiochip_request_own_desc(), how am I supposed to get desc ?
>>>>
>>>> I understand that there is still gpio_to_desc(), but I would have thought
>>>> that
>>>> desc = gpiochip_get_desc(chip, pin);
>>>> would be better than
>>>> desc = gpio_to_desc(chip->base + pin);
>>>>
>>>> Not that it makes much of a difference for me, just asking.
>>>
>>>
>>> Actually I was thinking of changing the prototype of
>>> gpiochip_request_own_desc(), and your comment definitely strenghtens
>>> that idea. gpiochip functions should not work with descriptors,
>>> especially since we are going to switch to a multiple-consumer scheme
>>> where there won't be any canonical descriptor anymore. Thus, how about
>>> turning gpiochip_request_own_desc() into this:
>>>
>>> struct gpio_desc *
>>> gpiochip_request_own_desc(struct gpio_chip *chip, u16 hwnum, const char
>>> *label);
>>>
>>> which would basically do both the gpiochip_get_desc() and former
>>> gpiochip_request_own_desc() in one call. I think it should satisfy
>>> everybody and removes the need to have gpiochip_get_desc() (a not very
>>> useful function by itself) exposed out of gpiolib.
>>>
>>> I will send a patch taking care of this if you agree that makes sense.
>>>
>>
>> I think you also plan to remove the capability to retrieve the chip
>> pointer, don't you ? If so, I won't be able to use the function from
>> the pca953x platform init function, since I won't be able to get the
>> pointer to the gpio chip. Even if you don't remove gpiod_to_chip(),
>> things would become a bit messy, since I would first have to convert
>> a pin to a desc and then to the chip pointer. Anyway, that change
>> would mean that exporting gpiochip_request_own_desc or its replacement
>> won't solve one of the problems addressed by my patch anymore, leaving
>> me more or less in the dark :-(.
>
> Here is why this change is taking place: right now you have a clear
> descriptor/pin mapping, i.e. there is only one descriptor per pin,
> anytime. For various reasons this is going to change soon, and
> descriptors will be allocated the provided to GPIO consumers as an
> abstraction of the pin. Meaning that you cannot really "get the
> descriptor for that pin" anymore. Since gpiochip_request_own_desc()'s
> purpose is precisely to request one descriptor for drivers to use, the
> new prototype makes much more sense IMHO.
>
> Another reason to have it work on a gpio_chip is that the gpio_chip
> pointer is a token to doing certain "priviledged" operations. Like
> obtaining an arbitrary descriptor. If consumers can get a pointer to
> the gpio_chip of a descriptor, this means they can basically do
> anything.
>
I understand, but my problem with pca953x platform initialization
is that the code to do that is outside the gpio directory in platform code.
Even though this is not consumer code, it still needs a means to perform
operations on a gpio pin, whatever those means are.
> Being in the board code, it seems to be that you are in a good
> position to obtain a pointer to the gpio_chip, and without knowing
> better I'd say that's what you should try to do. But maybe I would
> understand your problem better if you could post a small patch of what
> you want to achieve here.
>
Ok, but how do I get the pointer to the gpio chip from platform code
if gpiod_to_chip is gone ?
I attached the relevant parts of a platform file (scu.c), the one utilizing
pca953x initialization code to auto-export gpio pins. It currently uses
gpio_request_one(), which I am trying to replace. I can send you
the complete file if you like, but it is 1,600 bytes long so I figured
that would not help much.
I also attached a patch that tries to replace gpio_request_one with
gpiochip_request_own_desc in a gpio chip driver; maybe that gives
you an idea of that part of the problem.
>>
>> I was thinking about implementing a separate platform driver which
>> would enable me to auto-export (or initialize, if needed) gpio pins
>> from arbitrary gpio drivers to user space. I could make this work
>> with both devicetree data and platform data. Sure, that driver
>> would not have a chance to get accepted upstream, since it would use
>> devicetree data to, in a sense, configure the system, but on the
>> upside it would be independent of gpio API changes, and it would
>> work for _all_ gpio chips, not just for the ones with gpio driver
>> support. Unfortunately this approach doesn't really work either,
>> since exported pin names need to be configured with the chip driver,
>> and can not be selected afterwards when a pin is actually exported.
>>
>> On the other side, would you agree to adding something like
>> gpiod_export_name(), which would export a gpio pin with given name,
>> not using the default or chip->names ? That might help solving
>> at least some of my problems, and I would no longer depend on
>> gpiochip_request_own_desc or any of the related functions.
>
> Isn't that what gpiod_export_link() does?
>
This function assumes the existence of an exported pin,
and the resulting link is not in /sys/class/gpio but elsewhere.
That is not really the idea here; I did not want to create a second
export directory just for the sake of having well defined pin names
in some arbitrary place; the idea was to have those well defined
names in /sys/cass/gpio, and /sys/class/gpio is not available
as target for linking outside the gpio subsystem (if I remember
correctly because gpio_class is static and not exported).
>>
>> For reference, I currently need the ability to auto-export
>> gpio pins to user space for pca953x, ich, and for various
>> to-be-published gpio drivers used by my employer.
>
> Under which criteria are the GPIOs auto-exported? Can't you have the
> board code simply request all the GPIOs as a regular consumer and
> export them to user-space?
>
That is what the above mentioned platform driver would do. On x86 it
would use platform data for the purpose of identifying to-be-exported
pins, on devicetree platforms it would use devicetree data.
Thanks,
Guenter
On Wed, Jul 23, 2014 at 3:21 PM, Guenter Roeck <[email protected]> wrote:
> On 07/22/2014 10:39 PM, Alexandre Courbot wrote:
>>
>> On Wed, Jul 23, 2014 at 12:47 PM, Guenter Roeck <[email protected]>
>> wrote:
>>>
>>> On 07/22/2014 08:10 PM, Alexandre Courbot wrote:
>>>>
>>>>
>>>> On Wed, Jul 23, 2014 at 5:17 AM, Guenter Roeck <[email protected]>
>>>> wrote:
>>>>>
>>>>>
>>>>> On Tue, Jul 22, 2014 at 04:17:41PM +0900, Alexandre Courbot wrote:
>>>>>>
>>>>>>
>>>>>> As GPIO descriptors are not going to remain unique anymore, having
>>>>>> this
>>>>>> function public is not safe. Restrain its use to gpiolib since we have
>>>>>> no user outside of it.
>>>>>>
>>>>> If I implement a gpio chip driver built as module, and I want to use
>>>>> gpiochip_request_own_desc(), how am I supposed to get desc ?
>>>>>
>>>>> I understand that there is still gpio_to_desc(), but I would have
>>>>> thought
>>>>> that
>>>>> desc = gpiochip_get_desc(chip, pin);
>>>>> would be better than
>>>>> desc = gpio_to_desc(chip->base + pin);
>>>>>
>>>>> Not that it makes much of a difference for me, just asking.
>>>>
>>>>
>>>>
>>>> Actually I was thinking of changing the prototype of
>>>> gpiochip_request_own_desc(), and your comment definitely strenghtens
>>>> that idea. gpiochip functions should not work with descriptors,
>>>> especially since we are going to switch to a multiple-consumer scheme
>>>> where there won't be any canonical descriptor anymore. Thus, how about
>>>> turning gpiochip_request_own_desc() into this:
>>>>
>>>> struct gpio_desc *
>>>> gpiochip_request_own_desc(struct gpio_chip *chip, u16 hwnum, const char
>>>> *label);
>>>>
>>>> which would basically do both the gpiochip_get_desc() and former
>>>> gpiochip_request_own_desc() in one call. I think it should satisfy
>>>> everybody and removes the need to have gpiochip_get_desc() (a not very
>>>> useful function by itself) exposed out of gpiolib.
>>>>
>>>> I will send a patch taking care of this if you agree that makes sense.
>>>>
>>>
>>> I think you also plan to remove the capability to retrieve the chip
>>> pointer, don't you ? If so, I won't be able to use the function from
>>> the pca953x platform init function, since I won't be able to get the
>>> pointer to the gpio chip. Even if you don't remove gpiod_to_chip(),
>>> things would become a bit messy, since I would first have to convert
>>> a pin to a desc and then to the chip pointer. Anyway, that change
>>> would mean that exporting gpiochip_request_own_desc or its replacement
>>> won't solve one of the problems addressed by my patch anymore, leaving
>>> me more or less in the dark :-(.
>>
>>
>> Here is why this change is taking place: right now you have a clear
>> descriptor/pin mapping, i.e. there is only one descriptor per pin,
>> anytime. For various reasons this is going to change soon, and
>> descriptors will be allocated the provided to GPIO consumers as an
>> abstraction of the pin. Meaning that you cannot really "get the
>> descriptor for that pin" anymore. Since gpiochip_request_own_desc()'s
>> purpose is precisely to request one descriptor for drivers to use, the
>> new prototype makes much more sense IMHO.
>>
>> Another reason to have it work on a gpio_chip is that the gpio_chip
>> pointer is a token to doing certain "priviledged" operations. Like
>> obtaining an arbitrary descriptor. If consumers can get a pointer to
>> the gpio_chip of a descriptor, this means they can basically do
>> anything.
>>
> I understand, but my problem with pca953x platform initialization
> is that the code to do that is outside the gpio directory in platform code.
> Even though this is not consumer code, it still needs a means to perform
> operations on a gpio pin, whatever those means are.
>
>
>> Being in the board code, it seems to be that you are in a good
>> position to obtain a pointer to the gpio_chip, and without knowing
>> better I'd say that's what you should try to do. But maybe I would
>> understand your problem better if you could post a small patch of what
>> you want to achieve here.
>>
> Ok, but how do I get the pointer to the gpio chip from platform code
> if gpiod_to_chip is gone ?
>
> I attached the relevant parts of a platform file (scu.c), the one utilizing
> pca953x initialization code to auto-export gpio pins. It currently uses
> gpio_request_one(), which I am trying to replace. I can send you
> the complete file if you like, but it is 1,600 bytes long so I figured
> that would not help much.
Great, that's exactly what I need to understand where your troubles are.
First that's a very special case you are working on, and one that is
likely to be frown upon (basically, user-space drivers). An
upstreamable solution would likely be to write proper kernel drivers
for whatever is using these GPIOs and the one I have to recommend
here. I understand that you probably have customer requirements that
are going against that, and that consequently you are ok with a
non-upstreamable solution.
If non-upstream is ok, you can probably abuse gpio_request_one()
followed by gpio_to_desc(). After we switch to multi-descriptors,
gpio_request_one() will allocate the unique descriptor to be used by
the integer API, and gpio_to_desc() will return it. That would be the
simplest solution.
Using lookup tables here seems to be out of place, because as I
understand it you want this driver to be a loadable module. We could
consider exporting gpiod_add_lookup_table() - after all, if you have
the ability to load a kernel module, you certainly can reconfigure a
few GPIOs. Then you could probably generate your tables dynamically -
I believe it would not take that much code. Doing so will allow you to
go without the integer API at all. If you are out-of-tree already, you
probably don't care.
In the end your driver is a GPIO consumer combined with a few extra
GPIO mappings, so you should be able to implement it using the current
API in consumer.h and driver.h (maybe just a few changes for the
lookup tables if you want to go that way).
The export thing is a different issue, let's treat it below.
>
> I also attached a patch that tries to replace gpio_request_one with
> gpiochip_request_own_desc in a gpio chip driver; maybe that gives
> you an idea of that part of the problem.
Mmm what problem is this code supposed to highlight here? The proposed
change of prototype for gpiochip_request_own_desc()? This is a GPIO
driver, so don't you have a pointer to the gpio_chip here that allows
you to use this interface without problem?
Sorry if I miss some of your points - you have to explain your problem
like you would to a 5 years old. :P
>
>
>>>
>>> I was thinking about implementing a separate platform driver which
>>> would enable me to auto-export (or initialize, if needed) gpio pins
>>> from arbitrary gpio drivers to user space. I could make this work
>>> with both devicetree data and platform data. Sure, that driver
>>> would not have a chance to get accepted upstream, since it would use
>>> devicetree data to, in a sense, configure the system, but on the
>>> upside it would be independent of gpio API changes, and it would
>>> work for _all_ gpio chips, not just for the ones with gpio driver
>>> support. Unfortunately this approach doesn't really work either,
>>> since exported pin names need to be configured with the chip driver,
>>> and can not be selected afterwards when a pin is actually exported.
>>>
>>> On the other side, would you agree to adding something like
>>> gpiod_export_name(), which would export a gpio pin with given name,
>>> not using the default or chip->names ? That might help solving
>>> at least some of my problems, and I would no longer depend on
>>> gpiochip_request_own_desc or any of the related functions.
>>
>>
>> Isn't that what gpiod_export_link() does?
>>
> This function assumes the existence of an exported pin,
> and the resulting link is not in /sys/class/gpio but elsewhere.
> That is not really the idea here; I did not want to create a second
> export directory just for the sake of having well defined pin names
> in some arbitrary place; the idea was to have those well defined
> names in /sys/cass/gpio, and /sys/class/gpio is not available
> as target for linking outside the gpio subsystem (if I remember
> correctly because gpio_class is static and not exported).
We have grown radically against the sysfs interface recently - and the
fact that it never fits everyone's needs (like your very specific
naming requirements) adds just more fuel to this ressentment. I think
you can achieve something very close using gpiod_export_link(), if you
export all your GPIOs under your scu device. It won't be under
/sys/class/gpio but rather under /sys/devices/platform/scu (or
something like that), which seems to make even more sense to me as
they are to be used by this device.
My rationale here is you don't know who exported a GPIO under
/sys/class/gpio. It could be the driver you expect, or not. With
gpiod_export_link(), you know exactly which device provides you the
GPIO, and which function it is supposed to fulfil. So IMHO this is a
much better place for user-space to get GPIOs. If you cannot live with
this, I will need more arguments to understand why this cannot suit.
On Tue, Jul 22, 2014 at 9:17 AM, Alexandre Courbot <[email protected]> wrote:
> of_get_named_gpio_flags() has been made gpiolib-private by commit
> f01d907582, but its EXPORT statement has not been removed. Fix this.
>
> Signed-off-by: Alexandre Courbot <[email protected]>
Patch applied.
Yours,
Linus Walleij
On Tue, Jul 22, 2014 at 9:17 AM, Alexandre Courbot <[email protected]> wrote:
> For some reason gpiochip_export() would invalidate all the descriptors
> of a chip if exporting it to sysfs failed. This does not appear as
> necessary. Remove that part of the code.
>
> While we are at it, add a note about the non-safety of temporarily
> releasing a spinlock in the middle of the loop that protects its
> iterator, and explain why this is done.
>
> Signed-off-by: Alexandre Courbot <[email protected]>
Patch applied.
Yours,
Linus Walleij
On Tue, Jul 22, 2014 at 9:17 AM, Alexandre Courbot <[email protected]> wrote:
> As GPIO descriptors are not going to remain unique anymore, having this
> function public is not safe. Restrain its use to gpiolib since we have
> no user outside of it.
>
> Signed-off-by: Alexandre Courbot <[email protected]>
Patch applied.
Yours,
Linus Walleij
On Tue, Jul 22, 2014 at 9:17 AM, Alexandre Courbot <[email protected]> wrote:
> gpio_lock/unlock_as_irq() are working with (chip, offset) arguments and
> are thus not using the old integer namespace. Therefore, there is no
> reason to have gpiod variants of these functions working with
> descriptors, especially since the (chip, offset) tuple is more suitable
> to the users of these functions (GPIO drivers, whereas GPIO descriptors
> are targeted at GPIO consumers).
>
> Signed-off-by: Alexandre Courbot <[email protected]>
Argh the mess is likely my fault for not having wrapped my head around
descriptors properly at the time.
Patch applied, naturally.
Yours,
Linus Walleij
On Tue, Jul 22, 2014 at 9:17 AM, Alexandre Courbot <[email protected]> wrote:
> gpio_ensure_requested() only makes sense when using the integer-based
> GPIO API, so make sure it is called from there instead of the gpiod
> API which we know cannot be called with a non-requested GPIO anyway.
>
> The uses of gpio_ensure_requested() in the gpiod API were kind of
> out-of-place anyway, so putting them in gpio-legacy.c helps clearing the
> code.
>
> Actually, considering the time this ensure_requested mechanism has been
> around, maybe we should just turn this patch into "remove
> gpio_ensure_requested()" if we know for sure that no user depend on it
> anymore?
>
> Signed-off-by: Alexandre Courbot <[email protected]>
Patch applied.
Make a separate patch on top to delete this thing since there
are ZERO in-kernel users. The only place it's used in a comment
in arch/arm/mach-ep93xx/vision_ep9307.c ....
Yours,
Linus Walleij