| Message ID | 20221011-gpiolib-quirks-v1-2-e01d9d3e7b29@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpiolib: more quirks to handle legacy names | expand |
On Tue, Oct 11, 2022 at 03:19:30PM -0700, Dmitry Torokhov wrote: > This consolidates all quirks doing simple renames (either allowing > suffix-less names or trivial renames, when index changes are not > required) into a single quirk. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > drivers/gpio/gpiolib-of.c | 176 +++++++++++++++++----------------------------- > 1 file changed, 64 insertions(+), 112 deletions(-) Nice diffstat, almost a shame that the diff algo itself has latched onto spurious anchor points to generate something that is so hard to read ;-) . I've reviewed this pretty closely and AFAICT it does exactly what the preivous code does. Thus the comments below are all related to things that the new table makes obvious that the previous code handled in a rather inconsistent way. Maybe that means these could/should be fixed in an extra patch within this patch set. I guess that means, despite the feedback below, *this* patch is: Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org> > diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c > index cef4f6634125..619aae0c5476 100644 > @@ -365,127 +365,83 @@ struct gpio_desc *gpiod_get_from_of_node(const struct device_node *node, > +static struct gpio_desc *of_find_gpio_rename(struct device_node *np, > const char *con_id, > unsigned int idx, > enum of_gpio_flags *of_flags) > { > + static const struct of_rename_gpio { > + const char *con_id; > + const char *legacy_id; /* NULL - same as con_id */ > + const char *compatible; /* NULL - don't check */ "don't check" doesn't seem desirable. It's not too big a deal here because everything affected has a vendor prefix (meaning incorrect matching is unlikely). Should there be a comment about the general care needed for a NULL compatible? > + } gpios[] = { > +#if IS_ENABLED(CONFIG_MFD_ARIZONA) > + { "wlf,reset", NULL, NULL }, CONFIG_REGULATOR_ARIZONA_LDO1 is better guard for this con id. > +#endif > +#if IS_ENABLED(CONFIG_REGULATOR) > + /* > + * Some regulator bindings happened before we managed to > + * establish that GPIO properties should be named > + * "foo-gpios" so we have this special kludge for them. > + */ > + { "wlf,ldoena", NULL, NULL }, /* Arizona */ CONFIG_REGULATOR_ARIZONA_LDO1 is better for this one too. > + { "wlf,ldo1ena", NULL, NULL }, /* WM8994 */ > + { "wlf,ldo2ena", NULL, NULL }, /* WM8994 */ CONFIG_REGULATOR_WM8994 is a better guard for these. > +#endif > +#if IS_ENABLED(CONFIG_SPI_MASTER) > + /* > + * The SPI GPIO bindings happened before we managed to > + * establish that GPIO properties should be named > + * "foo-gpios" so we have this special kludge for them. > + */ > + { "miso", "gpio-miso", "spi-gpio" }, > + { "mosi", "gpio-mosi", "spi-gpio" }, > + { "sck", "gpio-sck", "spi-gpio" }, CONFIG_SPI_GPIO is a better guard for these. > > + /* > + * The old Freescale bindings use simply "gpios" as name > + * for the chip select lines rather than "cs-gpios" like > + * all other SPI hardware. Allow this specifically for > + * Freescale and PPC devices. > + */ > + { "cs", "gpios", "fsl,spi" }, > + { "cs", "gpios", "aeroflexgaisler,spictrl" }, CONFIG_SPI_FSL_SPI for these. > + { "cs", "gpios", "ibm,ppc4xx-spi" }, CONFIG_SPI_PPC4xx for this. > +#endif > +#if IS_ENABLED(CONFIG_TYPEC_FUSB302) > + /* > + * Fairchild FUSB302 host is using undocumented "fcs,int_n" > + * property without the compulsory "-gpios" suffix. > + */ > + { "fcs,int_n", NULL, "fcs,fusb302" }, > +#endif > }; > + struct gpio_desc *desc; > + const char *legacy_id; > + unsigned int i; > > if (!con_id) > return ERR_PTR(-ENOENT); > > + for (i = 0; i < ARRAY_SIZE(gpios); i++) { > + if (strcmp(con_id, gpios[i].con_id)) > + continue; > > + if (gpios[i].compatible && > + !of_device_is_compatible(np, gpios[i].compatible)) > + continue; > > + legacy_id = gpios[i].legacy_id ?: gpios[i].con_id; > + desc = of_get_named_gpiod_flags(np, legacy_id, idx, of_flags); > + if (!gpiod_not_found(desc)) { > + pr_info("%s uses legacy gpio name '%s' instead of '%s-gpios'\n", > + of_node_full_name(np), legacy_id, con_id); > + return desc; > + } > + } > > + return ERR_PTR(-ENOENT); > } I would normally trim last but this but given what git did to this particular patch I left it as a public service ;-) (it has the - parts of the patch removed). Daniel.
On Wed, Oct 12, 2022 at 11:12:03AM +0100, Daniel Thompson wrote: > On Tue, Oct 11, 2022 at 03:19:30PM -0700, Dmitry Torokhov wrote: > > This consolidates all quirks doing simple renames (either allowing > > suffix-less names or trivial renames, when index changes are not > > required) into a single quirk. > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > --- > > drivers/gpio/gpiolib-of.c | 176 +++++++++++++++++----------------------------- > > 1 file changed, 64 insertions(+), 112 deletions(-) > > Nice diffstat, almost a shame that the diff algo itself has latched onto > spurious anchor points to generate something that is so hard to read > ;-) . > > I've reviewed this pretty closely and AFAICT it does exactly what the > preivous code does. Thus the comments below are all related to things > that the new table makes obvious that the previous code handled in a > rather inconsistent way. Maybe that means these could/should be fixed > in an extra patch within this patch set. > > I guess that means, despite the feedback below, *this* patch is: > Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org> > > > > diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c > > index cef4f6634125..619aae0c5476 100644 > > @@ -365,127 +365,83 @@ struct gpio_desc *gpiod_get_from_of_node(const struct device_node *node, > > +static struct gpio_desc *of_find_gpio_rename(struct device_node *np, > > const char *con_id, > > unsigned int idx, > > enum of_gpio_flags *of_flags) > > { > > + static const struct of_rename_gpio { > > + const char *con_id; > > + const char *legacy_id; /* NULL - same as con_id */ > > + const char *compatible; /* NULL - don't check */ > > "don't check" doesn't seem desirable. It's not too big a deal here > because everything affected has a vendor prefix (meaning incorrect > matching is unlikely). Should there be a comment about the general care > needed for a NULL compatible? I'll add the wording that NULL is only acceptable if property has a vendor prefi, Will that be OK? Otherwise I'll have to add a lot of entries for Arizona and Madera. > > > > + } gpios[] = { > > +#if IS_ENABLED(CONFIG_MFD_ARIZONA) > > + { "wlf,reset", NULL, NULL }, > > CONFIG_REGULATOR_ARIZONA_LDO1 is better guard for this con id. Are you sure? I see reset handling happening in drivers/mfd/arizona-core.c independently of regulator code... > > > > +#endif > > +#if IS_ENABLED(CONFIG_REGULATOR) > > + /* > > + * Some regulator bindings happened before we managed to > > + * establish that GPIO properties should be named > > + * "foo-gpios" so we have this special kludge for them. > > + */ > > + { "wlf,ldoena", NULL, NULL }, /* Arizona */ > > CONFIG_REGULATOR_ARIZONA_LDO1 is better for this one too. Good idea. > > > > + { "wlf,ldo1ena", NULL, NULL }, /* WM8994 */ > > + { "wlf,ldo2ena", NULL, NULL }, /* WM8994 */ > > CONFIG_REGULATOR_WM8994 is a better guard for these. Yep. > > > > +#endif > > +#if IS_ENABLED(CONFIG_SPI_MASTER) > > + /* > > + * The SPI GPIO bindings happened before we managed to > > + * establish that GPIO properties should be named > > + * "foo-gpios" so we have this special kludge for them. > > + */ > > + { "miso", "gpio-miso", "spi-gpio" }, > > + { "mosi", "gpio-mosi", "spi-gpio" }, > > + { "sck", "gpio-sck", "spi-gpio" }, > > CONFIG_SPI_GPIO is a better guard for these. OK. > > > > > > + /* > > + * The old Freescale bindings use simply "gpios" as name > > + * for the chip select lines rather than "cs-gpios" like > > + * all other SPI hardware. Allow this specifically for > > + * Freescale and PPC devices. > > + */ > > + { "cs", "gpios", "fsl,spi" }, > > + { "cs", "gpios", "aeroflexgaisler,spictrl" }, > > CONFIG_SPI_FSL_SPI for these. OK. > > > + { "cs", "gpios", "ibm,ppc4xx-spi" }, > > CONFIG_SPI_PPC4xx for this. OK. > > > > +#endif > > +#if IS_ENABLED(CONFIG_TYPEC_FUSB302) > > + /* > > + * Fairchild FUSB302 host is using undocumented "fcs,int_n" > > + * property without the compulsory "-gpios" suffix. > > + */ > > + { "fcs,int_n", NULL, "fcs,fusb302" }, > > +#endif > > }; > > + struct gpio_desc *desc; > > + const char *legacy_id; > > + unsigned int i; > > > > if (!con_id) > > return ERR_PTR(-ENOENT); > > > > + for (i = 0; i < ARRAY_SIZE(gpios); i++) { > > + if (strcmp(con_id, gpios[i].con_id)) > > + continue; > > > > + if (gpios[i].compatible && > > + !of_device_is_compatible(np, gpios[i].compatible)) > > + continue; > > > > + legacy_id = gpios[i].legacy_id ?: gpios[i].con_id; > > + desc = of_get_named_gpiod_flags(np, legacy_id, idx, of_flags); > > + if (!gpiod_not_found(desc)) { > > + pr_info("%s uses legacy gpio name '%s' instead of '%s-gpios'\n", > > + of_node_full_name(np), legacy_id, con_id); > > + return desc; > > + } > > + } > > > > + return ERR_PTR(-ENOENT); > > } > > I would normally trim last but this but given what git did to this particular > patch I left it as a public service ;-) (it has the - parts of the > patch removed). > > > Daniel. Thanks.
On Wed, Oct 12, 2022 at 12:20:50PM -0700, Dmitry Torokhov wrote: > On Wed, Oct 12, 2022 at 11:12:03AM +0100, Daniel Thompson wrote: > > On Tue, Oct 11, 2022 at 03:19:30PM -0700, Dmitry Torokhov wrote: > > > diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c > > > index cef4f6634125..619aae0c5476 100644 > > > @@ -365,127 +365,83 @@ struct gpio_desc *gpiod_get_from_of_node(const struct device_node *node, > > > +static struct gpio_desc *of_find_gpio_rename(struct device_node *np, > > > const char *con_id, > > > unsigned int idx, > > > enum of_gpio_flags *of_flags) > > > { > > > + static const struct of_rename_gpio { > > > + const char *con_id; > > > + const char *legacy_id; /* NULL - same as con_id */ > > > + const char *compatible; /* NULL - don't check */ > > > > "don't check" doesn't seem desirable. It's not too big a deal here > > because everything affected has a vendor prefix (meaning incorrect > > matching is unlikely). Should there be a comment about the general care > > needed for a NULL compatible? There were certainly a lot of compatibles affected by this translation and given the structure of the drivers it is a tough code review to be sure you have picked up *all* of them! > I'll add the wording that NULL is only acceptable if property has a > vendor prefi, Will that be OK? Otherwise I'll have to add a lot of > entries for Arizona and Madera. > > > > > > > > + } gpios[] = { > > > +#if IS_ENABLED(CONFIG_MFD_ARIZONA) > > > + { "wlf,reset", NULL, NULL }, > > > > CONFIG_REGULATOR_ARIZONA_LDO1 is better guard for this con id. > > Are you sure? I see reset handling happening in > drivers/mfd/arizona-core.c independently of regulator code... Looks like I grepped for the wrong string so I was completely wrong here... and in two different ways! Firstly I'm wrong about replacing the guard. Existing guard is correct! Secondly, I didn't notice until now that wm8804 also uses the "wlf,reset" and it is a little odd that the wm8804 driver will accept or refuse a misspelled binding based whether the kernel has enabled the arizona drivers. Overall I can live with the code we have today but this makes me wonder if the comment discussed above should be stronger. Something like: "the NULL compatible code is used there to support legacy entries in the table; try to avoid adding new NULL entries". Daniel.
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c index cef4f6634125..619aae0c5476 100644 --- a/drivers/gpio/gpiolib-of.c +++ b/drivers/gpio/gpiolib-of.c @@ -365,127 +365,83 @@ struct gpio_desc *gpiod_get_from_of_node(const struct device_node *node, } EXPORT_SYMBOL_GPL(gpiod_get_from_of_node); -/* - * The SPI GPIO bindings happened before we managed to establish that GPIO - * properties should be named "foo-gpios" so we have this special kludge for - * them. - */ -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 */ - - /* - * Hopefully the compiler stubs the rest of the function if this - * is false. - */ - if (!IS_ENABLED(CONFIG_SPI_MASTER)) - return ERR_PTR(-ENOENT); - - /* Allow this specifically for "spi-gpio" devices */ - if (!of_device_is_compatible(np, "spi-gpio") || !con_id) - return ERR_PTR(-ENOENT); - - /* Will be "gpio-sck", "gpio-mosi" or "gpio-miso" */ - snprintf(prop_name, sizeof(prop_name), "%s-%s", "gpio", con_id); - - return of_get_named_gpiod_flags(np, prop_name, idx, of_flags); -} - -/* - * The old Freescale bindings use simply "gpios" as name for the chip select - * 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_node *np, +static struct gpio_desc *of_find_gpio_rename(struct device_node *np, const char *con_id, unsigned int idx, enum of_gpio_flags *of_flags) { - if (!IS_ENABLED(CONFIG_SPI_MASTER)) - return ERR_PTR(-ENOENT); - - /* Allow this specifically for Freescale and PPC devices */ - if (!of_device_is_compatible(np, "fsl,spi") && - !of_device_is_compatible(np, "aeroflexgaisler,spictrl") && - !of_device_is_compatible(np, "ibm,ppc4xx-spi")) - return ERR_PTR(-ENOENT); - /* Allow only if asking for "cs-gpios" */ - if (!con_id || strcmp(con_id, "cs")) - return ERR_PTR(-ENOENT); + static const struct of_rename_gpio { + const char *con_id; + const char *legacy_id; /* NULL - same as con_id */ + const char *compatible; /* NULL - don't check */ + } gpios[] = { +#if IS_ENABLED(CONFIG_MFD_ARIZONA) + { "wlf,reset", NULL, NULL }, +#endif +#if IS_ENABLED(CONFIG_REGULATOR) + /* + * Some regulator bindings happened before we managed to + * establish that GPIO properties should be named + * "foo-gpios" so we have this special kludge for them. + */ + { "wlf,ldoena", NULL, NULL }, /* Arizona */ + { "wlf,ldo1ena", NULL, NULL }, /* WM8994 */ + { "wlf,ldo2ena", NULL, NULL }, /* WM8994 */ +#endif +#if IS_ENABLED(CONFIG_SPI_MASTER) - /* - * While all other SPI controllers use "cs-gpios" the Freescale - * uses just "gpios" so translate to that when "cs-gpios" is - * requested. - */ - return of_get_named_gpiod_flags(np, "gpios", idx, of_flags); -} + /* + * The SPI GPIO bindings happened before we managed to + * establish that GPIO properties should be named + * "foo-gpios" so we have this special kludge for them. + */ + { "miso", "gpio-miso", "spi-gpio" }, + { "mosi", "gpio-mosi", "spi-gpio" }, + { "sck", "gpio-sck", "spi-gpio" }, -/* - * Some regulator bindings happened before we managed to establish that GPIO - * properties should be named "foo-gpios" so we have this special kludge for - * them. - */ -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) -{ - /* These are the connection IDs we accept as legacy GPIO phandles */ - const char *whitelist[] = { - "wlf,ldoena", /* Arizona */ - "wlf,ldo1ena", /* WM8994 */ - "wlf,ldo2ena", /* WM8994 */ + /* + * The old Freescale bindings use simply "gpios" as name + * for the chip select lines rather than "cs-gpios" like + * all other SPI hardware. Allow this specifically for + * Freescale and PPC devices. + */ + { "cs", "gpios", "fsl,spi" }, + { "cs", "gpios", "aeroflexgaisler,spictrl" }, + { "cs", "gpios", "ibm,ppc4xx-spi" }, +#endif +#if IS_ENABLED(CONFIG_TYPEC_FUSB302) + /* + * Fairchild FUSB302 host is using undocumented "fcs,int_n" + * property without the compulsory "-gpios" suffix. + */ + { "fcs,int_n", NULL, "fcs,fusb302" }, +#endif }; - int i; - - if (!IS_ENABLED(CONFIG_REGULATOR)) - return ERR_PTR(-ENOENT); + struct gpio_desc *desc; + const char *legacy_id; + unsigned int i; if (!con_id) return ERR_PTR(-ENOENT); - i = match_string(whitelist, ARRAY_SIZE(whitelist), con_id); - if (i < 0) - return ERR_PTR(-ENOENT); - - return of_get_named_gpiod_flags(np, con_id, idx, of_flags); -} - -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) -{ - if (!IS_ENABLED(CONFIG_MFD_ARIZONA)) - return ERR_PTR(-ENOENT); - - if (!con_id || strcmp(con_id, "wlf,reset")) - return ERR_PTR(-ENOENT); - - return of_get_named_gpiod_flags(np, con_id, idx, of_flags); -} + for (i = 0; i < ARRAY_SIZE(gpios); i++) { + if (strcmp(con_id, gpios[i].con_id)) + continue; -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) -{ - /* - * 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); + if (gpios[i].compatible && + !of_device_is_compatible(np, gpios[i].compatible)) + continue; - if (!con_id || strcmp(con_id, "fcs,int_n")) - return ERR_PTR(-ENOENT); + legacy_id = gpios[i].legacy_id ?: gpios[i].con_id; + desc = of_get_named_gpiod_flags(np, legacy_id, idx, of_flags); + if (!gpiod_not_found(desc)) { + pr_info("%s uses legacy gpio name '%s' instead of '%s-gpios'\n", + of_node_full_name(np), legacy_id, con_id); + return desc; + } + } - return of_get_named_gpiod_flags(np, con_id, idx, of_flags); + return ERR_PTR(-ENOENT); } static struct gpio_desc *of_find_mt2701_gpio(struct device_node *np, @@ -525,11 +481,7 @@ typedef struct gpio_desc *(*of_find_gpio_quirk)(struct device_node *np, 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, + of_find_gpio_rename, of_find_mt2701_gpio, NULL };
This consolidates all quirks doing simple renames (either allowing suffix-less names or trivial renames, when index changes are not required) into a single quirk. Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/gpio/gpiolib-of.c | 176 +++++++++++++++++----------------------------- 1 file changed, 64 insertions(+), 112 deletions(-)