diff mbox series

gpio: tegra: Fix offset of pinctrl calls

Message ID 20190219204843.20352-1-linus.walleij@linaro.org
State New
Headers show
Series gpio: tegra: Fix offset of pinctrl calls | expand

Commit Message

Linus Walleij Feb. 19, 2019, 8:48 p.m. UTC
This patch hunk is a lightly modified version of a diff found
in a Tegra code dump from a product tree. It makes a lot of
sense because this is what most drivers do.

Cc: Thierry Reding <treding@nvidia.com>
Cc: Stefan Agner <stefan@agner.ch>
Cc: Dmitry Osipenko <digetx@gmail.com>
Cc: Jon Hunter <jonathanh@nvidia.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpio-tegra.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

Comments

Dmitry Osipenko Feb. 19, 2019, 9:24 p.m. UTC | #1
19.02.2019 23:48, Linus Walleij пишет:
> This patch hunk is a lightly modified version of a diff found
> in a Tegra code dump from a product tree. It makes a lot of
> sense because this is what most drivers do.
> 
> Cc: Thierry Reding <treding@nvidia.com>
> Cc: Stefan Agner <stefan@agner.ch>
> Cc: Dmitry Osipenko <digetx@gmail.com>
> Cc: Jon Hunter <jonathanh@nvidia.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/gpio/gpio-tegra.c | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-tegra.c b/drivers/gpio/gpio-tegra.c
> index 02f6db925fd5..1ececf2c3282 100644
> --- a/drivers/gpio/gpio-tegra.c
> +++ b/drivers/gpio/gpio-tegra.c
> @@ -2,6 +2,7 @@
>   * arch/arm/mach-tegra/gpio.c
>   *
>   * Copyright (c) 2010 Google, Inc
> + * Copyright (c) 2011-2016, NVIDIA CORPORATION.  All rights reserved.
>   *
>   * Author:
>   *	Erik Gilling <konkers@google.com>
> @@ -141,14 +142,14 @@ static void tegra_gpio_disable(struct tegra_gpio_info *tgi, unsigned int gpio)
>  
>  static int tegra_gpio_request(struct gpio_chip *chip, unsigned int offset)
>  {
> -	return pinctrl_gpio_request(offset);
> +	return pinctrl_gpio_request(chip->base + offset);
>  }
>  
>  static void tegra_gpio_free(struct gpio_chip *chip, unsigned int offset)
>  {
>  	struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
>  
> -	pinctrl_gpio_free(offset);
> +	pinctrl_gpio_free(chip->base + offset);
>  	tegra_gpio_disable(tgi, offset);
>  }
>  
> @@ -176,10 +177,18 @@ static int tegra_gpio_direction_input(struct gpio_chip *chip,
>  				      unsigned int offset)
>  {
>  	struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
> +	int ret;
>  
>  	tegra_gpio_mask_write(tgi, GPIO_MSK_OE(tgi, offset), offset, 0);
>  	tegra_gpio_enable(tgi, offset);
> -	return 0;
> +
> +	ret = pinctrl_gpio_direction_input(chip->base + offset);
> +	if (ret < 0)
> +		dev_err(tgi->dev,
> +			"Failed to set pinctrl input direction of GPIO %d: %d",
> +			 chip->base + offset, ret);
> +
> +	return ret;
>  }
>  
>  static int tegra_gpio_direction_output(struct gpio_chip *chip,
> @@ -187,11 +196,19 @@ static int tegra_gpio_direction_output(struct gpio_chip *chip,
>  				       int value)
>  {
>  	struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
> +	int ret;
>  
>  	tegra_gpio_set(chip, offset, value);
>  	tegra_gpio_mask_write(tgi, GPIO_MSK_OE(tgi, offset), offset, 1);
>  	tegra_gpio_enable(tgi, offset);
> -	return 0;
> +
> +	ret = pinctrl_gpio_direction_output(chip->base + offset);
> +	if (ret < 0)
> +		dev_err(tgi->dev,
> +			"Failed to set pinctrl output direction of GPIO %d: %d",
> +			 chip->base + offset, ret);
> +
> +	return ret;
>  }
>  
>  static int tegra_gpio_get_direction(struct gpio_chip *chip,
> 

This is formally a correct patch, but none of Tegra's upstream pinctrl drivers implement gpio_set_direction() and chip->base=0. Hence I'm not sure whether this all is really necessary.. or maybe you're going to implement the gpio_set_direction()?
Linus Walleij Feb. 20, 2019, 9:50 p.m. UTC | #2
On Tue, Feb 19, 2019 at 10:24 PM Dmitry Osipenko <digetx@gmail.com> wrote:

> This is formally a correct patch, but none of Tegra's upstream pinctrl drivers implement
> gpio_set_direction() and chip->base=0. Hence I'm not sure whether this all is really
> necessary..

The API wants a valid global GPIO number, the current code is confusing since it
sends random numbers (offsets) instead.

> or maybe you're going to implement the gpio_set_direction()?

No but since nVidia has fixed it in their outoftree codebase and it is formally
correct I don't see why we shouldn't just fix it. It may confuse someone.

Yours,
Linus Walleij
Dmitry Osipenko Feb. 21, 2019, 12:02 a.m. UTC | #3
21.02.2019 0:50, Linus Walleij пишет:
> On Tue, Feb 19, 2019 at 10:24 PM Dmitry Osipenko <digetx@gmail.com> wrote:
> 
>> This is formally a correct patch, but none of Tegra's upstream pinctrl drivers implement
>> gpio_set_direction() and chip->base=0. Hence I'm not sure whether this all is really
>> necessary..
> 
> The API wants a valid global GPIO number, the current code is confusing since it
> sends random numbers (offsets) instead.
> 
>> or maybe you're going to implement the gpio_set_direction()?
> 
> No but since nVidia has fixed it in their outoftree codebase and it is formally
> correct I don't see why we shouldn't just fix it. It may confuse someone.

Well, downstream probably has a real use-case for a non-static PINCTRL-GPIO configuration. I don't really mind much if this makes easier for you to follow the code, maybe it will become really handy later on for upstream too.

Reviewed-by: Dmitry Osipenko <digetx@gmail.com
diff mbox series

Patch

diff --git a/drivers/gpio/gpio-tegra.c b/drivers/gpio/gpio-tegra.c
index 02f6db925fd5..1ececf2c3282 100644
--- a/drivers/gpio/gpio-tegra.c
+++ b/drivers/gpio/gpio-tegra.c
@@ -2,6 +2,7 @@ 
  * arch/arm/mach-tegra/gpio.c
  *
  * Copyright (c) 2010 Google, Inc
+ * Copyright (c) 2011-2016, NVIDIA CORPORATION.  All rights reserved.
  *
  * Author:
  *	Erik Gilling <konkers@google.com>
@@ -141,14 +142,14 @@  static void tegra_gpio_disable(struct tegra_gpio_info *tgi, unsigned int gpio)
 
 static int tegra_gpio_request(struct gpio_chip *chip, unsigned int offset)
 {
-	return pinctrl_gpio_request(offset);
+	return pinctrl_gpio_request(chip->base + offset);
 }
 
 static void tegra_gpio_free(struct gpio_chip *chip, unsigned int offset)
 {
 	struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
 
-	pinctrl_gpio_free(offset);
+	pinctrl_gpio_free(chip->base + offset);
 	tegra_gpio_disable(tgi, offset);
 }
 
@@ -176,10 +177,18 @@  static int tegra_gpio_direction_input(struct gpio_chip *chip,
 				      unsigned int offset)
 {
 	struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
+	int ret;
 
 	tegra_gpio_mask_write(tgi, GPIO_MSK_OE(tgi, offset), offset, 0);
 	tegra_gpio_enable(tgi, offset);
-	return 0;
+
+	ret = pinctrl_gpio_direction_input(chip->base + offset);
+	if (ret < 0)
+		dev_err(tgi->dev,
+			"Failed to set pinctrl input direction of GPIO %d: %d",
+			 chip->base + offset, ret);
+
+	return ret;
 }
 
 static int tegra_gpio_direction_output(struct gpio_chip *chip,
@@ -187,11 +196,19 @@  static int tegra_gpio_direction_output(struct gpio_chip *chip,
 				       int value)
 {
 	struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
+	int ret;
 
 	tegra_gpio_set(chip, offset, value);
 	tegra_gpio_mask_write(tgi, GPIO_MSK_OE(tgi, offset), offset, 1);
 	tegra_gpio_enable(tgi, offset);
-	return 0;
+
+	ret = pinctrl_gpio_direction_output(chip->base + offset);
+	if (ret < 0)
+		dev_err(tgi->dev,
+			"Failed to set pinctrl output direction of GPIO %d: %d",
+			 chip->base + offset, ret);
+
+	return ret;
 }
 
 static int tegra_gpio_get_direction(struct gpio_chip *chip,