In preparation to converting several drivers to gpiod API, and to keep
existing DTS working, this series adds additional quirks to locate
gpio lines with legacy names.
Additionally the quirk handling has been reworked (once again) to pull
all simple renames (ones that do not involve change of indices or other
complex manipulations) into a single quirk with a table containing
transformations. This should make adding new quirks easier.
When using legacy names gpiolib will emit a message to nudge users to
update DTSes (when possible).
Note that the last patch requires the following change from the OF tree:
88269151be67 ("of: base: make of_device_compatible_match() accept const device node")
The change is also available in mainline - it has been merged in 6.1
merge window.
Thanks.
To: Linus Walleij <[email protected]>
To: Bartosz Golaszewski <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: Daniel Thompson <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
Dmitry Torokhov (7):
gpiolib: of: add a quirk for legacy names in Mediatek mt2701-cs42448
gpiolib: of: consolidate simple renames into a single quirk
gpiolib: of: add quirk for locating reset lines with legacy bindings
gpiolib: of: add a quirk for reset line for Marvell NFC controller
gpiolib: of: add a quirk for reset line for Cirrus CS42L56 codec
gpiolib: of: factor out code overriding gpio line polarity
gpiolib: of: add quirk for phy reset polarity for Freescale Ethernet
drivers/gpio/gpiolib-of.c | 294 +++++++++++++++++++++++++++-------------------
1 file changed, 171 insertions(+), 123 deletions(-)
---
base-commit: cd9fd78f5c11b5e165d9317ef11e613f4aef4dd1
change-id: 20221011-gpiolib-quirks-d452ed31d24e
Best regards,
Dmitry
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 <[email protected]>
---
drivers/gpio/gpiolib-of.c | 176 +++++++++++++++++-----------------------------
1 file changed, 64 insertions(+), 112 deletions(-)
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
};
--
b4 0.11.0-dev-5166b
Bindings for Freescale Fast Ethernet Controller use a separate
property "phy-reset-active-high" to specify polarity of its phy
gpio line. To allow converting the driver to gpiod API we need
to add this quirk to gpiolib.
Signed-off-by: Dmitry Torokhov <[email protected]>
---
drivers/gpio/gpiolib-of.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 3200d705fbe3..c3d3fe4d927c 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -231,6 +231,33 @@ static void of_gpio_flags_quirks(const struct device_node *np,
!strcmp(propname, "snps,reset-gpio") &&
of_property_read_bool(np, "snps,reset-active-low"))
*flags |= OF_GPIO_ACTIVE_LOW;
+
+ /*
+ * Freescale Fast Ethernet Controller uses a separate property to
+ * describe polarity of the phy reset line.
+ */
+ if (IS_ENABLED(CONFIG_FEC)) {
+ static const char * const fec_devices[] = {
+ "fsl,imx25-fec",
+ "fsl,imx27-fec",
+ "fsl,imx28-fec",
+ "fsl,imx6q-fec",
+ "fsl,mvf600-fec",
+ "fsl,imx6sx-fec",
+ "fsl,imx6ul-fec",
+ "fsl,imx6mq-fec",
+ "fsl,imx6qm-fec",
+ "fsl,s32v234-fec",
+ NULL
+ };
+
+ if (!strcmp(propname, "phy-reset-gpios") &&
+ of_device_compatible_match(np, fec_devices)) {
+ bool active_high = of_property_read_bool(np,
+ "phy-reset-active-high");
+ of_gpio_quirk_polarity(np, active_high, flags);
+ }
+ }
}
/**
--
b4 0.11.0-dev-5166b
The controller is using non-standard "cirrus,gpio-nreset" name for its
reset gpio property, whereas gpiod API expects "<name>-gpios".
Add a quirk so that gpiod API will still work on unmodified DTSes.
Signed-off-by: Dmitry Torokhov <[email protected]>
---
drivers/gpio/gpiolib-of.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 7d4193fe36e5..953d1c23950a 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -407,6 +407,9 @@ static struct gpio_desc *of_find_gpio_rename(struct device_node *np,
{ "wlf,ldo1ena", NULL, NULL }, /* WM8994 */
{ "wlf,ldo2ena", NULL, NULL }, /* WM8994 */
#endif
+#if IS_ENABLED(CONFIG_SND_SOC_CS42L56)
+ { "reset", "cirrus,gpio-nreset", "cirrus,cs42l56" },
+#endif
#if IS_ENABLED(CONFIG_SND_SOC_TLV320AIC3X)
{ "reset", "gpio-reset", "ti,tlv320aic3x" },
{ "reset", "gpio-reset", "ti,tlv320aic33" },
--
b4 0.11.0-dev-5166b
Am Mittwoch, 12. Oktober 2022, 00:19:35 CEST schrieb Dmitry Torokhov:
> Bindings for Freescale Fast Ethernet Controller use a separate
> property "phy-reset-active-high" to specify polarity of its phy
> gpio line. To allow converting the driver to gpiod API we need
> to add this quirk to gpiolib.
>
> Signed-off-by: Dmitry Torokhov <[email protected]>
> ---
> drivers/gpio/gpiolib-of.c | 27 +++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 3200d705fbe3..c3d3fe4d927c 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -231,6 +231,33 @@ static void of_gpio_flags_quirks(const struct
> device_node *np, !strcmp(propname, "snps,reset-gpio") &&
> of_property_read_bool(np, "snps,reset-active-low"))
> *flags |= OF_GPIO_ACTIVE_LOW;
> +
> + /*
> + * Freescale Fast Ethernet Controller uses a separate property to
> + * describe polarity of the phy reset line.
> + */
> + if (IS_ENABLED(CONFIG_FEC)) {
> + static const char * const fec_devices[] = {
> + "fsl,imx25-fec",
> + "fsl,imx27-fec",
> + "fsl,imx28-fec",
> + "fsl,imx6q-fec",
> + "fsl,mvf600-fec",
> + "fsl,imx6sx-fec",
> + "fsl,imx6ul-fec",
> + "fsl,imx6mq-fec",
> + "fsl,imx6qm-fec",
These two should be 'fsl,imx8mq-fec' & 'fsl,imx8qm-fec' (imx8 instead of
imx6).
Best regards,
Alexander
> + "fsl,s32v234-fec",
> + NULL
> + };
> +
> + if (!strcmp(propname, "phy-reset-gpios") &&
> + of_device_compatible_match(np, fec_devices)) {
> + bool active_high = of_property_read_bool(np,
> + "phy-reset-
active-high");
> + of_gpio_quirk_polarity(np, active_high,
flags);
> + }
> + }
> }
>
> /**
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 <[email protected]>
> ---
> 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 <[email protected]>
> 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 Tue, Oct 11, 2022 at 03:19:33PM -0700, Dmitry Torokhov wrote:
> The controller is using non-standard "cirrus,gpio-nreset" name for its
> reset gpio property, whereas gpiod API expects "<name>-gpios".
> Add a quirk so that gpiod API will still work on unmodified DTSes.
>
> Signed-off-by: Dmitry Torokhov <[email protected]>
> ---
> drivers/gpio/gpiolib-of.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 7d4193fe36e5..953d1c23950a 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -407,6 +407,9 @@ static struct gpio_desc *of_find_gpio_rename(struct device_node *np,
> { "wlf,ldo1ena", NULL, NULL }, /* WM8994 */
> { "wlf,ldo2ena", NULL, NULL }, /* WM8994 */
> #endif
> +#if IS_ENABLED(CONFIG_SND_SOC_CS42L56)
> + { "reset", "cirrus,gpio-nreset", "cirrus,cs42l56" },
> +#endif
Same question as before about bindings maintainance but other than that:
Reviewed-by: Daniel Thompson <[email protected]>
Daniel.
On October 11, 2022 11:14:46 PM PDT, Alexander Stein <[email protected]> wrote:
>Am Mittwoch, 12. Oktober 2022, 00:19:35 CEST schrieb Dmitry Torokhov:
>> Bindings for Freescale Fast Ethernet Controller use a separate
>> property "phy-reset-active-high" to specify polarity of its phy
>> gpio line. To allow converting the driver to gpiod API we need
>> to add this quirk to gpiolib.
>>
>> Signed-off-by: Dmitry Torokhov <[email protected]>
>> ---
>> drivers/gpio/gpiolib-of.c | 27 +++++++++++++++++++++++++++
>> 1 file changed, 27 insertions(+)
>>
>> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
>> index 3200d705fbe3..c3d3fe4d927c 100644
>> --- a/drivers/gpio/gpiolib-of.c
>> +++ b/drivers/gpio/gpiolib-of.c
>> @@ -231,6 +231,33 @@ static void of_gpio_flags_quirks(const struct
>> device_node *np, !strcmp(propname, "snps,reset-gpio") &&
>> of_property_read_bool(np, "snps,reset-active-low"))
>> *flags |= OF_GPIO_ACTIVE_LOW;
>> +
>> + /*
>> + * Freescale Fast Ethernet Controller uses a separate property to
>> + * describe polarity of the phy reset line.
>> + */
>> + if (IS_ENABLED(CONFIG_FEC)) {
>> + static const char * const fec_devices[] = {
>> + "fsl,imx25-fec",
>> + "fsl,imx27-fec",
>> + "fsl,imx28-fec",
>> + "fsl,imx6q-fec",
>> + "fsl,mvf600-fec",
>> + "fsl,imx6sx-fec",
>> + "fsl,imx6ul-fec",
>
>> + "fsl,imx6mq-fec",
>> + "fsl,imx6qm-fec",
>
>These two should be 'fsl,imx8mq-fec' & 'fsl,imx8qm-fec' (imx8 instead of
>imx6).
Thank you for noticing this. I'll fix it up in the next version.
Thanks.
--
Dmitry
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 <[email protected]>
> > ---
> > 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 <[email protected]>
>
>
> > 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.
--
Dmitry
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.