[PATCH 0/1] gpio: twl4030: gpio LED regression fix for 3.13 rc

[PATCH 0/1] gpio: twl4030: gpio LED regression fix for 3.13 rc

[Roger Quadros]

Hi,

Commit 0b2aa8be that went into 3.13-rc2 introduced a regression
that causes failure in setting LED GPO direction to OUT.

This causes USB Host to fail probe on Beagleboard.

The following patch fixes this issue.

I've tried to higlight the issue in the original patch here
http://www.spinics.net/lists/linux-omap/msg100858.html

cheers,
-roger

Roger Quadros (1):
  gpio: twl4030: Fix regression for twl gpio LED output

 drivers/gpio/gpio-twl4030.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

-- 
1.8.3.2

[PATCH 1/1] gpio: twl4030: Fix regression for twl gpio LED output

[Roger Quadros]

Commit 0b2aa8be introduced a regression that causes failure
in setting LED GPO direction to OUT.

This causes USB host probe failures for Beagleboard C4.

[    2.075469] platform usb_phy_gen_xceiv.2: Driver usb_phy_gen_xceiv requests probe deferral
[    2.090454] hsusb2_vcc: Failed to request enable GPIO510: -22
[    2.100982] reg-fixed-voltage reg-fixed-voltage.0.auto: Failed to register regulator: -22
[    2.109954] reg-fixed-voltage: probe of reg-fixed-voltage.0.auto failed with error -22

direction_out/direction_in must return 0 if the operation succeeded.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/gpio/gpio-twl4030.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpio-twl4030.c b/drivers/gpio/gpio-twl4030.c
index b97d6a6..0999ed1 100644
--- a/drivers/gpio/gpio-twl4030.c
+++ b/drivers/gpio/gpio-twl4030.c
@@ -294,13 +294,13 @@ out:
 static int twl_direction_in(struct gpio_chip *chip, unsigned offset)
 {
 	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
-	int ret;
+	int ret = 0;
 
 	mutex_lock(&priv->mutex);
 	if (offset < TWL4030_GPIO_MAX)
-		ret = twl4030_set_gpio_direction(offset, 1);
+		twl4030_set_gpio_direction(offset, 1);
 	else
-		ret = -EINVAL;
+		ret = -EINVAL;	/* LED outputs can't be set as input */
 
 	if (!ret)
 		priv->direction &= ~BIT(offset);
@@ -354,18 +354,21 @@ static void twl_set(struct gpio_chip *chip, unsigned offset, int value)
 static int twl_direction_out(struct gpio_chip *chip, unsigned offset, int value)
 {
 	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
-	int ret = -EINVAL;
 
 	mutex_lock(&priv->mutex);
 	if (offset < TWL4030_GPIO_MAX)
-		ret = twl4030_set_gpio_direction(offset, 0);
+		twl4030_set_gpio_direction(offset, 0);
+
+	/*
+	 *  LED gpio's i.e. offset >= TWL4030_GPIO_MAX are always output
+	 */
 
 	priv->direction |= BIT(offset);
 	mutex_unlock(&priv->mutex);
 
 	twl_set(chip, offset, value);
 
-	return ret;
+	return 0;
 }
 
 static int twl_to_irq(struct gpio_chip *chip, unsigned offset)
-- 
1.8.3.2

Re: [PATCH 1/1] gpio: twl4030: Fix regression for twl gpio LED output

[Tony Lindgren]

* Roger Quadros <rogerq@ti.com> [131204 03:35]:
> Commit 0b2aa8be introduced a regression that causes failure
> in setting LED GPO direction to OUT.
> 
> This causes USB host probe failures for Beagleboard C4.
> 
> [    2.075469] platform usb_phy_gen_xceiv.2: Driver usb_phy_gen_xceiv requests probe deferral
> [    2.090454] hsusb2_vcc: Failed to request enable GPIO510: -22
> [    2.100982] reg-fixed-voltage reg-fixed-voltage.0.auto: Failed to register regulator: -22
> [    2.109954] reg-fixed-voltage: probe of reg-fixed-voltage.0.auto failed with error -22
> 
> direction_out/direction_in must return 0 if the operation succeeded.

Uh, OK, sorry that was an unexpected side effect. We still should keep the
return values though instead of just ignoring them.
 
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-twl4030.c b/drivers/gpio/gpio-twl4030.c
> index b97d6a6..0999ed1 100644
> --- a/drivers/gpio/gpio-twl4030.c
> +++ b/drivers/gpio/gpio-twl4030.c
> @@ -294,13 +294,13 @@ out:
>  static int twl_direction_in(struct gpio_chip *chip, unsigned offset)
>  {
>  	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
> -	int ret;
> +	int ret = 0;
>  
>  	mutex_lock(&priv->mutex);
>  	if (offset < TWL4030_GPIO_MAX)
> -		ret = twl4030_set_gpio_direction(offset, 1);
> +		twl4030_set_gpio_direction(offset, 1);
>  	else
> -		ret = -EINVAL;
> +		ret = -EINVAL;	/* LED outputs can't be set as input */
>  
>  	if (!ret)
>  		priv->direction &= ~BIT(offset);

This looks OK to me.

> @@ -354,18 +354,21 @@ static void twl_set(struct gpio_chip *chip, unsigned offset, int value)
>  static int twl_direction_out(struct gpio_chip *chip, unsigned offset, int value)
>  {
>  	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
> -	int ret = -EINVAL;
>  

Can't you just have:

	int ret = 0;

>  	mutex_lock(&priv->mutex);
>  	if (offset < TWL4030_GPIO_MAX)
> -		ret = twl4030_set_gpio_direction(offset, 0);
> +		twl4030_set_gpio_direction(offset, 0);
> +
> +	/*
> +	 *  LED gpio's i.e. offset >= TWL4030_GPIO_MAX are always output
> +	 */
>  
>  	priv->direction |= BIT(offset);
>  	mutex_unlock(&priv->mutex);
>  
>  	twl_set(chip, offset, value);
>  
> -	return ret;
> +	return 0;
>  }

Then the rest of this change is not needed and we check the return value for
twl4030_set_gpio_direction. Makes sense to keep the comment there though.

Regards,

Tony

Re: [PATCH 1/1] gpio: twl4030: Fix regression for twl gpio LED output

[Roger Quadros]

On 12/04/2013 07:37 PM, Tony Lindgren wrote:
> * Roger Quadros <rogerq@ti.com> [131204 03:35]:
>> Commit 0b2aa8be introduced a regression that causes failure
>> in setting LED GPO direction to OUT.
>>
>> This causes USB host probe failures for Beagleboard C4.
>>
>> [    2.075469] platform usb_phy_gen_xceiv.2: Driver usb_phy_gen_xceiv requests probe deferral
>> [    2.090454] hsusb2_vcc: Failed to request enable GPIO510: -22
>> [    2.100982] reg-fixed-voltage reg-fixed-voltage.0.auto: Failed to register regulator: -22
>> [    2.109954] reg-fixed-voltage: probe of reg-fixed-voltage.0.auto failed with error -22
>>
>> direction_out/direction_in must return 0 if the operation succeeded.
> 
> Uh, OK, sorry that was an unexpected side effect. We still should keep the
> return values though instead of just ignoring them.

OK.
>  
>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-twl4030.c b/drivers/gpio/gpio-twl4030.c
>> index b97d6a6..0999ed1 100644
>> --- a/drivers/gpio/gpio-twl4030.c
>> +++ b/drivers/gpio/gpio-twl4030.c
>> @@ -294,13 +294,13 @@ out:
>>  static int twl_direction_in(struct gpio_chip *chip, unsigned offset)
>>  {
>>  	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
>> -	int ret;
>> +	int ret = 0;
>>  
>>  	mutex_lock(&priv->mutex);
>>  	if (offset < TWL4030_GPIO_MAX)
>> -		ret = twl4030_set_gpio_direction(offset, 1);
>> +		twl4030_set_gpio_direction(offset, 1);
>>  	else
>> -		ret = -EINVAL;
>> +		ret = -EINVAL;	/* LED outputs can't be set as input */
>>  
>>  	if (!ret)
>>  		priv->direction &= ~BIT(offset);
> 
> This looks OK to me.
> 
>> @@ -354,18 +354,21 @@ static void twl_set(struct gpio_chip *chip, unsigned offset, int value)
>>  static int twl_direction_out(struct gpio_chip *chip, unsigned offset, int value)
>>  {
>>  	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
>> -	int ret = -EINVAL;
>>  
> 
> Can't you just have:
> 
> 	int ret = 0;

Yes, i'll add that.
> 
>>  	mutex_lock(&priv->mutex);
>>  	if (offset < TWL4030_GPIO_MAX)
>> -		ret = twl4030_set_gpio_direction(offset, 0);
>> +		twl4030_set_gpio_direction(offset, 0);
>> +
>> +	/*
>> +	 *  LED gpio's i.e. offset >= TWL4030_GPIO_MAX are always output
>> +	 */
>>  
>>  	priv->direction |= BIT(offset);
>>  	mutex_unlock(&priv->mutex);
>>  
>>  	twl_set(chip, offset, value);
>>  
>> -	return ret;
>> +	return 0;
>>  }
> 
> Then the rest of this change is not needed and we check the return value for
> twl4030_set_gpio_direction. Makes sense to keep the comment there though.

OK. Will post v2.

cheers,
-roger

[PATCH v2 1/1] gpio: twl4030: Fix regression for twl gpio LED output

[Roger Quadros]

Commit 0b2aa8be introduced a regression that causes failure
in setting LED GPO direction to OUT.

This causes USB host probe failures for Beagleboard C4.

[    2.075469] platform usb_phy_gen_xceiv.2: Driver usb_phy_gen_xceiv requests probe deferral
[    2.090454] hsusb2_vcc: Failed to request enable GPIO510: -22
[    2.100982] reg-fixed-voltage reg-fixed-voltage.0.auto: Failed to register regulator: -22
[    2.109954] reg-fixed-voltage: probe of reg-fixed-voltage.0.auto failed with error -22

direction_out/direction_in must return 0 if the operation succeeded.

Also, don't update direction flag and output data if twl4030_set_gpio_direction()
failed inside twl_direction_out();

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/gpio/gpio-twl4030.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-twl4030.c b/drivers/gpio/gpio-twl4030.c
index b97d6a6..f999689 100644
--- a/drivers/gpio/gpio-twl4030.c
+++ b/drivers/gpio/gpio-twl4030.c
@@ -300,7 +300,7 @@ static int twl_direction_in(struct gpio_chip *chip, unsigned offset)
 	if (offset < TWL4030_GPIO_MAX)
 		ret = twl4030_set_gpio_direction(offset, 1);
 	else
-		ret = -EINVAL;
+		ret = -EINVAL;	/* LED outputs can't be set as input */
 
 	if (!ret)
 		priv->direction &= ~BIT(offset);
@@ -354,11 +354,20 @@ static void twl_set(struct gpio_chip *chip, unsigned offset, int value)
 static int twl_direction_out(struct gpio_chip *chip, unsigned offset, int value)
 {
 	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
-	int ret = -EINVAL;
+	int ret = 0;
 
 	mutex_lock(&priv->mutex);
-	if (offset < TWL4030_GPIO_MAX)
+	if (offset < TWL4030_GPIO_MAX) {
 		ret = twl4030_set_gpio_direction(offset, 0);
+		if (ret) {
+			mutex_unlock(&priv->mutex);
+			return ret;
+		}
+	}
+
+	/*
+	 *  LED gpios i.e. offset >= TWL4030_GPIO_MAX are always output
+	 */
 
 	priv->direction |= BIT(offset);
 	mutex_unlock(&priv->mutex);
-- 
1.8.3.2

Re: [PATCH v2 1/1] gpio: twl4030: Fix regression for twl gpio LED output

[Tony Lindgren]

* Roger Quadros <rogerq@ti.com> [131205 01:24]:
> Commit 0b2aa8be introduced a regression that causes failure
> in setting LED GPO direction to OUT.
> 
> This causes USB host probe failures for Beagleboard C4.
> 
> [    2.075469] platform usb_phy_gen_xceiv.2: Driver usb_phy_gen_xceiv requests probe deferral
> [    2.090454] hsusb2_vcc: Failed to request enable GPIO510: -22
> [    2.100982] reg-fixed-voltage reg-fixed-voltage.0.auto: Failed to register regulator: -22
> [    2.109954] reg-fixed-voltage: probe of reg-fixed-voltage.0.auto failed with error -22
> 
> direction_out/direction_in must return 0 if the operation succeeded.
> 
> Also, don't update direction flag and output data if twl4030_set_gpio_direction()
> failed inside twl_direction_out();
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>

Acked-by: Tony Lindgren <tony@atomide.com>

Linus, commit 0b2aa8be is in mainline now, so feel free to pick this one
up if it looks OK to you. This should be also Cc stable as 0b2aa8be has
that.

Regards,

Tony

> ---
>  drivers/gpio/gpio-twl4030.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-twl4030.c b/drivers/gpio/gpio-twl4030.c
> index b97d6a6..f999689 100644
> --- a/drivers/gpio/gpio-twl4030.c
> +++ b/drivers/gpio/gpio-twl4030.c
> @@ -300,7 +300,7 @@ static int twl_direction_in(struct gpio_chip *chip, unsigned offset)
>  	if (offset < TWL4030_GPIO_MAX)
>  		ret = twl4030_set_gpio_direction(offset, 1);
>  	else
> -		ret = -EINVAL;
> +		ret = -EINVAL;	/* LED outputs can't be set as input */
>  
>  	if (!ret)
>  		priv->direction &= ~BIT(offset);
> @@ -354,11 +354,20 @@ static void twl_set(struct gpio_chip *chip, unsigned offset, int value)
>  static int twl_direction_out(struct gpio_chip *chip, unsigned offset, int value)
>  {
>  	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
> -	int ret = -EINVAL;
> +	int ret = 0;
>  
>  	mutex_lock(&priv->mutex);
> -	if (offset < TWL4030_GPIO_MAX)
> +	if (offset < TWL4030_GPIO_MAX) {
>  		ret = twl4030_set_gpio_direction(offset, 0);
> +		if (ret) {
> +			mutex_unlock(&priv->mutex);
> +			return ret;
> +		}
> +	}
> +
> +	/*
> +	 *  LED gpios i.e. offset >= TWL4030_GPIO_MAX are always output
> +	 */
>  
>  	priv->direction |= BIT(offset);
>  	mutex_unlock(&priv->mutex);
> -- 
> 1.8.3.2
> 

Re: [PATCH v2 1/1] gpio: twl4030: Fix regression for twl gpio LED output

[Linus Walleij]

On Thu, Dec 5, 2013 at 10:23 AM, Roger Quadros <rogerq@ti.com> wrote:

> Commit 0b2aa8be introduced a regression that causes failure
> in setting LED GPO direction to OUT.
>
> This causes USB host probe failures for Beagleboard C4.
>
> [    2.075469] platform usb_phy_gen_xceiv.2: Driver usb_phy_gen_xceiv requests probe deferral
> [    2.090454] hsusb2_vcc: Failed to request enable GPIO510: -22
> [    2.100982] reg-fixed-voltage reg-fixed-voltage.0.auto: Failed to register regulator: -22
> [    2.109954] reg-fixed-voltage: probe of reg-fixed-voltage.0.auto failed with error -22
>
> direction_out/direction_in must return 0 if the operation succeeded.
>
> Also, don't update direction flag and output data if twl4030_set_gpio_direction()
> failed inside twl_direction_out();
>
> Signed-off-by: Roger Quadros <rogerq@ti.com>

Patch applied for fixes with Tony's ACK.

Thanks!
Linus Walleij