diff mbox series

[1/4] gpiolib: of: do not ignore requested index when applying quirks

Message ID 20220908053949.3564796-1-dmitry.torokhov@gmail.com
State Accepted
Commit 98c3c940ea5c3957056717e8b77a91c7d94536ad
Headers show
Series [1/4] gpiolib: of: do not ignore requested index when applying quirks | expand

Commit Message

Dmitry Torokhov Sept. 8, 2022, 5:39 a.m. UTC
We should not ignore index passed into of_find_gpio() when handling
quirks. While in practice this change will not have any effect, it
will allow consolidate quirk handling.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/gpio/gpiolib-of.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

Comments

Bartosz Golaszewski Sept. 15, 2022, 8:37 a.m. UTC | #1
On Thu, Sep 8, 2022 at 7:40 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> We should not ignore index passed into of_find_gpio() when handling
> quirks. While in practice this change will not have any effect, it
> will allow consolidate quirk handling.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---

Applied the entire series. Thanks!

Bart
Conor Dooley Sept. 16, 2022, 11:03 a.m. UTC | #2
On 08/09/2022 06:39, Dmitry Torokhov wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Instead of having a string of "if" statements let's put all quirks into
> an array and iterate over them.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Hey,
This seems to have landed in -next today as a2b5e207cade which has
unfortunately broken boot for me:

[    0.765969] Unable to handle kernel paging request at virtual address ffffffad6f697066
[    0.774948] Oops [#1]
[    0.777491] Modules linked in:
[    0.780896] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.0.0-rc1-00037-ga2b5e207cade #1
[    0.789668] Hardware name: Microchip PolarFire-SoC Icicle Kit (DT)
[    0.796512] epc : 0xffffffad6f697066
[    0.800491]  ra : of_find_gpio+0x12c/0x1fa
[    0.805066] epc : ffffffad6f697066 ra : ffffffff8042032c sp : ffffffc80400b4e0
[    0.813062]  gp : ffffffff810ef490 tp : ffffffe77fe68000 t0 : 0000000400000000
[    0.821063]  t1 : 0000001000000000 t2 : 0000000000000010 s0 : ffffffc80400b560
[    0.829058]  s1 : ffffffff80c52a88 a0 : ffffffe7bfdf37b0 a1 : ffffffff80d5320e
[    0.837053]  a2 : 0000000000000000 a3 : ffffffc80400b4e4 a4 : 6e61722d6f697067
[    0.845057]  a5 : 0000000000000000 a6 : ffffffff80c4e760 a7 : ffffffe77ffb5b73
[    0.853048]  s2 : ffffffc80400b588 s3 : 0000000000000000 s4 : ffffffe77ffad810
[    0.861052]  s5 : ffffffff810f3098 s6 : ffffffff80d5320e s7 : fffffffffffffffe
[    0.869048]  s8 : fffffffffffff000 s9 : 0000000000000003 s10: 0000000000000000
[    0.877050]  s11: ffffffff80d5320e t3 : 0000000200000000 t4 : ffffffff80c4ebe8
[    0.885045]  t5 : ffffffff80d4ce2e t6 : ffffffe77ffb5b72
[    0.890929] status: 0000000200000120 badaddr: ffffffad6f697066 cause: 000000000000000c
[    0.900316] ---[ end trace 0000000000000000 ]---
[    0.905544] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
[    0.914024] SMP: stopping secondary CPUs
[    0.918394] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b ]---

In case it is useful to you, I have gpio nodes in my devicetree
but I am not building a driver that binds to those nodes. Since I
don't have a driver, that's pretty much all of the relevant info
from the boot log. Anything else you need, lmk and I will try to
provide :)

Thanks,
Conor.


> ---
>   drivers/gpio/gpiolib-of.c | 62 ++++++++++++++++-----------------------
>   1 file changed, 26 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 30b89b694530..097e948c1d49 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -372,14 +372,12 @@ EXPORT_SYMBOL_GPL(gpiod_get_from_of_node);
>    * properties should be named "foo-gpios" so we have this special kludge for
>    * them.
>    */
> -static struct gpio_desc *of_find_spi_gpio(struct device *dev,
> +static struct gpio_desc *of_find_spi_gpio(struct device_node *np,
>                                            const char *con_id,
>                                            unsigned int idx,
>                                            enum of_gpio_flags *of_flags)
>   {
>          char prop_name[32]; /* 32 is max size of property name */
> -       const struct device_node *np = dev->of_node;
> -       struct gpio_desc *desc;
> 
>          /*
>           * Hopefully the compiler stubs the rest of the function if this
> @@ -395,8 +393,7 @@ static struct gpio_desc *of_find_spi_gpio(struct device *dev,
>          /* Will be "gpio-sck", "gpio-mosi" or "gpio-miso" */
>          snprintf(prop_name, sizeof(prop_name), "%s-%s", "gpio", con_id);
> 
> -       desc = of_get_named_gpiod_flags(np, prop_name, idx, of_flags);
> -       return desc;
> +       return of_get_named_gpiod_flags(np, prop_name, idx, of_flags);
>   }
> 
>   /*
> @@ -404,13 +401,11 @@ static struct gpio_desc *of_find_spi_gpio(struct device *dev,
>    * lines rather than "cs-gpios" like all other SPI hardware. Account for this
>    * with a special quirk.
>    */
> -static struct gpio_desc *of_find_spi_cs_gpio(struct device *dev,
> +static struct gpio_desc *of_find_spi_cs_gpio(struct device_node *np,
>                                               const char *con_id,
>                                               unsigned int idx,
>                                               enum of_gpio_flags *of_flags)
>   {
> -       const struct device_node *np = dev->of_node;
> -
>          if (!IS_ENABLED(CONFIG_SPI_MASTER))
>                  return ERR_PTR(-ENOENT);
> 
> @@ -428,7 +423,7 @@ static struct gpio_desc *of_find_spi_cs_gpio(struct device *dev,
>           * uses just "gpios" so translate to that when "cs-gpios" is
>           * requested.
>           */
> -       return of_get_named_gpiod_flags(dev->of_node, "gpios", idx, of_flags);
> +       return of_get_named_gpiod_flags(np, "gpios", idx, of_flags);
>   }
> 
>   /*
> @@ -436,7 +431,7 @@ static struct gpio_desc *of_find_spi_cs_gpio(struct device *dev,
>    * properties should be named "foo-gpios" so we have this special kludge for
>    * them.
>    */
> -static struct gpio_desc *of_find_regulator_gpio(struct device *dev,
> +static struct gpio_desc *of_find_regulator_gpio(struct device_node *np,
>                                                  const char *con_id,
>                                                  unsigned int idx,
>                                                  enum of_gpio_flags *of_flags)
> @@ -447,8 +442,6 @@ static struct gpio_desc *of_find_regulator_gpio(struct device *dev,
>                  "wlf,ldo1ena", /* WM8994 */
>                  "wlf,ldo2ena", /* WM8994 */
>          };
> -       const struct device_node *np = dev->of_node;
> -       struct gpio_desc *desc;
>          int i;
> 
>          if (!IS_ENABLED(CONFIG_REGULATOR))
> @@ -461,11 +454,10 @@ static struct gpio_desc *of_find_regulator_gpio(struct device *dev,
>          if (i < 0)
>                  return ERR_PTR(-ENOENT);
> 
> -       desc = of_get_named_gpiod_flags(np, con_id, idx, of_flags);
> -       return desc;
> +       return of_get_named_gpiod_flags(np, con_id, idx, of_flags);
>   }
> 
> -static struct gpio_desc *of_find_arizona_gpio(struct device *dev,
> +static struct gpio_desc *of_find_arizona_gpio(struct device_node *np,
>                                                const char *con_id,
>                                                unsigned int idx,
>                                                enum of_gpio_flags *of_flags)
> @@ -476,10 +468,10 @@ static struct gpio_desc *of_find_arizona_gpio(struct device *dev,
>          if (!con_id || strcmp(con_id, "wlf,reset"))
>                  return ERR_PTR(-ENOENT);
> 
> -       return of_get_named_gpiod_flags(dev->of_node, con_id, idx, of_flags);
> +       return of_get_named_gpiod_flags(np, con_id, idx, of_flags);
>   }
> 
> -static struct gpio_desc *of_find_usb_gpio(struct device *dev,
> +static struct gpio_desc *of_find_usb_gpio(struct device_node *np,
>                                            const char *con_id,
>                                            unsigned int idx,
>                                            enum of_gpio_flags *of_flags)
> @@ -495,14 +487,27 @@ static struct gpio_desc *of_find_usb_gpio(struct device *dev,
>          if (!con_id || strcmp(con_id, "fcs,int_n"))
>                  return ERR_PTR(-ENOENT);
> 
> -       return of_get_named_gpiod_flags(dev->of_node, con_id, idx, of_flags);
> +       return of_get_named_gpiod_flags(np, con_id, idx, of_flags);
>   }
> 
> +typedef struct gpio_desc *(*of_find_gpio_quirk)(struct device_node *np,
> +                                               const char *con_id,
> +                                               unsigned int idx,
> +                                               enum of_gpio_flags *of_flags);
> +static const of_find_gpio_quirk of_find_gpio_quirks[] = {
> +       of_find_spi_gpio,
> +       of_find_spi_cs_gpio,
> +       of_find_regulator_gpio,
> +       of_find_arizona_gpio,
> +       of_find_usb_gpio,
> +};
> +
>   struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
>                                 unsigned int idx, unsigned long *flags)
>   {
>          char prop_name[32]; /* 32 is max size of property name */
>          enum of_gpio_flags of_flags;
> +       const of_find_gpio_quirk *q;
>          struct gpio_desc *desc;
>          unsigned int i;
> 
> @@ -522,24 +527,9 @@ struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
>                          break;
>          }
> 
> -       if (gpiod_not_found(desc)) {
> -               /* Special handling for SPI GPIOs if used */
> -               desc = of_find_spi_gpio(dev, con_id, idx, &of_flags);
> -       }
> -
> -       if (gpiod_not_found(desc))
> -               desc = of_find_spi_cs_gpio(dev, con_id, idx, &of_flags);
> -
> -       if (gpiod_not_found(desc)) {
> -               /* Special handling for regulator GPIOs if used */
> -               desc = of_find_regulator_gpio(dev, con_id, idx, &of_flags);
> -       }
> -
> -       if (gpiod_not_found(desc))
> -               desc = of_find_arizona_gpio(dev, con_id, idx, &of_flags);
> -
> -       if (gpiod_not_found(desc))
> -               desc = of_find_usb_gpio(dev, con_id, idx, &of_flags);
> +       /* Properly named GPIO was not found, try workarounds */
> +       for (q = of_find_gpio_quirks; gpiod_not_found(desc) && *q; q++)
> +               desc = (*q)(dev->of_node, con_id, idx, &of_flags);
> 
>          if (IS_ERR(desc))
>                  return desc;
> --
> 2.37.2.789.g6183377224-goog
>
Linus Walleij Sept. 16, 2022, 11:20 a.m. UTC | #3
On Fri, Sep 16, 2022 at 1:04 PM <Conor.Dooley@microchip.com> wrote:
>
> On 08/09/2022 06:39, Dmitry Torokhov wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > Instead of having a string of "if" statements let's put all quirks into
> > an array and iterate over them.
> >
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> Hey,
> This seems to have landed in -next today as a2b5e207cade which has
> unfortunately broken boot for me:

Michael Walle has sent a patch fixing it:
https://lore.kernel.org/linux-gpio/20220916110118.446132-1-michael@walle.cc/

Yours,
Linus Walleij
Conor Dooley Sept. 16, 2022, 11:25 a.m. UTC | #4
On 16/09/2022 12:20, Linus Walleij wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Fri, Sep 16, 2022 at 1:04 PM <Conor.Dooley@microchip.com> wrote:
>>
>> On 08/09/2022 06:39, Dmitry Torokhov wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> Instead of having a string of "if" statements let's put all quirks into
>>> an array and iterate over them.
>>>
>>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>
>> Hey,
>> This seems to have landed in -next today as a2b5e207cade which has
>> unfortunately broken boot for me:
> 
> Michael Walle has sent a patch fixing it:
> https://lore.kernel.org/linux-gpio/20220916110118.446132-1-michael@walle.cc/

Ohh great, I checked before I started writing my mail but didn't
see anything, which makes sense given the times..
diff mbox series

Patch

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index a037b50bef33..c08564948bf9 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -372,7 +372,9 @@  EXPORT_SYMBOL_GPL(gpiod_get_from_of_node);
  * properties should be named "foo-gpios" so we have this special kludge for
  * them.
  */
-static struct gpio_desc *of_find_spi_gpio(struct device *dev, const char *con_id,
+static struct gpio_desc *of_find_spi_gpio(struct device *dev,
+					  const char *con_id,
+					  unsigned int idx,
 					  enum of_gpio_flags *of_flags)
 {
 	char prop_name[32]; /* 32 is max size of property name */
@@ -393,7 +395,7 @@  static struct gpio_desc *of_find_spi_gpio(struct device *dev, const char *con_id
 	/* Will be "gpio-sck", "gpio-mosi" or "gpio-miso" */
 	snprintf(prop_name, sizeof(prop_name), "%s-%s", "gpio", con_id);
 
-	desc = of_get_named_gpiod_flags(np, prop_name, 0, of_flags);
+	desc = of_get_named_gpiod_flags(np, prop_name, idx, of_flags);
 	return desc;
 }
 
@@ -434,7 +436,9 @@  static struct gpio_desc *of_find_spi_cs_gpio(struct device *dev,
  * properties should be named "foo-gpios" so we have this special kludge for
  * them.
  */
-static struct gpio_desc *of_find_regulator_gpio(struct device *dev, const char *con_id,
+static struct gpio_desc *of_find_regulator_gpio(struct device *dev,
+						const char *con_id,
+						unsigned int idx,
 						enum of_gpio_flags *of_flags)
 {
 	/* These are the connection IDs we accept as legacy GPIO phandles */
@@ -457,12 +461,13 @@  static struct gpio_desc *of_find_regulator_gpio(struct device *dev, const char *
 	if (i < 0)
 		return ERR_PTR(-ENOENT);
 
-	desc = of_get_named_gpiod_flags(np, con_id, 0, of_flags);
+	desc = of_get_named_gpiod_flags(np, con_id, idx, of_flags);
 	return desc;
 }
 
 static struct gpio_desc *of_find_arizona_gpio(struct device *dev,
 					      const char *con_id,
+					      unsigned int idx,
 					      enum of_gpio_flags *of_flags)
 {
 	if (!IS_ENABLED(CONFIG_MFD_ARIZONA))
@@ -471,17 +476,18 @@  static struct gpio_desc *of_find_arizona_gpio(struct device *dev,
 	if (!con_id || strcmp(con_id, "wlf,reset"))
 		return ERR_PTR(-ENOENT);
 
-	return of_get_named_gpiod_flags(dev->of_node, con_id, 0, of_flags);
+	return of_get_named_gpiod_flags(dev->of_node, con_id, idx, of_flags);
 }
 
 static struct gpio_desc *of_find_usb_gpio(struct device *dev,
 					  const char *con_id,
+					  unsigned int idx,
 					  enum of_gpio_flags *of_flags)
 {
 	/*
-	 * Currently this USB quirk is only for the Fairchild FUSB302 host which is using
-	 * an undocumented DT GPIO line named "fcs,int_n" without the compulsory "-gpios"
-	 * suffix.
+	 * Currently this USB quirk is only for the Fairchild FUSB302 host
+	 * which is using an undocumented DT GPIO line named "fcs,int_n"
+	 * without the compulsory "-gpios" suffix.
 	 */
 	if (!IS_ENABLED(CONFIG_TYPEC_FUSB302))
 		return ERR_PTR(-ENOENT);
@@ -489,7 +495,7 @@  static struct gpio_desc *of_find_usb_gpio(struct device *dev,
 	if (!con_id || strcmp(con_id, "fcs,int_n"))
 		return ERR_PTR(-ENOENT);
 
-	return of_get_named_gpiod_flags(dev->of_node, con_id, 0, of_flags);
+	return of_get_named_gpiod_flags(dev->of_node, con_id, idx, of_flags);
 }
 
 struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
@@ -518,7 +524,7 @@  struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
 
 	if (gpiod_not_found(desc)) {
 		/* Special handling for SPI GPIOs if used */
-		desc = of_find_spi_gpio(dev, con_id, &of_flags);
+		desc = of_find_spi_gpio(dev, con_id, idx, &of_flags);
 	}
 
 	if (gpiod_not_found(desc)) {
@@ -530,14 +536,14 @@  struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
 
 	if (gpiod_not_found(desc)) {
 		/* Special handling for regulator GPIOs if used */
-		desc = of_find_regulator_gpio(dev, con_id, &of_flags);
+		desc = of_find_regulator_gpio(dev, con_id, idx, &of_flags);
 	}
 
 	if (gpiod_not_found(desc))
-		desc = of_find_arizona_gpio(dev, con_id, &of_flags);
+		desc = of_find_arizona_gpio(dev, con_id, idx, &of_flags);
 
 	if (gpiod_not_found(desc))
-		desc = of_find_usb_gpio(dev, con_id, &of_flags);
+		desc = of_find_usb_gpio(dev, con_id, idx, &of_flags);
 
 	if (IS_ERR(desc))
 		return desc;