| Message ID | 20220908053949.3564796-3-dmitry.torokhov@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/4] gpiolib: of: do not ignore requested index when applying quirks | expand |
On Thu, Sep 8, 2022 at 7:39 AM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > 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> That's a change I wanted to do but didn't get around to, thanks a lot! Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
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 >
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
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 --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;
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> --- drivers/gpio/gpiolib-of.c | 62 ++++++++++++++++----------------------- 1 file changed, 26 insertions(+), 36 deletions(-)