[PATCH 0/7] gpiolib: more quirks to handle legacy names

[PATCH 0/7] gpiolib: more quirks to handle legacy names

[Dmitry Torokhov]

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 <linus.walleij@linaro.org>
To: Bartosz Golaszewski <brgl@bgdev.pl>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Daniel Thompson <daniel.thompson@linaro.org>
Cc: linux-gpio@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-mediatek@lists.infradead.org

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

[PATCH 1/7] gpiolib: of: add a quirk for legacy names in Mediatek mt2701-cs42448

[Dmitry Torokhov]

The driver is using non-standard "i2s1-in-sel-gpio1" and
"i2s1-in-sel-gpio2" names to describe its gpios. In preparation to
converting to the standard naming (i2s1-in-sel-gpios) and switching the
driver to gpiod API add a quirk to gpiolib to keep compatibility with
existing DTSes.

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

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 0e4e1291604d..cef4f6634125 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -488,6 +488,38 @@ static struct gpio_desc *of_find_usb_gpio(struct device_node *np,
 	return of_get_named_gpiod_flags(np, con_id, idx, of_flags);
 }
 
+static struct gpio_desc *of_find_mt2701_gpio(struct device_node *np,
+					     const char *con_id,
+					     unsigned int idx,
+					     enum of_gpio_flags *of_flags)
+{
+	struct gpio_desc *desc;
+	const char *legacy_id;
+
+	if (!IS_ENABLED(CONFIG_SND_SOC_MT2701_CS42448))
+		return ERR_PTR(-ENOENT);
+
+	if (!of_device_is_compatible(np, "mediatek,mt2701-cs42448-machine"))
+		return ERR_PTR(-ENOENT);
+
+	if (!con_id || strcmp(con_id, "i2s1-in-sel"))
+		return ERR_PTR(-ENOENT);
+
+	if (idx == 0)
+		legacy_id = "i2s1-in-sel-gpio1";
+	else if (idx == 1)
+		legacy_id = "i2s1-in-sel-gpio2";
+	else
+		return ERR_PTR(-ENOENT);
+
+	desc = of_get_named_gpiod_flags(np, legacy_id, 0, of_flags);
+	if (!gpiod_not_found(desc))
+		pr_info("%s is using legacy gpio name '%s' instead of '%s-gpios'\n",
+			of_node_full_name(np), legacy_id, con_id);
+
+	return desc;
+}
+
 typedef struct gpio_desc *(*of_find_gpio_quirk)(struct device_node *np,
 						const char *con_id,
 						unsigned int idx,
@@ -498,6 +530,7 @@ static const of_find_gpio_quirk of_find_gpio_quirks[] = {
 	of_find_regulator_gpio,
 	of_find_arizona_gpio,
 	of_find_usb_gpio,
+	of_find_mt2701_gpio,
 	NULL
 };
 

-- 
b4 0.11.0-dev-5166b

Re: [PATCH 1/7] gpiolib: of: add a quirk for legacy names in Mediatek mt2701-cs42448

[Daniel Thompson]

On Tue, Oct 11, 2022 at 03:19:29PM -0700, Dmitry Torokhov wrote:
> The driver is using non-standard "i2s1-in-sel-gpio1" and
> "i2s1-in-sel-gpio2" names to describe its gpios. In preparation to
> converting to the standard naming (i2s1-in-sel-gpios) and switching the
> driver to gpiod API add a quirk to gpiolib to keep compatibility with
> existing DTSes.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

I'm just passing through since I was curious about this 'set but FWIW:
Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>


Daniel.

Re: [PATCH 1/7] gpiolib: of: add a quirk for legacy names in Mediatek mt2701-cs42448

[Linus Walleij]

On Wed, Oct 12, 2022 at 12:19 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:

> The driver is using non-standard "i2s1-in-sel-gpio1" and
> "i2s1-in-sel-gpio2" names to describe its gpios. In preparation to
> converting to the standard naming (i2s1-in-sel-gpios) and switching the
> driver to gpiod API add a quirk to gpiolib to keep compatibility with
> existing DTSes.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours.
Linus Walleij

[PATCH 2/7] gpiolib: of: consolidate simple renames into a single quirk

[Dmitry Torokhov]

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

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

Re: [PATCH 2/7] gpiolib: of: consolidate simple renames into a single quirk

[Daniel Thompson]

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.

Re: [PATCH 2/7] gpiolib: of: consolidate simple renames into a single quirk

[Dmitry Torokhov]

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.

-- 
Dmitry

Re: [PATCH 2/7] gpiolib: of: consolidate simple renames into a single quirk

[Daniel Thompson]

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.

[PATCH 3/7] gpiolib: of: add quirk for locating reset lines with legacy bindings

[Dmitry Torokhov]

Some legacy mappings used "gpio[s]-reset" instead of "reset-gpios",
add a quirk so that gpiod API will still work on unmodified DTSes.

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

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 619aae0c5476..576f2f0c3432 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -375,9 +375,18 @@ static struct gpio_desc *of_find_gpio_rename(struct device_node *np,
 		const char *legacy_id;	/* NULL - same as con_id */
 		const char *compatible; /* NULL - don't check */
 	} gpios[] = {
+#if !IS_ENABLED(CONFIG_LCD_HX8357)
+		/* Himax LCD controllers used "gpios-reset" */
+		{ "reset",	"gpios-reset",	"himax,hx8357" },
+		{ "reset",	"gpios-reset",	"himax,hx8369" },
+#endif
 #if IS_ENABLED(CONFIG_MFD_ARIZONA)
 		{ "wlf,reset",	NULL,		NULL },
 #endif
+#if !IS_ENABLED(CONFIG_PCI_LANTIQ)
+		/* MIPS Lantiq PCI */
+		{ "reset",	"gpios-reset",	"lantiq,pci-xway" },
+#endif
 #if IS_ENABLED(CONFIG_REGULATOR)
 		/*
 		 * Some regulator bindings happened before we managed to
@@ -388,8 +397,14 @@ 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_TLV320AIC3X)
+		{ "reset",	"gpio-reset",	"ti,tlv320aic3x" },
+		{ "reset",	"gpio-reset",	"ti,tlv320aic33" },
+		{ "reset",	"gpio-reset",	"ti,tlv320aic3007" },
+		{ "reset",	"gpio-reset",	"ti,tlv320aic3104" },
+		{ "reset",	"gpio-reset",	"ti,tlv320aic3106" },
+#endif
 #if IS_ENABLED(CONFIG_SPI_MASTER)
-
 		/*
 		 * The SPI GPIO bindings happened before we managed to
 		 * establish that GPIO properties should be named

-- 
b4 0.11.0-dev-5166b

Re: [PATCH 3/7] gpiolib: of: add quirk for locating reset lines with legacy bindings

[Daniel Thompson]

On Tue, Oct 11, 2022 at 03:19:31PM -0700, Dmitry Torokhov wrote:
> Some legacy mappings used "gpio[s]-reset" instead of "reset-gpios",
> add a quirk so that gpiod API will still work on unmodified DTSes.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>


Daniel.

[PATCH 4/7] gpiolib: of: add a quirk for reset line for Marvell NFC controller

[Dmitry Torokhov]

The controller is using non-standard "reset-n-io" 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 <dmitry.torokhov@gmail.com>
---
 drivers/gpio/gpiolib-of.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 576f2f0c3432..7d4193fe36e5 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -383,6 +383,16 @@ static struct gpio_desc *of_find_gpio_rename(struct device_node *np,
 #if IS_ENABLED(CONFIG_MFD_ARIZONA)
 		{ "wlf,reset",	NULL,		NULL },
 #endif
+
+#if IS_ENABLED(CONFIG_NFC_MRVL_I2C)
+		{ "reset",	"reset-n-io",	"marvell,nfc-i2c" },
+#endif
+#if IS_ENABLED(CONFIG_NFC_MRVL_SPI)
+		{ "reset",	"reset-n-io",	"marvell,nfc-spi" },
+#endif
+#if IS_ENABLED(CONFIG_NFC_MRVL_UART)
+		{ "reset",	"reset-n-io",	"marvell,nfc-uart" },
+#endif
 #if !IS_ENABLED(CONFIG_PCI_LANTIQ)
 		/* MIPS Lantiq PCI */
 		{ "reset",	"gpios-reset",	"lantiq,pci-xway" },

-- 
b4 0.11.0-dev-5166b

Re: [PATCH 4/7] gpiolib: of: add a quirk for reset line for Marvell NFC controller

[Daniel Thompson]

On Tue, Oct 11, 2022 at 03:19:32PM -0700, Dmitry Torokhov wrote:
> The controller is using non-standard "reset-n-io" 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 <dmitry.torokhov@gmail.com>

How/when has/will the DT bindings documentation for this hardware be
updated to describe the new bindings?

Delivering the quirks ahead of driver updates is great for avoiding
merge conflicts but it also conceals the rename from reviewers so
risks neglecting to update the bindings.

Other than that:
Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>


Daniel.

> ---
>  drivers/gpio/gpiolib-of.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 576f2f0c3432..7d4193fe36e5 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -383,6 +383,16 @@ static struct gpio_desc *of_find_gpio_rename(struct device_node *np,
>  #if IS_ENABLED(CONFIG_MFD_ARIZONA)
>  		{ "wlf,reset",	NULL,		NULL },
>  #endif
> +
> +#if IS_ENABLED(CONFIG_NFC_MRVL_I2C)
> +		{ "reset",	"reset-n-io",	"marvell,nfc-i2c" },
> +#endif
> +#if IS_ENABLED(CONFIG_NFC_MRVL_SPI)
> +		{ "reset",	"reset-n-io",	"marvell,nfc-spi" },
> +#endif
> +#if IS_ENABLED(CONFIG_NFC_MRVL_UART)
> +		{ "reset",	"reset-n-io",	"marvell,nfc-uart" },
> +#endif
>  #if !IS_ENABLED(CONFIG_PCI_LANTIQ)
>  		/* MIPS Lantiq PCI */
>  		{ "reset",	"gpios-reset",	"lantiq,pci-xway" },
>
> --
> b4 0.11.0-dev-5166b

Re: [PATCH 4/7] gpiolib: of: add a quirk for reset line for Marvell NFC controller

[Dmitry Torokhov]

On Wed, Oct 12, 2022 at 11:29:02AM +0100, Daniel Thompson wrote:
> On Tue, Oct 11, 2022 at 03:19:32PM -0700, Dmitry Torokhov wrote:
> > The controller is using non-standard "reset-n-io" 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 <dmitry.torokhov@gmail.com>
> 
> How/when has/will the DT bindings documentation for this hardware be
> updated to describe the new bindings?
> 
> Delivering the quirks ahead of driver updates is great for avoiding
> merge conflicts but it also conceals the rename from reviewers so
> risks neglecting to update the bindings.

I was planning on sending binding updates once driver patches land.

> 
> Other than that:
> Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>

Thank you for your reviews!

-- 
Dmitry

Re: [PATCH 4/7] gpiolib: of: add a quirk for reset line for Marvell NFC controller

[Daniel Thompson]

On Wed, Oct 12, 2022 at 11:45:02AM -0700, Dmitry Torokhov wrote:
> On Wed, Oct 12, 2022 at 11:29:02AM +0100, Daniel Thompson wrote:
> > On Tue, Oct 11, 2022 at 03:19:32PM -0700, Dmitry Torokhov wrote:
> > > The controller is using non-standard "reset-n-io" 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 <dmitry.torokhov@gmail.com>
> >
> > How/when has/will the DT bindings documentation for this hardware be
> > updated to describe the new bindings?
> >
> > Delivering the quirks ahead of driver updates is great for avoiding
> > merge conflicts but it also conceals the rename from reviewers so
> > risks neglecting to update the bindings.
>
> I was planning on sending binding updates once driver patches land.

I'd have a (weak) preference for them being shared in the same patchset.
Maintainers can either ack or the changes can land seperately but
having them in the same patchset helps avoid having to quibble or check!


Daniel.

Re: [PATCH 4/7] gpiolib: of: add a quirk for reset line for Marvell NFC controller

[Dmitry Torokhov]

On Wed, Oct 12, 2022 at 07:50:49PM +0100, Daniel Thompson wrote:
> On Wed, Oct 12, 2022 at 11:45:02AM -0700, Dmitry Torokhov wrote:
> > On Wed, Oct 12, 2022 at 11:29:02AM +0100, Daniel Thompson wrote:
> > > On Tue, Oct 11, 2022 at 03:19:32PM -0700, Dmitry Torokhov wrote:
> > > > The controller is using non-standard "reset-n-io" 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 <dmitry.torokhov@gmail.com>
> > >
> > > How/when has/will the DT bindings documentation for this hardware be
> > > updated to describe the new bindings?
> > >
> > > Delivering the quirks ahead of driver updates is great for avoiding
> > > merge conflicts but it also conceals the rename from reviewers so
> > > risks neglecting to update the bindings.
> >
> > I was planning on sending binding updates once driver patches land.
> 
> I'd have a (weak) preference for them being shared in the same patchset.
> Maintainers can either ack or the changes can land seperately but
> having them in the same patchset helps avoid having to quibble or check!

OK, so how about once we agree and land this patchset to gpiolib I can
blast driver patches + binding patches together?

Thanks.

-- 
Dmitry

Re: [PATCH 4/7] gpiolib: of: add a quirk for reset line for Marvell NFC controller

[Daniel Thompson]

On Wed, Oct 12, 2022 at 11:55:36AM -0700, Dmitry Torokhov wrote:
> On Wed, Oct 12, 2022 at 07:50:49PM +0100, Daniel Thompson wrote:
> > On Wed, Oct 12, 2022 at 11:45:02AM -0700, Dmitry Torokhov wrote:
> > > On Wed, Oct 12, 2022 at 11:29:02AM +0100, Daniel Thompson wrote:
> > > > On Tue, Oct 11, 2022 at 03:19:32PM -0700, Dmitry Torokhov wrote:
> > > > > The controller is using non-standard "reset-n-io" 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 <dmitry.torokhov@gmail.com>
> > > >
> > > > How/when has/will the DT bindings documentation for this hardware be
> > > > updated to describe the new bindings?
> > > >
> > > > Delivering the quirks ahead of driver updates is great for avoiding
> > > > merge conflicts but it also conceals the rename from reviewers so
> > > > risks neglecting to update the bindings.
> > >
> > > I was planning on sending binding updates once driver patches land.
> >
> > I'd have a (weak) preference for them being shared in the same patchset.
> > Maintainers can either ack or the changes can land seperately but
> > having them in the same patchset helps avoid having to quibble or check!
>
> OK, so how about once we agree and land this patchset to gpiolib I can
> blast driver patches + binding patches together?

That's good for me!


Daniel.

[PATCH 5/7] gpiolib: of: add a quirk for reset line for Cirrus CS42L56 codec

[Dmitry Torokhov]

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 <dmitry.torokhov@gmail.com>
---
 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

Re: [PATCH 5/7] gpiolib: of: add a quirk for reset line for Cirrus CS42L56 codec

[Daniel Thompson]

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 <dmitry.torokhov@gmail.com>
> ---
>  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 <daniel.thompson@linaro.org>


Daniel.

[PATCH 6/7] gpiolib: of: factor out code overriding gpio line polarity

[Dmitry Torokhov]

There are several instances where we use a separate property to
override polarity specified in gpio property. Factor it out into
a separate function.

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

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 953d1c23950a..3200d705fbe3 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -130,6 +130,28 @@ bool of_gpio_need_valid_mask(const struct gpio_chip *gc)
 	return false;
 }
 
+/*
+ * Overrides stated polarity of a gpio line and warns when there is a
+ * discrepancy.
+ */
+static void of_gpio_quirk_polarity(const struct device_node *np,
+				   bool active_high,
+				   enum of_gpio_flags *flags)
+{
+	if (active_high) {
+		if (*flags & OF_GPIO_ACTIVE_LOW) {
+			pr_warn("%s GPIO handle specifies active low - ignored\n",
+				of_node_full_name(np));
+			*flags &= ~OF_GPIO_ACTIVE_LOW;
+		}
+	} else {
+		if (!(*flags & OF_GPIO_ACTIVE_LOW))
+			pr_info("%s enforce active low on GPIO handle\n",
+				of_node_full_name(np));
+		*flags |= OF_GPIO_ACTIVE_LOW;
+	}
+}
+
 static void of_gpio_flags_quirks(const struct device_node *np,
 				 const char *propname,
 				 enum of_gpio_flags *flags,
@@ -145,7 +167,7 @@ static void of_gpio_flags_quirks(const struct device_node *np,
 	     (!(strcmp(propname, "enable-gpio") &&
 		strcmp(propname, "enable-gpios")) &&
 	      of_device_is_compatible(np, "regulator-gpio")))) {
-		bool active_low = !of_property_read_bool(np,
+		bool active_high = of_property_read_bool(np,
 							 "enable-active-high");
 		/*
 		 * The regulator GPIO handles are specified such that the
@@ -153,13 +175,7 @@ static void of_gpio_flags_quirks(const struct device_node *np,
 		 * the polarity of the GPIO line. Any phandle flags must
 		 * be actively ignored.
 		 */
-		if ((*flags & OF_GPIO_ACTIVE_LOW) && !active_low) {
-			pr_warn("%s GPIO handle specifies active low - ignored\n",
-				of_node_full_name(np));
-			*flags &= ~OF_GPIO_ACTIVE_LOW;
-		}
-		if (active_low)
-			*flags |= OF_GPIO_ACTIVE_LOW;
+		of_gpio_quirk_polarity(np, active_high, flags);
 	}
 	/*
 	 * Legacy open drain handling for fixed voltage regulators.
@@ -200,18 +216,10 @@ static void of_gpio_flags_quirks(const struct device_node *np,
 				 * conflict and the "spi-cs-high" flag will
 				 * take precedence.
 				 */
-				if (of_property_read_bool(child, "spi-cs-high")) {
-					if (*flags & OF_GPIO_ACTIVE_LOW) {
-						pr_warn("%s GPIO handle specifies active low - ignored\n",
-							of_node_full_name(child));
-						*flags &= ~OF_GPIO_ACTIVE_LOW;
-					}
-				} else {
-					if (!(*flags & OF_GPIO_ACTIVE_LOW))
-						pr_info("%s enforce active low on chipselect handle\n",
-							of_node_full_name(child));
-					*flags |= OF_GPIO_ACTIVE_LOW;
-				}
+				bool active_high = of_property_read_bool(child,
+								"spi-cs-high");
+				of_gpio_quirk_polarity(child, active_high,
+						       flags);
 				of_node_put(child);
 				break;
 			}

-- 
b4 0.11.0-dev-5166b

Re: [PATCH 6/7] gpiolib: of: factor out code overriding gpio line polarity

[Andy Shevchenko]

On Tue, Oct 11, 2022 at 03:19:34PM -0700, Dmitry Torokhov wrote:
> There are several instances where we use a separate property to
> override polarity specified in gpio property. Factor it out into
> a separate function.

...

>  static void of_gpio_flags_quirks(const struct device_node *np,
>  				 const char *propname,
>  				 enum of_gpio_flags *flags,
> @@ -145,7 +167,7 @@ static void of_gpio_flags_quirks(const struct device_node *np,
>  	     (!(strcmp(propname, "enable-gpio") &&
>  		strcmp(propname, "enable-gpios")) &&
>  	      of_device_is_compatible(np, "regulator-gpio")))) {
> -		bool active_low = !of_property_read_bool(np,
> +		bool active_high = of_property_read_bool(np,

Defining

	bool active_high;

at the top of the function will save you a few ugly indented code lines here
and in the next patch.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 6/7] gpiolib: of: factor out code overriding gpio line polarity

[Dmitry Torokhov]

On October 12, 2022 4:10:37 AM PDT, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>On Tue, Oct 11, 2022 at 03:19:34PM -0700, Dmitry Torokhov wrote:
>> There are several instances where we use a separate property to
>> override polarity specified in gpio property. Factor it out into
>> a separate function.
>
>...
>
>>  static void of_gpio_flags_quirks(const struct device_node *np,
>>  				 const char *propname,
>>  				 enum of_gpio_flags *flags,
>> @@ -145,7 +167,7 @@ static void of_gpio_flags_quirks(const struct device_node *np,
>>  	     (!(strcmp(propname, "enable-gpio") &&
>>  		strcmp(propname, "enable-gpios")) &&
>>  	      of_device_is_compatible(np, "regulator-gpio")))) {
>> -		bool active_low = !of_property_read_bool(np,
>> +		bool active_high = of_property_read_bool(np,
>
>Defining
>
>	bool active_high;
>
>at the top of the function will save you a few ugly indented code lines here
>and in the next patch.
>

Indeed, but I think the idea was to keep the quirks as independent as possible.
I guess I could split it further into separate functions, like the other quirk handling. 

Bartosz, Linus, any preference here? 

Thanks. 

-- 
Dmitry

[PATCH 7/7] gpiolib: of: add quirk for phy reset polarity for Freescale Ethernet

[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 <dmitry.torokhov@gmail.com>
---
 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

Re: [PATCH 7/7] gpiolib: of: add quirk for phy reset polarity for Freescale Ethernet

[Alexander Stein]

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 <dmitry.torokhov@gmail.com>
> ---
>  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);
> +		}
> +	}
>  }
> 
>  /**

Re: [PATCH 7/7] gpiolib: of: add quirk for phy reset polarity for Freescale Ethernet

[Dmitry Torokhov]

On October 11, 2022 11:14:46 PM PDT, Alexander Stein <alexander.stein@ew.tq-group.com> 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 <dmitry.torokhov@gmail.com>
>> ---
>>  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