[PATCH v6 0/7] gpio: expose line bias flags to userspace

[PATCH v6 0/7] gpio: expose line bias flags to userspace

[Kent Gibson]

The changes from v5: 
  - rebased onto Bart's gpio/for-kent branch.

The changes from v4:
Review changes:
 - relocate restriction on applying bias to as-is from patch 2 to patch 1.
 - propagate errors, other than ENOTSUPP, from gpio_set_bias. (squashed 
   into patches 3 and 4).
 - include SET_CONFIG patch series v2 (patch 6 and 7 here).

I've also fixed a few other nits I noticed along the way:
 - rework gpio_set_bias as flags are mutually exclusive.
 - remove input flag required to set bias restriction from 
   lineevent_create as events are implicitly assumed inputs anyway.
 - reorder patches to group gpiolib bias patches together before the 
   gpio-mockup changes.


This series adds gross control of pull-up/pull-down to the GPIO uAPI.
Gross control means enabling and disabling of bias functionality,
not finer grained control such as setting biasing impedances.

The support allows both input and output lines to have any one of the
following biases applied as part of the line handle or event request:
 0. As Is - bias is left alone.  This is the default for ABI compatibility.
 1. Bias Disable - bias is explicitly disabled.
 2. Pull Down - pull-down bias is enabled.
 3. Pull Up - pull-up bias is enabled.

The biases are set via three flags, BIAS_DISABLE, BIAS_PULL_DOWN
and BIAS_PULL_UP.  These map directly to the similarly named 
pinctrl pin_config_param flags.
As Is corresponds to none of the flags being set.

The setting of biases on output lines may seem odd, but is to allow for
utilisation of internal pull-up/pull-down on open drain and open source
outputs, where supported in hardware.

The series also adds the GPIOHANDLE_SET_CONFIG_IOCTL to the gpio chardev.
The ioctl allows some of the configuration of a requested handle to be
changed without having to release the line.
The primary use case is the changing of direction for bi-directional 
lines.

Patches are against Bart's gpio/for-kent branch[1].

The patch has been successfully tested against gpio-mockup, and 
on a Raspberry Pi, in both cases using the feature/pud_set_config 
branch of my Go gpiod library[2], as well as with my feature/pud 
development branch of libgpiod[3].  Patch 7 has only been tested using 
my gpiod library as libgpiod has not yet been updated to support the 
SET_CONFIG ioctl.

Patch 1 adds pull-up/pull-down support to line handle requests.
Patch 2 adds pull-up/pull-down support to line event requests.
Patch 3 adds support for disabling bias.
Patch 4 adds support for setting bias on output lines.
Patch 5 adds pull-up/down support to the gpio-mockup for uAPI testing.
Patch 6 refactors the flag validation from linehandle_create.
Patch 7 adds the SET_CONFIG ioctl.

Drew Fustini (1):
  gpio: expose pull-up/pull-down line flags to userspace

Kent Gibson (6):
  gpiolib: add support for pull up/down to lineevent_create
  gpiolib: add support for disabling line bias
  gpiolib: add support for biasing output lines
  gpio: mockup: add set_config to support pull up/down
  gpiolib: move validation of line handle flags into helper function
  gpio: add new SET_CONFIG ioctl() to gpio chardev

 drivers/gpio/gpio-mockup.c |  94 ++++++++++------
 drivers/gpio/gpiolib.c     | 213 +++++++++++++++++++++++++++++++------
 drivers/gpio/gpiolib.h     |   1 +
 include/uapi/linux/gpio.h  |  24 +++++
 4 files changed, 264 insertions(+), 68 deletions(-)

-- 
2.23.0

[PATCH v6 1/7] gpio: expose pull-up/pull-down line flags to userspace

[Kent Gibson]

From: Drew Fustini <drew@pdp7.com>

Add pull-up/pull-down flags to the gpio line get and
set ioctl() calls.  Use cases include a push button
that does not have an external resistor.

Addition use cases described by Limor Fried (ladyada) of
Adafruit in this PR for Adafruit_Blinka Python lib:
https://github.com/adafruit/Adafruit_Blinka/pull/59

Signed-off-by: Drew Fustini <drew@pdp7.com>
[Kent: added BIAS to GPIO flag names and restrict application to input
lines]
Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 drivers/gpio/gpiolib.c    | 18 ++++++++++++++++++
 include/uapi/linux/gpio.h |  4 ++++
 2 files changed, 22 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 0be71924c0f7..55a8443bd4af 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -422,6 +422,8 @@ struct linehandle_state {
 	(GPIOHANDLE_REQUEST_INPUT | \
 	GPIOHANDLE_REQUEST_OUTPUT | \
 	GPIOHANDLE_REQUEST_ACTIVE_LOW | \
+	GPIOHANDLE_REQUEST_BIAS_PULL_UP | \
+	GPIOHANDLE_REQUEST_BIAS_PULL_DOWN | \
 	GPIOHANDLE_REQUEST_OPEN_DRAIN | \
 	GPIOHANDLE_REQUEST_OPEN_SOURCE)
 
@@ -553,6 +555,12 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 	     (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE)))
 		return -EINVAL;
 
+	/* PULL_UP and PULL_DOWN flags only make sense for input mode. */
+	if (!(lflags & GPIOHANDLE_REQUEST_INPUT) &&
+	    ((lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP) ||
+	     (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN)))
+		return -EINVAL;
+
 	lh = kzalloc(sizeof(*lh), GFP_KERNEL);
 	if (!lh)
 		return -ENOMEM;
@@ -593,6 +601,10 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 			set_bit(FLAG_OPEN_DRAIN, &desc->flags);
 		if (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE)
 			set_bit(FLAG_OPEN_SOURCE, &desc->flags);
+		if (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN)
+			set_bit(FLAG_PULL_DOWN, &desc->flags);
+		if (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP)
+			set_bit(FLAG_PULL_UP, &desc->flags);
 
 		ret = gpiod_set_transitory(desc, false);
 		if (ret < 0)
@@ -1092,6 +1104,10 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		if (test_bit(FLAG_OPEN_SOURCE, &desc->flags))
 			lineinfo.flags |= (GPIOLINE_FLAG_OPEN_SOURCE |
 					   GPIOLINE_FLAG_IS_OUT);
+		if (test_bit(FLAG_PULL_DOWN, &desc->flags))
+			lineinfo.flags |= GPIOLINE_FLAG_BIAS_PULL_DOWN;
+		if (test_bit(FLAG_PULL_UP, &desc->flags))
+			lineinfo.flags |= GPIOLINE_FLAG_BIAS_PULL_UP;
 
 		if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
 			return -EFAULT;
@@ -2784,6 +2800,8 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
 		clear_bit(FLAG_REQUESTED, &desc->flags);
 		clear_bit(FLAG_OPEN_DRAIN, &desc->flags);
 		clear_bit(FLAG_OPEN_SOURCE, &desc->flags);
+		clear_bit(FLAG_PULL_UP, &desc->flags);
+		clear_bit(FLAG_PULL_DOWN, &desc->flags);
 		clear_bit(FLAG_IS_HOGGED, &desc->flags);
 		ret = true;
 	}
diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
index 4ebfe0ac6c5b..39e6c7854d63 100644
--- a/include/uapi/linux/gpio.h
+++ b/include/uapi/linux/gpio.h
@@ -33,6 +33,8 @@ struct gpiochip_info {
 #define GPIOLINE_FLAG_ACTIVE_LOW	(1UL << 2)
 #define GPIOLINE_FLAG_OPEN_DRAIN	(1UL << 3)
 #define GPIOLINE_FLAG_OPEN_SOURCE	(1UL << 4)
+#define GPIOLINE_FLAG_BIAS_PULL_UP	(1UL << 5)
+#define GPIOLINE_FLAG_BIAS_PULL_DOWN	(1UL << 6)
 
 /**
  * struct gpioline_info - Information about a certain GPIO line
@@ -62,6 +64,8 @@ struct gpioline_info {
 #define GPIOHANDLE_REQUEST_ACTIVE_LOW	(1UL << 2)
 #define GPIOHANDLE_REQUEST_OPEN_DRAIN	(1UL << 3)
 #define GPIOHANDLE_REQUEST_OPEN_SOURCE	(1UL << 4)
+#define GPIOHANDLE_REQUEST_BIAS_PULL_UP	(1UL << 5)
+#define GPIOHANDLE_REQUEST_BIAS_PULL_DOWN	(1UL << 6)
 
 /**
  * struct gpiohandle_request - Information about a GPIO handle request
-- 
2.23.0

[PATCH v6 2/7] gpiolib: add support for pull up/down to lineevent_create

[Kent Gibson]

Add support for pull up/down to lineevent_create.
Use cases include receiving asynchronous presses from a
push button without an external pull up/down.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 drivers/gpio/gpiolib.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 55a8443bd4af..24bdfdd26b85 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -951,6 +951,10 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 
 	if (lflags & GPIOHANDLE_REQUEST_ACTIVE_LOW)
 		set_bit(FLAG_ACTIVE_LOW, &desc->flags);
+	if (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN)
+		set_bit(FLAG_PULL_DOWN, &desc->flags);
+	if (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP)
+		set_bit(FLAG_PULL_UP, &desc->flags);
 
 	ret = gpiod_direction_input(desc);
 	if (ret)
-- 
2.23.0

[PATCH v6 3/7] gpiolib: add support for disabling line bias

[Kent Gibson]

Allow pull up/down bias to be disabled, allowing the line to float
or to be biased only by external circuitry.
Use case is for where the bias has been applied previously, either
by default or by the user, but that setting may conflict with the
current use of the line.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 drivers/gpio/gpiolib.c    | 61 ++++++++++++++++++++++++++++++++-------
 drivers/gpio/gpiolib.h    |  1 +
 include/uapi/linux/gpio.h |  2 ++
 3 files changed, 54 insertions(+), 10 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 24bdfdd26b85..0ac0a250b17a 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -424,6 +424,7 @@ struct linehandle_state {
 	GPIOHANDLE_REQUEST_ACTIVE_LOW | \
 	GPIOHANDLE_REQUEST_BIAS_PULL_UP | \
 	GPIOHANDLE_REQUEST_BIAS_PULL_DOWN | \
+	GPIOHANDLE_REQUEST_BIAS_DISABLE | \
 	GPIOHANDLE_REQUEST_OPEN_DRAIN | \
 	GPIOHANDLE_REQUEST_OPEN_SOURCE)
 
@@ -555,12 +556,21 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 	     (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE)))
 		return -EINVAL;
 
-	/* PULL_UP and PULL_DOWN flags only make sense for input mode. */
+	/* Bias flags only allowed for input mode. */
 	if (!(lflags & GPIOHANDLE_REQUEST_INPUT) &&
-	    ((lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP) ||
+	    ((lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE) ||
+	     (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP) ||
 	     (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN)))
 		return -EINVAL;
 
+	/* Only one bias flag can be set. */
+	if (((lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE) &&
+	     (lflags & (GPIOHANDLE_REQUEST_BIAS_PULL_DOWN |
+			GPIOHANDLE_REQUEST_BIAS_PULL_UP))) ||
+	    ((lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN) &&
+	     (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP)))
+		return -EINVAL;
+
 	lh = kzalloc(sizeof(*lh), GFP_KERNEL);
 	if (!lh)
 		return -ENOMEM;
@@ -601,6 +611,8 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 			set_bit(FLAG_OPEN_DRAIN, &desc->flags);
 		if (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE)
 			set_bit(FLAG_OPEN_SOURCE, &desc->flags);
+		if (lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE)
+			set_bit(FLAG_BIAS_DISABLE, &desc->flags);
 		if (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN)
 			set_bit(FLAG_PULL_DOWN, &desc->flags);
 		if (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP)
@@ -925,6 +937,14 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 	    (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE))
 		return -EINVAL;
 
+	/* Only one bias flag can be set. */
+	if (((lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE) &&
+	     (lflags & (GPIOHANDLE_REQUEST_BIAS_PULL_DOWN |
+			GPIOHANDLE_REQUEST_BIAS_PULL_UP))) ||
+	    ((lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN) &&
+	     (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP)))
+		return -EINVAL;
+
 	le = kzalloc(sizeof(*le), GFP_KERNEL);
 	if (!le)
 		return -ENOMEM;
@@ -951,6 +971,8 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 
 	if (lflags & GPIOHANDLE_REQUEST_ACTIVE_LOW)
 		set_bit(FLAG_ACTIVE_LOW, &desc->flags);
+	if (lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE)
+		set_bit(FLAG_BIAS_DISABLE, &desc->flags);
 	if (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN)
 		set_bit(FLAG_PULL_DOWN, &desc->flags);
 	if (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP)
@@ -1108,6 +1130,8 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		if (test_bit(FLAG_OPEN_SOURCE, &desc->flags))
 			lineinfo.flags |= (GPIOLINE_FLAG_OPEN_SOURCE |
 					   GPIOLINE_FLAG_IS_OUT);
+		if (test_bit(FLAG_BIAS_DISABLE, &desc->flags))
+			lineinfo.flags |= GPIOLINE_FLAG_BIAS_DISABLE;
 		if (test_bit(FLAG_PULL_DOWN, &desc->flags))
 			lineinfo.flags |= GPIOLINE_FLAG_BIAS_PULL_DOWN;
 		if (test_bit(FLAG_PULL_UP, &desc->flags))
@@ -2806,6 +2830,7 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
 		clear_bit(FLAG_OPEN_SOURCE, &desc->flags);
 		clear_bit(FLAG_PULL_UP, &desc->flags);
 		clear_bit(FLAG_PULL_DOWN, &desc->flags);
+		clear_bit(FLAG_BIAS_DISABLE, &desc->flags);
 		clear_bit(FLAG_IS_HOGGED, &desc->flags);
 		ret = true;
 	}
@@ -2932,6 +2957,7 @@ static int gpio_set_config(struct gpio_chip *gc, unsigned offset,
 	unsigned arg;
 
 	switch (mode) {
+	case PIN_CONFIG_BIAS_DISABLE:
 	case PIN_CONFIG_BIAS_PULL_DOWN:
 	case PIN_CONFIG_BIAS_PULL_UP:
 		arg = 1;
@@ -2945,6 +2971,26 @@ static int gpio_set_config(struct gpio_chip *gc, unsigned offset,
 	return gc->set_config ? gc->set_config(gc, offset, config) : -ENOTSUPP;
 }
 
+static int gpio_set_bias(struct gpio_chip *chip, struct gpio_desc *desc)
+{
+	int bias = 0;
+	int ret = 0;
+
+	if (test_bit(FLAG_BIAS_DISABLE, &desc->flags))
+		bias = PIN_CONFIG_BIAS_DISABLE;
+	else if (test_bit(FLAG_PULL_UP, &desc->flags))
+		bias = PIN_CONFIG_BIAS_PULL_UP;
+	else if (test_bit(FLAG_PULL_DOWN, &desc->flags))
+		bias = PIN_CONFIG_BIAS_PULL_DOWN;
+
+	if (bias) {
+		ret = gpio_set_config(chip, gpio_chip_hwgpio(desc), bias);
+		if (ret != -ENOTSUPP)
+			return ret;
+	}
+	return 0;
+}
+
 /**
  * gpiod_direction_input - set the GPIO direction to input
  * @desc:	GPIO to set to input
@@ -2989,15 +3035,10 @@ int gpiod_direction_input(struct gpio_desc *desc)
 			   __func__);
 		return -EIO;
 	}
-	if (ret == 0)
+	if (ret == 0) {
 		clear_bit(FLAG_IS_OUT, &desc->flags);
-
-	if (test_bit(FLAG_PULL_UP, &desc->flags))
-		gpio_set_config(chip, gpio_chip_hwgpio(desc),
-				PIN_CONFIG_BIAS_PULL_UP);
-	else if (test_bit(FLAG_PULL_DOWN, &desc->flags))
-		gpio_set_config(chip, gpio_chip_hwgpio(desc),
-				PIN_CONFIG_BIAS_PULL_DOWN);
+		ret = gpio_set_bias(chip, desc);
+	}
 
 	trace_gpio_direction(desc_to_gpio(desc), 1, ret);
 
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index b8b10a409c7b..ca9bc1e4803c 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -110,6 +110,7 @@ struct gpio_desc {
 #define FLAG_TRANSITORY 12	/* GPIO may lose value in sleep or reset */
 #define FLAG_PULL_UP    13	/* GPIO has pull up enabled */
 #define FLAG_PULL_DOWN  14	/* GPIO has pull down enabled */
+#define FLAG_BIAS_DISABLE    15	/* GPIO has pull disabled */
 
 	/* Connection label */
 	const char		*label;
diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
index 39e6c7854d63..7cc21c3b0839 100644
--- a/include/uapi/linux/gpio.h
+++ b/include/uapi/linux/gpio.h
@@ -35,6 +35,7 @@ struct gpiochip_info {
 #define GPIOLINE_FLAG_OPEN_SOURCE	(1UL << 4)
 #define GPIOLINE_FLAG_BIAS_PULL_UP	(1UL << 5)
 #define GPIOLINE_FLAG_BIAS_PULL_DOWN	(1UL << 6)
+#define GPIOLINE_FLAG_BIAS_DISABLE	(1UL << 7)
 
 /**
  * struct gpioline_info - Information about a certain GPIO line
@@ -66,6 +67,7 @@ struct gpioline_info {
 #define GPIOHANDLE_REQUEST_OPEN_SOURCE	(1UL << 4)
 #define GPIOHANDLE_REQUEST_BIAS_PULL_UP	(1UL << 5)
 #define GPIOHANDLE_REQUEST_BIAS_PULL_DOWN	(1UL << 6)
+#define GPIOHANDLE_REQUEST_BIAS_DISABLE	(1UL << 7)
 
 /**
  * struct gpiohandle_request - Information about a GPIO handle request
-- 
2.23.0

[PATCH v6 4/7] gpiolib: add support for biasing output lines

[Kent Gibson]

Allow pull up/down bias to be set on output lines.
Use case is for open source or open drain applications where
internal pull up/down may conflict with external biasing.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 drivers/gpio/gpiolib.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 0ac0a250b17a..f852ff60ae6e 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -556,8 +556,9 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 	     (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE)))
 		return -EINVAL;
 
-	/* Bias flags only allowed for input mode. */
-	if (!(lflags & GPIOHANDLE_REQUEST_INPUT) &&
+	/* Bias flags only allowed for input or output mode. */
+	if (!((lflags & GPIOHANDLE_REQUEST_INPUT) ||
+	      (lflags & GPIOHANDLE_REQUEST_OUTPUT)) &&
 	    ((lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE) ||
 	     (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP) ||
 	     (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN)))
@@ -3168,6 +3169,9 @@ int gpiod_direction_output(struct gpio_desc *desc, int value)
 	}
 
 set_output_value:
+	ret = gpio_set_bias(gc, desc);
+	if (ret)
+		return ret;
 	return gpiod_direction_output_raw_commit(desc, value);
 
 set_output_flag:
-- 
2.23.0

[PATCH v6 5/7] gpio: mockup: add set_config to support pull up/down

[Kent Gibson]

Add support for the pull up/down state set via gpiolib line requests to
be reflected in the state of the mockup.
Use case is for testing of the GPIO uAPI, specifically the pull up/down
flags.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 drivers/gpio/gpio-mockup.c | 94 ++++++++++++++++++++++++--------------
 1 file changed, 60 insertions(+), 34 deletions(-)

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index 213aedc97dc2..c28219962ae2 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -146,6 +146,61 @@ static void gpio_mockup_set_multiple(struct gpio_chip *gc,
 	mutex_unlock(&chip->lock);
 }
 
+static int gpio_mockup_apply_pull(struct gpio_mockup_chip *chip,
+				  unsigned int offset, int value)
+{
+	struct gpio_desc *desc;
+	struct gpio_chip *gc;
+	struct irq_sim *sim;
+	int curr, irq, irq_type;
+
+	gc = &chip->gc;
+	desc = &gc->gpiodev->descs[offset];
+	sim = &chip->irqsim;
+
+	mutex_lock(&chip->lock);
+
+	if (test_bit(FLAG_REQUESTED, &desc->flags) &&
+		!test_bit(FLAG_IS_OUT, &desc->flags)) {
+		curr = __gpio_mockup_get(chip, offset);
+		if (curr == value)
+			goto out;
+
+		irq = irq_sim_irqnum(sim, offset);
+		irq_type = irq_get_trigger_type(irq);
+
+		if ((value == 1 && (irq_type & IRQ_TYPE_EDGE_RISING)) ||
+			(value == 0 && (irq_type & IRQ_TYPE_EDGE_FALLING)))
+			irq_sim_fire(sim, offset);
+	}
+
+	/* Change the value unless we're actively driving the line. */
+	if (!test_bit(FLAG_REQUESTED, &desc->flags) ||
+		!test_bit(FLAG_IS_OUT, &desc->flags))
+		__gpio_mockup_set(chip, offset, value);
+
+out:
+	chip->lines[offset].pull = value;
+	mutex_unlock(&chip->lock);
+	return 0;
+}
+
+static int gpio_mockup_set_config(struct gpio_chip *gc,
+				  unsigned int offset, unsigned long config)
+{
+	struct gpio_mockup_chip *chip = gpiochip_get_data(gc);
+
+	switch (pinconf_to_config_param(config)) {
+	case PIN_CONFIG_BIAS_PULL_UP:
+		return gpio_mockup_apply_pull(chip, offset, 1);
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+		return gpio_mockup_apply_pull(chip, offset, 0);
+	default:
+		break;
+	}
+	return -ENOTSUPP;
+}
+
 static int gpio_mockup_dirout(struct gpio_chip *gc,
 			      unsigned int offset, int value)
 {
@@ -226,12 +281,8 @@ static ssize_t gpio_mockup_debugfs_write(struct file *file,
 					 size_t size, loff_t *ppos)
 {
 	struct gpio_mockup_dbgfs_private *priv;
-	int rv, val, curr, irq, irq_type;
-	struct gpio_mockup_chip *chip;
+	int rv, val;
 	struct seq_file *sfile;
-	struct gpio_desc *desc;
-	struct gpio_chip *gc;
-	struct irq_sim *sim;
 
 	if (*ppos != 0)
 		return -EINVAL;
@@ -244,35 +295,9 @@ static ssize_t gpio_mockup_debugfs_write(struct file *file,
 
 	sfile = file->private_data;
 	priv = sfile->private;
-	chip = priv->chip;
-	gc = &chip->gc;
-	desc = &gc->gpiodev->descs[priv->offset];
-	sim = &chip->irqsim;
-
-	mutex_lock(&chip->lock);
-
-	if (test_bit(FLAG_REQUESTED, &desc->flags) &&
-	    !test_bit(FLAG_IS_OUT, &desc->flags)) {
-		curr = __gpio_mockup_get(chip, priv->offset);
-		if (curr == val)
-			goto out;
-
-		irq = irq_sim_irqnum(sim, priv->offset);
-		irq_type = irq_get_trigger_type(irq);
-
-		if ((val == 1 && (irq_type & IRQ_TYPE_EDGE_RISING)) ||
-		    (val == 0 && (irq_type & IRQ_TYPE_EDGE_FALLING)))
-			irq_sim_fire(sim, priv->offset);
-	}
-
-	/* Change the value unless we're actively driving the line. */
-	if (!test_bit(FLAG_REQUESTED, &desc->flags) ||
-	    !test_bit(FLAG_IS_OUT, &desc->flags))
-		__gpio_mockup_set(chip, priv->offset, val);
-
-out:
-	chip->lines[priv->offset].pull = val;
-	mutex_unlock(&chip->lock);
+	rv = gpio_mockup_apply_pull(priv->chip, priv->offset, val);
+	if (rv)
+		return rv;
 
 	return size;
 }
@@ -418,6 +443,7 @@ static int gpio_mockup_probe(struct platform_device *pdev)
 	gc->direction_output = gpio_mockup_dirout;
 	gc->direction_input = gpio_mockup_dirin;
 	gc->get_direction = gpio_mockup_get_direction;
+	gc->set_config = gpio_mockup_set_config;
 	gc->to_irq = gpio_mockup_to_irq;
 	gc->free = gpio_mockup_free;
 
-- 
2.23.0

[PATCH v6 6/7] gpiolib: move validation of line handle flags into helper function

[Kent Gibson]

Move validation of line handle flags into helper function.
This reduces the size and complexity of linehandle_create and allows the
validation to be reused elsewhere.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 drivers/gpio/gpiolib.c | 93 +++++++++++++++++++++++-------------------
 1 file changed, 51 insertions(+), 42 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index f852ff60ae6e..8d6e5dd3d1cf 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -428,6 +428,54 @@ struct linehandle_state {
 	GPIOHANDLE_REQUEST_OPEN_DRAIN | \
 	GPIOHANDLE_REQUEST_OPEN_SOURCE)
 
+static int linehandle_validate_flags(u32 flags)
+{
+	/* Return an error if an unknown flag is set */
+	if (flags & ~GPIOHANDLE_REQUEST_VALID_FLAGS)
+		return -EINVAL;
+
+	/*
+	 * Do not allow both INPUT & OUTPUT flags to be set as they are
+	 * contradictory.
+	 */
+	if ((flags & GPIOHANDLE_REQUEST_INPUT) &&
+	    (flags & GPIOHANDLE_REQUEST_OUTPUT))
+		return -EINVAL;
+
+	/*
+	 * Do not allow OPEN_SOURCE & OPEN_DRAIN flags in a single request. If
+	 * the hardware actually supports enabling both at the same time the
+	 * electrical result would be disastrous.
+	 */
+	if ((flags & GPIOHANDLE_REQUEST_OPEN_DRAIN) &&
+	    (flags & GPIOHANDLE_REQUEST_OPEN_SOURCE))
+		return -EINVAL;
+
+	/* OPEN_DRAIN and OPEN_SOURCE flags only make sense for output mode. */
+	if (!(flags & GPIOHANDLE_REQUEST_OUTPUT) &&
+	    ((flags & GPIOHANDLE_REQUEST_OPEN_DRAIN) ||
+	     (flags & GPIOHANDLE_REQUEST_OPEN_SOURCE)))
+		return -EINVAL;
+
+	/* Bias flags only allowed for input or output mode. */
+	if (!((flags & GPIOHANDLE_REQUEST_INPUT) ||
+	      (flags & GPIOHANDLE_REQUEST_OUTPUT)) &&
+	    ((flags & GPIOHANDLE_REQUEST_BIAS_DISABLE) ||
+	     (flags & GPIOHANDLE_REQUEST_BIAS_PULL_UP) ||
+	     (flags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN)))
+		return -EINVAL;
+
+	/* Only one bias flag can be set. */
+	if (((flags & GPIOHANDLE_REQUEST_BIAS_DISABLE) &&
+	     (flags & (GPIOHANDLE_REQUEST_BIAS_PULL_DOWN |
+			GPIOHANDLE_REQUEST_BIAS_PULL_UP))) ||
+	    ((flags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN) &&
+	     (flags & GPIOHANDLE_REQUEST_BIAS_PULL_UP)))
+		return -EINVAL;
+
+	return 0;
+}
+
 static long linehandle_ioctl(struct file *filep, unsigned int cmd,
 			     unsigned long arg)
 {
@@ -529,48 +577,9 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 
 	lflags = handlereq.flags;
 
-	/* Return an error if an unknown flag is set */
-	if (lflags & ~GPIOHANDLE_REQUEST_VALID_FLAGS)
-		return -EINVAL;
-
-	/*
-	 * Do not allow both INPUT & OUTPUT flags to be set as they are
-	 * contradictory.
-	 */
-	if ((lflags & GPIOHANDLE_REQUEST_INPUT) &&
-	    (lflags & GPIOHANDLE_REQUEST_OUTPUT))
-		return -EINVAL;
-
-	/*
-	 * Do not allow OPEN_SOURCE & OPEN_DRAIN flags in a single request. If
-	 * the hardware actually supports enabling both at the same time the
-	 * electrical result would be disastrous.
-	 */
-	if ((lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN) &&
-	    (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE))
-		return -EINVAL;
-
-	/* OPEN_DRAIN and OPEN_SOURCE flags only make sense for output mode. */
-	if (!(lflags & GPIOHANDLE_REQUEST_OUTPUT) &&
-	    ((lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN) ||
-	     (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE)))
-		return -EINVAL;
-
-	/* Bias flags only allowed for input or output mode. */
-	if (!((lflags & GPIOHANDLE_REQUEST_INPUT) ||
-	      (lflags & GPIOHANDLE_REQUEST_OUTPUT)) &&
-	    ((lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE) ||
-	     (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP) ||
-	     (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN)))
-		return -EINVAL;
-
-	/* Only one bias flag can be set. */
-	if (((lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE) &&
-	     (lflags & (GPIOHANDLE_REQUEST_BIAS_PULL_DOWN |
-			GPIOHANDLE_REQUEST_BIAS_PULL_UP))) ||
-	    ((lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN) &&
-	     (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP)))
-		return -EINVAL;
+	ret = linehandle_validate_flags(lflags);
+	if (ret)
+		return ret;
 
 	lh = kzalloc(sizeof(*lh), GFP_KERNEL);
 	if (!lh)
-- 
2.23.0

[PATCH v6 7/7] gpio: add new SET_CONFIG ioctl() to gpio chardev

[Kent Gibson]

Add the GPIOHANDLE_SET_CONFIG_IOCTL to the gpio chardev.
The ioctl allows some of the configuration of a requested handle to be
changed without having to release the line.
The primary use case is the changing of direction for bi-directional
lines.

Based on initial work by Bartosz Golaszewski <bgolaszewski@baylibre.com>

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 drivers/gpio/gpiolib.c    | 69 +++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/gpio.h | 18 ++++++++++
 2 files changed, 87 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 8d6e5dd3d1cf..f46aa763ee6f 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -476,6 +476,73 @@ static int linehandle_validate_flags(u32 flags)
 	return 0;
 }
 
+static void linehandle_configure_flag(unsigned long *flagsp,
+				      u32 bit, bool active)
+{
+	if (active)
+		set_bit(bit, flagsp);
+	else
+		clear_bit(bit, flagsp);
+}
+
+static long linehandle_set_config(struct linehandle_state *lh,
+				  void __user *ip)
+{
+	struct gpiohandle_config gcnf;
+	struct gpio_desc *desc;
+	int i, ret;
+	u32 lflags;
+	unsigned long *flagsp;
+
+	if (copy_from_user(&gcnf, ip, sizeof(gcnf)))
+		return -EFAULT;
+
+	lflags = gcnf.flags;
+	ret = linehandle_validate_flags(lflags);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < lh->numdescs; i++) {
+		desc = lh->descs[i];
+		flagsp = &desc->flags;
+
+		linehandle_configure_flag(flagsp, FLAG_ACTIVE_LOW,
+			lflags & GPIOHANDLE_REQUEST_ACTIVE_LOW);
+
+		linehandle_configure_flag(flagsp, FLAG_OPEN_DRAIN,
+			lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN);
+
+		linehandle_configure_flag(flagsp, FLAG_OPEN_SOURCE,
+			lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE);
+
+		linehandle_configure_flag(flagsp, FLAG_PULL_UP,
+			lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP);
+
+		linehandle_configure_flag(flagsp, FLAG_PULL_DOWN,
+			lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN);
+
+		linehandle_configure_flag(flagsp, FLAG_BIAS_DISABLE,
+			lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE);
+
+		/*
+		 * Lines have to be requested explicitly for input
+		 * or output, else the line will be treated "as is".
+		 */
+		if (lflags & GPIOHANDLE_REQUEST_OUTPUT) {
+			int val = !!gcnf.default_values[i];
+
+			ret = gpiod_direction_output(desc, val);
+			if (ret)
+				return ret;
+		} else if (lflags & GPIOHANDLE_REQUEST_INPUT) {
+			ret = gpiod_direction_input(desc);
+			if (ret)
+				return ret;
+		}
+	}
+	return 0;
+}
+
 static long linehandle_ioctl(struct file *filep, unsigned int cmd,
 			     unsigned long arg)
 {
@@ -526,6 +593,8 @@ static long linehandle_ioctl(struct file *filep, unsigned int cmd,
 					      lh->descs,
 					      NULL,
 					      vals);
+	} else if (cmd == GPIOHANDLE_SET_CONFIG_IOCTL) {
+		return linehandle_set_config(lh, ip);
 	}
 	return -EINVAL;
 }
diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
index 7cc21c3b0839..799cf823d493 100644
--- a/include/uapi/linux/gpio.h
+++ b/include/uapi/linux/gpio.h
@@ -100,6 +100,24 @@ struct gpiohandle_request {
 	int fd;
 };
 
+/**
+ * struct gpiohandle_config - Configuration for a GPIO handle request
+ * @flags: updated flags for the requested GPIO lines, such as
+ * GPIOHANDLE_REQUEST_OUTPUT, GPIOHANDLE_REQUEST_ACTIVE_LOW etc, OR:ed
+ * together
+ * @default_values: if the GPIOHANDLE_REQUEST_OUTPUT is set in flags,
+ * this specifies the default output value, should be 0 (low) or
+ * 1 (high), anything else than 0 or 1 will be interpreted as 1 (high)
+ * @padding: reserved for future use and should be zero filled
+ */
+struct gpiohandle_config {
+	__u32 flags;
+	__u8 default_values[GPIOHANDLES_MAX];
+	__u32 padding[4]; /* padding for future use */
+};
+
+#define GPIOHANDLE_SET_CONFIG_IOCTL _IOWR(0xB4, 0x0a, struct gpiohandle_config)
+
 /**
  * struct gpiohandle_data - Information of values on a GPIO handle
  * @values: when getting the state of lines this contains the current
-- 
2.23.0

Re: [PATCH v6 0/7] gpio: expose line bias flags to userspace

[Bartosz Golaszewski]

wt., 5 lis 2019 o 03:04 Kent Gibson <warthog618@gmail.com> napisał(a):
>
> The changes from v5:
>   - rebased onto Bart's gpio/for-kent branch.
>
> The changes from v4:
> Review changes:
>  - relocate restriction on applying bias to as-is from patch 2 to patch 1.
>  - propagate errors, other than ENOTSUPP, from gpio_set_bias. (squashed
>    into patches 3 and 4).
>  - include SET_CONFIG patch series v2 (patch 6 and 7 here).
>
> I've also fixed a few other nits I noticed along the way:
>  - rework gpio_set_bias as flags are mutually exclusive.
>  - remove input flag required to set bias restriction from
>    lineevent_create as events are implicitly assumed inputs anyway.
>  - reorder patches to group gpiolib bias patches together before the
>    gpio-mockup changes.
>
>
> This series adds gross control of pull-up/pull-down to the GPIO uAPI.
> Gross control means enabling and disabling of bias functionality,
> not finer grained control such as setting biasing impedances.
>
> The support allows both input and output lines to have any one of the
> following biases applied as part of the line handle or event request:
>  0. As Is - bias is left alone.  This is the default for ABI compatibility.
>  1. Bias Disable - bias is explicitly disabled.
>  2. Pull Down - pull-down bias is enabled.
>  3. Pull Up - pull-up bias is enabled.
>
> The biases are set via three flags, BIAS_DISABLE, BIAS_PULL_DOWN
> and BIAS_PULL_UP.  These map directly to the similarly named
> pinctrl pin_config_param flags.
> As Is corresponds to none of the flags being set.
>
> The setting of biases on output lines may seem odd, but is to allow for
> utilisation of internal pull-up/pull-down on open drain and open source
> outputs, where supported in hardware.
>
> The series also adds the GPIOHANDLE_SET_CONFIG_IOCTL to the gpio chardev.
> The ioctl allows some of the configuration of a requested handle to be
> changed without having to release the line.
> The primary use case is the changing of direction for bi-directional
> lines.
>
> Patches are against Bart's gpio/for-kent branch[1].
>
> The patch has been successfully tested against gpio-mockup, and
> on a Raspberry Pi, in both cases using the feature/pud_set_config
> branch of my Go gpiod library[2], as well as with my feature/pud
> development branch of libgpiod[3].  Patch 7 has only been tested using
> my gpiod library as libgpiod has not yet been updated to support the
> SET_CONFIG ioctl.
>
> Patch 1 adds pull-up/pull-down support to line handle requests.
> Patch 2 adds pull-up/pull-down support to line event requests.
> Patch 3 adds support for disabling bias.
> Patch 4 adds support for setting bias on output lines.
> Patch 5 adds pull-up/down support to the gpio-mockup for uAPI testing.
> Patch 6 refactors the flag validation from linehandle_create.
> Patch 7 adds the SET_CONFIG ioctl.
>
> Drew Fustini (1):
>   gpio: expose pull-up/pull-down line flags to userspace
>
> Kent Gibson (6):
>   gpiolib: add support for pull up/down to lineevent_create
>   gpiolib: add support for disabling line bias
>   gpiolib: add support for biasing output lines
>   gpio: mockup: add set_config to support pull up/down
>   gpiolib: move validation of line handle flags into helper function
>   gpio: add new SET_CONFIG ioctl() to gpio chardev
>
>  drivers/gpio/gpio-mockup.c |  94 ++++++++++------
>  drivers/gpio/gpiolib.c     | 213 +++++++++++++++++++++++++++++++------
>  drivers/gpio/gpiolib.h     |   1 +
>  include/uapi/linux/gpio.h  |  24 +++++
>  4 files changed, 264 insertions(+), 68 deletions(-)
>
> --
> 2.23.0
>

Ok, all looks good now and since Linus already said he's fine with
this series, I'll take it through my tree for v5.5.

Thanks!
Bartosz

Re: [PATCH v6 0/7] gpio: expose line bias flags to userspace

[Kent Gibson]

On Tue, Nov 05, 2019 at 10:04:22AM +0800, Kent Gibson wrote:
> Patches are against Bart's gpio/for-kent branch[1].
> 
> The patch has been successfully tested against gpio-mockup, and 
> on a Raspberry Pi, in both cases using the feature/pud_set_config 
> branch of my Go gpiod library[2], as well as with my feature/pud 
> development branch of libgpiod[3].  Patch 7 has only been tested using 
> my gpiod library as libgpiod has not yet been updated to support the 
> SET_CONFIG ioctl.
> 

I've just pushed a first pass at SET_CONFIG support into my libgpiod 
feature/pud branch.  It is causing me a bit of grief.  Due to the way
the libgpiod API is structured, with the direction flags pulled out into 
the request type, I thought it would be cleaner to keep changes to direction 
orthogonal to changes to the other handle flags.

So I've added these methods to the API:

int gpiod_line_set_config(struct gpiod_line *line, int flags)
int gpiod_line_set_direction_input(struct gpiod_line *line)
int gpiod_line_set_direction_output(struct gpiod_line *line,
				    int value)

along with their bulk equivalents.

I've coded that and started adding tests when I tripped over changing
bias.  The kernel requires a direction to be set, but I'm setting it
as-is in gpiod_line_set_config - so that wont work.
Open drain/source are in the same boat - they require output mode.

I see these options:
 1. set the direction as part of gpiod_line_set_config
 2. relax the kernel restriction.
 3. don't support changing bias or open source/drain.
 4. rethink the API.

The first option requires caching the value set for outputs which I'm a
bit hesitant to do, though I'm not sure why - I've already added caching
of the handle flags for the direction functions.

Any preferences or suggestions?

Cheers,
Kent.

Re: [PATCH v6 0/7] gpio: expose line bias flags to userspace

[Bartosz Golaszewski]

wt., 5 lis 2019 o 16:26 Kent Gibson <warthog618@gmail.com> napisał(a):
>
> On Tue, Nov 05, 2019 at 10:04:22AM +0800, Kent Gibson wrote:
> > Patches are against Bart's gpio/for-kent branch[1].
> >
> > The patch has been successfully tested against gpio-mockup, and
> > on a Raspberry Pi, in both cases using the feature/pud_set_config
> > branch of my Go gpiod library[2], as well as with my feature/pud
> > development branch of libgpiod[3].  Patch 7 has only been tested using
> > my gpiod library as libgpiod has not yet been updated to support the
> > SET_CONFIG ioctl.
> >
>
> I've just pushed a first pass at SET_CONFIG support into my libgpiod
> feature/pud branch.  It is causing me a bit of grief.  Due to the way
> the libgpiod API is structured, with the direction flags pulled out into
> the request type, I thought it would be cleaner to keep changes to direction
> orthogonal to changes to the other handle flags.
>

I'd love to see that branch - is it public?

> So I've added these methods to the API:
>
> int gpiod_line_set_config(struct gpiod_line *line, int flags)
> int gpiod_line_set_direction_input(struct gpiod_line *line)
> int gpiod_line_set_direction_output(struct gpiod_line *line,
>                                     int value)
>
> along with their bulk equivalents.
>
> I've coded that and started adding tests when I tripped over changing
> bias.  The kernel requires a direction to be set, but I'm setting it
> as-is in gpiod_line_set_config - so that wont work.
> Open drain/source are in the same boat - they require output mode.
>

Ha! Yes this is a problem - how about this:

If the caller of set_config in the kernel doesn't pass any of the
direction flags, then we read the current direction, set the right
flag in lflags and only then call the function validating the flags?

> I see these options:
>  1. set the direction as part of gpiod_line_set_config
>  2. relax the kernel restriction.

Yes, I don't think we should force users to always pass the direction
flag in set_config. Good point.

>  3. don't support changing bias or open source/drain.

No! We definitely want to support it in libgpiod.

>  4. rethink the API.

As for libgpiod: I think we should have a low-level
gpiod_line_set_config() that would set both the direction and other
flags (it could for instance only accept two request flags: input and
output) and then a higher-level set_flags(), set_direction_input(),
set_direction_output() that would call the low-level variant and - for
set_flags() without the direction argument - it could simply retrieve
the current direction and pass it to gpiod_line_set_config().

But this is only a vague idea - I'd have to actually start looking at
the code to be sure. I'd love to see what you came up with so far
though!

Bart

>
> The first option requires caching the value set for outputs which I'm a
> bit hesitant to do, though I'm not sure why - I've already added caching
> of the handle flags for the direction functions.
>
> Any preferences or suggestions?
>
> Cheers,
> Kent.

Re: [PATCH v6 0/7] gpio: expose line bias flags to userspace

[Bartosz Golaszewski]

wt., 5 lis 2019 o 17:24 Bartosz Golaszewski
<bgolaszewski@baylibre.com> napisał(a):
>
> wt., 5 lis 2019 o 16:26 Kent Gibson <warthog618@gmail.com> napisał(a):
> >
> > On Tue, Nov 05, 2019 at 10:04:22AM +0800, Kent Gibson wrote:
> > > Patches are against Bart's gpio/for-kent branch[1].
> > >
> > > The patch has been successfully tested against gpio-mockup, and
> > > on a Raspberry Pi, in both cases using the feature/pud_set_config
> > > branch of my Go gpiod library[2], as well as with my feature/pud
> > > development branch of libgpiod[3].  Patch 7 has only been tested using
> > > my gpiod library as libgpiod has not yet been updated to support the
> > > SET_CONFIG ioctl.
> > >
> >
> > I've just pushed a first pass at SET_CONFIG support into my libgpiod
> > feature/pud branch.  It is causing me a bit of grief.  Due to the way
> > the libgpiod API is structured, with the direction flags pulled out into
> > the request type, I thought it would be cleaner to keep changes to direction
> > orthogonal to changes to the other handle flags.
> >
>
> I'd love to see that branch - is it public?
>
> > So I've added these methods to the API:
> >
> > int gpiod_line_set_config(struct gpiod_line *line, int flags)
> > int gpiod_line_set_direction_input(struct gpiod_line *line)
> > int gpiod_line_set_direction_output(struct gpiod_line *line,
> >                                     int value)
> >
> > along with their bulk equivalents.
> >
> > I've coded that and started adding tests when I tripped over changing
> > bias.  The kernel requires a direction to be set, but I'm setting it
> > as-is in gpiod_line_set_config - so that wont work.
> > Open drain/source are in the same boat - they require output mode.
> >
>
> Ha! Yes this is a problem - how about this:
>
> If the caller of set_config in the kernel doesn't pass any of the
> direction flags, then we read the current direction, set the right
> flag in lflags and only then call the function validating the flags?
>

After another thought: this would be a bit inconsistent with the rest
of the flags. IIRC this was the reason for me to split the
request_type and other flags into two separate fields in libgpiod in
the first place.

When I think about it: the kernel behavior should be as predictable as
possible - if we keep the behavior as is in v6, I don't see why we
couldn't make userspace cache (or re-read) the current direction when
setting flags other than direction? Do you see any trouble in that?
That way we'd avoid having the kernel treat different flags in
different way.

Bart

> > I see these options:
> >  1. set the direction as part of gpiod_line_set_config
> >  2. relax the kernel restriction.
>
> Yes, I don't think we should force users to always pass the direction
> flag in set_config. Good point.
>
> >  3. don't support changing bias or open source/drain.
>
> No! We definitely want to support it in libgpiod.
>
> >  4. rethink the API.
>
> As for libgpiod: I think we should have a low-level
> gpiod_line_set_config() that would set both the direction and other
> flags (it could for instance only accept two request flags: input and
> output) and then a higher-level set_flags(), set_direction_input(),
> set_direction_output() that would call the low-level variant and - for
> set_flags() without the direction argument - it could simply retrieve
> the current direction and pass it to gpiod_line_set_config().
>
> But this is only a vague idea - I'd have to actually start looking at
> the code to be sure. I'd love to see what you came up with so far
> though!
>
> Bart
>
> >
> > The first option requires caching the value set for outputs which I'm a
> > bit hesitant to do, though I'm not sure why - I've already added caching
> > of the handle flags for the direction functions.
> >
> > Any preferences or suggestions?
> >
> > Cheers,
> > Kent.

Re: [PATCH v6 0/7] gpio: expose line bias flags to userspace

[Kent Gibson]

On Tue, Nov 05, 2019 at 05:24:44PM +0100, Bartosz Golaszewski wrote:
> wt., 5 lis 2019 o 16:26 Kent Gibson <warthog618@gmail.com> napisał(a):
> >
> > On Tue, Nov 05, 2019 at 10:04:22AM +0800, Kent Gibson wrote:
> > > Patches are against Bart's gpio/for-kent branch[1].
> > >
> > > The patch has been successfully tested against gpio-mockup, and
> > > on a Raspberry Pi, in both cases using the feature/pud_set_config
> > > branch of my Go gpiod library[2], as well as with my feature/pud
> > > development branch of libgpiod[3].  Patch 7 has only been tested using
> > > my gpiod library as libgpiod has not yet been updated to support the
> > > SET_CONFIG ioctl.
> > >
> >
> > I've just pushed a first pass at SET_CONFIG support into my libgpiod
> > feature/pud branch.  It is causing me a bit of grief.  Due to the way
> > the libgpiod API is structured, with the direction flags pulled out into
> > the request type, I thought it would be cleaner to keep changes to direction
> > orthogonal to changes to the other handle flags.
> >
> 
> I'd love to see that branch - is it public?
> 

Sorry - I forgot to add the references to the bottom of the cover note
for v5 and v6.
It should've been:

[1] git://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git
[2] https://github.com/warthog618/gpiod.git
[3] https://github.com/warthog618/libgpiod.git

It is my libgpiod repo on github[3] - in the feature/pud branch.

Cheers,
Kent.

Re: [PATCH v6 0/7] gpio: expose line bias flags to userspace

[Kent Gibson]

On Tue, Nov 05, 2019 at 10:07:58PM +0100, Bartosz Golaszewski wrote:
> wt., 5 lis 2019 o 17:24 Bartosz Golaszewski
> <bgolaszewski@baylibre.com> napisał(a):
> >
> > wt., 5 lis 2019 o 16:26 Kent Gibson <warthog618@gmail.com> napisał(a):
> > >
> > > On Tue, Nov 05, 2019 at 10:04:22AM +0800, Kent Gibson wrote:
> > > > Patches are against Bart's gpio/for-kent branch[1].
> > > >
> > > > The patch has been successfully tested against gpio-mockup, and
> > > > on a Raspberry Pi, in both cases using the feature/pud_set_config
> > > > branch of my Go gpiod library[2], as well as with my feature/pud
> > > > development branch of libgpiod[3].  Patch 7 has only been tested using
> > > > my gpiod library as libgpiod has not yet been updated to support the
> > > > SET_CONFIG ioctl.
> > > >
> > >
> > > I've just pushed a first pass at SET_CONFIG support into my libgpiod
> > > feature/pud branch.  It is causing me a bit of grief.  Due to the way
> > > the libgpiod API is structured, with the direction flags pulled out into
> > > the request type, I thought it would be cleaner to keep changes to direction
> > > orthogonal to changes to the other handle flags.
> > >
> >
> > I'd love to see that branch - is it public?
> >
> > > So I've added these methods to the API:
> > >
> > > int gpiod_line_set_config(struct gpiod_line *line, int flags)
> > > int gpiod_line_set_direction_input(struct gpiod_line *line)
> > > int gpiod_line_set_direction_output(struct gpiod_line *line,
> > >                                     int value)
> > >
> > > along with their bulk equivalents.
> > >
> > > I've coded that and started adding tests when I tripped over changing
> > > bias.  The kernel requires a direction to be set, but I'm setting it
> > > as-is in gpiod_line_set_config - so that wont work.
> > > Open drain/source are in the same boat - they require output mode.
> > >
> >
> > Ha! Yes this is a problem - how about this:
> >
> > If the caller of set_config in the kernel doesn't pass any of the
> > direction flags, then we read the current direction, set the right
> > flag in lflags and only then call the function validating the flags?
> >
> 

I was also thinking along that line - the config would be carried over 
from the initial request and any subsequent SET_CONFIGs.
The kernel could overlay the existing config over any field set 
"as-is" before performing validation.
The validation requirement would stand, but you don't need to pass the 
complete state, possibly including output values, with each SET_CONFIG 
request.

In the bias case above, I create the line as an output and so should 
be able to set the bias, even if neither INPUT nor OUTPUT is set in 
the SET_CONFIG request.

> After another thought: this would be a bit inconsistent with the rest
> of the flags. IIRC this was the reason for me to split the
> request_type and other flags into two separate fields in libgpiod in
> the first place.
> 

It is a bit inconsisent, so how about changing the rest of the flags 
to make them consistent? They need to have an as-is state, which 
corresponds to no flags set, and you then leave that field as is.
We currently have four fields in the handle flags - direction, active
state, drive, and bias.  Of those, direction and bias have as-is states.
What we are missing are additional flags so we have an as-is state for 
active state and drive.  

Currently: 
    ACTIVE_LOW == 0 => ACTIVE_HIGH, and 
    OPEN_DRAIN == 0 && OPEN_SOURCE == 0 => PUSH_PULL.

If we added an ACTIVE_HIGH flag, to counter ACTIVE_LOW, and PUSH_PULL 
to counter OPEN_DRAIN/OPEN_SOURCE, then we can have SET_CONFIG change 
the four fields (direction, active state, drive and bias), independently,
or not, as the caller sees fit.

For backward compatibility, the lines would be created with ACTIVE_HIGH 
and PUSH_PULL set, should they be requested "as-is", by the new 
definition.

This feels like the right solution to me - as I write this anyway.
The biggest downside I can see is that it means pulling v6, or at least
the SET_CONFIG patches, pending an update.

> When I think about it: the kernel behavior should be as predictable as
> possible - if we keep the behavior as is in v6, I don't see why we
> couldn't make userspace cache (or re-read) the current direction when
> setting flags other than direction? Do you see any trouble in that?
> That way we'd avoid having the kernel treat different flags in
> different way.
> 

If in userspace then it will have to be cached - the kernel still has 
issues reading back output values for emulated open drain/source outputs.  
Fixing that is somewhere down my todo list.

I can't think of any concrete problems with caching.
It gives me "I have a bad feeling about this" vibe, but I can't pin
down why.  Maybe wanting to avoid shadowing kernel state in userspace?

But, as above, I'd rather fix the flags so we have as-is for all, and
caching becomes unnecessary.

> Bart
> 
> > > I see these options:
> > >  1. set the direction as part of gpiod_line_set_config
> > >  2. relax the kernel restriction.
> >
> > Yes, I don't think we should force users to always pass the direction
> > flag in set_config. Good point.
> >
> > >  3. don't support changing bias or open source/drain.
> >
> > No! We definitely want to support it in libgpiod.

Agreed.

> >
> > >  4. rethink the API.
> >
> > As for libgpiod: I think we should have a low-level
> > gpiod_line_set_config() that would set both the direction and other
> > flags (it could for instance only accept two request flags: input and
> > output) and then a higher-level set_flags(), set_direction_input(),
> > set_direction_output() that would call the low-level variant and - for
> > set_flags() without the direction argument - it could simply retrieve
> > the current direction and pass it to gpiod_line_set_config().
> >

I agree that there should add be a fully capable low level option.

The low level function would look have a signature like this:

int gpiod_line_set_config(struct gpiod_line *line, int direction, int flags,
    const int *default_vals)

The existing gpiod_line_set_config would be renamed gpiod_line_set_flags.

> > But this is only a vague idea - I'd have to actually start looking at
> > the code to be sure. I'd love to see what you came up with so far
> > though!
> >

Indeed - what I had in mind changed radically once I had a closer look 
at the libgpiod API.  And it is still changing.

Cheers,
Kent.

Re: [PATCH v6 0/7] gpio: expose line bias flags to userspace

[Bartosz Golaszewski]

śr., 6 lis 2019 o 07:48 Kent Gibson <warthog618@gmail.com> napisał(a):
>
> On Tue, Nov 05, 2019 at 10:07:58PM +0100, Bartosz Golaszewski wrote:
> > wt., 5 lis 2019 o 17:24 Bartosz Golaszewski
> > <bgolaszewski@baylibre.com> napisał(a):
> > >
> > > wt., 5 lis 2019 o 16:26 Kent Gibson <warthog618@gmail.com> napisał(a):
> > > >
> > > > On Tue, Nov 05, 2019 at 10:04:22AM +0800, Kent Gibson wrote:
> > > > > Patches are against Bart's gpio/for-kent branch[1].
> > > > >
> > > > > The patch has been successfully tested against gpio-mockup, and
> > > > > on a Raspberry Pi, in both cases using the feature/pud_set_config
> > > > > branch of my Go gpiod library[2], as well as with my feature/pud
> > > > > development branch of libgpiod[3].  Patch 7 has only been tested using
> > > > > my gpiod library as libgpiod has not yet been updated to support the
> > > > > SET_CONFIG ioctl.
> > > > >
> > > >
> > > > I've just pushed a first pass at SET_CONFIG support into my libgpiod
> > > > feature/pud branch.  It is causing me a bit of grief.  Due to the way
> > > > the libgpiod API is structured, with the direction flags pulled out into
> > > > the request type, I thought it would be cleaner to keep changes to direction
> > > > orthogonal to changes to the other handle flags.
> > > >
> > >
> > > I'd love to see that branch - is it public?
> > >
> > > > So I've added these methods to the API:
> > > >
> > > > int gpiod_line_set_config(struct gpiod_line *line, int flags)
> > > > int gpiod_line_set_direction_input(struct gpiod_line *line)
> > > > int gpiod_line_set_direction_output(struct gpiod_line *line,
> > > >                                     int value)
> > > >
> > > > along with their bulk equivalents.
> > > >
> > > > I've coded that and started adding tests when I tripped over changing
> > > > bias.  The kernel requires a direction to be set, but I'm setting it
> > > > as-is in gpiod_line_set_config - so that wont work.
> > > > Open drain/source are in the same boat - they require output mode.
> > > >
> > >
> > > Ha! Yes this is a problem - how about this:
> > >
> > > If the caller of set_config in the kernel doesn't pass any of the
> > > direction flags, then we read the current direction, set the right
> > > flag in lflags and only then call the function validating the flags?
> > >
> >
>

Cc'ing Linus, I'm not sure when he was dropped from this discussion.

> I was also thinking along that line - the config would be carried over
> from the initial request and any subsequent SET_CONFIGs.
> The kernel could overlay the existing config over any field set
> "as-is" before performing validation.
> The validation requirement would stand, but you don't need to pass the
> complete state, possibly including output values, with each SET_CONFIG
> request.
>
> In the bias case above, I create the line as an output and so should
> be able to set the bias, even if neither INPUT nor OUTPUT is set in
> the SET_CONFIG request.
>
> > After another thought: this would be a bit inconsistent with the rest
> > of the flags. IIRC this was the reason for me to split the
> > request_type and other flags into two separate fields in libgpiod in
> > the first place.
> >
>
> It is a bit inconsisent, so how about changing the rest of the flags
> to make them consistent? They need to have an as-is state, which
> corresponds to no flags set, and you then leave that field as is.
> We currently have four fields in the handle flags - direction, active
> state, drive, and bias.  Of those, direction and bias have as-is states.
> What we are missing are additional flags so we have an as-is state for
> active state and drive.
>
> Currently:
>     ACTIVE_LOW == 0 => ACTIVE_HIGH, and
>     OPEN_DRAIN == 0 && OPEN_SOURCE == 0 => PUSH_PULL.
>
> If we added an ACTIVE_HIGH flag, to counter ACTIVE_LOW, and PUSH_PULL
> to counter OPEN_DRAIN/OPEN_SOURCE, then we can have SET_CONFIG change
> the four fields (direction, active state, drive and bias), independently,
> or not, as the caller sees fit.
>
> For backward compatibility, the lines would be created with ACTIVE_HIGH
> and PUSH_PULL set, should they be requested "as-is", by the new
> definition.
>

I'm not in favor of having the same behavior triggered in two
different ways: one explicit and one implicit. This API got released
and we have to live with it, I'm afraid. We could for instance add a
comment to the uAPI header though. I also don't think new AS_IS flags
are necessary. We can live fine with certain inconveniences in the
ioctl() API as long as user-space libraries make up for it by
structuring these calls differently.

To summarize: I'd prefer to make the SET_CONFIG ioctl() require
specifying the direction and then simply caching it in user-space.

> This feels like the right solution to me - as I write this anyway.
> The biggest downside I can see is that it means pulling v6, or at least
> the SET_CONFIG patches, pending an update.
>
> > When I think about it: the kernel behavior should be as predictable as
> > possible - if we keep the behavior as is in v6, I don't see why we
> > couldn't make userspace cache (or re-read) the current direction when
> > setting flags other than direction? Do you see any trouble in that?
> > That way we'd avoid having the kernel treat different flags in
> > different way.
> >
>
> If in userspace then it will have to be cached - the kernel still has
> issues reading back output values for emulated open drain/source outputs.
> Fixing that is somewhere down my todo list.
>

Right, but we've lived with problems in this area for a long time and
nobody complained - maybe it can wait just a bit more. :)

> I can't think of any concrete problems with caching.
> It gives me "I have a bad feeling about this" vibe, but I can't pin
> down why.  Maybe wanting to avoid shadowing kernel state in userspace?
>

The user-space can always read back the current state with the
lineinfo ioctl(), right? Any problems with that?

> But, as above, I'd rather fix the flags so we have as-is for all, and
> caching becomes unnecessary.
>

This idea in turn gives me a bad feeling. There's something too
implicit in this behavior for me. Sounds like it's too easy to get it
wrong from user-space.

> > Bart
> >
> > > > I see these options:
> > > >  1. set the direction as part of gpiod_line_set_config
> > > >  2. relax the kernel restriction.
> > >
> > > Yes, I don't think we should force users to always pass the direction
> > > flag in set_config. Good point.
> > >
> > > >  3. don't support changing bias or open source/drain.
> > >
> > > No! We definitely want to support it in libgpiod.
>
> Agreed.
>
> > >
> > > >  4. rethink the API.
> > >
> > > As for libgpiod: I think we should have a low-level
> > > gpiod_line_set_config() that would set both the direction and other
> > > flags (it could for instance only accept two request flags: input and
> > > output) and then a higher-level set_flags(), set_direction_input(),
> > > set_direction_output() that would call the low-level variant and - for
> > > set_flags() without the direction argument - it could simply retrieve
> > > the current direction and pass it to gpiod_line_set_config().
> > >
>
> I agree that there should add be a fully capable low level option.
>
> The low level function would look have a signature like this:
>
> int gpiod_line_set_config(struct gpiod_line *line, int direction, int flags,
>     const int *default_vals)
>
> The existing gpiod_line_set_config would be renamed gpiod_line_set_flags.
>
> > > But this is only a vague idea - I'd have to actually start looking at
> > > the code to be sure. I'd love to see what you came up with so far
> > > though!
> > >
>
> Indeed - what I had in mind changed radically once I had a closer look
> at the libgpiod API.  And it is still changing.
>

Thanks for doing it! It's also great you included some test cases!

Bart

> Cheers,
> Kent.
>

Re: [PATCH v6 0/7] gpio: expose line bias flags to userspace

[Kent Gibson]

On Wed, Nov 06, 2019 at 02:59:33PM +0100, Bartosz Golaszewski wrote:
> śr., 6 lis 2019 o 07:48 Kent Gibson <warthog618@gmail.com> napisał(a):
> >
> > On Tue, Nov 05, 2019 at 10:07:58PM +0100, Bartosz Golaszewski wrote:
> > > wt., 5 lis 2019 o 17:24 Bartosz Golaszewski
> > > <bgolaszewski@baylibre.com> napisał(a):
> > > >
> > > > wt., 5 lis 2019 o 16:26 Kent Gibson <warthog618@gmail.com> napisał(a):
> > > > >
> > > > > On Tue, Nov 05, 2019 at 10:04:22AM +0800, Kent Gibson wrote:
> > > > > > Patches are against Bart's gpio/for-kent branch[1].
> > > > > >
> > > > > > The patch has been successfully tested against gpio-mockup, and
> > > > > > on a Raspberry Pi, in both cases using the feature/pud_set_config
> > > > > > branch of my Go gpiod library[2], as well as with my feature/pud
> > > > > > development branch of libgpiod[3].  Patch 7 has only been tested using
> > > > > > my gpiod library as libgpiod has not yet been updated to support the
> > > > > > SET_CONFIG ioctl.
> > > > > >
> > > > >
> > > > > I've just pushed a first pass at SET_CONFIG support into my libgpiod
> > > > > feature/pud branch.  It is causing me a bit of grief.  Due to the way
> > > > > the libgpiod API is structured, with the direction flags pulled out into
> > > > > the request type, I thought it would be cleaner to keep changes to direction
> > > > > orthogonal to changes to the other handle flags.
> > > > >
> > > >
> > > > I'd love to see that branch - is it public?
> > > >
> > > > > So I've added these methods to the API:
> > > > >
> > > > > int gpiod_line_set_config(struct gpiod_line *line, int flags)
> > > > > int gpiod_line_set_direction_input(struct gpiod_line *line)
> > > > > int gpiod_line_set_direction_output(struct gpiod_line *line,
> > > > >                                     int value)
> > > > >
> > > > > along with their bulk equivalents.
> > > > >
> > > > > I've coded that and started adding tests when I tripped over changing
> > > > > bias.  The kernel requires a direction to be set, but I'm setting it
> > > > > as-is in gpiod_line_set_config - so that wont work.
> > > > > Open drain/source are in the same boat - they require output mode.
> > > > >
> > > >
> > > > Ha! Yes this is a problem - how about this:
> > > >
> > > > If the caller of set_config in the kernel doesn't pass any of the
> > > > direction flags, then we read the current direction, set the right
> > > > flag in lflags and only then call the function validating the flags?
> > > >
> > >
> >
> 
> Cc'ing Linus, I'm not sure when he was dropped from this discussion.
> 

My bad - I cut it down to you and Drew + list in my initial reply as I 
thought we'd only be talking libgpiod changes.
But then it took a sharp u-turn back to the kernel patch.

Anway I think we are now in agreement that the kernel is fine and we're 
back in libgpiod territory - see below.

> > I was also thinking along that line - the config would be carried over
> > from the initial request and any subsequent SET_CONFIGs.
> > The kernel could overlay the existing config over any field set
> > "as-is" before performing validation.
> > The validation requirement would stand, but you don't need to pass the
> > complete state, possibly including output values, with each SET_CONFIG
> > request.
> >
> > In the bias case above, I create the line as an output and so should
> > be able to set the bias, even if neither INPUT nor OUTPUT is set in
> > the SET_CONFIG request.
> >
> > > After another thought: this would be a bit inconsistent with the rest
> > > of the flags. IIRC this was the reason for me to split the
> > > request_type and other flags into two separate fields in libgpiod in
> > > the first place.
> > >
> >
> > It is a bit inconsisent, so how about changing the rest of the flags
> > to make them consistent? They need to have an as-is state, which
> > corresponds to no flags set, and you then leave that field as is.
> > We currently have four fields in the handle flags - direction, active
> > state, drive, and bias.  Of those, direction and bias have as-is states.
> > What we are missing are additional flags so we have an as-is state for
> > active state and drive.
> >
> > Currently:
> >     ACTIVE_LOW == 0 => ACTIVE_HIGH, and
> >     OPEN_DRAIN == 0 && OPEN_SOURCE == 0 => PUSH_PULL.
> >
> > If we added an ACTIVE_HIGH flag, to counter ACTIVE_LOW, and PUSH_PULL
> > to counter OPEN_DRAIN/OPEN_SOURCE, then we can have SET_CONFIG change
> > the four fields (direction, active state, drive and bias), independently,
> > or not, as the caller sees fit.
> >
> > For backward compatibility, the lines would be created with ACTIVE_HIGH
> > and PUSH_PULL set, should they be requested "as-is", by the new
> > definition.
> >
> 
> I'm not in favor of having the same behavior triggered in two
> different ways: one explicit and one implicit. This API got released
> and we have to live with it, I'm afraid. We could for instance add a
> comment to the uAPI header though. I also don't think new AS_IS flags
> are necessary. We can live fine with certain inconveniences in the
> ioctl() API as long as user-space libraries make up for it by
> structuring these calls differently.
> 
> To summarize: I'd prefer to make the SET_CONFIG ioctl() require
> specifying the direction and then simply caching it in user-space.
> 

Agreed.  I had a quick play with changing the kernel and it was ugly.
Too many conversions between flag layouts and messing with flag masks
and bits.  It was absolutely hideous.
Requiring the userspace to provide the complete config is much simpler.

> > This feels like the right solution to me - as I write this anyway.
> > The biggest downside I can see is that it means pulling v6, or at least
> > the SET_CONFIG patches, pending an update.
> >
> > > When I think about it: the kernel behavior should be as predictable as
> > > possible - if we keep the behavior as is in v6, I don't see why we
> > > couldn't make userspace cache (or re-read) the current direction when
> > > setting flags other than direction? Do you see any trouble in that?
> > > That way we'd avoid having the kernel treat different flags in
> > > different way.
> > >
> >
> > If in userspace then it will have to be cached - the kernel still has
> > issues reading back output values for emulated open drain/source outputs.
> > Fixing that is somewhere down my todo list.
> >
> 
> Right, but we've lived with problems in this area for a long time and
> nobody complained - maybe it can wait just a bit more. :)
> 

Hey - I complained!  I had to skip over some of my tests because of that ;-).

> > I can't think of any concrete problems with caching.
> > It gives me "I have a bad feeling about this" vibe, but I can't pin
> > down why.  Maybe wanting to avoid shadowing kernel state in userspace?
> >
> 
> The user-space can always read back the current state with the
> lineinfo ioctl(), right? Any problems with that?
>

Only that you have to convert from the info flag layout to the request
handle flag layout, or the libgpiod request_type + flag layout.
And I've already had enough of that for one day.

> > But, as above, I'd rather fix the flags so we have as-is for all, and
> > caching becomes unnecessary.
> >
> 
> This idea in turn gives me a bad feeling. There's something too
> implicit in this behavior for me. Sounds like it's too easy to get it
> wrong from user-space.
> 
> > > Bart
> > >
> > > > > I see these options:
> > > > >  1. set the direction as part of gpiod_line_set_config
> > > > >  2. relax the kernel restriction.
> > > >
> > > > Yes, I don't think we should force users to always pass the direction
> > > > flag in set_config. Good point.
> > > >
> > > > >  3. don't support changing bias or open source/drain.
> > > >
> > > > No! We definitely want to support it in libgpiod.
> >
> > Agreed.
> >
> > > >
> > > > >  4. rethink the API.
> > > >
> > > > As for libgpiod: I think we should have a low-level
> > > > gpiod_line_set_config() that would set both the direction and other
> > > > flags (it could for instance only accept two request flags: input and
> > > > output) and then a higher-level set_flags(), set_direction_input(),
> > > > set_direction_output() that would call the low-level variant and - for
> > > > set_flags() without the direction argument - it could simply retrieve
> > > > the current direction and pass it to gpiod_line_set_config().
> > > >
> >
> > I agree that there should add be a fully capable low level option.
> >
> > The low level function would look have a signature like this:
> >
> > int gpiod_line_set_config(struct gpiod_line *line, int direction, int flags,
> >     const int *default_vals)
> >
> > The existing gpiod_line_set_config would be renamed gpiod_line_set_flags.
> >
> > > > But this is only a vague idea - I'd have to actually start looking at
> > > > the code to be sure. I'd love to see what you came up with so far
> > > > though!
> > > >
> >
> > Indeed - what I had in mind changed radically once I had a closer look
> > at the libgpiod API.  And it is still changing.
> >
> 
> Thanks for doing it! It's also great you included some test cases!
>

Oh, I always write test cases - manual testing is for monkeys.
The few I've added so far are just basic smoke tests - there will be
quite a few more added with better coverage once the API settles down.

I've pushed some more changes with the updated API we discussed earlier.
Those new tests I'd added now pass.  Yay.
One problem though - gpiod_line_set_config as written has no way to
accept an as-is direction.
Hopefully I'll have some time to take another look at that tomorrow.

Cheers,
Kent.

Re: [PATCH v6 0/7] gpio: expose line bias flags to userspace

[Bartosz Golaszewski]

śr., 6 lis 2019 o 17:58 Kent Gibson <warthog618@gmail.com> napisał(a):
>
> On Wed, Nov 06, 2019 at 02:59:33PM +0100, Bartosz Golaszewski wrote:
> > śr., 6 lis 2019 o 07:48 Kent Gibson <warthog618@gmail.com> napisał(a):
> > >
> > > On Tue, Nov 05, 2019 at 10:07:58PM +0100, Bartosz Golaszewski wrote:
> > > > wt., 5 lis 2019 o 17:24 Bartosz Golaszewski
> > > > <bgolaszewski@baylibre.com> napisał(a):
> > > > >
> > > > > wt., 5 lis 2019 o 16:26 Kent Gibson <warthog618@gmail.com> napisał(a):
> > > > > >
> > > > > > On Tue, Nov 05, 2019 at 10:04:22AM +0800, Kent Gibson wrote:
> > > > > > > Patches are against Bart's gpio/for-kent branch[1].
> > > > > > >
> > > > > > > The patch has been successfully tested against gpio-mockup, and
> > > > > > > on a Raspberry Pi, in both cases using the feature/pud_set_config
> > > > > > > branch of my Go gpiod library[2], as well as with my feature/pud
> > > > > > > development branch of libgpiod[3].  Patch 7 has only been tested using
> > > > > > > my gpiod library as libgpiod has not yet been updated to support the
> > > > > > > SET_CONFIG ioctl.
> > > > > > >
> > > > > >
> > > > > > I've just pushed a first pass at SET_CONFIG support into my libgpiod
> > > > > > feature/pud branch.  It is causing me a bit of grief.  Due to the way
> > > > > > the libgpiod API is structured, with the direction flags pulled out into
> > > > > > the request type, I thought it would be cleaner to keep changes to direction
> > > > > > orthogonal to changes to the other handle flags.
> > > > > >
> > > > >
> > > > > I'd love to see that branch - is it public?
> > > > >
> > > > > > So I've added these methods to the API:
> > > > > >
> > > > > > int gpiod_line_set_config(struct gpiod_line *line, int flags)
> > > > > > int gpiod_line_set_direction_input(struct gpiod_line *line)
> > > > > > int gpiod_line_set_direction_output(struct gpiod_line *line,
> > > > > >                                     int value)
> > > > > >
> > > > > > along with their bulk equivalents.
> > > > > >
> > > > > > I've coded that and started adding tests when I tripped over changing
> > > > > > bias.  The kernel requires a direction to be set, but I'm setting it
> > > > > > as-is in gpiod_line_set_config - so that wont work.
> > > > > > Open drain/source are in the same boat - they require output mode.
> > > > > >
> > > > >
> > > > > Ha! Yes this is a problem - how about this:
> > > > >
> > > > > If the caller of set_config in the kernel doesn't pass any of the
> > > > > direction flags, then we read the current direction, set the right
> > > > > flag in lflags and only then call the function validating the flags?
> > > > >
> > > >
> > >
> >
> > Cc'ing Linus, I'm not sure when he was dropped from this discussion.
> >
>
> My bad - I cut it down to you and Drew + list in my initial reply as I
> thought we'd only be talking libgpiod changes.
> But then it took a sharp u-turn back to the kernel patch.
>
> Anway I think we are now in agreement that the kernel is fine and we're
> back in libgpiod territory - see below.
>
> > > I was also thinking along that line - the config would be carried over
> > > from the initial request and any subsequent SET_CONFIGs.
> > > The kernel could overlay the existing config over any field set
> > > "as-is" before performing validation.
> > > The validation requirement would stand, but you don't need to pass the
> > > complete state, possibly including output values, with each SET_CONFIG
> > > request.
> > >
> > > In the bias case above, I create the line as an output and so should
> > > be able to set the bias, even if neither INPUT nor OUTPUT is set in
> > > the SET_CONFIG request.
> > >
> > > > After another thought: this would be a bit inconsistent with the rest
> > > > of the flags. IIRC this was the reason for me to split the
> > > > request_type and other flags into two separate fields in libgpiod in
> > > > the first place.
> > > >
> > >
> > > It is a bit inconsisent, so how about changing the rest of the flags
> > > to make them consistent? They need to have an as-is state, which
> > > corresponds to no flags set, and you then leave that field as is.
> > > We currently have four fields in the handle flags - direction, active
> > > state, drive, and bias.  Of those, direction and bias have as-is states.
> > > What we are missing are additional flags so we have an as-is state for
> > > active state and drive.
> > >
> > > Currently:
> > >     ACTIVE_LOW == 0 => ACTIVE_HIGH, and
> > >     OPEN_DRAIN == 0 && OPEN_SOURCE == 0 => PUSH_PULL.
> > >
> > > If we added an ACTIVE_HIGH flag, to counter ACTIVE_LOW, and PUSH_PULL
> > > to counter OPEN_DRAIN/OPEN_SOURCE, then we can have SET_CONFIG change
> > > the four fields (direction, active state, drive and bias), independently,
> > > or not, as the caller sees fit.
> > >
> > > For backward compatibility, the lines would be created with ACTIVE_HIGH
> > > and PUSH_PULL set, should they be requested "as-is", by the new
> > > definition.
> > >
> >
> > I'm not in favor of having the same behavior triggered in two
> > different ways: one explicit and one implicit. This API got released
> > and we have to live with it, I'm afraid. We could for instance add a
> > comment to the uAPI header though. I also don't think new AS_IS flags
> > are necessary. We can live fine with certain inconveniences in the
> > ioctl() API as long as user-space libraries make up for it by
> > structuring these calls differently.
> >
> > To summarize: I'd prefer to make the SET_CONFIG ioctl() require
> > specifying the direction and then simply caching it in user-space.
> >
>
> Agreed.  I had a quick play with changing the kernel and it was ugly.
> Too many conversions between flag layouts and messing with flag masks
> and bits.  It was absolutely hideous.
> Requiring the userspace to provide the complete config is much simpler.
>
> > > This feels like the right solution to me - as I write this anyway.
> > > The biggest downside I can see is that it means pulling v6, or at least
> > > the SET_CONFIG patches, pending an update.
> > >
> > > > When I think about it: the kernel behavior should be as predictable as
> > > > possible - if we keep the behavior as is in v6, I don't see why we
> > > > couldn't make userspace cache (or re-read) the current direction when
> > > > setting flags other than direction? Do you see any trouble in that?
> > > > That way we'd avoid having the kernel treat different flags in
> > > > different way.
> > > >
> > >
> > > If in userspace then it will have to be cached - the kernel still has
> > > issues reading back output values for emulated open drain/source outputs.
> > > Fixing that is somewhere down my todo list.
> > >
> >
> > Right, but we've lived with problems in this area for a long time and
> > nobody complained - maybe it can wait just a bit more. :)
> >
>
> Hey - I complained!  I had to skip over some of my tests because of that ;-).
>
> > > I can't think of any concrete problems with caching.
> > > It gives me "I have a bad feeling about this" vibe, but I can't pin
> > > down why.  Maybe wanting to avoid shadowing kernel state in userspace?
> > >
> >
> > The user-space can always read back the current state with the
> > lineinfo ioctl(), right? Any problems with that?
> >
>
> Only that you have to convert from the info flag layout to the request
> handle flag layout, or the libgpiod request_type + flag layout.
> And I've already had enough of that for one day.
>
> > > But, as above, I'd rather fix the flags so we have as-is for all, and
> > > caching becomes unnecessary.
> > >
> >
> > This idea in turn gives me a bad feeling. There's something too
> > implicit in this behavior for me. Sounds like it's too easy to get it
> > wrong from user-space.
> >
> > > > Bart
> > > >
> > > > > > I see these options:
> > > > > >  1. set the direction as part of gpiod_line_set_config
> > > > > >  2. relax the kernel restriction.
> > > > >
> > > > > Yes, I don't think we should force users to always pass the direction
> > > > > flag in set_config. Good point.
> > > > >
> > > > > >  3. don't support changing bias or open source/drain.
> > > > >
> > > > > No! We definitely want to support it in libgpiod.
> > >
> > > Agreed.
> > >
> > > > >
> > > > > >  4. rethink the API.
> > > > >
> > > > > As for libgpiod: I think we should have a low-level
> > > > > gpiod_line_set_config() that would set both the direction and other
> > > > > flags (it could for instance only accept two request flags: input and
> > > > > output) and then a higher-level set_flags(), set_direction_input(),
> > > > > set_direction_output() that would call the low-level variant and - for
> > > > > set_flags() without the direction argument - it could simply retrieve
> > > > > the current direction and pass it to gpiod_line_set_config().
> > > > >
> > >
> > > I agree that there should add be a fully capable low level option.
> > >
> > > The low level function would look have a signature like this:
> > >
> > > int gpiod_line_set_config(struct gpiod_line *line, int direction, int flags,
> > >     const int *default_vals)
> > >
> > > The existing gpiod_line_set_config would be renamed gpiod_line_set_flags.
> > >
> > > > > But this is only a vague idea - I'd have to actually start looking at
> > > > > the code to be sure. I'd love to see what you came up with so far
> > > > > though!
> > > > >
> > >
> > > Indeed - what I had in mind changed radically once I had a closer look
> > > at the libgpiod API.  And it is still changing.
> > >
> >
> > Thanks for doing it! It's also great you included some test cases!
> >
>
> Oh, I always write test cases - manual testing is for monkeys.
> The few I've added so far are just basic smoke tests - there will be
> quite a few more added with better coverage once the API settles down.
>
> I've pushed some more changes with the updated API we discussed earlier.
> Those new tests I'd added now pass.  Yay.
> One problem though - gpiod_line_set_config as written has no way to
> accept an as-is direction.
> Hopefully I'll have some time to take another look at that tomorrow.
>

This is not something that we need to merge the kernel changes, it
won't hit a libgpiod release until v5.5 is tagged anyway. Unless I'm
missing something, v6 is still fine to be merged upstream, right? Do
we agree on requiring the full config in SET_CONFIG?

Bart

> Cheers,
> Kent.
>

Re: [PATCH v6 4/7] gpiolib: add support for biasing output lines

[Drew Fustini]

On Tue, Nov 05, 2019 at 10:04:26AM +0800, Kent Gibson wrote:
> Allow pull up/down bias to be set on output lines.
> Use case is for open source or open drain applications where
> internal pull up/down may conflict with external biasing.
> 
> Signed-off-by: Kent Gibson <warthog618@gmail.com>
> ---
>  drivers/gpio/gpiolib.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)

I'm not sure if I am doing something wrong but I can't seem to apply this patch
from the series.

I checked out brgl/gpio/for-next.  I opened this mailbox from lore in mutt:
https://lore.kernel.org/linux-gpio/20191105231533.GA3478@sol/t.mbox.gz

And then I saved each message in the v6 series so I could run 'git am' on each.
I thought I should be able to run git am on the whole mailbox file but I wasn't
sure if the cover letter and replies would cause a problem.

I'd appreciate any suggestions as to how I can resolve this:

pdp7@x1:~/linux$ git checkout -b brgl-gpio-for-next brgl/gpio/for-next 
Branch 'brgl-gpio-for-next' set up to track remote branch 'gpio/for-next' from 'brgl'.
Switched to a new branch 'brgl-gpio-for-next'
pdp7@x1:~/linux$ git log --oneline -1
228fc0104070 (HEAD -> brgl-gpio-for-next, tag: gpio-v5.5-updates-for-linus-part-1, brgl/gpio/for-next) gpio: of: don't warn if ignored GPIO flag matches the behavior
pdp7@x1:~/linux$ grep Subject /tmp/7*
/tmp/71:Subject: [PATCH v6 1/7] gpio: expose pull-up/pull-down line flags to userspace
/tmp/72:Subject: [PATCH v6 2/7] gpiolib: add support for pull up/down to lineevent_create
/tmp/73:Subject: [PATCH v6 3/7] gpiolib: add support for disabling line bias
/tmp/74:Subject: [PATCH v6 4/7] gpiolib: add support for biasing output lines
/tmp/75:Subject: [PATCH v6 5/7] gpio: mockup: add set_config to support pull up/down
/tmp/76:Subject: [PATCH v6 6/7] gpiolib: move validation of line handle flags into helper function
/tmp/77:Subject: [PATCH v6 7/7] gpio: add new SET_CONFIG ioctl() to gpio chardev
pdp7@x1:~/linux$ git am /tmp/7*
Applying: gpio: expose pull-up/pull-down line flags to userspace
Applying: gpiolib: add support for pull up/down to lineevent_create
Applying: gpiolib: add support for disabling line bias
Applying: gpiolib: add support for biasing output lines
error: patch failed: drivers/gpio/gpiolib.c:3168
error: drivers/gpio/gpiolib.c: patch does not apply
Patch failed at 0004 gpiolib: add support for biasing output lines
hint: Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

thanks,
drew

Re: [PATCH v6 0/7] gpio: expose line bias flags to userspace

[Kent Gibson]

On Wed, Nov 06, 2019 at 06:06:12PM +0100, Bartosz Golaszewski wrote:
> 
> This is not something that we need to merge the kernel changes, it
> won't hit a libgpiod release until v5.5 is tagged anyway. Unless I'm
> missing something, v6 is still fine to be merged upstream, right? Do
> we agree on requiring the full config in SET_CONFIG?
> 

I agree that the user needs to pass the full config to SET_CONFIG
and therefore that v6 is still good to merge.

The corresponding libgpiod work will be ongoing for a while.

Cheers,
Kent.

Re: [PATCH v6 4/7] gpiolib: add support for biasing output lines

[Kent Gibson]

On Wed, Nov 06, 2019 at 11:39:31AM -0800, Drew Fustini wrote:
> On Tue, Nov 05, 2019 at 10:04:26AM +0800, Kent Gibson wrote:
> > Allow pull up/down bias to be set on output lines.
> > Use case is for open source or open drain applications where
> > internal pull up/down may conflict with external biasing.
> > 
> > Signed-off-by: Kent Gibson <warthog618@gmail.com>
> > ---
> >  drivers/gpio/gpiolib.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> I'm not sure if I am doing something wrong but I can't seem to apply this patch
> from the series.
> 
> I checked out brgl/gpio/for-next.  I opened this mailbox from lore in mutt:
> https://lore.kernel.org/linux-gpio/20191105231533.GA3478@sol/t.mbox.gz
> 
> And then I saved each message in the v6 series so I could run 'git am' on each.
> I thought I should be able to run git am on the whole mailbox file but I wasn't
> sure if the cover letter and replies would cause a problem.
> 
> I'd appreciate any suggestions as to how I can resolve this:
> 
> pdp7@x1:~/linux$ git checkout -b brgl-gpio-for-next brgl/gpio/for-next 

As per the cover note, v6 is based on brgl/gpio/for-kent, not brgl/gpio/for-next:

 The changes from v5:
  - rebased onto Bart's gpio/for-kent branch.
 
That is the only diff between v5 and v6.  Ironically you should be
applying v5 to your branch :)

Specifically, in my branch the v6 patch is applied to 

e812692b6e9c (brgl/gpio/for-kent) gpio: rcar: Use proper irq_chip name

which is still the HEAD at time of writing.

Applying the patch to for-next also breaks for me at 0004, so your 
patches are probably ok.

You aren't the only one wondering if there is an easier way to apply patches
from lore, especially when there are additional replies in the thread.
To write my demo showing how anyone, not just those that received the 
patches by mail, can rebase v5 onto for-kent to get v6, I ended up 
downloading the mbox.gz, and unzipping and removing the cruft from it.
Fortunately the mbox is chronological, so it was just removing the cover
note and then everything after patch 7.

Surely there is a better way to do this?
Is there a filter for lore or some tool out there that can filter
the returned mbox for you?

Cheers,
Kent.

Re: [PATCH v6 0/7] gpio: expose line bias flags to userspace

[Linus Walleij]

On Tue, Nov 5, 2019 at 4:05 PM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:

> Ok, all looks good now and since Linus already said he's fine with
> this series, I'll take it through my tree for v5.5.

OK good we get it to -next through your tree and I await a
pull request from you for these.

Very nice work from everyone involved here!

Yours,
Linus Walleij

Re: [PATCH v6 0/7] gpio: expose line bias flags to userspace

[Kent Gibson]

On Thu, Nov 07, 2019 at 12:58:04AM +0800, Kent Gibson wrote:
> I've pushed some more changes with the updated API we discussed earlier.
> Those new tests I'd added now pass.  Yay.
> One problem though - gpiod_line_set_config as written has no way to
> accept an as-is direction.
> Hopefully I'll have some time to take another look at that tomorrow.
> 

I've pushed some more updates to my libgpiod branch[1].  They fix the
direction limitation I mentioned (I was using the wrong set flags), 
and extend the tests to cover all of the SET_CONFIG fields.

That completes the C API changes.
If that is ok with you then I can take a look at the corresponding 
changes to the C++ and Python bindings.

And I guess we should move this libgpiod discussion to a new thread?

Cheers,
Kent.

[1] https://github.com/warthog618/libgpiod.git

Re: [PATCH v6 0/7] gpio: expose line bias flags to userspace

[Bartosz Golaszewski]

czw., 7 lis 2019 o 11:39 Kent Gibson <warthog618@gmail.com> napisał(a):
>
> On Thu, Nov 07, 2019 at 12:58:04AM +0800, Kent Gibson wrote:
> > I've pushed some more changes with the updated API we discussed earlier.
> > Those new tests I'd added now pass.  Yay.
> > One problem though - gpiod_line_set_config as written has no way to
> > accept an as-is direction.
> > Hopefully I'll have some time to take another look at that tomorrow.
> >
>
> I've pushed some more updates to my libgpiod branch[1].  They fix the
> direction limitation I mentioned (I was using the wrong set flags),
> and extend the tests to cover all of the SET_CONFIG fields.
>
> That completes the C API changes.
> If that is ok with you then I can take a look at the corresponding
> changes to the C++ and Python bindings.
>
> And I guess we should move this libgpiod discussion to a new thread?

Yes, and better yet - you could simply send these patches for review
and we can continue the discussion there.

Bart

>
> Cheers,
> Kent.
>
> [1] https://github.com/warthog618/libgpiod.git
>

Re: [PATCH v6 0/7] gpio: expose line bias flags to userspace

[Kent Gibson]

On Thu, Nov 07, 2019 at 12:28:09PM +0100, Bartosz Golaszewski wrote:
> czw., 7 lis 2019 o 11:39 Kent Gibson <warthog618@gmail.com> napisał(a):
> >
> > On Thu, Nov 07, 2019 at 12:58:04AM +0800, Kent Gibson wrote:
> > > I've pushed some more changes with the updated API we discussed earlier.
> > > Those new tests I'd added now pass.  Yay.
> > > One problem though - gpiod_line_set_config as written has no way to
> > > accept an as-is direction.
> > > Hopefully I'll have some time to take another look at that tomorrow.
> > >
> >
> > I've pushed some more updates to my libgpiod branch[1].  They fix the
> > direction limitation I mentioned (I was using the wrong set flags),
> > and extend the tests to cover all of the SET_CONFIG fields.
> >
> > That completes the C API changes.
> > If that is ok with you then I can take a look at the corresponding
> > changes to the C++ and Python bindings.
> >
> > And I guess we should move this libgpiod discussion to a new thread?
> 
> Yes, and better yet - you could simply send these patches for review
> and we can continue the discussion there.
> 

I was holding off on putting a patch series together until all the
functionality was complete - so after the C++ and Python bindings.

Cheers,
Kent.

Re: [PATCH v6 0/7] gpio: expose line bias flags to userspace

[Bartosz Golaszewski]

wt., 5 lis 2019 o 03:04 Kent Gibson <warthog618@gmail.com> napisał(a):
>
> The changes from v5:
>   - rebased onto Bart's gpio/for-kent branch.
>
> The changes from v4:
> Review changes:
>  - relocate restriction on applying bias to as-is from patch 2 to patch 1.
>  - propagate errors, other than ENOTSUPP, from gpio_set_bias. (squashed
>    into patches 3 and 4).
>  - include SET_CONFIG patch series v2 (patch 6 and 7 here).
>
> I've also fixed a few other nits I noticed along the way:
>  - rework gpio_set_bias as flags are mutually exclusive.
>  - remove input flag required to set bias restriction from
>    lineevent_create as events are implicitly assumed inputs anyway.
>  - reorder patches to group gpiolib bias patches together before the
>    gpio-mockup changes.
>
>
> This series adds gross control of pull-up/pull-down to the GPIO uAPI.
> Gross control means enabling and disabling of bias functionality,
> not finer grained control such as setting biasing impedances.
>
> The support allows both input and output lines to have any one of the
> following biases applied as part of the line handle or event request:
>  0. As Is - bias is left alone.  This is the default for ABI compatibility.
>  1. Bias Disable - bias is explicitly disabled.
>  2. Pull Down - pull-down bias is enabled.
>  3. Pull Up - pull-up bias is enabled.
>
> The biases are set via three flags, BIAS_DISABLE, BIAS_PULL_DOWN
> and BIAS_PULL_UP.  These map directly to the similarly named
> pinctrl pin_config_param flags.
> As Is corresponds to none of the flags being set.
>
> The setting of biases on output lines may seem odd, but is to allow for
> utilisation of internal pull-up/pull-down on open drain and open source
> outputs, where supported in hardware.
>
> The series also adds the GPIOHANDLE_SET_CONFIG_IOCTL to the gpio chardev.
> The ioctl allows some of the configuration of a requested handle to be
> changed without having to release the line.
> The primary use case is the changing of direction for bi-directional
> lines.
>
> Patches are against Bart's gpio/for-kent branch[1].
>
> The patch has been successfully tested against gpio-mockup, and
> on a Raspberry Pi, in both cases using the feature/pud_set_config
> branch of my Go gpiod library[2], as well as with my feature/pud
> development branch of libgpiod[3].  Patch 7 has only been tested using
> my gpiod library as libgpiod has not yet been updated to support the
> SET_CONFIG ioctl.
>
> Patch 1 adds pull-up/pull-down support to line handle requests.
> Patch 2 adds pull-up/pull-down support to line event requests.
> Patch 3 adds support for disabling bias.
> Patch 4 adds support for setting bias on output lines.
> Patch 5 adds pull-up/down support to the gpio-mockup for uAPI testing.
> Patch 6 refactors the flag validation from linehandle_create.
> Patch 7 adds the SET_CONFIG ioctl.
>
> Drew Fustini (1):
>   gpio: expose pull-up/pull-down line flags to userspace
>
> Kent Gibson (6):
>   gpiolib: add support for pull up/down to lineevent_create
>   gpiolib: add support for disabling line bias
>   gpiolib: add support for biasing output lines
>   gpio: mockup: add set_config to support pull up/down
>   gpiolib: move validation of line handle flags into helper function
>   gpio: add new SET_CONFIG ioctl() to gpio chardev
>
>  drivers/gpio/gpio-mockup.c |  94 ++++++++++------
>  drivers/gpio/gpiolib.c     | 213 +++++++++++++++++++++++++++++++------
>  drivers/gpio/gpiolib.h     |   1 +
>  include/uapi/linux/gpio.h  |  24 +++++
>  4 files changed, 264 insertions(+), 68 deletions(-)
>
> --
> 2.23.0
>

Applied all patches to for-next. Thanks!

Bart

boards to test gpio line bias flags?

[Drew Fustini]

On Tue, Nov 05, 2019 at 10:04:22AM +0800, Kent Gibson wrote:
> This series adds gross control o:f pull-up/pull-down to the GPIO uAPI.
> Gross control means enabling and disabling of bias functionality,
> not finer grained control such as setting biasing impedances.
> 
> The support allows both input and output lines to have any one of the
> following biases applied as part of the line handle or event request:
>  0. As Is - bias is left alone.  This is the default for ABI compatibility.
>  1. Bias Disable - bias is explicitly disabled.
>  2. Pull Down - pull-down bias is enabled.
>  3. Pull Up - pull-up bias is enabled.
> 
> The biases are set via three flags, BIAS_DISABLE, BIAS_PULL_DOWN
> and BIAS_PULL_UP.  These map directly to the similarly named 
> pinctrl pin_config_param flags.
> As Is corresponds to none of the flags being set.

I had been looking at how to make these flags work on the BeagleBone
(TI Sitara AM3358 SoC) which uses the  gpio-omap driver and
pinctrl-single driver.  Howeverm, it seems that it is not posssible as
the BeagleBone device tree uses compatible of "pinctrl-single" instead
of "padconf-single", and thus pcs_pinconf_set() is not called [0].

The bias flags already work on the Raspberry Pi as the Broadcom SoC uses
pinctrl-bcm2835.c which uses gpiochip_generic_config() from gpiolib.c 
for .set_config in bcm2835_gpio_chip.  This eventually calls
bcm2835_pinconf_set() which handles the PIN_CONFIG_BIAS_* flags.

Thus, I started thinking about what other boards I could test the bias
flags with, and potentially, find drivers that I could add or fix
support.

The PIN_CONFIG_BIAS_* flags exist in these GPIO drivers:

gpio-aspeed.c:
aspeed seems to be used in server BMC so not hardware that I have
access to or could easily acquire.

gpio-mockup.c:
mockup driver that has already been tested and works

gpio-pca953x.c:
an external I2C GPIO expander, easy for me to acquire, it appears
that gpio uapi bias flags should work, but I could test and verify

gpiolib.c:
like the bmc2835 in the raspberry pi, it seems some drivers in pinctrl
directory will define a gpiochip and use gpiochip_generic_config().

Here is a list of other pinctrl drivers which use gpiochip_generic_config:

drivers/pinctrl/pinctrl-stmfx.c and drivers/pinctrl/pinctrl-sx150x.c:
These are both GPIO expanders which I could probably purchase

drivers/pinctrl/intel/pinctrl-intel.c:
Maybe this means that I could try this on x86 boards?

drivers/pinctrl/sunxi/pinctrl-sunxi.c
Maybe the Allwinner boards might work too?


Any other hardware that I should take a look at testing?


Thanks,
Drew


[0] https://lore.kernel.org/linux-gpio/20200413123921.GA32586@x1/T/#t