[v2,3/5] gpio: use new gpio_set_config() helper in more places

Message ID 20190207162859.26252-4-thomas.petazzoni@bootlin.com
State New
Headers show
Series
  • gpio: add support for pull-up/pull-down configuration
Related show

Commit Message

Thomas Petazzoni Feb. 7, 2019, 4:28 p.m.
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(-)

Comments

Guenter Roeck March 16, 2019, 1:43 a.m. | #1
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. | #2
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 16, 2019, 2:20 p.m. | #3
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.
> 

I don't know the code well enough to make a recommendation one way or another
(meaning I am not entirely sure if the aspeed code generating the traceback code
is correct). However, the attached does fix the problem for me, suggesting that
gpio_set_config() might be missing an argument.

Guenter

---

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 144af0733581..e972836e8d8f 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2563,9 +2563,9 @@ EXPORT_SYMBOL_GPL(gpiochip_free_own_desc);
  */
 
 static int gpio_set_config(struct gpio_chip *gc, unsigned offset,
-			   enum pin_config_param mode)
+			   enum pin_config_param mode, u32 argument)
 {
-	unsigned long config = { PIN_CONF_PACKED(mode, 0) };
+	unsigned long config = { PIN_CONF_PACKED(mode, argument) };
 
 	return gc->set_config ? gc->set_config(gc, offset, config) : -ENOTSUPP;
 }
@@ -2619,10 +2619,10 @@ int gpiod_direction_input(struct gpio_desc *desc)
 
 	if (test_bit(FLAG_PULL_UP, &desc->flags))
 		gpio_set_config(chip, gpio_chip_hwgpio(desc),
-				PIN_CONFIG_BIAS_PULL_UP);
+				PIN_CONFIG_BIAS_PULL_UP, 0);
 	else if (test_bit(FLAG_PULL_DOWN, &desc->flags))
 		gpio_set_config(chip, gpio_chip_hwgpio(desc),
-				PIN_CONFIG_BIAS_PULL_DOWN);
+				PIN_CONFIG_BIAS_PULL_DOWN, 0);
 
 	trace_gpio_direction(desc_to_gpio(desc), 1, status);
 
@@ -2727,7 +2727,7 @@ int gpiod_direction_output(struct gpio_desc *desc, int value)
 	if (test_bit(FLAG_OPEN_DRAIN, &desc->flags)) {
 		/* First see if we can enable open drain in hardware */
 		ret = gpio_set_config(gc, gpio_chip_hwgpio(desc),
-				      PIN_CONFIG_DRIVE_OPEN_DRAIN);
+				      PIN_CONFIG_DRIVE_OPEN_DRAIN, 0);
 		if (!ret)
 			goto set_output_value;
 		/* Emulate open drain by not actively driving the line high */
@@ -2736,7 +2736,7 @@ int gpiod_direction_output(struct gpio_desc *desc, int value)
 	}
 	else if (test_bit(FLAG_OPEN_SOURCE, &desc->flags)) {
 		ret = gpio_set_config(gc, gpio_chip_hwgpio(desc),
-				      PIN_CONFIG_DRIVE_OPEN_SOURCE);
+				      PIN_CONFIG_DRIVE_OPEN_SOURCE, 0);
 		if (!ret)
 			goto set_output_value;
 		/* Emulate open source by not actively driving the line low */
@@ -2744,7 +2744,7 @@ int gpiod_direction_output(struct gpio_desc *desc, int value)
 			return gpiod_direction_input(desc);
 	} else {
 		gpio_set_config(gc, gpio_chip_hwgpio(desc),
-				PIN_CONFIG_DRIVE_PUSH_PULL);
+				PIN_CONFIG_DRIVE_PUSH_PULL, 0);
 	}
 
 set_output_value:
@@ -2764,19 +2764,11 @@ EXPORT_SYMBOL_GPL(gpiod_direction_output);
 int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
 {
 	struct gpio_chip	*chip;
-	unsigned long		config;
 
 	VALIDATE_DESC(desc);
 	chip = desc->gdev->chip;
-	if (!chip->set || !chip->set_config) {
-		gpiod_dbg(desc,
-			  "%s: missing set() or set_config() operations\n",
-			  __func__);
-		return -ENOTSUPP;
-	}
-
-	config = pinconf_to_config_packed(PIN_CONFIG_INPUT_DEBOUNCE, debounce);
-	return gpio_set_config(chip, gpio_chip_hwgpio(desc), config);
+	return gpio_set_config(chip, gpio_chip_hwgpio(desc),
+			       PIN_CONFIG_INPUT_DEBOUNCE, debounce);
 }
 EXPORT_SYMBOL_GPL(gpiod_set_debounce);
 
@@ -2791,7 +2783,6 @@ EXPORT_SYMBOL_GPL(gpiod_set_debounce);
 int gpiod_set_transitory(struct gpio_desc *desc, bool transitory)
 {
 	struct gpio_chip *chip;
-	unsigned long packed;
 	int gpio;
 	int rc;
 
@@ -2807,13 +2798,8 @@ int gpiod_set_transitory(struct gpio_desc *desc, bool transitory)
 
 	/* If the driver supports it, set the persistence state now */
 	chip = desc->gdev->chip;
-	if (!chip->set_config)
-		return 0;
-
-	packed = pinconf_to_config_packed(PIN_CONFIG_PERSIST_STATE,
-					  !transitory);
 	gpio = gpio_chip_hwgpio(desc);
-	rc = gpio_set_config(chip, gpio, packed);
+	rc = gpio_set_config(chip, gpio, PIN_CONFIG_PERSIST_STATE, !transitory);
 	if (rc == -ENOTSUPP) {
 		dev_dbg(&desc->gdev->dev, "Persistence not supported for GPIO %d\n",
 				gpio);
Guenter Roeck March 26, 2019, 8:03 p.m. | #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. | #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

Patch

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);
 }
 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);
 	if (rc == -ENOTSUPP) {
 		dev_dbg(&desc->gdev->dev, "Persistence not supported for GPIO %d\n",
 				gpio);