[PATCH 0/4] add support for bias pull-disable

[PATCH 0/4] add support for bias pull-disable

[Nuno Sá]

The gpio core looks at 'FLAG_BIAS_DISABLE' in preparation of calling the
gpiochip 'set_config()' hook. However, AFAICT, there's no way that this
flag is set because there's no support for it in firwmare code. Moreover,
in 'gpiod_configure_flags()', only pull-ups and pull-downs are being
handled.

On top of this, there are some users that are looking at
'PIN_CONFIG_BIAS_DISABLE' in the 'set_config()' hook. So, unless I'm
missing something, it looks like this was never working for these chips.

Note that the ACPI case is only compiled tested. At first glance, it seems
the current patch is enough but i'm not really sure...

As a side note, this came to my attention during this patchset [1]
(and, ofr OF,  was tested with it).

[1]: https://lore.kernel.org/linux-input/20220708093448.42617-5-nuno.sa@analog.com/

Nuno Sá (4):
  gpiolib: add support for bias pull disable
  gpiolib: of: support bias pull disable
  gpiolib: acpi: support bias pull disable
  dt-bindings: gpio: add pull-disable flag

 drivers/gpio/gpiolib-acpi.c     | 3 +++
 drivers/gpio/gpiolib-of.c       | 7 +++++++
 drivers/gpio/gpiolib.c          | 8 ++++++--
 include/dt-bindings/gpio/gpio.h | 3 +++
 include/linux/gpio/machine.h    | 1 +
 include/linux/of_gpio.h         | 1 +
 6 files changed, 21 insertions(+), 2 deletions(-)

-- 
2.37.0

[PATCH 1/4] gpiolib: add support for bias pull disable

[Nuno Sá]

This change prepares the gpio core to look at firmware flags and set
'FLAG_BIAS_DISABLE' if necessary. It works in similar way to
'GPIO_PULL_DOWN' and 'GPIO_PULL_UP'.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/gpio/gpiolib.c       | 8 ++++++--
 include/linux/gpio/machine.h | 1 +
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 9535f48e18d1..0692ec84d3b0 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -3945,9 +3945,11 @@ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
 	if (lflags & GPIO_OPEN_SOURCE)
 		set_bit(FLAG_OPEN_SOURCE, &desc->flags);
 
-	if ((lflags & GPIO_PULL_UP) && (lflags & GPIO_PULL_DOWN)) {
+	if (((lflags & GPIO_PULL_UP) && (lflags & GPIO_PULL_DOWN)) ||
+	    ((lflags & GPIO_PULL_UP) && (lflags & GPIO_PULL_DISABLE)) ||
+	    ((lflags & GPIO_PULL_DOWN) && (lflags & GPIO_PULL_DISABLE))) {
 		gpiod_err(desc,
-			  "both pull-up and pull-down enabled, invalid configuration\n");
+			  "multiple pull-up, pull-down or pull-disable enabled, invalid configuration\n");
 		return -EINVAL;
 	}
 
@@ -3955,6 +3957,8 @@ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
 		set_bit(FLAG_PULL_UP, &desc->flags);
 	else if (lflags & GPIO_PULL_DOWN)
 		set_bit(FLAG_PULL_DOWN, &desc->flags);
+	else if (lflags & GPIO_PULL_DISABLE)
+		set_bit(FLAG_BIAS_DISABLE, &desc->flags);
 
 	ret = gpiod_set_transitory(desc, (lflags & GPIO_TRANSITORY));
 	if (ret < 0)
diff --git a/include/linux/gpio/machine.h b/include/linux/gpio/machine.h
index 4d55da28e664..0b619eb7ae83 100644
--- a/include/linux/gpio/machine.h
+++ b/include/linux/gpio/machine.h
@@ -14,6 +14,7 @@ enum gpio_lookup_flags {
 	GPIO_TRANSITORY			= (1 << 3),
 	GPIO_PULL_UP			= (1 << 4),
 	GPIO_PULL_DOWN			= (1 << 5),
+	GPIO_PULL_DISABLE		= (1 << 6),
 
 	GPIO_LOOKUP_FLAGS_DEFAULT	= GPIO_ACTIVE_HIGH | GPIO_PERSISTENT,
 };
-- 
2.37.0

[PATCH 2/4] gpiolib: of: support bias pull disable

[Nuno Sá]

On top of looking at PULL_UP and PULL_DOWN flags, also look at
PULL_DISABLE and set the appropriate GPIO flag. The GPIO core will then
pass down this to controllers that support it.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/gpio/gpiolib-of.c | 7 +++++++
 include/linux/of_gpio.h   | 1 +
 2 files changed, 8 insertions(+)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index f80307be37d5..a037b50bef33 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -354,6 +354,9 @@ struct gpio_desc *gpiod_get_from_of_node(const struct device_node *node,
 	if (flags & OF_GPIO_PULL_DOWN)
 		lflags |= GPIO_PULL_DOWN;
 
+	if (flags & OF_GPIO_PULL_DISABLE)
+		lflags |= GPIO_PULL_DISABLE;
+
 	ret = gpiod_configure_flags(desc, propname, lflags, dflags);
 	if (ret < 0) {
 		gpiod_put(desc);
@@ -556,6 +559,8 @@ struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
 		*flags |= GPIO_PULL_UP;
 	if (of_flags & OF_GPIO_PULL_DOWN)
 		*flags |= GPIO_PULL_DOWN;
+	if (of_flags & OF_GPIO_PULL_DISABLE)
+		*flags |= GPIO_PULL_DISABLE;
 
 	return desc;
 }
@@ -621,6 +626,8 @@ static struct gpio_desc *of_parse_own_gpio(struct device_node *np,
 		*lflags |= GPIO_PULL_UP;
 	if (xlate_flags & OF_GPIO_PULL_DOWN)
 		*lflags |= GPIO_PULL_DOWN;
+	if (xlate_flags & OF_GPIO_PULL_DISABLE)
+		*lflags |= GPIO_PULL_DISABLE;
 
 	if (of_property_read_bool(np, "input"))
 		*dflags |= GPIOD_IN;
diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
index 8bf2ea859653..a5166eb93437 100644
--- a/include/linux/of_gpio.h
+++ b/include/linux/of_gpio.h
@@ -29,6 +29,7 @@ enum of_gpio_flags {
 	OF_GPIO_TRANSITORY = 0x8,
 	OF_GPIO_PULL_UP = 0x10,
 	OF_GPIO_PULL_DOWN = 0x20,
+	OF_GPIO_PULL_DISABLE = 0x40,
 };
 
 #ifdef CONFIG_OF_GPIO
-- 
2.37.0

[PATCH 3/4] gpiolib: acpi: support bias pull disable

[Nuno Sá]

On top of looking at PULL_UP and PULL_DOWN flags, also look at
PULL_DISABLE and set the appropriate GPIO flag. The GPIO core will then
pass down this to controllers that support it.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/gpio/gpiolib-acpi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index c2523ac26fac..9be1376f9a62 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -687,6 +687,9 @@ int acpi_gpio_update_gpiod_lookup_flags(unsigned long *lookupflags,
 	case ACPI_PIN_CONFIG_PULLDOWN:
 		*lookupflags |= GPIO_PULL_DOWN;
 		break;
+	case ACPI_PIN_CONFIG_NOPULL:
+		*lookupflags |= GPIO_PULL_DISABLE;
+		break;
 	default:
 		break;
 	}
-- 
2.37.0

[PATCH 4/4] dt-bindings: gpio: add pull-disable flag

[Nuno Sá]

This extends the flags that can be used in GPIO specifiers to indicate
that no bias is intended in the pin.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 include/dt-bindings/gpio/gpio.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/dt-bindings/gpio/gpio.h b/include/dt-bindings/gpio/gpio.h
index c029467e828b..5566e58196a2 100644
--- a/include/dt-bindings/gpio/gpio.h
+++ b/include/dt-bindings/gpio/gpio.h
@@ -39,4 +39,7 @@
 /* Bit 5 express pull down */
 #define GPIO_PULL_DOWN 32
 
+/* Bit 6 express pull disable */
+#define GPIO_PULL_DISABLE 64
+
 #endif
-- 
2.37.0

Re: [PATCH 1/4] gpiolib: add support for bias pull disable

[Andy Shevchenko]

On Wed, Jul 13, 2022 at 03:14:18PM +0200, Nuno Sá wrote:
> This change prepares the gpio core to look at firmware flags and set
> 'FLAG_BIAS_DISABLE' if necessary. It works in similar way to
> 'GPIO_PULL_DOWN' and 'GPIO_PULL_UP'.

...

>  	GPIO_PULL_UP			= (1 << 4),
>  	GPIO_PULL_DOWN			= (1 << 5),
> +	GPIO_PULL_DISABLE		= (1 << 6),

To me it seems superfluous. You have already two flags:
PUp
PDown
When none is set --> Pdisable

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/4] add support for bias pull-disable

[Andy Shevchenko]

On Wed, Jul 13, 2022 at 03:14:17PM +0200, Nuno Sá wrote:
> The gpio core looks at 'FLAG_BIAS_DISABLE' in preparation of calling the
> gpiochip 'set_config()' hook. However, AFAICT, there's no way that this
> flag is set because there's no support for it in firwmare code. Moreover,
> in 'gpiod_configure_flags()', only pull-ups and pull-downs are being
> handled.

Isn't it enough?

> On top of this, there are some users that are looking at
> 'PIN_CONFIG_BIAS_DISABLE' in the 'set_config()' hook. So, unless I'm
> missing something, it looks like this was never working for these chips.

It seems you are looking into wrong source of issues. Isn't it a issue of
particular pin control driver?

> Note that the ACPI case is only compiled tested. At first glance, it seems
> the current patch is enough but i'm not really sure...
> 
> As a side note, this came to my attention during this patchset [1]
> (and, ofr OF,  was tested with it).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/4] gpiolib: add support for bias pull disable

[Kent Gibson]

On Wed, Jul 13, 2022 at 08:36:38PM +0300, Andy Shevchenko wrote:
> On Wed, Jul 13, 2022 at 03:14:18PM +0200, Nuno Sá wrote:
> > This change prepares the gpio core to look at firmware flags and set
> > 'FLAG_BIAS_DISABLE' if necessary. It works in similar way to
> > 'GPIO_PULL_DOWN' and 'GPIO_PULL_UP'.
> 
> ...
> 
> >  	GPIO_PULL_UP			= (1 << 4),
> >  	GPIO_PULL_DOWN			= (1 << 5),
> > +	GPIO_PULL_DISABLE		= (1 << 6),
> 
> To me it seems superfluous. You have already two flags:
> PUp
> PDown
> When none is set --> Pdisable
> 

Agree with Andy on this.  The FLAG_BIAS_DISABLE was added, by me, to
allow the cdev interface to support bias.  cdev requires a "don't care"
state, distinct from an explicit BIAS_DISABLE.
The FLAG_BIAS_DISABLE allows gpiolib-cdev to communicate that to
gpiolib, without altering the interpretation of the existing PULL_UP and
PULL_DOWN flags.
That is not an issue on the machine interface, where the two GPIO_PULL
flags suffice.

If you are looking for the place where FLAG_BIAS_DISABLE is set it is in
gpio_v2_line_config_flags_to_desc_flags() in gpiolib-cdev.c.

Referring to gpio_set_bias(), the only place in gpiolib the
FLAG_BIAS_DISABLE is used, if neither FLAG_PULL_UP, FLAG_PULL_DOWN,
nor FLAG_BIAS_DISABLE are set then the bias configuration remains
unchanged (the don't care case) - no change is passed to the driver.
Otherwise the corresponding PIN_CONFIG_BIAS flag is passed to the
driver.

If there are cases of drivers not fully or correctly supporting those
PIN_CONFIG_BIAS flags, then that is an issue with those drivers.

Cheers,
Kent.

Re: [PATCH 0/4] add support for bias pull-disable

[Nuno Sá]

On Wed, 2022-07-13 at 20:39 +0300, Andy Shevchenko wrote:
> On Wed, Jul 13, 2022 at 03:14:17PM +0200, Nuno Sá wrote:
> > The gpio core looks at 'FLAG_BIAS_DISABLE' in preparation of
> > calling the
> > gpiochip 'set_config()' hook. However, AFAICT, there's no way that
> > this
> > flag is set because there's no support for it in firwmare code.
> > Moreover,
> > in 'gpiod_configure_flags()', only pull-ups and pull-downs are
> > being
> > handled.
> 
> Isn't it enough?
> 

I might be missing something but don't think so. Look at this driver
which seems a lot like the reference i put in the cover:

https://elixir.bootlin.com/linux/v5.19-rc6/source/drivers/gpio/gpio-pca953x.c#L573

I just don't see an in-kernel path (I'm aware now that we can get here
through gpio cdev) to get to the point where we want to disable the pin
BIAS.

> > On top of this, there are some users that are looking at
> > 'PIN_CONFIG_BIAS_DISABLE' in the 'set_config()' hook. So, unless
> > I'm
> > missing something, it looks like this was never working for these
> > chips.
> 
> It seems you are looking into wrong source of issues. Isn't it a
> issue of
> particular pin control driver?
> 
> 
> 

Think about gpio expanders on, eg, an i2c bus which don't really have
any pinmuxing capability [1]. For example, my device is an i2c keyboard
which has the capability of exposing pins as gpios (to be consumed by
gpio_keys). The pins, by default are PULL-UPs but we can disable them
doing an i2c write on the device. So to me, the way to do it is via the
gpiochip 'set_config()' hook but as things are, there's no way to get
into the callback with 'PIN_CONFIG_BIAS_DISABLE'. And the driver cannot
just assume that the default case is to disable bias...

Now taking your words (on patch 1 comments)

"
To me it seems superfluous. You have already two flags:
PUp
PDown
When none is set --> Pdisable
"

I guess we could do that assumption in 'gpiod_configure_flags()' and
extend the following code:


if (lflags & GPIO_PULL_UP)
	set_bit(FLAG_PULL_UP, &desc->flags);
else if (lflags & GPIO_PULL_DOWN)
	set_bit(FLAG_PULL_DOWN, &desc->flags);

with an else clause where we do 'set_bit(FLAG_BIAS_DISABLE, &desc-
>flags)' by default. As gpiolib does not consider '-ENOTSUPP' as an
error, this would not "explicitly" break existing drivers.

But I do have some concerns with making such an assumption. This
*might* change behavior on existing systems. Think on a system using
for example gpio-pca953x I linked before. If the default state of the
pins is PULL-UP (or down), it's legit to think that, for example,
devicetrees of such a system are not explicitly setting 'GPIO_PULL_UP'.
That's it, this change would break it because now the pins will have
BIAS disabled by default...

Note the above is just me speculating but might be a valid concern.
 
1: https://github.com/torvalds/linux/commit/ede033e1e863c


- Nuno Sá

Re: [PATCH 1/4] gpiolib: add support for bias pull disable

[Nuno Sá]

On Thu, 2022-07-14 at 12:20 +0800, Kent Gibson wrote:
> On Wed, Jul 13, 2022 at 08:36:38PM +0300, Andy Shevchenko wrote:
> > On Wed, Jul 13, 2022 at 03:14:18PM +0200, Nuno Sá wrote:
> > > This change prepares the gpio core to look at firmware flags and
> > > set
> > > 'FLAG_BIAS_DISABLE' if necessary. It works in similar way to
> > > 'GPIO_PULL_DOWN' and 'GPIO_PULL_UP'.
> > 
> > ...
> > 
> > >         GPIO_PULL_UP                    = (1 << 4),
> > >         GPIO_PULL_DOWN                  = (1 << 5),
> > > +       GPIO_PULL_DISABLE               = (1 << 6),
> > 
> > To me it seems superfluous. You have already two flags:
> > PUp
> > PDown
> > When none is set --> Pdisable
> > 
> 
> Agree with Andy on this.  The FLAG_BIAS_DISABLE was added, by me, to
> allow the cdev interface to support bias.  cdev requires a "don't
> care"
> state, distinct from an explicit BIAS_DISABLE.
> The FLAG_BIAS_DISABLE allows gpiolib-cdev to communicate that to
> gpiolib, without altering the interpretation of the existing PULL_UP
> and
> PULL_DOWN flags.
> That is not an issue on the machine interface, where the two
> GPIO_PULL
> flags suffice.
> 

I see, but this means we can only disable the pin BIAS through
userspace. I might be wrong but I don't see a reason why it wouldn't be
valid to do it from an in kernel path as we do for PULL-UPS and PULL-
DOWNS 

> If you are looking for the place where FLAG_BIAS_DISABLE is set it is
> in
> gpio_v2_line_config_flags_to_desc_flags() in gpiolib-cdev.c.
> 
> Referring to gpio_set_bias(), the only place in gpiolib the
> FLAG_BIAS_DISABLE is used, if neither FLAG_PULL_UP, FLAG_PULL_DOWN,
> nor FLAG_BIAS_DISABLE are set then the bias configuration remains
> unchanged (the don't care case) - no change is passed to the driver.
> Otherwise the corresponding PIN_CONFIG_BIAS flag is passed to the
> driver.
> 

Exactly, but note FLAG_BIAS_DISABLE can only be set from userspace at
this point (IIUTC). If everyone agrees that should be case, so be it.
But as I said, I just don't see why it's wrong to do it within the
kernel.

> If there are cases of drivers not fully or correctly supporting those
> PIN_CONFIG_BIAS flags, then that is an issue with those drivers.
> 

Look at my reply to Andy in the cover for more details

- Nuno Sá

Re: [PATCH 1/4] gpiolib: add support for bias pull disable

[Kent Gibson]

On Thu, Jul 14, 2022 at 09:14:21AM +0200, Nuno Sá wrote:
> On Thu, 2022-07-14 at 12:20 +0800, Kent Gibson wrote:
> > On Wed, Jul 13, 2022 at 08:36:38PM +0300, Andy Shevchenko wrote:
> > > On Wed, Jul 13, 2022 at 03:14:18PM +0200, Nuno Sá wrote:
> > > > This change prepares the gpio core to look at firmware flags and
> > > > set
> > > > 'FLAG_BIAS_DISABLE' if necessary. It works in similar way to
> > > > 'GPIO_PULL_DOWN' and 'GPIO_PULL_UP'.
> > > 
> > > ...
> > > 
> > > >         GPIO_PULL_UP                    = (1 << 4),
> > > >         GPIO_PULL_DOWN                  = (1 << 5),
> > > > +       GPIO_PULL_DISABLE               = (1 << 6),
> > > 
> > > To me it seems superfluous. You have already two flags:
> > > PUp
> > > PDown
> > > When none is set --> Pdisable
> > > 
> > 
> > Agree with Andy on this.  The FLAG_BIAS_DISABLE was added, by me, to
> > allow the cdev interface to support bias.  cdev requires a "don't
> > care"
> > state, distinct from an explicit BIAS_DISABLE.
> > The FLAG_BIAS_DISABLE allows gpiolib-cdev to communicate that to
> > gpiolib, without altering the interpretation of the existing PULL_UP
> > and
> > PULL_DOWN flags.
> > That is not an issue on the machine interface, where the two
> > GPIO_PULL
> > flags suffice.
> > 
> 
> I see, but this means we can only disable the pin BIAS through
> userspace. I might be wrong but I don't see a reason why it wouldn't be
> valid to do it from an in kernel path as we do for PULL-UPS and PULL-
> DOWNS 
> 

> > If you are looking for the place where FLAG_BIAS_DISABLE is set it is
> > in
> > gpio_v2_line_config_flags_to_desc_flags() in gpiolib-cdev.c.
> > 
> > Referring to gpio_set_bias(), the only place in gpiolib the
> > FLAG_BIAS_DISABLE is used, if neither FLAG_PULL_UP, FLAG_PULL_DOWN,
> > nor FLAG_BIAS_DISABLE are set then the bias configuration remains
> > unchanged (the don't care case) - no change is passed to the driver.
> > Otherwise the corresponding PIN_CONFIG_BIAS flag is passed to the
> > driver.
> > 
> 
> Exactly, but note FLAG_BIAS_DISABLE can only be set from userspace at
> this point (IIUTC). If everyone agrees that should be case, so be it.
> But as I said, I just don't see why it's wrong to do it within the
> kernel.
> 

Believe it or not gpiolib-cdev is part of the kernel, not userspace - it
just provides an interface to userspace.

Bias can be disabled by calling gpiod_direction_input() or
gpiod_direction_output() after setting the FLAG_BIAS_DISABLE, as
gpiolib-cdev does.

Does that work for you?

Cheers,
Kent.


> > If there are cases of drivers not fully or correctly supporting those
> > PIN_CONFIG_BIAS flags, then that is an issue with those drivers.
> > 
> 
> Look at my reply to Andy in the cover for more details
> 
> - Nuno Sá

Re: [PATCH 1/4] gpiolib: add support for bias pull disable

[Nuno Sá]

On Thu, 2022-07-14 at 16:27 +0800, Kent Gibson wrote:
> On Thu, Jul 14, 2022 at 09:14:21AM +0200, Nuno Sá wrote:
> > On Thu, 2022-07-14 at 12:20 +0800, Kent Gibson wrote:
> > > On Wed, Jul 13, 2022 at 08:36:38PM +0300, Andy Shevchenko wrote:
> > > > On Wed, Jul 13, 2022 at 03:14:18PM +0200, Nuno Sá wrote:
> > > > > This change prepares the gpio core to look at firmware flags
> > > > > and
> > > > > set
> > > > > 'FLAG_BIAS_DISABLE' if necessary. It works in similar way to
> > > > > 'GPIO_PULL_DOWN' and 'GPIO_PULL_UP'.
> > > > 
> > > > ...
> > > > 
> > > > >         GPIO_PULL_UP                    = (1 << 4),
> > > > >         GPIO_PULL_DOWN                  = (1 << 5),
> > > > > +       GPIO_PULL_DISABLE               = (1 << 6),
> > > > 
> > > > To me it seems superfluous. You have already two flags:
> > > > PUp
> > > > PDown
> > > > When none is set --> Pdisable
> > > > 
> > > 
> > > Agree with Andy on this.  The FLAG_BIAS_DISABLE was added, by me,
> > > to
> > > allow the cdev interface to support bias.  cdev requires a "don't
> > > care"
> > > state, distinct from an explicit BIAS_DISABLE.
> > > The FLAG_BIAS_DISABLE allows gpiolib-cdev to communicate that to
> > > gpiolib, without altering the interpretation of the existing
> > > PULL_UP
> > > and
> > > PULL_DOWN flags.
> > > That is not an issue on the machine interface, where the two
> > > GPIO_PULL
> > > flags suffice.
> > > 
> > 
> > I see, but this means we can only disable the pin BIAS through
> > userspace. I might be wrong but I don't see a reason why it
> > wouldn't be
> > valid to do it from an in kernel path as we do for PULL-UPS and
> > PULL-
> > DOWNS 
> > 
> 
> > > If you are looking for the place where FLAG_BIAS_DISABLE is set
> > > it is
> > > in
> > > gpio_v2_line_config_flags_to_desc_flags() in gpiolib-cdev.c.
> > > 
> > > Referring to gpio_set_bias(), the only place in gpiolib the
> > > FLAG_BIAS_DISABLE is used, if neither FLAG_PULL_UP,
> > > FLAG_PULL_DOWN,
> > > nor FLAG_BIAS_DISABLE are set then the bias configuration remains
> > > unchanged (the don't care case) - no change is passed to the
> > > driver.
> > > Otherwise the corresponding PIN_CONFIG_BIAS flag is passed to the
> > > driver.
> > > 
> > 
> > Exactly, but note FLAG_BIAS_DISABLE can only be set from userspace
> > at
> > this point (IIUTC). If everyone agrees that should be case, so be
> > it.
> > But as I said, I just don't see why it's wrong to do it within the
> > kernel.
> > 
> 
> Believe it or not gpiolib-cdev is part of the kernel, not userspace -
> it
> just provides an interface to userspace.
> 

Yes, I do know that. But don't you still need a userspace process to
open the cdev and do the ioctl()?

> Bias can be disabled by calling gpiod_direction_input() or
> gpiod_direction_output() after setting the FLAG_BIAS_DISABLE, as
> gpiolib-cdev does.
> 
> Does that work for you?
> 

I'm not seeing how would this work... We would need to make gpiod
consumers having to do this. Something like:


desc = giod_get();
set_bit(FLAG_BIAS_DISABLE, &desc->flags);
set_direction...


Having in mind that we can already specify the direction in gpiod_get,
I don't really think this is something that consumers should have to
worry. Moreover, I would say this means special devicetree properties
for all the consumers of such a gpiochip which want to disable bias...

...

Or do you have something else in mind?

- Nuno Sá

Re: [PATCH 0/4] add support for bias pull-disable

[Andy Shevchenko]

On Thu, Jul 14, 2022 at 9:10 AM Nuno Sá <noname.nuno@gmail.com> wrote:
> On Wed, 2022-07-13 at 20:39 +0300, Andy Shevchenko wrote:
> > On Wed, Jul 13, 2022 at 03:14:17PM +0200, Nuno Sá wrote:
> > > The gpio core looks at 'FLAG_BIAS_DISABLE' in preparation of
> > > calling the
> > > gpiochip 'set_config()' hook. However, AFAICT, there's no way that
> > > this
> > > flag is set because there's no support for it in firwmare code.
> > > Moreover,
> > > in 'gpiod_configure_flags()', only pull-ups and pull-downs are
> > > being
> > > handled.
> >
> > Isn't it enough?
>
> I might be missing something but don't think so. Look at this driver
> which seems a lot like the reference i put in the cover:
>
> https://elixir.bootlin.com/linux/v5.19-rc6/source/drivers/gpio/gpio-pca953x.c#L573
>
> I just don't see an in-kernel path (I'm aware now that we can get here
> through gpio cdev) to get to the point where we want to disable the pin
> BIAS.

Ah, that driver should be converted to pin control. It's definitely a
problem with the driver.

But let me look into the library code to understand better what your
point is in general.

P.S. Pin muxing has nothing to do with the pin control, many (I guess
more than 90%) of GPIO controllers do have pin control features.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/4] add support for bias pull-disable

[Nuno Sá]

On Thu, 2022-07-14 at 11:12 +0200, Andy Shevchenko wrote:
> On Thu, Jul 14, 2022 at 9:10 AM Nuno Sá <noname.nuno@gmail.com>
> wrote:
> > On Wed, 2022-07-13 at 20:39 +0300, Andy Shevchenko wrote:
> > > On Wed, Jul 13, 2022 at 03:14:17PM +0200, Nuno Sá wrote:
> > > > The gpio core looks at 'FLAG_BIAS_DISABLE' in preparation of
> > > > calling the
> > > > gpiochip 'set_config()' hook. However, AFAICT, there's no way
> > > > that
> > > > this
> > > > flag is set because there's no support for it in firwmare code.
> > > > Moreover,
> > > > in 'gpiod_configure_flags()', only pull-ups and pull-downs are
> > > > being
> > > > handled.
> > > 
> > > Isn't it enough?
> > 
> > I might be missing something but don't think so. Look at this
> > driver
> > which seems a lot like the reference i put in the cover:
> > 
> > https://elixir.bootlin.com/linux/v5.19-rc6/source/drivers/gpio/gpio-pca953x.c#L573
> > 
> > I just don't see an in-kernel path (I'm aware now that we can get
> > here
> > through gpio cdev) to get to the point where we want to disable the
> > pin
> > BIAS.
> 
> Ah, that driver should be converted to pin control. It's definitely a
> problem with the driver.
> 

I'm not too familiar with pinctrl or even gpiochips to argue much in
here so just looking to better understand it...

The driver has it's own way to control the pin BIAS and does not rely
on any other pinctrl chip on the system. Are you pointing that this
driver should be converted in a pinctrl one which registers the
gpiochip and drops the 'set_config()' callback so pin consumers could
use pinctrl to let's say, disable BIAS? If so, then why do we have
PIN_CONFIG_BIAS_PULL_UP|DOWN in 'set_config()'? Legacy? 

And for my test device which is an input keymap, having it converted
into a pinctrl driver does not make much sense  (and it also supports
pull-up pull-disable) and I guess that might make sense for other
devices that have some optional gpiochip support...

- Nuno Sá

Re: [PATCH 1/4] gpiolib: add support for bias pull disable

[Kent Gibson]

On Thu, Jul 14, 2022 at 10:47:27AM +0200, Nuno Sá wrote:
> On Thu, 2022-07-14 at 16:27 +0800, Kent Gibson wrote:
> > On Thu, Jul 14, 2022 at 09:14:21AM +0200, Nuno Sá wrote:
> > > On Thu, 2022-07-14 at 12:20 +0800, Kent Gibson wrote:
> > > > On Wed, Jul 13, 2022 at 08:36:38PM +0300, Andy Shevchenko wrote:
> > > > > On Wed, Jul 13, 2022 at 03:14:18PM +0200, Nuno Sá wrote:
> > > > > > This change prepares the gpio core to look at firmware flags
> > > > > > and
> > > > > > set
> > > > > > 'FLAG_BIAS_DISABLE' if necessary. It works in similar way to
> > > > > > 'GPIO_PULL_DOWN' and 'GPIO_PULL_UP'.
> > > > > 
> > > > > ...
> > > > > 
> > > > > >         GPIO_PULL_UP                    = (1 << 4),
> > > > > >         GPIO_PULL_DOWN                  = (1 << 5),
> > > > > > +       GPIO_PULL_DISABLE               = (1 << 6),
> > > > > 
> > > > > To me it seems superfluous. You have already two flags:
> > > > > PUp
> > > > > PDown
> > > > > When none is set --> Pdisable
> > > > > 
> > > > 
> > > > Agree with Andy on this.  The FLAG_BIAS_DISABLE was added, by me,
> > > > to
> > > > allow the cdev interface to support bias.  cdev requires a "don't
> > > > care"
> > > > state, distinct from an explicit BIAS_DISABLE.
> > > > The FLAG_BIAS_DISABLE allows gpiolib-cdev to communicate that to
> > > > gpiolib, without altering the interpretation of the existing
> > > > PULL_UP
> > > > and
> > > > PULL_DOWN flags.
> > > > That is not an issue on the machine interface, where the two
> > > > GPIO_PULL
> > > > flags suffice.
> > > > 
> > > 
> > > I see, but this means we can only disable the pin BIAS through
> > > userspace. I might be wrong but I don't see a reason why it
> > > wouldn't be
> > > valid to do it from an in kernel path as we do for PULL-UPS and
> > > PULL-
> > > DOWNS 
> > > 
> > 
> > > > If you are looking for the place where FLAG_BIAS_DISABLE is set
> > > > it is
> > > > in
> > > > gpio_v2_line_config_flags_to_desc_flags() in gpiolib-cdev.c.
> > > > 
> > > > Referring to gpio_set_bias(), the only place in gpiolib the
> > > > FLAG_BIAS_DISABLE is used, if neither FLAG_PULL_UP,
> > > > FLAG_PULL_DOWN,
> > > > nor FLAG_BIAS_DISABLE are set then the bias configuration remains
> > > > unchanged (the don't care case) - no change is passed to the
> > > > driver.
> > > > Otherwise the corresponding PIN_CONFIG_BIAS flag is passed to the
> > > > driver.
> > > > 
> > > 
> > > Exactly, but note FLAG_BIAS_DISABLE can only be set from userspace
> > > at
> > > this point (IIUTC). If everyone agrees that should be case, so be
> > > it.
> > > But as I said, I just don't see why it's wrong to do it within the
> > > kernel.
> > > 
> > 
> > Believe it or not gpiolib-cdev is part of the kernel, not userspace -
> > it
> > just provides an interface to userspace.
> > 
> 
> Yes, I do know that. But don't you still need a userspace process to
> open the cdev and do the ioctl()?
> 

Only if you want to drive the cdev interface, so not in this case.
You would not use cdev, you would use gpiolib directly.

> > Bias can be disabled by calling gpiod_direction_input() or
> > gpiod_direction_output() after setting the FLAG_BIAS_DISABLE, as
> > gpiolib-cdev does.
> > 
> > Does that work for you?
> > 
> 
> I'm not seeing how would this work... We would need to make gpiod
> consumers having to do this. Something like:
> 
> 
> desc = giod_get();
> set_bit(FLAG_BIAS_DISABLE, &desc->flags);
> set_direction...
> 
> 

In a nutshell.

If that solves your immediate problem then you need to decide what
problem your patch is trying to address.

Cheers,
Kent.

Re: [PATCH 1/4] gpiolib: add support for bias pull disable

[Nuno Sá]

On Thu, 2022-07-14 at 20:00 +0800, Kent Gibson wrote:
> On Thu, Jul 14, 2022 at 10:47:27AM +0200, Nuno Sá wrote:
> > On Thu, 2022-07-14 at 16:27 +0800, Kent Gibson wrote:
> > > On Thu, Jul 14, 2022 at 09:14:21AM +0200, Nuno Sá wrote:
> > > > On Thu, 2022-07-14 at 12:20 +0800, Kent Gibson wrote:
> > > > > On Wed, Jul 13, 2022 at 08:36:38PM +0300, Andy Shevchenko
> > > > > wrote:
> > > > > > On Wed, Jul 13, 2022 at 03:14:18PM +0200, Nuno Sá wrote:
> > > > > > > This change prepares the gpio core to look at firmware
> > > > > > > flags
> > > > > > > and
> > > > > > > set
> > > > > > > 'FLAG_BIAS_DISABLE' if necessary. It works in similar way
> > > > > > > to
> > > > > > > 'GPIO_PULL_DOWN' and 'GPIO_PULL_UP'.
> > > > > > 
> > > > > > ...
> > > > > > 
> > > > > > >         GPIO_PULL_UP                    = (1 << 4),
> > > > > > >         GPIO_PULL_DOWN                  = (1 << 5),
> > > > > > > +       GPIO_PULL_DISABLE               = (1 << 6),
> > > > > > 
> > > > > > To me it seems superfluous. You have already two flags:
> > > > > > PUp
> > > > > > PDown
> > > > > > When none is set --> Pdisable
> > > > > > 
> > > > > 
> > > > > Agree with Andy on this.  The FLAG_BIAS_DISABLE was added, by
> > > > > me,
> > > > > to
> > > > > allow the cdev interface to support bias.  cdev requires a
> > > > > "don't
> > > > > care"
> > > > > state, distinct from an explicit BIAS_DISABLE.
> > > > > The FLAG_BIAS_DISABLE allows gpiolib-cdev to communicate that
> > > > > to
> > > > > gpiolib, without altering the interpretation of the existing
> > > > > PULL_UP
> > > > > and
> > > > > PULL_DOWN flags.
> > > > > That is not an issue on the machine interface, where the two
> > > > > GPIO_PULL
> > > > > flags suffice.
> > > > > 
> > > > 
> > > > I see, but this means we can only disable the pin BIAS through
> > > > userspace. I might be wrong but I don't see a reason why it
> > > > wouldn't be
> > > > valid to do it from an in kernel path as we do for PULL-UPS and
> > > > PULL-
> > > > DOWNS 
> > > > 
> > > 
> > > > > If you are looking for the place where FLAG_BIAS_DISABLE is
> > > > > set
> > > > > it is
> > > > > in
> > > > > gpio_v2_line_config_flags_to_desc_flags() in gpiolib-cdev.c.
> > > > > 
> > > > > Referring to gpio_set_bias(), the only place in gpiolib the
> > > > > FLAG_BIAS_DISABLE is used, if neither FLAG_PULL_UP,
> > > > > FLAG_PULL_DOWN,
> > > > > nor FLAG_BIAS_DISABLE are set then the bias configuration
> > > > > remains
> > > > > unchanged (the don't care case) - no change is passed to the
> > > > > driver.
> > > > > Otherwise the corresponding PIN_CONFIG_BIAS flag is passed to
> > > > > the
> > > > > driver.
> > > > > 
> > > > 
> > > > Exactly, but note FLAG_BIAS_DISABLE can only be set from
> > > > userspace
> > > > at
> > > > this point (IIUTC). If everyone agrees that should be case, so
> > > > be
> > > > it.
> > > > But as I said, I just don't see why it's wrong to do it within
> > > > the
> > > > kernel.
> > > > 
> > > 
> > > Believe it or not gpiolib-cdev is part of the kernel, not
> > > userspace -
> > > it
> > > just provides an interface to userspace.
> > > 
> > 
> > Yes, I do know that. But don't you still need a userspace process
> > to
> > open the cdev and do the ioctl()?
> > 
> 
> Only if you want to drive the cdev interface, so not in this case.
> You would not use cdev, you would use gpiolib directly.
> 

That's what I'm trying to do :). Without having to change gpiod
consumers to have to explicitly set this flag.

> > > Bias can be disabled by calling gpiod_direction_input() or
> > > gpiod_direction_output() after setting the FLAG_BIAS_DISABLE, as
> > > gpiolib-cdev does.
> > > 
> > > Does that work for you?
> > > 
> > 
> > I'm not seeing how would this work... We would need to make gpiod
> > consumers having to do this. Something like:
> > 
> > 
> > desc = giod_get();
> > set_bit(FLAG_BIAS_DISABLE, &desc->flags);
> > set_direction...
> > 
> > 
> 
> In a nutshell.
> 
> If that solves your immediate problem then you need to decide what
> problem your patch is trying to address.
> 
> 

So my patch is trying to solve exactly this case because (IMO), it does
not make sense for consumers drivers to have to do the above code.
Moreover, they would need some custom firmware property (eg:
devicetree) for the cases where we want to disable BIAS since we cannot
just assume we want to do it.

Well, maybe we can just assume FLAG_BIAS_DISABLE in gpiolib (when
trying to get the gpiod) if no PULL is specified. However, I do have
some concerns with it (see my conversation with Andy in the cover).

- Nuno Sá

Re: [PATCH 0/4] add support for bias pull-disable

[Andy Shevchenko]

On Wed, Jul 13, 2022 at 03:14:17PM +0200, Nuno Sá wrote:
> The gpio core looks at 'FLAG_BIAS_DISABLE' in preparation of calling the
> gpiochip 'set_config()' hook. However, AFAICT, there's no way that this
> flag is set because there's no support for it in firwmare code. Moreover,
> in 'gpiod_configure_flags()', only pull-ups and pull-downs are being
> handled.
> 
> On top of this, there are some users that are looking at
> 'PIN_CONFIG_BIAS_DISABLE' in the 'set_config()' hook. So, unless I'm
> missing something, it looks like this was never working for these chips.
> 
> Note that the ACPI case is only compiled tested. At first glance, it seems
> the current patch is enough but i'm not really sure...

So, I looked closer to the issue you are trying to describe here.

As far as I understand we have 4 state of BIAS in the hardware:
1/ AS IS (defined by firnware)
2/ Disabled (neither PU, not PD)
3/ PU
4/ PD

The case when the default of bias is not disabled (for example specific, and I
think very special, hardware may reset it to PD or PU), it's a hardware driver
responsibility to inform the framework about the real state of the lines and
synchronize it.

Another case is when the firmware sets the line in non-disabled state and
by some reason you need to disable it. The question is, why?

> As a side note, this came to my attention during this patchset [1]
> (and, ofr OF,  was tested with it).
> 
> [1]: https://lore.kernel.org/linux-input/20220708093448.42617-5-nuno.sa@analog.com/

Since this provides a GPIO chip, correct?, it's responsibility of the driver to
synchronize it, no? Basically if you really don't trust firmware, you may
go via all GPIO lines and switch them to the known (in software) state. This
approach on the other hand is error prone, because firmware should know better
which pin is used for which purpose, no? If you don't trust firwmare (in some
cases), then it's a matter of buggy platform that has to be quirked out.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/4] gpiolib: add support for bias pull disable

[Andy Shevchenko]

On Thu, Jul 14, 2022 at 03:02:08PM +0200, Nuno Sá wrote:
> On Thu, 2022-07-14 at 20:00 +0800, Kent Gibson wrote:
> > On Thu, Jul 14, 2022 at 10:47:27AM +0200, Nuno Sá wrote:

...

> > If that solves your immediate problem then you need to decide what
> > problem your patch is trying to address.
> 
> So my patch is trying to solve exactly this case because (IMO), it does
> not make sense for consumers drivers to have to do the above code.
> Moreover, they would need some custom firmware property (eg:
> devicetree) for the cases where we want to disable BIAS since we cannot
> just assume we want to do it.

Why? This is the main question. Why do you need that _in kernel_ API.

> Well, maybe we can just assume FLAG_BIAS_DISABLE in gpiolib (when
> trying to get the gpiod) if no PULL is specified. However, I do have
> some concerns with it (see my conversation with Andy in the cover).

It's AS IS if you trust firmware.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/4] add support for bias pull-disable

[Nuno Sá]

On Thu, 2022-07-14 at 17:58 +0300, Andy Shevchenko wrote:
> On Wed, Jul 13, 2022 at 03:14:17PM +0200, Nuno Sá wrote:
> > The gpio core looks at 'FLAG_BIAS_DISABLE' in preparation of
> > calling the
> > gpiochip 'set_config()' hook. However, AFAICT, there's no way that
> > this
> > flag is set because there's no support for it in firwmare code.
> > Moreover,
> > in 'gpiod_configure_flags()', only pull-ups and pull-downs are
> > being
> > handled.
> > 
> > On top of this, there are some users that are looking at
> > 'PIN_CONFIG_BIAS_DISABLE' in the 'set_config()' hook. So, unless
> > I'm
> > missing something, it looks like this was never working for these
> > chips.
> > 
> > Note that the ACPI case is only compiled tested. At first glance,
> > it seems
> > the current patch is enough but i'm not really sure...
> 
> So, I looked closer to the issue you are trying to describe here.
> 
> As far as I understand we have 4 state of BIAS in the hardware:
> 1/ AS IS (defined by firnware)
> 2/ Disabled (neither PU, not PD)
> 3/ PU
> 4/ PD
> 
> The case when the default of bias is not disabled (for example
> specific, and I
> think very special, hardware may reset it to PD or PU), it's a
> hardware driver
> responsibility to inform the framework about the real state of the
> lines and
> synchronize it.
> 
> Another case is when the firmware sets the line in non-disabled state
> and
> by some reason you need to disable it. The question is, why?
> 

Not getting this point... 

> > As a side note, this came to my attention during this patchset [1]
> > (and, ofr OF,  was tested with it).
> > 
> > [1]:
> > https://lore.kernel.org/linux-input/20220708093448.42617-5-nuno.sa@analog.com/
> 
> Since this provides a GPIO chip, correct?, it's responsibility of the
> driver to
> synchronize it, no? Basically if you really don't trust firmware, you
> may

What do you mean by synchronize?

> go via all GPIO lines and switch them to the known (in software)
> state. This
> approach on the other hand is error prone, because firmware should
> know better
> which pin is used for which purpose, no? If you don't trust firwmare
> (in some
> cases), then it's a matter of buggy platform that has to be quirked
> out.
> 

I'm not getting what you mean by "firmware should know better"? So,
basically, and let's take OF as example, you can request a GPIO in OF
by doing something like:

	foo-gpios = <&gpio 1 GPIO_PULL_UP>;

In this way, when the consumer driver gets the gpio, all the flags will
be properly set so that when we set a direction (for example) the
gpiochip's 'set_config()' will be called and the driver does what it
needs to setup the pull-up. If we want BIAS_DISABLED on the pin,
there's no way to the same as the above. So basically, this can ever
happen:

https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib.c#L2227

(only possible from the gpiochip cdev interface)

So, what I'm proposing is to be possible to do from OF:

	foo-gpios = <&gpio 1 GPIO_PULL_DISABLE>;

And then we will get into the gpiochip's 'set_config()' to disable the
pull-up or pull-down.

As I said, my device is an input keymap that can export pins as GPIOs
(to be consumed by gpio_keys). The pins by default have pull-ups that
can be disabled by doing a device i2c write. I'm just trying to use the
infrastructure that already exists in gpiolib (for pull-up|down) to
accomplish this. There's no pinctrl driver controlling the pins. The
device itself controls them and having this device as a pinctrl one is
not really applicable.



- Nuno Sá

Re: [PATCH 1/4] gpiolib: add support for bias pull disable

[Nuno Sá]

On Thu, 2022-07-14 at 18:08 +0300, Andy Shevchenko wrote:
> On Thu, Jul 14, 2022 at 03:02:08PM +0200, Nuno Sá wrote:
> > On Thu, 2022-07-14 at 20:00 +0800, Kent Gibson wrote:
> > > On Thu, Jul 14, 2022 at 10:47:27AM +0200, Nuno Sá wrote:
> 
> ...
> 
> > > If that solves your immediate problem then you need to decide
> > > what
> > > problem your patch is trying to address.
> > 
> > So my patch is trying to solve exactly this case because (IMO), it
> > does
> > not make sense for consumers drivers to have to do the above code.
> > Moreover, they would need some custom firmware property (eg:
> > devicetree) for the cases where we want to disable BIAS since we
> > cannot
> > just assume we want to do it.
> 
> Why? This is the main question. Why do you need that _in kernel_ API.
> 
> > Well, maybe we can just assume FLAG_BIAS_DISABLE in gpiolib (when
> > trying to get the gpiod) if no PULL is specified. However, I do
> > have
> > some concerns with it (see my conversation with Andy in the cover).
> 
> It's AS IS if you trust firmware.
> 

In my use case, there's no firmware controlling the pin... The input
driver (which exposes the gpichip) directly controls the pins and I
want to have a way to tell it (from firmware) to disable the BIAS if
users want to do so (by default it's pull-up)...

- Nuno Sá

Re: [PATCH 0/4] add support for bias pull-disable

[Andy Shevchenko]

On Thu, Jul 14, 2022 at 05:43:41PM +0200, Nuno Sá wrote:
> On Thu, 2022-07-14 at 17:58 +0300, Andy Shevchenko wrote:
> > On Wed, Jul 13, 2022 at 03:14:17PM +0200, Nuno Sá wrote:
> > > The gpio core looks at 'FLAG_BIAS_DISABLE' in preparation of
> > > calling the
> > > gpiochip 'set_config()' hook. However, AFAICT, there's no way that
> > > this
> > > flag is set because there's no support for it in firwmare code.
> > > Moreover,
> > > in 'gpiod_configure_flags()', only pull-ups and pull-downs are
> > > being
> > > handled.
> > > 
> > > On top of this, there are some users that are looking at
> > > 'PIN_CONFIG_BIAS_DISABLE' in the 'set_config()' hook. So, unless
> > > I'm
> > > missing something, it looks like this was never working for these
> > > chips.
> > > 
> > > Note that the ACPI case is only compiled tested. At first glance,
> > > it seems
> > > the current patch is enough but i'm not really sure...
> > 
> > So, I looked closer to the issue you are trying to describe here.
> > 
> > As far as I understand we have 4 state of BIAS in the hardware:
> > 1/ AS IS (defined by firnware)
> > 2/ Disabled (neither PU, not PD)
> > 3/ PU
> > 4/ PD
> > 
> > The case when the default of bias is not disabled (for example
> > specific, and I
> > think very special, hardware may reset it to PD or PU), it's a
> > hardware driver
> > responsibility to inform the framework about the real state of the
> > lines and
> > synchronize it.
> > 
> > Another case is when the firmware sets the line in non-disabled state
> > and
> > by some reason you need to disable it. The question is, why?
> 
> Not getting this point... 

I understand that in your case "firmware" is what DTB provides.
So taking into account that the default of hardware is PU, it needs
a mechanism to override that, correct?

> > > As a side note, this came to my attention during this patchset [1]
> > > (and, ofr OF,  was tested with it).
> > > 
> > > [1]:
> > > https://lore.kernel.org/linux-input/20220708093448.42617-5-nuno.sa@analog.com/
> > 
> > Since this provides a GPIO chip, correct?, it's responsibility of the
> > driver to
> > synchronize it, no? Basically if you really don't trust firmware, you
> > may
> 
> What do you mean by synchronize?

Full duplex sync, i.e. setting flag to PU for the pins that should stay PU:ed
and disabling bias for the ones, that want it to be disabled. (PD accordingly)

> > go via all GPIO lines and switch them to the known (in software)
> > state. This
> > approach on the other hand is error prone, because firmware should
> > know better
> > which pin is used for which purpose, no? If you don't trust firwmare
> > (in some
> > cases), then it's a matter of buggy platform that has to be quirked
> > out.
> 
> I'm not getting what you mean by "firmware should know better"? So,
> basically, and let's take OF as example, you can request a GPIO in OF
> by doing something like:
> 
> 	foo-gpios = <&gpio 1 GPIO_PULL_UP>;
> 
> In this way, when the consumer driver gets the gpio, all the flags will
> be properly set so that when we set a direction (for example) the
> gpiochip's 'set_config()' will be called and the driver does what it
> needs to setup the pull-up. If we want BIAS_DISABLED on the pin,
> there's no way to the same as the above. So basically, this can ever
> happen:
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib.c#L2227
> 
> (only possible from the gpiochip cdev interface)
> 
> So, what I'm proposing is to be possible to do from OF:
> 
> 	foo-gpios = <&gpio 1 GPIO_PULL_DISABLE>;
> 
> And then we will get into the gpiochip's 'set_config()' to disable the
> pull-up or pull-down.
> 
> As I said, my device is an input keymap that can export pins as GPIOs
> (to be consumed by gpio_keys). The pins by default have pull-ups that
> can be disabled by doing a device i2c write. I'm just trying to use the
> infrastructure that already exists in gpiolib (for pull-up|down) to
> accomplish this. There's no pinctrl driver controlling the pins. The
> device itself controls them and having this device as a pinctrl one is
> not really applicable.

Yes, I have got it eventually. The root cause is that after reset you have a
hardware that doesn't disable bias.

Now, we have DT properties for PD and PU, correct?
For each requested pin you decide either to leave the state as it is, or
apply bias.

in ->probe() of your GPIO you reset hardware and for each GPIO descriptor you
set PU flag.
In ->request(), don;t know the name by heart, you disable BIAS based on absence
of flags, it can be done without an additional properties, purely in the GPIO
OF code. Do I understand this correctly?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/4] add support for bias pull-disable

[Nuno Sá]

On Thu, 2022-07-14 at 21:57 +0300, Andy Shevchenko wrote:
> On Thu, Jul 14, 2022 at 05:43:41PM +0200, Nuno Sá wrote:
> > On Thu, 2022-07-14 at 17:58 +0300, Andy Shevchenko wrote:
> > > On Wed, Jul 13, 2022 at 03:14:17PM +0200, Nuno Sá wrote:
> > > > The gpio core looks at 'FLAG_BIAS_DISABLE' in preparation of
> > > > calling the
> > > > gpiochip 'set_config()' hook. However, AFAICT, there's no way
> > > > that
> > > > this
> > > > flag is set because there's no support for it in firwmare code.
> > > > Moreover,
> > > > in 'gpiod_configure_flags()', only pull-ups and pull-downs are
> > > > being
> > > > handled.
> > > > 
> > > > On top of this, there are some users that are looking at
> > > > 'PIN_CONFIG_BIAS_DISABLE' in the 'set_config()' hook. So,
> > > > unless
> > > > I'm
> > > > missing something, it looks like this was never working for
> > > > these
> > > > chips.
> > > > 
> > > > Note that the ACPI case is only compiled tested. At first
> > > > glance,
> > > > it seems
> > > > the current patch is enough but i'm not really sure...
> > > 
> > > So, I looked closer to the issue you are trying to describe here.
> > > 
> > > As far as I understand we have 4 state of BIAS in the hardware:
> > > 1/ AS IS (defined by firnware)
> > > 2/ Disabled (neither PU, not PD)
> > > 3/ PU
> > > 4/ PD
> > > 
> > > The case when the default of bias is not disabled (for example
> > > specific, and I
> > > think very special, hardware may reset it to PD or PU), it's a
> > > hardware driver
> > > responsibility to inform the framework about the real state of
> > > the
> > > lines and
> > > synchronize it.
> > > 
> > > Another case is when the firmware sets the line in non-disabled
> > > state
> > > and
> > > by some reason you need to disable it. The question is, why?
> > 
> > Not getting this point... 
> 
> I understand that in your case "firmware" is what DTB provides.
> So taking into account that the default of hardware is PU, it needs
> a mechanism to override that, correct?
> 

Exactly...

> > > > As a side note, this came to my attention during this patchset
> > > > [1]
> > > > (and, ofr OF,  was tested with it).
> > > > 
> > > > [1]:
> > > > https://lore.kernel.org/linux-input/20220708093448.42617-5-nuno.sa@analog.com/
> > > 
> > > Since this provides a GPIO chip, correct?, it's responsibility of
> > > the
> > > driver to
> > > synchronize it, no? Basically if you really don't trust firmware,
> > > you
> > > may
> > 
> > What do you mean by synchronize?
> 
> Full duplex sync, i.e. setting flag to PU for the pins that should
> stay PU:ed
> and disabling bias for the ones, that want it to be disabled. (PD
> accordingly)
> 
> > > go via all GPIO lines and switch them to the known (in software)
> > > state. This
> > > approach on the other hand is error prone, because firmware
> > > should
> > > know better
> > > which pin is used for which purpose, no? If you don't trust
> > > firwmare
> > > (in some
> > > cases), then it's a matter of buggy platform that has to be
> > > quirked
> > > out.
> > 
> > I'm not getting what you mean by "firmware should know better"? So,
> > basically, and let's take OF as example, you can request a GPIO in
> > OF
> > by doing something like:
> > 
> >         foo-gpios = <&gpio 1 GPIO_PULL_UP>;
> > 
> > In this way, when the consumer driver gets the gpio, all the flags
> > will
> > be properly set so that when we set a direction (for example) the
> > gpiochip's 'set_config()' will be called and the driver does what
> > it
> > needs to setup the pull-up. If we want BIAS_DISABLED on the pin,
> > there's no way to the same as the above. So basically, this can
> > ever
> > happen:
> > 
> > https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib.c#L2227
> > 
> > (only possible from the gpiochip cdev interface)
> > 
> > So, what I'm proposing is to be possible to do from OF:
> > 
> >         foo-gpios = <&gpio 1 GPIO_PULL_DISABLE>;
> > 
> > And then we will get into the gpiochip's 'set_config()' to disable
> > the
> > pull-up or pull-down.
> > 
> > As I said, my device is an input keymap that can export pins as
> > GPIOs
> > (to be consumed by gpio_keys). The pins by default have pull-ups
> > that
> > can be disabled by doing a device i2c write. I'm just trying to use
> > the
> > infrastructure that already exists in gpiolib (for pull-up|down) to
> > accomplish this. There's no pinctrl driver controlling the pins.
> > The
> > device itself controls them and having this device as a pinctrl one
> > is
> > not really applicable.
> 
> Yes, I have got it eventually. The root cause is that after reset you
> have a
> hardware that doesn't disable bias.
> 
> Now, we have DT properties for PD and PU, correct?
> For each requested pin you decide either to leave the state as it is,
> or
> apply bias.
> 
> in ->probe() of your GPIO you reset hardware and for each GPIO
> descriptor you
> set PU flag.
> In ->request(), don;t know the name by heart, you disable BIAS based
> on absence
> of flags, it can be done without an additional properties, purely in
> the GPIO
> OF code. Do I understand this correctly?
> 
> 

Alright, I think now you got it and we are on the same page. If I
understood your suggestion, users would just use GPIO_PULL_UP in dtb if
wanting the default behavior. I would then use the gpiochip 'request()'
callback to test the for pull-up flag right?

If I'm getting this right, there's a problem with it...
gpiod_configure_flags() is called after gpiod_request() which means
that the gpiod descriptor won't still have the BIAS flags set. And I
don't think there's a way (at least clean and easy) to get the firmware
lookup flags from the request callback?

So, honestly the only option I see to do it without changing gpioblib
would be to hook this change in output/input callbacks which is far
from being optimal...

So, in the end having this explicitly like this feels the best option
to me. Sure, I can find some workaround in my driver but that does not
change this...


"
git grep "PIN_CONFIG_BIAS_DISABLE" drivers/gpio/

drivers/gpio/gpio-aspeed.c:963: else if (param ==
PIN_CONFIG_BIAS_DISABLE ||
drivers/gpio/gpio-merrifield.c:197:     if
((pinconf_to_config_param(config) == PIN_CONFIG_BIAS_DISABLE) ||
drivers/gpio/gpio-omap.c:903:   case PIN_CONFIG_BIAS_DISABLE:
drivers/gpio/gpio-pca953x.c:573:        if (config ==
PIN_CONFIG_BIAS_DISABLE)
drivers/gpio/gpio-pca953x.c:592:        case PIN_CONFIG_BIAS_DISABLE:
"

AFAICT, the only way this path is possible for these drivers is through
gpiolib cdev which might not be what the authors of the drivers were
expecting...

- Nuno Sá

Re: [PATCH 0/4] add support for bias pull-disable

[Andy Shevchenko]

On Fri, Jul 15, 2022 at 12:20:56PM +0200, Nuno Sá wrote:
> On Thu, 2022-07-14 at 21:57 +0300, Andy Shevchenko wrote:
> > On Thu, Jul 14, 2022 at 05:43:41PM +0200, Nuno Sá wrote:
> > > On Thu, 2022-07-14 at 17:58 +0300, Andy Shevchenko wrote:
> > > > On Wed, Jul 13, 2022 at 03:14:17PM +0200, Nuno Sá wrote:
> > > > > The gpio core looks at 'FLAG_BIAS_DISABLE' in preparation of
> > > > > calling the
> > > > > gpiochip 'set_config()' hook. However, AFAICT, there's no way
> > > > > that
> > > > > this
> > > > > flag is set because there's no support for it in firwmare code.
> > > > > Moreover,
> > > > > in 'gpiod_configure_flags()', only pull-ups and pull-downs are
> > > > > being
> > > > > handled.
> > > > > 
> > > > > On top of this, there are some users that are looking at
> > > > > 'PIN_CONFIG_BIAS_DISABLE' in the 'set_config()' hook. So,
> > > > > unless
> > > > > I'm
> > > > > missing something, it looks like this was never working for
> > > > > these
> > > > > chips.
> > > > > 
> > > > > Note that the ACPI case is only compiled tested. At first
> > > > > glance,
> > > > > it seems
> > > > > the current patch is enough but i'm not really sure...
> > > > 
> > > > So, I looked closer to the issue you are trying to describe here.
> > > > 
> > > > As far as I understand we have 4 state of BIAS in the hardware:
> > > > 1/ AS IS (defined by firnware)
> > > > 2/ Disabled (neither PU, not PD)
> > > > 3/ PU
> > > > 4/ PD
> > > > 
> > > > The case when the default of bias is not disabled (for example
> > > > specific, and I
> > > > think very special, hardware may reset it to PD or PU), it's a
> > > > hardware driver
> > > > responsibility to inform the framework about the real state of
> > > > the
> > > > lines and
> > > > synchronize it.
> > > > 
> > > > Another case is when the firmware sets the line in non-disabled
> > > > state
> > > > and
> > > > by some reason you need to disable it. The question is, why?
> > > 
> > > Not getting this point... 
> > 
> > I understand that in your case "firmware" is what DTB provides.
> > So taking into account that the default of hardware is PU, it needs
> > a mechanism to override that, correct?
> > 
> 
> Exactly...
> 
> > > > > As a side note, this came to my attention during this patchset
> > > > > [1]
> > > > > (and, ofr OF,  was tested with it).
> > > > > 
> > > > > [1]:
> > > > > https://lore.kernel.org/linux-input/20220708093448.42617-5-nuno.sa@analog.com/
> > > > 
> > > > Since this provides a GPIO chip, correct?, it's responsibility of
> > > > the
> > > > driver to
> > > > synchronize it, no? Basically if you really don't trust firmware,
> > > > you
> > > > may
> > > 
> > > What do you mean by synchronize?
> > 
> > Full duplex sync, i.e. setting flag to PU for the pins that should
> > stay PU:ed
> > and disabling bias for the ones, that want it to be disabled. (PD
> > accordingly)
> > 
> > > > go via all GPIO lines and switch them to the known (in software)
> > > > state. This
> > > > approach on the other hand is error prone, because firmware
> > > > should
> > > > know better
> > > > which pin is used for which purpose, no? If you don't trust
> > > > firwmare
> > > > (in some
> > > > cases), then it's a matter of buggy platform that has to be
> > > > quirked
> > > > out.
> > > 
> > > I'm not getting what you mean by "firmware should know better"? So,
> > > basically, and let's take OF as example, you can request a GPIO in
> > > OF
> > > by doing something like:
> > > 
> > >         foo-gpios = <&gpio 1 GPIO_PULL_UP>;
> > > 
> > > In this way, when the consumer driver gets the gpio, all the flags
> > > will
> > > be properly set so that when we set a direction (for example) the
> > > gpiochip's 'set_config()' will be called and the driver does what
> > > it
> > > needs to setup the pull-up. If we want BIAS_DISABLED on the pin,
> > > there's no way to the same as the above. So basically, this can
> > > ever
> > > happen:
> > > 
> > > https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib.c#L2227
> > > 
> > > (only possible from the gpiochip cdev interface)
> > > 
> > > So, what I'm proposing is to be possible to do from OF:
> > > 
> > >         foo-gpios = <&gpio 1 GPIO_PULL_DISABLE>;
> > > 
> > > And then we will get into the gpiochip's 'set_config()' to disable
> > > the
> > > pull-up or pull-down.
> > > 
> > > As I said, my device is an input keymap that can export pins as
> > > GPIOs
> > > (to be consumed by gpio_keys). The pins by default have pull-ups
> > > that
> > > can be disabled by doing a device i2c write. I'm just trying to use
> > > the
> > > infrastructure that already exists in gpiolib (for pull-up|down) to
> > > accomplish this. There's no pinctrl driver controlling the pins.
> > > The
> > > device itself controls them and having this device as a pinctrl one
> > > is
> > > not really applicable.
> > 
> > Yes, I have got it eventually. The root cause is that after reset you
> > have a
> > hardware that doesn't disable bias.
> > 
> > Now, we have DT properties for PD and PU, correct?
> > For each requested pin you decide either to leave the state as it is,
> > or
> > apply bias.
> > 
> > in ->probe() of your GPIO you reset hardware and for each GPIO
> > descriptor you
> > set PU flag.
> > In ->request(), don;t know the name by heart, you disable BIAS based
> > on absence
> > of flags, it can be done without an additional properties, purely in
> > the GPIO
> > OF code. Do I understand this correctly?
> > 
> 
> Alright, I think now you got it and we are on the same page. If I
> understood your suggestion, users would just use GPIO_PULL_UP in dtb if
> wanting the default behavior. I would then use the gpiochip 'request()'
> callback to test the for pull-up flag right?

Something like this, yes.

> If I'm getting this right, there's a problem with it...
> gpiod_configure_flags() is called after gpiod_request() which means
> that the gpiod descriptor won't still have the BIAS flags set. And I
> don't think there's a way (at least clean and easy) to get the firmware
> lookup flags from the request callback?
> 
> So, honestly the only option I see to do it without changing gpioblib
> would be to hook this change in output/input callbacks which is far
> from being optimal...
> 
> So, in the end having this explicitly like this feels the best option
> to me. Sure, I can find some workaround in my driver but that does not
> change this...

Ok, let me think about it. Meanwhile, maybe others have better ideas already?

> "
> git grep "PIN_CONFIG_BIAS_DISABLE" drivers/gpio/

Hint: `git grep -lw "PIN_CONFIG_BIAS_DISABLE" -- drivers/gpio`

> drivers/gpio/gpio-aspeed.c:963: else if (param ==
> PIN_CONFIG_BIAS_DISABLE ||
> drivers/gpio/gpio-merrifield.c:197:     if
> ((pinconf_to_config_param(config) == PIN_CONFIG_BIAS_DISABLE) ||
> drivers/gpio/gpio-omap.c:903:   case PIN_CONFIG_BIAS_DISABLE:
> drivers/gpio/gpio-pca953x.c:573:        if (config ==
> PIN_CONFIG_BIAS_DISABLE)
> drivers/gpio/gpio-pca953x.c:592:        case PIN_CONFIG_BIAS_DISABLE:
> "
> 
> AFAICT, the only way this path is possible for these drivers is through
> gpiolib cdev which might not be what the authors of the drivers were
> expecting...

gpio-merrifield is bad example, it has a pin control.
gpio-pca953x as I said should effectively be a pin control driver.

For the two left it might be the case.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/4] add support for bias pull-disable

[Nuno Sá]

On Fri, 2022-07-15 at 15:05 +0300, Andy Shevchenko wrote:
> On Fri, Jul 15, 2022 at 12:20:56PM +0200, Nuno Sá wrote:
> > On Thu, 2022-07-14 at 21:57 +0300, Andy Shevchenko wrote:
> > > On Thu, Jul 14, 2022 at 05:43:41PM +0200, Nuno Sá wrote:
> > > > On Thu, 2022-07-14 at 17:58 +0300, Andy Shevchenko wrote:
> > > > > On Wed, Jul 13, 2022 at 03:14:17PM +0200, Nuno Sá wrote:
> > > > > > The gpio core looks at 'FLAG_BIAS_DISABLE' in preparation
> > > > > > of
> > > > > > calling the
> > > > > > gpiochip 'set_config()' hook. However, AFAICT, there's no
> > > > > > way
> > > > > > that
> > > > > > this
> > > > > > flag is set because there's no support for it in firwmare
> > > > > > code.
> > > > > > Moreover,
> > > > > > in 'gpiod_configure_flags()', only pull-ups and pull-downs
> > > > > > are
> > > > > > being
> > > > > > handled.
> > > > > > 
> > > > > > On top of this, there are some users that are looking at
> > > > > > 'PIN_CONFIG_BIAS_DISABLE' in the 'set_config()' hook. So,
> > > > > > unless
> > > > > > I'm
> > > > > > missing something, it looks like this was never working for
> > > > > > these
> > > > > > chips.
> > > > > > 
> > > > > > Note that the ACPI case is only compiled tested. At first
> > > > > > glance,
> > > > > > it seems
> > > > > > the current patch is enough but i'm not really sure...
> > > > > 
> > > > > So, I looked closer to the issue you are trying to describe
> > > > > here.
> > > > > 
> > > > > As far as I understand we have 4 state of BIAS in the
> > > > > hardware:
> > > > > 1/ AS IS (defined by firnware)
> > > > > 2/ Disabled (neither PU, not PD)
> > > > > 3/ PU
> > > > > 4/ PD
> > > > > 
> > > > > The case when the default of bias is not disabled (for
> > > > > example
> > > > > specific, and I
> > > > > think very special, hardware may reset it to PD or PU), it's
> > > > > a
> > > > > hardware driver
> > > > > responsibility to inform the framework about the real state
> > > > > of
> > > > > the
> > > > > lines and
> > > > > synchronize it.
> > > > > 
> > > > > Another case is when the firmware sets the line in non-
> > > > > disabled
> > > > > state
> > > > > and
> > > > > by some reason you need to disable it. The question is, why?
> > > > 
> > > > Not getting this point... 
> > > 
> > > I understand that in your case "firmware" is what DTB provides.
> > > So taking into account that the default of hardware is PU, it
> > > needs
> > > a mechanism to override that, correct?
> > > 
> > 
> > Exactly...
> > 
> > > > > > As a side note, this came to my attention during this
> > > > > > patchset
> > > > > > [1]
> > > > > > (and, ofr OF,  was tested with it).
> > > > > > 
> > > > > > [1]:
> > > > > > https://lore.kernel.org/linux-input/20220708093448.42617-5-nuno.sa@analog.com/
> > > > > 
> > > > > Since this provides a GPIO chip, correct?, it's
> > > > > responsibility of
> > > > > the
> > > > > driver to
> > > > > synchronize it, no? Basically if you really don't trust
> > > > > firmware,
> > > > > you
> > > > > may
> > > > 
> > > > What do you mean by synchronize?
> > > 
> > > Full duplex sync, i.e. setting flag to PU for the pins that
> > > should
> > > stay PU:ed
> > > and disabling bias for the ones, that want it to be disabled. (PD
> > > accordingly)
> > > 
> > > > > go via all GPIO lines and switch them to the known (in
> > > > > software)
> > > > > state. This
> > > > > approach on the other hand is error prone, because firmware
> > > > > should
> > > > > know better
> > > > > which pin is used for which purpose, no? If you don't trust
> > > > > firwmare
> > > > > (in some
> > > > > cases), then it's a matter of buggy platform that has to be
> > > > > quirked
> > > > > out.
> > > > 
> > > > I'm not getting what you mean by "firmware should know better"?
> > > > So,
> > > > basically, and let's take OF as example, you can request a GPIO
> > > > in
> > > > OF
> > > > by doing something like:
> > > > 
> > > >         foo-gpios = <&gpio 1 GPIO_PULL_UP>;
> > > > 
> > > > In this way, when the consumer driver gets the gpio, all the
> > > > flags
> > > > will
> > > > be properly set so that when we set a direction (for example)
> > > > the
> > > > gpiochip's 'set_config()' will be called and the driver does
> > > > what
> > > > it
> > > > needs to setup the pull-up. If we want BIAS_DISABLED on the
> > > > pin,
> > > > there's no way to the same as the above. So basically, this can
> > > > ever
> > > > happen:
> > > > 
> > > > https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib.c#L2227
> > > > 
> > > > (only possible from the gpiochip cdev interface)
> > > > 
> > > > So, what I'm proposing is to be possible to do from OF:
> > > > 
> > > >         foo-gpios = <&gpio 1 GPIO_PULL_DISABLE>;
> > > > 
> > > > And then we will get into the gpiochip's 'set_config()' to
> > > > disable
> > > > the
> > > > pull-up or pull-down.
> > > > 
> > > > As I said, my device is an input keymap that can export pins as
> > > > GPIOs
> > > > (to be consumed by gpio_keys). The pins by default have pull-
> > > > ups
> > > > that
> > > > can be disabled by doing a device i2c write. I'm just trying to
> > > > use
> > > > the
> > > > infrastructure that already exists in gpiolib (for pull-
> > > > up|down) to
> > > > accomplish this. There's no pinctrl driver controlling the
> > > > pins.
> > > > The
> > > > device itself controls them and having this device as a pinctrl
> > > > one
> > > > is
> > > > not really applicable.
> > > 
> > > Yes, I have got it eventually. The root cause is that after reset
> > > you
> > > have a
> > > hardware that doesn't disable bias.
> > > 
> > > Now, we have DT properties for PD and PU, correct?
> > > For each requested pin you decide either to leave the state as it
> > > is,
> > > or
> > > apply bias.
> > > 
> > > in ->probe() of your GPIO you reset hardware and for each GPIO
> > > descriptor you
> > > set PU flag.
> > > In ->request(), don;t know the name by heart, you disable BIAS
> > > based
> > > on absence
> > > of flags, it can be done without an additional properties, purely
> > > in
> > > the GPIO
> > > OF code. Do I understand this correctly?
> > > 
> > 
> > Alright, I think now you got it and we are on the same page. If I
> > understood your suggestion, users would just use GPIO_PULL_UP in
> > dtb if
> > wanting the default behavior. I would then use the gpiochip
> > 'request()'
> > callback to test the for pull-up flag right?
> 
> Something like this, yes.
> 
> > If I'm getting this right, there's a problem with it...
> > gpiod_configure_flags() is called after gpiod_request() which means
> > that the gpiod descriptor won't still have the BIAS flags set. And
> > I
> > don't think there's a way (at least clean and easy) to get the
> > firmware
> > lookup flags from the request callback?
> > 
> > So, honestly the only option I see to do it without changing
> > gpioblib
> > would be to hook this change in output/input callbacks which is far
> > from being optimal...
> > 
> > So, in the end having this explicitly like this feels the best
> > option
> > to me. Sure, I can find some workaround in my driver but that does
> > not
> > change this...
> 
> Ok, let me think about it. Meanwhile, maybe others have better ideas
> already?
> 

Sure, I'm still thinking that having this extra property and explicitly
set it from OF is not that bad :)

> > "
> > git grep "PIN_CONFIG_BIAS_DISABLE" drivers/gpio/
> 
> Hint: `git grep -lw "PIN_CONFIG_BIAS_DISABLE" -- drivers/gpio`
> 

nice..

> > drivers/gpio/gpio-aspeed.c:963: else if (param ==
> > PIN_CONFIG_BIAS_DISABLE ||
> > drivers/gpio/gpio-merrifield.c:197:     if
> > ((pinconf_to_config_param(config) == PIN_CONFIG_BIAS_DISABLE) ||
> > drivers/gpio/gpio-omap.c:903:   case PIN_CONFIG_BIAS_DISABLE:
> > drivers/gpio/gpio-pca953x.c:573:        if (config ==
> > PIN_CONFIG_BIAS_DISABLE)
> > drivers/gpio/gpio-pca953x.c:592:        case
> > PIN_CONFIG_BIAS_DISABLE:
> > "
> > 
> > AFAICT, the only way this path is possible for these drivers is
> > through
> > gpiolib cdev which might not be what the authors of the drivers
> > were
> > expecting...
> 
> gpio-merrifield is bad example, it has a pin control.
> gpio-pca953x as I said should effectively be a pin control driver.
> 
> For the two left it might be the case.
> 

Well the thing is that even if we have pinctrl like for example,
gpio-omap, it is still true that there's no way to get into
'omap_gpio_set_config()' for 'PIN_CONFIG_BIAS_DISABLE' and call
'gpiochip_generic_config()'.

(naturally in this case, one can directly use pinctrl properties to
control the pin but still...)


- Nuno Sá

Re: [PATCH 0/4] add support for bias pull-disable

[Bartosz Golaszewski]

On Fri, Jul 15, 2022 at 2:19 PM Nuno Sá <noname.nuno@gmail.com> wrote:
>
> On Fri, 2022-07-15 at 15:05 +0300, Andy Shevchenko wrote:
> > On Fri, Jul 15, 2022 at 12:20:56PM +0200, Nuno Sá wrote:
> > > On Thu, 2022-07-14 at 21:57 +0300, Andy Shevchenko wrote:
> > > > On Thu, Jul 14, 2022 at 05:43:41PM +0200, Nuno Sá wrote:
> > > > > On Thu, 2022-07-14 at 17:58 +0300, Andy Shevchenko wrote:
> > > > > > On Wed, Jul 13, 2022 at 03:14:17PM +0200, Nuno Sá wrote:
> > > > > > > The gpio core looks at 'FLAG_BIAS_DISABLE' in preparation
> > > > > > > of
> > > > > > > calling the
> > > > > > > gpiochip 'set_config()' hook. However, AFAICT, there's no
> > > > > > > way
> > > > > > > that
> > > > > > > this
> > > > > > > flag is set because there's no support for it in firwmare
> > > > > > > code.
> > > > > > > Moreover,
> > > > > > > in 'gpiod_configure_flags()', only pull-ups and pull-downs
> > > > > > > are
> > > > > > > being
> > > > > > > handled.
> > > > > > >
> > > > > > > On top of this, there are some users that are looking at
> > > > > > > 'PIN_CONFIG_BIAS_DISABLE' in the 'set_config()' hook. So,
> > > > > > > unless
> > > > > > > I'm
> > > > > > > missing something, it looks like this was never working for
> > > > > > > these
> > > > > > > chips.
> > > > > > >
> > > > > > > Note that the ACPI case is only compiled tested. At first
> > > > > > > glance,
> > > > > > > it seems
> > > > > > > the current patch is enough but i'm not really sure...
> > > > > >
> > > > > > So, I looked closer to the issue you are trying to describe
> > > > > > here.
> > > > > >
> > > > > > As far as I understand we have 4 state of BIAS in the
> > > > > > hardware:
> > > > > > 1/ AS IS (defined by firnware)
> > > > > > 2/ Disabled (neither PU, not PD)
> > > > > > 3/ PU
> > > > > > 4/ PD
> > > > > >
> > > > > > The case when the default of bias is not disabled (for
> > > > > > example
> > > > > > specific, and I
> > > > > > think very special, hardware may reset it to PD or PU), it's
> > > > > > a
> > > > > > hardware driver
> > > > > > responsibility to inform the framework about the real state
> > > > > > of
> > > > > > the
> > > > > > lines and
> > > > > > synchronize it.
> > > > > >
> > > > > > Another case is when the firmware sets the line in non-
> > > > > > disabled
> > > > > > state
> > > > > > and
> > > > > > by some reason you need to disable it. The question is, why?
> > > > >
> > > > > Not getting this point...
> > > >
> > > > I understand that in your case "firmware" is what DTB provides.
> > > > So taking into account that the default of hardware is PU, it
> > > > needs
> > > > a mechanism to override that, correct?
> > > >
> > >
> > > Exactly...
> > >
> > > > > > > As a side note, this came to my attention during this
> > > > > > > patchset
> > > > > > > [1]
> > > > > > > (and, ofr OF,  was tested with it).
> > > > > > >
> > > > > > > [1]:
> > > > > > > https://lore.kernel.org/linux-input/20220708093448.42617-5-nuno.sa@analog.com/
> > > > > >
> > > > > > Since this provides a GPIO chip, correct?, it's
> > > > > > responsibility of
> > > > > > the
> > > > > > driver to
> > > > > > synchronize it, no? Basically if you really don't trust
> > > > > > firmware,
> > > > > > you
> > > > > > may
> > > > >
> > > > > What do you mean by synchronize?
> > > >
> > > > Full duplex sync, i.e. setting flag to PU for the pins that
> > > > should
> > > > stay PU:ed
> > > > and disabling bias for the ones, that want it to be disabled. (PD
> > > > accordingly)
> > > >
> > > > > > go via all GPIO lines and switch them to the known (in
> > > > > > software)
> > > > > > state. This
> > > > > > approach on the other hand is error prone, because firmware
> > > > > > should
> > > > > > know better
> > > > > > which pin is used for which purpose, no? If you don't trust
> > > > > > firwmare
> > > > > > (in some
> > > > > > cases), then it's a matter of buggy platform that has to be
> > > > > > quirked
> > > > > > out.
> > > > >
> > > > > I'm not getting what you mean by "firmware should know better"?
> > > > > So,
> > > > > basically, and let's take OF as example, you can request a GPIO
> > > > > in
> > > > > OF
> > > > > by doing something like:
> > > > >
> > > > >         foo-gpios = <&gpio 1 GPIO_PULL_UP>;
> > > > >
> > > > > In this way, when the consumer driver gets the gpio, all the
> > > > > flags
> > > > > will
> > > > > be properly set so that when we set a direction (for example)
> > > > > the
> > > > > gpiochip's 'set_config()' will be called and the driver does
> > > > > what
> > > > > it
> > > > > needs to setup the pull-up. If we want BIAS_DISABLED on the
> > > > > pin,
> > > > > there's no way to the same as the above. So basically, this can
> > > > > ever
> > > > > happen:
> > > > >
> > > > > https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib.c#L2227
> > > > >
> > > > > (only possible from the gpiochip cdev interface)
> > > > >
> > > > > So, what I'm proposing is to be possible to do from OF:
> > > > >
> > > > >         foo-gpios = <&gpio 1 GPIO_PULL_DISABLE>;
> > > > >
> > > > > And then we will get into the gpiochip's 'set_config()' to
> > > > > disable
> > > > > the
> > > > > pull-up or pull-down.
> > > > >
> > > > > As I said, my device is an input keymap that can export pins as
> > > > > GPIOs
> > > > > (to be consumed by gpio_keys). The pins by default have pull-
> > > > > ups
> > > > > that
> > > > > can be disabled by doing a device i2c write. I'm just trying to
> > > > > use
> > > > > the
> > > > > infrastructure that already exists in gpiolib (for pull-
> > > > > up|down) to
> > > > > accomplish this. There's no pinctrl driver controlling the
> > > > > pins.
> > > > > The
> > > > > device itself controls them and having this device as a pinctrl
> > > > > one
> > > > > is
> > > > > not really applicable.
> > > >
> > > > Yes, I have got it eventually. The root cause is that after reset
> > > > you
> > > > have a
> > > > hardware that doesn't disable bias.
> > > >
> > > > Now, we have DT properties for PD and PU, correct?
> > > > For each requested pin you decide either to leave the state as it
> > > > is,
> > > > or
> > > > apply bias.
> > > >
> > > > in ->probe() of your GPIO you reset hardware and for each GPIO
> > > > descriptor you
> > > > set PU flag.
> > > > In ->request(), don;t know the name by heart, you disable BIAS
> > > > based
> > > > on absence
> > > > of flags, it can be done without an additional properties, purely
> > > > in
> > > > the GPIO
> > > > OF code. Do I understand this correctly?
> > > >
> > >
> > > Alright, I think now you got it and we are on the same page. If I
> > > understood your suggestion, users would just use GPIO_PULL_UP in
> > > dtb if
> > > wanting the default behavior. I would then use the gpiochip
> > > 'request()'
> > > callback to test the for pull-up flag right?
> >
> > Something like this, yes.
> >
> > > If I'm getting this right, there's a problem with it...
> > > gpiod_configure_flags() is called after gpiod_request() which means
> > > that the gpiod descriptor won't still have the BIAS flags set. And
> > > I
> > > don't think there's a way (at least clean and easy) to get the
> > > firmware
> > > lookup flags from the request callback?
> > >
> > > So, honestly the only option I see to do it without changing
> > > gpioblib
> > > would be to hook this change in output/input callbacks which is far
> > > from being optimal...
> > >
> > > So, in the end having this explicitly like this feels the best
> > > option
> > > to me. Sure, I can find some workaround in my driver but that does
> > > not
> > > change this...
> >
> > Ok, let me think about it. Meanwhile, maybe others have better ideas
> > already?
> >
>
> Sure, I'm still thinking that having this extra property and explicitly
> set it from OF is not that bad :)
>
> > > "
> > > git grep "PIN_CONFIG_BIAS_DISABLE" drivers/gpio/
> >
> > Hint: `git grep -lw "PIN_CONFIG_BIAS_DISABLE" -- drivers/gpio`
> >
>
> nice..
>
> > > drivers/gpio/gpio-aspeed.c:963: else if (param ==
> > > PIN_CONFIG_BIAS_DISABLE ||
> > > drivers/gpio/gpio-merrifield.c:197:     if
> > > ((pinconf_to_config_param(config) == PIN_CONFIG_BIAS_DISABLE) ||
> > > drivers/gpio/gpio-omap.c:903:   case PIN_CONFIG_BIAS_DISABLE:
> > > drivers/gpio/gpio-pca953x.c:573:        if (config ==
> > > PIN_CONFIG_BIAS_DISABLE)
> > > drivers/gpio/gpio-pca953x.c:592:        case
> > > PIN_CONFIG_BIAS_DISABLE:
> > > "
> > >
> > > AFAICT, the only way this path is possible for these drivers is
> > > through
> > > gpiolib cdev which might not be what the authors of the drivers
> > > were
> > > expecting...
> >
> > gpio-merrifield is bad example, it has a pin control.
> > gpio-pca953x as I said should effectively be a pin control driver.
> >
> > For the two left it might be the case.
> >
>
> Well the thing is that even if we have pinctrl like for example,
> gpio-omap, it is still true that there's no way to get into
> 'omap_gpio_set_config()' for 'PIN_CONFIG_BIAS_DISABLE' and call
> 'gpiochip_generic_config()'.
>
> (naturally in this case, one can directly use pinctrl properties to
> control the pin but still...)
>
>
> - Nuno Sá
>

Ideologically I don't have anything against adding this flag (except
that it should be called BIAS_DISABLE not PULL_DISABLE IMO). Nuno is
right in that the character device is the only way to set this mode
ATM and. However I would like to see the first user added together
with the series because adding features nobody uses in the mainline
kernel tree is generally frowned upon and it's also not clear that
anyone actually wants to use it.

Bart

Re: [PATCH 0/4] add support for bias pull-disable

[Nuno Sá]

On Fri, 2022-07-15 at 21:31 +0200, Bartosz Golaszewski wrote:
> On Fri, Jul 15, 2022 at 2:19 PM Nuno Sá <noname.nuno@gmail.com>
> wrote:
> > 
> > On Fri, 2022-07-15 at 15:05 +0300, Andy Shevchenko wrote:
> > > On Fri, Jul 15, 2022 at 12:20:56PM +0200, Nuno Sá wrote:
> > > > On Thu, 2022-07-14 at 21:57 +0300, Andy Shevchenko wrote:
> > > > > On Thu, Jul 14, 2022 at 05:43:41PM +0200, Nuno Sá wrote:
> > > > > > On Thu, 2022-07-14 at 17:58 +0300, Andy Shevchenko wrote:
> > > > > > > On Wed, Jul 13, 2022 at 03:14:17PM +0200, Nuno Sá wrote:
> > > > > > > > The gpio core looks at 'FLAG_BIAS_DISABLE' in
> > > > > > > > preparation
> > > > > > > > of
> > > > > > > > calling the
> > > > > > > > gpiochip 'set_config()' hook. However, AFAICT, there's
> > > > > > > > no
> > > > > > > > way
> > > > > > > > that
> > > > > > > > this
> > > > > > > > flag is set because there's no support for it in
> > > > > > > > firwmare
> > > > > > > > code.
> > > > > > > > Moreover,
> > > > > > > > in 'gpiod_configure_flags()', only pull-ups and pull-
> > > > > > > > downs
> > > > > > > > are
> > > > > > > > being
> > > > > > > > handled.
> > > > > > > > 
> > > > > > > > On top of this, there are some users that are looking
> > > > > > > > at
> > > > > > > > 'PIN_CONFIG_BIAS_DISABLE' in the 'set_config()' hook.
> > > > > > > > So,
> > > > > > > > unless
> > > > > > > > I'm
> > > > > > > > missing something, it looks like this was never working
> > > > > > > > for
> > > > > > > > these
> > > > > > > > chips.
> > > > > > > > 
> > > > > > > > Note that the ACPI case is only compiled tested. At
> > > > > > > > first
> > > > > > > > glance,
> > > > > > > > it seems
> > > > > > > > the current patch is enough but i'm not really sure...
> > > > > > > 
> > > > > > > So, I looked closer to the issue you are trying to
> > > > > > > describe
> > > > > > > here.
> > > > > > > 
> > > > > > > As far as I understand we have 4 state of BIAS in the
> > > > > > > hardware:
> > > > > > > 1/ AS IS (defined by firnware)
> > > > > > > 2/ Disabled (neither PU, not PD)
> > > > > > > 3/ PU
> > > > > > > 4/ PD
> > > > > > > 
> > > > > > > The case when the default of bias is not disabled (for
> > > > > > > example
> > > > > > > specific, and I
> > > > > > > think very special, hardware may reset it to PD or PU),
> > > > > > > it's
> > > > > > > a
> > > > > > > hardware driver
> > > > > > > responsibility to inform the framework about the real
> > > > > > > state
> > > > > > > of
> > > > > > > the
> > > > > > > lines and
> > > > > > > synchronize it.
> > > > > > > 
> > > > > > > Another case is when the firmware sets the line in non-
> > > > > > > disabled
> > > > > > > state
> > > > > > > and
> > > > > > > by some reason you need to disable it. The question is,
> > > > > > > why?
> > > > > > 
> > > > > > Not getting this point...
> > > > > 
> > > > > I understand that in your case "firmware" is what DTB
> > > > > provides.
> > > > > So taking into account that the default of hardware is PU, it
> > > > > needs
> > > > > a mechanism to override that, correct?
> > > > > 
> > > > 
> > > > Exactly...
> > > > 
> > > > > > > > As a side note, this came to my attention during this
> > > > > > > > patchset
> > > > > > > > [1]
> > > > > > > > (and, ofr OF,  was tested with it).
> > > > > > > > 
> > > > > > > > [1]:
> > > > > > > > https://lore.kernel.org/linux-input/20220708093448.42617-5-nuno.sa@analog.com/
> > > > > > > 
> > > > > > > Since this provides a GPIO chip, correct?, it's
> > > > > > > responsibility of
> > > > > > > the
> > > > > > > driver to
> > > > > > > synchronize it, no? Basically if you really don't trust
> > > > > > > firmware,
> > > > > > > you
> > > > > > > may
> > > > > > 
> > > > > > What do you mean by synchronize?
> > > > > 
> > > > > Full duplex sync, i.e. setting flag to PU for the pins that
> > > > > should
> > > > > stay PU:ed
> > > > > and disabling bias for the ones, that want it to be disabled.
> > > > > (PD
> > > > > accordingly)
> > > > > 
> > > > > > > go via all GPIO lines and switch them to the known (in
> > > > > > > software)
> > > > > > > state. This
> > > > > > > approach on the other hand is error prone, because
> > > > > > > firmware
> > > > > > > should
> > > > > > > know better
> > > > > > > which pin is used for which purpose, no? If you don't
> > > > > > > trust
> > > > > > > firwmare
> > > > > > > (in some
> > > > > > > cases), then it's a matter of buggy platform that has to
> > > > > > > be
> > > > > > > quirked
> > > > > > > out.
> > > > > > 
> > > > > > I'm not getting what you mean by "firmware should know
> > > > > > better"?
> > > > > > So,
> > > > > > basically, and let's take OF as example, you can request a
> > > > > > GPIO
> > > > > > in
> > > > > > OF
> > > > > > by doing something like:
> > > > > > 
> > > > > >         foo-gpios = <&gpio 1 GPIO_PULL_UP>;
> > > > > > 
> > > > > > In this way, when the consumer driver gets the gpio, all
> > > > > > the
> > > > > > flags
> > > > > > will
> > > > > > be properly set so that when we set a direction (for
> > > > > > example)
> > > > > > the
> > > > > > gpiochip's 'set_config()' will be called and the driver
> > > > > > does
> > > > > > what
> > > > > > it
> > > > > > needs to setup the pull-up. If we want BIAS_DISABLED on the
> > > > > > pin,
> > > > > > there's no way to the same as the above. So basically, this
> > > > > > can
> > > > > > ever
> > > > > > happen:
> > > > > > 
> > > > > > https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib.c#L2227
> > > > > > 
> > > > > > (only possible from the gpiochip cdev interface)
> > > > > > 
> > > > > > So, what I'm proposing is to be possible to do from OF:
> > > > > > 
> > > > > >         foo-gpios = <&gpio 1 GPIO_PULL_DISABLE>;
> > > > > > 
> > > > > > And then we will get into the gpiochip's 'set_config()' to
> > > > > > disable
> > > > > > the
> > > > > > pull-up or pull-down.
> > > > > > 
> > > > > > As I said, my device is an input keymap that can export
> > > > > > pins as
> > > > > > GPIOs
> > > > > > (to be consumed by gpio_keys). The pins by default have
> > > > > > pull-
> > > > > > ups
> > > > > > that
> > > > > > can be disabled by doing a device i2c write. I'm just
> > > > > > trying to
> > > > > > use
> > > > > > the
> > > > > > infrastructure that already exists in gpiolib (for pull-
> > > > > > up|down) to
> > > > > > accomplish this. There's no pinctrl driver controlling the
> > > > > > pins.
> > > > > > The
> > > > > > device itself controls them and having this device as a
> > > > > > pinctrl
> > > > > > one
> > > > > > is
> > > > > > not really applicable.
> > > > > 
> > > > > Yes, I have got it eventually. The root cause is that after
> > > > > reset
> > > > > you
> > > > > have a
> > > > > hardware that doesn't disable bias.
> > > > > 
> > > > > Now, we have DT properties for PD and PU, correct?
> > > > > For each requested pin you decide either to leave the state
> > > > > as it
> > > > > is,
> > > > > or
> > > > > apply bias.
> > > > > 
> > > > > in ->probe() of your GPIO you reset hardware and for each
> > > > > GPIO
> > > > > descriptor you
> > > > > set PU flag.
> > > > > In ->request(), don;t know the name by heart, you disable
> > > > > BIAS
> > > > > based
> > > > > on absence
> > > > > of flags, it can be done without an additional properties,
> > > > > purely
> > > > > in
> > > > > the GPIO
> > > > > OF code. Do I understand this correctly?
> > > > > 
> > > > 
> > > > Alright, I think now you got it and we are on the same page. If
> > > > I
> > > > understood your suggestion, users would just use GPIO_PULL_UP
> > > > in
> > > > dtb if
> > > > wanting the default behavior. I would then use the gpiochip
> > > > 'request()'
> > > > callback to test the for pull-up flag right?
> > > 
> > > Something like this, yes.
> > > 
> > > > If I'm getting this right, there's a problem with it...
> > > > gpiod_configure_flags() is called after gpiod_request() which
> > > > means
> > > > that the gpiod descriptor won't still have the BIAS flags set.
> > > > And
> > > > I
> > > > don't think there's a way (at least clean and easy) to get the
> > > > firmware
> > > > lookup flags from the request callback?
> > > > 
> > > > So, honestly the only option I see to do it without changing
> > > > gpioblib
> > > > would be to hook this change in output/input callbacks which is
> > > > far
> > > > from being optimal...
> > > > 
> > > > So, in the end having this explicitly like this feels the best
> > > > option
> > > > to me. Sure, I can find some workaround in my driver but that
> > > > does
> > > > not
> > > > change this...
> > > 
> > > Ok, let me think about it. Meanwhile, maybe others have better
> > > ideas
> > > already?
> > > 
> > 
> > Sure, I'm still thinking that having this extra property and
> > explicitly
> > set it from OF is not that bad :)
> > 
> > > > "
> > > > git grep "PIN_CONFIG_BIAS_DISABLE" drivers/gpio/
> > > 
> > > Hint: `git grep -lw "PIN_CONFIG_BIAS_DISABLE" -- drivers/gpio`
> > > 
> > 
> > nice..
> > 
> > > > drivers/gpio/gpio-aspeed.c:963: else if (param ==
> > > > PIN_CONFIG_BIAS_DISABLE ||
> > > > drivers/gpio/gpio-merrifield.c:197:     if
> > > > ((pinconf_to_config_param(config) == PIN_CONFIG_BIAS_DISABLE)
> > > > ||
> > > > drivers/gpio/gpio-omap.c:903:   case PIN_CONFIG_BIAS_DISABLE:
> > > > drivers/gpio/gpio-pca953x.c:573:        if (config ==
> > > > PIN_CONFIG_BIAS_DISABLE)
> > > > drivers/gpio/gpio-pca953x.c:592:        case
> > > > PIN_CONFIG_BIAS_DISABLE:
> > > > "
> > > > 
> > > > AFAICT, the only way this path is possible for these drivers is
> > > > through
> > > > gpiolib cdev which might not be what the authors of the drivers
> > > > were
> > > > expecting...
> > > 
> > > gpio-merrifield is bad example, it has a pin control.
> > > gpio-pca953x as I said should effectively be a pin control
> > > driver.
> > > 
> > > For the two left it might be the case.
> > > 
> > 
> > Well the thing is that even if we have pinctrl like for example,
> > gpio-omap, it is still true that there's no way to get into
> > 'omap_gpio_set_config()' for 'PIN_CONFIG_BIAS_DISABLE' and call
> > 'gpiochip_generic_config()'.
> > 
> > (naturally in this case, one can directly use pinctrl properties to
> > control the pin but still...)
> > 
> > 
> > - Nuno Sá
> > 
> 
> Ideologically I don't have anything against adding this flag (except
> that it should be called BIAS_DISABLE not PULL_DISABLE IMO). Nuno is

It makes sense, yes.

> right in that the character device is the only way to set this mode
> ATM and. However I would like to see the first user added together
> with the series because adding features nobody uses in the mainline
> kernel tree is generally frowned upon and it's also not clear that
> anyone actually wants to use it.

Hmm, you mean something like a system's devicetree needing this flag?
If so, I don't really have such a thing. I did all my testing on a rpi
using overlays.

- Nuno Sá  

Re: [PATCH 0/4] add support for bias pull-disable

[Linus Walleij]

On Fri, Jul 15, 2022 at 9:31 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> On Fri, Jul 15, 2022 at 2:19 PM Nuno Sá <noname.nuno@gmail.com> wrote:

> Ideologically I don't have anything against adding this flag (except

Ugh that sounds political :)

> that it should be called BIAS_DISABLE not PULL_DISABLE IMO). Nuno is
> right in that the character device is the only way to set this mode
> ATM and. However I would like to see the first user added together
> with the series because adding features nobody uses in the mainline
> kernel tree is generally frowned upon and it's also not clear that
> anyone actually wants to use it.

I agree: makers and tinkerers definitely need this flag and it will further
emplasize the transition to the character device and to the v2 character
device interface (which, by the way, is looking awesome, especially when
taken into the picture the libgpiod changes and the new language
bindings for libgpiod).

So we need to add this to become ever more appealing for one-offs
such as industrial control.

Yours,
Linus Walleij

Re: [PATCH 0/4] add support for bias pull-disable

[Linus Walleij]

On Mon, Jul 18, 2022 at 9:50 AM Nuno Sá <noname.nuno@gmail.com> wrote:

> > right in that the character device is the only way to set this mode
> > ATM and. However I would like to see the first user added together
> > with the series because adding features nobody uses in the mainline
> > kernel tree is generally frowned upon and it's also not clear that
> > anyone actually wants to use it.
>
> Hmm, you mean something like a system's devicetree needing this flag?
> If so, I don't really have such a thing. I did all my testing on a rpi
> using overlays.

I would assume a driver with a .set_config() responding to this flag?

I actually think some gpio drivers using pin control as back-end
such as
drivers/pinctrl/bcm/pinctrl-bcm2835.c
will do this out-of-the box after this patch but I may be wrong.

To me supporting this on the Rpi driver is a good enough demonstrator
of the usefulness.

Yours,
Linus Walleij

Re: [PATCH 2/4] gpiolib: of: support bias pull disable

[Linus Walleij]

On Wed, Jul 13, 2022 at 3:13 PM Nuno Sá <nuno.sa@analog.com> wrote:

> On top of looking at PULL_UP and PULL_DOWN flags, also look at
> PULL_DISABLE and set the appropriate GPIO flag. The GPIO core will then
> pass down this to controllers that support it.
>
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 3/4] gpiolib: acpi: support bias pull disable

[Linus Walleij]

On Wed, Jul 13, 2022 at 3:13 PM Nuno Sá <nuno.sa@analog.com> wrote:

> On top of looking at PULL_UP and PULL_DOWN flags, also look at
> PULL_DISABLE and set the appropriate GPIO flag. The GPIO core will then
> pass down this to controllers that support it.
>
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>

Do we have a semantic check that PULLDOWN and PULLUP
is not used in combination with NOPULL here?

(We should also be checking that PULLDOWN and PULLUP
are not used simultaneously but that is an unrelated thing.)

Yours,
Linus Walleij

Re: [PATCH 4/4] dt-bindings: gpio: add pull-disable flag

[Linus Walleij]

On Wed, Jul 13, 2022 at 3:13 PM Nuno Sá <nuno.sa@analog.com> wrote:

> This extends the flags that can be used in GPIO specifiers to indicate
> that no bias is intended in the pin.
>
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 1/4] gpiolib: add support for bias pull disable

[Linus Walleij]

On Wed, Jul 13, 2022 at 7:37 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Wed, Jul 13, 2022 at 03:14:18PM +0200, Nuno Sá wrote:
> > This change prepares the gpio core to look at firmware flags and set
> > 'FLAG_BIAS_DISABLE' if necessary. It works in similar way to
> > 'GPIO_PULL_DOWN' and 'GPIO_PULL_UP'.
>
> ...
>
> >       GPIO_PULL_UP                    = (1 << 4),
> >       GPIO_PULL_DOWN                  = (1 << 5),
> > +     GPIO_PULL_DISABLE               = (1 << 6),
>
> To me it seems superfluous. You have already two flags:
> PUp
> PDown
> When none is set --> Pdisable

What happens in the pin control case for some drivers at least is that
the machine
comes up with some pull up/downs enabled (by power-on default or from
the boot loader), and some systems need to explicitly disable these
pulls.

In these (device tree) cases they set bias-disable; in the device tree, and
the driver will actively disable any pull up/down.

OK this is maybe not the most elegant system engineering. But some of
those users are hobbyists and cannot affect what the ASIC or firmware
is doing, because vendors are not really listening.

Another semantic reason is that pins can also be set to bias-high-impedance;
which is what "some people" would assume is the default if you disable
both pull up and pull down. (Yeah ... semantics...)

Device tree also has bias-pull-pin-default; to make things more complicated.
This should *really* leave it at power-on default. Explicitly.

I think for Nuno's usecase (using a random pin from userspace) the state
of biasing cannot be assumed, the driver will not change bias to
disabled just because neither pull up or down is specified, so the driver
needs an explicit kick saying "disable any bias".

Yours,
Linus Walleij

Re: [PATCH 0/4] add support for bias pull-disable

[Nuno Sá]

On Mon, 2022-07-18 at 12:29 +0200, Linus Walleij wrote:
> On Mon, Jul 18, 2022 at 9:50 AM Nuno Sá <noname.nuno@gmail.com>
> wrote:
> 
> > > right in that the character device is the only way to set this
> > > mode
> > > ATM and. However I would like to see the first user added
> > > together
> > > with the series because adding features nobody uses in the
> > > mainline
> > > kernel tree is generally frowned upon and it's also not clear
> > > that
> > > anyone actually wants to use it.
> > 
> > Hmm, you mean something like a system's devicetree needing this
> > flag?
> > If so, I don't really have such a thing. I did all my testing on a
> > rpi
> > using overlays.
> 
> I would assume a driver with a .set_config() responding to this flag?
> 

Ahh if that is the case, then there are actually users of this flag
already. 

"
git grep -lw "PIN_CONFIG_BIAS_DISABLE" drivers/gpio
drivers/gpio/gpio-aspeed.c
drivers/gpio/gpio-merrifield.c
drivers/gpio/gpio-omap.c
drivers/gpio/gpio-pca953x.c
"

This is also one of my arguments why I think having this new flag is
better than me adding a workaround for the device I'm supporting. Also,
in the cover I have a link to a new user (which is an input keyboard
driver that I'm refactoring) of this flag in .set_config().

- Nuno Sá

Re: [PATCH 3/4] gpiolib: acpi: support bias pull disable

[Nuno Sá]

On Mon, 2022-07-18 at 12:32 +0200, Linus Walleij wrote:
> On Wed, Jul 13, 2022 at 3:13 PM Nuno Sá <nuno.sa@analog.com> wrote:
> 
> > On top of looking at PULL_UP and PULL_DOWN flags, also look at
> > PULL_DISABLE and set the appropriate GPIO flag. The GPIO core will
> > then
> > pass down this to controllers that support it.
> > 
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> 
> Do we have a semantic check that PULLDOWN and PULLUP
> is not used in combination with NOPULL here?
> 
> (We should also be checking that PULLDOWN and PULLUP
> are not used simultaneously but that is an unrelated thing.)

I did extended this check:

https://elixir.bootlin.com/linux/v5.19-rc7/source/drivers/gpio/gpiolib.c#L3948

on patch 1 to make sure that PULLDOWN and PULLUP are not used with
NOPULL. Is this what you have in mind or is it something else?

- Nuno Sá

Re: [PATCH 3/4] gpiolib: acpi: support bias pull disable

[Linus Walleij]

On Mon, Jul 18, 2022 at 12:48 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> On Mon, 2022-07-18 at 12:32 +0200, Linus Walleij wrote:
> > On Wed, Jul 13, 2022 at 3:13 PM Nuno Sá <nuno.sa@analog.com> wrote:
> >
> > > On top of looking at PULL_UP and PULL_DOWN flags, also look at
> > > PULL_DISABLE and set the appropriate GPIO flag. The GPIO core will
> > > then
> > > pass down this to controllers that support it.
> > >
> > > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> >
> > Do we have a semantic check that PULLDOWN and PULLUP
> > is not used in combination with NOPULL here?
> >
> > (We should also be checking that PULLDOWN and PULLUP
> > are not used simultaneously but that is an unrelated thing.)
>
> I did extended this check:
>
> https://elixir.bootlin.com/linux/v5.19-rc7/source/drivers/gpio/gpiolib.c#L3948
>
> on patch 1 to make sure that PULLDOWN and PULLUP are not used with
> NOPULL. Is this what you have in mind or is it something else?

Excellent, thanks! That is exactly what we need, sorry for not being
able to keep all floating patches in my head :D

Yours,
Linus Walleij

Re: [PATCH 3/4] gpiolib: acpi: support bias pull disable

[Andy Shevchenko]

On Wed, Jul 13, 2022 at 03:14:20PM +0200, Nuno Sá wrote:
> On top of looking at PULL_UP and PULL_DOWN flags, also look at
> PULL_DISABLE and set the appropriate GPIO flag. The GPIO core will then
> pass down this to controllers that support it.

In case this patch will be in v2,
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

P.S. I will be on vacation till mid-August.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/4] dt-bindings: gpio: add pull-disable flag

[Rob Herring]

On Wed, 13 Jul 2022 15:14:21 +0200, Nuno Sá wrote:
> This extends the flags that can be used in GPIO specifiers to indicate
> that no bias is intended in the pin.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> ---
>  include/dt-bindings/gpio/gpio.h | 3 +++
>  1 file changed, 3 insertions(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 0/4] add support for bias pull-disable

[Bartosz Golaszewski]

On Wed, Jul 13, 2022 at 3:13 PM Nuno Sá <nuno.sa@analog.com> wrote:
>
> The gpio core looks at 'FLAG_BIAS_DISABLE' in preparation of calling the
> gpiochip 'set_config()' hook. However, AFAICT, there's no way that this
> flag is set because there's no support for it in firwmare code. Moreover,
> in 'gpiod_configure_flags()', only pull-ups and pull-downs are being
> handled.
>
> On top of this, there are some users that are looking at
> 'PIN_CONFIG_BIAS_DISABLE' in the 'set_config()' hook. So, unless I'm
> missing something, it looks like this was never working for these chips.
>
> Note that the ACPI case is only compiled tested. At first glance, it seems
> the current patch is enough but i'm not really sure...
>
> As a side note, this came to my attention during this patchset [1]
> (and, ofr OF,  was tested with it).
>
> [1]: https://lore.kernel.org/linux-input/20220708093448.42617-5-nuno.sa@analog.com/
>
> Nuno Sá (4):
>   gpiolib: add support for bias pull disable
>   gpiolib: of: support bias pull disable
>   gpiolib: acpi: support bias pull disable
>   dt-bindings: gpio: add pull-disable flag
>
>  drivers/gpio/gpiolib-acpi.c     | 3 +++
>  drivers/gpio/gpiolib-of.c       | 7 +++++++
>  drivers/gpio/gpiolib.c          | 8 ++++++--
>  include/dt-bindings/gpio/gpio.h | 3 +++
>  include/linux/gpio/machine.h    | 1 +
>  include/linux/of_gpio.h         | 1 +
>  6 files changed, 21 insertions(+), 2 deletions(-)
>
> --
> 2.37.0
>

Series applied, thanks!

Bart

Re: [PATCH 0/4] add support for bias pull-disable

[Nuno Sá]

On Tue, 2022-07-19 at 10:25 +0200, Bartosz Golaszewski wrote:
> On Wed, Jul 13, 2022 at 3:13 PM Nuno Sá <nuno.sa@analog.com> wrote:
> > 
> > The gpio core looks at 'FLAG_BIAS_DISABLE' in preparation of
> > calling the
> > gpiochip 'set_config()' hook. However, AFAICT, there's no way that
> > this
> > flag is set because there's no support for it in firwmare code.
> > Moreover,
> > in 'gpiod_configure_flags()', only pull-ups and pull-downs are
> > being
> > handled.
> > 
> > On top of this, there are some users that are looking at
> > 'PIN_CONFIG_BIAS_DISABLE' in the 'set_config()' hook. So, unless
> > I'm
> > missing something, it looks like this was never working for these
> > chips.
> > 
> > Note that the ACPI case is only compiled tested. At first glance,
> > it seems
> > the current patch is enough but i'm not really sure...
> > 
> > As a side note, this came to my attention during this patchset [1]
> > (and, ofr OF,  was tested with it).
> > 
> > [1]:
> > https://lore.kernel.org/linux-input/20220708093448.42617-5-nuno.sa@analog.com/
> > 
> > Nuno Sá (4):
> >   gpiolib: add support for bias pull disable
> >   gpiolib: of: support bias pull disable
> >   gpiolib: acpi: support bias pull disable
> >   dt-bindings: gpio: add pull-disable flag
> > 
> >  drivers/gpio/gpiolib-acpi.c     | 3 +++
> >  drivers/gpio/gpiolib-of.c       | 7 +++++++
> >  drivers/gpio/gpiolib.c          | 8 ++++++--
> >  include/dt-bindings/gpio/gpio.h | 3 +++
> >  include/linux/gpio/machine.h    | 1 +
> >  include/linux/of_gpio.h         | 1 +
> >  6 files changed, 21 insertions(+), 2 deletions(-)
> > 
> > --
> > 2.37.0
> > 
> 
> Series applied, thanks!

Hi Bart, 

I was actually planning to spin a v2 with your suggestion for the
naming of the new define... Did you changed it while applying or should
I still send it? Or (last option), we just leave it like this :)?

- Nuno Sá

Re: [PATCH 0/4] add support for bias pull-disable

[Bartosz Golaszewski]

On Tue, Jul 19, 2022 at 10:51 AM Nuno Sá <noname.nuno@gmail.com> wrote:
>
> On Tue, 2022-07-19 at 10:25 +0200, Bartosz Golaszewski wrote:
> > On Wed, Jul 13, 2022 at 3:13 PM Nuno Sá <nuno.sa@analog.com> wrote:
> > >
> > > The gpio core looks at 'FLAG_BIAS_DISABLE' in preparation of
> > > calling the
> > > gpiochip 'set_config()' hook. However, AFAICT, there's no way that
> > > this
> > > flag is set because there's no support for it in firwmare code.
> > > Moreover,
> > > in 'gpiod_configure_flags()', only pull-ups and pull-downs are
> > > being
> > > handled.
> > >
> > > On top of this, there are some users that are looking at
> > > 'PIN_CONFIG_BIAS_DISABLE' in the 'set_config()' hook. So, unless
> > > I'm
> > > missing something, it looks like this was never working for these
> > > chips.
> > >
> > > Note that the ACPI case is only compiled tested. At first glance,
> > > it seems
> > > the current patch is enough but i'm not really sure...
> > >
> > > As a side note, this came to my attention during this patchset [1]
> > > (and, ofr OF,  was tested with it).
> > >
> > > [1]:
> > > https://lore.kernel.org/linux-input/20220708093448.42617-5-nuno.sa@analog.com/
> > >
> > > Nuno Sá (4):
> > >   gpiolib: add support for bias pull disable
> > >   gpiolib: of: support bias pull disable
> > >   gpiolib: acpi: support bias pull disable
> > >   dt-bindings: gpio: add pull-disable flag
> > >
> > >  drivers/gpio/gpiolib-acpi.c     | 3 +++
> > >  drivers/gpio/gpiolib-of.c       | 7 +++++++
> > >  drivers/gpio/gpiolib.c          | 8 ++++++--
> > >  include/dt-bindings/gpio/gpio.h | 3 +++
> > >  include/linux/gpio/machine.h    | 1 +
> > >  include/linux/of_gpio.h         | 1 +
> > >  6 files changed, 21 insertions(+), 2 deletions(-)
> > >
> > > --
> > > 2.37.0
> > >
> >
> > Series applied, thanks!
>
> Hi Bart,
>
> I was actually planning to spin a v2 with your suggestion for the
> naming of the new define... Did you changed it while applying or should
> I still send it? Or (last option), we just leave it like this :)?
>
> - Nuno Sá

Yeah, I'm alright with it how it is after a second though: uAPI uses
the BIAS_PULL_UP/DOWN/DISABLE notation while the in-kernel API uses
the same scheme but without the BIAS prefix. Unless you want to change
something else - let's keep it as you first submitted it.

Bart

Re: [PATCH 0/4] add support for bias pull-disable

[Nuno Sá]

On Tue, 2022-07-19 at 11:14 +0200, Bartosz Golaszewski wrote:
> On Tue, Jul 19, 2022 at 10:51 AM Nuno Sá <noname.nuno@gmail.com>
> wrote:
> > 
> > On Tue, 2022-07-19 at 10:25 +0200, Bartosz Golaszewski wrote:
> > > On Wed, Jul 13, 2022 at 3:13 PM Nuno Sá <nuno.sa@analog.com>
> > > wrote:
> > > > 
> > > > The gpio core looks at 'FLAG_BIAS_DISABLE' in preparation of
> > > > calling the
> > > > gpiochip 'set_config()' hook. However, AFAICT, there's no way
> > > > that
> > > > this
> > > > flag is set because there's no support for it in firwmare code.
> > > > Moreover,
> > > > in 'gpiod_configure_flags()', only pull-ups and pull-downs are
> > > > being
> > > > handled.
> > > > 
> > > > On top of this, there are some users that are looking at
> > > > 'PIN_CONFIG_BIAS_DISABLE' in the 'set_config()' hook. So,
> > > > unless
> > > > I'm
> > > > missing something, it looks like this was never working for
> > > > these
> > > > chips.
> > > > 
> > > > Note that the ACPI case is only compiled tested. At first
> > > > glance,
> > > > it seems
> > > > the current patch is enough but i'm not really sure...
> > > > 
> > > > As a side note, this came to my attention during this patchset
> > > > [1]
> > > > (and, ofr OF,  was tested with it).
> > > > 
> > > > [1]:
> > > > https://lore.kernel.org/linux-input/20220708093448.42617-5-nuno.sa@analog.com/
> > > > 
> > > > Nuno Sá (4):
> > > >   gpiolib: add support for bias pull disable
> > > >   gpiolib: of: support bias pull disable
> > > >   gpiolib: acpi: support bias pull disable
> > > >   dt-bindings: gpio: add pull-disable flag
> > > > 
> > > >  drivers/gpio/gpiolib-acpi.c     | 3 +++
> > > >  drivers/gpio/gpiolib-of.c       | 7 +++++++
> > > >  drivers/gpio/gpiolib.c          | 8 ++++++--
> > > >  include/dt-bindings/gpio/gpio.h | 3 +++
> > > >  include/linux/gpio/machine.h    | 1 +
> > > >  include/linux/of_gpio.h         | 1 +
> > > >  6 files changed, 21 insertions(+), 2 deletions(-)
> > > > 
> > > > --
> > > > 2.37.0
> > > > 
> > > 
> > > Series applied, thanks!
> > 
> > Hi Bart,
> > 
> > I was actually planning to spin a v2 with your suggestion for the
> > naming of the new define... Did you changed it while applying or
> > should
> > I still send it? Or (last option), we just leave it like this :)?
> > 
> > - Nuno Sá
> 
> Yeah, I'm alright with it how it is after a second though: uAPI uses
> the BIAS_PULL_UP/DOWN/DISABLE notation while the in-kernel API uses
> the same scheme but without the BIAS prefix. Unless you want to
> change
> something else - let's keep it as you first submitted it.
> 

Alright, works for me...

- Nuno Sá