[PATCH v2 0/6] gpio: expose pull-up/pull-down line flags to userspace

[PATCH v2 0/6] gpio: expose pull-up/pull-down line flags to userspace

[Kent Gibson]

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. Pull Up - pull-up bias is enabled.
 2. Pull Down - pull-down bias is enabled.
 3. Pull None - bias is explicitly disabled.

The biases are encoded in two flags, PULL_UP and PULL_DOWN, where
setting both is interpreted as Pull None. So the flags effectively form
a two bit field encoding the values above.

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.

Patches are against v5.4-rc2.

Patch 1 adds support to line handle requests.
Patch 2 adds support to line event requests.
Patch 3 adds pull-up/down support to the gpio-mockup for uAPI testing.
Patch 4 rejects biasing changes to lines requested as-is.
Patch 5 adds support for disabling bias (pull none).
Patch 6 adds support for setting bias on output lines.

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

Kent Gibson (5):
  gpiolib: add support for pull up/down to lineevent_create
  gpio: mockup: add set_config to support pull up/down
  gpiolib: pull requires explicit input mode
  gpiolib: disable bias on inputs when pull up/down are both set
  gpiolib: allow pull up/down on outputs

 drivers/gpio/gpio-mockup.c |  94 +++++++++++++++++++++-------------
 drivers/gpio/gpiolib.c     | 100 +++++++++++++++++++++++++------------
 include/uapi/linux/gpio.h  |   4 ++
 3 files changed, 133 insertions(+), 65 deletions(-)

-- 
2.23.0

[PATCH v2 1/6] 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>
[Bartosz: added flag sanitization]
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpiolib.c    | 17 +++++++++++++++++
 include/uapi/linux/gpio.h |  4 ++++
 2 files changed, 21 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index bdbc1649eafa..9d2a5e2f6e77 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -421,6 +421,8 @@ struct linehandle_state {
 	(GPIOHANDLE_REQUEST_INPUT | \
 	GPIOHANDLE_REQUEST_OUTPUT | \
 	GPIOHANDLE_REQUEST_ACTIVE_LOW | \
+	GPIOHANDLE_REQUEST_PULL_UP | \
+	GPIOHANDLE_REQUEST_PULL_DOWN | \
 	GPIOHANDLE_REQUEST_OPEN_DRAIN | \
 	GPIOHANDLE_REQUEST_OPEN_SOURCE)
 
@@ -537,6 +539,11 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 	    (lflags & GPIOHANDLE_REQUEST_OUTPUT))
 		return -EINVAL;
 
+	/* Same with pull-up and pull-down. */
+	if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
+	    (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))
+		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
@@ -592,6 +599,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_PULL_DOWN)
+			set_bit(FLAG_PULL_DOWN, &desc->flags);
+		if (lflags & GPIOHANDLE_REQUEST_PULL_UP)
+			set_bit(FLAG_PULL_UP, &desc->flags);
 
 		ret = gpiod_set_transitory(desc, false);
 		if (ret < 0)
@@ -1097,6 +1108,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_PULL_DOWN;
+		if (test_bit(FLAG_PULL_UP, &desc->flags))
+			lineinfo.flags |= GPIOLINE_FLAG_PULL_UP;
 
 		if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
 			return -EFAULT;
@@ -2770,6 +2785,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..c2d1f7d908d6 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_PULL_UP	(1UL << 5)
+#define GPIOLINE_FLAG_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_PULL_UP	(1UL << 5)
+#define GPIOHANDLE_REQUEST_PULL_DOWN	(1UL << 6)
 
 /**
  * struct gpiohandle_request - Information about a GPIO handle request
-- 
2.23.0

[PATCH v2 2/6] gpiolib: add support for pull up/down to lineevent_create

[Kent Gibson]

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

Move all the flags sanitization before any memory allocation in
lineevent_create() in order to remove a couple unneeded gotos.
(from Bartosz Golaszewski <bgolaszewski@baylibre.com>)

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

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 9d2a5e2f6e77..053847b6187f 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -905,6 +905,38 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 	if (copy_from_user(&eventreq, ip, sizeof(eventreq)))
 		return -EFAULT;
 
+	offset = eventreq.lineoffset;
+	lflags = eventreq.handleflags;
+	eflags = eventreq.eventflags;
+
+	if (offset >= gdev->ngpio)
+		return -EINVAL;
+
+	/* Return an error if a unknown flag is set */
+	if ((lflags & ~GPIOHANDLE_REQUEST_VALID_FLAGS) ||
+	    (eflags & ~GPIOEVENT_REQUEST_VALID_FLAGS))
+		return -EINVAL;
+
+	/* This is just wrong: we don't look for events on output lines */
+	if ((lflags & GPIOHANDLE_REQUEST_OUTPUT) ||
+	    (lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN) ||
+	    (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_PULL_UP) ||
+	     (lflags & GPIOHANDLE_REQUEST_PULL_DOWN)))
+		return -EINVAL;
+
+	/*
+	 * Do not allow both pull-up and pull-down flags to be set as they
+	 *  are contradictory.
+	 */
+	if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
+	    (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))
+		return -EINVAL;
+
 	le = kzalloc(sizeof(*le), GFP_KERNEL);
 	if (!le)
 		return -ENOMEM;
@@ -922,30 +954,6 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 		}
 	}
 
-	offset = eventreq.lineoffset;
-	lflags = eventreq.handleflags;
-	eflags = eventreq.eventflags;
-
-	if (offset >= gdev->ngpio) {
-		ret = -EINVAL;
-		goto out_free_label;
-	}
-
-	/* Return an error if a unknown flag is set */
-	if ((lflags & ~GPIOHANDLE_REQUEST_VALID_FLAGS) ||
-	    (eflags & ~GPIOEVENT_REQUEST_VALID_FLAGS)) {
-		ret = -EINVAL;
-		goto out_free_label;
-	}
-
-	/* This is just wrong: we don't look for events on output lines */
-	if ((lflags & GPIOHANDLE_REQUEST_OUTPUT) ||
-	    (lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN) ||
-	    (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE)) {
-		ret = -EINVAL;
-		goto out_free_label;
-	}
-
 	desc = &gdev->descs[offset];
 	ret = gpiod_request(desc, le->label);
 	if (ret)
@@ -955,6 +963,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_PULL_DOWN)
+		set_bit(FLAG_PULL_DOWN, &desc->flags);
+	if (lflags & GPIOHANDLE_REQUEST_PULL_UP)
+		set_bit(FLAG_PULL_UP, &desc->flags);
 
 	ret = gpiod_direction_input(desc);
 	if (ret)
-- 
2.23.0

[PATCH v2 3/6] gpio: mockup: add set_config to support pull up/down

[Kent Gibson]

This patch adds 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..493077229677 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 v2 4/6] gpiolib: pull requires explicit input mode

[Kent Gibson]

This patch prevents pull up/down flags being applied to as-is line
requests, which should be left as-is, and for output mode for which
setting pulls is not currently supported.

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

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 053847b6187f..647334f53622 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -559,6 +559,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_PULL_UP) ||
+	     (lflags & GPIOHANDLE_REQUEST_PULL_DOWN)))
+		return -EINVAL;
+
 	lh = kzalloc(sizeof(*lh), GFP_KERNEL);
 	if (!lh)
 		return -ENOMEM;
-- 
2.23.0

[PATCH v2 5/6] gpiolib: disable bias on inputs when pull up/down are both set

[Kent Gibson]

This patch allows 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 | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 647334f53622..f90b20d548b9 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -539,11 +539,6 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 	    (lflags & GPIOHANDLE_REQUEST_OUTPUT))
 		return -EINVAL;
 
-	/* Same with pull-up and pull-down. */
-	if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
-	    (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))
-		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
@@ -935,14 +930,6 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 	     (lflags & GPIOHANDLE_REQUEST_PULL_DOWN)))
 		return -EINVAL;
 
-	/*
-	 * Do not allow both pull-up and pull-down flags to be set as they
-	 *  are contradictory.
-	 */
-	if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
-	    (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))
-		return -EINVAL;
-
 	le = kzalloc(sizeof(*le), GFP_KERNEL);
 	if (!le)
 		return -ENOMEM;
@@ -2931,6 +2918,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;
@@ -2991,7 +2979,11 @@ int gpiod_direction_input(struct gpio_desc *desc)
 	if (ret == 0)
 		clear_bit(FLAG_IS_OUT, &desc->flags);
 
-	if (test_bit(FLAG_PULL_UP, &desc->flags))
+	if (test_bit(FLAG_PULL_UP, &desc->flags) &&
+		test_bit(FLAG_PULL_DOWN, &desc->flags))
+		gpio_set_config(chip, gpio_chip_hwgpio(desc),
+				PIN_CONFIG_BIAS_DISABLE);
+	else 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))
@@ -4462,7 +4454,7 @@ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
 
 	if (lflags & GPIO_PULL_UP)
 		set_bit(FLAG_PULL_UP, &desc->flags);
-	else if (lflags & GPIO_PULL_DOWN)
+	if (lflags & GPIO_PULL_DOWN)
 		set_bit(FLAG_PULL_DOWN, &desc->flags);
 
 	ret = gpiod_set_transitory(desc, (lflags & GPIO_TRANSITORY));
-- 
2.23.0

[PATCH v2 6/6] gpiolib: allow pull up/down on outputs

[Kent Gibson]

This patch allows pull up/down bias to be set on outputs.
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 | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index f90b20d548b9..9cc0f9077c7b 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -554,8 +554,9 @@ 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) &&
+	/* PULL_UP and PULL_DOWN flags only allowed for input or output mode. */
+	if (!((lflags & GPIOHANDLE_REQUEST_INPUT) ||
+	      (lflags & GPIOHANDLE_REQUEST_OUTPUT)) &&
 	    ((lflags & GPIOHANDLE_REQUEST_PULL_UP) ||
 	     (lflags & GPIOHANDLE_REQUEST_PULL_DOWN)))
 		return -EINVAL;
@@ -2932,6 +2933,24 @@ 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;
+
+	if (test_bit(FLAG_PULL_UP, &desc->flags) &&
+	    test_bit(FLAG_PULL_DOWN, &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)
+		return gpio_set_config(chip, gpio_chip_hwgpio(desc), bias);
+
+	return 0;
+}
+
 /**
  * gpiod_direction_input - set the GPIO direction to input
  * @desc:	GPIO to set to input
@@ -2979,16 +2998,7 @@ int gpiod_direction_input(struct gpio_desc *desc)
 	if (ret == 0)
 		clear_bit(FLAG_IS_OUT, &desc->flags);
 
-	if (test_bit(FLAG_PULL_UP, &desc->flags) &&
-		test_bit(FLAG_PULL_DOWN, &desc->flags))
-		gpio_set_config(chip, gpio_chip_hwgpio(desc),
-				PIN_CONFIG_BIAS_DISABLE);
-	else 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);
+	gpio_set_bias(chip, desc);
 
 	trace_gpio_direction(desc_to_gpio(desc), 1, ret);
 
@@ -3114,6 +3124,7 @@ int gpiod_direction_output(struct gpio_desc *desc, int value)
 	}
 
 set_output_value:
+	gpio_set_bias(gc, desc);
 	return gpiod_direction_output_raw_commit(desc, value);
 }
 EXPORT_SYMBOL_GPL(gpiod_direction_output);
-- 
2.23.0

Re: [PATCH v2 2/6] gpiolib: add support for pull up/down to lineevent_create

[Bartosz Golaszewski]

sob., 12 paź 2019 o 03:57 Kent Gibson <warthog618@gmail.com> napisał(a):
>
> This patch adds support for pull up/down to lineevent_create.
> Use cases include receiving asynchronous presses from a
> push button without an external pull up/down.
>
> Move all the flags sanitization before any memory allocation in
> lineevent_create() in order to remove a couple unneeded gotos.
> (from Bartosz Golaszewski <bgolaszewski@baylibre.com>)
>

The patch you pulled in into your commit already sits in my for-next branch.
I didn't know you would be modifying the code I touched earlier. In this case
you can rebase the series on top of gpio/for-next from my tree[1]

> Signed-off-by: Kent Gibson <warthog618@gmail.com>
> ---
>  drivers/gpio/gpiolib.c | 60 +++++++++++++++++++++++++-----------------
>  1 file changed, 36 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 9d2a5e2f6e77..053847b6187f 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -905,6 +905,38 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
>         if (copy_from_user(&eventreq, ip, sizeof(eventreq)))
>                 return -EFAULT;
>
> +       offset = eventreq.lineoffset;
> +       lflags = eventreq.handleflags;
> +       eflags = eventreq.eventflags;
> +
> +       if (offset >= gdev->ngpio)
> +               return -EINVAL;
> +
> +       /* Return an error if a unknown flag is set */
> +       if ((lflags & ~GPIOHANDLE_REQUEST_VALID_FLAGS) ||
> +           (eflags & ~GPIOEVENT_REQUEST_VALID_FLAGS))
> +               return -EINVAL;
> +
> +       /* This is just wrong: we don't look for events on output lines */
> +       if ((lflags & GPIOHANDLE_REQUEST_OUTPUT) ||
> +           (lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN) ||
> +           (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_PULL_UP) ||
> +            (lflags & GPIOHANDLE_REQUEST_PULL_DOWN)))
> +               return -EINVAL;
> +
> +       /*
> +        * Do not allow both pull-up and pull-down flags to be set as they
> +        *  are contradictory.
> +        */
> +       if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
> +           (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))
> +               return -EINVAL;
> +
>         le = kzalloc(sizeof(*le), GFP_KERNEL);
>         if (!le)
>                 return -ENOMEM;
> @@ -922,30 +954,6 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
>                 }
>         }
>
> -       offset = eventreq.lineoffset;
> -       lflags = eventreq.handleflags;
> -       eflags = eventreq.eventflags;
> -
> -       if (offset >= gdev->ngpio) {
> -               ret = -EINVAL;
> -               goto out_free_label;
> -       }
> -
> -       /* Return an error if a unknown flag is set */
> -       if ((lflags & ~GPIOHANDLE_REQUEST_VALID_FLAGS) ||
> -           (eflags & ~GPIOEVENT_REQUEST_VALID_FLAGS)) {
> -               ret = -EINVAL;
> -               goto out_free_label;
> -       }
> -
> -       /* This is just wrong: we don't look for events on output lines */
> -       if ((lflags & GPIOHANDLE_REQUEST_OUTPUT) ||
> -           (lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN) ||
> -           (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE)) {
> -               ret = -EINVAL;
> -               goto out_free_label;
> -       }
> -
>         desc = &gdev->descs[offset];
>         ret = gpiod_request(desc, le->label);
>         if (ret)
> @@ -955,6 +963,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_PULL_DOWN)
> +               set_bit(FLAG_PULL_DOWN, &desc->flags);
> +       if (lflags & GPIOHANDLE_REQUEST_PULL_UP)
> +               set_bit(FLAG_PULL_UP, &desc->flags);
>

Correct me if I'm wrong but this looks like it should be part of
Drew's patch (1/6) in this series right? You can modify it and add
your Signed-off-by tag. In fact your Signed-off-by is needed anyway if
you're sending someone else's patches.

>         ret = gpiod_direction_input(desc);
>         if (ret)
> --
> 2.23.0
>

Bart

[1] https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git/

Re: [PATCH v2 4/6] gpiolib: pull requires explicit input mode

[Bartosz Golaszewski]

sob., 12 paź 2019 o 03:57 Kent Gibson <warthog618@gmail.com> napisał(a):
>
> This patch prevents pull up/down flags being applied to as-is line
> requests, which should be left as-is, and for output mode for which
> setting pulls is not currently supported.
>

This again looks like it should be done right in patch 1/6 instead of
being fixed later in the same series. Or is there some reason to do it
this way I'm not seeing?

Bart

> Signed-off-by: Kent Gibson <warthog618@gmail.com>
> ---
>  drivers/gpio/gpiolib.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 053847b6187f..647334f53622 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -559,6 +559,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_PULL_UP) ||
> +            (lflags & GPIOHANDLE_REQUEST_PULL_DOWN)))
> +               return -EINVAL;
> +
>         lh = kzalloc(sizeof(*lh), GFP_KERNEL);
>         if (!lh)
>                 return -ENOMEM;
> --
> 2.23.0
>

Re: [PATCH v2 5/6] gpiolib: disable bias on inputs when pull up/down are both set

[Bartosz Golaszewski]

sob., 12 paź 2019 o 03:57 Kent Gibson <warthog618@gmail.com> napisał(a):
>
> This patch allows 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 | 22 +++++++---------------
>  1 file changed, 7 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 647334f53622..f90b20d548b9 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -539,11 +539,6 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
>             (lflags & GPIOHANDLE_REQUEST_OUTPUT))
>                 return -EINVAL;
>
> -       /* Same with pull-up and pull-down. */
> -       if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
> -           (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))
> -               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
> @@ -935,14 +930,6 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
>              (lflags & GPIOHANDLE_REQUEST_PULL_DOWN)))
>                 return -EINVAL;
>
> -       /*
> -        * Do not allow both pull-up and pull-down flags to be set as they
> -        *  are contradictory.
> -        */
> -       if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
> -           (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))
> -               return -EINVAL;
> -
>         le = kzalloc(sizeof(*le), GFP_KERNEL);
>         if (!le)
>                 return -ENOMEM;
> @@ -2931,6 +2918,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;
> @@ -2991,7 +2979,11 @@ int gpiod_direction_input(struct gpio_desc *desc)
>         if (ret == 0)
>                 clear_bit(FLAG_IS_OUT, &desc->flags);
>
> -       if (test_bit(FLAG_PULL_UP, &desc->flags))
> +       if (test_bit(FLAG_PULL_UP, &desc->flags) &&
> +               test_bit(FLAG_PULL_DOWN, &desc->flags))
> +               gpio_set_config(chip, gpio_chip_hwgpio(desc),
> +                               PIN_CONFIG_BIAS_DISABLE);
> +       else if (test_bit(FLAG_PULL_UP, &desc->flags))

From looking at the code: user-space can disable bias when setting
both PULL_UP and PULL_DOWN flags. I don't understand why it's done in
this implicit way? Why not a separate flag? Also: I don't like that
this patch deletes code added by earlier patches in this series. If we
really need this, then it should be rearranged.

Bart

>                 gpio_set_config(chip, gpio_chip_hwgpio(desc),
>                                 PIN_CONFIG_BIAS_PULL_UP);
>         else if (test_bit(FLAG_PULL_DOWN, &desc->flags))
> @@ -4462,7 +4454,7 @@ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
>
>         if (lflags & GPIO_PULL_UP)
>                 set_bit(FLAG_PULL_UP, &desc->flags);
> -       else if (lflags & GPIO_PULL_DOWN)
> +       if (lflags & GPIO_PULL_DOWN)
>                 set_bit(FLAG_PULL_DOWN, &desc->flags);
>
>         ret = gpiod_set_transitory(desc, (lflags & GPIO_TRANSITORY));
> --
> 2.23.0
>

Re: [PATCH v2 4/6] gpiolib: pull requires explicit input mode

[Kent Gibson]

On Mon, Oct 14, 2019 at 02:38:38PM +0200, Bartosz Golaszewski wrote:
> sob., 12 paź 2019 o 03:57 Kent Gibson <warthog618@gmail.com> napisał(a):
> >
> > This patch prevents pull up/down flags being applied to as-is line
> > requests, which should be left as-is, and for output mode for which
> > setting pulls is not currently supported.
> >
> 
> This again looks like it should be done right in patch 1/6 instead of
> being fixed later in the same series. Or is there some reason to do it
> this way I'm not seeing?
> 
The patch series adds full support for pull up/down in stages - in order
of increasing level of controversy, at least IMHO.
That way you can drop the more contraversial components if you disagree
with them by rejecting individual patches, and most likely all the
ones that follow.

And I certainly wasn't going to bundle everything into Drew's patch.

Cheers,
Kent.

Re: [PATCH v2 2/6] gpiolib: add support for pull up/down to lineevent_create

[Kent Gibson]

On Mon, Oct 14, 2019 at 02:35:38PM +0200, Bartosz Golaszewski wrote:
> sob., 12 paź 2019 o 03:57 Kent Gibson <warthog618@gmail.com> napisał(a):
> >
> > This patch adds support for pull up/down to lineevent_create.
> > Use cases include receiving asynchronous presses from a
> > push button without an external pull up/down.
> >
> > Move all the flags sanitization before any memory allocation in
> > lineevent_create() in order to remove a couple unneeded gotos.
> > (from Bartosz Golaszewski <bgolaszewski@baylibre.com>)
> >
> 
> The patch you pulled in into your commit already sits in my for-next branch.
> I didn't know you would be modifying the code I touched earlier. In this case
> you can rebase the series on top of gpio/for-next from my tree[1]
> 
You explicitly told me to rebase it onto the latest release candidate,
and to squash the incomplete changes from your branch into my changes.
How did you think that was going to pan out?

> > Signed-off-by: Kent Gibson <warthog618@gmail.com>
> > ---
> >  drivers/gpio/gpiolib.c | 60 +++++++++++++++++++++++++-----------------
> >  1 file changed, 36 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index 9d2a5e2f6e77..053847b6187f 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -905,6 +905,38 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
> >         if (copy_from_user(&eventreq, ip, sizeof(eventreq)))
> >                 return -EFAULT;
> >
> > +       offset = eventreq.lineoffset;
> > +       lflags = eventreq.handleflags;
> > +       eflags = eventreq.eventflags;
> > +
> > +       if (offset >= gdev->ngpio)
> > +               return -EINVAL;
> > +
> > +       /* Return an error if a unknown flag is set */
> > +       if ((lflags & ~GPIOHANDLE_REQUEST_VALID_FLAGS) ||
> > +           (eflags & ~GPIOEVENT_REQUEST_VALID_FLAGS))
> > +               return -EINVAL;
> > +
> > +       /* This is just wrong: we don't look for events on output lines */
> > +       if ((lflags & GPIOHANDLE_REQUEST_OUTPUT) ||
> > +           (lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN) ||
> > +           (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_PULL_UP) ||
> > +            (lflags & GPIOHANDLE_REQUEST_PULL_DOWN)))
> > +               return -EINVAL;
> > +
> > +       /*
> > +        * Do not allow both pull-up and pull-down flags to be set as they
> > +        *  are contradictory.
> > +        */
> > +       if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
> > +           (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))
> > +               return -EINVAL;
> > +
> >         le = kzalloc(sizeof(*le), GFP_KERNEL);
> >         if (!le)
> >                 return -ENOMEM;
> > @@ -922,30 +954,6 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
> >                 }
> >         }
> >
> > -       offset = eventreq.lineoffset;
> > -       lflags = eventreq.handleflags;
> > -       eflags = eventreq.eventflags;
> > -
> > -       if (offset >= gdev->ngpio) {
> > -               ret = -EINVAL;
> > -               goto out_free_label;
> > -       }
> > -
> > -       /* Return an error if a unknown flag is set */
> > -       if ((lflags & ~GPIOHANDLE_REQUEST_VALID_FLAGS) ||
> > -           (eflags & ~GPIOEVENT_REQUEST_VALID_FLAGS)) {
> > -               ret = -EINVAL;
> > -               goto out_free_label;
> > -       }
> > -
> > -       /* This is just wrong: we don't look for events on output lines */
> > -       if ((lflags & GPIOHANDLE_REQUEST_OUTPUT) ||
> > -           (lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN) ||
> > -           (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE)) {
> > -               ret = -EINVAL;
> > -               goto out_free_label;
> > -       }
> > -
> >         desc = &gdev->descs[offset];
> >         ret = gpiod_request(desc, le->label);
> >         if (ret)
> > @@ -955,6 +963,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_PULL_DOWN)
> > +               set_bit(FLAG_PULL_DOWN, &desc->flags);
> > +       if (lflags & GPIOHANDLE_REQUEST_PULL_UP)
> > +               set_bit(FLAG_PULL_UP, &desc->flags);
> >
> 
> Correct me if I'm wrong but this looks like it should be part of
> Drew's patch (1/6) in this series right? You can modify it and add
> your Signed-off-by tag. In fact your Signed-off-by is needed anyway if
> you're sending someone else's patches.
> 
No problem - you are wrong.  Drew's patch added support on handle requests.
This patch adds support on event requests.

Or are you do you want me to squash the whole series down to 2 - gpiolib
and mockup?

Re: [PATCH v2 5/6] gpiolib: disable bias on inputs when pull up/down are both set

[Kent Gibson]

On Mon, Oct 14, 2019 at 02:43:54PM +0200, Bartosz Golaszewski wrote:
> sob., 12 paź 2019 o 03:57 Kent Gibson <warthog618@gmail.com> napisał(a):
> >
> > This patch allows 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 | 22 +++++++---------------
> >  1 file changed, 7 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index 647334f53622..f90b20d548b9 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -539,11 +539,6 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
> >             (lflags & GPIOHANDLE_REQUEST_OUTPUT))
> >                 return -EINVAL;
> >
> > -       /* Same with pull-up and pull-down. */
> > -       if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
> > -           (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))
> > -               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
> > @@ -935,14 +930,6 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
> >              (lflags & GPIOHANDLE_REQUEST_PULL_DOWN)))
> >                 return -EINVAL;
> >
> > -       /*
> > -        * Do not allow both pull-up and pull-down flags to be set as they
> > -        *  are contradictory.
> > -        */
> > -       if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
> > -           (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))
> > -               return -EINVAL;
> > -
> >         le = kzalloc(sizeof(*le), GFP_KERNEL);
> >         if (!le)
> >                 return -ENOMEM;
> > @@ -2931,6 +2918,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;
> > @@ -2991,7 +2979,11 @@ int gpiod_direction_input(struct gpio_desc *desc)
> >         if (ret == 0)
> >                 clear_bit(FLAG_IS_OUT, &desc->flags);
> >
> > -       if (test_bit(FLAG_PULL_UP, &desc->flags))
> > +       if (test_bit(FLAG_PULL_UP, &desc->flags) &&
> > +               test_bit(FLAG_PULL_DOWN, &desc->flags))
> > +               gpio_set_config(chip, gpio_chip_hwgpio(desc),
> > +                               PIN_CONFIG_BIAS_DISABLE);
> > +       else if (test_bit(FLAG_PULL_UP, &desc->flags))
> 
> From looking at the code: user-space can disable bias when setting
> both PULL_UP and PULL_DOWN flags. I don't understand why it's done in
> this implicit way? Why not a separate flag?

An extra flag would waste a bit and add nothing but more sanity checking.

Kent.
> Also: I don't like that
> this patch deletes code added by earlier patches in this series. If we
> really need this, then it should be rearranged.
> 
> Bart
> 
> >                 gpio_set_config(chip, gpio_chip_hwgpio(desc),
> >                                 PIN_CONFIG_BIAS_PULL_UP);
> >         else if (test_bit(FLAG_PULL_DOWN, &desc->flags))
> > @@ -4462,7 +4454,7 @@ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
> >
> >         if (lflags & GPIO_PULL_UP)
> >                 set_bit(FLAG_PULL_UP, &desc->flags);
> > -       else if (lflags & GPIO_PULL_DOWN)
> > +       if (lflags & GPIO_PULL_DOWN)
> >                 set_bit(FLAG_PULL_DOWN, &desc->flags);
> >
> >         ret = gpiod_set_transitory(desc, (lflags & GPIO_TRANSITORY));
> > --
> > 2.23.0
> >

Re: [PATCH v2 2/6] gpiolib: add support for pull up/down to lineevent_create

[Bartosz Golaszewski]

pon., 14 paź 2019 o 14:59 Kent Gibson <warthog618@gmail.com> napisał(a):
>
> On Mon, Oct 14, 2019 at 02:35:38PM +0200, Bartosz Golaszewski wrote:
> > sob., 12 paź 2019 o 03:57 Kent Gibson <warthog618@gmail.com> napisał(a):
> > >
> > > This patch adds support for pull up/down to lineevent_create.
> > > Use cases include receiving asynchronous presses from a
> > > push button without an external pull up/down.
> > >
> > > Move all the flags sanitization before any memory allocation in
> > > lineevent_create() in order to remove a couple unneeded gotos.
> > > (from Bartosz Golaszewski <bgolaszewski@baylibre.com>)
> > >
> >
> > The patch you pulled in into your commit already sits in my for-next branch.
> > I didn't know you would be modifying the code I touched earlier. In this case
> > you can rebase the series on top of gpio/for-next from my tree[1]
> >
> You explicitly told me to rebase it onto the latest release candidate,

Yes, because - as you can find out in
Documentation/process/5.Posting.rst - the general rule is to rebase
the series against the latest release candidate tag from mainline, but
it may become necessary to rebase against the maintainer's tree, which
is the case here.

> and to squash the incomplete changes from your branch into my changes.
> How did you think that was going to pan out?
>

When saying that you can squash my change if you like, I referred to
the unfinished patch[1], not an applied, self-contained change
from[2].

[1] https://github.com/brgl/linux/commit/99b85d1c26ea51b7b6841b2f2971c31d1d544c2f
[2] https://github.com/brgl/linux/commit/f6cfbbe2950b2f7ae6a99d5e13285c8fad5975d2

> > > Signed-off-by: Kent Gibson <warthog618@gmail.com>
> > > ---
> > >  drivers/gpio/gpiolib.c | 60 +++++++++++++++++++++++++-----------------
> > >  1 file changed, 36 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > index 9d2a5e2f6e77..053847b6187f 100644
> > > --- a/drivers/gpio/gpiolib.c
> > > +++ b/drivers/gpio/gpiolib.c
> > > @@ -905,6 +905,38 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
> > >         if (copy_from_user(&eventreq, ip, sizeof(eventreq)))
> > >                 return -EFAULT;
> > >
> > > +       offset = eventreq.lineoffset;
> > > +       lflags = eventreq.handleflags;
> > > +       eflags = eventreq.eventflags;
> > > +
> > > +       if (offset >= gdev->ngpio)
> > > +               return -EINVAL;
> > > +
> > > +       /* Return an error if a unknown flag is set */
> > > +       if ((lflags & ~GPIOHANDLE_REQUEST_VALID_FLAGS) ||
> > > +           (eflags & ~GPIOEVENT_REQUEST_VALID_FLAGS))
> > > +               return -EINVAL;
> > > +
> > > +       /* This is just wrong: we don't look for events on output lines */
> > > +       if ((lflags & GPIOHANDLE_REQUEST_OUTPUT) ||
> > > +           (lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN) ||
> > > +           (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_PULL_UP) ||
> > > +            (lflags & GPIOHANDLE_REQUEST_PULL_DOWN)))
> > > +               return -EINVAL;
> > > +
> > > +       /*
> > > +        * Do not allow both pull-up and pull-down flags to be set as they
> > > +        *  are contradictory.
> > > +        */
> > > +       if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
> > > +           (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))
> > > +               return -EINVAL;
> > > +
> > >         le = kzalloc(sizeof(*le), GFP_KERNEL);
> > >         if (!le)
> > >                 return -ENOMEM;
> > > @@ -922,30 +954,6 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
> > >                 }
> > >         }
> > >
> > > -       offset = eventreq.lineoffset;
> > > -       lflags = eventreq.handleflags;
> > > -       eflags = eventreq.eventflags;
> > > -
> > > -       if (offset >= gdev->ngpio) {
> > > -               ret = -EINVAL;
> > > -               goto out_free_label;
> > > -       }
> > > -
> > > -       /* Return an error if a unknown flag is set */
> > > -       if ((lflags & ~GPIOHANDLE_REQUEST_VALID_FLAGS) ||
> > > -           (eflags & ~GPIOEVENT_REQUEST_VALID_FLAGS)) {
> > > -               ret = -EINVAL;
> > > -               goto out_free_label;
> > > -       }
> > > -
> > > -       /* This is just wrong: we don't look for events on output lines */
> > > -       if ((lflags & GPIOHANDLE_REQUEST_OUTPUT) ||
> > > -           (lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN) ||
> > > -           (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE)) {
> > > -               ret = -EINVAL;
> > > -               goto out_free_label;
> > > -       }
> > > -
> > >         desc = &gdev->descs[offset];
> > >         ret = gpiod_request(desc, le->label);
> > >         if (ret)
> > > @@ -955,6 +963,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_PULL_DOWN)
> > > +               set_bit(FLAG_PULL_DOWN, &desc->flags);
> > > +       if (lflags & GPIOHANDLE_REQUEST_PULL_UP)
> > > +               set_bit(FLAG_PULL_UP, &desc->flags);
> > >
> >
> > Correct me if I'm wrong but this looks like it should be part of
> > Drew's patch (1/6) in this series right? You can modify it and add
> > your Signed-off-by tag. In fact your Signed-off-by is needed anyway if
> > you're sending someone else's patches.
> >
> No problem - you are wrong.  Drew's patch added support on handle requests.
> This patch adds support on event requests.
>
> Or are you do you want me to squash the whole series down to 2 - gpiolib
> and mockup?
>

Re: [PATCH v2 5/6] gpiolib: disable bias on inputs when pull up/down are both set

[Bartosz Golaszewski]

pon., 14 paź 2019 o 15:04 Kent Gibson <warthog618@gmail.com> napisał(a):
>
> On Mon, Oct 14, 2019 at 02:43:54PM +0200, Bartosz Golaszewski wrote:
> > sob., 12 paź 2019 o 03:57 Kent Gibson <warthog618@gmail.com> napisał(a):
> > >
> > > This patch allows 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 | 22 +++++++---------------
> > >  1 file changed, 7 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > index 647334f53622..f90b20d548b9 100644
> > > --- a/drivers/gpio/gpiolib.c
> > > +++ b/drivers/gpio/gpiolib.c
> > > @@ -539,11 +539,6 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
> > >             (lflags & GPIOHANDLE_REQUEST_OUTPUT))
> > >                 return -EINVAL;
> > >
> > > -       /* Same with pull-up and pull-down. */
> > > -       if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
> > > -           (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))
> > > -               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
> > > @@ -935,14 +930,6 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
> > >              (lflags & GPIOHANDLE_REQUEST_PULL_DOWN)))
> > >                 return -EINVAL;
> > >
> > > -       /*
> > > -        * Do not allow both pull-up and pull-down flags to be set as they
> > > -        *  are contradictory.
> > > -        */
> > > -       if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
> > > -           (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))
> > > -               return -EINVAL;
> > > -
> > >         le = kzalloc(sizeof(*le), GFP_KERNEL);
> > >         if (!le)
> > >                 return -ENOMEM;
> > > @@ -2931,6 +2918,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;
> > > @@ -2991,7 +2979,11 @@ int gpiod_direction_input(struct gpio_desc *desc)
> > >         if (ret == 0)
> > >                 clear_bit(FLAG_IS_OUT, &desc->flags);
> > >
> > > -       if (test_bit(FLAG_PULL_UP, &desc->flags))
> > > +       if (test_bit(FLAG_PULL_UP, &desc->flags) &&
> > > +               test_bit(FLAG_PULL_DOWN, &desc->flags))
> > > +               gpio_set_config(chip, gpio_chip_hwgpio(desc),
> > > +                               PIN_CONFIG_BIAS_DISABLE);
> > > +       else if (test_bit(FLAG_PULL_UP, &desc->flags))
> >
> > From looking at the code: user-space can disable bias when setting
> > both PULL_UP and PULL_DOWN flags. I don't understand why it's done in
> > this implicit way? Why not a separate flag?
>
> An extra flag would waste a bit and add nothing but more sanity checking.
>

I disagree. The user API needs to be very explicit. Sanity checking is
alright - if there'll be too many ifdefs, we can start thinking about
adding some core library helpers for sanitizing conflicting flags, I'm
sure other frameworks could use something like this as well.

Especially in this context: setting PULL_UP and PULL_DOWN together
disables bias - this doesn't make sense logically.

Bart

> Kent.
> > Also: I don't like that
> > this patch deletes code added by earlier patches in this series. If we
> > really need this, then it should be rearranged.
> >
> > Bart
> >
> > >                 gpio_set_config(chip, gpio_chip_hwgpio(desc),
> > >                                 PIN_CONFIG_BIAS_PULL_UP);
> > >         else if (test_bit(FLAG_PULL_DOWN, &desc->flags))
> > > @@ -4462,7 +4454,7 @@ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
> > >
> > >         if (lflags & GPIO_PULL_UP)
> > >                 set_bit(FLAG_PULL_UP, &desc->flags);
> > > -       else if (lflags & GPIO_PULL_DOWN)
> > > +       if (lflags & GPIO_PULL_DOWN)
> > >                 set_bit(FLAG_PULL_DOWN, &desc->flags);
> > >
> > >         ret = gpiod_set_transitory(desc, (lflags & GPIO_TRANSITORY));
> > > --
> > > 2.23.0
> > >

Re: [PATCH v2 4/6] gpiolib: pull requires explicit input mode

[Bartosz Golaszewski]

pon., 14 paź 2019 o 14:55 Kent Gibson <warthog618@gmail.com> napisał(a):
>
> On Mon, Oct 14, 2019 at 02:38:38PM +0200, Bartosz Golaszewski wrote:
> > sob., 12 paź 2019 o 03:57 Kent Gibson <warthog618@gmail.com> napisał(a):
> > >
> > > This patch prevents pull up/down flags being applied to as-is line
> > > requests, which should be left as-is, and for output mode for which
> > > setting pulls is not currently supported.
> > >
> >
> > This again looks like it should be done right in patch 1/6 instead of
> > being fixed later in the same series. Or is there some reason to do it
> > this way I'm not seeing?
> >
> The patch series adds full support for pull up/down in stages - in order
> of increasing level of controversy, at least IMHO.
> That way you can drop the more contraversial components if you disagree
> with them by rejecting individual patches, and most likely all the
> ones that follow.
>

I will not - and I think Linus will agree on that - apply half a
series when it addresses the user API. We need to be very precise
about what changes will be merged and the patches must be well
organized logically.

For instance: the commit message of this patch makes me think that it
fixes an issue introduced in an earlier patch in this very series.
Unless that's not true - in which case the commit message should be
reworded - it's not acceptable.

Bart

> And I certainly wasn't going to bundle everything into Drew's patch.
>
> Cheers,
> Kent.
>

Re: [PATCH v2 4/6] gpiolib: pull requires explicit input mode

[Kent Gibson]

On Mon, Oct 14, 2019 at 07:00:37PM +0200, Bartosz Golaszewski wrote:
> pon., 14 paź 2019 o 14:55 Kent Gibson <warthog618@gmail.com> napisał(a):
> >
> > On Mon, Oct 14, 2019 at 02:38:38PM +0200, Bartosz Golaszewski wrote:
> > > sob., 12 paź 2019 o 03:57 Kent Gibson <warthog618@gmail.com> napisał(a):
> > > >
> > > > This patch prevents pull up/down flags being applied to as-is line
> > > > requests, which should be left as-is, and for output mode for which
> > > > setting pulls is not currently supported.
> > > >
> > >
> > > This again looks like it should be done right in patch 1/6 instead of
> > > being fixed later in the same series. Or is there some reason to do it
> > > this way I'm not seeing?
> > >
> > The patch series adds full support for pull up/down in stages - in order
> > of increasing level of controversy, at least IMHO.
> > That way you can drop the more contraversial components if you disagree
> > with them by rejecting individual patches, and most likely all the
> > ones that follow.
> >
> 
> I will not - and I think Linus will agree on that - apply half a
> series when it addresses the user API. We need to be very precise
> about what changes will be merged and the patches must be well
> organized logically.

And that is fine.  The series was structured so I could easily cull the
more contraversial aspects, such as the Pull None, and issue an updated
series.

Conversely it is easy enough for you to squash patches together if you
want all of them, but don't want the intermediate states.
> 
> For instance: the commit message of this patch makes me think that it
> fixes an issue introduced in an earlier patch in this very series.
> Unless that's not true - in which case the commit message should be
> reworded - it's not acceptable.

> 

It fixes what I consider an oversight in a patch that I thought you had
already signed off on. Much the same as my recent patch to reject both 
INPUT and OUTPUT flags being set.  Is that an issue?  Given no one will
even be using the new flags until the feature is added to the kernel 
and libgpiod is updated?

I don't see it as relevant that the oversight it addresses was
introduced in an earlier patch (Drew's).  So what?  You just said 
that you will only apply the series all or nothing, so why does it
matter?

As the problem begins in Drew's patch, that would require changing
Drew's patch itself.  I'm happy to build on top of other's patches,
but I'm not happy to do that. It just doesn't feel right to me.
Doesn't doing that muddle who has changed what, and confuse
attribution?

I obviously don't understand how this whole patch series thing works.

Cheers,
Kent.

Re: [PATCH v2 5/6] gpiolib: disable bias on inputs when pull up/down are both set

[Kent Gibson]

On Mon, Oct 14, 2019 at 06:50:41PM +0200, Bartosz Golaszewski wrote:
> pon., 14 paź 2019 o 15:04 Kent Gibson <warthog618@gmail.com> napisał(a):
> >
> > On Mon, Oct 14, 2019 at 02:43:54PM +0200, Bartosz Golaszewski wrote:
> > > sob., 12 paź 2019 o 03:57 Kent Gibson <warthog618@gmail.com> napisał(a):
> > > >
> > > > This patch allows 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 | 22 +++++++---------------
> > > >  1 file changed, 7 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > > index 647334f53622..f90b20d548b9 100644
> > > > --- a/drivers/gpio/gpiolib.c
> > > > +++ b/drivers/gpio/gpiolib.c
> > > > @@ -539,11 +539,6 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
> > > >             (lflags & GPIOHANDLE_REQUEST_OUTPUT))
> > > >                 return -EINVAL;
> > > >
> > > > -       /* Same with pull-up and pull-down. */
> > > > -       if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
> > > > -           (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))
> > > > -               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
> > > > @@ -935,14 +930,6 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
> > > >              (lflags & GPIOHANDLE_REQUEST_PULL_DOWN)))
> > > >                 return -EINVAL;
> > > >
> > > > -       /*
> > > > -        * Do not allow both pull-up and pull-down flags to be set as they
> > > > -        *  are contradictory.
> > > > -        */
> > > > -       if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
> > > > -           (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))
> > > > -               return -EINVAL;
> > > > -
> > > >         le = kzalloc(sizeof(*le), GFP_KERNEL);
> > > >         if (!le)
> > > >                 return -ENOMEM;
> > > > @@ -2931,6 +2918,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;
> > > > @@ -2991,7 +2979,11 @@ int gpiod_direction_input(struct gpio_desc *desc)
> > > >         if (ret == 0)
> > > >                 clear_bit(FLAG_IS_OUT, &desc->flags);
> > > >
> > > > -       if (test_bit(FLAG_PULL_UP, &desc->flags))
> > > > +       if (test_bit(FLAG_PULL_UP, &desc->flags) &&
> > > > +               test_bit(FLAG_PULL_DOWN, &desc->flags))
> > > > +               gpio_set_config(chip, gpio_chip_hwgpio(desc),
> > > > +                               PIN_CONFIG_BIAS_DISABLE);
> > > > +       else if (test_bit(FLAG_PULL_UP, &desc->flags))
> > >
> > > From looking at the code: user-space can disable bias when setting
> > > both PULL_UP and PULL_DOWN flags. I don't understand why it's done in
> > > this implicit way? Why not a separate flag?
> >
> > An extra flag would waste a bit and add nothing but more sanity checking.
> >
> 
> I disagree. The user API needs to be very explicit. Sanity checking is
> alright - if there'll be too many ifdefs, we can start thinking about
> adding some core library helpers for sanitizing conflicting flags, I'm
> sure other frameworks could use something like this as well.
> 
> Especially in this context: setting PULL_UP and PULL_DOWN together
> disables bias - this doesn't make sense logically.
> 
In a way it does make a weird kind of sense - they cancel.  Physically.

Did you read the cover letter?  The problem, as I see it,
is that we're stuck using a flag field to encode a two bit enum.
That fact the we only have a flag field to play with can't be
changed due to ABI.
I'd be happier adding utils to pull bit fields out of flags.

It makes no sense to me to add another flag, so I wont be doing that.

Cheers,
Kent.

Re: [PATCH v2 5/6] gpiolib: disable bias on inputs when pull up/down are both set

[Bartosz Golaszewski]

wt., 15 paź 2019 o 02:58 Kent Gibson <warthog618@gmail.com> napisał(a):
>
> On Mon, Oct 14, 2019 at 06:50:41PM +0200, Bartosz Golaszewski wrote:
> > pon., 14 paź 2019 o 15:04 Kent Gibson <warthog618@gmail.com> napisał(a):
> > >
> > > On Mon, Oct 14, 2019 at 02:43:54PM +0200, Bartosz Golaszewski wrote:
> > > > sob., 12 paź 2019 o 03:57 Kent Gibson <warthog618@gmail.com> napisał(a):
> > > > >
> > > > > This patch allows 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 | 22 +++++++---------------
> > > > >  1 file changed, 7 insertions(+), 15 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > > > index 647334f53622..f90b20d548b9 100644
> > > > > --- a/drivers/gpio/gpiolib.c
> > > > > +++ b/drivers/gpio/gpiolib.c
> > > > > @@ -539,11 +539,6 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
> > > > >             (lflags & GPIOHANDLE_REQUEST_OUTPUT))
> > > > >                 return -EINVAL;
> > > > >
> > > > > -       /* Same with pull-up and pull-down. */
> > > > > -       if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
> > > > > -           (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))
> > > > > -               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
> > > > > @@ -935,14 +930,6 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
> > > > >              (lflags & GPIOHANDLE_REQUEST_PULL_DOWN)))
> > > > >                 return -EINVAL;
> > > > >
> > > > > -       /*
> > > > > -        * Do not allow both pull-up and pull-down flags to be set as they
> > > > > -        *  are contradictory.
> > > > > -        */
> > > > > -       if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
> > > > > -           (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))
> > > > > -               return -EINVAL;
> > > > > -
> > > > >         le = kzalloc(sizeof(*le), GFP_KERNEL);
> > > > >         if (!le)
> > > > >                 return -ENOMEM;
> > > > > @@ -2931,6 +2918,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;
> > > > > @@ -2991,7 +2979,11 @@ int gpiod_direction_input(struct gpio_desc *desc)
> > > > >         if (ret == 0)
> > > > >                 clear_bit(FLAG_IS_OUT, &desc->flags);
> > > > >
> > > > > -       if (test_bit(FLAG_PULL_UP, &desc->flags))
> > > > > +       if (test_bit(FLAG_PULL_UP, &desc->flags) &&
> > > > > +               test_bit(FLAG_PULL_DOWN, &desc->flags))
> > > > > +               gpio_set_config(chip, gpio_chip_hwgpio(desc),
> > > > > +                               PIN_CONFIG_BIAS_DISABLE);
> > > > > +       else if (test_bit(FLAG_PULL_UP, &desc->flags))
> > > >
> > > > From looking at the code: user-space can disable bias when setting
> > > > both PULL_UP and PULL_DOWN flags. I don't understand why it's done in
> > > > this implicit way? Why not a separate flag?
> > >
> > > An extra flag would waste a bit and add nothing but more sanity checking.
> > >
> >
> > I disagree. The user API needs to be very explicit. Sanity checking is
> > alright - if there'll be too many ifdefs, we can start thinking about
> > adding some core library helpers for sanitizing conflicting flags, I'm
> > sure other frameworks could use something like this as well.
> >
> > Especially in this context: setting PULL_UP and PULL_DOWN together
> > disables bias - this doesn't make sense logically.
> >
> In a way it does make a weird kind of sense - they cancel.  Physically.
>

Yes, on some devices we set both bits to disable bias, but on others
the pull-up and pull-down bits need to be cleared and yet others have
a dedicated bit for that. It's not standardized and the pinctrl
framework defines all three values as separate bits to expose a common
programming interface.

> Did you read the cover letter?  The problem, as I see it,
> is that we're stuck using a flag field to encode a two bit enum.
> That fact the we only have a flag field to play with can't be
> changed due to ABI.

For some reason I haven't received the cover letter on my inbox. I'm
only now seeing it on linux-gpio archives.

Anyway: I don't understand why you insist on using two instead of
three bits. You have 32 bits in total that can be used and only 5 are
used so far. There's plenty left.

I'd prefer to see:

GPIOHANDLE_REQUEST_PULL_UP
GPIOHANDLE_REQUEST_PULL_DOWN
GPIOHANDLE_REQUEST_PULL_DISABLED

or maybe even

GPIOHANDLE_REQUEST_BIAS_PULL_UP
GPIOHANDLE_REQUEST_BIAS_PULL_DOWN
GPIOHANDLE_REQUEST_BIAS_DISABLED

to stay consistent with the pinctrl flags. No bit set among these
three would mean AS_IS.

Bart

> I'd be happier adding utils to pull bit fields out of flags.
>
> It makes no sense to me to add another flag, so I wont be doing that.
>
> Cheers,
> Kent.
>

Re: [PATCH v2 5/6] gpiolib: disable bias on inputs when pull up/down are both set

[Kent Gibson]

On Tue, Oct 15, 2019 at 02:51:18PM +0200, Bartosz Golaszewski wrote:
> wt., 15 paź 2019 o 02:58 Kent Gibson <warthog618@gmail.com> napisał(a):
> >
> > On Mon, Oct 14, 2019 at 06:50:41PM +0200, Bartosz Golaszewski wrote:
> > > pon., 14 paź 2019 o 15:04 Kent Gibson <warthog618@gmail.com> napisał(a):
> > > >
> > > > On Mon, Oct 14, 2019 at 02:43:54PM +0200, Bartosz Golaszewski wrote:
> > > > > sob., 12 paź 2019 o 03:57 Kent Gibson <warthog618@gmail.com> napisał(a):
> > > > > >
> > > > > > This patch allows 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 | 22 +++++++---------------
> > > > > >  1 file changed, 7 insertions(+), 15 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > > > > index 647334f53622..f90b20d548b9 100644
> > > > > > --- a/drivers/gpio/gpiolib.c
> > > > > > +++ b/drivers/gpio/gpiolib.c
> > > > > > @@ -539,11 +539,6 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
> > > > > >             (lflags & GPIOHANDLE_REQUEST_OUTPUT))
> > > > > >                 return -EINVAL;
> > > > > >
> > > > > > -       /* Same with pull-up and pull-down. */
> > > > > > -       if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
> > > > > > -           (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))
> > > > > > -               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
> > > > > > @@ -935,14 +930,6 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
> > > > > >              (lflags & GPIOHANDLE_REQUEST_PULL_DOWN)))
> > > > > >                 return -EINVAL;
> > > > > >
> > > > > > -       /*
> > > > > > -        * Do not allow both pull-up and pull-down flags to be set as they
> > > > > > -        *  are contradictory.
> > > > > > -        */
> > > > > > -       if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
> > > > > > -           (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))
> > > > > > -               return -EINVAL;
> > > > > > -
> > > > > >         le = kzalloc(sizeof(*le), GFP_KERNEL);
> > > > > >         if (!le)
> > > > > >                 return -ENOMEM;
> > > > > > @@ -2931,6 +2918,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;
> > > > > > @@ -2991,7 +2979,11 @@ int gpiod_direction_input(struct gpio_desc *desc)
> > > > > >         if (ret == 0)
> > > > > >                 clear_bit(FLAG_IS_OUT, &desc->flags);
> > > > > >
> > > > > > -       if (test_bit(FLAG_PULL_UP, &desc->flags))
> > > > > > +       if (test_bit(FLAG_PULL_UP, &desc->flags) &&
> > > > > > +               test_bit(FLAG_PULL_DOWN, &desc->flags))
> > > > > > +               gpio_set_config(chip, gpio_chip_hwgpio(desc),
> > > > > > +                               PIN_CONFIG_BIAS_DISABLE);
> > > > > > +       else if (test_bit(FLAG_PULL_UP, &desc->flags))
> > > > >
> > > > > From looking at the code: user-space can disable bias when setting
> > > > > both PULL_UP and PULL_DOWN flags. I don't understand why it's done in
> > > > > this implicit way? Why not a separate flag?
> > > >
> > > > An extra flag would waste a bit and add nothing but more sanity checking.
> > > >
> > >
> > > I disagree. The user API needs to be very explicit. Sanity checking is
> > > alright - if there'll be too many ifdefs, we can start thinking about
> > > adding some core library helpers for sanitizing conflicting flags, I'm
> > > sure other frameworks could use something like this as well.
> > >
> > > Especially in this context: setting PULL_UP and PULL_DOWN together
> > > disables bias - this doesn't make sense logically.
> > >
> > In a way it does make a weird kind of sense - they cancel.  Physically.
> >
> 
> Yes, on some devices we set both bits to disable bias, but on others
> the pull-up and pull-down bits need to be cleared and yet others have
> a dedicated bit for that. It's not standardized and the pinctrl
> framework defines all three values as separate bits to expose a common
> programming interface.
> 
Ok. And, since gpiolib has no knowledge of what combinations are
appropriate for a given chip, we can't provide a higher level
abstraction and have no option but to expose that pinconf
complexity in the GPIO uapi?

In fact, pinconf doesn't just define 3 bias bits - it defines 6:

enum pin_config_param {
	PIN_CONFIG_BIAS_BUS_HOLD,
	PIN_CONFIG_BIAS_DISABLE,
	PIN_CONFIG_BIAS_HIGH_IMPEDANCE,
	PIN_CONFIG_BIAS_PULL_DOWN,
	PIN_CONFIG_BIAS_PULL_PIN_DEFAULT,
	PIN_CONFIG_BIAS_PULL_UP,

Do we need to support any of the remaining 3 in the GPIO uapi, either
now or possibly in the future?

And what about the other PIN_CONFIG flags?  Some of these might be
candidates for controlling via SET_CONFIG_IOCTL, if not in the request
itself? (again this is contemplating the future, not suggesting being part
of this patch)

> > Did you read the cover letter?  The problem, as I see it,
> > is that we're stuck using a flag field to encode a two bit enum.
> > That fact the we only have a flag field to play with can't be
> > changed due to ABI.
> 
> For some reason I haven't received the cover letter on my inbox. I'm
> only now seeing it on linux-gpio archives.
> 
And for some reason I didn't get 0001, yet all 7 parts made it to the mailing
list. Spam filters kicking in? Though it isn't in my spam folder either.
Something odd going on.

> Anyway: I don't understand why you insist on using two instead of
> three bits. You have 32 bits in total that can be used and only 5 are
> used so far. There's plenty left.
> 
Cos it makes no sense to me to encode 4 values into 3 bits when 2 will
do.  But if you want to expose part of the pinconf API within the GPIO
uapi then that goes out the window - it's not 4 values anymore.

And partly cos I'm frustrated that I'd asked questions regarding how the
API should look earlier and got no reply.  This is the sort of thing I
usually deal with in the design stage, not review.

I realise you guys are busy, but a little time spent clarifying design 
would save a whole lot more time in coding, testing and review.

> I'd prefer to see:
> 
> GPIOHANDLE_REQUEST_PULL_UP
> GPIOHANDLE_REQUEST_PULL_DOWN
> GPIOHANDLE_REQUEST_PULL_DISABLED
> 
> or maybe even
> 
> GPIOHANDLE_REQUEST_BIAS_PULL_UP
> GPIOHANDLE_REQUEST_BIAS_PULL_DOWN
> GPIOHANDLE_REQUEST_BIAS_DISABLED
> 
> to stay consistent with the pinctrl flags. No bit set among these
> three would mean AS_IS.
> 
That makes sense, if we are exposing the pinctrl API here.

Wrt the patch series, before I take another go at it, I'd like some 
guidance as to how you would like it structured.
Are you good, in principle, with the feature set in the existing patches?
What branch would you like it based on?
It will begin with Drew's patch, either his original or with your
additions.
Other than that I'm open to your suggestions on number, ordering,
content, comments etc.

Though if you change your mind later I'll probably be cranky.
Or should I say crankier.

Cheers,
Kent.

Re: [PATCH v2 5/6] gpiolib: disable bias on inputs when pull up/down are both set

[Kent Gibson]

On Wed, Oct 16, 2019 at 09:01:04AM +0800, Kent Gibson wrote:
> On Tue, Oct 15, 2019 at 02:51:18PM +0200, Bartosz Golaszewski wrote:
> > wt., 15 paź 2019 o 02:58 Kent Gibson <warthog618@gmail.com> napisał(a):
> > >
> > > On Mon, Oct 14, 2019 at 06:50:41PM +0200, Bartosz Golaszewski wrote:
> > > > pon., 14 paź 2019 o 15:04 Kent Gibson <warthog618@gmail.com> napisał(a):
> > > > >
> > > > > On Mon, Oct 14, 2019 at 02:43:54PM +0200, Bartosz Golaszewski wrote:
> > > > > > sob., 12 paź 2019 o 03:57 Kent Gibson <warthog618@gmail.com> napisał(a):
> > > > > > >
> > > > > > > This patch allows 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 | 22 +++++++---------------
> > > > > > >  1 file changed, 7 insertions(+), 15 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > > > > > index 647334f53622..f90b20d548b9 100644
> > > > > > > --- a/drivers/gpio/gpiolib.c
> > > > > > > +++ b/drivers/gpio/gpiolib.c
> > > > > > > @@ -539,11 +539,6 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
> > > > > > >             (lflags & GPIOHANDLE_REQUEST_OUTPUT))
> > > > > > >                 return -EINVAL;
> > > > > > >
> > > > > > > -       /* Same with pull-up and pull-down. */
> > > > > > > -       if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
> > > > > > > -           (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))
> > > > > > > -               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
> > > > > > > @@ -935,14 +930,6 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
> > > > > > >              (lflags & GPIOHANDLE_REQUEST_PULL_DOWN)))
> > > > > > >                 return -EINVAL;
> > > > > > >
> > > > > > > -       /*
> > > > > > > -        * Do not allow both pull-up and pull-down flags to be set as they
> > > > > > > -        *  are contradictory.
> > > > > > > -        */
> > > > > > > -       if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
> > > > > > > -           (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))
> > > > > > > -               return -EINVAL;
> > > > > > > -
> > > > > > >         le = kzalloc(sizeof(*le), GFP_KERNEL);
> > > > > > >         if (!le)
> > > > > > >                 return -ENOMEM;
> > > > > > > @@ -2931,6 +2918,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;
> > > > > > > @@ -2991,7 +2979,11 @@ int gpiod_direction_input(struct gpio_desc *desc)
> > > > > > >         if (ret == 0)
> > > > > > >                 clear_bit(FLAG_IS_OUT, &desc->flags);
> > > > > > >
> > > > > > > -       if (test_bit(FLAG_PULL_UP, &desc->flags))
> > > > > > > +       if (test_bit(FLAG_PULL_UP, &desc->flags) &&
> > > > > > > +               test_bit(FLAG_PULL_DOWN, &desc->flags))
> > > > > > > +               gpio_set_config(chip, gpio_chip_hwgpio(desc),
> > > > > > > +                               PIN_CONFIG_BIAS_DISABLE);
> > > > > > > +       else if (test_bit(FLAG_PULL_UP, &desc->flags))
> > > > > >
> > > > > > From looking at the code: user-space can disable bias when setting
> > > > > > both PULL_UP and PULL_DOWN flags. I don't understand why it's done in
> > > > > > this implicit way? Why not a separate flag?
> > > > >
> > > > > An extra flag would waste a bit and add nothing but more sanity checking.
> > > > >
> > > >
> > > > I disagree. The user API needs to be very explicit. Sanity checking is
> > > > alright - if there'll be too many ifdefs, we can start thinking about
> > > > adding some core library helpers for sanitizing conflicting flags, I'm
> > > > sure other frameworks could use something like this as well.
> > > >
> > > > Especially in this context: setting PULL_UP and PULL_DOWN together
> > > > disables bias - this doesn't make sense logically.
> > > >
> > > In a way it does make a weird kind of sense - they cancel.  Physically.
> > >
> > 
> > Yes, on some devices we set both bits to disable bias, but on others
> > the pull-up and pull-down bits need to be cleared and yet others have
> > a dedicated bit for that. It's not standardized and the pinctrl
> > framework defines all three values as separate bits to expose a common
> > programming interface.
> > 

Is there any documentation on this?  The pinctrl docs stay pretty high
level and doesn't touch on this. And from the pinconf-generic.h 
documentation, I'd consider drivers that require both pull-up and 
pull-down set to disable bias to be non-compliant with the API - for 
BIAS_DISABLE it says "this setting disables all biasing", so you'd think
the driver would support that and do any mapping (setting both pulls
high or low or whatever) internally.

> Ok. And, since gpiolib has no knowledge of what combinations are
> appropriate for a given chip, we can't provide a higher level
> abstraction and have no option but to expose that pinconf
> complexity in the GPIO uapi?
> 
> In fact, pinconf doesn't just define 3 bias bits - it defines 6:
> 
> enum pin_config_param {
> 	PIN_CONFIG_BIAS_BUS_HOLD,
> 	PIN_CONFIG_BIAS_DISABLE,
> 	PIN_CONFIG_BIAS_HIGH_IMPEDANCE,
> 	PIN_CONFIG_BIAS_PULL_DOWN,
> 	PIN_CONFIG_BIAS_PULL_PIN_DEFAULT,
> 	PIN_CONFIG_BIAS_PULL_UP,
> 
> Do we need to support any of the remaining 3 in the GPIO uapi, either
> now or possibly in the future?
> 
> And what about the other PIN_CONFIG flags?  Some of these might be
> candidates for controlling via SET_CONFIG_IOCTL, if not in the request
> itself? (again this is contemplating the future, not suggesting being part
> of this patch)
> 
> > > Did you read the cover letter?  The problem, as I see it,
> > > is that we're stuck using a flag field to encode a two bit enum.
> > > That fact the we only have a flag field to play with can't be
> > > changed due to ABI.
> > 
> > For some reason I haven't received the cover letter on my inbox. I'm
> > only now seeing it on linux-gpio archives.
> > 
> And for some reason I didn't get 0001, yet all 7 parts made it to the mailing
> list. Spam filters kicking in? Though it isn't in my spam folder either.
> Something odd going on.
> 
> > Anyway: I don't understand why you insist on using two instead of
> > three bits. You have 32 bits in total that can be used and only 5 are
> > used so far. There's plenty left.
> > 
> Cos it makes no sense to me to encode 4 values into 3 bits when 2 will
> do.  But if you want to expose part of the pinconf API within the GPIO
> uapi then that goes out the window - it's not 4 values anymore.
> 
> And partly cos I'm frustrated that I'd asked questions regarding how the
> API should look earlier and got no reply.  This is the sort of thing I
> usually deal with in the design stage, not review.
> 
> I realise you guys are busy, but a little time spent clarifying design 
> would save a whole lot more time in coding, testing and review.
> 
> > I'd prefer to see:
> > 
> > GPIOHANDLE_REQUEST_PULL_UP
> > GPIOHANDLE_REQUEST_PULL_DOWN
> > GPIOHANDLE_REQUEST_PULL_DISABLED
> > 
> > or maybe even
> > 
> > GPIOHANDLE_REQUEST_BIAS_PULL_UP
> > GPIOHANDLE_REQUEST_BIAS_PULL_DOWN
> > GPIOHANDLE_REQUEST_BIAS_DISABLED
> > 
> > to stay consistent with the pinctrl flags. No bit set among these
> > three would mean AS_IS.
> > 
> That makes sense, if we are exposing the pinctrl API here.
> 

Looking at going with the naming including BIAS...
What to do with constants defined in headers prior to this patch that 
don't include the BIAS?  e.g. FLAG_PULL_UP and FLAG_PULL_DOWN in gpiolib.h?
Safe to assume they can't be renamed?
So ok to stay with the unBIASed names for both old (cos they are there)
and also the new (to be consistent with the old)?

Also, while the DT interface (gpiod_configure_flags) has GPIO_PULL_UP
and GPIO_PULL_DOWN, it doesn't support DISABLE, and it explicitly rejects
both having both PULL_UP and PULL_DOWN set.  Should we be extending the 
DISABLE support to the DT interface, and should the API behaviour also 
mirror the pinctrl behaviour you describe above?

And are there any combinations that are guaranteed to be invalid,
and so should be rejected, like DISABLE + PULL_UP??  In fact are there
any combinations that are valid other then PULL_UP + PULL_DOWN?

Cheers,
Kent.

Re: [PATCH v2 5/6] gpiolib: disable bias on inputs when pull up/down are both set

[Bartosz Golaszewski]

śr., 16 paź 2019 o 03:01 Kent Gibson <warthog618@gmail.com> napisał(a):
>
> On Tue, Oct 15, 2019 at 02:51:18PM +0200, Bartosz Golaszewski wrote:
> > wt., 15 paź 2019 o 02:58 Kent Gibson <warthog618@gmail.com> napisał(a):
> > >
> > > On Mon, Oct 14, 2019 at 06:50:41PM +0200, Bartosz Golaszewski wrote:
> > > > pon., 14 paź 2019 o 15:04 Kent Gibson <warthog618@gmail.com> napisał(a):
> > > > >
> > > > > On Mon, Oct 14, 2019 at 02:43:54PM +0200, Bartosz Golaszewski wrote:
> > > > > > sob., 12 paź 2019 o 03:57 Kent Gibson <warthog618@gmail.com> napisał(a):
> > > > > > >
> > > > > > > This patch allows 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 | 22 +++++++---------------
> > > > > > >  1 file changed, 7 insertions(+), 15 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > > > > > index 647334f53622..f90b20d548b9 100644
> > > > > > > --- a/drivers/gpio/gpiolib.c
> > > > > > > +++ b/drivers/gpio/gpiolib.c
> > > > > > > @@ -539,11 +539,6 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
> > > > > > >             (lflags & GPIOHANDLE_REQUEST_OUTPUT))
> > > > > > >                 return -EINVAL;
> > > > > > >
> > > > > > > -       /* Same with pull-up and pull-down. */
> > > > > > > -       if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
> > > > > > > -           (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))
> > > > > > > -               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
> > > > > > > @@ -935,14 +930,6 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
> > > > > > >              (lflags & GPIOHANDLE_REQUEST_PULL_DOWN)))
> > > > > > >                 return -EINVAL;
> > > > > > >
> > > > > > > -       /*
> > > > > > > -        * Do not allow both pull-up and pull-down flags to be set as they
> > > > > > > -        *  are contradictory.
> > > > > > > -        */
> > > > > > > -       if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
> > > > > > > -           (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))
> > > > > > > -               return -EINVAL;
> > > > > > > -
> > > > > > >         le = kzalloc(sizeof(*le), GFP_KERNEL);
> > > > > > >         if (!le)
> > > > > > >                 return -ENOMEM;
> > > > > > > @@ -2931,6 +2918,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;
> > > > > > > @@ -2991,7 +2979,11 @@ int gpiod_direction_input(struct gpio_desc *desc)
> > > > > > >         if (ret == 0)
> > > > > > >                 clear_bit(FLAG_IS_OUT, &desc->flags);
> > > > > > >
> > > > > > > -       if (test_bit(FLAG_PULL_UP, &desc->flags))
> > > > > > > +       if (test_bit(FLAG_PULL_UP, &desc->flags) &&
> > > > > > > +               test_bit(FLAG_PULL_DOWN, &desc->flags))
> > > > > > > +               gpio_set_config(chip, gpio_chip_hwgpio(desc),
> > > > > > > +                               PIN_CONFIG_BIAS_DISABLE);
> > > > > > > +       else if (test_bit(FLAG_PULL_UP, &desc->flags))
> > > > > >
> > > > > > From looking at the code: user-space can disable bias when setting
> > > > > > both PULL_UP and PULL_DOWN flags. I don't understand why it's done in
> > > > > > this implicit way? Why not a separate flag?
> > > > >
> > > > > An extra flag would waste a bit and add nothing but more sanity checking.
> > > > >
> > > >
> > > > I disagree. The user API needs to be very explicit. Sanity checking is
> > > > alright - if there'll be too many ifdefs, we can start thinking about
> > > > adding some core library helpers for sanitizing conflicting flags, I'm
> > > > sure other frameworks could use something like this as well.
> > > >
> > > > Especially in this context: setting PULL_UP and PULL_DOWN together
> > > > disables bias - this doesn't make sense logically.
> > > >
> > > In a way it does make a weird kind of sense - they cancel.  Physically.
> > >
> >
> > Yes, on some devices we set both bits to disable bias, but on others
> > the pull-up and pull-down bits need to be cleared and yet others have
> > a dedicated bit for that. It's not standardized and the pinctrl
> > framework defines all three values as separate bits to expose a common
> > programming interface.
> >
> Ok. And, since gpiolib has no knowledge of what combinations are
> appropriate for a given chip, we can't provide a higher level
> abstraction and have no option but to expose that pinconf
> complexity in the GPIO uapi?
>
> In fact, pinconf doesn't just define 3 bias bits - it defines 6:
>
> enum pin_config_param {
>         PIN_CONFIG_BIAS_BUS_HOLD,
>         PIN_CONFIG_BIAS_DISABLE,
>         PIN_CONFIG_BIAS_HIGH_IMPEDANCE,
>         PIN_CONFIG_BIAS_PULL_DOWN,
>         PIN_CONFIG_BIAS_PULL_PIN_DEFAULT,
>         PIN_CONFIG_BIAS_PULL_UP,
>
> Do we need to support any of the remaining 3 in the GPIO uapi, either
> now or possibly in the future?
>
> And what about the other PIN_CONFIG flags?  Some of these might be
> candidates for controlling via SET_CONFIG_IOCTL, if not in the request
> itself? (again this is contemplating the future, not suggesting being part
> of this patch)
>

Is anyone asking for these though? Users have been asking for adding
support for PULL-UP/DOWN and since it seems like a reasonable
use-case, we're now open to adding it. IMO this is all we need for now
(plus direction changes on taken lines but it's unrelated to this
series IMO) but we should make sure that adding additional options in
the future will be feasible should we need it. Having a hidden
functionality of disabling bias when pull-up and pull-down bits are
set quickly becomes obscure once you add more flags.

> > > Did you read the cover letter?  The problem, as I see it,
> > > is that we're stuck using a flag field to encode a two bit enum.
> > > That fact the we only have a flag field to play with can't be
> > > changed due to ABI.
> >
> > For some reason I haven't received the cover letter on my inbox. I'm
> > only now seeing it on linux-gpio archives.
> >
> And for some reason I didn't get 0001, yet all 7 parts made it to the mailing
> list. Spam filters kicking in? Though it isn't in my spam folder either.
> Something odd going on.
>
> > Anyway: I don't understand why you insist on using two instead of
> > three bits. You have 32 bits in total that can be used and only 5 are
> > used so far. There's plenty left.
> >
> Cos it makes no sense to me to encode 4 values into 3 bits when 2 will
> do.  But if you want to expose part of the pinconf API within the GPIO
> uapi then that goes out the window - it's not 4 values anymore.
>

It does make perfect sense if you want to stay as explicit as possible
on the border between the kernel and user-space. And no, I don't want
to expose the pinctrl API - I just want to follow its example.

> And partly cos I'm frustrated that I'd asked questions regarding how the
> API should look earlier and got no reply.  This is the sort of thing I
> usually deal with in the design stage, not review.
>

I don't really have good news here. This is what kernel development is
like: it's easier for maintainers to discuss existing code than
hypothetical design concepts. Just to give you an idea: it recently
took me 11 iterations of patches and several significant changes to
get a relatively simple and - what I assumed was a rather -
non-controversial pmic driver merged upstream. This is simply how it
usually works: after some initial high-level discussion, there's an
expectation that a patch series will be posted which can then be
discussed and worked upon until a consensus is reached.

Also remember that an idea that sounds good in theory can turn out to
be wrong once the code is ready and there often is no way of
predicting it.

> I realise you guys are busy, but a little time spent clarifying design
> would save a whole lot more time in coding, testing and review.
>
> > I'd prefer to see:
> >
> > GPIOHANDLE_REQUEST_PULL_UP
> > GPIOHANDLE_REQUEST_PULL_DOWN
> > GPIOHANDLE_REQUEST_PULL_DISABLED
> >
> > or maybe even
> >
> > GPIOHANDLE_REQUEST_BIAS_PULL_UP
> > GPIOHANDLE_REQUEST_BIAS_PULL_DOWN
> > GPIOHANDLE_REQUEST_BIAS_DISABLED
> >
> > to stay consistent with the pinctrl flags. No bit set among these
> > three would mean AS_IS.
> >
> That makes sense, if we are exposing the pinctrl API here.
>

But we're not - we're just following the example of the in-kernel
pinctrl API, making it easy on user-space programs and making this
future-proof in that adding new options will be painless.

> Wrt the patch series, before I take another go at it, I'd like some
> guidance as to how you would like it structured.
> Are you good, in principle, with the feature set in the existing patches?

Yes, as I understand it: the patches should add support for
pull-up/down on inputs, the ability to disable bias and then
additional support for pull-up/downs on outputs. This sounds like a
self-contained set of changes.

> What branch would you like it based on?

For now rebase it on top of my gpio/for-next branch[1].

> It will begin with Drew's patch, either his original or with your
> additions.

I don't want a series in which some things are added by earlier
patches only to be removed by subsequent ones - this is nonsensical.
If you know you'll change some parts of Drew's patch later, then
change it right away: leave only the parts that will make it to the
end and that make logical sense as a single patch while keeping the
code bisectable (for instance flags definitions etc.) to give Drew
credit and build on top of that. Be creative and try to imagine what
code you'd want to review.

> Other than that I'm open to your suggestions on number, ordering,
> content, comments etc.
>

Patch 5 is wrong to me, I want an explicit DISABLE flag. Patch 4
should be part of an earlier patch. I'm fine with separate patches for
lineevent and linehandle as long as the kernel never breaks when
applying them one-by-one. Adding support for outputs separately is
fine too.

> Though if you change your mind later I'll probably be cranky.
> Or should I say crankier.
>

This is unnecessary and I won't comment on that.

Bart

> Cheers,
> Kent.

[1] git://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git

Re: [PATCH v2 5/6] gpiolib: disable bias on inputs when pull up/down are both set

[Bartosz Golaszewski]

czw., 17 paź 2019 o 07:06 Kent Gibson <warthog618@gmail.com> napisał(a):
>
> On Wed, Oct 16, 2019 at 09:01:04AM +0800, Kent Gibson wrote:
> > On Tue, Oct 15, 2019 at 02:51:18PM +0200, Bartosz Golaszewski wrote:
> > > wt., 15 paź 2019 o 02:58 Kent Gibson <warthog618@gmail.com> napisał(a):
> > > >
> > > > On Mon, Oct 14, 2019 at 06:50:41PM +0200, Bartosz Golaszewski wrote:
> > > > > pon., 14 paź 2019 o 15:04 Kent Gibson <warthog618@gmail.com> napisał(a):
> > > > > >
> > > > > > On Mon, Oct 14, 2019 at 02:43:54PM +0200, Bartosz Golaszewski wrote:
> > > > > > > sob., 12 paź 2019 o 03:57 Kent Gibson <warthog618@gmail.com> napisał(a):
> > > > > > > >
> > > > > > > > This patch allows 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 | 22 +++++++---------------
> > > > > > > >  1 file changed, 7 insertions(+), 15 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > > > > > > index 647334f53622..f90b20d548b9 100644
> > > > > > > > --- a/drivers/gpio/gpiolib.c
> > > > > > > > +++ b/drivers/gpio/gpiolib.c
> > > > > > > > @@ -539,11 +539,6 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
> > > > > > > >             (lflags & GPIOHANDLE_REQUEST_OUTPUT))
> > > > > > > >                 return -EINVAL;
> > > > > > > >
> > > > > > > > -       /* Same with pull-up and pull-down. */
> > > > > > > > -       if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
> > > > > > > > -           (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))
> > > > > > > > -               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
> > > > > > > > @@ -935,14 +930,6 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
> > > > > > > >              (lflags & GPIOHANDLE_REQUEST_PULL_DOWN)))
> > > > > > > >                 return -EINVAL;
> > > > > > > >
> > > > > > > > -       /*
> > > > > > > > -        * Do not allow both pull-up and pull-down flags to be set as they
> > > > > > > > -        *  are contradictory.
> > > > > > > > -        */
> > > > > > > > -       if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
> > > > > > > > -           (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))
> > > > > > > > -               return -EINVAL;
> > > > > > > > -
> > > > > > > >         le = kzalloc(sizeof(*le), GFP_KERNEL);
> > > > > > > >         if (!le)
> > > > > > > >                 return -ENOMEM;
> > > > > > > > @@ -2931,6 +2918,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;
> > > > > > > > @@ -2991,7 +2979,11 @@ int gpiod_direction_input(struct gpio_desc *desc)
> > > > > > > >         if (ret == 0)
> > > > > > > >                 clear_bit(FLAG_IS_OUT, &desc->flags);
> > > > > > > >
> > > > > > > > -       if (test_bit(FLAG_PULL_UP, &desc->flags))
> > > > > > > > +       if (test_bit(FLAG_PULL_UP, &desc->flags) &&
> > > > > > > > +               test_bit(FLAG_PULL_DOWN, &desc->flags))
> > > > > > > > +               gpio_set_config(chip, gpio_chip_hwgpio(desc),
> > > > > > > > +                               PIN_CONFIG_BIAS_DISABLE);
> > > > > > > > +       else if (test_bit(FLAG_PULL_UP, &desc->flags))
> > > > > > >
> > > > > > > From looking at the code: user-space can disable bias when setting
> > > > > > > both PULL_UP and PULL_DOWN flags. I don't understand why it's done in
> > > > > > > this implicit way? Why not a separate flag?
> > > > > >
> > > > > > An extra flag would waste a bit and add nothing but more sanity checking.
> > > > > >
> > > > >
> > > > > I disagree. The user API needs to be very explicit. Sanity checking is
> > > > > alright - if there'll be too many ifdefs, we can start thinking about
> > > > > adding some core library helpers for sanitizing conflicting flags, I'm
> > > > > sure other frameworks could use something like this as well.
> > > > >
> > > > > Especially in this context: setting PULL_UP and PULL_DOWN together
> > > > > disables bias - this doesn't make sense logically.
> > > > >
> > > > In a way it does make a weird kind of sense - they cancel.  Physically.
> > > >
> > >
> > > Yes, on some devices we set both bits to disable bias, but on others
> > > the pull-up and pull-down bits need to be cleared and yet others have
> > > a dedicated bit for that. It's not standardized and the pinctrl
> > > framework defines all three values as separate bits to expose a common
> > > programming interface.
> > >
>
> Is there any documentation on this?  The pinctrl docs stay pretty high
> level and doesn't touch on this. And from the pinconf-generic.h
> documentation, I'd consider drivers that require both pull-up and
> pull-down set to disable bias to be non-compliant with the API - for
> BIAS_DISABLE it says "this setting disables all biasing", so you'd think
> the driver would support that and do any mapping (setting both pulls
> high or low or whatever) internally.
>
> > Ok. And, since gpiolib has no knowledge of what combinations are
> > appropriate for a given chip, we can't provide a higher level
> > abstraction and have no option but to expose that pinconf
> > complexity in the GPIO uapi?
> >
> > In fact, pinconf doesn't just define 3 bias bits - it defines 6:
> >
> > enum pin_config_param {
> >       PIN_CONFIG_BIAS_BUS_HOLD,
> >       PIN_CONFIG_BIAS_DISABLE,
> >       PIN_CONFIG_BIAS_HIGH_IMPEDANCE,
> >       PIN_CONFIG_BIAS_PULL_DOWN,
> >       PIN_CONFIG_BIAS_PULL_PIN_DEFAULT,
> >       PIN_CONFIG_BIAS_PULL_UP,
> >
> > Do we need to support any of the remaining 3 in the GPIO uapi, either
> > now or possibly in the future?
> >
> > And what about the other PIN_CONFIG flags?  Some of these might be
> > candidates for controlling via SET_CONFIG_IOCTL, if not in the request
> > itself? (again this is contemplating the future, not suggesting being part
> > of this patch)
> >
> > > > Did you read the cover letter?  The problem, as I see it,
> > > > is that we're stuck using a flag field to encode a two bit enum.
> > > > That fact the we only have a flag field to play with can't be
> > > > changed due to ABI.
> > >
> > > For some reason I haven't received the cover letter on my inbox. I'm
> > > only now seeing it on linux-gpio archives.
> > >
> > And for some reason I didn't get 0001, yet all 7 parts made it to the mailing
> > list. Spam filters kicking in? Though it isn't in my spam folder either.
> > Something odd going on.
> >
> > > Anyway: I don't understand why you insist on using two instead of
> > > three bits. You have 32 bits in total that can be used and only 5 are
> > > used so far. There's plenty left.
> > >
> > Cos it makes no sense to me to encode 4 values into 3 bits when 2 will
> > do.  But if you want to expose part of the pinconf API within the GPIO
> > uapi then that goes out the window - it's not 4 values anymore.
> >
> > And partly cos I'm frustrated that I'd asked questions regarding how the
> > API should look earlier and got no reply.  This is the sort of thing I
> > usually deal with in the design stage, not review.
> >
> > I realise you guys are busy, but a little time spent clarifying design
> > would save a whole lot more time in coding, testing and review.
> >
> > > I'd prefer to see:
> > >
> > > GPIOHANDLE_REQUEST_PULL_UP
> > > GPIOHANDLE_REQUEST_PULL_DOWN
> > > GPIOHANDLE_REQUEST_PULL_DISABLED
> > >
> > > or maybe even
> > >
> > > GPIOHANDLE_REQUEST_BIAS_PULL_UP
> > > GPIOHANDLE_REQUEST_BIAS_PULL_DOWN
> > > GPIOHANDLE_REQUEST_BIAS_DISABLED
> > >
> > > to stay consistent with the pinctrl flags. No bit set among these
> > > three would mean AS_IS.
> > >
> > That makes sense, if we are exposing the pinctrl API here.
> >
>
> Looking at going with the naming including BIAS...
> What to do with constants defined in headers prior to this patch that
> don't include the BIAS?  e.g. FLAG_PULL_UP and FLAG_PULL_DOWN in gpiolib.h?

But this has nothing to do with user-space. This was added so that
GPIO expanders can use this without pulling in the pinctrl framework.

> Safe to assume they can't be renamed?

What for?

> So ok to stay with the unBIASed names for both old (cos they are there)
> and also the new (to be consistent with the old)?

There's no need for perfect naming consistency between user and kernel
space declarations. The difference is: you need to be sure to get the
user-space flags right the first time - unlike the kernel APIs, they
cannot be renamed later.

>
> Also, while the DT interface (gpiod_configure_flags) has GPIO_PULL_UP
> and GPIO_PULL_DOWN, it doesn't support DISABLE, and it explicitly rejects
> both having both PULL_UP and PULL_DOWN set.  Should we be extending the
> DISABLE support to the DT interface, and should the API behaviour also
> mirror the pinctrl behaviour you describe above?

Is someone needing it? Adding new features without users is frowned
upon for good reasons.

>
> And are there any combinations that are guaranteed to be invalid,
> and so should be rejected, like DISABLE + PULL_UP??  In fact are there
> any combinations that are valid other then PULL_UP + PULL_DOWN?

You mean invalid? You just said PULL_UP + PULL_DOWN is rejected.

Bart

>
> Cheers,
> Kent.

Re: [PATCH v2 5/6] gpiolib: disable bias on inputs when pull up/down are both set

[Kent Gibson]

On Fri, Oct 18, 2019 at 09:48:09AM +0200, Bartosz Golaszewski wrote:
> śr., 16 paź 2019 o 03:01 Kent Gibson <warthog618@gmail.com> napisał(a):
> >
> > On Tue, Oct 15, 2019 at 02:51:18PM +0200, Bartosz Golaszewski wrote:
> > > wt., 15 paź 2019 o 02:58 Kent Gibson <warthog618@gmail.com> napisał(a):
> > > >
> > > > On Mon, Oct 14, 2019 at 06:50:41PM +0200, Bartosz Golaszewski wrote:
> > > > > pon., 14 paź 2019 o 15:04 Kent Gibson <warthog618@gmail.com> napisał(a):
> > > > > >
> > > > > > On Mon, Oct 14, 2019 at 02:43:54PM +0200, Bartosz Golaszewski wrote:
> > > > > > > sob., 12 paź 2019 o 03:57 Kent Gibson <warthog618@gmail.com> napisał(a):
> > > > > > > >
> > > > > > > > This patch allows 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 | 22 +++++++---------------
> > > > > > > >  1 file changed, 7 insertions(+), 15 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > > > > > > index 647334f53622..f90b20d548b9 100644
> > > > > > > > --- a/drivers/gpio/gpiolib.c
> > > > > > > > +++ b/drivers/gpio/gpiolib.c
> > > > > > > > @@ -539,11 +539,6 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
> > > > > > > >             (lflags & GPIOHANDLE_REQUEST_OUTPUT))
> > > > > > > >                 return -EINVAL;
> > > > > > > >
> > > > > > > > -       /* Same with pull-up and pull-down. */
> > > > > > > > -       if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
> > > > > > > > -           (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))
> > > > > > > > -               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
> > > > > > > > @@ -935,14 +930,6 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
> > > > > > > >              (lflags & GPIOHANDLE_REQUEST_PULL_DOWN)))
> > > > > > > >                 return -EINVAL;
> > > > > > > >
> > > > > > > > -       /*
> > > > > > > > -        * Do not allow both pull-up and pull-down flags to be set as they
> > > > > > > > -        *  are contradictory.
> > > > > > > > -        */
> > > > > > > > -       if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
> > > > > > > > -           (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))
> > > > > > > > -               return -EINVAL;
> > > > > > > > -
> > > > > > > >         le = kzalloc(sizeof(*le), GFP_KERNEL);
> > > > > > > >         if (!le)
> > > > > > > >                 return -ENOMEM;
> > > > > > > > @@ -2931,6 +2918,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;
> > > > > > > > @@ -2991,7 +2979,11 @@ int gpiod_direction_input(struct gpio_desc *desc)
> > > > > > > >         if (ret == 0)
> > > > > > > >                 clear_bit(FLAG_IS_OUT, &desc->flags);
> > > > > > > >
> > > > > > > > -       if (test_bit(FLAG_PULL_UP, &desc->flags))
> > > > > > > > +       if (test_bit(FLAG_PULL_UP, &desc->flags) &&
> > > > > > > > +               test_bit(FLAG_PULL_DOWN, &desc->flags))
> > > > > > > > +               gpio_set_config(chip, gpio_chip_hwgpio(desc),
> > > > > > > > +                               PIN_CONFIG_BIAS_DISABLE);
> > > > > > > > +       else if (test_bit(FLAG_PULL_UP, &desc->flags))
> > > > > > >
> > > > > > > From looking at the code: user-space can disable bias when setting
> > > > > > > both PULL_UP and PULL_DOWN flags. I don't understand why it's done in
> > > > > > > this implicit way? Why not a separate flag?
> > > > > >
> > > > > > An extra flag would waste a bit and add nothing but more sanity checking.
> > > > > >
> > > > >
> > > > > I disagree. The user API needs to be very explicit. Sanity checking is
> > > > > alright - if there'll be too many ifdefs, we can start thinking about
> > > > > adding some core library helpers for sanitizing conflicting flags, I'm
> > > > > sure other frameworks could use something like this as well.
> > > > >
> > > > > Especially in this context: setting PULL_UP and PULL_DOWN together
> > > > > disables bias - this doesn't make sense logically.
> > > > >
> > > > In a way it does make a weird kind of sense - they cancel.  Physically.
> > > >
> > >
> > > Yes, on some devices we set both bits to disable bias, but on others
> > > the pull-up and pull-down bits need to be cleared and yet others have
> > > a dedicated bit for that. It's not standardized and the pinctrl
> > > framework defines all three values as separate bits to expose a common
> > > programming interface.
> > >
> > Ok. And, since gpiolib has no knowledge of what combinations are
> > appropriate for a given chip, we can't provide a higher level
> > abstraction and have no option but to expose that pinconf
> > complexity in the GPIO uapi?
> >
> > In fact, pinconf doesn't just define 3 bias bits - it defines 6:
> >
> > enum pin_config_param {
> >         PIN_CONFIG_BIAS_BUS_HOLD,
> >         PIN_CONFIG_BIAS_DISABLE,
> >         PIN_CONFIG_BIAS_HIGH_IMPEDANCE,
> >         PIN_CONFIG_BIAS_PULL_DOWN,
> >         PIN_CONFIG_BIAS_PULL_PIN_DEFAULT,
> >         PIN_CONFIG_BIAS_PULL_UP,
> >
> > Do we need to support any of the remaining 3 in the GPIO uapi, either
> > now or possibly in the future?
> >
> > And what about the other PIN_CONFIG flags?  Some of these might be
> > candidates for controlling via SET_CONFIG_IOCTL, if not in the request
> > itself? (again this is contemplating the future, not suggesting being part
> > of this patch)
> >
> 
> Is anyone asking for these though? Users have been asking for adding
> support for PULL-UP/DOWN and since it seems like a reasonable
> use-case, we're now open to adding it. IMO this is all we need for now
> (plus direction changes on taken lines but it's unrelated to this
> series IMO) but we should make sure that adding additional options in
> the future will be feasible should we need it. Having a hidden
> functionality of disabling bias when pull-up and pull-down bits are
> set quickly becomes obscure once you add more flags.
> 

The point of the question was whether there may be other bias modes we
want to support in the future. That would put another nail in the
coffin of encoding the 4 values we currently want to support, and give
more reason to mirror the pinctrl API.

> > > > Did you read the cover letter?  The problem, as I see it,
> > > > is that we're stuck using a flag field to encode a two bit enum.
> > > > That fact the we only have a flag field to play with can't be
> > > > changed due to ABI.
> > >
> > > For some reason I haven't received the cover letter on my inbox. I'm
> > > only now seeing it on linux-gpio archives.
> > >
> > And for some reason I didn't get 0001, yet all 7 parts made it to the mailing
> > list. Spam filters kicking in? Though it isn't in my spam folder either.
> > Something odd going on.
> >
> > > Anyway: I don't understand why you insist on using two instead of
> > > three bits. You have 32 bits in total that can be used and only 5 are
> > > used so far. There's plenty left.
> > >
> > Cos it makes no sense to me to encode 4 values into 3 bits when 2 will
> > do.  But if you want to expose part of the pinconf API within the GPIO
> > uapi then that goes out the window - it's not 4 values anymore.
> >
> 
> It does make perfect sense if you want to stay as explicit as possible
> on the border between the kernel and user-space. And no, I don't want
> to expose the pinctrl API - I just want to follow its example.
> 

I'm not sure I see the distinction.
I said part of the pinctrl API, and the flags you want to follow
explicitly are part of it.

But we agree on the end result so moving on...

> > And partly cos I'm frustrated that I'd asked questions regarding how the
> > API should look earlier and got no reply.  This is the sort of thing I
> > usually deal with in the design stage, not review.
> >
> 
> I don't really have good news here. This is what kernel development is
> like: it's easier for maintainers to discuss existing code than
> hypothetical design concepts. Just to give you an idea: it recently
> took me 11 iterations of patches and several significant changes to
> get a relatively simple and - what I assumed was a rather -
> non-controversial pmic driver merged upstream. This is simply how it
> usually works: after some initial high-level discussion, there's an
> expectation that a patch series will be posted which can then be
> discussed and worked upon until a consensus is reached.
> 

I understand that, but conversely there are times when the may be clear
preferences or even hard rules as to what is acceptable or unacceptable,
and it wouldn't be that hard for a maintainer to just say a word to stop
development on a branch which is sure to be dead.  I thought this was
one of those cases, rather than the case you cite.

But anyway, moving on...

> Also remember that an idea that sounds good in theory can turn out to
> be wrong once the code is ready and there often is no way of
> predicting it.
> 
> > I realise you guys are busy, but a little time spent clarifying design
> > would save a whole lot more time in coding, testing and review.
> >
> > > I'd prefer to see:
> > >
> > > GPIOHANDLE_REQUEST_PULL_UP
> > > GPIOHANDLE_REQUEST_PULL_DOWN
> > > GPIOHANDLE_REQUEST_PULL_DISABLED
> > >
> > > or maybe even
> > >
> > > GPIOHANDLE_REQUEST_BIAS_PULL_UP
> > > GPIOHANDLE_REQUEST_BIAS_PULL_DOWN
> > > GPIOHANDLE_REQUEST_BIAS_DISABLED
> > >
> > > to stay consistent with the pinctrl flags. No bit set among these
> > > three would mean AS_IS.
> > >
> > That makes sense, if we are exposing the pinctrl API here.
> >
> 
> But we're not - we're just following the example of the in-kernel
> pinctrl API, making it easy on user-space programs and making this
> future-proof in that adding new options will be painless.
> 

Again, I'm not sure I see the distinction, but I agree going with the
pinctrl flags (PULL_UP, PULL_DOWN and DISABLE) is the way to go.

> > Wrt the patch series, before I take another go at it, I'd like some
> > guidance as to how you would like it structured.
> > Are you good, in principle, with the feature set in the existing patches?
> 
> Yes, as I understand it: the patches should add support for
> pull-up/down on inputs, the ability to disable bias and then
> additional support for pull-up/downs on outputs. This sounds like a
> self-contained set of changes.
> 
> > What branch would you like it based on?
> 
> For now rebase it on top of my gpio/for-next branch[1].
> 
> > It will begin with Drew's patch, either his original or with your
> > additions.
> 
> I don't want a series in which some things are added by earlier
> patches only to be removed by subsequent ones - this is nonsensical.
> If you know you'll change some parts of Drew's patch later, then
> change it right away: leave only the parts that will make it to the
> end and that make logical sense as a single patch while keeping the
> code bisectable (for instance flags definitions etc.) to give Drew
> credit and build on top of that. Be creative and try to imagine what
> code you'd want to review.
> 

Ironically I think it is your additions that are later removed.
So I'll start with Drew's original patch and see how that goes.

> > Other than that I'm open to your suggestions on number, ordering,
> > content, comments etc.
> >
> 
> Patch 5 is wrong to me, I want an explicit DISABLE flag. Patch 4
> should be part of an earlier patch. I'm fine with separate patches for
> lineevent and linehandle as long as the kernel never breaks when
> applying them one-by-one. Adding support for outputs separately is
> fine too.
> 

I do try to ensure that the kernel is never broken by any of my patches.
I believe that is the case with the patches in this series.

Wrt Patch 5 - agreed - DISABLE will be a separate flag as per pinctrl.
And I'll squash 4 into 2.

Cheers,
Kent.

Re: [PATCH v2 5/6] gpiolib: disable bias on inputs when pull up/down are both set

[Kent Gibson]

On Fri, Oct 18, 2019 at 10:03:17AM +0200, Bartosz Golaszewski wrote:
> czw., 17 paź 2019 o 07:06 Kent Gibson <warthog618@gmail.com> napisał(a):
> >
> > On Wed, Oct 16, 2019 at 09:01:04AM +0800, Kent Gibson wrote:
> > > On Tue, Oct 15, 2019 at 02:51:18PM +0200, Bartosz Golaszewski wrote:
> > > > wt., 15 paź 2019 o 02:58 Kent Gibson <warthog618@gmail.com> napisał(a):
> > > > >
> > > > > On Mon, Oct 14, 2019 at 06:50:41PM +0200, Bartosz Golaszewski wrote:
> > > > > > pon., 14 paź 2019 o 15:04 Kent Gibson <warthog618@gmail.com> napisał(a):
> > > > > > >
> > > > > > > On Mon, Oct 14, 2019 at 02:43:54PM +0200, Bartosz Golaszewski wrote:
> > > > > > > > sob., 12 paź 2019 o 03:57 Kent Gibson <warthog618@gmail.com> napisał(a):
> > > > > > > > >
> > > > > > > > > This patch allows 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 | 22 +++++++---------------
> > > > > > > > >  1 file changed, 7 insertions(+), 15 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > > > > > > > index 647334f53622..f90b20d548b9 100644
> > > > > > > > > --- a/drivers/gpio/gpiolib.c
> > > > > > > > > +++ b/drivers/gpio/gpiolib.c
> > > > > > > > > @@ -539,11 +539,6 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
> > > > > > > > >             (lflags & GPIOHANDLE_REQUEST_OUTPUT))
> > > > > > > > >                 return -EINVAL;
> > > > > > > > >
> > > > > > > > > -       /* Same with pull-up and pull-down. */
> > > > > > > > > -       if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
> > > > > > > > > -           (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))
> > > > > > > > > -               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
> > > > > > > > > @@ -935,14 +930,6 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
> > > > > > > > >              (lflags & GPIOHANDLE_REQUEST_PULL_DOWN)))
> > > > > > > > >                 return -EINVAL;
> > > > > > > > >
> > > > > > > > > -       /*
> > > > > > > > > -        * Do not allow both pull-up and pull-down flags to be set as they
> > > > > > > > > -        *  are contradictory.
> > > > > > > > > -        */
> > > > > > > > > -       if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
> > > > > > > > > -           (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))
> > > > > > > > > -               return -EINVAL;
> > > > > > > > > -
> > > > > > > > >         le = kzalloc(sizeof(*le), GFP_KERNEL);
> > > > > > > > >         if (!le)
> > > > > > > > >                 return -ENOMEM;
> > > > > > > > > @@ -2931,6 +2918,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;
> > > > > > > > > @@ -2991,7 +2979,11 @@ int gpiod_direction_input(struct gpio_desc *desc)
> > > > > > > > >         if (ret == 0)
> > > > > > > > >                 clear_bit(FLAG_IS_OUT, &desc->flags);
> > > > > > > > >
> > > > > > > > > -       if (test_bit(FLAG_PULL_UP, &desc->flags))
> > > > > > > > > +       if (test_bit(FLAG_PULL_UP, &desc->flags) &&
> > > > > > > > > +               test_bit(FLAG_PULL_DOWN, &desc->flags))
> > > > > > > > > +               gpio_set_config(chip, gpio_chip_hwgpio(desc),
> > > > > > > > > +                               PIN_CONFIG_BIAS_DISABLE);
> > > > > > > > > +       else if (test_bit(FLAG_PULL_UP, &desc->flags))
> > > > > > > >
> > > > > > > > From looking at the code: user-space can disable bias when setting
> > > > > > > > both PULL_UP and PULL_DOWN flags. I don't understand why it's done in
> > > > > > > > this implicit way? Why not a separate flag?
> > > > > > >
> > > > > > > An extra flag would waste a bit and add nothing but more sanity checking.
> > > > > > >
> > > > > >
> > > > > > I disagree. The user API needs to be very explicit. Sanity checking is
> > > > > > alright - if there'll be too many ifdefs, we can start thinking about
> > > > > > adding some core library helpers for sanitizing conflicting flags, I'm
> > > > > > sure other frameworks could use something like this as well.
> > > > > >
> > > > > > Especially in this context: setting PULL_UP and PULL_DOWN together
> > > > > > disables bias - this doesn't make sense logically.
> > > > > >
> > > > > In a way it does make a weird kind of sense - they cancel.  Physically.
> > > > >
> > > >
> > > > Yes, on some devices we set both bits to disable bias, but on others
> > > > the pull-up and pull-down bits need to be cleared and yet others have
> > > > a dedicated bit for that. It's not standardized and the pinctrl
> > > > framework defines all three values as separate bits to expose a common
> > > > programming interface.
> > > >
> >
> > Is there any documentation on this?  The pinctrl docs stay pretty high
> > level and doesn't touch on this. And from the pinconf-generic.h
> > documentation, I'd consider drivers that require both pull-up and
> > pull-down set to disable bias to be non-compliant with the API - for
> > BIAS_DISABLE it says "this setting disables all biasing", so you'd think
> > the driver would support that and do any mapping (setting both pulls
> > high or low or whatever) internally.
> >

So no answer for this one?

I find it unsettling that we will have a user space API that doesn't 
provide a definitive way to disable bias, independent of the underlying 
hardware.  I thought the kernel was all about hardware abstraction?

> > > Ok. And, since gpiolib has no knowledge of what combinations are
> > > appropriate for a given chip, we can't provide a higher level
> > > abstraction and have no option but to expose that pinconf
> > > complexity in the GPIO uapi?
> > >
> > > In fact, pinconf doesn't just define 3 bias bits - it defines 6:
> > >
> > > enum pin_config_param {
> > >       PIN_CONFIG_BIAS_BUS_HOLD,
> > >       PIN_CONFIG_BIAS_DISABLE,
> > >       PIN_CONFIG_BIAS_HIGH_IMPEDANCE,
> > >       PIN_CONFIG_BIAS_PULL_DOWN,
> > >       PIN_CONFIG_BIAS_PULL_PIN_DEFAULT,
> > >       PIN_CONFIG_BIAS_PULL_UP,
> > >
> > > Do we need to support any of the remaining 3 in the GPIO uapi, either
> > > now or possibly in the future?
> > >
> > > And what about the other PIN_CONFIG flags?  Some of these might be
> > > candidates for controlling via SET_CONFIG_IOCTL, if not in the request
> > > itself? (again this is contemplating the future, not suggesting being part
> > > of this patch)
> > >
> > > > > Did you read the cover letter?  The problem, as I see it,
> > > > > is that we're stuck using a flag field to encode a two bit enum.
> > > > > That fact the we only have a flag field to play with can't be
> > > > > changed due to ABI.
> > > >
> > > > For some reason I haven't received the cover letter on my inbox. I'm
> > > > only now seeing it on linux-gpio archives.
> > > >
> > > And for some reason I didn't get 0001, yet all 7 parts made it to the mailing
> > > list. Spam filters kicking in? Though it isn't in my spam folder either.
> > > Something odd going on.
> > >
> > > > Anyway: I don't understand why you insist on using two instead of
> > > > three bits. You have 32 bits in total that can be used and only 5 are
> > > > used so far. There's plenty left.
> > > >
> > > Cos it makes no sense to me to encode 4 values into 3 bits when 2 will
> > > do.  But if you want to expose part of the pinconf API within the GPIO
> > > uapi then that goes out the window - it's not 4 values anymore.
> > >
> > > And partly cos I'm frustrated that I'd asked questions regarding how the
> > > API should look earlier and got no reply.  This is the sort of thing I
> > > usually deal with in the design stage, not review.
> > >
> > > I realise you guys are busy, but a little time spent clarifying design
> > > would save a whole lot more time in coding, testing and review.
> > >
> > > > I'd prefer to see:
> > > >
> > > > GPIOHANDLE_REQUEST_PULL_UP
> > > > GPIOHANDLE_REQUEST_PULL_DOWN
> > > > GPIOHANDLE_REQUEST_PULL_DISABLED
> > > >
> > > > or maybe even
> > > >
> > > > GPIOHANDLE_REQUEST_BIAS_PULL_UP
> > > > GPIOHANDLE_REQUEST_BIAS_PULL_DOWN
> > > > GPIOHANDLE_REQUEST_BIAS_DISABLED
> > > >
> > > > to stay consistent with the pinctrl flags. No bit set among these
> > > > three would mean AS_IS.
> > > >
> > > That makes sense, if we are exposing the pinctrl API here.
> > >
> >
> > Looking at going with the naming including BIAS...
> > What to do with constants defined in headers prior to this patch that
> > don't include the BIAS?  e.g. FLAG_PULL_UP and FLAG_PULL_DOWN in gpiolib.h?
> 
> But this has nothing to do with user-space. This was added so that
> GPIO expanders can use this without pulling in the pinctrl framework.
> 
> > Safe to assume they can't be renamed?
> 
> What for?
> 

For consistency and clarity.  I need to add a flag into the gpio_desc
flags, which are usedf throughout gpiolib.c and aredefined in gpiolib.h,
and those FLAG_PULL_UP and FLAG_PULL_DOWN are already there.

To be consistent I'd be dropping the GPIOHANDLE_REQUEST_BIAS_ from your
preferred names when determining the name for the new flag, but then it
would be called FLAG_DISABLE, which is obviously too vague.

I intend to use FLAG_BIAS_DISABLE, and for consistency it would be nice
to rename FLAG_PULL_UP to FLAG_BIAS_PULL_UP, and similarly PULL_DOWN, 
but that may break things and so be unacceptable, right?

> > So ok to stay with the unBIASed names for both old (cos they are there)
> > and also the new (to be consistent with the old)?
> 
> There's no need for perfect naming consistency between user and kernel
> space declarations. The difference is: you need to be sure to get the
> user-space flags right the first time - unlike the kernel APIs, they
> cannot be renamed later.
> 

So it may be acceptable to change the gpiolib.h flag names?

This is one of those cases where I'd rather ask than guess and not find
out until the patch gets rejected.

> >
> > Also, while the DT interface (gpiod_configure_flags) has GPIO_PULL_UP
> > and GPIO_PULL_DOWN, it doesn't support DISABLE, and it explicitly rejects
> > both having both PULL_UP and PULL_DOWN set.  Should we be extending the
> > DISABLE support to the DT interface, and should the API behaviour also
> > mirror the pinctrl behaviour you describe above?
> 
> Is someone needing it? Adding new features without users is frowned
> upon for good reasons.
> 

Not that I am aware of, so we can always add it later.
And that is why I asked - my first instinct is to keep the APIs
aligned, especially where we now have the user space API containing
functionality not available to DT, but I am also aware that might be
considered unimportant or even unacceptable.

> >
> > And are there any combinations that are guaranteed to be invalid,
> > and so should be rejected, like DISABLE + PULL_UP??  In fact are there
> > any combinations that are valid other then PULL_UP + PULL_DOWN?
> 
> You mean invalid? You just said PULL_UP + PULL_DOWN is rejected.
> 

No, I my example was  DISABLE + PULL_UP, which I assume is an invalid combo.
And then on reflection the only valid combo I could think of was
PULL_UP + PULL_DOWN, and even then only because you said we need it to
disable bias on some chips.

Cheers,
Kent.

Re: [PATCH v2 5/6] gpiolib: disable bias on inputs when pull up/down are both set

[Bartosz Golaszewski]

pt., 18 paź 2019 o 12:13 Kent Gibson <warthog618@gmail.com> napisał(a):
>
> On Fri, Oct 18, 2019 at 10:03:17AM +0200, Bartosz Golaszewski wrote:
> > czw., 17 paź 2019 o 07:06 Kent Gibson <warthog618@gmail.com> napisał(a):
> > >
> > > On Wed, Oct 16, 2019 at 09:01:04AM +0800, Kent Gibson wrote:
> > > > On Tue, Oct 15, 2019 at 02:51:18PM +0200, Bartosz Golaszewski wrote:
> > > > > wt., 15 paź 2019 o 02:58 Kent Gibson <warthog618@gmail.com> napisał(a):
> > > > > >
> > > > > > On Mon, Oct 14, 2019 at 06:50:41PM +0200, Bartosz Golaszewski wrote:
> > > > > > > pon., 14 paź 2019 o 15:04 Kent Gibson <warthog618@gmail.com> napisał(a):
> > > > > > > >
> > > > > > > > On Mon, Oct 14, 2019 at 02:43:54PM +0200, Bartosz Golaszewski wrote:
> > > > > > > > > sob., 12 paź 2019 o 03:57 Kent Gibson <warthog618@gmail.com> napisał(a):
> > > > > > > > > >
> > > > > > > > > > This patch allows 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 | 22 +++++++---------------
> > > > > > > > > >  1 file changed, 7 insertions(+), 15 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > > > > > > > > index 647334f53622..f90b20d548b9 100644
> > > > > > > > > > --- a/drivers/gpio/gpiolib.c
> > > > > > > > > > +++ b/drivers/gpio/gpiolib.c
> > > > > > > > > > @@ -539,11 +539,6 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
> > > > > > > > > >             (lflags & GPIOHANDLE_REQUEST_OUTPUT))
> > > > > > > > > >                 return -EINVAL;
> > > > > > > > > >
> > > > > > > > > > -       /* Same with pull-up and pull-down. */
> > > > > > > > > > -       if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
> > > > > > > > > > -           (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))
> > > > > > > > > > -               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
> > > > > > > > > > @@ -935,14 +930,6 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
> > > > > > > > > >              (lflags & GPIOHANDLE_REQUEST_PULL_DOWN)))
> > > > > > > > > >                 return -EINVAL;
> > > > > > > > > >
> > > > > > > > > > -       /*
> > > > > > > > > > -        * Do not allow both pull-up and pull-down flags to be set as they
> > > > > > > > > > -        *  are contradictory.
> > > > > > > > > > -        */
> > > > > > > > > > -       if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
> > > > > > > > > > -           (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))
> > > > > > > > > > -               return -EINVAL;
> > > > > > > > > > -
> > > > > > > > > >         le = kzalloc(sizeof(*le), GFP_KERNEL);
> > > > > > > > > >         if (!le)
> > > > > > > > > >                 return -ENOMEM;
> > > > > > > > > > @@ -2931,6 +2918,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;
> > > > > > > > > > @@ -2991,7 +2979,11 @@ int gpiod_direction_input(struct gpio_desc *desc)
> > > > > > > > > >         if (ret == 0)
> > > > > > > > > >                 clear_bit(FLAG_IS_OUT, &desc->flags);
> > > > > > > > > >
> > > > > > > > > > -       if (test_bit(FLAG_PULL_UP, &desc->flags))
> > > > > > > > > > +       if (test_bit(FLAG_PULL_UP, &desc->flags) &&
> > > > > > > > > > +               test_bit(FLAG_PULL_DOWN, &desc->flags))
> > > > > > > > > > +               gpio_set_config(chip, gpio_chip_hwgpio(desc),
> > > > > > > > > > +                               PIN_CONFIG_BIAS_DISABLE);
> > > > > > > > > > +       else if (test_bit(FLAG_PULL_UP, &desc->flags))
> > > > > > > > >
> > > > > > > > > From looking at the code: user-space can disable bias when setting
> > > > > > > > > both PULL_UP and PULL_DOWN flags. I don't understand why it's done in
> > > > > > > > > this implicit way? Why not a separate flag?
> > > > > > > >
> > > > > > > > An extra flag would waste a bit and add nothing but more sanity checking.
> > > > > > > >
> > > > > > >
> > > > > > > I disagree. The user API needs to be very explicit. Sanity checking is
> > > > > > > alright - if there'll be too many ifdefs, we can start thinking about
> > > > > > > adding some core library helpers for sanitizing conflicting flags, I'm
> > > > > > > sure other frameworks could use something like this as well.
> > > > > > >
> > > > > > > Especially in this context: setting PULL_UP and PULL_DOWN together
> > > > > > > disables bias - this doesn't make sense logically.
> > > > > > >
> > > > > > In a way it does make a weird kind of sense - they cancel.  Physically.
> > > > > >
> > > > >
> > > > > Yes, on some devices we set both bits to disable bias, but on others
> > > > > the pull-up and pull-down bits need to be cleared and yet others have
> > > > > a dedicated bit for that. It's not standardized and the pinctrl
> > > > > framework defines all three values as separate bits to expose a common
> > > > > programming interface.
> > > > >
> > >
> > > Is there any documentation on this?  The pinctrl docs stay pretty high
> > > level and doesn't touch on this. And from the pinconf-generic.h
> > > documentation, I'd consider drivers that require both pull-up and
> > > pull-down set to disable bias to be non-compliant with the API - for
> > > BIAS_DISABLE it says "this setting disables all biasing", so you'd think
> > > the driver would support that and do any mapping (setting both pulls
> > > high or low or whatever) internally.
> > >
>
> So no answer for this one?
>
> I find it unsettling that we will have a user space API that doesn't
> provide a definitive way to disable bias, independent of the underlying
> hardware.  I thought the kernel was all about hardware abstraction?
>

If you expose three options to user-space: PULL-UP, PULL-DOWN and
DISABLE and then that gets translated by the driver to whatever the
driver understands - how is this not abstraction?

> > > > Ok. And, since gpiolib has no knowledge of what combinations are
> > > > appropriate for a given chip, we can't provide a higher level
> > > > abstraction and have no option but to expose that pinconf
> > > > complexity in the GPIO uapi?
> > > >
> > > > In fact, pinconf doesn't just define 3 bias bits - it defines 6:
> > > >
> > > > enum pin_config_param {
> > > >       PIN_CONFIG_BIAS_BUS_HOLD,
> > > >       PIN_CONFIG_BIAS_DISABLE,
> > > >       PIN_CONFIG_BIAS_HIGH_IMPEDANCE,
> > > >       PIN_CONFIG_BIAS_PULL_DOWN,
> > > >       PIN_CONFIG_BIAS_PULL_PIN_DEFAULT,
> > > >       PIN_CONFIG_BIAS_PULL_UP,
> > > >
> > > > Do we need to support any of the remaining 3 in the GPIO uapi, either
> > > > now or possibly in the future?
> > > >
> > > > And what about the other PIN_CONFIG flags?  Some of these might be
> > > > candidates for controlling via SET_CONFIG_IOCTL, if not in the request
> > > > itself? (again this is contemplating the future, not suggesting being part
> > > > of this patch)
> > > >
> > > > > > Did you read the cover letter?  The problem, as I see it,
> > > > > > is that we're stuck using a flag field to encode a two bit enum.
> > > > > > That fact the we only have a flag field to play with can't be
> > > > > > changed due to ABI.
> > > > >
> > > > > For some reason I haven't received the cover letter on my inbox. I'm
> > > > > only now seeing it on linux-gpio archives.
> > > > >
> > > > And for some reason I didn't get 0001, yet all 7 parts made it to the mailing
> > > > list. Spam filters kicking in? Though it isn't in my spam folder either.
> > > > Something odd going on.
> > > >
> > > > > Anyway: I don't understand why you insist on using two instead of
> > > > > three bits. You have 32 bits in total that can be used and only 5 are
> > > > > used so far. There's plenty left.
> > > > >
> > > > Cos it makes no sense to me to encode 4 values into 3 bits when 2 will
> > > > do.  But if you want to expose part of the pinconf API within the GPIO
> > > > uapi then that goes out the window - it's not 4 values anymore.
> > > >
> > > > And partly cos I'm frustrated that I'd asked questions regarding how the
> > > > API should look earlier and got no reply.  This is the sort of thing I
> > > > usually deal with in the design stage, not review.
> > > >
> > > > I realise you guys are busy, but a little time spent clarifying design
> > > > would save a whole lot more time in coding, testing and review.
> > > >
> > > > > I'd prefer to see:
> > > > >
> > > > > GPIOHANDLE_REQUEST_PULL_UP
> > > > > GPIOHANDLE_REQUEST_PULL_DOWN
> > > > > GPIOHANDLE_REQUEST_PULL_DISABLED
> > > > >
> > > > > or maybe even
> > > > >
> > > > > GPIOHANDLE_REQUEST_BIAS_PULL_UP
> > > > > GPIOHANDLE_REQUEST_BIAS_PULL_DOWN
> > > > > GPIOHANDLE_REQUEST_BIAS_DISABLED
> > > > >
> > > > > to stay consistent with the pinctrl flags. No bit set among these
> > > > > three would mean AS_IS.
> > > > >
> > > > That makes sense, if we are exposing the pinctrl API here.
> > > >
> > >
> > > Looking at going with the naming including BIAS...
> > > What to do with constants defined in headers prior to this patch that
> > > don't include the BIAS?  e.g. FLAG_PULL_UP and FLAG_PULL_DOWN in gpiolib.h?
> >
> > But this has nothing to do with user-space. This was added so that
> > GPIO expanders can use this without pulling in the pinctrl framework.
> >
> > > Safe to assume they can't be renamed?
> >
> > What for?
> >
>
> For consistency and clarity.  I need to add a flag into the gpio_desc
> flags, which are usedf throughout gpiolib.c and aredefined in gpiolib.h,
> and those FLAG_PULL_UP and FLAG_PULL_DOWN are already there.
>
> To be consistent I'd be dropping the GPIOHANDLE_REQUEST_BIAS_ from your
> preferred names when determining the name for the new flag, but then it
> would be called FLAG_DISABLE, which is obviously too vague.
>
> I intend to use FLAG_BIAS_DISABLE, and for consistency it would be nice
> to rename FLAG_PULL_UP to FLAG_BIAS_PULL_UP, and similarly PULL_DOWN,
> but that may break things and so be unacceptable, right?
>

Yeah, not for device tree, no. It sounds fine for core gpiolib but I
wouldn't change it in this series though personally - let's try to
limit its scope and we can come back to it later.

> > > So ok to stay with the unBIASed names for both old (cos they are there)
> > > and also the new (to be consistent with the old)?
> >
> > There's no need for perfect naming consistency between user and kernel
> > space declarations. The difference is: you need to be sure to get the
> > user-space flags right the first time - unlike the kernel APIs, they
> > cannot be renamed later.
> >
>
> So it may be acceptable to change the gpiolib.h flag names?
>

As I said above - yes, but I'd wait until this series is at least in
Linus' or my for-next branch.

> This is one of those cases where I'd rather ask than guess and not find
> out until the patch gets rejected.
>
> > >
> > > Also, while the DT interface (gpiod_configure_flags) has GPIO_PULL_UP
> > > and GPIO_PULL_DOWN, it doesn't support DISABLE, and it explicitly rejects
> > > both having both PULL_UP and PULL_DOWN set.  Should we be extending the
> > > DISABLE support to the DT interface, and should the API behaviour also
> > > mirror the pinctrl behaviour you describe above?
> >
> > Is someone needing it? Adding new features without users is frowned
> > upon for good reasons.
> >
>
> Not that I am aware of, so we can always add it later.
> And that is why I asked - my first instinct is to keep the APIs
> aligned, especially where we now have the user space API containing
> functionality not available to DT, but I am also aware that might be
> considered unimportant or even unacceptable.
>

In general: we're adding new interfaces when someone needs them, not
"just in case" someone needs them in the future.

> > >
> > > And are there any combinations that are guaranteed to be invalid,
> > > and so should be rejected, like DISABLE + PULL_UP??  In fact are there
> > > any combinations that are valid other then PULL_UP + PULL_DOWN?
> >
> > You mean invalid? You just said PULL_UP + PULL_DOWN is rejected.
> >
>
> No, I my example was  DISABLE + PULL_UP, which I assume is an invalid combo.
> And then on reflection the only valid combo I could think of was
> PULL_UP + PULL_DOWN, and even then only because you said we need it to
> disable bias on some chips.

I think there's some misunderstanding. I said that on some chips there
are bits in control registers that are used to control the bias and
sometimes it's three (pull-up, pull-down and disable) and sometimes
it's two (pull-up, pull-down and both set = disabled) but that we
should only have a single interface in user-space where the
BIAS_DISABLE bit would be translated to whatever the hardware
understands by the driver. So in this case: only one of the bits could
be set.

I understand that this sounds like a task for an enum, but introducing
an enum in a separate field would modify the ABI while simply adding
new mutually-exclusive flags would not. Not to mention that encoding
this enum on two bits of a field that's normally used for flags and is
even called "flags" would introduce a lot of confusion.

I got your new patches - I'll take a look at them shortly.

Bart

>
> Cheers,
> Kent.

Re: [PATCH v2 5/6] gpiolib: disable bias on inputs when pull up/down are both set

[Kent Gibson]

On Mon, Oct 21, 2019 at 04:57:01PM +0200, Bartosz Golaszewski wrote:
> pt., 18 paź 2019 o 12:13 Kent Gibson <warthog618@gmail.com> napisał(a):
> >
> > On Fri, Oct 18, 2019 at 10:03:17AM +0200, Bartosz Golaszewski wrote:
> > > czw., 17 paź 2019 o 07:06 Kent Gibson <warthog618@gmail.com> napisał(a):
> > > >
> > > > On Wed, Oct 16, 2019 at 09:01:04AM +0800, Kent Gibson wrote:
> > > > > On Tue, Oct 15, 2019 at 02:51:18PM +0200, Bartosz Golaszewski wrote:
> > > > > > wt., 15 paź 2019 o 02:58 Kent Gibson <warthog618@gmail.com> napisał(a):
> > > > > > >
> > > > > > > On Mon, Oct 14, 2019 at 06:50:41PM +0200, Bartosz Golaszewski wrote:
> > > > > > > > pon., 14 paź 2019 o 15:04 Kent Gibson <warthog618@gmail.com> napisał(a):
> > > > > > > > >
> > > > > > > > > On Mon, Oct 14, 2019 at 02:43:54PM +0200, Bartosz Golaszewski wrote:
> > > > > > > > > > sob., 12 paź 2019 o 03:57 Kent Gibson <warthog618@gmail.com> napisał(a):
> > > > > > > > > > >
> > > > > > > > > > > This patch allows 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 | 22 +++++++---------------
> > > > > > > > > > >  1 file changed, 7 insertions(+), 15 deletions(-)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > > > > > > > > > index 647334f53622..f90b20d548b9 100644
> > > > > > > > > > > --- a/drivers/gpio/gpiolib.c
> > > > > > > > > > > +++ b/drivers/gpio/gpiolib.c
> > > > > > > > > > > @@ -539,11 +539,6 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
> > > > > > > > > > >             (lflags & GPIOHANDLE_REQUEST_OUTPUT))
> > > > > > > > > > >                 return -EINVAL;
> > > > > > > > > > >
> > > > > > > > > > > -       /* Same with pull-up and pull-down. */
> > > > > > > > > > > -       if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
> > > > > > > > > > > -           (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))
> > > > > > > > > > > -               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
> > > > > > > > > > > @@ -935,14 +930,6 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
> > > > > > > > > > >              (lflags & GPIOHANDLE_REQUEST_PULL_DOWN)))
> > > > > > > > > > >                 return -EINVAL;
> > > > > > > > > > >
> > > > > > > > > > > -       /*
> > > > > > > > > > > -        * Do not allow both pull-up and pull-down flags to be set as they
> > > > > > > > > > > -        *  are contradictory.
> > > > > > > > > > > -        */
> > > > > > > > > > > -       if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
> > > > > > > > > > > -           (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))
> > > > > > > > > > > -               return -EINVAL;
> > > > > > > > > > > -
> > > > > > > > > > >         le = kzalloc(sizeof(*le), GFP_KERNEL);
> > > > > > > > > > >         if (!le)
> > > > > > > > > > >                 return -ENOMEM;
> > > > > > > > > > > @@ -2931,6 +2918,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;
> > > > > > > > > > > @@ -2991,7 +2979,11 @@ int gpiod_direction_input(struct gpio_desc *desc)
> > > > > > > > > > >         if (ret == 0)
> > > > > > > > > > >                 clear_bit(FLAG_IS_OUT, &desc->flags);
> > > > > > > > > > >
> > > > > > > > > > > -       if (test_bit(FLAG_PULL_UP, &desc->flags))
> > > > > > > > > > > +       if (test_bit(FLAG_PULL_UP, &desc->flags) &&
> > > > > > > > > > > +               test_bit(FLAG_PULL_DOWN, &desc->flags))
> > > > > > > > > > > +               gpio_set_config(chip, gpio_chip_hwgpio(desc),
> > > > > > > > > > > +                               PIN_CONFIG_BIAS_DISABLE);
> > > > > > > > > > > +       else if (test_bit(FLAG_PULL_UP, &desc->flags))
> > > > > > > > > >
> > > > > > > > > > From looking at the code: user-space can disable bias when setting
> > > > > > > > > > both PULL_UP and PULL_DOWN flags. I don't understand why it's done in
> > > > > > > > > > this implicit way? Why not a separate flag?
> > > > > > > > >
> > > > > > > > > An extra flag would waste a bit and add nothing but more sanity checking.
> > > > > > > > >
> > > > > > > >
> > > > > > > > I disagree. The user API needs to be very explicit. Sanity checking is
> > > > > > > > alright - if there'll be too many ifdefs, we can start thinking about
> > > > > > > > adding some core library helpers for sanitizing conflicting flags, I'm
> > > > > > > > sure other frameworks could use something like this as well.
> > > > > > > >
> > > > > > > > Especially in this context: setting PULL_UP and PULL_DOWN together
> > > > > > > > disables bias - this doesn't make sense logically.
> > > > > > > >
> > > > > > > In a way it does make a weird kind of sense - they cancel.  Physically.
> > > > > > >
> > > > > >
> > > > > > Yes, on some devices we set both bits to disable bias, but on others
> > > > > > the pull-up and pull-down bits need to be cleared and yet others have
> > > > > > a dedicated bit for that. It's not standardized and the pinctrl
> > > > > > framework defines all three values as separate bits to expose a common
> > > > > > programming interface.
> > > > > >
> > > >
> > > > Is there any documentation on this?  The pinctrl docs stay pretty high
> > > > level and doesn't touch on this. And from the pinconf-generic.h
> > > > documentation, I'd consider drivers that require both pull-up and
> > > > pull-down set to disable bias to be non-compliant with the API - for
> > > > BIAS_DISABLE it says "this setting disables all biasing", so you'd think
> > > > the driver would support that and do any mapping (setting both pulls
> > > > high or low or whatever) internally.
> > > >
> >
> > So no answer for this one?
> >
> > I find it unsettling that we will have a user space API that doesn't
> > provide a definitive way to disable bias, independent of the underlying
> > hardware.  I thought the kernel was all about hardware abstraction?
> >
> 
> If you expose three options to user-space: PULL-UP, PULL-DOWN and
> DISABLE and then that gets translated by the driver to whatever the
> driver understands - how is this not abstraction?
> 

Ok, I misunderstood what you were saying.

> > > > > Ok. And, since gpiolib has no knowledge of what combinations are
> > > > > appropriate for a given chip, we can't provide a higher level
> > > > > abstraction and have no option but to expose that pinconf
> > > > > complexity in the GPIO uapi?
> > > > >
> > > > > In fact, pinconf doesn't just define 3 bias bits - it defines 6:
> > > > >
> > > > > enum pin_config_param {
> > > > >       PIN_CONFIG_BIAS_BUS_HOLD,
> > > > >       PIN_CONFIG_BIAS_DISABLE,
> > > > >       PIN_CONFIG_BIAS_HIGH_IMPEDANCE,
> > > > >       PIN_CONFIG_BIAS_PULL_DOWN,
> > > > >       PIN_CONFIG_BIAS_PULL_PIN_DEFAULT,
> > > > >       PIN_CONFIG_BIAS_PULL_UP,
> > > > >
> > > > > Do we need to support any of the remaining 3 in the GPIO uapi, either
> > > > > now or possibly in the future?
> > > > >
> > > > > And what about the other PIN_CONFIG flags?  Some of these might be
> > > > > candidates for controlling via SET_CONFIG_IOCTL, if not in the request
> > > > > itself? (again this is contemplating the future, not suggesting being part
> > > > > of this patch)
> > > > >
> > > > > > > Did you read the cover letter?  The problem, as I see it,
> > > > > > > is that we're stuck using a flag field to encode a two bit enum.
> > > > > > > That fact the we only have a flag field to play with can't be
> > > > > > > changed due to ABI.
> > > > > >
> > > > > > For some reason I haven't received the cover letter on my inbox. I'm
> > > > > > only now seeing it on linux-gpio archives.
> > > > > >
> > > > > And for some reason I didn't get 0001, yet all 7 parts made it to the mailing
> > > > > list. Spam filters kicking in? Though it isn't in my spam folder either.
> > > > > Something odd going on.
> > > > >
> > > > > > Anyway: I don't understand why you insist on using two instead of
> > > > > > three bits. You have 32 bits in total that can be used and only 5 are
> > > > > > used so far. There's plenty left.
> > > > > >
> > > > > Cos it makes no sense to me to encode 4 values into 3 bits when 2 will
> > > > > do.  But if you want to expose part of the pinconf API within the GPIO
> > > > > uapi then that goes out the window - it's not 4 values anymore.
> > > > >
> > > > > And partly cos I'm frustrated that I'd asked questions regarding how the
> > > > > API should look earlier and got no reply.  This is the sort of thing I
> > > > > usually deal with in the design stage, not review.
> > > > >
> > > > > I realise you guys are busy, but a little time spent clarifying design
> > > > > would save a whole lot more time in coding, testing and review.
> > > > >
> > > > > > I'd prefer to see:
> > > > > >
> > > > > > GPIOHANDLE_REQUEST_PULL_UP
> > > > > > GPIOHANDLE_REQUEST_PULL_DOWN
> > > > > > GPIOHANDLE_REQUEST_PULL_DISABLED
> > > > > >
> > > > > > or maybe even
> > > > > >
> > > > > > GPIOHANDLE_REQUEST_BIAS_PULL_UP
> > > > > > GPIOHANDLE_REQUEST_BIAS_PULL_DOWN
> > > > > > GPIOHANDLE_REQUEST_BIAS_DISABLED
> > > > > >
> > > > > > to stay consistent with the pinctrl flags. No bit set among these
> > > > > > three would mean AS_IS.
> > > > > >
> > > > > That makes sense, if we are exposing the pinctrl API here.
> > > > >
> > > >
> > > > Looking at going with the naming including BIAS...
> > > > What to do with constants defined in headers prior to this patch that
> > > > don't include the BIAS?  e.g. FLAG_PULL_UP and FLAG_PULL_DOWN in gpiolib.h?
> > >
> > > But this has nothing to do with user-space. This was added so that
> > > GPIO expanders can use this without pulling in the pinctrl framework.
> > >
> > > > Safe to assume they can't be renamed?
> > >
> > > What for?
> > >
> >
> > For consistency and clarity.  I need to add a flag into the gpio_desc
> > flags, which are usedf throughout gpiolib.c and aredefined in gpiolib.h,
> > and those FLAG_PULL_UP and FLAG_PULL_DOWN are already there.
> >
> > To be consistent I'd be dropping the GPIOHANDLE_REQUEST_BIAS_ from your
> > preferred names when determining the name for the new flag, but then it
> > would be called FLAG_DISABLE, which is obviously too vague.
> >
> > I intend to use FLAG_BIAS_DISABLE, and for consistency it would be nice
> > to rename FLAG_PULL_UP to FLAG_BIAS_PULL_UP, and similarly PULL_DOWN,
> > but that may break things and so be unacceptable, right?
> >
> 
> Yeah, not for device tree, no. It sounds fine for core gpiolib but I
> wouldn't change it in this series though personally - let's try to
> limit its scope and we can come back to it later.
> 

Agreed, and coded that way in v3.

> > > > So ok to stay with the unBIASed names for both old (cos they are there)
> > > > and also the new (to be consistent with the old)?
> > >
> > > There's no need for perfect naming consistency between user and kernel
> > > space declarations. The difference is: you need to be sure to get the
> > > user-space flags right the first time - unlike the kernel APIs, they
> > > cannot be renamed later.
> > >
> >
> > So it may be acceptable to change the gpiolib.h flag names?
> >
> 
> As I said above - yes, but I'd wait until this series is at least in
> Linus' or my for-next branch.
> 
> > This is one of those cases where I'd rather ask than guess and not find
> > out until the patch gets rejected.
> >
> > > >
> > > > Also, while the DT interface (gpiod_configure_flags) has GPIO_PULL_UP
> > > > and GPIO_PULL_DOWN, it doesn't support DISABLE, and it explicitly rejects
> > > > both having both PULL_UP and PULL_DOWN set.  Should we be extending the
> > > > DISABLE support to the DT interface, and should the API behaviour also
> > > > mirror the pinctrl behaviour you describe above?
> > >
> > > Is someone needing it? Adding new features without users is frowned
> > > upon for good reasons.
> > >
> >
> > Not that I am aware of, so we can always add it later.
> > And that is why I asked - my first instinct is to keep the APIs
> > aligned, especially where we now have the user space API containing
> > functionality not available to DT, but I am also aware that might be
> > considered unimportant or even unacceptable.
> >
> 
> In general: we're adding new interfaces when someone needs them, not
> "just in case" someone needs them in the future.
> 

Ok. That is that way I went with v3 - leaving the DT as it was.

> > > >
> > > > And are there any combinations that are guaranteed to be invalid,
> > > > and so should be rejected, like DISABLE + PULL_UP??  In fact are there
> > > > any combinations that are valid other then PULL_UP + PULL_DOWN?
> > >
> > > You mean invalid? You just said PULL_UP + PULL_DOWN is rejected.
> > >
> >
> > No, I my example was  DISABLE + PULL_UP, which I assume is an invalid combo.
> > And then on reflection the only valid combo I could think of was
> > PULL_UP + PULL_DOWN, and even then only because you said we need it to
> > disable bias on some chips.
> 
> I think there's some misunderstanding. I said that on some chips there
> are bits in control registers that are used to control the bias and
> sometimes it's three (pull-up, pull-down and disable) and sometimes
> it's two (pull-up, pull-down and both set = disabled) but that we
> should only have a single interface in user-space where the
> BIAS_DISABLE bit would be translated to whatever the hardware
> understands by the driver. So in this case: only one of the bits could
> be set.
> 

Ok - I misinterpreted what you were saying.
That answers one of the outstanding issues I mention in the v3 cover 
letter.  For v4 I will add sanity checks that only one bit bias is set.

> I understand that this sounds like a task for an enum, but introducing
> an enum in a separate field would modify the ABI while simply adding
> new mutually-exclusive flags would not. Not to mention that encoding
> this enum on two bits of a field that's normally used for flags and is
> even called "flags" would introduce a lot of confusion.
> 

Considering the vast majority of users will use a library, not the uAPI 
directly, I doubt it would introduce that much confusion.
Though if we want to expose any of the other modes at a later date
the flags do work better. 
And its coded as separate flags now, so what the hell.

> I got your new patches - I'll take a look at them shortly.
> 

I've also got the corresponding updates for libgpiod in a branch on
github now, though not structured as a patch series yet.  I'll tidy them
up and submit them once this series is in.

The updated C and CXX tests pass. The one problem I have is getting 
the Python tests running - they can't import gpiomockup, though I've
built and installed everything.  Not sure what I'm missing there.

Cheers,
Kent.

Re: [PATCH v2 5/6] gpiolib: disable bias on inputs when pull up/down are both set

[Kent Gibson]

On Tue, Oct 22, 2019 at 07:14:56AM +0800, Kent Gibson wrote:
> 
> The updated C and CXX tests pass. The one problem I have is getting 
> the Python tests running - they can't import gpiomockup, though I've
> built and installed everything.  Not sure what I'm missing there.
> 

Sorted - the python bindings get installed into site-packages, but
Debian/Ubuntu doesn't look there, so needed to manually set my
PYTHONPATH. 
The extended Python tests are now passing against v3 as well.

Cheers,
Kent.