[PATCH] gpiolib: fix gpio-hog functionality

[PATCH] 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] gpiolib: fix gpio-hog functionality

[Sascha Hauer]

On Thu, Jun 01, 2023 at 04:25:06PM +0200, 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.

3641d381e63321016e3bf09504852a6b2a2f879b was introduced for sunxi. As
Jules is currently working on sunxi support this is likely needed soon,
so what's the second easiest way?

Sascha

> 
> 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
> 
> 
> 

-- 
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] gpiolib: fix gpio-hog functionality

[Marco Felsch]

On 23-06-01, Sascha Hauer wrote:
> On Thu, Jun 01, 2023 at 04:25:06PM +0200, 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.
> 
> 3641d381e63321016e3bf09504852a6b2a2f879b was introduced for sunxi. As
> Jules is currently working on sunxi support this is likely needed soon,
> so what's the second easiest way?

There is no sunxi driver within the kernel which make use of of_xlate.

Regards,
  Marco

> 
> Sascha
> 
> > 
> > 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
> > 
> > 
> > 
> 
> -- 
> 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] gpiolib: fix gpio-hog functionality

[Jules Maselbas]

On Thu, Jun 01, 2023 at 04:38:42PM +0200, Marco Felsch wrote:
> On 23-06-01, Sascha Hauer wrote:
> > On Thu, Jun 01, 2023 at 04:25:06PM +0200, 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.
> > 
> > 3641d381e63321016e3bf09504852a6b2a2f879b was introduced for sunxi. As
> > Jules is currently working on sunxi support this is likely needed soon,
> > so what's the second easiest way?
> 
> There is no sunxi driver within the kernel which make use of of_xlate.

of_xlate is used by sunxi pinctrl driver, see:
https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/sunxi/pinctrl-sunxi.c#L975

Best
-- Jules

Re: [PATCH] gpiolib: fix gpio-hog functionality

[Johannes Zink]

On 6/1/23 16:25, 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.
> 

Tested-by: Johannes Zink <j.zink@pengutronix.de>

> 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 */

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

Re: [PATCH] gpiolib: fix gpio-hog functionality

[Marco Felsch]

Hi Jules,

On 23-06-01, Jules Maselbas wrote:
> On Thu, Jun 01, 2023 at 04:38:42PM +0200, Marco Felsch wrote:
> > On 23-06-01, Sascha Hauer wrote:
> > > On Thu, Jun 01, 2023 at 04:25:06PM +0200, 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.
> > > 
> > > 3641d381e63321016e3bf09504852a6b2a2f879b was introduced for sunxi. As
> > > Jules is currently working on sunxi support this is likely needed soon,
> > > so what's the second easiest way?
> > 
> > There is no sunxi driver within the kernel which make use of of_xlate.
> 
> of_xlate is used by sunxi pinctrl driver, see:
> https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/sunxi/pinctrl-sunxi.c#L975

Thanks for the link, didn't searched the pinctrl driver dir..

Anyway I still think that we should (re-)sync the barebox gpio code with
linux. The sync can start with adding the of_xlate support like it is
done in linux? This was also my reason for completely revert your patch
since a gpiolib sync would adapt the of_xlate handling anyway.

Regards,
  Marco

Re: [PATCH] gpiolib: fix gpio-hog functionality

[Jules Maselbas]

On Thu, Jun 01, 2023 at 04:32:09PM +0200, Sascha Hauer wrote:
> On Thu, Jun 01, 2023 at 04:25:06PM +0200, 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.
> 
> 3641d381e63321016e3bf09504852a6b2a2f879b was introduced for sunxi. As
> Jules is currently working on sunxi support this is likely needed soon,
> so what's the second easiest way?
I am not very familiar with the gpio-hog concept, what I understand is:

"gpio-hog" are a description of gpios config to be initialized early on.
The main difference with classic gpio is the omission of the phandle of
the gpio controller (since gpio-hog are expected to be a child node of
gpio controller). Correct me if I am wrong.

In this case the current version of of_hog_gpio fails since it calls
of_parse_phandle_with_args which expect the gpio to start with the controller
phandle.

My best guess would be to rework of_hog_gpio to not use of_parse_phandle_with_args
and probably not gpio_of_xlate

This sounds easier than a sync of barebox gpio code with linux.

Cheers
-- Jules

Re: [PATCH] gpiolib: fix gpio-hog functionality

[Marco Felsch]

On 23-06-01, Jules Maselbas wrote:
> On Thu, Jun 01, 2023 at 04:32:09PM +0200, Sascha Hauer wrote:
> > On Thu, Jun 01, 2023 at 04:25:06PM +0200, 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.
> > 
> > 3641d381e63321016e3bf09504852a6b2a2f879b was introduced for sunxi. As
> > Jules is currently working on sunxi support this is likely needed soon,
> > so what's the second easiest way?
> I am not very familiar with the gpio-hog concept, what I understand is:
> 
> "gpio-hog" are a description of gpios config to be initialized early on.
> The main difference with classic gpio is the omission of the phandle of
> the gpio controller (since gpio-hog are expected to be a child node of
> gpio controller). Correct me if I am wrong.

Correct hogs are used if you have a static gpio e.g. a power-switch
which is never turned off.

> In this case the current version of of_hog_gpio fails since it calls
> of_parse_phandle_with_args which expect the gpio to start with the controller
> phandle.

Correct.

> My best guess would be to rework of_hog_gpio to not use of_parse_phandle_with_args
> and probably not gpio_of_xlate

This would be a partly revert of your commit.

> This sounds easier than a sync of barebox gpio code with linux.

You don't need to sync the complete code, let me check if I can prepare
a patch which adds the of_xlate support and is a bit more in sync with
the kernel. If this is not possible I go with the partly revert.

Regards,
  Marco

Re: [PATCH] gpiolib: fix gpio-hog functionality

[Marco Felsch]

Hi Jules,

On 23-06-01, Marco Felsch wrote:
> On 23-06-01, Jules Maselbas wrote:
> > This sounds easier than a sync of barebox gpio code with linux.
> 
> You don't need to sync the complete code, let me check if I can prepare
> a patch which adds the of_xlate support and is a bit more in sync with
> the kernel. If this is not possible I go with the partly revert.

please have a look on:

- https://lore.barebox.org/barebox/20230602074921.2687669-1-m.felsch@pengutronix.de/

I've added you to the cc as well. This is what I meant by having a
of_xlate implementation which is more like the kernel.

Regards,
  Marco

Re: [PATCH] gpiolib: fix gpio-hog functionality

[Jules Maselbas]

Hi Marco,

On Fri, Jun 02, 2023 at 09:54:48AM +0200, Marco Felsch wrote:
> Hi Jules,
> 
> On 23-06-01, Marco Felsch wrote:
> > On 23-06-01, Jules Maselbas wrote:
> > > This sounds easier than a sync of barebox gpio code with linux.
> > 
> > You don't need to sync the complete code, let me check if I can prepare
> > a patch which adds the of_xlate support and is a bit more in sync with
> > the kernel. If this is not possible I go with the partly revert.
> 
> please have a look on:
> 
> - https://lore.barebox.org/barebox/20230602074921.2687669-1-m.felsch@pengutronix.de/
> 
> I've added you to the cc as well. This is what I meant by having a
> of_xlate implementation which is more like the kernel.

Thanks, I will test this when possible (might be this weekend).
 
Cheers