* [PATCH 1/2] gpiolib: add gpio_export_with_name
@ 2012-11-21 10:14 ` Jean-Christophe PLAGNIOL-VILLARD
0 siblings, 0 replies; 30+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2012-11-21 10:14 UTC (permalink / raw)
To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ
allow to specify a name to an exported gpio
Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
---
arch/mips/include/asm/mach-au1x00/gpio-au1000.h | 3 ++-
arch/mips/include/asm/mach-au1x00/gpio-au1300.h | 3 ++-
drivers/gpio/gpiolib.c | 12 +++++++-----
include/asm-generic/gpio.h | 6 ++++--
include/linux/gpio.h | 23 ++++++++++++++++++++++-
5 files changed, 37 insertions(+), 10 deletions(-)
diff --git a/arch/mips/include/asm/mach-au1x00/gpio-au1000.h b/arch/mips/include/asm/mach-au1x00/gpio-au1000.h
index 73853b5a..af4accb 100644
--- a/arch/mips/include/asm/mach-au1x00/gpio-au1000.h
+++ b/arch/mips/include/asm/mach-au1x00/gpio-au1000.h
@@ -633,7 +633,8 @@ static inline int gpio_set_debounce(unsigned gpio, unsigned debounce)
return -ENOSYS;
}
-static inline int gpio_export(unsigned gpio, bool direction_may_change)
+static inline int gpio_export_with_name(unsigned gpio,
+ bool direction_may_change, const char *name)
{
return -ENOSYS;
}
diff --git a/arch/mips/include/asm/mach-au1x00/gpio-au1300.h b/arch/mips/include/asm/mach-au1x00/gpio-au1300.h
index fb9975c..fcd152e 100644
--- a/arch/mips/include/asm/mach-au1x00/gpio-au1300.h
+++ b/arch/mips/include/asm/mach-au1x00/gpio-au1300.h
@@ -234,7 +234,8 @@ static inline void gpio_unexport(unsigned gpio)
{
}
-static inline int gpio_export(unsigned gpio, bool direction_may_change)
+static inline int gpio_export_with_name(unsigned gpio,
+ bool direction_may_change, const char *name)
{
return -ENOSYS;
}
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index b667f76..2141165 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -714,9 +714,10 @@ static struct class gpio_class = {
/**
- * gpio_export - export a GPIO through sysfs
+ * gpio_export_with_name - export a GPIO through sysfs
* @gpio: gpio to make available, already requested
* @direction_may_change: true if userspace may change gpio direction
+ * @name: gpio name
* Context: arch_initcall or later
*
* When drivers want to make a GPIO accessible to userspace after they
@@ -728,7 +729,7 @@ static struct class gpio_class = {
*
* Returns zero on success, else an error.
*/
-int gpio_export(unsigned gpio, bool direction_may_change)
+int gpio_export_with_name(unsigned gpio, bool direction_may_change, const char *name)
{
unsigned long flags;
struct gpio_desc *desc;
@@ -766,7 +767,9 @@ int gpio_export(unsigned gpio, bool direction_may_change)
direction_may_change = false;
spin_unlock_irqrestore(&gpio_lock, flags);
- if (desc->chip->names && desc->chip->names[gpio - desc->chip->base])
+ if (name)
+ ioname = name;
+ else if (desc->chip->names && desc->chip->names[gpio - desc->chip->base])
ioname = desc->chip->names[gpio - desc->chip->base];
dev = device_create(&gpio_class, desc->chip->dev, MKDEV(0, 0),
@@ -804,7 +807,7 @@ fail_unlock:
pr_debug("%s: gpio%d status %d\n", __func__, gpio, status);
return status;
}
-EXPORT_SYMBOL_GPL(gpio_export);
+EXPORT_SYMBOL_GPL(gpio_export_with_name);
static int match_export(struct device *dev, void *data)
{
@@ -856,7 +859,6 @@ done:
}
EXPORT_SYMBOL_GPL(gpio_export_link);
-
/**
* gpio_sysfs_set_active_low - set the polarity of gpio sysfs value
* @gpio: gpio to change
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index b6516f3..d04fc55 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -204,7 +204,8 @@ 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_with_name(unsigned gpio, bool direction_may_change,
+ const char *name);
extern int gpio_export_link(struct device *dev, const char *name,
unsigned gpio);
extern int gpio_sysfs_set_active_low(unsigned gpio, int value);
@@ -249,7 +250,8 @@ struct device;
/* sysfs support is only available with gpiolib, where it's optional */
-static inline int gpio_export(unsigned gpio, bool direction_may_change)
+static inline int gpio_export_with_name(unsigned gpio,
+ bool direction_may_change, const char *name)
{
return -ENOSYS;
}
diff --git a/include/linux/gpio.h b/include/linux/gpio.h
index 7ba2762..6e01b8c 100644
--- a/include/linux/gpio.h
+++ b/include/linux/gpio.h
@@ -189,7 +189,8 @@ static inline void gpio_set_value_cansleep(unsigned gpio, int value)
WARN_ON(1);
}
-static inline int gpio_export(unsigned gpio, bool direction_may_change)
+static inline int gpio_export_with_name(unsigned gpio,
+ bool direction_may_change, const char *name)
{
/* GPIO can never have been requested or set as {in,out}put */
WARN_ON(1);
@@ -247,4 +248,24 @@ gpiochip_remove_pin_ranges(struct gpio_chip *chip)
#endif /* ! CONFIG_GENERIC_GPIO */
+/**
+ * gpio_export - export a GPIO through sysfs
+ * @gpio: gpio to make available, already requested
+ * @direction_may_change: true if userspace may change gpio direction
+ * Context: arch_initcall or later
+ *
+ * When drivers want to make a GPIO accessible to userspace after they
+ * have requested it -- perhaps while debugging, or as part of their
+ * public interface -- they may use this routine. If the GPIO can
+ * change direction (some can't) and the caller allows it, userspace
+ * will see "direction" sysfs attribute which may be used to change
+ * the gpio's direction. A "value" attribute will always be provided.
+ *
+ * Returns zero on success, else an error.
+ */
+static inline int gpio_export(unsigned gpio,bool direction_may_change)
+{
+ return gpio_export_with_name(gpio, direction_may_change, NULL);
+}
+
#endif /* __LINUX_GPIO_H */
--
1.7.10.4
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 2/2] gpiolib-of: ad gpio-export support
2012-11-21 10:14 ` Jean-Christophe PLAGNIOL-VILLARD
@ 2012-11-21 10:14 ` Jean-Christophe PLAGNIOL-VILLARD
-1 siblings, 0 replies; 30+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2012-11-21 10:14 UTC (permalink / raw)
To: linux-arm-kernel
This will allow to export gpios with or without names
Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
---
Documentation/devicetree/bindings/gpio/gpio.txt | 60 ++++++++++++++++++++++
drivers/gpio/gpiolib-of.c | 61 +++++++++++++++++++++++
2 files changed, 121 insertions(+)
diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt
index a336287..c2a9024 100644
--- a/Documentation/devicetree/bindings/gpio/gpio.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio.txt
@@ -112,3 +112,63 @@ where,
The pinctrl node must have "#gpio-range-cells" property to show number of
arguments to pass with phandle from gpio controllers node.
+
+3) gpio-export
+--------------
+
+gpio-export will allow you to automatically export gpio
+
+required properties:
+- compatible: Should be "gpio-export"
+
+in each child node will reprensent a gpio or if no name is specified
+a list of gpio to export
+
+required properties:
+- gpios: gpio to export
+
+optional properties:
+ - gpio-export,name: export name
+ - gpio-export,output: to set the as output with default value
+ if no present gpio as input
+ - pio-export,direction_may_change: boolean to allow the direction to be controllable
+
+Example:
+
+
+gpio_export {
+ compatible = "gpio-export";
+ #size-cells = <0>;
+
+ in {
+ gpio-export,name = "in";
+ gpios = <&pioC 20 0>;
+ };
+
+ out {
+ gpio-export,name = "out";
+ gpio-export,output = <1>;
+ gpio-export,direction_may_change;
+ gpios = <&pioC 21 0>;
+ };
+
+ in_out {
+ gpio-export,name = "in_out";
+ gpio-export,direction_may_change;
+ gpios = <&pioC 21 0>;
+ };
+
+ gpios_in {
+ gpios = <&pioB 0 0
+ &pioB 3 0
+ &pioC 4 0>;
+ gpio-export,direction_may_change;
+ };
+
+ gpios_out {
+ gpios = <&pioB 1 0
+ &pioB 2 0
+ &pioC 3 0>;
+ gpio-export,output = <1>;
+ };
+};
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index a40cd84..0ab9be4 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -21,6 +21,8 @@
#include <linux/of_gpio.h>
#include <linux/pinctrl/pinctrl.h>
#include <linux/slab.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
/* Private data structure for of_gpiochip_find_and_xlate */
struct gg_data {
@@ -277,3 +279,62 @@ void of_gpiochip_remove(struct gpio_chip *chip)
if (chip->of_node)
of_node_put(chip->of_node);
}
+
+static struct of_device_id gpio_export_ids[] = {
+ { .compatible = "gpio-export" },
+ { /* sentinel */ }
+};
+
+static int __init of_gpio_export_probe(struct platform_device *pdev)
+{
+ struct device_node *np = pdev->dev.of_node;
+ struct device_node *cnp;
+ u32 val;
+ int nb = 0;
+
+ for_each_child_of_node(np, cnp) {
+ const char *name = NULL;
+ int gpio;
+ bool dmc;
+ int max_gpio = 1;
+ int i;
+
+ of_property_read_string(cnp, "gpio-export,name", &name);
+
+ if (!name)
+ max_gpio = of_gpio_count(cnp);
+
+ for (i = 0; i < max_gpio; i++) {
+ gpio = of_get_gpio(cnp, i);
+ if (devm_gpio_request(&pdev->dev, gpio, name ? name : of_node_full_name(np)))
+ continue;
+
+ if (!of_property_read_u32(cnp, "gpio-export,output", &val))
+ gpio_direction_output(gpio, val);
+ else
+ gpio_direction_input(gpio);
+
+ dmc = of_property_read_bool(np, "gpio-export,direction_may_change");
+ gpio_export_with_name(gpio, dmc, name);
+ nb++;
+ }
+ }
+
+ dev_info(&pdev->dev, "%d gpio(s) exported\n", nb);
+
+ return 0;
+}
+
+static struct platform_driver gpio_export_driver = {
+ .driver = {
+ .name = "gpio-export",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(gpio_export_ids),
+ },
+};
+
+static int __init of_gpio_export_init(void)
+{
+ return platform_driver_probe(&gpio_export_driver, of_gpio_export_probe);
+}
+device_initcall(of_gpio_export_init);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 2/2] gpiolib-of: ad gpio-export support
@ 2012-11-21 10:14 ` Jean-Christophe PLAGNIOL-VILLARD
0 siblings, 0 replies; 30+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2012-11-21 10:14 UTC (permalink / raw)
To: linux-arm-kernel; +Cc: devicetree-discuss, Jean-Christophe PLAGNIOL-VILLARD
This will allow to export gpios with or without names
Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
---
Documentation/devicetree/bindings/gpio/gpio.txt | 60 ++++++++++++++++++++++
drivers/gpio/gpiolib-of.c | 61 +++++++++++++++++++++++
2 files changed, 121 insertions(+)
diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt
index a336287..c2a9024 100644
--- a/Documentation/devicetree/bindings/gpio/gpio.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio.txt
@@ -112,3 +112,63 @@ where,
The pinctrl node must have "#gpio-range-cells" property to show number of
arguments to pass with phandle from gpio controllers node.
+
+3) gpio-export
+--------------
+
+gpio-export will allow you to automatically export gpio
+
+required properties:
+- compatible: Should be "gpio-export"
+
+in each child node will reprensent a gpio or if no name is specified
+a list of gpio to export
+
+required properties:
+- gpios: gpio to export
+
+optional properties:
+ - gpio-export,name: export name
+ - gpio-export,output: to set the as output with default value
+ if no present gpio as input
+ - pio-export,direction_may_change: boolean to allow the direction to be controllable
+
+Example:
+
+
+gpio_export {
+ compatible = "gpio-export";
+ #size-cells = <0>;
+
+ in {
+ gpio-export,name = "in";
+ gpios = <&pioC 20 0>;
+ };
+
+ out {
+ gpio-export,name = "out";
+ gpio-export,output = <1>;
+ gpio-export,direction_may_change;
+ gpios = <&pioC 21 0>;
+ };
+
+ in_out {
+ gpio-export,name = "in_out";
+ gpio-export,direction_may_change;
+ gpios = <&pioC 21 0>;
+ };
+
+ gpios_in {
+ gpios = <&pioB 0 0
+ &pioB 3 0
+ &pioC 4 0>;
+ gpio-export,direction_may_change;
+ };
+
+ gpios_out {
+ gpios = <&pioB 1 0
+ &pioB 2 0
+ &pioC 3 0>;
+ gpio-export,output = <1>;
+ };
+};
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index a40cd84..0ab9be4 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -21,6 +21,8 @@
#include <linux/of_gpio.h>
#include <linux/pinctrl/pinctrl.h>
#include <linux/slab.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
/* Private data structure for of_gpiochip_find_and_xlate */
struct gg_data {
@@ -277,3 +279,62 @@ void of_gpiochip_remove(struct gpio_chip *chip)
if (chip->of_node)
of_node_put(chip->of_node);
}
+
+static struct of_device_id gpio_export_ids[] = {
+ { .compatible = "gpio-export" },
+ { /* sentinel */ }
+};
+
+static int __init of_gpio_export_probe(struct platform_device *pdev)
+{
+ struct device_node *np = pdev->dev.of_node;
+ struct device_node *cnp;
+ u32 val;
+ int nb = 0;
+
+ for_each_child_of_node(np, cnp) {
+ const char *name = NULL;
+ int gpio;
+ bool dmc;
+ int max_gpio = 1;
+ int i;
+
+ of_property_read_string(cnp, "gpio-export,name", &name);
+
+ if (!name)
+ max_gpio = of_gpio_count(cnp);
+
+ for (i = 0; i < max_gpio; i++) {
+ gpio = of_get_gpio(cnp, i);
+ if (devm_gpio_request(&pdev->dev, gpio, name ? name : of_node_full_name(np)))
+ continue;
+
+ if (!of_property_read_u32(cnp, "gpio-export,output", &val))
+ gpio_direction_output(gpio, val);
+ else
+ gpio_direction_input(gpio);
+
+ dmc = of_property_read_bool(np, "gpio-export,direction_may_change");
+ gpio_export_with_name(gpio, dmc, name);
+ nb++;
+ }
+ }
+
+ dev_info(&pdev->dev, "%d gpio(s) exported\n", nb);
+
+ return 0;
+}
+
+static struct platform_driver gpio_export_driver = {
+ .driver = {
+ .name = "gpio-export",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(gpio_export_ids),
+ },
+};
+
+static int __init of_gpio_export_init(void)
+{
+ return platform_driver_probe(&gpio_export_driver, of_gpio_export_probe);
+}
+device_initcall(of_gpio_export_init);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 1/2] gpiolib: add gpio_export_with_name
2012-11-21 10:14 ` Jean-Christophe PLAGNIOL-VILLARD
@ 2012-11-26 13:59 ` Grant Likely
-1 siblings, 0 replies; 30+ messages in thread
From: Grant Likely @ 2012-11-26 13:59 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, 21 Nov 2012 11:14:08 +0100, Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote:
> allow to specify a name to an exported gpio
>
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
The gpio sysfs ABI is already horrible, racy, and unsafe. Really, we
need a proper chrdev interface for controlling gpios. Sysfs is fine for
poking around and experimenting, but we cannot provide any fine grained
access control, locking or faster IO with the one-file-per-gpio sysfs
model. So, no, I don't think this is a good idea to extend gpiolib in
this way.
However, I would be okay with making gpiochips first-class kobjects or
struct devices that appear as a child of the gpio controller device so
that userspace could interrogate the device tree node associated with
the device.
g.
> ---
> arch/mips/include/asm/mach-au1x00/gpio-au1000.h | 3 ++-
> arch/mips/include/asm/mach-au1x00/gpio-au1300.h | 3 ++-
> drivers/gpio/gpiolib.c | 12 +++++++-----
> include/asm-generic/gpio.h | 6 ++++--
> include/linux/gpio.h | 23 ++++++++++++++++++++++-
> 5 files changed, 37 insertions(+), 10 deletions(-)
>
> diff --git a/arch/mips/include/asm/mach-au1x00/gpio-au1000.h b/arch/mips/include/asm/mach-au1x00/gpio-au1000.h
> index 73853b5a..af4accb 100644
> --- a/arch/mips/include/asm/mach-au1x00/gpio-au1000.h
> +++ b/arch/mips/include/asm/mach-au1x00/gpio-au1000.h
> @@ -633,7 +633,8 @@ static inline int gpio_set_debounce(unsigned gpio, unsigned debounce)
> return -ENOSYS;
> }
>
> -static inline int gpio_export(unsigned gpio, bool direction_may_change)
> +static inline int gpio_export_with_name(unsigned gpio,
> + bool direction_may_change, const char *name)
> {
> return -ENOSYS;
> }
> diff --git a/arch/mips/include/asm/mach-au1x00/gpio-au1300.h b/arch/mips/include/asm/mach-au1x00/gpio-au1300.h
> index fb9975c..fcd152e 100644
> --- a/arch/mips/include/asm/mach-au1x00/gpio-au1300.h
> +++ b/arch/mips/include/asm/mach-au1x00/gpio-au1300.h
> @@ -234,7 +234,8 @@ static inline void gpio_unexport(unsigned gpio)
> {
> }
>
> -static inline int gpio_export(unsigned gpio, bool direction_may_change)
> +static inline int gpio_export_with_name(unsigned gpio,
> + bool direction_may_change, const char *name)
> {
> return -ENOSYS;
> }
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index b667f76..2141165 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -714,9 +714,10 @@ static struct class gpio_class = {
>
>
> /**
> - * gpio_export - export a GPIO through sysfs
> + * gpio_export_with_name - export a GPIO through sysfs
> * @gpio: gpio to make available, already requested
> * @direction_may_change: true if userspace may change gpio direction
> + * @name: gpio name
> * Context: arch_initcall or later
> *
> * When drivers want to make a GPIO accessible to userspace after they
> @@ -728,7 +729,7 @@ static struct class gpio_class = {
> *
> * Returns zero on success, else an error.
> */
> -int gpio_export(unsigned gpio, bool direction_may_change)
> +int gpio_export_with_name(unsigned gpio, bool direction_may_change, const char *name)
> {
> unsigned long flags;
> struct gpio_desc *desc;
> @@ -766,7 +767,9 @@ int gpio_export(unsigned gpio, bool direction_may_change)
> direction_may_change = false;
> spin_unlock_irqrestore(&gpio_lock, flags);
>
> - if (desc->chip->names && desc->chip->names[gpio - desc->chip->base])
> + if (name)
> + ioname = name;
> + else if (desc->chip->names && desc->chip->names[gpio - desc->chip->base])
> ioname = desc->chip->names[gpio - desc->chip->base];
>
> dev = device_create(&gpio_class, desc->chip->dev, MKDEV(0, 0),
> @@ -804,7 +807,7 @@ fail_unlock:
> pr_debug("%s: gpio%d status %d\n", __func__, gpio, status);
> return status;
> }
> -EXPORT_SYMBOL_GPL(gpio_export);
> +EXPORT_SYMBOL_GPL(gpio_export_with_name);
>
> static int match_export(struct device *dev, void *data)
> {
> @@ -856,7 +859,6 @@ done:
> }
> EXPORT_SYMBOL_GPL(gpio_export_link);
>
> -
> /**
> * gpio_sysfs_set_active_low - set the polarity of gpio sysfs value
> * @gpio: gpio to change
> diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> index b6516f3..d04fc55 100644
> --- a/include/asm-generic/gpio.h
> +++ b/include/asm-generic/gpio.h
> @@ -204,7 +204,8 @@ 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_with_name(unsigned gpio, bool direction_may_change,
> + const char *name);
> extern int gpio_export_link(struct device *dev, const char *name,
> unsigned gpio);
> extern int gpio_sysfs_set_active_low(unsigned gpio, int value);
> @@ -249,7 +250,8 @@ struct device;
>
> /* sysfs support is only available with gpiolib, where it's optional */
>
> -static inline int gpio_export(unsigned gpio, bool direction_may_change)
> +static inline int gpio_export_with_name(unsigned gpio,
> + bool direction_may_change, const char *name)
> {
> return -ENOSYS;
> }
> diff --git a/include/linux/gpio.h b/include/linux/gpio.h
> index 7ba2762..6e01b8c 100644
> --- a/include/linux/gpio.h
> +++ b/include/linux/gpio.h
> @@ -189,7 +189,8 @@ static inline void gpio_set_value_cansleep(unsigned gpio, int value)
> WARN_ON(1);
> }
>
> -static inline int gpio_export(unsigned gpio, bool direction_may_change)
> +static inline int gpio_export_with_name(unsigned gpio,
> + bool direction_may_change, const char *name)
> {
> /* GPIO can never have been requested or set as {in,out}put */
> WARN_ON(1);
> @@ -247,4 +248,24 @@ gpiochip_remove_pin_ranges(struct gpio_chip *chip)
>
> #endif /* ! CONFIG_GENERIC_GPIO */
>
> +/**
> + * gpio_export - export a GPIO through sysfs
> + * @gpio: gpio to make available, already requested
> + * @direction_may_change: true if userspace may change gpio direction
> + * Context: arch_initcall or later
> + *
> + * When drivers want to make a GPIO accessible to userspace after they
> + * have requested it -- perhaps while debugging, or as part of their
> + * public interface -- they may use this routine. If the GPIO can
> + * change direction (some can't) and the caller allows it, userspace
> + * will see "direction" sysfs attribute which may be used to change
> + * the gpio's direction. A "value" attribute will always be provided.
> + *
> + * Returns zero on success, else an error.
> + */
> +static inline int gpio_export(unsigned gpio,bool direction_may_change)
> +{
> + return gpio_export_with_name(gpio, direction_may_change, NULL);
> +}
> +
> #endif /* __LINUX_GPIO_H */
> --
> 1.7.10.4
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 1/2] gpiolib: add gpio_export_with_name
@ 2012-11-26 13:59 ` Grant Likely
0 siblings, 0 replies; 30+ messages in thread
From: Grant Likely @ 2012-11-26 13:59 UTC (permalink / raw)
To: linux-arm-kernel; +Cc: devicetree-discuss, Jean-Christophe PLAGNIOL-VILLARD
On Wed, 21 Nov 2012 11:14:08 +0100, Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote:
> allow to specify a name to an exported gpio
>
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
The gpio sysfs ABI is already horrible, racy, and unsafe. Really, we
need a proper chrdev interface for controlling gpios. Sysfs is fine for
poking around and experimenting, but we cannot provide any fine grained
access control, locking or faster IO with the one-file-per-gpio sysfs
model. So, no, I don't think this is a good idea to extend gpiolib in
this way.
However, I would be okay with making gpiochips first-class kobjects or
struct devices that appear as a child of the gpio controller device so
that userspace could interrogate the device tree node associated with
the device.
g.
> ---
> arch/mips/include/asm/mach-au1x00/gpio-au1000.h | 3 ++-
> arch/mips/include/asm/mach-au1x00/gpio-au1300.h | 3 ++-
> drivers/gpio/gpiolib.c | 12 +++++++-----
> include/asm-generic/gpio.h | 6 ++++--
> include/linux/gpio.h | 23 ++++++++++++++++++++++-
> 5 files changed, 37 insertions(+), 10 deletions(-)
>
> diff --git a/arch/mips/include/asm/mach-au1x00/gpio-au1000.h b/arch/mips/include/asm/mach-au1x00/gpio-au1000.h
> index 73853b5a..af4accb 100644
> --- a/arch/mips/include/asm/mach-au1x00/gpio-au1000.h
> +++ b/arch/mips/include/asm/mach-au1x00/gpio-au1000.h
> @@ -633,7 +633,8 @@ static inline int gpio_set_debounce(unsigned gpio, unsigned debounce)
> return -ENOSYS;
> }
>
> -static inline int gpio_export(unsigned gpio, bool direction_may_change)
> +static inline int gpio_export_with_name(unsigned gpio,
> + bool direction_may_change, const char *name)
> {
> return -ENOSYS;
> }
> diff --git a/arch/mips/include/asm/mach-au1x00/gpio-au1300.h b/arch/mips/include/asm/mach-au1x00/gpio-au1300.h
> index fb9975c..fcd152e 100644
> --- a/arch/mips/include/asm/mach-au1x00/gpio-au1300.h
> +++ b/arch/mips/include/asm/mach-au1x00/gpio-au1300.h
> @@ -234,7 +234,8 @@ static inline void gpio_unexport(unsigned gpio)
> {
> }
>
> -static inline int gpio_export(unsigned gpio, bool direction_may_change)
> +static inline int gpio_export_with_name(unsigned gpio,
> + bool direction_may_change, const char *name)
> {
> return -ENOSYS;
> }
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index b667f76..2141165 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -714,9 +714,10 @@ static struct class gpio_class = {
>
>
> /**
> - * gpio_export - export a GPIO through sysfs
> + * gpio_export_with_name - export a GPIO through sysfs
> * @gpio: gpio to make available, already requested
> * @direction_may_change: true if userspace may change gpio direction
> + * @name: gpio name
> * Context: arch_initcall or later
> *
> * When drivers want to make a GPIO accessible to userspace after they
> @@ -728,7 +729,7 @@ static struct class gpio_class = {
> *
> * Returns zero on success, else an error.
> */
> -int gpio_export(unsigned gpio, bool direction_may_change)
> +int gpio_export_with_name(unsigned gpio, bool direction_may_change, const char *name)
> {
> unsigned long flags;
> struct gpio_desc *desc;
> @@ -766,7 +767,9 @@ int gpio_export(unsigned gpio, bool direction_may_change)
> direction_may_change = false;
> spin_unlock_irqrestore(&gpio_lock, flags);
>
> - if (desc->chip->names && desc->chip->names[gpio - desc->chip->base])
> + if (name)
> + ioname = name;
> + else if (desc->chip->names && desc->chip->names[gpio - desc->chip->base])
> ioname = desc->chip->names[gpio - desc->chip->base];
>
> dev = device_create(&gpio_class, desc->chip->dev, MKDEV(0, 0),
> @@ -804,7 +807,7 @@ fail_unlock:
> pr_debug("%s: gpio%d status %d\n", __func__, gpio, status);
> return status;
> }
> -EXPORT_SYMBOL_GPL(gpio_export);
> +EXPORT_SYMBOL_GPL(gpio_export_with_name);
>
> static int match_export(struct device *dev, void *data)
> {
> @@ -856,7 +859,6 @@ done:
> }
> EXPORT_SYMBOL_GPL(gpio_export_link);
>
> -
> /**
> * gpio_sysfs_set_active_low - set the polarity of gpio sysfs value
> * @gpio: gpio to change
> diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> index b6516f3..d04fc55 100644
> --- a/include/asm-generic/gpio.h
> +++ b/include/asm-generic/gpio.h
> @@ -204,7 +204,8 @@ 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_with_name(unsigned gpio, bool direction_may_change,
> + const char *name);
> extern int gpio_export_link(struct device *dev, const char *name,
> unsigned gpio);
> extern int gpio_sysfs_set_active_low(unsigned gpio, int value);
> @@ -249,7 +250,8 @@ struct device;
>
> /* sysfs support is only available with gpiolib, where it's optional */
>
> -static inline int gpio_export(unsigned gpio, bool direction_may_change)
> +static inline int gpio_export_with_name(unsigned gpio,
> + bool direction_may_change, const char *name)
> {
> return -ENOSYS;
> }
> diff --git a/include/linux/gpio.h b/include/linux/gpio.h
> index 7ba2762..6e01b8c 100644
> --- a/include/linux/gpio.h
> +++ b/include/linux/gpio.h
> @@ -189,7 +189,8 @@ static inline void gpio_set_value_cansleep(unsigned gpio, int value)
> WARN_ON(1);
> }
>
> -static inline int gpio_export(unsigned gpio, bool direction_may_change)
> +static inline int gpio_export_with_name(unsigned gpio,
> + bool direction_may_change, const char *name)
> {
> /* GPIO can never have been requested or set as {in,out}put */
> WARN_ON(1);
> @@ -247,4 +248,24 @@ gpiochip_remove_pin_ranges(struct gpio_chip *chip)
>
> #endif /* ! CONFIG_GENERIC_GPIO */
>
> +/**
> + * gpio_export - export a GPIO through sysfs
> + * @gpio: gpio to make available, already requested
> + * @direction_may_change: true if userspace may change gpio direction
> + * Context: arch_initcall or later
> + *
> + * When drivers want to make a GPIO accessible to userspace after they
> + * have requested it -- perhaps while debugging, or as part of their
> + * public interface -- they may use this routine. If the GPIO can
> + * change direction (some can't) and the caller allows it, userspace
> + * will see "direction" sysfs attribute which may be used to change
> + * the gpio's direction. A "value" attribute will always be provided.
> + *
> + * Returns zero on success, else an error.
> + */
> +static inline int gpio_export(unsigned gpio,bool direction_may_change)
> +{
> + return gpio_export_with_name(gpio, direction_may_change, NULL);
> +}
> +
> #endif /* __LINUX_GPIO_H */
> --
> 1.7.10.4
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.
^ permalink raw reply [flat|nested] 30+ messages in thread* [PATCH 1/2] gpiolib: add gpio_export_with_name
2012-11-26 13:59 ` Grant Likely
@ 2012-11-28 20:19 ` Jean-Christophe PLAGNIOL-VILLARD
-1 siblings, 0 replies; 30+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2012-11-28 20:19 UTC (permalink / raw)
To: linux-arm-kernel
On 13:59 Mon 26 Nov , Grant Likely wrote:
> On Wed, 21 Nov 2012 11:14:08 +0100, Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote:
> > allow to specify a name to an exported gpio
> >
> > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
>
> The gpio sysfs ABI is already horrible, racy, and unsafe. Really, we
> need a proper chrdev interface for controlling gpios. Sysfs is fine for
> poking around and experimenting, but we cannot provide any fine grained
> access control, locking or faster IO with the one-file-per-gpio sysfs
> model.
I do agree on this and mention it on the ML multiple times but this an ABI
and we can not change it anymore we need to support it
> So, no, I don't think this is a good idea to extend gpiolib in
> this way.
>
> However, I would be okay with making gpiochips first-class kobjects or
> struct devices that appear as a child of the gpio controller device so
> that userspace could interrogate the device tree node associated with
> the device.
the problem here is that I do not want the user space to become smart
I want the user space to known which gpio use for a specific feature
as example you just want to read lack restistor to detect a hw features
you known the encoding it does not change cross hw but the pin to read do
So the idea is just to tell the user space use this pin to get your
information
how to do you want to solve this?
This apply to other case:
- chip poweron/off
- reset
etc... all control by userspace
Best Regards,
J.
>
> g.
>
> > ---
> > arch/mips/include/asm/mach-au1x00/gpio-au1000.h | 3 ++-
> > arch/mips/include/asm/mach-au1x00/gpio-au1300.h | 3 ++-
> > drivers/gpio/gpiolib.c | 12 +++++++-----
> > include/asm-generic/gpio.h | 6 ++++--
> > include/linux/gpio.h | 23 ++++++++++++++++++++++-
> > 5 files changed, 37 insertions(+), 10 deletions(-)
> >
> > diff --git a/arch/mips/include/asm/mach-au1x00/gpio-au1000.h b/arch/mips/include/asm/mach-au1x00/gpio-au1000.h
> > index 73853b5a..af4accb 100644
> > --- a/arch/mips/include/asm/mach-au1x00/gpio-au1000.h
> > +++ b/arch/mips/include/asm/mach-au1x00/gpio-au1000.h
> > @@ -633,7 +633,8 @@ static inline int gpio_set_debounce(unsigned gpio, unsigned debounce)
> > return -ENOSYS;
> > }
> >
> > -static inline int gpio_export(unsigned gpio, bool direction_may_change)
> > +static inline int gpio_export_with_name(unsigned gpio,
> > + bool direction_may_change, const char *name)
> > {
> > return -ENOSYS;
> > }
> > diff --git a/arch/mips/include/asm/mach-au1x00/gpio-au1300.h b/arch/mips/include/asm/mach-au1x00/gpio-au1300.h
> > index fb9975c..fcd152e 100644
> > --- a/arch/mips/include/asm/mach-au1x00/gpio-au1300.h
> > +++ b/arch/mips/include/asm/mach-au1x00/gpio-au1300.h
> > @@ -234,7 +234,8 @@ static inline void gpio_unexport(unsigned gpio)
> > {
> > }
> >
> > -static inline int gpio_export(unsigned gpio, bool direction_may_change)
> > +static inline int gpio_export_with_name(unsigned gpio,
> > + bool direction_may_change, const char *name)
> > {
> > return -ENOSYS;
> > }
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index b667f76..2141165 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -714,9 +714,10 @@ static struct class gpio_class = {
> >
> >
> > /**
> > - * gpio_export - export a GPIO through sysfs
> > + * gpio_export_with_name - export a GPIO through sysfs
> > * @gpio: gpio to make available, already requested
> > * @direction_may_change: true if userspace may change gpio direction
> > + * @name: gpio name
> > * Context: arch_initcall or later
> > *
> > * When drivers want to make a GPIO accessible to userspace after they
> > @@ -728,7 +729,7 @@ static struct class gpio_class = {
> > *
> > * Returns zero on success, else an error.
> > */
> > -int gpio_export(unsigned gpio, bool direction_may_change)
> > +int gpio_export_with_name(unsigned gpio, bool direction_may_change, const char *name)
> > {
> > unsigned long flags;
> > struct gpio_desc *desc;
> > @@ -766,7 +767,9 @@ int gpio_export(unsigned gpio, bool direction_may_change)
> > direction_may_change = false;
> > spin_unlock_irqrestore(&gpio_lock, flags);
> >
> > - if (desc->chip->names && desc->chip->names[gpio - desc->chip->base])
> > + if (name)
> > + ioname = name;
> > + else if (desc->chip->names && desc->chip->names[gpio - desc->chip->base])
> > ioname = desc->chip->names[gpio - desc->chip->base];
> >
> > dev = device_create(&gpio_class, desc->chip->dev, MKDEV(0, 0),
> > @@ -804,7 +807,7 @@ fail_unlock:
> > pr_debug("%s: gpio%d status %d\n", __func__, gpio, status);
> > return status;
> > }
> > -EXPORT_SYMBOL_GPL(gpio_export);
> > +EXPORT_SYMBOL_GPL(gpio_export_with_name);
> >
> > static int match_export(struct device *dev, void *data)
> > {
> > @@ -856,7 +859,6 @@ done:
> > }
> > EXPORT_SYMBOL_GPL(gpio_export_link);
> >
> > -
> > /**
> > * gpio_sysfs_set_active_low - set the polarity of gpio sysfs value
> > * @gpio: gpio to change
> > diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> > index b6516f3..d04fc55 100644
> > --- a/include/asm-generic/gpio.h
> > +++ b/include/asm-generic/gpio.h
> > @@ -204,7 +204,8 @@ 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_with_name(unsigned gpio, bool direction_may_change,
> > + const char *name);
> > extern int gpio_export_link(struct device *dev, const char *name,
> > unsigned gpio);
> > extern int gpio_sysfs_set_active_low(unsigned gpio, int value);
> > @@ -249,7 +250,8 @@ struct device;
> >
> > /* sysfs support is only available with gpiolib, where it's optional */
> >
> > -static inline int gpio_export(unsigned gpio, bool direction_may_change)
> > +static inline int gpio_export_with_name(unsigned gpio,
> > + bool direction_may_change, const char *name)
> > {
> > return -ENOSYS;
> > }
> > diff --git a/include/linux/gpio.h b/include/linux/gpio.h
> > index 7ba2762..6e01b8c 100644
> > --- a/include/linux/gpio.h
> > +++ b/include/linux/gpio.h
> > @@ -189,7 +189,8 @@ static inline void gpio_set_value_cansleep(unsigned gpio, int value)
> > WARN_ON(1);
> > }
> >
> > -static inline int gpio_export(unsigned gpio, bool direction_may_change)
> > +static inline int gpio_export_with_name(unsigned gpio,
> > + bool direction_may_change, const char *name)
> > {
> > /* GPIO can never have been requested or set as {in,out}put */
> > WARN_ON(1);
> > @@ -247,4 +248,24 @@ gpiochip_remove_pin_ranges(struct gpio_chip *chip)
> >
> > #endif /* ! CONFIG_GENERIC_GPIO */
> >
> > +/**
> > + * gpio_export - export a GPIO through sysfs
> > + * @gpio: gpio to make available, already requested
> > + * @direction_may_change: true if userspace may change gpio direction
> > + * Context: arch_initcall or later
> > + *
> > + * When drivers want to make a GPIO accessible to userspace after they
> > + * have requested it -- perhaps while debugging, or as part of their
> > + * public interface -- they may use this routine. If the GPIO can
> > + * change direction (some can't) and the caller allows it, userspace
> > + * will see "direction" sysfs attribute which may be used to change
> > + * the gpio's direction. A "value" attribute will always be provided.
> > + *
> > + * Returns zero on success, else an error.
> > + */
> > +static inline int gpio_export(unsigned gpio,bool direction_may_change)
> > +{
> > + return gpio_export_with_name(gpio, direction_may_change, NULL);
> > +}
> > +
> > #endif /* __LINUX_GPIO_H */
> > --
> > 1.7.10.4
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
> --
> Grant Likely, B.Sc, P.Eng.
> Secret Lab Technologies, Ltd.
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 1/2] gpiolib: add gpio_export_with_name
@ 2012-11-28 20:19 ` Jean-Christophe PLAGNIOL-VILLARD
0 siblings, 0 replies; 30+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2012-11-28 20:19 UTC (permalink / raw)
To: Grant Likely
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On 13:59 Mon 26 Nov , Grant Likely wrote:
> On Wed, 21 Nov 2012 11:14:08 +0100, Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org> wrote:
> > allow to specify a name to an exported gpio
> >
> > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
>
> The gpio sysfs ABI is already horrible, racy, and unsafe. Really, we
> need a proper chrdev interface for controlling gpios. Sysfs is fine for
> poking around and experimenting, but we cannot provide any fine grained
> access control, locking or faster IO with the one-file-per-gpio sysfs
> model.
I do agree on this and mention it on the ML multiple times but this an ABI
and we can not change it anymore we need to support it
> So, no, I don't think this is a good idea to extend gpiolib in
> this way.
>
> However, I would be okay with making gpiochips first-class kobjects or
> struct devices that appear as a child of the gpio controller device so
> that userspace could interrogate the device tree node associated with
> the device.
the problem here is that I do not want the user space to become smart
I want the user space to known which gpio use for a specific feature
as example you just want to read lack restistor to detect a hw features
you known the encoding it does not change cross hw but the pin to read do
So the idea is just to tell the user space use this pin to get your
information
how to do you want to solve this?
This apply to other case:
- chip poweron/off
- reset
etc... all control by userspace
Best Regards,
J.
>
> g.
>
> > ---
> > arch/mips/include/asm/mach-au1x00/gpio-au1000.h | 3 ++-
> > arch/mips/include/asm/mach-au1x00/gpio-au1300.h | 3 ++-
> > drivers/gpio/gpiolib.c | 12 +++++++-----
> > include/asm-generic/gpio.h | 6 ++++--
> > include/linux/gpio.h | 23 ++++++++++++++++++++++-
> > 5 files changed, 37 insertions(+), 10 deletions(-)
> >
> > diff --git a/arch/mips/include/asm/mach-au1x00/gpio-au1000.h b/arch/mips/include/asm/mach-au1x00/gpio-au1000.h
> > index 73853b5a..af4accb 100644
> > --- a/arch/mips/include/asm/mach-au1x00/gpio-au1000.h
> > +++ b/arch/mips/include/asm/mach-au1x00/gpio-au1000.h
> > @@ -633,7 +633,8 @@ static inline int gpio_set_debounce(unsigned gpio, unsigned debounce)
> > return -ENOSYS;
> > }
> >
> > -static inline int gpio_export(unsigned gpio, bool direction_may_change)
> > +static inline int gpio_export_with_name(unsigned gpio,
> > + bool direction_may_change, const char *name)
> > {
> > return -ENOSYS;
> > }
> > diff --git a/arch/mips/include/asm/mach-au1x00/gpio-au1300.h b/arch/mips/include/asm/mach-au1x00/gpio-au1300.h
> > index fb9975c..fcd152e 100644
> > --- a/arch/mips/include/asm/mach-au1x00/gpio-au1300.h
> > +++ b/arch/mips/include/asm/mach-au1x00/gpio-au1300.h
> > @@ -234,7 +234,8 @@ static inline void gpio_unexport(unsigned gpio)
> > {
> > }
> >
> > -static inline int gpio_export(unsigned gpio, bool direction_may_change)
> > +static inline int gpio_export_with_name(unsigned gpio,
> > + bool direction_may_change, const char *name)
> > {
> > return -ENOSYS;
> > }
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index b667f76..2141165 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -714,9 +714,10 @@ static struct class gpio_class = {
> >
> >
> > /**
> > - * gpio_export - export a GPIO through sysfs
> > + * gpio_export_with_name - export a GPIO through sysfs
> > * @gpio: gpio to make available, already requested
> > * @direction_may_change: true if userspace may change gpio direction
> > + * @name: gpio name
> > * Context: arch_initcall or later
> > *
> > * When drivers want to make a GPIO accessible to userspace after they
> > @@ -728,7 +729,7 @@ static struct class gpio_class = {
> > *
> > * Returns zero on success, else an error.
> > */
> > -int gpio_export(unsigned gpio, bool direction_may_change)
> > +int gpio_export_with_name(unsigned gpio, bool direction_may_change, const char *name)
> > {
> > unsigned long flags;
> > struct gpio_desc *desc;
> > @@ -766,7 +767,9 @@ int gpio_export(unsigned gpio, bool direction_may_change)
> > direction_may_change = false;
> > spin_unlock_irqrestore(&gpio_lock, flags);
> >
> > - if (desc->chip->names && desc->chip->names[gpio - desc->chip->base])
> > + if (name)
> > + ioname = name;
> > + else if (desc->chip->names && desc->chip->names[gpio - desc->chip->base])
> > ioname = desc->chip->names[gpio - desc->chip->base];
> >
> > dev = device_create(&gpio_class, desc->chip->dev, MKDEV(0, 0),
> > @@ -804,7 +807,7 @@ fail_unlock:
> > pr_debug("%s: gpio%d status %d\n", __func__, gpio, status);
> > return status;
> > }
> > -EXPORT_SYMBOL_GPL(gpio_export);
> > +EXPORT_SYMBOL_GPL(gpio_export_with_name);
> >
> > static int match_export(struct device *dev, void *data)
> > {
> > @@ -856,7 +859,6 @@ done:
> > }
> > EXPORT_SYMBOL_GPL(gpio_export_link);
> >
> > -
> > /**
> > * gpio_sysfs_set_active_low - set the polarity of gpio sysfs value
> > * @gpio: gpio to change
> > diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> > index b6516f3..d04fc55 100644
> > --- a/include/asm-generic/gpio.h
> > +++ b/include/asm-generic/gpio.h
> > @@ -204,7 +204,8 @@ 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_with_name(unsigned gpio, bool direction_may_change,
> > + const char *name);
> > extern int gpio_export_link(struct device *dev, const char *name,
> > unsigned gpio);
> > extern int gpio_sysfs_set_active_low(unsigned gpio, int value);
> > @@ -249,7 +250,8 @@ struct device;
> >
> > /* sysfs support is only available with gpiolib, where it's optional */
> >
> > -static inline int gpio_export(unsigned gpio, bool direction_may_change)
> > +static inline int gpio_export_with_name(unsigned gpio,
> > + bool direction_may_change, const char *name)
> > {
> > return -ENOSYS;
> > }
> > diff --git a/include/linux/gpio.h b/include/linux/gpio.h
> > index 7ba2762..6e01b8c 100644
> > --- a/include/linux/gpio.h
> > +++ b/include/linux/gpio.h
> > @@ -189,7 +189,8 @@ static inline void gpio_set_value_cansleep(unsigned gpio, int value)
> > WARN_ON(1);
> > }
> >
> > -static inline int gpio_export(unsigned gpio, bool direction_may_change)
> > +static inline int gpio_export_with_name(unsigned gpio,
> > + bool direction_may_change, const char *name)
> > {
> > /* GPIO can never have been requested or set as {in,out}put */
> > WARN_ON(1);
> > @@ -247,4 +248,24 @@ gpiochip_remove_pin_ranges(struct gpio_chip *chip)
> >
> > #endif /* ! CONFIG_GENERIC_GPIO */
> >
> > +/**
> > + * gpio_export - export a GPIO through sysfs
> > + * @gpio: gpio to make available, already requested
> > + * @direction_may_change: true if userspace may change gpio direction
> > + * Context: arch_initcall or later
> > + *
> > + * When drivers want to make a GPIO accessible to userspace after they
> > + * have requested it -- perhaps while debugging, or as part of their
> > + * public interface -- they may use this routine. If the GPIO can
> > + * change direction (some can't) and the caller allows it, userspace
> > + * will see "direction" sysfs attribute which may be used to change
> > + * the gpio's direction. A "value" attribute will always be provided.
> > + *
> > + * Returns zero on success, else an error.
> > + */
> > +static inline int gpio_export(unsigned gpio,bool direction_may_change)
> > +{
> > + return gpio_export_with_name(gpio, direction_may_change, NULL);
> > +}
> > +
> > #endif /* __LINUX_GPIO_H */
> > --
> > 1.7.10.4
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
> --
> Grant Likely, B.Sc, P.Eng.
> Secret Lab Technologies, Ltd.
^ permalink raw reply [flat|nested] 30+ messages in thread* [PATCH 1/2] gpiolib: add gpio_export_with_name
@ 2012-12-18 6:04 ` Jean-Christophe PLAGNIOL-VILLARD
0 siblings, 0 replies; 30+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2012-12-18 6:04 UTC (permalink / raw)
To: linux-arm-kernel
On 21:19 Wed 28 Nov , Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 13:59 Mon 26 Nov , Grant Likely wrote:
> > On Wed, 21 Nov 2012 11:14:08 +0100, Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote:
> > > allow to specify a name to an exported gpio
> > >
> > > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> >
> > The gpio sysfs ABI is already horrible, racy, and unsafe. Really, we
> > need a proper chrdev interface for controlling gpios. Sysfs is fine for
> > poking around and experimenting, but we cannot provide any fine grained
> > access control, locking or faster IO with the one-file-per-gpio sysfs
> > model.
> I do agree on this and mention it on the ML multiple times but this an ABI
> and we can not change it anymore we need to support it
> > So, no, I don't think this is a good idea to extend gpiolib in
> > this way.
> >
> > However, I would be okay with making gpiochips first-class kobjects or
> > struct devices that appear as a child of the gpio controller device so
> > that userspace could interrogate the device tree node associated with
> > the device.
> the problem here is that I do not want the user space to become smart
>
> I want the user space to known which gpio use for a specific feature
> as example you just want to read lack restistor to detect a hw features
>
> you known the encoding it does not change cross hw but the pin to read do
>
> So the idea is just to tell the user space use this pin to get your
> information
>
> how to do you want to solve this?
>
> This apply to other case:
> - chip poweron/off
> - reset
> etc... all control by userspace
ping
>
> Best Regards,
> J.
> >
> > g.
> >
> > > ---
> > > arch/mips/include/asm/mach-au1x00/gpio-au1000.h | 3 ++-
> > > arch/mips/include/asm/mach-au1x00/gpio-au1300.h | 3 ++-
> > > drivers/gpio/gpiolib.c | 12 +++++++-----
> > > include/asm-generic/gpio.h | 6 ++++--
> > > include/linux/gpio.h | 23 ++++++++++++++++++++++-
> > > 5 files changed, 37 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/arch/mips/include/asm/mach-au1x00/gpio-au1000.h b/arch/mips/include/asm/mach-au1x00/gpio-au1000.h
> > > index 73853b5a..af4accb 100644
> > > --- a/arch/mips/include/asm/mach-au1x00/gpio-au1000.h
> > > +++ b/arch/mips/include/asm/mach-au1x00/gpio-au1000.h
> > > @@ -633,7 +633,8 @@ static inline int gpio_set_debounce(unsigned gpio, unsigned debounce)
> > > return -ENOSYS;
> > > }
> > >
> > > -static inline int gpio_export(unsigned gpio, bool direction_may_change)
> > > +static inline int gpio_export_with_name(unsigned gpio,
> > > + bool direction_may_change, const char *name)
> > > {
> > > return -ENOSYS;
> > > }
> > > diff --git a/arch/mips/include/asm/mach-au1x00/gpio-au1300.h b/arch/mips/include/asm/mach-au1x00/gpio-au1300.h
> > > index fb9975c..fcd152e 100644
> > > --- a/arch/mips/include/asm/mach-au1x00/gpio-au1300.h
> > > +++ b/arch/mips/include/asm/mach-au1x00/gpio-au1300.h
> > > @@ -234,7 +234,8 @@ static inline void gpio_unexport(unsigned gpio)
> > > {
> > > }
> > >
> > > -static inline int gpio_export(unsigned gpio, bool direction_may_change)
> > > +static inline int gpio_export_with_name(unsigned gpio,
> > > + bool direction_may_change, const char *name)
> > > {
> > > return -ENOSYS;
> > > }
> > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > index b667f76..2141165 100644
> > > --- a/drivers/gpio/gpiolib.c
> > > +++ b/drivers/gpio/gpiolib.c
> > > @@ -714,9 +714,10 @@ static struct class gpio_class = {
> > >
> > >
> > > /**
> > > - * gpio_export - export a GPIO through sysfs
> > > + * gpio_export_with_name - export a GPIO through sysfs
> > > * @gpio: gpio to make available, already requested
> > > * @direction_may_change: true if userspace may change gpio direction
> > > + * @name: gpio name
> > > * Context: arch_initcall or later
> > > *
> > > * When drivers want to make a GPIO accessible to userspace after they
> > > @@ -728,7 +729,7 @@ static struct class gpio_class = {
> > > *
> > > * Returns zero on success, else an error.
> > > */
> > > -int gpio_export(unsigned gpio, bool direction_may_change)
> > > +int gpio_export_with_name(unsigned gpio, bool direction_may_change, const char *name)
> > > {
> > > unsigned long flags;
> > > struct gpio_desc *desc;
> > > @@ -766,7 +767,9 @@ int gpio_export(unsigned gpio, bool direction_may_change)
> > > direction_may_change = false;
> > > spin_unlock_irqrestore(&gpio_lock, flags);
> > >
> > > - if (desc->chip->names && desc->chip->names[gpio - desc->chip->base])
> > > + if (name)
> > > + ioname = name;
> > > + else if (desc->chip->names && desc->chip->names[gpio - desc->chip->base])
> > > ioname = desc->chip->names[gpio - desc->chip->base];
> > >
> > > dev = device_create(&gpio_class, desc->chip->dev, MKDEV(0, 0),
> > > @@ -804,7 +807,7 @@ fail_unlock:
> > > pr_debug("%s: gpio%d status %d\n", __func__, gpio, status);
> > > return status;
> > > }
> > > -EXPORT_SYMBOL_GPL(gpio_export);
> > > +EXPORT_SYMBOL_GPL(gpio_export_with_name);
> > >
> > > static int match_export(struct device *dev, void *data)
> > > {
> > > @@ -856,7 +859,6 @@ done:
> > > }
> > > EXPORT_SYMBOL_GPL(gpio_export_link);
> > >
> > > -
> > > /**
> > > * gpio_sysfs_set_active_low - set the polarity of gpio sysfs value
> > > * @gpio: gpio to change
> > > diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> > > index b6516f3..d04fc55 100644
> > > --- a/include/asm-generic/gpio.h
> > > +++ b/include/asm-generic/gpio.h
> > > @@ -204,7 +204,8 @@ 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_with_name(unsigned gpio, bool direction_may_change,
> > > + const char *name);
> > > extern int gpio_export_link(struct device *dev, const char *name,
> > > unsigned gpio);
> > > extern int gpio_sysfs_set_active_low(unsigned gpio, int value);
> > > @@ -249,7 +250,8 @@ struct device;
> > >
> > > /* sysfs support is only available with gpiolib, where it's optional */
> > >
> > > -static inline int gpio_export(unsigned gpio, bool direction_may_change)
> > > +static inline int gpio_export_with_name(unsigned gpio,
> > > + bool direction_may_change, const char *name)
> > > {
> > > return -ENOSYS;
> > > }
> > > diff --git a/include/linux/gpio.h b/include/linux/gpio.h
> > > index 7ba2762..6e01b8c 100644
> > > --- a/include/linux/gpio.h
> > > +++ b/include/linux/gpio.h
> > > @@ -189,7 +189,8 @@ static inline void gpio_set_value_cansleep(unsigned gpio, int value)
> > > WARN_ON(1);
> > > }
> > >
> > > -static inline int gpio_export(unsigned gpio, bool direction_may_change)
> > > +static inline int gpio_export_with_name(unsigned gpio,
> > > + bool direction_may_change, const char *name)
> > > {
> > > /* GPIO can never have been requested or set as {in,out}put */
> > > WARN_ON(1);
> > > @@ -247,4 +248,24 @@ gpiochip_remove_pin_ranges(struct gpio_chip *chip)
> > >
> > > #endif /* ! CONFIG_GENERIC_GPIO */
> > >
> > > +/**
> > > + * gpio_export - export a GPIO through sysfs
> > > + * @gpio: gpio to make available, already requested
> > > + * @direction_may_change: true if userspace may change gpio direction
> > > + * Context: arch_initcall or later
> > > + *
> > > + * When drivers want to make a GPIO accessible to userspace after they
> > > + * have requested it -- perhaps while debugging, or as part of their
> > > + * public interface -- they may use this routine. If the GPIO can
> > > + * change direction (some can't) and the caller allows it, userspace
> > > + * will see "direction" sysfs attribute which may be used to change
> > > + * the gpio's direction. A "value" attribute will always be provided.
> > > + *
> > > + * Returns zero on success, else an error.
> > > + */
> > > +static inline int gpio_export(unsigned gpio,bool direction_may_change)
> > > +{
> > > + return gpio_export_with_name(gpio, direction_may_change, NULL);
> > > +}
> > > +
> > > #endif /* __LINUX_GPIO_H */
> > > --
> > > 1.7.10.4
> > >
> > >
> > > _______________________________________________
> > > linux-arm-kernel mailing list
> > > linux-arm-kernel at lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >
> > --
> > Grant Likely, B.Sc, P.Eng.
> > Secret Lab Technologies, Ltd.
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 1/2] gpiolib: add gpio_export_with_name
@ 2012-12-18 6:04 ` Jean-Christophe PLAGNIOL-VILLARD
0 siblings, 0 replies; 30+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2012-12-18 6:04 UTC (permalink / raw)
To: Grant Likely
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On 21:19 Wed 28 Nov , Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 13:59 Mon 26 Nov , Grant Likely wrote:
> > On Wed, 21 Nov 2012 11:14:08 +0100, Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org> wrote:
> > > allow to specify a name to an exported gpio
> > >
> > > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
> >
> > The gpio sysfs ABI is already horrible, racy, and unsafe. Really, we
> > need a proper chrdev interface for controlling gpios. Sysfs is fine for
> > poking around and experimenting, but we cannot provide any fine grained
> > access control, locking or faster IO with the one-file-per-gpio sysfs
> > model.
> I do agree on this and mention it on the ML multiple times but this an ABI
> and we can not change it anymore we need to support it
> > So, no, I don't think this is a good idea to extend gpiolib in
> > this way.
> >
> > However, I would be okay with making gpiochips first-class kobjects or
> > struct devices that appear as a child of the gpio controller device so
> > that userspace could interrogate the device tree node associated with
> > the device.
> the problem here is that I do not want the user space to become smart
>
> I want the user space to known which gpio use for a specific feature
> as example you just want to read lack restistor to detect a hw features
>
> you known the encoding it does not change cross hw but the pin to read do
>
> So the idea is just to tell the user space use this pin to get your
> information
>
> how to do you want to solve this?
>
> This apply to other case:
> - chip poweron/off
> - reset
> etc... all control by userspace
ping
>
> Best Regards,
> J.
> >
> > g.
> >
> > > ---
> > > arch/mips/include/asm/mach-au1x00/gpio-au1000.h | 3 ++-
> > > arch/mips/include/asm/mach-au1x00/gpio-au1300.h | 3 ++-
> > > drivers/gpio/gpiolib.c | 12 +++++++-----
> > > include/asm-generic/gpio.h | 6 ++++--
> > > include/linux/gpio.h | 23 ++++++++++++++++++++++-
> > > 5 files changed, 37 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/arch/mips/include/asm/mach-au1x00/gpio-au1000.h b/arch/mips/include/asm/mach-au1x00/gpio-au1000.h
> > > index 73853b5a..af4accb 100644
> > > --- a/arch/mips/include/asm/mach-au1x00/gpio-au1000.h
> > > +++ b/arch/mips/include/asm/mach-au1x00/gpio-au1000.h
> > > @@ -633,7 +633,8 @@ static inline int gpio_set_debounce(unsigned gpio, unsigned debounce)
> > > return -ENOSYS;
> > > }
> > >
> > > -static inline int gpio_export(unsigned gpio, bool direction_may_change)
> > > +static inline int gpio_export_with_name(unsigned gpio,
> > > + bool direction_may_change, const char *name)
> > > {
> > > return -ENOSYS;
> > > }
> > > diff --git a/arch/mips/include/asm/mach-au1x00/gpio-au1300.h b/arch/mips/include/asm/mach-au1x00/gpio-au1300.h
> > > index fb9975c..fcd152e 100644
> > > --- a/arch/mips/include/asm/mach-au1x00/gpio-au1300.h
> > > +++ b/arch/mips/include/asm/mach-au1x00/gpio-au1300.h
> > > @@ -234,7 +234,8 @@ static inline void gpio_unexport(unsigned gpio)
> > > {
> > > }
> > >
> > > -static inline int gpio_export(unsigned gpio, bool direction_may_change)
> > > +static inline int gpio_export_with_name(unsigned gpio,
> > > + bool direction_may_change, const char *name)
> > > {
> > > return -ENOSYS;
> > > }
> > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > index b667f76..2141165 100644
> > > --- a/drivers/gpio/gpiolib.c
> > > +++ b/drivers/gpio/gpiolib.c
> > > @@ -714,9 +714,10 @@ static struct class gpio_class = {
> > >
> > >
> > > /**
> > > - * gpio_export - export a GPIO through sysfs
> > > + * gpio_export_with_name - export a GPIO through sysfs
> > > * @gpio: gpio to make available, already requested
> > > * @direction_may_change: true if userspace may change gpio direction
> > > + * @name: gpio name
> > > * Context: arch_initcall or later
> > > *
> > > * When drivers want to make a GPIO accessible to userspace after they
> > > @@ -728,7 +729,7 @@ static struct class gpio_class = {
> > > *
> > > * Returns zero on success, else an error.
> > > */
> > > -int gpio_export(unsigned gpio, bool direction_may_change)
> > > +int gpio_export_with_name(unsigned gpio, bool direction_may_change, const char *name)
> > > {
> > > unsigned long flags;
> > > struct gpio_desc *desc;
> > > @@ -766,7 +767,9 @@ int gpio_export(unsigned gpio, bool direction_may_change)
> > > direction_may_change = false;
> > > spin_unlock_irqrestore(&gpio_lock, flags);
> > >
> > > - if (desc->chip->names && desc->chip->names[gpio - desc->chip->base])
> > > + if (name)
> > > + ioname = name;
> > > + else if (desc->chip->names && desc->chip->names[gpio - desc->chip->base])
> > > ioname = desc->chip->names[gpio - desc->chip->base];
> > >
> > > dev = device_create(&gpio_class, desc->chip->dev, MKDEV(0, 0),
> > > @@ -804,7 +807,7 @@ fail_unlock:
> > > pr_debug("%s: gpio%d status %d\n", __func__, gpio, status);
> > > return status;
> > > }
> > > -EXPORT_SYMBOL_GPL(gpio_export);
> > > +EXPORT_SYMBOL_GPL(gpio_export_with_name);
> > >
> > > static int match_export(struct device *dev, void *data)
> > > {
> > > @@ -856,7 +859,6 @@ done:
> > > }
> > > EXPORT_SYMBOL_GPL(gpio_export_link);
> > >
> > > -
> > > /**
> > > * gpio_sysfs_set_active_low - set the polarity of gpio sysfs value
> > > * @gpio: gpio to change
> > > diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> > > index b6516f3..d04fc55 100644
> > > --- a/include/asm-generic/gpio.h
> > > +++ b/include/asm-generic/gpio.h
> > > @@ -204,7 +204,8 @@ 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_with_name(unsigned gpio, bool direction_may_change,
> > > + const char *name);
> > > extern int gpio_export_link(struct device *dev, const char *name,
> > > unsigned gpio);
> > > extern int gpio_sysfs_set_active_low(unsigned gpio, int value);
> > > @@ -249,7 +250,8 @@ struct device;
> > >
> > > /* sysfs support is only available with gpiolib, where it's optional */
> > >
> > > -static inline int gpio_export(unsigned gpio, bool direction_may_change)
> > > +static inline int gpio_export_with_name(unsigned gpio,
> > > + bool direction_may_change, const char *name)
> > > {
> > > return -ENOSYS;
> > > }
> > > diff --git a/include/linux/gpio.h b/include/linux/gpio.h
> > > index 7ba2762..6e01b8c 100644
> > > --- a/include/linux/gpio.h
> > > +++ b/include/linux/gpio.h
> > > @@ -189,7 +189,8 @@ static inline void gpio_set_value_cansleep(unsigned gpio, int value)
> > > WARN_ON(1);
> > > }
> > >
> > > -static inline int gpio_export(unsigned gpio, bool direction_may_change)
> > > +static inline int gpio_export_with_name(unsigned gpio,
> > > + bool direction_may_change, const char *name)
> > > {
> > > /* GPIO can never have been requested or set as {in,out}put */
> > > WARN_ON(1);
> > > @@ -247,4 +248,24 @@ gpiochip_remove_pin_ranges(struct gpio_chip *chip)
> > >
> > > #endif /* ! CONFIG_GENERIC_GPIO */
> > >
> > > +/**
> > > + * gpio_export - export a GPIO through sysfs
> > > + * @gpio: gpio to make available, already requested
> > > + * @direction_may_change: true if userspace may change gpio direction
> > > + * Context: arch_initcall or later
> > > + *
> > > + * When drivers want to make a GPIO accessible to userspace after they
> > > + * have requested it -- perhaps while debugging, or as part of their
> > > + * public interface -- they may use this routine. If the GPIO can
> > > + * change direction (some can't) and the caller allows it, userspace
> > > + * will see "direction" sysfs attribute which may be used to change
> > > + * the gpio's direction. A "value" attribute will always be provided.
> > > + *
> > > + * Returns zero on success, else an error.
> > > + */
> > > +static inline int gpio_export(unsigned gpio,bool direction_may_change)
> > > +{
> > > + return gpio_export_with_name(gpio, direction_may_change, NULL);
> > > +}
> > > +
> > > #endif /* __LINUX_GPIO_H */
> > > --
> > > 1.7.10.4
> > >
> > >
> > > _______________________________________________
> > > linux-arm-kernel mailing list
> > > linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >
> > --
> > Grant Likely, B.Sc, P.Eng.
> > Secret Lab Technologies, Ltd.
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
^ permalink raw reply [flat|nested] 30+ messages in thread* [PATCH 1/2] gpiolib: add gpio_export_with_name
@ 2012-12-19 13:11 ` Grant Likely
0 siblings, 0 replies; 30+ messages in thread
From: Grant Likely @ 2012-12-19 13:11 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, 18 Dec 2012 07:04:15 +0100, Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote:
> On 21:19 Wed 28 Nov , Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 13:59 Mon 26 Nov , Grant Likely wrote:
> > > On Wed, 21 Nov 2012 11:14:08 +0100, Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote:
> > > > allow to specify a name to an exported gpio
> > > >
> > > > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > >
> > > The gpio sysfs ABI is already horrible, racy, and unsafe. Really, we
> > > need a proper chrdev interface for controlling gpios. Sysfs is fine for
> > > poking around and experimenting, but we cannot provide any fine grained
> > > access control, locking or faster IO with the one-file-per-gpio sysfs
> > > model.
> > I do agree on this and mention it on the ML multiple times but this an ABI
> > and we can not change it anymore we need to support it
> > > So, no, I don't think this is a good idea to extend gpiolib in
> > > this way.
> > >
> > > However, I would be okay with making gpiochips first-class kobjects or
> > > struct devices that appear as a child of the gpio controller device so
> > > that userspace could interrogate the device tree node associated with
> > > the device.
> > the problem here is that I do not want the user space to become smart
> >
> > I want the user space to known which gpio use for a specific feature
> > as example you just want to read lack restistor to detect a hw features
> >
> > you known the encoding it does not change cross hw but the pin to read do
> >
> > So the idea is just to tell the user space use this pin to get your
> > information
> >
> > how to do you want to solve this?
> >
> > This apply to other case:
> > - chip poweron/off
> > - reset
> > etc... all control by userspace
>
> ping
I've already stated in other thread that I want to see a significantly
better GPIO interface created. Something based on a char dev and can
handle 'bursts' of gpio manipulations/reads. I'm not going to accept
extensions to the sysfs interface.
Within that context I would consider named subsets of the same interface
that pre-packages a set of related gpio pins.
That said, I'm not convinced that direct gpio access is the abstraction
that you want. If it is an interface for, say, reading feature bits,
then it probably does want to be an actual driver that reads the
gpios/registers/adc/etc and returns the parsed status in a file. That's
the only way to reliably get the abstraction from your example above.
g.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] gpiolib: add gpio_export_with_name
@ 2012-12-19 13:11 ` Grant Likely
0 siblings, 0 replies; 30+ messages in thread
From: Grant Likely @ 2012-12-19 13:11 UTC (permalink / raw)
To: Jean-Christophe PLAGNIOL-VILLARD
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On Tue, 18 Dec 2012 07:04:15 +0100, Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org> wrote:
> On 21:19 Wed 28 Nov , Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 13:59 Mon 26 Nov , Grant Likely wrote:
> > > On Wed, 21 Nov 2012 11:14:08 +0100, Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org> wrote:
> > > > allow to specify a name to an exported gpio
> > > >
> > > > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
> > >
> > > The gpio sysfs ABI is already horrible, racy, and unsafe. Really, we
> > > need a proper chrdev interface for controlling gpios. Sysfs is fine for
> > > poking around and experimenting, but we cannot provide any fine grained
> > > access control, locking or faster IO with the one-file-per-gpio sysfs
> > > model.
> > I do agree on this and mention it on the ML multiple times but this an ABI
> > and we can not change it anymore we need to support it
> > > So, no, I don't think this is a good idea to extend gpiolib in
> > > this way.
> > >
> > > However, I would be okay with making gpiochips first-class kobjects or
> > > struct devices that appear as a child of the gpio controller device so
> > > that userspace could interrogate the device tree node associated with
> > > the device.
> > the problem here is that I do not want the user space to become smart
> >
> > I want the user space to known which gpio use for a specific feature
> > as example you just want to read lack restistor to detect a hw features
> >
> > you known the encoding it does not change cross hw but the pin to read do
> >
> > So the idea is just to tell the user space use this pin to get your
> > information
> >
> > how to do you want to solve this?
> >
> > This apply to other case:
> > - chip poweron/off
> > - reset
> > etc... all control by userspace
>
> ping
I've already stated in other thread that I want to see a significantly
better GPIO interface created. Something based on a char dev and can
handle 'bursts' of gpio manipulations/reads. I'm not going to accept
extensions to the sysfs interface.
Within that context I would consider named subsets of the same interface
that pre-packages a set of related gpio pins.
That said, I'm not convinced that direct gpio access is the abstraction
that you want. If it is an interface for, say, reading feature bits,
then it probably does want to be an actual driver that reads the
gpios/registers/adc/etc and returns the parsed status in a file. That's
the only way to reliably get the abstraction from your example above.
g.
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 1/2] gpiolib: add gpio_export_with_name
2012-11-26 13:59 ` Grant Likely
@ 2013-05-30 9:19 ` Florian Vaussard
-1 siblings, 0 replies; 30+ messages in thread
From: Florian Vaussard @ 2013-05-30 9:19 UTC (permalink / raw)
To: linux-arm-kernel
Hello Grant,
On 11/26/2012 02:59 PM, Grant Likely wrote:
> On Wed, 21 Nov 2012 11:14:08 +0100, Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote:
>> allow to specify a name to an exported gpio
>>
>> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
>
> The gpio sysfs ABI is already horrible, racy, and unsafe. Really, we
> need a proper chrdev interface for controlling gpios. Sysfs is fine for
> poking around and experimenting, but we cannot provide any fine grained
> access control, locking or faster IO with the one-file-per-gpio sysfs
> model. So, no, I don't think this is a good idea to extend gpiolib in
> this way.
>
Would it make sense to provide only the DT binding to export a GPIO,
without changing the sysfs ABI? Even if work is progressing towards
having gpio-controlled reset pins [1], some boards still need GPIOs to
be exported to userspace for other functionalities.
Regards,
Florian
[1] http://article.gmane.org/gmane.linux.power-management.general/31364
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] gpiolib: add gpio_export_with_name
@ 2013-05-30 9:19 ` Florian Vaussard
0 siblings, 0 replies; 30+ messages in thread
From: Florian Vaussard @ 2013-05-30 9:19 UTC (permalink / raw)
To: Grant Likely
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, Philipp Zabel,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Hello Grant,
On 11/26/2012 02:59 PM, Grant Likely wrote:
> On Wed, 21 Nov 2012 11:14:08 +0100, Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org> wrote:
>> allow to specify a name to an exported gpio
>>
>> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
>
> The gpio sysfs ABI is already horrible, racy, and unsafe. Really, we
> need a proper chrdev interface for controlling gpios. Sysfs is fine for
> poking around and experimenting, but we cannot provide any fine grained
> access control, locking or faster IO with the one-file-per-gpio sysfs
> model. So, no, I don't think this is a good idea to extend gpiolib in
> this way.
>
Would it make sense to provide only the DT binding to export a GPIO,
without changing the sysfs ABI? Even if work is progressing towards
having gpio-controlled reset pins [1], some boards still need GPIOs to
be exported to userspace for other functionalities.
Regards,
Florian
[1] http://article.gmane.org/gmane.linux.power-management.general/31364
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 1/2] gpiolib: add gpio_export_with_name
@ 2013-05-30 20:03 ` Linus Walleij
0 siblings, 0 replies; 30+ messages in thread
From: Linus Walleij @ 2013-05-30 20:03 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, May 30, 2013 at 11:19 AM, Florian Vaussard
<florian.vaussard@epfl.ch> wrote:
> Even if work is progressing towards
> having gpio-controlled reset pins [1], some boards still need GPIOs to
> be exported to userspace for other functionalities.
Which are these?
When I have inquired I have heard all kind of horrible things:
- Userspace replicating drivers/leds/leds-gpio.c to
turn on/off and even PWM (!) LEDs from userspace, when we have
a standard sysfs interface for LEDs.
- Userspace replicating drivers/input/keyboard/gpio_keys[_polled].c
to "just read this one button" instead of going through the
Linux input subsystem like everyone else.
- Userspace replicating drivers/regulator/gpio-regulator.c
to "turn on just this one voltage source".
All invalid reasons for using the sysfs ABI and trying to do the
kernels work. All creating horrs for users and developers who now
have to stash everything were trying to stach into the device tree
(the above is all perfectly expressed with DT nodes) into their
userspace app and then requiring their users to match a certain
app to a certain board.
The only examples I've really come to accept considers using
things like relays and switches on factory lines where the meaning
(semantics) of the GPIOs can only be properly understood from the
GUI in the userspace APP running that factory line. Or robotics.
In both cases presumably RT processes.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] gpiolib: add gpio_export_with_name
@ 2013-05-30 20:03 ` Linus Walleij
0 siblings, 0 replies; 30+ messages in thread
From: Linus Walleij @ 2013-05-30 20:03 UTC (permalink / raw)
To: Florian Vaussard
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
Philipp Zabel,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
On Thu, May 30, 2013 at 11:19 AM, Florian Vaussard
<florian.vaussard-p8DiymsW2f8@public.gmane.org> wrote:
> Even if work is progressing towards
> having gpio-controlled reset pins [1], some boards still need GPIOs to
> be exported to userspace for other functionalities.
Which are these?
When I have inquired I have heard all kind of horrible things:
- Userspace replicating drivers/leds/leds-gpio.c to
turn on/off and even PWM (!) LEDs from userspace, when we have
a standard sysfs interface for LEDs.
- Userspace replicating drivers/input/keyboard/gpio_keys[_polled].c
to "just read this one button" instead of going through the
Linux input subsystem like everyone else.
- Userspace replicating drivers/regulator/gpio-regulator.c
to "turn on just this one voltage source".
All invalid reasons for using the sysfs ABI and trying to do the
kernels work. All creating horrs for users and developers who now
have to stash everything were trying to stach into the device tree
(the above is all perfectly expressed with DT nodes) into their
userspace app and then requiring their users to match a certain
app to a certain board.
The only examples I've really come to accept considers using
things like relays and switches on factory lines where the meaning
(semantics) of the GPIOs can only be properly understood from the
GUI in the userspace APP running that factory line. Or robotics.
In both cases presumably RT processes.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 1/2] gpiolib: add gpio_export_with_name
@ 2013-05-30 20:38 ` Florian Vaussard
0 siblings, 0 replies; 30+ messages in thread
From: Florian Vaussard @ 2013-05-30 20:38 UTC (permalink / raw)
To: linux-arm-kernel
Hello Linus,
On 05/30/2013 10:03 PM, Linus Walleij wrote:
> On Thu, May 30, 2013 at 11:19 AM, Florian Vaussard
> <florian.vaussard@epfl.ch> wrote:
>
>> Even if work is progressing towards
>> having gpio-controlled reset pins [1], some boards still need GPIOs to
>> be exported to userspace for other functionalities.
>
> Which are these?
>
Sorry, I should have been more precise.
> When I have inquired I have heard all kind of horrible things:
>
> - Userspace replicating drivers/leds/leds-gpio.c to
> turn on/off and even PWM (!) LEDs from userspace, when we have
> a standard sysfs interface for LEDs.
>
> - Userspace replicating drivers/input/keyboard/gpio_keys[_polled].c
> to "just read this one button" instead of going through the
> Linux input subsystem like everyone else.
>
> - Userspace replicating drivers/regulator/gpio-regulator.c
> to "turn on just this one voltage source".
>
> All invalid reasons for using the sysfs ABI and trying to do the
> kernels work. All creating horrs for users and developers who now
> have to stash everything were trying to stach into the device tree
> (the above is all perfectly expressed with DT nodes) into their
> userspace app and then requiring their users to match a certain
> app to a certain board.
>
100% agree with you.
> The only examples I've really come to accept considers using
> things like relays and switches on factory lines where the meaning
> (semantics) of the GPIOs can only be properly understood from the
> GUI in the userspace APP running that factory line. Or robotics.
> In both cases presumably RT processes.
>
Indeed, I work in a robotics lab :-) One of our board (mach-imx/
mx31moboard*.c) controls for example the multiplexing of two cameras
sharing the same acquisition bus (I know, a bit hackish).
I developed a similar board with an OMAP3. Such control do not
fit well in the above-mentioned cases, but we do not need RT processes.
Best regards,
Florian
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] gpiolib: add gpio_export_with_name
@ 2013-05-30 20:38 ` Florian Vaussard
0 siblings, 0 replies; 30+ messages in thread
From: Florian Vaussard @ 2013-05-30 20:38 UTC (permalink / raw)
To: Linus Walleij
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
Philipp Zabel,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
Hello Linus,
On 05/30/2013 10:03 PM, Linus Walleij wrote:
> On Thu, May 30, 2013 at 11:19 AM, Florian Vaussard
> <florian.vaussard-p8DiymsW2f8@public.gmane.org> wrote:
>
>> Even if work is progressing towards
>> having gpio-controlled reset pins [1], some boards still need GPIOs to
>> be exported to userspace for other functionalities.
>
> Which are these?
>
Sorry, I should have been more precise.
> When I have inquired I have heard all kind of horrible things:
>
> - Userspace replicating drivers/leds/leds-gpio.c to
> turn on/off and even PWM (!) LEDs from userspace, when we have
> a standard sysfs interface for LEDs.
>
> - Userspace replicating drivers/input/keyboard/gpio_keys[_polled].c
> to "just read this one button" instead of going through the
> Linux input subsystem like everyone else.
>
> - Userspace replicating drivers/regulator/gpio-regulator.c
> to "turn on just this one voltage source".
>
> All invalid reasons for using the sysfs ABI and trying to do the
> kernels work. All creating horrs for users and developers who now
> have to stash everything were trying to stach into the device tree
> (the above is all perfectly expressed with DT nodes) into their
> userspace app and then requiring their users to match a certain
> app to a certain board.
>
100% agree with you.
> The only examples I've really come to accept considers using
> things like relays and switches on factory lines where the meaning
> (semantics) of the GPIOs can only be properly understood from the
> GUI in the userspace APP running that factory line. Or robotics.
> In both cases presumably RT processes.
>
Indeed, I work in a robotics lab :-) One of our board (mach-imx/
mx31moboard*.c) controls for example the multiplexing of two cameras
sharing the same acquisition bus (I know, a bit hackish).
I developed a similar board with an OMAP3. Such control do not
fit well in the above-mentioned cases, but we do not need RT processes.
Best regards,
Florian
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 1/2] gpiolib: add gpio_export_with_name
@ 2013-05-30 21:11 ` Linus Walleij
0 siblings, 0 replies; 30+ messages in thread
From: Linus Walleij @ 2013-05-30 21:11 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, May 30, 2013 at 10:38 PM, Florian Vaussard
<florian.vaussard@epfl.ch> wrote:
> Indeed, I work in a robotics lab :-) One of our board (mach-imx/
> mx31moboard*.c) controls for example the multiplexing of two cameras
> sharing the same acquisition bus (I know, a bit hackish).
> I developed a similar board with an OMAP3. Such control do not
> fit well in the above-mentioned cases, but we do not need RT processes.
It does seem to fit well with V4L and/or the media controller
framework though, or is this one of those "oh, we don't really
like to use V4L so we have this special hack instead" things?
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] gpiolib: add gpio_export_with_name
@ 2013-05-30 21:11 ` Linus Walleij
0 siblings, 0 replies; 30+ messages in thread
From: Linus Walleij @ 2013-05-30 21:11 UTC (permalink / raw)
To: Florian Vaussard
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
Philipp Zabel,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
On Thu, May 30, 2013 at 10:38 PM, Florian Vaussard
<florian.vaussard-p8DiymsW2f8@public.gmane.org> wrote:
> Indeed, I work in a robotics lab :-) One of our board (mach-imx/
> mx31moboard*.c) controls for example the multiplexing of two cameras
> sharing the same acquisition bus (I know, a bit hackish).
> I developed a similar board with an OMAP3. Such control do not
> fit well in the above-mentioned cases, but we do not need RT processes.
It does seem to fit well with V4L and/or the media controller
framework though, or is this one of those "oh, we don't really
like to use V4L so we have this special hack instead" things?
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 1/2] gpiolib: add gpio_export_with_name
@ 2013-05-31 8:17 ` Florian Vaussard
0 siblings, 0 replies; 30+ messages in thread
From: Florian Vaussard @ 2013-05-31 8:17 UTC (permalink / raw)
To: linux-arm-kernel
Hello Linus,
On 05/30/2013 11:11 PM, Linus Walleij wrote:
> On Thu, May 30, 2013 at 10:38 PM, Florian Vaussard
> <florian.vaussard@epfl.ch> wrote:
>
>> Indeed, I work in a robotics lab :-) One of our board (mach-imx/
>> mx31moboard*.c) controls for example the multiplexing of two cameras
>> sharing the same acquisition bus (I know, a bit hackish).
>> I developed a similar board with an OMAP3. Such control do not
>> fit well in the above-mentioned cases, but we do not need RT processes.
>
> It does seem to fit well with V4L and/or the media controller
> framework though, or is this one of those "oh, we don't really
> like to use V4L so we have this special hack instead" things?
>
Sorry, I was not aware of this mux capability inside V4L. Thank
you. So we can live without gpio-export for most of our hardware.
Regards,
Florian
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] gpiolib: add gpio_export_with_name
@ 2013-05-31 8:17 ` Florian Vaussard
0 siblings, 0 replies; 30+ messages in thread
From: Florian Vaussard @ 2013-05-31 8:17 UTC (permalink / raw)
To: Linus Walleij
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
Philipp Zabel,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
Hello Linus,
On 05/30/2013 11:11 PM, Linus Walleij wrote:
> On Thu, May 30, 2013 at 10:38 PM, Florian Vaussard
> <florian.vaussard-p8DiymsW2f8@public.gmane.org> wrote:
>
>> Indeed, I work in a robotics lab :-) One of our board (mach-imx/
>> mx31moboard*.c) controls for example the multiplexing of two cameras
>> sharing the same acquisition bus (I know, a bit hackish).
>> I developed a similar board with an OMAP3. Such control do not
>> fit well in the above-mentioned cases, but we do not need RT processes.
>
> It does seem to fit well with V4L and/or the media controller
> framework though, or is this one of those "oh, we don't really
> like to use V4L so we have this special hack instead" things?
>
Sorry, I was not aware of this mux capability inside V4L. Thank
you. So we can live without gpio-export for most of our hardware.
Regards,
Florian
^ permalink raw reply [flat|nested] 30+ messages in thread