[PATCH v4 0/5] gpio: expose line bias flags to userspace

[PATCH v4 0/5] gpio: expose line bias flags to userspace

[Kent Gibson]

The major changes from v3 is the renaming of flags in patch 1, rather 
than in later patches, and the addition of sanity checking on bias flag 
combinations - only allowing one bias flag to be set at a time.

There are still some deficiencies that I'm uncertain on how to best 
resolve:

The setting of bias is performed by gpio_set_bias, which is hooked into 
gpiod_direction_input and gpiod_direction_output.  It extends the setting
of bias that was already present in gpiod_direction_input.
In keeping with the existing behaviour, the bias is set on a best-effort
basis - no error is returned to the user if an error is returned by the
pinctrl driver.  Returning an error makes sense to me, particularly for 
the uAPI, but that conflicts with the existing gpiod_direction_input
behaviour. So leave as best-effort, change gpiod_direction_input 
behaviour, or restructure to support both behaviours?

Also, the gpio_set_bias is hooked into gpiod_direction_output, which is 
fine for the uAPI, but perhaps it should be hooked into 
gpiod_direction_output_raw_commit?  Or the setting of direction 
and bias should be decoupled?

And now the actual blurb...

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

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

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

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

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

The patch has been successfully tested against gpio-mockup, and 
on a Raspberry Pi, in both cases using the feature/pud branch of my Go 
gpiod library[2], as well as with my feature/pud development branch 
of libgpiod[3].

Patch 1 adds support to line handle requests.
Patch 2 adds support to line event requests and restricts bias settings 
to lines explicitly requested as inputs.
Patch 3 adds pull-up/down support to the gpio-mockup for uAPI testing.
Patch 4 adds support for disabling bias.
Patch 5 adds support for setting bias on output lines.

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

Kent Gibson (4):
  gpiolib: add support for pull up/down to lineevent_create
  gpio: mockup: add set_config to support pull up/down
  gpiolib: add support for disabling line bias
  gpiolib: add support for biasing output lines

 drivers/gpio/gpio-mockup.c | 94 ++++++++++++++++++++++++--------------
 drivers/gpio/gpiolib.c     | 81 +++++++++++++++++++++++++++++---
 drivers/gpio/gpiolib.h     |  1 +
 include/uapi/linux/gpio.h  |  6 +++
 4 files changed, 142 insertions(+), 40 deletions(-)

-- 
2.23.0

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

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

[Kent Gibson]

From: Drew Fustini <drew@pdp7.com>

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

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

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

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index e8964493c571..e80e689be2cc 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_BIAS_PULL_UP | \
+	GPIOHANDLE_REQUEST_BIAS_PULL_DOWN | \
 	GPIOHANDLE_REQUEST_OPEN_DRAIN | \
 	GPIOHANDLE_REQUEST_OPEN_SOURCE)
 
@@ -592,6 +594,10 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 			set_bit(FLAG_OPEN_DRAIN, &desc->flags);
 		if (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE)
 			set_bit(FLAG_OPEN_SOURCE, &desc->flags);
+		if (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN)
+			set_bit(FLAG_PULL_DOWN, &desc->flags);
+		if (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP)
+			set_bit(FLAG_PULL_UP, &desc->flags);
 
 		ret = gpiod_set_transitory(desc, false);
 		if (ret < 0)
@@ -1091,6 +1097,10 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		if (test_bit(FLAG_OPEN_SOURCE, &desc->flags))
 			lineinfo.flags |= (GPIOLINE_FLAG_OPEN_SOURCE |
 					   GPIOLINE_FLAG_IS_OUT);
+		if (test_bit(FLAG_PULL_DOWN, &desc->flags))
+			lineinfo.flags |= GPIOLINE_FLAG_BIAS_PULL_DOWN;
+		if (test_bit(FLAG_PULL_UP, &desc->flags))
+			lineinfo.flags |= GPIOLINE_FLAG_BIAS_PULL_UP;
 
 		if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
 			return -EFAULT;
@@ -2764,6 +2774,8 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
 		clear_bit(FLAG_REQUESTED, &desc->flags);
 		clear_bit(FLAG_OPEN_DRAIN, &desc->flags);
 		clear_bit(FLAG_OPEN_SOURCE, &desc->flags);
+		clear_bit(FLAG_PULL_UP, &desc->flags);
+		clear_bit(FLAG_PULL_DOWN, &desc->flags);
 		clear_bit(FLAG_IS_HOGGED, &desc->flags);
 		ret = true;
 	}
diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
index 4ebfe0ac6c5b..39e6c7854d63 100644
--- a/include/uapi/linux/gpio.h
+++ b/include/uapi/linux/gpio.h
@@ -33,6 +33,8 @@ struct gpiochip_info {
 #define GPIOLINE_FLAG_ACTIVE_LOW	(1UL << 2)
 #define GPIOLINE_FLAG_OPEN_DRAIN	(1UL << 3)
 #define GPIOLINE_FLAG_OPEN_SOURCE	(1UL << 4)
+#define GPIOLINE_FLAG_BIAS_PULL_UP	(1UL << 5)
+#define GPIOLINE_FLAG_BIAS_PULL_DOWN	(1UL << 6)
 
 /**
  * struct gpioline_info - Information about a certain GPIO line
@@ -62,6 +64,8 @@ struct gpioline_info {
 #define GPIOHANDLE_REQUEST_ACTIVE_LOW	(1UL << 2)
 #define GPIOHANDLE_REQUEST_OPEN_DRAIN	(1UL << 3)
 #define GPIOHANDLE_REQUEST_OPEN_SOURCE	(1UL << 4)
+#define GPIOHANDLE_REQUEST_BIAS_PULL_UP	(1UL << 5)
+#define GPIOHANDLE_REQUEST_BIAS_PULL_DOWN	(1UL << 6)
 
 /**
  * struct gpiohandle_request - Information about a GPIO handle request
-- 
2.23.0

[PATCH v4 2/5] gpiolib: add support for pull up/down to lineevent_create

[Kent Gibson]

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

Also restrict the application of bias to lines
explicitly requested as inputs to prevent bias being applied
to as-is line requests.

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

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index e80e689be2cc..7dfbb3676ee0 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -554,6 +554,12 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 	     (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE)))
 		return -EINVAL;
 
+	/* PULL_UP and PULL_DOWN flags only make sense for input mode. */
+	if (!(lflags & GPIOHANDLE_REQUEST_INPUT) &&
+	    ((lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP) ||
+	     (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN)))
+		return -EINVAL;
+
 	lh = kzalloc(sizeof(*lh), GFP_KERNEL);
 	if (!lh)
 		return -ENOMEM;
@@ -944,6 +950,10 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 
 	if (lflags & GPIOHANDLE_REQUEST_ACTIVE_LOW)
 		set_bit(FLAG_ACTIVE_LOW, &desc->flags);
+	if (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN)
+		set_bit(FLAG_PULL_DOWN, &desc->flags);
+	if (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP)
+		set_bit(FLAG_PULL_UP, &desc->flags);
 
 	ret = gpiod_direction_input(desc);
 	if (ret)
-- 
2.23.0

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

[Bartosz Golaszewski]

pon., 28 paź 2019 o 08:38 Kent Gibson <warthog618@gmail.com> napisał(a):
>
> Add support for pull up/down to lineevent_create.
> Use cases include receiving asynchronous presses from a
> push button without an external pull up/down.
>
> Also restrict the application of bias to lines
> explicitly requested as inputs to prevent bias being applied
> to as-is line requests.
>
> Signed-off-by: Kent Gibson <warthog618@gmail.com>
> ---
>  drivers/gpio/gpiolib.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index e80e689be2cc..7dfbb3676ee0 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -554,6 +554,12 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
>              (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE)))
>                 return -EINVAL;
>
> +       /* PULL_UP and PULL_DOWN flags only make sense for input mode. */
> +       if (!(lflags & GPIOHANDLE_REQUEST_INPUT) &&
> +           ((lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP) ||
> +            (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN)))
> +               return -EINVAL;
> +

This is not part of this patch - this must go into 1/5.

Bart

>         lh = kzalloc(sizeof(*lh), GFP_KERNEL);
>         if (!lh)
>                 return -ENOMEM;
> @@ -944,6 +950,10 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
>
>         if (lflags & GPIOHANDLE_REQUEST_ACTIVE_LOW)
>                 set_bit(FLAG_ACTIVE_LOW, &desc->flags);
> +       if (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN)
> +               set_bit(FLAG_PULL_DOWN, &desc->flags);
> +       if (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP)
> +               set_bit(FLAG_PULL_UP, &desc->flags);
>

Bart.


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

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

[Kent Gibson]

On Thu, Oct 31, 2019 at 08:11:12AM +0100, Bartosz Golaszewski wrote:
> pon., 28 paź 2019 o 08:38 Kent Gibson <warthog618@gmail.com> napisał(a):
> >
> > Add support for pull up/down to lineevent_create.
> > Use cases include receiving asynchronous presses from a
> > push button without an external pull up/down.
> >
> > Also restrict the application of bias to lines
> > explicitly requested as inputs to prevent bias being applied
> > to as-is line requests.
> >
> > Signed-off-by: Kent Gibson <warthog618@gmail.com>
> > ---
> >  drivers/gpio/gpiolib.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index e80e689be2cc..7dfbb3676ee0 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -554,6 +554,12 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
> >              (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE)))
> >                 return -EINVAL;
> >
> > +       /* PULL_UP and PULL_DOWN flags only make sense for input mode. */
> > +       if (!(lflags & GPIOHANDLE_REQUEST_INPUT) &&
> > +           ((lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP) ||
> > +            (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN)))
> > +               return -EINVAL;
> > +
> 
> This is not part of this patch - this must go into 1/5.
> 
Yeah, that would make more sense.  Will relocate it in v5.

Cheers,
Kent.

[PATCH v4 3/5] gpio: mockup: add set_config to support pull up/down

[Kent Gibson]

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

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

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

[PATCH v4 4/5] gpiolib: add support for disabling line bias

[Kent Gibson]

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

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

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 7dfbb3676ee0..177d25e19758 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -423,6 +423,7 @@ struct linehandle_state {
 	GPIOHANDLE_REQUEST_ACTIVE_LOW | \
 	GPIOHANDLE_REQUEST_BIAS_PULL_UP | \
 	GPIOHANDLE_REQUEST_BIAS_PULL_DOWN | \
+	GPIOHANDLE_REQUEST_BIAS_DISABLE | \
 	GPIOHANDLE_REQUEST_OPEN_DRAIN | \
 	GPIOHANDLE_REQUEST_OPEN_SOURCE)
 
@@ -554,12 +555,21 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 	     (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE)))
 		return -EINVAL;
 
-	/* PULL_UP and PULL_DOWN flags only make sense for input mode. */
+	/* Bias flags only allowed for input mode. */
 	if (!(lflags & GPIOHANDLE_REQUEST_INPUT) &&
-	    ((lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP) ||
+	    ((lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE) ||
+	     (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP) ||
 	     (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN)))
 		return -EINVAL;
 
+	/* Only one bias flag can be set. */
+	if (((lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE) &&
+	     (lflags & (GPIOHANDLE_REQUEST_BIAS_PULL_DOWN |
+			GPIOHANDLE_REQUEST_BIAS_PULL_UP))) ||
+	    ((lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN) &&
+	     (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP)))
+		return -EINVAL;
+
 	lh = kzalloc(sizeof(*lh), GFP_KERNEL);
 	if (!lh)
 		return -ENOMEM;
@@ -600,6 +610,8 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 			set_bit(FLAG_OPEN_DRAIN, &desc->flags);
 		if (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE)
 			set_bit(FLAG_OPEN_SOURCE, &desc->flags);
+		if (lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE)
+			set_bit(FLAG_BIAS_DISABLE, &desc->flags);
 		if (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN)
 			set_bit(FLAG_PULL_DOWN, &desc->flags);
 		if (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP)
@@ -924,6 +936,21 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 	    (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE))
 		return -EINVAL;
 
+	/* Bias flags only make sense for input mode. */
+	if (!(lflags & GPIOHANDLE_REQUEST_INPUT) &&
+	    ((lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE) ||
+	     (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN) ||
+	     (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP)))
+		return -EINVAL;
+
+	/* Only one bias flag can be set. */
+	if (((lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE) &&
+	     (lflags & (GPIOHANDLE_REQUEST_BIAS_PULL_DOWN |
+			GPIOHANDLE_REQUEST_BIAS_PULL_UP))) ||
+	    ((lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN) &&
+	     (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP)))
+		return -EINVAL;
+
 	le = kzalloc(sizeof(*le), GFP_KERNEL);
 	if (!le)
 		return -ENOMEM;
@@ -950,6 +977,8 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 
 	if (lflags & GPIOHANDLE_REQUEST_ACTIVE_LOW)
 		set_bit(FLAG_ACTIVE_LOW, &desc->flags);
+	if (lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE)
+		set_bit(FLAG_BIAS_DISABLE, &desc->flags);
 	if (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN)
 		set_bit(FLAG_PULL_DOWN, &desc->flags);
 	if (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP)
@@ -1107,6 +1136,8 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		if (test_bit(FLAG_OPEN_SOURCE, &desc->flags))
 			lineinfo.flags |= (GPIOLINE_FLAG_OPEN_SOURCE |
 					   GPIOLINE_FLAG_IS_OUT);
+		if (test_bit(FLAG_BIAS_DISABLE, &desc->flags))
+			lineinfo.flags |= GPIOLINE_FLAG_BIAS_DISABLE;
 		if (test_bit(FLAG_PULL_DOWN, &desc->flags))
 			lineinfo.flags |= GPIOLINE_FLAG_BIAS_PULL_DOWN;
 		if (test_bit(FLAG_PULL_UP, &desc->flags))
@@ -2786,6 +2817,7 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
 		clear_bit(FLAG_OPEN_SOURCE, &desc->flags);
 		clear_bit(FLAG_PULL_UP, &desc->flags);
 		clear_bit(FLAG_PULL_DOWN, &desc->flags);
+		clear_bit(FLAG_BIAS_DISABLE, &desc->flags);
 		clear_bit(FLAG_IS_HOGGED, &desc->flags);
 		ret = true;
 	}
@@ -2912,6 +2944,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;
@@ -2925,6 +2958,23 @@ 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_BIAS_DISABLE, &desc->flags))
+		bias |= PIN_CONFIG_BIAS_DISABLE;
+	if (test_bit(FLAG_PULL_UP, &desc->flags))
+		bias |= PIN_CONFIG_BIAS_PULL_UP;
+	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
@@ -2972,12 +3022,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))
-		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);
 
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index b8b10a409c7b..ca9bc1e4803c 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -110,6 +110,7 @@ struct gpio_desc {
 #define FLAG_TRANSITORY 12	/* GPIO may lose value in sleep or reset */
 #define FLAG_PULL_UP    13	/* GPIO has pull up enabled */
 #define FLAG_PULL_DOWN  14	/* GPIO has pull down enabled */
+#define FLAG_BIAS_DISABLE    15	/* GPIO has pull disabled */
 
 	/* Connection label */
 	const char		*label;
diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
index 39e6c7854d63..7cc21c3b0839 100644
--- a/include/uapi/linux/gpio.h
+++ b/include/uapi/linux/gpio.h
@@ -35,6 +35,7 @@ struct gpiochip_info {
 #define GPIOLINE_FLAG_OPEN_SOURCE	(1UL << 4)
 #define GPIOLINE_FLAG_BIAS_PULL_UP	(1UL << 5)
 #define GPIOLINE_FLAG_BIAS_PULL_DOWN	(1UL << 6)
+#define GPIOLINE_FLAG_BIAS_DISABLE	(1UL << 7)
 
 /**
  * struct gpioline_info - Information about a certain GPIO line
@@ -66,6 +67,7 @@ struct gpioline_info {
 #define GPIOHANDLE_REQUEST_OPEN_SOURCE	(1UL << 4)
 #define GPIOHANDLE_REQUEST_BIAS_PULL_UP	(1UL << 5)
 #define GPIOHANDLE_REQUEST_BIAS_PULL_DOWN	(1UL << 6)
+#define GPIOHANDLE_REQUEST_BIAS_DISABLE	(1UL << 7)
 
 /**
  * struct gpiohandle_request - Information about a GPIO handle request
-- 
2.23.0

[PATCH v4 5/5] gpiolib: add support for biasing output lines

[Kent Gibson]

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

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

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 177d25e19758..56b2814b843e 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -555,8 +555,9 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 	     (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE)))
 		return -EINVAL;
 
-	/* Bias flags only allowed for input mode. */
-	if (!(lflags & GPIOHANDLE_REQUEST_INPUT) &&
+	/* Bias flags only allowed for input or output mode. */
+	if (!((lflags & GPIOHANDLE_REQUEST_INPUT) ||
+	      (lflags & GPIOHANDLE_REQUEST_OUTPUT)) &&
 	    ((lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE) ||
 	     (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP) ||
 	     (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN)))
@@ -3148,6 +3149,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 v4 0/5] gpio: expose line bias flags to userspace

[Bartosz Golaszewski]

pon., 28 paź 2019 o 08:37 Kent Gibson <warthog618@gmail.com> napisał(a):
>
> The major changes from v3 is the renaming of flags in patch 1, rather
> than in later patches, and the addition of sanity checking on bias flag
> combinations - only allowing one bias flag to be set at a time.
>
> There are still some deficiencies that I'm uncertain on how to best
> resolve:
>
> The setting of bias is performed by gpio_set_bias, which is hooked into
> gpiod_direction_input and gpiod_direction_output.  It extends the setting
> of bias that was already present in gpiod_direction_input.
> In keeping with the existing behaviour, the bias is set on a best-effort
> basis - no error is returned to the user if an error is returned by the
> pinctrl driver.  Returning an error makes sense to me, particularly for
> the uAPI, but that conflicts with the existing gpiod_direction_input
> behaviour. So leave as best-effort, change gpiod_direction_input
> behaviour, or restructure to support both behaviours?

Thomas: is there any reason not to check the return value of these
calls for errors other than -EOPNOTSUPP?

>
> Also, the gpio_set_bias is hooked into gpiod_direction_output, which is
> fine for the uAPI, but perhaps it should be hooked into
> gpiod_direction_output_raw_commit?  Or the setting of direction
> and bias should be decoupled?
>
> And now the actual blurb...
>
> This series adds gross control of pull-up/pull-down to the GPIO uAPI.
> Gross control means enabling and disabling of bias functionality,
> not finer grained control such as setting biasing impedances.
>
> The support allows both input and output lines to have any one of the
> following biases applied as part of the line handle or event request:
>  0. As Is - bias is left alone.  This is the default for ABI compatibility.
>  1. Bias Disable - bias is explicitly disabled.
>  2. Pull Down - pull-down bias is enabled.
>  3. Pull Up - pull-up bias is enabled.
>
> The biases are set via three flags, BIAS_DISABLE, BIAS_PULL_DOWN
> and BIAS_PULL_UP.  These map directly to the similarly named
> pinctrl pin_config_param flags.
> As Is corresponds to none of the flags being set.
>
> The setting of biases on output lines may seem odd, but is to allow for
> utilisation of internal pull-up/pull-down on open drain and open source
> outputs, where supported in hardware.
>
> Patches are against Bart's gpio/for-next branch[1].
>
> The patch has been successfully tested against gpio-mockup, and
> on a Raspberry Pi, in both cases using the feature/pud branch of my Go
> gpiod library[2], as well as with my feature/pud development branch
> of libgpiod[3].
>
> Patch 1 adds support to line handle requests.
> Patch 2 adds support to line event requests and restricts bias settings
> to lines explicitly requested as inputs.
> Patch 3 adds pull-up/down support to the gpio-mockup for uAPI testing.
> Patch 4 adds support for disabling bias.
> Patch 5 adds support for setting bias on output lines.
>
> Drew Fustini (1):
>   gpio: expose pull-up/pull-down line flags to userspace
>
> Kent Gibson (4):
>   gpiolib: add support for pull up/down to lineevent_create
>   gpio: mockup: add set_config to support pull up/down
>   gpiolib: add support for disabling line bias
>   gpiolib: add support for biasing output lines
>
>  drivers/gpio/gpio-mockup.c | 94 ++++++++++++++++++++++++--------------
>  drivers/gpio/gpiolib.c     | 81 +++++++++++++++++++++++++++++---
>  drivers/gpio/gpiolib.h     |  1 +
>  include/uapi/linux/gpio.h  |  6 +++
>  4 files changed, 142 insertions(+), 40 deletions(-)
>
> --
> 2.23.0
>
> [1] git://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git
> [2] https://github.com/warthog618/gpiod.git
> [3] https://github.com/warthog618/libgpiod.git

I think this starts to look good. There are some nits and I'd like to
also hear an opinion from Linus on that, but I personally like that
and think it'll be useful.

Bart

Re: [PATCH v4 0/5] gpio: expose line bias flags to userspace

[Linus Walleij]

On Thu, Oct 31, 2019 at 8:10 AM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:

> [Kent]
> > 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.

Right, excellent and persistent work here, much appreciated!

As long as I get Bartosz's blanket ACK on v5 I think it is ready
to merge. His consent is required for this.

It looks pretty much as I imagined it when I discussed it with
Drew some while back, with some gritty details fixed up.

Yours,
Linus Walleij

Re: [PATCH v4 0/5] gpio: expose line bias flags to userspace

[Kent Gibson]

On Mon, Nov 04, 2019 at 01:26:54AM +0100, Linus Walleij wrote:
> On Thu, Oct 31, 2019 at 8:10 AM Bartosz Golaszewski
> <bgolaszewski@baylibre.com> wrote:
> 
> > [Kent]
> > > 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.
> 
> Right, excellent and persistent work here, much appreciated!
> 

No problem - hopefully I haven't irritated too many people in the process.

> As long as I get Bartosz's blanket ACK on v5 I think it is ready
> to merge. His consent is required for this.
> 

I'm still waiting on open questions from v4 before submitting v5:

One, handling of errors when setting bias, Bart has referred to Thomas, 
so waiting for feedback on that.

The other, where gpio_set_bias is hooked into gpiod_direction_output,
is fine as is for the UAPI - it can always be relocated subsequently if 
other APIs need to set bias.  On the other hand, if decoupling setting 
direction and bias is in order then that really should be done now.
Can I get an an ACK on that either way?

I've also made a couple of minor changes myself while reviewing v4 - 
reordering the patches to group the gpiolib.c ones and leaving the 
gpio-mockup til last, and removing the "bias requires input mode" check 
from lineevent_create as the line is assumed to be input for events 
regardless of the input flag - there is no such thing as as-is for
event requests.
Only mentioning here in case such changes are clearly wrong...

Cheers,
Kent.

> It looks pretty much as I imagined it when I discussed it with
> Drew some while back, with some gritty details fixed up.
> 
> Yours,
> Linus Walleij

Re: [PATCH v4 0/5] gpio: expose line bias flags to userspace

[Bartosz Golaszewski]

pon., 4 lis 2019 o 02:07 Kent Gibson <warthog618@gmail.com> napisał(a):
>
> On Mon, Nov 04, 2019 at 01:26:54AM +0100, Linus Walleij wrote:
> > On Thu, Oct 31, 2019 at 8:10 AM Bartosz Golaszewski
> > <bgolaszewski@baylibre.com> wrote:
> >
> > > [Kent]
> > > > 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.
> >
> > Right, excellent and persistent work here, much appreciated!
> >
>
> No problem - hopefully I haven't irritated too many people in the process.
>
> > As long as I get Bartosz's blanket ACK on v5 I think it is ready
> > to merge. His consent is required for this.
> >
>
> I'm still waiting on open questions from v4 before submitting v5:
>
> One, handling of errors when setting bias, Bart has referred to Thomas,
> so waiting for feedback on that.
>

If we can get it merged for v5.5, then I don't want to block it
waiting for answers. Looking at the code I think we should only ignore
the EOPNOTSUPP error and propagate all other codes. Can you add a
patch changing that and then fix the other nits I pointed out? Also:
please Cc Thomas Petazzoni so that he gets the chance to yell at us if
it breaks something.

> The other, where gpio_set_bias is hooked into gpiod_direction_output,
> is fine as is for the UAPI - it can always be relocated subsequently if
> other APIs need to set bias.  On the other hand, if decoupling setting
> direction and bias is in order then that really should be done now.
> Can I get an an ACK on that either way?
>

This is in line with what you did for input. I don't think it should
be decoupled (any particular reason for that? There is none mentioned
in the cover letter), so I propose to leave it as it is in patch 5/5.

One more thing - since we all want this to make it for v5.5 - can you
make the set config patches part of this series (simply bunch it all
together)? This will make it easy to review and merge everything.

Thanks in advance and great job!
Bartosz

> I've also made a couple of minor changes myself while reviewing v4 -
> reordering the patches to group the gpiolib.c ones and leaving the
> gpio-mockup til last, and removing the "bias requires input mode" check
> from lineevent_create as the line is assumed to be input for events
> regardless of the input flag - there is no such thing as as-is for
> event requests.
> Only mentioning here in case such changes are clearly wrong...
>
> Cheers,
> Kent.
>
> > It looks pretty much as I imagined it when I discussed it with
> > Drew some while back, with some gritty details fixed up.
> >
> > Yours,
> > Linus Walleij

Re: [PATCH v4 0/5] gpio: expose line bias flags to userspace

[Kent Gibson]

On Mon, Nov 04, 2019 at 11:14:56AM +0100, Bartosz Golaszewski wrote:
> pon., 4 lis 2019 o 02:07 Kent Gibson <warthog618@gmail.com> napisał(a):
> >
> > On Mon, Nov 04, 2019 at 01:26:54AM +0100, Linus Walleij wrote:
> > > On Thu, Oct 31, 2019 at 8:10 AM Bartosz Golaszewski
> > > <bgolaszewski@baylibre.com> wrote:
> > >
> > > > [Kent]
> > > > > 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.
> > >
> > > Right, excellent and persistent work here, much appreciated!
> > >
> >
> > No problem - hopefully I haven't irritated too many people in the process.
> >
> > > As long as I get Bartosz's blanket ACK on v5 I think it is ready
> > > to merge. His consent is required for this.
> > >
> >
> > I'm still waiting on open questions from v4 before submitting v5:
> >
> > One, handling of errors when setting bias, Bart has referred to Thomas,
> > so waiting for feedback on that.
> >
> 
> If we can get it merged for v5.5, then I don't want to block it
> waiting for answers. Looking at the code I think we should only ignore
> the EOPNOTSUPP error and propagate all other codes. Can you add a
> patch changing that and then fix the other nits I pointed out? Also:
> please Cc Thomas Petazzoni so that he gets the chance to yell at us if
> it breaks something.
> 

Will do.

> > The other, where gpio_set_bias is hooked into gpiod_direction_output,
> > is fine as is for the UAPI - it can always be relocated subsequently if
> > other APIs need to set bias.  On the other hand, if decoupling setting
> > direction and bias is in order then that really should be done now.
> > Can I get an an ACK on that either way?
> >
> 
> This is in line with what you did for input. I don't think it should
> be decoupled (any particular reason for that? There is none mentioned
> in the cover letter), so I propose to leave it as it is in patch 5/5.
> 

Wrt decoupling, I was thinking of both input and output, not just output,
though it was the output that got me onto that line of thought as
gpiod_direction_output sets bias, but gpiod_direction_output_raw doesn't.
That seemed totally arbitrary.

That lead to thinking that the gpiod_direction_output (and _input) is now 
doing more than implied by the name, and by the documentation for that 
matter.  So perhaps it should be split out and promote gpio_set_bias to
gpiod_set_bias?  Anyway, that was the line of thought.
The problem there being some callers of gpiod_direction_input already
expect it to set bias, at least on a best effort basis, and they would 
have to be updated to call gpiod_set_bias.

Maybe just update the documentation for exported functions that do set 
bias?

> One more thing - since we all want this to make it for v5.5 - can you
> make the set config patches part of this series (simply bunch it all
> together)? This will make it easy to review and merge everything.
> 

May as well - I've got it all in the one branch anyway.

> Thanks in advance and great job!

I was about to start updating libgpiod to add set_config, after adding
the equivalent to my gpiod library (nearly finished writing the tests 
for that), but I'll put all that on the back burner and get v5,
including in the set_config patches, out as soon as I can.

Cheers,
Kent.

> Bartosz
> 
> > I've also made a couple of minor changes myself while reviewing v4 -
> > reordering the patches to group the gpiolib.c ones and leaving the
> > gpio-mockup til last, and removing the "bias requires input mode" check
> > from lineevent_create as the line is assumed to be input for events
> > regardless of the input flag - there is no such thing as as-is for
> > event requests.
> > Only mentioning here in case such changes are clearly wrong...
> >
> > Cheers,
> > Kent.
> >
> > > It looks pretty much as I imagined it when I discussed it with
> > > Drew some while back, with some gritty details fixed up.
> > >
> > > Yours,
> > > Linus Walleij

Re: [PATCH v4 0/5] gpio: expose line bias flags to userspace

[Bartosz Golaszewski]

pon., 4 lis 2019 o 12:12 Kent Gibson <warthog618@gmail.com> napisał(a):
>
> On Mon, Nov 04, 2019 at 11:14:56AM +0100, Bartosz Golaszewski wrote:
> > pon., 4 lis 2019 o 02:07 Kent Gibson <warthog618@gmail.com> napisał(a):
> > >
> > > On Mon, Nov 04, 2019 at 01:26:54AM +0100, Linus Walleij wrote:
> > > > On Thu, Oct 31, 2019 at 8:10 AM Bartosz Golaszewski
> > > > <bgolaszewski@baylibre.com> wrote:
> > > >
> > > > > [Kent]
> > > > > > 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.
> > > >
> > > > Right, excellent and persistent work here, much appreciated!
> > > >
> > >
> > > No problem - hopefully I haven't irritated too many people in the process.
> > >
> > > > As long as I get Bartosz's blanket ACK on v5 I think it is ready
> > > > to merge. His consent is required for this.
> > > >
> > >
> > > I'm still waiting on open questions from v4 before submitting v5:
> > >
> > > One, handling of errors when setting bias, Bart has referred to Thomas,
> > > so waiting for feedback on that.
> > >
> >
> > If we can get it merged for v5.5, then I don't want to block it
> > waiting for answers. Looking at the code I think we should only ignore
> > the EOPNOTSUPP error and propagate all other codes. Can you add a
> > patch changing that and then fix the other nits I pointed out? Also:
> > please Cc Thomas Petazzoni so that he gets the chance to yell at us if
> > it breaks something.
> >
>
> Will do.
>
> > > The other, where gpio_set_bias is hooked into gpiod_direction_output,
> > > is fine as is for the UAPI - it can always be relocated subsequently if
> > > other APIs need to set bias.  On the other hand, if decoupling setting
> > > direction and bias is in order then that really should be done now.
> > > Can I get an an ACK on that either way?
> > >
> >
> > This is in line with what you did for input. I don't think it should
> > be decoupled (any particular reason for that? There is none mentioned
> > in the cover letter), so I propose to leave it as it is in patch 5/5.
> >
>
> Wrt decoupling, I was thinking of both input and output, not just output,
> though it was the output that got me onto that line of thought as
> gpiod_direction_output sets bias, but gpiod_direction_output_raw doesn't.
> That seemed totally arbitrary.
>
> That lead to thinking that the gpiod_direction_output (and _input) is now
> doing more than implied by the name, and by the documentation for that
> matter.  So perhaps it should be split out and promote gpio_set_bias to
> gpiod_set_bias?  Anyway, that was the line of thought.
> The problem there being some callers of gpiod_direction_input already
> expect it to set bias, at least on a best effort basis, and they would
> have to be updated to call gpiod_set_bias.
>

I see. I think that in this case, the _raw variants should stay as
simple as possible (hence the name) while the bias *should* be set in
the regular gpiod_direction_intput()/output(). For now I don't think
we need an exported gpiod_set_bias(), but if someone should request it
in the future it will be straightforward to add.

> Maybe just update the documentation for exported functions that do set
> bias?

Sure, sounds good. You can even extend the doc to include other flags
these functions handle.

>
> > One more thing - since we all want this to make it for v5.5 - can you
> > make the set config patches part of this series (simply bunch it all
> > together)? This will make it easy to review and merge everything.
> >
>
> May as well - I've got it all in the one branch anyway.
>
> > Thanks in advance and great job!
>
> I was about to start updating libgpiod to add set_config, after adding
> the equivalent to my gpiod library (nearly finished writing the tests
> for that), but I'll put all that on the back burner and get v5,
> including in the set_config patches, out as soon as I can.
>

Let me know if you still want to do it and you'll have patches ready
before v5.5 is released. Otherwise I can do it myself too if needed.

Bart

> Cheers,
> Kent.
>
> > Bartosz
> >
> > > I've also made a couple of minor changes myself while reviewing v4 -
> > > reordering the patches to group the gpiolib.c ones and leaving the
> > > gpio-mockup til last, and removing the "bias requires input mode" check
> > > from lineevent_create as the line is assumed to be input for events
> > > regardless of the input flag - there is no such thing as as-is for
> > > event requests.
> > > Only mentioning here in case such changes are clearly wrong...
> > >
> > > Cheers,
> > > Kent.
> > >
> > > > It looks pretty much as I imagined it when I discussed it with
> > > > Drew some while back, with some gritty details fixed up.
> > > >
> > > > Yours,
> > > > Linus Walleij

Re: [PATCH v4 0/5] gpio: expose line bias flags to userspace

[Kent Gibson]

On Mon, Nov 04, 2019 at 12:48:33PM +0100, Bartosz Golaszewski wrote:
> pon., 4 lis 2019 o 12:12 Kent Gibson <warthog618@gmail.com> napisał(a):
> >
> > On Mon, Nov 04, 2019 at 11:14:56AM +0100, Bartosz Golaszewski wrote:
> > > pon., 4 lis 2019 o 02:07 Kent Gibson <warthog618@gmail.com> napisał(a):
> > > >
> > > > On Mon, Nov 04, 2019 at 01:26:54AM +0100, Linus Walleij wrote:
> > > > > On Thu, Oct 31, 2019 at 8:10 AM Bartosz Golaszewski
> > > > > <bgolaszewski@baylibre.com> wrote:
> > > > >
> > > > > > [Kent]
> > > > > > > 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.
> > > > >
> > > > > Right, excellent and persistent work here, much appreciated!
> > > > >
> > > >
> > > > No problem - hopefully I haven't irritated too many people in the process.
> > > >
> > > > > As long as I get Bartosz's blanket ACK on v5 I think it is ready
> > > > > to merge. His consent is required for this.
> > > > >
> > > >
> > > > I'm still waiting on open questions from v4 before submitting v5:
> > > >
> > > > One, handling of errors when setting bias, Bart has referred to Thomas,
> > > > so waiting for feedback on that.
> > > >
> > >
> > > If we can get it merged for v5.5, then I don't want to block it
> > > waiting for answers. Looking at the code I think we should only ignore
> > > the EOPNOTSUPP error and propagate all other codes. Can you add a
> > > patch changing that and then fix the other nits I pointed out? Also:
> > > please Cc Thomas Petazzoni so that he gets the chance to yell at us if
> > > it breaks something.
> > >
> >

Can you just confirm if it is EOPNOTSUPP or ENOTSUPP that you want ignored?

> > Will do.
> >
> > > > The other, where gpio_set_bias is hooked into gpiod_direction_output,
> > > > is fine as is for the UAPI - it can always be relocated subsequently if
> > > > other APIs need to set bias.  On the other hand, if decoupling setting
> > > > direction and bias is in order then that really should be done now.
> > > > Can I get an an ACK on that either way?
> > > >
> > >
> > > This is in line with what you did for input. I don't think it should
> > > be decoupled (any particular reason for that? There is none mentioned
> > > in the cover letter), so I propose to leave it as it is in patch 5/5.
> > >
> >
> > Wrt decoupling, I was thinking of both input and output, not just output,
> > though it was the output that got me onto that line of thought as
> > gpiod_direction_output sets bias, but gpiod_direction_output_raw doesn't.
> > That seemed totally arbitrary.
> >
> > That lead to thinking that the gpiod_direction_output (and _input) is now
> > doing more than implied by the name, and by the documentation for that
> > matter.  So perhaps it should be split out and promote gpio_set_bias to
> > gpiod_set_bias?  Anyway, that was the line of thought.
> > The problem there being some callers of gpiod_direction_input already
> > expect it to set bias, at least on a best effort basis, and they would
> > have to be updated to call gpiod_set_bias.
> >
> 
> I see. I think that in this case, the _raw variants should stay as
> simple as possible (hence the name) while the bias *should* be set in
> the regular gpiod_direction_intput()/output(). For now I don't think
> we need an exported gpiod_set_bias(), but if someone should request it
> in the future it will be straightforward to add.
> 

OK.

> > Maybe just update the documentation for exported functions that do set
> > bias?
> 
> Sure, sounds good. You can even extend the doc to include other flags
> these functions handle.
> 

OK to add this later, or does it need to be in this series?

> >
> > > One more thing - since we all want this to make it for v5.5 - can you
> > > make the set config patches part of this series (simply bunch it all
> > > together)? This will make it easy to review and merge everything.
> > >
> >
> > May as well - I've got it all in the one branch anyway.
> >
> > > Thanks in advance and great job!
> >
> > I was about to start updating libgpiod to add set_config, after adding
> > the equivalent to my gpiod library (nearly finished writing the tests
> > for that), but I'll put all that on the back burner and get v5,
> > including in the set_config patches, out as soon as I can.
> >
> 
> Let me know if you still want to do it and you'll have patches ready
> before v5.5 is released. Otherwise I can do it myself too if needed.
> 

When does the window close for v5.5?
I've got an updated series ready - other than the doc updates mentioned
above.

Cheers,
Kent.

Re: [PATCH v4 0/5] gpio: expose line bias flags to userspace

[Bartosz Golaszewski]

pon., 4 lis 2019 o 15:22 Kent Gibson <warthog618@gmail.com> napisał(a):
>
> On Mon, Nov 04, 2019 at 12:48:33PM +0100, Bartosz Golaszewski wrote:
> > pon., 4 lis 2019 o 12:12 Kent Gibson <warthog618@gmail.com> napisał(a):
> > >
> > > On Mon, Nov 04, 2019 at 11:14:56AM +0100, Bartosz Golaszewski wrote:
> > > > pon., 4 lis 2019 o 02:07 Kent Gibson <warthog618@gmail.com> napisał(a):
> > > > >
> > > > > On Mon, Nov 04, 2019 at 01:26:54AM +0100, Linus Walleij wrote:
> > > > > > On Thu, Oct 31, 2019 at 8:10 AM Bartosz Golaszewski
> > > > > > <bgolaszewski@baylibre.com> wrote:
> > > > > >
> > > > > > > [Kent]
> > > > > > > > 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.
> > > > > >
> > > > > > Right, excellent and persistent work here, much appreciated!
> > > > > >
> > > > >
> > > > > No problem - hopefully I haven't irritated too many people in the process.
> > > > >
> > > > > > As long as I get Bartosz's blanket ACK on v5 I think it is ready
> > > > > > to merge. His consent is required for this.
> > > > > >
> > > > >
> > > > > I'm still waiting on open questions from v4 before submitting v5:
> > > > >
> > > > > One, handling of errors when setting bias, Bart has referred to Thomas,
> > > > > so waiting for feedback on that.
> > > > >
> > > >
> > > > If we can get it merged for v5.5, then I don't want to block it
> > > > waiting for answers. Looking at the code I think we should only ignore
> > > > the EOPNOTSUPP error and propagate all other codes. Can you add a
> > > > patch changing that and then fix the other nits I pointed out? Also:
> > > > please Cc Thomas Petazzoni so that he gets the chance to yell at us if
> > > > it breaks something.
> > > >
> > >
>
> Can you just confirm if it is EOPNOTSUPP or ENOTSUPP that you want ignored?
>

Oops, it's -ENOTSUPP of course, the error code returned from
gpio_set_config() by this line:

  return gc->set_config ? gc->set_config(gc, offset, config) : -ENOTSUPP;

> > > Will do.
> > >
> > > > > The other, where gpio_set_bias is hooked into gpiod_direction_output,
> > > > > is fine as is for the UAPI - it can always be relocated subsequently if
> > > > > other APIs need to set bias.  On the other hand, if decoupling setting
> > > > > direction and bias is in order then that really should be done now.
> > > > > Can I get an an ACK on that either way?
> > > > >
> > > >
> > > > This is in line with what you did for input. I don't think it should
> > > > be decoupled (any particular reason for that? There is none mentioned
> > > > in the cover letter), so I propose to leave it as it is in patch 5/5.
> > > >
> > >
> > > Wrt decoupling, I was thinking of both input and output, not just output,
> > > though it was the output that got me onto that line of thought as
> > > gpiod_direction_output sets bias, but gpiod_direction_output_raw doesn't.
> > > That seemed totally arbitrary.
> > >
> > > That lead to thinking that the gpiod_direction_output (and _input) is now
> > > doing more than implied by the name, and by the documentation for that
> > > matter.  So perhaps it should be split out and promote gpio_set_bias to
> > > gpiod_set_bias?  Anyway, that was the line of thought.
> > > The problem there being some callers of gpiod_direction_input already
> > > expect it to set bias, at least on a best effort basis, and they would
> > > have to be updated to call gpiod_set_bias.
> > >
> >
> > I see. I think that in this case, the _raw variants should stay as
> > simple as possible (hence the name) while the bias *should* be set in
> > the regular gpiod_direction_intput()/output(). For now I don't think
> > we need an exported gpiod_set_bias(), but if someone should request it
> > in the future it will be straightforward to add.
> >
>
> OK.
>
> > > Maybe just update the documentation for exported functions that do set
> > > bias?
> >
> > Sure, sounds good. You can even extend the doc to include other flags
> > these functions handle.
> >
>
> OK to add this later, or does it need to be in this series?
>

Yeah, we can do it later.

> > >
> > > > One more thing - since we all want this to make it for v5.5 - can you
> > > > make the set config patches part of this series (simply bunch it all
> > > > together)? This will make it easy to review and merge everything.
> > > >
> > >
> > > May as well - I've got it all in the one branch anyway.
> > >
> > > > Thanks in advance and great job!
> > >
> > > I was about to start updating libgpiod to add set_config, after adding
> > > the equivalent to my gpiod library (nearly finished writing the tests
> > > for that), but I'll put all that on the back burner and get v5,
> > > including in the set_config patches, out as soon as I can.
> > >
> >
> > Let me know if you still want to do it and you'll have patches ready
> > before v5.5 is released. Otherwise I can do it myself too if needed.
> >
>
> When does the window close for v5.5?
> I've got an updated series ready - other than the doc updates mentioned
> above.
>

Linus mentioned during the ELCE that this release may take 8 release
candidates, so we should be good, but it would be great to have it in
for-next by the end of this week. But I was actually referring to
related libgpiod changes - in this case I'd like to do a release as
soon as v5.5 is out which will be in around 3 months anyway, so we
have time.

Bart

> Cheers,
> Kent.

Re: [PATCH v4 0/5] gpio: expose line bias flags to userspace

[Kent Gibson]

On Mon, Nov 04, 2019 at 04:20:03PM +0100, Bartosz Golaszewski wrote:
> 
> Linus mentioned during the ELCE that this release may take 8 release
> candidates, so we should be good, but it would be great to have it in
> for-next by the end of this week. But I was actually referring to
> related libgpiod changes - in this case I'd like to do a release as
> soon as v5.5 is out which will be in around 3 months anyway, so we
> have time.
> 

OK, I'll send out v5 shortly.

I have the bias changes for libgpiod completed - just need to tidy them 
up into a patch.  Still have to do the SET_CONFIG though.  Should have
a first pass of that in the next day or two.

Cheers,
Kent.