All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH 0/7] gpiolib: add bias support
@ 2017-12-14 14:21 ` Ludovic Desroches
  0 siblings, 0 replies; 26+ messages in thread
From: Ludovic Desroches @ 2017-12-14 14:21 UTC (permalink / raw)
  To: linux-gpio, linux-arm-kernel; +Cc: Ludovic Desroches

Hi,

Here is an attempt to add the bias configuration to the gpiolib since
I want and need to enable the pinmuxing strict mode to get an error if
I request a GPIO which is used by a peripheral.

I have not monitored the gpio mailing list for a while. Sorry I may have
missed some discussions about this topic. In this case, let me know which
threads I should read.

I have a memory of a discussion about the pinmux strict mode and the
constraints related to it. Several pin controllers can mux a pin as a GPIO
or as a peripheral. Doing both is not possible, so the strict mode may or
must be enabled. Unfortunately, to perform the pin configuration of the GPIO,
such as the bias (pull up, pull down, disabled), we have to declare this pin
in the pinmuxing. Doing this leads to a conflict when the device requests the
GPIO. The owner of the pin is the device which requested the pinmuxing whereas
the owner of the GPIO is the pin controller.

To solve this, there are two solutions:
- fix this owner mismatching. The owner of the GPIO must be the requester. It's
not difficult to solve this issue if we add the requester as a parameter when
the GPIO is requested but it impacts a lot of code.
- allow the gpiolib to set the pin configuration. I think the main issue
was code duplication. Since I have seen the introduction of
gpiochip_generic_config(), gpiod_set_debounce(), I suppose it's the way to go.

I have not introduced a gpiod_set_bias() because I think the only place to
setup this is in the device tree. It's useless to setup a bias pull up if there
is an external pull up on the board.

Ludovic Desroches (7):
  gpio: of: use the BIT macro for of_gpio_flags
  gpio: gpiolib: split the gpiod_configure_flags function
  gpio: gpiolib: save GPIO flags in of_get_named_gpiod_flags
  gpio: gpiolib: add bias support
  pinctrl: at91-pio4: allow the gpiolib to set pin configuration
  pinctrl: at91-pio4: use strict mode if explicitly requested
  ARM: dts: remove gpio from pinmux

 .../bindings/pinctrl/atmel,at91-pio4-pinctrl.txt   |  4 ++
 arch/arm/boot/dts/at91-sama5d2_xplained.dts        | 45 ++----------
 drivers/gpio/gpiolib-of.c                          | 11 +++
 drivers/gpio/gpiolib.c                             | 81 ++++++++++++++++------
 drivers/gpio/gpiolib.h                             | 10 ++-
 drivers/pinctrl/pinctrl-at91-pio4.c                | 26 ++++++-
 include/dt-bindings/gpio/gpio.h                    |  9 +++
 include/linux/gpio/machine.h                       |  3 +
 include/linux/of_gpio.h                            | 12 ++--
 9 files changed, 134 insertions(+), 67 deletions(-)

-- 
2.12.2


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

* [RFC PATCH 0/7] gpiolib: add bias support
@ 2017-12-14 14:21 ` Ludovic Desroches
  0 siblings, 0 replies; 26+ messages in thread
From: Ludovic Desroches @ 2017-12-14 14:21 UTC (permalink / raw)
  To: linux-arm-kernel

Hi,

Here is an attempt to add the bias configuration to the gpiolib since
I want and need to enable the pinmuxing strict mode to get an error if
I request a GPIO which is used by a peripheral.

I have not monitored the gpio mailing list for a while. Sorry I may have
missed some discussions about this topic. In this case, let me know which
threads I should read.

I have a memory of a discussion about the pinmux strict mode and the
constraints related to it. Several pin controllers can mux a pin as a GPIO
or as a peripheral. Doing both is not possible, so the strict mode may or
must be enabled. Unfortunately, to perform the pin configuration of the GPIO,
such as the bias (pull up, pull down, disabled), we have to declare this pin
in the pinmuxing. Doing this leads to a conflict when the device requests the
GPIO. The owner of the pin is the device which requested the pinmuxing whereas
the owner of the GPIO is the pin controller.

To solve this, there are two solutions:
- fix this owner mismatching. The owner of the GPIO must be the requester. It's
not difficult to solve this issue if we add the requester as a parameter when
the GPIO is requested but it impacts a lot of code.
- allow the gpiolib to set the pin configuration. I think the main issue
was code duplication. Since I have seen the introduction of
gpiochip_generic_config(), gpiod_set_debounce(), I suppose it's the way to go.

I have not introduced a gpiod_set_bias() because I think the only place to
setup this is in the device tree. It's useless to setup a bias pull up if there
is an external pull up on the board.

Ludovic Desroches (7):
  gpio: of: use the BIT macro for of_gpio_flags
  gpio: gpiolib: split the gpiod_configure_flags function
  gpio: gpiolib: save GPIO flags in of_get_named_gpiod_flags
  gpio: gpiolib: add bias support
  pinctrl: at91-pio4: allow the gpiolib to set pin configuration
  pinctrl: at91-pio4: use strict mode if explicitly requested
  ARM: dts: remove gpio from pinmux

 .../bindings/pinctrl/atmel,at91-pio4-pinctrl.txt   |  4 ++
 arch/arm/boot/dts/at91-sama5d2_xplained.dts        | 45 ++----------
 drivers/gpio/gpiolib-of.c                          | 11 +++
 drivers/gpio/gpiolib.c                             | 81 ++++++++++++++++------
 drivers/gpio/gpiolib.h                             | 10 ++-
 drivers/pinctrl/pinctrl-at91-pio4.c                | 26 ++++++-
 include/dt-bindings/gpio/gpio.h                    |  9 +++
 include/linux/gpio/machine.h                       |  3 +
 include/linux/of_gpio.h                            | 12 ++--
 9 files changed, 134 insertions(+), 67 deletions(-)

-- 
2.12.2

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

* [RFC PATCH 1/7] gpio: of: use the BIT macro for of_gpio_flags
  2017-12-14 14:21 ` Ludovic Desroches
@ 2017-12-14 14:21   ` Ludovic Desroches
  -1 siblings, 0 replies; 26+ messages in thread
From: Ludovic Desroches @ 2017-12-14 14:21 UTC (permalink / raw)
  To: linux-gpio, linux-arm-kernel; +Cc: Ludovic Desroches

Use the BIT macro for the of_gpio_flags enumeration.

Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com>
---
 include/linux/of_gpio.h | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
index 18a7f03e1182..f928c2df2bcd 100644
--- a/include/linux/of_gpio.h
+++ b/include/linux/of_gpio.h
@@ -14,6 +14,7 @@
 #ifndef __LINUX_OF_GPIO_H
 #define __LINUX_OF_GPIO_H
 
+#include <linux/bitops.h>
 #include <linux/compiler.h>
 #include <linux/kernel.h>
 #include <linux/errno.h>
@@ -28,10 +29,10 @@ struct device_node;
  * Linux-specific in their .xlate callback. Though, 1:1 mapping is recommended.
  */
 enum of_gpio_flags {
-	OF_GPIO_ACTIVE_LOW = 0x1,
-	OF_GPIO_SINGLE_ENDED = 0x2,
-	OF_GPIO_OPEN_DRAIN = 0x4,
-	OF_GPIO_TRANSITORY = 0x8,
+	OF_GPIO_ACTIVE_LOW	= BIT(0),
+	OF_GPIO_SINGLE_ENDED	= BIT(1),
+	OF_GPIO_OPEN_DRAIN	= BIT(2),
+	OF_GPIO_TRANSITORY	= BIT(3),
 };
 
 #ifdef CONFIG_OF_GPIO
-- 
2.12.2


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

* [RFC PATCH 1/7] gpio: of: use the BIT macro for of_gpio_flags
@ 2017-12-14 14:21   ` Ludovic Desroches
  0 siblings, 0 replies; 26+ messages in thread
From: Ludovic Desroches @ 2017-12-14 14:21 UTC (permalink / raw)
  To: linux-arm-kernel

Use the BIT macro for the of_gpio_flags enumeration.

Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com>
---
 include/linux/of_gpio.h | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
index 18a7f03e1182..f928c2df2bcd 100644
--- a/include/linux/of_gpio.h
+++ b/include/linux/of_gpio.h
@@ -14,6 +14,7 @@
 #ifndef __LINUX_OF_GPIO_H
 #define __LINUX_OF_GPIO_H
 
+#include <linux/bitops.h>
 #include <linux/compiler.h>
 #include <linux/kernel.h>
 #include <linux/errno.h>
@@ -28,10 +29,10 @@ struct device_node;
  * Linux-specific in their .xlate callback. Though, 1:1 mapping is recommended.
  */
 enum of_gpio_flags {
-	OF_GPIO_ACTIVE_LOW = 0x1,
-	OF_GPIO_SINGLE_ENDED = 0x2,
-	OF_GPIO_OPEN_DRAIN = 0x4,
-	OF_GPIO_TRANSITORY = 0x8,
+	OF_GPIO_ACTIVE_LOW	= BIT(0),
+	OF_GPIO_SINGLE_ENDED	= BIT(1),
+	OF_GPIO_OPEN_DRAIN	= BIT(2),
+	OF_GPIO_TRANSITORY	= BIT(3),
 };
 
 #ifdef CONFIG_OF_GPIO
-- 
2.12.2

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

* [RFC PATCH 2/7] gpio: gpiolib: split the gpiod_configure_flags function
  2017-12-14 14:21 ` Ludovic Desroches
@ 2017-12-14 14:21   ` Ludovic Desroches
  -1 siblings, 0 replies; 26+ messages in thread
From: Ludovic Desroches @ 2017-12-14 14:21 UTC (permalink / raw)
  To: linux-gpio, linux-arm-kernel; +Cc: Ludovic Desroches

The gpiod_configure_flags function doesn't only configure flags, it
also performs some processing. It implies that it should be called
after having requested the GPIO. Split configuration and processing
to allow flags configuration before requesting the GPIO. It is
needed if we want to set the pin configuration.

Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com>
---
 drivers/gpio/gpiolib.c | 49 +++++++++++++++++++++++++++++++------------------
 drivers/gpio/gpiolib.h |  7 ++++++-
 2 files changed, 37 insertions(+), 19 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 0621baddfddc..c887602ca0ff 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -3564,23 +3564,9 @@ struct gpio_desc *__must_check gpiod_get_optional(struct device *dev,
 EXPORT_SYMBOL_GPL(gpiod_get_optional);
 
 
-/**
- * gpiod_configure_flags - helper function to configure a given GPIO
- * @desc:	gpio whose value will be assigned
- * @con_id:	function within the GPIO consumer
- * @lflags:	gpio_lookup_flags - returned from of_find_gpio() or
- *		of_get_gpio_hog()
- * @dflags:	gpiod_flags - optional GPIO initialization flags
- *
- * Return 0 on success, -ENOENT if no GPIO has been assigned to the
- * requested function and/or index, or another IS_ERR() code if an error
- * occurred while trying to acquire the GPIO.
- */
-int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
+void gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
 		unsigned long lflags, enum gpiod_flags dflags)
 {
-	int status;
-
 	if (lflags & GPIO_ACTIVE_LOW)
 		set_bit(FLAG_ACTIVE_LOW, &desc->flags);
 
@@ -3601,6 +3587,11 @@ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
 	if (lflags & GPIO_OPEN_SOURCE)
 		set_bit(FLAG_OPEN_SOURCE, &desc->flags);
 
+int gpiod_process_flags(struct gpio_desc *desc, const char *con_id,
+		unsigned long lflags, enum gpiod_flags dflags)
+{
+	int status;
+
 	status = gpiod_set_transitory(desc, (lflags & GPIO_TRANSITORY));
 	if (status < 0)
 		return status;
@@ -3622,6 +3613,28 @@ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
 }
 
 /**
+ * gpiod_configure_and_process_flags - helper function to configure a
+ *				       given GPIO
+ * @desc:	gpio whose value will be assigned
+ * @con_id:	function within the GPIO consumer
+ * @lflags:	gpio_lookup_flags - returned from of_find_gpio() or
+ *		of_get_gpio_hog()
+ * @dflags:	gpiod_flags - optional GPIO initialization flags
+ *
+ * Return 0 on success, -ENOENT if no GPIO has been assigned to the
+ * requested function and/or index, or another IS_ERR() code if an error
+ * occurred while trying to acquire the GPIO.
+ */
+int gpiod_configure_and_process_flags(struct gpio_desc *desc,
+				      const char *con_id,
+				      unsigned long lflags,
+				      enum gpiod_flags dflags)
+{
+	gpiod_configure_flags(desc, con_id, lflags, dflags);
+	return gpiod_process_flags(desc, con_id, lflags, dflags);
+}
+
+/**
  * gpiod_get_index - obtain a GPIO from a multi-index GPIO function
  * @dev:	GPIO consumer, can be NULL for system-global GPIOs
  * @con_id:	function within the GPIO consumer
@@ -3675,7 +3688,7 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
 	if (status < 0)
 		return ERR_PTR(status);
 
-	status = gpiod_configure_flags(desc, con_id, lookupflags, flags);
+	status = gpiod_configure_and_process_flags(desc, con_id, lookupflags, flags);
 	if (status < 0) {
 		dev_dbg(dev, "setup of GPIO %s failed\n", con_id);
 		gpiod_put(desc);
@@ -3764,7 +3777,7 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
 	if (transitory)
 		lflags |= GPIO_TRANSITORY;
 
-	ret = gpiod_configure_flags(desc, propname, lflags, dflags);
+	ret = gpiod_configure_and_process_flags(desc, propname, lflags, dflags);
 	if (ret < 0) {
 		gpiod_put(desc);
 		return ERR_PTR(ret);
@@ -3830,7 +3843,7 @@ int gpiod_hog(struct gpio_desc *desc, const char *name,
 		return status;
 	}
 
-	status = gpiod_configure_flags(desc, name, lflags, dflags);
+	status = gpiod_configure_and_process_flags(desc, name, lflags, dflags);
 	if (status < 0) {
 		pr_err("setup of hog GPIO %s (chip %s, offset %d) failed, %d\n",
 		       name, chip->label, hwnum, status);
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 5e1f7cc6eeb6..a03553d4be1c 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -219,8 +219,13 @@ struct gpio_desc {
 
 int gpiod_request(struct gpio_desc *desc, const char *label);
 void gpiod_free(struct gpio_desc *desc);
-int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
+void gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
 		unsigned long lflags, enum gpiod_flags dflags);
+int gpiod_process_flags(struct gpio_desc *desc, const char *con_id,
+		unsigned long lflags, enum gpiod_flags dflags);
+int gpiod_configure_and_process_flags(struct gpio_desc *desc,
+		const char *con_id, unsigned long lflags,
+		enum gpiod_flags dflags);
 int gpiod_hog(struct gpio_desc *desc, const char *name,
 		unsigned long lflags, enum gpiod_flags dflags);
 
-- 
2.12.2


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

* [RFC PATCH 2/7] gpio: gpiolib: split the gpiod_configure_flags function
@ 2017-12-14 14:21   ` Ludovic Desroches
  0 siblings, 0 replies; 26+ messages in thread
From: Ludovic Desroches @ 2017-12-14 14:21 UTC (permalink / raw)
  To: linux-arm-kernel

The gpiod_configure_flags function doesn't only configure flags, it
also performs some processing. It implies that it should be called
after having requested the GPIO. Split configuration and processing
to allow flags configuration before requesting the GPIO. It is
needed if we want to set the pin configuration.

Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com>
---
 drivers/gpio/gpiolib.c | 49 +++++++++++++++++++++++++++++++------------------
 drivers/gpio/gpiolib.h |  7 ++++++-
 2 files changed, 37 insertions(+), 19 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 0621baddfddc..c887602ca0ff 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -3564,23 +3564,9 @@ struct gpio_desc *__must_check gpiod_get_optional(struct device *dev,
 EXPORT_SYMBOL_GPL(gpiod_get_optional);
 
 
-/**
- * gpiod_configure_flags - helper function to configure a given GPIO
- * @desc:	gpio whose value will be assigned
- * @con_id:	function within the GPIO consumer
- * @lflags:	gpio_lookup_flags - returned from of_find_gpio() or
- *		of_get_gpio_hog()
- * @dflags:	gpiod_flags - optional GPIO initialization flags
- *
- * Return 0 on success, -ENOENT if no GPIO has been assigned to the
- * requested function and/or index, or another IS_ERR() code if an error
- * occurred while trying to acquire the GPIO.
- */
-int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
+void gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
 		unsigned long lflags, enum gpiod_flags dflags)
 {
-	int status;
-
 	if (lflags & GPIO_ACTIVE_LOW)
 		set_bit(FLAG_ACTIVE_LOW, &desc->flags);
 
@@ -3601,6 +3587,11 @@ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
 	if (lflags & GPIO_OPEN_SOURCE)
 		set_bit(FLAG_OPEN_SOURCE, &desc->flags);
 
+int gpiod_process_flags(struct gpio_desc *desc, const char *con_id,
+		unsigned long lflags, enum gpiod_flags dflags)
+{
+	int status;
+
 	status = gpiod_set_transitory(desc, (lflags & GPIO_TRANSITORY));
 	if (status < 0)
 		return status;
@@ -3622,6 +3613,28 @@ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
 }
 
 /**
+ * gpiod_configure_and_process_flags - helper function to configure a
+ *				       given GPIO
+ * @desc:	gpio whose value will be assigned
+ * @con_id:	function within the GPIO consumer
+ * @lflags:	gpio_lookup_flags - returned from of_find_gpio() or
+ *		of_get_gpio_hog()
+ * @dflags:	gpiod_flags - optional GPIO initialization flags
+ *
+ * Return 0 on success, -ENOENT if no GPIO has been assigned to the
+ * requested function and/or index, or another IS_ERR() code if an error
+ * occurred while trying to acquire the GPIO.
+ */
+int gpiod_configure_and_process_flags(struct gpio_desc *desc,
+				      const char *con_id,
+				      unsigned long lflags,
+				      enum gpiod_flags dflags)
+{
+	gpiod_configure_flags(desc, con_id, lflags, dflags);
+	return gpiod_process_flags(desc, con_id, lflags, dflags);
+}
+
+/**
  * gpiod_get_index - obtain a GPIO from a multi-index GPIO function
  * @dev:	GPIO consumer, can be NULL for system-global GPIOs
  * @con_id:	function within the GPIO consumer
@@ -3675,7 +3688,7 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
 	if (status < 0)
 		return ERR_PTR(status);
 
-	status = gpiod_configure_flags(desc, con_id, lookupflags, flags);
+	status = gpiod_configure_and_process_flags(desc, con_id, lookupflags, flags);
 	if (status < 0) {
 		dev_dbg(dev, "setup of GPIO %s failed\n", con_id);
 		gpiod_put(desc);
@@ -3764,7 +3777,7 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
 	if (transitory)
 		lflags |= GPIO_TRANSITORY;
 
-	ret = gpiod_configure_flags(desc, propname, lflags, dflags);
+	ret = gpiod_configure_and_process_flags(desc, propname, lflags, dflags);
 	if (ret < 0) {
 		gpiod_put(desc);
 		return ERR_PTR(ret);
@@ -3830,7 +3843,7 @@ int gpiod_hog(struct gpio_desc *desc, const char *name,
 		return status;
 	}
 
-	status = gpiod_configure_flags(desc, name, lflags, dflags);
+	status = gpiod_configure_and_process_flags(desc, name, lflags, dflags);
 	if (status < 0) {
 		pr_err("setup of hog GPIO %s (chip %s, offset %d) failed, %d\n",
 		       name, chip->label, hwnum, status);
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 5e1f7cc6eeb6..a03553d4be1c 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -219,8 +219,13 @@ struct gpio_desc {
 
 int gpiod_request(struct gpio_desc *desc, const char *label);
 void gpiod_free(struct gpio_desc *desc);
-int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
+void gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
 		unsigned long lflags, enum gpiod_flags dflags);
+int gpiod_process_flags(struct gpio_desc *desc, const char *con_id,
+		unsigned long lflags, enum gpiod_flags dflags);
+int gpiod_configure_and_process_flags(struct gpio_desc *desc,
+		const char *con_id, unsigned long lflags,
+		enum gpiod_flags dflags);
 int gpiod_hog(struct gpio_desc *desc, const char *name,
 		unsigned long lflags, enum gpiod_flags dflags);
 
-- 
2.12.2

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

* [RFC PATCH 3/7] gpio: gpiolib: save GPIO flags in of_get_named_gpiod_flags
  2017-12-14 14:21 ` Ludovic Desroches
@ 2017-12-14 14:21   ` Ludovic Desroches
  -1 siblings, 0 replies; 26+ messages in thread
From: Ludovic Desroches @ 2017-12-14 14:21 UTC (permalink / raw)
  To: linux-gpio, linux-arm-kernel; +Cc: Ludovic Desroches

When we get a GPIO descriptor from the device device, the flags
are updated for the caller to know if the GPIO is active low or
high. After calling of_get_named_gpio_flags the next step is
usually calling gpiod_request which doesn't take flags as a
parameter.
Updating the flags of the GPIO descriptor will allow to configure
the GPIO when it will be requested.

Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com>
---
 drivers/gpio/gpiolib-of.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 4a2b8d3397c7..67b1a7ff1e97 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -97,6 +97,8 @@ struct gpio_desc *of_get_named_gpiod_flags(struct device_node *np,
 		 __func__, propname, np, index,
 		 PTR_ERR_OR_ZERO(desc));
 
+	gpiod_configure_flags(desc, propname, *flags, 0);
+
 out:
 	of_node_put(gpiospec.np);
 
-- 
2.12.2


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

* [RFC PATCH 3/7] gpio: gpiolib: save GPIO flags in of_get_named_gpiod_flags
@ 2017-12-14 14:21   ` Ludovic Desroches
  0 siblings, 0 replies; 26+ messages in thread
From: Ludovic Desroches @ 2017-12-14 14:21 UTC (permalink / raw)
  To: linux-arm-kernel

When we get a GPIO descriptor from the device device, the flags
are updated for the caller to know if the GPIO is active low or
high. After calling of_get_named_gpio_flags the next step is
usually calling gpiod_request which doesn't take flags as a
parameter.
Updating the flags of the GPIO descriptor will allow to configure
the GPIO when it will be requested.

Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com>
---
 drivers/gpio/gpiolib-of.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 4a2b8d3397c7..67b1a7ff1e97 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -97,6 +97,8 @@ struct gpio_desc *of_get_named_gpiod_flags(struct device_node *np,
 		 __func__, propname, np, index,
 		 PTR_ERR_OR_ZERO(desc));
 
+	gpiod_configure_flags(desc, propname, *flags, 0);
+
 out:
 	of_node_put(gpiospec.np);
 
-- 
2.12.2

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

* [RFC PATCH 4/7] gpio: gpiolib: add bias support
  2017-12-14 14:21 ` Ludovic Desroches
@ 2017-12-14 14:21   ` Ludovic Desroches
  -1 siblings, 0 replies; 26+ messages in thread
From: Ludovic Desroches @ 2017-12-14 14:21 UTC (permalink / raw)
  To: linux-gpio, linux-arm-kernel; +Cc: Ludovic Desroches

If we want to setup the bias of a pin used as a GPIO, we have,
at least for some pin controllers, to declare it in the pinmuxing
in order to apply the configuration wanted.

If the pinmuxing strict mode is enabled, there is a conflict when
the device driver requests the GPIO. It is due to an ownership
mismatch. The device driver has requested the pin through the
pinctrl so the owner is the device. When the GPIO is requested,
there is a conflict because the owner of the GPIO is the pin
controller not the requester.

This patch provides a way to add the bias configuration to the
flags of a GPIO.

Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com>
---
 drivers/gpio/gpiolib-of.c       |  9 +++++++++
 drivers/gpio/gpiolib.c          | 34 ++++++++++++++++++++++++++++------
 drivers/gpio/gpiolib.h          |  3 +++
 include/dt-bindings/gpio/gpio.h |  9 +++++++++
 include/linux/gpio/machine.h    |  3 +++
 include/linux/of_gpio.h         |  3 +++
 6 files changed, 55 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 67b1a7ff1e97..9c6da8c2e97c 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -158,6 +158,15 @@ struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
 	if (of_flags & OF_GPIO_TRANSITORY)
 		*flags |= GPIO_TRANSITORY;
 
+	if (of_flags & OF_GPIO_BIAS_PULL_UP)
+		*flags |= GPIO_BIAS_PULL_UP;
+
+	if (of_flags & OF_GPIO_BIAS_PULL_DOWN)
+		*flags |= GPIO_BIAS_PULL_DOWN;
+
+	if (of_flags & OF_GPIO_BIAS_DISABLE)
+		*flags |= GPIO_BIAS_DISABLE;
+
 	return desc;
 }
 
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index c887602ca0ff..2caec626dcd5 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2133,7 +2133,7 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
 {
 	struct gpio_chip	*chip = desc->gdev->chip;
 	int			status;
-	unsigned long		flags;
+	unsigned long		flags, config = 0;
 
 	spin_lock_irqsave(&gpio_lock, flags);
 
@@ -2161,6 +2161,16 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
 			goto done;
 		}
 	}
+	if (chip->set_config) {
+		/* The following flags are exclusive. */
+		if (test_bit(FLAG_BIAS_PULL_UP,  &desc->flags))
+			config |= pinconf_to_config_packed(PIN_CONFIG_BIAS_PULL_UP, 1);
+		else if (test_bit(FLAG_BIAS_PULL_DOWN,  &desc->flags))
+			config |= pinconf_to_config_packed(PIN_CONFIG_BIAS_PULL_DOWN, 1);
+		else if (test_bit(FLAG_BIAS_DISABLE,  &desc->flags))
+			config |= pinconf_to_config_packed(PIN_CONFIG_BIAS_DISABLE, 1);
+		chip->set_config(chip, gpio_chip_hwgpio(desc), config);
+	}
 	if (chip->get_direction) {
 		/* chip->get_direction may sleep */
 		spin_unlock_irqrestore(&gpio_lock, flags);
@@ -3587,6 +3597,16 @@ void gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
 	if (lflags & GPIO_OPEN_SOURCE)
 		set_bit(FLAG_OPEN_SOURCE, &desc->flags);
 
+	if (lflags & GPIO_BIAS_PULL_UP)
+		set_bit(FLAG_BIAS_PULL_UP, &desc->flags);
+
+	if (lflags & GPIO_BIAS_PULL_DOWN)
+		set_bit(FLAG_BIAS_PULL_DOWN, &desc->flags);
+
+	if (lflags & GPIO_BIAS_DISABLE)
+		set_bit(FLAG_BIAS_DISABLE, &desc->flags);
+}
+
 int gpiod_process_flags(struct gpio_desc *desc, const char *con_id,
 		unsigned long lflags, enum gpiod_flags dflags)
 {
@@ -3760,10 +3780,6 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
 	if (IS_ERR(desc))
 		return desc;
 
-	ret = gpiod_request(desc, label);
-	if (ret)
-		return ERR_PTR(ret);
-
 	if (active_low)
 		lflags |= GPIO_ACTIVE_LOW;
 
@@ -3777,7 +3793,13 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
 	if (transitory)
 		lflags |= GPIO_TRANSITORY;
 
-	ret = gpiod_configure_and_process_flags(desc, propname, lflags, dflags);
+	gpiod_configure_flags(desc, propname, lflags, dflags);
+
+	ret = gpiod_request(desc, label);
+	if (ret)
+		return ERR_PTR(ret);
+
+	ret = gpiod_process_flags(desc, propname, lflags, dflags);
 	if (ret < 0) {
 		gpiod_put(desc);
 		return ERR_PTR(ret);
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index a03553d4be1c..381a7a1bf540 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -210,6 +210,9 @@ struct gpio_desc {
 #define FLAG_USED_AS_IRQ 9	/* GPIO is connected to an IRQ */
 #define FLAG_IS_HOGGED	11	/* GPIO is hogged */
 #define FLAG_TRANSITORY 12	/* GPIO may lose value in sleep or reset */
+#define FLAG_BIAS_PULL_UP 13
+#define FLAG_BIAS_PULL_DOWN 14
+#define FLAG_BIAS_DISABLE 15
 
 	/* Connection label */
 	const char		*label;
diff --git a/include/dt-bindings/gpio/gpio.h b/include/dt-bindings/gpio/gpio.h
index 2cc10ae4bbb7..28ccfce72c59 100644
--- a/include/dt-bindings/gpio/gpio.h
+++ b/include/dt-bindings/gpio/gpio.h
@@ -33,4 +33,13 @@
 #define GPIO_PERSISTENT 0
 #define GPIO_TRANSITORY 8
 
+/* Bit 4 express Bias Pull up */
+#define GPIO_BIAS_PULL_UP 16
+
+/* Bit 5 express Bias Pull down */
+#define GPIO_BIAS_PULL_DOWN 32
+
+/* Bit 6 express Bias Disable */
+#define GPIO_BIAS_DISABLE 64
+
 #endif
diff --git a/include/linux/gpio/machine.h b/include/linux/gpio/machine.h
index b2f2dc638463..e0b6ac64871f 100644
--- a/include/linux/gpio/machine.h
+++ b/include/linux/gpio/machine.h
@@ -12,6 +12,9 @@ enum gpio_lookup_flags {
 	GPIO_OPEN_SOURCE = (1 << 2),
 	GPIO_PERSISTENT = (0 << 3),
 	GPIO_TRANSITORY = (1 << 3),
+	GPIO_BIAS_PULL_UP = (1 << 4),
+	GPIO_BIAS_PULL_DOWN = (1 << 5),
+	GPIO_BIAS_DISABLE = (1 << 6),
 };
 
 /**
diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
index f928c2df2bcd..1b025e6680ea 100644
--- a/include/linux/of_gpio.h
+++ b/include/linux/of_gpio.h
@@ -33,6 +33,9 @@ enum of_gpio_flags {
 	OF_GPIO_SINGLE_ENDED	= BIT(1),
 	OF_GPIO_OPEN_DRAIN	= BIT(2),
 	OF_GPIO_TRANSITORY	= BIT(3),
+	OF_GPIO_BIAS_PULL_UP	= BIT(4),
+	OF_GPIO_BIAS_PULL_DOWN	= BIT(5),
+	OF_GPIO_BIAS_DISABLE	= BIT(6),
 };
 
 #ifdef CONFIG_OF_GPIO
-- 
2.12.2


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

* [RFC PATCH 4/7] gpio: gpiolib: add bias support
@ 2017-12-14 14:21   ` Ludovic Desroches
  0 siblings, 0 replies; 26+ messages in thread
From: Ludovic Desroches @ 2017-12-14 14:21 UTC (permalink / raw)
  To: linux-arm-kernel

If we want to setup the bias of a pin used as a GPIO, we have,
at least for some pin controllers, to declare it in the pinmuxing
in order to apply the configuration wanted.

If the pinmuxing strict mode is enabled, there is a conflict when
the device driver requests the GPIO. It is due to an ownership
mismatch. The device driver has requested the pin through the
pinctrl so the owner is the device. When the GPIO is requested,
there is a conflict because the owner of the GPIO is the pin
controller not the requester.

This patch provides a way to add the bias configuration to the
flags of a GPIO.

Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com>
---
 drivers/gpio/gpiolib-of.c       |  9 +++++++++
 drivers/gpio/gpiolib.c          | 34 ++++++++++++++++++++++++++++------
 drivers/gpio/gpiolib.h          |  3 +++
 include/dt-bindings/gpio/gpio.h |  9 +++++++++
 include/linux/gpio/machine.h    |  3 +++
 include/linux/of_gpio.h         |  3 +++
 6 files changed, 55 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 67b1a7ff1e97..9c6da8c2e97c 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -158,6 +158,15 @@ struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
 	if (of_flags & OF_GPIO_TRANSITORY)
 		*flags |= GPIO_TRANSITORY;
 
+	if (of_flags & OF_GPIO_BIAS_PULL_UP)
+		*flags |= GPIO_BIAS_PULL_UP;
+
+	if (of_flags & OF_GPIO_BIAS_PULL_DOWN)
+		*flags |= GPIO_BIAS_PULL_DOWN;
+
+	if (of_flags & OF_GPIO_BIAS_DISABLE)
+		*flags |= GPIO_BIAS_DISABLE;
+
 	return desc;
 }
 
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index c887602ca0ff..2caec626dcd5 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2133,7 +2133,7 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
 {
 	struct gpio_chip	*chip = desc->gdev->chip;
 	int			status;
-	unsigned long		flags;
+	unsigned long		flags, config = 0;
 
 	spin_lock_irqsave(&gpio_lock, flags);
 
@@ -2161,6 +2161,16 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
 			goto done;
 		}
 	}
+	if (chip->set_config) {
+		/* The following flags are exclusive. */
+		if (test_bit(FLAG_BIAS_PULL_UP,  &desc->flags))
+			config |= pinconf_to_config_packed(PIN_CONFIG_BIAS_PULL_UP, 1);
+		else if (test_bit(FLAG_BIAS_PULL_DOWN,  &desc->flags))
+			config |= pinconf_to_config_packed(PIN_CONFIG_BIAS_PULL_DOWN, 1);
+		else if (test_bit(FLAG_BIAS_DISABLE,  &desc->flags))
+			config |= pinconf_to_config_packed(PIN_CONFIG_BIAS_DISABLE, 1);
+		chip->set_config(chip, gpio_chip_hwgpio(desc), config);
+	}
 	if (chip->get_direction) {
 		/* chip->get_direction may sleep */
 		spin_unlock_irqrestore(&gpio_lock, flags);
@@ -3587,6 +3597,16 @@ void gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
 	if (lflags & GPIO_OPEN_SOURCE)
 		set_bit(FLAG_OPEN_SOURCE, &desc->flags);
 
+	if (lflags & GPIO_BIAS_PULL_UP)
+		set_bit(FLAG_BIAS_PULL_UP, &desc->flags);
+
+	if (lflags & GPIO_BIAS_PULL_DOWN)
+		set_bit(FLAG_BIAS_PULL_DOWN, &desc->flags);
+
+	if (lflags & GPIO_BIAS_DISABLE)
+		set_bit(FLAG_BIAS_DISABLE, &desc->flags);
+}
+
 int gpiod_process_flags(struct gpio_desc *desc, const char *con_id,
 		unsigned long lflags, enum gpiod_flags dflags)
 {
@@ -3760,10 +3780,6 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
 	if (IS_ERR(desc))
 		return desc;
 
-	ret = gpiod_request(desc, label);
-	if (ret)
-		return ERR_PTR(ret);
-
 	if (active_low)
 		lflags |= GPIO_ACTIVE_LOW;
 
@@ -3777,7 +3793,13 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
 	if (transitory)
 		lflags |= GPIO_TRANSITORY;
 
-	ret = gpiod_configure_and_process_flags(desc, propname, lflags, dflags);
+	gpiod_configure_flags(desc, propname, lflags, dflags);
+
+	ret = gpiod_request(desc, label);
+	if (ret)
+		return ERR_PTR(ret);
+
+	ret = gpiod_process_flags(desc, propname, lflags, dflags);
 	if (ret < 0) {
 		gpiod_put(desc);
 		return ERR_PTR(ret);
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index a03553d4be1c..381a7a1bf540 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -210,6 +210,9 @@ struct gpio_desc {
 #define FLAG_USED_AS_IRQ 9	/* GPIO is connected to an IRQ */
 #define FLAG_IS_HOGGED	11	/* GPIO is hogged */
 #define FLAG_TRANSITORY 12	/* GPIO may lose value in sleep or reset */
+#define FLAG_BIAS_PULL_UP 13
+#define FLAG_BIAS_PULL_DOWN 14
+#define FLAG_BIAS_DISABLE 15
 
 	/* Connection label */
 	const char		*label;
diff --git a/include/dt-bindings/gpio/gpio.h b/include/dt-bindings/gpio/gpio.h
index 2cc10ae4bbb7..28ccfce72c59 100644
--- a/include/dt-bindings/gpio/gpio.h
+++ b/include/dt-bindings/gpio/gpio.h
@@ -33,4 +33,13 @@
 #define GPIO_PERSISTENT 0
 #define GPIO_TRANSITORY 8
 
+/* Bit 4 express Bias Pull up */
+#define GPIO_BIAS_PULL_UP 16
+
+/* Bit 5 express Bias Pull down */
+#define GPIO_BIAS_PULL_DOWN 32
+
+/* Bit 6 express Bias Disable */
+#define GPIO_BIAS_DISABLE 64
+
 #endif
diff --git a/include/linux/gpio/machine.h b/include/linux/gpio/machine.h
index b2f2dc638463..e0b6ac64871f 100644
--- a/include/linux/gpio/machine.h
+++ b/include/linux/gpio/machine.h
@@ -12,6 +12,9 @@ enum gpio_lookup_flags {
 	GPIO_OPEN_SOURCE = (1 << 2),
 	GPIO_PERSISTENT = (0 << 3),
 	GPIO_TRANSITORY = (1 << 3),
+	GPIO_BIAS_PULL_UP = (1 << 4),
+	GPIO_BIAS_PULL_DOWN = (1 << 5),
+	GPIO_BIAS_DISABLE = (1 << 6),
 };
 
 /**
diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
index f928c2df2bcd..1b025e6680ea 100644
--- a/include/linux/of_gpio.h
+++ b/include/linux/of_gpio.h
@@ -33,6 +33,9 @@ enum of_gpio_flags {
 	OF_GPIO_SINGLE_ENDED	= BIT(1),
 	OF_GPIO_OPEN_DRAIN	= BIT(2),
 	OF_GPIO_TRANSITORY	= BIT(3),
+	OF_GPIO_BIAS_PULL_UP	= BIT(4),
+	OF_GPIO_BIAS_PULL_DOWN	= BIT(5),
+	OF_GPIO_BIAS_DISABLE	= BIT(6),
 };
 
 #ifdef CONFIG_OF_GPIO
-- 
2.12.2

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

* [RFC PATCH 5/7] pinctrl: at91-pio4: allow the gpiolib to set pin configuration
  2017-12-14 14:21 ` Ludovic Desroches
@ 2017-12-14 14:21   ` Ludovic Desroches
  -1 siblings, 0 replies; 26+ messages in thread
From: Ludovic Desroches @ 2017-12-14 14:21 UTC (permalink / raw)
  To: linux-gpio, linux-arm-kernel; +Cc: Ludovic Desroches

Use gpiochip_generic_config to allow the gpiolib to set pin
configuration. Since it relies on .pin_config_set(), add it too.
For this controller, one pin is on group so we can use
atmel_conf_pin_config_group_set() function.

Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com>
---
 drivers/pinctrl/pinctrl-at91-pio4.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-at91-pio4.c b/drivers/pinctrl/pinctrl-at91-pio4.c
index b1ca838dd80a..4b8dda770af8 100644
--- a/drivers/pinctrl/pinctrl-at91-pio4.c
+++ b/drivers/pinctrl/pinctrl-at91-pio4.c
@@ -364,6 +364,7 @@ static struct gpio_chip atmel_gpio_chip = {
 	.set                    = atmel_gpio_set,
 	.to_irq                 = atmel_gpio_to_irq,
 	.base                   = 0,
+	.set_config		= gpiochip_generic_config,
 };
 
 /* --- PINCTRL --- */
@@ -817,6 +818,7 @@ static void atmel_conf_pin_config_dbg_show(struct pinctrl_dev *pctldev,
 }
 
 static const struct pinconf_ops atmel_confops = {
+	.pin_config_set		= atmel_conf_pin_config_group_set, /* In our case, a pin = a group */
 	.pin_config_group_get	= atmel_conf_pin_config_group_get,
 	.pin_config_group_set	= atmel_conf_pin_config_group_set,
 	.pin_config_dbg_show	= atmel_conf_pin_config_dbg_show,
-- 
2.12.2


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

* [RFC PATCH 5/7] pinctrl: at91-pio4: allow the gpiolib to set pin configuration
@ 2017-12-14 14:21   ` Ludovic Desroches
  0 siblings, 0 replies; 26+ messages in thread
From: Ludovic Desroches @ 2017-12-14 14:21 UTC (permalink / raw)
  To: linux-arm-kernel

Use gpiochip_generic_config to allow the gpiolib to set pin
configuration. Since it relies on .pin_config_set(), add it too.
For this controller, one pin is on group so we can use
atmel_conf_pin_config_group_set() function.

Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com>
---
 drivers/pinctrl/pinctrl-at91-pio4.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-at91-pio4.c b/drivers/pinctrl/pinctrl-at91-pio4.c
index b1ca838dd80a..4b8dda770af8 100644
--- a/drivers/pinctrl/pinctrl-at91-pio4.c
+++ b/drivers/pinctrl/pinctrl-at91-pio4.c
@@ -364,6 +364,7 @@ static struct gpio_chip atmel_gpio_chip = {
 	.set                    = atmel_gpio_set,
 	.to_irq                 = atmel_gpio_to_irq,
 	.base                   = 0,
+	.set_config		= gpiochip_generic_config,
 };
 
 /* --- PINCTRL --- */
@@ -817,6 +818,7 @@ static void atmel_conf_pin_config_dbg_show(struct pinctrl_dev *pctldev,
 }
 
 static const struct pinconf_ops atmel_confops = {
+	.pin_config_set		= atmel_conf_pin_config_group_set, /* In our case, a pin = a group */
 	.pin_config_group_get	= atmel_conf_pin_config_group_get,
 	.pin_config_group_set	= atmel_conf_pin_config_group_set,
 	.pin_config_dbg_show	= atmel_conf_pin_config_dbg_show,
-- 
2.12.2

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

* [RFC PATCH 6/7] pinctrl: at91-pio4: use strict mode if explicitly requested
  2017-12-14 14:21 ` Ludovic Desroches
@ 2017-12-14 14:21   ` Ludovic Desroches
  -1 siblings, 0 replies; 26+ messages in thread
From: Ludovic Desroches @ 2017-12-14 14:21 UTC (permalink / raw)
  To: linux-gpio, linux-arm-kernel; +Cc: Ludovic Desroches

Add a property to use pinmux strict mode. It corresponds to the
legacy behavior of the controller. It was not used because there
were no solution to configure (open drain, bias, etc) a pin
requested as a GPIO.

If the strict mode is used by default, it will break several boards.
The owner of a GPIO requested by a device is not the device itself
but the pinctrl. So there is an ownership mismatch since the owner
of the pinmuxing is the device which requested it. It will lead to
an error when requesting the GPIO.

By adding a DT property, we can enable it only for DTs which are
written correctly.

The gpio_request_enable operation is needed to mux the pin as a
GPIO but it has to be used only if strict mode is enabled. If not,
a pin muxed to a device may be muxed to a GPIO silently.

Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com>
---
 .../bindings/pinctrl/atmel,at91-pio4-pinctrl.txt   |  4 ++++
 drivers/pinctrl/pinctrl-at91-pio4.c                | 24 +++++++++++++++++++++-
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pio4-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pio4-pinctrl.txt
index 61ac75706cc9..440cb5687b4e 100644
--- a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pio4-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pio4-pinctrl.txt
@@ -11,6 +11,10 @@ Required properties:
 - #interrupt-cells: should be two.
 - gpio-controller: mark the device node as a gpio controller.
 - #gpio-cells: should be two.
+Optional properties:
+- atmel,use-strict-mode: enable the pinmux strict mode which prevents
+simultaneous use of the same pin for GPIO and another function. It implies to
+not put a pin requested as a GPIO in the pinmux property.
 
 Please refer to ../gpio/gpio.txt and ../interrupt-controller/interrupts.txt for
 a general description of GPIO and interrupt bindings.
diff --git a/drivers/pinctrl/pinctrl-at91-pio4.c b/drivers/pinctrl/pinctrl-at91-pio4.c
index 4b8dda770af8..6acbbcc9b4a6 100644
--- a/drivers/pinctrl/pinctrl-at91-pio4.c
+++ b/drivers/pinctrl/pinctrl-at91-pio4.c
@@ -358,6 +358,8 @@ static int atmel_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
 }
 
 static struct gpio_chip atmel_gpio_chip = {
+	.request		= gpiochip_generic_request,
+	.free			= gpiochip_generic_free,
 	.direction_input        = atmel_gpio_direction_input,
 	.get                    = atmel_gpio_get,
 	.direction_output       = atmel_gpio_direction_output,
@@ -644,7 +646,22 @@ static int atmel_pmx_set_mux(struct pinctrl_dev *pctldev,
 	return 0;
 }
 
-static const struct pinmux_ops atmel_pmxops = {
+static int atmel_pmx_gpio_request_enable(struct pinctrl_dev *pctldev,
+					 struct pinctrl_gpio_range *range,
+					 unsigned offset)
+{
+	u32 conf;
+
+	conf = atmel_pin_config_read(pctldev, offset);
+	conf &= (~ATMEL_PIO_CFGR_FUNC_MASK);
+	atmel_pin_config_write(pctldev, offset, conf);
+
+	dev_dbg(pctldev->dev, "enable pin %u as GPIO\n", offset);
+
+	return 0;
+}
+
+static struct pinmux_ops atmel_pmxops = {
 	.get_functions_count	= atmel_pmx_get_functions_count,
 	.get_function_name	= atmel_pmx_get_function_name,
 	.get_function_groups	= atmel_pmx_get_function_groups,
@@ -930,6 +947,11 @@ static int atmel_pinctrl_probe(struct platform_device *pdev)
 	atmel_pioctrl->nbanks = atmel_pioctrl_data->nbanks;
 	atmel_pioctrl->npins = atmel_pioctrl->nbanks * ATMEL_PIO_NPINS_PER_BANK;
 
+	if (of_property_read_bool(dev->of_node, "atmel,use-strict-mode")) {
+		atmel_pmxops.strict = true;
+		atmel_pmxops.gpio_request_enable = atmel_pmx_gpio_request_enable;
+	}
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {
 		dev_err(dev, "unable to get atmel pinctrl resource\n");
-- 
2.12.2


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

* [RFC PATCH 6/7] pinctrl: at91-pio4: use strict mode if explicitly requested
@ 2017-12-14 14:21   ` Ludovic Desroches
  0 siblings, 0 replies; 26+ messages in thread
From: Ludovic Desroches @ 2017-12-14 14:21 UTC (permalink / raw)
  To: linux-arm-kernel

Add a property to use pinmux strict mode. It corresponds to the
legacy behavior of the controller. It was not used because there
were no solution to configure (open drain, bias, etc) a pin
requested as a GPIO.

If the strict mode is used by default, it will break several boards.
The owner of a GPIO requested by a device is not the device itself
but the pinctrl. So there is an ownership mismatch since the owner
of the pinmuxing is the device which requested it. It will lead to
an error when requesting the GPIO.

By adding a DT property, we can enable it only for DTs which are
written correctly.

The gpio_request_enable operation is needed to mux the pin as a
GPIO but it has to be used only if strict mode is enabled. If not,
a pin muxed to a device may be muxed to a GPIO silently.

Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com>
---
 .../bindings/pinctrl/atmel,at91-pio4-pinctrl.txt   |  4 ++++
 drivers/pinctrl/pinctrl-at91-pio4.c                | 24 +++++++++++++++++++++-
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pio4-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pio4-pinctrl.txt
index 61ac75706cc9..440cb5687b4e 100644
--- a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pio4-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pio4-pinctrl.txt
@@ -11,6 +11,10 @@ Required properties:
 - #interrupt-cells: should be two.
 - gpio-controller: mark the device node as a gpio controller.
 - #gpio-cells: should be two.
+Optional properties:
+- atmel,use-strict-mode: enable the pinmux strict mode which prevents
+simultaneous use of the same pin for GPIO and another function. It implies to
+not put a pin requested as a GPIO in the pinmux property.
 
 Please refer to ../gpio/gpio.txt and ../interrupt-controller/interrupts.txt for
 a general description of GPIO and interrupt bindings.
diff --git a/drivers/pinctrl/pinctrl-at91-pio4.c b/drivers/pinctrl/pinctrl-at91-pio4.c
index 4b8dda770af8..6acbbcc9b4a6 100644
--- a/drivers/pinctrl/pinctrl-at91-pio4.c
+++ b/drivers/pinctrl/pinctrl-at91-pio4.c
@@ -358,6 +358,8 @@ static int atmel_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
 }
 
 static struct gpio_chip atmel_gpio_chip = {
+	.request		= gpiochip_generic_request,
+	.free			= gpiochip_generic_free,
 	.direction_input        = atmel_gpio_direction_input,
 	.get                    = atmel_gpio_get,
 	.direction_output       = atmel_gpio_direction_output,
@@ -644,7 +646,22 @@ static int atmel_pmx_set_mux(struct pinctrl_dev *pctldev,
 	return 0;
 }
 
-static const struct pinmux_ops atmel_pmxops = {
+static int atmel_pmx_gpio_request_enable(struct pinctrl_dev *pctldev,
+					 struct pinctrl_gpio_range *range,
+					 unsigned offset)
+{
+	u32 conf;
+
+	conf = atmel_pin_config_read(pctldev, offset);
+	conf &= (~ATMEL_PIO_CFGR_FUNC_MASK);
+	atmel_pin_config_write(pctldev, offset, conf);
+
+	dev_dbg(pctldev->dev, "enable pin %u as GPIO\n", offset);
+
+	return 0;
+}
+
+static struct pinmux_ops atmel_pmxops = {
 	.get_functions_count	= atmel_pmx_get_functions_count,
 	.get_function_name	= atmel_pmx_get_function_name,
 	.get_function_groups	= atmel_pmx_get_function_groups,
@@ -930,6 +947,11 @@ static int atmel_pinctrl_probe(struct platform_device *pdev)
 	atmel_pioctrl->nbanks = atmel_pioctrl_data->nbanks;
 	atmel_pioctrl->npins = atmel_pioctrl->nbanks * ATMEL_PIO_NPINS_PER_BANK;
 
+	if (of_property_read_bool(dev->of_node, "atmel,use-strict-mode")) {
+		atmel_pmxops.strict = true;
+		atmel_pmxops.gpio_request_enable = atmel_pmx_gpio_request_enable;
+	}
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {
 		dev_err(dev, "unable to get atmel pinctrl resource\n");
-- 
2.12.2

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

* [RFC PATCH 7/7] ARM: dts: at91-sama5d2_xplained: remove gpios from pinmux
  2017-12-14 14:21 ` Ludovic Desroches
@ 2017-12-14 14:21   ` Ludovic Desroches
  -1 siblings, 0 replies; 26+ messages in thread
From: Ludovic Desroches @ 2017-12-14 14:21 UTC (permalink / raw)
  To: linux-gpio, linux-arm-kernel; +Cc: Ludovic Desroches

As we can setup the pin configuration of a GPIO through the gpiolib,
enable the pinmuxing strict mode and remove GPIOs from the pinmuxing.

Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com>
---
 arch/arm/boot/dts/at91-sama5d2_xplained.dts | 45 +++++------------------------
 1 file changed, 7 insertions(+), 38 deletions(-)

diff --git a/arch/arm/boot/dts/at91-sama5d2_xplained.dts b/arch/arm/boot/dts/at91-sama5d2_xplained.dts
index 56de21de2779..9e0bf162e6bd 100644
--- a/arch/arm/boot/dts/at91-sama5d2_xplained.dts
+++ b/arch/arm/boot/dts/at91-sama5d2_xplained.dts
@@ -68,20 +68,16 @@
 
 	ahb {
 		usb0: gadget@300000 {
-			atmel,vbus-gpio = <&pioA PIN_PA31 GPIO_ACTIVE_HIGH>;
-			pinctrl-names = "default";
-			pinctrl-0 = <&pinctrl_usba_vbus>;
+			atmel,vbus-gpio = <&pioA PIN_PA31 (GPIO_ACTIVE_LOW | GPIO_BIAS_PULL_UP)>;
 			status = "okay";
 		};
 
 		usb1: ohci@400000 {
 			num-ports = <3>;
 			atmel,vbus-gpio = <0 /* &pioA PIN_PB9 GPIO_ACTIVE_HIGH */
-					   &pioA PIN_PB10 GPIO_ACTIVE_HIGH
+					   &pioA PIN_PB10 (GPIO_ACTIVE_LOW | GPIO_BIAS_PULL_UP)
 					   0
 					  >;
-			pinctrl-names = "default";
-			pinctrl-0 = <&pinctrl_usb_default>;
 			status = "okay";
 		};
 
@@ -325,6 +321,7 @@
 			};
 
 			pinctrl@fc038000 {
+				atmel,use-strict-mode;
 				/*
 				 * There is no real pinmux for ADC, if the pin
 				 * is not requested by another peripheral then
@@ -412,18 +409,6 @@
 					bias-disable;
 				};
 
-				pinctrl_key_gpio_default: key_gpio_default {
-					pinmux = <PIN_PB9__GPIO>;
-					bias-pull-up;
-				};
-
-				pinctrl_led_gpio_default: led_gpio_default {
-					pinmux = <PIN_PB0__GPIO>,
-						 <PIN_PB5__GPIO>,
-						 <PIN_PB6__GPIO>;
-					bias-pull-up;
-				};
-
 				pinctrl_macb0_default: macb0_default {
 					pinmux = <PIN_PB14__GTXCK>,
 						 <PIN_PB15__GTXEN>,
@@ -509,16 +494,6 @@
 					bias-disable;
 				};
 
-				pinctrl_usb_default: usb_default {
-					pinmux = <PIN_PB10__GPIO>;
-					bias-disable;
-				};
-
-				pinctrl_usba_vbus: usba_vbus {
-					pinmux = <PIN_PA31__GPIO>;
-					bias-disable;
-				};
-
 				pinctrl_pwm0_pwm2_default: pwm0_pwm2_default {
 					pinmux = <PIN_PB5__PWMH2>,
 						 <PIN_PB6__PWML2>;
@@ -544,13 +519,9 @@
 
 	gpio_keys {
 		compatible = "gpio-keys";
-
-		pinctrl-names = "default";
-		pinctrl-0 = <&pinctrl_key_gpio_default>;
-
 		bp1 {
 			label = "PB_USER";
-			gpios = <&pioA PIN_PB9 GPIO_ACTIVE_LOW>;
+			gpios = <&pioA PIN_PB9 (GPIO_ACTIVE_LOW | GPIO_BIAS_PULL_UP)>;
 			linux,code = <0x104>;
 			wakeup-source;
 		};
@@ -558,24 +529,22 @@
 
 	leds {
 		compatible = "gpio-leds";
-		pinctrl-names = "default";
-		pinctrl-0 = <&pinctrl_led_gpio_default>;
 		status = "okay"; /* conflict with pwm0 */
 
 		red {
 			label = "red";
-			gpios = <&pioA PIN_PB6 GPIO_ACTIVE_LOW>;
+			gpios = <&pioA PIN_PB6 (GPIO_ACTIVE_LOW | GPIO_BIAS_PULL_UP)>;
 		};
 
 
 		green {
 			label = "green";
-			gpios = <&pioA PIN_PB5 GPIO_ACTIVE_LOW>;
+			gpios = <&pioA PIN_PB5 (GPIO_ACTIVE_LOW | GPIO_BIAS_PULL_UP)>;
 		};
 
 		blue {
 			label = "blue";
-			gpios = <&pioA PIN_PB0 GPIO_ACTIVE_LOW>;
+			gpios = <&pioA PIN_PB0 (GPIO_ACTIVE_LOW | GPIO_BIAS_PULL_UP)>;
 			linux,default-trigger = "heartbeat";
 		};
 	};
-- 
2.12.2


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

* [RFC PATCH 7/7] ARM: dts: at91-sama5d2_xplained: remove gpios from pinmux
@ 2017-12-14 14:21   ` Ludovic Desroches
  0 siblings, 0 replies; 26+ messages in thread
From: Ludovic Desroches @ 2017-12-14 14:21 UTC (permalink / raw)
  To: linux-arm-kernel

As we can setup the pin configuration of a GPIO through the gpiolib,
enable the pinmuxing strict mode and remove GPIOs from the pinmuxing.

Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com>
---
 arch/arm/boot/dts/at91-sama5d2_xplained.dts | 45 +++++------------------------
 1 file changed, 7 insertions(+), 38 deletions(-)

diff --git a/arch/arm/boot/dts/at91-sama5d2_xplained.dts b/arch/arm/boot/dts/at91-sama5d2_xplained.dts
index 56de21de2779..9e0bf162e6bd 100644
--- a/arch/arm/boot/dts/at91-sama5d2_xplained.dts
+++ b/arch/arm/boot/dts/at91-sama5d2_xplained.dts
@@ -68,20 +68,16 @@
 
 	ahb {
 		usb0: gadget at 300000 {
-			atmel,vbus-gpio = <&pioA PIN_PA31 GPIO_ACTIVE_HIGH>;
-			pinctrl-names = "default";
-			pinctrl-0 = <&pinctrl_usba_vbus>;
+			atmel,vbus-gpio = <&pioA PIN_PA31 (GPIO_ACTIVE_LOW | GPIO_BIAS_PULL_UP)>;
 			status = "okay";
 		};
 
 		usb1: ohci at 400000 {
 			num-ports = <3>;
 			atmel,vbus-gpio = <0 /* &pioA PIN_PB9 GPIO_ACTIVE_HIGH */
-					   &pioA PIN_PB10 GPIO_ACTIVE_HIGH
+					   &pioA PIN_PB10 (GPIO_ACTIVE_LOW | GPIO_BIAS_PULL_UP)
 					   0
 					  >;
-			pinctrl-names = "default";
-			pinctrl-0 = <&pinctrl_usb_default>;
 			status = "okay";
 		};
 
@@ -325,6 +321,7 @@
 			};
 
 			pinctrl at fc038000 {
+				atmel,use-strict-mode;
 				/*
 				 * There is no real pinmux for ADC, if the pin
 				 * is not requested by another peripheral then
@@ -412,18 +409,6 @@
 					bias-disable;
 				};
 
-				pinctrl_key_gpio_default: key_gpio_default {
-					pinmux = <PIN_PB9__GPIO>;
-					bias-pull-up;
-				};
-
-				pinctrl_led_gpio_default: led_gpio_default {
-					pinmux = <PIN_PB0__GPIO>,
-						 <PIN_PB5__GPIO>,
-						 <PIN_PB6__GPIO>;
-					bias-pull-up;
-				};
-
 				pinctrl_macb0_default: macb0_default {
 					pinmux = <PIN_PB14__GTXCK>,
 						 <PIN_PB15__GTXEN>,
@@ -509,16 +494,6 @@
 					bias-disable;
 				};
 
-				pinctrl_usb_default: usb_default {
-					pinmux = <PIN_PB10__GPIO>;
-					bias-disable;
-				};
-
-				pinctrl_usba_vbus: usba_vbus {
-					pinmux = <PIN_PA31__GPIO>;
-					bias-disable;
-				};
-
 				pinctrl_pwm0_pwm2_default: pwm0_pwm2_default {
 					pinmux = <PIN_PB5__PWMH2>,
 						 <PIN_PB6__PWML2>;
@@ -544,13 +519,9 @@
 
 	gpio_keys {
 		compatible = "gpio-keys";
-
-		pinctrl-names = "default";
-		pinctrl-0 = <&pinctrl_key_gpio_default>;
-
 		bp1 {
 			label = "PB_USER";
-			gpios = <&pioA PIN_PB9 GPIO_ACTIVE_LOW>;
+			gpios = <&pioA PIN_PB9 (GPIO_ACTIVE_LOW | GPIO_BIAS_PULL_UP)>;
 			linux,code = <0x104>;
 			wakeup-source;
 		};
@@ -558,24 +529,22 @@
 
 	leds {
 		compatible = "gpio-leds";
-		pinctrl-names = "default";
-		pinctrl-0 = <&pinctrl_led_gpio_default>;
 		status = "okay"; /* conflict with pwm0 */
 
 		red {
 			label = "red";
-			gpios = <&pioA PIN_PB6 GPIO_ACTIVE_LOW>;
+			gpios = <&pioA PIN_PB6 (GPIO_ACTIVE_LOW | GPIO_BIAS_PULL_UP)>;
 		};
 
 
 		green {
 			label = "green";
-			gpios = <&pioA PIN_PB5 GPIO_ACTIVE_LOW>;
+			gpios = <&pioA PIN_PB5 (GPIO_ACTIVE_LOW | GPIO_BIAS_PULL_UP)>;
 		};
 
 		blue {
 			label = "blue";
-			gpios = <&pioA PIN_PB0 GPIO_ACTIVE_LOW>;
+			gpios = <&pioA PIN_PB0 (GPIO_ACTIVE_LOW | GPIO_BIAS_PULL_UP)>;
 			linux,default-trigger = "heartbeat";
 		};
 	};
-- 
2.12.2

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

* Re: [RFC PATCH 0/7] gpiolib: add bias support
  2017-12-14 14:21 ` Ludovic Desroches
@ 2017-12-14 16:06   ` Andrew Lunn
  -1 siblings, 0 replies; 26+ messages in thread
From: Andrew Lunn @ 2017-12-14 16:06 UTC (permalink / raw)
  To: Ludovic Desroches; +Cc: linux-gpio, linux-arm-kernel

> I have not introduced a gpiod_set_bias() because I think the only place to
> setup this is in the device tree. It's useless to setup a bias pull up if there
> is an external pull up on the board.

Hi Ludovic

I have a use case where having gpiod_set_bias() would be good. The
DLN2 is a USB device, which offers GPIO, I2C, and SPI services on its
pins. It is a hot pluggable device, and i'm using it on Intel
machines, making it double hard to use with Device Tree.

It would be good to be able to set the pull up/down on the GPIO pins,
probably from a small kernel module which calls gpiod_set_bias(), or
via /sysfs.

    Andrew

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

* [RFC PATCH 0/7] gpiolib: add bias support
@ 2017-12-14 16:06   ` Andrew Lunn
  0 siblings, 0 replies; 26+ messages in thread
From: Andrew Lunn @ 2017-12-14 16:06 UTC (permalink / raw)
  To: linux-arm-kernel

> I have not introduced a gpiod_set_bias() because I think the only place to
> setup this is in the device tree. It's useless to setup a bias pull up if there
> is an external pull up on the board.

Hi Ludovic

I have a use case where having gpiod_set_bias() would be good. The
DLN2 is a USB device, which offers GPIO, I2C, and SPI services on its
pins. It is a hot pluggable device, and i'm using it on Intel
machines, making it double hard to use with Device Tree.

It would be good to be able to set the pull up/down on the GPIO pins,
probably from a small kernel module which calls gpiod_set_bias(), or
via /sysfs.

    Andrew

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

* Re: [RFC PATCH 0/7] gpiolib: add bias support
  2017-12-14 16:06   ` Andrew Lunn
@ 2017-12-15  6:54     ` Ludovic Desroches
  -1 siblings, 0 replies; 26+ messages in thread
From: Ludovic Desroches @ 2017-12-15  6:54 UTC (permalink / raw)
  To: Andrew Lunn; +Cc: Ludovic Desroches, linux-gpio, linux-arm-kernel

Hi Andrew,

On Thu, Dec 14, 2017 at 05:06:25PM +0100, Andrew Lunn wrote:
> > I have not introduced a gpiod_set_bias() because I think the only place to
> > setup this is in the device tree. It's useless to setup a bias pull up if there
> > is an external pull up on the board.
> 
> Hi Ludovic
> 
> I have a use case where having gpiod_set_bias() would be good. The
> DLN2 is a USB device, which offers GPIO, I2C, and SPI services on its
> pins. It is a hot pluggable device, and i'm using it on Intel
> machines, making it double hard to use with Device Tree.
> 
> It would be good to be able to set the pull up/down on the GPIO pins,
> probably from a small kernel module which calls gpiod_set_bias(), or
> via /sysfs.

Thanks for your feedback. I'll try to keep your need in mind for next
version or rework.

Ludovic

> 
>     Andrew
> --
> 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] 26+ messages in thread

* [RFC PATCH 0/7] gpiolib: add bias support
@ 2017-12-15  6:54     ` Ludovic Desroches
  0 siblings, 0 replies; 26+ messages in thread
From: Ludovic Desroches @ 2017-12-15  6:54 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Andrew,

On Thu, Dec 14, 2017 at 05:06:25PM +0100, Andrew Lunn wrote:
> > I have not introduced a gpiod_set_bias() because I think the only place to
> > setup this is in the device tree. It's useless to setup a bias pull up if there
> > is an external pull up on the board.
> 
> Hi Ludovic
> 
> I have a use case where having gpiod_set_bias() would be good. The
> DLN2 is a USB device, which offers GPIO, I2C, and SPI services on its
> pins. It is a hot pluggable device, and i'm using it on Intel
> machines, making it double hard to use with Device Tree.
> 
> It would be good to be able to set the pull up/down on the GPIO pins,
> probably from a small kernel module which calls gpiod_set_bias(), or
> via /sysfs.

Thanks for your feedback. I'll try to keep your need in mind for next
version or rework.

Ludovic

> 
>     Andrew
> --
> 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] 26+ messages in thread

* Re: [RFC PATCH 2/7] gpio: gpiolib: split the gpiod_configure_flags function
  2017-12-14 14:21   ` Ludovic Desroches
@ 2017-12-15  9:26     ` Julien Thierry
  -1 siblings, 0 replies; 26+ messages in thread
From: Julien Thierry @ 2017-12-15  9:26 UTC (permalink / raw)
  To: Ludovic Desroches, linux-gpio, linux-arm-kernel

Hi Ludovic,

On 14/12/17 14:21, Ludovic Desroches wrote:
> The gpiod_configure_flags function doesn't only configure flags, it
> also performs some processing. It implies that it should be called
> after having requested the GPIO. Split configuration and processing
> to allow flags configuration before requesting the GPIO. It is
> needed if we want to set the pin configuration.
> 
> Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com>
> ---
>   drivers/gpio/gpiolib.c | 49 +++++++++++++++++++++++++++++++------------------
>   drivers/gpio/gpiolib.h |  7 ++++++-
>   2 files changed, 37 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 0621baddfddc..c887602ca0ff 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -3564,23 +3564,9 @@ struct gpio_desc *__must_check gpiod_get_optional(struct device *dev,
>   EXPORT_SYMBOL_GPL(gpiod_get_optional);
>   
>   
> -/**
> - * gpiod_configure_flags - helper function to configure a given GPIO
> - * @desc:	gpio whose value will be assigned
> - * @con_id:	function within the GPIO consumer
> - * @lflags:	gpio_lookup_flags - returned from of_find_gpio() or
> - *		of_get_gpio_hog()
> - * @dflags:	gpiod_flags - optional GPIO initialization flags
> - *
> - * Return 0 on success, -ENOENT if no GPIO has been assigned to the
> - * requested function and/or index, or another IS_ERR() code if an error
> - * occurred while trying to acquire the GPIO.
> - */
> -int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
> +void gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
>   		unsigned long lflags, enum gpiod_flags dflags)
>   {
> -	int status;
> -
>   	if (lflags & GPIO_ACTIVE_LOW)
>   		set_bit(FLAG_ACTIVE_LOW, &desc->flags);
>   
> @@ -3601,6 +3587,11 @@ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
>   	if (lflags & GPIO_OPEN_SOURCE)
>   		set_bit(FLAG_OPEN_SOURCE, &desc->flags);
>   

Small issue, I believe the patch is missing a '}' here to properly split 
the function.

Otherwise:

Reviewed-by: Julien Thierry <julien.thierry@arm.com>

Cheers,

-- 
Julien Thierry

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

* [RFC PATCH 2/7] gpio: gpiolib: split the gpiod_configure_flags function
@ 2017-12-15  9:26     ` Julien Thierry
  0 siblings, 0 replies; 26+ messages in thread
From: Julien Thierry @ 2017-12-15  9:26 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Ludovic,

On 14/12/17 14:21, Ludovic Desroches wrote:
> The gpiod_configure_flags function doesn't only configure flags, it
> also performs some processing. It implies that it should be called
> after having requested the GPIO. Split configuration and processing
> to allow flags configuration before requesting the GPIO. It is
> needed if we want to set the pin configuration.
> 
> Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com>
> ---
>   drivers/gpio/gpiolib.c | 49 +++++++++++++++++++++++++++++++------------------
>   drivers/gpio/gpiolib.h |  7 ++++++-
>   2 files changed, 37 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 0621baddfddc..c887602ca0ff 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -3564,23 +3564,9 @@ struct gpio_desc *__must_check gpiod_get_optional(struct device *dev,
>   EXPORT_SYMBOL_GPL(gpiod_get_optional);
>   
>   
> -/**
> - * gpiod_configure_flags - helper function to configure a given GPIO
> - * @desc:	gpio whose value will be assigned
> - * @con_id:	function within the GPIO consumer
> - * @lflags:	gpio_lookup_flags - returned from of_find_gpio() or
> - *		of_get_gpio_hog()
> - * @dflags:	gpiod_flags - optional GPIO initialization flags
> - *
> - * Return 0 on success, -ENOENT if no GPIO has been assigned to the
> - * requested function and/or index, or another IS_ERR() code if an error
> - * occurred while trying to acquire the GPIO.
> - */
> -int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
> +void gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
>   		unsigned long lflags, enum gpiod_flags dflags)
>   {
> -	int status;
> -
>   	if (lflags & GPIO_ACTIVE_LOW)
>   		set_bit(FLAG_ACTIVE_LOW, &desc->flags);
>   
> @@ -3601,6 +3587,11 @@ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
>   	if (lflags & GPIO_OPEN_SOURCE)
>   		set_bit(FLAG_OPEN_SOURCE, &desc->flags);
>   

Small issue, I believe the patch is missing a '}' here to properly split 
the function.

Otherwise:

Reviewed-by: Julien Thierry <julien.thierry@arm.com>

Cheers,

-- 
Julien Thierry

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

* Re: [RFC PATCH 2/7] gpio: gpiolib: split the gpiod_configure_flags function
  2017-12-15  9:26     ` Julien Thierry
@ 2017-12-18  7:02       ` Ludovic Desroches
  -1 siblings, 0 replies; 26+ messages in thread
From: Ludovic Desroches @ 2017-12-18  7:02 UTC (permalink / raw)
  To: Julien Thierry; +Cc: Ludovic Desroches, linux-gpio, linux-arm-kernel

On Fri, Dec 15, 2017 at 09:26:27AM +0000, Julien Thierry wrote:
> Hi Ludovic,
> 
> On 14/12/17 14:21, Ludovic Desroches wrote:
> > The gpiod_configure_flags function doesn't only configure flags, it
> > also performs some processing. It implies that it should be called
> > after having requested the GPIO. Split configuration and processing
> > to allow flags configuration before requesting the GPIO. It is
> > needed if we want to set the pin configuration.
> > 
> > Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com>
> > ---
> >   drivers/gpio/gpiolib.c | 49 +++++++++++++++++++++++++++++++------------------
> >   drivers/gpio/gpiolib.h |  7 ++++++-
> >   2 files changed, 37 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index 0621baddfddc..c887602ca0ff 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -3564,23 +3564,9 @@ struct gpio_desc *__must_check gpiod_get_optional(struct device *dev,
> >   EXPORT_SYMBOL_GPL(gpiod_get_optional);
> > -/**
> > - * gpiod_configure_flags - helper function to configure a given GPIO
> > - * @desc:	gpio whose value will be assigned
> > - * @con_id:	function within the GPIO consumer
> > - * @lflags:	gpio_lookup_flags - returned from of_find_gpio() or
> > - *		of_get_gpio_hog()
> > - * @dflags:	gpiod_flags - optional GPIO initialization flags
> > - *
> > - * Return 0 on success, -ENOENT if no GPIO has been assigned to the
> > - * requested function and/or index, or another IS_ERR() code if an error
> > - * occurred while trying to acquire the GPIO.
> > - */
> > -int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
> > +void gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
> >   		unsigned long lflags, enum gpiod_flags dflags)
> >   {
> > -	int status;
> > -
> >   	if (lflags & GPIO_ACTIVE_LOW)
> >   		set_bit(FLAG_ACTIVE_LOW, &desc->flags);
> > @@ -3601,6 +3587,11 @@ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
> >   	if (lflags & GPIO_OPEN_SOURCE)
> >   		set_bit(FLAG_OPEN_SOURCE, &desc->flags);
> 
> Small issue, I believe the patch is missing a '}' here to properly split the
> function.

Thanks, I'll fix it, this ligne was unstaged.

Regards

Ludovic

> 
> Otherwise:
> 
> Reviewed-by: Julien Thierry <julien.thierry@arm.com>
> 
> Cheers,
> 
> -- 
> Julien Thierry

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

* [RFC PATCH 2/7] gpio: gpiolib: split the gpiod_configure_flags function
@ 2017-12-18  7:02       ` Ludovic Desroches
  0 siblings, 0 replies; 26+ messages in thread
From: Ludovic Desroches @ 2017-12-18  7:02 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Dec 15, 2017 at 09:26:27AM +0000, Julien Thierry wrote:
> Hi Ludovic,
> 
> On 14/12/17 14:21, Ludovic Desroches wrote:
> > The gpiod_configure_flags function doesn't only configure flags, it
> > also performs some processing. It implies that it should be called
> > after having requested the GPIO. Split configuration and processing
> > to allow flags configuration before requesting the GPIO. It is
> > needed if we want to set the pin configuration.
> > 
> > Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com>
> > ---
> >   drivers/gpio/gpiolib.c | 49 +++++++++++++++++++++++++++++++------------------
> >   drivers/gpio/gpiolib.h |  7 ++++++-
> >   2 files changed, 37 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index 0621baddfddc..c887602ca0ff 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -3564,23 +3564,9 @@ struct gpio_desc *__must_check gpiod_get_optional(struct device *dev,
> >   EXPORT_SYMBOL_GPL(gpiod_get_optional);
> > -/**
> > - * gpiod_configure_flags - helper function to configure a given GPIO
> > - * @desc:	gpio whose value will be assigned
> > - * @con_id:	function within the GPIO consumer
> > - * @lflags:	gpio_lookup_flags - returned from of_find_gpio() or
> > - *		of_get_gpio_hog()
> > - * @dflags:	gpiod_flags - optional GPIO initialization flags
> > - *
> > - * Return 0 on success, -ENOENT if no GPIO has been assigned to the
> > - * requested function and/or index, or another IS_ERR() code if an error
> > - * occurred while trying to acquire the GPIO.
> > - */
> > -int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
> > +void gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
> >   		unsigned long lflags, enum gpiod_flags dflags)
> >   {
> > -	int status;
> > -
> >   	if (lflags & GPIO_ACTIVE_LOW)
> >   		set_bit(FLAG_ACTIVE_LOW, &desc->flags);
> > @@ -3601,6 +3587,11 @@ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
> >   	if (lflags & GPIO_OPEN_SOURCE)
> >   		set_bit(FLAG_OPEN_SOURCE, &desc->flags);
> 
> Small issue, I believe the patch is missing a '}' here to properly split the
> function.

Thanks, I'll fix it, this ligne was unstaged.

Regards

Ludovic

> 
> Otherwise:
> 
> Reviewed-by: Julien Thierry <julien.thierry@arm.com>
> 
> Cheers,
> 
> -- 
> Julien Thierry

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

* [RFC PATCH v2] draft for gpio pinconf
  2017-12-14 14:21 ` Ludovic Desroches
@ 2017-12-19  9:40   ` Ludovic Desroches
  -1 siblings, 0 replies; 26+ messages in thread
From: Ludovic Desroches @ 2017-12-19  9:40 UTC (permalink / raw)
  To: linux-gpio, linux-arm-kernel; +Cc: Ludovic Desroches

Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com>
---

Hi,

After discussing with Maxime Ripard, I realize that the first approach
may not be a good one.

For instance, the drive-strength is a digital value so it will take
ake several bits. Having a whole pin configuration in 32 bits may not
be the right solution.

My aim is to use the existing pinconf bindings.

This patch is a draft to test the feasibility. Sorry to provide it in
this state (several ugly hacks) but I didn't want to spend time on a clean
solution if it will be rejected.

 arch/arm/boot/dts/at91-sama5d2_xplained.dts |  6 +-----
 arch/arm/boot/dts/sama5d2.dtsi              |  2 +-
 drivers/gpio/gpiolib-of.c                   | 10 +++++++++-
 drivers/gpio/gpiolib.c                      | 24 +++++++++++++++++++-----
 drivers/gpio/gpiolib.h                      |  2 ++
 drivers/pinctrl/core.c                      |  5 ++---
 drivers/pinctrl/pinctrl-at91-pio4.c         | 19 +++++++++++++++++++
 include/linux/gpio/driver.h                 |  5 +++--
 include/linux/pinctrl/consumer.h            |  4 ++--
 9 files changed, 58 insertions(+), 19 deletions(-)

diff --git a/arch/arm/boot/dts/at91-sama5d2_xplained.dts b/arch/arm/boot/dts/at91-sama5d2_xplained.dts
index 56de21de2779..709c9c3ae622 100644
--- a/arch/arm/boot/dts/at91-sama5d2_xplained.dts
+++ b/arch/arm/boot/dts/at91-sama5d2_xplained.dts
@@ -413,7 +413,6 @@
 				};
 
 				pinctrl_key_gpio_default: key_gpio_default {
-					pinmux = <PIN_PB9__GPIO>;
 					bias-pull-up;
 				};
 
@@ -545,12 +544,9 @@
 	gpio_keys {
 		compatible = "gpio-keys";
 
-		pinctrl-names = "default";
-		pinctrl-0 = <&pinctrl_key_gpio_default>;
-
 		bp1 {
 			label = "PB_USER";
-			gpios = <&pioA PIN_PB9 GPIO_ACTIVE_LOW>;
+			gpios = <&pioA PIN_PB9 GPIO_ACTIVE_LOW &pinctrl_key_gpio_default>;
 			linux,code = <0x104>;
 			wakeup-source;
 		};
diff --git a/arch/arm/boot/dts/sama5d2.dtsi b/arch/arm/boot/dts/sama5d2.dtsi
index b44e63995583..cccd404192cc 100644
--- a/arch/arm/boot/dts/sama5d2.dtsi
+++ b/arch/arm/boot/dts/sama5d2.dtsi
@@ -1444,7 +1444,7 @@
 				interrupt-controller;
 				#interrupt-cells = <2>;
 				gpio-controller;
-				#gpio-cells = <2>;
+				#gpio-cells = <3>;
 				clocks = <&pioA_clk>;
 			};
 
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 4a2b8d3397c7..605017a4d2d8 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -46,7 +46,7 @@ static struct gpio_desc *of_xlate_and_get_gpiod_flags(struct gpio_chip *chip,
 {
 	int ret;
 
-	if (chip->of_gpio_n_cells != gpiospec->args_count)
+	if (chip->of_gpio_n_cells > gpiospec->args_count)
 		return ERR_PTR(-EINVAL);
 
 	ret = chip->of_xlate(chip, gpiospec, flags);
@@ -93,6 +93,14 @@ struct gpio_desc *of_get_named_gpiod_flags(struct device_node *np,
 	if (IS_ERR(desc))
 		goto out;
 
+	if (gpiospec.args_count > chip->of_gpio_n_cells) {
+		desc->np_pincfg = of_parse_phandle(np, propname, gpiospec.args_count);
+		if (!desc->np_pincfg) {
+			printk("ERROR\n");
+			goto out;
+		}
+	}
+
 	pr_debug("%s: parsed '%s' property of node '%pOF[%d]' - status (%d)\n",
 		 __func__, propname, np, index,
 		 PTR_ERR_OR_ZERO(desc));
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 0621baddfddc..543a120a0f84 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -29,6 +29,8 @@
 #include <uapi/linux/gpio.h>
 
 #include "gpiolib.h"
+#include "../pinctrl/core.h"
+#include "../pinctrl/pinconf.h"
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/gpio.h>
@@ -1997,9 +1999,9 @@ EXPORT_SYMBOL_GPL(gpiochip_generic_free);
  * @config: the configuration to be applied
  */
 int gpiochip_generic_config(struct gpio_chip *chip, unsigned offset,
-			    unsigned long config)
+			    unsigned long *configs, unsigned num_configs)
 {
-	return pinctrl_gpio_set_config(chip->gpiodev->base + offset, config);
+	return pinctrl_gpio_set_config(chip->gpiodev->base + offset, configs, num_configs);
 }
 EXPORT_SYMBOL_GPL(gpiochip_generic_config);
 
@@ -2161,6 +2163,18 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
 			goto done;
 		}
 	}
+	if (chip->set_config) {
+		unsigned long *configs;
+		unsigned num_configs;
+		int ret;
+
+		ret = pinconf_generic_parse_dt_config(desc->np_pincfg, NULL, &configs, &num_configs);
+		if (ret < 0) {
+			/* TODO */
+		}
+		chip->set_config(chip, gpio_chip_hwgpio(desc), configs, num_configs);
+		kfree(configs);
+	}
 	if (chip->get_direction) {
 		/* chip->get_direction may sleep */
 		spin_unlock_irqrestore(&gpio_lock, flags);
@@ -2404,7 +2418,7 @@ static int gpio_set_drive_single_ended(struct gpio_chip *gc, unsigned offset,
 {
 	unsigned long config = { PIN_CONF_PACKED(mode, 0) };
 
-	return gc->set_config ? gc->set_config(gc, offset, config) : -ENOTSUPP;
+	return gc->set_config ? gc->set_config(gc, offset, &config, 1) : -ENOTSUPP;
 }
 
 static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
@@ -2529,7 +2543,7 @@ int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
 	}
 
 	config = pinconf_to_config_packed(PIN_CONFIG_INPUT_DEBOUNCE, debounce);
-	return chip->set_config(chip, gpio_chip_hwgpio(desc), config);
+	return chip->set_config(chip, gpio_chip_hwgpio(desc), &config, 1);
 }
 EXPORT_SYMBOL_GPL(gpiod_set_debounce);
 
@@ -2565,7 +2579,7 @@ int gpiod_set_transitory(struct gpio_desc *desc, bool transitory)
 	packed = pinconf_to_config_packed(PIN_CONFIG_PERSIST_STATE,
 					  !transitory);
 	gpio = gpio_chip_hwgpio(desc);
-	rc = chip->set_config(chip, gpio, packed);
+	rc = chip->set_config(chip, gpio, &packed, 1);
 	if (rc == -ENOTSUPP) {
 		dev_dbg(&desc->gdev->dev, "Persistence not supported for GPIO %d\n",
 				gpio);
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 5e1f7cc6eeb6..79119548cc33 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -215,6 +215,8 @@ struct gpio_desc {
 	const char		*label;
 	/* Name of the GPIO */
 	const char		*name;
+	/* Pin configuration node */
+	struct device_node	*np_pincfg;
 };
 
 int gpiod_request(struct gpio_desc *desc, const char *label);
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 4c8d5b23e4d0..89976f35a6a3 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -857,9 +857,8 @@ EXPORT_SYMBOL_GPL(pinctrl_gpio_direction_output);
  * they need to call the underlying pin controller to change GPIO config
  * (for example set debounce time).
  */
-int pinctrl_gpio_set_config(unsigned gpio, unsigned long config)
+int pinctrl_gpio_set_config(unsigned gpio, unsigned long *configs, unsigned num_configs)
 {
-	unsigned long configs[] = { config };
 	struct pinctrl_gpio_range *range;
 	struct pinctrl_dev *pctldev;
 	int ret, pin;
@@ -870,7 +869,7 @@ int pinctrl_gpio_set_config(unsigned gpio, unsigned long config)
 
 	mutex_lock(&pctldev->mutex);
 	pin = gpio_to_pin(range, gpio);
-	ret = pinconf_set_config(pctldev, pin, configs, ARRAY_SIZE(configs));
+	ret = pinconf_set_config(pctldev, pin, configs, num_configs);
 	mutex_unlock(&pctldev->mutex);
 
 	return ret;
diff --git a/drivers/pinctrl/pinctrl-at91-pio4.c b/drivers/pinctrl/pinctrl-at91-pio4.c
index b1ca838dd80a..477a4d7ddbaf 100644
--- a/drivers/pinctrl/pinctrl-at91-pio4.c
+++ b/drivers/pinctrl/pinctrl-at91-pio4.c
@@ -358,12 +358,15 @@ static int atmel_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
 }
 
 static struct gpio_chip atmel_gpio_chip = {
+	.request		= gpiochip_generic_request,
+	.free			= gpiochip_generic_free,
 	.direction_input        = atmel_gpio_direction_input,
 	.get                    = atmel_gpio_get,
 	.direction_output       = atmel_gpio_direction_output,
 	.set                    = atmel_gpio_set,
 	.to_irq                 = atmel_gpio_to_irq,
 	.base                   = 0,
+	.set_config		= gpiochip_generic_config,
 };
 
 /* --- PINCTRL --- */
@@ -643,11 +646,26 @@ static int atmel_pmx_set_mux(struct pinctrl_dev *pctldev,
 	return 0;
 }
 
+static int atmel_pmx_gpio_request_enable(struct pinctrl_dev *pctldev, struct pinctrl_gpio_range *range, unsigned offset)
+{
+	u32 conf;
+
+	conf = atmel_pin_config_read(pctldev, offset);
+	conf &= (~ATMEL_PIO_CFGR_FUNC_MASK);
+	atmel_pin_config_write(pctldev, offset, conf);
+
+	dev_dbg(pctldev->dev, "enable pin %u as GPIO\n", offset);
+
+	return 0;
+}
+
 static const struct pinmux_ops atmel_pmxops = {
+	.gpio_request_enable	= atmel_pmx_gpio_request_enable,
 	.get_functions_count	= atmel_pmx_get_functions_count,
 	.get_function_name	= atmel_pmx_get_function_name,
 	.get_function_groups	= atmel_pmx_get_function_groups,
 	.set_mux		= atmel_pmx_set_mux,
+	.strict			= true,
 };
 
 static int atmel_conf_pin_config_group_get(struct pinctrl_dev *pctldev,
@@ -817,6 +835,7 @@ static void atmel_conf_pin_config_dbg_show(struct pinctrl_dev *pctldev,
 }
 
 static const struct pinconf_ops atmel_confops = {
+	.pin_config_set		= atmel_conf_pin_config_group_set,
 	.pin_config_group_get	= atmel_conf_pin_config_group_get,
 	.pin_config_group_set	= atmel_conf_pin_config_group_set,
 	.pin_config_dbg_show	= atmel_conf_pin_config_dbg_show,
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 55e672592fa9..b54968cd6dfc 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -247,7 +247,8 @@ struct gpio_chip {
 						unsigned long *bits);
 	int			(*set_config)(struct gpio_chip *chip,
 					      unsigned offset,
-					      unsigned long config);
+					      unsigned long *configs,
+					      unsigned num_configs);
 	int			(*to_irq)(struct gpio_chip *chip,
 						unsigned offset);
 
@@ -490,7 +491,7 @@ static inline int gpiochip_irqchip_add_nested(struct gpio_chip *gpiochip,
 int gpiochip_generic_request(struct gpio_chip *chip, unsigned offset);
 void gpiochip_generic_free(struct gpio_chip *chip, unsigned offset);
 int gpiochip_generic_config(struct gpio_chip *chip, unsigned offset,
-			    unsigned long config);
+			    unsigned long *configs, unsigned num_configs);
 
 #ifdef CONFIG_PINCTRL
 
diff --git a/include/linux/pinctrl/consumer.h b/include/linux/pinctrl/consumer.h
index 0412cc9833e9..14be53e1e053 100644
--- a/include/linux/pinctrl/consumer.h
+++ b/include/linux/pinctrl/consumer.h
@@ -29,7 +29,7 @@ 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);
+extern int pinctrl_gpio_set_config(unsigned gpio, unsigned long *configs, unsigned num_configs);
 
 extern struct pinctrl * __must_check pinctrl_get(struct device *dev);
 extern void pinctrl_put(struct pinctrl *p);
@@ -81,7 +81,7 @@ static inline int pinctrl_gpio_direction_output(unsigned gpio)
 	return 0;
 }
 
-static inline int pinctrl_gpio_set_config(unsigned gpio, unsigned long config)
+static inline int pinctrl_gpio_set_config(unsigned gpio, unsigned long *configs, unsigned num_configs)
 {
 	return 0;
 }
-- 
2.12.2


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

* [RFC PATCH v2] draft for gpio pinconf
@ 2017-12-19  9:40   ` Ludovic Desroches
  0 siblings, 0 replies; 26+ messages in thread
From: Ludovic Desroches @ 2017-12-19  9:40 UTC (permalink / raw)
  To: linux-arm-kernel

Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com>
---

Hi,

After discussing with Maxime Ripard, I realize that the first approach
may not be a good one.

For instance, the drive-strength is a digital value so it will take
ake several bits. Having a whole pin configuration in 32 bits may not
be the right solution.

My aim is to use the existing pinconf bindings.

This patch is a draft to test the feasibility. Sorry to provide it in
this state (several ugly hacks) but I didn't want to spend time on a clean
solution if it will be rejected.

 arch/arm/boot/dts/at91-sama5d2_xplained.dts |  6 +-----
 arch/arm/boot/dts/sama5d2.dtsi              |  2 +-
 drivers/gpio/gpiolib-of.c                   | 10 +++++++++-
 drivers/gpio/gpiolib.c                      | 24 +++++++++++++++++++-----
 drivers/gpio/gpiolib.h                      |  2 ++
 drivers/pinctrl/core.c                      |  5 ++---
 drivers/pinctrl/pinctrl-at91-pio4.c         | 19 +++++++++++++++++++
 include/linux/gpio/driver.h                 |  5 +++--
 include/linux/pinctrl/consumer.h            |  4 ++--
 9 files changed, 58 insertions(+), 19 deletions(-)

diff --git a/arch/arm/boot/dts/at91-sama5d2_xplained.dts b/arch/arm/boot/dts/at91-sama5d2_xplained.dts
index 56de21de2779..709c9c3ae622 100644
--- a/arch/arm/boot/dts/at91-sama5d2_xplained.dts
+++ b/arch/arm/boot/dts/at91-sama5d2_xplained.dts
@@ -413,7 +413,6 @@
 				};
 
 				pinctrl_key_gpio_default: key_gpio_default {
-					pinmux = <PIN_PB9__GPIO>;
 					bias-pull-up;
 				};
 
@@ -545,12 +544,9 @@
 	gpio_keys {
 		compatible = "gpio-keys";
 
-		pinctrl-names = "default";
-		pinctrl-0 = <&pinctrl_key_gpio_default>;
-
 		bp1 {
 			label = "PB_USER";
-			gpios = <&pioA PIN_PB9 GPIO_ACTIVE_LOW>;
+			gpios = <&pioA PIN_PB9 GPIO_ACTIVE_LOW &pinctrl_key_gpio_default>;
 			linux,code = <0x104>;
 			wakeup-source;
 		};
diff --git a/arch/arm/boot/dts/sama5d2.dtsi b/arch/arm/boot/dts/sama5d2.dtsi
index b44e63995583..cccd404192cc 100644
--- a/arch/arm/boot/dts/sama5d2.dtsi
+++ b/arch/arm/boot/dts/sama5d2.dtsi
@@ -1444,7 +1444,7 @@
 				interrupt-controller;
 				#interrupt-cells = <2>;
 				gpio-controller;
-				#gpio-cells = <2>;
+				#gpio-cells = <3>;
 				clocks = <&pioA_clk>;
 			};
 
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 4a2b8d3397c7..605017a4d2d8 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -46,7 +46,7 @@ static struct gpio_desc *of_xlate_and_get_gpiod_flags(struct gpio_chip *chip,
 {
 	int ret;
 
-	if (chip->of_gpio_n_cells != gpiospec->args_count)
+	if (chip->of_gpio_n_cells > gpiospec->args_count)
 		return ERR_PTR(-EINVAL);
 
 	ret = chip->of_xlate(chip, gpiospec, flags);
@@ -93,6 +93,14 @@ struct gpio_desc *of_get_named_gpiod_flags(struct device_node *np,
 	if (IS_ERR(desc))
 		goto out;
 
+	if (gpiospec.args_count > chip->of_gpio_n_cells) {
+		desc->np_pincfg = of_parse_phandle(np, propname, gpiospec.args_count);
+		if (!desc->np_pincfg) {
+			printk("ERROR\n");
+			goto out;
+		}
+	}
+
 	pr_debug("%s: parsed '%s' property of node '%pOF[%d]' - status (%d)\n",
 		 __func__, propname, np, index,
 		 PTR_ERR_OR_ZERO(desc));
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 0621baddfddc..543a120a0f84 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -29,6 +29,8 @@
 #include <uapi/linux/gpio.h>
 
 #include "gpiolib.h"
+#include "../pinctrl/core.h"
+#include "../pinctrl/pinconf.h"
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/gpio.h>
@@ -1997,9 +1999,9 @@ EXPORT_SYMBOL_GPL(gpiochip_generic_free);
  * @config: the configuration to be applied
  */
 int gpiochip_generic_config(struct gpio_chip *chip, unsigned offset,
-			    unsigned long config)
+			    unsigned long *configs, unsigned num_configs)
 {
-	return pinctrl_gpio_set_config(chip->gpiodev->base + offset, config);
+	return pinctrl_gpio_set_config(chip->gpiodev->base + offset, configs, num_configs);
 }
 EXPORT_SYMBOL_GPL(gpiochip_generic_config);
 
@@ -2161,6 +2163,18 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
 			goto done;
 		}
 	}
+	if (chip->set_config) {
+		unsigned long *configs;
+		unsigned num_configs;
+		int ret;
+
+		ret = pinconf_generic_parse_dt_config(desc->np_pincfg, NULL, &configs, &num_configs);
+		if (ret < 0) {
+			/* TODO */
+		}
+		chip->set_config(chip, gpio_chip_hwgpio(desc), configs, num_configs);
+		kfree(configs);
+	}
 	if (chip->get_direction) {
 		/* chip->get_direction may sleep */
 		spin_unlock_irqrestore(&gpio_lock, flags);
@@ -2404,7 +2418,7 @@ static int gpio_set_drive_single_ended(struct gpio_chip *gc, unsigned offset,
 {
 	unsigned long config = { PIN_CONF_PACKED(mode, 0) };
 
-	return gc->set_config ? gc->set_config(gc, offset, config) : -ENOTSUPP;
+	return gc->set_config ? gc->set_config(gc, offset, &config, 1) : -ENOTSUPP;
 }
 
 static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
@@ -2529,7 +2543,7 @@ int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
 	}
 
 	config = pinconf_to_config_packed(PIN_CONFIG_INPUT_DEBOUNCE, debounce);
-	return chip->set_config(chip, gpio_chip_hwgpio(desc), config);
+	return chip->set_config(chip, gpio_chip_hwgpio(desc), &config, 1);
 }
 EXPORT_SYMBOL_GPL(gpiod_set_debounce);
 
@@ -2565,7 +2579,7 @@ int gpiod_set_transitory(struct gpio_desc *desc, bool transitory)
 	packed = pinconf_to_config_packed(PIN_CONFIG_PERSIST_STATE,
 					  !transitory);
 	gpio = gpio_chip_hwgpio(desc);
-	rc = chip->set_config(chip, gpio, packed);
+	rc = chip->set_config(chip, gpio, &packed, 1);
 	if (rc == -ENOTSUPP) {
 		dev_dbg(&desc->gdev->dev, "Persistence not supported for GPIO %d\n",
 				gpio);
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 5e1f7cc6eeb6..79119548cc33 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -215,6 +215,8 @@ struct gpio_desc {
 	const char		*label;
 	/* Name of the GPIO */
 	const char		*name;
+	/* Pin configuration node */
+	struct device_node	*np_pincfg;
 };
 
 int gpiod_request(struct gpio_desc *desc, const char *label);
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 4c8d5b23e4d0..89976f35a6a3 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -857,9 +857,8 @@ EXPORT_SYMBOL_GPL(pinctrl_gpio_direction_output);
  * they need to call the underlying pin controller to change GPIO config
  * (for example set debounce time).
  */
-int pinctrl_gpio_set_config(unsigned gpio, unsigned long config)
+int pinctrl_gpio_set_config(unsigned gpio, unsigned long *configs, unsigned num_configs)
 {
-	unsigned long configs[] = { config };
 	struct pinctrl_gpio_range *range;
 	struct pinctrl_dev *pctldev;
 	int ret, pin;
@@ -870,7 +869,7 @@ int pinctrl_gpio_set_config(unsigned gpio, unsigned long config)
 
 	mutex_lock(&pctldev->mutex);
 	pin = gpio_to_pin(range, gpio);
-	ret = pinconf_set_config(pctldev, pin, configs, ARRAY_SIZE(configs));
+	ret = pinconf_set_config(pctldev, pin, configs, num_configs);
 	mutex_unlock(&pctldev->mutex);
 
 	return ret;
diff --git a/drivers/pinctrl/pinctrl-at91-pio4.c b/drivers/pinctrl/pinctrl-at91-pio4.c
index b1ca838dd80a..477a4d7ddbaf 100644
--- a/drivers/pinctrl/pinctrl-at91-pio4.c
+++ b/drivers/pinctrl/pinctrl-at91-pio4.c
@@ -358,12 +358,15 @@ static int atmel_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
 }
 
 static struct gpio_chip atmel_gpio_chip = {
+	.request		= gpiochip_generic_request,
+	.free			= gpiochip_generic_free,
 	.direction_input        = atmel_gpio_direction_input,
 	.get                    = atmel_gpio_get,
 	.direction_output       = atmel_gpio_direction_output,
 	.set                    = atmel_gpio_set,
 	.to_irq                 = atmel_gpio_to_irq,
 	.base                   = 0,
+	.set_config		= gpiochip_generic_config,
 };
 
 /* --- PINCTRL --- */
@@ -643,11 +646,26 @@ static int atmel_pmx_set_mux(struct pinctrl_dev *pctldev,
 	return 0;
 }
 
+static int atmel_pmx_gpio_request_enable(struct pinctrl_dev *pctldev, struct pinctrl_gpio_range *range, unsigned offset)
+{
+	u32 conf;
+
+	conf = atmel_pin_config_read(pctldev, offset);
+	conf &= (~ATMEL_PIO_CFGR_FUNC_MASK);
+	atmel_pin_config_write(pctldev, offset, conf);
+
+	dev_dbg(pctldev->dev, "enable pin %u as GPIO\n", offset);
+
+	return 0;
+}
+
 static const struct pinmux_ops atmel_pmxops = {
+	.gpio_request_enable	= atmel_pmx_gpio_request_enable,
 	.get_functions_count	= atmel_pmx_get_functions_count,
 	.get_function_name	= atmel_pmx_get_function_name,
 	.get_function_groups	= atmel_pmx_get_function_groups,
 	.set_mux		= atmel_pmx_set_mux,
+	.strict			= true,
 };
 
 static int atmel_conf_pin_config_group_get(struct pinctrl_dev *pctldev,
@@ -817,6 +835,7 @@ static void atmel_conf_pin_config_dbg_show(struct pinctrl_dev *pctldev,
 }
 
 static const struct pinconf_ops atmel_confops = {
+	.pin_config_set		= atmel_conf_pin_config_group_set,
 	.pin_config_group_get	= atmel_conf_pin_config_group_get,
 	.pin_config_group_set	= atmel_conf_pin_config_group_set,
 	.pin_config_dbg_show	= atmel_conf_pin_config_dbg_show,
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 55e672592fa9..b54968cd6dfc 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -247,7 +247,8 @@ struct gpio_chip {
 						unsigned long *bits);
 	int			(*set_config)(struct gpio_chip *chip,
 					      unsigned offset,
-					      unsigned long config);
+					      unsigned long *configs,
+					      unsigned num_configs);
 	int			(*to_irq)(struct gpio_chip *chip,
 						unsigned offset);
 
@@ -490,7 +491,7 @@ static inline int gpiochip_irqchip_add_nested(struct gpio_chip *gpiochip,
 int gpiochip_generic_request(struct gpio_chip *chip, unsigned offset);
 void gpiochip_generic_free(struct gpio_chip *chip, unsigned offset);
 int gpiochip_generic_config(struct gpio_chip *chip, unsigned offset,
-			    unsigned long config);
+			    unsigned long *configs, unsigned num_configs);
 
 #ifdef CONFIG_PINCTRL
 
diff --git a/include/linux/pinctrl/consumer.h b/include/linux/pinctrl/consumer.h
index 0412cc9833e9..14be53e1e053 100644
--- a/include/linux/pinctrl/consumer.h
+++ b/include/linux/pinctrl/consumer.h
@@ -29,7 +29,7 @@ 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);
+extern int pinctrl_gpio_set_config(unsigned gpio, unsigned long *configs, unsigned num_configs);
 
 extern struct pinctrl * __must_check pinctrl_get(struct device *dev);
 extern void pinctrl_put(struct pinctrl *p);
@@ -81,7 +81,7 @@ static inline int pinctrl_gpio_direction_output(unsigned gpio)
 	return 0;
 }
 
-static inline int pinctrl_gpio_set_config(unsigned gpio, unsigned long config)
+static inline int pinctrl_gpio_set_config(unsigned gpio, unsigned long *configs, unsigned num_configs)
 {
 	return 0;
 }
-- 
2.12.2

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

end of thread, other threads:[~2017-12-19  9:42 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-14 14:21 [RFC PATCH 0/7] gpiolib: add bias support Ludovic Desroches
2017-12-14 14:21 ` Ludovic Desroches
2017-12-14 14:21 ` [RFC PATCH 1/7] gpio: of: use the BIT macro for of_gpio_flags Ludovic Desroches
2017-12-14 14:21   ` Ludovic Desroches
2017-12-14 14:21 ` [RFC PATCH 2/7] gpio: gpiolib: split the gpiod_configure_flags function Ludovic Desroches
2017-12-14 14:21   ` Ludovic Desroches
2017-12-15  9:26   ` Julien Thierry
2017-12-15  9:26     ` Julien Thierry
2017-12-18  7:02     ` Ludovic Desroches
2017-12-18  7:02       ` Ludovic Desroches
2017-12-14 14:21 ` [RFC PATCH 3/7] gpio: gpiolib: save GPIO flags in of_get_named_gpiod_flags Ludovic Desroches
2017-12-14 14:21   ` Ludovic Desroches
2017-12-14 14:21 ` [RFC PATCH 4/7] gpio: gpiolib: add bias support Ludovic Desroches
2017-12-14 14:21   ` Ludovic Desroches
2017-12-14 14:21 ` [RFC PATCH 5/7] pinctrl: at91-pio4: allow the gpiolib to set pin configuration Ludovic Desroches
2017-12-14 14:21   ` Ludovic Desroches
2017-12-14 14:21 ` [RFC PATCH 6/7] pinctrl: at91-pio4: use strict mode if explicitly requested Ludovic Desroches
2017-12-14 14:21   ` Ludovic Desroches
2017-12-14 14:21 ` [RFC PATCH 7/7] ARM: dts: at91-sama5d2_xplained: remove gpios from pinmux Ludovic Desroches
2017-12-14 14:21   ` Ludovic Desroches
2017-12-14 16:06 ` [RFC PATCH 0/7] gpiolib: add bias support Andrew Lunn
2017-12-14 16:06   ` Andrew Lunn
2017-12-15  6:54   ` Ludovic Desroches
2017-12-15  6:54     ` Ludovic Desroches
2017-12-19  9:40 ` [RFC PATCH v2] draft for gpio pinconf Ludovic Desroches
2017-12-19  9:40   ` 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.