All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH 0/2] fixing the gpio ownership
@ 2018-01-15 16:22 ` Ludovic Desroches
  0 siblings, 0 replies; 23+ messages in thread
From: Ludovic Desroches @ 2018-01-15 16:22 UTC (permalink / raw)
  To: linux-gpio, linux-arm-kernel
  Cc: linus.walleij, linux-kernel, nicolas.free, Ludovic Desroches

Hi,

A few weeks ago, I have sent an RFC about adding bias support for GPIOs [1].

It was motivated by the fact that I wanted to enable the pinmuxing strict mode
for my pin controller which can muxed a pin as a peripheral or as a GPIO.

Enabling the strict mode prevents several devices to be probed because
requesting a GPIO fails. The pin request function complains about the
ownership of the GPIO which is different from the mux ownership. I have to
remove my pinctrl node to avoid this conflict but I need it to configure my
pins and to set a pull-up bias for my GPIOs.

My first idea was to add new flags in addition to GPIO_ACTIVE_HIGH and others.
Obviously, it was not the way to go since many new flags may be added:
strength, debounce, etc.

Then I proposed a very "quick and dirty" patch to give the picture of what I
have in mind but I had no feedback. It was probably too dirty. The idea was
to add a cell to the gpios property with a phandle on a pinctrl node which
contains only the pinconf, no pinmux. The configuration is applied later when
requesting the GPIO. The main issue is that enabling the strict mode will
break old DTBs. I was going to submit patches for this but, after using the
sysfs which still show me a bad ownership, I decided that it should be fixed.

So I did these patches. Unfortunately, there are several ways to lead to
gpiod_request(). It does the trick only for the gpiod_get family. The issue is
still present with legacy gpio_request and fwnode_get_named_gpiod. It seems
that more and more drivers are converted to use GPIO descriptors so there is
some hope. The advantage of this solution is to not break old DTBs. As I am
not aware of all usage of the gpiolib, I tried to implement it in the safest
way.

Regards

Ludovic

[1] https://www.spinics.net/lists/arm-kernel/msg623149.html


Ludovic Desroches (2):
  pinctrl: add consumer variant for gpio request
  gpio: provide a consumer when requesting a gpio

 drivers/gpio/gpiolib.c           | 40 +++++++++++++++++++++++++++++++++-------
 drivers/pinctrl/core.c           | 13 ++++++++++---
 drivers/pinctrl/pinmux.c         | 16 ++++++++++++++--
 drivers/pinctrl/pinmux.h         | 10 ++++++++++
 include/linux/gpio/driver.h      |  5 +++++
 include/linux/pinctrl/consumer.h |  6 ++++++
 6 files changed, 78 insertions(+), 12 deletions(-)

-- 
2.12.2


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [RFC PATCH 0/2] fixing the gpio ownership
@ 2018-01-15 16:22 ` Ludovic Desroches
  0 siblings, 0 replies; 23+ messages in thread
From: Ludovic Desroches @ 2018-01-15 16:22 UTC (permalink / raw)
  To: linux-gpio, linux-arm-kernel
  Cc: linus.walleij, linux-kernel, nicolas.free, Ludovic Desroches

Hi,

A few weeks ago, I have sent an RFC about adding bias support for GPIOs [1].

It was motivated by the fact that I wanted to enable the pinmuxing strict mode
for my pin controller which can muxed a pin as a peripheral or as a GPIO.

Enabling the strict mode prevents several devices to be probed because
requesting a GPIO fails. The pin request function complains about the
ownership of the GPIO which is different from the mux ownership. I have to
remove my pinctrl node to avoid this conflict but I need it to configure my
pins and to set a pull-up bias for my GPIOs.

My first idea was to add new flags in addition to GPIO_ACTIVE_HIGH and others.
Obviously, it was not the way to go since many new flags may be added:
strength, debounce, etc.

Then I proposed a very "quick and dirty" patch to give the picture of what I
have in mind but I had no feedback. It was probably too dirty. The idea was
to add a cell to the gpios property with a phandle on a pinctrl node which
contains only the pinconf, no pinmux. The configuration is applied later when
requesting the GPIO. The main issue is that enabling the strict mode will
break old DTBs. I was going to submit patches for this but, after using the
sysfs which still show me a bad ownership, I decided that it should be fixed.

So I did these patches. Unfortunately, there are several ways to lead to
gpiod_request(). It does the trick only for the gpiod_get family. The issue is
still present with legacy gpio_request and fwnode_get_named_gpiod. It seems
that more and more drivers are converted to use GPIO descriptors so there is
some hope. The advantage of this solution is to not break old DTBs. As I am
not aware of all usage of the gpiolib, I tried to implement it in the safest
way.

Regards

Ludovic

[1] https://www.spinics.net/lists/arm-kernel/msg623149.html


Ludovic Desroches (2):
  pinctrl: add consumer variant for gpio request
  gpio: provide a consumer when requesting a gpio

 drivers/gpio/gpiolib.c           | 40 +++++++++++++++++++++++++++++++++-------
 drivers/pinctrl/core.c           | 13 ++++++++++---
 drivers/pinctrl/pinmux.c         | 16 ++++++++++++++--
 drivers/pinctrl/pinmux.h         | 10 ++++++++++
 include/linux/gpio/driver.h      |  5 +++++
 include/linux/pinctrl/consumer.h |  6 ++++++
 6 files changed, 78 insertions(+), 12 deletions(-)

-- 
2.12.2

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [RFC PATCH 0/2] fixing the gpio ownership
@ 2018-01-15 16:22 ` Ludovic Desroches
  0 siblings, 0 replies; 23+ messages in thread
From: Ludovic Desroches @ 2018-01-15 16:22 UTC (permalink / raw)
  To: linux-arm-kernel

Hi,

A few weeks ago, I have sent an RFC about adding bias support for GPIOs [1].

It was motivated by the fact that I wanted to enable the pinmuxing strict mode
for my pin controller which can muxed a pin as a peripheral or as a GPIO.

Enabling the strict mode prevents several devices to be probed because
requesting a GPIO fails. The pin request function complains about the
ownership of the GPIO which is different from the mux ownership. I have to
remove my pinctrl node to avoid this conflict but I need it to configure my
pins and to set a pull-up bias for my GPIOs.

My first idea was to add new flags in addition to GPIO_ACTIVE_HIGH and others.
Obviously, it was not the way to go since many new flags may be added:
strength, debounce, etc.

Then I proposed a very "quick and dirty" patch to give the picture of what I
have in mind but I had no feedback. It was probably too dirty. The idea was
to add a cell to the gpios property with a phandle on a pinctrl node which
contains only the pinconf, no pinmux. The configuration is applied later when
requesting the GPIO. The main issue is that enabling the strict mode will
break old DTBs. I was going to submit patches for this but, after using the
sysfs which still show me a bad ownership, I decided that it should be fixed.

So I did these patches. Unfortunately, there are several ways to lead to
gpiod_request(). It does the trick only for the gpiod_get family. The issue is
still present with legacy gpio_request and fwnode_get_named_gpiod. It seems
that more and more drivers are converted to use GPIO descriptors so there is
some hope. The advantage of this solution is to not break old DTBs. As I am
not aware of all usage of the gpiolib, I tried to implement it in the safest
way.

Regards

Ludovic

[1] https://www.spinics.net/lists/arm-kernel/msg623149.html


Ludovic Desroches (2):
  pinctrl: add consumer variant for gpio request
  gpio: provide a consumer when requesting a gpio

 drivers/gpio/gpiolib.c           | 40 +++++++++++++++++++++++++++++++++-------
 drivers/pinctrl/core.c           | 13 ++++++++++---
 drivers/pinctrl/pinmux.c         | 16 ++++++++++++++--
 drivers/pinctrl/pinmux.h         | 10 ++++++++++
 include/linux/gpio/driver.h      |  5 +++++
 include/linux/pinctrl/consumer.h |  6 ++++++
 6 files changed, 78 insertions(+), 12 deletions(-)

-- 
2.12.2

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [RFC PATCH 1/2] pinctrl: add consumer variant for gpio request
  2018-01-15 16:22 ` Ludovic Desroches
  (?)
@ 2018-01-15 16:22   ` Ludovic Desroches
  -1 siblings, 0 replies; 23+ messages in thread
From: Ludovic Desroches @ 2018-01-15 16:22 UTC (permalink / raw)
  To: linux-gpio, linux-arm-kernel
  Cc: linus.walleij, linux-kernel, nicolas.free, Ludovic Desroches

Add a consumer variant to GPIO request relative functions. The goal
is to fix the bad ownership, which is arbitrary set to
"range->name:gpio", of a GPIO.

There is a lack of configuration features for GPIO. For instance,
we can't set the bias. Some pin controllers manage both device's
pins and GPIOs. GPIOs can benefit from pin configuration. Usually,
a pinctrl node is used to mux the pin as a GPIO and to set up its
configuration.

The pinmuxing strict mode involves that a pin which is muxed can't
be requested as a GPIO if the owner is not the same. Unfortunately,
the ownership of a GPIO is set arbitrarily to "range->name:gpio".
So there is a mismatch about the ownership which prevents a device
from being the owner of the pinmuxing and requesting the same pin as
a GPIO.

Adding some consumer variants for GPIO request stuff will allow to
pass the name of the device which requests the GPIO to not return an
error if it's also the owner of the pinmuxing.

Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com>
---
 drivers/pinctrl/core.c           | 13 ++++++++++---
 drivers/pinctrl/pinmux.c         | 16 ++++++++++++++--
 drivers/pinctrl/pinmux.h         | 10 ++++++++++
 include/linux/pinctrl/consumer.h |  6 ++++++
 4 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 2c0dbfcff3e6..51c75a6057b2 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -733,14 +733,15 @@ int pinctrl_get_group_selector(struct pinctrl_dev *pctldev,
 }
 
 /**
- * pinctrl_gpio_request() - request a single pin to be used as GPIO
+ * pinctrl_gpio_request_consumer() - request a single pin to be used as GPIO
  * @gpio: the GPIO pin number from the GPIO subsystem number space
+ * @consumer: the name of the device requesting the GPIO
  *
  * This function should *ONLY* be used from gpiolib-based GPIO drivers,
  * as part of their gpio_request() semantics, platforms and individual drivers
  * shall *NOT* request GPIO pins to be muxed in.
  */
-int pinctrl_gpio_request(unsigned gpio)
+int pinctrl_gpio_request_consumer(unsigned gpio, const char *consumer)
 {
 	struct pinctrl_dev *pctldev;
 	struct pinctrl_gpio_range *range;
@@ -759,12 +760,18 @@ int pinctrl_gpio_request(unsigned gpio)
 	/* Convert to the pin controllers number space */
 	pin = gpio_to_pin(range, gpio);
 
-	ret = pinmux_request_gpio(pctldev, range, pin, gpio);
+	ret = pinmux_request_gpio_consumer(pctldev, range, pin, gpio, consumer);
 
 	mutex_unlock(&pctldev->mutex);
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(pinctrl_gpio_request_consumer);
+
+int pinctrl_gpio_request(unsigned gpio)
+{
+	return pinctrl_gpio_request_consumer(gpio, NULL);
+}
 EXPORT_SYMBOL_GPL(pinctrl_gpio_request);
 
 /**
diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index 55502fc4479c..8d422eb0ed38 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -232,14 +232,19 @@ static const char *pin_free(struct pinctrl_dev *pctldev, int pin,
  * @pctldev: pin controller device affected
  * @pin: the pin to mux in for GPIO
  * @range: the applicable GPIO range
+ * @consumer: the name of the device requesting the GPIO
  */
-int pinmux_request_gpio(struct pinctrl_dev *pctldev,
+int pinmux_request_gpio_consumer(struct pinctrl_dev *pctldev,
 			struct pinctrl_gpio_range *range,
-			unsigned pin, unsigned gpio)
+			unsigned pin, unsigned gpio,
+			const char *consumer)
 {
 	const char *owner;
 	int ret;
 
+	if (consumer)
+		return pin_request(pctldev, pin, consumer, range);
+
 	/* Conjure some name stating what chip and pin this is taken by */
 	owner = kasprintf(GFP_KERNEL, "%s:%d", range->name, gpio);
 	if (!owner)
@@ -252,6 +257,13 @@ int pinmux_request_gpio(struct pinctrl_dev *pctldev,
 	return ret;
 }
 
+int pinmux_request_gpio(struct pinctrl_dev *pctldev,
+			struct pinctrl_gpio_range *range,
+			unsigned pin, unsigned gpio)
+{
+	return pinmux_request_gpio_consumer(pctldev, range, pin, gpio, NULL);
+}
+
 /**
  * pinmux_free_gpio() - release a pin from GPIO muxing
  * @pctldev: the pin controller device for the pin
diff --git a/drivers/pinctrl/pinmux.h b/drivers/pinctrl/pinmux.h
index a331fcdbedd9..837599922a42 100644
--- a/drivers/pinctrl/pinmux.h
+++ b/drivers/pinctrl/pinmux.h
@@ -19,6 +19,9 @@ int pinmux_validate_map(const struct pinctrl_map *map, int i);
 int pinmux_request_gpio(struct pinctrl_dev *pctldev,
 			struct pinctrl_gpio_range *range,
 			unsigned pin, unsigned gpio);
+int pinmux_request_gpio_consumer(struct pinctrl_dev *pctldev,
+			struct pinctrl_gpio_range *range,
+			unsigned pin, unsigned gpio, const char *consumer);
 void pinmux_free_gpio(struct pinctrl_dev *pctldev, unsigned pin,
 		      struct pinctrl_gpio_range *range);
 int pinmux_gpio_direction(struct pinctrl_dev *pctldev,
@@ -50,6 +53,13 @@ static inline int pinmux_request_gpio(struct pinctrl_dev *pctldev,
 	return 0;
 }
 
+static inline int pinmux_request_gpio_consumer(struct pinctrl_dev *pctldev,
+			struct pinctrl_gpio_range *range,
+			unsigned pin, unsigned gpio, const char *consumer)
+{
+	return 0;
+}
+
 static inline void pinmux_free_gpio(struct pinctrl_dev *pctldev,
 				    unsigned pin,
 				    struct pinctrl_gpio_range *range)
diff --git a/include/linux/pinctrl/consumer.h b/include/linux/pinctrl/consumer.h
index 0412cc9833e9..8c521a14db43 100644
--- a/include/linux/pinctrl/consumer.h
+++ b/include/linux/pinctrl/consumer.h
@@ -26,6 +26,7 @@ struct device;
 
 /* External interface to pin control */
 extern int pinctrl_gpio_request(unsigned gpio);
+extern int pinctrl_gpio_request_consumer(unsigned gpio, const char *consumer);
 extern void pinctrl_gpio_free(unsigned gpio);
 extern int pinctrl_gpio_direction_input(unsigned gpio);
 extern int pinctrl_gpio_direction_output(unsigned gpio);
@@ -67,6 +68,11 @@ static inline int pinctrl_gpio_request(unsigned gpio)
 	return 0;
 }
 
+static inline int pinctrl_gpio_request_consumer(unsigned gpio, const char *consumer);
+{
+	return 0;
+}
+
 static inline void pinctrl_gpio_free(unsigned gpio)
 {
 }
-- 
2.12.2

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [RFC PATCH 1/2] pinctrl: add consumer variant for gpio request
@ 2018-01-15 16:22   ` Ludovic Desroches
  0 siblings, 0 replies; 23+ messages in thread
From: Ludovic Desroches @ 2018-01-15 16:22 UTC (permalink / raw)
  To: linux-gpio, linux-arm-kernel
  Cc: linus.walleij, linux-kernel, nicolas.free, Ludovic Desroches

Add a consumer variant to GPIO request relative functions. The goal
is to fix the bad ownership, which is arbitrary set to
"range->name:gpio", of a GPIO.

There is a lack of configuration features for GPIO. For instance,
we can't set the bias. Some pin controllers manage both device's
pins and GPIOs. GPIOs can benefit from pin configuration. Usually,
a pinctrl node is used to mux the pin as a GPIO and to set up its
configuration.

The pinmuxing strict mode involves that a pin which is muxed can't
be requested as a GPIO if the owner is not the same. Unfortunately,
the ownership of a GPIO is set arbitrarily to "range->name:gpio".
So there is a mismatch about the ownership which prevents a device
from being the owner of the pinmuxing and requesting the same pin as
a GPIO.

Adding some consumer variants for GPIO request stuff will allow to
pass the name of the device which requests the GPIO to not return an
error if it's also the owner of the pinmuxing.

Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com>
---
 drivers/pinctrl/core.c           | 13 ++++++++++---
 drivers/pinctrl/pinmux.c         | 16 ++++++++++++++--
 drivers/pinctrl/pinmux.h         | 10 ++++++++++
 include/linux/pinctrl/consumer.h |  6 ++++++
 4 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 2c0dbfcff3e6..51c75a6057b2 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -733,14 +733,15 @@ int pinctrl_get_group_selector(struct pinctrl_dev *pctldev,
 }
 
 /**
- * pinctrl_gpio_request() - request a single pin to be used as GPIO
+ * pinctrl_gpio_request_consumer() - request a single pin to be used as GPIO
  * @gpio: the GPIO pin number from the GPIO subsystem number space
+ * @consumer: the name of the device requesting the GPIO
  *
  * This function should *ONLY* be used from gpiolib-based GPIO drivers,
  * as part of their gpio_request() semantics, platforms and individual drivers
  * shall *NOT* request GPIO pins to be muxed in.
  */
-int pinctrl_gpio_request(unsigned gpio)
+int pinctrl_gpio_request_consumer(unsigned gpio, const char *consumer)
 {
 	struct pinctrl_dev *pctldev;
 	struct pinctrl_gpio_range *range;
@@ -759,12 +760,18 @@ int pinctrl_gpio_request(unsigned gpio)
 	/* Convert to the pin controllers number space */
 	pin = gpio_to_pin(range, gpio);
 
-	ret = pinmux_request_gpio(pctldev, range, pin, gpio);
+	ret = pinmux_request_gpio_consumer(pctldev, range, pin, gpio, consumer);
 
 	mutex_unlock(&pctldev->mutex);
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(pinctrl_gpio_request_consumer);
+
+int pinctrl_gpio_request(unsigned gpio)
+{
+	return pinctrl_gpio_request_consumer(gpio, NULL);
+}
 EXPORT_SYMBOL_GPL(pinctrl_gpio_request);
 
 /**
diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index 55502fc4479c..8d422eb0ed38 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -232,14 +232,19 @@ static const char *pin_free(struct pinctrl_dev *pctldev, int pin,
  * @pctldev: pin controller device affected
  * @pin: the pin to mux in for GPIO
  * @range: the applicable GPIO range
+ * @consumer: the name of the device requesting the GPIO
  */
-int pinmux_request_gpio(struct pinctrl_dev *pctldev,
+int pinmux_request_gpio_consumer(struct pinctrl_dev *pctldev,
 			struct pinctrl_gpio_range *range,
-			unsigned pin, unsigned gpio)
+			unsigned pin, unsigned gpio,
+			const char *consumer)
 {
 	const char *owner;
 	int ret;
 
+	if (consumer)
+		return pin_request(pctldev, pin, consumer, range);
+
 	/* Conjure some name stating what chip and pin this is taken by */
 	owner = kasprintf(GFP_KERNEL, "%s:%d", range->name, gpio);
 	if (!owner)
@@ -252,6 +257,13 @@ int pinmux_request_gpio(struct pinctrl_dev *pctldev,
 	return ret;
 }
 
+int pinmux_request_gpio(struct pinctrl_dev *pctldev,
+			struct pinctrl_gpio_range *range,
+			unsigned pin, unsigned gpio)
+{
+	return pinmux_request_gpio_consumer(pctldev, range, pin, gpio, NULL);
+}
+
 /**
  * pinmux_free_gpio() - release a pin from GPIO muxing
  * @pctldev: the pin controller device for the pin
diff --git a/drivers/pinctrl/pinmux.h b/drivers/pinctrl/pinmux.h
index a331fcdbedd9..837599922a42 100644
--- a/drivers/pinctrl/pinmux.h
+++ b/drivers/pinctrl/pinmux.h
@@ -19,6 +19,9 @@ int pinmux_validate_map(const struct pinctrl_map *map, int i);
 int pinmux_request_gpio(struct pinctrl_dev *pctldev,
 			struct pinctrl_gpio_range *range,
 			unsigned pin, unsigned gpio);
+int pinmux_request_gpio_consumer(struct pinctrl_dev *pctldev,
+			struct pinctrl_gpio_range *range,
+			unsigned pin, unsigned gpio, const char *consumer);
 void pinmux_free_gpio(struct pinctrl_dev *pctldev, unsigned pin,
 		      struct pinctrl_gpio_range *range);
 int pinmux_gpio_direction(struct pinctrl_dev *pctldev,
@@ -50,6 +53,13 @@ static inline int pinmux_request_gpio(struct pinctrl_dev *pctldev,
 	return 0;
 }
 
+static inline int pinmux_request_gpio_consumer(struct pinctrl_dev *pctldev,
+			struct pinctrl_gpio_range *range,
+			unsigned pin, unsigned gpio, const char *consumer)
+{
+	return 0;
+}
+
 static inline void pinmux_free_gpio(struct pinctrl_dev *pctldev,
 				    unsigned pin,
 				    struct pinctrl_gpio_range *range)
diff --git a/include/linux/pinctrl/consumer.h b/include/linux/pinctrl/consumer.h
index 0412cc9833e9..8c521a14db43 100644
--- a/include/linux/pinctrl/consumer.h
+++ b/include/linux/pinctrl/consumer.h
@@ -26,6 +26,7 @@ struct device;
 
 /* External interface to pin control */
 extern int pinctrl_gpio_request(unsigned gpio);
+extern int pinctrl_gpio_request_consumer(unsigned gpio, const char *consumer);
 extern void pinctrl_gpio_free(unsigned gpio);
 extern int pinctrl_gpio_direction_input(unsigned gpio);
 extern int pinctrl_gpio_direction_output(unsigned gpio);
@@ -67,6 +68,11 @@ static inline int pinctrl_gpio_request(unsigned gpio)
 	return 0;
 }
 
+static inline int pinctrl_gpio_request_consumer(unsigned gpio, const char *consumer);
+{
+	return 0;
+}
+
 static inline void pinctrl_gpio_free(unsigned gpio)
 {
 }
-- 
2.12.2

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [RFC PATCH 1/2] pinctrl: add consumer variant for gpio request
@ 2018-01-15 16:22   ` Ludovic Desroches
  0 siblings, 0 replies; 23+ messages in thread
From: Ludovic Desroches @ 2018-01-15 16:22 UTC (permalink / raw)
  To: linux-arm-kernel

Add a consumer variant to GPIO request relative functions. The goal
is to fix the bad ownership, which is arbitrary set to
"range->name:gpio", of a GPIO.

There is a lack of configuration features for GPIO. For instance,
we can't set the bias. Some pin controllers manage both device's
pins and GPIOs. GPIOs can benefit from pin configuration. Usually,
a pinctrl node is used to mux the pin as a GPIO and to set up its
configuration.

The pinmuxing strict mode involves that a pin which is muxed can't
be requested as a GPIO if the owner is not the same. Unfortunately,
the ownership of a GPIO is set arbitrarily to "range->name:gpio".
So there is a mismatch about the ownership which prevents a device
from being the owner of the pinmuxing and requesting the same pin as
a GPIO.

Adding some consumer variants for GPIO request stuff will allow to
pass the name of the device which requests the GPIO to not return an
error if it's also the owner of the pinmuxing.

Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com>
---
 drivers/pinctrl/core.c           | 13 ++++++++++---
 drivers/pinctrl/pinmux.c         | 16 ++++++++++++++--
 drivers/pinctrl/pinmux.h         | 10 ++++++++++
 include/linux/pinctrl/consumer.h |  6 ++++++
 4 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 2c0dbfcff3e6..51c75a6057b2 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -733,14 +733,15 @@ int pinctrl_get_group_selector(struct pinctrl_dev *pctldev,
 }
 
 /**
- * pinctrl_gpio_request() - request a single pin to be used as GPIO
+ * pinctrl_gpio_request_consumer() - request a single pin to be used as GPIO
  * @gpio: the GPIO pin number from the GPIO subsystem number space
+ * @consumer: the name of the device requesting the GPIO
  *
  * This function should *ONLY* be used from gpiolib-based GPIO drivers,
  * as part of their gpio_request() semantics, platforms and individual drivers
  * shall *NOT* request GPIO pins to be muxed in.
  */
-int pinctrl_gpio_request(unsigned gpio)
+int pinctrl_gpio_request_consumer(unsigned gpio, const char *consumer)
 {
 	struct pinctrl_dev *pctldev;
 	struct pinctrl_gpio_range *range;
@@ -759,12 +760,18 @@ int pinctrl_gpio_request(unsigned gpio)
 	/* Convert to the pin controllers number space */
 	pin = gpio_to_pin(range, gpio);
 
-	ret = pinmux_request_gpio(pctldev, range, pin, gpio);
+	ret = pinmux_request_gpio_consumer(pctldev, range, pin, gpio, consumer);
 
 	mutex_unlock(&pctldev->mutex);
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(pinctrl_gpio_request_consumer);
+
+int pinctrl_gpio_request(unsigned gpio)
+{
+	return pinctrl_gpio_request_consumer(gpio, NULL);
+}
 EXPORT_SYMBOL_GPL(pinctrl_gpio_request);
 
 /**
diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index 55502fc4479c..8d422eb0ed38 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -232,14 +232,19 @@ static const char *pin_free(struct pinctrl_dev *pctldev, int pin,
  * @pctldev: pin controller device affected
  * @pin: the pin to mux in for GPIO
  * @range: the applicable GPIO range
+ * @consumer: the name of the device requesting the GPIO
  */
-int pinmux_request_gpio(struct pinctrl_dev *pctldev,
+int pinmux_request_gpio_consumer(struct pinctrl_dev *pctldev,
 			struct pinctrl_gpio_range *range,
-			unsigned pin, unsigned gpio)
+			unsigned pin, unsigned gpio,
+			const char *consumer)
 {
 	const char *owner;
 	int ret;
 
+	if (consumer)
+		return pin_request(pctldev, pin, consumer, range);
+
 	/* Conjure some name stating what chip and pin this is taken by */
 	owner = kasprintf(GFP_KERNEL, "%s:%d", range->name, gpio);
 	if (!owner)
@@ -252,6 +257,13 @@ int pinmux_request_gpio(struct pinctrl_dev *pctldev,
 	return ret;
 }
 
+int pinmux_request_gpio(struct pinctrl_dev *pctldev,
+			struct pinctrl_gpio_range *range,
+			unsigned pin, unsigned gpio)
+{
+	return pinmux_request_gpio_consumer(pctldev, range, pin, gpio, NULL);
+}
+
 /**
  * pinmux_free_gpio() - release a pin from GPIO muxing
  * @pctldev: the pin controller device for the pin
diff --git a/drivers/pinctrl/pinmux.h b/drivers/pinctrl/pinmux.h
index a331fcdbedd9..837599922a42 100644
--- a/drivers/pinctrl/pinmux.h
+++ b/drivers/pinctrl/pinmux.h
@@ -19,6 +19,9 @@ int pinmux_validate_map(const struct pinctrl_map *map, int i);
 int pinmux_request_gpio(struct pinctrl_dev *pctldev,
 			struct pinctrl_gpio_range *range,
 			unsigned pin, unsigned gpio);
+int pinmux_request_gpio_consumer(struct pinctrl_dev *pctldev,
+			struct pinctrl_gpio_range *range,
+			unsigned pin, unsigned gpio, const char *consumer);
 void pinmux_free_gpio(struct pinctrl_dev *pctldev, unsigned pin,
 		      struct pinctrl_gpio_range *range);
 int pinmux_gpio_direction(struct pinctrl_dev *pctldev,
@@ -50,6 +53,13 @@ static inline int pinmux_request_gpio(struct pinctrl_dev *pctldev,
 	return 0;
 }
 
+static inline int pinmux_request_gpio_consumer(struct pinctrl_dev *pctldev,
+			struct pinctrl_gpio_range *range,
+			unsigned pin, unsigned gpio, const char *consumer)
+{
+	return 0;
+}
+
 static inline void pinmux_free_gpio(struct pinctrl_dev *pctldev,
 				    unsigned pin,
 				    struct pinctrl_gpio_range *range)
diff --git a/include/linux/pinctrl/consumer.h b/include/linux/pinctrl/consumer.h
index 0412cc9833e9..8c521a14db43 100644
--- a/include/linux/pinctrl/consumer.h
+++ b/include/linux/pinctrl/consumer.h
@@ -26,6 +26,7 @@ struct device;
 
 /* External interface to pin control */
 extern int pinctrl_gpio_request(unsigned gpio);
+extern int pinctrl_gpio_request_consumer(unsigned gpio, const char *consumer);
 extern void pinctrl_gpio_free(unsigned gpio);
 extern int pinctrl_gpio_direction_input(unsigned gpio);
 extern int pinctrl_gpio_direction_output(unsigned gpio);
@@ -67,6 +68,11 @@ static inline int pinctrl_gpio_request(unsigned gpio)
 	return 0;
 }
 
+static inline int pinctrl_gpio_request_consumer(unsigned gpio, const char *consumer);
+{
+	return 0;
+}
+
 static inline void pinctrl_gpio_free(unsigned gpio)
 {
 }
-- 
2.12.2

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [RFC PATCH 2/2] gpio: provide a consumer when requesting a gpio
  2018-01-15 16:22 ` Ludovic Desroches
  (?)
@ 2018-01-15 16:22   ` Ludovic Desroches
  -1 siblings, 0 replies; 23+ messages in thread
From: Ludovic Desroches @ 2018-01-15 16:22 UTC (permalink / raw)
  To: linux-gpio, linux-arm-kernel
  Cc: linus.walleij, linux-kernel, nicolas.free, Ludovic Desroches

It can be useful for the pinmuxing layer to know which device is
requesting a GPIO. Add a consumer variant for gpiod_request to
reach this goal.

GPIO chips managed by pin controllers should provide the new
request_consumer operation. They can rely on
gpiochip_generic_request_consumer instead of
gpiochip_generic_request.

Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com>
---
 drivers/gpio/gpiolib.c      | 40 +++++++++++++++++++++++++++++++++-------
 include/linux/gpio/driver.h |  5 +++++
 2 files changed, 38 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 39a0144d5be5..e6645101ec74 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1970,6 +1970,20 @@ int gpiochip_generic_request(struct gpio_chip *chip, unsigned offset)
 EXPORT_SYMBOL_GPL(gpiochip_generic_request);
 
 /**
+ * gpiochip_generic_request_consumer() - request the gpio function for a pin
+ * @chip: the gpiochip owning the GPIO
+ * @offset: the offset of the GPIO to request for GPIO function
+ * @consumer: name of the device requesting the GPIO
+ */
+int gpiochip_generic_request_consumer(struct gpio_chip *chip, unsigned offset,
+				      const char *consumer)
+{
+	return pinctrl_gpio_request_consumer(chip->gpiodev->base + offset,
+					     consumer);
+}
+EXPORT_SYMBOL_GPL(gpiochip_generic_request_consumer);
+
+/**
  * gpiochip_generic_free() - free the gpio function from a pin
  * @chip: the gpiochip to request the gpio function for
  * @offset: the offset of the GPIO to free from GPIO function
@@ -2119,7 +2133,8 @@ EXPORT_SYMBOL_GPL(gpiochip_remove_pin_ranges);
  * on each other, and help provide better diagnostics in debugfs.
  * They're called even less than the "set direction" calls.
  */
-static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
+static int gpiod_request_commit(struct gpio_desc *desc, const char *label,
+				const char *consumer)
 {
 	struct gpio_chip	*chip = desc->gdev->chip;
 	int			status;
@@ -2139,10 +2154,14 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
 		goto done;
 	}
 
-	if (chip->request) {
+	if (chip->request || chip->request_consumer) {
 		/* chip->request may sleep */
 		spin_unlock_irqrestore(&gpio_lock, flags);
-		status = chip->request(chip, gpio_chip_hwgpio(desc));
+		if (chip->request_consumer)
+			status = chip->request_consumer(chip,
+					gpio_chip_hwgpio(desc), consumer);
+		else
+			status = chip->request(chip, gpio_chip_hwgpio(desc));
 		spin_lock_irqsave(&gpio_lock, flags);
 
 		if (status < 0) {
@@ -2200,7 +2219,8 @@ static int validate_desc(const struct gpio_desc *desc, const char *func)
 		return; \
 	} while (0)
 
-int gpiod_request(struct gpio_desc *desc, const char *label)
+int gpiod_request_consumer(struct gpio_desc *desc, const char *label,
+			   const char *consumer)
 {
 	int status = -EPROBE_DEFER;
 	struct gpio_device *gdev;
@@ -2209,7 +2229,7 @@ int gpiod_request(struct gpio_desc *desc, const char *label)
 	gdev = desc->gdev;
 
 	if (try_module_get(gdev->owner)) {
-		status = gpiod_request_commit(desc, label);
+		status = gpiod_request_commit(desc, label, consumer);
 		if (status < 0)
 			module_put(gdev->owner);
 		else
@@ -2222,6 +2242,11 @@ int gpiod_request(struct gpio_desc *desc, const char *label)
 	return status;
 }
 
+int gpiod_request(struct gpio_desc *desc, const char *label)
+{
+	return gpiod_request_consumer(desc, label, NULL);
+}
+
 static bool gpiod_free_commit(struct gpio_desc *desc)
 {
 	bool			ret = false;
@@ -2320,7 +2345,7 @@ struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *chip, u16 hwnum,
 		return desc;
 	}
 
-	err = gpiod_request_commit(desc, label);
+	err = gpiod_request_commit(desc, label, NULL);
 	if (err < 0)
 		return ERR_PTR(err);
 
@@ -3672,7 +3697,8 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
 	}
 
 	/* If a connection label was passed use that, else use the device name as label */
-	status = gpiod_request(desc, con_id ? con_id : dev_name(dev));
+	status = gpiod_request_consumer(desc, con_id ? con_id : dev_name(dev),
+					dev_name(dev));
 	if (status < 0)
 		return ERR_PTR(status);
 
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 1ba9a331ec51..6bc5c57f0cbd 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -228,6 +228,9 @@ struct gpio_chip {
 
 	int			(*request)(struct gpio_chip *chip,
 						unsigned offset);
+	int			(*request_consumer)(struct gpio_chip *chip,
+						unsigned offset,
+						const char *consumer);
 	void			(*free)(struct gpio_chip *chip,
 						unsigned offset);
 	int			(*get_direction)(struct gpio_chip *chip,
@@ -500,6 +503,8 @@ static inline int gpiochip_irqchip_add_nested(struct gpio_chip *gpiochip,
 #endif /* CONFIG_GPIOLIB_IRQCHIP */
 
 int gpiochip_generic_request(struct gpio_chip *chip, unsigned offset);
+int gpiochip_generic_request_consumer(struct gpio_chip *chip, unsigned offset,
+				      const char *consumer);
 void gpiochip_generic_free(struct gpio_chip *chip, unsigned offset);
 int gpiochip_generic_config(struct gpio_chip *chip, unsigned offset,
 			    unsigned long config);
-- 
2.12.2

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [RFC PATCH 2/2] gpio: provide a consumer when requesting a gpio
@ 2018-01-15 16:22   ` Ludovic Desroches
  0 siblings, 0 replies; 23+ messages in thread
From: Ludovic Desroches @ 2018-01-15 16:22 UTC (permalink / raw)
  To: linux-gpio, linux-arm-kernel
  Cc: linus.walleij, linux-kernel, nicolas.free, Ludovic Desroches

It can be useful for the pinmuxing layer to know which device is
requesting a GPIO. Add a consumer variant for gpiod_request to
reach this goal.

GPIO chips managed by pin controllers should provide the new
request_consumer operation. They can rely on
gpiochip_generic_request_consumer instead of
gpiochip_generic_request.

Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com>
---
 drivers/gpio/gpiolib.c      | 40 +++++++++++++++++++++++++++++++++-------
 include/linux/gpio/driver.h |  5 +++++
 2 files changed, 38 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 39a0144d5be5..e6645101ec74 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1970,6 +1970,20 @@ int gpiochip_generic_request(struct gpio_chip *chip, unsigned offset)
 EXPORT_SYMBOL_GPL(gpiochip_generic_request);
 
 /**
+ * gpiochip_generic_request_consumer() - request the gpio function for a pin
+ * @chip: the gpiochip owning the GPIO
+ * @offset: the offset of the GPIO to request for GPIO function
+ * @consumer: name of the device requesting the GPIO
+ */
+int gpiochip_generic_request_consumer(struct gpio_chip *chip, unsigned offset,
+				      const char *consumer)
+{
+	return pinctrl_gpio_request_consumer(chip->gpiodev->base + offset,
+					     consumer);
+}
+EXPORT_SYMBOL_GPL(gpiochip_generic_request_consumer);
+
+/**
  * gpiochip_generic_free() - free the gpio function from a pin
  * @chip: the gpiochip to request the gpio function for
  * @offset: the offset of the GPIO to free from GPIO function
@@ -2119,7 +2133,8 @@ EXPORT_SYMBOL_GPL(gpiochip_remove_pin_ranges);
  * on each other, and help provide better diagnostics in debugfs.
  * They're called even less than the "set direction" calls.
  */
-static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
+static int gpiod_request_commit(struct gpio_desc *desc, const char *label,
+				const char *consumer)
 {
 	struct gpio_chip	*chip = desc->gdev->chip;
 	int			status;
@@ -2139,10 +2154,14 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
 		goto done;
 	}
 
-	if (chip->request) {
+	if (chip->request || chip->request_consumer) {
 		/* chip->request may sleep */
 		spin_unlock_irqrestore(&gpio_lock, flags);
-		status = chip->request(chip, gpio_chip_hwgpio(desc));
+		if (chip->request_consumer)
+			status = chip->request_consumer(chip,
+					gpio_chip_hwgpio(desc), consumer);
+		else
+			status = chip->request(chip, gpio_chip_hwgpio(desc));
 		spin_lock_irqsave(&gpio_lock, flags);
 
 		if (status < 0) {
@@ -2200,7 +2219,8 @@ static int validate_desc(const struct gpio_desc *desc, const char *func)
 		return; \
 	} while (0)
 
-int gpiod_request(struct gpio_desc *desc, const char *label)
+int gpiod_request_consumer(struct gpio_desc *desc, const char *label,
+			   const char *consumer)
 {
 	int status = -EPROBE_DEFER;
 	struct gpio_device *gdev;
@@ -2209,7 +2229,7 @@ int gpiod_request(struct gpio_desc *desc, const char *label)
 	gdev = desc->gdev;
 
 	if (try_module_get(gdev->owner)) {
-		status = gpiod_request_commit(desc, label);
+		status = gpiod_request_commit(desc, label, consumer);
 		if (status < 0)
 			module_put(gdev->owner);
 		else
@@ -2222,6 +2242,11 @@ int gpiod_request(struct gpio_desc *desc, const char *label)
 	return status;
 }
 
+int gpiod_request(struct gpio_desc *desc, const char *label)
+{
+	return gpiod_request_consumer(desc, label, NULL);
+}
+
 static bool gpiod_free_commit(struct gpio_desc *desc)
 {
 	bool			ret = false;
@@ -2320,7 +2345,7 @@ struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *chip, u16 hwnum,
 		return desc;
 	}
 
-	err = gpiod_request_commit(desc, label);
+	err = gpiod_request_commit(desc, label, NULL);
 	if (err < 0)
 		return ERR_PTR(err);
 
@@ -3672,7 +3697,8 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
 	}
 
 	/* If a connection label was passed use that, else use the device name as label */
-	status = gpiod_request(desc, con_id ? con_id : dev_name(dev));
+	status = gpiod_request_consumer(desc, con_id ? con_id : dev_name(dev),
+					dev_name(dev));
 	if (status < 0)
 		return ERR_PTR(status);
 
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 1ba9a331ec51..6bc5c57f0cbd 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -228,6 +228,9 @@ struct gpio_chip {
 
 	int			(*request)(struct gpio_chip *chip,
 						unsigned offset);
+	int			(*request_consumer)(struct gpio_chip *chip,
+						unsigned offset,
+						const char *consumer);
 	void			(*free)(struct gpio_chip *chip,
 						unsigned offset);
 	int			(*get_direction)(struct gpio_chip *chip,
@@ -500,6 +503,8 @@ static inline int gpiochip_irqchip_add_nested(struct gpio_chip *gpiochip,
 #endif /* CONFIG_GPIOLIB_IRQCHIP */
 
 int gpiochip_generic_request(struct gpio_chip *chip, unsigned offset);
+int gpiochip_generic_request_consumer(struct gpio_chip *chip, unsigned offset,
+				      const char *consumer);
 void gpiochip_generic_free(struct gpio_chip *chip, unsigned offset);
 int gpiochip_generic_config(struct gpio_chip *chip, unsigned offset,
 			    unsigned long config);
-- 
2.12.2

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [RFC PATCH 2/2] gpio: provide a consumer when requesting a gpio
@ 2018-01-15 16:22   ` Ludovic Desroches
  0 siblings, 0 replies; 23+ messages in thread
From: Ludovic Desroches @ 2018-01-15 16:22 UTC (permalink / raw)
  To: linux-arm-kernel

It can be useful for the pinmuxing layer to know which device is
requesting a GPIO. Add a consumer variant for gpiod_request to
reach this goal.

GPIO chips managed by pin controllers should provide the new
request_consumer operation. They can rely on
gpiochip_generic_request_consumer instead of
gpiochip_generic_request.

Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com>
---
 drivers/gpio/gpiolib.c      | 40 +++++++++++++++++++++++++++++++++-------
 include/linux/gpio/driver.h |  5 +++++
 2 files changed, 38 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 39a0144d5be5..e6645101ec74 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1970,6 +1970,20 @@ int gpiochip_generic_request(struct gpio_chip *chip, unsigned offset)
 EXPORT_SYMBOL_GPL(gpiochip_generic_request);
 
 /**
+ * gpiochip_generic_request_consumer() - request the gpio function for a pin
+ * @chip: the gpiochip owning the GPIO
+ * @offset: the offset of the GPIO to request for GPIO function
+ * @consumer: name of the device requesting the GPIO
+ */
+int gpiochip_generic_request_consumer(struct gpio_chip *chip, unsigned offset,
+				      const char *consumer)
+{
+	return pinctrl_gpio_request_consumer(chip->gpiodev->base + offset,
+					     consumer);
+}
+EXPORT_SYMBOL_GPL(gpiochip_generic_request_consumer);
+
+/**
  * gpiochip_generic_free() - free the gpio function from a pin
  * @chip: the gpiochip to request the gpio function for
  * @offset: the offset of the GPIO to free from GPIO function
@@ -2119,7 +2133,8 @@ EXPORT_SYMBOL_GPL(gpiochip_remove_pin_ranges);
  * on each other, and help provide better diagnostics in debugfs.
  * They're called even less than the "set direction" calls.
  */
-static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
+static int gpiod_request_commit(struct gpio_desc *desc, const char *label,
+				const char *consumer)
 {
 	struct gpio_chip	*chip = desc->gdev->chip;
 	int			status;
@@ -2139,10 +2154,14 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
 		goto done;
 	}
 
-	if (chip->request) {
+	if (chip->request || chip->request_consumer) {
 		/* chip->request may sleep */
 		spin_unlock_irqrestore(&gpio_lock, flags);
-		status = chip->request(chip, gpio_chip_hwgpio(desc));
+		if (chip->request_consumer)
+			status = chip->request_consumer(chip,
+					gpio_chip_hwgpio(desc), consumer);
+		else
+			status = chip->request(chip, gpio_chip_hwgpio(desc));
 		spin_lock_irqsave(&gpio_lock, flags);
 
 		if (status < 0) {
@@ -2200,7 +2219,8 @@ static int validate_desc(const struct gpio_desc *desc, const char *func)
 		return; \
 	} while (0)
 
-int gpiod_request(struct gpio_desc *desc, const char *label)
+int gpiod_request_consumer(struct gpio_desc *desc, const char *label,
+			   const char *consumer)
 {
 	int status = -EPROBE_DEFER;
 	struct gpio_device *gdev;
@@ -2209,7 +2229,7 @@ int gpiod_request(struct gpio_desc *desc, const char *label)
 	gdev = desc->gdev;
 
 	if (try_module_get(gdev->owner)) {
-		status = gpiod_request_commit(desc, label);
+		status = gpiod_request_commit(desc, label, consumer);
 		if (status < 0)
 			module_put(gdev->owner);
 		else
@@ -2222,6 +2242,11 @@ int gpiod_request(struct gpio_desc *desc, const char *label)
 	return status;
 }
 
+int gpiod_request(struct gpio_desc *desc, const char *label)
+{
+	return gpiod_request_consumer(desc, label, NULL);
+}
+
 static bool gpiod_free_commit(struct gpio_desc *desc)
 {
 	bool			ret = false;
@@ -2320,7 +2345,7 @@ struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *chip, u16 hwnum,
 		return desc;
 	}
 
-	err = gpiod_request_commit(desc, label);
+	err = gpiod_request_commit(desc, label, NULL);
 	if (err < 0)
 		return ERR_PTR(err);
 
@@ -3672,7 +3697,8 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
 	}
 
 	/* If a connection label was passed use that, else use the device name as label */
-	status = gpiod_request(desc, con_id ? con_id : dev_name(dev));
+	status = gpiod_request_consumer(desc, con_id ? con_id : dev_name(dev),
+					dev_name(dev));
 	if (status < 0)
 		return ERR_PTR(status);
 
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 1ba9a331ec51..6bc5c57f0cbd 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -228,6 +228,9 @@ struct gpio_chip {
 
 	int			(*request)(struct gpio_chip *chip,
 						unsigned offset);
+	int			(*request_consumer)(struct gpio_chip *chip,
+						unsigned offset,
+						const char *consumer);
 	void			(*free)(struct gpio_chip *chip,
 						unsigned offset);
 	int			(*get_direction)(struct gpio_chip *chip,
@@ -500,6 +503,8 @@ static inline int gpiochip_irqchip_add_nested(struct gpio_chip *gpiochip,
 #endif /* CONFIG_GPIOLIB_IRQCHIP */
 
 int gpiochip_generic_request(struct gpio_chip *chip, unsigned offset);
+int gpiochip_generic_request_consumer(struct gpio_chip *chip, unsigned offset,
+				      const char *consumer);
 void gpiochip_generic_free(struct gpio_chip *chip, unsigned offset);
 int gpiochip_generic_config(struct gpio_chip *chip, unsigned offset,
 			    unsigned long config);
-- 
2.12.2

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH 1/2] pinctrl: add consumer variant for gpio request
  2018-01-15 16:22   ` Ludovic Desroches
@ 2018-01-15 20:19     ` Andy Shevchenko
  -1 siblings, 0 replies; 23+ messages in thread
From: Andy Shevchenko @ 2018-01-15 20:19 UTC (permalink / raw)
  To: Ludovic Desroches
  Cc: open list:GPIO SUBSYSTEM, linux-arm Mailing List, Linus Walleij,
	Linux Kernel Mailing List, nicolas.free

On Mon, Jan 15, 2018 at 6:22 PM, Ludovic Desroches
<ludovic.desroches@microchip.com> wrote:

Did I miss cover letter for this?

> Add a consumer variant to GPIO request relative functions. The goal
> is to fix the bad ownership, which is arbitrary set to
> "range->name:gpio", of a GPIO.

Hmm... It's supposed to be name of the owner of the pin range (pin
control device name IIUC).

> There is a lack of configuration features for GPIO. For instance,
> we can't set the bias. Some pin controllers manage both device's
> pins and GPIOs. GPIOs can benefit from pin configuration. Usually,
> a pinctrl node is used to mux the pin as a GPIO and to set up its
> configuration.

Don't we have means to do that?

At least that what I see in aspeed_gpio_set_config().

Or I missed a point here?

> The pinmuxing strict mode involves that a pin which is muxed can't
> be requested as a GPIO if the owner is not the same.

Any elaborated example?

> Unfortunately,
> the ownership of a GPIO is set arbitrarily to "range->name:gpio".
> So there is a mismatch about the ownership which prevents a device
> from being the owner of the pinmuxing and requesting the same pin as
> a GPIO.

> Adding some consumer variants for GPIO request stuff will allow to
> pass the name of the device which requests the GPIO to not return an
> error if it's also the owner of the pinmuxing.

I think we need something more generic in core than producing more API
functions.

But I would like to get problem first.

> +       if (consumer)
> +               return pin_request(pctldev, pin, consumer, range);
> +

Hmm... My understanding that GPIO is just a (special) function out of
pin muxing. So, doing musing is essential to get proper function out
of it.

Does your hardware considers this differently? If so, I would really
want to see some datasheets.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [RFC PATCH 1/2] pinctrl: add consumer variant for gpio request
@ 2018-01-15 20:19     ` Andy Shevchenko
  0 siblings, 0 replies; 23+ messages in thread
From: Andy Shevchenko @ 2018-01-15 20:19 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jan 15, 2018 at 6:22 PM, Ludovic Desroches
<ludovic.desroches@microchip.com> wrote:

Did I miss cover letter for this?

> Add a consumer variant to GPIO request relative functions. The goal
> is to fix the bad ownership, which is arbitrary set to
> "range->name:gpio", of a GPIO.

Hmm... It's supposed to be name of the owner of the pin range (pin
control device name IIUC).

> There is a lack of configuration features for GPIO. For instance,
> we can't set the bias. Some pin controllers manage both device's
> pins and GPIOs. GPIOs can benefit from pin configuration. Usually,
> a pinctrl node is used to mux the pin as a GPIO and to set up its
> configuration.

Don't we have means to do that?

At least that what I see in aspeed_gpio_set_config().

Or I missed a point here?

> The pinmuxing strict mode involves that a pin which is muxed can't
> be requested as a GPIO if the owner is not the same.

Any elaborated example?

> Unfortunately,
> the ownership of a GPIO is set arbitrarily to "range->name:gpio".
> So there is a mismatch about the ownership which prevents a device
> from being the owner of the pinmuxing and requesting the same pin as
> a GPIO.

> Adding some consumer variants for GPIO request stuff will allow to
> pass the name of the device which requests the GPIO to not return an
> error if it's also the owner of the pinmuxing.

I think we need something more generic in core than producing more API
functions.

But I would like to get problem first.

> +       if (consumer)
> +               return pin_request(pctldev, pin, consumer, range);
> +

Hmm... My understanding that GPIO is just a (special) function out of
pin muxing. So, doing musing is essential to get proper function out
of it.

Does your hardware considers this differently? If so, I would really
want to see some datasheets.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH 1/2] pinctrl: add consumer variant for gpio request
  2018-01-15 20:19     ` Andy Shevchenko
@ 2018-01-16  9:01       ` Ludovic Desroches
  -1 siblings, 0 replies; 23+ messages in thread
From: Ludovic Desroches @ 2018-01-16  9:01 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Ludovic Desroches, open list:GPIO SUBSYSTEM,
	linux-arm Mailing List, Linus Walleij, Linux Kernel Mailing List,
	nicolas.free

Hi,

On Mon, Jan 15, 2018 at 10:19:39PM +0200, Andy Shevchenko wrote:
> On Mon, Jan 15, 2018 at 6:22 PM, Ludovic Desroches
> <ludovic.desroches@microchip.com> wrote:
> 
> Did I miss cover letter for this?
> 

It seems: https://lkml.org/lkml/2018/1/15/841

> > Add a consumer variant to GPIO request relative functions. The goal
> > is to fix the bad ownership, which is arbitrary set to
> > "range->name:gpio", of a GPIO.
> 
> Hmm... It's supposed to be name of the owner of the pin range (pin
> control device name IIUC).
>

Yes, the owner is the pinctrl device.

> > There is a lack of configuration features for GPIO. For instance,
> > we can't set the bias. Some pin controllers manage both device's
> > pins and GPIOs. GPIOs can benefit from pin configuration. Usually,
> > a pinctrl node is used to mux the pin as a GPIO and to set up its
> > configuration.
> 
> Don't we have means to do that?
> 

I don't think so. I am not the only one to have this issue for a long
time.

There is an interesting thread here:
https://www.spinics.net/lists/linux-gpio/msg16769.html

If things have changed and I missed it, please tell me. I have seen some
improvements but it still don't fit my needs.

> At least that what I see in aspeed_gpio_set_config().
> 

Yes this is part of the improvements I have seen. The set_config
operation is used at several places in the gpiolib:
- gpio_set_drive_single_ended
- gpiod_set_debounce
- gpiod_set_transitory

It doesn't cover all my needs. In the cover letter, I mentionned that I
based my job on this adding parameters I need. Someone told me it may be
difficult to pul all the parameters in one cell. For instance, the drive
strength is a numeric value using several bits.

I am not sure duplicating the parameters we have for pinconf is the
appropriate solution. Then I prepare a second set of patches to add a
cell with a phandle on the pin configuration. I was going to send it
when I realize that fixing the ownership issue is probably a better
solution. It may involve more code change but less duplication.

> Or I missed a point here?
> 
> > The pinmuxing strict mode involves that a pin which is muxed can't
> > be requested as a GPIO if the owner is not the same.
> 
> Any elaborated example?
> 

More details in the thread I mentionned earlier. I want to enable the
strict mode to prevent weird behavior using the sysfs (or the char
device). Moreover, the strict mode fits the behavior of my pin
controller.

In my DTS, at the moment, if a device needs GPIOs, then I add a pinctrl
node where I put the pinmuxing of the pins as GPIOs and I set the
configuration (for instance, bias pull-up, debounce). If the strict mode
is enabled, when the driver will request the GPIOs, it will fail even if
it's the owner of the pinmuxing.

> > Unfortunately,
> > the ownership of a GPIO is set arbitrarily to "range->name:gpio".
> > So there is a mismatch about the ownership which prevents a device
> > from being the owner of the pinmuxing and requesting the same pin as
> > a GPIO.
> 
> > Adding some consumer variants for GPIO request stuff will allow to
> > pass the name of the device which requests the GPIO to not return an
> > error if it's also the owner of the pinmuxing.
> 
> I think we need something more generic in core than producing more API
> functions.
> 

I didn't want to introduce too many changes to keep it safe and I didn't
want to break current API by adding a parameter.

> But I would like to get problem first.
> 
> > +       if (consumer)
> > +               return pin_request(pctldev, pin, consumer, range);
> > +
> 
> Hmm... My understanding that GPIO is just a (special) function out of
> pin muxing. So, doing musing is essential to get proper function out
> of it.
> 
> Does your hardware considers this differently? If so, I would really
> want to see some datasheets.

No you're right about the behavior of my hardware.

Regards

Ludovic

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH 1/2] pinctrl: add consumer variant for gpio request
@ 2018-01-16  9:01       ` Ludovic Desroches
  0 siblings, 0 replies; 23+ messages in thread
From: Ludovic Desroches @ 2018-01-16  9:01 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Ludovic Desroches, open list:GPIO SUBSYSTEM,
	linux-arm Mailing List, Linus Walleij, Linux Kernel Mailing List,
	nicolas.free

Hi,

On Mon, Jan 15, 2018 at 10:19:39PM +0200, Andy Shevchenko wrote:
> On Mon, Jan 15, 2018 at 6:22 PM, Ludovic Desroches
> <ludovic.desroches@microchip.com> wrote:
> 
> Did I miss cover letter for this?
> 

It seems: https://lkml.org/lkml/2018/1/15/841

> > Add a consumer variant to GPIO request relative functions. The goal
> > is to fix the bad ownership, which is arbitrary set to
> > "range->name:gpio", of a GPIO.
> 
> Hmm... It's supposed to be name of the owner of the pin range (pin
> control device name IIUC).
>

Yes, the owner is the pinctrl device.

> > There is a lack of configuration features for GPIO. For instance,
> > we can't set the bias. Some pin controllers manage both device's
> > pins and GPIOs. GPIOs can benefit from pin configuration. Usually,
> > a pinctrl node is used to mux the pin as a GPIO and to set up its
> > configuration.
> 
> Don't we have means to do that?
> 

I don't think so. I am not the only one to have this issue for a long
time.

There is an interesting thread here:
https://www.spinics.net/lists/linux-gpio/msg16769.html

If things have changed and I missed it, please tell me. I have seen some
improvements but it still don't fit my needs.

> At least that what I see in aspeed_gpio_set_config().
> 

Yes this is part of the improvements I have seen. The set_config
operation is used at several places in the gpiolib:
- gpio_set_drive_single_ended
- gpiod_set_debounce
- gpiod_set_transitory

It doesn't cover all my needs. In the cover letter, I mentionned that I
based my job on this adding parameters I need. Someone told me it may be
difficult to pul all the parameters in one cell. For instance, the drive
strength is a numeric value using several bits.

I am not sure duplicating the parameters we have for pinconf is the
appropriate solution. Then I prepare a second set of patches to add a
cell with a phandle on the pin configuration. I was going to send it
when I realize that fixing the ownership issue is probably a better
solution. It may involve more code change but less duplication.

> Or I missed a point here?
> 
> > The pinmuxing strict mode involves that a pin which is muxed can't
> > be requested as a GPIO if the owner is not the same.
> 
> Any elaborated example?
> 

More details in the thread I mentionned earlier. I want to enable the
strict mode to prevent weird behavior using the sysfs (or the char
device). Moreover, the strict mode fits the behavior of my pin
controller.

In my DTS, at the moment, if a device needs GPIOs, then I add a pinctrl
node where I put the pinmuxing of the pins as GPIOs and I set the
configuration (for instance, bias pull-up, debounce). If the strict mode
is enabled, when the driver will request the GPIOs, it will fail even if
it's the owner of the pinmuxing.

> > Unfortunately,
> > the ownership of a GPIO is set arbitrarily to "range->name:gpio".
> > So there is a mismatch about the ownership which prevents a device
> > from being the owner of the pinmuxing and requesting the same pin as
> > a GPIO.
> 
> > Adding some consumer variants for GPIO request stuff will allow to
> > pass the name of the device which requests the GPIO to not return an
> > error if it's also the owner of the pinmuxing.
> 
> I think we need something more generic in core than producing more API
> functions.
> 

I didn't want to introduce too many changes to keep it safe and I didn't
want to break current API by adding a parameter.

> But I would like to get problem first.
> 
> > +       if (consumer)
> > +               return pin_request(pctldev, pin, consumer, range);
> > +
> 
> Hmm... My understanding that GPIO is just a (special) function out of
> pin muxing. So, doing musing is essential to get proper function out
> of it.
> 
> Does your hardware considers this differently? If so, I would really
> want to see some datasheets.

No you're right about the behavior of my hardware.

Regards

Ludovic

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH 1/2] pinctrl: add consumer variant for gpio request
  2018-01-16  9:01       ` Ludovic Desroches
  (?)
@ 2018-01-16 14:33       ` Andy Shevchenko
  2018-01-17 14:54           ` Ludovic Desroches
  -1 siblings, 1 reply; 23+ messages in thread
From: Andy Shevchenko @ 2018-01-16 14:33 UTC (permalink / raw)
  To: Andy Shevchenko, open list:GPIO SUBSYSTEM,
	linux-arm Mailing List, Linus Walleij, Linux Kernel Mailing List,
	nicolas.free
  Cc: Ludovic Desroches

On Tue, Jan 16, 2018 at 11:01 AM, Ludovic Desroches
<ludovic.desroches@microchip.com> wrote:
> On Mon, Jan 15, 2018 at 10:19:39PM +0200, Andy Shevchenko wrote:
>> On Mon, Jan 15, 2018 at 6:22 PM, Ludovic Desroches
>> <ludovic.desroches@microchip.com> wrote:
>>
>> Did I miss cover letter for this?
> It seems: https://lkml.org/lkml/2018/1/15/841

>> > Add a consumer variant to GPIO request relative functions. The goal
>> > is to fix the bad ownership, which is arbitrary set to
>> > "range->name:gpio", of a GPIO.
>>
>> Hmm... It's supposed to be name of the owner of the pin range (pin
>> control device name IIUC).

> Yes, the owner is the pinctrl device.

> There is an interesting thread here:
> https://www.spinics.net/lists/linux-gpio/msg16769.html

Okay, I have dove into all links provided. Below a set of my thoughts
with regard to topic.

> If things have changed and I missed it, please tell me. I have seen some
> improvements but it still don't fit my needs.

First of all, the main architectural issue with current pin control
design is so called "states". It's quite artificial entity which is
not represented by hardware per se.

Instead we better to provide an API to pin configuration and pin
muxing: I would like to switch to *this* pin function with *this* pin
configuration.

Second, the pin control and GPIOs are orthogonal as your hardware
confirms. The propagating pin configuration or muxing via GPIO API
looks to me a wrong direction.

To the point of the specific issue you are trying to solve.  I would
rather agree with Maxime:

"Something like "if the configuration is a gpio, and my pinctrl driver
is also a gpio controller, and that gpiod_request is being called on
that pin, hand over the reference"

Just in case of architectural review imagine a below case. We have two
UART devices which share same pins, and at the same time their pins
can be switched to GPIO function. So, use cases and questions:
- allow potential driver to switch between UARTs at run time
- allow UART driver to switch mode between no flow control (2 wire
mode) and HW flow (4 wire mode), though not specifically at run time
- allow GPIO function for some pins at run time to support OOB wake
up, for example, when UART is a console

More caveats:
- switching and reconfiguring pins at run time is a bad idea for the
start (only some exceptions can be applied, see above) because of
electrical properties of the devices and potential damage to the
hardware
- switching to GPIO is allowed at run time for the purpose of OOB wake source
- after switching to GPIO and freeing it the pin configuration (and
perhaps muxing) would return back to previous (before GPIO) settings
(this would be helpful to case described above: OOB wake up)

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH 1/2] pinctrl: add consumer variant for gpio request
  2018-01-16 14:33       ` Andy Shevchenko
  2018-01-17 14:54           ` Ludovic Desroches
@ 2018-01-17 14:54           ` Ludovic Desroches
  0 siblings, 0 replies; 23+ messages in thread
From: Ludovic Desroches @ 2018-01-17 14:54 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: open list:GPIO SUBSYSTEM, linux-arm Mailing List, Linus Walleij,
	Linux Kernel Mailing List, nicolas.ferre, Ludovic Desroches

On Tue, Jan 16, 2018 at 04:33:29PM +0200, Andy Shevchenko wrote:
> On Tue, Jan 16, 2018 at 11:01 AM, Ludovic Desroches
> <ludovic.desroches@microchip.com> wrote:
> > On Mon, Jan 15, 2018 at 10:19:39PM +0200, Andy Shevchenko wrote:
> >> On Mon, Jan 15, 2018 at 6:22 PM, Ludovic Desroches
> >> <ludovic.desroches@microchip.com> wrote:
> >>
> >> Did I miss cover letter for this?
> > It seems: https://lkml.org/lkml/2018/1/15/841
> 
> >> > Add a consumer variant to GPIO request relative functions. The goal
> >> > is to fix the bad ownership, which is arbitrary set to
> >> > "range->name:gpio", of a GPIO.
> >>
> >> Hmm... It's supposed to be name of the owner of the pin range (pin
> >> control device name IIUC).
> 
> > Yes, the owner is the pinctrl device.
> 
> > There is an interesting thread here:
> > https://www.spinics.net/lists/linux-gpio/msg16769.html
> 
> Okay, I have dove into all links provided. Below a set of my thoughts
> with regard to topic.
> 
> > If things have changed and I missed it, please tell me. I have seen some
> > improvements but it still don't fit my needs.
> 
> First of all, the main architectural issue with current pin control
> design is so called "states". It's quite artificial entity which is
> not represented by hardware per se.
> 
> Instead we better to provide an API to pin configuration and pin
> muxing: I would like to switch to *this* pin function with *this* pin
> configuration.
> 

gpio_request_enable() allows to switch to the GPIO pin function.

To clarify the situation, from my point of view there are two issues:

- Several pin controllers didn't enabled the strict mode when they were
  introduced. Nowadays, enabling it will break several DTs. Maybe a DT
  property to enable/disable strict mode could do the trick: we remove
  pinctrl nodes (so pinmux + pinconf) for pins which will be requested
  by gpiod_request() and we set the strict property to true.

- But... The GPIO pin configuration is missing. Some configuration features
  have been added, we can continue to add new ones but I am not sure
  it's the best solution. At the moment, we rely on a single cell to
  manage the configuration. It may not be enough and each time a new pin
  configuration will appear, it will have to be added both to the gpiolib
  and pinconf.

> Second, the pin control and GPIOs are orthogonal as your hardware
> confirms. The propagating pin configuration or muxing via GPIO API
> looks to me a wrong direction.
> 

I agree but it seems we have started to follow this path.

> To the point of the specific issue you are trying to solve.  I would
> rather agree with Maxime:
> 
> "Something like "if the configuration is a gpio, and my pinctrl driver
> is also a gpio controller, and that gpiod_request is being called on
> that pin, hand over the reference"
> 

Sorry, what do you see behind "hand over the reference"?

> Just in case of architectural review imagine a below case. We have two
> UART devices which share same pins, and at the same time their pins
> can be switched to GPIO function. So, use cases and questions:
> - allow potential driver to switch between UARTs at run time
> - allow UART driver to switch mode between no flow control (2 wire
> mode) and HW flow (4 wire mode), though not specifically at run time
> - allow GPIO function for some pins at run time to support OOB wake
> up, for example, when UART is a console
> 

I have the feeling that 1 and 2 are achievable with pinctrl states. 3 may be
also acheviable excepting if the pin is part of a pinctrl state.

> More caveats:
> - switching and reconfiguring pins at run time is a bad idea for the
> start (only some exceptions can be applied, see above) because of
> electrical properties of the devices and potential damage to the
> hardware
> - switching to GPIO is allowed at run time for the purpose of OOB wake source
> - after switching to GPIO and freeing it the pin configuration (and
> perhaps muxing) would return back to previous (before GPIO) settings
> (this would be helpful to case described above: OOB wake up)
> 

I share another case which is the one motivating me to do these patches.

I am writing a driver for a new device which uses several lines. The lines used
depends on the firmware loaded so there is no reason to reserve all the
lines and so the pins corresponding to these lines.

The pins will be requested as GPIOs because the pin controller in this specific
case is bypassed. I mean, if the device uses a line, it will take the ownership
even if it is muxed to a function. I want to prevent this.

Before using the line, I'll request the pin as a GPIO. If an error occurs (this
is why I need to enable the strict mode), it means the pin is already muxed and
we are going to break the device which uses it.

> -- 
> With Best Regards,
> Andy Shevchenko
> --
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH 1/2] pinctrl: add consumer variant for gpio request
@ 2018-01-17 14:54           ` Ludovic Desroches
  0 siblings, 0 replies; 23+ messages in thread
From: Ludovic Desroches @ 2018-01-17 14:54 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: open list:GPIO SUBSYSTEM, linux-arm Mailing List, Linus Walleij,
	Linux Kernel Mailing List, nicolas.ferre, Ludovic Desroches

On Tue, Jan 16, 2018 at 04:33:29PM +0200, Andy Shevchenko wrote:
> On Tue, Jan 16, 2018 at 11:01 AM, Ludovic Desroches
> <ludovic.desroches@microchip.com> wrote:
> > On Mon, Jan 15, 2018 at 10:19:39PM +0200, Andy Shevchenko wrote:
> >> On Mon, Jan 15, 2018 at 6:22 PM, Ludovic Desroches
> >> <ludovic.desroches@microchip.com> wrote:
> >>
> >> Did I miss cover letter for this?
> > It seems: https://lkml.org/lkml/2018/1/15/841
> 
> >> > Add a consumer variant to GPIO request relative functions. The goal
> >> > is to fix the bad ownership, which is arbitrary set to
> >> > "range->name:gpio", of a GPIO.
> >>
> >> Hmm... It's supposed to be name of the owner of the pin range (pin
> >> control device name IIUC).
> 
> > Yes, the owner is the pinctrl device.
> 
> > There is an interesting thread here:
> > https://www.spinics.net/lists/linux-gpio/msg16769.html
> 
> Okay, I have dove into all links provided. Below a set of my thoughts
> with regard to topic.
> 
> > If things have changed and I missed it, please tell me. I have seen some
> > improvements but it still don't fit my needs.
> 
> First of all, the main architectural issue with current pin control
> design is so called "states". It's quite artificial entity which is
> not represented by hardware per se.
> 
> Instead we better to provide an API to pin configuration and pin
> muxing: I would like to switch to *this* pin function with *this* pin
> configuration.
> 

gpio_request_enable() allows to switch to the GPIO pin function.

To clarify the situation, from my point of view there are two issues:

- Several pin controllers didn't enabled the strict mode when they were
  introduced. Nowadays, enabling it will break several DTs. Maybe a DT
  property to enable/disable strict mode could do the trick: we remove
  pinctrl nodes (so pinmux + pinconf) for pins which will be requested
  by gpiod_request() and we set the strict property to true.

- But... The GPIO pin configuration is missing. Some configuration features
  have been added, we can continue to add new ones but I am not sure
  it's the best solution. At the moment, we rely on a single cell to
  manage the configuration. It may not be enough and each time a new pin
  configuration will appear, it will have to be added both to the gpiolib
  and pinconf.

> Second, the pin control and GPIOs are orthogonal as your hardware
> confirms. The propagating pin configuration or muxing via GPIO API
> looks to me a wrong direction.
> 

I agree but it seems we have started to follow this path.

> To the point of the specific issue you are trying to solve.  I would
> rather agree with Maxime:
> 
> "Something like "if the configuration is a gpio, and my pinctrl driver
> is also a gpio controller, and that gpiod_request is being called on
> that pin, hand over the reference"
> 

Sorry, what do you see behind "hand over the reference"?

> Just in case of architectural review imagine a below case. We have two
> UART devices which share same pins, and at the same time their pins
> can be switched to GPIO function. So, use cases and questions:
> - allow potential driver to switch between UARTs at run time
> - allow UART driver to switch mode between no flow control (2 wire
> mode) and HW flow (4 wire mode), though not specifically at run time
> - allow GPIO function for some pins at run time to support OOB wake
> up, for example, when UART is a console
> 

I have the feeling that 1 and 2 are achievable with pinctrl states. 3 may be
also acheviable excepting if the pin is part of a pinctrl state.

> More caveats:
> - switching and reconfiguring pins at run time is a bad idea for the
> start (only some exceptions can be applied, see above) because of
> electrical properties of the devices and potential damage to the
> hardware
> - switching to GPIO is allowed at run time for the purpose of OOB wake source
> - after switching to GPIO and freeing it the pin configuration (and
> perhaps muxing) would return back to previous (before GPIO) settings
> (this would be helpful to case described above: OOB wake up)
> 

I share another case which is the one motivating me to do these patches.

I am writing a driver for a new device which uses several lines. The lines used
depends on the firmware loaded so there is no reason to reserve all the
lines and so the pins corresponding to these lines.

The pins will be requested as GPIOs because the pin controller in this specific
case is bypassed. I mean, if the device uses a line, it will take the ownership
even if it is muxed to a function. I want to prevent this.

Before using the line, I'll request the pin as a GPIO. If an error occurs (this
is why I need to enable the strict mode), it means the pin is already muxed and
we are going to break the device which uses it.

> -- 
> With Best Regards,
> Andy Shevchenko
> --
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [RFC PATCH 1/2] pinctrl: add consumer variant for gpio request
@ 2018-01-17 14:54           ` Ludovic Desroches
  0 siblings, 0 replies; 23+ messages in thread
From: Ludovic Desroches @ 2018-01-17 14:54 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Jan 16, 2018 at 04:33:29PM +0200, Andy Shevchenko wrote:
> On Tue, Jan 16, 2018 at 11:01 AM, Ludovic Desroches
> <ludovic.desroches@microchip.com> wrote:
> > On Mon, Jan 15, 2018 at 10:19:39PM +0200, Andy Shevchenko wrote:
> >> On Mon, Jan 15, 2018 at 6:22 PM, Ludovic Desroches
> >> <ludovic.desroches@microchip.com> wrote:
> >>
> >> Did I miss cover letter for this?
> > It seems: https://lkml.org/lkml/2018/1/15/841
> 
> >> > Add a consumer variant to GPIO request relative functions. The goal
> >> > is to fix the bad ownership, which is arbitrary set to
> >> > "range->name:gpio", of a GPIO.
> >>
> >> Hmm... It's supposed to be name of the owner of the pin range (pin
> >> control device name IIUC).
> 
> > Yes, the owner is the pinctrl device.
> 
> > There is an interesting thread here:
> > https://www.spinics.net/lists/linux-gpio/msg16769.html
> 
> Okay, I have dove into all links provided. Below a set of my thoughts
> with regard to topic.
> 
> > If things have changed and I missed it, please tell me. I have seen some
> > improvements but it still don't fit my needs.
> 
> First of all, the main architectural issue with current pin control
> design is so called "states". It's quite artificial entity which is
> not represented by hardware per se.
> 
> Instead we better to provide an API to pin configuration and pin
> muxing: I would like to switch to *this* pin function with *this* pin
> configuration.
> 

gpio_request_enable() allows to switch to the GPIO pin function.

To clarify the situation, from my point of view there are two issues:

- Several pin controllers didn't enabled the strict mode when they were
  introduced. Nowadays, enabling it will break several DTs. Maybe a DT
  property to enable/disable strict mode could do the trick: we remove
  pinctrl nodes (so pinmux + pinconf) for pins which will be requested
  by gpiod_request() and we set the strict property to true.

- But... The GPIO pin configuration is missing. Some configuration features
  have been added, we can continue to add new ones but I am not sure
  it's the best solution. At the moment, we rely on a single cell to
  manage the configuration. It may not be enough and each time a new pin
  configuration will appear, it will have to be added both to the gpiolib
  and pinconf.

> Second, the pin control and GPIOs are orthogonal as your hardware
> confirms. The propagating pin configuration or muxing via GPIO API
> looks to me a wrong direction.
> 

I agree but it seems we have started to follow this path.

> To the point of the specific issue you are trying to solve.  I would
> rather agree with Maxime:
> 
> "Something like "if the configuration is a gpio, and my pinctrl driver
> is also a gpio controller, and that gpiod_request is being called on
> that pin, hand over the reference"
> 

Sorry, what do you see behind "hand over the reference"?

> Just in case of architectural review imagine a below case. We have two
> UART devices which share same pins, and at the same time their pins
> can be switched to GPIO function. So, use cases and questions:
> - allow potential driver to switch between UARTs at run time
> - allow UART driver to switch mode between no flow control (2 wire
> mode) and HW flow (4 wire mode), though not specifically at run time
> - allow GPIO function for some pins at run time to support OOB wake
> up, for example, when UART is a console
> 

I have the feeling that 1 and 2 are achievable with pinctrl states. 3 may be
also acheviable excepting if the pin is part of a pinctrl state.

> More caveats:
> - switching and reconfiguring pins at run time is a bad idea for the
> start (only some exceptions can be applied, see above) because of
> electrical properties of the devices and potential damage to the
> hardware
> - switching to GPIO is allowed at run time for the purpose of OOB wake source
> - after switching to GPIO and freeing it the pin configuration (and
> perhaps muxing) would return back to previous (before GPIO) settings
> (this would be helpful to case described above: OOB wake up)
> 

I share another case which is the one motivating me to do these patches.

I am writing a driver for a new device which uses several lines. The lines used
depends on the firmware loaded so there is no reason to reserve all the
lines and so the pins corresponding to these lines.

The pins will be requested as GPIOs because the pin controller in this specific
case is bypassed. I mean, if the device uses a line, it will take the ownership
even if it is muxed to a function. I want to prevent this.

Before using the line, I'll request the pin as a GPIO. If an error occurs (this
is why I need to enable the strict mode), it means the pin is already muxed and
we are going to break the device which uses it.

> -- 
> With Best Regards,
> Andy Shevchenko
> --
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH 1/2] pinctrl: add consumer variant for gpio request
  2018-01-17 14:54           ` Ludovic Desroches
@ 2018-01-17 16:07             ` Andy Shevchenko
  -1 siblings, 0 replies; 23+ messages in thread
From: Andy Shevchenko @ 2018-01-17 16:07 UTC (permalink / raw)
  To: Andy Shevchenko, open list:GPIO SUBSYSTEM,
	linux-arm Mailing List, Linus Walleij, Linux Kernel Mailing List,
	Nicolas Ferre
  Cc: Ludovic Desroches

On Wed, Jan 17, 2018 at 4:54 PM, Ludovic Desroches
<ludovic.desroches@microchip.com> wrote:
> On Tue, Jan 16, 2018 at 04:33:29PM +0200, Andy Shevchenko wrote:

>> First of all, the main architectural issue with current pin control
>> design is so called "states". It's quite artificial entity which is
>> not represented by hardware per se.
>>
>> Instead we better to provide an API to pin configuration and pin
>> muxing: I would like to switch to *this* pin function with *this* pin
>> configuration.

> gpio_request_enable() allows to switch to the GPIO pin function.

...as a particular case of "set this pin to this function".

> To clarify the situation, from my point of view there are two issues:
>
> - Several pin controllers didn't enabled the strict mode when they were
>   introduced. Nowadays, enabling it will break several DTs. Maybe a DT
>   property to enable/disable strict mode could do the trick: we remove
>   pinctrl nodes (so pinmux + pinconf) for pins which will be requested
>   by gpiod_request() and we set the strict property to true.

OK.

> - But... The GPIO pin configuration is missing.

Here you mixed up the things. Either we are talking about GPIO
configuration (direction, enabling/disabling I/O buffers), or we are
talking about pin configuration (bias, drive strength, slew rate,
debounce, etc.).

>  Some configuration features
>   have been added, we can continue to add new ones but I am not sure
>   it's the best solution.

See below.

>  At the moment, we rely on a single cell to
>   manage the configuration. It may not be enough and each time a new pin
>   configuration will appear, it will have to be added both to the gpiolib
>   and pinconf.

>> Second, the pin control and GPIOs are orthogonal as your hardware
>> confirms. The propagating pin configuration or muxing via GPIO API
>> looks to me a wrong direction.
>>
>
> I agree but it seems we have started to follow this path.

Which is still wrong and nothing prevents us to just stop here and
consider better way.

The issue is how we historically represent pins inside kernel and how
hardware evolves at the same time.

Looking from now retrospectively I can state the following:
- each GPIO controller *might* have (few) capabilities of pin configuration
- pin control may not necessary have GPIO function of the pin

Design is now GPIO oriented while it should be pin oriented.

So, instead of littering GPIO API, would we consider to redesign a bit
from the above point of view?

(Read ahead: to be backward compatible and be more friendly for simple
GPIO IPs it would make sense to leave some of the basic pin properties
to be controlled from GPIO API, otherwise forget completely about GPIO
driver, and think of pin control driver for new complex IPs)

[pin]: might have attached set of functions, set of [electrical] properties.
It might be re-configured run time in terms of function and configuration.
It might have none, one, or more owners (this is already an OS abstraction)

Thus, the core function is pin request, while GPIO request is just a
particular case.
Owner of the pin is defined independently on what function or
configuration is chosen.

Therefore, we will have something like this (permissions):
- none (no one can do anything, except acquiring an ownership == requesting pin)
- exclusive (only one user allowed to cover all properties of the pin)
- shared (several owners can do modify all or selected properties)
- shared for all (anyone can do anything, perhaps we don't need this)

>> To the point of the specific issue you are trying to solve.  I would
>> rather agree with Maxime:
>>
>> "Something like "if the configuration is a gpio, and my pinctrl driver
>> is also a gpio controller, and that gpiod_request is being called on
>> that pin, hand over the reference"

> Sorry, what do you see behind "hand over the reference"?

This is similar to what I called before as "shared" ownership. To be
precise, transformation "exclusive" to "shared".

>> Just in case of architectural review imagine a below case. We have two
>> UART devices which share same pins, and at the same time their pins
>> can be switched to GPIO function. So, use cases and questions:
>> - allow potential driver to switch between UARTs at run time
>> - allow UART driver to switch mode between no flow control (2 wire
>> mode) and HW flow (4 wire mode), though not specifically at run time
>> - allow GPIO function for some pins at run time to support OOB wake
>> up, for example, when UART is a console
>>
>
> I have the feeling that 1 and 2 are achievable with pinctrl states. 3 may be
> also acheviable excepting if the pin is part of a pinctrl state.

Please, no artificial states here. It brings more issues than solves.
Deterministic is:
- pin electrical properties
- current (active) function
- current (active) owner(s)

Whatever currently means "states" they are defined per owner basis.

>> More caveats:
>> - switching and reconfiguring pins at run time is a bad idea for the
>> start (only some exceptions can be applied, see above) because of
>> electrical properties of the devices and potential damage to the
>> hardware
>> - switching to GPIO is allowed at run time for the purpose of OOB wake source
>> - after switching to GPIO and freeing it the pin configuration (and
>> perhaps muxing) would return back to previous (before GPIO) settings
>> (this would be helpful to case described above: OOB wake up)
>>
>
> I share another case which is the one motivating me to do these patches.
>
> I am writing a driver for a new device which uses several lines. The lines used
> depends on the firmware loaded so there is no reason to reserve all the
> lines and so the pins corresponding to these lines.

Reserve how? In DT?

> The pins will be requested as GPIOs because the pin controller in this specific
> case is bypassed. I mean, if the device uses a line, it will take the ownership
> even if it is muxed to a function. I want to prevent this.

Yes, valid point.

> Before using the line, I'll request the pin as a GPIO. If an error occurs (this
> is why I need to enable the strict mode), it means the pin is already muxed and
> we are going to break the device which uses it.

Correct, or any other function.

What we need is pin_request_function() API and pin_set_config(). GPIO
is kinda legacy (in terns of configuring and controlling pins).

So, consider my idea above about ownership. It would give you less
pain how to proceed with pins. In DT or ACPI we may supply a property
to tell OS how ownership has to be handled:
strict (or "exclusive", one owner for pin properties), "shared", or
"none" (not requested, first come, first served)

Yes, pin function might need a special type of owners to do power
management, for example, but in above scheme it would be just a
subtype of "shared" (okay, "strict" in that way also would "shared"
but only for PM core).

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [RFC PATCH 1/2] pinctrl: add consumer variant for gpio request
@ 2018-01-17 16:07             ` Andy Shevchenko
  0 siblings, 0 replies; 23+ messages in thread
From: Andy Shevchenko @ 2018-01-17 16:07 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Jan 17, 2018 at 4:54 PM, Ludovic Desroches
<ludovic.desroches@microchip.com> wrote:
> On Tue, Jan 16, 2018 at 04:33:29PM +0200, Andy Shevchenko wrote:

>> First of all, the main architectural issue with current pin control
>> design is so called "states". It's quite artificial entity which is
>> not represented by hardware per se.
>>
>> Instead we better to provide an API to pin configuration and pin
>> muxing: I would like to switch to *this* pin function with *this* pin
>> configuration.

> gpio_request_enable() allows to switch to the GPIO pin function.

...as a particular case of "set this pin to this function".

> To clarify the situation, from my point of view there are two issues:
>
> - Several pin controllers didn't enabled the strict mode when they were
>   introduced. Nowadays, enabling it will break several DTs. Maybe a DT
>   property to enable/disable strict mode could do the trick: we remove
>   pinctrl nodes (so pinmux + pinconf) for pins which will be requested
>   by gpiod_request() and we set the strict property to true.

OK.

> - But... The GPIO pin configuration is missing.

Here you mixed up the things. Either we are talking about GPIO
configuration (direction, enabling/disabling I/O buffers), or we are
talking about pin configuration (bias, drive strength, slew rate,
debounce, etc.).

>  Some configuration features
>   have been added, we can continue to add new ones but I am not sure
>   it's the best solution.

See below.

>  At the moment, we rely on a single cell to
>   manage the configuration. It may not be enough and each time a new pin
>   configuration will appear, it will have to be added both to the gpiolib
>   and pinconf.

>> Second, the pin control and GPIOs are orthogonal as your hardware
>> confirms. The propagating pin configuration or muxing via GPIO API
>> looks to me a wrong direction.
>>
>
> I agree but it seems we have started to follow this path.

Which is still wrong and nothing prevents us to just stop here and
consider better way.

The issue is how we historically represent pins inside kernel and how
hardware evolves at the same time.

Looking from now retrospectively I can state the following:
- each GPIO controller *might* have (few) capabilities of pin configuration
- pin control may not necessary have GPIO function of the pin

Design is now GPIO oriented while it should be pin oriented.

So, instead of littering GPIO API, would we consider to redesign a bit
from the above point of view?

(Read ahead: to be backward compatible and be more friendly for simple
GPIO IPs it would make sense to leave some of the basic pin properties
to be controlled from GPIO API, otherwise forget completely about GPIO
driver, and think of pin control driver for new complex IPs)

[pin]: might have attached set of functions, set of [electrical] properties.
It might be re-configured run time in terms of function and configuration.
It might have none, one, or more owners (this is already an OS abstraction)

Thus, the core function is pin request, while GPIO request is just a
particular case.
Owner of the pin is defined independently on what function or
configuration is chosen.

Therefore, we will have something like this (permissions):
- none (no one can do anything, except acquiring an ownership == requesting pin)
- exclusive (only one user allowed to cover all properties of the pin)
- shared (several owners can do modify all or selected properties)
- shared for all (anyone can do anything, perhaps we don't need this)

>> To the point of the specific issue you are trying to solve.  I would
>> rather agree with Maxime:
>>
>> "Something like "if the configuration is a gpio, and my pinctrl driver
>> is also a gpio controller, and that gpiod_request is being called on
>> that pin, hand over the reference"

> Sorry, what do you see behind "hand over the reference"?

This is similar to what I called before as "shared" ownership. To be
precise, transformation "exclusive" to "shared".

>> Just in case of architectural review imagine a below case. We have two
>> UART devices which share same pins, and at the same time their pins
>> can be switched to GPIO function. So, use cases and questions:
>> - allow potential driver to switch between UARTs at run time
>> - allow UART driver to switch mode between no flow control (2 wire
>> mode) and HW flow (4 wire mode), though not specifically at run time
>> - allow GPIO function for some pins at run time to support OOB wake
>> up, for example, when UART is a console
>>
>
> I have the feeling that 1 and 2 are achievable with pinctrl states. 3 may be
> also acheviable excepting if the pin is part of a pinctrl state.

Please, no artificial states here. It brings more issues than solves.
Deterministic is:
- pin electrical properties
- current (active) function
- current (active) owner(s)

Whatever currently means "states" they are defined per owner basis.

>> More caveats:
>> - switching and reconfiguring pins at run time is a bad idea for the
>> start (only some exceptions can be applied, see above) because of
>> electrical properties of the devices and potential damage to the
>> hardware
>> - switching to GPIO is allowed at run time for the purpose of OOB wake source
>> - after switching to GPIO and freeing it the pin configuration (and
>> perhaps muxing) would return back to previous (before GPIO) settings
>> (this would be helpful to case described above: OOB wake up)
>>
>
> I share another case which is the one motivating me to do these patches.
>
> I am writing a driver for a new device which uses several lines. The lines used
> depends on the firmware loaded so there is no reason to reserve all the
> lines and so the pins corresponding to these lines.

Reserve how? In DT?

> The pins will be requested as GPIOs because the pin controller in this specific
> case is bypassed. I mean, if the device uses a line, it will take the ownership
> even if it is muxed to a function. I want to prevent this.

Yes, valid point.

> Before using the line, I'll request the pin as a GPIO. If an error occurs (this
> is why I need to enable the strict mode), it means the pin is already muxed and
> we are going to break the device which uses it.

Correct, or any other function.

What we need is pin_request_function() API and pin_set_config(). GPIO
is kinda legacy (in terns of configuring and controlling pins).

So, consider my idea above about ownership. It would give you less
pain how to proceed with pins. In DT or ACPI we may supply a property
to tell OS how ownership has to be handled:
strict (or "exclusive", one owner for pin properties), "shared", or
"none" (not requested, first come, first served)

Yes, pin function might need a special type of owners to do power
management, for example, but in above scheme it would be just a
subtype of "shared" (okay, "strict" in that way also would "shared"
but only for PM core).

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH 1/2] pinctrl: add consumer variant for gpio request
  2018-01-17 16:07             ` Andy Shevchenko
@ 2018-01-18  7:56               ` Ludovic Desroches
  -1 siblings, 0 replies; 23+ messages in thread
From: Ludovic Desroches @ 2018-01-18  7:56 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: open list:GPIO SUBSYSTEM, linux-arm Mailing List, Linus Walleij,
	Linux Kernel Mailing List, Nicolas Ferre, Ludovic Desroches

On Wed, Jan 17, 2018 at 06:07:02PM +0200, Andy Shevchenko wrote:
> On Wed, Jan 17, 2018 at 4:54 PM, Ludovic Desroches
> <ludovic.desroches@microchip.com> wrote:
> > On Tue, Jan 16, 2018 at 04:33:29PM +0200, Andy Shevchenko wrote:
> 
> >> First of all, the main architectural issue with current pin control
> >> design is so called "states". It's quite artificial entity which is
> >> not represented by hardware per se.
> >>
> >> Instead we better to provide an API to pin configuration and pin
> >> muxing: I would like to switch to *this* pin function with *this* pin
> >> configuration.
> 
> > gpio_request_enable() allows to switch to the GPIO pin function.
> 
> ...as a particular case of "set this pin to this function".
> 
> > To clarify the situation, from my point of view there are two issues:
> >
> > - Several pin controllers didn't enabled the strict mode when they were
> >   introduced. Nowadays, enabling it will break several DTs. Maybe a DT
> >   property to enable/disable strict mode could do the trick: we remove
> >   pinctrl nodes (so pinmux + pinconf) for pins which will be requested
> >   by gpiod_request() and we set the strict property to true.
> 
> OK.
> 
> > - But... The GPIO pin configuration is missing.
> 
> Here you mixed up the things. Either we are talking about GPIO
> configuration (direction, enabling/disabling I/O buffers), or we are
> talking about pin configuration (bias, drive strength, slew rate,
> debounce, etc.).

I was talking about the pin configuration of the GPIO so about bias,
drive strength, etc.

> 
> >  Some configuration features
> >   have been added, we can continue to add new ones but I am not sure
> >   it's the best solution.
> 
> See below.
> 
> >  At the moment, we rely on a single cell to
> >   manage the configuration. It may not be enough and each time a new pin
> >   configuration will appear, it will have to be added both to the gpiolib
> >   and pinconf.
> 
> >> Second, the pin control and GPIOs are orthogonal as your hardware
> >> confirms. The propagating pin configuration or muxing via GPIO API
> >> looks to me a wrong direction.
> >>
> >
> > I agree but it seems we have started to follow this path.
> 
> Which is still wrong and nothing prevents us to just stop here and
> consider better way.
> 
> The issue is how we historically represent pins inside kernel and how
> hardware evolves at the same time.
> 
> Looking from now retrospectively I can state the following:
> - each GPIO controller *might* have (few) capabilities of pin configuration
> - pin control may not necessary have GPIO function of the pin
> 
> Design is now GPIO oriented while it should be pin oriented.
> 

Agree

> So, instead of littering GPIO API, would we consider to redesign a bit
> from the above point of view?
> 
> (Read ahead: to be backward compatible and be more friendly for simple
> GPIO IPs it would make sense to leave some of the basic pin properties
> to be controlled from GPIO API, otherwise forget completely about GPIO
> driver, and think of pin control driver for new complex IPs)
> 
> [pin]: might have attached set of functions, set of [electrical] properties.
> It might be re-configured run time in terms of function and configuration.
> It might have none, one, or more owners (this is already an OS abstraction)
> 
> Thus, the core function is pin request, while GPIO request is just a
> particular case.
> Owner of the pin is defined independently on what function or
> configuration is chosen.
> 
> Therefore, we will have something like this (permissions):
> - none (no one can do anything, except acquiring an ownership == requesting pin)
> - exclusive (only one user allowed to cover all properties of the pin)
> - shared (several owners can do modify all or selected properties)
> - shared for all (anyone can do anything, perhaps we don't need this)
> 

It seems nice.

> >> To the point of the specific issue you are trying to solve.  I would
> >> rather agree with Maxime:
> >>
> >> "Something like "if the configuration is a gpio, and my pinctrl driver
> >> is also a gpio controller, and that gpiod_request is being called on
> >> that pin, hand over the reference"
> 
> > Sorry, what do you see behind "hand over the reference"?
> 
> This is similar to what I called before as "shared" ownership. To be
> precise, transformation "exclusive" to "shared".
> 
> >> Just in case of architectural review imagine a below case. We have two
> >> UART devices which share same pins, and at the same time their pins
> >> can be switched to GPIO function. So, use cases and questions:
> >> - allow potential driver to switch between UARTs at run time
> >> - allow UART driver to switch mode between no flow control (2 wire
> >> mode) and HW flow (4 wire mode), though not specifically at run time
> >> - allow GPIO function for some pins at run time to support OOB wake
> >> up, for example, when UART is a console
> >>
> >
> > I have the feeling that 1 and 2 are achievable with pinctrl states. 3 may be
> > also acheviable excepting if the pin is part of a pinctrl state.
> 
> Please, no artificial states here. It brings more issues than solves.
> Deterministic is:
> - pin electrical properties
> - current (active) function
> - current (active) owner(s)
> 
> Whatever currently means "states" they are defined per owner basis.
> 
> >> More caveats:
> >> - switching and reconfiguring pins at run time is a bad idea for the
> >> start (only some exceptions can be applied, see above) because of
> >> electrical properties of the devices and potential damage to the
> >> hardware
> >> - switching to GPIO is allowed at run time for the purpose of OOB wake source
> >> - after switching to GPIO and freeing it the pin configuration (and
> >> perhaps muxing) would return back to previous (before GPIO) settings
> >> (this would be helpful to case described above: OOB wake up)
> >>
> >
> > I share another case which is the one motivating me to do these patches.
> >
> > I am writing a driver for a new device which uses several lines. The lines used
> > depends on the firmware loaded so there is no reason to reserve all the
> > lines and so the pins corresponding to these lines.
> 
> Reserve how? In DT?
> 

Yes with the usual pinctrl node describing all the pins which can be used by
the device.

Since the device pins is only SoC dependant, I would like to get the
pins list in the driver (depending on the compatible string) or using
line_name-gpio in the DT. I am not sure about which solution is the best
one. Then, once the firmware is loaded, I can pick only the pins I need.

> > The pins will be requested as GPIOs because the pin controller in this specific
> > case is bypassed. I mean, if the device uses a line, it will take the ownership
> > even if it is muxed to a function. I want to prevent this.
> 
> Yes, valid point.
> 
> > Before using the line, I'll request the pin as a GPIO. If an error occurs (this
> > is why I need to enable the strict mode), it means the pin is already muxed and
> > we are going to break the device which uses it.
> 
> Correct, or any other function.
> 
> What we need is pin_request_function() API and pin_set_config(). GPIO
> is kinda legacy (in terns of configuring and controlling pins).
> 
> So, consider my idea above about ownership. It would give you less
> pain how to proceed with pins. In DT or ACPI we may supply a property
> to tell OS how ownership has to be handled:
> strict (or "exclusive", one owner for pin properties), "shared", or
> "none" (not requested, first come, first served)
> 

I have to clear up my mind regarding your interesting proposals.

> Yes, pin function might need a special type of owners to do power
> management, for example, but in above scheme it would be just a
> subtype of "shared" (okay, "strict" in that way also would "shared"
> but only for PM core).
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> --
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [RFC PATCH 1/2] pinctrl: add consumer variant for gpio request
@ 2018-01-18  7:56               ` Ludovic Desroches
  0 siblings, 0 replies; 23+ messages in thread
From: Ludovic Desroches @ 2018-01-18  7:56 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Jan 17, 2018 at 06:07:02PM +0200, Andy Shevchenko wrote:
> On Wed, Jan 17, 2018 at 4:54 PM, Ludovic Desroches
> <ludovic.desroches@microchip.com> wrote:
> > On Tue, Jan 16, 2018 at 04:33:29PM +0200, Andy Shevchenko wrote:
> 
> >> First of all, the main architectural issue with current pin control
> >> design is so called "states". It's quite artificial entity which is
> >> not represented by hardware per se.
> >>
> >> Instead we better to provide an API to pin configuration and pin
> >> muxing: I would like to switch to *this* pin function with *this* pin
> >> configuration.
> 
> > gpio_request_enable() allows to switch to the GPIO pin function.
> 
> ...as a particular case of "set this pin to this function".
> 
> > To clarify the situation, from my point of view there are two issues:
> >
> > - Several pin controllers didn't enabled the strict mode when they were
> >   introduced. Nowadays, enabling it will break several DTs. Maybe a DT
> >   property to enable/disable strict mode could do the trick: we remove
> >   pinctrl nodes (so pinmux + pinconf) for pins which will be requested
> >   by gpiod_request() and we set the strict property to true.
> 
> OK.
> 
> > - But... The GPIO pin configuration is missing.
> 
> Here you mixed up the things. Either we are talking about GPIO
> configuration (direction, enabling/disabling I/O buffers), or we are
> talking about pin configuration (bias, drive strength, slew rate,
> debounce, etc.).

I was talking about the pin configuration of the GPIO so about bias,
drive strength, etc.

> 
> >  Some configuration features
> >   have been added, we can continue to add new ones but I am not sure
> >   it's the best solution.
> 
> See below.
> 
> >  At the moment, we rely on a single cell to
> >   manage the configuration. It may not be enough and each time a new pin
> >   configuration will appear, it will have to be added both to the gpiolib
> >   and pinconf.
> 
> >> Second, the pin control and GPIOs are orthogonal as your hardware
> >> confirms. The propagating pin configuration or muxing via GPIO API
> >> looks to me a wrong direction.
> >>
> >
> > I agree but it seems we have started to follow this path.
> 
> Which is still wrong and nothing prevents us to just stop here and
> consider better way.
> 
> The issue is how we historically represent pins inside kernel and how
> hardware evolves at the same time.
> 
> Looking from now retrospectively I can state the following:
> - each GPIO controller *might* have (few) capabilities of pin configuration
> - pin control may not necessary have GPIO function of the pin
> 
> Design is now GPIO oriented while it should be pin oriented.
> 

Agree

> So, instead of littering GPIO API, would we consider to redesign a bit
> from the above point of view?
> 
> (Read ahead: to be backward compatible and be more friendly for simple
> GPIO IPs it would make sense to leave some of the basic pin properties
> to be controlled from GPIO API, otherwise forget completely about GPIO
> driver, and think of pin control driver for new complex IPs)
> 
> [pin]: might have attached set of functions, set of [electrical] properties.
> It might be re-configured run time in terms of function and configuration.
> It might have none, one, or more owners (this is already an OS abstraction)
> 
> Thus, the core function is pin request, while GPIO request is just a
> particular case.
> Owner of the pin is defined independently on what function or
> configuration is chosen.
> 
> Therefore, we will have something like this (permissions):
> - none (no one can do anything, except acquiring an ownership == requesting pin)
> - exclusive (only one user allowed to cover all properties of the pin)
> - shared (several owners can do modify all or selected properties)
> - shared for all (anyone can do anything, perhaps we don't need this)
> 

It seems nice.

> >> To the point of the specific issue you are trying to solve.  I would
> >> rather agree with Maxime:
> >>
> >> "Something like "if the configuration is a gpio, and my pinctrl driver
> >> is also a gpio controller, and that gpiod_request is being called on
> >> that pin, hand over the reference"
> 
> > Sorry, what do you see behind "hand over the reference"?
> 
> This is similar to what I called before as "shared" ownership. To be
> precise, transformation "exclusive" to "shared".
> 
> >> Just in case of architectural review imagine a below case. We have two
> >> UART devices which share same pins, and at the same time their pins
> >> can be switched to GPIO function. So, use cases and questions:
> >> - allow potential driver to switch between UARTs at run time
> >> - allow UART driver to switch mode between no flow control (2 wire
> >> mode) and HW flow (4 wire mode), though not specifically at run time
> >> - allow GPIO function for some pins at run time to support OOB wake
> >> up, for example, when UART is a console
> >>
> >
> > I have the feeling that 1 and 2 are achievable with pinctrl states. 3 may be
> > also acheviable excepting if the pin is part of a pinctrl state.
> 
> Please, no artificial states here. It brings more issues than solves.
> Deterministic is:
> - pin electrical properties
> - current (active) function
> - current (active) owner(s)
> 
> Whatever currently means "states" they are defined per owner basis.
> 
> >> More caveats:
> >> - switching and reconfiguring pins at run time is a bad idea for the
> >> start (only some exceptions can be applied, see above) because of
> >> electrical properties of the devices and potential damage to the
> >> hardware
> >> - switching to GPIO is allowed at run time for the purpose of OOB wake source
> >> - after switching to GPIO and freeing it the pin configuration (and
> >> perhaps muxing) would return back to previous (before GPIO) settings
> >> (this would be helpful to case described above: OOB wake up)
> >>
> >
> > I share another case which is the one motivating me to do these patches.
> >
> > I am writing a driver for a new device which uses several lines. The lines used
> > depends on the firmware loaded so there is no reason to reserve all the
> > lines and so the pins corresponding to these lines.
> 
> Reserve how? In DT?
> 

Yes with the usual pinctrl node describing all the pins which can be used by
the device.

Since the device pins is only SoC dependant, I would like to get the
pins list in the driver (depending on the compatible string) or using
line_name-gpio in the DT. I am not sure about which solution is the best
one. Then, once the firmware is loaded, I can pick only the pins I need.

> > The pins will be requested as GPIOs because the pin controller in this specific
> > case is bypassed. I mean, if the device uses a line, it will take the ownership
> > even if it is muxed to a function. I want to prevent this.
> 
> Yes, valid point.
> 
> > Before using the line, I'll request the pin as a GPIO. If an error occurs (this
> > is why I need to enable the strict mode), it means the pin is already muxed and
> > we are going to break the device which uses it.
> 
> Correct, or any other function.
> 
> What we need is pin_request_function() API and pin_set_config(). GPIO
> is kinda legacy (in terns of configuring and controlling pins).
> 
> So, consider my idea above about ownership. It would give you less
> pain how to proceed with pins. In DT or ACPI we may supply a property
> to tell OS how ownership has to be handled:
> strict (or "exclusive", one owner for pin properties), "shared", or
> "none" (not requested, first come, first served)
> 

I have to clear up my mind regarding your interesting proposals.

> Yes, pin function might need a special type of owners to do power
> management, for example, but in above scheme it would be just a
> subtype of "shared" (okay, "strict" in that way also would "shared"
> but only for PM core).
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> --
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH 1/2] pinctrl: add consumer variant for gpio request
  2018-01-15 16:22   ` Ludovic Desroches
@ 2018-01-18  9:46     ` Linus Walleij
  -1 siblings, 0 replies; 23+ messages in thread
From: Linus Walleij @ 2018-01-18  9:46 UTC (permalink / raw)
  To: Ludovic Desroches; +Cc: linux-gpio, Linux ARM, linux-kernel, nicolas.free

On Mon, Jan 15, 2018 at 5:22 PM, Ludovic Desroches
<ludovic.desroches@microchip.com> wrote:

> Add a consumer variant to GPIO request relative functions. The goal
> is to fix the bad ownership, which is arbitrary set to
> "range->name:gpio", of a GPIO.

For this patch on its own (apart from the context):

I what you want to achieve is to pass a consumer name from
gpiolib to pinmux portions of pinctrl, then augment the existing
pinctrl_gpio_request() to pass an optional consumer name,
and change all existing in-kernel users to just pass NULL
and then use the range name as fallback if the consumer
name is NULL.

> There is a lack of configuration features for GPIO. For instance,
> we can't set the bias. Some pin controllers manage both device's
> pins and GPIOs. GPIOs can benefit from pin configuration. Usually,
> a pinctrl node is used to mux the pin as a GPIO and to set up its
> configuration.

Pin config takes care of bias, pull up/down, drive strength etc etc.

GPIO hammers lines 0->1, 1->2, and reads them as 0 or 1.
It can group lines into arrays etc but that is what it does.

The two systems are cross-connected using the GPIO ranges.

There are cross-calls for GPIO to ask pin controllers for favors:
extern int pinctrl_gpio_request(unsigned gpio);
extern void pinctrl_gpio_free(unsigned gpio);
extern int pinctrl_gpio_direction_input(unsigned gpio);
extern int pinctrl_gpio_direction_output(unsigned gpio);
extern int pinctrl_gpio_set_config(unsigned gpio, unsigned long config);

Note: when the GPIO hardware is very simple apart from some extra
register or two providing open drain and/or debounce, the gpio driver
can simply implement .set_config() itself so no pin control back-end
is needed. We could require a separate pin config driver but why
complicate things for the sake of abstraction. Nah.

The last API (pinctrl_gpio_set_config()) should only be called
to set up electrical properties under special circumstances.

Those are as follows:

1) When the in-kernel client needs to configure electrical properties,
gpiolib exposes interfaces for this, such as:
int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce);
int gpiod_set_transitory(struct gpio_desc *desc, bool transitory);

Likewise devm_gpiod_get() can pass flags such as
GPIOD_OUT_LOW_OPEN_DRAIN to state that "I want this line,
and I want it to be initialized logical low (deasserted), and it must
be open drain" If the corresponding device tree phandle flag or board file
description or whatever is not also flagging the line as
open drain, gpiolib will protest with a warning print and
enforce open drain. But it is clear that it *should* have been
configured correctly in the device tree.

This API is not all inclusive: notice that we just support open drain,
not open source. (No upfront design: we just deal with what drivers
need, not what they may theoretically need.)

This is partly for historical reasons, but it also makes sense that things
like I2C that can only work electrically with open drain, has a
way to specify that these lines must really be configured as
open drain for this thing to work.

This is necessary when the logic of the code must tell gpiolib how to
electrically use the pin, i.e. it is not a static configuration that comes
from device tree or ACPI or a board file. This is why open
drain/source and debounce can be set up from the in-kernel API.

2) Userspace consumers.

To be clear: these are not laptops, desktops, servers, set-top boxes,
mobile phones etc. Anything comprising a mass-produced system
should NOT use userspace GPIO. That is just WRONG. Real, widely
deployed systems should have proper kernel drivers for their
hardware and deploy their systems with pin config and GPIO use
cases set up in device tree or ACPI or whatever. NOT in userspace
scripts.

Userpace consumers are automatic control: oil burners, electric
bikes, door openers, alarms etc using some GPIO lines as, yeah,
essentially as GPIO lines. "General purpose", as opposed to
"dedicated purpose".

These users include the maker community that do some
fiddely-fiddling with GPIOs from userspace, and that is fine.

Future of this ABI/API:

I do not yet know how much pin config we should allow to come
in from this end *ONLY*. What does userspace consumers really
need?

Also there is no reciprocation between userspace ABI and
in-kernel API.

If we for example decide to let bias and drive strength be set up
from userspace, that does not necessarily mean that we must
let all in-kernel drivers do that too. We can allow
.set_config() to be called from the userspace ABI and down to
.set_config() in the pin control driver ONLY without allowing the
same path for in-kernel users.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [RFC PATCH 1/2] pinctrl: add consumer variant for gpio request
@ 2018-01-18  9:46     ` Linus Walleij
  0 siblings, 0 replies; 23+ messages in thread
From: Linus Walleij @ 2018-01-18  9:46 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jan 15, 2018 at 5:22 PM, Ludovic Desroches
<ludovic.desroches@microchip.com> wrote:

> Add a consumer variant to GPIO request relative functions. The goal
> is to fix the bad ownership, which is arbitrary set to
> "range->name:gpio", of a GPIO.

For this patch on its own (apart from the context):

I what you want to achieve is to pass a consumer name from
gpiolib to pinmux portions of pinctrl, then augment the existing
pinctrl_gpio_request() to pass an optional consumer name,
and change all existing in-kernel users to just pass NULL
and then use the range name as fallback if the consumer
name is NULL.

> There is a lack of configuration features for GPIO. For instance,
> we can't set the bias. Some pin controllers manage both device's
> pins and GPIOs. GPIOs can benefit from pin configuration. Usually,
> a pinctrl node is used to mux the pin as a GPIO and to set up its
> configuration.

Pin config takes care of bias, pull up/down, drive strength etc etc.

GPIO hammers lines 0->1, 1->2, and reads them as 0 or 1.
It can group lines into arrays etc but that is what it does.

The two systems are cross-connected using the GPIO ranges.

There are cross-calls for GPIO to ask pin controllers for favors:
extern int pinctrl_gpio_request(unsigned gpio);
extern void pinctrl_gpio_free(unsigned gpio);
extern int pinctrl_gpio_direction_input(unsigned gpio);
extern int pinctrl_gpio_direction_output(unsigned gpio);
extern int pinctrl_gpio_set_config(unsigned gpio, unsigned long config);

Note: when the GPIO hardware is very simple apart from some extra
register or two providing open drain and/or debounce, the gpio driver
can simply implement .set_config() itself so no pin control back-end
is needed. We could require a separate pin config driver but why
complicate things for the sake of abstraction. Nah.

The last API (pinctrl_gpio_set_config()) should only be called
to set up electrical properties under special circumstances.

Those are as follows:

1) When the in-kernel client needs to configure electrical properties,
gpiolib exposes interfaces for this, such as:
int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce);
int gpiod_set_transitory(struct gpio_desc *desc, bool transitory);

Likewise devm_gpiod_get() can pass flags such as
GPIOD_OUT_LOW_OPEN_DRAIN to state that "I want this line,
and I want it to be initialized logical low (deasserted), and it must
be open drain" If the corresponding device tree phandle flag or board file
description or whatever is not also flagging the line as
open drain, gpiolib will protest with a warning print and
enforce open drain. But it is clear that it *should* have been
configured correctly in the device tree.

This API is not all inclusive: notice that we just support open drain,
not open source. (No upfront design: we just deal with what drivers
need, not what they may theoretically need.)

This is partly for historical reasons, but it also makes sense that things
like I2C that can only work electrically with open drain, has a
way to specify that these lines must really be configured as
open drain for this thing to work.

This is necessary when the logic of the code must tell gpiolib how to
electrically use the pin, i.e. it is not a static configuration that comes
from device tree or ACPI or a board file. This is why open
drain/source and debounce can be set up from the in-kernel API.

2) Userspace consumers.

To be clear: these are not laptops, desktops, servers, set-top boxes,
mobile phones etc. Anything comprising a mass-produced system
should NOT use userspace GPIO. That is just WRONG. Real, widely
deployed systems should have proper kernel drivers for their
hardware and deploy their systems with pin config and GPIO use
cases set up in device tree or ACPI or whatever. NOT in userspace
scripts.

Userpace consumers are automatic control: oil burners, electric
bikes, door openers, alarms etc using some GPIO lines as, yeah,
essentially as GPIO lines. "General purpose", as opposed to
"dedicated purpose".

These users include the maker community that do some
fiddely-fiddling with GPIOs from userspace, and that is fine.

Future of this ABI/API:

I do not yet know how much pin config we should allow to come
in from this end *ONLY*. What does userspace consumers really
need?

Also there is no reciprocation between userspace ABI and
in-kernel API.

If we for example decide to let bias and drive strength be set up
from userspace, that does not necessarily mean that we must
let all in-kernel drivers do that too. We can allow
.set_config() to be called from the userspace ABI and down to
.set_config() in the pin control driver ONLY without allowing the
same path for in-kernel users.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2018-01-18  9:46 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-15 16:22 [RFC PATCH 0/2] fixing the gpio ownership Ludovic Desroches
2018-01-15 16:22 ` Ludovic Desroches
2018-01-15 16:22 ` Ludovic Desroches
2018-01-15 16:22 ` [RFC PATCH 1/2] pinctrl: add consumer variant for gpio request Ludovic Desroches
2018-01-15 16:22   ` Ludovic Desroches
2018-01-15 16:22   ` Ludovic Desroches
2018-01-15 20:19   ` Andy Shevchenko
2018-01-15 20:19     ` Andy Shevchenko
2018-01-16  9:01     ` Ludovic Desroches
2018-01-16  9:01       ` Ludovic Desroches
2018-01-16 14:33       ` Andy Shevchenko
2018-01-17 14:54         ` Ludovic Desroches
2018-01-17 14:54           ` Ludovic Desroches
2018-01-17 14:54           ` Ludovic Desroches
2018-01-17 16:07           ` Andy Shevchenko
2018-01-17 16:07             ` Andy Shevchenko
2018-01-18  7:56             ` Ludovic Desroches
2018-01-18  7:56               ` Ludovic Desroches
2018-01-18  9:46   ` Linus Walleij
2018-01-18  9:46     ` Linus Walleij
2018-01-15 16:22 ` [RFC PATCH 2/2] gpio: provide a consumer when requesting a gpio Ludovic Desroches
2018-01-15 16:22   ` Ludovic Desroches
2018-01-15 16:22   ` Ludovic Desroches

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.