Replace the legacy of_get_named_gpio() / gpio_request_one() /
gpio_set_value() API with the descriptor-based devm_gpiod_get_optional() /
gpiod_set_value() API from <linux/gpio/consumer.h>, removing the
dependency on <linux/of_gpio.h>.

The "reset-n-io" property rename quirk already exists in gpiolib-of.c
(added in commit 9c2cc7171e08), so no additional quirk is needed.

Signed-off-by: Jialu Xu <xujialu@vimux.org>
Reviewed-by: Linus Walleij <linusw@kernel.org>
---
 drivers/nfc/nfcmrvl/main.c    | 46 ++++++++++++-----------------------
 drivers/nfc/nfcmrvl/nfcmrvl.h |  4 ++-
 drivers/nfc/nfcmrvl/uart.c    | 23 ++++++++++++------
 drivers/nfc/nfcmrvl/usb.c     |  2 +-
 4 files changed, 35 insertions(+), 40 deletions(-)

diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c
index c51d22e4579c1..46cc1386ad3bc 100644
--- a/drivers/nfc/nfcmrvl/main.c
+++ b/drivers/nfc/nfcmrvl/main.c
@@ -6,9 +6,9 @@
  */
 
 #include <linux/module.h>
-#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/delay.h>
-#include <linux/of_gpio.h>
+#include <linux/of.h>
 #include <linux/nfc.h>
 #include <net/nfc/nci.h>
 #include <net/nfc/nci_core.h>
@@ -112,13 +112,12 @@ struct nfcmrvl_private *nfcmrvl_nci_register_dev(enum nfcmrvl_phy phy,
 
 	memcpy(&priv->config, pdata, sizeof(*pdata));
 
-	if (gpio_is_valid(priv->config.reset_n_io)) {
-		rc = gpio_request_one(priv->config.reset_n_io,
-				      GPIOF_OUT_INIT_LOW,
-				      "nfcmrvl_reset_n");
-		if (rc < 0) {
-			priv->config.reset_n_io = -EINVAL;
-			nfc_err(dev, "failed to request reset_n io\n");
+	if (!priv->config.reset_gpio) {
+		priv->config.reset_gpio =
+			devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
+		if (IS_ERR(priv->config.reset_gpio)) {
+			priv->config.reset_gpio = NULL;
+			nfc_err(dev, "failed to get reset gpio\n");
 		}
 	}
 
@@ -144,7 +143,7 @@ struct nfcmrvl_private *nfcmrvl_nci_register_dev(enum nfcmrvl_phy phy,
 	if (!priv->ndev) {
 		nfc_err(dev, "nci_allocate_device failed\n");
 		rc = -ENOMEM;
-		goto error_free_gpio;
+		goto error_free;
 	}
 
 	rc = nfcmrvl_fw_dnld_init(priv);
@@ -171,9 +170,7 @@ struct nfcmrvl_private *nfcmrvl_nci_register_dev(enum nfcmrvl_phy phy,
 	nfcmrvl_fw_dnld_deinit(priv);
 error_free_dev:
 	nci_free_device(priv->ndev);
-error_free_gpio:
-	if (gpio_is_valid(priv->config.reset_n_io))
-		gpio_free(priv->config.reset_n_io);
+error_free:
 	kfree(priv);
 	return ERR_PTR(rc);
 }
@@ -189,9 +186,6 @@ void nfcmrvl_nci_unregister_dev(struct nfcmrvl_private *priv)
 
 	nfcmrvl_fw_dnld_deinit(priv);
 
-	if (gpio_is_valid(priv->config.reset_n_io))
-		gpio_free(priv->config.reset_n_io);
-
 	nci_free_device(ndev);
 	kfree(priv);
 }
@@ -233,34 +227,24 @@ void nfcmrvl_chip_reset(struct nfcmrvl_private *priv)
 	/* Reset possible fault of previous session */
 	clear_bit(NFCMRVL_PHY_ERROR, &priv->flags);
 
-	if (gpio_is_valid(priv->config.reset_n_io)) {
+	if (priv->config.reset_gpio) {
 		nfc_info(priv->dev, "reset the chip\n");
-		gpio_set_value(priv->config.reset_n_io, 0);
+		gpiod_set_value(priv->config.reset_gpio, 0);
 		usleep_range(5000, 10000);
-		gpio_set_value(priv->config.reset_n_io, 1);
+		gpiod_set_value(priv->config.reset_gpio, 1);
 	} else
 		nfc_info(priv->dev, "no reset available on this interface\n");
 }
 
 void nfcmrvl_chip_halt(struct nfcmrvl_private *priv)
 {
-	if (gpio_is_valid(priv->config.reset_n_io))
-		gpio_set_value(priv->config.reset_n_io, 0);
+	if (priv->config.reset_gpio)
+		gpiod_set_value(priv->config.reset_gpio, 0);
 }
 
 int nfcmrvl_parse_dt(struct device_node *node,
 		     struct nfcmrvl_platform_data *pdata)
 {
-	int reset_n_io;
-
-	reset_n_io = of_get_named_gpio(node, "reset-n-io", 0);
-	if (reset_n_io < 0) {
-		pr_info("no reset-n-io config\n");
-	} else if (!gpio_is_valid(reset_n_io)) {
-		pr_err("invalid reset-n-io GPIO\n");
-		return reset_n_io;
-	}
-	pdata->reset_n_io = reset_n_io;
 	pdata->hci_muxed = of_property_read_bool(node, "hci-muxed");
 
 	return 0;
diff --git a/drivers/nfc/nfcmrvl/nfcmrvl.h b/drivers/nfc/nfcmrvl/nfcmrvl.h
index f61a99e553db0..62fa77d587b2b 100644
--- a/drivers/nfc/nfcmrvl/nfcmrvl.h
+++ b/drivers/nfc/nfcmrvl/nfcmrvl.h
@@ -10,6 +10,8 @@
 
 #include "fw_dnld.h"
 
+struct gpio_desc;
+
 /* Define private flags: */
 #define NFCMRVL_NCI_RUNNING			1
 #define NFCMRVL_PHY_ERROR			2
@@ -54,7 +56,7 @@ struct nfcmrvl_platform_data {
 	 */
 
 	/* GPIO that is wired to RESET_N signal */
-	int reset_n_io;
+	struct gpio_desc *reset_gpio;
 	/* Tell if transport is muxed in HCI one */
 	bool hci_muxed;
 
diff --git a/drivers/nfc/nfcmrvl/uart.c b/drivers/nfc/nfcmrvl/uart.c
index 2037cd6d4f4f4..6ea28340bb58e 100644
--- a/drivers/nfc/nfcmrvl/uart.c
+++ b/drivers/nfc/nfcmrvl/uart.c
@@ -8,6 +8,7 @@
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/err.h>
+#include <linux/gpio/consumer.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/printk.h>
@@ -20,7 +21,6 @@
 static unsigned int hci_muxed;
 static unsigned int flow_control;
 static unsigned int break_control;
-static int reset_n_io = -EINVAL;
 
 /*
  * NFCMRVL NCI OPS
@@ -62,9 +62,11 @@ static const struct nfcmrvl_if_ops uart_ops = {
 };
 
 static int nfcmrvl_uart_parse_dt(struct device_node *node,
-				 struct nfcmrvl_platform_data *pdata)
+				 struct nfcmrvl_platform_data *pdata,
+				 struct device *dev)
 {
 	struct device_node *matched_node;
+	struct gpio_desc *reset_gpio;
 	int ret;
 
 	matched_node = of_get_compatible_child(node, "marvell,nfc-uart");
@@ -84,6 +86,16 @@ static int nfcmrvl_uart_parse_dt(struct device_node *node,
 	pdata->flow_control = of_property_read_bool(matched_node, "flow-control");
 	pdata->break_control = of_property_read_bool(matched_node, "break-control");
 
+	reset_gpio = devm_fwnode_gpiod_get_optional(dev,
+					of_fwnode_handle(matched_node),
+					"reset", GPIOD_OUT_LOW,
+					"nfcmrvl_reset_n");
+	if (IS_ERR(reset_gpio)) {
+		of_node_put(matched_node);
+		return PTR_ERR(reset_gpio);
+	}
+	pdata->reset_gpio = reset_gpio;
+
 	of_node_put(matched_node);
 
 	return 0;
@@ -107,13 +119,13 @@ static int nfcmrvl_nci_uart_open(struct nci_uart *nu)
 	 */
 
 	if (dev && dev->parent && dev->parent->of_node)
-		if (nfcmrvl_uart_parse_dt(dev->parent->of_node, &config) == 0)
+		if (nfcmrvl_uart_parse_dt(dev->parent->of_node, &config, dev) == 0)
 			pdata = &config;
 
 	if (!pdata) {
 		pr_info("No platform data / DT -> fallback to module params\n");
 		config.hci_muxed = hci_muxed;
-		config.reset_n_io = reset_n_io;
+		config.reset_gpio = NULL;
 		config.flow_control = flow_control;
 		config.break_control = break_control;
 		pdata = &config;
@@ -201,6 +213,3 @@ MODULE_PARM_DESC(break_control, "Tell if UART driver must drive break signal.");
 
 module_param(hci_muxed, uint, 0);
 MODULE_PARM_DESC(hci_muxed, "Tell if transport is muxed in HCI one.");
-
-module_param(reset_n_io, int, 0);
-MODULE_PARM_DESC(reset_n_io, "GPIO that is wired to RESET_N signal.");
diff --git a/drivers/nfc/nfcmrvl/usb.c b/drivers/nfc/nfcmrvl/usb.c
index ea73094530968..ac62358445bf8 100644
--- a/drivers/nfc/nfcmrvl/usb.c
+++ b/drivers/nfc/nfcmrvl/usb.c
@@ -294,7 +294,7 @@ static int nfcmrvl_probe(struct usb_interface *intf,
 
 	/* No configuration for USB */
 	memset(&config, 0, sizeof(config));
-	config.reset_n_io = -EINVAL;
+	config.reset_gpio = NULL;
 
 	nfc_info(&udev->dev, "intf %p id %p\n", intf, id);
 
-- 
2.47.3

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
nfc: nfcmrvl: convert to gpio descriptors

This commit replaces the legacy GPIO API (of_get_named_gpio() /
gpio_request_one() / gpio_set_value()) with the descriptor-based API
(devm_gpiod_get_optional() / gpiod_set_value()) from
<linux/gpio/consumer.h>, removing the dependency on <linux/of_gpio.h>.

> diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c
> index c51d22e4579c..46cc1386ad3b 100644
> --- a/drivers/nfc/nfcmrvl/main.c
> +++ b/drivers/nfc/nfcmrvl/main.c

[ ... ]

> @@ -112,13 +112,12 @@ struct nfcmrvl_private *nfcmrvl_nci_register_dev(enum nfcmrvl_phy phy,
>
>  	memcpy(&priv->config, pdata, sizeof(*pdata));
>
> -	if (gpio_is_valid(priv->config.reset_n_io)) {
> -		rc = gpio_request_one(priv->config.reset_n_io,
> -				      GPIOF_OUT_INIT_LOW,
> -				      "nfcmrvl_reset_n");
> -		if (rc < 0) {
> -			priv->config.reset_n_io = -EINVAL;
> -			nfc_err(dev, "failed to request reset_n io\n");
> +	if (!priv->config.reset_gpio) {
> +		priv->config.reset_gpio =
> +			devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> +		if (IS_ERR(priv->config.reset_gpio)) {
> +			priv->config.reset_gpio = NULL;
> +			nfc_err(dev, "failed to get reset gpio\n");
>  		}
>  	}

Does the I2C and SPI paths properly initialize reset_gpio? Looking at the
I2C path (nfcmrvl_i2c_probe -> nfcmrvl_i2c_parse_dt), the function declares
'struct nfcmrvl_platform_data config' on the stack without zeroing and calls
nfcmrvl_parse_dt() which now only sets hci_muxed:

drivers/nfc/nfcmrvl/main.c:nfcmrvl_parse_dt() {
	pdata->hci_muxed = of_property_read_bool(node, "hci-muxed");
	return 0;
}

The reset_gpio field is left uninitialized with stack garbage. The same
issue affects SPI (nfcmrvl_spi_probe -> nfcmrvl_spi_parse_dt).

When the uninitialized config is passed to nfcmrvl_nci_register_dev(), the
check 'if (!priv->config.reset_gpio)' may evaluate to false (garbage
non-NULL pointer), causing the code to skip devm_gpiod_get_optional() and
later use the garbage pointer in gpiod_set_value():

nfcmrvl_chip_halt() {
	if (priv->config.reset_gpio)
		gpiod_set_value(priv->config.reset_gpio, 0);
}

Can this cause a kernel crash? The UART path was updated in this patch to
initialize the field properly, and the USB path already uses memset to zero
the config, so only I2C and SPI are affected.

[ ... ]

> @@ -233,34 +227,24 @@ void nfcmrvl_chip_reset(struct nfcmrvl_private *priv)
>  	/* Reset possible fault of previous session */
>  	clear_bit(NFCMRVL_PHY_ERROR, &priv->flags);
>
> -	if (gpio_is_valid(priv->config.reset_n_io)) {
> +	if (priv->config.reset_gpio) {
>  		nfc_info(priv->dev, "reset the chip\n");
> -		gpio_set_value(priv->config.reset_n_io, 0);
> +		gpiod_set_value(priv->config.reset_gpio, 0);
>  		usleep_range(5000, 10000);
> -		gpio_set_value(priv->config.reset_n_io, 1);
> +		gpiod_set_value(priv->config.reset_gpio, 1);
>  	} else
>  		nfc_info(priv->dev, "no reset available on this interface\n");
>  }
>
>  void nfcmrvl_chip_halt(struct nfcmrvl_private *priv)
>  {
> -	if (gpio_is_valid(priv->config.reset_n_io))
> -		gpio_set_value(priv->config.reset_n_io, 0);
> +	if (priv->config.reset_gpio)
> +		gpiod_set_value(priv->config.reset_gpio, 0);
>  }

Are the GPIO polarity values inverted here? The DT binding documentation
(Documentation/devicetree/bindings/net/nfc/marvell,nci.yaml) specifies
GPIO_ACTIVE_LOW for the reset-n-io property:

reset-n-io = <&gpio3 19 GPIO_ACTIVE_LOW>;

With the gpiod API, gpiod_set_value() respects the active-low flag:
  - gpiod_set_value(desc, 1) = assert (physically LOW for active-low)
  - gpiod_set_value(desc, 0) = deassert (physically HIGH for active-low)

The old gpio_set_value() ignored polarity flags and drove raw physical
levels directly.

The patch keeps the same raw values (0 then 1) but with gpiod_set_value()
this inverts the actual behavior:

nfcmrvl_chip_reset() now does:
  - gpiod_set_value(gpio, 0) = deassert reset (chip running)
  - usleep_range(5000, 10000)
  - gpiod_set_value(gpio, 1) = assert reset (chip in reset)

This is backwards - it leaves the chip stuck in reset after a "reset"
operation.

Similarly, nfcmrvl_chip_halt() calls gpiod_set_value(gpio, 0) which
deasserts reset (chip runs) instead of asserting it (chip halted).

Also, the GPIOD_OUT_LOW initial state in devm_gpiod_get_optional() means
"deasserted" (physically HIGH) rather than the old GPIOF_OUT_INIT_LOW
(physically LOW).

Should the values be inverted to work correctly with the descriptor API?

  - chip_reset: gpiod_set_value(gpio, 1) then gpiod_set_value(gpio, 0)
  - chip_halt: gpiod_set_value(gpio, 1)
  - initial state: GPIOD_OUT_HIGH
-- 
pw-bot: cr