[PATCH 00/10] Fix gpio-hogs and sync with Linux gpiolib

[PATCH 00/10] Fix gpio-hogs and sync with Linux gpiolib

[Marco Felsch]

Hi,

the purpose of this series is to fix the gpio-hogs mechanism since this
is broken since commit 3641d381e6 ("gpiolib: Add of_xlate support").

Patch1: Revert the above mentioned commit to make the gpio-hogs working
        again.

Patch2-10: Add the of_xlate support required for the upcoming sunxi
           pinctrl driver. The mechanism is now more in sync with kernel
           gpiolib. This allows to sync easier with the kernel gpiolib
           in case of new features or fixes.

I've tested this rework on a i.mx8mm-evk.

Regards,
  Marco

Marco Felsch (10):
  gpiolib: fix gpio-hog functionality
  gpiolib: simplify for loop break condition
  gpiolib: rename local gpio-line-names variable
  gpiolib: fix gpio name memory leak
  gpiolib: fix missing error check while query gpio-line-names
  gpiolib: refactor gpio-line-names parsing
  gpiolib: introduce of_gpiochip_add to bundle all of functions
  OF: gpio: snyc of_get_named_gpio_flags variable with kernel
  OF: gpio: fix device_node leakage
  gpiolib: add of_xlate support

 drivers/gpio/gpiolib.c | 205 ++++++++++++++++++++++++++++++-----------
 drivers/of/of_gpio.c   |  69 ++++++++++----
 include/gpio.h         |  29 +++++-
 3 files changed, 231 insertions(+), 72 deletions(-)

-- 
2.39.2

[PATCH 01/10] gpiolib: fix gpio-hog functionality

[Marco Felsch]

This reverts commit 3641d381e63321016e3bf09504852a6b2a2f879b.

Since the of_xlate support the gpio-hog support is broken because the
'gpio' property used to specify the gpio-hog pin does not contain any
phandle. Due to the fact that of_xlate was never implemented the easiest
way to fix the gpio-hog functionality is to revert the commit.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/gpio/gpiolib.c | 58 ++++++++++++++++++++----------------------
 drivers/of/of_gpio.c   |  5 ++--
 include/gpio.h         |  4 +--
 3 files changed, 30 insertions(+), 37 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 6ec63b1119..b4a3a4e550 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -444,27 +444,44 @@ static int of_hog_gpio(struct device_node *np, struct gpio_chip *chip,
 		       unsigned int idx)
 {
 	struct device_node *chip_np = chip->dev->of_node;
-	struct of_phandle_args gpiospec;
 	unsigned long flags = 0;
-	u32 gpio_flags;
+	u32 gpio_cells, gpio_num, gpio_flags;
 	int ret, gpio;
 	const char *name = NULL;
 
-	ret = of_parse_phandle_with_args(chip_np, "gpios", "#gpio-cells", idx,
-					&gpiospec);
+	ret = of_property_read_u32(chip_np, "#gpio-cells", &gpio_cells);
 	if (ret)
 		return ret;
 
-	gpio = gpio_of_xlate(chip->dev, &gpiospec, &gpio_flags);
+	/*
+	 * Support for GPIOs that don't have #gpio-cells set to 2 is
+	 * not implemented
+	 */
+	if (WARN_ON(gpio_cells != 2))
+		return -ENOTSUPP;
+
+	ret = of_property_read_u32_index(np, "gpios", idx * gpio_cells,
+					 &gpio_num);
+	if (ret)
+		return ret;
+
+	ret = of_property_read_u32_index(np, "gpios", idx * gpio_cells + 1,
+					 &gpio_flags);
+	if (ret)
+		return ret;
+
+	if (gpio_flags & OF_GPIO_ACTIVE_LOW)
+		flags |= GPIOF_ACTIVE_LOW;
+
+	gpio = gpio_get_num(chip->dev, gpio_num);
 	if (gpio == -EPROBE_DEFER)
 		return gpio;
+
 	if (gpio < 0) {
-		dev_err(chip->dev, "unable to get gpio: %d\n", gpio);
+		dev_err(chip->dev, "unable to get gpio %u\n", gpio_num);
 		return gpio;
 	}
 
-	if (gpio_flags & OF_GPIO_ACTIVE_LOW)
-		flags |= GPIOF_ACTIVE_LOW;
 
 	/*
 	 * Note that, in order to be compatible with Linux, the code
@@ -625,23 +642,6 @@ void gpiochip_remove(struct gpio_chip *chip)
 	list_del(&chip->list);
 }
 
-static int of_gpio_simple_xlate(struct gpio_chip *chip,
-				const struct of_phandle_args *gpiospec,
-				u32 *flags)
-{
-	/*
-	 * Support for GPIOs that don't have #gpio-cells set to 2 is
-	 * not implemented
-	 */
-	if (WARN_ON(gpiospec->args_count != 2))
-		return -ENOTSUPP;
-
-	if (flags)
-		*flags = gpiospec->args[1];
-
-	return chip->base + gpiospec->args[0];
-}
-
 struct gpio_chip *gpio_get_chip_by_dev(struct device *dev)
 {
 	struct gpio_chip *chip;
@@ -654,8 +654,7 @@ struct gpio_chip *gpio_get_chip_by_dev(struct device *dev)
 	return NULL;
 }
 
-int gpio_of_xlate(struct device *dev, struct of_phandle_args *gpiospec,
-		  int *flags)
+int gpio_get_num(struct device *dev, int gpio)
 {
 	struct gpio_chip *chip;
 
@@ -666,10 +665,7 @@ int gpio_of_xlate(struct device *dev, struct of_phandle_args *gpiospec,
 	if (!chip)
 		return -EPROBE_DEFER;
 
-	if (chip->ops->of_xlate)
-		return chip->ops->of_xlate(chip, gpiospec, flags);
-	else
-		return of_gpio_simple_xlate(chip, gpiospec, flags);
+	return chip->base + gpio;
 }
 
 struct gpio_chip *gpio_get_chip(int gpio)
diff --git a/drivers/of/of_gpio.c b/drivers/of/of_gpio.c
index b662563742..5da80e2493 100644
--- a/drivers/of/of_gpio.c
+++ b/drivers/of/of_gpio.c
@@ -62,7 +62,6 @@ int of_get_named_gpio_flags(struct device_node *np, const char *propname,
 {
 	struct of_phandle_args out_args;
 	struct device *dev;
-	int of_flags;
 	int ret;
 
 	ret = of_parse_phandle_with_args(np, propname, "#gpio-cells",
@@ -80,7 +79,7 @@ int of_get_named_gpio_flags(struct device_node *np, const char *propname,
 		return -EPROBE_DEFER;
 	}
 
-	ret = gpio_of_xlate(dev, &out_args, &of_flags);
+	ret = gpio_get_num(dev, out_args.args[0]);
 	if (ret == -EPROBE_DEFER)
 		return ret;
 	if (ret < 0) {
@@ -90,7 +89,7 @@ int of_get_named_gpio_flags(struct device_node *np, const char *propname,
 	}
 
 	if (flags) {
-		*flags = of_flags;
+		*flags = out_args.args[1];
 		of_gpio_flags_quirks(np, propname, flags, index);
 	}
 
diff --git a/include/gpio.h b/include/gpio.h
index 10ac7fd7c4..5f2c16584c 100644
--- a/include/gpio.h
+++ b/include/gpio.h
@@ -177,7 +177,6 @@ struct gpio_ops {
 	int (*get_direction)(struct gpio_chip *chip, unsigned offset);
 	int (*get)(struct gpio_chip *chip, unsigned offset);
 	void (*set)(struct gpio_chip *chip, unsigned offset, int value);
-	int (*of_xlate)(struct gpio_chip *chip, const struct of_phandle_args *gpiospec, u32 *flags);
 };
 
 struct gpio_chip {
@@ -194,8 +193,7 @@ struct gpio_chip {
 int gpiochip_add(struct gpio_chip *chip);
 void gpiochip_remove(struct gpio_chip *chip);
 
-int gpio_of_xlate(struct device *dev, struct of_phandle_args *gpiospec,
-		  int *flags);
+int gpio_get_num(struct device *dev, int gpio);
 struct gpio_chip *gpio_get_chip(int gpio);
 
 #endif /* __GPIO_H */
-- 
2.39.2

Re: [PATCH 01/10] gpiolib: fix gpio-hog functionality

[Ahmad Fatoum]

On 02.06.23 09:49, Marco Felsch wrote:
> This reverts commit 3641d381e63321016e3bf09504852a6b2a2f879b.
> 
> Since the of_xlate support the gpio-hog support is broken because the
> 'gpio' property used to specify the gpio-hog pin does not contain any
> phandle. Due to the fact that of_xlate was never implemented the easiest
> way to fix the gpio-hog functionality is to revert the commit.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

Sascha, can this be taken to master?

> ---
>  drivers/gpio/gpiolib.c | 58 ++++++++++++++++++++----------------------
>  drivers/of/of_gpio.c   |  5 ++--
>  include/gpio.h         |  4 +--
>  3 files changed, 30 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 6ec63b1119..b4a3a4e550 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -444,27 +444,44 @@ static int of_hog_gpio(struct device_node *np, struct gpio_chip *chip,
>  		       unsigned int idx)
>  {
>  	struct device_node *chip_np = chip->dev->of_node;
> -	struct of_phandle_args gpiospec;
>  	unsigned long flags = 0;
> -	u32 gpio_flags;
> +	u32 gpio_cells, gpio_num, gpio_flags;
>  	int ret, gpio;
>  	const char *name = NULL;
>  
> -	ret = of_parse_phandle_with_args(chip_np, "gpios", "#gpio-cells", idx,
> -					&gpiospec);
> +	ret = of_property_read_u32(chip_np, "#gpio-cells", &gpio_cells);
>  	if (ret)
>  		return ret;
>  
> -	gpio = gpio_of_xlate(chip->dev, &gpiospec, &gpio_flags);
> +	/*
> +	 * Support for GPIOs that don't have #gpio-cells set to 2 is
> +	 * not implemented
> +	 */
> +	if (WARN_ON(gpio_cells != 2))
> +		return -ENOTSUPP;
> +
> +	ret = of_property_read_u32_index(np, "gpios", idx * gpio_cells,
> +					 &gpio_num);
> +	if (ret)
> +		return ret;
> +
> +	ret = of_property_read_u32_index(np, "gpios", idx * gpio_cells + 1,
> +					 &gpio_flags);
> +	if (ret)
> +		return ret;
> +
> +	if (gpio_flags & OF_GPIO_ACTIVE_LOW)
> +		flags |= GPIOF_ACTIVE_LOW;
> +
> +	gpio = gpio_get_num(chip->dev, gpio_num);
>  	if (gpio == -EPROBE_DEFER)
>  		return gpio;
> +
>  	if (gpio < 0) {
> -		dev_err(chip->dev, "unable to get gpio: %d\n", gpio);
> +		dev_err(chip->dev, "unable to get gpio %u\n", gpio_num);
>  		return gpio;
>  	}
>  
> -	if (gpio_flags & OF_GPIO_ACTIVE_LOW)
> -		flags |= GPIOF_ACTIVE_LOW;
>  
>  	/*
>  	 * Note that, in order to be compatible with Linux, the code
> @@ -625,23 +642,6 @@ void gpiochip_remove(struct gpio_chip *chip)
>  	list_del(&chip->list);
>  }
>  
> -static int of_gpio_simple_xlate(struct gpio_chip *chip,
> -				const struct of_phandle_args *gpiospec,
> -				u32 *flags)
> -{
> -	/*
> -	 * Support for GPIOs that don't have #gpio-cells set to 2 is
> -	 * not implemented
> -	 */
> -	if (WARN_ON(gpiospec->args_count != 2))
> -		return -ENOTSUPP;
> -
> -	if (flags)
> -		*flags = gpiospec->args[1];
> -
> -	return chip->base + gpiospec->args[0];
> -}
> -
>  struct gpio_chip *gpio_get_chip_by_dev(struct device *dev)
>  {
>  	struct gpio_chip *chip;
> @@ -654,8 +654,7 @@ struct gpio_chip *gpio_get_chip_by_dev(struct device *dev)
>  	return NULL;
>  }
>  
> -int gpio_of_xlate(struct device *dev, struct of_phandle_args *gpiospec,
> -		  int *flags)
> +int gpio_get_num(struct device *dev, int gpio)
>  {
>  	struct gpio_chip *chip;
>  
> @@ -666,10 +665,7 @@ int gpio_of_xlate(struct device *dev, struct of_phandle_args *gpiospec,
>  	if (!chip)
>  		return -EPROBE_DEFER;
>  
> -	if (chip->ops->of_xlate)
> -		return chip->ops->of_xlate(chip, gpiospec, flags);
> -	else
> -		return of_gpio_simple_xlate(chip, gpiospec, flags);
> +	return chip->base + gpio;
>  }
>  
>  struct gpio_chip *gpio_get_chip(int gpio)
> diff --git a/drivers/of/of_gpio.c b/drivers/of/of_gpio.c
> index b662563742..5da80e2493 100644
> --- a/drivers/of/of_gpio.c
> +++ b/drivers/of/of_gpio.c
> @@ -62,7 +62,6 @@ int of_get_named_gpio_flags(struct device_node *np, const char *propname,
>  {
>  	struct of_phandle_args out_args;
>  	struct device *dev;
> -	int of_flags;
>  	int ret;
>  
>  	ret = of_parse_phandle_with_args(np, propname, "#gpio-cells",
> @@ -80,7 +79,7 @@ int of_get_named_gpio_flags(struct device_node *np, const char *propname,
>  		return -EPROBE_DEFER;
>  	}
>  
> -	ret = gpio_of_xlate(dev, &out_args, &of_flags);
> +	ret = gpio_get_num(dev, out_args.args[0]);
>  	if (ret == -EPROBE_DEFER)
>  		return ret;
>  	if (ret < 0) {
> @@ -90,7 +89,7 @@ int of_get_named_gpio_flags(struct device_node *np, const char *propname,
>  	}
>  
>  	if (flags) {
> -		*flags = of_flags;
> +		*flags = out_args.args[1];
>  		of_gpio_flags_quirks(np, propname, flags, index);
>  	}
>  
> diff --git a/include/gpio.h b/include/gpio.h
> index 10ac7fd7c4..5f2c16584c 100644
> --- a/include/gpio.h
> +++ b/include/gpio.h
> @@ -177,7 +177,6 @@ struct gpio_ops {
>  	int (*get_direction)(struct gpio_chip *chip, unsigned offset);
>  	int (*get)(struct gpio_chip *chip, unsigned offset);
>  	void (*set)(struct gpio_chip *chip, unsigned offset, int value);
> -	int (*of_xlate)(struct gpio_chip *chip, const struct of_phandle_args *gpiospec, u32 *flags);
>  };
>  
>  struct gpio_chip {
> @@ -194,8 +193,7 @@ struct gpio_chip {
>  int gpiochip_add(struct gpio_chip *chip);
>  void gpiochip_remove(struct gpio_chip *chip);
>  
> -int gpio_of_xlate(struct device *dev, struct of_phandle_args *gpiospec,
> -		  int *flags);
> +int gpio_get_num(struct device *dev, int gpio);
>  struct gpio_chip *gpio_get_chip(int gpio);
>  
>  #endif /* __GPIO_H */

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 02/10] gpiolib: simplify for loop break condition

[Marco Felsch]

No functional change just a simplification to align it with the Linux
kernel. Also add the kernel comment to make the reason clear.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/gpio/gpiolib.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index b4a3a4e550..0df43c9f8f 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -529,7 +529,15 @@ static int of_gpiochip_scan_hogs(struct gpio_chip *chip)
 		of_property_read_string_array(chip->dev->of_node,
 					      "gpio-line-names", arr, count);
 
-		for (i = 0; i < chip->ngpio && i < count; i++)
+		/*
+		 * Since property 'gpio-line-names' cannot contains gaps, we
+		 * have to be sure we only assign those pins that really exists
+		 * since chip->ngpio can be less.
+		 */
+		if (count > chip->ngpio)
+			count = chip->ngpio;
+
+		for (i = 0; i < count; i++)
 			gpio_desc[chip->base + i].name = xstrdup(arr[i]);
 
 		free(arr);
-- 
2.39.2

Re: [PATCH 02/10] gpiolib: simplify for loop break condition

[Ahmad Fatoum]

On 02.06.23 09:49, Marco Felsch wrote:
> No functional change just a simplification to align it with the Linux
> kernel. Also add the kernel comment to make the reason clear.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> ---
>  drivers/gpio/gpiolib.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index b4a3a4e550..0df43c9f8f 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -529,7 +529,15 @@ static int of_gpiochip_scan_hogs(struct gpio_chip *chip)
>  		of_property_read_string_array(chip->dev->of_node,
>  					      "gpio-line-names", arr, count);
>  
> -		for (i = 0; i < chip->ngpio && i < count; i++)
> +		/*
> +		 * Since property 'gpio-line-names' cannot contains gaps, we
> +		 * have to be sure we only assign those pins that really exists
> +		 * since chip->ngpio can be less.
> +		 */
> +		if (count > chip->ngpio)
> +			count = chip->ngpio;
> +
> +		for (i = 0; i < count; i++)
>  			gpio_desc[chip->base + i].name = xstrdup(arr[i]);
>  
>  		free(arr);

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 03/10] gpiolib: rename local gpio-line-names variable

[Marco Felsch]

This is just a cosmetic commit to align the code more with the Linux
kernel.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/gpio/gpiolib.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 0df43c9f8f..eb2bba042e 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -524,10 +524,10 @@ static int of_gpiochip_scan_hogs(struct gpio_chip *chip)
 					  "gpio-line-names");
 
 	if (count > 0) {
-		const char **arr = xzalloc(count * sizeof(char *));
+		const char **names = xzalloc(count * sizeof(char *));
 
 		of_property_read_string_array(chip->dev->of_node,
-					      "gpio-line-names", arr, count);
+					      "gpio-line-names", names, count);
 
 		/*
 		 * Since property 'gpio-line-names' cannot contains gaps, we
@@ -538,9 +538,9 @@ static int of_gpiochip_scan_hogs(struct gpio_chip *chip)
 			count = chip->ngpio;
 
 		for (i = 0; i < count; i++)
-			gpio_desc[chip->base + i].name = xstrdup(arr[i]);
+			gpio_desc[chip->base + i].name = xstrdup(names[i]);
 
-		free(arr);
+		free(names);
 	}
 
 	for_each_available_child_of_node(chip->dev->of_node, np) {
-- 
2.39.2

Re: [PATCH 03/10] gpiolib: rename local gpio-line-names variable

[Ahmad Fatoum]

On 02.06.23 09:49, Marco Felsch wrote:
> This is just a cosmetic commit to align the code more with the Linux
> kernel.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> ---
>  drivers/gpio/gpiolib.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 0df43c9f8f..eb2bba042e 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -524,10 +524,10 @@ static int of_gpiochip_scan_hogs(struct gpio_chip *chip)
>  					  "gpio-line-names");
>  
>  	if (count > 0) {
> -		const char **arr = xzalloc(count * sizeof(char *));
> +		const char **names = xzalloc(count * sizeof(char *));
>  
>  		of_property_read_string_array(chip->dev->of_node,
> -					      "gpio-line-names", arr, count);
> +					      "gpio-line-names", names, count);
>  
>  		/*
>  		 * Since property 'gpio-line-names' cannot contains gaps, we
> @@ -538,9 +538,9 @@ static int of_gpiochip_scan_hogs(struct gpio_chip *chip)
>  			count = chip->ngpio;
>  
>  		for (i = 0; i < count; i++)
> -			gpio_desc[chip->base + i].name = xstrdup(arr[i]);
> +			gpio_desc[chip->base + i].name = xstrdup(names[i]);
>  
> -		free(arr);
> +		free(names);
>  	}
>  
>  	for_each_available_child_of_node(chip->dev->of_node, np) {

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 04/10] gpiolib: fix gpio name memory leak

[Marco Felsch]

We never freed the name allocated by xstrdup(). Fix this by use the
const devicetree property value. While on it check if the name is valid
and add the comment to align the code with the kernel.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/gpio/gpiolib.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index eb2bba042e..f05e2ac735 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -18,7 +18,7 @@ struct gpio_info {
 	bool requested;
 	bool active_low;
 	char *label;
-	char *name;
+	const char *name;
 };
 
 static struct gpio_info *gpio_desc;
@@ -537,8 +537,16 @@ static int of_gpiochip_scan_hogs(struct gpio_chip *chip)
 		if (count > chip->ngpio)
 			count = chip->ngpio;
 
-		for (i = 0; i < count; i++)
-			gpio_desc[chip->base + i].name = xstrdup(names[i]);
+		for (i = 0; i < count; i++) {
+			/*
+			 * Allow overriding "fixed" names provided by the GPIO
+			 * provider. The "fixed" names are more often than not
+			 * generic and less informative than the names given in
+			 * device properties.
+			 */
+			if (names[i] && names[i][0])
+				gpio_desc[chip->base + i].name = names[i];
+		}
 
 		free(names);
 	}
-- 
2.39.2

Re: [PATCH 04/10] gpiolib: fix gpio name memory leak

[Ahmad Fatoum]

On 02.06.23 09:49, Marco Felsch wrote:
> We never freed the name allocated by xstrdup(). Fix this by use the
> const devicetree property value. While on it check if the name is valid
> and add the comment to align the code with the kernel.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> ---
>  drivers/gpio/gpiolib.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index eb2bba042e..f05e2ac735 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -18,7 +18,7 @@ struct gpio_info {
>  	bool requested;
>  	bool active_low;
>  	char *label;
> -	char *name;
> +	const char *name;
>  };
>  
>  static struct gpio_info *gpio_desc;
> @@ -537,8 +537,16 @@ static int of_gpiochip_scan_hogs(struct gpio_chip *chip)
>  		if (count > chip->ngpio)
>  			count = chip->ngpio;
>  
> -		for (i = 0; i < count; i++)
> -			gpio_desc[chip->base + i].name = xstrdup(names[i]);
> +		for (i = 0; i < count; i++) {
> +			/*
> +			 * Allow overriding "fixed" names provided by the GPIO
> +			 * provider. The "fixed" names are more often than not
> +			 * generic and less informative than the names given in
> +			 * device properties.
> +			 */
> +			if (names[i] && names[i][0])
> +				gpio_desc[chip->base + i].name = names[i];
> +		}
>  
>  		free(names);
>  	}

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 05/10] gpiolib: fix missing error check while query gpio-line-names

[Marco Felsch]

Don't ignore the possible error return code of
of_property_read_string_array().

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/gpio/gpiolib.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index f05e2ac735..166356c85a 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -526,8 +526,14 @@ static int of_gpiochip_scan_hogs(struct gpio_chip *chip)
 	if (count > 0) {
 		const char **names = xzalloc(count * sizeof(char *));
 
-		of_property_read_string_array(chip->dev->of_node,
-					      "gpio-line-names", names, count);
+		ret = of_property_read_string_array(chip->dev->of_node,
+						    "gpio-line-names", names,
+						    count);
+		if (ret < 0) {
+			dev_warn(chip->dev, "failed to read GPIO line names\n");
+			kfree(names);
+			return ret;
+		}
 
 		/*
 		 * Since property 'gpio-line-names' cannot contains gaps, we
-- 
2.39.2

Re: [PATCH 05/10] gpiolib: fix missing error check while query gpio-line-names

[Ahmad Fatoum]

On 02.06.23 09:49, Marco Felsch wrote:
> Don't ignore the possible error return code of
> of_property_read_string_array().
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

I know the kernel does it this way, but IMO this can't fail if
of_property_count_strings() called before it succeeded.

I'd suggest we drop this or if you feel strongly about this
drop just the never hit warning message.

> ---
>  drivers/gpio/gpiolib.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index f05e2ac735..166356c85a 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -526,8 +526,14 @@ static int of_gpiochip_scan_hogs(struct gpio_chip *chip)
>  	if (count > 0) {
>  		const char **names = xzalloc(count * sizeof(char *));
>  
> -		of_property_read_string_array(chip->dev->of_node,
> -					      "gpio-line-names", names, count);
> +		ret = of_property_read_string_array(chip->dev->of_node,
> +						    "gpio-line-names", names,
> +						    count);
> +		if (ret < 0) {
> +			dev_warn(chip->dev, "failed to read GPIO line names\n");
> +			kfree(names);
> +			return ret;
> +		}
>  
>  		/*
>  		 * Since property 'gpio-line-names' cannot contains gaps, we

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 06/10] gpiolib: refactor gpio-line-names parsing

[Marco Felsch]

Move the gpio-line-names parsing out of of_gpiochip_scan_hogs() since
this has nothing to do with gpio-hogs. The new function is very similar
with the kernel function devprop_gpiochip_set_names().

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/gpio/gpiolib.c | 104 ++++++++++++++++++++++++++---------------
 1 file changed, 66 insertions(+), 38 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 166356c85a..127cc60abd 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -515,48 +515,11 @@ static int of_hog_gpio(struct device_node *np, struct gpio_chip *chip,
 static int of_gpiochip_scan_hogs(struct gpio_chip *chip)
 {
 	struct device_node *np;
-	int ret, i, count;
+	int ret, i;
 
 	if (!IS_ENABLED(CONFIG_OFDEVICE) || !chip->dev->of_node)
 		return 0;
 
-	count = of_property_count_strings(chip->dev->of_node,
-					  "gpio-line-names");
-
-	if (count > 0) {
-		const char **names = xzalloc(count * sizeof(char *));
-
-		ret = of_property_read_string_array(chip->dev->of_node,
-						    "gpio-line-names", names,
-						    count);
-		if (ret < 0) {
-			dev_warn(chip->dev, "failed to read GPIO line names\n");
-			kfree(names);
-			return ret;
-		}
-
-		/*
-		 * Since property 'gpio-line-names' cannot contains gaps, we
-		 * have to be sure we only assign those pins that really exists
-		 * since chip->ngpio can be less.
-		 */
-		if (count > chip->ngpio)
-			count = chip->ngpio;
-
-		for (i = 0; i < count; i++) {
-			/*
-			 * Allow overriding "fixed" names provided by the GPIO
-			 * provider. The "fixed" names are more often than not
-			 * generic and less informative than the names given in
-			 * device properties.
-			 */
-			if (names[i] && names[i][0])
-				gpio_desc[chip->base + i].name = names[i];
-		}
-
-		free(names);
-	}
-
 	for_each_available_child_of_node(chip->dev->of_node, np) {
 		if (!of_property_read_bool(np, "gpio-hog"))
 			continue;
@@ -576,6 +539,66 @@ static int of_gpiochip_scan_hogs(struct gpio_chip *chip)
 	return 0;
 }
 
+/*
+ * of_gpiochip_set_names - Set GPIO line names using OF properties
+ * @chip: GPIO chip whose lines should be named, if possible
+ *
+ * Looks for device property "gpio-line-names" and if it exists assigns
+ * GPIO line names for the chip. The memory allocated for the assigned
+ * names belong to the underlying firmware node and should not be released
+ * by the caller.
+ */
+static int of_gpiochip_set_names(struct gpio_chip *chip)
+{
+	struct device *dev = chip->dev;
+	struct device_node *np;
+	const char **names;
+	int ret, i, count;
+
+	np = dev_of_node(dev);
+	if (!np)
+		return 0;
+
+	count = of_property_count_strings(np, "gpio-line-names");
+	if (count < 0)
+		return 0;
+
+	names = kcalloc(count, sizeof(*names), GFP_KERNEL);
+	if (!names)
+		return -ENOMEM;
+
+	ret = of_property_read_string_array(np, "gpio-line-names",
+					    names, count);
+	if (ret < 0) {
+		dev_warn(dev, "failed to read GPIO line names\n");
+		kfree(names);
+		return ret;
+	}
+
+	/*
+	 * Since property 'gpio-line-names' cannot contains gaps, we
+	 * have to be sure we only assign those pins that really exists
+	 * since chip->ngpio can be less.
+	 */
+	if (count > chip->ngpio)
+		count = chip->ngpio;
+
+	for (i = 0; i < count; i++) {
+		/*
+		 * Allow overriding "fixed" names provided by the GPIO
+		 * provider. The "fixed" names are more often than not
+		 * generic and less informative than the names given in
+		 * device properties.
+		 */
+		if (names[i] && names[i][0])
+			gpio_desc[chip->base + i].name = names[i];
+	}
+
+	free(names);
+
+	return 0;
+}
+
 #ifdef CONFIG_OFDEVICE
 static const char *gpio_suffixes[] = {
 	"gpios",
@@ -637,6 +660,7 @@ int dev_gpiod_get_index(struct device *dev,
 
 int gpiochip_add(struct gpio_chip *chip)
 {
+	int ret;
 	int i;
 
 	if (chip->base >= 0) {
@@ -656,6 +680,10 @@ int gpiochip_add(struct gpio_chip *chip)
 	for (i = chip->base; i < chip->base + chip->ngpio; i++)
 		gpio_desc[i].chip = chip;
 
+	ret = of_gpiochip_set_names(chip);
+	if (ret)
+		return ret;
+
 	return of_gpiochip_scan_hogs(chip);
 }
 
-- 
2.39.2

Re: [PATCH 06/10] gpiolib: refactor gpio-line-names parsing

[Ahmad Fatoum]

On 02.06.23 09:49, Marco Felsch wrote:
> Move the gpio-line-names parsing out of of_gpiochip_scan_hogs() since
> this has nothing to do with gpio-hogs. The new function is very similar
> with the kernel function devprop_gpiochip_set_names().
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> ---
>  drivers/gpio/gpiolib.c | 104 ++++++++++++++++++++++++++---------------
>  1 file changed, 66 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 166356c85a..127cc60abd 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -515,48 +515,11 @@ static int of_hog_gpio(struct device_node *np, struct gpio_chip *chip,
>  static int of_gpiochip_scan_hogs(struct gpio_chip *chip)
>  {
>  	struct device_node *np;
> -	int ret, i, count;
> +	int ret, i;
>  
>  	if (!IS_ENABLED(CONFIG_OFDEVICE) || !chip->dev->of_node)
>  		return 0;
>  
> -	count = of_property_count_strings(chip->dev->of_node,
> -					  "gpio-line-names");
> -
> -	if (count > 0) {
> -		const char **names = xzalloc(count * sizeof(char *));
> -
> -		ret = of_property_read_string_array(chip->dev->of_node,
> -						    "gpio-line-names", names,
> -						    count);
> -		if (ret < 0) {
> -			dev_warn(chip->dev, "failed to read GPIO line names\n");
> -			kfree(names);
> -			return ret;
> -		}
> -
> -		/*
> -		 * Since property 'gpio-line-names' cannot contains gaps, we
> -		 * have to be sure we only assign those pins that really exists
> -		 * since chip->ngpio can be less.
> -		 */
> -		if (count > chip->ngpio)
> -			count = chip->ngpio;
> -
> -		for (i = 0; i < count; i++) {
> -			/*
> -			 * Allow overriding "fixed" names provided by the GPIO
> -			 * provider. The "fixed" names are more often than not
> -			 * generic and less informative than the names given in
> -			 * device properties.
> -			 */
> -			if (names[i] && names[i][0])
> -				gpio_desc[chip->base + i].name = names[i];
> -		}
> -
> -		free(names);
> -	}
> -
>  	for_each_available_child_of_node(chip->dev->of_node, np) {
>  		if (!of_property_read_bool(np, "gpio-hog"))
>  			continue;
> @@ -576,6 +539,66 @@ static int of_gpiochip_scan_hogs(struct gpio_chip *chip)
>  	return 0;
>  }
>  
> +/*
> + * of_gpiochip_set_names - Set GPIO line names using OF properties
> + * @chip: GPIO chip whose lines should be named, if possible
> + *
> + * Looks for device property "gpio-line-names" and if it exists assigns
> + * GPIO line names for the chip. The memory allocated for the assigned
> + * names belong to the underlying firmware node and should not be released
> + * by the caller.
> + */
> +static int of_gpiochip_set_names(struct gpio_chip *chip)
> +{
> +	struct device *dev = chip->dev;
> +	struct device_node *np;
> +	const char **names;
> +	int ret, i, count;
> +
> +	np = dev_of_node(dev);
> +	if (!np)
> +		return 0;
> +
> +	count = of_property_count_strings(np, "gpio-line-names");
> +	if (count < 0)
> +		return 0;
> +
> +	names = kcalloc(count, sizeof(*names), GFP_KERNEL);
> +	if (!names)
> +		return -ENOMEM;
> +
> +	ret = of_property_read_string_array(np, "gpio-line-names",
> +					    names, count);
> +	if (ret < 0) {
> +		dev_warn(dev, "failed to read GPIO line names\n");
> +		kfree(names);
> +		return ret;
> +	}
> +
> +	/*
> +	 * Since property 'gpio-line-names' cannot contains gaps, we
> +	 * have to be sure we only assign those pins that really exists
> +	 * since chip->ngpio can be less.
> +	 */
> +	if (count > chip->ngpio)
> +		count = chip->ngpio;
> +
> +	for (i = 0; i < count; i++) {
> +		/*
> +		 * Allow overriding "fixed" names provided by the GPIO
> +		 * provider. The "fixed" names are more often than not
> +		 * generic and less informative than the names given in
> +		 * device properties.
> +		 */
> +		if (names[i] && names[i][0])
> +			gpio_desc[chip->base + i].name = names[i];
> +	}
> +
> +	free(names);
> +
> +	return 0;
> +}
> +
>  #ifdef CONFIG_OFDEVICE
>  static const char *gpio_suffixes[] = {
>  	"gpios",
> @@ -637,6 +660,7 @@ int dev_gpiod_get_index(struct device *dev,
>  
>  int gpiochip_add(struct gpio_chip *chip)
>  {
> +	int ret;
>  	int i;
>  
>  	if (chip->base >= 0) {
> @@ -656,6 +680,10 @@ int gpiochip_add(struct gpio_chip *chip)
>  	for (i = chip->base; i < chip->base + chip->ngpio; i++)
>  		gpio_desc[i].chip = chip;
>  
> +	ret = of_gpiochip_set_names(chip);
> +	if (ret)
> +		return ret;
> +
>  	return of_gpiochip_scan_hogs(chip);
>  }
>  

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 07/10] gpiolib: introduce of_gpiochip_add to bundle all of functions

[Marco Felsch]

Introduce a common OF related function which does all the OF related
part. This allow us to drop the santity checks and harmonizes the code
with the linux kernel.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/gpio/gpiolib.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 127cc60abd..d1087aa583 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -517,9 +517,6 @@ static int of_gpiochip_scan_hogs(struct gpio_chip *chip)
 	struct device_node *np;
 	int ret, i;
 
-	if (!IS_ENABLED(CONFIG_OFDEVICE) || !chip->dev->of_node)
-		return 0;
-
 	for_each_available_child_of_node(chip->dev->of_node, np) {
 		if (!of_property_read_bool(np, "gpio-hog"))
 			continue;
@@ -550,15 +547,11 @@ static int of_gpiochip_scan_hogs(struct gpio_chip *chip)
  */
 static int of_gpiochip_set_names(struct gpio_chip *chip)
 {
+	struct device_node *np = dev_of_node(chip->dev);
 	struct device *dev = chip->dev;
-	struct device_node *np;
 	const char **names;
 	int ret, i, count;
 
-	np = dev_of_node(dev);
-	if (!np)
-		return 0;
-
 	count = of_property_count_strings(np, "gpio-line-names");
 	if (count < 0)
 		return 0;
@@ -599,6 +592,22 @@ static int of_gpiochip_set_names(struct gpio_chip *chip)
 	return 0;
 }
 
+static int of_gpiochip_add(struct gpio_chip *chip)
+{
+	struct device_node *np;
+	int ret;
+
+	np = dev_of_node(chip->dev);
+	if (!np)
+		return 0;
+
+	ret = of_gpiochip_set_names(chip);
+	if (ret)
+		return ret;
+
+	return of_gpiochip_scan_hogs(chip);
+}
+
 #ifdef CONFIG_OFDEVICE
 static const char *gpio_suffixes[] = {
 	"gpios",
@@ -660,7 +669,6 @@ int dev_gpiod_get_index(struct device *dev,
 
 int gpiochip_add(struct gpio_chip *chip)
 {
-	int ret;
 	int i;
 
 	if (chip->base >= 0) {
@@ -680,11 +688,7 @@ int gpiochip_add(struct gpio_chip *chip)
 	for (i = chip->base; i < chip->base + chip->ngpio; i++)
 		gpio_desc[i].chip = chip;
 
-	ret = of_gpiochip_set_names(chip);
-	if (ret)
-		return ret;
-
-	return of_gpiochip_scan_hogs(chip);
+	return of_gpiochip_add(chip);
 }
 
 void gpiochip_remove(struct gpio_chip *chip)
-- 
2.39.2

Re: [PATCH 07/10] gpiolib: introduce of_gpiochip_add to bundle all of functions

[Ahmad Fatoum]

On 02.06.23 09:49, Marco Felsch wrote:
> Introduce a common OF related function which does all the OF related
> part. This allow us to drop the santity checks and harmonizes the code
> with the linux kernel.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> ---
>  drivers/gpio/gpiolib.c | 32 ++++++++++++++++++--------------
>  1 file changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 127cc60abd..d1087aa583 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -517,9 +517,6 @@ static int of_gpiochip_scan_hogs(struct gpio_chip *chip)
>  	struct device_node *np;
>  	int ret, i;
>  
> -	if (!IS_ENABLED(CONFIG_OFDEVICE) || !chip->dev->of_node)
> -		return 0;
> -
>  	for_each_available_child_of_node(chip->dev->of_node, np) {
>  		if (!of_property_read_bool(np, "gpio-hog"))
>  			continue;
> @@ -550,15 +547,11 @@ static int of_gpiochip_scan_hogs(struct gpio_chip *chip)
>   */
>  static int of_gpiochip_set_names(struct gpio_chip *chip)
>  {
> +	struct device_node *np = dev_of_node(chip->dev);
>  	struct device *dev = chip->dev;
> -	struct device_node *np;
>  	const char **names;
>  	int ret, i, count;
>  
> -	np = dev_of_node(dev);
> -	if (!np)
> -		return 0;
> -
>  	count = of_property_count_strings(np, "gpio-line-names");
>  	if (count < 0)
>  		return 0;
> @@ -599,6 +592,22 @@ static int of_gpiochip_set_names(struct gpio_chip *chip)
>  	return 0;
>  }
>  
> +static int of_gpiochip_add(struct gpio_chip *chip)
> +{
> +	struct device_node *np;
> +	int ret;
> +
> +	np = dev_of_node(chip->dev);
> +	if (!np)
> +		return 0;
> +
> +	ret = of_gpiochip_set_names(chip);
> +	if (ret)
> +		return ret;
> +
> +	return of_gpiochip_scan_hogs(chip);
> +}
> +
>  #ifdef CONFIG_OFDEVICE
>  static const char *gpio_suffixes[] = {
>  	"gpios",
> @@ -660,7 +669,6 @@ int dev_gpiod_get_index(struct device *dev,
>  
>  int gpiochip_add(struct gpio_chip *chip)
>  {
> -	int ret;
>  	int i;
>  
>  	if (chip->base >= 0) {
> @@ -680,11 +688,7 @@ int gpiochip_add(struct gpio_chip *chip)
>  	for (i = chip->base; i < chip->base + chip->ngpio; i++)
>  		gpio_desc[i].chip = chip;
>  
> -	ret = of_gpiochip_set_names(chip);
> -	if (ret)
> -		return ret;
> -
> -	return of_gpiochip_scan_hogs(chip);
> +	return of_gpiochip_add(chip);
>  }
>  
>  void gpiochip_remove(struct gpio_chip *chip)

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 08/10] OF: gpio: snyc of_get_named_gpio_flags variable with kernel

[Marco Felsch]

Sync local of_phandle_args variable with the kernel to make it easier to
see the diff between both versions. No functional change.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/of/of_gpio.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/of/of_gpio.c b/drivers/of/of_gpio.c
index 5da80e2493..c20133bbfd 100644
--- a/drivers/of/of_gpio.c
+++ b/drivers/of/of_gpio.c
@@ -60,26 +60,26 @@ static void of_gpio_flags_quirks(struct device_node *np,
 int of_get_named_gpio_flags(struct device_node *np, const char *propname,
 			   int index, enum of_gpio_flags *flags)
 {
-	struct of_phandle_args out_args;
+	struct of_phandle_args gpiospec;
 	struct device *dev;
 	int ret;
 
 	ret = of_parse_phandle_with_args(np, propname, "#gpio-cells",
-					index, &out_args);
+					index, &gpiospec);
 	if (ret) {
 		pr_debug("%s: cannot parse %s property: %d\n",
 			__func__, propname, ret);
 		return ret;
 	}
 
-	dev = of_find_device_by_node(out_args.np);
+	dev = of_find_device_by_node(gpiospec.np);
 	if (!dev) {
 		pr_debug("%s: unable to find device of node %s\n",
-			 __func__, out_args.np->full_name);
+			 __func__, gpiospec.np->full_name);
 		return -EPROBE_DEFER;
 	}
 
-	ret = gpio_get_num(dev, out_args.args[0]);
+	ret = gpio_get_num(dev, gpiospec.args[0]);
 	if (ret == -EPROBE_DEFER)
 		return ret;
 	if (ret < 0) {
@@ -89,7 +89,7 @@ int of_get_named_gpio_flags(struct device_node *np, const char *propname,
 	}
 
 	if (flags) {
-		*flags = out_args.args[1];
+		*flags = gpiospec.args[1];
 		of_gpio_flags_quirks(np, propname, flags, index);
 	}
 
-- 
2.39.2

Re: [PATCH 08/10] OF: gpio: snyc of_get_named_gpio_flags variable with kernel

[Jules Maselbas]

Typo in commit title s/snyc/sync/ 
otherwise it looks good to me

On Fri, Jun 02, 2023 at 09:49:19AM +0200, Marco Felsch wrote:
> Sync local of_phandle_args variable with the kernel to make it easier to
> see the diff between both versions. No functional change.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/of/of_gpio.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/of/of_gpio.c b/drivers/of/of_gpio.c
> index 5da80e2493..c20133bbfd 100644
> --- a/drivers/of/of_gpio.c
> +++ b/drivers/of/of_gpio.c
> @@ -60,26 +60,26 @@ static void of_gpio_flags_quirks(struct device_node *np,
>  int of_get_named_gpio_flags(struct device_node *np, const char *propname,
>  			   int index, enum of_gpio_flags *flags)
>  {
> -	struct of_phandle_args out_args;
> +	struct of_phandle_args gpiospec;
>  	struct device *dev;
>  	int ret;
>  
>  	ret = of_parse_phandle_with_args(np, propname, "#gpio-cells",
> -					index, &out_args);
> +					index, &gpiospec);
>  	if (ret) {
>  		pr_debug("%s: cannot parse %s property: %d\n",
>  			__func__, propname, ret);
>  		return ret;
>  	}
>  
> -	dev = of_find_device_by_node(out_args.np);
> +	dev = of_find_device_by_node(gpiospec.np);
>  	if (!dev) {
>  		pr_debug("%s: unable to find device of node %s\n",
> -			 __func__, out_args.np->full_name);
> +			 __func__, gpiospec.np->full_name);
>  		return -EPROBE_DEFER;
>  	}
>  
> -	ret = gpio_get_num(dev, out_args.args[0]);
> +	ret = gpio_get_num(dev, gpiospec.args[0]);
>  	if (ret == -EPROBE_DEFER)
>  		return ret;
>  	if (ret < 0) {
> @@ -89,7 +89,7 @@ int of_get_named_gpio_flags(struct device_node *np, const char *propname,
>  	}
>  
>  	if (flags) {
> -		*flags = out_args.args[1];
> +		*flags = gpiospec.args[1];
>  		of_gpio_flags_quirks(np, propname, flags, index);
>  	}
>  
> -- 
> 2.39.2
> 
> 
> 
> 
> 

Re: [PATCH 08/10] OF: gpio: snyc of_get_named_gpio_flags variable with kernel

[Ahmad Fatoum]

On 02.06.23 09:49, Marco Felsch wrote:
> Sync local of_phandle_args variable with the kernel to make it easier to
> see the diff between both versions. No functional change.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> ---
>  drivers/of/of_gpio.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/of/of_gpio.c b/drivers/of/of_gpio.c
> index 5da80e2493..c20133bbfd 100644
> --- a/drivers/of/of_gpio.c
> +++ b/drivers/of/of_gpio.c
> @@ -60,26 +60,26 @@ static void of_gpio_flags_quirks(struct device_node *np,
>  int of_get_named_gpio_flags(struct device_node *np, const char *propname,
>  			   int index, enum of_gpio_flags *flags)
>  {
> -	struct of_phandle_args out_args;
> +	struct of_phandle_args gpiospec;
>  	struct device *dev;
>  	int ret;
>  
>  	ret = of_parse_phandle_with_args(np, propname, "#gpio-cells",
> -					index, &out_args);
> +					index, &gpiospec);
>  	if (ret) {
>  		pr_debug("%s: cannot parse %s property: %d\n",
>  			__func__, propname, ret);
>  		return ret;
>  	}
>  
> -	dev = of_find_device_by_node(out_args.np);
> +	dev = of_find_device_by_node(gpiospec.np);
>  	if (!dev) {
>  		pr_debug("%s: unable to find device of node %s\n",
> -			 __func__, out_args.np->full_name);
> +			 __func__, gpiospec.np->full_name);
>  		return -EPROBE_DEFER;
>  	}
>  
> -	ret = gpio_get_num(dev, out_args.args[0]);
> +	ret = gpio_get_num(dev, gpiospec.args[0]);
>  	if (ret == -EPROBE_DEFER)
>  		return ret;
>  	if (ret < 0) {
> @@ -89,7 +89,7 @@ int of_get_named_gpio_flags(struct device_node *np, const char *propname,
>  	}
>  
>  	if (flags) {
> -		*flags = out_args.args[1];
> +		*flags = gpiospec.args[1];
>  		of_gpio_flags_quirks(np, propname, flags, index);
>  	}
>  

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 09/10] OF: gpio: fix device_node leakage

[Marco Felsch]

Drop the device_node after we are done.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/of/of_gpio.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/of/of_gpio.c b/drivers/of/of_gpio.c
index c20133bbfd..76398f7542 100644
--- a/drivers/of/of_gpio.c
+++ b/drivers/of/of_gpio.c
@@ -76,16 +76,17 @@ int of_get_named_gpio_flags(struct device_node *np, const char *propname,
 	if (!dev) {
 		pr_debug("%s: unable to find device of node %s\n",
 			 __func__, gpiospec.np->full_name);
-		return -EPROBE_DEFER;
+		ret = -EPROBE_DEFER;
+		goto out;
 	}
 
 	ret = gpio_get_num(dev, gpiospec.args[0]);
 	if (ret == -EPROBE_DEFER)
-		return ret;
+		goto out;
 	if (ret < 0) {
 		pr_err("%s: unable to get gpio num of device %s: %d\n",
 			__func__, dev_name(dev), ret);
-		return ret;
+		goto out;
 	}
 
 	if (flags) {
@@ -93,6 +94,9 @@ int of_get_named_gpio_flags(struct device_node *np, const char *propname,
 		of_gpio_flags_quirks(np, propname, flags, index);
 	}
 
+out:
+	of_node_put(gpiospec.np);
+
 	return ret;
 }
 EXPORT_SYMBOL(of_get_named_gpio_flags);
-- 
2.39.2

Re: [PATCH 09/10] OF: gpio: fix device_node leakage

[Ahmad Fatoum]

On 02.06.23 09:49, Marco Felsch wrote:
> Drop the device_node after we are done.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

There's no leakage fixed here. of_node_put is a no-op in barebox
and even in the kernel, it only does reference counting when
CONFIG_OF_DYNAMIC=y.

I don't mind if change is to align with kernel, so with commit
message reworded:

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> ---
>  drivers/of/of_gpio.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/of/of_gpio.c b/drivers/of/of_gpio.c
> index c20133bbfd..76398f7542 100644
> --- a/drivers/of/of_gpio.c
> +++ b/drivers/of/of_gpio.c
> @@ -76,16 +76,17 @@ int of_get_named_gpio_flags(struct device_node *np, const char *propname,
>  	if (!dev) {
>  		pr_debug("%s: unable to find device of node %s\n",
>  			 __func__, gpiospec.np->full_name);
> -		return -EPROBE_DEFER;
> +		ret = -EPROBE_DEFER;
> +		goto out;
>  	}
>  
>  	ret = gpio_get_num(dev, gpiospec.args[0]);
>  	if (ret == -EPROBE_DEFER)
> -		return ret;
> +		goto out;
>  	if (ret < 0) {
>  		pr_err("%s: unable to get gpio num of device %s: %d\n",
>  			__func__, dev_name(dev), ret);
> -		return ret;
> +		goto out;
>  	}
>  
>  	if (flags) {
> @@ -93,6 +94,9 @@ int of_get_named_gpio_flags(struct device_node *np, const char *propname,
>  		of_gpio_flags_quirks(np, propname, flags, index);
>  	}
>  
> +out:
> +	of_node_put(gpiospec.np);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL(of_get_named_gpio_flags);

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 09/10] OF: gpio: fix device_node leakage

[Marco Felsch]

On 23-06-13, Ahmad Fatoum wrote:
> On 02.06.23 09:49, Marco Felsch wrote:
> > Drop the device_node after we are done.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> 
> There's no leakage fixed here. of_node_put is a no-op in barebox
> and even in the kernel, it only does reference counting when
> CONFIG_OF_DYNAMIC=y.

Didn't noticed that of_node_put() is a no-op but since barebox does
start to handle of-overlays for its own devicetrees we should reconsider
that.

> I don't mind if change is to align with kernel, so with commit
> message reworded:

Sure.

Regards,
  Marco

> 
> Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> 
> > ---
> >  drivers/of/of_gpio.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/of/of_gpio.c b/drivers/of/of_gpio.c
> > index c20133bbfd..76398f7542 100644
> > --- a/drivers/of/of_gpio.c
> > +++ b/drivers/of/of_gpio.c
> > @@ -76,16 +76,17 @@ int of_get_named_gpio_flags(struct device_node *np, const char *propname,
> >  	if (!dev) {
> >  		pr_debug("%s: unable to find device of node %s\n",
> >  			 __func__, gpiospec.np->full_name);
> > -		return -EPROBE_DEFER;
> > +		ret = -EPROBE_DEFER;
> > +		goto out;
> >  	}
> >  
> >  	ret = gpio_get_num(dev, gpiospec.args[0]);
> >  	if (ret == -EPROBE_DEFER)
> > -		return ret;
> > +		goto out;
> >  	if (ret < 0) {
> >  		pr_err("%s: unable to get gpio num of device %s: %d\n",
> >  			__func__, dev_name(dev), ret);
> > -		return ret;
> > +		goto out;
> >  	}
> >  
> >  	if (flags) {
> > @@ -93,6 +94,9 @@ int of_get_named_gpio_flags(struct device_node *np, const char *propname,
> >  		of_gpio_flags_quirks(np, propname, flags, index);
> >  	}
> >  
> > +out:
> > +	of_node_put(gpiospec.np);
> > +
> >  	return ret;
> >  }
> >  EXPORT_SYMBOL(of_get_named_gpio_flags);
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 
> 

[PATCH 10/10] gpiolib: add of_xlate support

[Marco Felsch]

The of_xlate function can be used to by drivers which need a other gpio
encoding than:

  gpio = <&phandle gpio-nr gpio-flags>;

The of_xlate code is as close as possible to the kernel counter part
which makes it easier to add 'struct gpio_desc' support later on.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/gpio/gpiolib.c | 51 ++++++++++++++++++++++++++++++++++++++
 drivers/of/of_gpio.c   | 56 +++++++++++++++++++++++++++++++++---------
 include/gpio.h         | 25 +++++++++++++++++++
 3 files changed, 120 insertions(+), 12 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index d1087aa583..ab9a0e30be 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -592,6 +592,43 @@ static int of_gpiochip_set_names(struct gpio_chip *chip)
 	return 0;
 }
 
+/**
+ * of_gpio_simple_xlate - translate gpiospec to the GPIO number and flags
+ * @gc:		pointer to the gpio_chip structure
+ * @gpiospec:	GPIO specifier as found in the device tree
+ * @flags:	a flags pointer to fill in
+ *
+ * This is simple translation function, suitable for the most 1:1 mapped
+ * GPIO chips. This function performs only one sanity check: whether GPIO
+ * is less than ngpios (that is specified in the gpio_chip).
+ */
+static int of_gpio_simple_xlate(struct gpio_chip *gc,
+				const struct of_phandle_args *gpiospec,
+				u32 *flags)
+{
+	/*
+	 * We're discouraging gpio_cells < 2, since that way you'll have to
+	 * write your own xlate function (that will have to retrieve the GPIO
+	 * number and the flags from a single gpio cell -- this is possible,
+	 * but not recommended).
+	 */
+	if (gc->of_gpio_n_cells < 2) {
+		WARN_ON(1);
+		return -EINVAL;
+	}
+
+	if (WARN_ON(gpiospec->args_count < gc->of_gpio_n_cells))
+		return -EINVAL;
+
+	if (gpiospec->args[0] >= gc->ngpio)
+		return -EINVAL;
+
+	if (flags)
+		*flags = gpiospec->args[1];
+
+	return gc->base + gpiospec->args[0];
+}
+
 static int of_gpiochip_add(struct gpio_chip *chip)
 {
 	struct device_node *np;
@@ -601,6 +638,20 @@ static int of_gpiochip_add(struct gpio_chip *chip)
 	if (!np)
 		return 0;
 
+	if (!chip->ops->of_xlate)
+		chip->ops->of_xlate = of_gpio_simple_xlate;
+
+	/*
+	 * Seperate check since the 'struct gpio_ops' is alawys the same for
+	 * every 'struct gpio_chip' of the same instance (e.g. 'struct
+	 * imx_gpio_chip').
+	 */
+	if (chip->ops->of_xlate == of_gpio_simple_xlate)
+		chip->of_gpio_n_cells = 2;
+
+	if (chip->of_gpio_n_cells > MAX_PHANDLE_ARGS)
+		return -EINVAL;
+
 	ret = of_gpiochip_set_names(chip);
 	if (ret)
 		return ret;
diff --git a/drivers/of/of_gpio.c b/drivers/of/of_gpio.c
index 76398f7542..a1fa563a35 100644
--- a/drivers/of/of_gpio.c
+++ b/drivers/of/of_gpio.c
@@ -46,6 +46,44 @@ static void of_gpio_flags_quirks(struct device_node *np,
 
 }
 
+static struct gpio_chip *of_find_gpiochip_by_xlate(
+					struct of_phandle_args *gpiospec)
+{
+	struct gpio_chip *chip;
+	struct device *dev;
+
+	dev = of_find_device_by_node(gpiospec->np);
+	if (!dev) {
+		pr_debug("%s: unable to find device of node %s\n",
+			 __func__, gpiospec->np->full_name);
+		return NULL;
+	}
+
+	chip = gpio_get_chip_by_dev(dev);
+	if (!chip) {
+		pr_debug("%s: unable to find gpiochip\n", __func__);
+		return NULL;
+	}
+
+	if (chip->ops->of_xlate &&
+	    chip->ops->of_xlate(chip, gpiospec, NULL) >= 0)
+		return chip;
+
+	pr_err("%s: failed to execute of_xlate\n", __func__);
+
+	return NULL;
+}
+
+static int of_xlate_and_get_gpiod_flags(struct gpio_chip *chip,
+					struct of_phandle_args *gpiospec,
+					enum of_gpio_flags *flags)
+{
+	if (chip->of_gpio_n_cells != gpiospec->args_count)
+		return -EINVAL;
+
+	return chip->ops->of_xlate(chip, gpiospec, flags);
+}
+
 /**
  * of_get_named_gpio_flags() - Get a GPIO number and flags to use with GPIO API
  * @np:		device node to get GPIO from
@@ -61,7 +99,7 @@ int of_get_named_gpio_flags(struct device_node *np, const char *propname,
 			   int index, enum of_gpio_flags *flags)
 {
 	struct of_phandle_args gpiospec;
-	struct device *dev;
+	struct gpio_chip *chip;
 	int ret;
 
 	ret = of_parse_phandle_with_args(np, propname, "#gpio-cells",
@@ -72,27 +110,21 @@ int of_get_named_gpio_flags(struct device_node *np, const char *propname,
 		return ret;
 	}
 
-	dev = of_find_device_by_node(gpiospec.np);
-	if (!dev) {
-		pr_debug("%s: unable to find device of node %s\n",
-			 __func__, gpiospec.np->full_name);
+	chip = of_find_gpiochip_by_xlate(&gpiospec);
+	if (!chip) {
 		ret = -EPROBE_DEFER;
 		goto out;
 	}
 
-	ret = gpio_get_num(dev, gpiospec.args[0]);
-	if (ret == -EPROBE_DEFER)
-		goto out;
+	ret = of_xlate_and_get_gpiod_flags(chip, &gpiospec, flags);
 	if (ret < 0) {
 		pr_err("%s: unable to get gpio num of device %s: %d\n",
-			__func__, dev_name(dev), ret);
+			__func__, dev_name(chip->dev), ret);
 		goto out;
 	}
 
-	if (flags) {
-		*flags = gpiospec.args[1];
+	if (flags)
 		of_gpio_flags_quirks(np, propname, flags, index);
-	}
 
 out:
 	of_node_put(gpiospec.np);
diff --git a/include/gpio.h b/include/gpio.h
index 5f2c16584c..d82b4505e0 100644
--- a/include/gpio.h
+++ b/include/gpio.h
@@ -177,6 +177,22 @@ struct gpio_ops {
 	int (*get_direction)(struct gpio_chip *chip, unsigned offset);
 	int (*get)(struct gpio_chip *chip, unsigned offset);
 	void (*set)(struct gpio_chip *chip, unsigned offset, int value);
+
+#if defined(CONFIG_OF_GPIO)
+	/*
+	 * If CONFIG_OF_GPIO is enabled, then all GPIO controllers described in
+	 * the device tree automatically may have an OF translation
+	 */
+
+	/**
+	 * @of_xlate:
+	 *
+	 * Callback to translate a device tree GPIO specifier into a chip-
+	 * relative GPIO number and flags.
+	 */
+	int (*of_xlate)(struct gpio_chip *gc,
+			const struct of_phandle_args *gpiospec, u32 *flags);
+#endif
 };
 
 struct gpio_chip {
@@ -185,6 +201,15 @@ struct gpio_chip {
 	int base;
 	int ngpio;
 
+#if defined(CONFIG_OF_GPIO)
+	/**
+	 * @of_gpio_n_cells:
+	 *
+	 * Number of cells used to form the GPIO specifier.
+	 */
+	unsigned int of_gpio_n_cells;
+#endif
+
 	struct gpio_ops *ops;
 
 	struct list_head list;
-- 
2.39.2

Re: [PATCH 10/10] gpiolib: add of_xlate support

[Jules Maselbas]

Hi Marco,

On Fri, Jun 02, 2023 at 09:49:21AM +0200, Marco Felsch wrote:
> The of_xlate function can be used to by drivers which need a other gpio
> encoding than:
> 
>   gpio = <&phandle gpio-nr gpio-flags>;
> 
> The of_xlate code is as close as possible to the kernel counter part
> which makes it easier to add 'struct gpio_desc' support later on.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/gpio/gpiolib.c | 51 ++++++++++++++++++++++++++++++++++++++
>  drivers/of/of_gpio.c   | 56 +++++++++++++++++++++++++++++++++---------
>  include/gpio.h         | 25 +++++++++++++++++++
>  3 files changed, 120 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index d1087aa583..ab9a0e30be 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -592,6 +592,43 @@ static int of_gpiochip_set_names(struct gpio_chip *chip)
>  	return 0;
>  }
>  
> +/**
> + * of_gpio_simple_xlate - translate gpiospec to the GPIO number and flags
> + * @gc:		pointer to the gpio_chip structure
> + * @gpiospec:	GPIO specifier as found in the device tree
> + * @flags:	a flags pointer to fill in
> + *
> + * This is simple translation function, suitable for the most 1:1 mapped
> + * GPIO chips. This function performs only one sanity check: whether GPIO
> + * is less than ngpios (that is specified in the gpio_chip).
> + */
> +static int of_gpio_simple_xlate(struct gpio_chip *gc,
> +				const struct of_phandle_args *gpiospec,
> +				u32 *flags)
> +{
> +	/*
> +	 * We're discouraging gpio_cells < 2, since that way you'll have to
> +	 * write your own xlate function (that will have to retrieve the GPIO
> +	 * number and the flags from a single gpio cell -- this is possible,
> +	 * but not recommended).
> +	 */
> +	if (gc->of_gpio_n_cells < 2) {
> +		WARN_ON(1);
> +		return -EINVAL;
> +	}
> +
> +	if (WARN_ON(gpiospec->args_count < gc->of_gpio_n_cells))
> +		return -EINVAL;
> +
> +	if (gpiospec->args[0] >= gc->ngpio)
> +		return -EINVAL;
> +
> +	if (flags)
> +		*flags = gpiospec->args[1];
> +
> +	return gc->base + gpiospec->args[0];
> +}
> +
>  static int of_gpiochip_add(struct gpio_chip *chip)
>  {
>  	struct device_node *np;
> @@ -601,6 +638,20 @@ static int of_gpiochip_add(struct gpio_chip *chip)
>  	if (!np)
>  		return 0;
>  
> +	if (!chip->ops->of_xlate)
> +		chip->ops->of_xlate = of_gpio_simple_xlate;
> +
> +	/*
> +	 * Seperate check since the 'struct gpio_ops' is alawys the same for
typos: s/Seperate/Separate/  and s/alawys/always/

> +	 * every 'struct gpio_chip' of the same instance (e.g. 'struct
> +	 * imx_gpio_chip').
> +	 */
> +	if (chip->ops->of_xlate == of_gpio_simple_xlate)
> +		chip->of_gpio_n_cells = 2;
> +
> +	if (chip->of_gpio_n_cells > MAX_PHANDLE_ARGS)
> +		return -EINVAL;
> +
>  	ret = of_gpiochip_set_names(chip);
>  	if (ret)
>  		return ret;

Thanks :)

Re: [PATCH 10/10] gpiolib: add of_xlate support

[Jules Maselbas]

Hi Marco,

I tried this patch series on my sunxi branch and it applies nicely
but failed to get the gpio with the following message:
of_get_named_gpio_flags: unable to get gpio num of device 1c20800.pinctrl@1c20800.of: -22

The error -22 is for -EINVAL which comes form of_xlate_and_get_gpiod_flags
because the test `chip->of_gpio_n_cells != gpiospec->args_count` fails,
`of_gpio_n_cells` hasn't been set by the driver and it value was 0.

I wonder if `of_gpio_n_cells` should be set by the driver or by parsing the
#gpio-cells property from device-tree?

By setting the expected value in the driver resolves this "issue".

Cheers,
-- Jules

> diff --git a/drivers/of/of_gpio.c b/drivers/of/of_gpio.c
> +static int of_xlate_and_get_gpiod_flags(struct gpio_chip *chip,
> +					struct of_phandle_args *gpiospec,
> +					enum of_gpio_flags *flags)
> +{
> +	if (chip->of_gpio_n_cells != gpiospec->args_count)
> +		return -EINVAL;
error message caused by this ^

> +
> +	return chip->ops->of_xlate(chip, gpiospec, flags);
> +}
> +
>  /**
>   * of_get_named_gpio_flags() - Get a GPIO number and flags to use with GPIO API
>   * @np:		device node to get GPIO from
> @@ -61,7 +99,7 @@ int of_get_named_gpio_flags(struct device_node *np, const char *propname,
>  			   int index, enum of_gpio_flags *flags)
>  {
>  	struct of_phandle_args gpiospec;
> -	struct device *dev;
> +	struct gpio_chip *chip;
>  	int ret;
>  
>  	ret = of_parse_phandle_with_args(np, propname, "#gpio-cells",
> @@ -72,27 +110,21 @@ int of_get_named_gpio_flags(struct device_node *np, const char *propname,
>  		return ret;
>  	}
>  
> -	dev = of_find_device_by_node(gpiospec.np);
> -	if (!dev) {
> -		pr_debug("%s: unable to find device of node %s\n",
> -			 __func__, gpiospec.np->full_name);
> +	chip = of_find_gpiochip_by_xlate(&gpiospec);
> +	if (!chip) {
>  		ret = -EPROBE_DEFER;
>  		goto out;
>  	}
>  
> -	ret = gpio_get_num(dev, gpiospec.args[0]);
> -	if (ret == -EPROBE_DEFER)
> -		goto out;
> +	ret = of_xlate_and_get_gpiod_flags(chip, &gpiospec, flags);
>  	if (ret < 0) {
>  		pr_err("%s: unable to get gpio num of device %s: %d\n",
got this error message here

> -			__func__, dev_name(dev), ret);
> +			__func__, dev_name(chip->dev), ret);
>  		goto out;
>  	}
>  
> -	if (flags) {
> -		*flags = gpiospec.args[1];
> +	if (flags)
>  		of_gpio_flags_quirks(np, propname, flags, index);
> -	}
>  
>  out:
>  	of_node_put(gpiospec.np);

Re: [PATCH 10/10] gpiolib: add of_xlate support

[Marco Felsch]

Hi Jules,

On 23-06-05, Jules Maselbas wrote:
> Hi Marco,
> 
> I tried this patch series on my sunxi branch and it applies nicely
> but failed to get the gpio with the following message:
> of_get_named_gpio_flags: unable to get gpio num of device 1c20800.pinctrl@1c20800.of: -22
> 
> The error -22 is for -EINVAL which comes form of_xlate_and_get_gpiod_flags
> because the test `chip->of_gpio_n_cells != gpiospec->args_count` fails,
> `of_gpio_n_cells` hasn't been set by the driver and it value was 0.
> 
> I wonder if `of_gpio_n_cells` should be set by the driver or by parsing the
> #gpio-cells property from device-tree?

The driver need to define the cells since the driver is the one
interpreting the cells, please have a look on:

https://elixir.bootlin.com/linux/v6.4-rc1/source/drivers/pinctrl/sunxi/pinctrl-sunxi.c#L1583

> By setting the expected value in the driver resolves this "issue".

Cool :) so you are able to controll the sunxi-gpios? :)

Thanks a lot for testing. I will send a v2 with your found spelling
issues.

Regards,
  Marco

> Cheers,
> -- Jules
> 
> > diff --git a/drivers/of/of_gpio.c b/drivers/of/of_gpio.c
> > +static int of_xlate_and_get_gpiod_flags(struct gpio_chip *chip,
> > +					struct of_phandle_args *gpiospec,
> > +					enum of_gpio_flags *flags)
> > +{
> > +	if (chip->of_gpio_n_cells != gpiospec->args_count)
> > +		return -EINVAL;
> error message caused by this ^
> 
> > +
> > +	return chip->ops->of_xlate(chip, gpiospec, flags);
> > +}
> > +
> >  /**
> >   * of_get_named_gpio_flags() - Get a GPIO number and flags to use with GPIO API
> >   * @np:		device node to get GPIO from
> > @@ -61,7 +99,7 @@ int of_get_named_gpio_flags(struct device_node *np, const char *propname,
> >  			   int index, enum of_gpio_flags *flags)
> >  {
> >  	struct of_phandle_args gpiospec;
> > -	struct device *dev;
> > +	struct gpio_chip *chip;
> >  	int ret;
> >  
> >  	ret = of_parse_phandle_with_args(np, propname, "#gpio-cells",
> > @@ -72,27 +110,21 @@ int of_get_named_gpio_flags(struct device_node *np, const char *propname,
> >  		return ret;
> >  	}
> >  
> > -	dev = of_find_device_by_node(gpiospec.np);
> > -	if (!dev) {
> > -		pr_debug("%s: unable to find device of node %s\n",
> > -			 __func__, gpiospec.np->full_name);
> > +	chip = of_find_gpiochip_by_xlate(&gpiospec);
> > +	if (!chip) {
> >  		ret = -EPROBE_DEFER;
> >  		goto out;
> >  	}
> >  
> > -	ret = gpio_get_num(dev, gpiospec.args[0]);
> > -	if (ret == -EPROBE_DEFER)
> > -		goto out;
> > +	ret = of_xlate_and_get_gpiod_flags(chip, &gpiospec, flags);
> >  	if (ret < 0) {
> >  		pr_err("%s: unable to get gpio num of device %s: %d\n",
> got this error message here
> 
> > -			__func__, dev_name(dev), ret);
> > +			__func__, dev_name(chip->dev), ret);
> >  		goto out;
> >  	}
> >  
> > -	if (flags) {
> > -		*flags = gpiospec.args[1];
> > +	if (flags)
> >  		of_gpio_flags_quirks(np, propname, flags, index);
> > -	}
> >  
> >  out:
> >  	of_node_put(gpiospec.np);
> 

Re: [PATCH 10/10] gpiolib: add of_xlate support

[Ahmad Fatoum]

On 02.06.23 09:49, Marco Felsch wrote:
> The of_xlate function can be used to by drivers which need a other gpio
> encoding than:
> 
>   gpio = <&phandle gpio-nr gpio-flags>;
> 
> The of_xlate code is as close as possible to the kernel counter part
> which makes it easier to add 'struct gpio_desc' support later on.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Small nitpicks below, but looks good:

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

Thanks for reworking this! Would be great if this can go into next
before release is tagged.

> ---
>  drivers/gpio/gpiolib.c | 51 ++++++++++++++++++++++++++++++++++++++
>  drivers/of/of_gpio.c   | 56 +++++++++++++++++++++++++++++++++---------
>  include/gpio.h         | 25 +++++++++++++++++++
>  3 files changed, 120 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index d1087aa583..ab9a0e30be 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -592,6 +592,43 @@ static int of_gpiochip_set_names(struct gpio_chip *chip)
>  	return 0;
>  }
>  
> +/**
> + * of_gpio_simple_xlate - translate gpiospec to the GPIO number and flags
> + * @gc:		pointer to the gpio_chip structure
> + * @gpiospec:	GPIO specifier as found in the device tree
> + * @flags:	a flags pointer to fill in
> + *
> + * This is simple translation function, suitable for the most 1:1 mapped
> + * GPIO chips. This function performs only one sanity check: whether GPIO
> + * is less than ngpios (that is specified in the gpio_chip).
> + */
> +static int of_gpio_simple_xlate(struct gpio_chip *gc,
> +				const struct of_phandle_args *gpiospec,
> +				u32 *flags)
> +{
> +	/*
> +	 * We're discouraging gpio_cells < 2, since that way you'll have to
> +	 * write your own xlate function (that will have to retrieve the GPIO
> +	 * number and the flags from a single gpio cell -- this is possible,
> +	 * but not recommended).
> +	 */
> +	if (gc->of_gpio_n_cells < 2) {
> +		WARN_ON(1);

Nitpick: could have changed above to WARN_ON(gc->of_gpio_n_cells < 2)
and dropped the curlies.

> +		return -EINVAL;
> +	}
> +
> +	if (WARN_ON(gpiospec->args_count < gc->of_gpio_n_cells))
> +		return -EINVAL;
> +
> +	if (gpiospec->args[0] >= gc->ngpio)
> +		return -EINVAL;
> +
> +	if (flags)
> +		*flags = gpiospec->args[1];
> +
> +	return gc->base + gpiospec->args[0];
> +}
> +
>  static int of_gpiochip_add(struct gpio_chip *chip)
>  {
>  	struct device_node *np;
> @@ -601,6 +638,20 @@ static int of_gpiochip_add(struct gpio_chip *chip)
>  	if (!np)
>  		return 0;
>  
> +	if (!chip->ops->of_xlate)
> +		chip->ops->of_xlate = of_gpio_simple_xlate;
> +
> +	/*
> +	 * Seperate check since the 'struct gpio_ops' is alawys the same for

seperate, always

> +	 * every 'struct gpio_chip' of the same instance (e.g. 'struct
> +	 * imx_gpio_chip').
> +	 */
> +	if (chip->ops->of_xlate == of_gpio_simple_xlate)
> +		chip->of_gpio_n_cells = 2;
> +
> +	if (chip->of_gpio_n_cells > MAX_PHANDLE_ARGS)
> +		return -EINVAL;
> +
>  	ret = of_gpiochip_set_names(chip);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/of/of_gpio.c b/drivers/of/of_gpio.c
> index 76398f7542..a1fa563a35 100644
> --- a/drivers/of/of_gpio.c
> +++ b/drivers/of/of_gpio.c
> @@ -46,6 +46,44 @@ static void of_gpio_flags_quirks(struct device_node *np,
>  
>  }
>  
> +static struct gpio_chip *of_find_gpiochip_by_xlate(
> +					struct of_phandle_args *gpiospec)
> +{
> +	struct gpio_chip *chip;
> +	struct device *dev;
> +
> +	dev = of_find_device_by_node(gpiospec->np);
> +	if (!dev) {
> +		pr_debug("%s: unable to find device of node %s\n",
> +			 __func__, gpiospec->np->full_name);

Nitpick: We have %pOF for this.

> +		return NULL;
> +	}
> +
> +	chip = gpio_get_chip_by_dev(dev);
> +	if (!chip) {
> +		pr_debug("%s: unable to find gpiochip\n", __func__);
> +		return NULL;
> +	}
> +
> +	if (chip->ops->of_xlate &&
> +	    chip->ops->of_xlate(chip, gpiospec, NULL) >= 0)
> +		return chip;

Very nitpicky: inverting this if and turning it into an early-exit
would make code more readable. But if kernel does it that way, that's
fine too.

> +
> +	pr_err("%s: failed to execute of_xlate\n", __func__);
> +
> +	return NULL;
> +}
> +
> +static int of_xlate_and_get_gpiod_flags(struct gpio_chip *chip,
> +					struct of_phandle_args *gpiospec,
> +					enum of_gpio_flags *flags)
> +{
> +	if (chip->of_gpio_n_cells != gpiospec->args_count)
> +		return -EINVAL;
> +
> +	return chip->ops->of_xlate(chip, gpiospec, flags);
> +}
> +
>  /**
>   * of_get_named_gpio_flags() - Get a GPIO number and flags to use with GPIO API
>   * @np:		device node to get GPIO from
> @@ -61,7 +99,7 @@ int of_get_named_gpio_flags(struct device_node *np, const char *propname,
>  			   int index, enum of_gpio_flags *flags)
>  {
>  	struct of_phandle_args gpiospec;
> -	struct device *dev;
> +	struct gpio_chip *chip;
>  	int ret;
>  
>  	ret = of_parse_phandle_with_args(np, propname, "#gpio-cells",
> @@ -72,27 +110,21 @@ int of_get_named_gpio_flags(struct device_node *np, const char *propname,
>  		return ret;
>  	}
>  
> -	dev = of_find_device_by_node(gpiospec.np);
> -	if (!dev) {
> -		pr_debug("%s: unable to find device of node %s\n",
> -			 __func__, gpiospec.np->full_name);
> +	chip = of_find_gpiochip_by_xlate(&gpiospec);
> +	if (!chip) {
>  		ret = -EPROBE_DEFER;
>  		goto out;
>  	}
>  
> -	ret = gpio_get_num(dev, gpiospec.args[0]);
> -	if (ret == -EPROBE_DEFER)
> -		goto out;
> +	ret = of_xlate_and_get_gpiod_flags(chip, &gpiospec, flags);
>  	if (ret < 0) {
>  		pr_err("%s: unable to get gpio num of device %s: %d\n",
> -			__func__, dev_name(dev), ret);
> +			__func__, dev_name(chip->dev), ret);
>  		goto out;
>  	}
>  
> -	if (flags) {
> -		*flags = gpiospec.args[1];
> +	if (flags)
>  		of_gpio_flags_quirks(np, propname, flags, index);
> -	}
>  
>  out:
>  	of_node_put(gpiospec.np);
> diff --git a/include/gpio.h b/include/gpio.h
> index 5f2c16584c..d82b4505e0 100644
> --- a/include/gpio.h
> +++ b/include/gpio.h
> @@ -177,6 +177,22 @@ struct gpio_ops {
>  	int (*get_direction)(struct gpio_chip *chip, unsigned offset);
>  	int (*get)(struct gpio_chip *chip, unsigned offset);
>  	void (*set)(struct gpio_chip *chip, unsigned offset, int value);
> +
> +#if defined(CONFIG_OF_GPIO)
> +	/*
> +	 * If CONFIG_OF_GPIO is enabled, then all GPIO controllers described in
> +	 * the device tree automatically may have an OF translation
> +	 */
> +
> +	/**
> +	 * @of_xlate:
> +	 *
> +	 * Callback to translate a device tree GPIO specifier into a chip-
> +	 * relative GPIO number and flags.
> +	 */
> +	int (*of_xlate)(struct gpio_chip *gc,
> +			const struct of_phandle_args *gpiospec, u32 *flags);
> +#endif
>  };
>  
>  struct gpio_chip {
> @@ -185,6 +201,15 @@ struct gpio_chip {
>  	int base;
>  	int ngpio;
>  
> +#if defined(CONFIG_OF_GPIO)
> +	/**
> +	 * @of_gpio_n_cells:
> +	 *
> +	 * Number of cells used to form the GPIO specifier.
> +	 */
> +	unsigned int of_gpio_n_cells;
> +#endif
> +
>  	struct gpio_ops *ops;
>  
>  	struct list_head list;

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 10/10] gpiolib: add of_xlate support

[Ahmad Fatoum]

On 02.06.23 09:49, Marco Felsch wrote:
> The of_xlate function can be used to by drivers which need a other gpio
> encoding than:
> 
>   gpio = <&phandle gpio-nr gpio-flags>;
> 
> The of_xlate code is as close as possible to the kernel counter part
> which makes it easier to add 'struct gpio_desc' support later on.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/gpio/gpiolib.c | 51 ++++++++++++++++++++++++++++++++++++++
>  drivers/of/of_gpio.c   | 56 +++++++++++++++++++++++++++++++++---------
>  include/gpio.h         | 25 +++++++++++++++++++
>  3 files changed, 120 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index d1087aa583..ab9a0e30be 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -592,6 +592,43 @@ static int of_gpiochip_set_names(struct gpio_chip *chip)
>  	return 0;
>  }
>  
> +/**
> + * of_gpio_simple_xlate - translate gpiospec to the GPIO number and flags
> + * @gc:		pointer to the gpio_chip structure
> + * @gpiospec:	GPIO specifier as found in the device tree
> + * @flags:	a flags pointer to fill in
> + *
> + * This is simple translation function, suitable for the most 1:1 mapped
> + * GPIO chips. This function performs only one sanity check: whether GPIO
> + * is less than ngpios (that is specified in the gpio_chip).
> + */
> +static int of_gpio_simple_xlate(struct gpio_chip *gc,
> +				const struct of_phandle_args *gpiospec,
> +				u32 *flags)
> +{
> +	/*
> +	 * We're discouraging gpio_cells < 2, since that way you'll have to
> +	 * write your own xlate function (that will have to retrieve the GPIO
> +	 * number and the flags from a single gpio cell -- this is possible,
> +	 * but not recommended).
> +	 */
> +	if (gc->of_gpio_n_cells < 2) {
> +		WARN_ON(1);
> +		return -EINVAL;
> +	}
> +
> +	if (WARN_ON(gpiospec->args_count < gc->of_gpio_n_cells))
> +		return -EINVAL;
> +
> +	if (gpiospec->args[0] >= gc->ngpio)
> +		return -EINVAL;
> +
> +	if (flags)
> +		*flags = gpiospec->args[1];
> +
> +	return gc->base + gpiospec->args[0];
> +}
> +
>  static int of_gpiochip_add(struct gpio_chip *chip)
>  {
>  	struct device_node *np;
> @@ -601,6 +638,20 @@ static int of_gpiochip_add(struct gpio_chip *chip)
>  	if (!np)
>  		return 0;
>  
> +	if (!chip->ops->of_xlate)
> +		chip->ops->of_xlate = of_gpio_simple_xlate;
> +
> +	/*
> +	 * Seperate check since the 'struct gpio_ops' is alawys the same for
> +	 * every 'struct gpio_chip' of the same instance (e.g. 'struct
> +	 * imx_gpio_chip').
> +	 */
> +	if (chip->ops->of_xlate == of_gpio_simple_xlate)
> +		chip->of_gpio_n_cells = 2;
> +
> +	if (chip->of_gpio_n_cells > MAX_PHANDLE_ARGS)
> +		return -EINVAL;
> +
>  	ret = of_gpiochip_set_names(chip);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/of/of_gpio.c b/drivers/of/of_gpio.c
> index 76398f7542..a1fa563a35 100644
> --- a/drivers/of/of_gpio.c
> +++ b/drivers/of/of_gpio.c
> @@ -46,6 +46,44 @@ static void of_gpio_flags_quirks(struct device_node *np,
>  
>  }
>  
> +static struct gpio_chip *of_find_gpiochip_by_xlate(
> +					struct of_phandle_args *gpiospec)
> +{
> +	struct gpio_chip *chip;
> +	struct device *dev;
> +
> +	dev = of_find_device_by_node(gpiospec->np);
> +	if (!dev) {
> +		pr_debug("%s: unable to find device of node %s\n",
> +			 __func__, gpiospec->np->full_name);
> +		return NULL;
> +	}
> +
> +	chip = gpio_get_chip_by_dev(dev);
> +	if (!chip) {
> +		pr_debug("%s: unable to find gpiochip\n", __func__);
> +		return NULL;
> +	}
> +
> +	if (chip->ops->of_xlate &&
> +	    chip->ops->of_xlate(chip, gpiospec, NULL) >= 0)
> +		return chip;
> +
> +	pr_err("%s: failed to execute of_xlate\n", __func__);
> +
> +	return NULL;
> +}
> +
> +static int of_xlate_and_get_gpiod_flags(struct gpio_chip *chip,
> +					struct of_phandle_args *gpiospec,
> +					enum of_gpio_flags *flags)
> +{
> +	if (chip->of_gpio_n_cells != gpiospec->args_count)
> +		return -EINVAL;
> +
> +	return chip->ops->of_xlate(chip, gpiospec, flags);
> +}
> +
>  /**
>   * of_get_named_gpio_flags() - Get a GPIO number and flags to use with GPIO API
>   * @np:		device node to get GPIO from
> @@ -61,7 +99,7 @@ int of_get_named_gpio_flags(struct device_node *np, const char *propname,
>  			   int index, enum of_gpio_flags *flags)
>  {
>  	struct of_phandle_args gpiospec;
> -	struct device *dev;
> +	struct gpio_chip *chip;
>  	int ret;
>  
>  	ret = of_parse_phandle_with_args(np, propname, "#gpio-cells",
> @@ -72,27 +110,21 @@ int of_get_named_gpio_flags(struct device_node *np, const char *propname,
>  		return ret;
>  	}
>  
> -	dev = of_find_device_by_node(gpiospec.np);
> -	if (!dev) {
> -		pr_debug("%s: unable to find device of node %s\n",
> -			 __func__, gpiospec.np->full_name);
> +	chip = of_find_gpiochip_by_xlate(&gpiospec);
> +	if (!chip) {
>  		ret = -EPROBE_DEFER;
>  		goto out;
>  	}
>  
> -	ret = gpio_get_num(dev, gpiospec.args[0]);
> -	if (ret == -EPROBE_DEFER)
> -		goto out;
> +	ret = of_xlate_and_get_gpiod_flags(chip, &gpiospec, flags);
>  	if (ret < 0) {
>  		pr_err("%s: unable to get gpio num of device %s: %d\n",
> -			__func__, dev_name(dev), ret);
> +			__func__, dev_name(chip->dev), ret);
>  		goto out;
>  	}
>  
> -	if (flags) {
> -		*flags = gpiospec.args[1];
> +	if (flags)
>  		of_gpio_flags_quirks(np, propname, flags, index);
> -	}
>  
>  out:
>  	of_node_put(gpiospec.np);
> diff --git a/include/gpio.h b/include/gpio.h
> index 5f2c16584c..d82b4505e0 100644
> --- a/include/gpio.h
> +++ b/include/gpio.h
> @@ -177,6 +177,22 @@ struct gpio_ops {
>  	int (*get_direction)(struct gpio_chip *chip, unsigned offset);
>  	int (*get)(struct gpio_chip *chip, unsigned offset);
>  	void (*set)(struct gpio_chip *chip, unsigned offset, int value);
> +
> +#if defined(CONFIG_OF_GPIO)
> +	/*
> +	 * If CONFIG_OF_GPIO is enabled, then all GPIO controllers described in
> +	 * the device tree automatically may have an OF translation
> +	 */
> +
> +	/**
> +	 * @of_xlate:
> +	 *
> +	 * Callback to translate a device tree GPIO specifier into a chip-
> +	 * relative GPIO number and flags.
> +	 */
> +	int (*of_xlate)(struct gpio_chip *gc,
> +			const struct of_phandle_args *gpiospec, u32 *flags);

Forward declaration for of_phandle_args missing.

> +#endif
>  };
>  
>  struct gpio_chip {
> @@ -185,6 +201,15 @@ struct gpio_chip {
>  	int base;
>  	int ngpio;
>  
> +#if defined(CONFIG_OF_GPIO)
> +	/**
> +	 * @of_gpio_n_cells:
> +	 *
> +	 * Number of cells used to form the GPIO specifier.
> +	 */
> +	unsigned int of_gpio_n_cells;
> +#endif
> +
>  	struct gpio_ops *ops;
>  
>  	struct list_head list;

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |