mbox series

[v2,0/5] gpio: add support for pull-up/pull-down configuration

Message ID 20190207162859.26252-1-thomas.petazzoni@bootlin.com
Headers show
Series gpio: add support for pull-up/pull-down configuration | expand

Message

Thomas Petazzoni Feb. 7, 2019, 4:28 p.m. UTC 
Hello,

As we started discussing in [1], it would be useful to have a way to
configure pull-up/pull-down resistors for simple GPIO controllers that
don't have any pinmuxing capability, and therefore no interaction with
the pinctrl subsystem.

This is a second iteration of the patches (the v1 was posted at
https://patchwork.ozlabs.org/cover/1020392/).

Changes since v1:

 - Add a comment in the binding that explicitly states that the new
   bit 4 and 5 should only be used for HW with a simple on/off
   pull-up/pull-down configuration. More complex HW should use a pin
   control binding. Suggested by Linus Walleij.

 - Change gpiod_set_debounce() and gpiod_set_transitory() to use the
   new gpio_set_config() helper. This is done in a new, separate
   patch. Suggested by Linus Walleij.

 - Add error checking in gpiod_configure_flags() to make sure pull-up
   and pull-down are not both enabled on the same GPIO. Suggested by
   Jan Kundrát. Jan suggested to put this in the code converting from
   DT properties to internal flags, but it actually makes more sense
   to do it in gpiod_configure_flags(), independently from DT parsing.

 - Add a check in gpio-pca953x driver that pull-up/pull-down is indeed
   supported by the underlying HW in the
   pca953x_gpio_set_pull_up_down() function. Noticed by Linus Walleij.

 - Move the changes in include/dt-bindings/gpio/gpio.h to the
   dt-bindings patch, as suggested by Rob Herring.

Rob: due to the changes in the dt-bindings patch, I have not kept your
Acked-by.

Thomas

[1] https://marc.info/?l=linux-gpio&m=154491873506701&w=2

Thomas Petazzoni (5):
  dt-bindings: gpio: document the new pull-up/pull-down flags
  gpio: rename gpio_set_drive_single_ended() to gpio_set_config()
  gpio: use new gpio_set_config() helper in more places
  gpio: add core support for pull-up/pull-down configuration
  gpio: pca953x: add ->set_config implementation

 .../devicetree/bindings/gpio/gpio.txt         | 12 ++++
 drivers/gpio/gpio-pca953x.c                   | 66 ++++++++++++++++++-
 drivers/gpio/gpiolib-of.c                     |  5 ++
 drivers/gpio/gpiolib.c                        | 50 +++++++++-----
 drivers/gpio/gpiolib.h                        |  2 +
 include/dt-bindings/gpio/gpio.h               |  6 ++
 include/linux/gpio/machine.h                  |  2 +
 include/linux/of_gpio.h                       |  2 +
 8 files changed, 127 insertions(+), 18 deletions(-)

Comments

Linus Walleij Feb. 13, 2019, 8:15 a.m. UTC | #1 
On Thu, Feb 7, 2019 at 5:29 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:

> As we started discussing in [1], it would be useful to have a way to
> configure pull-up/pull-down resistors for simple GPIO controllers that
> don't have any pinmuxing capability, and therefore no interaction with
> the pinctrl subsystem.
>
> This is a second iteration of the patches (the v1 was posted at
> https://patchwork.ozlabs.org/cover/1020392/).

I don't see any problem with this patch set so I have merged it
into an immutable branch in my GPIO tree, and if it builds fine
I will merge it for devel (for-v5.1).

Notably we do not add a userspace ABI in this patch series.

I guess that is fine for now, but I think we might see userspace
support requests soon. But that can be a different patch
and should be driven by actual need for that.

Yours,
Linus Walleij
Guenter Roeck March 16, 2019, 1:43 a.m. UTC | #2 
On Thu, Feb 07, 2019 at 05:28:57PM +0100, Thomas Petazzoni wrote:
> As suggested by Linus Walleij, let's use the new gpio_set_config()
> helper in gpiod_set_debounce() and gpiod_set_transitory().
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> ---
>  drivers/gpio/gpiolib.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index cf8a4402fef1..9762a836fec9 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2762,7 +2762,7 @@ int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
>  	}
>  
>  	config = pinconf_to_config_packed(PIN_CONFIG_INPUT_DEBOUNCE, debounce);
> -	return chip->set_config(chip, gpio_chip_hwgpio(desc), config);
> +	return gpio_set_config(chip, gpio_chip_hwgpio(desc), config);

Are you sure this is correct ? This patch results in a number of tracebacks
in mainline. Reverting it fixes the problem.

gpio_set_config() seems to pack config, but it is already packed above.
That seems a bit suspicious.

>  }
>  EXPORT_SYMBOL_GPL(gpiod_set_debounce);
>  
> @@ -2799,7 +2799,7 @@ int gpiod_set_transitory(struct gpio_desc *desc, bool transitory)
>  	packed = pinconf_to_config_packed(PIN_CONFIG_PERSIST_STATE,
>  					  !transitory);
>  	gpio = gpio_chip_hwgpio(desc);
> -	rc = chip->set_config(chip, gpio, packed);
> +	rc = gpio_set_config(chip, gpio, packed);

Same here.

Guenter

---
Bisect log:

# bad: [f261c4e529dac5608a604d3dd3ae1cd2adf23c89] Merge branch 'akpm' (patches from Andrew)
# good: [1c163f4c7b3f621efff9b28a47abb36f7378d783] Linux 5.0
git bisect start 'HEAD' 'v5.0'
# good: [45763bf4bc1ebdf8eb95697607e1fd042a3e1221] Merge tag 'char-misc-5.1-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc
git bisect good 45763bf4bc1ebdf8eb95697607e1fd042a3e1221
# good: [cf2e8c544cd3b33e9e403b7b72404c221bf888d1] Merge tag 'mfd-next-5.1' of git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd
git bisect good cf2e8c544cd3b33e9e403b7b72404c221bf888d1
# bad: [d6075262969321bcb5d795de25595fc2a141ac02] Merge tag 'nios2-v5.1-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/lftan/nios2
git bisect bad d6075262969321bcb5d795de25595fc2a141ac02
# bad: [96a6de1a541c86e9e67b9c310c14db4099bd1cbc] Merge tag 'media/v5.1-1' of git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media
git bisect bad 96a6de1a541c86e9e67b9c310c14db4099bd1cbc
# bad: [d1cae94871330cb9f5fdcea34529abf7917e682e] Merge tag 'fscrypt-for-linus' of git://git.kernel.org/pub/scm/fs/fscrypt/fscrypt
git bisect bad d1cae94871330cb9f5fdcea34529abf7917e682e
# bad: [80201fe175cbf7f3e372f53eba0a881a702ad926] Merge tag 'for-5.1/block-20190302' of git://git.kernel.dk/linux-block
git bisect bad 80201fe175cbf7f3e372f53eba0a881a702ad926
# bad: [4221b807d1f73c03d22543416d303b60a5d1ef31] Merge tag 'for-5.1/libata-20190301' of git://git.kernel.dk/linux-block
git bisect bad 4221b807d1f73c03d22543416d303b60a5d1ef31
# bad: [8fab3d713ca36bf4ad4dadec0bf38f5e70b8999d] Merge tag 'gpio-v5.1-updates-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux into devel
git bisect bad 8fab3d713ca36bf4ad4dadec0bf38f5e70b8999d
# good: [b868db94a6a704755a33a362cfcf4625abdda115] gpio: tqmx86: Add GPIO from for this IO controller
git bisect good b868db94a6a704755a33a362cfcf4625abdda115
# good: [5340f23df8fe27a270af3fa1a93cd07293d23dd9] gpio: sprd: Add missing break in switch statement
git bisect good 5340f23df8fe27a270af3fa1a93cd07293d23dd9
# bad: [7f2f787c10596f486644d730a0a23e78abe8cbe0] gpio: pcf857x: Simpify wake-up handling
git bisect bad 7f2f787c10596f486644d730a0a23e78abe8cbe0
# bad: [6581eaf0e890756e093e2f44916edb5e7e6558ca] gpio: use new gpio_set_config() helper in more places
git bisect bad 6581eaf0e890756e093e2f44916edb5e7e6558ca
# good: [71479789851bdd9d56cd9e82892b0a3bee0a4c2a] gpio: rename gpio_set_drive_single_ended() to gpio_set_config()
git bisect good 71479789851bdd9d56cd9e82892b0a3bee0a4c2a
# first bad commit: [6581eaf0e890756e093e2f44916edb5e7e6558ca] gpio: use new gpio_set_config() helper in more places

Sample traceback:

[   15.965307] ------------[ cut here ]------------
[   15.965655] WARNING: CPU: 0 PID: 1 at drivers/gpio/gpio-aspeed.c:834 unregister_allocated_timer+0x60/0x98
[   15.965884] No timer allocated to offset 125
[   15.966177] CPU: 0 PID: 1 Comm: swapper Not tainted 5.0.0-11515-g3b319ee220a8 #1
[   15.966360] Hardware name: Generic DT based system
[   15.966586] Backtrace: 
[   15.966815] [<80106f9c>] (dump_backtrace) from [<80107248>] (show_stack+0x20/0x24)
[   15.967112]  r7:00000009 r6:00000000 r5:80bed4a0 r4:878a1c74
[   15.967704] [<80107228>] (show_stack) from [<8093f830>] (dump_stack+0x20/0x28)
[   15.967925] [<8093f810>] (dump_stack) from [<801147c8>] (__warn+0xf0/0x118)
[   15.968133] [<801146d8>] (__warn) from [<80114844>] (warn_slowpath_fmt+0x54/0x74)
[   15.968370]  r9:00000001 r8:60000113 r7:879f5ed0 r6:879f5e20 r5:80bed4bc r4:80e0b028
[   15.968612] [<801147f4>] (warn_slowpath_fmt) from [<805a83ac>] (unregister_allocated_timer+0x60/0x98)
[   15.968854]  r3:0000007d r2:80bed4bc
[   15.968989]  r5:00000000 r4:0000007d
[   15.969141] [<805a834c>] (unregister_allocated_timer) from [<805a8538>] (aspeed_gpio_set_config+0x154/0x444)
[   15.969419] [<805a83e4>] (aspeed_gpio_set_config) from [<805a0e6c>] (gpiod_set_debounce+0x60/0xcc)
[   15.969696]  r10:8718e8a0 r9:00000001 r8:84346f4c r7:00000000 r6:879bc010 r5:871606a8
[   15.969893]  r4:87a987d0
[   15.970008] [<805a0e0c>] (gpiod_set_debounce) from [<807462c8>] (gpio_keys_probe+0x4d8/0x924)
[   15.970211]  r4:8718e8bc
[   15.970326] [<80745df0>] (gpio_keys_probe) from [<80633fec>] (platform_drv_probe+0x58/0xa8)
Thomas Petazzoni March 16, 2019, 1:45 p.m. UTC | #3 
Hello,

On Fri, 15 Mar 2019 18:43:52 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index cf8a4402fef1..9762a836fec9 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -2762,7 +2762,7 @@ int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
> >  	}
> >  
> >  	config = pinconf_to_config_packed(PIN_CONFIG_INPUT_DEBOUNCE, debounce);
> > -	return chip->set_config(chip, gpio_chip_hwgpio(desc), config);
> > +	return gpio_set_config(chip, gpio_chip_hwgpio(desc), config);  
> 
> Are you sure this is correct ? This patch results in a number of tracebacks
> in mainline. Reverting it fixes the problem.
> 
> gpio_set_config() seems to pack config, but it is already packed above.
> That seems a bit suspicious.

I'll have a look. In the mean time, I'm fine with the patch being
reverted.

Thanks,

Thomas
Guenter Roeck March 26, 2019, 8:03 p.m. UTC | #4 
On Sat, Mar 16, 2019 at 02:45:19PM +0100, Thomas Petazzoni wrote:
> Hello,
> 
> On Fri, 15 Mar 2019 18:43:52 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
> > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > index cf8a4402fef1..9762a836fec9 100644
> > > --- a/drivers/gpio/gpiolib.c
> > > +++ b/drivers/gpio/gpiolib.c
> > > @@ -2762,7 +2762,7 @@ int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
> > >  	}
> > >  
> > >  	config = pinconf_to_config_packed(PIN_CONFIG_INPUT_DEBOUNCE, debounce);
> > > -	return chip->set_config(chip, gpio_chip_hwgpio(desc), config);
> > > +	return gpio_set_config(chip, gpio_chip_hwgpio(desc), config);  
> > 
> > Are you sure this is correct ? This patch results in a number of tracebacks
> > in mainline. Reverting it fixes the problem.
> > 
> > gpio_set_config() seems to pack config, but it is already packed above.
> > That seems a bit suspicious.
> 
> I'll have a look. In the mean time, I'm fine with the patch being
> reverted.
> 

The problem is still seen in the latest kernel as of last night, and
I did not see any further activities. Should I send a revert request ?

Guenter
Thomas Petazzoni March 26, 2019, 9:04 p.m. UTC | #5 
Hello Guenter,

On Tue, 26 Mar 2019 13:03:02 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> > I'll have a look. In the mean time, I'm fine with the patch being
> > reverted.
> 
> The problem is still seen in the latest kernel as of last night, and
> I did not see any further activities. Should I send a revert request ?

Sorry for not replying. I have not had the chance to investigate this
further due to lack of time. Linus Walleij initially suggested to use
gpio_set_config() in more places, but that doesn't to work as expected.

A revert patch has already been sent:

  https://lore.kernel.org/lkml/20190326044954.18671-1-andrew@aj.id.au/

Best regards,

Thomas Petazzoni